feat(replay): ADR 0012 migration step 4 — target-binding verification enforcement#1209
feat(replay): ADR 0012 migration step 4 — target-binding verification enforcement#1209thymikee wants to merge 3 commits into
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Current-head review found two blockers:
Also tighten locality before readiness: the new implementation is 598 LOC and its test is 557 LOC, exceeding the repo's 500-LOC extraction tripwire. Assigning a focused fixer for correctness and a concept-based split. |
…inner against the verified member Coordinator addition to step 4 (PR #1209): pre-action verification resolves without dispatch's occlusion-filtering/visibility-preferring guards, so verification's winner W could differ from the node dispatch actually acts on — verified-but-taps-a-different-element, or a false identity-mismatch. Close the split with a cheap cross-check at dispatch's own resolution: - Verified outcomes now carry the verified member's normalized local identity (`ReplayVerifiedTargetGuard`: readNodeLocalIdentity(winner) + verification's matchCount). The replay loop threads it onto the dispatch request as `internal.replayTargetGuard`. - Interaction handlers (press/click/longpress/fill in interaction-touch.ts, get in selector-runtime.ts) pass it into command options as `expectedResolvedTarget`, and a guarded dispatch skips the direct-iOS selector fast path and the native-ref tapTarget/fillTarget fast paths — the guard needs the runtime tree resolution to have a node to check. - `assertExpectedResolvedTarget` (resolution.ts) compares the resolution winner's identity — PRE-promotion (hittable-ancestor promotion deliberately retargets to the same element's actionable container) and PRE-action — against the verified identity, and refuses with `details.reason: 'replay_target_guard_mismatch'` on a mismatch. Enforced on the selector path, the ref path, and get's ref/selector reads. - The replay loop converts that refusal into an identity-mismatch target-binding divergence (`buildReplayTargetGuardMismatchResponse`): classification == kind, matchCount from verification's recorded-selector resolution (present, per the rule), recorded/observed identities, field-level mismatches, fresh screen observation — via the same bounding/sanitizing builder as every other target-binding divergence (finalize refactored into module-level buildTargetBindingDivergenceResponse for the two callers). - The identity reader moved to src/replay/target-identity-node.ts (readNodeLocalIdentity + localIdentitiesEqual + the reason marker) so the commands and daemon layers share it without a layering back-edge; session-target-evidence.ts's boundedLocalIdentity is now an alias. The guard is replay-only by construction: `internal.replayTargetGuard` is set exclusively by the replay step loop for annotated verified actions; live interactive commands never carry it. PR #1211 coordination: buildTargetBindingDivergenceResponse is the single resume site for target-binding divergences, marked as the step-5 wiring point — after #1211 merges, replace the not-supported stub with the real computed resume (pre-action divergences typically allow same-step resume) plus a test. Kept as a small follow-up by construction. Tests: split-resolver fixture (covered deeper A vs visible sibling B — verification verifies A, dispatch resolves B) proving the guard refuses pre-action with zero backend taps, the matching-identity pass-through, the ref-fast-path forcing, guard threading on the dispatch request (and its absence for unannotated actions), and end-to-end conversion of a guard mismatch into an identity-mismatch divergence with matchCount present.
|
Re-review of
The new regression covers the initial dispatch guard, not the update/heal bypass or ambiguous-first/later-resolvable selector chain. |
…inner against the verified member Coordinator addition to step 4 (PR #1209): pre-action verification resolves without dispatch's occlusion-filtering/visibility-preferring guards, so verification's winner W could differ from the node dispatch actually acts on — verified-but-taps-a-different-element, or a false identity-mismatch. Close the split with a cheap cross-check at dispatch's own resolution: - Verified outcomes now carry the verified member's normalized local identity (`ReplayVerifiedTargetGuard`: readNodeLocalIdentity(winner) + verification's matchCount). The replay loop threads it onto the dispatch request as `internal.replayTargetGuard`. - Interaction handlers (press/click/longpress/fill in interaction-touch.ts, get in selector-runtime.ts) pass it into command options as `expectedResolvedTarget`, and a guarded dispatch skips the direct-iOS selector fast path and the native-ref tapTarget/fillTarget fast paths — the guard needs the runtime tree resolution to have a node to check. - `assertExpectedResolvedTarget` (resolution.ts) compares the resolution winner's identity — PRE-promotion (hittable-ancestor promotion deliberately retargets to the same element's actionable container) and PRE-action — against the verified identity, and refuses with `details.reason: 'replay_target_guard_mismatch'` on a mismatch. Enforced on the selector path, the ref path, and get's ref/selector reads. - The replay loop converts that refusal into an identity-mismatch target-binding divergence (`buildReplayTargetGuardMismatchResponse`): classification == kind, matchCount from verification's recorded-selector resolution (present, per the rule), recorded/observed identities, field-level mismatches, fresh screen observation — via the same bounding/sanitizing builder as every other target-binding divergence (finalize refactored into module-level buildTargetBindingDivergenceResponse for the two callers). - The identity reader moved to src/replay/target-identity-node.ts (readNodeLocalIdentity + localIdentitiesEqual + the reason marker) so the commands and daemon layers share it without a layering back-edge; session-target-evidence.ts's boundedLocalIdentity is now an alias. The guard is replay-only by construction: `internal.replayTargetGuard` is set exclusively by the replay step loop for annotated verified actions; live interactive commands never carry it. PR #1211 coordination: buildTargetBindingDivergenceResponse is the single resume site for target-binding divergences, marked as the step-5 wiring point — after #1211 merges, replace the not-supported stub with the real computed resume (pre-action divergences typically allow same-step resume) plus a test. Kept as a small follow-up by construction. Tests: split-resolver fixture (covered deeper A vs visible sibling B — verification verifies A, dispatch resolves B) proving the guard refuses pre-action with zero backend taps, the matching-identity pass-through, the ref-fast-path forcing, guard threading on the dispatch request (and its absence for unannotated actions), and end-to-end conversion of a guard mismatch into an identity-mismatch divergence with matchCount present.
4ed7ef2 to
a3749f2
Compare
|
Rebased onto main after #1193 merged (branch force-pushed with lease — re-fetch if you were running against the old head
|
thymikee
left a comment
There was a problem hiding this comment.
Adversarial review of ADR-0012 migration step 4 (target-binding verification enforcement + the coordinator's post-resolution guard). Reviewed statically against docs/adr/0012-interactive-replay.md decisions 3/4 and live on iPhone 17 Pro sim (C25DBB5B), RN playground (org.reactnavigation.playground), Metro 8082. Also reviewed the post-rebase pin commit (a3749f26a, test-only, confirms the guard doesn't suppress #1193's resolution disclosure).
Findings
1. (Medium, verified-sound-with-caveat) Pre-promotion guard comparison doesn't guarantee the physical tap stays inside the verified element's own container
assertExpectedResolvedTarget runs on resolved.node before resolveActionableNodeOrThrow's hittable-ancestor promotion (resolution.ts:188,250), and verification (classifyReplayTarget) never promotes either — so both sides of the guard compare the same pre-promotion leaf. That part is internally consistent.
The residual risk is in the promotion step itself, not the guard: resolveActionableTouchResolution (src/core/interaction-targeting.ts:48-74) climbs to findNearestHittableAncestor — the first ancestor with hittable === true — and only refuses via isOverlyBroadAncestor when that ancestor covers ~90%+ of the viewport. Nothing checks that the promoted ancestor is exclusively the verified leaf's own container (e.g. it could be a list-row wrapping two distinct buttons, if the leaf itself isn't independently hittable). In that shape, the guard passes (leaf identity matches), but the coordinates actually tapped are the promoted ancestor's center — which is not guaranteed to be "the same element's actionable container" as the PR's comment (resolution.ts:50-51) asserts. That comment states an invariant the promotion code doesn't itself enforce. Worth either a stronger isOverlyBroadAncestor-style check (reject promotion into an ancestor containing another semantic-touch-role descendant) or softening the code comment's claim. Not a regression introduced by this PR (the promotion logic predates it), but this PR's guard is exactly the mechanism whose safety story leans on that unverified assumption.
2. (Informational, live-reproduced) AX role instability produces a real false-positive identity-mismatch on an unrelated, functionally-unchanged screen
Recording a click "Push Input" on the RN playground's Article screen and replaying it moments later (same screen, no navigation) reproducibly diverged as identity-mismatch: ancestry[0]: recorded=scrollview/Push Input observed=other. Direct snapshot inspection confirmed the same physical container (height 2194px — clearly the screen's ScrollView) is typed scrollview in one capture and Other in the very next, coinciding with the daemon's own "fell back to the private-ax snapshot backend" notice for this deep/complex tree (matches the known bluesky-sparse-ax-root-cause class of issue in this repo). This isn't a defect in this PR's classification code — classifyReplayTarget/classifyTargetBindingMatch faithfully implement decision 3 — but decision 3's own premise ("record and replay compute this ordinal identically by definition") assumes stable AX role reporting across captures, which doesn't hold under this pre-existing backend-fallback condition. Net effect: on deep/complex RN trees (exactly this repo's target apps), step 4 will sometimes fire a spurious pre-action divergence on a screen that hasn't actually changed. Worth a follow-up chip against the AX role-reporting instability itself; not a blocker for this PR since it inherits rather than introduces the instability, and fail-closed (refusing the action) is the correct response given the ADR's own design philosophy.
3. (Verified sound) Guard cannot be spoofed from the wire, and cannot cause a false negative
internal.replayTargetGuard lives on DaemonRequestInternal (src/daemon/types.ts), which is not part of the public wire DaemonRequest (src/kernel/contracts.ts:102-110), and toDaemonRequest in http-server.ts:163-174 builds the internal request field-by-field (allowlist), never spreading client JSON — so no client input can set or forge this field. Only one call site sets it (session-replay-runtime.ts:151), gated on a passed verification outcome. Even a hypothetical forged guard could only ever add a stricter check (fail-closed on mismatch), never bypass one — there's no code path where its presence weakens what would otherwise be checked.
4. (Verified sound) Zero device action on divergence, traced end-to-end
Live ndjson request trace for an induced identity-mismatch divergence shows exactly one ios_runner_command_send → snapshot_capture pair for the step, then request_failed — no tap/press runner command anywhere in the trace. Combined with the unit-level "zero backend taps" assertions in the split-resolver tests and the fact all four fast paths (direct-iOS tap/fill, native-ref tapTarget/fillTarget) are unconditionally skipped when replayTargetGuard is set (interaction-touch.ts:161-172,548-557, interactions.ts maybeTapRefTarget/maybeFillRefTarget), forcing every guarded dispatch through resolveInteractionTarget, where assertExpectedResolvedTarget throws strictly before any runtime.backend.* call — this holds uniformly across platforms since the guard is checked in platform-agnostic command code, not per-backend.
5. (Verified sound) Identity normalization is shared byte-for-byte across writer/verifier/guard
readNodeLocalIdentity (target-identity-node.ts) is the literal function aliased as boundedLocalIdentity in the record-time writer (session-target-evidence.ts:124) and used directly in the dispatch-side guard (resolution.ts) — same NFC/trim/whitespace-collapse/256-byte-cap normalization on every path. A label differing only in whitespace between captures cannot trip the guard.
6. (Verified sound) Six-path mapping, wire contract, capture cost
classifyTargetBindingMatch (target-identity.ts:425-458) matches the ADR's paths 2–6 exactly, including the signal-isolated-wrong (path 5/6-compare) vs no-signal-isolation (true path-6 fall-through, ≤5 candidates, document order) split. matchCount presence rule is correct (omitted only for path-1 recorded-unverifiable via a real undefined, never null). recorded/observed/mismatches/candidates all route through createReplayDivergenceSanitizer. Live trace confirms verification performs exactly one extra capture per annotated step via the existing captureDivergenceObservation (no settle/stable machinery invoked, no per-signal recapture). Fast-path-forcing is directly unit-tested (tapTargets empty, taps.length === 1 when guarded) and the pre-existing native-ref fast-path regression suite (unmodified by this PR) continues to cover the unguarded case.
Live evidence summary
- Real annotation recording verified correct per decision 3's format (id-less nodes, ancestry, sibling, viewportOrder, scrollRegion, rect,
verification: "verified"). - Reproduced both
selector-miss(app on splash screen at capture) andidentity-mismatch(finding #2 above) divergences live, both pre-action, both zero-tap (confirmed via ndjson trace), both carrying correctkind/targetBinding/screen evidence. - Did not obtain a fully clean end-to-end pass live — this RN playground app's own navigation-state persistence (documented in the ADR's Context section as a known property of this exact app) combined with finding #2 made a deterministic "nothing changed, all steps verify" run hard to land in the session time available. Given #2 reproduces independent of navigation state, I'm confident this is capture-backend variance, not a step-4 correctness defect — but a maintainer with more runway on this device might want a clean-pass artifact for the record.
Gates
All local: pnpm typecheck, lint, format:check, check:layering (775 files, no back-edge growth), check:fallow (18 changed files, no issues), check:production-exports — all clean. pnpm test:unit (3742 tests) and pnpm test:integration:provider (120 tests): 5 failures on first parallel run, all confirmed CPU-contention flakiness (pass clean in isolation; none touch this PR's files) — matches this repo's known unit-suite-flaky-under-contention pattern.
gh pr checks 1209: all completed checks green (Typecheck, Lint & Format, Layering Guard, Fallow Code Quality, Integration Tests, Coverage, Smoke Tests ×N, Bundle Size, CodeQL, Swift Runner Unit Compile, iOS Runner Swift Compatibility); a couple of Smoke Tests/Coverage shards were still in flight at review time with no failures observed.
Verdict
No blockers. Finding #1 (promotion-exemption assumption not structurally enforced) is worth a follow-up — either tighten isOverlyBroadAncestor-style promotion policy or soften the code comment's claim to match what the code actually guarantees. Finding #2 (AX role instability under the private-ax fallback) is a real, live-reproduced false-positive-divergence source but predates this PR and is orthogonal to its own logic, which faithfully implements the ADR. Everything else checked out sound: no guard false negatives, no wire spoofing surface, zero-device-action on every dispatch path, correct six-path/wire mapping, and capture cost exactly as documented.
|
Fix re-review at I am still withholding
|
|
New sequencing blocker after #1210 merged: this branch is now DIRTY against current |
…inner against the verified member Coordinator addition to step 4 (PR #1209): pre-action verification resolves without dispatch's occlusion-filtering/visibility-preferring guards, so verification's winner W could differ from the node dispatch actually acts on — verified-but-taps-a-different-element, or a false identity-mismatch. Close the split with a cheap cross-check at dispatch's own resolution: - Verified outcomes now carry the verified member's normalized local identity (`ReplayVerifiedTargetGuard`: readNodeLocalIdentity(winner) + verification's matchCount). The replay loop threads it onto the dispatch request as `internal.replayTargetGuard`. - Interaction handlers (press/click/longpress/fill in interaction-touch.ts, get in selector-runtime.ts) pass it into command options as `expectedResolvedTarget`, and a guarded dispatch skips the direct-iOS selector fast path and the native-ref tapTarget/fillTarget fast paths — the guard needs the runtime tree resolution to have a node to check. - `assertExpectedResolvedTarget` (resolution.ts) compares the resolution winner's identity — PRE-promotion (hittable-ancestor promotion deliberately retargets to the same element's actionable container) and PRE-action — against the verified identity, and refuses with `details.reason: 'replay_target_guard_mismatch'` on a mismatch. Enforced on the selector path, the ref path, and get's ref/selector reads. - The replay loop converts that refusal into an identity-mismatch target-binding divergence (`buildReplayTargetGuardMismatchResponse`): classification == kind, matchCount from verification's recorded-selector resolution (present, per the rule), recorded/observed identities, field-level mismatches, fresh screen observation — via the same bounding/sanitizing builder as every other target-binding divergence (finalize refactored into module-level buildTargetBindingDivergenceResponse for the two callers). - The identity reader moved to src/replay/target-identity-node.ts (readNodeLocalIdentity + localIdentitiesEqual + the reason marker) so the commands and daemon layers share it without a layering back-edge; session-target-evidence.ts's boundedLocalIdentity is now an alias. The guard is replay-only by construction: `internal.replayTargetGuard` is set exclusively by the replay step loop for annotated verified actions; live interactive commands never carry it. PR #1211 coordination: buildTargetBindingDivergenceResponse is the single resume site for target-binding divergences, marked as the step-5 wiring point — after #1211 merges, replace the not-supported stub with the real computed resume (pre-action divergences typically allow same-step resume) plus a test. Kept as a small follow-up by construction. Tests: split-resolver fixture (covered deeper A vs visible sibling B — verification verifies A, dispatch resolves B) proving the guard refuses pre-action with zero backend taps, the matching-identity pass-through, the ref-fast-path forcing, guard threading on the dispatch request (and its absence for unannotated actions), and end-to-end conversion of a guard mismatch into an identity-mismatch divergence with matchCount present.
96141ed to
12204cb
Compare
|
Rebased onto current main after #1210 merged (architecture-ownership consolidation). Branch force-pushed with lease — re-fetch if running against the old head; new head is Non-trivial resolutions:
Full gate re-run post-rebase: typecheck / lint / format / layering (794 files) / fallow / production-exports clean; 3769 unit tests (406 files) and 121 provider-integration tests (33 files) green. Ready for #1211 to merge first as planned — I'll do the one small follow-up rebase to wire |
…lution guard + step-5 resume wiring Rebuilt onto current main (#1210 selector relocation, #1211 resume + --update retirement, #1213, #1215) as one coherent commit — the prior 4-commit history collided structurally with #1211's own session-replay-runtime split. Step 4 (decision 3, enforcement): every annotated resolved-target replay/test action is verified against a fresh pre-action snapshot BEFORE dispatch (verifyReplayActionTarget → classifyReplayTarget, decision 3's six paths). Only a verified outcome sends the action; selector-miss / identity-mismatch / identity-unverifiable each return a complete target-binding REPLAY_DIVERGENCE built from its own capture, with targetBinding {classification==kind, matchCount per the presence rule, recorded/observed, mismatches, candidates} through #1211's shared failure-response + bounding/sanitizer pipeline. Post-resolution guard: a verified outcome mints internal.replayTargetGuard (the verified member's identity); dispatch's own resolution (occlusion/ visibility guards verification does not replicate) must land on the SAME element or assertExpectedResolvedTarget refuses pre-action, which the loop converts to an identity-mismatch divergence. Guarded dispatch skips the direct-iOS and native-ref fast paths so a tree node exists to check. Step-5 resume wiring (this rebase): the target-binding divergence resume stub is replaced with #1211's buildReplayDivergenceResume — a pre-action divergence resumes AT the failed step (from = step index, real SHA-256 planDigest, allowed unless preflight rejects the skipped range). buildTargetBindingDivergenceResponse is the single resume site; planActions/planDigest are threaded from the loop. --update healing is retired by #1211, so the in-branch "verify healed action" work is dropped (its module and test removed as moot). Tests: classification (13), guard (4), token (5), runtime end-to-end (9, incl. old-script pass-through, guard threading + mismatch conversion, and a real computed resume assertion), plus target-binding wire tests in divergence.test.ts (classification==kind, matchCount presence, overflow-fallback targetBinding, computed-resume-not-stub, text report). Also repoints daemon-http-disconnect.test.ts already handled upstream by #1215.
12204cb to
41b61c8
Compare
|
Rebased onto current main (now includes #1211, #1215, #1213) + resume-stub wiring done. Branch rebuilt as one coherent commit — the prior 4-commit history collided structurally with #1211's own Key points:
Full gate green: typecheck / lint / format / layering (798 files, zero back-edges) / fallow / production-exports. 3798 unit tests (412 files) and 121 provider-integration tests (33 files) pass, including #1211's own resume/plan-digest suites. @coordinator — green and ready for the re-review pass on the resume wiring. |
|
Current-head blocker at P1 — the post-resolution guard can still authorize a different duplicate element. ADR 0012 path 6 may use Carry enough structural denotation for the verified member into the guard and compare it against the dispatch capture before promotion. Add an end-to-end regression with two same-local-identity candidates where verification denotes A, dispatch resolves B, and assert |
Final review + live verification evidence (head 41b61c8)Scoped final pass per the coordinator's request: (A) the resume-wiring delta only (correctness engine already reviewed clean in prior rounds), and (B) the live device evidence gap noted for (A) Static — resume-wiring deltaTraced the full wiring:
Verdict: resume-wiring delta is correct. (B) Live evidenceBuilt the branch in the scratch worktree ( 1. Recorded 2. VERIFIED PASS — replay on the matching (freshly recorded) app state: All 4 annotated steps verified and executed cleanly — no false positives on a matching screen. 3. TARGET-BINDING DIVERGENCE, PRE-ACTION — navigated the app to the Article screen (via an unrecorded manual press, outside the script), then replayed a single-action script carrying the real recorded annotation for "Discard and go back" (which only exists on the Input screen, not Article). Real "divergence": {
"kind": "selector-miss",
"cause": { "code": "SELECTOR_MISS", "message": "The recorded target no longer matches any current element." },
"resume": { "allowed": true, "from": 1, "planDigest": "a6327965e5bea5f08e29a97c87e7b95f59897eff4d4f09a0129ea7b04fdfdbfb" },
"targetBinding": {
"classification": "selector-miss",
"matchCount": 0,
"recorded": { "role": "button", "label": "Discard and go back" },
"mismatches": [],
"candidates": []
}
}
Zero-tap proof — the request's per-request ndjson ( No 4. Guard path — skipped. Forcing dispatch's occlusion/visibility resolver to disagree with verification requires an engineered overlapping-sibling layout; not cheaply reachable in the RN playground's real screens. The guard path already has dedicated unit/integration coverage ( OverallAPPROVE-worthy. Resume wiring is correct (from = failed step index, real preflight, single-source digest). Live evidence confirms both the happy path (clean verified replay) and the safety property (pre-action divergence blocks the device tap, with a real computed resume object). CI is green (all checks passing, including previously-pending Smoke Tests). One non-blocking follow-up: add explicit |
…ard-mismatch paths
Non-blocking review follow-up: only the selector-miss runtime test asserted
the computed resume {allowed, from, planDigest}. All three target-binding
paths funnel through buildTargetBindingDivergenceResponse (one shared resume
site), so this is coverage not a bug — but pin resume on the identity-mismatch
and guard-mismatch tests too so no path can regress its resume wiring silently.
|
Closed the non-blocking resume-coverage gap: added the computed |
…rd (P1)
The guard carried only LOCAL identity {id, role, label}. ADR path 6 isolates
ONE member among several SAME-local-identity nodes via sibling / region-scoped
viewportOrder — a discriminator lost at the guard. So if verification denoted
duplicate A but dispatch's occlusion/visibility filter selected duplicate B
(identical id/role/label), the guard passed and SENT the action on the wrong
element — the exact verified-but-taps-different-node mis-binding step 4 exists
to prevent.
Fix: replayTargetGuard now carries a structural denotation of the verified
member — its pre-order document index (decision 3's canonical total order,
`node.index` after the shared reindex pipeline) plus its same-parent sibling
ordinal. `assertExpectedResolvedTarget` compares BOTH local identity AND
structural denotation against dispatch's pre-promotion capture, refusing
pre-action when EITHER differs. Two distinct duplicates always differ in
document order, so a different-member resolution can no longer pass. The
promotion exemption is preserved (comparison is on the verified LEAF, and
duplicates are distinct leaves). New helpers live in target-identity-node.ts
(replay zone) so writer/classifier/guard share one sibling-ordinal
implementation; computeSiblingOrdinal now delegates to it.
The guard-mismatch divergence surfaces a `position:` mismatch line (from the
structural denotations) so `mismatches` is never empty when local identity is
identical.
END-TO-END regression (session-replay-target-guard.test.ts): two "Save"
buttons with IDENTICAL id/role/label, A covered+deeper (verification's winner),
B visible (dispatch's winner) — driven through the real interaction runtime
(`device.interactions.press`), the guard refuses pre-action with ZERO backend
taps AND zero native tapTargets, the refusal driven purely by the structural
denotation (both observedStructural/expectedStructural asserted). Plus a
same-structural passing case, a runtime-level `position:`-mismatch divergence
test, and updated guard-threading assertions for the {identity, structural}
shape.
Wire contract unchanged: replayTargetGuard is DaemonRequestInternal (never on
the wire); decision 4's targetBinding {recorded, observed} stay LOCAL identity
per decision 3's Identity tier — the structural denotation is an internal
guard concern, surfaced only as a `mismatches` string. No ADR change needed.
|
P1 fixed (head
ADR/wire-contract check (per your ask): no change needed. Also addresses your earlier finding #1 (promotion-exemption assumption): the guard no longer leans on "same element's container" for correctness — it compares the verified leaf's structural denotation, which distinguishes distinct leaves regardless of promotion. Full gate green: typecheck / lint / format / layering (798 files, zero back-edges) / fallow / production-exports. 3802 unit tests (412 files) and 121 provider-integration tests (33 files) pass. |
|
Re-review of P1 — raw The new duplicate regression is non-vacuous for the prior bug, but it passes the same |
Summary
Implements ADR 0012's migration step 4: target-binding verification enforcement (docs/adr/0012-interactive-replay.md, decision 3's "Replay-time verification" + decision 4's
targetBindingwire fields), plus the coordinator-requested post-resolution target guard closing the verification/dispatch resolver split.Pre-action verification (commit 1)
Every annotated resolved-target replay/test action (
click/press/longpress/fill/getcarrying a# agent-device:target-v1 {...}comment, per #1196) is verified before its device action is sent:verifyReplayActionTarget(new:src/daemon/handlers/session-replay-target-verification.ts) resolves the recorded selector/ref against a fresh pre-action snapshot, using the same per-command rect/disambiguation config heal's suggestion re-resolution already uses (resolveSuggestionMatchingConfig).classifyTargetBindingMatch, from feat: parse and preserve .ad target-v1 evidence (ADR 0012 migration step 3) #1196, unchanged). Only path 4 / path-6-verified sends the action.REPLAY_DIVERGENCE(from feat(replay): ADR 0012 migration step 2 — structured replay divergence report #1197's transport) withkind:selector-miss—matchCount == 0identity-mismatch— empty identity set, or a unique/isolated identity-set member differing from the resolution winneridentity-unverifiable— a recorded-unverifiableannotation (path 1, before any resolution), or several candidates sharing the identity with neither disambiguation signal isolating one (path 6 fall-through, ≤5 candidates in document order)targetBindingon the divergence:classification(==kind),matchCountper decision 3's conditional presence rule (omitted only for a recorded-unverifiable annotation),recorded/observedidentity, boundedmismatches, andcandidates— all sanitized through the existing scrub → redact → truncate pipeline.session-replay-runtime.ts), never into live interactive command dispatch.Post-resolution guard (commit 2, coordinator addition)
Verification resolves without dispatch's occlusion-filtering / visibility-preferring guards, so its winner W could differ from the node dispatch actually acts on — verified-but-taps-a-different-element. The guard closes that split pre-action:
internal.replayTargetGuard(the verified member's normalized local identity via the sharedreadNodeLocalIdentity, newsrc/replay/target-identity-node.ts) onto the dispatch request.expectedResolvedTarget; a guarded dispatch skips the direct-iOS selector fast path and the native-reftapTarget/fillTargetfast paths (the guard needs a runtime tree node to check).assertExpectedResolvedTarget(resolution.ts) compares the resolution winner — pre-promotion (hittable-ancestor promotion deliberately retargets to the same element's actionable container), pre-action — against the verified identity, refusing withdetails.reason: 'replay_target_guard_mismatch'on a mismatch. Enforced on the selector path, the ref path, and get's ref/selector reads.buildReplayTargetGuardMismatchResponse) carrying verification'smatchCount(present, per the rule), recorded/observed identities, and a fresh screen observation — through the same bounding/sanitizing builder as every other target-binding divergence.internal.replayTargetGuardis set exclusively by the replay step loop for annotated verified actions; live interactive commands never carry it.Cost decision + planned follow-ups
commands/interaction/runtime/resolution.tsso verification can share dispatch's literal resolution (including guards) instead of paying a second capture+resolve.buildTargetBindingDivergenceResponseis the single resume site for target-binding divergences and is marked as the step-5 wiring point. After feat(replay): ADR 0012 migration steps 5+6 — resume + --update retirement #1211 merges, this branch rebases and replaces theresume: not-supportedstub with feat(replay): ADR 0012 migration steps 5+6 — resume + --update retirement #1211's real computed resume — target-binding divergences fire pre-action, so same-step resume is typically allowed when preflight permits (computed, not assumed) — plus a test asserting a target-binding divergence carries a real computed resume object. Structured to be a one-site change.Test plan
classifyReplayTargetpure-core: all six verification paths, an extended real-capture fixture (bottomTabsRealCaptureFixture, react-navigation bottom tab bar, shared withsession-target-evidence.test.ts), same-parent sibling recurrence under different anonymous parents, region-partitioned viewportOrder domains, a recorded scroll region gone (unavailable, never compared cross-region), an out-of-range viewportOrder, and document-order determinism for equal rect centers.matchCount 2), dispatch's occlusion filter resolves B; the guard refuses pre-action with zero backend taps, a matching identity passes through, and a guardedclick @refis forced off the native fast path onto the runtime resolution path.runReplayScriptFile: the device action is never dispatched on a divergence; old unannotated scripts execute unchanged (and carry no guard); a verified target proceeds to dispatch with the guard on its request; a dispatch-time guard mismatch converts to an identity-mismatch divergence withmatchCountpresent and field-level mismatches.matchCountpresence rule (present 0..N / absent-never-null on recorded-unverifiable),targetBinding.classification === kind, overflow-fallback preservation oftargetBinding, text-report rendering of matchCount/mismatches/candidates.pnpm typecheck,pnpm lint,pnpm format:check,pnpm check:layering(774 files, no back-edge growth — the shared identity reader lives in the replay zone precisely to avoid adaemon → commandsback-edge),pnpm check:fallow,pnpm check:production-exports— all clean.pnpm test:unit(402 files / 3742 tests) andpnpm test:integration:provider(32 files / 120 tests) — all green (single slow-test-gate flag was CPU contention on an unrelated Apple screenshot retry test; clean in isolation).