fix: address review findings on workflow skill docs and agents models

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}`,
   });

skills/workflows/reference.md

@@ -113,8 +113,13 @@ For proven multi-step engineering loops (plan, implement, review, QA, bake-offs,
 
 `devspace workflow run --resume <id>` 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.

src/cli.ts

@@ -718,14 +718,22 @@ async function runAgentsModels(args: string[], json: boolean): Promise<void> {
   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<typeof providerErrorDetail>[0]) => {

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<LocalAgentModel[]> {
-    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);
   }

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<T>(promise: Promise<T>, timeoutMs: number, message: string): Promise<T> {
+  let timer: NodeJS.Timeout | undefined;
+
+  try {
+    return await Promise.race([
+      promise,
+      new Promise<never>((_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);

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]",
+  );
+});

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 <credential>` (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]");

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 });
+  }
+});

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<string> {
     });
   });
 }
+
+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<string> {
+  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}.`)),
+    ));
+  });
+}

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())

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<WireInput> {
+    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();