diff --git a/CHANGELOG.md b/CHANGELOG.md index b13586fbe..e3f1fac4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Codex startup handling could stall a launch, or press keys into the wrong + dialog.** `startup_prompt_handler_timeout` is now the idle gap between + startup prompts, with `provider_init_timeout` as the hard cap, so lowering + the gap for another provider no longer truncates Codex's handler; the + first-run sign-in menu is recognised as a settled state instead of running + the handler to its cap; the handler now decides once per frame which startup + block is actually live at the bottom of the pane and sends a key only to + that block, so stale trust text left in scrollback can no longer answer a + live update dialog or sign-in menu; a frame in which a further dialog is + still being drawn is held rather than keyed, whether or not a complete + dialog is on screen above it; the idle gap is judged on a freshly read frame + with no dialog on it, not on the clock alone; the idle composer takes part in + that same positional decision and status detection uses the same resolver, so + trust wording a dismissed dialog leaves above the live composer no longer + reports `WAITING_USER_ANSWER` and a modal arriving below a stale composer no + longer reads as ready; initialisation fails, instead of succeeding through + the login menu's `WAITING_USER_ANSWER` path, when a trust or update dialog is + still on screen at the handler's cap or after the readiness wait; a + profile's own `provider_init_timeout` now governs every Codex initialisation + wait; the resolver's mid-redraw ("transitional") reading is honoured only + until initialisation is over, so assistant prose quoting a startup phrase + mid-turn no longer flips a processing terminal to `WAITING_USER_ANSWER` on + the runtime status path; and the post-readiness check re-reads a mid-redraw + frame a few times instead of failing an otherwise-valid login start on a + single capture (#731) + - **enabling `CAO_MEMORY_API_URL` rejected memory keys that work without it.** The `/internal/memory/store` and `/forget` routes validated the wire `key` as the strict `MemoryKey` (`^[a-z0-9-]{1,60}$`), while the MCP tools have always diff --git a/docs/codex-cli.md b/docs/codex-cli.md index 0758d29b6..c1b3e081f 100644 --- a/docs/codex-cli.md +++ b/docs/codex-cli.md @@ -22,6 +22,13 @@ export OPENAI_API_KEY=your-key-here codex login ``` +> **Security note.** If Codex is launched under CAO without credentials, the +> startup handler leaves the first-run sign-in menu on screen for an operator to +> complete (it cannot be answered automatically). That live sign-in pane — OAuth +> device code or API-key paste — is attachable through the Web UI and the PTY +> WebSocket like any other CAO terminal, so enable control-plane authentication +> before launching Codex workers that will need to sign in interactively. + ### Using Codex Provider with CAO Create a terminal using the Codex provider: diff --git a/docs/configuration.md b/docs/configuration.md index a86f89c51..ca287983f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -138,8 +138,8 @@ Timeouts and buffer sizes used by the CAO runtime. All values have safe defaults |---------|---------|--------------| | `mcp_request_timeout` | `30` | Seconds to wait for HTTP calls between the MCP server process and the CAO API. | | `event_bus_max_queue_size` | `1024` | Max events buffered per subscriber queue in the internal event bus. | -| `provider_init_timeout` | `60` | Seconds to wait for a CLI agent to reach IDLE. Also the hard outer cap on total time a startup-prompt handler (Claude Code, Kimi, Antigravity) may run. Overridable per-profile via `provider_init_timeout` in the agent profile — see [Agent Profile Format](agent-profile.md#optional-fields). | -| `startup_prompt_handler_timeout` | `20` | Idle gap, in seconds, between consecutive startup prompts (e.g. workspace trust / bypass dialogs, Kimi's upgrade dialog, Antigravity's trust/survey dialogs). The handler polls and resets this timer each time it answers a prompt; it only starts counting once the FIRST prompt has been handled, so a first dialog arriving later than this value (e.g. a cold/containerized start) is still caught — before any prompt is seen, only `provider_init_timeout` bounds the wait. Once at least one prompt has been handled, the handler exits after this many seconds pass with no further prompt. | +| `provider_init_timeout` | `60` | Seconds to wait for a CLI agent to reach IDLE. Also the hard outer cap on total time a startup-prompt handler (Claude Code, Kimi, Antigravity, Codex) may run; Codex fails initialisation if a trust or update dialog (or a startup modal still mid-redraw after a short ride-out) is on screen when the cap is reached or after the readiness wait. Overridable per-profile via `provider_init_timeout` in the agent profile — see [Agent Profile Format](agent-profile.md#optional-fields). | +| `startup_prompt_handler_timeout` | `20` | Idle gap, in seconds, between consecutive startup prompts (e.g. workspace trust / bypass dialogs, Kimi's upgrade dialog, Antigravity's trust/survey dialogs, Codex's trust and update dialogs). The handler polls and resets this timer each time it answers a prompt; it only starts counting once the FIRST prompt has been handled, so a first dialog arriving later than this value (e.g. a cold/containerized start) is still caught — before any prompt is seen, only `provider_init_timeout` bounds the wait. Once at least one prompt has been handled, the handler exits after this many seconds pass with no further prompt. | | `state_buffer_max` | `32768` | Bytes of raw terminal output `StatusMonitor` keeps per terminal for raw-path status detection and `GET /terminals/{id}/output` (`mode=full`). Not unbounded scrollback — a long, chatty session is truncated to this trailing window; raise it if a still-pending prompt is getting evicted before it's read back. | ### Memory (`memory`) diff --git a/src/cli_agent_orchestrator/providers/codex.py b/src/cli_agent_orchestrator/providers/codex.py index 9dcb8c883..55d7802ca 100644 --- a/src/cli_agent_orchestrator/providers/codex.py +++ b/src/cli_agent_orchestrator/providers/codex.py @@ -7,7 +7,7 @@ import shlex import time from pathlib import Path -from typing import Any, Optional +from typing import Any, Dict, Optional from cli_agent_orchestrator.backends.registry import get_backend from cli_agent_orchestrator.constants import CAO_HOME_DIR @@ -145,8 +145,12 @@ # actually contains either way. UPDATE_DIALOG_PATTERN = r"Update available!\s+[\w.+-]+\s+->\s+[\w.+-]+" UPDATE_DIALOG_MENU_PATTERN = r"Skip until next version" -UPDATE_DIALOG_FOOTER = TRUST_PROMPT_FOOTER STARTUP_PROMPT_BOTTOM_LINES = 15 +# How many times initialize()'s post-readiness check re-reads a frame that +# resolves to "transitional" (a header mid-redraw) before failing the start. +# 6 reads 0.5s apart give a redraw ~3s to complete -- generous for a TUI +# write, and small next to provider_init_timeout. +POST_READINESS_TRANSITIONAL_REREADS = 6 STARTUP_ACTIVITY_PATTERN = r"^\s*•[^\S\n]+\S" # Codex's runtime approval prompt as actually rendered by codex-cli 0.147.0, # verified against a live tmux capture (test/providers/fixtures/ @@ -435,14 +439,144 @@ def _toml_override(key: str, value: Any) -> str: raise TypeError(f"codexConfig key '{key}': {exc}") from exc -def _has_update_dialog_in_bottom(clean_output: str) -> bool: - """Return True when Codex's update-available dialog is active in the bottom region.""" - bottom = "\n".join(clean_output.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) - return ( - re.search(UPDATE_DIALOG_PATTERN, bottom) is not None - and re.search(UPDATE_DIALOG_MENU_PATTERN, bottom) is not None - and re.search(UPDATE_DIALOG_FOOTER, bottom) is not None - ) +def _composer_position(bottom_region: str) -> Optional[int]: + """Offset of the idle composer in ``bottom_region``, or None when none is drawn. + + The composer has two signatures, the same two ``_has_startup_idle_composer`` + accepts: a bare prompt line (``›`` with nothing after it -- a dialog's own + ``› 1. Yes, ...`` selector line does not qualify) or, from Codex 0.145, a + known placeholder line with the TUI status footer somewhere below it. The + LAST such line is the composer; earlier ones are history. + """ + lines = bottom_region.split("\n") + offsets: list[int] = [] + position = 0 + for line in lines: + offsets.append(position) + position += len(line) + 1 + composer: Optional[int] = None + for index, line in enumerate(lines): + if re.match(IDLE_PROMPT_STRICT_PATTERN, line): + composer = offsets[index] + elif re.match(STARTUP_IDLE_PLACEHOLDER_PATTERN, line) and any( + re.search(TUI_FOOTER_PATTERN, later) for later in lines[index + 1 :] + ): + composer = offsets[index] + return composer + + +def _live_startup_block(bottom_region: str) -> Optional[str]: + """Name the startup state currently drawn lowest in the pane, if any. + + Returns ``"composer"`` (the idle composer), ``"trust"`` (workspace-trust + dialog, either wording), ``"update"`` (update-available dialog), ``"login"`` + (first-run sign-in menu), ``"transitional"`` (a recognised header is being + drawn below everything else complete, so the frame is mid-redraw and no key + is safe) or ``None``. ``_handle_trust_prompt`` and ``get_status`` both take + exactly one decision per frame from this, so a frame cannot be "ready" to + one and "waiting" to the other. ``"transitional"`` carries startup-only + weight: it is a hold for the handler and for ``initialize()``'s checks, but + ``get_status`` ignores it once initialization is over, because on a live + terminal a lone header is the model quoting a startup phrase, not a modal + (see the comment at ``get_status``'s startup check). + + THE COMPOSER IS A STATE, not a veto. Codex draws whatever is current LAST, + and that holds for the composer as much as for a modal: an idle composer + drawn below trust wording means the dialog has been answered and its text + is history (the v0.130+ dialog leaves its option line in view for a while), + and a header appearing below an idle composer means a modal is arriving and + the composer is the stale thing. Testing the modal patterns without the + composer in the ordering got both of those backwards -- status stayed + WAITING on a dismissed prompt, and startup was declared ready under a + modal mid-redraw (round-5 review of #731). + + THE RULE. Codex draws the active modal last, so of every recognised block + whose signature sits in ``bottom_region`` the LOWEST one is on screen, and + everything above it is copy left behind by a screen the TUI has already + replaced. Each block is therefore located by the LAST occurrence of its + signature, and the block with the greatest offset wins. Testing each + signature on its own -- which is what the handler used to do, branch by + branch -- fires on stale copy: v1 trust wording anywhere in scrollback + pressed Enter into a live update dialog (default item: "Update now", a + global npm install), and stale v2 trust wording borrowed a live login menu's + footer and chose a sign-in method on the operator's behalf. + + THE FOOTER. "Press enter to continue" is shared by the v2 trust dialog, the + update dialog and the login menu, so it cannot identify a block by itself. + It belongs to whichever block it is drawn under, so a block that renders one + (update, login, the v2 trust header) must be followed by a footer to count + at all; the v1 wording carries its own numbered options and has never had + one. The update dialog must additionally be followed by its numbered menu, + as the update-dialog check has always required. + + Offsets are only comparable because every match comes from the same string. + Callers pass the bottom window, never the whole capture: a live dialog is by + definition in view, and widening the search only admits more stale copy. + """ + + def _last_start(pattern: str) -> Optional[int]: + last: Optional[int] = None + for match in re.finditer(pattern, bottom_region): + last = match.start() + return last + + footer_starts = [m.start() for m in re.finditer(TRUST_PROMPT_FOOTER, bottom_region)] + + def _footer_below(position: int) -> bool: + return any(start > position for start in footer_starts) + + candidates: Dict[str, int] = {} + + v1 = _last_start(TRUST_PROMPT_PATTERN) + v2 = _last_start(TRUST_PROMPT_PATTERN_V2) + if v1 is not None or v2 is not None: + # One dialog can match both wordings (v0.130+ shows the v2 header over + # the v1 option text), so trust is one block located by its lowest line. + trust = max(position for position in (v1, v2) if position is not None) + if v1 is not None or _footer_below(trust): + candidates["trust"] = trust + + update = _last_start(UPDATE_DIALOG_PATTERN) + if update is not None: + menu_below = any( + m.start() > update for m in re.finditer(UPDATE_DIALOG_MENU_PATTERN, bottom_region) + ) + if menu_below and _footer_below(update): + candidates["update"] = update + + login = _last_start(LOGIN_MENU_PATTERN) + if login is not None and _footer_below(login): + candidates["login"] = login + + composer = _composer_position(bottom_region) + if composer is not None: + candidates["composer"] = composer + + incomplete = [ + position + for name, position in (("trust", v2), ("update", update), ("login", login)) + if position is not None and name not in candidates + ] + if not candidates: + # A lone header with nothing complete anywhere is a modal being drawn, + # not nothing: report it so no exit fires on this frame. + return "transitional" if incomplete else None + live = max(candidates, key=lambda name: candidates[name]) + + # THE HALF-DRAWN BLOCK. A header that sits BELOW the winning block but is not + # yet followed by the lines that would make it a block of its own (the + # update menu, the shared footer) is a modal Codex is in the middle of + # drawing, not stale copy: stale copy is above the live block by the rule + # above. Naming the complete block above it "live" here would let the + # handler send that block's key into the modal that is appearing under it + # -- a bare Enter meant for an already-answered trust dialog landing on + # "1. Update now" or a sign-in method (round-4 review of #731). So the + # frame is reported as transitional: send nothing, keep every exit closed, + # and read again. The outer cap bounds how long that can go on. + live_position = candidates[live] + if any(position > live_position for position in incomplete): + return "transitional" + return live def _modal_line_content(line: str) -> Optional[str]: @@ -1094,10 +1228,61 @@ def _build_codex_command(self) -> str: command = f"{command} {developer_instructions_fragment}" return command - async def _handle_trust_prompt(self, timeout: float = 20.0) -> None: + async def _handle_trust_prompt( + self, + idle_gap: Optional[float] = None, + outer_timeout: Optional[float] = None, + ) -> str: """Dismiss startup prompts that block readiness. - Handles two classes of blocking dialog in a single poll loop: + Returns ``"settled"`` when startup reached a state the caller may build + on (idle composer, first-run login menu, or the idle gap elapsing on a + frame with no dialog), ``"blocked"`` when the outer cap was reached + with a trust/update dialog or a half-drawn modal still on screen (a + dismissal key was ignored or the redraw stalled), and ``"unresolved"`` + when the cap was reached on a frame this handler does not recognise. + ``initialize()`` fails on ``"blocked"``: a dialog this handler exists to + dismiss must not be carried into readiness through the login-only + WAITING_USER_ANSWER success path. + + Args: + idle_gap: Seconds with no new prompt before startup is considered + settled. Defaults to the ``startup_prompt_handler_timeout`` + setting. Per that setting's contract this is an IDLE GAP, not a + total budget: it is reset every time a prompt is answered, and + it only starts counting once at least one prompt has been + handled -- the semantics ``kimi_cli``/``antigravity_cli`` document + for the same setting (their handlers still judge the gap on the + clock before the poll; see the idle-gap note in the loop). + outer_timeout: Hard cap on total time this handler may run. + Defaults to the ``provider_init_timeout`` setting, which is + what that setting documents itself as bounding. + + Before this split, ``startup_prompt_handler_timeout`` was used as a + FIXED TOTAL budget here, contradicting its documented semantics: an + operator who lowered it to make another provider settle faster silently + capped codex's whole handler, so a dialog rendered after the gap was + never dismissed. ``initialize()`` treats WAITING_USER_ANSWER as success, + so the undismissed dialog then made ``send_input`` raise + ``TerminalInputBlockedError`` and the initial message was never + delivered. + + Every backend call here (get_history/send_keys/send_special_key) is a + blocking subprocess exec, and this loop makes one per second for up to + ``outer_timeout`` seconds. cao-server runs a SINGLE event loop, so leaving + them loop-side froze every other concurrent request — including every + other terminal's own init — for the duration. They are offloaded to + threads for the same reason claude_code's startup handler was in #451 + and kimi_cli/antigravity_cli/copilot_cli's were in #494. Codex is not + the last such gap — kiro_cli, opencode_cli and cursor_cli still make + loop-side backend calls in ``initialize()`` — but it is the slowest + init of the group, so a concurrent fan-out of codex launches is where + the self-inflicted queueing showed up first. + + Each frame is read ONCE into a single answer -- which startup block is + live at the bottom of the pane (``_live_startup_block``) -- and only + that block is ever sent a key. Handles two classes of blocking dialog + in a single poll loop: 1. Workspace trust prompt (two variants): v0.98+: "allow Codex to work in this folder" @@ -1110,101 +1295,205 @@ async def _handle_trust_prompt(self, timeout: float = 20.0) -> None: Dismissed with '3'+Enter ("Skip until next version"). A blind Enter would select "1. Update now" (global npm install). """ - start_time = time.time() + if idle_gap is None: + idle_gap = float(get_server_settings()["startup_prompt_handler_timeout"]) + if outer_timeout is None: + outer_timeout = float(get_server_settings()["provider_init_timeout"]) + outer_deadline = time.monotonic() + outer_timeout + last_prompt_time = time.monotonic() + any_prompt_handled = False trust_dismissed = False update_dismissed = False - while time.time() - start_time < timeout: - output = get_backend().get_history(self.session_name, self.window_name) + last_state: Optional[str] = None + while True: + now = time.monotonic() + if now >= outer_deadline: + break + output = await asyncio.to_thread( + get_backend().get_history, self.session_name, self.window_name + ) if not output: await asyncio.sleep(1.0) continue clean_output = strip_terminal_escapes(re.sub(ANSI_CODE_PATTERN, "", output)) - if not trust_dismissed and re.search(TRUST_PROMPT_PATTERN, clean_output): - from cli_agent_orchestrator.services.status_monitor import status_monitor - - logger.info("Codex workspace trust prompt (v1) detected, auto-accepting") - status_monitor.notify_input_sent(self.terminal_id) - get_backend().send_special_key(self.session_name, self.window_name, "Enter") - trust_dismissed = True - await asyncio.sleep(1.0) - continue - bottom_region = "\n".join(clean_output.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) - - if ( - not trust_dismissed - and re.search(TRUST_PROMPT_PATTERN_V2, bottom_region) - and re.search(TRUST_PROMPT_FOOTER, bottom_region) - ): + # ONE decision per frame, made before any key is sent: which startup + # block is live? See _live_startup_block for the rule (lowest + # recognised block wins; the shared footer belongs to it) and for the + # two keystrokes-into-the-wrong-modal failures the per-branch tests it + # replaces produced. Every branch below keys off this single answer, + # so a block that is live is always the one dismissed, a block that is + # dismissed-but-still-rendered still blocks every exit, and stale copy + # above the live block is invisible rather than merely tolerated. + live = _live_startup_block(bottom_region) + last_state = live + + if live == "trust" and not trust_dismissed: from cli_agent_orchestrator.services.status_monitor import status_monitor - logger.info("Codex workspace trust prompt (v2) detected, auto-accepting") + wording = "v2" if re.search(TRUST_PROMPT_PATTERN_V2, bottom_region) else "v1" + logger.info("Codex workspace trust prompt (%s) detected, auto-accepting", wording) status_monitor.notify_input_sent(self.terminal_id) - get_backend().send_special_key(self.session_name, self.window_name, "Enter") + await asyncio.to_thread( + get_backend().send_special_key, self.session_name, self.window_name, "Enter" + ) trust_dismissed = True + any_prompt_handled = True + last_prompt_time = time.monotonic() # reset idle timer await asyncio.sleep(1.0) continue - if not update_dismissed and _has_update_dialog_in_bottom(clean_output): + if live == "update" and not update_dismissed: from cli_agent_orchestrator.services.status_monitor import status_monitor logger.info( "Codex update-available dialog detected, selecting " "'Skip until next version'" ) status_monitor.notify_input_sent(self.terminal_id) - get_backend().send_keys(self.session_name, self.window_name, "3", enter_count=0) + await asyncio.to_thread( + get_backend().send_keys, + self.session_name, + self.window_name, + "3", + enter_count=0, + ) # TUI rendering latency: '3' highlights the menu item, Enter confirms. await asyncio.sleep(0.3) - get_backend().send_special_key(self.session_name, self.window_name, "Enter") + await asyncio.to_thread( + get_backend().send_special_key, self.session_name, self.window_name, "Enter" + ) update_dismissed = True + any_prompt_handled = True + last_prompt_time = time.monotonic() # reset idle timer await asyncio.sleep(1.0) continue + # A dismissed dialog can stay on screen for a frame after its key was + # sent (rendering lag after Enter). While it does it is still the live + # block, and it still blocks every exit below -- the idle gap, the + # login menu and the idle composer alike: returning with it up would + # let ``initialize()`` succeed on WAITING_USER_ANSWER, and a delivery + # landing in that window is refused with TerminalInputBlockedError and + # dropped -- the failure this handler's idle-gap split exists to prevent. + # A transitional frame (a header still being drawn under the lowest + # complete block) is held the same way: nothing is sent to it above, + # and nothing may return on it here. + has_dialog = live in ("trust", "update", "transitional") + + # The idle gap is judged on THIS frame, after the block decision, not + # at the top of the loop before a read. Judged first, the mandatory + # one-second sleep after a dismissal is itself enough to expire a + # one-second gap (the smallest the settings validator accepts), so a + # follow-up dialog already rendered by then was never observed; and + # judged without ``has_dialog`` a dismissed block still on screen + # after the gap returned the handler onto it. Quiet means a fresh + # frame with no prompt on it, ``idle_gap`` seconds after the last one + # was answered. + if any_prompt_handled and not has_dialog and now - last_prompt_time >= idle_gap: + # No new prompt within the idle gap — startup settled. + return "settled" + + # First-run login menu: this handler must NOT answer it (picking a + # sign-in method for the operator is not ours to do), but it is a + # settled startup state, and ``initialize()``'s next wait already + # accepts it as WAITING_USER_ANSWER. Without this exit the loop ran to + # the outer cap and logged "no prompt or welcome banner detected" about + # a screen that plainly showed one; with the default 60s cap and a 30s + # ``mcp_request_timeout``, a non-headless ``cao launch`` had its client + # raise ReadTimeout before the operator could attach to authenticate. + # "live == login" already means no dismissable block is drawn below it. + if live == "login": + logger.info( + "Codex first-run login menu detected — startup is settled, leaving the " + "menu for the operator to answer" + ) + return "settled" # Exit when the bottom region shows the idle composer prompt AND no # dialog is active. The welcome banner alone is insufficient — it # renders as normal startup chrome BEFORE a late update dialog appears. - has_idle = _has_startup_idle_composer(clean_output) - has_dialog = ( - re.search(TRUST_PROMPT_PATTERN, bottom_region) - or ( - re.search(TRUST_PROMPT_PATTERN_V2, bottom_region) - and re.search(TRUST_PROMPT_FOOTER, bottom_region) - ) - or _has_update_dialog_in_bottom(clean_output) - ) - if has_idle and not has_dialog: + # Readiness is the resolver's own answer -- the composer is the lowest + # thing drawn -- with ``_has_startup_idle_composer`` as the activity + # veto (a spinner or an approval prompt in the tail means the + # composer is not idle yet). + if live == "composer" and _has_startup_idle_composer(clean_output): logger.info("Codex started — idle prompt visible, no blocking dialog") - return + return "settled" await asyncio.sleep(1.0) pane_tail = "" try: - output = get_backend().get_history(self.session_name, self.window_name) + output = await asyncio.to_thread( + get_backend().get_history, self.session_name, self.window_name + ) if output: pane_tail = "\n".join(output.splitlines()[-10:]) except Exception: pass + blocked = last_state in ("trust", "update", "transitional") logger.error( - "Codex startup prompt handler timed out — no prompt or welcome banner detected. " + "Codex startup prompt handler hit its provider_init_timeout outer cap (%ss) — %s. " "Pane tail:\n%s", + outer_timeout, + ( + f"a startup dialog is still on screen ({last_state})" + if blocked + else "no prompt or welcome banner detected" + ), pane_tail, ) + return "blocked" if blocked else "unresolved" + + async def _current_startup_state(self) -> Optional[str]: + """Resolve the startup state of the pane as it is right now.""" + output = await asyncio.to_thread( + get_backend().get_history, self.session_name, self.window_name + ) + if not output: + return None + clean_output = strip_terminal_escapes(re.sub(ANSI_CODE_PATTERN, "", output)) + bottom_region = "\n".join(clean_output.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) + return _live_startup_block(bottom_region) + + def _try_load_profile(self): + """Best-effort profile load for timeout resolution only. + + Returns None on any load failure instead of raising -- unlike + ``_build_codex_command``'s inline load, which legitimately raises + ``ProviderError`` on a broken profile. This helper only feeds + ``BaseProvider.get_init_timeout``, so a missing/unloadable profile + should fall back to the server default here, not abort init before the + real (error-raising) load gets a chance to report the actual problem. + Same shape as kimi_cli/antigravity_cli/grok_cli/minimax_code. + """ + if self._agent_profile is None: + return None + try: + return load_agent_profile(self._agent_profile) + except Exception: + return None async def initialize(self) -> bool: """Initialize Codex provider by starting codex command.""" from cli_agent_orchestrator.services.status_monitor import status_monitor - init_timeout = get_server_settings()["provider_init_timeout"] + # The ONE hard cap for every wait below, resolved through + # BaseProvider.get_init_timeout so a profile's own provider_init_timeout + # wins over the server default. Read straight from settings, as this used + # to be, a containerized profile that declared 180 got 60 in + # wait_for_shell, the startup-prompt handler and the readiness wait alike. + init_timeout = self.get_init_timeout(self._try_load_profile()) if not await wait_for_shell(self.terminal_id, timeout=init_timeout): raise TimeoutError(f"Shell initialization timed out after {init_timeout}s") # Capture the shell process name before launching codex — used later to # detect when codex has exited and the pane is back to a bare shell. - self.shell_baseline = get_backend().get_pane_current_command( - self.session_name, self.window_name + # Offloaded like the rest of this method's backend calls (#451): each is + # a blocking subprocess exec on cao-server's single shared event loop. + self.shell_baseline = await asyncio.to_thread( + get_backend().get_pane_current_command, self.session_name, self.window_name ) # Send a warm-up command before launching codex. @@ -1214,7 +1503,9 @@ async def initialize(self) -> bool: # external input that must be allowed to drive PROCESSING transitions # past any previously-latched ready state. status_monitor.notify_input_sent(self.terminal_id) - get_backend().send_keys(self.session_name, self.window_name, "echo ready") + await asyncio.to_thread( + get_backend().send_keys, self.session_name, self.window_name, "echo ready" + ) await asyncio.sleep(2.0) # Build command with flags and agent profile (developer_instructions). @@ -1224,10 +1515,32 @@ async def initialize(self) -> bool: # caused by the shell_snapshot subprocess inheriting stdin. command = self._build_codex_command() status_monitor.notify_input_sent(self.terminal_id) - get_backend().send_keys(self.session_name, self.window_name, command) + await asyncio.to_thread( + get_backend().send_keys, self.session_name, self.window_name, command + ) - # Handle workspace trust prompt if it appears (new/untrusted directories) - await self._handle_trust_prompt(timeout=20.0) + # Handle workspace trust prompt if it appears (new/untrusted directories). + # The handler's bounds now come from settings rather than a hard-coded + # 20.0 that could not be raised without a code change, so an operator on + # a slow or containerized host can widen them. It reads + # ``startup_prompt_handler_timeout`` itself as the IDLE GAP between + # consecutive prompts; ``provider_init_timeout`` (resolved above as + # ``init_timeout``) is passed as the hard outer cap. Keeping those two + # roles distinct is what those settings document, and matches + # kimi_cli/antigravity_cli/claude_code — so an operator who lowers the + # gap for one provider cannot silently truncate codex's whole handler + # and leave a late dialog undismissed. + outcome = await self._handle_trust_prompt(outer_timeout=float(init_timeout)) + if outcome == "blocked": + # The handler's own hard cap failed: a trust/update dialog (or a + # modal mid-redraw) is still up. Letting the readiness wait below + # accept it as WAITING_USER_ANSWER -- a status meant for the login + # menu -- would report a successful initialisation that every + # assign/handoff is then refused into (round-5 review of #731). + raise TimeoutError( + f"Codex startup prompt could not be dismissed within {init_timeout}s: " + "a trust or update dialog is still on screen" + ) # WAITING_USER_ANSWER is included here specifically for the first-run login/auth # menu (see LOGIN_MENU_PATTERN's own comment) — an account with no credentials @@ -1248,10 +1561,33 @@ async def initialize(self) -> bool: if not await wait_until_status( self.terminal_id, {TerminalStatus.IDLE, TerminalStatus.COMPLETED, TerminalStatus.WAITING_USER_ANSWER}, - timeout=float(get_server_settings()["provider_init_timeout"]), + timeout=float(init_timeout), polling_interval=1.0, ): - raise TimeoutError("Codex initialization timed out after 60 seconds") + # Interpolated, not the old hard-coded "60 seconds": the bound is + # ``provider_init_timeout``, so an operator who changed it was told + # a number the code never used. + raise TimeoutError(f"Codex initialization timed out after {init_timeout}s") + + # WAITING_USER_ANSWER satisfied the wait; only the login menu is entitled + # to. A trust/update dialog that appeared after the handler returned (or + # a modal mid-redraw) is a failed start, not a settled one. But a + # transitional frame is not condemned on one read: it may be the login + # menu (or the composer) caught between redraw writes at this exact + # instant, so it gets re-read -- the same hold the startup handler gives + # such frames -- and only a frame still unresolved after the re-reads + # fails the start (round-7 review of #731). + startup_state = await self._current_startup_state() + for _ in range(POST_READINESS_TRANSITIONAL_REREADS): + if startup_state != "transitional": + break + await asyncio.sleep(0.5) + startup_state = await self._current_startup_state() + if startup_state in ("trust", "update", "transitional"): + raise TimeoutError( + f"Codex initialization ended with a startup dialog on screen ({startup_state}); " + "it was not dismissed within the init window" + ) self._initialized = True return True @@ -1315,56 +1651,35 @@ def get_status(self, output: str) -> TerminalStatus: last_user and _find_assistant_marker(output_after_last_user) is not None ) - # Check trust prompt early — the trust menu uses › which matches the idle prompt - # pattern, and PROCESSING_PATTERN matches "running" in "You are running Codex in..." - if re.search(TRUST_PROMPT_PATTERN, clean_output): + # Startup states, resolved ONCE and positionally, by the same function the + # startup handler uses (``_live_startup_block``): trust wording (the menu + # uses › which matches the idle prompt pattern, and PROCESSING_PATTERN + # matches "running" in "You are running Codex in..."), the v2 dialog, the + # update dialog, the login menu, or a modal mid-redraw. Each is only + # WAITING when it is the LOWEST thing drawn; the idle composer takes part + # in that ordering, so the option line a dismissed v0.130+ trust dialog + # leaves above the composer is history, not a prompt (round-5 review of + # #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"): return TerminalStatus.WAITING_USER_ANSWER - - # V2 trust dialog ("Do you trust the contents of this directory?" / "Press enter - # to continue"). Only classify as WAITING when BOTH the question AND the footer - # appear in the bottom region — avoids false positives if the question text - # appears in scrollback from a previous model response. - bottom_region = "\n".join(clean_output.splitlines()[-15:]) - if re.search(TRUST_PROMPT_PATTERN_V2, bottom_region) and re.search( - TRUST_PROMPT_FOOTER, bottom_region - ): + # ``transitional`` is a STARTUP-ONLY answer. It names a lone recognised + # header with no completing footer/menu -- during startup that is a modal + # mid-redraw, but on an initialized terminal it is the model QUOTING a + # startup phrase mid-turn ("Do you trust the contents of this + # directory?", "Sign in with ChatGPT", "Update available! X -> Y" in + # assistant prose), and mapping it to WAITING here flipped PROCESSING + # frames to WAITING_USER_ANSWER on the always-on status path (round-7 + # review of #731). The complete blocks above corroborate themselves with + # their footer/menu; a lone header corroborates nothing, so once + # initialization is over it falls through to the ordinary classification. + # Startup keeps the conservative reading: ``initialize()``'s readiness + # wait may accept the frame, and its post-readiness startup-state check + # re-reads until the frame resolves. + if startup_state == "transitional" and not self._initialized: return TerminalStatus.WAITING_USER_ANSWER - # Update-available dialog. Bottom-anchored like trust-v2 to avoid false - # positives from scrollback. Never let this fall through to IDLE/COMPLETED - # where a queued message or blind Enter could select "Update now". - # Eager inbox delivery is not a vector: accepts_input_while_processing=False. - if _has_update_dialog_in_bottom(clean_output): - return TerminalStatus.WAITING_USER_ANSWER - - # First-run login/auth menu (no credentials configured yet). Bottom-anchored like - # trust-v2, same reasoning. See LOGIN_MENU_PATTERN's own comment for why this can't - # be auto-dismissed the way trust/update dialogs are, and why classifying it here - # (rather than leaving it unrecognized) matters for initialize()'s own timeout. - if re.search(LOGIN_MENU_PATTERN, bottom_region) and re.search( - LOGIN_MENU_FOOTER, bottom_region - ): - return TerminalStatus.WAITING_USER_ANSWER - - # Boxed command-approval modal ("Command Approval Required" / "[a] Accept" - # / "[d] Decline"). Reuses the copy that STARTUP_BLOCKING_INPUT_PATTERN - # already vetoes readiness on at startup — the same modal can appear at - # RUNTIME under any approval-prompting codexProfile, and only the startup - # path used to notice it. - # - # Bottom-anchored like trust-v2 and the update dialog, and placed BEFORE - # the idle/COMPLETED classification for the same reason: the TUI composer - # and status bar keep rendering while the modal is up, so the idle-prompt - # check below would otherwise report COMPLETED (or PROCESSING when the - # composer has scrolled off) for a pane that is hard-blocked on a - # keystroke. A COMPLETED there is the dangerous case — it tells the - # conductor the agent is free and invites more work into a dead pane. - # - # NOT gated on `not assistant_after_last_user` (unlike WAITING_PROMPT_PATTERN - # below): the modal is raised mid-turn, after the model has already emitted - # bullets, so that gate would suppress every real occurrence. Prose that - # merely quotes the copy is excluded structurally instead — see - # _has_approval_modal_in_bottom. if _has_approval_modal_in_bottom(clean_output): return TerminalStatus.WAITING_USER_ANSWER diff --git a/test/providers/test_codex_provider_unit.py b/test/providers/test_codex_provider_unit.py index 05cc0e850..01f6bd5f7 100644 --- a/test/providers/test_codex_provider_unit.py +++ b/test/providers/test_codex_provider_unit.py @@ -4,7 +4,9 @@ import os import re import shlex +import time from pathlib import Path +from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -13,12 +15,19 @@ from cli_agent_orchestrator.models.terminal import TerminalStatus from cli_agent_orchestrator.providers.codex import ( APPROVAL_PROMPT_FOOTER, + LOGIN_MENU_FOOTER, + LOGIN_MENU_PATTERN, + POST_READINESS_TRANSITIONAL_REREADS, + STARTUP_PROMPT_BOTTOM_LINES, + TRUST_PROMPT_PATTERN, + TRUST_PROMPT_PATTERN_V2, CodexProvider, ProviderError, _find_response_marker, _has_approval_modal_in_bottom, _has_approval_prompt_in_bottom, _has_startup_idle_composer, + _live_startup_block, _toml_override, _toml_scalar, ) @@ -31,6 +40,38 @@ def load_fixture(filename: str) -> str: return f.read() +def fake_clock(*values: float) -> SimpleNamespace: + """A stand-in for codex's ``time`` module yielding a fixed ``monotonic`` series. + + Patch ``providers.codex.time`` with this rather than + ``providers.codex.time.monotonic``: ``codex.time`` IS the shared stdlib + module, so patching the attribute mutates it process-wide and asyncio's own + event loop — which calls ``time.monotonic()`` on every step — consumes the + ``side_effect`` sequence, raising StopIteration inside the loop instead of + in the code under test. Replacing the module reference keeps it local. + """ + return SimpleNamespace(monotonic=MagicMock(side_effect=values)) + + +def frames(*sequence: str): + """A ``get_history`` side effect that yields ``sequence`` then repeats its last frame. + + Never use a bare list: once exhausted it raises StopIteration inside + ``asyncio.to_thread``, which cannot deliver it, so the test HANGS instead of + failing. Repeating the final frame makes an unexpected extra read look like a + pane that stopped changing, and the handler fails on its own cap in bounded time. + """ + remaining = list(sequence) + + def _next(*_args, **_kwargs): + return remaining.pop(0) if len(remaining) > 1 else remaining[0] + + return _next + + +SETTLED = "OpenAI Codex (v0.98.0)\n› " + + class TestCodexCurrentComposer: @pytest.mark.parametrize( ("screen", "expected"), @@ -70,13 +111,18 @@ def read_developer_instructions_file(command: str) -> str: class TestCodexProviderInitialization: @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.wait_until_status") @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") @patch("cli_agent_orchestrator.providers.codex.get_backend") - async def test_initialize_success(self, mock_tmux, mock_wait_shell, mock_wait_status): + async def test_initialize_success( + self, mock_tmux, mock_wait_shell, mock_wait_status, mock_sleep + ): mock_wait_shell.return_value = True mock_wait_status.return_value = True - mock_tmux.return_value.get_history.return_value = "OpenAI Codex (v0.98.0)" + # A settled frame (idle composer): the handler returns on its first poll. + # A banner-only frame would make it poll, with real sleeps, to the 60s cap. + mock_tmux.return_value.get_history.return_value = SETTLED provider = CodexProvider("test1234", "test-session", "window-0", None) result = await provider.initialize() @@ -106,13 +152,16 @@ async def test_initialize_shell_timeout(self, mock_tmux, mock_wait_shell): await provider.initialize() @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.wait_until_status") @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") @patch("cli_agent_orchestrator.providers.codex.get_backend") - async def test_initialize_codex_timeout(self, mock_tmux, mock_wait_shell, mock_wait_status): + async def test_initialize_codex_timeout( + self, mock_tmux, mock_wait_shell, mock_wait_status, mock_sleep + ): mock_wait_shell.return_value = True mock_wait_status.return_value = False - mock_tmux.return_value.get_history.return_value = "OpenAI Codex (v0.98.0)" + mock_tmux.return_value.get_history.return_value = SETTLED provider = CodexProvider("test1234", "test-session", "window-0", None) @@ -451,21 +500,26 @@ def test_build_command_profile_load_failure(self, mock_load_profile): provider._build_codex_command() @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.wait_until_status") @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") @patch("cli_agent_orchestrator.providers.codex.load_agent_profile") @patch("cli_agent_orchestrator.providers.codex.get_backend") async def test_initialize_with_agent_profile( - self, mock_tmux, mock_load_profile, mock_wait_shell, mock_wait_status, tmp_path + self, mock_tmux, mock_load_profile, mock_wait_shell, mock_wait_status, mock_sleep, tmp_path ): mock_wait_shell.return_value = True mock_wait_status.return_value = True - mock_tmux.return_value.get_history.return_value = "OpenAI Codex (v0.98.0)" + # A settled frame: on the bare banner the handler polls to its cap -- + # which, with a MagicMock profile, is float(MagicMock()) == 1.0, so the + # test was quietly timing-dependent as well as slow. + mock_tmux.return_value.get_history.return_value = SETTLED mock_profile = MagicMock() mock_profile.model = None mock_profile.system_prompt = "You are a supervisor." mock_profile.mcpServers = None mock_profile.codexProfile = None + mock_profile.provider_init_timeout = None mock_load_profile.return_value = mock_profile provider = CodexProvider("test1234", "test-session", "window-0", "code_supervisor") @@ -2315,14 +2369,14 @@ def test_v0149_idle_composer_placeholder(self): @pytest.mark.asyncio @patch( - "cli_agent_orchestrator.providers.codex.time.time", - side_effect=[0.0, 0.0, 20.0], + "cli_agent_orchestrator.providers.codex.time", + new_callable=lambda: fake_clock(0.0, 0.0, 0.0), ) @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.logger.error") @patch("cli_agent_orchestrator.providers.codex.get_backend") async def test_handle_trust_prompt_returns_on_v0149_idle_composer( - self, mock_backend, mock_error, mock_sleep, _mock_time + self, mock_backend, mock_error, mock_sleep, _fake_time ): mock_backend.return_value.get_history.return_value = ( "OpenAI Codex (v0.149.0)\n" @@ -2331,7 +2385,7 @@ async def test_handle_trust_prompt_returns_on_v0149_idle_composer( ) provider = CodexProvider("test1234", "test-session", "window-0") - await provider._handle_trust_prompt(timeout=20.0) + await provider._handle_trust_prompt(outer_timeout=20.0) mock_backend.return_value.get_history.assert_called_once() mock_sleep.assert_not_awaited() @@ -2341,14 +2395,14 @@ async def test_handle_trust_prompt_returns_on_v0149_idle_composer( @pytest.mark.asyncio @patch( - "cli_agent_orchestrator.providers.codex.time.time", - side_effect=[0.0, 0.0, 20.0], + "cli_agent_orchestrator.providers.codex.time", + new_callable=lambda: fake_clock(0.0, 0.0, 0.0), ) @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.logger.error") @patch("cli_agent_orchestrator.providers.codex.get_backend") async def test_handle_trust_prompt_returns_on_v0145_idle_composer( - self, mock_backend, mock_error, mock_sleep, _mock_time + self, mock_backend, mock_error, mock_sleep, _fake_time ): """Codex 0.145's placeholder composer is a ready state, not a timeout.""" mock_backend.return_value.get_history.return_value = load_fixture( @@ -2356,7 +2410,7 @@ async def test_handle_trust_prompt_returns_on_v0145_idle_composer( ) provider = CodexProvider("test1234", "test-session", "window-0") - await provider._handle_trust_prompt(timeout=20.0) + await provider._handle_trust_prompt(outer_timeout=20.0) mock_backend.return_value.get_history.assert_called_once() mock_sleep.assert_not_awaited() @@ -2366,14 +2420,14 @@ async def test_handle_trust_prompt_returns_on_v0145_idle_composer( @pytest.mark.asyncio @patch( - "cli_agent_orchestrator.providers.codex.time.time", - side_effect=[0.0, 0.0, 1.0, 2.0, 20.0], + "cli_agent_orchestrator.providers.codex.time", + new_callable=lambda: fake_clock(0.0, 0.0, 0.0, 1.0, 2.0), ) @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.logger.error") @patch("cli_agent_orchestrator.providers.codex.get_backend") async def test_handle_trust_prompt_waits_for_complete_v0145_composer_frame( - self, mock_backend, mock_error, mock_sleep, _mock_time + self, mock_backend, mock_error, mock_sleep, _fake_time ): """Chunked redraws are not ready until composer and footer are both visible.""" fixture = load_fixture("codex_v0145_idle_output.txt") @@ -2385,7 +2439,7 @@ async def test_handle_trust_prompt_waits_for_complete_v0145_composer_frame( ] provider = CodexProvider("test1234", "test-session", "window-0") - await provider._handle_trust_prompt(timeout=20.0) + await provider._handle_trust_prompt(outer_timeout=20.0) assert mock_backend.return_value.get_history.call_count == 3 assert mock_sleep.await_count == 2 @@ -2439,19 +2493,20 @@ async def test_handle_trust_prompt_waits_for_complete_v0145_composer_frame( ) @pytest.mark.asyncio @patch( - "cli_agent_orchestrator.providers.codex.time.time", - side_effect=[0.0, 0.0, 20.0], + # One poll (not ready), then the outer cap ends the loop. + "cli_agent_orchestrator.providers.codex.time", + new_callable=lambda: fake_clock(0.0, 0.0, 0.0, 20.0), ) @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.logger.error") @patch("cli_agent_orchestrator.providers.codex.get_backend") async def test_handle_trust_prompt_does_not_treat_non_ready_output_as_idle( - self, mock_backend, mock_error, mock_sleep, _mock_time, output + self, mock_backend, mock_error, mock_sleep, _fake_time, output ): mock_backend.return_value.get_history.return_value = output provider = CodexProvider("test1234", "test-session", "window-0") - await provider._handle_trust_prompt(timeout=20.0) + await provider._handle_trust_prompt(outer_timeout=20.0) mock_sleep.assert_awaited_once_with(1.0) mock_error.assert_called_once() @@ -2459,8 +2514,9 @@ async def test_handle_trust_prompt_does_not_treat_non_ready_output_as_idle( mock_backend.return_value.send_special_key.assert_not_called() @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.get_backend") - async def test_handle_trust_prompt_detected_and_accepted(self, mock_tmux): + async def test_handle_trust_prompt_detected_and_accepted(self, mock_tmux, mock_sleep): """Test that trust prompt is detected and auto-accepted.""" mock_tmux.return_value.get_history.return_value = ( "> You are running Codex in /Users/test/project\n" @@ -2473,7 +2529,14 @@ async def test_handle_trust_prompt_detected_and_accepted(self, mock_tmux): ) provider = CodexProvider("test1234", "test-session", "window-0") - await provider._handle_trust_prompt(timeout=2.0) + # The frame never settles, so the handler runs to its cap; the fake + # clock reaches it on the second poll instead of after 2 real seconds. + # deadline, last_prompt_time, poll1 now, trust reset, poll2 now (cap). + with patch( + "cli_agent_orchestrator.providers.codex.time", + fake_clock(0.0, 0.0, 0.0, 0.0, 100.0), + ): + await provider._handle_trust_prompt(outer_timeout=2.0) mock_tmux.return_value.send_special_key.assert_called_once_with( "test-session", "window-0", "Enter" @@ -2486,10 +2549,217 @@ async def test_handle_trust_prompt_not_needed(self, mock_tmux): mock_tmux.return_value.get_history.return_value = "OpenAI Codex (v0.98.0)\n› " provider = CodexProvider("test1234", "test-session", "window-0") - await provider._handle_trust_prompt(timeout=2.0) + await provider._handle_trust_prompt(outer_timeout=2.0) mock_tmux.return_value.send_special_key.assert_not_called() + # ── startup_prompt_handler_timeout is an IDLE GAP, not a total budget ── + # + # The setting documents itself (settings_service.py) as the gap between + # consecutive startup prompts, reset each time one is answered, with total + # time bounded by provider_init_timeout. kimi_cli/antigravity_cli implement + # exactly that. Codex read it as a fixed total budget, so lowering the gap + # for another provider silently truncated codex's whole handler and left a + # late dialog undismissed -- and because initialize() accepts + # WAITING_USER_ANSWER as success, send_input then raised + # TerminalInputBlockedError and the initial message was never delivered. + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_small_idle_gap_does_not_truncate_the_handler(self, mock_backend, mock_sleep): + """A dialog rendered later than the idle gap is still dismissed. + + Regression guard: with the gap used as a total budget, a 2s gap meant a + dialog first visible on the 4th poll was never answered. + """ + blank = "OpenAI Codex (v0.98.0)\n" + trust = ( + "> You are running Codex in /Users/test/project\n" + "\n" + " Since this folder is version controlled, you may wish to " + "allow Codex to work in this folder without asking for approval.\n" + "\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + ) + settled = "OpenAI Codex (v0.98.0)\n› " + # The gap is judged on the frame read AFTER it elapses, so the poll at + # t=60 reads once more; a fifth frame is what it sees. + mock_backend.return_value.get_history.side_effect = [blank, blank, blank, trust, settled] + + provider = CodexProvider("test1234", "test-session", "window-0") + # Gap far shorter than the time the dialog takes to appear; the outer cap + # is what governs, so the handler must still be polling when it arrives. + with patch( + "cli_agent_orchestrator.providers.codex.time", + fake_clock(0.0, 0.0, 0.0, 1.0, 2.0, 3.0, 3.0, 60.0), + ): + await provider._handle_trust_prompt(idle_gap=2.0, outer_timeout=120.0) + + mock_backend.return_value.send_special_key.assert_called_once_with( + "test-session", "window-0", "Enter" + ) + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_idle_gap_resets_after_each_answered_prompt(self, mock_backend, mock_sleep): + """Answering the trust dialog restarts the gap, so a later update dialog + still gets dismissed rather than being cut off by the first gap.""" + trust = ( + " Since this folder is version controlled, you may wish to " + "allow Codex to work in this folder without asking for approval.\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + ) + update = ( + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + "2. Skip\n" + "3. Skip until next version\n" + "Press enter to continue\n" + ) + mock_backend.return_value.get_history.side_effect = [trust, update, "quiet tail"] + + provider = CodexProvider("test1234", "test-session", "window-0") + # Poll 1 answers trust at t=10, which RESETS the gap; poll 2 at t=12 is + # only 2s past that reset, so it is still inside the 5s gap. Without the + # reset the gap would be measured from t=0 -- already 10s, past the 5s + # gap -- so the loop would have returned before ever seeing the update + # dialog. That asymmetry is what makes this a guard rather than a + # restatement of the happy path. + with patch( + "cli_agent_orchestrator.providers.codex.time", + fake_clock(0.0, 0.0, 10.0, 10.0, 12.0, 12.0, 100.0), + ): + await provider._handle_trust_prompt(idle_gap=5.0, outer_timeout=120.0) + + # '3' + Enter dismisses the update dialog; a blind Enter would pick + # "1. Update now". + mock_backend.return_value.send_keys.assert_called_once_with( + "test-session", "window-0", "3", enter_count=0 + ) + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_idle_gap_ends_the_loop_once_startup_is_quiet(self, mock_backend, mock_sleep): + """After a prompt is answered, no new prompt within the gap returns — + the handler must not sit until the outer cap on a healthy start.""" + trust = ( + " Since this folder is version controlled, you may wish to " + "allow Codex to work in this folder without asking for approval.\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + ) + # The dialog is gone from the frame after its Enter; "quiet" is judged on + # that frame, not on the clock alone (see the two tests in + # TestIdleGapIsJudgedOnAFreshFrame for why the frame must be read). + quiet = "OpenAI Codex (v0.98.0)\n" + frames = iter([trust, quiet]) + mock_backend.return_value.get_history.side_effect = lambda *a, **k: next(frames, quiet) + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch( + "cli_agent_orchestrator.providers.codex.time", + fake_clock(0.0, 0.0, 0.0, 0.0, 10.0), + ): + await provider._handle_trust_prompt(idle_gap=5.0, outer_timeout=600.0) + + # Returned on the gap, not the outer cap: the poll that answered trust and + # the one fresh, quiet frame the gap was judged on. The outer cap would + # have needed a 600s clock value this fake clock never yields. + assert mock_backend.return_value.get_history.call_count == 2 + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_outer_cap_defaults_to_provider_init_timeout(self, mock_backend, mock_sleep): + """The outer cap must resolve to provider_init_timeout, not to the idle gap. + + Asserting only that ``get_server_settings`` was *called* would pass with the + two settings swapped, which is the mistake this split exists to prevent. So + this discriminates by behaviour instead: the pane matches no prompt, banner + or login menu, so nothing can end the loop except the outer cap, and the + clock crosses the gap setting (7s) without reaching the init setting (99s). + A correctly wired handler is therefore still polling; if the cap had + resolved to 7s it would have broken before the first poll. + + Counting reads rather than elapsed time is what makes this observable: + the cap's own error path reads the pane once more for its diagnostic + tail, so a correct cap leaves 2 reads (one in-loop, one diagnostic) and + a cap wrongly resolved to 7s leaves only the diagnostic one. + """ + mock_backend.return_value.get_history.return_value = "unrecognized scrollback\n" + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch( + "cli_agent_orchestrator.providers.codex.get_server_settings", + return_value={ + "startup_prompt_handler_timeout": 7.0, + "provider_init_timeout": 99.0, + }, + ): + with patch( + "cli_agent_orchestrator.providers.codex.time", + fake_clock(0.0, 0.0, 50.0, 100.0), + ): + await provider._handle_trust_prompt() + + # t=50 is past the 7s gap but inside the 99s cap, so the in-loop poll + # happened; t=100 then ends it and the cap logs its pane tail. Swapping + # the two settings drops the in-loop poll, leaving 1. + assert mock_backend.return_value.get_history.call_count == 2 + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_idle_gap_defaults_to_startup_prompt_handler_timeout( + self, mock_backend, mock_sleep + ): + """The idle gap must resolve to startup_prompt_handler_timeout. + + Exercised the way ``initialize()`` actually calls it — outer cap passed + explicitly, gap left to default — so the cap cannot mask which setting the + gap read. Trust is answered on poll 1, then the pane goes quiet; 10s later + the 7s gap has elapsed and the handler returns. Had the gap defaulted to + ``provider_init_timeout`` (99s) it would still be polling. + """ + trust = ( + " Since this folder is version controlled, you may wish to " + "allow Codex to work in this folder without asking for approval.\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + ) + mock_backend.return_value.get_history.side_effect = [ + trust, + "unrecognized scrollback\n", + "unrecognized scrollback\n", + "unrecognized scrollback\n", + ] + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch( + "cli_agent_orchestrator.providers.codex.get_server_settings", + return_value={ + "startup_prompt_handler_timeout": 7.0, + "provider_init_timeout": 99.0, + }, + ): + with patch( + "cli_agent_orchestrator.providers.codex.time", + # The trailing 700.0 crosses the explicit 600s cap so that a gap + # wrongly resolved to 99s ends the loop on the cap and fails this + # assertion, rather than exhausting the clock and raising + # StopIteration — a confusing error where a plain failure belongs. + fake_clock(0.0, 0.0, 0.0, 0.0, 10.0, 20.0, 700.0), + ): + await provider._handle_trust_prompt(outer_timeout=600.0) + + # Two polls: the one that answered trust, and the quiet frame at t=10 the + # 7s gap was judged on (the gap is decided on a fresh frame, never on the + # clock alone). A gap resolved to 99s would still be polling at t=20 and + # would read a third frame before the 700s clock value hit the cap. + assert mock_backend.return_value.get_history.call_count == 2 + def test_get_status_trust_prompt_is_waiting_user_answer(self): """Test that trust prompt reports WAITING_USER_ANSWER, not PROCESSING.""" output = ( @@ -2512,12 +2782,15 @@ async def test_initialize_with_trust_prompt(self, mock_tmux, mock_wait_shell, mo """Test that initialize handles trust prompt during startup.""" mock_wait_shell.return_value = True mock_wait_status.return_value = True - mock_tmux.return_value.get_history.return_value = ( - "allow Codex to work in this folder without asking for approval.\n" + # The dialog is gone from the frame after its Enter. A frame that never + # changed would (correctly) run the handler to its cap and fail init. + mock_tmux.return_value.get_history.side_effect = frames( + "allow Codex to work in this folder without asking for approval.\n", SETTLED ) provider = CodexProvider("test1234", "test-session", "window-0") - result = await provider.initialize() + with patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock): + result = await provider.initialize() assert result is True mock_tmux.return_value.send_special_key.assert_called_with( @@ -2534,7 +2807,7 @@ async def test_initialize_with_trust_prompt_v2( """Test that initialize handles v2 trust prompt (git worktree variant).""" mock_wait_shell.return_value = True mock_wait_status.return_value = True - mock_tmux.return_value.get_history.return_value = ( + mock_tmux.return_value.get_history.side_effect = frames( "Note: You're in a subdirectory of a Git project. Trusting will apply\n" "to the repository root: /Users/test/project\n" "\n" @@ -2543,12 +2816,14 @@ async def test_initialize_with_trust_prompt_v2( "› 1. Yes, continue\n" " 2. No, quit\n" "\n" - "Press enter to continue\n" + "Press enter to continue\n", + SETTLED, ) mock_tmux.return_value.get_pane_current_command.return_value = "zsh" provider = CodexProvider("test1234", "test-session", "window-0") - result = await provider.initialize() + with patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock): + result = await provider.initialize() assert result is True mock_tmux.return_value.send_special_key.assert_called_with( @@ -2677,11 +2952,12 @@ def test_get_status_login_menu_in_scrollback_does_not_false_positive(self, mock_ assert status != TerminalStatus.WAITING_USER_ANSWER @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.wait_until_status") @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") @patch("cli_agent_orchestrator.providers.codex.get_backend") async def test_initialize_includes_waiting_user_answer_in_target_status( - self, mock_tmux, mock_wait_shell, mock_wait_status + self, mock_tmux, mock_wait_shell, mock_wait_status, mock_sleep ): """Regression test for the real, live-reproduced failure: an account with no credentials configured yet reaches a correctly-rendered, fully-alive login screen @@ -2693,7 +2969,7 @@ async def test_initialize_includes_waiting_user_answer_in_target_status( the session and complete login. WAITING_USER_ANSWER must be in the target set.""" mock_wait_shell.return_value = True mock_wait_status.return_value = True - mock_tmux.return_value.get_history.return_value = "OpenAI Codex (v0.98.0)" + mock_tmux.return_value.get_history.return_value = SETTLED provider = CodexProvider("test1234", "test-session", "window-0", None) result = await provider.initialize() @@ -2768,10 +3044,11 @@ def test_get_status_update_dialog_scrollback_with_padding(self): assert status == TerminalStatus.COMPLETED @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.get_backend") - async def test_handle_trust_prompt_dismisses_update_dialog(self, mock_tmux): + async def test_handle_trust_prompt_dismisses_update_dialog(self, mock_tmux, mock_sleep): """_handle_trust_prompt detects update dialog and selects '3'+Enter.""" - mock_tmux.return_value.get_history.side_effect = [ + mock_tmux.return_value.get_history.side_effect = frames( ( "✨ Update available! 0.142.5 -> 0.144.5\n" "1. Update now (runs npm install -g @openai/codex)\n" @@ -2780,10 +3057,10 @@ async def test_handle_trust_prompt_dismisses_update_dialog(self, mock_tmux): "Press enter to continue\n" ), "OpenAI Codex (v0.142.5)\n› ", - ] + ) provider = CodexProvider("test1234", "test-session", "window-0") - await provider._handle_trust_prompt(timeout=5.0) + await provider._handle_trust_prompt(outer_timeout=5.0) mock_tmux.return_value.send_keys.assert_any_call( "test-session", "window-0", "3", enter_count=0 @@ -2797,135 +3074,644 @@ async def test_handle_trust_prompt_no_update_dialog(self, mock_tmux): mock_tmux.return_value.get_history.return_value = "OpenAI Codex (v0.142.5)\n› " provider = CodexProvider("test1234", "test-session", "window-0") - await provider._handle_trust_prompt(timeout=2.0) + await provider._handle_trust_prompt(outer_timeout=2.0) mock_tmux.return_value.send_keys.assert_not_called() mock_tmux.return_value.send_special_key.assert_not_called() + # ── The first-run login menu is a settled state, not a stall ── + # + # It is neither a dismissable dialog nor the idle composer, so before this it + # matched no exit condition and the handler ran to its outer cap. That cap is + # ``provider_init_timeout`` (60s by default) while a non-headless ``cao launch`` + # sends no ``initial_message`` — so ``POST /sessions`` initializes synchronously + # against the client's ``mcp_request_timeout`` (30s), and the client raised + # ReadTimeout before the operator could attach and authenticate. + + LOGIN_MENU_OUTPUT = ( + " Welcome to Codex, OpenAI's command-line coding agent\n" + "\n" + " Sign in with ChatGPT to use Codex as part of your paid plan\n" + " or connect an API key for usage-based billing\n" + "\n" + "> 1. Sign in with ChatGPT\n" + " Usage included with Plus, Pro, Business, and Enterprise plans\n" + "\n" + " 2. Sign in with Device Code\n" + " Sign in from another device with a one-time code\n" + "\n" + " 3. Provide your own API key\n" + " Pay for what you use\n" + "\n" + " Press enter to continue\n" + ) + @pytest.mark.asyncio - @patch("cli_agent_orchestrator.providers.codex.wait_until_status") - @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.logger.error") @patch("cli_agent_orchestrator.providers.codex.get_backend") - async def test_initialize_dismisses_update_dialog( - self, mock_tmux, mock_wait_shell, mock_wait_status - ): - """initialize() sees the update dialog, sends '3'+Enter, then reaches ready.""" - mock_wait_shell.return_value = True - mock_wait_status.return_value = True - mock_tmux.return_value.get_history.side_effect = [ - ( - "OpenAI Codex (v0.142.5)\n" - "✨ Update available! 0.142.5 -> 0.144.5\n" - "1. Update now (runs npm install -g @openai/codex)\n" - "2. Skip\n" - "3. Skip until next version\n" - "Press enter to continue\n" - ), - "OpenAI Codex (v0.142.5)\n› ", - ] + async def test_handle_trust_prompt_returns_on_login_menu(self, mock_tmux, mock_error): + """The handler returns promptly, having sent nothing, and logs no error. + + Returning matters because ``initialize()``'s next ``wait_until_status`` + already accepts this state as WAITING_USER_ANSWER — every second spent + here is spent against a client budget half the size of the outer cap. + Sending nothing matters more: choosing a sign-in method on the operator's + behalf is not this handler's call. The outer cap is set high here so a + timeout exit cannot be mistaken for a pass. + """ + mock_tmux.return_value.get_history.return_value = self.LOGIN_MENU_OUTPUT provider = CodexProvider("test1234", "test-session", "window-0") - result = await provider.initialize() + started = time.monotonic() + await provider._handle_trust_prompt(idle_gap=20.0, outer_timeout=30.0) + elapsed = time.monotonic() - started - assert result is True - mock_tmux.return_value.send_keys.assert_any_call( - "test-session", "window-0", "3", enter_count=0 + assert elapsed < 5.0, f"handler waited {elapsed:.1f}s on a settled login menu" + mock_tmux.return_value.send_keys.assert_not_called() + mock_tmux.return_value.send_special_key.assert_not_called() + # The old behaviour ended in "no prompt or welcome banner detected" — + # about a screen that plainly showed one. + mock_error.assert_not_called() + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_trust_v1_does_not_fire_on_scrollback_above_a_live_login_menu(self, mock_tmux): + """Stale trust copy in scrollback must not make the handler answer the menu. + + The v1 trust check is the only one of the four signatures matched against + the whole capture rather than ``bottom_region``, so before the ``not + has_login`` gate it could fire on text the login exit could not see. The + handler would then press Enter — selecting a sign-in method for the + operator — and log that it was leaving the menu alone, in that order. + + The consequence is worse than the stall this branch fixes: the keystroke + selects a sign-in method, so the pane leaves the login menu and with it the + set ``initialize()`` waits on ({IDLE, COMPLETED, WAITING_USER_ANSWER}) — + measured as PROCESSING on the API-key option, with the OAuth option not + exercised. Either way the session is torn down on a TimeoutError instead + of waiting for the operator to authenticate. + + One scrollback line is enough to demonstrate it, which is why the gate is + worth having even though no natural Codex sequence produces that line + today (the login menu renders *before* the trust prompt). + """ + contaminated = ( + " $ echo 'allow Codex to work in this folder' >> notes.txt\n" + self.LOGIN_MENU_OUTPUT ) - mock_tmux.return_value.send_special_key.assert_any_call("test-session", "window-0", "Enter") - mock_wait_status.assert_called_once() + mock_tmux.return_value.get_history.return_value = contaminated + + provider = CodexProvider("test1234", "test-session", "window-0") + await provider._handle_trust_prompt(idle_gap=20.0, outer_timeout=30.0) + + mock_tmux.return_value.send_special_key.assert_not_called() + mock_tmux.return_value.send_keys.assert_not_called() @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.logger.error") @patch("cli_agent_orchestrator.providers.codex.get_backend") - async def test_handle_trust_then_late_update_dialog(self, mock_tmux): - """Multi-frame: trust dismissed → transitional banner (no dialog yet) → update dialog → dismissed.""" - # Frame 1: trust prompt visible - frame_trust = ( - "Note: You're in a subdirectory of a Git project. Trusting will apply\n" - "to the repository root: /Users/test/project\n" - "\n" - "Do you trust the contents of this directory?\n" + async def test_live_v1_dialog_over_a_login_menu_is_dismissed_not_live_locked( + self, mock_tmux, mock_error + ): + """A *live* v1 dialog stacked on the menu must still be answered. + + This is why the v1 dismissal is bottom-anchored rather than simply + suppressed whenever ``has_login`` holds. Such a frame satisfies BOTH + ``has_login`` (menu text + footer in the bottom region) and ``has_dialog`` + (the v1 copy), so under a ``not has_login`` gate neither the dismissal nor + the login exit could fire and the handler live-locked to its outer cap — + reproducing the very stall the login branch exists to remove, and in the + non-headless case the original ReadTimeout with it, since the handler's cap + is twice the client's request budget. + + Asserting on the ORDER matters: dismiss first, then exit on the menu. That + is exactly what the login exit's ``not has_dialog`` comment claims happens. + """ + stacked_live = ( + "> 1. Sign in with ChatGPT\n" + " 2. Sign in with Device Code\n" + " 3. Provide your own API key\n" "\n" - "› 1. Yes, continue\n" - " 2. No, quit\n" + " Do you trust this workspace?\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" "\n" - "Press enter to continue\n" - ) - # Frame 2: trust dismissed, welcome banner visible but NO idle prompt yet - # (transitional state before update dialog renders) - frame_transitional = "OpenAI Codex (v0.142.5)\n" - # Frame 3: update dialog renders - frame_update = ( - "OpenAI Codex (v0.142.5)\n" - "✨ Update available! 0.142.5 -> 0.144.5\n" - "1. Update now (runs npm install -g @openai/codex)\n" - "2. Skip\n" - "3. Skip until next version\n" - "Press enter to continue\n" + " Press enter to continue\n" ) - # Frame 4: update dismissed, idle prompt visible - frame_idle = "OpenAI Codex (v0.142.5)\n› " + # Precondition: this frame really does set both predicates, or the test + # would be pinning nothing. Computed against the module's own patterns. + bottom = "\n".join(stacked_live.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) + assert re.search(LOGIN_MENU_PATTERN, bottom) and re.search(LOGIN_MENU_FOOTER, bottom) + assert re.search(TRUST_PROMPT_PATTERN, bottom) - mock_tmux.return_value.get_history.side_effect = [ - frame_trust, - frame_transitional, - frame_update, - frame_idle, - ] + mock_tmux.return_value.get_history.side_effect = frames( + stacked_live, self.LOGIN_MENU_OUTPUT + ) provider = CodexProvider("test1234", "test-session", "window-0") - await provider._handle_trust_prompt(timeout=10.0) + await provider._handle_trust_prompt(idle_gap=20.0, outer_timeout=30.0) - # Trust was dismissed with Enter mock_tmux.return_value.send_special_key.assert_any_call("test-session", "window-0", "Enter") - # Update dialog was dismissed with '3' then Enter - mock_tmux.return_value.send_keys.assert_any_call( - "test-session", "window-0", "3", enter_count=0 - ) - - @patch("cli_agent_orchestrator.providers.codex.load_agent_profile") - def test_update_check_suppression_is_last_override(self, mock_load): - """CAO's update suppression must win even if a profile sets the key.""" - mock_profile = MagicMock() - mock_profile.model = None - mock_profile.system_prompt = None - mock_profile.mcpServers = None - mock_profile.codexProfile = None - mock_profile.codexConfig = {"check_for_update_on_startup": True} - mock_load.return_value = mock_profile + # Did not run to the cap: the cap exit is the only thing that logs an error. + mock_error.assert_not_called() - provider = CodexProvider("tid", "sess", "win", "agent") - command = provider._build_codex_command() + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.logger.error") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_trust_copy_above_a_live_menu_inside_the_window_is_not_answered( + self, mock_tmux, mock_error + ): + """Nearby stale trust copy is still stale — presence in the window isn't liveness. + + Anchoring the v1 dismissal to ``bottom_region`` alone would only shrink the + scrollback exploit from "anywhere in the capture" to "the last 15 lines", + not close it. Position closes it: Codex draws the active modal last, so + trust copy ABOVE the menu text is history no matter how close it sits. + + What distinguishes this from + ``test_live_v1_dialog_over_a_login_menu_is_dismissed_not_live_locked`` is the + ORDER of the two blocks — that fixture also carries an inert + "Do you trust this workspace?" line, which matches no pattern in this module, + so order is the only difference that any predicate can see. Reversing the + comparison therefore fails both, which is what pins the comparison rather + than mere containment. + """ + trust_above = ( + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + "\n" + "> 1. Sign in with ChatGPT\n" + " 2. Sign in with Device Code\n" + " 3. Provide your own API key\n" + "\n" + " Press enter to continue\n" + ) + bottom = "\n".join(trust_above.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) + # Both signatures are inside the window; only their ORDER differs from the + # live-dialog case. Without that assertion this test could pass by losing + # one of them to the 15-line cut. + assert re.search(TRUST_PROMPT_PATTERN, bottom), "v1 copy must be in the window" + assert re.search(LOGIN_MENU_PATTERN, bottom) and re.search(LOGIN_MENU_FOOTER, bottom) - assert "check_for_update_on_startup=true" in command - assert command.endswith("-c check_for_update_on_startup=false") + mock_tmux.return_value.get_history.side_effect = frames(trust_above) + provider = CodexProvider("test1234", "test-session", "window-0") + started = time.monotonic() + await provider._handle_trust_prompt(idle_gap=20.0, outer_timeout=30.0) + elapsed = time.monotonic() - started -class TestCodexProviderApprovalModal: - """Tests for Codex's boxed command-approval modal appearing at RUNTIME. + mock_tmux.return_value.send_special_key.assert_not_called() + mock_tmux.return_value.send_keys.assert_not_called() + # Sending nothing is only half of it. Asserting the keystroke alone let this + # test pass while the handler still burned its whole cap: the copy was too + # stale to dismiss, yet a bare presence test still counted it in + # ``has_dialog`` and blocked the login exit — so neither branch fired and, at + # the production 60s cap against a 30s client budget, this PR's own P1 + # recurred in this frame class. Both assertions together are what pin it. + assert elapsed < 5.0, f"handler burned {elapsed:.1f}s instead of taking the login exit" + mock_error.assert_not_called() - The modal's copy was previously only consulted on the startup path - (STARTUP_BLOCKING_INPUT_PATTERN in _has_startup_idle_composer), so a pane - blocked on it mid-session was classified COMPLETED/PROCESSING and the - conductor would keep sending work into a pane hard-blocked on a keystroke. - """ + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_v1_twin_login_exit_waits_for_a_dismissed_v1_dialog_to_clear( + self, mock_tmux, mock_sleep + ): + """``has_dialog`` must count a live v1 dialog, not just the v2 variant. + + The v2 sibling of this test passes even with the v1 term removed from + ``has_dialog`` entirely — its fixture is a v2 frame, so the v2 term covers + for the v1 one. That left the v1 term unpinned: dropping it would let the + login exit fire on a frame where a just-answered v1 dialog is still + rendered, and since ``initialize()`` treats WAITING_USER_ANSWER as success, + a delivery landing in that window is refused with + ``TerminalInputBlockedError`` and dropped. + + Frame 1 dismisses (the v1 copy is below the menu, so it is live). Frame 2 + still shows it, and the handler must NOT take the login exit there. + Reaching frame 3 is the observable proof it kept waiting. + """ + stacked_v1 = ( + "> 1. Sign in with ChatGPT\n" + " 2. Sign in with Device Code\n" + " 3. Provide your own API key\n" + "\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + "\n" + " Press enter to continue\n" + ) + bottom = "\n".join(stacked_v1.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) + assert re.search(LOGIN_MENU_PATTERN, bottom) and re.search(LOGIN_MENU_FOOTER, bottom) + assert re.search(TRUST_PROMPT_PATTERN, bottom) + # No v2 signature anywhere, or the v2 term would cover for the v1 one and + # this test would pin nothing — which is the gap it exists to close. + assert not re.search(TRUST_PROMPT_PATTERN_V2, stacked_v1) - def test_get_status_approval_modal_waiting(self): - """Active approval modal classifies as WAITING_USER_ANSWER.""" - output = load_fixture("codex_approval_modal.txt") + mock_tmux.return_value.get_history.side_effect = frames( + stacked_v1, stacked_v1, self._SETTLED + ) provider = CodexProvider("test1234", "test-session", "window-0") - status = provider.get_status(output) - - assert status == TerminalStatus.WAITING_USER_ANSWER + await provider._handle_trust_prompt(idle_gap=30.0, outer_timeout=30.0) - def test_get_status_approval_modal_below_tui_footer_is_not_completed(self): - """Composer chrome above the modal must not win over the modal. + mock_tmux.return_value.send_special_key.assert_any_call("test-session", "window-0", "Enter") + assert mock_tmux.return_value.get_history.call_count == 3, ( + "login exit fired while a dismissed v1 dialog was still rendered " + f"(read {mock_tmux.return_value.get_history.call_count} frames, expected 3)" + ) - This is the dangerous shape: the TUI keeps rendering the idle composer - and status bar while the modal is up, so the idle-prompt check reported - COMPLETED — telling the conductor the agent was free. - """ - output = ( + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_stale_copy_above_and_live_dialog_below_dismisses(self, mock_tmux, mock_sleep): + """Both sides of the position test must take the LAST occurrence. + + Frame C: stale v1 copy above the menu AND a live v1 dialog below it. If the + v1 side takes the FIRST match while the menu side takes the LAST, the stale + copy wins the comparison, ``v1_is_live`` goes false, and — because + ``has_dialog`` shares that predicate — the login exit fires with a live + dialog still on screen. ``initialize()`` then succeeds on + WAITING_USER_ANSWER and the following ``send_input`` is refused with + ``TerminalInputBlockedError``, dropping the message: the exact failure ``not + has_dialog`` exists to prevent, and worse than the stall it replaced. + + This frame does NOT discriminate ``_menu_matches[-1]`` from ``[0]`` — the last + v1 copy sits below both menu matches, so either index gives the same answer. + ``test_v1_copy_between_two_menu_renders_is_not_live`` covers that. + """ + frame_c = ( + " $ grep 'allow Codex to work in this folder' audit.log\n" + "\n" + "> 1. Sign in with ChatGPT\n" + " 2. Sign in with Device Code\n" + " 3. Sign in with ChatGPT (retry)\n" + "\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + "\n" + " Press enter to continue\n" + ) + bottom = "\n".join(frame_c.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) + # The frame must genuinely contain the interleaving, or this pins nothing: + # two v1 occurrences straddling two menu occurrences. + assert len(list(re.finditer(TRUST_PROMPT_PATTERN, bottom))) == 2 + assert len(list(re.finditer(LOGIN_MENU_PATTERN, bottom))) == 2 + assert re.search(LOGIN_MENU_FOOTER, bottom) + + mock_tmux.return_value.get_history.side_effect = frames(frame_c, self.LOGIN_MENU_OUTPUT) + + provider = CodexProvider("test1234", "test-session", "window-0") + await provider._handle_trust_prompt(idle_gap=20.0, outer_timeout=30.0) + + mock_tmux.return_value.send_special_key.assert_any_call("test-session", "window-0", "Enter") + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_v1_copy_between_two_menu_renders_is_not_live(self, mock_tmux): + """The menu side must take the LAST occurrence, not the first. + + Frame: menu line, then v1 copy, then a second menu line. The lowest block is + the menu, so the v1 copy between them is stale and must not be answered. + Comparing against the FIRST menu match instead would put the v1 copy below it, + call it live, and press Enter into the menu. + + This is the frame that discriminates ``_menu_matches[-1]`` from ``[0]``; the + stale-above/live-below test does not. + """ + frame = ( + "> 1. Sign in with ChatGPT\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. Sign in with ChatGPT\n" + "\n" + " Press enter to continue\n" + ) + bottom = "\n".join(frame.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) + menus = list(re.finditer(LOGIN_MENU_PATTERN, bottom)) + v1s = list(re.finditer(TRUST_PROMPT_PATTERN, bottom)) + # The interleaving is the whole point: the last v1 must sit strictly between + # the first and last menu matches, or the two indices agree and this pins + # nothing. + assert len(menus) == 2 and len(v1s) == 1 + assert menus[0].start() < v1s[-1].start() < menus[-1].start() + + mock_tmux.return_value.get_history.side_effect = frames(frame) + + provider = CodexProvider("test1234", "test-session", "window-0") + await provider._handle_trust_prompt(idle_gap=20.0, outer_timeout=30.0) + + mock_tmux.return_value.send_special_key.assert_not_called() + mock_tmux.return_value.send_keys.assert_not_called() + + # ── ``has_login``'s conjunction is load-bearing in BOTH directions ── + # + # Since it now gates the v1 trust dismissal, a ``has_login`` that is too + # EAGER suppresses a dismissal that should happen — the mirror of the + # scrollback bug above, and not covered by tests that only check the login + # exit. It is easy to be too eager by accident: ``LOGIN_MENU_FOOTER`` IS + # ``TRUST_PROMPT_FOOTER``, so a footer-only ``has_login`` reads True on any + # frame carrying "Press enter to continue". Each test below feeds a frame + # where a real trust prompt must still be answered, and asserts it is. + + _SETTLED = "OpenAI Codex (v0.149.0)\n› \n" + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_shared_footer_alone_does_not_suppress_trust_dismissal(self, mock_tmux): + """The footer is shared with the trust prompt, so it cannot imply a login menu. + + Kills a ``has_login`` weakened to the footer alone: this frame has the + footer and the v1 trust copy but no menu text, so the trust prompt must + still be auto-accepted. + """ + frame = ( + " Do you trust this workspace?\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + "\n" + " Press enter to continue\n" + ) + mock_tmux.return_value.get_history.side_effect = frames(frame, self._SETTLED) + + provider = CodexProvider("test1234", "test-session", "window-0") + await provider._handle_trust_prompt(idle_gap=30.0, outer_timeout=30.0) + + mock_tmux.return_value.send_special_key.assert_any_call("test-session", "window-0", "Enter") + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_menu_text_without_the_footer_does_not_suppress_trust_dismissal( + self, mock_tmux, mock_sleep + ): + """Kills a login block that counts without its footer. + + A menu line with no footer is not a live login menu — Codex's own + ``get_status`` requires both. Under a trust dialog it is a menu still + being drawn, so THAT frame is held (round-4 review of #731: the trust + dialog's Enter must not land on it). The hold is per frame, not sticky: + when the next frame shows the trust dialog alone, it is answered. + + A login block admitted on the menu line alone would instead name the + first frame "login" and return the handler -- no Enter at all -- which is + the mutation this test exists to kill. + """ + # The v1 copy must sit ABOVE the menu line: below it, trust wins on position + # alone and the footer requirement is never exercised. + mid_draw = ( + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + " 1. Sign in with ChatGPT\n" + ) + trust_alone = ( + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + ) + _bottom = "\n".join(mid_draw.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) + assert re.search(LOGIN_MENU_PATTERN, _bottom) and not re.search( + LOGIN_MENU_FOOTER, _bottom + ), "fixture must carry the menu pattern WITHOUT the footer" + mock_tmux.return_value.get_history.side_effect = frames( + mid_draw, trust_alone, self._SETTLED + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + await provider._handle_trust_prompt(idle_gap=30.0, outer_timeout=30.0) + + # Answered once, and only after the mid-draw frame was read and held. + mock_tmux.return_value.send_special_key.assert_called_once_with( + "test-session", "window-0", "Enter" + ) + assert mock_tmux.return_value.get_history.call_count >= 3 + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_login_menu_only_in_scrollback_does_not_suppress_trust_dismissal( + self, mock_tmux, mock_sleep + ): + """Kills a ``has_login`` matched against the whole capture. + + ``has_login`` must stay bottom-anchored. A login menu that has scrolled + out of the bottom region is history, not a live prompt, so a trust dialog + at the bottom must still be dismissed — otherwise the gate that fixes the + scrollback bug above reintroduces it with the roles reversed. + """ + frame = ( + self.LOGIN_MENU_OUTPUT + + "\n".join(f" build step {i}" for i in range(20)) + + "\n Do you trust this workspace?\n" + + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + ) + mock_tmux.return_value.get_history.side_effect = frames(frame, self._SETTLED) + + provider = CodexProvider("test1234", "test-session", "window-0") + await provider._handle_trust_prompt(idle_gap=30.0, outer_timeout=30.0) + + mock_tmux.return_value.send_special_key.assert_any_call("test-session", "window-0", "Enter") + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_login_menu_does_not_short_circuit_a_stacked_trust_dialog( + self, mock_tmux, mock_sleep + ): + """A trust dialog rendered over the login menu is still dismissed first. + + This is the hazard in returning early on a recognized-but-unanswerable + screen: ``initialize()`` treats WAITING_USER_ANSWER as success, so exiting + with a trust dialog still up would make the following ``send_input`` raise + ``TerminalInputBlockedError`` and drop the initial message — the exact + failure the idle-gap split in this PR exists to prevent. Hence the login + exit is gated on no dismissable dialog being present. + """ + # A single stacked frame does NOT exercise the guard: the dismissal branch + # is earlier in the loop and ``continue``s, so the login check is never + # reached on that iteration. The guard decides something only on a LATER + # iteration, once the dialog has been answered but is still on screen — a + # rendering lag after Enter. Hence the same frame twice. + # + # Trust is not the only arm that gets there: ``has_dialog``'s third term is + # the same predicate as the update branch's condition, so a still-rendered + # update dialog reaches the guard once ``update_dismissed`` is set. Trust + # is used here because it is the cheaper frame to build. + # + # Both signatures must also land inside the 15-line bottom region, or + # ``has_login`` is False and the guard is never reached: an earlier version + # of this test appended the dialog to the full-height menu, which pushed + # "Sign in with ChatGPT" out of the window and passed whether the guard + # existed or not. This frame is 10 lines, so all of it is in the window: + # has_login holds, and so do BOTH trust signatures — TRUST_PROMPT_PATTERN + # via the option-1 line ("allow Codex to work in this folder") and the v2 + # pattern with its footer — so has_dialog stays True on frame 2 either way. + # + # Condensed rather than observed — this is a defensive guard for frame + # interleavings the TUI controls, not a transcript of one. + stacked = ( + "> 1. Sign in with ChatGPT\n" + " 2. Sign in with Device Code\n" + " 3. Provide your own API key\n" + "\n" + " Do you trust the contents of this directory?\n" + "\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + "\n" + " Press enter to continue\n" + ) + # Precondition, asserted rather than asserted-in-prose: an earlier version of + # this test lost has_login to the 15-line window and passed regardless. The + # comment above documented that trap; this line is what actually defends + # against it, and it fails loudly if the fixture ever drifts. + _bottom = "\n".join(stacked.splitlines()[-STARTUP_PROMPT_BOTTOM_LINES:]) + assert re.search(LOGIN_MENU_PATTERN, _bottom) and re.search( + LOGIN_MENU_FOOTER, _bottom + ), "fixture no longer sets has_login; this test would pin nothing" + + mock_tmux.return_value.get_history.side_effect = frames(stacked, stacked, self._SETTLED) + + provider = CodexProvider("test1234", "test-session", "window-0") + await provider._handle_trust_prompt(idle_gap=30.0, outer_timeout=30.0) + + # Frame 1 dismissed the dialog. + mock_tmux.return_value.send_special_key.assert_any_call("test-session", "window-0", "Enter") + # Frame 2 still shows it. The handler must NOT take the login exit there — + # if it does, initialize() succeeds on WAITING_USER_ANSWER with a live + # dialog and the next send_input raises TerminalInputBlockedError. Reaching + # frame 3 is the observable proof it kept waiting. + assert mock_tmux.return_value.get_history.call_count == 3, ( + "handler returned while a dismissable dialog was still on screen " + f"(read {mock_tmux.return_value.get_history.call_count} frames, expected 3)" + ) + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.wait_until_status") + @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_initialize_dismisses_update_dialog( + self, mock_tmux, mock_wait_shell, mock_wait_status + ): + """initialize() sees the update dialog, sends '3'+Enter, then reaches ready.""" + mock_wait_shell.return_value = True + mock_wait_status.return_value = True + # frames(), not a bare list: initialize() now reads the pane once more + # after the readiness wait to check no startup dialog is up, and a list + # exhausted inside asyncio.to_thread cannot deliver its StopIteration. + mock_tmux.return_value.get_history.side_effect = frames( + ( + "OpenAI Codex (v0.142.5)\n" + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + "2. Skip\n" + "3. Skip until next version\n" + "Press enter to continue\n" + ), + "OpenAI Codex (v0.142.5)\n› ", + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock): + result = await provider.initialize() + + assert result is True + mock_tmux.return_value.send_keys.assert_any_call( + "test-session", "window-0", "3", enter_count=0 + ) + mock_tmux.return_value.send_special_key.assert_any_call("test-session", "window-0", "Enter") + mock_wait_status.assert_called_once() + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_handle_trust_then_late_update_dialog(self, mock_tmux, mock_sleep): + """Multi-frame: trust dismissed → transitional banner (no dialog yet) → update dialog → dismissed.""" + # Frame 1: trust prompt visible + frame_trust = ( + "Note: You're in a subdirectory of a Git project. Trusting will apply\n" + "to the repository root: /Users/test/project\n" + "\n" + "Do you trust the contents of this directory?\n" + "\n" + "› 1. Yes, continue\n" + " 2. No, quit\n" + "\n" + "Press enter to continue\n" + ) + # Frame 2: trust dismissed, welcome banner visible but NO idle prompt yet + # (transitional state before update dialog renders) + frame_transitional = "OpenAI Codex (v0.142.5)\n" + # Frame 3: update dialog renders + frame_update = ( + "OpenAI Codex (v0.142.5)\n" + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + "2. Skip\n" + "3. Skip until next version\n" + "Press enter to continue\n" + ) + # Frame 4: update dismissed, idle prompt visible + frame_idle = "OpenAI Codex (v0.142.5)\n› " + + mock_tmux.return_value.get_history.side_effect = frames( + frame_trust, + frame_transitional, + frame_update, + frame_idle, + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + await provider._handle_trust_prompt(outer_timeout=10.0) + + # Trust was dismissed with Enter + mock_tmux.return_value.send_special_key.assert_any_call("test-session", "window-0", "Enter") + # Update dialog was dismissed with '3' then Enter + mock_tmux.return_value.send_keys.assert_any_call( + "test-session", "window-0", "3", enter_count=0 + ) + + @patch("cli_agent_orchestrator.providers.codex.load_agent_profile") + def test_update_check_suppression_is_last_override(self, mock_load): + """CAO's update suppression must win even if a profile sets the key.""" + mock_profile = MagicMock() + mock_profile.model = None + mock_profile.system_prompt = None + mock_profile.mcpServers = None + mock_profile.codexProfile = None + mock_profile.codexConfig = {"check_for_update_on_startup": True} + mock_load.return_value = mock_profile + + provider = CodexProvider("tid", "sess", "win", "agent") + command = provider._build_codex_command() + + assert "check_for_update_on_startup=true" in command + assert command.endswith("-c check_for_update_on_startup=false") + + +class TestCodexProviderApprovalModal: + """Tests for Codex's boxed command-approval modal appearing at RUNTIME. + + The modal's copy was previously only consulted on the startup path + (STARTUP_BLOCKING_INPUT_PATTERN in _has_startup_idle_composer), so a pane + blocked on it mid-session was classified COMPLETED/PROCESSING and the + conductor would keep sending work into a pane hard-blocked on a keystroke. + """ + + def test_get_status_approval_modal_waiting(self): + """Active approval modal classifies as WAITING_USER_ANSWER.""" + output = load_fixture("codex_approval_modal.txt") + + provider = CodexProvider("test1234", "test-session", "window-0") + status = provider.get_status(output) + + assert status == TerminalStatus.WAITING_USER_ANSWER + + def test_get_status_approval_modal_below_tui_footer_is_not_completed(self): + """Composer chrome above the modal must not win over the modal. + + This is the dangerous shape: the TUI keeps rendering the idle composer + and status bar while the modal is up, so the idle-prompt check reported + COMPLETED — telling the conductor the agent was free. + """ + output = ( "› run the deploy script\n" "• I'll run the deploy script now.\n" "› \n" @@ -4162,16 +4948,17 @@ def test_get_status_skips_exit_check_without_baseline(self, mock_tmux): assert status == TerminalStatus.IDLE @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) @patch("cli_agent_orchestrator.providers.codex.wait_until_status") @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") @patch("cli_agent_orchestrator.providers.codex.get_backend") async def test_initialize_captures_shell_baseline( - self, mock_tmux, mock_wait_shell, mock_wait_status + self, mock_tmux, mock_wait_shell, mock_wait_status, mock_sleep ): """Initialize captures shell_baseline for exit detection.""" mock_wait_shell.return_value = True mock_wait_status.return_value = True - mock_tmux.return_value.get_history.return_value = "OpenAI Codex (v0.98.0)" + mock_tmux.return_value.get_history.return_value = SETTLED mock_tmux.return_value.get_pane_current_command.return_value = "zsh" provider = CodexProvider("test1234", "test-session", "window-0") @@ -4289,3 +5076,897 @@ async def test_waiting_on_login_menu_leaves_worker_alive_task_undelivered( mock_tmux.send_keys.assert_not_called() mock_notify.assert_called_once() assert mock_notify.call_args.kwargs["delete_worker"] is False + + +class TestCodexInitConfiguredTimeouts: + """Codex init timeouts must come from settings, not hard-coded literals.""" + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.services.settings_service.get_server_settings") + @patch("cli_agent_orchestrator.providers.codex.get_server_settings") + @patch("cli_agent_orchestrator.providers.codex.wait_until_status") + @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_initialize_passes_provider_init_timeout_as_the_outer_cap( + self, + mock_backend, + mock_wait_shell, + mock_wait_status, + mock_settings, + mock_base_settings, + mock_sleep, + ): + """The handler's hard cap is ``provider_init_timeout``, not the idle gap. + + Both bounds come from settings rather than a hard-coded 20.0, so an + operator on a slow/containerized host can widen them without a code + change. But they are DIFFERENT settings doing different jobs: + ``startup_prompt_handler_timeout`` is the idle gap (read inside the + handler) and ``provider_init_timeout`` is the outer cap passed here. + Passing the gap as the total budget — as this call used to — meant + lowering the gap for another provider silently truncated codex's whole + handler, leaving a late dialog undismissed. + """ + mock_settings.return_value = { + "provider_init_timeout": 60, + "startup_prompt_handler_timeout": 45, + } + # BaseProvider.get_init_timeout reads settings_service directly. + mock_base_settings.return_value = mock_settings.return_value + mock_wait_shell.return_value = True + mock_wait_status.return_value = True + mock_backend.return_value.get_history.return_value = "OpenAI Codex (v0.98.0)" + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch.object(provider, "_handle_trust_prompt", new_callable=AsyncMock) as mock_trust: + await provider.initialize() + + # The gap (45) must NOT be what bounds the handler's total run. + mock_trust.assert_awaited_once_with(outer_timeout=60.0) + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.services.settings_service.get_server_settings") + @patch("cli_agent_orchestrator.providers.codex.get_server_settings") + @patch("cli_agent_orchestrator.providers.codex.wait_until_status") + @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_init_timeout_error_reports_the_configured_bound( + self, + mock_backend, + mock_wait_shell, + mock_wait_status, + mock_settings, + mock_base_settings, + mock_sleep, + ): + """The init-timeout message must name the timeout actually applied. + + The bound is ``provider_init_timeout``, but the message was hard-coded + to "60 seconds" — so an operator who raised or lowered the setting was + told a number the code never used. + """ + mock_settings.return_value = { + "provider_init_timeout": 150, + "startup_prompt_handler_timeout": 20, + } + # BaseProvider.get_init_timeout reads settings_service directly. + mock_base_settings.return_value = mock_settings.return_value + mock_wait_shell.return_value = True + mock_wait_status.return_value = False # never reaches a ready status + mock_backend.return_value.get_history.return_value = "OpenAI Codex (v0.98.0)" + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch.object(provider, "_handle_trust_prompt", new_callable=AsyncMock): + with pytest.raises(TimeoutError, match="150"): + await provider.initialize() + + assert mock_wait_status.await_args.kwargs["timeout"] == 150.0 + + +class TestLiveStartupBlock: + """``_live_startup_block`` names the block drawn LOWEST in the bottom window. + + Round-3 review of #731 (haofeif), P1: the handler tested each dialog's text + on its own and so pressed keys into whichever modal was actually live. The + resolver makes that decision once, by position, before any key is sent. + """ + + _UPDATE = ( + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + "2. Skip\n" + "3. Skip until next version\n" + "Press enter to continue\n" + ) + _LOGIN = ( + " Sign in with ChatGPT to use Codex as part of your paid plan\n" + "> 1. Sign in with ChatGPT\n" + " 2. Sign in with Device Code\n" + " 3. Provide your own API key\n" + "\n" + " Press enter to continue\n" + ) + _TRUST_V1 = ( + " Since this folder is version controlled, you may wish to " + "allow Codex to work in this folder without asking for approval.\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + ) + _TRUST_V2 = ( + " Do you trust the contents of this directory?\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + "\n" + " Press enter to continue\n" + ) + + def test_nothing_recognised_is_none(self): + assert _live_startup_block("OpenAI Codex (v0.98.0)\nTip: try /help") is None + + def test_the_idle_composer_is_a_state(self): + assert _live_startup_block("OpenAI Codex (v0.98.0)\n› ") == "composer" + + def test_each_block_alone_is_itself(self): + assert _live_startup_block(self._TRUST_V1) == "trust" + assert _live_startup_block(self._TRUST_V2) == "trust" + assert _live_startup_block(self._UPDATE) == "update" + assert _live_startup_block(self._LOGIN) == "login" + + def test_stale_v1_trust_copy_above_a_live_update_dialog_is_update(self): + """The reviewer's first reproduction: bare Enter here selects 'Update now'.""" + assert _live_startup_block(self._TRUST_V1 + "\n" + self._UPDATE) == "update" + + def test_stale_v2_trust_copy_above_a_live_login_menu_is_login(self): + """The reviewer's second reproduction: the v2 header borrowed the menu's footer.""" + stale_v2_header_only = " Do you trust the contents of this directory?\n" + assert _live_startup_block(stale_v2_header_only + "\n" + self._LOGIN) == "login" + + def test_live_trust_dialog_stacked_over_a_login_menu_is_trust(self): + assert _live_startup_block(self._LOGIN + "\n" + self._TRUST_V2) == "trust" + + def test_the_shared_footer_belongs_to_the_lowest_block(self): + # A login header with no footer of its own does not become a block just + # because the trust dialog BELOW it renders one. + login_no_footer = "> 1. Sign in with ChatGPT\n 2. Sign in with Device Code\n" + assert _live_startup_block(login_no_footer + self._TRUST_V2) == "trust" + # ...and a v2 header with an idle composer under it is stale copy above + # the live composer (round 5: the composer is a state of its own). + assert ( + _live_startup_block(" Do you trust the contents of this directory?\n› ") == "composer" + ) + + def test_a_footer_above_a_block_does_not_count_for_it(self): + """The discriminating half of footer attribution. + + A login header (or an update header with its menu) drawn BELOW a trust + dialog, with the only footer on screen belonging to that dialog, is not + yet a dialog of its own: nothing has been drawn under it. Counting any + footer anywhere would make the half-rendered lower block win and return + the handler on a menu that has not finished appearing. + + Nor is the complete block above it live any more (round-4 review of + #731): the header under it is a modal mid-draw, and the trust dialog's + bare Enter would land on it. The frame is transitional -- no key, no exit. + """ + login_no_footer = "> 1. Sign in with ChatGPT\n 2. Sign in with Device Code\n" + assert _live_startup_block(self._TRUST_V2 + login_no_footer) == "transitional" + update_no_footer = ( + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + "3. Skip until next version\n" + ) + assert _live_startup_block(self._TRUST_V2 + update_no_footer) == "transitional" + + def test_a_half_drawn_header_under_a_live_update_dialog_is_transitional(self): + """The v2 trust header appearing under a complete update dialog, footer not + yet drawn: the update dialog is not live to be keyed with '3' either.""" + v2_header_only = " Do you trust the contents of this directory?\n" + assert _live_startup_block(self._UPDATE + v2_header_only) == "transitional" + login_header_only = "> 1. Sign in with ChatGPT\n" + assert _live_startup_block(self._UPDATE + login_header_only) == "transitional" + + def test_a_half_drawn_header_above_the_live_block_is_stale_not_transitional(self): + """Position is what separates mid-draw from left-behind. The same footerless + header ABOVE the complete block is copy the TUI has already replaced, and + holding the frame for it would leave the live dialog unanswered.""" + update_no_footer = ( + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + "3. Skip until next version\n" + ) + assert _live_startup_block(update_no_footer + self._TRUST_V2) == "trust" + v2_header_only = " Do you trust the contents of this directory?\n" + assert _live_startup_block(v2_header_only + self._UPDATE) == "update" + + def test_duplicate_trust_text_is_located_by_its_lowest_occurrence(self): + """Stale trust copy above a live menu, and a live trust dialog below it. + + Only the LAST occurrence of a signature can be the block currently drawn. + Locating trust by its first occurrence would put it above the menu and + hand the frame to "login" -- returning the handler with a live dialog up. + """ + frame = self._TRUST_V1 + "\n" + self._LOGIN + "\n" + self._TRUST_V2 + assert _live_startup_block(frame) == "trust" + # And the mirror image: menu copy above, live menu below a stale dialog. + frame = self._LOGIN + "\n" + self._TRUST_V1 + "\n" + self._LOGIN + assert _live_startup_block(frame) == "login" + + def test_update_header_without_its_menu_alone_is_transitional(self): + """Round-5 review of #731: a lone half-drawn header is a modal arriving, + not nothing. Returning None here let the composer exit fire on a frame + whose stale composer sat above the header.""" + assert ( + _live_startup_block("✨ Update available! 0.1 -> 0.2\nPress enter to continue\n") + == "transitional" + ) + + +class TestStartupHandlerKeysOnlyTheLiveModal: + """Round-3 review of #731 (haofeif), P1 — reproduced on the exact head. + + Stale trust copy in the bottom window used to be answered with a bare Enter + regardless of what was actually live below it. Both reproductions below sent + Enter on the unfixed head; the first never reached the safe '3' path. + """ + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_stale_v1_copy_above_a_live_update_dialog_takes_the_skip_path( + self, mock_tmux, mock_sleep + ): + frame = ( + " Since this folder is version controlled, you may wish to " + "allow Codex to work in this folder without asking for approval.\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + "\n" + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + "2. Skip\n" + "3. Skip until next version\n" + "Press enter to continue\n" + ) + mock_tmux.return_value.get_history.side_effect = [frame, "OpenAI Codex (v0.142.5)\n› "] + + provider = CodexProvider("test1234", "test-session", "window-0") + await provider._handle_trust_prompt(idle_gap=30.0, outer_timeout=30.0) + + backend = mock_tmux.return_value + # '3' was sent BEFORE any Enter: a bare Enter first would have selected + # "1. Update now" and run a global npm install under every other worker. + assert backend.send_keys.call_args_list[0].args[2] == "3" + order = [name for name, _args, _kwargs in backend.mock_calls] + assert order.index("send_keys") < order.index("send_special_key") + # Exactly one Enter -- the one confirming '3'. A second would mean the stale + # trust copy was also "dismissed" as a dialog of its own. + assert backend.send_special_key.call_count == 1 + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.logger.error") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_stale_v2_copy_above_a_live_login_menu_sends_nothing_and_settles( + self, mock_tmux, mock_error + ): + frame = ( + " Do you trust the contents of this directory?\n" + "\n" + " Sign in with ChatGPT to use Codex as part of your paid plan\n" + "> 1. Sign in with ChatGPT\n" + " 2. Sign in with Device Code\n" + " 3. Provide your own API key\n" + "\n" + " Press enter to continue\n" + ) + mock_tmux.return_value.get_history.return_value = frame + + provider = CodexProvider("test1234", "test-session", "window-0") + started = time.monotonic() + await provider._handle_trust_prompt(idle_gap=20.0, outer_timeout=30.0) + + assert time.monotonic() - started < 5.0, "settled login menu was not recognised" + # The v2 wording borrowed the menu's footer on the unfixed head and this + # Enter picked a sign-in method for the operator. + mock_tmux.return_value.send_special_key.assert_not_called() + mock_tmux.return_value.send_keys.assert_not_called() + mock_error.assert_not_called() + + +class TestCodexInitHonoursProfileTimeout: + """Round-3 review of #731 (haofeif), P1: the per-profile override was ignored.""" + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.services.settings_service.get_server_settings") + @patch("cli_agent_orchestrator.providers.codex.get_server_settings") + @patch("cli_agent_orchestrator.providers.codex.load_agent_profile") + @patch("cli_agent_orchestrator.providers.codex.wait_until_status") + @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_profile_override_reaches_all_three_waits( + self, + mock_backend, + mock_wait_shell, + mock_wait_status, + mock_load_profile, + mock_codex_settings, + mock_base_settings, + mock_sleep, + ): + settings = {"provider_init_timeout": 60, "startup_prompt_handler_timeout": 45} + mock_codex_settings.return_value = settings + mock_base_settings.return_value = settings + profile = MagicMock() + profile.provider_init_timeout = 180 + profile.codexProfile = None + mock_load_profile.return_value = profile + mock_wait_shell.return_value = True + mock_wait_status.return_value = True + mock_backend.return_value.get_history.return_value = "OpenAI Codex (v0.98.0)" + + provider = CodexProvider("test1234", "test-session", "window-0", agent_profile="slow-box") + with ( + patch.object(provider, "_handle_trust_prompt", new_callable=AsyncMock) as mock_trust, + patch.object(provider, "_build_codex_command", return_value="codex"), + ): + await provider.initialize() + + # On the unfixed head every one of these received the server default, 60. + assert mock_wait_shell.await_args.kwargs["timeout"] == 180 + mock_trust.assert_awaited_once_with(outer_timeout=180.0) + assert mock_wait_status.await_args.kwargs["timeout"] == 180.0 + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.services.settings_service.get_server_settings") + @patch("cli_agent_orchestrator.providers.codex.get_server_settings") + @patch("cli_agent_orchestrator.providers.codex.load_agent_profile") + @patch("cli_agent_orchestrator.providers.codex.wait_until_status") + @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_unloadable_profile_falls_back_to_the_server_default( + self, + mock_backend, + mock_wait_shell, + mock_wait_status, + mock_load_profile, + mock_codex_settings, + mock_base_settings, + mock_sleep, + ): + """Timeout resolution is best-effort; the real, error-raising load comes later.""" + settings = {"provider_init_timeout": 75, "startup_prompt_handler_timeout": 45} + mock_codex_settings.return_value = settings + mock_base_settings.return_value = settings + mock_load_profile.side_effect = FileNotFoundError("no such profile") + mock_wait_shell.return_value = True + mock_wait_status.return_value = True + mock_backend.return_value.get_history.return_value = "OpenAI Codex (v0.98.0)" + + provider = CodexProvider("test1234", "test-session", "window-0", agent_profile="gone") + with patch.object(provider, "_handle_trust_prompt", new_callable=AsyncMock): + # Resolution did not abort init; the real, error-raising load in + # _build_codex_command is what reports the broken profile. + with pytest.raises(ProviderError, match="gone"): + await provider.initialize() + + assert mock_wait_shell.await_args.kwargs["timeout"] == 75 + + +class TestStartupHandlerHoldsATransitionalFrame: + """Round-4 review of #731 (haofeif), P1. + + ``_live_startup_block`` used to name the complete block ABOVE a header that + was still being drawn under it, so a capture between the TUI's redraw writes + -- trust dialog still on screen, update header down but its menu not yet -- + keyed the trust dialog's bare Enter into the update dialog appearing below + it. Reachable only while the trust dialog is not yet marked dismissed by this + handler (the flag already stops a re-send), i.e. when the operator or a + previous frame's redraw answered it; the frame is now held instead. + """ + + _TRUST_V2 = ( + " Do you trust the contents of this directory?\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + "\n" + " Press enter to continue\n" + ) + _UPDATE_HEADER_ONLY = ( + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + ) + _UPDATE = _UPDATE_HEADER_ONLY + "2. Skip\n3. Skip until next version\nPress enter to continue\n" + _SETTLED = "OpenAI Codex (v0.98.0)\n› " + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_no_key_is_sent_while_a_lower_modal_is_mid_draw(self, mock_tmux, mock_sleep): + """Frame 1 is the mid-redraw capture; frame 2 the finished update dialog. + + On the unfixed head frame 1 returned "trust" and the handler pressed Enter + -- the key that selects "1. Update now" once the menu finishes drawing. + Fixed, the only Enter is the one that confirms '3' on frame 2, and it is + sent AFTER the '3'. + """ + mid_redraw = self._TRUST_V2 + self._UPDATE_HEADER_ONLY + finished = self._TRUST_V2 + self._UPDATE + mock_tmux.return_value.get_history.side_effect = frames(mid_redraw, finished, self._SETTLED) + + provider = CodexProvider("test1234", "test-session", "window-0") + await provider._handle_trust_prompt(idle_gap=30.0, outer_timeout=30.0) + + backend = mock_tmux.return_value + keys = [ + (call[0], call[1][2:]) + for call in backend.mock_calls + if call[0] in ("send_keys", "send_special_key") + ] + assert keys == [ + ("send_keys", ("3",)), + ("send_special_key", ("Enter",)), + ], keys + + +class TestIdleGapIsJudgedOnAFreshFrame: + """Round-4 review of #731 (haofeif), P2 -- and the broader hole behind it. + + The idle-gap exit ran at the top of the loop, before the poll. Two ways that + returned the handler onto a dialog: + + * ``idle_gap`` of one second (the smallest the settings validator accepts): + the mandatory one-second sleep after a dismissal expired the gap by itself, + so a follow-up dialog already rendered was never read. + * any gap: a dismissed dialog still on screen when the gap elapsed was not + consulted, so the exit the handler's own comment said was blocked was not. + + Both left ``initialize()`` to succeed on WAITING_USER_ANSWER and the initial + message to be refused. The gap is now judged on the frame read after it + elapses, and only a frame with no dialog on it may return. + """ + + _TRUST_V1 = ( + " Since this folder is version controlled, you may wish to " + "allow Codex to work in this folder without asking for approval.\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + ) + _UPDATE = ( + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + "2. Skip\n" + "3. Skip until next version\n" + "Press enter to continue\n" + ) + _SETTLED = "OpenAI Codex (v0.98.0)\n› " + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_one_second_gap_still_reads_the_frame_after_the_sleep( + self, mock_backend, mock_sleep + ): + """Trust answered at t=0; the update dialog is on screen by t=1. + + Unfixed: the t=1 check saw 1.0 >= 1.0 and returned before reading, so the + update dialog was never dismissed (no '3' was ever sent). + """ + mock_backend.return_value.get_history.side_effect = frames( + self._TRUST_V1, self._UPDATE, self._SETTLED + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + # deadline, last_prompt_time, poll1 now, trust reset, poll2 now, update + # reset, poll3 now (settled frame, gap elapsed -> return). + with patch( + "cli_agent_orchestrator.providers.codex.time", + fake_clock(0.0, 0.0, 0.0, 0.0, 1.0, 1.0, 2.0), + ): + await provider._handle_trust_prompt(idle_gap=1.0, outer_timeout=120.0) + + mock_backend.return_value.send_keys.assert_called_once_with( + "test-session", "window-0", "3", enter_count=0 + ) + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_gap_does_not_return_onto_a_dialog_still_on_screen( + self, mock_backend, mock_sleep + ): + """The dismissed trust dialog is still rendered when the gap elapses. + + Unfixed: the t=10 check returned without a read (1 poll total). Fixed: + the frame is read, the dialog on it holds the exit, and the handler + returns on the next frame, which is quiet (3 polls). + """ + mock_backend.return_value.get_history.side_effect = frames( + self._TRUST_V1, self._TRUST_V1, self._SETTLED + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch( + "cli_agent_orchestrator.providers.codex.time", + fake_clock(0.0, 0.0, 0.0, 0.0, 10.0, 11.0), + ): + await provider._handle_trust_prompt(idle_gap=5.0, outer_timeout=120.0) + + assert mock_backend.return_value.get_history.call_count == 3 + # And the still-rendered dialog was not answered a second time. + mock_backend.return_value.send_special_key.assert_called_once() + + +class TestDismissedTrustWordingIsNotWaiting: + """Round-4 review of #731 (haofeif), P1 -- on a check that predates the PR. + + ``get_status`` matched the v1 trust wording anywhere in what it was handed, + while the v2 header, the update dialog and the login menu were all + bottom-anchored. The v0.130+ trust dialog draws the v2 header over the v1 + option text, so once it is dismissed that option line can sit above the + composer and keep the terminal WAITING_USER_ANSWER for a prompt no longer on + screen: ``initialize()`` accepts that as success and the orchestrated-input + guard then refuses the initial message into it. + """ + + _DISMISSED_TRUST = ( + " Do you trust the contents of this directory?\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + ) + # Enough chrome to push the dismissed dialog out of the 15-line bottom window. + _CHROME = "".join(f" startup line {index}\n" for index in range(16)) + _IDLE = "› \n ? for shortcuts 100% context left\n" + + def test_dismissed_wording_above_the_window_is_not_waiting(self): + provider = CodexProvider("test1234", "test-session", "window-0") + status = provider.get_status(self._DISMISSED_TRUST + self._CHROME + self._IDLE) + assert status != TerminalStatus.WAITING_USER_ANSWER + assert status == TerminalStatus.IDLE + + def test_the_same_wording_inside_the_window_is_still_waiting(self): + """The positive control: a live v1 dialog is in view and still reports.""" + provider = CodexProvider("test1234", "test-session", "window-0") + status = provider.get_status(self._CHROME + self._DISMISSED_TRUST) + assert status == TerminalStatus.WAITING_USER_ANSWER + + def test_rendered_screen_path_is_anchored_the_same_way(self): + """The default (pyte) path strips blank rows and reuses get_status, so + the anchor must hold on a padded screen too.""" + provider = CodexProvider("test1234", "test-session", "window-0") + rows = (self._DISMISSED_TRUST + self._CHROME + self._IDLE).splitlines() + screen = rows + [""] * (200 - len(rows)) + assert provider.get_status_from_screen(screen) == TerminalStatus.IDLE + + +class TestComposerIsPartOfThePositionalModel: + """Round-5 review of #731 (haofeif), P1: the live composer was absent from the + positional startup-state model, giving two opposite failures of one invariant. + """ + + _TRUST_V2 = ( + " Do you trust the contents of this directory?\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + "\n" + " Press enter to continue\n" + ) + _UPDATE_HEADER_ONLY = ( + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + ) + _UPDATE = _UPDATE_HEADER_ONLY + "2. Skip\n3. Skip until next version\nPress enter to continue\n" + _COMPOSER = "› \n ? for shortcuts 100% context left\n" + + # -- the resolver -------------------------------------------------------- + + def test_dismissed_trust_wording_above_the_live_composer_is_composer(self): + """The v0.130+ dialog leaves its option line in view after Enter.""" + frame = ( + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + self._COMPOSER + ) + assert _live_startup_block(frame) == "composer" + assert _live_startup_block(self._TRUST_V2 + self._COMPOSER) == "composer" + + def test_a_header_drawing_below_a_stale_composer_is_transitional(self): + assert _live_startup_block(self._COMPOSER + self._UPDATE_HEADER_ONLY) == "transitional" + + def test_a_complete_dialog_below_a_stale_composer_is_the_dialog(self): + assert _live_startup_block(self._COMPOSER + self._UPDATE) == "update" + assert _live_startup_block(self._COMPOSER + self._TRUST_V2) == "trust" + + def test_a_dialog_selector_line_is_not_a_composer(self): + """``› 1. Yes, ...`` is the dialog's own cursor, not an idle prompt.""" + assert _live_startup_block(self._TRUST_V2) == "trust" + + # -- get_status ------------------------------------------------------------ + + def test_get_status_is_not_waiting_on_wording_left_above_the_composer(self): + """The round-4 anchor only removed DISTANT stale text; this is the same + wording three lines above the live composer, inside the window.""" + provider = CodexProvider("test1234", "test-session", "window-0") + frame = ( + " Do you trust the contents of this directory?\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + " Press enter to continue\n" + self._COMPOSER + ) + assert provider.get_status(frame) == TerminalStatus.IDLE + + def test_get_status_is_waiting_on_a_dialog_drawn_below_the_composer(self): + provider = CodexProvider("test1234", "test-session", "window-0") + assert ( + provider.get_status(self._COMPOSER + self._UPDATE) == TerminalStatus.WAITING_USER_ANSWER + ) + assert ( + provider.get_status(self._COMPOSER + self._UPDATE_HEADER_ONLY) + == TerminalStatus.WAITING_USER_ANSWER + ) + + # -- the handler ----------------------------------------------------------- + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_no_stray_enter_into_a_composer_below_dismissed_wording( + self, mock_tmux, mock_sleep + ): + """An operator answered the prompt between polls; the composer is live. + + Unfixed: the wording won as "trust", Enter was sent into the composer, + and the handler waited on a dialog that was not there. + """ + frame = ( + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + " 2. No, ask me to approve edits and commands\n" + self._COMPOSER + ) + mock_tmux.return_value.get_history.side_effect = frames(frame) + + provider = CodexProvider("test1234", "test-session", "window-0") + outcome = await provider._handle_trust_prompt(idle_gap=30.0, outer_timeout=30.0) + + assert outcome == "settled" + mock_tmux.return_value.send_special_key.assert_not_called() + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_startup_is_not_declared_ready_under_a_modal_mid_redraw( + self, mock_tmux, mock_sleep + ): + """A stale composer above a header still being drawn. + + Unfixed: no complete block -> None -> the composer exit fired and the + update dialog that finished drawing a frame later was never answered. + """ + mock_tmux.return_value.get_history.side_effect = frames( + self._COMPOSER + self._UPDATE_HEADER_ONLY, + self._COMPOSER + self._UPDATE, + SETTLED, + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + outcome = await provider._handle_trust_prompt(idle_gap=30.0, outer_timeout=30.0) + + assert outcome == "settled" + mock_tmux.return_value.send_keys.assert_called_once_with( + "test-session", "window-0", "3", enter_count=0 + ) + + +class TestOuterCapWithADialogUpFailsInitialization: + """Round-5 review of #731 (haofeif), P2: the handler's cap exhaustion was + logged and swallowed, and ``initialize()`` then accepted the still-live + dialog through the WAITING_USER_ANSWER path meant for the login menu.""" + + _TRUST_V1 = ( + " Since this folder is version controlled, you may wish to " + "allow Codex to work in this folder without asking for approval.\n" + "› 1. Yes, allow Codex to work in this folder without asking for approval\n" + ) + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_handler_reports_blocked_when_the_dialog_outlives_the_cap( + self, mock_backend, mock_sleep + ): + mock_backend.return_value.get_history.return_value = self._TRUST_V1 + + provider = CodexProvider("test1234", "test-session", "window-0") + # deadline, last_prompt_time, poll1 now, trust reset, poll2 now (still up), cap. + with patch( + "cli_agent_orchestrator.providers.codex.time", + fake_clock(0.0, 0.0, 0.0, 0.0, 5.0, 100.0), + ): + outcome = await provider._handle_trust_prompt(idle_gap=30.0, outer_timeout=60.0) + + assert outcome == "blocked" + # Dismissed once; a still-rendered dialog is never re-keyed. + mock_backend.return_value.send_special_key.assert_called_once() + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.wait_until_status") + @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_initialize_fails_instead_of_succeeding_on_the_stuck_dialog( + self, mock_tmux, mock_wait_shell, mock_wait_status, mock_sleep + ): + """Unfixed: ``initialize()`` returned True here (the dialog reads as + WAITING_USER_ANSWER), and every assign/handoff was then refused.""" + mock_wait_shell.return_value = True + mock_wait_status.return_value = True + mock_tmux.return_value.get_history.return_value = self._TRUST_V1 + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch.object(provider, "_handle_trust_prompt", new=AsyncMock(return_value="blocked")): + with pytest.raises(TimeoutError, match="could not be dismissed"): + await provider.initialize() + mock_wait_status.assert_not_called() + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.wait_until_status") + @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_a_dialog_that_appears_after_the_handler_is_not_readiness( + self, mock_tmux, mock_wait_shell, mock_wait_status, mock_sleep + ): + """WAITING_USER_ANSWER satisfied the readiness wait, but the pane shows + an update dialog, not the login menu.""" + update = ( + "✨ Update available! 0.142.5 -> 0.144.5\n" + "1. Update now (runs npm install -g @openai/codex)\n" + "2. Skip\n" + "3. Skip until next version\n" + "Press enter to continue\n" + ) + mock_wait_shell.return_value = True + mock_wait_status.return_value = True + mock_tmux.return_value.get_history.return_value = update + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch.object(provider, "_handle_trust_prompt", new=AsyncMock(return_value="settled")): + with pytest.raises(TimeoutError, match="startup dialog on screen"): + await provider.initialize() + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.wait_until_status") + @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_the_login_menu_is_still_a_successful_start( + self, mock_tmux, mock_wait_shell, mock_wait_status, mock_sleep + ): + login = ( + " Sign in with ChatGPT to use Codex as part of your paid plan\n" + "> 1. Sign in with ChatGPT\n" + " 2. Sign in with Device Code\n" + " 3. Provide your own API key\n" + "\n" + " Press enter to continue\n" + ) + mock_wait_shell.return_value = True + mock_wait_status.return_value = True + mock_tmux.return_value.get_history.return_value = login + + provider = CodexProvider("test1234", "test-session", "window-0") + assert await provider.initialize() is True + + +class TestTransitionalIsAStartupOnlyState: + """Round-7 review of #731 (gutosantos82, blocking; haofeif, P3). + + ``transitional`` names a lone recognised startup header with no completing + footer/menu. During startup that is a modal mid-redraw and must hold every + exit -- but ``get_status`` runs for the whole session, and on an + initialized terminal the same shape is the model QUOTING a startup phrase + in its own reply. Mapping it to WAITING_USER_ANSWER flipped frames that + ``main`` classified PROCESSING, so a supervisor saw a generating worker as + blocked on input. The state is now honoured only until initialization is + over. ``initialize()``'s post-readiness check, which legitimately cares, + also stopped condemning it on a single read: a login menu caught between + redraw writes at that one instant now gets re-read instead of failing an + otherwise-valid start. + """ + + # The reviewer's reproduction shape: an active turn (user message, reply + # bullets, live progress spinner) whose prose quotes one startup phrase. + _QUOTED_HEADERS = [ + "Do you trust the contents of this directory?", + "Sign in with ChatGPT to use Codex as part of your paid plan", + "✨ Update available! 0.142.5 -> 0.144.5", + ] + + @staticmethod + def _mid_turn_frame(quoted: str) -> str: + return ( + "› tell me what the codex first-run flow looks like\n" + "• The first-run flow can show a few dialogs. One asks:\n" + f" {quoted}\n" + " and there may be more before the composer appears.\n" + "• Working (3s • esc to interrupt)\n" + ) + + @pytest.mark.parametrize("quoted", _QUOTED_HEADERS) + def test_quoted_startup_phrase_mid_turn_is_processing_once_initialized(self, quoted): + frame = self._mid_turn_frame(quoted) + # The precondition the mapping decision rests on: the resolver DOES read + # this frame as transitional, so the outcome below is the gating. + assert _live_startup_block(frame) == "transitional" + + provider = CodexProvider("test1234", "test-session", "window-0") + provider._initialized = True + assert provider.get_status(frame) == TerminalStatus.PROCESSING + + @pytest.mark.parametrize("quoted", _QUOTED_HEADERS) + def test_the_same_frame_is_still_waiting_during_startup(self, quoted): + """The conservative reading is kept where it is load-bearing: + ``initialize()``'s readiness wait may accept a mid-redraw frame as + WAITING, and its post-readiness re-reads then resolve it.""" + provider = CodexProvider("test1234", "test-session", "window-0") + assert provider.get_status(self._mid_turn_frame(quoted)) == ( + TerminalStatus.WAITING_USER_ANSWER + ) + + _LOGIN_HEADER_ONLY = ( + " Sign in with ChatGPT to use Codex as part of your paid plan\n" + "> 1. Sign in with ChatGPT\n" + ) + _LOGIN_COMPLETE = _LOGIN_HEADER_ONLY + ( + " 2. Sign in with Device Code\n" + " 3. Provide your own API key\n" + "\n" + " Press enter to continue\n" + ) + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.wait_until_status") + @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_post_readiness_rides_out_a_login_menu_mid_redraw( + self, mock_tmux, mock_wait_shell, mock_wait_status, mock_sleep + ): + """Header drawn, footer not yet, at the exact post-readiness instant. + + On the previous head this one-shot read raised TimeoutError against a + start the handler's own loop would have ridden out. + """ + mock_wait_shell.return_value = True + mock_wait_status.return_value = True + mock_tmux.return_value.get_history.side_effect = frames( + self._LOGIN_HEADER_ONLY, self._LOGIN_COMPLETE + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch.object(provider, "_handle_trust_prompt", new=AsyncMock(return_value="settled")): + assert await provider.initialize() is True + + @pytest.mark.asyncio + @patch("cli_agent_orchestrator.providers.codex.asyncio.sleep", new_callable=AsyncMock) + @patch("cli_agent_orchestrator.providers.codex.wait_until_status") + @patch("cli_agent_orchestrator.providers.codex.wait_for_shell") + @patch("cli_agent_orchestrator.providers.codex.get_backend") + async def test_a_frame_that_never_resolves_still_fails_the_start( + self, mock_tmux, mock_wait_shell, mock_wait_status, mock_sleep + ): + """The ride-out is bounded: a header that never completes is a failed + start, exactly as before -- just judged on the last re-read, not the + first.""" + mock_wait_shell.return_value = True + mock_wait_status.return_value = True + mock_tmux.return_value.get_history.return_value = self._LOGIN_HEADER_ONLY + + provider = CodexProvider("test1234", "test-session", "window-0") + with patch.object(provider, "_handle_trust_prompt", new=AsyncMock(return_value="settled")): + with pytest.raises(TimeoutError, match="startup dialog on screen"): + await provider.initialize() + + # One initial read plus every re-read: the bound is the constant, not luck. + assert ( + mock_tmux.return_value.get_history.call_count == 1 + POST_READINESS_TRANSITIONAL_REREADS + ) diff --git a/test/providers/test_startup_handler_nonblocking.py b/test/providers/test_startup_handler_nonblocking.py index 442166d00..9c16ebfba 100644 --- a/test/providers/test_startup_handler_nonblocking.py +++ b/test/providers/test_startup_handler_nonblocking.py @@ -19,9 +19,19 @@ bug #494 reports: "mirrors ClaudeCodeProvider" docstrings that were never true because the body stayed fully sync). -ClaudeCodeProvider._handle_startup_prompts is deliberately excluded from both -layers: PR #451 (which converts it) is open/changes-requested, not merged, as -of this test. +ClaudeCodeProvider._handle_startup_prompts is not in the parametrized lists +below. It was excluded when PR #451 (which converts it) was still open; #451 has +since merged and the conversion is at head, so adding it here is a follow-up. + +``CodexProvider._handle_trust_prompt`` and ``CodexProvider.initialize`` were +added later (caom-7it), for the same reason and against the same property. Codex +is not the last remaining gap — kiro_cli, opencode_cli and cursor_cli still make +loop-side backend calls in ``initialize()`` — but it has the slowest init of the +group, so its stall was the one that showed up under a concurrent fan-out. +``initialize`` is covered as well as the handler because its OWN send_keys / +get_pane_current_command calls are blocking subprocess execs too, not just the +prompt poll; it gets its own layer (2b) because a tick count cannot measure a +coroutine that awaits a fixed ``asyncio.sleep`` of its own. """ import asyncio @@ -32,6 +42,7 @@ from cli_agent_orchestrator.models.terminal import TerminalStatus from cli_agent_orchestrator.providers.antigravity_cli import AntigravityCliProvider +from cli_agent_orchestrator.providers.codex import CodexProvider from cli_agent_orchestrator.providers.copilot_cli import CopilotCliProvider from cli_agent_orchestrator.providers.kimi_cli import KimiCliProvider @@ -42,6 +53,17 @@ _BLOCKING_CALL_SECONDS = 0.05 _TICKER_INTERVAL_SECONDS = 0.01 +# Blocking latency and threshold for the LONGEST-GAP probe (layer 2b). Larger +# than _BLOCKING_CALL_SECONDS so the two outcomes sit far apart: offloaded, the +# worst gap stays near the ticker interval (0.01s) and must clear a 10x jitter +# margin; left on the loop, the gap is at least the full 0.2s blocking call. +_MAX_GAP_BLOCKING_SECONDS = 0.2 +_MAX_ACCEPTABLE_GAP_SECONDS = 0.1 + +# A codex frame showing the idle composer and no blocking dialog, so +# _handle_trust_prompt returns after a single poll. +_CODEX_READY_FRAME = "OpenAI Codex (v0.145.0)\n› Explain this codebase\n gpt-5.6-sol high · /tmp\n" + def _blocking_history(ready_output: str): """Build a side_effect that blocks the calling thread, then returns. @@ -69,6 +91,12 @@ async def _run_with_heartbeat_probe(handler_coro) -> int: a worker thread. Zero would mean the handler's "blocking" call actually ran on the event-loop thread and starved everything else -- the exact pathology issue #494 (and PR #451 before it) fixes. + + The zero reading is load-bearing and depends on the ticker NOT having run + yet: ``create_task`` only schedules it, so a handler that never yields + starves it before its very first tick. Do not "warm up" the ticker before + ``await handler_coro`` -- that makes the count non-zero unconditionally and + silently voids every case below. """ ticks = 0 stop = asyncio.Event() @@ -92,6 +120,58 @@ async def _ticker() -> None: return ticks +async def _run_with_max_gap_probe(handler_coro) -> tuple[int, float]: + """Like ``_run_with_heartbeat_probe``, but returns ``(ticks, worst gap)``. + + For coroutines that ``await`` on their own for reasons unrelated to the + offload -- ``CodexProvider.initialize()``'s fixed ``asyncio.sleep(2.0)`` + shell warm-up -- the tick COUNT proves nothing: the loop ticks during those + sleeps whether or not the backend calls are offloaded. The LONGEST interval + between two ticks does prove it, because it measures the stall itself and + is therefore bounded by the size of the largest un-offloaded call rather + than by the coroutine's total runtime. + + Unlike the tick-count probe, this one deliberately brackets the handler + with ticks: + + * At least one tick BEFORE it runs, so ``last`` is anchored -- otherwise a + stall that happens before the ticker's first tick goes unmeasured. + * At least one tick AFTER it returns, because a gap is only recorded when + the ticker resumes -- a handler that blocks and then returns without ever + suspending would otherwise look gap-free because the cancel lands first. + """ + ticks = 0 + max_gap = 0.0 + stop = asyncio.Event() + + async def _ticker() -> None: + nonlocal ticks, max_gap + last = time.monotonic() + while not stop.is_set(): + await asyncio.sleep(_TICKER_INTERVAL_SECONDS) + now = time.monotonic() + max_gap = max(max_gap, now - last) + last = now + ticks += 1 + + ticker_task = asyncio.create_task(_ticker()) + try: + while ticks < 1: + await asyncio.sleep(_TICKER_INTERVAL_SECONDS) + await handler_coro + settled = ticks + while ticks == settled: + await asyncio.sleep(_TICKER_INTERVAL_SECONDS) + finally: + stop.set() + ticker_task.cancel() + try: + await ticker_task + except asyncio.CancelledError: + pass + return ticks, max_gap + + async def _kimi_handler_run() -> int: """KimiCliProvider._handle_startup_dialog: one poll, already-ready output.""" provider = KimiCliProvider("t1", "sess", "win") @@ -144,6 +224,50 @@ async def _copilot_shell_ready_run() -> int: ) +async def _codex_trust_prompt_run() -> int: + """CodexProvider._handle_trust_prompt: one poll, idle composer, no dialog.""" + provider = CodexProvider("t1", "sess", "win") + mock_backend = MagicMock() + mock_backend.get_history.side_effect = _blocking_history(_CODEX_READY_FRAME) + with patch("cli_agent_orchestrator.providers.codex.get_backend", return_value=mock_backend): + return await _run_with_heartbeat_probe(provider._handle_trust_prompt(outer_timeout=5.0)) + + +async def _codex_initialize_run() -> tuple[int, float]: + """CodexProvider.initialize: its OWN backend calls, not just the prompt poll. + + ``get_pane_current_command`` and both ``send_keys`` calls are blocking + subprocess execs as well. Stubbed at the longer + ``_MAX_GAP_BLOCKING_SECONDS`` because this case is measured by worst gap + (layer 3), not tick count -- ``initialize()`` awaits a fixed + ``asyncio.sleep`` warm-up that ticks the ticker either way. + """ + + def _blocking(*_args, **_kwargs): + time.sleep(_MAX_GAP_BLOCKING_SECONDS) + + def _blocking_pane_command(*_args, **_kwargs) -> str: + time.sleep(_MAX_GAP_BLOCKING_SECONDS) + return "zsh" + + mock_backend = MagicMock() + mock_backend.send_keys.side_effect = _blocking + mock_backend.get_pane_current_command.side_effect = _blocking_pane_command + mock_backend.get_history.return_value = _CODEX_READY_FRAME + + provider = CodexProvider("t1", "sess", "win") + with ( + patch("cli_agent_orchestrator.providers.codex.get_backend", return_value=mock_backend), + patch( + "cli_agent_orchestrator.providers.codex.get_server_settings", + return_value={"provider_init_timeout": 60, "startup_prompt_handler_timeout": 20}, + ), + patch("cli_agent_orchestrator.providers.codex.wait_for_shell", return_value=True), + patch("cli_agent_orchestrator.providers.codex.wait_until_status", return_value=True), + ): + return await _run_with_max_gap_probe(provider.initialize()) + + # --------------------------------------------------------------------------- # Layer 1: structural pin -- must be real coroutine functions. # --------------------------------------------------------------------------- @@ -153,6 +277,7 @@ async def _copilot_shell_ready_run() -> int: ("antigravity:_handle_startup_dialog", AntigravityCliProvider._handle_startup_dialog), ("copilot:_accept_trust_prompts", CopilotCliProvider._accept_trust_prompts), ("copilot:_wait_for_shell_ready", CopilotCliProvider._wait_for_shell_ready), + ("codex:_handle_trust_prompt", CodexProvider._handle_trust_prompt), ] @@ -170,6 +295,7 @@ def test_handler_is_a_real_coroutine_function(name, handler): ("antigravity:_handle_startup_dialog", _antigravity_handler_run), ("copilot:_accept_trust_prompts", _copilot_trust_run), ("copilot:_wait_for_shell_ready", _copilot_shell_ready_run), + ("codex:_handle_trust_prompt", _codex_trust_prompt_run), ] @@ -180,6 +306,33 @@ async def test_handler_does_not_starve_event_loop(name, run_case): assert ticks > 0, f"{name}: event loop starved while its backend call was in flight" +# --------------------------------------------------------------------------- +# Layer 2b: longest-gap probe -- for coroutines whose own awaits tick the +# ticker regardless of whether their backend calls are offloaded. +# +# ``CodexProvider.initialize()`` awaits a fixed ``asyncio.sleep(2.0)`` shell +# warm-up, so layer 2's tick COUNT would pass even with every backend call left +# on the event loop. The metric that survives is the LONGEST interval between +# two ticks: it measures the stall itself, so it is bounded by the size of the +# largest un-offloaded call rather than by the coroutine's total runtime. +# --------------------------------------------------------------------------- + +_MAX_GAP_CASES = [ + ("codex:initialize", _codex_initialize_run), +] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("name,run_case", _MAX_GAP_CASES, ids=[c[0] for c in _MAX_GAP_CASES]) +async def test_handler_never_stalls_the_loop_for_a_full_backend_call(name, run_case): + ticks, max_gap = await run_case() + assert ticks > 0, f"{name}: ticker never sampled" + assert max_gap < _MAX_ACCEPTABLE_GAP_SECONDS, ( + f"{name}: event loop stalled for {max_gap:.3f}s " + f"(a {_MAX_GAP_BLOCKING_SECONDS}s backend call ran on the loop thread)" + ) + + # --------------------------------------------------------------------------- # Layer 3: cleanup() lock offload -- _unregister_mcp_servers must not block # the event loop when cleanup() is called from an async context (e.g.