fix(coding-agent): recover oversized session resumes - #1263
Conversation
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: 1424e6752d
ℹ️ 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 systemPromptChange = await this._switchActiveModel(model, { | ||
| persistDefault: false, | ||
| appendSessionEntry: false, | ||
| emitModelSelect: true, |
There was a problem hiding this comment.
When the recovery candidate supports different thinking levels and a model_select handler subsequently rejects it—for example, by installing an oversized model-specific prompt—_switchActiveModel calls _setThinkingLevel(..., false, ...) before the final usability assertion, and that method still appends a thinking_level_change entry. Although appendSessionEntry: false prevents the candidate model from being recorded, the failed startup therefore mutates the saved session's thinking history; defer this mutation until admission succeeds or roll it back with the model and prompt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b3be0a: startup recovery now uses the target model’s normal thinking resolution but applies it ephemerally (persistThinkingLevel: false) until model-select admission succeeds, so a rejected recovery cannot append candidate-specific thinking history.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/coding-agent/src/core/changes.md">
<violation number="1" location="packages/coding-agent/src/core/changes.md:10">
P2: The doc claims recovery 'select[s] the authenticated candidate with the greatest remaining context budget' and that a session with 'no capable authenticated recovery model' keeps the typed ModelUsabilityBudgetError. In the code, findStartupRecoveryModel() selects purely by remaining budget from configured (not authenticated) models, and setStartupRecoveryModel() throws a plain `No API key` Error when the winner lacks auth — it never falls back to the next authenticated candidate, and the typed error is not preserved on that path. Make the recovery selection auth-filtered (or fall back to the next authenticated candidate) so the doc and the promised stack-free, typed failure match the behavior.</violation>
</file>
<file name="packages/coding-agent/src/main.ts">
<violation number="1" location="packages/coding-agent/src/main.ts:1132">
P2: When implicit resume recovery finds no capable model, this catch prints the ORIGINAL assertModelUsable error, whose live-context variant says the restored model "cannot switch ... Compact the session, then revalidate and retry the model switch." That message is misleading for this no-capable-model startup case: the failure is that no authenticated model fits the live context, not that a model switch should be retried, and the session cannot be resumed at all. Consider building a distinct message here (or in sdk.ts before rethrowing) that states no authenticated model has enough budget and suggests starting a new session, instead of surfacing the restored-model message.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| - `sdk.ts` now distinguishes an implicit saved-model restore from an explicit | ||
| startup model selection when the assembled runtime fails the live-context | ||
| usability projection. | ||
| - Implicit restores deterministically select the authenticated candidate with |
There was a problem hiding this comment.
P2: The doc claims recovery 'select[s] the authenticated candidate with the greatest remaining context budget' and that a session with 'no capable authenticated recovery model' keeps the typed ModelUsabilityBudgetError. In the code, findStartupRecoveryModel() selects purely by remaining budget from configured (not authenticated) models, and setStartupRecoveryModel() throws a plain No API key Error when the winner lacks auth — it never falls back to the next authenticated candidate, and the typed error is not preserved on that path. Make the recovery selection auth-filtered (or fall back to the next authenticated candidate) so the doc and the promised stack-free, typed failure match the behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/core/changes.md, line 10:
<comment>The doc claims recovery 'select[s] the authenticated candidate with the greatest remaining context budget' and that a session with 'no capable authenticated recovery model' keeps the typed ModelUsabilityBudgetError. In the code, findStartupRecoveryModel() selects purely by remaining budget from configured (not authenticated) models, and setStartupRecoveryModel() throws a plain `No API key` Error when the winner lacks auth — it never falls back to the next authenticated candidate, and the typed error is not preserved on that path. Make the recovery selection auth-filtered (or fall back to the next authenticated candidate) so the doc and the promised stack-free, typed failure match the behavior.</comment>
<file context>
@@ -1,5 +1,33 @@
+- `sdk.ts` now distinguishes an implicit saved-model restore from an explicit
+ startup model selection when the assembled runtime fails the live-context
+ usability projection.
+- Implicit restores deterministically select the authenticated candidate with
+ the greatest remaining context budget, persist that change only in the
+ session history, and return a visible fallback notice.
</file context>
There was a problem hiding this comment.
Fixed in b5d6c84 and documented in the trackers: candidates are budget-ranked, then provider auth is checked in that order; unavailable providers are skipped and deduplicated before the next capable candidate is selected.
| }); | ||
| } catch (error) { | ||
| if (error instanceof ModelUsabilityBudgetError) { | ||
| console.error(chalk.red(error.message)); |
There was a problem hiding this comment.
P2: When implicit resume recovery finds no capable model, this catch prints the ORIGINAL assertModelUsable error, whose live-context variant says the restored model "cannot switch ... Compact the session, then revalidate and retry the model switch." That message is misleading for this no-capable-model startup case: the failure is that no authenticated model fits the live context, not that a model switch should be retried, and the session cannot be resumed at all. Consider building a distinct message here (or in sdk.ts before rethrowing) that states no authenticated model has enough budget and suggests starting a new session, instead of surfacing the restored-model message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/main.ts, line 1132:
<comment>When implicit resume recovery finds no capable model, this catch prints the ORIGINAL assertModelUsable error, whose live-context variant says the restored model "cannot switch ... Compact the session, then revalidate and retry the model switch." That message is misleading for this no-capable-model startup case: the failure is that no authenticated model fits the live context, not that a model switch should be retried, and the session cannot be resumed at all. Consider building a distinct message here (or in sdk.ts before rethrowing) that states no authenticated model has enough budget and suggests starting a new session, instead of surfacing the restored-model message.</comment>
<file context>
@@ -1113,13 +1118,22 @@ export async function main(args: string[], options?: MainOptions) {
+ });
+ } catch (error) {
+ if (error instanceof ModelUsabilityBudgetError) {
+ console.error(chalk.red(error.message));
+ process.exit(1);
+ }
</file context>
There was a problem hiding this comment.
Fixed in 0b3be0a: no-capable implicit resume now throws SessionResumeModelUnavailableError with explicit guidance that no authenticated model fits and the user must configure a larger-context model or start a new session.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b3be0a23f
ℹ️ 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 restoredModel = session.model; | ||
| if (!restoredModel) throw error; | ||
| await session.setStartupRecoveryModel(recovery.model, liveContextTokens); |
There was a problem hiding this comment.
When the highest-budget candidate passes the preliminary projection but its model_select handler installs a prompt that exceeds the budget, this await throws and aborts the entire resume even if a later authenticated candidate would pass full admission. The candidates are ranked using the restored prompt before model-specific hooks run, so recovery should continue through the remaining candidates after a budget-admission rejection rather than failing on the first one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 83a032d: full model_select admission now runs inside the ranked candidate loop. A ModelUsabilityBudgetError rolls back the candidate and continues; the admitted candidate alone is persisted. The new regression proves an oversized largest-candidate hook falls through to the medium candidate.
|
Ready for maintainer merge: all required checks are green, the automated Codex/Cubic findings are addressed through 0b3be0a, and the active OMO runtime has resumed representative oversized and fitting sessions successfully. I do not have permission to request a reviewer or enable auto-merge on this repository. |
|
Addressed the latest P2 review in 83a032d: startup now performs full model-select budget admission per authenticated candidate and continues to the next ranked candidate when a hook makes one unusable. Added deterministic regression coverage; focused tests, root check, and package build pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83a032d419
ℹ️ 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".
| `Restored context exceeds ${restoredModel.provider}/${restoredModel.id}'s usable budget. ` + | ||
| `Using ${recovery.model} for this session with ` + | ||
| `${recovery.contextWindow - recovery.requiredTokens} tokens of remaining budget.`; | ||
| modelFallbackMessage = modelFallbackMessage ? `${modelFallbackMessage}. ${recoveryMessage}` : recoveryMessage; |
There was a problem hiding this comment.
When recovery occurs during --print --continue, main.ts never emits or passes this modelFallbackMessage to runPrintMode; similarly, after the TUI session picker calls switchSession, AgentSessionRuntime.apply() updates the property but handleResumeSession() only shows the generic “Resumed session” status. In those contexts the agent can silently switch to a different model or provider, changing output and potentially cost, so propagate the recovery notice through headless modes and runtime replacements rather than consuming it only when constructing the initial InteractiveMode.
Useful? React with 👍 / 👎.
# Conflicts: # packages/coding-agent/CHANGELOG.md
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/coding-agent/test/suite/model-usability-budget.test.ts">
<violation number="1" location="packages/coding-agent/test/suite/model-usability-budget.test.ts:449">
P3: The tool-rollback assertion may be insensitive to the rollback it claims to verify. `tools: ["stable-tool"]` sets `allowedToolNames = ["stable-tool"]`, and the model-change path re-filters active tools to the allowed set (`_refreshToolRegistry` in agent-session.ts drops names outside `allowedToolNames`). If that filter runs for the accepted candidate-medium, a leaked `candidate-tool` is dropped regardless of rollback, so `getActiveToolNames()` equals `["stable-tool"]` even when the rejected candidate's tool state was not restored. The test still catches the extension-state leak via the `rejectedCandidateState` flag (candidate-medium then receives an oversized prompt and recovery fails), but the tools half of the regression can pass without exercising real tool rollback. Consider leaving `candidate-tool` inside the allowed set (or not narrowing `tools`) so the rejection of a leaked active tool is observable.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| // then | ||
| expect(resumed.session.model).toMatchObject({ id: "candidate-medium" }); | ||
| expect(resumed.session.getActiveToolNames()).toEqual(["stable-tool"]); |
There was a problem hiding this comment.
P3: The tool-rollback assertion may be insensitive to the rollback it claims to verify. tools: ["stable-tool"] sets allowedToolNames = ["stable-tool"], and the model-change path re-filters active tools to the allowed set (_refreshToolRegistry in agent-session.ts drops names outside allowedToolNames). If that filter runs for the accepted candidate-medium, a leaked candidate-tool is dropped regardless of rollback, so getActiveToolNames() equals ["stable-tool"] even when the rejected candidate's tool state was not restored. The test still catches the extension-state leak via the rejectedCandidateState flag (candidate-medium then receives an oversized prompt and recovery fails), but the tools half of the regression can pass without exercising real tool rollback. Consider leaving candidate-tool inside the allowed set (or not narrowing tools) so the rejection of a leaked active tool is observable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/test/suite/model-usability-budget.test.ts, line 449:
<comment>The tool-rollback assertion may be insensitive to the rollback it claims to verify. `tools: ["stable-tool"]` sets `allowedToolNames = ["stable-tool"]`, and the model-change path re-filters active tools to the allowed set (`_refreshToolRegistry` in agent-session.ts drops names outside `allowedToolNames`). If that filter runs for the accepted candidate-medium, a leaked `candidate-tool` is dropped regardless of rollback, so `getActiveToolNames()` equals `["stable-tool"]` even when the rejected candidate's tool state was not restored. The test still catches the extension-state leak via the `rejectedCandidateState` flag (candidate-medium then receives an oversized prompt and recovery fails), but the tools half of the regression can pass without exercising real tool rollback. Consider leaving `candidate-tool` inside the allowed set (or not narrowing `tools`) so the rejection of a leaked active tool is observable.</comment>
<file context>
@@ -366,6 +368,89 @@ describe("model usability budget", () => {
+
+ // then
+ expect(resumed.session.model).toMatchObject({ id: "candidate-medium" });
+ expect(resumed.session.getActiveToolNames()).toEqual(["stable-tool"]);
+ expect(sessionManager.getEntries().filter((entry) => entry.type === "model_change")).toMatchObject([
+ { provider: "faux", modelId: "saved-small" },
</file context>
|
Final hardening in 6dc052a: recovery candidate model-select now has a provisional probe phase followed by a committed selection only after budget admission. Claude SDK OAuth ignores provisional probes, so a rejected non-Claude candidate cannot erase a restored Claude continuity binding. Regression coverage now includes that continuity invariant, candidate tool/extension rollback, and per-provider auth caching. |
There was a problem hiding this comment.
2 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/coding-agent/src/core/agent-session.ts">
<violation number="1" location="packages/coding-agent/src/core/agent-session.ts:4822">
P2: When a recovery hook returns the same prompt on both passes, the provisional pass consumes the prompt transition and the committed pass emits no `system_prompt_change`. Restore the pre-probe prompt before the committed pass, or otherwise publish the committed transition against the original prompt.</violation>
</file>
<file name="packages/coding-agent/src/core/extensions/types.ts">
<violation number="1" location="packages/coding-agent/src/core/extensions/types.ts:1132">
P3: This public API addition to `ModelSelectEvent` (the new `provisional` field) is not recorded in `extensions/changes.md`, which the extensions AGENTS.md requires for every types.ts public-API change with no exceptions. Add a section documenting the new `provisional` probe field and its side-effect deferral contract, including the expected merge-conflict zone.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| opts.transactionalModelSelect, | ||
| ); | ||
| this.assertModelUsable(model, liveContextTokens); | ||
| const committedSystemPromptChange = opts.transactionalModelSelect |
There was a problem hiding this comment.
P2: When a recovery hook returns the same prompt on both passes, the provisional pass consumes the prompt transition and the committed pass emits no system_prompt_change. Restore the pre-probe prompt before the committed pass, or otherwise publish the committed transition against the original prompt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/core/agent-session.ts, line 4822:
<comment>When a recovery hook returns the same prompt on both passes, the provisional pass consumes the prompt transition and the committed pass emits no `system_prompt_change`. Restore the pre-probe prompt before the committed pass, or otherwise publish the committed transition against the original prompt.</comment>
<file context>
@@ -4814,18 +4816,24 @@ export class AgentSession {
+ opts.transactionalModelSelect,
);
this.assertModelUsable(model, liveContextTokens);
+ const committedSystemPromptChange = opts.transactionalModelSelect
+ ? await this._emitModelSelect(model, previousModel, opts.modelSelectSource, false)
+ : systemPromptChange;
</file context>
| * A startup-recovery candidate probe. Handlers may adjust in-memory prompt or | ||
| * tool state, but must defer external side effects until the committed event. | ||
| */ | ||
| provisional?: boolean; |
There was a problem hiding this comment.
P3: This public API addition to ModelSelectEvent (the new provisional field) is not recorded in extensions/changes.md, which the extensions AGENTS.md requires for every types.ts public-API change with no exceptions. Add a section documenting the new provisional probe field and its side-effect deferral contract, including the expected merge-conflict zone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/core/extensions/types.ts, line 1132:
<comment>This public API addition to `ModelSelectEvent` (the new `provisional` field) is not recorded in `extensions/changes.md`, which the extensions AGENTS.md requires for every types.ts public-API change with no exceptions. Add a section documenting the new `provisional` probe field and its side-effect deferral contract, including the expected merge-conflict zone.</comment>
<file context>
@@ -1125,6 +1125,11 @@ export interface ModelSelectEvent {
+ * A startup-recovery candidate probe. Handlers may adjust in-memory prompt or
+ * tool state, but must defer external side effects until the committed event.
+ */
+ provisional?: boolean;
/** The active system prompt before model_select handlers run. */
systemPrompt: string;
</file context>
1 participant
Summary
This supersedes the crash-only handling in #1259 by making the session actually resumable when a capable authenticated model exists.
Verification
npx vitest --run test/suite/model-usability-budget.test.ts -t 'recovers an oversized implicit saved-model restore|does not persist a recovery model|explicit oversized startup model|restored transcript exceeds'npx vitest --run test/suite/regressions/model-usability-budget-startup-error.test.tsnpx vitest --run test/model-resolver.test.tsnpx tsc --noEmitnpm run build --workspace @code-yeongyu/senpinpx biome check packages/coding-agent/src/core/agent-session.ts packages/coding-agent/src/core/sdk.ts packages/coding-agent/src/main.ts packages/coding-agent/test/suite/model-usability-budget.test.ts packages/coding-agent/test/suite/regressions/model-usability-budget-startup-error.test.tsmodel-runtime-text-toolcall-recovery.test.tspermission/external-dir.test.tsapp-server-thread-handlers-archive.test.ts(1 ms timestamp boundary)model-usability-budget.test.ts:127(2-token brittle boundary)Real-surface QA
Installed the built runtime into the active OMO wrapper and exercised:
Review Issue 406: resumed interactively instead of failing startup, opened at90K/1M, quit cleanlyBuild Chat UI Mockup: resumed interactively at20K/272K, quit cleanlycliproxyapi/gpt-5.6-solat217/272K--model cliproxyapi/gpt-5.6-sol: still failed closed with the actionable budget message and no Node stackReview
Independent code review: approved with no blockers after atomic-history and fixture-path fixes.
Summary by cubic
Reopens existing sessions whose restored transcript outgrew their saved model by falling back to the authenticated model with the largest verified remaining context budget. Previously these sessions crashed at startup; now they resume on a capable model while explicit
--modelselections stay fail-closed.Bug Fixes
Written for commit 6dc052a. Summary will update on new commits.