Skip to content

fix(coverage): Stop claiming complete coverage on symbol-level languages - #174

Merged
JordanCoin merged 5 commits into
mainfrom
claude/codemap-prs-review-merge-nwifof
Sep 4, 2026
Merged

fix(coverage): Stop claiming complete coverage on symbol-level languages#174
JordanCoin merged 5 commits into
mainfrom
claude/codemap-prs-review-merge-nwifof

Conversation

@JordanCoin

Copy link
Copy Markdown
Owner

First of the Graph accuracy milestone (#172). Fixes #148.

What was wrong

Coverage was derived from one question: did the scanner run? ast-grep parses Swift fine — #148 notes 1068 functions extracted correctly — so the source came back authoritative and coverage came back complete.

But the edge model is what doesn't apply. In Swift, Kotlin, Java, C# and Scala an import names a module, package or namespace, never a file, and two files in the same package reference each other with no import statement at all. So resolving imports yields SwiftUI, Foundation, System.Text — names belonging to no file in the project — and the graph comes back structurally empty rather than empty-by-accident.

Pairing that with complete is what makes it the worst of the bugs: a consumer reads "complete" beside zero dependents and concludes a change is isolated, when nothing capable of finding those edges ever ran.

The fixture that proves it

testdata/symbol-imports-swift/ — three Swift files where UserViewModel references User from Models.swift, with no import between them, exactly as Swift requires. On main today:

"coverage": {"status": "complete", "sources": [{"name": "ast-grep", "status": "authoritative"}], "issues": []}

Zero edges, full confidence. After:

"coverage": {"status": "partial", "sources": [
  {"name": "ast-grep", "status": "authoritative"},
  {"name": "symbol-imports/swift", "status": "unavailable",
   "detail": "Swift (3 files): imports name modules, not files, and same-package files need no import: intra-project edges need symbol-reference resolution and are not represented"}
]}

--importers inherits it, which also answers the shape of #138 — "No files import X" no longer stands alone when the graph cannot know:

No files import Sources/App/Models.swift.
Coverage: partial — Swift: imports name modules, not files, and same-package files need no import: …

testdata/file-imports-go/ is the counterpart guard, so this never becomes a blanket downgrade: Go imports do resolve to files, so an empty graph there is a real finding.

--importers pkg/user.go  ->  importers: ["main.go"]  (exact)  coverage: complete

TestSwiftFixtureNeverReportsCompleteCoverage is load-bearing — against unmodified filegraph.go it fails with coverage sources [{Name:ast-grep Status:authoritative Detail:}] omit a symbol-imports source naming Swift, which is the bug verbatim.

What changed

scanner/importmodel.go classifies languages by whether an import can resolve to a file. Presence of a symbol-level language downgrades coverage to partial and attaches one source per language with its file count — so the output names which slice it cannot see, rather than emitting a bare partial with nothing to point at. Wired into the graph's provenance, which is where --deps, --importers and blast-radius all read from, plus the newDepsProject default path.

The rule only ever removes confidence: coverage already partial or unavailable keeps its status, and languages with a file-level import model are untouched.

Per-ecosystem summary (for release notes)

Ecosystem Before After
Swift, Kotlin, Java, C#, Scala complete with 0 edges partial, naming the language and file count
Go, Python, JS/TS, Rust, C/C++, Dart, Ruby, PHP, Lua, Solidity, Bash, CUE unchanged unchanged

There is no CHANGELOG in the repo, so this table is the release-note text rather than a file I invented.

Two things I did not do, deliberately

  • The false edges. --deps / --importers / blast-radius report coverage: complete on languages with no file-level imports #148's closing note about VendorSDK ───▶ src/Integrations/VendorSDK is real: Strategy 6 in fuzzyResolveWithWorkspace matches a normalized namespace against a directory name, and its comment says it exists for C#. So for these languages codemap can also emit a wrong edge, not just miss them. Removing that is a behavior change for current C# users and deserves its own PR and fixture — say the word and I'll do it next.
  • Elixir is not classified. alias MyApp.Foo is module-level, but Mix conventions do map modules to paths, so suffix matching often works. I'd rather leave it resolvable than wrongly mark a working ecosystem degraded.

One side effect you should decide on

testdata/symbol-imports-swift/ contains real .swift files, so codemap . on this repo now reports partial with a Swift note. That statement is true — the repo does contain 3 unresolvable Swift files — but it's noise from fixtures. The natural fix is "exclude": ["testdata"], except .codemap/ is gitignored so it can't ship. The alternative is adding testdata to IgnoredDirs in scanner/walker.go alongside vendor and node_modules, which is a default-behavior change for every user — so I didn't make that call unilaterally. Happy to do either.

Verification

go vet ./... clean. go test ./... at the main baseline (only the three known root-environment permission failures). Fixture behavior verified end-to-end with a built binary for --deps, --deps --json and --importers.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo


Generated by Claude Code

Swift, Kotlin, Java, C# and Scala name modules in their imports, not files,
and two files in the same package reference each other with no import at all.
The file-to-file edge model cannot represent their intra-project structure, so
resolving their imports yields framework names belonging to no file and the
graph comes back structurally empty.

Coverage was derived only from whether the scanner ran, so ast-grep succeeding
made the result authoritative and complete. A consumer reading "complete"
beside zero dependents concludes a change is isolated, when in fact nothing
capable of finding those edges ever ran.

Classify languages by whether an import can resolve to a file. When files in a
symbol-level language are present, coverage drops to partial and carries one
source per language naming it and its file count, so the result says which
slice of the project it cannot see instead of emitting a bare partial.
Languages whose imports do resolve to files are untouched: an empty graph there
is a real finding and stays complete.

Relates to #148, #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 13:41

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.

A zero-importer answer carried no honesty signal: coverage_status was
omitempty and the graph's complete status is the empty zero value, so the
common case emitted no field at all. A consumer could not tell "nothing
imports this, and I checked" from "nothing imports this, and I could not
check" — the two were byte-identical.

Spell the zero value out as complete and always emit the field.

Deciding whether a blast-radius importer section is empty now keys on the
report's data rather than its rendered text, because every report carries a
coverage line and rendering first made files with no importers look like they
had content.

Relates to #148, #173, #172

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo

Copy link
Copy Markdown
Owner Author

Pushed e0c07b7, folding in the --importers --json coverage field — it's asked for by both #148's spirit and #173 explicitly, and it turned out to be a bigger hole than "the field is missing".

ImportersReport.CoverageStatus was omitempty, and a complete graph's status is the empty zero value. So the common case emitted nothing, and on main today these two answers are byte-identical:

{"file":"pkg/user.go","importers":null,"importer_count":0,"is_hub":false}   // checked thoroughly, genuinely zero
{"file":"App/Models.swift","importers":null,"importer_count":0,"is_hub":false}   // could not check at all

Now:

{"file":"pkg/user.go",...,"coverage_status":"complete"}
{"file":"Sources/App/Models.swift",...,"coverage_status":"partial","coverage_notes":["Swift: imports name modules, not files, …"]}

One thing worth calling out, because it's a regression I introduced and then caught rather than something the change did cleanly: making the status always present broke TestBlastRadiusOmitsEmptyImporterSections (passes on main, failed with my change). Blast-radius decided whether an importer section was empty by rendering it and checking for whitespace — and now every report renders a Coverage: complete footer, so files with no importers stopped looking empty and started emitting sections. Fixed by deciding on the report's data (len(Importers) == 0 && len(HubImports) == 0) instead of its rendered text, which is the sturdier test anyway.

Full suite back at the baseline (only the three root-environment permission failures); go vet clean.


Generated by Claude Code

JordanCoin and others added 2 commits September 4, 2026 10:37
Independent review of #174 found the MCP importers surface still built
coverage_status from the raw zero value, so a fully checked graph emitted
"" while the CLI emitted "complete". Use EffectiveStatus() and guard it
with a test that fails without the change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t
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
@JordanCoin
JordanCoin merged commit c02e15a into main Sep 4, 2026
12 checks passed
@JordanCoin
JordanCoin deleted the claude/codemap-prs-review-merge-nwifof branch September 4, 2026 14:51
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>
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.

--deps / --importers / blast-radius report coverage: complete on languages with no file-level imports

3 participants