ci: make the cleanup gate and the stuck-job watchdog mean what they report - #331
Open
Eli Pinkerton (wallstop) wants to merge 18 commits into
Open
ci: make the cleanup gate and the stuck-job watchdog mean what they report#331Eli Pinkerton (wallstop) wants to merge 18 commits into
Eli Pinkerton (wallstop) wants to merge 18 commits into
Conversation
…entory
The watchdog has been reporting success while blind. It listed runners from
two endpoints its token can never reach -- `/orgs/{org}/actions/runners` is
organization-scoped and the job's GITHUB_TOKEN is repository-scoped, and the
repository-level fallback needs `administration: read` -- logged an ERROR, took
no action, and exited 0. A `Performance Numbers` run then sat queued for ten
hours with the automation meant to notice it green every five minutes (#328).
The repository-level fallback was worse than no fallback: this repository
registers zero repository-level runners, so with the permission added the call
would succeed and report an empty inventory, which reads identically to "no
runner matches" and still issues no action. It is removed rather than fixed.
Inventory now comes from the build-lock reader App, the credential
`check-unity-runner-availability` already uses, narrowed to
`organization_self_hosted_runners: read` and walking only the runner groups
visible to this repository, so a runner another repository owns is never
mistaken for one that could have taken our job.
Every input the verdict depends on -- the queued-run listing, the runner
inventory, a per-run job listing, and the cancel-cap state branch -- now fails
the job instead of exiting 0. A green run means the queue was evaluated.
The single "no matching idle runner" test conflated four states, so the one
case #328 actually hit was invisible. They are now distinguished: idle
(dispatcher-stuck, cancel), busy (healthy backpressure), offline and
unregistered (starved -- reported in its own step-summary section and as a
`::warning::`, never cancelled, because the fleet still intends to run it).
A GitHub-hosted label set is unregistered by definition and is reported as
not evaluable rather than as starvation.
Two latent defects found while rewriting: label matching was case-sensitive in
both directions, and an empty label set vacuously satisfied the superset test,
which would have manufactured a dispatcher-stuck verdict and cancelled the run.
The cancel-cap state branch is now materialized only after a run is classified
dispatcher-stuck, so the common cycle clones nothing and cannot be failed by a
state-branch problem it never needed to solve.
Covered by an executed truth table in scripts/validate-unity-pr-policy.py that
runs the workflow's own audit script against a stubbed `gh` and `git` across
thirteen cases. It reads the step's literal `env:` from the workflow rather
than restating it, so a case cannot pass by asserting its own stub. Verified
live against ten mutations, including the exact #328 regression.
Refs #328
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A licensed Unity leg that aborts at its first step produced two failures: the honest one, and a second from `Require confirmed Unity cleanup` reporting a cleanup that was never needed. Five of nine legs in run 30642224168 showed the shape (#327). `Return Unity license` and `Classify Unity cleanup evidence` correctly skip when the lock was never acquired, so the gate read an empty `classification-complete` and failed closed on work that never started. The gate already had the right contract -- its `acquired` input documents that `false` proves licensed cleanup was not required, and it short-circuits every other check on that value. The workflow simply never supplied it: a skipped `Acquire organization Unity lock` publishes no outputs, so the gate saw empty rather than false. All five licensed lock windows now derive the input from `outcome`, which is empty or `skipped` only when the step did not execute. That keeps the bare `if: always()` the issue asks for, and keeps the case the gate must never miss: an acquire that RAN and failed part-way reports outcome=failure while `acquired` reads empty, so the empty value still flows through and the gate still fails. Gating the gate on `acquired == 'true'` would have skipped exactly that seat leak. Pinned in scripts/validate-unity-pr-policy.py across every window in LICENSED_LOCK_WINDOWS: the bare `always()` must survive, the input must map only a never-executed step to false, and an `outcome == 'failure'` clause is rejected outright so a real leak cannot be laundered into a not-attempted verdict. Verified against three mutations. Refs #327 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Cursor (@cursor) review |
A merge could land up to three follow-up commits on master -- one per generator -- and each one re-triggered the full push-side workflow set. Roughly a third of recent master history is bot commits (#330). update-llms-txt.yml and update-issue-template-versions.yml were the same 180-line workflow twice: same App token, same branch refresh, same three-attempt push loop, different `npm run` line. They are replaced by post-merge-maintenance.yml, which runs both generators and pushes at most ONE commit. The file list lives in a single GENERATED_PATHS env var that the change probe, the staging step, and the push loop all read, so a third generator is wired in one place or not at all. perf-numbers.yml deliberately stays separate: it runs on a self-hosted Windows runner behind the organization Unity build lock and can take tens of minutes, so folding these two-minute generators behind it would delay them by the benchmark's whole queue for no benefit. That left the reason the llms.txt commit fired at all. `update:llms-txt` restamped "Last Updated" on every run while `check:llms-txt` deliberately normalizes the date away, so the generator produced a diff on any day it ran and the checker never objected. 1efb732, the last llms.txt auto-commit on master, changed exactly one line -- the date -- and still re-triggered CI, Release Drafter, and Deploy Documentation. The date now survives a byte-identical regeneration and moves only when the content moves, so the field means what it claims and the bot commits only when it has something to say. The comparison normalizes ONLY the date, not the skill-count claim that normalizeForComparison also folds away: a count change is real content and must carry the date with it. Covered by a CLI-driven test that exercises the real write path in both directions; reverting to an unconditional restamp, or preserving the date through a genuine content change, each fail the suite. JS budget raised to 17582 with entry 077. The change is a net simplification of the workflow layer (367 YAML lines to 219) that costs a small, tested addition to the generator that removes a recurring no-information commit. Refs #330 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Cursor (@cursor) review |
None of these were caught by the branch's own suite, so each fix ships with a case that fails without it. **The watchdog kept one fail-open, in the first input it reads.** `mapfile -t x < <(jq ...)` discards the process substitution's exit status, and `pipefail` does not reach inside one. A single unparseable `created_at` aborted jq, left the id list empty, and printed "Queue is clean. No action." over a genuinely stuck queue -- the exact #328 shape the rewrite claims to have eliminated, in the one read that was not wrapped in `fail_closed`. Fixed at all three jq read sites, not just the reachable one. **The starvation report latched on the first label set while the verdict accumulated across all of them.** Because jq `unique` sorts, a run with one GitHub-hosted job and one starved self-hosted job reported "targets GitHub-hosted capacity" when the hosted label sorted first (`macos-latest`) and reported starvation when it sorted last (`ubuntu-latest`). Identical semantics, opposite verdict, decided by alphabetical order -- and in the losing case the `::warning::` was suppressed entirely, which is the blindness the rewrite exists to remove. When it did warn, it interpolated the first set's labels, so it could tell an operator to bring online a runner named `macos-latest`. The whole-run verdict and the reported set are now tracked separately, and only a self-hosted set can drive starvation. **Any unanticipated abort produced a red run with an empty summary.** `emit_summary` was reachable only through `finish`. An EXIT trap now emits the buckets on every path and labels aborts that did not come from `finish`. The concrete trigger: `EXCLUDED_BY_FILE[""]` is a hard bash error, and the code already anticipates a null workflow path via `(.path // "")`. A run whose workflow cannot be named is now reported and left alone -- cancelling is the one irreversible thing this job does. **Merging the two generators re-coupled what two workflows kept apart.** A `bug_report.yml` the dropdown generator could not process would abort the step before llms.txt drift on master was ever corrected. They are now one step each, the second running even when the first failed, and each reverts its OWN files when its `--check` rejects them -- so the tree can only ever carry accepted output, the generator that converged still self-heals, and a terminal gate keeps the job red. Verified by executing it: a failing dropdown generator leaves llms.txt regenerated and its own garbage discarded. **`preserveUnchangedDate` removed update mode's ability to repair a malformed date.** The comparison erases the date, so a file whose only defect IS a corrupt date line looks identical to a fresh generation; copying the corrupt line back left a state the post-write validator rejects, and update exited 1 while advertising itself as the remediation. Reproduced against master: `unknown` was repaired to a real date before, and preserved after. Now only a date the generator would itself accept is carried forward. **The runner-group visibility claim was stronger than the evidence.** The header asserted the filter as a guarantee against counting another repository's runners. It could not be verified from here, and the organization currently has a single group, so visible-to-us and all-org-runners are the same set today. The comment now says what the parameter actually buys, and the counted group names are logged so a future second group is visible in the summary. JS budget 17612 (entry 077 extended). Refs #328, #330 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Cursor (@cursor) review |
The actionlint container CI runs ships a newer shellcheck than the local one, and the two report the same thing under different codes: 0.9 calls the body of a trap-invoked function unreachable (SC2317), newer builds call the function never invoked (SC2329). The disable listed only SC2317, so `Lint GitHub Actions workflows` failed on the previous commit. The first attempt at explaining this in a comment made it worse. A comment line that BEGINS with the analyzer's own name is parsed as a directive, so the prose became a malformed one (SC1073) and shellcheck stopped analyzing the whole 500-line script rather than just that function. The wording now keeps the name out of the leading position and says why. Verified against shellcheck 0.11.0 -- newer than CI's -- at `-S info`, which is stricter than the container's default: both this workflow and post-merge-maintenance.yml are clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Cursor (@cursor) review |
Round two reviewed the round-one fixes. One of them was worse than what it
replaced, and three shipped with a coverage claim that was not true.
**The generator isolation defeated its own gate.** `regenerate()` in the push
loop ended each branch in `|| git checkout`, which succeeds, so the function
always returned 0. A generator that broke only on the REFRESHED head (the
stale-head retry path) reverted silently, the loop pushed the other generator's
output, and the job went green with stale content and no annotation -- while
the terminal gate read the two step outcomes, both `success`, because the
failure happened in the loop rather than in the steps. It now records the
failure and every success exit carries that verdict. Executed end to end
against a real local remote: the step exits 1 with an error annotation where it
previously exited 0.
**The empty-subscript fix only hardened the read site.** `EXCLUDED_BY_FILE`
is WRITTEN from the exclusion list, where `base="${wf##*/}"` turns any operator
entry ending in `/` into an empty subscript -- a hard bash error that kills the
audit before it reads anything, every five minutes, on a repo-variable typo.
The guard checked `wf`, not `base`.
**`Check for changes` fail-opened.** `git diff --exit-code || echo changed=true`
treats 128/129 ("not a git repository") the same as 1 ("there are
differences"). That was unreachable while a failed `Checkout` skipped the step;
adding `if: always()` in the last commit made it reachable, so a failed
checkout would have reported `changed=true` and driven the commit step. Only
exit 1 now means changed; anything else fails.
**Three fixes had no regression coverage**, contrary to what the last commit
message claimed. Removing the EXIT trap, dropping the emit-once guard, and
deleting the runner-group name log all left the suite green. All three now have
cases; the trap is exercised through a failing `date`, which is the only
unguarded assignment left and therefore the only way to reach an abort the
script does not anticipate.
**The not-evaluable branch claimed something it could not know.** "Targets
GitHub-hosted capacity" is not the same statement as "no job asked for
self-hosted", and can be false -- a registered runner may carry `ubuntu-latest`
and simply be offline. Reaching that branch provably implies no queued job
requested `self-hosted` (an idle self-hosted set breaks out, a busy one returns,
and offline/unregistered ones set `starved_kind`), so the second arm was dead
code. It is deleted and the message now states only what is true.
Also: the third `mapfile` site kept the empty guard its two siblings got. It is
currently unreachable -- `group_count == 0` fail-closes first -- so it is fixed
for consistency and deliberately NOT pinned by a test, because a case for
unreachable code is the fragile kind the plan says to remove rather than add.
Verified against shellcheck 0.11.0 at `-S info`, stricter than CI's container.
Refs #328, #330
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Cursor (@cursor) review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Eli Pinkerton (wallstop)
enabled auto-merge (squash)
July 31, 2026 21:21
Round four fuzzed the audit script 120 ways and killed 47 of 57 mutations, so
the workflow logic itself holds. These are the ten it could still delete.
**`workflow_dispatch:` detection was never exercised.** The dispatcher-stuck
case feeds `{"content": ""}`, which is not valid base64, so `base64 -d` fails
and the grep never runs -- it reads as though it asserts detection while
actually asserting the decode-failure path. Replacing the grep with `true`
survived. A regression there would POST to a workflow with no such trigger, get
a 422, and leave the run cancelled with no recovery. Pinned with a body that
decodes cleanly and declares only `on: push`, which is the one input that
separates the two.
**Starvation precedence was entirely unpinned.** No case had two self-hosted
sets starving for DIFFERENT reasons, so inverting the offline-versus-
unregistered preference changed nothing. The inverted report points the
operator at a machine that exists and will reconnect instead of the label set
that needs a human -- the exact swap the code comment claims to prevent.
**The 24h cap reset was structurally unreachable.** The stub hardcoded
`last_cancel` an hour ago, so no case could place it outside the window;
widening the window to 999999999 survived. The age is now a parameter, and a
25-hour-old cap must reset and cancel again.
**The reader App token was proven minted, not used.** Dropping
`GH_TOKEN="${RUNNER_INVENTORY_TOKEN}"` from `gh_reader` survived even though the
harness already gave the two tokens distinct values. The stub now 403s any
`orgs/*` call that does not carry the reader token, which is precisely #328's
credential shape.
**The push loop's top-of-loop regeneration was dead in the fixture.** Only the
bottom-of-loop path (after a failed push) was reachable, so deleting the
stale-head regeneration survived. That branch fires when master advances
between the generator steps and the commit step -- without it the loop commits
the old head's output onto a new head, which is what the whole stale-head
design exists to prevent.
**Per-cancel state persistence was unobservable.** Only one dispatcher-stuck run
was ever exercised, so the final sync covered it and deleting the per-cancel
push changed nothing. That immediacy is what makes the cap survive a crash
partway through the loop; without it a job that dies after cancelling loses
every increment and the next cycle cancels the same runs again. Two stuck runs
now assert the push count.
Also confines the audit's `mktemp` churn to each case's own directory. It was
leaking roughly 440 entries per validator run into the developer's /tmp --
free on an ephemeral runner, permanent locally. Measured delta is now zero.
The empty-clone warnings the push-loop fixture printed to stderr are captured.
`validate:all` stays clean; the validator runs in 17s.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| echo "::error::llms.txt generation did not converge; discarding its output." | ||
| # shellcheck disable=SC2086 # LLMS_PATHS is an intentional word list. | ||
| git checkout -- ${LLMS_PATHS} | ||
| exit 1 |
There was a problem hiding this comment.
Partial maintenance commit skips llms
Medium Severity
Regenerate llms.txt has no if: always(), so a failed Install dependencies skips it while the issue-template generator, change probe, and commit steps still run. Both generators are plain node scripts and do not need node_modules, so the issue-template path can still converge and push. The terminal gate also treats skipped as success, so a skipped llms leg does not fail that check. Relative to the old separate workflows, that is a regression: install failure used to skip the commit entirely; the merged workflow can land a partial auto-commit that never refreshed llms.txt.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 1775064. Configure here.
Round five found no production defect -- the workflow logic is equivalent to the pre-refactor version across all 1364 verdict sequences, and 500 fuzzed combinations produced no unbound variable, no bad subscript, and no non-zero exit missing its summary. These are the four behaviors its mutations could still delete. **The harness could not tell the step summary from the job log.** `run_watchdog` returned `summary + stdout + stderr` as one string while `log_summary` tees to stdout, so every needle was satisfiable from the log alone and NO case could prove a line reached `GITHUB_STEP_SUMMARY`. Three of the four buckets and the entire audit narrative were deletable with the suite green -- the operator's summary would have degraded to four bare headers unnoticed. The returned value is now a `str` subclass carrying `.summary`, so existing assertions are unchanged and the channel-specific ones are possible. **The starvation precedence fix was only superficially closed.** jq `unique` sorted the fixture's UNREGISTERED set first, so plain first-wins already gave the right answer and the upgrade clause never executed. Round four pinned "a later set must not clobber an earlier one", not the upgrade its comment claims. The sets are relabelled so the offline one sorts first, which is the only ordering where the clause does any work. **A rejected `gh run cancel` was unpinned.** `cancel_ok` existed as a knob that no case ever set. Without the guard the audit increments the cap, pushes state, and RE-DISPATCHES a run it never cancelled -- a duplicate of live work. **The maintenance terminal gate had no coverage at all**, despite its own comment saying that without it a generator that never converged is reported green. Eight outcome pairs now execute it. Alongside it: both generator reverts and the non-race push failure are pinned by reading back what actually reached the remote, per file. Asserting exit codes alone let the reverts be deleted, and a push rejected while master stood still could report green having pushed nothing. Two fixture bugs surfaced only by running these. The combined-blob assertion could not tell whose marker it saw, since the SUCCEEDING generator legitimately writes one; and a transient failure that later converges legitimately pushes its output, so the revert assertion belongs only on the never-converged rows. Also documents in .llm/context.md that agents must not poll GitHub: every `gh` invocation re-presents credentials, so a 60-second watch loop over a 40-minute Unity matrix is 40 authenticated calls, and several at once multiply it. The credential ORDER was already documented; the volume rule was not, and this session violated it badly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…te blocks GOAL.md asks for data-driven tests over repeated similar cases, and the watchdog suite had drifted the other way: 31 table rows alongside 16 hand-rolled run-and-assert blocks, most differing only in one keyword argument. Each block was individually justified by a surviving mutation, which is exactly how a suite accretes duplication without anyone deciding to. Table rows now carry an optional sixth element of `run_watchdog` kwargs, so a case needing a failing push, a pre-populated cancel cap, a stale cap, a failing `ls-remote`, a failing clone, or a rejected cancel is a ROW. Six blocks folded; the whole verdict space reads as one table. 136 lines removed, and all six behaviors still fail their own mutation as rows. Four standalone blocks remain on purpose, because their assertions are not substring checks and forcing them into the table would obscure rather than share: the summary-channel loop (asserts the `.summary` view specifically), the emit-once count, the per-cancel push count, and the EXIT-trap probe. The validator is 3032 lines against master's 1774. That is a lot, and worth stating plainly: it covers ~1080 lines of workflow bash that can cancel a live Unity run and can free a licence seat, none of which any other check in this repository executes. Every case earned its place by killing a mutation that survived without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Cursor (@cursor) review |
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The stuck-job watchdog’s runner-group inventory filter uses visible_to_repository with the repo name (not repo id) and the policy validator adds an unused Python import, both of which should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
.github/workflows/stuck-job-watchdog.yml:302
- Runner-group inventory filtering uses
visible_to_repository=${repo_name}, but the API parameter is keyed by repository id. Using the repo name risks getting an empty/wrong inventory and misclassifying queued runs. Use the numeric id passed via env instead.
repo_name="${REPO##*/}"
runner_groups_json="$(mktemp)"
if ! gh_reader api --paginate \
"orgs/${OWNER}/actions/runner-groups?visible_to_repository=${repo_name}&per_page=100" \
| jq -s '[.[] | (.runner_groups // [])[] | select(.id != null)]' > "${runner_groups_json}"; then
scripts/validate-unity-pr-policy.py:13
datetime/timezoneare imported but never used (the file contains no otherdatetime/timezonereferences). This will trip linting/type-checking in stricter environments and is dead code.
import re
import subprocess
import tempfile
from datetime import datetime, timezone
from pathlib import Path
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
126
to
130
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| RUNNER_INVENTORY_TOKEN: ${{ steps.reader_token.outputs.token }} | ||
| REPO: ${{ github.repository }} | ||
| OWNER: ${{ github.repository_owner }} | ||
| SELF_RUN_ID: ${{ github.run_id }} |
Cursor Bugbot, two medium findings on 98cd209, and both are the same self-inflicted regression. The previous commit folded six standalone cases into table rows and removed 136 lines. It also removed a span that contained two OTHER blocks -- the EXIT-trap probe and the step-summary channel assertions -- which had been added one commit earlier to close round five's findings. `validate:all` stayed green, so I shipped it. Green was not evidence. Those two blocks only fail when the WORKFLOW is mutated; deleting the assertions themselves breaks nothing. So the suite lost the coverage silently, which is precisely the failure mode this file exists to catch, arriving through a cleanup. Bugbot spotted it structurally, from the fact that `WatchdogOutput.summary` and `break_date` had no callers left -- a dead seam is the visible symptom of a deleted test. Both restored, and both verified live: removing `trap on_exit EXIT`, deleting `cat "${summary_file}"`, deleting the Starved bucket, or dropping `log_summary`'s `tee` each fail the suite again. Without them, all four survive. The lesson worth keeping: a refactor of a TEST file cannot be validated by running the tests. It has to be validated by re-running the mutations the tests exist to kill, which is the check I skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Cursor (@cursor) review |
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The current diff introduces a bash-pattern-matching hole in the watchdog gh stub (unescaped case needles) plus a couple of small correctness/robustness nits (unused Python import and an unquoted associative-array key) that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
scripts/validate-unity-pr-policy.py:13
datetime/timezoneare imported but never used, which adds noise and can trip linters.
from datetime import datetime, timezone
scripts/validate-unity-pr-policy.py:876
watchdog_gh_stubbuilds a bashcasepattern fromneedlewithout escaping glob metacharacters (notably?in query strings). That can make the stub match the wrong endpoint and let a regression slip through the truth-table (e.g., a changed query delimiter still matching). Quote/escapeneedlebefore embedding it in thecasepatterns.
cases = "".join(
f" *{needle}*)\n cat <<'PAYLOAD'\n"
f"{payload if isinstance(payload, str) else (json.dumps(payload) if payload is not None else '')}"
f"\nPAYLOAD\n exit {code} ;;\n"
for needle, (code, payload) in routes.items()
)
.github/workflows/stuck-job-watchdog.yml:423
- The associative-array lookup uses an unquoted key:
${EXCLUDED_BY_FILE[${run_path_base}]+x}. Ifrun_path_baseever contains glob characters or whitespace, bash will perform expansions and the exclusion check can misbehave. Use the quoted form (consistent with the write site above).
if [[ -n "${EXCLUDED_BY_FILE[${run_path_base}]+x}" ]]; then
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…wo gaps Round six established that the table fold in 98cd209 deleted SIX assertion blocks, not the two Bugbot found. Bugbot could only see the two that left a dead seam behind -- `WatchdogOutput.summary` and `break_date` with no callers. The other four left no such trace, so nothing pointed at them. Each was killed before the fold and survived after it: - **the cancel cap can be made completely inert.** Deleting `state_dirty=1` makes `persist_state_changes` return early on every call, so no cancel count is ever committed. Every cycle then reads `cancels=0` and cancels the same run again, without limit, against live Unity runs holding licence seats. This is the single most dangerous unpinned behavior in the file and the fold removed its only test. - the unparseable-job-labels guard could become `continue` -- a run skipped and the audit green over a queue it never evaluated, the #328 fail-open shape. - the unclassifiable-run guard could become `continue`, same shape. - `MIN_QUEUE_AGE_SECONDS: "300"` could become `"0"`, classifying normal scheduling latency as dispatcher-stuck and cancelling it. Three are restored as table ROWS, so the fold's benefit survives; only the cap needs a standalone block, because it asserts a push COUNT. The `datetime` import is live again, which is what its unused state was quietly reporting. Also closes two gaps round six found that predate the fold: - Two of the four `exit "${regenerate_failed}"` sites were unreachable, so either could become `exit 0` undetected -- reinstating the always-green bug 764540b claims to fix. The "already current" exit is now reached by a `quiet_other` fixture where the failing generator's revert leaves nothing to commit. - The exclusion list only ever saw bare filenames, leaving `base="${wf##*/}"` unpinned. `release.yml` cannot prove it, since the workflow's own default list already excludes it; a workflow excluded SOLELY by a prefixed repo-variable entry can. Two fixture bugs of my own surfaced while writing these, both the same shape as the bug being fixed: a case that does not reach the branch it names. The exclusion case was shadowed by the default list, and the "already current" case never got there because the other generator rewrote its file every call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Cursor (@cursor) review |
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The review found a few concrete issues in the new policy harness and maintenance workflow scripts that should be corrected before relying on them for CI safety guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (9)
scripts/validate-unity-pr-policy.py:1243
- This
contents/...stub is fed togh api ... --jq '.content'in the watchdog script, so it should be the raw base64 string (possibly empty), not a JSON object with acontentfield.
"actions/workflows/42": (0, {"path": ".github/workflows/perf-numbers.yml"}),
"contents/.github/workflows/perf-numbers.yml": (0, {"content": ""}),
},
scripts/validate-unity-pr-policy.py:1570
- This
contents/...route is used withgh api ... --jq '.content'in the watchdog. It should stub the jq-extracted string output, not an object, to match realghbehavior and the stub’s own comment.
"actions/workflows/42": (0, {"path": ".github/workflows/perf-numbers.yml"}),
"contents/.github/workflows/perf-numbers.yml": (0, {"content": ""}),
},
scripts/validate-unity-pr-policy.py:1879
- This stubbed workflow-contents call is consumed through
gh api ... --jq '.content' | base64 -din the watchdog. It should be the extracted base64 string, not a JSON object, to match realghoutput and avoid exercising decode-failure paths unintentionally.
"actions/workflows/42": (0, {"path": ".github/workflows/perf-numbers.yml"}),
"contents/.github/workflows/perf-numbers.yml": (0, {"content": ""}),
}
scripts/validate-unity-pr-policy.py:1547
- The watchdog calls
gh api ... --jq '.content'for workflow contents; this stub should provide the extracted base64 string, not a JSON object. Using JSON here makes the test depend on base64-decode failing rather than the intendedworkflow_dispatchdetection behavior.
"actions/workflows/42": (0, {"path": ".github/workflows/perf-numbers.yml"}),
"contents/.github/workflows/perf-numbers.yml": (0, {"content": ""}),
},
scripts/validate-unity-pr-policy.py:1658
- The watchdog consumes workflow contents via
--jq '.content', which returns a base64 string. Stubbing it as{ "content": "" }doesn’t match the actual CLI output format and can mask regressions in the dispatchability branch.
"actions/workflows/42": (0, {"path": ".github/workflows/perf-numbers.yml"}),
"contents/.github/workflows/perf-numbers.yml": (0, {"content": ""}),
},
scripts/validate-unity-pr-policy.py:1031
run_watchdogreturns aWatchdogOutput(and callers use the.summaryattribute), but its signature declares-> tuple[int, str]. This makes the annotation inconsistent with runtime behavior and breaks static analysis/IDE assistance for.summaryusages.
) -> tuple[int, str]:
scripts/validate-unity-pr-policy.py:1188
- The watchdog reads workflow content via
gh api ... --jq '.content', which outputs the raw base64 string. Stubbing this route with a JSON object ({"content": ""}) contradictswatchdog_gh_stub’s own contract (routes feeding--jqshould supply the extracted value) and exercises base64-decode failure for the wrong reason.
This issue also appears in the following locations of the same file:
- line 1241
- line 1545
- line 1568
- line 1656
- line 1877
"actions/workflows/42": (0, {"path": ".github/workflows/perf-numbers.yml"}),
"contents/.github/workflows/perf-numbers.yml": (0, {"content": ""}),
}
.github/workflows/post-merge-maintenance.yml:165
- This step relies on
git checkout -- ${LLMS_PATHS}to discard rejected generator output, but it runs withoutset -e, so a failed checkout would be ignored and could leave the workspace dirty for later steps (including the commit step). If a revert fails, the step should fail immediately rather than continuing with potentially invalid output.
set -uo pipefail
if npm run update:llms-txt && npm run check:llms-txt; then
exit 0
fi
echo "::error::llms.txt generation did not converge; discarding its output."
# shellcheck disable=SC2086 # LLMS_PATHS is an intentional word list.
git checkout -- ${LLMS_PATHS}
exit 1
.github/workflows/post-merge-maintenance.yml:178
- Same issue as the llms.txt step: without
set -e, a failedgit checkout -- ${ISSUE_TEMPLATE_PATHS}revert would be ignored, and later steps could act on output the checker rejected. Make the step fail on any unexpected command failure during the revert path.
set -uo pipefail
if npm run update:issue-template-versions && npm run check:issue-template-versions; then
exit 0
fi
echo "::error::issue-template version generation did not converge; discarding its output."
# shellcheck disable=SC2086 # ISSUE_TEMPLATE_PATHS is an intentional word list.
git checkout -- ${ISSUE_TEMPLATE_PATHS}
exit 1
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The last two gaps round six listed, both predating the table fold.
**The re-dispatch POST was never verified.** The row asserted "cancelled 1" and
"re-dispatching workflow 42" -- both printed BEFORE the request -- so replacing
`gh api -X POST .../dispatches` with `true` passed. A regression there leaves
every dispatcher-stuck push run cancelled and never re-triggered, silently, with
a summary that says it was re-dispatched. The stub now serves that path its own
marker, ordered ahead of the broader `actions/workflows/42` route which was
swallowing it.
**The "already current" verdict exit is now reachable.** It is one of two sites
where `exit "${regenerate_failed}"` could become `exit 0` undetected,
reinstating the always-green bug 764540b claims to fix. It needed a fixture
where the failing generator's revert leaves nothing to commit, which meant
stopping the other generator rewriting its file on every call.
The sibling "No staged changes remain" exit is left deliberately unpinned, with
the reason recorded in the workflow: `git add` makes the staged and unstaged
views agree, so reaching it requires them to disagree, and every fixture that
tries exercises the branch above instead. A case for an unreachable branch is
not coverage, it is decoration -- the same call made earlier for the third
`mapfile` guard.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The new watchdog harness stubs can match unintended routes and hardcode state paths, weakening the “unstubbed calls fail” guarantees that the PR relies on for test confidence.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
scripts/validate-unity-pr-policy.py:876
- watchdog_gh_stub builds a bash
casestatement using raw route needles (e.g. containing?). Incasepatterns,?,*, and[...]are wildcards, so an endpoint that is not actually stubbed can still match and return a payload, weakening the intended "unstubbed call exits 3" guarantee.
cases = "".join(
f" *{needle}*)\n cat <<'PAYLOAD'\n"
f"{payload if isinstance(payload, str) else (json.dumps(payload) if payload is not None else '')}"
f"\nPAYLOAD\n exit {code} ;;\n"
for needle, (code, payload) in routes.items()
)
scripts/validate-unity-pr-policy.py:940
- watchdog_git_stub hardcodes the state directory as
.watchdog-statewhen materializing an existing state branch. That means the harness is no longer driven solely by the workflow’sSTATE_DIRliteral, and a workflow change toSTATE_DIRcould desync the script-under-test from the stubbed clone contents.
writes = "".join(
f"printf '{{\"cancels\": {n}, \"last_cancel\": "
f"'$(( $(date -u +%s) - {state_age_seconds} ))'}}' "
f'> "$target/.watchdog-state/{run_id}.json"; '
for run_id, n in existing_state.items()
)
clone = (
'target="${@: -1}"; /usr/bin/git init -q "$target"; '
'mkdir -p "$target/.watchdog-state"; '
f"{writes}"
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ca478ef. Configure here.
| - name: Commit and push changes | ||
| if: >- | ||
| always() && steps.git-check.outputs.changed == 'true' && | ||
| steps.check-autocommit-app.outputs.has-app == 'true' |
There was a problem hiding this comment.
Cancel can push partial artifacts
High Severity
gen_issue_template, git-check, and Commit and push changes all use bare always(), so they still run after a concurrency or operator cancel. A generator killed mid-write never reaches its revert path, and the push step can commit that partial tree to the default branch. The old split workflows did not keep generating or pushing after cancellation.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ca478ef. Configure here.
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
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.
Summary
Three CI signals that could go green (or red) without meaning it, plus two root-cause investigations that closed lines of inquiry rather than opening them. Every fix is the same class of defect: automation whose verdict did not follow from what it actually observed.
successwhile it could not read runner inventory at allRequire confirmed Unity cleanupfor a cleanup that was never neededacquired: falsecontract, derived fromoutcome#327 — two failures where the leg had one
Return Unity licenseandClassify Unity cleanup evidencecorrectly skip when the lock was never acquired, so the gate read an emptyclassification-completeand failed closed on work that never started. Five of nine legs in run 30642224168 showed the shape.The gate already had the right contract — its
acquiredinput documents thatfalseproves licensed cleanup was not required, and it short-circuits every other check on that value. The workflow simply never supplied it: a skipped step publishes no outputs, so the gate saw empty rather than false.acquire_lockstateoutcomeskipped/''falsesuccesstruefailure''(unchanged)Gating the gate on
acquired == 'true'instead would have skipped exactly the case it must never miss. The bareif: always()survives. Applied to all five licensed lock windows.Verified independently:
acquire_lockcarries noif:and nocontinue-on-errorin any of the five windows, sooutcome ∈ {'', 'skipped'}implies the step never executed, which implies no seat was held. No path produces'false'while the lock is held.#328 — blind and green at the same time
Runner inventory was read from two endpoints the job's token can never reach. The repository-level fallback was worse than no fallback: this repository registers zero repository-level runners, so with the missing permission added the call would succeed and report an empty inventory — indistinguishable from "no runner matches". It is removed, not fixed. Inventory now comes from the build-lock reader App, narrowed to
organization_self_hosted_runners: read.The single "no matching idle runner" test conflated four states, which is why the case #328 actually hit was invisible:
idlebusyoffline::warning::, never cancelledunregisteredself-hosted; otherwise GitHub-hosted and not evaluableStarvation is deliberately not a cancel (the fleet still intends to run that job) and deliberately not a failure (a five-minute cron going permanently red trains people to ignore it).
Two latent defects found while rewriting: label matching was case-sensitive in both directions, and an empty label set vacuously satisfied the superset test — which would have manufactured a dispatcher-stuck verdict and cancelled a live run.
#330 — three bot commits per merge, one of which said nothing
update-llms-txt.ymlandupdate-issue-template-versions.ymlwere the same 180-line workflow twice. They are replaced by one workflow that pushes at most one commit.perf-numbers.ymldeliberately stays separate: it runs behind the organization Unity build lock on a self-hosted runner and can take tens of minutes.The reason the llms.txt commit fired at all:
update:llms-txtrestampedLast Updatedon every run whilecheck:llms-txtdeliberately normalizes the date away, so the generator produced a diff on any day it ran and the checker never objected.1efb7326changed exactly one line — the date — and still re-triggered CI, Release Drafter, and Deploy Documentation.Root causes recorded, no code changed
3 Total / 1 Unique / 0 Errors / 3 Timeouts, and--accept-timeoutstakes it from exit 2 to exit 0. v0.24.2 is also the latest release, so the issue's preferred "bump the version" fix does not exist. Recommended closing on ci: retry the external link probe before failing the build #325's retry.lock-statebranch history. The stale reaper matched the holder to the wrong matrix leg (91204876077editmode, finished 16:01:02Z) instead of the live holder (91204876143playmode, started 16:01:03Z), because it resolves jobs by a time window aroundacquiredAtand the holder's ownacquiredAtpreceded GitHub's reportedstarted_at. A live holder was removed from the lock state, so this is a lock-safety bug, not only a false red. Filed upstream as build-lock#169.Testing
Both workflow fixes are pinned in
scripts/validate-unity-pr-policy.py, the repository's executed-workflow harness (Python, so no JS budget pressure). The watchdog table runs the workflow's own audit script against a stubbedghandgitacross eighteen cases, and reads the step's literalenv:from the workflow rather than restating it — so a case cannot pass by asserting its own stub.Everything was mutation-tested rather than assumed. Two survivors in the first round were real gaps in the tests, not the code, and both were closed.
Two adversarial review rounds
Round one found seven defects, none of which the branch's own suite caught. All are fixed and each ships with a case that fails without it:
mapfile < <(jq …)discarded jq's exit status — one unparseablecreated_atprinted "Queue is clean" over a stuck queue, reintroducing the exact ci: the stuck-job watchdog reports success while it cannot read runner inventory #328 fail-open in the one read not wrapped infail_closed.preserveUnchangedDatecopied a malformed date forward, removing update mode's ability to repair one; it exited 1 while advertising itself as the remediation.uniquesorts, somacos-latestvsubuntu-latestflipped the verdict and suppressed the warning entirely in one ordering.EXCLUDED_BY_FILE[""]is a hard bash error and the code already anticipates a null path.A further fix silences the trap-invoked function under both shellcheck code numbers (0.9 reports SC2317, CI's newer container reports SC2329). The first attempt at explaining that in a comment began with the analyzer's own name, which parses as a malformed directive and would have stopped shellcheck analyzing the entire 500-line script; verified clean against shellcheck 0.11.0 at
-S info.Reviewer status
GitHub Copilot returned "unable to review — quota limit" on all four requests, so it has not reviewed this PR. Cursor Bugbot has not responded to four triggers. The substantive review above came from adversarial passes, with a second pass verifying the fixes themselves.
npm test(406 pass),npm run validate:all,actionlint,yamllint,prettier --check,markdownlint, andcspellare clean. JS budget raised to 17612 with a documented entry.Closes #327
Closes #328
🤖 Generated with Claude Code
Note
Medium Risk
Changes cancel/redispatch logic and Unity license cleanup gating on production workflows; risk is mitigated by extensive policy harness tests and deliberate fail-closed behavior when inventory or state cannot be read.
Overview
Makes several CI automation paths fail closed or report one honest verdict instead of going green when inputs were missing or misleading.
Unity cleanup gate (#327) — In
perf-numbers,release,unity-benchmarks, andunity-tests,Require confirmed Unity cleanupnow passesacquired: falsewhenacquire_locknever ran (outcomeskipped/empty), so legs that aborted before lock acquisition are not failed for cleanup that was never required. A failed acquire still surfaces emptyacquiredand keeps the seat-leak path intact.Stuck-job watchdog (#328) —
stuck-job-watchdog.ymlis rewritten to fail the job when queued runs, org runner inventory, or per-run jobs cannot be read; inventory uses the build-lock reader App (no repo-runner fallback). Classification splits idle / busy / offline / unregistered (case-insensitive labels), adds a Starved summary for offline/unregistered self-hosted demand without cancelling, defers state-branch work until a cancel is needed, and uses EXIT traps plus richer step summaries.Post-merge churn (#330) —
update-llms-txt.ymlandupdate-issue-template-versions.ymlare removed and replaced bypost-merge-maintenance.yml, which regeneratesllms.txt/README.mdand the issue-template dropdown in one commit with isolated per-generator revert + a terminal convergence gate.update-llms-txt.jspreservesLast Updatedwhen content is unchanged (and repairs malformed dates) so date-only diffs no longer auto-commit.Policy tests —
validate-unity-pr-policy.pygains large executable harnesses for the watchdog audit script, maintenance push loop, cleanup-gate expression, and related cases; docs and fetch-refspec tests point at the new workflow names.Reviewed by Cursor Bugbot for commit ca478ef. Bugbot is set up for automated code reviews on this repo. Configure here.