Auto-reply: honor compaction model for memory flush#51159
Auto-reply: honor compaction model for memory flush#51159johnkhagler wants to merge 466 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes memory flush to use compaction-aware model fallback resolution (
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/agent-runner-utils.test.ts
Line: 238-241
Comment:
**Unrelated test assertion removed — reduces coverage**
The removal of `currentThreadTs: "928"` from this `toMatchObject` expectation is outside the stated scope of this PR (memory flush model resolution). The PR description explicitly says "no non-memory-flush execution paths changed," but `buildThreadingToolContext` is unrelated to memory flush.
Since `toMatchObject` only checks for _inclusion_, the removal of this assertion means the test no longer verifies that `currentThreadTs` is correctly set from `MessageThreadId: 928`. If the Telegram plugin stopped populating `currentThreadTs`, this test would still pass silently.
Was this intentional? If so, it should be in its own commit/PR with an explanation. If not, consider restoring the assertion:
```suggestion
expect(context).toMatchObject({
currentChannelId: "telegram:-1003841603622",
currentThreadTs: "928",
currentMessageId: "2284",
});
```
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/auto-reply/reply/agent-runner-utils.ts
Line: 178-189
Comment:
**Edge case: model starting with `/` falls through to model-only path with leading slash**
If `agents.defaults.compaction.model` is set to a value like `"/some-model"`, `slashIdx` will be `0`, which fails the `> 0` guard and falls through to:
```ts
return { ...resolved, model: override }; // model === "/some-model"
```
The resulting model string would contain a leading `/`, which is almost certainly not the intended behavior. This is an unlikely config value, but it could produce a confusing runtime error.
Consider using `slashIdx > 0 && provider && model` with an explicit check, or document the `> 0` constraint in a comment to clarify the intent.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "Tests: align telegra..." |
![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: 8b5c2f91cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@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: 934a3ffcba
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Addressed bot feedback in follow-up commit 934a3ff:
I also replied to/resolved applicable bot threads. This is ready for maintainer review. |
johnkhagler
force-pushed
the
main
branch
2 times, most recently
from
1dcd841 to
fb22d98
Compare
|
Refreshed this PR branch to current upstream main while preserving the memory-flush compaction-model fix. What changed:
Validation I ran locally in sandbox runtime:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb22d98f66
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try { | ||
| await runWithModelFallback({ | ||
| ...resolveModelFallbackOptions(params.followupRun.run), | ||
| ...resolveCompactionModelFallbackOptions(params.followupRun.run), |
There was a problem hiding this comment.
This call now executes memory flush on agents.defaults.compaction.model, but flush gating is still computed earlier from params.followupRun.run.model via resolveMemoryFlushContextWindowTokens/shouldRunMemoryFlush. When the compaction override has a smaller context window than the primary model (e.g., primary 200k, compaction 32k), flush can trigger too late and this compaction run can overflow/fail before memory is written. The gating model/window should be derived from the same resolved compaction model used here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15ed1fbaa2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const trimmed = fallback.trim(); | ||
| if (!trimmed || trimmed.includes("/")) { | ||
| return trimmed; | ||
| } | ||
| return `${provider}/${trimmed}`; |
There was a problem hiding this comment.
This rewrite prefixes every slash-less fallback with the compaction provider, which changes alias entries into literal provider/model strings. In runWithModelFallback, alias resolution only happens for raw fallback strings without /, so a config like fallbacks: ["cheap"] (where cheap is a configured alias) no longer resolves after compaction.model switches provider. In memory-flush fallback scenarios this can skip valid alias targets and fail over incorrectly.
Useful? React with 👍 / 👎.
|
Rebased onto latest upstream/main (38adeb8) as of 2026-03-27. No conflicts. Fix logic and tests are unchanged — the compaction model override for memory flush still applies cleanly. Happy to answer any questions. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b42eca97d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (params.sessionKey ? params.sessionStore?.[params.sessionKey] : undefined); | ||
| const contextWindowTokens = resolveMemoryFlushContextWindowTokens({ | ||
| modelId: params.followupRun.run.model ?? params.defaultModel, | ||
| modelId: compactionFallbackOptions.model ?? params.defaultModel, |
There was a problem hiding this comment.
This line feeds resolveMemoryFlushContextWindowTokens with compactionFallbackOptions.model, but that value is the raw suffix from agents.defaults.compaction.model (for example aurora-alpha from openrouter/aurora-alpha). lookupContextTokens is exact-key based, while the fallback runner normalizes some provider/model pairs (e.g. OpenRouter native ids become openrouter/aurora-alpha), so flush gating can miss the real context window and fall back to agentCfgContextTokens/DEFAULT_CONTEXT_TOKENS. In practice this can trigger memory flush too late (risking overflow on smaller-window models) or too early (extra flush churn) whenever the compaction override needs model normalization.
Useful? React with 👍 / 👎.
* Net: block missing IPv6 special-use ranges * Tests: refresh public IPv6 pinning fixtures
* Extensions: require admin for config write commands * Tests: cover phone control disarm auth
* Gateway: align chat.send reset scope checks * Gateway: tighten chat.send reset regression test * Gateway: honor internal provider reset scope
* Discord: gate text approvals by approver policy * Discord: require approvers for plugin text approvals * Discord: preserve legacy text approval fallback
* nextcloud-talk: throttle repeated webhook auth failures Co-authored-by: Brian Mendonca <208517100+bmendonca3@users.noreply.github.com> * nextcloud-talk: scope webhook auth limiter per server * nextcloud-talk: limit repeated webhook auth failures only --------- Co-authored-by: Brian Mendonca <208517100+bmendonca3@users.noreply.github.com>
* Discord: apply component interaction policy gates Co-authored-by: nexrin <268879349+nexrin@users.noreply.github.com> * Discord: pass carbon rate limit request * Discord: reply to blocked component interactions --------- Co-authored-by: nexrin <268879349+nexrin@users.noreply.github.com>
* Telegram: tighten media SSRF policy * Telegram: restrict media downloads to configured hosts * Telegram: preserve custom media apiRoot hosts
openclaw-barnacle
bot
added
channel: twitch
|
Closing this PR because it looks dirty (too many unrelated or unexpected changes). This usually happens when a branch picks up unrelated commits or a merge went sideways. Please recreate the PR from a clean branch. |
1 similar comment
|
Closing this PR because it looks dirty (too many unrelated or unexpected changes). This usually happens when a branch picks up unrelated commits or a merge went sideways. Please recreate the PR from a clean branch. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f8b4d6094
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!trimmed || trimmed.includes("/")) { | ||
| return trimmed; | ||
| } | ||
| return `${provider}/${trimmed}`; |
There was a problem hiding this comment.
Rewriting every slash-less fallback to ${provider}/${fallback} breaks configured model aliases during memory-flush fallback. In runWithModelFallback, alias resolution only runs for raw strings without a slash, so an alias like "cheap" now becomes "openrouter/cheap" and is treated as a literal model id instead of resolving through agents.defaults.models[*].alias. This makes valid compaction fallback chains fail specifically when agents.defaults.compaction.model switches providers and fallbacks include aliases.
Useful? React with 👍 / 👎.
| const canAttemptFlush = memoryFlushWritable && !params.isHeartbeat && !isCli; | ||
| let entry = | ||
| params.sessionEntry ?? | ||
| (params.sessionKey ? params.sessionStore?.[params.sessionKey] : undefined); | ||
| const contextWindowTokens = resolveMemoryFlushContextWindowTokens({ | ||
| modelId: params.followupRun.run.model ?? params.defaultModel, | ||
| modelId: compactionFallbackOptions.model ?? params.defaultModel, |
There was a problem hiding this comment.
Memory-flush gating now looks up context tokens from compactionFallbackOptions.model, but that value is the raw suffix from agents.defaults.compaction.model and may be unnormalized (for example "aurora-alpha" from "openrouter/aurora-alpha"). lookupContextTokens is exact-key based, while runtime model resolution normalizes some provider/model pairs (notably OpenRouter native IDs), so this can miss the true context window and compute a wrong flush threshold, causing late flushes (overflow risk) or premature flushes.
Useful? React with 👍 / 👎.
17 participants
Summary
Describe the problem and fix in 2–5 bullets:
Problem: memory flush used generic model fallback resolution instead of compaction-aware resolution.
Why it matters: when agents.defaults.compaction.model is set, memory flush could still run on the primary fallback path, causing inconsistent behavior.
What changed: memory flush now resolves model fallbacks through compaction-aware logic, with focused helper and test updates.
What did NOT change: no non-memory-flush execution paths changed, no config key renames, no auth/token flow changes, no UI changes.
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
-If yes, exact upgrade steps: N/A
Failure Recovery (if this breaks)
Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.