Document cross-cutting architecture audit findings
This commit is contained in:
269
audit.md
269
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.
|
||||
|
||||
Reference in New Issue
Block a user