From 68cf5c489bf37ec32fc08060f231fc7fca509ff6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 24 Oct 2026 17:52:20 +0000 Subject: [PATCH] Record configuration discovery review findings --- docs/roadmap/remediation.md | 135 ++++++++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..cf8331d --- /dev/null +++ b/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.