Skip to content

fix(codex): stop oversized resumes without losing the current task - #1443

Draft
baron wants to merge 2 commits into
mainfrom
fix/1377-codex-fresh-thread-task-notice
Draft

baron wants to merge 2 commits into
mainfrom
fix/1377-codex-fresh-thread-task-notice

Conversation

@baron

@baron baron commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Refs #1377.

Scope and root cause

Merged #1379 (27d13e762) already fixed recovery-notice retention. The remaining oversized-resume path on the task's origin/main base (ab84d47e6) still automatically started a fresh thread, then delivered a follow-up without the prior task. This PR stops that unsafe replacement; it is part of #1377, not the complete history-free-resume / explicit recovery-choice solution described in its maintainer comment.

Changes, including adversarial-review response

  1. Preserve the real failure. The oversized-resume explanation uses the recognized native-resume failure prefix. Send returns that explanation instead of session not ready; terminal settlement does not append a duplicate generic error. Failure notifications use the settled transcript error when the provider has already reported it. A real terminal-barrier regression asserts one error per user turn, exact send/notification text, and canonical failure publication.
  2. Do not repeat a known-doomed resume. Remember the exact oversized conversation/rollout pair in session state. Later sends, recovery, and native control commands stop before launching another app-server for that pair. Guidance points to the existing Handoff to New Chat flow on an assistant message, which carries context over, rather than suggesting a doomed retry or context-free new tab. Failed sends publish the distinct failure_reason: resume_history_too_large; run snapshots, lane-board mapping, display text, and CLI help support that reason. The blocker is session-scoped like resume-timeout accounting; changed references can resume, and a restart/restoration may make one new attempt before an overflow is remembered again.
  3. Do not claim the whole issue is solved. This body uses Refs #1377, not an auto-closing keyword.
  4. Cover non-fallback callers. Regression coverage exercises both stream-end recovery and /compact, including repeat attempts, no replacement/turn/compact dispatch, preserved identity, and successful unblocking when the rollout reference changes.
  5. Preserve error severity after compaction. Recovery diagnostics carry the originating system/error kind. Old or unknown persisted kinds safely default to system; request/assistant kinds cannot be impersonated. Compacted stop notices rebuild as errors, and latestErrorText consults retained summary diagnostics. Tests assert row kind, exact text, serialization round-trip, and latest-error lookup.
  6. Use stable diagnostic constants. Added resumeStoppedOutcome and resumeFrameTooLargeReason, and replaced the stale notice-length comment with the actual consumer-budget constraint.

The original conversation ID, rollout path, and cleanup handle remain intact. The poisoned controller is retired; the current follow-up is not sent to a replacement thread.

Remaining overlap / deliberately not implemented

  • Oversized history can also manifest as a 120-second resume timeout rather than a frame-overflow error. That still reaches the existing repeated-timeout automatic-fresh fallback covered by Codex: after a timed-out resume, a fresh thread starts silently and the conversation is lost #1299; this PR does not change its policy or UX.
  • thread/read with full turns can overflow too. This PR does not implement bounded/full-history-free reads or resume reconciliation.
  • No new recovery-choice dialog or automatic task recap is added. Missing-rollout and repeated-timeout fallback behavior is unchanged.

Validation

  • Final make dev-test FILTER='CodexResumeWedgeCommitTests|AgentTranscriptRecoveryDiagnosticTests|AgentRunLifecycleContractsTests': 57 passed, 0 failures (43 + 10 + 4), ticket f6c39dc3-f9bf-4c5f-94f4-498e5032dd15. Includes a second appended user turn, barrier publication/notification assertions, and the /compact/recovery cases.
  • Coordinated ./conductor swift-build --product all --async --request-key issue-1377-review-products: RepoPrompt and repoprompt-mcp both passed, ticket 9502bdd8-60a5-4469-9e98-098d229d9b06. Production source was unchanged by the subsequent fixture correction / second-user-turn assertion.
  • Final make dev-lint: passed, ticket f92f84e6-bc42-4d76-8407-22409d62a185.
  • Repository-pinned SwiftFormat 0.61.1 used only on changed files; no repository-wide formatting mutation or unrelated Sendable changes retained.
  • In-session Oracle review: no concrete production correctness findings; its stronger second-user-turn regression suggestion was applied.
  • Mandatory staged-index commit and outgoing-range push preflights: passed, including redacted secret scans, whitespace checks, and repository guardrails.

Iteration evidence: the first review-response test build caught a fixture assignment to read-only runID (ticket d77f66e8-a265-4a4d-9719-666c07b8c343); it was corrected to use AgentModeProcessRunIdentity before the passing run. The original implementation's safety regression failed first against the old automatic-replacement source (ticket 3b8f49cb-cdf3-4ac6-9c94-b10b9731a474; its redundant automatic cold retry was canceled). The earlier PR revision passed 51 focused tests, lint, and the App product build, but its no-barrier fixture missed the failure-propagation issue now covered above.

Not run: full root/provider suites, full pr-ready, packaging/release validation, live MCP/UI smoke, live Codex requests, or any visible app launch/stop/relaunch. Regression coverage uses injected controllers, not a live provider.

PR remains draft for opposing-model review; the dedicated worktree is retained.

@baron baron left a comment

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.

Reviewed 9a7a9b1a9f6b289cb4f8e30d783e76e0aa95848b against its merge base.

No actionable correctness defect identified in the oversized-resume fix. I checked preserved conversation identity, controller retirement, blocking repeated attempts for the same conversation/rollout pair, allowing a changed target, /compact and watchdog recovery refusal, and retained error projection through transcript compaction. The tests assert that no replacement thread or user turn is started and that the terminal snapshot carries resume_history_too_large.

The observed CI run for this exact head passed: https://github.com/repoprompt/repoprompt-ce/actions/runs/39811205618. Review here was static source/test inspection, not a local macOS run.

Integration note: #1457 changes the same resume fallback and diagnostic paths to stop on timeouts. This branch still retains the old timeout-to-fresh policy, while #1457 still retains the old oversized-to-fresh policy. Integrate them deliberately and run both sets of regressions; their independent green checks do not prove the combined behavior.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant