Record discovery algorithm audit findings
This commit is contained in:
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user