fix(review): cache grounding's full-file-content fetch by (repo, path, head SHA)#4538
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 #4538 +/- ##
=======================================
Coverage 94.04% 94.04%
=======================================
Files 422 422
Lines 37600 37609 +9
Branches 13736 13740 +4
=======================================
+ Hits 35362 35371 +9
Misses 1583 1583
Partials 655 655
🚀 New features to boost your workflow:
|
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review result - approve/merge recommendedReview updated: 2026-07-10 00:11:21 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.
|
…, head SHA) makeGithubFileFetcher re-fetched every changed file's full post-change body from GitHub on every invocation with zero caching. Grounding bypasses the durable ai_review result cache (a dynamic feature), so the only throttle was the 30-minute non-cacheable cooldown -- after which, or on any non-push webhook event (review comments, review submissions, thread resolutions -- none of which change the head SHA), the full multi-file re-fetch repeated from scratch. Adds grounding_file_content_cache, keyed by (repo, path, head_sha) without a pull number (file content at an immutable commit is universal, not PR-specific). Only a genuinely successful fetch is cached -- a transient network/timeout failure stays retryable rather than being treated as a confirmed-permanent condition. Closes #4499.
…t cache Adds direct tests for getCachedGroundingFileContent/putCachedGroundingFileContent's nullish-headSha no-op paths, and for the cache read/write fail-safe .catch() handlers (a throwing DB read/write must never block the underlying file fetch). Documents two genuinely-unreachable-via-the-real-code-path branches (a Drizzle-query-builder-only schema default never exercised by the raw-SQL write path, and readTextWithLimit's defensive-only null return type) with v8 ignore comments explaining why.
0bd0523 to
f1f6d68
Compare
…-wide for confirmed miners (#4546) * fix(review): make submitter-reputation burst/AI-spend defense install-wide for confirmed miners getSubmitterReputation's burst/low-sample thresholds are scoped WHERE project = ? AND submitter = ? -- a single repo. A fleet identity spreading a handful of gate-passing-but-low-value PRs across dozens of repos in one self-hosted install never accumulates enough same-repo sample density to read as "burst" or "low" anywhere, so the reputation defense never fires for it and every one of its submissions burns full paid AI-review spend indefinitely. - New getSubmitterReputationAcrossInstall in submitter-reputation.ts: the identical quality-weighted signal derivation, scoped by review_targets.installation_id instead of project (that column already existed, migrations/0050; this adds the supporting index). - New getEffectiveSubmitterReputation in reputation-wire.ts: the per-repo signal, additionally widened to the install-wide view for a CONFIRMED official Gittensor miner -- but only when the per-repo signal alone doesn't already justify caution, so an ordinary contributor or an already-flagged submitter pays no extra lookup. - Extracted the miner-identity check (shared with #4512) into src/gittensor/miner-detection-cache.ts so both this module and unlinked-issue-guardrail.ts can use it without a circular import through processors.ts. - Wired into both call sites: shouldSkipAiForReputation (the main AI-spend gate) and the vision-review reputation check. Fixes #4513 * fix: renumber migration 0130 -> 0131 (0130 was claimed by #4538, already merged to main) * fix(test): account for getEffectiveSubmitterReputation's miner-identity check in visual-vision tests runVisualVisionForAdvisory now resolves reputation via getEffectiveSubmitterReputation (#4513), which checks confirmed-official-miner identity (a fetch to api.gittensor.io/miners) whenever the submitter's per-repo signal is neutral -- a real, intentional behavior change this test file's existing "no network calls at all" assertions didn't account for. Stub that one identity check to resolve cleanly and assert precisely that no OTHER (BYOK/vision-spend) network call happens, rather than asserting zero fetch calls outright. * fix(db): renumber migration 0131 -> 0133 (0131/0132 claimed by merged PRs) origin/main has since merged #4545 (0131_screenshot_table_gate_matrix.sql) and #4554 (0132_impact_map_query_cache.sql, itself a collision fix); rebase onto current main and take the next free number. * fix(db): renumber migration 0133 -> 0134 (0133 claimed by merged PR #4556) Rebase onto current main and take the next free number now that migrations/0133_screenshot_table_gate_skill_link.sql (from #4556) occupies the number this branch previously took. * fix(db): renumber migration 0134 -> 0139 (0134 has a pre-existing 3-way collision on main) main currently has three DIFFERENT already-merged files at 0134 (#4549/#4558/#4563) -- tracked separately as its own fix (PR #4577, not yet merged). Since that fix already claims through 0138, take 0139 here to avoid colliding with it once it lands; this branch's own db:migrations:check will stay red until #4577 merges (unrelated to this branch's own changes), and will need one more rebase afterward. * test(review): close 2 coverage gaps surfaced by the #4514 rebase-merge getEffectiveSubmitterReputation's getRepository().catch() needed a real read-failure test (added); its isConfirmedOfficialMiner().catch() is unreachable (that function already catches every internal failure point itself) and getSubmitterReputationAcrossInstall's results ?? [] fallback is the same "D1 always populates results" case already v8-ignored elsewhere in this codebase -- both marked accordingly. * fix(review): isolate #4507's reputation-single-read invariant from cross-test miner-cache pollution Several earlier tests in this file cache "contributor" as a confirmed official Gittensor miner (5-min TTL) in the shared per-file D1 instance. That collided with #4513's new install-wide reputation widening, which correctly detects the stale cache and adds a 4th reputation-scan D1 read for a submitter this test never intended to be a miner. Use a submitter login unique to this test instead.
Closes #4499.
Summary
makeGithubFileFetcher(src/review/grounding-wire.ts) re-fetched every changed file's full post-change body from GitHub on every invocation with zero cache/memoization. Grounding is a dynamic feature (bypasses the durableai_reviewresult cache), so the only throttle was the 30-minuteAI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MScooldown — after which, or on any non-push webhook event (review comments, review submissions, thread resolutions — none of which change the head SHA), the full multi-file re-fetch repeated from scratch.grounding_file_content_cache(migration0130), keyed by(repo_full_name, path, head_sha)— deliberately without a pull number, unlikelinked_issue_satisfaction_cache: file content at an immutable git commit is universal, not PR-specific, so two PRs that happen to share a(repo, path, head_sha)triple (e.g. a cherry-pick) correctly share one cached row.GITTENSORY_REVIEW_GROUNDINGdefaults off on hosted, on in.env.selfhost.example— self-host deployments are the population most exposed to this bug.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, re-verified after rebase)grounding-wire.tslocally): a repeat-call invariant test (2 fetches → 1) and a regression test reproducing the exact incident shape (5 cooldown-driven passes on an unchanged head → 5 fetches without the fix, 1 with it). Also added negative-path coverage: a genuinely new head SHA still fetches fresh, and a failed fetch is never cached (stays retryable).npx vitest run test/unit/grounding-wiring.test.ts test/unit/review-grounding.test.ts test/unit/impact-map-grounding.test.ts test/unit/queue.test.ts(830 passed)If any required check was skipped, explain why:
npm run test:coveragenot re-run unsharded locally; this is an additive change (new table + one function's internal caching, no shared-interface changes) with a broad targeted run already covering grounding + the full queue integration suite. Relying on CI for the full gate.Safety