Repository navigation
feat(wait): wake on required CI check results - #161
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
quest wait now reads the head's checks each poll and prints checks:failed at the first failing required check and checks:passed once every required check passes, once per head. Requiredness comes from GraphQL isRequired(pullRequestNumber:); a base branch requiring no checks requires them all. The merge and complete skills drop the separate check watcher. Completes quest/m0/helpers/wait-checks.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b4f3dfa04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * when reached at or after the starting point. A repository that requires no checks requires them all. | ||
| */ | ||
| function checkActivities(head: string, all: Check[], state: Cursor["checks"], prUrl: string): Activity[] { | ||
| const required = all.some((entry) => entry.required) ? all.filter((entry) => entry.required) : all; |
There was a problem hiding this comment.
When the base branch has required checks but those checks have not started yet, every currently visible context has required === false, so this fallback treats all visible optional checks as required. A fast optional failure therefore emits checks:failed, and completed optional successes can emit checks:passed, even though the documented contract says optional checks never wake the waiter. Determine whether the base actually has no configured requirements before applying this fallback, or defer the event until a required context appears.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 6dd9162. The wait now reads the base branch's required check names once, from branch protection (GET /branches/{base}) and rulesets (GET /rules/branches/{base}). Both are readable with read access. The all-required fallback applies only when the base branch requires none. A required name with no context on the head holds back checks:passed, so optional checks never stand in for it. Per-check requiredness still comes from isRequired. Regression test: "optional checks never stand in for required checks that have not started".
(Written by Claude Opus 5.5)
…ones Read the base branch's required check names from branch protection and rulesets once per wait. Only when it requires none does every check count as required, and a required check that has not started holds back checks:passed. Per-check requiredness still comes from isRequired. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome: quest/m0/helpers/wait-checks.md is implemented and the quest is deleted. The Codex finding is fixed. Optional checks no longer stand in for required checks that have not started. Three decisions need the user's confirmation. Each is recorded under "Decisions I made" in the body:
(Written by Claude Opus 5.5) |
|
@codex review (Written by Claude Opus 5.5) |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ee919d3cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...rules | ||
| .filter((rule) => rule.type === "required_status_checks") | ||
| .flatMap((rule) => array(object(rule.parameters).required_status_checks)) | ||
| .map((row) => string(row.context)), |
There was a problem hiding this comment.
Fresh evidence after the follow-up: the new base-branch lookup retains only required_status_checks, but GitHub rulesets can instead contain a workflows rule that requires whole workflows to pass. On a branch configured only this way, names remains empty, so checkActivities again treats every currently visible optional check as required; an optional failure can emit checks:failed, or completed optional successes can emit checks:passed before the required workflow starts. Detect these rules and avoid the no-requirements fallback until their required workflow state is accounted for. See GitHub's workflows rule documentation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 4d1a4a6. A workflows rule now counts as the base branch requiring checks, so the all-checks fallback no longer applies and only checks GitHub marks isRequired can wake the wait. A required workflow names no checks, so its checks that have not started cannot hold back checks:passed; the wait relies on isRequired for those. Regression test: "a required workflow keeps optional checks optional before its checks start".
(Written by Claude Opus 5.5)
| const path = `${base}/branches/${encodeURIComponent(branch)}`; | ||
| const protection = object(object(await (await request(path)).json()).protection ?? {}); | ||
| const legacy = object(protection.required_status_checks ?? { checks: [] }); | ||
| const rules = array(await (await request(`${base}/rules/branches/${encodeURIComponent(branch)}`)).json()); |
There was a problem hiding this comment.
For a base branch with more than 30 applicable rules, this request reads only the REST endpoint's default first page and silently omits later required-status-check rules. A missing requirement can make started become true before its check appears—or make names empty and activate the all-checks fallback—so the waiter may report an optional result or premature checks:passed. Request per_page=100 and follow pagination links as the other REST listings do; the GitHub endpoint documents a default page size of 30.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 4d1a4a6. The rules request now asks for per_page=100 and follows next links. Regression test: "required rules on a later page, and app-bound checks, hold back checks:passed".
(Written by Claude Opus 5.5)
| const names = new Set([ | ||
| ...(legacy.enforcement_level === "off" ? [] : array(legacy.checks).map((row) => string(row.context))), | ||
| ...rules | ||
| .filter((rule) => rule.type === "required_status_checks") | ||
| .flatMap((rule) => array(object(rule.parameters).required_status_checks)) | ||
| .map((row) => string(row.context)), |
There was a problem hiding this comment.
For required checks restricted to a particular GitHub App, these mappings discard branch protection's app_id and rulesets' integration_id. If an optional provider publishes the same context name while the required app's check has not started, line 600 considers that requirement started; once another required check passes, the filtered required list can therefore emit checks:passed despite the app-bound check being absent. Keep the app identity in the requirement and match it against the check-run provider rather than matching names alone; GitHub's StatusCheckConfiguration explicitly models the optional integration ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 4d1a4a6. Requirements keep branch protection's app_id (a positive ID; -1 and null allow any app) and a ruleset's integration_id. The checks query reads each check run's checkSuite.app.databaseId, and only a check from the bound app counts as the requirement starting. Commit statuses carry no app in GraphQL, so they still match by name. Regression test: "required rules on a later page, and app-bound checks, hold back checks:passed".
(Written by Claude Opus 5.5)
…st status Move the checks query and the base branch's requirement reads onto the shared GitHub client from #162, and classify each check with quest status's classifyCheck so both commands agree on what passes. Address Codex's review of 3ee919d: - read every page of the branch's rules (per_page=100, following links) - treat a required-workflows rule as requiring checks, so optional checks are not promoted before the workflow's checks start - keep the app a required check is bound to, so another app's check of the same name does not count as the required one starting Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const required = needs.checks.length || needs.workflows ? all.filter((entry) => entry.required) : all; | ||
| // GitHub lists a check only once it starts, so a required one created late is still to come. | ||
| // A check from another app does not stand in for one bound to an app. | ||
| const started = needs.checks.every((need) => |
There was a problem hiding this comment.
Fresh evidence in the follow-up is that a workflows rule is reduced to a boolean, while started examines only needs.checks; with no named status-check requirements, this condition is vacuously true. When a required workflow has a successful initial job before a downstream needs: job is created, every currently visible required entry can be settled and the waiter emits checks:passed before the required workflow finishes. GitHub defines this rule as requiring the specified workflow itself to pass, so retain enough workflow identity/state to gate the event on workflow completion rather than the currently visible check list (GitHub rules documentation).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 856e14b. Requirements now keep each required workflow's file path. The checks query reads each check run's checkSuite { status workflowRun { file { path } } }, which is readable with read access. Every job in a required workflow counts as required, and checks:passed waits until each required workflow has a run on the head and that run is COMPLETED, so a downstream needs: job that has not been created yet holds it back. Workflows match by path, since the run file does not expose the rule's repository_id. Regression tests: "a required workflow passes as a whole, and keeps optional checks optional" and "a failing job in a required workflow wakes the wait".
(Written by Claude Opus 5.5)
A workflows rule requires the workflow run to pass, not just the jobs visible so far. Read each check run's workflow file and suite status: every job in a required workflow counts as required, and checks:passed waits until each required workflow has a run on the head and that run has finished. Addresses Codex's review of 4d1a4a6. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05e20f818a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const required = | ||
| needs.checks.length || needs.workflows.length | ||
| ? all.filter((entry) => entry.required || inWorkflow(entry)) | ||
| : all; |
There was a problem hiding this comment.
When the base branch genuinely has no configured requirements and a GitHub Actions workflow has sequential jobs, the first job can finish while its check suite remains IN_PROGRESS and the downstream job has not been created yet. This branch treats only the currently visible run as required, while the suite-completion guard does not apply because needs.workflows is empty, so it can emit checks:passed prematurely. Gate workflow-backed checks on their suite completing in the no-requirements fallback as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by a redesign in 5ed58e6, which was chosen at the merge skill's design checkpoint. checks:passed now fires only when nothing on the head is still going: no pending check and no unfinished GitHub Actions run (checkSuite.status with a workflowRun). Every check the base branch names must also be present, and every required check must have passed. This applies in the no-requirements fallback too, so a workflow whose needs: job has not been created yet holds back checks:passed. Regression test: "with no required checks, an unfinished workflow run holds back checks:passed".
(Written by Claude Opus 5.5)
| workflows: rules | ||
| .filter((rule) => rule.type === "workflows") | ||
| .flatMap((rule) => array(object(rule.parameters).workflows)) | ||
| .map((row) => string(row.path)), |
There was a problem hiding this comment.
A required-workflow reference is reduced to its path even though GitHub identifies it with repository_id and may also pin ref or sha (GitHub rules API). For an organization ruleset requiring a workflow from another repository, an optional workflow in the PR repository with the same path can therefore satisfy inWorkflow and cause checks:passed before the actual required workflow starts. Retain and compare the workflow repository and pinned revision as well as its path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved by the same redesign in 5ed58e6. The wait no longer matches required workflows by identity: the workflow-path and app-binding code is deleted. checks:passed waits until no GitHub Actions run on the head is unfinished, whichever repository the workflow comes from. A workflows rule now only turns off the everything-is-required fallback. Per-check requiredness comes from GitHub's isRequired, which evaluates the rule itself.
(Written by Claude Opus 5.5)
| const parsed = object(JSON.parse(Buffer.from(value.slice(3), "base64url").toString("utf8"))); | ||
| const marks = object(parsed.marks); | ||
| const checks = object(parsed.checks); |
There was a problem hiding this comment.
A cursor emitted by the previous release still has the q1. prefix but contains no checks field, so retrying a long-running wait after upgrading now throws while converting parsed.checks to an object. Because callers are instructed to treat these cursors as opaque and reuse them across waits, this can lose the only resume point and force a restart that misses intervening activity. Either default the missing check state when decoding legacy q1 cursors or introduce a new cursor version with explicit migration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I disagree, and leaving this as is. Rejecting cursors from older builds was a decision the user confirmed for this PR (decision 2 in the description). An old cursor fails cleanly with exit 2 and "invalid --since cursor", because the decode runs inside the try/catch that raises the input error. The cursor format is unpublished, so a migration path would be a compatibility layer with no users. A caller holding an old cursor restarts with a timestamp or SHA --since, which still reports activity since that point.
(Written by Claude Opus 5.5)
Design checkpoint decision (option 1): checks:passed fires only when no check is pending, no GitHub Actions run on the head is unfinished, every check the base branch names is present, and every required check passed. checks:failed stays immediate on the first required failure. This replaces app-bound requirement matching and required-workflow path tracking, which are deleted. A workflows rule only disables the everything-is-required fallback. Resolves Codex's findings r4341313633 (sequential jobs in the fallback) and r4341313643 (workflow identity). Main brings #164's client helpers; the rules listing now uses GitHub.all instead of a local pagination loop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ed58e64b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state: Cursor["checks"], | ||
| prUrl: string, | ||
| ): Activity[] { | ||
| const required = needs.names.size || needs.workflows ? all.filter((entry) => entry.required) : all; |
There was a problem hiding this comment.
When the base branch uses a workflows ruleset and its jobs are not individually required status contexts, this filter drops every job because their isRequired value is false. Consequently a required-workflow failure never emits checks:failed; if another named required check passes, it can even produce checks:passed despite the workflow failure. Fresh evidence is that the final merge removed the earlier workflow-path association and retained only a boolean, even though GitHub defines this rule as requiring the specified workflow itself to pass. Retain enough workflow identity to include its jobs in required. GitHub rules API
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 7168a69. Requirements keep each required workflow's file path again, read from checkSuite.workflowRun.file.path. Every job of a required workflow now counts as required whatever its isRequired says, so a failing job emits checks:failed. checks:passed also waits until each required workflow has a run on the head. Workflows match by path only. A same-path workflow from another repository is a remaining edge case, listed as a follow-up. Regression tests: "a failing job in a required workflow wakes the wait" and "a required workflow requires its jobs, keeps optional checks optional, and holds back checks:passed until its run finishes".
(Written by Claude Opus 5.5)
| const required = needs.names.size || needs.workflows ? all.filter((entry) => entry.required) : all; | ||
| const reached = (outcome: "passed" | "failed", entry: Check | undefined): Activity[] => { | ||
| const id = `${head}:${outcome}`; | ||
| if (!entry?.result || entry.result.time < state.since || state.reported.includes(id)) return []; |
There was a problem hiding this comment.
When a wait starts after every required check has finished but while an optional check is still pending, idle becomes true only when that optional check later settles, yet passed remains the last required check and this condition rejects its older completion time as preceding state.since. The waiter therefore never emits checks:passed even though the aggregate pass condition was reached during this wait; freshness needs to use the time the head became idle rather than only the last required check's timestamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 7168a69. checks:passed is now dated and named by the last check to finish, which is when the head went idle, not by the last required check. A wait started after the required checks finished but before an optional check settled now reports the pass. Regression test: "checks:passed is dated when the head goes idle, after required checks finished before the start".
(Written by Claude Opus 5.5)
…ow-rule jobs - checks:passed now names and dates the last check to finish, so a wait started after the required checks finished still reports the pass once an optional check settles. - A ruleset's required workflow makes each of its jobs required (matched by workflow file path), and the workflow must be present before checks:passed, since GitHub may not mark those jobs isRequired. Addresses Codex's review of 5ed58e6. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7168a69d09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const rules = await github.all(`${base}/rules/branches/${ref}?per_page=100`); | ||
| const found: Requirements = { | ||
| names: new Set([ | ||
| ...(legacy.enforcement_level === "off" ? [] : array(legacy.checks).map((row) => string(row.context))), |
There was a problem hiding this comment.
When the base uses classic branch protection, GET /branches/{branch} can return required checks only in required_status_checks.contexts; the official protected-branch response shows that shape without a checks property (GitHub REST documentation). Passing the missing property to array() throws invalid GitHub response: expected an array, so quest wait fails before reporting any activity on such repositories. Read contexts as a fallback, or query the full protection endpoint when app-bound checks are needed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in de691f0. When required_status_checks has no checks array, the wait now reads the classic contexts list. App-bound checks are no longer needed, because the idle rule replaced app matching. Regression test: "classic protection that lists only contexts still names required checks".
(Written by Claude Opus 5.5)
Classic branch protection can list required checks only in required_status_checks.contexts, without checks; reading checks alone threw and failed the wait. Fall back to contexts. Addresses Codex's P1 finding on 7168a69. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary for de691f0:
(Written by Claude Opus 5.5) |
Problem
quest waitignored CI, so themergeskill told agents to run their own blocking check watcher. 119 of 396 audited sessions ended up pollinggh pr checksor writing their ownwait-ci.shscript.Behavior
quest waitnow reads the head's checks in each poll and reports two new events:checks:failedright away, at the head's first failing required check.checks:passedonce nothing on the head is still going and every required check passed. "Nothing still going" means no pending check, no unfinished GitHub Actions run (a run may still create jobs), and every check the base branch names is present.Each event fires at most once per head. It uses the check's name as the actor (spaces become
_) and links its details URL, or the PR's checks page when there is none.checks:passednames and dates the last check to finish, which is when the head went idle.isRequired(pullRequestNumber:)on check runs and commit statuses. It needs only read access and covers both branch protection and rulesets.classifyCheckfromquest status(kixelated/quest#162), so both commands agree on what passes. Neutral and skipped results pass. Optional failures never wake the wait.src/cli/github.ts, including itsallpagination from kixelated/quest#164.GET /repos/{owner}/{repo}/branches/{base}) and every page of rulesets (GET /repos/{owner}/{repo}/rules/branches/{base}). Both are readable with read access.checkSuite.workflowRun.file.path).checks:passed.--helpand the README describe the events and drop "Check runs ... are not activity".mergeskill drops the separate check watcher, andcompletesays onequest waitcovers CI.quest/m0/helpers/wait-checks.mdand its entry in the helpers epic.Decisions
isRequired.isRequiredalone cannot tell "the repository requires no checks" apart from "required checks have not started yet".checks:passednames the last check to finish, using its completion time and URL.The last required checkwas superseded with decision 4: the pass is dated when the head goes idle, so a wait started after the required checks finished still reports it (r4341431078).checks:passedwaits for the head to go idle. It no longer rebuilds GitHub's requirement model. It waits until no check is pending and no Actions run is unfinished, then requires every named check present and every required check passed. This closes the gaps Codex kept finding (r4341313633: sequential jobs with no requirements; r4341313643: required-workflow identity). The cost is thatchecks:passedalso waits for optional checks to finish.checks:failedis unchanged.Match app-bound requirements against the reporting app: superseded and deleted.Gate: superseded by the idle rule. Workflow file paths are still read so a required workflow's jobs count as required even when GitHub does not mark themchecks:passedon each required workflow's own run finishingisRequired(r4341431066).Validation
checks:passedwaiting for an optional check to finishworkflowsrule requiring its jobs and keeping optional checks optional, with an unfinished run holding backchecks:passed, and a failing job in itchecks:passedwhen nothing is requiredchecks:passeddated when the head goes idle, after the required checks finished before the start--sincejust checkandjust testpass in the Nix shell.checks:passed checkfor the required job and ignored the optional ones.checks:passed build_(windows-latest). ThecheckSuitefields it reads are also readable there.quest waiton this PR reportedchecks:passedon each new head, and resumed cursors did not replay it.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code