Skip to content

feat: codemap collide, rank open PRs by shared-file merge-order hazard - #180

Merged
JordanCoin merged 7 commits into
mainfrom
feat/collide-shared-file-hazard
Sep 4, 2026
Merged

feat: codemap collide, rank open PRs by shared-file merge-order hazard#180
JordanCoin merged 7 commits into
mainfrom
feat/collide-shared-file-hazard

Conversation

@JordanCoin

Copy link
Copy Markdown
Owner

Implements the third item on #134's suggested order: cross-PR collision prediction, graph-weighted.

Why

CI structurally cannot see this. Every PR is built against main, never against its siblings, so four PRs can each be green and be mutually exclusive. #134 measured exactly that by creating six worktrees and merging each pair by hand; #117/#118 already shipped a miscompile through the same blind spot.

codemap collide reads open PRs via gh pr list --state open --json number,title,headRefName,files, intersects the changed paths, and weights each shared file by the importer count from the graph on the current checkout. As #134 notes, the intersection needs no graph and is glue — the weighting is the part that belongs here, because only codemap knows that a collision on a 23-importer hub is a different severity from one on a test fixture.

Honesty rules

Per #134's design principle — a composite inherits the honesty of its primitives and states it with more authority; refuse a confident verdict when inputs report degraded coverage:

  • Importer counts are stated as facts only while graph coverage is complete. Degraded coverage prints unknown importers, drops the verdict to TRUST LOW, prints the graph's own notes as the reason, 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 exactly as unreliable as a positive one.
  • Coverage attribution is deliberately whole-graph, not per-language. Coverage notes are free text, so narrowing "this graph is partial" to a subset of languages by matching those strings would hand back confidence the graph never claimed. collideImportersKnown is the single seam where per-language attribution belongs once fix(coverage): Stop claiming complete coverage on symbol-level languages #174's scanner.ResolvesFileLevelImports lands — no other code needs to change.
  • --min-importers never hides a file whose importer count is unknown (a threshold can only exclude a severity that was measured), and never drops a hazard silently: the hidden count and the flag that reveals them are printed. An all-filtered result says No shared files at or above --min-importers 1, never No shared files between open PRs.
  • A file the graph carries no edges for at all (a YAML rule file, a fixture) reports not in graph, not a 0 that reads as "nothing imports it".

Importer counts come from buildImportersReportFromGraph, the same helper behind codemap --importers and blast-radius, so all three report the same number for the same file. No scanner files are touched.

Compatibility with #174

#174 makes GraphCoverage.Status go partial for symbol-level languages. Because collide's verdict keys off exactly that status, a Swift/Java/Kotlin repository automatically produces unknown importers and TRUST LOW once #174 merges, with no change here.

Output

OPEN PRs (4): #124, #125, #126, #127

SHARED FILES (each = a merge-order hazard):
  4 PRs   scanner/astgrep.go         23 importers      <- #124, #125, #126, #127
  4 PRs   scanner/rustgraph.go       4 importers       <- #124, #125, #126, #127
  3 PRs   scanner/sg-rules/rust.yml  not in graph      <- #125, #126, #127

PREDICTED COLLIDING PAIRS:
  #125 + #126  ->  3 shared file(s)  top: scanner/astgrep.go (23 importers)
  ...

Graph coverage: go complete (2 files), 1 file not tracked by the import graph. TRUST HIGH.

--json emits the same data (schema codemap.collide/v1): PRs, shared files with per-file coverage flags, ranked pairs, language coverage, and the trust verdict.

gh is the only network access. Missing or unauthenticated gh fails with one line naming the command to run.

Open question: the --min-importers 1 default

On this repository the default hides everything. Go resolves imports at package level, so same-package files have 0 cross-package importers, and all 7 currently shared files score 0:

$ codemap collide
OPEN PRs (7): #171, #174, #175, #176, #177, #178, #179

No shared files at or above --min-importers 1.
  (7 shared file(s) hidden by --min-importers 1; rerun with --min-importers 0 to see them)

Graph coverage: graph complete, no shared files to weight. TRUST HIGH.

The same is true of #134's own validated example: scanner/astgrep.go and scanner/rustgraph.go are same-package Go files, so a literal default of 1 would have reproduced 0 of the 6 pairs the issue verified. The default is shipped as specified rather than changed unilaterally, and the omission is always announced rather than silent — but 0 is arguably the right default for Go-majority repositories, and that is worth deciding in review.

Tests

collide_test.go (10 tests, no gh calls):

  • pair/shared-file computation against compose primitives: ship the maintainer combos as commands (validated: cross-PR collision prediction) #134's measured 4-PR matrix: 6 of 6 pairs, including the 2-vs-3 distinction
  • ranking by importer count first, shared-file count second
  • degraded coverage yields TRUST LOW with unknown importers and no fabricated zero
  • an empty result from an unavailable graph still carries TRUST LOW
  • --min-importers hides measured-low files but never unknown ones, and announces what it hid
  • golden human output
  • gh failures collapse to one actionable line

go vet ./... clean; go test ./... green across all packages.

Closes part of #134.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t

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
Copilot AI lite review requested due to automatic review settings September 4, 2026 14:25

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.

claude and others added 3 commits September 4, 2026 10:34
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)
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)
…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

Copy link
Copy Markdown
Owner Author

The two CI failures were both pre-existing flakes, not this PR

Run 33883666326. Neither failure touches anything collide adds, and both are known:

  • Test (ubuntu-latest, 1.26)mcp/TestRustGraphContextHandlersDisclosePartialCoverage:
    main_more_test.go:487: importers MCP output omits partial coverage: Failed to build file graph: context deadline exceeded.
    buildRustWorkspaceIndex shadowed its caller's ctx with the cargo-metadata deadline, so once three seconds passed the ctx.Err() check aborted the whole graph build instead of falling back to the manually derived Rust workspace. Fixed by the 9-line hunk in scanner/rustcargo.go that fix(scanner): Match JS imports structurally instead of by quote style #176 carries (originally @reneleonhardt's, in perf: Bound repository analysis hot paths #171). Cherry-picked, not rewritten.
  • Test (macos-latest, 1.25)TestRunWatchStartWaitsForChildReadinessFailure:
    runWatchSubcommand(start) error = starting daemon: reading daemon readiness: unexpected end of JSON input.
    The test's child writes readiness with > "$FILE", which truncates the file into existence before printf fills it; waitWatchReadiness treated that first unparseable read as fatal. That is fix(watch): Treat an unparseable readiness file as not-ready-yet #177. Cherry-picked.

Both are git cherry-pick -x of the existing commits, so they drop out cleanly when #176 and #177 land.

The weighting was blind inside a Go package

The open question in the description turned out to have a real answer underneath it, and it was not about the threshold.

codemap resolves Go imports at package level, and BuildFileGraph deliberately drops an import that resolves to more than one file rather than fanning it into one edge per file (that would inflate every hub count in the repo). The consequence: a file inside a multi-file Go package has zero file-level importers by construction. scanner/filegraph.go scored 0. So did every same-package collision, including all six pairs #134 verified by hand. --min-importers 1 then hid the lot, which is why the default printed nothing on this very repository.

What it does now:

  • A file whose language resolves at package granularity is weighted as (files outside its package that import the package) + (the package's other files), and the number is labelled package importers so nobody reads it as a file-level count. Two PRs editing two files of one Go package are editing one compilation unit; that is the collision this command exists to name.
  • A file whose language resolves at file granularity (TypeScript, JavaScript, Python) keeps the file-level count and the plain importers label. FileGraph.Packages is populated for Go and nothing else, which is exactly the set the package hop is correct for — inventing one for TS would inflate its counts.
  • The cross-package term cannot be read off the graph, because the edges it needs are precisely the ones that were dropped. So collide now keeps the scan outcome it was already paying for and counts the raw import strings: ScanForDeps + BuildFileGraphFromOutcome is the same single scan BuildFileGraph was doing internally. A file importing its own package is not counted, and third-party imports are rejected by the fg.Packages lookup.
  • --min-importers now defaults to 0. A file two open PRs both change is a merge-order hazard whatever its weight, and a default that hides hazards answers "no collisions" on a repo full of them. The flag stays for narrowing a long list to the worst of it.

What it actually prints on this repo now

Previously: No shared files at or above --min-importers 1.

OPEN PRs (9): #171, #174, #175, #176, #178, #179, #180, #181, #182

SHARED FILES (each = a merge-order hazard):
  6 PRs   scanner/filegraph.go       73 package importers <- #171, #174, #175, #178, #181, #182
  6 PRs   scanner/rustcargo.go       73 package importers <- #171, #176, #178, #179, #181, #182
  4 PRs   scanner/cargofallback.go   73 package importers <- #171, #178, #181, #182
  4 PRs   scanner/types.go           73 package importers <- #171, #174, #181, #182
  4 PRs   scanner/walker.go          73 package importers <- #171, #179, #181, #182
  3 PRs   config/config.go           38 package importers <- #171, #181, #182
  3 PRs   watch/daemon.go            32 package importers <- #171, #181, #182
  3 PRs   main.go                     6 package importers <- #175, #179, #180
  ... (49 shared files)

PREDICTED COLLIDING PAIRS:
  #171 + #181  ->  47 shared file(s)  top: scanner/filegraph.go (73 package importers)
  #171 + #182  ->  47 shared file(s)  top: scanner/filegraph.go (73 package importers)
  #181 + #182  ->  47 shared file(s)  top: scanner/filegraph.go (73 package importers)
  #171 + #178  ->   3 shared file(s)  top: scanner/filegraph.go (73 package importers)
  ...
  #175 + #179  ->   2 shared file(s)  top: main.go (6 package importers)
  #175 + #180  ->   1 shared file(s)  top: main.go (6 package importers)

Graph coverage: go complete (49 files). TRUST HIGH.

scanner sitting on top is the answer you would expect and the one the file-level count could never give. The 23 importers / 4 importers examples in the description above are pre-weighting and now read package importers for Go paths.

Not allowed to change #134's answer

#134 measured the collision matrix among #124#127 by building six worktrees and merging every pair by hand, and plain file intersection got all six right. Weighting may reorder that list. It may not change it. TestCollideWeightingReordersPairsWithoutChangingThem runs the same four PRs through a file-scoped lookup and a package-scoped one, asserts the pair set and every shared-file count are identical, and asserts the order does move (the top file flips to the heavier file). A dropped pair or an invented one fails it.

Also 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 and a test file weighted with its package rather than given a file-level zero; and a file-resolved language keeping file scope and its plain label. The golden output test is unchanged, since its fixture is file-scoped.

Verification

gofmt clean, go vet ./... clean, go test ./... fully green (17/17 packages), and go test -race green on ., ./mcp and ./scanner — the three the CI failures came from.

Co-Authored-By: Claude Fable 5.1 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t

https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t

r and others added 3 commits September 4, 2026 10:54
…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
@JordanCoin

Copy link
Copy Markdown
Owner Author

Independent review found one ranking defect: a pair's top file came from the shared-file iteration order, which sorts by PR count first, so a pair colliding on a hub could be reported by a low-weight fixture and ranked below a lesser pair. Fixed in 3764134: the top file is now the heaviest shared file (topWeight()), with TestCollidePairsTopFileIsHeaviestNotFirst reproducing the reviewer's case and failing without the change. Pair set and shared-file counts are unchanged, so the #134 matrix test still holds.

Also noted by the reviewer, not a defect: with #174 merged and before #179, codemap's own repo reported go partial because of the Swift testdata fixture, which made every weight unknown; #179 (skip testdata by default) is merged now, so that resolves on the updated branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t

@JordanCoin
JordanCoin merged commit a4b071e into main Sep 4, 2026
12 checks passed
@JordanCoin
JordanCoin deleted the feat/collide-shared-file-hazard branch September 4, 2026 15:22
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.

3 participants