Skip to content

Commit 63a9545

Browse files
Mario Ferreiraclaude
authored and
ztrange
committed
merge: an absent tick is not a passing tick
Step 5 could merge a PR whose CI had not run. Two ways, both the same shape — an absence of signal read as a positive answer. Hit while landing ps-mutuus/mutuus-changelog#52. 1. `[]` was documented as "no CI gate exists — proceed". It is also what GitHub returns for a minute or two after a force-push, while it re-associates workflow runs with the new head. Observed on #52: rollup `[]` and `gh pr checks` printing "no checks reported" while a run was already queued for that exact head sha. `[]` now means re-query, and "no CI" may only be concluded after `gh run list` positively shows no run for the head — with the caveat, stated, that it sees Actions only. The wait loop now requires `total > 0` before it can conclude nothing is pending, so zero checks can never satisfy "all passed"; verified against a scripted always-empty rollup, which now times out instead of passing. 2. `.conclusion // .state` never consulted `.state`. jq's `//` falls through on null and false; an in-progress check run has `conclusion: ""`. Verified: `{"conclusion":"","state":null} | .conclusion // .state` yields `""`, which lands in a bucket no rule mentioned and reads as "not FAILURE, therefore fine". The replacement branches on `__typename` rather than sniffing nulls. The reason the null-sniffing shorthand is tempting and wrong: gh's export (cli/cli `api/export_pr.go`) emits *disjoint* key sets — CheckRun has no `state` key at all, StatusContext has no `status`/`conclusion` and names itself via `context` — so `.state != null` is a type test, not a completion test. That matters for the fix as first drafted, which used `.state != null` for "done": a legacy status context that is still running is `state: "PENDING"`, so a pending external CI would have counted as passed. Terminal for a StatusContext is SUCCESS/FAILURE/ERROR only; PENDING and EXPECTED are not. Enum values taken from GraphQL introspection, not memory: StatusState, CheckStatusState (COMPLETED is the only terminal one of six) and CheckConclusionState (the failing set is wider than FAILURE — ACTION_REQUIRED, STARTUP_FAILURE and STALE were missing). An unknown `__typename` falls into the else branch with no state and lands in `pending`. Unknown fails closed. Verified: the 5a filter extracted verbatim from the doc and run against cli/cli#14334 (37 rows, all green); against fixtures for in-progress check runs, a pending legacy context, mixed failures, SKIPPED, empty and null rollups; the 5b cross-check run in both directions on cli/cli trunk (0 for a foreign sha, 20 for the real head) so it is known to be able to answer negative and positive. Not observed: a live StatusContext row — that half is derived from gh's export and the enum, and is flagged as such under Limits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent f5c615d commit 63a9545

2 files changed

Lines changed: 94 additions & 19 deletions

File tree

‎README.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,9 @@ edits are live — there is no build or install step for day-to-day use.
5959

6060
- **[`merge`](merge/SKILL.md)** — land a PR on a `main` that may have moved: detect it locally
6161
rather than trusting `mergeStateStatus`, rebase, force-push under a *pinned* lease (the bare one
62-
silently clobbers after any fetch), read the CI result instead of the tick, then merge and clean
63-
up branch, primary clone and worktree.
62+
silently clobbers after any fetch), read the CI result instead of the tick — *and* instead of the
63+
missing tick, since an empty rollup and an empty `conclusion` are both what a not-yet-started
64+
check looks like — then merge and clean up branch, primary clone and worktree.
6465

6566
- **[`setup-git-guardrail`](setup-git-guardrail/SKILL.md)** — make the rules enforceable instead of
6667
remembered: a `reference-transaction` hook that refuses commits on `main` even under

‎merge/SKILL.md‎

Lines changed: 91 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,13 @@
22
name: merge
33
description: >-
44
Land a pull request safely: check `main` has not moved since the branch was cut, rebase onto it
5-
and force-push under a pinned lease if it has, read the CI result rather than trusting a tick,
6-
then rebase-merge and clean up the branch, the primary clone and the worktree. Use when the user
7-
says "merge it", "land this", "merge the PR", "ship it", "mergea", or "haz merge". Never merges
8-
without being told to. Treats the local `HEAD..origin/main` count as the authority on whether
9-
`main` moved, pins the force-with-lease to a recorded sha because the bare form silently
10-
clobbers, and confirms from PR state because `gh`'s own exit code lies here.
5+
and force-push under a pinned lease if it has, read the CI result rather than trusting a tick —
6+
or the absence of one — then rebase-merge and clean up the branch, the primary clone and the
7+
worktree. Use when the user says "merge it", "land this", "merge the PR", "ship it", "mergea", or
8+
"haz merge". Never merges without being told to. Treats the local `HEAD..origin/main` count as the
9+
authority on whether `main` moved, pins the force-with-lease to a recorded sha because the bare
10+
form silently clobbers, never lets an empty check rollup or an empty `conclusion` count as a pass,
11+
and confirms from PR state because `gh`'s own exit code lies here.
1112
---
1213

1314
# Merge — land it on a `main` that has not moved under you
@@ -85,26 +86,94 @@ Reproduced: a colleague pushed to the branch, bare lease correctly rejected with
8586

8687
Rejection means someone else moved the branch. Stop and look; do not escalate to `--force`.
8788

88-
## 5. Read the checks, don't trust the tick
89+
## 5. Read the checks — and don't trust the *absence* of a tick either
90+
91+
Every mistake available here has one shape: **an absence of signal read as a positive answer.** A
92+
check that cannot tell "no CI exists" from "CI has not started yet" is not a gate. `[]`, `""` and a
93+
missing key all mean *ask again*, never *pass*.
94+
95+
### 5a. The rollup is the authority
8996

9097
```bash
91-
gh pr checks <n>
98+
gh pr view <n> --json statusCheckRollup --jq '
99+
[ .statusCheckRollup[]? | {
100+
name: (.name // .context),
101+
done: (if .__typename == "CheckRun" then .status == "COMPLETED"
102+
else (.state != null and (.state | IN("PENDING","EXPECTED") | not)) end),
103+
bad: (if .__typename == "CheckRun"
104+
then ((.conclusion // "") | IN("FAILURE","TIMED_OUT","CANCELLED","ACTION_REQUIRED","STARTUP_FAILURE","STALE"))
105+
else ((.state // "") | IN("FAILURE","ERROR")) end) } ]
106+
| {total: length, pending: [.[]|select(.done|not)|.name], failed: [.[]|select(.bad)|.name]}'
92107
```
93108

94-
Exit `0` all passed, `1` a failure exists, `8` pending (documented, unobserved here).
109+
Merge only on **`total > 0` and `pending == []` and `failed == []`**. `total == 0` is never a pass
110+
— it goes to 5b. Requiring `total > 0` is what stops "0 checks, therefore 0 pending, therefore
111+
done" from satisfying a wait loop.
112+
113+
Why each clause is written that way — the shorthands that look equivalent are not:
114+
115+
- **The two row shapes have disjoint keys**, so a missing key reads as `null` and null-sniffing
116+
becomes a type test by accident. `gh`'s own export (`api/export_pr.go`) emits, for a **CheckRun**:
117+
`__typename, name, workflowName, status, conclusion, startedAt, completedAt, detailsUrl` — and no
118+
`state` at all; for a **StatusContext** (legacy commit status, e.g. an external CI provider):
119+
`__typename, context, state, targetUrl, startedAt` — no `status`, no `conclusion`, and the name
120+
lives in `context`. Branch on `__typename`, which is always present.
121+
- **`.conclusion // .state` does not fall through.** jq's `//` falls through on `null` and `false`
122+
only; an in-progress check run has `conclusion: ""`. Observed on ps-mutuus/mutuus-changelog#52:
123+
`{"name":"Site typecheck + build","status":"IN_PROGRESS","conclusion":""}` collapses into an
124+
empty-string bucket that is not `FAILURE` and so reads as fine. Gate on `status` first; look at
125+
`conclusion` only once `status == "COMPLETED"`.
126+
- **`.state != null` means "this is a legacy status context", not "it finished".** A pending one is
127+
`state: "PENDING"`. `StatusState` is `EXPECTED ERROR FAILURE PENDING SUCCESS` — only the last
128+
three are terminal, so `PENDING`/`EXPECTED` must count as pending.
129+
- **`COMPLETED` is the only terminal check-run status.** `CheckStatusState` is
130+
`REQUESTED QUEUED IN_PROGRESS COMPLETED WAITING PENDING`; five of the six are still running.
131+
- **The failing conclusions are more than `FAILURE`.** `CheckConclusionState` failures are
132+
`FAILURE TIMED_OUT CANCELLED ACTION_REQUIRED STARTUP_FAILURE STALE`; `SUCCESS`, `NEUTRAL` and
133+
`SKIPPED` pass.
134+
- An unrecognised `__typename` falls into the `else` branch with no `state` and lands in `pending`.
135+
That is deliberate: unknown fails closed.
136+
137+
### 5b. An empty rollup means re-query, not "no CI"
138+
139+
`[]` is also what GitHub returns for a minute or two after a force-push, while it re-associates
140+
workflow runs with the new head. Observed on #52 immediately after the step-4 push: the rollup was
141+
`[]` and `gh pr checks 52` printed `no checks reported on the 'claude/gh-path-helper' branch`,
142+
while a `PR checks` run was already queued for that exact head sha. Concluding "no CI" there merges
143+
with CI unrun.
144+
145+
Prove a run exists before concluding one does not:
95146

96-
**Exit `1` also means "no checks are configured"** — it prints `no checks reported on the '<b>'
97-
branch` to stderr with empty stdout. Branching on exit `1` alone will read a repo with no CI as a
98-
repo with failing CI. Disambiguate:
147+
```bash
148+
HEAD=$(git rev-parse HEAD)
149+
gh run list --branch <branch> --limit 20 --json headSha,status,conclusion,workflowName \
150+
--jq "[.[]|select(.headSha==\"$HEAD\")]"
151+
```
152+
153+
Non-empty → CI exists; wait, whatever the rollup says. Empty **and** an empty rollup is still not
154+
proof: `gh run list` sees GitHub Actions only, so an external provider that posts commit statuses
155+
appears in neither until it registers. Re-query over a settle window (~60s) before calling it "no
156+
CI", and say in the final report that no-CI was concluded from an absence.
157+
158+
### 5c. The wait loop
99159

100160
```bash
101-
gh pr view <n> --json statusCheckRollup \
102-
--jq '[.statusCheckRollup[]?|.conclusion // .state]|group_by(.)|map({v:.[0],n:length})'
161+
for _ in $(seq 1 60); do
162+
R=$(gh pr view <n> --json statusCheckRollup --jq '<the 5a filter>')
163+
T=$(jq -r .total <<<"$R"); P=$(jq -r '.pending|length' <<<"$R"); F=$(jq -r '.failed|length' <<<"$R")
164+
[ "$F" -gt 0 ] && { echo "failed: $(jq -c .failed <<<"$R")"; break; }
165+
[ "$T" -gt 0 ] && [ "$P" -eq 0 ] && { echo "all passed"; break; }
166+
sleep 15
167+
done
103168
```
104169

105-
`[]` means no CI gate exists — proceed. Otherwise pass = no `FAILURE` and nothing outside
106-
`COMPLETED`. `.conclusion // .state` is needed because check runs and legacy status contexts expose
107-
different fields.
170+
`T > 0` guards the exit. Without it the loop terminates instantly on the post-force-push `[]`,
171+
which is exactly how a hand-written poll loop passed a PR whose checks had not started.
172+
173+
`gh pr checks <n>` is a hint, not the gate: exit `0` all passed, `1` **either** a failure exists
174+
**or** no checks are configured (the two are indistinguishable from the exit code), `8` pending
175+
(documented, unobserved here). Use the rollup to decide; use `gh pr checks` only for a readable
176+
dump when reporting.
108177

109178
## 6. Merge
110179

@@ -144,3 +213,8 @@ handed back, a check that was pending, a branch left in place.
144213
the base. Expect `405 Merge cannot be performed` or `409 head did not match`; re-run from step 2.
145214
- `gh pr checks` exit `8` is documented but was not observed here. Treat any non-`0`, non-`1` exit
146215
as pending and wait rather than merging.
216+
- The `StatusContext` half of the step-5a filter is derived, not observed: the field list comes from
217+
`gh`'s `api/export_pr.go` and the terminal/failing values from the `StatusState` GraphQL enum, but
218+
no live legacy status context has been seen through it. Only the `CheckRun` half is observed
219+
(ps-mutuus/mutuus-changelog#52, and a 37-row live rollup on cli/cli#14334). If a repo's CI posts
220+
commit statuses rather than check runs, read the raw rows once before trusting the summary.

0 commit comments

Comments
 (0)