From 91769e9f9bded88561551e132bfb4a5691f39cb9 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 27 Oct 2026 02:52:47 +0000 Subject: [PATCH] Record interface remediation review findings --- docs/roadmap/remediation.md | 277 ++++++++++++++++++++++++++++++++++++ 1 file changed, 277 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..4dea83e --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,277 @@ +# Interface Remediation Review Findings + +Reviewed commit: `98c8b7c8423a816acdfceace36165366b13719ee`. +Comparison baseline: `0dd16aa8a935645ae2847d32edd86461dc4618f0` +(the planning commit, before the implementation sprint). + +Scope: the completed [implementation plan](implementation.md) and +[interface remediation roadmap](interface-remediation.md). This report records +remaining defects and newly introduced scalability problems; it does not change +the selected architecture or public contracts. All six findings below are open. + +## 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 three 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: + +```sh +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. + +## 1. Final Dynamic Field Tags Can Be Lost From Redaction + +Priority: P1. Status: open. + +Relevant code: `resolveLoadInput` in +[load_result.go](../../internal/engine/load_result.go), lines 56–73; +`freezeMetadataHistory` in [metadata.go](../../internal/engine/metadata.go), +lines 146–155; and `sensitivityCandidates` in +[history_select.go](../../internal/engine/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: + +```go +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](../config.md#secret-classification-and-effective-rendering): +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: open. + +Relevant code: `observeSourceEvidence` in +[history_source.go](../../internal/engine/history_source.go), lines 82–117; +and previous-origin construction in `selectHistory` in +[history_select.go](../../internal/engine/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: open. + +Relevant code: `historySelectionForApplication` in +[explain.go](../../internal/engine/explain.go), lines 183–207; and descendant +selection in [history_select.go](../../internal/engine/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: + +```yaml +a: base +group: {x: X, y: Y} +``` + +Select an overlay containing: + +```yaml +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: open. + +Relevant code: `observeTypedCheckpoint` in +[history_cli.go](../../internal/engine/history_cli.go), lines 38–49; +`sourceContributions` in [history_source.go](../../internal/engine/history_source.go), +lines 185–218; and the lookup implementation in +[document_walk.go](../../internal/engine/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. + +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. 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: open. + +Relevant code: `addObservation` in +[history_model.go](../../internal/engine/history_model.go), lines 116–139; +replacement observation construction in +[history_source.go](../../internal/engine/history_source.go), lines 101–116; +`sourceOriginRepresentedByOperation` in that file, lines 245–263; and candidate +collection in [history_select.go](../../internal/engine/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: open. + +Relevant code: `sensitivityCandidates` and `appendDistinctHistoryPath` in +[history_select.go](../../internal/engine/history_select.go), especially +lines 207–234; and `sameConcreteHistoryPath` in +[history_model.go](../../internal/engine/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.