Skip to content

fix(agent): recover bounded Codex false stops across runtimes - #83560

Open
CryptoKylan wants to merge 4 commits into
NousResearch:mainfrom
CryptoKylan:fix/codex-false-stop-upstream-refresh-20260811
Open

CryptoKylan wants to merge 4 commits into
NousResearch:mainfrom
CryptoKylan:fix/codex-false-stop-upstream-refresh-20260811

Conversation

@CryptoKylan

@CryptoKylan CryptoKylan commented Aug 11, 2026

Copy link
Copy Markdown

Problem

Hermes can accept a provider-returned progress checkpoint as a completed turn even when the response explicitly promises more work or says the task remains unfinished. This can stop tool-driven tasks prematurely.

The existing recovery paths do not consistently cover:

  • post-tool progress checkpoints;
  • verification or synthetic rounds sharing one human-turn budget;
  • Codex Responses and Codex app-server parity;
  • native app-server completion identity and stale completion notifications;
  • persistence and restoration ordering after bounded recovery.

Changes

  • Add a shared pure terminal-continuation policy for Codex runtimes.
  • Cap recovery at two automatic continuations per original human turn; verification and synthetic rounds share that cap.
  • Require trusted native turn/completed identity before app-server recovery.
  • Keep recovery scaffolding ephemeral while preserving genuine tool projections and strict role alternation.
  • Persist an explicit PAUSED notice when recovery or iteration budgets are exhausted.
  • Preserve the latest valid checkpoint without letting stale restored text overwrite newer recovery output.
  • Keep the legacy acknowledgement-nudge literal synthetic during resume and compaction so pre-upgrade transcripts remain compatible.
  • Treat explicit waits for the user or the user's review as terminal rather than spending a recovery attempt.
  • Consolidate duplicate app-server pause/projection paths into local helpers.

Safety boundaries

Automatic recovery does not run through:

  • user interruption;
  • approval, confirmation, credential, or password requests;
  • questions or explicit user-input waits;
  • active background jobs or processes;
  • transport errors, invalid finish reasons, or untrusted/stale app-server completion events;
  • genuine completion or refusal language.

Recovery remains bounded and consumes the app-server iteration budget. No tools, environment variables, prompt mutations, external telemetry, or provider-specific credentials are introduced.

Verification

Against current main (a1bfbccc02d5bfdaef1568facfca2cc1456c59f0):

  • Ruff: pass (one unrelated pre-existing invalid-noqa warning in run_agent.py).
  • py_compile: pass.
  • git diff --check: pass.
  • Changed-path suite: 178 passed.
  • Adjacent Codex runtime/transport/app-server/TUI suite: 184 passed.
  • Full tests/run_agent: 1,498 passed, 4 skipped, 2 deselected.
  • Secret/private-instance scan: no findings.

One earlier broad candidate run observed an order-dependent credential-pool mock failure. The failing test collects before this PR's changed tests/run_agent files; it passed alone and in paired isolation, the clean-base full suite passed in the same environment, and two later full candidate runs passed. No candidate-adjacent causal path was found.

Related work and credit

This reconciles the false-stop family covered by:

The adversarial corpus adaptation remains credited in test comments. This PR is intended as one architectural reconciliation rather than a novelty claim; the authors above are explicitly invited to review whether their reported scenarios are covered.

Closes #69778 if maintainers agree this shared bounded policy supersedes the narrower open implementations.

Share a two-attempt continuation budget across Codex runtimes and verification rounds. Preserve durable terminal checkpoints, tool-call projections, and app-server attempt accounting across recovery and failure paths.
Remove terminal sentinels before restoring checkpoint transcript rows while preserving interrupted-turn durability. Correct the app-server recovery budget comment and add focused regressions for returned and persisted history.
@CryptoKylan
CryptoKylan marked this pull request as ready for review August 11, 2026 01:13
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation labels Aug 11, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(agent): recover bounded Codex false stops across runtimes

  1. agent/codex_runtime.pyrun_codex_app_server_turn hardcodes for continuation_attempt in range(3) (initial turn + retries) while the actual cap lives in agent/terminal_continuation.py as MAX_TERMINAL_CONTINUATIONS = 2. The two are coupled but not derived from each other; if the constant changes, the loop bound silently drifts. Consider range(MAX_TERMINAL_CONTINUATIONS + 1).
  2. agent/codex_runtime.py — the loop's error/interrupt break only restores fallback_candidate_text when not current_turn.final_text. If a retry turn fails (or is interrupted) after emitting partial text, that partial text becomes the final response with turn.error set but no PAUSED notice appended — the user sees unmarked partial output instead of the checkpoint + pause notice that the empty-text path gets. Consider applying the notice/checkpoint handling whenever the final turn is non-terminal, not only when its text is empty.
  3. agent/terminal_continuation.py — the classifier's _ANNOUNCE_RE / _POST_TOOL_PROGRESS_RE use bounded wildcards ({0,80}?, {0,8}) that make the lexical surface hard to reason about and easy to regress. The adversarial corpus test (test_conversational_or_waiting_prose_is_terminal) helps, but consider extracting the corpus into a committed fixture file so contributors can extend it without touching the test function itself.
  4. Minor: identity comparisons (is ContinuationReason.NONE) are relied on across all callers. Safe today because it is a singleton str Enum, but a stray ContinuationReason("none") construction elsewhere would silently break those checks; a __new__ cache or a comment documenting the singleton invariant would make it explicit.

@CryptoKylan

Copy link
Copy Markdown
Author

Addressed the automated review and pushed 9d2f7ef.

  • Derive the app-server recovery loop from MAX_TERMINAL_CONTINUATIONS, preserving one initial turn plus the configured recovery attempts.
  • When a non-user retry fails, retires, or is interrupted with partial text, restore the prior checkpoint and persist a clear PAUSED recovery notice.
  • Preserve existing user-requested interrupt behavior without adding a pause notice.
  • Document the enum-singleton invariant behind ContinuationReason.NONE identity checks.

I left the suggested classifier-corpus fixture extraction out because it is test-only refactoring with no behavioral correction.

Validation: 75 focused tests passed, Ruff and git diff --check passed. An independent exact-head re-review passed.

@cygnostik

Copy link
Copy Markdown
Contributor

Related focused contribution: #117240 salvages the historical-tool gate work from #69779 and covers visible acknowledgments after an answered clarification, including conversational proceed and the overlapping tail path. Its real-loop/SQLite tests preserve declines, approval waits, completed/revoked context and live-work suppression without synthetic user rows. This does not port or replace your broader app-server/shared-policy/persistence work. Linking explicitly so maintainers can consolidate the overlapping detector slice; final local affected checks and exact-head independent review are recorded in that PR.

This branch has not been deployed

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: intent_ack_continuation never fires in established sessions with local models

4 participants