ConfigKit/docs/roadmap/remediation.md

Implementation Review Remediation

Status and scope

Status: in progress. Review baseline: f42ecc70c330a399aa6dada87658694402a515ec. This review covers the initial implementation against the architecture policy, configuration contract, and implementation plan. The package boundaries and principal lifecycle are in place, but the findings below must be addressed before claiming that the initial target is fully satisfied. No production code was changed during this review. 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 remains open until its regression checks and the relevant baseline pass. 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. Full tests pass with the temporary canonical-TMPDIR workaround for issue 8; vet, build and whitespace checks pass.

Evidence: secret.go, schemaSecretRoot stops at descriptorDynamic; 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, readSource, calls os.Open before checking regular-file status. imports.go, readImport, and 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

Evidence: resolution.go, Default/Normalize, directly assign supplied values; runResolution projects them but does not clone the typed object. 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

Evidence: 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

Evidence: 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

Evidence: metadata.go indexes queries solely by Path.String, losing explicit key/index distinctions. 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

Evidence: 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 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

Evidence: 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

Evidence: scripts/release-check.sh uses mapfile -d '', but the release procedure 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.