From 26771dc03e874a6e025c6bd32fcbad2375b5e643 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 24 Oct 2026 22:04:26 +0000 Subject: [PATCH] Consolidate refactoring roadmaps and record open questions --- docs/development.md | 3 + docs/roadmap/complexity.md | 227 ------------------------------ docs/roadmap/consolidation.md | 228 ++++++++++++++++++++++-------- docs/roadmap/remediation.md | 255 ---------------------------------- 4 files changed, 176 insertions(+), 537 deletions(-) delete mode 100644 docs/roadmap/complexity.md delete mode 100644 docs/roadmap/remediation.md diff --git a/docs/development.md b/docs/development.md index d1f0a5a..277927e 100644 --- a/docs/development.md +++ b/docs/development.md @@ -189,6 +189,9 @@ remediation record are preserved in Git history at `0369111`, for example [configuration discovery](config.md#configuration-discovery); the public [discovery declarations](../discovery.go) own the Go API. +The [consolidation roadmap](roadmap/consolidation.md) owns the current refactoring +proposal and the associated complexity-audit findings. + Potential enhancements are tracked in [future features](roadmap/future.md). Install the Go version required by `go.mod` or a compatible newer toolchain. diff --git a/docs/roadmap/complexity.md b/docs/roadmap/complexity.md deleted file mode 100644 index ebe65be..0000000 --- a/docs/roadmap/complexity.md +++ /dev/null @@ -1,227 +0,0 @@ -# Internal Complexity Refactoring Roadmap - -Status: proposed target state. This roadmap defines a behavior-preserving -refactor; a later implementation plan will define stages and exact private -signatures and file names. - -The combined proposal for the next scope is -[ConfigKit Consolidation Toward 1.0](consolidation.md). It incorporates this -internal target, the later audit, and a review of consumer-facing simplification. -Use that proposal for scope review; this document retains the earlier analysis. - -## Purpose - -Make ConfigKit's import, inheritance, and projection implementation easier to -understand, review, and change while preserving its public APIs and configuration -behavior. Concentrate on functions that coordinate several responsibilities and -carry substantial shared state. - -The target is clearer responsibility and ownership. Adding a few well-named -helpers, private records, or focused files can serve that target even when total -line count increases. Complexity measurements help locate candidates; they do -not establish function-size limits, score thresholds, or release gates. - -The [architecture policy](../policy/architecture.md) owns package and ownership -constraints. The [configuration contract](../config.md) and exported Go comments -remain authoritative for consumer behavior. - -## Current Baseline - -Reviewed baseline: `7a83704f43ef9870db33661231cdff5d6ee502f7`. - -The root facade delegates to a cohesive private engine, supported by the ordered -document representation and YAML adapter. Most functions are small. The -complexity audit identified these concentrated maintenance costs: - -| Area | Current implementation | Required improvement | -| --- | --- | --- | -| Imports | `imports.go` spans acquisition, item-local composition, and evidence projection. Private loading variants pass long positional argument lists. Source admission and directory selection contain substantial mixed control flow. | Give import entry points explicit, cohesive inputs and separate the responsibilities of acquisition, scope assembly, and evidence projection. | -| Inheritance | `applyInheritanceScopeTemplates` coordinates payload preparation, parent combination, policy ownership, origins, history, and accounting in one executor. | Keep one readable executor with focused operations and explicit state handoffs. | -| Projection | `valueOperation.project` combines type dispatch with struct flattening, map and sequence traversal, scalar conversion, sensitivity, and accounting. | Retain clear dispatch while moving substantial value-kind handling into focused helpers. | - -The implemented inventory belongs to the -[component overview](../internal/overview.md). Existing import flow and scheduling -are described in [import internals](../internal/imports.md). - -## Target Architecture - -The refactor stays within `internal/engine`. The root facade, `internal/document`, -and `internal/yaml` retain their existing responsibilities and dependency -direction. Focused files and ordinary private functions express the new internal -organization. Package extraction is outside this roadmap's selected design. - -`executeLoadRequest` remains the visible coordinator of the shared load lifecycle. -Its phase order should remain easy to follow. Helpers expose meaningful work and -results rather than distributing that order across a generic execution framework. - -Existing shared mechanisms remain shared: recursive mapping combination, -identified-sequence operations, dependency-ready scheduling, schema descriptors, -and projection/accounting state. New helper boundaries make their ownership and -use clearer without creating parallel implementations. - -### Imports: Cohesive Inputs And Scope Ownership - -Import loading has a clear private entry surface using named request/context -fields for related inputs. Reuse the existing load-request, acquisition-scope, -and traversal models where they fit. Consolidate forwarding variants whose only -purpose is passing another optional positional argument through the chain. -Retain convenience helpers when they express a useful, distinct operation. - -Keep these state lifetimes identifiable: - -- Load-wide state: accounting, shared inclusion/repetition information, - namespaces, and source-version processing. -- Acquisition requests: source and filesystem context, placement, declarations, - applicable policy, and the relevant assembly handoff. -- Composition scopes: peer contributions, item-local helpers, record ownership, - and pending declarations. -- Evidence: physical source records, effective destinations, origins, and retained - history projections. - -Group acquisition, pending-item assembly, and evidence projection in focused -files. A caller should be able to identify which operation acquires resources, -which composes already acquired content, and which projects retained evidence. -Shared mutable state has a visible owner; a request record need not duplicate -state already held by that owner. - -Within dense admission, directory-selection, and declaration-parsing routines, -extract coherent operations where that reduces nested, mixed responsibilities. -Preserve the existing selection algorithms and host/native filesystem behavior. -Resource ownership and closure remain explicit across success and failure paths. - -The resulting flow preserves the distinction among acquisition order, strict -scope scheduling, and precedence. Completing a helper scope before placing its -item remains part of the same import flow. Ordinary and layered loading continue -to share the applicable mechanisms. - -### Inheritance: One Executor With Explicit Phases - -Keep the mapping and identified-sequence adapters and their shared template -executor. The executor coordinates focused operations for: - -- Preparing child payload, directives, and destination-local state. -- Preparing the resolved parent payload and applicable inherited policy. -- Combining values through the existing mapping and sequence mechanisms. -- Producing policy, origin, replacement, and replay evidence from that combination. -- Accounting for expansion and retained history under the existing budgets. - -These are responsibility boundaries, not a required helper count or a new -scheduler. Small private result records are appropriate when related outputs -otherwise travel through long argument lists or implicit mutations. - -Authored templates, resolved values, scoped policy, and inspection evidence remain -distinguishable. Immutable borrowed inputs and owned mutable results have clear -lifetimes. Nested instructions remain available for destination-local evaluation. -For identified-record inheritance, the child's own identity fields and origins -remain intact; the parent's identity fields are excluded from inherited payload -and its history, as the current contract requires. - -One sequence-operation result continues to drive value construction, record -ownership, origins, and historical position projection. Helpers consume that -evidence directly. Scheduling remains dependency-ready, enclosing relationships -resolve before nested ones, and assembled sequence order remains unchanged. - -### Projection: Explicit Value-Kind Handlers - -Keep `projectConfiguration` as the schema-checked entry and `valueOperation` as -the owner of per-operation traversal and accounting state. The central dispatch -clearly selects the appropriate handling for pointers, dynamic values, structs, -maps, sequences, and scalars. - -Substantial struct, map, and sequence work lives in focused handlers. Struct -projection retains declaration order, inline flattening, collision diagnostics, -and sensitivity propagation. Map projection retains deterministic key ordering. -Scalar conversion retains the existing number, duration, text-codec, and secret -reference representations. - -Depth and node accounting retain their existing meaning, including flattened -containers. Cycle tracking stays active through recursive child traversal and -is balanced on error paths as well as success. Passing state through a helper -does not introduce another full-tree projection or an unnecessary ownership copy. - -Cloning and source-value construction remain distinct operations with their -existing semantics. Reuse small helpers when the semantics coincide; keep the -projection refactor centered on making the existing traversal readable. - -## Behavior And Scope Preservation - -The implementation preserves: - -- Root-owned public types, signatures, supported import paths, and facade boundaries. -- Supported source forms, opt-ins, import controls, placement, acquisition and - composition ordering, and source/version evidence. -- Strict peer composition, precedence operations, identity agreement and lifetime, - record ownership, and destination-local inheritance. -- The load lifecycle, including typed defaults, post-decode CLI assignment, - recorded application resolution, and final validation. -- Explicit presence, provenance, history, sensitivity, resource contexts, and - detached publication/query results. -- Established diagnostic codes, semantic paths, source coordinates, ordering, - resource lifetime, and existing limit behavior. - -The owning contracts define the extent of these obligations. Incidental private -call sequences, exact helper names, and allocation counts do not acquire new -guarantees through this roadmap. The work adds no consumer options, configuration -syntax, dependencies, or complexity-based gates. - -## Relationship To Remediation And Other Work - -The completed remediation record at `ea46001:docs/roadmap/remediation.md` -documents the identified correctness and algorithmic efficiency findings. -Several overlap these files, particularly item-scope application matching and -inheritance policy localization. - -Use the documented contract and completed remediation as the behavioral baseline. -Keep corrective changes reviewable under their findings, and retain their -regressions through this refactor. The later implementation plan should coordinate -overlapping work and select its starting revision rather than duplicating the -remediation plan here. These structural changes do not close a finding merely -because its function moved or became smaller. - -[Configuration discovery](../config.md#configuration-discovery) is implemented. -Import cleanup should preserve its source-acquisition boundary and behavior as -part of this refactor. - -## Work Needed And Validation - -Implementation needs to consolidate import inputs and adapters, separate import -responsibilities, extract inheritance operations with explicit outputs, and -decompose projection handling. Remove superseded private plumbing as its callers -migrate. Review the resulting call flow for repeated traversals, policy scans, -copies, or hidden state transitions introduced by extraction. - -Reuse the existing behavioral suites under the -[development guide](../development.md) and [testing policy](../policy/testing.md): - -- Imports: selection, scheduling, source/version evidence, placement, helper scopes, - ordinary/layered equivalence, filesystem failures, and resource lifetime. -- Inheritance: mapping and record relationships, nested destination resolution, - cancellation and replacement, policy ownership, history, sensitivity, and - independent loads. -- Projection: schema/decoding agreement, inline fields, text codecs, nil/empty - values, cycles and budgets, secrets, and detached results. - -Add focused regressions only where a consequential handoff risk lacks coverage. -Tests should continue to describe behavior as private functions and files move. -Use the existing everyday check and applicable ownership race checks during -implementation. Complexity measurements can inform review; acceptance does not -depend on numerical scores or a new benchmark campaign. - -Update the component overview and import internals to describe the implemented -ownership and file responsibilities when the refactor lands. Update contributor -navigation and test-owner references where moves affect them. Public behavior -continues to be defined by its current canonical owners. - -## Completion Criteria - -- Import callers pass cohesive named inputs, with clearly owned load-wide and - scope-local state and fewer redundant forwarding variants. -- Import acquisition, item assembly, and evidence projection are easy to locate - and follow across focused private files. -- Inheritance orchestration exposes value, policy, evidence, and accounting work - without requiring readers to reconstruct their interaction from one large body. -- Projection dispatch and substantial value-kind handlers have clear boundaries - and share the original operation state correctly. -- Existing APIs and contracted behavior, including completed remediation, remain - covered by the relevant behavioral and ownership checks. -- Internal documentation matches the resulting implementation. diff --git a/docs/roadmap/consolidation.md b/docs/roadmap/consolidation.md index 8241137..bafc93e 100644 --- a/docs/roadmap/consolidation.md +++ b/docs/roadmap/consolidation.md @@ -2,7 +2,8 @@ Status: proposed scope and target state for review. This document recommends refactors; it does not record implementation progress or authorize a release. -Prepare the staged implementation plan after reviewing this roadmap. +Prepare the staged implementation plan after reviewing this roadmap and settling +the [open questions](#open-questions). ## Purpose @@ -20,13 +21,22 @@ unused reverse inspection conversions. No public signature or configuration semantic change is currently recommended. This is an assessment of the reviewed consumer workflows, not a requirement to preserve every beta API in perpetuity. -## Evidence And Relationship To Existing Work +## Evidence And Baseline Reviewed worktree: `c9574ef`, immediately after the `v0.10.0` release at `5959537`. -The inputs are the earlier [complexity roadmap](complexity.md), whose reviewed -baseline predates discovery, and the new [complexity audit](remediation.md). -The older roadmap already acknowledges implemented discovery, but its selected -work is narrower than this proposal. +This document replaces the earlier `complexity.md` roadmap and `remediation.md` +audit record. Their recommendations and relevant evidence are incorporated here; +the original documents remain available in Git history: + +```sh +git show fbaaaf1:docs/roadmap/complexity.md +git show fbaaaf1:docs/roadmap/remediation.md +``` + +The earlier roadmap reviewed `7a83704f43ef9870db33661231cdff5d6ee502f7`, before +discovery. The audit reviewed `5959537d1adc2516e7f7e8881df72b707ec48c3f`, including +implemented discovery. The consolidation review checked their recommendations +against that newer baseline and also considered the public API. This review inspected the current public load/input/discovery declarations and maintained consumer examples, the shared request executor and acquisition flow, @@ -34,24 +44,65 @@ import parsing and selection, inheritance execution, reflective projection and cloning, schema field construction, and filesystem-context derivation. Graph caller traces and text-reference checks confirmed the unused reverse conversion chain. This was source review, not a new runtime correctness or performance audit. -The audit's complexity scores locate candidates; they are not standardized -acceptance thresholds or evidence of a runtime defect. +Neither this review nor the audit established a runtime correctness defect. -| Audit finding | Assessment and recommended treatment | -| --- | --- | -| 1. Import entry points and responsibilities | Main refactor. Include shared acquisition normalization, import-default ownership, declaration parsing, and directory admission. | -| 2. Shared inheritance executor | Main refactor. Expose meaningful phases and explicit combination results while retaining one executor. | -| 3. Projection and cloning | Main refactor for projection; bounded follow-up for substantial clone handlers, preserving the distinction between the operations. | -| 4. Schema field discovery and inline assembly | Include a focused refactor coordinated with projection. Keep one schema builder and its two descriptor views. | -| 5. Resolved filesystem context | Include alongside acquisition work. Reuse resolved facts without merging resource lifetimes or changing validation timing. | -| 6. Unused reverse inspection conversions | Include as an independent cleanup in the root facade and engine. Retain useful forward-conversion and detached-result coverage. | +### Audit Measurements -This is the combined proposal for scope review, rather than a second independent -implementation of the earlier roadmap. Keep the audit as evidence; its findings -are not closed by adopting this document. Completed correctness and algorithmic -remediations remain part of the baseline, including sequence records preserved -at `ea46001:docs/roadmap/remediation.md` and discovery records preserved at -`0369111:docs/roadmap/remediation.md`. +The audit refreshed the codebase-memory graph and used function size, cognitive +complexity, branch counts, and parameter counts to rank candidates for source +inspection. It indexed 901 non-test functions/methods and 928 test-file +declarations. Its fast index excludes examples, documentation, and scripts; a +tracked-file inventory covered those separately. Standalone `gocyclo`, `gocognit`, +`scc`, and `cloc` tools were unavailable. + +These are the graph tool's reported measurements at the audited revision. +Function spans include comments and blank lines. The scores support relative +triage; they are not standardized acceptance thresholds or evidence of a defect. + +| 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 | + +At that revision, the engine contained 88 production Go files and approximately +20,144 physical lines; the root facade contained 16 files and 1,861 lines. +`imports.go` was the largest production file at 1,195 lines. These measurements +locate concentrated responsibilities. They do not establish a need for more +packages or make the smaller document/YAML visitors and explicit facade +conversions maintenance defects. + +### Findings And Recommended Treatment + +The finding numbers below preserve the audit's identifiers. All six remain open +work; retiring their original documents does not mark them implemented. + +| Audit finding | Maintenance priority | Assessment and recommended treatment | +| --- | --- | --- | +| 1. Import entry points and responsibilities | Higher | Main refactor. Include shared acquisition normalization, import-default ownership, declaration parsing, and directory admission. | +| 2. Shared inheritance executor | Higher | Main refactor. Expose meaningful phases and explicit combination results while retaining one executor. | +| 3. Projection and cloning | Higher for projection; secondary for cloning | Main refactor for projection; conditional, bounded follow-up for substantial clone handlers, preserving the distinction between the operations. | +| 4. Schema field discovery and inline assembly | Medium | Include a focused refactor coordinated with projection. Keep one schema builder and its two descriptor views. | +| 5. Resolved filesystem context | Medium | Include alongside acquisition work. Reuse resolved facts without merging resource lifetimes or changing validation timing. | +| 6. Unused reverse inspection conversions | Small, independently reviewable cleanup | Include in the root facade and engine. Retain useful forward-conversion and detached-result coverage. | + +Findings 1–3 confirm the earlier roadmap's main opportunities; findings 4–6 add +focused work. The following sections own their rationale and intended changes. +Implementation and validation will establish each finding's disposition. + +Completed correctness and algorithmic remediations remain part of the baseline, +including sequence records preserved at `ea46001:docs/roadmap/remediation.md` +and discovery records preserved at `0369111:docs/roadmap/remediation.md`. +Item-scope application matching and inheritance policy localization are examples +of completed corrections that overlap this work and whose regressions should +remain protected. The [architecture policy](../policy/architecture.md), [configuration contract](../config.md), and exported declaration comments own @@ -140,6 +191,16 @@ processing, version rules, imports, placement, accounting, and contexts. Import entry points carry long positional lists through several forwarding variants. This repeats the representation of information already normalized for a load. +In [imports.go](../../internal/engine/imports.go), the +`loadImportedDocumentsTrackedWithVersion`, +`loadImportedDocumentsTrackedWithProcessing`, and +`loadImportedDocumentsTrackedWithProcessingDeferred` variants have corresponding +`loadImportedWithAccess...` variants. Both final signatures carry 13 parameters. +Placement, processing results, sequence policy, accounting, repetition tracking, +namespace identity, and deferred assembly must stay aligned through the chain. +The same file's `importTraversal.visitPath` combines source-admission decisions +in 174 lines, alongside acquisition, pending-item assembly, and evidence work. + Introduce cohesive private acquisition inputs and explicit state ownership, reusing `loadRequest`, `loadInput`, `acquisitionScope`, and `importTraversal` where they fit. Translate ordinary and layered public declarations into the @@ -165,8 +226,7 @@ The state boundaries are especially important: | One composition scope | Peer contributions, pending collection declarations, item-local helpers, and deferred item placement. Nested scopes do not become separate precedence tiers. | | Retained result | Source records, effective destinations, origins, history, and passive resource-context descriptions. | -The older roadmap's broad description of load-wide repetition state should not -be implemented as a global visited-file restriction. In the current code, +Keep repetition tracking tier-scoped. In the current code, `acquireUnderlayGroupScopes` creates a fresh visited map per group. Preserve valid reuse across tiers while keeping shared aggregate limits. Namespace equality, candidate spelling, and stable scheduling keys also serve different purposes. @@ -178,14 +238,15 @@ source IDs and source-local version processing in acquisition order, even when composition uses a different dependency-ready order. The layered main is acquired before underlays; lower-tier identity can still inform its later strict assembly. -Within import parsing, extract boolean/collection/placement field handling and -selector-value parsing where it clarifies responsibilities. Keep field presence, -authored error locations, selector applicability, and the order of consequential -validation visible. Within directory selection, separate entry admission and -descent decisions from traversal orchestration. Retain the existing root -deduplication, candidate union, exclusions, sorting, and active-path cycle rules. -No shared abstraction should conflate literal first-match main discovery with -multi-file import selection. +Within `parseImportDeclarationSpec`, extract boolean/collection/placement field +handling and selector-value parsing where it clarifies responsibilities. Keep +field presence, authored error locations, selector applicability, and the order +of consequential validation visible. Within `selectDirectoryImportCandidates` and +`importTraversal.visitPath`, separate admission and descent decisions from +traversal orchestration where they represent distinct work. Retain the existing +root deduplication, candidate union, exclusions, sorting, and active-path cycle +rules. No shared abstraction should conflate literal first-match main discovery +with multi-file import selection. ### 2. Share Resolved Filesystem Facts Without Sharing Live Handles @@ -193,6 +254,8 @@ multi-file import selection. derive overlapping root/base/namespace facts. The latter reconstructs context from combinations of source kind and optional fields, including absent input. Discovery and independent layered overlays make these combinations consequential. +Context construction is used by ordinary acquisition and both selected-overlay +paths, so those callers need to use the same applicable derivation rules. Use a small engine-owned resolved-context description for the facts actually needed by acquisition and passive views: namespace identity/kind, resolved root, @@ -255,8 +318,9 @@ Struct projection needs particular care: physical members drive traversal, flattened fields drive name lookup and sensitivity, and an inline container is not retained as an extra output node. Preserve declaration order, deterministic map order, nil/empty distinctions, collision diagnostics, codec failures, and -secret classification. Keep cycle entries active through child traversal and -balance their release on every exit. +secret classification. Scalar projection retains the existing number, duration, +text-codec, and secret-reference representations. Keep cycle entries active +through child traversal and balance their release on every exit. In `schemaBuilder.describeFields`, separate field discovery and tag parsing from inline struct assembly, inline map assembly, and ordinary field registration. @@ -265,9 +329,14 @@ Retain one builder/cache and its recursion detection. Make the paired updates to Keep index-path construction and inherited secret flags intact. These descriptor views are both needed; combining them into one collection would shift complexity into decoding and projection. +Keep pointer-chain and recursive-inline decisions explicit, preserve supported +field/tag forms and ambiguity paths, and retain cache/building-state behavior. -Follow with a bounded cleanup of substantial clone handlers where it makes their -ownership behavior easier to read. Keep cloning distinct from projection: +Provisionally treat clone cleanup as a conditional follow-up, as recommended in +[OQ-2](#oq-2-how-much-clone-cleanup-should-this-effort-commit-to). Extract substantial +handlers where they make ownership behavior easier to read after the projection +and schema work; completing this scope need not require clone edits if the +remaining flow is already clear. Keep cloning distinct from projection: cloning reconstructs typed values, preserves physical fields, round-trips text codecs, and distinguishes unresolved from already validated secret state. Projection builds a canonical document and charges output nodes. Extract common @@ -277,19 +346,25 @@ visitor solely because it has a high complexity score. ### 5. Remove Unused Inspection Conversions And Consolidate Real Adapters -Remove `toEngineSourceInfo`, `toEngineOrigin`, the exclusive reverse version and -import-selection converters, and the engine adapters used only by that chain. +Remove `toEngineSourceInfo`, `toEngineOrigin`, `toEngineSourceVersionCheck`, and +`toEngineImportSelections` from [conversion.go](../../conversion.go), along with +the engine adapters used only by that chain. At the reviewed revision, the three callers of `toEngineSourceInfo` are tests; `toEngineOrigin` has none. `SourceInfoWithPlacement`, -`SourceInfoWithImportSelections`, and their exclusive string-to-enum helpers -have no production role beyond that reverse chain. Recheck callers at the -implementation baseline before removal. +`SourceInfoWithImportSelections`, `importSelectorFromString`, and +`importSelectionOutcomeFromString` in [records.go](../../internal/engine/records.go) +have no production role beyond that reverse chain. Consumers receive inspection +records; production facade operations do not send those records back to the +engine. The unused inverse path adds another ownership mapping to maintain as +inspection evidence grows. Recheck callers at the implementation baseline before +removal. -Rewrite the affected record tests around real engine-to-public conversion and -detached public inspection. Preserve useful placement, version evidence, -import-selection, and copy-isolation assertions. Avoid manufacturing replacement -production setters solely to keep an old round-trip fixture working. Existing -facade loads already provide genuine sources and inspection records. +Rewrite the affected [record tests](../../records_test.go) around real +engine-to-public conversion and detached public inspection. Preserve useful +placement, version evidence, import-selection, and copy-isolation assertions. +Avoid manufacturing replacement production setters solely to keep an old +round-trip fixture working. Existing facade loads already provide genuine sources +and inspection records. Retain reverse conversions that serve real inputs: `toEngineDiagnostics`, for example, supports `WriteDiagnostics`. Consolidate repeated override/input @@ -301,9 +376,10 @@ the boundary rather than simplify it. ## Scope, Compatibility, And Policy Consequences The proposed work spans private engine code, private root-facade conversions, -affected tests, consumer guidance, and implementation documentation. This is a -deliberate expansion beyond the older engine-only roadmap. Preserve public -types/signatures and the behavior owned by the current contracts, including: +affected tests, consumer guidance, and implementation documentation. Under the +recommended answer to [OQ-1](#oq-1-should-the-public-loading-apis-change-in-this-effort), +preserve public types/signatures and the behavior owned by the current contracts, +including: - Acquisition versus composition ordering, strict scopes versus precedence tiers, identity ownership, placement, and source-version protection. @@ -339,8 +415,8 @@ or is required to claim readability improvements. This is dependency guidance for the later plan, not a commit-by-commit schedule: -1. Confirm the reviewed revision and public-surface direction; map the retained - behavior to existing tests and record concrete coverage gaps. +1. Confirm the implementation baseline and answers to the open questions; map + retained behavior to existing tests and record concrete coverage gaps. 2. Remove unused reverse inspection conversions and adjust their tests. This is small and independently reviewable. 3. Normalize acquisition inputs and separate import-default ownership. Develop @@ -349,7 +425,8 @@ This is dependency guidance for the later plan, not a commit-by-commit schedule: 4. Refactor inheritance around explicit preparation and combination results, retaining completed correctness and algorithmic regressions. 5. Refactor projection and schema field assembly in separate reviewable changes; - complete the bounded clone cleanup where it improves the remaining flow. + assess and, where useful, complete bounded clone cleanup under the scope + selected in OQ-2. 6. Consolidate real facade adapters, finish consumer/internal navigation, and run the applicable integration and ownership checks on the combined result. @@ -402,6 +479,47 @@ Completion requires reviewable evidence that: the resulting implementation. Current contracts continue to own behavior. Private type/helper names, final file splits, and exact stage boundaries belong -in the implementation plan. The public-surface recommendation and scope above -are ready for review; no additional feature semantics are needed to carry out -the recommended refactors. +in the implementation plan. The questions below concern the public direction and +scope of the work. No additional feature semantics are needed to carry out the +recommended refactors. + +## Open Questions + +The target above follows the recommended answers below, pending discussion. +Existing behavior and accepted architectural boundaries remain the baseline; +routine private design choices can be settled in the implementation plan. + +### OQ-1: Should The Public Loading APIs Change In This Effort? + +**Recommendation:** retain `Load`/`Options` and `LoadLayered`/`LayeredOptions`, +and consolidate their private inputs, conversions, and acquisition machinery. +The current simple call needs little setup, while layered loading makes input +groups and independent overlay context explicit. The maintained consumer examples +demonstrate useful roles for both. Better guidance and shared private machinery +address the observed duplication without adding public concepts. + +**Alternative:** introduce one public request model for main input, underlays, +and load-wide options, with a convenience entry for simple loading. This could +reduce repeated option declarations and make a consumer's move to layered loading +more uniform. It is viable before 1.0, but needs concrete simple and advanced +examples to show an improvement, plus a deliberate choice about ordinary versus +layered overlay-context behavior. It would expand this effort into public API +design and consumer migration. If selected, revise the public target before +preparing the internal implementation plan. + +### OQ-2: How Much Clone Cleanup Should This Effort Commit To? + +**Recommendation:** commit to projection and schema refactoring, then assess +clone handlers as a bounded follow-up. Include extractions that clarify typed +copy ownership, secret-state handling, or codec behavior; allow the review to +conclude that no clone changes are needed. Projection and schema have identified +mixed responsibilities, while part of cloning's size is ordinary type dispatch. +This keeps the effort focused on demonstrated maintenance benefit. Record the +assessment under audit finding 3 so conditional work is not silently omitted. + +**Alternative:** include a focused clone-handler refactor as committed scope +alongside projection and schema. Reviewing the related traversals together could +give maintainers more consistent places to find struct, map, and sequence handling. +The tradeoff is additional ownership-sensitive code movement when extraction may +not simplify the flow. Cloning and projection would still retain their distinct +semantics and operation state; a common visitor is not needed for either choice. diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md deleted file mode 100644 index 9b878c3..0000000 --- a/docs/roadmap/remediation.md +++ /dev/null @@ -1,255 +0,0 @@ -# Complexity Audit Findings - -Status: open recommendations for consideration. This is an audit record, not an -implementation plan or a new release gate. - -The [consolidation roadmap](consolidation.md) evaluates all six findings alongside -the earlier internal roadmap and the public API. It proposes one coordinated -scope; the findings remain open until implementation and validation establish -their disposition. - -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.