Skip to content

fix(review): close the contributor open-PR cap's ordering + TOCTOU race#7695

Merged
JSONbored merged 1 commit into
mainfrom
fix/contributor-cap-check-race
Jul 21, 2026
Merged

fix(review): close the contributor open-PR cap's ordering + TOCTOU race#7695
JSONbored merged 1 commit into
mainfrom
fix/contributor-cap-check-race

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

On 2026-07-21 a burst of 6 PRs from one contributor against a cap of 2 exposed two bugs in the per-contributor open-item cap (PRs #7284-#7289): only 2 of the 4 excess PRs were caught, and PRs merged that should have been closed.

Ordering (resource waste): the cap check only ran as the LAST step of the full review pipeline (miner detection, AI slop advisory, linked-issue satisfaction, the full AI code review, CI-completeness verification, public surface publish), so an over-cap PR paid for that entire pipeline before being caught. Confirmed via the real audit log: contributor_cap_ci_cancelled for the burst logged "cancelled 0 of 0 in-flight workflow run(s)" — CI had already finished by the time the close fired. Adds a cheap, CI-independent short-circuit on pull_request opened that closes an over-cap PR before any of that work runs. The existing end-of-pipeline check stays in place as defense-in-depth for state that changes mid-pipeline. A repo with no cap configured, an ignored/exempt author, or a lock-contended pass pays nothing extra — byte-identical to today.

Race (TOCTOU): cap membership was recomputed independently by every sibling PR's own pipeline pass, using whichever "confirmed still open" siblings existed at that exact moment — not one shared snapshot for the burst. A merge could therefore execute off a stale "under cap" conclusion after a sibling's later close made the author over cap. Adds a per-(repo, author) mutex (the existing atomic claim/release transient-lock primitive already used for the per-PR actuation lock) around the cap decision, plus a re-check under that same mutex immediately before a merge actually executes — redirecting to a "denied" outcome instead of merging when a fresh check confirms the author is now over cap, so the next natural re-evaluation plans a close off accurate state.

prReadyForReview's own CI-wait/staleness-cap/dirty-base internals are completely untouched by either fix.

Test plan

  • Full regression sweep across every test file touching the modified functions (queue, queue-2/3/4, agent-actions, agent-action-executor, ci-completion-fork-resume, pr-panel-retrigger-pending-force-review) — 1010/1010 passing
  • New regression tests: early-path close before the expensive pipeline runs, resource-waste guard (no cap configured → zero added cost), ignore-authors parity, new-account label preserved, lock-contended fail-open (both the early path and the pre-merge re-check), pre-merge re-check redirects a would-be merge to denied when now over cap, lock release on both success and denial paths, full-pipeline integration test proving an under-cap contributor still merges normally
  • npm run test:coverage (full, unsharded) — all green, no regressions
  • Coverage scoped to the diff: 100% of changed lines and branches covered across all four modified source files (verified via lcov DA/BRDA against the diff hunks); remaining pre-existing/defensive branch arms carry justified v8 ignore comments mirroring this file's own established convention
  • npx tsc --noEmit clean
  • npm audit --audit-level=moderate clean

On 2026-07-21 a burst of 6 PRs from one contributor against a cap of 2
exposed two bugs in the per-contributor open-item cap: only 2 of the 4
excess PRs were caught, and PRs merged that should have been closed.

Ordering: the cap check only ran as the LAST step of the full review
pipeline (miner detection, AI slop advisory, linked-issue satisfaction, the
full AI code review, CI-completeness verification, public surface publish),
so an over-cap PR paid for that entire pipeline before being caught.
Confirmed live: the contributor_cap_ci_cancelled event for the burst logged
"cancelled 0 of 0 in-flight workflow runs" -- CI had already finished by the
time the close fired. Adds a cheap, CI-independent short-circuit on PR-open
that closes an over-cap PR before any of that work runs, while the existing
end-of-pipeline check stays as defense-in-depth for state that changes mid-
pipeline. A repo with no cap configured, an ignored/exempt author, or a
lock-contended pass pays nothing extra -- byte-identical to today.

Race: cap membership was recomputed independently by every sibling PR's own
pipeline pass, using whichever "confirmed still open" siblings existed at
that moment -- not one shared snapshot for the burst. A merge could
therefore execute off a stale "under cap" conclusion after a sibling's later
close made the author over cap. Adds a per-(repo, author) mutex (the
existing atomic claim/release transient-lock primitive) around the cap
decision, plus a re-check under that same mutex immediately before a merge
actually executes -- redirecting to a "denied" outcome instead of merging
when a fresh check confirms the author is now over cap, so the next natural
re-evaluation plans a close off accurate state.
@JSONbored JSONbored self-assigned this Jul 21, 2026
@JSONbored
JSONbored merged commit ad02897 into main Jul 21, 2026
5 checks passed
@JSONbored
JSONbored deleted the fix/contributor-cap-check-race branch July 21, 2026 08:54
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 21, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.93814% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.52%. Comparing base (e78a03d) to head (dc15d86).
⚠️ Report is 13 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/queue/processors.ts 96.96% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7695      +/-   ##
==========================================
- Coverage   88.52%   88.52%   -0.01%     
==========================================
  Files         725      725              
  Lines       76052    76111      +59     
  Branches    22640    22658      +18     
==========================================
+ Hits        67326    67374      +48     
- Misses       7681     7688       +7     
- Partials     1045     1049       +4     
Flag Coverage Δ
shard-1 36.42% <89.69%> (+0.05%) ⬆️
shard-2 35.63% <23.71%> (-0.03%) ⬇️
shard-3 31.38% <27.83%> (-0.02%) ⬇️
shard-4 38.93% <23.71%> (-0.12%) ⬇️
shard-5 31.86% <22.68%> (-0.01%) ⬇️
shard-6 33.85% <20.61%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/queue/transient-locks.ts 100.00% <100.00%> (ø)
src/services/agent-action-executor.ts 97.23% <100.00%> (+0.10%) ⬆️
src/settings/agent-actions.ts 98.00% <100.00%> (+0.04%) ⬆️
src/queue/processors.ts 95.66% <96.96%> (+0.05%) ⬆️

... and 2 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant