feat(compaction): add modelFallbacks for compaction model resolution#52012
feat(compaction): add modelFallbacks for compaction model resolution#52012scottgl9 wants to merge 1 commit intoopenclaw:mainfrom
Conversation
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..." |
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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f27c7d613e
ℹ️ 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".
|
Re P1 (PRRT_kwDOQb6kR854znUK) — Preserve runtime-auth exchange state after model auth prep Fixed in ca7cbf7d9c. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca7cbf7d9c
ℹ️ 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: 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.
Pass context token budget into prepared settings
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.
Include modelContextTokens in ctx window resolution
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.
Restore think-level retry before failing compaction
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.
Persist checkpoints after direct compaction succeeds
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.
Pass senderIsOwner into action discovery context
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.
Compute post-compaction metrics from fallback session state
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.
Honor configured prompt overrides in compaction prompt build
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.
Wrap provider stream before assigning compaction session streamFn
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.
Respect heartbeat policy when setting compaction heartbeat prompt
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 👍 / 👎.
|
Codex review: found issues before merge. Summary Reproducibility: yes. A high-confidence static reproduction exists by comparing PR head Next step before merge Security Review findings
Review detailsBest possible solution: If maintainers want explicit compaction fallback configuration, implement it as a narrow additive patch on current main that reuses the existing fallback/session-preparation pipeline and preserves the explicit-model semantics introduced by #74470. Do we have a high-confidence way to reproduce the issue? Yes. A high-confidence static reproduction exists by comparing PR head Is this the best way to solve the issue? No. The feature direction is useful, but this PR is not the best implementation because it rewrites stale compaction internals; the safer solution is a small current-main change that layers explicit fallback config onto the existing pipeline. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 2d849bbafa41. |
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