diff --git a/apps/streamdeck/package.json b/apps/streamdeck/package.json index 10da5d802..f3b4c2195 100644 --- a/apps/streamdeck/package.json +++ b/apps/streamdeck/package.json @@ -8,9 +8,9 @@ "build": "node esbuild.js", "watch": "node esbuild.js --watch", "check-types": "tsc --noEmit", - "validate": "streamdeck validate com.cluesmith.codev.sdPlugin", + "validate": "node scripts/validate.mjs", "pack": "streamdeck pack com.cluesmith.codev.sdPlugin --output dist --force", - "package": "node esbuild.js && rm -f com.cluesmith.codev.sdPlugin/bin/plugin.js.map && rm -rf com.cluesmith.codev.sdPlugin/logs && find com.cluesmith.codev.sdPlugin -name .DS_Store -delete && streamdeck validate com.cluesmith.codev.sdPlugin && streamdeck pack com.cluesmith.codev.sdPlugin --output dist --force", + "package": "node esbuild.js && rm -f com.cluesmith.codev.sdPlugin/bin/plugin.js.map && rm -rf com.cluesmith.codev.sdPlugin/logs && find com.cluesmith.codev.sdPlugin -name .DS_Store -delete && node scripts/validate.mjs && streamdeck pack com.cluesmith.codev.sdPlugin --output dist --force", "test": "vitest run" }, "dependencies": { diff --git a/apps/streamdeck/scripts/validate.d.mts b/apps/streamdeck/scripts/validate.d.mts new file mode 100644 index 000000000..30dd686c5 --- /dev/null +++ b/apps/streamdeck/scripts/validate.d.mts @@ -0,0 +1,27 @@ +// Type declarations for validate.mjs so its exported, unit-tested core resolves under tsc +// (NodeNext maps a `./validate.mjs` import to this `./validate.d.mts`) without a suppression. + +/** One run of the validate command: its exit code and combined stdout+stderr. */ +export interface RunResult { + code: number; + output: string; +} + +/** A finished attempt loop: the last run's result plus how many attempts it took. */ +export interface BackoffResult extends RunResult { + attempts: number; +} + +export interface BackoffOptions { + run: (attempt: number) => Promise; + attempts?: number; + baseBackoffMs?: number; + isTransient?: (output: string) => boolean; + sleep?: (ms: number) => Promise; + log?: (message: string) => void; +} + +export const TRANSIENT_SIGNATURES: string[]; +export const DEFAULTS: { attempts: number; baseBackoffMs: number }; +export function isTransientError(output: string): boolean; +export function runWithBackoff(options?: BackoffOptions): Promise; diff --git a/apps/streamdeck/scripts/validate.mjs b/apps/streamdeck/scripts/validate.mjs new file mode 100644 index 000000000..fbb88c5fa --- /dev/null +++ b/apps/streamdeck/scripts/validate.mjs @@ -0,0 +1,124 @@ +// Run `streamdeck validate` with bounded re-attempts + backoff around transient network errors (#1436). +// +// WHY: the Elgato CLI's `manifestUrlsExist` validation rule does a live HEAD request to the +// manifest's top-level `URL` (ours is https://github.com/cluesmith/codev). Its catch block turns +// only `ENOTFOUND` into a graceful validation error; ANY other fetch failure (UND_ERR_SOCKET, +// ECONNRESET, "fetch failed", …) is rethrown and crashes the whole `validate` run. That put the +// network on CI's pass/fail path and flaked unrelated PRs (#1432, #1434) with no code defect. +// +// FIX: run the WHOLE validate command again a few times with exponential backoff, but ONLY when +// the failure output matches a transient-network signature. Real validation errors fail fast on +// the first attempt (no masking, no wasted backoff). `--no-update-check` does NOT help here — it +// only gates the separate schema-update fetch, not this URL-reachability probe. +// +// The core loop is exported and unit-tested (see src/__tests__/validate.test.ts); +// `main()` only wires it to the real child process. Mirrors scripts/render-action-icons.mjs. + +import { execFile } from 'node:child_process'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const PLUGIN_DIR = 'com.cluesmith.codev.sdPlugin'; + +// Case-insensitive signatures of a transient network failure worth another attempt. These are the +// error shapes the CLI rethrows from a failed `fetch(URL, { method: 'HEAD' })`. Each is a specific +// error code/phrase, not a loose word, so a plugin description that merely mentions "network" +// can't trigger a false re-run. +// +// EAI_AGAIN (temporary DNS failure) IS here but ENOTFOUND (permanent "host doesn't exist") is +// NOT — and that asymmetry is deliberate: the CLI already converts ENOTFOUND into a graceful +// "must be resolvable" validation error rather than rethrowing it, so a genuinely bad URL fails +// loudly on attempt 1, while a transient DNS blip gets another attempt. +export const TRANSIENT_SIGNATURES = [ + 'UND_ERR_SOCKET', + 'UND_ERR_CONNECT_TIMEOUT', + 'fetch failed', + 'ECONNRESET', + 'ECONNREFUSED', + 'ETIMEDOUT', + 'EAI_AGAIN', + 'ENETUNREACH', + 'ENETDOWN', + 'socket hang up', +]; + +export const DEFAULTS = { attempts: 3, baseBackoffMs: 1000 }; + +/** Does this combined stdout+stderr look like a transient network failure? */ +export function isTransientError(output) { + const haystack = String(output ?? '').toLowerCase(); + return TRANSIENT_SIGNATURES.some((sig) => haystack.includes(sig.toLowerCase())); +} + +const defaultSleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Bounded attempt loop. `run()` resolves to { code, output }. We run again only when a non-zero + * result is transient AND attempts remain; otherwise we return the last result (fail fast on real + * errors, and still surface the failure after exhausting transient attempts). Injectable `run`, + * `sleep`, and `log` keep this deterministic under test. + */ +export async function runWithBackoff({ + run, + attempts = DEFAULTS.attempts, + baseBackoffMs = DEFAULTS.baseBackoffMs, + isTransient = isTransientError, + sleep = defaultSleep, + log = () => {}, +} = {}) { + let last; + for (let attempt = 1; attempt <= attempts; attempt++) { + last = await run(attempt); + if (last.code === 0) { + return { ...last, attempts: attempt }; + } + const transient = isTransient(last.output); + const hasMore = attempt < attempts; + if (!transient || !hasMore) { + return { ...last, attempts: attempt }; + } + const backoff = baseBackoffMs * 2 ** (attempt - 1); + log( + `streamdeck validate: transient network error on attempt ${attempt}/${attempts}; ` + + `trying again in ${backoff}ms…`, + ); + await sleep(backoff); + } + return { ...last, attempts }; +} + +/** Spawn `streamdeck validate ` once, capturing combined output. */ +function runValidateOnce() { + return new Promise((resolve) => { + const child = execFile( + 'streamdeck', + ['validate', PLUGIN_DIR], + { cwd: join(HERE, '..'), encoding: 'utf-8', maxBuffer: 10 * 1024 * 1024 }, + (error, stdout, stderr) => { + let output = `${stdout ?? ''}${stderr ?? ''}`; + // A spawn failure (e.g. the CLI isn't on PATH) yields empty stdio; surface the error + // message so CI shows *why* rather than an exit 1 with no diagnostic. ENOENT and the like + // aren't in TRANSIENT_SIGNATURES, so appending it won't trigger a spurious re-run. + if (error && output.trim() === '') { + output = `${error.message}\n`; + } + resolve({ code: error ? (error.code ?? 1) : 0, output }); + }, + ); + // The execFile callback already receives spawn errors; this handler just prevents an + // unhandled 'error' event from crashing the process before the callback resolves. + child.on('error', () => {}); + }); +} + +async function main() { + const result = await runWithBackoff({ run: runValidateOnce, log: (m) => console.warn(m) }); + process.stdout.write(result.output); + process.exit(result.code === 0 ? 0 : 1); +} + +// Only run when invoked as a script, not when imported by the test. +if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) { + main(); +} diff --git a/apps/streamdeck/src/__tests__/validate.test.ts b/apps/streamdeck/src/__tests__/validate.test.ts new file mode 100644 index 000000000..5300b2b61 --- /dev/null +++ b/apps/streamdeck/src/__tests__/validate.test.ts @@ -0,0 +1,87 @@ +import { describe, it, expect, vi } from 'vitest'; +import { + runWithBackoff, + isTransientError, + TRANSIENT_SIGNATURES, +} from '../../scripts/validate.mjs'; + +/** + * #1436: `streamdeck validate`'s `manifestUrlsExist` rule does a live HEAD request to the + * manifest's `URL`. Anything other than ENOTFOUND (UND_ERR_SOCKET, "fetch failed", …) is rethrown + * and crashes the whole validate run, flaking unrelated PRs' CI. The fix wraps the invocation in a + * bounded loop that runs again ONLY on transient network failures. These tests pin that contract: + * they fail against a single-shot implementation and pass with the backoff loop. + */ + +const ok = { code: 0, output: 'Validation successful.' }; +const socketFail = { code: 1, output: 'validate failed\nTypeError: fetch failed\n UND_ERR_SOCKET' }; +const realFail = { + code: 1, + output: 'manifest.json\n error: Actions must not be empty\n1 error', +}; + +// No real waiting under test. +const noSleep = () => Promise.resolve(); + +describe('isTransientError', () => { + it('matches the observed socket errors', () => { + expect(isTransientError('TypeError: fetch failed\n UND_ERR_SOCKET')).toBe(true); + expect(isTransientError('read ECONNRESET')).toBe(true); + expect(isTransientError('connect ETIMEDOUT')).toBe(true); + }); + + it('does not match a real validation failure or ENOTFOUND', () => { + expect(isTransientError('error: Actions must not be empty')).toBe(false); + // ENOTFOUND is reported by the CLI as a normal "must be resolvable" error, not a crash. + expect(isTransientError('URL must be resolvable (ENOTFOUND)')).toBe(false); + }); + + it('is case-insensitive across every declared signature', () => { + for (const sig of TRANSIENT_SIGNATURES) { + expect(isTransientError(`prefix ${sig.toUpperCase()} suffix`)).toBe(true); + expect(isTransientError(`prefix ${sig.toLowerCase()} suffix`)).toBe(true); + } + }); +}); + +describe('runWithBackoff', () => { + it('recovers from a transient failure then a success (fails with a single attempt)', async () => { + const run = vi.fn().mockResolvedValueOnce(socketFail).mockResolvedValueOnce(ok); + const result = await runWithBackoff({ run, sleep: noSleep }); + expect(run).toHaveBeenCalledTimes(2); + expect(result.code).toBe(0); + expect(result.attempts).toBe(2); + }); + + it('runs up to the attempt cap on repeated transient failures, then surfaces the failure', async () => { + const run = vi.fn().mockResolvedValue(socketFail); + const result = await runWithBackoff({ run, attempts: 3, sleep: noSleep }); + expect(run).toHaveBeenCalledTimes(3); + expect(result.code).toBe(1); + expect(result.attempts).toBe(3); + }); + + it('fails fast on a real validation error without a second attempt', async () => { + const run = vi.fn().mockResolvedValue(realFail); + const result = await runWithBackoff({ run, attempts: 3, sleep: noSleep }); + expect(run).toHaveBeenCalledTimes(1); + expect(result.code).toBe(1); + }); + + it('applies exponential backoff between transient attempts', async () => { + const run = vi.fn().mockResolvedValue(socketFail); + const sleep = vi.fn().mockResolvedValue(undefined); + await runWithBackoff({ run, attempts: 3, baseBackoffMs: 1000, sleep }); + // Two waits between three attempts: 1000ms then 2000ms. + expect(sleep.mock.calls.map((c) => c[0])).toEqual([1000, 2000]); + }); + + it('returns immediately on first-attempt success', async () => { + const run = vi.fn().mockResolvedValue(ok); + const sleep = vi.fn().mockResolvedValue(undefined); + const result = await runWithBackoff({ run, sleep }); + expect(run).toHaveBeenCalledTimes(1); + expect(sleep).not.toHaveBeenCalled(); + expect(result.code).toBe(0); + }); +}); diff --git a/codev/projects/bugfix-1436-ci-streamdeck-validate-step-fl/status.yaml b/codev/projects/bugfix-1436-ci-streamdeck-validate-step-fl/status.yaml new file mode 100644 index 000000000..e907374eb --- /dev/null +++ b/codev/projects/bugfix-1436-ci-streamdeck-validate-step-fl/status.yaml @@ -0,0 +1,22 @@ +id: bugfix-1436 +title: ci-streamdeck-validate-step-fl +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-08-14T00:40:33.414Z' + approved_at: '2026-08-15T02:53:15.320Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-14T00:27:39.607Z' +updated_at: '2026-08-15T02:53:15.321Z' +pr_ready_for_human: false +pr_history: + - phase: pr + pr_number: 1451 + branch: builder/bugfix-1436 + created_at: '2026-08-14T00:40:49.690Z' diff --git a/codev/state/bugfix-1436_thread.md b/codev/state/bugfix-1436_thread.md new file mode 100644 index 000000000..816b417a0 --- /dev/null +++ b/codev/state/bugfix-1436_thread.md @@ -0,0 +1,78 @@ +# bugfix-1436 — streamdeck validate CI flake on transient network errors + +Issue #1436. BUGFIX protocol, strict mode. + +## Investigate (iter 1) + +### Root cause (traced from source, not assumed) + +The flake is NOT a schema-update fetch as the issue title guessed. The exact failure +path is the Elgato CLI validation rule `manifestUrlsExist` +(`@elgato/cli` dist, rule from `src/validation/plugin/rules/manifest-urls-exist.ts`): + +```js +const { status } = await fetch(url.value, { method: "HEAD" }); // url = manifest top-level "URL" +... +} catch (err) { + if (err.cause?.code === "ENOTFOUND") { + this.addError(..., "must be resolvable", url); // graceful validation error + } else { + throw err; // <-- ANY other fetch error (UND_ERR_SOCKET, ECONNRESET, ETIMEDOUT, + // "fetch failed") is RETHROWN → crashes `streamdeck validate` → CI job fails + } +} +``` + +Our manifest declares `"URL": "https://github.com/cluesmith/codev"` +(`com.cluesmith.codev.sdPlugin/manifest.json:10`). `streamdeck validate` does a live +HEAD request to that URL every run. A transient socket error (not ENOTFOUND) is +rethrown unhandled and fails the whole validate step — exactly the observed +`UND_ERR_SOCKET` / `fetch failed` on PRs #1432, #1434, with no code defect. + +### Offline fallback verified NOT viable +CLI `--help` empirically checked: `streamdeck validate` has `--no-update-check` +("Disables updating schemas") and `--force-update-check`. But those only gate the +SCHEMA update; they do NOT disable the manifest-URL reachability probe. So offline / +`--no-update-check` would NOT remove this flake. Retry is the correct and only clean fix. +Schemas themselves are bundled locally via `@elgato/schemas`. + +### Fix (architect preference: bounded retry with backoff) +Wrap the `streamdeck validate` invocation in a bounded retry (3 attempts, exp backoff), +retrying ONLY on transient/network error signatures; fail fast on real validation errors. +Place a small testable helper `apps/streamdeck/scripts/validate.mjs` (mirrors +the existing `scripts/render-action-icons.mjs` + matching vitest test pattern) and point +the `validate` npm script at it. Both CI workflows call `pnpm validate` +(test.yml:113, sdk-canary.yml:57), so this fixes the flake at both sites. Local `package` +script inlines `streamdeck validate` — swap that one call for the helper too (no script +restructuring). + +Regression test: simulate a transient failure then success against the retry helper; +fails without retry (single-shot throws), passes with it. Real validation errors must +NOT be retried. + +Scope: << 300 LOC. Fits BUGFIX. + +## Fix + PR (iter 1) + +Implemented `apps/streamdeck/scripts/validate.mjs` (retry core exported + +unit-tested), wired `package.json` `validate` + inline `package` call to it. 170 +streamdeck tests pass (11 new). Verified end-to-end: happy path exit 0, real error +fails fast (exit 1), transient retries in unit tests, regression test fails without fix. +Committed 84fd5e785. Porch checks (build, tests) passed. PR #1451 opened (Fixes #1436). + +CMAP: first run failed to auto-detect project from worktree ("Multiple projects found"); +re-ran with `--issue 1436 --project-id bugfix-1436` — both resolve PR #1451 correctly. +Awaiting all three verdicts before notifying architect + `porch done` (pr gate). + +## CMAP verdicts + review fixes + +- gemini=APPROVE (HIGH), codex=APPROVE (HIGH), claude=COMMENT (HIGH, non-blocking nits). +- Addressed claude's substantive points: + - Dropped the over-broad `'network'` substring from TRANSIENT_SIGNATURES (could false-retry + a plugin description mentioning "network"); added specific `ENETUNREACH`/`ENETDOWN`. + - Spawn failure now surfaces `error.message` in output instead of exit-1-with-empty-output. + - Sharpened the comment on the deliberate EAI_AGAIN (retry) vs ENOTFOUND (fail-fast) asymmetry. + - Corrected PR body test count: 8 new tests (170 suite total), not 11. +- Left as-is (correct by design): ENOTFOUND exclusion — the CLI already reports it as a graceful + "must be resolvable" error, so we must not retry a genuinely bad URL. +- 170 tests still pass after the fixes.