Skip to content

refactor: extract the .ad script codec into packages/ad-script (#1478) - #1536

Merged
thymikee merged 4 commits into
mainfrom
refactor/extract-ad-script-codec
Aug 1, 2026
Merged

refactor: extract the .ad script codec into packages/ad-script (#1478)#1536
thymikee merged 4 commits into
mainfrom
refactor/extract-ad-script-codec

Conversation

@thymikee

@thymikee thymikee commented Jul 31, 2026

Copy link
Copy Markdown
Member

Summary

Implements option 1 from the P5 scoping dossier on #1478 ("the codec seam"): extracts the mutually-coupled .ad script codec into a new private leaf workspace package, @agent-device/ad-script, exporting only ..

Based directly on main — the rest of the stack (#1532 P4a, #1535 P4b) has merged, and this is the last layer.

What moved vs. stayed

Moved into packages/ad-script/src/internal/ (unchanged logic, only import paths updated):

  • script.ts (parser: parseReplayScriptDetailed, readReplayScriptMetadata)
  • script-utils.ts (arg formatting/flag parsing, shared read+write)
  • script-formatting.ts (writer: formatPortableActionLine, formatTargetAnnotationLines)
  • open-script.ts (open action's read+write half)
  • New target-annotation-serde.ts: the target-v1 annotation SERDE slice split out of src/replay/target-identity.ts — the wire type re-export, canonical field order, normalization (normalizeIdentifierField/normalizeRoleField/normalizeLabelField/truncateToUtf8Bytes/utf8ByteLength), size-cap constants, and serializeTargetAnnotationV1/formatTargetAnnotationCommentLine/parseTargetAnnotationCommentLine/parseTargetAnnotationV1Payload. None of this is used by classification — verified by checking every call site of each moved symbol before splitting.
  • REPLAY_VAR_KEY_RE (the .ad env/var key-shape regex) moved from src/replay/vars.ts into script.ts and is exported from the façade — vars.ts's env-directive parsing already lived in the codec, and vars.ts stays outside the package (it's engine, not codec) — so it now re-exports the constant from @agent-device/ad-script instead of duplicating the regex literal.

Split src/replay/target-identity.ts — stays, classification-only now:

  • classifyTargetBindingMatch and its input/output types
  • annotationLocalIdentity, matchesLocalIdentity, matchesAncestryPrefix, LocalIdentity
  • Imports the shared annotation types (TargetAnnotationV1, TargetAncestryEntry) from @agent-device/ad-script instead of declaring them.

Does not move (per the dossier, unchanged): target-identity-node.ts (record-time local-identity reader over RawSnapshotNode) and target-evidence-tree.ts (tree-structural evidence primitives) stay in src/replay/. Both needed a small import fix to source normalization helpers/types from @agent-device/ad-script.

Test relocation

  • src/replay/__tests__/script.test.tspackages/ad-script/src/internal/__tests__/script.test.ts, minus its one property-based round-trip test, which stays at root as new src/replay/__tests__/ad-script-round-trip.test.ts (its generator is derived from the root command catalog/selector grammar — a package test importing it would be an R11 package→root-src escape).
  • src/replay/__tests__/target-identity.test.ts split along the serde/classification line: serde tests moved to new packages/ad-script/src/internal/__tests__/target-annotation-serde.test.ts; matchesAncestryPrefix/matchesLocalIdentity tests stayed (classification side).

Façade export list (consumer-derived, re-swept and trimmed per review)

Current packages/ad-script/src/index.ts:

  • script.ts: parseReplayScriptDetailed, readReplayScriptMetadata, REPLAY_VAR_KEY_RE, types ParsedReplayScript/ReplayScriptMetadata
  • script-utils.ts: appendScriptSeriesFlags, formatDivergenceActionLabel, formatScriptArg, formatScriptStringLiteral, isClickLikeCommand, isTouchTargetCommand, stripRecordedRefGeneration
  • script-formatting.ts: formatPortableActionLine, formatTargetAnnotationLines
  • target-annotation-serde.ts: normalizeIdentifierField, normalizeLabelField, normalizeRoleField, parseTargetAnnotationV1Payload, serializeTargetAnnotationV1, truncateToUtf8Bytes, utf8ByteLength, TARGET_ANNOTATION_MAX_ANCESTRY, TARGET_ANNOTATION_MAX_FIELD_BYTES, TARGET_ANNOTATION_MAX_PAYLOAD_BYTES, and types TargetAncestryEntry/TargetAnnotationV1/TargetScrollRegion/TargetVerification

A first review round removed 5 exports with zero external consumers (verified by grepping the whole repo outside packages/ad-script): parseReplaySeriesFlags, formatTargetAnnotationCommentLine, parseTargetAnnotationCommentLine, TargetAnnotationLineParseResult, TargetRect. All five stay exported from their declaring internal module for the package's own internal use; only the façade re-export was dropped.

The one-export boundary is locked

scripts/layering/package-boundaries.test.ts now carries the same exact-assertion shape it already has for maestro/provider-webdriver/provider-limrun/xml for @agent-device/ad-script: exports exactly ., depends on exactly contracts+kernel, is declared in root package.json, and is included in the deep-resolution-rejection coverage (Node resolution enforces the exports map at runtime). Verified the lock actually catches a regression: temporarily added a fake ./codec subpath export to packages/ad-script/package.json and confirmed both the export-key-list assertion and the deep-resolution-rejection assertion failed; removed the plant and reconfirmed green.

The contracts decision

packages/contracts/src/target-annotation.ts does not import src/replay/target-identity.ts — verified directly (it has zero imports). It's the reverse: target-identity.ts imports the annotation shape types from @agent-device/contracts/replay. No change to packages/contracts was needed or made.

Registry updates

  • Root package.json: "@agent-device/ad-script": "workspace:*" in devDependencies, and packages/ad-script in the typecheck script's tsc -b list.
  • scripts/layering/model.ts: ['ad-script', 1] added to TARGET_DAG_RANK, alongside maestro/replay-test/xml at leaf rank 1.
  • scripts/layering/daemon-modularity.ts intentionally not touched — the src/ad-replay/ entries there are P5's, and ad-script (a plain leaf codec) needs no LOGICAL_MODULE_POLICIES entry, matching contracts/kernel/xml's precedent.

CodeQL: polynomial-redos in the annotation line regex (fixed)

TARGET_ANNOTATION_LINE_RE in target-annotation-serde.ts let \s+ and a bare .* both match whitespace in the payload group, so a run of separator whitespace that ultimately fails to complete the match had many splits to backtrack through. Anchored the payload group on \S (the exact complement of \s) so the split point is unique — no backtracking possible. Behavior-preserving (the only caller always matches an already-trimmed line) and verified against the frozen replay-compat corpus + full serde/parser suite; a direct test on the exported pattern pins the linear rejection (the retired ambiguous form takes 3,751 ms on the same pinned non-match). The same ReDoS class was cleared across main by #1546; this branch carries the fix in the moved file, so after rebase the fixed regex exists in exactly one place (packages/ad-script).

A regression this PR's investigation found (fix lives elsewhere)

Live-device validation of this PR surfaced a pre-existing, unrelated daemon hang: annotateCoveredSnapshotNodes (src/snapshot/snapshot-occlusion.ts) had an unmemoized double recursion that goes exponential once a snapshot carries many mutually-overlapping "overlay-like" nodes — concretely, an open Android IME keyboard's ~40 keys — wedging the daemon at ~99% CPU and blocking this PR's own test-app:replay live evidence. The corrected (immutable-input) fix merged to main as #1541; on rebase this branch's interim copy deduped away automatically, so the fix now lives only on main and this PR carries zero occlusion changes (git diff main -- src/snapshot/ is empty). Root-cause writeup: issuecomment-5150828335.

Live evidence

Device validation (m6.ad minimal repro + the full checkout-form-android.ad + gesture-lab-android.ad two-script run on a real Android emulator) is recorded in issuecomment-5150851825, including before/after timings for the occlusion wedge above; the exact-head follow-up run (two-fill live repro green) is in the later thread comments. The post-rebase head is tree-identical to that evaluated head for everything this PR touches — the only file deltas vs the reviewed 5df4b05f9 tree are main's own merged #1546/#1547 hunks (replay-test slug helpers + docs), which this PR does not modify.

Gate evidence (at head 754274c9e, rebased onto main past #1541/#1546/#1547)

  • pnpm typecheck, pnpm lint, pnpm format:check — clean.
  • pnpm check:layering — OK; R11 exact-export lock for @agent-device/ad-script holds (exports exactly ., deps exactly contracts+kernel).
  • pnpm check:replay-compat — verified against all released tags.
  • npx fallow audit --base origin/main — 0 issues in 63 changed files.
  • Full npx vitest run — 5252 passed; the only failures across runs were the documented unit-suite contention-timeout class (changing set between runs, each file green in isolation, none touch this PR's files).

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.92 MB 1.92 MB -195 B
JS gzip 615.8 kB 615.5 kB -305 B
npm tarball 733.9 kB 733.6 kB -258 B
npm unpacked 2.57 MB 2.57 MB -130 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.0 ms 28.6 ms +0.6 ms
CLI --help 60.5 ms 58.9 ms -1.7 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/selector-vocabulary.js +36.3 kB +11.1 kB
dist/src/physical-device-control.js +23.0 kB +7.5 kB
dist/src/screenshot-result.js +6.7 kB +1.9 kB
dist/src/internal/daemon.js +13 B +15 B
dist/src/cli.js +15 B +15 B

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-01 18:21 UTC

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed head 02a0ac5 against #1478's P5 scoping dossier. The package boundary and production rewiring are otherwise sound, and all CI is green, but I found two readiness items:

  • P2 — keep the new root façade consumer-derived. packages/ad-script/src/index.ts exports parseReplaySeriesFlags, but no caller outside the package uses it; only internal/script.ts does. Remove it from the package façade while keeping the internal export.
  • P2 — lock the promised one-export package boundary. scripts/layering/package-boundaries.test.ts has exact root-export/dependency assertions for extracted packages such as Maestro, but none for @agent-device/ad-script. The generic R11 scan trusts whatever appears in exports, so a future /codec subpath would pass. Add the parallel assertion that this package exports only @agent-device/ad-script, depends only on contracts/kernel, is declared by root, and include it in the runtime deep-resolution rejection coverage.

Before readiness, also add the required live evidence for the moved replay/session-publication route: rebuilt clean-daemon iOS and Android test-app:replay runs with artifacts and session cleanup, or state the exact blocker/commands as residual risk. The PR is also intentionally stacked on open #1532, so merge sequencing remains.

No ready-for-human label yet.

@thymikee
thymikee force-pushed the refactor/extract-ad-script-codec branch from 02a0ac5 to b6761a8 Compare August 1, 2026 06:00
@thymikee
thymikee changed the base branch from p4a/session-script-publication to p4b/replay-transaction-coordinator August 1, 2026 06:02
Comment thread packages/ad-script/src/internal/target-annotation-serde.ts Fixed
@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Addressed both P2 items from review (commit e5c1e5a, on top of the rebase to b6761a8d7).

1. Façade trimmed to real consumers

Re-swept every export in packages/ad-script/src/index.ts by grepping the whole repo (outside packages/ad-script) for each symbol name. Removed the five with zero external consumers:

  • parseReplaySeriesFlags (script-utils.ts) — only internal/script.ts used it, as flagged.
  • formatTargetAnnotationCommentLine (target-annotation-serde.ts) — only mentioned in a comment in session-script-writer.ts, never actually imported; real consumer is internal/script-formatting.ts.
  • parseTargetAnnotationCommentLine (target-annotation-serde.ts) — same pattern, only a comment mention in session-script-writer.ts; real consumer is internal/script.ts.
  • TargetAnnotationLineParseResult (target-annotation-serde.ts) — the return type of the now-unexported parseTargetAnnotationCommentLine; used only within its own declaring file.
  • TargetRect (target-annotation-serde.ts, re-exported from contracts) — zero consumers anywhere; nothing in the repo names it directly (structural typing means TargetAnnotationV1.rect consumers don't need it imported by name).

All five stay exported from their declaring internal module (unchanged internal usage), per the "keep the internal export" pattern from the review — only the façade re-export was removed.

Everything else in the façade was confirmed to have at least one real external consumer: parseReplayScriptDetailed, readReplayScriptMetadata, REPLAY_VAR_KEY_RE, ParsedReplayScript/ReplayScriptMetadata types, appendScriptSeriesFlags, formatDivergenceActionLabel, formatScriptArg, formatScriptStringLiteral, isClickLikeCommand, isTouchTargetCommand, stripRecordedRefGeneration, formatPortableActionLine, formatTargetAnnotationLines, normalizeIdentifierField/normalizeLabelField/normalizeRoleField, parseTargetAnnotationV1Payload, serializeTargetAnnotationV1, truncateToUtf8Bytes, utf8ByteLength, the three TARGET_ANNOTATION_MAX_* constants, and the TargetAncestryEntry/TargetAnnotationV1/TargetScrollRegion/TargetVerification types.

2. One-export boundary locked

Added to scripts/layering/package-boundaries.test.ts, mirroring Maestro's assertion shape in the real tree parses, declares, and passes R11:

  • adScriptPackage.exportTargets.keys() deep-equals exactly ['@agent-device/ad-script']
  • adScriptPackage.workspaceDependencies deep-equals exactly ['@agent-device/contracts', '@agent-device/kernel']
  • root package.json declares the @agent-device/ad-script workspace dependency

Also added ad-script entries to the deep-resolution rejection list in Node resolution enforces the exports map at runtime (@agent-device/ad-script/codec, .../internal/script.ts, .../src/index.ts must all fail to resolve), plus a positive-resolution assertion that the bare specifier resolves to packages/ad-script/src/index.ts.

Plant-proof: temporarily added a fake "./codec" subpath to packages/ad-script/package.json#exports. Both new assertions failed as expected:

  • the real tree parses, declares, and passes R11: AssertionError [ERR_ASSERTION] — actual ['@agent-device/ad-script', '@agent-device/ad-script/codec'] vs expected ['@agent-device/ad-script'].
  • Node resolution enforces the exports map at runtime: Missing expected exception: @agent-device/ad-script/codec must not resolve (the planted subpath now legitimately resolves).

Removed the plant; both assertions pass again (11/11 tests in that file).

Gate evidence

  • pnpm typecheck — exit 0.
  • pnpm lint — exit 0 (oxlint . --deny-warnings, 0 warnings).
  • node ./node_modules/oxfmt/bin/oxfmt + pnpm format:check — exit 0, "All matched files use the correct format."
  • pnpm check:layering — exit 0. R11 holds 8 workspace package(s) behind 28 exported subpath(s) with zero root back-imports (unchanged from before this follow-up — the façade trim removed re-exports, not the package/subpath count).
  • npx fallow audit --base origin/p4a/session-script-publicationNo issues in 63 changed files. Baselines untouched.
  • npx vitest run packages/ad-script scripts/layering test/replay-compatpackages/ad-script (2 files, 66 tests) and test/replay-compat (1 file, 51 tests) all pass. Note: scripts/layering/*.test.ts isn't part of any vitest project glob (those 51 assertions, including the new ones, run via Node's own test runner under pnpm check:layering, confirmed green above) — npx vitest run scripts/layering correctly discovers 0 files there by design.
  • Full pnpm vitest run: ran three times back-to-back due to heavy contention on the runner. Failure counts varied each time (0, 2, then 2 again) across a shifting set of files — help-conformance-bench.test.ts, device-claims.test.ts, runner-session.test.ts, android-recording.test.ts, doctor.test.ts, daemon-client.test.ts — all unrelated process-spawn/timing-sensitive tests, none touching this PR's changed files. Every single failure passed cleanly when re-run in isolation. This matches the repo's documented unit-suite-flaky-under-contention pattern (timeouts and one Missing expected rejection race, not assertion regressions). A clean full run: 627 test files, 5233 tests, 0 failed.

Live-device evidence

Live iOS/Android test-app:replay evidence is being produced separately on a maintainer machine and will be posted on this PR. The Android leg additionally depends on the fixture repairs in #1538.


Generated by Claude Code

@thymikee
thymikee force-pushed the refactor/extract-ad-script-codec branch from e5c1e5a to 5866a42 Compare August 1, 2026 06:24
@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed latest head e5c1e5a. Both prior P2 code findings are fixed: the package façade is trimmed to real external consumers, and the R11 tests now lock the sole root export, allowed dependencies, root declaration, legal root resolution, and deep-import rejection. The delta is code-review clean.

Not ready to label yet:

No ready-for-human label yet.

@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Exact-head re-review at 5866a420: clean. The rebase now includes current P4b head cb35d978; range-diff shows no semantic conflict-resolution drift, both prior façade/R11 fixes remain intact, and every exact-head check is green, including CodeQL, Replay-Compat, integration, coverage, and all platform smoke.

Remaining readiness blockers:

Code and CI are clean; no ready-for-human label until the live-evidence blocker is resolved.

@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

The reviewed head 5866a42 is unchanged and its prior code verdict remains clean, but the advanced #1535 base now makes this PR CONFLICTING / DIRTY. Rebase onto 9a381ba and resolve the five import conflicts while preserving the @agent-device/ad-script codec boundary (not the old src/replay/* imports), then rerun exact-head checks and request review of the resolution delta. The existing green CI predates the new base; clean-daemon iOS/Android replay evidence and the body refresh are also still outstanding.

@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

STOP-priority regression: root-caused and fixed (commit 71994d7)

Repro confirmed

node bin/agent-device.mjs test /tmp/m6.ad --platform android reliably hung (exit 124 at the 180s cap, ~99% CPU, orphaned daemon) at head 5866a4204. Reproduced deterministically 3/3 times from a clean device state (am force-stop + dismiss keyboard between runs, ruling out cross-run device-state contamination).

Mechanism

annotateCoveredSnapshotNodes (src/snapshot/snapshot-occlusion.ts) asks, for every overlay-classified candidate cover of a touch target, whether that candidate is itself covered by something later in the snapshot — via a recursive call from visibleCoverRect back into findCoveringNode. That recursive question was never memoized: resolving position P's answer required resolving every later position Q > P from scratch, and resolving Q required resolving every position after it from scratch again — O(2^overlayPositions.length) work with no upper bound.

A live CDP pause on the wedged daemon (kill -USR1 <pid>, Debugger.pause over ws://127.0.0.1:9229) landed repeatedly inside exactly this recursive triad (findCoveringNodecanCoverPointvisibleCoverRectfindCoveringNode, ~20 levels deep, over and over), matching the sample profile from the original report (normalizeType/role-normalization hot — it's called from inside this loop via isOverlayLikeNode).

The pathological input is real, not synthetic: the second fill in the two-field Android form runs while the on-screen IME keyboard is open, and each individual key gets classified isAdditionalOverlayNode — instrumenting the function directly against the live repro showed nodes=62 overlayPositions=39 at exactly the point the daemon stops responding.

Why this branch and not p4a/p4b/main

snapshot-occlusion.ts is untouched by the codec extraction — this is a pre-existing latent algorithmic defect, not new logic. What differs is the input it's fed. A/B testing with matching instrumentation copied onto the p4a head shows the equivalent snapshot there carries zero overlay-classified nodes at the same point in the script and completes in ~7s, reproduced 3/3 clean runs. I also ruled out a pure timing race: extending the real (non-instant) gap between fills on p4a with a genuine multi-second wait still does not reproduce the 39-overlay state.

I was not able to pin the exact line connecting this branch's consumer-rewiring/import-path changes to that shift in the fill-resolution snapshot's timing/content — target-identity-node.ts, target-evidence-tree.ts, and session-target-evidence.ts sit in the same import graph as the fill dispatch/post-resolution-guard path, but nothing in that diff is runtime-observable (the touched imports are type-only or point-for-point equivalent value re-exports). I'm flagging this as a residual open question rather than claiming full closure on it — the mechanism that hangs is conclusively identified and fixed; the precise trigger for why this branch reaches the pathological input reliably is not.

Fix

Cache findCoveringNode's answer per position on the scan object, scoped to one annotateCoveredSnapshotNodes call. The scan's node list is immutable input for the duration of one pass (a position's byIndex entry is only ever mutated after that position has been fully resolved, and recursion only ever looks at strictly-later positions), so a given position's covered-by-something-later answer is provably stable across every path that asks it. This turns the unbounded double recursion into O(K) resolutions of O(K) work each — O(K^2) instead of O(2^K).

Regression test

src/snapshot/__tests__/snapshot-occlusion.test.ts — builds a synthetic 40-node "keyboard" (mutually non-overlapping, same-kind overlay-classified nodes, matching the live scale) and asserts annotateCoveredSnapshotNodes returns in well under a second, plus two existing-behavior sanity cases (a covered touch target, an uncovered one) to guard against the memoization silently changing output.

Verified the test catches the regression: with the memoization reverted, the exact same test never returns within the suite's timeout (timeout 20 npx vitest run ... → exit 124) instead of failing an assertion — it hangs exactly like the daemon did, which is the strongest possible proof for a synchronous-recursion bug like this one.

Live verification (this repo's own emulator, emulator-5554)

  • node bin/agent-device.mjs test /tmp/m6.ad --platform android7.51s, passed, no orphaned daemon (ps -axo pid,command | grep dist/src/internal/daemon shows none from this worktree after the run).
  • Full two-script run: node bin/agent-device.mjs test examples/test-app/replays/checkout-form-android.ad examples/test-app/replays/gesture-lab-android.ad --platform androidboth passed, 75.0s total (58.7s + 16.4s), no orphan. Ran with the PR fix(test): repair Android replay fixtures against live device reality #1538 Android fixture fixes in place (uncommitted, per instruction — examples/test-app/replays/{checkout-form,gesture-lab}-android.ad and test/integration/replays/android/fixture/01-navigation-scroll.ad are still modified-but-uncommitted on the branch).

Gates

  • pnpm typecheck — exit 0.
  • pnpm lint — exit 0 (oxlint . --deny-warnings).
  • node ./node_modules/oxfmt/bin/oxfmt + pnpm format:check — exit 0, no reformatting needed.
  • pnpm check:layering — exit 0, unchanged ratchets (R11 still 8 packages / 28 subpaths).
  • npx fallow audit --base origin/p4a/session-script-publicationNo issues in 76 changed files.
  • npx vitest run packages/ad-script scripts/layering test/replay-compat src/snapshot/__tests__/snapshot-occlusion.test.ts — 4 files, 120 tests, all passed.
  • Full pnpm vitest run: ran 3 times back-to-back (this machine was under heavy load from the live-device bisection work). Failure counts/sets varied each time (10 → 2 → 2) across provider-integration/unit-core tests entirely unrelated to this change (doctor, android-lifecycle, android-test-suite, android-recording, daemon-entrypoint, device-claims, help-conformance-bench) — every one of them passed cleanly when re-run in isolation. A clean baseline run: 629 test files / 5241 tests, 0 failed.

Generated by Claude Code

@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Live test-app:replay evidence at exact head cbdd1ba01 (the branch as rebased onto the merged-#1532 main via p4b 11946b1e5), per docs/agents/device-verification.md: fresh pnpm build + pnpm clean:daemon before each leg, purpose-named sessions all closed, no orphaned daemons after the runs.

Android — green

node bin/agent-device.mjs test examples/test-app/replays/checkout-form-android.ad examples/test-app/replays/gesture-lab-android.ad --platform android on emulator-5554 (Pixel_7_CI AVD: API 36 google_apis_playstore, 1080×2400@420 — CI's profile; cached-apps freezer disabled; Release fixture APK):

✓ checkout-form-android.ad 12.1s
✓ gesture-lab-android.ad 9.79s
Test summary: 2 passed (2) in 21.9s

Active-path proof: snapshot -i --json reports androidSnapshot.backend: "android-helper", helperVersion: "0.20.3" (== package.json), not a UIAutomator fallback. The run requires PR #1538's fixture repairs (kept uncommitted in the evidence worktree; the unfixed fixtures fail on package-name and geometry defects that never ran in CI). Artifacts preserved under the evidence worktree's .tmp/evidence-android-final/.

This leg is also what caught the daemon CPU-spin fixed in cbdd1ba01 (comment above): the checkout script's two consecutive IME-open fills detonated the pre-existing O(2^K) occlusion recursion. The fix is cherry-picked to main as #1541 so main isn't exposed while this stack is in review; when #1541 merges, this branch's copy dedupes on the next sync.

Residual open question (from the fix investigation, flagged rather than hand-waved): the occlusion code is untouched by this branch, and instrumented A/B against the p4a head showed zero overlay-classified nodes at the equivalent capture point there (3/3), while this branch sees ~39. The hang mechanism is conclusively the unmemoized recursion, but why this branch's fill-resolution snapshot observes the keyboard-overlay state where p4a's didn't is not yet explained — plausibly benign scheduling drift moving the capture into the IME-open window, but unproven.

iOS — gesture-lab green; checkout-form blocked by pre-existing main defects (#1542)

node bin/agent-device.mjs test examples/test-app/replays/gesture-lab.ad examples/test-app/replays/checkout-form.ad --platform ios on iPhone 17 Pro (26.2), freshly cycled simulator:

✓ gesture-lab.ad 38.3s
⨯ checkout-form.ad 15.0s — step 11 click id="shipping-pickup": off-screen
Test summary: 1 passed (2), 1 failed in 53.3s

The checkout failure is attribution-tested byte-identical on a pre-stack main binary (same simulator, same minutes): the Form screen is currently scroll-inert to synthetic gestures (scroll/pan report success, content never moves; verified with click probes) and separately exhibits the recurring slow-AX deferral wedge after one open/close cycle. Both filed with full repro as #1542; until that lands, checkout-form.ad is residual risk on any branch, this one included.


Generated by Claude Code

@thymikee
thymikee marked this pull request as ready for review August 1, 2026 09:41
@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Exact-head re-review at cbdd1ba: the codec rebase is clean and patch-equivalent; the @agent-device/ad-script boundary, façade/R11 fixes, and exact-head checks remain intact. However this head duplicates #1541’s occlusion memoization and therefore carries its confirmed mutable-cache correctness gap: coverCache reuses per-position results while annotation mutates nodes that the caller predicate may observe. Merge a corrected #1541 and resync so the duplicate behavior commit drops. Android live replay is green but depends on #1538 fixture repairs; iOS gesture passes while checkout is a main-reproduced #1542 residual. Refresh the stale body’s base/scope/exports/evidence. Not ready yet.

@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 887302282. The target-annotation regex now removes the CodeQL-reported overlap and all exact-head checks are green, but this is not ready yet:

  1. P1 — resync after corrected fix: memoize snapshot occlusion coverage checks to stop a daemon CPU-spin #1541. This head still carries cbdd1ba's mutable coverCache implementation; fix: memoize snapshot occlusion coverage checks to stop a daemon CPU-spin #1541's immutable-input correction at 59edc2615 is not present. Merge fix: memoize snapshot occlusion coverage checks to stop a daemon CPU-spin #1541, rebase, and drop the duplicate first-pass commit.
  2. P2 — make the ReDoS regression non-vacuous. The added adversarial input successfully matches even with the old regex; reverting the fix still passes the <1s assertion. Use a failing overlap input that forces the old \s+/.* backtracking path and verify the test fails on revert.

The codec boundary, replay semantics, façade/R11 constraints, and current CI are otherwise clean. Android live evidence remains applicable; iOS checkout is the independently main-reproduced #1542 residual. No ready-for-human label yet.

Base automatically changed from p4b/replay-transaction-coordinator to main August 1, 2026 11:51
@thymikee
thymikee force-pushed the refactor/extract-ad-script-codec branch from 8873022 to 5df4b05 Compare August 1, 2026 12:01
@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Rebased and both findings addressed at head 5df4b05f9:

P1 — corrected occlusion, ahead of #1541's merge. The branch is rebased onto post-#1535/#1538 main (base auto-retargeted to main when the p4b branch was deleted; the fixture-repair dependency is now inherited rather than pending). The first-pass mutable-cache commit (cbdd1ba) is dropped, replaced by the exact corrected commits from #1541's head (6e36ee1c8 = immutable-input restructure + all invariant tests including the new mutation-sensitive predicate/ancestor regression, verbatim file parity with cae69aed3). When #1541 merges, the next rebase dedupes this to nothing — the correctness gap no longer exists on this branch in the interim.

P2 — the ReDoS regression is now non-vacuous, honestly scoped. You were right, and the vacuousness turned out to be structural: through parseTargetAnnotationCommentLine the retired regex was never slow — trim() strips edge whitespace and per-line input carries no newline, so the old form matched greedily in one attempt on every reachable shape. The regression surface is the pattern itself, so TARGET_ANNOTATION_LINE_RE is internally exported and pinned directly: an interior 100k-tab run with an x\n tail (a genuine non-match — . can't cross the newline, $ needs end-of-input) takes 3,751ms with the retired form reinstated and rejects in one attempt at head; the entry-point case remains as a behavior-contract test, re-commented to say what it does and doesn't prove.

Gates at head: typecheck, lint, format, layering, replay-compat, fallow (0 in 65 files vs main), 140 tests across the touched suites, and the live two-fill daemon-spin repro green in 7.2s with zero orphaned daemons.

Generated by Claude Code

@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed at 5df4b05f: the codec extraction, corrected immutable occlusion implementation, mutation-sensitive regression, and direct annotation-pattern ReDoS test are clean. The retired pattern demonstrably takes 3,751 ms on the pinned adversarial non-match; all exact-head checks are green, and the exact-head follow-up reports the two-fill live repro green.

Not ready solely for merge ownership: this branch still carries the same occlusion repair as open #1541 and the annotation-regex remediation covered by open #1546. Merge those focused fixes first, then rebase this PR, dedupe the inherited changes, rerun exact-head checks, and request the final re-review. Refresh the body at the same time: it still describes the superseded mutable fix/old #1541 head, a deleted P4b base, and the earlier cbdd1ba evidence rather than the final branch state.

thymikee added a commit that referenced this pull request Aug 1, 2026
* fix: clear the polynomial-redos class across main

Three sites of the same CodeQL js/polynomial-redos family:

- packages/replay-test session-test-artifacts/-discovery slugs trimmed edge
  dashes with /^-+|-+$/g, which backtracks polynomially on long dash runs
  built from caller-supplied paths (alerts #27/#28). Replaced with a shared
  linear trimEdgeDashes.
- src/replay/target-identity.ts's target-v1 annotation line regex had the
  \s+(.*) ambiguity (the shape flagged as alert #29 on the #1536 copy).
  Anchored the payload group on \S so the split point is unique; the only
  caller matches against trimmed lines, so behavior is unchanged.

Adversarial regression test on the slug path (100k-char dash run,
sub-second); the annotation-regex adversarial case is covered on the #1536
package copy and the frozen replay-compat corpus passes here unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>

* test: make the redos regression fail against the retired regex form

The edge-run input matched the old /^-+|-+$/g in one pass; the quadratic
case is an interior run (each dash restarts a -+$ attempt that fails at the
trailing byte). The slug pipeline collapses runs before trimming, so the
test targets trimEdgeDashes directly and asserts the input comes back
byte-identical.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: drop the import the test rewrite orphaned

Co-Authored-By: Claude <noreply@anthropic.com>

* test: pin the all-dash fallback identifiers

Artifact slug falls back to 'test', invocation id to 'suite', and a
session-name slug that trims to nothing is omitted without a dangling
separator.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
thymikee and others added 4 commits August 1, 2026 15:13
Moves the mutually-coupled .ad read/write codec (script.ts, script-utils.ts,
script-formatting.ts, open-script.ts) plus the target-v1 annotation SERDE
slice of target-identity.ts into a new private leaf package,
@agent-device/ad-script, exporting only `.`. This is option 1 from the P5
scoping dossier on #1478: the codec is shared by the daemon's session-script
publication writer, the future replay engine, the CLI's `replay export`, and
Maestro's failure-label formatting, so it can no longer live in root src/
once packages/ad-replay lands (R11 forbids a package reaching into root src),
and a second export subpath or writer-half duplication are both ruled out by
existing gates/tests.

target-identity.ts keeps only the record/replay-shared classification core
(classifyTargetBindingMatch, local-identity/ancestry-prefix matching),
importing its shared types from the new package. Every real consumer
(re-derived by grep, not the dossier's list alone) is rewired to
@agent-device/ad-script.

Refs #1478

Co-Authored-By: Claude <noreply@anthropic.com>
…xport boundary

- packages/ad-script/src/index.ts: drop parseReplaySeriesFlags,
  formatTargetAnnotationCommentLine, parseTargetAnnotationCommentLine,
  TargetAnnotationLineParseResult, and TargetRect from the public façade —
  none has a consumer outside the package (re-swept every remaining export
  by grep; everything else kept has at least one real external importer).
  The functions/types stay exported from their declaring internal modules
  for the package's own internal use (script.ts, script-formatting.ts).
- scripts/layering/package-boundaries.test.ts: add the parallel R11
  assertions "the real tree parses, declares, and passes R11" already makes
  for maestro/provider-webdriver/provider-limrun/xml — ad-script exports
  exactly `.`, depends on exactly contracts+kernel, and is declared in root
  package.json — plus ad-script entries in the deep-resolution rejection
  coverage. Verified the lock catches a regression: temporarily added a
  fake `./codec` export to packages/ad-script/package.json and confirmed
  both the export-key-list assertion and the deep-resolution-rejection
  assertion fail; removed the plant and reconfirmed green.

Refs #1478

Co-Authored-By: Claude <noreply@anthropic.com>
…line regex

CodeQL js/polynomial-redos flagged TARGET_ANNOTATION_LINE_RE
(packages/ad-script/src/internal/target-annotation-serde.ts): the payload
group's `\s+(.*)` let `\s+` and the unconstrained `.*` both match whitespace,
so a run of separator whitespace that ultimately fails to complete the match
has many `\s+`/`.*` splits to backtrack through before concluding failure.

Anchor the payload group on `\S` (the exact complement of `\s`), so the
mandatory `\s+` separator and the payload's first character can never
overlap — the split point becomes unique and no backtracking is possible.

Behavior-preserving: the only caller (parseTargetAnnotationCommentLine)
always matches against an already-.trim()-ed line, whose last character
(whenever the tag matches at all) is never whitespace — so a payload section
`\S.*` would reject (content that is entirely whitespace) can never reach
this regex through the real call path. Verified against the frozen
replay-compat corpus and the full serde/parser test suites, unmodified.

Added a regression test with the exact adversarial shape CodeQL/the reviewer
cited (many tab pairs after the version digits), asserting sub-second parse.

Refs #1478

Co-Authored-By: Claude <noreply@anthropic.com>
…irectly

The entry-point adversarial case matched greedily even with the retired
regex (trim strips edge whitespace and per-line input carries no newline),
so it proved nothing about the pattern. The regression surface is the
pattern itself: an interior tab run with an x-newline tail fails the match,
which the retired form re-split quadratically (3.7s at 100k tabs) and the
\S anchor rejects in one attempt.

Co-Authored-By: Claude <noreply@anthropic.com>
@thymikee
thymikee force-pushed the refactor/extract-ad-script-codec branch from 5df4b05 to 754274c Compare August 1, 2026 13:19
@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 754274c9e: clean. The merged #1541/#1546 fixes are deduped (no PR-local snapshot delta; one moved annotation regex), and the resync introduces no P5 semantic drift from the previously reviewed tree. The extracted codec preserves ADR 0012 SERDE semantics, keeps classification outside the package, and the consumer-derived one-export/contracts+kernel boundary remains locked by R11. The refreshed body accurately reflects main, dependencies, scope, gates, and live evidence. Android helper-backed replay and iOS gesture replay cover the shipped route; iOS checkout remains the independently main-reproduced #1542 residual. All completed exact-head checks are green; the remaining checks are still in progress with no failure. Ready for human review.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 1, 2026
@thymikee

thymikee commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Resync complete at head 754274c9e — ready for the final re-review.

Per the sequencing in your last re-review:

Generated by Claude Code

@thymikee
thymikee merged commit b9509fe into main Aug 1, 2026
33 checks passed
@thymikee
thymikee deleted the refactor/extract-ad-script-codec branch August 1, 2026 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants