Skip to content

Tick /ship-it's CI item from the head commit's status rollup - #176

Merged
mattmenefee merged 1 commit into
mainfrom
tick-ship-it-ci-from-head-rollup
Oct 24, 2026
Merged

Tick /ship-it's CI item from the head commit's status rollup#176
mattmenefee merged 1 commit into
mainfrom
tick-ship-it-ci-from-head-rollup

Conversation

@mattmenefee

@mattmenefee mattmenefee commented Oct 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

  • /ship-it Step 5 now ticks the CI test-plan item from gh pr view --json state,headRefOid,statusCheckRollup instead of gh pr checks, whose output names no commit
  • The item is ticked only when local HEAD, the pushed branch and the pull request's head name one commit, the rollup lists at least one check and every check reports SUCCESS (the workflow's own ship-it status left out), and it names that commit's short SHA
  • An empty rollup counts as pending, since a push updates the pull request's head before its checks register; a CANCELLED check leaves the item open and is put to the user; a tick left from an earlier commit is reopened
  • Step 7 re-reads the rollup before asking to mark the pull request ready, states the CI result in that question and reports in its final summary whether the item was ticked and on which commit

Fixes #175

Test plan

  • Ran the new rollup block on this branch: local HEAD, the pushed branch and headRefOid printed the same commit, followed by every lint check
  • Ran the block's --jq filter against merged PR Add a /rebase skill that rebases, verifies and pushes under a lease #173: it printed nothing
  • Observed the empty-rollup window: right after a push, headRefOid named the new commit while the rollup listed no checks
  • Confirmed gh pr checks output carries no commit SHA (run against PR Find unfilled blocks for /reflow and run it from /ship-it #171)
  • markdownlint with the repository's config passes on the changed skill file
  • /reflow finds no unfilled block among the changed lines
  • CI green on the head commit, 1174585 (status rollup: every lint check succeeded)
  • Ran /ship-it on this branch end to end with the new Step 5 and Step 7 wording: Step 5 left the CI item open while the rollup showed lint QUEUED, and Step 7 re-read it, stated it in the mark-ready question and ticked it once every check succeeded

@mattmenefee mattmenefee self-assigned this Oct 24, 2026
@mattmenefee
mattmenefee force-pushed the tick-ship-it-ci-from-head-rollup branch from 609f3a6 to 07849f2 Compare October 24, 2026 18:38
@mattmenefee
Tick /ship-it's CI item from the head commit's status rollup
1174585 
Step 5 ticked the pull request's CI test-plan item from `gh pr checks`,
whose output names no commit, so a green result could not be tied to
the head Step 4 had just pushed. That push has usually only just
started the checks, so the item was either ticked from a stale result
or left open with nothing to bring the run back to it.

Step 5 now reads `gh pr view --json state,headRefOid,statusCheckRollup`
for an open pull request, leaving out the workflow's own `ship-it`
status, beside local HEAD and the pushed branch tip. The item is ticked
only when those three name one commit, the rollup lists at least one
check and every check reports `SUCCESS`, and it names that commit. A
push updates the pull request's head before its checks register, so an
empty list counts as pending, like a running check. A `CANCELLED` check
leaves the item open and is put to the user, since GitHub Actions
reports a superseded run that way rather than as a failure, and a tick
left from an earlier commit is reopened.

Step 7 reads the rollup again before asking to mark the pull request
ready, ticks the item if the checks now pass, states the CI result in
that question and says in the final report whether the item was ticked
and on which commit.

Fixes #175.
@mattmenefee
mattmenefee force-pushed the tick-ship-it-ci-from-head-rollup branch from 36630e8 to 1174585 Compare October 24, 2026 18:53
@mattmenefee

mattmenefee commented Oct 24, 2026

Copy link
Copy Markdown
Owner Author

Document Review: /ship-it SKILL.md (skill-DOC-REVIEW.md) — all findings fixed

9 findings: 6 fixed (1 high, 2 medium, 3 low), 3 observations, all applied

Redacted before posting: none

This run squashed the branch's history, so file and line references may have drifted, and the cited commit 07849f2 is now part of 1174585.

Click to expand full details

Document Review: /ship-it SKILL.md
Scope .claude/skills/ship-it/SKILL.md, the changes on this branch since base commit ea056b8
Branch tick-ship-it-ci-from-head-rollup
Reviewed 2026-10-24
Reviewed by Opus 5.5 (claude-opus-5-5)
Collated by Opus 5.5 (claude-opus-5-5)

Documents

Document Commit
.claude/skills/ship-it/SKILL.md 07849f2

Review History

2026-10-24 — Initial review by Opus 5.5 (claude-opus-5-5)

Overall Assessment

The change does what it sets out to do in intent, and the reviewer ran its jq filter against the pull request and saw it print the head commit and its checks with ship-it excluded, but the condition for ticking the CI item has three gaps that can still produce a wrong status claim. An empty rollup satisfies "every check listed has succeeded" (F1, High, confirmed by the orchestrator right after this branch's push), the canceled-run test can't be applied from the block's output and describes CircleCI rather than this repository's GitHub Actions (F2), and the tick skips the branch-tip check the user-wide rule requires (F3); together with the re-run case (F6) these form G1, the Step 5 tick paragraph and its block. G2 covers Step 7: the CI re-read comes after the irreversible mark-ready question (F4), and the final report's contents don't include the CI state the new paragraph sends there (F5). No ⚖️ Decision is open; three 💡 observations offer optional wording and filter tweaks. All six actionable findings are fixed and both groups are closed, and the three observations were applied in the same edits.

Accuracy

F1 🟠 High Priority - An empty rollup meets "every check listed … has succeeded" ✅ Fixed

  • Document — .claude/skills/ship-it/SKILL.md

  • Location — Step 5: Write the Pull Request, the paragraph after the rollup block ("Tick the CI item only when headRefOid equals local HEAD and every check listed for it has succeeded…"). The Step 7 paragraph relies on the same rule.

  • Issue — The tick condition can be met with no checks at all. Step 4's push updates the pull request's headRefOid right away, but the workflow takes a few seconds to queue its lint check run, so .statusCheckRollup can be empty while headRefOid already equals HEAD. "Every check listed has succeeded" is then true of an empty list. Nothing is pending either, so the "while any is pending" clause doesn't keep the item open. The agent would tick the CI item in a public pull request body before CI has started.

    The text also never says what counts as success. A check run reports conclusion, a status context reports state, and values such as SKIPPED, NEUTRAL and CANCELLED are left to judgment. This is the kind of wrong status claim the change exists to prevent.

  • Suggestion — Reword the condition to: "Tick the CI item only when headRefOid equals local HEAD, the rollup lists at least one check and every check listed reports SUCCESS, and name that commit's short SHA in the item. An empty list means the checks have not registered yet: treat it like a pending check." Step 7's "If the checks have since succeeded" then inherits the rule.

  • Recommendation — Implement: it's a one-sentence change that closes the most likely false tick, the one right after Step 4's push.

  • Verification — Observed by the orchestrator: immediately after this branch's force push in this session, gh pr view 176 --json headRefOid,statusCheckRollup returned headRefOid 07849f2 (equal to local HEAD) with an empty statusCheckRollup, confirming the empty-rollup window.

  • Status — ✅ Fixed: the tick now requires the rollup to list at least one check and every check listed to report SUCCESS, and treats an empty list as pending

F2 🟡 Medium Priority - The canceled-run test can't be applied here and doesn't fit this repository's CI ✅ Fixed

  • Document — .claude/skills/ship-it/SKILL.md

  • Location — Step 5, same paragraph, last sentence ("A check far shorter than usual that reports a failure may be a run canceled on purpose; ask rather than tick or call it red.")

  • Issue — The sentence has two problems:

    • No timing in the output. The rollup block prints only name, status and conclusion or state. It prints no startedAt or completedAt, so the agent can't tell whether a check was "far shorter than usual".
    • Wrong platform. The heuristic comes from the user-wide CLAUDE.md, whose wording is about CircleCI: "A CircleCI run canceled on purpose reports as a failed status". This repository runs GitHub Actions (.github/workflows/ci.yml, with concurrency: cancel-in-progress: true). There a canceled run reports the conclusion cancelled, not a failure. I verified this with gh run list --repo mattmenefee/dotfiles --status cancelled --limit 3 --json conclusion,…, where each run shows "conclusion":"cancelled".

    In the rollup a cancellation therefore shows as CANCELLED and is never confused with FAILURE.

  • Suggestion — Replace the sentence with: "A CANCELLED check was superseded by a later push or stopped by hand: it is neither a pass nor a failure, so leave the item open and ask rather than tick it or call it red."

  • Recommendation — Implement: the current test can't be carried out from the block, and it describes a failure mode this CI doesn't have.

  • Status — ✅ Fixed: replaced the shorter-than-usual test with the suggested CANCELLED rule

F3 🟡 Medium Priority - The tick doesn't check the pushed branch tip, which the user-wide rule requires ✅ Fixed

  • Document — .claude/skills/ship-it/SKILL.md

  • Location — Step 5, rollup block and the paragraph after it. Step 7's paragraph reuses them.

  • Issue — The user-wide CLAUDE.md, under Pull Requests, says: "Before calling a branch green or merging it, check that git ls-remote origin refs/heads/<branch> equals local HEAD and the rollup's commit." Ticking the CI item is calling the branch green.

    The new block compares only headRefOid with HEAD. If another session pushes after this one, GitHub's headRefOid can briefly lag behind the branch. Then headRefOid still equals the local HEAD while the branch tip has moved, and the item is ticked for a commit that is no longer the head. Step 7's record block already checks all three values (head, remote, pr_head), so the CI tick is the only status claim in the skill that skips the branch tip.

  • Suggestion — Add git ls-remote origin 'refs/heads/<branch>' | cut -f1 to the rollup block after git rev-parse HEAD, matching the line in Step 7's record block. Change the condition to "when local HEAD, the pushed branch and headRefOid all name one commit".

  • Recommendation — Implement: it's one line, it matches the user-wide rule and Step 7's own block, and it closes the race.

  • Status — ✅ Fixed: the rollup block prints the pushed branch tip with git ls-remote, and the tick requires local HEAD, the pushed branch and headRefOid to name one commit

Clarity and Structure

F4 🟢 Low Priority - Step 7 re-reads CI only after the mark-ready question ✅ Fixed

  • Document — .claude/skills/ship-it/SKILL.md
  • Location — Step 7: Mark Ready and Record the Run, the new paragraph ("When Step 5 left the CI item open, read the rollup with its block again…")
  • Issue — The paragraph comes after the AskUserQuestion about marking the pull request ready. Marking ready notifies watchers and can't be undone. As written, the user rules on that without the CI state the skill is about to read. The pull request could go out of draft with red or pending CI, and the user would learn this only from the final report.
  • Suggestion — Move the paragraph to just before "On every run, name each deferred finding…", and include the CI state (passed on which commit, pending or failed) in the mark-ready question.
  • Recommendation — Implement: it's a paragraph move that puts the CI state in front of the user before a step that can't be undone.
  • Status — ✅ Fixed: the CI re-read now comes before the mark-ready question, which states the CI state just read

F5 🟢 Low Priority - The final report doesn't include the CI state that the new paragraph sends there ✅ Fixed

  • Document — .claude/skills/ship-it/SKILL.md
  • Location — Step 7, the closing paragraph ("Finish by telling the user what was pushed and how, …"), compared with the new paragraph's "say so in the final report"
  • Issue — The new paragraph points a pending or failed CI result to "the final report". The paragraph that defines the report lists what was pushed, which files were posted, deleted or kept, the ship-it status and the URL, but not CI. An agent following the closing list literally can leave CI out.
  • Suggestion — In the closing paragraph, add "whether the CI item was ticked and on which commit, or what kept it open" after "…what withheld it".
  • Recommendation — Implement: it's one clause, and it keeps the two paragraphs from disagreeing.
  • Status — ✅ Fixed: the closing paragraph now reports whether the CI item was ticked and on which commit, or what kept it open

F6 🟢 Low Priority - On a re-run, a carried-over body can keep a CI tick for an older head ✅ Fixed

  • Document — .claude/skills/ship-it/SKILL.md
  • Location — Step 5, the CI tick rule and the paragraph "When an open pull request already has a body, carry over anything in it that was not derived from the commits…"
  • Issue — After an earlier /ship-it run, the existing body may hold a ticked CI item naming the previous head's short SHA. On a re-run after a new push, the skill composes from the commits but also edits an existing body. Nothing says an earlier CI tick must be reopened when its SHA is no longer HEAD. The tick rule implies it, but an agent revising the existing body can keep the old tick as it stands.
  • Suggestion — Append to the tick paragraph: "A CI item already ticked for an earlier commit is reopened until the rollup passes on this one."
  • Recommendation — Implement: it's one sentence, and re-runs after a push are the normal case for this skill.
  • Status — ✅ Fixed: added the suggested reopen sentence to the tick paragraph

Observations

F7 💡 Observation (optional action) - "status rollup, which names the commit" is slightly loose

  • Document — .claude/skills/ship-it/SKILL.md
  • Location — Step 5, "Read CI for an open pull request from its status rollup, which names the commit it reports on; gh pr checks does not:"
  • Issue — statusCheckRollup has no SHA field. The commit is named by headRefOid, which the same query returns. A reader looking for a SHA inside the rollup entries won't find one. Optional fix: "from gh pr view, whose headRefOid names the commit its status rollup reports on; gh pr checks names none:". Applied: the lead-in now says headRefOid names the commit the status rollup reports on and that gh pr checks names none.

F8 💡 Observation (optional action) - The rollup block lacks the OPEN filter the skill's other gh pr view reads use

  • Document — .claude/skills/ship-it/SKILL.md
  • Location — Step 5, rollup block
  • Issue — Step 5's check-and-send block and Step 7's record block both use select(.state == "OPEN"). Step 5 notes that gh returns a closed or merged pull request for a reused branch name. The rollup block relies only on the prose gate "for an open pull request". Optional fix: add state to --json and wrap the filter in select(.state == "OPEN") | …, so a closed pull request prints nothing. Applied: the block reads state and wraps the filter in select(.state == "OPEN"); run against merged pull request 173, it prints nothing.

F9 💡 Observation (optional action) - "with its block" is ambiguous

  • Document — .claude/skills/ship-it/SKILL.md
  • Location — Step 7, "read the rollup with its block again"
  • Issue — "Its" refers back to the rollup, and the block it means is in another step. Optional fix: "read the rollup again with Step 5's rollup block". This matches the next sentence's "Step 5's check-and-send block". Applied: Step 7 now reads "read the rollup again with Step 5's rollup block".

Consolidated Summary

Finding Type Category Description Document Location Recommendation Group Status
F1 🟠 High Accuracy An empty rollup meets "every check listed … has succeeded" .claude/skills/ship-it/SKILL.md Step 5, tick paragraph Implement G1 ✅
F2 🟡 Medium Accuracy Canceled-run test can't be applied and doesn't fit this repository's CI .claude/skills/ship-it/SKILL.md Step 5, tick paragraph Implement G1 ✅
F3 🟡 Medium Accuracy Tick doesn't check the pushed branch tip .claude/skills/ship-it/SKILL.md Step 5, rollup block Implement G1 ✅
F4 🟢 Low Clarity and Structure Step 7 re-reads CI only after the mark-ready question .claude/skills/ship-it/SKILL.md Step 7, CI paragraph Implement G2 ✅
F5 🟢 Low Clarity and Structure Final report omits the CI state sent there .claude/skills/ship-it/SKILL.md Step 7, closing paragraph Implement G2 ✅
F6 🟢 Low Clarity and Structure Re-run can keep a CI tick for an older head .claude/skills/ship-it/SKILL.md Step 5, tick paragraph Implement G1 ✅
F7 💡 Observation Accuracy "status rollup, which names the commit" is slightly loose .claude/skills/ship-it/SKILL.md Step 5, rollup lead-in — — —
F8 💡 Observation Clarity and Structure Rollup block lacks the OPEN filter .claude/skills/ship-it/SKILL.md Step 5, rollup block — — —
F9 💡 Observation Clarity and Structure "with its block" is ambiguous .claude/skills/ship-it/SKILL.md Step 7, CI paragraph — — —

Pre-Merge Checklist

G1 ✅ — Tighten the CI tick condition in Step 5

Closed: every member was fixed in Step 5's rollup block and the tick paragraph after it.

  • ✅ F1 - Require at least one check and every check SUCCESS; treat an empty rollup as pending
  • ✅ F2 - Replace the shorter-than-usual test with a CANCELLED rule
  • ✅ F3 - Add the git ls-remote branch-tip line and require all three commits to match
  • ✅ F6 - Reopen a CI item ticked for an earlier commit

G2 ✅ — Surface the CI state in Step 7

Closed: both members were fixed in Step 7, which now puts the CI state in the mark-ready question and the final report.

  • ✅ F4 - Move the CI re-read ahead of the mark-ready question and include its state there
  • ✅ F5 - Add the CI item's state to the final report's contents

@mattmenefee
mattmenefee marked this pull request as ready for review October 24, 2026 18:55
@mattmenefee
mattmenefee merged commit a41d887 into main Oct 24, 2026
9 checks passed
@mattmenefee
mattmenefee deleted the tick-ship-it-ci-from-head-rollup branch October 24, 2026 18:56
This was referenced Oct 24, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@mattmenefee mattmenefee

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tick /ship-it's CI test-plan item from the head commit's status rollup

1 participant

@mattmenefee