feat(ci): green-on-success queue monitor with issue-per-incident alerting - #140
Merged
Conversation
…ting The managed-runner queue monitor alerted by intentionally failing its own scheduled run, which polluted fleet-wide failure dashboards and reached nobody since schedule-triggered runs have no actor. A successful execution now stays green regardless of what it detects; detection instead upserts a marker-deduped incident issue per target owner in this repository (the fleet's established alert-per-incident pattern), closing it with a recovery comment once the queue clears. Genuine execution errors still fail the run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
![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: be9f06a887
ℹ️ 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
enabled auto-merge (squash)
July 22, 2026 19:22
kyle-sexton
changed the title
…tity The incident title and marker embedded in queued-job-monitor.yml are public (this is a public repository), so matching on them alone would let a non-maintainer open a decoy issue this automation adopts, silently updates, or closes as recovered - suppressing a real alert. Adds a body-embedded marker (replacing title-only matching) and restricts adoption to open issues authored by this workflow's own GITHUB_TOKEN identity (github-actions[bot]), failing closed on an ambiguous multi-match. Mirrors the same hardening applied to the sync-alert workflow in ci-workflows#213. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
kyle-sexton
disabled auto-merge
July 22, 2026 19:31
Contributor
Author
|
🔒 babysit-prs lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, safe tier). Will fix clear branch-owned findings and push; will not resolve threads or merge (safe tier). |
Three fixes from independent review of the queue-monitor alert channel: - STUCK_JSON now requires an explicit value (even "[]" for zero stuck jobs) instead of falling back to '[]' on missing/empty input. A missing detection-step output - a wiring bug or skipped step, not a healthy run - would otherwise be silently treated as recovered and close a real open incident. Malformed JSON and a non-array payload are now rejected explicitly too. - findOpenIncident filters the open-issue scan to the 'automated' label server-side (every incident issue already carries it), instead of paginating the full set of open issues in the repo on every ~15 minute run while an incident stays open. - Documents that the upsert's create-if-absent race freedom for a given target owner depends on the workflow's cancel-in-progress: false concurrency group serializing scheduled runs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dc449e2fb
ℹ️ 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
added a commit
to melodic-software/ci-workflows
that referenced
this pull request
Jul 22, 2026
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 melodic-software/ci-runner#139 (issue) and melodic-software/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](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Reviewer-suggested hardening on top of the author-identity restriction: renderStuckMarkdownTable embeds monitored-repo job/workflow names verbatim (pipe-escaped for table integrity, not HTML-comment-escaped), so a crafted job name in a monitored repo could inject a different owner's marker string into a bot-authored incident body. Requiring an exact title match too - built solely from the trusted targetOwner value, never from monitored content - closes that cross-owner collision path even though the marker alone could be spoofed via injected content. Adds a regression test locking the marker format's trailing delimiter that already made prefix-related owners (melodic-software vs melodic-software-fork) substring-safe. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… title guard Per team-lead ruling: root-cause the cross-owner marker-injection risk by encoding '<'/'>' in escapeMarkdownTableCell (all monitored-repo content entering the incident body) instead of layering a title-equality guard onto findOpenIncident. Escaping at the source closes the injection for every consumer of that content and restores the fleet precedent's deliberate marker-survives-retitle property, which the title guard would have traded away for redundant protection now that the injection itself is neutralized. Keeps the cross-owner marker-distinctness regression test (a format-level invariant, still worth pinning) and replaces the title-guard tests with direct coverage of the neutralized rendering and an end-to-end proof that a crafted job name embedding a foreign marker cannot cause cross-owner incident adoption. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c7adfffec
ℹ️ 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".
… failure modes Per team-lead ruling after independent review escalated the cross-owner marker-injection risk to CRITICAL (against a commit predating the escape-at-source fix): adds direct end-to-end coverage of the exact scenario a monitored-repo job name carrying another owner's marker text cannot achieve any of three failure modes against that owner's genuine, currently open incident - adopt-and-overwrite it, false-close it, or trigger a false fail-closed ambiguity error. Bodies are built through the real render/assembly shape, not hand-written, so the tests exercise the actual pipeline. Also adds an exact-string round-trip test (a job name that IS a legitimate-looking marker still renders inert) and a pipe-escape/comment-escape composition test. Also documents, per the reviewer's low-confidence suggestion, that CI_RUNNER_MONITOR_TARGETS_JSON entries sharing an owner would race the incident-issue upsert intra-run (the concurrency group only serializes across runs, not the matrix within one). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
kyle-sexton
enabled auto-merge (squash)
July 22, 2026 20:42
…limit Per Codex review: an unbounded number of stuck jobs could grow the detection table (and therefore the incident issue body) without limit. GitHub rejects an issue/comment body over 65536 characters (empirically observed API error, not a formally documented field limit but consistently reproduced). renderStuckMarkdownTable now caps at 50 rows and appends a "...and N more, see the workflow run" remainder note linking to the run (GITHUB_SERVER_URL/GITHUB_RUN_ID, GitHub Actions' own default env vars). boundBodyLength is a defense-in-depth backstop that truncates the assembled body if it would still exceed the limit, while always preserving the trailing incident marker fully intact - truncating it would break every future upsert/close for that incident. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c02681de69
ℹ️ 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".
|
|
||
| // Handed to the incident-upsert step via job output; a successful execution | ||
| // stays green from here regardless of what it found (see upsertIncident). | ||
| core.setOutput('stuck', JSON.stringify(stuck)); |
There was a problem hiding this comment.
When a fleet-wide outage queues thousands of managed jobs, this serializes the entire stuck array into a step output before the upsert step can apply MAX_STUCK_TABLE_ROWS/MAX_BODY_LENGTH. GitHub caps outputs at 1 MB per job, so the inspect step can fail or hand the next step a truncated/empty STUCK_JSON exactly during the largest incidents; cap the data placed in the output or hand it off through a workspace file while preserving the total count.
Useful? React with 👍 / 👎.
kyle-sexton
added a commit
to melodic-software/ci-workflows
that referenced
this pull request
Jul 24, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #139
Summary
queued-job-monitor.ymlno longer alerts by failing its own scheduled run. A successful execution is green regardless of what it detects; detection upserts a marker-deduped incident issue per target owner in this repository instead ([Alert] Managed runner queue capacity — <owner>, with a hidden<!-- ci-runner:queued-job-monitor:incident:<owner> -->marker in the body). Matching is marker-only, preserving the fleet precedent's deliberate "a marker survives a retitle" property (link-check.yml,queue-monitor-liveness.yml,standards-sync-stuck-automerge-alert.ymlinci-workflows).48b3a6a): candidate issues are restricted to ones authored by this workflow's ownGITHUB_TOKENidentity (github-actions[bot], verified live), and more than one own-authored issue carrying the marker fails the run closed. Mirrorsci-workflowsfix(doctor): ignore vanished entries during acl tree verification #213.4dc449e):upsertIncidentrequiresSTUCK_JSONto be explicit/valid/array-decoding.findOpenIncidentfilters server-side tolabels: 'automated'(load-bearing for dedup — documented).4c7adff/ad6c79c):escapeMarkdownTableCellHTML-entity-encodes</>so no<!--/-->sequence from untrusted monitored-repo content can survive into the raw body textfindOpenIncidentsubstring-searches. End-to-end tests prove all three failure modes (adopt-overwrite, false-close, false-ambiguity) are prevented. Independently confirmed by a hard-bounded micro-review.c02681d, from Codex review):renderStuckMarkdownTablecaps at 50 rows with a "...and N more, see the workflow run" remainder note (linked viaGITHUB_SERVER_URL/GITHUB_RUN_ID).boundBodyLengthis a defense-in-depth backstop truncating the assembled body under GitHub's empirically-observed 65536-character write limit while always preserving the trailing marker intact.automatedlabel is never applied to a monitored repo —upsertIncidentcreates issues exclusively inhomeOwner/homeRepo, derived fromGITHUB_REPOSITORY(this repo, ci-runner), never a monitored target; and the marker-injection finding was already fixed by the escape-at-source commit above (reply cites the commit, the tests, and the micro-review verdict).STUCK_JSON) still fails the run. The "Upsert queue capacity incident" step runs only on the defaultsuccess()condition.GITHUB_TOKEN(job-levelissues: write), separate from the read-only, target-scoped observer App token used to inspect queued jobs.docs/queue-monitor.mdupdated throughout to describe the current design.Config-integrity note (documented in-file): two
CI_RUNNER_MONITOR_TARGETS_JSONentries sharing an owner would race the incident-issue upsert intra-run (the concurrency group only serializes across scheduled runs, not the matrix within one).Companion PR: melodic-software/ci-workflows#222 — MERGED.
Environment note:
ci-runneris public, so itsubuntu-24.04runner is free/unmetered, unaffected by the org's $0 spending cap. This monitor is deliberately kept hosted, off the managed self-hosted fleet, so it still reports when both self-hosted hosts are down.Test plan
node --test .github/scripts/queue-monitor.test.cjs— 37/37 pass.typos,npx markdownlint-cli2,ec(editorconfig-checker) run clean against every changed file.node --test .github/scripts/*.test.cjs(full suite): all other suites pass; 5 pre-existing failures inverify-existing-release.test.cjsare unrelated Windows-only temp-dirEPERMcleanup issues, reproduced identically onmain.ad6c79c;c02681dre-running the same gates now.Related
docs/topics/cicd-fleet-audit/REPORT.md(Theme 1 F6 + Status-quo BP4)melodic-software/ci-workflowsqueue-monitor-liveness.yml,link-check.yml,standards-sync-stuck-automerge-alert.yml/ ci-workflows#213🤖 Generated with Claude Code