fix(scanner): Resolve tsconfig alias targets written as "./*" - #175
Conversation
create-next-app writes "paths": {"@/*": ["./*"]}, and has for years, so this
is the most common TypeScript layout in the wild. Substituting the wildcard
produced "./lib/a1" while the file index stores repository-relative paths with
no "./" prefix, so tryExactMatch and trySuffixMatch found nothing and a file
imported everywhere reported no importers at all.
The failure was silent: nothing marks an unresolved alias, so a whole Next.js
project read as standalone files with a confident answer.
Normalize the substituted target before matching. filepath.Join already
cleaned it whenever a baseUrl was set, which is why this only ever bit
projects without one, and why "@/*": ["*"] worked as a workaround.
The same normalization applies to the no-wildcard exact-alias branch, which
had the identical bug for targets like {"@app": ["./lib/a1"]}.
Relates to #173, #172
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo
The graph appends importers in analysis order, which is not stable across runs, so asserting a sequence made the alias fixture test fail on two CI legs while passing locally. The exactness that matters here is membership: neither a superset nor a subset of the expected importers. The underlying nondeterminism is pre-existing and unrelated to the alias fix; it is reported separately rather than papered over here. Relates to #173, #172 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo
|
CI went red on Same members, different order. I'd asserted a sequence, so the test was reading a real property of the output as a failure.
|
|
Not this PR'sThis diff touches Identical. Unloaded it's 0/15 on both, which is why local runs and most CI runs are green. What's actually broken
data, err := os.ReadFile(path)
if err == nil {
var status watchReadiness
if err := json.Unmarshal(data, &status); err != nil {
return fmt.Errorf("reading daemon readiness: %w", err) // ← gives up here
}A file that exists but is empty or half-written reads successfully with zero or partial bytes, so Proved directly against the real function: Note the 0s — it doesn't wait out any part of the 30s timeout, it fails on the first poll. In the test the child is This isn't confined to the test. Any daemon that creates its readiness file non-atomically has the same window, so on a loaded machine What I'm doing about itNot fixing it inside this PR — it's
Generated by Claude Code |
|
Correcting two things in my comment above before they mislead anyone. 1. The "20/20 failures on both branches" measurement was wrong. I ran relocated test binaries from outside the repo; from there they exit without running the test at all, so I was measuring my own harness, not the test. Redone properly — in-repo, via The conclusion that it isn't this PR's still holds, on better evidence: the diff touches only 2. I overstated the production impact. I wrote that "on a loaded machine What survives both corrections: the mechanism is confirmed. An empty or partial readiness file makes Fix is up as #177, standalone. It's right on its own terms — an unparseable file is a file still being written, not a failed daemon — but I'm not claiming it as proven to fix this leg, since I can't reproduce the failure to test against. This PR's own head Generated by Claude Code |
Ported from #177, which I opened for this failure. Carrying it here so this PR has its best chance of going green rather than waiting on that one to merge; it becomes a no-op once main has it. waitWatchReadiness returned on the first successful read, so a readiness file caught mid-write failed json.Unmarshal and aborted the wait immediately. Measured against the real function, an empty file returns "reading daemon readiness: unexpected end of JSON input" after 0s, which is the string TestRunWatchStartWaitsForChildReadinessFailure reported on this PR's ubuntu-1.24 leg. I could not reproduce that CI failure locally, so this is the plausible cause rather than a proven one. The change is correct on its own terms either way: an unparseable file is a file still being written, not a daemon that failed. Relates to #173, #172 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo
…olves `--min-importers 1` hid every hazard on this repository, and the reason was not the threshold. Go resolves imports at package level, and BuildFileGraph deliberately drops an import that resolves to more than one file rather than fanning it into an edge per file, so a file inside a multi-file Go package has zero file-level importers by construction. `scanner/filegraph.go` scored 0 and read as harmless. So did every same-package collision, including all six pairs issue #134 verified by hand. A shared file whose language resolves at package granularity is now weighted as the files outside its package that import the package, plus the package's other files, and the count is labelled `package importers` so it is not read as a file-level number. Languages whose imports name files keep the file-level count and the plain label. FileGraph.Packages is populated for Go and nothing else, which is exactly the set this is correct for. The cross-package term cannot come from the graph — the edges are the ones that were dropped — so collide now keeps the scan outcome it was already paying for and counts the raw import strings. ScanForDeps plus BuildFileGraphFromOutcome is the same single scan BuildFileGraph was doing. Real effect on this repository, where the default previously printed nothing: 6 PRs scanner/filegraph.go 73 package importers <- #171, #174, #175, ... 3 PRs config/config.go 38 package importers <- #171, #181, #182 3 PRs main.go 6 package importers <- #175, #179, #180 --min-importers now defaults to 0. A file two open PRs both change is a hazard whatever its weight, and a default that hides hazards answers "no collisions" on a repository full of them. The flag stays for narrowing a long list. Three tests added: a Go fixture where two same-package files collide and carry a non-zero package weight (with the self-import and third-party cases held out of the count), a file-resolved language keeping file scope and its plain label, and #134's six measured pairs proved unchanged by the weighting — reordering them is allowed, adding or dropping one is not. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t
#180) * feat: codemap collide, rank open PRs by shared-file merge-order hazard CI structurally cannot see cross-PR collisions: every PR is built against main and never against its siblings. Issue #134 measured that blind spot by merging six worktree pairs by hand, and #117/#118 shipped a miscompile through it. `codemap collide` reads open PRs through `gh pr list --json files`, intersects their changed paths, and weights each shared file by the importer count from the graph on the current checkout. The intersection is glue; the weighting is the part that needs codemap, because only the graph knows that a collision on a 23-importer hub is a different severity from one on a test fixture. Honesty rules, per the design principle in #134 (a composite inherits the honesty of its primitives and states it with more authority): - Importer counts are stated as facts only while graph coverage is complete. Degraded coverage prints "unknown importers", drops the verdict to TRUST LOW, and says ranking fell back to shared-file count. - A "no collisions" answer from a degraded graph is TRUST LOW too: a negative finding from a partial graph is as unreliable as a positive one. - Coverage attribution is whole-graph, not per-language. Narrowing "partial" to a subset of languages by matching free-text notes would hand back confidence the graph never claimed. #174's ResolvesFileLevelImports is the supported seam for per-language attribution; collideImportersKnown is the single function it belongs in. - --min-importers never hides a file whose count is unknown, and never drops a hazard silently: the hidden count and the way to see them are printed. - A file the graph carries no edges for at all (a YAML rule, a fixture) reports "not in graph" rather than a zero that reads as "nothing imports it". Tests cover the pair/shared-file computation against issue #134's measured 4-PR matrix (6 of 6 pairs, including the 2-vs-3 distinction), the ranking order, degraded coverage yielding TRUST LOW with unknown counts, and a golden human output. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t * fix(scanner): Keep the Rust fallback when cargo metadata times out Ported verbatim from @reneleonhardt's open PR #171, which fixes this already. Carrying it here so this PR can go green rather than waiting on that one to merge; it becomes a no-op once main has it. buildRustWorkspaceIndex shadowed its caller's ctx with the cargo-metadata deadline, so once that deadline passed ctx.Err() returned DeadlineExceeded and the whole graph build failed with a bare "context deadline exceeded" instead of falling back to the manually derived Rust workspace. On a cold or loaded runner three seconds is not always enough for cargo metadata, and mcp/TestRustGraphContextHandlersDisclosePartialCoverage has now failed this way on three separate pull requests. Separating the metadata context from the caller's lets an expired deadline break out of the loop and keep the fallback index, which is what the test asserts and what a consumer needs: partial coverage disclosed, not a failed graph. Relates to #147, #172 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo (cherry picked from commit 24af8fb) * fix(watch): Treat an unparseable readiness file as not-ready-yet waitWatchReadiness returned on the first successful read, so a readiness file caught mid-write — existing but empty or partial — failed json.Unmarshal and aborted the wait immediately, reporting a startup failure for a daemon that had not finished writing. Only os.ErrNotExist counted as "not ready". Measured against the real function: an empty file returns "reading daemon readiness: unexpected end of JSON input" after 0s, without waiting out any part of the 30s timeout. Keep polling on a parse failure until the deadline, and surface the last parse error when the deadline passes, so a file that never becomes valid still says why rather than only that it timed out. publishWatchReadiness already renames its payload into place atomically, so codemap's own daemon does not open this window; the reader was brittle to any writer that is not atomic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo (cherry picked from commit 35d2af3) * fix(collide): Weight Go collisions at the granularity Go actually resolves `--min-importers 1` hid every hazard on this repository, and the reason was not the threshold. Go resolves imports at package level, and BuildFileGraph deliberately drops an import that resolves to more than one file rather than fanning it into an edge per file, so a file inside a multi-file Go package has zero file-level importers by construction. `scanner/filegraph.go` scored 0 and read as harmless. So did every same-package collision, including all six pairs issue #134 verified by hand. A shared file whose language resolves at package granularity is now weighted as the files outside its package that import the package, plus the package's other files, and the count is labelled `package importers` so it is not read as a file-level number. Languages whose imports name files keep the file-level count and the plain label. FileGraph.Packages is populated for Go and nothing else, which is exactly the set this is correct for. The cross-package term cannot come from the graph — the edges are the ones that were dropped — so collide now keeps the scan outcome it was already paying for and counts the raw import strings. ScanForDeps plus BuildFileGraphFromOutcome is the same single scan BuildFileGraph was doing. Real effect on this repository, where the default previously printed nothing: 6 PRs scanner/filegraph.go 73 package importers <- #171, #174, #175, ... 3 PRs config/config.go 38 package importers <- #171, #181, #182 3 PRs main.go 6 package importers <- #175, #179, #180 --min-importers now defaults to 0. A file two open PRs both change is a hazard whatever its weight, and a default that hides hazards answers "no collisions" on a repository full of them. The flag stays for narrowing a long list. Three tests added: a Go fixture where two same-package files collide and carry a non-zero package weight (with the self-import and third-party cases held out of the count), a file-resolved language keeping file scope and its plain label, and #134's six measured pairs proved unchanged by the weighting — reordering them is allowed, adding or dropping one is not. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t * fix(collide): rank a pair by its heaviest shared file, not the first one seen Shared files sort by PR count first, so a pair colliding on a hub could be reported by a fixture touched by more PRs and ranked below a lesser pair. Found by independent review; the new test reproduces it and fails without the change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: r <r@r>
Second of the Graph accuracy milestone (#172). Fixes #173.
Your diagnosis was exact, so this mostly confirms it and adds one case you didn't mention.
What was wrong
resolvePathAliassubstitutes the wildcard into the target, so"@/*": ["./*"]with@/lib/a1produces./lib/a1. The file index stores repository-relative paths with no./prefix, sotryExactMatchandtrySuffixMatchboth miss and the import resolves to nothing.filepath.Join(baseURL, resolved)cleans the path, which is why this only ever bit projects without abaseUrl— and why"@/*": ["*"]works as the workaround.The failure is silent. Nothing marks an unresolved alias, so a whole Next.js project reads as standalone files and the answer looks confident.
The fixture that proves it
testdata/tsconfig-alias-dotslash/— create-next-app's defaulttsconfig.json, onelib/a1.ts, and two files importing it via@/lib/a1. Onmain:{"file":"lib/a1.ts","importers":null,"importer_count":0,"is_hub":false}After, the importers are
app/layout.tsxandapp/page.tsx— both of them, and nothing else.TestTsconfigDotSlashAliasResolvesImportersfails against unmodifiedresolvePathAliaswithlib/a1.ts importers = [], want exactly [app/layout.tsx app/page.tsx].Correction to an earlier version of this description: it showed that result as an ordered JSON array and called it an "exact list". The membership is exact; the order is not stable, and that is a pre-existing property of
mainrather than anything this PR introduces — see the comment below for twelve runs producing twelve orderings. The test compares as a set for that reason.The case #173 didn't mention
The no-wildcard exact-alias branch has the identical bug —
{"@app": ["./lib/a1"]}never resolved either. Same one-line fix, and it's covered byTestPathAliasTargetShapes/exact_alias_with_dot_slash, which also fails on main.Every target shape, checked for regression
TestPathAliasTargetShapespins all of them so this fix cannot break the forms that already worked. Against main, exactly the three broken ones fail and the three working ones pass:"@/*": ["./*"]"@/*": ["./src/*"]"@app": ["./lib/a1"]"@/*": ["*"]"@/*": ["src/*"]"@/*": ["./*"]+baseUrlnormalizeAliasTargetdeliberately preserves../shared/xrather than collapsing it, so a target escaping the project can't be rewritten into something that matches an unrelated file.Per-ecosystem summary (for release notes)
pathstargets starting./(create-next-app default, nobaseUrl)Notes
coveragein the--importersJSON") is in fix(coverage): Stop claiming complete coverage on symbol-level languages #174, not here — a zero-importer answer there was byte-identical whether the scan was thorough or impossible. The two fixes are complementary rather than redundant: this PR is a genuine miss, so coverage would have saidcompleteand been wrong. Disclosure can't substitute for resolution; it only helps where the tool can know it doesn't know.mainlocally: clean merge,go buildandgo vetclean, suite at baseline.Imports/Importerschanges user-visible output for everyone and wants its own PR and fixture. It does block Graph accuracy: fix the four known holes before building on the graph #172's "exact importer list" exit criterion, so it's worth scheduling..js→.ts), which scanner: tsconfig alias target "./*" (create-next-app default) never resolves; whole Next.js graph reads as standalone #173 notes it compounds. That's still queued behind js-imports rule only matches double-quoted require()/import — single-quoted CommonJS invisible to --importers #147.Verification
go vet ./...clean,gofmtclean,go test ./...at the main baseline (only the three known root-environment permission failures). Fixture verified end-to-end with a built binary for--importersand--importers --json, and the fixture test passes at-count=20.🤖 Generated with Claude Code
https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo