diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 9f35dfa..f74c669 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -18,9 +18,10 @@ change only roadmap audit documents do not change that production target. ## Executive Summary -Pending final synthesis. The initial baseline is healthy. The architecture and -configuration/CLI reviews have found one Medium correctness finding and two -Low findings, with no production dependency inversion. +Pending final synthesis. The initial baseline is healthy. The architecture, +configuration/CLI, and pipeline composition reviews have found one Medium +correctness finding and four Low findings, with no production dependency +inversion or unsafe typed-erasure boundary. ## Finding Index @@ -31,6 +32,8 @@ Final cross-area ordering is pending synthesis. | ARCH-001 | Low | Documentation/Comments | Repair broken ADR cross-references | | CFGCLI-001 | Medium | Correctness | Reject additional YAML documents | | CFGCLI-002 | Low | Correctness | Reject a blank command-level LLM profile | +| PIPE-001 | Low | Correctness | Reject normalized module-reference collisions in the resolver | +| PIPE-002 | Low | Efficiency | Clone construction inputs once per builder boundary | ## Findings @@ -116,6 +119,79 @@ Final cross-area ordering is pending synthesis. valid and unknown-profile run cases; run `go test ./internal/cli`. - **Grouping:** Independent. +### Pipeline Resolution, Preparation, And Typed Registries + +### PIPE-001 — Reject normalized module-reference collisions in the resolver + +- **Severity:** Low +- **Category:** Correctness +- **Evidence:** `internal/framework/pipeline/profile.go:1239`–`1252` sends every + module binding's reference map through `normalizeReferenceMap`. That helper, + at lines 1361–1377, trims each key but overwrites + `rawByNormalized[trimmedKey]` without checking whether another raw key already + produced the same identity. A programmatic binding containing both `slot` + and ` slot ` therefore retains whichever raw key is visited last by Go's map + iteration, then silently emits only one resolved binding. The later strict + reference resolver sees only the collapsed map and cannot diagnose the + collision. `internal/core/config/validation.go:256`–`266` correctly rejects + this shape for validated file/config flows, but direct `ResolvePipeline` + callers do not pass through that owner and there is no focused framework + regression case. +- **Impact:** A programmatically assembled profile can resolve successfully to + different external paths or generated selectors across processes from the + same ambiguous input. The normal CLI configuration path is protected by + upstream validation, which limits current production exposure, but the + resolver's own contract is nondeterministic. +- **Recommendation:** Make binding reference normalization return an error for + empty or duplicate trimmed keys before constructing the normalized map, and + propagate stage/lane context through `resolveBinding` callers. Avoid relying + on the config layer to make the framework resolver deterministic. +- **Preserve:** Keep whitespace normalization, exact external-versus-generated + source validation, sorted resolved bindings, local-over-pipeline precedence, + and the config layer's earlier contextual diagnostics. +- **Validation:** Add focused `ResolvePipeline` cases for whitespace-equivalent + chunk, extract, merge, and normalize reference keys, including different + source forms, and assert deterministic contextual rejection; run + `go test ./internal/framework/pipeline ./internal/core/config`. +- **Grouping:** Independent. + +### PIPE-002 — Clone construction inputs once per builder boundary + +- **Severity:** Low +- **Category:** Efficiency +- **Evidence:** `Prepare` and `prepareLane` clone binding option maps while + forming requests (`internal/framework/pipeline/prepare.go:102`–`104` and + 202–206), and `prepareValidatorChain` does the same at lines 258–263. + Registry/build boundaries then clone the complete request again. Typed + extractors, mergers, normalizers, and validators add another clone inside + their registered erased-builder adapters + (`extractor_registry.go:56`, `merger_registry.go:68`–`74`, + `normalizer_registry.go:63`–`69`, and `validator_registry.go:101`–`108`), + after `buildErasedModule` or `buildPreparedValidator` already called + `cloneBuildRequest` (`prepare.go:272`–`299` and 325–327). Each request clone + deep-copies materialized reference content as well as options, so typed + builders receive two reference copies and as many as three option copies; + untyped stage and validator builders use fewer copies. +- **Impact:** Every preparation repeats allocation and byte copying for bounded + external references and nested options, with the highest cost and a + different ownership path specifically for typed lanes and validators. The + work is run-construction-time rather than a concurrent operation hot path, + so the issue is low severity. +- **Recommendation:** Designate one private construction invocation as the + ownership boundary and clone the complete `BuildRequest` exactly there. + Store raw builders or remove the caller-side clone consistently so all stage + and validator registry variants follow the same single-copy rule. +- **Preserve:** Builders must continue to receive independently owned options, + reference maps, slot slices, metadata, and content bytes; preparation must + retain its own immutable resolved/reference state; nil, key/name, execution + class, and exact artifact-type checks must remain contextual errors. +- **Validation:** Extend construction hooks to mutate nested options and + reference bytes for typed and untyped modules/validators, assert no aliasing + with resolved or sibling requests, and use allocation/byte-copy observations + or a focused benchmark to confirm a single defensive copy; run + `go test ./internal/framework/contracts ./internal/framework/pipeline`. +- **Grouping:** Independent. +