ConfigKit/docs/roadmap/remediation.md

# ConfigKit review findings

Reviewed `d3d468a..945e111` against the approved
[input-destinations roadmap](input-destinations.md),
input-destination implementation plan (preserved in Git history at
`315f464:docs/roadmap/implementation.md`), and
[ADR 0011](../adr/0011-input-destinations.md).

The root-owned destination fields, placement before strict composition,
transitive import placement, effective-path version ownership, final lifecycle,
source records, resource contexts, retained history, and existing disclosure
mechanisms broadly match the selected architecture. The required correction
below concerns an early diagnostic path, not loading or composition semantics.
No additional unintended strengthening of contracts was identified.

## 1. Rebase known Mapping-source failure paths to the input destination (P2)

Status: complete. Known Mapping value-builder paths are now prefixed with the
normalized destination at acquisition, preserving semantic key/index segments.
Public regressions cover mounted main and underlay failures, escaped keys,
sequence indices, cyclic values, unchanged root paths, and source-constructor
errors retaining source coordinates. The mounted value cases failed before the
fix and pass afterward. Existing directive/syntax checks remain unchanged.
Focused destination/source/diagnostic/import tests, `./scripts/check.sh`, and
`GOTOOLCHAIN=local GOPROXY=off go test -race ./...` passed on
`go1.26.8 linux/amd64`.

Affected: `assembleBaseInputTracked` in
[load_input.go](../../internal/engine/load_input.go), lines 27–32 at the reviewed
commit; the Mapping failure originates in `readSource` in
[source.go](../../internal/engine/source.go), lines 206–208.

`Mapping` snapshots its input through the existing value builder. When a nested
value is unsupported, cyclic, or exceeds a value-builder limit, the retained
issue already identifies a semantic path inside that application mapping.
`readSource` turns it into a structured diagnostic. The early return from
`assembleBaseInputTracked` annotates the source record with its destination but
returns those diagnostic paths unchanged.

For example, select this input as either main or an underlay:

```go
configkit.Input{
    Source: configkit.Mapping("bad mapping", map[string]any{
        "item": map[string]any{"bad": func() {}},
    }),
    Destination: configkit.Keys("catalog", "entries"),
}
```

The load correctly fails with `source.invalid`, but `Diagnostic.Path` is
`/item/bad` rather than `/catalog/entries/item/bad`. The source record correctly
reports `/catalog/entries`, so the two inspection records use inconsistent
application coordinates. A consumer displaying or associating the failure with
the final application shape would point to the wrong configuration field.

This diverges from implementation decision 10 and Stage 6: existing paths that
genuinely identify application content must be translated to effective paths
exactly once. A Mapping value-builder issue has that information; it is not a
source-read or YAML syntax failure for which a logical child path is unavailable.

Required remediation:

- Prefix known application-relative Mapping value-builder diagnostic paths with
  the normalized input destination at the acquisition boundary. Preserve key
  escaping and sequence-index intent in the existing semantic Path model.
- Keep source IDs, original primary/related locations, codes, value-free
  messages, source destinations, and nil-result behavior unchanged. Root
  placement must retain its current paths.
- Do not blanket-prefix parse/read failures, source-constructor errors, or
  structural import-directive diagnostics. Do not invent logical child paths
  for failures that do not identify application content.
- Add bounded regression coverage through the public API for mounted main and
  underlay Mapping failures, alongside unchanged root placement. Reuse existing
  syntax/directive tests to protect their different coordinate convention.
- Run the focused destination/source/diagnostic checks and the everyday check.
  Reconcile the completed plan's diagnostic evidence with this correction;
  do not redefine the agreed effective-path behavior to accept the gap.

## Review validation

- `GOTOOLCHAIN=local ./scripts/check.sh` passed at `945e111` on
  `go1.26.8 linux/amd64` (formatting, whitespace, tests, vet, and build).
- `GOTOOLCHAIN=local GOPROXY=off go test -race ./...` passed.
- `GOTOOLCHAIN=local GOPROXY=off go test -count=1 ./...` passed.
- An additional external-package probe in a disposable checkout outside the
  repository failed for both main and underlay placement, observing `/item/bad`
  instead of `/catalog/entries/item/bad` with `source.invalid`.
- No production fixes or regression tests were added to this repository during
  the review. No performance measurements or new guarantees were inferred.

## Relative import destination review (2026-10-14)

Reviewed `7248a8b..10edd95` against the
[relative-import roadmap](relative-import-destinations.md), completed
implementation plan (preserved in Git history at
`d9f08ff:docs/roadmap/implementation.md`), and
[ADR 0012](../adr/0012-relative-import-destinations.md).

The implementation matches the selected architecture: literal string segments
append to each immediate parent's destination; placement uses the existing
strict-composition and final-loading pipeline; source records retain distinct
effective destinations without changing physical resource contexts. No runtime
defect, missing feature behavior, or unintended strengthening of consumer
contracts was identified. The completed input-destination finding above remains
resolved. The documentation correction identified below is now resolved.

### 2. Restore ADR 0011's accepted decision text (P3)

Status: complete. Restored the accepted Decision paragraph verbatim from
`7248a8b`, leaving the extension link in ADR 0011's Status section and ADR 0012
unchanged. A comparison of everything from `## Date` onward is identical to the
baseline; the full ADR diff against `7248a8b` changes only Status. The uncached
local documentation-link check and `git diff --check` passed. The existing
`TestFacadeDestinationMappingFailurePaths` regression also passed, confirming
the earlier completed finding remains addressed. No runtime code, current
configuration/API documentation, or tests were changed.

Affected: [ADR 0011](../adr/0011-input-destinations.md), lines 34–42 at
`10edd95`. That commit rewrote its accepted Decision paragraph to incorporate
relative import destinations. Stage 1 of the implementation plan explicitly
required preserving that text, and the
[documentation policy](../policy/documentation.md#architecture-decision-records)
treats accepted decision content as immutable. Rewriting it obscures the original
decision and its subsequent extension, despite the accurate current behavior.

Required remediation:

- Restore that Decision paragraph verbatim from `7248a8b` (the same paragraph
  remains unchanged at `a8beb3e`). Keep the extension link in ADR 0011's Status
  section and retain ADR 0012 as the accepted extension record.
- Leave the implemented relative-import behavior and current configuration/API
  documentation unchanged. No additional ADR or runtime tests are needed.
- Confirm the ADR 0011 diff against `7248a8b` changes only its Status section;
  run the local documentation-link and whitespace checks.

### Relative import review validation

At `10edd95d4d9060bd46db51e0d6782c12dbda3120`, using
`go1.26.8 linux/amd64`, all of these passed:

- `GOTOOLCHAIN=local ./scripts/check.sh` (formatting, whitespace, tests,
  examples, links, package boundaries, vet, and build).
- `GOTOOLCHAIN=local GOPROXY=off go test -race ./...`.
- `GOTOOLCHAIN=local GOPROXY=off go test -count=1 ./...`.

The review changed only this findings document; it did not implement fixes.

## Shared source-version review (2026-10-15)

Reviewed `70c95bf..020d720` against the
[source-version roadmap](source-version-checks.md), completed
[implementation plan](implementation.md), and
[ADR 0013](../adr/0013-shared-source-version-checks.md).

The root-owned mode and evidence records delegate to private per-load checking
state. Main's local header is retained, accepted non-main headers are removed
before placement/composition, and the destination-prefixed main rule reaches
payload protection and typed conversion. Warnings, raw accounting, source
contexts, and final loading reuse the existing pipeline. Four must-address
issues remain. Findings 1 and 2 above remain completed historical findings.

### 3. Preserve distinct semantic shapes during evidence classification (P1)

Status: complete. Original evidence shapes are classified independently before
their results enter the existing pointer-based sensitivity/dependency closure.
Evidence no longer overwrites payload candidates. New engine and public
regressions failed before the fix and pass afterward, covering both acquisition
orders, schema tags, predicate observation of both shapes, and unchanged final
payload. Focused source-version/sensitivity/history tests, the everyday check,
and the full race suite passed on `go1.26.9 linux/amd64`. The implementation
plan records the additional Stage 9–12 regression evidence.

Affected: `classifySecretPathsWithEvidence` and `addSecretEvidencePath` in
[secret.go](../../internal/engine/secret.go), lines 78–104 at `020d720`.

Evidence candidates are stored in `map[string]Path`, keyed only by pointer text.
Adding a later candidate overwrites an earlier semantic Path with the same
pointer. A numeric mapping key and sequence index therefore compete for one
classification candidate. If the last shape is incompatible with a schema map,
`schemaSecretRoot` misses the secret field tag on the earlier mapping-shaped
header. Final source inspection can disclose its accepted text.

Reproduction through `LoadLayered`:

- Select required mode, `ParsePath("/meta/0/version")`, and a checker accepting
  every declaration.
- The final Go type has ordinary `Meta map[string]Header` and
  `Data.Meta map[string]SecretHeader`, with
  `SecretHeader.Version string` tagged `yaml:"version" configkit:"secret"`.
- Main is `meta: {"0": {version: main}}` plus `data: {meta: null}`.
- Mount two separate underlay tiers at `Keys("data")`. The first document is
  `meta: {"0": {version: KEY-HEADER-CANARY}}`; the second is
  `meta: [{version: INDEX-HEADER-CANARY}]`.

Both headers are accepted and consumed. Main's null replaces their remaining
payload, so final decoding succeeds. The mapping header's source record exposes
`Text: "KEY-HEADER-CANARY"` with `Redacted: false`, despite its schema tag.
Reversing the underlay order redacts that same header; the classification result
depends on which shape overwrites the candidate, not the established sensitivity
policy. Three repeated runs reproduced this difference. The canaries are
disposable fixture text, not real credentials.

Required remediation:

- Retain/evaluate all distinct original semantic candidate shapes before
  combining their classification results under the existing sensitivity policy.
  Preserve both evidence and payload candidates; replacing one shape with
  another must not erase applicable schema or predicate classification.
- Reuse historical/dependency sensitivity closure and conservative failure
  disclosure. Do not fix this by exposing less evidence, forbidding numeric
  keys/sequence headers, or requiring fragment conformance to the final Go type.
- Add a public regression covering both acquisition orders, the tagged consumed
  header, and unchanged final payload. Include a focused shape-collision test at
  the classification boundary and reuse existing neighboring-public-value tests.
- Run source-version/sensitivity/history tests, the everyday check, and the full
  race suite. Reconcile the plan's Stage 9–12 completion evidence.

### 4. Preserve import opt-in before stripping source directives (P2)

Status: open.

Affected: `assembleBaseInputTrackedForRole` in
[load_input.go](../../internal/engine/load_input.go), lines 54–87, and
`processSourceVersion` in
[source_version.go](../../internal/engine/source_version.go), at `020d720`.

Enabled source checking calls `splitImportDirectives` regardless of Imports.
For an input with imports disabled, it then checks `disabledDirectiveDiagnostics`
on the already stripped application payload. A valid root directive disappears
without either traversal or the required `import.disabled` error.

For example, load bytes containing:

```yaml
version: main
configkit:
  imports:
    - path: missing.yml
value: ok
```

Use a version rule at `Keys("version")`, an accepting checker, and leave Imports
false. Default mode correctly returns `import.disabled`. Both enabled modes
incorrectly return a successful configuration, silently omitting the requested
import. This changes import semantics even though the new mode selects only
version checking.

Required remediation:

- Enforce the existing directive opt-in against original source content before
  stripping/consumption can hide it. Cover both ordinary main and direct underlay
  acquisition through their shared boundary. Keep genuine enabled imports and
  selected-overlay restrictions unchanged.
- Add a focused public regression for enabled modes with Imports false and
  source-root imports. Keep default-mode behavior and safe source-local failure
  attribution; do not add a second implicit import-enabling mechanism.
- Run source-version/directive/import tests and the everyday check.

### 5. Retain the complete requested path for absent declaration evidence (P2)

Status: open.

Affected: `resolveSourceVersionPath` and `sourceVersionCheckForEvidence` in
[source_version.go](../../internal/engine/source_version.go), especially lines
74–90 at `020d720`; evidence classification uses the same partial-path fallback.

Lookup retains only the prefix reached before absence or obstruction, and that
prefix is still a valid Path. Public evidence therefore selects it instead of
the complete configured header path. For a rule at `Keys("meta", "version")`,
an optional underlay containing `meta: false` produces `Present: false` but
`VersionCheck.Path == "/meta"`, not `/meta/version`. Missing an early member or
an intermediate sequence index likewise discards the remaining suffix. The
record identifies an existing ancestor or partial path rather than the header
whose absence was checked.

Required remediation:

- Preserve the full requested local path when lookup stops early. Resolve the
  traversed prefix's key/index intent using actual source shape and retain
  unresolved suffix intent without guessing nonexistent container kinds.
- Keep nearest physical Location separate from the declaration Path. Reuse the
  complete path consistently in successful/callback/failure evidence and its
  classification candidates; do not change omission policy or invent authored
  coordinates below the obstruction.
- Add bounded regressions for a missing early member, scalar obstruction, and
  unavailable intermediate index, including successful optional and failed
  required loads. Reuse existing present-path and copy-ownership coverage.
- Run source-version/path/source-record tests and the everyday check.

### 6. Apply structural-overlap rejection only to an actual declaration (P2)

Status: open. This is an unintended strengthening of the agreed restriction.

Affected: `processSourceVersion` in
[source_version.go](../../internal/engine/source_version.go), lines 197–219 at
`020d720`.

The structural-overlap test runs before handling absence and examines the
partially resolved prefix. A valid scalar `extends` control obstructing a deeper
header lookup is treated as a present overlapping declaration, even though
lookup correctly classified the requested header as absent. The roadmap's
accepted overlap restriction concerns an actual header/control overlap; missing
headers remain governed by the selected mode.

Reproduction: choose if-present mode and
`Keys("entries", "child", "extends", "version")`, with a checker accepting
main absence. Mount main `{}` at `Keys("main")`. Mount a separate underlay at
`Keys("under")` containing:

```yaml
entries:
  base: {value: good}
  child: {extends: base}
```

The underlay has no declaration, so its checker should be skipped and its valid
inheritance should remain available. Its inheritance paths do not intersect
main's destination-prefixed protected path. Instead, the load stops with
`version.error` reporting a structural overlap. This rejects an otherwise valid
optional omission, not a present value serving two structural roles.
The same fixture succeeds in default mode.

Required remediation:

- Base declaration-overlap rejection on actual header presence. Continue
  validating malformed source controls independently, and preserve remaining
  payload/inheritance ownership checks after omission handling.
- Keep optional non-main absence permissible and required absence a missing
  declaration failure. Do not widen the accepted restriction into a ban on all
  configured paths beneath directive spellings.
- Add a focused load-level regression with separate mounted scopes as above,
  plus missing-required behavior. Reuse present-overlap and malformed-control
  tests to ensure genuine conflicts remain rejected.
- Run source-version/directive/inheritance tests and the everyday check.

### Shared source-version review validation

At `020d72084d5988e958c46381724e5dad78cada3d`, using
`go1.26.9 linux/amd64`, these existing checks passed:

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

Four additional external-package regression probes reproduced findings 3–6 in
an isolated temporary module outside this repository, with a local replace to
the reviewed checkout. The full probe command was
`GOTOOLCHAIN=local GOPROXY=off GOWORK=off go test -mod=mod -count=1 -v ./...`;
the classification and absent-control cases also reproduced with `-count=3`.
The module was located at `/tmp/configkit-source-version-review.L7YGaM` during
this review; the descriptions above preserve the cases independently of that
temporary directory.

No production fixes, repository regression tests, dependency updates, or release
changes were made. Only this findings document was revised.