Repository navigation
Conversation
…timeout ends it The image's codex command is the npm launcher, which starts the native binary with inherited stdio. The model check's 30 s spawnSync timeout killed only the launcher; the native child kept retrying with the pipes open, so spawnSync waited on and the timeout was no hard limit. The check now runs the binary the launcher would start, resolved the same way, so the SIGKILL ends it. Any other codex on PATH runs as before. Finding 2274.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Review: access-token login retries a timed-out attempt up to five times, and each timeout's SIGKILL ended only the npm launcher, so a native login process kept running with the credential while the next attempt started. Login now uses the same helper, renamed codexExecutable and moved to the program's top level so the login block can call it. The helper is folded to one variable, which saves bytes in the rendered command (65,237 of 65,536).
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: blocked before merge. What this changesThis PR runs native Codex directly for startup login and model checks so their timeout kills the process doing the work. Example: A Codex startup model check exceeds its 30-second timeout.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness⛔ Blocked before merge - 2 items remain Keep this PR open: it addresses a current startup timeout defect with a focused implementation; the required runtime-flow documentation still needs updating. Priority: P2 Before merge
Findings
Tests
Agent review detailsHow this fits togetherThe Kubernetes Compute Driver renders the Harness launcher, which consumes projected credentials, performs Codex login and a model check, then publishes readiness and starts app-server. flowchart TB
Compute[Kubernetes Compute Driver] --> Launcher[Generated Harness launcher]
Credentials[Projected model credentials] --> Launcher
Launcher --> Native[Native Codex login and model check]
Native --> Provider[Model provider]
Native --> Gate[Startup result and readiness]
Gate --> Server[Codex app server]
Technical reviewBest possible solution: Keep direct native execution in the existing launcher, document its process boundary, and establish compatible handling of previously stored Sandbox commands. Do we have a high-confidence way to reproduce the issue? Current-main spawnSync calls, the runtime image symlink, and the pinned upstream launcher establish the surviving-child path; the author also reports a base-failing container regression, but this review executed no target code. Is this the best way to solve the issue? Directly invoking the same native executable is a small repair that preserves synchronous startup, retry decisions, and the existing command budget without requiring the draft PR's asynchronous conversion. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6f81df8e4df5. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceAcceptance criteria:
What I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 24, 2026, 9:16 AM ET / 13:16 UTC. |
Problem
The Codex Harness startup model check runs
spawnSync("codex", ...)with a 30 s timeout andSIGKILL. Access-token login does the same, for up to five attempts. In the runtime image,codexis the npm launcher (@openai/codex/bin/codex.js). It starts the platform package's native binary with inherited stdio, and onSIGTERMit forwards the signal to that binary.SIGKILLcannot be forwarded, so the timeout killed only the launcher. The native process kept running after the check gave up, still retrying against the model provider with the check's credential, and it held the inherited pipes. So the 30 s cap was not a hard limit on the attempt's process (finding 2274). After a login timeout, the next attempt started while the previous native login process was still running with the credential.Change
codexExecutable(). The helper takes the firstcodexonPATH. If it resolves to@openai/codex/bin/codex.js, the helper resolves@openai/codex-linux-<arch>/package.jsonfrom the launcher, as the launcher does, and falls back to the launcher's ownvendordirectory. The binary isvendor/<arch>-unknown-linux-musl/bin/codex. The timeout'sSIGKILLnow ends the process that makes the model calls.codexonPATH(a native install or a test fixture) still runs ascodex. The arguments, environment, timeout, output limit and result handling do not change. The launcher sets only update anddoctormetadata (CODEX_MANAGED_*) thatcodex execdoes not use. The native binary finds its bundled resources relative to its own path.app-serverstill goes through the launcher. It is long-lived, andforwardTerminationsendsSIGTERM, which the launcher forwards.Why not #3353: that draft makes the check async so it can kill a process group. That needs the vm-sandbox startup harnesses reworked, and it takes the dedicated Harness command over the 64 KiB exec-string budget (66,072 bytes). This change stays synchronous. It changes about 15 lines and keeps the command at 65,237 bytes. #3353's other half, stopping at the first 401, is a separate question and stays with that PR.
Verification
tests/integration/codex-model-probe.test.mjs:PATHstarts the native fixture, which hangs on the first call. Every scenario now also checks the container's process table after the check settles. A held failure leaves no Codex process, and a ready check leaves onlyapp-server.codex ... execprocess is still running after the check settled. With the fix all 12 cases pass. Run locally against a Node 24 image.kubernetes-provider-harnessconformance: the Disable unbounded connection retries for protected Codex launches #3310 command-shape regex now namescodexExecutable(). The vm-sandboxed startup harnesses (codex-native-startup,openshell-harness-status) still seecodex, because their stubbedfshas norealpathSync.kubernetes-provider-harnesspluscodex-native-startuppass 20/20, pinned to 2 CPUs. The new scenario passed 20/20 before the login change. On the final head it passed 18/20, five runs at a time, each pinned to 2 CPUs, under a local Podman engine. Both failures were the existing 5 sdocker exectimeout of the status snapshot helper, not an assertion. Main's unchanged "timeout then success" scenario hit the same timeout in 1 of 20 runs under the same load.codex-native-startup,kubernetes-compute(exec-string budget),kubernetes-runtime-credentials,openshell-harness-status,plugin-compute,workspace-node-supervisor,contracts,codex-stderr-filterandegress-enrollment(899 tests). ESLint and Prettier pass.codex-model-probe. Image Runtime Startup runs the real launcher and binary in the runtime image.Risks
SIGKILLbounds the native process but not any child process it starts. With every tool feature disabled, user config ignored (no MCP), a read-only sandbox and no Git repository,codex exechas no reason to start long-lived children. fix(runtime): stop the codex model check at its first credential rejection #3353's process-group kill would also cover those..bin/codexsymlink with a shell shim, the helper falls back tocodexand the old behavior returns without a failing test (follow-up finding).docs/reference/harness-execution.mdalready says attempts cap at 30 seconds, and this makes that true. The page is at 2,498 of 2,500 counted words, so it gets no new sentence.