Record configuration discovery review findings

docs/roadmap/remediation.md

@@ -0,0 +1,135 @@
+# Configuration Discovery Review Findings
+
+Status: three open findings. Implementation has not been changed by this review.
+
+Reviewed commit: `e50fee2`, including the discovery implementation commits after
+`424c897`. The review compared the implementation with the
+[discovery roadmap](discovery.md), [implementation plan](implementation.md),
+[ADR 0019](../adr/0019-explicit-configuration-discovery.md), and the existing
+configuration and architecture contracts.
+
+## 1. Defer Absence-Only Context Requirements Until The Search Outcome Is Known
+
+Priority: P2. Status: open.
+
+**Location:** [filesystem.go](../../internal/engine/filesystem.go), lines 77–123,
+and the outcome-specific validation in
+[load_input.go](../../internal/engine/load_input.go), lines 103–122.
+
+The `sourceSearch` branch calls `absentFilesystemOptionDiagnostics` before
+selection. That helper requires a nonempty host `Root`, even though a selected
+host file can derive both its base and default root from its own path.
+
+**Confirmed reproduction:** with an existing file containing `value: selected`
+and `Options[Config]{Filesystem: &FilesystemContext{}}`, ordinary `File(path)`
+loading succeeds. Replacing only the source with
+`SearchConfig("", Paths(path))` fails with `options.invalid` and the message
+"absent-input filesystem root and base directories must be absolute host paths".
+No candidate is acquired. The same source distinction affects layered inputs
+because their validation uses the same helper.
+
+This applies an absent-input requirement to a selected-file workflow and
+unintentionally strengthens its contract. The roadmap requires selected files
+to retain normal file-context behavior; the plan explicitly separates static
+syntax checks from outcome-dependent compatibility checks.
+
+**Required correction:** validate only outcome-independent context syntax before
+discovery. Once the outcome is known, apply normal host-file rules to selection
+and absent-input rules to exhaustion. Preserve the required explicit root for an
+absent input with a supplied host context, selected-file rejection of `BaseDir`
+or native `FS`, containment checks, and terminal context failures.
+
+**Verification:** add ordinary and layered selected-file equivalence cases with
+an explicit empty context. Also verify that optional exhaustion with that same
+context still fails, and that the existing root-only selected/absent overlay
+cases continue to pass. Do not change `File` to reject its currently valid context
+or require callers to omit an otherwise valid options value.
+
+## 2. Preserve The Failing Candidate's Identity In Access Diagnostics
+
+Priority: P2. Status: open.
+
+**Location:** [discovery.go](../../internal/engine/discovery.go), lines 517–528,
+and the early resolver-error return in
+[load_input.go](../../internal/engine/load_input.go), lines 105–109.
+
+When symlink resolution or stat fails for a reached candidate with an error other
+than absence, the resolver returns a diagnostic with an empty source ID and
+discards the candidate. The acquisition boundary returns no source evidence.
+Consequently neither `LoadError.Sources()` nor the diagnostic identifies which
+candidate failed. The permission-probe test in
+[discovery_test.go](../../internal/engine/discovery_test.go) also asserts a nil
+primary location, preserving this omission rather than the intended normal
+source diagnostics.
+
+**Confirmed reproduction:** create a regular `app.yml`, then search paths
+`app.yml/child.yml` followed by the valid `app.yml`. The first path fails with
+`ENOTDIR`. Explicit `File(badPath)` returns `source.read` with a primary source
+location and a file record identifying `badPath`. The equivalent search correctly
+stops, but returns `source.read` with no primary location and an empty source
+list. An operator cannot locate the problem from the structured error.
+
+The roadmap calls for normal candidate access diagnostics, and the plan requires
+normal candidate records for file failures. The decision to omit a public attempt
+trace does not require discarding the one failing file's identity.
+
+**Required correction:** retain the failing candidate and expose its normal
+file-source diagnostic identity at the acquisition boundary, with the appropriate
+source ID, safe path/label, and input placement/layer. Keep missing attempts
+absent from records and budgets. Required exhaustion and directory-helper errors
+still have no invented file location. Preserve terminal access errors and avoid
+adding a public attempt history, raw OS error strings, or YAML values.
+
+**Verification:** cover a deterministic real candidate-resolution failure and a
+controlled permission/stat failure. Check the diagnostic's location against the
+returned file record, including an underlay after earlier acquired sources.
+Assert that no later candidate is loaded and that skipped missing candidates
+still produce no records. Revise the existing nil-primary assertion accordingly.
+
+## 3. Isolate User-Configuration Preset Tests Using The Platform's Go Convention
+
+Priority: P2. Status: open.
+
+**Location:** [discovery_load_test.go](../../internal/engine/discovery_load_test.go),
+`TestSearchSourceObservesUserConfigEnvironmentChanges`, lines 231–279.
+
+The new test treats every non-Windows platform as using `XDG_CONFIG_HOME`, writes
+both fixtures beneath that variable, and leaves the actual home environment
+unchanged. On macOS, `os.UserConfigDir` uses
+`$HOME/Library/Application Support` and ignores `XDG_CONFIG_HOME`. The first load
+therefore searches the real user's configuration directory instead of the
+fixture, and the environment change cannot select the second fixture. This makes
+the test fail on an ordinary macOS checkout and potentially depend on local files.
+
+This is confirmed by inspection of the selected Go 1.26.10 standard library's
+`os.UserConfigDir` implementation and the test. A native macOS run was not
+performed or required. Production preset resolution correctly delegates to Go;
+the defect is in the test setup.
+
+**Required correction:** control the environment consumed by the platform's Go
+helper, then obtain fixture bases through `os.UserConfigDir` rather than assuming
+the XDG layout. On macOS, use a temporary home and its derived configuration
+directory; retain appropriate isolation on Linux and Windows. Keep environment
+changes restored and the test nonparallel. Use the existing private helper seam
+for portable directory-resolution failure tests rather than assuming that a
+relative environment value fails on every platform.
+
+**Verification:** the environment-change test must observe both controlled
+fixtures and avoid the real home/config directory. Review the macOS branch
+against Go's helper behavior. Keep macOS best-effort support and the existing
+release procedure; do not introduce native macOS validation as a gate.
+
+## Review Validation
+
+- `./scripts/check.sh` passed with Go 1.26.10 on Linux: formatting/whitespace,
+  all tests, vet, and build.
+- `go test -race ./...` passed on the same toolchain.
+- An external temporary Go program reproduced findings 1 and 2 through the
+  public API, comparing explicit file loading with discovery. No production or
+  test files were modified for these reproductions.
+- The three findings above remain open despite the existing suites passing.
+  The broader discovery architecture is implemented: immutable root-owned
+  descriptions, incremental ordered selection during shared source acquisition,
+  concrete selected-source propagation, and shared absent-input handling before
+  the existing composition, inheritance, decoding, resolution, and publication
+  lifecycle. No additional release gates or new composition semantics were found.