fix(ios): pin tap-outcome corroboration probes to the baseline's backend - #1634
Conversation
The recorded-failure screens are exactly where the capture plan flips between XCTest and private-AX (the penalty boundary), so #1605's same-backend requirement failed closed right where XCTest tap false negatives actually happen: the baseline was captured via private-AX under penalty, the probe came back via tree, and a landed tap surfaced as XCTEST_RECORDED_FAILURE. In the AppControlBench bsky-16 run this fired four times, each sending the model into a re-observe/retry spiral. The comparison stays same-backend by design (backends are not comparable views of a screen); instead the probe is now CAPTURED the way its baseline was: a new internal preferredBackend option (never CLI-exposed) threads daemon -> runner, and a private-AX-preferred capture takes the exact penalized route — privateAX-first plan, 'deferred' verdict, no degradation warning, no settle budget reset. Live-verified on the deterministic repro (Bluesky drawer-menu press under penalty, seeded bench feed): errored with the backend-mismatch diagnostic before, corroborates as landed after, with no mismatch phase in the request diagnostics. Daemon tests cover pinned and unpinned baselines end to end through the dispatch context; the Swift plan gate is a pure function with an executed in-bundle test (added to the ios.yml regression list).
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact head
Keep CI classification separate, but do not apply |
thymikee
left a comment
There was a problem hiding this comment.
Reviewed against #1605/#1599 and the capture-reliability contract in CONTEXT.md.
The mechanism is the right one. Backends genuinely aren't comparable views of a screen, so fixing how the probe is captured rather than loosening hasMatchingPresentation is the correct side to change, and pinning only when the baseline is private-ax keeps the blast radius at the penalty boundary where the false negatives actually live. Threading is clean, and I confirmed snapshotPreferredBackend appears in no CLI grammar or option spec — getConfigurableOptionSpecs builds from flag definitions, so project config can't reach it. Worth stating explicitly given GHSA-m7q5-6423-2mwq was exactly about config-derived connection flags.
Four notes, none of them blocking.
1. The other half of the same gate is still fail-closed — and it's on-theme
hasMatchingPresentation rejects on two conditions. This PR fixes the cause of the backend one; the presentation one is still manufactured internally.
matchingCaptureFlags (interaction-ios-tap-outcome.ts:232-236 on this branch) returns undefined whenever params.flags is falsy, discarding the baseline's depth/scope/raw. The probe is then captured at default presentation and presentationKey can't match a non-default baseline — the same "landed tap reported as failure" outcome this PR is closing, through the sibling condition.
The asymmetry is visible in the function you just edited: :109 reads interactiveOnly: presentation?.interactiveOnly ?? true unconditionally, while depth/scope/raw are dropped one line up.
params.flags is req.flags, optional on DaemonRequest, and two production paths omit the key entirely — batch steps (src/cli/batch-steps.ts:89 spreads it away when undefined) and the JSON-RPC boundary (src/daemon/server/http-server.ts:170 passes through whatever the client sent). CLI and Node-client callers always populate it, so this is narrower than the backend case, but it defeats the rescue on exactly the "snapshot --scope <region>, then act inside it" flow.
Fix is small — if (!flags && !presentation) return undefined; plus ...(flags ?? {}). I have it written up with a regression test. Happy to fold it into this PR or stack it behind you; flagging mainly because whichever lands second will conflict in this function.
2. Test 1's behavioural assertions survive a revert
Applying the repo's own rule ("identify what deletion or revert would make it fail"):
In a private-ax baseline pins the corroboration probe to private-ax, the mock returns snapshotPayload(imageViewerNodes, 'private-ax') regardless of the context it's handed. So with the production change reverted, baseline and probe are still both private-ax, hasMatchingPresentation passes, and expect(response?.ok).toBe(true) plus the warning assertion still hold. The only revert-sensitive assertion is snapshotPreferredBackend === 'private-ax' — the wiring, at the layer the change was made.
That's acceptable here, because the Swift test proves pin → plan composition and the live before/after covers the end-to-end binding. But no automated test proves "pin ⇒ the probe actually comes back private-AX-backed", and the mock's hardcoded backend is what hides it. Worth a comment on the test so a later reader doesn't mistake it for that proof.
a tree baseline does not pin the corroboration probe backend passes identically before and after the change — it guards against future over-pinning, which is worth having, but it isn't evidence for this fix.
3. The capture-reliability contract now under-describes the mechanism
CONTEXT.md's corroboration bullet still says the daemon "may take one same-presentation post-action capture against a usable retained snapshot". It doesn't mention that the probe is now captured with a pinned backend on a deliberately penalized plan. That file is the durable vocabulary, and ADR 0004 owns capture-plan/verdict semantics — one line in each would stop the doc drifting from the code, per the repo's own "an ADR conflict is a review finding unless the PR updates it".
4. Nit: hand-mirrored test context builder
contextFromFlags in the test now mirrors daemon/context.ts by hand for a single field. That's a drift surface: if the corroboration path grows a dependence on another context field, the helper silently diverges while the tests stay green. Deriving it from the production builder would be sturdier if that's cheap to do.
Not independently verified by me: the claim that settle's budget reset doesn't fire for .deferredToIndependentBackend. I'm taking the live evidence on that one.
The Swift gate itself reads well — snapshotXCTestChannelTreatedAsPenalized is a pure function with a real truth table, preferredBackend: "tree" correctly doesn't pin, and keeping it inside #if os(iOS) matches the isLocalIosRunnerSession guard on the caller.
…o end (review) Raw baselines could not be pinned: the raw diagnostic plan keeps tree-first error propagation by contract and is never rerouted by the penalty or the preferred backend, so preserving 'raw: true' on the probe recreated exactly the backend-mismatch false failure this PR removes. Corroboration now declines raw baselines up front (they are diagnostics, not evidence baselines) with a regression pinning that no probe capture is dispatched at all. The wire is now regression-proven at every hop: a dispatch-level test drives dispatchCommand with the context flag and asserts the emitted RunnerCommand carries preferredBackend (red if handleSnapshotCommand or the interactor stops forwarding); the injected-transport test asserts the interactor's snapshot payload both ways; and a runner unit test decodes the wire JSON, projects it through the extracted snapshotOptions(from:), and composes it with the plan rule — pinned regular plan defers to privateAX-first, RAW plan stays untouched. Executed on-simulator; added to the ios.yml regression list.
|
Both findings addressed in b2ed38e: P1 — raw baselines: took the exclusion contract. P2 — wire proof at every hop:
Layer coverage now: daemon corroboration behavior (mocked dispatch) → dispatch→RunnerCommand (new) → provider transport (new) → decoded command→options→plan (new, executed) — no hop left unpinned. |
|
Re-reviewed exact head |
|
* 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
Why
#1605's corroboration rescues taps that XCTest falsely reports as failed — but its same-backend requirement fails closed exactly where those false negatives happen. The recorded-failure screens are the penalty-boundary screens: the baseline was captured via private-AX under penalty, the corroboration probe comes back via the tree backend,
hasMatchingPresentationcorrectly refuses the cross-backend comparison, and a tap that landed surfaces asXCTEST_RECORDED_FAILURE.Cost is measured: the AppControlBench bsky-16 success run hit this 4 times in 85 tools, each one sending the model into a re-observe/retry spiral (~16 wasted tool calls). The Bluesky drawer-menu press under penalty is a deterministic repro — it errored in every session tonight while provably landing (
ios_tap_failure_corroboration_backend_mismatch: baselineBackend=private-ax, afterBackend=tree).What
The comparison stays same-backend by design (backends are never comparable views of a screen). Instead, the probe is captured the way its baseline was: a new internal
preferredBackendoption (never CLI-exposed) threads daemon→runner, and a private-AX-preferred capture takes the exact penalized route — privateAX-first plan,'deferred'verdict, so no degradation warning renders and settle's budget reset does not fire for a deliberately pre-selected backend. Pinning only engages when the baseline backend isprivate-ax; tree/queries baselines behave as before.Evidence
Error (XCTEST_RECORDED_FAILURE)with the mismatch diagnostic; after —Warning: XCTest reported the tap as failed, but a same-scope post-action accessibility capture changed; treating the tap as landed, and the request diagnostics contain no mismatch phase.snapshotPreferredBackend: 'private-ax'and the tap corroborates; tree baseline → no pin; all pre-existing fail-closed cases (cross-backend, sparse, stale, keyless, presentation mismatch) unchanged.snapshotXCTestChannelTreatedAsPenalized, with an in-bundle test asserting pin→penalized-plan composition ([.privateAX],.deferredToIndependentBackend) — added to ios.yml's executed regression list. Ran on-simulator locally: passed.Part of #1599.