From 3c35124db419a42101b5f680fe45d9d23177133c Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 18 Jul 2026 14:36:47 +0000 Subject: [PATCH] Complete run state hardening audit --- docs/cli.md | 3 +- docs/internal/state.md | 5 +- docs/roadmap/future.md | 6 + docs/roadmap/implementation.md | 403 ----------------------- internal/cli/state_hardening_test.go | 11 +- internal/core/debugbundle/bundle_test.go | 6 +- internal/core/debugbundle/summary.go | 28 +- 7 files changed, 38 insertions(+), 424 deletions(-) delete mode 100644 docs/roadmap/implementation.md diff --git a/docs/cli.md b/docs/cli.md index 1e22527..7010549 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -30,7 +30,8 @@ Flags: - `--only lane-a,lane-b`: run only the named artifact lanes. Values are comma-separated and must be non-empty. - `--resume`: request checkpoint reuse for this invocation. See - [Operations](operations.md#checkpoints) for prerequisites and reuse behavior. + [Operations](operations.md#checkpoint-cache) for prerequisites and reuse + behavior. - `--chunk_cache auto|bypass|refresh`: select chunk-plan reuse for this invocation. `auto` reuses a valid plan by canonical source digest, `bypass` performs no plan-cache I/O, and `refresh` regenerates and replaces a valid diff --git a/docs/internal/state.md b/docs/internal/state.md index b64b711..371ce6d 100644 --- a/docs/internal/state.md +++ b/docs/internal/state.md @@ -14,8 +14,9 @@ debug roots. ## Output And Cache -The pipeline runner returns logical output files. The CLI places them beneath -the selected output root with confined, atomic writes. +The pipeline runner returns logical output files. After validating every +logical name, the CLI exclusively creates the run directory beneath the +selected output root and performs confined, atomic file writes within it. `internal/framework/chunkplan` owns source-addressed plan storage, validation, and atomic publication. Its store is constructed only when the selected mode is diff --git a/docs/roadmap/future.md b/docs/roadmap/future.md index f1475bf..8d8433f 100644 --- a/docs/roadmap/future.md +++ b/docs/roadmap/future.md @@ -41,6 +41,12 @@ future work only. - Optional generated example output fixtures with a regeneration procedure. - Additional diagnostics or reporting views if operator workflows need them. +## Candidate Developer Work + +- Reassess broad CLI and configuration regression coverage, including whether + to retain a legacy-checkpoint compatibility fixture and adopt a coverage + policy. + ## Candidate Workspace Work - Default-idempotent run behavior with an explicit force override. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md deleted file mode 100644 index ebbda9a..0000000 --- a/docs/roadmap/implementation.md +++ /dev/null @@ -1,403 +0,0 @@ -# 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:** Complete - -### 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:** Complete - -### 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:** Complete - -### 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. diff --git a/internal/cli/state_hardening_test.go b/internal/cli/state_hardening_test.go index ae73da9..5da5634 100644 --- a/internal/cli/state_hardening_test.go +++ b/internal/cli/state_hardening_test.go @@ -523,13 +523,22 @@ func TestRunRefusesExistingOutputDirectoryWithoutChangingIt(t *testing.T) { opts := newStateTestHarness().options() opts.RunIDGenerator = func(time.Time) (string, error) { return runID, nil } - result := runStateTest(t, roots, opts, false, false, "bypass") + result := runStateTest(t, roots, opts, true, false, "bypass") if result.code != 1 || !strings.Contains(result.stderr, "output run directory") || !strings.Contains(result.stderr, "already exists") { t.Fatalf("code=%d stderr=%q", result.code, result.stderr) } if after := readTree(t, runPath); !sameFiles(after, before) { t.Fatalf("existing output changed: before=%v after=%v", before, after) } + bundlePath := filepath.Join(roots.debug, runID) + report := readStateTestRunReport(t, bundlePath) + if report.Succeeded || report.RunID != runID || report.OutputPath != runPath || report.DebugPath != bundlePath || report.OutputCount != 1 || report.ValidationStatus != "approved" { + t.Fatalf("output collision report = %#v", report) + } + errorLog, err := os.ReadFile(filepath.Join(bundlePath, "summary", "error.log")) + if err != nil || !strings.Contains(string(errorLog), "already exists") { + t.Fatalf("output collision error log = %q, %v", errorLog, err) + } } func TestRepeatedRunIdentityCannotOverwriteFirstOutput(t *testing.T) { diff --git a/internal/core/debugbundle/bundle_test.go b/internal/core/debugbundle/bundle_test.go index ab65946..e830ae9 100644 --- a/internal/core/debugbundle/bundle_test.go +++ b/internal/core/debugbundle/bundle_test.go @@ -137,15 +137,15 @@ func TestSummaryWriterWritesEverySummaryArtifact(t *testing.T) { } } } -func TestSummaryWriterConfinesArtifacts(t *testing.T) { +func TestSummaryWriterInternalWritesConfineArtifacts(t *testing.T) { bundle, err := Allocate(t.TempDir(), testBundleRunID, time.Unix(0, 42)) if err != nil { t.Fatal(err) } - if err := bundle.Summary().WriteJSON("../outside.json", map[string]any{}); err == nil { + if err := bundle.Summary().writeJSON("../outside.json", map[string]any{}); err == nil { t.Fatal("accepted traversal") } - if err := bundle.Summary().WriteBytes(`trace\\x`, []byte("x")); err == nil { + if err := bundle.Summary().writeBytes(`trace\\x`, []byte("x")); err == nil { t.Fatal("accepted backslash") } } diff --git a/internal/core/debugbundle/summary.go b/internal/core/debugbundle/summary.go index d596c1c..49b291e 100644 --- a/internal/core/debugbundle/summary.go +++ b/internal/core/debugbundle/summary.go @@ -67,40 +67,40 @@ func (w *SummaryWriter) WriteInvocation(payload Invocation) error { if payload.StartedAt.IsZero() { payload.StartedAt = w.createdAt } - return w.WriteJSON(ArtifactInvocationMetadata, payload) + return w.writeJSON(ArtifactInvocationMetadata, payload) } func (w *SummaryWriter) WriteRedactedEffectiveConfig(payload RedactedSummaryPayload) error { if payload == nil { return fmt.Errorf("redacted summary payload must not be nil") } - return w.WriteJSON(ArtifactEffectiveConfig, payload.RedactedSummaryPayload()) + return w.writeJSON(ArtifactEffectiveConfig, payload.RedactedSummaryPayload()) } func (w *SummaryWriter) WriteResolvedPipeline(payload RedactedResolvedPipelinePayload) error { if payload == nil { return fmt.Errorf("redacted resolved pipeline payload must not be nil") } - return w.WriteJSON(ArtifactResolvedPipeline, payload.RedactedResolvedPipelinePayload()) + return w.writeJSON(ArtifactResolvedPipeline, payload.RedactedResolvedPipelinePayload()) } -func (w *SummaryWriter) WriteResolvedReferences(v any) error { - return w.WriteJSON(ArtifactResolvedReferences, v) +func (w *SummaryWriter) WriteResolvedReferences(v []artifacts.ReferenceProvenance) error { + return w.writeJSON(ArtifactResolvedReferences, v) } -func (w *SummaryWriter) WriteCheckpointEvents(v any) error { - return w.WriteJSON(ArtifactCheckpointEvents, v) +func (w *SummaryWriter) WriteCheckpointEvents(v []pipeline.CheckpointEvent) error { + return w.writeJSON(ArtifactCheckpointEvents, v) } func (w *SummaryWriter) WriteRunManifest(v artifacts.RunManifest) error { - return w.WriteJSON(ArtifactRunManifest, v) + return w.writeJSON(ArtifactRunManifest, v) } func (w *SummaryWriter) WriteChunkPlan(v artifacts.ChunkPlanSummary) error { - return w.WriteJSON(ArtifactChunkPlan, v) + return w.writeJSON(ArtifactChunkPlan, v) } -func (w *SummaryWriter) WriteRunReport(v RunReport) error { return w.WriteJSON(ArtifactRunReport, v) } +func (w *SummaryWriter) WriteRunReport(v RunReport) error { return w.writeJSON(ArtifactRunReport, v) } func (w *SummaryWriter) WriteWarnings(v []contracts.Warning) error { - return w.WriteJSON(ArtifactWarnings, v) + return w.writeJSON(ArtifactWarnings, v) } func (w *SummaryWriter) WriteError(message string) error { - return w.WriteBytes(ArtifactErrorLog, []byte(message+"\n")) + return w.writeBytes(ArtifactErrorLog, []byte(message+"\n")) } -func (w *SummaryWriter) WriteJSON(name string, v any) error { +func (w *SummaryWriter) writeJSON(name string, v any) error { if w == nil { return fmt.Errorf("debug summary writer must not be nil") } @@ -109,7 +109,7 @@ func (w *SummaryWriter) WriteJSON(name string, v any) error { } return nil } -func (w *SummaryWriter) WriteBytes(name string, v []byte) error { +func (w *SummaryWriter) writeBytes(name string, v []byte) error { if w == nil { return fmt.Errorf("debug summary writer must not be nil") }