feat: parameterize sensitive recorded inputs - #1369
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Needs changes at
CI is green, but the current provider scenario is mocked. After fixes, provide live simulator/device evidence that a parameterized fill executes the literal while the response, session state/events, diagnostics, temp/target |
|
Addressed all three review findings in ff980e9:
Validation:
Live iOS simulator evidence: recorded a Settings Search fill with |
|
Re-review at |
|
Addressed the remaining blocker in d8f0071.
Validation: |
|
P1 — Arbitrary backend output can still retain the literal. At The new response/settle regressions are meaningful for delimiter-separated echoes, but they do not cover concatenated strings or keys. Please scrub every literal occurrence in untrusted backend/settle output and keys while preserving only explicitly structural provenance, then add production-route regressions for concatenated, prefix/suffix, and object-key echoes. Live evidence still does not exercise this leak path. |
|
Addressed the new P1 in 8af3931.
Validation: focused handler/recorder/replay tests 14/14, provider scenario 3/3, format, lint, typecheck, layering, fallow, MCP metadata, and build passed. The bounded full coverage run passed 541 files and 4,583/4,583 tests. |
|
Re-reviewed exact head
The prior non-whitespace concatenation/key leak is otherwise fixed, structural trust is path-specific, and current provider/handler regressions are meaningful for their covered inputs. All exact-head checks are green. No ready label. |
|
Addressed both P1 edge cases in 4a47b15.
Validation:
|
|
Re-review at Not ready to merge or label: GitHub reports the draft branch as |
4a47b15 to
97a9862
Compare
|
Rebased onto Live verification on the rebased head exposed one production-path gap: coordinate fills were serialized correctly ( Validation:
Live iOS simulator evidence at
|
|
Re-reviewed exact head |
thymikee
added
the
ready-for-human
97a9862 to
4c604bc
Compare
4c604bc to
7d7bb1d
Compare
|
… 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).
…vent journal) (#1399) * docs(adr): rules-first restructure of 0012/0014/0016, drop completed 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. * docs(adr): propose ADR 0017 — unified request event journal 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): 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): ADR 0017 — fork, never rebind, for per-attempt trace routing 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): ADR 0017 — scope identity on the envelope for future exporter 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. * docs(adr): address re-review — renumber to 0018, full fork isolation, 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). * docs(adr): ADR 0018 — name the usage sink as first consumer, privacy 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.
* 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
1 participant
Summary
fill --record-as <VAR>authoring contract across CLI, Node, and MCP surfaces${VAR}in session state, diagnostics, target evidence, and published.adscriptsCloses #1348
Validation
pnpm check:quickpnpm check:fallow --base origin/mainpnpm exec vitest run --project provider-integration --maxWorkers=4(144 passed)pnpm test:integration:node(15 passed, 6 live-only tests skipped)pnpm test:integration:progress:checkSkillGym was not completed locally because its external model runners were unavailable from the sandbox.