Skip to content

Import four upstream fixes: a crash, a silent hook timeout, a silent wait timeout, and a hung connect - #14

Merged
Edo771977 merged 9 commits into
mainfrom
claude/focused-carson-khonz0
Sep 20, 2026
Merged

Edo771977 merged 9 commits into
mainfrom
claude/focused-carson-khonz0

Conversation

@Edo771977

Copy link
Copy Markdown
Owner

Four PRs opened upstream in the last day, each one verified against this fork's own code before importing — all four defects were present here.

341/341, full suite run twice. tsc clean.

Upstream PR The defect, in this fork Conflicts
#775 codex.mjs read item.changes.length on every fileChange start event. Codex can send that notification before the change list exists, and the TypeError aborted the whole turn — a job lost to a log line none
#772 STOP_REVIEW_TIMEOUT_MS was 15 minutes and hooks.json gives the Stop hook 900 seconds — the same instant. Claude Code killed the hook exactly when spawnSync would have reported ETIMEDOUT, so the gate's own "review timed out, run /codex:review --wait or bypass" was never printed and the turn just ended none
#774 status --wait recorded waitTimedOut in the JSON, but the text report was rendered from the job alone and the process exited 0, so a script polling /codex:status <id> --wait read a timed-out wait as a finished check none
#773 BrokerCodexAppServerClient waited on connect or error and nothing else: an endpoint that never completes hung the command for good 2, resolved

openai#774 is the same principle this fork already applied to cancellation in #656 — an outcome nothing confirmed must not be reported as one.

openai#773 is taken in half, deliberately

The probe half stays ours. probeEndpoint() already bounds every connect attempt, and probeBrokerEndpoint() reports why an attempt failed — which ensureBrokerSession() needs in order to tell "nothing is there" from "there, but refusing" (PR #10). openai#773 rewrites waitForBrokerEndpoint() into a bounded loop returning a bare boolean, which would undo that, so HEAD wins for broker-lifecycle.mjs and its tests.

Their test for that half is also not imported: it replaces net.createConnection globally with a stub whose setTimeout() is a no-op, so it would fail against our implementation — which bounds the attempt through exactly that call — while proving nothing about it.

The client half was genuinely missing and is taken, with three changes on top:

  • the connection is created through an injectable seam, so tests hand it a socket that never settles instead of monkey-patching net
  • every settle path runs through one function that clears the deadline. A timer left armed after the attempt settled holds the event loop open for its whole budget — on a short-lived command, the difference between exiting now and exiting in 2s. Structurally impossible now rather than guarded by a test
  • the budget is a named export resolved through resolveBrokerConnectTimeoutMs(), which a test pins, with the comment explaining why it is short: a broker that is merely refusing is handled a level up, by waiting for it, not by replacing it

Sabotage matrix for the new tests

sabotage which test fails
connect deadline removed "a broker connect that never settles is given up on"
default budget 2000 → 20000 "the broker connect budget defaults to 2s and honours an override"

🤖 Generated with Claude Code

https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg


Generated by Claude Code

kevin9327 and others added 9 commits September 20, 2026 23:53
The stop-review child timeout matched the Stop hook's 900s budget, so
Claude Code killed the hook before it could emit the ETIMEDOUT reason.
Lower the inner spawnSync timeout to 14 minutes so a slow review still
returns the bypass/manual-review message instead of ending the turn
silently.

Fixes openai#766.
waitForBrokerEndpoint only resolved on connect or error, so a named
pipe or Unix socket that never completed left ensureBrokerSession and
later commands hung past their readiness budget. Time out each connect
attempt, apply the same deadline when opening a broker client, and
treat ETIMEDOUT like the other connection failures that already fall
back to a direct app-server.
status --wait already set waitTimedOut on the JSON snapshot, but text
output still rendered a normal running job and the process exited 0.
Callers that poll with /codex:status <id> --wait therefore treated a
timed-out wait as a finished status check. Print the timeout and exit
non-zero while keeping waitTimedOut in the JSON payload.
describeStartedItem assumed item.changes was always present on
item/started fileChange notifications. Codex can emit the start event
before the change list is known, which threw TypeError and aborted the
turn. Treat a missing or non-array changes field as zero file changes
so the job can finish.
…anges

describeStartedItem() read item.changes.length on every fileChange
item/started notification. Codex can send that notification before the
change list exists, and the TypeError aborted the whole turn — a job lost
to a log line.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
The review child's timeout was 15 minutes and hooks.json gives the Stop
hook 900 seconds, so Claude Code killed the hook at the same instant
spawnSync would have reported ETIMEDOUT. The gate's own message — review
timed out, run /codex:review --wait or bypass — could never be printed,
and the turn just ended.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
…successful

The wait already recorded waitTimedOut in the JSON snapshot, but the text
report was rendered from the job alone and the process exited 0, so a
caller polling `/codex:status <id> --wait` read a timed-out wait as a
finished status check. Same principle this fork already applies to
cancellation in openai#656: an outcome nothing confirmed must not be reported as
one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Two halves, and only one was missing here.

The probe half is already ours and stays: probeEndpoint() bounds every
connect attempt and probeBrokerEndpoint() reports *why* an attempt failed,
which ensureBrokerSession() needs to tell "nothing is there" from "there,
but refusing". openai#773 rewrites waitForBrokerEndpoint() into a bounded loop
that returns a bare boolean, which would undo that, so HEAD wins for
broker-lifecycle.mjs and its tests.

The client half was genuinely missing: BrokerCodexAppServerClient waited
on connect or error and nothing else, so an endpoint that never completes
— a wedged broker, a named pipe whose server is not accepting — hung the
command for good. It now gives up after 2s and withAppServer treats
ETIMEDOUT like the other connection failures that fall back to a direct
app-server.

Changes on top of the import:

- the connect is created through an injectable seam, so the tests can hand
  it a socket that never settles instead of monkey-patching net
- every settle path runs through one function that clears the deadline, so
  a timer cannot outlive its attempt and hold a short-lived command open
  for its whole budget
- the budget is a named export resolved through
  resolveBrokerConnectTimeoutMs(), which a test pins, with the comment
  explaining why it is short: a broker that is merely refusing is handled
  a level up, by waiting for it, not by replacing it

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
@Edo771977
Edo771977 merged commit e479614 into main Sep 20, 2026
1 check passed
@Edo771977
Edo771977 deleted the claude/focused-carson-khonz0 branch September 20, 2026 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants