diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index cf8331d..8350c8e 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Configuration Discovery Review Findings -Status: three open findings. Implementation has not been changed by this review. +Status: five open findings. Implementation has not been changed by these reviews. Reviewed commit: `e50fee2`, including the discovery implementation commits after `424c897`. The review compared the implementation with the @@ -119,6 +119,119 @@ 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, @@ -127,9 +240,13 @@ release procedure; do not introduce native macOS validation as a gate. - 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. +- 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.