Skip to content

fix(github): block applies when a required check has not reported#300

Merged
aparajon merged 2 commits into
mainfrom
fix/required-check-absent-blocks
Jun 15, 2026
Merged

fix(github): block applies when a required check has not reported#300
aparajon merged 2 commits into
mainfrom
fix/required-check-absent-blocks

Conversation

@morgo

@morgo morgo commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

When required_checks is configured, the passing-checks apply gate only evaluated the required checks GitHub had already reported on the PR commit. A configured required check that had not reported yet neither failed nor counted as in-progress, so the apply could proceed before that check ever ran — a fail-open gap in a tier-0 safety gate.

This treats every configured required check that is absent from the fetched statuses as a not-yet-reported in-progress blocker, mirroring how a legacy commit status in the EXPECTED state maps to in_progress. The gate now fails closed and the apply-blocked comment names the missing required check(s) so the operator knows what still has to report.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings June 11, 2026 16:07

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.

Pull request overview

This PR closes a fail-open gap in the GitHub “passing checks” apply gate when required_checks is configured: required checks that have not yet reported on the commit are now treated as blocking (in-progress) so apply cannot proceed until they appear.

Changes:

  • Treat configured required checks absent from fetched PR statuses as in-progress blockers and include them in the apply-blocked comment.
  • Add logging to distinguish total in-progress blockers vs. missing-required blockers.
  • Add unit/integration-style tests covering missing required checks behavior and enforcePassingChecks end-to-end comment output.

Reviewed changes

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

File Description
pkg/webhook/apply_gating.go Adds missing-required detection and folds missing required checks into the in-progress apply gate path.
pkg/webhook/apply_gating_test.go Adds coverage for missingRequiredChecks and new enforcePassingChecks scenarios where required checks haven’t reported yet.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@morgo morgo force-pushed the fix/required-check-absent-blocks branch from 7758175 to b26aefd Compare June 12, 2026 14:46
@morgo morgo marked this pull request as ready for review June 12, 2026 14:46
@morgo morgo requested review from Kiran01bm and aparajon as code owners June 12, 2026 14:46
@aparajon

Copy link
Copy Markdown
Collaborator

The fail-closed mechanics are right: missingRequiredChecks only runs after a successful status fetch, the three causes (API/permission failure, failing checks, never-reported checks) keep separate branches with distinct handling, no path converts an API error into a pass, and the REST fetch paginates fully. Name matching is consistent with IsCheckRequired. The safety direction is correct — but the operator-facing message needs a change before merge:

  1. A never-reported check renders as "still running… wait and retry" — for the realistic causes (typo in required_checks, check not configured on the repo, path-filtered workflow), the check will never report, and this advice sends an operator into an indefinite wait. The table's "not reported" row helps, but the remediation text is wrong for those rows. Suggest a conditional paragraph when missing checks exist: "<name> has not reported on this commit. If it never reports, verify the name in required_checks matches the check exactly and that it runs on this PR."

  2. Log the missing names — "apply blocked by in-progress PR checks" with a missing_required_count conflates two causes in one message. When missing checks are the blocker, log them by name so triage doesn't require reproducing the gate.

  3. Nit: a SchemaBot-owned check counts as "reported" here (correct — avoids self-deadlock), but an aggregate-check entry in required_checks is then silently unenforced by this gate. Consider config validation rejecting it explicitly.

Verdict: needs changes for the message; mechanics are sound.


🤖 This review was generated by Claude Code (claude-fable-5) with maintainer approval.

morgo and others added 2 commits June 12, 2026 11:02
When required_checks is configured, the passing-checks apply gate only
evaluated the required checks GitHub had already reported on the commit.
A configured required check that had not reported yet neither failed nor
counted as in-progress, so the apply could proceed before that check ran.

Treat every configured required check absent from the fetched statuses as
a not-yet-reported in-progress blocker, mirroring how a legacy commit
status in the EXPECTED state maps to in_progress. The gate now fails
closed and names the missing required check(s) so the operator knows what
to wait for.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… and logging

A configured required check that GitHub has never reported on the PR
commit was surfaced through the in-progress blockers, so the operator
saw the "checks still running, wait and retry" guidance. For the
realistic causes of a missing report — a typo in required_checks, a
check not configured on the repo, or a path-filtered workflow — waiting
never resolves, sending the operator into an indefinite wait.

Render the never-reported checks in their own section of the
apply-blocked comment with remediation that tells the operator to verify
the name in required_checks matches the check exactly and that it runs
on the PR. In-progress checks keep the wait-and-retry guidance. The
block log names the never-reported required checks so triage does not
require reproducing the gate.

Reject a required_checks entry that names SchemaBot's own aggregate
check (the base name or its environment-scoped form) at config load:
SchemaBot checks are excluded from the passing-checks gate, so such an
entry would be silently unenforced.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@morgo morgo force-pushed the fix/required-check-absent-blocks branch from b26aefd to 27c5118 Compare June 12, 2026 17:12
@aparajon aparajon merged commit 9b0766d into main Jun 15, 2026
41 of 43 checks passed
@aparajon aparajon deleted the fix/required-check-absent-blocks branch June 15, 2026 13:34
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.

3 participants