From c5d6dcfc84e8ee731832f6661244ea717ebb0df2 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 26 Jul 2026 22:02:36 -0400 Subject: [PATCH] fix(scripts): run changed checks locally when Blacksmith never ran them (#114225) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(scripts): run changed checks locally when Blacksmith never ran them AGENTS.md already says trusted-source work falls back to local execution when the remote backend is unavailable, but the tooling did not implement it: a lease, broker, DNS, or network failure surfaced as a plain exit 1, so the lanes were reported red without ever having been evaluated. That is worse than slow — a run that never happened looked the same as a run that failed. Tee the wrapper output and use its run summary as the discriminator. The summary only appears once the command reached the box, so a failure carrying `command-exit` is a real verdict and propagates unchanged; anything else never produced one and re-runs locally, with a loud note so the proof summary records which machine produced it. Deliberately a positive test for `command-exit` rather than a blocklist of infrastructure errors. Guessing wrong toward "infrastructure" only re-runs the checks locally; guessing wrong toward "real failure" would block on an outage. It must never widen to "fall back on any non-zero exit" — prompt snapshots are Linux-only truth and would pass locally on macOS, turning a red gate green. The sparse-checkout path keeps no fallback: it exists precisely because the checkout cannot resolve the diff refs, so there is nothing local to run. * fix(scripts): require positive pre-dispatch evidence before falling back A missing run summary does not prove the remote never started: a wrapper that crashes or loses its output transport after dispatch looks identical. Reading that absence as "never ran" would rerun locally and could turn an unknown or failing Linux-only lane green, which is the exact masking this guard exists to prevent. Require a positive pre-dispatch signature instead, and keep the command-exit veto. Also reapply backpressure on the tee: inherited stdio got it from the OS, piping does not, so a verbose delegated run could buffer its whole output here. --- scripts/check-changed.d.mts | 1 + scripts/check-changed.mjs | 79 ++++++++++++++++++++++++++++-- test/scripts/changed-lanes.test.ts | 50 +++++++++++++++++++ 3 files changed, 127 insertions(+), 3 deletions(-) diff --git a/scripts/check-changed.d.mts b/scripts/check-changed.d.mts index 38d1a06826ac..7282afeafc9b 100644 --- a/scripts/check-changed.d.mts +++ b/scripts/check-changed.d.mts @@ -38,6 +38,7 @@ export function shouldDelegateChangedCheckToCrabbox( options?: { cwd?: string; result?: ChangedLaneResult; diffRefsReady?: boolean }, ): boolean; export function buildChangedCheckCrabboxArgs(argv?: string[], options?: { cwd?: string }): string[]; +export function delegationFailedBeforeRunning(output: string): boolean; export function shouldRunNpmLockGuard(paths: string[]): boolean; export function shouldRunPromptSnapshotCheck(paths: string[]): boolean; export function shouldRunPromptSnapshotOwnerTest(paths: string[]): boolean; diff --git a/scripts/check-changed.mjs b/scripts/check-changed.mjs index 67395f518cb7..303d3f56fabf 100644 --- a/scripts/check-changed.mjs +++ b/scripts/check-changed.mjs @@ -354,13 +354,67 @@ export function createNpmLockGuardCommand(paths) { }; } +// Enough of the wrapper tail to hold its run summary; the rest is streamed, not kept. +const DELEGATION_OUTPUT_TAIL_LIMIT = 64 * 1024; + +/** + * Signatures of a failure that happened before the remote command was dispatched: + * the broker or its API was unreachable, or no lease was ever obtained. + */ +const BACKEND_UNAVAILABLE_SIGNATURES = [ + /request failed: \w+ "https?:\/\/[^"]*blacksmith[^"]*"/iu, + /context deadline exceeded/iu, + /(?:no such host|dial tcp|connection refused|network is unreachable)/iu, + /failed to (?:acquire|create|warm|start)\b[^\n]*\b(?:lease|testbox)/iu, +]; + +/** + * Whether a failed delegation provably never ran our command. + * + * Fails closed on purpose. A missing final summary alone cannot prove the remote + * never started — a wrapper that crashes or loses its output transport after + * dispatch looks identical — so this requires a positive pre-dispatch signature + * and treats everything else as a real failure. Getting this backwards is the + * dangerous direction: some lanes (prompt snapshots) are Linux-only truth, so a + * local rerun on macOS could turn an unknown or failing gate green. + * + * `command-exit` vetoes regardless: it only appears once the command reached the + * box, so it is proof a verdict exists and must be propagated as-is. + */ +export function delegationFailedBeforeRunning(output) { + if (/"errorKind"\s*:\s*"command-exit"/u.test(output)) { + return false; + } + return BACKEND_UNAVAILABLE_SIGNATURES.some((signature) => signature.test(output)); +} + async function runChangedCheckViaCrabbox(argv = [], env = process.env) { console.error("[check:changed] delegating to Blacksmith Testbox via the Node wrapper."); - return await runManagedCommand({ + let tail = ""; + const exitCode = await runManagedCommand({ bin: "node", args: buildChangedCheckCrabboxArgs(argv), env, + stdio: ["inherit", "pipe", "pipe"], + onReady: (child) => { + for (const stream of [child.stdout, child.stderr]) { + stream?.on("data", (chunk) => { + tail = (tail + chunk).slice(-DELEGATION_OUTPUT_TAIL_LIMIT); + // Inherited stdio used to get OS backpressure for free. Piping means we + // have to reapply it, or a verbose delegated run buffers its whole + // output in this process when stderr is an async pipe (typical in CI). + if (!process.stderr.write(chunk)) { + stream.pause(); + process.stderr.once("drain", () => stream.resume()); + } + }); + } + }, }); + return { + exitCode, + backendUnavailable: exitCode !== 0 && delegationFailedBeforeRunning(tail), + }; } export function createChangedCheckPlan(result, options = {}) { @@ -1006,7 +1060,13 @@ if (isDirectRun()) { if (!shouldDelegateChangedCheckToCrabbox(argv, process.env)) { throw error; } - process.exitCode = await runChangedCheckViaCrabbox(argv, process.env); + // No local fallback here: this path exists because the checkout cannot + // resolve the diff refs itself, so there is nothing local to run. + const delegated = await runChangedCheckViaCrabbox(argv, process.env); + if (delegated.backendUnavailable) { + throw error; + } + process.exitCode = delegated.exitCode; } if (paths) { const result = detectChangedLanesForPaths({ @@ -1028,7 +1088,20 @@ if (isDirectRun()) { : undefined, }) ) { - process.exitCode = await runChangedCheckViaCrabbox(argv, process.env); + const delegated = await runChangedCheckViaCrabbox(argv, process.env); + if (delegated.backendUnavailable) { + // Say this loudly: the proof below is local, so whoever reads the run + // knows which machine produced it and that Linux-only lanes are unproven. + console.error( + "[check:changed] Blacksmith never ran the checks (no run summary). Falling back to local execution; note this in the proof summary.", + ); + } + process.exitCode = delegated.backendUnavailable + ? await runChangedCheck(result, { + ...args, + explicitPaths: args.paths.length > 0, + }) + : delegated.exitCode; } else { process.exitCode = await runChangedCheck(result, { ...args, diff --git a/test/scripts/changed-lanes.test.ts b/test/scripts/changed-lanes.test.ts index b79121002e8c..6a90dd8ff2e7 100644 --- a/test/scripts/changed-lanes.test.ts +++ b/test/scripts/changed-lanes.test.ts @@ -38,6 +38,7 @@ import { shouldRunSqliteSessionSchemaBaselineCheck, shouldRunTestTempCreationReport, createNpmLockGuardCommand, + delegationFailedBeforeRunning, } from "../../scripts/check-changed.mjs"; import { resolveOxfmtInvocation } from "../../scripts/format-docs.mjs"; import { isDirectRunPath } from "../../scripts/lib/direct-run.mjs"; @@ -2083,3 +2084,52 @@ describe("scripts/changed-lanes", () => { ]); }); }); + +describe("delegationFailedBeforeRunning", () => { + // The wrapper only prints a run summary once the command reached the box, so + // the summary is the evidence that a verdict exists at all. + it("treats a lease or network failure as never having run", () => { + const output = [ + 'request failed: Get "https://backend.blacksmith.sh/api/testbox/list?all=true": context deadline exceeded', + "blacksmith testbox run exited 1", + ].join("\n"); + + expect(delegationFailedBeforeRunning(output)).toBe(true); + }); + + it("treats a reported command exit as a real check failure", () => { + const output = [ + " 64.95s failed:1 typecheck core tests", + '{"provider":"blacksmith-testbox","runStatus":"failed","errorKind":"command-exit","exitCode":1}', + ].join("\n"); + + // Falling back locally here would re-run on macOS and could pass a lane + // whose truth is Linux, turning a red gate green. + expect(delegationFailedBeforeRunning(output)).toBe(false); + }); + + it("does not mistake an infrastructure error kind for a command verdict", () => { + const output = [ + "failed to acquire lease for testbox", + '{"provider":"blacksmith-testbox","runStatus":"failed","errorKind":"lease-timeout","exitCode":1}', + ].join("\n"); + + expect(delegationFailedBeforeRunning(output)).toBe(true); + }); + + // A crash after dispatch produces no summary either, so absence of one cannot + // be read as "never ran" — that is how an unknown Linux result would go green. + it("fails closed when the wrapper dies without saying why", () => { + expect(delegationFailedBeforeRunning("node: killed\n")).toBe(false); + expect(delegationFailedBeforeRunning("")).toBe(false); + }); + + it("keeps a command verdict authoritative even alongside network noise", () => { + const output = [ + 'request failed: Get "https://backend.blacksmith.sh/api/testbox/list": context deadline exceeded', + '{"provider":"blacksmith-testbox","runStatus":"failed","errorKind":"command-exit","exitCode":1}', + ].join("\n"); + + expect(delegationFailedBeforeRunning(output)).toBe(false); + }); +});