Skip to content

Count the Unread Files Rather Than Assuming There Is One - #616

Merged
ptr727 merged 3 commits into
developfrom
fix/partial-coverage-count
Aug 8, 2026
Merged

Count the Unread Files Rather Than Assuming There Is One#616
ptr727 merged 3 commits into
developfrom
fix/partial-coverage-count

Conversation

@ptr727

Copy link
Copy Markdown
Owner

Fixes the finding Copilot raised on the promotion pull request #615, against code #613 shipped.

The defect

The status=COVERAGE_IS_PARTIAL line ended with "taken knowing one file of the diff has no review". The code knows only that the reviewed and changed counts differ. It never computed the difference.

Every partial on record skipped exactly one file, over four pull requests and seven rounds, which is where the wording came from. That is a measurement of those rounds rather than a property the state carries, so a round skipping two files would have been reported as skipping one, on the single line whose whole purpose is to inform a merge decision.

The fix

report_verdict now keeps the coverage line that decided PARTIAL and re-reads its counts, so the message reports the difference the run carries and agrees with the coverage=PARTIAL ratio printed beside it:

... taken knowing 1 of the 3 changed files has no review
... taken knowing 4 of the 9 changed files have no review

Re-reading cannot return None, because PARTIAL is reached only where that same line parsed, and the comment says so rather than leaving a reader to derive it.

Verification

  • python3 scripts/test_pr_review.py, 175 tests, OK. The new case asserts both a one-file and a four-file gap, and it fails against the previous wording, confirmed by stashing the fix and re-running: FAILED (failures=2).
  • The rest of the CI gate list run locally and green: spec/validate.py, test_prose_lint.py, test_repo_gate.py, spec/audit.py --selftest, gh-write-guard.py --selftest, repo_gate.py, and both prose_lint.py invocations.
  • ruff reports 22 findings across these two files, all pre-existing and none inside the changed hunks, and the hub's CI does not run it over scripts/.

Raised on the promotion pull request against the partial-coverage status
line, which told a maintainer that one file of the diff has no review
while the code knew only that the two counts differ.
Every partial on record skipped exactly one file, over four pull requests
and seven rounds, and that is a measurement of those rounds rather than a
property the state carries. A round skipping two would have been reported
as skipping one, on the one line whose whole purpose is to inform a merge
decision.
The line now reads the counts from the coverage statement that decided
PARTIAL, so it reports the difference the run actually carries and agrees
with the ratio printed beside it. The test asserts both a one-file and a
four-file gap, and fails against the previous wording.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI lite review requested due to automatic review settings August 8, 2026 04:31

CopilotAI 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.

Pull request overview

This pull request fixes the status=COVERAGE_IS_PARTIAL wording in scripts/pr_review.py so it reports the actual number of unread files (computed from the parsed coverage line) instead of assuming the gap is always one file, and adds test coverage to lock in the corrected behavior.

Changes:

  • Re-read the coverage line that determined PARTIAL and compute unread = changed - reviewed for accurate messaging.
  • Update the partial-coverage status line to reflect the computed unread-file count with correct singular/plural grammar.
  • Add a regression test covering both a 1-file gap and a 4-file gap.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
scripts/pr_review.pyComputes unread-file count from the parsed coverage line and prints it in the partial-coverage status message.
scripts/test_pr_review.pyAdds a test ensuring the partial-coverage message reports the computed unread-file count (including pluralization).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadscripts/pr_review.py Outdated
The review finding is that read_coverage returns an optional and the
branch indexed it on an invariant the type does not carry. The invariant
holds, PARTIAL being reached only where that same line parsed, and an
invariant a reader has to reconstruct is the one a later change breaks
quietly.
The optional is narrowed, and the unparseable arm says what it does not
know rather than printing a number it could not compute. It keeps exit
42, because a crash on the gate reads as this script being broken rather
than as a round that read part of the diff.
The arm is unreachable by construction, so the test drives report_verdict
with a PARTIAL verdict whose line carries no counts, which is the
invariant violation itself rather than a mock of the reader.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings August 8, 2026 04:37

CopilotAI 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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

scripts/pr_review.py:628

  • This new 7-line comment block is unusually long and repeats closely related points. README.md says "Keep comments concise and only for the non-obvious, and never grow them on edit" (README.md:119); consider condensing this into 1-2 lines that capture the key invariant (derive unread count from the PARTIAL-triggering coverage line, and narrow the optional to avoid a crash).
 # The count comes from the line that decided PARTIAL, not from what past rounds skipped.
# Every partial measured here skipped exactly one file, across seven rounds.
# That is a measurement of those rounds rather than a property the state carries.
# Asserting it would be a claim this script cannot check on the run it prints for.
# None is unreachable while PARTIAL is set only where this same line parsed.
# It is narrowed rather than indexed, since the alternative is a crash on the gate's path.
# A crash there reads as this script being broken rather than as a round that read part.

scripts/pr_review.py:632

  • In the defensive counts is None path, the message currently reads "taken knowing how much of the diff went unread, which could not be re-read...", which is self-contradictory. Rephrase the gap string so it reads coherently with the surrounding "taken knowing ..." clause.
 if counts is None:
gap = 'how much of the diff went unread, which could not be re-read from that line'
else:

Two suppressed findings, both correct.
The comment had grown to seven lines restating one invariant, against the
README rule that a comment stays concise and is never grown on edit. It
is two lines now, saying where the count comes from and why the optional
is narrowed, which is the whole of what a reader cannot see from the
code.
The withheld-count message contradicted the clause it completes, reading
as taken knowing how much went unread, which could not be re-read. It now
says the counts could not be re-read and the total is unknown, which is
what that arm actually knows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CopilotAI review requested due to automatic review settings August 8, 2026 04:41
@ptr727

Copy link
Copy Markdown
OwnerAuthor

Answering both round 2 suppressed findings here, since a suppressed comment has no thread to resolve. Both accepted, both fixed in 5a7a6aa.

This new 7-line comment block is unusually long and repeats closely related points. README.md says "Keep comments concise and only for the non-obvious, and never grow them on edit" (README.md:119); consider condensing this into 1-2 lines that capture the key invariant (derive unread count from the PARTIAL-triggering coverage line, and narrow the optional to avoid a crash).

Correct, and the rule cited is the one it broke. The block reached seven lines by accumulating a line per review round rather than by having seven things to say, which is the growing-on-edit the rule names.

It is two lines now, saying where the count comes from and why the optional is narrowed. The reasoning that came out of it, that every partial on record skipped exactly one file and that this is a measurement rather than a property of the state, is in the commit message and in this pull request's description, which is where a reader looks for why a change was made rather than what the code does.

In the defensive counts is None path, the message currently reads "taken knowing how much of the diff went unread, which could not be re-read...", which is self-contradictory. Rephrase the gap string so it reads coherently with the surrounding "taken knowing ..." clause.

Correct. Read against the clause it completes, it told a maintainer they knew the thing it went on to say could not be read.

It now completes as "taken knowing that the counts on that line could not be re-read, so the total is unknown", which is what that arm knows. The test asserting this arm still checks that no file count is printed, so a future rewording cannot quietly restore a number the branch never computed.

CopilotAI 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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@ptr727
ptr727 merged commit 08cf819 into developAug 8, 2026
7 checks passed
@ptr727
ptr727 deleted the fix/partial-coverage-count branch August 8, 2026 04:44
ptr727 added a commit that referenced this pull request Aug 8, 2026
…-coverage remedy (#615)
Promotes `develop` to `main`, carrying five merged pull requests.
## What is being promoted
- **[#617](#617
`8515666`, the branch rename written `-M` rather than `--move --force`,
which git gained later, checked against git's own option tables at
`v1.5.4` and `v2.0.0`.
- **[#616](#616
`08cf819`, the partial-coverage status line counting the unread files
rather than asserting there is one.
- **[#614](#614
`f39e0c0`, the branch bootstrap step in [`STANDUP.md`](./STANDUP.md).
Section 0B states the sequence that keeps the exploratory standup off
`main` and `develop`, carries the procedure's only `git init` alongside
both of section 0's identity checks, and records why the post-hoc
cleanup is unavailable. It also carries one Disproved Claims entry and
the removal of the shipped backlog cluster.
- **[#613](#613
`676a2bd`, the partial-coverage remedy in `scripts/pr_review.py` and its
two documentation mirrors, corrected to what the record supports after
measuring 4 pull requests and 7 rounds with 0 recoveries.
- **[#612](#612
`1892d6f`, a Dependabot group bump of four actions.
#616 and #617 both answer findings raised by the review **on this
promotion**. A promotion's head is `develop`, so neither could be fixed
here, and each took its own pull request into `develop` and its own
review rounds before merging.
## Review state
Each of the five was reviewed and merged on its own pull request, so
this promotion carries no unreviewed change. #614 ran 13 rounds, 12
findings accepted and 1 declined and recorded under Disproved Claims.
#616 ran 3 rounds and #617 ran 2, all findings accepted.
## Merge shape
This is a promotion, so it merges as a **merge commit** rather than a
squash, per [`GOVERNANCE.md`](./GOVERNANCE.md) "Branching Model". Its
head is `develop` itself, so it must **not** be merged with
`--delete-branch`.
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.

2 participants

@ptr727