Skip to content

feat(scanner): Skip testdata directories by default - #179

Merged
JordanCoin merged 4 commits into
mainfrom
claude/codemap-ignore-testdata
Sep 4, 2026
Merged

feat(scanner): Skip testdata directories by default#179
JordanCoin merged 4 commits into
mainfrom
claude/codemap-ignore-testdata

Conversation

@JordanCoin

@JordanCoin JordanCoin commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Adds testdata to the hardcoded ignore list, alongside vendor and node_modules. Relates to #172.

Why

Fixture repositories under testdata are not the project's source. They are checked-in inputs for tests: scanning them counts fixture files as project files, and a fixture written in a language the file-level edge model does not cover drags the project's reported coverage down with it.

Go's own toolchain takes the same position — go build and the go tool ignore testdata entirely, and have since the beginning — so this matches a convention Go users already have. It is the justification for the default; the coverage effect is the consequence.

An earlier draft of this PR showed a coverage partialcomplete before/after. That measurement came from a Swift fixture added by #174, which is not merged, so it does not reproduce on main — it has been removed rather than left as an unverifiable claim.

What actually changes on this repo

main's tracked tree contains exactly one testdata directory: watch/testdata/, seven shell scripts (run_test.sh, test_mcp.sh, and five under output/). Measured with binaries built from origin/main and from this branch, both scanning a clean export of origin/main:

main branch:  Files: 283   structure output contains  watch/testdata/  (7 files, 17.4KB, all .sh)
this branch:  Files: 276   no testdata node in the tree

So the visible effect today is modest and mostly forward-looking: seven shell fixtures leave the file count, and the bare watch/testdata/ directory node leaves the tree. The value is in what it prevents as fixture directories accumulate — this repo's own milestone work adds them steadily.

Can it be turned off? No — and that is worth being explicit about

IgnoredDirs is a hardcoded map in scanner/walker.go, checked on the fast path before gitignore and before any project config. Only and Exclude in .codemap/config.json are additive filters — they narrow what a scan returns, they cannot widen it — so there is no config key, flag, or gitignore negation that puts testdata back into a scan.

The one escape hatch is scanning the fixture directly as the root, and this PR now guarantees that: IgnoredDirs is no longer applied to the walk root, so codemap testdata/, or codemap . from inside vendor/, scans the directory the user actually named. Before that fix the walker matched the root's own base name and returned Files: 0 — no error, no message, just an empty result. ReadExternalDeps gets the same exemption so a manifest sitting at that root is still read.

Anyone who deliberately keeps project source under a directory named testdata will see those files disappear from counts and from the tree, with no way to opt back in short of renaming the directory or scanning it directly. Flagging it plainly: it is a default-behaviour change with no override.

Tests

  • TestScanSkipsTestdataDirectories — skipped at top level and nested (internal/testdata/nested/).
  • TestTestdataFixtureScansWhenItIsTheRoot — a fixture passed as the scan root still scans, which is how the fixture tests in scanner call BuildFileGraph.
  • TestIgnoredDirNameScansWhenItIsTheRoot — a root literally named testdata, vendor, or node_modules scans its files. Fails on the pre-fix walker with returned [], want [app.go] for all three.
  • TestIgnoredDirRootStillSkipsNestedIgnoredDirs — the root exemption covers the root only; a nested testdata under a testdata root is still skipped.

Also on this branch

Two fixes ported from open PRs so this branch's CI can go green without waiting on them; both no-op once main carries them.

Release note

testdata directories are now skipped by default, matching Go's own toolchain convention and joining vendor and node_modules. Fixture files no longer count as project files or affect a project's reported coverage. Passing such a directory as the scan root still scans it — and now does so for every name on the ignore list, where previously it returned no files.

Verification

go vet ./... clean, gofmt clean, go test ./... fully green on this branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t

Fixture repositories under testdata are not the project's source. Scanning
them inflates file counts, and once a fixture is written in a language the
file-level edge model does not cover, it changes the project's reported
coverage: a three-file Swift fixture is enough to make an entire Go project
report partial.

Go's own toolchain ignores the directory for the same reason, so this
matches the convention users already expect, alongside vendor and
node_modules.

A fixture is still scannable when it is itself the scan root, which is how
fixture tests use them; only testdata encountered during a walk is skipped.

Relates to #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:22

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.

Two failures on this PR, neither from the testdata exclusion — scanner
passed on both legs.

codemap/mcp: the cargo-metadata deadline failed the whole graph build
instead of falling back. Ported from @reneleonhardt's #171.

codemap root: TestRunWatchModeRunDaemonAndWatchStart hit "reading daemon
readiness: unexpected end of JSON input" — the same window #177 fixes, in
a different test than the one that led me to open it. waitWatchReadiness
gave up on the first unparseable read of a file still being written.

Both no-op once main carries them.

Relates to #172

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

Copy link
Copy Markdown
Owner Author

The review findings on this PR's description are correct and both errors are mine. Not pushing while the branch is being patched — recording the acknowledgement.

The before/after in my description cannot be reproduced on main. I wrote:

before:  codemap --deps .  ->  coverage: partial, source: symbol-imports/swift
after:   codemap --deps .  ->  coverage: complete

That partial depends on #174, which is unmerged — it's #174's Swift fixture and #174's symbol-level coverage rule that produce it. Anyone checking against main would find the premise absent. The effect this PR actually has on main today is on the file inventory: testdata paths in the tree drop from 11 to 1. I built that framing while working with #174's changes in my tree and never re-derived it from a clean main, which is precisely the trap of describing behaviour from a working copy rather than from the base.

My "scan a fixture directly by passing it as the root" claim was under-tested. I verified testdata/probe as root, where the root's basename is probe — so IgnoredDirs never sees it. I did not try testdata/ itself as root, where the basename is the ignored name and the scan silently returns zero files. testdata is a plausible working directory in a way vendor isn't, so the pre-existing behaviour matters more here. Making the walker not apply IgnoredDirs to the root directory itself is the right fix, and it should carry a test — my TestTestdataFixtureScansWhenItIsTheRoot passes without covering the case that actually breaks.

The underlying change stands: fixture repositories aren't project source, and Go's toolchain ignores the directory for the same reason. It's the description and the test's coverage that were wrong, not the exclusion.

I'll re-verify the restated body and the root-directory fix against a clean main build once the branch is free, rather than reading the diff and assuming.


Generated by Claude Code

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 and others added 2 commits September 4, 2026 10:47
IgnoredDirs is matched on a directory's base name, and the walk root is
matched along with everything under it. So `codemap testdata/`, or `codemap .`
from inside vendor/ or node_modules/, returned Files: 0 — no error, no
message, just an empty result for the directory the user explicitly named.
Adding testdata to the list widened the set of paths that hit this.

Exempt the walk root from the fast-path check in ScanFiles, and from the same
check in ReadExternalDeps so a manifest at that root is still read. Nested
directories with those names are still skipped, at any depth.

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

3 participants