feat(replay): ADR 0012 decision 6 — agent-supervised re-record repair#1228
Merged
Conversation
Implements "heal-by-doing": `replay --save-script` arms recording from the first attempt (R1), a boundary watermark scopes the healed `.ad` to the repair run (R6), the daemon computes a total `repairHint` enum at divergence time via genuine ancestor-containment over the daemon's own tree (R3), and the writer fails loud on a bare `@ref` in a repair-armed session instead of emitting an unreplayable script (R4). - session-replay-runtime.ts: `armReplaySaveScript` arms `session.recordSession` + `saveScriptBoundary` before every step; `--save-script`'s absence leaves replay byte-identical to today. - session-replay-repair-hint.ts (new): `computeReplayRepairHint` — a total mapping over the 4 divergence kinds, with two fail-safes to `manual` (no recorded evidence; sparse/unavailable capture). Wired into both divergence-minting sites (session-replay-divergence.ts, session-replay-target-verification.ts) and threaded through divergence.ts's bounding/minimal-fallback/text-report paths so it survives every response level and projection. - session-script-writer.ts: `saveScriptBoundary` slices the healed script to the repair run; a bare unresolved `@ref` throws in a repair-armed session (ordinary open/close --save-script keeps its existing refLabel fallback). - cli-help.ts: documents the repairHint-routed repair sub-flows and the close --save-script / <stem>.healed.ad promotion step. Tests: repairHint mapping (all 4 kinds x container-present/absent x fail-safes, with genuine-containment fixtures), R1 fresh evidence, R6 sticky boundary across --from legs, --from ordering with no prefix duplication, opt-in byte-identical behavior, no-record/thrown-failure exclusion, R4 fail-loud, and a full fresh-session replay acceptance test.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Addresses P0-P2 semantic holes + CI found in review of the repair loop: P0 (session-replay-repair-hint.ts): the scrollRegion container test was a near-universal false positive. An UNIDENTIFIED scrollRegion (RN's default ScrollView/FlatList with no testID) matched "any anonymous scrollview exists," routing unrelated screens to record-and-heal (the exact mis-binding --update retirement prevents). Now an identified region must ALSO satisfy the recorded ancestry containment (AND); an unidentified/absent region falls back to ancestry alone, walking the WHOLE recorded chain (not just ancestry[0]). P1 (session-replay-runtime.ts): the boundary watermark was captured pre-loop from the current session, but step-1 open --relaunch can REPLACE the session with a fresh actions:[] one, amputating the healed open/prefix. Reworked into a per-step armer that stamps the boundary on the session that actually accumulates the run (firstArm captures prior actions; a later arm on an unset boundary means open replaced the session → boundary 0). Added R2 preflight: a fresh full replay --save-script on an already-armed session is rejected (INVALID_ARGS) rather than silently duplicating the recorded prefix. P1 (session-replay.ts): test now rejects --save-script (replay-only), matching the sibling --from/--plan-digest guard, so buildNestedReplayFlags can't fan it across a suite run. P2 (session-script-writer.ts): the default <stem>.healed.ad is never silently clobbered — a second repair against the same original fails loud unless an explicit --save-script=<path> is given, so an unreviewed diff survives. CI (session-replay-runtime.ts): extracted the per-step verify/dispatch/guard body into resolveReplayStepResponse (runReplayScriptFile 205→163 lines) and the arming into preflight/armer helpers; refactored the branchy test mock invokes into a shared makeRecordingReplayInvoke fixture — fallow complexity is green. New/updated tests: unidentified-scrollRegion false-positive (state-repair/ manual) + labeled positive + full-chain ancestry; R6 no-amputation (open replaces) + preserved-session prior-action exclusion; R2 full-replay rejection; test --save-script rejection; P2 clobber refusal + explicit-path overwrite.
Member
Author
|
Re-review at
The focused PR suite is green (161 tests), but independent adversarial regressions for all three cases fail on this head. No fixer was dispatched per coordinator policy. |
B1 (session-replay-runtime.ts): R2 was bypassable by OMITTING --save-script — the session stays repair-armed (recordSession + saveScriptBoundary), so a plain full replay re-appended the prefix. preflightReplayAgainstActiveRepair now fires for ANY full replay (entryIndex 0) on an armed session, regardless of the flag; --from resumes stay allowed. B2 (session-replay-runtime.ts + session-action-recorder.ts): an explicit --save-script=<path> at BOTH the replay-arming and close (recordActionEntry) sites now clears saveScriptDefaultedHealedPath, so the clobber guard no longer wrongly refuses a caller-directed overwrite. Invariant: marker set iff the current saveScriptPath was defaulted. B3 (session-replay-repair-hint.ts): a root-level target with NO usable structural container signal (empty ancestry AND no identified scrollRegion) was reading as "present" on any non-empty screen -> record-and-heal on an unrelated screen. computeReplayRepairHint now fails such no-signal cases to manual (same class as the unidentified-region P0); empty ancestry + IDENTIFIED region still uses the region test. Regressions: plain full replay on an armed session -> INVALID_ARGS + zero dispatches + no prefix duplication; close --save-script=<explicit existing path> overwrites (marker cleared) while a defaulted path still fails loud; no-ancestry/no-region on an unrelated screen -> manual (selector-miss and action-failure) with the identified-region positive kept.
Member
Author
|
Re-review at Readiness is still withheld pending practical evidence through the shipped CLI path. The acceptance test uses a fresh session store but mocks device dispatch, so it does not prove the real command -> daemon -> platform recording/resume flow. Please run one simulator/device repair loop on this head that demonstrates: divergence, a recorded corrective selector action, No fixer was dispatched. External Claude review was not run because export was not separately authorized. |
|
This was referenced Jul 13, 2026
thymikee
added a commit
that referenced
this pull request
Jul 13, 2026
- C1: R7 keep-alive keys off a DISTINCT resume.repairSessionHeld signal, not resume.allowed (which means plan-resumability and fires for every divergence). - C2: define the ARMED -> COMPLETE -> COMMITTED commit state machine. close before COMPLETE = abort (publish nothing); close at COMPLETE = atomic commit; close after COMMITTED = idempotent teardown, no re-publish. No auto-commit. - C4: precise terminal-source-close contract (last source action == close) — SKIPPED (not dispatched) under armed repair so the session is not deleted; regression required (added to migration step 9). - C5a: REPAIR_SESSION_EXPIRED backed by a bounded tombstone keyed by session key (owner + expiry), cleared by a fresh replay --save-script. - C5b: atomic publication temp file in the target's own directory; race-safe no-clobber via create-exclusive/rename-if-absent. - C6: Status block corrected — Decisions 1-6 base MERGED (#1228 et al.); R7 + commit machine UNIMPLEMENTED, tracked by #1235. Migration split into step 8 (merged) and step 9 (#1235). Validation extended for all of the above.
thymikee
added a commit
that referenced
this pull request
Jul 13, 2026
- C1: R7 keep-alive keys off a DISTINCT resume.repairSessionHeld signal, not resume.allowed (which means plan-resumability and fires for every divergence). - C2: define the ARMED -> COMPLETE -> COMMITTED commit state machine. close before COMPLETE = abort (publish nothing); close at COMPLETE = atomic commit; close after COMMITTED = idempotent teardown, no re-publish. No auto-commit. - C4: precise terminal-source-close contract (last source action == close) — SKIPPED (not dispatched) under armed repair so the session is not deleted; regression required (added to migration step 9). - C5a: REPAIR_SESSION_EXPIRED backed by a bounded tombstone keyed by session key (owner + expiry), cleared by a fresh replay --save-script. - C5b: atomic publication temp file in the target's own directory; race-safe no-clobber via create-exclusive/rename-if-absent. - C6: Status block corrected — Decisions 1-6 base MERGED (#1228 et al.); R7 + commit machine UNIMPLEMENTED, tracked by #1235. Migration split into step 8 (merged) and step 9 (#1235). Validation extended for all of the above.
thymikee
added a commit
that referenced
this pull request
Jul 13, 2026
… semantics) (#1234) * docs: ADR-0012 Decision 6 — repair-transaction lifecycle (R7 + commit semantics) Frames --save-script as a multi-invocation repair TRANSACTION committed only on completion: - R7 (new normative rule): a repair-armed replay returning resume.allowed:true must keep its daemon/session live until close; heal/--from target that same session (strengthens R2). Plain close/teardown/idle-reap while armed = abort/ discard. Bounded-expiry must surface REPAIR_SESSION_EXPIRED, not bare SESSION_NOT_FOUND. Persistent-daemon precondition rejected: fail-fast before step 1, never a later SESSION_NOT_FOUND. - Decision 4 resume: one sentence noting the session is kept addressable so resume.allowed:true is not misleading. - Commit semantics: healed .ad committed only on full-plan completion or explicit close --save-script; never on divergence-only exit, teardown, or idle-reap; atomic temp->publish. R6 defines the slice, this defines when it is complete. - Terminal lifecycle steps: non-target steps (incl. source close, unannotated steps) are already exempt from target-binding divergence per decision 3 (clarification, not a change); prefer SKIPPING the source terminal close while armed and finalize via close --save-script. - Clobber P2: no-clobber guards a COMPLETE (heal-complete sentinel) artifact only; partials are overwritable; auto-versioned names out of scope. Validation + migration step 8 extended accordingly. * docs: ADR-0012 R7 — fix 6 architecture blockers (transaction contract) - C1: R7 keep-alive keys off a DISTINCT resume.repairSessionHeld signal, not resume.allowed (which means plan-resumability and fires for every divergence). - C2: define the ARMED -> COMPLETE -> COMMITTED commit state machine. close before COMPLETE = abort (publish nothing); close at COMPLETE = atomic commit; close after COMMITTED = idempotent teardown, no re-publish. No auto-commit. - C4: precise terminal-source-close contract (last source action == close) — SKIPPED (not dispatched) under armed repair so the session is not deleted; regression required (added to migration step 9). - C5a: REPAIR_SESSION_EXPIRED backed by a bounded tombstone keyed by session key (owner + expiry), cleared by a fresh replay --save-script. - C5b: atomic publication temp file in the target's own directory; race-safe no-clobber via create-exclusive/rename-if-absent. - C6: Status block corrected — Decisions 1-6 base MERGED (#1228 et al.); R7 + commit machine UNIMPLEMENTED, tracked by #1235. Migration split into step 8 (merged) and step 9 (#1235). Validation extended for all of the above. * docs: ADR-0012 R7 — teardown-commits model + persisted-state continuation Resolves the two contract ambiguities blocking merge (aligns with #1235): - Completion model = TEARDOWN-COMMITS (not explicit-close-only). A repair-armed session stays addressable until the transaction ends; ANY teardown (explicit close, idle-reap, daemon shutdown) commits the healed .ad atomically iff the transaction is COMPLETE, else aborts with no publish (never a prefix). An incomplete reap/shutdown leaves the REPAIR_SESSION_EXPIRED tombstone; an explicit close of an incomplete tx just discards. Kept the ARMED->COMPLETE->COMMITTED machine and idempotent post-COMMITTED teardown; removed the "no auto-commit / commit only on explicit close" language. - Continuation by PERSISTED transaction state, not the per-request flag: replay --from <n> --plan-digest <sha> resumes on the persisted repair-armed session WITHOUT repeating --save-script; --save-script appears only on the transaction opener. Implementation MUST key keep-alive/continuation off persisted state. Decision 4 repairSessionHeld updated to match. Edited: R7, Decision 4 resume signal, terminal-close, Emitting, commit state machine, tombstone, migration step 9, and validation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements ADR-0012 Decision 6 ("heal-by-doing"): the agent-supervised re-record repair loop for
replaydivergences. All net-new implementation per the ADR's migration step 8 scope; no new splice engine — the healed script ISsession.actions, reusing theclose --save-scriptwriter.Workstream -> Rule -> Implementation map
replay --save-script[=<out>]arm + watermarksrc/daemon/handlers/session-replay-runtime.ts—armReplaySaveScript(called at the top of every loop iteration; no-op until step 1 creates the session for a fresh one) setssession.recordSession = trueandsession.saveScriptBoundaryonce. Default<out>(healedScriptSiblingPath) is<original-stem>.healed.ad, an explicit<out>always wins. Absent--save-script, replay is unchanged (opt-in).repairHintcomputation + wiresrc/daemon/handlers/session-replay-repair-hint.ts(new) —computeReplayRepairHint, a total mapping over the 4 divergence kinds with two fail-safes tomanual. The container-presence test is genuine ancestor containment walked overparentIndex(buildAncestryChain/computeScrollRegionKey, reusing decision 3's own tree-walking machinery insession-target-evidence.ts) against the daemon's full capture — never the cappedscreen.refswire projection. Wired into both divergence-minting sites (session-replay-divergence.tsforaction-failure's post-response capture,session-replay-target-verification.tsfor the 3 target-binding kinds' pre-action capture) and threaded throughsrc/replay/divergence.ts's type, minimal-fallback, and text-report renderer so it survives every response level (including--level digest) and all 4 projections (daemon text, JSON, Node clientAppError, MCPstructuredContent).@refsrc/daemon/session-script-writer.ts—assertNoUnresolvedRefFallbackthrowsAppErrorwhen a selector-targeting action's ref never resolved to aselectorChain;write()'s catch rethrowsAppErrorinstead of swallowing it. Scoped to a repair-armed session (saveScriptBoundary !== undefined) — R4's own text frames it as "corrective actions must materialize to selector form" and explicitly preserves "existing session-write paths swallow such failures" for ordinaryopen/close --save-script, which keeps its existing refLabel/scoped-snapshot fallback unchanged.src/daemon/session-script-writer.ts—buildOptimizedActionsslicessession.actionsfromsaveScriptBoundary ?? 0; the default output path is computed at arm time in workstream 1.src/cli/parser/cli-help.ts— one added sentence documenting the tworepairHint-routed sub-flows and theclose --save-script/<stem>.healed.adpromotion step, appended to the existing "Replay divergence and resume" paragraph.Tests
session-replay-repair-hint.test.ts— therepairHintmapping: all 4 kinds x container-present/absent x both fail-safes (no evidence, unavailable capture), including fixtures that specifically distinguish genuine ancestor containment from a flat identity-string match (a decoy node sharing the container's role/label but with zero children is correctly "absent").session-replay-repair-loop.test.ts— R1 (prefix steps get fresh evidence, never the.ad's old annotation), R2 (--fromordering, no prefix duplication), R6 (boundary is sticky across a redundant--save-scripton a second--fromleg — mutation-tested to confirm the assertion actually depends on the guard), opt-in (no--save-script-> no arming/evidence), a thrown dispatch failure never records a partial action, and--no-recordstate-fix exclusion.session-replay-repair-acceptance.test.ts— the mandatory acceptance test: full repair loop (diverge -> corrective press -> resume -> write) then replays the healed.adend-to-end in a completely freshSessionStore/state dir, asserting a self-containedopen(relaunch + platform + metro flags travel with it) and no bare@refanywhere.session-script-writer.test.ts(new) — boundary slicing, snapshot stripping post-slice, R4 fail-loud (resolved ref -> clean line; unresolved -> throws, nothing written), and confirms an ordinary (non-repair-armed) recording keeps its existing bare-ref fallback unchanged.divergence.test.ts,command-tools.test.ts(MCP), andoutput.test.ts(CLI) withrepairHintpresence assertions across the minimal-overflow fallback and the text/structuredContent projections.Verification
npx vitest run src/replay src/daemon src/commands/replay src/mcp src/utils src/cli src/client— all green.npx vitest run src/daemonsweep — 138 files / 1172 tests green.--project unit-corerun — 3787/3789 green; the 2 failures are insrc/platforms/apple/core/__tests__/index.test.ts(unrelated iOS IPA install tests, untouched by this diff) and pass in isolation — pre-existing CPU-contention flakiness, not a regression.npx tsc --noEmit— clean.oxlint . --deny-warnings— clean.oxfmt --check— clean.Notes
No ADR-vs-code conflicts found. One real design nuance surfaced and resolved: a literal, unscoped reading of R4 would have broken the existing refLabel/scoped-snapshot fallback for ordinary (non-repair)
--save-scriptrecordings — 5 pre-existing tests insession-store.test.tsexercise exactly that fallback. R4's own text ("corrective actions," "existing session-write paths swallow such failures... this one must not") scopes the fail-loud guard to repair-armed sessions, so I gated onsaveScriptBoundary !== undefinedrather than changingSessionScriptWriter's general behavior — all 5 pre-existing tests pass unmodified.