Audit code quality and deduplication opportunities
This commit is contained in:
791
docs/roadmap/audit.md
Normal file
791
docs/roadmap/audit.md
Normal file
@@ -0,0 +1,791 @@
|
||||
# Pre-1.0 Code Quality and Deduplication Audit
|
||||
|
||||
## 1. Executive summary
|
||||
|
||||
Audita is in good shape for a limited pre-1.0 cleanup pass. The repository is small, package boundaries are mostly explicit, and the core public contract is already documented around `audita process`, config loading, output schemas, diagnostics, reports, embedded prompts, modules, and validators. The highest-value improvements are targeted centralization, not a rewrite.
|
||||
|
||||
Top three refactoring targets before 1.0:
|
||||
|
||||
1. Centralize module proposal plumbing and prompt payload construction across the four production modules.
|
||||
2. Centralize effective config loading plus schema/module catalog validation so `process`, `config print-effective`, and `config validate` cannot drift.
|
||||
3. Centralize diagnostics artifact names, stage names, and validator classification metadata used by reports and the correction ledger.
|
||||
|
||||
No major architectural risk is apparent. The main pre-1.0 risk is public-behavior drift from repeated policy strings, catalog values, artifact paths, and nearly identical command/module scaffolding.
|
||||
|
||||
This report was written to `docs/roadmap/audit.md`. `docs/roadmap/` already exists in the repository, although its previous `publish.md` file is currently deleted in the worktree by an unrelated change.
|
||||
|
||||
## 2. Repository map reviewed
|
||||
|
||||
Reviewed documentation:
|
||||
|
||||
- `README.md`
|
||||
- `docs/configuration.md`
|
||||
- `docs/architecture/architecture.md`
|
||||
- `docs/architecture/public-contract.md`
|
||||
- `docs/architecture/diagnostics.md`
|
||||
- `docs/architecture/output-schemas.md`
|
||||
- `docs/architecture/prompts.md`
|
||||
- `docs/architecture/validators.md`
|
||||
- `docs/architecture/structured-llm.md`
|
||||
- `docs/integration/subprocess-operations.md`
|
||||
- `docs/release-checklist.md`
|
||||
|
||||
Reviewed implementation areas:
|
||||
|
||||
- `cmd/audita`
|
||||
- `internal/cli`
|
||||
- `internal/core/config`
|
||||
- `internal/core/schema`
|
||||
- `internal/core/io`
|
||||
- `internal/core/normalization`
|
||||
- `internal/core/chunking`
|
||||
- `internal/core/diagnostics`
|
||||
- `internal/core/outputschema`
|
||||
- `internal/core/reporting`
|
||||
- `internal/framework/contracts`
|
||||
- `internal/framework/modules`
|
||||
- `internal/framework/proposal_generation`
|
||||
- `internal/framework/proposals`
|
||||
- `internal/framework/runner`
|
||||
- `internal/framework/validators`
|
||||
- `internal/framework/llm`
|
||||
- `internal/framework/responseschema`
|
||||
- `internal/framework/promptcontext`
|
||||
- `internal/framework/warnings`
|
||||
- `internal/modules/glossary`
|
||||
- `internal/modules/homophones`
|
||||
- `internal/modules/spoken_word`
|
||||
- `internal/modules/grammar`
|
||||
- `internal/prompts`
|
||||
- `internal/validators`
|
||||
- package tests and CLI parity/release fixtures under `internal/cli/testdata`
|
||||
|
||||
Major execution paths reviewed:
|
||||
|
||||
- `audita process <transcript.json> --glossary <glossary.yaml>`
|
||||
- `audita config validate --config <path>`
|
||||
- `audita config print-effective [--config <path>]`
|
||||
- default module sequence resolution and repeated glossary instance naming
|
||||
- proposal generation, validator execution, proposal application, report writing, diagnostics writing, and retention
|
||||
|
||||
Important absent or not-applicable areas:
|
||||
|
||||
- No `pkg/` directory exists.
|
||||
- No `examples/` directory exists.
|
||||
- No `docs/internal/` directory exists.
|
||||
- No `internal/app`, `internal/stage`, `internal/storage`, `internal/artifacts`, or `internal/manifest` packages exist. Their closest equivalents are `internal/cli`, `internal/framework/runner`, `internal/core/diagnostics`, and `internal/core/reporting`.
|
||||
|
||||
## 3. High-confidence deduplication opportunities
|
||||
|
||||
### 3.1 Module proposal plumbing is duplicated across all production modules
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/modules/glossary/module.go`
|
||||
- `internal/modules/homophones/module.go`
|
||||
- `internal/modules/spoken_word/module.go`
|
||||
- `internal/modules/grammar/module.go`
|
||||
- `internal/modules/*/prompt.go`
|
||||
- `internal/framework/proposal_generation`
|
||||
- `internal/framework/promptcontext`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- Each module has the same `Module` struct shape, `Validators` copy behavior, `Propose` flow, section transcript extraction, transcript description extraction, `proposal_generation.GenerateCandidates` request construction, prompt metadata map construction, and stage-name formatting.
|
||||
- Each module also has a near-identical prompt payload builder with local `promptSegment` and `promptTranscriptSection` types, glossary JSON marshaling, transcript section JSON marshaling, transcript description block rendering, and two-message return shape.
|
||||
- `collectSectionProposals` already passes a section transcript to each module, but each module then filters that transcript again by section metadata.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- A diagnostics or prompt-context bug fix would need to be repeated in four modules.
|
||||
- Prompt metadata fields and stage names are diagnostics-visible and could drift by module.
|
||||
- The double section filtering is currently harmless, but it obscures the runner/module contract.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Add a small shared helper for module proposal execution, likely in `internal/framework/proposal_generation` or a narrow `internal/modules/modulekit` package.
|
||||
- Keep domain-specific prompt IDs and prompt text local to each module.
|
||||
- Move transcript section prompt payload construction into a shared prompt-context helper, for example `promptcontext.MarshalTranscriptSection`.
|
||||
- Provide one helper for prompt metadata maps instead of manually expanding `prompt_id`, `prompt_version`, `prompt_source`, `embedded_path`, and `sha256` in every module.
|
||||
- Preserve current module `Key`, replacement policy, and validator chain ownership.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Keep one golden or table-driven prompt payload test per module for domain-specific wording.
|
||||
- Add shared tests for transcript section JSON shape, empty transcript handling, categories copy behavior, and prompt metadata fields.
|
||||
- Add a parity test that all four module `Propose` methods still write diagnostics under the same module instance directory and produce the same correction mapping.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Low to medium. The behavior is highly duplicated, but prompt and diagnostics behavior is sensitive. Refactor behind existing module tests and CLI parity fixtures.
|
||||
|
||||
### 3.2 Effective config loading is repeated between commands
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/cli/run.go`
|
||||
- `internal/core/config`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- `runProcess` and `runConfigPrintEffective` both resolve config path, start from defaults, optionally load/apply file config, then apply environment overrides.
|
||||
- `runConfigValidate` separately loads a file, applies it to defaults, and validates it.
|
||||
- Path source metadata is computed in `internal/cli`, not `internal/core/config`, even though the precedence contract is documented as config behavior.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- Config precedence is part of the public contract. If a future setting is added, three command paths may need coordinated updates.
|
||||
- `config print-effective` is the user-visible diagnostic for effective config. It should use the same loader as `process`, except for intentionally omitted CLI overrides.
|
||||
- The current code is understandable, but the behavior is repeated in a way that makes drift likely as config grows.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Add a narrow effective-config loader in `internal/core/config`, returning `Config`, source path, source type, and version metadata.
|
||||
- Keep command-specific CLI flag parsing in `internal/cli`.
|
||||
- Model the intentional differences explicitly:
|
||||
- `process`: defaults + file + env + CLI overrides
|
||||
- `config print-effective`: defaults + file + env
|
||||
- `config validate`: file schema + default-backed config validation, no env
|
||||
- Move `resolveConfigPath` or an equivalent path resolver into `internal/core/config`.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- One table-driven config loader test covering explicit `--config`, `AUDITA_CONFIG`, default search paths, missing explicit paths, and missing default paths.
|
||||
- CLI tests asserting `process` and `print-effective` share file+env behavior.
|
||||
- A regression test that `config validate` remains file-only and does not read environment overrides.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Low. Behavior is already explicit and well tested; the refactor can be done by moving code without changing precedence.
|
||||
|
||||
### 3.3 Module catalog validation is split across config, contracts, and module factory
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/core/config/validation.go`
|
||||
- `internal/framework/contracts/contracts.go`
|
||||
- `internal/framework/modules/registry.go`
|
||||
- `internal/validators/chains.go`
|
||||
- `internal/framework/validators/models.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- Module keys appear in multiple places:
|
||||
- config default CSV: `glossary,homophones,glossary,spoken_word,grammar`
|
||||
- module factory constants and known-key map
|
||||
- built-in validator chains
|
||||
- confidence threshold lookup
|
||||
- individual module `Key()` methods
|
||||
- `Config.Validate` checks only that module names are non-empty. An unsupported configured module can pass `audita config validate` and fail later in `process` runner setup.
|
||||
- `contracts.ResolveModuleRunSpecs` only assigns instance names; it does not validate production module support.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- `audita config validate` is documented as a CI/preflight command. Letting unsupported modules pass weakens that preflight.
|
||||
- Module key drift could affect thresholds, validator chains, reports, and unsupported-module errors.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Introduce a small canonical module catalog or key package that can be imported by config validation, module factory construction, validator chain resolution, and threshold lookup without creating a cycle.
|
||||
- Keep module construction in `internal/framework/modules`; the catalog should expose keys and validation only.
|
||||
- Make `Config.Validate` reject unknown built-in module keys through that catalog.
|
||||
- Keep repeated module instances valid.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- `internal/core/config` test: unknown `pipeline.modules` fails validation.
|
||||
- `internal/cli` test: `audita config validate --config` rejects an unsupported module before runtime.
|
||||
- Existing `internal/framework/modules` unknown-module tests should continue to pass.
|
||||
- Validator chain tests should assert every catalog module has a built-in chain.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Medium. This tightens validation behavior. It is desirable before 1.0, but if unknown modules were intentionally allowed for future extension, document that explicitly instead.
|
||||
|
||||
### 3.4 Output schema support is hardcoded in config validation and registry
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/core/config/validation.go`
|
||||
- `internal/core/outputschema/registry.go`
|
||||
- `docs/architecture/output-schemas.md`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- `Config.Validate` hardcodes `bare-segments` and `audita-v1`.
|
||||
- `outputschema.Resolve` owns the actual output schema registry and returns the runtime error for unsupported schema names.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- Adding or deferring a schema requires updating multiple places.
|
||||
- Public behavior could drift: a schema might validate in config but fail at output time, or vice versa.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Make `internal/core/outputschema` expose `IsSupported`, `SupportedKeys`, or a validation function.
|
||||
- Have config validation call that helper or consume shared constants.
|
||||
- Keep actual encoding logic in `outputschema`; config should not know encoder details.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Config validation test for every output schema returned by the registry.
|
||||
- Output schema registry test that unsupported `seriatim-intermediate` still fails clearly until implemented.
|
||||
- CLI test that unsupported `--output-schema` fails before output write.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Low. This is a straightforward catalog centralization.
|
||||
|
||||
### 3.5 Diagnostics artifact names and report metadata paths are repeated
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/core/diagnostics/run_dir.go`
|
||||
- `internal/cli/run.go`
|
||||
- `internal/core/reporting/report.go`
|
||||
- docs under `docs/architecture` and `docs/integration`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- Artifact filenames such as `source-transcript.json`, `source-transcript-parsed.json`, `normalized-transcript.json`, `normalization-summary.json`, `chunking-summary.json`, `utilization-diagnostics.json`, `correction-ledger.json`, `invocation.json`, `effective-config.json`, `report.json`, and `error.log` are repeated between run-directory writers and `buildProcessReport`.
|
||||
- `runProcess` writes `utilization-diagnostics.json` and `correction-ledger.json` by raw string on both success and failure paths.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- These names are part of the documented diagnostics contract.
|
||||
- A filename change would need to be made in multiple places, and report metadata could point at files that are no longer written.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Define diagnostics artifact name constants in `internal/core/diagnostics`.
|
||||
- Add a helper that returns `reporting.DiagnosticsMetadata` for a run directory and status.
|
||||
- Add named methods for utilization diagnostics and correction ledger writes, or at least constants used by `WriteJSONArtifact`.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Unit test that `diagnostics.MetadataForRunDirectory` matches files written by `RunDirectory`.
|
||||
- CLI success/failure tests should continue to assert report metadata paths and actual file existence.
|
||||
- Add a test for failure report metadata including `error.log`.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Low. This is mostly string centralization, with high public-contract value.
|
||||
|
||||
### 3.6 Validator execution class is duplicated and partially hardcoded
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/validators/registry.go`
|
||||
- `internal/validators/metadata/metadata.go`
|
||||
- `internal/validators/*/validator.go`
|
||||
- `internal/framework/runner/runner.go`
|
||||
- `internal/cli/review_artifacts.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- Validator constructors wrap validators with execution class metadata.
|
||||
- `BuiltInValidatorDefinition` also has an `LLMBacked` field.
|
||||
- Runner uses `metadata.ClassOf` to order deterministic validators before LLM-backed validators.
|
||||
- Correction ledger classification uses a local hardcoded map of LLM-backed validator names.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- Adding a new LLM-backed validator could be ordered correctly by runner metadata but appear in the wrong correction-ledger section.
|
||||
- Validator class is domain metadata, not report-building policy. It should have one source of truth.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Make validator classification resolvable by validator instance or stable key from a single metadata source.
|
||||
- Remove the unused or redundant `LLMBacked` field, or make it the canonical source used by constructors, runner ordering, and ledger formatting.
|
||||
- Replace the local ledger map with `metadata.ClassOf` when possible, or a registry lookup by stable key.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Correction ledger test that LLM-backed decisions are classified from validator metadata, not a local string map.
|
||||
- Registry test that every registered LLM-backed validator reports the same class through every public metadata path.
|
||||
- Runner ordering test should remain in place.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Low to medium. The implementation is small, but correction-ledger shape is diagnostics-visible.
|
||||
|
||||
### 3.7 Malformed structured-output classification is duplicated
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/framework/proposal_generation/generate.go`
|
||||
- `internal/framework/validators/llm_validators.go`
|
||||
- `internal/framework/llm/openai_compatible_client.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- Proposal generation and LLM validators both classify malformed structured-output errors by scanning error message substrings.
|
||||
- The marker lists are currently the same, but they are maintained independently.
|
||||
- The actual errors originate in the LLM adapter.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- Proposal-generation malformed payloads become warnings with zero proposals, while validator malformed payloads reject affected batches with warnings. If classifiers drift, similar adapter failures could be downgraded in one workflow and hard-fail in another.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Prefer a typed error or exported classifier from `internal/framework/llm`.
|
||||
- If typed errors are too invasive, create one shared classifier function in a lower framework package used by both proposal generation and validators.
|
||||
- Preserve the different handling semantics at each call site.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Shared classifier table for all adapter malformed-output errors.
|
||||
- Proposal-generation test and validator test should assert the same representative malformed adapter errors are downgraded.
|
||||
- Adapter tests should assert typed/classified errors wrap useful context and still redact secrets.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Medium. Error typing can accidentally affect retry and wrapping behavior; do this with focused tests.
|
||||
|
||||
## 4. Medium-confidence opportunities
|
||||
|
||||
### 4.1 CLI flag registration and override extraction are large and repetitive
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/cli/run.go`
|
||||
- `internal/core/config/flags.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- Each process flag has a field in `processFlags`, a registration entry in `newProcessFlagSet`, a case in `fs.Visit`, and an assignment in `config.ApplyCLIOverrides`.
|
||||
- File config and environment config also set many of the same effective config fields.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- Adding a new config option requires multiple edits. Missing one edit could create a flag that displays but does not override, or a config field with no CLI override.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Avoid a generic reflection-heavy flag system before 1.0.
|
||||
- Consider a small metadata table only for simple scalar flags, or a focused helper that maps visited flags to `CLIOverrides`.
|
||||
- Keep nontrivial semantics, such as legacy concurrency alias precedence, explicit in code.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- CLI override parity test for every stable flag that mutates config.
|
||||
- A test that default flag values reflect file+env effective config before CLI overrides.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Medium. A broad flag abstraction would be riskier than the current duplication. Do only a small helper if it clearly reduces missed updates.
|
||||
|
||||
### 4.2 Config source application repeats field-level assignments
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/core/config/file_config.go`
|
||||
- `internal/core/config/env.go`
|
||||
- `internal/core/config/flags.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- The same effective fields are assigned from file config, env vars, and CLI overrides.
|
||||
- Some semantics differ intentionally: file config supports `api_key_env`, env supports `OPENROUTER_API_KEY` fallback, CLI uses direct values.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- Field additions are easy to miss in one source.
|
||||
- Error messages and trimming behavior can drift.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Do not force all config sources through one generic mapper.
|
||||
- Add small setter helpers for repeated config subdomains such as LLM target, concurrency, thresholds, normalization, and diagnostics.
|
||||
- Keep source-specific parsing and error labels local.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Cross-source table proving file, env, and CLI all reach the same effective fields where they are meant to.
|
||||
- Tests for intentional differences: API key env resolution, `OPENROUTER_API_KEY` fallback, CLI direct API key, and transcript description trimming.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Medium. Useful, but only after the effective loader and catalog cleanup.
|
||||
|
||||
### 4.3 Prompt metadata and response schema metadata map construction repeats
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/modules/*/module.go`
|
||||
- `internal/framework/proposal_generation/generate.go`
|
||||
- `internal/framework/validators/llm_validators.go`
|
||||
- `internal/prompts`
|
||||
- `internal/framework/responseschema`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- Prompt metadata maps are manually expanded in module proposal generation and validator diagnostics.
|
||||
- Response schema metadata maps are built independently in proposal generation and validator diagnostics.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- Metadata fields are diagnostics-visible and useful for reproducibility.
|
||||
- Adding a metadata field requires updating multiple call sites.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Add `Metadata.Map()` or a typed diagnostics metadata struct in `internal/prompts`.
|
||||
- Add `responseschema.Metadata()` or a method returning a stable diagnostics shape.
|
||||
- Prefer typed structs over `map[string]any` where possible.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Prompt metadata rendering test should assert all registered prompts expose stable metadata.
|
||||
- Proposal and validator diagnostics tests should assert the shared metadata helper is used.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Low.
|
||||
|
||||
### 4.4 Secret redaction logic is split across config, LLM diagnostics, and adapter errors
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/core/config/redaction.go`
|
||||
- `internal/framework/llm/diagnostics.go`
|
||||
- `internal/framework/llm/client_common.go`
|
||||
- `internal/framework/proposal_generation/generate.go`
|
||||
- `internal/framework/runner/runner.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- Config redaction replaces non-empty API keys with `[REDACTED]`.
|
||||
- LLM diagnostics replace configured secret values and `Bearer <secret>`.
|
||||
- Adapter error sanitization separately replaces secrets and bearer values.
|
||||
- Proposal and validator paths separately assemble secret lists.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- Secret redaction is a public guarantee.
|
||||
- New secret-bearing config fields could be missed in one path.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Add a small redaction helper package or keep it in `internal/framework/llm` only if it remains LLM-specific.
|
||||
- Centralize `[]string` secret extraction from `config.Config`.
|
||||
- Keep config structural redaction separate from byte/string payload redaction, but share the redaction token and value replacement behavior.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- One test that a proposal-generation error, validator diagnostic artifact, effective config artifact, and surfaced provider error all redact the same configured secrets.
|
||||
- Existing subprocess no-secret-leak test should remain as an end-to-end guard.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Medium. The current coverage appears strong; change carefully.
|
||||
|
||||
### 4.5 Test fakes and fixture helpers are duplicated across packages
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/modules/*/module_test.go`
|
||||
- `internal/framework/proposal_generation/generate_test.go`
|
||||
- `internal/framework/validators/llm_validators_test.go`
|
||||
- `internal/cli/run_test.go`
|
||||
- `cmd/audita/main_integration_test.go`
|
||||
- `internal/cli/release_fixtures_test.go`
|
||||
- `internal/cli/parity_test.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- Several packages define fake structured LLM clients, fixture path helpers, read/write helpers, diagnostics glob assertions, and run-directory helpers.
|
||||
- The four module test files have particularly similar fake clients and proposal-diagnostics assertions.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- Refactors in LLM or diagnostics behavior require updating many tests.
|
||||
- Some duplicated tests are valuable because they preserve per-module public behavior; the issue is helper duplication, not coverage volume.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Add package-local helper files where duplication is within a package.
|
||||
- For cross-package fakes, prefer a small internal test support package only if it does not create import cycles or hide test intent.
|
||||
- Keep module-specific assertions local.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- This is test infrastructure cleanup. Existing tests should remain semantically equivalent.
|
||||
- Add helper tests only if helpers contain nontrivial behavior, such as fake response sequencing.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Low.
|
||||
|
||||
### 4.6 Stage-name construction is inconsistent enough to centralize, but not enough to redesign
|
||||
|
||||
Affected files/packages:
|
||||
|
||||
- `internal/modules/*/module.go`
|
||||
- `internal/framework/proposal_generation/generate.go`
|
||||
- `internal/framework/validators/llm_validators.go`
|
||||
- `internal/framework/runner/observability.go`
|
||||
|
||||
Duplicated or near-duplicated behavior:
|
||||
|
||||
- Modules pass stage names like `<module_instance>:proposal:section-0001`.
|
||||
- `proposal_generation` has a default builder using `<module_instance>:proposal-generation:section-0001`, but production modules bypass it.
|
||||
- Validators build `<module_instance>:<validator>:batch-0001`.
|
||||
- Utilization extracts module instance by splitting stage names on `:`.
|
||||
|
||||
Why it matters:
|
||||
|
||||
- Stage names affect diagnostics filenames and observability grouping.
|
||||
- Current behavior works, but the naming grammar is implicit.
|
||||
|
||||
Recommended refactor:
|
||||
|
||||
- Add narrow helpers for proposal and validator stage names.
|
||||
- Preserve current production stage names unless there is a deliberate pre-1.0 diagnostics compatibility decision.
|
||||
- Keep filename sanitization in `internal/framework/llm`.
|
||||
|
||||
Suggested tests:
|
||||
|
||||
- Unit tests for stage-name helper output.
|
||||
- Utilization test that module instance extraction still works for proposal and validator stage names.
|
||||
|
||||
Risk level:
|
||||
|
||||
- Medium. Renaming stages can change diagnostics filenames, so avoid unnecessary churn.
|
||||
|
||||
## 5. Boundary and responsibility concerns
|
||||
|
||||
### CLI owns too much report and diagnostics metadata assembly
|
||||
|
||||
`internal/cli/run.go` is doing orchestration, command parsing, config loading, output routing, report assembly, diagnostics metadata path assembly, and correction-ledger construction. This is acceptable for a small CLI, but two pieces are drifting beyond command responsibility:
|
||||
|
||||
- diagnostics artifact path metadata belongs closer to `internal/core/diagnostics`;
|
||||
- report assembly and correction-ledger mapping belong closer to `internal/core/reporting` or a narrow reporting adapter package.
|
||||
|
||||
Recommended home:
|
||||
|
||||
- `internal/core/diagnostics`: artifact constants and diagnostics metadata path construction.
|
||||
- `internal/core/reporting`: pure mapping from runner/config/diagnostics state into report payloads.
|
||||
- `internal/cli`: command parsing, invocation wiring, exit codes, stdout/stderr behavior.
|
||||
|
||||
### Config validation lacks catalog ownership
|
||||
|
||||
`internal/core/config` currently validates only generic module list shape and hardcodes output schema keys. Because modules and output schemas are public contract values, config validation should use a catalog owned by the relevant domain.
|
||||
|
||||
Recommended home:
|
||||
|
||||
- output schema validation: `internal/core/outputschema`;
|
||||
- module key validation: a small catalog package or lower-level constants package importable by config, module factory, validator chains, and threshold lookup.
|
||||
|
||||
### Runner owns adapter shims between contracts and validator framework
|
||||
|
||||
`internal/framework/runner` contains `validationLLMClientAdapter` and `llmDiagnosticsWriterAdapter`. This is not a serious problem today because runner wires proposal and validation workflows. If these adapters grow, move them to `internal/framework/validators` or a small integration package so runner remains focused on orchestration.
|
||||
|
||||
### LLM malformed-output policy is spread across callers
|
||||
|
||||
The LLM adapter emits the errors, while proposal generation and validators classify them by message text. The policy decision is caller-specific, but the classification should live with the LLM/framework error type.
|
||||
|
||||
## 6. Path, key, and naming construction review
|
||||
|
||||
Centralized enough:
|
||||
|
||||
- LLM diagnostics artifact suffixes and stage sanitization are centralized in `internal/framework/llm/diagnostics.go`.
|
||||
- Output file writing is routed through `internal/core/io.WriteFile`.
|
||||
- Run directories are created in `internal/core/diagnostics.NewRunDirectory`.
|
||||
|
||||
Needs cleanup:
|
||||
|
||||
- Core diagnostics artifact names are repeated between `RunDirectory` writer methods and `buildProcessReport`.
|
||||
- `utilization-diagnostics.json` and `correction-ledger.json` are raw strings in both success and failure paths.
|
||||
- Proposal and validator diagnostics subdirectory construction repeats `filepath.Join(diagnosticsDir, moduleInstance)`.
|
||||
- Proposal and validator stage names are manually formatted in multiple packages.
|
||||
- Module keys are repeated across config defaults, module factory, validator chains, confidence threshold lookup, and module implementations.
|
||||
- Output schema names are repeated between config validation and `outputschema`.
|
||||
|
||||
Recommendation:
|
||||
|
||||
- Start with artifact constants and metadata helpers because that is the lowest-risk path/key cleanup.
|
||||
- Then centralize stage-name helpers without changing current production naming.
|
||||
- Defer any broader "path manager" abstraction.
|
||||
|
||||
## 7. Resolution and catalog review
|
||||
|
||||
Modules:
|
||||
|
||||
- Runtime module construction has a production registry in `internal/framework/modules`.
|
||||
- Instance naming for repeated modules is centralized in `contracts.ResolveModuleRunSpecs`.
|
||||
- Unknown module failure exists in the factory, but config validation does not catch unknown modules.
|
||||
- Built-in validator chain resolution separately maps module key to validator keys.
|
||||
|
||||
Output schemas:
|
||||
|
||||
- Encoding is centralized in `internal/core/outputschema`.
|
||||
- Validation is duplicated in config.
|
||||
|
||||
Prompts:
|
||||
|
||||
- Prompt asset lookup and metadata are centralized in `internal/prompts`.
|
||||
- Prompt metadata map construction is repeated at call sites.
|
||||
- Prompt source selection is intentionally built-in only and should remain that way for 1.0.
|
||||
|
||||
Validators:
|
||||
|
||||
- Validator construction is package-owned under `internal/validators`.
|
||||
- Chains are centralized in `internal/validators/chains.go`.
|
||||
- Execution class metadata exists, but reporting/correction-ledger classification does not fully use it.
|
||||
|
||||
Schemas:
|
||||
|
||||
- Transcript and glossary parsing/validation are centralized in `internal/core/schema`.
|
||||
- Structured LLM response schemas are centralized in `internal/framework/responseschema`.
|
||||
- Output schema registry and response schema registry are appropriately separate.
|
||||
|
||||
Recommendation:
|
||||
|
||||
- Introduce only small catalog helpers for module keys, output schema keys, prompt metadata maps, response schema metadata maps, and validator execution class.
|
||||
- Avoid user-configurable modules, validators, prompts, or schemas before 1.0 unless already planned elsewhere.
|
||||
|
||||
## 8. Config and command-loading review
|
||||
|
||||
Consistent behavior:
|
||||
|
||||
- The documented precedence for `process` is implemented: defaults, file config, environment, CLI.
|
||||
- `config print-effective` intentionally omits CLI process flags and uses defaults, file config, and environment.
|
||||
- `config validate` intentionally requires `--config` and does not require transcript/glossary inputs.
|
||||
- Missing explicit config paths are hard failures; missing default paths are non-fatal.
|
||||
- Environment parsing and CLI parsing both preserve legacy total-concurrency alias behavior.
|
||||
|
||||
Likely accidental or high-risk differences:
|
||||
|
||||
- Unsupported module names pass `Config.Validate` and `audita config validate`.
|
||||
- Output schema support is duplicated instead of delegated to the output schema registry.
|
||||
- Config path resolution lives in CLI even though it is part of config behavior.
|
||||
|
||||
Intentional differences:
|
||||
|
||||
- File config resolves `api_key_env`; env and CLI set direct API key values.
|
||||
- `OPENROUTER_API_KEY` is an environment fallback only for the primary LLM.
|
||||
- `transcript-description` has CLI/config support but no `AUDITA_*` environment variable, matching documentation.
|
||||
|
||||
Recommendation:
|
||||
|
||||
- Build a shared effective config context helper and keep source-specific parsing semantics explicit.
|
||||
- Tighten catalog validation before 1.0 if unknown modules are not meant to be accepted.
|
||||
|
||||
## 9. State, manifest, or progress handling review
|
||||
|
||||
Audita does not currently have a manifest/checkpoint/resume model. State is per-run diagnostics and report artifacts.
|
||||
|
||||
Consistent behavior:
|
||||
|
||||
- `process` creates one diagnostics run directory when diagnostics initialization succeeds.
|
||||
- Failures after run-dir creation write `error.log`, best-effort report artifacts, and retain diagnostics.
|
||||
- Success writes optional `--report-json`, run-dir `report.json`, utilization diagnostics, and correction ledger.
|
||||
- Retention is centralized in `diagnostics.ShouldRetainRunDirectory`.
|
||||
- There is no resume/retry/force behavior to preserve.
|
||||
|
||||
Drift risks:
|
||||
|
||||
- Success and failure paths both write utilization and correction-ledger artifacts with duplicated raw filenames.
|
||||
- Report diagnostics metadata is assembled independently from the run-directory writer methods.
|
||||
- Retention mode `never` currently still retains successful run directories in `ShouldRetainRunDirectory`, which may be intentional per tests or a naming/documentation mismatch. Do not change it in a dedup pass without first confirming semantics.
|
||||
|
||||
Recommendation:
|
||||
|
||||
- Centralize artifact names and report metadata path construction.
|
||||
- Keep retention behavior unchanged unless a separate bug review confirms the intended meaning of `never`.
|
||||
|
||||
## 10. Refactors to avoid before 1.0
|
||||
|
||||
- Do not introduce a generic workflow engine. The current sequential runner is clear and explicit.
|
||||
- Do not add a plugin architecture for modules, validators, prompts, or schemas before 1.0.
|
||||
- Do not redesign the CLI or replace `flag` with a larger framework only for deduplication.
|
||||
- Do not collapse all config source parsing into a reflection-based mapper; source semantics differ intentionally.
|
||||
- Do not merge module packages into one generic module type. Keep domain-specific prompt assets, keys, validator chains, and replacement policies visible.
|
||||
- Do not rewrite diagnostics or reporting schemas broadly. Centralize names and mapping helpers first.
|
||||
- Do not change diagnostics stage names casually; they affect artifact filenames and debugging workflows.
|
||||
- Do not consolidate deterministic and LLM validator behavior just because both return decisions. Their failure and batching semantics differ.
|
||||
- Do not generalize transcript/glossary schema parsing into a broad schema framework.
|
||||
- Do not reduce duplicated tests where the duplication protects distinct public command/module behavior.
|
||||
|
||||
## 11. Recommended implementation sequence
|
||||
|
||||
1. Centralize diagnostics artifact constants and diagnostics metadata path construction.
|
||||
2. Centralize output schema validation through `internal/core/outputschema`.
|
||||
3. Introduce a small module key catalog and use it in config validation, module factory, validator chains, and threshold lookup.
|
||||
4. Add an effective config loading context helper for defaults + file + env, then update `process` and `config print-effective`.
|
||||
5. Extract shared module proposal plumbing and prompt transcript-section payload construction.
|
||||
6. Centralize prompt metadata and response schema metadata map construction.
|
||||
7. Centralize validator execution-class lookup and update correction-ledger classification.
|
||||
8. Centralize malformed structured-output classification through a typed/shared LLM error helper.
|
||||
9. Add or consolidate focused test helpers for module LLM fakes, diagnostics assertions, and fixture paths.
|
||||
10. Do a final dead-code and legacy sweep for redundant helper fields such as unused validator definition metadata.
|
||||
|
||||
Each item can be a separate commit with package-level tests and at least one CLI regression where public behavior is involved.
|
||||
|
||||
## 12. Test strategy
|
||||
|
||||
Tests to add before refactoring:
|
||||
|
||||
- `internal/core/config`: unknown module key fails validation, if unsupported modules are not intended to be accepted.
|
||||
- `internal/core/config`: every output schema registry key validates through config.
|
||||
- `internal/core/diagnostics`: report metadata paths match run-directory artifact names.
|
||||
- `internal/validators`: validator class by key/instance is consistent for all registered validators.
|
||||
- `internal/framework/llm`: shared malformed structured-output classifier covers all current adapter malformed errors.
|
||||
|
||||
Tests to add during refactoring:
|
||||
|
||||
- `internal/framework/promptcontext`: transcript section prompt payload preserves IDs, speaker, timestamps, text, and categories.
|
||||
- `internal/framework/proposal_generation`: shared module proposal helper preserves current stage name, diagnostics dir, schema metadata, and malformed-output warning behavior.
|
||||
- `internal/cli`: `process` and `config print-effective` share defaults+file+env behavior.
|
||||
- `internal/cli`: `config validate` remains file-only and does not read env overrides.
|
||||
- `internal/cli`: correction ledger classifies deterministic and LLM validator decisions through canonical metadata.
|
||||
|
||||
Existing tests to run after each cleanup:
|
||||
|
||||
- `go test ./internal/core/config ./internal/core/outputschema`
|
||||
- `go test ./internal/core/diagnostics ./internal/core/reporting`
|
||||
- `go test ./internal/framework/proposal_generation ./internal/framework/validators ./internal/framework/runner`
|
||||
- `go test ./internal/validators/...`
|
||||
- `go test ./internal/modules/...`
|
||||
- `go test ./internal/cli ./cmd/audita`
|
||||
- Run `go test ./...` before merging a multi-package cleanup.
|
||||
|
||||
Validation note:
|
||||
|
||||
- During this report-only pass, no full test suite was run. A lightweight `go list ./...` completed package listing but emitted a sandbox warning while trying to write the Go module stat cache outside the repository.
|
||||
|
||||
## 13. Appendix: findings not worth acting on
|
||||
|
||||
### Separate module packages
|
||||
|
||||
The four production module packages contain visible repetition, but keeping separate packages is useful. The module domains, prompt assets, validator chains, and tests are distinct enough that a single generic module package would hide important behavior.
|
||||
|
||||
Do not refactor now beyond shared proposal/prompt plumbing.
|
||||
|
||||
### Report type duplication between runner and reporting
|
||||
|
||||
`runner.ModuleResult` and `reporting.ModuleReport` look similar. Keeping separate runtime and public report shapes is reasonable because runner owns execution state and reporting owns serialized public schema.
|
||||
|
||||
Only centralize mapping helpers; do not merge the types.
|
||||
|
||||
### Transcript and glossary parsing stay separate
|
||||
|
||||
Transcript JSON and glossary YAML parsing have different formats, validation rules, and error messages. There is no useful shared parser abstraction to extract.
|
||||
|
||||
### Response schema registry and output schema registry stay separate
|
||||
|
||||
Structured LLM response schemas and transcript output schemas are both "schemas", but they serve different users and have different lifecycles. Do not combine their registries.
|
||||
|
||||
### `flag` package usage
|
||||
|
||||
The CLI command surface is small. Replacing `flag` with a larger CLI framework would not pay for itself before 1.0.
|
||||
|
||||
### Local test duplication that protects public behavior
|
||||
|
||||
Some test duplication in CLI, subprocess, parity, and release fixtures is intentional. These tests exercise different public surfaces and should remain explicit even if helpers are shared.
|
||||
|
||||
### Filesystem state as diagnostics state
|
||||
|
||||
Audita has no resume/checkpoint semantics. Treating diagnostics artifacts as filesystem outputs is currently acceptable. A manifest system would be speculative before there is a resume or audit workflow that needs it.
|
||||
Reference in New Issue
Block a user