fix(memory-core): register memory tools independently to prevent coupled failure#52639
fix(memory-core): register memory tools independently to prevent coupled failure#52639frankekn wants to merge 4 commits intoopenclaw:mainfrom
Conversation
…led failure The memory-core plugin registered both memory_search and memory_get in a single factory that returned null if either tool creation failed. This caused both tools to silently disappear from new sessions when only one tool's prerequisites were unavailable. Split into two independent registerTool calls so each tool's availability is evaluated separately. Also add debug logging when a plugin tool factory returns null to aid future diagnosis. Fixes openclaw#50173 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
openclaw-barnacle
bot
added
channel: telegram
Greptile SummaryThis PR fixes a coupling bug in Key changes:
Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/memory-core/index.test.ts
Line: 33
Comment:
**Test nested under wrong describe block**
The new regression test exercises `plugin.register(api)`, not `buildPromptSection`. Nesting it inside the `describe("buildPromptSection", ...)` block makes it harder to find and slightly misleading. Consider moving it to its own top-level block, e.g. `describe("plugin.register", ...)`, to keep test intent clear.
```suggestion
describe("plugin registration", () => {
it("registers memory tools independently so one unavailable tool does not suppress the other", () => {
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: CHANGELOG.md
Line: 83
Comment:
**Changelog entry in wrong section**
This is a bug fix (coupled failure causing silent tool loss), so it belongs under `### Fixes` rather than `### Changes`. Placing it in `### Changes` may mislead consumers who rely on changelog sections to assess upgrade risk.
```suggestion
- Memory/core tools: register `memory_search` and `memory_get` independently so one unavailable memory tool no longer suppresses the other in new sessions. (#50198) Thanks @artwalker.
```
(Move this line to the `### Fixes` section instead.)
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: cover independent memory-core regis..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@codex review |
![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: 98145de58c
ℹ️ 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".
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🔵 Log forging / terminal control injection via unsanitized pluginId and tool names in debug log
DescriptionThe plugin tool resolution path logs plugin-controlled strings without output neutralization:
Vulnerable code: log.debug(
`plugin tool factory returned null (${entry.pluginId}): [${entry.names.join(", ")}]`,
);RecommendationAvoid interpolating untrusted/plugin-controlled identifiers directly into console log lines. Option A (preferred): log structured fields and let the logger encode them (and keep the message constant): log.debug("plugin tool factory returned null", {
pluginId: sanitizeForLog(entry.pluginId),
toolNames: entry.names.map(sanitizeForLog),
});Option B: sanitize before interpolation by stripping control characters and ANSI escapes: function sanitizeForLog(value: string): string {
// remove CR/LF and other control chars; optionally strip ANSI escapes too
return value
.replace(/[\r\n\t\0\x08\f\v]/g, " ")
.replace(/[\x00-\x1F\x7F]/g, "");
}
log.debug(
`plugin tool factory returned null (${sanitizeForLog(entry.pluginId)}): ` +
`[${entry.names.map(sanitizeForLog).join(", ")}]`,
);Additionally consider applying stricter validation on Analyzed PR: #52639 at commit Last updated on: 2026-03-23T05:58:39Z |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3bb2d2883
ℹ️ 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".
|
|
||
| ### Fixes | ||
|
|
||
| - Memory/core tools: register `memory_search` and `memory_get` independently so one unavailable memory tool no longer suppresses the other in new sessions. (#50198) Thanks @artwalker. |
There was a problem hiding this comment.
This new bullet is inserted at the top of the active ### Fixes block, but AGENTS.md requires changelog entries to be appended at the end of the target section. Keeping it at the top breaks the repo’s documented release-note ordering and increases avoidable merge conflicts when multiple fixes land in parallel, so this line should be moved to the tail of the current ### Fixes list.
Useful? React with 👍 / 👎.
2 participants
Summary
memory-coreregisteredmemory_searchandmemory_getin one factory, so if either tool was unavailable the factory returnednulland both tools disappeared from new sessions.extensions/memory-core/index.ts, keep the PR's debug log insrc/plugins/tools.ts, add a regression test for independent registration, and add the required changelog line.check/testafter rebasing the replacement branch.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
New sessions no longer lose both
memory_searchandmemory_getwhen only one of the memory tool factories is unavailable.Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
origin/mainSteps
extensions/memory-core/index.tson current main: oneregisterTool(...)factory creates both tools and returnsnullif either tool is unavailable.null.origin/main.Expected
memory_searchstays registered whenmemory_getis unavailable.Actual
Evidence
Commands run:
pnpm buildpnpm checkpnpm testHuman Verification (required)
What you personally verified (not just CI), and how:
memory_search/memory_getregistration via the new regression inextensions/memory-core/index.test.ts, plus fullbuild/check/teston the rebased replacement branch.nullno longer suppresses the other tool; changelog entry is appended at the tail of## Unreleased->### Changes.Review Conversations
Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
extensions/memory-core/index.ts,src/plugins/tools.ts,extensions/memory-core/index.test.ts,CHANGELOG.md.Risks and Mitigations