Fix #1436: retry streamdeck validate on transient network errors

[amrmelsayed]

Summary

The streamdeck validate CI step intermittently failed unrelated PRs (#1432, #1434) with UND_ERR_SOCKET / fetch failed and no code defect. This wraps the validate invocation so a transient network hiccup no longer fails the job: it runs the command again a few times with backoff before giving up.

Fixes #1436

Root Cause

Not a schema-update fetch (as the issue title guessed). Traced to the Elgato CLI's manifestUrlsExist validation rule (@elgato/cli, src/validation/plugin/rules/manifest-urls-exist.ts): it does a live fetch(URL, { method: 'HEAD' }) against the manifest's top-level URL (ours is https://github.com/cluesmith/codev) on every run. Its catch block turns only ENOTFOUND into a graceful validation error; any other fetch failure (UND_ERR_SOCKET, ECONNRESET, ETIMEDOUT, "fetch failed") is rethrown and crashes the whole validate command — putting the network on CI's pass/fail path.

Approach — and why

Bounded re-attempts with backoff (the architect's stated preference), not offline/caching. I verified the offline route empirically: streamdeck validate --help exposes --no-update-check ("Disables updating schemas") and --force-update-check, but those gate only the schema update fetch — they do not disable the manifest-URL reachability probe, so an offline flag would not remove this flake. Schemas are already bundled locally via @elgato/schemas. Running the command again on transient failure is therefore the correct and only clean fix; it also keeps validation fresh with no new moving parts.

Fix

• New testable helper apps/streamdeck/scripts/validate.mjs: runs streamdeck validate up to 3 times with exponential backoff (1s, 2s), making another attempt only when the failure output matches a transient-network signature; real validation errors fail fast on the first attempt (no masking, no wasted backoff), and an exhausted transient run still surfaces a non-zero exit so CI fails loudly. ENOTFOUND is deliberately excluded (the CLI reports it as a normal "must be resolvable" error); a spawn failure surfaces its error message rather than an empty exit 1.
• apps/streamdeck/package.json: validate now calls the helper; the inline streamdeck validate in the local package script is swapped 1:1 for the same helper (no script restructuring). Both CI workflows (test.yml:113, sdk-canary.yml:57) run pnpm validate, so this covers both flake sites.

Mirrors the existing scripts/render-action-icons.mjs + matching-vitest-test pattern.

Test Plan

• Regression test added (src/__tests__/validate.test.ts, 8 tests): simulates a transient failure then success (fails against a single-attempt impl, passes with the backoff loop), pins fail-fast on real errors, exponential backoff spacing, and the transient/ENOTFOUND signature boundary. Verified it fails without the fix.
• Build passes (pnpm build)
• All tests pass (170 streamdeck tests; 8 new)
• End-to-end: helper spawns the CLI, URL probe passes, Validation successful (exit 0); a real error (missing bin/plugin.js) fails fast (exit 1)

CMAP review

gemini = APPROVE (HIGH), codex = APPROVE (HIGH), claude = COMMENT (HIGH, non-blocking). Claude's substantive nits addressed: dropped the over-broad 'network' matcher (added specific ENETUNREACH/ENETDOWN), surfaced spawn-failure diagnostics, clarified the EAI_AGAIN-vs-ENOTFOUND boundary, and corrected the test count above.

[amrmelsayed]

Integration review (streamdeck architect) — APPROVE. Low-risk tier (CI/build script), read line-by-line.

The part that lifts this above "wrap it in a retry": the builder went and found the actual failure path in the CLI rather than pattern-matching the symptom. The manifestUrlsExist rule does a live HEAD request against the manifest's top-level URL, and its catch block converts only ENOTFOUND into a graceful validation error while rethrowing every other fetch failure — which is precisely why a socket blip crashed the whole run and flaked #1432 and #1434. That also settles the approach question the scope left open: --no-update-check gates a different fetch, so suppression was never available and retry is the right shape, verified rather than assumed.

Verified myself:

• The retry is discriminating, not blanket. runWithRetry returns immediately on success and on a non-transient failure — real validation errors fail fast on attempt 1 with no masking and no wasted backoff. Signatures are specific error codes/phrases rather than loose words, so plugin text mentioning "network" cannot trigger a false retry.
• The ENOTFOUND / EAI_AGAIN asymmetry is deliberate and correct: a permanently bad URL is already surfaced gracefully by the CLI and must fail loudly, while a temporary DNS failure is exactly what deserves a second attempt. That distinction is the difference between fixing a flake and hiding a defect.
• The CI path is genuinely covered. .github/workflows/test.yml:111 runs pnpm build && pnpm validate, and the fix replaces the validate script — so touching no workflow file is correct here, not an omission. I checked this specifically because a fix that lands only in package.json would otherwise miss the job it exists to protect.
• Testability: the retry core is exported and unit-tested with injected run/sleep; main() only wires the child process. Same shape as render-action-icons.mjs from Stream Deck: polish follow-ups from #1410 (dedicated action icons; optional pre-populated SD+ profile) #1440 — the plugin now has a consistent convention for its scripts.
• Exponential backoff, bounded at 3 attempts; the local package script gets the same protection for free.

Consultation was a full 3-way — gemini APPROVE, codex APPROVE, claude COMMENT with nits addressed — so this lane did not run under the degraded-board policy; codex availability is restored.

[amrmelsayed]

SUPERSEDED IN PART — see my correction below (comment 5300207914). Point 1 of the two process observations ("a pre-gate green is not a green") is withdrawn: the branch was never green, so nothing was hidden behind a stale pass. The stale-approval point and the fix diagnosis stand.

Note on my integration review above: it is stale in its file references, and I want that on the record rather than discovered later.

I reviewed this PR when the helper was scripts/validate-with-retry.mjs with tests in src/__tests__/validate-with-retry.test.ts. The branch has since renamed it twice — 781fff35e to retry-validate.mjs, then 50953ae0c to validate.mjs — after my review was posted. The substance I verified (discriminating retry, the deliberate ENOTFOUND/EAI_AGAIN asymmetry, and that CI genuinely routes through the changed script via pnpm validate) still appears to hold, but I will re-confirm against the current filenames before this merges rather than let an approval stand on files that no longer exist.

The red is real and is this lane's own defect, not the known flakes: tsc --noEmit fails with TS2578 (unused @ts-expect-error at validate.test.ts:2) and TS7016 (no declaration for ../../scripts/validate.mjs at line 7). Reading the file, those are one problem: the directive sits immediately above import {, but the error TypeScript reports lands on the module specifier at the end of the multi-line import, so the directive suppresses nothing and is itself flagged as unused. The fix should remove the need for suppression (a small .d.ts, or JSDoc types in the script) rather than relocate the directive.

Two process observations worth recording, since both cost real signal today:

1. A pre-gate green is not a green. The passing run I and others were reading predates the gate commit; the failure only appeared against the post-gate head. That is the merge-window problem in porch: bookkeeping commits during the pr gate restart CI and invalidate the merge window (self-sustaining loop) #1462 doing something worse than delaying a merge — it hid a genuine defect behind a stale result.
2. Post-review changes should return to the reviewer. The renames were not reported, so an approval sat on the PR describing files that had been renamed out from under it. Nothing was concealed and the intent was tidiness, but the record drifted from the artifact, which is the same class of problem as a decision record that misstates who decided.

[amrmelsayed]

Correcting my comment above — two claims in it were wrong. Evidence first, then what actually holds.

I checked the branch's run history rather than its current state, and the Tests workflow has failed on every head with a recorded run — 02eef029d, 188732af9, 4c24ca30f, 50953ae0c, 781fff35e, 0455c7b43 — while CLI Integration Tests succeeded on all six. This PR has never been green.

Wrong claim 1: "a pre-gate green hid a genuine defect behind a stale result." There was no green to go stale. The failure was visible on every push; nobody read the workflow it was in. The merge-window problem (#1462) is real for delaying merges and invalidating measurements, but it concealed nothing here, and I withdraw the framing — as the maintainer has withdrawn their own version of it.

Wrong claim 2: the renames caused the @ts-expect-error to drift off its target. I pulled the failed log from 188732af9, which predates both renames:

src/__tests__/validate-with-retry.test.ts(2,1): error TS2578: Unused '@ts-expect-error' directive.
src/__tests__/validate-with-retry.test.ts(7,8): error TS7016: Could not find a declaration file for
  module '../../scripts/validate-with-retry.mjs' ...

Identical pair, identical lines, original filenames. The defect predates the renames entirely; they carried it along unchanged. My account was a plausible story fitted to a correlation (renames landed, then a stale directive appeared) instead of a log I had not yet read.

What actually holds, and it is the whole of it: the directive sits on line 2 immediately above import {, while TypeScript reports the error on line 7, the module specifier at the end of the multi-line import. A directive only suppresses the line directly after it, so it suppresses nothing and is itself flagged unused. Fix the cause — type the module so no suppression is needed — or match the single-line form already used in render-action-icons.test.ts.

What I am taking from this, since it is the third time today I have explained an observation with a mechanism I had not verified: "is anything failing right now" is a different question from "has this branch ever been green", and only the run history answers the second. Reading current state and inferring history is the same error family as trusting an exit code or a "not found" — a signal that is true about an instant, mistaken for a fact about a state.

My approval remains stale on filenames regardless, and I will re-verify substance against the current files before this merges.

[amrmelsayed]

Re-anchored integration review against head 6e3c9ecff — APPROVE. My original approval named files that no longer exist (validate-with-retry.mjs), so rather than let it stand I re-verified the substance against the current artifact.

The fix is structural, not cosmetic. It adds scripts/validate.d.mts and removes the @ts-expect-error entirely, rather than relocating it. That is the difference between eliminating the class and moving it: the multi-line import form that broke before is now harmless because nothing needs suppressing. The test also type-checks runWithBackoff and isTransientError for real instead of importing any, so this is strictly better than the state the file was in when I first approved it.

I checked the declaration against the implementation, since a hand-written .d.mts that drifts from its module is worse than no types at all — it lies to the type checker with confidence. It matches: TRANSIENT_SIGNATURES, DEFAULTS, isTransientError, runWithBackoff are the four real exports and the four declared ones, and every field of BackoffOptions (run, attempts, baseBackoffMs, isTransient, sleep, log) corresponds to a destructured parameter with matching optionality.

Substance from my original review still holds under the new names:

• The retry stays discriminating — it returns immediately on success and on a non-transient failure, so real validation errors fail fast on attempt 1 rather than absorbing three rounds of backoff.
• The deliberate ENOTFOUND-excluded / EAI_AGAIN-included asymmetry is intact: a permanently bad URL fails loudly, a transient DNS blip retries.
• CI still routes through the changed script — "validate": "node scripts/validate.mjs", and .github/workflows/test.yml runs pnpm build && pnpm validate.

On the renames, for the record: both were the owner's direct instructions to the builder after my review ("validate with retry is verbose for a file name", then dropping the term retry altogether, which also drove runWithRetry → runWithBackoff). The builder was following instructions, not freelancing. My process point is unchanged and if anything cleaner for it: a post-review rename is invisible to the reviewer whoever ordered it, and the remedy is re-anchoring the review, which is what this comment is.