fix(codex): startup-prompt handling that can't live-lock or block the loop (split from #566) - #731
tedswinyar wants to merge 8 commits into
Conversation
… loop Extracted from awslabs#566, which had accumulated two independent state machines behind one review. This is the Codex-provider half; the server-side initial-message delivery and its non-completable mask stay in awslabs#566. The two share no files: this touches only providers/codex.py and its tests, and nothing here references the mask surface. Five defects in Codex startup handling, all reachable on a default `cao launch`: - ``startup_prompt_handler_timeout`` was a TOTAL budget, so a handler that kept seeing new frames burned the whole allowance and returned late. It is now an IDLE GAP with a separate outer cap, so a live dialog is answered promptly and a quiet pane still exits. - The first-run login menu ("Sign in with ChatGPT") cannot be auto-dismissed -- it needs a human to complete OAuth or supply a key. The handler used to keep answering into it until the outer cap expired, and initialize() then tore the worker down before an operator could reach the session. It now returns as soon as the menu is recognised, leaving the pane alive and answerable. - A trust-prompt answer surviving in scrollback could satisfy the trust check while a LIVE login menu was on screen, so CAO believed the dialog was cleared and dismissed nothing. - v1 trust matching ran against the whole capture, so a stale copy anywhere in scrollback counted. Liveness is now decided by POSITION, which is what stops a live dialog live-locking against its own history. - The startup handler ran blocking tmux I/O on the event loop -- the same defect awslabs#451 fixed for claude_code, which codex still had. Now offloaded via asyncio.to_thread. Reverting that offload fails two loop-starvation tests. Tests: test_codex_provider_unit.py + test_startup_handler_nonblocking.py at this head, 292 passed / 3 skipped. Verified independent of awslabs#566: with main's unmodified launch.py, test_launch.py + test_terminal_service_full.py + test_status_monitor.py give 194 passed. black and isort clean. The non-blocking startup tests live in test_startup_handler_nonblocking.py, the shared parametrized module added by awslabs#509, rather than in the codex file -- that placement was a review ask on awslabs#566 and is carried forward here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #731 +/- ##
=======================================
Coverage ? 91.95%
=======================================
Files ? 207
Lines ? 29496
Branches ? 0
=======================================
Hits ? 27124
Misses ? 2372
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… delivery The split was announced but only half done. awslabs#731 was opened carrying the Codex provider work, but the same changes were still present here, so both PRs shipped codex.py and its tests. Merging both would have applied that work twice. Reverts providers/codex.py, test/providers/test_codex_provider_unit.py and test/providers/test_startup_handler_nonblocking.py to this branch's base, so awslabs#731 is now their only home. What remains here is one concern: server-side initial-message delivery and the non-completable mask that makes polling it safe. Reverted to the BASE (c0c9b72) rather than to current origin/main deliberately -- taking origin/main's copies would have pulled awslabs#723's merged yolo/codexProfile warning into this diff, making it look like this PR contained work it did not. Scope is now 8 files, +1593/-63: launch.py, session_service.py, status_monitor.py, terminal_service.py and their four test modules. 306 passed across test_launch.py, test_terminal_service_full.py, test_session_service.py, test_deferred_submit_verification.py and test_status_monitor.py. black and isort clean (verified by exit code).
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #731 — fix(codex): startup-prompt handling that can't live-lock or block the loop (split from #566)
Summary
This PR extracts the Codex-provider half of #566: it makes startup_prompt_handler_timeout an idle gap (with provider_init_timeout as the hard outer cap), recognizes the first-run login menu as a settled state instead of stalling to the cap, decides v1 trust-prompt liveness by position in the bottom region (last-occurrence comparison), and offloads blocking tmux I/O via asyncio.to_thread. The core work is careful, matches the sibling implementations in kimi_cli/antigravity_cli, and ships an unusually thorough regression-test suite. However, the diff also deletes the #707/#723 sandbox-visibility logging and its three tests — merged to main earlier the same day and nowhere mentioned in the PR body — which is a silent revert of a merged operator-safety feature and must be fixed before merge.
Blocking (must fix before merge)
- [consistency/security] src/cli_agent_orchestrator/providers/codex.py
_build_codex_command(first hunk) — The PR deletes the #707 logging merged via #723 (05f86a67, merged tomain2026-09-03): the WARNING whenallowed_tools: ["*"]forces--yoloand discards an explicitcodexProfile, and the INFO record that~/.codex/config.toml[sandbox_*]settings (incl.network_access) do not apply under--yolo. Its three tests (test_discarded_codex_profile_is_warned_not_silent,test_no_warning_when_codex_profile_is_honored,test_default_yolo_launch_records_that_sandbox_settings_do_not_apply) are deleted with it. Verified:05f86a67IS an ancestor of this PR's single commitaae1ec20and the merge base (3eafb6d6, currentmainHEAD) contains the logging — so this is an active deletion introduced by this PR, not a stale-branch artifact. The PR body's "What's fixed" never mentions it. Compounding the drift,docs/codex-cli.md(added by #723, line ~144) still states "CAO logs a WARNING" for the discarded-codexProfilecase — the docs now describe logging this PR removes. That logging was itself the fix for a reported containment-expectation gap (#707); reverting it silently regresses operator safety visibility. Fix: restore the two log statements and the three tests (likely a squash/extraction mistake when collapsing the #566 branch into one commit), or — if the removal is intentional — say so in the body and updatedocs/codex-cli.mdto match.
Nits (optional)
- [tests] test/providers/test_codex_provider_unit.py
test_bounds_default_to_their_documented_settings— asserts onlymock_settings.called; it would still pass if the two settings were swapped (idle_gap ←provider_init_timeout, outer cap ←startup_prompt_handler_timeout). Consider asserting the resolved values (e.g. via the fake clock's observed exit behavior or by spying on the loop bounds) so the test pins what its name claims. - [correctness] providers/codex.py
_handle_trust_promptfailure path — on a pane that never shows a recognizable prompt, banner, or login menu, the handler now burns the fullprovider_init_timeout(60s default, previously 20s) beforeinitialize()waits up to anotherprovider_init_timeoutinwait_until_status, so worst-case init on an unrecognizable pane is ~2× the setting. This mirrorskimi_cli/antigravity_cliexactly, so it is consistent — noted only so the doubling is a known property, not a surprise.
Tests
Coverage of the new behavior is excellent and unusually well-argued: idle-gap-not-total-budget, gap reset after each answered prompt, gap-based early exit, login-menu early exit (prompt sent = nothing, no error logged), all four position-liveness cases for the v1 trust prompt (stale-above-menu, live-below-menu, interleaved stale+live, v1-between-two-menu-renders), the has_login conjunction defended in both directions (footer-only, menu-only, scrollback-only), dismissal-before-login-exit ordering, and the _frames helper that converts a would-be hang into a bounded assertion failure. The fake_clock module-replacement approach (vs. patching time.monotonic process-wide) is correct and its rationale documented. Layer 2b's longest-gap probe for initialize() is a genuine improvement over tick counting. The one regression: three tests covering the #707 logging are deleted along with the logging itself (see Blocking).
Verification
Dynamic verification did not return. The verifier (and all seven static reviewer agents) could not be dispatched this run — the agent store was unreadable from this session's sandbox (permission denied on all profile loads), so this review was synthesized by the supervisor from the diff, the PR worktree at head aae1ec20, upstream main (3eafb6d6), and the #566 review thread. The PR body's claims of 292 passed, 3 skipped (changed test files), 194 passed (independence check against main's launch/services), and clean black/isort were NOT independently executed. Manual check: cd <worktree> && pytest test/providers/test_codex_provider_unit.py test/providers/test_startup_handler_nonblocking.py -q.
Verdict
Request changes — restore the #723 logging (codexProfile-discarded WARNING, --yolo sandbox-inert INFO) and its three tests, or explicitly justify the removal in the PR body and update docs/codex-cli.md accordingly. The startup-handling work itself is merge-quality: once the revert is fixed, this deserves prompt approval.
…ranch's squash dropped Extracting the Codex half of awslabs#566 into a single commit silently reverted awslabs#723 (05f86a6), which is an ancestor of this branch: the WARNING when allowed_tools ["*"] forces --yolo and discards an explicit codexProfile, the INFO recording that ~/.codex/config.toml's [sandbox_*] tables -- including network_access -- are inert under --yolo, and the three tests that pin them. docs/codex-cli.md still documented the WARNING, so the branch shipped docs describing logging it had removed. Restored byte-identical to main; no startup-handling code is touched.
haofeif
left a comment
There was a problem hiding this comment.
Reviewed exact head 7a07efac214f819da54cbe9f04c7b1f8c6d99ac2 against base 3eafb6d64684650155a9b2c422ea1d85c3f39c18. The corrective commit restores #723's logging, and the repository-owned Codex workflow passes 283 tests / 3 skips on Python 3.10, 3.11, and 3.12. Two blocking correctness gaps remain:
-
[P1]
src/cli_agent_orchestrator/providers/codex.py:1209-1238— stale trust copy can still drive Enter into a different live menu. The new positional liveness check only protects the v1 trust branch whenhas_loginis true. With no login menu,(v1_is_live or not has_login)admits any v1 phrase anywhere in the full capture, so stale scrollback above a live update dialog takes the trust arm first and sends bare Enter, selecting update option 1 (Update now) instead of the later branch's3+ Enter. The v2 branch has the same omission for a live login menu: a stale v2 question aboveSign in with ChatGPTplus the menu's sharedPress enter to continuefooter satisfies lines 1227-1230 and selects a sign-in method. Both are independently reproduced at this head. Please derive liveness for every auto-answerable dialog from the active bottom block (or use a viewport-only capture) before sending keys; do not make the guard conditional on one competing menu. -
[P1]
src/cli_agent_orchestrator/providers/codex.py:1359-1426— the documented per-profile init timeout is ignored.initialize()still reads only the server-wide setting, even thoughAgentProfile.provider_init_timeoutandBaseProvider.get_init_timeout()define the profile value as replacing that hard cap. A Codex profile withprovider_init_timeout: 180and a 60-second server default passes 60 towait_for_shell,_handle_trust_prompt, andwait_until_status; the focused reproduction expected 180 and observed 60. That recreates the late-dialog/container timeout this change is intended to fix specifically for profiles that need the override. Load/reuse the profile and resolveinit_timeoutthroughget_init_timeout, as Claude/Kimi/Antigravity do, then cover all three consumers.
The current Security Scan failure is unrelated shared dependency baseline (fast-uri / qs), not this three-file delta.
…ings test_bounds_default_to_their_documented_settings asserted only mock_settings.called, so it passed with idle_gap and the outer cap resolved from each other's setting -- the exact mistake the idle-gap/outer-cap split exists to prevent, and the one thing its name claimed to pin. Replaced with two behavioural tests, one per bound: - the outer cap: a pane matching no prompt, banner or login menu, so only the cap can end the loop, with the clock crossing the gap setting (7s) but not the init setting (99s). Correct wiring leaves 2 reads (one in-loop, one from the cap's own diagnostic pane-tail read); a cap resolved to 7s leaves 1. - the idle gap: called the way initialize() calls it, outer cap explicit and gap defaulted, so the cap cannot mask which setting the gap read. Both fail under a swap of the two lookups (1 != 2 and 4 != 1). The gap test's clock ends at 700.0 past its explicit 600s cap on purpose, so the wrong case fails on the assertion instead of exhausting the clock and raising StopIteration from inside the coroutine.
|
@gutosantos82 Good catch, and you were right about the cause: it was a squash mistake, not an intentional removal. Restored at Collapsing the Codex half of #566 into a single commit dropped #723 ( Everything is restored byte-identical to The restore is its own commit, and it touches no startup-handling code. It can't happen silently again. Re-deleting the logging now fails The restore itself took the changed test files from the 292 passed in the PR body to 295, which is precisely the three restored tests. Your first nit is also fixed, at
Both fail under a swap of the two lookups ( 296 passed, 3 skipped after both commits, in fixed and in random order. On your second nit: the One thing you may want to know independently of this PR: the |
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 0e5e64233e9fe0b172e75a6c2817e5c014f2ee40 against base 3eafb6d64684650155a9b2c422ea1d85c3f39c18. The incremental commit from the previously reviewed head changes only tests; both implementation blockers remain and were independently reproduced again:
-
[P1]
src/cli_agent_orchestrator/providers/codex.py:1203-1245— stale trust text still sends Enter into a different live modal. Stale v1 text anywhere in scrollback bypasses positional liveness wheneverhas_loginis false, so it preempts a live update dialog and bare Enter selects the defaultUpdate now; stale v2 text can likewise borrow a live login menu's genericPress enter to continuefooter and select a login method. Exact-head reproductions observed Enter for both cases, while the safe update3path was never called. Identify the currently live bottom modal/viewport block before dispatching any key rather than adding another prose-specific exception. -
[P1]
src/cli_agent_orchestrator/providers/codex.py:1359-1360,1405,1423-1427— Codex still ignores per-profile initialization timeout. With a profile override of 180 and server default 60,get_init_timeout()is never called andwait_for_shell,_handle_trust_prompt, andwait_until_statusall receive 60. Load the profile, resolve the single hard cap throughBaseProvider.get_init_timeout(), and propagate it to all three consumers.
The focused suite passed 296 tests with 3 skipped; all three blocker reproductions still reproduce. The Security Scan failure is inherited dependency baseline and unrelated to this test-only increment.
Resolves the codex unit-test conflict by keeping both blocks added at the same spot: this branch's fake_clock helper and main's TestCodexCurrentComposer.
…d honour the profile's init timeout Review (haofeif, round 3), two P1s. 1. The startup-prompt handler tested each dialog's text on its own, so it pressed keys into whichever modal was actually live: stale v1 trust wording anywhere in the capture pressed Enter into a live update dialog (default item "Update now", a global npm install), and stale v2 wording borrowed a live login menu's shared footer and picked a sign-in method. _live_startup_block(bottom_region) now names the block drawn lowest in the bottom window -- trust, update, login or none -- locating each by the last occurrence of its signature and attributing the shared "Press enter to continue" footer to the block it is drawn under. The handler makes that one decision per frame and sends a key only to that block; the whole-capture v1 search and the has_login/v1_is_live machinery are gone. 2. initialize() read provider_init_timeout straight from settings, so a profile's own override never reached wait_for_shell, the handler or the readiness wait. It now resolves the single cap through BaseProvider.get_init_timeout(self._try_load_profile()), the same best-effort shape kimi_cli/antigravity_cli/grok_cli/minimax_code use. Tests: both reviewer reproductions at resolver and handler level, footer attribution, lowest-occurrence location, the profile override reaching all three waits, and the unloadable-profile fallback. Adds the missing CHANGELOG entry.
|
@haofeif Both fixed; head is P1 #1, stale trust text keyed into a live modal. You were right that another prose exception was the wrong shape. The handler now makes one decision per frame, before any key is sent: Your two reproductions are now tests at the resolver level and at the handler level: stale v1 wording above a live update dialog takes the Two objections a reader might raise against the rule, and why they do not hold. First, "shell output containing the trust wording could sit inside an update dialog block and win": the update dialog is one contiguous block the TUI draws, nothing interleaves inside it, and the handler runs at startup before any command output exists; the old whole-capture search fired on that same text anywhere in the pane, so the new rule is strictly narrower. Second, "a trust dialog stacked above a full-height login menu falls outside the 15-line window and is missed": in that frame the login menu was drawn after the dialog, so the menu is the live modal and returning settled is correct; pressing Enter into it is exactly the bug you reported. The window is also the one P1 #2, per-profile init timeout ignored. Also added the Verification (HOME sandboxed), on the branch merged with current |
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 4cb9dc2da79d56f171fa9ca82a36911061f30488 against base 948c3d8004faa9f2f61b1c65dcde19291d7e072b.
The profile-specific timeout and fully rendered wrong-modal cases from the earlier rounds are fixed, but three startup-state defects remain:
-
[P1] Do not act on a stale completed block while a lower modal is mid-redraw —
src/cli_agent_orchestrator/providers/codex.py:493-512,1257-1292._live_startup_block()excludes an incomplete lower update/login block but retains a complete stale trust block above it. A capture between redraw writes therefore routes the trust prompt's bare Enter into the new modal, potentially selecting "Update now" or a sign-in method. Any recognized lower incomplete block needs to be treated as transitional: send no key until the lowest block is complete and current. -
[P1] Apply positional liveness to the v1 trust status check —
src/cli_agent_orchestrator/providers/codex.py:1438-1450,1511-1524. A modern v2 trust transcript contains the v1 option wording. After dismissal, that wording remains in the rolling buffer and the whole-buffer v1 check continues returningWAITING_USER_ANSWEReven when the current pane is idle. Initialization accepts that stale status, while the initial-task input guard rejects delivery, leaving the worker parked on a prompt that no longer exists. -
[P2] Read a fresh frame before applying idle-gap expiry —
src/cli_agent_orchestrator/providers/codex.py:1232-1238,1268-1271,1293-1305. The loop checks the idle gap before its next pane read. With the valid one-second setting, the mandatory post-key sleep can expire the gap after dismissing trust, so an already-rendered follow-up update dialog is never observed or skipped. Exit should require a fresh frame confirming that no actionable dialog remains; live dialogs should remain governed by the outer cap.
…fresh frame, anchor the v1 trust check Review (haofeif, round 4), two P1s and a P2. 1. _live_startup_block named the complete block ABOVE a header that was still being drawn under it, so a capture between the TUI's redraw writes could send the trust dialog's bare Enter into the update dialog or sign-in menu appearing below. Such a frame is now "transitional": the handler sends no key and keeps every exit closed until the next read. Reachable only while trust_dismissed is false, i.e. when something other than the handler answered the prompt inside a poll interval; held regardless. 2. get_status matched the v1 trust wording anywhere in the text it was handed while the v2, update and login checks were bottom-anchored. The check predates this PR (7467dfc) but its consequence is a startup-state one: a dismissed v0.130+ dialog (v2 header over the v1 option text) could keep the terminal WAITING_USER_ANSWER, which initialize() accepts and the orchestrated-input guard then refuses to deliver into. Anchored to the same 15-line window as the others. 3. The idle-gap exit ran at the top of the loop, before the poll, and was not gated on a dialog still being on screen. With a one-second gap (the smallest the settings validator accepts) the mandatory post-dismissal sleep expired it before a follow-up dialog was ever read; with any gap, a dismissed block still rendered when the gap elapsed returned the handler onto it. The gap is now judged on the frame just read, after the block decision, and only a frame with no dialog on it can return. Seven tests fail on 4cb9dc2 and pass here. Four existing idle-gap tests read one more frame under the fresh-frame rule and say so; the menu-without-footer test now pins that the hold is per frame, not sticky.
|
@haofeif All three addressed; head is 1. Half-drawn lower block (P1) — fixed. One note on reach, not on the fix: the Enter you describe only fires while 2. Whole-buffer v1 trust check (P1) — fixed. 3. Idle gap judged before a fresh frame (P2) — fixed, and it was wider than the one-second case. The gap exit ran at the top of the loop, before the poll, and was not gated on a dialog still being on screen — so with any gap, a dismissed block still rendered when the gap elapsed returned the handler onto it. The comment above Tests. Nine tests at this head fail on |
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 5ae3dacabb9baccb5ddda6403ad5c185f0b23e33 against base 29b235cf62ed0f9d624bc9ad9afce09ab72f8ddf.
The original lower-partial-over-complete-modal trigger is fixed by the new transitional state, and reading a fresh frame before idle-gap expiry fixes the one-second follow-up-dialog case. The stale-status fix is incomplete, and three blocking state-machine gaps remain:
-
[P1] Treat the first incomplete modal as transitional even when no complete modal exists —
src/cli_agent_orchestrator/providers/codex.py:516-533,1324-1359._live_startup_block()returnsNoneimmediately when there is no complete candidate, before checking whether a lower update/login/v2-trust header has started drawing. If the previous idle composer is still present, initialization accepts that stale composer as ready while the new modal is mid-redraw. Deferred input can then be pasted into the modal and its terminating Enter can select the default update/login action or lose the task. The transitional check must also cover a lone incomplete recognized block. -
[P1] Let the live composer outrank stale modal text above it —
src/cli_agent_orchestrator/providers/codex.py:516-534,1274-1324,1552-1579. After modern trust is dismissed, its v1 option wording can remain directly above the live composer inside the 15-line window. Because the composer does not participate in_live_startup_block()ordering, the stale text still wins astrust;get_status()returnsWAITING_USER_ANSWER, initialization accepts it, and the initial-task guard refuses delivery. If an operator dismissed the prompt between handler polls, the handler can also send a stray Enter into an existing composer draft. Moving the test from the whole buffer to a fixed tail only removes distant stale text; current-screen ordering still has to account for the composer. -
[P2] Do not convert an auto-dismissible prompt timeout into successful initialization —
src/cli_agent_orchestrator/providers/codex.py:1363-1377,1470-1477,1554-1579. When trust or update remains visible because its dismissal key was ignored or redraw stalled, the handler reachesprovider_init_timeout, logs an error, and returns normally.get_status()reports the still-live dialog asWAITING_USER_ANSWER, andinitialize()accepts every waiting status even though only the login menu is intended to be a successful human-action state. The result isinitialize() == Trueafter its own hard-cap failure, followed by assign/handoff delivery being rejected. Login readiness must be distinguished from unresolved trust/update prompts.
The earlier logging-restoration, fully rendered wrong-modal, profile-timeout precedence, and distant stale-v1 cases are fixed at this head.
|
Correction/clarification to my review at The first two items should be counted as one P1 root-cause finding, not two independent P1s. Both are opposite failures of the same invariant: the live composer is absent from the positional startup-state model.
The complete fix is not another wording-specific exception. Resolve one lowest current startup state across The outer-cap finding remains a separate P2: exhaustion is logged but not propagated, so a persistent trust/update modal can be accepted through the login-only Correct current count: 1 P1 + 1 P2. The change request remains because the P1 still defeats the startup-safety contract. |
gutosantos82
left a comment
There was a problem hiding this comment.
PR #731 — fix(codex): startup-prompt handling (split from #566)
- Head reviewed:
5ae3dacabb9baccb5ddda6403ad5c185f0b23e33 - Base:
main - Author: tedswinyar
- Size: 4 files, +1,776 / −72 (CHANGELOG.md,
providers/codex.py,test_codex_provider_unit.py,test_startup_handler_nonblocking.py) - State at review time: OPEN,
mergeable: MERGEABLE,reviewDecision: CHANGES_REQUESTED - Pipeline: all 7 specialist reviewers + verifier completed (native-subagent fallback; CAO agent-store was unreadable from this sandbox)
Verdict
Request changes — deferring to the standing human decision. Maintainer
@haofeif's fourth CHANGES_REQUESTED review (5203944607) was filed against this
exact head and explicitly reaffirmed ten minutes later: "The change request
remains because the P1 still defeats the startup-safety contract." No commit
has landed since. Approving would directly conflict with a live human
maintainer decision. The other blocking reviewer (@gutosantos82) has been fully
addressed (byte-identical #707/#723 logging restore at 7a07efa, mutation-tested;
test nit fixed at 0e5e642).
The engineering itself is strong: every one of the five claimed defect fixes
holds under both static inspection and dynamic verification, and the security
posture is a net improvement.
Human maintainer conditions (unaddressed at this head)
- [P1 — haofeif] The live composer is absent from the positional
startup-state model. Two opposite failures of one invariant: an incomplete
modal below a stale composer lets the composer win (startup declared ready
mid-redraw); stale modal text above a live composer lets the modal win
(status falsely WAITING). Prescribed fix is architectural: resolve ONE lowest
current startup state across composer/trust/update/login/transitional and use
it in BOTH_handle_trust_prompt()andget_status(). - [P2 — haofeif] Outer-cap exhaustion is logged but not propagated:
a persistent trust/update modal is accepted through the login-only
WAITING_USER_ANSWER success path (initialize()returns True after its own
hard-cap failure). - Fairness caveat, in the reviewer's own words: the destructive-input frame is
plausible but not established by a committed live capture; the classifier
defect is deterministically reproduced, only real-world frequency is unproven.
#566 carryover checked: its remaining live conditions target the delivery half
that stays in #566 (files this PR does not touch) and do NOT carry over. The one
codex-half condition — shared-module test placement per #509 — is satisfied.
Verified claims (dynamic — verifier ran in the PR worktree)
- Two changed test modules: 323 passed, 3 skipped, 0 failed (body says 292;
head is a merge of main, which added tests — e.g. #707 codexProfile warnings). black --check/isort --check-onlyon the three changed .py files: clean.- Reverting the
asyncio.to_threadoffload fails exactly the two
loop-starvation tests (event loop stalled for 0.413s); restoring passes. - Worktree left clean (
git status --porcelainempty).
All five claimed fixes independently confirmed by the correctness reviewer:
idle-gap + outer cap (codex.py:1240-1243), login early-return leaving the pane
alive (:1344 → WAITING_USER_ANSWER), scrollback-trust vs live-login (:519),
position-based v1 liveness (:483), full to_thread offload. The handler's safety
invariant (never return while a live dismissable/mid-draw block is at pane
bottom; never key anything but the lowest live block) checks out against
stacked, interleaved, footer-borrowing, and half-drawn frames.
Findings by severity
Critical
- None.
Major
- [tests] ~370s CI-cost regression in six pre-existing init tests.
initialize()now derives the handler's outer cap from
provider_init_timeout(60s default) instead of the old hard-coded 20.0.
Six tests that feed a non-settling frame without stubbing
_handle_trust_promptnow loop with realasyncio.sleep(1.0)to the full
cap — measured 62.10s each (test_initialize_success:97,
test_initialize_codex_timeout:133,test_initialize_with_trust_prompt
:2740,_v2,_captures_shell_baseline,
_includes_waiting_user_answer_in_target_status). The PR's own new init
tests correctly stub with AsyncMock; the same treatment (or patching
codex.asyncio.sleep) should be applied to these six. - [consistency] "Mirrors kimi_cli/antigravity_cli" overstates parity.
codex.py:1335 guards the idle-gap exit withnot has_dialogevaluated
AFTER the frame read — the round-4 fix. The three siblings
(antigravity_cli.py:579, kimi_cli.py:549, claude_code.py:673) still check
the gap at loop top on the clock with nohas_dialogguard, i.e. still
carry the defect this PR fixes for codex. Out of scope to fix here, but
soften the docstring claim or file a backport follow-up. - [security] Unauthenticated credential-entry pane now held open on a
default-unauthenticated control plane. The login early-return
(codex.py:1348-1353) + WAITING_USER_ANSWER-as-success is the correct fix
for the old teardown bug, but the live sign-in pane (OAuth device code /
API-key paste) is attachable via the unauthenticated Web UI/PTY WebSocket
on localhost:9889. Recommend a doc note: enable control-plane auth before
launching Codex without pre-provisioned credentials. Largely inherent to
CAO's shared-terminal model — awareness, not a regression.
Minor
- [correctness] Residual false-WAITING window in
get_statusv1 check
(codex.py:1554): matches TRUST_PROMPT_PATTERN anywhere in the bottom 15
lines with no positional/composer check — the round-4 failure mode shrunk
from whole-capture to 15 lines, not closed. This is the concrete face of
haofeif's P1. Tests cover only the pushed-out-of-window case. - [correctness] Outer cap does not bound a single wedged backend call
(codex.py:1257-1260):to_threadfutures aren't cancellable; a hung tmux
subprocess hangs the handler pastouter_timeout. Low likelihood,
pre-existing to the offload pattern. - [security]
pane_tailERROR logging on outer-cap timeout can leak a
mid-paste API key (codex.py:1363-1376). Recommend masking secret shapes
or demoting pane content. - [security] v1 trust match auto-presses Enter on a bare substring with no
corroborating footer (codex.py:493-499) — the weakest signature among
the four block types; consider requiring the option-1 line or footer. - [consistency] Stale docstring:
_handle_trust_promptstill documents a
removedtimeoutparam (codex.py:1214; signature is now
(idle_gap, outer_timeout)). - [consistency] test_startup_handler_nonblocking.py:22-24 says claude_code
is excluded because "#451 is not merged" — at head, claude_code is
already converted (claude_code.py:676) yet absent from the parametrized
case lists. Reconcile #451's status. - [conventions] docs/configuration.md:141-142 stale provider
enumeration: codex now usesprovider_init_timeoutas the outer cap and
idle-gap semantics forstartup_prompt_handler_timeout, but the doc's
provider lists weren't updated (CODEBASE.md doc-maintenance rule). - [tests] Uncovered changed branch codex.py:1256-1258 (
if not output
empty-frame path) — only substantive uncovered changed line (~95%
changed-line coverage otherwise). - [tests] Two timing-sensitive tests rely on real wall clock (layer-2b
max-gap probe with 0.1s threshold vs 0.2s call; the 62s init tests) — can
flake under CI load. - [consistency] PR body's "five defects" under-enumerates: per-profile
provider_init_timeoutgovernance (codex.py:1406,1476-1479) and
bottom-anchored v1 status wording (:1551) are in the diff/CHANGELOG but
not the body's list.
Nit
- Idle-gap exit ordering makes the login-branch log partly unreachable
(codex.py:1329 vs :1344) — functionally identical, cosmetic. - Transitional detection pairs "trust" with
v2only (codex.py:531-536) —
correct by construction; worth a comment. - Magic number: codex.py:1551 uses literal
[-15:]where siblings use
STARTUP_PROMPT_BOTTOM_LINES. mock_tmuxparam name for aget_backendpatch in older tests; fake_clock
sequences positionally coupled totime.monotonic()call count (documented
by the author); fixture duplication across four test classes.- Title is valid Conventional Commits but ~91 chars; CHANGELOG bullet is one
dense seven-clause sentence (house style, optional split).
Positives
- Security net-positive: update dialog answered with '3' (Skip) instead of
blind Enter, and the transitional-frame hold prevents Enter landing on
"1. Update now (runs npm install -g @openai/codex)" — avoids a global npm
install swapping the codex binary under all workers. No new
command-injection or path-traversal surface; MCP config keys validated;
dev-instructions file 0o600 + shlex-quoted. - Tests: every one of the 5 headline fixes plus the round-4 additions has
a regression-catching test; #509 shared-module pattern followed and
justifiedly extended (layer-2b max-gap probe). - Vision: STRONG FIT — provider-parity plumbing that keeps a frontier CLI
reliably orchestrable; ports #451's fix via the shared parametrized
invariant, lowering the cost of the next provider. - Scope discipline: mask-surface isolation verified (zero references to
reported_status/initial_delivery_pending/_pending_initial_delivery);
independence-from-#566 claim consistent with the file set.
Suggested path to approval
- Land haofeif's P1 (single lowest-current-startup-state resolver shared by
_handle_trust_prompt()andget_status()) and P2 (propagate outer-cap
exhaustion instead of accepting via the login-only success path). - Stub
_handle_trust_prompt(AsyncMock) in the six slowed init tests. - Sweep the stale docstrings/doc enumeration (items 8-10) in the same pass.
…that outlives the cap Review (haofeif, round 5): one P1, one P2. Plus gutosantos82's review of the same head. P1. The live composer was absent from the positional startup-state model, so one invariant failed in both directions: a header being drawn below a stale composer resolved to None and the composer exit declared startup ready mid-redraw; trust wording a dismissed v0.130+ dialog leaves above the live composer still resolved to "trust", get_status stayed WAITING_USER_ANSWER, initialize() accepted it and the initial task was refused delivery (an operator answering between polls also drew a stray Enter into the composer). _live_startup_block now resolves one lowest current state across composer, trust, update, login and transitional -- a lone half-drawn header is transitional even with nothing complete above it -- and get_status uses the same resolver for its startup checks, so a frame cannot be ready to the handler and waiting to the detector. P2. The handler's cap exhaustion was logged and swallowed; a trust/update dialog whose dismissal key was ignored then read as WAITING_USER_ANSWER and initialize() succeeded through the path meant for the login menu. The handler returns "settled" / "blocked" / "unresolved"; initialize() raises on "blocked" and, after the readiness wait, re-resolves the pane and raises if a trust/update dialog or a mid-redraw modal is what satisfied it. The login menu remains a successful start. gutosantos82: six init tests fed a banner-only frame and polled with real sleeps to the 60s cap (~62s each); they now see a settled frame, or a trust-then-settled sequence with sleep patched, and the file runs in ~12s. The handler docstring names outer_timeout rather than a removed timeout parameter and no longer claims kimi_cli/antigravity_cli share the fresh-frame idle-gap rule; docs/configuration.md lists Codex under both settings; get_status uses STARTUP_PROMPT_BOTTOM_LINES rather than a literal 15; the nonblocking test header no longer says awslabs#451 is unmerged. The handler's readiness exit keys off the resolver's own "composer" answer (with _has_startup_idle_composer as the activity veto), so both consumers use the one result. docs/codex-cli.md carries the note gutosantos82 asked for: the first-run sign-in pane the handler leaves open is attachable through the Web UI/PTY WebSocket, so enable control-plane auth before launching Codex workers that will need to sign in interactively. 12 tests fail on 5ae3dac and pass here; three are controls.
|
@haofeif @gutosantos82 Both round-5 items addressed, plus the test-cost finding and the doc note; head is P1 — the composer is now a startup state. You're right that the round-4 anchor only moved the stale wording out of reach instead of putting the composer into the ordering. P2 — a dialog that outlives the cap fails initialisation. gutosantos82's items. Seven pre-existing init tests polled with real sleeps to the 60-second cap — four fed a banner-only frame, two a trust dialog that never cleared, one an exhausted frame list — and the six you named are among them. They now see a settled frame, or a dialog-then-settled sequence with Tests. 12 cases fail on |
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #731 — fix(codex): startup-prompt handling that can't live-lock or block the loop (split from #566)
Summary
This PR fixes seven Codex startup-handling defects behind one positional startup-state resolver (_live_startup_block), offloads blocking tmux I/O off the event loop, and honors per-profile provider_init_timeout — all seven claimed fixes were independently verified against the checked-out head, and the prior review rounds' P1/P2 (composer in the positional model; outer-cap propagation) are implemented with revert-failing tests. However, the shared resolver introduces one new instance of the bug class this PR set out to kill: the transitional state, correct as a hold semantic inside the startup handler, is over-eager on the always-on get_status path, where a lone or model-quoted dialog header mid-turn now classifies as WAITING_USER_ANSWER (main returned PROCESSING for the same frame). That should be fixed before merge; with it addressed, this is a strong, well-tested, well-scoped change.
Blocking (must fix before merge)
- [correctness] src/cli_agent_orchestrator/providers/codex.py —
transitionalleaks a false WAITING_USER_ANSWER into runtimeget_status. 🆕_live_startup_blockreturns"transitional"for a lone recognised header (v2 trust wording, "Update available! X -> Y", "Sign in with ChatGPT") with no completing footer/menu, andget_statusmaps that to WAITING_USER_ANSWER.get_statusruns continuously for the whole session, so at runtime this fires on quoted/streamed model output containing those phrases, not just a real modal mid-redraw. Reproduced against both trees: a frame with the v2 trust sentence inside an assistant reply mid-turn returns PROCESSING on main but WAITING_USER_ANSWER at this head; same flip for the sign-in and update phrases. The startup-state check also sits before (and ignores) the assistant-after-last-user gate. The composer usually wins on position at true idle, which mitigates the COMPLETED-masking case, but mid-turn (or when the composer is out of the captured window on a long response) a supervisor sees a worker as blocked-on-input while it is generating. Suggested fix: keeptransitionalfor the handler, but gate the transitional→WAITING mapping on the runtime path (e.g. only honour it during initialization, or require corroboration such as the dialog footer/menu before classifying WAITING). This lands in provider status-detection — the highest-risk path in the repo and the exact class the previous six rounds were about — hence classified blocking rather than important.
Important (should fix)
- [tests] test/providers/test_codex_provider_unit.py — the test-cost concern is only partially closed; ~16 new or PR-touched tests still pay real sleeps (~30s of the ~47s module runtime). The clock-sensitive idle-gap tests correctly use
fake_clockwith patchedasyncio.sleep, but the newTestCodexInitConfiguredTimeouts,TestCodexInitHonoursProfileTimeout, andTestOuterCapWithADialogUpFailsInitializationclasses leaveinitialize()'s 2.0s warm-up unmocked,test_handle_trust_then_late_update_dialogreads four frames at a real 1s each, and several new handler tests pay real 1–2s poll sleeps. Applying the same patch pattern the PR already uses elsewhere would cut ~30s. (Builds on the test-cost point haofeif raised in round 5 — the previously flagged tests were fixed; this applies the same treatment to the tests added since.) - [correctness] src/cli_agent_orchestrator/providers/codex.py —
initialize()'s post-readiness_current_startup_state()is a single-shot read with no ride-out. Becausetransitionalfires on a lone header, a login-menu worker caught mid-redraw at that one instant (header drawn, footer not yet) spuriously fails an otherwise-valid login start with TimeoutError, where the handler's own loop rides such frames out. Narrow window, low likelihood; a re-read or small retry would match the handler's tolerance. - [consistency] PR body accuracy — "touches only
providers/codex.pyand its two test modules" / "the two halves share no files" overstates the separation. The diff touches six files including CHANGELOG.md, docs/codex-cli.md, and docs/configuration.md, and CHANGELOG.md is almost certainly also edited by #566, so a rebase conflict there is expected. The spirit of the claim (no shared source, no mask-surface overlap — verified:reported_status/initial_delivery_pending/_pending_initial_deliveryappear nowhere in this diff) holds; suggest softening to "share no source files."
Nits (optional)
- [conventions] src/cli_agent_orchestrator/providers/codex.py:448 —
offsets: List[int] = []butListis never imported (the import line addsDictonly). Harmless at runtime (local-variable annotations are not evaluated) but a new mypy/pyflakesname-definederror, and it contradicts the PR's "introduces no new mypy errors" note. Cheapest fix matching file style (lines 1738/1752 use lowercase builtins):offsets: list[int] = []. - [consistency] src/cli_agent_orchestrator/providers/codex.py:148 —
UPDATE_DIALOG_FOOTERis now fully dead (its only consumer,_has_update_dialog_in_bottom, was removed); delete it.LOGIN_MENU_FOOTER(:128) survives only for tests — consider moving or removing. - [tests] test/providers/test_codex_provider_unit.py — the
frames/_frameshelper is defined four times (module-level plus three identical staticmethods); consolidate on the module-level helper. - [tests] PR body — the "292 passed, 3 skipped" count appears stale: the two modules yield 335 passed, 3 skipped at this head. Worth re-confirming the number.
- [consistency] docs/configuration.md — the handler list "(Claude Code, Kimi, Antigravity, Codex)" still omits Copilot (pre-existing omission), and "fails initialisation if a trust or update dialog is still on screen" omits the transitional case — both harmless simplifications.
- [security] advisory — the first-run sign-in pane is deliberately left alive and is attachable via Web UI / PTY WebSocket; the docs note covers it, but a launch-time warning when a login pane is left alive with control-plane auth disabled would close the gap for operators who miss the doc.
Tests
Coverage is strong: each of the seven claimed fixes has at least one test that fails if the fix is reverted, including discriminating assertions (occurrence-position pins, send-keys counts, per-wait timeout propagation). The non-blocking startup tests are correctly placed in the shared parametrized test_startup_handler_nonblocking.py per the #566 review ask, and the new layer-2b worst-gap probe is a sound, justified addition (a tick-count probe cannot detect the stall once initialize()'s own sleeps tick the loop). Markers, mocked backends, and the fake_clock/frames() fixture design are all correct. Running both touched modules at this head: 335 passed, 3 skipped (pre-existing live-provider gates), 1 environmental failure unrelated to the diff (sandbox PermissionError in a non-startup profile test). Remaining gap: the real-sleep cost noted under Important.
Verification
Dynamic verification did not return by synthesis time; the tests reviewer independently executed both touched test modules against the PR head (335 passed / 3 skipped / 1 environmental failure unrelated to this diff), and the correctness reviewer reproduced its Blocking finding with concrete frames run against both this head and main. The PR's "292 passed" figure could not be reproduced (335 observed); the independence claim (194 passed against main's launch/services) was not independently re-run.
Verdict
Request changes — one introduced defect should be fixed before merge: the transitional startup state leaks into runtime get_status and classifies quoted/streamed dialog wording mid-turn as WAITING_USER_ANSWER (a behavior change from main on the always-on status path). The seven claimed fixes are otherwise verified, prior review conditions from @haofeif and @gutosantos82 appear addressed at this head with revert-failing tests, and the open change request from @haofeif awaits his re-review of 3fd0541; the remaining items (real-sleep test cost, single-shot post-readiness read, PR-body wording, List import) are small and well-understood.
There was a problem hiding this comment.
Re-reviewed corrective commit 3fd05416e7e27ae7413f45c84633dd1176285895 against base 29b235cf62ed0f9d624bc9ad9afce09ab72f8ddf, including the changes since my previous review at 5ae3dacabb9baccb5ddda6403ad5c185f0b23e33.
Both previous root causes are fixed: the composer now participates in the shared positional startup model, and a persistent dismissible startup dialog no longer passes initialization through the login-only waiting path. I am not retaining those earlier P1/P2 findings.
Requesting changes for one P3 regression introduced by the corrective commit: an ambiguous startup transitional frame is now classified as a real runtime user-input request. The inline comment gives the exact trigger and its propagation through the public status/input path. This independently corroborates the current-head runtime-status concern already raised in review 5216945037; it is not an additional duplicate finding.
| # #731). Bottom-anchored: a live dialog is in view by definition. | ||
| bottom_region = "\n".join(clean_output.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) | ||
| startup_state = _live_startup_block(bottom_region) | ||
| if startup_state in ("trust", "update", "login", "transitional"): | ||
| return TerminalStatus.WAITING_USER_ANSWER |
There was a problem hiding this comment.
[P3, non-blocking] Keep transitional startup headers out of runtime status detection
Priority correction after tracing the completion consumers: my original P2 rating overstated the demonstrated impact.
At 3fd05416e7e27ae7413f45c84633dd1176285895, an initialized Codex terminal can still be classified as WAITING_USER_ANSWER when assistant prose quotes a startup header without a later recognized composer. The constructed active-turn frames include a processing indicator; the previous head classifies them as PROCESSING. The public status monitor can retain this false waiting state through subsequent processing detections. This remains a real status defect and can produce a spurious approval card where the AG-UI approval bridge is active.
However, the monitor permits transitions from waiting to COMPLETED and IDLE. The normal handoff completion waiter and CLI completion poller continue through waiting and complete successfully when a real completed or stable-idle state arrives. Inbox delivery already waits during ordinary Codex processing, so the earlier refusal of a direct ASSIGN into an already-busy worker does not by itself demonstrate lost work. The approval bridge also expires the card when the terminal leaves waiting.
The demonstrated impact is therefore misleading mid-turn status/approval UI, not an established permanent stall or normal orchestration failure. Keep startup-only transitional handling out of continuous runtime detection, but this is a P3 follow-up rather than a merge-blocking P2.
haofeif
left a comment
There was a problem hiding this comment.
Priority correction and updated decision for base 29b235cf62ed0f9d624bc9ad9afce09ab72f8ddf / head 3fd05416e7e27ae7413f45c84633dd1176285895.
I am downgrading my sole remaining finding from P2 to P3, non-blocking, and changing my review to APPROVE. The inline comment has been corrected: #731 (comment)
The false runtime waiting classification is real, but my earlier impact assessment did not sufficiently account for recovery: StatusMonitor accepts subsequent COMPLETED/IDLE, and the shared handoff completion waiter and CLI poller continue through waiting and finish normally on completed/stable-idle states. Inbox delivery is already withheld while Codex is processing. The direct busy-worker ASSIGN rejection therefore did not establish normal task loss or a permanent orchestration stall. A transient false status/approval indication remains; the current evidence does not justify blocking on it.
Both previously reported startup roots remain fixed. My corrected current-head tally is 0 P1/P2 and 1 non-blocking P3. This supersedes my September 16 change request; it does not dismiss another reviewer or claim the remaining note is fixed.
Extracted from #566. This is the Codex-provider half; the server-side initial-message delivery and its non-completable mask stay there.
Why split
#566 had grown to +2,400 lines across 8 files and 13 commits, combining two independent state machines behind one review. Six review rounds each found another instance of the same causal-status bug in the delivery half, while the Codex startup work — reviewed and settled several rounds ago — sat behind it unable to land.
The two halves share no files. This PR touches only
providers/codex.pyand its two test modules; nothing here references the mask surface (reported_status/initial_delivery_pending/_pending_initial_delivery). Verified rather than assumed: withmain's unmodifiedlaunch.py,test_launch.py+test_terminal_service_full.py+test_status_monitor.pygive 194 passed.What's fixed
Seven defects in Codex startup handling, all reachable on a default
cao launch:startup_prompt_handler_timeoutwas a total budget, not an idle gap. A handler that kept seeing new frames burned the whole allowance and returned late. Now an idle gap with a separate outer cap, so a live dialog is answered promptly and a quiet pane still exits.initialize()then deleted the worker before an operator could reach the session. It now returns as soon as the menu is recognised, leaving the pane alive and answerable.claude_code, which codex still had. Offloaded viaasyncio.to_thread; reverting the offload fails two loop-starvation tests.provider_init_timeoutwas ignored by every Codex initialisation wait.wait_for_shell, the startup handler's outer cap and the readiness wait all read the server default; a containerised profile that declared 180 got 60. All three now resolve throughBaseProvider.get_init_timeout.WAITING_USER_ANSWER. One positional resolver now names the lowest current startup state — idle composer, trust, update, login, or a modal mid-redraw — and both the handler andget_statususe its answer; a dialog still on screen at the handler's cap fails initialisation instead of passing through the login menu's success path.Verification
test_codex_provider_unit.py+test_startup_handler_nonblocking.py: 292 passed, 3 skipped.main's launch/services.black --checkandisort --check-onlyclean (615 files).Note on test placement
The non-blocking startup tests live in
test_startup_handler_nonblocking.py, the shared parametrized module from #509, rather than in the codex test file. That placement was a review ask on #566 and is carried forward here deliberately.What stays in #566
Server-side
initial_messagedelivery viaPOST /sessions, the_pending_initial_deliverymask and itsreported_statuswiring. That half has an open P1 from @haofeif — confirmation accepts aCOMPLETEDcached before dispatch, so the mask can clear before the new task emits activity — which needs a generation-stamped status observation rather than another release-point change. Keeping it separate is what lets that be designed properly instead of patched a fourth time.