feat(standards-sync): arm auto-merge at PR creation and alert on stuck armed PRs - #213
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96de87a01e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…k armed PRs Across 7 repos, 193 of 193 merged standards-sync PRs were merged by one human (18 via human-armed auto-merge, 175 manual, 0 by the App) — the sync PR body is gate-compliant by construction, so this was a recurring manual chore with no safety benefit. The companion risk (medley#1619): an auto-merge-armed sync PR sat silently BLOCKED for hours by an unrelated pre-existing ci-status failure, with nothing watching. - standards-sync.yml now arms auto-merge (squash) via GraphQL on a newly created sync PR, using the same target-scoped App token the PR was opened with. Gated on PR-creation only (never a later update, so a reviewer who deliberately disarmed a PR is not overridden) and on the manifest's new per-target `automerge` flag (standards#246). A rejected mutation (for example an already-mergeable PR) is logged and does not fail the sync. - New standards-sync-stuck-automerge-alert.yml reusable: scans the manifest's target repos (read at run time, never hardcoded) for open, App-authored PRs with auto-merge armed and GraphQL mergeStateStatus BLOCKED past a threshold (default 4h). Reuses the existing marker-deduped tracking-issue pattern (find-tracking-issue.sh, now a fourth consumer of the generated block) for one rolling aggregate issue in the caller's own repo, and fails the scheduled run so a stuck PR notifies through both channels. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYvF6bWGqemS9aYFfWJRiW
…se onto #212) ci-workflows#212 (merged during this branch's lifetime) ported link-check.yml's gh-CLI-based tracking-issue lookup/close steps to actions/github-script, because it runs on a caller-selected runner where the gh CLI is not guaranteed present, and dropped it from the shared find-tracking-issue.sh codegen consumers accordingly. standards-sync-stuck-automerge-alert.yml is the same shape (workflow_call, runs-on: inputs.runner), so it needed the same port rather than joining the shared bash consumer list as originally written. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYvF6bWGqemS9aYFfWJRiW
96de87a to
531bf85
Compare
|
Claude finished @kyle-sexton's task in 3m 52s —— View job PR Review: feat(standards-sync) auto-merge arming + stuck-PR alert
Overall AssessmentThe design is solid — the arming gate logic, the token scoping, the HIGH — Pagination gap in the stuck-PR scanFile: The GraphQL query fetches exactly The test suite exercises multi-repo aggregation and the Fix: add let after = null;
do {
const result = await github.graphql(query, { owner, repo, after });
const { nodes, pageInfo } = result.repository.pullRequests;
for (const pr of nodes) { if (isStuck(pr)) { ... } }
after = pageInfo.hasNextPage ? pageInfo.endCursor : null;
} while (after);MEDIUM — GraphQL errors in the scan loop abort the step and confuse the downstream guardsFile: The per-repo This causes the "Open or update tracking issue" step to run — but the report file ( Fix: wrap each LOW —
|
|
Claude finished @kyle-sexton's task in 2m 34s —— View job Security Review
FindingsIMPORTANT — Logic flaw: Watchdog silently drops stuck PRs when a target has > 20 open PRsConfidence: HIGH The GraphQL query in the ci-workflows/.github/workflows/standards-sync-stuck-automerge-alert.yml Lines 132 to 145 in 531bf85 A This is amplified by the fact that the arming step ( Codex flagged the same issue (P2 in the inline review comment). The fix is straightforward: replace the No other security issues foundThe remaining surface area is clean:
|
|
Claude finished @kyle-sexton's task in 1m 32s —— View job Re-review: feat(standards-sync) auto-merge arming + stuck-PR alert
The two new commits are purely maintenance — a biome line-wrap style fix ( Still OPEN — HIGH: Pagination gap silently drops stuck PRsFile: The query still uses The REST issue-list steps already use let after = null;
do {
const result = await github.graphql(query, { owner, repo, after });
const { nodes, pageInfo } = result.repository.pullRequests;
for (const pr of nodes) {
if (isStuck(pr)) { /* ... */ }
}
after = pageInfo.hasNextPage ? pageInfo.endCursor : null;
} while (after);The query signature needs Still OPEN — MEDIUM: Unguarded
|
| Commit | What it changed |
|---|---|
2e86bdd |
Merge origin/main into this branch — no logic changes to the PR's own files |
a6252c3 |
Biome line-length style fix in the two new test files — no test logic changed |
No findings were addressed. The two correctness issues (pagination gap, unguarded scan-loop graphql) need to be fixed before merge.
|
Claude finished @kyle-sexton's task in 3m 0s —— View job Security Review
No security vulnerabilities foundThe two prior reviews (code and security) flagged a pagination gap and an unguarded GraphQL error path. Neither is a security vulnerability — they are correctness/reliability gaps (the watchdog can silently miss a stuck PR; an unhandled error causes a confusing secondary failure). The current HEAD commit ( Below is a full accounting of the surfaces I checked. Trigger hygiene — PASSBoth new/modified workflows use Script injection — PASSEvery Arming step ( Scan step ( Fail step ( Auto-merge privilege — PASSThe There is no attacker-controlled path to arm auto-merge on an arbitrary PR without compromising either the App credentials or the SHA-pinned CPR action. The Token handling — PASS
SHA pinning — PASSAll new
No mutable tags.
|
Babysit triage — automated review findingsSafe-tier babysit pass. CI was red only on biome formatting (the The review findings below are validated against the branch code and surfaced for author decision — none were auto-applied: each is a logic/design change to a reusable workflow (blast radius across every consumer repo), which this conservative pass deliberately does not guess at. No threads resolved. Covers claude[bot] PR review (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6252c3530
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…estrict issue adoption to the bot author Two real findings from review (Codex threads PRRT_kwDOTCB_B86S_Vu5 and PRRT_kwDOTCB_B86TBs1E): - The per-target GraphQL query fetched only the first 20 open PRs with no pagination, so an armed sync PR sorted past that page would be invisible — and a scan that silently missed it would still report stuck-count=0 and close the existing tracking issue, converting a real incident into a false recovery. Verified @actions/github bundles @octokit/plugin-paginate-rest but no GraphQL pagination plugin, so github.paginate cannot walk this query; added a manual pageInfo.hasNextPage/endCursor cursor loop bounded by MAX_PAGES (fails closed with core.setFailed rather than hanging or under-scanning on a pathological repo). - Tracking-issue adoption matched on marker/title text alone, and both strings are public (embedded verbatim in this workflow file). In a caller repo where non-maintainers can open issues, a decoy carrying the marker would be adopted — at minimum closed on the next healthy run, or, with two decoys, tripping the ambiguity check and failing the lookup closed, which suppresses a real alert entirely (the "Open or update tracking issue" step never runs). Verified empirically against this repo's own tracking issue (#218, opened by tool-version-drift-check.yml's default GITHUB_TOKEN) that such issues are authored by github-actions[bot]; both the lookup and close steps now filter candidates to that author before matching marker or title, so an attacker-authored decoy is never a candidate regardless of its text. link-check.yml and queue-monitor-liveness.yml's tracking-issue steps share this same author-unrestricted-adoption exposure — noted as a follow-up candidate in the PR, not fixed here (out of this PR's scope). Added direct execution tests for both fixes: multi-page GraphQL pagination (a stuck PR on page 2, a false all-clear must fetch every page, and the MAX_PAGES guard), and issue adoption (decoy-by-marker, decoy-by-title, two decoys not falsely triggering the ambiguity fail-closed, and a genuine ambiguity between two bot-authored issues still failing closed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYvF6bWGqemS9aYFfWJRiW
|
Claude finished @kyle-sexton's task in 2m 59s —— View job Re-review after
|
| Finding | Status |
|---|---|
| HIGH — pagination gap (first: 20, no cursor) | FIXED — manual cursor loop + MAX_PAGES guard + 3 tests |
| Codex P2 — decoy issue injection via public marker/title | ADDRESSED — bot-author restriction on both lookup and close steps + 7 tests |
MEDIUM — unguarded per-repo github.graphql() |
Still open — reliability gap, not a correctness bug |
LOW — UNKNOWN mergeStateStatus excluded |
Still open — design tradeoff, worth a comment |
| LOW — pre-marker close path edge case | Still open — acceptable; one-time deployment edge case |
The two original correctness blockers are resolved. The MEDIUM finding downgraded from a correctness concern once the GitHub Actions step-failure semantics are considered. This PR is in good shape to merge.
|
Claude finished @kyle-sexton's task in 3m 32s —— View job Security Review — feat(standards-sync) auto-merge arming + stuck-PR alert
No new security vulnerabilities foundThis is the third security pass on this PR. The HEAD commit ( Prior HIGH finding — Pagination gap — FIXEDThe prior reviews flagged that the GraphQL PR query used
Remaining MEDIUM — Unguarded
|
| Token | Scope | Usage |
|---|---|---|
Alert scan token (steps.token.outputs.token) |
permission-pull-requests: read, scoped to manifest target repos |
Read-only GraphQL PR query |
Arming token (steps.token.outputs.token) |
permission-pull-requests: write, scoped to one target repo |
enablePullRequestAutoMerge mutation |
Default GITHUB_TOKEN |
issues: write on the caller repo only |
Opens/closes the rolling tracking issue in the caller's repo |
No token is echoed or logged. No new permission grants beyond the existing sync job's scope.
GITHUB_OUTPUT injection via repo-names — PASS
echo "repo-names=${repo_names}" >> "$GITHUB_OUTPUT"repo_names is constructed from jq -r '[.include[].repo_name] | join(",")' <<<"$matrix". GitHub repository names are restricted to alphanumeric, hyphens, underscores, and dots — they cannot contain newlines or = characters, so GITHUB_OUTPUT key injection is not achievable through the manifest.
Issue body content (markdown/XSS) — PASS
The report body includes entry.repo (manifest-derived internal name), entry.number (PR number integer from GraphQL), and entry.url (canonical GitHub PR URL from GraphQL). All originate from GitHub's own API. GitHub also sanitizes Markdown on render.
SHA pinning — PASS
All new uses: references in this PR are fully SHA-pinned with version comments per CLAUDE.md. No mutable tags.
Summary
The pagination gap (HIGH) is fixed. The unguarded per-repo github.graphql() call (MEDIUM from prior reviews) remains a watchdog-reliability concern but is not a security vulnerability. No new security issues are introduced by this PR.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2be1cd341
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (pullRequest.author?.login !== botLogin) return false; | ||
| const armedAt = pullRequest.autoMergeRequest?.enabledAt; | ||
| if (!armedAt) return false; | ||
| if (pullRequest.mergeStateStatus !== 'BLOCKED') return false; |
There was a problem hiding this comment.
For armed sync PRs blocked by a failed required check, this predicate reports all clear because it only accepts BLOCKED; GitHub's MergeStateStatus enum separately uses UNSTABLE for a mergeable PR with a non-passing status (docs). In that scenario the watchdog emits stuck-count=0, and the recovery step can close the rolling issue even though auto-merge is still unable to complete.
Useful? React with 👍 / 👎.
| # checks on main, so a fresh PR is never immediately mergeable — except | ||
| # the rare case GitHub's mutation itself rejects, handled below. | ||
| - name: Arm auto-merge on the newly created sync PR | ||
| if: steps.cpr.outputs.pull-request-operation == 'created' && matrix.automerge |
There was a problem hiding this comment.
When the checked-out standards ref does not yet emit the new automerge matrix key, matrix.automerge evaluates as a missing/null value and GitHub conditionals coerce that to false (docs). That disables arming for every target during the advertised absent-key-safe rollout path instead of preserving the documented default of auto-merge on unless the manifest explicitly sets automerge: false.
Useful? React with 👍 / 👎.
…as the App, not the ambient token (#223) No related issue: design amendment to merged #213 (runner-policy contract conflict) ## Related #210, #213, melodic-software/standards components/runner-policy ## Summary - The reusable already mints a read-only App token to scan every standards-sync target repo for stuck armed pull requests, but it still opens/updates/closes its own tracking issue with the ambient `GITHUB_TOKEN`, authored as `github-actions[bot]`. - That only works if the CALLING job explicitly grants `issues: write` (cross-org reusable workflows cannot elevate their own ambient-token scope beyond what the caller's job permissions declare — GitHub's own reusable-workflow calling example shows `permissions:` set on the job with `uses:`, and this repo's own `runner-policy` precedent for `link-check.yml`/`pulumi-version-drift-check.yml` already encodes the same requirement for their issue-writing behavior). - Granting `issues: write` triggers `runner-policy`'s invariant that a contract with `allowedCallerPermissions` must map every `allowedSecrets` entry as an exact identity passthrough `${{ secrets.<name> }}`, where `<name>` must be a valid, non-hyphenated identifier. This reusable's own `workflow_call.secrets` inputs are `app-client-id` / `app-private-key` — kebab-case, so no mapping can ever satisfy that invariant. A caller adopting this reusable for its issue-writing behavior is permanently blocked by `runner-policy`, independent of which secret names it picks. - Rejected alternative: relax `runner-policy`'s write-caller-permissions invariant to allow non-identity secret mappings. Declined — that invariant is a deliberate defense-in-depth boundary (don't let a write-permission grant hide behind an opaque secret remap), and weakening it is a security-relevant call for the `standards` owner, not something to do as a side effect of unblocking one caller. ## Change - Mint a second, narrowly-scoped App token (`permission-issues: write`, `owner`/`repositories` omitted so it defaults to the calling repository only — verified against `create-github-app-token`'s own input docs) and use it for all three issue-management steps (find/adopt, open-or-update, close). - Update `ISSUE_AUTHOR_LOGIN` from `github-actions[bot]` to `melodic-standards-sync[bot]` — empirically verified via `gh api users/melodic-standards-sync%5Bbot%5D` (`login: "melodic-standards-sync[bot]"`, `type: "Bot"`). - Drop the job's own `issues: write` permission — no ambient-token issue write remains, so it isn't needed. - This is *stronger* decoy resistance than before, not just a policy workaround: `github-actions[bot]` is the shared identity of every ambient-token workflow in a repo, so any other workflow with `issues: write` could theoretically author a decoy tracking issue. The App's identity is exclusive to this workflow's own token mint. - Result: the reusable's `workflow_call` inputs/secrets contract is unchanged, but a caller no longer needs to grant `issues: write` at all — its `runner-policy` contract entry can be secrets-only, with no `allowedCallerPermissions` waiver. ## Test plan - [x] `actionlint .github/workflows/standards-sync-stuck-automerge-alert.yml` — clean - [x] `zizmor .github/workflows/standards-sync-stuck-automerge-alert.yml` — no findings - [ ] CI (this PR's own checks) - [ ] Manual verification once melodic-standards-sync App gets its Issues: read+write grant (tracked separately as an operator action) — first live scheduled run in a caller repo confirms the tracking issue is authored by `melodic-standards-sync[bot]` 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01KYvF6bWGqemS9aYFfWJRiW Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…tomerge-alert (#251) No related issue: caller rollout for merged melodic-software/ci-workflows#213 (issue #210 closed) ## Related ci-workflows#210, ci-workflows#213, ci-workflows#223 ## Summary - standards owns the sync trigger point and already holds the App secrets `sync.yml` uses, so it hosts the scheduled caller for the stuck-automerge watchdog reusable added in melodic-software/ci-workflows#213, rather than medley. - **This PR is a draft, blocked on melodic-software/ci-workflows#223.** The pinned SHA (`dd45dacd7b74dd05f3334b78769e57225f7356d8`) is the reusable as merged in #213, which authors its tracking issue with the caller's ambient `GITHUB_TOKEN` and therefore needs a caller-granted `issues: write`. That combination cannot satisfy `runner-policy`'s write-caller-permissions invariant (identity-passthrough-only secret mapping, no hyphens) against this reusable's kebab-case `app-client-id`/`app-private-key` secret inputs — see #223 for the full diagnosis and the fix (the reusable mints its own App token for issue writes instead). - The workflow file and `policy.json` entry here are already written for the **post-#223 shape**: no caller `issues: write`, no `allowedCallerPermissions` waiver, secrets-only contract. Only the pinned SHA (in both the workflow file and the policy.json key) needs to flip to #223's merge SHA once it lands — no other change. ## Test plan - [x] `npm run test:runner-policy` — 228/228 pass - [x] `npm run lint:runner-policy` — Runner policy passed - [x] `actionlint .github/workflows/standards-sync-stuck-automerge-alert.yml` — clean - [x] `zizmor .github/workflows/standards-sync-stuck-automerge-alert.yml` — no findings - [x] `npx biome check components/runner-policy/policy.json` — no fixes needed - [ ] Flip pinned SHA to ci-workflows#223's merge SHA once it merges, mark ready for review 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01KYvF6bWGqemS9aYFfWJRiW --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…rict tracking-issue adoption to the workflow's own token author (#235) Closes #221 ## Summary - Restricts tracking-issue adoption to the workflow's own token identity in `link-check.yml`, `queue-monitor-liveness.yml`, and `tool-version-drift-check.yml`: candidates are filtered to `user.login == ISSUE_AUTHOR_LOGIN && user.type == "Bot"` BEFORE any marker/title matching, in every lookup path (close paths reuse the single filtered lookup). Marker/title strings are public in workflow source, so without this filter any issue author could craft a decoy that gets adopted (then closed on recovery) or a duplicate decoy that trips the fail-closed ambiguity guard to suppress a real alert — the pattern ported from the stuck-automerge alert (#213). - The filter lands at the owning source `find-tracking-issue.sh`; both generated consumer blocks are re-rendered and stay byte-identical (`render-find-tracking-issue.cjs --check` green). `ISSUE_AUTHOR_LOGIN` is a required input (`:?`), so an unset value fails closed rather than silently dropping the restriction; a null/missing `user` never matches and never throws. - Scope note: the issue named two workflows; `tool-version-drift-check.yml` is a third consumer of the same shared script with the identical exposure, so the port covers it too (its diff is exactly the regenerated block plus the one `ISSUE_AUTHOR_LOGIN` env line — no pins, versions, or checksums touched). - Token identity verified, not assumed: none of the three workflows override the ambient `GITHUB_TOKEN` (create-issue-from-file@v6.0.0 defaults `token: ${{ github.token }}`), and the live API confirms `users/github-actions[bot]` → `type: "Bot"`, matching real tracking issues in this repo. ## Test plan - `bash .github/scripts/find-tracking-issue.test.sh`: all cases pass, including new decoy-by-marker (User), decoy-by-marker (other bot), decoy-by-title, decoy-cannot-trip-ambiguity, and unset-`ISSUE_AUTHOR_LOGIN`-fails-closed cases. - New `.github/scripts/link-check-tracking-author.test.cjs` (executing harness — extracts and runs the inline lookup): 8/8 — decoy-by-marker, decoy-by-different-bot, decoy-by-title, decoy-cannot-suppress-real-report, genuine-double-match-still-fails-closed, PR-never-adopted, plus positive controls. - `node --test .github/scripts/*.test.cjs`: 260/260. `node .github/scripts/render-find-tracking-issue.cjs --check`: both consumers byte-identical. `shellcheck` on the shared script + test: clean. `actionlint` (YAML/expr) on all three workflows: clean. - Independent fresh-context review of the full diff: no issues (author-filter precedence, fail-closed posture, jq/JS null-safety, generated-block sync, no unexpected edits). ## Related - Refs #213 — pattern source (stuck-automerge alert author restriction) and discovery context - Refs melodic-software/ci-runner#140 — same hardening applied to queue-monitor incident issues 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…never armed (#291) ## Summary `standards-sync.yml` arms squash auto-merge on sync PRs. The mutation is wrapped in a `try`/`catch` that downgrades **every** rejection to `core.warning`, so an arming failure produces a sync PR that simply sits unarmed — indistinguishable from the pre-arming status quo, and invisible to a watchdog that only hunted PRs stuck **with** auto-merge armed. Failing closed is correct; failing closed **invisibly** is the defect, because the operator's mental model says "armed" while the PR waits on a human who was never told to look. This PR closes that blind spot, and fixes the arming gate that would otherwise have made the new detection fire on PRs where arming was deliberately skipped. ### Today's failure behavior, from source `.github/workflows/standards-sync.yml` at `ac223bb`, the arming step's tail: ```js } catch (error) { // Known rejection: a PR that is already immediately mergeable // (GraphQL mergeStateStatus CLEAN) has nothing to wait for, and // GitHub's mutation errors instead of no-op succeeding. Any // other transient rejection is handled the same way: log and // let the sync continue, never fail the run over arming. core.warning( `Could not arm auto-merge on ${owner}/${repo}#${pull_number}: ${error.message}`, ); } ``` The failure is **trapped**. The step carries no `continue-on-error` and does not need one — the `catch` swallows the throw, so the step succeeds regardless, and nothing downstream reads the outcome. `standards-sync-automerge-arm.test.cjs` pinned this deliberately ("a rejected mutation ... is logged and swallowed, not thrown"). ### Can a GitHub App installation token arm auto-merge? Researched against official GitHub sources, not recall. **Yes** — established by a chain, because no single page states it: 1. `GITHUB_TOKEN` **is** an App installation access token — github/docs `content/actions/concepts/security/github_token.md`: *"The `GITHUB_TOKEN` secret is a GitHub App installation access token."* 2. GitHub documents a workflow enabling auto-merge with exactly that token — `content/code-security/tutorials/secure-your-dependencies/automate-dependabot-with-actions.md` runs `gh pr merge --auto` under `permissions: contents: write` + `pull-requests: write`. 3. `gh pr merge --auto` issues this mutation — `cli/cli` `pkg/cmd/pr/merge/http.go`: `graphql:"enablePullRequestAutoMerge(input: $input)"`. First-party **source**, not documentation; flagged as such. The App behind `GITHUB_TOKEN` is GitHub's own Actions App, and docs state no divergence for third-party Apps in either direction. Not settled by documentation, and marked as such rather than inferred past: | Question | Verdict | | --- | --- | | Minimal permission for the mutation | **UNVERIFIED** — no GraphQL per-mutation permissions table exists. `content/apps/.../choosing-permissions-for-a-github-app.md` explicitly punts: *"you should test your app to ensure that it has the required permissions."* The sync token already requests the `contents: write` + `pull-requests: write` pair from GitHub's own working example. **Would settle it:** a throwaway repo, arming with each permission alone and then both, recording `errors[].type`. | | Does the mutation error rather than no-op when the PR is already mergeable? | **UNVERIFIED** — the GA changelog says auto-merge *"can only be enabled ... when there are unsatisfied merge requirements"*, but never states the API errors. **Would settle it:** same throwaway repo, a PR with zero unsatisfied requirements. | Both experiments mutate state, so a disposable repo is the ceiling — never the live fleet. **Verified preconditions:** *"Before you use auto-merge, it must be enabled for the repository"* (`allow_auto_merge`, default `false`), and *"People with write permissions to a repository can enable auto-merge for a pull request."* ## The fix ### Part 1 — the watchdog reports never-armed sync PRs `standards-sync-stuck-automerge-alert.yml` gains a second category beside armed-but-BLOCKED: a sync PR past `threshold-hours`, in a target the manifest marks `automerge: true`, on which auto-merge was **never** armed. On detection it takes the path the existing category already takes — a marker-deduped tracking issue in the calling repo, and `exit 1` so the *scheduled* run fails and notifies. That is what makes the failure observable rather than merely logged. The discriminator is the **absence of any auto-merge *enabled* event** in the PR's timeline. That is direct positive evidence the mutation never succeeded. The first draft of this PR keyed on the absence of an `AutoMergeDisabledEvent` instead, which is only an inference and conflates two different things: GitHub disables auto-merge on its own when someone without write access pushes to the head branch or the base is switched, and the event's `reason` / `reasonCode` are free-form `String` in the published schema, not an enum, so they cannot be keyed on. An enabled event answers the question that actually matters — *did arming ever take?* **Two GraphQL semantics had to be established live, and both were wrong in an intermediate revision of this PR.** Each independently inverts the check, so in a live run they would have masked each other: - `timelineItems.totalCount` reports the **whole** timeline and ignores `itemTypes` — only `nodes` is filtered. Verified on `github-iac#234`: `totalCount: 4` alongside zero matching nodes. Reading `totalCount` makes every PR look already-armed, which would have stopped the sync arming anything at all and made the watchdog exonerate every real failure. - `mergeMethod: SQUASH` records an **`AutoSquashEnabledEvent`**, not an `AutoMergeEnabledEvent`. Verified on `medley#1619` (armed, `mergeMethod: SQUASH`): zero `AUTO_MERGE_ENABLED_EVENT` nodes, one `AutoSquashEnabledEvent`. Both workflows now read `nodes` across all three enabled-event types (`AUTO_MERGE_` / `AUTO_SQUASH_` / `AUTO_REBASE_ENABLED_EVENT`), so a future merge-method change cannot silently blind the check. `first: 1` is safe because GitHub applies the `itemTypes` filter *before* pagination (verified: an event at raw timeline index 2 is still returned by `first: 1`). Hand-written mocks are structurally unable to catch this class of defect — they encode whatever semantics the author believed. So the mocks now model the real behavior (filtered `nodes` plus a non-zero `totalCount` decoy the production code must not read), and four contract tests pin both facts directly against the shipped query text. ### Part 2 — arming is self-healing, which is the root cause Arming was gated on `steps.cpr.outputs.pull-request-operation == 'created'`. `peter-evans/create-pull-request` at the pinned SHA sets `pull-request-number` whenever the branch differs from base, but only ever reports `created` once (verified in `src/create-pull-request.ts` at `5f6978faf089d4d20b00c7766989d076bb2fc7f1`). So a sync PR opened while its target carried `automerge: false` — how a rollout window is held — **stayed unarmed forever**, and lifting the opt-out would not have repaired it. Two such PRs are open on the fleet right now: `github-iac#234` and `medley#1665`, both created 2026-07-27, both unarmed with an empty *arming* timeline (they do carry ordinary timeline items — that distinction is the subject of the next section). Under the created-only gate they would have been reported as arming failures every hour, permanently, and sent the operator to a warning line the *skipped* step never wrote. So the gate is now "this PR exists, the manifest says arm it, and it has never been armed" — the same predicate the watchdog uses. Consequences, stated up front: - Once the standards sync-engine pin carries this change, the next sync after the manifest restores `automerge: true` arms every open sync PR fleet-wide. That is the intended end state, and it repairs #234 and #1665, which today will never self-merge and which nothing is watching. **Sequencing matters:** if the manifest is restored while the engine pin still predates this change, those PRs stay unarmed and a re-pinned watchdog reports them permanently. The engine re-pin should target this PR's merge SHA, not `ac223bb`. - Given the right order, a **transient** alert is still possible if the hourly watchdog fires in the gap between the manifest flip and the next sync. Standards' `sync.yml` triggers on push to `main`, so the flip is itself the trigger and the gap is minutes. It self-resolves via the existing `Close recovered tracking issue` step — a bounded transient, not the permanent hourly alarm this removes. - The recovery text now covers both cases: an arming step that ran and was rejected leaves a warning to read; a caller pinned to an engine that predates arming on already-open PRs skips the step entirely, and the text names the re-pin as that fix rather than pointing at a log line that does not exist. - A reviewer who disarms a PR to hold it back is still never overridden. Verified live on `medley#1613`: disarming does **not** erase the enabled event. The single GraphQL read also replaces the REST `pulls.get` the step used to fetch the node id. ### Rejected alternatives **Rejected — make the arming step fail its leg.** The step's own comment records that an already-mergeable PR is a *known benign* rejection, and the docs research above leaves the mutation's error-vs-no-op semantics **UNVERIFIED**. Failing the sync leg would turn a benign, undocumented condition into a red sync, and the token already requests exactly GitHub's documented working permission pair, so a permission wall is not the likely failure mode. A post-condition check ("did the PR end up armed?") was considered and folded into Part 2 instead, where it costs nothing and repairs rather than merely reports. **Rejected — emit a distinguishable annotation.** `core.error` instead of `core.warning` colors the log, but nothing reads sync logs on a schedule — which is precisely why the stuck-PR case needed a watchdog rather than louder logging. Louder, not observable. **Rejected — a new dedicated workflow.** It would duplicate the manifest read, the App-token mint, the pagination-with-retry, the marker-deduped tracking issue, and the decoy-resistant issue adoption this workflow already has, then file a second competing issue for one incident. Both conditions answer the same question ("can this sync PR merge itself?") and belong in one report. ## Activation dependency — this ships inert The watchdog is a reusable workflow. Exactly **one** `uses:` pin exists across the default branches of every org repo the token can see (`gh search code` returns 13 mentions across 7 repos; every other hit is prose or a materialized copy of standards' governance data, not a `uses:`): `melodic-software/standards` `.github/workflows/standards-sync-stuck-automerge-alert.yml:19`, pinned at `43bc8d0`. `components/runner-policy/policy.json:438` carries the same SHA as governance data. Until that caller is re-pinned, the never-armed detection never runs. Reachability also needs the manifest's Phase 3d `automerge: false` window to close. The sync-engine pin is the second axis: standards' `sync.yml` pins `0b45b9f`, which has no arming step at all, so the self-healing arm is equally latent until that is re-pinned past this PR. This is the same second-staleness axis already recorded for the sync engine's own pin. ## Scope: the `43bc8d0` → `42329ef` re-pin is split out Not because it is unrelated — `42329ef` (#234) hardened the very scan loop this PR extends — but because: - The pin lives in `melodic-software/standards`, not this repository (`git grep 43bc8d0` finds zero hits here), and standards is outside this work's write fence. - `42329ef` is no longer the right target. Once this PR merges, the caller needs the SHA that carries **this** change, which supersedes `42329ef` entirely. It belongs in the Phase 3g re-pin sweep, aimed at this PR's merge SHA. ## Deferred, with trigger Auto-merge that was armed and later fell off by itself (a push from someone without write access, a base-branch switch) is deliberately **not** detected — that PR carries an enabled event and is exonerated by both the watchdog and the self-healing arm. It is a different failure from the arming blind spot this PR closes. **Trigger to revisit:** an armed sync PR observed silently reverting to unarmed on the live fleet. ## Test plan All counts below regenerated from the commands shown, not from memory. - `node --test .github/scripts/*.test.cjs` (CI's exact command, from `ci.yml:373`): **304 pass, 0 fail.** - `standards-sync-stuck-automerge-alert.test.cjs`: **40 pass** (27 on `origin/main`, so 13 new). - `standards-sync-automerge-arm.test.cjs`: **10 pass** (5 on `origin/main`, so 5 new). - `actionlint 1.7.12` on both changed workflows: clean, exit 0. - `npx @biomejs/biome@2.5.4 ci --config-path=fixtures/typescript/good/biome.json --error-on-warnings fixtures/typescript/good .github/scripts`: clean, exit 0. (A previous revision of this PR failed this check; the config lives at `fixtures/typescript/good/biome.json`, which an earlier note wrongly said did not exist.) - `markdownlint-cli2@0.23.1 README.md`: 0 issues. The tests execute the shipped code, not a re-implementation: `extractScanScript` / `extractArmingScript` read the actual `.yml`, slice the `script: |` block by step name, and run it through `AsyncFunction`. Only `github.graphql` and `core` are mocked. New coverage: - detection past threshold; no alarm inside threshold - a PR armed-then-disarmed exonerated, with the probe proven to have run - an opted-out target never probed at all - a PR armed, merged, or closed in the page→probe race - non-sync authors ignored - a persistent probe error failing loudly with no false all-clear - both categories reported in separate sections; all-clear requires both empty - arming skipped for a currently-armed PR and for an armed-then-disarmed PR - an unreadable PR warns instead of mutating - contract tests, in both files, pinning that the arming history is read from filtered `nodes` and never `totalCount`, and that all three merge methods' enabled events are probed ## Related - Refs #213 — the arming step and this watchdog's original half. - Refs #234 (`42329ef`) — the scan-loop split and retry this change extends. - Refs melodic-software/standards#289 — the engine re-pin that first puts the arming step into production. No linked issue. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #210
Summary
Across 7 repos, 193 of 193 merged standards-sync PRs were merged by one human (18 via human-armed auto-merge, 175 manual, 0 by the App) — the fleet audit's only CONFIRMED-critical coverage gap. The sync PR body is gate-compliant by construction (the App identity that opened it is the whole risk gate), so routine sync PRs were a recurring manual chore with no safety benefit. The companion risk this closes:
medley#1619showed an auto-merge-armed sync PR silently BLOCKED for hours by an unrelated pre-existingci-statusfailure, with nothing watching.standards-sync.ymlnow arms GitHub auto-merge (squash) on a newly created sync PR via agraphqlenablePullRequestAutoMergecall, reusing the same target-scoped App token the PR was just opened with (the same pattern the existingattestjob already uses for API calls in this file — noghCLI, no new runner-tool assumption). Gated on:steps.cpr.outputs.pull-request-operation == 'created'— never a later update, so a reviewer who deliberately disarmed a PR is not overridden by the next sync run.matrix.automerge— the new per-target opt-out flag from standards#246 (policy-as-data insync-manifest.yml; defaulttrue, absent-key-safe both ways for whichever of these two PRs merges first).A rejected mutation (for example a PR that's already immediately mergeable, which GitHub's
enablePullRequestAutoMergerejects instead of no-op succeeding) is caught, logged viacore.warning, and does not fail the sync — matching the issue's "handle the clean-status error gracefully" requirement, generalized to catch any rejection rather than string-matching a specific message.New
standards-sync-stuck-automerge-alert.ymlreusable: scans every standards-sync target repository — read from the manifest at run time via the samesync-manifest.sh matrixinterpreter callstandards-sync.yml's ownplanjob uses, never a second hardcoded list — for open pull requests authored by themelodic-standards-syncApp with auto-merge armed and GraphQLmergeStateStatus: BLOCKEDfor at leastthreshold-hours(default 4). It reuses the existing marker-deduped tracking-issue upsert pattern (find-tracking-issue.sh, now a fourth consumer of the codegen'd block alongsidelink-check.yml,queue-monitor-liveness.yml, andtool-version-drift-check.yml) for one rolling aggregate issue, filed in the caller's own repository (not scattered across the scanned targets). Unlikelink-check.yml's advisory posture, this fails the scheduled run when it finds a stuck PR — a stuck armed sync PR is an actionable, non-flaky condition, so it gets both the tracking issue and the run-failure notification channel, matchingqueue-monitor-liveness.yml's posture.Naming
standards-sync-stuck-automerge-alert.yml/ jobdetect-stuck-armed-prs/ issue title[Alert] standards-sync stuck auto-merge PR(s)— every name states exactly what it does, per the issue's hard requirement.Verified operational note (not a code change)
Two target repos already have an open sync PR predating this change:
claude-code-plugins#951andmedley#1619(the exact motivating incident PR — still open, human-armed,mergeStateStatus: UNKNOWNas of this PR). Both will reportpull-request-operation: updated, notcreated, on their next sync run, so neither gets auto-armed automatically. Closing either lets the next scheduled sync recreate and arm it; leaving it open just means it stays on the pre-existing human-merge path until closed.Test plan
node --test .github/scripts/*.test.cjs— full suite, 220/220 pass, including two new files:standards-sync-automerge-arm.test.cjs(extracts and directly executes the arming script — verifies the creation-only +matrix.automergegate, the target-scoped token, the exactpullRequestId/SQUASHGraphQL call, and that both a "clean status"-shaped rejection and an unrelated rejection are logged and swallowed rather than thrown) andstandards-sync-stuck-automerge-alert.test.cjs(extracts and directly executes the scan script — verifies unarmed/CLEAN/under-threshold PRs are excluded, an over-threshold BLOCKED armed PR is reported with the marker and a recovery section, exact-login/exact-__typenamematching resists both an unrelated-bot and a human-impersonating-the-login case, and multiple stuck PRs across repos are all captured).node .github/scripts/render-find-tracking-issue.cjs --check— the new workflow's embedded tracking-issue block is byte-identical to the generated source.actionlinton both changed/new workflow files — clean.zizmoron both — clean (one informational template-injection finding on the initialFail so the scheduled run notifiesstep was found and fixed by routing the output throughenv:instead of an inline${{ }}in therun:block, matching how the rest of the codebase avoids this).shellcheck,typos,editorconfig-checker,gitleaks,markdownlint-cli2— all clean on every changed/new file.Related
matrix.automerge).🤖 Generated with Claude Code