Prepare the ConfigKit v0.10.0 release
This commit is contained in:
@@ -1,298 +0,0 @@
|
||||
# Configuration Discovery Review Findings
|
||||
|
||||
Status: complete. All five findings have been addressed in separate commits.
|
||||
|
||||
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: complete.
|
||||
|
||||
**Completed:** the environment-change test selects the environment variable used
|
||||
by Go on Windows, macOS/iOS, Plan 9, or other Unix hosts, and obtains both fixture
|
||||
bases through `os.UserConfigDir`. It remains nonparallel and restores the
|
||||
environment. The redundant relative-XDG failure assumption was removed; existing
|
||||
private-helper tests retain portable failure/short-circuit coverage. Focused
|
||||
tests and `./scripts/check.sh` passed on Linux. The macOS branch was checked
|
||||
against Go's helper implementation; no native macOS gate was added.
|
||||
|
||||
**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: complete. Algorithmic audit: correctness finding.
|
||||
|
||||
**Completed:** Windows validation now delegates reserved-name decisions to
|
||||
`filepath.IsLocal` while preserving the selected component/character checks. The
|
||||
manual device table was removed. Tests cover console names, platform-dependent
|
||||
extension behavior, and invalid later search components; Unix tests retain their
|
||||
own filename rules. Focused tests and `./scripts/check.sh` passed on Linux. The
|
||||
Windows engine test binary cross-compiled successfully; it was not run natively.
|
||||
|
||||
**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: complete. Algorithmic audit: correctness finding.
|
||||
|
||||
**Completed:** drive-root-relative Windows paths now use `filepath.Abs` alongside
|
||||
drive-relative paths, preserving native host interpretation. Windows regression
|
||||
tests compare candidates and loaded files with explicit `File` for relative,
|
||||
drive-root-relative (both separators), drive-relative, and absolute paths, through
|
||||
both `Paths` and `SearchDirectory`. Existing Unix path/short-circuit tests and
|
||||
`./scripts/check.sh` passed. The Windows engine test binary cross-compiled; no
|
||||
native Windows execution or new release gate was introduced.
|
||||
|
||||
**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 were carried forward 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 remained open at the original review despite the 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.
|
||||
|
||||
## Completed Remediation Validation
|
||||
|
||||
All five fixes include focused regression coverage and per-issue completion
|
||||
markers above. The final `./scripts/check.sh` and `go test -race ./...` passed
|
||||
with Go 1.26.10 on Linux. The Windows engine tests also cross-compiled with
|
||||
`GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go test -c`, with the binary kept outside
|
||||
the repository. No native Windows or macOS run was performed or made a gate.
|
||||
The implementation preserves the public API, candidate ordering, optional
|
||||
absence, and existing composition/version/ownership contracts.
|
||||
Reference in New Issue
Block a user