fix(review): cache impact-map's per-file vector query results#4544
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4544 +/- ##
=======================================
Coverage 94.06% 94.07%
=======================================
Files 425 425
Lines 37774 37794 +20
Branches 13794 13797 +3
=======================================
+ Hits 35533 35553 +20
Misses 1586 1586
Partials 655 655
🚀 New features to boost your workflow:
|
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review result - approve/merge recommendedReview updated: 2026-07-10 01:22:38 UTC
✅ Suggested Action - Approve/Merge
Review summary Nits — 6 non-blocking
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
computeImpactMap issues one retrieveContextWithMetrics call per changed- symbol file (up to MAX_IMPACT_MAP_INPUT_FILES=20), each a real embedding call plus a live vector-index query with no result cache -- only a 60-second cold-index existence check is memoized. Impact-map is a dynamic feature (bypasses the durable ai_review cache), so only the 30-minute non-cacheable retry cooldown throttled repeat computation; once that lapsed, the full up-to-20x embed+query loop re-fired for an unchanged PR. Adds impact_map_query_cache, keyed by (project, repo, fingerprint) where the fingerprint hashes every input that affects the result (queryText, excludePaths, topK, minScore, reranker) -- excludePaths in particular varies per changed file, since each excludes itself. Unlike the grounding file-content cache, this needs a TTL: the underlying vector index can change as new commits get embedded, so an identical query issued later could legitimately have a different correct answer. Matches the same 30-minute cooldown that already throttles how often this computation is re-attempted. Closes #4500.
Restructure the cache-hit branch to a proper if/else (a braceless single-statement if with no else was mis-instrumented by v8's coverage collector) and add a fail-safe test for a throwing cache read.
4071680 to
ca8ea7d
Compare
Closes #4500.
Summary
computeImpactMap(src/review/impact-map.ts) issues oneretrieveContextWithMetricscall per changed-symbol file (up toMAX_IMPACT_MAP_INPUT_FILES=20), each a real embedding-model inference call plus a live vector-index query with no result cache — only a 60-second cold-index existence check is memoized. Impact-map is a dynamic feature (bypasses the durableai_reviewresult cache), so only the 30-minute non-cacheable retry cooldown throttled repeat computation; once that lapsed, the full up-to-20x embed+query loop re-fired for an unchanged PR.impact_map_query_cache(migration0131), keyed by(project, repo, query_fingerprint). The fingerprint hashes every input that affects the result —queryText,excludePaths,topK,minScore,reranker— sinceexcludePathsin particular varies per changed file (each excludes itself);topK/minScore/rerankerare constants for this module's own calls but are hashed rather than assumed, so the fingerprint stays correct if a future caller ever varies them.AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS(30 min) — the same cooldown that already throttles how often this computation is re-attempted.impact-map.tsitself (using the already-injectedRagInfra.storageadapter), not insiderag.ts's sharedretrieveContextWithMetrics— that function has another caller (rag-wire.ts) that issues one call per pass with no amplification problem, so this scopes the fix to the actual amplification site instead of changing shared retrieval behavior for every caller.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
npm run typechecknpm run db:migrations:check/npm run db:schema-drift:check(both clean; migration renumbered 0130→0131 after rebasing past fix(review): grounding re-fetches every changed file's full body every 30 min with zero caching #4499's own 0130, which merged first)impact-map.tslocally): an invariant test (2 embed/query calls → 1) and a regression test reproducing the exact incident shape (5 cooldown-driven passes on an unchanged head → 5 embed/query calls without the fix, 1 with it). Also added: a genuinely different query still fetches fresh (never masked by another file's cache entry), and a cache entry past the TTL is treated as a miss (never serves a possibly-stale answer).npx vitest run test/unit/impact-map.test.ts test/unit/impact-map-grounding.test.ts test/unit/impact-map-processor-wiring.test.ts test/unit/impact-map-wire.test.ts test/unit/impact-map-collapsible.test.ts test/unit/rag.test.ts test/unit/rag-wiring.test.ts(165 passed) — includes rag.ts/rag-wiring.test.ts to confirm the sibling RAG-grounding caller is completely unaffected, since this fix intentionally does not touch shared retrieval code.If any required check was skipped, explain why:
npm run test:coveragenot re-run unsharded locally; this change is scoped to one module (impact-map.ts) plus a new table, with no shared-interface changes. Relying on CI for the full gate.Safety