configkit/docs/roadmap/remediation.md

Implementation Remediation Review Findings

Current follow-up: the public API cleanup review has one open finding (9). The earlier interface-remediation review and completed findings 1–8 below are retained as historical evidence. The current review's scope and validation are recorded with finding 9.

Reviewed commit: 98c8b7c8423a816acdfceace36165366b13719ee. Algorithmic follow-up: 91769e9 (unchanged production code). Comparison baseline: 0dd16aa8a935645ae2847d32edd86461dc4618f0 (the planning commit, before the implementation sprint).

Scope: the completed implementation plan at 07e4ae5:docs/roadmap/implementation.md and interface remediation roadmap. This report records remaining defects and newly introduced scalability problems; it does not change the selected architecture or public contracts. All eight findings are complete; implementation and validation evidence is recorded per finding.

Architecture And Review Evidence

The public root-owned facade still delegates to the synchronous engine. A per-load history builder now normalizes baseline, source, inheritance, typed checkpoint, CLI, and resolution evidence. Metadata and inspection share its frozen model, with separate public projections and detached query results. The resolution callback retains a separate fixed entry-state view. Retained-root admission remains before effective secret lookup and application validation. The old independent metadata history and published inheritance copies are gone.

The original presence/explicitness corrections and composition claimant-order correction are present. The shared-history migration is substantial, but the findings below prevent treating its behavioral preservation and indexed-query targets as fully satisfied. No additional public restriction, guarantee, or prohibition requiring reversal was identified in the reviewed changes. In particular, private builder lifecycle guards are not new consumer obligations.

Review combined code/plan inspection, graph-based producer/consumer tracing, existing tests, and disposable public-API probes run against both revisions. The first three findings were reproduced; the remaining five follow from the new loops and index contents, without performance measurements. Probe programs and the baseline checkout were kept outside the repository. Production code and repository tests were not changed during the review.

The following checks passed with Go 1.26.10 on Linux, using the installed toolchain and cached dependencies:

GOTOOLCHAIN=local GOTELEMETRY=off GOPROXY=off go test ./... -count=1
GOTOOLCHAIN=local GOTELEMETRY=off GOPROXY=off go test -race ./...
GOTOOLCHAIN=local GOTELEMETRY=off GOPROXY=off ./scripts/check.sh
git diff --check

The passing suite does not detect the reproduced regressions. Suggested coverage below targets those gaps rather than adding a general test matrix or new gates.

The algorithmic follow-up checked normalization, lookup/index construction, selection, replay dispatch, sensitivity preparation, and retained-root traversal. It expanded finding 4 to the inheritance/resolution adapters and added findings 7–8. The existing focused engine history, explanation, inheritance, and secret tests passed with go test ./internal/engine -run 'History|Explain|Inheritance|Secret' -count=1. No benchmark data was collected. The selected shared model, ordered-ID sorting, net resolution grouping, scratch-state normalization, whole-tree explanation replay, and fixed-point sensitivity closure are not being reconsidered. The deferred contributor-trace and branch-local replay experiments remain separate.

1. Final Dynamic Field Tags Can Be Lost From Redaction

Priority: P1. Status: complete.

Completion: final inventory/sensitivity is registered unconditionally before freezing. TestFinalDynamicTagRedactsEqualValuedResolution failed before the fix and passes afterward, protecting effective and historical public rendering. Validation: the full ordinary suite and git diff --check passed.

Relevant code: resolveLoadInput in load_result.go, lines 56–73; freezeMetadataHistory in metadata.go, lines 146–155; and sensitivityCandidates in history_select.go, lines 201–253.

Resolution evidence is observed only when resolution produces snapshots. The final projection is assigned to model.final but never unconditionally observed for path/sensitivity facts. Classification now consumes model observations, instead of also traversing the final projection as the baseline did.

Reproduction through the public API:

type classified struct {
    Token string `yaml:"token" configkit:"secret"`
}
type configuration struct {
    Value any `yaml:"value"`
}

result, err := configkit.Load[configuration](
    configkit.Bytes("input", []byte("value: {token: AUDIT-CANARY}\n")),
    configkit.Options[configuration]{
        Definition: configkit.Definition[configuration]{
            Resolve: func(c *configuration, _ *configkit.Resolution) error {
                c.Value = classified{Token: "AUDIT-CANARY"}
                return nil
            },
        },
    },
)

The map-to-struct assignment leaves canonical values unchanged, so the existing net-change audit accepts the load without a recorded normalization. The final projection carries the concrete struct's secret tag, but no snapshot adds that classification to the model. WriteEffective and Explain(Keys("value", "token")) expose the canary, including historical explanation values. At the baseline they redact it and report Redacted: true.

Expected behavior follows the existing secret-classification contract: concrete dynamic tags and final/historical classification remain effective. The plan also expressly requires observing the final projection. This concerns ordinary tagged configuration, not evidence of resolved credential bytes being stored in history.

Remediation: register final concrete path and sensitivity facts before freezing and classification, independently of whether resolution recorded a value change. Preserve pointer-conservative historical masking and dependency closure. Do not require extra resolver records or reject equal-valued dynamic type changes as a workaround. Add a public lifecycle regression for this case that checks effective and historical redaction.

2. Intermediate Source Definitions Are Duplicated In Metadata History

Priority: P2. Status: complete.

Completion: displaced definitions are omitted only when a preceding operation already introduced that concrete path with the same represented origin. TestTierHistoryRecordsEachContributionOnce failed before the fix and passes for both differing and equal-valued tiers. The full ordinary suite and git diff --check passed, including the existing shape-change regressions.

Relevant code: observeSourceEvidence in history_source.go, lines 82–117; and previous-origin construction in selectHistory in history_select.go, lines 108–117.

Each overlay record unconditionally registers its displaced definitions. A definition already represented by the preceding replacement operation is therefore emitted again when the next tier displaces it. Selection chains Previous through both records. The baseline's appendInputOriginEvent suppressed an already represented origin; the new normalization does not apply that rule to displaced definitions.

Reproduction: load map[string]any with two ordered underlays containing x: low and x: mid, and main input containing x: high. Query Metadata().History(Keys("x")).

• Expected and baseline: three contributions, low → mid → high.
• Actual: four contributions, low → mid → mid → high. The second mid record names the same source location as its own previous origin.

This is not a genuine additional equal-valued write. It is one source contribution registered twice, contrary to the plan's explicit preservation of origin containment and duplicate suppression.

Remediation: normalize displaced definitions against already represented semantic operations in the shared model. Preserve genuine independent writes, including equal values, and keep key/index shapes distinct. Add a three-tier history regression checking source chronology and previous-origin links.

3. Container Explanations Take The Last Descendant Replacement's Origin

Priority: P2. Status: complete.

Completion: explanation uses the model's direct query-scoped contribution when available and reserves operation fallback for paths without that contribution. TestOverlayExplanationUsesQueryScopedOrigin failed before the fix and now checks root, enclosing mapping, and leaf locations. The full ordinary suite and git diff --check passed, including removed/replaced descendant coverage.

Relevant code: historySelectionForApplication in explain.go, lines 183–207; and descendant selection in history_select.go.

The explanation helper obtains a direct application selection, then overwrites it with every selected replacement linked to that application. For a container query, multiple child replacements can be relevant; the last one supplies the origin for the entire application event, regardless of the query's scope.

Reproduction: main YAML is:

a: base
group: {x: X, y: Y}

Select an overlay containing:

a: overlay
group: {x: O}

The overlay event in Explain(Keys("group")) is attributed to the O scalar (line 2, column 12), instead of the contributing mapping (baseline: line 2, column 8). The root explanation also attributes the whole overlay event to that scalar, despite the independent change to a (baseline: document root, line 1, column 1). Values are correct; the contribution location is not.

The roadmap requires retaining source attribution and using common semantic selection, while allowing different record grouping between public projections. It does not authorize replacing enclosing attribution with an arbitrary leaf.

Remediation: resolve application attribution at the requested semantic scope in the common selection/attribution path. Prefer a valid direct contribution for an enclosing query; retain replacement fallback where it is needed for removed or replaced descendants. Do not restore a second independent explanation attribution system. Cover enclosing root/mapping and precise leaf attribution in one focused overlay regression.

4. History Construction Repeatedly Performs Unindexed Mapping Searches

Priority: P2. Status: complete.

Completion: checkpoints, inheritance sides, and resolution before/final states use one immutable document lookup per stable state, rather than scanning broad mappings for every path. Lookups are rebuilt after scratch replay changes. Removed the unused summary contribution list and its searches; the existing test still verifies composite metadata and individual explanation attribution. The full ordinary suite and git diff --check passed, including collection, inheritance, resolution shape, and typed-checkpoint cases.

Relevant code: observeTypedCheckpoint in history_cli.go, lines 38–49; sourceContributions in history_source.go, lines 185–218; observeInheritanceTransfer and documentAtInheritancePath in history_inheritance.go, lines 87–141 and 211–217; historyConcretePathAt and its callers in history_resolution.go; and the lookup implementation in document_walk.go.

For every checkpoint path, construction calls documentAtPath on both trees. That helper supplies no lookup index, so each mapping step linearly searches its entries. For a flat mapping with n fields, locating all n children requires Theta(n²) entry comparisons. Both checkpoints run even on ordinary loads with no CLI overrides. Source-summary construction introduces the same pattern: it calls sourceContributions for surviving paths, and that helper searches each raw application again without an index.

The new inheritance adapter similarly enumerates every before/parent/effective path and then calls documentAtInheritancePath for each one. That helper falls back to the same unindexed search: one inheritance transfer of a broad n-field mapping incurs Theta(n²) sibling comparisons. Resolution normalization calls historyConcretePathAt before and after for each record; its hand-written mapping scan costs O(rn) for r records under an n-field mapping, or Theta(n²) when all n fields are independently recorded. These are the same lookup problem, not separate findings about the selected inheritance or resolution algorithms.

These are new repeated sibling scans, not the deliberately retained whole-tree explanation replay. Existing document lookup machinery or paired traversal can avoid them. Existing sorting and genuinely repeated producer evidence still have their own costs; no universal linear-time guarantee is implied.

Remediation: build/reuse phase-local lookups or collect corresponding nodes during shared traversal instead of independently searching a broad mapping for every child or record. Respect scratch-tree mutation boundaries: do not reuse a stale lookup after replacing or editing its indexed state. Concrete resolution paths must still reflect the corresponding before/after containers. Review the summary membership work as well: the new historySourcePayload.contributions list is populated and tested but currently has no production reader. Preserve any membership the selected design needs, but avoid computing unused data through repeated searches. Retain behavior coverage; do not add timing thresholds or implementation-specific scan counts.

5. Prefix Indexing Multiplies One Operation By All Its Descendants

Priority: P2. Status: complete.

Completion: indexes locate concrete affected paths rather than placing every descendant at the reported ancestor. Exact path buckets drive source-origin containment (with early success); enclosing queries visit only their affected subtree. Logical operations remain payload references, deduplicated once by the common selector. Thus a leaf no longer scans sibling facts of an ancestor replacement. The full ordinary suite and git diff --check passed, including parsed key/index sides, removed descendants, inheritance, and enclosing queries.

Relevant code: addObservation in history_model.go, lines 116–139; replacement observation construction in history_source.go, lines 101–116; sourceOriginRepresentedByOperation in that file, lines 245–263; and candidate collection in history_select.go, lines 35–53. CLI and resolution effects use the same operation index.

An ancestor replacement registers before/after facts for every affected descendant, indexing every distinct observation ID at the same reported operation pointer. The prefix index consequently holds O(n) entries for one logical replacement of an n-node subtree, not one indexed operation plus shape-specific affected-path lookup.

During source normalization, each surviving descendant calls sourceOriginRepresentedByOperation. It visits all those ancestor entries and re-resolves the same operation attribution, even after finding a match. One large subtree replacement therefore creates Theta(n²) visits. Later, a single leaf History query gathers and sorts all n entries at its enclosing operation before rejecting unrelated affected paths. Querying n such leaves performs O(n² log n) candidate work despite each leaf's short returned history.

Remediation: distinguish logical operation indexing from exact affected-path and before/after-shape indexing. Visit operation attribution once, then locate the query's relevant shape facts rather than expanding every sibling fact. Preserve parsed-query deduplication, removed descendants, ancestor replacement effects, and enclosing queries. This should implement the selected localized candidate-query design, not replace the existing prefix mechanism wholesale or coarsen semantic identity.

6. Sensitivity Candidate Deduplication Cannot Match Parsed Paths

Priority: P3. Status: complete.

Completion: candidate and dependency deduplication uses structural segment equality, including parsed tokens, without conflating explicit keys and indexes. TestSensitivityCandidatesMergeRepeatedParsedPaths failed before the fix and now verifies unique semantic/dependency candidates and merged sensitivity facts. The full ordinary suite and git diff --check passed.

Relevant code: sensitivityCandidates and appendDistinctHistoryPath in history_select.go, especially lines 207–234; and sameConcreteHistoryPath in history_model.go, lines 271–294.

The candidate collector adds a parsed version of every observation's pointer, then uses a concrete-only comparator to deduplicate candidates. That comparator returns false for a parsed segment, including comparison of a non-root parsed path with itself. Repeated observations of one path therefore append repeated parsed candidates instead of merging them. Parsed resolution dependencies have the same problem in appendDistinctHistoryPath.

For k observations of one non-root pointer, the per-pointer candidate list grows with k and insertion repeatedly scans it: Theta(k²) comparisons and unnecessary retained temporary paths/dependency entries. The outer classifier later uses structural equality to suppress duplicate predicate candidates; this finding does not establish repeated public predicate calls or another redaction leak.

Remediation: use structural path equality that handles parsed segments for candidate/dependency deduplication, while retaining concrete-only comparison where concrete inventory requires it. Preserve distinct parsed, explicit-key, and explicit-index candidates until classification is unioned by pointer. Extend focused candidate/dependency coverage for repeated parsed paths without asserting timing or promising a new public call-count contract.

7. Explanation Dispatch Repeatedly Searches All Selections And Payloads

Priority: P2. Status: complete.

Completion: a query-local payload-reference index and application-fallback map are built in one pass over ordered selections. Source/CLI, inheritance, and resolution replay use direct lookups while retaining model chronology and the query-scoped attribution correction. The tier-history regression now also checks explanation contribution order/origins. The full ordinary suite and git diff --check passed; no replay algorithm or shared-state lifetime changed.

Relevant code: Inspection.Explain, historySelectionForPayload, historySelectionForApplication, and historySelectionForResolutionGroup in explain.go, especially lines 64–71, 106–113, and 174–232; and appendInheritanceExplanation in inheritance_explain.go.

After the model has selected relevant evidence, each source/CLI application linearly searches the selection list, scans the entire phase payload list to find linked replacement records, and searches selections again for each linked record. Inheritance steps and resolution group members also repeatedly search the same selection slice. Payload references already provide stable, comparable keys; these are lookup operations, not semantic comparisons.

For A applications, P phase payloads, S selections, and R linked replacements, source/CLI dispatch alone costs O(A(P+S) + RS). With m successive underlay tiers each setting the same scalar in a constant-size configuration, A, P, and S all grow with m: this introduces Theta(m²) dispatch work even though the existing value replay and rendered history for that query have O(m) size/work. It is independent of finding 5's candidate expansion and remains after that index is repaired. The inheritance/resolution lookup loops have the corresponding operation-count times selection-count cost.

Remediation: index the ordered selections by historyPayloadRef once per query and index or group replacement-to-application links once, rather than rediscover them for every application. Reuse that lookup for inheritance substeps and original resolution record IDs. Dispatch can then be linear in the selected records and traversed relationships, excluding the deliberately selected sorting, replay, cloning, and rendering costs. Keep replay chronology unchanged and resolve the attribution precedence correctly with finding 3 rather than preserving its last-descendant bug. Use query-local or eagerly frozen indexes; no lazy mutation of published history is needed. Existing explanation owners should protect output equivalence without a timing assertion.

8. Sensitivity Preparation Expands One Dependency List Per Affected Path

Priority: P2. Status: complete.

Completion: ordinary dependency lists are expanded once per recorded target/ attribution, and dependency candidates once per attribution, without descendant dependency-list copies. Classification supplies already computed inheritance links to candidate preparation, avoiding a second ordinary-edge construction. TestResolutionSensitivityKeepsLogicalEdgesWithoutSubtreeDuplication failed before the fix and now verifies the original edges, including a coalesced child record, and sensitivity isolation from unrelated paths. The full ordinary suite, full race suite, everyday check, and git diff --check passed. The original public redaction/history probes also now produce the expected results.

Relevant code: sensitivityCandidates, lines 238–247, and sensitivityDependencies, lines 272–286, in history_select.go; observeResolutionEvidence and observeResolutionSubtree in history_resolution.go; and classifySecretPathsWithEvidence in secret.go.

One original resolution record owns one dependency list, and all its before/ after subtree observations share its attribution reference and reported target. Nevertheless, sensitivityDependencies resolves and clones that attribution and emits its full dependency list for every observation. For a single recorded normalization of an n-node subtree with d distinct dependencies, this creates Theta(nd) ordinary edges although there are only d logical target/dependency pairs. With n and d growing together, edge materialization alone is quadratic in the O(n+d) underlying evidence. Those duplicates also become separate triggers in the unchanged sensitivity-closure implementation.

The candidate collector repeats the attribution dependency loop for every affected path and retains a dependency list on each concrete candidate, although its consumer uses those lists to register dependency paths, not distinct descendant-target edges. It also calls sensitivityDependencies a second time for inheritance links, rebuilding and discarding the ordinary edge list. These costs remain even if finding 6's parsed-path comparator is corrected.

Remediation: enumerate ordinary dependencies once per logical recorded target/ attribution, or directly from the original resolution payload, and register each required dependency candidate without broadcasting its list to all descendant observations. A single record's edge preparation should require O(n+d) traversal and at most O(d) ordinary edges, apart from path/annotation sizes and unrelated evidence. Avoid rebuilding ordinary edges merely to obtain inheritance links. Preserve original records omitted by explanation grouping, concrete/parsed predicate candidates, dependency order where observable, and existing pointer-conservative propagation. The fixed-point algorithm and inheritance link semantics need not change. Reuse transitive/cyclic sensitivity regressions and add focused coverage for a subtree record with several dependencies; no performance threshold or new public dependency restriction is warranted.

9. Source Lookup Test Compares Canonical Paths With Symlinked Fixture Paths

Priority: P2. Status: open. This is a test portability defect, not a production source-lookup defect.

Reviewed revision: 69bd9ead53b0bd1d4b325de2f357b566291a4803. Comparison baseline: 556b10b2cd32cbe3240ced79f72bfcc2fe1ebc35. Scope: the completed API cleanup plan and accepted roadmap, including facade conversions, recording intents, source lookup, observable string migrations, maintained examples, and current documentation. Existing findings 1–8 were not reopened.

Relevant code: TestResultSourceLookup in source_lookup_test.go, lines 23–26 and 62–74.

The test identifies imported and underlay sources by comparing SourceInfo.Path with the original paths returned by its fixture writer. Host source paths are canonicalized by ConfigKit, but t.TempDir() can return a path containing a symlink when the temporary-directory environment names one. The same files then have different lexical path spellings. Both records are present with the correct layers and version evidence, but neither comparison matches, so the test fails with "imported or underlay source evidence missing".

This is reproducible on Linux with a symlinked TMPDIR; it does not depend on access to macOS or establish a new platform-validation gate. The failure-path lookup test in the same file already uses filepath.EvalSymlinks for its fixture path and does not have this particular comparison defect.

Reproduction from the repository root, with disposable files outside the checkout:

probeRoot=$(mktemp -d /tmp/configkit-source-review.XXXXXX)
mkdir "$probeRoot/real"
ln -s "$probeRoot/real" "$probeRoot/link"
TMPDIR="$probeRoot/link" GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off \
  go test . -run '^TestResultSourceLookup$' -count=1

Expected: the focused test passes for either spelling of a valid temporary directory, retaining its source-evidence assertions. Actual: it fails at line 74 with canonical records under the real directory and expected paths under its symlink. The same focused test passes with TMPDIR set to the real directory. This false failure can prevent ordinary development/release checks from completing in an otherwise valid environment; it does not indicate lost source evidence.

Remediation: canonicalize the test's fixture directory before creating its files, or resolve the imported/underlay expected paths with filepath.EvalSymlinks, as the failed-load case already does. Preserve record attribution and detachment assertions. Do not change production canonical-path behavior, forbid symlinked temporary directories, or add a platform/CI gate. Verify the existing focused test with both real and symlinked temporary roots after correction.

Other review results: the root inventory contains the expected 370 exported declarations/members, with exactly the accepted name/signature/value changes against the prior 372-entry catalog. The two fixed recording methods and the existing assignment helpers are the only addRecord callers. Source lookup clones retained records using existing ownership/sanitization machinery. No other required architectural gap, production regression, or inadvertently strengthened public contract was identified in this sprint.

Validation passed on Linux with Go 1.26.10 and cached dependencies:

GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off ./scripts/check.sh
GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off go test -race ./...
GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off \
  go test . -run 'SourceLookup|ResolutionRecordingIntents|TestPublicFacadeBoundary|TestDependencyImportBoundary|TestLocalDocumentationLinks' -count=1

The additional symlink-root invocation above failed as described. Production code and repository tests were not changed during this review.