From c8b298ffac6ca788ee31b43f49d51208d7ddb706 Mon Sep 17 00:00:00 2001 From: Waishnav <86405648+Waishnav@users.noreply.github.com> Date: Fri, 30 Oct 2026 18:08:39 +0530 Subject: [PATCH 1/5] fix: address review findings on workflow skill docs and agents models Workflow skill (#440): - Resume docs now state the RECOVERY_REQUIRED refusal for runs whose calls used allowed write mode, isolation, or workspace (read-only ones included) and the RECOVERY_CONTEXT_CHANGED refusal. - The limits note now says stack overflow ends the run but heap exhaustion is a catchable InternalError, as verified in the sandbox. - The QA-gate pattern no longer applies a final fix it never tests. - A directory at a supporting skill file path is skipped instead of failing every workspace open; SKILL.md stays strict. agents models (#441): - A built-in provider left out of config can still be listed by name. - providerErrorDetail reads the agent's stderr from process-launch causes ({ error, stderr }), which also fixes ACP init errors in runs. - ACP session/new and Codex model/list are bounded at 30s so one hung harness cannot stall the whole command. - opencode models launches through cross-spawn, so Windows .cmd shims work like the server launch. - Pi resolves the listed provider/id before a gateway's bare model id. Co-Authored-By: Claude Opus 5.5 --- skills/workflows/patterns.md | 7 ++++-- skills/workflows/reference.md | 16 ++++++++----- src/cli.ts | 10 ++++++++- src/local-agent-acp.ts | 7 ++++-- src/local-agent-codex.ts | 25 ++++++++++++++++++++- src/local-agent-events.test.ts | 11 +++++++++ src/local-agent-events.ts | 10 ++++++++- src/local-agent-opencode-version.ts | 35 +++++++++++++++++++++++++++++ src/local-agent-opencode.ts | 15 ++----------- src/local-agent-pi.test.ts | 27 ++++++++++++++++++++++ src/local-agent-pi.ts | 6 +++-- src/skills.ts | 2 ++ 12 files changed, 144 insertions(+), 27 deletions(-) diff --git a/skills/workflows/patterns.md b/skills/workflows/patterns.md index a8faafc8d..64eda9996 100644 --- a/skills/workflows/patterns.md +++ b/skills/workflows/patterns.md @@ -19,8 +19,9 @@ keep the parts that fit the task. Each one is a complete script. - **Use the right workspace.** Read-only steps run in the checkout. Sequential edits share one named worktree (`isolation: "worktree"` plus `workspace`). Parallel edits each get their own worktree. - **Handle per-item failures.** `parallel` and `pipeline` never throw; check each `status`. -- **Keep it resumable.** Same inputs should produce the same calls in the same order: no timestamps or random values - in prompts. +- **Know what resumes.** Same inputs should produce the same calls in the same order: no timestamps or random values + in prompts. Only read-only runs in the checkout can be resumed; a run that used `writeMode: "allowed"`, + `isolation`, or `workspace` (as most edit loops below do) must be restarted instead. Start writing runs with `--write-mode allowed`; everything else stays read-only by default. @@ -115,6 +116,8 @@ for (let attempt = 1; attempt <= attempts; attempt++) { { target: tester, writeMode: "read_only", workspace: "qa", schema: RESULT, label: `test-${attempt}` }, ); if (result.passed) return { passed: true, attempts: attempt }; + // The verdict always comes from a test run: do not apply a fix that nothing will test. + if (attempt === attempts) return { passed: false, attempts, failures: result.failures }; await agent(`Make these failures pass without weakening the tests:\n${result.failures.join("\n")}`, { target: implementer, writeMode: "allowed", workspace: "qa", label: `fix-${attempt}`, }); diff --git a/skills/workflows/reference.md b/skills/workflows/reference.md index 840e2924f..b199f8554 100644 --- a/skills/workflows/reference.md +++ b/skills/workflows/reference.md @@ -113,8 +113,13 @@ For proven multi-step engineering loops (plan, implement, review, QA, bake-offs, `devspace workflow run --resume ` reruns the script with the original arguments. Completed read-only calls are reused as long as the script makes the same calls in the same order with the same prompts and options; from the first -call that differs, everything runs fresh. Runs that made editing calls cannot be resumed. Keep prompts deterministic -(no timestamps or random values) so resume can reuse work. +call that differs, everything runs fresh. Keep prompts deterministic (no timestamps or random values) so resume can +reuse work. + +Resume is refused (`RECOVERY_REQUIRED`) for a run that made any call with `writeMode: "allowed"`, `isolation`, or +`workspace`, even a read-only call, because those may have changed a workspace: inspect its calls and preserved +worktrees, then start a new run. It is also refused (`RECOVERY_CONTEXT_CHANGED`) when the project's context changed +since the original run. ## Limits @@ -128,6 +133,7 @@ call that differs, everything runs fresh. Runs that made editing calls cannot be | `log` and `phase` events | 64 KiB per run | | Sandbox heap / stack | 32 MiB / 512 KiB | -Exceeding a limit ends the whole run, including a `workflow()` call nested past one level: `try`/`catch` in the -script cannot recover from it. Ordinary agent failures (unknown target, provider errors, invalid output) are normal -rejections you can catch, and `parallel`/`pipeline` report them per item. +Exceeding a run limit ends the whole run, and `try`/`catch` in the script cannot recover from it: the turn, nesting +(a `workflow()` nested past one level), time, size, and event limits, and stack overflow. Running out of sandbox heap +is different: it throws a catchable `InternalError: out of memory`. Ordinary agent failures (unknown target, provider +errors, invalid output) are normal rejections you can catch, and `parallel`/`pipeline` report them per item. diff --git a/src/cli.ts b/src/cli.ts index b55c93f9f..752555375 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -718,14 +718,22 @@ async function runAgentsModels(args: string[], json: boolean): Promise { const config = loadAgentConfig(); const { workspaceRoot } = resolveAgentCliWorkspaceContext(config); // Loaded on demand: the adapters pull in every provider SDK, which other commands never need. - const { createLocalAgentDrivers } = await import("./local-agent-adapters.js"); + const { createLocalAgentDrivers, createLocalAgentProviderRegistry } = await import("./local-agent-adapters.js"); const { providerErrorDetail } = await import("./local-agent-events.js"); + const { defaultDriverForProviderId } = await import("./local-agent-provider.js"); const drivers = createLocalAgentDrivers({ subagents: config.subagents }); const selected = name ? drivers.filter((driver) => driver.providerInstanceId === name) : drivers.filter((driver) => config.subagents.providers.some((provider) => provider.id === driver.providerInstanceId && provider.enabled)); + // A built-in provider left out of config is still answerable by name, with its default settings. + const builtInDriver = name && selected.length === 0 ? defaultDriverForProviderId(name) : undefined; + + if (name && builtInDriver) { + selected.push(createLocalAgentProviderRegistry().create({ id: name, driver: builtInDriver, enabled: true }, undefined, process.env)); + } + if (name && selected.length === 0) throw new Error(`Unknown subagent provider: ${name}.`); const detail = (error: Parameters[0]) => { diff --git a/src/local-agent-acp.ts b/src/local-agent-acp.ts index 6d2106cfc..d7d9eeaee 100644 --- a/src/local-agent-acp.ts +++ b/src/local-agent-acp.ts @@ -108,6 +108,8 @@ export interface AcpRuntimeOptions { stderrTail?: () => string; } +const LIST_MODELS_TIMEOUT_MS = 30_000; + export class AcpRuntime implements LocalAgentRuntime { readonly provider = "acp" as const; private readonly flavor: AcpFlavor; @@ -336,12 +338,13 @@ export class AcpRuntime implements LocalAgentRuntime { /** Opens a throwaway read-only session in `workspaceRoot` to read the models the agent offers there. */ async listModels(workspaceRoot: string): Promise { - const response = await this.connection.agent.request("session/new", { + // Bounded so one unresponsive agent cannot stall `devspace agents models` for every provider. + const response = await withTimeout(this.connection.agent.request("session/new", { cwd: workspaceRoot, mcpServers: [], ...this.additionalDirectoryParams(), ...this.permissionMeta({ prompt: "", workspaceRoot, writeMode: "read_only" }), - }); + }), LIST_MODELS_TIMEOUT_MS, `${acpAgentName(this.flavor)} did not answer session/new within ${LIST_MODELS_TIMEOUT_MS / 1000}s.`); return acpModelsFromSession(response, this.flavor); } diff --git a/src/local-agent-codex.ts b/src/local-agent-codex.ts index 854b44ac7..c43293c18 100644 --- a/src/local-agent-codex.ts +++ b/src/local-agent-codex.ts @@ -96,6 +96,23 @@ export interface CodexAppServerRuntimeOptions { version?: string; } +const CODEX_LIST_MODELS_TIMEOUT_MS = 30_000; + +async function withDeadline(promise: Promise, timeoutMs: number, message: string): Promise { + let timer: NodeJS.Timeout | undefined; + + try { + return await Promise.race([ + promise, + new Promise((_resolve, reject) => { + timer = setTimeout(() => reject(new Error(message)), timeoutMs); + }), + ]); + } finally { + clearTimeout(timer); + } +} + export class CodexAppServerRuntime implements LocalAgentRuntime { readonly provider = "codex" as const; private readonly child: ChildProcessWithoutNullStreams; @@ -137,7 +154,13 @@ export class CodexAppServerRuntime implements LocalAgentRuntime { let cursor: string | undefined; do { - const page = await this.rpc.request("model/list", optionalFields(cursor ? { cursor } : undefined)); + // Bounded so a stalled app-server cannot stall `devspace agents models` for every provider. + const page = await withDeadline( + this.rpc.request("model/list", optionalFields(cursor ? { cursor } : undefined)), + CODEX_LIST_MODELS_TIMEOUT_MS, + `Codex app-server did not answer model/list within ${CODEX_LIST_MODELS_TIMEOUT_MS / 1000}s.`, + ); + pages.push(page); cursor = readString(page, "nextCursor"); } while (cursor && pages.length < 20); diff --git a/src/local-agent-events.test.ts b/src/local-agent-events.test.ts index 7dde587c9..3f9413408 100644 --- a/src/local-agent-events.test.ts +++ b/src/local-agent-events.test.ts @@ -48,3 +48,14 @@ test("provider error detail surfaces the most specific distinct cause", () => { ); assert.equal(providerErrorDetail({ message: "No cause." }), undefined); }); + +test("a process-launch cause reports the agent's stderr, or else its nested error", () => { + assert.equal( + providerErrorDetail({ message: "grok ACP initialization failed.", cause: { error: new Error("closed"), stderr: "not logged in\n" } }), + "not logged in", + ); + assert.equal( + providerErrorDetail({ message: "grok ACP initialization failed.", cause: { error: new Error("spawn grok ENOENT") } }), + "spawn grok ENOENT", + ); +}); diff --git a/src/local-agent-events.ts b/src/local-agent-events.ts index 1a5cf2c54..4155cabb5 100644 --- a/src/local-agent-events.ts +++ b/src/local-agent-events.ts @@ -123,7 +123,15 @@ export function providerErrorDetail(error: { message: string; cause?: WireInput } // SAFETY: The surrounding checks or construction already match this type. - current = isObjectValue(current) ? (current as { cause?: unknown }).cause : undefined; + const record = isObjectValue(current) ? (current as { cause?: unknown; error?: unknown; stderr?: unknown }) : undefined; + + // Process-launch failures (such as ACP initialization) carry `{ error, stderr }`: the agent's own stderr + // explains the failure better than the transport error, so prefer it. + if (isJsonString(record?.stderr) && record.stderr.trim()) { + return boundText(redactSecrets(record.stderr.trim()), MAX_ERROR_DETAIL_CHARS); + } + + current = record?.cause ?? record?.error; } return undefined; diff --git a/src/local-agent-opencode-version.ts b/src/local-agent-opencode-version.ts index 37a2906f6..0ea56d0a1 100644 --- a/src/local-agent-opencode-version.ts +++ b/src/local-agent-opencode-version.ts @@ -146,3 +146,38 @@ function runOpenCodeVersion(env: NodeJS.ProcessEnv): Promise { }); }); } + +const OPENCODE_MODELS_TIMEOUT_MS = 60_000; + +/** Runs `opencode models` through cross-spawn, so a Windows `.cmd` shim launches like the server does. */ +export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise { + return new Promise((resolve, reject) => { + const child = spawn(resolveOpenCodeCommand(env), ["models"], { cwd, env, stdio: ["ignore", "pipe", "pipe"] }); + let stdout = ""; + let stderr = ""; + let settled = false; + + const finish = (error?: Error) => { + if (settled) return; + settled = true; + clearTimeout(timer); + + if (error) reject(error); + else resolve(stdout); + }; + + const timer = setTimeout(() => { + child.kill("SIGTERM"); + finish(new Error(`opencode models did not finish within ${OPENCODE_MODELS_TIMEOUT_MS / 1000}s.`)); + }, OPENCODE_MODELS_TIMEOUT_MS); + + timer.unref(); + child.stdout?.on("data", (chunk: Buffer | string) => { stdout += chunk.toString(); }); + child.stderr?.on("data", (chunk: Buffer | string) => { stderr += chunk.toString(); }); + child.once("error", (cause) => finish(cause)); + // `close` waits for stdout/stderr to drain; `exit` can fire before the list is read. + child.once("close", (code) => finish( + code === 0 ? undefined : new Error(stderr.trim() || `opencode models exited with code ${code ?? 1}.`), + )); + }); +} diff --git a/src/local-agent-opencode.ts b/src/local-agent-opencode.ts index 21b110478..00f4de039 100644 --- a/src/local-agent-opencode.ts +++ b/src/local-agent-opencode.ts @@ -1,5 +1,3 @@ -import { execFile } from "node:child_process"; -import { promisify } from "node:util"; import { isObjectValue, isJsonString, optionalFields } from "./json-value.js"; import { unsafeNarrow } from "./unsafe-narrow.js"; import type { WireInput, JsonObject } from "./json-value.js"; @@ -31,7 +29,7 @@ import { } from "./local-agent-opencode-v2.js"; import { probeOpenCodeBinary, - resolveOpenCodeCommand, + runOpenCodeModels, requireOpenCodeV1NativeSessionId, type OpenCodeRuntimeProbeFunction, } from "./local-agent-opencode-version.js"; @@ -237,21 +235,12 @@ export class OpencodeLocalAgentDriver implements LocalAgentDriver { provider: this.provider, operation: "list_models", run: async () => { - const { stdout } = await execFileAsync(resolveOpenCodeCommand(this.env), ["models"], { - cwd: workspaceRoot, - env: this.env, - timeout: 60_000, - maxBuffer: 4 * 1024 * 1024, - }); - - return opencodeModelsFromCli(stdout); + return opencodeModelsFromCli(await runOpenCodeModels(this.env, workspaceRoot)); }, }); } } -const execFileAsync = promisify(execFile); - export function opencodeModelsFromCli(stdout: string): LocalAgentModel[] { return stdout.split("\n") .map((line) => line.trim()) diff --git a/src/local-agent-pi.test.ts b/src/local-agent-pi.test.ts index 6b03b8aeb..56915b067 100644 --- a/src/local-agent-pi.test.ts +++ b/src/local-agent-pi.test.ts @@ -305,6 +305,33 @@ test("an exact model id wins over provider/model parsing", async () => { await gatewayRuntime.value.close(); }); +// The provider/id value `devspace agents models` lists wins over a gateway model whose bare id is the same string. +test("a listed provider/id wins over a gateway model with that id", async () => { + const connection = new FakePiRpcConnection(); + connection.request = async function(record: JsonObject): Promise { + if (record.type === "get_available_models") { + this.requests.push(record); + + return { models: [{ provider: "gateway", id: "xai/grok-4.5" }, { provider: "xai", id: "grok-4.5" }] }; + } + + return FakePiRpcConnection.prototype.request.call(this, record); + }; + + const runtime = await new PiLocalAgentDriver(async () => ({ connection })).createRuntime(context); + + if (runtime.isErr()) throw runtime.error; + + const result = await runtime.value.run({ prompt: "inspect", workspaceRoot: "/tmp/project", model: "xai/grok-4.5" }); + + assert.equal(result.isOk(), true); + assert.deepEqual( + connection.requests.find((request) => request.type === "set_model"), + { type: "set_model", provider: "xai", modelId: "grok-4.5" }, + ); + await runtime.value.close(); +}); + // Stop: Pi is aborted, a Pi that never settles is discarded, and stale events never reach the next turn. test("stopping aborts Pi and discards a Pi that does not settle", async () => { const stalledConnection = new FakePiRpcConnection(); diff --git a/src/local-agent-pi.ts b/src/local-agent-pi.ts index 5159a7862..5afd23aba 100644 --- a/src/local-agent-pi.ts +++ b/src/local-agent-pi.ts @@ -203,10 +203,12 @@ export class PiRpcRuntime implements LocalAgentRuntime { } private async resolveModel(model: string): Promise<{ provider: string; modelId: string }> { - // Gateway providers have model ids that contain "/", so an exact id wins over provider/model. + // `devspace agents models` lists Pi models as provider/id, so that form wins. Gateway providers have model ids + // that contain "/", so an exact id comes next, before parsing the value as provider/model. const available = await this.connection.request({ type: "get_available_models" }); const models = recordArray(available, "models"); - const matches = models.filter((entry) => piRecordString(entry, "id") === model); + const listed = models.filter((entry) => `${piRecordString(entry, "provider")}/${piRecordString(entry, "id")}` === model); + const matches = listed.length === 1 ? listed : models.filter((entry) => piRecordString(entry, "id") === model); if (matches.length === 1) { const provider = piRecordString(matches[0], "provider"); diff --git a/src/skills.ts b/src/skills.ts index 59201d724..345aa0eee 100644 --- a/src/skills.ts +++ b/src/skills.ts @@ -65,6 +65,8 @@ function syncManagedFile(sourcePath: string, targetPath: string, name: ManagedSk } if (stat.isDirectory()) { + // A broken supporting file must not stop the skill (or the workspace) from loading; SKILL.md must. + if (!targetPath.endsWith("SKILL.md")) return; throw new Error(`Managed ${name} skill path is a directory: ${targetPath}`); } } From f979537e9094146d0d51421a93330ef56800ec5d Mon Sep 17 00:00:00 2001 From: Waishnav <86405648+Waishnav@users.noreply.github.com> Date: Fri, 30 Oct 2026 18:22:37 +0530 Subject: [PATCH 2/5] fix: redact full Authorization values and bound opencode models - Redact the whole Authorization value (any scheme), since agent stderr now reaches error details and Basic credentials survived. - Escalate opencode models from SIGTERM to SIGKILL and cap its output at 4 MiB, as the replaced execFile did. Co-Authored-By: Claude Opus 5.5 --- src/local-agent-events.test.ts | 11 +++++++++++ src/local-agent-events.ts | 4 ++++ src/local-agent-opencode-version.ts | 30 +++++++++++++++++++++++++---- 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/local-agent-events.test.ts b/src/local-agent-events.test.ts index 3f9413408..6c7d77df4 100644 --- a/src/local-agent-events.test.ts +++ b/src/local-agent-events.test.ts @@ -59,3 +59,14 @@ test("a process-launch cause reports the agent's stderr, or else its nested erro "spawn grok ENOENT", ); }); + +test("an Authorization value is redacted in full, whatever its scheme", () => { + assert.equal( + providerErrorDetail({ message: "init failed", cause: { stderr: "GET /models Authorization: Basic dXNlcjpwYXNz failed" } }), + "GET /models Authorization: [redacted] failed", + ); + assert.equal( + providerErrorDetail({ message: "init failed", cause: { stderr: "authorization=Bearer abcdefghijklmnop" } }), + "authorization=[redacted]", + ); +}); diff --git a/src/local-agent-events.ts b/src/local-agent-events.ts index 4155cabb5..db41379bd 100644 --- a/src/local-agent-events.ts +++ b/src/local-agent-events.ts @@ -150,6 +150,9 @@ const SECRET_PATTERNS: readonly RegExp[] = [ const SECRET_ASSIGNMENT = /\b([A-Za-z0-9_]*(?:api[_-]?key|token|secret|password|passwd|authorization|credential)[A-Za-z0-9_]*)(["']?\s*[:=]\s*["']?)(?:Bearer\s+)?([^\s"',;]{4,})/gi; const BEARER = /\bBearer\s+[A-Za-z0-9._~+/=-]{8,}/g; +// `Authorization: Basic ` (or any scheme): redact the whole value, not just the scheme word. +const AUTHORIZATION_VALUE = /\b(authorization["']?\s*[:=]\s*["']?)[A-Za-z][\w-]*\s+[^\s"',;]+/gi; + // `https://user:pass@host`: keep the user, drop the password. The password runs to the // authority's last `@` (it may contain `@`) and never past `/`, `?` or `#`. const URL_PASSWORD = /\b([a-z][a-z0-9+.-]*:\/\/[^\s/?#@:]*):[^\s/?#]*@(?=[^\s/?#@]*(?:[\s/?#]|$))/gi; @@ -162,6 +165,7 @@ export function redactSecrets(text: string): string { let redacted = text; for (const pattern of SECRET_PATTERNS) redacted = redacted.replace(pattern, "[redacted]"); + redacted = redacted.replace(AUTHORIZATION_VALUE, "$1[redacted]"); redacted = redacted.replace(BEARER, "Bearer [redacted]"); redacted = redacted.replace(URL_PASSWORD, "$1:[redacted]@"); redacted = redacted.replace(SECRET_FLAG, "$1[redacted]"); diff --git a/src/local-agent-opencode-version.ts b/src/local-agent-opencode-version.ts index 0ea56d0a1..ccb1410a4 100644 --- a/src/local-agent-opencode-version.ts +++ b/src/local-agent-opencode-version.ts @@ -148,6 +148,8 @@ function runOpenCodeVersion(env: NodeJS.ProcessEnv): Promise { } const OPENCODE_MODELS_TIMEOUT_MS = 60_000; +const OPENCODE_MODELS_MAX_BYTES = 4 * 1024 * 1024; +const OPENCODE_MODELS_KILL_GRACE_MS = 2_000; /** Runs `opencode models` through cross-spawn, so a Windows `.cmd` shim launches like the server does. */ export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise { @@ -155,6 +157,7 @@ export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise< const child = spawn(resolveOpenCodeCommand(env), ["models"], { cwd, env, stdio: ["ignore", "pipe", "pipe"] }); let stdout = ""; let stderr = ""; + let bytes = 0; let settled = false; const finish = (error?: Error) => { @@ -166,14 +169,33 @@ export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise< else resolve(stdout); }; - const timer = setTimeout(() => { + // A child that ignores SIGTERM is killed outright, so a stuck listing never outlives the command. + const stop = (error: Error) => { child.kill("SIGTERM"); - finish(new Error(`opencode models did not finish within ${OPENCODE_MODELS_TIMEOUT_MS / 1000}s.`)); + setTimeout(() => { if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL"); }, OPENCODE_MODELS_KILL_GRACE_MS).unref(); + finish(error); + }; + + const timer = setTimeout(() => { + stop(new Error(`opencode models did not finish within ${OPENCODE_MODELS_TIMEOUT_MS / 1000}s.`)); }, OPENCODE_MODELS_TIMEOUT_MS); + const collect = (chunk: Buffer | string, into: "stdout" | "stderr") => { + bytes += Buffer.byteLength(chunk); + + if (bytes > OPENCODE_MODELS_MAX_BYTES) { + stop(new Error(`opencode models printed more than ${OPENCODE_MODELS_MAX_BYTES / 1024 / 1024} MiB.`)); + + return; + } + + if (into === "stdout") stdout += chunk.toString(); + else stderr += chunk.toString(); + }; + timer.unref(); - child.stdout?.on("data", (chunk: Buffer | string) => { stdout += chunk.toString(); }); - child.stderr?.on("data", (chunk: Buffer | string) => { stderr += chunk.toString(); }); + child.stdout?.on("data", (chunk: Buffer | string) => collect(chunk, "stdout")); + child.stderr?.on("data", (chunk: Buffer | string) => collect(chunk, "stderr")); child.once("error", (cause) => finish(cause)); // `close` waits for stdout/stderr to drain; `exit` can fire before the list is read. child.once("close", (code) => finish( From 64ff2d5513e58b93c6d6806df8c3685faa123e88 Mon Sep 17 00:00:00 2001 From: Waishnav <86405648+Waishnav@users.noreply.github.com> Date: Fri, 30 Oct 2026 18:27:39 +0530 Subject: [PATCH 3/5] fix(opencode): report a stopped model listing only after it exits Reject only once the child closes (or after the SIGKILL escalation), and run it in its own process group so the stop reaches processes it started. A real-process test covers a listing that ignores SIGTERM and floods past the cap. Co-Authored-By: Claude Opus 5.5 --- src/local-agent-opencode-version.test.ts | 24 +++++++++++++++++++++++- src/local-agent-opencode-version.ts | 23 +++++++++++++++++------ 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/local-agent-opencode-version.test.ts b/src/local-agent-opencode-version.test.ts index d18220821..b178d4361 100644 --- a/src/local-agent-opencode-version.test.ts +++ b/src/local-agent-opencode-version.test.ts @@ -1,5 +1,5 @@ import assert from "node:assert/strict"; -import { chmodSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import test from "node:test"; @@ -10,6 +10,7 @@ import { encodeOpenCodeV2SessionId, requireOpenCodeV1NativeSessionId, requireOpenCodeV2NativeSessionId, + runOpenCodeModels, } from "./local-agent-opencode-version.js"; test("CLI version output is classified into a protocol generation", () => { @@ -66,3 +67,24 @@ test( server.close(); }, ); + +test("opencode models stops a flooding process and reports only after it exits", { skip: process.platform === "win32" }, async () => { + const root = mkdtempSync(join(tmpdir(), "devspace-opencode-models-")); + const command = join(root, "opencode"); + const pidFile = join(root, "pid"); + + try { + // Ignores SIGTERM and prints past the 4 MiB cap, so only the SIGKILL escalation can end it. + writeFileSync(command, `#!/bin/sh\necho $$ > "${pidFile}"\ntrap '' TERM\nsleep 30 &\necho $! >> "${pidFile}"\nhead -c 5000000 /dev/zero | tr '\\0' x\nwait\n`); + chmodSync(command, 0o755); + + await assert.rejects(runOpenCodeModels({ ...process.env, OPENCODE_COMMAND: command }, root), /more than 4 MiB/); + + // The command and a process it started (`sleep`) are both gone once the call rejects. + for (const pid of readFileSync(pidFile, "utf8").trim().split("\n").map(Number)) { + assert.throws(() => process.kill(pid, 0), `process ${pid} is gone once the call rejects`); + } + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); diff --git a/src/local-agent-opencode-version.ts b/src/local-agent-opencode-version.ts index ccb1410a4..4145f1559 100644 --- a/src/local-agent-opencode-version.ts +++ b/src/local-agent-opencode-version.ts @@ -1,4 +1,5 @@ import { createRequire } from "node:module"; +import { terminateProcessTree } from "./process-platform.js"; import { AgentProviderProtocolError, AgentProviderUnavailableError, @@ -154,11 +155,14 @@ const OPENCODE_MODELS_KILL_GRACE_MS = 2_000; /** Runs `opencode models` through cross-spawn, so a Windows `.cmd` shim launches like the server does. */ export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise { return new Promise((resolve, reject) => { - const child = spawn(resolveOpenCodeCommand(env), ["models"], { cwd, env, stdio: ["ignore", "pipe", "pipe"] }); + // Its own process group, so a stop reaches anything `opencode models` started, not just the command itself. + const detached = process.platform !== "win32"; + const child = spawn(resolveOpenCodeCommand(env), ["models"], { cwd, env, detached, stdio: ["ignore", "pipe", "pipe"] }); let stdout = ""; let stderr = ""; let bytes = 0; let settled = false; + let stopError: Error | undefined; const finish = (error?: Error) => { if (settled) return; @@ -169,11 +173,16 @@ export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise< else resolve(stdout); }; - // A child that ignores SIGTERM is killed outright, so a stuck listing never outlives the command. + // A child that ignores SIGTERM is killed outright, and the command reports the failure only once the child is + // gone (or, if it never closes, after the kill), so a stuck listing never outlives the command. const stop = (error: Error) => { - child.kill("SIGTERM"); - setTimeout(() => { if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL"); }, OPENCODE_MODELS_KILL_GRACE_MS).unref(); - finish(error); + if (stopError || settled) return; + stopError = error; + terminateProcessTree(child, "SIGTERM", detached); + setTimeout(() => { + terminateProcessTree(child, "SIGKILL", detached); + setTimeout(() => finish(error), OPENCODE_MODELS_KILL_GRACE_MS).unref(); + }, OPENCODE_MODELS_KILL_GRACE_MS).unref(); }; const timer = setTimeout(() => { @@ -183,6 +192,8 @@ export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise< const collect = (chunk: Buffer | string, into: "stdout" | "stderr") => { bytes += Buffer.byteLength(chunk); + if (stopError) return; + if (bytes > OPENCODE_MODELS_MAX_BYTES) { stop(new Error(`opencode models printed more than ${OPENCODE_MODELS_MAX_BYTES / 1024 / 1024} MiB.`)); @@ -199,7 +210,7 @@ export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise< child.once("error", (cause) => finish(cause)); // `close` waits for stdout/stderr to drain; `exit` can fire before the list is read. child.once("close", (code) => finish( - code === 0 ? undefined : new Error(stderr.trim() || `opencode models exited with code ${code ?? 1}.`), + stopError ?? (code === 0 ? undefined : new Error(stderr.trim() || `opencode models exited with code ${code ?? 1}.`)), )); }); } From f9db0eb6a88499994627525c5bd62288537c594a Mon Sep 17 00:00:00 2001 From: Waishnav <86405648+Waishnav@users.noreply.github.com> Date: Fri, 30 Oct 2026 18:30:57 +0530 Subject: [PATCH 4/5] fix(opencode): decode model listing output as streaming UTF-8 Co-Authored-By: Claude Opus 5.5 --- src/local-agent-opencode-version.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/local-agent-opencode-version.ts b/src/local-agent-opencode-version.ts index 4145f1559..ce8d7668e 100644 --- a/src/local-agent-opencode-version.ts +++ b/src/local-agent-opencode-version.ts @@ -205,6 +205,9 @@ export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise< }; timer.unref(); + // Decode across chunk boundaries so a multibyte character split between chunks stays intact in model ids. + child.stdout?.setEncoding("utf8"); + child.stderr?.setEncoding("utf8"); child.stdout?.on("data", (chunk: Buffer | string) => collect(chunk, "stdout")); child.stderr?.on("data", (chunk: Buffer | string) => collect(chunk, "stderr")); child.once("error", (cause) => finish(cause)); From d0bb235d4bde3f7c367f80b022c913418cd0edce Mon Sep 17 00:00:00 2001 From: Waishnav <86405648+Waishnav@users.noreply.github.com> Date: Fri, 30 Oct 2026 18:47:51 +0530 Subject: [PATCH 5/5] fix(opencode): forward Ctrl-C to the detached model listing The listing runs in its own process group, so a terminal interrupt did not reach it. Forward SIGINT and SIGTERM to its stop path while it runs, and keep the SIGKILL escalation timer referenced. Co-Authored-By: Claude Opus 5.5 --- src/local-agent-opencode-version.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/local-agent-opencode-version.ts b/src/local-agent-opencode-version.ts index ce8d7668e..2cfb824d6 100644 --- a/src/local-agent-opencode-version.ts +++ b/src/local-agent-opencode-version.ts @@ -168,6 +168,8 @@ export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise< if (settled) return; settled = true; clearTimeout(timer); + process.off("SIGINT", interrupt); + process.off("SIGTERM", interrupt); if (error) reject(error); else resolve(stdout); @@ -179,12 +181,19 @@ export function runOpenCodeModels(env: NodeJS.ProcessEnv, cwd: string): Promise< if (stopError || settled) return; stopError = error; terminateProcessTree(child, "SIGTERM", detached); + // Kept referenced: the escalation must run even if nothing else holds the process open. setTimeout(() => { terminateProcessTree(child, "SIGKILL", detached); - setTimeout(() => finish(error), OPENCODE_MODELS_KILL_GRACE_MS).unref(); - }, OPENCODE_MODELS_KILL_GRACE_MS).unref(); + setTimeout(() => finish(error), OPENCODE_MODELS_KILL_GRACE_MS); + }, OPENCODE_MODELS_KILL_GRACE_MS); }; + // The listing has its own process group, so a terminal Ctrl-C does not reach it: forward it while it runs. + const interrupt = (signal: NodeJS.Signals) => stop(new Error(`opencode models was interrupted by ${signal}.`)); + + process.once("SIGINT", interrupt); + process.once("SIGTERM", interrupt); + const timer = setTimeout(() => { stop(new Error(`opencode models did not finish within ${OPENCODE_MODELS_TIMEOUT_MS / 1000}s.`)); }, OPENCODE_MODELS_TIMEOUT_MS);