fix(codex-local): skip benign stderr warnings when deriving the fallback run error (#10003)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agents execute through adapters; the codex_local adapter runs the Codex CLI and reports each run's outcome, including an error message when the CLI exits nonzero > - When no error can be parsed from the CLI's JSONL output, `toResult` in `packages/adapters/codex-local/src/server/execute.ts` falls back to the first non-empty stderr line as the run error > - The adapter itself passes the approvals-bypass flag, so the CLI's first stderr line is always the benign startup warning "YOLO mode is enabled. All tool calls will be automatically approved." > - Failed runs therefore record that warning as their error, hiding the real cause (for example an OpenAI API error further down in stderr) and making failures hard to diagnose from the run record > - This pull request derives the fallback error from the first meaningful stderr line, skipping a conservative set of known benign lines, and keeps the existing behavior when every line is benign > - The benefit is that failed Codex runs surface the actual failure reason instead of a harmless startup warning, without ever producing an emptier message than before ## Linked Issues or Issue Description No public issue exists for the codex_local case. The same bug class was fixed for gemini-local in Refs #5099 and Refs #3476; this PR applies the equivalent fix to codex_local. **What happened?** On a multi-tenant cloud deployment of Paperclip, several codex_local runs failed and their run records showed `error_code=adapter_failed` with the error text "YOLO mode is enabled. All tool calls will be automatically approved." That is a benign Codex CLI startup warning, printed on every run because the adapter passes the approvals-bypass flag itself. The real failure (an OpenAI API error printed later in stderr) was never surfaced. **Expected behavior** When the Codex CLI exits nonzero and no error was parsed from its JSONL output, the run error should be the first stderr line that actually explains the failure, not a startup warning the adapter itself provoked. **Steps to reproduce** 1. Configure a codex_local agent and make the underlying Codex CLI invocation fail after startup (for example, configure a model id the active credentials cannot use). 2. Run the agent so the CLI exits nonzero with no parsed JSONL error. 3. Inspect the run's error message: it shows the YOLO approvals warning (the first stderr line) instead of the real error printed further down in stderr. ## What Changed - Added `firstMeaningfulStderrLine` next to `firstNonEmptyLine` in `packages/adapters/codex-local/src/server/execute.ts`, with a conservative benign-line predicate covering the YOLO approvals warning and `[paperclip] ...` diagnostic lines the adapter injected (for example ACP fallback notes). - Used it only in the `toResult` fallback error derivation. If every stderr line is benign, the existing chain still applies (first non-empty line, then `Codex exited with code N`), so the message never gets emptier than today. Logging is unchanged. - Added `packages/adapters/codex-local/src/server/execute.stderr-error.test.ts`: four end-to-end cases through `execute()` with a mocked CLI process, plus unit coverage for the new helper. Tests were written first and confirmed failing before the fix. ## Verification - `pnpm exec vitest run packages/adapters/codex-local/src/server/execute.stderr-error.test.ts` (7 tests pass; 5 failed before the fix as expected) - `pnpm exec vitest run packages/adapters/codex-local` (21 files, 188 tests pass) - `pnpm run typecheck` in `packages/adapters/codex-local` (clean) ## Risks Low risk. Only the derived fallback `errorMessage` changes, and only when a benign line would otherwise have been picked; parsed JSONL errors, logging, retry/quota/auth classification inputs, and the empty-stderr exit-code fallback are untouched. The benign-line list is deliberately conservative (exact prefixes) so real errors are never skipped. ## Model Used Claude Fable 5 (claude-fable-5), extended thinking, via Claude Code ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Jannes Stubbemann
GitHub
parent
676e20a894
commit
1f7959bc69
|
|
@ -0,0 +1,186 @@
|
|||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
const YOLO_WARNING = "YOLO mode is enabled. All tool calls will be automatically approved.";
|
||||
|
||||
const {
|
||||
ensureAdapterExecutionTargetCommandResolvable,
|
||||
ensureAdapterExecutionTargetRuntimeCommandInstalled,
|
||||
prepareCodexRuntimeConfig,
|
||||
readPaperclipRuntimeSkillEntries,
|
||||
resolveAdapterExecutionTargetCommandForLogs,
|
||||
runAdapterExecutionTargetProcess,
|
||||
tempCodexHome,
|
||||
} = vi.hoisted(() => ({
|
||||
ensureAdapterExecutionTargetCommandResolvable: vi.fn(async () => undefined),
|
||||
ensureAdapterExecutionTargetRuntimeCommandInstalled: vi.fn(async () => undefined),
|
||||
prepareCodexRuntimeConfig: vi.fn(async () => ({ cleanup: vi.fn(async () => undefined), notes: [] })),
|
||||
readPaperclipRuntimeSkillEntries: vi.fn(async () => []),
|
||||
resolveAdapterExecutionTargetCommandForLogs: vi.fn(async () => "codex"),
|
||||
runAdapterExecutionTargetProcess: vi.fn(),
|
||||
tempCodexHome: "/tmp/paperclip-codex-stderr-error-test-home",
|
||||
}));
|
||||
|
||||
vi.mock("./acp.js", () => ({
|
||||
createCodexAcpExecutor: () => vi.fn(),
|
||||
formatCodexAcpFallbackMessage: (reason: string) =>
|
||||
`[paperclip] Codex ACP default unavailable; falling back to Codex CLI. ${reason} Set engine=acp to require ACP or engine=cli to silence this fallback.\n`,
|
||||
resolveCodexExecutionEngineForRun: async () => ({ engine: "cli", explicit: true }),
|
||||
}));
|
||||
|
||||
vi.mock("@paperclipai/adapter-utils/execution-target", async () => {
|
||||
const actual = await vi.importActual<typeof import("@paperclipai/adapter-utils/execution-target")>(
|
||||
"@paperclipai/adapter-utils/execution-target",
|
||||
);
|
||||
return {
|
||||
...actual,
|
||||
ensureAdapterExecutionTargetCommandResolvable,
|
||||
ensureAdapterExecutionTargetRuntimeCommandInstalled,
|
||||
resolveAdapterExecutionTargetCommandForLogs,
|
||||
runAdapterExecutionTargetProcess,
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("@paperclipai/adapter-utils/server-utils", async () => {
|
||||
const actual = await vi.importActual<typeof import("@paperclipai/adapter-utils/server-utils")>(
|
||||
"@paperclipai/adapter-utils/server-utils",
|
||||
);
|
||||
return {
|
||||
...actual,
|
||||
readPaperclipRuntimeSkillEntries,
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("./codex-home.js", async () => {
|
||||
const actual = await vi.importActual<typeof import("./codex-home.js")>("./codex-home.js");
|
||||
return {
|
||||
...actual,
|
||||
evaluateCodexCredentialReadiness: vi.fn(async () => ({
|
||||
managed: true,
|
||||
authMode: "api",
|
||||
ready: true,
|
||||
effectiveHome: tempCodexHome,
|
||||
sharedSourceHome: tempCodexHome,
|
||||
})),
|
||||
isManagedCodexHomePath: vi.fn(() => true),
|
||||
prepareManagedCodexHome: vi.fn(async () => ({ status: "seeded", home: tempCodexHome })),
|
||||
resolveManagedCodexHomeDir: vi.fn(() => tempCodexHome),
|
||||
seedManagedCodexHome: vi.fn(async () => ({ status: "seeded", home: tempCodexHome })),
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("./runtime-config.js", async () => {
|
||||
const actual = await vi.importActual<typeof import("./runtime-config.js")>("./runtime-config.js");
|
||||
return {
|
||||
...actual,
|
||||
prepareCodexRuntimeConfig,
|
||||
};
|
||||
});
|
||||
|
||||
import { execute, firstMeaningfulStderrLine } from "./execute.js";
|
||||
|
||||
function mockFailedProcess(stderr: string) {
|
||||
runAdapterExecutionTargetProcess.mockImplementation(async () => ({
|
||||
exitCode: 1,
|
||||
signal: null,
|
||||
timedOut: false,
|
||||
stdout: "",
|
||||
stderr,
|
||||
pid: 123,
|
||||
startedAt: new Date().toISOString(),
|
||||
}));
|
||||
}
|
||||
|
||||
function buildContext(config: Record<string, unknown> = {}) {
|
||||
return {
|
||||
runId: "run-1",
|
||||
agent: {
|
||||
id: "agent-1",
|
||||
companyId: "company-1",
|
||||
name: "Codex Coder",
|
||||
adapterType: "codex_local",
|
||||
adapterConfig: {},
|
||||
},
|
||||
runtime: {
|
||||
sessionId: null,
|
||||
sessionParams: null,
|
||||
sessionDisplayId: null,
|
||||
taskKey: null,
|
||||
},
|
||||
config: {
|
||||
outputInactivityTimeoutMs: null,
|
||||
env: { OPENAI_API_KEY: "test-key" },
|
||||
...config,
|
||||
},
|
||||
context: {},
|
||||
onLog: vi.fn(async () => {}),
|
||||
};
|
||||
}
|
||||
|
||||
describe("codex_local stderr fallback error derivation", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it("skips the benign YOLO approvals warning and surfaces the real stderr error", async () => {
|
||||
mockFailedProcess(
|
||||
[
|
||||
YOLO_WARNING,
|
||||
"Error: unexpected status 400 Bad Request: {\"error\":{\"message\":\"The requested model 'gpt-5.3-codex-spark' does not exist.\",\"code\":\"model_not_found\"}}",
|
||||
].join("\n"),
|
||||
);
|
||||
|
||||
const result = await execute(buildContext() as never);
|
||||
|
||||
expect(result.exitCode).toBe(1);
|
||||
expect(result.errorMessage).toContain("model_not_found");
|
||||
expect(result.errorMessage).not.toContain("YOLO mode");
|
||||
});
|
||||
|
||||
it("skips adapter-injected [paperclip] diagnostic lines when picking the fallback error", async () => {
|
||||
mockFailedProcess(
|
||||
[
|
||||
"[paperclip] Codex ACP default unavailable; falling back to Codex CLI. Set engine=acp to require ACP or engine=cli to silence this fallback.",
|
||||
YOLO_WARNING,
|
||||
"Error: stream disconnected before completion",
|
||||
].join("\n"),
|
||||
);
|
||||
|
||||
const result = await execute(buildContext() as never);
|
||||
|
||||
expect(result.errorMessage).toBe("Error: stream disconnected before completion");
|
||||
});
|
||||
|
||||
it("falls back to the first non-empty stderr line when every line is benign", async () => {
|
||||
mockFailedProcess(`${YOLO_WARNING}\n`);
|
||||
|
||||
const result = await execute(buildContext() as never);
|
||||
|
||||
expect(result.errorMessage).toBe(YOLO_WARNING);
|
||||
});
|
||||
|
||||
it("falls back to the exit-code message when stderr is empty", async () => {
|
||||
mockFailedProcess("\n \n");
|
||||
|
||||
const result = await execute(buildContext() as never);
|
||||
|
||||
expect(result.errorMessage).toBe("Codex exited with code 1");
|
||||
});
|
||||
});
|
||||
|
||||
describe("firstMeaningfulStderrLine", () => {
|
||||
it("returns the first line that is not a known benign warning", () => {
|
||||
expect(firstMeaningfulStderrLine(`${YOLO_WARNING}\nError: boom`)).toBe("Error: boom");
|
||||
expect(firstMeaningfulStderrLine("[paperclip] Confining Codex with workspace scope.\nError: boom")).toBe(
|
||||
"Error: boom",
|
||||
);
|
||||
});
|
||||
|
||||
it("keeps the first non-empty line when all lines are benign", () => {
|
||||
expect(firstMeaningfulStderrLine(`${YOLO_WARNING}\n[paperclip] note\n`)).toBe(YOLO_WARNING);
|
||||
});
|
||||
|
||||
it("returns an empty string for blank input", () => {
|
||||
expect(firstMeaningfulStderrLine("")).toBe("");
|
||||
expect(firstMeaningfulStderrLine(" \n\t\n")).toBe("");
|
||||
});
|
||||
});
|
||||
|
|
@ -134,6 +134,29 @@ function firstNonEmptyLine(text: string): string {
|
|||
);
|
||||
}
|
||||
|
||||
// Benign stderr lines that never explain a nonzero exit and must not be
|
||||
// surfaced as the run error: Codex always prints the YOLO approvals warning
|
||||
// because this adapter passes the approvals-bypass flag itself, and
|
||||
// "[paperclip] ..." lines are diagnostics the adapter injected (e.g. ACP
|
||||
// fallback notes). Keep this list conservative so real errors are never
|
||||
// skipped.
|
||||
const BENIGN_CODEX_STDERR_LINE_RES: readonly RegExp[] = [
|
||||
/^YOLO mode is enabled\b/i,
|
||||
/^\[paperclip\]/,
|
||||
];
|
||||
|
||||
function isBenignCodexStderrLine(line: string): boolean {
|
||||
return BENIGN_CODEX_STDERR_LINE_RES.some((re) => re.test(line));
|
||||
}
|
||||
|
||||
export function firstMeaningfulStderrLine(text: string): string {
|
||||
const meaningful = text
|
||||
.split(/\r?\n/)
|
||||
.map((line) => line.trim())
|
||||
.find((line) => line && !isBenignCodexStderrLine(line));
|
||||
return meaningful ?? firstNonEmptyLine(text);
|
||||
}
|
||||
|
||||
function signalCodexChild(
|
||||
target: { pid: number | null; processGroupId: number | null },
|
||||
signal: NodeJS.Signals,
|
||||
|
|
@ -1391,7 +1414,7 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
|
|||
} as Record<string, unknown>)
|
||||
: null;
|
||||
const parsedError = typeof attempt.parsed.errorMessage === "string" ? attempt.parsed.errorMessage.trim() : "";
|
||||
const stderrLine = firstNonEmptyLine(attempt.proc.stderr);
|
||||
const stderrLine = firstMeaningfulStderrLine(attempt.proc.stderr);
|
||||
const fallbackErrorMessage =
|
||||
parsedError ||
|
||||
stderrLine ||
|
||||
|
|
|
|||
Loading…
Reference in New Issue