Skip to content

Flag stale current-head reviews in queue health#727

Open
xyjk0511 wants to merge 1 commit into
ramimbo:mainfrom
xyjk0511:codex/647-679-stale-review-signal
Open

Flag stale current-head reviews in queue health#727
xyjk0511 wants to merge 1 commit into
ramimbo:mainfrom
xyjk0511:codex/647-679-stale-review-signal

Conversation

@xyjk0511
Copy link
Copy Markdown
Contributor

@xyjk0511 xyjk0511 commented May 31, 2026

Bounty #647
Source issue: #679

Summary:

  • add a read-only stale_current_head_reviews queue-health category for latest useful non-author human reviews whose review commit differs from the current PR head;
  • keep bot reviews from satisfying the human current-head freshness signal, including CodeRabbit login-only review authors;
  • include reviewer, review state, current head, latest review commit, and reason in JSON/text/Markdown reports;
  • document the advisory signal in the admin runbook.

Verification:

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/test_pr_queue_health.py -q
  • python -m ruff check scripts/pr_queue_health.py tests/test_pr_queue_health.py
  • python -m ruff format --check scripts/pr_queue_health.py tests/test_pr_queue_health.py
  • python scripts/docs_smoke.py
  • python -m py_compile scripts/pr_queue_health.py
  • git diff --check
  • python scripts/pr_queue_health.py --repo ramimbo/mergework --format json

No auto-labeling, auto-closing, payout, treasury, wallet, ledger, proof, private data, price, bridge, exchange, or cash-out behavior changed.

Summary by CodeRabbit

  • New Features

    • Added detection for human reviews based on outdated PR commits, flagging them as needing renewal against the current version.
  • Documentation

    • Clarified PR queue health rules: how bot reviews are handled and when human reviews are considered fresh relative to the current PR state.
  • Tests

    • Added comprehensive test coverage for stale review detection logic.

Constraint: ramimbo#679 requests read-only queue-health reporting for review commit freshness under live ramimbo#647 scope. Rejected: auto-labeling or subjective review scoring | outside the accepted advisory reporting scope. Confidence: high. Scope-risk: narrow. Directive: Keep this signal advisory and human-review focused; bot reviews must not satisfy current-head freshness. Tested: PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/test_pr_queue_health.py -q; python -m ruff check scripts/pr_queue_health.py tests/test_pr_queue_health.py; python -m ruff format --check scripts/pr_queue_health.py tests/test_pr_queue_health.py; python scripts/docs_smoke.py; python -m py_compile scripts/pr_queue_health.py; git diff --check; python scripts/pr_queue_health.py --repo ramimbo/mergework --format json. Not-tested: full pytest suite.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 31, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d303f7d9-99bf-4d84-8f4e-0a8e0e3e0d82

📥 Commits

Reviewing files that changed from the base of the PR and between 0eb12a3 and caad880.

📒 Files selected for processing (3)
  • docs/admin-runbook.md
  • scripts/pr_queue_health.py
  • tests/test_pr_queue_health.py

📝 Walkthrough

Walkthrough

This PR adds detection and reporting of PRs whose latest non-author human review is stale relative to the PR's current head commit. The implementation queries GitHub for review metadata, identifies the newest useful human review (excluding bot authors), compares its target commit against the current head, and exposes any freshness gaps in the queue health report via new summary counts and detail sections.

Changes

Stale Current-Head Review Detection

Layer / File(s) Summary
Constants and helper functions
scripts/pr_queue_health.py
Introduces USEFUL_REVIEW_STATES and KNOWN_BOT_LOGINS constants; implements get_author_login(), is_bot_author(), get_commit_oid(), get_latest_useful_human_review(), and _stale_review_issue() to extract and normalize review metadata and produce stale-review issue records.
Live data collection from gh
scripts/pr_queue_health.py
Expands gh pr list --json query to fetch headRefOid, author, and reviews fields in addition to existing fields.
Stale review detection in analyze_queue()
scripts/pr_queue_health.py
Extends normalized PR objects with headRefOid, author, and reviews; initializes stale_current_head_reviews accumulator; and computes _stale_review_issue(pr) for each PR to populate findings.
Report generation and formatting
scripts/pr_queue_health.py
Adds stale_current_head_reviews count to report summary and top-level payload; updates has_queue_issues() to treat stale reviews as a queue problem; adds "Stale current-head reviews" sections to text and markdown formatters.
Test coverage for stale review detection
tests/test_pr_queue_health.py
Updates test_pr_queue_health_flags_required_queue_cases() to assert the new summary field; adds three new test cases: one accepting an approved review matching current head, one flagging a stale review with detail validation, and one confirming bot reviews do not suppress stale human review findings.
Runbook documentation
docs/admin-runbook.md
Documents the human-review freshness rule, clarifies bot-review exclusion, and explicitly references PR scoping within the same bounty issue.

Possibly related issues

  • ramimbo/mergework#679: Main changes implement the stale current-head human-review detection that this issue proposes.
  • ramimbo/mergework#693: This PR implements the stale current-head freshness signal that the retrieved proposal wants to consume for review-bounty candidate selection.

Possibly related PRs

  • ramimbo/mergework#413: Both PRs modify Markdown report section entries in format_markdown_report().
  • ramimbo/mergework#324: The main PR extends the queue health tool with new detection logic and report fields that build on the existing report infrastructure from the retrieved PR.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Flag stale current-head reviews in queue health' is concrete and accurately names the primary changed surface—it describes the new stale_current_head_reviews feature introduced across the files.
Description check ✅ Passed The PR description provides a clear summary of changes, verification steps, and scope details tied to Bounty #647 and issue #679. All key sections from the template are addressed or explicitly noted as out of scope.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed PR adds code-review freshness signals to queue health reporting. No investment claims, price claims, cash-out claims, or payout fabrications found in modified files.
Bounty Pr Focus ✅ Passed PR modifies only stated files with focused stale-review detection. No scope creep to excluded systems (auto-label, payout, treasury, ledger, etc.). Complete test coverage and documentation included.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

Copy link
Copy Markdown
Contributor

@Difficult-Burger Difficult-Burger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the stale current-head review freshness signal.

I checked the implementation against #679's acceptance criteria. The new queue-health category stays read-only, compares the latest useful non-author human review commit against the current PR head, ignores author/bot reviews for that human freshness signal, and exposes the finding consistently in JSON, text, Markdown, and --fail-on-issues paths.

Verification run locally from the PR head caad880e4a8e41ecfa928ae78957ce9ca4223d9f in an isolated checkout with the dollar-hunt-mergework conda env:

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/test_pr_queue_health.py -q -> 21 passed
  • python -m ruff check scripts/pr_queue_health.py tests/test_pr_queue_health.py -> All checks passed
  • python -m ruff format --check scripts/pr_queue_health.py tests/test_pr_queue_health.py -> 2 files already formatted
  • python scripts/docs_smoke.py -> docs smoke ok
  • python -m py_compile scripts/pr_queue_health.py -> passed
  • python scripts/pr_queue_health.py --repo ramimbo/mergework --format json -> completed successfully against the live queue; output included stale_current_head_reviews in the summary and top-level payload

No blocking issues found.

Copy link
Copy Markdown
Contributor

@JONASXZB JONASXZB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head caad880e4a8e41ecfa928ae78957ce9ca4223d9f as a non-author.

No blocker found in the stale current-head review queue-health signal. Evidence checked:

  • inspected scripts/pr_queue_health.py and confirmed the new category is read-only reporting only;
  • verified it compares the latest useful non-author human review commit to headRefOid/head oid and reports reviewer, state, current head, and latest review commit;
  • checked bot handling, including explicit bot metadata, [bot] suffixes, and known bot logins, so bot reviews do not mask stale human review freshness;
  • inspected tests covering current-head human reviews, stale human review commits, and current-head bot reviews that should not mask stale human reviews;
  • ran python -m pytest tests/test_pr_queue_health.py -q -> 21 passed;
  • ran python -m ruff check scripts/pr_queue_health.py tests/test_pr_queue_health.py, python -m ruff format --check ..., python scripts/docs_smoke.py, python -m py_compile scripts/pr_queue_health.py, python -m mypy scripts/pr_queue_health.py, and git diff --check origin/main...HEAD;
  • checked a merge-tree against current origin/main; no conflict markers or both-modified conflict diagnostics were present.

Hosted commit status for this head is green (CodeRabbit: success / Review skipped).

Copy link
Copy Markdown
Contributor

@Jorel97 Jorel97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head caad880e4a8e41ecfa928ae78957ce9ca4223d9f as a non-author.

Requesting changes for one bot-classification edge case in the stale current-head review signal.

Evidence checked:

  • Inspected scripts/pr_queue_health.py, tests/test_pr_queue_health.py, and docs/admin-runbook.md.
  • Confirmed the new category is read-only reporting and is wired into JSON/text/Markdown summaries plus --fail-on-issues.
  • Confirmed the intended behavior is to ignore bot reviews, including CodeRabbit, so a current-head bot review does not mask a stale human review.
  • Found a concrete gap in _is_bot_author(): when author includes a false boolean bot flag together with a known bot login such as coderabbitai, the function returns False immediately and never checks KNOWN_BOT_LOGINS.
  • Reproduced the impact with analyze_queue() using a stale human review on commit aaaaaaaaaaaa... followed by a current-head coderabbitai review with is_bot: False; the report produced stale_current_head_reviews == 0, so the stale human review was hidden.
  • The existing regression test covers {"login": "coderabbitai"} without an is_bot key, but not the false-flag + known-login shape.

Validation run locally on the PR head:

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/test_pr_queue_health.py -q -> 21 passed.
  • python scripts/docs_smoke.py -> docs smoke ok.
  • python -m ruff check scripts/pr_queue_health.py tests/test_pr_queue_health.py -> passed.
  • python -m ruff format --check scripts/pr_queue_health.py tests/test_pr_queue_health.py -> 2 files already formatted.
  • git diff --check origin/main...HEAD -> clean.
  • git merge-tree --write-tree origin/main HEAD -> clean tree 9030c41071773044fe6796f826695aa2f7ee4a41.

Suggested fix: keep explicit is_bot: True as bot, but do not return early on is_bot: False; still check __typename, [bot] suffixes, and KNOWN_BOT_LOGINS, with a regression test for {"login": "coderabbitai", "is_bot": False}.

No private data, credentials, wallet material, production mutation, payout/proof/ledger execution, exchange, bridge, cash-out, price behavior, or fabricated payout claims were used or changed.

@MyTH-zyxeon
Copy link
Copy Markdown

Reviewed current head caad880e4a8e41ecfa928ae78957ce9ca4223d9f as a non-author review for the #654 review bounty.

Requesting changes for one bot-classification edge case in the stale current-head review signal.

Evidence checked:

  • Inspected scripts/pr_queue_health.py, tests/test_pr_queue_health.py, and docs/admin-runbook.md.
  • Confirmed the new stale-current-head review category is read-only, appears in JSON/text/Markdown summaries, and uses headRefOid plus non-author useful human reviews to identify stale review freshness.
  • Confirmed the current head is mergeable/CLEAN and Quality, readiness, docs, and image checks is SUCCESS.
  • Reproduced the blocker locally from the PR logic: with {"login":"coderabbitai","is_bot":False}, _is_bot_author() returns False because it returns immediately on any boolean is_bot value and never reaches the known-login fallback. That allows a known bot review to be treated as a useful human review and can hide a stale human review.
  • Confirmed the existing regression covers {"login":"coderabbitai"} without an is_bot key, but not the false-flag shape.

Suggested fix: keep is_bot: True as an immediate bot signal, but do not return early on is_bot: False; still check __typename/type, [bot] suffixes, and KNOWN_BOT_LOGINS, then add a regression case for {"login":"coderabbitai","is_bot":False} following a stale human review.

No private data, credentials, wallet material, production mutation, payout/proof/ledger execution, exchange, bridge, cash-out, live trading, local code mutation, git command, or worktree change was used.

@ramimbo
Copy link
Copy Markdown
Owner

ramimbo commented Jun 2, 2026

Maintainer queue pass 2026-06-02: holding only as a possible future #777 queue-clarity candidate after that bounty finalizes. #647 is exhausted and the latest current-head review requests changes for a bot-classification edge case, so this is not mergeable or payable now. If #777 opens, fix the requested-changes item, retarget to Bounty #777, and get fresh current-head review evidence before maintainer acceptance. No accepted/paid labels now.

@ramimbo ramimbo added the mrwk:needs-info More information needed label Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mrwk:needs-info More information needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants