Skip to content

Read the Reviewer's Own File Table Instead of Claiming It Has None - #625

Merged
ptr727 merged 1 commit into
developfrom
copilot-review-file-table
Aug 8, 2026
Merged

Read the Reviewer's Own File Table Instead of Claiming It Has None#625
ptr727 merged 1 commit into
developfrom
copilot-review-file-table

Conversation

@ptr727

@ptr727 ptr727 commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Closes #623.

Disposition Amends "Find out which file a partial round skips, and why re-requesting never clears it." in the "A Programmatic Reading of a Copilot Review" cluster. #623 was filed after this repository's last TODO.md edit and appeared nowhere in the file, so selection step 1 was false until this change.

What was wrong

The COVERAGE_IS_PARTIAL caveat, scripts/README.md and the runbook all said the reviewer names no file list in these rounds, so which file went unread cannot be read from the API. Measured over 348 Copilot review bodies on the newest 120 pull requests here and 121 on Blog, that is false. 91 bodies carry a | File | Description | table, and every table row in the corpus belongs to one of those tables.

What the measurement says the table is worth

The issue proposed reading a table that names every changed file as evidence that the count is a reporting artifact. It cannot carry that, and the corpus is what says so.

round states names reading
#476 10 of 11 all 11 table contradicts the count
#592 2 of 3 all 3 table contradicts the count
#479 16 of 17 16, omitting GOVERNANCE.md table corroborates the count and names the file
#609 61 of 62 50 of 62 short by 12 where the count is short by 1
#574 33 of 33, full 32, omitting TODO.md a short table on a fully covered round
#606 7 of 7 names GOVENANCE.md a path no diff carries

On Blog every table names exactly the changed set, on all seven partial rounds and on every full round alike. A reading identical under both outcomes discriminates neither, so a full table is reported as corroborating nothing rather than as a miscount. #574 kills the converse, since a short table sits on a round that read everything.

What ships

  • Q_FULL selects the pull request's own files, windowed at 100 with hasNextPage carried, since a path outside a short window reads exactly like a path the reviewer left out and the record holds a pull request of 301 changed files.
  • file_table reads the table, quotations dropped for the reason the coverage line's are.
  • head_table takes it from any round covering the current head and never from a round before a push. Three of the four partials here carry their table on the pre-push round, describing a diff that push replaced, and comparing that against the current changed files would name a file unreviewed on a stale list. Thirteen commits here carry more than one round, and on one of them a round with a table sits beside a round without, so which of the two the verdict reads must not decide whether a table is found.
  • table_against_diff prints one sentence and decides nothing. It names an omitted file only where the table is short by exactly what the counts leave unread and names nothing outside the diff, that second guard existing because Clear the carried half of the prose backlog, and plan its re-vendor #606's typo would otherwise push the real GOVERNANCE.md into the omissions and report it as the file nobody reviewed.
  • Exit 42 is unchanged in every arm, and the caveat keeps its maintainer-decision framing, which is the issue's third suggestion.

Verified against live data

#479       the reviewer's own file table omits exactly the 1 file the counts leave unread, naming GOVERNANCE.md
Blog #60   the reviewer's own file table names all 6 changed files, ... so it corroborates nothing
#476/#592/#609   no round covering this head carries a file table

Gates

228 test_pr_review cases, plus test_prose_lint, test_repo_gate, spec/audit.py --selftest, spec/validate.py, both prose_lint.py invocations, markdownlint-cli2 and editorconfig-checker, all green locally.

Generated with Claude Code

The PARTIAL caveat said the reviewer names no file list, so which file went
unread cannot be read from the API. Measured over 348 review bodies here and
121 on Blog, that is wrong: 91 bodies carry a `| File | Description |` table,
and every table row in the corpus belongs to one.

The table is reported and never read as coverage, because the measurement says
it cannot be. It names the whole changed set on partial and fully covered
rounds alike, including all seven partials on Blog, so a full table is
identical under both outcomes and discriminates neither. One round of the seven
is the exception, stating 16 of 17 and naming 16 while omitting GOVERNANCE.md,
and that is the only evidence on record that the unread file is a real file
rather than an artifact of counting.

`status` therefore prints one sentence beside the counts and takes no verdict
from it. It names an omitted file only where the table is short by exactly what
the counts leave unread and names nothing outside the diff, one round here
naming GOVENANCE.md, a path no diff carries. The comparison is head-scoped like
the counts, so a table from before a push, describing the diff that push
replaced, reports as no table rather than as a stale list of unreviewed files.

Exit 42 is unchanged in every case, and the merge decision stays the
maintainer's.

Fixes #623

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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.

Pull request overview

Updates the pr_review.py partial-coverage caveat and supporting docs/tests to reflect that Copilot reviews sometimes include a per-file summary table, and to report what that table says (without letting it change the coverage verdict).

Changes:

  • Parse the review body’s per-file summary table and compare it to the PR’s own changed-file list, reporting the result alongside COVERAGE_IS_PARTIAL.
  • Extend the GraphQL full query to fetch the PR’s changed-file paths (windowed) and add test coverage for the observed table shapes and edge cases.
  • Amend TODO.md, scripts/README.md, and .github/copilot-instructions.md to remove the incorrect “no file list” claim and document the measured limitations of the table.

Reviewed changes

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

Show a summary per file
File Description
TODO.md Updates the backlog entry with issue #623 and records the corrected measurement/disposition about file tables.
scripts/test_pr_review.py Adds fixtures and a dedicated test suite covering parsing and reporting behavior for the review’s file table.
scripts/README.md Updates documentation for partial-coverage handling to reflect the measured behavior of per-file tables.
scripts/pr_review.py Implements file-table parsing, fetches PR file paths, and reports table-vs-diff comparisons under partial coverage.
.github/copilot-instructions.md Updates the runbook guidance to note that the file table usually cannot identify the unread file and should be treated as a lead only in a narrow shape.

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

@ptr727
ptr727 merged commit a706ddb into develop Aug 8, 2026
7 checks passed
@ptr727
ptr727 deleted the copilot-review-file-table branch August 8, 2026 17:46
ptr727 added a commit that referenced this pull request Aug 8, 2026
One commit, `a706ddb` from #625, promoting the answer to #623 to `main`.

## What is promoted

`scripts/pr_review.py` reads the Copilot review's own `| File |
Description |` table and reports it beside a partial-coverage verdict,
and the docs drop a claim that was false.

- **The removed claim.** "The reviewer names no file list in these
rounds, so which file went unread cannot be read from the API" was
wrong. Measured over 348 review bodies on the newest 120 pull requests
here and 121 on Blog, 91 carry a table and every table row in either
corpus belongs to one.
- **The table decides nothing, and the measurement is why.** It names
the whole changed set on partial and fully covered rounds alike,
including all seven partials on Blog, so it is identical under both
outcomes. One round here states 33 of 33 and names 32, another states 61
of 62 and names 50, and a third names `GOVENANCE.md`, a path no diff
carries.
- **The one arm that locates a file.** #479 states 16 of 17 and names
16, omitting `GOVERNANCE.md`, and that is the only evidence on record
that the unread file is real rather than an artifact of counting.
`status` names an omitted file only in that shape, where the table is
short by exactly what the counts leave unread and names nothing outside
the diff.
- **Head-scoped like the counts.** Three of the four partials here carry
their table on the round before a push, describing the diff that push
replaced, so those report as no table rather than as a stale list of
unreviewed files.

Exit `42` is unchanged in every arm, and the merge decision stays the
maintainer's.

## State

#623 is already closed by hand, since the keyword sat on a pull request
into `develop`. Review on #625 was one round, `coverage=full shapes=ok
threads=0 unresolved=0 suppressed=0 checks=6/6`, and the body was read
rather than the digest trusted.

**Merge as a merge commit, never a squash**, so `develop` and `main`
stay level.

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