docs(adr): rules-first ADR restructure + ADR 0017 proposal (unified event journal)#1399
Merged
Merged
Conversation
Member
Author
|
Reviewed exact head 3a504a6. The rules-first ADR edits preserve the operative decisions, but ADR 0017 has one blocking concurrency issue. Its replay-trace sink proposes rebinding and clearing a per-attempt trace path on the request journal scope. Sharded tests run attempts concurrently via Promise.allSettled under the same inherited AsyncLocalStorage scope, and the existing rebinding API mutates that shared scope. One attempt can therefore overwrite or clear another attempt’s route after an await, cross-writing or dropping replay-timing events. Require an immutable/forked per-attempt async routing context, propagate it through nested replay work, and add a concurrent-shard regression proving each replay-timing.ndjson contains only its own attempt. CodeQL/Analyze checks are green. |
Member
Author
|
Re-reviewed exact head babf43a. The immutable per-attempt trace binding fixes the previously reported replay-trace path race, but the revision is not ready yet:
The five CodeQL checks are green; this remains a docs/architecture revision blocker, not a CI failure. |
…migration logs 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.
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.
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.
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.
…ter sinks 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.
… 0016 record-as 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 <VAR> (ADR 0017); unparameterized fill/type stays literal (body sections already updated by #1369's merge).
thymikee
force-pushed
the
claude/codebase-architecture-modularity-6e5af0
branch
from
July 25, 2026 10:20
40ed43e to
9c545e0
Compare
thymikee
added
the
ready-for-human
Member
Author
|
Re-reviewed exact head 9c545e0: the three prior blockers are addressed. The proposal is now ADR 0018 alongside accepted ADR 0017; forks clone every mutable envelope/log-path/routing/buffer field while sharing only sinks and request-wide phase counts; and the required concurrent regression now covers replay trace, diagnostics, and session-event routing through nested replay after awaits. ADR 0016 also correctly requires fill --record-as VAR for sensitive inputs and keeps plain fill/type literal. The PR is mergeable and all five reported CodeQL checks pass. Code/architecture review is clean; implementation remains responsible for the specified compatibility, redaction, and concurrency gates. |
…by construction 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.
|
thymikee
added a commit
that referenced
this pull request
Jul 25, 2026
* origin/main: docs(adr): rules-first ADR restructure + ADR 0017 proposal (unified event journal) (#1399) feat: add first-class Vega VVD TV support (#1396) fix(replay): preserve cwd scope for opened sessions (#1401) docs: restructure AGENTS.md and CONTEXT.md for progressive disclosure (#1402) fix(cli): compact stale device status (#1388) feat: add WebView accessibility lab (#1397) feat: parameterize sensitive recorded inputs (#1369) fix(daemon): keep an active replay session's daemon alive over the CLI path (#1390) # Conflicts: # docs/adr/0012-interactive-replay.md
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two related docs-only changes; no source or behavior changes.
1. ADR shape cleanup (0012 / 0014 / 0016 / README)
ADR 0012 alone was 42% of the ADR corpus (~28k tokens), mostly process history, and its Status section had drifted (it still claimed the #1235 repair-transaction lifecycle was unimplemented while its own landing table said Shipped). This applies a shape convention, now written into the ADR README:
No rule's meaning changed — edits are reorganization plus stale-status fixes. Verified: no external links target the deleted section anchors; source comments referencing "migration step N" all carry PR numbers so they stay resolvable.
2. ADR 0017 (Proposed): unified request event journal
A design draft for review, applying ADR 0008's registry thesis to events. A code 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 doesn't redact) — with consumers coupled to emit sites by string: agent-cost counts runner round-trips by matching two phase-name literals inrequest-router.ts.Proposal in one line: an
EVENT_CATALOGincontractsmakes every event kind a typed, trait-carrying declaration; the diagnostics scope becomes the single journal append point; every consumer (debug ndjson, session event log, progress stream, replay trace, agent-cost) becomes an explicitly registered sink; all existing file/wire formats stay byte-compatible behind golden fixtures. Explicitly rejected: EventEmitter pub-sub, event sourcing, and per-kind payload typing as a prerequisite.Review focus for 0017
src/request/progress.ts's separate AsyncLocalStorage into the journal) worth touching the transport path, or should progress stay a permanent second channel?0017 is deliberately not added to the ADR README index until accepted.