feat(recovery): add session fallback restore and manual compact - #1268
Open
eddieparc wants to merge 3 commits into
Open
feat(recovery): add session fallback restore and manual compact#1268eddieparc wants to merge 3 commits into
eddieparc wants to merge 3 commits into
Conversation
Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>
Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>
Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 14 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/test/claude-sdk-oauth-compaction-alignment.test.ts">
<violation number="1" location="packages/coding-agent/test/claude-sdk-oauth-compaction-alignment.test.ts:238">
P3: The manual-compaction test only asserts that the result is not rejected as `external-owner`, so it also passes when the handler returns `undefined` and no compaction actually ran. Since this test is the failing-first proof for the `/compact` override, assert the positive outcome (e.g. `result?.rejectionCause` is undefined / the handler returns a compaction or ran `applyCompaction`) instead of only the absence of the lane rejection.</violation>
</file>
<file name="packages/coding-agent/src/core/agent-session.ts">
<violation number="1" location="packages/coding-agent/src/core/agent-session.ts:6887">
P2: When the original model disappears from the registry, `/fallback restore` reports that no fallback is active while leaving the fallback state active. Return a distinct unavailable result or surface the unresolved original model instead of mapping this case to inactive.</violation>
<violation number="2" location="packages/coding-agent/src/core/agent-session.ts:6890">
P2: When the model-select step fails, `setSessionModel` has already cleared `_retryFallback`; the runtime rolls back to the fallback model, but `/fallback restore` can no longer retry it. Use the fallback-revert switch path and clear the controller only after the switch succeeds.</violation>
<violation number="3" location="packages/coding-agent/src/core/agent-session.ts:6890">
P3: Restore bypasses the retry controller's revert semantics. It clears state only via the side effect of `setSessionModel` -> `clearForManualModelChange` (which does not emit), so no `retry_fallback_reverted` event fires and the switch is recorded as an ordinary manual model change rather than a fallback revert. Consider restoring through a controller-level revert/`switchModel(...,"fallback-revert")` so history and the `retry_fallback_reverted` event stay consistent.</violation>
<violation number="4" location="packages/coding-agent/src/core/agent-session.ts:6890">
P2: `restoreFallbackPrimary` delegates to `setSessionModel`, which throws (auth missing via `checkAuth`, model unusable via `assertModelUsable`, or `emitModelSelect` failure) instead of returning false. The `/fallback restore` handler awaits it without try/catch, so such a failure surfaces as an uncaught command error rather than the graceful boolean/warning path, and on the auth-failure path the fallback state is left uncleared. Catch errors here and return false so the command reports "could not restore" instead of throwing.</violation>
<violation number="5" location="packages/coding-agent/src/core/agent-session.ts:6891">
P2: A restore interrupted after `setSessionModel` persists its model entry but before the next line persists the original thinking level reloads with the wrong thinking level. Apply the original level through the existing `fallback-revert` transition so model and thinking restoration are one persisted operation.</violation>
</file>
<file name="packages/coding-agent/src/core/extensions/builtin/compaction/index.ts">
<violation number="1" location="packages/coding-agent/src/core/extensions/builtin/compaction/index.ts:564">
P2: When `/compact` fails on the Claude SDK lane, the session-compaction failure handler still treats the lane as externally owned and does not debit the circuit breaker. Count failures for the newly senpi-owned `manual` route as well, so repeated failures remain protected after switching to a senpi-owned provider.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const active = this._retryFallback.activeState; | ||
| if (!active) return false; | ||
| const selector = parseFallbackSelector(active.originalSelector, this._modelRegistry); | ||
| const model = selector ? this._modelRegistry.find(selector.provider, selector.id) : undefined; |
There was a problem hiding this comment.
P2: When the original model disappears from the registry, /fallback restore reports that no fallback is active while leaving the fallback state active. Return a distinct unavailable result or surface the unresolved original model instead of mapping this case to inactive.
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 6887:
<comment>When the original model disappears from the registry, `/fallback restore` reports that no fallback is active while leaving the fallback state active. Return a distinct unavailable result or surface the unresolved original model instead of mapping this case to inactive.</comment>
<file context>
@@ -6880,6 +6880,17 @@ export class AgentSession {
+ const active = this._retryFallback.activeState;
+ if (!active) return false;
+ const selector = parseFallbackSelector(active.originalSelector, this._modelRegistry);
+ const model = selector ? this._modelRegistry.find(selector.provider, selector.id) : undefined;
+ if (!model) return false;
+ const originalThinkingLevel = active.originalThinkingLevel;
</file context>
| if (!model) return false; | ||
| const originalThinkingLevel = active.originalThinkingLevel; | ||
| await this.setSessionModel(model); | ||
| if (originalThinkingLevel !== undefined) this.setSessionThinkingLevel(originalThinkingLevel); |
There was a problem hiding this comment.
P2: A restore interrupted after setSessionModel persists its model entry but before the next line persists the original thinking level reloads with the wrong thinking level. Apply the original level through the existing fallback-revert transition so model and thinking restoration are one persisted operation.
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 6891:
<comment>A restore interrupted after `setSessionModel` persists its model entry but before the next line persists the original thinking level reloads with the wrong thinking level. Apply the original level through the existing `fallback-revert` transition so model and thinking restoration are one persisted operation.</comment>
<file context>
@@ -6880,6 +6880,17 @@ export class AgentSession {
+ if (!model) return false;
+ const originalThinkingLevel = active.originalThinkingLevel;
+ await this.setSessionModel(model);
+ if (originalThinkingLevel !== undefined) this.setSessionThinkingLevel(originalThinkingLevel);
+ return true;
+ },
</file context>
| const model = selector ? this._modelRegistry.find(selector.provider, selector.id) : undefined; | ||
| if (!model) return false; | ||
| const originalThinkingLevel = active.originalThinkingLevel; | ||
| await this.setSessionModel(model); |
There was a problem hiding this comment.
P2: When the model-select step fails, setSessionModel has already cleared _retryFallback; the runtime rolls back to the fallback model, but /fallback restore can no longer retry it. Use the fallback-revert switch path and clear the controller only after the switch succeeds.
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 6890:
<comment>When the model-select step fails, `setSessionModel` has already cleared `_retryFallback`; the runtime rolls back to the fallback model, but `/fallback restore` can no longer retry it. Use the fallback-revert switch path and clear the controller only after the switch succeeds.</comment>
<file context>
@@ -6880,6 +6880,17 @@ export class AgentSession {
+ const model = selector ? this._modelRegistry.find(selector.provider, selector.id) : undefined;
+ if (!model) return false;
+ const originalThinkingLevel = active.originalThinkingLevel;
+ await this.setSessionModel(model);
+ if (originalThinkingLevel !== undefined) this.setSessionThinkingLevel(originalThinkingLevel);
+ return true;
</file context>
| // compaction overrides the delegation: senpi cannot observe that the SDK | ||
| // failed to compact, so `/compact` is the only way back under the limit | ||
| // once the delegated owner has not delivered. | ||
| if (!isLaneOverrideReason(event.reason) && lanePolicy.disablesSenpiCompaction(ctx)) { |
There was a problem hiding this comment.
P2: When /compact fails on the Claude SDK lane, the session-compaction failure handler still treats the lane as externally owned and does not debit the circuit breaker. Count failures for the newly senpi-owned manual route as well, so repeated failures remain protected after switching to a senpi-owned provider.
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/builtin/compaction/index.ts, line 564:
<comment>When `/compact` fails on the Claude SDK lane, the session-compaction failure handler still treats the lane as externally owned and does not debit the circuit breaker. Count failures for the newly senpi-owned `manual` route as well, so repeated failures remain protected after switching to a senpi-owned provider.</comment>
<file context>
@@ -556,7 +557,11 @@ export default function compactionExtension(
+ // compaction overrides the delegation: senpi cannot observe that the SDK
+ // failed to compact, so `/compact` is the only way back under the limit
+ // once the delegated owner has not delivered.
+ if (!isLaneOverrideReason(event.reason) && lanePolicy.disablesSenpiCompaction(ctx)) {
return {
cancel: true,
</file context>
| const model = selector ? this._modelRegistry.find(selector.provider, selector.id) : undefined; | ||
| if (!model) return false; | ||
| const originalThinkingLevel = active.originalThinkingLevel; | ||
| await this.setSessionModel(model); |
There was a problem hiding this comment.
P2: restoreFallbackPrimary delegates to setSessionModel, which throws (auth missing via checkAuth, model unusable via assertModelUsable, or emitModelSelect failure) instead of returning false. The /fallback restore handler awaits it without try/catch, so such a failure surfaces as an uncaught command error rather than the graceful boolean/warning path, and on the auth-failure path the fallback state is left uncleared. Catch errors here and return false so the command reports "could not restore" instead of throwing.
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 6890:
<comment>`restoreFallbackPrimary` delegates to `setSessionModel`, which throws (auth missing via `checkAuth`, model unusable via `assertModelUsable`, or `emitModelSelect` failure) instead of returning false. The `/fallback restore` handler awaits it without try/catch, so such a failure surfaces as an uncaught command error rather than the graceful boolean/warning path, and on the auth-failure path the fallback state is left uncleared. Catch errors here and return false so the command reports "could not restore" instead of throwing.</comment>
<file context>
@@ -6880,6 +6880,17 @@ export class AgentSession {
+ const model = selector ? this._modelRegistry.find(selector.provider, selector.id) : undefined;
+ if (!model) return false;
+ const originalThinkingLevel = active.originalThinkingLevel;
+ await this.setSessionModel(model);
+ if (originalThinkingLevel !== undefined) this.setSessionThinkingLevel(originalThinkingLevel);
+ return true;
</file context>
| // The delegation covers senpi's AUTOMATIC compaction only. `/compact` is the | ||
| // user's escape hatch for the case the lane cannot detect: the SDK did not | ||
| // compact and the session has no other way back under the limit. | ||
| expect(result?.rejectionCause).not.toBe("external-owner"); |
There was a problem hiding this comment.
P3: The manual-compaction test only asserts that the result is not rejected as external-owner, so it also passes when the handler returns undefined and no compaction actually ran. Since this test is the failing-first proof for the /compact override, assert the positive outcome (e.g. result?.rejectionCause is undefined / the handler returns a compaction or ran applyCompaction) instead of only the absence of the lane rejection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/test/claude-sdk-oauth-compaction-alignment.test.ts, line 238:
<comment>The manual-compaction test only asserts that the result is not rejected as `external-owner`, so it also passes when the handler returns `undefined` and no compaction actually ran. Since this test is the failing-first proof for the `/compact` override, assert the positive outcome (e.g. `result?.rejectionCause` is undefined / the handler returns a compaction or ran `applyCompaction`) instead of only the absence of the lane rejection.</comment>
<file context>
@@ -227,6 +227,17 @@ describe("claude-sdk-oauth lane: senpi compaction stands down", () => {
+ // The delegation covers senpi's AUTOMATIC compaction only. `/compact` is the
+ // user's escape hatch for the case the lane cannot detect: the SDK did not
+ // compact and the session has no other way back under the limit.
+ expect(result?.rejectionCause).not.toBe("external-owner");
+ });
+
</file context>
Suggested change
| expect(result?.rejectionCause).not.toBe("external-owner"); | |
| expect(result?.rejectionCause).toBeUndefined(); | |
| expect(harness.applyCompaction).toHaveBeenCalled(); |
| const model = selector ? this._modelRegistry.find(selector.provider, selector.id) : undefined; | ||
| if (!model) return false; | ||
| const originalThinkingLevel = active.originalThinkingLevel; | ||
| await this.setSessionModel(model); |
There was a problem hiding this comment.
P3: Restore bypasses the retry controller's revert semantics. It clears state only via the side effect of setSessionModel -> clearForManualModelChange (which does not emit), so no retry_fallback_reverted event fires and the switch is recorded as an ordinary manual model change rather than a fallback revert. Consider restoring through a controller-level revert/switchModel(...,"fallback-revert") so history and the retry_fallback_reverted event stay consistent.
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 6890:
<comment>Restore bypasses the retry controller's revert semantics. It clears state only via the side effect of `setSessionModel` -> `clearForManualModelChange` (which does not emit), so no `retry_fallback_reverted` event fires and the switch is recorded as an ordinary manual model change rather than a fallback revert. Consider restoring through a controller-level revert/`switchModel(...,"fallback-revert")` so history and the `retry_fallback_reverted` event stay consistent.</comment>
<file context>
@@ -6880,6 +6880,17 @@ export class AgentSession {
+ const model = selector ? this._modelRegistry.find(selector.provider, selector.id) : undefined;
+ if (!model) return false;
+ const originalThinkingLevel = active.originalThinkingLevel;
+ await this.setSessionModel(model);
+ if (originalThinkingLevel !== undefined) this.setSessionThinkingLevel(originalThinkingLevel);
+ return true;
</file context>
Owner
|
Thanks @eddieparc — the compact-override diagnosis is still right, and it is not superseded. What already landed, so we do not double-count it:
Remaining work on this PR:
Suggested tests after rebase (must pass in one run): |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add two explicit, session-only recovery surfaces:
/fallback restorereturns the current session to the model and thinking level active before retry fallback;/compactcan explicitly override Claude SDK OAuth's automatic-compaction delegation.This is intentionally separate from the persistence/root-cause fixes in #1267.
Behavior
/fallback restoremanual
/compactClaude SDK OAuth continues to own all automatic compaction reasons. Only
reason: "manual"bypasses the lane delegation, providing an escape hatch when SDK-native compaction did not fire.The existing post-compaction continuity path remains unchanged: the binding is tainted, the next turn forks at the last assistant boundary, and delta traffic resumes after re-attachment.
Failing-first proof
restorecommand leaves the session onfaux-2; the host-wiring test expects restoration tofaux-1;expected 'external-owner' not to be 'external-owner'.Verification
bun run check: passed;QA evidence is stored locally under
local-ignore/qa-evidence/20260902-recovery-commands/.Summary by cubic
Adds
/fallback restoreso a session can return to the model used before retry fallback, restoring the prior thinking level when one was active. Also makes a manual/compactoverrideclaude-sdk-oauth's automatic-compaction delegation; previously every/compacton that lane was rejected withexternal-owner. Both commands are session-only and leave global defaults unchanged.New Features
/fallback restoreclears active fallback state and warns when no fallback is active.manualis the only compaction reason that bypasses the lane delegation; all automatic reasons are still delegated to the SDK.Written for commit 23150e5. Summary will update on new commits.