Skip to content

Hold a Rollup of Only Unreadable Nodes as an Empty One - #3632

Merged
ptr727 merged 5 commits into
developfrom
fix/unreadable-only-rollup-holds
Oct 30, 2026
Merged

ptr727 merged 5 commits into
developfrom
fix/unreadable-only-rollup-holds

Conversation

@ptr727

@ptr727 ptr727 commented Oct 29, 2026 •

Copy link
Copy Markdown
Owner

Summary

check_nodes in scripts/pr_review_checks.py reads a rollup node of an unrecognized GraphQL union member as a node with empty values and an unreadable key. held_checks_open and checks_never_ran tested for an empty rollup with not nodes. So a rollup holding only such nodes was not read as empty. It released a BLOCKED, BEHIND, or DRAFT merge that an empty rollup holds, and under DIRTY it let wait exit 0 rather than 45.

Both readers now count readable checks through the existing checks_tally, so a rollup of only unreadable types is judged as one with no check. The DIRTY release names the unread types. checks_unread now returns each unread typename once, sorted, so that message and the digest's CHECKS PARTIALLY UNREAD line share one form. The wait help, the CHECKS_PENDING status line, and scripts/README.md say the same.

scripts/pr_review_checks.py is at 447 lines against its 450 ceiling.

Refs #3488

Verification

  • A new end-to-end test holds a rollup of only an unreadable node to 30 under BLOCKED, BEHIND, and DRAFT, and exits it 45 under DIRTY with the unread type named. It fails on the unfixed code for all four merges.
  • The checks_never_ran unit test adds a mixed rollup, which still releases, and an unread-only one of two nodes, which names the type once.
  • The full suite ran 3091 tests, OK (skipped=7).
  • The local strict review ran three rounds at the frontier tier. Round one found a stale status line, two stale help paragraphs, a repeated type name, and two sentences over the bound. Round two found the fix's wording problems and a duplicated dedupe. All were fixed. Round three found one, which spent the edit budget and was listed here as open. Splitting the exit-45 help sentence had dropped "Either", so the payload case read as true of every exit 45. It is fixed in 1b86688, which names each case as one of three, and its own recorded pass found nothing.
  • A Copilot round on 3885af8 raised the same help finding, answered by 1b86688. It also found that no test pinned the sorted order. 31a86e7 adds two distinct types in reverse order, and that test fails against an input-order dedupe. Its recorded pass found nothing.

🤖 Generated with Claude Code

ptr727 and others added 3 commits October 29, 2026 16:43
`held_checks_open` and `checks_never_ran` read an empty rollup as `not
nodes`, so a rollup holding only nodes of a type the reader cannot parse
released a BLOCKED, BEHIND, or DRAFT merge and exited 0 under DIRTY.
Both now count readable checks through `checks_tally`, and the DIRTY
release names the unread types. The `wait` help and `scripts/README.md`
say so.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The `wait` help's exit-45 and exit-0 paragraphs and the CHECKS_PENDING
status line now say a rollup of only unreadable types is read as having
no check posted. The DIRTY release names each unread type once, as the
digest does, and the README's exit-45 row is split under the sentence
bound.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`checks_unread` now returns each unread typename once, sorted, so the
DIRTY release and the CHECKS PARTIALLY UNREAD line share one form
rather than each deduping. The `wait` help and status line name the
reader as "this", in present tense, and the exit-45 paragraph is split
under the sentence bound.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings October 29, 2026 23:54
@ptr727

ptr727 commented Oct 29, 2026

Copy link
Copy Markdown
Owner Author

A recorded local strict-review pass covers head 3885af85568941e09ab40a2de01b4812e9622c83, the content this pull request carries at that commit against develop, and it recorded 1 finding.

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Splitting the exit-45 sentence dropped "Either", so the payload case
read as true of every exit 45. The paragraph now names each case as
one of three, the form scripts/README.md uses.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ptr727

ptr727 commented Oct 29, 2026

Copy link
Copy Markdown
Owner Author

A recorded local strict-review pass covers head 1b86688a8c0787d3d01bddf756480b620aee1d89, the content this pull request carries at that commit against develop, and it recorded 0 findings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Two active nit-level issues remain: the exit-code help is ambiguous, and the sorted-order contract lacks a regression test covering multiple typenames.

2 open issues
What changed in this PR

This pull request makes rollups containing only unreadable GraphQL node types behave like empty rollups during held-merge evaluation and wait, while aligning diagnostics, documentation, and regression coverage.

Changes:

  • Counts only readable checks when evaluating rollup state.
  • Deduplicates and sorts unreadable typename reporting.
  • Updates status output, help text, and README documentation.
  • Adds end-to-end and unit coverage for unreadable-only rollups.
File Review findings
tests/​test_pr_review.py [nit] Add coverage asserting that unreadable typenames are deduplicated and sorted, including multiple types in reverse order. (1 vote)
scripts/​README.md [nit] Keep the checks=0/0 documentation consistent by distinguishing unreadable-only rollups from other unreadable cases. (1 vote)
scripts/​pr_review.py [nit] Preserve the first-case qualifier in exit-45 help text. (3 votes)
[nit] Use a grammatical phrase for the unread-check status message. (1 vote)
scripts/​pr_review_checks.py [nit] Add a regression test covering sorted output for multiple distinct unreadable typenames. (2 votes)

Review effort: Lite

Comment thread scripts/pr_review.py Resolved
Comment thread scripts/pr_review_checks.py Resolved
@codecov

codecov Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.63%. Comparing base (15d4c30) to head (31a86e7).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3632   +/-   ##
========================================
  Coverage    81.62%   81.63%           
========================================
  Files           26       26           
  Lines         9078     9081    +3     
========================================
+ Hits          7410     7413    +3     
  Misses        1668     1668           
Flag Coverage Δ
python-3.13 81.63% <100.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Every unread-only case used one typename, so a dedupe that kept input
order would still pass. Two distinct types given in reverse order now
pin the helper's result and the DIRTY message.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ptr727

ptr727 commented Oct 30, 2026

Copy link
Copy Markdown
Owner Author

A recorded local strict-review pass covers head 31a86e71907513cb13fa332fce3418be74fc8ac0, the content this pull request carries at that commit against develop, and it recorded 0 findings.

@ptr727
ptr727 merged commit e15559f into develop Oct 30, 2026
13 checks passed
@ptr727
ptr727 deleted the fix/unreadable-only-rollup-holds branch October 30, 2026 00:43
ptr727 added a commit that referenced this pull request Oct 30, 2026
…ne (#3633)

Promotes the #3488 unread-only rollup hold from `develop` to `main`.

- [#3632](#3632) Holds a
rollup of only unreadable nodes as an empty one, so `wait` holds a
`BLOCKED`, `BEHIND`, or `DRAFT` merge and exits `45` under `DIRTY`
rather than releasing on checks it never read.

Closes #3488

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to 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.

2 participants