ConfigKit/docs/roadmap/remediation.md

# Configuration Discovery Review Findings

Status: finding 1 complete; findings 2–5 remain open.

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: complete.

**Completed:** search prevalidation now defers absence-only root requirements
until selection. Ordinary, layered-main, and underlay regression cases compare
searched files with explicit files under an empty context and retain errors for
absent inputs. The regression failed before the fix. Focused context/absence
tests and `./scripts/check.sh` passed with Go 1.26.10 on Linux.

**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.

## 4. Use Platform-Aware Windows Component Validation

Priority: P3. Status: open. Algorithmic audit: correctness finding.

**Location:** [discovery.go](../../internal/engine/discovery.go),
`validWindowsFilenameComponent`, lines 266–281.

The hand-maintained device-name switch omits `CONIN$` and `CONOUT$`. Both are
Windows console device names, not ordinary filename components. The helper
accepts them, so on Windows a slug or custom filename using one can pass static
validation. That conflicts with the selected usable-host-component rule and can
defer an invalid description until acquisition or hide it behind an earlier
successful candidate.

**Evidence:** an external Go test overlay calling the current private helper
failed for both names: `validWindowsFilenameComponent` returned true. The Go
1.26.10 Windows implementation of `filepath.IsLocal` recognizes both as reserved,
and its own tests include both spellings. The overlay ran on Linux and exercised
the helper directly; it was not a native Windows load test. No repository test
file was added.

**Complexity and refactoring opportunity:** the current predicate takes O(L)
time for a component of length L, with O(L) temporary storage for case conversion.
No asymptotic optimization is needed. Prefer the standard library's
platform-aware component/local-path checks within the Windows branch, retaining
the selected single-component and invalid-character checks as needed. This
avoids maintaining an incomplete device table. Go's Windows reserved-name logic
also accounts for OS-dependent extension behavior; do not replace it with a
broader blanket naming prohibition.

**Verification:** add appropriate Windows-specific cases for console names and
ordinary names, including invalid later components behind an earlier match.
Keep those tests consistent with the host Go helpers rather than imposing their
Windows behavior on Unix. This is a correction to existing platform-specific
code, not a new Windows support promise or native-platform release gate.

## 5. Preserve Drive-Root-Relative Windows Path Meaning

Priority: P3. Status: open. Algorithmic audit: correctness finding.

**Location:** [discovery.go](../../internal/engine/discovery.go),
`absoluteCandidate`, lines 380–391; also used for literal directory searches.

On Windows, a path such as `\config.yml` has no volume name and is not absolute
according to `filepath.IsAbs`, but it is rooted at the current drive's root.
The helper treats it as an ordinary relative path and joins it to the whole
working directory. With a working directory of `C:\work`, discovery therefore
probes `C:\work\config.yml` instead of `C:\config.yml`. The same issue affects
slash-rooted Windows spellings and custom directory searches.

This can select a different file from explicit `File` loading, which uses
`filepath.Abs`. The roadmap requires literal host paths consistent with explicit
file loading; it does not select an alternate interpretation of drive-root paths.

**Evidence:** static tracing against the selected Go 1.26.10 Windows `IsAbs`,
`VolumeName`, `Join`, and `Abs` implementations establishes the branch and result.
The first two checks route this path into `Join`, which appends it beneath the
working directory; Windows `Abs` delegates to the native full-path operation.
A native Windows execution was not performed or required.

**Complexity and refactoring opportunity:** both current normalization and the
correct standard-library route are linear in the path spelling length, apart
from OS helper work. This is a correctness refactoring, not a scaling problem.
Route Windows drive-root-relative paths through `filepath.Abs`, as the helper
already does for volume-qualified relative paths, or use a narrow private
absolute-path helper that delegates to Go in production. Preserve lazy base
resolution and the ordinary-relative-path test seam. No new public API or
hand-written Windows path parser is needed.

**Verification:** on Windows, compare discovery's candidate interpretation with
`filepath.Abs` and explicit `File` for ordinary relative, drive-root-relative,
drive-relative, and absolute paths. Cover both `Paths` and a literal
`SearchDirectory`, and retain the existing Unix literal-path tests. Do not add a
new native Windows validation requirement to the release procedure.

## Algorithmic Audit Scope And Conclusions

The additional audit reviewed new production code from `424c897` through
`e50fee2`; the current code at `68cf5c4` is unchanged from that implementation.
It preserved the expressly planned flat snapshots, ordered first-match search,
directory-first expansion, whole-description static validation, and reuse of
ordinary file acquisition. It did not benchmark or reopen those choices.

- Let S be the description data processed by an operation, including entries,
  filename groups, custom names, retained issues, and supplied option lists.
  Snapshot/concatenation construction is O(S) per construction operation with
  O(S) owned storage.
  The nested clone loops visit each stored group/name once; they are not repeated
  scans of the same list for each candidate. Repeated immutable prefix rebuilding
  by a caller follows the explicitly planned flat-snapshot representation and
  was not treated as a new algorithm finding.
- Validation is O(S) plus the existing canonical diagnostic ordering cost,
  O(E log E) comparisons for E errors. Validation occurs a fixed number of times
  per consumed description, not once per candidate. The audit found no new
  repeated sorting or quadratic validation pass.
- Iteration advances persistent entry/group/name indexes. Across the search it
  visits each reached group and candidate a constant number of times, with no
  restart of earlier candidate lists. Traversal is O(S + C) in descriptor and
  candidate counts, plus generated path bytes and ordinary filesystem work,
  with O(1) iterator state beyond the description and current path storage.
  Descriptions are not expanded into a complete candidate product in memory.
- Each attempt performs a bounded number of standard path/probe operations;
  selection stops on success or a terminal error. Selected-file acquisition and
  the final lifecycle run once. The added absent-context handling checks a fixed
  number of directories and does not introduce scans over prior configuration
  contributions.

No unplanned quadratic algorithm or other actionable asymptotic regression was
identified. Findings 4 and 5 record the additional ordinary correctness issues
found while examining those algorithms; findings 1–3 remain open from the earlier
review. Existing configuration-tree algorithms outside the discovery changes
were not re-audited as new feature work.

## 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.
- Findings 1–3 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.
- The algorithmic audit used source-level complexity analysis and the temporary
  helper regression overlay described in finding 4. It collected no timing or
  allocation measurements. Findings 4 and 5 are platform-specific correctness
  findings, not claims of a native Windows validation run.