Address post-merge Copilot review on #1079: pagination, commit statuses, raw-run route - #1082
Conversation
#1079 merged at 03:36:11Z; Copilot's review posted at 03:46:25Z, ten minutes later, carrying three inline findings and two suppressed ones. All five verified against the API before addressing. - Add --paginate and per_page=100 to the check-runs command. The endpoint returns 30 runs per page by default, so an unfinished run on page 2 yields the same empty result as a finished head, reintroducing the incompleteness the block is about. - Note the endpoint covers check runs only. gh pr checks folds in legacy commit statuses, so swapping one for the other can hide a status context. Adds the commits/<sha>/status query and warns that its combined state reads pending with zero statuses; measured {"state":"pending","n":0} on both ai-config heads checked. - Soften the raw-workflow-run claim, which was too categorical. Run 30781006647, "Running Copilot Code Review", sits at #1056's head cbf39b6, and gh run list --commit does surface it, so the raw-run route is best-effort corroboration rather than blind. - Name the narrower surface where the text read "an absent check run": the run exists and only the rollup omits it. - Reflow the touched case-record paragraph one clause per line. Follow-up to #1079, which closed #1074.
Code reviewReviewed the diff to Verified independently:
No issues found. Checked for bugs, CLAUDE.md/lab-manual compliance (prose style, ASCII punctuation, semantic line breaks), and hallucinated content. VerdictReady for merge. |
|
Claude finished review — View run Review posted successfully to the PR. Summary: PR #1082 makes prose-only changes to VerdictReady for merge. |
|
Ready for merge, on Reviewers. Copilot was requested and never weighed in at this head. Its check run went State, taken from the paginated check-runs endpoint rather than
Behind The conflict sweep had controls, so the zero is not a detector that failed to run: a known-conflicting pair (this PR's superseded branch against |
1 participant
Follow-up to #1079 (which closed #1074).
#1079 was merged at
2026-08-03T03:36:11Z. Copilot's review posted at03:46:25Z, ten minutes later, carrying three inline findings plus two more inside aSuppressed comments (2)block. All five are correct. This PR addresses them againstmain.Fittingly, that review is itself an instance of two things #1079 documents:
gh pr checks 1079returned 8 contexts whilecommits/<sha>/check-runsreturned 9, the extra beingcopilot-pull-request-reviewer; and the findings sat in a collapsed suppression block that a body-summary read would have missed.Findings addressed
1.
--paginateis required (suppressed,fully-clean.md:102).The command shipped without it. The endpoint returns 30 check runs per page by default, so on a head with more than 30 an unfinished run can sit on page 2 while the query returns nothing and reads as an all-clear -- reintroducing, one surface over, the exact incompleteness the block is about. Added
--paginateandper_page=100, plus a paragraph and aDon't:bullet saying why it is load-bearing.2. The endpoint covers check runs only, not legacy commit statuses (inline,
:85).Correct, and the criterion's own older text already says
gh pr checkscovers "check runs (plus legacy commit statuses)". So "read the endpoint instead" could hide a pending or failing status context. Added a paragraph, thecommits/<sha>/statusquery, and aDo:bullet asking for both rather than either alone. Also recorded the trap that its combinedstatereadspendingwhen a repo posts no statuses at all:3. "the raw-workflow-run cross-check finds nothing missing" is too categorical (inline,
:92).Verified, and Copilot's particular is right:
So the omitted check does have a backing Actions run, and a raw-run sweep surfaces it -- under the run's name rather than the check's. Reworded to call the raw-run route indirect and best-effort, carrying that paragraph's own unreliability caveats, rather than blind.
4. "An absent check run" reintroduces the corrected claim (inline,
:695, recurring at:713).Right, and this is the finding I most needed: the run is not absent, only omitted from the rollup, which is the very confusion #1079 exists to remove. Both occurrences now name the narrower surface, and the sentence at
:693was tightened the same way.5. Semantic line breaks in the touched case-record paragraph (suppressed,
:717).Reflowed one clause per line.
check-new-line-breakshad passed on it -- it flags multi-sentence lines and mid-line semicolons, not long single-sentence lines with several clauses -- so this was a real gap that CI does not gate.Checks
sync-codex-skill-wrappers.py(no wrapper changes),validate-skills.py,check-links.py(953 links, 434 files),check-vendored-drift.py,check-memory-file-size.py,test_check_memory_file_size.py(13 passed),markdownlint-cli2@0.22.1(456 files, 0 errors).check-new-line-breaks.pyrun after committing againstorigin/main...HEAD: no lines missing semantic breaks.Branch cut fresh from
origin/mainrather than continuing #1079's branch, since that PR squash-merged and its commits are no longer ancestors ofmain. Diff is 83 insertions / 26 deletions in one file.