Preserve secret tags in dynamic configuration values

docs/roadmap/remediation.md

@@ -0,0 +1,242 @@
+# 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
+
+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
+
+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
+
+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
+
+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
+
+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
+
+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
+
+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
+
+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.