fix(queue-monitor-liveness): drop the failed-audit-step carve-out

[kyle-sexton]

No related issue: companion to melodic-software/ci-runner#140 contract change

Summary

melodic-software/ci-runner#140 makes queued-job-monitor.yml green-on-success: a successful execution now reports success whether or not it detected queue strain, alerting instead by upserting a marker-deduped incident issue in ci-runner rather than failing its own scheduled run.

This workflow's liveness check hard-coded the old contract: a MONITOR_AUDIT_STEP ("Inspect queued jobs") carve-out treated a monitor run whose only failed step was the audit step as healthy=true, on the theory that the failure itself WAS the queue alert. Under the new contract there is no longer a step whose failure is an expected, healthy outcome — that same failure shape now means the monitor is genuinely broken (config drift, a GitHub API error, or a failure writing the incident issue), while the watchdog carve-out would suppress its own alert exactly when both the monitor and this watchdog need to fire.

• Removes the MONITOR_AUDIT_STEP env var and the per-step-failure classification logic (the gh_read api .../jobs call that inspected which steps failed).
• Simplifies the health check to: any completed run whose conclusion is not success is monitor breakage, full stop.
• Updates the recovery guidance text and the header rationale comment, which both described the old failure-is-the-alert contract.

Safe to merge under the old queued-job-monitor.yml contract too, ahead of ci-runner#140: today, the monitor's audit-step failure would newly be classified as breakage and this watchdog would fire (correct-if-noisy for the transition window) rather than silently swallowing it as healthy. Per the team lead's merge sequencing, this PR merges first; ci-runner#140 re-arms after.

Test plan

• actionlint .github/workflows/queue-monitor-liveness.yml — clean
• zizmor .github/workflows/queue-monitor-liveness.yml — clean, no findings
• typos .github/workflows/queue-monitor-liveness.yml — clean
• Confirmed no other file in the repo references MONITOR_AUDIT_STEP (fully removed, no dangling reference)

Related

• Companion to queued-job-monitor: green-on-success + push alert channel (stop alert-by-failing) ci-runner#139 (issue) and feat(ci): green-on-success queue monitor with issue-per-incident alerting ci-runner#140 (PR) — the green-on-success contract change this fix keeps in sync with
• References ci-workflows#213 (standards-sync-stuck-automerge-alert.yml), the sibling alert workflow whose author-identity hardening ci-runner#140 also mirrors

🤖 Generated with Claude Code

[claude]

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

---

PR Review

• Gather context and understand the request
• Read the changed file
• Review for correctness, security, and CLAUDE.md alignment
• Post review findings

---

Verdict: Approve. This is a clean, well-reasoned simplification with no correctness, security, or style issues.

---

What changed

The old health check treated a failure conclusion specially: it made a second GitHub API call to inspect which steps failed, and classified a run whose only failure was the MONITOR_AUDIT_STEP ("Inspect queued jobs") as healthy=true — on the theory that the failure itself was the queue alert. All other non-success conclusions (cancelled, timed_out, etc.) were already treated as breakage via a separate branch.

The new check collapses both branches into one: any conclusion != "success" is breakage, full stop. The MONITOR_AUDIT_STEP env var and the per-step API call are removed entirely.

---

Correctness

Logic is sound. Under the new green-on-success contract, there is no longer a step whose failure is an expected outcome. Collapsing to a single != "success" branch is strictly correct and simpler. The removed broken_steps/unexpected detection path was load-bearing only for the old contract.

The conclusion value space is handled. The previous code's two-branch approach (failure → inspect steps; other non-success → "reached no verdict") mapped cleanly onto the old contract. The new single-branch approach maps cleanly onto the new one — failure, cancelled, timed_out, action_required, stale are all now correctly classified as breakage.

MONITOR_AUDIT_STEP is fully removed. No dangling references elsewhere in the repo (verified with grep). The env var block is clean.

Transition-window behavior is acceptable. The PR description is honest: during the window between this merge and ci-runner#140, if the old monitor fails at "Inspect queued jobs" (a real queue alert), this liveness check will now also fire. That's a brief noisy period, not a silent failure — strictly preferable to having this watchdog suppress its alert when both systems are trying to fire.

Error message is improved. The new message (completed as \${conclusion}`, not `success``) includes the actual conclusion value and doesn't rely on knowledge of what the audit step was, making incidents easier to triage.

Recovery guidance updated correctly. Adding "and the incident-issue write in ci-runner" to step 3 is accurate — under the new contract, a write failure is now a legitimate failure mode this liveness check covers.

---

Security

No security concerns:

• Triggers unchanged (schedule, push, workflow_dispatch). No pull_request_target or workflow_run with secrets — the CLAUDE.md tripwire is not implicated.
• Permissions unchanged: contents: read at workflow level; contents: read + issues: write at job level.
• peter-evans/create-issue-from-file remains SHA-pinned at fca9117c27cdc29c6c4db3b86c48e4115a786710 # v6.0.0 — unchanged.
• The removed second gh_read api .../jobs call is gone, which is a mild security improvement: the job no longer needs read access to the per-run job list. (The token already had it, but dead code that makes API calls is better gone.)
• No new credentials, no new echoes of tokens, no untrusted checkouts.

---

CLAUDE.md alignment

• SHA pins: no new uses: introduced; existing pin is intact.
• The new inline comment at lines 95–102 is a good "why" comment — it explains a non-obvious external contract, exactly the use case CLAUDE.md calls out.

---

No findings. Safe to merge.

[claude]

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

---

Security Review

• Gather context and read PR diff
• Read changed file in full
• Analyze for security vulnerabilities
• Post findings

---

No security vulnerabilities found in this PR. This is the security-relevant analysis of what changed:

Attack-surface reduction (net positive)

The old code extracted step names from the ci-runner API and interpolated them into the problems string that became a GitHub issue body:

broken_steps="$(gh_read api "repos/${MONITOR_REPOSITORY}/actions/runs/${run_id}/jobs" \
  --jq '[.jobs[].steps[] | select(.conclusion == "failure") | .name] | unique | .[]')"
unexpected="$(grep -Fxv "$MONITOR_AUDIT_STEP" <<<"$broken_steps" || true)"
problems+="...(failed: $(tr '\n' ',' <<<"$unexpected" | sed 's/,$//')); ..."

Step names are developer-authored strings from workflow YAML files. They were flowing — without sanitization — into an issue body (markdown injection surface). The risk was minimal (org-controlled repo, issue body only, not a code-execution context), but it was the most user-influenced content in the pipeline.

The new code removes that entire branch. The only values now interpolated into problems are:

• run_id — a GitHub-generated numeric integer; no injection potential
• conclusion — a fixed GitHub enum (success / failure / cancelled / timed_out / action_required / stale / neutral); no special characters possible

Unchanged surfaces (no regression)

• Trigger set is unchanged (schedule, push, workflow_dispatch). No pull_request_target or workflow_run introduced.
• permissions: block unchanged (contents: read, issues: write).
• The only ${{ }} expressions in run: blocks are secrets.GITHUB_TOKEN (line 55) and github.repository (line 151) — both passed as env: vars, not inline script interpolation. No change here.
• peter-evans/create-issue-from-file remains SHA-pinned at fca9117c27cdc29c6c4db3b86c48e4115a786710 # v6.0.0 — no regression.

Logic correctness note (not a security issue, but relevant to alert contract)

The PR description correctly identifies the transition-window behavior: under the old contract, merging this first means a legitimate failure-conclusion monitor run (the old alert shape) would now be classified as breakage and fire the watchdog. That is the acknowledged correct-if-noisy behavior; there is no security consequence to it.

---

Verdict: Clean. The simplification removes the most complex data-flow path (step-name extraction and interpolation) and replaces it with values that are structurally impossible to contain injection payloads.