Skip to content

fix(scanner): distinguish never-indexed from zero-importers, and resolve gitignored import targets - #195

Open
nicknsheth-beep wants to merge 2 commits into
JordanCoin:mainfrom
nicknsheth-beep:fix/coverage-honesty-and-gitignore-resolution
Open

nicknsheth-beep wants to merge 2 commits into
JordanCoin:mainfrom
nicknsheth-beep:fix/coverage-honesty-and-gitignore-resolution

Conversation

@nicknsheth-beep

Copy link
Copy Markdown

This bundles two fixes found in the same investigation. They're in one PR because the second commit's fix depends on the first commit's new type — splitting them would leave the second half unbuildable on its own.

Commit 1 — fix(coverage): distinguish "never indexed" from "zero importers"

Problem

codemap --importers <file> prints "No files import <file>. Coverage: complete" for a file that was never part of the scanned file set at all — indistinguishable from a file that was genuinely scanned and truly has zero importers. An indexing gap reads as a confident answer.

repro/
├── .git/
└── node_modules/
    ├── a.js   # require('./b')
    └── b.js
codemap --importers node_modules/b.js
# before: "No files import node_modules/b.js.\nCoverage: complete"

node_modules/b.js is correctly excluded from indexing, yet the CLI reports "complete" coverage and zero importers as if it had actually looked — identical output to a real zero-importers file.

Root cause

Coverage is a project-level scan-provenance signal (did ast-grep run at all, was the scan authoritative/partial). It says nothing about whether one specific queried file was ever part of the scanned inventory.

Fix

  • scanner/filegraph.go: FileGraph gains KnownFiles map[string]bool (populated from the same file list used to build the import resolver) and an Indexed(path) bool query.
  • scanner/types.go: ImportersReport gains NotIndexed bool (json not_indexed, omitempty), defaulting to false so any caller/test that hand-constructs a bare report literal keeps its existing meaning unchanged.
  • blast_radius.go: buildImportersReportFromGraph sets NotIndexed from !fg.Indexed(file). renderImportersReportCLI prints "<file>: not indexed (under a hidden or excluded folder, or the path doesn't exist)." instead of "No files import <file>" when set, leaving the message unchanged for genuinely-indexed files. Checked this new line against docs/OUTPUT-STANDARD.md's four criteria (sourced, precise, bounded, useful) per CONTRIBUTING.md's review rule — it names the file, states the two concrete reasons plainly, explicitly says it's not a confirmed zero-importers result, and tells the reader what explains it.

Tests

blast_radius_fixes_test.go: TestBuildImportersReportFromGraphMarksNotIndexedFile (unit — both the "not indexed" and "genuinely zero importers" cases against a hand-built FileGraph) and TestImportersDistinguishesNotIndexedFromZeroImporters (end-to-end via the built binary).

Commit 2 — fix(scanner): resolve a plain relative import whose target is gitignored

Problem

A plain, unambiguous relative require('./x') can have its importer edge silently dropped when the target is excluded by .gitignore, even though the importing file is tracked and scanned and the path is completely unambiguous.

repro/
├── .gitignore                    # reviewAdapters/helpers.js
├── quoteRotationService.js       # require('./reviewAdapters/helpers')
└── reviewAdapters/
    └── helpers.js
codemap --importers reviewAdapters/helpers.js
# before: "No files import reviewAdapters/helpers.js.\nCoverage: complete"

I originally suspected a read-only-permission interaction; testing the three combinations independently (gitignore+readonly, readonly-only, gitignore-only) showed permissions alone never break it — .gitignore alone is sufficient.

Root cause

File discovery applies .gitignore when building the project's file inventory, and that same inventory builds the import-resolution index. A tracked source file's plain relative import of a gitignored-but-present sibling has no index entry to resolve against, so the edge silently vanishes.

Fix (scoped narrowly — see note below)

scanner/filegraph.go, tryExactMatch: added a fallback that activates only when the normal in-memory lookup finds zero occurrences of the exact candidate path (never the ambiguous "2+ copies" case, which stays unresolved as before). It checks the candidate against disk, but only when it would also pass the project's --only/--exclude filters and isn't under one of the scanner's never-source directory names (IgnoredDirs) — both stay absolutely excluded. fileIndex gained absRoot/filters fields to support this; an empty absRoot disables the fallback entirely, so every existing test building an index from bare FileInfo values gets the unchanged old behavior. FileGraph.KnownFiles is backfilled for any import target resolved this way, so it correctly reads as indexed via commit 1's Indexed/NotIndexed rather than self-contradicting.

Note for review: this is the one real behavior change here, not a clear-cut bug fix like the -C or hidden-directory issues — codemap has otherwise always treated gitignored files as fully outside the project (file counts, coverage, --diff). I scoped it as tightly as I could (exact-match only, never fuzzy/suffix resolution, explicit --exclude/IgnoredDirs still win outright), but "should a gitignored-but-present file ever be a resolvable import target" is a product call worth your explicit sign-off — happy to narrow further, or to drop this commit and keep just commit 1's honesty fix (report the drop rather than resolve it) if you'd rather not change that invariant at all.

Tests

scanner/filegraph_test.go: TestGitignoreOnlyRecoverable (unit test of the fallback: gitignore-only resolves; the same file blocked by an explicit --exclude or an IgnoredDirs-named directory stays unresolved) and TestGitignoredExactRelativeImportTargetStillResolves (end-to-end, reproducing the fixture above including the read-only bit to confirm it's inert, plus a check that an identical root-level copy of the file doesn't pick up a false importer).

Test suite

go test ./... passes (all 17 packages), gofmt -l . and go vet ./... are clean.

Related: filing alongside two other independent fixes found during the same investigation (hidden-directory scanning, -C scoping) — those have no dependency on this PR.

--importers reported "No files import X. Coverage: complete" for a
file that was never part of the scanned file set at all (e.g. it lives
under an excluded or hidden directory, or the path doesn't exist) —
identical output to a file that was genuinely scanned and truly has no
importers. The project-level coverage status (complete/partial/
unavailable) describes whether the *scan as a whole* succeeded; it says
nothing about whether one particular queried file was in scope, so an
authoritative, complete scan could still turn a blind spot on a single
file into a confidently wrong answer.

FileGraph gains KnownFiles, the set of files actually in the scanned
inventory (populated from the same file list used to build the fuzzy
import resolver), and an Indexed(path) query against it.
ImportersReport gains NotIndexed (JSON: not_indexed, omitempty),
defaulting to false so any report built without this field in mind —
including existing callers/tests that construct FileGraph or
ImportersReport by hand — keeps meaning what it always meant.
buildImportersReportFromGraph sets it from fg.Indexed(file); coverage
status/notes describing the overall scan are left untouched.

renderImportersReportCLI now prints "<file>: not indexed (under a
hidden or excluded folder, or the path doesn't exist)" instead of "No
files import <file>" when NotIndexed is set — quoted here per
CONTRIBUTING's output-standard review rule: sourced (names the file),
precise (no hedge words), bounded (says outright that this isn't a
confirmed zero-importers result), useful (tells the reader which two
things to check). The token-budgeted blast-radius bundle renderer is
unchanged; NotIndexed is still available on the report for any
consumer that wants it (JSON output already surfaces it).

Added TestBuildImportersReportFromGraphMarksNotIndexedFile (unit,
FileGraph with a partial KnownFiles set) and
TestImportersDistinguishesNotIndexedFromZeroImporters (end-to-end via
the built binary: a node_modules/ file reads as not indexed, a real
zero-importer file still reads as a plain negative).
Isolated the reported drop (adgen/reviewAdapters/helpers.js: a plain
require('./reviewAdapters/helpers') showing no importers, while an
identical copy at the repo root showed its real 16) by testing
.gitignore and the file's read-only permission bit independently: a
read-only, tracked file resolved correctly; only adding .gitignore for
the target reproduced the drop. Permissions were not the trigger.

Root cause: ScanFiles applies the project's .gitignore when building
the file inventory, and that same inventory is what the import
resolver's fileIndex is built from. So a file's presence for "does the
project have this source file" and for "can an import land on this
file" were the same gitignore-filtered set — a tracked file's ordinary,
unambiguous require() of an untracked-but-present sibling had nowhere
to resolve to, and silently dropped the edge.

tryExactMatch (scanner/filegraph.go) gains a narrow fallback: when its
normal exact-match lookup misses (the candidate has zero occurrences in
the scanned inventory, as opposed to the ambiguous 2+ case already
handled), it checks the candidate against disk directly. It only does
this when the candidate would otherwise have passed the project's
--only/--exclude filters and isn't under one of the scanner's own
never-source directory names (IgnoredDirs: node_modules, vendor,
testdata, ...) — both stay absolutely excluded, matching the existing
TestFilteredImportResolutionTargets invariant, which is exactly the
test this first caught: an initial version of the fallback resolved a
file blocked by an explicit --exclude filter too, which it must not.
Only .gitignore's blind spot is recovered.

fileIndex gains absRoot and filters fields (both required to enable the
fallback; existing callers that build an index from bare FileInfo
values with no real directory, i.e. nearly every scanner test, pass ""
and get the old behavior unchanged). FileGraph.KnownFiles is backfilled
for any import target that ends up with a resolved importer, so a file
recovered this way correctly reads as indexed rather than contradicting
itself by claiming a resolved edge while also saying "not indexed."

Added TestGitignoreOnlyRecoverable (fast, ast-grep-independent unit
test of tryExactMatch's fallback and its --exclude/IgnoredDirs guards)
and TestGitignoredExactRelativeImportTargetStillResolves (end-to-end
via BuildFileGraph, reproducing Nick's fixture almost exactly,
including the read-only permission bit, to confirm it's inert here).

go test ./... passes, including TestFilteredImportResolutionTargets.
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.

1 participant