Skip to content

feat(scanner): Recover Cargo topology on scan failure - #117

Merged
JordanCoin merged 4 commits into
JordanCoin:mainfrom
reneleonhardt:feat/scanner-cargo-fallback
Aug 12, 2026
Merged

feat(scanner): Recover Cargo topology on scan failure#117
JordanCoin merged 4 commits into
JordanCoin:mainfrom
reneleonhardt:feat/scanner-cargo-fallback

Conversation

@reneleonhardt

Copy link
Copy Markdown
Contributor

What does this PR do?

When ast-grep fails or times out, the scanner recovers the Rust dependency graph from cargo metadata instead of failing hard:

  • buildFileGraphWithFallback runs the primary scan, and on an ast-grep IncompleteScanError builds a fallback ScanOutcome from cargo metadata (buildCargoFallbackOutcome).
  • The fallback outcome carries honest provenance (Sources: [{name: "ast-grep", status: timeout|failed}, {name: "rust-cargo", status: fallback}]) so coverage and fail-closed behavior stay consistent with the scan contract.
  • Conditional dependencies are identified (cargoDependencyIsConditional) and reported as conditional edges.

Review follow-ups included

  • fix(scanner): Normalize null Cargo targetscargo metadata entries with null target fields no longer crash or produce malformed edges.
  • fix(scanner): return context.Canceled when the fallback file scan fails on a pre-cancelled context — a pre-cancelled caller receives context.Canceled (the primary error is still preserved for real scan failures).

CLI / MCP surface

No new CLI commands or arguments, and no new MCP tools. Behavior of existing surface:

  • codemap --deps <path> on a Rust repository returns the recovered graph with rust-cargo fallback provenance instead of a hard error when ast-grep is unavailable or times out.
  • MCP get_dependencies returns the same fallback outcome with its provenance.

Coverage provenance notes

The fallback only engages on an ast-grep failure; a successful scan remains authoritative. Degraded outcomes are bounded and deterministic, matching the fail-closed contract.

Developed with carefully directed, manually reviewed AI assistance.

reneleonhardt and others added 4 commits August 8, 2026 22:57
When ast-grep fails or times out, recover the Rust dependency graph from `cargo metadata` instead of failing hard. The fallback outcome carries honest provenance (ast-grep `timeout|failed` plus a `rust-cargo` fallback source) so coverage and fail-closed behavior stay consistent with the scan contract. Conditional dependencies are identified and reported as conditional edges.
`cargo metadata` entries with null target fields no longer crash or produce malformed edges.
…ls on a pre-cancelled context

Preserve the primary ast-grep error for real scan failures; honour caller
cancellation when ScanFiles fails because the context was already cancelled.

Co-Authored-By: Whale integration <whale@local>
scanForGraphOutcome only delegated to scanForGraphOutcomeWithFilters with an
empty Filters{} and had no callers; staticcheck U1000.
@reneleonhardt
reneleonhardt force-pushed the feat/scanner-cargo-fallback branch from f01d812 to 7d14431 Compare August 8, 2026 20:59
@JordanCoin

Copy link
Copy Markdown
Owner

Heads-up before these land: #117 and #118 don't compile together, in either merge order. Both are green in CI because each is built against main, not against the other.

Git auto-merges them with no textual conflict — the collision is semantic:

main today:

func discoverCargoManifests(root string, files []FileInfo) []string

#118 changes the signature:

func discoverCargoManifests(ctx context.Context, root string, files []FileInfo) ([]string, error)

#117 adds a new caller (scanner/cargofallback.go:72) using the old one:

manifests := discoverCargoManifests(root, files)

Result whichever way round they go:

scanner/cargofallback.go:72:15: assignment mismatch: 1 variable but discoverCargoManifests returns 2 values
scanner/cargofallback.go:72:44: not enough arguments in call to discoverCargoManifests
	have (string, []FileInfo)
	want (context.Context, string, []FileInfo)

Verified by merging both into a scratch worktree in each order and building.

The fix is one line in buildCargoFallbackOutcome, which already has a ctx in scope:

manifests, err := discoverCargoManifests(ctx, root, files)
if err != nil {
    return ScanOutcome{}, err
}

Simplest is to fix it in whichever of the two you'd like to merge second. No need to change anything else — #115, #116 and #119 are unaffected, and all five merge cleanly apart from this.

@reneleonhardt

reneleonhardt commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

If you want to merge them both, can you just merge this one first? That was the order for the integration stack of my next wave, I'll fix the other one then 😅

Or you can just update yourself, edits are enabled as always.

@JordanCoin JordanCoin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Reviewed the full Cargo fallback path and provenance handling at 7d14431. The fallback is bounded, preserves the primary failure when unavailable, reports partial evidence honestly, and the full scanner suite passes. This should land before #118, which must then update its changed discoverCargoManifests caller. No blocking finding in this PR — approved.

@JordanCoin
JordanCoin merged commit 930fda2 into JordanCoin:main Aug 12, 2026
12 checks passed
@reneleonhardt
reneleonhardt deleted the feat/scanner-cargo-fallback branch August 12, 2026 07:37
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.

2 participants