Skip to content

fix(launch): deliver the initial message via POST /sessions instead of a dropped second request - #566

Open
tedswinyar wants to merge 18 commits into
awslabs:mainfrom
tedswinyar:agent/caom-7it-fix
Open

tedswinyar wants to merge 18 commits into
awslabs:mainfrom
tedswinyar:agent/caom-7it-fix

Conversation

@tedswinyar

@tedswinyar tedswinyar commented Aug 6, 2026

Copy link
Copy Markdown
Member

Problem

cao launch --agents <worker> <message> created the session, then delivered the message as a separate POST /terminals/{id}/input. When provider init outlives the client's read timeout that second request never fires, and the message is silently dropped — the worker sits idle with a healthy TUI and no task. Reproduced on codex workers in a mixed fan-out.

Approach

POST /sessions already accepts initial_message, and session_service.create_session sets defer_init=initial_message is not None, so the server owns init, delivery and resubmit after the response returns. mcp_server and ops_mcp_server already use this; cao launch was the last client that didn't.

launch.py now sends initial_message in the create body (alongside env_vars, keeping secrets out of the HTTP access log) and deletes the client-side /input POST, the pre-send readiness wait, and the settling sleep. One delivery ships. The create call returns to mcp_request_timeout; _create_session_timeout / _effective_init_timeout / _CREATE_OVERHEAD_MARGIN are gone.

The hard part: making the deferred terminal safe to poll

Deferring delivery means a client can poll before the task is delivered. Three review rounds each found the same class of hole — a release point keyed on a status value rather than on when that value was earned:

  1. Release when send_input() returns. Holed: send_input only calls notify_input_sent(), which arms the next transition without touching the cached status. A poller reads the stale pre-send IDLE for ~1s. Reproduced returning 0.93s before the first PROCESSING signal.
  2. Release after _confirm_worker_started_or_resubmit. Holed by @haofeif: _DEFERRED_STARTED_STATUSES contains COMPLETED, and provider startup output can latch one before dispatch. Confirmation succeeded in 0.008s on a status earned before the send.
  3. Release on post-dispatch evidence — current. status_monitor gains one read-only accessor, output_generation(), exposing the _capture_generation counter fix(status): self-heal a stuck-PROCESSING terminal from a bounded, detector-routed pane capture #712 already maintains. It advances only in notify_input_sent (new turn) and _process_chunk (real output). _run samples it after the send; confirmation requires a started status and a strictly greater generation. A cached pre-dispatch COMPLETED can no longer confirm.

While held, reported_status() masks IDLE/COMPLETED as UNKNOWN at the three outward surfaces only (get_terminal, list_siblings, the session listing). WAITING_USER_ANSWER / PROCESSING / ERROR and every internal status_monitor caller keep the raw state.

Event-inbox backends (herdr) are exempt via dispatch_generation=None. They start no FIFO reader, so the generation never advances from output and gating them would make confirmation unsatisfiable — every resubmit burned, then teardown of a working worker. They need no gate: get_status derives their status on demand, so nothing is cached to go stale.

Scope note

Delivery is deferred on the headless path only (bool(message) and headless). Deferring on the attach path would race the pre-attach readiness poll against the agent's first turn. Covered by test_launch_non_headless_does_not_defer_init.

Split

The Codex startup work that was here — idle-gap redefinition, first-run login-menu liveness, trust-copy positioning, and the event-loop offload — is now #731. It shares no files with this PR and is independently reviewable.

Verification

  • 306 passed across test_launch.py, test_terminal_service_full.py, test_session_service.py, test_deferred_submit_verification.py, test_status_monitor.py.
  • Mutation-tested: removing the generation clause fails the pre-dispatch test while its two siblings still pass; removing coro.close() or bypassing reported_status at either call site fails exactly the three tests written for them.
  • The masking wiring at session_service.get_session and list_siblings is now pinned — previously deleting either call left 162 and 130 tests passing respectively.
  • black --check / isort --check-only clean.

@codecov-commenter

codecov-commenter commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@948c3d8). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #566   +/-   ##
=======================================
  Coverage        ?   91.96%           
=======================================
  Files           ?      207           
  Lines           ?    29478           
  Branches        ?        0           
=======================================
  Hits            ?    27110           
  Misses          ?     2368           
  Partials        ?        0           
Flag Coverage Δ
unittests 91.96% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adjusts cao launch and Codex provider initialization to avoid client-side timeouts that prevent the initial message from being delivered when provider initialization is slow, and reduces event-loop blocking during Codex startup.

Changes:

  • Add a dedicated POST /sessions timeout computation that covers server-side provider initialization (including profile overrides and fixed overhead), with a readiness-wait floor.
  • Offload Codex backend subprocess-backed calls in initialize() / _handle_trust_prompt() via asyncio.to_thread to avoid starving the server event loop.
  • Add/extend unit tests covering session-create budgeting, profile override behavior, partial settings resilience, and Codex init non-starvation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/cli_agent_orchestrator/cli/commands/launch.py Adds _create_session_timeout / _effective_init_timeout and applies widened timeout only to POST /sessions, plus centralizes readiness timeout constant.
src/cli_agent_orchestrator/providers/codex.py Offloads blocking backend calls during trust-prompt handling and initialization; reads trust-prompt timeout from server settings.
test/cli/commands/test_launch.py Adds tests asserting correct create-session budgeting, readiness floor, scaling, and profile override handling.
test/providers/test_codex_provider_unit.py Adds heartbeat-based tests to assert Codex init/trust handling does not starve the event loop; tests configured trust-prompt timeout wiring.
Suppressed comments (1)

src/cli_agent_orchestrator/providers/codex.py:635

  • The TimeoutError message is now misleading: the init timeout is configurable via server settings, but this hard-codes "60 seconds". If provider_init_timeout is raised, the error should report the actual configured value to aid debugging.
        if not await wait_until_status(
            self.terminal_id,
            {TerminalStatus.IDLE, TerminalStatus.COMPLETED},
            timeout=float(get_server_settings()["provider_init_timeout"]),
            polling_interval=1.0,
        ):
            raise TimeoutError("Codex initialization timed out after 60 seconds")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +2409 to +2411
finally:
beat.cancel()
return max_gap, ticks

@haofeif haofeif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Codex event-loop fix looks sound, but the new create timeout still expires before several supported providers can finish a successful initialization, preserving the initial-message loss this PR is intended to fix.

"""
init_timeout = _effective_init_timeout(agent_profile, settings)
return max(
2 * init_timeout

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Budget the providers' actual initialization paths. startup_prompt_handler_timeout is only the idle gap after a prompt; the Claude handler's hard outer cap is another full provider_init_timeout. A successful Claude path can therefore take shell(T) + handler(T) + ready(T) + the 5s input settle: 185s with defaults, while this returns 170s (and about 545s versus 410s for a profile with T=180). This is not Claude-specific: Kimi can take T + 2max(120,T) = 300s by default, Kiro's legacy fallback can take 4T = 240s, and Antigravity/OpenCode/Hermes also have successful paths beyond 170s. In each case requests can still time out before /sessions responds, so the second /input request is skipped and the initial message is silently lost—the exact failure this PR addresses. Please derive a bound from the real provider path, or avoid the race by putting the message in the existing CreateSessionBody.initial_message field so /sessions uses deferred initialization and delivery.

@call-me-ram call-me-ram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug is real and the root-cause trace is correct — nice find, and the diagnosis is unusually well-evidenced. I confirmed every link in the chain against head: launch sends message only as a second request (launch.py:433-447), POST /sessionssession_service.create_sessioncreate_terminal runs await provider_instance.initialize() inline (terminal_service.py:445) because defer_init is False when no initial_message is present, and requests.exceptions.ReadTimeout is swallowed by the blanket except requests.exceptions.RequestException at launch.py:459 into "Failed to connect to cao-server" — so the create-then-send branch never runs while the server happily finishes init and keeps the session. The codex event-loop offload (item 4) is also correct, is a genuine parity gap versus #451, and its three tests are real regression guards (I reverted codex.py to upstream/main in a worktree and all three fail).

Where I land differently is the fix. The timeout widening does not close the hole it claims to close — it closes it for codex and leaves it open for most other providers on default settings — and POST /sessions already has a purpose-built mechanism for this exact problem that cao launch is the last client not using. Details below.

Must-fix

1. launch.py:106-140 — the new budget is below the worst-case successful init for 7 of 9 providers on default settings, so the silent drop survives for everything except the provider that was reported.

I derived each provider's worst-case successful initialize() wall clock directly from head by stubbing every bounded wait to report the timeout it was handed. With defaults (provider_init_timeout=60, startup_prompt_handler_timeout=20), _create_session_timeout returns 170s, against:

provider worst-case successful init vs 170s
antigravity_cli 10 + max(180,T) + max(180,T) = 370s +200
kimi_cli T + max(120,T) + max(120,T) = 300s +130
kiro_cli (non-yolo TUI→--legacy-ui fallback) 4T = 240s +70
copilot_cli (shell-ready fallback) T + T + 10 + 60 = 190s +20
claude_code T + T + T + 5 = 185s +15
opencode_cli T + 120 = 180s +10
hermes T + 120 = 180s +10
codex T + 20 + T = 140s OK
cursor_cli (under) OK

The formula 2*T + startup_prompt_handler_timeout + 30 encodes codex's shape specifically, and two of its three assumptions are false elsewhere:

  • startup_prompt_handler_timeout is only the idle gap, not the handler's cap. claude_code._handle_startup_prompts and kimi_cli/antigravity_cli._handle_startup_dialog take outer_timeout — a full provider_init_timeout (or max(120/180, T)) — as the hard cap (claude_code.py:565-575, kimi_cli.py:523-533, antigravity_cli.py:563-576). So the middle term is T-or-larger, not 20.
  • "the init timeout applies TWICE per init" isn't a general invariant: kimi_cli floors its two readiness waits at max(120, T) (kimi_cli.py:607), antigravity_cli at max(180, T) (antigravity_cli.py:660-668), and kiro_cli has a four-window path when the TUI attempt times out and it retries with --legacy-ui (kiro_cli.py:285, 349, 363, 381).

With a profile override the gap widens rather than closes: for provider_init_timeout: 180, claude_code can legitimately spend 3*180+5 = 545s against a 410s budget. _CREATE_OVERHEAD_MARGIN doesn't help — the shortfall is structural, not marginal.

I confirmed the residual failure is byte-identical to the reported one at head:

Error: Failed to connect to cao-server: HTTPConnectionPool(host='127.0.0.1', port=8001): Read timed out. (read timeout=170)
posts made: ['http://127.0.0.1:9889/sessions']

One POST, message never sent, exit 1 — the original bug, just with a bigger number in it.

This is haofeif's P1 and I confirm it independently, with the same numbers for claude_code (185) and kimi_cli (300); antigravity_cli is worse than their "also beyond 170s" — it's 370s.

2. launch.py:390-394 — wrong layer: POST /sessions already accepts initial_message and defers init + delivery server-side, and cao launch is the only client that doesn't use it.

CreateSessionBody.initial_message exists (api/main.py:215, 219), and session_service.create_session sets defer_init=initial_message is not None (session_service.py:88), so the response returns as soon as the session and terminal record exist and _schedule_deferred_init (terminal_service.py:768-896) owns init and delivery. Both sibling clients already do this: mcp_server/server.py:321-335 and ops_mcp_server/server.py:123-125 pass {"initial_message": ...} — and mcp_server keeps timeout=_mcp_timeout() (30s) on that call, which is direct evidence that 30s is the right budget once the body field is used.

The layer matters beyond aesthetics, because client-side create-then-send is unrecoverable by construction: on ReadTimeout the client has no terminal_id — the identity of the thing it would need to retry against only arrives in the response that timed out. So the create call can never be made safe by widening it; it can only be made less likely to fail, and its failure is always a silent orphan. The deferred path is also strictly more robust than what the client can do: it confirms the worker actually started and re-submits (_confirm_worker_started_or_resubmit), and on failure notifies the caller's inbox and tears the worker down instead of leaving a healthy idle TUI with a frozen last_active.

Concretely: send {"initial_message": message} in the create body when message is set, keep mcp_request_timeout on the create call, and drop the client-side /input POST. Two things to handle: the deferred path returns TerminalStatus.UNKNOWN (terminal_service.py:460), so the --async branch should just report and return rather than waiting for IDLE; and the non---async branch should go straight to poll_until_done. Note also that message is silently ignored today on the attach (non---headless) path — worth deciding deliberately rather than inheriting.

No double-delivery risk in either direction here, since the client send would be removed in the same change — but please don't ship both paths concurrently, because _schedule_deferred_init + a client /input would deliver twice.

3. launch.py:393 — the widened value is a scalar, so it also raises the connect timeout from 30s to 170s (unbounded with a profile override), and a genuine connection failure is still reported with the same string as a slow init.

requests applies a scalar timeout to both connect and read. So the change trades a slow-init failure for a new one: with cao-server unreachable rather than refused (filtered port, wrong host, stale SERVER_HOST), cao launch now blocks for 170s with zero output after the confirmation prompt, where it used to fail in 30s. And AgentProfile.provider_init_timeout is an unbounded Optional[int] (models/agent_profile.py:66) — I measured provider_init_timeout: 3600 producing a 7250s (121 min) budget.

Minimal fix, whichever layer you land on: pass a tuple, timeout=(connect, read) with a small fixed connect (5-10s) and the computed read budget; cap the derived read budget; and split the handler so ReadTimeout is distinguishable from ConnectionError — right now launch.py:459 flattens "server is initialising, your message was NOT delivered, a session may now exist" into "Failed to connect to cao-server", which is the misleading message the PR body itself calls out as part of the bug.

Non-blocking

  1. codex.py:458-465 and launch.py:16-27 — "codex was the last provider still running its init's blocking subprocess calls directly on cao-server's single shared event loop" is not true, and the claim is now baked into source comments. After this PR, kiro_cli.initialize() still makes four loop-side backend calls (kiro_cli.py:289, 344, 361, 380), plus opencode_cli.py:160 and cursor_cli.py:589. kiro_cli is DEFAULT_PROVIDER. The offload is still right; please just reword the causal claim, since a future reader will otherwise trust "codex was last" and skip these.

  2. Item 5 in the PR body (settings.get("startup_prompt_handler_timeout", 20)) guards a state that cannot occur, and the guard is self-defeating anyway. get_server_settings() starts from dict(_SERVER_DEFAULTS) and merges only keys already in it (settings_service.py:249-250), so a hand-edited partial settings.json can never yield a missing key. And within the same expression _effective_init_timeout hard-indexes settings["provider_init_timeout"] — I passed {"startup_prompt_handler_timeout": 20} and got KeyError('provider_init_timeout'), so the guard protects one of two keys against an impossible input. test_create_session_timeout_survives_partial_settings asserts against a synthetic dict get_server_settings() cannot return. Either drop both, or make it consistent. Related: codex.py:626 hard-indexes the same key that launch.py:138 guards — pick one convention.

  3. The codex non-starvation tests belong in test/providers/test_startup_handler_nonblocking.py. That module already exists for exactly this property, is parametrized over _COROUTINE_TARGETS / _HEARTBEAT_CASES, and covers kimi_cli/antigravity_cli/copilot_cli from #494. Adding codex there is ~2 list entries instead of a bespoke 50-line _heartbeat_gap helper in the codex file, and it gives the still-loop-side providers from item 1 an obvious home. That module also does cancel()await → suppress CancelledError (test_startup_handler_nonblocking.py:87-91), which is Copilot's inline point on test_codex_provider_unit.py:2410that one is correct, beat.cancel() without an await can emit "Task was destroyed but it is pending". Moving the tests fixes it for free.

  4. codex.py:635 still raises TimeoutError("Codex initialization timed out after 60 seconds") while the timeout is configurable. Copilot suppressed this one; it's worth taking, since this PR is what makes the neighbouring timeout configurable — interpolate the value like claude_code.py:701 does.

  5. _READINESS_WAIT_TIMEOUT as a floor penalises deliberately-low configs. An operator or CI run that sets provider_init_timeout: 5 for fast failure now still waits 120s on create. Also, the floor's stated rationale ("the create call gives up on work the very next step is still willing to wait for") doesn't hold on the synchronous path — the readiness wait runs after create returns and init has already completed, so the two waits are sequential, not competing.

  6. _effective_init_timeout re-loads the profile that launch already loaded at launch.py:288 on the non---yolo path. Pass it through. Also note the two load sites disagree on what they catch — (FileNotFoundError, RuntimeError) vs bare Exception.

Asks

  • mcp_server/app_tools.py:404 calls _post_json("/sessions", params) with no initial_message and timeout=MCP_REQUEST_TIMEOUT, so it has the same synchronous-init-outlives-the-client hole. Not this PR's job to fix, but the body's "This closes the hole for every provider" should be scoped to "for cao launch" — and if the answer to must-fix 2 is initial_message, this becomes a natural follow-up.
  • Whatever the create budget ends up being, cao launch prints nothing between "Proceed?" and the result. A 170s+ silent wait needs some progress feedback.
  • The "Known limitation (deferred)" section is honest and I agree with the diagnosis there — but adopting initial_message dissolves it rather than deferring it, since the client stops needing to guess the server's init budget at all.

On haofeif's review: their single P1 is correct and I reproduce it with the same numbers. I'd add that it's not only an arithmetic shortfall — their suggested alternative (CreateSessionBody.initial_message) is the one that removes the race rather than resizing it, and it's already the pattern used by both other POST /sessions clients.

What I verified

  • git fetch upstream pull/566/headd041a7d; all reading and testing at that rev in an isolated worktree.
  • Diagnosis trace: launch.py:433-447 (second request), launch.py:459 (blanket RequestException), session_service.py:88 (defer_init=initial_message is not None), terminal_service.py:435-445 (inline initialize()), api/main.py:215-232, 1919-1976 (CreateSessionBody.initial_message), terminal_service.py:768-896 (_schedule_deferred_init + resubmit + failure notification). Confirmed.
  • Per-provider budget derivation: script stubbing wait_for_shell / wait_until_status / each startup handler / wait_until_input_ready to report the timeout each was handed, run against real initialize() for claude_code, kimi_cli, codex, opencode_cli (antigravity_cli, hermes, kiro_cli, copilot_cli read statically where the probe aborted on a missing binary). Output: client budget 170s; claude_code 185, kimi_cli 300, opencode_cli 180, codex 140. Table above.
  • Residual failure repro: requests.post stubbed to raise ReadTimeout at head → exit 1, Failed to connect to cao-server: ... (read timeout=170), exactly one POST, message never sent, no terminal id in output.
  • Scalar/unbounded timeout: _create_session_timeout returns int (scalar → governs connect too); with profile.provider_init_timeout = 3600 it returns 7250.
  • Self-defeating settings guard: _create_session_timeout({"startup_prompt_handler_timeout": 20}, "any")KeyError('provider_init_timeout').
  • Loop-side survey: grep for un-offloaded get_backend(). across all 9 providers → kiro_cli ×4, opencode_cli ×1, cursor_cli ×1 remain; copilot_cli's _send_enter/_send_key are correctly offloaded inside _accept_trust_prompts.
  • Tests: test/cli/commands/test_launch.py → 58 passed. test/providers/test_codex_provider_unit.py + test/services/test_session_service.py → 188 passed, 3 skipped. Reverting providers/codex.py to upstream/main and re-running TestCodexInitEventLoopBlocking → 3 failed (genuine guards).

@tedswinyar tedswinyar changed the title fix(launch): stop dropping the initial message when provider init outlives the client fix(launch): deliver the initial message via POST /sessions instead of a dropped second request Aug 17, 2026
@tedswinyar

Copy link
Copy Markdown
Member Author

Reworked per @haofeif's and @call-me-ram's reviews and rebased onto current main.

Adopted the layer you both pointed at. Dropped the create-timeout widening entirely and switched to CreateSessionBody.initial_message — the message now rides in the POST /sessions body and the server's deferred-init path owns delivery + resubmit, so cao launch stops being the last client not using it. The client-side /input POST, the pre-send readiness wait, and the settling sleep are gone; the create call is back on mcp_request_timeout; _create_session_timeout/_effective_init_timeout/_CREATE_OVERHEAD_MARGIN are deleted. Only one delivery ships, so no double-send.

@call-me-ram — the specific non-blocking items:

  • codex non-starvation tests moved into test_startup_handler_nonblocking.py (the shared parametrized module) rather than the bespoke helper.
  • reworded the "codex was the last provider" comment — kiro_cli/opencode_cli/cursor_cli still make loop-side backend calls.
  • codex init TimeoutError now interpolates the configured provider_init_timeout instead of the hard-coded "60 seconds".
  • kept the codex event-loop offload; confirmed it still guards (reverting codex.py to main fails 2 loop-starvation tests).

One deliberate scope call worth a look: delivery is deferred on the headless path only. Non-headless never delivered a message here, and deferring on the attach path would race the pre-attach readiness poll against the agent's first turn — so that's a separate attach-path change, not a flag flip. Pinned by test_launch_non_headless_does_not_defer_init.

294 passed / 3 skipped on the touched suites; full-suite failures are the pre-existing ag-ui/otel missing-dep set, identical to main. Re-review when you have a moment.

gutosantos82 added a commit to gutosantos82/cli-agent-orchestrator that referenced this pull request Aug 19, 2026
awslabs#567, awslabs#566 and awslabs#564 were named as needing review by the gate on every scheduled
run for two days (2026-08-17..19) and never once launched. Nothing looked broken:
the driver truthfully reported "Launched 0 review session(s)" and "All reviews
complete", because from its own point of view every PR it considered was already
reviewed at its current head.

Cause: discovery took `gh pr list`'s newest-first order and truncated to LIMIT
immediately, before checking which PRs actually needed work. ~20 newer PRs already
had current-head reports, so the window filled entirely with no-ops and any PR
below the cut could never be reached — permanently, since the same ordering
repeats every run. Older PRs were structurally unreachable, not merely delayed.

Fix: fetch all open non-draft PRs, partition into needs-review (no report at the
current head) and already-reviewed, order needs-review first, and only then apply
LIMIT. Newest-first is preserved within each group, so new PRs still get fast
feedback; the cap now only ever drops work that is already done. When more PRs
need review than LIMIT allows, log how many are deferred and state explicitly that
none is starved.

Verified against the live queue: the four PRs needing review (awslabs#567 awslabs#566 awslabs#564 awslabs#498)
now sort ahead of the 31 already-reviewed ones, where previously they fell outside
the first 20 entirely.

@call-me-ram call-me-ram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. This round did the thing rather than resizing the thing, and I verified each piece by execution rather than by reading.

The layer change is exactly right. launch.py:332-346 hands MESSAGE to the server in the POST /sessions body, the client-side /input POST and its pre-send IDLE gate are gone, and the create call is back on mcp_request_timeout — correct now that defer_init makes create return before init starts (session_service.py:98). The unrecoverable window from round 1 is closed by construction: there is no second request left to drop, and the server path confirms the worker started and resubmits (terminal_service.py:882-978). I mutation-tested the guard by forcing server_delivers_message = False — five of the new tests fail, including the one-POST assertion and the body-not-query-string pin, so the regression coverage is real, not decorative. The per-provider budget table from round 1 is moot for the same structural reason: no client-side number gates delivery anymore, which also resolves haofeif's P1. The one remaining client wait, poll_until_done(timeout=120+300) at launch.py:398, fails loud and late rather than silently, and I confirmed poll_until_done treats a deferred terminal's UNKNOWN correctly (utils/terminal.py:284-294 — never counted as "started", so no early empty-output return).

The non-blocking items all landed too. The codex tests moved into test_startup_handler_nonblocking.py and picked up the cancel→await→suppress pattern, which fixes Copilot's pending-task point for free; the max-gap probe is a genuinely better instrument than a tick count for a coroutine with its own asyncio.sleep. Reverting codex.py to the merge base fails four tests — both non-starvation probes and both configured-timeout guards — so all of the new codex behavior is pinned. The "codex was last" claim is reworded accurately in both the source comment and the test-module header, the self-defeating settings guard went away with the deleted helpers, and the TimeoutError at codex.py:847 now reports the number the code actually used. I also checked that the settings-sourced trust-prompt timeout can't slow a clean start (early exit at codex.py:750-752) and can't KeyError (both keys are in _SERVER_DEFAULTS, settings_service.py:170-171).

The headless-only scope call is right, and I'm glad it's pinned by test_launch_non_headless_does_not_defer_init with the race spelled out rather than left implicit.

Two things before merge, neither a code change to this diff:

  1. Rebase — #625 (MiniMax provider) landed after your 2026-08-18 rebase and test/cli/commands/test_launch.py now conflicts. git merge-tree shows it's the only conflicting file.
  2. Not blockers, for the record as follow-ups: --async init failure is now log-only for a CLI launch (no caller inbox; terminal_service.py:712-716) where the old flow at least raised "did not become ready" — worth a later "check on it with cao session status" hint in the async echo. The no-message create still runs init inline against the 30s timeout, same as main and same family as the app_tools.py:404 hole from round 1; and a message passed without --headless is still ignored without a word — now a deliberate, tested choice, but a one-line warning would cost nothing.

Ran the three touched suites in a clean worktree: 264 passed, 3 skipped. CI is green on b5ac06f6 across all 23 checks.

@call-me-ram

Copy link
Copy Markdown
Collaborator

@tedswinyar friendly nudge: this is approved on my side (08-21) but went CONFLICTING after #625 (MiniMax provider) merged — it should be the one-file rebase we anticipated. Once you push the rebase this is merge-ready from my perspective; @haofeif's earlier CHANGES_REQUESTED predates your b5ac06f rework (the server-side initial_message/defer_init rebuild resolved their P1 about the 170s budget, which is now moot), so it would be worth their re-look at the same time.

@tedswinyar

Copy link
Copy Markdown
Member Author

Rebased onto current main (5ef6d665) — thanks for the nudge @call-me-ram, and sorry for the lag, I was out.

The rebase. It was the one-file rebase we anticipated. The only conflict was in test/cli/commands/test_launch.py, where #625 appended test_minimax_code_requires_workspace_access_confirmation at the same end-of-file spot as this PR's new delivery block; both are kept, and I verified no upstream line was dropped and no intentional change from b5ac06f6 was lost. launch.py also absorbed #666's --resume-session-id — the two compose correctly rather than one defeating the other: resume rides the query params and is baked into the provider command by create_provider before the defer_init branch, while initial_message rides the body and is delivered after initialize() returns. So --resume-session-id X "MESSAGE" is resume-then-deliver, and a non-claude_code provider still fails loudly as an HTTP 400 rather than becoming a swallowed background failure.

Full suite green locally on 3.11 (8435 passed / 20 skipped / 1 xfailed), black --check and isort --check-only clean.

One new commit since your approval, and it is a defect this PR introduced929a308f. Round 2 replaced _handle_trust_prompt(timeout=20.0) with timeout=float(get_server_settings()["startup_prompt_handler_timeout"]), which was the right instinct (that 20.0 could not be widened without a code change) but the wrong plumbing: it passes the value as the loop's fixed total budget, whereas the setting documents itself as an idle gap between consecutive prompts, reset each time one is answered, with total time bounded by provider_init_timeout. kimi_cli/antigravity_cli already implement those semantics.

It matters because it fails in this PR's own bug class, through a documented knob. Lower the gap to make kimi settle faster — say to the 5 that appears in get_server_settings()'s own docstring example — and codex's whole handler is capped at 5s. A cold start rendering its trust dialog at 6s is then never dismissed; initialize() accepts WAITING_USER_ANSWER as success (the first-run login menu is legitimately that), so it returns True with the dialog still up, codex blocks orchestrated input while waiting, send_input raises TerminalInputBlockedError, and MESSAGE is never delivered. The fix splits the two bounds, resets the gap on each answered prompt, and moves the loop to time.monotonic(). Defaults are unchanged — the gap default of 20 equals the old hard-coded 20.0 — so only tuned configurations change, and only toward honoring what the setting says. Mutation-tested: removing the resets, using the gap as the outer cap, and counting the gap before any prompt is handled each fail the new tests.

Two gaps this PR newly exposes cao launch to, which I do not think it should merge silently. Neither is introduced by this diff — both live in _schedule_deferred_init, built for the MCP assign/handoff flow — but routing cao launch down that path is what makes them reachable from the CLI, so they belong in this conversation. @call-me-ram you already flagged the first half yourself as a non-blocking follow-up; having now traced it, I would rather not leave it implicit:

  1. --async can report success for work that never ran. The CLI prints "Message accepted" and exits 0 before delivery. If deferred init then fails, _notify_caller_of_deferred_failure notifies the caller's inbox only if caller_id: — and cao launch sets no caller_id, so the else branch logs "failure is log-only" and the worker is deleted. Net: exit 0 plus a success message, with the real reason only in the server log. Pre-change that same failure was exit 1 carrying the actual cause.
  2. The non-async wait can declare done before delivery. poll_until_done's gate is "observed working once, then N stable idles", and utils/terminal.py flips observed_working on WAITING_USER_ANSWER as well as PROCESSING. Pre-change the poll only started after a 200-confirmed delivery, so that evidence was causally downstream of the message; now it starts concurrent with init, so startup activity can satisfy it — and kiro_cli, the default provider, publishes WAITING_USER_ANSWER for its --trust-all-tools consent dialog on essentially every init. A brief idle window before the paste lands can then satisfy the idle streak and return DONE with empty output. create_terminal is deliberately careful to report UNKNOWN rather than IDLE for exactly this reason, and _schedule_deferred_init drops that guard the moment initialize() returns, before send_input.

I think both are server-side fixes rather than CLI ones — hold the terminal non-ready until the send is issued (status_monitor.notify_input_sent(..., assume_processing=True) already exists), and give the deferred path a way to surface failure to a caller that has no inbox. Happy to take that as a follow-up PR immediately after this one, or to fold it in here if you would rather they land together. I don't have a strong preference; I do think shipping (1) without at least an issue open against it would be a mistake.

@haofeif — when you have a moment, would you re-look? Your 08-07 review predates the b5ac06f rework, and I believe it implemented the second option you offered: delivery moved into CreateSessionBody.initial_message so /sessions uses deferred initialization, which means no client-side timeout budget gates delivery any more and the per-provider arithmetic in your P1 is moot by construction rather than by re-tuning. Flagging the new 929a308f commit for you too, since it postdates both reviews.

@haofeif haofeif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 929a308f after the rebase and new Codex idle-gap commit. The server-side initial-message delivery change remains sound, but the new commit introduces one default-path P1 for first-run interactive Codex launches.

# 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.
await self._handle_trust_prompt(outer_timeout=float(init_timeout))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Return when the first-run login menu is ready, before synchronous launch times out

With defaults this now gives _handle_trust_prompt the 60s provider_init_timeout. The Codex login menu is neither an auto-dismissed trust/update prompt nor an idle composer, so any_prompt_handled stays false and the 20s idle gap never applies; the handler sits for the full 60s even though the next wait_until_status explicitly accepts that menu as WAITING_USER_ANSWER. A normal non-headless cao launch sends no initial_message, so /sessions initializes synchronously while the CLI request still has the default 30s mcp_request_timeout. The client therefore raises ReadTimeout before it can attach to the screen where the user must authenticate. I reproduced this at the exact head: the handler hit 60s with the real login fixture/status while the request budget remained 30s. Detect the login menu (or the accepted non-auto-dismissable WAITING_USER_ANSWER state) inside the handler and return so the following status wait can accept it.

@tedswinyar

Copy link
Copy Markdown
Member Author

Confirmed and fixed — and your reproduction was exact, including the detail that makes this a default-path P1 rather than a slow path: no dismissal branch fires on a login menu, so any_prompt_handled stays false, the 20s idle-gap exit is unreachable, and the handler runs all the way to its outer cap. That cap is provider_init_timeout (60s), against an mcp_request_timeout of exactly half.

The fix. One branch in _handle_trust_prompt's poll loop, ahead of the idle-composer exit: return when LOGIN_MENU_PATTERN and LOGIN_MENU_FOOTER are both in bottom_region and no dismissable dialog is up. It returns on the first frame and deliberately answers nothing — a blind Enter would pick option 1 on the operator's behalf. As you say, the state was already understood everywhere else: get_status classifies it WAITING_USER_ANSWER and initialize()'s next wait_until_status accepts it, both pinned by pre-existing tests. Only the handler had no branch for it.

On the parenthetical. Your main clause is what shipped. I didn't take the broader form — "the accepted non-auto-dismissable WAITING_USER_ANSWER state" — because that qualifier is the hard part: get_status can't distinguish such a state from a trust dialog that has been answered but is still rendered. The v1 trust check matches over the whole detector input, while trust-v2, update and login are bottom-anchored, so a dismissed v1 prompt keeps returning WAITING_USER_ANSWER at any distance (I measured 0 through 150 rows) where the v2 variant flips to IDLE past the 15-line window. A status-driven exit would therefore fire on the first frame after Enter, and a late update dialog would never be dismissed — the failure this PR's idle-gap split exists to prevent.

There's a client-side half you couldn't see from the traceback. Returning early makes POST /sessions respond in seconds, which means the CLI now reaches the pre-attach readiness poll in launch.py — code the pre-fix path never got to, because the client had already aborted. That poll accepted only {IDLE, COMPLETED}, and a login menu never becomes IDLE on its own, so it burned the full 120s _READINESS_WAIT_TIMEOUT and then printed

Warning: <id> did not reach idle within 120s — attaching anyway; input may be unreliable until init completes.

about a pane where initialization had finished and the only outstanding thing was the sign-in — which nothing told the operator to do, since the handler's explanation goes to the server log. So the handler fix alone traded a 30s wrong error for a 120s wrong warning. WAITING_USER_ANSWER now counts as settled there, and the CLI says so instead of blaming init. That's safe because non-headless POST /sessions initializes synchronously, so every provider's startup handler has already returned by the time the poll runs; the code records the ordering constraint in case that ever changes, since a deferred init would race the handler and resize the pty mid-init (#220).

Hardening the branch took three iterations, two of which reintroduced this same P1 in narrower frame classes. The branch claimed the handler must not answer the menu, but the v1 trust check matches the whole capture, so scrollback trust copy could make it press Enter into a live menu and then log that it was leaving the menu for the operator. Suppressing on has_login live-locked a genuinely stacked dialog; anchoring by presence only shrank the window. What separates a live dialog from stale copy is position — Codex draws the active modal last, so v1 copy counts as live only when it appears below the menu text, and has_dialog shares that same predicate so every term in it has a dismissal arm that can actually fire. The commit messages carry the full derivation and the frames that drove each step.

Verification.

  • Live codex-cli 0.151.0 first-run menu through the real tmux backend, not just fixtures — an isolated empty CODEX_HOME is what actually renders the menu inside the detection window. Driving the real handler against that pane: 0.07s, no keystroke, no error, and the pane's sha256 unchanged before and after. Running the parent commit against the same live pane, at the same 10s cap: 11.15s and the "no prompt or welcome banner detected" error, about a screen that plainly showed one.
  • Five frame classes all behave: live dialog below the menu → dismissed, then the login exit; stale copy above the menu, copy in scrollback only, and the pure menu → immediate login exit, no keystroke; live v1 dialog with no menu → dismissed, unchanged from before this PR.
  • Every guard is mutation-checked. I'll skip a tally, since a second pass built a different mutant set and got a different total; two in mine survive, and I'd rather name them than let that go unsaid: >>= is provably equivalent (the two patterns start with different characters, so a v1 and a menu match can never share an offset), and sourcing either match from clean_output instead of bottom_region changes no outcome in any frame I could construct, so it's constrained by a comment rather than a test.
  • 426 passed / 3 skipped across the four affected suites; black/isort clean; PYTHONPATH=src mypy on the two changed files reports Success: no issues found in 2 source files, identical at the parent. Two notes so no number ambushes you: a repo-wide run reports 98 errors in 11 files, none of them files this diff modifies; and some invocations surface pre-existing errors in transitively-imported modules, whose set is also identical at the parent.

Filed separately rather than grown into this PR — happy to pull any of them in if you'd rather see it fixed at the root:

  • initialize() gives wait_for_shell, _handle_trust_prompt and its final wait_until_status a full provider_init_timeout each, so a successful path can take ~3× that inside one synchronous request. That's the general form of the budget mismatch you're pointing at, and it means any future unrecognised settled screen reproduces this P1's shape. kiro_cli.py:448-451 already solves it the right way.
  • That same whole-capture v1 match may let a settled pane read as WAITING_USER_ANSWER after a trust prompt is auto-accepted, which could refuse an initial_message delivery. The misclassification is measured; the delivery refusal I couldn't prove end to end, so it's filed with the mechanism rather than asserted as a live defect.
  • An exhausted mock side_effect hangs pytest instead of failing it — StopIteration can't cross asyncio.to_thread — and there's no global test timeout, so a regression can present as a stopped suite. The tests here avoid it; making that suite-wide is the follow-up.

Thanks for catching this — it was in the commit meant to fix a delivery race.

@haofeif haofeif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Codex first-run login-menu stall from the previous review is fixed: the handler now returns without answering the menu, initialization accepts the settled prompt, and non-headless launch attaches without the extra 120-second wait. One blocking race remains in the synchronous headless path introduced by server-side deferred delivery.

# both. A deferred terminal reports UNKNOWN until init finishes,
# which ``poll_until_done`` deliberately does not count as "started"
# — it returns only once the agent has been observed working.
poll_until_done(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Wait for post-delivery activity, not provider startup

This poll starts as soon as the deferred POST /sessions returns, so its observed_working evidence is not causally downstream of the initial-message send. Provider startup itself can report WAITING_USER_ANSWER (the default Kiro consent dialog is one example), which makes poll_until_done() set observed_working=True; once initialization reaches IDLE, _schedule_deferred_init() still has to resolve metadata/memory and enter send_input(). During that gap three IDLE samples make this call return, after which launch reads empty output and exits 0 even though the task has not been dispatched. A curated memory lookup can widen that pre-dispatch gap to roughly 15 seconds. I reproduced the real deferred scheduler plus this poll with one valid startup sequence: the CLI observed waiting_user_answer followed by three idle samples and returned while the patched send_input was still in pre-dispatch work (message_dispatched was false). Keep the terminal non-completable until the initial send is issued, or expose a server-side delivery generation/acknowledgement and begin completion tracking from that boundary; the regression test should assert the synchronous command cannot return before dispatch.

tedswinyar added a commit to tedswinyar/cli-agent-orchestrator that referenced this pull request Aug 31, 2026
…s issued

haofeif's P1 on awslabs#566. launch's poll_until_done starts the moment the deferred
POST /sessions returns, so its observed_working evidence is not causally
downstream of the send it is waiting on. Provider startup alone reports
WAITING_USER_ANSWER (kiro-cli's consent dialog), which flips the flag; once
initialize() returns the pane reads a genuine IDLE while _schedule_deferred_init
still resolves shell_baseline and metadata and send_input runs
inject_memory_context. Three IDLE samples inside that window and the CLI reads
empty output and exits 0 with the task never dispatched.

Track the accepted-but-undispatched state on the server and mask IDLE/COMPLETED
as UNKNOWN while it holds, at the points where a status crosses the API boundary
(get_terminal, list_siblings, the session listing). Pollers already treat UNKNOWN
as neither progress nor completion, so no number of pre-dispatch samples can
satisfy an idle gate -- and no client has to remember to wait for a separate
handshake. launch.py is unchanged.

The mark is released at the dispatch boundary rather than after delivery
confirmation, so a task that completes during the confirm/resubmit window stays
visible instead of stranding the caller until its timeout.

WAITING_USER_ANSWER, PROCESSING and ERROR are deliberately not masked, and
neither are the internal status_monitor callers that make delivery decisions
(send_input's guards, the deferred retry loop) -- they need the raw state.
@tedswinyar

Copy link
Copy Markdown
Member Author

Confirmed, and fixed. Your reproduction was right about the part that makes this a default-path bug rather than a corner case: the poll's observed_working evidence is not causally downstream of the send, so provider startup can satisfy it on its own. I reproduced it before changing anything — a test driving the real _schedule_deferred_init, the real poll_until_done, and the real get_terminal reporting path against your startup sequence returns with dispatched still false, which is the synchronous cao launch printing empty output and exiting 0.

Two details I confirmed while tracing it, both of which shaped the fix:

  • The pre-dispatch window extends into send_input, exactly as your message_dispatched probe showed. inject_memory_context runs at terminal_service.py:1622, before the notify_input_sent arm at 1632 and send_keys at 1663. So the boundary that matters is the dispatch itself, not send_input's entry — anything keyed to "we called send_input" would still be wrong.
  • Three IDLE samples is ~3s at the default 1.0s interval, so any pre-dispatch window longer than that is enough. The curated-memory lookup you point at is the widest contributor, but it is not required to trigger it.

The fix: make the terminal non-completable until the initial send is issued. I took that option rather than the delivery-generation handshake, because it puts the invariant at the source instead of asking each client to remember to wait for it — launch.py is unchanged.

  • _schedule_deferred_init marks the terminal as having an accepted-but-undispatched initial message, set before the task is scheduled (the point at which the message has been accepted and a caller is free to poll).
  • While that mark holds, reported_status() masks IDLE/COMPLETED as UNKNOWN where a status crosses the API boundary. Pollers already treat UNKNOWN as neither progress nor completion, so no number of pre-dispatch samples can satisfy an idle gate.
  • The mark is released at the dispatch boundary — in a finally around the send_input call — not after _confirm_worker_started_or_resubmit. Holding it across the confirm/resubmit window would trade this bug for its mirror image: a short task can finish inside that window, and a poller masked for all of it would sit through a completion that already happened. There is a separate test for that direction, and it is the one mutant that distinguishes the two placements.
  • finally rather than a plain sequence because a raising send_input (TerminalInputBlockedError, the pane parked on a prompt) must also stop masking — the terminal is then honestly WAITING_USER_ANSWER and the operator has to see it.

Deliberately not masked, since the blast radius is the part worth checking:

  • WAITING_USER_ANSWER, PROCESSING, ERROR. Masking WAITING_USER_ANSWER would hide the one state an operator must see to unblock the pane. Letting it through is harmless here: it still flips a poller's "has started" flag early, but with IDLE masked no idle streak can accumulate to act on it.
  • Every internal status_monitor.get_status() caller — send_input's own ERROR/WAITING_USER_ANSWER guards, the deferred path's retry loop, the inbox/flow/memory services. Those make delivery decisions and need the raw state; this is a reporting-layer concern only.

I applied it at all three outward reporting sites (get_terminal, list_siblings, and the session listing) rather than only the one the P1 lands on, so GET /sessions and GET /terminals/{id} can't disagree about whether a terminal is idle. The MCP servers read status over HTTP, so they inherit it.

This does change what an orchestrating client sees during assign/handoff, not just during cao launch: for the pre-dispatch window a supervisor polling check_worker_status or GET /sessions now reads UNKNOWN rather than IDLE. That is deliberate — it is the same unfounded "idle" the P1 is about, reaching a different caller — and UNKNOWN is already the status those callers get while a deferred terminal is initializing, so it is not a new state for them to handle. Flagging it explicitly because it is a wider blast radius than the reported symptom.

One thing I want to be explicit about rather than let it read as a bigger win than it is: services/agent_step.py has a structurally identical observed_working gate, and I checked whether this fixed that too. It does not need it — that wait is entered after send_input has returned (agent_step.py:216-219), so it never races a deferred delivery. No change there.

Verification.

  • The regression test asserts the synchronous command cannot return before dispatch, as you asked. It fails at the current head with poll_until_done returned before the initial message was dispatched.
  • Four mutants, each killed by the test that claims it: mask removed from get_terminal, mask narrowed to COMPLETED only, and the mark never set — all three fail the ordering test; the mark held past dispatch fails only the confirm-window test, with the ordering test correctly still green.
  • Full CI suite at the fix versus the unmodified head in the same environment: failure sets identical, 61 each, zero new and zero fixed. Those 61 are all local environment — 58 need the agui extra I had not installed and 3 are telemetry — which is why CI here is green; the number I am relying on is the diff, not the absolute.
  • mypy clean on both changed files, black/isort clean, and a throwaway probe confirming the new module-level state leaks nothing across tests.

@tedswinyar
tedswinyar requested a review from haofeif August 31, 2026 21:51

@haofeif haofeif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at exact head dab082ec1a29af09691b5c59e43fe56276fc45f9. The pending-delivery mask fixes the originally reported pre-dispatch completion window, but releasing it as soon as send_input() returns exposes a second stale-IDLE window before post-dispatch processing is observed. Synchronous launch can therefore still report completion before the task starts producing output.

# send_input (TerminalInputBlockedError: the pane is parked on
# a prompt) must also stop masking: the terminal is then
# honestly WAITING_USER_ANSWER and the operator has to see it.
_clear_initial_delivery_pending(terminal_id)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Keep completion masked until post-dispatch activity is observed

This closes the pre-dispatch race, but clearing the mark when send_input() returns is still too early. A provider-startup WAITING_USER_ANSWER is intentionally unmasked, so poll_until_done() can already have observed_working=True; meanwhile send_input() only calls status_monitor.notify_input_sent(), which arms a future transition without changing the cached IDLE status (no current provider enables assume_processing_on_dispatch). If the provider emits no new output for the next three polls, this line exposes that stale IDLE and the CLI returns empty before the dispatched task ever begins processing. I reproduced the exact-head path with the real scheduler, status monitor, mask, and poller: after startup WAITING and masked pre-send IDLE, the mask cleared here and three IDLE samples returned DONE about 1.4 seconds before the first PROCESSING signal. Keep the outward state non-completable until a genuine post-dispatch transition/acknowledgement, rather than treating transport return as task activity.

@tedswinyar

Copy link
Copy Markdown
Member Author

Confirmed and fixed at 259fc6ac. Releasing the mark when send_input() returned moved the window rather than closing it.

I reproduced it against dab082ec first, with the real scheduler, the real _confirm_worker_started_or_resubmit and the real poll_until_done against a status fixture carrying the post-dispatch IDLE lag. poll_until_done returned 0.93s before the first PROCESSING signal appeared. Your ~1.4s on a live worker is the same window, mine just compressed.

The mechanism is what you described. send_input only calls notify_input_sent(), which arms the next transition without touching the cached status. I also checked assume_processing_on_dispatch: it returns False in providers/base.py:262 and no provider overrides it, so this affects every provider.

What changed

The early release is gone. The mark is released by _run's outer finally, reached after _confirm_worker_started_or_resubmit has observed a status in _DEFERRED_STARTED_STATUSES.

My reason for releasing at the dispatch boundary was wrong, and so was the test backing it. I argued that holding the mark across the confirm window would hide a genuine early completion. That test mocked _confirm_worker_started_or_resubmit as a flat 1.5s sleep returning True regardless of status, so holding the mark across it stranded the poller by construction. The real function starts with wait_until_status(_DEFERRED_STARTED_STATUSES, polling_interval=0.5), and that set contains COMPLETED, so it returns as soon as a completion is visible and the mark lifts with it. Replaced with a test that drives the real confirm loop through a turn going IDLE to COMPLETED without ever publishing PROCESSING.

Worst case

When no started status is ever observed, the mark is held for the confirm loop's whole budget: _DEFERRED_SUBMIT_CONFIRM_TIMEOUT 8.0s x (1 + 3 resubmits), so about 32s, where before it was dropped at dispatch. That sits inside cao launch's 420s headless budget and the worker is torn down at the end of it, so UNKNOWN looks right for a delivery we cannot confirm.

Cutting the timeout or the resubmit count would keep it under the default 30s mcp_request_timeout. I have not, because those constants tune delivery-retry reliability from #562/#496, and the 32s lands after the defer_init call has already returned. Two constants if you want it changed.

The create_task guard

_mark_initial_delivery_pending runs before loop.create_task(_run()), and only _run's finally releases it, so a create_task that raises would leave the mark set with nothing to clear it. Now guarded, and the orphaned coroutine is closed so it cannot warn "never awaited" on GC.

Two caveats on it. The trigger is not a closed loop: get_running_loop() only succeeds on the loop thread, so reaching create_task means we are the running loop, and closing a running loop raises. What reaches the guard is _run() not being a coroutine, MemoryError, or a KeyboardInterrupt in the gap. The last of those is why it catches BaseException, and it re-raises, so shutdown is unaffected.

It also does not cover loop.stop(): create_task then succeeds and the coroutine never runs, leaking the mark with nothing raised. Left alone, since that is teardown only and _pending_initial_delivery dies with the process. Worth a shutdown hook if you read it differently.

Tests

83 passed in test/services/test_terminal_service_full.py, and 241 across launch, deferred-submit, session_service and api_endpoints. Both new tests are mutation-tested: restoring the early release fails the stale-IDLE test, and removing the try/except fails the leak test. The coroutine close is verified by the warning reappearing when I remove it rather than by an assertion, since GC timing makes it non-deterministic to pin. The test/services/agui and test/api/test_agui_* failures in my local run are the ag-ui-protocol extra not being installed; they reproduce identically at the parent commit.

@tedswinyar
tedswinyar requested a review from haofeif September 1, 2026 23:50
@gutosantos82

Copy link
Copy Markdown
Contributor

PR Review: #566 — fix(launch): deliver the initial message via POST /sessions instead of a dropped second request

  • Author: tedswinyar — branch agent/caom-7it-fixmain
  • Head: 7b6c6bcff63d17f8044f4639c4624b3f96cc4999 · 8 files, +2407/−90 · MERGEABLE, merge state BLOCKED (by review decision, not CI) · reviewDecision=CHANGES_REQUESTED · all 23 CI checks pass
  • Round: 8th local review. Delta since our last report (head b8a6d693) is two commits / ~100 diff lines: 259fc6ac (close the orphaned coroutine on create_task failure; scope the pending-mark clear to initial_message) and 7b6c6bcf (black formatting, removing the exact violation we blocked on).

Summary

The delta resolves both grounds of our previous Request-changes verdict. (1) The black violation at test_terminal_service_full.py:2868-2870 is gone — black --check src/ test/ and isort --check-only pass at the CI-locked versions (black 26.3.1, isort 6.0.1), and all 23 GitHub checks are green at this head. (2) haofeif's round-5 stale-IDLE window is genuinely closed: the pending mark is set synchronously before create_task (so it exists before POST /sessions returns to a polling client), reported_status masks IDLE/COMPLETED→UNKNOWN while it is held, and it is released only in _run's outer finally after _confirm_worker_started_or_resubmit observes a post-dispatch _DEFERRED_STARTED_STATUSES reading — evidence causally downstream of the send, which was the round-5 complaint. The new create_task-raises guard is correct on all paths: coro.close() on a never-started coroutine cannot raise, and the conditional clear exactly mirrors the conditional set, so no mark can strand. Dynamic verification at this head: 397 passed / 3 skipped across the PR's named test files plus 83 passed in the delta file; a mutation check confirmed the production guard suppresses the never-awaited warning. Security posture is unchanged-positive. What keeps this open is process, not code: haofeif's CHANGES_REQUESTED still stands, un-re-reviewed, and call-me-ram's 08-21 approval is stale (predates rounds 3–5). Do not approve or advance this PR — the standing human block is haofeif's to clear.

Blocking (process, not code — not ours to clear)

  • haofeif's round-5 CHANGES_REQUESTED is standing and unconfirmed at this head. The fix (259fc6ac) landed ~21h after his review; the author replied with a reproduction-first account, and our own correctness trace and dynamic run confirm the mechanism — but only haofeif's re-review (or a maintainer dismissal) clears reviewDecision=CHANGES_REQUESTED. Any approve/merge-ready verdict from us would preempt a human decision. Correct posture: comment, and note the fix awaits his re-review.
  • Note: call-me-ram's APPROVED (08-21, at b5ac06f6) predates the codex idle-gap, login-menu, and mask-lifecycle work; treat it as stale rather than current sign-off.

Important (should fix)

  • [tests] The leak-guard test does not pin the delta's headline change — coro.close() is mutation-survivable. (test/services/test_terminal_service_full.py:2889 vs src/cli_agent_orchestrator/services/terminal_service.py:1565) Proven empirically by two independent reviewers: with the production coro.close() removed, TestPendingMarkNeverLeaks stays green under both the repo's real config and -W error::RuntimeWarning — the never-awaited warning fires at GC in the unraisable hook, which pytest surfaces only as a non-failing PytestUnraisableExceptionWarning. The test's sole assertion (mark not leaked) is satisfied by the independent _clear_initial_delivery_pending statement, so commit 259fc6ac's stated intent ("the production guard is what has to close it") is not achieved by the test. Fix: force gc.collect() and assert via pytest.warns/recwarn, or a test-scoped filterwarnings making PytestUnraisableExceptionWarning an error. (The mark-leak invariant itself — the round-5 substance — IS pinned and passes.)
  • [tests, carried 2nd round] get_session/list_siblings reported_status masking wiring is still regression-unguarded. (session_service.py:222, terminal_service.py:1677) TestReportedStatusMasking pins the function in isolation only; the existing get_session enrichment test never sets a pending mark, and the list_siblings tests use empty sibling lists, so reverting either wiring site fails no test. One test per site setting _pending_initial_delivery and asserting the masked status through the public function would close this.
  • [consistency, carried 2nd round] PR body still under-describes the codex.py scope. Re-fetched at this head: the body covers the offload/test-move/reword/interpolation but still omits (a) the redefinition of startup_prompt_handler_timeout from total budget to idle gap (idle_gap/outer_timeout split in _handle_trust_prompt) and (b) the first-run login-menu liveness subsystem (LOGIN_MENU_PATTERN, has_login/v1_is_live, the settled-menu exit) with its ~500 lines of tests. A reviewer working from the description cannot anticipate the two largest codex changes.

Minor / Nits

  • [consistency] PR-body test tally is stale: "294 passed, 3 skipped" — the same four files at this head yield 397 passed / 3 skipped (zero failures; the count grew with the new tests). Substance holds; numbers don't.
  • [consistency] Comment wording at terminal_service.py:1553: "KeyboardInterrupt is one of the two realistic triggers named above" — the block above names three items; the count reconciles only if the programming-error case is deemed unrealistic. Cosmetic.
  • [correctness] coro.close() runs before the mark clear; harmless (close on an un-started coroutine cannot raise), but clearing first would make the no-strand property independent of that reasoning.
  • [conversation] Codecov: 1 uncovered line at this head — the new create_task BaseException guard, consistent with the mutation-survivability finding above.
  • Carried nits from the previous report, unchanged by this delta: _READINESS_WAIT_TIMEOUT=120 silently couples to default provider_init_timeout; masking broadens transient UNKNOWN to session/sibling listings; the codex outer-cap log interpolation isn't asserted.
  • [conversation] Four author-acknowledged deferred follow-ups live in the thread (async log-only failure with no caller inbox; initialize() ~3× provider_init_timeout general budget; whole-capture v1 trust-match misclassification; exhausted-mock hang under asyncio.to_thread) — flagged so they aren't lost; none block merge.

Tests

Strong overall (see the previous report for the full inventory, all still present and passing). This round's additions: the RefusingLoop fake no longer closes the coroutine itself — right idea (the production guard should be what's exercised) but, per the Important above, the test still cannot go red if the production coro.close() is deleted. The two carried coverage gaps (masking wiring at get_session/list_siblings) remain the only untested wiring in the change.

Verification (dynamic, at this head, in an isolated worktree)

  • PR's named test files: 397 passed, 3 skipped (isolated HOME, -p no:libtmux); delta file test_terminal_service_full.py: 83 passed. Zero failures attributable to the PR (a sandbox-permission trio in TestSessionOwnershipIntegration fails identically on main in one reviewer's environment; the clean-room run had none).
  • black --check src/ test/: clean at latest and CI-locked 26.3.1 ("601 files would be left unchanged"). isort --check-only: clean at CI-locked 6.0.1 (latest-isort flags two PR-untouched files identically at merge-base — version drift, not a regression).
  • Mutation check: with coro.close() removed, the never-awaited RuntimeWarning appears in output but no test fails (evidence for the Important); mutation reverted, tree verified clean at 7b6c6bcf, test re-passes with zero warnings.
  • The :415/:796 line references in the new BaseException comment were verified exact at this head.
  • GitHub: all 23 checks pass; MERGEABLE; merge state BLOCKED solely by the review decision.

@haofeif haofeif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 7b6c6bcf. Holding the pending-delivery mask through confirmation fixes the stale post-dispatch IDLE case, and the create-task rollback paths are sound. The confirmation still accepts a COMPLETED value that was already cached before dispatch, however, so the mask can be released without any newer activity. I reproduced _confirm_worker_started_or_resubmit returning success in 0.2 ms from a pre-existing COMPLETED status; the full headless path then allows synchronous polling to finish on the old response.

# terminal is then honestly WAITING_USER_ANSWER and the operator has
# to see it), the worker never starting after all resubmits, or the
# loop being torn down. Idempotent, so double-clearing is harmless.
_clear_initial_delivery_pending(terminal_id)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Require a post-dispatch transition before clearing this mask

_confirm_worker_started_or_resubmit() checks only whether the current cached value belongs to {PROCESSING, COMPLETED, WAITING_USER_ANSWER}; it does not establish that the value changed after this dispatch. A resumed Claude conversation can already be COMPLETED from its previous response when initialization finishes. send_input() merely arms future detection, so the first confirmation poll immediately accepts that stale value and reaches this finally; poll_until_done() can then return the old response before the new task emits anything. I reproduced the helper returning True in 0.0002 s with a pre-dispatch cached COMPLETED, and the end-to-end probe returned before delayed post-dispatch activity. Snapshot a generation/event boundary before sending and release only after a strictly newer transition; removing COMPLETED is not sufficient because legitimate fast turns may go directly there.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reconfirmed at rewritten exact head eade1338: cached pre-dispatch COMPLETED still satisfies confirmation immediately, so this remains unresolved. Updated exact-head review: pullrequestreview-5097575559.

`cao launch MESSAGE --headless` created the session and then issued a
SEPARATE `POST /terminals/{id}/input`. Because `POST /sessions` ran the
provider's full `initialize()` inline, a slow cold start outlived the
client's read timeout: `requests` raised `ReadTimeout`, launch reported
"Failed to connect to cao-server", and the second request never
happened. MESSAGE was silently dropped even though the session, the
terminal and a healthy idle TUI all existed server-side, and nothing
retried because from the server's point of view the launch had
succeeded.

Widening the client's create timeout was the first attempt and was the
wrong fix: it only helps providers whose init happens to fit the new
budget, and it raises the connect timeout along with the read timeout.

Instead, pass `initial_message` in the `POST /sessions` body. That puts
the initial terminal on the deferred-init path that already exists
(`session_service.create_session` -> `create_terminal(defer_init=True)`),
so the server responds as soon as the terminal record exists and then
owns init, delivery, and re-submission if the TUI swallowed the send.
`mcp_server` and `ops_mcp_server` already do this; `cao launch` was the
last client doing its own create-then-send. The client-side `/input`
POST, its pre-send readiness gate and the settling `sleep(3)` are all
gone — shipping both deliveries would double-submit.

Deferred terminals report UNKNOWN until init finishes, which
`poll_until_done` deliberately refuses to count as "started", so the
non-async path can wait on it directly. Its budget grows by
`_READINESS_WAIT_TIMEOUT` because init now runs inside that window
rather than in a separate poll beforehand. `--async` reports that the
server will deliver the message and returns without waiting for a
status the deferred path never reports. `initial_message` is sent on the
headless path only: a non-headless launch attaches instead and has
never delivered MESSAGE, and deferring init there would make the
pre-attach readiness poll race the agent's first turn.

Also offload codex's blocking backend calls (`get_history`, `send_keys`,
`send_special_key`, `get_pane_current_command`) via `asyncio.to_thread`,
the same event-loop-starvation fix awslabs#451 applied to claude_code and awslabs#494
to kimi/antigravity/copilot. codex is not the last such gap — kiro_cli,
opencode_cli and cursor_cli still make loop-side calls in
`initialize()` — but it has the slowest init, so a concurrent fan-out of
codex launches is where the self-inflicted queueing showed up. Its
regression coverage lives in the shared
`test_startup_handler_nonblocking.py`, which already parametrizes this
property; `initialize()` needs a longest-gap probe rather than the tick
count because its own `asyncio.sleep` warm-up ticks the ticker either
way. The init `TimeoutError` now interpolates `provider_init_timeout`
instead of hard-coding "60 seconds".
…a total budget

The previous commit wired ``startup_prompt_handler_timeout`` into
``_handle_trust_prompt`` to replace a hard-coded 20.0, so an operator on a
slow or containerized host could widen it without a code change. But it
passed the value as the loop's FIXED TOTAL budget, which contradicts what
that setting documents itself to be (settings_service.py): an IDLE GAP
between consecutive startup prompts, reset each time one is answered, with
total time bounded by ``provider_init_timeout``. kimi_cli and
antigravity_cli already implement exactly those semantics.

The mismatch is reachable through a documented knob and fails in the bug
class this PR exists to close. An operator who lowers the gap to make kimi
settle faster -- e.g. to the 5 that appears in get_server_settings()'s own
docstring example -- silently caps codex's whole handler at 5s total. A
cold start that renders its trust dialog at 6s is then never dismissed;
``initialize()`` accepts WAITING_USER_ANSWER as success (the first-run login
menu is a legitimate state there), so it returns True with the dialog still
up, codex blocks orchestrated input while waiting on a prompt, ``send_input``
raises ``TerminalInputBlockedError``, and the initial message is never
delivered.

- ``_handle_trust_prompt(idle_gap=None, outer_timeout=None)``: the gap
  defaults to ``startup_prompt_handler_timeout`` and only starts counting
  once a prompt has actually been handled; each of the three dismissal
  branches resets it. The outer cap defaults to ``provider_init_timeout``
  and is passed explicitly from ``initialize()``.
- Switched the loop clock from ``time.time()`` to ``time.monotonic()`` so a
  wall-clock adjustment mid-init cannot shorten or extend either bound,
  matching kimi_cli.
- The give-up log now names the cap it actually hit.

Defaults are unchanged (the gap default of 20 equals the old hard-coded
20.0), so out-of-the-box behavior is identical; only tuned configurations
change, and only toward honoring what the setting says.

Tests: three guards for the gap semantics (a dialog later than the gap is
still dismissed; answering one prompt resets the gap so a later update
dialog is still caught; a quiet startup returns on the gap rather than
sitting until the outer cap) plus one pinning that the two bounds come from
their own separate settings. Mutation-tested: removing the resets, using the
gap as the outer cap, and counting the gap before any prompt is handled each
fail these tests.

Test-harness note: the clock is patched by replacing the module reference
``providers.codex.time``, not the ``time.monotonic`` attribute. ``codex.time``
IS the shared stdlib module, so patching the attribute mutates it
process-wide and asyncio's event loop -- which calls ``time.monotonic()`` on
every step -- consumes the ``side_effect`` sequence, raising StopIteration
inside the loop rather than in the code under test.
…rning the outer cap

haofeif's P1: with defaults, `_handle_trust_prompt` gets the 60s
`provider_init_timeout` as its outer cap. The Codex first-run login menu is
neither a dismissable dialog nor the idle composer, so no loop exit fired and
the handler sat for the whole cap -- then logged "no prompt or welcome banner
detected" about a screen that plainly showed one. Meanwhile a non-headless
`cao launch` sends no `initial_message`, so `POST /sessions` initializes
synchronously against the client's 30s `mcp_request_timeout`: the client raised
ReadTimeout at 30s, before the operator could attach to the screen where they
must authenticate.

The state was already understood everywhere else -- `get_status` classifies the
menu as WAITING_USER_ANSWER via LOGIN_MENU_PATTERN, and `initialize()`'s next
`wait_until_status` explicitly accepts it. Only the handler had no branch for
it. Reproduced at the exact head: 6s outer cap fully consumed on the real login
fixture, against a 3s client budget at the same 2:1 ratio; after the fix the
handler returns in 0.0s having sent nothing.

Deliberately narrow. A general "return whenever get_status is an accepted
state" would also fire on a trust or update dialog that had been answered but
was still on screen, and `initialize()` treats WAITING_USER_ANSWER as success --
so it would exit with a live dialog and the next `send_input` would raise
`TerminalInputBlockedError`, which is the failure this handler's idle-gap split
exists to prevent. The login exit is therefore gated on `not has_dialog`, and a
test pins that a trust dialog stacked over the menu is still dismissed first.

The handler sends NO keys for this state: picking a sign-in method on the
operator's behalf is not its call.
The companion to the previous commit's handler fix, and the half that the
operator actually sees.

Returning from `_handle_trust_prompt` on the login menu makes `POST /sessions`
return in seconds instead of dying at the client's `mcp_request_timeout`. That
newly *reaches* the pre-attach readiness poll below, which accepted only
{IDLE, COMPLETED}. A first-run login menu never becomes IDLE on its own, so the
poll burned its full 120s `_READINESS_WAIT_TIMEOUT` and then printed

    Warning: <id> did not reach idle within 120s — attaching anyway; input may
    be unreliable until init completes.

about a pane where init had in fact finished and the only outstanding thing was
the operator's sign-in. Nothing told them to sign in; the handler's explanation
goes to the server log, which a `cao launch` user is not reading. So the
previous commit traded a 30s wrong error for a 120s wrong warning -- an
improvement, since the session now survives and is reachable, but not a fix
from the seat that reported it.

WAITING_USER_ANSWER now counts as settled, and when the poll ends there the CLI
says so instead of blaming init.

Safe because non-headless `POST /sessions` initializes synchronously -- it sends
no `initial_message` (see `server_delivers_message`), so `defer_init` is False
and every provider's startup handler has already returned by the time this poll
runs. A WAITING_USER_ANSWER here is therefore a settled prompt, not a dialog
caught mid-dismissal. The comment records the ordering constraint this creates:
if non-headless init is ever deferred, this poll would race the startup handler
and attaching early would resize the pty mid-init, which is issue awslabs#220 again --
so that change must gate attach on init completion rather than reuse this set.

`_is_waiting_on_user` is a separate read because `wait_until_terminal_status`
reports only whether one of its targets was reached, not which one, and the set
now has three members. It swallows transport errors deliberately: it decides
only which advisory line to print, and it runs inside the command's
`RequestException` handler, so an escaping blip would report "Failed to connect
to cao-server" about a server the poll just finished talking to.

Verified: the widened set is pinned by its own assertion, so narrowing it back
fails `test_readiness_poll_accepts_waiting_user_answer` rather than showing up
only as a 120s stall no test waits for. 122 passed across the launch and
terminal-util suites; black/isort clean; mypy clean on launch.py (the parent was
clean too, so the `bool()` cast keeps it that way).
Closes a hole in the previous commit's own stated invariant, found in
adversarial review and reproduced against a live codex-cli 0.151.0 pane.

That commit's branch says the handler must not answer the login menu. It
doesn't enforce it. Alone among the four signatures this loop recognises, the
v1 trust check matches the WHOLE capture rather than `bottom_region`, so it can
fire on trust copy anywhere in scrollback -- text the login gate cannot see.
One prepended line is enough: the handler sends Enter and *then* logs that it
is leaving the menu for the operator, in that order.

The consequence is worse than the stall being fixed. 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}), which it can
then no longer satisfy: a recoverable "operator must authenticate" becomes
`TimeoutError("Codex initialization timed out after 60s")` and teardown.

Precision on the evidence: PROCESSING was measured after selecting the local
API-key option, and the OAuth option was deliberately not exercised to avoid
kicking off a browser flow. So the departure from the accepted set is measured;
the specific landing state for a blind Enter is inferred.

Gated rather than anchored. Bottom-anchoring the v1 search would change
trust-v1 detection for every codex launch, and a genuine trust prompt may
legitimately sit higher than the bottom 15 lines -- outside this PR's business.
Instead `bottom_region`/`has_login` are hoisted above the dismissal branches
(nothing between the old and new positions touches `clean_output`) and the v1
branch gains `and not has_login`, which reads as: if the bottom of the screen
is the login menu, whatever trust text is in the buffer is not the active
dialog, so Enter is wrong regardless of where the string came from.

Fail-safe direction: if a genuine v1 prompt and the login signatures ever share
the bottom 15 lines, the v1 auto-accept is suppressed AND `has_dialog` blocks
the login exit, so the handler stalls to its cap rather than sending a wrong
key. A stall is recoverable; a mis-selected sign-in method is not.

Reachability today is low, and the new test says so rather than overselling it:
Codex renders the login menu BEFORE the trust prompt, so no natural sequence
produces the contaminating line, and the pane's pre-codex content is just the
shell prompt and launch line (awslabs#540 moved developer_instructions to $(cat file),
so that text is no longer echoed). What remains is operator-supplied text --
a codexConfig string value, or an agent prompt quoting the trust copy. Mechanism
certain, trigger unlikely, one line to close.

The update-dialog arm is not exploitable the same way: `_has_update_dialog_in_bottom`
is bottom-anchored, verified by injecting the full update triple into scrollback.
Two gaps found by a second adversarial pass over the previous three commits.

1. `_is_waiting_on_user` aborted the launch on a 200 with non-object JSON.

`resp.json().get(...)` raises `AttributeError` when the body parses to a list,
string, number or null -- and `AttributeError` is no kind of `RequestException`,
so it escaped the local except, reached the command's generic handler, and exited
1 *after* `POST /sessions` had already created the session. Driven against a real
local HTTP server:

    200 + JSON object   -> True,  exit 0, attached
    200 + HTML          -> False, exit 0, attached   (JSONDecodeError IS a RequestException)
    200 + JSON array    -> AttributeError, exit 1, NEVER ATTACHED
    200 + null / string -> same

Leaving the session orphaned in tmux is the precise outcome that except clause
was added to prevent, so the docstring's "including for a non-JSON body" was
true of unparseable bodies and quietly wrong about parseable non-objects. Now
the shape is checked rather than assumed, and the docstring says which case each
mechanism covers.

2. `has_login`'s conjunction was unpinned in the direction that now matters.

The previous commit made `has_login` gate a *dismissal*, so a `has_login` that is
too EAGER suppresses a trust prompt that should be answered -- the mirror of the
scrollback bug it fixed. Four mutants survived the suite: footer-only,
menu-only, `and`->`or`, and matching the whole capture instead of `bottom_region`.

The footer-only mutant is the dangerous one, because `LOGIN_MENU_FOOTER` IS
`TRUST_PROMPT_FOOTER` -- so a footer-only `has_login` reads True on any frame
containing "Press enter to continue" and would suppress v1 dismissal there.

Why the suite missed it is subtler than "no test pairs them" -- two frames DO pair
the v1 copy with the shared footer. What no test did was require the *v1 arm* to
FIRE on such a frame. Where the two co-occurred, either the v2 signature
co-occurred as well, so the ungated v2 arm dismissed and every assertion passed
regardless; or the test asserted that nothing should be dismissed at all. One arm
covering for another is the pattern to watch for in this file.

Three tests close all four, each feeding a frame where a real trust prompt must
still be answered: footer without menu text, menu text without footer, and a
login menu present only in scrollback.

Not a field break in either case -- I could not confirm whether a real v1 dialog
carries that footer (0.151.0 renders the v2 wording, and reaching a trust prompt
needs authentication), so this pins the gap rather than fixing an observed
failure.
…ve-lock

The `not has_login` gate two commits back was too blunt in the other direction,
and the comment on the login exit was false because of it.

A GENUINE v1 trust dialog stacked over a live login menu satisfies both
predicates: `has_login` (menu text + footer in the bottom region) suppressed the
dismissal, and the same frame's v1 copy kept `has_dialog` true, which suppressed
the login exit. Neither branch could fire, so the handler live-locked to its
outer cap. Reproduced: 6.01s of a 6.0s cap, zero keystrokes, and the misleading
"no prompt or welcome banner detected" -- i.e. in that one frame class the fix
reintroduced the exact stall it exists to remove, and with it the original
non-headless ReadTimeout, since the handler's cap is twice the client's request
budget.

Meanwhile the login exit's comment asserted that a stacked dialog "must be
dismissed first". For the v1 signature nothing dismissed it. That is precisely
the class of confident-but-wrong mechanism comment this codebase keeps getting
burned by, so it is corrected rather than softened.

The distinction the gate actually wanted is live-vs-stale, and position is what
separates them, so the dismissal is now bottom-anchored: `v1_is_live or not
has_login`. A live v1 dialog is answered even with the menu visible (dismiss
first, then exit on the menu, which is what the comment claims); stale v1 copy in
scrollback is still ignored, keeping the earlier exploit closed.

Verified across all three frame classes:
    stacked live v1 over menu -> 2.30s, 1 key, dismiss THEN login exit, no error
    v1 copy in scrollback     -> 0.00s, 0 keys, login exit          (exploit still closed)
    pure login menu           -> 0.00s, 0 keys, login exit

The asymmetry that remains is deliberate and now documented: the dismissal is
bottom-anchored while `has_dialog` still consults the whole capture, so stale v1
copy delays the early return rather than provoking a keystroke. Slow beats
mis-answering a sign-in menu.

Also from the same audit:

- `test_is_waiting_on_user_swallows_transport_errors` was half vacuous. Its
  non-200 case left `json()` as an unconfigured MagicMock, and
  `isinstance(MagicMock(), dict)` is already False -- so the isinstance guard
  carried the assertion and deleting the `status_code == 200` check failed
  nothing. A 500 carrying a valid waiting body would have been believed. Split
  out and parametrized with a real waiting body across seven status codes.

- The stacked-dialog test asserted its precondition only in prose. An earlier
  version of it lost `has_login` to the 15-line window and passed regardless;
  the comment documented that trap while nothing defended against it. Now
  computed and asserted, so fixture drift fails loudly.

- The new tests fed `get_history` a bare `side_effect` list. Once exhausted that
  raises StopIteration, which `asyncio.to_thread` cannot deliver out of its
  future, so the test HANGS rather than fails -- demonstrated while
  mutation-testing this very commit: under `has_login = False` the stacked-dialog
  test stopped producing output instead of failing. A regression that stops the
  suite reads as infrastructure flake. They now use a `_frames()` helper that
  repeats the final frame, so an unexpected extra read looks like a pane that
  stopped changing: the handler hits its outer_timeout and the test fails on its
  own assertion, in bounded time.

Follow-up from a second reviewer on the same commit: anchoring to `bottom_region`
by PRESENCE only shrank the scrollback exploit from "anywhere in the 200-line
capture" to "the last 15 lines" -- it did not close it, because trust copy above
a live menu inside the window would still be answered. Liveness is decided by
POSITION instead: Codex draws the active modal last, so the v1 copy counts as
live only when it appears below the menu text. The shared footer is no help here
(`LOGIN_MENU_FOOTER is TRUST_PROMPT_FOOTER`, so it cannot attribute itself to
either block), which is why the comparison is against the menu pattern.

That makes the two stacked tests a matched pair differing only in block ORDER --
trust-below is dismissed, trust-above is not -- so what is pinned is the
comparison rather than mere containment.

Third correction, from a third reviewer: position-anchoring the DISMISSAL alone
left two notions of liveness in the loop, and they disagreed exactly where it
hurt. `has_dialog` still tested the v1 pattern by bare presence in
`bottom_region`, so stale trust copy ABOVE a live menu was correctly too stale to
dismiss yet still counted as a blocking dialog -- neither the dismissal nor the
login exit could fire, and the handler burned its whole cap. At the 60s default
against a 30s `mcp_request_timeout` that is this PR's own P1 again, in a
different frame ordering from the live-lock it replaced.

`has_dialog` now shares the `v1_is_live` predicate, so the loop's implicit
invariant holds: every term in `has_dialog` has a dismissal arm that can actually
fire on it. This only changes behaviour when a login menu is present -- without
one, `not has_login` makes `v1_is_live` true whenever the copy is in
`bottom_region`, which is the previous meaning exactly, so the `has_idle` exit
still treats a live trust dialog as blocking.

My test for that frame was itself the lesson: it asserted only that no keystroke
was sent, which passed while the handler burned the cap. It now also asserts the
handler returns promptly and logs no error -- the keystroke and the exit are two
different properties and this frame needs both.

Verified across all five frame classes (repeating frame, 4s gap / 8s cap):
    trust BELOW menu (live)      -> 1 key, dismissed, no error
    trust ABOVE menu (stale)     -> 0.00s, 0 keys, login exit
    trust in scrollback only     -> 0.00s, 0 keys, login exit
    pure login menu              -> 0.00s, 0 keys, login exit
    live v1 dialog, no menu      -> 1 key, dismissed  (unchanged)

Mutation testing the above turned up one more gap: removing the v1 term from
`has_dialog` ENTIRELY still passed all 55 tests. The v2 sibling test's fixture is
a v2 frame, so the v2 term was covering for the v1 one -- the same
one-arm-covers-for-the-other pattern found earlier in the dismissal. Without that
term the login exit can fire on a frame where a just-answered v1 dialog is still
rendered, which is the TerminalInputBlockedError window.

Added the v1 twin of the stacked test, which asserts there is no v2 signature in
its own fixture so the v2 term cannot cover for it again.

Comment correction from a fourth pass: two comments in this file claimed
`has_dialog` "consults the whole capture". It never did -- all three of its terms
are confined to `bottom_region`, including `_has_update_dialog_in_bottom`, which
takes the same 15-line slice internally. The loop's ONLY whole-capture match is
the dismissal's own trigger, which is the opposite side of the asymmetry from
where I had placed it.

The consequence I drew from it was inverted too. I wrote that stale v1 copy which
had scrolled away "delays this exit rather than skipping it". Measured, stale copy
is invisible either way -- scrolled out of the region, or still inside it above the
menu -- and the login exit fires at 0.00s with no keystroke in both cases. It was
NEARBY stale copy that used to burn the entire cap, and only until these two
predicates were unified.

Frame C, and a guard this commit had silently un-covered. Both found by a fourth
adversarial pass; both are defects introduced by the position comparison itself.

1. The two sides of the comparison were asymmetric: `_v1_match` took the FIRST v1
   occurrence while the menu index took the LAST. On a frame carrying stale v1 copy
   ABOVE the menu and a live v1 dialog BELOW it, the stale copy won the comparison,
   `v1_is_live` went false, and -- because `has_dialog` now shares that predicate --
   the login exit fired at 0.00s with zero keystrokes while a live dialog was on
   screen. `initialize()` then succeeds on WAITING_USER_ANSWER and the next
   `send_input` is refused with TerminalInputBlockedError and the message dropped:
   the exact failure `not has_dialog` exists to prevent, and worse than the stall it
   replaced. Both sides now take the last occurrence, since only the lowest render of
   each block can be the one currently drawn.

2. Making liveness positional un-covered commit 4's menu-only guard. That test's
   fixture had the v1 copy BELOW the menu line, so `v1_is_live` was true on position
   alone and the dismissal fired whether or not `has_login` still required the
   footer -- the mutant it existed to kill started surviving. The copy now sits above
   the menu, and the fixture asserts it carries the menu pattern WITHOUT the footer.

3. The menu side needed its own frame. I had claimed the stale-above/live-below
   test also killed `_menu_matches[-1]` -> `[0]`; mutation testing said otherwise --
   its last v1 copy sits below BOTH menu matches, so either index agrees. The false
   claim is out of the docstring and
   `test_v1_copy_between_two_menu_renders_is_not_live` covers it properly: menu, v1,
   menu, where the lowest block is the menu, so the copy between them is stale. That
   frame does discriminate the two indices.

Two latent weaknesses are documented rather than fixed, because neither is
reachable through an observable:
  - `>` vs `>=` is an equivalent mutant: the two patterns begin with different
    characters, so a v1 match and a menu match can never share a start offset.
  - Sourcing either match from `clean_output` instead of `bottom_region` compares
    offsets across two different strings. No frame I could construct makes that
    change an outcome, so the constraint is stated in a comment at the comparison
    instead of pinned by a test.

Docstring precision: the stale-above test claimed its only difference from the
live-below test was block order. The fixtures also differ by an inert
"Do you trust this workspace?" line, which matches no pattern in this module
(verified: not TRUST_PROMPT_PATTERN_V2, whose text is "Do you trust the contents
of this directory?"). Order is therefore the only difference any predicate can
see, which is the property the pair relies on -- now stated that way rather than
as a claim about the literal text.
…s issued

haofeif's P1 on awslabs#566. launch's poll_until_done starts the moment the deferred
POST /sessions returns, so its observed_working evidence is not causally
downstream of the send it is waiting on. Provider startup alone reports
WAITING_USER_ANSWER (kiro-cli's consent dialog), which flips the flag; once
initialize() returns the pane reads a genuine IDLE while _schedule_deferred_init
still resolves shell_baseline and metadata and send_input runs
inject_memory_context. Three IDLE samples inside that window and the CLI reads
empty output and exits 0 with the task never dispatched.

Track the accepted-but-undispatched state on the server and mask IDLE/COMPLETED
as UNKNOWN while it holds, at the points where a status crosses the API boundary
(get_terminal, list_siblings, the session listing). Pollers already treat UNKNOWN
as neither progress nor completion, so no number of pre-dispatch samples can
satisfy an idle gate -- and no client has to remember to wait for a separate
handshake. launch.py is unchanged.

The mark is released at the dispatch boundary rather than after delivery
confirmation, so a task that completes during the confirm/resubmit window stays
visible instead of stranding the caller until its timeout.

WAITING_USER_ANSWER, PROCESSING and ERROR are deliberately not masked, and
neither are the internal status_monitor callers that make delivery decisions
(send_input's guards, the deferred retry loop) -- they need the raw state.
… observed

Round-4 review (haofeif), P1. The previous revision released the
non-completable mark when send_input() returned, on the reasoning that a
keystroke had been dispatched so a poller's evidence was now causally
downstream of the send. It isn't.

send_input only calls status_monitor.notify_input_sent(), which ARMS the next
transition without changing the cached status, and no current provider enables
assume_processing_on_dispatch. So the status a poller reads immediately after
dispatch is still the pre-send IDLE, and stays that way until the agent's first
output chunk is detected. Releasing at the dispatch boundary moved the
false-completion window from before the send to after it rather than closing it.

Reproduced against the exact previous head with the real scheduler, the real
_confirm_worker_started_or_resubmit and the real poll_until_done:
poll_until_done returned 0.93s BEFORE the first post-dispatch PROCESSING signal
ever appeared, so the synchronous `cao launch` would still exit 0 with empty
output.

The mark is now released by _run's outer finally, reached only after the confirm
loop has observed a status in _DEFERRED_STARTED_STATUSES.

The concern that motivated the earlier release point -- that holding the mark
across the confirm window would hide a genuine early completion -- does not
hold, and its supporting test was a fixture artifact. That test mocked
_confirm_worker_started_or_resubmit as a flat 1.5s sleep returning True
regardless of status, so holding the mark across it necessarily stranded the
poller. The real function's first action is
wait_until_status(_DEFERRED_STARTED_STATUSES, polling_interval=0.5), and that
set contains COMPLETED -- it returns as soon as a completion is visible, and the
mark lifts with it. Replaced with a test that drives the REAL confirm loop
through a turn that goes IDLE -> COMPLETED without ever publishing PROCESSING,
asserting the poll still returns promptly.

The outer finally continues to cover every abnormal exit, so a terminal can
never be left permanently masked: initialize() raising, send_input raising
TerminalInputBlockedError, the worker never starting after all resubmits, or the
loop being torn down.

Tests: test/services/test_terminal_service_full.py 82 passed; launch,
deferred-submit, startup-handler and utils/terminal suites 172 passed. The 43
test/services/agui failures are pre-existing (ag-ui-protocol extra not
installed) and reproduce identically with this change stashed.
Found by adversarial review of the previous commit. Pre-existing since the mask
was introduced rather than new to it, but it is this mechanism's defect either
way, and it is cheap to close.

_mark_initial_delivery_pending is called before loop.create_task(_run()) — set
there deliberately, so the mark is established by the time _schedule_deferred_init
returns and the invariant does not depend on the loop winning a race to schedule
_run. But only _run's finally releases it, so if create_task itself raises the
mark is never cleared: create_task raises RuntimeError when the loop closed
between the get_running_loop() check above and that line (server teardown racing
a create_terminal), and create_terminal's own exception cleanup does not know
about this mark.

_pending_initial_delivery is module-level, so the consequence is not scoped to
the failed request: the terminal_id reports UNKNOWN for the life of the process,
for a terminal that is already gone.

Catches BaseException rather than Exception so KeyboardInterrupt/SystemExit
arriving between the mark and the task creation cannot leak it either, and
re-raises unchanged.

Test: test_create_task_raising_does_not_leak_the_mark drives the real
_schedule_deferred_init against a loop whose create_task raises, and asserts the
mark is clear afterwards. Mutation-tested — it fails with the try/except removed.

Tests: test_terminal_service_full.py 83 passed; launch + deferred-submit +
session_service + api_endpoints 241 passed.
The guard added in the previous commit is right; the reason I gave for it was
not, and it was the kind of claim a reviewer can check in one line.

I wrote that create_task "raises RuntimeError if the loop closed between
get_running_loop() above and this line". That cannot happen. get_running_loop()
only succeeds on the loop thread, so reaching create_task means we ARE the
running loop, and loop.close() on a running loop raises "Cannot close a running
event loop". Verified both directions rather than reasoned about them.

What actually reaches the guard is duller: _run() not being a coroutine
(programming error), MemoryError, or a KeyboardInterrupt landing in the gap. The
guard stays — it is two lines and it pins a real invariant — but it no longer
claims a trigger it cannot have.

Also recording the gap the guard does NOT close, since it is the reachable one:
after loop.stop(), create_task SUCCEEDS and the coroutine is never run, so the
mark leaks with nothing raised to catch. Left unguarded on purpose. That state
only arises while the loop is being torn down, and _pending_initial_delivery is
module-level state that dies with the process, so there is nothing for it to leak
into; guarding it would mean a shutdown hook for state that cannot outlive
shutdown. This also means the previous commit's "reports UNKNOWN for the life of
the process" overstated the consequence: every path here is teardown.

Test docstring and fixture renamed to match (ClosedLoop -> RefusingLoop): it
pins "any create_task failure clears the mark", not the closed-loop story.

Tests: test/services/test_terminal_service_full.py 83 passed.
Two refinements to the create_task guard, from review of it.

1. ``_run()`` was being constructed inline as the create_task argument, so when
   create_task raised, the coroutine object was left un-awaited and the
   interpreter warned "coroutine was never awaited" on GC. Build it first, close
   it in the handler. Verified by removing the close and watching the warning
   come back.

2. The handler cleared the pending mark unconditionally, including on the
   ``initial_message is None`` path where no mark was ever taken. Harmless today
   because terminal_id is generated per create_terminal, but it was reaching for
   state this call does not own. Now guarded by the same ``if initial_message``
   that set it.

Also pinning why the handler catches BaseException, since it reads as too broad
and a reviewer asked: KeyboardInterrupt is one of the two realistic triggers and
is not an Exception subclass, so narrowing to Exception would skip the case the
guard exists for. The bare ``raise`` means shutdown semantics are untouched --
this only cleans up on the way past -- and it matches the existing rollback
guards at :415 and :796.

The test's fake no longer closes the coroutine for the production code: a real
loop doesn't either, so having the fake do it hid the warning above. It now
passes under ``-W error::RuntimeWarning``, which it could not before.

Tests: 183 passed across test_terminal_service_full.py, test_launch.py and
test_deferred_submit_verification.py.
CI's Code Quality gate failed on black --check for
test/services/test_terminal_service_full.py: the two nested patch() context
managers want the parenthesised form, and one assert wants its message on the
same logical line. No behavioural change; 83 passed either way.

isort is clean and mypy reports nothing in terminal_service.py (its repo-wide
errors are pre-existing and the step is continue-on-error).

@haofeif haofeif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the rewritten exact head eade1338. The previously reported P1 remains: confirmation accepts a cached pre-dispatch COMPLETED without requiring a newer status generation, so the pending-delivery mask can clear before the new task emits activity. An exact-head probe returned success in 0.008s with the stale status unchanged. The focused existing launch/deferred-delivery suite passes (100 tests) but does not seed a pre-existing completion.

… status

Round-6 review (haofeif), P1, and the third release point to fail for the same
underlying reason. Confirmation keyed on a status VALUE, and a value cannot say
when it was earned.

Provider startup output can legitimately parse as COMPLETED, which then latches
via _STICKY_READY_STATUSES, and send_input only ARMS the next transition without
touching the cached value. So wait_until_status returned on its first poll
against a COMPLETED from BEFORE the send -- the reviewer measured 0.008s at exact
head -- and the pending-delivery mask cleared on the previous turn's result while
the new task had emitted nothing. Every existing test in this area seeded IDLE
and produced COMPLETED afterwards, so none of them could see it.

The fix uses recency, not value. awslabs#712 already maintains exactly the needed
counter: _capture_generation advances in notify_input_sent (a new turn began) and
in _process_chunk (real output arrived), and its own comment names this problem --
"checking _last_status alone cannot see a new turn, because notify_input_sent
deliberately leaves _last_status ... while arming the revert".

- status_monitor gains one read-only accessor, output_generation(), exposing that
  counter under the lock. No behaviour change; generic get_status callers are
  untouched.
- _run samples it immediately after send_input (so the notify_input_sent bump is
  already counted) and passes it down.
- _wait_for_post_dispatch_start replaces the bare wait at both sites, requiring a
  started status AND a generation strictly greater than the dispatch sample. A
  cached pre-dispatch COMPLETED can no longer confirm, because no output arrived.

Event-inbox backends (herdr) are deliberately exempt via dispatch_generation=None.
They start no FIFO reader, so _process_chunk never runs, the generation never
advances from output, and gating them would make confirmation unsatisfiable --
every resubmit burned and the worker torn down. They need no gate: get_status
derives their status on demand at call time, so there is no cached value to go
stale. This was the one thing that would have broken the design and it is tested.

Tests: three seeded-completion tests in TestConfirmationRequiresPostDispatchEvidence,
which seed the COMPLETED FIRST rather than producing it after dispatch -- the gap
all reviewers named. Mutation-tested: removing the generation clause fails
test_pre_dispatch_completed_does_not_confirm_the_send while the other two still
pass, so the pair discriminates in both directions.

test_deferred_submit_verification.py's 8 stub sites moved from wait_until_status to
_wait_for_post_dispatch_start. They stub the wait as a collaborator to script
resubmit/direct-probe outcomes; the refactor changed which collaborator, not what
they assert.

304 passed across test_terminal_service_full.py, test_launch.py,
test_session_service.py, test_status_monitor.py and
test_deferred_submit_verification.py. black and isort clean.

Still owed on this PR, from gutosantos82's review: reported_status wiring at
session_service.get_session and list_siblings is unguarded (deleting either call
fails no test), and coro.close() is mutation-survivable.
Closes the three coverage gaps gutosantos82 raised, two of which they called
blocking and which had been carried across two review rounds. All three were
mutation-survivable: the code was correct, but nothing would have noticed if it
stopped being.

The masking invariant -- a terminal whose accepted initial message is undelivered
must not read completable anywhere a client can see it -- was pinned only at the
function level by TestReportedStatusMasking. Nothing pinned that the outward
surfaces actually CALL it. Their measurements: deleting the call in
session_service.get_session left 162 tests passing; deleting it in list_siblings
left 130 passing. So the whole security property of this PR rested on two
unguarded call sites.

- get_session: two terminals, one with a pending mark, asserting the pending one
  reads UNKNOWN and the other still reads IDLE. This is the surface
  examples/fleet/panel and ops_mcp_server.get_session_info read.
- list_siblings: same shape against a seeded COMPLETED, which is the reading that
  would invite a supervisor to treat an undelivered worker as finished. The
  existing sibling tests all use empty sibling lists, which is why this was
  invisible.

The coro.close() pin uses cr_frame rather than the warning. A closed coroutine has
cr_frame is None; a never-started, never-closed one does not. gutosantos82
suggested forcing gc.collect() and asserting via pytest.warns or making
PytestUnraisableExceptionWarning an error -- both work, but they hang the assertion
off GC timing and pytest's unraisable hook, which is why deleting production's
coro.close() previously failed nothing. Capturing the coroutine from the fake loop
and checking cr_frame is deterministic instead.

All three mutation-tested together: removing coro.close() and bypassing
reported_status at both call sites fails exactly the three new tests and nothing
else.

306 passed across test_terminal_service_full.py, test_session_service.py,
test_launch.py, test_deferred_submit_verification.py and test_status_monitor.py.
black and isort clean.
… delivery

The split was announced but only half done. awslabs#731 was opened carrying the Codex
provider work, but the same changes were still present here, so both PRs shipped
codex.py and its tests. Merging both would have applied that work twice.

Reverts providers/codex.py, test/providers/test_codex_provider_unit.py and
test/providers/test_startup_handler_nonblocking.py to this branch's base, so awslabs#731
is now their only home. What remains here is one concern: server-side
initial-message delivery and the non-completable mask that makes polling it safe.

Reverted to the BASE (c0c9b72) rather than to current origin/main deliberately --
taking origin/main's copies would have pulled awslabs#723's merged yolo/codexProfile
warning into this diff, making it look like this PR contained work it did not.

Scope is now 8 files, +1593/-63: launch.py, session_service.py, status_monitor.py,
terminal_service.py and their four test modules.

306 passed across test_launch.py, test_terminal_service_full.py,
test_session_service.py, test_deferred_submit_verification.py and
test_status_monitor.py. black and isort clean (verified by exit code).
tedswinyar added a commit to tedswinyar/cli-agent-orchestrator that referenced this pull request Sep 4, 2026
…ranch's squash dropped

Extracting the Codex half of awslabs#566 into a single commit silently reverted
awslabs#723 (05f86a6), which is an ancestor of this branch: the WARNING when
allowed_tools ["*"] forces --yolo and discards an explicit codexProfile,
the INFO recording that ~/.codex/config.toml's [sandbox_*] tables --
including network_access -- are inert under --yolo, and the three tests
that pin them. docs/codex-cli.md still documented the WARNING, so the
branch shipped docs describing logging it had removed.

Restored byte-identical to main; no startup-handling code is touched.
@tedswinyar

Copy link
Copy Markdown
Member Author

@haofeif Confirmed and fixed at 91c3590b. Head is now 4cf66925.

You were right that the mask could clear on the previous turn's result, and right that the existing suite couldn't see it. The reason all three release points I tried failed for the same reason: confirmation keyed on a status VALUE, and a value cannot say when it was earned. Provider startup output can legitimately parse as COMPLETED, that latches via _STICKY_READY_STATUSES, and send_input only arms the next transition without touching the cached value. So wait_until_status returned on its first poll against a pre-dispatch COMPLETED — your 0.008s measurement — with the new task having emitted nothing.

The fix uses recency instead of value. #712 already maintains exactly the counter needed: _capture_generation advances in notify_input_sent (a new turn began) and in _process_chunk (real output arrived), and its own comment names this very problem — "checking _last_status alone cannot see a new turn, because notify_input_sent deliberately leaves _last_status … while arming the revert."

  • status_monitor gains one read-only accessor, output_generation(), exposing that counter under the lock. No behaviour change; generic get_status callers are untouched.
  • _run samples it immediately after send_input, so the notify_input_sent bump is already counted.
  • _wait_for_post_dispatch_start replaces the bare wait at both sites, requiring a started status and a generation strictly greater than the dispatch sample. A cached pre-dispatch COMPLETED can no longer confirm, because no output arrived.

Event-inbox backends (herdr) are deliberately exempt via dispatch_generation=None: they start no FIFO reader, so _process_chunk never runs and the generation could never advance — gating them would burn every resubmit and then tear the worker down. They need no gate anyway, since get_status derives their status on demand, so there is no cached value to go stale.

Tests. Three tests in TestConfirmationRequiresPostDispatchEvidence that seed the COMPLETED first rather than producing it after dispatch — the gap you and the other reviewers named. Mutation-tested: removing the generation clause fails test_pre_dispatch_completed_does_not_confirm_the_send while test_post_dispatch_output_does_confirm and test_event_inbox_backends_are_not_gated_on_generation both stay green, so the set discriminates in both directions rather than just asserting the happy path.

test_deferred_submit_verification.py's 8 stub sites moved from wait_until_status to _wait_for_post_dispatch_start. They stub the wait as a collaborator to script resubmit/direct-probe outcomes, so the refactor changed which collaborator, not what they assert.

306 passed across test_terminal_service_full.py, test_launch.py, test_session_service.py, test_status_monitor.py and test_deferred_submit_verification.py. black/isort clean.

Also addressed since your last look, from @gutosantos82's review (7f52b23c): the reported_status wiring at session_service.get_session and list_siblings was unguarded — deleting either call failed no test, so the masking property of this PR rested on two unpinned call sites — and coro.close() was mutation-survivable. All three now have tests; the coro.close() one pins cr_frame is None rather than the warning.

Scope change worth flagging: the Codex startup-handling work that was on this branch has moved to #731 (4cf66925). This PR is now delivery only — launch.py, terminal_service.py, status_monitor.py, session_service.py and their tests. Reviewing #731 separately should be easier, and it means nothing here touches the provider.

CI is green except Security Scan, whose Trivy step currently fails on main too (every recent run, including 3eafb6d6) — not something this branch introduces.

@haofeif haofeif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 4cf669250dac33f8b3dcd50d11d9241461c2c13c against base 3eafb6d64684650155a9b2c422ea1d85c3f39c18. The moved commits fix the prior cached pre-dispatch COMPLETED false-confirmation, but introduce the opposite boundary race:

[P1] src/cli_agent_orchestrator/services/terminal_service.py:2158-2163 — sampling dispatch_generation after send_input() returns can reject a task that already completed. send_input() arms the generation before sending keys, and its own comments allow processing/completion frames to arrive while the blocking key send is still executing. If a fast worker emits and completes before await asyncio.to_thread(send_input, ...) returns, the valid output increment is already included in the newly sampled baseline. With no later output, _wait_for_post_dispatch_start() sees COMPLETED at a generation equal to that baseline, rejects it, retries Enter or redelivers the task, and can ultimately delete a worker that successfully performed the task. This exact scheduling path was reproduced at the reviewed head. Capture/return the boundary inside send_input() immediately after notify_input_sent() and before keys can produce output, or use an output-only generation sampled before dispatch; do not sample the combined counter after dispatch returns.

The focused launch/session/terminal/deferred-verification suite passed 306 tests with 3 deselected. The current Security Scan failure is inherited dependency baseline, not this delta.

…ut-only generation

Review (haofeif, round 8): the deferred initial-message path sampled its
dispatch_generation AFTER send_input returned. send_keys blocks for the
provider's submit delay, during which a fast worker can emit and complete,
so that output sat inside the baseline; _wait_for_post_dispatch_start then
rejected a genuine COMPLETED, resubmitted, and could delete a worker that
had done the task.

Two changes. StatusMonitor gains an output-only generation that only
_process_chunk advances; output_generation() now returns it. The counter it
used to expose (_capture_generation) also moves on notify_input_sent, so a
redelivery's own arm advanced it past the baseline and a still-cached
pre-dispatch COMPLETED satisfied the post-resubmit wait. send_input's body
moves to dispatch_input, which returns the generation sampled after
notify_input_sent and clear_rolling_buffer and before the first key;
send_input stays a bool wrapper because POST /terminals/{id}/input echoes
its value as {"success": ...}. The deferred path calls dispatch_input and
hands the boundary to the confirmation gate.

Tests pin the pre-key sample under output landing mid-send, the gate
confirming with the inner boundary and rejecting with the post-send one,
and the output-only counter ignoring arms. Adds the missing CHANGELOG entry.
@tedswinyar

Copy link
Copy Markdown
Member Author

@haofeif Confirmed and fixed; head is c6f15fde. You were right about the schedule and about the remedy: the boundary is now captured inside the send.

What was wrong. send_input armed the monitor, then blocked in send_keys for the provider's submit delay, and only after it returned did the deferred path sample output_generation(). A worker that emitted and completed inside that delay had its output folded into the baseline. _wait_for_post_dispatch_start then demanded a generation strictly greater than a value that already contained this task's only output, rejected a real COMPLETED, resubmitted, and could delete a worker that had done the task.

A second hole in the same gate, found while fixing the first. output_generation() exposed _capture_generation, which notify_input_sent also bumps. So the first redelivery's own arm advanced the generation past the baseline, and a still-cached pre-dispatch COMPLETED satisfied the post-resubmit wait. The comment at that call site claimed the same recency requirement as the first wait; the counter could not deliver it.

The fix (two parts).

  • StatusMonitor keeps a separate output-only generation that _process_chunk alone advances; output_generation() now returns it. _capture_generation is unchanged and still moves on arm, because the stale-capture logic needs that.
  • send_input's body moved to dispatch_input, which returns the output generation sampled after notify_input_sent and clear_rolling_buffer and before the first key. send_input is a thin wrapper that keeps its bool contract for every other caller. The deferred path calls dispatch_input and hands the returned boundary to the confirmation gate; the event-inbox None opt-out is unchanged.

Two design questions a reader will have, answered up front. Why not return the boundary from send_input itself? Because POST /terminals/{id}/input returns that value on the wire as {"success": ...}; an int would change the response, and a first-ever dispatch has boundary 0. Why a new counter rather than the byte-buffer epoch clear_rolling_buffer already hands stateful providers? That epoch advances on clear, never on output, so it cannot say whether anything arrived after the boundary. The sample is deliberately a separate lock acquisition after the clear: a chunk landing between the two is pre-dispatch output and this ordering folds it into the baseline, whereas sampling under the clear's lock would leave it outside and count it as evidence. The docstring also states plainly what the counter proves: that the reader delivered a chunk after the sample, not that the chunk belongs to this task; a late startup frame landing before the keys reach the pane counts too, which is the same approximation the rolling-buffer status detection already makes and the conservative side of the previous defect.

Tests. dispatch_input returns the pre-key generation while a stubbed send_keys advances the counter mid-send (the reviewer schedule); the gate confirms with that boundary and, as documentation of the defect, rejects with the post-send sample; notify_input_sent leaves the output generation alone while moving the capture generation; each chunk advances it by one; reset and forget clear it. Mutation-checked: sampling after send_keys fails the first test, reading the capture counter fails the arm test, dropping the chunk bump fails the count test. The two existing deferred-path tests that stubbed send_input now stub dispatch_input; nothing else changed in them.

Verification (HOME sandboxed): status-monitor, terminal-service, deferred-verification, session-service, launch and plugin-event suites: 345 passed, 3 deselected, on the branch merged with current main; wider test/services, test/cli, test/utils, test/providers: 5781 passed, 10 skipped, 45 deselected, 1 xfailed (--ignore=test/services/agui). black/isort clean; mypy reports only the findings already present on main for these two files (none new).

Also added the [Unreleased] CHANGELOG entry this PR was missing.

On the six open threads. The two on terminal_service.py are this gate: the 09-01 one is outdated by the post-dispatch-evidence commit, and the one at 2248 is what this commit closes. launch.py:478 is the same concern seen from the CLI: the server-side mask keeps the terminal non-completable until the send has been made and confirmed, so poll_until_done cannot return on pre-dispatch samples; test_poll_until_done_cannot_return_before_initial_send_is_issued drives the real scheduler and the real poller against your startup sequence. The 08-07 launch.py budget thread described the client-side second /input request racing a longer-than-budgeted init; that request no longer exists, and on the headless path init runs inside poll_until_done's window. The non-headless pre-attach poll is still a fixed 120s and can be shorter than a slow provider's init, so I would leave that thread open as its own question unless you consider it answered here. The 08-29 codex.py login-menu thread and the copilot heartbeat nit are on Codex code that moved to #731, where the login menu is a settled exit. I'll resolve the ones answered here unless you'd rather keep any open.

@tedswinyar
tedswinyar requested a review from haofeif September 13, 2026 23:57

@haofeif haofeif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head c6f15fde3aed00bb4088e71837cc8a0d2d6ceac5 against base 948c3d8004faa9f2f61b1c65dcde19291d7e072b.

The pending mark now closes the original pre-dispatch polling window and is held through confirmation, but three delivery-contract defects remain:

  1. [P1] Stamp accepted status observations with their output generationsrc/cli_agent_orchestrator/services/terminal_service.py:2008-2012,2184-2186. Confirmation still reads cached status and global output generation independently. A post-boundary composer redraw can therefore be paired with a pre-dispatch cached COMPLETED; if Enter was swallowed, delivery is falsely confirmed, the mask is cleared, and the previous response can be returned. Herdr bypasses the recency requirement entirely through dispatch_generation=None. This is the unresolved causal-status problem in the existing cached-COMPLETED thread.

  2. [P1] Do not report --async success before delivery has an observable durable outcomesrc/cli_agent_orchestrator/cli/commands/launch.py:445-450 and src/cli_agent_orchestrator/services/terminal_service.py:1705-1762,2218-2259,2311-2325. The CLI exits zero immediately after session creation. Later initialization/submit failures are log-only without a caller, startup WAITING_USER_ANSWER drops the pending message, and a server restart loses the in-memory delivery task. This regresses the base contract, where async launch returned only after initialization and a successful input request.

  3. [P2] Preserve a separate provider initialization budgetsrc/cli_agent_orchestrator/cli/commands/launch.py:49-57,451-458. The combined wait assumes 120 seconds for initialization plus 300 for task execution, but supported providers can legitimately consume substantially more than 120 seconds across shell, prompt-handler, and readiness phases. A valid initialization can consume the task budget or exceed the client deadline before dispatch, while the server later executes the task and a retry creates duplicate work. Start the task budget at confirmed delivery or derive initialization allowance from the selected provider/profile.

The Codex-specific historical threads are now outside this PR and belong to #731; the earlier mark-release and pre-dispatch polling findings are fixed at this head.

@gutosantos82 gutosantos82 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: #566 — fix(launch): deliver the initial message via POST /sessions instead of a dropped second request

  • Author: tedswinyar · Head: c6f15fde3aed00bb4088e71837cc8a0d2d6ceac5 (base 948c3d80) · Size: +1897/−66 across 10 files
  • CI: all 19 checks SUCCESS · Mergeable: MERGEABLE · reviewDecision: CHANGES_REQUESTED
  • Human review state: haofeif CHANGES_REQUESTED at this exact head (2026-09-14T10:08Z, round 9); call-me-ram's APPROVED (08-21) is stale (predates rounds 3–8)

Summary

cao launch now delivers the initial message inside the POST /sessions create body (server-owned deferred init) and deletes the client-side /input POST, the pre-send readiness wait, and the settling sleep — fixing a real, well-diagnosed silent message drop. The hard part is making the deferred terminal safe to poll: a dispatch-evidence gate (output_generation() sampled at the send boundary) must confirm delivery, and reported_status() masks unearned IDLE/COMPLETED as UNKNOWN at three outward surfaces. Our verifier reproduced every mutation claim exactly and the gate rejects a cached pre-dispatch COMPLETED in the unit-level exercise. However, our correctness reviewer independently confirms haofeif's standing round-9 P1: the gate reads cached status and the global output generation independently, so a sticky pre-dispatch COMPLETED can be paired with an unrelated post-boundary output frame (spinner redraw, trailing startup output) and falsely confirm a send whose Enter was swallowed. With that P1, the --async early-success P1, and the init-budget P2 all outstanding — and haofeif's CHANGES_REQUESTED sitting at this exact head — the only correct verdict is request-changes deferring to haofeif.

Findings by severity

CRITICAL

  1. Confirmation gate pairs cached status with an unrelated output generation — haofeif's standing P1 confirmed at this head. _wait_for_post_dispatch_start (terminal_service.py:2008–2012) reads get_status() and output_generation() independently; the status value is never required to have been produced by the output that bumped the generation. Reachable chain: startup output latches a sticky COMPLETED (_STICKY_READY_STATUSES, status_monitor.py:41–49; preserved across clear_rolling_buffer, :708–724; returned verbatim by get_status, :830–836) → the paste/Enter is swallowed (the exact case this machinery exists for) → any non-task chunk bumps _output_generation (:247, its own docstring at :641–651 acknowledges spinner/MCP frames) without downgrading the sticky COMPLETED → status ∈ _DEFERRED_STARTED_STATUSES AND generation > boundary → falsely confirmed, mask released, cao launch prints startup output and exits 0, task silently dropped. haofeif's quoted fix direction: "stamp accepted status observations with their output generation." The herdr opt-out (dispatch_generation=None, terminal_service.py:2184–2186) bypasses the recency requirement entirely (same class, see Minor 4). (correctness; conversation; confirms haofeif 09-14 P1)

  2. A standing human CHANGES_REQUESTED sits at this exact head — an approve would directly conflict with it. haofeif's 09-14 review is against c6f15fde itself, so its conditions are outstanding by construction: 2×P1 (finding 1 above; --async durability below) + 1×P2 (init budget below). All 6 inline review threads are unresolved (isResolved=false); two are anchored to live code (launch.py:456; terminal_service.py:2272 — same substance as P1 #1). Do not approve or advance; the block is haofeif's to clear. (conversation)

MAJOR

  1. --async reports success before delivery has any observable durable outcome (launch.py:444–450). The CLI exits 0 right after session creation; later init/submit failures are log-only for a bare launch (caller_id=None), a startup WAITING_USER_ANSWER drops the pending message (TerminalInputBlockedError), and a server restart loses the in-memory _deferred_init_tasks entry. Regresses the base contract, where async launch returned only after init and a successful input request. = haofeif 09-14 P1 #2. (correctness; conversation; security notes the in-memory mask likewise has no cross-restart guarantee)

  2. Combined 120s+300s wait folds the init budget into the task budget (launch.py:49–57, 451–458). Supported providers can legitimately exceed 120s of init, so a valid init consumes the task budget or blows the client deadline while the server later executes the task — a retry then duplicates work. haofeif's asked-for fix: start the task budget at confirmed delivery, or derive the init allowance from the provider/profile. = haofeif 09-14 P2. (correctness rated Minor on impact; kept MAJOR as a standing maintainer condition)

  3. Vacuous regression guard: test_a_genuine_early_completion_is_not_hidden_by_the_mask patches the wrong function (test_terminal_service_full.py:3450–3452). It patches terminal_service.send_input, but _run now dispatches via dispatch_input (terminal_service.py:2124) — its two sibling tests were updated (:3180, :3318); this one was missed. Verified by probe: the real dispatch_input raises (no tmux), the mask is cleared in except/finally, fake_send_input never runs, and the documented round-4 "mask strands a genuine early completion" scenario is never exercised. Fix: patch dispatch_input. (tests)

  4. PR-description drift: output_generation() does not expose _capture_generation. The implementation adds a NEW dedicated output-only counter self._output_generation (status_monitor.py:158, bumped only at :247) — deliberately, per the code comment (:150) and the author's final PR comment — while the PR body still claims it exposes "the _capture_generation counter #712 already maintains." Code is right, CHANGELOG is right ("output-only generation"); the body's claim is stale from revision 91c3590. Author should update the body. (consistency)

MINOR

  1. herdr exemption assumes on-demand status is always current across a task boundary (terminal_service.py:2185, dispatch_generation=None). If get_native_status still reports the prior turn's COMPLETED until the new inbox event lands, confirmation succeeds before the task starts — the stale-value class of finding 1 via the opt-out. haofeif's 09-14 P1 names this bypass explicitly. (correctness)
  2. Dead import: wait_until_status (terminal_service.py:113) is no longer called anywhere in the file — this PR removed both call sites; remaining mentions (:1920, :1979) are comments. (consistency)
  3. Event-inbox branch of the _run wiring is untested: the None if supports_event_inbox() else dispatch_boundary conditional is only ever exercised on the else branch; the None path is tested only by calling _wait_for_post_dispatch_start directly. 100% line coverage hides this (branch coverage disabled). (tests)
  4. Generation gate inert in one integration test: test_poll_cannot_complete_on_the_stale_post_dispatch_idle (test_terminal_service_full.py:3320) leaves output_generation as an unconfigured MagicMock, so MagicMock() > 0 is truthy — the _run→confirm→wait generation plumbing is never asserted end-to-end with a real integer. (tests)
  5. Masking surface enumeration is the only guarantee: reported_status is wired into exactly 3 surfaces (terminal_service.py:2335, session_service.py:303–306, terminal_service.py:2429–2431); any other status-emitting channel (AG-UI snapshots, PTY/websocket broadcasts) reports raw IDLE/COMPLETED during the pending window. Not exploitable — the mask fails toward "not-yet-done", and ERROR/WAITING_USER_ANSWER always pass through unmasked. (security)
  6. Verification-claim count drift: PR body says "306 passed"; actual at this head is 325 passed, 2 failed (pre-existing environmental: ~/.aws PermissionError, fail identically on main), 3 deselected. More passes than claimed; worth refreshing the body. (verifier)
  7. Stale comment in untouched file: claude_code.py:1341 still describes the deferred path as _schedule_deferred_init -> send_input(...); it is now dispatch_input. (consistency)

NIT

  1. Merge commit "merge: main into agent/caom-7it-fix" is not Conventional-Commits-compliant; harmless under squash-merge (the other 17 commits and the PR title are compliant). (conventions)
  2. get_terminal masking has no dedicated wiring test mirroring the get_session/list_siblings ones (it is pinned indirectly via the poll integration test). (tests)
  3. Theoretical false-teardown wedge: a delivered task emitting zero output within the confirm budget never confirms, burns resubmits, and tears down a healthy worker — rests on "echo/spinner always bumps the generation" as an assumption, not an invariant. (correctness)
  4. PR body wording "deletes _create_session_timeout/_effective_init_timeout/_CREATE_OVERHEAD_MARGIN" doesn't match this diff — relative to base 948c3d8 they never existed; end state is correct. Similarly, the #731 split "shares no files" is literally true, but launch.py:389–424 (WAITING_USER_ANSWER readiness widening + sign-in hint) is the client-side half of #731's Codex login-menu behavior — behaviorally coupled PRs. (consistency)

Dynamic verification (executed in the PR worktree at c6f15fd)

  • Tests: 2 failed, 325 passed, 3 deselected in 29.32s across the five cited files (Python 3.10; bare uv run pytest blocked by an unrelated numpy-2.4.6 source-build issue on this host). Both failures reproduce identically on pre-PR main (sandbox ~/.aws PermissionError) — environmental, not PR-caused.
  • Mutation claims: all reproduced exactly. (a) Removing the generation clause → only test_pre_dispatch_completed_does_not_confirm_the_send fails, both siblings pass. (b) Deleting reported_status at get_session → exactly its wiring test fails; at list_siblings → exactly its wiring test fails. All mutations reverted; worktree left clean.
  • Formatters: black --check (4 changed src files) and isort --check-only both clean.
  • Behavior exercise: with the real StatusMonitor + real _wait_for_post_dispatch_start, a cached pre-dispatch COMPLETED with no new output is rejected; one real post-boundary output chunk confirms. (Finding 1 is about output that is unrelated to the task bumping the generation — a case this unit exercise does not cover.)

Security assessment

Net security improvement; no CRITICAL/MAJOR findings. The "secrets out of the access log" claim is TRUE: the deleted path sent the prompt as a URL query param (logged by HTTP access logs); the new path carries it in the POST body, matching how env_vars already traveled. No new logging of message content (idempotency uses SHA-256; exception logs use {e!r}); no new injection surface (tmux bracketed-paste preserved, ERROR-state paste guard intact); POST /sessions scope-gating identical to the removed route; the UNKNOWN mask never hides ERROR or WAITING_USER_ANSWER.

Conventions

Clean: compliant PR title, correctly placed CHANGELOG entry, inclusive language, no pyproject/uv.lock changes needed, no provider files touched, no doc updates owed (initial_message on POST /sessions pre-existed). Only the merge-commit NIT (14).

Verdict

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants