diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 43c5397..2b09bf9 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -19,9 +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, and reference/handoff reviews have -found two Medium findings and seven Low findings, with no production dependency -inversion or unsafe typed-erasure boundary. +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. ## Finding Index @@ -37,6 +38,10 @@ Final cross-area ordering is pending synthesis. | REF-001 | Medium | Efficiency | Bound reference reads before allocating the file | | REF-002 | Low | Efficiency | Index accepted outputs once per ordered handoff | | REF-003 | Low | Correctness | Include canonical size in generated-reference fingerprints | +| RUN-001 | High | Correctness | Check cancellation at unguarded dispatch boundaries | +| 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 | ## Findings @@ -292,6 +297,164 @@ Final cross-area ordering is pending synthesis. `go test ./internal/framework/pipeline ./internal/modules/integration/...`. - **Grouping:** Independent. +### Execution, Validation, Retry, And Concurrency + +### RUN-001 — Check cancellation at unguarded dispatch boundaries + +- **Severity:** High +- **Category:** Correctness +- **Evidence:** `Runner.Run` invokes the input adapter at + `internal/framework/pipeline/runner.go:159`–`177` without checking + `ctx.Err()` first; the first framework-owned cancellation check on that path + is inside `runWithRetry` at lines 389–418, after source work and its + checkpoint/debug side effects. At the other end of the run, ordered lane work + completes at lines 278–282, then evidence/debug assembly and + `encoder.Encode` run at lines 297–346 without another cancellation check. + The worker and retry paths do check cancellation, but + `TestRunnerReturnsParentCancellationAndStopsQueuedExtracts` covers only + cancellation while two extract calls are active + (`runner_concurrency_test.go:559`–`585`). A context-ignoring input or output + module can therefore be called with an already-cancelled context; in the + output case it can return files and make the cancelled run report success. +- **Impact:** Work can start after cancellation, and a cancellation arriving + after lane completion or during output debug assembly can still publish a + successful logical output. This violates the documented guarantee that + parent cancellation prevents queued work and output encoding, and it leaves + correctness dependent on every module independently honoring an already + cancelled context. +- **Recommendation:** Add framework-owned cancellation gates at run entry and + immediately before every module dispatch not already protected by + `runWithRetry` or a lane worker, especially input parsing and output encoding. + Recheck after intervening collaborator/debug/evidence work so a cancellation + cannot slip between the gate and the operation, and after an unguarded module + returns so a module that did not observe mid-call cancellation cannot publish + success. Return the parent context error through the existing failed-output + path. +- **Preserve:** Continue to pass the caller context into active operations, + wait for all started lane work, suppress output files on framework failure, + select an actual framework error deterministically when the parent remains + live, and prefer the parent cancellation when it is set. +- **Validation:** Add a pre-cancelled run whose input adapter ignores context + and must not be called, plus a run whose debug recorder cancels immediately + before output dispatch while a context-ignoring encoder records calls, and an + encoder that cancels mid-call but returns files. Assert `context.Canceled`, no + invocation after pre-dispatch cancellation, and no output files in either + output case; retain the active-extract cancellation case and run the package + under `-race`. +- **Grouping:** Independent. + +### RUN-002 — Isolate typed validator values from stage output + +- **Severity:** High +- **Category:** Correctness +- **Evidence:** Extract, merge, and normalize serialize a canonical candidate + before validation, but pass the operation's original typed value alongside + it; representative extract code is + `internal/framework/pipeline/runner_concurrent.go:424`–`439`, and merge and + normalize do the same in `runner_typed.go:264`–`277` and 359–392. + `validateTypedArtifact` clones source input, references, metadata, and chunks, + but `requestTarget := target` leaves `target.value` shallow-copied + (`runner_typed.go:458`–`499`). The registered typed adapter then exact-casts + that value and passes it directly to the validator + (`validator_registry.go:109`–`119`). Struct values containing slices, maps, + or pointers therefore retain aliases to the stage output. A validator can + mutate what later validators and final checkpoint serialization observe, + while serialized validators and attempt debug continue to describe the + pre-mutation canonical candidate. Candidate-encoding tests verify encode + counts and rejection-before-checkpoint behavior, but no focused test mutates + a typed validator request. +- **Impact:** A buggy validator can corrupt an accepted output, cause validators + in one chain to inspect different values, or make persisted output differ + from the candidate that was serialized for validation and debug. This breaks + the documented immutable whole-output validation contract at all three typed + stages. +- **Recommendation:** Treat the already serialized canonical candidate as the + clone boundary. Decode a fresh exact typed value through the active codec for + each typed validator, or provide an equivalent codec-backed deep clone, and + never expose the stage operation's retained value. Keep serialized validators + on independent schema/content copies of the same candidate. +- **Preserve:** Retain exact Go-type checks, one canonical candidate encode per + attempt, validator declaration order, isolated request metadata/references, + serialized-validator byte ownership, contextual errors, and final encoding + only after the entire chain approves. +- **Validation:** Use an artifact with both slice and map fields. Have the first + typed validator mutate both, then assert that the next typed validator, a + serialized validator, attempt debug, and accepted extract/merge/normalize + output all observe the original canonical value. Run the focused package + tests under `-race` as well as normally. +- **Grouping:** Independent. + +### RUN-003 — Preserve warnings from the terminal rejected attempt + +- **Severity:** Medium +- **Category:** Correctness +- **Evidence:** The runtime contract says warnings from the final accepted or + rejected attempt are preserved (`docs/internal/pipeline.md:122`–`126`). Each + retry closure assembles per-attempt operation and validator warnings, but + stores them only on acceptance. Generated chunk-plan rejection returns at + `internal/framework/pipeline/runner_chunk_plan.go:142`–`150` before assigning + `result.warnings` at line 154, and `Runner.Run` promotes chunk warnings only + for acceptance or a cache hit (`runner.go:228`–`233`). Extract follows the + same pattern at `runner_concurrent.go:425`–`457`; `finalizeLaneExtract` skips + directly from a rejected result to the next chunk at lines 469–476. Merge + and normalize collect `attemptWarnings` but return terminal rejections before + appending them to `RunOutput` (`runner_typed.go:264`–`301` and 359–415). + `runWithRetry` returns only the last rejection, not its warnings + (`runner.go:389`–`428`). Existing retry-warning tests prove that discarded + attempts are not promoted, and final-rejection debug tests prove rejection + recording, but neither asserts terminal rejection warnings. +- **Impact:** Module warnings and warnings from validators that approved before + the rejecting validator disappear exactly when the final candidate is + rejected. Operators receive the rejection but lose diagnostics produced by + that terminal attempt, contrary to the documented output contract. +- **Recommendation:** Carry the warnings associated with the last rejection + through the retry result, then promote only that terminal attempt's warnings + at chunk, extract, merge, and normalize rejection handling. Keep warnings + from earlier failed or rejected attempts confined to attempt debug. Coordinate + any persistence-format addition with the later checkpoint/state audit rather + than silently changing state ownership here. +- **Preserve:** Do not promote warnings from attempts superseded by a later + retry, do not turn rejection into a framework error, keep deterministic + lane/chunk warning order, and retain accepted and reused checkpoint warning + behavior. +- **Validation:** Add table-driven chunk, extract, merge, and normalize cases + with distinct first-attempt and final-rejection warning scopes. Assert that + only the final scopes reach `RunOutput`, rejection attempt counts remain + correct, intermediate warnings remain in attempt debug, and shuffled/race + runs preserve order. +- **Grouping:** Independent. + +### RUN-004 — Document the lane collector's liveness invariant + +- **Severity:** Low +- **Category:** Documentation/Comments +- **Evidence:** `laneEngine.collect` coordinates a dynamically enabled + continuation send, extract-result closure, cancellation, and the + `pending`/`launched`/`completed` counters in + `internal/framework/pipeline/runner_concurrent.go:256`–`281`, with state + changes split across `handleExtractResult` and `handleCompletion` at lines + 283–313. The loop has no invariant-level comment. Its non-obvious purpose is + to discard only unlaunched continuations after cancellation while continuing + to drain extract results and exactly one completion from every launched + continuation before the continuation channel is closed and workers are + awaited. +- **Impact:** Current tests and the race run support the implementation, but a + future change to channel buffering, cancellation cleanup, or a counter can + introduce a deadlock or early close without the ownership/liveness rule being + visible at the maintenance point. +- **Recommendation:** Add one short comment immediately above + `laneEngine.collect` stating that it is the sole owner of pending/launch/ + completion accounting and must drain closed extract results plus every + launched continuation completion even after cancellation. Do not narrate the + `select` cases. +- **Preserve:** Keep chunk-first/lane-second extract dispatch, bounded extract + and continuation pools, overlap between completed lanes and remaining + extracts, cancellation of undispatched work, and the final worker wait. +- **Validation:** Review the comment against the collector termination + predicate and retain the existing cancellation, reverse-completion, + continuation-overlap, worker-bound, race, and shuffled tests. +- **Grouping:** Independent. +