Skip to content

fix(codex-supervisor): disable app-server websocket compression#88550

Closed
imwyvern wants to merge 1 commit into
openclaw:mainfrom
imwyvern:investigate/codex-supervisor-endpoint
Closed

fix(codex-supervisor): disable app-server websocket compression#88550
imwyvern wants to merge 1 commit into
openclaw:mainfrom
imwyvern:investigate/codex-supervisor-endpoint

Conversation

@imwyvern

@imwyvern imwyvern commented May 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • disable permessage-deflate on Codex Supervisor app-server WebSocket clients
  • add a Unix socket handshake regression test that verifies the client does not offer WebSocket compression

Verification

  • node scripts/run-vitest.mjs extensions/codex-supervisor/src/supervisor.test.ts
  • pnpm build
  • autoreview --mode commit --commit HEAD via .agents/skills/autoreview/scripts/autoreview

Real behavior proof

Behavior addressed: the default local unix:// Codex Supervisor endpoint could fail health probes because the Node ws client offered permessage-deflate during the Unix socket WebSocket upgrade and Codex closed that handshake.
Real environment tested: local Codex CLI 0.135.0 app-server daemon with the default Unix control socket.
Exact steps or command run after this patch: used the patched connectCodexAppServerEndpoint({ id: "local", transport: "websocket", url: "unix://" }), then requested thread/loaded/list with { limit: 1 }.
Evidence after fix: the patched source returned {"ok":true,"result":{"data":[],"nextCursor":null}}.
Observed result after fix: the default unix:// endpoint initializes and answers thread/loaded/list.
What was not tested: full CI and cross-platform Codex daemon behavior.

@openclaw-barnacle openclaw-barnacle Bot added extensions: codex-supervisor Extension: codex-supervisor size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 31, 2026
@byungskers

byungskers commented May 31, 2026

Copy link
Copy Markdown

Nice narrow fix and the UDS handshake test is especially helpful. One thing I wanted to double-check: perMessageDeflate: false is now applied to both the unix-socket branch and the normal URL branch via the shared options object, but the code comment only mentions the Codex app-server Unix transport. Is disabling compression for all supervisor websocket endpoints intentional? If the bug is UDS-specific, it may be worth either scoping the option to unix:// endpoints or documenting why TCP endpoints should also stop advertising extensions.

chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed May 31, 2026
View reviewed changes

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d15d7ad01

ℹ️ 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".

Comment thread extensions/codex-supervisor/src/supervisor.test.ts
requestText = chunk.toString("utf8");
socket.end("HTTP/1.1 400 Bad Request\r\n\r\n");
});
});

@chatgpt-codex-connector chatgpt-codex-connector Bot May 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Guard the Unix socket test on Windows

When this test is run on Windows, socketPath is an os.tmpdir() filesystem path, but Node's IPC contract requires Windows named-pipe paths under \\?\pipe\ or \\.\pipe\; server.listen(socketPath) will emit an error and this promise never rejects, so pnpm test extensions/codex-supervisor hangs/fails before reaching the assertions. Either skip this Unix-domain-socket regression on non-POSIX platforms or construct a valid Windows pipe path.

Useful? React with 👍 / 👎.

@clawsweeper

clawsweeper Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 29, 2026, 5:40 PM ET / 21:40 UTC.

Summary
The PR disables WebSocket permessage-deflate for Codex Supervisor app-server clients and adds a regression test that inspects the Unix-socket upgrade request.

PR surface: Source +5, Tests +29. Total +34 across 2 files.

Reproducibility: yes. Source inspection shows current main uses ws default compression for unix://, ws 8.21.0 offers Sec-WebSocket-Extensions by default, and Codex accepts the Unix control-socket upgrade through accept_async; the PR body also includes after-fix live output from Codex CLI 0.135.0.

Review metrics: 1 noteworthy metric.

  • WebSocket Compression Scope: 1 option applied to 2 endpoint branches. The live proof covers the Unix-socket branch, but the diff also changes configured ws/wss endpoint negotiation before merge.

Stored data model
Persistent data-model change detected: migration/backfill/repair: extensions/codex-supervisor/src/supervisor.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦞 diamond lobster
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Scope perMessageDeflate: false to unix:// endpoints unless maintainers approve the all-endpoint change.
  • Skip or rewrite the Unix-socket regression for Windows named-pipe semantics.

Risk before merge

  • [P1] Configured ws:// and wss:// Codex Supervisor endpoints would stop advertising permessage-deflate even though the bug report, Codex dependency proof, and real behavior proof cover the Unix control socket path.
  • [P1] The added regression test uses a POSIX filesystem socket path and can fail or hang Windows automation before reaching the handshake assertions.

Maintainer options:

  1. Limit Unix Scope And Guard Windows (recommended)
    Apply perMessageDeflate: false only to unix:// connections and skip or adapt the Unix-domain-socket regression for Windows named-pipe semantics.
  2. Approve All-Endpoint Compression Change
    Maintainers can accept disabling compression for configured ws/wss endpoints if the PR adds endpoint proof and a clear compatibility rationale.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Scope `perMessageDeflate: false` to the `unix://` WebSocket branch in `extensions/codex-supervisor/src/json-rpc-client.ts`, preserve default `ws` options for normal `ws://` and `wss://` endpoints unless maintainers explicitly choose otherwise, and guard the new Unix-domain-socket handshake regression in `extensions/codex-supervisor/src/supervisor.test.ts` so it does not run with a POSIX filesystem socket path on Windows. Then run `node scripts/run-vitest.mjs extensions/codex-supervisor/src/supervisor.test.ts`.

Next step before merge

  • The remaining blockers are narrow repairs in the two touched files and do not require choosing a new product direction.

Security
Cleared: The diff changes a WebSocket client option and a local regression test, with no dependency, lockfile, credential, CI, or package-publishing surface changed.

Review findings

  • [P1] Scope no-compression to Unix endpoints — extensions/codex-supervisor/src/json-rpc-client.ts:288-293
  • [P2] Guard the Unix socket test on Windows — extensions/codex-supervisor/src/supervisor.test.ts:811
Review details

Best possible solution:

Keep the Unix-socket no-compression fix, preserve existing ws/wss negotiation unless maintainers intentionally approve the broader compatibility change, and make the regression test POSIX-only or Windows-pipe aware.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main uses ws default compression for unix://, ws 8.21.0 offers Sec-WebSocket-Extensions by default, and Codex accepts the Unix control-socket upgrade through accept_async; the PR body also includes after-fix live output from Codex CLI 0.135.0.

Is this the best way to solve the issue?

No, not as written. The Unix-socket repair is the right target, but the implementation should avoid changing normal ws/wss endpoints and should keep the regression test portable.

Full review comments:

  • [P1] Scope no-compression to Unix endpoints — extensions/codex-supervisor/src/json-rpc-client.ts:288-293
    This shared options object is also passed to normal ws:// and wss:// endpoints, so configured Codex Supervisor endpoints silently stop advertising permessage-deflate. The dependency proof and real behavior proof cover the Unix control socket; keep perMessageDeflate: false on that branch unless maintainers explicitly approve and prove the broader endpoint change.
    Confidence: 0.9
  • [P2] Guard the Unix socket test on Windows — extensions/codex-supervisor/src/supervisor.test.ts:811
    This regression passes an os.tmpdir() filesystem socket path to server.listen(). Node requires Windows IPC paths under \\?\pipe\ or \\.\pipe\, so Windows runs can fail before the handshake assertions; skip this POSIX-only test or use a valid Windows named pipe path.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 15de9d881a84.

Label changes

Label justifications:

  • P2: This is a normal-priority Codex Supervisor bug fix with limited runtime blast radius and concrete pre-merge blockers.
  • merge-risk: 🚨 compatibility: The patch changes compression negotiation for existing configured ws/wss supervisor endpoints, not only the default Unix-socket endpoint it proves.
  • merge-risk: 🚨 automation: The added regression test uses a POSIX socket path and can fail Windows automation unless guarded or rewritten.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body includes after-fix live output from a local Codex CLI 0.135.0 app-server daemon over the default Unix control socket, which is sufficient for the Unix-socket behavior it claims.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix live output from a local Codex CLI 0.135.0 app-server daemon over the default Unix control socket, which is sufficient for the Unix-socket behavior it claims.
Evidence reviewed

PR surface:

Source +5, Tests +29. Total +34 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 7 2 +5
Tests 1 29 0 +29
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 36 2 +34

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs extensions/codex-supervisor/src/supervisor.test.ts.
  • [P1] pnpm build.

What I checked:

Likely related people:

  • steipete: Live GitHub commit history shows steipete authored the merged Codex Supervisor plugin and follow-up stabilization/docs changes touching the endpoint config, JSON-RPC client, and tests. (role: feature introducer and recent area contributor; confidence: high; commits: 9dd3bce549b4, 69c3b56bdecb, 8b477d2887ed; files: extensions/codex-supervisor/src/json-rpc-client.ts, extensions/codex-supervisor/src/config.ts, extensions/codex-supervisor/src/supervisor.test.ts)
  • byungskers: Left the focused review note identifying that the compression option was broader than the Unix-socket bug and proof. (role: reviewer; confidence: medium; files: extensions/codex-supervisor/src/json-rpc-client.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-06-21T08:37:00.660Z sha 82b260c :: needs changes before merge. :: [P1] Scope no-compression to Unix endpoints | [P2] Guard the Unix socket test on Windows

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels May 31, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 31, 2026
@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels May 31, 2026
@imwyvern
imwyvern force-pushed the investigate/codex-supervisor-endpoint branch from 7d15d7a to 82b260c Compare May 31, 2026 15:14
@openclaw-barnacle openclaw-barnacle Bot added triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. labels May 31, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. labels May 31, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 31, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels May 31, 2026
@Alix-007 Alix-007 mentioned this pull request Jul 7, 2026
Closed
@steipete

steipete commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Superseded by #104045, which removes the separate codex-supervisor plugin and moves supervision into the official Codex plugin/App Server runtime. The retired supervisor endpoint—and therefore its WebSocket compression path—no longer exists on main.

Thanks for investigating and fixing the original endpoint behavior.

@steipete steipete closed this Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

No one assigned

Labels

extensions: codex-supervisor Extension: codex-supervisor merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@imwyvern @byungskers @steipete