Skip to content

Commit 302eac3

Browse files
mchwangclaude
andauthored
Read app-pinned required checks from the head's check runs (#176) (#195)
* Read app-pinned required checks from the head's check runs (#176) A required check that a ruleset (integration_id) or classic protection (app_id) pins to a GitHub App was matched through check.app.databaseId in gh's statusCheckRollup. gh 2.102.0 never asks GitHub for a run's app, so the check always read as missing, even when it passed. When any required check is pinned to an app, the inspection now also reads the check runs of the exact head the PR read returned, through gh api graphql: each check suite with its app's databaseId and the suite's latest run of each name. A pinned check matches only runs of its name from a suite of its app; a commit status, or a run whose suite GitHub names no app for, never matches it. Checks any app may report are still read from the rollup alone. The read fails closed and keeps its reason: a gh failure, errors or malformed fields, another commit, a suite with more than 100 runs, more than 1,000 suites, or suites that change between pages fail the inspection; a deadline keeps the inspection's timeout. The test fakes no longer give rollup entries an app, and answer the new read in the shape gh 2.102.0 prints. The vitejs/vite and sveltejs/svelte recordings were made again at the same heads with the new read: vite's three checks pinned to GitHub Actions now read as passing, and svelte's pinned check reads as missing because GitHub names no app for its suite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Ask GitHub for each pinned check's runs by app and name (#176 review) Round 1 of the independent review of PR #195 found that the read of every check run on the head, with each suite's app, misread real repositories: - A private app's suite has `app: null` in GraphQL, so its runs never met the check pinned to it (sveltejs/svelte's vite-ecosystem-ci). - A suite with more than 100 runs failed the whole inspection, though the pinned check was elsewhere (microsoft/vscode has one with 101). The read now asks GitHub, per pinned check, for the suites of its app (checkSuites filterBy appId, which also returns a private app's suite) and, in each, the latest run of its name (checkRuns filterBy checkName, checkType LATEST). Each check is one alias; up to 25 go in one call, and a check whose app has more than 100 suites is paged up to 10 pages. The read still fails closed on any overflow left in that narrowed query. The read is skipped when the rules are unknown, so it cannot replace that blocker. When stages stop each other, the inspection now reports the first stage's failure, even when a stopped read then ends with a gh exit of its own. Tests: the fake answers the new query as GitHub does, filtering by app and name; the exit-0 GraphQL error case is labelled defensive. New: several checks per call, more checks than one call holds, paging to exactly the page limit, a pinned check with the rules unknown, and a stop-reason case with a pinned check. The vite and svelte recordings were made again (svelte now reads as passing), and new recordings show a re-run inside one suite (codeboost PR 151), a suite with 101 runs (vscode PR 343099) and the private app's suite in GraphQL and REST. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Match test fakes to real gh and document the pinned-check read (#176 review) Round 2 of the independent review of PR #195 found five low issues: - The fakes in inspectPinned, the stop-reason test and requiredTestState now answer as gh 2.102.0 does: the protection read's 404 as a CommandFailed with GitHub's body on stdout, the branch read with its protection, and a GraphQL error with GitHub's response on stdout. Each notes the fields it leaves out and that the gateway does not read them. - scripts/record-pinned-gh.ts remakes pinned-rerun, pinned-large-suite and private-app-suites, and recorded.ts says which script writes which files and what each holds. Remade today, vscode's suite now has 116 runs (101 earlier); the test checks the new count. - The docs and the gateway's comment say the inspection's deadline, not the 10-page cap, is the real limit on paging: a head with more suites than fit fails closed with the inspection's timeout. - guarded-merge.md's long Gate paragraph is split, and the pinned-check read has its own short section. The T12 evidence names the removed test as removed and points to its replacement; build step 4's status lines are left for the refresh after the parallel PRs merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Describe the stop order runWithInput gives, and name what fakes leave out (#176 review) Round 3 of the independent review of PR #195 found that the code comment, the stop-reason test and guarded-merge.md described a stopped read that exits with its own error after the stop. runWithInput cannot give that: once it sees the abort before gh exits, it rejects with the abort's reason. The real case is a gh that had already exited while its pipes closed after the stop. All three now say so, and the test's fake models that order. It still fails without the first-failure line. The new fakes now use gh 2.102.0's shapes: the 401 and 404 bodies with documentation_url and runWithInput's stdout suffix, the unprotected branch read with its protection (UNPROTECTED_BRANCH), and the abort's reason for a stopped read. Notes name what the fakes leave out. guarded-merge.md says the fresh inspection's deadline fits a few pages and the 12 s display inspection about nine. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Note svelte's changed mergeable, the re-record date and the Checks-permission gap (#176 review round 4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Report a next page without a cursor as an incomplete answer (#176 review round 5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Check the cursor before the page limit, so a 10th page without one is incomplete too (#176 review round 6) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Drop the last rollup app field gh never sends, and scope the first-failure claim (#176 review round 7) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Give the measured per-call time of the pinned-check read (#176 review round 8) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 9c228ad commit 302eac3

15 files changed

Lines changed: 811 additions & 44 deletions

‎docs/designs/codeboost-plan-indexed-review.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2044,7 +2044,7 @@ These `T` IDs are requirement identifiers, not the build-order numbers. Build st
20442044
- Surfaced by: R6 (D7: A)
20452045
- Files: github/checks
20462046
- Verify: recorded-output tests for zero checks, pending then passing, and timeout
2047-
- Evidence (2026-10-20): `test/merge-recorded.test.ts` replays GitHub's real answers, recorded with gh 2.102.0 from this repository by `scripts/record-merge-gh.ts` (`test/fixtures/gh/`). Zero checks: pull request 104, into a branch no rule covers, reads as an empty required set with the rules known. Pending then passing: pull request 183 at one head, read while `test` and `real-docker` ran and after they passed. Through the merge job, the wait sees both checks pending, then merges once they pass; at the time limit with the checks still pending, the job ends `blocked`. For the job, the recorded ruleset is changed in one field, its strict policy, because the repository's ruleset is not strict, so a job against it stops at once for the missing atomic base guard (T7), which is also tested ("ends the job for a person, before waiting, against the repository's real non-strict ruleset"). Zero checks pass a job only with an atomic base guard, and with no required checks only a merge queue gives one (a strict policy guards only a non-empty set). That job case is tested with injected states: a merge queue and no required checks (`test/merge-job.test.ts`, "ends queued when the merge queue accepted the head"). The recordings found two bugs, fixed in PR #183: a ruleset check with no `integration_id` and a ruleset-only branch's 404 "Branch not protected" both made the rules unknown, so a branch whose required checks come only from a ruleset (this repository's `main` among them) blocked merging. The 404 was recorded with a token with admin rights; a token without them may get "Not Found" instead, which still blocks merging as unknown rules. Reviews are not checks: required approving reviews in a ruleset or classic protection are ignored, and only their required checks count (`test/merge.test.ts`, "ignores required reviews in both rule sources and reads only their required checks"). #176 stays open and is tracked under build step 4, not here: a required check pinned to an app always reads as missing, which fails closed (the job blocks at its time limit) rather than merging without the check (`test/merge.test.ts`, "reads a requirement pinned to an app as missing against gh's app-less check runs"; a missing check blocks the job at its time limit: `test/merge-job.test.ts`, "ends approved, merge blocked when required checks do not pass in time"). The PR #183 recordings have no app-pinned check. *Later:* a token without admin rights does get "Not Found", on ruleset-only and classic-protected branches alike; the rules are now read for it, with classic checks from the branch read, replayed from recordings of vitejs/vite, cli/cli and sveltejs/svelte (`guarded-merge.md`). The vite and svelte recordings have app-pinned checks, which read as missing (#176).
2047+
- Evidence (2026-10-20): `test/merge-recorded.test.ts` replays GitHub's real answers, recorded with gh 2.102.0 from this repository by `scripts/record-merge-gh.ts` (`test/fixtures/gh/`). Zero checks: pull request 104, into a branch no rule covers, reads as an empty required set with the rules known. Pending then passing: pull request 183 at one head, read while `test` and `real-docker` ran and after they passed. Through the merge job, the wait sees both checks pending, then merges once they pass; at the time limit with the checks still pending, the job ends `blocked`. For the job, the recorded ruleset is changed in one field, its strict policy, because the repository's ruleset is not strict, so a job against it stops at once for the missing atomic base guard (T7), which is also tested ("ends the job for a person, before waiting, against the repository's real non-strict ruleset"). Zero checks pass a job only with an atomic base guard, and with no required checks only a merge queue gives one (a strict policy guards only a non-empty set). That job case is tested with injected states: a merge queue and no required checks (`test/merge-job.test.ts`, "ends queued when the merge queue accepted the head"). The recordings found two bugs, fixed in PR #183: a ruleset check with no `integration_id` and a ruleset-only branch's 404 "Branch not protected" both made the rules unknown, so a branch whose required checks come only from a ruleset (this repository's `main` among them) blocked merging. The 404 was recorded with a token with admin rights; a token without them may get "Not Found" instead, which still blocks merging as unknown rules. Reviews are not checks: required approving reviews in a ruleset or classic protection are ignored, and only their required checks count (`test/merge.test.ts`, "ignores required reviews in both rule sources and reads only their required checks"). #176 stays open and is tracked under build step 4, not here: a required check pinned to an app always reads as missing, which fails closed (the job blocks at its time limit) rather than merging without the check (`test/merge.test.ts`, "reads a requirement pinned to an app as missing against gh's app-less check runs", a test the #176 fix removed; a missing check blocks the job at its time limit: `test/merge-job.test.ts`, "ends approved, merge blocked when required checks do not pass in time"). The PR #183 recordings have no app-pinned check. *Later:* a token without admin rights does get "Not Found", on ruleset-only and classic-protected branches alike; the rules are now read for it, with classic checks from the branch read, replayed from recordings of vitejs/vite, cli/cli and sveltejs/svelte (`guarded-merge.md`). The vite and svelte recordings have app-pinned checks, which read as missing (#176). *Later (#176):* for a check pinned to an app, the inspection now asks GitHub, through `gh api graphql`, for that app's runs of that name on the head, since gh's rollup names no app; the vite and svelte recordings were made again with that read, and their pinned checks read as passing, svelte's from a private app (`test/merge-recorded.test.ts`; `test/merge.test.ts`, "reads a required check pinned to an app from that app's run on the head, which %s", which replaces the removed test).
20482048
- [x] **T13 (P2, human: ~1 hour / CC: ~5 min)** — runner — Use `node:sqlite` only; enforce the minimum Node version; add a CI warning check
20492049
- Surfaced by: R7 (D8: A)
20502050
- Files: runner/store, web/cli

0 commit comments

Comments
 (0)