Repository navigation
feat(screenshot): select the XCTest runner capture with --capture-backend runner - #4184
vitalyiegorov wants to merge 1 commit into
Conversation
…kend runner On iOS simulators with a Dynamic Island, simctl io screenshot draws the island into every capture (no --mask policy removes it), while XCUIScreen, which Maestro and other XCTest tools capture with, leaves it out. A visual baseline made by such a tool therefore cannot be matched by agent-device's default capture. The runner capture already existed for the Maestro port behind an internal flag. screenshot --capture-backend runner (SDK and MCP: captureBackend: 'runner') selects it on Apple targets. It is refused on other platforms and with --pixel-density, since the runner encodes the device density. The Maestro port now uses the public flag; flags.maestro.screenshotCaptureBackend is removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@vitalyiegorov is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
|
The code at 4b8e1fd needs changes before merge. The screenshot admission policy accepts runner capture for macOS desktop, menubar, frontmost-app helper surfaces and native app sessions, but runAppleScreenshot returns the helper capture before reaching the runner branch. CLI, SDK and MCP can therefore report success using a different backend than requested. The owning admission and binding policy must account for both the selected surface and app backend: honor runner where supported and reject incompatible helper captures. Validate that an explicit runner request cannot succeed through the helper path. The changed public device route still lacks reproducible live evidence; the supplied simulator RMSE claim has no linked capture artifacts. Attach commands attributed to the reviewed head, device and OS details, exit status, artifact output and session cleanup. Run public screenshot commands with the default backend and --capture-backend runner on a Dynamic Island simulator, with viewable PNGs and a comparison showing the runner capture has no island. Run the same public runner request on an Android session and show UNSUPPORTED_OPERATION, then run simulator runner capture with --pixel-density and show INVALID_ARGS. Could the shared flag and projection extension stay as written, with runner capability declared at the owning Apple surface binding and enforced during screenshot admission, instead of treating every Apple surface as runner-capable? The only failing CI entry is Vercel deployment authorization, which explicitly requires team authorization and appears unrelated to the code changes; the diff changes website docs but no deployment authorization configuration. The local integration-node retirement failure remains unattributed without logs, because the changes overlap daemon request and context paths. No conflicts are known. Before merge, resolve the silent macOS backend override, supply the exact-head live evidence, complete outstanding validation and obtain Vercel authorization. |
There was a problem hiding this comment.
6 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/command-surface/src/capture/screenshot.ts">
<violation number="1" location="packages/command-surface/src/capture/screenshot.ts:49">
P2: `captureBackend` has no schema description, so the MCP `screenshot` input is undocumented and `command-tools-input-docs.test.ts` fails on `screenshot.captureBackend`. Pass a description to `enumField`, which the MCP input schema and `--help` rely on.</violation>
</file>
<file name="packages/command-surface/src/capture/screenshot-options.test.ts">
<violation number="1" location="packages/command-surface/src/capture/screenshot-options.test.ts:142">
P3: This test covers the recorded-flag contract but never checks the `recorded` marker, so the new `screenshotCaptureBackend` entry (declared `recorded: false`) is not verified as excluded from `.ad` scripts. Add an assertion that filters `SCREENSHOT_SPECIFIC_FLAG_DEFINITIONS` by `recorded` and excludes `screenshotCaptureBackend`, and assert `appendScreenshotScriptFlags` emits nothing for `screenshotCaptureBackend: 'runner'`.</violation>
</file>
<file name="scripts/integration-progress-model.ts">
<violation number="1" location="scripts/integration-progress-model.ts:383">
P3: Excluding `screenshotCaptureBackend` from provider-backed coverage needs a justification comment. The group is named for rendering options, and runner capture selects the capture source, which is a device-observable path. Add a comment above this entry that explains why no provider scenario can observe runner capture, or move the key to a group that matches its behavior.</violation>
</file>
<file name="packages/daemon/src/execution/screenshot-runtime.ts">
<violation number="1" location="packages/daemon/src/execution/screenshot-runtime.ts:335">
P2: `assertSupportedScreenshotCaptureBackend` accepts `--capture-backend runner` on macOS with a non-app `--surface`, but that surface is captured through the native helper and the runner flag is silently dropped. Refuse the combination here by passing `session.surface` and rejecting runner when the macOS helper surface is in use.</violation>
</file>
<file name="packages/contracts/src/screenshot.ts">
<violation number="1" location="packages/contracts/src/screenshot.ts:300">
P1: `captureBackend` is accepted and projected here, but the shared screenshot command drops it before calling the platform backend. Forward `options.captureBackend` in `packages/command-runtime/src/commands/capture/runtime/screenshot.ts`; otherwise `--capture-backend runner` is effectively ignored.</violation>
</file>
<file name="packages/maestro/src/daemon-port/daemon-runtime-public-operation.ts">
<violation number="1" location="packages/maestro/src/daemon-port/daemon-runtime-public-operation.ts:401">
P3: This projection no longer uses a Maestro-nested capture backend, but `dispatch-context.ts` still documents it as “maestro-nested.” Update that comment to describe the top-level screenshot capture backend.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| stabilize: | ||
| flags?.screenshotNoStabilize === undefined ? undefined : !flags.screenshotNoStabilize, | ||
| normalizeStatusBar: flags?.screenshotNormalizeStatusBar, | ||
| captureBackend: flags?.screenshotCaptureBackend, |
There was a problem hiding this comment.
P1: captureBackend is accepted and projected here, but the shared screenshot command drops it before calling the platform backend. Forward options.captureBackend in packages/command-runtime/src/commands/capture/runtime/screenshot.ts; otherwise --capture-backend runner is effectively ignored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/contracts/src/screenshot.ts, line 300:
<comment>`captureBackend` is accepted and projected here, but the shared screenshot command drops it before calling the platform backend. Forward `options.captureBackend` in `packages/command-runtime/src/commands/capture/runtime/screenshot.ts`; otherwise `--capture-backend runner` is effectively ignored.</comment>
<file context>
@@ -273,6 +297,7 @@ export function screenshotOptionsFromFlags(
stabilize:
flags?.screenshotNoStabilize === undefined ? undefined : !flags.screenshotNoStabilize,
normalizeStatusBar: flags?.screenshotNormalizeStatusBar,
+ captureBackend: flags?.screenshotCaptureBackend,
});
}
</file context>
| maxSize: retiredField(RETIRED_SCREENSHOT_MAX_SIZE.migration.screenshot), | ||
| stabilize: booleanField(), | ||
| normalizeStatusBar: booleanField(), | ||
| captureBackend: enumField(SCREENSHOT_CAPTURE_BACKENDS), |
There was a problem hiding this comment.
P2: captureBackend has no schema description, so the MCP screenshot input is undocumented and command-tools-input-docs.test.ts fails on screenshot.captureBackend. Pass a description to enumField, which the MCP input schema and --help rely on.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/command-surface/src/capture/screenshot.ts, line 49:
<comment>`captureBackend` has no schema description, so the MCP `screenshot` input is undocumented and `command-tools-input-docs.test.ts` fails on `screenshot.captureBackend`. Pass a description to `enumField`, which the MCP input schema and `--help` rely on.</comment>
<file context>
@@ -45,6 +46,7 @@ const screenshotCommandMetadata = defineFieldCommandMetadata(
maxSize: retiredField(RETIRED_SCREENSHOT_MAX_SIZE.migration.screenshot),
stabilize: booleanField(),
normalizeStatusBar: booleanField(),
+ captureBackend: enumField(SCREENSHOT_CAPTURE_BACKENDS),
surface: enumField(SESSION_SURFACES),
},
</file context>
| captureBackend: enumField(SCREENSHOT_CAPTURE_BACKENDS), | |
| captureBackend: enumField( | |
| SCREENSHOT_CAPTURE_BACKENDS, | |
| 'Apple targets only: capture through the XCTest runner (XCUIScreen) instead of simctl, leaving out the Dynamic Island. Cannot be combined with pixelDensity.', | |
| ), |
| `--capture-backend takes ${SCREENSHOT_CAPTURE_BACKENDS.join(', ')}`, | ||
| ); | ||
| } | ||
| if (!isApplePlatform(device.platform)) { |
There was a problem hiding this comment.
P2: assertSupportedScreenshotCaptureBackend accepts --capture-backend runner on macOS with a non-app --surface, but that surface is captured through the native helper and the runner flag is silently dropped. Refuse the combination here by passing session.surface and rejecting runner when the macOS helper surface is in use.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/daemon/src/execution/screenshot-runtime.ts, line 335:
<comment>`assertSupportedScreenshotCaptureBackend` accepts `--capture-backend runner` on macOS with a non-app `--surface`, but that surface is captured through the native helper and the runner flag is silently dropped. Refuse the combination here by passing `session.surface` and rejecting runner when the macOS helper surface is in use.</comment>
<file context>
@@ -317,6 +319,33 @@ async function annotateScreenshotWithSessionRefs(
+ `--capture-backend takes ${SCREENSHOT_CAPTURE_BACKENDS.join(', ')}`,
+ );
+ }
+ if (!isApplePlatform(device.platform)) {
+ throw new AppError(
+ 'UNSUPPORTED_OPERATION',
</file context>
| 'screenshotScale', | ||
| 'screenshotNoStabilize', | ||
| 'screenshotNormalizeStatusBar', | ||
| 'screenshotCaptureBackend', |
There was a problem hiding this comment.
P3: This test covers the recorded-flag contract but never checks the recorded marker, so the new screenshotCaptureBackend entry (declared recorded: false) is not verified as excluded from .ad scripts. Add an assertion that filters SCREENSHOT_SPECIFIC_FLAG_DEFINITIONS by recorded and excludes screenshotCaptureBackend, and assert appendScreenshotScriptFlags emits nothing for screenshotCaptureBackend: 'runner'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/command-surface/src/capture/screenshot-options.test.ts, line 142:
<comment>This test covers the recorded-flag contract but never checks the `recorded` marker, so the new `screenshotCaptureBackend` entry (declared `recorded: false`) is not verified as excluded from `.ad` scripts. Add an assertion that filters `SCREENSHOT_SPECIFIC_FLAG_DEFINITIONS` by `recorded` and excludes `screenshotCaptureBackend`, and assert `appendScreenshotScriptFlags` emits nothing for `screenshotCaptureBackend: 'runner'`.</comment>
<file context>
@@ -135,6 +139,7 @@ test('screenshot script flags use the shared recorded flag contract', () => {
'screenshotScale',
'screenshotNoStabilize',
'screenshotNormalizeStatusBar',
+ 'screenshotCaptureBackend',
]);
assert.deepEqual(
</file context>
| name: 'Apple simulator screenshot rendering options', | ||
| owner: 'iOS platform and screenshot-diff runtime tests', | ||
| keys: ['screenshotNormalizeStatusBar', 'screenshotPixelDensity'], | ||
| keys: ['screenshotNormalizeStatusBar', 'screenshotPixelDensity', 'screenshotCaptureBackend'], |
There was a problem hiding this comment.
P3: Excluding screenshotCaptureBackend from provider-backed coverage needs a justification comment. The group is named for rendering options, and runner capture selects the capture source, which is a device-observable path. Add a comment above this entry that explains why no provider scenario can observe runner capture, or move the key to a group that matches its behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/integration-progress-model.ts, line 383:
<comment>Excluding `screenshotCaptureBackend` from provider-backed coverage needs a justification comment. The group is named for rendering options, and runner capture selects the capture source, which is a device-observable path. Add a comment above this entry that explains why no provider scenario can observe runner capture, or move the key to a group that matches its behavior.</comment>
<file context>
@@ -380,7 +380,7 @@ function summarizeProviderScenarioFlagExclusions() {
name: 'Apple simulator screenshot rendering options',
owner: 'iOS platform and screenshot-diff runtime tests',
- keys: ['screenshotNormalizeStatusBar', 'screenshotPixelDensity'],
+ keys: ['screenshotNormalizeStatusBar', 'screenshotPixelDensity', 'screenshotCaptureBackend'],
},
{
</file context>
| ...(operation.stabilize === false ? { screenshotNoStabilize: true } : {}), | ||
| ...(operation.captureBackend === 'runner' | ||
| ? { maestro: { screenshotCaptureBackend: 'runner' as const } } | ||
| ? { screenshotCaptureBackend: 'runner' as const } |
There was a problem hiding this comment.
P3: This projection no longer uses a Maestro-nested capture backend, but dispatch-context.ts still documents it as “maestro-nested.” Update that comment to describe the top-level screenshot capture backend.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/maestro/src/daemon-port/daemon-runtime-public-operation.ts, line 401:
<comment>This projection no longer uses a Maestro-nested capture backend, but `dispatch-context.ts` still documents it as “maestro-nested.” Update that comment to describe the top-level screenshot capture backend.</comment>
<file context>
@@ -398,7 +398,7 @@ function projectCaptureOperation(
...(operation.stabilize === false ? { screenshotNoStabilize: true } : {}),
...(operation.captureBackend === 'runner'
- ? { maestro: { screenshotCaptureBackend: 'runner' as const } }
+ ? { screenshotCaptureBackend: 'runner' as const }
: {}),
},
</file context>
Summary
On an iOS simulator with a Dynamic Island,
simctl io screenshotdraws the island into every capture. No--maskpolicy (ignored,alpha,black) removes it: the island is in the framebuffer.XCUIScreen, which Maestro and other XCTest tools capture with, leaves it out. So a baseline made by such a tool cannot be matched by agent-device's default capture: the pill lands in the top rows.The runner capture already existed for the Maestro port, behind the internal
flags.maestro.screenshotCaptureBackend. This PR makes it public:UNSUPPORTED_OPERATION), with--pixel-density(INVALID_ARGS: the runner encodes the device density and skips normalization), and for any value butrunner.flags.maestro.screenshotCaptureBackendis removed.website/docs/docs/commands.md. Not recorded in.adscripts.Validation
Tested SHA
4b8e1fdd1.check:affected --run --head 4b8e1fdd1: format, lint, typecheck, layering, di-seams, fallow, native-dupes, mcp-metadata, build and package passed.integration-nodefailed onsmoke-daemon-clean"forced retirement…". It also fails 2/3 runs onmainon this host. Later checks did not run, so this is a draft; CI is the authority.🤖 Generated with Claude Code