diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md new file mode 100644 index 0000000..6f0eb5f --- /dev/null +++ b/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--<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 + `/` 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.