Audit pipeline composition and typed registries
This commit is contained in:
@@ -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.
|
||||
|
||||
<!--
|
||||
Finding template for later audit stages:
|
||||
|
||||
@@ -172,6 +248,26 @@ Finding template for later audit stages:
|
||||
forward forced/backward reusable checkpoint closure. Keeping these typed
|
||||
traversals separate avoids adding command syntax or checkpoint policy to the
|
||||
framework resolver.
|
||||
- `pipeline.ResolvePipeline` and `resolveArtifactLane` are long, but their
|
||||
linear sections retain the authoritative composition order: normalize
|
||||
identity, select lanes, prove capabilities and exact artifact variants,
|
||||
resolve stage-local references and validator chains, apply effective LLM
|
||||
profiles, validate options, and only then compute the digest. Splitting these
|
||||
checks into a generic stage engine would erase the different input, chunk,
|
||||
typed-lane, validator, and output contracts. PIPE-001 is a bounded
|
||||
normalization fix and should not reorganize this sequence.
|
||||
- `pipeline.validateGeneratedBindings` has deeply nested traversal because it
|
||||
proves a cross-step selector against ordered producer identity, the target's
|
||||
declared slot, accepted artifact kinds, registered codec, and accepted media
|
||||
types in one pass. Those checks are distinct static composition invariants;
|
||||
materialized bytes, runtime handoff construction, and checkpoint hydration
|
||||
remain owned by the next audit area.
|
||||
- The stage, validator, codec, and evidence registries intentionally use
|
||||
private typed entries and small stage-specific lookup methods. Their
|
||||
repetition preserves compile-time generic types until a narrow erased
|
||||
closure, exact artifact-kind variant selection, and stage-specific
|
||||
diagnostics. PIPE-002 concerns redundant request copies around those
|
||||
closures, not the typed registry split itself.
|
||||
|
||||
## Areas Reviewed Without Findings
|
||||
|
||||
@@ -276,6 +372,65 @@ Finding template for later audit stages:
|
||||
repeating lower-level resolver assertions. The two uncovered command/parser
|
||||
cases are recorded as CFGCLI-001 and CFGCLI-002.
|
||||
|
||||
### Pipeline Resolution, Preparation, And Typed Registries
|
||||
|
||||
- **Static composition:** `ResolvePipeline` rejects mixed legacy/ordered
|
||||
shapes, empty and duplicate normalized step/lane identities, invalid
|
||||
invocation filtering, unknown modules, missing capabilities, incompatible
|
||||
artifact variants, unsupported lane-level validators, invalid generated
|
||||
selectors, and missing output capabilities before producing a resolved
|
||||
value. Apart from PIPE-001's direct-call collision, selected lanes and steps
|
||||
have stable sorted/order-preserving identities.
|
||||
- **Effective policy and identity:** Execution classes come from normalized
|
||||
registry specs. Command override → binding → pipeline LLM-profile precedence
|
||||
applies only to selected LLM-backed modules and validators; deterministic
|
||||
bindings reject explicit profiles. Module and validator option validators
|
||||
receive owned maps before `resolvedPipelineDigest` hashes the complete
|
||||
effective composition. Digest tests cover map canonicalization, validator
|
||||
policy, artifact schema identity, effective profiles, and exclusion of the
|
||||
digest field itself.
|
||||
- **Typed registry boundary:** Extractor registrations retain one exact Go type;
|
||||
merger, normalizer, and typed-validator registrations select an exact
|
||||
module/artifact-kind variant; codecs validate complete schema/media identity;
|
||||
and evidence projectors must match the active codec type. Every erased
|
||||
operation checks the implementation or value type and returns an error rather
|
||||
than asserting or panicking. Kind-neutral `Spec` methods are confined to
|
||||
catalog inspection, while behavior-sensitive resolution uses exact variant
|
||||
lookups.
|
||||
- **Construction boundary:** `Prepare` validates the resolved shape and needed
|
||||
registries, clones retained bindings, options, validator chains, reference
|
||||
targets, schemas, and bytes, then constructs input, chunker, chunk validators,
|
||||
every ordered typed lane and local validator chain, output, and the optional
|
||||
evidence plan before returning. Implementations and operations remain
|
||||
private; public prepared bindings/lanes are separate clones. Focused tests
|
||||
verify deterministic construction order, late failure before input parsing,
|
||||
nil and identity rejection, generated-selector retention, and independent
|
||||
builder reference inputs. PIPE-002 records only the extra adjacent copies.
|
||||
- **Checkpoint supplements:** Preparation collects component-provided semantic
|
||||
fingerprints only after the complete implementation set exists. Scopes
|
||||
include stage, globally unique lane identity, module, and validator position;
|
||||
empty or duplicate values fail preparation, results are sorted, and the
|
||||
accessor returns a defensive copy. Scheduling limits and diagnostics are not
|
||||
included. Resolved composition—including options, reference selectors,
|
||||
effective profiles, retries, and validator order—remains owned by the
|
||||
resolved digest rather than being redundantly restated as component
|
||||
fingerprints.
|
||||
- **Registry comparison:** Input, chunker, and output registries consistently
|
||||
normalize keys/specs, reject nil validators/builders, validate options on
|
||||
owned maps, clone stored specs, sort discovery output, and verify constructed
|
||||
identity. Typed stage and validator registries add only the exact-type and
|
||||
artifact-variant mechanics their contracts require. Validator-chain lookup
|
||||
distinguishes absent, default, explicit replacement, and explicit empty
|
||||
chains while returning defensive copies. No dead compatibility path or safe
|
||||
consolidation was found beyond the copy reduction in PIPE-002.
|
||||
- **Test ownership:** Contract tests cover clone/serialization boundaries;
|
||||
registry tests cover invalid registration, sorted/defensive discovery,
|
||||
strict options, exact types, schema compatibility, and evidence ownership;
|
||||
resolution tests cover heterogeneous variants and effective identity; and
|
||||
preparation tests own all-before-parse construction and fingerprint
|
||||
collection. The missing module-reference collision case is recorded in
|
||||
PIPE-001 rather than as a separate test-only finding.
|
||||
|
||||
## Validation Record
|
||||
|
||||
| Date | Scope | Command or check | Result |
|
||||
@@ -292,6 +447,10 @@ Finding template for later audit stages:
|
||||
| 2026-08-08 | YAML decoder contract | `go doc gopkg.in/yaml.v3.Decoder.Decode` and `ParseFileConfigYAML` call trace | Decode consumes the next document; no EOF/second-document check, recorded as CFGCLI-001 |
|
||||
| 2026-08-08 | Focused configuration and CLI tests | `go test ./internal/core/config ./internal/cli` | Pass |
|
||||
| 2026-08-08 | Focused configuration and CLI static analysis | `go vet ./internal/core/config ./internal/cli` | Pass |
|
||||
| 2026-08-08 | Audit target integrity before pipeline composition review | `git diff --quiet 92e89076a268089e703978fb9d7176200e93344c..HEAD -- . ':(exclude)docs/roadmap/**'` | Pass; production target unchanged |
|
||||
| 2026-08-08 | Pipeline graph review | Architecture, complexity query, exact symbol reads, and call traces for `ResolvePipeline`, binding normalization, typed registries, `Prepare`, and checkpoint fingerprints | Pass; PIPE-001 and PIPE-002 recorded; generated handoff execution deferred to the next area |
|
||||
| 2026-08-08 | Focused contracts and pipeline tests | `go test ./internal/framework/contracts ./internal/framework/pipeline` | Pass |
|
||||
| 2026-08-08 | Focused contracts and pipeline static analysis | `go vet ./internal/framework/contracts ./internal/framework/pipeline` | Pass |
|
||||
|
||||
## Coverage Matrix
|
||||
|
||||
@@ -299,7 +458,7 @@ Finding template for later audit stages:
|
||||
| --- | --- | --- | --- | --- |
|
||||
| Architecture and dependency boundaries | Reviewed | Architecture, documentation, and testing policies; internal overview; accepted ADRs; `internal/cli/catalog.go`; production registrars; root `assets` package; representative pipeline, typed-codec, output, and state-owner symbols | Full baseline, fresh graph, direct import map, cross-layer call traces, ADR link scan | ARCH-001 |
|
||||
| Configuration and CLI composition | Reviewed | `docs/config.md`, `docs/cli.md`, `docs/operations.md`, internal configuration/CLI docs; `internal/core/config/`; CLI run, catalog, session, profile, result, and terminal owners; focused config, command, run, reference, recomputation, session, production, example, result, and state tests | Target-integrity check, graph call/data-owner traces, YAML decoder contract, focused tests and vet | CFGCLI-001, CFGCLI-002 |
|
||||
| Pipeline resolution, preparation, and typed registries | Pending | — | — | — |
|
||||
| Pipeline resolution, preparation, and typed registries | Reviewed | Internal pipeline/module docs; `internal/framework/contracts/`; pipeline profile, options, module, construction, preparation, fingerprint, stage/validator/chain/codec/evidence registry implementations and focused tests | Target-integrity check, graph architecture/complexity/call traces, focused tests and vet | PIPE-001, PIPE-002 |
|
||||
| References and ordered handoffs | Pending | — | — | — |
|
||||
| Execution, validation, retry, and concurrency | Pending | — | — | — |
|
||||
| State, checkpoints, debugging, and file safety | Pending | — | — | — |
|
||||
|
||||
Reference in New Issue
Block a user