From f3506240c2b2da23ac436d111156fa9cfafa86b6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 8 Aug 2026 21:37:57 +0000 Subject: [PATCH] Audit state persistence and file safety --- docs/roadmap/audit.md | 228 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 223 insertions(+), 5 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 2b09bf9..ef5525d 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -19,10 +19,10 @@ change only roadmap audit documents do not change that production target. ## Executive Summary Pending final synthesis. The initial baseline is healthy. The architecture, -configuration/CLI, pipeline composition, reference/handoff, and runtime reviews -have found two High findings, three Medium findings, and eight Low findings, -with no production dependency inversion, unbounded framework worker pool, or -completion-order-dependent result assembly. +configuration/CLI, pipeline composition, reference/handoff, runtime, and state +reviews have found two High findings, four Medium findings, and ten Low +findings, with no production dependency inversion, unbounded framework worker +pool, completion-order-dependent result assembly, or debug-to-cache coupling. ## Finding Index @@ -42,6 +42,9 @@ Final cross-area ordering is pending synthesis. | RUN-002 | High | Correctness | Isolate typed validator values from stage output | | RUN-003 | Medium | Correctness | Preserve warnings from the terminal rejected attempt | | RUN-004 | Low | Documentation/Comments | Document the lane collector's liveness invariant | +| STATE-001 | Medium | Correctness | Preserve distinct identities in state paths | +| STATE-002 | Low | Efficiency | Remove redundant post-decode clones from canonical codecs | +| STATE-003 | Low | Duplication | Publish output through the confined file writer | ## Findings @@ -455,6 +458,114 @@ Final cross-area ordering is pending synthesis. continuation-overlap, worker-bound, race, and shuffled tests. - **Grouping:** Independent. +### State, Checkpoints, Debugging, And File Safety + +### STATE-001 — Preserve distinct identities in state paths + +- **Severity:** Medium +- **Category:** Correctness +- **Evidence:** The resolver accepts any non-empty trimmed ordered-step and + lane IDs (`internal/framework/pipeline/profile.go:341`–`351` and + 1382–1392). Checkpoint path construction then sends both IDs through + `checkpointPathComponent` (`internal/framework/checkpoint/recorder.go:476`– + `508`), which maps `.`, `..`, and every value containing `..` to the single + component `_`. The trace side duplicates that encoder in + `debugPathComponent` (`internal/framework/pipeline/debug.go:37`–`62`), and + `cleanDebugPath` at lines 364–374 additionally applies `path.Clean` before + encoding. Thus valid distinct identities such as `.` and `_`, or `a..b` and + `_`, select the same checkpoint component and can also collapse to the same + debug path. Existing filesystem tests cover traversal and symlink rejection, + but no focused test asserts that accepted identities map injectively. +- **Impact:** Two valid lanes or steps in one resolved pipeline can overwrite + each other's checkpoint manifests and payloads. Exact manifest validation + prevents a mismatched artifact from being silently reused, but ordinary + resume repeatedly invalidates and re-executes the colliding work, and a + required selective-recompute predecessor can become unavailable after its + sibling overwrites it. Requested debug records can likewise overwrite or be + attributed to the wrong identity. Atomic rename does not protect against a + logical-name collision. +- **Recommendation:** Introduce one shared, injective path-component encoding + for accepted application identities. Preserve already-safe components, but + encode reserved dot components and every unsafe rune rather than replacing a + whole value with `_`; do not run identity-bearing debug paths through a + lossy clean operation first. Keep path joining and confinement separate from + identity encoding. +- **Preserve:** Retain trimmed non-empty identity validation, human-readable + ordinary IDs, exact step/lane checks inside checkpoint manifests, narrow + relative state paths, symlink rejection, and recoverable cold execution for + incompatible reconstructible cache entries. +- **Validation:** Add table and uniqueness cases covering `.`, `..`, `_`, + embedded `..`, separators, tildes, and Unicode, then run a two-identity + filesystem checkpoint/resume and debug trace case that proves distinct files + and successful accepted-normalize hydration. Include selective recomputation + so a required predecessor remains reusable. +- **Grouping:** Independent. + +### STATE-002 — Remove redundant post-decode clones from canonical codecs + +- **Severity:** Low +- **Category:** Efficiency +- **Evidence:** `chunkmap.Codec.Decode` schema-validates and decodes JSON into a + fresh value, canonicalizes that value, then deep-clones it again before + returning (`internal/framework/chunkmap/codec.go:149`–`171`). + `evidencecontext.Codec.Decode` follows the same sequence + (`internal/framework/evidencecontext/codec.go:79`–`101`), while its + `canonicalize` already deep-clones the decoded document at lines 169–203; + the final `clone` at lines 299–320 therefore makes a second complete copy of + context/reference slices, source-unit structs, and nested metadata. Unlike an + encode caller, the JSON decoder retains no caller-owned object graph that + must be protected from canonicalization. +- **Impact:** Hydrating or consuming canonical artifacts performs avoidable + slice and nested-metadata allocations. The evidence-context path can clone a + large source-derived object graph twice after JSON decoding, increasing peak + allocation and latency without adding an ownership boundary. +- **Recommendation:** Separate canonicalization of an already-owned decoded + value from the defensive-copy entry point used by encoders and external + callers. Return the canonical decoded value directly once validation and + normalization succeed; retain exactly one clone wherever caller-owned input + could otherwise be mutated. +- **Preserve:** Keep schema validation, unknown-field and trailing-value + rejection, digest reconstruction, annotation/context normalization, + encode-time caller isolation, and a fully independently owned decode result. +- **Validation:** Retain and extend ownership tests that mutate both the input + value after encode and the returned value after decode, then use focused + allocation assertions or benchmarks for a nested chunk map and evidence + context to confirm that decode no longer performs the redundant deep copy. +- **Grouping:** Independent. + +### STATE-003 — Publish output through the confined file writer + +- **Severity:** Low +- **Category:** Duplication +- **Evidence:** The CLI's private `writeFileAtomic` + (`internal/cli/run.go:826`–`856`) independently implements the same + create-temp, write, chmod, close, rename, and cleanup sequence as + `internal/core/fileio.writeAtomic` (`internal/core/fileio/fileio.go:105`– + `133`). `writeOutputFiles` calls the CLI copy after its own path checks and + directory creation (`internal/cli/run.go:754`–`787`), while + `fileio.WriteBytes` additionally centralizes relative-path confinement and + symlink-component rejection. Debug summaries, traces, and checkpoints + already use that core primitive; durable output is the divergent state + writer. +- **Impact:** Atomic-publication fixes must be made and tested in two places, + and the copies have already drifted in their confinement checks. The fresh, + exclusively created run directory limits current output exposure, so this is + a maintenance and defense-in-depth issue rather than an observed escape. +- **Recommendation:** After prevalidating all logical output names and + exclusively creating the run directory, publish each file through + `fileio.WriteBytes` (or a narrowly exported equivalent) using the run + directory as root. Remove the CLI atomic-write copy rather than introducing + another generic filesystem abstraction. +- **Preserve:** Validate every logical name before allocating state, refuse an + existing run directory unchanged, retain a newly created partial bundle on a + later write failure, keep output directory/file modes at 0755/0644, and + preserve logical-name context in errors. +- **Validation:** Retain the existing output ordering, collision, permission, + and partial-bundle cases; add a symlink-component case at the shared writer + boundary and verify no temporary file remains after write, chmod, close, or + rename failure. +- **Grouping:** Independent. +