diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..e4d1191 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -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.