feat(compaction): add modelFallbacks for compaction model resolution#52012
feat(compaction): add modelFallbacks for compaction model resolution#52012scottgl9 wants to merge 1 commit into
Conversation
openclaw-barnacle
Bot
added
docs
Greptile SummaryThis PR adds There is one correctness bug in the fallback auth-profile logic worth addressing before merge:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-embedded-runner/compact.ts
Line: 483-486
Comment:
**Auth profile not restored when fallback matches original provider**
The condition only clears `authProfileId` when `fbProvider` differs from `params.provider`, but it never restores it when they match. This means if the primary compaction override uses a different provider (which already cleared `authProfileId` to `undefined` at lines 424–427), and a fallback then resolves to the original `params.provider`, `authProfileId` remains `undefined` even though `params.authProfileId` would be the correct credential to use for that provider.
This causes silent auth failures — `getApiKeyForModel` receives `profileId: undefined` and falls back to unscoped key resolution instead of the user's configured profile.
```suggestion
if (fbProvider !== (params.provider ?? "").trim()) {
authProfileId = undefined;
} else {
authProfileId = params.authProfileId;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/compact.ts
Line: 495-497
Comment:
**Error message omits exhausted fallbacks**
When all fallbacks also fail, the error returned is either the primary model's `error` string or `Unknown model: {primaryProvider}/{primaryModelId}`. This gives the user no indication that fallback attempts were also made and failed, which can make debugging confusing.
Consider accumulating failure reasons and surfacing them in the final message, e.g.:
```ts
// After the fallback loop, if no model was resolved:
const triedModels = [`${provider}/${modelId}`, ...(fallbacks ?? [])].join(", ");
const reason = error ?? `All compaction models failed to resolve (tried: ${triedModels})`;
return fail(reason);
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "feat(compaction): ad..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 9a8f301e1b
ℹ️ 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".
|
Thanks for the review! Both issues addressed in 1d60b67:
Also fixed the TS type-check failure by avoiding mutable destructuring — now uses explicit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58b808df91
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b598c1be54
ℹ️ 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".
fda22a0 to
5136655
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51366552dd
ℹ️ 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".
5136655 to
3c98c59
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c98c59b89
ℹ️ 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".
3a5c4c8 to
a83b020
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a83b0208cf
ℹ️ 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".
|
Conflicts resolved (merged main → branch) and all remaining review items addressed:
Branch: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1075941a0a
ℹ️ 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".
1f0584c to
d86d217
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d86d21796c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9103aa59f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ad4536006
ℹ️ 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".
6ad4536 to
f27c7d6
Compare
openclaw-barnacle
Bot
removed
docs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70c3c58141
ℹ️ 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".
70c3c58 to
a7fe08d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7fe08d71a
ℹ️ 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".
a7fe08d to
a452f00
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a452f00176
ℹ️ 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".
a452f00 to
c9793db
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9793dba80
ℹ️ 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 settingsManager = createPreparedEmbeddedPiSettingsManager({ | ||
| cwd: effectiveWorkspace, | ||
| agentDir, | ||
| cfg: params.config, | ||
| contextTokenBudget: ctxInfo.tokens, | ||
| }); |
There was a problem hiding this comment.
This call no longer provides contextTokenBudget, so createPreparedEmbeddedPiSettingsManager cannot cap compaction reserve settings to the active model window. In src/agents/pi-settings.ts, the small-context safety cap only runs when contextTokenBudget is present; without it, the default 20k reserve floor can exceed 16k-class models and drive repeated overflow/compaction behavior instead of stable compaction.
Useful? React with 👍 / 👎.
| cfg: params.config, | ||
| provider, | ||
| modelId, | ||
| modelContextTokens: readPiModelContextTokens(runtimeModel), | ||
| modelContextWindow: runtimeModelWithContext.contextWindow, | ||
| modelContextWindow: runtimeModel.contextWindow, | ||
| defaultTokens: DEFAULT_CONTEXT_TOKENS, |
There was a problem hiding this comment.
This compaction path now computes ctxInfo from runtimeModel.contextWindow only, so models that publish limits via contextTokens are ignored here and can fall back to DEFAULT_CONTEXT_TOKENS. That can inflate the token budget used for compaction and lead to oversized prompts or avoidable overflow failures; the inconsistency is visible because the queued compaction path still reads contextTokens explicitly.
Useful? React with 👍 / 👎.
c9793db to
e134d3e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e134d3e06a
ℹ️ 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".
| !compactFallbacks || | ||
| !Array.isArray(compactFallbacks) || | ||
| compactFallbacks.length === 0 | ||
| ) { | ||
| throw compactErr; |
There was a problem hiding this comment.
This branch now throws immediately when modelFallbacks is unset, but the previous flow retried once with pickFallbackThinkingLevel (typically off -> minimal) for reasoning-required providers. In environments that rely on that compatibility retry and do not configure modelFallbacks, compaction now fails outright on provider rejections that would previously recover on the second attempt.
Useful? React with 👍 / 👎.
| await runPostCompactionSideEffects({ | ||
| config: params.config, | ||
| sessionKey: params.sessionKey, | ||
| sessionFile: params.sessionFile, | ||
| }); |
There was a problem hiding this comment.
After a successful compact, this path now proceeds directly to side effects/metrics without persisting a compaction checkpoint snapshot. I checked src/agents/pi-embedded-runner/compact.queued.ts: checkpoint persistence there is gated behind contextEngine.info.ownsCompaction === true, while the default legacy engine delegates into compactEmbeddedPiSessionDirect, so standard compaction flows lose fresh checkpoints for sessions.compaction.list/restore.
Useful? React with 👍 / 👎.
| sessionId: params.sessionId, | ||
| agentId: sessionAgentId, | ||
| senderId: params.senderId, | ||
| senderIsOwner: params.senderIsOwner, | ||
| }), |
There was a problem hiding this comment.
This discovery call no longer forwards senderIsOwner, so compaction prompt action discovery runs without ownership context even when the caller is an owner. That can suppress owner-scoped channel actions/capabilities during compaction and make tool guidance differ from normal run-time action discovery for the same session.
Useful? React with 👍 / 👎.
e134d3e to
cef8926
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cef8926457
ℹ️ 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 tokensAfter = estimateTokensAfterCompaction({ | ||
| messagesAfter: session.messages, | ||
| observedTokenCount, | ||
| fullSessionTokensBefore, |
There was a problem hiding this comment.
When session.compact(...) fails and fallback compaction succeeds, the code runs compaction on a separate fbSession (tryCompactWithModel) but then computes tokensAfter from session.messages here. Because the primary session never runs the successful fallback compact(), this can report pre-compaction token counts and propagate stale values to hooks/callers, which can in turn trigger incorrect follow-up compaction decisions.
Useful? React with 👍 / 👎.
| const systemPromptOverride = createSystemPromptOverride(appendPrompt); | ||
|
|
There was a problem hiding this comment.
This now hardwires the compaction prompt to buildEmbeddedSystemPrompt(...) and wraps it directly, but the prior flow applied resolveSystemPromptOverride(...) (and provider prompt transforms) before setting the session override. In setups that rely on agent-level systemPromptOverride or provider prompt hooks, compaction requests will diverge from normal runtime prompts and can regress provider-specific behavior.
Useful? React with 👍 / 👎.
Adds modelFallbacks?: string[] to AgentCompactionConfig. When the
primary compaction model fails (resolution, auth, or runtime), each
fallback is tried in order before giving up.
Changes:
- types.agent-defaults.ts: add modelFallbacks?: string[]
- zod-schema.agent-defaults.ts: add modelFallbacks schema
- compact.ts:
- Model resolution fallback loop (primary model not in registry)
- Auth fallback loop with tryAuthForModel returning hasExchange
- Runtime fallback loop: on session.compact() throw, retry with full
re-resolve + re-auth + createAgentSession per fallback
- fbEffectiveModel now passes through applyLocalNoAuthHeaderOverride
and applyAuthHeaderOverride using fbKeyInfo/fbHasExchange (P1 fix)
- tryCompactWithModel disposes fbSession in finally block (P2 fix)
- All error messages surface tried models list
- Backward compatible when modelFallbacks is unset
cef8926 to
1c6b2f9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c6b2f91d6
ℹ️ 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".
| if (providerStreamFn) { | ||
| session.agent.streamFn = providerStreamFn; | ||
| } |
There was a problem hiding this comment.
This direct assignment bypasses the compaction stream preparation stack that previously went through resolveEmbeddedAgentStreamFn and extra-param/provider wrappers. In provider-owned transport paths, that wrapper layer is what injects resolved runtime credentials (including exchanged keys from prepareProviderRuntimeAuth) and applies transport/extra-param behavior; without it, compaction can still fail authentication or ignore provider/runtime stream settings even after model auth succeeded.
Useful? React with 👍 / 👎.
| heartbeatPrompt: isDefaultAgent | ||
| ? resolveHeartbeatPrompt(params.config?.agents?.defaults?.heartbeat?.prompt) | ||
| : undefined, |
There was a problem hiding this comment.
Using resolveHeartbeatPrompt(...) here unconditionally injects a heartbeat prompt for the default agent, even when heartbeat guidance should be suppressed by policy (for example includeSystemPromptSection: false or disabled cadence). This regresses prompt parity with the existing heartbeat-system-prompt policy path and can cause /compact prompts to include unintended HEARTBEAT instructions.
Useful? React with 👍 / 👎.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: the remaining compaction/summary fallback API work is already tracked by the broader canonical issue, while this branch is conflicting, lacks real behavior proof, and would regress current-main compaction auth/session paths if merged as-is. Canonical path: Keep the API/product decision in #56781, then land a narrow current-main implementation that preserves existing auth, stream-wrapper, checkpoint, and session-metrics behavior. So I’m closing this here and keeping the remaining discussion on #56781. Review detailsBest possible solution: Keep the API/product decision in #56781, then land a narrow current-main implementation that preserves existing auth, stream-wrapper, checkpoint, and session-metrics behavior. Do we have a high-confidence way to reproduce the issue? Yes for the current capability gap and PR regressions by source inspection: explicit Is this the best way to solve the issue? No. The requested fallback behavior is reasonable, but this stale branch is not the best path because the public API is now canonicalized in a broader issue and the implementation regresses current-main compaction invariants. Security review: Security review needs attention: The diff has no dependency or workflow supply-chain change, but it introduces concrete auth-routing regressions in compaction credential and stream resolution.
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against b79effefee92. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
ClawSweeper applied the proposed close for this PR.
|
1 participant
Closes #52011
Summary
When the primary compaction model override is unreachable (e.g. a local vLLM server is down), compaction currently fails immediately. This adds a modelFallbacks array to AgentCompactionConfig that allows specifying fallback models tried in order before giving up.
Changes
Config Example
{ "agents": { "defaults": { "compaction": { "model": "vllm/qwen3-coder-next", "modelFallbacks": ["anthropic/claude-haiku-4-5"] } } } }Testing