Repository navigation
fix(codex): say why the Codex app-server exited #23938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
c4434fc
52df14a
70efc1f
aa039e8
3cbfa52
0864c8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| import * as Cause from "effect/Cause"; | ||
| import * as Deferred from "effect/Deferred"; | ||
| import * as Effect from "effect/Effect"; | ||
| import * as Queue from "effect/Queue"; | ||
| import * as Ref from "effect/Ref"; | ||
| import * as Sink from "effect/Sink"; | ||
| import * as Stdio from "effect/Stdio"; | ||
| import * as Stream from "effect/Stream"; | ||
|
|
@@ -44,20 +46,82 @@ export const makeInMemoryStdio = Effect.fn("makeInMemoryStdio")(function* () { | |
| }; | ||
| }); | ||
|
|
||
| const STDERR_TAIL_MAX_CHARS = 8_192; | ||
| const STDERR_EXCERPT_MAX_CHARS = 512; | ||
| // eslint-disable-next-line no-control-regex -- ANSI escape sequences start with ESC. | ||
| const TERMINAL_CONTROL_PATTERN = /\x1b\[[0-9;?]*[ -/]*[@-~]|\x1b\][^\x07\x1b]*(?:\x07|\x1b\\)/g; | ||
| // `Error: …`, `TypeError: …`, `Error [ERR_MODULE_NOT_FOUND]: …`, `error: …` | ||
| const ERROR_LINE_PATTERN = /^(?:[A-Za-z]*Error|error|fatal)(?: ?\[[^\]]+\])?:\s*\S/; | ||
|
|
||
| export const appendStderrTail = (current: string, chunk: string): string => { | ||
| const next = `${current}${chunk}`; | ||
| return next.length <= STDERR_TAIL_MAX_CHARS ? next : next.slice(-STDERR_TAIL_MAX_CHARS); | ||
| }; | ||
|
|
||
| /** | ||
| * The stderr line that names the failure: the last error-labelled line, else the last line. | ||
| * A Node launcher prints its thrown error between a source excerpt and the stack frames. | ||
| */ | ||
| export const stderrExcerpt = (tail: string): string | undefined => { | ||
| const lines = tail | ||
| .replace(TERMINAL_CONTROL_PATTERN, "") | ||
| .split(/\r?\n/) | ||
| .map((line) => line.trim().replace(/\s+/g, " ")) | ||
| .filter((line) => line.length > 0); | ||
| const line = lines.findLast((candidate) => ERROR_LINE_PATTERN.test(candidate)) ?? lines.at(-1); | ||
| if (line === undefined) { | ||
| return undefined; | ||
| } | ||
| return line.length <= STDERR_EXCERPT_MAX_CHARS | ||
| ? line | ||
| : `${line.slice(0, STDERR_EXCERPT_MAX_CHARS - 1)}…`; | ||
| }; | ||
|
|
||
| export const makeStderrTail = Effect.fn("makeStderrTail")(function* ( | ||
| stderr: ChildProcessSpawner.ChildProcessHandle["stderr"], | ||
| ) { | ||
| const tail = yield* Ref.make(""); | ||
| const drained = yield* Deferred.make<void>(); | ||
| yield* stderr.pipe( | ||
| Stream.decodeText(), | ||
| Stream.runForEach((chunk) => Ref.update(tail, (current) => appendStderrTail(current, chunk))), | ||
| Effect.ignore, | ||
| Effect.ensuring(Deferred.succeed(drained, undefined)), | ||
| Effect.forkScoped, | ||
| ); | ||
| // The exit status can arrive before the last stderr chunk has been read. | ||
| return Deferred.await(drained).pipe( | ||
| Effect.timeout("250 millis"), | ||
| Effect.ignore, | ||
| Effect.andThen(Ref.get(tail)), | ||
| ); | ||
| }); | ||
|
|
||
| type ChildProcessTerminationHandle = Pick< | ||
| ChildProcessSpawner.ChildProcessHandle, | ||
| "exitCode" | "pid" | ||
| >; | ||
|
|
||
| export const makeTerminationError = ( | ||
| handle: ChildProcessTerminationHandle, | ||
| stderrTail: Effect.Effect<string>, | ||
| ): Effect.Effect<CodexError.CodexAppServerError> => | ||
| Effect.match(handle.exitCode, { | ||
| Effect.matchEffect(handle.exitCode, { | ||
| onFailure: (cause) => | ||
| new CodexError.CodexAppServerTransportError({ | ||
| operation: "read-process-exit-status", | ||
| pid: handle.pid, | ||
| cause, | ||
| Effect.succeed( | ||
| new CodexError.CodexAppServerTransportError({ | ||
| operation: "read-process-exit-status", | ||
| pid: handle.pid, | ||
| cause, | ||
| }), | ||
| ), | ||
| onSuccess: (code) => | ||
| Effect.map(stderrTail, (tail) => { | ||
| const stderr = stderrExcerpt(tail); | ||
| return new CodexError.CodexAppServerProcessExitedError({ | ||
| code, | ||
| pid: handle.pid, | ||
| ...(stderr ? { stderr } : {}), | ||
|
Comment on lines
+116
to
+120
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Keep raw stderr out of the exported error. If Codex prints a credential, signed URL, or command argument on the selected line, this code places that value in 🤖 Prompt for AI Agents
Source: Path instructions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The excerpt follows
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mkantautas Understood. The ACP precedent explains why the launcher’s failure line is useful here. The excerpt limits control size and formatting, but do not redact sensitive values. I’ll leave the finding open for maintainers to decide whether Codex should receive the same exception as ACP. I’m not requesting another change before that decision. You are interacting with an AI system. |
||
| }); | ||
| }), | ||
| onSuccess: (code) => new CodexError.CodexAppServerProcessExitedError({ code, pid: handle.pid }), | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.