Skip to content

Read app-pinned required checks from the head's check runs (#176) - #195

Merged
mchwang merged 9 commits into
mainfrom
fix/176-app-pinned-checks
Oct 21, 2026
Merged

Read app-pinned required checks from the head's check runs (#176)#195
mchwang merged 9 commits into
mainfrom
fix/176-app-pinned-checks

Conversation

@mchwang

@mchwang mchwang commented Oct 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #176. Part of closing build step 4.

Problem

A required check pinned to a GitHub App (ruleset integration_id, classic app_id) always read as missing: gh pr view --json statusCheckRollup (gh 2.102.0) never returns a check run's app, and the test fake returned an app field real gh does not send.

Change

  • When any required check is pinned (and the rules are known), github/merge.ts asks GitHub for exactly what each pinned check needs, at the exact head, through gh api graphql: checkSuites(filterBy:{appId}) then checkRuns(filterBy:{checkName, checkType:LATEST}), one alias per pinned check, up to 25 per call; only an app with more than 100 suites is paged (up to 10 pages). The app filter also finds a private app's suite (GraphQL shows its app as null). The name filter is exact.
  • Unpinned checks still come from the rollup, with no extra call. A commit status never meets a pinned check here: whether GitHub lets an app's status do so is unverified, so this is a known fail-closed gap.
  • When stages stop each other, the inspection reports the first failure (failed.signal.reason), as the inspect comment says.
  • The read fails closed with its reason (gh's error, or codeboost's own when the answer is malformed, too large or changed between pages): GraphQL errors, malformed or unknown fields, a missing alias, another or no commit, more than 100 latest runs of the pinned name in a suite, more than 10 pages of an app's suites, or suites changing between pages. Its timeout keeps the inspection's own message.
  • Test fakes now return only fields real gh returns, with the version named beside them. Recordings nonadmin-ruleset (vite) and nonadmin-mixed (svelte, whose private-app pinned check now reads success) re-recorded at the same heads. New recordings: private-app-suites (svelte suites via GraphQL and REST), pinned-rerun (a real re-run, LATEST drops the failed run), pinned-large-suite (a vscode suite with more than 100 runs: 116 when last recorded).
  • Docs: guarded-merge.md, merge-handoff.md, design T12 note.

Validation

Validated head: f622c5e (CI and local gates below ran on it).

  • CI on f622c5e: test passed (run 39539638476); real-docker passed (run 39539638522).
  • Full unit suite without the Docker files (vitest run --project parallel) on f622c5e: 2501 passed, 1 skipped, 0 failed.
  • Focused merge suites (merge, merge-recorded, merge-stop-reasons) on f622c5e: 297 passed. Typecheck on f622c5e: pass.
  • Fail-without-fix checks (each at the commit that added the test): removing the first-failure line fails the stop-reasons test (5ec3b0c, re-checked at f622c5e by round 9); removing the unknown-rules skip fails its test (5ec3b0c); the round-1 merge.ts fails 33 tests including the svelte, re-run and vscode recordings (5ec3b0c); the old cursor/page order fails the page-10 test (4f88972).

Historical counts (intermediate checkpoints, superseded by the above): 455 merge-suite tests at 75efb01; 477 at 5ec3b0c, 429d9c2 and 40717e1; full suite 2497 passed at 5ec3b0c.

Not proven: real paging past 100 suites of one app, more than 100 latest runs of one name in a suite, more than 25 pinned checks (fakes only); whether an app's commit status meets a pinned check (fails closed); a token without the Checks permission (fails closed). The re-run and vscode recordings come from repos whose rules pin nothing, so their rule and PR answers are faked.

Review record

  • Self-review (author subagent): done before commit.
  • Independent review round 1 (fresh general-purpose agent, base main / head 75efb01). Findings and outcome (fixed in 5ec3b0c unless noted):
    • M1: private-app runs always read missing (GraphQL app: null). Fixed with the server-side app filter; svelte now reads success.
    • M2: suites with more than 100 runs in the wild failed the whole inspection. Fixed with the name filter; vscode recording.
    • L1: latency inside the 6 s fresh inspection. Query narrowed (vite: ~1.0 s, 2.8 KB). Declined: running it in parallel with the rule reads, because the pinned set is known only after both rule reads, and a parallel stage would add another stop path on every exit for one call's time.
    • L2: rules stage could now reject and break first-stop ordering; read ran with unknown rules. Fixed (skip when unknown; first failure reported); stop-reasons test.
    • L3: doc inaccuracies (codeboost's own errors, commit-status claim, svelte cause). Fixed.
    • L4: an unreal exit-0 GraphQL-error fake; missing cases. Fake labelled defensive; exit-1 shape matches gh; tests added for the page limit, unknown rules and a real LATEST re-run.
  • Independent review round 2 (fresh general-purpose agent, base main / head 5ec3b0c): no critical, high or medium issues; the query checked live against GitHub (exact names, private apps, unknown app/commit, exit-1 errors, LATEST with re-runs, filtered paging, 25-alias cost). Low findings, fixed in 429d9c2:
    1. inspectPinned fakes did not match gh. Now real 404 shape and branch fields; version and omissions noted.
    2. Stop-reasons and requiredTestState fakes left out output silently. Real stdout or a note; the "Something went wrong" text is labelled not reproduced live.
    3. RecordedGh comment wrong for the three new fixtures, and their script uncommitted. New scripts/record-pinned-gh.ts; comment fixed. Re-recording grew the vscode suite from 101 to 116 runs; the test now checks 116.
    4. "Up to 10 pages" cannot fit the 6 s inspection. Kept the cap and documented the inspection deadline as the real limit (it fails closed with the timeout); a cap tuned to one deadline would be fragile under load, and the 12 s display inspection fits more.
    5. Docs: T12 evidence names the replacing test; the long guarded-merge paragraph is split, with a new "Required checks pinned to an app" section. Build step 4 status lines are left for a docs refresh after Show the merge job's step list and let a person cancel the merge alone (D16) #194, Read app-pinned required checks from the head's check runs (#176) #195 and Keep the rebase conflict resolver's trust-read timeout over a later stop (#174) #197 merge.
  • Independent review round 3 (fresh general-purpose agent, base main / head 429d9c2): no critical, high or medium issues; all five recordings reproduced from their scripts; live gh 2.102.0 checks matched. Findings, fixed in 40717e1 (item 3 in this body):
    1. Low: the stop-order comment, docs and fake described an order runWithInput cannot produce. Reworded (gh exited, pipes closed after the stop); fake models that order; the test still fails without the first-failure line.
    2. Low: new fakes left out real gh output silently. Now built like runWithInput (stderr, newline, stdout; bodies with documentation_url), UNPROTECTED_BRANCH from the recording, the abort's reason on timeout; one shared note names gh 2.102.0 and what the fakes leave out.
    3. Low: PR body said 101 runs. Fixed (116 when last recorded).
    4. Nit: page-limit wording now separates the 6 s fresh and 12 s display inspections; "read" → "parsed" in a test comment.
  • Independent review round 4 (fresh general-purpose agent, merge base aced300 / head 40717e1): no critical, high or medium issues; rounds 1–3 verified; live checks (fork suites excluded from the base repository's read, latency 3.3–3.4 s of 6 s). Nits, fixed in 673b9d1 (comment and docs only; merge-recorded passes):
    1. A test comment said the re-recorded files' other answers did not change; svelte's mergeable did. Noted.
    2. The recordings' date in guarded-merge.md omitted the 2026-10-21 re-record. Added.
    3. A token without the Checks permission was not in the "Not checked" list. Added (fails closed).
  • Independent review round 5 (fresh general-purpose agent, merge base aced300 / head 673b9d1): no findings at critical, high, medium or low; rounds 1–4 verified (live: an app with no suites, case-sensitive names). One nit, fixed in 30c3c18: a next page without a cursor was reported with the page-limit reason; it is now "GitHub returned incomplete check runs." (defensive test added). merge, merge-recorded, merge-stop-reasons pass.
  • Independent review round 6 (fresh general-purpose agent, merge base aced300 / head 30c3c18): no findings at critical, high, medium or low; rounds 1–5 verified; live checks repeated. One nit, fixed in 4f88972: on the 10th page a missing cursor still got the page-limit reason. The cursor is now checked first; a page-10 case was added and fails with the old order (checked). The 3 merge test files pass.
  • Independent review round 7 (fresh general-purpose agent, merge base aced300 / head 4f88972): nothing critical, high or medium; rounds 1–6 verified; live: LATEST drops the failed re-run, GraphQL cost 1–25 points per call (worst case about 875 per 30-minute job). Findings, fixed in aa23839:
    1. Low: one fake rollup (the invalid-app-identity test) still returned app, which gh never sends. Removed; no app field is left in any rollup fake.
    2. Nit: guarded-merge.md said the first stage's failure is always reported; the deadline can replace a failure that is not gh's own exit. Scoped. The 3 merge test files pass.
  • Independent review round 8 (fresh general-purpose agent, merge base aced300 / head aa23839): nothing critical, high or medium; rounds 1–7 verified. Findings, fixed:
    1. Low: per-call latency was too optimistic (measured 2–3 s for a full page of 100 suites on cli/cli and next.js). Code comment and guarded-merge.md corrected in f622c5e: about one full page fits a fresh inspection, three or four the display one; it still fails closed.
    2. Low: this body said the full suite was not re-run because only fakes and docs changed, but 30c3c18 and 4f88972 changed code. The full non-Docker suite was re-run at f622c5e (see Validation).
  • Independent review round 9 (fresh general-purpose agent, merge base aced300 / head f622c5e): clean for the code, tests, fakes, recordings and docs; rounds 1–8 verified; live timings support the latency wording. Two nits in this body's record (stale CI state; items not naming their commit), fixed by rewriting the Validation section above (no code change).

Before Copilot (head f622c5e, validated head for this request)

Copilot round 1 (head f622c5e)

Copilot (copilot-pull-request-reviewer): "Approval recommended", no open issues. No change made.

Review-lesson audit (merge gate)

No new rule is needed; each finding is covered by an existing rule or is a one-off.

  • Covered by CLAUDE.md "A fake of an external tool must give … each part of the answer the code reads … as the real tool gives it": round 1 L4 (exit-0 GraphQL error fake), round 2 L1 and L2 (inspectPinned, stop-reasons and requiredTestState fakes), round 3 L2 (fakes leaving out output), round 7 L1 (the last rollup app field). Also the issue's own cause (the app field the merge fake returned).
  • Covered by CLAUDE.md "Treat every behavioural claim … as something to verify": round 1 L3 (docs on codeboost's own errors, the commit-status claim, the svelte cause), round 2 nit 4 and round 8 L1 (page limit and latency), round 3 L1 (a stop order run-with-input cannot produce), round 4 nits (re-record notes), round 7 nit (first-failure claim scoped).
  • Covered by CLAUDE.md "Report focused tests, affected-suite tests, the full suite … as separate gates" and "label cited commits": round 3 L3, round 8 L2, round 9 nits (stale counts and commits in this body).
  • Covered by AGENTS.md "Preserve the original timeout, cancellation, and shutdown reason through every layer": round 1 L2 (the rules stage rejecting and the first stop reason).
  • One-off (GitHub API behaviour, now pinned by recordings and tests): round 1 M1 (GraphQL hides a private app's app; filter on the server instead), round 1 M2 (suites with more than 100 runs in the wild; filter by name), round 1 L1 (latency of the extra call), rounds 5–6 nits (the missing-cursor reason and its order against the page limit).

🤖 Generated with Claude Code

mchwang and others added 3 commits October 20, 2026 18:52
@mchwang @claude
Read app-pinned required checks from the head's check runs (#176)
75efb01 
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>
@mchwang @claude
Ask GitHub for each pinned check's runs by app and name (#176 review)
5ec3b0c 
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>
@mchwang @claude
Match test fakes to real gh and document the pinned-check read (#176 …
429d9c2 
…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>
This was referenced Oct 21, 2026
Open
Merged
mchwang and others added 6 commits October 20, 2026 20:01
@mchwang @claude
Describe the stop order runWithInput gives, and name what fakes leave…
40717e1 
… 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>
@mchwang @claude
Note svelte's changed mergeable, the re-record date and the Checks-pe…
673b9d1 
…rmission gap (#176 review round 4)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mchwang @claude
Report a next page without a cursor as an incomplete answer (#176 rev…
30c3c18 
…iew round 5)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mchwang @claude
Check the cursor before the page limit, so a 10th page without one is…
4f88972 
… incomplete too (#176 review round 6)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mchwang @claude
Drop the last rollup app field gh never sends, and scope the first-fa…
aa23839 
…ilure claim (#176 review round 7)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mchwang @claude
Give the measured per-call time of the pinned-check read (#176 review…
f622c5e 
… round 8)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mchwang
mchwang marked this pull request as ready for review October 21, 2026 04:21
@mchwang
mchwang requested a balanced review from Copilot October 21, 2026 04:21
Copilot AI reviewed Oct 21, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No blocking defects were identified, and regression coverage supports the matching and fail-closed behavior.

No open issues 🎉

What changed in this PR

Fixes #176 by reading app-pinned required checks from the exact PR head, enabling guarded merges when those checks pass.

Changes:

  • Adds filtered, bounded GraphQL reads while preserving fail-closed behavior.
  • Covers app matching, pagination, reruns, timeouts, and failure ordering.
  • Updates recordings, recording scripts, and implementation documentation.
File Description
test/​merge.test.ts Tests pinned-check matching and failure handling.
test/​merge-stop-reasons.test.ts Tests preservation of the first stage failure.
test/​merge-recorded.test.ts Verifies behavior against recorded GitHub responses.
test/​fixtures/​gh/​recorded.ts Clarifies recording provenance.
test/​fixtures/​gh/​private-app-suites.json Records private-app visibility differences.
test/​fixtures/​gh/​pinned-rerun.json Records latest-run filtering after a rerun.
test/​fixtures/​gh/​pinned-large-suite.json Records filtering within a large suite.
test/​fixtures/​gh/​nonadmin-ruleset.json Adds pinned-check responses for Vite.
test/​fixtures/​gh/​nonadmin-mixed.json Adds private-app check responses for Svelte.
scripts/​record-pinned-gh.ts Adds reproducible pinned-check recordings.
scripts/​record-merge-gh.ts Updates the recording scope comment.
github/​merge.ts Reads pinned checks and preserves failure ordering.
docs/​implementation/​merge-handoff.md Documents pinned-check behavior during merge jobs.
docs/​implementation/​guarded-merge.md Explains filtering, limits, and failure behavior.
docs/​designs/​codeboost-plan-indexed-review.md Updates T12 evidence for the fix.

Review effort: Balanced

@mchwang
mchwang merged commit 302eac3 into main Oct 21, 2026
3 checks passed
mchwang added a commit that referenced this pull request Oct 21, 2026
@mchwang @claude
Mark build step 4 done: #174, #176 and the step list are closed (#194 …
891e6c3 
…review round 13)

Merges main (#195, #196, #197) and updates the status lines this PR
rewrites; tests that a held cancel refusal also survives Refresh.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mchwang mchwang mentioned this pull request Oct 21, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

App-pinned required checks always read as missing

2 participants

@mchwang