feat(replay): recorded landmark identity for wait, is coverage — read-only step identity (#1349)#1381
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact head
Descriptor phase → replay verifier → daemon-only landmark guard → snapshot-polling wait → bounded divergence conversion otherwise looks correct: impostors keep polling, ordinary timeout remains intact, and guarded recording/replay disables direct iOS fast paths as intended. Exact-head CI is green and merge state is clean. No ready label. |
|
Both points addressed at head 1. P2 — stray Android binaries: fixed
2. Live backend evidence — attached, and it earned its keepRan the full ADR 0016 loop on real backends (isolated Android — Pixel 9 Pro XL API 37 emulator,
iOS — iPhone 17 Pro simulator:
Live-found product fix shipped in Pre-existing gaps found live, reproduced at merge base, filed instead of absorbed:
Exact-head CI is green apart from the in-flight run for 🤖 Addressed by Claude Code |
c249bcd to
e1f601c
Compare
…c/replay Move buildIndexMap/buildAncestryChain/filterIdentitySet out of the daemon's session-target-evidence into the shared replay zone so the commands runtime (wait's polling loop, #1349) can consume them without importing the daemon; press-retarget drops its private buildIndexMap duplicate.
…attern coverage for is (#1349) - New CommandDescriptor trait targetIdentityVerification pins the evidence-carrying command set and routes wait to a post-resolution phase so an annotated wait never enters the generic pre-dispatch verification (an absent landmark is its expected starting condition). - wait <selector> records landmark-mode target-v1 evidence (existence self-check; identity-empty matches record no annotation) and, on replay, keeps polling until a selector match carries the recorded identity; a deadline with only impostor matches fails closed as an identity-mismatch REPLAY_DIVERGENCE, a recorded-unverifiable annotation refuses before polling, and a plain timeout stays an action-failure divergence. - is (except exists) joins the get pattern: evidence at record time, generic pre-dispatch verification, and the post-resolution guard threaded through dispatch; direct-iOS fast paths for wait/is are gated during recording and guarded replays. - Read-only find stays intentionally unannotated (fuzzy-locator resolution has no selector-chain identity token), proven by test.
…ark identity (#1349) A qualifying ADR 0016 guard is now a selector wait whose target-v1 annotation is verified; identity-less or unverifiable guards are refused with a recovery hint. Adds the reshuffled-screen false-pass regression: record -> publish -> replay against a same-label/different-ancestry tree diverges as identity-mismatch (matchCount >= 1 proving the selector alone would have false-passed).
…m evidence-writer complexity Shared buildPostDispatchIdentityMismatchResponse behind the guard and wait-landmark conversions; extracted payload-ceiling helpers from computeTargetEvidence; identity-refusal conversion split out of resolveReplayStepResponse. Docs: ADR 0012 decision 3 amendment (#1349), ADR 0016 guard strengthening, help workflow/save-script text.
…xplicit, trim ADR-restating docs The landmark-mode self-check was provably a tautology (the winner is a member of its own identity set whenever the parent walk is intact), so a membership scan defended only by a comment is replaced with the explicit decision: broken walk fails closed, landmark is verified by construction, action mode keeps decision 3's step-5 self-check. Doc comments that re-argued the ADR amendment now state behavior and point to it.
…st dirs Generated Android helper output swept into the earlier refactor commit by accident; analogous snapshot-helper/ime-helper build dirs were already ignored.
…ve-validated on Android) Live ADR 0016 validation on a Pixel emulator showed a destination-guard wait replayed immediately after a navigation press deterministically dies: the first poll's capture lands mid-transition and the Android helper's 'insufficient foreground app content' verdict threw out of the polling loop. iOS already yields the same state as a sparse verdict with no matches, so the loop kept polling there — this makes wait semantics platform-consistent. A content-verdict capture failure (isUnreadableCaptureContentError) now counts as a no-match poll for selector and text waits; a wait whose screen never became readable rethrows the last capture verdict at the deadline, so persistent breakage keeps its diagnosis. Other capture failures still throw immediately.
e1f601c to
88ba8cd
Compare
…nism failure Address review feedback on #1386: the bounded retry was retrying every thrown capture-failed, including permanent mechanism failures (e.g. a missing helper artifact) that a retry can never fix, at the cost of the full backoff budget before the identical divergence. Gate the retry on the same signal Android's helper capture path already emits for this exact distinction: retriable:true on a content-poor rejection (rejectAndroidHelperContentUnavailable), unset on a permanent one (androidSnapshotHelperUnavailableError). The non-throwing sparse-snapshot verdict still always retries. Mirrors #1381's isUnreadableCaptureContentError taxonomy for the wait keep-poll loop. Also update the two #1385 tests to use a retriable-tagged error and add a case proving a permanent failure fails on the first attempt.
|
Reviewed exact head 88ba8cd. P1 — narrow unreadable-capture classification to actual content verdicts. isUnreadableCaptureContentError currently treats any string androidSnapshotHelperFailureReason as content-unreadable, but Android attaches that field to general helper mechanism failures too (timeouts, adb/helper failures, malformed output), not only content recovery. Selector/text waits therefore poll those failures until their deadline instead of failing immediately; the synthetic non-content test misses this because it omits the field used by the real wrapper. Match only the actual content-recovery reasons (empty-helper-output, system-window-only, content-poor-app-window) and add a realistic wrapped mechanism-error regression. The generated binaries are removed, live iOS/Android evidence is credible, and exact-head CI is green; no ready label until this is fixed. |
… content verdicts Android stamps androidSnapshotHelperFailureReason on mechanism failures too (helper timeouts, adb failures, missing helper artifact — free-form reason strings), so matching any string made waits poll those to their deadline instead of failing immediately. The predicate now matches only the enumerated content-recovery reasons, and AndroidHelperContentRecoveryDecision derives its reason union from the same list so a new content verdict cannot miss the predicate. Adds the realistic wrapped mechanism-error regression the synthetic test missed.
|
P1 addressed at head
Gates: tsc, oxlint, oxfmt, layering (the new 🤖 Addressed by Claude Code |
|
Re-reviewed exact head f7ae97b. The implementation correctly narrows content-unreadable capture reasons to the shared enumeration and prior live evidence remains valid, but the new mechanism-failure regression is vacuous: it only asserts the eventual error message. Reverting to the old broad classifier would poll the same repeated fixture error until the fake-clock deadline, rethrow it, and still pass. Assert exactly one capture attempt (or no clock/poll advancement) so reverting the classifier fails the test. Exact-head CI is green and the branch is conflict-free; no ready label until the regression proves fail-fast behavior. |
…-sensitive Assert exactly one capture attempt: the broad any-string classifier would poll the repeated fixture error to the fake-clock deadline and rethrow the same message, passing the message-only assertion. Verified the mechanism test fails against the broadened classifier and passes against the narrowed one.
|
Addressed at head Both fail-fast tests (the bare non-content error and the realistic wrapped mechanism error) now count capture attempts through a shared throwing-device helper and assert exactly one attempt — the fail-fast claim, not just the eventual message. Verified revert-sensitivity directly: with the classifier temporarily broadened back to any-string, the mechanism regression fails (17 attempts ≠ 1) while everything else passes; restored, all 26 file tests green. Gates clean (tsc, oxlint, oxfmt, fallow). 🤖 Addressed by Claude Code |
|
Re-reviewed exact head |
thymikee
added
the
ready-for-human
|
1 participant
Closes #1349.
What
Extends ADR 0012's
target-v1identity verification to the eligible read-only steps without changing their execution semantics, and strengthens ADR 0016's destination guard from selector existence to recorded landmark identity. Four commits, sequenced dependency-first:refactor(replay): shared tree helpers — moves the pure evidence primitives (buildIndexMap/buildAncestryChain/filterIdentitySet) from the daemon intosrc/replay/target-evidence-tree.tsso the commands-layer wait loop can consume them (commands → replaywas already a legal DAG edge); deletespress-retarget.ts's privatebuildIndexMapduplicate.feat(replay): wait landmark verification +iscoverage — the core.feat(publication): guard requires verified evidence + the reshuffled-screen false-pass regression.refactor(replay): dedupe/complexity cleanup shaken out by the fallow gates, plus ADR/help docs.Design decisions a reviewer should check
.adformat: what's new istargetIdentityVerification: 'pre-dispatch' | 'post-resolution'onCommandDescriptor, pinning the evidence-carrying set ({click, press, longpress, fill, get, is} → pre-dispatch; wait → post-resolution) with a parity test. An annotated wait therefore structurally cannot enter the generic pre-action path — the issue's key acceptance criterion. Only ADR 0012 path 1 (recorded-unverifiable, which consults no screen) still refuses a wait before polling.identity-mismatchREPLAY_DIVERGENCE(matchCountfrom the last matching poll), while a never-matched selector stays the plain timeout (action-failure→ state-repair routing, unchanged). Positional signals (sibling/viewportOrder) are deliberately not compared — a guard proves the landmark exists, not that it kept its list position, and comparing position would false-fail legitimately reshuffled-but-correct screens.computeTargetEvidence(..., {mode:'landmark'})self-checks identity-set membership (mirroring what replay verifies); an identity-empty match (no id, no label after Android replay: non-unique framework idandroid:id/titleused as primary identity → systematic false divergences (FDR 1.0) #1269 demotion) records no annotation — fail-open to today's semantics, and publication then refuses it as a guard with a hint naming a labeled landmark.is(exceptexists) = the exactgetpattern: evidence from its resolved node, generic pre-dispatch verification,assertExpectedResolvedTargetguard threaded through dispatch. Direct-iOSwait/isfast paths are gated during recording and guarded replays (same rulegethas had since feat: parse and preserve .ad target-v1 evidence (ADR 0012 migration step 3) #1196; authoring-only cost).is exists, all read-onlyfindvariants (fuzzy-locator resolution has no selector-chain identity token for the classifier),wait text/stable/duration/@ref(wait @refstays rejected by publication, never converted).verification: "verified".Wire/plumbing
The recorded landmark travels the daemon-only
internal.replayLandmarkGuardchannel (unspoofable, same asreplayTargetGuard); the loop's deadline refusal isreason: wait_landmark_identity_mismatchwith a compact{matchCount, observed, observedAncestry}payload, converted by the step loop into the standard bounded divergence (shared shaping with the guard-mismatch path viabuildPostDispatchIdentityMismatchResponse).node/preActionNodesare stripped before bothsession.actionsand the wire on every path.Validation
isrecord/verify/guard, read-onlyfind+is existsdeferral proofs.matchCountomitted; loop refusal →identity-mismatchdivergence withrepairHint: caution; duration-wait annotation inert.session save-script(annotation lands in the.ad) → replay against a same-label/different-ancestry tree →REPLAY_DIVERGENCE identity-mismatchwithmatchCount ≥ 1(the proof the selector alone would have false-passed).Perf / size
No measurable regression: live unannotated wait polls differ by one small allocation; the annotated-replay identity check is O(tree) per poll, dwarfed by the poll's own snapshot capture. No new startup-parse weight (the commands bundle already reached the identity modules; the new shared module is ~80 dependency-free lines). The one deliberate cost: recording sessions use the snapshot path for
wait/isinstead of the direct-iOS runner query, which evidence capture requires.Docs: ADR 0012 decision-3 amendment (#1349) + migration row 11, ADR 0016 amendment,
help workflow/session save-scripttext.