refactor: extract the .ad script codec into packages/ad-script (#1478) - #1536
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
|
Reviewed head
Before readiness, also add the required live evidence for the moved replay/session-publication route: rebuilt clean-daemon iOS and Android No |
02a0ac5 to
b6761a8
Compare
|
Addressed both P2 items from review (commit e5c1e5a, on top of the rebase to 1. Façade trimmed to real consumersRe-swept every export in
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: 2. One-export boundary lockedAdded to
Also added Plant-proof: temporarily added a fake
Removed the plant; both assertions pass again (11/11 tests in that file). Gate evidence
Live-device evidenceLive iOS/Android Generated by Claude Code |
e5c1e5a to
5866a42
Compare
|
Re-reviewed latest head Not ready to label yet:
No |
|
Exact-head re-review at Remaining readiness blockers:
Code and CI are clean; no |
|
The reviewed head |
STOP-priority regression: root-caused and fixed (commit 71994d7)Repro confirmed
Mechanism
A live CDP pause on the wedged daemon ( The pathological input is real, not synthetic: the second Why this branch and not p4a/p4b/main
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. FixCache Regression test
Verified the test catches the regression: with the memoization reverted, the exact same test never returns within the suite's timeout ( Live verification (this repo's own emulator,
|
71994d7 to
cbdd1ba
Compare
|
Live Android — green
Active-path proof: This leg is also what caught the daemon CPU-spin fixed in 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)
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 |
|
Exact-head re-review at |
|
Re-reviewed exact head
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 |
8873022 to
5df4b05
Compare
|
Rebased and both findings addressed at head P1 — corrected occlusion, ahead of #1541's merge. The branch is rebased onto post-#1535/#1538 main (base auto-retargeted to P2 — the ReDoS regression is now non-vacuous, honestly scoped. You were right, and the vacuousness turned out to be structural: through 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 |
|
Re-reviewed at 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 |
* 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>
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>
5df4b05 to
754274c
Compare
|
Re-reviewed exact head |
|
Resync complete at head Per the sequencing in your last re-review:
Generated by Claude Code |
Summary
Implements option 1 from the P5 scoping dossier on #1478 ("the codec seam"): extracts the mutually-coupled
.adscript 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(openaction's read+write half)target-annotation-serde.ts: the target-v1 annotation SERDE slice split out ofsrc/replay/target-identity.ts— the wire type re-export, canonical field order, normalization (normalizeIdentifierField/normalizeRoleField/normalizeLabelField/truncateToUtf8Bytes/utf8ByteLength), size-cap constants, andserializeTargetAnnotationV1/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.adenv/var key-shape regex) moved fromsrc/replay/vars.tsintoscript.tsand is exported from the façade —vars.ts's env-directive parsing already lived in the codec, andvars.tsstays outside the package (it's engine, not codec) — so it now re-exports the constant from@agent-device/ad-scriptinstead of duplicating the regex literal.Split
src/replay/target-identity.ts— stays, classification-only now:classifyTargetBindingMatchand its input/output typesannotationLocalIdentity,matchesLocalIdentity,matchesAncestryPrefix,LocalIdentityTargetAnnotationV1,TargetAncestryEntry) from@agent-device/ad-scriptinstead of declaring them.Does not move (per the dossier, unchanged):
target-identity-node.ts(record-time local-identity reader overRawSnapshotNode) andtarget-evidence-tree.ts(tree-structural evidence primitives) stay insrc/replay/. Both needed a small import fix to source normalization helpers/types from@agent-device/ad-script.Test relocation
src/replay/__tests__/script.test.ts→packages/ad-script/src/internal/__tests__/script.test.ts, minus its one property-based round-trip test, which stays at root as newsrc/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.tssplit along the serde/classification line: serde tests moved to newpackages/ad-script/src/internal/__tests__/target-annotation-serde.test.ts;matchesAncestryPrefix/matchesLocalIdentitytests 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, typesParsedReplayScript/ReplayScriptMetadatascript-utils.ts:appendScriptSeriesFlags,formatDivergenceActionLabel,formatScriptArg,formatScriptStringLiteral,isClickLikeCommand,isTouchTargetCommand,stripRecordedRefGenerationscript-formatting.ts:formatPortableActionLine,formatTargetAnnotationLinestarget-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 typesTargetAncestryEntry/TargetAnnotationV1/TargetScrollRegion/TargetVerificationA 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.tsnow 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 exactlycontracts+kernel, is declared in rootpackage.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./codecsubpath export topackages/ad-script/package.jsonand 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.tsdoes not importsrc/replay/target-identity.ts— verified directly (it has zero imports). It's the reverse:target-identity.tsimports the annotation shape types from@agent-device/contracts/replay. No change topackages/contractswas needed or made.Registry updates
package.json:"@agent-device/ad-script": "workspace:*"indevDependencies, andpackages/ad-scriptin thetypecheckscript'stsc -blist.scripts/layering/model.ts:['ad-script', 1]added toTARGET_DAG_RANK, alongsidemaestro/replay-test/xmlat leaf rank 1.scripts/layering/daemon-modularity.tsintentionally not touched — thesrc/ad-replay/entries there are P5's, andad-script(a plain leaf codec) needs noLOGICAL_MODULE_POLICIESentry, matchingcontracts/kernel/xml's precedent.CodeQL: polynomial-redos in the annotation line regex (fixed)
TARGET_ANNOTATION_LINE_REintarget-annotation-serde.tslet\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 acrossmainby #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 owntest-app:replaylive evidence. The corrected (immutable-input) fix merged tomainas #1541; on rebase this branch's interim copy deduped away automatically, so the fix now lives only onmainand this PR carries zero occlusion changes (git diff main -- src/snapshot/is empty). Root-cause writeup: issuecomment-5150828335.Live evidence
Device validation (
m6.adminimal repro + the fullcheckout-form-android.ad+gesture-lab-android.adtwo-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 reviewed5df4b05f9tree 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 ontomainpast #1541/#1546/#1547)pnpm typecheck,pnpm lint,pnpm format:check— clean.pnpm check:layering— OK; R11 exact-export lock for@agent-device/ad-scriptholds (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.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).