Record complexity audit findings
This commit is contained in:
250
docs/roadmap/remediation.md
Normal file
250
docs/roadmap/remediation.md
Normal file
@@ -0,0 +1,250 @@
|
|||||||
|
# Complexity Audit Findings
|
||||||
|
|
||||||
|
Status: open recommendations for consideration. This is an audit record, not an
|
||||||
|
implementation plan or a new release gate.
|
||||||
|
|
||||||
|
Reviewed revision: `5959537d1adc2516e7f7e8881df72b707ec48c3f`.
|
||||||
|
|
||||||
|
## Scope And Method
|
||||||
|
|
||||||
|
The review considered production Go code, package boundaries, private interfaces,
|
||||||
|
and supporting tests. The local codebase-memory graph was refreshed, and its
|
||||||
|
function-size, cognitive-complexity, branch, and parameter metrics were used to
|
||||||
|
rank candidates. Source inspection and inbound call traces then distinguished
|
||||||
|
mixed responsibilities from ordinary dispatch and deliberate boundary code.
|
||||||
|
Text-reference searches checked the apparently unused conversions in finding 6.
|
||||||
|
No benchmark campaign or runtime correctness testing was part of this audit.
|
||||||
|
|
||||||
|
The graph indexed 901 non-test functions/methods and 928 test-file declarations.
|
||||||
|
Its fast index excludes examples, documentation, and scripts; the tracked-file
|
||||||
|
inventory covered those separately. Standalone `gocyclo`, `gocognit`, `scc`, and
|
||||||
|
`cloc` executables were not available. The following scores are the graph tool's
|
||||||
|
reported cognitive scores, used for relative triage rather than as limits or
|
||||||
|
standardized acceptance criteria. Function spans include comments and blank
|
||||||
|
lines within the declaration.
|
||||||
|
|
||||||
|
| Function | Source | Lines | Cognitive score |
|
||||||
|
| --- | --- | ---: | ---: |
|
||||||
|
| `valueOperation.project` | [projection.go](../../internal/engine/projection.go) | 195 | 174 |
|
||||||
|
| `parseImportDeclarationSpec` | [import_spec.go](../../internal/engine/import_spec.go) | 127 | 129 |
|
||||||
|
| `valueOperation.clone` | [clone.go](../../internal/engine/clone.go) | 130 | 104 |
|
||||||
|
| `schemaBuilder.describeFields` | [schema.go](../../internal/engine/schema.go) | 99 | 100 |
|
||||||
|
| `assembleBaseInputTrackedForRole` | [load_input.go](../../internal/engine/load_input.go) | 188 | 96 |
|
||||||
|
| `selectDirectoryImportCandidates` | [import_selection.go](../../internal/engine/import_selection.go) | 154 | 96 |
|
||||||
|
| `valueBuilder.build` | [value.go](../../internal/engine/value.go) | 104 | 93 |
|
||||||
|
| `decodeValue` | [decode.go](../../internal/engine/decode.go) | 115 | 85 |
|
||||||
|
| `applyInheritanceScopeTemplates` | [inheritance_schedule.go](../../internal/engine/inheritance_schedule.go) | 219 | 75 |
|
||||||
|
| `contextsForAcquiredSources` | [source_context.go](../../internal/engine/source_context.go) | 102 | 72 |
|
||||||
|
|
||||||
|
The private engine contains 88 production Go files and approximately 20,144
|
||||||
|
physical lines, compared with 16 files and 1,861 lines in the root facade.
|
||||||
|
`imports.go` is the largest production file at 1,195 lines. Size identifies where
|
||||||
|
to inspect; it does not establish that the engine needs to be split into packages.
|
||||||
|
|
||||||
|
Findings 1–3 confirm opportunities already selected in the pending
|
||||||
|
[complexity roadmap](complexity.md). Findings 4–6 add narrower opportunities.
|
||||||
|
They should be coordinated with that roadmap rather than implemented as an
|
||||||
|
independent overlapping refactor. No ordinary correctness defect was established
|
||||||
|
by this static review.
|
||||||
|
|
||||||
|
## 1. Consolidate Import Entry Points And Separate Import Responsibilities
|
||||||
|
|
||||||
|
Status: open. Priority: higher maintenance benefit. Scope: `internal/engine`.
|
||||||
|
|
||||||
|
**Evidence and complexity.** [imports.go](../../internal/engine/imports.go)
|
||||||
|
contains parallel forwarding ladders such as
|
||||||
|
`loadImportedDocumentsTrackedWithVersion`,
|
||||||
|
`loadImportedDocumentsTrackedWithProcessing`, and
|
||||||
|
`loadImportedDocumentsTrackedWithProcessingDeferred`, plus corresponding
|
||||||
|
`loadImportedWithAccess...` variants. Both final signatures carry 13 declared
|
||||||
|
parameters, counting names in grouped declarations.
|
||||||
|
Related values must remain aligned across these calls: placement, version state,
|
||||||
|
processing results, sequence policy, accounting, repetition tracking, namespace
|
||||||
|
identity, and deferred assembly.
|
||||||
|
|
||||||
|
The same file also owns source admission, filesystem access lifetime, recursive
|
||||||
|
acquisition, pending-item assembly, and evidence projection. Source admission in
|
||||||
|
`importTraversal.visitPath` is 174 lines. The 188-line input assembler in
|
||||||
|
[load_input.go](../../internal/engine/load_input.go) adds discovery, source-role,
|
||||||
|
directive, version, placement, and resource-context decisions around that flow.
|
||||||
|
Import defaults are carried through `sourceVersionState`, although import
|
||||||
|
defaulting and version checking are independent concerns. Readers have to follow
|
||||||
|
that incidental state relationship to understand ordinary import behavior.
|
||||||
|
|
||||||
|
**Potential refactoring.** Follow the roadmap's cohesive request/context design,
|
||||||
|
with named inputs and explicit ownership of load-wide state. Remove forwarding
|
||||||
|
variants that only add another optional argument; retain helpers that express
|
||||||
|
distinct operations. Give acquisition, already-acquired scope composition, and
|
||||||
|
evidence projection focused homes. Keep load-wide import defaults with an
|
||||||
|
appropriate acquisition/load context rather than requiring version state to be
|
||||||
|
their transport.
|
||||||
|
|
||||||
|
Within that work, extract coherent declaration-field parsing and directory-entry
|
||||||
|
admission operations from `parseImportDeclarationSpec` and
|
||||||
|
`selectDirectoryImportCandidates`. Keep their selector-policy checks and
|
||||||
|
deterministic traversal visible. Moving code into smaller functions without
|
||||||
|
clarifying state handoffs would not resolve the main maintenance problem.
|
||||||
|
|
||||||
|
Preserve acquisition order, strict scope scheduling, item-local composition,
|
||||||
|
source/version evidence, existing limits, and explicit resource closure. The
|
||||||
|
existing import/discovery and ordinary/layered integration tests are the relevant
|
||||||
|
behavioral protection.
|
||||||
|
|
||||||
|
## 2. Expose The Phases Of The Shared Inheritance Executor
|
||||||
|
|
||||||
|
Status: open. Priority: higher maintenance benefit. Scope: `internal/engine`.
|
||||||
|
|
||||||
|
**Evidence and complexity.** `applyInheritanceScopeTemplates` in
|
||||||
|
[inheritance_schedule.go](../../internal/engine/inheritance_schedule.go)
|
||||||
|
coordinates template preparation, directive controls, retained-node accounting,
|
||||||
|
parent identity exclusion, inherited collection-policy preparation, recursive
|
||||||
|
mapping/sequence combination, origin transfer, replacement history, replay
|
||||||
|
operations, final policy projection, and scope rebuilding in one 219-line body.
|
||||||
|
Its merge callbacks mutate several surrounding results, so the evidence produced
|
||||||
|
by one combination is difficult to assess independently of the whole executor.
|
||||||
|
|
||||||
|
**Potential refactoring.** Keep the useful mapping/identified-sequence adapters
|
||||||
|
and one shared executor. Extract parent/child preparation, combination, evidence
|
||||||
|
construction, and accounting into meaningful operations with small, explicit
|
||||||
|
result records where appropriate. Make it clear which values are borrowed
|
||||||
|
templates and which are owned resolved results. Keep one sequence-combination
|
||||||
|
result as the source of value, policy, origin, and historical-position evidence.
|
||||||
|
|
||||||
|
This is the inheritance refactor already described in the complexity roadmap.
|
||||||
|
Preserve dependency-ready ordering, enclosing-before-nested evaluation, child
|
||||||
|
identity, policy ownership, history budgets, and sensitivity. Reuse the mapping
|
||||||
|
and identified-record inheritance, history, and ownership tests.
|
||||||
|
|
||||||
|
## 3. Decompose Reflective Projection And Review Clone Handlers Separately
|
||||||
|
|
||||||
|
Status: open. Priority: higher maintenance benefit for projection; secondary for
|
||||||
|
cloning. Scope: `internal/engine`.
|
||||||
|
|
||||||
|
**Evidence and complexity.** `valueOperation.project` in
|
||||||
|
[projection.go](../../internal/engine/projection.go) has the highest reported
|
||||||
|
cognitive score. Its struct branch alone handles member traversal, inline
|
||||||
|
flattening, key collisions, flattened-container accounting, and secret-field
|
||||||
|
classification. The enclosing dispatcher also handles pointer/cycle lifetime,
|
||||||
|
dynamic descriptors, maps, sequences, and scalar codecs.
|
||||||
|
`valueOperation.clone` in [clone.go](../../internal/engine/clone.go) has a similar
|
||||||
|
large dispatcher, including distinct secret-copy modes and text-codec round trips.
|
||||||
|
|
||||||
|
**Potential refactoring.** Implement the roadmap's focused struct/map/sequence
|
||||||
|
projection handlers while retaining `valueOperation` as the traversal-state
|
||||||
|
owner. Keep the dispatch and scalar representation decisions easy to locate.
|
||||||
|
Apply the same readability review to the substantial clone branches as a separate
|
||||||
|
small follow-up if it remains beneficial; keep cloning and projection as distinct
|
||||||
|
operations.
|
||||||
|
|
||||||
|
The high-scoring `valueBuilder.build` and `decodeValue` are predominantly explicit
|
||||||
|
type dispatch; decoding already delegates collection handling. Their scores alone
|
||||||
|
do not justify a generic reflection visitor. Share only operations whose semantics
|
||||||
|
actually match. Preserve balanced cycle tracking, depth/node accounting, inline
|
||||||
|
order, nil/empty distinctions, codecs, and secret behavior. Existing projection,
|
||||||
|
clone, schema, decoding, callback-ownership, and independent-load tests cover the
|
||||||
|
important risks.
|
||||||
|
|
||||||
|
## 4. Separate Schema Field Discovery From Inline Field Assembly
|
||||||
|
|
||||||
|
Status: open. Priority: medium maintenance benefit. Scope: `internal/engine`.
|
||||||
|
|
||||||
|
**Evidence and complexity.** `schemaBuilder.describeFields` in
|
||||||
|
[schema.go](../../internal/engine/schema.go) nests field enumeration and tag
|
||||||
|
handling around pointer-chain inspection, recursive-inline detection, inline
|
||||||
|
struct flattening, index-path construction, inline-map admission, duplicate
|
||||||
|
checks, secret propagation, and construction of both `members` and flattened
|
||||||
|
`fields`. Those two descriptor views serve different traversal needs, and their
|
||||||
|
consistency currently depends on understanding the full nested routine.
|
||||||
|
|
||||||
|
**Potential refactoring.** Retain one schema builder/cache. Extract named inline
|
||||||
|
struct and inline-map assembly operations and make ordinary-field registration
|
||||||
|
clear. Small helpers can centralize the paired updates to `members`, `fields`,
|
||||||
|
`fieldByName`, and `inlineMap`, including inherited secret classification, where
|
||||||
|
the same registration semantics apply. Keep pointer and recursive-type decisions
|
||||||
|
explicit rather than moving them into a configurable registration framework.
|
||||||
|
|
||||||
|
Preserve accepted field/tag forms, cache/building-state behavior, declaration
|
||||||
|
order, ambiguity diagnostics and paths, and schema/projection/decode agreement.
|
||||||
|
Existing inline, recursive-schema, text-codec, secret, and ambiguity tests are the
|
||||||
|
appropriate protection.
|
||||||
|
|
||||||
|
## 5. Give Resolved Filesystem Context A Clear Shared Representation
|
||||||
|
|
||||||
|
Status: open. Priority: medium maintenance benefit. Scope: acquisition and passive
|
||||||
|
context handling within `internal/engine`.
|
||||||
|
|
||||||
|
**Evidence and complexity.** [filesystem.go](../../internal/engine/filesystem.go)
|
||||||
|
derives host/native roots, selected-file bases, and containment during import and
|
||||||
|
context validation. `contextsForAcquiredSources` in
|
||||||
|
[source_context.go](../../internal/engine/source_context.go) independently derives
|
||||||
|
much of the same information, then computes each source's root-relative base.
|
||||||
|
It branches among native sources, explicit native contexts, selected host files,
|
||||||
|
absent host input, and in-memory sources before branching again for relative-path
|
||||||
|
calculation. The lifecycle calls it from ordinary acquisition and both selected
|
||||||
|
overlay paths, so a change in context derivation has several integration points.
|
||||||
|
|
||||||
|
**Potential refactoring.** Consider a small engine-owned resolved-context record
|
||||||
|
that names the namespace, root, selected-source path, and base directory. Share
|
||||||
|
the overlapping derivation/relative-base operations across acquisition and
|
||||||
|
passive-context construction, while keeping host and `fs.FS` path rules explicit.
|
||||||
|
Pass already-resolved facts where they are available instead of reconstructing
|
||||||
|
them from source-kind and optional-context combinations.
|
||||||
|
|
||||||
|
Validation, acquisition access, and returned passive views have different jobs;
|
||||||
|
a refactor should preserve their existing timing and ownership. In particular,
|
||||||
|
reuse of a context description need not retain an acquisition handle or make
|
||||||
|
previously deferred checks eager. Preserve selected-file identity, source-ID
|
||||||
|
association, containment diagnostics, absent-input behavior, and independent
|
||||||
|
overlay contexts. Existing native/host filesystem, discovery integration,
|
||||||
|
resource-context, and facade tests provide the behavioral baseline.
|
||||||
|
|
||||||
|
## 6. Remove Unused Reverse Inspection Conversions Across The Facade Boundary
|
||||||
|
|
||||||
|
Status: open. Priority: smaller, well-bounded cleanup. Scope: root `configkit` and
|
||||||
|
`internal/engine` together.
|
||||||
|
|
||||||
|
**Evidence and complexity.** In [conversion.go](../../conversion.go),
|
||||||
|
`toEngineSourceInfo` is called only by three tests in
|
||||||
|
[records_test.go](../../records_test.go). Its helpers
|
||||||
|
`toEngineSourceVersionCheck` and `toEngineImportSelections` have no callers outside
|
||||||
|
that reverse-conversion chain. `toEngineOrigin` has no callers. The corresponding
|
||||||
|
engine setters `SourceInfoWithImportSelections` and `SourceInfoWithPlacement` in
|
||||||
|
[records.go](../../internal/engine/records.go) exist only for that chain, as do
|
||||||
|
`importSelectorFromString` and `importSelectionOutcomeFromString`.
|
||||||
|
|
||||||
|
Consumers receive these inspection records; the production facade does not send
|
||||||
|
them back into the engine. Maintaining an unused reverse path adds a second
|
||||||
|
representation/ownership mapping whenever inspection evidence grows, and the
|
||||||
|
round-trip tests make that machinery appear necessary despite its lack of a
|
||||||
|
runtime caller.
|
||||||
|
|
||||||
|
**Potential refactoring.** Remove the unused inverse conversions and their
|
||||||
|
exclusive engine-side adapters together. Rewrite the affected tests around
|
||||||
|
engine-to-public conversion and detached public query records, preserving useful
|
||||||
|
placement, version-evidence, and import-selection ownership checks. Continue to
|
||||||
|
exercise real source creation/loading through the existing facade integration
|
||||||
|
tests. This can reduce both production interface surface and test scaffolding
|
||||||
|
without adding a transport framework.
|
||||||
|
|
||||||
|
Retain the forward inspection conversions and root-owned public types. Other
|
||||||
|
reverse conversions have real users: for example, `toEngineDiagnostics` supports
|
||||||
|
the public diagnostic renderer. Apply caller-based removal rather than deleting
|
||||||
|
conversions merely because they look symmetrical.
|
||||||
|
|
||||||
|
## Architectural Assessment And Follow-up
|
||||||
|
|
||||||
|
The package direction remains coherent: root-owned public wrappers delegate to
|
||||||
|
the engine; the engine uses the ordered document model and the private YAML
|
||||||
|
adapter. Most complexity is concentrated in multi-responsibility engine routines,
|
||||||
|
rather than distributed through unnecessary packages. The smaller document and
|
||||||
|
YAML packages have clear responsibilities; their tree visitors and the facade's
|
||||||
|
explicit conversions are not inherently maintenance defects.
|
||||||
|
|
||||||
|
The strongest next investment is the pending complexity roadmap's import,
|
||||||
|
inheritance, and projection work. Finding 6 is an independently reviewable cleanup;
|
||||||
|
findings 4 and 5 can be assessed as focused follow-ups. Package extraction or
|
||||||
|
public API redesign is not needed to pursue these findings. Keep the current
|
||||||
|
contracts as the behavioral baseline and use the
|
||||||
|
[development guide](../development.md) to select proportionate validation during
|
||||||
|
implementation. This audit introduces no new complexity thresholds, configuration
|
||||||
|
restrictions, ownership guarantees, or release requirements.
|
||||||
Reference in New Issue
Block a user