Skip to content

Report failed pull request runs, and state that a failed post reaches nobody - #14

Merged
GavinLucas merged 2 commits into
mainfrom
report-pull-request-failures
Oct 9, 2026
Merged

GavinLucas merged 2 commits into
mainfrom
report-pull-request-failures

Conversation

@GavinLucas

@GavinLucas GavinLucas commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

What

  • Pull request runs are reported. The hygiene check counts pull_request alongside schedule, push and release, so each repository must list its pull request workflows in report-failures.yaml with pull_request among the watcher's events. The reporter itself is unchanged: it posts any trigger the caller lists. Manual runs stay out.
  • A failed post reaches nobody, stated as an accepted limitation in the README and the reporter's header, replacing the claim that a broken webhook shows as a red run "rather than a failure that reached nobody". The reasons the alternatives were not taken are given: an issue in the failing repository scatters them, and one here needs a cross-repository credential or a scheduled sweep.
  • This repository's watcher lists Code review, which the extended check found.

Effect on other repositories

Nothing changes in a repository until its pins move to the release carrying this. The pull request that moves them must also add pull_request to the watcher's events and list the repository's pull request workflows, or the hygiene check fails. A Dependabot pin bump alone would fail the same way.

A pull request from a fork is not posted: its run uses the fork's own workflow file, so its author writes the job names and branch a post would carry, and could put text of their choosing in the channel. The reporter skips a pull_request run whose head repository is not the repository itself.

Testing

  • actionlint clean.
  • The check's function against eighteen fixtures, each passing or giving its intended finding, including a listed pull request workflow, one left off, and pull_request missing from events.
  • The whole script over a fresh clone of each repository at its current branch: each gets exactly one finding per pull request workflow it has yet to list. This repository's own workflows give none.

… nobody

A failing pull request run is worth a post too: such failures are rare, and the run's page is
easy to miss.  The hygiene check now counts `pull_request` with `schedule`, `push` and
`release`, so every repository must list its pull request workflows in its watcher with that
trigger among its `events`, and the check's label and messages no longer call these runs
unattended.  The reporter needs no change: it already posts any trigger the caller lists.  A
manual run is still left out, because whoever started it is watching.

A pull request from a fork posts as well, through the base repository's own watcher and secret,
once GitHub has let its checks run.

The README and the reporter's header said a broken webhook shows as a red watcher run "rather
than a failure that reached nobody".  The watcher's run is itself unwatched and nothing reports
it, so a webhook that stops working is noticed only by the absence of posts.  Both now state
that as an accepted limitation, with the reasons the alternatives were not taken.

This repository's own watcher lists its Code review workflow, which the extended check found.

Tested: the check's function against eighteen fixtures, including a listed pull request
workflow, one left off, and `pull_request` missing from `events`; and the whole script over a
fresh clone of each repository, where it names exactly the pull request workflows each has yet
to list.

Signed-off-by: Gavin Lucas <Gavin@L337.org>
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

Claude review: changes requested

Summary

The change adds pull_request to the triggers that get reported. It does this in three places: the hygiene check's REPORTED, this repository's watcher, and the README example. It also corrects the claim that a broken webhook "shows as a red watcher run rather than a failure that reached nobody", and records that limitation as accepted. That correction is right: nothing reports the watcher's own run. The rename from "unattended" to "reported" is applied consistently across the constant, the function names, the docstring, the CHECKS label and the finding text.

One problem should be settled before merge. Because the hygiene check now requires every repository to report pull_request, every repository also posts text that a fork's author wrote into its Slack channel. Two smaller documentation leftovers and one narrower edge case follow.

Findings

1. Bug (security): fork pull requests can post text they choose to the organisation's Slack channel, and the hygiene check now makes every repository opt in

Where: .github/workflows/slack-on-failure.yaml (the jobs and BRANCH values that go into text), actions/repo-hygiene/check-repo-hygiene.py:112, README.md:136

  • How the content gets in. A pull_request run from a fork uses the workflow file from the pull request's merge ref, so the fork's author writes it. They control:

    • the job names. The reporter lists failed jobs as "\(.name) (\(.conclusion))" and posts them.
    • head_branch, which is posted as on \$BRANCH``.

    The workflow's name: must still match the watcher's list, but the author can keep that unchanged.

  • What escaping does and doesn't stop. escape() replaces &, < and >, so the author can't use <!channel> or a <url|label> link. Slack still turns bare URLs into links, though. A post like Jobs: Your SSO session has expired, sign in again at https://… (failure) would arrive in an internal channel through the organisation's own webhook, looking like CI output. The author can repeat it on every push, once GitHub's fork approval setting lets their runs start.

  • Why this is new. Before this PR the reporter accepted pull_request, but no caller listed it, and the check didn't ask for it. Now REPORTED makes the check fail in any repository whose pull request workflows aren't reported for pull_request. Every repository that adopts the check gets this exposure.

  • It contradicts an existing rule. claude-review.yaml's gate refuses fork pull requests "because the reviewed text is written by someone with no access while the run carries the token". The same reasoning applies here: the watcher holds SLACK_WEBHOOK, and the posted text comes from the fork.

  • The README states the behaviour but not the risk. README.md:136 says "a fork's failing checks post too", which reads as an intended feature.

Suggested fix: in the reporter's if:, skip a pull_request run whose github.event.workflow_run.head_repository.full_name differs from github.repository. Alternatively, post fork runs with only the fields the base repository controls: repository, workflow, conclusion and run link, without job names or branch. Then update the README sentence to say which one was chosen.

2. Bug (minor): a pull request run cancelled by its timeout can be hidden by a different pull request's run

Where: .github/workflows/slack-on-failure.yaml, the cancelled-run check

  • The newer-run lookup filters on branch=$BRANCH&event=$TRIGGER. For pull request runs, head_branch is just the branch name, with no fork owner. Fork pull requests often share names like main or patch-1.
  • So a timed-out run on one pull request isn't posted if any later pull request run, from any pull request with the same head branch name, exists.
  • The accepted limitation in the README and the header covers a newer run "of the same workflow" already queued. This case is wider and isn't stated anywhere.
  • If finding 1 is fixed by skipping fork runs, this mostly goes away. Otherwise, compare head_repository or the pull request number as well, or add this case to the stated limitation.

3. Maintainability: the README's table still describes the reporter as posting only unwatched runs

  • README.md:12 says "posts a failed run nobody was watching to Slack".
  • After this change, the reporter posts pull request runs too, which the commit itself treats as watched by their author but easy to miss.
  • The same PR changed the section text and the hygiene row in that table, but not this row. It now disagrees with README.md:104-106.

4. Maintainability: the hygiene finding's example events value leaves out pull_request

  • actions/repo-hygiene/check-repo-hygiene.py:570 still suggests '["schedule", "push", "release"]' when a watcher's events doesn't parse.
  • Someone who copies it will get a second finding for every pull request workflow.
  • The example in the reporter's input description and in the README were both updated to include pull_request. This one should match.

Recorded-state check

I went through every added or edited line in the README, the workflow comments and input descriptions, and the script's comments and docstrings. None of them records state that will go out of date: they state rules, reasons, and accepted limitations named as limitations. The "Tested:" paragraph is in the commit message, which is the right place for it.

What I checked and what I didn't

Checked:

  • The full diff, the whole of slack-on-failure.yaml, report-failures.yaml, premerge.yaml and code-review.yaml.
  • The claude-review.yaml gate and its failure paths:
    • a changes_requested verdict doesn't fail the run, so routine reviews won't post;
    • fork and Dependabot runs end in a skip notice rather than a failure;
    • issue_comment-triggered review runs are outside REPORTED and the watcher's events, which is consistent.
  • The hygiene check's reported_run_findings logic after the rename.
  • A search for any remaining "unattended" or "nobody watching" wording.

Not checked:

  • I couldn't run the hygiene script or the commit's fixtures here: the command needed approval.
  • I didn't confirm this repository's visibility or its fork-approval setting through the API.
  • I didn't test how Slack renders a crafted job name. Finding 1 relies on Slack documenting that it turns bare URLs in webhook text into links.

full review of c406e0fddc7d..de52f7cec631. Run.

A pull request from a fork runs the fork's own copy of the workflow file, so its author writes
the job names and the branch name the post would carry.  The reporter escapes Slack's markup,
but a bare URL still becomes a link, so a fork could put text of its choosing - a fake sign-in
prompt, say - in the channel through the repository's own webhook, once its runs are allowed to
start.  The reporter now skips a `pull_request` run whose head repository is not the
repository itself, as the review refuses forks for the same reason.  Its failure still shows
on the pull request.  This also removes the case where a fork's run on a common branch name,
such as `patch-1`, hid another pull request's cancelled run.

The README's table no longer describes the reporter as posting only unwatched runs, and the
hygiene check's example `events` now includes `pull_request`, matching the others.

Signed-off-by: Gavin Lucas <Gavin@L337.org>
@GavinLucas

Copy link
Copy Markdown
Contributor Author

Addressed in 8fb9b45:

  1. Fork pull requests. Taken, by skipping them: the reporter's if: now requires a pull_request run's workflow_run.head_repository.full_name to equal github.repository. A same-repository pull request is still posted. The header comment and README say why, matching the review's refusal of forks. Confirmed on a real run object that head_repository.full_name is present and equals the repository for a same-repository pull request.
  2. A timeout hidden by another pull request's run. Resolved by 1: only same-repository pull requests are posted, and branch names within one repository are unique.
  3. The README table. Taken: it now says the reporter posts a failed run to Slack.
  4. The example events. Taken: it now includes pull_request.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

Claude review: approve

Delta review of de52f7c..8fb9b45

Resolved

  • Finding 1 (fork pull requests post author-chosen text): fixed. slack-on-failure.yaml:40-41 now requires a pull_request run's head_repository.full_name to equal github.repository.
    • A run whose head_repository is null, for example from a deleted fork, makes the comparison false, so it is skipped. That fails safe.
    • Other triggers pass the first clause, so their behaviour is unchanged.
    • The header comment and README.md:136-139 give the reason and say the failure still shows on the pull request.
    • A search for "fork" across the repository finds no remaining text that says forks are posted.
  • Finding 2 (a timeout hidden by another pull request's run): resolved by finding 1. Within one repository, head branch names are unique, so the newer-run lookup can't match a different pull request.
  • Finding 3 (README table row): fixed. The row now says "posts a failed run to Slack".
  • Finding 4 (example events value): fixed. check-repo-hygiene.py:570 now includes pull_request.

Still open

None.

Declined

None.

New

None. Two things I considered and left alone:

  • A same-repository pull request is still posted. Its author needs write access, so the same trust applies as for a push. This is stated in the header comment and the README.
  • The expression's || and && nesting is correct.

Checked and not checked

Checked: the full delta, the if: expression in context, the wording in the README and header comment, and a search for stale fork wording.

Not checked: I didn't run the hygiene script or a live workflow. The author reports confirming on a real run object that head_repository.full_name is present and matches the repository for a same-repository pull request.

Recorded-state check: the added comments and README text state rules and their reasons. None of them records state that will go stale.


delta review of de52f7cec631..8fb9b45d64c5. Run.

@GavinLucas
GavinLucas merged commit fd692a6 into main Oct 9, 2026
7 checks passed
@GavinLucas
GavinLucas deleted the report-pull-request-failures branch October 9, 2026 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant