ci: make the cleanup gate and the stuck-job watchdog mean what they report by wallstop · Pull Request #331 · Ambiguous-Interactive/DxMessaging

Eli Pinkerton (wallstop) · 2026-07-31T19:41:42Z

Makes three pieces of CI automation report what is actually true. Each change
below fixes a case where a check reported a verdict it had not earned.

Closes #327, #328, #330, #333.

The shape of the bugs

Every defect here is the same shape: a gate that could not fail, or a signal
that could not be trusted.

#	The check said	The truth
#328	watchdog green every 5 min	it could not read runner inventory at all; a run sat queued 10 hours
#327	`Require confirmed Unity cleanup` failed	the leg aborted before taking the lock, so there was nothing to clean up
#333	`Player build failed` after ~20 min	the host had no `cl.exe`; three licence seats and ~2h burned
#330	post-merge maintenance green	a generator that never converged had its output silently reverted

What changed

stuck-job-watchdog.yml — fails closed on every input it depends on: the
queued-run listing, the organization runner inventory, per-run job listings, and
the cancel-cap state branch. Inventory is read with an organization-scoped reader
App token, because the job's own GITHUB_TOKEN can never reach
/orgs/{org}/actions/runner-groups. Starvation is now reported from every branch
that can observe it, including the busy and dispatcher-stuck ones — a busy
sibling does not make a starved sibling less starved. An EXIT trap guarantees a
step summary even on an unanticipated abort.

post-merge-maintenance.yml (replaces update-llms-txt.yml +
update-issue-template-versions.yml) — each generator reverts its own paths when
its checker rejects the output, and the failure flag is reset per regeneration so
it describes the output that actually gets pushed. Conditional steps use
!cancelled() rather than always(): a generator killed mid-write never reaches
its revert, and always() would let the push step commit a half-written file to
master.

scripts/unity/assert-msvc-toolchain.ps1 (new) — fails an IL2CPP leg in
seconds, before the organization build lock is taken, when the host cannot
compile. Distinguishes "no Visual Studio" from "toolset present, cl.exe
missing" — the latter is what #333 was, and Unity discovers the version directory
and puts it on PATH before failing twenty minutes later with a message naming
neither the host nor the cause.

Three defects the gate shipped with, found in review

Worth calling out because they were invisible to the unit tests:

exit 1 could not fail a step. Under this repository's custom shell:
template (pwsh … -Command ". '{0}'") — unlike the built-in shell: pwsh —
no exit $LASTEXITCODE is appended. Measured in the workflow's exact shape:
exit 1 → 0, throw → 1. The gate printed its error and passed.
A failing vswhere was reported as a missing toolchain. That blames a
possibly healthy host and blocks all IL2CPP CI on it.

The argument list was splatted, glob-expanding -products '*'.
Source-level quoting does not prevent this:

& $p -products '*'                ->  -products *
$a=@('-products','*'); & $p @a    ->  -products __pycache__ _site coverage …

Swept the repository for both classes: all 13 custom-template steps are clean of
exit N, and no other PowerShell splat carries a glob metacharacter.

Testing

scripts/validate-unity-pr-policy.py extracts the workflows' run: blocks and
executes them against stubbed gh/git, so the assertions are about
behaviour rather than about text matching. ~41 truth-table rows plus standalone
blocks cover the watchdog's verdict space, the post-merge push loop, and the
terminal gate.

scripts/__tests__/test-msvc-toolchain.ps1 covers every verdict on any platform,
plus four end-to-end guards that drive the script the way the workflow does
against a fake vswhere that records its own argv. The fake is described as data
and rendered to .cmd or bash per host, because Windows is the only platform the
gate actually runs on.

Findings are pinned by mutation, not by passing: throw→exit 1, ignored
vswhere exit code, dropped path-rooted validation, dropped -requiresAny,
re-splatted arguments, lexical version sort, always-true compiler probe, removed
cancel cap, unpersisted cancel state, reader token swapped for GITHUB_TOKEN,
ignored inventory-read failure, and tolerated empty group set are all killed.

Deliberately not done

visible_to_repository was removed from the runner-group walk rather than
corrected. The REST docs type it only as "string" with no format, the endpoint
needs admin:org to probe, and both wrong-guess outcomes land in fail_closed —
so a wrong guess fails all 288 daily runs. The organization has one group, so the
filter selects what the unfiltered call already returns. Tracked by #335 for the
day a second group appears.

Follow-ups filed: #332, #334, #335, and build-lock#169.

Note

Medium Risk
Changes affect org runner visibility, automatic run cancellation, and default-branch auto-commits, plus a new pre-lock gate on self-hosted Windows IL2CPP legs—high operational impact but limited to CI infrastructure, not shipped product code.

Overview
CI automation is tightened so green runs and cleanup gates match what actually happened, and IL2CPP legs fail before consuming Unity licence seats.

Stuck Job Watchdog now fails closed when queued runs, org runner inventory, per-run jobs, or cancel-cap state cannot be read (fixes silent green while a run sat queued for hours). Runner inventory uses an organization-scoped reader App token instead of a repo token or empty org fallback. Queued runs are classified with starvation warnings even when a sibling job is busy or dispatcher-stuck, and an EXIT trap always emits the step summary.

Post-merge maintenance replaces the separate update-llms-txt and update-issue-template-versions workflows with one job that regenerates both artifacts in a single commit. Generators use !cancelled(), revert only their own paths when checks fail, verify reverts, and block push when revert fails; llms.txt no longer restamps Last Updated on byte-identical runs (#330).

Require confirmed Unity cleanup in Unity workflows now passes acquired: false when the lock step was skipped, so a pre-lock abort does not double-fail (#327).

New assert-msvc-toolchain.ps1 runs on standalone / IL2CPP matrix legs before lock acquisition, distinguishing missing VS, missing toolset, and toolset without cl.exe (#333). The workflow step uses throw (not exit 1) under the custom pwsh -Command ". '{0}'" shell, and vswhere args are passed without splatting *.

Also: MSVC assertion tests in CI, doc/runbook updates, JS LOC budget bump, and cspell word list reorder.

^{Reviewed by Cursor Bugbot for commit 100400f. Bugbot is set up for automated code reviews on this repo. Configure here.}

…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>

Eli Pinkerton (wallstop) · 2026-07-31T19:41:54Z

Cursor (@cursor) review

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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>

Eli Pinkerton (wallstop) · 2026-07-31T19:55:51Z

Cursor (@cursor) review

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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>

Eli Pinkerton (wallstop) · 2026-07-31T20:32:04Z

Cursor (@cursor) review

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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>

Eli Pinkerton (wallstop) · 2026-07-31T20:39:07Z

Cursor (@cursor) review

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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>

Eli Pinkerton (wallstop) · 2026-07-31T21:09:36Z

Cursor (@cursor) review

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

cursor

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 13d844a. Configure here.}

Copilot

🟡 Not ready to approve

scripts/validate-unity-pr-policy.py uses a wall-clock timestamp for a “just-created run” case, which can make the suite time-dependent/flaky as it grows.

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

Files not reviewed (2)

scripts/tests/test-msvc-toolchain.ps1.meta: Generated file
scripts/unity/assert-msvc-toolchain.ps1.meta: Generated file

Suppressed comments (1)

scripts/validate-unity-pr-policy.py:1196

FRESH_TIMESTAMP uses the wall clock (datetime.now(...)) to model a “just-created” run. Because the watchdog age gate is 300s, this row can become time-dependent/flaky if the validator ever takes >5 minutes end-to-end (the timestamp becomes older than MIN_QUEUE_AGE_SECONDS by the time the row executes). Prefer a deterministic future timestamp so the row is always below the age gate regardless of runtime.

    FRESH_TIMESTAMP = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ")

Files reviewed: 20/22 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.

…ot when it is built `FRESH_TIMESTAMP` was frozen with `datetime.now()` where the table is constructed, hundreds of lines above the loop that executes it and several standalone blocks earlier. The timestamp ages while the suite runs, and once it passes MIN_QUEUE_AGE_SECONDS this row's verdict silently flips. Today the margin is wide -- ~20s of runtime against a 300s gate -- but the file tripled in size this session and CI hosts are slower. It is now a sentinel resolved at execution. (GitHub Copilot.) Copilot suggested a fixed FUTURE timestamp instead, which would have removed the flake and the test's point together. The row needs an age in [0, MIN_QUEUE_AGE_SECONDS): a NEGATIVE age passes `($now - $created) >= $min` for no `$min` at all, including 0, so dropping the age gate to 0 would stop being detectable. Both mutations are verified still caught after this change -- lowering the literal to 0, and deleting the age filter outright. An age of ~0 at execution is the only value that both stays under the gate and keeps the gate's removal detectable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Eli Pinkerton (wallstop) · 2026-08-01T19:33:13Z

Round 16 — Bugbot clean; Copilot's flake is real, its fix would have broken the test

Bugbot on 13d844aa: ✅ no new issues.

Copilot — FRESH_TIMESTAMP is wall-clock (validate-unity-pr-policy.py:1196).
The concern is right and now fixed in 59c1bed3. The timestamp was frozen where
the table is constructed, hundreds of lines above the loop that executes it
and several standalone blocks earlier, so it ages while the suite runs. Margin
today is wide — ~20s of runtime against the 300s gate — but this file tripled in
size during this PR and CI hosts are slower. It is now a sentinel resolved at
execution time.

The suggested fix, though, would have removed the flake and the test's point
together. A fixed future timestamp makes the age negative, and a negative age
fails ($now - $created) >= $min for every $min — including 0. So the row
would stop detecting the age gate being dropped to 0.

Measured before changing anything, because the comment on that row claims exactly
this property:

variant	`MIN_QUEUE_AGE_SECONDS` → 0	age filter deleted
before (frozen `now`)	caught	caught
after (resolved at execution)	caught	caught
future timestamp (as suggested)	would not be caught	—

The row needs an age in [0, MIN_QUEUE_AGE_SECONDS). An age of ~0 at execution
is the only value that both stays under the gate and keeps the gate's removal
detectable — so the fix was to correct when it is evaluated, not what it is.

That is every anchored finding from both reviewers addressed: fixed, or refuted
with a mutation showing the code is already correct. Full list re-enumerated on
this head rather than read from the summary line.

Eli Pinkerton (wallstop) · 2026-08-01T19:33:14Z

bugbot run

Copilot

🟡 Not ready to approve

post-merge-maintenance.yml generator steps omit set -e, so a failed revert (git checkout) could be ignored and allow checker-rejected output to be committed/pushed later in the job.

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

Files not reviewed (2)

scripts/tests/test-msvc-toolchain.ps1.meta: Generated file
scripts/unity/assert-msvc-toolchain.ps1.meta: Generated file

Suppressed comments (2)

.github/workflows/post-merge-maintenance.yml:186

Same issue as the llms generator step: this step reverts its own paths on failure so subsequent steps can still run safely. Without set -e, a failing git checkout could be ignored, leaving checker-rejected output to be committed/pushed later in the job (the push step is intentionally allowed to run after failures). Prefer set -euo pipefail here.

          set -uo pipefail

.github/workflows/post-merge-maintenance.yml:167

The step relies on git checkout reverting rejected output before later steps (including the commit/push step) run after a failure. Without set -e, a failed git checkout (e.g., pathspec mismatch) would be ignored and could leave rejected output in the worktree, which the later commit/push step may then publish. Use set -euo pipefail here; set -e is safe because failures inside the if npm ...; then condition do not trigger errexit.

This issue also appears on line 186 of the same file.

          set -uo pipefail

Files reviewed: 20/22 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.

cursor

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 59c1bed. Configure here.}

…use the push if it failed The commit step is deliberately allowed to run after a FAILED generator. The only thing making that safe is the invariant its own comment states: "the tree can only ever carry output a checker accepted, never a half-written file." Each generator reverts its own paths to hold that up -- but nothing checked the revert worked. A `git checkout` that fails (a pathspec matching nothing, a file the generator deleted) breaks the invariant silently and hands rejected output to the push step, which publishes it to the default branch. Each generator now verifies its own revert with `git diff --quiet` and reports `revert-failed=true`; the push step refuses when either says so. Copilot proposed `set -euo pipefail` on the generator steps instead. That does not address the harm, which is worth recording because it is the obvious-looking fix. Measured, both variants: set -uo pipefail -> step exit=1, file still holds the rejected content set -euo pipefail -> step exit=1, file still holds the rejected content Stopping the step earlier does not un-modify a file. Only refusing the push does. The explicit `exit 1` already made the step fail either way, so `set -e` changes neither the step's verdict nor the worktree. Mutations killed: either tree-trust clause dropped from the push condition, either generator no longer verifying its revert, and a generator no longer reporting `revert-failed`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Eli Pinkerton (wallstop) · 2026-08-01T20:19:01Z

Round 17 — real hole, fixed in `74fcefbe`; but `set -e` was not the fix

Bugbot on 59c1bed3: ✅ no new issues.

Copilot — generator steps omit set -e, so a failed revert could let rejected
output be pushed. The risk is real and now closed. The commit step is
deliberately allowed to run after a failed generator, and the only thing making
that safe is the invariant the file itself states: "the tree can only ever
carry output a checker accepted." Each generator reverts its own paths to hold
that up — but nothing ever checked the revert worked. A git checkout that
fails (pathspec matching nothing, a file the generator deleted) breaks the
invariant silently.

Each generator now verifies its revert with git diff --quiet and reports
revert-failed=true; the push step refuses when either says so.

set -euo pipefail would not have fixed it, which is worth recording because
it is the obvious-looking fix. Measured both ways:

set -uo pipefail   -> step exit=1, file still holds the rejected content
set -euo pipefail  -> step exit=1, file still holds the rejected content

Stopping the step earlier does not un-modify a file, and the explicit exit 1
already made the step fail either way — so set -e changes neither the step's
verdict nor the worktree. Only refusing the push changes the outcome.

Mutations killed: either tree-trust clause dropped from the push condition,
either generator no longer verifying its revert, and a generator no longer
reporting revert-failed.

That is the second time this round-trip has been productive in the same shape:
the finding is sound and the suggested remedy would have missed (or, with
the future-dated timestamp last round, actively removed) the thing it was meant
to protect. Both are fixed on the substance rather than the letter.

Eli Pinkerton (wallstop) · 2026-08-01T20:19:02Z

bugbot run

cursor

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 74fcefb. Configure here.}

Copilot

🟡 Not ready to approve

The updated comment block in unity-benchmarks.yml currently describes IL2CPP/MSVC gating that does not exist in that workflow’s matrix and should be corrected to avoid misleading operators.

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

Files not reviewed (2)

scripts/tests/test-msvc-toolchain.ps1.meta: Generated file
scripts/unity/assert-msvc-toolchain.ps1.meta: Generated file

Suppressed comments (1)

.github/workflows/unity-benchmarks.yml:210

This comment block describes an IL2CPP/standalone MSVC toolchain prerequisite gate “running before the lock”, but this workflow’s matrix only includes editmode and playmode (no standalone), and there is no MSVC assertion step here. As written it implies coverage that does not exist and can mislead operators during an outage like #333.

      # IL2CPP compiles generated C++ with `cl.exe`; Mono legs never touch it.
      # `assert-unity-host-prereqs` covers the VC++ RUNTIME (so the Editor
      # launches) but not the BUILD TOOLS (so IL2CPP can compile), and that gap
      # cost three licence seats and ~2h on 2026-07-31 (#333): three standalone
      # legs each took the organization lock, consumed a seat, built for twenty

Files reviewed: 20/22 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.

Removing the dead step left its explanatory comment behind, so the file still described an IL2CPP toolchain assertion it no longer has -- and never could, since its matrix is editmode and playmode only. An incomplete deletion that reads as documentation is worse than the dead step was: the step at least did nothing, while the comment actively tells an operator something untrue. (GitHub Copilot.) The comment now says why there is no gate here, and where the real ones live. Swept for the same class -- references to things this PR removed. The only mention of the two deleted workflows is the deliberate "it replaces" line in their successor's header; `ci.yml`'s MSVC mentions are it RUNNING the test suite, not declaring a gate; and `visible_to_repository` survives only in the comment explaining why it is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Eli Pinkerton (wallstop) · 2026-08-01T20:47:37Z

Round 18 — Bugbot clean; Copilot's stale-comment catch was right

Fixed in 100400ff. Removing the dead MSVC step from unity-benchmarks.yml left its explanatory comment behind, so the file still described an IL2CPP toolchain assertion it no longer has — and never could, since that matrix is editmode and playmode only.

An incomplete deletion that reads as documentation is worse than the dead step was: the step at least did nothing, while the comment actively tells an operator something untrue. The comment now says why there's no gate there and where the real ones live.

Swept for the same class — references to anything this PR removed:

reference	status
the two deleted workflows	only the deliberate "it replaces" line in their successor's header
MSVC in `ci.yml`	it runs the test suite; not a gate declaration
`visible_to_repository`	survives only in the comment explaining why it's gone

Bugbot on 74fcefbe: ✅ no new issues.

Eli Pinkerton (wallstop) · 2026-08-01T20:47:38Z

bugbot run

Copilot

🟡 Human review recommended

It makes high-impact changes to CI automation behavior (org runner inventory access, automatic cancellation/redispatch, and default-branch auto-commits) that warrant final human validation in real CI runs.

Review details

Files not reviewed (2)

scripts/tests/test-msvc-toolchain.ps1.meta: Generated file
scripts/unity/assert-msvc-toolchain.ps1.meta: Generated file

Files reviewed: 20/22 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.

cursor

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 100400f. Configure here.}

Follow-up to #331, correcting a claim I made in it. ## What was wrong #331 removed the `visible_to_repository` filter from the watchdog's runner-group walk, on the stated grounds that *"the organization registers a single group today, so the filter selects exactly what the unfiltered call already returns."* The first production run after that merge disproved it: ``` Organization runner groups: Default, ambiguous-interactive-organization-builds, Default ``` Two distinct groups, one repeated by `--paginate`. The premise was simply wrong, and the risk I filed as hypothetical in #335 was live: runners in a group this repository may not be allowed to use were being counted as available capacity. **Why that matters.** Inflated capacity is what turns a legitimately queued run into a *wrongful cancel* — the audit sees an idle runner carrying the required labels, concludes the dispatcher is stuck, and cancels a run that was correctly waiting for a runner it can actually use. No wrongful cancel has occurred: every post-merge run so far reported `No dispatcher-stuck run found`. The exposure was real but unrealised. ## The fix Visibility is resolved through **documented shapes** rather than the ambiguous query parameter that started all this: * each group's own `visibility` field — `all` needs no further call; * `selected` groups are resolved against the group's repositories endpoint and compared with `github.repository_id`. Numeric ids on both sides, nothing to guess. This is what #335 asked for, minus the `admin:org` probe, because the group object already carries `visibility`. Groups that exclude us are named in the step summary, and a walk ending with **no** visible group fails closed — capacity we cannot read is not capacity that is zero. Also dedupes the group list by id. The runner dedupe already made the repeat harmless for the counts, so this is not a correctness fix; it removes a redundant API call per duplicate and stops the summary printing `Default, …, Default`, which is exactly the kind of line that makes an operator distrust the report. ## A harness bug that made the first version of these tests worthless `watchdog_runners` numbered runner ids from 1 on **every** call, and the inventory is deduplicated by runner id — so two groups' runners collided and the second group's silently vanished. My multi-group fixture therefore proved nothing: the case asserting *"an invisible group's runner is excluded"* passed identically whether or not the exclusion worked. Ids are now distinct per group, and only then did the mutations start failing as they should. ## Verification Four mutations, each against a verified-green baseline: | mutation | result | |---|---| | visibility filtering removed (the pre-fix behaviour) | killed | | an invisible group treated as visible | killed | | group dedupe removed | killed | | an empty visible set tolerated instead of failing closed | killed | New rows: an idle runner in a group we cannot use is not capacity · an idle runner in a group we can use is capacity · no visible group fails closed · a group repeated by pagination is walked once. Refs #335.  --- > [!NOTE] > **High Risk** > Changes how the watchdog decides dispatcher-stuck vs legitimately queued runs; over-counting capacity was the path to cancelling runs that were correctly waiting, and this PR is the corrective logic on that automation. > > **Overview** > The stuck-job watchdog no longer treats **every** organization runner group as capacity for this repo. It filters groups by documented `visibility`, `allows_public_repositories`, and (for `selected` groups) whether `github.repository_id` appears in the group’s repository list—so idle runners in groups this public repo cannot use are not counted and cannot trigger a wrongful cancel. > > Runner groups are **deduped by id** after pagination (fewer redundant API calls and cleaner summaries). Excluded groups are named in the step summary; if **no** group is usable, the audit **fails closed** instead of reporting zero capacity. Inventory logging now says capacity is from groups visible to `${REPO}`. > > `REPO_ID` is passed into the audit step. The unity PR policy harness adds `REPO_ID`, fixes `watchdog_runners` so multi-group fixtures use distinct runner ids, and adds regression cases for invisible groups, public-repo refusal, `private` visibility, fail-closed when nothing is visible, and pagination duplicates. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 05c2967. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>  --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Eli Pinkerton (wallstop) and others added 2 commits July 31, 2026 19:35

Copilot AI review requested due to automatic review settings July 31, 2026 19:41

Copilot started reviewing on behalf of Eli Pinkerton (wallstop) July 31, 2026 19:42 View session

Copilot AI reviewed Jul 31, 2026

Copilot AI review requested due to automatic review settings July 31, 2026 19:55

Copilot started reviewing on behalf of Eli Pinkerton (wallstop) July 31, 2026 19:56 View session

Copilot AI reviewed Jul 31, 2026

Copilot AI review requested due to automatic review settings July 31, 2026 20:32

Copilot started reviewing on behalf of Eli Pinkerton (wallstop) July 31, 2026 20:32 View session

Copilot AI reviewed Jul 31, 2026

Copilot AI review requested due to automatic review settings July 31, 2026 20:38

Copilot started reviewing on behalf of Eli Pinkerton (wallstop) July 31, 2026 20:39 View session

Copilot AI reviewed Jul 31, 2026

Eli Pinkerton (wallstop) mentioned this pull request Jul 31, 2026

ci: a Unity run superseded mid-flight blocks the next run behind legs that only abort #332

Open

Copilot AI review requested due to automatic review settings July 31, 2026 21:08

Copilot started reviewing on behalf of Eli Pinkerton (wallstop) July 31, 2026 21:09 View session

Copilot AI reviewed Jul 31, 2026

cursor Bot reviewed Jul 31, 2026

View reviewed changes

Comment thread .github/workflows/stuck-job-watchdog.yml

Eli Pinkerton (wallstop) enabled auto-merge (squash) July 31, 2026 21:21

Eli Pinkerton (wallstop) mentioned this pull request Jul 31, 2026

ci: IL2CPP standalone legs fail because cl.exe is missing on the Windows runner #333

Closed

Copilot started reviewing on behalf of Eli Pinkerton (wallstop) August 1, 2026 18:38 View session

cursor Bot reviewed Aug 1, 2026

View reviewed changes

Copilot AI reviewed Aug 1, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 1, 2026 19:32

Copilot started reviewing on behalf of Eli Pinkerton (wallstop) August 1, 2026 19:33 View session

Copilot AI reviewed Aug 1, 2026

View reviewed changes

cursor Bot reviewed Aug 1, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 1, 2026 20:18

Copilot started reviewing on behalf of Eli Pinkerton (wallstop) August 1, 2026 20:19 View session

cursor Bot reviewed Aug 1, 2026

View reviewed changes

Copilot AI reviewed Aug 1, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 1, 2026 20:47

Copilot started reviewing on behalf of Eli Pinkerton (wallstop) August 1, 2026 20:47 View session

Copilot AI reviewed Aug 1, 2026

View reviewed changes

cursor Bot reviewed Aug 1, 2026

View reviewed changes

Eli Pinkerton (wallstop) merged commit 66feaae into master Aug 1, 2026
39 checks passed

Eli Pinkerton (wallstop) deleted the dev/wallstop/ci-signal-integrity branch August 1, 2026 21:29

This was referenced Aug 1, 2026

ci: the stuck-job watchdog reports success while it cannot read runner inventory #328

Closed

[Feature]: Collapse Multi Merges Onto Main #330

Open

ci: count only runner groups this repository can actually use #338

Merged

Uh oh!

Conversation

Eli Pinkerton (wallstop) commented Jul 31, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The shape of the bugs

What changed

Three defects the gate shipped with, found in review

Testing

Deliberately not done

Uh oh!

Eli Pinkerton (wallstop) commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Eli Pinkerton (wallstop) commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Eli Pinkerton (wallstop) commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Eli Pinkerton (wallstop) commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Eli Pinkerton (wallstop) commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Files not reviewed (2)

Suppressed comments (1)

Uh oh!

Eli Pinkerton (wallstop) commented Aug 1, 2026

Round 16 — Bugbot clean; Copilot's flake is real, its fix would have broken the test

Uh oh!

Eli Pinkerton (wallstop) commented Aug 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Files not reviewed (2)

Suppressed comments (2)

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Eli Pinkerton (wallstop) commented Aug 1, 2026

Round 17 — real hole, fixed in 74fcefbe; but set -e was not the fix

Uh oh!

Eli Pinkerton (wallstop) commented Aug 1, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Files not reviewed (2)

Suppressed comments (1)

Uh oh!

Eli Pinkerton (wallstop) commented Aug 1, 2026

Round 18 — Bugbot clean; Copilot's stale-comment catch was right

Uh oh!

Eli Pinkerton (wallstop) commented Aug 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Eli Pinkerton (wallstop) commented Jul 31, 2026 •

edited by cursor Bot

Loading

Round 17 — real hole, fixed in `74fcefbe`; but `set -e` was not the fix