P1 bridge follow-ups: send receipts, bounded transport, bounded thread reads - #40
Conversation
…read reads Closes #34 (delivery states: rejected/accepted/unknown, IDs preserved), #35 (idempotent shutdown, handshake/fragmentation/budgets, no leaks), #36 (truncation metadata, bounded full reads, safe normalize, 1-200 limits). Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 78a2510 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR |
Collision note (Session Miner)Active ubuntu Composer session on
Recommendation: freeze AB-full moves on bridge/plugin until this PR merges, then rebase AB-full onto it — otherwise expect merge conflict / lost P1 receipt+transport+#36 hardening. GPT-6 Pro review of this PR is in flight. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78a2510a86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "grok-bot-cli": patch | ||
| --- |
There was a problem hiding this comment.
This file starts directly with the package release entry instead of ---, unlike every other changeset in the repository. Changesets therefore cannot parse this release metadata, blocking commands such as changeset status, changeset version, and the release workflow until the opening delimiter is added.
Useful? React with 👍 / 👎.
| if (frame.opcode === 0x1 || frame.opcode === 0x2) { | ||
| if (fragOpcode != null) return failProtocol("new message before finishing fragments"); | ||
| if (!frame.fin) { | ||
| fragOpcode = frame.opcode; | ||
| fragParts = [frame.payload]; | ||
| fragBytes = frame.payload.length; |
There was a problem hiding this comment.
When an oversized frame arrives complete in a TCP chunk, decodeFrame succeeds, so the size check in the !frame branch is bypassed; unfragmented frames are passed directly to onText, while an oversized first fragment is retained without checking fragBytes until another fragment arrives. A daemon can consequently make this client buffer and parse messages above WS_MAX_MESSAGE_BYTES, defeating the new transport budget.
Useful? React with 👍 / 👎.
| async function readJson(res) { | ||
| const text = await res.text(); | ||
| if (text.length > GATEWAY_MAX_RESPONSE_BYTES) { | ||
| throw new GatewayError("Gateway response too large (" + text.length + " bytes, limit " + GATEWAY_MAX_RESPONSE_BYTES + ")"); |
There was a problem hiding this comment.
For a large or unbounded gateway response, res.text() materializes the entire body before the limit is checked, so the new cap cannot prevent excessive memory consumption. It also compares UTF-16 code units while reporting bytes, allowing multibyte responses substantially larger than 2 MiB; read the response stream incrementally and abort once the byte budget is exceeded.
Useful? React with 👍 / 👎.
| let msg; | ||
| try { | ||
| msg = JSON.parse(text); | ||
| } catch (err) { | ||
| failAll(new Error("Codex app-server sent an unreadable message: " + err.message)); | ||
| return; | ||
| } | ||
| onMessage(msg); |
There was a problem hiding this comment.
If the daemon sends syntactically valid JSON that is not an object, especially null, parsing succeeds and onMessage immediately dereferences msg.id, causing an uncaught TypeError from the socket event handler and terminating the CLI with a stack trace. Previously the dispatch was inside the parsing try; malformed JSON-RPC values should instead settle pending requests through failAll.
Useful? React with 👍 / 👎.
| // Scope refusals to this turn: server requests from earlier calls belong to another context. | ||
| const seenRefused = client.refused.length; |
There was a problem hiding this comment.
When the app-server emits an approval or input request for this thread during thread/resume, it is recorded and refused before the resume response arrives, but this snapshot is taken afterward and excludes it from freshRefused. The command then starts a turn and can report success despite having just rejected a request required by that same thread; snapshot before resuming or retain all recorded requests whose threadId matches the target.
Useful? React with 👍 / 👎.
2 participants
Why: GPT-6 Pro's post-land review left three P1 gaps in the Grok↔Codex bridge — dropped send receipts, a leaky/unbounded WebSocket transport, and irreversible 400-char thread truncation. This closes #34, #35, #36 in one potato-mode PR (shortest working diffs, no new deps, root stays zero-dependency).
Scope:
CodexSendErrorwithdelivery(rejected/accepted/unknown) + preserved thread/turn IDs; refusals scoped to the current turn's thread; malformed acks and post-write loss classified unknown with no blind retries. GatewaysendPromptreturnsdelivery+messageIdonly on a confirmed string receipt — unconfirmed bodies are unknown, never silent accepts.gbot send/gbot_sendprint the receipt. CLI--jsonfailures emit structured stderr JSON witherror,delivery,threadId/turnId/targetId.close()settling pending requests/timers with guaranteed socket destruction (graceful close frame, then destroy with a 1 s force fallback); absolute handshake deadline immune to trickled bytes; exact handshake validation with the size cap applied to terminated headers too; fragmentation reassembly; strict UTF-8; malformed RPC (incl. JSONnull) routed throughfailAll; masked-frame/opcode/control-frame protocol errors; header/frame/message/buffer budgets enforced before decoding.truncated/fullLengthmetadata, bounded full reads (gbot_thread full:trueup to 20k chars/entry within a 200k total cap; CLIthread --full), truncation notices with char counts, remainder retrievable viagbot thread --full/--jsonon the machine, safe string normalization, 1–200 limit consistency across CLI/tool/gateway, 30 s gateway deadline + 2 MB response cap counted as true bytes via a streaming reader that cancels on overflow. Deliberate ceilings marked withponytail:comments.Hold review + fixes: GPT-6 Pro put a HOLD on the first revision (receipt serialization, transport bounds, malformed-RPC handling). Fixed on this branch: streaming byte-counted reads, absolute handshake deadline, terminated-header and complete-frame caps, guaranteed close destruction, null-RPC validation, structured
--jsonerrors, ack-shape-gated acceptance, and aggregate MCP output budgets with an explicit continuation path.Correlation IDs:
sendPromptreuses the existingclientNonce/replyToId; the full send/execution correlation envelope stays in #37 and is not claimed here.Verification: root
npm test94/94 green; pluginnpm run checkgreen (validate, build, artifact, typecheck, 9 route-unit tests).Closes #34 #35 #36. Out of scope (untouched): #37 correlation envelope, #38 native queue, #39 status diagnostics, Desktop private pipes. No
approval_policychanges; public app-server JSON-RPC only.