Files
ConfigKit/docs/roadmap/remediation.md

9.9 KiB
Raw Blame History

Consolidation Review Findings

Reviewed revision: 557e006a4ab98b666735a71b3845418c702f7cec. Comparison baseline: 8ab1568 (the committed implementation plan).

The review compared the consolidation target and implementation plan with the completed sprint, including public/private conversions, preflight and acquisition, import selection and evidence, filesystem contexts, inheritance, projection, schema construction, changed tests, and documentation. The development check (./scripts/check.sh) and full race suite (go test -race ./...) passed with Go 1.26.10 on Linux.

1. Complete The Non-Import Resolved-Context Handoff

Status: open. Priority: medium. Category: implementation gap and maintainability. No user-facing correctness regression has been established for this finding.

Target. Roadmap internal target 2 and implementation Stages 7–9 select one resolved filesystem description shared by validation/acquisition and passive context construction. Stage 8 explicitly requires populating the description at the existing non-import validation point and resolving only facts that are still missing later.

Evidence. In acquisition.go, acquireInput calls validateFilesystemContext at line 181 for inputs without import traversal and with an applicable explicit context. That validator returns only a diagnostic. Its host/native branches derive a resolved description but discard it. At lines 220–226, acquisition sees input.resolvedFilesystem == nil and calls resolveFilesystemContext again before building passive views.

For example, an ordinary Load of a byte or mapping source with explicit host FilesystemContext and imports disabled resolves/canonicalizes the base and root during validation, then derives those same facts again for its source context. The retained description therefore does not complete the intended validation-to-context handoff.

Additionally, filesystem.go still contains validateAbsentHostFilesystemContext (line 40), with its own root/base canonicalization, root-defaulting, and containment logic. That derivation duplicates resolveAbsentFilesystemContext in filesystem_context.go (line 29). The two paths already maintain separate containment-diagnostic messages. Graph caller inspection confirms that the old validator is reachable from acquireInput; it is not unused migration scaffolding.

Why address it. The new model exists, but one important path still discards and reconstructs its results, while absent-host context derivation has two production implementations. This leaves part of audit finding 5 unresolved and preserves the drift risk the consolidation was intended to remove. This is a structural completion issue, not a claim of measured performance loss or a new single-lookup guarantee.

Required revision.

  • Have non-import context resolution/validation return or populate the resolved description, and retain it on assembledInput at the existing validation point. Later passive-view construction and ordinary-overlay handling should reuse those applicable facts.
  • Route absent-host derivation through the common resolved-context owner and remove the duplicate derivation from the old validator. Keep the existing directory/access checks and close-error handling at their appropriate points.
  • Preserve deferred discovery/ordinary-overlay checks, root-only absent host context, host/native distinctions, diagnostics, and borrowed filesystem ownership. Independently required validation must not disappear merely to reduce filesystem calls.
  • Keep internal documentation aligned with the completed handoff. Describe aggregate accounting as load-wide and repetition/namespace state as tier-scoped; the new internal overview/import prose currently calls the latter per-tier accounting despite its distinct role.

Verification. Review the non-import call path to confirm that applicable resolved facts are carried forward and absent-host derivation has one owner. Reuse the existing host/native, discovery selected/absent, ordinary/layered overlay, and passive-resource-context tests. Add a focused behavioral regression only for a consequential gap exposed by the change; do not assert incidental canonicalization/stat call counts. Run the development check and applicable ownership race checks after remediation.

Algorithmic Audit Scope

Audited worktree: d898cb1, with production code at the reviewed implementation revision above. This follow-up used source inspection and graph caller traces, not benchmarks or new runtime measurements. It covered acquisition and import bookkeeping, scope scheduling and projection, filesystem context handoffs, inheritance preparation/combination/finalization, and reflective projection and schema construction.

Findings 2–3 are retained inefficiencies in paths carried through the refactor, not algorithmic regressions introduced by the consolidation. They warrant focused follow-up without changing the selected configuration model. The audit did not identify an additional correctness defect. Finding 1 remains open.

The selected stable Kahn scheduler, deterministic sorting, composition rules, scope boundaries, and required ownership copies are not reconsidered here. Previously deferred experiments in future.md and the clone review retain their separate disposition. These findings do not establish new public complexity guarantees, input restrictions, performance thresholds, or release gates.

2. Avoid Searching All Sources For Each Empty Import Selection

Status: open. Priority: low. Category: algorithmic efficiency.

Evidence. walkDeclarations calls recordEmptyImportSelection for each optional declaration that admits no source. In import_acquisition.go, lines 289–300, the latter finds the declaring source by scanning s.sources from the beginning, even though traversal already knows which source owns the declaration.

Complexity. With S acquired sources and D empty declarations, source lookup takes O(S × D) comparisons in the worst case, excluding path-copying and source selection itself. A broad main import of N constant-size fragments, each with one empty optional declaration, visits source positions 1 through N: the sum of lookup work is Θ(N²), despite linear source and selection-evidence volume and constant import depth. Existing input budgets bound the workload but do not remove this avoidable scaling within those budgets.

Potential refactoring. Carry the declaring source's integer index through the declaration traversal, or maintain a traversal-local source-ID-to-index map. Either approach makes each evidence-owner lookup O(1), with O(S + D) total bookkeeping for the map alternative. Keep source order and IDs unchanged. Use indices rather than retained pointers into the appendable source slice, and preserve optional-missing rollback and failed-source retention if maintaining an index across admission.

Verification. Reuse the source-local/detached empty-selection and partial failure coverage in import_evidence_test.go. A broad-fragment behavioral case is useful if existing coverage does not protect the chosen index handoff. Verify the removed scan by source review rather than adding timing thresholds or assertions about private lookup counts.

3. Restrict Import-Scope Edge Preparation To Relevant Nodes

Status: open. Priority: medium. Category: algorithmic efficiency and allocation scaling.

Evidence. scheduleAcquisitionScopes in imports.go, lines 57–64, recursively schedules the outer scope and every pending item-local scope using the same complete inclusion graph. scheduleContributionSlots in import_graph.go, lines 185–213, constructs a local identity-to-slot map but then scans every graph edge and allocates an edge slice with capacity len(graph.edges) for each scope with at least two eligible contributions. Most edges need not touch that scope.

Complexity. For Q such scopes and E inclusion edges, preparing the scheduler inputs takes O(Q × E) edge inspections and requests O(Q × E) edge-slice capacity over the operation. This is cumulative allocation demand, not a claim that all those slices remain live together. For N sibling item imports, each with one distinct mapping helper, Q and E are both Θ(N), making this preparation Θ(N²) even at constant nesting depth. The selected scheduler itself is not the problem: each small item-local graph has constant size, and the outer graph requires its normal deterministic scheduling work.

Potential refactoring. Build a traversal-local adjacency index once after acquisition, then gather edges for a scope by its participating source identities and retain only edges whose endpoints both have local slots. Alternatively, partition the existing edges by their applicable scopes once. Preserve the current endpoint-membership rule, item-boundary representation, slot placement, group scheduling, and stable Kahn tie-breaking. For the broad sibling family, this reduces edge preparation to O(E + P), where P is the total number of local slots, followed by the unchanged scheduler cost. Keep this an indexing change around the agreed scheduler, not a different ordering algorithm or a cross-load cache.

Verification. Reuse item/helper interleaving and grouped import scheduling coverage in imports_test.go and load_request_test.go, together with existing destination/provenance evidence tests. Cover broad independent item scopes if a consequential gap remains; verify index construction and edge visitation bounds by source review without new performance gates.