refactor(daemon): extract native .ad replay to packages/ad-replay (#1478 P5) - #1555
refactor(daemon): extract native .ad replay to packages/ad-replay (#1478 P5)#1555thymikee wants to merge 21 commits into
Conversation
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Review verdict: request changes / not ready at
The selector port direction, package dependency direction, coordinator ownership, and current CI are otherwise clean. The PR should remain draft: #1478 still requires both exact-head live replay suites plus target-v1 verification, divergence, resume, and save-script repair evidence; the iOS checkout leg is currently blocked by #1542. No |
…inspectAdReplay (#1555 review) P1 "do not smuggle daemon wire failures through a generic": drop the TResponse generic from AdReplayStepRuntime/runAdReplay. executeStep and handleActionFailure now return neutral tagged AdReplayStepOutcome/ AdReplayStepFailure values (kind/message/artifactPaths only); runAdReplay returns a neutral completed/failed AdReplayRunOutcome. The engine never holds or returns a DaemonResponse. The daemon adapter (createAdReplayStepRuntime, session-replay-runtime.ts) keeps its real wire response in a local side-map as it builds each neutral outcome, and runReplayScriptFile reads it back once runAdReplay reports which step failed, so the final response is byte-identical to before this split. P1 "parsing/planning/digest/resume must also occur behind runAdReplay": relocate computeReplayPlanDigest's call site and the --from/--plan-digest resume-point math (resolveReplayEntryIndex) behind inspectAdReplay's manifest as planDigest and a resolveEntryIndex closure. Neither is a new top-level export -- inspectAdReplay/runAdReplay stay the only two. Timing is preserved exactly (still called eagerly in prepareReplayPlan, before prepareReplaySession's coordinator-mutating side effects) since moving resume validation to run inside runAdReplay itself would let a rejected --from request mutate coordinator/session state first -- a real ordering hazard, not just a cosmetic one. computeReplayPlanDigest/ReplayPlanDigestMetadata/resolveReplayEntryIndex leave the ad-replay façade; request-router-repair-expired.test.ts and prepareReplayPlan read the digest/resume result off the manifest instead.
…replay façade (#1555 review) P1 "complete the binding façade instead of documenting deviations": classifyTargetBindingMatch never had a real consumer reachable through inspectAdReplay/runAdReplay -- both its callers (the daemon's record-time self-check in session-target-evidence.ts and its replay-time classification wrapper in session-replay-target-classification.ts) are daemon files that imported it directly. It interprets TargetAnnotationV1 evidence semantics shared beyond the engine, so it moves to packages/ad-script alongside target-annotation-identity.ts (new target-annotation-classification.ts + its test), and both daemon call sites now import it from there instead of @agent-device/ad-replay. One deviation remains and is reported rather than papered over per the review's own instruction: the four target-verification policy functions (planPreDispatchTargetVerification, planPostResolutionTargetVerification, deriveReplayTargetGuardMismatchEvidence, deriveWaitLandmarkMismatchEvidence) and the ReplaySelectorPort type family stay exported. Their sole caller, session-replay-target-verification.ts, interleaves these pure decisions with daemon-only async work (capture, SessionStore, coordinator/resume stamping, wire shaping) that must stay outside the engine by design; moving their call sites to live only behind runAdReplay would require restructuring that whole orchestration into new fine-grained AdReplayStepRuntime capabilities, which is out of scope for this pass. See packages/ad-replay/src/index.ts's header comment for the full reasoning. P1 "add the reviewer-required exact exported-symbol gate": adds readNamedExports (scripts/layering/package-boundaries.ts), a small parser over a façade's `export { .. } from`, `export type { .. } from`, and direct-declaration forms, and pins @agent-device/ad-replay's exact 21-symbol export list in package-boundaries.test.ts. Plant-verified: a stray `export const` addition failed the assertion; removed it and the gate went green again.
…#1555 review) Moves the verify-then-dispatch decision flow into packages/ad-replay's step loop so the four target-verification policy functions (plan{PostResolution,PreDispatch}TargetVerification, derive{ReplayTargetGuardMismatch,WaitLandmark}MismatchEvidence) become engine-private and leave the ad-replay façade. The daemon (session-replay-target-verification.ts) shrinks to the narrow AdReplayStepRuntime capabilities the engine drives: routing (beginTargetVerification), capture (captureObservation), classification (classifyTarget), dispatch (dispatchStep), and wire-building (buildRecordedUnverifiableFailure, buildTargetBindingFailure, buildPostDispatchTargetBindingFailure). Wire output and replay-compat stay byte-identical; the exact-symbol façade gate is updated to the shrunken export list.
|
Re-review verdict: still request changes at Resolved since Remaining blockers:
The prior live evidence is for |
|
All four findings addressed at head P1 invalid-backend rejection — restored at the authoritative point ( P1 complete the façade — done, no deviations remain. Shared vocabulary went to its proper owner ( P1 no wire smuggling — the P2 second adapter conformance — the in-memory adapter honors Live-readiness — round-2 evidence at Remaining ready-for-human blocker: #1542 (iOS checkout leg). The PR stays draft until that's fixed or the corpus is repaired without weakening coverage. Generated by Claude Code |
5a4f4c2 to
e6cbe6b
Compare
…inspectAdReplay (#1555 review) P1 "do not smuggle daemon wire failures through a generic": drop the TResponse generic from AdReplayStepRuntime/runAdReplay. executeStep and handleActionFailure now return neutral tagged AdReplayStepOutcome/ AdReplayStepFailure values (kind/message/artifactPaths only); runAdReplay returns a neutral completed/failed AdReplayRunOutcome. The engine never holds or returns a DaemonResponse. The daemon adapter (createAdReplayStepRuntime, session-replay-runtime.ts) keeps its real wire response in a local side-map as it builds each neutral outcome, and runReplayScriptFile reads it back once runAdReplay reports which step failed, so the final response is byte-identical to before this split. P1 "parsing/planning/digest/resume must also occur behind runAdReplay": relocate computeReplayPlanDigest's call site and the --from/--plan-digest resume-point math (resolveReplayEntryIndex) behind inspectAdReplay's manifest as planDigest and a resolveEntryIndex closure. Neither is a new top-level export -- inspectAdReplay/runAdReplay stay the only two. Timing is preserved exactly (still called eagerly in prepareReplayPlan, before prepareReplaySession's coordinator-mutating side effects) since moving resume validation to run inside runAdReplay itself would let a rejected --from request mutate coordinator/session state first -- a real ordering hazard, not just a cosmetic one. computeReplayPlanDigest/ReplayPlanDigestMetadata/resolveReplayEntryIndex leave the ad-replay façade; request-router-repair-expired.test.ts and prepareReplayPlan read the digest/resume result off the manifest instead.
…replay façade (#1555 review) P1 "complete the binding façade instead of documenting deviations": classifyTargetBindingMatch never had a real consumer reachable through inspectAdReplay/runAdReplay -- both its callers (the daemon's record-time self-check in session-target-evidence.ts and its replay-time classification wrapper in session-replay-target-classification.ts) are daemon files that imported it directly. It interprets TargetAnnotationV1 evidence semantics shared beyond the engine, so it moves to packages/ad-script alongside target-annotation-identity.ts (new target-annotation-classification.ts + its test), and both daemon call sites now import it from there instead of @agent-device/ad-replay. One deviation remains and is reported rather than papered over per the review's own instruction: the four target-verification policy functions (planPreDispatchTargetVerification, planPostResolutionTargetVerification, deriveReplayTargetGuardMismatchEvidence, deriveWaitLandmarkMismatchEvidence) and the ReplaySelectorPort type family stay exported. Their sole caller, session-replay-target-verification.ts, interleaves these pure decisions with daemon-only async work (capture, SessionStore, coordinator/resume stamping, wire shaping) that must stay outside the engine by design; moving their call sites to live only behind runAdReplay would require restructuring that whole orchestration into new fine-grained AdReplayStepRuntime capabilities, which is out of scope for this pass. See packages/ad-replay/src/index.ts's header comment for the full reasoning. P1 "add the reviewer-required exact exported-symbol gate": adds readNamedExports (scripts/layering/package-boundaries.ts), a small parser over a façade's `export { .. } from`, `export type { .. } from`, and direct-declaration forms, and pins @agent-device/ad-replay's exact 21-symbol export list in package-boundaries.test.ts. Plant-verified: a stray `export const` addition failed the assertion; removed it and the gate went green again.
…#1555 review) Moves the verify-then-dispatch decision flow into packages/ad-replay's step loop so the four target-verification policy functions (plan{PostResolution,PreDispatch}TargetVerification, derive{ReplayTargetGuardMismatch,WaitLandmark}MismatchEvidence) become engine-private and leave the ad-replay façade. The daemon (session-replay-target-verification.ts) shrinks to the narrow AdReplayStepRuntime capabilities the engine drives: routing (beginTargetVerification), capture (captureObservation), classification (classifyTarget), dispatch (dispatchStep), and wire-building (buildRecordedUnverifiableFailure, buildTargetBindingFailure, buildPostDispatchTargetBindingFailure). Wire output and replay-compat stay byte-identical; the exact-symbol façade gate is updated to the shrunken export list.
…les (#1555) Splits the ~1096-line replay runtime into cohesive pieces, keeping session-replay-runtime.ts as thin orchestration (~240 LOC): - session-replay-runtime-engine-adapter.ts: the AdReplayStepRuntime adapter (createAdReplayStepRuntime, the build*Failure capability implementations, and the lastResponse/lastObservation side-map mechanics), extracted verbatim. - session-replay-runtime-plan.ts: extended with the plan-side helpers (validateReplayBackendFlag, inspectReplayPlanManifest, resolveReplayPlanEntryIndex, prepareReplayPlan, routeMaestroReplay) alongside the buildReplayMetadataFlags helper already there — buildReplayMetadataFlags is now module-private since its one caller moved into the same file. Also introduces ReplayScriptFileParams, named here (instead of derived via Parameters<typeof runReplayScriptFile>) so routeMaestroReplay can reference the shape without importing back from session-replay-runtime.ts. - session-replay-runtime-session.ts (new): session preparation (prepareReplaySession and its coordinator arming/repair-preflight helpers), extracted verbatim. Coordinator ownership is unchanged: createReplayCoordinator is still constructed only in session-replay-runtime.ts, matching replay-coordinator-ownership.test.ts's allowlist as-is — every extracted module receives the already-constructed ReplayCoordinator as a parameter. Pure move; no behavior change.
…tion (#1555) Two invariants found during the P5 decomposition pass now have direct counterfactual-verified coverage: - packages/ad-replay/src/internal/__tests__/step-loop.test.ts: a post-dispatch target-binding mismatch (dispatchWithGuard) must report the accumulated PRE-step artifact snapshot it was called with, never the artifacts the failed dispatch itself produced. Verified red by swapping the buildPostDispatchTargetBindingFailure call to outcome.artifactPaths. - src/daemon/handlers/__tests__/session-replay-runtime-plan.test.ts: a rejected --from/--plan-digest resume must never reach prepareReplaySession's coordinator-mutating writes (the R2 ordering invariant) — a pre-armed repair transaction and corrective-resume watermark are asserted byte-for-byte unchanged after rejection. Verified red by calling prepareReplaySession before honoring the plan-validation rejection.
|
Resync complete at head Since the redraft: rebased onto main absorbing #1554 (its terminal-lifecycle policy folded INTO the engine step loop, unified with the repair terminal-close predicate as one suppression condition; the daemon module deleted; all six #1554 unit tests pass unchanged end-to-end, plus five new package-internal Round-3 live evidence at exact head Prior review findings all addressed in Generated by Claude Code |
|
Review findings at e6cbe6b:\n\n- [P1] Enforce the accepted two-entrypoint facade. The approved P5 amendment says packages/ad-replay exports only inspectAdReplay and runAdReplay; the current facade additionally exports formatReplaySuccessMessage, runtime/outcome types, and the full selector-port family, and the new exact-symbol gate blesses that widening. The gate also ignores export-star declarations, so it can miss future widening. Keep success formatting daemon-side, hide internal capability/selector types behind entrypoint signatures, and make the gate reject every export form.\n- [P1] Translate wire failures before the engine boundary. AdReplayDispatchOutcome carries details as a generic unknown-valued record, the daemon adapter assigns response.error.details verbatim, and engine policy parses that bag. This is still daemon wire projection crossing into the engine despite the PR's neutral-outcomes/no-generic claim. Narrow each mismatch in the adapter into explicit tagged evidence values.\n- [P1] Move variable semantics/planning behind the replay entrypoint. The daemon still assembles ReplayVarScope, interpolates actions in invokeReplayAction, and independently interpolates target verification. P5 assigns variables and planning to ad-replay; leaving these paths daemon-owned preserves duplicated orchestration/semantics.\n- [P1] Keep this draft pending exact-head live evidence. The PR body's live corpus is for 947b90c, not current head e6cbe6b, and the prescribed iOS checkout leg remains blocked by #1542. Re-run the full required corpus on the corrected exact head before readiness.\n\nThe #1554 terminal-close fold-in and current CI checks look sound, but the accepted P5 boundary/readiness gates are not yet met. |
… P1) packages/ad-replay/src/index.ts now exports exactly two value symbols, inspectAdReplay and runAdReplay, and zero types — formatReplaySuccessMessage (presentation) moves beside its one caller in session-replay-runtime.ts, and every type a root daemon file needs is derived structurally off the two entrypoints in the one new src/daemon/ad-replay-facade-types.ts module instead of being named off the façade. scripts/layering/package-boundaries.ts's readNamedExports is rewritten on oxc-parser's own static-export table instead of a regex, so it can no longer silently miss a widening export form: a bare `export *` re-export or an `export default` now throws (an un-enumerable, and therefore un-pinnable, export), while `export * as ns` and every other enumerable form is still counted. The pinned exact-symbol assertion in package-boundaries.test.ts is narrowed to ['inspectAdReplay', 'runAdReplay'].
…1555 review P1) AdReplayDispatchOutcome's guard-mismatch/landmark-mismatch variants carried a generic `details: Record<string, unknown> | undefined` bag straight off the wire response — a daemon wire projection crossing into the engine even though the outcome itself was already a neutral type. The daemon adapter (session-replay-runtime-engine-adapter.ts) now narrows that bag into the typed AdReplayGuardMismatchEvidence/AdReplayLandmarkMismatchEvidence shapes (observed identity, expected/observed structural denotation, ancestry entries, match count) before returning the outcome; the unknown-parsing readers move there with the wire-reading responsibility they always were. target-verification.ts's deriveReplayTargetGuardMismatchEvidence/ deriveWaitLandmarkMismatchEvidence now consume only the typed values — no `unknown`-valued record type remains on any engine-crossing signature.
…1555 review P1) The daemon assembled the `${VAR}` scope (buildPreparedReplayScope) and interpolated actions at two independent call sites: dispatch's own (invokeReplayAction) and target verification's separate one (resolveTargetVerificationEntry) — duplicated orchestration the P5 design assigns to the engine. runAdReplay's request now carries the raw scope INPUTS (varSources: plain builtins/file/shell/cli-env data, plus actionLines/actionSourcePaths/ resolvedPath for interpolation-error location) instead of a built scope; the engine builds the scope and resolves each action exactly once per step, handing the RESOLVED action to dispatchStep/beginTargetVerification while every other capability still receives the ORIGINAL recorded action (a target-binding divergence reports the recorded selector, never an expanded ${VAR}). This is the one resolution site now — session-replay-action-runtime.ts's invokeReplayAction and session-replay-target-verification.ts's resolveTargetVerificationEntry no longer hold a scope or call resolveReplayAction themselves. Scrub-value collection (collectReplayScrubbableVarValues, for divergence-report redaction) is kept single-sourced in the engine too: it's computed from the engine's own live scope and threaded to each build-failure/handleActionFailure capability as an explicit scrubVars argument, rather than the daemon recomputing it from a second scope object (which would have gone stale, since expandedBuiltinNames tracking now only happens engine-side). The Maestro replay path's own daemon-side vars usage is unrelated (a different engine) and is out of scope here.
…s/ad-replay Stage A of the #1478 P5 extraction: vars, plan-digest (+canonical-json, sole consumer), the target-identity classification core, report-action, and suggestion-ranking move verbatim; imports updated. The package facade temporarily re-exports the moved symbols so root consumers keep compiling; a later stage narrows it to inspectAdReplay/runAdReplay only.
…h dual adapters (#1478 P5)
…emon authority (#1478 P5)
…d-script (#1555 review)
…inspectAdReplay (#1555 review) P1 "do not smuggle daemon wire failures through a generic": drop the TResponse generic from AdReplayStepRuntime/runAdReplay. executeStep and handleActionFailure now return neutral tagged AdReplayStepOutcome/ AdReplayStepFailure values (kind/message/artifactPaths only); runAdReplay returns a neutral completed/failed AdReplayRunOutcome. The engine never holds or returns a DaemonResponse. The daemon adapter (createAdReplayStepRuntime, session-replay-runtime.ts) keeps its real wire response in a local side-map as it builds each neutral outcome, and runReplayScriptFile reads it back once runAdReplay reports which step failed, so the final response is byte-identical to before this split. P1 "parsing/planning/digest/resume must also occur behind runAdReplay": relocate computeReplayPlanDigest's call site and the --from/--plan-digest resume-point math (resolveReplayEntryIndex) behind inspectAdReplay's manifest as planDigest and a resolveEntryIndex closure. Neither is a new top-level export -- inspectAdReplay/runAdReplay stay the only two. Timing is preserved exactly (still called eagerly in prepareReplayPlan, before prepareReplaySession's coordinator-mutating side effects) since moving resume validation to run inside runAdReplay itself would let a rejected --from request mutate coordinator/session state first -- a real ordering hazard, not just a cosmetic one. computeReplayPlanDigest/ReplayPlanDigestMetadata/resolveReplayEntryIndex leave the ad-replay façade; request-router-repair-expired.test.ts and prepareReplayPlan read the digest/resume result off the manifest instead.
…replay façade (#1555 review) P1 "complete the binding façade instead of documenting deviations": classifyTargetBindingMatch never had a real consumer reachable through inspectAdReplay/runAdReplay -- both its callers (the daemon's record-time self-check in session-target-evidence.ts and its replay-time classification wrapper in session-replay-target-classification.ts) are daemon files that imported it directly. It interprets TargetAnnotationV1 evidence semantics shared beyond the engine, so it moves to packages/ad-script alongside target-annotation-identity.ts (new target-annotation-classification.ts + its test), and both daemon call sites now import it from there instead of @agent-device/ad-replay. One deviation remains and is reported rather than papered over per the review's own instruction: the four target-verification policy functions (planPreDispatchTargetVerification, planPostResolutionTargetVerification, deriveReplayTargetGuardMismatchEvidence, deriveWaitLandmarkMismatchEvidence) and the ReplaySelectorPort type family stay exported. Their sole caller, session-replay-target-verification.ts, interleaves these pure decisions with daemon-only async work (capture, SessionStore, coordinator/resume stamping, wire shaping) that must stay outside the engine by design; moving their call sites to live only behind runAdReplay would require restructuring that whole orchestration into new fine-grained AdReplayStepRuntime capabilities, which is out of scope for this pass. See packages/ad-replay/src/index.ts's header comment for the full reasoning. P1 "add the reviewer-required exact exported-symbol gate": adds readNamedExports (scripts/layering/package-boundaries.ts), a small parser over a façade's `export { .. } from`, `export type { .. } from`, and direct-declaration forms, and pins @agent-device/ad-replay's exact 21-symbol export list in package-boundaries.test.ts. Plant-verified: a stray `export const` addition failed the assertion; removed it and the gate went green again.
…#1555 review) Moves the verify-then-dispatch decision flow into packages/ad-replay's step loop so the four target-verification policy functions (plan{PostResolution,PreDispatch}TargetVerification, derive{ReplayTargetGuardMismatch,WaitLandmark}MismatchEvidence) become engine-private and leave the ad-replay façade. The daemon (session-replay-target-verification.ts) shrinks to the narrow AdReplayStepRuntime capabilities the engine drives: routing (beginTargetVerification), capture (captureObservation), classification (classifyTarget), dispatch (dispatchStep), and wire-building (buildRecordedUnverifiableFailure, buildTargetBindingFailure, buildPostDispatchTargetBindingFailure). Wire output and replay-compat stay byte-identical; the exact-symbol façade gate is updated to the shrunken export list.
… into the ad-replay engine Rebasing p5/extract-ad-replay onto main pulled in #1554's --keep-session feature, which had grown its own daemon-side terminal-close-suppression predicate (session-replay-terminal-lifecycle.ts's resolveSuppressedTerminalCloseIndex/countExecutedReplayActions) independently of this branch's own engine-side one (step-loop.ts's isRepairArmedTerminalCloseAction). Both are the same decision family — replay --keep-session and an active --save-script repair now share ONE structural resolution (resolveSuppressedTerminalCloseIndex, generalized to "terminal among EXECUTABLE actions" rather than the old physical-last-index check) and one suppression check inside runAdReplay, gated on keepSession OR runtime.isRepairArmed(). AdReplayRunRequest grew a keepSession field; the neutral 'replayed' count in AdReplayRunOutcome is now computed inline in the loop instead of the daemon's old actions.length - entryIndex approximation. requireLiveSessionForKeepSession (the --keep-session live-session postcondition) stays daemon-side, inlined into session-replay-runtime.ts, since it inspects SessionStore state the engine never sees. The daemon-only session-replay-terminal-lifecycle.ts this arrived with is deleted entirely — its isExecutableReplayAction was a duplicate of the engine's own. runReplayScriptFile's Maestro-format routing (including the new --keep-session Maestro rejection) was extracted into routeMaestroReplay to keep the function under fallow's complexity threshold after re-threading keepSession through it. Added packages/ad-replay/src/internal/__tests__/step-loop.test.ts covering the unified suppression decision (both keepSession and repair-armed) directly against runAdReplay, including the terminal-among-executable-actions case with a trailing nested replay marker. The daemon-level integration tests (6 tests in session-replay-terminal-lifecycle.test.ts, exercising the same behavior through runReplayScriptFile) and the SDK provider-scenario test (active-session-script-publication.test.ts) needed no changes and pass unmodified.
…les (#1555) Splits the ~1096-line replay runtime into cohesive pieces, keeping session-replay-runtime.ts as thin orchestration (~240 LOC): - session-replay-runtime-engine-adapter.ts: the AdReplayStepRuntime adapter (createAdReplayStepRuntime, the build*Failure capability implementations, and the lastResponse/lastObservation side-map mechanics), extracted verbatim. - session-replay-runtime-plan.ts: extended with the plan-side helpers (validateReplayBackendFlag, inspectReplayPlanManifest, resolveReplayPlanEntryIndex, prepareReplayPlan, routeMaestroReplay) alongside the buildReplayMetadataFlags helper already there — buildReplayMetadataFlags is now module-private since its one caller moved into the same file. Also introduces ReplayScriptFileParams, named here (instead of derived via Parameters<typeof runReplayScriptFile>) so routeMaestroReplay can reference the shape without importing back from session-replay-runtime.ts. - session-replay-runtime-session.ts (new): session preparation (prepareReplaySession and its coordinator arming/repair-preflight helpers), extracted verbatim. Coordinator ownership is unchanged: createReplayCoordinator is still constructed only in session-replay-runtime.ts, matching replay-coordinator-ownership.test.ts's allowlist as-is — every extracted module receives the already-constructed ReplayCoordinator as a parameter. Pure move; no behavior change.
…tion (#1555) Two invariants found during the P5 decomposition pass now have direct counterfactual-verified coverage: - packages/ad-replay/src/internal/__tests__/step-loop.test.ts: a post-dispatch target-binding mismatch (dispatchWithGuard) must report the accumulated PRE-step artifact snapshot it was called with, never the artifacts the failed dispatch itself produced. Verified red by swapping the buildPostDispatchTargetBindingFailure call to outcome.artifactPaths. - src/daemon/handlers/__tests__/session-replay-runtime-plan.test.ts: a rejected --from/--plan-digest resume must never reach prepareReplaySession's coordinator-mutating writes (the R2 ordering invariant) — a pre-armed repair transaction and corrective-resume watermark are asserted byte-for-byte unchanged after rejection. Verified red by calling prepareReplaySession before honoring the plan-validation rejection.
… P1) packages/ad-replay/src/index.ts now exports exactly two value symbols, inspectAdReplay and runAdReplay, and zero types — formatReplaySuccessMessage (presentation) moves beside its one caller in session-replay-runtime.ts, and every type a root daemon file needs is derived structurally off the two entrypoints in the one new src/daemon/ad-replay-facade-types.ts module instead of being named off the façade. scripts/layering/package-boundaries.ts's readNamedExports is rewritten on oxc-parser's own static-export table instead of a regex, so it can no longer silently miss a widening export form: a bare `export *` re-export or an `export default` now throws (an un-enumerable, and therefore un-pinnable, export), while `export * as ns` and every other enumerable form is still counted. The pinned exact-symbol assertion in package-boundaries.test.ts is narrowed to ['inspectAdReplay', 'runAdReplay'].
…1555 review P1) AdReplayDispatchOutcome's guard-mismatch/landmark-mismatch variants carried a generic `details: Record<string, unknown> | undefined` bag straight off the wire response — a daemon wire projection crossing into the engine even though the outcome itself was already a neutral type. The daemon adapter (session-replay-runtime-engine-adapter.ts) now narrows that bag into the typed AdReplayGuardMismatchEvidence/AdReplayLandmarkMismatchEvidence shapes (observed identity, expected/observed structural denotation, ancestry entries, match count) before returning the outcome; the unknown-parsing readers move there with the wire-reading responsibility they always were. target-verification.ts's deriveReplayTargetGuardMismatchEvidence/ deriveWaitLandmarkMismatchEvidence now consume only the typed values — no `unknown`-valued record type remains on any engine-crossing signature.
…1555 review P1) The daemon assembled the `${VAR}` scope (buildPreparedReplayScope) and interpolated actions at two independent call sites: dispatch's own (invokeReplayAction) and target verification's separate one (resolveTargetVerificationEntry) — duplicated orchestration the P5 design assigns to the engine. runAdReplay's request now carries the raw scope INPUTS (varSources: plain builtins/file/shell/cli-env data, plus actionLines/actionSourcePaths/ resolvedPath for interpolation-error location) instead of a built scope; the engine builds the scope and resolves each action exactly once per step, handing the RESOLVED action to dispatchStep/beginTargetVerification while every other capability still receives the ORIGINAL recorded action (a target-binding divergence reports the recorded selector, never an expanded ${VAR}). This is the one resolution site now — session-replay-action-runtime.ts's invokeReplayAction and session-replay-target-verification.ts's resolveTargetVerificationEntry no longer hold a scope or call resolveReplayAction themselves. Scrub-value collection (collectReplayScrubbableVarValues, for divergence-report redaction) is kept single-sourced in the engine too: it's computed from the engine's own live scope and threaded to each build-failure/handleActionFailure capability as an explicit scrubVars argument, rather than the daemon recomputing it from a second scope object (which would have gone stale, since expandedBuiltinNames tracking now only happens engine-side). The Maestro replay path's own daemon-side vars usage is unrelated (a different engine) and is out of scope here.
717b0e4 to
a554fae
Compare
CodeQL flagged the interpolation regex's fallback group as js/polynomial-redos
once vars.ts moved into packages/ (library-input classification): every
${NAME:- prefix of an unclosed input rescanned to end-of-string, quadratic
overall — 1,857 ms measured on 20k repetitions of '${A:-['. Replaced with a
single-pass scanner; failed fallback scans emit their span verbatim and resume
after it (escape-pair alignment is identical from every candidate start inside
the span, so no later candidate can terminate where the failed scan could not).
Equivalence: 200k-trial differential fuzz against the retired regex over the
adversarial alphabet, zero mismatches; both adversarial shapes now resolve in
1-2 ms.
|
All four findings from the latest review addressed; branch reconciled onto main; all CI green at head P1 façade — P1 wire translation — P1 variables/planning behind the entrypoint — the daemon passes plain Reconcile + one new finding fixed en route — rebased onto main absorbing #1558/#1559 (zero conflicts; P1 exact-head live evidence at Remaining readiness blocker is unchanged and external to this branch: #1542 defect 2 (post-gesture stabilization semantics — options awaiting a maintainer decision in #1559's body). Everything else in the ready-gate is now green at the exact head. Generated by Claude Code |
|
Re-review at bfa4b04: the three code P1s are resolved. The package facade now exports only inspectAdReplay/runAdReplay with an AST-based gate that rejects unbounded/default exports; wire error.details is narrowed into typed mismatch evidence in the daemon adapter; and variable scope/interpolation now runs once inside runAdReplay. Exact-head CI is green, and no new code finding was found.\n\nReadiness remains blocked:\n\n- [P1 validation] The prescribed iOS checkout leg is still red on #1542 defect 2. #1559 merged only the first defect and explicitly left stale-AX stabilization unresolved, so P5 step 7 is not satisfied. Keep this draft and do not apply ready-for-human until that corpus is green (or the accepted gate is deliberately changed without weakening coverage).\n- [P1 evidence] Preserve the claimed exact-head extended evidence. /private/tmp/ad-p5-live-artifacts-final/ contains the suite JUnit files and three recorded scripts, but no command log/results substantiating the claimed target-v1 replay, divergence, resume, repair, keep-session, and variable-execution legs. Attach/preserve those outputs or put verifiable result excerpts in the PR before readiness.\n\nAlso refresh the PR body: it still describes the old expanded facade and older validation heads, contradicting the current implementation and latest response. |
refactor(daemon): extract native .ad replay to packages/ad-replay (#1478 P5)
Implements P5 per the approved design of record (proposal, binding amendment), built as staged, individually-gated commits on one branch, cutting over atomically in this PR. Step 2 of the approved sequence (behavior pinning) merged separately as #1552 and is this PR's regression net — none of those tests' assertions changed anywhere in this branch. All four findings from the first review are addressed in commits
281f33599..947b90ca3; the response comment maps them one-by-one.The façade — complete, no deviations
packages/ad-replayexports exactlyinspectAdReplay,runAdReplay,formatReplaySuccessMessage, and the types their signatures reference (AdReplayManifest,AdReplayDigestFlags,AdReplayRunOutcome,AdReplayStepOutcome,AdReplayStepFailure,AdReplayStepRuntime, and theReplaySelectorPortfamily). Packageexportsmap is exactly".", and a new exact exported-symbol gate inscripts/layering/package-boundaries.test.tspins the named list (plant-verified: a stray export fails the gate with a clear diff).inspectAdReplay(sourcePath)returns the manifest includingplanDigestandresolveEntryIndex(...); the digest/resume internals (plan-digest.ts,resume.ts) are package-private. Resume validation deliberately stays eager inprepareReplayPlancall order — moving it later would let a rejected--frommutate coordinator state before failing (traced ordering hazard, avoided).verifyAndDispatchStepin the package drives the verify-then-dispatch sequence, calling narrow daemon capabilities (beginTargetVerification,captureObservation,classifyTarget,dispatchStep,buildRecordedUnverifiableFailure,buildTargetBindingFailure,buildPostDispatchTargetBindingFailure,handleActionFailure). The four policy functions are package-private. The per-step order — including the subtlety that post-dispatch mismatch divergences use the pre-step artifact snapshot — is preserved exactly (mapped before/after in the stage report).AdReplayStepRuntimehas no type parameter. The engine returns tagged neutral outcomes (AdReplayRunOutcome/AdReplayStepFailure: kind, message, artifact paths, evidence values). The daemon adapter keeps a side-map (lastResponseclosure increateAdReplayStepRuntime) so the client-visibleDaemonResponseis the literal same object as pre-refactor — no wire type crosses the boundary in either direction (the onlyDaemonResponsementions inside the package are comments stating its absence).Shared vocabulary went to its owner
Per the review's "proper shared owner" alternative, measured per symbol: the identity vocabulary (
annotationLocalIdentity,matchesLocalIdentity,matchesAncestryPrefix,LocalIdentity,identityFieldMismatches,firstAncestryMismatch) andclassifyTargetBindingMatch(+vars.ts) moved topackages/ad-script— they interpret.ad/TargetAnnotationV1semantics and are consumed by recording-side root code (and, for vars, the Maestro path).session-replay-report-action.ts/session-replay-suggestion-ranking.tsmeasured as root-only consumers and moved back to the daemon (undoing an over-move). ad-script's façade additions are covered by its existing boundary row.The selector port
Three operations (
readSelectorExpression,resolveRecordedTarget— same-alternative winner+domain invariant implemented in the production adapter, lifted verbatim;buildSelectorCandidates), trafficking only in strings, kernel snapshot values, and tagged unions. Production adapter:src/daemon/replay-selector-port.ts. In-memory adapter:src/__tests__/test-utils/in-memory-replay-selector-port.ts— now honorsReplaySelectorCandidateOptions.nodeswith production's exact shared-ID drop semantics. Contract suite: 9 cells × 2 adapters (18 tests), including the shared-ID demotion cell (counterfactual: ignoringnodesfails the in-memory leg while production stays green). Two AST-needing helpers (resolveReplaySuggestionCandidate,readReplaySelectorDisplayValue) are daemon-side plain exports beside the adapter — provably inexpressible through the port without leaking the AST.Invalid-backend rejection restored
prepareReplayPlanrejects any non-maestroreplayBackendwith the byte-identicalINVALID_ARGSmessage from main, before any inspection or session work. Handler-level regression test proves zero step dispatch (counterfactual: without the guard, the script executed); a companion test pins thatmaestrostill routes.What moved / stayed (final)
${VAR}vars module (shared with the Maestro path).format.tsrouting (above both engines), report-action + suggestion-ranking,sanitizeIdentity/describeCandidate(pinned by snapshot-lines),target-identity-node.ts/target-evidence-tree.ts(shared with dispatch/recording).Rebase: #1554's keep-session absorbed into the engine (head
e6cbe6b76)After #1554 merged, this branch rebased onto main and folded its terminal-lifecycle policy into the P5 architecture rather than keeping a parallel decider:
resolveSuppressedTerminalCloseIndex/countExecutedReplayActionsunified with the engine's repair terminal-close predicate inside the step loop (one OR'd suppression condition,keepSession || runtime.isRepairArmed(), checked dynamically afterarmStepso a first-time--save-scriptarm is visible);AdReplayRunRequestgained one field (keepSession); the daemon'ssession-replay-terminal-lifecycle.tsmodule is deleted (no duplicateisExecutableReplayActionanywhere); theSessionStorepostcondition stays daemon-side. #1554's six unit tests pass unchanged end-to-end, plus five new package-internalrunAdReplaytests cover the unified policy directly. Thereplayedcount is now a per-dispatched-step counter (fixing the old approximation that over-counted nested-replay markers).Decomposition: the daemon adapter is now four cohesive modules
session-replay-runtime.tswent from ~1100 lines (post-rebase) to 242 — thin orchestration only. Extracted along its natural seams:session-replay-runtime-engine-adapter.ts(473: the runtime-bag capabilities,build*Failureimplementations, side-map mechanics),session-replay-runtime-plan.ts(261: backend validation, manifest inspection, resume-index resolution, Maestro routing),session-replay-runtime-session.ts(219: session preparation, repair preflight, save-script arming). Coordinator construction stays solely in the orchestrator — the ownership test passes untouched, no allowlist changes.Two load-bearing ordering invariants discovered during the extraction are now pinned by counterfactual-verified tests: post-dispatch mismatch divergences report the pre-step artifact snapshot (engine test; counterfactual red showed the failed dispatch's own artifacts leaking in), and a rejected
--from/--plan-digestnever reachesprepareReplaySession's coordinator-mutating writes (plan test; counterfactual red showedpendingRecordAndHealbeing cleared before the failure).Gates (re-run at every stage; latest full chain at head
e6cbe6b76)typecheck / lint / format:check / check:layering (53 tests incl. the new exact-symbol gate) / check:replay-compat (10 mined scripts, 6 tags, 12 digest-pinned entries) / fallow — green. Full vitest: 5351/5352 at the last full run with the only failures being the documented contention-timeout class (isolate-rerun green; the two pid-liveness assertion races are fixed separately in #1556). Fallow baseline: one surgical 8-line addition for the relocated in-memory adapter; full regen deliberately rejected (would silently drop 12 unrelated stale entries).
Live evidence (exact head
947b90ca3, round 2)Standard suites:
checkout-form-android.adgesture-lab-android.adgesture-lab.adcheckout-form.adExtended evidence (Android, constructed via the CLI's own record/replay loop; verbatim log in artifacts):
open --save-script; saved script carries# agent-device:target-v1 {"id":"refresh-metrics","role":"button",...,"verification":"verified"}. (Finding: annotations require arming atopen—close --save-scriptalone yields selector chains without target-v1 evidence, persession-open-surface.tsarming semantics.)REPLAY_DIVERGENCE, classificationselector-miss (matchCount 0) — recorded target evidence did not verify, with a record-and-heal repair suggestion including--from 3 --plan-digest 8ff7b932….replay --from 3 --plan-digest <hash>after correcting the screen —Replayed 2 steps in 0.8s, completed cleanly.@ref, resumed, explicitclose --save-scriptcommitted the repair; repaired script ends with# agent-device:heal-complete, carries fresh"verification":"verified"annotations for every step, and replays green (6 steps, 4.0s). Behavioral observation (informational, consistent with the repair-transaction commit boundary): an armed session's--fromresume does not auto-run a trailing scripted bareclose; the explicit close commits.Artifacts:
/private/tmp/ad-p5-live-artifacts-r2/(28 files incl. the verbatim command log, original/divergence/repaired scripts, per-attempt replay trees). Round-1 artifacts at the pre-review head remain in/private/tmp/ad-p5-live-artifacts/.Residual risks
Generated by Claude Code