fix(replay): default-exclude observation-only reads from repair heals, add --record opt-in (#1271 stage 2)

[thymikee]

Summary

Stage 2 of #1271 (stage 1: #1287, shipped interim --no-record guidance). This is the semantic fix the maintainer's triage gated behind an ADR-0012 amendment: default-exclude observation-only commands from a repair-armed heal, with an explicit --record opt-in for the one case where a blanket exclusion would be unsafe.

The trap this resolves (and why a blanket rule is wrong)

"Exclude every read from a repair segment" is not safe on its own: a divergence's correction can itself be a read. In the wave-3 E3 drift, the diverged step was a get, so its corrective replacement is a recorded get that must land in the heal. A blanket read-exclusion would silently drop the very step being repaired — producing an incomplete heal with no signal anything was missing.

The resolution: default-exclude snapshot/get/is/a read-only find only inside a repair-armed session (session.saveScriptBoundary set by a replay --save-script, never by ordinary open --save-script authoring recording), and add a new --record flag that forces one specific action through when its own correction is itself a read. wait is never excluded (flow timing, not observation). --record/--no-record are mutually exclusive (INVALID_ARGS if both set).

Where it lives

Single daemon-side choke point: isExcludedRepairSegmentObservation in recordActionEntry (src/daemon/session-action-recorder.ts), reached by every surface (CLI/Node client/MCP) since they all converge on the same daemon request. Because the exclusion happens at record time — before an action reaches session.actions — an excluded read never grows session.actions.length, which is exactly the counter the existing record-and-heal resume watermark (describeUnperformedRecordAndHeal, from #1262) already checks to prove a corrective action happened. So the empty-segment fail-loud guard falls out with zero new state — only its error message changed, to also name --record alongside the existing --no-record mention.

--record is plumbed identically to how --no-record is plumbed today: contracts/cli-flags.ts, cli-grammar/flag-definitions-action.ts, cli-grammar/flag-groups.ts, commands/command-flags.ts, client/client-types.ts, commands/management/app.ts, plus the interaction/snapshot command metadata so it's exposed on the MCP tool schemas for get/is/find/snapshot.

A bug found along the way

Tracing --no-record's existing plumbing turned up a latent gap: the get/is/find/snapshot CLI readers never actually forwarded --no-record/--record into the built request — only open's reader did. So stage 1's shipped guidance ("use --no-record on those commands") was silently inert when followed via the CLI. Fixed as part of this change (the 4 readers these commands use now forward both flags); a similar gap likely exists for press/fill/click/etc.'s CLI readers, left out of scope here since it's unrelated to the observation-only exclusion — worth a separate follow-up.

ADR

docs/adr/0012-interactive-replay.md gets a new amendment under decision 6 covering: which commands are excluded, the corrective-read trap and why position-based detection was rejected in favor of the explicit --record opt-in, how the rule projects across CLI/Node/MCP, and why the fail-loud guard needed no new state. Also fixed some pre-existing staleness in the same doc (R7/#1235 was marked "not yet implemented" despite being merged).

Test plan

• tsc -p tsconfig.json --noEmit — clean
• oxlint . --deny-warnings — clean
• format:check (oxfmt) — clean
• pnpm check:fallow --base origin/main — no issues in 28 changed files
• pnpm test:integration:progress:check — passes (record classified and covered: 2 provider-scenario references; 0 unclassified flags, 0 missing flag coverage)
• vitest run --project provider-integration — 34/34 files, 127/127 tests (run serially; the suite is flaky under parallel contention on a loaded dev box, baseline included)
• Full vitest run --project unit-core — 4058/4061 passing; the 3 failures are known-flaky process-spawn tests unrelated to this change (confirmed passing in isolation)
• New regression coverage:
  • src/daemon/__tests__/session-action-recorder.test.ts — the core exclusion mechanism
  • src/daemon/__tests__/selector-recording.test.ts — get/is/find classified observation-only, wait is not
  • src/daemon/__tests__/request-router-record-flags.test.ts — --record+--no-record mutual exclusivity
  • src/daemon/handlers/__tests__/session-replay-repair-record-exclusion.test.ts — end-to-end: a diagnostic read is omitted by default; a mutating corrective press remains; a --recorded corrective read remains (the diverged-step-was-a-get case) and the healed script replays cleanly in a fresh session; the empty-segment guard refuses with the actionable --record hint; non-repair authoring recording is unchanged
  • src/__tests__/cli-grammar.test.ts — proves the CLI-reader forwarding fix
  • src/mcp/__tests__/command-tools.test.ts — record/noRecord exposed on the MCP tool schemas and forwarded through
  • src/replay/__tests__/divergence.test.ts — updated repair-hint guidance text
  • test/integration/provider-scenarios/replay-repair-record-exclusion.test.ts — provider-backed end-to-end (see below)

Provider-backed integration coverage (--record)

The integration progress ratchet (test:integration:progress:check) flags any public CLI flag that is neither classified nor covered, so --record needed real provider-backed coverage — the ratchet deliberately does not count mock-heavy handler tests, which is the point of the check. (The exclusions bucket is scoped to config/output/transport flags; putting a behavior flag there would dodge the ratchet, not satisfy it.)

Added test/integration/provider-scenarios/replay-repair-record-exclusion.test.ts, a focused scenario alongside the existing --no-record precedent in android-lifecycle.test.ts. It drives the real request router, session store, replay runtime, and script writer — only the ADB provider is faked — and proves the flag's actual purpose end-to-end:

1. replay --save-script arms a repair transaction and diverges (REPLAY_DIVERGENCE, resume.repairSessionHeld: true).
2. Inside that repair segment the same get text <selector> runs twice, differing only in --record.
3. Resume past the diverged step → transaction COMPLETE → close --save-script commits the healed .ad.
4. The committed heal contains exactly one get line — the --recorded one. The identical diagnostic read that ran first is absent.
5. --record + --no-record together → INVALID_ARGS.

Step 4 is the whole amendment in one assertion, and it's also why a blanket read-exclusion would be unsafe: it would drop that line too, silently.

Verified the scenario reproduces the bug rather than passing vacuously: with isExcludedRepairSegmentObservation neutered to return false, it fails on a diagnostic read inside a repair segment must not be recorded.

Closes #1271 (pending review).

---

Review round 2 — provenance rule (P1 fix), --record scoping, rebase onto #1305

Base is now #1305 (fix/cli-readers-forward-no-record); merge that first.

P1: the exclusion dropped PLANNED reads from the heal

The maintainer was right, and it reproduced. Excluding by command class was wrong — the real discriminator is provenance. Replayed plan steps dispatch through the ordinary request path, so an authored get/is/find step hit the same recording path as an interactive read and never reached session.actions — and the heal is session.actions.slice(saveScriptBoundary). Result: a repaired flow replayed its authored is visible assertion and then silently dropped it from its own healed script.

Reproduced with the scenario's own fixture (authored step 3 = is visible), resumed.replayed === 1 — it executed — and the committed heal was:

context platform=android device="Pixel 8" kind=emulator theme=unknown
# agent-device:target-v1 {...}
get "text" "id=\"com.android.settings:id/search\" || ..."
close
# agent-device:heal-complete

No is line. The heal quietly stopped checking what the original checked.

Fix — an explicit provenance marker, not a heuristic. internal.replayPlanStep is stamped by invokeResolvedReplayAction (session-replay-action-runtime.ts), the single point every plan step is dispatched, so it covers annotated and unannotated steps alike. It's trustworthy because internal is daemon-only — toDaemonRequest (server/http-server.ts) never copies it off the wire, so authored provenance can't be spoofed; same channel replayTargetGuard/findResolvedTarget already use. The rule lives once in isInteractiveObservation, and both recording call sites consume it — the mock fixture now calls the production classifier rather than mirroring it, so it can't drift.

Planned prefix/suffix observations survive automatically; users never annotate their own .ad steps.

--record is no longer a common flag

• Removed from COMMON_COMMAND_SUPPORTED_FLAG_KEYS; statically scoped via allowedFlags to snapshot/get/is.
• find validates dynamically (its observe-vs-mutate split is a positional the grammar can't see): read-only allows; a mutating find … click|fill|focus|type is INVALID_ARGS before any device work. Both halves share one isReadOnlyFindAction predicate (src/selectors/find.ts), so routing and validation cannot disagree.
• --no-record stays shared — it applies to every recordable command.
• Removed from open (never observation-only).

Projection: rebased onto #1305, helper split not broadened

Dropped the four hand-rolled noRecord/record reader blocks. Chose two named helpers over one allowRecord policy argument:

• noRecordInputFromFlags — all 13 recordable readers (renamed from recordControlInputFromFlags; @coordinator this touches your PR's helper name).
• observationRecordInputFromFlags — snapshot/get/is/find only.

Rationale: the capability becomes the helper's name, so a mutating reader physically cannot forward --record. A boolean policy arg would let a future mutating reader opt in by flipping a literal with no schema change — the exact fail-open the split exists to prevent.

Bar

• The scenario fails without the provenance fix: the authored 'is visible' step must survive the heal (0 !== 1). Verified by reverting just the replayPlanStep check.
• ADR-0012 decision 6 rewritten: rule 1 is now the provenance rule (command-class + out-of-band, ANDed), with the P1 spelled out as the reason.

Gates

tsc · oxlint --deny-warnings · format:check · fallow (exit 0) · test:integration:progress:check (0 unclassified, 0 missing) · unit-core targeted 118 files/1421 tests · provider-integration 34/34 serial.

Ambient note: on a loaded dev box both suites flake with 5s real-time timeouts (baseline included, in files this PR doesn't touch — runner-client, runtime-hints, package-exports, …). Run serially they pass with zero AssertionErrors.

One flag for @coordinator: fallow reports a 7-line clone group at interactions.ts:38-44 vs 48-54 (the click/press readers, identical once both spread the shared helper). That's introduced by #1305, not this PR, and it's a warn (fallow exits 0) — leaving it to you rather than editing your PR's surface.

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	1.8 MB	1.8 MB	+2.1 kB
JS gzip	566.4 kB	567.2 kB	+768 B
npm tarball	680.7 kB	681.8 kB	+1.1 kB
npm unpacked	2.4 MB	2.4 MB	+2.8 kB

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	23.7 ms	22.9 ms	-0.7 ms
CLI --help	50.9 ms	47.3 ms	-3.7 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/registry.js	+927 B	+264 B
dist/src/cli-help.js	+252 B	+87 B
dist/src/internal/daemon.js	+159 B	+86 B
dist/src/session.js	+124 B	+85 B
dist/src/snapshot-diagnostics.js	+254 B	+83 B

[thymikee]

Current-head review (5f194a67e): two blockers.

1. P1: preserve replay-originated observation steps; exclude only out-of-band repair diagnostics. isExcludedRepairSegmentObservation() keys only on observationOnly plus saveScriptBoundary, but the boundary is armed during source replay. Legitimate snapshot/get/is/read-only find steps from the original plan are therefore silently dropped before the divergence and in the resumed suffix, producing incomplete heals. The provider regression masks this: its source has a snapshot prefix and get suffix, never asserts prefix preservation, and the manual --record get uses the same selector, so “exactly one get” passes when the planned suffix get was dropped and replaced. Carry replay-vs-interactive provenance (or force planned replay reads through) and add distinct prefix/suffix read assertions proving both survive while only an out-of-band diagnostic is omitted.

2. P2: enforce --record/--no-record conflict before command readers discard them. Both are syntactically common, but mutating readers such as press/click/fill forward neither, so press --record --no-record ... loses both before the daemon conflict check and executes. Validate raw flags before projection or include them in shared CLI input; add a non-observation CLI regression.

This draft is not ready despite the otherwise coherent daemon/MCP/Node plumbing.

[thymikee]

Two things from working #1304, which overlaps this PR's reader plumbing.

1. A design question this PR should settle: --record is accepted where it can never do anything.

record is in COMMON_COMMAND_SUPPORTED_FLAG_KEYS, so agent-device press @e5 --record parses fine. But the exclusion it opts out of is gated on observationOnly:

function isExcludedRepairSegmentObservation(session, entry) {
  if (!entry.observationOnly) return false;   // press/fill/click stop here, always
  ...
  return entry.flags?.record !== true;
}

and this PR's own docs confirm the consequence — contracts/cli-flags.ts: "a no-op outside a repair-armed session and on any non-observation-only command", plus RecordControlOptions is mixed into GetOptions/Is*Options and deliberately not into the press/fill option types.

So --record on a mutating command is accepted and silently inert. That's the same shape as the #1304 bug (accepted-then-ignored), one layer over. Scoping record to the observation-capable commands instead of COMMON_* would make press --record fail loudly, which matches the fail-loud convention elsewhere. Not something I wanted to decide unilaterally inside your ADR-gated PR — flagging it for you.

Note it can't be a complete grammar fix: find is one command with both read-only and mutating sub-actions, so find … --record --click stays inert-but-accepted regardless. That's a runtime nuance, not an argument against scoping the other eight.

2. #1305 will make this PR smaller.

#1305 fixes the underlying --no-record drop, which turned out to affect all 13 recordable readers (0/13 forwarded it; measured through parseArgs → readInputFromCli), not just the interaction ones. It routes the flag through one recordControlInputFromFlags() helper next to settleInputFromFlags, rather than per-reader copies.

After #1305 lands, this PR should rebase and drop its four hand-rolled noRecord: flags.noRecord, record: flags.record blocks (interactions.ts get, selectors.ts find/is, snapshot.ts) plus their repeated comments, and instead add record to the shared helper. Small conflict, net negative diff here.

[thymikee]

P1 reproduced: the exclusion drops planned reads from the heal

Following up on the "planned vs out-of-band" point — it is not just a design gap, it is live in this PR, and this PR's own scenario passes while it happens.

Mechanism. isExcludedRepairSegmentObservation discriminates by command class (observationOnly), but the real discriminator is provenance:

• Replayed plan steps dispatch through the normal request path — invokeReplayAction → buildReplayActionFlags, which merges parent+recorded flags and never sets noRecord.
• So they reach recordIfSession → observationOnly: true → the new exclusion → they never enter session.actions.
• buildOptimizedActions builds the heal from session.actions.slice(saveScriptBoundary).

Net: an authored get/is/find step in a repaired flow is replayed and then silently dropped from its own healed script.

Reproduction (this PR's scenario, one line changed). Fixture step 3 get text ${SEARCH_SELECTOR} → is visible ${SEARCH_SELECTOR} — a distinguishable command, so there is no ambiguity about which line is which — then print healedScript:

context platform=android device="Pixel 8" kind=emulator theme=unknown
# agent-device:target-v1 {...}
get "text" "id=\"com.android.settings:id/search\" || ..."
close
# agent-device:heal-complete

resumed.replayed === 1 — the planned is visible did execute — and there is no is line in the heal. The healed flow silently lost an authored assertion.

For the 10×-known-scenario-QA-replay use case this is the worst-shaped failure: the heal keeps replaying clean while quietly no longer checking what it used to check. Note also that the scenario passes throughout, because it only counts get lines — so this is a test-fidelity gap, not merely an uncovered case.

Fix direction (per maintainer): exclusion applies only to out-of-band interactive observations. Planned prefix/suffix observations survive automatically — users must never annotate authored replay steps with --record. --record is only for an interactive corrective read intentionally inserted into the heal. Gate on an explicit provenance marker rather than a heuristic — the same reasoning observationOnly's own doc already gives for not sniffing command.

Also queued on this PR:

1. --record leaves COMMON_COMMAND_SUPPORTED_FLAG_KEYS: scoped statically to snapshot/get/is; find validated dynamically (read-only action allows it, mutating find … click|fill|focus|type rejects as INVALID_ARGS). --no-record stays shared — it applies to every recordable command, mutations included.
2. Rebase onto fix(cli): forward --no-record from every recordable command reader #1305 and drop the four hand-rolled noRecord/record blocks — but split the projection (noRecordInputFromFlags() for every recordable reader, observationRecordInputFromFlags() only for observation-capable ones, or one helper with an explicit allowRecord policy) rather than broadening the shared helper across all 13 readers.
3. The ADR-0012 decision-6 amendment text must state the provenance rule, not a command-class rule.

Worker is on it.

[thymikee]

Exact-head re-review (de65ecb05): the provenance fix, flag scoping/conflict handling, #1305 integration, and suffix-read regression are sound. One P1 serialization blocker remains.

buildOptimizedActions() still unconditionally drops every snapshot in session-script-writer.ts. Therefore the provider scenario’s authored prefix snapshot -i does not survive into the heal, despite the amended ADR promising planned prefix/suffix observations survive. The same bug makes interactive snapshot --record accepted across surfaces and admitted to session.actions, only to be silently discarded at publication. The test masks both by asserting only the suffix is visible line.

Either preserve authored and explicit---record snapshot actions through serialization and assert distinct prefix snapshot + suffix read in the final artifact/fresh replay, or remove snapshot from the opt-in/survival contract and use a preservable prefix read (get/is/read-only find) with explicit prefix+suffix artifact assertions. All checks are green, but this draft is not ready.

[thymikee]

P3 closed: internal provenance is enforced, not conventional — plus rebase onto merged #1305

The P3 finding was better than expected. The concern was that internal might be accepted from socket/MCP ingress, letting a caller stamp replayPlanStep: true. It cannot, and the guarantee is structural and doubled, not a convention:

1. commandRpcParamsSchema (kernel/contracts.ts) is an allowlist projection — it builds {token, session, command, positionals, input, flags, runtime, meta} field by field. Unknown keys can't survive parsing; internal is not expressible in CommandRpcParams at all.
2. toDaemonRequest (server/http-server.ts) then builds the request field by field from that projection.

Checked every other ingress too: the four remaining DaemonRequest builders in http-server.ts don't touch internal, and no file under daemon/server, mcp, or client references req.internal (the one 'internal' hit in client-companion-tunnel.ts is a directory name in a path.join).

Still worth pinning, exactly as asked, since nothing stopped a future edit from opening it: 93a1cd80 posts a real JSON-RPC request carrying internal: { replayPlanStep: true } through a loopback server and asserts the dispatched request has no internal.

It's a genuine guard rather than a restatement: with only toDaemonRequest regressed the test still passes (the schema already dropped the field) — it fails with a wire-supplied internal must never reach the daemon request only when both allowlists are regressed. That's the composite contract, which is the honest thing to pin given the defense-in-depth.

Rebased onto merged #1305

#1305 is merged (c245906b7). Rebased with --onto to drop its now-squashed commit. The only conflict was not in cli-grammar/common.ts as predicted — it was the ADR amendment list, where main had gained the decision-3 press-retarget bullet (#1280); both bullets are independent and both kept.

Post-rebase verification (not inherited from the pre-rebase run):

• helper composition landed clean — noRecordInputFromFlags + observationRecordInputFromFlags, zero references to fix(cli): forward --no-record from every recordable command reader #1305's old recordControlInputFromFlags name
• tsc --noEmit, oxlint --deny-warnings, format:check clean (pnpm install needed for main's new @limrun/api dep)
• the P1 scenario and the new ingress test pass
• test:integration:progress:check exit 0 — 0 unclassified flags, 0 missing coverage

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-16 18:31 UTC

[thymikee]

Exact-head re-review (de65ecb05): the provenance fix, flag scoping/conflict handling, #1305 integration, and suffix-read regression are sound. One P1 serialization blocker remains.

buildOptimizedActions() still unconditionally drops every snapshot in session-script-writer.ts. Therefore the provider scenario’s authored prefix snapshot -i does not survive into the heal, despite the amended ADR promising planned prefix/suffix observations survive. The same bug makes interactive snapshot --record accepted across surfaces and admitted to session.actions, only to be silently discarded at publication. The test masks both by asserting only the suffix is visible line.

Either preserve authored and explicit---record snapshot actions through serialization and assert distinct prefix snapshot + suffix read in the final artifact/fresh replay, or remove snapshot from the opt-in/survival contract and use a preservable prefix read (get/is/read-only find) with explicit prefix+suffix artifact assertions. All checks are green, but this draft is not ready.