diff --git a/docs/roadmap/complexity.md b/docs/roadmap/complexity.md new file mode 100644 index 0000000..e46e390 --- /dev/null +++ b/docs/roadmap/complexity.md @@ -0,0 +1,283 @@ +# 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.