mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-02 05:51:39 +00:00
fix(scripts): run changed checks locally when Blacksmith never ran them (#114225)
* 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.
This commit is contained in:
committed by
GitHub
parent
e9ef30e739
commit
c5d6dcfc84
@@ -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;
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user