Skip to content

Stop Excluding the Current PR From the Repo-Wide Quota Signal - #972

Merged
ptr727 merged 1 commit into
developfrom
fix-signal-self-exclusion
Aug 24, 2026
Merged

Stop Excluding the Current PR From the Repo-Wide Quota Signal#972
ptr727 merged 1 commit into
developfrom
fix-signal-self-exclusion

Conversation

@ptr727

@ptr727ptr727 commented Aug 24, 2026

Copy link
Copy Markdown
Owner

Qodo finding on PR #970 (the develop -> main promotion PR, whose head is
develop so this fix lands here first): the prior fix that excluded the
current pull request's own history from quota_signal (PR #969) over-
corrected. A genuine Copilot review on an earlier head of the same
pull request, superseded since by a push, is real evidence about the
account rather than a self-reference to discard, and excluding it could
let a strictly older refusal elsewhere read as the newest activity
instead.

Reverted quota_signal to read the same unfiltered history
copilot_bot_id already does. A refusal on this pull request's own
current head still never reaches this signal: it is caught directly, at
higher priority, before either reading is consulted.

274 tests (2 new), ruff, ruff-format, mypy, and prose_lint all clean.

Summary by CodeRabbit

  • Bug Fixes
    • Improved quota detection by considering prior Copilot activity on the current pull request.
    • Earlier successful reviews can now override older quota refusals when assessing availability.
    • Current pull-request quota refusals continue to receive priority.
  • Tests
    • Expanded coverage for quota detection and pull-request history scenarios.

The prior fix (PR #969) excluded the current pull request's own history
entries from quota_signal entirely, reasoning that its own head is
already read directly. That reasoning covers the current head only: a
genuine Copilot review on an *earlier* head of the same pull request,
superseded since by a push, is real evidence about the account, not a
self-reference to discard. Excluding it could let a strictly older
refusal on some other pull request read as the newest activity instead,
producing exit 47 even though this pull request's own more-recent
history says otherwise.
Reverted to passing quota_signal the same unfiltered history
copilot_bot_id already reads. A refusal on this pull request's own
current head still never reaches this signal at all: it is caught
directly, at higher priority, before either reading is consulted.
Replaced the test that exercised the exclusion with one proving the
scenario it got wrong (a genuine review on this pull request's own
earlier head correctly outranks an older refusal elsewhere) and one
documenting the accepted symmetric case (this pull request's own older
refusal is exactly as much evidence as anyone else's).
274 tests (2 new), ruff, ruff-format, mypy, and prose_lint all clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Aug 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3f417684-3c79-4e33-873a-907b751778cf

📥 Commits

Reviewing files that changed from the base of the PR and between b89d9a5 and b7b5f88.

📒 Files selected for processing (2)
  • scripts/pr_review.py
  • scripts/tests/test_pr_review.py

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

wait now includes the current pull request’s earlier Copilot activity when detecting repository-wide quota exhaustion. Current-head refusals still take precedence. Tests cover genuine reviews, stale refusals, bot identity, and unfiltered history.

Changes

Quota history evaluation

Layer / File(s)Summary
Unfiltered Copilot history contract
scripts/pr_review.py, scripts/tests/test_pr_review.py
copilot_history now documents and tests that callers receive the current pull request’s history for later interpretation.
Current pull request quota evaluation
scripts/pr_review.py, scripts/tests/test_pr_review.py
quota_signal receives complete history. Tests verify that genuine self-reviews clear older refusals and stale self-refusals still trigger the repository-wide signal.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to b7b5f

This localized change restores the quota signal's intended history handling, with the reported test and lint checks passing; no actionable merge-blocking risk remains beyond normal review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title clearly and concisely describes the main change: including the current pull request in the repository-wide quota signal.
Docstring Coverage✅ PassedDocstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-signal-self-exclusion

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Stop excluding current PR history from repo-wide quota_signal

🐞 Bug fix🧪 Tests🕐 20-40 Minutes

Grey Divider

AI Description

• Use unfiltered Copilot history when computing the repo-wide quota refusal signal.
• Clarify that only the current head is special-cased; earlier rounds remain valid evidence.
• Replace/add tests covering earlier-head evidence and stale-refusal signaling behavior.
Diagram

graph TD
A["wait CLI (main)"] --> B{Current head
Copilot activity?} -->|"refusal / activity"| C["Exit via head check
(eg 46/covered)"]
B -->|"silent head"| D["copilot_history()
(repo window)"] --> E["quota_signal()
(unfiltered)" ] --> F["Exit 47 if
latest is refusal"]
D --> G[["GitHub GraphQL API"]]
H["tests: test_pr_review.py"] --> A
subgraph Legend
direction LR
_p["Process"] ~~~ _d{"Decision"} ~~~ _e[["External API"]]
end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Filter only current-head events (by commit OID)
  • ➕ Preserves earlier-round evidence from the same PR while still avoiding true self-reference from the current head
  • ➕ More explicit and resilient if future code reorders the priority checks
  • ➖ Requires reliably mapping history items to a specific head OID (more API fields/parsing)
  • ➖ Higher implementation complexity than relying on existing short-circuit logic
2. Keep PR-number exclusion but widen the history window
  • ➕ Maintains the original ‘exclude self’ intent while reducing chances of missing newer evidence
  • ➕ Could mitigate the specific failure mode if earlier-round evidence falls outside the window less often
  • ➖ Still fundamentally misclassifies earlier rounds on the same PR as irrelevant
  • ➖ Increases API load and still fails when ordering within-window matters

Recommendation: The current approach (use unfiltered history for quota_signal and rely on the higher-priority current-head check to avoid true self-reference) is the best tradeoff: it fixes the incorrect exclusion with minimal complexity and matches how bot-id lookup already treats history. If future behavior ever needs to avoid considering the current head in quota_signal, prefer OID-based filtering rather than PR-number filtering.

Files changed (2) +57 / -31

Bug fix (1) +15 / -16
pr_review.pyUse unfiltered Copilot history for quota_signal and clarify semantics+15/-16

Use unfiltered Copilot history for quota_signal and clarify semantics

• Updates the exit-code 47 documentation to explicitly refer to the current head and to treat earlier rounds of the same PR as valid repo-wide evidence. Removes PR-number filtering when computing quota_signal, passing the same unfiltered history used for bot-id seeding while relying on current-head checks for priority.

scripts/pr_review.py

Tests (1) +42 / -15
test_pr_review.pyReplace self-exclusion test with earlier-head and stale-refusal coverage+42/-15

Replace self-exclusion test with earlier-head and stale-refusal coverage

• Refactors the prior test that validated excluding the current PR from quota_signal into tests that prove the corrected behavior: an earlier-head genuine review on the same PR clears an older refusal elsewhere, and a stale refusal on an earlier head of the same PR still triggers the repo-wide signal when nothing newer answers it. Updates the copilot_history test to assert fetches are unfiltered without demonstrating caller-side exclusion.

scripts/tests/test_pr_review.py

@ptr727
ptr727 merged commit 11c7787 into developAug 24, 2026
8 checks passed
@ptr727
ptr727 deleted the fix-signal-self-exclusion branch August 24, 2026 18:02
@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0)📘 Rule violations (0)📜 Skill insights (3)

Grey Divider


Informational

1. Wrapped sentences in copilot_history docstring 📜 Skill insight✧ Quality
Description
The updated copilot_history() docstring wraps a single sentence across multiple lines, violating
the one-sentence-per-line comment structure rule. This reduces readability and makes future edits
more error-prone.
Code

scripts/pr_review.py[R517-520]

+ Unfiltered, including the caller's own pull request if it appears in the window. No pull+ request is special-cased out: `copilot_bot_id` is happy to read a valid id from anywhere,+ including an earlier round on the very pull request `wait` is running against, and+ `quota_signal` treats an earlier round on that same pull request as real evidence about the
Relevance

● Weak

PR #959 recently rejected this exact one-sentence-per-line reflow request for wrapped docstrings.

PR-#959

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2826725 requires multi-line comments/docstrings to be formatted as one sentence per
line with no mid-sentence wrapping. The modified docstring text breaks sentences across lines (e.g.,
the sentence beginning with Unfiltered, including... continues onto subsequent lines).

scripts/pr_review.py[517-522]
Skill: comment-and-doc-style

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The `copilot_history()` docstring wraps sentences across lines, but comments/docstrings must be structured as one sentence per line with no mid-sentence wraps.
## Issue Context
This docstring was modified in this PR and now includes multi-line sentences.
## Fix Focus Areas
- scripts/pr_review.py[517-522]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Wrapped sentences in new test docstring 📜 Skill insight✧ Quality
Description
The newly expanded test docstring wraps sentences across multiple lines, violating the
one-sentence-per-line comment structure rule. This introduces long wrapped prose blocks that the
rule explicitly disallows.
Code

scripts/tests/test_pr_review.py[R2526-2531]

+ """A pull request number is not what makes an entry irrelevant to the repo-wide signal.- The history entry carries a quota refusal specifically, since ordinary content would- pass this same assertion whether or not the exclusion existed at all: it is the refusal- that a caller-scoped exclusion has to catch and a missing one would leak through as 47.+ An earlier round posted before this pull request's current head is real evidence about+ the account, not a self-reference to discard, and it correctly outranks a strictly+ older refusal elsewhere: excluding this pull request's own history entirely, as an+ earlier fix here did, would have hidden that evidence and let the older refusal read
Relevance

● Weak

PR #959 recently rejected the same formatting request on newly added test docstrings.

PR-#959

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2826725 forbids mid-sentence line wraps in multi-line comments/docstrings. The
updated test docstring breaks sentences across lines (e.g., An earlier round posted before...
continues on the next line), violating the required structure.

scripts/tests/test_pr_review.py[2526-2533]
Skill: comment-and-doc-style

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The docstring for `test_a_genuine_review_on_this_pull_requests_own_earlier_head_clears_the_signal` wraps sentences across lines; multi-line comments/docstrings must be one sentence per line without wrapping.
## Issue Context
This docstring was edited/added in this PR and currently contains mid-sentence line breaks.
## Fix Focus Areas
- scripts/tests/test_pr_review.py[2526-2533]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. quota_signal comment block too long 📜 Skill insight⚙ Maintainability
Description
The new two-line comment above signal = ... is extended rationale prose rather than a concise
constraint, violating the requirement that comments be one line by default. This increases noise in
a tight control-flow area.
Code

scripts/pr_review.py[R2171-2172]

+ # Read from the same, unfiltered history rather than one that drops this pull request's own entries: a genuine review on an earlier head of this same pull request, superseded since by a push, is real evidence about the account and not a self-reference to discard.+ # A refusal on this pull request's own current head still never reaches this signal, since it is caught directly and at higher priority first.
Relevance

● Weak

PR #959 recently rejected this exact one-sentence-per-line reflow request for multi-line rationale
comments.

PR-#959

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2826677 requires comments to be a single line by default and forbids multi-line
elaboration blocks. The added comment spans two long lines of rationale text rather than a minimal
constraint.

scripts/pr_review.py[2171-2172]
Skill: comment-and-doc-style

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The comment explaining the unfiltered history choice is multi-line explanatory prose; comments must be one line by default, with a second line only for genuine constraints the code cannot express.
## Issue Context
This comment was added/updated in this PR next to the quota-signal computation.
## Fix Focus Areas
- scripts/pr_review.py[2171-2173]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 70 rules
✅ Skills: 5 invoked
comment-and-doc-style
dotnet-codestyle
python-codestyle
shell-codestyle
workflow-ci-contract
Review mode: ⚖️ Balanced

Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@ptr727