base-layer/configkit
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Record interface remediation review findings

Browse Source
This commit is contained in:
Eric Rakestraw
2026-10-27 02:52:47 +00:00
parent 98c8b7c842
commit 91769e9f9b
1 changed files with 277 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

277
docs/roadmap/remediation.md Normal file
View File

@@ -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.