# Implementation Review Remediation ## Status and scope Status: in progress. 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 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](../../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