feat: open (or update) an issue when a publish run fails - #326
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
…e action A workflow that runs where no PR carries its result -- a push to the default branch, a schedule, a release -- fails silently: the red run is visible only in the Actions tab, so a broken deploy can sit unnoticed while the published site goes stale. report-failure.yml files an issue for that, or comments on the issue already open for the same title rather than filing a duplicate on every failing run. Consumers add it as a final job gated on the job they want watched. It is its own workflow rather than a job inside quarto-publish.yml because a reusable workflow's jobs can only hold permissions the caller granted: folding it in would have made issues: write mandatory for every existing caller of that workflow. As a separate workflow the permission is opt-in, and any workflow can be watched. The lookup and issue calls live in a shared open-failure-issue composite, which check-links.yml now uses in place of its own inline gh issue create -- so the dedupe and the missing-label tolerance apply there too. Closes #325
A uses: ref resolves when the job is prepared, before any step-level if: is evaluated, so check-links.yml's reference to open-failure-issue@v2 failed link-checker outright -- on a step gated behind failure() that could never have run. _selftest.yml drives check-links through a local ./ ref, so this went red on every PR, and no change inside this PR could fix it: the action does not exist at @v2 until this merges and the tag slides. website-publish.yml's dogfood job carries the same dependency and is deferred with it. Both land in #327, once @v2 includes the action. CLAUDE.md records the distinction the existing bootstrapping note misses: whether this gap is a coverage footnote or a red check depends on whether the new caller is dogfooded here on every PR.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code reviewReviewed the Four issues found, posted as inline comments, most significant first:
One additional finding not tied to a specific diff line: VerdictNeeds more work — primarily finding 1 (missing |
|
Claude finished review — View run Review posted on PR #326. Summary: Four issues found and posted as inline comments (most severe first):
Also flagged (not tied to a diff line): Verdict: Needs more work — the missing
|
Two real bugs from review, plus three doc-accuracy fixes.
1. report-failure.yml's report job granted issues: write and then ran
actions/checkout. Naming any scope zeroes the rest, so contents was none
and the checkout would fail on a private or internal repo -- at the moment
the job exists to report a failure. Rather than widen the grant, the
checkout is gone: the composite already carries its own files via
github.action_path, and now names the repository through GH_REPO instead
of inferring it from a git remote. issues: write alone is now true rather
than aspirational, and the composite no longer silently depends on the
caller having checked out the repo being reported against.
2. The labels input went to a single --label unsplit. That flag is a Cobra
StringSlice, which splits on commas without trimming, so a natural
"bug, automated" yields " automated", matches no label, and fails the
create -- and the fallback then drops the valid "bug" too, warning about
a missing label when the real cause was a space. Same shape as gha#253.
split-csv-list.sh now splits and trims, one --label per name, with tests
including the internal-space case ("good first issue" is a real label).
3. _selftest.yml's comment claimed the action was already shared with
quarto-publish.yml and check-links.yml; neither is true in this PR.
4. The examples' pull_request clause is inert as those stubs stand; say so
rather than leaving it looking load-bearing.
5. website/permissions.qmd's own per-workflow list was missed when README's
parallel section was updated.
ARD round 1 --- all five addressed in
|
| # | Finding | Disposition |
|---|---|---|
| 1 | report job checks out with contents: none |
Address --- checkout removed entirely |
| 2 | labels not split/trimmed before --label |
Address --- new split-csv-list.sh, one --label per name |
| 3 | _selftest.yml comment claims wiring that does not exist |
Address --- reworded |
| 4 | Inert pull_request clause in the examples |
Address --- kept, comment reworded in both copies |
| 5 | website/permissions.qmd list missed (not tied to a diff line) |
Address --- entry added |
Two of these were real bugs, and both would have surfaced first in a consumer repo rather than here, which is the worst place to find them. Worth recording what each one actually was:
Finding 1 was a permissions bug with a design smell underneath it. The fix took the reviewer's cheaper option --- GH_REPO in the composite's env:, checkout deleted --- because the checkout was not just costing a contents: read grant, it was how gh was learning which repository to file against. That made the composite silently dependent on the caller having checked out the right repo, which is precisely the assumption gha#327's check-links.yml migration would have broken.
Finding 2 is worse than it first reads, and the review's diagnosis is what showed it: a space after a comma fails the create, the fallback then drops the valid labels too, and the warning blames a missing label. Three steps between cause and symptom. It is also the same shape as gha#253, in a repo that had already fixed it once --- so the fix went into a shared, tested script rather than inline.
On the website/permissions.qmd gap, worth noting for its own sake: CLAUDE.md names three doc-sync sites for a new workflow input, and permissions.qmd is not among them because it is not an inputs table. It is still a per-workflow list that drifts independently of its README.md counterpart, and it was missed for exactly that reason. I have not added it to CLAUDE.md's list in this PR --- that is a convention change worth its own change rather than a side effect of this one --- but it is a real gap in the stated procedure.
Verification for the two code fixes:
run-split-csv-list-tests.shcovers ten cases, including the space-after-comma case that motivated it, tabs, empty and comma-only inputs, and a label with internal spaces (good first issueis a real GitHub label, so trimming must not usexargs). Also confirmed the built argument vector directly:--label bug --label automated --label 'good first issue'.- The fallback's guard moved from
-z "$LABELS"to${#labels[@]} -eq 0, which fixes an adjacent case the reviewer did not raise:" , "is a non-empty input yielding no labels, and the old guard would have warned about labels being dropped that never existed.
Generated by Claude Code
Code reviewRe-reviewed after the follow-up commit (
The fifth item ( Two new, non-blocking findings from this round, posted inline:
I also re-checked (and found no new issues with) the items this round's fix commit touched: shell-injection safety of the new Verdict heading below. VerdictReady for merge - the blocking issue from the prior round (missing |
build-reviewer-args.sh now delegates its split/trim to split-csv-list.sh and only reshapes the result into JSON, instead of carrying a second copy of the same loop in the same directory. Its five existing tests pass unchanged. That also retires its `xargs` trim, which interprets quotes and backslashes as shell syntax -- harmless for usernames, but the wrong tool, and the reason the new script did not use it in the first place. The CLAUDE.md Layout bullet named only select-existing-issue.sh. The edit adding the second script was made but never reached a commit: the Tests section's half landed and the Layout section's did not, so the file described a one-script composite while the Tests section below it named both.
|
Claude finished review — View run gh pr comment 326 --repo d-morrison/gha --body "## Code review Re-reviewed after the follow-up commit (`0d90314`, "fix: drop the checkout, and split/trim labels before passing them to gh") that addressed all four issues from the prior review round:
The fifth item (`examples/report-failure.yml`'s dead `pull_request` clause) was cosmetic/non-blocking in the original review; the author left the clause in place and added a comment explaining it's inert for this stub but relevant for consumers who add a `pull_request` trigger — a reasonable disposition for a non-blocking finding. Two new, non-blocking findings from this round, posted inline:
I also re-checked (and found no new issues with) the items this round's fix commit touched: shell-injection safety of the new `GH_REPO`/label/title/body handling (all routed through `env:`, never interpolated into `run:`), the `mapfile < <(...)` pattern used for labels (a real "exit status is swallowed" gotcha, but not reachable under any realistic input since `SPLIT_SCRIPT`'s path resolution is proven by the same `github.action_path` mechanism the sibling `SELECT_SCRIPT` call already exercises end-to-end — not flagging per this review's "don't flag issues that depend on specific/unreachable state" bar), and the `dry-run` gating logic (correctly exits before any real `gh issue` call). VerdictReady for merge — the blocking issue from the prior round (missing `contents: read`) is resolved, and the two new findings in this round are non-blocking DRY/documentation suggestions, not correctness bugs." |
The CLAUDE.md bullet illustrated the untrimmed label by putting it in a code span that begins with a space, which is what markdownlint's no-space-in-code rule exists to catch. Say it in prose instead. Ironically this only surfaced now: lint-markdown was green on the previous commit because the bullet itself had never landed.
…udo-quote The paragraph cited the Layout section for a gap the Tests section describes, and put quotation marks around a phrase that appears nowhere in the file -- it was a paraphrase dressed as a quotation. Both are the kind of claim this repo's own review guidelines ask to be checked against what the document actually says, and neither survived that check.
|
test comment - please ignore, will be replaced |
2026-07-27 18:21 UTC |
Code reviewConfirming review — the previous verdict was Ready for merge, posted here, after commit I re-verified the current head (
I also re-swept doc-sync completeness ( No new issues found in the commits since the last review round ( VerdictReady for merge — the prior verdict stands; all outstanding findings (blocking and non-blocking) have been addressed. |
|
Claude finished review — View run Posted the confirming review: #326 (comment) Summary: All findings from prior review rounds — the VerdictReady for merge |
Ready for mergeFully clean at Checks --- all 30 completed, none failing, including Review --- Ready for merge, confirming the round-2 verdict and re-verifying each earlier finding against the current head. All 7 inline threads resolved. Merge state --- What the four rounds actually caughtWorth recording, because only the first round found a bug in the feature itself:
The last two rounds found nothing but documentation defects, which is the shape of a PR converging rather than churning. Not mergingLeaving that to you. Two notes for when you do:
Generated by Claude Code |
Closes #325.
A workflow that runs where no pull request carries its result --- a push to the default branch, a schedule, a release --- fails silently. The red run shows up in the Actions tab and nowhere else, so a broken deploy can sit for days while the published site goes stale. That is what prompted this:
d-morrison/altdoc's deploy went red four times in three days on an unrelated upstream outage (d-morrison/altdoc#60), and every occurrence was found by a person going looking.What this adds
report-failure.yml--- a reusable workflow filing an issue when a watched job fails. Consumers add it as a final job gated on the job they want watched:open-failure-issue--- the composite doing the work, so the logic has one home rather than being copied to each caller. It wraps two scripts:scripts/select-existing-issue.sh, where the deduplication rule lives, andscripts/split-csv-list.sh, which splits and trims thelabelsinput. Both are unit-tested offline.Three design points worth review
It is a separate workflow, not a job inside
quarto-publish.yml. That was the first approach and it is unshippable: a reusable workflow's jobs can only hold permissions the caller granted, and the docs are explicit that permissions "can only be maintained or reduced---not elevated." A failure-reporting job living insidequarto-publish.ymlwould therefore makeissues: writemandatory for every existing caller of that workflow, wanted or not --- andexamples/quarto-publish.yml's own comment records that this repo has already learned a caller must grant a job's permissions even when that job is skipped, so defaulting the feature off would not have avoided it. Kept separate, the permission is granted only by callers that opt in, and any workflow can be watched rather than only publishing.The reporting job holds
issues: writeand nothing else, and performs no checkout. Naming any scope in apermissions:block zeroes the rest, so a job that checked the repository out would also needcontents: read--- and without it the checkout fails on a private or internal repo, exactly when the job exists to report a failure. Instead the composite names the repository throughGH_REPOand carries its own files viagithub.action_path. That also removes a silent dependency on the caller having checked out the repo being reported against, which need not be the same one (relevant to #327).Repeat failures comment rather than pile up.
check-links.yml's existing copy of this pattern callsgh issue createunconditionally, so a recurring failure files an identical issue every run. This matches on the title first and comments on the open report instead. The match is exact and case-sensitive on purpose:Publish failed: websiteandPublish failed: website previeware different problems, and folding the second onto the first would bury its evidence.A label the calling repository does not define is dropped with a warning and the issue filed anyway --- losing a failure report over a missing label is the worse outcome, and
check-links.yml's hard-codedbug,automated,copilotshows how easily a consumer repo ends up without one.Deferred to #327, with a finding worth recording
The two intended call sites --- migrating
check-links.ymlonto the composite, and dogfoodingreport-failureinwebsite-publish.yml--- were both cut from this PR and moved to #327.A
uses:ref resolves when the job is prepared, before any step runs and before any step-levelif:is evaluated. Socheck-links.ymlreferring toopen-failure-issue@v2failedlinks / link-checkeroutright (run 30251435485) on a step gated behindfailure()that could never have executed --- and_selftest.ymldrivescheck-links.ymlthrough a local./ref, so it went red on every PR. Nothing inside this PR could fix it: the action does not exist at@v2until this merges and the tag slides, and a relative local path is not a workaround (#284: inside a reusable workflow it resolves against the caller's checkout).CLAUDE.mdgains this as a distinction its existing bootstrapping note misses. That note calls the gap a coverage footnote, which is true forrequest-dependabot-reviewbecause it only runs on Dependabot PRs; when the new caller is dogfooded here on every PR, the same gap is a red check instead.Verification
run-select-existing-issue-tests.shcovers the matching rule offline: no-match, prefix-is-not-a-match, case sensitivity, converging on the oldest when duplicates already exist, a title carrying shell and jq metacharacters, and an empty title failing loudly rather than matching arbitrarily.run-split-csv-list-tests.shcovers the splitter: the space-after-comma case that motivated it, tabs, empty and comma-only inputs, and a label whose internal spaces must survive trimming (good first issueis a real label, which is whyxargsis the wrong tool here).build-reviewer-args.shnow delegates to that same splitter rather than carrying a second copy; its five pre-existing tests pass unchanged._selftest.yml'sfailure-issuejob calls the composite through a realuses:step, proving it resolves its helpers viagithub.action_path--- the wiring gha#196 showed a unit test alone does not cover.dry-runexists for this: without it the only end-to-end call would file an issue on this repo every selftest run, so the job needs justissues: read.CLAUDE.md:README.md's workflow table and permissions list,website/workflows.qmd,website/permissions.qmd, the newwebsite/reference/report-failure.qmd,website/_quarto.yml's nav, and every@v2enumeration inREADME.md,website/versioning.qmd,website/workflows.qmd, andCLAUDE.mditself.report-failure.yml's own@v2reference to the composite is the ordinary convention here (quarto-publish.ymlrefers toquarto-publish@v2the same way) and resolves once the tag advances. No workflow in this repo calls it yet, which is why it does not reproduce the problem above.