fix(review): close two gaps in the per-repo dual-AI combine override - #2695
Conversation
|
Caution 🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥 🛑 Gittensory review result - fixes requiredReview updated: 2026-07-03 07:09:07 UTC
🛑 Suggested Action - Manual Review
Review summary Blockers
Nits — 4 non-blocking
Why this is blocked
CI checks failing
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.
|
d40449f to
c3d9509
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2695 +/- ##
=======================================
Coverage 96.13% 96.13%
=======================================
Files 241 241
Lines 27033 27048 +15
Branches 9822 9832 +10
=======================================
+ Hits 25988 26003 +15
Misses 433 433
Partials 612 612
🚀 New features to boost your workflow:
|
The gate's own live AI review flagged that resolveEffectiveAiReviewOnMerge clamped onMerge alone, but a repo could still neuter an operator's either-floor by shrinking gate.aiReview.reviewers or switching to combine: "single" -- either change reduces the number of independent opinions that can trigger a blocker, the same effective loosening onMerge clamping alone was meant to prevent. Added resolveEffectiveAiReviewPlan, which clamps combine/reviewers together with onMerge whenever the operator has an either floor and a repo override would reduce the effective reviewer count. Also closes a cache-staleness gap an independent adversarial review found: the AI-review result cache's fingerprint never included the new per-repo aiReviewCombine/aiReviewOnMerge/aiReviewReviewers overrides, so a same-head-SHA cache hit (a re-delivered webhook or the block-mode re-gate sweep) could replay a stale verdict computed under the old plan after a maintainer changed the override on an already-open PR.
…cache fingerprint The fingerprint collapsed null/undefined/[] to the same value, but resolveEffectiveAiReviewPlan treats an explicit [] as a real repo override (falls through to the built-in default reviewers) while nullish falls through to the operator's own reviewer plan -- a same-SHA cache hit could replay a verdict produced under the other effective plan.
…or's own combine value collapsesToSingleReviewer fell through to the operator's own combine setting via repoOverride.combine ?? operatorPlan?.combine, so an operator plan that itself sets combine: "single" reported clamped: true on every call even with no repo override at all. Now requires the repo to have actually set combine: "single" itself. Addresses gate-review findings on #2695.
c3d9509 to
9e30a31
Compare
|
Fixed the blocker: `collapsesToSingleReviewer` computed its combine value via `repoOverride.combine ?? operatorPlan?.combine`, so an operator plan that itself sets `combine: "single"` (with no repo override at all) fell through to that same value and reported `clamped: true` on every single call, incrementing the clamp metric for no reason — there was nothing for a repo to have bypassed. Now requires `repoOverride.combine === "single"` directly, i.e. an actual repo-authored override. Added a regression test for the operator's-own-single-combine case (asserts `clamped: false`) and a companion test confirming the reviewer-count clamp still correctly fires when a repo does try to shrink the reviewer list under that same operator plan. Verified by temporarily reverting the fix and confirming the new test fails exactly as expected, then restoring it. |
Summary
Two confirmed bugs in the already-merged #2677 (issue #2567):
1. The operator's onMerge floor could be bypassed via unclamped combine/reviewers. Flagged by gittensory's own live AI review on the original PR:
resolveEffectiveAiReviewOnMergeclampedonMergealone, so an operator plan of{ combine: "synthesis", onMerge: "either", reviewers: [A, B] }(two reviewers, either one can flag a blocker) could be effectively neutered by a repo override of{ combine: "single", reviewers: [A] }— reviewer B's vote never happens, without ever touchingonMergedirectly.Added
resolveEffectiveAiReviewPlan, which clampscombine/reviewerstogether withonMerge: when the operator has set aneitherfloor, a repo override that would reduce the effective reviewer count (a shorterreviewerslist or a switch tocombine: "single") is clamped back to the operator's own values, whileonMergestill resolves through the existingresolveEffectiveAiReviewOnMerge.2. The AI-review cache fingerprint didn't include the new per-repo overrides. Found by an independent adversarial review:
aiReviewCacheInputFingerprint'sreviewerPlanfield only reflectsenv.AI_REVIEW_PLAN(the operator's boot config) — neveraiReviewCombine/aiReviewOnMerge/aiReviewReviewers. Since the actual gate-blocking verdict is directly shaped by these, a same-head-SHA cache hit (a re-delivered webhook or the block-mode re-gate sweep — both normal operational triggers) could replay a stale verdict computed under the old plan after a maintainer changes the override on an already-open PR.Added the three fields to
AiReviewCacheInput/aiReviewCacheInputFingerprint, mirroring exactly how the precedent fieldaiReviewCloseConfidenceis already handled there.Addresses gate-review findings on #2677.
Test plan
npm run typechecknpx vitest run test/unit/ai-review.test.ts test/unit/ai-review-cache-input.test.ts test/unit/ai-review-cache.test.ts test/unit/ai-review-advisory.test.ts test/unit/focus-manifest.test.ts test/unit/queue.test.ts— 732/732 pass, including 7 new tests coveringresolveEffectiveAiReviewPlan's clamp/no-clamp cases (no floor, reduced reviewer count, collapse to single, implicit default reviewer count, unaffected tightening, independent onMerge-only clamp) and 1 new test covering the cache fingerprint change for all three new fields