diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index 44e4366e..03bfa6ab 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -29,6 +29,7 @@ import { isNoActiveTurnError, parseExpectedActiveTurnMismatch, } from "./CodexThreadErrors"; +import {requiresLogout} from "./CodexErrorExplanations"; import {type CodexConnection} from "./CodexJsonRpcConnection"; import {AppServerRecovery, recoveryLimitsFromEnv} from "./app-server-recovery/AppServerRecovery"; import { @@ -933,15 +934,11 @@ export class CodexAcpServer { } async handleError(e: Error){ - if (e.message.includes("log out") || e.message.includes("cloud requirements")) { + if (requiresLogout(e)) { await this.runWithProcessCheck(() => this.codexAcpClient.logout()); await this.refreshAuthState(null); throw RequestError.internalError(`${(e.message)}\n\nYou have been logged out. Please try again.`); } - const configPath = this.codexAcpClient.getHomePath() ?? "global"; - if (e.message.includes("load config")) { - throw RequestError.internalError(`${e.message}\n\nCheck ${configPath} and project .codex directories, especially their config.toml files, or any CODEX_CONFIG override.`); - } } private beginSessionOpen(sessionId: string): number { diff --git a/src/CodexAppServerClient.ts b/src/CodexAppServerClient.ts index d7977d90..21033105 100644 --- a/src/CodexAppServerClient.ts +++ b/src/CodexAppServerClient.ts @@ -1,5 +1,6 @@ import {type MessageConnection, RequestType} from "vscode-jsonrpc/node"; import {AppServerConnectionLostError, ConnectionLoss} from "./app-server-recovery/ConnectionLoss"; +import {explainCodexError} from "./CodexErrorExplanations"; import {McpOauthCompletions} from "./mcp/McpOauthCompletions"; import {McpStartupTracker} from "./mcp/McpStartupTracker"; import type { @@ -185,6 +186,8 @@ export class CodexAppServerClient { private readonly leakedReviewErrors = new Map}>(); /** Ends the notification waits below when the connection is gone, see {@link waitWhileConnected}. */ readonly connectionLoss: ConnectionLoss; + /** The `$CODEX_HOME` of the app-server, for the error explanations. */ + private codexHome: string | null = null; constructor(connection: MessageConnection) { this.connection = connection; @@ -304,7 +307,9 @@ export class CodexAppServerClient { } async initialize(params: InitializeParams): Promise { - return await this.sendRequest({ method: "initialize", params: params }); + const response: InitializeResponse = await this.sendRequest({ method: "initialize", params: params }); + this.codexHome = response?.codexHome ?? null; + return response; } async turnStart(params: TurnStartParams): Promise { @@ -1281,15 +1286,19 @@ export class CodexAppServerClient { } const loadedThreadId = threadLoadOf(request); let result: any; - if (loadedThreadId === null) { - if (request.params) { - result = await this.connection.sendRequest(request.method, request.params) - } - else { - result = await this.connection.sendRequest(request.method); + try { + if (loadedThreadId === null) { + if (request.params) { + result = await this.connection.sendRequest(request.method, request.params) + } + else { + result = await this.connection.sendRequest(request.method); + } + } else { + result = await this.threadLoadGate(loadedThreadId, () => this.sendThreadLoad(loadedThreadId, request)); } - } else { - result = await this.threadLoadGate(loadedThreadId, () => this.sendThreadLoad(loadedThreadId, request)); + } catch (err) { + throw err instanceof Error ? explainCodexError(err, {codexHome: this.codexHome}) : err; } for (const callback of this.codexEventHandlers) { callback({ eventType: "response", ...result}); diff --git a/src/CodexErrorExplanations.ts b/src/CodexErrorExplanations.ts new file mode 100644 index 00000000..5f3baf7f --- /dev/null +++ b/src/CodexErrorExplanations.ts @@ -0,0 +1,91 @@ +/** + * Adds the likely cause to app-server errors whose text is too generic to act on. + * + * Every app-server request error passes through {@link explainCodexError}. A rule only rewrites the message, + * and the code and data stay. Other code matches the message text too (`CodexThreadErrors`), so a rule must not + * match the errors that such code recognizes. + */ + +import {statSync} from "node:fs"; +import {ResponseError} from "vscode-jsonrpc/node"; + +export interface ExplanationContext { + /** The `$CODEX_HOME` of the app-server, from its `initialize` answer. */ + readonly codexHome: string | null; +} + +/** Returns the error with the likely cause in its message, or the same error when no rule applies. */ +export function explainCodexError(err: Error, context: ExplanationContext): Error { + for (const rule of RULES) { + const message = rule(err, context); + if (message !== null) return withMessage(err, message); + } + return err; +} + +/** True for an error that a new login fixes, so the agent logs out. */ +export function requiresLogout(err: Error): boolean { + return err.message.includes("log out") || err.message.includes("cloud requirements"); +} + +/** Returns the new message, or `null` when the rule does not apply. */ +type ExplanationRule = (err: Error, context: ExplanationContext) => string | null; + +/** + * `config/read` wraps every config load failure as "failed to resolve feature override precedence" + * (codex-rs/app-server `config_processor.rs`), and `thread/start` wraps it as "failed to load configuration". + */ +const FEATURE_OVERRIDE_PRECEDENCE_PREFIX = "failed to resolve feature override precedence: "; +const CONFIG_LOAD_FAILURE_PREFIX = "failed to load configuration: "; + +/** + * Codex loads the project config from its process working directory, even when the request has a `cwd`. The + * app-server inherits that directory from this process, so a deleted directory fails every config load with a bare + * "No such file or directory". The config files are not the cause, so the message does not point to them. + */ +const deletedWorkingDirectory: ExplanationRule = ({message}) => { + const configLoadFailure = message.startsWith(FEATURE_OVERRIDE_PRECEDENCE_PREFIX) + || message.startsWith(CONFIG_LOAD_FAILURE_PREFIX); + if (!configLoadFailure || !message.includes("No such file or directory") || !workingDirectoryDeleted()) { + return null; + } + return `Codex cannot read its configuration: its working directory ${workingDirectory} ` + + "was deleted or replaced after the agent started. Restart the agent."; +}; + +/** Points a config load failure to the config files. A failure that needs a new login is not about them. */ +const configFiles: ExplanationRule = (err, {codexHome}) => { + if (requiresLogout(err)) return null; + const message = err.message.startsWith(FEATURE_OVERRIDE_PRECEDENCE_PREFIX) + ? CONFIG_LOAD_FAILURE_PREFIX + err.message.slice(FEATURE_OVERRIDE_PRECEDENCE_PREFIX.length) + : err.message; + if (!message.includes("load config")) return null; + return `${message}\n\nCheck ${codexHome ?? "global"} and project .codex directories, ` + + "especially their config.toml files, or any CODEX_CONFIG override."; +}; + +const RULES: readonly ExplanationRule[] = [deletedWorkingDirectory, configFiles]; + +function withMessage(err: Error, message: string): Error { + return err instanceof ResponseError ? new ResponseError(err.code, message, err.data) : new Error(message, {cause: err}); +} + +// Read at start: the agent never changes it, and `process.cwd()` can fail once the directory is deleted. +const workingDirectory = (() => { + try { + return process.cwd(); + } catch { + return ""; + } +})(); + +/** A directory created again at the same path is a new one: the process still points to the deleted directory. */ +function workingDirectoryDeleted(): boolean { + try { + const current = statSync("."); + const atPath = statSync(workingDirectory); + return current.ino !== atPath.ino || current.dev !== atPath.dev; + } catch { + return true; + } +} diff --git a/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts index f9959460..04498451 100644 --- a/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts +++ b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts @@ -150,7 +150,7 @@ url = "https://example.com/mcp" cwd: "", mcpServers: [conflictingMcp], })).rejects.toMatchObject({ - data: expect.stringContaining("url is not supported for stdio"), + message: expect.stringMatching(/url is not supported for stdio[\s\S]*config\.toml/), }); }); }); diff --git a/src/__tests__/CodexACPAgent/new-session-logout.test.ts b/src/__tests__/CodexACPAgent/new-session-logout.test.ts index 4fbe6713..3d50460b 100644 --- a/src/__tests__/CodexACPAgent/new-session-logout.test.ts +++ b/src/__tests__/CodexACPAgent/new-session-logout.test.ts @@ -1,6 +1,7 @@ import {describe, expect, it, vi} from "vitest"; import {createCodexMockTestFixture, createTestModel} from "../acp-test-utils"; import {ModelId} from "../../ModelId"; +import {ResponseError, type MessageConnection} from "vscode-jsonrpc/node"; describe("New session logout handling", () => { it("logs out when newSession fails with an error containing log out", async () => { @@ -50,13 +51,18 @@ describe("New session logout handling", () => { const logoutSpy = vi.spyOn(codexAcpClient, "logout").mockResolvedValue(); const errorMessage = 'Internal error: "failed to reload config: filesystem path `/tmp` must be absolute, use `~/...`, or start with `:`"'; - vi.spyOn(codexAppServerClient, "threadStart").mockRejectedValue(new Error(errorMessage)); + // The app-server answer goes through the error explanations of the client. + const {connection} = codexAppServerClient as unknown as {connection: MessageConnection}; + vi.spyOn(connection, "sendRequest").mockImplementation((async (method: string) => { + if (method === "thread/start") throw new ResponseError(-32603, errorMessage); + return undefined; + }) as never); - expect(logoutSpy).toHaveBeenCalledTimes(0); await expect(codexAcpAgent.newSession({cwd: "", mcpServers: []})) .rejects.toMatchObject({ - data: expect.stringContaining(`Check global and project .codex directories`), + message: expect.stringContaining(`Check global and project .codex directories`), }); + expect(logoutSpy).toHaveBeenCalledTimes(0); }); it("refreshes OpenAI sessions when newSession error forces logout", async () => { diff --git a/src/__tests__/CodexErrorExplanations.test.ts b/src/__tests__/CodexErrorExplanations.test.ts new file mode 100644 index 00000000..1144349c --- /dev/null +++ b/src/__tests__/CodexErrorExplanations.test.ts @@ -0,0 +1,100 @@ +import {afterEach, describe, expect, it, vi} from "vitest"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import {ResponseError, type MessageConnection} from "vscode-jsonrpc/node"; +import {explainCodexError} from "../CodexErrorExplanations"; +import {CodexAppServerClient} from "../CodexAppServerClient"; +import {createMockConnections} from "./CodexACPAgent/test-utils"; +import {createCodexMockTestFixture} from "./acp-test-utils"; + +// The texts Codex 0.159.1 answers with when its working directory is gone (IJAI-2923). +const configReadFailure = () => new ResponseError( + -32603, "failed to resolve feature override precedence: No such file or directory (os error 2)", {extra: 1}); +const threadStartFailure = () => new ResponseError( + -32600, "failed to load configuration: No such file or directory (os error 2)"); + +const context = {codexHome: "/home/codex"}; +const originalDirectory = process.cwd(); +const directories: string[] = []; + +afterEach(() => { + process.chdir(originalDirectory); + directories.splice(0).forEach(directory => fs.rmSync(directory, {recursive: true, force: true})); +}); + +/** Loads the module as in an agent started in a new directory, then runs `change` on that directory. */ +async function explainerStartedIn(change: (directory: string) => void = () => {}) { + const directory = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "codex-cwd-"))); + directories.push(directory); + process.chdir(directory); + vi.resetModules(); + const {explainCodexError} = await import("../CodexErrorExplanations"); + change(directory); + return (err: Error) => { + const result = explainCodexError(err, context) as ResponseError; + return { + code: result.code, + message: result.message.replace(directory, ""), + data: result.data ?? null, + }; + }; +} + +describe("CodexErrorExplanations", () => { + it.skipIf(process.platform === "win32")("explains config load failures", async () => { + // Each explanation runs while the process is still in the directory of its agent. + const deleted = await explainerStartedIn(fs.rmdirSync); + const withDeletedDirectory = { + configReadWithDeletedDirectory: deleted(configReadFailure()), + threadStartWithDeletedDirectory: deleted(threadStartFailure()), + otherConfigReadFailureWithDeletedDirectory: deleted(new ResponseError( + -32603, "failed to resolve feature override precedence: invalid TOML in config.toml")), + }; + const recreated = await explainerStartedIn(directory => { + fs.rmdirSync(directory); + fs.mkdirSync(directory); + }); + const withRecreatedDirectory = {threadStartWithRecreatedDirectory: recreated(threadStartFailure())}; + const present = await explainerStartedIn(); + const results = { + ...withDeletedDirectory, + ...withRecreatedDirectory, + configReadWithPresentDirectory: present(configReadFailure()), + threadStartWithPresentDirectory: present(threadStartFailure()), + reloadFailure: present(new Error("failed to reload config: filesystem path `/tmp` must be absolute")), + reloadFailureThatNeedsLogout: present(new ResponseError(-32603, "failed to reload config: " + + "Your refresh token was already used. Please log out and sign in again.")), + }; + await expect(`${JSON.stringify(results, null, 2)}\n`) + .toMatchFileSnapshot("data/codex-error-explanations/config-load-failures.json"); + }); + + it("keeps other errors unchanged", () => { + const unrelated = new ResponseError(-32600, "thread not found: thread-1"); + expect(explainCodexError(unrelated, context)).toBe(unrelated); + }); + + it("explains the errors of app-server requests", async () => { + const mocks = createMockConnections(); + mocks.mockCodexConnection.sendRequest.mockRejectedValue(configReadFailure()); + const client = new CodexAppServerClient(mocks.mockCodexConnection as unknown as MessageConnection); + + await expect(client.configRead({includeLayers: false})).rejects.toMatchObject({ + code: -32603, + message: expect.stringMatching(/^failed to load configuration: No such file or directory \(os error 2\)\n\nCheck global /), + }); + }); + + it.skipIf(process.platform === "win32")("does not point to the config files when the working directory was deleted", async () => { + const explain = await explainerStartedIn(fs.rmdirSync); + const fixture = createCodexMockTestFixture(); + vi.spyOn(fixture.getCodexAcpClient(), "readAuthRequirement").mockResolvedValue({required: false, account: null}); + const {code, message} = explain(threadStartFailure()); + vi.spyOn(fixture.getCodexAppServerClient(), "threadStart").mockRejectedValue(new ResponseError(code, message)); + + const error = await fixture.getCodexAcpAgent().newSession({cwd: "", mcpServers: []}).catch(e => e); + expect(error.message).toMatch(/was deleted or replaced after the agent started/); + expect(error.message).not.toContain("config.toml"); + }); +}); diff --git a/src/__tests__/data/codex-error-explanations/config-load-failures.json b/src/__tests__/data/codex-error-explanations/config-load-failures.json new file mode 100644 index 00000000..fd50493a --- /dev/null +++ b/src/__tests__/data/codex-error-explanations/config-load-failures.json @@ -0,0 +1,45 @@ +{ + "configReadWithDeletedDirectory": { + "code": -32603, + "message": "Codex cannot read its configuration: its working directory was deleted or replaced after the agent started. Restart the agent.", + "data": { + "extra": 1 + } + }, + "threadStartWithDeletedDirectory": { + "code": -32600, + "message": "Codex cannot read its configuration: its working directory was deleted or replaced after the agent started. Restart the agent.", + "data": null + }, + "otherConfigReadFailureWithDeletedDirectory": { + "code": -32603, + "message": "failed to load configuration: invalid TOML in config.toml\n\nCheck /home/codex and project .codex directories, especially their config.toml files, or any CODEX_CONFIG override.", + "data": null + }, + "threadStartWithRecreatedDirectory": { + "code": -32600, + "message": "Codex cannot read its configuration: its working directory was deleted or replaced after the agent started. Restart the agent.", + "data": null + }, + "configReadWithPresentDirectory": { + "code": -32603, + "message": "failed to load configuration: No such file or directory (os error 2)\n\nCheck /home/codex and project .codex directories, especially their config.toml files, or any CODEX_CONFIG override.", + "data": { + "extra": 1 + } + }, + "threadStartWithPresentDirectory": { + "code": -32600, + "message": "failed to load configuration: No such file or directory (os error 2)\n\nCheck /home/codex and project .codex directories, especially their config.toml files, or any CODEX_CONFIG override.", + "data": null + }, + "reloadFailure": { + "message": "failed to reload config: filesystem path `/tmp` must be absolute\n\nCheck /home/codex and project .codex directories, especially their config.toml files, or any CODEX_CONFIG override.", + "data": null + }, + "reloadFailureThatNeedsLogout": { + "code": -32603, + "message": "failed to reload config: Your refresh token was already used. Please log out and sign in again.", + "data": null + } +}