From 5d5d51ab117a41dfd2f352a64563244c1fd9fdcf Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 27 Oct 2026 16:25:33 +0000 Subject: [PATCH] Record the public API cleanup review --- docs/roadmap/remediation.md | 79 ++++++++++++++++++++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 851dfe4..56d73bc 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,4 +1,9 @@ -# Interface Remediation Review Findings +# Implementation Remediation Review Findings + +Current follow-up: the public API cleanup review has one open finding (9). +The earlier interface-remediation review and completed findings 1–8 below are +retained as historical evidence. The current review's scope and validation are +recorded with finding 9. Reviewed commit: `98c8b7c8423a816acdfceace36165366b13719ee`. Algorithmic follow-up: `91769e9` (unchanged production code). @@ -437,3 +442,75 @@ pointer-conservative propagation. The fixed-point algorithm and inheritance link semantics need not change. Reuse transitive/cyclic sensitivity regressions and add focused coverage for a subtree record with several dependencies; no performance threshold or new public dependency restriction is warranted. + +## 9. Source Lookup Test Compares Canonical Paths With Symlinked Fixture Paths + +Priority: P2. Status: open. This is a test portability defect, not a production +source-lookup defect. + +Reviewed revision: `69bd9ead53b0bd1d4b325de2f357b566291a4803`. +Comparison baseline: `556b10b2cd32cbe3240ced79f72bfcc2fe1ebc35`. +Scope: the completed [API cleanup plan](implementation.md) and +[accepted roadmap](api.md), including facade conversions, recording intents, +source lookup, observable string migrations, maintained examples, and current +documentation. Existing findings 1–8 were not reopened. + +Relevant code: `TestResultSourceLookup` in +[source_lookup_test.go](../../source_lookup_test.go), lines 23–26 and 62–74. + +The test identifies imported and underlay sources by comparing `SourceInfo.Path` +with the original paths returned by its fixture writer. Host source paths are +canonicalized by ConfigKit, but `t.TempDir()` can return a path containing a +symlink when the temporary-directory environment names one. The same files then +have different lexical path spellings. Both records are present with the correct +layers and version evidence, but neither comparison matches, so the test fails +with "imported or underlay source evidence missing". + +This is reproducible on Linux with a symlinked `TMPDIR`; it does not depend on +access to macOS or establish a new platform-validation gate. The failure-path +lookup test in the same file already uses `filepath.EvalSymlinks` for its fixture +path and does not have this particular comparison defect. + +Reproduction from the repository root, with disposable files outside the checkout: + +```sh +probeRoot=$(mktemp -d /tmp/configkit-source-review.XXXXXX) +mkdir "$probeRoot/real" +ln -s "$probeRoot/real" "$probeRoot/link" +TMPDIR="$probeRoot/link" GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off \ + go test . -run '^TestResultSourceLookup$' -count=1 +``` + +Expected: the focused test passes for either spelling of a valid temporary +directory, retaining its source-evidence assertions. Actual: it fails at line 74 +with canonical records under the real directory and expected paths under its +symlink. The same focused test passes with `TMPDIR` set to the real directory. +This false failure can prevent ordinary development/release checks from completing +in an otherwise valid environment; it does not indicate lost source evidence. + +Remediation: canonicalize the test's fixture directory before creating its files, +or resolve the imported/underlay expected paths with `filepath.EvalSymlinks`, as +the failed-load case already does. Preserve record attribution and detachment +assertions. Do not change production canonical-path behavior, forbid symlinked +temporary directories, or add a platform/CI gate. Verify the existing focused +test with both real and symlinked temporary roots after correction. + +Other review results: the root inventory contains the expected 370 exported +declarations/members, with exactly the accepted name/signature/value changes +against the prior 372-entry catalog. The two fixed recording methods and the +existing assignment helpers are the only `addRecord` callers. Source lookup +clones retained records using existing ownership/sanitization machinery. No other +required architectural gap, production regression, or inadvertently strengthened +public contract was identified in this sprint. + +Validation passed on Linux with Go 1.26.10 and cached dependencies: + +```sh +GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off ./scripts/check.sh +GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off go test -race ./... +GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off \ + go test . -run 'SourceLookup|ResolutionRecordingIntents|TestPublicFacadeBoundary|TestDependencyImportBoundary|TestLocalDocumentationLinks' -count=1 +``` + +The additional symlink-root invocation above failed as described. Production +code and repository tests were not changed during this review.