fix(acp): exit help and EOF cleanly#111091
Conversation
openclaw-barnacle
Bot
added
cli
5f50fd7 to
7df9907
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed July 18, 2026, 9:37 PM ET / July 19, 2026, 01:37 UTC. Summary PR surface: Source +81, Tests +360. Total +441 across 4 files. Reproducibility: no. high-confidence live reproduction was available in the supplied evidence. The changed paths and added process/startup tests make the reported help-hang and pre-Gateway EOF paths source-reproducible with medium confidence. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Keep the lazy imports, then provide redacted terminal or live-output proof covering both help commands, EOF after a real ACP frame with no Gateway, and successful delivery of buffered input when the Gateway becomes ready; confirm the bounded-buffer failure contract before merge. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction was available in the supplied evidence. The changed paths and added process/startup tests make the reported help-hang and pre-Gateway EOF paths source-reproducible with medium confidence. Is this the best way to solve the issue? Unclear. Lazy-loading runtime imports is a narrow fix for help-process handles, and teeing stdin addresses EOF observability, but the new bounded buffering contract needs maintainer confirmation and observed end-to-end proof. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b51e3d02c75b. Label changesLabel justifications:
Evidence reviewedPR surface: Source +81, Tests +360. Total +441 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
7df9907 to
d38a9b8
Compare
d38a9b8 to
6027466
Compare
|
Merged via squash.
|
![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: 6027466b48
ℹ️ 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".
| if (!gatewayConnected) { | ||
| void shutdown(); | ||
| } | ||
| }, shutdown); |
There was a problem hiding this comment.
When stdin raises a read error or pre-hello data crosses the new 1 MiB limit, startupInput.ended rejects, but passing shutdown directly as the rejection callback converts that rejection into successful cleanup. Because shutdown also resolves gatewayReady, serveAcpGateway then returns normally and Commander exits with status 0 without reporting the constructed error. Preserve cleanup while rethrowing the original failure so invalid or failed input produces a diagnostic and nonzero exit.
Useful? React with 👍 / 👎.
1 participant
What Problem This Solves
Two
acpexit hangs. (1)openclaw acp --helpandopenclaw acp client --helpprinted their help and then never exited (>8s / >18s, needed killing) — every other command tree's--helpexits cleanly. (2) The ACP bridge (acp … --require-existing) did not exit on stdin EOF while waiting for the Gateway. Found by the R7 QA campaign.Why This Change Was Made
acpcommand module eagerly pulled in the ACP client/server runtime (which opens a handle), so even--help— which never runs the command — kept the process alive.read(0), but Node doesn't emitenduntil buffered data is consumed, so a client that wrote an initialize/request frame and then closed was never detected.User Impact
acp --help/acp client --helpnow exit in <0.5s (rc 0) — the ACP runtime is imported lazily inside the action handler, so help/registration doesn't load it. No[INEFFECTIVE_DYNAMIC_IMPORT].Evidence
acp --help0.48s,acp client --help0.47s (rc 0).src/acp/translator.lifecycle.test.ts); happy-path assertion confirms buffered frames reach the SDK reader.