Skip to content

fix(scanner): Resolve TypeScript ESM specifiers that name emitted files - #182

Merged
JordanCoin merged 10 commits into
mainfrom
claude/codemap-graph-accuracy-132
Sep 5, 2026
Merged

fix(scanner): Resolve TypeScript ESM specifiers that name emitted files#182
JordanCoin merged 10 commits into
mainfrom
claude/codemap-graph-accuracy-132

Conversation

@JordanCoin

@JordanCoin JordanCoin commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Last of the Graph accuracy milestone (#172). Fixes #132.

Now based on main. #171 has merged (49f9ed6), so main is brought in and the diff is this PR's own 12 files: scanner/filegraph.go, scanner/tsesm_test.go, testdata/typescript-esm-specifiers/.

What was wrong

Under ESM and NodeNext, TypeScript requires the specifier to name the emitted file. So a project's own source reads:

import { helper } from "./helper.js";   // the only file on disk is helper.ts

The specifier already carries an extension, so tryExactMatch appending resolver extensions produced helper.js.ts and matched nothing. Every such import was lost — and since NodeNext makes this mandatory rather than stylistic, that's most imports in an affected project.

The fix, and the case it must not break

An emitted extension now maps back to the TypeScript sources that produce it — but only after the literal path has been tried. That ordering is the important part:

src/real.js  importers = [src/c_real_js_wins.ts]     <- an existing .js wins
src/real.ts  importers = []                          <- its .ts twin does not steal the edge

If helper.js genuinely exists beside helper.ts, the specifier names a file that exists, and rewriting it to TypeScript would be a wrong edge rather than a missing one. The fixture has exactly that pair to pin it.

main's tryExactMatch (post-#171) checks the literal path first, then loops the resolver extensions; the new candidate loop sits between the two, so the ordering guarantee is structural rather than incidental.

The fixture

testdata/typescript-esm-specifiers/, a NodeNext tsconfig.json and:

src/helper.ts    importers = [a_js_to_ts.ts, d_extensionless.ts]   ./helper.js and ./helper
src/widget.tsx   importers = [b_jsx_to_tsx.ts]                     ./widget.jsx
src/real.js      importers = [c_real_js_wins.ts]                   real .js wins
src/real.ts      importers = []
src/e_missing.ts imports   = []                                    ./absent.js resolves to nothing

Against the base, TestTypeScriptESMSpecifiersResolve fails with src/helper.ts importers = [src/d_extensionless.ts], want [src/a_js_to_ts.ts src/d_extensionless.ts] and src/widget.tsx importers = [].

What I deliberately did not claim

.mjs and .cjs are unmapped. They emit from .mts and .cts, which aren't recognized source extensions here — the scanner never indexes such a file, so a mapping for them would be unreachable code that reads like support. I had both in the first draft and removed them once I checked extToLang. Adding .mts/.cts as scanned extensions is a separate change with its own file-count consequences; the tests pin the current behaviour (mjs has no reachable counterpart) so it's visible rather than silently missing.

Per-ecosystem summary (for release notes)

Ecosystem Before After
TypeScript ESM / NodeNext (./x.jsx.ts, ./x.jsxx.tsx) resolved to nothing; most imports lost resolved, including .d.ts
A real .js beside a same-named .ts resolved to the .js unchanged — still the .js
.mjs / .cjs specifiers unresolved unresolved (.mts/.cts are not scanned)
Every other ecosystem unchanged unchanged

Verification (on the merge, at 0695c28)

go build ./... OK, go vet ./... clean, gofmt clean. TestTypeScriptESMSpecifiersResolve and TestTypeScriptESMMissingSpecifierResolvesToNothing both pass. go test ./... at the repository baseline — only the three known root-environment permission failures (TestRunSetupCreatesConfigAndHooks, TestSelectReportsInaccessiblePrimarySetup, TestListProjectsPreservesPerChildScanErrors).

The merge of main had one conflict, in tryExactMatch: an add-on-one-side conflict where this PR contributes the candidate loop and main contributes nothing. Resolved by keeping the block, positioned after the literal check and before the extension loop — verified by reading the resulting function and by the real.js / real.ts fixture pair, which is precisely the assertion that fails if the ordering is wrong.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo

reneleonhardt and others added 9 commits September 4, 2026 13:45
Reuse shared subsystem scoring and scanner inventories directly. Bound prefix
and basename routing while preserving uniqueness and ambiguity checks.
Keep Rust coverage and workspace-boundary fixtures independent of Cargo
metadata latency. Preserve parser and graph contracts in focused tests.
Measure routing, scanner indexing, topology discovery, rendering, and watch publication with deterministic large inventories.
Reuse scanner inventories through fallback and CUE paths. Replace eager suffix maps with a compact sorted index while preserving exact-path ambiguity.
Collect provider files and manifests in one filtered walk. Hash cache inputs directly to avoid formatting and repeated path normalization.
Cache tree statistics and retain only the largest files. Compute skyline totals without copying the full source inventory.
Reuse resolved policy paths and startup inventories to avoid repeated worktree discovery and directory walks. Stream state directly into atomic replacements while preserving the previous state on encoding failure.
Keep the shared Cargo metadata deadline from canceling manual workspace recovery. Report fallback coverage when metadata probes time out.
Under ESM and NodeNext, TypeScript requires an import specifier to name the
emitted JavaScript file, so a project's own source reads
"import { helper } from './helper.js'" when the only file on disk is
helper.ts. The specifier already carries an extension, so appending resolver
extensions produced "helper.js.ts" and matched nothing, and most imports in
such a project were lost.

Map an emitted extension back to the TypeScript sources that produce it,
for JS-family importers only, and only after the literal path has been
tried: a real helper.js sitting beside helper.ts still wins, because the
specifier names a file that exists and rewriting it would be a wrong edge
rather than a missing one.

".mjs" and ".cjs" are deliberately unmapped. They emit from ".mts" and
".cts", which are not recognized source extensions, so the scanner never
indexes such a file and a mapping would be unreachable code that looks like
support.

Built on #171 because it rewrites tryExactMatch and the file index this
resolution depends on. Rebase onto main once that lands.

Relates to #132, #172

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo
Copilot AI lite review requested due to automatic review settings September 4, 2026 14:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

JordanCoin added a commit that referenced this pull request Sep 4, 2026
…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

Copy link
Copy Markdown
Owner Author

Correction to this PR's rebase note: the rebase onto main is not mechanical

I wrote above that I'd "rebase once #171 lands and the diff will shrink to the four files below." I tested that claim rather than leaving it as an assumption, and it is wrong in a way that matters: a naive rebase of this change onto main fabricates a wrong edge.

Why

The two branches order tryExactMatch differently.

main (c02e15a) tries the bare path lastResolverExtensions() ends with exts = append(exts, ""), described in its own doc comment as "empty string last as the final fallback":

extensions := ResolverExtensions()
for _, ext := range extensions {      // "" is the LAST element

#171 tries the literal path first, then loops the extensions with the trailing "" dropped:

if idx.byExact[path] == 1 && languagesCompatible(...) { return []string{path} }
for _, ext := range resolverExtensions[:len(resolverExtensions)-1] {

This PR's fix inserts typescriptSourceCandidates after the literal check, which on #171 is correct. Carried onto main with the same hunk shape, the candidates land before the literal path is ever tried — so ./real.js matches real.ts while real.js is sitting on disk beside it.

Reproduced, not argued

Cherry-picking 364966f onto c02e15a conflicts in scanner/filegraph.go. Resolving it the obvious way (keep main's loop, insert the candidate loop above it) builds clean and then fails this PR's own fixture:

tsesm_test.go:37: src/real.js importers = [], want exactly [src/c_real_js_wins.ts]
                  (an existing .js beats the .ts counterpart)
tsesm_test.go:37: src/real.ts importers = [src/c_real_js_wins.ts], want exactly []
                  (the .ts counterpart must not steal the edge)

That is a wrong edge, not a missing one — the failure mode this repo treats as the cardinal sin, and precisely what c_real_js_wins.ts was added to catch. The fixture does its job; I'm flagging it because the PR body currently invites the exact resolution that trips it.

The safe main-shaped resolution

main needs an explicit literal-path check ahead of the candidates, using its own []string / compatibleFiles index shape:

// main orders the bare-path match LAST (ResolverExtensions ends in ""),
// so the literal path has to be tried explicitly before rewriting a
// specifier to its TypeScript source. Otherwise "./real.js" matches
// real.ts while real.js sits on disk beside it -- a wrong edge, not a
// missing one.
if files, ok := idx.byExact[path]; ok {
    if compatible := compatibleFiles(sourceLanguage, files); len(compatible) > 0 {
        return compatible
    }
}

for _, candidate := range typescriptSourceCandidates(path, sourceLanguage) {
    if files, ok := idx.byExact[candidate]; ok {
        if compatible := compatibleFiles(sourceLanguage, files); len(compatible) > 0 {
            return compatible
        }
    }
}

With that, on main at c02e15a: TestTypeScriptESMSpecifiersResolve passes, go vet ./... is clean, go test ./scanner/ is green, and go test ./... is at the known baseline (the three root-environment permission failures only).

What I'm not doing

Not re-cutting this PR yet — the standing decision was to build on #171 and rebase after it lands, and I'm not overriding that on my own. This is here so that whoever performs the rebase (me, later, or anyone else) knows it needs the literal-path check added rather than a straight conflict resolution, and has a verified version to use.

Note this cuts both ways: #181 cherry-picks onto main cleanly, but this one does not, so "both were verified to merge cleanly" — which is true of a git merge of main into these branches — should not be read as "both rebase safely." A clean merge here still carries #171's commits, which is what makes it clean.


Generated by Claude Code

JordanCoin added a commit that referenced this pull request Sep 4, 2026
#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>
…accuracy-132

# Conflicts:
#	scanner/filegraph.go

Copy link
Copy Markdown
Owner Author

Resolved — and my earlier hazard comment no longer applies

My comment above warned that rebasing this PR onto main would fabricate a wrong edge, and gave a main-shaped remedy. That remedy is now unnecessary, and following it would be wrong. Recording that explicitly so nobody applies a fix to a problem that no longer exists.

The hazard was entirely about an ordering difference between the two bases:

#171 has since merged as 49f9ed6, so main now has the literal-path-first ordering this PR was written against. The two bases converged, and the conflict that produced the wrong edge is gone with them.

What I actually did

Merged main into this branch — no rebase, no force-push, so the existing history and any in-flight review stay valid. One conflict, in tryExactMatch, and it was the benign kind: this PR contributes the candidate loop, main contributes nothing at that spot. Kept the block, positioned after the literal check and before the extension loop:

if idx.byExact[path] == 1 && languagesCompatible(sourceLanguage, DetectLanguage(path)) {
    return []string{path}
}
// Under ESM and NodeNext, TypeScript requires the specifier to name the
// emitted JavaScript file, ...
for _, candidate := range typescriptSourceCandidates(path, sourceLanguage) {
    ...
}
for _, ext := range resolverExtensions[:len(resolverExtensions)-1] {

I didn't take "one trivial conflict" as sufficient reason to trust it — the whole point of the earlier finding was that a plausible-looking resolution here silently produces a wrong edge. So the check is the fixture, not the diff: src/real.js and src/real.ts exist side by side specifically so that any resolution which lets the .ts steal the edge fails loudly. It passes.

At 0695c28: diff is this PR's own 12 files, go build OK, go vet clean, gofmt clean, both TypeScript ESM tests pass, go test ./... at the repository baseline (the three known root-environment permission failures only).

This is the last of the Graph accuracy milestone (#172) — #148, #173, #147 and #136 are all on main.


Generated by Claude Code

@JordanCoin
JordanCoin merged commit 1a7aae7 into main Sep 5, 2026
12 checks passed
@JordanCoin
JordanCoin deleted the claude/codemap-graph-accuracy-132 branch September 5, 2026 20:50
@reneleonhardt

Copy link
Copy Markdown
Contributor

After all those substantial PRs I didn't expect a patch release 😄

Can your Claude double-check all of README, everything up-to-date?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scanner: TypeScript ESM/NodeNext projects silently lose most import edges (.js specifier -> .ts file)

4 participants