Repository navigation
Skip CI on draft pull requests, run main on a schedule and add Dependabot - #182
Conversation
c893504 to
daa342e
Compare
Every push to a pull request branch ran the lint job, so incremental pushes to a draft paid for the tests, the zsh install and the linters on trees that were about to be superseded. The pull_request trigger now lists ready_for_review alongside the default event types, and the job skips while the pull request is a draft. A draft push therefore runs no steps and leaves only a skipped lint check; marking it ready runs the job, and so does every later push. A skipped job satisfies a required check, which is safe only because a draft cannot be merged. Without ready_for_review, a pull request marked ready with no further push would carry only that skipped check, which satisfies main's required lint check, and could merge unlinted. A weekday schedule, plus a manual dispatch, takes the place of the run on every push to main. main's ruleset does not require a branch to be up to date before it merges, so a rebase merge can land a tree no pull request run linted, and the next weekday's run checks it. super-linter lints the whole codebase here, so it still has work to do on a scheduled run. Each workflow run on a commit adds its own lint check, so a draft's skipped check stays listed beside the run that marking it ready starts. /ship-it now keeps only the latest check of each name before requiring every check to pass, treats a check skipped while the pull request is a draft as not yet run, and after marking a pull request ready waits for that run and ticks the CI item from it. The project CLAUDE.md warns that a draft's skipped check is not a green build.
Dependabot security updates are enabled for the repository, but nothing proposes routine version bumps, so the checkout and super-linter actions the CI workflow pins move only when someone remembers to bump them. GitHub Actions is the only ecosystem Dependabot can update here: the repository has no language manifests, and Brewfiles are not supported. A weekly schedule groups every actions update into one pull request, and a limit of one open pull request keeps a second from piling up while the first waits. Each is assigned to the repository owner, the default for pull requests here. A comment asks that the actions stay on tag refs, since the Ruff step reads the super-linter version from its tag. Dependabot opens its pull requests as ready rather than as drafts, so CI runs on them, and each still needs the ship-it status main's ruleset requires. /ship-it as written would rebase, squash and rewrite the body, and Dependabot stops rebasing a pull request once anyone else pushes to it, so the project CLAUDE.md now says to run only /ship-it's CI read and its final step on one.
daa342e to
31f525d
Compare
Plan (
|
Code Review (
|
| Scope | .github/workflows/ci.yml, .github/dependabot.yml, .claude/skills/ship-it/SKILL.md and the commit messages of PR #182 |
| Branch | ci-skip-drafts-schedule-main |
| Reviewed | 2026-10-24 |
| Reviewed by | code-best-practices-reviewer, Claude Opus 5.5 (claude-opus-5-5) |
| Collated by | Claude Opus 5.5 (claude-opus-5-5) |
Documents
| Document | Branch | Commit |
|---|---|---|
.github/workflows/ci.yml |
ci-skip-drafts-schedule-main |
9ec0c62 |
.github/dependabot.yml |
ci-skip-drafts-schedule-main |
31f525d |
.claude/skills/ship-it/SKILL.md |
ci-skip-drafts-schedule-main |
9ec0c62 |
Reviewed at 4d17b35 and collated after the clean rebase onto origin/main (be5b711 → 855ca60, 4d17b35 → c893504, both patches unchanged). Line numbers are as of c893504. The Dependabot commit was since amended to daa342e and pushed (weekly, limit 1, assignee, modeled on another project's config).
Findings
F1 🔴 High — /ship-it's rollup rule can never tick CI after a draft goes ready
File: .claude/skills/ship-it/SKILL.md:427-440
Status: ✅ Fixed in 9ec0c62: the rollup jq keeps the latest check per name (tested on PR 121's duplicate lint runs); the sentence now reads "SKIPPED while the pull request is a draft"
A draft push leaves lint SKIPPED on the head; gh pr ready without a new push adds lint SUCCESS on the same SHA. The rollup lists both (confirmed: PR 121's rollup shows two lint runs on 6678542), so "every check listed reports SUCCESS" never holds, and the new sentence misreads it as a draft's skip on a pull request that is no longer a draft. A ready marked seconds after a push can also leave CANCELLED beside SUCCESS.
Recommendation: Implement. Keep only the latest check per name in the rollup jq (group_by(.name // .context) | map(max_by(.startedAt // ""))) and reword the sentence to "SKIPPED while the pull request is a draft".
F2 🟡 Medium — Step 7 marks ready before CI has ever run, and nothing reads CI afterwards
File: .claude/skills/ship-it/SKILL.md:874-884
Status: ✅ Fixed in 9ec0c62: Step 7 waits in the background for the CI run gh pr ready starts, then re-reads the rollup and ticks by Step 5's rule (ruled: wait after, not dispatch first)
/ship-it opens every pull request as a draft, so by Step 7 the rollup always shows SKIPPED. The mark-ready question is asked with no lint result, gh pr ready starts lint, and the run ends with the CI item unticked.
Recommendation: Implement, after F1. After gh pr ready, wait for the lint run on the head, re-read the rollup and tick or report it. Alternative: offer gh workflow run ci.yml --ref <branch> before asking, since workflow_dispatch is now a trigger.
F3 🟡 Medium — Commit message states the required-check hazard backwards
File: commit 855ca60, paragraph 2
Status: ✅ Fixed: 9ec0c62's message restates the hazard (a lone skipped check could merge unlinted)
With a job-level if, a draft push always reports SKIPPED. Without ready_for_review, that skipped check would satisfy the requirement and the pull request could merge unlinted; "a required check that never reports blocks the merge" is the path-filter failure mode.
Recommendation: Implement before merge (a merged message cannot be edited).
F4 🟡 Medium — Unverified: does a SKIPPED run mask a later FAILURE on the same SHA?
File: .github/workflows/ci.yml:23-25
Status: ⏸️ Deferred to #183 (test in a disposable public repository)
The "safe only because a draft cannot be merged" comment assumes GitHub judges the required check by the latest lint run. If any passing run satisfies it, a failure after ready would not block the merge, and ship-it does not check CI. GitHub's docs don't say how duplicate runs combine.
Recommendation: Defer to a test in a disposable public repository; if unsafe, have Step 7 withhold ship-it until the latest lint is SUCCESS.
F5 ⚖️ Decision — Schedule versus push on main
File: .github/workflows/ci.yml:7-11
Status: ✅ Ruled: keep the schedule as requested; 9ec0c62's message no longer frames the push run as a cost
main had 22, 20 and 28 push runs in August, September and October; a weekday cron is about 22 a month, so the schedule saves little and its signal lags a merge by up to three days. The schedule is what was asked for ("only run main on occasion like [redacted: private repository name] does").
Recommendation: Keep the schedule as requested, and drop paragraph 1's framing of the push run as a cost (part of F3's reword).
F6 🟢 Low — "within a day" ignores weekends
File: commit 855ca60, paragraph 3
Status: ✅ Fixed: "the next weekday's run checks it"
A Friday merge after 11:17 UTC is first checked Monday.
Recommendation: Implement with F3: "by the next weekday's run".
F7 🟢 Low — "A draft push therefore starts nothing" is inaccurate
File: commit 855ca60, paragraph 2
Status: ✅ Fixed: "runs no steps and leaves only a skipped lint check"
It creates a workflow run and a skipped lint check (run 40128401968 on 4d17b35).
Recommendation: Implement with F3: "runs no steps".
F8 🟡 Medium — No route for shipping a Dependabot pull request
File: .github/dependabot.yml, CLAUDE.md
Status: ✅ Fixed in 31f525d: project CLAUDE.md note (ruled: note, not a /ship-it branch)
Every Dependabot pull request needs ship-it, but /ship-it as written rebases, may squash and rewrites the body, after which Dependabot stops maintaining the branch.
Recommendation: Implement: a short CLAUDE.md note or a /ship-it branch for dependabot[bot] pull requests that skips Steps 1-3, keeps the body and runs only the CI read and Steps 6-7.
F9 🟢 Low — Scheduled workflows stop after 60 days without activity
File: .github/workflows/ci.yml:9-10
Status: 🚫 Ignored: weekly Dependabot pull requests keep the repository active
Low risk given commit frequency and monthly Dependabot pull requests; GitHub emails when it disables one, and gh workflow enable ci.yml restores it.
Recommendation: Skip.
F10 🟢 Low — Generic rollup readers may report a draft's SKIPPED as green
File: CLAUDE.md (Pull Requests)
Status: ✅ Fixed in 9ec0c62: project CLAUDE.md note. Corrected while fixing: gh pr checks shows skipping, not pass, though it exits 0
gh pr checks and the merge box show SKIPPED as passing, so a handoff could call a draft "CI green".
Recommendation: Implement: "CI skips draft pull requests; a SKIPPED lint check has not run".
F11 🔵 Nit — "the job below waits for it"
File: .github/workflows/ci.yml:5
Status: ✅ Fixed in 9ec0c62
The job skips drafts and runs on that event; it does not wait.
Recommendation: Implement: "the job below skips drafts and runs on it".
F12 🟢 Low — Dependabot config should say to keep tag refs
File: .github/dependabot.yml
Status: ✅ Fixed in 31f525d
Now that #180's Ruff step on main reads the super-linter tag with sed, SHA pinning would break it.
Recommendation: Implement: a one-line comment on the entry.
Checked and fine
- The
ifexpression, default event types, concurrency groups and the Dependabot token'sstatuses: writeall behave as intended - super-linter supports scheduled and dispatched runs with
VALIDATE_ALL_CODEBASE: true - dependabot.yml is valid; commit c893504's claims are accurate
- Nothing else in the tree assumes CI on push to main or on draft pushes
Pre-Merge Checklist
Group A: /ship-it rollup (F1, then F2)
- ✅ F1 Dedupe the rollup to the latest check per name; reword the SKIPPED sentence
- ✅ F2 Read CI after
gh pr ready(depends on F1)
Group B: Commit message reword of 855ca60 (F3, F6, F7, F5's framing)
- ✅ F3 Restate the hazard
- ✅ F6 "by the next weekday's run"
- ✅ F7 "runs no steps"
- ✅ F5 ⚖️ Keep the schedule; drop the push-run-as-cost framing
Group C: Comments and guidance (F11, F12, F10, F8)
- ✅ F11 Fix the
ready_for_reviewcomment - ✅ F12 Tag-ref comment in dependabot.yml
- ✅ F10 CLAUDE.md note on SKIPPED
- ✅ F8 Dependabot shipping route
Not recommended for this revision
- ⏸️ F4 Test duplicate-run semantics in a disposable repository (Defer)
- 🚫 F9 60-day schedule disable (Skip)
Review History
Handoff notes — 8 decisions, 5 insights, 1 dead end, 1 open questionFrom Click to expand full detailsDecisions & Rationale
Insights & Learnings
Dead Ends
Open Questions
References
|
Summary
ready_for_reviewadded to thepull_requesttypes), then on every later push. A skipped job satisfies the requiredlintcheck, which is safe only because a draft cannot be merged.workflow_dispatchreplace the run on every push tomain.main's ruleset does not require branches to be up to date, so a rebase merge can land a tree no pull request run linted; the next weekday's run checks it. super-linter still lints the whole codebase, so a scheduled run has files to check./ship-itkeeps only the latest check of each name when reading the rollup, since a draft's skippedlintstays listed beside the run that marking it ready starts. It treats a check skipped while the pull request is a draft as not yet run, and aftergh pr readywaits for that run and ticks the CI item from it..github/dependabot.yml: weekly GitHub Actions updates grouped into one pull request, at most one open at a time and assigned to the repository owner, with a comment to keep tag refs because the Ruff step reads the super-linter tag/ship-it's CI read and Step 7, since Dependabot stops rebasing a branch anyone else pushes tolintcan mask a later failure on the same commitTest plan
.yaml-lint.ymlonci.ymlanddependabot.ymlci.yml/ship-itskill andCLAUDE.md/ship-itscrubber test (test-scrub.sh)lintfor a commit carrying twolintruns (PR 121's 6678542)lintcheck reports skipped on this draft (rollup on 31f525d)lint, and it passes (run 40130496930 on 31f525d)/ship-it's Step 7 waits for that run, and the deduped rollup reports onelint SUCCESSbeside the draft's skipped run on 31f525dmainrunslint(run 40130724110 on 03e7665, passed)lintrunning on it