fix(scanner): distinguish never-indexed from zero-importers, and resolve gitignored import targets - #195
Open
nicknsheth-beep wants to merge 2 commits into
Conversation
--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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.codemap --importers node_modules/b.js # before: "No files import node_modules/b.js.\nCoverage: complete"node_modules/b.jsis 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
Coverageis a project-level scan-provenance signal (didast-greprun 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:FileGraphgainsKnownFiles map[string]bool(populated from the same file list used to build the import resolver) and anIndexed(path) boolquery.scanner/types.go:ImportersReportgainsNotIndexed bool(jsonnot_indexed,omitempty), defaulting to false so any caller/test that hand-constructs a bare report literal keeps its existing meaning unchanged.blast_radius.go:buildImportersReportFromGraphsetsNotIndexedfrom!fg.Indexed(file).renderImportersReportCLIprints"<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 againstdocs/OUTPUT-STANDARD.md's four criteria (sourced, precise, bounded, useful) perCONTRIBUTING.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-builtFileGraph) andTestImportersDistinguishesNotIndexedFromZeroImporters(end-to-end via the built binary).Commit 2 —
fix(scanner): resolve a plain relative import whose target is gitignoredProblem
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.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 —
.gitignorealone is sufficient.Root cause
File discovery applies
.gitignorewhen 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/--excludefilters and isn't under one of the scanner's never-source directory names (IgnoredDirs) — both stay absolutely excluded.fileIndexgainedabsRoot/filtersfields to support this; an emptyabsRootdisables the fallback entirely, so every existing test building an index from bareFileInfovalues gets the unchanged old behavior.FileGraph.KnownFilesis backfilled for any import target resolved this way, so it correctly reads as indexed via commit 1'sIndexed/NotIndexedrather than self-contradicting.Note for review: this is the one real behavior change here, not a clear-cut bug fix like the
-Cor 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/IgnoredDirsstill 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--excludeor anIgnoredDirs-named directory stays unresolved) andTestGitignoredExactRelativeImportTargetStillResolves(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 .andgo vet ./...are clean.Related: filing alongside two other independent fixes found during the same investigation (hidden-directory scanning,
-Cscoping) — those have no dependency on this PR.