Skip to content

fix(codex): opt into the deferred-init direct status probe (#659) - #714

Closed
call-me-ram wants to merge 3 commits into
awslabs:mainfrom
call-me-ram:fix/codex-direct-status-probe
Closed

call-me-ram wants to merge 3 commits into
awslabs:mainfrom
call-me-ram:fix/codex-direct-status-probe

Conversation

@call-me-ram

@call-me-ram call-me-ram commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes #659. The diagnosis and the tested fix are @karstenjakobsen's — this PR just lands their one-liner with the regression coverage the issue suggested, per the triage on the issue (2026-08-23).

The bug

Codex workers created through deferred assign() can be classified as "submit dropped" while they are visibly working. The confirm loop polls the cached event-driven status, which detects only at rising-edge/quiescence — and a repainting Working (…) spinner defers quiescence, so the cache can sit IDLE past the whole 8s window. The loop's direct capture-pane fallback exists for exactly this (#496), but it is gated on supports_direct_status_probe, which CodexProvider never set. Outcome, verified in the issue: up to three re-deliveries of the full task into the already-working pane, then delete_worker=True teardown of an active worker.

The fix

supports_direct_status_probe = True on CodexProvider, with a comment recording why the opt-in is valid: Codex's get_status() is line-oriented text analysis of exactly the rendered shape a capture-pane snapshot provides — the same frames supports_screen_detection already feeds it in production — and it has no dispatch bookkeeping a fresh capture would bypass (the kiro_cli-style disqualifier documented on _worker_is_started_direct). This is the per-provider opt-in the #496 review deliberately scoped for; the family so far: claude_code (#480), opencode (#496), minimax (#625), now codex.

Failure-direction check (the reason the flag is an opt-in at all): on every dropped-submit shape the probe fails toward recovery, not away from it — an empty composer reads IDLE (probe returns False → normal re-delivery), a pasted-but-unsubmitted message reads IDLE (→ box check → bare Enter), and any probe exception falls through to the cached path. The probe can only short-circuit re-delivery on frames the detector classifies as started, which is the same detector the cached path already trusts — just on live data.

Testing

  • test_codex_confirm_succeeds_from_live_frame_without_redelivery: the issue's suggested regression, end to end — REAL CodexProvider, a real rendered Working (3s • esc to interrupt) frame behind the mocked capture, cached status pinned IDLE for every poll → confirm returns started, nothing typed into the pane (no re-delivery, no blind Enter), so the teardown arm can't fire. Plus a direct flag pin.
  • Mutation-checked: reverting the one-line flag turns both tests red — the end-to-end one at the re-delivery assertion, not just the attribute assert.
  • Suites: codex provider unit tests + deferred-submit + terminal_service suites green; black/isort clean.

Round 2 (dca3dd34)

The status alone is no longer the verdict. After a started read the probe asks the provider direct_probe_confirms_submission(output, message); the base default keeps the status-only verdict (opencode/minimax unchanged), and Codex binds it to the trace a submitted turn leaves: the message echoed as a transcript line with the turn's activity rendered below it. Startup residue (the MCP startup spinner / any startup bullet, which get_status reads as PROCESSING within its 25-line tail and as COMPLETED beyond it) sits above the composer and cannot bind, so a dropped paste falls through to redelivery. Regressions cover the dropped-task residue frame at both distances, residue over an unsubmitted paste, residue over an accepted turn, an accepted turn parked on the 0.147 approval menu, a paste whose own lines are bullets, and the short-message floor.

Round 3 (510eea3b) — the verdict no longer reads message text

All three findings reproduced verbatim, and they share one root cause: pane text cannot establish causality. Round 2 is withdrawn rather than patched.

The binding is now the dispatch boundary the code already maintains. send_input empties the StatusMonitor rolling buffer immediately BEFORE sending keystrokes (deliberately, so the turn's first bytes survive), so every byte in it arrived post-dispatch by construction — unforgeable by a shared prefix, unevictable by a bounded capture tail, indistinguishable from nothing when the pane was idle.

  • direct_probe_confirms_dispatch(post_dispatch_output) replaces the text-matching hook and inspects no message. Base default unchanged, so opencode and minimax keep their status-only verdict.
  • Codex accepts only the progress spinner, which a live turn repaints and a stopped one cannot emit. A bullet is refused, because the composer echoes a paste as it renders.
  • A False now means UNPROVEN, so the full re-send is withheld from a probe-capable provider whenever output arrived after the dispatch without proving the turn started. That is the accepted-turn-past-its-echo shape; re-pasting there duplicates work. A dropped paste emits nothing and still re-sends.

Five mutations each redden their tests. 616 passed across the codex/opencode/minimax/omp/grok, terminal_service, status_monitor and deferred-submit suites.

The same prefix collision also affects _message_visible_in_box on main, independently of this probe — filed as #727 rather than widened into this PR, since the step path (#562) shares that helper.

The deferred-assign confirm loop polls the cached event-driven status,
which detects only at rising-edge/quiescence; a repainting Working
spinner defers quiescence, so the cache can sit IDLE past the whole
confirm window while the pane visibly works. The loop's capture-pane
fallback exists for exactly this lag but is gated on
supports_direct_status_probe, which CodexProvider never set — so the
loop re-delivered the task into the working pane up to three times and
then tore down the active worker.

Codex's get_status() is line-oriented analysis of exactly the rendered
shape a capture provides (the same frames supports_screen_detection
feeds it in production) with no dispatch bookkeeping to bypass, so the
opt-in is valid; every dropped-submit shape still reads IDLE, keeping
the probe fail-toward-recovery. Regression pins the opt-in end to end
with the real provider and a real Working frame.

Fixes awslabs#659
@codecov-commenter

codecov-commenter commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@7fed05b). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/cli_agent_orchestrator/providers/base.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #714   +/-   ##
=======================================
  Coverage        ?   91.63%           
=======================================
  Files           ?      203           
  Lines           ?    28461           
  Branches        ?        0           
=======================================
  Hits            ?    26081           
  Misses          ?     2380           
  Partials        ?        0           
Flag Coverage Δ
unittests 91.63% <94.44%> (?)

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.

@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.

Reviewed at exact head 23d1472435d5ea8fd561df08d7cbc26cb62885a1. The Codex opt-in fixes the reported stale-cache case for a clean current-task Working frame, but the direct detector is not causally bound to the submitted task. Recent startup activity can satisfy it after the task paste was dropped, suppressing the recovery this path exists to perform.

Comment thread src/cli_agent_orchestrator/providers/codex.py
A started status read off the live pane is only half the verdict: the
pane must also attribute it to the message being confirmed. get_status
classifies the frame as a whole, and Codex's startup chrome shares the
shape of its task activity — the "Starting MCP servers (4s • esc to
interrupt)" spinner is the TUI progress pattern and any startup bullet
is an assistant marker. initialize() returns once the bottom 15 lines
are activity-free, but get_status scans 25 lines for the spinner and
the whole capture for a marker, so a task-less pane with that residue
16+ lines above the idle composer reads PROCESSING (or COMPLETED once
the spinner leaves the tail). The unqualified opt-in accepted that as
"started", skipped the redelivery a dropped paste needs, and left the
supervisor waiting on a task that was never submitted.

Add a provider hook, direct_probe_confirms_submission(output, message),
that the probe consults after a started read. The default keeps the
status-only verdict for providers whose indicators are turn-scoped
(the existing opt-ins are unchanged). Codex overrides it with the
causal trace a submitted turn leaves: the message is echoed as a
transcript line and the turn's activity (spinner or bullet) renders
below it, whereas startup residue sits above the composer and an
unsubmitted paste has nothing but the footer beneath it. Bullets that
are part of the pasted message text do not count as activity, and a
message too short to match reliably cannot bind, so every unattributed
read falls through to the box check and redelivery.

Regressions pin the finding's frame (residue at both the PROCESSING
and COMPLETED distances, no task → full redelivery on every attempt),
residue over an unsubmitted paste (bare Enter), residue over an
accepted turn (started, nothing sent), an accepted turn parked on the
0.147 approval menu (started, no blind Enter into the menu), a paste
whose own lines are bullets (no self-attribution), and the short-
message floor. Neutralizing the hook, the probe's use of it, or the
message-internal bullet exclusion each turns the corresponding tests
red.
@call-me-ram
call-me-ram requested a review from haofeif September 2, 2026 07:26

@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 dca3dd34. The new provider hook rejects the original startup-residue frame, but its textual attribution still has both false-positive and false-negative paths. I independently reproduced stale/pristine prompt frames being accepted, valid fast replies being rejected, and the bounded-history ambiguity that authorizes a duplicate full submission. The focused provider/deferred-delivery set passes (299 tests, 3 skipped) but does not cover these cases.

Comment thread src/cli_agent_orchestrator/providers/codex.py Outdated
Comment thread src/cli_agent_orchestrator/services/terminal_service.py Outdated
return True
if re.match(STARTUP_ACTIVITY_PATTERN, line):
kept = re.sub(r"[^a-z0-9]", "", line.lower())
if kept and kept in message_text:

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] Do not discard a real reply because its text appears in the prompt

This global substring test cannot distinguish a prompt-owned bullet from a later response. For Reply with Done followed by the valid fast reply • Done, get_status() returns COMPLETED but this hook returns False because done occurs in the prompt. With the cached status still stale, the recovery loop sends bare Enter three times, reports non-delivery, and can tear down a worker that already completed successfully. Exclude bullets only when their line is inside the proven rendered message span; reply text matching words in the prompt is normal.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed, and the mechanism it belonged to is gone. Reproduced exactly as described: for Reply with Done answered by • Done, get_status was COMPLETED while the hook returned False, because done occurs in the prompt — a completed worker would have collected three bare Enters and then the teardown.

You identified the root cause precisely: a global substring test cannot tell a prompt-owned line from a later response, and reply text reusing the prompt's words is ordinary. Rather than tighten the span, 510eea3b removes message-text inspection from the verdict entirely — the hook never sees the message, so there is nothing left to discard a reply for resembling. That case is now a regression test asserting started=True with nothing sent.

Your suggested narrowing (exclude bullets only inside the proven rendered message span) would have fixed this specific case, but it still rests on locating that span in the pane, which is the same foundation the other two findings knocked out. The post-dispatch buffer avoids needing the span at all.

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 510eea3b: fast completion without a captured spinner still returns unproven and falls into repeated bare Enter. This remains open in pullrequestreview-5097575589.

…e text

Round 2 bound the probe's verdict to the submitted message by matching
that message against the rendered pane. haofeif showed the matching
itself is unsound, and reproducing each case confirmed all of it:

- The leading 24 collapsed characters are not message-unique. Every
  orchestrated handoff opens with the same banner, so an OLD handoff
  still in the transcript satisfied the match and its completion
  bullet vouched for a NEW message that was never delivered.
- A message absent from the bounded 200-line capture was read as a
  dropped paste, but an ACCEPTED turn can out-scroll its own echo
  while a spinner still runs. The full re-send then submitted the
  task a second time into the pane already working on it.
- Excluding bullets whose text appears in the message discarded real
  replies: "Reply with Done" answered by "• Done" was rejected, so
  a completed worker got three bare Enters and a teardown.
- A non-ASCII bullet in a pasted message normalizes to nothing, so an
  unsubmitted paste confirmed itself.

Text cannot establish causality. Use the dispatch boundary the code
already maintains instead: send_input empties the StatusMonitor
rolling buffer immediately BEFORE sending keystrokes, precisely so the
turn's first bytes are captured, so every byte in that buffer arrived
after the dispatch by construction. It cannot be forged by a shared
prefix, evicted by a bounded capture tail, or confused with leftover
chrome.

The provider hook becomes direct_probe_confirms_dispatch(post_dispatch
_output) and inspects no message text at all. The base default keeps
the status-only verdict, so the opencode and minimax opt-ins are
unchanged. Codex accepts one piece of evidence: the progress spinner,
whose "(<n>s • esc to interrupt)" shape a live turn necessarily
repaints and a stopped one cannot emit. A bullet is deliberately not
accepted, because the composer echoes a pasted message as it renders.

Because a False verdict now means unproven rather than idle, the full
re-send is also withheld from a probe-capable provider whenever any
output arrived after the dispatch without proving the turn started.
That is exactly the accepted-turn-past-its-echo shape, and re-pasting
there duplicates work. A genuinely dropped paste emits nothing after
the dispatch, so it still re-sends.

Regressions cover every reproduction above plus the round-1 startup
residue at both the PROCESSING and COMPLETED distances, escape-laden
spinner bytes, and the herdr backend that feeds no buffer. Five
mutations each redden their tests: neutralizing the hook, accepting
bullets as evidence, ignoring the hook, dropping the withheld-resend
guard, and sourcing the evidence from the pane instead of the buffer.

The pane-text prefix collision also affects _message_visible_in_box on
main, independently of this probe; filed as awslabs#727 rather than widened
into this PR, since the step path (awslabs#562) shares that helper.
@call-me-ram
call-me-ram requested a review from haofeif September 3, 2026 03:15

@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 510eea3b. Removing message-text matching fixes shared-prefix attribution and the nonempty-buffer guard prevents the bounded-history full-resend duplication, but two P1 submission-attribution failures remain: unrelated/prompt-owned post-dispatch spinner text can falsely confirm a dropped task, while a legitimate fast completion without a captured spinner is still rejected. The focused suites pass (302 tests, 3 skipped) but their fast-completion fixture injects a spinner not present in the rendered frame.

if not post_dispatch_output:
return False
return bool(
re.search(

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] A post-dispatch spinner is not evidence of this submission

Clearing the rolling buffer establishes when bytes arrived, not what caused them. If startup accepts the idle composer while an MCP-startup spinner above it is still repainting, that spinner enters this buffer after a dropped task paste and returns true. A multiline prompt containing the same progress line can also echo it when Enter is swallowed. Both exact-head probes report the worker started even though the task was never submitted. Confirmation needs a submission-specific acceptance signal; arbitrary prompt text or concurrent startup output must not vouch for the dispatch.

feeds a byte buffer: the probe then declines to vouch for the turn and
the caller falls back to its pre-existing behavior.
"""
if not post_dispatch_output:

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] Accept a legitimate fast completion without requiring a spinner

A fast turn can render the prompt echo and final reply without any captured progress frame. For Reply with Done followed by • Done, the pane is COMPLETED but this hook returns false; _message_visible_in_box() then finds the historical prompt echo and treats it as text still in the composer, sending bare Enter three times before teardown. The new regression inserts _SPINNER into post_dispatch_output even though its completion frame has none. Preserve a post-dispatch submission-generation/acceptance signal that survives direct completion rather than requiring spinner text.

@call-me-ram

Copy link
Copy Markdown
Collaborator Author

Both new findings reproduce at 510eea3b, and tracing why led me somewhere that changes my recommendation for this PR. Laying out the reasoning rather than pushing a fourth proxy.

The startup-spinner finding is not an edge case — it is the primary case

You wrote that a still-repainting startup spinner enters the buffer after a dropped paste. Reproduced exactly: _has_startup_idle_composer returns True with a live Starting MCP servers spinner 18 lines above the composer, so initialize() returns and the task is dispatched while that spinner is alive; its next repaints land in the post-dispatch buffer and my hook returns True. Task lost.

What I had not appreciated is when the cached status actually goes stale, which is the condition this probe exists for. In _schedule_screen_detection, the rising edge only fires if not was_bursting, and every chunk re-arms the quiescence timer. So the cache can sit stale for a whole confirm window in exactly one situation: output was already flowing when we dispatched, so no rising edge fires and the repaint keeps deferring quiescence. clear_rolling_buffer does not reset _bursting, so that state carries across the dispatch boundary.

That is the same condition as your finding. The circumstance that makes the direct probe necessary is the circumstance in which pre-existing output is streaming into my evidence window. Any pane-derived signal is therefore at its least trustworthy precisely when it is load-bearing. That is not a hole to patch; it is the design being wrong.

Your prompt-echo variant lands too — a message containing the progress line confirms itself, because the composer echoes it.

On the fast-completion finding

Also correct, and my test deserved the callout: it inserted _SPINNER into a buffer whose frame has none, which encoded my assumption instead of the scenario. That is the second time a test of mine asserted the thing I believed rather than the thing that happens, and it is the part of this round I am least happy about.

One qualifier on its production reachability, offered as information rather than a defence. A fast completion goes quiet, so quiescence fires 0.2s later and the cached status becomes COMPLETED well inside the 8s window — wait_until_status succeeds and the probe is never consulted. Reaching your repro in production needs the cache to be broken for a reason other than the lag this probe addresses. But the verdict is still wrong on the facts, the bare-Enter cascade you traced is real once it is reached, and it flows partly through the prefix defect in _message_visible_in_box I filed as #727 — the historical echo it matches is in the transcript, not the composer.

What I think should happen

I have now offered three signals — message text, then post-dispatch bytes, then the spinner within them — and you have found a real defeat for each. The pattern is not bad luck. Codex's TUI emits no submission-acceptance event, and every proxy I derive from the pane is either forgeable by the prompt, absent on a legitimate turn, or contaminated by whatever was already painting. I do not have a fourth candidate I would defend, and I would rather say so than spend another of your review rounds finding out.

So I propose narrowing this PR, and I would like your call on which shape:

A — Keep only the provider-agnostic safety fix. Withholding the full re-send when post-dispatch output exists without proof is correct independently of any opt-in, and it fixes a genuine duplicate-submission bug that affects opencode and minimax on main today (you verified that half as fixed). Drop the codex opt-in and the provider hook entirely. #659 stays open with a much sharper problem statement than it had: it is not a missing flag, it needs an acceptance signal that does not exist yet.

B — Same, plus build the acceptance signal properly, as its own piece of work: capture the rendered composer state at the dispatch boundary and confirm on the transition from "holds our text" to "empty with our text in the transcript". That is a real signal rather than a proxy, but it needs a pre-dispatch snapshot that nothing captures today, and it is a change to shared dispatch plumbing that deserves its own review rather than riding a provider opt-in.

My preference is A now, B as a separate issue, because A is small, unambiguously an improvement, and unblocks the duplicate-submission fix from the acceptance-signal question. But #659 is your issue to close or keep open, and if you would rather I attempt B here, say so and I will.

Holding the code as it stands pending your call, including the misleading test — it disappears entirely under A, and rewriting it now would only be work we throw away.

@call-me-ram

Copy link
Copy Markdown
Collaborator Author

Update with measurements rather than reasoning. I installed codex-cli 0.153.2 and captured a real turn under tmux, launched with CAO's own flags (--yolo --no-alt-screen --disable shell_snapshot), delivered the way send_input delivers (load-buffer + paste-buffer), with pipe-pane recording the raw stream in three separate windows: startup, pasted-but-unsubmitted, and the Enter that submits.

Two of my assumptions were wrong, one of yours does not reproduce on this version, and the thing I told you I did not have turns out to exist.

The acceptance signal is the composer resetting to its placeholder. This is the observable I said I lacked:

pasted, Enter withheld:   › Reply with exactly one word: Done      <- our text IS the composer
                          (no placeholder line present)

after Enter:              › Reply with exactly one word: Done      <- moved into the transcript
                          • Done
                          › Ask Codex to do anything               <- composer RESET to placeholder

Acceptance is structural and needs no message matching: on submit Codex clears the composer back to its placeholder and the message becomes a transcript line. Unsubmitted, the placeholder is absent because our text occupies it. STARTUP_IDLE_PLACEHOLDER_PATTERN already recognizes that placeholder, so the primitive exists in the codebase today. That is option B, and it is buildable — it was not a hypothesis I could justify before this capture.

Your fast-completion case does not reproduce on 0.153.2. The turn answered in about 3 seconds and the post-dispatch bytes still contained the spinner exactly once:

spinner occurrences in the accepted turn's post-dispatch bytes: 1
samples: ['• Working (0s • esc to interrupt)']

So on this build a fast turn does paint Working (0s) before replying. I am not claiming that settles it — one sample, one model, and a cached or refused turn may well skip it. But the premise is not free, and if you have a build where a completion renders with no progress frame at all I would like the version, because it changes which signal is viable.

Your paste-forgery case did not reproduce either, though the mechanism is real. A pasted message produced 125 bytes containing no spinner and no bullet, so an ordinary paste does not manufacture the evidence. Your point stands for a message that literally contains the progress line — that echoes — which is one more reason the composer transition is a better signal than any byte pattern.

Separately, the capture found a live bug that is worse than anything in this PR. On that same idle pane, _has_startup_idle_composer returns False and initialize() runs to its 60s timeout, because STARTUP_ACTIVITY_PATTERN matches Codex's own account notice • You have 1 usage limit reset available. — and get_status returns COMPLETED on that pre-task pane for the same reason. Filed as #739 with the captures. It is the concrete form of the thing we have been arguing about abstractly for three rounds: Codex's chrome is genuinely indistinguishable from agent output under the current patterns.

On the A-or-B question, this changes my recommendation. I proposed A because I had no acceptance signal and did not want to burn another round guessing. I now have one that is structural rather than textual, so I would rather build B — confirm on the composer's return to its placeholder, with the dispatch boundary establishing which side of the submit we are on. It also composes with #739: both want CAO to stop treating any bullet as agent activity.

Your call still governs the shape, and A remains on the table if you would rather land the safety half now and keep the acceptance signal out of a provider opt-in. Tell me which and I will build it. I have the raw streams for all three phases and can share or extend the capture — including any specific frame you would like me to reproduce on real hardware rather than argue about.

@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: #714 — fix(codex): opt into the deferred-init direct status probe (#659)

Summary

This PR (round 3) opts Codex into the deferred-init direct status probe (#659) and binds the verdict to post-dispatch bytes: a new direct_probe_confirms_dispatch(post_dispatch_output) hook on BaseProvider (default True), a Codex override that accepts only the TUI progress spinner as evidence, and a withhold-full-resend branch in redeliver_dropped_message. The engineering direction is genuinely improved over rounds 1–2 (no message-text matching, buffer emptied at the dispatch boundary), and the buffer-clear-before-keystrokes claim verifies. However, the core attribution problem remains: post-dispatch timing still does not establish causality. Our verifier demonstrated end-to-end that a message whose text contains a spinner-shaped token falsely confirms a swallowed-Enter delivery (silent task loss, a regression vector new in this PR, since pre-PR Codex ran the box check and recovered via bare Enter), and a legitimate completion that leaves no spinner in the rolling buffer is rejected toward the confirm-loop deadline and delete_worker=True teardown. Recommend Request changes.

Blocking (must fix before merge)

  • [security/correctness — verifier-demonstrated] providers/codex.py:824-847 — The composer echoes the pasted message into the rolling buffer, so a message whose literal text contains a (<n>s • esc to interrupt)-shaped line satisfies TUI_PROGRESS_PATTERN and falsely confirms a dropped submission. Verified end-to-end: with the Enter swallowed and the message still in the composer, redeliver_dropped_message returns started=True from the echoed paste alone, the bare-Enter recovery at the box check is never reached, and the task is silently lost while the caller records success. This is a regression vector introduced by this PR (pre-PR Codex was not probe-capable, so the box check ran and bare Enter recovered the paste). The docstring claim that the spinner shape "cannot be produced that way" is refuted. Fix direction: require the elapsed counter to be observed changing across two reads (real time-binding), require ≥2 distinct counter values, or exclude the echoed-paste region from the evidence.
  • [correctness] services/terminal_service.py:1229-1231, 1350-1366 + providers/codex.py:843-847 — A genuine turn that leaves no spinner in the buffer is now rejected where pre-PR status alone confirmed it: get_status=COMPLETED (assistant marker) passes the started check, but the spinner-only hook returns False; with the echo scrolled off, the new withhold branch suppresses the re-send; the confirm loop exhausts its 3 attempts and the caller tears down a worker that did the work. Reachable via (a) the 32 KiB rolling-buffer tail cap evicting early spinner frames on byte-heavy turns, (b) sub-spinner-time completions, (c) started-then-WAITING_USER_ANSWER with no spinner. The same shape reaches the step path (#562) as a spurious StepExecutionError(kind=timeout) for a completed step. Fix direction: treat an assistant-marker COMPLETED as sufficient proof, or make the unproven branch fall back to the pre-PR status-only verdict rather than toward teardown.

Important (should fix)

  • [correctness] services/terminal_service.py:1347-1366 — Partial-echo-then-drop silently loses the task: a paste that echoes partially before dropping leaves a non-empty post-dispatch buffer, _message_visible_in_box (first-24-char match) fails on the truncated echo, and the withhold branch then blocks the full re-send. "A dropped paste emits nothing after the dispatch" is not guaranteed. Introduced by the withhold branch.
  • [consistency] services/terminal_service.py:1347 (if probe_capable and post_dispatch_output:) — Undisclosed cross-provider behavior change: the withhold branch is provider-agnostic, so opencode and minimax full re-sends are now withheld whenever any post-dispatch bytes exist and the message is off-screen (pre-PR they re-sent). The PR body's "opencode/minimax unchanged" holds only for the hook's verdict, not the redelivery flow. Likely a safe generalization, but it should be disclosed in the PR description and pinned by a non-Codex probe-capable regression test.
  • [tests] test/services/test_deferred_submit_verification.py — No test covers the genuine no-spinner completion: every positive started=True case feeds a buffer containing the spinner. test_reply_wording_that_echoes_the_prompt_still_confirms proves only "the verdict ignores message text" — the injected _SPINNER is what makes it pass, and tracing the same frame with a realistic spinner-less buffer produces a different outcome (bare Enter via the transcript false-positive of _message_visible_in_box) that no test asserts. Add a no-spinner completed-buffer regression and re-scope/rename the fast-reply test.
  • [tests] providers/base.py:202 — The base-class default (return True, keeping opencode/minimax status-only) is load-bearing per the PR body but not pinned by any test; flipping it to False would silently regress both with a green suite. One-line pin requested.

Nits (optional)

  • [consistency] providers/codex.py:~796, ~811, ~826 — The opt-in comment and docstring spell the spinner separator as a hyphen ((4s - esc to interrupt)), but TUI_PROGRESS_PATTERN requires the bullet ; the docstring's own worked example would not match the regex. s/-/•/.
  • [correctness] providers/codex.py:844-846re.MULTILINE is a no-op (TUI_PROGRESS_PATTERN has no ^/$ anchors); harmless but implies an intent the pattern doesn't have.
  • [tests] services/terminal_service.py:1333-1338 — The get_buffer exception fallback (# noqa: BLE001) is the one changed branch with zero coverage.
  • [consistency] services/terminal_service.py _worker_is_started_direct docstring — The pre-existing paragraph says the probe uses capture-pane "NOT the 8 KB rolling buffer"; the new paragraph introduces post_dispatch_output, which is that buffer. Both true (status read vs causality read) but the juxtaposition reads as a contradiction — add a bridging clause. Relatedly, base.py's "cannot be evicted by a bounded capture tail" doesn't acknowledge that the rolling buffer is itself bounded (~32 KiB) — which is exactly the eviction path in the blocking fast-completion finding.

Tests

Well-constructed and genuinely integrative: real CodexProvider, real redeliver_dropped_message, only I/O boundaries mocked; the new terminal_service lines and the withhold branch are covered; frames satisfy the real regexes; black/isort verified clean. The gaps are targeted, not structural: no genuine no-spinner completion case (the decisive edge in this round's dispute), the base-class default unpinned, the get_buffer exception fallback uncovered, and frames are hand-authored rather than recorded captures. The "five mutations" claim was not independently re-run, but coverage is consistent with it.

Verification

Baseline: 412 passed, 3 skipped across the deferred-submit, codex/opencode/minimax provider, and status_monitor suites in the PR worktree; 2 additional terminal_service failures are sandbox-environment-only (PermissionError on ~/.aws, pass with isolated HOME) and unrelated to the PR.

  • ✓ VERIFIED — send_input empties the rolling buffer before send_keys (ordering confirmed in source; dynamic check of clear_rolling_buffer); a dropped paste with an empty buffer still re-sends at spinner gaps 12/20/28.
  • ✓ VERIFIED — a fast completion without a captured spinner is rejected: direct_probe_confirms_dispatch('› Reply with Done\n• Done\n') → False (also with realistic SGR-escaped bytes); downstream, message-off-screen yields started=False with the withhold log.
  • ✗ REFUTED — the docstring claim that an echoed paste cannot produce the spinner shape: probing with only the echoed paste of a message containing • Working (3s • esc to interrupt) returns True; worst case traced end-to-end to a swallowed-Enter task silently lost with ok=True.
  • ⁇ NOT VERIFIED — whether a real sub-second Codex turn ever commits zero spinner frames to the buffer (empirical; Codex usually paints • Working (0s…), but the 32 KiB eviction path needs no timing luck).

Verdict

Request changes — the maintainer's CHANGES_REQUESTED at this exact head stands, and both of its P1s reproduce independently: post-dispatch spinner text can falsely confirm a dropped task (demonstrated, including via the message's own echo — a task-loss regression vector new in this PR), and a spinner-less genuine completion is rejected toward worker teardown and step-path timeouts. The direction (dispatch-boundary evidence, no text matching) is right; the evidence accepted still binds time, not causality. A submission-specific signal (e.g. counter observed advancing across reads, or echo-region exclusion) plus real no-spinner fixtures would likely converge this in round 4.

@haofeif

haofeif commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

@call-me-ram do you get a chance to address comments ?

@call-me-ram

Copy link
Copy Markdown
Collaborator Author

@haofeif sorry for the gap. Here is where I have landed after re-reading this against what has happened on main and on #740 since.

What has changed since my last comment. atirna's #740 is building the thing I called option B: a pre-send transcript baseline captured strictly before send_keys, compared as parsed marker cells, with a refused dispatch when the capture cannot be obtained. It has been through five rounds with you on exactly the ownership questions this PR kept tripping over, and it closes #739 as well. Two PRs deriving a Codex acceptance signal in parallel would be the wrong use of your reviewing time, and mine is the weaker of the two.

Why I am not proposing option A either. I had described the withheld full re-send as the half that is safe on its own. gutosantos82's round found the case where it is not: a paste that echoes partially and then drops leaves a non-empty post-dispatch buffer with our text not visible, and the withhold branch then blocks the only recovery that would work, so the task is lost at the deadline instead of duplicated. That is a real trade of one failure for another rather than a fix, and it also changes opencode and minimax behaviour without a test pinning it. The dropped-paste premise "emits nothing after the dispatch" is exactly the kind of assumption my earlier tests encoded rather than checked.

Proposal. Close this PR. #659 stays open with the sharper problem statement from the earlier thread (Codex needs an acceptance signal, not an opt-in), and #740 is the vehicle for that signal; once it lands I will re-check #659 against it on real codex-cli and either close it or file what is left. If you would still rather have the withhold half landed independently, say so and I will rebuild it on current main with the partial-echo case handled and the cross-provider change disclosed and tested, but I would not do that by default.

I will close this in a few days if I do not hear otherwise, so the queue stays honest.

@haofeif

haofeif commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

@call-me-ram happy for you to proceed with your proposal

@call-me-ram

Copy link
Copy Markdown
Collaborator Author

Closing as agreed above. #740 is the vehicle for the Codex acceptance signal and #739; #659 stays open and I will re-check it against #740 on real codex-cli once that lands. The withheld-resend half can come back as its own small PR if it is still needed then.

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.

Codex assign can delete active worker when cached status lags visible screen

4 participants