From f3b941f1e32cdca1304762b7b2e9d738d596deae Mon Sep 17 00:00:00 2001 From: Via Date: Thu, 16 Jul 2026 07:08:45 +0000 Subject: [PATCH] fix(cli): reduce CRAP score by extracting exit-path helpers; bump contactSheet Windows timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fallow flagged the `uncaughtException` arrow at CRAP 30.0 (right at threshold). Extracting the three exit paths — emitCliErrorEvent, reportPostRenderTerminationEvent, exitAfterPostRenderTermination, exitAfterCliFailure — pulls the two `_flushSync?.()` optional-chain branches out of the arrow body and drops cyclomatic to 3. Same shape already used on compileStage in a sibling PR. Behavior preserved: EPIPE → exit(0), renderSucceeded → report + flush + exit(0), default → commandFailed + track + flush + exit(1) for uncaught; renderSucceeded → report + return, default → commandFailed + track for rejection. Windows contactSheet flake: `createContactSheet > writes PNG output` timed out at 20025ms (default ceiling 20000ms) on Windows. Test does milliseconds of real work — the wall-clock overhead is sharp's native binary fork under runner I/O jitter. PR #2492 already tried lightening the test; the flake persists at the default ceiling. Bumping the per-test timeout to 60s absorbs runner jitter without hiding real slowdowns (a genuine sharp regression would blow through 60s just as readily as 20s). Targeted per-test bump, not a global suite bump. Co-Authored-By: Claude — Via --- packages/cli/src/capture/contactSheet.test.ts | 9 +- packages/cli/src/cli.ts | 113 ++++++++++-------- 2 files changed, 73 insertions(+), 49 deletions(-) diff --git a/packages/cli/src/capture/contactSheet.test.ts b/packages/cli/src/capture/contactSheet.test.ts index 1f55672895..5b2dc74464 100644 --- a/packages/cli/src/capture/contactSheet.test.ts +++ b/packages/cli/src/capture/contactSheet.test.ts @@ -10,6 +10,13 @@ function tempDir(): string { } describe("createContactSheet", () => { + // Sharp on Windows CI runners exercises a native-binary fork per operation + // and the runner's I/O throughput varies with concurrent-job pressure. The + // default 20s ceiling has landed just-over the wall clock repeatedly (see + // PR #2492's earlier lightweighting attempt); the actual work here — two + // 16×9 PNG writes + one contact-sheet composite + one metadata probe — + // is milliseconds of compute, so the extra ceiling only absorbs runner + // I/O jitter, it does not hide a real slowdown. it("writes PNG output when the output path uses a .png extension", async () => { const dir = tempDir(); try { @@ -49,5 +56,5 @@ describe("createContactSheet", () => { } finally { rmSync(dir, { recursive: true, force: true }); } - }); + }, 60_000); }); diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index a1ac2a036c..5829980412 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -295,45 +295,77 @@ process.on("exit", (code) => { _flushSync?.(); }); -process.on("uncaughtException", (error) => { - if ((error as NodeJS.ErrnoException).code === "EPIPE") { - commandFailed = true; - process.exit(0); - } - // Post-artifact-validated shutdown throws (worker teardown, browser - // process cleanup, stray subprocess stream error) must not turn a valid - // render into an exit-1 "no final error message" failure. The render - // command sets `renderSucceeded` right after the producer resolves and - // the artifact is committed — from that point on, every throw here is - // a teardown artifact, not a render failure. Field signals: - // ts=1784169760, ts=1784171150, ts=1784172467 (all win32/x64, CLI - // 0.7.58, ffmpeg=no, 1080x1920, valid MP4s on disk). - if (isRenderSucceeded()) { - // Log to stderr for diagnosis but do NOT flip commandFailed and do NOT - // exit non-zero. The render is valid. - process.stderr.write( - ` [hyperframes] Post-render uncaughtException (render already succeeded): ${error.message}\n`, - ); - _trackCliError?.({ - error_name: error.name, - error_message: error.message, - stack_trace: error.stack, - command, - kind: "uncaught_exception", - }); - _flushSync?.(); - process.exit(0); - } - commandFailed = true; +// Report a CLI error event to telemetry. Extracted from the process-error +// handlers so their bodies stay simple linear branches (see fallow CRAP +// scoring — arrow handlers with inline telemetry calls tip past threshold). +function emitCliErrorEvent(kind: "uncaught_exception" | "unhandled_rejection", error: Error): void { _trackCliError?.({ error_name: error.name, error_message: error.message, stack_trace: error.stack, command, - kind: "uncaught_exception", + kind, }); +} + +// Handle a post-artifact-validated throw: record the diagnostic, emit the +// telemetry event, but do NOT mark the run as failed. Field signals: +// ts=1784169760, ts=1784171150, ts=1784172467 (all win32/x64, CLI 0.7.58, +// ffmpeg=no, 1080x1920, valid MP4s on disk). +// The render is valid — a worker teardown / browser shutdown / stray +// subprocess stream error after `renderSucceeded` was set must not flip +// exit code or telemetry success to failure. +function reportPostRenderTerminationEvent( + label: "uncaughtException" | "unhandledRejection", + kind: "uncaught_exception" | "unhandled_rejection", + error: Error, +): void { + process.stderr.write( + ` [hyperframes] Post-render ${label} (render already succeeded): ${error.message}\n`, + ); + emitCliErrorEvent(kind, error); +} + +// Terminate the process after a post-artifact-validated throw. Wraps +// report + flush + exit(0) so the caller arrow handler doesn't accumulate +// optional-chain branches (fallow CRAP scoring on the arrow tips past +// threshold otherwise). +function exitAfterPostRenderTermination( + label: "uncaughtException" | "unhandledRejection", + kind: "uncaught_exception" | "unhandled_rejection", + error: Error, +): never { + reportPostRenderTerminationEvent(label, kind, error); + _flushSync?.(); + process.exit(0); +} + +// Terminate the process after a genuine CLI failure — mark commandFailed, +// emit telemetry, flush, exit(1). Same rationale as above: keeps the arrow +// handler linear so fallow CRAP stays under threshold. +function exitAfterCliFailure( + kind: "uncaught_exception" | "unhandled_rejection", + error: Error, +): never { + commandFailed = true; + emitCliErrorEvent(kind, error); _flushSync?.(); process.exit(1); +} + +process.on("uncaughtException", (error) => { + if ((error as NodeJS.ErrnoException).code === "EPIPE") { + commandFailed = true; + process.exit(0); + } + // Post-artifact-validated shutdown throws must not turn a valid render + // into an exit-1 "no final error message" failure. The render command + // sets `renderSucceeded` right after the producer resolves and the + // artifact is committed. + if (isRenderSucceeded()) { + exitAfterPostRenderTermination("uncaughtException", "uncaught_exception", error); + } + exitAfterCliFailure("uncaught_exception", error); }); // unhandledRejection does not call process.exit() — Node may continue @@ -346,26 +378,11 @@ process.on("unhandledRejection", (reason) => { // render as failed. `commandFailed` gates the success:true telemetry // field — keep it false when the render actually succeeded. if (isRenderSucceeded()) { - process.stderr.write( - ` [hyperframes] Post-render unhandledRejection (render already succeeded): ${error.message}\n`, - ); - _trackCliError?.({ - error_name: error.name, - error_message: error.message, - stack_trace: error.stack, - command, - kind: "unhandled_rejection", - }); + reportPostRenderTerminationEvent("unhandledRejection", "unhandled_rejection", error); return; } commandFailed = true; - _trackCliError?.({ - error_name: error.name, - error_message: error.message, - stack_trace: error.stack, - command, - kind: "unhandled_rejection", - }); + emitCliErrorEvent("unhandled_rejection", error); }); // Lazy-load help renderer — avoids allocating help data on non-help invocations