Add an audit plan and staged audit sequence to identify opportunities for code quality improvement

2026-08-08 20:42:35 +00:00
parent d9b87347b8
commit 92e89076a2
2 changed files with 1521 additions and 0 deletions
--- a/docs/roadmap/audit-plan.md
+++ b/docs/roadmap/audit-plan.md
@@ -0,0 +1,516 @@
 # Codebase Audit Plan
 ## Purpose
 This document defines a repository-wide audit of Notarius for correctness,
 efficiency, maintainability, and clarity. The audit should identify concrete
 improvements without treating abstraction, fewer lines, or higher test coverage
 as goals in themselves.
 The audit is intentionally separate from implementation. Its findings should
 be evidence-backed and sufficiently specific to support a later remediation
 roadmap, but the audit should not modify production code, tests, assets, or
 current-behavior documentation.
 ## Governing Principles
 The audit must preserve the architecture and testing policies in
 `docs/policy/architecture.md` and `docs/policy/testing.md`.
 In particular:
 - Notarius remains a fixed, staged pipeline rather than a general workflow
  engine.
 - Generic framework packages must remain domain-neutral, and production
  modules must not acquire CLI or physical-state responsibilities.
 - Typed artifact boundaries, exact codec compatibility, deterministic ordering,
  whole-output validation, and generated-reference provenance are correctness
  properties, not incidental complexity to be optimized away.
 - The root `assets` package remains a content-only dependency leaf.
 - Shared helpers should protect demonstrated common semantics. Similar-looking
  code with different ownership, error policy, identity rules, or type contracts
  should remain separate.
 - Tests should protect durable behavior and meaningful risks. The audit should
  not recommend tests merely to increase coverage or freeze implementation
  details.
 - Efficiency claims must distinguish measured or structurally credible costs
  from cosmetic line-count reductions. Optimizing local CPU work that is
  insignificant beside an LLM call is low priority unless it also simplifies
  correctness or applies to large inputs.
 ## Audit Questions
 Every audited area should be examined through the following questions.
 ### Correctness
 - Are documented architecture invariants enforced at the correct boundary?
 - Can invalid configuration, incompatible artifact types, malformed references,
  or unavailable dependencies reach execution when they could be rejected
  during resolution or preparation?
 - Are nil, empty, absent, rejected, failed, and canceled states distinguished
  consistently?
 - Are stored or returned slices, maps, byte slices, options, metadata, source
  documents, references, and artifacts defensively owned where required?
 - Are public ordering, selected errors, warnings, and checkpoint decisions
  deterministic regardless of map or goroutine completion order?
 - Do cancellation, retry, validation, and partial-work semantics match their
  documented ownership?
 - Do checkpoint and chunk-plan identities include every semantic dependency and
  exclude scheduling-only or diagnostic state?
 - Can auxiliary references accidentally become source evidence, or can
  generated references bypass codec, schema, provenance, or step-order checks?
 - Can provider-specific values, credentials, or source content escape through
  errors, manifests, debug summaries, cache state, or logs?
 - Do schemas, codecs, candidate decoders, normalizers, and validators agree on
  the exact durable contract without silently accepting incompatible shapes?
 ### Duplication And Shared Mechanics
 - Which exact or near-duplicate implementations express the same invariant and
  failure policy?
 - Has duplicated code already drifted in naming, nil handling, canonicalization,
  metadata, fingerprints, validation, or diagnostics?
 - Would a helper have a natural owner and a smaller, clearer contract than the
  duplicated callers?
 - Can an extraction preserve static typing and package ownership, or would it
  require reflection, `any`, callbacks with many policy parameters, or a
  domain-neutral package importing domain concepts?
 - Is repeated code required by a small interface adapter or typed registration
  boundary and therefore clearer when left explicit?
 As a default heuristic, prioritize a shared helper when identical semantics
 appear in three or more production sites, or in two sites where divergence
 would create a meaningful correctness risk. Do not use that heuristic as a
 quota: one substantial duplicate may warrant extraction, while widespread
 one-line interface methods may not.
 ### Simplicity And Idiomatic Go
 - Does a function combine orchestration, policy, transformation, persistence,
  and reporting that could be separated along existing ownership boundaries?
 - Are repeated scans, sorts, conversions, clones, encodes, or decodes doing work
  that can safely occur once?
 - Are intermediate representations necessary, or can a value be validated,
  canonicalized, and mapped in one comprehensible pass?
 - Are maps, sets, stable sorts, generics, standard-library helpers, and error
  wrapping used idiomatically?
 - Are abstractions earning their complexity, or are interfaces, option layers,
  wrappers, aliases, compatibility paths, and private types left over after a
  completed migration?
 - Are there unreachable error branches, redundant fingerprints or digests,
  duplicated sources of truth, or accessors used only by tests?
 - Can a smaller implementation preserve exact observable behavior and safety
  properties?
 ### Explanatory Comments
 Comments should be recommended where the code is necessarily complex because
 it preserves a non-obvious invariant. Good candidates include:
 - concurrency coordination, cancellation, and stable error selection;
 - checkpoint identity, reuse, forced recomputation, and dependency invalidation;
 - typed erasure and restoration at framework boundaries;
 - generated-reference ordering and provenance;
 - canonicalization and identity resolution where registry evidence differs
  from occurrence evidence;
 - prompt ordering or input identity required for backend caching; and
 - path confinement, atomic publication, redaction, or terminal error precedence.
 Recommend comments that explain *why* a step or ordering constraint exists and
 what would break if it changed. Do not recommend comments that narrate syntax,
 repeat a function name, duplicate current-behavior documentation, or preserve
 implementation history.
 ### Tests
 - Is each consequential invariant protected at the narrowest stable boundary?
 - Are concurrency, cancellation, retries, recovery, compatibility, path safety,
  and data-integrity behavior credibly exercised?
 - Do higher-level contract tests duplicate lower-level cases without adding
  integration confidence?
 - Are tests coupled to private constants, helper shape, exact prose, full error
  strings, or collaborator choreography rather than behavior?
 - Can repetitive fixtures or fakes be simplified without creating a test
  framework more complex than the tests?
 - Would a focused fuzz test, race test, or package-level invariant test protect
  a realistic risk better than several example tests?
 ## Evidence And Finding Standards
 Static metrics and textual similarity are discovery aids, not findings. A long
 function may be a clear linear coordinator; identical methods may be useful
 typed adapters. Every reported finding must include:
 1. a concise title and severity;
 2. exact files and symbols;
 3. the observed behavior or structural evidence;
 4. the correctness, efficiency, maintenance, or comprehension impact;
 5. a concrete recommended direction;
 6. important invariants the remediation must preserve;
 7. focused validation that would demonstrate success; and
 8. whether the recommendation is independent or should be grouped with another
   finding.
 Use these severities:
 - **High:** a credible risk of corrupt output, unsafe state handling, secret
  exposure, stale reuse, deadlock, nondeterminism, or violated external
  contract.
 - **Medium:** a plausible behavioral defect, meaningful wasted work on common
  paths, or complexity/duplication likely to cause future correctness drift.
 - **Low:** a contained simplification, small efficiency improvement, dead code,
  naming issue, or missing explanation with no current behavioral failure.
 The audit should explicitly record examined areas with no findings. This makes
 coverage visible and prevents later agents from repeatedly rediscovering the
 same safe design.
 ## Repository Areas
 ### 1. Architecture And Dependency Boundaries
 Inspect `docs/policy/architecture.md`, `docs/adr/`, `docs/internal/overview.md`,
 package imports, module registrars, and the CLI composition root.
 Look for:
 - framework or core code depending on production modules;
 - modules depending on CLI, physical roots, or provider-specific types;
 - domain knowledge placed in generic helpers;
 - duplicated registries or composition policy outside the owning registrar;
 - abstractions that turn the fixed pipeline into an implicit general graph; and
 - current code that no longer matches an accepted ADR or documented invariant.
 Graph-reported cross-layer calls must be traced before being classified because
 tests and interface implementations can resemble dependency inversions without
 creating a production import violation.
 ### 2. Configuration And CLI Composition
 Inspect `internal/core/config`, `internal/cli`, configuration parsing and
 redaction tests, profile construction, session derivation, catalog assembly,
 reference overrides, run-result handling, terminal reporting, and maintained
 example contract tests.
 Pay particular attention to the currently dense paths around
 `runPipelineCommand`, configuration profile validation, selected reference
 targets, recomputation policy, and option normalization. Determine whether
 their complexity reflects necessary composition or mixed responsibilities that
 can be separated without moving policy into the framework.
 Verify:
 - file, environment, CLI, pipeline, binding, and prompt-default precedence;
 - consistent strict option and unknown-field handling;
 - session identity independence from references and pipeline-local changes;
 - effective profile and runtime fingerprint consistency;
 - redaction before errors or debug/manifest boundaries;
 - output publication only after framework success; and
 - one guarded terminalization path that preserves the primary failure.
 ### 3. Pipeline Resolution, Preparation, And Typed Registries
 Inspect `internal/framework/pipeline/profile.go`, `prepare.go`, registry files,
 `options.go`, `references.go`, `handoff.go`, `construction.go`, typed contracts,
 and their focused tests.
 This area deserves a dedicated pass because the current graph identifies
 `ResolvePipeline`, generated-binding validation, reference-target resolution,
 and generated-reference construction as high-complexity or high-fan-in code.
 Verify:
 - static failures occur before source parsing;
 - selected and unselected lanes do not contaminate each other's requirements;
 - stage defaults and overrides have one canonical resolution path;
 - typed registration and private erasure cannot panic or accept near-matching
  artifact types;
 - generated bindings reject cycles, forward references, ambiguity, wrong kinds,
  and missing accepted normalized producers;
 - materialized reference bytes and options are cloned and bounded; and
 - resolved composition and prepared fingerprints include the complete semantic
  policy exactly once.
 Compare input, chunker, extractor, merger, normalizer, output, validator, codec,
 evidence-projector, and validator-chain registries for shared mechanics and
 intentional differences. Repeated typed registration code is a candidate only
 if a helper can retain useful compile-time guarantees and stage-specific
 diagnostics.
 ### 4. Pipeline Execution, Validation, Retry, And Concurrency
 Inspect `runner*.go`, `typed_execution.go`, `runner_typed.go`,
 `runner_concurrent.go`, validation-chain execution, normalize retry behavior,
 synchronized collaborators, and the concurrency, cancellation, retry, debug,
 and checkpoint tests.
 Trace complete paths rather than reviewing helper files in isolation:
 - source and chunk-plan selection through chunk validation;
 - deterministic chunk-first/lane-second dispatch;
 - lane extraction through merge and normalize continuations;
 - rejection versus framework-error propagation;
 - cancellation before dispatch, while queued, and while running;
 - retry attempts and warning retention;
 - stable error selection after concurrent completion;
 - checkpoint hydration back into typed execution; and
 - output suppression after a framework error.
 Look for goroutine leaks, unbounded work, lock-order risks, double release or
 double recording, races on shared result state, unnecessary serialization,
 and repeated canonicalization. Comments are especially valuable here when they
 explain ordering or cancellation invariants that are not apparent from local
 control flow.
 ### 5. State, Checkpoints, Chunk Plans, Debugging, And File Safety
 Inspect `internal/framework/checkpoint`, `chunkplan`, `chunkmap`, `debug`,
 `evidencecontext`, `internal/core/fileio`, `debugbundle`, and their CLI
 composition.
 Verify:
 - narrow path validation and symlink-resistant confinement;
 - atomic writes and recoverable explicit cleanup;
 - separation of checkpoint recording, resume loading, chunk-plan caching, and
  debug capture;
 - canonical encoding before content identity is trusted;
 - complete but non-secret checkpoint fingerprints;
 - correct ordinary-resume and selective-recompute behavior;
 - producer dependency invalidation across ordered steps;
 - immutable hydration and no aliasing with stored bytes;
 - debug data never influencing execution or reuse; and
 - terminal persistence failures never obscuring the primary error.
 Review the repeated extract/merge/normalize recorder and loader methods, path
 component validators in multiple state packages, and clone/encode/decode paths.
 Determine which repetition is a clear stage adapter and which can share a
 private primitive without weakening reason-code ownership or diagnostics.
 ### 6. LLM Runtime, Prompt Filesystems, And Assets
 Inspect `internal/framework/llm`, `promptfs`, the PromptKit integration,
 scheduler, profile-source construction, prompt/schema registries, root
 `assets`, module prompt manifests, and relevant D&D shared assets.
 Verify:
 - every provider call passes through the shared scheduler and cancellation
  removes queued calls safely;
 - PromptKit and Notarius concurrency limits compose as documented;
 - profile inspection and runtime use identical source precedence;
 - session IDs, profile-source fingerprints, prompt fingerprints, and schema
  fingerprints reflect the intended semantic inputs;
 - secrets and provider-specific error types do not cross the boundary;
 - prompt inputs and private outputs do not expose opaque entity IDs;
 - prompt ordering, stable prefixes, and cache controls remain intentional;
 - schema loaders and filesystem adapters validate once and return defensive
  data; and
 - the root assets package contains no business logic.
 Compare the LLM asset registry and prompt-filesystem adapters for duplicated
 filesystem behavior. Review repeated prompt/schema loader and metadata code in
 module packages, but reject an extraction that would centralize domain prompt
 ownership or make unrelated assets share one invalidation boundary.
 ### 7. Generic And Seriatim Modules
 Inspect `internal/modules/generic` and `internal/modules/seriatim`, including
 module specs, option decoding, chunk planning, input translation, validators,
 output encoding, evidence-context publication, registration, and tests.
 Verify that:
 - external Seriatim details end at the input boundary;
 - generic chunking and output remain domain-neutral;
 - chunk plans and source units preserve source-addressed invariants;
 - output logical names are safe and deterministic;
 - output options do not bypass preparation-time compatibility checks; and
 - option decoding is strict, small, and consistent with configuration
  validation.
 The graph flags generic integer option parsing and JSON output policy decoding
 as relatively complex. Examine whether that is inherent strict decoding or an
 opportunity for a smaller typed parser with equally precise diagnostics.
 ### 8. D&D Domain Model, Codecs, And Shared Helpers
 Inspect `internal/modules/dnd` domain types, codecs, candidate decoders,
 identity packages, registries, shared source-reference helpers, diagnostics,
 registry resolution, entity reconciliation, mergers, registrar, and assets.
 Compare all ten current artifact families. Build a convention matrix covering:
 - module specs and execution classes;
 - constructor and option behavior;
 - manifest metadata and checkpoint fingerprints;
 - response-schema loading and private-versus-durable types;
 - source-reference conversion, canonicalization, ordering, and deduplication;
 - nil versus present-empty output;
 - codecs and strict JSON behavior;
 - registry lookup, identity derivation, immutable projections, and resolution;
 - normalizer retry/fallback behavior;
 - validators and default chains; and
 - registration, prompt assets, and documentation ownership.
 The graph reports many exact similarities among codec `Decode` methods,
 fingerprint/metadata methods, registry extractors, identity helpers, occurrence
 normalizers, and validators. Treat these as a prioritized review list, not an
 instruction to create one generic D&D engine. A worthwhile helper must preserve
 domain-specific identity, evidence, kind ordering, validation, diagnostics,
 and artifact typing.
 ### 9. D&D Extraction And Normalization Flows
 Trace each lane end to end rather than auditing only similarly named files:
 - spells;
 - NPC registry and NPC occurrences;
 - combat turns and enemy events;
 - item registry and item occurrences;
 - scene descriptions; and
 - location registry and location occurrences.
 For registry/occurrence pairs, verify the complete semantic boundary: the model
 uses contextual evidence, deterministic code attaches opaque identity, registry
 evidence does not become occurrence evidence, and unresolved or ambiguous
 selections fail according to lane policy.
 Review whether any lane resolves or canonicalizes the same entity, source
 reference, or response twice; constructs unnecessary intermediate response
 forms; performs repeated sorts or scans; or retains transitional paths. Compare
 registry normalizers and occurrence normalizers for genuinely identical
 mechanics, while keeping currency, same-name location, NPC ambiguity, spell
 catalog, scene eligibility, and combat-specific policy with their owners.
 ### 10. Test Suite And Comment Coverage
 Review the tests associated with every preceding area after understanding the
 production contracts. This should be a cross-cutting pass, not a request to add
 tests for every flagged function.
 Identify:
 - consequential unprotected invariants;
 - duplicated policy assertions across layers;
 - brittle tests coupled to internal constants, prompt prose, or private helper
  shape;
 - oversized test harnesses and repeated fixtures that obscure intent;
 - race-sensitive code not exercised under `-race`;
 - parsers, canonicalizers, and path handlers where fuzzing would address a real
  input-space risk; and
 - complex production code whose tests reveal an unclear ownership boundary.
 Also identify necessarily complex symbols that lack a concise invariant-level
 comment. Comment recommendations should name the exact symbol and the fact the
 comment should explain; “add more comments” is not an actionable finding.
 ## Efficiency Evaluation
 The audit should consider both runtime and maintenance efficiency.
 For runtime efficiency, examine algorithmic behavior relative to realistic
 input dimensions: source units, chunks, lanes, references, artifacts, registry
 records, checkpoint files, and prompt assets. Prioritize repeated full-input
 passes, nested linear lookup, unnecessary JSON round trips, repeated hashing,
 large defensive copies at adjacent ownership boundaries, and serialization on
 concurrent hot paths. Preserve a defensive copy when it establishes ownership;
 removing it solely to reduce allocation is not an improvement.
 For maintenance efficiency, prioritize repeated policy, parallel type systems,
 duplicated error classification, scattered defaults, and migrations that left
 two ways to perform the same operation. Boilerplate is costly only when it can
 drift or obscures the semantic core. Small explicit typed adapters can be more
 maintainable than a generic abstraction.
 Do not recommend caching, pooling, concurrency, or a benchmark without naming
 the workload and risk it addresses. Add a benchmark only when a proposed
 optimization concerns a repeatable local path and the result would influence
 the decision.
 ## Audit Method
 Each area should use the same method:
 1. Read its architecture/internal documentation and focused tests.
 2. Map public/package contracts and trace the main call paths.
 3. Inspect high-fan-in, high-cognitive-complexity, nested-loop, and repeated-
   conversion symbols.
 4. Review exact and near-duplicate code side by side, including callers and
   failure semantics.
 5. Check dependency direction, ownership, aliasing, deterministic order,
   cancellation, and error classification.
 6. Compare tests with the risks owned at that layer.
 7. Record findings and inspected-with-no-finding areas before moving on.
 8. Run focused read-only validation when it can confirm or refute a suspected
   problem.
 Prefer the repository knowledge graph for symbol discovery, call tracing, and
 similarity candidates. Use textual search for literals, diagnostics, config
 keys, asset content, and stale names. Read complete implementations and tests
 before reporting a metric-derived candidate.
 ## Execution Sequence
 This document owns audit scope, questions, evidence standards, and the quality
 bar. [Staged Codebase Audit Sequence](audit-sequence.md) is the sole canonical
 owner of prompt order, stage boundaries, per-stage reading, validation commands,
 and acceptance criteria. Do not derive or maintain a second sequence here.
 The audit is executed as bounded prompts and writes its accumulated findings to
 `docs/roadmap/audit.md`. Later stages must build on and reconcile earlier
 evidence rather than concatenate independent reports. Implementation and
 roadmap retirement remain separate work after maintainers review the completed
 audit.
 ## Baseline And Validation
 Before the first audit stage, record the commit under review and require a clean
 worktree. Refresh the code knowledge graph so renamed or deleted code does not
 produce false findings. Run the normal offline baseline:
 ```sh
 go test ./...
 go vet ./...
 go build ./cmd/notarius
 git diff --check
 ```
 Run `go test -race` for packages with concurrency or mutable shared state,
 especially `internal/framework/pipeline`, `internal/framework/llm`, state
 packages, and D&D registries. A repository-wide race run is appropriate for
 final verification if its cost remains reasonable.
 Optional diagnostic commands should be used only when relevant:
 - `go test -count=1` to rule out cache-masked failures;
 - `go test -shuffle=on` to detect order coupling;
 - focused fuzzing for existing or newly justified fuzz targets; and
 - focused benchmarks or profiles for a specific efficiency finding.
 The audit itself should not change tests to make the baseline pass. Record any
 pre-existing failure and distinguish it from an audit finding.
 ## Deliverable Quality Bar
 The completed audit should:
 - cover every repository area listed above;
 - distinguish defects from refactoring opportunities and comment requests;
 - distinguish credible performance costs from aesthetic simplification;
 - identify intentional duplication that should remain explicit;
 - avoid recommendations that violate dependency direction or weaken typing;
 - cite exact evidence and preserve named invariants for every finding;
 - consolidate root causes rather than report many symptoms;
 - rank independent work so a later implementation plan can stage it safely;
 - recommend no code change whose expected benefit is smaller than its added
  abstraction or test-maintenance cost; and
 - leave implementation and roadmap retirement to later work.
 ## Open Questions
 None are required to begin the audit. If a later stage cannot determine whether
 behavior is intentional from code, tests, policies, ADRs, or current
 documentation, it should record the uncertainty and a recommended resolution
 rather than silently treating preference as a defect.
--- a/docs/roadmap/audit-sequence.md
+++ b/docs/roadmap/audit-sequence.md