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..6c7d77df4 100644 --- a/src/local-agent-events.test.ts +++ b/src/local-agent-events.test.ts @@ -48,3 +48,25 @@ 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", + ); +}); + +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 1a5cf2c54..db41379bd 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; @@ -142,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; @@ -154,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.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 37a2906f6..2cfb824d6 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, @@ -146,3 +147,82 @@ 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 { + return new Promise((resolve, reject) => { + // 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; + settled = true; + clearTimeout(timer); + process.off("SIGINT", interrupt); + process.off("SIGTERM", interrupt); + + if (error) reject(error); + else resolve(stdout); + }; + + // 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) => { + 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); + }, 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); + + 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.`)); + + return; + } + + if (into === "stdout") stdout += chunk.toString(); + else stderr += chunk.toString(); + }; + + 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)); + // `close` waits for stdout/stderr to drain; `exit` can fire before the list is read. + child.once("close", (code) => finish( + stopError ?? (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}`); } }