fix(standards-sync-stuck-automerge-alert): split the stuck-PR scan into a cheap page fetch and a per-candidate merge-state probe with retry

[kyle-sexton]

Closes #226

Summary

• Splits the stuck-automerge scan into two phases so the hourly cron stops dying on GitHub's opaque server-side GraphQL error: phase 1 pages open PRs selecting only number/url/author/autoMergeRequest { enabledAt } (no mergeStateStatus), phase 2 probes mergeStateStatus one PR at a time via repository.pullRequest(number:), and only for armed, past-threshold bot PRs.
• Wraps every GraphQL call in graphqlWithRetry — exponential backoff (GRAPHQL_RETRY_ATTEMPTS=4, GRAPHQL_RETRY_BASE_MS=1000, both step-env-tunable), rethrowing the last error on exhaustion so a persistent failure still fails the run loudly. On a throw the stuck-count output is never set, so the close-issue branch cannot fire a false all-clear.
• Retains the existing manual-cursor pagination and MAX_PAGES soundness guard unchanged; "stuck" semantics are preserved (the BLOCKED check moved from the bulk page filter to the per-candidate probe).
• No changes to the trigger surface, App-authored issue identity, uses: SHA pins, or permissions blocks.

Test plan

• node --test .github/scripts/*.test.cjs (CI's exact command): 257/257 pass, including 24 in standards-sync-stuck-automerge-alert.test.cjs — new coverage asserts the page query never selects mergeStateStatus, exactly one phase-2 probe per candidate (no bulk fan-out), transient-error retry on both page fetch and probe, and persistent-error loud failure with no false all-clear.
• actionlint on the changed workflow: clean.
• biome check (CI config/scope): clean.
• Post-merge follow-through: the phase-2 query shape is mock-validated only — confirm ≥2 consecutive green scheduled runs against the real fleet (the issue's remaining acceptance criterion).

Related

• Refs feat(runner-policy): add scheduled caller for standards-sync-stuck-automerge-alert standards#251 — deployment PR for the failing hourly caller
• Refs feat(select-runner): opt-in local routing for comment/review events #224 — comment-events opt-in context named by the issue
• Refs fix(standards-sync-stuck-automerge-alert): author the tracking issue as the App, not the ambient token #223 — prior fix on the same workflow (App-authored issue identity), deliberately untouched here

🤖 Generated with Claude Code

[claude]

Claude finished @kyle-sexton's task in 2m 41s —— View job

---

PR Review

• Gather context: read PR diff and changed files
• Review workflow YAML changes
• Review test file changes
• Assess correctness, security, and CLAUDE.md alignment
• Post findings

---

Summary

This is a solid fix to a real operational problem. The two-phase split is the right architectural move: removing mergeStateStatus from the bulk page query eliminates the root cause of GitHub's opaque server-side errors, and graphqlWithRetry provides the belt-and-suspenders defense for the narrower per-PR probes. No correctness bugs, no security issues, and full CLAUDE.md alignment.

---

Correctness

Retry logic — the off-by-one is correct:

for (let attempt = 1; ; attempt += 1) {
  try {
    return await github.graphql(query, variables);
  } catch (error) {
    if (attempt >= retryAttempts) throw error;   // throws on attempt 4 (with retryAttempts=4)
    ...
    await sleep(delayMs);
  }
}

With retryAttempts=4: attempts 1–3 retry (delays 1 s, 2 s, 4 s), attempt 4 throws. Total time for a fully failing call: ~7 s — well within the 15-minute timeout.

False all-clear prevention is structurally sound: core.setOutput('stuck-count', ...) is placed after both loops. A throw from graphqlWithRetry propagates out of the script function, the github-script step fails, and subsequent steps — including the close-issue branch (if: steps.scan.outputs.stuck-count == '0') — are skipped by GitHub Actions. The output is never set, so == '0' is false. This is verified end-to-end by the persistent-failure test.

Candidate scoping — const candidates = [] is declared inside the for (const repo of repoNames) loop, so it correctly resets per-repo. No cross-repo leakage.

Phase-2 probes are sequential — for ... await over candidates is intentional and appropriate: this workflow is a low-frequency watchdog (hourly cron), the expected candidate count is tiny (ideally zero), and sequential probes reduce concurrent load on GitHub's mergeability computation — the very thing that was tripping the original error.

One minor observation — the ?? 4 / ?? 1000 fallbacks in:

// .github/workflows/standards-sync-stuck-automerge-alert.yml, line 141–142
const retryAttempts = Number(process.env.GRAPHQL_RETRY_ATTEMPTS ?? 4);
const retryBaseMs   = Number(process.env.GRAPHQL_RETRY_BASE_MS ?? 1000);

are dead code in the Actions runtime: both vars are declared in the step's env: block directly above, so they are always strings, never undefined. This is harmless and might be intentional defensive coding; leaving it is fine.

---

Security / CLAUDE.md alignment

Trigger surface: unchanged — still workflow_call only; no pull_request_target or workflow_run. ✓

SHA pins: no uses: lines were touched. ✓

Permissions: no changes to the job-level contents: read grant or the issue-token scope. ✓

Log hygiene — the new core.info() in the retry path:

// line 159
core.info(`GraphQL ${description} failed (attempt ${attempt}/${retryAttempts}): ${error.message}; retrying in ${delayMs}ms.`);

logs error.message from GitHub API responses in a private workflow run (this is a scheduled reusable, not a consumer of fork PRs). No token echoing, no display_report/show_full_output surface. Acceptable.

GraphQL variables: queries use parameterized $owner, $repo, $after, $number. The only string interpolation is first: ${PAGE_SIZE} (numeric constant 50). No injection risk.

---

Test coverage

The new tests directly match the invariants the PR claims to establish:

Test	What it locks in
page query never selects mergeStateStatus	Structural regression guard on the query shape
only armed, past-threshold bot PRs are probed	No bulk fan-out (the perf contract)
transient error on page fetch is retried	Retry path for phase 1
transient error on probe is retried	Retry path for phase 2
persistent error fails loudly, no false all-clear	The safety invariant

The GRAPHQL_RETRY_BASE_MS: "0" trick in the test harness is clean — it exercises the retry code path without actually sleeping.

One note on the "queries every manifest-derived target repository" test: it maps all graphqlCalls to .variables.repo (not filtered to page calls). It works here because the test data is empty and produces no candidates — but if someone later adds armed PRs to this test, the extra probe calls (which also carry variables.repo) would cause deepEqual to fail with a duplicated repo entry. The test's intent remains valid; just worth a comment if it ever needs to be extended.

---

Verdict

Approve. The fix is correct, secure, well-tested, and CLAUDE.md-compliant. The one remaining acceptance criterion from the PR description (≥2 consecutive green scheduled runs against the real fleet) is appropriately flagged as post-merge follow-through.

[claude]

Claude finished @kyle-sexton's task in 2m 16s —— View job

---

Security Review

• Gather PR diff and changed files
• Review workflow YAML for security issues
• Review test file for security issues
• Analyze data-flow, trust boundaries, and logic for vulnerabilities
• Post findings

---

Scope: .github/workflows/standards-sync-stuck-automerge-alert.yml and .github/scripts/standards-sync-stuck-automerge-alert.test.cjs — changes in this PR only.

Summary

No critical or important security issues found. The change is security-positive overall: the split-phase design reduces the attack surface of each GraphQL call, and the false-clear protection is correctly preserved — stuck-count is set only after all scans complete, so a throw mid-scan leaves the output unset, neither the update nor the close branch fires, and the run fails loudly.

One low-confidence suggestion follows.

---

SUGGESTION — error.message from GitHub API response written to stdout without workflow-command escaping

Confidence: LOW

Line 159:

core.info(`GraphQL ${description} failed (attempt ${attempt}/${retryAttempts}): ${error.message}; retrying in ${delayMs}ms.`);

@actions/core's core.info() writes to stdout without escaping workflow command sequences. If a GitHub GraphQL error response contained text that looks like a runner workflow command (e.g., ::error::, ::warning::, ::set-output name=...::value), the runner would interpret it. The error.message here is sourced entirely from GitHub's own API error response, so controlling its content requires compromising GitHub's servers — not a practical attacker model. The risk is negligible in this context, but worth noting as a pattern to avoid in workflows where the error source is less trusted.

No remediation is required for this PR. If hardening is desired, the content could be sanitized before logging (e.g., replacing :: sequences), though that adds complexity with no real threat to defend against here.

---

Other checks (all clean)

Check	Result
on: trigger surface	Unchanged; no dangerous pull_request_target or workflow_run with secrets introduced
permissions: blocks	Unchanged; narrowest-needed grants preserved
New uses: entries	None introduced
Script injection via ${{ }} in run:	New env vars GRAPHQL_RETRY_ATTEMPTS/GRAPHQL_RETRY_BASE_MS are literal string constants, not expressions over user-controlled context
GraphQL query construction	Both listQuery and mergeStateQuery use parameterized typed variables ($repo: String!, $number: Int!); no interpolation into query text
False-clear invariant	core.setOutput('stuck-count', ...) is only reached after the full scan loop; a throw anywhere in the loop leaves the output unset, so the close-issue branch is unreachable on failure
Token scope	permission-pull-requests: read App token unchanged; issue-write token scope unchanged
GRAPHQL_RETRY_ATTEMPTS as attack surface	Set to the literal '4' in step env:, not derived from any workflow inputs: or context expression; callers cannot tune it to trigger unbounded retry loops

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ed4293b87

ℹ️ 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".

[kyle-sexton]

AI reply (work-loop lane). Review-finding classification:

Source	Finding	Classification	Disposition
claude PR review	?? 4 / ?? 1000 fallbacks are dead code in Actions runtime	VALID (informational)	Reviewer: "leaving it is fine" — kept as defensive coding, no change
claude PR review	repo-mapping test would need call filtering if armed PRs added to fixture	VALID (informational)	No change now; the incoming TOCTOU-fix commit extends probe tests and keeps that fixture unambiguous
claude security review	error.message logged via core.info without workflow-command escaping (LOW)	VALID (no remediation)	Reviewer's own verdict: content is GitHub-API-sourced, not a practical attacker model; sanitization adds complexity with no real threat — not filed as follow-up per that recommendation
codex P2 (inline)	Revalidate auto-merge state in per-candidate probe	VALID (fix now)	Fix in flight; follow-up with commit SHA on the thread

[claude]

Claude finished @kyle-sexton's task in 1m 37s —— View job

---

Review of TOCTOU fix commit (07f706e)

• Gather context: identify the new commit scope
• Diff latest commit against prior state
• Verify probe re-fetches autoMergeRequest { enabledAt } and re-validates armed + threshold
• Verify tests cover disarmed-between-phases and probe-fresh-duration scenarios
• Post findings

---

What changed

The fix addresses the Codex P2 finding: a PR could be disarmed (or re-armed) in the window between the phase-1 page fetch and the phase-2 probe, causing the stuck decision to use a stale enabledAt from the page. Two things changed.

Workflow script (24 lines, 2 files):

1. Extracted armedHoursSince(enabledAt) — returns null for falsy input, a number otherwise. Used in both isArmedCandidate (page filter) and the probe post-processing below, eliminating the prior duplicate elapsed-time calculation.

2. Added autoMergeRequest { enabledAt } to mergeStateQuery — the probe now returns fresh armed state alongside mergeStateStatus.

3. The stuck decision at the probe site re-validates entirely from probe-fresh data:

   const pullRequest = result.repository?.pullRequest;
   if (pullRequest?.mergeStateStatus !== 'BLOCKED') continue;
   const armedHours = armedHoursSince(pullRequest.autoMergeRequest?.enabledAt);
   if (armedHours === null || armedHours < thresholdHours) continue;
   stuck.push({ ..., armedHoursAgo: Math.floor(armedHours) });

   Three cases now handled correctly:

   • Disarmed between phases: autoMergeRequest is null → armedHoursSince(undefined) returns null → skipped.
   • Re-armed under threshold between phases: fresh enabledAt is recent → armedHours < thresholdHours → skipped.
   • Re-armed over threshold: reports using fresh enabledAt, not the stale page value.

Null-probe safety — if the PR was deleted or returned null from the probe, pullRequest?.mergeStateStatus is undefined !== 'BLOCKED' → skipped immediately, no attempt to read autoMergeRequest. Correct.

---

Tests (92 new lines in standards-sync-stuck-automerge-alert.test.cjs)

The test harness gains a probeOverrides parameter to runScan(). When probeOverrides[number] is present, the mock uses the override's enabledAt/mergeStateStatus rather than echoing the page node — cleanly simulating divergence between page and probe without requiring separate fixtures. Backward-compatible (default {}).

Three new tests covering all TOCTOU scenarios:

Test	Scenario	Assert
disarmed between page fetch and probe	probeOverrides: { 8: { enabledAt: null } }	stuck-count === '0', report === null
duration computed from probe-fresh enabledAt	page says 20h, probe says 6h	report matches /| 6h |/, not /20h/
re-armed under threshold between page and probe	page says 20h (over), probe says 1h (under)	stuck-count === '0', report === null

The "enabledAt" in override check (not just override.enabledAt) is the right way to distinguish "explicitly set to null" from "not present" — correctly handles the disarm case where null is a meaningful value.

---

No regressions introduced

• isArmedCandidate behavior is unchanged — the refactor to armedHoursSince is equivalent to the previous inline arithmetic.
• BLOCKED check remains first in the probe post-processing, so non-blocked PRs still skip without touching enabledAt.
• armedHoursAgo: Math.floor(armedHours) is computed from the same armedHours variable used for the threshold check — single source of truth.
• False-all-clear prevention and retry logic are untouched.

---

Verdict

Approve. The fix is correct, complete, and well-tested. All three TOCTOU scenarios are covered, the helper extraction is a genuine DRY improvement (not incidental cleanup), and the probeOverrides harness mechanism is clean without being over-engineered. No issues found.

[claude]

Claude finished @kyle-sexton's task in 3m 23s —— View job

---

Security Review

• Gather context
• Read changed files (workflow YAML + test script)
• Analyze for logic flaws, data-flow, trust-boundary issues
• Post findings

---

Scope: .github/workflows/standards-sync-stuck-automerge-alert.yml and .github/scripts/standards-sync-stuck-automerge-alert.test.cjs — changes in this PR, including the TOCTOU fix in commit 07f706e.

No critical or important security findings.

---

TOCTOU fix verification (07f706e)

The codex P2 finding is correctly resolved. The phase-2 probe now fetches autoMergeRequest { enabledAt } alongside mergeStateStatus, and the stuck decision re-validates both armed state and threshold from that fresh value:

// https://github.com/melodic-software/ci-workflows/blob/07f706eb6e9a101a032ba310372c842c66139cfd/.github/workflows/standards-sync-stuck-automerge-alert.yml#L249-L253
const pullRequest = result.repository?.pullRequest;
if (pullRequest?.mergeStateStatus !== 'BLOCKED') continue;
const armedHours = armedHoursSince(pullRequest.autoMergeRequest?.enabledAt);
if (armedHours === null || armedHours < thresholdHours) continue;
stuck.push({ repo, number: candidate.number, url: candidate.url, armedHoursAgo: Math.floor(armedHours) });

The phase-1 value (candidate.url) carried into the report is cosmetic (a display link), sourced from GitHub's own API (https://github.com/owner/repo/pull/N format), and carries no correctness or security weight. The stuck decision depends only on probe-fresh data. The fix tightens the boundary correctly — no new attack surface introduced.

---

New code introduced — graphqlWithRetry

Lines 152–163

Retry cap is caller-isolated. GRAPHQL_RETRY_ATTEMPTS and GRAPHQL_RETRY_BASE_MS are set as literal string constants in the step env: block, not derived from any workflow_call input or github context expression. Callers cannot influence them to force an unbounded retry loop or a DoS-by-backoff.

Backoff arithmetic is safe. With retryAttempts=4, the maximum exponent is 2**(3-1)=4 (1 s, 2 s, 4 s total sleep, 7 s worst-case for a failed call). No overflow; well within the 15-minute job timeout.

error.message logging — previously noted at LOW confidence by the prior review. The source is GitHub's own API error responses; controlling their content requires compromising GitHub's servers. No remediation needed here; no new surface introduced by this PR.

---

Phase boundary data-flow

Phase-1 → phase-2 data crossing the boundary:

Field	Type	Source	Used as
candidate.number	Int! (GraphQL typed)	GitHub API	GraphQL $number variable in phase-2 probe
candidate.url	string	GitHub API	Cosmetic display link in Markdown report

Neither field is string-interpolated into query text. candidate.number is passed as a typed GraphQL variable ($number: Int!). No injection path.

The stuck array entries contain only hardcoded strings, GitHub-API-sourced integers and URLs, and Math.floor(armedHours) (integer). The Markdown report wraps repo in backticks and uses integer values elsewhere. No injection surface in the report body.

---

Other checks (clean)

Check	Result
on: trigger surface	Unchanged; workflow_call only
permissions:	Unchanged; contents: read on the job
New uses: entries	None
${{ }} expressions in run:	None new; MANIFEST env-var pattern (pre-existing, correct)
GraphQL query construction	Both queries use parameterized typed variables; only first: ${PAGE_SIZE} (numeric constant 50) is interpolated
False-clear invariant	core.setOutput('stuck-count', ...) is only reached after the full scan loop; a throw anywhere leaves the output unset
Retry-amplified false-clear	graphqlWithRetry rethrows on exhaustion — the throw propagates, scan step fails, close-issue branch is unreachable
candidates array scope	Declared inside for (const repo of repoNames) — resets per-repo, no cross-repo leakage

---

No security issues to flag. The change is security-neutral to positive: the split-phase design reduces each query's attack surface, the TOCTOU fix is correctly scoped to probe-fresh data only, and the retry wrapper introduces no new caller-reachable surface.