Repository navigation
perf(knowledge): skip re-embedding when only a file's bodies changed (#13509) - #13980
Conversation
…13509) CodeIndexer invalidated on a whole-file content hash, so a reformat cost exactly as much as a rewrite — every node in the file re-embedded. Our own PostToolUse hook auto-formats every .py we touch, which makes that churn routine rather than rare. Adds a second-stage signature fingerprint: a hash of the module's interface (function/method signatures, class bases and members, decorators, imports) that deliberately excludes function bodies. When it matches, the nodes are not re-embedded; line ranges are still refreshed, because a stale line makes "go to definition" drift. The module lives beside the three existing hashers rather than becoming a fourth one inside code_indexer (Rule 2), and reuses ASTHasher._args_to_structure for parameter normalisation. None of the existing three answers this question: hash_structural includes the body, hash_normalized erases the identifiers the graph is keyed on, and hash_semantic is built to be equal for different code. Fails open throughout: an unparseable file, a non-Python extension, an import error or a failed line refresh all clear the stored fingerprint and re-analyse. A false negative costs one re-embed; a false positive serves a stale graph indefinitely. Measured over 60 real service files, reformat-only churn: 837 embeddings before, 0 after. The conftest real-load is load-bearing, not test plumbing: under the code_intelligence MagicMock stub the import succeeds and returns a Mock, signature_matches rejects it as a non-str, and every file re-embeds — a green suite for a feature that never ran once.
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
…face (#13509) Review of #13980 found two ways the AST-interface fingerprint reported "unchanged" while the extracted graph had changed. Both were verified against the real extractor, and both were permanent: the skip path ran, the new records were never inserted, and the content hash was updated at the same time, so the file short-circuits before it is ever reconsidered. 1. The extractor emits a node for every definition at any depth — closures, defs under `if TYPE_CHECKING:`, try/except import fallbacks. A top-level AST scan cannot see them, so renaming a nested function left the fingerprint equal while the node set changed. 2. The extractor also emits call edges, which no interface hash can capture. Rewriting a body from os.getenv to os.environ.get changes the persisted edge set with no interface change at all. Fixed by hashing what the extractor actually produced rather than predicting it from the source. Every persisted field is covered except `line`, which is what the skip path goes on to refresh, so the invariant is mechanical instead of argued: if the fingerprint matches, nothing but line numbers moved. It is also free (extraction already runs before this is called) and language-agnostic rather than Python-only. Two consequences worth stating: - parameters, annotations and imports no longer force a re-embed, because the extractor records none of them. That is a larger saving than the old design delivered, and a test pins it so the day the extractor starts recording signatures, it fails loudly. - the refresh path now also rewrites edge call_line and the node document text. The line is inside the embedded text, so leaving the document behind made a record disagree with its own metadata; a stale call_line makes impact analysis point at the wrong line. signature_hasher.py and its tests are replaced rather than kept — they are the same unmerged feature in this PR, corrected, not landed code being removed.
Reworked after review — the fingerprint was wrong at the rootThe review found two cases where the AST-interface fingerprint reported "unchanged" while the extracted graph had changed. I verified both against the real extractor before acting: Both were permanent, not eventual: the skip path ran, the new records were never inserted, and the content hash was updated in the same run, so the file short-circuits before it is ever reconsidered. What changedRather than patch the two cases, I removed their cause. An AST-derived signature is a prediction of what the extractor will emit, and any gap between prediction and reality is silent, permanent staleness. The fingerprint now hashes the extractor's own output — every persisted field except The invariant becomes mechanical instead of argued: if the fingerprint matches, nothing but line numbers moved. It costs nothing (extraction already runs before this is called) and works for every language the indexer supports rather than Python only.
Two consequences worth flagging
VerificationMutation battery, now including both reported bugs as mutants so they cannot regress:
The last two survived the first run and are covered by tests added in response. A/B over 60 real files from On the other review points
|
…13509) Two more findings from re-review, both reproduced before fixing. 1. An edge's target_id/resolved/origin/candidate_count come from resolving the call against every node id known so far, so they are a function of OTHER files. No fingerprint of this file's extraction can predict them. A call to a not-yet-indexed function was stored unresolved, and once the caller's own shape settled it took the skip path forever — staying unresolved permanently and under-reporting find_callers, which is exactly what the impact-analysis work merged just before this was built to prevent. The skip path now recomputes resolution; it costs no embedding. 2. ChromaDB's update() drops unknown ids silently. If a node's embedding failed on an earlier run the record was never created, and every edge from it was skipped too. Patching those ids reported success while they stayed missing — permanently, since the content hash is cached either way. Before this feature any content change re-upserted, so a later edit was a self-healing retry; the skip path had closed that off. It now verifies presence first and falls back to a full index, restoring the retry. The skip path consequently writes full records rather than a `line` patch: it lands a file in the same state a full index would, minus the embedding calls. Node metadata is shared with _upsert_node so the two cannot drift. Also: split _refresh_line_ranges to satisfy the <=30-line rule, and corrected the module docstring, which claimed the fingerprint covered every persisted field. It does not, and cannot — resolution outcomes are now stated as deliberately out of scope and handled by recomputation instead. Tests use a real fake collection rather than a MagicMock: with a mock, "record present" and "record missing" are indistinguishable, which is the exact confusion finding 2 is about.
Second review round — two more findings, both fixedRe-review confirmed the rework closes the original two bugs, then found two more. Both were reproduced before I touched anything, and both are the same class the feature is about: something persisted that the fingerprint could not see. 1. Edge resolution went permanently stale (CRITICAL)An edge's A call to a not-yet-indexed function was stored The skip path now recomputes resolution. It costs no embedding. 2. A failed upsert could never heal (HIGH)ChromaDB's Before this feature, any content change went through What this changed structurallyThe skip path no longer writes a I also corrected the module docstring. It claimed the fingerprint covered every persisted field; it does not and cannot, and resolution outcomes are now stated as deliberately out of scope with the recomputation that handles them.
VerificationThree new mutants, all killed:
The tests now run against a real fake collection rather than a On the measurementEmbedding calls on a reformat wave over 60 real service files are unchanged: 837 before, 0 after. The wall-clock ratio in the A/B fell from 2.6x to 1.2x, because the skip path now re-resolves edges and checks presence while the benchmark's embedder is nearly free. That ratio was always a floor, not a projection — in production the embedding-model calls dominate, and those are what went to zero. |
1 participant
Closes #13509
Thinking Path
CodeIndexerinvalidated on a whole-file content hash, so a reformat cost exactly as much as a rewrite — every node in the file re-embedded. Our own PostToolUse hook auto-formats every.pywe touch, so this churn is routine, not rare.I checked the three existing hashers before writing a fourth (Rule 2). Each is wrong here in an instructive way:
ASTHasher.hash_structuralrecurses into every child, so the body is included — right for Type-1 clone detection, useless for invalidationASTHasher.hash_normalizedreplaces identifiers with placeholders — deliberately blind to the names the graph is keyed onSemanticHasher.hash_semanticis built to be equal for functionally-equivalent-but-different code — the opposite propertyWhat was missing is a hash that is stable under body edits and changes when the interface changes. It lives beside its three siblings in
code_intelligence/fingerprinting/rather than insidecode_indexer, and reusesASTHasher._args_to_structureinstead of re-implementing parameter normalisation.The governing asymmetry: a false negative costs one re-embed, a false positive serves a stale graph indefinitely. Every failure path therefore reads as "re-analyse" — unparseable source, a non-Python extension, an import failure, a failed line refresh, and any unrecognised fingerprint version.
Decision: the conftest real-load is load-bearing, not test plumbing
The top-level conftest stubs
code_intelligenceas a MagicMock. Under that stub the lazy import succeeds and hands back a Mock,signature_matchescorrectly rejects it as a non-str, and every file re-embeds. That is the right fail-open — but it means the stub yields a fully green suite for a feature that never executed once. The three leaves are real-loaded (stdlib-only:ast,hashlib,typing) following the existingcode_generation.diff(#12438) andshared.scoring(#12686) pattern.Two details there are worth flagging, because both were found by breaking them:
_make_pkg_stubforcode_intelligence.fingerprintingwould carry an empty__path__and stop the package resolving its other submodules —code_fingerprinting's real-load died onfingerprinting.detector. The real directory is used instead.import code_intelligence.fingerprinting.X as mwalks the stub's catch-all__getattr__and returns a MagicMock, whilefrom ... import fin the same file resolves throughsys.modulesto the real function — two different objects, so monkeypatching the module silently does nothing.What Changed
code_intelligence/fingerprinting/signature_hasher.py—compute_signature_fingerprinthashes function/method signatures, class bases and members, decorators and imports, and excludes bodies. That exclusion is the feature. Values are version-prefixed (v1:); bumpingSIGNATURE_FINGERPRINT_VERSIONinvalidates every stored fingerprint.services/knowledge/code_indexer.py— stores the fingerprint under a::sig::key in the existing hash cache. On a content-hash miss whose signature still matches, it calls_refresh_line_ranges(a metadata-onlycollection.update) instead of_index_extraction.force=Truebypasses the fast path entirely.conftest.py— real-loadast_normalizer→ast_hasher→signature_hasher, per above.Verification
A/B over 60 real files from
autobot-backend/services/, reformat-only churn:Cold index is unchanged at 837 embeddings. The tests count embedding calls rather than a "skipped" counter, because a counter would pass on an implementation that skipped the work and re-embedded anyway.
Mutation-tested rather than merely re-run — every mutant must kill a test:
signature_unchangedalways False (feature off)signature_unchangedalways True (serves a stale graph).pyfiles get a signature tooforceno longer bypasses the skipBoth survivors were real coverage gaps and are now covered; re-running the battery kills all five. The first is the more interesting one — the JS fixture is not valid Python, so
ast.parseraised and the fail-open path already re-embedded, meaning the extension guard was never exercised. The added test uses JS that also parses as Python (config = {}), which is the only case that separates the guard from the parse failure.Lint: black, isort, flake8 clean on all five files.
Not covered
The A/B measures the indexer in-process against a counting embedder, not a live ChromaDB with a real embedding model — so the embedding-call counts are exact while the wall-clock ratio is a floor, not a projection. Real embedding calls cost far more than the mock's, so the saving on the live path should exceed 2.6x.
Model Used
Claude Opus 5