From 6e5a41d726615b7fe50cc6fa8e06eb864f93f149 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 24 Jul 2026 20:32:55 +0200 Subject: [PATCH 1/7] docs(adr): rules-first restructure of 0012/0014/0016, drop completed migration logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0012 alone was 42% of the ADR corpus by bytes; consulting it cost ~28k tokens of mostly process history. Restructure per the new shape convention (added to the ADR README): Status + a normative 'Rules at a glance' first so a reader can stop after ~50 lines, rationale and refuted alternatives kept below the fold, and completed migration plans/landing tables deleted — git history is the archive. - 0012: delete migration plan/progress; fix the Status section that still claimed #1235 unimplemented against its own landing table; demote the 2026-07-10 evidence audit to the end (still cited by the decisions). - 0014: same; the accepted Android blocking-dialog-recovery evidence gap and its covering fixture tests move into Status so the waiver survives. - 0016: verified implemented; rules summary added (nothing was history). No rule's meaning changed; edits are reorganization plus stale-status fixes. --- docs/adr/0012-interactive-replay.md | 499 +++++++----------- docs/adr/0014-session-ref-frame-lifetime.md | 80 ++- .../0016-active-session-script-publication.md | 25 +- docs/adr/README.md | 12 + 4 files changed, 270 insertions(+), 346 deletions(-) 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..ddd6b85d20 100644 --- a/docs/adr/0016-active-session-script-publication.md +++ b/docs/adr/0016-active-session-script-publication.md @@ -2,7 +2,30 @@ ## 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. +- Literal `fill` inputs persist into the artifact: do not record secret-bearing journeys until + #1348's parameterized inputs ship. ## Context 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. From 83200d231edc59bdaa67cc436c9079e4061371c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 24 Jul 2026 20:32:55 +0200 Subject: [PATCH 2/7] =?UTF-8?q?docs(adr):=20propose=20ADR=200017=20?= =?UTF-8?q?=E2=80=94=20unified=20request=20event=20journal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply ADR 0008's registry thesis to events. Inventory (2026-07-24) found four parallel event vocabularies — ~155 stringly-typed diagnostics phases, the session events.ndjson, the progress wire stream, and the replay timing trace (one of its two writers unredacted) — with consumers coupled to emit sites by string: agent-cost counts runner round-trips by matching two phase names. Proposal: an EVENT_CATALOG in contracts making every kind a typed, trait-carrying declaration; the diagnostics scope becomes the single journal append point; every consumer becomes an explicitly registered sink; all existing file/wire formats stay byte-compatible behind golden fixtures. Explicitly rejects pub-sub and event sourcing. Status: Proposed — not indexed in the ADR README until accepted. --- docs/adr/0017-unified-event-journal.md | 224 +++++++++++++++++++++++++ 1 file changed, 224 insertions(+) create mode 100644 docs/adr/0017-unified-event-journal.md diff --git a/docs/adr/0017-unified-event-journal.md b/docs/adr/0017-unified-event-journal.md new file mode 100644 index 0000000000..664786e4e6 --- /dev/null +++ b/docs/adr/0017-unified-event-journal.md @@ -0,0 +1,224 @@ +# ADR 0017: Unified Request Event Journal + +## Status + +Proposed (2026-07-24). Draft for review; nothing here is implemented. + +## 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. +- One **journal** per request scope is the single append point (`emitDiagnostic` evolves into it). + Emitters state facts; they never know who is listening. There is no `EventEmitter`, no dynamic + subscription. +- Every consumer is a **sink**: an explicitly registered, statically enumerable projection of the + journal (debug ndjson, session event log, progress wire stream, replay timing trace, agent-cost + tally). Sinks filter by kind/trait and own their output format. +- **Wire and file formats do not change.** `events.ndjson` v1 entries, progress envelopes, the + `replay-timing.ndjson` shape, `cost.runnerRoundTrips`, and per-request + `sessions//requests/.ndjson` stay byte-compatible; golden fixtures gate this. +- **Redaction happens once, at the journal boundary.** Every event is redacted on append; sinks + write pre-redacted events and add no second pass. +- 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. + +## 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 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. +3. **Progress streaming** (`src/request/progress.ts`, `src/daemon/request-progress-protocol.ts`). + A typed `RequestProgressEvent` union emitted through a single-sink `AsyncLocalStorage`, + serialized as ndjson envelopes multiplexed onto the same socket/HTTP response stream as the + final response. Opt-in per request via `meta.requestProgress`; disabled under `--json`. +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` with `replay_action_start/stop` and attempt-lifecycle events — 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 (a request start is an event in at least two of them) 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*. + +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. + +## 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 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', 'progress', 'session-lifecycle' +}; + +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 follow-up, not a +requirement — see Alternatives). + +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 + +The diagnostics scope becomes the **request journal**: the single append point for every event in +the four vocabularies. `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. + +Sinks are constructed with the scope (daemon request entry, CLI entry, fatal scope) and listed in +one place per entry point — statically enumerable, greppable, no dynamic subscription API: + +- **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 + (`request.started`/`request.finished`/`action.recorded`) and writes today's `events.ndjson` v1 + entries, reusing the existing presentation builders (`buildActionSummary`, + `buildRequestSuccessEventPresentation`) and per-path write queue; +- **progress sink** — consumes `progress`-trait kinds and forwards to the existing wire envelope + serializer when `meta.requestProgress` opted in; the `RequestProgressEvent` union becomes the + `data` payload of those kinds, and `src/request/progress.ts`'s separate `AsyncLocalStorage` + is retired in favor of the journal scope; +- **replay trace sink** — consumes replay-trace kinds and writes the per-attempt + `replay-timing.ndjson`, replacing both existing append helpers (closing the unredacted + `appendReplayTestTimingEvent` gap for free); +- **agent-cost tally** — already a projection (`phaseCounts`); now reads its kind set from the + `runner-round-trip` trait. + +The 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. Invariants + +- **Byte compatibility.** Every existing output — `events.ndjson` v1, progress envelopes and their + transport framing, `replay-timing.ndjson` event shapes, per-request diagnostics ndjson, + `cost.runnerRoundTrips` — is produced identically by the sinks. Golden fixtures under + `contracts/fixtures/` pin each format (the parity-table pattern). +- **Single redaction boundary.** `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. +- **Emitters never branch on consumers.** An emit site must not know or care whether debug mode, + progress streaming, 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`": one envelope, one ordering, across what + are today four files with three envelope shapes. +- 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 a kind is a visible, reviewable catalog diff (kinds are wire-visible in debug + ndjson, so renames are breaking for log consumers and need the same care as today). +- The dead `traceLogPath` option, the duplicate redaction pass, and the unredacted replay-timing + helper are deleted. +- 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 sink-dispatch loop + per event over today's buffer push — negligible against device I/O. + +## Alternatives considered + +- **One closed discriminated union with typed payloads per kind (full event-sourcing typing):** + 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. +- **`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. +- **Leave the four 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 four 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. Neither shares the journal's + envelope or consumers. + +## Validation required for implementation + +- Type-level completeness: an emit with an uncataloged kind fails to compile; a test enumerates the + catalog against the set of kinds actually emitted in a full unit-suite run and fails on + uncataloged or orphaned (declared-but-never-emitted) kinds, with an explicit allowlist for + rare-path kinds. +- Golden byte-compatibility fixtures for `events.ndjson`, progress envelopes (socket-legacy and + ndjson-envelope framings), `replay-timing.ndjson`, and the per-request diagnostics file. +- `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 sink output, + including the replay timing trace (the currently unredacted path). +- Progress: `replay-test` and `command` progress render identically through the journal path + (reporter/runtime tests unchanged); `--json` mode still emits no progress. +- 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. No runtime + behavior change. +2. **Sink seam** — restructure `diagnostics.ts` so the scope owns an explicit sink list; 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** — lifecycle emit points route through the journal; the + `events.ndjson` writer becomes a sink; `events` command and pagination untouched. +4. **Replay trace as sink** — both trace helpers replaced by one sink; unredacted path closed. +5. **Progress as sink** — retire `src/request/progress.ts`'s separate storage; progress kinds and + the wire serializer move behind the journal; transports unchanged. From ed82b74dfbf429b0c64554911ff97c9a7529b776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 24 Jul 2026 20:39:12 +0200 Subject: [PATCH 3/7] docs(adr): revise ADR 0017 per architecture review Address all five review findings and adopt both requested judgments: - P1 out-of-request events: finalizeRepairTeardown records a synthesized close during idle-reap/daemon-shutdown with no live request; a request-scoped-only journal would silently drop it. Added an explicit session-scoped teardown scope model (fatal-scope precedent) and rejected the ambient-fallback alternative. - P1 redaction vs byte-compat: progress stays unredacted on its own channel; the replay-trace unredacted->redacted change is now a declared, intentional compatibility change with its own fixture update, not smuggled under a byte-compat claim. - P1 per-attempt trace routing: sinks with dynamic destinations read scope-bound routing context (logPath-rebind precedent); drop-when-unbound semantics; sink ordering/isolation/flush contract made normative. - P2 progress typing: progress streaming removed from the journal entirely - it is a transport-owned output port (ordering, disconnect-as-cancellation, closed typed union); mirror emits noted as the future opt-in shape. - P2 completeness check: orphan detection is now a static source scan in the layering-lint style; runtime unit-suite observation explicitly rejected. Also per review: catalog keys are internal identities; sinks map to legacy wire discriminators, which are never automatically canonical. Migration plan reduced to 4 steps. --- docs/adr/0017-unified-event-journal.md | 296 ++++++++++++++++--------- 1 file changed, 192 insertions(+), 104 deletions(-) diff --git a/docs/adr/0017-unified-event-journal.md b/docs/adr/0017-unified-event-journal.md index 664786e4e6..30e74d1d16 100644 --- a/docs/adr/0017-unified-event-journal.md +++ b/docs/adr/0017-unified-event-journal.md @@ -2,7 +2,13 @@ ## Status -Proposed (2026-07-24). Draft for review; nothing here is implemented. +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 @@ -12,17 +18,27 @@ Normative summary of the proposal; contracts and rationale below. 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. -- One **journal** per request scope is the single append point (`emitDiagnostic` evolves into it). - Emitters state facts; they never know who is listening. There is no `EventEmitter`, no dynamic +- 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**: an explicitly registered, statically enumerable projection of the - journal (debug ndjson, session event log, progress wire stream, replay timing trace, agent-cost - tally). Sinks filter by kind/trait and own their output format. -- **Wire and file formats do not change.** `events.ndjson` v1 entries, progress envelopes, the - `replay-timing.ndjson` shape, `cost.runnerRoundTrips`, and per-request - `sessions//requests/.ndjson` stay byte-compatible; golden fixtures gate this. -- **Redaction happens once, at the journal boundary.** Every event is redacted on append; sinks - write pre-redacted events and add no second pass. +- 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). Sinks with dynamic destinations (the per-attempt replay trace) read + routing context that the owning lifecycle binds onto the scope, mirroring today's mid-request + `logPath` rebinding. +- **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. @@ -39,40 +55,44 @@ redaction discipline, and sink (inventoried 2026-07-24): `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 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. + 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 typed `RequestProgressEvent` union emitted through a single-sink `AsyncLocalStorage`, - serialized as ndjson envelopes multiplexed onto the same socket/HTTP response stream as the - final response. Opt-in per request via `meta.requestProgress`; disabled under `--json`. + 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` with `replay_action_start/stop` and attempt-lifecycle events — written by - **two different helpers, one of which redacts and one of which does not** + `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 (a request start is an event in at least two of them) 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 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*. 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. +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 phase strings (no renames in this ADR): +kind, keyed by today's diagnostics phase strings (no renames in this ADR): ```ts type EventDescriptor = { @@ -81,7 +101,7 @@ type EventDescriptor = { | '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', 'progress', 'session-lifecycle' + traits?: readonly EventTrait[]; // e.g. 'runner-round-trip', 'session-lifecycle', 'replay-trace' }; type EventKind = keyof typeof EVENT_CATALOG; @@ -90,8 +110,14 @@ 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 follow-up, not a -requirement — see Alternatives). +`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 @@ -103,49 +129,94 @@ The catalog lives in `contracts` (ranked, kernel-adjacent) so every zone — inc 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 - -The diagnostics scope becomes the **request journal**: the single append point for every event in -the four vocabularies. `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. - -Sinks are constructed with the scope (daemon request entry, CLI entry, fatal scope) and listed in -one place per entry point — statically enumerable, greppable, no dynamic subscription API: +### 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. +- **Dynamic destinations:** a sink whose output path is not known at scope construction reads + routing context bound onto the scope by the owning lifecycle. Precedent: + `createRequestExecutionScope` already rebinds `logPath` mid-request. The replay trace sink works + the same way: `prepareReplayTestTimingTrace` binds the current attempt's trace path onto the + scope (and clears it at attempt end); the sink drops trace-kind events while no destination is + bound. Multiple attempts in one request therefore route to their own files, exactly as today. + +**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 - (`request.started`/`request.finished`/`action.recorded`) and writes today's `events.ndjson` v1 - entries, reusing the existing presentation builders (`buildActionSummary`, - `buildRequestSuccessEventPresentation`) and per-path write queue; -- **progress sink** — consumes `progress`-trait kinds and forwards to the existing wire envelope - serializer when `meta.requestProgress` opted in; the `RequestProgressEvent` union becomes the - `data` payload of those kinds, and `src/request/progress.ts`'s separate `AsyncLocalStorage` - is retired in favor of the journal scope; -- **replay trace sink** — consumes replay-trace kinds and writes the per-attempt - `replay-timing.ndjson`, replacing both existing append helpers (closing the unredacted - `appendReplayTestTimingEvent` gap for free); +- **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 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. Invariants - -- **Byte compatibility.** Every existing output — `events.ndjson` v1, progress envelopes and their - transport framing, `replay-timing.ndjson` event shapes, per-request diagnostics ndjson, - `cost.runnerRoundTrips` — is produced identically by the sinks. Golden fixtures under - `contracts/fixtures/` pin each format (the parity-table pattern). -- **Single redaction boundary.** `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. -- **Emitters never branch on consumers.** An emit site must not know or care whether debug mode, - progress streaming, or a replay trace is active. Anything consumer-conditional lives in the sink. +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. + +## 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). @@ -154,25 +225,31 @@ event remains a projection of that push. ## Consequences -- Traceability becomes "filter the journal by `requestId`": one envelope, one ordering, across what - are today four files with three envelope shapes. +- 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 a kind is a visible, reviewable catalog diff (kinds are wire-visible in debug - ndjson, so renames are breaking for log consumers and need the same care as today). + 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. + 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 sink-dispatch loop - per event over today's buffer push — negligible against device I/O. + 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 -- **One closed discriminated union with typed payloads per kind (full event-sourcing typing):** - 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. +- **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. @@ -180,29 +257,38 @@ event remains a projection of that push. (`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. -- **Leave the four channels, just type the phases:** considered as a stopping point — step 1 alone +- **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 four write paths. Kept as the migration's first independently useful step + 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. Neither shares the journal's - envelope or consumers. + 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; a test enumerates the - catalog against the set of kinds actually emitted in a full unit-suite run and fails on - uncataloged or orphaned (declared-but-never-emitted) kinds, with an explicit allowlist for - rare-path kinds. -- Golden byte-compatibility fixtures for `events.ndjson`, progress envelopes (socket-legacy and - ndjson-envelope framings), `replay-timing.ndjson`, and the per-request diagnostics file. +- 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. +- 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 sink output, - including the replay timing trace (the currently unredacted path). -- Progress: `replay-test` and `command` progress render identically through the journal path - (reporter/runtime tests unchanged); `--json` mode still emits no progress. +- Redaction: a fixture proves a sensitive value emitted in `data` never reaches any journal-owned + sink output, including the replay timing trace. - Layering: `scripts/layering/check.ts` stays green — catalog in `contracts`, runtime in `utils`, no new back-edges. @@ -212,13 +298,15 @@ 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. No runtime - behavior change. -2. **Sink seam** — restructure `diagnostics.ts` so the scope owns an explicit sink list; 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** — lifecycle emit points route through the journal; the - `events.ndjson` writer becomes a sink; `events` command and pagination untouched. -4. **Replay trace as sink** — both trace helpers replaced by one sink; unredacted path closed. -5. **Progress as sink** — retire `src/request/progress.ts`'s separate storage; progress kinds and - the wire serializer move behind the journal; transports unchanged. + `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** — both trace helpers replaced by one sink with scope-bound per-attempt + routing and legacy `type` mapping; the declared redaction change ships here with its fixture and + changelog entry. From a3dba71b389da3716559bf48926f242d19cbb3ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 25 Jul 2026 11:40:28 +0200 Subject: [PATCH 4/7] =?UTF-8?q?docs(adr):=20ADR=200017=20=E2=80=94=20fork,?= =?UTF-8?q?=20never=20rebind,=20for=20per-attempt=20trace=20routing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found a blocking concurrency flaw in the revised routing design: sharded test attempts run concurrently (Promise.allSettled in runReplayTestShards) under one inherited AsyncLocalStorage request scope, so mutable scope rebinding would let one attempt overwrite or clear another's replay-timing destination after an await — cross-writing or dropping events. Replace rebinding with a journal fork primitive: journal.fork(bindings, fn) runs fn in a new ALS scope object sharing the parent's buffer/phaseCounts/ envelope/sinks but carrying frozen routing bindings. Each attempt wraps its work (including nested replay dispatch) in a fork binding its own trace path; the binding dies with the fork, so no clearing step exists to race. Existing updateDiagnosticsScope rebinds stay confined to sequential request setup, pre-fan-out. Validation gains a concurrent-shard regression proving each replay-timing.ndjson contains only its own attempt's events. --- docs/adr/0017-unified-event-journal.md | 35 +++++++++++++++++--------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/docs/adr/0017-unified-event-journal.md b/docs/adr/0017-unified-event-journal.md index 30e74d1d16..a0136a6892 100644 --- a/docs/adr/0017-unified-event-journal.md +++ b/docs/adr/0017-unified-event-journal.md @@ -29,8 +29,8 @@ Normative summary of the proposal; contracts and rationale below. - 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). Sinks with dynamic destinations (the per-attempt replay trace) read - routing context that the owning lifecycle binds onto the scope, mirroring today's mid-request - `logPath` rebinding. + **immutable routing bindings carried by a forked child scope** — concurrent sub-work (sharded + test attempts run under `Promise.allSettled`) forks, never rebinds shared scope state. - **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, @@ -158,12 +158,20 @@ statically enumerable, greppable, no dynamic subscription API. Semantics, normat 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. -- **Dynamic destinations:** a sink whose output path is not known at scope construction reads - routing context bound onto the scope by the owning lifecycle. Precedent: - `createRequestExecutionScope` already rebinds `logPath` mid-request. The replay trace sink works - the same way: `prepareReplayTestTimingTrace` binds the current attempt's trace path onto the - scope (and clears it at attempt end); the sink drops trace-kind events while no destination is - bound. Multiple attempts in one request therefore route to their own files, exactly as today. +- **Dynamic destinations — fork, never rebind:** a sink whose output path is not known at scope + construction reads **immutable routing bindings** carried by the current scope. The journal + gains a fork primitive: `journal.fork(bindings, fn)` runs `fn` inside a new AsyncLocalStorage + scope object that shares the parent's buffer, `phaseCounts`, envelope, and sink list, but + carries its own **frozen** bindings. Sharded `test` runs execute attempts concurrently + (`Promise.allSettled` in `runReplayTestShards`, `src/daemon/handlers/session-test.ts`) under one + inherited request scope, so mutating shared scope state to route traces would let one attempt + overwrite or clear another's destination after an `await`. Instead, each attempt's lifecycle + wraps its work — including all nested replay dispatch — in a fork binding that attempt's + `replay-timing.ndjson` path; the binding dies with the fork, so no clearing step exists to race. + The sink drops trace-kind events emitted with no bound destination. The existing + `updateDiagnosticsScope` rebinds (`session`, `logPath` in `createRequestExecutionScope`) remain: + they run once during sequential request setup, before any concurrency fan-out; new routing + context for parallel or attempt-scoped work must use forks. **Registered sinks:** @@ -282,7 +290,9 @@ opt-in decision. 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. + misrouted. A **concurrent-shard regression** runs sharded attempts in parallel and proves each + `replay-timing.ndjson` contains only its own attempt's events — no cross-writes, no drops — + including events emitted from nested replay work 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 @@ -307,6 +317,7 @@ Each step lands green and independently useful: 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** — both trace helpers replaced by one sink with scope-bound per-attempt - routing and legacy `type` mapping; the declared redaction change ships here with its fixture and - changelog entry. +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. From dd5d36e296309371271917c182de4789d35935fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 25 Jul 2026 12:07:49 +0200 Subject: [PATCH 5/7] =?UTF-8?q?docs(adr):=20ADR=200017=20=E2=80=94=20scope?= =?UTF-8?q?=20identity=20on=20the=20envelope=20for=20future=20exporter=20s?= =?UTF-8?q?inks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reserve the one shape decision an OTel-style exporter would otherwise force a retrofit for: every scope (request, teardown, fork) carries scopeId, forks record parentScopeId, both ride the event envelope. Forks already form a tree, so an exporter sink can emit parent-child spans from envelope fields alone. Cross-process correlation stays requestId; a traceparent-style meta field is additive under ADR 0006 and deferred. No exporter in this ADR. --- docs/adr/0017-unified-event-journal.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/adr/0017-unified-event-journal.md b/docs/adr/0017-unified-event-journal.md index a0136a6892..e501ab31de 100644 --- a/docs/adr/0017-unified-event-journal.md +++ b/docs/adr/0017-unified-event-journal.md @@ -172,6 +172,14 @@ statically enumerable, greppable, no dynamic subscription API. Semantics, normat `updateDiagnosticsScope` rebinds (`session`, `logPath` in `createRequestExecutionScope`) remain: they run once during sequential request setup, before any concurrency fan-out; new routing context for parallel or attempt-scoped work must use forks. +- **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:** From 9c545e08dc00dbd76b82d60ac52f3a8e3e540b26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 25 Jul 2026 12:19:56 +0200 Subject: [PATCH 6/7] =?UTF-8?q?docs(adr):=20address=20re-review=20?= =?UTF-8?q?=E2=80=94=20renumber=20to=200018,=20full=20fork=20isolation,=20?= =?UTF-8?q?0016=20record-as?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Renumber the proposal 0017 -> 0018: main now carries accepted ADR 0017 (parameterized recorded inputs, #1369); branch rebased onto it. 2. Fork contract strengthened: forks clone EVERY mutable scope field (envelope, logPath, routing bindings) and own their event buffer; only the sink list and the request-global phaseCounts tally are shared. Verified in code: nested dispatch creates child execution scopes (request-router.ts:257) whose updateDiagnosticsScope rebinds session/logPath mid-flight, so a shared mutable envelope would cross-route debug/session-log events between concurrent shards even with frozen trace bindings. updateDiagnosticsScope now specified as mutating only the innermost scope. The regression now covers all three routed outputs (replay-timing, per-request diagnostics ndjson, events.ndjson). 3. 0016 rules summary updated for shipped #1348: sensitive fills use fill --record-as (ADR 0017); unparameterized fill/type stays literal (body sections already updated by #1369's merge). --- .../0016-active-session-script-publication.md | 5 +- ...urnal.md => 0018-unified-event-journal.md} | 48 +++++++++++-------- 2 files changed, 30 insertions(+), 23 deletions(-) rename docs/adr/{0017-unified-event-journal.md => 0018-unified-event-journal.md} (89%) diff --git a/docs/adr/0016-active-session-script-publication.md b/docs/adr/0016-active-session-script-publication.md index ddd6b85d20..6a9d21f749 100644 --- a/docs/adr/0016-active-session-script-publication.md +++ b/docs/adr/0016-active-session-script-publication.md @@ -24,8 +24,9 @@ Normative summary; the binding contracts and refusal cases are in [Decision](#de 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. -- Literal `fill` inputs persist into the artifact: do not record secret-bearing journeys until - #1348's parameterized inputs ship. +- 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/0017-unified-event-journal.md b/docs/adr/0018-unified-event-journal.md similarity index 89% rename from docs/adr/0017-unified-event-journal.md rename to docs/adr/0018-unified-event-journal.md index e501ab31de..b46661d702 100644 --- a/docs/adr/0017-unified-event-journal.md +++ b/docs/adr/0018-unified-event-journal.md @@ -1,4 +1,4 @@ -# ADR 0017: Unified Request Event Journal +# ADR 0018: Unified Request Event Journal ## Status @@ -28,9 +28,10 @@ Normative summary of the proposal; contracts and rationale below. 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). Sinks with dynamic destinations (the per-attempt replay trace) read - **immutable routing bindings carried by a forked child scope** — concurrent sub-work (sharded - test attempts run under `Promise.allSettled`) forks, never rebinds shared scope state. + 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, @@ -158,20 +159,23 @@ statically enumerable, greppable, no dynamic subscription API. Semantics, normat 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. -- **Dynamic destinations — fork, never rebind:** a sink whose output path is not known at scope - construction reads **immutable routing bindings** carried by the current scope. The journal - gains a fork primitive: `journal.fork(bindings, fn)` runs `fn` inside a new AsyncLocalStorage - scope object that shares the parent's buffer, `phaseCounts`, envelope, and sink list, but - carries its own **frozen** bindings. Sharded `test` runs execute attempts concurrently - (`Promise.allSettled` in `runReplayTestShards`, `src/daemon/handlers/session-test.ts`) under one - inherited request scope, so mutating shared scope state to route traces would let one attempt - overwrite or clear another's destination after an `await`. Instead, each attempt's lifecycle - wraps its work — including all nested replay dispatch — in a fork binding that attempt's - `replay-timing.ndjson` path; the binding dies with the fork, so no clearing step exists to race. - The sink drops trace-kind events emitted with no bound destination. The existing - `updateDiagnosticsScope` rebinds (`session`, `logPath` in `createRequestExecutionScope`) remain: - they run once during sequential request setup, before any concurrency fan-out; new routing - context for parallel or attempt-scoped work must use forks. +- **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 @@ -298,9 +302,11 @@ opt-in decision. 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-shard regression** runs sharded attempts in parallel and proves each - `replay-timing.ndjson` contains only its own attempt's events — no cross-writes, no drops — - including events emitted from nested replay work after `await` points. + 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 From bfd2ded23f73d544c5e496f9ec131bfcb0160b2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 25 Jul 2026 12:33:19 +0200 Subject: [PATCH 7/7] =?UTF-8?q?docs(adr):=20ADR=200018=20=E2=80=94=20name?= =?UTF-8?q?=20the=20usage=20sink=20as=20first=20consumer,=20privacy=20by?= =?UTF-8?q?=20construction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The motivating consumer is opt-in usage analytics over agent behavior: command frequencies, typed failure codes, and outcome sequences that trip agents (consecutive snapshots, screenshot-after-snapshot). Decision 4 pins its discipline now, before any exporter exists: an allowlist-by-construction UsageRecord schema whose every field draws from a registry-enumerated vocabulary (command names, ADR 0010 error codes, flag names, durations, hashed session + sequence number) — positionals, selectors, labels, fill text, and error messages are unrepresentable by type, not redacted. Anti-pattern detection is downstream analysis over the stream, never emission-side logic; the sink itself is a follow-up after migration step 3. Adds the matching invariant and a schema gate to validation. --- docs/adr/0018-unified-event-journal.md | 38 ++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/docs/adr/0018-unified-event-journal.md b/docs/adr/0018-unified-event-journal.md index b46661d702..34f2233d8c 100644 --- a/docs/adr/0018-unified-event-journal.md +++ b/docs/adr/0018-unified-event-journal.md @@ -42,6 +42,10 @@ Normative summary of the proposal; contracts and rationale below. - **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 @@ -84,6 +88,10 @@ is `countDiagnosticEventsByPhase(['ios_runner_command_send', 'ios_runner_readine 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). @@ -222,6 +230,32 @@ timeline), the correct shape is a **mirror emit** — the progress emitter addit 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 @@ -313,6 +347,10 @@ opt-in decision. 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.