fix: carry response warnings to every surface, and stamp the sheet behind a refusal

[thymikee]

Summary

Refs #2560 (capture-routing/lineage half stays with #2450/#2485).

An optional: true Maestro step that skipped left a warning that vanished from human output the moment a later step failed — the repro printed a bare REPLAY_DIVERGENCE with no trace of the 7s skip. Warnings now travel with the run whether it passes or fails:

Test summary: 0 passed 1 failed, 1 failed in 9.4s

Warnings:
  sign-in [01-login.yaml]: warning: Optional Maestro assertVisible skipped at line 3: ...

• Engine emits the accumulated warnings on the step-failure event; the daemon copies them (scrubbed) onto the failure response's error details; the failed test result gained the warnings array the passing result already had; the default reporter's Warnings: section, JUnit lines, failing replay stderr, and MCP error text all render them.
• An Apple runner presentation refusal now carries the registry identity of the system surface the tree came from as error.details.systemSurface, so a capture-boundary miss stops looking like a selector bug. Rebased over fix(apple): carry the runner's sparse verdict through a viewport refusal #2572: the sparse-declared verdict and its surface-host hint are owned there; this PR keeps the provenance plumbing and the warnings channel only.

Validation

Rebased head 2c78c6f; format, lint, typecheck, unit-core+apple-runner (10377; one unrelated subprocess-contention flake in runner-client.test.ts that passes isolated), output-economy, maestro-compat, layering, production-exports, wire/replay-compat, command-docs, tmpdir, agent-guidance all green locally. check:affected's mutation-model lane fails identically on pristine origin/main@775163 (reproduced in a clean worktree) — pre-existing, not from this diff. No device run yet: the live SafariViewService refusal recording is still an open ask from review.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-15 13:37 UTC

[github-actions]

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	4.57 MB	4.57 MB	+1.0 kB
Package (unpacked)	4.56 MB	4.57 MB	+1.0 kB
Package (download)	1.36 MB	1.36 MB	+260 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	28.7 ms	28.6 ms	-0.0 ms
CLI --help	78.5 ms	80.5 ms	+2.0 ms

[thymikee]

No code findings at fb07951: skipped-step warnings reach the failure response and human reporters, with replay-variable scrubbing preserved. Checks are green. The Apple presentation hint is also a changed device-facing path, so fixture evidence alone leaves a validation gap: please record a live simulator refusal on a confirmed SafariViewService sheet and verify the hint and systemSurface details. The underlying sheet-interaction fix remains outside this PR.

[thymikee]

Rebased onto main for #2572 (same refusal path). Consequences of the rebase: the runner-side viewport-refusal hint from the first revision is dropped, honoring #2572's byte-for-byte decision for non-sparse refusals; the sparse path's surface-host hint is yours there. What survives here: error.details.systemSurface provenance on runner refusals, the warnings channel end-to-end, and the MCP/stderr error rendering. Provider/runner tests re-pin main's exact details shapes. Rebase-time note: scripts/mutation ownership model fails identically on pristine origin/main@775163 (verified in a clean worktree) — flagging since it gates check:affected.

[thymikee]

The rebase at 4932e0f preserves the warnings channel and typed system-surface provenance; no new runtime defect found. CHANGELOG.md:25 still promises a non-sparse snapshot --raw/surface hint that this revision deliberately removes. Please correct that claim and supply the outstanding live SafariViewService refusal evidence for systemSurface.

The failed iOS check is testAlertDismissDoesNotActivateAReplacementWithTheSameTitle returning ALERT_DEADLINE_EXCEEDED in unchanged native code, so it looks unrelated to this diff. It remains a separate CI merge blocker.

[thymikee]

Fixed in 1efc67c. You were right — my "one shared reader" claim was false: readResponseWarnings was an independent filter, and there were two more (the daemon readStringArray, the inline filter in messageWithWarningsText) plus the pre-existing daemon session-open-warnings reader.

The field now has one parser: readResponseWarnings in packages/kernel/src/success-text.ts, next to readCommandMessage — the same depth and pattern the message field already uses, so daemon, CLI, and MCP all reach it. Named consumers at this head: session-open-warnings.ts (append), session-open-foreground.ts:157, session-test-outcome.ts:21/30, output-common.ts messageWithWarningsText, printHumanError, formatToolErrorText. The CLI-side duplicate is deleted, not wrapped; no unclaimed compatibility wrappers remain. Parser contract pinned in packages/kernel/src/success-text.test.ts; adoption sites pinned by session-open-warnings.test.ts (append through the parser), session-test-outcome.test.ts (both branches), and the existing CLI/MCP error-render tests.

On the fifth fold: at fb07951 the cited coordinates 240-247/354-362 of replay-plan-step-execution.ts land in flowConditionMatches/observe, not warning code — formatOptionalWarning at :97 is the engine's only warning producer (rg "warnings.push" packages/maestro/src finds one hit), and it emits bare warning text with no prefix; every Warning:/warning: prefix is added by the presentation consumers. So no engine-side prefix fold exists to unify — the prefix lives only at render, which is where you put it. readStringArray in kernel/errors.ts reads candidate arrays, a different field, and stays.

[thymikee]

The warnings change at 1efc67c looks right. Every consumer now reads through readResponseWarnings, keeps the earlier warnings, and renders the same text as before.

Two items from the last review are still open. CHANGELOG.md still says a non-sparse "requires a valid viewport" refusal now carries a snapshot --raw hint that names the surface. The code does not do that: throwSnapshotEngineError in packages/platform-apple/src/runner/snapshot-presentation.ts only adds details.systemSurface. Please change the entry to match the code. The live SafariViewService refusal run for error.details.systemSurface is also still missing.

CI is green and there are no conflicts.

[thymikee]

Update: the branch now conflicts with current main in src/commands/replay/index.ts. Please rebase and fix the CHANGELOG.md wording at the same time. The live SafariViewService refusal run is still needed.

[thymikee]

Rebased onto main at 6737b0c (took the #2590 command-registry/command-schema import path in replay/index.ts, kept the facet formatter). The CHANGELOG claim is corrected: the second entry now describes exactly what the code does — error.details.systemSurface provenance on runner presentation refusals, with the sparse hint credited to #2572; the snapshot --raw/non-sparse-hint wording is gone from the diff. Gates re-run on the new head: typecheck, unit-core+apple-runner+output-economy (two full-run timeouts in untouched Android runtime-hints tests pass in isolation — contention, flagged by the slow-test gate pre-rebase), wire/replay/command-docs/maestro compat, layering, production-exports, fallow, lint. Live SafariViewService refusal evidence for systemSurface is next; results will be posted here.

[thymikee]

Live SafariViewService refusal recorded at head bd19119.

Setup: minimal UIKit probe app (dev.callstack.sheetprobe, source compiled in-house with swiftc, no third-party binary) presenting SFSafariViewController(url: https://example.com) on iPhone 16 Pro / iOS 26.2, driven by the repo CLI (node bin/agent-device.mjs) from this worktree.

• Healthy path: snapshot -i succeeds under the sheet and carries the pre-existing disclosure warning ("A system web sign-in sheet is presented over the app…").
• Refusal: snapshot --actions (the same private-AX-requested capture fix(apple): carry the runner's sparse verdict through a viewport refusal #2572 documented) returns the invariant message unchanged with fix(apple): carry the runner's sparse verdict through a viewport refusal #2572's hint naming the host, and now:

"details": {
  "reason": "invalid-viewport",
  "field": "viewport",
  "systemSurface": { "bundleId": "com.apple.SafariViewService", "kind": "web-auth" },
  "snapshotQuality": { "state": "sparse", "backend": "private-ax", "reason": "…", "reasonCode": "requested-backend" }
}

The run exposed one gap worth naming: the natural sheet refusal lands on #2572's sparse-declared branch, which carried the hint but not the structured provenance — the requested evidence could not occur anywhere else live. So bd19119 also attaches details.systemSurface on the sparse branch (additive keyed field; message and hint bytes unchanged, non-sparse invariant message unchanged). Runner test extended to pin both carriers. If you'd rather the sparse verdict's details stay exactly as #2572 shaped them, say so and I'll drop that hunk — but then the structured provenance has no live trigger.

[thymikee]

At bd19119 the rebase keeps both sides in src/commands/replay/index.ts, the CHANGELOG now describes only details.systemSurface and the warnings channel, and your SafariViewService run exercises the sparse refusal branch at this head. Two small things remain.

The new comment on readResponseWarnings says every reader goes through it, but src/agent-device-client.ts:269 still has its own copy of the same filter. Please use the shared reader there, or narrow the comment.

The title still says "hint iOS viewport refusals", but that hint now lives in #2572 and this PR carries systemSurface provenance. Please retitle it so the squash commit matches the code.

CI is green and there are no conflicts.

[thymikee]

Both done at 14c9d0b. src/agent-device-client.ts now reads open warnings through readResponseWarnings — that was a sixth inline copy, missed because the SDK client wasn't in the sweep; the kernel comment now names the full consumer set (daemon append + attempt outcome, CLI success line, CLI/MCP error text, SDK client). Two other data.warnings readers stay as they are by concept: snapshot-quality-latch passes the raw unknown array through for latching, and contracts/snapshot-capture-annotations is a serialized wire reader with absent-means-undefined tri-state, not a notes projection. Title retitled to match the code: fix(replay): carry optional-skip warnings to every human surface and name system surfaces in refusals.

[thymikee]

Reviewed the bd19119..14c9d0b delta. The earlier findings are addressed: the SDK client now reads open warnings through readResponseWarnings, and the title no longer mentions viewport hints.

One small gap remains. readSnapshotWarnings in src/commands/output/snapshot.ts:283 still parses data.warnings itself, through a local rawWarnings. The new comment in packages/kernel/src/success-text.ts says this field has one parser, so that is not true yet. The effect is small: snapshot and diff text drop empty-string warnings, while the CLI success line and error text print them as a bare Warning: line. Could readSnapshotWarnings call readResponseWarnings(data) (and keep the empty-string filter if snapshot needs it), or could the comment list it as a consumer? A follow-up is fine too.

CI was still running at review time, with no failures. The delta is a comment rewrite and a reader swap with the same behavior, so it does not reach the smoke device paths.

[thymikee]

Reviewed at 14c9d0b. The client now reads warnings through the shared readResponseWarnings, and the title matches the change, so both points from the last review are fixed. There are no conflicts.

Smoke Tests failed in the iOS simulator fixture run at wait text Automation lab. This change does not touch that wait route, so the failure is likely unrelated; the iOS smoke lane is also unstable on main (#2491). A rerun should confirm it.

[thymikee]

At 35c959f: readSnapshotWarnings is now readResponseWarnings(data).filter((warning) => warning.length > 0) — one parser for the field, with snapshot's empty-note drop kept as its own rendering rule, and the kernel comment lists it as a consumer so the claim is now literally true. The empty-string asymmetry you noted stays as-is (snapshot text drops empty notes, success/error lines print them); unifying that rendering is a separate call, flag if you want it here. Smoke: the push re-triggers the lanes on the new head, so the iOS fixture rerun you suggested is in flight — if wait text Automation lab fails again on 35c959f I'll treat it as the #2491 lane instability rather than this diff, since nothing here touches the wait route.

[thymikee]

Retitled to match the code: the non-sparse viewport hint lives in #2572, and this PR carries the warnings channel plus error.details.systemSurface provenance. The SDK client's duplicate filter is gone at 35c959f3ab, which reads the shared readResponseWarnings.

[thymikee]

CI on 35c959f fully green, including both Smoke Tests lanes — the iOS fixture rerun passed, consistent with the #2491 lane instability on the earlier head.

[thymikee]

Reviewed at 35c959f. readSnapshotWarnings now uses the shared readResponseWarnings, and snapshot text still drops non-string and empty notes, so the duplicate from the last round is gone.

One copy of the same reader is left. readScreenshotWarnings in src/client/screenshot-result.ts#L79 still parses warnings with its own Array.isArray and string filter, while the new kernel doc says the SDK client readers all go through the shared parser. Output is the same today, but a later change to the field contract would miss the screenshot result. Could it call readResponseWarnings (and keep returning undefined when the field is not an array), or could the doc name it as an exception?

CI is green on 35c959f. There are no conflicts.

[thymikee]

Done at 027eae4: readScreenshotWarnings now takes the record and returns readResponseWarnings(data).filter((w) => w.length > 0) behind its Array.isArray(data.warnings) guard, so absent-or-non-array still means undefined (no warnings channel on the result) while the string-filter contract lives in the shared parser. Kernel doc updated — the SDK client entry now names open and screenshot result, and the rendering-rule sentence credits the empty-note drop to snapshot text and screenshot result. Pins already existed in src/client/__tests__/screenshot-result.test.ts (surfacing + omission) and stay green. I also swept the remaining entry is string sites repo-wide: client-companion-tunnel filters the tunnel-file consumers array, and interaction.ts/request-generic-dispatch compose the singular warning string field — different fields, untouched; contracts/snapshot-capture-annotations stays the acknowledged serialized tri-state reader. CI is re-triggering on 027eae4.

[thymikee]

Reviewed at 027eae4. The finding from 35c959f is fixed: screenshot warnings now go through the shared readResponseWarnings reader.

One reader still has its own copy of that rule. readSerializedSnapshotCaptureAnnotations (snapshot-capture-annotations.ts#L60) filters warnings inline instead of calling readResponseWarnings, and it backs the SDK client's snapshot result. The kernel doc in packages/kernel/src/success-text.ts says every response-warnings reader uses readResponseWarnings. Behavior is the same today, but a later change to the warnings contract would miss the snapshot path, and the doc would mislead the next reader. Can this call switch to Array.isArray(data.warnings) ? readResponseWarnings(data) : undefined, or should the doc name the snapshot reader as an intended exception?

All 20 checks pass on 027eae4, and there are no conflicts. The next step is to settle the snapshot reader question either way.

[thymikee]

Rebased onto main (4b15606, incl. the #2590 schema-layer and #2618/#2619 android waves) and settled the snapshot-reader question — delegation turned out to be closed off, so the doc names the exception:

Why not delegate: readSerializedSnapshotCaptureAnnotations sits inside the eager module closure of 26-31 pinned package facades (scripts/__tests__/eager-closure-budgets.test.ts). Any runtime edge into the shared parser — I tried both kernel/success-text and kernel/record — adds a newly-evaluated module to every one of them, and the gate's contract is zero eager-closure growth (its own message offers only "existing home" or a lazy edge; this reader is synchronous). So: contracts keeps its local filter, readResponseWarnings' doc names it as the single declared exception with the reason, and a new packages/contracts/src/snapshot-capture-annotations.test.ts cross-checks the annotations filter against the shared parser on adversarial arrays (['a note', 42, {nested}, null], ['', 'kept'], …) — a future contract change that misses the snapshot path trips that pin. The serialized []-to-absent mapping is pinned separately.

Gate status: everything I can run locally is green on 4b15606: unit-core (2258), apple-runner, output-economy, maestro-compat (388), wire/replay/command-docs compat, layering, production-exports, fallow, tmpdir, lint, budgets. check:affected --run stops at typecheck because main itself is broken at 2cafab3: #2619 re-added installArgs: ['install', '-r'] to snapshot-helper-install.test.ts after #2618 removed the field (TS2353, one error; reproduced in a clean origin/main worktree; this diff touches zero android files). Two further pre-existing main failures reproduced identically in that clean worktree: runner-recovery-wiring.test.ts alert-retryability (2 tests) and the scripts/mutation ownership model. CI on this head will show the same main-side typecheck failure until main heals.

[thymikee]

Reviewed at 4b15606, as a follow-up to the review at 027eae4. The earlier finding is not fully closed.

The doc at success-text.ts#L17 says every reader that projects a response record onto warnings goes through readResponseWarnings, with the contracts annotations reader as the only exception. But src/daemon/snapshot-quality-latch.ts lines 95-96 still read Array.isArray(data.warnings) ? data.warnings : [] directly, keep non-string entries, and prepend the recovered-snapshot warning without the shared parser. That code is already on main, so this PR did not break it, but the new doc makes a claim that is not true. If the warnings contract changes later, the latch will miss it. git grep 'data.warnings' under src/ and packages/ lists the sites. Can you route line 95 through readResponseWarnings(data), or name it in the doc as an exception with a reason?

Typecheck & Package fails with TS2353 on installArgs in packages/platform-android/src/__tests__/snapshot-helper-install.test.ts. This PR does not touch that file, and main fails the same way at 2cafab3 because #2618 removed the field while that test still sets it, so the failure is unrelated to this PR. A rebase will not clear it until main is fixed.

Smoke Tests fails because the long-press canary on the live iOS fixture is not found after scrolling. The only PR change near that route, packages/platform-apple/src/runner/snapshot-presentation.ts, adds systemSurface to a thrown error's details and does not touch scrolling or selector matching, and 027eae4 passed all checks. It looks unrelated, but a rerun should confirm it.

Next: fix or document the latch reader, then rerun Smoke Tests.

[thymikee]

Latch routed at 262b68a: applyRecoveredWarningLatch now prepends through readResponseWarnings(data), so foreign non-string entries are dropped on write-back instead of echoed to the reader — pinned by a new latch test. The kernel doc lists the latch among consumers; the contracts annotations reader remains the only declared exception. Also took the eight new main commits (clean rebase; #2621/#2600 touched the same Apple area without conflict). pnpm check:affected --run now passes end-to-end on this head — main's typecheck break (installArgs) and the mutation-ownership failure are both healed upstream, so the earlier unresolved failures are closed. Smoke reruns automatically on 262b68a.

[thymikee]

Reviewed at 262b68a. The finding from 4b15606 is fixed: applyRecoveredWarningLatch now reads through readResponseWarnings(data), the new latch test fails on the old code, and no other raw response-warnings reader is left outside the documented contracts exception. Commits 1-3 are unchanged since the earlier reviews.

All checks pass and there are no conflicts. This is ready for human review.