feat: parse and preserve .ad target-v1 evidence (ADR 0012 migration step 3)#1196
feat: parse and preserve .ad target-v1 evidence (ADR 0012 migration step 3)#1196thymikee wants to merge 9 commits into
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
thymikee
left a comment
There was a problem hiding this comment.
Adversarial review — ADR 0012 migration step 3 (.ad target-v1 evidence, inert)
Reviewed src/replay/target-identity.ts, src/daemon/session-target-evidence.ts, the parser wiring in
src/replay/script.ts/script-formatting.ts, the response-shape plumbing in
interaction-touch-response.ts/interaction-common.ts/resolution.ts, and all new tests, line by line
against decision 3. Also independently re-ran/confirmed CI: gh pr checks 1196.
Blocking (CI is currently red, not flaky)
1. Coverage check fails — real regression, not a wire leak. preflightNativeRefInteraction
(resolution.ts:490-522) now spreads node/preActionNodes into the PressCommandResult/
FillCommandResult returned by the native-ref fast path (interactions.ts maybeTapRefTarget/
maybeFillRefTarget, ...preflight spread). test/integration/interaction-contract/native-ref.contract.test.ts
→ native-ref responseConstruction: fast-path result carries the canonical ref field set asserts
result.node === undefined at this exact layer and now fails (confirmed via gh run view --log-failed:
1/3777 tests failing, isolated to this one assertion). This is an ADR-0011-governed provider-contract test
(NATIVE_REF_COVERAGE guarantee registry), not an incidental unit test.
I traced the full path to confirm there is no actual public wire leak: buildInteractionResponseData
(interaction-touch-response.ts:128-138) attaches node/preActionNodes only to the internal
visualization object, never responseData; interactionResultExtra (interaction-touch-targets.ts:173-198)
is a hard allowlist (ref/refLabel/selectorChain/targetHittable/hint/evidence/settle) that never
copies them across; and extractTargetEvidenceForRecording (interaction-common.ts:102-117) strips them
before sessionStore.recordAction persists anything. So this is a stale contract test, not a security
issue — but it's a real, currently-failing gate the PR's own checklist doesn't mention (pnpm test only runs
the unit-core+android-adb vitest projects; this test lives under provider-integration, exercised only by
pnpm test:coverage/CI's Coverage job, which the PR apparently never ran locally). Needs either an updated
assertion (with a comment tying the new shape to ADR 0012 decision 3) or a guarantee-registry note.
2. Lint & Format check fails. pnpm format:check (oxfmt) reports formatting issues in exactly the 6
files this PR touches: src/daemon/__tests__/session-target-evidence.test.ts,
src/daemon/handlers/interaction-common.ts, src/daemon/session-target-evidence.ts,
src/replay/__tests__/script.test.ts, src/replay/__tests__/target-identity.test.ts,
src/replay/target-identity.ts. Trivial (pnpm format / oxfmt --fix), but currently a red CI check —
oxlint itself passes, so this is a separate, apparently-unrun gate.
Both confirmed independently via gh run view <run> --job <job> --log-failed (exact log excerpts pulled),
not just relayed from a subagent.
Design notes (not blockers for this inert step, but relevant before step 4 lands)
3. classifyTargetBindingMatch path 6 collapses two spec-distinct outcomes. Decision 3 says: when
sibling or region-scoped viewportOrder isolates exactly one member, "compare with W as in paths 4/5" — i.e.
if the isolated member ≠ winner, that's the same class as path 5 (identity-mismatch), distinct from true
non-isolation fall-through (identity-unverifiable, up to 5 candidates). The current return type
(target-identity.ts:416-421) only has {path:6, outcome:'verified'|'unverifiable'} — no reason,
unlike path 5's reason:'unique-but-wrong' — so both sub-cases collapse into one undifferentiated
unverifiable. Harmless today (the writer's self-check only reads the boolean outcome), but the module's
own docstring frames this exact function as what "a future migration step (4) reuses... so the two can never
drift" — as written it can't yet drive decision 4's kind/candidates wire shape without extension. Flag
for whoever picks up step 4, not this PR.
4. Overflow-reduction loop sizes the payload against a placeholder before the real value is known.
computeTargetEvidence's reduction loop (session-target-evidence.ts:89-94) measures
serializeTargetAnnotationV1(candidate) while candidate.verification is still the placeholder
'verified', then overwrites it with the real self-check result — without re-measuring. "unverifiable"
is 4 bytes longer than "verified" once serialized, so if a candidate's placeholder-sized payload lands
within 4 bytes of the 4096 cap and the real self-check yields 'unverifiable', the writer could emit a
payload its own parser would reject — violating the explicit "writer must never emit a payload its own
parser rejects" invariant. The terminal/floor branch (lines 95-107) does re-check and drops rect as a
fallback; the general loop body does not apply the same defense.
I traced the self-check's actual reachability and believe this is very hard to hit in practice: by
construction, the winner is always a member of every candidate set the self-check derives from the winner's
own computed sibling ordinal / region partition, so runRecordTimeSelfCheck returns 'verified' on every
normal path — 'unverifiable' only arises from a genuine capture anomaly (e.g. duplicate node indices),
exactly the rare case decision 3 calls out as a fail-closed safety valve. But that's precisely when the
missing re-check would matter. Recommend applying the same recheck-and-drop-rect treatment uniformly, or
sizing against the worst-case ('unverifiable') string length up front.
Verified sound (spec fidelity, line by line against decision 3)
- Canonical field order, NFC + label trim/collapse, case sensitivity, empty-after-normalization omission —
all correct and tested (serializeTargetAnnotationV1field-order test, NFC/whitespace test). - 256-byte field / 4096-byte payload / K=8 ancestry / non-negative-safe-integer bounds all enforced by the
parser withINVALID_ARGS, including exact boundaries: 4096 bytes accepted, 4097 rejected;2^53and
floats/negatives rejected forsibling/viewportOrder; writer-side truncation is surrogate-pair-safe
(dedicated test at the exact 3-byte/4-byte-emoji boundary). - Parser accepts any field order, ignores unknown fields, treats
target-vN(N≠1) as an ordinary comment,
and correctly rejects a line that merely mentions the tag in prose (regex is start-anchored). - Binding rule (
parseReplayScriptDetailedinscript.ts) is a correct state machine: blank line, another
comment, an env directive, or end-of-script while an annotation is pending all reject withINVALID_ARGS;
no path lets an annotation silently drift to a non-adjacent action. Both writers
(session-script-writer.ts's live recorder andscript.ts'swriteReplayScriptheal/rewrite path) push
the annotation and its action line as consecutive array entries — the binding is structurally guaranteed,
not just tested incidentally. - Ancestry leaf-anchored prefix matching (root-side-truncation tolerant, inserted-wrapper rejecting) and
local-identity matching (id-wins, absent-label-equals-absent) match decision 3's prose exactly, with tests
for both directions. - The self-caught truncation bug (identity-set scan comparing an untruncated candidate against the bounded
recorded identity) is genuinely fixed in3f8097d9b. I grepped every identity-comparison site in
session-target-evidence.ts:buildAncestryChain,computeScrollRegionKey, and the identitySet filter
all now go throughboundedLocalIdentityconsistently — the first two already did before the fix, so the
fix closed the one remaining gap. Has its own correctly-targeted regression test (500-byte id). - Privacy/wire-shape (full trace, see finding #1 above): no public CLI/MCP response shape gains
node/preActionNodes, and the on-disk.ad/session-history artifacts only ever contain the compact
target-v1JSON, never a raw tree. - Scope confinement to click/press/longpress/fill is structural, not incidental:
buildInteractionResponseData's
kind:'runtime'branch only acceptsPressCommandResult | FillCommandResult | LongPressCommandResult.
scroll/swipe/pinch/type/get/isnever populateresult.node, and
extractTargetEvidenceForRecording's early return degrades silently for all of them, including
point-target fill (fill x y "text") andtype's raw keyboard entry (both confirmed to route through
finalizeTouchInteractionbut never carrynode). - True inertness confirmed: grepped the whole tree for
.targetEvidencereads outside the
parser/writer/session-recording files touched by this PR — nothing in replay execution consumes it yet. - Fixture realism:
bottomTabsRealCaptureFixture(session-target-evidence.test.ts) is a genuine
real-capture shape (undefinedhittableon plain text, an anonymous role-only wrapper ancestor,
hittable:falseon real buttons) and exercises ancestry/sibling/scrollRegion plus a full round trip. It
would not by itself have caught the untruncated-identity bug (no field exceeds 256 bytes there) — that's
covered by its own separate, precisely-targeted regression test instead, which is the right way to pin a
boundary condition rather than relying on a "realistic" fixture to organically hit it.
Not completed: live device verification (axis 8)
Spawned a background agent to record a session against the iPhone 17 Pro simulator / RN playground
(open --save-script ... --relaunch + rne://stack-prevent-remove deep link + press "Push Article" + a
fill), cat the resulting .ad, and replay it to confirm inertness. The agent's tool calls were interrupted
by the user mid-run (simulator/build tool permissions denied) and it did not produce a final report before
stopping. I did not force a retry. Given the static trace above is unusually thorough (I read every
comparison/serialization site directly, not just the tests), I'm comfortable shipping this review without
that piece, but it remains a real gap — recommend a manual spot-check of the .ad output (annotation lines
present immediately before press/fill, plausible sibling/viewportOrder, verification:"verified") before
merge, since it's cheap and this is exactly the kind of thing static review can miss.
Gates
gh pr checks 1196: 21 checks — 19 pass, Coverage fail, Lint & Format fail (both detailed above).
Everything else (Typecheck, Build, Bundle Size, CodeQL, Fallow Code Quality, Integration Tests, Layering
Guard, Smoke Tests ×4, Swift Runner Unit Compile, Packaged CLI, Web Platform Smoke, iOS Runner Swift
Compatibility) passes.- Local
pnpm typecheck/pnpm lint/pnpm build: pass. - Local
pnpm test(unit-core + android-adb, 386 files/3566 tests): green; the 17 failures seen on one full
run were isolated toplatforms/apple/core/__tests__/index.test.tsandrunner-client.test.ts
(timeout-pattern under contention) and passed cleanly when rerun alone — consistent with this repo's known
flaky-under-contention iOS process-spawn tests, not a regression. pnpm fallow/ Fallow Code Quality CI check: clean, no findings against origin/main.
Verdict
Needs changes before merge — two concrete, cheap-to-fix blockers (the stale native-ref contract
assertion, and formatting on the 6 touched files), plus one recommended pre-merge step (live spot-check of a
recorded .ad file, since I couldn't complete that here). The spec-fidelity work itself is strong: I did not
find any deviation from decision 3's normative text in the parser, the binding rule, or the writer's
record-time algorithm, and the one bug the author self-caught (untruncated candidate vs. bounded recorded
identity) is genuinely fixed everywhere it needed to be. The two design notes (#3, #4) are worth a comment
but not a blocker for an explicitly inert PR.
…reasons, worst-case sizing
- native-ref contract test: deliberately updated to assert the new ADR 0012
boundary — the preflight's node/preActionNodes ride the INTERNAL runtime
result and visualization payload only; the public responseData never
carries node/preActionNodes/targetEvidence (asserted directly against
buildInteractionResponseData).
- oxfmt formatting on all touched files.
- classifyTargetBindingMatch path 6 now distinguishes decision 3's two
spec-distinct outcomes: a signal isolating a member that differs from the
winner ('signal-isolated-wrong', the paths-4/5 comparison class, future
identity-mismatch) vs true fall-through ('no-signal-isolation', future
identity-unverifiable), so step 4 can consume the reasons directly.
- writer reduction loop sizes every candidate against the worst-case
verification value ("unverifiable" is 4 bytes longer than "verified"), so
a fail-closed self-check downgrade can never push an accepted payload over
the 4 KiB cap; pinned by a 1-byte-granularity sweep across the boundary
that fails against the old placeholder-sized check.
|
Thanks for the thorough review — all four findings addressed in 7951072. Blocker 1 (Coverage / native-ref contract test): Updated Blocker 2 (oxfmt): Design note 3 (path-6 collapse): Took this now rather than deferring — Design note 4 (placeholder sizing): Also taken now, via your second suggested option: the reduction loop sizes every candidate against the worst-case serialization ( Live verification (axis 8): Deferred — the simulator is currently in active use by the maintainer, so I'm not opening sessions or touching Metro ports right now. Will run the recorded- Local gates after the changes: typecheck, oxlint, format:check, layering, build, fallow audit vs origin/main all clean; full |
Review — 🟢 SHIP-WITH-NITSA large but well-scoped and exceptionally well-tested change: an inert Findings:
Cleared: 🤖 Generated with Claude Code |
|
Second-review nits addressed in 06eea52: 1 (missing top-level role): Taken as real hardening, not just a note — the parser now rejects a MISSING 2 (comment overstating data flow): Reworded — the comment now says 3 (trailing newline): Checked all touched files byte-level — every one already ends with Gates rerun after the changes: typecheck, oxlint, format:check clean; the target-identity/script/session-target-evidence suites green (77 tests). |
|
Two blocking ADR 0012 findings on current head
|
|
Self-audit per the maintainer directive ("if a workaround needs a paragraph to justify it, fix the code") — landed in d181c1a (+ c617506 formatting the two test files added in 852c680, which failed Redesigned so the explanation became unnecessary:
Trimmed to one-line constraint statements (arguments now live in the PR body/history, not the code): the module docs in Also rebased onto 852c680 (thanks for the recording-time direct-iOS gating + scrollRegion id-precedence) — the structural-channel refactor merged cleanly with it, net −75 lines. Gates after the rebase: typecheck / oxlint / format:check / layering / build / fallow clean; unit + interaction-contract + provider-integration projects green (three provider timeouts under parallel-project contention, each passing in isolation). |
|
Current head
Also still required before readiness: live simulator/device evidence that records click/fill/get annotations to a real |
…tep 3)
Recording now emits a `# agent-device:target-v1 {...}` comment immediately
before every click/press/longpress/fill action that resolves through the
tree (ref or selector), carrying the identity/ancestry/sibling/scrollRegion/
viewportOrder tuple from decision 3's record-time write algorithm plus a
record-time self-check (verified/unverifiable). The parser accepts known
fields in any order, NFC-normalizes, rejects malformed/oversized annotations
with INVALID_ARGS, binds an annotation only to the immediately-next action
line, and treats unknown future target-vN comments as ordinary comments.
writeReplayScript's read-then-rewrite (heal) path preserves v1 annotations
in canonical form. Nothing consumes the parsed evidence yet beyond
preservation — replay-time enforcement is a later migration step.
The identity tuple's own node/tree only ever lived on the internal
visualization/session-history payload (never the public response); it is
converted to the compact target-v1 evidence and stripped before anything is
persisted or returned.
The identity-set scan in computeTargetEvidence compared an untruncated candidate identity against the 256-byte-bounded recorded identity, so a node whose own id/label exceeds the field cap could fail to match itself, corrupting the record-time self-check. Compare through the same bounding on both sides instead.
…reasons, worst-case sizing
- native-ref contract test: deliberately updated to assert the new ADR 0012
boundary — the preflight's node/preActionNodes ride the INTERNAL runtime
result and visualization payload only; the public responseData never
carries node/preActionNodes/targetEvidence (asserted directly against
buildInteractionResponseData).
- oxfmt formatting on all touched files.
- classifyTargetBindingMatch path 6 now distinguishes decision 3's two
spec-distinct outcomes: a signal isolating a member that differs from the
winner ('signal-isolated-wrong', the paths-4/5 comparison class, future
identity-mismatch) vs true fall-through ('no-signal-isolation', future
identity-unverifiable), so step 4 can consume the reasons directly.
- writer reduction loop sizes every candidate against the worst-case
verification value ("unverifiable" is 4 bytes longer than "verified"), so
a fail-closed self-check downgrade can never push an accepted payload over
the 4 KiB cap; pinned by a 1-byte-granularity sweep across the boundary
that fails against the old placeholder-sized check.
…flow comment Second-review nits: the writer emits `role` unconditionally (top level, ancestry entries, scrollRegion) — possibly as the empty string for typeless nodes, which stays accepted — so a MISSING role key can only come from a hand-edited/adversarial annotation and is now rejected with INVALID_ARGS instead of silently parsing as an implicit empty role, which step-4 enforcement could otherwise match against anonymous wrapper nodes. Also corrects the interaction-touch-response comment that overstated where the raw node/tree flows (finalizeTouchInteraction strips it before both session history and touch overlay telemetry).
…ment-comments Maintainer directive: comments that argue a workaround is acceptable mark code to redesign. Applied to the whole PR diff: - The record-time node/tree now travel on a typed `recordedTarget` side channel of InteractionResponsePayloads instead of being smuggled through the visualization Record and stripped later. The construction site routes them there exclusively, finalizeTouchInteraction consumes the channel directly, and extractTargetEvidenceForRecording (the strip helper and its justifying doc) is deleted — the public/internal split is now enforced by the type shape, so the contract test asserts it in two lines instead of a paragraph. - findNearestScrollableContainer/findNearestAncestor made generic over the node type, removing a cast plus its safety-argument comment. - computeLocalIdentity/boundedLocalIdentity collapsed into one always-capped identity reader — the raw/bounded split was the root of the earlier self-match bug and existed only to be explained. - parseReplayScriptDetailed's rejectUnbound takes the pending annotation as a parameter, removing a non-null assertion and its comment. - Remaining paragraph-length argument-comments trimmed to one-line constraint statements (module docs, sizing/floor comments, regex/role rationales, test comments); spec-mapping docs stay.
Maintainer blockers on ADR 0012 step 3: - get text/attrs now records target-v1 evidence: GetCommandResult carries the resolution tree (preActionNodes, internal — neither the recorded result nor the public payload copies it), recordIfSession gains the typed recordedTarget channel, and dispatchGetViaRuntime threads the capture through. The direct-iOS get query is gated during recording so the snapshot path supplies the evidence tree, mirroring the tap/fill gating. find/is stay uncovered: their results are match-set shaped (found flags, predicate booleans over possibly-many matches), not a single resolved winner — noted in the PR. - buildAncestryChain now reports a broken parent walk (dangling parentIndex or cycle) instead of silently producing a root-like chain; the writer fails the annotation closed to 'unverifiable' per decision 3's capture-anomaly rule, and broken candidates cannot prove an ancestry prefix. Regression tests for both anomaly shapes. - New ADR 0012 recording tests live in focused files (interaction-target-evidence.test.ts) instead of growing the interaction.test.ts aggregation; the earlier additions moved out.
c617506 to
5b6e932
Compare
|
All three blockers from the readiness review addressed in 68b5698 + 5b6e932 (branch rebased onto current main, so the new production-unused-exports ratchet and the layering cycle guard both ran against this work): 1. Recorded 2. Missing parent linkage now fails closed. 3. Test split. The earlier evidence/privacy cases are moved out of Also on this push: rebase onto main is clean; the new Live simulator evidence (record click/fill/get to a real Gates: typecheck / oxlint / format:check / layering (incl. the new cycle guard) / build / |
Summary
Implements ADR 0012 migration step 3 — decision 3's INERT
target-v1.adparser/writer (docs/adr/0012-interactive-replay.md). Recordings gain per-action identity annotations; replay parses and preserves them but does not enforce anything yet (that is a later migration step, gated on step 2's divergence transport).src/daemon/session-target-evidence.ts): for everyclick/press/longpress/fillaction that resolves an element target (ref or selector, through the full runtime resolution path or the native-ref fast path), emits# agent-device:target-v1 {...}immediately before the action line. Computes decision 3's record-time write algorithm steps 1-5: identity tuple (idorrole+normalizedlabel, whererole = normalizeType(node.type ?? "")exactly asbuildSelectorChainForNodeuses), leaf-anchoredancestry(K=8, nearest-first, root-side truncated),sibling(genuine same-parent child ordinal viaparentIndex),scrollRegion/viewportOrder(nearest-scrollable-ancestor partition, ordered by rect center then document order), diagnosticrect, and a record-time self-check that runs the shared replay-time classification algorithm against the record-time tree to decideverified/unverifiable.ancestry[0], with the self-check re-run against the actually-reduced tuple at each step; the (spec-asserted-unreachable) terminal overflow falls back toverification: "unverifiable"rather than ever emitting something the parser would reject.src/replay/target-identity.ts, wired intosrc/replay/script.ts): accepts known fields in any order, ignores unknown fields, NFC-normalizes (label additionally trims/collapses whitespace), rejects malformed/oversized annotations withINVALID_ARGS(parser rejects, never truncates), binds an annotation only to the immediately-next physical action line (any intervening/blank line is rejected), and treats an unknown futuretarget-vNas an ordinary comment.writeReplayScript's read-then-rewrite (heal/--update) path re-emits parsedtarget-v1annotations in canonical form rather than silently dropping them.Contradictions / findings surfaced along the way
None that required stopping — the recording pipeline does supply everything decision 3 needs, but two plumbing gaps had to be closed (documented in code comments) since the current pipeline didn't carry them yet, not because the data was unavailable:
preActionNodes/the resolved node never reached session history.interactionResultExtra(src/daemon/handlers/interaction-touch-targets.ts) deliberately allowlists onlyref/selector/selectorChain/refLabel/targetHittable/hint/evidence/settleinto the sharedcommonExtrathat feeds both the internal session-history payload and the public response. I did not widen that allowlist (that would leak the raw tree into the public response). Instead,buildInteractionResponseData(interaction-touch-response.ts) now attachesnode/preActionNodesonly onto the internalresult/visualization payload it returns, andfinalizeTouchInteraction(interaction-common.ts) computes the compacttarget-v1evidence from them — gated onsession.recordSessionso the O(nodes) ancestry/identity-set scan never runs outside an active--save-scriptrecording — then strips the raw node/tree before anything is persisted or logged. The public response never seesnode/preActionNodes/targetEvidence(covered by a new test ininteraction.test.ts).click @ref/fill @refwith default options) looked up the node for its offscreen/occlusion preflight but discarded it.preflightNativeRefInteraction(resolution.ts) now also returns that already-fetchednode/preActionNodesat zero extra capture cost, so recording gets evidence on that path too, not only the full-resolution path.boundedLocalIdentity) — covered by a regression test.get/find/waitselector reads don't currently carrynode/preActionNodesinto their recorded action result (selector-recording.ts'sbuildGetRecordResult/buildFindRecordResultonly keepref/selector/selectorChain/text), so no annotation is emitted for them. This is a legitimate scope decision, not a silent gap: decision 3's own worked example is aclick, and extending to selector reads is separable follow-up work.Test plan
pnpm typecheck,pnpm lint(oxlint --deny-warnings),pnpm check:layering,pnpm build,pnpm check:mcp-metadata,fallow audit --base origin/main— all clean.pnpm test— full suite: 386 files / 3566 tests, all green (two re-runs; one earlier run tripped the slow-test-gate on unrelated iOS runner-client tests under load, reproduced as pre-existing contention flakiness againstorigin/mainwith my changes stashed).src/replay/__tests__/target-identity.test.ts(v1 round trips, old/new reader compat, escaping/Unicode, normalized-role source, leaf-anchored ancestry prefix incl. root-side truncation + inserted-wrapper mismatch, malformed/unbound annotations, rect-diagnostic-only, and the full classification core incl. duplicate/unverifiable paths),src/daemon/__tests__/session-target-evidence.test.ts(record-time write algorithm, writer overflow reduction with max-size K=8 fields, fail-closed self-consistency, and a real-capture-shaped fixture with undefinedhittable/anonymous wrapper nodes per the PR fix: make settle tail list real actionable targets #1172 fixture-realism pattern),src/replay/__tests__/script.test.tsandsrc/daemon/__tests__/session-store.test.ts(binding rule,writeReplayScriptpreservation end to end throughSessionScriptWriter), andsrc/daemon/handlers/__tests__/interaction.test.ts(evidence computed only while recording, never leaked to the public response).