chore(ci): move the Vale upgrade wait loops into a tested script

[theCodeDrift]

What

Replaces the two hand-rolled shell wait loops in .github/workflows/vale-upgrade.yml with one step that runs a new zero-dependency script, .github/scripts/vale-upgrade-wait.cjs, paired with vale-upgrade-wait.test.cjs under pnpm test:scripts.

The script exports both waits with every dependency injectable (runGh, probe, sleep, now, log) and a main() guarded by require.main === module:

• waitForReleaseRun polls gh run list --workflow release-vale.yml --event push --commit $GITHUB_SHA --json status,conclusion (argv array, no shell) every 30 s for up to 25 minutes.
• waitForRegistry spawns node vale-upgrade-detect.cjs --json as a child process up to five times a minute apart, with GITHUB_OUTPUT removed from its env so the probe's update=false never lands in this step's outputs, and stderr passed through so "see the error above" stays true.

It always exits 0 and reports through ::warning:: lines plus two $GITHUB_OUTPUT values: ready (should detect run) and outcome (one word saying why). The detect step now carries if: steps.wait.outputs.ready == 'true'; every later step already gates on steps.detect.outputs.update, which a skipped step leaves empty.

The step no longer has if: github.event_name == 'push'. The script reads the event itself and skips both waits with ready=true on anything else, so that ready is defined on the schedule and dispatch events too; a skipped step has no outputs, and gating detect on it would have silently skipped the daily backstop.

Why

The #369 review asked for it as a follow-up: both loops were the only decision logic in this pipeline not living in a .cjs with a paired test, and that is why the set -e finding in that review slipped past local testing. Thread: #369 (comment) (paragraph starting "Also worth a look").

The explanatory comments from the YAML (the race with the publish, the 3.22.0 measurement, why a persistently erroring probe is left for detect to fail loudly on) moved into the script's header; the workflow keeps a shorter version and points at it.

Behaviour preserved

Case	Warning	ready	outcome	Detect runs
Run registers late, then succeeds; registry ahead	none	true	ahead	yes
Run concludes anything but success	yes (names the conclusions)	false	publish-failed	no
Run never registers inside 25 min	yes	false	publish-unfinished	no
gh errors twice, then the run succeeds	none	true	ahead	yes
gh errors on every poll until the bound	yes (names the last error)	false	publish-unfinished	no
Registry ahead after two stale probes	none	true	ahead	yes
Probe throws on every attempt	yes	true	probe-errored	yes, and fails loudly on the same read
Probe answers "current" every time	yes	false	registry-stale	no
Non-push event (schedule, workflow_dispatch)	none	true	skipped	yes, no waiting

One behaviour changed on purpose: previously detect ran unconditionally after a wait that gave up, and re-asked the registry a sixth time to say "nothing to do". It is now skipped in those cases; the probe already asked the same question with the same code. The probe-errored row keeps the #369 reasoning that a broken registry read is a real failure, not the race.

How verified

• pnpm test:scripts: 466 pass, including the 23 new cases (fake clock advanced by the injected sleep, so the 25-minute bound costs nothing).
• pnpm lint (builds first, then pnpm cli check): clean. Prettier clean on the three touched files. Workflow parsed with the yaml package; step ids and if: conditions read back as expected.
• Ran the script locally against the real repo:
  • GITHUB_EVENT_NAME=schedule → ready=true outcome=skipped, exit 0, no gh call.
  • GITHUB_EVENT_NAME=push GITHUB_SHA=000…0 --timeout-minutes 0.2 --poll-seconds 4 → three real gh run list polls, ::warning::gave up waiting … (no run found), ready=false outcome=publish-unfinished, exit 0.
  • GITHUB_EVENT_NAME=push GITHUB_SHA=4bca529c (the 3.22.0 manifest commit) --attempts 1 → Release Vale succeeded, one real registry probe reporting {"pinned":"3.22.0-20260921180930","upstream":"3.22.0-20260921180930","ahead":false}, ready=false outcome=registry-stale, exit 0.
• Dry-run git merge --no-commit of origin/chore/self-consistent-vendor-upgrades (chore(ci): make vendor upgrade PRs self-consistent so Validate is green by construction #374) onto this branch: clean, no conflicts. This change stops above the "No dependency install" comment that chore(ci): make vendor upgrade PRs self-consistent so Validate is green by construction #374 rewrites, and the detect if: sits below the hunk context chore(ci): make vendor upgrade PRs self-consistent so Validate is green by construction #374 touches.
• actionlint is not installed here, so it was not run.

[theCodeDrift]

@claude /review

[claude]

Claude finished @theCodeDrift's task in 5m 16s —— View job

---

Review mode: incremental — read 0 prior review thread(s) before reviewing.

.prior-review.json contains no reviewThreads, reviews, or comments at all, so this is the first review of this PR — the whole diff was assessed fresh, nothing was treated as already addressed.

• Read .prior-review.json
• Fetch diff via gh pr diff and read changed files
• Review script logic (vale-upgrade-wait.cjs)
• Review test coverage (vale-upgrade-wait.test.cjs)
• Review workflow changes (vale-upgrade.yml)
• Post findings

Summary

This is a clean, well-structured refactor: the two hand-rolled bash wait loops become a single injectable, unit-tested script (waitForReleaseRun / waitForRegistry / main), and the behavior table in the PR description checks out against the code — I traced each row (ahead, publish-failed, publish-unfinished, registry-stale, probe-errored, skipped, and the two gh-errors-then-recovers cases) through vale-upgrade-wait.cjs and the corresponding test, and they match. The workflow's if: steps.wait.outputs.ready == 'true' gate on detect and the removal of if: github.event_name == 'push' (replaced by the script reading GITHUB_EVENT_NAME itself) are both correct and match vale-upgrade-detect.cjs's existing --json/--write contract.

Two things worth a look before/after merge, posted inline:

1. No timeout on either execFileSync call (runGh, line 139, and probeRegistry, line 157). The script's whole design is bounded waits (25 min / 5 attempts), but that bound is only checked between calls — a hang (vs. a fast error) in gh run list or the registry fetch blocks indefinitely, and with no timeout-minutes on the job either, that could tie up the runner far longer than intended. Not exercised by any test (all fakes resolve or throw immediately, never hang).
2. The top-level require.main === module catch handler (lines 398–409) — the part that guarantees "always exit 0" even when main() throws — isn't exercised by any of the 23 new tests; they all call the exported main() directly with valid env. This is the exact kind of untested decision logic the PR is written to eliminate, just one level higher (the outer safety net rather than the two waits themselves).

Everything else checked out:

• parseArgs scaling/validation and defaults match the tests and the header docs.
• waitForReleaseRun's exact-commit/workflow/event filtering preserves the old --commit precision, and gh errors are correctly treated as "not yet registered" rather than aborting the poll (the chore(ci): open the Vale upgrade PR when the manifest lands on main #369 set -e class of bug is fixed here — errors are caught per-attempt, not propagated).
• waitForRegistry correctly distinguishes "answered but stale" from "never answered" (probe-errored vs registry-stale), and correctly skips the pointless sleep after the last attempt (a small, deliberate improvement over the old bash, which always slept after attempt 5 too).
• probeRegistry's child-process approach (stripping GITHUB_OUTPUT, inheriting stderr) correctly prevents the detect script's own update=false from leaking into the wait step's outputs, and preserves "see the error above" in the log.
• The duplicated setOutput helper is consistent with the existing pattern already used identically across vale-detect.cjs, vale-gate.cjs, nightly-pack.cjs, update-badges.cjs, vale-prepare.cjs, vale-upgrade-detect.cjs, nightly-breadcrumb.cjs, and sg-detect.cjs — not a new duplication introduced by this PR.
• No CLAUDE.md/STYLEGUIDE violations found in the changed files.

[theCodeDrift]

Re: @claude[bot] — "Claude finished @theCodeDrift's task in 5m 16s — Review complete…"
#378 (comment)

Finding 2 (the untested require.main catch handler) is fixed in 5410aaf: the handler is now an exported recover() with its output writes guarded, covered by two direct tests and two that spawn the real entry point and assert exit 0 with ready=true. Finding 1 (no timeout on the two execFileSync calls, no timeout-minutes on the step) is a real gap but a design choice about what bound to pick and where; left open for the maintainer rather than acted on here. The branch was also rebased onto main after #374 merged; the rebase was clean and the step order is wait, detect, lockfile, install, vocabulary, consistency check, propose.

— AI Coding Agent