fix(coding-agent): run resume budget check before session teardown - #1473
fix(coding-agent): run resume budget check before session teardown#1473Tinycute00 wants to merge 2 commits into
Conversation
Resuming a session whose restored transcript exceeds the current model's context budget threw ModelUsabilityBudgetError only from createAgentSession, which runs after switchSession has already torn down the live session and invalidated its extension runner. A resume that was always going to be rejected therefore destroyed the session the user was still in, and the next input crashed with the stale-extension-context error. In interactive mode the error was routed to handleFatalRuntimeError -> process.exit(1), so the user was silently dropped to the shell. AgentSessionRuntime.switchSession now runs assertSessionAdmissible before teardownCurrent, mirroring the post-teardown admission check in createAgentSession against the current model. A rejected resume becomes a clean no-op that leaves the live session intact. InteractiveMode.handleResumeSession now catches ModelUsabilityBudgetError, shows it via showError, and returns cancelled instead of exiting the process.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99e7984681
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const model = this.session.model; | ||
| if (!model) return; | ||
| const liveContextTokens = existingSession.messages.reduce((total, message) => total + estimateTokens(message), 0); | ||
| this.session.assertModelUsable(model, liveContextTokens, { includeSpeculationLead: false, admission: "resume" }); |
There was a problem hiding this comment.
When the destination session stores a different model from the active session, this preflight evaluates this.session.model (and the active session's prompt/tool budget), while createAgentSession later restores existingSession.model using the destination services before performing the authoritative check. If the active model has a larger window than the restored model, this check passes, teardown still invalidates the live session, and the post-teardown check throws—the destructive failure this change intends to prevent; the inverse combination incorrectly rejects a valid resume. Resolve the destination runtime inputs before performing the preflight.
AGENTS.md reference: packages/coding-agent/src/core/AGENTS.md:L37-L40
Useful? React with 👍 / 👎.
| if (error instanceof ModelUsabilityBudgetError) { | ||
| this.showError(`Failed to resume session: ${error.message}`); | ||
| return { cancelled: true }; | ||
| } |
There was a problem hiding this comment.
When the selected session's recorded cwd is missing, the first switch enters the MissingSessionCwdError branch and the user supplies a replacement cwd, but any ModelUsabilityBudgetError thrown by the second switchSession call occurs from inside this catch block and cannot be handled by the sibling branch below. The session selector invokes this async callback through a void callback, so an over-budget moved session still produces an unhandled rejection instead of the visible recoverable error promised here; apply the budget-error handling to the override attempt as well.
Useful? React with 👍 / 👎.
| if (error instanceof ModelUsabilityBudgetError) { | ||
| this.showError(`Failed to resume session: ${error.message}`); | ||
| return { cancelled: true }; | ||
| } |
There was a problem hiding this comment.
When experimental.sharedHost is enabled, switchSession runs through RpcClient, whose getData reconstructs all non-missing-cwd failures as plain Error objects. Consequently a host-side ModelUsabilityBudgetError never satisfies this instanceof check and falls through to handleFatalRuntimeError, so the TUI still exits for exactly the recoverable resume failure this branch is intended to handle. Carry a typed RPC error code and reconstruct or otherwise classify the budget error on the client.
Useful? React with 👍 / 👎.
| const previousSessionFile = this.session.sessionFile; | ||
| const sessionManager = SessionManager.open(sessionPath, undefined, options?.cwdOverride); | ||
| assertSessionCwdExists(sessionManager, this.cwd); | ||
| await this.assertSessionAdmissible(sessionManager); |
There was a problem hiding this comment.
On an over-budget resume, session_before_switch is emitted before this new check, so handlers can irreversibly tear down live-session state even though the switch is then rejected and the current session remains active. This already affects the bundled btw extension: its handler aborts the active side query and removes its widget, after which assertSessionAdmissible throws and there is no shutdown/start sequence or rollback. Perform the non-mutating admission check before emitting the pre-switch event so a rejected resume is actually a no-op.
AGENTS.md reference: packages/coding-agent/AGENTS.md:L62-L64
Useful? React with 👍 / 👎.
…ore the switch event Resolve the model a resume will actually restore (the destination session's stored model via resolveStoredModelReference when present and authorized, else the live model) and run the admission check against it, so a resume that the active model would fit but the restored model cannot is rejected before the live session is torn down. Move the non-mutating preflight (SessionManager.open, cwd existence, admission) ahead of the session_before_switch emit so a rejected resume is a true no-op and never lets handlers mutate live-session state. Give the cwd-override retry in handleResumeSession the same recoverable budget handling via a shared cancelResumeWithBudgetError helper instead of letting a second-attempt budget error escape as an unhandled rejection. Preserve the budget rejection's typed identity across the shared-host RPC boundary: connection-handler emits a model_usability_budget error code with the projection, and rpc-client reconstructs ModelUsabilityBudgetError so the TUI's instanceof check still holds client-side.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed all four Codex findings plus the Changelog gate on this branch (commit
Tests: extended |
1 participant
Summary
Interactive
/resumeof a session whose restored transcript exceeds the current model's context budget silently killed the whole process (exit 1, no visible error). This fixes it in two layers.Root cause
AgentSessionRuntime.switchSession()(packages/coding-agent/src/core/agent-session-runtime.ts) ranteardownCurrent("resume", ...)— which aborts the session, emitssession_shutdown, callsdispose()and invalidates the extension runner — before the resume admission check. That admission check (assertModelUsable→ModelUsabilityBudgetError) only fires later, insidecreateAgentSession(sdk.ts), i.e. after teardown. So a resume that was always going to be rejected first destroyed the live session the user was still in. Any subsequent input then crashed withThis extension ctx is stale after session replacement or reload.On top of that,
InteractiveMode.handleResumeSession()(packages/coding-agent/src/modes/interactive/interactive-mode.ts) routed every non-MissingSessionCwdErrortohandleFatalRuntimeError → process.exit(1). The error text was appended to the chat container, but the TUI was torn down before it could repaint, so the user saw a silent drop to the shell.Repro (kimi-coding/k3-256k, contextWindow 262144, maxTokens 131072): a target session at ~148011 live tokens requires ~323030 tokens →
ModelUsabilityBudgetError. Before: silent exit 1. With a 1M-context model the same resume succeeds.Fix
Part A —
agent-session-runtime.ts.switchSessionnow calls a newassertSessionAdmissible(sessionManager)beforeteardownCurrent. It rebuilds the target session context, sums live-context tokens viaestimateTokens, and runsthis.session.assertModelUsable(model, liveContextTokens, { includeSpeculationLead: false, admission: "resume" })against the current model — mirroring the checkcreateAgentSessionperforms after teardown. Running it pre-teardown makes a rejected resume a clean no-op: the live session is never disposed and its extension runner stays active. No extension hook exists betweenteardownCurrentandcreateRuntime, so this ordering must live in the core runtime.Part B —
interactive-mode.ts.handleResumeSession's catch block now handlesModelUsabilityBudgetErrorbefore thehandleFatalRuntimeErrorfallback: it shows the message viashowErrorand returns{ cancelled: true }. An over-budget resume is an expected, recoverable outcome, not a fatal fault — the interactive session stays alive with a visible explanation instead ofprocess.exit(1).Why not an extension
The teardown/invalidate ordering is core
AgentSessionRuntimestate-machine behavior with no extension hook betweenteardownCurrentandcreateRuntime; the process-exit decision is core interactive-mode control flow in the caught branch. Both changed files are upstream-tracked, so each has achanges.mdentry (src/core/changes.md,src/modes/interactive/changes.md) covering what changed, why, why an extension could not handle it, and expected merge-conflict zones (all LOW).Verification evidence
process.exit(1)with no visible message.ModelUsabilityBudgetErrorwithout touching the live session (extensionRunner.isActive === true, same session object/file, next prompt runs); interactive mode shows the error and cancels; a large-context model still resumes fine.Test plan
packages/coding-agent/test/suite/agent-session-runtime.test.ts→ "rejects an over-budget resume without invalidating the live session". It seeds a target session whose transcript far exceeds the model's context window, assertsswitchSessionrejects withModelUsabilityBudgetError, and asserts the live session is untouched (extensionRunner.isActive === true, same object/file, nextpromptresolves). Confirmed it fails without Part A (expected false to be trueonisActive) and passes with it.bun run --cwd packages/coding-agent test -- suite/agent-session-runtime.test.ts suite/model-usability-budget.test.ts→ 24 passed.bun run check(biome + pinned-deps/ts-imports/shrinkwrap/install-lock/claude-sdk-platform-lock +tsc --noEmit+ browser-smoke) → pass (exit 0, 0 TS errors). Also enforced by the pre-commit hook on commit.Notes
badlogic/pi-mono(the ordering lives in the sharedAgentSessionRuntime); this PR targetscode-yeongyu/senpiper the fork's contribution rules.changes.mdentries. No drive-by edits.Summary by cubic
Fixes
/resumeof an over-budget session so it no longer destroys the live session or exits the process. The model-budget admission check now runs before session teardown and the switch lifecycle event, against the model the resume will actually restore, and interactive mode shows the error and cancels instead of callingprocess.exit(1).session_before_switchevent fires.Written for commit 0b85a15. Summary will update on new commits.