314 lines
19 KiB
Markdown
314 lines
19 KiB
Markdown
# Codebase Audit
|
||
|
||
## Audit Metadata
|
||
|
||
- **Production target:** `92e89076a268089e703978fb9d7176200e93344c`
|
||
- **Branch at target:** `main`
|
||
- **Audit date:** 2026-08-08
|
||
- **Go version:** `go1.26.5 linux/amd64`
|
||
- **PromptKit version:** `gitea.maximumdirect.net/eric/promptkit v0.5.0`
|
||
- **Knowledge-graph project:** `notarius-audit-92e8907`
|
||
- **Knowledge-graph target:** branch `main`, head
|
||
`92e89076a268089e703978fb9d7176200e93344c`
|
||
- **Initial worktree:** Clean. There were no pre-existing production or roadmap
|
||
changes to record.
|
||
|
||
The commit above is the production snapshot under audit. Later commits that
|
||
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.
|
||
|
||
## Finding Index
|
||
|
||
Final cross-area ordering is pending synthesis.
|
||
|
||
| ID | Severity | Category | Title |
|
||
| --- | --- | --- | --- |
|
||
| 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 |
|
||
|
||
## Findings
|
||
|
||
### Architecture And Dependency Boundaries
|
||
|
||
### ARCH-001 — Repair broken ADR cross-references
|
||
|
||
- **Severity:** Low
|
||
- **Category:** Documentation/Comments
|
||
- **Evidence:**
|
||
`docs/adr/0012-resolve-opaque-entity-identifiers-deterministically.md:15`
|
||
links ADR-0003 as `0003-strongly-typed-stage-interfaces.md`, and line 17
|
||
links ADR-0009 as
|
||
`0009-prefer-minimal-evidence-grounded-extraction-artifacts.md`. Neither file
|
||
exists. The maintained files are
|
||
`0003-typed-interfaces-with-two-zone-data-model.md` and
|
||
`0009-minimal-evidence-grounded-extraction-artifacts.md`.
|
||
- **Impact:** Readers and documentation tooling cannot follow ADR-0012 to the
|
||
two architectural decisions it explicitly relies on. Runtime behavior is
|
||
unaffected.
|
||
- **Recommendation:** Correct only the two relative link targets in ADR-0012.
|
||
- **Preserve:** Keep the accepted decision text and its intended references to
|
||
ADR-0003 and ADR-0009 unchanged.
|
||
- **Validation:** Run a relative Markdown-link check across `docs/adr/` and
|
||
confirm both targets resolve; verify the change contains no decision-text
|
||
edits.
|
||
- **Grouping:** Independent.
|
||
|
||
### Configuration And CLI Composition
|
||
|
||
### CFGCLI-001 — Reject additional YAML documents
|
||
|
||
- **Severity:** Medium
|
||
- **Category:** Correctness
|
||
- **Evidence:** `internal/core/config/file_config.go:327`–`353`
|
||
constructs a `yaml.Decoder`, enables `KnownFields`, and calls `Decode` only
|
||
once. `yaml.Decoder.Decode` reads the next YAML document, so input such as a
|
||
valid version 4 configuration followed by `---` and another configuration is
|
||
accepted with the second document ignored. The strict file tests in
|
||
`internal/core/config/file_config_contract_test.go` cover malformed values,
|
||
unknown fields, and duplicate normalized identifiers, but not an additional
|
||
document.
|
||
- **Impact:** An operator can append a syntactically valid configuration
|
||
document, receive a successful validation result, and then run with only the
|
||
first document. Settings in the ignored document—including operational or
|
||
pipeline settings—have no effect without a diagnostic, contradicting the
|
||
documented strict single-file model.
|
||
- **Recommendation:** After decoding `FileConfig`, decode once more and require
|
||
`io.EOF`; reject any second document, including an empty or malformed one,
|
||
with a contextual configuration error.
|
||
- **Preserve:** Keep version gating before the full strict decode, unknown-field
|
||
and duplicate-key rejection, and the existing defaults → file → environment
|
||
precedence unchanged.
|
||
- **Validation:** Add focused parser cases for a second valid document, a
|
||
second malformed document, and ordinary trailing whitespace/comments; run
|
||
`go test ./internal/core/config ./internal/cli`.
|
||
- **Grouping:** Independent.
|
||
|
||
### CFGCLI-002 — Reject a blank command-level LLM profile
|
||
|
||
- **Severity:** Low
|
||
- **Category:** Correctness
|
||
- **Evidence:** `internal/cli/run.go:149`–`166` registers `--llm-profile` as a
|
||
plain string flag, while the command's presence-aware empty-value checks
|
||
cover session ID, reasoning effort, output/debug directories, and recompute
|
||
step but not this flag. `runPipelineCommand` passes the resulting string to
|
||
`Config.Resolve`; `internal/framework/pipeline/profile.go:1255`–`1277` trims
|
||
an empty override and treats it as absent. The run contract tests cover a
|
||
valid override and an unknown non-empty profile, but not an explicitly
|
||
supplied blank value.
|
||
- **Impact:** A shell expansion such as `--llm-profile "$PROFILE"` with an
|
||
unset or blank value succeeds by silently using binding, pipeline, or
|
||
PromptKit defaults. The run can therefore use a different model/profile than
|
||
the operator explicitly intended to select.
|
||
- **Recommendation:** Make the CLI flag presence-aware and reject an explicitly
|
||
supplied empty or whitespace-only profile ID as command syntax before config
|
||
loading or physical-state allocation.
|
||
- **Preserve:** Keep a genuinely omitted override optional, trim non-empty IDs,
|
||
retain command → binding → pipeline → PromptKit precedence, and continue to
|
||
apply overrides only to selected LLM-backed bindings and validators.
|
||
- **Validation:** Add command-contract cases for blank and whitespace-only
|
||
values that assert exit status 2 and no state allocation, plus retain the
|
||
valid and unknown-profile run cases; run `go test ./internal/cli`.
|
||
- **Grouping:** Independent.
|
||
|
||
<!--
|
||
Finding template for later audit stages:
|
||
|
||
### AREA-001 — Concise title
|
||
|
||
- **Severity:** High, Medium, or Low
|
||
- **Category:** Correctness, Efficiency, Duplication, Simplicity, Test Quality,
|
||
or Documentation/Comments
|
||
- **Evidence:** Exact files, symbols, call paths, and observed behavior
|
||
- **Impact:** The concrete risk or cost
|
||
- **Recommendation:** A bounded implementation direction
|
||
- **Preserve:** Invariants and contracts that remediation must retain
|
||
- **Validation:** Focused checks that would demonstrate success
|
||
- **Grouping:** Independent, or the IDs with which this should be implemented
|
||
-->
|
||
|
||
## Intentional Complexity And Duplication To Preserve
|
||
|
||
- `internal/modules/generic/register.Register`,
|
||
`internal/modules/seriatim/register.Register`, and
|
||
`internal/modules/dnd/register.Register` deliberately expose the same small
|
||
registrar shape while retaining family-local registration policy and
|
||
diagnostics. Combining them would move extension ownership out of the domain
|
||
registrars and weaken the composition boundary established by ADR-0004.
|
||
- `internal/modules/dnd/register.registerModules`, `registerEvidence`,
|
||
`registerValidators`, and `registerDefaultChains` use explicit typed
|
||
registration lists. At this architectural pass, that repetition preserves
|
||
artifact Go types, module-specific validator order, and registrar-owned
|
||
production policy. Later D&D stages may evaluate individual shared
|
||
mechanics, but should not replace these lists with a dynamically typed
|
||
registration engine.
|
||
- `internal/framework/pipeline.RegisterArtifactCodec` and
|
||
`exactTypedValue` perform apparently repetitive exact-type checks around
|
||
private erasure. The checks deliberately turn incompatible values into
|
||
errors at each erased boundary rather than permitting a panic or accepting a
|
||
near-matching type, preserving ADR-0003.
|
||
- `internal/cli.runPipelineCommand` is a large linear orchestrator, but its
|
||
ordering is policy: syntax and config rejection precede run identity and
|
||
debug allocation; resolution and profile inspection precede module
|
||
preparation and input parsing; framework success precedes durable output;
|
||
and terminal debug publication precedes the optional JSON receipt. Existing
|
||
helpers isolate reference selection, recomputation, stores, output, result
|
||
encoding, and terminal error precedence. A generic lifecycle abstraction
|
||
would hide physical-state allocation and publication boundaries; bounded
|
||
parsing fixes such as CFGCLI-002 should not reorganize that lifecycle.
|
||
- `internal/core/config.validatePipelineProfiles` explicitly walks pipelines,
|
||
ordered steps, lanes, bindings, and references. Its nested structure mirrors
|
||
the public configuration shape and retains the nearest pipeline/step/lane
|
||
context in errors. Replacing it with a reflection-driven validator would
|
||
weaken those diagnostics and the presence-aware file-model boundary.
|
||
- `internal/cli.selectedReferenceTargets` and `recomputePolicy` perform
|
||
explicit resolved-shape traversals for distinct CLI policies: disambiguating
|
||
reference selectors against selected module capabilities, and computing the
|
||
forward forced/backward reusable checkpoint closure. Keeping these typed
|
||
traversals separate avoids adding command syntax or checkpoint policy to the
|
||
framework resolver.
|
||
|
||
## Areas Reviewed Without Findings
|
||
|
||
### Architecture And Dependency Boundaries
|
||
|
||
- **Composition root:** `internal/cli.newProductionComponents` constructs the
|
||
complete registry set and asset registry, then invokes only the generic,
|
||
Seriatim, and D&D family registrars. Direct production imports confirm that
|
||
`internal/cli` is the only layer importing those registrar packages.
|
||
- **Dependency direction:** A direct production import map found no core or
|
||
framework package importing `internal/modules`, no module importing
|
||
`internal/cli`, no concrete generic or Seriatim module importing D&D, and no
|
||
module importing the file-backed checkpoint, chunk-plan, debug, file-I/O, or
|
||
debug-bundle implementations. PromptKit is imported directly only by
|
||
`internal/framework/llm`.
|
||
- **Graph cross-layer calls:** The refreshed graph reported one
|
||
framework-to-module edge from `pipeline.Prepare` to a symbol named `request`
|
||
in a D&D validator test. Tracing it showed a confidence `0.06` suffix match
|
||
from the local closure call in `Prepare`; `trace_path` classified the target
|
||
as test-only, and the production import map disproved a dependency. The graph
|
||
reported no module-to-CLI calls.
|
||
- **Assets leaf:** `assets/package.go` imports only `embed` and `io/fs`, embeds
|
||
content, and exposes the read-only `FS() fs.FS` accessor. It contains no
|
||
business logic and has no `internal` or PromptKit dependency.
|
||
- **Fixed pipeline shape:** `pipeline.ResolvePipeline` resolves input and
|
||
chunk once, fixed extract/merge/normalize bindings per artifact lane, and one
|
||
output binding. Ordered steps are barriers around those fixed lanes rather
|
||
than arbitrary graph topology. `pipeline.Prepare`, `Runner.Run`,
|
||
`runPreparedSteps`, and `runLanes` retain that shape through construction and
|
||
execution.
|
||
- **Typed artifact boundary:** Typed registrations retain the exact Go type for
|
||
codecs and lane operations. Private erasure in `RegisterArtifactCodec` and
|
||
`exactTypedValue` verifies exact types and returns contextual errors;
|
||
normalized values cross into output through serialized artifacts.
|
||
- **Physical-state ownership:** The CLI owns root selection, store factories,
|
||
and durable file placement (`chunkPlanStoreForRun`, checkpoint/debug setup,
|
||
and `writeOutputFiles`). The framework receives collaborator interfaces and
|
||
returns logical output files. The generic JSON output module's direct import
|
||
of `internal/framework/chunkmap` validates and republishes the accepted
|
||
serialized chunk-map contract; it neither chooses a physical root nor writes
|
||
files.
|
||
- **Accepted architectural decisions:** ADRs 0001–0005 and 0007–0012 were read
|
||
against the current high-level composition. Apart from ARCH-001, the
|
||
composition root, fixed ordered pipeline, typed boundary, domain packaging,
|
||
canonical chunk-plan policy, separate state surfaces, checkpoint policy,
|
||
evidence rules, workload profile ownership, centralized asset leaf, and
|
||
deterministic entity identity boundary have corresponding current owners.
|
||
|
||
### Configuration And CLI Composition
|
||
|
||
- **End-to-end command path:** `RunWithOptions` normalizes injectable process
|
||
collaborators once and dispatches to `runPipelineCommand`. The run command
|
||
parses and normalizes command input, discovers and loads configuration,
|
||
applies command overrides, builds the effective catalog, resolves reference
|
||
selectors and the pipeline, inspects effective profiles, materializes
|
||
references, constructs runtime/state collaborators, invokes `pipeline.Run`,
|
||
publishes output files, terminalizes debug state, and only then publishes a
|
||
requested JSON receipt.
|
||
- **Precedence and resolution:** `loadConfig` enforces explicit `--config` over
|
||
`NOTARIUS_CONFIG` over the system default, then applies `Default`, file
|
||
configuration, supported environment overrides, and run-only CLI overrides
|
||
in order. `Config.Resolve` recomputes derived worker defaults, validates,
|
||
clones the selected profile, and delegates catalog-dependent composition to
|
||
the framework resolver. Apart from CFGCLI-001 and CFGCLI-002, unknown fields,
|
||
malformed values, normalized-key collisions, unknown command flags, and
|
||
invalid selected modules/options are rejected at their owning boundary.
|
||
- **Profile-source equality:** Validation-time
|
||
`validateExplicitPromptKitProfiles` and runtime
|
||
`buildProductionLLMClient` pass the same profile directory, profile file,
|
||
mapped local backend, and shared fallback asset registry. Effective profile
|
||
collection is sorted, deduplicated, and limited to selected LLM-backed
|
||
modules and validators, so inspection and runtime selection use the resolved
|
||
profile values rather than recomputing inheritance.
|
||
- **Session identity:** `resolvePromptSessionID` uses a versioned SHA-256 value
|
||
over the trimmed resolved input-module key, a separator, and exact raw input
|
||
bytes. It contains no pipeline ID, reference, profile, retry, input path,
|
||
working directory, or run ID; an explicit non-empty session replaces the
|
||
generated value. Run contracts verify the same effective session reaches all
|
||
prompt-facing requests, manifests, debug metadata, and checkpoint identity.
|
||
- **Reference and recomputation controls:** CLI reference selectors are
|
||
resolved only against selected chunk/extract/merge/normalize capabilities
|
||
before the authoritative effective resolution. Recompute policy is derived
|
||
after reference materialization, forces the requested step and transitive
|
||
consumers, and requires reusable checkpoints for non-forced transitive
|
||
producers. Focused contract tests exercise selector ambiguity, lane
|
||
selection, ordered dependency closure, and execution behavior.
|
||
- **Publication and terminal outcomes:** Syntax/config failures before run
|
||
identity allocate no output or debug state. After debug allocation,
|
||
resolution, profile, preparation, input, framework, partial-summary, and
|
||
output failures all pass through `failPipelineCommand`. `terminalize` writes
|
||
a run report once, preserves an existing primary failure over report/error-log
|
||
failures, promotes a success-report failure to primary, and reports other
|
||
persistence failures secondarily. Framework cancellation follows the same
|
||
wrapped primary-error path. Durable outputs are attempted only after runner
|
||
success; a JSON result is encoded before output publication but written to
|
||
stdout only after output and debug terminalization. A receipt-delivery
|
||
failure leaves already published bundles intact and returns failure.
|
||
- **Test ownership:** Configuration tests own strict file/env application,
|
||
structural validation, effective cloning/digests, and redaction. CLI command,
|
||
run, reference, recomputation, session, production, example, result, and state
|
||
contracts assert process-level ordering and side effects rather than merely
|
||
repeating lower-level resolver assertions. The two uncovered command/parser
|
||
cases are recorded as CFGCLI-001 and CFGCLI-002.
|
||
|
||
## Validation Record
|
||
|
||
| Date | Scope | Command or check | Result |
|
||
| --- | --- | --- | --- |
|
||
| 2026-08-08 | Initial worktree | `git status --short` | Pass; no output |
|
||
| 2026-08-08 | Knowledge graph | Full index as `notarius-audit-92e8907` | Pass; 8,322 nodes and 47,887 edges; branch/head matched the production target |
|
||
| 2026-08-08 | Baseline tests | `go test ./...` | Pass |
|
||
| 2026-08-08 | Baseline static analysis | `go vet ./...` | Pass |
|
||
| 2026-08-08 | Baseline build | `go build ./cmd/notarius` | Pass |
|
||
| 2026-08-08 | Baseline whitespace | `git diff --check` | Pass |
|
||
| 2026-08-08 | Production imports | Direct `go list` import-edge audit plus graph call tracing | Pass; no production dependency inversion found |
|
||
| 2026-08-08 | Accepted ADR links | Relative Markdown-link target scan under `docs/adr/` | Two unresolved targets recorded as ARCH-001 |
|
||
| 2026-08-08 | Audit target integrity before configuration/CLI review | `git diff --quiet 92e89076a268089e703978fb9d7176200e93344c..HEAD -- . ':(exclude)docs/roadmap/**'` | Pass; production target unchanged |
|
||
| 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 |
|
||
|
||
## Coverage Matrix
|
||
|
||
| Audit area | Status | Packages and documents inspected | Validation run | Finding IDs |
|
||
| --- | --- | --- | --- | --- |
|
||
| 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 | — | — | — |
|
||
| References and ordered handoffs | Pending | — | — | — |
|
||
| Execution, validation, retry, and concurrency | Pending | — | — | — |
|
||
| State, checkpoints, debugging, and file safety | Pending | — | — | — |
|
||
| LLM runtime, prompt filesystems, and assets | Pending | — | — | — |
|
||
| Generic and Seriatim modules | Pending | — | — | — |
|
||
| Shared D&D types, codecs, and family mechanics | Pending | — | — | — |
|
||
| NPC, item, and location registries | Pending | — | — | — |
|
||
| NPC, item, and location occurrences | Pending | — | — | — |
|
||
| Spells, scene chunking, and scene descriptions | Pending | — | — | — |
|
||
| Combat turns and enemy events | Pending | — | — | — |
|
||
| Test ownership, comments, and final synthesis | Pending | — | — | — |
|