feat(memory-core): configurable MEMORY.md injection mode + per-turn autoRecall#63662
feat(memory-core): configurable MEMORY.md injection mode + per-turn autoRecall#63662ZardLi1115 wants to merge 10 commits into
Conversation
Greptile SummaryThis PR adds Two P2 concerns worth revisiting before wider adoption: (1) Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/clarity issues. The core logic is sound: src/agents/bootstrap-files.ts —
|
| function applyMemoryInjectionFilter( | ||
| files: WorkspaceBootstrapFile[], | ||
| config?: OpenClawConfig, | ||
| ): WorkspaceBootstrapFile[] { | ||
| if (resolveMemoryInjectionMode(config) !== "recall-only") { | ||
| return files; | ||
| } | ||
| return files.filter( | ||
| (file) => file.name !== DEFAULT_MEMORY_FILENAME && file.name !== DEFAULT_MEMORY_ALT_FILENAME, | ||
| ); | ||
| } |
There was a problem hiding this comment.
core-only and full modes are currently identical
applyMemoryInjectionFilter only gates on "recall-only", so "core-only" passes through unchanged — the same as "full". The docs describe them differently ("core-only keeps it inside the normal bootstrap size limits"), but the code applies identical truncation in both cases. Users who set "core-only" get no behavioural difference from "full", which could cause confusion.
If the intent is to diverge them later (e.g. "full" will eventually bypass bootstrapMaxChars for MEMORY.md), that should be noted in the doc comment. Otherwise, one of the modes or the documentation description should be updated to match reality.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/bootstrap-files.ts
Line: 43-53
Comment:
**`core-only` and `full` modes are currently identical**
`applyMemoryInjectionFilter` only gates on `"recall-only"`, so `"core-only"` passes through unchanged — the same as `"full"`. The docs describe them differently ("core-only keeps it inside the normal bootstrap size limits"), but the code applies identical truncation in both cases. Users who set `"core-only"` get no behavioural difference from `"full"`, which could cause confusion.
If the intent is to diverge them later (e.g. `"full"` will eventually bypass `bootstrapMaxChars` for `MEMORY.md`), that should be noted in the doc comment. Otherwise, one of the modes or the documentation description should be updated to match reality.
How can I resolve this? If you propose a fix, please make it concise.| function shouldWarnForRecallOnlyWithoutAutoRecall(cfg: OpenClawConfig): boolean { | ||
| return cfg.agents?.defaults?.memoryInjection === "recall-only" && !hasAnyAutoRecallEnabled(cfg); | ||
| } |
There was a problem hiding this comment.
autoRecall
shouldWarnForRecallOnlyWithoutAutoRecall fires only when no agent at all has autoRecall enabled. If memoryInjection: "recall-only" is global but only one specific agent has autoRecall enabled (while others rely on the default false), the warning is suppressed — leaving the other agents with no memory context silently. Consider checking whether the defaults also enable autoRecall before suppressing the warning, or narrowing the message to indicate that only some agents may lose memory context.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/auto-recall.ts
Line: 59-61
Comment:
**Warning suppresses when any single agent enables `autoRecall`**
`shouldWarnForRecallOnlyWithoutAutoRecall` fires only when *no* agent at all has `autoRecall` enabled. If `memoryInjection: "recall-only"` is global but only one specific agent has `autoRecall` enabled (while others rely on the default `false`), the warning is suppressed — leaving the other agents with no memory context silently. Consider checking whether the defaults also enable `autoRecall` before suppressing the warning, or narrowing the message to indicate that only some agents may lose memory context.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec9c365b86
ℹ️ 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 (resolveMemoryInjectionMode(config) !== "recall-only") { | ||
| return files; |
There was a problem hiding this comment.
Implement distinct behavior for memoryInjection core-only
applyMemoryInjectionFilter only branches on "recall-only", so "core-only" and "full" currently follow the exact same path and inject the same bootstrap files. That makes the new core-only mode effectively a no-op for runtime behavior, so users who select it won’t get a different memory-injection policy. Please add explicit core-only handling (or collapse the option) so the setting has observable effect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c05d65e5b8
ℹ️ 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 (cfg.agents?.defaults?.memorySearch?.autoRecall?.enabled) { | ||
| return undefined; |
There was a problem hiding this comment.
Warn when recall-only is paired with disabled memory search
This early return assumes autoRecall.enabled means recall is active, but resolveMemorySearchConfig returns null whenever memorySearch.enabled is false (src/agents/memory-search.ts:410-412). With memoryInjection: "recall-only", memorySearch.enabled: false, and autoRecall.enabled: true, bootstrap memory is removed and auto-recall never injects anything, yet startup emits no warning, so users can end up with zero memory context and no clear signal about the misconfiguration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf7a675778
ℹ️ 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 (!hasAnyAutoRecallEnabled(api.config)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Always register auto-recall hook
This early return makes autoRecall impossible to enable via hot config reload: if the gateway starts with auto-recall off, the before_prompt_build hook is never registered, so later toggling agents.defaults.memorySearch.autoRecall.enabled (or agent-level overrides) has no runtime effect until a process/plugin restart. That is a functional regression for live config updates, especially since this hook already reads api.runtime.config.loadConfig() per turn and agents.* changes are generally treated as non-restart config paths.
Useful? React with 👍 / 👎.
…ion-auto-recall # Conflicts: # src/auto-reply/reply/followup-runner.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17e65a7e67
ℹ️ 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 (channel && isThreadChild) { | ||
| if (channel === "slack" || channel === "mattermost" || channel === "telegram") { | ||
| return { | ||
| to: formatConversationTarget({ |
There was a problem hiding this comment.
Let plugin resolver run before Telegram thread fallback
This new early return executes for Telegram thread children before pluginTarget is consulted, so messaging.resolveDeliveryTarget in the Telegram plugin is skipped for exactly the cases where channel-specific normalization is needed. For topic conversations, this returns threadId as the raw conversationId instead of the plugin-normalized topic id, which can produce inconsistent routing keys and thread targeting compared with the plugin contract.
Useful? React with 👍 / 👎.
| if (cfg.agents?.defaults?.memorySearch?.autoRecall?.enabled) { | ||
| return undefined; |
There was a problem hiding this comment.
Warn when agent overrides disable autoRecall in recall-only mode
resolveRecallOnlyAutoRecallWarning returns early as soon as defaults have memorySearch.autoRecall.enabled=true, but agent-level overrides can still set autoRecall.enabled=false. In that configuration, those agents run with memoryInjection: "recall-only" plus no auto-recall (because resolveMemorySearchConfig applies the override), so they lose all automatic memory context without the startup warning this guard is meant to provide.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed June 5, 2026, 1:02 AM ET / 05:02 UTC. Summary PR surface: Source +228, Tests +378, Docs +67, Generated +94, Other +23. Total +790 across 24 files. Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Review detailsBest possible solution: Retry the Codex review after fixing the execution failure. Do we have a high-confidence way to reproduce the issue? Unclear. The review failed before ClawSweeper could establish a reproduction path. Is this the best way to solve the issue? Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction. AGENTS.md: unclear because the file could not be read completely. Codex review notes: model gpt-5.5, reasoning high; reviewed against e0018382eb00. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +228, Tests +378, Docs +67, Generated +94, Other +23. Total +790 across 24 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
Real user data point — running 2026.5.7 with active-memory denied (per latency triage 2026-04-29: 10-11s elapsed + 16k cache_read + 19k cache_create per call on a 25-char message via stock model). Per-turn autoRecall would let us re-enable with a sane frequency (e.g. every-3rd-turn) instead of all-or-nothing. We're a multi-channel deployment (Discord direct + channel + Telegram); the lack of memory recall is causing real engineering rework — agents re-discover prior decisions on every conversation. +1. |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
Summary
agents.defaults.memoryInjectionwithfull,core-only, andrecall-onlymodesagents.defaults.memorySearch.autoRecallfor per-turn memory recall injection inmemory-coreValidation
corepack pnpm buildcorepack pnpm test src/agents/bootstrap-files.test.ts src/agents/memory-search.test.ts src/config/config.schema-regressions.test.ts extensions/memory-core/src/auto-recall.test.ts extensions/memory-core/index.test.ts src/config/schema.base.generated.test.tsCloses #24624