ConfigKit/docs/roadmap/remediation.md

# Implementation Review Remediation

## Status and scope

Status: complete. All nine findings have been addressed. Review baseline: `f42ecc70c330a399aa6dada87658694402a515ec`.
This review covers the initial implementation against the
[architecture policy](../policy/architecture.md), [configuration contract](../config.md),
and [implementation plan](implementation.md). The package boundaries and principal
lifecycle were in place at review time; the original findings below are retained
with completion evidence. No production code was changed during the original
review; subsequent remediation is recorded here. All findings below were reproduced through the public API or local
validation tools, with the additional affected call sites identified by inspection.

Priorities: P1 means high-impact confidentiality, isolation, or availability;
P2 means a correctness or validation gap that also requires remediation.
Each item below records its completed correction and validation evidence.
Keep current contracts and implementation evidence accurate when fixing these issues;
do not change the agreed semantics simply to accommodate the implementation.

## 1. P1 — Honor secret tags inside supported dynamic concrete values

Status: complete. Typed projections now retain concrete field sensitivity; final,
baseline and pre-resolution projections feed historical/dependency classification.
Dynamic nested, replaced, deleted and introduced-value regressions pass. A final
audit additionally verified that validators cannot change dynamic field sensitivity
through equal-text type replacement; the added regression failed before the fix
and now passes. Full tests
pass with the temporary canonical-TMPDIR workaround for issue 8; vet, build and
whitespace checks pass.

Evidence: [secret.go](../../secret.go), `schemaSecretRoot` stops at
`descriptorDynamic`; [projection.go](../../projection.go), `dynamicDescriptor`
accepts concrete structs and describes their fields, but their sensitivity is not
carried into the classification pass.

Reproduction: define `Sensitive` with a string field
`Token` tagged `yaml:"token" configkit:"secret"`. Return
`Config{Payload: Sensitive{Token: "review-canary"}}` from defaults, where `Payload`
is `any`, and load with `NoSource()`. Loading succeeds. `WriteEffective` prints
`payload.token` in plaintext, and `Explain(Keys("payload", "token"))` returns that
same text with `Redacted=false`. This does not involve a typed `Secret` reference;
it breaks the separate field-tag classification contract for accepted Go values.

Required work: preserve sensitivity from concrete dynamic descriptors through
projection and historical classification, including values subsequently replaced
or deleted and dependency propagation. Respect existing supported value shapes;
do not silently ignore tags or require callers to duplicate them in a predicate.

Acceptance: canary tests for dynamic structs, nested containers, defaults and
recorded resolution must prove redaction in effective output and final/historical
explanation. Include replacement/deletion and derived dependencies. Typed secret
references must remain prohibited inside dynamic values.

## 2. P1 — Reject nonregular source files before a blocking open

Status: complete. Selected files, rooted imports and profiles now inspect canonical
targets before opening and retain post-open checks. Deadline-controlled FIFO
subprocess tests cover all three paths. Full tests pass with the issue-8 TMPDIR
workaround; vet, build and whitespace checks pass.

Evidence: [source.go](../../source.go), `readSource`, calls `os.Open` before
checking regular-file status. [imports.go](../../imports.go), `readImport`, and
[profile.go](../../profile.go), `readProfileFile`, likewise open before checking.

Reproduction: create a POSIX FIFO with no writer and call
`Load[map[string]any](File(fifoPath))`. The child process remained blocked until
terminated after two seconds; no nonregular-file diagnostic was returned. Byte
limits do not help because the open itself blocks.

Required work: inspect canonical targets before opening and reject nonregular
files while retaining containment and post-open checks. Apply the protection to
selected files, imports, and profiles. This fits the existing stable-filesystem
contract and does not require introducing a cancellation API or promising hostile
filesystem race protection. Explicit `Reader` inputs retain their stream behavior.

Acceptance: subprocess-based FIFO regressions on supported POSIX platforms must
finish promptly with safe nonregular-file diagnostics for all three paths. Keep
regular-file, permitted symlink, containment, and permission tests passing.

## 3. P1 — Detach mutable values introduced by resolution before publication

Status: complete. Added post-resolution and publication ownership copies; the latter
preserves already validated secret state. Regressions cover nested maps/slices,
pointers, direct/helper assignments, validator aliases, and one lookup per load.
Full tests pass with the issue-8 TMPDIR workaround; vet, build and whitespace pass.

Evidence: [resolution.go](../../resolution.go), `Default`/`Normalize`, directly
assign supplied values; `runResolution` projects them but does not clone the typed
object. [load.go](../../load.go) publishes that same object in `Result.Config`.

Reproduction: a reusable resolver normalizes a map field to one captured map and
records the assignment through `Normalize`. Perform two sequential `NoSource`
loads with this definition. Changing the first result's map changes the second
result's map. Both loads succeed, while the second result's frozen inspection
still shows the earlier value. There is no concurrent callback use or retained
callback context in this reproduction.

Required work: establish detached typed ownership after application resolution,
covering direct recorded assignments as well as helpers. Preserve resolved-secret
semantics and avoid extra environment reads; cloning only defaults is insufficient.
Ensure validators do not reintroduce equivalent-valued mutable aliases that bypass
the projection-based mutation check and escape into published results.

Acceptance: repeated loads with shared resolver-provided maps, slices, pointers,
and nested values remain independent of each other and captured application state.
Keep final projection, secret lookup, and validation ordering consistent.

## 4. P2 — Reconcile history replay with final-only default application

Status: complete. Explanation now records an explicit canonical decoding transition
when replay differs from final typed input, preserving decoded origins and showing
default restoration. Null/scalar-to-map profile and CLI regressions pass at parent
and descendant paths. Full tests pass with the issue-8 TMPDIR workaround; vet,
build and whitespace checks pass.

Evidence: [explain.go](../../explain.go), `Explain` initializes replay from defaults
and merges each input into it. Actual decoding applies defaults to the final
composed/overlaid input, so intermediate replacements are not lasting default resets.

Reproduction: defaults contain `m: {a: 1}`, base YAML contains `m: null`, and a
selected profile contains `m: {b: 2}`. The successful typed result correctly contains
both `a: 1` and `b: 2`. Explanation of `/m` ends its history at only `b: 2`.
Explanation of `/m/a` reports its removal by the base and never explains its return,
even though its effective value is `1` with a default origin.

Required work: reconstruct the input-layer history without conflating it with
incremental typed-default application. Explicitly account for effective default
retention/reappearance and canonical decoding so history and final inspection agree.
Preserve visibility of superseded policy-valid inputs, even when they were not
valid for the final destination type. Do not change decoding to retain reset markers.

Acceptance: cover null-to-map and scalar-to-map overlays above defaulted containers,
including CLI overlays and descendant queries. The historical account must explain
the effective value and origin without implying that a final-present path is absent.

## 5. P2 — Replay newly created resolution paths

Status: complete. Replay inserts new mapping entries/subtrees and coalesces
overlapping parent/child net operations while retaining reasons and dependencies.
Leaf/container regressions cover nested creation, removal and overlapping records.
Full tests pass with the issue-8 TMPDIR workaround; vet, build and whitespace pass.

Evidence: [explain.go](../../explain.go), `replaceHistoryPath`, returns immediately
when a mapping entry is absent, even when the replacement is a newly created value.

Reproduction: load `m: {}` into a string-keyed integer map. In the resolver, set
`m["new"] = 7` and record `/m/new` with `ChangeDerivedDefault`. Loading succeeds.
`Explain(Keys("m", "new"))` reports effective YAML `7`, but the recorded resolution
change has `After.Present=false` and empty YAML. Parent explanation also misses
that insertion during replay.

Required work: apply recorded additions as well as replacements and deletions,
including creation of necessary mapping ancestors when justified by the audited
change. Handle overlapping parent/child records without inventing assignment order
or duplicating effects; the contract records net domain changes.

Acceptance: explain added map entries and nested additions at both leaf and parent
paths, alongside deletion/replacement cases. After-values and final snapshots must
agree while preserving reasons, dependencies, origins and redaction.

## 6. P2 — Preserve semantic path kinds and reject nonexistent explanation paths

Status: complete. Metadata retains value-free effective/historical path shapes;
queries validate explicit segment kinds, and explanation omits absent-to-absent
operations for paths that never existed. Regressions cover wrong-kind and parsed
numeric queries, out-of-range indices, no-output failures and removed descendants.
Full tests pass with the issue-8 TMPDIR workaround; vet, build and whitespace pass.

Evidence: [metadata.go](../../metadata.go) indexes queries solely by `Path.String`,
losing explicit key/index distinctions. [explain.go](../../explain.go),
`applicationOperationPath`, considers an atomic ancestor relevant even when the
requested descendant never existed; an absent-to-absent event then makes `Explain`
report success.

Reproduction: load `items: [1]`, `m: {"0": 2}`, and `x: 3` into `map[string]any`.
`Metadata.Lookup(Keys("items", "0"))` and `Metadata.Lookup(Keys("m").Index(0))`
both succeed despite the incorrect segment kinds. Their explanation behavior
also disagrees with metadata. `Explain(Keys("x", "missing"))` returns true and
`WriteExplanation` succeeds, although this path never existed.

Required work: resolve query segment kinds against effective and historical
container shapes before looking up textual indexes. Distinguish a genuinely
removed path from an impossible descendant of an atomic value. Share traversal
semantics across metadata and explanation while allowing parsed numeric tokens
to resolve contextually.

Acceptance: explicit key/index queries reject incompatible containers; parsed
`/items/0` and `/m/0` still resolve correctly. Never-existing scalar descendants
return false/nil, and their writer returns an error without output. Historically
existing descendants remain explainable after parent replacement.

## 7. P2 — Reject unsupported inline pointer-to-map schemas safely

Status: complete. Schema construction rejects inline map pointers, including
multiple pointer levels, and detects nonterminating inline pointer chains. Public
regressions verify safe errors before defaults run; supported inline coverage stays
green. Full tests pass with the issue-8 TMPDIR workaround; vet, build and whitespace
checks pass.

Evidence: [schema.go](../../schema.go), `describeFields`, dereferences inline field
types before choosing the map branch, then stores a map descriptor for a field
whose actual reflected value is a pointer. [projection.go](../../projection.go)
subsequently applies map operations to that pointer.

Reproduction: a configuration field declared
`Extra *map[string]int` with `yaml:",inline"`, initialized to a non-nil map in
defaults, panics in `Load` with `reflect: call of reflect.Value.Len on ptr to
non-array Value`. This is an internal reflection panic, not an application-hook panic.

Required work: reject pointer-to-map inline declarations during schema construction
with a safe schema diagnostic. The agreed inline shapes are structs,
pointers-to-structs, and string-keyed maps; do not broaden them accidentally.
Audit nearby descriptor/value-kind assumptions for the same failure class.

Acceptance: nil and non-nil inline map pointers fail consistently without invoking
default factories or panicking. Supported inline maps and inline struct pointers
continue to load, project, clone and classify correctly.

## 8. P2 — Fix the baseline test's canonical-path assumption on macOS

Status: complete. The source test compares canonical path identity and separately
checks the declared label. Full default and race suites pass on macOS/arm64 using
both the default TMPDIR and an intentionally symlinked TMPDIR. Vet, build and
whitespace checks also pass; no production canonicalization behavior changed.

Evidence: [source_test.go](../../source_test.go),
`TestFileSourcesReopenAndClassifyFailures`, compares `SourceInfo.Path` directly
with the path under `t.TempDir()`. Successful sources intentionally report a
symlink-resolved canonical path.

Reproduction: both `GOPROXY=off go test ./...` and its `-race` variant fail at this
assertion on the review host, where the temporary path traverses macOS's `/var`
symlink. Both suites pass with `TMPDIR=/private/tmp`. Source contents are correct;
the test expectation is wrong.

Required work: derive the expected canonical path through the filesystem contract,
and separately verify the declared source label. Do not remove canonicalization
from production code or require developers to override `TMPDIR`.

Acceptance: the ordinary documented test and race commands pass with the host's
default temporary directory and with an intentionally symlinked temporary directory.

## 9. P2 — Make the release checker's Bash prerequisite executable on macOS

Status: complete. Replaced mapfile with a Bash-3.2-compatible null-delimited array
read loop. Shell orchestration tests cover spaces/newlines and fail-fast behavior.
The complete checker passes under macOS system Bash 3.2 with cached Go 1.25.5 and
1.26.0, including both baselines, race tests, hygiene/docs/examples and all four
production cross-builds. New native Linux execution remains a release gate, not a
claim made by this local check.

Evidence: [scripts/release-check.sh](../../scripts/release-check.sh) uses
`mapfile -d ''`, but [the release procedure](../release.md) requires only Bash.
The macOS system Bash is 3.2 and has no `mapfile`; the null-delimiter option also
requires a sufficiently recent newer Bash.

Reproduction: `/bin/bash -c "mapfile -d '' xs </dev/null"` fails with
`mapfile: command not found` on the supported macOS review host. The complete checker
was not run with two configured toolchains; this isolates its shell incompatibility.

Required work: preferably use a Bash-3.2-compatible null-delimited read loop, keeping
filenames safe. Alternatively require Bash 4.4+ explicitly, validate it before
performing other work, and document the executable used to invoke the checker.
Do not imply that the stock macOS shell satisfies undocumented requirements.

Acceptance: exercise the checker on supported native platforms with the documented
shell and cached Go prerequisites. Confirm filename handling remains null-delimited
and failures still stop the checker.

## Review validation and limits

Reviewed on macOS/arm64 with Go 1.25.5. The worktree was clean at review start.

- `GOPROXY=off go vet ./...` and `GOPROXY=off go build ./...` passed.
- Default `GOPROXY=off go test ./...` and `go test -race ./...` failed at finding 8;
  no additional suite failures or race reports were observed.
- With `TMPDIR=/private/tmp`, both complete suites passed. This is diagnostic
  evidence, not the proposed fix for the normal baseline command.
- Temporary external Go programs reproduced findings 1–7 using public APIs.
  The FIFO reproduction used a subprocess deadline and was terminated safely.
  Reproduction values were disposable canaries, not actual credentials.
- The system-shell probe reproduced finding 9. No release publication, full
  two-toolchain release check, new Linux execution, or extended fuzz campaign was
  performed during this review.

The upstream YAML adapter, node-based composition, direct typed binding, fixed
precedence, separate explicit bootstrap, and application-owned domain lifecycle
remain appropriate. Repair these boundary and consistency defects within that
architecture. Re-run focused regressions, the normal baseline and race suites,
local documentation checks, and applicable release gates before closing this roadmap.

## Remediation completion

All nine findings are corrected and committed in individual units of work, with an
additional issue-1 audit correction for validator sensitivity mutation. The normal
macOS baseline no longer requires a TMPDIR workaround. The historical review and
intermediate validation notes above retain the conditions that applied at the time.

The complete source checker is validated locally with:

```sh
CONFIGKIT_BASELINE_TOOLCHAIN=go1.25.5 CONFIGKIT_NEWER_TOOLCHAIN=go1.26.0 /bin/bash scripts/release-check.sh
```

This uses the available cached toolchains, runs both native baselines and the race
suite, checks module hygiene, formatting, documentation and example boundaries,
and cross-builds Linux/macOS on amd64/arm64. Native execution during remediation
was macOS/arm64; new native Linux evidence and selection of current supported patch
releases remain maintainer release gates. No release, tag, push or downstream
application migration was performed. Retire this completed roadmap during release
preparation according to the documentation policy.