Add an implementation plan to address gaps and fixes for the ADR-0006 refactor

docs/roadmap/implementation.md

@@ -0,0 +1,403 @@
+# ADR-0006 Findings 1–3 Remediation Plan
+
+This document is the executable implementation plan for correcting Findings 1,
+2, and 3 from the post-implementation review of ADR-0006:
+
+1. unredacted resolved-pipeline data in the debug summary;
+2. collision-prone output run directories; and
+3. missing failure `run-report.json` artifacts.
+
+Finding 4—the broad loss of CLI and configuration regression coverage—is
+explicitly deferred. The focused regression tests required to prove these three
+fixes are in scope; restoring or redesigning the general test suites is not.
+
+The audience is an LLM coding agent. Implement the stages in order. Each stage
+must finish with a compiling, tested repository.
+
+## Execution Rules
+
+Before every stage:
+
+1. read [Development](../development.md), both documents under `docs/policy/`,
+   [ADR-0006](../adr/0006-separate-output-cache-and-debug-state.md), this plan,
+   and every stage-specific document listed below;
+2. inspect `git status` and preserve user changes;
+3. inspect the focused implementation and tests before editing; and
+4. confirm all earlier stages are complete.
+
+During every stage:
+
+- keep the CLI as the run-identity and physical-path composition root;
+- preserve durable logical output schemas, checkpoint identity and wire
+  compatibility, chunk-plan behavior, debug layout, exit codes, and current
+  configuration contracts unless this plan expressly changes them;
+- never allow a secondary debug persistence error to hide the primary command
+  error;
+- add focused tests for the changed behavior without expanding into Finding 4;
+  and
+- update a canonical current-behavior document only if the implemented contract
+  changes or its existing statement is inaccurate.
+
+At the end of every stage:
+
+1. run the focused tests listed for that stage;
+2. run `go test ./...`;
+3. run `go vet ./...`;
+4. run `go build ./cmd/notarius`;
+5. run `git diff --check`; and
+6. mark the stage complete here only after all checks pass.
+
+If repository reality conflicts with this plan, update the plan before making a
+materially different design choice. Do not silently weaken redaction, output
+exclusivity, or failure reporting.
+
+## Fixed Decisions
+
+### Redacted summary boundary
+
+- Every file under `summary/` is a redacted artifact. The fact that the adjacent
+  `trace/` may contain application data does not relax this boundary.
+- `resolved-pipeline.json` retains its current logical shape and useful module,
+  validator, digest, capability, and reference-provenance data, but all module
+  and validator option maps pass through the same recursive sensitive-key
+  redaction used by `effective-config.json`.
+- The debug-bundle writer must not accept an arbitrary resolved pipeline for a
+  method whose name promises redacted summary output. Introduce a narrow
+  payload interface, analogous to `RedactedSummaryPayload`, that can only be
+  satisfied by a producer responsible for redaction. `config.EffectiveConfig`
+  implements it and returns a deep-cloned, redacted
+  `pipeline.ResolvedPipeline`.
+- The CLI passes the effective configuration to that method, not
+  `effective.ResolvedPipeline` directly. Do not expose the private redaction
+  helper merely to keep the unsafe call shape.
+- Redaction must cover input, chunk, output, lane extract/merge/normalize,
+  resolved validator-chain bindings, deprecated lane validator bindings, and
+  recursively nested option maps and lists. Safe option values remain intact.
+- Reference contents remain excluded through the existing `json:"-"`
+  `ReferenceSet` contract. Reference provenance and configured paths may remain
+  in the summary.
+- Credentials and sensitive values must not be copied into errors while testing
+  this behavior.
+
+### Run identity and output allocation
+
+- The CLI owns one run ID before allocating debug or executing the pipeline.
+  Output, debug, manifests, metadata, stdout, and stderr use that same ID.
+- The default ID format becomes:
+
+  ```text
+  run-<started-at-unix-nanoseconds>-<32-lowercase-hex-characters>
+  ```
+
+  The suffix is 16 bytes from `crypto/rand`. Time preserves useful ordering;
+  randomness makes IDs collision-resistant across processes and hosts sharing a
+  root.
+- Add an injectable `RunIDGenerator func(time.Time) (string, error)` to
+  `internal/cli.Options`. The production default uses `crypto/rand.Reader`.
+  Tests inject deterministic valid IDs. Do not retain package-global clocks or
+  random sources for this path.
+- Generate the ID after configuration and CLI validation but before debug
+  allocation and pipeline resolution. A generation failure is a normal command
+  failure with exit code `1` and cannot have a debug bundle because no run
+  identity exists yet.
+- Change debug allocation to accept the CLI-generated run ID and `startedAt`.
+  It validates that the ID is one safe path component and creates that exact
+  bundle with `os.Mkdir`, failing on an existing entry. Debug no longer creates
+  a different ID from its own clock or retries by changing identity.
+- Durable output allocation is exclusive. Validate every logical output name
+  first, create the output parent as needed, then create
+  `<output-root>/<run-id>` with `os.Mkdir`, not `os.MkdirAll`. An existing run
+  directory is an error; Notarius never writes into it.
+- Nested logical output directories may still use `MkdirAll` after the exclusive
+  run directory is owned by the invocation. File writes remain atomic.
+- If output allocation or a later output write fails, retain any newly created
+  partial run directory for inspection. Never remove or overwrite an existing
+  run directory. Document this only if current Operations wording needs
+  clarification.
+- Keep output directory permissions and logical file permissions unchanged;
+  this remediation addresses identity and exclusivity, not the public output
+  permission contract.
+
+### Terminal debug reports
+
+- Every debug-enabled invocation whose bundle was successfully allocated makes
+  one best-effort attempt to write a terminal `run-report.json`, whether the
+  command succeeds or fails.
+- A failure report uses `Succeeded: false`. Populate fields only from state that
+  is known at the failure point:
+  - `run_id`, requested `pipeline_id`, and `debug_path` are always known after
+    bundle allocation;
+  - `output_path` is included once the effective output root and run ID are
+    known;
+  - output, rejection, warning, and validation fields are populated when a
+    `pipeline.RunOutput` is available; otherwise their zero values are retained.
+- `error.log` remains the human-readable primary error artifact. Do not add the
+  error string to `RunReport`; this avoids a second unstructured error surface
+  and preserves the existing report schema.
+- Introduce one CLI-owned run-state value that accumulates terminal-report data
+  as resolution and execution progress. Success and failure reporting both
+  derive `debugbundle.RunReport` from this value rather than constructing
+  unrelated literals.
+- Add a single summary terminalization operation that attempts
+  `run-report.json` and, on failure, `error.log`. On a successful command it
+  writes only the success report. On a failed command it writes the failure
+  report and error log, attempting both even if one write fails and joining
+  secondary persistence errors for stderr reporting.
+- Terminalization is invoked at most once. If the primary failure is itself a
+  summary persistence failure, do not recursively retry the failed operation;
+  make one terminalization attempt that excludes the artifact already known to
+  have failed. In particular, a failure to write `run-report.json` must never
+  call itself again.
+- Stderr prints the primary error first, then any terminalization error, then
+  the allocated debug path. Exit code remains `1`. A secondary error never
+  replaces or wraps away the primary error.
+- Runs without debug perform no summary work and retain current concise stderr
+  behavior.
+
+## Stage 1: Enforce redaction for resolved-pipeline summaries
+
+**Status:** Not started
+
+### Objective
+
+Close the summary credential leak and make the redacted-payload boundary
+difficult to bypass accidentally.
+
+### Read first
+
+- `internal/core/config/redaction.go`
+- `internal/core/config/effective_config.go`
+- `internal/core/debugbundle/summary.go`
+- the summary-writing section of `internal/cli/run.go`
+- `internal/core/config/v3_test.go`
+- `internal/cli/state_hardening_test.go`
+- [Architecture: State, Output, And Safety](../policy/architecture.md#state-output-and-safety)
+
+### Implement
+
+1. Add the resolved-pipeline redacted payload interface to `debugbundle` and
+   change `SummaryWriter.WriteResolvedPipeline` to accept it rather than `any`.
+2. Implement the interface on `config.EffectiveConfig` using the existing deep
+   clone/redaction path.
+3. Audit recursive option redaction. Extend it only where nested list/map values
+   can currently retain a value under a sensitive key.
+4. Change the CLI call to pass `effective` through the redacted interface.
+5. Keep `effective-config.json` behavior and `resolved-pipeline.json` logical
+   structure unchanged apart from required redaction.
+
+### Focused tests
+
+- Unit-test a resolved pipeline containing distinct sentinel secrets in every
+  module and validator binding listed under Fixed Decisions.
+- Include nested maps and lists, safe neighboring values, and materialized
+  reference content. Assert secrets and reference contents are absent, redaction
+  markers are present, and safe provenance remains.
+- Run a debug-enabled CLI invocation whose accepted fake module options include
+  sensitive keys. Inspect both `effective-config.json` and
+  `resolved-pipeline.json` and prove neither contains the sentinels.
+- Assert mutation of a redacted payload cannot alter the effective configuration
+  or resolved pipeline.
+
+Run at minimum:
+
+```sh
+go test ./internal/core/config ./internal/core/debugbundle ./internal/cli
+```
+
+### Exit criteria
+
+No arbitrary resolved pipeline can be passed to the redacted summary writer;
+all resolved bindings are recursively redacted; focused leak tests fail against
+the old implementation and pass against the new one; and all repository checks
+pass.
+
+## Stage 2: Make run identity collision-resistant and output exclusive
+
+**Status:** Not started
+
+### Objective
+
+Ensure separate invocations can never silently share or overwrite one durable
+output run directory.
+
+### Read first
+
+- run initialization and output writing in `internal/cli/run.go`
+- `internal/core/debugbundle/bundle.go`
+- `internal/core/debugbundle/bundle_test.go`
+- `internal/cli/state_surfaces_test.go`
+- `internal/cli/state_hardening_test.go`
+- [Operations: Output](../operations.md#output)
+
+### Implement
+
+1. Add the default cryptographic run-ID generator and inject it through
+   `cli.Options` as specified above. Validate generated IDs before any path use.
+2. Generate one ID from `startedAt` before debug allocation. Remove the debug
+   allocator's package-global clock and identity generation.
+3. Refactor `debugbundle.Allocate` to accept the run ID and start time and
+   exclusively create that exact bundle.
+4. Refactor output writing so the run directory is created exclusively after
+   all logical file names validate. Return a contextual collision error without
+   touching the existing directory.
+5. Preserve atomic per-file writes, partial-new-directory behavior on later
+   write failures, and the single ID across manifests and reported paths.
+6. Update existing focused tests to inject deterministic run IDs rather than
+   relying on timestamps or the debug allocator's global clock.
+
+### Focused tests
+
+- Verify the production generator's format and validate many generated IDs for
+  uniqueness and path safety without asserting random bytes.
+- Inject a deterministic ID and prove debug and output use the exact same run
+  directory name and manifest run ID.
+- Pre-create the target output run directory with sentinel files. Run the
+  command and assert exit `1`, a collision error, and byte-for-byte preservation
+  of the existing tree.
+- Execute two invocations with the same injected ID and root. The first may
+  succeed; the second must fail without changing the first output.
+- Pre-create the target debug bundle and assert allocation fails without
+  changing it.
+- Prove nested logical output paths still work and unsafe logical paths are
+  rejected before the run directory is created.
+- Prove a later output-file failure retains only the newly allocated partial
+  directory and never affects a sibling run.
+
+Run at minimum:
+
+```sh
+go test ./internal/core/debugbundle ./internal/cli
+```
+
+### Exit criteria
+
+Every invocation has one collision-resistant ID, output and debug allocation
+are exclusive, an existing run directory is never reused or overwritten, and
+all repository checks pass.
+
+## Stage 3: Write terminal run reports for failures
+
+**Status:** Not started
+
+### Objective
+
+Make every allocated debug summary record a structured terminal outcome while
+preserving primary errors and preventing recursive persistence failures.
+
+### Read first
+
+- `internal/core/debugbundle/summary.go`
+- failure and summary paths in `internal/cli/run.go`
+- `internal/cli/state_hardening_test.go`
+- [Run State Internals](../internal/state.md)
+- [Operations: Debug Bundles](../operations.md#debug-bundles)
+- [Operations: Failures And Warnings](../operations.md#failures-and-warnings)
+
+### Implement
+
+1. Add the CLI-owned run-state/report builder described under Fixed Decisions.
+   Initialize it immediately after run-ID generation and update it as pipeline
+   identity, output path, and `RunOutput` become available.
+2. Add a debug-summary terminalization method or focused helper that writes the
+   success report, or the failure report plus error log, exactly once.
+3. Route every post-allocation failure through the common terminalization path:
+   catalog and reference resolution, pipeline resolution, profile validation,
+   reference materialization, registry and LLM construction, preparation, input
+   read, cache/checkpoint construction, pipeline execution, summary writes,
+   output allocation, and output writes.
+4. Replace the existing success-only report literal with the same report
+   builder and terminalization path.
+5. Preserve failures before run-ID generation or debug allocation as stderr-only
+   failures.
+6. Keep explicit guards for failures caused by summary terminalization itself so
+   the command never recursively rewrites `run-report.json` or `error.log`.
+
+### Focused tests
+
+- For resolution, pipeline, and output-write failures, assert both
+  `run-report.json` and `error.log` exist, `succeeded` is false, and every known
+  field is correct.
+- For a pipeline failure with partial `RunOutput`, assert available counts,
+  validation status, manifest, warnings, checkpoint events, and chunk-plan
+  summary are retained.
+- Assert a success report uses `succeeded: true` and contains the same paths and
+  counts printed by the CLI.
+- Inject independent failures for the run-report writer and error-log writer.
+  Assert each operation is attempted no more than once, the primary error is
+  printed first, the secondary error is reported, the debug path is printed,
+  and exit code is `1`.
+- Assert a pre-allocation configuration failure creates no report or debug root.
+- Assert a non-debug failure performs no summary writes.
+
+Run at minimum:
+
+```sh
+go test ./internal/core/debugbundle ./internal/cli
+```
+
+### Exit criteria
+
+Every successfully allocated debug bundle has a best-effort terminal report;
+ordinary failures retain both structured and textual outcomes; summary failures
+cannot recurse or mask the primary error; and all repository checks pass.
+
+## Stage 4: Final audit and close the remediation plan
+
+**Status:** Not started
+
+### Objective
+
+Verify the three findings are closed, align any affected current-behavior
+documentation, and remove this completed planning artifact.
+
+### Read first
+
+- the final code and tests from Stages 1–3
+- [Documentation Policy](../policy/documentation.md)
+- [Architecture Policy](../policy/architecture.md)
+- [CLI Reference](../cli.md)
+- [Operations](../operations.md)
+- [Run State Internals](../internal/state.md)
+
+### Implement
+
+1. Audit every `summary/` writer call and confirm each accepts or constructs a
+   redacted payload. Search for direct serialization of raw effective pipeline
+   bindings into summary files.
+2. Audit every output path and confirm no code writes into a pre-existing run
+   directory. Confirm debug and output use one CLI-owned ID.
+3. Audit every post-allocation return path and confirm it reaches terminal
+   reporting exactly once or is itself an explicitly guarded terminalization
+   failure.
+4. Update Operations or Run State Internals only where the final implementation
+   adds a useful current-behavior clarification about run IDs, exclusive output
+   allocation, partial output after failure, or best-effort failure reports.
+   Do not duplicate volatile CLI or configuration contracts.
+5. Confirm Finding 4 remains deferred. Do not restore unrelated deleted tests,
+   introduce a coverage threshold, or add a legacy-checkpoint fixture in this
+   remediation.
+6. After all validation succeeds and documentation is current, delete this
+   implementation plan. The accepted ADR and canonical current-behavior
+   documents remain authoritative.
+
+### Validation
+
+Run:
+
+```sh
+go test ./...
+go vet ./...
+go build ./cmd/notarius
+git diff --check
+```
+
+Manually inspect the changed debug artifacts from one success, one pipeline
+failure, and one output collision. Confirm no test fixture contains real
+credentials or private data.
+
+### Exit criteria
+
+Findings 1–3 are closed with focused regression tests; durable output cannot be
+overwritten by a run-ID collision; failure bundles have terminal reports;
+summary redaction is enforced by contract; documentation is accurate; Finding
+4 remains deferred; the plan is removed; and all checks pass.