diff --git a/audit.md b/audit.md index 3c341ed..fe0b718 100644 --- a/audit.md +++ b/audit.md @@ -4086,3 +4086,272 @@ go test ./internal/llm -run '^TestOpenAICompatibleClientAllowsEmptyConfiguredBas gained deterministic execution-example validation or removed the fixed-port assumption. Its synthesis should count earlier test gaps once at their owning component rather than cloning them from this matrix. + +## Stage 17: Cross-Cutting Duplication, Efficiency, And Architecture Review + +### Scope Reviewed + +This review synthesized all accepted component findings and coverage ledgers, +then checked the complete production graph for package dependencies, interface +width, structural similarity, complexity, transitive loop depth, and repeated +work. Source inspection covered the graph's production similarity pairs and +hotspots, every production interface, the root facade's assembly and +public/internal adapters, the execution-profile and output-contract ingress +paths, source discovery, JSON-value copying, schema preparation, request +construction, and the capacity boundary. Canonical architecture and internal +component documents were compared with the resulting dependency and ownership +map. + +The graph found 15 production interfaces. Thirteen contain one operation, +`PreparedValidation` contains the cohesive pair needed to validate against and +describe one frozen plan, and `Option` contains only its private application +operation. The import map has the root facade depending inward on the +documented internal components, with no internal dependency back on the root +or a consumer. Similarity and complexity metrics were treated as locators: +each candidate below was accepted or rejected only after its rule, frequency, +and boundary were inspected. + +### Accepted Findings + +#### S17-F01: Execution-setting bounds have three independent acceptance owners + +- **Category:** duplication +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `profiles.go` (`validatePublicProfile`), + `internal/profile/filesystem_repository.go` (`validateProfile`), + `internal/usecase/runner.go` (`mergeExecutionTargetOverride`), and the + execution-setting declarations in `internal/domain/domain.go` +- **Contract at issue:** Temperature, maximum tokens, top-p, and timeout are + the same resolved execution settings whether they enter through an + in-memory profile, a file profile, or a request override. Optional pointer + presence and source error categories differ, but the accepted scalar values + do not; each setting needs one source-neutral invariant owner. +- **Evidence:** The two profile validators repeat the same four checks and + error text, while `mergeExecutionTargetOverride` repeats those checks for + pointer values. The completed component probes found the same IEEE NaN hole + independently in profile inputs (S08-F01) and request overrides (S11-F01), + and S02-F02 had already established drift risk between the two profile + validators. The three paths all consume `internal/domain` values but no + leaf package owns their scalar invariants. The model client separately + rechecks non-negative timeout as a defensive request boundary, reinforcing + that the value rule is broader than any one source. +- **Failure mode:** Corrections and new constraints require coordinated edits + across source and orchestration packages. The existing non-finite defect + already reaches supposedly valid profile and prepared values through more + than one ingress, and a later one-path fix can make identical settings valid + or invalid according to their source. +- **Recommended direction:** Put pure execution-setting scalar validation in + `internal/domain`, beside the shared values whose invariants it defines. + Profile and use-case packages should retain source-specific required fields, + pointer-presence handling, normalization, and error translation while + calling that owner. This dependency remains inward and acyclic because all + three consumers already depend on `internal/domain`, which need not depend + on a source, use case, or provider. The model client may consume the same + predicate while retaining its own defensive error identity. +- **Required verification:** Give the shared owner one table containing NaN, + both infinities, every exact bound, finite neighbors on both sides, and + representative interior values for all four settings. Retain small + integration cases for in-memory profiles, both file-source forms, request + overrides including explicit zero presence, and the model-client timeout + defense. Each boundary must preserve its current public or internal error + category, and no stable JSON or provider request may receive a non-finite + effective setting. + +#### S17-F02: Output-contract legality is split between prompt loading and request resolution + +- **Category:** duplication +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/promptdef/filesystem_repository.go` + (`normalizePromptDefinitionWithContent`, `isValidOutputFormat`, and + `isValidValidationMode`), `internal/usecase/runner.go` + (`resolveOutputContract` and structured-output resolution), + `internal/domain/domain.go` (`OutputContract` and its enums), and the public + `OutputContract` conversion in `convert.go` +- **Contract at issue:** Supported format and validation-mode membership, + non-negative repair attempts, and the JSON-Schema path relationship describe + one `domain.OutputContract`. Prompt-file requirements and request defaults + can differ, but a request replacement must not create a domain value that + the prompt source would reject under the same stable vocabulary. +- **Evidence:** Prompt normalization owns private enum predicates and rejects + unsupported formats, unsupported modes, a missing JSON-Schema path, and + negative repair attempts. Request resolution copies the same domain value + wholesale and defaults only an empty format. S11-F02 confirmed that both + public preparation APIs accept unknown request enums. Source inspection also + shows that a negative request repair count bypasses the prompt-owned + non-negative repair-count rule, and missing request schema information is + left to later schema work and its different error category. The format reference + declares the same output-contract value set before describing complete + request replacement, so this is a split semantic owner rather than two + provider-specific policies. +- **Failure mode:** File and request forms of the same contract have different + acceptance behavior, preparation can publish unsupported stable metadata, + and adding a format or validation mode requires source and orchestration + edits that have no shared compile-time or test owner. A direct fix to + S11-F02 that copies the prompt package's switches would deepen that drift. +- **Recommended direction:** Put source-neutral output-contract predicates and + cross-field invariants in `internal/domain`. Keep file-required fields and + YAML error context in `internal/promptdef`; keep the request's documented + empty-format default and invalid-request translation in `internal/usecase`; + and keep schema loading and compilation in `internal/validate`. Both current + consumers already depend on the domain leaf, so the shared dependency stays + inward without making prompt loading depend on orchestration or validation. +- **Required verification:** Run one shared table over every supported enum, + empty and unknown values, negative and non-negative repair counts, and + schema-path relationships. Retain source-specific prompt cases and require + `Prepare`, `Run`, and `PrepareExecution` request replacements to fail as + invalid requests before source completion, admission, or generation. Valid + contracts must normalize identically across both public preparation paths. + +### Retained Cross-Cutting Opportunities + +The following accepted component findings already identify the correct owner +or a decision-complete consolidation. They remain accepted without new IDs: + +- S02-F05 owns stable public JSON field mapping. Its repeated declarations are + one serialization policy; a wire alias or embedded representation can make + ordinary fields compile-time shared while keeping timing exceptions local. +- S07-F05 owns prompt selection across OS and `fs.FS` repositories. A source + adapter can leave real opening and display-path mechanics at the edge while + `internal/promptdef` owns one selector and normalizer. +- S08-F02 should make file profiles consume the existing + `internal/jsonvalue` invariant owner. The dependency is already valid for + other profile ingress; creating another JSON-shape validator is not needed. +- S12-F01 owns initial-versus-repair generation request construction. The two + calls follow one effective target policy, and a use-case-local request + constructor can share presence, credentials, backend identity, and + structured output without moving provider mapping out of `internal/llm`. +- S14-F06 owns repeated transport test scaffolding. A recording-provider + fixture belongs in the LLM test package and should consolidate mechanics, + not protocol assertions. + +### Efficiency Disposition + +No new efficiency finding was accepted. The demonstrated opportunities remain +owned by their component findings with the following cross-cutting cost and +verification constraints: + +| Finding | Frequency and scale | Evidence and remediation invariant | +| --- | --- | --- | +| S07-F04 | Every exact prompt lookup used by inspection, preparation, and ordinary execution; cost currently scales with all unrelated file-backed template bytes. | A counting filesystem observed one unrelated body open per lookup. Preserve point-in-time YAML scanning and duplicate detection while proving only selected content is opened once. | +| S08-F06 | Every profile lookup, for every YAML file in every consulted overlay source; cost scales with total catalog YAML bytes. | Source inspection confirms two decoder passes per file. Benchmark small and large catalogs with allocation counts, and preserve strict selected-file and repeated point-in-time behavior. | +| S09-F04 | Every `input` helper reference during rendering; cost scales with artifact bytes times references across the session and messages. | The 1 MiB probe measured approximately one additional body-sized allocation per reference. Benchmark one and repeated references while preserving exact bytes and per-render ownership. | +| S10-F03 | Every ordinary JSON-Schema preparation or run; cost scales with the root and transitive schema graph. | A counting source observed the root read twice in ordinary execution, and compiled-plan parity is also a correctness requirement. Prove each schema document is read once per operation without adding a cross-operation cache. | +| S10-F05 | Every generated artifact validated in JSON mode; cost scales with output bytes and nesting. | The approximately 1 MiB probe measured about 22.7 MiB and 250,031 allocations for a discarded tree versus a zero-allocation syntax scan. Retain scalar, object, large-array, trailing-data, and exact-number benchmarks and semantics. | + +The Stage 15 concurrency review found no avoidable lock contention or serial +work. Admission is a constant-time locked count update, generation locks cover +only pool bookkeeping, provider calls run unlocked, and separate backends use +separate pools. No scheduling change is justified without representative +contention measurements that preserve the documented FIFO and cancellation +linearization points. + +The model-client extra-parameter path validates values before marshaling the +whole request, so it can traverse that subgraph twice. As recorded in Stage +14, no representative measurement established material cost relative to a +provider call; it remains rejected as a standalone efficiency finding pending +a benchmark over realistic parameter sizes. JSON-value defensive copies at +public/internal ownership transfers are likewise contract work, not removable +overhead. S05-F03 already owns their missing depth and work bounds. + +### Rejected Candidates + +- **Facade method similarity:** The graph scored `Prepare` and `Run` as + identical and the two inspection methods nearly so. These are distinct + supported operations whose thin methods consistently perform nil checking, + boundary conversion, error mapping, and result conversion. Their substantive + selection and preparation work is already shared in `internal/usecase`; + hiding the public methods behind another abstraction would enforce no new + rule. +- **Source-option similarity:** File and `fs.FS` option constructors for + prompts, profiles, fallbacks, and schemas have identical small shapes, but + each sets a different typed source slot and precedence category in the + facade that owns assembly. A generic setter would weaken that distinction + without removing semantic policy. The broader prompt repository duplication + remains S07-F05. +- **File-catalog walker similarity:** `FindYAMLFiles` and `FindFSYAMLFiles` + repeat filtering, cancellation, sorting, and collection, but differ at the + standard-library OS-versus-`fs.FS` boundary and return different path forms. + They are short leaf adapters with shared filename helpers and no observed + behavioral drift. Repository-level selection duplication, where drift is + material, is already accepted. +- **Public/internal interface pairs:** `ArtifactReader` and `LLMClient` mirror + one-operation internal consumer interfaces, but their adapters translate + supported public values into private domain values and establish copying and + error boundaries. Merging them would expose internal representations; + S02-F03 owns missing protection for the LLM copy promise. +- **Validator capability interfaces:** `Validator`, `ValidationPreparer`, and + `SchemaDocumentLoader` describe different live, frozen, and document-only + capabilities. They are narrow, consumed conditionally, and preserve test and + source boundaries. S10-F03 may eliminate ordinary document-only schema work, + but metrics alone do not justify broadening or collapsing all capabilities. +- **Constructor width and facade concentration:** `NewRunner` and its internal + repair-capable variant accept eight and nine narrow collaborators, while + `NewEngine` has high graph centrality and transitive loop depth. The runner + is the assigned orchestration owner and the facade is the assigned assembly + root. A parameter object or service-locator wrapper would add indirection + without reducing responsibility or dependencies; `NewEngine`'s propagated + loop metric comes from one-time construction callees, not a nested runtime + hot path. +- **Complex source and copy functions:** Prompt loading and normalization are + the graph's largest source functions, while recursive JSON copying has the + highest cognitive score. S07-F04 and S07-F05 already identify the meaningful + prompt split and repeated work. JSON copying deliberately centralizes type + preservation, deterministic paths, cycle detection, and value validation; + S05-F03 owns its consequential resource risk. Splitting either solely to + lower a metric would obscure its invariant without changing cost. +- **Shallow map-helper similarity:** `copyShallowAnyMap` and `copyStringMap` + received a perfect token-similarity score but copy different value domains + at different boundaries. Each is a few lines, neither owns validation, and + a generic helper would add type indirection without credible drift risk. + +### Documentation And Dependency Check + +The implemented dependency direction and package responsibilities match the +architecture policy and the internal overview, runner, source, LLM, and +capacity documents. The root remains assembly and translation, use-case code +remains orchestration, provider mapping remains in `internal/llm`, and no +internal component imports a consumer. The two accepted consolidations would +expand `internal/domain` from shared values to their source-neutral +invariants; if implemented, the architecture policy and internal overview +must be updated in the same change rather than leaving validation ownership +implicit. + +### Unresolved Observations + +None. Every graph similarity, interface-width, complexity, repeated-work, +contention, transformation, and responsibility candidate reviewed above is +accepted under an existing or new finding or explicitly rejected with its +boundary rationale. + +### Verification Performed + +The refreshed code knowledge graph supplied the complete production interface +inventory, import map, structural similarity pairs, function complexity, +transitive loop depth, and call paths from public entry points to the candidate +owners. Exact graph snippets and source inspection confirmed each candidate; +no temporary benchmark or repository mutation was needed because the completed +confirmed findings already contained the required measurements and probes. +The repository-wide tests and static analysis also passed: + +```sh +go test ./... +go vet ./... +``` + +### Handoff + +- Stage 18 should merge S17-F01 with the shared root cause represented by + S02-F02, S08-F01, and S11-F01 without losing their source-specific error and + regression requirements. +- Stage 18 should merge S17-F02 with S11-F02 and retain the prompt-file cases + as integration protection for one domain-level output-contract rule. +- Previously accepted efficiency and consolidation findings remain separate + where their owners and remediation invariants differ. No lock optimization, + generic facade abstraction, or cross-operation cache should be added to the + final accepted set without new measurement or boundary evidence. +- The Stage 0 baseline remains absent and was not backfilled during this + synthesis-driven review.