284 lines
16 KiB
Markdown
284 lines
16 KiB
Markdown
# Maintainability And Complexity Audit
|
||
|
||
Reviewed revision: `320d0c5f439a059a7081cf7532c9b5d65891f5c7`.
|
||
Audit date: 2026-10-25.
|
||
|
||
## Scope And Method
|
||
|
||
This audit examines readability, responsibility boundaries, state handoffs, and
|
||
interface duplication. It covers the root facade, engine subsystems, document
|
||
and YAML boundaries, and relevant tests, documentation, and prior refactoring
|
||
decisions. It does not propose new feature behavior or performance guarantees.
|
||
|
||
The codebase-memory graph was refreshed and its function spans, cyclomatic and
|
||
cognitive complexity estimates, parameter counts, and caller traces were used
|
||
to select code for inspection. The graph contains 907 non-test functions/methods
|
||
and 940 test-file functions/methods. Its index excludes examples, documentation,
|
||
and scripts; tracked-file inventory and direct inspection supplemented it.
|
||
Standalone `gocyclo`, `gocognit`, and `golangci-lint` were unavailable. No tool
|
||
installation, benchmarks, or empirical performance testing was needed.
|
||
|
||
The tracked production inventory, excluding examples, contains 16 root Go files,
|
||
97 engine files, one document file, and two YAML adapter files: approximately
|
||
22,612 physical lines overall. The largest production files are `compose.go`
|
||
(599 lines), `history.go` (568), `overlay.go` (564), and `discovery.go` (540).
|
||
These measurements locate responsibilities; they are not refactoring thresholds.
|
||
|
||
Representative graph measurements:
|
||
|
||
| Function | File | Function span | Cognitive estimate |
|
||
| --- | --- | ---: | ---: |
|
||
| `valueOperation.clone` | `internal/engine/clone.go` | 130 | 104 |
|
||
| `acquireInput` | `internal/engine/acquisition.go` | 202 | 99 |
|
||
| `valueBuilder.build` | `internal/engine/value.go` | 104 | 93 |
|
||
| `decodeValue` | `internal/engine/decode.go` | 115 | 85 |
|
||
| `selectGlobImportCandidates` | `internal/engine/glob.go` | 112 | 73 |
|
||
| `assembleLayeredInput` | `internal/engine/load_request.go` | 146 | 63 |
|
||
| `executeLoadRequest` | `internal/engine/load.go` | 198 | 56 |
|
||
| `mergeOverlayMappingsWithCollectionPolicy` | `internal/engine/overlay.go` | 163 | 46 |
|
||
|
||
Spans include comments and blank lines; scores are tool estimates for comparative
|
||
triage, not standardized quality grades. Source review, rather than a numerical
|
||
cutoff, determines the findings below.
|
||
|
||
## Architectural Assessment
|
||
|
||
The [implemented architecture](../internal/overview.md) remains coherent: the
|
||
root owns the supported public types and explicit conversions; the engine owns
|
||
loading and frozen inspection; the document package owns the ordered tree; and
|
||
the YAML adapter isolates the upstream parser. Ordinary and layered loads share
|
||
one normalized request and execution lifecycle. Imports and inheritance retain
|
||
their separate scopes and use the common dependency scheduler.
|
||
|
||
The recent consolidation improved import ownership and file boundaries, separated
|
||
inheritance preparation/combination/finalization, shared filesystem facts, and
|
||
clarified schema and projection responsibilities. The findings concern remaining
|
||
maintenance opportunities, not a recommendation to repeat that refactor or split
|
||
the engine into a package per subsystem.
|
||
|
||
The existing [correctness](correctness.md) and [efficiency](efficiency.md) audits
|
||
remain separate owners of their findings. No additional runtime defect was
|
||
established in this pass. Related risks are referenced rather than duplicated.
|
||
|
||
## 1. Remove The Test-Only Eager Acquisition Mode
|
||
|
||
Status: open. Priority: medium. Scope: engine acquisition and its test helpers.
|
||
|
||
**Relevant code.** `inputAcquisitionRequest.deferAssembly` in
|
||
[acquisition.go](../../internal/engine/acquisition.go), and the final branch of
|
||
`acquireImportedWithAccess` in
|
||
[import_acquisition.go](../../internal/engine/import_acquisition.go), lines
|
||
206–224. Production construction sites in `executeLoadRequest`
|
||
([load.go](../../internal/engine/load.go), lines 210–214) and
|
||
`acquireUnderlayGroupScopes`
|
||
([load_request.go](../../internal/engine/load_request.go), lines 243–246) both
|
||
set this flag to true.
|
||
|
||
**Complexity.** Caller tracing found no production caller selecting eager
|
||
composition. The false branch remains exercised by private test entry helpers,
|
||
including `testImportRequest`, `testAcquireImport`, and `testAcquireInput` in
|
||
[imports_test.go](../../internal/engine/imports_test.go). Acquisition therefore
|
||
maintains two return protocols: an acquired scope with a nil composed root, or
|
||
an already composed document. The eager branch reconstructs `assembledInput`
|
||
and reenters composition from the acquisition function, while the published
|
||
loading paths perform that phase later. Tests can consequently exercise a
|
||
different handoff from the one consumers use.
|
||
|
||
**Potential refactoring.** Make acquisition return retained scopes consistently
|
||
and remove the unused production mode. Tests needing composed values can invoke
|
||
the real composition phase explicitly through a small test-only convenience
|
||
helper. Keep acquisition-only tests acquisition-only. Preserve handle closure,
|
||
pending-item placement reservation, accounting, diagnostics, and meaningful
|
||
composition coverage. This removes a private alternative path, not support for
|
||
any public loading workflow. Do not infer that other nil-state or directive
|
||
contexts are unused without separately tracing their callers.
|
||
|
||
## 2. Clarify Acquisition And Load-Coordinator Phase Handoffs
|
||
|
||
Status: open. Priority: medium. Scope: engine, across acquisition, version,
|
||
assembly, and lifecycle files.
|
||
|
||
**Relevant code.** `acquireInput` in
|
||
[acquisition.go](../../internal/engine/acquisition.go), `executeLoadRequest` in
|
||
[load.go](../../internal/engine/load.go), and `assembleLayeredInput` in
|
||
[load_request.go](../../internal/engine/load_request.go).
|
||
|
||
**Complexity.** `acquireInput` still combines search outcome handling, parsing,
|
||
disabled-directive checks, three directive/version-processing branches,
|
||
import-versus-standalone dispatch, placement/accounting, and passive context
|
||
construction. Its version-processing section repeats source-ID extraction,
|
||
failure handling, and `mainProcessing` updates, while the layered branch must
|
||
extract directives separately and restore them after version processing.
|
||
|
||
The coordinators also manipulate several intentionally distinct representations:
|
||
parsed source, local payload, acquired scope, composed root, inherited root,
|
||
local/protected version paths, and effective version evidence. For example,
|
||
`executeLoadRequest` includes a substantial ordinary-load resource-context
|
||
fallback between assembly and inheritance, while `assembleLayeredInput` repeats
|
||
declaration gathering, tier application, and destination projection for underlays
|
||
and main. Understanding a source-version or placement change requires tracing
|
||
both those mechanics and the overall phase order through long functions.
|
||
|
||
**Potential refactoring.** Extract cohesive operations for processing one parsed
|
||
input's directives/version evidence and for placing/registering a standalone
|
||
contribution. Reuse the existing processing result and acquisition state rather
|
||
than inventing another context hierarchy. Isolate the substantial context and
|
||
structural-expansion operations from the top-level lifecycle, and consider one
|
||
helper for the repeated tier application/projection handoff. Keep phase order,
|
||
error exits, and source acquisition order readable in the coordinators.
|
||
|
||
Extraction is useful only when it gives one operation a clear owner and result;
|
||
moving blocks into helpers that still share the same mutable surrounding locals
|
||
would not solve the problem. Preserve the deliberate ordinary/layered overlay
|
||
differences, default versus enabled version-check timing, warning accumulation,
|
||
exactly-once callbacks, and load-wide versus tier-local state. This is separate
|
||
from finding 1: removing the eager branch does not simplify these active paths.
|
||
|
||
## 3. Separate Collection-Policy Preparation From Merge Evidence Application
|
||
|
||
Status: open. Priority: medium. Scope: engine precedence and inheritance.
|
||
|
||
**Relevant code.** `mergeOverlayMappingsWithCollectionPolicy` and
|
||
`applyOverlaySequenceEvidence` in
|
||
[overlay.go](../../internal/engine/overlay.go);
|
||
`resolveStrictCollectionPolicies` in
|
||
[compose.go](../../internal/engine/compose.go);
|
||
`combineInheritanceTemplate` in
|
||
[inheritance_combine.go](../../internal/engine/inheritance_combine.go); and the
|
||
policy/origin helpers in
|
||
[inheritance_collection.go](../../internal/engine/inheritance_collection.go).
|
||
|
||
**Complexity.** The overlay coordinator prepares claims and declaration groups,
|
||
discovers applicable destinations, resolves operations, configures recursive
|
||
merge callbacks, accumulates diagnostics/history, reconciles declaration-only
|
||
policies after mutation, and sorts the output. Its callbacks close over several
|
||
maps and mutate the assembly while reporting failures through an outer slice.
|
||
`applyOverlaySequenceEvidence` then mixes policy retirement, displaced origins,
|
||
replacement records, copied incoming origins, and record-policy reconstruction.
|
||
|
||
There is also concrete duplicated bookkeeping: overlay and inheritance callbacks
|
||
both translate a sequence result into replace-versus-mapped history descriptors;
|
||
overlay evidence and `applyInheritanceSequencePolicy` both rebuild record policy
|
||
order from lower/higher item projections. The shared `combineSequence` is already
|
||
the correct value-combination owner, but maintainers must separately coordinate
|
||
these sidecar translations when its result representation changes.
|
||
|
||
**Potential refactoring.** Give overlay preparation, value/evidence application,
|
||
and declaration-only finalization explicit local boundaries. Let a small
|
||
operation owner or result carry the diagnostic and evidence collections currently
|
||
captured by the large callbacks. Share the straightforward history-descriptor
|
||
construction; evaluate a common projected-record selection helper while retaining
|
||
named precedence and inheritance ownership rules.
|
||
|
||
Do not merge the distinct origin semantics or hide mode-lifetime differences
|
||
behind a generic strategy framework. Keep `combineSequence` and the recursive
|
||
mapping merge shared, and consume their result rather than recomputing values.
|
||
Coordinate this work with collection-policy findings 1–2 in the
|
||
[efficiency audit](efficiency.md) so clearer preparation does not preserve or
|
||
introduce repeated whole-table copying/scanning. Preserve replacement ordering,
|
||
pending declarations, new versus retained record owners, and stable history.
|
||
|
||
## 4. Extract Patterned Glob Entry Preparation From Traversal Orchestration
|
||
|
||
Status: open. Priority: medium. Scope: engine import discovery.
|
||
|
||
**Relevant code.** `selectGlobImportCandidates` in
|
||
[glob.go](../../internal/engine/glob.go), especially its patterned-entry branch.
|
||
The corresponding directory-search entry operation already has a named owner,
|
||
`prepareImportDirectoryEntry` in
|
||
[import_selection.go](../../internal/engine/import_selection.go).
|
||
|
||
**Complexity.** The glob function nests pattern-segment traversal, parent
|
||
candidates, and directory entries. Its innermost branch handles matching,
|
||
hidden-entry policy, lexical and access paths, containment, link inspection,
|
||
follow behavior, intermediate-directory eligibility, native/host identity
|
||
differences, canonicalization, labels, and candidate creation. Set-union and
|
||
component-advancement mechanics are interleaved with those admission decisions,
|
||
making the important lexical-versus-canonical distinction difficult to follow.
|
||
|
||
**Potential refactoring.** Extract a glob-specific entry preparation operation
|
||
returning an admitted candidate, an ordinary skip, or a diagnostic. Leave pattern
|
||
component advancement, literal segments, per-component union/sorting, and final
|
||
exclusion/requiredness visible in the selector. Reuse low-level filesystem facts
|
||
where semantics coincide; directory and glob admission need not become one
|
||
general visitor interface. Preserve inspection/error order, entry accounting,
|
||
native namespace spelling, and selected-file validation. This is a readability
|
||
change, not a proposal to change the selected discovery algorithm or add caching.
|
||
|
||
## 5. Simplify Redundant Facade-To-Engine Input Transport
|
||
|
||
Status: open. Priority: low. Scope: root facade and engine, across packages.
|
||
|
||
**Relevant code.** Root `toEngineImportDefaults` in
|
||
[conversion.go](../../conversion.go), `toEngineOptions` in
|
||
[lifecycle.go](../../lifecycle.go), and `toEngineLayeredOptions` in
|
||
[layered.go](../../layered.go); engine transport/private defaults and option
|
||
decorators in [import_spec.go](../../internal/engine/import_spec.go), lines
|
||
46–87; and placement transport helpers in
|
||
[destination.go](../../internal/engine/destination.go), lines 23–39.
|
||
|
||
**Complexity.** Import defaults have a root-owned public representation, an
|
||
engine facade-transport representation, and a second engine-private representation
|
||
with the same five fields. Conversion constructs the transport, an option
|
||
decorator converts it to private defaults, and load normalization snapshots it
|
||
again. Placement similarly requires constructing an engine record and then
|
||
passing it through a separate setter-style helper. Even a simple control change
|
||
has multiple mechanical mapping sites across the package boundary.
|
||
|
||
Caller and text-reference checks also found no uses of the standalone
|
||
`PlacementModeFromFacade` and `PlacementModeForFacade` functions beyond their
|
||
declarations. The similarly named `SourceInfoPlacementModeForFacade` is used by
|
||
root source-record conversion and is not dead code. The unused pair adds a
|
||
second apparent placement conversion route for maintainers to investigate.
|
||
|
||
Root type ownership and explicit conversion are deliberate and useful; the
|
||
opportunity is the extra engine-internal representation/construction protocol,
|
||
not eliminating the facade. The public wrappers do not need to change.
|
||
|
||
**Potential refactoring.** Remove the two unused enum bridges. Then consider one
|
||
engine representation of import-default input values and a straightforward
|
||
construction surface on the existing engine option/input records. Review inert
|
||
placement decorators as part of the same bounded cleanup. Keep helpers that
|
||
actually encode domain rules or ownership conversion, and keep
|
||
definition/registration compilation separate from transport.
|
||
Avoid adding another universal request DTO, code generator, reflective mapper,
|
||
or adapter framework to remove a few mechanical functions.
|
||
|
||
Preserve root-owned named types and signatures, invalid-enum validation,
|
||
nil-versus-empty patterns, pointer/slice snapshots, and the two public loading
|
||
entry points. Exported declarations inside Go's `internal` boundary need not
|
||
become public root aliases or bridge methods. Pursue this low-priority cleanup
|
||
only if it reduces total concepts and mapping sites, rather than relocating them.
|
||
|
||
## Areas To Retain And Review Boundaries
|
||
|
||
- The clone traversal was explicitly reviewed and retained in the completed
|
||
consolidation record at `34cbc0d:docs/roadmap/consolidation.md`. Its high score
|
||
is not new evidence for reopening that work. `valueBuilder.build`, typed
|
||
decoding, and the YAML translator also contain substantial but recognizable
|
||
type dispatch; no repository-wide visitor framework is recommended.
|
||
- The cohesive engine package and explicit root record conversions follow
|
||
[ADR 0006](../adr/0006-public-facade.md). Function/file counts alone do not
|
||
justify package extraction, public API consolidation, or generated conversions.
|
||
- The shared scheduler, mapping/sequence primitives, recently separated
|
||
inheritance phases, schema field assembly, and resolved filesystem facts remain
|
||
useful common owners. Preserve those improvements.
|
||
- `history.go` and the larger integration-test files have several substantial
|
||
routines, but file size alone did not establish another prioritized refactor.
|
||
The deferred replay, copy-handoff, caching, and contributor-index experiments
|
||
remain in [future work](future.md#3-performance-experiments).
|
||
|
||
These are opportunities for further consideration, not a staged implementation
|
||
plan or new release blockers. Favor the concrete unused-mode removal and active
|
||
acquisition/collection boundaries before lower-value transport cleanup. A later
|
||
implementation should preserve the established contracts and relevant behavioral
|
||
coverage; complexity scores, helper counts, and private call sequences should
|
||
not become test thresholds or repository guarantees.
|
||
|
||
## Validation
|
||
|
||
This audit changes only this findings document. No production code, tests,
|
||
dependencies, policies, or public contracts are changed. Validation consists of
|
||
the existing local documentation-link test and `git diff --check`; runtime and
|
||
performance measurements are not part of this review.
|