Skip to content

feat(screenshot): add a per-request iOS simulator screen mask - #4206

Merged
thymikee merged 7 commits into
callstack:mainfrom
sync:feat/ios-simulator-screenshot-mask
Nov 4, 2026
Merged

thymikee merged 7 commits into
callstack:mainfrom
sync:feat/ios-simulator-screenshot-mask

Conversation

@sync

@sync sync commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds request-scoped --screen-mask black for iOS/iPadOS simulator outlines and deterministic visual baselines. Apple declares support for handheld simctl captures; other capture bindings refuse masks. Masked requests cannot fall back to runner.

agent-device screenshot baseline.png --screen-mask black
agent-device diff screenshot --baseline baseline.png --screen-mask black

Node/MCP: screenMask: "black". Saved-image diffs reject it; defaults and thresholds stay unchanged. tvOS/visionOS are unsupported.

31 files. Scope expanded to per-request APIs, live diffs, and capture-owner enforcement following review. No dedicated issue; related #4184 addresses runner capture.

Validation

Head: 71739389ed2b21d7188d11a7055fdcea797e58cc.
Main: 035c2b6cae0b5a2d6b769c36f63daa1e99f92420.

pnpm check:affected --run --head 71739389ed2b21d7188d11a7055fdcea797e58cc (Node 24.13): failed, exactCommit:true. Identical related-suite command on unchanged main: 1,256 passed/4 failed on both, all Apple-runner symlink-removal failures.

Same regression command: prior head 0bbe8a6f1148daa7bb743c85d739c2c273d6946b plus tests 11 failed/33 passed; head 44 passed. Eager-loading/capture suites: 936 passed. Separate unit diagnosis reproduced host-kit failures and Android line 117 on main; Android line 143 remains inconclusive.

Receipt and commands.

Live at da8b7036d8874d7322e07026d50ab7c4a18b759f (production unchanged): iPhone 12 mini, iOS 18.6, 360×780. Black/default/black confirms request isolation. Same-screen diff matched: 0/280,800 pixels; changed screen differed: 40,549 (14.44%). Overlay preserves the outline; its red annotation crosses the top-left corner.

Black Default Black again Changed-screen overlay
Black mask Default capture Black mask again Mismatch overlay

Live evidence.

Ready for CI as requested by @thymikee; local gate remains base-blocked. CI/native/provider lanes remain required. Vercel needs maintainer authorization.

@vercel

vercel Bot commented Nov 3, 2026

Copy link
Copy Markdown

@sync is attempting to deploy a commit to the Callstack Team on Vercel.

A member of the Team first needs to authorize it.

@thymikee

thymikee commented Nov 4, 2026

Copy link
Copy Markdown
Member

At c7ef225, the simulator capture change does what it says. The live evidence is good: the same iPhone 12 mini simulator, with the setting black and with it unset, went through a checkout-built daemon. All four corners came out [0,0,0,255] versus [242,242,247,255], and the base comparison shows the new tests fail without the change.

The open question is where the policy lives. Every other screenshot option is request-scoped, but this one is read from the daemon's startup environment, so it silently changes pixels for every client of that daemon, including live diff-screenshot captures. Could it instead be an explicit screenshot option, carried through the CLI, Node and MCP surfaces and the daemon capture request, with the Apple backend translating it to --mask=black? Please also decide whether diff-screenshot takes the same option. If the environment variable stays, please document its effect on diff-screenshot.

The full unit gate failed on two Android fill-verification assertions. The base reproduced the line-117 failure, and the line-143 failure is a timing assertion (3380 ms against a 1500–1649 ms window), so both look unrelated to this change; a clean rerun would settle it. The only failing check is the Vercel deployment authorization, which is not a code problem. No conflicts are known.

Before merge, please settle where the capture policy lives and get a passing full gate.

@sync

sync commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

At c7ef225, the simulator capture change does what it says. The live evidence is good: the same iPhone 12 mini simulator, with the setting black and with it unset, went through a checkout-built daemon. All four corners came out [0,0,0,255] versus [242,242,247,255], and the base comparison shows the new tests fail without the change.

The open question is where the policy lives. Every other screenshot option is request-scoped, but this one is read from the daemon's startup environment, so it silently changes pixels for every client of that daemon, including live diff-screenshot captures. Could it instead be an explicit screenshot option, carried through the CLI, Node and MCP surfaces and the daemon capture request, with the Apple backend translating it to --mask=black? Please also decide whether diff-screenshot takes the same option. If the environment variable stays, please document its effect on diff-screenshot.

The full unit gate failed on two Android fill-verification assertions. The base reproduced the line-117 failure, and the line-143 failure is a timing assertion (3380 ms against a 1500–1649 ms window), so both look unrelated to this change; a clean rerun would settle it. The only failing check is the Vercel deployment authorization, which is not a code problem. No conflicts are known.

Before merge, please settle where the capture policy lives and get a passing full gate.

On it, thanks for the quick reply

@sync sync changed the title feat(screenshot): opt into black iOS simulator screen outlines feat(screenshot): add a per-request iOS simulator screen mask Nov 4, 2026
@sync

sync commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

Agreed, making this a per-request flag was the better call. I’ve replaced the env var with --screen-mask black, exposed it in Node/MCP, and added it to live screenshot diffs too. Thanks for the suggestion!

@sync
sync marked this pull request as ready for review November 4, 2026 01:54
@sync
sync marked this pull request as draft November 4, 2026 01:54

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 28 files

Reply to a comment to ask cubic a question or push back. It learns from your replies.

View guided diff | Re-trigger cubic

@thymikee

thymikee commented Nov 4, 2026

Copy link
Copy Markdown
Member

At 0bbe8a6, the earlier environment-to-request concern is fixed, but the capture support rule and live verification still need work.

Could you provide live evidence for the public client and diff capture route? Replay does not exercise the changed client or live-diff path, which the supplied validation describes as mocked. From a checkout-built daemon, capture black/default/black through the public screenshot client on one simulator and daemon, and show the commands, source SHA, device identity, route, dimensions and corner pixels. Then compare a masked live screenshot with a masked baseline, force a mismatch with --overlay-refs, and inspect the overlay capture's corners. Attribute the unchanged backend evidence to 35fba70 and run the changed diff route at head.

Could the existing capture-operation binding receive the selected implementation's mask capability, default to unsupported, and enforce it at the common capture invocation? Keep the per-request API and one enum/value reader in the screenshot contract. Apple can determine support from the handheld simctl, runner, macOS or physical route, and masked captures must use a flow that cannot fall back to runner. Extend the binding's owning type first, then remove the superseded daemon family test and provider wrapper; keep saved-image rejection in diff because it has no capture invocation.

The open threads on direct Apple calls and lower-level mask handling still apply. The simulator family scope needs clarification or native evidence before it can be treated as a defect. The CLI diff guard does not apply because daemon admission guards that route; the test assertion does not apply because it also checks that interactor resolution was not called; the optional path does not apply because enum parsing rejects the invalid value first.

The only failing check is the Vercel deployment authorization, which is not a code problem. The local affected gate also failed, and the second Android assertion has no conclusive base comparison yet. No conflicts are known. Before merge, please provide the live evidence, settle mask support at the capture owner, and get a passing exact-head affected gate or show that the failure also happens on main.

@thymikee

thymikee commented Nov 4, 2026

Copy link
Copy Markdown
Member

At da8b703, the code looks good. Mask support is now decided once, by the Apple screenshot support check, and enforced where the capture runs. A masked capture cannot fall back to a route that ignores the mask. The two remaining threads, on diff capture and optional paths, no longer apply: diff goes through the guarded capture, and the parser rejects the invalid value first.

The live evidence covers what I asked for. A checkout-built daemon went through the public client with black, then default, then black again on one simulator. The corners came back [0,0,0,255], [242,242,247,255] and black again, so the option stays per request. The masked live diff matched the same screen exactly and flagged the changed screen, and the overlay keeps the black outline.

The local gate failures also happen on main (the Apple runner symlink removal and the Android fill-verification line 117). The line-143 failure is a timing assertion. No GitHub Actions checks have run on this head yet; the only reported check is the Vercel deployment authorization. No conflicts are known. Before merge, please mark the PR ready so the CI lanes run on this head.

@sync
sync marked this pull request as ready for review November 4, 2026 04:50

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 31 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread website/docs/docs/commands.md Resolved
@sync

sync commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks Michael. The live results and screenshots are inline, and the PR is ready for CI. I also tightened the runner assertion and removed the ambiguous tvOS/visionOS wording in 7173938. Production code is unchanged from the live-tested commit. The exact-head gate still hits the same four Apple-runner failures reproduced on main; the new receipt is linked in the description.

@thymikee

thymikee commented Nov 4, 2026

Copy link
Copy Markdown
Member

At 7173938, the code looks good. The screenshot backend test now asserts the exact refusal reason, and the docs describe the refusal, so the reason assertion thread and the documentation thread no longer apply. The live evidence from da8b703 still covers this head, since only a test and docs changed.

The local gate failures match main, as noted before. The only failing check is the Vercel deployment authorization, which is not a code problem, and the GitHub Actions lanes have not run on this head yet. No conflicts are known.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Nov 4, 2026
@thymikee
thymikee merged commit 9cf8168 into callstack:main Nov 4, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants