From 474e8bd9729405208a5a1c79be79c518fe77d235 Mon Sep 17 00:00:00 2001 From: JJ Fullmer Date: Mon, 17 Aug 2026 21:35:44 -0600 Subject: [PATCH] Say pull_request, because pull_request_target never fired #17 described the merge entry point as `pull_request_target: closed`. That was what fogproject's stub used, and it did not work: GitHub reads a `pull_request_target` workflow from the repository's DEFAULT branch, `stable`, rather than from the pull request's base branch, so a stub living on working-1.6/dev-branch was never consulted. It never registered as a workflow at all, and four PRs merged into working-1.6 without it running. fogproject has moved the stub to `pull_request: types: [closed]`, which is read from the base branch and so reaches the file where it actually lives. This brings the description here back in line with what the caller does. Records why, not just what. `pull_request_target` is the obvious-looking choice -- it is the variant that gets secrets on a fork PR -- so without the reason written down the natural next move is to switch it back. The cost of `pull_request` is that a merged fork PR has no secrets and is skipped by the stub's same-repo guard; the schedule already covers those, as it does direct pushes. Adds a second standing rule alongside the trigger-safety one, because they are different questions and only the first was written down. The safety question is "can the bot's own push raise this event". The new one is "which ref is this trigger read from" -- most events, including pull_request_target, schedule and workflow_dispatch, are read only from the default branch, while push, create and pull_request resolve per-ref. A workflow that is correct but never runs is indistinguishable from one that ran and found nothing to do, which is exactly how this went unnoticed, so the rule ends by saying to confirm a new trigger actually produced a run. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PqufBbuckux8kitJeW3uAK --- .../update-lang-fix-psr-and-sync-version.yml | 31 +++++++++++------ CLAUDE.md | 34 +++++++++++++------ 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/.github/workflows/update-lang-fix-psr-and-sync-version.yml b/.github/workflows/update-lang-fix-psr-and-sync-version.yml index dc93101..5cf3c8f 100644 --- a/.github/workflows/update-lang-fix-psr-and-sync-version.yml +++ b/.github/workflows/update-lang-fix-psr-and-sync-version.yml @@ -59,18 +59,29 @@ name: Update Language Files, Fix PSR2, and Sync FOG Version # version stays wrong until the next cron tick, up to a day later. # # The property that makes it safe is the event, not restraint. That stub -# triggers on `pull_request_target: closed`, and the push below is a direct -# push to a branch, not a PR merge -- so the bot cannot raise the event that -# would call it again. The loop is closed by the shape of the trigger, the -# same way a cron tick cannot feed itself, rather than by an actor-name -# guard that has to be kept correct. +# triggers on `pull_request: types: [closed]`, and the push below is a +# direct push to a branch, not a PR merge -- so the bot cannot raise the +# event that would call it again. The loop is closed by the shape of the +# trigger, the same way a cron tick cannot feed itself, rather than by an +# actor-name guard that has to be kept correct. +# +# It is `pull_request` and not `pull_request_target`, which is worth knowing +# because the latter looks like the better choice: it is the variant that +# gets secrets on a fork PR. It was tried first and never fired once. +# GitHub reads a `pull_request_target` workflow from the repository's +# DEFAULT branch (`stable`), not from the PR's base branch, so a stub living +# on working-1.6/dev-branch is never consulted -- it did not even register +# as a workflow, and four PRs merged without it running. The cost of using +# `pull_request` instead is that a merged FORK PR gets no secrets and is +# therefore skipped by the stub's same-repo guard; the schedule below picks +# those up, exactly as it does direct pushes. # # The schedule stays exactly as it is, and is still the only cover for -# direct pushes and for rc-*/feature-* branches. rc-* in particular must -# NOT be merge-synced: fog-version.sh increments rc off the committed -# suffix rather than a commit count, so it reports drift on every run, and -# a per-merge sync there would bump the RC suffix on every merge. The -# stub's branch allowlist is what keeps rc-* on the schedule. +# direct pushes, for merged fork PRs, and for rc-*/feature-* branches. rc-* +# in particular must NOT be merge-synced: fog-version.sh increments rc off +# the committed suffix rather than a commit count, so it reports drift on +# every run, and a per-merge sync there would bump the RC suffix on every +# merge. The stub's branch allowlist is what keeps rc-* on the schedule. # # Never touches `stable` - the branch filter below excludes it, because # that branch's version and contents are owned by stable-releases.yml. diff --git a/CLAUDE.md b/CLAUDE.md index 29fa1fa..df9db1e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -78,18 +78,26 @@ don't "simplify" those back to the default `GITHUB_TOKEN` just because the call `github.token`: under `workflow_call` that is the *caller's* token, which cannot write to another repository. - It runs on a **schedule** and on **merge**, and the difference between those two entry - points matters. The schedule is the only cover for direct pushes and for `rc-*`/`feature-*`. - The merge path is `fogproject`'s `sync-generated-files.yml`, which calls this over - `workflow_call` on `pull_request_target: closed` for merges into `working-1.6`/`dev-branch`, - and exists because the pre-commit hook is client-side and never runs for a PR merged in the - web UI. + points matters. The schedule is the only cover for direct pushes, for merged fork PRs, and + for `rc-*`/`feature-*`. The merge path is `fogproject`'s `sync-generated-files.yml`, which + calls this over `workflow_call` on `pull_request: types: [closed]` for merges into + `working-1.6`/`dev-branch`, and exists because the pre-commit hook is client-side and never + runs for a PR merged in the web UI. + - **`pull_request`, not `pull_request_target`** — worth knowing, because the latter looks like + the better choice (it is the variant that gets secrets on a fork PR) and was tried first. It + never fired once: GitHub reads a `pull_request_target` workflow from the repository's + **default** branch (`stable`), not from the PR's base branch, so a stub on + `working-1.6`/`dev-branch` is never consulted — it did not even register as a workflow, and + four PRs merged without it running. `pull_request` is read from the base branch. The cost is + that a merged fork PR has no secrets, so the stub's same-repo guard skips it and the + schedule picks it up. - It is **not push-triggered**, on purpose: an earlier push-triggered design (a stub in `fogproject` calling this as a reusable workflow) caused a runaway loop — the bot's own fixup push re-triggered the stub, which re-ran this workflow, which pushed another fixup, - producing ~30 unwanted commits in ~20 minutes. A `pull_request_target` merge event is safe - where a push event is not, because the bot pushes directly to the branch and a direct push - is not a PR merge — it cannot raise the event that calls this again. See the comment block - at the top of the file before changing the trigger model. + producing ~30 unwanted commits in ~20 minutes. A merge event is safe where a push event is + not, because the bot pushes directly to the branch and a direct push is not a PR merge — it + cannot raise the event that calls this again. See the comment block at the top of the file + before changing the trigger model. - Never runs against `stable` — that branch's version is owned exclusively by `stable-releases.yml`. - This used to be two separate workflows (a version check and a generated-files sync) on a @@ -168,9 +176,15 @@ don't "simplify" those back to the default `GITHUB_TOKEN` just because the call that means **never `push`**, per the incident documented in `update-lang-fix-psr-and-sync-version.yml`: the bot pushes its fixup directly to the branch, a `push` trigger sees it, and the workflow feeds itself. Scheduled, - `workflow_call`/`workflow_dispatch`, and `pull_request_target: closed` (a merge — which the + `workflow_call`/`workflow_dispatch`, and `pull_request: types: [closed]` (a merge — which the bot's direct push is not) are all safe for the same reason, and all three are in use. The test to apply to a new trigger is that one question, not whether it happens to be a cron. +- Separately from that safety question, check **which ref a trigger is read from** before + relying on it. Most events — including `pull_request_target`, `schedule` and + `workflow_dispatch` — are read only from the repository's default branch, so a workflow file + on a non-default branch is silently never consulted; `push`, `create` and `pull_request` + resolve per-ref. A workflow that is correct but never runs looks exactly like one that ran and + found nothing to do, so confirm a new trigger actually produced a run rather than assuming it. - Keep version/translation-generation logic (`fog-version.sh` / `apply-fog-version.sh` / `update-language.sh`) living in `fogproject`, not duplicated here — this repo should only *call* those scripts. PSR2 formatting is the one exception, since fogproject only ever invokes