diff --git a/.changeset/fuzzy-otters-signal.md b/.changeset/fuzzy-otters-signal.md new file mode 100644 index 00000000000..eb802171c1e --- /dev/null +++ b/.changeset/fuzzy-otters-signal.md @@ -0,0 +1,5 @@ +--- +"@kilocode/cli": patch +--- + +Report Bash commands terminated by a signal with the conventional 128 + signum exit code (e.g. 139 for SIGSEGV) instead of hanging until the command timeout. diff --git a/packages/core/src/cross-spawn-spawner.ts b/packages/core/src/cross-spawn-spawner.ts index 554bea48928..0591311f1ce 100644 --- a/packages/core/src/cross-spawn-spawner.ts +++ b/packages/core/src/cross-spawn-spawner.ts @@ -4,6 +4,7 @@ import * as NodePath from "@effect/platform-node/NodePath" import { prepareCommand as prepareSandbox } from "@kilocode/sandbox" // kilocode_change import { tap as tapStdio, tapped } from "./kilocode/stdio-tap" // kilocode_change - Bun drops buffered stdio on close import * as SpawnValidation from "./kilocode/spawn-validation" // kilocode_change +import { settle } from "./kilocode/exit-code" // kilocode_change - settle signal termination as 128 + signum import * as Deferred from "effect/Deferred" import * as Effect from "effect/Effect" import * as Exit from "effect/Exit" @@ -441,16 +442,7 @@ export const make = Effect.gen(function* () { getInputFd: fd.getInputFd, getOutputFd: fd.getOutputFd, isRunning: Effect.map(Deferred.isDone(signal), (done) => !done), - exitCode: Effect.flatMap(Deferred.await(signal), ([code, signal]) => { - if (Predicate.isNotNull(code)) return Effect.succeed(ExitCode(code)) - return Effect.fail( - toPlatformError( - "exitCode", - new Error(`Process interrupted due to receipt of signal: '${signal}'`), - command, - ), - ) - }), + exitCode: Effect.flatMap(Deferred.await(signal), settle), // kilocode_change - signal termination settles as 128 + signum kill: (opts?: ChildProcess.KillOptions) => { const sig = opts?.killSignal ?? "SIGTERM" const send = (s: NodeJS.Signals) => diff --git a/packages/core/src/kilocode/exit-code.ts b/packages/core/src/kilocode/exit-code.ts new file mode 100644 index 00000000000..4a4408a7b4a --- /dev/null +++ b/packages/core/src/kilocode/exit-code.ts @@ -0,0 +1,16 @@ +import { constants } from "node:os" +import * as Effect from "effect/Effect" +import * as Predicate from "effect/Predicate" +import { ExitCode } from "effect/unstable/process/ChildProcessSpawner" + +// A process terminated by a signal produces a null exit code (see the "exit" +// event on node:child_process). Report the conventional 128 + signum code +// (e.g. 139 for SIGSEGV) instead of failing, which left consumers like the +// bash tool waiting on a numeric code that never arrived. +export const settle = ([code, signal]: readonly [code: number | null, signal: NodeJS.Signals | null]) => { + if (Predicate.isNotNull(code)) return Effect.succeed(ExitCode(code)) + if (Predicate.isNotNull(signal) && signal in constants.signals) { + return Effect.succeed(ExitCode(128 + constants.signals[signal])) + } + return Effect.succeed(ExitCode(1)) +} diff --git a/packages/core/test/kilocode/exit-code.test.ts b/packages/core/test/kilocode/exit-code.test.ts new file mode 100644 index 00000000000..a316e839b38 --- /dev/null +++ b/packages/core/test/kilocode/exit-code.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, test } from "bun:test" +import { Effect } from "effect" +import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process" +import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" +import { settle } from "@opencode-ai/core/kilocode/exit-code" +import { testEffect } from "../lib/effect" + +const fx = testEffect(CrossSpawnSpawner.defaultLayer) + +const code = (exit: readonly [number | null, NodeJS.Signals | null]) => Effect.runPromise(settle(exit)) + +describe("exit-code settle", () => { + test("maps exit results to numeric codes", async () => { + expect(await code([0, null])).toBe(ChildProcessSpawner.ExitCode(0)) + expect(await code([42, null])).toBe(ChildProcessSpawner.ExitCode(42)) + expect(await code([null, "SIGSEGV"])).toBe(ChildProcessSpawner.ExitCode(128 + 11)) + expect(await code([null, "SIGTERM"])).toBe(ChildProcessSpawner.ExitCode(128 + 15)) + expect(await code([null, null])).toBe(ChildProcessSpawner.ExitCode(1)) + expect(await code([null, "SIGWHAT" as NodeJS.Signals])).toBe(ChildProcessSpawner.ExitCode(1)) + }) + + fx.effect( + "reports signal termination as 128 + signum", + Effect.gen(function* () { + if (process.platform === "win32") return + const handle = yield* ChildProcess.make(process.execPath, ["-e", "process.kill(process.pid, 'SIGKILL')"]) + expect(yield* handle.exitCode).toBe(ChildProcessSpawner.ExitCode(128 + 9)) + }), + ) +}) diff --git a/packages/opencode/test/kilocode/tool/shell-signal.test.ts b/packages/opencode/test/kilocode/tool/shell-signal.test.ts new file mode 100644 index 00000000000..e62ab3ce540 --- /dev/null +++ b/packages/opencode/test/kilocode/tool/shell-signal.test.ts @@ -0,0 +1,72 @@ +// Regression tests for Kilo-Org/kilocode#12677. +// +// A shell terminated by a signal (for example `bash -c` exec'ing a binary that +// then segfaults) produces no numeric exit code. The spawner used to fail the +// exit-code effect in that case, so the bash tool's race kept waiting for the +// abort or timeout branches and the call appeared to hang. The spawner now +// settles signal termination as the conventional 128 + signum code. + +import { describe, expect } from "bun:test" +import { Effect, Layer } from "effect" +import path from "path" +import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" +import { FSUtil } from "@opencode-ai/core/fs-util" +import { Agent } from "../../../src/agent/agent" +import { Config } from "../../../src/config/config" +import { RuntimeFlags } from "../../../src/effect/runtime-flags" +import { Plugin } from "../../../src/plugin" +import { SessionID, MessageID } from "../../../src/session/schema" +import { ShellTool } from "../../../src/tool/shell" +import { Truncate } from "../../../src/tool/truncate" +import { provideInstance, testInstanceStoreLayer } from "../../fixture/fixture" +import { testEffect } from "../../lib/effect" + +const layer = Layer.mergeAll( + CrossSpawnSpawner.defaultLayer, + FSUtil.defaultLayer, + Plugin.defaultLayer, + Truncate.defaultLayer, + Config.defaultLayer, + Agent.defaultLayer, + RuntimeFlags.defaultLayer, + testInstanceStoreLayer, +) +const it = testEffect(layer) + +const ctx = { + sessionID: SessionID.make("ses_test"), + messageID: MessageID.make("msg_test"), + callID: "", + agent: "code", + abort: AbortSignal.any([]), + messages: [], + metadata: () => Effect.void, + ask: () => Effect.void, +} + +const root = path.join(__dirname, "../../..") + +describe("shell tool signal termination", () => { + if (process.platform !== "win32") { + it.live("settles with 128 + signum when the shell dies from a signal", () => + provideInstance(root)( + Effect.gen(function* () { + const tool = yield* ShellTool + const bash = yield* tool.init() + const result = yield* bash + .execute( + { + command: "kill -SEGV $$", + description: "Terminate the shell with SIGSEGV", + timeout: 60_000, + }, + ctx, + ) + .pipe(Effect.timeout("5 seconds")) + expect(result.metadata.exit).toBe(128 + 11) + expect(result.output).not.toContain("exceeding timeout") + }), + ), + ) + } +})