162 lines
9.8 KiB
Markdown
162 lines
9.8 KiB
Markdown
# Maintainability Audit: Findings and Recommended Changes
|
||
|
||
**Reviewed:** 2026-09-25, source commit `011b021`.
|
||
**Scope:** Recent partial-decision reconciliation, validation context, prompt
|
||
alignment, and request-local reference changes, including related existing code.
|
||
**Status:** R27–R30 addressed; see the resolution record below. Previous findings and resolution records remain in Git
|
||
history through `011b021`.
|
||
|
||
## Architecture Assessment
|
||
|
||
The recent changes fit Audita's existing boundaries. Proposal generation,
|
||
validation, prompt assets, and outcome reporting already have suitable owners;
|
||
this audit does not recommend a new common package or a broad refactor.
|
||
|
||
The strongest opportunities are shared prompt instructions and request-scoped
|
||
validation context preparation. Two smaller improvements concern recovery-reason
|
||
classification and partial-result assembly; both predate the latest commits.
|
||
These are bounded maintenance recommendations, not new release gates or changes
|
||
to correction policy, response acceptance, or reporting semantics.
|
||
|
||
## R27 — P2: Consolidate Shared Prompt Instructions
|
||
|
||
**Category:** Repeated behavioral-contract text; risk of future drift.
|
||
|
||
**Evidence:** The four [proposal prompts](../../internal/prompts/assets/prompts/modules.grammar.proposal.yaml)
|
||
repeat literal source-span matching, request-local identity, replacement-text,
|
||
source-based proposal, and confidence instructions. The three active
|
||
[validator prompts](../../internal/prompts/assets/prompts/validators.editorial_review.yaml)
|
||
repeat candidate-reference, supplemental-context, and verdict-confidence guidance.
|
||
Recent contract changes required coordinated edits across these assets. The
|
||
[prompt registry](../../internal/prompts/registry.go) already provides a shared
|
||
hardening fragment supplied through the role-bound adapter.
|
||
|
||
**Recommended change:** Extract genuinely common passages into embedded fragments
|
||
under `internal/prompts/assets/shared`, following the existing fragment-supply
|
||
approach. Keep the fragments owned by `internal/prompts` and use the existing
|
||
PromptKit rendering path. Keep module-specific examples, replacement-policy
|
||
instructions, protected-name exceptions, and semantic criteria in their individual
|
||
prompt definitions. Do not merge similar-looking rules with different meanings.
|
||
|
||
**Acceptance:** Common instructions have one maintained source and appear in all
|
||
intended rendered prompts. The existing module and validator distinctions remain
|
||
intact, including glossary replace-all behavior and the spoken-word protected-name
|
||
exception. Focused rendering checks cover fragment inclusion and representative
|
||
module-specific guidance without requiring exact-prose snapshots or introducing
|
||
a separate templating framework.
|
||
|
||
## R28 — P2: Prepare Validation Context Once Per Request
|
||
|
||
**Category:** Repeated computation and overlapping segment lookup.
|
||
|
||
**Evidence:** [BuildLLMValidationRequest](../../internal/framework/validators/llm_validators.go)
|
||
already indexes transcript segments, but
|
||
[validationContext](../../internal/framework/validators/llm_context.go) scans the
|
||
transcript again for each candidate. It also calls
|
||
[NewProtectedVocabulary](../../internal/framework/validators/protected_terms.go)
|
||
for each candidate, rebuilding the same vocabulary and compiling its matching
|
||
expression repeatedly. This is duplicated preparation rather than copied code.
|
||
|
||
**Recommended change:** Prepare a segment-position lookup and protected vocabulary
|
||
once per validation request, then pass that prepared context into the existing
|
||
context-selection helper. Keep the implementation within `framework/validators`.
|
||
Continue selecting glossary evidence for each candidate individually because its
|
||
proposed replacement can mention different terms. A process-wide cache or new
|
||
context-management package is unnecessary for this cleanup.
|
||
|
||
**Acceptance:** Candidates in one validation request reuse the prepared lookup and
|
||
vocabulary. Context still consists of the available immediate neighbors within the
|
||
current chunk and relevant glossary entries in glossary order. Candidate-specific
|
||
replacement text still participates in glossary selection. Existing optional-context
|
||
trimming, token estimation, and oversized-input handling retain their behavior.
|
||
Verify these outcomes with focused behavioral tests rather than assertions about
|
||
private helper call counts or unmeasured performance guarantees.
|
||
|
||
## R29 — P3: Share Execution-Recovery Classification
|
||
|
||
**Category:** Duplicate reporting rule; predates the latest commits.
|
||
|
||
**Evidence:** [Proposal generation](../../internal/framework/proposal_generation/generate.go)
|
||
and [LLM validation](../../internal/framework/validators/llm_validators.go)
|
||
independently translate structural-repair and transient-retry counts into
|
||
`output_repaired` and `service_recovered` reasons. The conversion is the same in
|
||
both locations; the surrounding success and domain-outcome decisions differ.
|
||
|
||
**Recommended change:** Place a small conversion helper in the existing
|
||
`framework/outcomes` package and use it from both callers. Keep the decision to
|
||
record recoveries at the call sites, along with domain-specific reasons such as
|
||
filtered validator decisions. Do not combine proposal and validation orchestration
|
||
or move domain-success classification into this helper.
|
||
|
||
**Acceptance:** Both callers use the same execution-count conversion. Existing
|
||
reason ordering, successful-operation reporting, and domain-specific additions
|
||
remain intact. A partially unavailable validation batch continues to record its
|
||
failure and execution counts without acquiring recovery records merely because a
|
||
repair or retry occurred. Existing outcome tests should continue to protect these
|
||
distinctions.
|
||
|
||
## R30 — P3: Centralize Partial Validator Result Assembly
|
||
|
||
**Category:** Repeated early-return construction; predates the latest commits.
|
||
|
||
**Evidence:** Several early returns in
|
||
[LLMBackedValidator.Validate](../../internal/framework/validators/llm_validators.go)
|
||
repeat nested slice appends to combine immediate decisions, oversized-input
|
||
decisions, and decisions from completed model batches, together with accumulated
|
||
operation outcomes. This makes it easier for a future failure path to omit facts
|
||
already collected by the validator.
|
||
|
||
**Recommended change:** Use a small local result-building helper to assemble the
|
||
partial result consistently. Keep error handling, batching, success-path ordering,
|
||
and cardinality enforcement explicit. A general-purpose result-aggregation package
|
||
is not needed.
|
||
|
||
**Acceptance:** Cancellation and failure paths retain the same previously collected
|
||
decisions and operations. Result ordering and cardinality behavior remain unchanged;
|
||
the helper does not enforce complete cardinality on interrupted work. Retain the
|
||
existing partial-progress and fatal-failure coverage, adding tests only for a
|
||
material gap exposed by the refactor.
|
||
|
||
## Boundaries to Preserve
|
||
|
||
- Keep proposal and validator reference mapping separate. Proposals can target the
|
||
same segment repeatedly; duplicate validator decisions make that candidate
|
||
unavailable. Similar indexing arithmetic alone does not justify a generic
|
||
reference-management abstraction.
|
||
- Keep distinct protected-term policy checks separate. They already share the
|
||
vocabulary implementation and intentionally apply different policies.
|
||
- Preserve exact source-span matching, request-local references, batching, and
|
||
partial-success behavior. Consolidation is not a reason to strengthen contracts
|
||
or broaden the accepted correction scope.
|
||
|
||
## Review Verification and Limits
|
||
|
||
- This is a source-level maintainability audit, not a finding of demonstrated
|
||
production failures or a measured performance assessment.
|
||
- No application tests were rerun for the original read-only audit; its roadmap
|
||
update used documentation link and whitespace checks. Implementation validation
|
||
is recorded below.
|
||
- Future remediation should run focused behavioral tests and the broader suite
|
||
appropriate to the actual change, following the testing policy. No live-provider
|
||
evaluation or new evaluation framework is required by these recommendations.
|
||
- The findings above describe the audited baseline. Implementation and validation
|
||
are recorded below; no release publication is included.
|
||
|
||
## Resolution Record
|
||
|
||
Implemented as focused changes within the existing packages. Proposal and validator
|
||
reference mapping and distinct protected-term policies remain separate.
|
||
|
||
| Finding | Resolution and evidence |
|
||
| --- | --- |
|
||
| R27 | Common proposal and validation instructions live in two embedded shared fragments, supplied through the existing role-bound PromptKit adapter. Individual YAML definitions retain policy differences and examples. Rendering tests check fragment inclusion once per applicable prompt and representative module-specific guidance. |
|
||
| R28 | Validation request construction builds one segment-position lookup and protected vocabulary, reused by context selection for every candidate. A new test verifies that two replacements of the same source span receive their own relevant glossary entries. Existing neighbor, glossary-order, chunk-boundary, and budget tests pass. |
|
||
| R29 | Both proposal generation and validation use `outcomes.ExecutionRecoveries`. Call sites still control success and domain-specific recovery reasons. Existing tests cover combined recoveries and failed validation with repair counts but no recovery record. |
|
||
| R30 | A local result builder combines collected decisions and operation outcomes for partial returns and completed validation. Sorting and full cardinality checks remain outside it. Existing fatal-batch, partial-progress, runner, and reporting tests pass. |
|
||
|
||
Validation: `go test ./...` passed, including PromptKit preparation, partial-decision
|
||
reconciliation, subprocess reports, and fatal-progress coverage. Documentation
|
||
links and whitespace checks passed. No live model evaluation or performance
|
||
measurement was performed; the changes make no new model-quality or latency
|
||
promise.
|