Skip to content

feat: add scale-only screenshot sizing - #1617

Draft
szdziedzic wants to merge 1 commit into
callstack:mainfrom
szdziedzic:szdziedzic-codex/screenshot-scale
Draft

feat: add scale-only screenshot sizing#1617
szdziedzic wants to merge 1 commit into
callstack:mainfrom
szdziedzic:szdziedzic-codex/screenshot-scale

Conversation

@szdziedzic

Copy link
Copy Markdown
Contributor

Summary

  • add scale-only screenshot sizing across CLI, config/env, Node.js/MCP, daemon execution, and replay scripts
  • resize captured PNGs centrally before artifact publication so local and matching-version remote daemons share the same behavior
  • remove screenshot and video max-size behavior, including Android/iOS recording plumbing and the obsolete Swift resize helper
  • keep recording focused on FPS, quality, and touch-overlay controls; older clients sending the removed recording field now receive an explicit error

The 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 --scale only, and recording --max-size fails as an unknown flag. No live EAS Simulator or physical-device session was run.

@szdziedzic
szdziedzic force-pushed the szdziedzic-codex/screenshot-scale branch from 33e52c7 to 0ddbcaf Compare August 5, 2026 12:37
@thymikee

thymikee commented Aug 5, 2026

Copy link
Copy Markdown
Member

The matching-version architecture is sound: scale projects through CLI/config, Node/MCP and the daemon, then the daemon resizes the captured PNG centrally before artifact publication. The 64×64 → 32×32 provider scenario also exercises that production route rather than only the helper. This head is not ready, though:

  • P1 — preserve or explicitly refuse released .ad syntax. Removing --max-size recognition from the screenshot and record script parsers turns a released line such as screenshot "x.png" --max-size 1024 or record start "x.mp4" --max-size 1024 into extra positionals. The handlers consume the normal path/action fields and silently run at native size. Add frozen replay-compat witnesses for both shipped forms and either retain legacy execution or return an actionable migration refusal; sizing must not disappear silently.
  • P1/P2 — fail closed for old-client/new-daemon screenshots. A released client sends flags.screenshotMaxSize; the new screenshot projection ignores it and returns a full-size artifact. Recording already detects the raw retired field and returns an explicit error. Screenshot needs equivalent compatibility handling (or preserved semantics). The disclosed inverse skew—new scale sent to an older daemon—is still a residual product gap until command capability/acknowledgement exists.
  • Rebase/ownership blocker. This commit is based on the pre-package tree, hundreds of commits behind current main, conflicts, and edits superseded src/contracts / src/replay owners. Rebase and port the change into @agent-device/contracts and @agent-device/ad-script, preserving current screenshot pixelDensity, recording scope/backend/session behavior, package façades, and current docs.
  • Gate omission. scripts/integration-progress-model.ts still classifies screenshotMaxSize and does not classify screenshotScale; update the device-observable flag row. Also include the required quoted red proof for the new dimension regression (without central scale resize it must remain 64×64 rather than 32×32).
  • Readiness evidence. This head has no CI and the PR reports no live device run. After the rebase and green authoritative gates, show at least one simulator/emulator native-vs---scale 0.3 PNG dimension comparison (ideally with --overlay-refs) and that the session remains usable. Because this also removes recording plumbing across Android and Apple, smoke a default native recording start/stop on the affected backends or state the unverified backend as residual risk.

Please request a fresh review after the rebase; the ownership and production routes will be materially different.

@thymikee

thymikee commented Aug 5, 2026

Copy link
Copy Markdown
Member

Re-reviewed exact head 0ddbcaf. The rebase/ownership blocker is fixed: the change now uses the current @agent-device/contracts and @agent-device/ad-script owners, screenshotScale is classified in the progress model, and authoritative CI is green. Two blockers remain:

  • P1 — released max-size inputs still disappear silently. The .ad parser removes --max-size handling for both screenshot and record without frozen replay-compat witnesses; released lines therefore leave the flag/value as extra positionals, while the command readers consume the normal path/action slots and ignore the remainder, producing native-size success. The screenshot daemon also lacks recording’s explicit retired-field guard, so an old client sending screenshotMaxSize receives a full-size artifact. Released Node callers using { maxSize } and AGENT_DEVICE_SCREENSHOT_MAX_SIZE are likewise dropped. Preserve the shipped semantics or reject every retired path explicitly with actionable migration guidance.
  • P2 — the scale/removal evidence is not yet merge-grade. Exact-head iOS/Android smoke invokes screenshot --scale 0.5, but asserts only the returned path and a valid PNG signature, so it remains green if scale is ignored. The provider 64×64 → 32×32 assertion has no quoted red proof. Recording belongs to the full observability tiers, not the smoke tiers that ran here, and the PR body reports no live run. Please provide native-vs-scaled dimensions on a simulator/emulator (ideally with overlay refs and a subsequent command proving session usability), plus a live default recording start/stop on the affected Apple/Android paths, or explicitly downgrade the unverified backends to residual risk.

The PR remains draft; no readiness label should be applied yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants