266 lines
15 KiB
Markdown
266 lines
15 KiB
Markdown
# Configuration Discovery Review Findings
|
||
|
||
Status: findings 1–2 complete; findings 3–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: complete.
|
||
|
||
**Completed:** terminal probe failures retain the candidate descriptor and gain
|
||
normal file-source evidence at the acquisition boundary. Regression tests compare
|
||
ordinary and later-underlay failures with explicit-file errors, including source
|
||
IDs, placement, layers, and skipped missing candidates. A controlled permission
|
||
probe checks the diagnostic-to-file link. The load regression failed before the
|
||
fix; focused discovery/failure tests and `./scripts/check.sh` passed on Linux.
|
||
|
||
**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.
|