Repository navigation
fix: strip ANSI escape sequences from JSONL before parsing - #97
murataslan1 wants to merge 1 commit into
Conversation
Terminals can inject escape sequences like bracketed paste mode (\x1b[?2004h) into the Codex app-server stdout stream. These were causing JSON.parse failures that killed the entire client connection. Now strips CSI/OSC sequences before parsing, and silently skips lines that become empty after cleanup. Closes openai#23
There was a problem hiding this comment.
Pull request overview
This PR hardens the Codex app-server JSONL client by removing terminal ANSI escape sequences from each received line before attempting JSON.parse, preventing terminal artifacts (like bracketed paste mode sequences) from crashing the protocol handler.
Changes:
- Strip ANSI escape sequences from incoming JSONL lines prior to parsing; skip lines that become empty after stripping.
- Export
AppServerClientBaseto enable direct unit testing of line-handling behavior. - Add unit tests covering ANSI-only lines, inline ANSI + JSON, and invalid JSON behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
plugins/codex/scripts/lib/app-server.mjs |
Sanitizes incoming JSONL lines by stripping ANSI escapes before JSON parsing; exports the base client for testing. |
tests/app-server.test.mjs |
Adds tests validating ANSI stripping behavior and preserving error behavior for truly invalid JSON. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Strip ANSI escape sequences that terminals may inject into stdout | ||
| // (e.g. bracketed paste mode \x1b[?2004h from shell initialization). | ||
| const cleaned = line.replace(/\x1b\[[0-9;?]*[A-Za-z]|\x1b\][^\x07]*\x07|\x1b[()][A-B012]/g, "").trim(); |
There was a problem hiding this comment.
The ANSI-stripping regex doesn’t cover some common escape variants, so JSON parsing can still fail if those appear in the stream (e.g. CSI sequences whose final byte is '' like ESC[3, or OSC sequences terminated with ST (ESC\) instead of BEL). Consider using a more complete CSI/OSC matcher (including ST termination) so the fix matches the PR’s stated scope (CSI/OSC/charset designators).
| const cleaned = line.replace(/\x1b\[[0-9;?]*[A-Za-z]|\x1b\][^\x07]*\x07|\x1b[()][A-B012]/g, "").trim(); | |
| const cleaned = line.replace(/\x1b\[[0-9:;<=>?]*[ -/]*[@-~]|\x1b\][^\x07\x1b]*(?:\x07|\x1b\\)|\x1b[()][A-B0-2]/g, "").trim(); |
| message = JSON.parse(cleaned); | ||
| } catch (error) { | ||
| this.handleExit(createProtocolError(`Failed to parse codex app-server JSONL: ${error.message}`, { line })); | ||
| this.handleExit(createProtocolError(`Failed to parse codex app-server JSONL: ${error.message}`, { line: cleaned })); |
There was a problem hiding this comment.
In the JSON parse error path, the error data now only includes the cleaned line. That can make debugging harder because the raw bytes that caused the failure (including any non-ANSI corruption) are lost. Consider including both the original line and the cleaned value in the error data so failures remain diagnosable.
| this.handleExit(createProtocolError(`Failed to parse codex app-server JSONL: ${error.message}`, { line: cleaned })); | |
| this.handleExit( | |
| createProtocolError( | |
| `Failed to parse codex app-server JSONL: ${error.message}`, | |
| { line: cleaned, cleaned, rawLine: line }, | |
| ), | |
| ); |
|
|
||
| import { AppServerClientBase } from "../plugins/codex/scripts/lib/app-server.mjs"; | ||
|
|
||
| function createTestClient() { | ||
| const client = new AppServerClientBase("/tmp"); |
There was a problem hiding this comment.
Test helper hard-codes "/tmp" for the client cwd. Other tests use makeTempDir()/os.tmpdir() for portability; using a platform-specific path can be fragile on Windows (and if the constructor starts validating/using the cwd later). Consider switching to the shared temp-dir helper or os.tmpdir() here.
| import { AppServerClientBase } from "../plugins/codex/scripts/lib/app-server.mjs"; | |
| function createTestClient() { | |
| const client = new AppServerClientBase("/tmp"); | |
| import { tmpdir } from "node:os"; | |
| import { AppServerClientBase } from "../plugins/codex/scripts/lib/app-server.mjs"; | |
| function createTestClient() { | |
| const client = new AppServerClientBase(tmpdir()); |
dkundel-openai
added
the
duplicate
ChatGPT Codex Connector
|
Duplicate of #311. |
Summary
app-server.mjs\x1b[?2004hand similar terminal artifacts from crashing the JSONL protocol handlerhandleExitas beforeAppServerClientBasefor direct unit testingTest plan
tests/app-server.test.mjswith 3 cases: pure ANSI lines ignored, JSON with inline ANSI parsed correctly, invalid JSON still errors/codex:review— should no longer crash with JSONL parse errorCloses #23