fix(scanner): Sort importers so identical scans give identical answers - #178
Merged
Conversation
Importers are appended while iterating analyses, whose order the scanner does not fix, so scanning the same repository twice produced the same importers in a different sequence. Twelve consecutive runs of one binary over one unmodified fixture produced twelve different orderings. That made --importers output shift between identical runs, made diffs of codemap output show changes that were not changes, and left no caller able to assert an exact importer list. Imports are deliberately not sorted. They are appended per file in resolution order, which is already stable across runs (verified: 10/10 identical) and which callers rely on — the CUE resolver returns a selected package before the package it falls back to, and DepsProject sorts its own copy for JSON output regardless. Relates to #153, #172 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo
Ported from @reneleonhardt's open PR #171 so this PR can go green rather than waiting on that one to merge; it no-ops once main carries it. buildRustWorkspaceIndex shadowed its caller's ctx with the cargo-metadata deadline, so an expired deadline failed the whole graph build instead of falling back to the manually derived Rust workspace. Relates to #153, #172 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo
The sort landed in the graph builder, but a state.json written before it still validates: ValidateCachedGraph only checks the builder revision, and that revision did not change. A repo that had a watch daemon running keeps serving map-ordered edge lists from cache until something else invalidates it, so the fix does not reach existing checkouts. Bump graphBuilderRevision to filegraph-v2 so any state file carrying filegraph-v1 fails provenance and is rebuilt. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t
HubFiles ranged over the Importers map and returned the result unsorted. cmd/hooks.go and watch/publication.go pass that slice straight through, and the hook renderer truncates it at maxHubs, so which hubs a hook printed varied run to run over an unchanged graph. Order by non-test importer count descending, then by path, so the truncated head is the most-imported files rather than whichever ones the map yielded first. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t
09d3f4c ported reneleonhardt's fix for buildRustWorkspaceIndex shadowing its caller's ctx with the cargo-metadata deadline, but not the test that proves it. Port TestCargoMetadataDeadlinePreservesFallbackTopology from PR #171 so the fallback topology stays covered on this branch too; it no-ops once #171 merges. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t
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.
Prerequisite for #172's "exact importer list" exit criterion, and for the golden test being written elsewhere. Relates to #153.
What was wrong
Importers are appended while iterating analyses, and nothing fixes that order. Twelve consecutive runs of the same binary over the same unmodified fixture, on
main:So
--importersoutput shifted between identical runs, a diff of codemap output showed changes that weren't changes, and no caller could assert an exact list. After: 12/12 identical.Scope: importers only, deliberately
The first version of this sorted
Importstoo, and that brokeTestCueImportFiltersPackageSelector— which turned out to be encoding a real contract, not an incidental order. The CUE resolver returns a selected package before the package it falls back to, and that sequence is meaningful.So I measured which half was actually unstable, rather than sorting both and adjusting the test to match:
Importsis appended per file in resolution order, which is already stable, andDepsProjectsorts its own copy for JSON output regardless — so--deps --jsonis unchanged by this PR. Only the reverse map needed fixing, and only it is touched.Tests
testdata/deterministic-edges/— four files importing one shared module, named so that alphabetical order differs from every plausible discovery order.TestFileGraphEdgeOrderIsDeterministicbuilds the graph eight times and requires identical output, then requires the result be sorted rather than merely stable, since a caller asserting an exact list needs to know which order it gets.Against unsorted code it fails with:
TestSortEdgesHandlesNilGraphcovers the nil receiver.Also on this branch
state.json:ValidateCachedGraphaccepts any cache whose builder revision matches, and the revision had not changed.graphBuilderRevisionis bumped tofilegraph-v2so pre-sort state files fail provenance and are rebuilt.HubFiles()ordering. It ranged over theImportersmap and returned the slice unsorted;cmd/hooks.goandwatch/publication.gopass it through and the hook renderer truncates atmaxHubs, so which hubs a hook printed still varied. Now ordered by non-test importer count descending, then path.TestCargoMetadataDeadlinePreservesFallbackTopologyis now ported from perf: Bound repository analysis hot paths #171 as well and no-ops once that PR merges.Release note
--importersand the file graph now return importers in sorted order. Identical scans of an unchanged repository produce identical output; previously the order varied between runs. Import lists and--deps --jsonare unaffected.Verification
go vet ./...clean,gofmtclean,go test ./...at the main baseline (only the three known root-environment permission failures). Determinism confirmed end-to-end with a built binary at 12/12 identical on the fixture that previously gave twelve orderings.🤖 Generated with Claude Code
https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo
Generated by Claude Code