diff --git a/docs/adr/0012-interactive-replay.md b/docs/adr/0012-interactive-replay.md index 0350d8a1b2..8f3e55b3b3 100644 --- a/docs/adr/0012-interactive-replay.md +++ b/docs/adr/0012-interactive-replay.md @@ -2,207 +2,52 @@ ## Status -Accepted (2026-07-10); partially implemented (last updated 2026-07-16). See [Migration progress](#migration-progress) for the per-step landing record. - -**Implemented and merged to `main`:** - -- Decisions 1-5 and their migration steps 1-7 — resolution disclosure (#1193), structured divergence - transport (#1197), `.ad` target annotations and target-binding verification (#1196, #1209), `replay - --from`/`--plan-digest` resume and `--update` retirement (#1211), and the selector-miss → - `REPLAY_DIVERGENCE` repair-loop fix (#1223). -- Decision 6, the base agent-supervised re-record repair — `replay --save-script` arming, the - post-watermark healed slice, `repairHint`, and the writer's bare-`@ref` fail-loud guard (#1228). -- Decision 6, R7 + commit state machine (repair-transaction lifecycle) — the ARMED → COMPLETE → COMMITTED - commit state machine, keep-alive keyed off the persisted transaction (`resume.repairSessionHeld`, never - the per-request `--save-script` flag), terminal-source-`close` skipping, the `REPAIR_SESSION_EXPIRED` - tombstone, and race-safe atomic publication via exclusive `linkSync` (#1235). -- Decision 4 amendment, `screen`'s capture scope and ref selection — the divergence `screen` capture runs - through the same `captureSnapshot` wrapper as plain `snapshot` (full-window scope + Android freshness / - post-action retry parity) under a clean, fixed capture-flags policy (a failed raw/scoped/`-d` action can - no longer narrow the diagnostic tree), with the chrome and meaningful-target filters layered on top as - filters, not scopings; and `screen.refs` is ranked within the byte cap (foreign-window dismiss targets - ahead of app content; mass-covered app nodes surfaced rather than emptied) instead of sliced in document - order, so a captured overlay is never buried past the cap (#1264). -- Decision 3 amendment, id demotion under non-unique capture-time match — a recorded id no longer serves - as identity, or leads the selector chain, unless it is unique in the record-time tree; a non-unique id - (a shared Android framework resource id such as `android:id/title`, or a reused RN `FlatList` `testID`) - falls back to role+label in both `computeTargetEvidence`'s `target-v1` tuple and - `buildSelectorChainForNode`'s chain (#1269). -- Decision 3 amendment, record-time press retarget to a labeled descendant — a click/press whose - resolved winner is an identity-empty container (no surviving id, no label, no value, from the demoted - identity view) is recorded against its first labeled descendant via a recording-only side channel, so - both writers key off a node with selective identity while the live response stays container-based - end-to-end; a container whose subtree holds a competing interactive node (canonical classification or - hittable flag), or whose selected descendant's center falls outside the container rect, is recorded - unchanged; `fill` is excluded (#1280). -- Decision 6 amendment, repair-segment default exclusion of observation-only commands and the `--record` - opt-in — stage 1 (interim guidance only, #1287) and stage 2 (the semantic default-exclusion fix, - this amendment) of #1271. See the amendment under decision 6 below. -- Decision 3 amendment (#1349), read-only step identity — landmark-mode `target-v1` evidence and - post-resolution (in-loop) verification for selector `wait`, `get`-pattern coverage for `is` (except - `exists`), the `targetIdentityVerification` descriptor trait pinning the evidence-carrying command - set, explicit deferral of read-only `find`/`is exists`/non-selector waits, and ADR 0016's destination - guard strengthened to require a `verified` landmark annotation, with a provider-level - reshuffled-screen false-pass regression. See the amendment under decision 3 below. - -**Accepted but NOT yet implemented** (this amendment; tracked by #1235 — repair-transaction lifecycle): -the R7 repair-transaction keep-alive and its distinct `resume.repairSessionHeld` signal, the ARMED → -COMPLETE → COMMITTED commit state machine, terminal-source-`close` skipping, the `REPAIR_SESSION_EXPIRED` -tombstone, and race-safe atomic publication. Until #1235 lands, `--save-script` on a diverging replay -does not carry the lifetime, abort-on-incomplete, or atomic-commit guarantees R7 specifies. - -## Context (historical baseline) - -This section records the repository state audited on 2026-07-10, before the migration steps recorded -below shipped. Its present-tense observations are historical evidence; -[Migration progress](#migration-progress) is the authoritative record of current behavior. - -At acceptance, replay was deterministic. `.ad` scripts were plain text — one action per line, `#` -comments, and a `context platform=... device=... theme=...` header (`src/replay/script.ts`) — recorded -via `open --save-script` (`src/daemon/session-action-recorder.ts`, -`src/daemon/session-script-writer.ts`) or hand-written, and executed step-by-step by -`runReplayScriptFile` -(`src/daemon/handlers/session-replay-runtime.ts`) under the daemon's `replay`/`test` commands -(`src/daemon/handlers/session-replay.ts`). Recorded touch/fill/get targets are selector chains with -`||` alternates (`buildSelectorChainForNode(...).join(' || ')`, -`src/commands/interaction/runtime/resolution.ts:242`, mirrored in -`src/daemon/handlers/session-replay-heal.ts:131-135`); Maestro YAML flows import through `--maestro` -(`src/compat/maestro/`); progress is step-indexed (`stepIndex`/`stepTotal` in -`emitReplayTestActionProgress`, `session-replay-runtime.ts:243-260`). - -Recovery was opt-in `--update`/`-u` healing (`replayUpdate` flag, -`src/commands/cli-grammar/flag-definitions-workflow.ts`). It only fires after a step has already -returned a hard failure (`session-replay-runtime.ts:118-149`: `if (!shouldUpdate) return failure; ... -healReplayAction(...)`), and it only retries the SAME recorded selector material — -`collectReplaySelectorCandidates` (`session-replay-heal.ts:39-81`) gathers the step's originally -recorded `selectorChain`/positionals, then `resolveSelectorChain` re-resolves those exact candidate -strings against a freshly captured snapshot (`session-replay-heal.ts:122-135`). If the identifying term -itself changed — an id or label rename — the same string will not match the new tree either, so heal -cannot rescue renames; it can only recover drift the ORIGINAL selector still matches (a moved or -re-rendered node with the same id). PR #297 (closing #279) already trimmed heal once, removing -`refLabel`-synthesis and numeric `get text` drift healing to keep it "centered on recorded selectors and -explicit selector expressions" — heal has a maintained history of narrowing, not growing. - -**Benchmark evidence** (2026-07-09/10, iOS simulator, react-navigation/RN playground matrix; harness -follows the `~/.agent-device-bench/rnnav-matrix.py` pattern, external — the key numbers are recorded -here so the evidence stays durable without the harness directory): - -| Measurement | Result | -| --- | --- | -| Snapshot captures per interaction, `--settle` off → on | 3.67 → 1.00 (the 1-snapshot floor) | -| Commands per task, settled arm vs unsettled arms | 14.3 vs 23.3 / 26.7 | -| react-navigation Maestro suite via deterministic replay | 38/38 flows green in 539 s, zero model turns | - -With the settle loop at its snapshot floor, wall time for an agent-driven QA flow is dominated by model -turn latency, not device I/O. A happy-path agent-driven QA flow costs O(steps) model turns end-to-end; a -deterministic replay of the same flow costs O(divergences) — the 38/38 sweep is that limit realized at -zero divergences. The entire economic case for replay is collapsing the per-step model-turn cost toward -zero on the happy path and paying only where reality diverged from the recording. - -**Audit evidence** (2026-07-10) on where that divergence cost actually goes: - -- **(a) Heal is narrow and mostly unable to act.** Per the mechanism above, heal only recovers - same-selector drift. Most real replay failures are renames or removals heal's candidate-recycling - cannot reach. -- **(b) The real mis-binding surface is not heal — it is silent disambiguation in ORDINARY resolution**, - live and replay alike. `resolveSelectorInteractionTarget` calls `resolveSelectorChain(..., { - disambiguateAmbiguous: true })` on every press/click/fill (`resolution.ts:170-183`); when a selector - matches N>1 nodes, `accumulateDisambiguationCandidate`/`compareDisambiguationCandidates` - (`src/selectors/resolve.ts:181-285`) silently pick a winner — visible candidates over - off-screen ones, then deepest node, then smallest on-screen area, only an exact tie failing. - `describeResolvedInteractionNode` (`resolution.ts:227-249`), the response's entire identity payload, - carries `node`/`selectorChain`/`refLabel`/`targetHittable`/`hint` — no match count, no signal a - tiebreak happened at all. This was live-reproduced during the audit on an RN playground screen with - two identical-rect "Prevent Remove" buttons, where scroll position alone decided which one a selector - hit. The general policy is documented (`agent-device help workflow`, - `src/cli/parser/cli-help.ts:243,384`: "does not fail by default ... auto-resolves deepest node first - ... then smallest on-screen area") but never disclosed per response — an agent that hasn't read the - help topic, or whose target moved between recording and replay, gets no signal a heuristic rather than - an exact match chose its target. -- **(c) No target-binding verification existed in this path at acceptance.** `--verify` - (`captureEvidenceBaseline`, `resolution.ts:45-58,104-134`; the `verifyEvidence` guarantee cell in ADR - 0011's registry) attaches a pre/post-action node diff so the caller can see SOMETHING changed — it - says nothing about whether the CORRECT node was the one tapped. A wrong-but-plausible pick (the - sibling "Prevent Remove" button) produces a real, visible diff and is still the wrong action. -- **(d) Heal auditability was a bare count.** A successful `--update` run returned - `{ replayed, healed, ... }` (`session-replay-runtime.ts:186-195`) — `healed` is a number, nothing - else — and rewrote the `.ad` file in place via `writeReplayScript` - (`session-replay-runtime.ts:182-184`, `src/replay/script.ts:459-484`) with no diff shown anywhere in - the response. -- **(e) This silent-pick default is in real tension with this repo's general posture toward ambiguity.** - Elsewhere, ambiguous input is refused and hinted about rather than silently guessed — `start`/`restart` - are deliberately left out of the CLI alias-suggestion table because `start` is "genuinely ambiguous, so - a hint beats silently guessing" (`src/cli/parser/command-suggestions.ts:16-17`). Selector resolution - took the opposite default, and ADR 0011's own registry records that choice precisely: the - `disambiguation` cell for `runtime-selector` is classified `{ kind: 'runtime', via: - '...selectors-resolve.ts#resolveSelectorChain' }` (`src/contracts/interaction-guarantees.ts:176-179`) - — proving the heuristic runs consistently across paths, not that the caller is told it ran. That - default is not being revisited here; see the rejected hard-reject alternative below for why. -- **(f) Issue #1037 / PR #1040 is the direct, partial precedent.** A UNIQUE-but-wrong match (Apple - Maps' `text="Anthropic - Headquarters"` exact-matching a 30x30 map-pin annotation instead of the - recents row) now surfaces as `targetHittable:false` plus a hint - (`describeNonHittableTarget`, `resolution.ts:259-268`) — disclosed, but not prevented; the tap still - lands on the wrong element, just no longer silently. Disambiguation (N>1 matches, as opposed to one - unique-but-non-hittable match) has no equivalent disclosure today. -- **(g) Issues #279/#297 are precedent for trimming heal rather than growing it** when the evidence - says a heuristic isn't earning its complexity — see above. - -**Live hands-on evidence** (2026-07-10, driving replay by hand on the RN playground, iOS simulator, -both `.ad` and Maestro paths) grounds the same conclusions from the caller's seat: - -- **Successful replay is silent in text mode.** Exit 0, zero output; `replayed: 5` appears only under - `--json`. Structurally: replay's success payload (`{ replayed, healed, session, artifactPaths }`, - `session-replay-runtime.ts:186-195`) has no `message` field, so the generic CLI success path prints - nothing (`writeGenericCliOutput` → `readCommandMessage` → `writeCommandOutput`, - `src/cli/commands/generic.ts:68-71`, `src/utils/success-text.ts:12-14`, - `src/cli/commands/shared.ts:4-15`). An agent pays a verification turn just to learn what happened. -- **Failure output today is step + action + selector + a generic hint — no screen evidence.** The live - divergence hit was pure app state: the RN example app persists navigation state, so relaunch+deeplink - restored the Article screen and a perfectly correct selector legitimately missed. Heal can never fix - that class (the selector isn't wrong; reality is), while one line of screen evidence ("current - screen: Article") would have made the repair instant. The only recovery available was a full re-run — - no `--from` — and re-running earlier steps is precisely what makes state-restoring apps - nondeterministic across attempts. -- **Maestro step indices are untraceable to source today.** Breaking `tapOn: Push Input` — the 4th - top-level YAML step — failed as "Replay failed at step 5 (`__maestroTapOn` ...)": the flow's - `runFlow file: ../launch.yml` include had expanded into the linear plan and shifted every subsequent - index, and no file or line appears anywhere in the failure. Code-verified: `--maestro` input flattens - at parse time (`parseReplayInput`, `src/compat/replay-input.ts:47-68`) — `runFlow file:` inlines the - included file's actions (`convertRunFlow`/`readRunFlowActions`, - `src/compat/maestro/flow-control.ts:40-41,123-124`, via `parseRunFlowFile`, - `src/compat/maestro/replay-flow.ts:267-280`), platform/`true` `when` conditions are evaluated at - parse time (`flow-control.ts:47-48`), and `repeat.times` expands deterministically - (`flow-control.ts:84-87`). Provenance is lost in two stages: every action converted from one root - command inherits that PARENT command's YAML line (`convertRootCommands`, `replay-flow.ts:76-83`), - and `parseRunFlowFile`'s callers keep only `.actions`, discarding the included file's own line table - and path entirely. Even for `.ad`, the tracked line never reaches the caller: `actionLines` flows - into the per-action ndjson trace (`appendReplayTraceEvent`, - `src/daemon/handlers/session-replay-action-runtime.ts:47-56`) but `withReplayFailureContext` - (`session-replay-runtime.ts:349-369`) puts only `replayPath` + `step` in the error details. -- **The same failure class reports differently per format.** An `.ad` selector miss is - `COMMAND_FAILED` with the targeted hint "Run snapshot -i ... or use find ..." - (`selectorFailureHint`, `src/selectors/resolve.ts:110-113`, thrown at `resolution.ts:213-217`); - the equivalent Maestro miss is `ELEMENT_NOT_FOUND` constructed with no hint - (`src/compat/maestro/runtime-interactions.ts:644-652`), falling through to the generic default - "Retry with --debug and inspect diagnostics log for details." (`defaultHintForCode`, - `src/kernel/errors.ts:253-254`). -- **Recordings contain zero verification steps.** The script writer strips every recorded `snapshot` - action (`buildOptimizedActions`, `src/daemon/session-script-writer.ts:69`: `if (action.command === - 'snapshot') continue;` — only synthetic ref-scoped snapshots are re-inserted, as resolution aids, not - observations), and the record-time flag allowlist (`SANITIZED_FLAG_KEYS`, - `src/daemon/session-action-recorder.ts:46-77`) carries neither `settle`/`settleQuietMs` nor `verify`, - so `--settle`/`--verify` are dropped from recorded steps. A recording therefore replays actions with - no outcome observation at all — exactly the gap decision 3's record-time identity evidence fills. - -A related, currently under-used precedent: recorded `@ref` steps already carry an optional identity -hint in the `.ad` file. `appendRefLabel` (`src/daemon/session-script-writer.ts:235-240`) writes the -node's label as a trailing token, parsed back into `action.result.refLabel` -(`src/replay/script.ts:269,295,315`). Today that label is used only as a fallback LOOKUP key -(`tryResolveRefNode`'s `fallbackLabel`, `resolution.ts:393,413-430`) when the ref itself fails to -resolve, and to scope the pre-action snapshot capture (`buildScopedSnapshotAction`, -`session-script-writer.ts:136-155`) — never as a check against what disambiguation actually picked. It -establishes the pattern this ADR's decision 3 extends into a verification role: per-step identity -already travels in the `.ad` file. +Accepted (2026-07-10). Implemented on `main`, including the amendments folded into the decisions +below (#1264, #1269, #1271 stage 2, #1280, #1349); only decision 5's replay benchmark extension +remains deferred. The per-step landing record (PRs #1193-#1349) and the pre-acceptance migration +plan live in this file's git history. + +## Rules at a glance + +Normative summary, one entry per decision. The binding contracts, amendments, and edge cases are in +[Decision](#decision); the registries and tests named there are the living source of truth. + +1. **`--update` never rewrites.** Heal's candidate machinery only ranks the bounded `suggestions` + list inside a divergence report (identity components, then same `scrollRegion`, then document + order; deduplicated by node). No unattended repair path exists. +2. **Every element resolution discloses how it resolved.** Additive `resolution` response field: + `runtime`/`unique` or `runtime`/`disambiguated` (`matchCount`, `tiebreak`, up to 5 alternatives), + `ref`/`exact` or `ref`/`label-fallback`, `direct-ios`/`not-observed`; coordinate dispatches and + executed maestro-fallbacks carry none. Disclosed alternatives are pre-action diagnostics, never + issued refs. Enforced as ADR 0011's `resolutionDisclosure` guarantee row + (`src/contracts/interaction-guarantees.ts`). +3. **Recording writes `target-v1` identity evidence; replay verifies it before acting.** One + versioned JSON comment per element-targeting action carries identity (unique id, else + role+label, plus a leaf-anchored ancestry prefix), disambiguation signals (`sibling`, + region-scoped `viewportOrder`), and never-compared diagnostics (`rect`). Replay classifies every + annotated step through six exact verification paths; anything the evidence cannot isolate is an + `identity-unverifiable` divergence, never a silent pick. Amendments: non-unique ids demote to + role+label (#1269); an identity-empty pressed container records its first labeled descendant, + double-guarded fail-closed (#1280); `wait` landmark identity and `is` coverage dispatch on the + `targetIdentityVerification` descriptor trait (#1349). +4. **Divergence is a structured error, resumable by plan ordinal.** `ok:false` with code + `REPLAY_DIVERGENCE` and `details.divergence` v1: `kind` (`action-failure` | `selector-miss` | + `identity-mismatch` | `identity-unverifiable`), a bounded `screen` (same capture scope as plain + `snapshot`, ranked refs — #1264 — activating a partial ref frame per ADR 0014), one `repairHint` + enum, and `resume` (`from`, optional `alternateFrom`, `planDigest`, `repairSessionHeld`). + Serialized ceilings per response level: 8/24/64 KiB. Resume is `replay --from N --plan-digest + `, replay-only (`test` rejects `--from`), with no state reconstruction: skipped `outputEnv` + producers or control flow reject the resume. +5. **Validation is contractual.** The coverage inventory under decision 5 (guarantee-matrix cells, + parser/writer round trips, all six verification paths, wire-projection parity, `--update` + no-write) gates acceptance; benchmark evidence alone is insufficient. +6. **Repair is agent-performed and transactional.** `replay --save-script` opens a persisted + ARMED -> COMPLETE -> COMMITTED transaction governed by rules R1-R7: recording armed from step 1, + `--from` continuation only, daemon-computed `repairHint` routing, fail-loud on bare `@ref` + export, healed slice bounded by the `saveScriptBoundary` watermark, session kept alive until + commit or abort, and exclusive-`linkSync` publication that refuses any pre-existing target. + Out-of-band observations are excluded from the healed script by provenance + (`internal.replayPlanStep`), with `--record` as the per-action opt-in (#1271 stage 2). ## Decision @@ -218,9 +63,8 @@ recorded-id match outranks a role+label match, which outranks a label-only match candidates in the same `scrollRegion` as recorded rank before candidates in other regions; (3) document order is the final tie-break. Suggestions are deduplicated by node: a node reachable through several recorded selector terms appears once, tagged with its strongest match basis. The list is bounded by -decision 4's suggestion cap. Response levels affect only report content, never file behavior: before -retirement lands (migration step 6), `--update` keeps its legacy rewrite semantics regardless of level; -after retirement, `--update` at any level performs no rewrite and returns the same bounded suggestions +decision 4's suggestion cap. Response levels affect only report +content, never file behavior: `--update` at any level performs no rewrite and returns the same bounded suggestions object, with `--level digest` omitting suggestion entries but carrying `suggestionCount` per decision 4. With an agent in the loop, adjudicating a heal @@ -822,8 +666,7 @@ corrective interactive action(s) land in `session.actions` with fresh `target-v1 recording is armed and armed recording also disables the direct-iOS fast path (PR #1196) so evidence is computable. `formatSessionScript` over `session.actions` from the repair-run boundary (R6 — the slice recorded during this repair, not the whole session history) is therefore the healed script: the path that -actually worked. The only net-new code is flag-threading plus one writer entry point (see Migration -plan). +actually worked. The only net-new code is flag-threading plus one writer entry point. **The two repair sub-flows (routed by the mechanical `repairHint`).** A `selector-miss` with `matchCount: 0` (decision 3) is the same wire surface for "label renamed" and "app is on the wrong screen @@ -1240,95 +1083,155 @@ The tombstone itself expires after its bounded window, after which the key is fu - **Silent auto-rewrite (the old `--update`)**: already retired (decision 1) — mis-binding risk; selector agreement is not proof of the same target. -## Migration plan - -Steps are ordered so every dependency lands before its consumer; each step is independently useful, and -each states its dependencies explicitly. - -1. **Resolution disclosure** (decision 2) — no dependencies. Update all six matrix cells, the exact - waiver list, and provider mutation contracts together. Additive to response data; does not claim - direct-iOS selection parity or issue pre-action refs. -2. **Structured divergence transport** (decision 4, report only) — no dependencies. `REPLAY_DIVERGENCE` - with `kind: "action-failure"` attaches to the EXISTING replay failure paths: step provenance (source - path + line preserved through Maestro includes), bounded/redacted payloads, actionable-or-unavailable - screen semantics, error-path MCP pinning, ranked suggestions (decision 1's candidate machinery, - read-only), and the one-line text success summary. Immediately useful on its own — this closes the - provenance/evidence gaps the live hands-on evidence documents — and introduces no verification - semantics. -3. **`.ad` target annotations, inert** (decision 3, parser/writer only) — no dependencies. Bounded - parser/writer round trips, the writer-parser invariant with root-side reduction, old/new reader - compatibility, structural uniqueness, and duplicate detection. Recordings gain annotations; replay - parses and preserves them but does not yet enforce. -4. **Target-binding verification** (decision 3, enforcement) — depends on 2 (reports through the - divergence transport, adding the `selector-miss`/`identity-mismatch`/`identity-unverifiable` kinds) - and on 3 (consumes the annotations). -5. **`replay --from` + `--plan-digest` resume** (decision 4, resume) — depends on 2 only (the report - supplies `resume` and `planDigest`); may land before, with, or after 3/4. `test` does not expose - `--from`. -6. **`--update` retirement** (decision 1) — depends on 2 (ranked suggestions must be available in the - report before the write path is removed), with a no-write regression test. -7. **Benchmark extension** (decision 5) — follows the mandatory contracts; measures the economic claim - (clean replay plus one induced divergence repaired through the allowed `--from` loop). -8. **Agent-supervised re-record repair, base** (decision 6, R1-R6) — **MERGED (#1228)**; depends on 2 - and 5 (the repair loop is built entirely on the existing divergence report and `--from`/`--plan-digest` - resume machinery) and on 3/4 (corrective actions record fresh `target-v1` evidence, so the healed - script is self-consistent). Prerequisite: the selector-miss → `REPLAY_DIVERGENCE` defensive fix - (PR #1223) — a thrown per-action selector-miss must route through the same divergence-wrapping path as - a returned failure, or the repair loop never sees a divergence report to act on. Delivered: the - daemon-side `repairHint` computation over all four `kind`s (R3), `--save-script` arming plus the - repair-run boundary watermark on `replay` (R1/R6), the writer's post-watermark slice and bare-`@ref` - fail-loud guard (R4/R6) — reusing `close --save-script`'s existing `session.actions` serializer. -9. **Repair-transaction lifecycle** (decision 6, R7 + commit state machine) — **SHIPPED (#1235)**; - depends on 8. Adds: the distinct `resume.repairSessionHeld` divergence signal and - R7 keep-alive keyed off the session's **persisted** repair-transaction state (so `--from` continuations - need no repeat of `--save-script`), with `fail-fast-before-step-1` when keep-alive is impossible; the - `ARMED → COMPLETE → COMMITTED` commit state machine where **any teardown** (explicit `close`, - idle-reap, or daemon shutdown) commits atomically when `COMPLETE`, aborts (no prefix) when not, and is - idempotent once `COMMITTED`; terminal-source-`close` **skipping** during an armed replay/resume, with a - regression proving the session is NOT deleted when the terminal `close` is reached; the - `REPAIR_SESSION_EXPIRED` tombstone for an incomplete-transaction reap/shutdown (keyed by session key, - owner + bounded expiry, cleared by a fresh `replay --save-script`); and race-safe atomic publication - (temp file in the target's own directory, published via a single exclusive `linkSync` that refuses ANY - pre-existing target — complete or partial, default or explicit path alike, and uniformly for an - ordinary non-repair recording's target too, since the publish primitive is shared — never overwriting - one; see "Scope" under Decision 6 above). -10. **Repair-segment default exclusion of out-of-band observations, and the `--record` opt-in** - (decision 6 amendment) — **SHIPPED (#1271 stage 2)**; depends on 9 (`session.saveScriptBoundary`/ - `repairSessionHeld` are the signals the exclusion and its guidance key off) and on #1304 (the shared - reader helper `--record`'s scoped sibling is split from). Adds: the `internal.replayPlanStep` - provenance marker stamped by `invokeResolvedReplayAction` (`session-replay-action-runtime.ts`); - default exclusion of OUT-OF-BAND `snapshot`/`get`/`is`/read-only `find` from `session.actions` while - repair-armed, keyed off that provenance via `isInteractiveObservation`/ - `isExcludedRepairSegmentObservation` (`session-action-recorder.ts`), so authored plan steps survive - their own heal automatically; the `--record` opt-in, statically scoped to `snapshot`/`get`/`is` and - dynamically validated for `find`, rejected together with `--no-record` as `INVALID_ARGS`; and an - updated `describeUnperformedRecordAndHeal` message naming `--record` for the corrective-read case. - Stage 1 (#1287) shipped interim `--no-record` guidance only, gated on this same `repairSessionHeld` - signal. - -## Migration progress - -Landing record for the plan above (main as of 2026-07-16). This section tracks progress only; it does -not restate or amend the decisions. - -| Step | Decision | Status | Landed in | -| --- | --- | --- | --- | -| 1. Resolution disclosure | 2 | Shipped | #1193 | -| 2. Structured divergence transport | 4 (report) | Shipped | #1197 | -| 3. `.ad` target annotations, inert | 3 (parser/writer) | Shipped | #1196 | -| 4. Target-binding verification | 3 (enforcement) | Shipped | #1209 | -| 5. `replay --from` + `--plan-digest` resume | 4 (resume) | Shipped | #1211 | -| 6. `--update` retirement | 1 | Shipped | #1211 | -| 7. Benchmark extension | 5 | Deferred | — | -| 8. Agent-supervised re-record repair, base | 6 (R1-R6) | Shipped | #1228 | -| 9. Repair-transaction lifecycle | 6 (R7) | Shipped | #1235 | -| 10. Repair-segment default exclusion + `--record` | 6 amendment | Shipped | #1271 stage 2 | -| 11. Read-only step identity (wait landmark verification, `is` coverage) | 3 amendment (#1349) | Shipped | — | - -Step 4 (#1209) added the `selector-miss`/`identity-mismatch`/`identity-unverifiable` divergence kinds -and a post-resolution target guard that cross-checks the dispatched winner against the verified member. -Issue #1221 ("Complete ADR 0012 replay target-binding verification") was closed as already implemented: -its verification scope is covered by step 4. Step 7 remains deferred. Step 8 implements the repair -design accepted in #1226 and shipped in #1228; step 9 (R7 + commit machine) shipped in #1235. Step 10 -is #1271's two-stage arc: stage 1 (#1287) shipped interim `--no-record` guidance only; stage 2 (this -amendment) makes exclusion the default and adds the `--record` opt-in. +## Context and evidence at acceptance (2026-07-10) + +This section records the repository state audited on 2026-07-10, before the decisions above +shipped. Its present-tense observations and code pointers are historical evidence; the decisions +above and the gates they name are the authoritative record of current behavior. + +At acceptance, replay was deterministic. `.ad` scripts were plain text — one action per line, `#` +comments, and a `context platform=... device=... theme=...` header (`src/replay/script.ts`) — recorded +via `open --save-script` (`src/daemon/session-action-recorder.ts`, +`src/daemon/session-script-writer.ts`) or hand-written, and executed step-by-step by +`runReplayScriptFile` +(`src/daemon/handlers/session-replay-runtime.ts`) under the daemon's `replay`/`test` commands +(`src/daemon/handlers/session-replay.ts`). Recorded touch/fill/get targets are selector chains with +`||` alternates (`buildSelectorChainForNode(...).join(' || ')`, +`src/commands/interaction/runtime/resolution.ts:242`, mirrored in +`src/daemon/handlers/session-replay-heal.ts:131-135`); Maestro YAML flows import through `--maestro` +(`src/compat/maestro/`); progress is step-indexed (`stepIndex`/`stepTotal` in +`emitReplayTestActionProgress`, `session-replay-runtime.ts:243-260`). + +Recovery was opt-in `--update`/`-u` healing (`replayUpdate` flag, +`src/commands/cli-grammar/flag-definitions-workflow.ts`). It only fires after a step has already +returned a hard failure (`session-replay-runtime.ts:118-149`: `if (!shouldUpdate) return failure; ... +healReplayAction(...)`), and it only retries the SAME recorded selector material — +`collectReplaySelectorCandidates` (`session-replay-heal.ts:39-81`) gathers the step's originally +recorded `selectorChain`/positionals, then `resolveSelectorChain` re-resolves those exact candidate +strings against a freshly captured snapshot (`session-replay-heal.ts:122-135`). If the identifying term +itself changed — an id or label rename — the same string will not match the new tree either, so heal +cannot rescue renames; it can only recover drift the ORIGINAL selector still matches (a moved or +re-rendered node with the same id). PR #297 (closing #279) already trimmed heal once, removing +`refLabel`-synthesis and numeric `get text` drift healing to keep it "centered on recorded selectors and +explicit selector expressions" — heal has a maintained history of narrowing, not growing. + +**Benchmark evidence** (2026-07-09/10, iOS simulator, react-navigation/RN playground matrix; harness +follows the `~/.agent-device-bench/rnnav-matrix.py` pattern, external — the key numbers are recorded +here so the evidence stays durable without the harness directory): + +| Measurement | Result | +| --- | --- | +| Snapshot captures per interaction, `--settle` off → on | 3.67 → 1.00 (the 1-snapshot floor) | +| Commands per task, settled arm vs unsettled arms | 14.3 vs 23.3 / 26.7 | +| react-navigation Maestro suite via deterministic replay | 38/38 flows green in 539 s, zero model turns | + +With the settle loop at its snapshot floor, wall time for an agent-driven QA flow is dominated by model +turn latency, not device I/O. A happy-path agent-driven QA flow costs O(steps) model turns end-to-end; a +deterministic replay of the same flow costs O(divergences) — the 38/38 sweep is that limit realized at +zero divergences. The entire economic case for replay is collapsing the per-step model-turn cost toward +zero on the happy path and paying only where reality diverged from the recording. + +**Audit evidence** (2026-07-10) on where that divergence cost actually goes: + +- **(a) Heal is narrow and mostly unable to act.** Per the mechanism above, heal only recovers + same-selector drift. Most real replay failures are renames or removals heal's candidate-recycling + cannot reach. +- **(b) The real mis-binding surface is not heal — it is silent disambiguation in ORDINARY resolution**, + live and replay alike. `resolveSelectorInteractionTarget` calls `resolveSelectorChain(..., { + disambiguateAmbiguous: true })` on every press/click/fill (`resolution.ts:170-183`); when a selector + matches N>1 nodes, `accumulateDisambiguationCandidate`/`compareDisambiguationCandidates` + (`src/selectors/resolve.ts:181-285`) silently pick a winner — visible candidates over + off-screen ones, then deepest node, then smallest on-screen area, only an exact tie failing. + `describeResolvedInteractionNode` (`resolution.ts:227-249`), the response's entire identity payload, + carries `node`/`selectorChain`/`refLabel`/`targetHittable`/`hint` — no match count, no signal a + tiebreak happened at all. This was live-reproduced during the audit on an RN playground screen with + two identical-rect "Prevent Remove" buttons, where scroll position alone decided which one a selector + hit. The general policy is documented (`agent-device help workflow`, + `src/cli/parser/cli-help.ts:243,384`: "does not fail by default ... auto-resolves deepest node first + ... then smallest on-screen area") but never disclosed per response — an agent that hasn't read the + help topic, or whose target moved between recording and replay, gets no signal a heuristic rather than + an exact match chose its target. +- **(c) No target-binding verification existed in this path at acceptance.** `--verify` + (`captureEvidenceBaseline`, `resolution.ts:45-58,104-134`; the `verifyEvidence` guarantee cell in ADR + 0011's registry) attaches a pre/post-action node diff so the caller can see SOMETHING changed — it + says nothing about whether the CORRECT node was the one tapped. A wrong-but-plausible pick (the + sibling "Prevent Remove" button) produces a real, visible diff and is still the wrong action. +- **(d) Heal auditability was a bare count.** A successful `--update` run returned + `{ replayed, healed, ... }` (`session-replay-runtime.ts:186-195`) — `healed` is a number, nothing + else — and rewrote the `.ad` file in place via `writeReplayScript` + (`session-replay-runtime.ts:182-184`, `src/replay/script.ts:459-484`) with no diff shown anywhere in + the response. +- **(e) This silent-pick default is in real tension with this repo's general posture toward ambiguity.** + Elsewhere, ambiguous input is refused and hinted about rather than silently guessed — `start`/`restart` + are deliberately left out of the CLI alias-suggestion table because `start` is "genuinely ambiguous, so + a hint beats silently guessing" (`src/cli/parser/command-suggestions.ts:16-17`). Selector resolution + took the opposite default, and ADR 0011's own registry records that choice precisely: the + `disambiguation` cell for `runtime-selector` is classified `{ kind: 'runtime', via: + '...selectors-resolve.ts#resolveSelectorChain' }` (`src/contracts/interaction-guarantees.ts:176-179`) + — proving the heuristic runs consistently across paths, not that the caller is told it ran. That + default is not being revisited here; see the rejected hard-reject alternative below for why. +- **(f) Issue #1037 / PR #1040 is the direct, partial precedent.** A UNIQUE-but-wrong match (Apple + Maps' `text="Anthropic - Headquarters"` exact-matching a 30x30 map-pin annotation instead of the + recents row) now surfaces as `targetHittable:false` plus a hint + (`describeNonHittableTarget`, `resolution.ts:259-268`) — disclosed, but not prevented; the tap still + lands on the wrong element, just no longer silently. Disambiguation (N>1 matches, as opposed to one + unique-but-non-hittable match) has no equivalent disclosure today. +- **(g) Issues #279/#297 are precedent for trimming heal rather than growing it** when the evidence + says a heuristic isn't earning its complexity — see above. + +**Live hands-on evidence** (2026-07-10, driving replay by hand on the RN playground, iOS simulator, +both `.ad` and Maestro paths) grounds the same conclusions from the caller's seat: + +- **Successful replay is silent in text mode.** Exit 0, zero output; `replayed: 5` appears only under + `--json`. Structurally: replay's success payload (`{ replayed, healed, session, artifactPaths }`, + `session-replay-runtime.ts:186-195`) has no `message` field, so the generic CLI success path prints + nothing (`writeGenericCliOutput` → `readCommandMessage` → `writeCommandOutput`, + `src/cli/commands/generic.ts:68-71`, `src/utils/success-text.ts:12-14`, + `src/cli/commands/shared.ts:4-15`). An agent pays a verification turn just to learn what happened. +- **Failure output today is step + action + selector + a generic hint — no screen evidence.** The live + divergence hit was pure app state: the RN example app persists navigation state, so relaunch+deeplink + restored the Article screen and a perfectly correct selector legitimately missed. Heal can never fix + that class (the selector isn't wrong; reality is), while one line of screen evidence ("current + screen: Article") would have made the repair instant. The only recovery available was a full re-run — + no `--from` — and re-running earlier steps is precisely what makes state-restoring apps + nondeterministic across attempts. +- **Maestro step indices are untraceable to source today.** Breaking `tapOn: Push Input` — the 4th + top-level YAML step — failed as "Replay failed at step 5 (`__maestroTapOn` ...)": the flow's + `runFlow file: ../launch.yml` include had expanded into the linear plan and shifted every subsequent + index, and no file or line appears anywhere in the failure. Code-verified: `--maestro` input flattens + at parse time (`parseReplayInput`, `src/compat/replay-input.ts:47-68`) — `runFlow file:` inlines the + included file's actions (`convertRunFlow`/`readRunFlowActions`, + `src/compat/maestro/flow-control.ts:40-41,123-124`, via `parseRunFlowFile`, + `src/compat/maestro/replay-flow.ts:267-280`), platform/`true` `when` conditions are evaluated at + parse time (`flow-control.ts:47-48`), and `repeat.times` expands deterministically + (`flow-control.ts:84-87`). Provenance is lost in two stages: every action converted from one root + command inherits that PARENT command's YAML line (`convertRootCommands`, `replay-flow.ts:76-83`), + and `parseRunFlowFile`'s callers keep only `.actions`, discarding the included file's own line table + and path entirely. Even for `.ad`, the tracked line never reaches the caller: `actionLines` flows + into the per-action ndjson trace (`appendReplayTraceEvent`, + `src/daemon/handlers/session-replay-action-runtime.ts:47-56`) but `withReplayFailureContext` + (`session-replay-runtime.ts:349-369`) puts only `replayPath` + `step` in the error details. +- **The same failure class reports differently per format.** An `.ad` selector miss is + `COMMAND_FAILED` with the targeted hint "Run snapshot -i ... or use find ..." + (`selectorFailureHint`, `src/selectors/resolve.ts:110-113`, thrown at `resolution.ts:213-217`); + the equivalent Maestro miss is `ELEMENT_NOT_FOUND` constructed with no hint + (`src/compat/maestro/runtime-interactions.ts:644-652`), falling through to the generic default + "Retry with --debug and inspect diagnostics log for details." (`defaultHintForCode`, + `src/kernel/errors.ts:253-254`). +- **Recordings contain zero verification steps.** The script writer strips every recorded `snapshot` + action (`buildOptimizedActions`, `src/daemon/session-script-writer.ts:69`: `if (action.command === + 'snapshot') continue;` — only synthetic ref-scoped snapshots are re-inserted, as resolution aids, not + observations), and the record-time flag allowlist (`SANITIZED_FLAG_KEYS`, + `src/daemon/session-action-recorder.ts:46-77`) carries neither `settle`/`settleQuietMs` nor `verify`, + so `--settle`/`--verify` are dropped from recorded steps. A recording therefore replays actions with + no outcome observation at all — exactly the gap decision 3's record-time identity evidence fills. + +A related, currently under-used precedent: recorded `@ref` steps already carry an optional identity +hint in the `.ad` file. `appendRefLabel` (`src/daemon/session-script-writer.ts:235-240`) writes the +node's label as a trailing token, parsed back into `action.result.refLabel` +(`src/replay/script.ts:269,295,315`). Today that label is used only as a fallback LOOKUP key +(`tryResolveRefNode`'s `fallbackLabel`, `resolution.ts:393,413-430`) when the ref itself fails to +resolve, and to scope the pre-action snapshot capture (`buildScopedSnapshotAction`, +`session-script-writer.ts:136-155`) — never as a check against what disambiguation actually picked. It +establishes the pattern this ADR's decision 3 extends into a verification role: per-step identity +already travels in the `.ad` file. diff --git a/docs/adr/0014-session-ref-frame-lifetime.md b/docs/adr/0014-session-ref-frame-lifetime.md index 799a052b21..46072fb44a 100644 --- a/docs/adr/0014-session-ref-frame-lifetime.md +++ b/docs/adr/0014-session-ref-frame-lifetime.md @@ -2,7 +2,37 @@ ## Status -Accepted +Accepted; implemented and enforced on `main` across all platforms. Fresh live evidence exercised +every production seam except Android blocking-dialog recovery, which shipped as a documented, +accepted gap: no deterministic repro exists to raise the system dialog on demand, and the seam's +transition and abort logic are pinned by fixture regressions +(`android-system-dialog-ref-frame.test.ts`, `interaction-android-recovery-abort.test.ts`). The +staged migration plan lives in this file's git history; PR #1241 was an earlier compatible +transitional fix, not this architecture migration. + +## Rules at a glance + +Normative summary; the binding contracts and edge cases are in [Decision](#decision), and the +ref-frame vocabulary is promoted into `CONTEXT.md`. + +- A session owns at most one **ref frame** — the authorization namespace for mutation refs (epoch + exposed as `refsGeneration`, immutable source tree, `active`/`expired` state, `all` or bounded + issuance scope) — owned by `src/daemon/ref-frame.ts` and kept separate from the latest + operational observation (`session.snapshot`). +- A complete snapshot activates an `all` frame; `find`, settled diffs, and replay divergence + screens activate a bounded partial frame that supersedes the prior one; internal read-only + captures never activate, reindex, or expire a frame. +- Every mutating leaf expires the frame at the side-effect seam — after all pre-action guards, + immediately before the device op — with no success-only rollback; a post-dispatch failure still + leaves it expired. +- Mutation admission requires an active frame whose epoch and issuance scope authorize the ref; + rejections carry ordered typed reasons `ref_frame_expired` -> `ref_generation_mismatch` -> + `plain_ref_requires_complete_frame` -> `ref_not_issued`. +- Read-only ref consumers stay fail-open with a structured staleness warning only while the frame + retains the ref's evidence; missing evidence fails rather than resolving by positional + coincidence against a newer observation. +- Every daemon command declares a `RefFrameEffect` (`preserve` | `may-invalidate` | `delegated`) in + its descriptor daemon facet; the registry classification is completeness-gated. ## Context @@ -36,7 +66,7 @@ remain compatible with selector-based replay, and add no automatic capture or pe ## Decision The terms introduced below are now the implemented model; the ref-frame vocabulary is promoted into -`CONTEXT.md`. The superseded coarse `snapshotRefsStale` marker has been removed (migration step 8): +`CONTEXT.md`. The superseded coarse `snapshotRefsStale` marker has been removed: read-only ref staleness is now derived from frame state — a plain ref warns once the frame has expired, and a read-only capture no longer marks refs stale because it does not expire the frame. @@ -64,7 +94,7 @@ the evidence tree, bounds partial authority. The frame and latest observation sh data when they originate from the same capture; neither transition deep-copies the tree. The existing `snapshotGeneration` implementation evolves behind one ref-frame module; the -`snapshotRefsStale` marker it originally paired with has since been removed (migration step 8). The +`snapshotRefsStale` marker it originally paired with has since been removed. The public name `refsGeneration` and the `@e12~s42` grammar remain unchanged for wire compatibility. ### Frame transitions @@ -418,50 +448,6 @@ the stale ref is rejected before dispatch, and a fresh observation restores usab exercised remains disabled or is recorded as an explicit release blocker. Fixture-backed tests and registry claims are necessary but do not substitute for this live evidence. -## Migration - -Each step lands green and independently useful: - -1. introduce the bounded ref-frame module and separate operational observations without changing - enforcement; -2. add the complete daemon descriptor classification and gate, including request-sensitive resolvers; -3. route every leaf side effect, fallback, retry, and readiness recovery through the idempotent - pre-side-effect transition; -4. correct complete/partial publication, bounded scoped-snapshot lineage, MCP pin retention, pinned - partial CLI text, and JSON/Node.js response-level generation handling; -5. decouple Android freshness capture from positional ref authorization; -6. add sequence, batch, replay, series, failure-boundary, and cross-platform provider contracts; -7. enable fail-closed mutation enforcement per platform only after its paths have evidence, ending in - one uniform policy; and -8. update help, changelog, ADR 0012's proposed-amendment note and tests, and replay/batch guidance for - refresh-between-mutations, conservative fused no-op invalidation, and reads without retained - evidence; promote the implemented vocabulary into `CONTEXT.md`; then remove the superseded coarse - stale marker. - -Implementation status: all migration steps have landed — the ref-frame module and observation split -(1), the complete daemon classification and gate (2), the pre-side-effect seam at every leaf (3), -correct complete/partial publication with bounded scope, MCP pin retention, and pinned partial CLI text -(4), Android freshness decoupled from positional ref authorization (5), the cross-platform contract and -provider evidence (6), fail-closed admission enforcement across platforms with typed reasons (7), and -the docs/vocabulary promotion plus removal of the superseded coarse `snapshotRefsStale` marker (8). -Fresh live evidence has exercised nearly every production seam — Apple runtime-ref, direct/native -selector, generation-pin, generic, and lifecycle paths; Android helper freshness (including proven -non-retarget) and Android existing-session relaunch; and a real provider-backed interaction plus -provider-backed lifecycle operation (AWS Device Farm, `backend: webdriver`: a fresh ref succeeded, an -immediate stale ref was rejected before dispatch with the shared typed fields, an `open --relaunch` -lifecycle mutation expired the frame, and a fresh observation restored authorization). ONE seam — -Android blocking-dialog recovery — was NOT live-exercised: the repo harness exposes no deterministic -app-owned ANR trigger, and no reproducible control exists to raise the system dialog on demand. The -team accepted shipping without a live run for it: its transition and abort logic are covered by fixture -regressions (`android-system-dialog-ref-frame.test.ts` proves recovery expires the frame before its -tap; `interaction-android-recovery-abort.test.ts` proves an outstanding ref action then aborts with the -shared `ref_frame_expired` rejection and no dispatch), the seam is enforced in code identically to the -verified paths, and it is recorded here as a documented, accepted evidence gap rather than an open -release blocker. Every other enabled seam is proven on hardware. - -PR #1241 landed independently as a compatible transitional fix. It rejects a known iOS stale-marker -case before this full lifecycle is implemented; it does not own the architecture migration. - ## Alternatives considered - **Stable refs across trees:** rejected because positional accessibility snapshots have no durable diff --git a/docs/adr/0016-active-session-script-publication.md b/docs/adr/0016-active-session-script-publication.md index a123e1caa8..6a9d21f749 100644 --- a/docs/adr/0016-active-session-script-publication.md +++ b/docs/adr/0016-active-session-script-publication.md @@ -2,7 +2,31 @@ ## Status -Accepted +Accepted; implemented on `main` (`src/daemon/handlers/session-script-publication.ts`, +`src/daemon/session-script-active-publication.ts`), including the #1349 identity-level +destination-guard amendment. + +## Rules at a glance + +Normative summary; the binding contracts and refusal cases are in [Decision](#decision). + +- `session save-script [path] [--force]` publishes the current ordinary script recording without + closing the app or the session; the session stays live at the destination. +- Recording lifecycle: **ARMED** (only by a new session's initial `open --save-script`) -> + **ABORTED** (a second successful plain `open`) or **PUBLISHED** (successful save). ABORTED and + PUBLISHED are terminal until the session is destroyed; a filesystem/target failure leaves the + recording ARMED and retryable. +- Publication requires exactly one initial recorded `open`, a portable selector-`wait` destination + guard after the last descriptor-classified mutating action — since #1349 the guard's `target-v1` + annotation must be `verification: "verified"` — and no unresolved session-local `@ref`. Every + refusal happens before filesystem work and names its recovery. +- Repair transactions (ADR 0012) are disjoint: a session with `saveScriptBoundary` set refuses this + action without changing repair state. +- `ReplayCommandResult.sessionActive` (from the daemon's session store, never script re-parsing) is + what lets a one-shot client keep the daemon alive on a close-less handoff. +- Sensitive `fill` inputs must be recorded as placeholders via `fill --record-as ` + (ADR 0017, shipped for #1348); unparameterized `fill`/`type` values persist literally into the + artifact, so a secret entered without `--record-as` is published. ## Context diff --git a/docs/adr/0018-unified-event-journal.md b/docs/adr/0018-unified-event-journal.md new file mode 100644 index 0000000000..34f2233d8c --- /dev/null +++ b/docs/adr/0018-unified-event-journal.md @@ -0,0 +1,375 @@ +# ADR 0018: Unified Request Event Journal + +## Status + +Proposed (2026-07-24; revised same day after architecture review). Nothing here is implemented. + +Scope reduction relative to the first draft, per review: progress streaming stays a permanent, +transport-owned second channel (it is an output port, not telemetry); the journal absorbs +diagnostics, agent-cost, the session event log, and the replay timing trace. Out-of-request +session events get an explicit scope model, and the replay-trace redaction fix is declared as an +intentional compatibility change rather than hidden under a byte-compatibility claim. + +## Rules at a glance + +Normative summary of the proposal; contracts and rationale below. + +- One **event catalog** (`src/contracts/events.ts`) is the single declaration site for every + diagnostic/telemetry event kind: key, subsystem, default level, and derivation traits. Emitting a + kind not in the catalog is a compile error; consumers derive kind sets from traits, never from + parsing kind names. +- Catalog keys are **internal identities**. Legacy wire/file discriminators (today's diagnostics + `phase` strings, the replay trace's `type` values) are preserved by sinks as output mappings; + a wire name is never automatically the canonical cross-channel identity. +- One **journal** per scope is the single append point (`emitDiagnostic` evolves into it). Scopes + are: the existing per-request scope, and explicit short-lived **session-scoped teardown scopes** + for out-of-request work (idle reap, daemon shutdown), following the existing fatal-scope + precedent. Emitters state facts; they never know who is listening. No `EventEmitter`, no dynamic + subscription. +- Every consumer is a **sink**: explicitly registered at scope construction, invoked synchronously + in registration order, individually best-effort (a sink error is swallowed and must not affect + other sinks or the request). Concurrent sub-work (sharded test attempts run under + `Promise.allSettled`) runs in **forked child scopes that clone every mutable binding** — + envelope, log path, routing — and share only concurrency-safe aggregation state; nothing mutable + is ever shared across concurrent scopes. +- **Progress streaming is not journal-owned.** `src/request/progress.ts` and its typed + `RequestProgressEvent` union remain the transport's channel, unchanged. +- **Existing formats do not change, with one declared exception.** `events.ndjson` v1 entries, + `cost.runnerRoundTrips`, per-request diagnostics ndjson, and `replay-timing.ndjson` shapes stay + byte-compatible behind golden fixtures. The exception: replay-timing events written by the + currently unredacted helper become redacted — an intentional security fix, called out as a + behavior change, not smuggled in as "compatible." +- **Redaction happens once, at the journal boundary**, for every journal-owned output. +- The journal is an observability spine, **not** a source of truth: `session.actions` and all other + session state remain owned by their stores. No state is ever rebuilt from journal events. +- Anything that leaves the machine goes through a **closed, allowlist-by-construction schema** + (the usage sink's `UsageRecord`): every field drawn from a registry-enumerated vocabulary — + command names, typed error codes, flag names, durations — with positionals, selectors, labels, + and messages unrepresentable by type, not merely redacted. + +## Context + +The codebase has grown four parallel event vocabularies, each with its own emit call, shape, +redaction discipline, and sink (inventoried 2026-07-24): + +1. **Diagnostics** (`src/utils/diagnostics.ts`). ~155 distinct stringly-typed `phase` values across + ~70 files, an `AsyncLocalStorage` request scope entered in exactly three places (CLI pre-parse, + daemon per-request in `request-router.ts`, daemon fatal catch-all), an in-memory buffer plus a + `phaseCounts` tally, and debug-mode live streaming to the per-request ndjson file (after + `createRequestExecutionScope` rebinds `logPath`), `daemon.log`, or stderr. The + `traceLogPath` scope option is dead: no call site ever sets it. +2. **Session event log** (`src/daemon/session-event-log.ts`). Append-only per-session + `events.ndjson` with kinds `request.started`/`request.finished`/`action.recorded`, written from + three request-lifecycle points plus `SessionStore.recordAction`, read only by the public + `events` command. `action.recorded` is already a projection of `session.actions` pushes — the + model this ADR generalizes. Crucially, not every append happens inside a request: + `finalizeRepairTeardown` (`src/daemon/session-store.ts`) records a synthesized `close` action + during idle reap or daemon shutdown, with — per its own comment — "no live request here." +3. **Progress streaming** (`src/request/progress.ts`, `src/daemon/request-progress-protocol.ts`). + A closed, typed `RequestProgressEvent` union emitted through a single-sink `AsyncLocalStorage` + installed by the socket/HTTP transport, serialized as ndjson envelopes multiplexed onto the same + stream as the final response, with client-disconnect-as-cancellation tracking. Opt-in per + request via `meta.requestProgress`; disabled under `--json`. Events are written to the wire + **unredacted** today. +4. **Replay timing trace** (`src/daemon/handlers/session-replay-trace.ts`, + `session-test-runtime.ts`, read by `src/replay/test/trace.ts`). Per-**attempt** + `replay-timing.ndjson` files whose paths are created dynamically inside each attempt — written + by **two different helpers, one of which redacts and one of which does not** + (`appendReplayTraceEvent` vs `appendReplayTestTimingEvent`). + +(`src/upload-progress.ts` is a fifth, local-only callback sink for artifact uploads; it never +crosses the request scope and is out of scope here. Production code contains no `EventEmitter` +usage at all — the codebase already avoids implicit pub/sub, which this design preserves.) + +The channels overlap but share nothing: no common envelope, no shared kind vocabulary, and +consumers couple to emit sites by string. The sharpest instance: agent-cost's `runnerRoundTrips` +is `countDiagnosticEventsByPhase(['ios_runner_command_send', 'ios_runner_readiness_preflight'])` +(`src/daemon/request-router.ts`) — a hand-picked name list a new runner phase can silently drift +from. That violates two established repo rules: *what enumerates N?* (nothing enumerates +round-trip phases), and *categories come from recorded fields, never parsed names*. + +The near-term consumer motivating the unification is opt-in usage analytics over agent behavior — +command frequencies, failure codes, and outcome sequences that trip agents — under a hard privacy +constraint: request metadata only, never request content (decision 4). + +ADR 0008 solved the same shape of problem for commands: one declaration registry, behavior derived +by parity-tested projection. This ADR applies that thesis to events — to the channels that are +genuinely telemetry. Progress is not one of them (decision 3). + +## Decision + +### 1. Event catalog: kinds are declared data + +`src/contracts/events.ts` exports one `EVENT_CATALOG` — a const object with one entry per event +kind, keyed by today's diagnostics phase strings (no renames in this ADR): + +```ts +type EventDescriptor = { + /** Grouping for docs/filtering; not parsed from the kind name. */ + subsystem: 'apple-runner' | 'android' | 'daemon' | 'request' | 'record' | 'replay' + | 'snapshot' | 'cli' | 'web' | 'compat' | 'util'; + level: 'info' | 'warn' | 'error' | 'debug'; + /** Derivation traits — the only way a consumer may select kind sets. */ + traits?: readonly EventTrait[]; // e.g. 'runner-round-trip', 'session-lifecycle', 'replay-trace' +}; + +type EventKind = keyof typeof EVENT_CATALOG; +``` + +`emitDiagnostic`'s `phase` parameter narrows from `string` to `EventKind`, making every +uncataloged emit a compile error — the same completeness mechanism as the ADR 0011 matrix. The +existing ~155 phases enter the catalog verbatim; per-kind `data` payloads stay +`Record` in this ADR (typed payload schemas are a possible per-kind follow-up, +not a prerequisite — see Alternatives). + +Kinds absorbed from other channels (the session event log's `request.started`/`request.finished`/ +`action.recorded`, the replay trace's `replay_action_start`/`replay_action_stop`/attempt-lifecycle +events) get catalog entries whose **internal keys need not equal their legacy wire discriminators**; +the owning sink maps the internal kind to the exact legacy `kind`/`type` value on output. Wire and +file names stay frozen for consumers; catalog identities stay free to be consistent. + +Consumers derive kind sets from traits: `RUNNER_ROUND_TRIP_PHASES` becomes +`kindsWithTrait('runner-round-trip')` (exactly `ios_runner_command_send` and +`ios_runner_readiness_preflight` carry the trait; their `_skipped`/`_recovered` variants +deliberately do not, preserving today's cost semantics). A parity test pins the derived sets so a +trait edit is a reviewed decision, not a drift. + +The catalog lives in `contracts` (ranked, kernel-adjacent) so every zone — including unranked +peripherals and `utils` — may import it without a layering back-edge. The journal runtime evolves +in place in `src/utils/diagnostics.ts`, keeping all existing import directions legal. + +### 2. One journal, explicit sinks, defined scope model + +The diagnostics scope becomes the **journal**: the single append point for diagnostics, agent-cost +tallies, session lifecycle events, and replay traces. `emit(event)` does exactly what +`emitDiagnostic` does today — stamp the scope envelope (`ts`, `requestId?`, `session?`, +`command?`), redact `data`, buffer, tally `phaseCounts` — and then offers the event to each +registered sink. + +**Scope model.** Two scope shapes, both explicit: + +- the existing **per-request scope** (CLI entry, daemon `handleRequest`, fatal catch-all), + unchanged; and +- a **session-scoped teardown scope** for out-of-request work: idle reap and daemon-shutdown + finalizers (`finalizeRepairTeardown`, session teardown cleanup) open a short-lived scope bound to + the session (no `requestId`), following the existing `emitFatalDiagnostic` precedent. This is + what keeps "no sink writes without the journal" true for the teardown-recorded `close` action — + today's direct event-log append from `SessionStore.recordAction` must keep working when no + request exists, and silently dropping it is unacceptable. + +**Sink contract.** Sinks are constructed with the scope and listed in one place per entry point — +statically enumerable, greppable, no dynamic subscription API. Semantics, normative: + +- **Ordering:** sinks are invoked synchronously, in registration order, at emit time. A sink may + queue its own I/O (the session event log keeps its per-path serialized write queue). +- **Isolation:** each sink invocation is individually best-effort — an error is swallowed (as + `emitDiagnostic`'s stream writes are today) and must never affect other sinks, the buffer, or + the request. +- **Flushing:** flush remains journal-owned (`flushDiagnosticsToSessionFile` semantics unchanged); + sinks that buffer expose flush hooks the journal calls on request finalization and fatal paths. +- **Concurrency — fork, never rebind; forks clone all mutable state:** the journal gains a fork + primitive: `journal.fork(bindings, fn)` runs `fn` inside a new AsyncLocalStorage scope object + that **clones every mutable field of the parent scope** — the envelope (`session`, `command`, + `debug`), `logPath`, and all routing bindings — applies the fork's own frozen `bindings` on top, + and owns its own event buffer, flushed against the fork's bindings when the fork ends. Shared + with the parent are only the sink list and concurrency-safe aggregation state: the `phaseCounts` + tally stays request-global (single-threaded atomic increments; agent-cost must span shards). + Sharded `test` runs execute attempts concurrently (`Promise.allSettled` in + `runReplayTestShards`, `src/daemon/handlers/session-test.ts`) and their **nested dispatch calls + `createRequestExecutionScope` (`request-router.ts` child scopes), which rebinds `session` and + `logPath` mid-flight** — so a shared mutable envelope would cross-route debug and session-log + events between shards even with frozen trace bindings. Therefore: `updateDiagnosticsScope` + mutates only the innermost (current) scope, each shard/attempt lifecycle wraps its work — + including all nested replay dispatch — in a fork, and every mid-flight rebind lands on that + shard's clone. The fork's trace binding (the attempt's `replay-timing.ndjson` path) dies with + the fork, so no clearing step exists to race; the replay trace sink drops trace-kind events + emitted with no bound destination. +- **Scope identity (exporter readiness):** every scope — request, teardown, and fork — carries a + `scopeId`, and a fork records its parent's as `parentScopeId`; both ride the event envelope. + This is deliberately minimal span plumbing: forks already form a tree, so a future exporter sink + (e.g. OpenTelemetry) can emit real parent-child spans from envelope fields alone, without + retrofitting the envelope or touching emit sites. Cross-process correlation stays `requestId` + (already stamped on both sides of the RPC); a W3C `traceparent`-style request-meta field is + additive under ADR 0006 and deferred to whatever ADR introduces an exporter. No exporter is part + of this ADR. + +**Registered sinks:** + +- **debug stream sink** — today's debug-mode live ndjson writer (per-request file / `daemon.log` / + stderr fallback), unchanged behavior including the `liveWrittenEventCount` flush watermark; +- **session event log sink** — consumes the `session-lifecycle` trait kinds and writes today's + `events.ndjson` v1 entries, reusing the existing presentation builders (`buildActionSummary`, + `buildRequestSuccessEventPresentation`) and write queue; receives events from request scopes and + teardown scopes alike; +- **replay trace sink** — consumes `replay-trace` trait kinds, maps internal kinds to the legacy + `type` values, writes the per-attempt file via bound routing context; replaces both existing + append helpers; +- **agent-cost tally** — already a projection (`phaseCounts`); now reads its kind set from the + `runner-round-trip` trait. + +The request-lifecycle points that today call `sessionStore.recordEvent`/`appendActionEvent` +directly (`request-execution-scope.ts`, `request-router.ts`, `session-store.ts`) instead `emit` +the corresponding journal kinds; the sink owns the file. `SessionAction` recording itself is +untouched: `recordActionEntry` still pushes to `session.actions` first, and the `action.recorded` +journal event remains a projection of that push. + +### 3. Progress streaming stays a separate, transport-owned channel + +Progress is an **output port of the transport**, not telemetry: it has ordering and +interleaving guarantees relative to the final response on the same stream, client-disconnect +tracking that feeds request cancellation, a closed typed union (`RequestProgressEvent`) consumed +directly by the wire serializer, and a single-sink `AsyncLocalStorage` installed by the transport +outside `handleRequest`. Forcing it through the journal would either weaken its payload type to +`Record` at the wire boundary or make the journal envelope part of the RPC +protocol — both worse than the status quo. It also writes unredacted today, so journal-boundary +redaction would silently alter wire bytes. + +`src/request/progress.ts`, `request-progress-protocol.ts`, and both transports are therefore +unchanged by this ADR. If a future feature needs progress events in the journal (e.g. a request +timeline), the correct shape is a **mirror emit** — the progress emitter additionally emitting a +cataloged journal kind — never rerouting the wire path through the journal. That is a separate, +opt-in decision. + +### 4. First planned consumer: the usage sink (privacy by construction) + +The motivating consumer for this architecture is opt-in **usage analytics over agent behavior**: +which commands run, how often, what fails with which error codes, and outcome sequences that trip +agents (consecutive full snapshots, screenshot-after-snapshot, repeated same-command failures). +The unit of observation is request **metadata**, never request **content**. + +The privacy rule is therefore **allowlist by construction, not redaction**. Redaction is a +blocklist over rich data and one missed field leaks a selector or label; the usage sink instead +projects `session-lifecycle` events into a closed `UsageRecord` schema in which every field draws +from an enumerated vocabulary: + +- `command`/action — enumerated by the CommandDescriptor registry; +- outcome — `ok` or the typed error **code** (ADR 0010's closed set); never the error message; +- `durationMs`, cost fields (`runnerRoundTrips`, `nodeCount`), `platform`/`appleOs`; +- flag **names** present on the request — never flag values; +- a hashed session identity plus per-session sequence number, so command bigrams are computable + downstream. + +Positionals, selectors, labels, fill text, snapshot content, and error messages are unrepresentable +in the schema — there is nothing to scrub. Sequence/anti-pattern detection is downstream analysis +over the record stream (a local stats read first; opt-in export later), never emission-side logic. +The sink itself is a follow-up landing after migration step 3 (it consumes the +`session-lifecycle` kinds); this ADR fixes only its schema discipline so no richer interim export +gets built. + +## Invariants + +- **Byte compatibility, one declared exception.** `events.ndjson` v1, per-request diagnostics + ndjson, `replay-timing.ndjson` shapes and legacy `type` discriminators, and + `cost.runnerRoundTrips` are produced identically by the sinks; golden fixtures under + `contracts/fixtures/` pin each format. The declared exception: replay-timing events from the + previously unredacted helper become redacted (URL query stripping, sensitive-key masking, long + string truncation may alter those lines). This is an intentional security fix shipped as such — + changelog-visible, fixture-updated — not an unnoticed side effect. +- **Single redaction boundary for journal-owned outputs.** `redactDiagnosticData` runs once, on + append. The flush path's second whole-entry pass is retained only until sinks own their writes, + then removed. No sink may write an event that did not pass through the journal. (Progress is not + journal-owned and keeps its current unredacted wire behavior.) +- **Emitters never branch on consumers.** An emit site must not know or care whether debug mode or + a replay trace is active. Anything consumer-conditional lives in the sink. +- **No state from events.** The journal is write-only telemetry. Recovering session state, replay + provenance, or repair watermarks from journal events is out of scope and rejected (see + Alternatives). +- **Traits over names.** No production code may select events by matching kind-name substrings + (`startsWith('ios_runner_')` etc.); a lint-style test enforces this for the derived-set helpers. + +## Consequences + +- Traceability becomes "filter the journal by `requestId`" (or by `session` for teardown scopes): + one envelope, one ordering, across what are today three telemetry files with different envelope + shapes. Progress remains separately observable on the wire, as an operational channel should be. +- New observability features (a timeline command, an OTel exporter, a perf-phase report) are new + sinks — no new emit sites, no new vocabularies. +- Adding an event kind is a catalog entry plus emit sites; forgetting the catalog is a compile + error. Renaming an internal kind is invisible on the wire when the owning sink maps it; changing + a **wire** name remains a breaking change for log consumers and needs the same care as today. +- The dead `traceLogPath` option, the duplicate redaction pass, and the unredacted replay-timing + helper are deleted; replay-timing redaction is the one visible output change. +- Cost: a mechanical, wide migration (~70 files import the typed kind; the string values + themselves do not change, so diffs are type-level). Runtime overhead is one synchronous + sink-dispatch loop per event over today's buffer push — negligible against device I/O. + +## Alternatives considered + +- **Absorb progress streaming into the journal** (the first draft's step 5): rejected after + review. Progress is a transport-owned output port with ordering, cancellation, and closed-union + type-safety requirements; rerouting it through a generic journal weakens a protocol contract to + gain nothing operationally. Mirror emits remain available if journal visibility is ever needed. +- **One closed discriminated union with typed payloads per kind:** rejected for this ADR — 155 + kinds × payload schemas is a large, low-yield migration, and payload types can be added per-kind + incrementally once the catalog exists. The catalog is the enforcement point; payload typing is a + follow-up, not a prerequisite. (Progress keeps its typed union precisely because it stays on its + own channel.) +- **`EventEmitter`/pub-sub bus:** rejected. Implicit subscription defeats both goals: deletability + (who listens is not statically visible) and traceability (causality disappears into `.on()`). + The codebase currently has zero production `EventEmitter` usage; this design keeps it that way. +- **Event sourcing (rebuild session state from the journal):** rejected. Session state has owners + (`SessionStore`, ref frames, repair transactions) with their own invariants and gates; deriving + it from telemetry would couple correctness to an observability channel and is a large behavioral + rewrite with no user-facing benefit. +- **A process-global fallback scope instead of explicit teardown scopes:** rejected — an ambient + catch-all scope would make "which scope did this event land in" environment-dependent and hide + missing-scope bugs; teardown entry points are few and known, so explicit scopes are cheap. +- **Leave the channels, just type the phases:** considered as a stopping point — step 1 alone + fixes the stringly agent-cost coupling — but it leaves the duplicate envelopes, the redaction + inconsistency, and three write paths. Kept as the migration's first independently useful step + instead. +- **Merge `upload-progress` and `app-events` in:** rejected. Upload progress is a local + byte-counter callback that never crosses the request scope; `core/app-events.ts` is a deep-link + builder for the `trigger-app-event` command, not an event channel. + +## Validation required for implementation + +- Type-level completeness: an emit with an uncataloged kind fails to compile. Orphan detection + (declared-but-never-emitted kinds) is **static**: a source-scan check in the layering-lint style + proves every catalog kind has at least one emit reference; runtime observation is explicitly not + the mechanism (it conflates untested branches with dead declarations). +- Golden byte-compatibility fixtures for `events.ndjson`, the per-request diagnostics file, and + `replay-timing.ndjson` — the latter's fixture updated once, in the same change that declares the + redaction fix. +- Out-of-request coverage: an idle-reap/shutdown `finalizeRepairTeardown` still lands its + synthesized `close` as an `action.recorded` entry in `events.ndjson`, via a teardown scope, with + no request active. +- Per-attempt routing: a multi-attempt `test` run writes each attempt's trace to its own file with + legacy `type` values; trace-kind events emitted with no bound destination are dropped, not + misrouted. A **concurrent nested-action regression** runs sharded attempts in parallel — each + performing nested dispatch that rebinds `session`/`logPath` via `createRequestExecutionScope` — + and proves per-fork isolation across **all three routed outputs**: each `replay-timing.ndjson`, + each per-request diagnostics ndjson, and each session's `events.ndjson` contain only their own + shard's events — no cross-writes, no drops — including events emitted after `await` points. +- Sink isolation: a sink that throws does not affect the buffer, other sinks, or the response; + ordering across sinks is registration order. +- `cost.runnerRoundTrips` parity: the trait-derived set equals the current literal list; the + existing `request-router-cost` test keeps passing unchanged. +- Redaction: a fixture proves a sensitive value emitted in `data` never reaches any journal-owned + sink output, including the replay timing trace. +- Usage schema gate: a type/test check proves every `UsageRecord` field's vocabulary source — a + field is either numeric, an enum imported from the owning registry (command names, error codes, + flag keys), or a hash — and that no open-string field exists; adding one is a failing gate, not + a review comment. +- Layering: `scripts/layering/check.ts` stays green — catalog in `contracts`, runtime in `utils`, + no new back-edges. + +## Migration plan + +Each step lands green and independently useful: + +1. **Catalog + typed kinds** — introduce `EVENT_CATALOG`, narrow `emitDiagnostic`'s `phase` to + `EventKind`, migrate all emit sites mechanically (string values unchanged), derive + `RUNNER_ROUND_TRIP_PHASES` from the `runner-round-trip` trait with a parity test, add the + static orphan check. No runtime behavior change. +2. **Sink seam** — restructure `diagnostics.ts` so the scope owns an explicit sink list with the + ordering/isolation/flush contract; move the debug stream and agent-cost tally behind it; delete + `traceLogPath`; collapse to a single redaction pass. Byte-compat fixtures land here. +3. **Session event log as sink + teardown scopes** — request-lifecycle emit points route through + the journal; the `events.ndjson` writer becomes a sink; idle-reap/shutdown finalizers open + session-scoped teardown scopes so out-of-request `action.recorded` events keep flowing; the + `events` command and pagination untouched. +4. **Replay trace as sink** — the journal fork primitive lands with its concurrent-shard + regression; both trace helpers are replaced by one sink with fork-bound per-attempt routing and + legacy `type` mapping; the declared redaction change ships here with its fixture and changelog + entry. diff --git a/docs/adr/README.md b/docs/adr/README.md index 2b7a1ce1d7..c947daf888 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -22,3 +22,15 @@ ADRs record *why*; the registries and gates they describe are the living source of truth — when prose and a registry disagree, the registry wins and the ADR needs a follow-up. + +Shape conventions, so consulting an ADR stays cheap: + +- **Normative rules first, terse.** Status, then a "Rules at a glance" summary a reader can stop + after; full contracts and rationale below it. +- **Rationale and refuted alternatives stay in the ADR** — they are what stops re-litigating + settled ideas — but below the fold. +- **Process history is deleted once complete, not archived in-file.** Migration plans, per-step + landing tables, and point-in-time status change-logs go to git history; the Status section keeps + one line saying so plus any accepted, still-relevant waiver or evidence gap. +- Once a rule is gate-enforced, the ADR keeps the why and points at the gate rather than restating + the rule's details.