fix: drop non-JSONL garbage on codex app-server stdout#311
Open
lask3802 wants to merge 2 commits into
Open
Conversation
…penai#23) The JSONL parser in `lib/app-server.mjs` `handleLine()` previously called `JSON.parse()` on every line read from the codex app-server stdout pipe and, on failure, called `handleExit()` — rejecting every in-flight request and tearing the connection down. In practice the codex CLI occasionally emits non-JSON bytes onto stdout that have nothing to do with the protocol. Two known sources: 1. Terminal/shell init noise (issue openai#310 sibling-bug openai#23) — e.g. zsh writes the bracketed-paste marker `\x1b[?2004h` when a subprocess inherits the parent's TTY. 2. Localized OS messages on Windows non-English locales (issue openai#310 + openai/codex#21957). On zh-TW (CP-950 / Big5), when codex.exe runs `taskkill /T /F` to clean up a failed MCP child and inadvertently routes taskkill's stdout to its own stdout, the JSONL stream receives the bytes `A6 A8 A5 5C 3A 20 50 49 44 ...` ("成功: PID 為 xxxx ..."), which decode under UTF-8 as `���\: PID ...` — the exact substring in the original error. Both bug classes share the same correct shape of fix at the plugin layer: lines that cannot possibly be JSONL records should be dropped, not treated as protocol violations. A JSONL record always begins with `{` or `[`. Any line whose first non-whitespace, post-strip-ANSI character is something else is definitively garbage from outside the protocol. This commit factors a `cleanProtocolLine()` helper into `lib/jsonl.mjs` that: - strips CSI / OSC ANSI escape sequences, - trims whitespace, - returns `null` if the result is empty or does not start with `{` / `[`, - otherwise returns the cleaned candidate string. `handleLine()` skips the line on `null` and parses the candidate otherwise. Lines that pass the guard but are still malformed JSON continue to surface as real protocol errors via the existing `handleExit()` path — that branch is preserved. The root corruption — codex.exe leaking taskkill's stdout into its own stdout on Windows zh-TW — is filed upstream as openai/codex#21957. This PR is the plugin-side guard; it is also a strict superset of the ANSI-escape strip approach explored in openai#24, openai#97, and openai#171, so any of those can be closed in favour of this if the maintainers prefer. Test plan: - `node --test tests/jsonl.test.mjs` — 11/11 pass, covers bracketed-paste prefix, OSC sequences, SGR colours, the exact CP-950 mojibake from openai#310, plain JSON passthrough, malformed-but- JSON-shaped fall-through to the parser, and non-string input. - The pre-existing test failures on this branch (`state`, `runtime`, `process` Windows tmpdir / MSYS quirks) are unrelated and reproduce on `main`. Closes openai#310 Refs openai#23 Refs openai/codex#21957
There was a problem hiding this comment.
Pull request overview
This PR hardens the Codex app-server JSONL reader against “garbage” lines on stdout by sanitizing/guarding input before parsing, preventing the client from tearing down the connection on clearly non-protocol output (e.g., terminal escape noise and localized Windows messages).
Changes:
- Added
cleanProtocolLine(rawLine)to strip ANSI/OSC escapes, trim whitespace, and drop lines that can’t be JSONL (don’t start with{or[). - Updated
AppServerClientBase.handleLine()to skip dropped lines instead of treating them as fatal protocol violations. - Added unit tests covering ANSI/OSC stripping, mojibake/taskkill output, JSON passthrough, and malformed-but-JSON-shaped fall-through behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/jsonl.test.mjs |
Adds focused unit coverage for cleanProtocolLine behavior across real-world noise inputs. |
plugins/codex/scripts/lib/jsonl.mjs |
Introduces the protocol-line cleaning/guard helper and ANSI/OSC stripping regex. |
plugins/codex/scripts/lib/app-server.mjs |
Integrates cleanProtocolLine into the JSONL parsing path to avoid disconnecting on non-JSON lines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+35
to
+38
| // CSI: ESC [ … final-byte (e.g. \x1b[?2004h, \x1b[31m) | ||
| // OSC: ESC ] … (BEL | ESC \) (e.g. window-title sets) | ||
| // eslint-disable-next-line no-control-regex | ||
| const ANSI_ESCAPE_RE = /\x1b\[[0-9;?]*[a-zA-Z]|\x1b\][^\x07]*(?:\x07|\x1b\\)/g; |
Comment on lines
+40
to
+49
| /** | ||
| * Returns a JSON-shaped candidate string for the given raw line, or | ||
| * `null` if the line should be skipped because it cannot be valid JSONL. | ||
| * | ||
| * @param {string} rawLine | ||
| * @returns {string | null} | ||
| */ | ||
| export function cleanProtocolLine(rawLine) { | ||
| if (typeof rawLine !== "string") { | ||
| return null; |
…SDoc Both addresses Copilot inline review on PR openai#311. 1. The CSI regex previously matched final bytes only in `[a-zA-Z]`, which misses valid CSI sequences that end on a non-letter. The most relevant missing case in this codebase is `\x1b[200~` / `\x1b[201~` — bracketed-paste content markers — which a TTY-attached subprocess emits *around* pasted content, not just the on/off `\x1b[?2004h` / `\x1b[?2004l` toggles. With the old regex, a JSON record sandwiched between `200~` and `201~` would survive `replace`, then fail the first-char `{`/`[` guard, then be silently dropped — the worst kind of regression for this fix's stated goal. Broaden to the ECMA-48 CSI grammar: parameter bytes in 0x30..0x3F, optional intermediate bytes in 0x20..0x2F, final byte in 0x40..0x7E. Adds a regression test for `\x1b[200~{...}\x1b[201~` and the boundary finals `@` (0x40) and `` ` `` (0x60). 2. The JSDoc declared `@param {string} rawLine` even though the function explicitly handles non-string input by short-circuiting to `null` (and a unit test asserts that). Update to `@param {unknown}` so editors and tsc-via-checkJs don't mislead callers into thinking they must type-narrow first.
Author
|
Thanks @copilot for the review — both points addressed in 2dc20bf:
Full test run: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lib/jsonl.mjs#cleanProtocolLine(rawLine): strips ANSI/OSC escape sequences, trims whitespace, and returns the cleaned line only if it starts with a JSON delimiter ({or[). Otherwise returnsnull.lib/app-server.mjshandleLine()to skip onnullinstead of treating non-JSON lines as protocol violations that tear the connection down.tests/jsonl.test.mjs(11 cases) covering bracketed-paste prefix, OSC window-title, SGR colours, the exact CP-950 mojibake from Windows zh-TW: codex app-server JSONL parser crashes on Big5-encoded taskkill stdout leak #310, plain JSON passthrough, malformed-but-JSON-shaped fall-through to the parser, and non-string input.Problem
handleLine()callsJSON.parse()on every line read from the codex app-server stdout pipe and, on failure, callshandleExit()— rejecting every in-flight request and closing the connection. In practice the codex CLI occasionally emits bytes onto stdout that have nothing to do with the protocol. Two known sources:\x1b[?2004hwhen a subprocess inherits the parent's TTY. The user-visible error:codex.exerunstaskkill /T /Fto clean up a failed MCP child and inadvertently routes taskkill's stdout to its own stdout, the JSONL stream receives:Unexpected token '�', "���\: PID "...reported in Windows zh-TW: codex app-server JSONL parser crashes on Big5-encoded taskkill stdout leak #310.Both bug classes share the same correct fix at the plugin layer: lines that cannot possibly be JSONL records should be dropped, not treated as protocol violations. A JSONL record always begins with
{or[. Any line whose first non-whitespace, post-strip-ANSI character is something else is definitively garbage from outside the protocol.Why this approach over the existing PRs (#24, #97, #171)
#171 (the most recent of three open PRs) introduces a
stripAnsi()helper and applies it beforeJSON.parse(). That correctly fixes #23 — but does not fix #310, because the leaked bytes are not ANSI escapes. They are localized OS text. AfterstripAnsi()the line is still not JSON, the parser still throws, the connection still dies.The
cleanProtocolLine()guard in this PR is a strict superset:{or[(so Windows zh-TW: codex app-server JSONL parser crashes on Big5-encoded taskkill stdout leak #310 is covered).If maintainers prefer to land #171 first I'm happy to rebase this onto it (the regex code can move to
lib/strings.mjs#stripAnsiandcleanProtocolLinebecomes a one-liner that imports it). Either order works; the unit tests stay the same.What this PR explicitly does NOT do
app-server-broker.mjs's socket data handler). That handler reads JSONRPC requests from the companion, which sends well-formed JSON; there is no observed corruption source on that path. fix: strip ANSI escape sequences from JSONL parsing #171 does patch it for symmetry — happy to add the same here if you'd like.codex.exe. That is filed as openai/codex#21957 and is out of scope for this repo. This PR is defense-in-depth for any embedder ofcodex app-server.Test plan
node --test tests/jsonl.test.mjs— 11/11 pass.node --test tests/*.test.mjs— all failures present on this branch (state.test.mjs,runtime.test.mjs,process.test.mjs— all Windows-onlyos.tmpdir()/ MSYS path translation issues) reproduce againstmainfrom the same checkout, i.e. no regressions introduced by this PR.codex-companion.mjs taskinvocations (each preceded by killing the broker to force the leaky direct-spawn path) now all return{"status":0,...}./codex:reviewfrom a zsh-default Claude Code session (issue Review fails with JSONL parse error: bracketed paste mode escape sequence in output ([?2004h) #23 case) — I do not have a zsh box handy; logically equivalent to PR fix: strip ANSI escape sequences from JSONL parsing #171's verification.Closes / refs