Skip to content

fix(ci): promote on the push to dev, not on a trigger registered from main (#2822) - #2825

Merged
vybe merged 3 commits into
Abilityai:devfrom
L4XB:fix/2822-issue-status-on-merge-trigger
Sep 16, 2026
Merged

vybe merged 3 commits into
Abilityai:devfrom
L4XB:fix/2822-issue-status-on-merge-trigger

Conversation

@L4XB

@L4XB L4XB commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Description

issue-status-on-merge.yml has produced zero runs since #2769 moved it from
pull_request to pull_request_target on 2026-09-14. Not red — absent.

Both hypotheses in the issue were tested. The types: [closed] one is refuted:
git show b8a790b2 -- .github/workflows/issue-status-on-merge.yml changes exactly
one line in the on: block, pull_request:pull_request_target:, with
types: [closed] and branches: [dev] untouched context lines either side. The
default-branch-registration hypothesis holds.

Why neither trigger could start a run

GitHub reads the two events' definitions from different refs, and after #2769
each of them pointed at a branch whose copy declared the other trigger.

Event GITHUB_REF per GitHub's docs Copy consulted What that copy said
pull_request refs/pull/PULL_REQUEST_NUMBER/merge the merge ref, which resolves against dev pull_request_target
pull_request_target Default branch (GITHUB_SHA: "Last commit on default branch") main pull_request

The pull_request_target section of
Events that trigger workflows
says it plainly: "This event runs in the context of the default branch of the
base repository
, rather than in the context of the merge commit, as the
pull_request event does."
main receives release cuts only (last cut
2026-09-04), so the trigger that decides whether a run starts was being read
from a branch no PR touches.

Worth naming, because it is what made the inference reasonable and because
#2816 concluded the opposite ("the docs describe it only as runs in the context
of the base of the pull request
… and the docs are not [the proof]"): both
wordings are in the docs, behind a version switch.
In
content/actions/reference/workflows-and-actions/events-that-trigger-workflows.md
the sentence is {% ifversion ghes < 3.20 %}base of the pull request{% else %}default branch of the base repository{% endif %}, and the pull_request_target table
has the matching pair of rows — GITHUB_REF: "PR base branch" for GHES < 3.20,
"Default branch" otherwise. github.com renders the second. So the empirical
record and the current documentation agree; only the GHES-era wording says base
branch, and it is the wording this workflow's own header comment quotes as a
security property. That comment is corrected here.

The fix

push to dev. Its GITHUB_REF is the "Updated ref", and the docs are
explicit that push "includes workflows that are not merged into the default
branch"
— so a trigger change to this file takes effect on the merge that lands
it and cannot go dark until a release cut again. That is #2822's fourth
acceptance criterion ("switch to a trigger that is evaluated from the PR's base
branch"
), not merely a way to restore today's promotion. It also means this PR
is self-verifying: its own squash merge is the first real test.

#2767's fix is kept, by a safer route. A push to dev is a base-repo event
whatever the merged PR's origin, so the GITHUB_TOKEN carries exactly the
declared permissions: and a fork author's issue is promoted like anyone
else's — without ever handing a write token to an event a stranger can start.

The pull_request_target safety property is preserved and then made moot.
There is still no actions/checkout and still no run: step, so no PR code is
ever fetched or executed; the only untrusted values read are the PR title and
body, and they still reach exactly one regex that extracts \d+. Nothing is
interpolated into a shell and nothing is eval'd. On top of that, the
escalation surface itself is gone: the workflow no longer runs on an event a
non-collaborator can trigger. tests/unit/test_2767_issue_status_fork_prs.py
still pins every one of those properties.

Resolving the PR from a push. A push payload carries no pull request, so the
merged PR is looked up from the pushed commits with
github.rest.repos.listPullRequestsAssociatedWithCommit. That endpoint requires
the pull_requests permission at read and no other — already declared, so the
token block is unchanged.

Only PRs that merged into the pushed branch are read
(pr.merged_at && pr.base.ref === base), and both halves of that filter are
load-bearing. The endpoint's own description: "Lists the merged pull request
that introduced the commit to the repository. If the commit is not present in
the default branch, it will return merged and open pull requests associated with
the commit.
"
Every dev commit is "not present in the default branch" until a
release cut, so that second sentence is the normal case here, not an edge one.
Measured against b8a790b2:

$ gh api "/repos/Abilityai/trinity/commits/b8a790b2.../pulls" \
    --jq '.[] | "PR #\(.number) base=\(.base.ref) merged_at=\(.merged_at // "null")"'
PR #2632 base=dev merged_at=null
PR #2634 base=dev merged_at=null
PR #2756 base=dev merged_at=null
PR #2769 base=dev merged_at=2026-09-14T14:24:22Z
PR #2793 base=dev merged_at=null
PR #2804 base=dev merged_at=null

Six PRs, one merged and five open — and every one of them base: dev, because
any branch cut from dev afterwards contains the commit. So merged_at is what
stops a single merge promoting the close list of every open PR in flight,
and base.ref is what excludes the devmain release PR, whose body closes
every issue in the release and which merged_at stops excluding the moment the
release lands. Each has its own test.

Measured

$ gh api "/repos/Abilityai/trinity/actions/workflows/265938549/runs?per_page=100" \
    --jq '[.workflow_runs[] | select(.created_at > "2026-09-14T14:24:00Z")] | length'
0

$ gh api "/repos/Abilityai/trinity/actions/runs?event=pull_request_target&per_page=1" --jq .total_count
0

$ gh api "/repos/Abilityai/trinity/actions/workflows/265938549/runs?per_page=1" \
    --jq '.workflow_runs[0] | "run #\(.run_number) \(.created_at) event=\(.event)"'
run #1108 2026-09-14T14:23:38Z event=pull_request

Last run is 44 seconds before #2769's merge commit b8a790b2 (2026-09-14
14:24:22 UTC). Zero pull_request_target runs repo-wide, ever is the
discriminator: if dev's copy were being consulted, the branches: [dev] filter
matches and runs would exist and merely fail. That also rules out the third
candidate in the issue — the branches: filter is not the cause.

Re-counted at 2026-09-15 15:00 UTC, the window is wider than the issue's six:
17 PRs have merged into dev since b8a790b2, 10 of them carrying a closing
keyword naming 11 distinct issues, none of which the workflow promoted —
#2730, #2742, #2349, #2524, #2691, #2692, #2695, #2759, #2794, #2795, #2796.

The control: push: [dev] fired on every one of those merges

This is what makes the fix more than an inference. Sixteen other workflows in this
repo already declare push: branches: [dev], and in the same window, on the
same branch, for the same merges, they ran — while this file ran zero times:

$ gh api "/repos/Abilityai/trinity/actions/workflows/267877290/runs?per_page=5" \
    --jq '.workflow_runs[] | "#\(.run_number) \(.created_at) event=\(.event) branch=\(.head_branch)"'
#1136 2026-09-15T14:58:41Z event=push branch=dev      # ← the merge of #2532
#1135 2026-09-15T14:58:06Z event=push branch=dev      # ← #2773
#1134 2026-09-15T14:50:38Z event=push branch=dev      # ← #2699
#1133 2026-09-15T14:50:16Z event=push branch=dev      # ← #2810
#1132 2026-09-15T14:49:52Z event=push branch=dev      # ← #2761

$ gh api "/repos/Abilityai/trinity/actions/runs?event=push&branch=dev&per_page=100" \
    --jq '[.workflow_runs[] | select(.created_at > "2026-09-14T14:24:22Z")] | length'
100   # (page cap; the point is it is not 0)

deploy-dev.yml is on: push: branches: [dev] and lives on dev under exactly
the same release-cut lag as this file. Same branch, same merges, same lag — the
only difference is which ref the trigger is read from. So the chosen trigger is
not a hypothesis about GitHub's behaviour; it is the behaviour this repository
is already relying on for its deploys.

Acceptance criteria

  • AC 1 (root cause in the header comment) — done; the comment now names the
    registration ref for each candidate trigger and why the old wording misled.
  • AC 2 (a merge promotes with no human involved) — cannot be closed before
    merge; see What I could not verify.
  • AC 3 (the stranded promotions are reconciled) — checked all 11 issues
    above: every one already carries status-in-dev, none is left in
    status-in-progress. Nothing to backfill. One note for a maintainer: bug(mcp): schedule validation config is readable but not writable — create/update_agent_schedule omit validation_enabled/prompt/timeout #2759
    carries a stale status-ready beside its hand-added status-in-dev. No labels
    were touched from this branch.
  • AC 4 (a future trigger change cannot go dark until a release cut) — this is
    the reason push was chosen over backporting the file to main.

Relationship to #2814 / #2816

#2814 is the same outage, reported an hour earlier and reaching the same cause.
#2816 deliberately ships the guard rather than the trigger, recording
"wait for the release cut" as an accepted delay; #2822 asks for the trigger fix
itself. The two are complementary and the tests do not overlap — #2816's is a
cross-workflow dev-vs-main parity sweep, this one is specific to this
workflow's trigger and its promotion logic.

They do collide on one line, whichever merges second: #2816 adds an
ACCEPTED_UNTIL_RELEASE entry for ("issue-status-on-merge.yml", "pull_request_target"), and this PR removes that trigger. Its
test_every_accepted_entry_names_a_real_divergence then fails with
"this branch no longer declares it". Deleting that one dict entry is the whole
follow-up. Happy to rebase behind #2816 if you would rather land the guard
first — and if #2822 lands first, #2816's remaining six entries and its guard
are unaffected.

Related Issue

Closes #2822

Journey Impact

Journey Impact: none: CI issue-label automation — no promise in tests/journeys/catalog.yaml is exercised by a workflow trigger, and no user-facing surface changes.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Testing

  • I have tested this locally
  • New tests added (if applicable)
  • All existing tests pass — one pre-existing failure unrelated to this diff, see below

tests/unit/test_2822_issue_status_trigger_registration.py (new, 15 tests) both
pins the trigger statically and — following test_2533_alembic_head_watch.py's
precedent — executes the workflow's own script: under node with stubbed
context / github / core, because the promotion logic changed shape and a
substring check would not have caught the open-PR case above.

Discriminating run, base = origin/dev @ 7b3cf20a in a separate worktree with
only the test file copied in:

$ pytest unit/test_2822_issue_status_trigger_registration.py -q     # on origin/dev
14 failed, 1 passed

$ pytest unit/test_2822_issue_status_trigger_registration.py -q     # on this branch
15 passed

The one that passes on both is test_the_token_still_covers_the_new_call_and_nothing_more
— an invariant guard, not a regression assertion. On the base the behavioural
tests fail with TypeError: Cannot read properties of undefined (reading 'title'),
which is precisely what a push payload does to context.payload.pull_request.

Behaviour proven by executing the script: a squash merge promotes the issue its
PR closes (from the body and from the title); an open PR that merely contains
the commit promotes nothing; the devmain release PR promotes nothing; a
closed-unmerged PR promotes nothing; every PR in a multi-commit push is
promoted; a push carrying no commits is a no-op rather than a crash; a refused
label write still attempts every issue and still ends core.setFailed (#2767);
a 404 on status-in-progress is still not a failure.

Also run:

$ pytest unit/test_2767_issue_status_fork_prs.py -q
10 passed        # 2 guards rewritten for the new trigger, 8 untouched and green

$ pytest unit/<every file that parses .github/workflows/*, + both lint guards> -q
425 passed, 2 skipped

$ pytest unit/ -q                                    # the whole unit island
1 failed, 16309 passed, 31 skipped in 789.18s

$ actionlint .github/workflows/issue-status-on-merge.yml
(clean)

The single full-suite failure is
test_2582_portal_uploads.py::test_read_inbox_populates_mime_from_the_extension,
which reproduces identically on origin/dev with none of this diff applied
(1 failed, 32 passed in a clean worktree at 7b3cf20a) — pre-existing, and
untouched by anything here. That is why the "all existing tests pass" box above
is left unticked rather than ticked with an asterisk.

actionlint is green on the broken file too — it cannot see this class of
defect. That is why the guard is a unit test and not a linter rule.

Mutation-checked, so the guards are known to bite rather than assumed to (each
mutation asserts it actually changed the file before the run):

Mutation to the workflow Result
drop pr.merged_at && from the PR filter 3 failed
drop && pr.base.ref === base 2 failed
put pull_request_target: types: [closed] back 4 failed
const shas = [] (kill the commits lookup) 1 failed

The last one fails only once because the head_commit fallback correctly
covers the single-commit case; the multi-commit test is what catches it.

What I could not verify

  • That this file's first run happens. A trigger only truly proves itself
    after merge, and GitHub exposes no "which events is this workflow registered
    for" API — GET /actions/workflows/265938549 returns state: active and
    nothing about events. What the control above removes is the doubt about the
    trigger family: push: [dev] demonstrably fires on this branch today. What
    is left unverified is only that this particular file, with this on: block,
    starts run refactor(ui): unify Health + Ops + Executions into a single Operations nav area #1109 — and that is self-verifying: merging this PR should produce
    a push run that moves bug(ci): issue-status-on-merge has not run since its trigger moved to pull_request_target — every merge since 2026-09-14 needed a manual status-in-dev bump #2822 to status-in-dev with no human involved,
    closing AC 2. If no run appears, the diagnosis is wrong and nothing has been
    made worse: the promotion is already at zero.
  • The event payload shape is modelled, not observed. The harness stubs
    payload.commits[].id and payload.head_commit against the documented push
    payload and the documented commits-API response; it proves the script's logic,
    not GitHub's delivery.
  • One behavioural difference, deliberately accepted: push honours
    [skip ci] in a commit message and pull_request_target did not. A squash
    merge whose title contains [skip ci] would skip the promotion. No trinity
    commit message in the last 3000 on dev carries [skip ci] or any of its
    spellings (checked with git log --grep), and the alternative — a trigger that only
    works after a release cut — is what this PR exists to remove.
  • Nothing about the [skip ci] case or the pre-existing test_2582 failure
    was investigated further than stated
    — neither is caused by this diff, and
    chasing either would widen it.

Checklist

  • My code follows the project's style guidelines
  • I have updated the documentation (if applicable)
  • I have not committed any sensitive data (API keys, credentials, etc.)
  • I have added appropriate logging for new functionality

Conventional-commit fix(ci): subject; branch fix/2822-issue-status-on-merge-trigger off dev and
targeting dev; three files, +509/−39 against dev, no unrelated reformatting. The
documentation this change requires is AC 1's header comment on the workflow
itself — a bug fix needs no docs/memory/ update under the tiered rules, and
none is included. The script's core.info lines are unchanged and the new
no-commits path logs before returning, so a run that promotes nothing says why.

Screenshots (if applicable)

Not applicable — a workflow trigger change with no UI surface. The observable
artefact is a workflow run, and the first one can only exist after merge.


File, symbol and API references are against dev @ 7b3cf20a and main @ 646c9314, measured 2026-09-15 ~15:00 UTC. Documentation quotes are from github/docs@main, content/actions/reference/workflows-and-actions/events-that-trigger-workflows.md.

…om `main` (Abilityai#2822)

`issue-status-on-merge.yml` has produced zero runs since Abilityai#2769 moved it to
`pull_request_target` on 2026-09-14. Seventeen PRs have merged into `dev`
since, ten of them carrying a closing keyword for eleven issues, and every one
of those issues was relabelled by hand.

Neither trigger could start a run, for opposite reasons. GitHub reads a
`pull_request` workflow from the PR's merge ref, which resolves against `dev` --
and `dev`'s copy no longer declared it. It reads a `pull_request_target`
workflow from the repository's DEFAULT branch (`GITHUB_REF` is the default
branch, `GITHUB_SHA` its last commit) -- and `main` receives release cuts only,
so `main`'s copy still says `pull_request`. The trigger that decides whether a
run starts was being read from a branch the PR never touches, which is why the
failure was absent rather than red.

`push` is read from the ref being pushed, explicitly including workflows that
are not merged into the default branch, so a trigger change to this file takes
effect on the merge that lands it and cannot go dark until a release cut again.
That is Abilityai#2822's fourth acceptance criterion, not merely a way to restore
today's promotion.

It keeps Abilityai#2767's fix by a safer route: a push to `dev` is a base-repo event
whatever the merged PR's origin, so the token carries the declared
`permissions:` and a fork author's issue is promoted like anyone else's --
without handing a write token to an event a stranger can start.

A push payload carries no pull request, so the merged PR is resolved from the
pushed commits with `listPullRequestsAssociatedWithCommit`, which needs
`pull-requests: read` and nothing more. Only PRs that merged INTO the pushed
branch are read: a commit on `dev` is also in the head of the open `dev` ->
`main` release PR, whose body closes every issue in the release.

Still no `actions/checkout` and no `run:` step.

Closes Abilityai#2822
`listPullRequestsAssociatedWithCommit` documents that for a commit "not
present in the default branch" -- which every `dev` commit is until a release
cut -- it returns "merged AND open pull requests associated with the commit".

Measured on `b8a790b2`: six PRs come back, one merged and five OPEN, and every
one of them is `base: dev`, because any branch cut from `dev` after that commit
contains it. So the release PR was the smaller half of the story: `merged_at`
is what stops one merge promoting the close list of every open PR in flight,
and `base.ref` is what excludes the `dev` -> `main` release PR. The comment
said only the second; both are now stated with the measurement behind them,
and each has its own test.
@L4XB
L4XB force-pushed the fix/2822-issue-status-on-merge-trigger branch from 367a0ad to 04e103f Compare September 15, 2026 22:01
@L4XB

L4XB commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto dev. The only conflict was the header comment: #2814 landed a block on dev explaining that pull_request_target is registered from the default branch, which is the same failure this PR fixes by moving to push.

Kept this branch's header, since it documents the trigger the file now has, and added a line noting that push also takes the file out of the scope of tests/unit/test_2814_workflow_trigger_parity.py — that check governs events GitHub registers from the default branch, and it asserts push is not one of them (line 258). The divergence it was written to catch is the one this change removes.

No YAML or job-body changes in the rebase, so what test_2767_issue_status_fork_prs.py and test_2822_issue_status_trigger_registration.py assert on is untouched: the parsed trigger is still {'push': {'branches': ['dev']}} with neither pull_request nor pull_request_target present.

…s stale

test_2814_workflow_trigger_parity::test_every_accepted_entry_names_a_real_divergence
fails once this branch stops declaring pull_request_target on
issue-status-on-merge.yml: the ACCEPTED_UNTIL_RELEASE entry recording that
divergence no longer matches anything, which is exactly what that guard is for.

The entry existed because the promotion was accepted as broken until the next
release cut. Moving the trigger to push: branches: [dev] removes the need to
wait for a release at all, so the record goes with it.
@L4XB

L4XB commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

regression diff was red on one new failure and it was mine — fixed in 4eabb2c7.

## ❌ New failures introduced by HEAD (1)
- [F] test_2814_workflow_trigger_parity::test_every_accepted_entry_names_a_real_divergence

ACCEPTED_UNTIL_RELEASE carried an entry for ("issue-status-on-merge.yml", "pull_request_target"). Once this branch stops declaring that event, the entry matches nothing, and test_every_accepted_entry_names_a_real_divergence fails it by design — "An allowlist entry that no longer matches anything is a decision nobody can find the subject of. Fail it so the record is pruned when the release closes the gap." The guard did exactly its job.

I pruned the entry rather than amending it, because the decision it recorded no longer applies. It read:

Decided 2026-09-15 to let it land with the next release cut rather than hotfix main; until then a merged Fixes #N promotes nothing and the issue is relabelled by hand.

The whole point of moving to push: branches: [dev] is that it works on merge today — no release cut, no hand-relabelling, nothing to wait for. So the record goes with the workaround it recorded.

Discriminating, both runs local on this branch:

with the entry present : 1 failed, 30 passed
with the entry removed : 31 passed

No other allowlist entry is touched; deploy-dev.yml, alembic-head-watch.yml and journey-smoke.yml still declare the events theirs record. test_pr_triggers_are_deliberately_out_of_scope is unaffected and still passes — push is deliberately not in DEFAULT_BRANCH_REGISTERED_EVENTS, which is precisely why the new trigger needs no allowlist entry of its own.

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge-train: batch validated on train/20260916-0919 (train PR #2839)

@vybe
vybe merged commit f3fdcfd into Abilityai:dev Sep 16, 2026
25 checks passed
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.

2 participants