fix(providers/codex): fresh AbortController per retry attempt (#1266) (#1371)

* fix(providers/codex): create a fresh AbortController per retry attempt

Fixes #1266. The codex provider's retry loop reused the caller's
AbortSignal across every attempt. When attempt N's Codex subprocess
crashes, Node's `spawn({ signal })` linkage aborts the shared signal
as part of SIGTERM'ing the dying child. On attempt N+1, `runStreamed`
passes that same (already-aborted) signal into the next `spawn`,
which SIGTERMs the freshly-spawned child before it reads any input.
The "Reading prompt from stdin..." line in the resulting error is
Codex CLI's normal startup banner, not a crash locus.

The fix: signal assignment moves out of buildTurnOptions and into
the retry loop. Each attempt gets a brand-new AbortController; the
caller's signal (if provided) is chained in via a once-listener so
cancellation still propagates. A try/finally removes the listener
and aborts the per-attempt controller once the attempt terminates.

Regression tests:

- `retry after crash receives a fresh (non-aborted) AbortSignal`
  captures the signal passed to `runStreamed` at call-time (not by
  mock.calls reference, which would see the mutated .signal after
  reassignment) and asserts attempt 1 got a distinct, non-aborted
  signal.
- `caller abort forwards into the active per-attempt signal` aborts
  the caller mid-attempt and asserts the per-attempt signal observes
  it.
- Two existing tests updated: `buildTurnOptions` no longer attaches
  the caller's signal, so both "passes signal in TurnOptions" tests
  now assert presence of an AbortSignal without identity-equality
  against the caller's.

Without the fix, these four tests fail and the rest pass (47/51).
With the fix, all 51 pass.

Out of scope: the binary HTTP timeout class-B path in the issue.

* fix(providers/codex): synchronous abort check at stream entry, plus review-pass fixes

`streamCodexEvents` now checks `abortSignal?.aborted` before entering
the `for await` so a caller abort that lands between attempt setup and
the first event surfaces immediately instead of waiting on the next
event or end-of-stream. The existing between-events check is retained.

Also from the same review pass:
- Pi shim: wrap `mkdirSync`/`writeFileSync` in try/catch so EACCES/
  ENOSPC surfaces as a classified "Pi shim setup failed at <dir>"
  instead of a raw node:fs error.
- Codex retry path: `getLog().debug` before throwing the
  model-access error from a retry-attempt `startThread`; the outer
  query_error log only runs for retryable errors.
- Docs: ai-assistants.md and configuration.md updated for Claude
  `~/.local/bin/claude` autodetect; ai-assistants.md gains a Codex
  autodetect bullet listing the five probed paths.
- Tests: `homedir()` instead of `process.env.HOME ?? '/Users/test'`
  to match the implementation; Windows autodetect probe covered;
  config-over-autodetect precedence covered.

Author: truffle-dev

packages/docs-web/src/content/docs/getting-started/ai-assistants.md

@@ -43,7 +43,7 @@ See [Anthropic's setup guide](https://code.claude.com/docs/en/setup) for the ful
 
 ### Binary path configuration (compiled binaries only)
 
-Compiled Archon binaries cannot auto-discover Claude Code at runtime. Supply the path via either:
+In compiled Archon binaries, if `claude` is not on the default install path Archon autodetects, supply the path via either:
 
 1. **Environment variable** (highest precedence):
    ```ini
@@ -55,8 +55,9 @@ Compiled Archon binaries cannot auto-discover Claude Code at runtime. Supply the
      claude:
        claudeBinaryPath: /absolute/path/to/claude
    ```
+3. **Autodetect** (zero-config fallback): Archon probes `~/.local/bin/claude` (POSIX) and `%USERPROFILE%\.local\bin\claude.exe` (Windows), matching the native curl/PowerShell installer layouts.
 
-If neither is set in a compiled binary, Archon throws with install instructions on first Claude query.
+If none of the three resolves in a compiled binary, Archon throws with install instructions on first Claude query.
 
 The Claude Agent SDK accepts either the native compiled binary or a JS `cli.js`.
 
@@ -173,6 +174,7 @@ In compiled Archon binaries, if `codex` is not on the default PATH Archon expect
        codexBinaryPath: /absolute/path/to/codex
    ```
 3. **Vendor directory** (zero-config fallback): drop the native binary at `~/.archon/vendor/codex/codex` (or `codex.exe` on Windows).
+4. **Autodetect** (zero-config fallback): if the vendor directory is empty, Archon probes the common npm-global install layouts: `~/.npm-global/bin/codex` (POSIX), `/opt/homebrew/bin/codex` (macOS Apple Silicon), `/usr/local/bin/codex` (macOS Intel and Linux), `%APPDATA%\npm\codex.cmd` and `%USERPROFILE%\.npm-global\codex.cmd` (Windows). For other npm prefixes or custom layouts, set `CODEX_BIN_PATH` or the config path explicitly.
 
 Dev mode (`bun run`) does not require any of the above — the SDK resolves `codex` via `node_modules`.
 

packages/docs-web/src/content/docs/getting-started/configuration.md

@@ -14,7 +14,7 @@ Set these in your shell or `.env` file:
 
 | Variable | Required | Description |
 |----------|----------|-------------|
-| `CLAUDE_BIN_PATH` | Yes (binary builds) | Absolute path to the Claude Code SDK's `cli.js`. Required in compiled Archon binaries unless `assistants.claude.claudeBinaryPath` is set. Dev mode (`bun run`) auto-resolves via `node_modules`. |
+| `CLAUDE_BIN_PATH` | No (binary builds autodetect `~/.local/bin/claude`) | Absolute path to the Claude Code binary or SDK `cli.js`. Overrides autodetection in compiled Archon binaries. Falls back to `assistants.claude.claudeBinaryPath`, then to the native-installer path. Dev mode (`bun run`) auto-resolves via `node_modules`. |
 | `CLAUDE_USE_GLOBAL_AUTH` | No | Set to `true` to use credentials from `claude /login` (default when no other Claude token is set) |
 | `CLAUDE_CODE_OAUTH_TOKEN` | No | OAuth token from `claude setup-token` (alternative to global auth) |
 | `CLAUDE_API_KEY` | No | Anthropic API key for pay-per-use (alternative to global auth) |

packages/providers/src/codex/binary-resolver.test.ts

@@ -4,6 +4,8 @@
  * Must run in its own bun test invocation because it mocks @archon/paths
  * with BUNDLED_IS_BINARY=true, which conflicts with other test files.
  */
+import { homedir } from 'node:os';
+
 import { describe, test, expect, mock, beforeEach, afterAll, spyOn } from 'bun:test';
 import { createMockLogger } from '../test/mocks/logger';
 
@@ -89,8 +91,7 @@ describe('resolveCodexBinaryPath (binary mode)', () => {
 
   test('autodetects npm global install at ~/.npm-global/bin/codex (POSIX)', async () => {
     if (process.platform === 'win32') return; // POSIX-only probe
-    const home = process.env.HOME ?? '/Users/test';
-    const expected = `${home}/.npm-global/bin/codex`;
+    const expected = `${homedir()}/.npm-global/bin/codex`;
     fileExistsSpy = spyOn(resolver, 'fileExists').mockImplementation(
       (path: string) => path === expected
     );
@@ -103,6 +104,33 @@ describe('resolveCodexBinaryPath (binary mode)', () => {
     );
   });
 
+  test('autodetects npm global install at %APPDATA%\\npm\\codex.cmd (Windows)', async () => {
+    if (process.platform !== 'win32') return; // Windows-only probe
+    const appData = process.env.APPDATA ?? 'C:\\Users\\test\\AppData\\Roaming';
+    const expected = `${appData}\\npm\\codex.cmd`;
+    fileExistsSpy = spyOn(resolver, 'fileExists').mockImplementation(
+      (path: string) => path === expected
+    );
+
+    const result = await resolver.resolveCodexBinaryPath();
+    expect(result).toBe(expected);
+    expect(mockLogger.info).toHaveBeenCalledWith(
+      { binaryPath: expected, source: 'autodetect' },
+      'codex.binary_resolved'
+    );
+  });
+
+  test('config codexBinaryPath takes precedence over autodetect', async () => {
+    // Both the explicit config path AND a typical autodetect path are
+    // present on disk; config must win. Mirrors the env-over-config and
+    // env-over-autodetect tests above so the four-tier precedence
+    // (env → config → vendor → autodetect) is fully covered.
+    fileExistsSpy = spyOn(resolver, 'fileExists').mockReturnValue(true);
+
+    const result = await resolver.resolveCodexBinaryPath('/explicit/config/codex');
+    expect(result).toBe('/explicit/config/codex');
+  });
+
   test('autodetects homebrew install on Apple Silicon', async () => {
     if (process.platform !== 'darwin' || process.arch !== 'arm64') {
       // `/opt/homebrew/bin/codex` is only probed on darwin-arm64; on other

packages/providers/src/codex/provider.test.ts

@@ -686,7 +686,7 @@ describe('CodexProvider', () => {
       );
     });
 
-    test('passes abortSignal as signal in TurnOptions', async () => {
+    test('passes a per-attempt AbortSignal in TurnOptions when caller provides one', async () => {
       mockRunStreamed.mockResolvedValue({
         events: (async function* () {
           yield { type: 'turn.completed', usage: defaultUsage };
@@ -702,13 +702,16 @@ describe('CodexProvider', () => {
         chunks.push(chunk);
       }
 
-      expect(mockRunStreamed).toHaveBeenCalledWith(
-        'test prompt',
-        expect.objectContaining({ signal: controller.signal })
-      );
+      // Signal passed to runStreamed is the per-attempt signal, not the
+      // caller's signal directly. Aborting the caller still propagates via
+      // the forwarding once-listener (covered by separate tests below).
+      const call = mockRunStreamed.mock.calls[0];
+      expect(call[0]).toBe('test prompt');
+      expect(call[1].signal).toBeInstanceOf(AbortSignal);
+      expect(call[1].signal).not.toBe(controller.signal);
     });
 
-    test('passes empty TurnOptions when no outputFormat or abortSignal', async () => {
+    test('passes a per-attempt AbortSignal in TurnOptions even when caller provides none', async () => {
       mockRunStreamed.mockResolvedValue({
         events: (async function* () {
           yield { type: 'turn.completed', usage: defaultUsage };
@@ -720,7 +723,10 @@ describe('CodexProvider', () => {
         chunks.push(chunk);
       }
 
-      expect(mockRunStreamed).toHaveBeenCalledWith('test prompt', {});
+      expect(mockRunStreamed).toHaveBeenCalledWith(
+        'test prompt',
+        expect.objectContaining({ signal: expect.any(AbortSignal) })
+      );
     });
 
     test('creates a per-call Codex instance when env is provided', async () => {
@@ -1605,4 +1611,97 @@ describe('sendQuery decomposition behaviors', () => {
     expect(systemChunks.length).toBeGreaterThanOrEqual(1);
     expect(systemChunks.some(c => c.type === 'system' && c.content.includes('Task 1'))).toBe(true);
   }, 5_000);
+
+  // Regression for issue #1266 (crash class A).
+  // Before the fix, buildTurnOptions captured the caller's abortSignal once
+  // before the retry loop, and the same signal object was passed to every
+  // runStreamed attempt. Node.js aborts the spawn-linked signal when a
+  // subprocess crashes, so attempt N's crash left `turnOptions.signal`
+  // already aborted, and attempt N+1 was SIGTERM'd before it could read the
+  // prompt. The fix creates a fresh AbortController per attempt and chains
+  // the caller's signal through a once-listener.
+  test('retry after crash receives a fresh (non-aborted) AbortSignal', async () => {
+    // Capture signals at call-time. Inspecting mockRunStreamed.mock.calls
+    // after the fact reads from a shared turnOptions reference whose .signal
+    // has since been rewritten; that's fine for the implementation (each
+    // spawn() captures the signal at its own call) but misleading here.
+    const signalsAtCallTime: Array<{ signal: AbortSignal; aborted: boolean }> = [];
+    let callCount = 0;
+    mockRunStreamed.mockImplementation((_prompt: unknown, opts: { signal?: AbortSignal }) => {
+      const s = opts.signal!;
+      signalsAtCallTime.push({ signal: s, aborted: s.aborted });
+      callCount++;
+      if (callCount === 1) {
+        return Promise.reject(new Error('codex exec crashed'));
+      }
+      return Promise.resolve({
+        events: (async function* () {
+          yield {
+            type: 'item.completed',
+            item: { type: 'agent_message', text: 'recovered', id: 'r' },
+          };
+          yield { type: 'turn.completed', usage: defaultUsage };
+        })(),
+      });
+    });
+
+    const callerController = new AbortController();
+
+    const chunks = [];
+    for await (const chunk of client.sendQuery('test', '/workspace', undefined, {
+      abortSignal: callerController.signal,
+    })) {
+      chunks.push(chunk);
+    }
+
+    expect(mockRunStreamed).toHaveBeenCalledTimes(2);
+    expect(signalsAtCallTime).toHaveLength(2);
+    // Distinct signal objects per attempt.
+    expect(signalsAtCallTime[1].signal).not.toBe(signalsAtCallTime[0].signal);
+    // Attempt 1's signal was NOT aborted at the moment of spawn, even
+    // though attempt 0 crashed. This is the exact property that was
+    // broken in the old implementation.
+    expect(signalsAtCallTime[1].aborted).toBe(false);
+    // Caller signal was never aborted.
+    expect(callerController.signal.aborted).toBe(false);
+  }, 5_000);
+
+  test('caller abort forwards into the active per-attempt signal', async () => {
+    const callerController = new AbortController();
+
+    let capturedSignal: AbortSignal | undefined;
+    mockRunStreamed.mockImplementation((_prompt, opts: { signal?: AbortSignal }) => {
+      capturedSignal = opts.signal;
+      return Promise.resolve({
+        events: (async function* () {
+          yield {
+            type: 'item.completed',
+            item: { type: 'agent_message', text: 'partial', id: '1' },
+          };
+          // Caller aborts mid-stream; this must surface on the per-attempt signal.
+          callerController.abort();
+          yield {
+            type: 'item.completed',
+            item: { type: 'agent_message', text: 'should not appear', id: '2' },
+          };
+          yield { type: 'turn.completed', usage: defaultUsage };
+        })(),
+      });
+    });
+
+    const consumeGenerator = async (): Promise<void> => {
+      for await (const _ of client.sendQuery('test', '/workspace', undefined, {
+        abortSignal: callerController.signal,
+      })) {
+        // consume
+      }
+    };
+
+    await expect(consumeGenerator()).rejects.toThrow('Query aborted');
+    // The signal observed by runStreamed is the per-attempt one, and it
+    // reflects the caller's abort via the forwarding listener.
+    expect(capturedSignal).toBeDefined();
+    expect(capturedSignal).not.toBe(callerController.signal);
+    expect(capturedSignal?.aborted).toBe(true);
+  }, 5_000);
 });

packages/providers/src/codex/provider.ts

@@ -287,9 +287,10 @@ function buildTurnOptions(requestOptions?: SendQueryOptions): {
   if (requestOptions?.nodeConfig?.output_format && !requestOptions?.outputFormat) {
     turnOptions.outputSchema = requestOptions.nodeConfig.output_format;
   }
-  if (requestOptions?.abortSignal) {
-    turnOptions.signal = requestOptions.abortSignal;
-  }
+  // Signal assignment is intentionally per-attempt (in sendQuery's retry
+  // loop), not here. Reusing a single AbortSignal across retries can poison
+  // later attempts once any earlier attempt's subprocess is SIGTERM'd.
+  // See issue #1266.
   return { turnOptions, hasOutputFormat };
 }
 
@@ -314,6 +315,11 @@ async function* streamCodexEvents(
   const state: CodexStreamState = {};
   let accumulatedText = '';
 
+  if (abortSignal?.aborted) {
+    getLog().info('query_aborted_before_stream');
+    throw new Error('Query aborted');
+  }
+
   // If the iterator closes without a terminal event (e.g. the model was
   // rejected before the turn even started), we synthesize a fail-stop result
   // after the loop so the dag-executor's `msg.isError` branch catches it
@@ -760,57 +766,86 @@ export class CodexProvider implements IAgentProvider {
         throw new Error('Query aborted');
       }
 
-      if (attempt > 0) {
-        getLog().debug({ cwd, attempt }, 'starting_new_thread');
-        try {
-          thread = codex.startThread(threadOptions);
-        } catch (startError) {
-          const err = startError as Error;
-          if (isModelAccessError(err.message)) {
-            throw new Error(buildModelAccessMessage(requestOptions?.model));
-          }
-          throw new Error(`Codex query failed: ${err.message}`);
-        }
+      // Fresh AbortController per attempt. Caller's abortSignal, if any, is
+      // chained in via a once-listener so cancellation still propagates.
+      // Without this, a signal aborted during attempt N (e.g. when the
+      // Codex subprocess crashes and Node.js reacts to the `spawn({ signal })`
+      // linkage) would wire an already-aborted signal into attempt N+1's
+      // `spawn`, SIGTERMing the freshly spawned child before it reads any
+      // input. The "Reading prompt from stdin..." in the resulting error is
+      // Codex CLI's startup banner, not an indicator of crash location.
+      // See issue #1266.
+      const attemptController = new AbortController();
+      const onCallerAbort = (): void => {
+        attemptController.abort();
+      };
+      if (requestOptions?.abortSignal) {
+        requestOptions.abortSignal.addEventListener('abort', onCallerAbort, { once: true });
       }
+      turnOptions.signal = attemptController.signal;
 
       try {
-        // 4. Run streamed turn
-        const result = await thread.runStreamed(prompt, turnOptions);
-
-        // 5. Stream normalized events (fresh state per attempt to avoid dedup leaks)
-        yield* streamCodexEvents(
-          result.events as AsyncIterable<Record<string, unknown>>,
-          hasOutputFormat,
-          thread.id,
-          requestOptions?.abortSignal,
-          Boolean(requestOptions?.nodeConfig?.mcp)
-        );
-        return;
-      } catch (error) {
-        const err = error as Error;
-
-        if (requestOptions?.abortSignal?.aborted) {
-          throw new Error('Query aborted');
+        if (attempt > 0) {
+          getLog().debug({ cwd, attempt }, 'starting_new_thread');
+          try {
+            thread = codex.startThread(threadOptions);
+          } catch (startError) {
+            const err = startError as Error;
+            if (isModelAccessError(err.message)) {
+              getLog().debug({ attempt, errorClass: 'model_access' }, 'query_error_pre_retry');
+              throw new Error(buildModelAccessMessage(requestOptions?.model));
+            }
+            throw new Error(`Codex query failed: ${err.message}`);
+          }
         }
 
-        const { enrichedError, errorClass, shouldRetry } = classifyAndEnrichCodexError(
-          err,
-          requestOptions?.model
-        );
+        try {
+          // 4. Run streamed turn
+          const result = await thread.runStreamed(prompt, turnOptions);
+
+          // 5. Stream normalized events (fresh state per attempt to avoid dedup leaks)
+          yield* streamCodexEvents(
+            result.events as AsyncIterable<Record<string, unknown>>,
+            hasOutputFormat,
+            thread.id,
+            attemptController.signal,
+            Boolean(requestOptions?.nodeConfig?.mcp)
+          );
+          return;
+        } catch (error) {
+          const err = error as Error;
 
-        getLog().error(
-          { err, errorClass, attempt, maxRetries: MAX_SUBPROCESS_RETRIES },
-          'query_error'
-        );
+          if (requestOptions?.abortSignal?.aborted) {
+            throw new Error('Query aborted');
+          }
 
-        if (!shouldRetry || attempt >= MAX_SUBPROCESS_RETRIES) {
-          throw enrichedError;
-        }
+          const { enrichedError, errorClass, shouldRetry } = classifyAndEnrichCodexError(
+            err,
+            requestOptions?.model
+          );
+
+          getLog().error(
+            { err, errorClass, attempt, maxRetries: MAX_SUBPROCESS_RETRIES },
+            'query_error'
+          );
 
-        const delayMs = this.retryBaseDelayMs * Math.pow(2, attempt);
-        getLog().info({ attempt, delayMs, errorClass }, 'retrying_query');
-        await new Promise(resolve => setTimeout(resolve, delayMs));
-        lastError = enrichedError;
+          if (!shouldRetry || attempt >= MAX_SUBPROCESS_RETRIES) {
+            throw enrichedError;
+          }
+
+          const delayMs = this.retryBaseDelayMs * Math.pow(2, attempt);
+          getLog().info({ attempt, delayMs, errorClass }, 'retrying_query');
+          await new Promise(resolve => setTimeout(resolve, delayMs));
+          lastError = enrichedError;
+        }
+      } finally {
+        if (requestOptions?.abortSignal) {
+          requestOptions.abortSignal.removeEventListener('abort', onCallerAbort);
+        }
+        // Signal to any downstream consumers that this attempt is done.
+        // Next iteration creates a fresh controller; caller's signal state
+        // is unchanged.
+        attemptController.abort();
       }
     }