feat: add scale-only screenshot sizing - #1617
Conversation
33e52c7 to
0ddbcaf
Compare
|
The matching-version architecture is sound:
Please request a fresh review after the rebase; the ownership and production routes will be materially different. |
|
Re-reviewed exact head
The PR remains draft; no readiness label should be applied yet. |
Released sizing inputs must fail closed with migration guidance instead of
silently producing native-size artifacts:
- contracts: RETIRED_SCREENSHOT_MAX_SIZE declaration + SCREENSHOT_SCALE_LIMITS
as the single source for the scale bounds and migration messages
- .ad parser: released 'screenshot ... --max-size N' and 'record start ...
--max-size N' lines now refuse at parse time (frozen replay-compat witnesses)
- daemon: screenshot rejects old-client screenshotMaxSize like recording does;
the recording guard now shares the same contract data
- Node client: screenshot/record daemon writers refuse the removed { maxSize }
option before transport
- CLI: --max-size unknown-flag error carries the migration guidance
- config/env: stale screenshotMaxSize config keys and the retired
AGENT_DEVICE_SCREENSHOT_MAX_SIZE env var are refused for sizing commands
(other commands keep working)
Quality: numberField now reuses the canonical readOptionalNumber contract
helper (AppError bounds instead of plain Error); png-resize inlines one-use
wrappers and restores the worker-thread rationale; docs typo fixed.
|
Addressed both remaining blockers at P1 — every released
|
| step | result |
|---|---|
screenshot native.png |
402×874 (pixelDensity 1) |
screenshot --scale 0.3 |
121×262 (exact: 402→121, 874→262), verified on disk with sips |
screenshot --scale 0.5 --overlay-refs |
201×437 with refs burned for the final size |
| session usable after | snapshot -i ok, click text=General → Tapped text=General (201, 406) |
record start/stop --hide-touches |
playable MP4 at native 1206×2622 |
record start/stop (default overlays) |
playable MP4 + gesture telemetry sidecar; 220×480 output is the pre-existing overlay medium-preset export, untouched by this PR |
Live Android emulator (Pixel_7_CI, emulator-5554):
| step | result |
|---|---|
screenshot |
1080×2400 |
screenshot --scale 0.3 |
324×720 (exact) |
record start/stop --hide-touches |
playable MP4, native 1080×2400, 5.8 s — exercises the rebuilt screenrecord command (no --size, no wm size probe) |
| session usable after | press mid-recording ok, snapshot -i after stop ok |
Live retired-path proofs on the same builds: CLI --max-size and exported AGENT_DEVICE_SCREENSHOT_MAX_SIZE both return the migration refusal quoted above.
Residual risk (explicitly unverified live): physical-iOS-device and macOS runner recording (recordStart no longer carries maxSize). Covered by the runner-protocol fixture round-trip and the macOS recording provider scenario (transcript asserts recordStart without maxSize); no hardware/runner session was exercised here.
Quality changes riding along
- Scale bounds (0.01–1) had been hardcoded in five places with two message spellings; now
SCREENSHOT_SCALE_LIMITSin contracts is the single source (flag definition, script parser, validator,numberField,png-resize). numberField's bounds checks threw plainErrorwhile its siblings threwAppError('INVALID_ARGS', …); it now reuses a new canonicalreadOptionalNumbernext toreadOptionalIntegerin contracts, and the local duplicate is deleted.png-resizesingle-usereadPng/writeResizedPngwrappers inlined back (matching its siblingresizePngFile), and the worker-thread rationale comment restored.screenshotFlagsFromPublicOptionsnow states its constraint: the base projection is fed generic cross-command option bags where a barescalebelongs to the pinch gesture, so only the screenshot-boundary variant may read it.- Docs: dropped the stray "Argent-style" phrasing; added a
--max-size→--scalemigration line.
Gates on this head: typecheck, oxlint, format, layering guard (facade symbols updated), full unit suite (613 files / 5424 tests), all 42 provider-scenario files (149 tests), fallow audit + production-exports — all green locally.
* origin/main: chore: Update GitHub Sponsors usernames in FUNDING.yml fix: resolve Dependabot security alerts (callstack#1623) fix: update MCP registry namespace (callstack#1618) fix(ios): corroborate recorded tap outcomes (callstack#1605) fix(ios): never signal a recycled runner pid from a stale lease (callstack#1621) build: eliminate tsdown bundle warnings (callstack#1607) refactor(contracts): one viewport-root predicate for the whole repo (callstack#1613) refactor(contracts): name façade exports explicitly and retire the pin table (callstack#1614) refactor(ios): share one private-XCTest event bridge between gesture and text synthesis (callstack#1608) refactor(daemon): give the Maestro fallback and ambiguous-match details real types (callstack#1612) docs: clarify iOS drag synthesis profiles (callstack#1616) # Conflicts: # scripts/layering/facade-symbols.ts # src/commands/capture/screenshot.ts # src/commands/recording/index.ts
|
Addendum: after the fix commit, the branch had gone stale against
Authoritative CI on |
|
Re-reviewed exact head
No readiness label yet. |
…e-provenance corpus witnesses
- readFieldInput silently dropped undeclared keys before the daemon writers
could refuse them, so an MCP call carrying { maxSize } reached transport and
returned native-size success. New retiredField() combinator declares the
removed key in the field map: the projection seam refuses it with the
canonical migration message and the JSON schema no longer advertises it.
Real-route MCP executor regressions cover screenshot and record.
- replay-compat corpus: derived v0.20.5 witnesses for the released screenshot
and record --max-size forms (SHA-256 pinned, new retired-capture-size
coverage surface) so check:replay-compat proves the shipped syntax refuses
with migration guidance instead of degrading silently.
|
All three items addressed at P1 — MCP retired input now refuses at the projection seamYou were right that my earlier Regressions run the REAL route (no P2 — release-provenance corpus witnessesTwo derived entries under
Both SHA-256 pinned; Exact-head evidence (
|
| step | result |
|---|---|
screenshot |
402×874 |
screenshot --scale 0.3 |
121×262 (sips-verified on disk) |
screenshot --scale 0.5 --overlay-refs |
201×437 |
| continuity | click text=General → Tapped text=General (201, 406); snapshot -i ok after recording |
record start/stop --hide-touches |
playable MP4, native 1206×2622 |
Pixel_7_CI emulator:
| step | result |
|---|---|
screenshot |
1080×2400 |
screenshot --scale 0.3 |
324×720 (sips-verified) |
| continuity | snapshot -i ok before and after recording; press mid-recording ok |
record start/stop --hide-touches |
first two stops failed (pulled file is not a playable MP4); third attempt playable native 1080×2400, 8.7 s |
The two Android stop failures are a pre-existing pull-before-finalize race, not this PR: after kill -2, screenrecord needs ~1–3 s to write the moov atom; record-trace-android-copy.ts pulls immediately and retries once after 750 ms, and its waitForStableFile runs on the local copy, which cannot detect an unfinalized remote file. Live proof on the same emulator, outside our code: pull immediately after kill → 3232 bytes, ffprobe "moov atom not found"; pull 3 s later → 65512 bytes, playable 1080×2400. The failing runs' own diagnostics show the signature (identical fileSize across both attempts). The copy path is byte-identical on main and untouched by this PR; filed separately for a remote-finalization wait. Residual risk unchanged: macOS runner and physical-iOS-device recording (recordStart without maxSize) remain protocol/provider-verified only.
Gates at this head: typecheck, lint, format, layering, 5470 unit tests, 42 provider files, corpus (55), check:replay-compat, fallow audit + production-exports — green locally; CI running on the push.
|
Re-reviewed exact head This is still not merge-ready: the PR remains draft and GitHub reports |
* origin/main: ci: remove package smoke workflow (callstack#1624) fix(ios): pin tap-outcome corroboration probes to the baseline's backend (callstack#1634) refactor: sink backend.ts's cycle-closing types below both zones (callstack#1632) (callstack#1636) refactor(daemon): one interface for the deferred interaction outcome (callstack#1633) feat(ios): extend depth-capped private-AX captures via element-rooted requests (callstack#1627) # Conflicts: # src/daemon/request-generic-dispatch.ts
|
Note: |
|
Re-reviewed exact head |
…137) ## Why agent-device [PR #1617](callstack/agent-device#1617) adds a scale-only `screenshot --scale` knob and removes `--max-size`. Our controller reference told agents the opposite: that agent-device screenshots are "full-resolution with no scale knob." ## How - Rewrote the agent-device screenshot note in `references/controllers.md`: - `screenshot --scale <0.01–1>` resizes both dimensions, `1` = full resolution. - Token-conscious default via `AGENT_DEVICE_SCREENSHOT_SCALE` / `screenshotScale` in config, overridden by an explicit `--scale`. - `--max-size` is removed and older calls are refused with migration guidance. - Added a remote-session caveat: the resize runs on the daemon, so a newer client against an older EAS session daemon can have `--scale` silently ignored. - Gated the whole thing on `v0.20.6` as a single token, easy to bump if it ships as a different version. - Bumped the plugin version to 1.9.7. ## Test Plan - Cross-check every claim against agent-device#1617's own doc changes: flag name, the `0.01–1` range, the env var / config key, and the `--max-size` removal
Summary
max-sizebehavior, including Android/iOS recording plumbing and the obsolete Swift resize helperThe change touches 58 files. Scope expanded from the screenshot command family into recording because video scaling was intentionally removed rather than left as a hidden or partially supported path.
Known gap: a newer client talking to an older remote daemon can have the optional screenshot scale field ignored. A follow-up capability/acknowledgement mechanism is needed for guaranteed behavior across daemon version skew; matching-version remote daemons resize before artifact download.
Validation
Formatting, lint, typecheck, production build, and fallow audit passed. The unit suite passed 2,972 tests; smoke passed 9 cases with one expected live-web skip. Cross-platform integration passed 10 Node cases with six expected live-device skips and 85 provider-integration tests. The provider scenario verifies a 64x64 screenshot becomes 32x32 at scale 0.5. Both iOS and macOS XCTest runner builds passed. CLI help exposes screenshot
--scaleonly, and recording--max-sizefails as an unknown flag. No live EAS Simulator or physical-device session was run.