Skip to content

fix(codex): ignore account notices at startup - #740

Draft
atirna wants to merge 7 commits into
awslabs:mainfrom
atirna:fix/codex-startup-notice
Draft

atirna wants to merge 7 commits into
awslabs:mainfrom
atirna:fix/codex-startup-notice

Conversation

@atirna

@atirna atirna commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Codex startup readiness and completion now classify content by per-dispatch ownership instead of by glyph.
  • Readiness is decided from the bottom-most TUI cell above the composer: a live timed spinner vetoes, an unterminated bullet is a possible mid-paint frame that must stabilize across polls, older cells above (a stale spinner or finished turn) no longer block, and sentence-terminated notice bullets are static text. A frame paused on a trailing ellipsis is treated as possibly still painting, not printed prose.
  • After a dispatch, the pane's transcript baseline is captured strictly before the send: parsed marker cells (assistant bullets and submitted user cells, above the TUI footer cutoff), never a hash of raw pane text and never derived from a post-send observation. The tmux history window and the buffer/viewport views of the same unchanged pane start at different lines once a transcript exceeds 200 rows, so the baseline is compared against every observation by marker-cell suffix: a differently bounded view of unchanged content still reads unchanged, and a dropped dispatch over a long retained transcript cannot falsely complete.
  • Baseline and observation reduce the pane through one canonical representation. The pre-send baseline is a tmux capture-pane render (literal column spacing, cells wrapped at the pane width) while status observations come from the raw byte stream (cursor-forward runs collapsed, no wrapping), so exact string equality between the two productions of one unchanged pane read as new content and disarmed the ownership gate. Marker cells are whitespace-canonicalized and compared with wrap tolerance (a cell that is a prefix of the other is a truncated view of the same line), so every view of an unchanged pane parses to the same cells. Both sides also share the same cleaner and the same footer-cutoff rule.
  • An incomplete repaint frame now carries no ownership verdict at all. A frame that has drawn the composer but not yet the status bar has no footer to bound the transcript, so it cannot tell the composer from a submitted user cell: ownership mutates only on a structurally complete, footer-bounded observation. A footerless frame can delay a verdict (the pane reads not-started), never disarm the gate; the footer-restored frame still decides, so a dropped dispatch followed by a partial repaint and the unchanged settled notice pane cannot falsely complete.
  • An equal-content reply is resolved by a monotonic post-dispatch occurrence, not by cell text. A fast later turn whose user marker has been evicted from the observation window and whose response cell equals (or is a width-truncated view of) the retained cell is invisible to text comparison, so the gate escalates exactly that ambiguous suffix match to the full pane history, where an appended turn is always visible as growth past the pre-send baseline (transcript cells only accumulate). The escalation read is rate-limited and fail-closed: a read that fails or is rate-limited keeps the gate armed, so it can only delay a verdict, never fake one.
  • A pre-send capture that fails is retried bounded inside the dispatch (a single failed tmux capture-pane is infrastructure noise, not a pane verdict). When it keeps failing, the dispatch refuses before any key is typed with a distinct retryable TerminalCaptureUnavailableError (not a generic ProviderError): inbox delivery resets the message to PENDING for the reconcile sweep instead of terminally failing it, and deferred initialization retries the delivery and leaves the initialized worker alive instead of deleting it. A pane with no retained marker cells (fresh session, login menu) is a valid empty baseline: nothing is retained, any cell observed later is new.
  • The ownership gate is consulted only at the two COMPLETED decision points, after the modal and error classifiers: an unchanged approval prompt or first-run login menu keeps WAITING_USER_ANSWER, and a pane with no new transcript content reads IDLE ("not started"), so dropped-input redelivery retries instead of accepting the task as started.

Why

Account notices use the same glyph as activity and responses, and the rendered screen survives the rolling-buffer clear at dispatch, so glyph- and dispatch-boolean-based classification both misfire (#739, follow-up review). The tmux capture window and the status observations are differently bounded, differently produced views of the same pane, so ownership must be compared on a representation that survives both; a text comparison alone cannot tell a terse new turn from retained text, which is why the ambiguous case resolves against the full history's cell count (latest follow-up review).

Verification

  • Regressions that fail on the previously reviewed head 58519bc and pass here: the unrecognized-footer partial repaint followed by the unchanged settled notice pane (was permanently disarming the gate, then falsely COMPLETED), the equal-content and prefix-content evicted-marker second turns on the raw and pyte paths (were staying IDLE forever: full resend or sync timeout), the persistent pre-send capture failure (was a generic ProviderError the consumers terminally failed), and the flaky-capture-recovered-inside-the-dispatch case (was refusing a valid delivery).
  • Consumer-path coverage for the retryable refusal: inbox deliver_pending resets the message to PENDING (never FAILED); deferred init retries a transient refusal and delivers with no failure notification, and a persistent refusal leaves the worker alive (delete_worker=False) with the caller told the task was not delivered.
  • The escalation read is rate-bounded: repeated ambiguous polls of an unchanged pane fork at most one capture-pane per 3s interval (loop-level bound asserted).
  • The prior-round regressions (unchanged >200-row retained transcript raw and pyte views, render-baseline vs raw-observation representation, capture failure followed by visible-marker and evicted-marker completion, fast completion on first observation, retry-after-refusal, approval/login precedence, draft-in-composer, footerless partial repaint) are retained and still pass.
  • uv run pytest test/providers/test_codex_provider_unit.py --no-cov: 299 passed, 3 skipped.
  • uv run pytest test/providers/ --no-cov: 1557 passed, 3 skipped, 7 deselected, 1 xfailed.
  • uv run pytest test/services/ --no-cov: full services suite numbers recorded in the thread.
  • uv run black --check src test / uv run isort --check-only src test: clean. uv run mypy on the four changed source files: clean.

Fixes #739

@codecov-commenter

codecov-commenter commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.66197% with 9 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@79befd0). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/cli_agent_orchestrator/providers/codex.py 92.68% 9 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #740   +/-   ##
=======================================
  Coverage        ?   91.61%           
=======================================
  Files           ?      204           
  Lines           ?    28791           
  Branches        ?        0           
=======================================
  Hits            ?    26376           
  Misses          ?     2415           
  Partials        ?        0           
Flag Coverage Δ
unittests 91.61% <93.66%> (?)

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 exact head 8120151f59ac2c86ae28008171d78f8bd74a004f against base bab1faf2e62843ef65c7f20e99d32cf524258df9. The account-notice change introduces two startup-state blockers:

  1. [P1] src/cli_agent_orchestrator/providers/codex.py:1403-1422 — retained startup content can complete the first dispatched task before it starts. _task_dispatched records only that some input was dispatched; it does not prove that a visible assistant bullet belongs to that dispatch. clear_rolling_buffer() retains the pyte-rendered screen, so after mark_input_received() a control-only redraw can reclassify the unchanged account-notice bullet from IDLE to COMPLETED before Codex renders the new user turn. Stale adjacent / cells can also satisfy the ungated branch before dispatch. Completion consumers then accept the notice as current-turn completion, defeating dropped-input retries or tearing down a still-starting task. Reproduced with retained ANSI notice content and with stale adjacent user/assistant cells. Replace the ever-dispatched boolean with per-dispatch response ownership: record a screen/cell generation baseline at dispatch and require new current-turn content or an observed current-turn user/processing transition before COMPLETED. Apply it to both visible- and evicted-user-marker paths.

  2. [P2] src/cli_agent_orchestrator/providers/codex.py:674-696 — startup readiness is tied to one exact spinner rendering. Replacing the generic bullet veto with TUI_PROGRESS_PATTERN treats a partial live repaint such as • Working as idle-ready, while an old timed spinner above a newer notice/composer still forces PROCESSING. Reproductions returned ready for the partial live frame and not-ready for the stale spinner/current composer frame. Determine readiness from the bottom-most live TUI cell/modal associated with the current composer, ignore older cells above it, and require unknown/partial frames to stabilize. Restore the removed partial-frame regression and cover ANSI notice variants, stale timed activity, adjacent stale turns, and multi-poll stabilization.

These share one root cause: historical rendered text is being classified without current-turn/live-cell ownership. Please fix that invariant rather than adding notice- or phrase-specific exceptions. Focused startup validation passed 25 tests with 244 deselected; all current GitHub checks are green.

atirna added a commit to atirna/cli-agent-orchestrator that referenced this pull request Sep 6, 2026
Review follow-up on awslabs#740: readiness now keys on the bottom-most TUI cell
above the composer (live spinner vetoes; unterminated bullets must settle;
older cells no longer block), and a dispatch baseline recorded at
mark_input_received keeps retained notices or prior completions from
completing the new turn on both user-marker paths.
@atirna

atirna commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

pushed a rework for both findings, same invariant underneath

  • dispatch now records a content baseline at mark_input_received (tail hash of the glyph lines, same idea as the claude_code [Bug] terminal_service.send_input: stale cached COMPLETED satisfies the next wait_until_status immediately (returns previous turn's output) #407 guard). while the pane is unchanged from that baseline status stays PROCESSING, so a retained notice or a prior turn's completion cant satisfy either completion branch. once the pane differs the guard disarms and the turn's own evidence decides
  • readiness reads only the bottom-most cell above the composer. a timed spinner still vetoes, an unterminated bullet (mid paint like "• Working") vetoes until two polls see the identical frame, older cells above (stale spinner, finished turn) are ignored, and sentence terminated notice bullets are static text
  • restored the partial-frame case that got dropped and added the variants you listed: ansi notice variants, stale timed activity above a newer composer, adjacent stale turns, multi poll stabilization

focused suite green, black/isort/mypy clean

@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 8b20ff92155a5f355fa7f76af338647c959e3d08 against base dc5efb3179742bed5bca7f0c17f2a4b5d52a9f4c. The spinner/live-bottom classifier blocker is fixed, but the dispatch-ownership delta still has two blockers:

  1. [P1] src/cli_agent_orchestrator/providers/codex.py:1409-1413 — draft text disarms ownership and lets a retained notice complete a dropped submission. A successful paste with swallowed Enter changes the composer, so the pane hash differs from the dispatch baseline even though no user transcript, processing, or response exists. The typed composer is excluded as footer chrome, while the retained notice then satisfies the evicted-user completion branch. Exact-head reproduction returned COMPLETED with only an unsubmitted draft, causing deferred delivery to accept the task as started. A transient baseline-capture failure also leaves None and immediately restores the unsafe completion behavior. Track assistant/transcript ownership, not arbitrary pane mutation: require a newly submitted user cell plus response, a new assistant cell attributable to the dispatch, or observed current-turn processing. A typed composer must not disarm the gate, and failed baseline capture must fail closed. Cover dropped Enter over a retained notice and baseline-read failure while preserving genuine visible/evicted completion cases.

  2. [P2] src/cli_agent_orchestrator/providers/codex.py:1402-1413 — the ownership gate masks unresolved approval and login prompts. It runs before modal/error classifiers, so an unchanged approval/login pane after a dropped prompt answer changes from WAITING_USER_ANSWER to PROCESSING; another answer_user_prompt call is then rejected because the terminal no longer reports waiting. Reproduced for both numbered approval and first-run login, and the prior head preserved waiting. Apply the ownership gate only to COMPLETED decisions, or distinguish task dispatches from prompt answers; unchanged blocking modals must retain classifier precedence and remain WAITING_USER_ANSWER. Test both modal types through the status-monitor path.

The prior exact-spinner P2 is resolved across alternate ANSI notices, stale/current spinners, adjacent turns, and partial-frame stabilization. The complete Codex suite passed 277 tests with 3 skipped. No current-head CI jobs ran; workflows are action_required.

atirna added a commit to atirna/cli-agent-orchestrator that referenced this pull request Sep 6, 2026
Review follow-up on awslabs#740: the dispatch baseline now fingerprints the
pane's transcript (assistant bullets and submitted user cells, above the
TUI footer cutoff) instead of the whole pane, so a typed composer draft
or a failed baseline capture can no longer disarm it — capture failure
fails closed by arming from the first post-dispatch observation. The
gate moved from the top of get_status to the two COMPLETED decision
points, so unchanged approval and login prompts keep their
WAITING_USER_ANSWER precedence instead of collapsing to PROCESSING,
and a pane with no new transcript content reads IDLE (not started),
letting dropped-input redelivery retry.
@atirna

atirna commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Pushed a rework for both blockers in 301f34c.

  • P1: the dispatch baseline now fingerprints the pane's transcript, assistant bullets and submitted user cells, above the TUI footer cutoff, instead of the whole pane. A typed draft lives inside the footer cutoff, so it can't change the signature: a paste with swallowed Enter keeps the pane not-started (IDLE), and the redelivery loop retries. A failed baseline capture fails closed by arming from the first post-dispatch observation, so retained content still can't complete.
  • P2: the ownership gate moved from the top of get_status to the two COMPLETED decision points, after the modal classifiers. An unchanged approval prompt or login menu keeps WAITING_USER_ANSWER (verified through both the buffer and get_status_from_screen paths).

All four new regression cases (draft disarm, fail-closed capture, approval precedence, login precedence) fail on 8b20ff9 and pass on 301f34c; full Codex suite 281 passed, deferred-init and step consumers 71 passed, black/isort/mypy clean.

@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 exact head 301f34c672b75a4008f29cca5b03ff8515886de1 against base dc5efb3179742bed5bca7f0c17f2a4b5d52a9f4c.

P1 — dispatch ownership still has no reliable canonical pre-send baseline. The current fix has two opposing production failure modes:

  1. The baseline is captured from tmux history, while normal and stale-status observations use differently bounded pyte/visible-pane views. With more than 200 rows of retained multi-line turns, the last marker cells differ solely because the windows truncate at different boundaries. An unchanged retained response is then treated as newly owned, so a dropped dispatch is falsely accepted as COMPLETED.
  2. If baseline capture fails, the first post-dispatch observation is adopted as the baseline. When that first observation is a genuine fast completion, it remains IDLE on later polls; deferred confirmation can retry and fully resend an already-completed task, while synchronous completion can time out.

The typed-draft case and approval/login precedence are fixed. The remaining root fix is to establish ownership from one canonical pre-send transcript representation used by every status path, compare parsed marker-cell suffixes with truncation awareness, and fail the dispatch before sending if that pre-send capture cannot be obtained. Do not derive a baseline from a post-send observation. Add raw and pyte regressions for an unchanged >200-row retained transcript and for baseline failure followed immediately by visible-marker and evicted-marker completion; neither path may falsely complete, resend, or time out.

The full Codex suite passed 281 tests with 3 skipped, focused coverage passed 97 with 1 skipped, and consumer coverage passed 71. Current-head workflows are action_required with zero jobs, so no current-head CI result validates this revision.

atirna added a commit to atirna/cli-agent-orchestrator that referenced this pull request Sep 7, 2026
…baseline

Review follow-up on awslabs#740 (5126947959): the transcript baseline is now
parsed marker cells (assistant bullets and submitted user cells above
the TUI footer cutoff) captured strictly before send_keys, never a hash
of raw pane text and never derived from a post-send observation.

- Boundary-aware ownership: the tmux history window and the
  buffer/viewport views of the same unchanged pane start at different
  lines once a transcript exceeds 200 rows, so the old raw signature
  read a retained transcript's shorter observation as new content and
  falsely completed a dropped dispatch. Marker cells are compared by
  suffix, so a differently bounded view of unchanged content still
  reads unchanged.
- Fail before send: a pre-send history read that raises refuses the
  dispatch with ProviderError before any key is typed, so the message
  is retried by callers instead of sending unowned output. The previous
  lazy fail-closed armed from the first post-dispatch observation, which
  latched a genuine fast completion at IDLE and caused deferred
  redelivery to resend the finished task.
- A pane with no retained marker cells (fresh session, login menu) is a
  valid empty baseline: nothing is retained, any cell observed later is
  new content this dispatch owns.

Raw and pyte regressions cover the >200-row retained transcript, the
capture-failure refusal followed immediately by visible-marker and
evicted-marker completion, fast completion on the first observation,
and the prior draft/notice/second-dispatch/modal-precedence guards.
@atirna

atirna commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

pushed 3b4c885, reworked the baseline per the review

  • the baseline is now parsed marker cells (assistant bullets + submitted user cells above the footer cutoff) captured strictly before send_keys, not a raw pane hash. tmux history and the buffer/viewport views cut the same unchanged pane at different boundaries past 200 rows, so comparisons are suffix-aware on the parsed cells: a shorter observation of unchanged retained content reads unchanged, and the dropped-dispatch case over a long transcript no longer completes
  • a failed pre-send capture refuses the dispatch (ProviderError from mark_input_received, right before send_keys) so the message is retried instead of sent unowned. no baseline is ever derived post-send now, so the fast-completion case reports COMPLETED on the first observation instead of latching IDLE and getting resent
  • regressions added: unchanged >200-row retained transcript (raw + pyte views), capture failure followed immediately by visible-marker and evicted-marker completion, fast completion on first observation, retry-after-refusal. all fail on 301f34c and pass on this head

full provider suite 1544 passed, consumers 111 passed, black/isort/mypy clean

@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: #740 — fix(codex): ignore account notices at startup

Head reviewed: 3b4c88502d87cce1d3e1fa4d9fb09a21698a1ff4 (2 files, +783/−13; src/cli_agent_orchestrator/providers/codex.py + test/providers/test_codex_provider_unit.py). Fixes #739. Third revision after three CHANGES_REQUESTED rounds from @haofeif.

Summary

This head substantially delivers what the maintainer's latest review demanded: the dispatch baseline is now captured strictly pre-send from parsed marker cells, a capture failure refuses the dispatch with ProviderError before send_keys (dynamically verified — the mocked-backend spot-check confirmed send_keys is never called on refusal, and terminal_service.py:2326-2328 confirms the real call ordering), the ownership gate sits after all modal/error classifiers and only downgrades COMPLETED, and the demanded raw+pyte >200-row and capture-failure-then-both-completion-paths regressions exist and are load-bearing (4 of 5 fail on prior head 301f34c6). Test-count claims in the PR body reproduce almost exactly (287 passed / 3 skipped codex unit; 1545 vs claimed 1544 on the full providers run — immaterial drift; 111 services consumers).

However, the core of the maintainer's P1 — "one canonical pre-send transcript representation used by every status path" — is only partially met: the baseline and the observations are extracted by a shared rule but from two differently-produced text sources, and two reviewers independently found concrete windows where that asymmetry re-opens the dropped-dispatch-falsely-COMPLETED failure mode. Combined with one PR-body inaccuracy (one "new regression" already passes on the prior head) and an untested stabilization loop, this warrants another round.

Critical (blocking)

  • [conversation] Standing human CHANGES_REQUESTED; zero CI at this head. @haofeif's third review (at 301f34c6) remains undismissed; reviewDecision=CHANGES_REQUESTED, mergeStateStatus=BLOCKED, and check-runs at 3b4c885 total 0 (action_required, no jobs) — no current-head CI validates this revision. The maintainer's cadence is to re-verify personally at exact head; any verdict we post cannot clear that block. Not a code defect, but it caps the achievable verdict regardless of findings below.

Important (should fix)

  • [correctness] codex.py:1422 (baseline capture) vs :1659/base.py:452 (observation) — baseline and observation are two different capture representations. The baseline comes from a tmux capture-pane render; the status observation comes from the raw FIFO buffer passed through strip_terminal_escapes (which collapses cursor-forward layout to single spaces and converts cursor moves to newlines). _is_suffix requires exact per-cell string equality (rstrip only). Shared extraction rule, unshared sources — any marker line Codex lays out with cursor positioning, or that tmux width-wraps differently than the raw stream, yields divergent cell strings, so unchanged content reads as non-suffix, the gate disarms, and a dropped dispatch can falsely COMPLETE. This is precisely the "one canonical representation" the maintainer demanded, and it is untested: every new test feeds the identical string to both sides.

  • [correctness] codex.py:778/:794/:1450-1481 — footer-cutoff asymmetry lets a post-clear partial repaint permanently disarm the ownership gate. The composer line matches _USER_CELL_RE and is excluded only when TUI_FOOTER_PATTERN appears in the last 5 lines. The rolling buffer is cleared at dispatch, so an early poll can observe a partial repaint with the composer present but the status bar not yet rendered → cutoff = full text → composer becomes a marker cell absent from the baseline → not a suffix → _dispatch_pending set False permanently for that dispatch. Every later well-formed frame then bypasses the gate, and a retained notice/prior-turn bullet satisfies the evicted-marker COMPLETED branch — the exact regression this fix targets. Narrow window, permanent damage per dispatch.

  • [correctness] codex.py:821/:1685 — identity-based ownership misses a real completion whose marker cells coincide with a baseline suffix. A terse reply ("• Done.") equal to the trailing baseline cell(s) reads as retained → IDLE → sync timeout / redelivery resend on repeated identical tasks. Residual gap inherent to content-identity ownership; worth acknowledging in the code or PR body even if accepted.

  • [tests] test_codex_provider_unit.py — the mid-paint stabilization loop is untested at loop level. The decision (previous_tail_signature compare in _handle_trust_prompt, codex.py~1301) is only exercised by calling the helper directly with allow_partial_activity_cell=True; no async test feeds two identical partial-activity tails on consecutive polls and asserts the loop exits ready. A broken signature update would time out while all current tests still pass.

  • [verification] PR-body accuracy — one of the five named "new regressions" does not discriminate base from patch. test_fast_completion_on_first_observation_completes already passes on 301f34c6 (base also captures a pre-send baseline when get_history succeeds). The other four genuinely fail on base and pass here. The blanket claim "new regressions fail on the previously reviewed head 301f34c" should be corrected — this maintainer verifies claims literally.

Nits (optional)

  • [consistency] codex.py:1437 vs :1506 — baseline cleans with strip_terminal_escapes(re.sub(ANSI_CODE_PATTERN, ...)) (two-step) while the observation uses one-step strip_terminal_escapes. Equivalent today (SGR subset), but two cleaners on the two sides of the load-bearing comparison is latent drift risk; share one cleaner.
  • [consistency] codex.py~1405mark_input_received docstring claims a union of "BOTH history bounds"; code captures scrollback with a visible-only fallback when empty. Reword.
  • [consistency] base.py~308 (not in diff, made load-bearing by this PR) — base-contract docstring says mark_input_received is called "after send_input() delivers"; it is called pre-send_keys, and this PR's correctness now depends on that timing. A one-line correction hardens the interface against a future reorder silently reintroducing the bug.
  • [security] codex.py _is_suffix — pane-width re-wrap between baseline and observation disarms the gate (same misclassification class as #739); low exploitability, approval precedence unaffected.
  • [correctness] codex.py:147STARTUP_STATIC_BULLET_PATTERN treats a bullet ending in /... as static with no stabilization wait; a live ellipsis frame could declare readiness mid-paint.
  • [correctness] codex.py:1246~1300previous_tail_signature updates only after has_idle, so "consecutive polls" may compare non-adjacent polls; two sub-second polls catching the same pre-first-tick fragment could pass prematurely.
  • [tests] test:2557 — pyte test asserts after the get_backend patch block closes; safe today, latently fragile. [tests] test:2530 — >200-row test models full-transcript baseline vs short viewport rather than the literal 200-row window boundary; add a comment or resize.
  • [conventions] — bare list annotations on new helpers match the file's existing convention and mypy is clean; leave as-is.

Tests / dynamic verification

  • uv run pytest test/providers/test_codex_provider_unit.py --no-cov → 287 passed, 3 skipped (matches claim; needs CAO_HOME_DIR override in this sandbox).
  • uv run pytest test/providers/ --no-cov → 1545 passed, 3 skipped, 7 deselected, 1 pre-existing unrelated xfail (claim said 1544; nothing failed).
  • Services consumers (4 files) → 111 passed (matches claim).
  • Base-head check at 301f34c6 (test file transplanted): 4/5 named regressions fail on base with the expected COMPLETED-vs-IDLE assertion; all 5 pass at head. Throwaway worktree removed.
  • Behavioral spot-check: mocked get_history raise → ProviderError raised, send_keys never called; successful capture → send proceeds.
  • Conventions gates run directly on the worktree: black/isort/mypy all clean.

Verdict rationale

Request changes. The engineering direction is right and most of the maintainer's demands are demonstrably met, but the two Important capture-representation findings are concrete re-openings of the exact failure mode under review (and untested), the stabilization loop lacks loop-level coverage, and the PR body overstates one regression's discriminating power. Independently, an approve is procedurally unavailable: the human CHANGES_REQUESTED stands undismissed and no CI has validated this head. Mission fit is core — reliable dispatch-status detection is the substrate of orchestration — with a non-blocking strategic note that this is the third TUI-heuristic fix in this provider and a shared TUI-classification abstraction would amortize the recurring cost.

atirna added a commit to atirna/cli-agent-orchestrator that referenced this pull request Sep 7, 2026
Review follow-up on awslabs#740: the dispatch baseline now fingerprints the
pane's transcript (assistant bullets and submitted user cells, above the
TUI footer cutoff) instead of the whole pane, so a typed composer draft
or a failed baseline capture can no longer disarm it; capture failure
fails closed by arming from the first post-dispatch observation. The
gate moved from the top of get_status to the two COMPLETED decision
points, so unchanged approval and login prompts keep their
WAITING_USER_ANSWER precedence instead of collapsing to PROCESSING,
and a pane with no new transcript content reads IDLE (not started),
letting dropped-input redelivery retry.
atirna added a commit to atirna/cli-agent-orchestrator that referenced this pull request Sep 7, 2026
…baseline

Review follow-up on awslabs#740 (5126947959): the transcript baseline is now
parsed marker cells (assistant bullets and submitted user cells above
the TUI footer cutoff) captured strictly before send_keys, never a hash
of raw pane text and never derived from a post-send observation.

- Boundary-aware ownership: the tmux history window and the
  buffer/viewport views of the same unchanged pane start at different
  lines once a transcript exceeds 200 rows, so the old raw signature
  read a retained transcript's shorter observation as new content and
  falsely completed a dropped dispatch. Marker cells are compared by
  suffix, so a differently bounded view of unchanged content still
  reads unchanged.
- Fail before send: a pre-send history read that raises refuses the
  dispatch with ProviderError before any key is typed, so the message
  is retried by callers instead of sending unowned output. The previous
  lazy fail-closed armed from the first post-dispatch observation, which
  latched a genuine fast completion at IDLE and caused deferred
  redelivery to resend the finished task.
- A pane with no retained marker cells (fresh session, login menu) is a
  valid empty baseline: nothing is retained, any cell observed later is
  new content this dispatch owns.

Raw and pyte regressions cover the >200-row retained transcript, the
capture-failure refusal followed immediately by visible-marker and
evicted-marker completion, fast completion on the first observation,
and the prior draft/notice/second-dispatch/modal-precedence guards.
@atirna
atirna force-pushed the fix/codex-startup-notice branch from 3b4c885 to 58519bc Compare September 7, 2026 10:12
@atirna

atirna commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

pushed 58519bc, rework for the capture-representation findings

  • baseline and observations now reduce to one canonical cell representation. the render view carries literal column spacing while the raw stream collapses cursor-forward runs, so cells canonicalize whitespace on both sides, and a cell compares equal when one is a prefix of the other (capture-pane wraps at the pane width, the raw stream does not). both sides also share the same single cleaner and footer-cutoff rule now. the new regression feeds the rendered string to the pre-send baseline and the raw string to the observation; on 3b4c885 it returned COMPLETED for a dropped dispatch, here it stays IDLE.
  • the post-clear partial repaint can no longer disarm the gate. with no footer drawn yet, a user-marker line that is the last content line of the pane is the composer in that frame, so it is dropped instead of becoming a baseline cell. regression covers the early poll plus the later settled frames, both were falsely COMPLETED before.
  • the stabilization loop has loop-level coverage now: an async test feeds two identical partial-activity tails on consecutive polls and asserts the loop exits ready. the tail signature also updates on every poll so the comparison is always between adjacent observations, and an ellipsis frame is no longer treated as printed prose.
  • PR body corrected: the fast-completion case passes on the earlier head too (it was preservation coverage), and the body now separates which regressions discriminate this head from 3b4c885 from which are retained preservation cases.
  • the terse-reply residual is acknowledged in the code and the body: a reply byte-equal to the retained baseline tail stays IDLE until the next differing frame, it delays a completion rather than faking one.

all suites green on this head: 291 codex unit, 1549 providers, 111 consumers, black/isort/mypy clean

@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 exact head 58519bc9c21dc0cb19bb5083ea87ff4a7a643e19 against base 79befd0fcca6534f8cf0f606c7213a478c3f318d. The canonical representation fixes the prior cross-window mismatch for complete snapshots, but the frozen ownership blocker is not fully resolved.

P1 — cell-content comparison still cannot establish a new dispatch occurrence

providers/codex.py:836-864,1493-1527 still fails in two opposite supported cases. First, a partial repaint with the composer followed by a not-yet-recognized footer row retains that composer as a transcript cell and permanently disarms _dispatch_pending; when the unchanged notice pane settles, retained content becomes COMPLETED for a dropped task. Second, a fast later turn whose user marker is evicted and whose assistant response equals—or is a prefix of—the prior terse response remains IDLE forever because equal/prefix cells are deliberately classified as unchanged. Deferred delivery then fully resends an already-completed task, while synchronous completion times out.

Do not mutate ownership from incomplete/unbounded repaint frames. Require a structurally complete observation or explicit current-turn evidence, and use a monotonic post-dispatch transcript occurrence/generation for ambiguous equal-content responses rather than response text alone. Add raw and pyte regressions for an unrecognized partial footer followed by the unchanged settled notice, plus second-turn equal/prefix completions with evicted user markers through both deferred and synchronous consumers.

P2 — pre-send capture refusal is terminally failed instead of retried

Failing before send_keys correctly avoids inventing a post-send baseline, but the generic ProviderError is not retryable in the actual consumers. Inbox delivery transitions the message from DELIVERED to FAILED and reconciliation retries only PENDING; deferred initialization notifies failure and deletes the healthy worker. A transient pane-capture error therefore permanently loses or rejects an otherwise valid delivery even though no input was sent.

Retry the capture with a bounded policy inside dispatch or raise a distinct retryable error. Inbox delivery should restore that case to PENDING, and deferred initialization should retry without deleting the worker. Cover both consumer paths, not only the provider method.

The >200-row history/pyte/visible mismatch, typed draft, modal precedence, baseline self-adoption, and ordinary visible/evicted completion cases are fixed. The full Codex suite passed 291 tests with 3 skipped; consumer coverage passed 111. Current-head workflows are action_required with zero jobs, so no CI execution validates this revision. No P3 findings remain.

Review follow-up on awslabs#740: readiness now keys on the bottom-most TUI cell
above the composer (live spinner vetoes; unterminated bullets must settle;
older cells no longer block), and a dispatch baseline recorded at
mark_input_received keeps retained notices or prior completions from
completing the new turn on both user-marker paths.
Review follow-up on awslabs#740: the dispatch baseline now fingerprints the
pane's transcript (assistant bullets and submitted user cells, above the
TUI footer cutoff) instead of the whole pane, so a typed composer draft
or a failed baseline capture can no longer disarm it; capture failure
fails closed by arming from the first post-dispatch observation. The
gate moved from the top of get_status to the two COMPLETED decision
points, so unchanged approval and login prompts keep their
WAITING_USER_ANSWER precedence instead of collapsing to PROCESSING,
and a pane with no new transcript content reads IDLE (not started),
letting dropped-input redelivery retry.
…baseline

Review follow-up on awslabs#740 (5126947959): the transcript baseline is now
parsed marker cells (assistant bullets and submitted user cells above
the TUI footer cutoff) captured strictly before send_keys, never a hash
of raw pane text and never derived from a post-send observation.

- Boundary-aware ownership: the tmux history window and the
  buffer/viewport views of the same unchanged pane start at different
  lines once a transcript exceeds 200 rows, so the old raw signature
  read a retained transcript's shorter observation as new content and
  falsely completed a dropped dispatch. Marker cells are compared by
  suffix, so a differently bounded view of unchanged content still
  reads unchanged.
- Fail before send: a pre-send history read that raises refuses the
  dispatch with ProviderError before any key is typed, so the message
  is retried by callers instead of sending unowned output. The previous
  lazy fail-closed armed from the first post-dispatch observation, which
  latched a genuine fast completion at IDLE and caused deferred
  redelivery to resend the finished task.
- A pane with no retained marker cells (fresh session, login menu) is a
  valid empty baseline: nothing is retained, any cell observed later is
  new content this dispatch owns.

Raw and pyte regressions cover the >200-row retained transcript, the
capture-failure refusal followed immediately by visible-marker and
evicted-marker completion, fast completion on the first observation,
and the prior draft/notice/second-dispatch/modal-precedence guards.
…tation

The pre-send baseline came from a tmux capture-pane render while status
observations come from the raw stream, so literal spacing and wrap
differences read as new content and disarmed the ownership gate; cells
now canonicalize whitespace and compare with wrap tolerance. A partial
repaint that has not drawn the status bar yet no longer turns the
composer hint into a baseline cell: a user-cell line that is the last
content line of a footerless pane is the composer in that frame and is
dropped. Startup stabilization now compares adjacent polls only, and a
trailing ellipsis no longer reads as printed prose.
@atirna
atirna force-pushed the fix/codex-startup-notice branch from 58519bc to 04b00f0 Compare September 7, 2026 14:48
An incomplete repaint frame (composer drawn, status bar not yet) cannot
tell the composer from a submitted user cell, so it now carries no
ownership verdict: the gate mutates only on a structurally complete,
footer-bounded observation, and a footerless frame can delay a verdict
but never disarm the gate. The dropped-dispatch-then-partial-repaint-
then-unchanged-notice sequence can no longer falsely complete.

An equal- or prefix-content reply whose user marker was evicted from the
observation window was invisible to the cell-text comparison and stayed
IDLE forever (full resend / sync timeout). The ambiguous suffix match
now escalates to the full pane history, where an appended turn is always
visible as growth past the pre-send baseline. The escalation read is
rate-limited (3s) and fail-closed.

The pre-send capture refusal retries bounded inside the dispatch, then
raises a distinct retryable TerminalCaptureUnavailableError instead of a
generic ProviderError: inbox delivery resets the message to PENDING for
the reconcile sweep, and deferred initialization retries the delivery
and leaves the initialized worker alive instead of deleting it.
@atirna

atirna commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

pushed 69c6461, both blockers addressed

  • incomplete frames now carry no ownership verdict at all: the gate only mutates on a footer-bounded observation, so the partial repaint with the unrecognized footer row keeps it armed and the settled unchanged notice pane still reads IDLE for the dropped task (raw + pyte regressions; both fail on 58519bc)
  • ambiguous equal/prefix cells resolve against the full pane history instead of cell text: transcript cells only accumulate, so an appended turn is visible there as growth past the pre-send baseline. the escalation read is rate-limited (one capture-pane per 3s while a pane stays ambiguous) and fail-closed, it can only delay a verdict. second-turn equal and prefix completions with evicted user markers now report COMPLETED through the raw and pyte paths, covered on both the deferred and synchronous consumer shapes
  • the capture refusal now retries bounded inside the dispatch first, then raises a distinct retryable TerminalCaptureUnavailableError instead of the generic ProviderError: inbox delivery resets the message to PENDING for the reconcile sweep, and deferred init retries the delivery and leaves the worker alive instead of deleting it. both consumer paths have their own tests now

the new regressions all fail on 58519bc for the named modes and pass on this head. codex unit suite 299 passed / 3 skipped, providers 1557 passed, services suite unchanged from base. also rebased onto current main (79befd0) since the branch was 3 behind, content identical apart from that.

@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 69c64611395278d73778355a19914735eabb5bb2 against base 79befd0fcca6534f8cf0f606c7213a478c3f318d, scoped to the frozen findings and direct remediation regressions. The pre-send capture-refusal P2 is fixed. One consolidated dispatch-ownership P1 remains; there are no additional independent blockers.

P1 - history escalation still does not establish current-dispatch completion ownership

src/cli_agent_orchestrator/providers/codex.py:1565-1571,1602-1610 reopens the ownership failure through the new escalation path:

  1. The ordinary observation requires a recognized footer, but _full_history_owns() independently captures and parses another frame without that completeness guard. An unrecognized-footer repaint can therefore count the composer as a transcript cell, permanently clear _dispatch_pending, and let a retained notice complete a dropped task. The previous head remains IDLE for the reproduced complete-observation/partial-escalation interleaving; this head reports COMPLETED on both raw and pyte paths.
  2. Ownership and completion can come from different snapshots. An escalation containing a newly submitted, still-processing turn authorizes the completed response in the older observation passed to get_status(). The old observation reports COMPLETED even though classifying the new capture reports PROCESSING.
  3. The purported full-history capture is still bounded: both pre-send capture and escalation omit full_history=True. The actual tmux contract (clients/tmux.py:1213-1221) defaults to 200 scrollback rows plus the viewport. When current-turn markers leave that window, a later equal/prefix response still matches the baseline and remains IDLE, including repeated polls beyond the escalation interval. The reproduction exercised real backend/client flag selection rather than making every mocked capture return an unlimited transcript.

These have supported consumer consequences: deferred confirmation accepts the dropped task in the false-completion case and attempts a full resend in the missed-completion case; synchronous completion accepts the false completion or times out. The synchronous path does not fully resend here because Codex does not enable supports_direct_status_probe; no missing capability opt-in is being requested.

Fix the shared ownership invariant: validate every ownership capture, derive ownership and completion from the same validated snapshot, and establish a pre-send occurrence boundary that remains meaningful when the observation window moves. At minimum, baseline and escalation must use the same explicit full-history contract; changing only escalation would introduce another baseline mismatch. Account for history truncation/reset rather than treating suffix inequality alone as proof of a new turn. A Codex/backend-local mechanism is sufficient; this does not require unsupported extraction from other providers. Cover partial-escalation and old-response/new-processing interleavings, equal/prefix completions beyond the real capture window, and both consumer helpers while retaining ordinary distinct-response completion.

Frozen P2 - fixed

The bounded capture retries and distinct TerminalCaptureUnavailableError now preserve delivery semantics: inbox refusal returns to PENDING; deferred initialization retries and preserves the worker if refusal persists. A transient synchronous refusal recovered and sent exactly once; persistent refusal captured three times, sent zero times, and surfaced the distinct error. That explicit refusal is not silent delivery loss and does not justify retaining P2.

The focused existing suite passed 24 tests, with additional hermetic raw/pyte, capture-contract, and consumer reproductions. No live provider was contacted, so repaint frequency was not measured. The earlier canonical complete-window, draft, and modal-precedence fixes remain intact. Current-head workflows are action_required with zero jobs, not passing CI.

The escalation captured and parsed its own frame with no completeness
guard, so an unrecognized-footer repaint counted the composer as a
transcript cell and cleared the gate, letting a retained notice complete a
dropped task. Ownership and completion could also come from different
frames: an escalation holding a newly submitted, still-processing turn
authorized the completed response in the older observation.

get_status now widens the observation once, before classifying, and uses
that single snapshot for both. An incomplete, failed or rate-limited
capture leaves the original view in place and the gate armed, so it can
only delay a verdict.

Both reads also pass full_history=True. get_history defaults to
TMUX_HISTORY_LINES, so the previous full-history read was 200 lines.
@atirna

atirna commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

all three are real, pushed 3187d3f.

  1. _full_history_owns() captured and parsed its own frame with no completeness check, so an unrecognized-footer repaint counted the composer as a transcript cell and cleared the gate.
  2. ownership and completion did come from different snapshots: the escalation decided ownership from its capture while get_status() completed from the original clean_output.
  3. get_history() defaults to TMUX_HISTORY_LINES (200), so the full-history read was never full.

rather than guard the escalation separately, get_status now widens the observation once before classifying and both verdicts read that same snapshot:

clean_output = self._completion_observation(strip_terminal_escapes(output))

_completion_observation returns the widened view only when it is footer-bounded; a failed, incomplete or rate-limited capture returns the original and leaves the gate armed, so it can still only delay a verdict. both the baseline read and the escalation pass full_history=True now, otherwise older cells exposed by widening would look new. the 3s rate limit is unchanged.

5 regressions, unrecognized footer and newer-processing-frame across buffer and screen modes plus the full-scrollback case. all 5 fail on 69c6461:

FAILED ...escalation_classifies_one_complete_snapshot[unrecognized-footer-buffer]
FAILED ...escalation_classifies_one_complete_snapshot[unrecognized-footer-screen]
FAILED ...escalation_classifies_one_complete_snapshot[newer-processing-frame-buffer]
FAILED ...escalation_classifies_one_complete_snapshot[newer-processing-frame-screen]
FAILED ...escalation_reads_full_scrollback_on_both_sides_of_dispatch
5 failed, 302 deselected

full codex provider suite on this head: 304 passed, 3 skipped. production side is net -15 lines, the old _full_history_owns boolean and its comment block are gone.

@atirna
atirna marked this pull request as draft September 17, 2026 07:42
@atirna

atirna commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

history truncation can remove the turn boundary, leaving a repeated response stuck at idle. marked this draft until completion tracking can distinguish that case without relying on retained text.

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.

[Bug] Codex startup never completes when the TUI shows an account notice bullet (codex-cli 0.153.2)

4 participants