Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAlma plugin migrated from nmem CLI to Nowledge Mem HTTP API with deterministic thread IDs and append-first flush; OpenClaw plugin adds optional corpus-supplement integration, cron/isolated-session capture exclusions, and related config/manifest bumps; Hermes plugin install path/imports fixed; registry formatting and manifest versions updated. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
participant Alma as Alma Plugin
participant HTTP as HTTP Client (_fetch)
participant API as Nowledge Mem API
participant Store as Memory Store
end
Alma->>HTTP: GET /memories/search?q=...
HTTP->>API: HTTP GET with Authorization: Bearer
API->>Store: query graph
Store-->>API: results (relevance_score)
API-->>HTTP: JSON results
HTTP-->>Alma: mapped results
Alma->>HTTP: POST /threads/{stableId}/append (messages)
HTTP->>API: POST append (Authorization: Bearer)
API->>Store: append to thread
Store-->>API: success
API-->>HTTP: thread response
HTTP-->>Alma: confirmation
sequenceDiagram
rect rgba(220,255,200,0.5)
participant OpenClaw as OpenClaw Plugin
participant MemCore as Memory Core
participant Supplement as Corpus Supplement
participant API as Nowledge Mem API
end
Note over OpenClaw,MemCore: Dreaming/recall cycle
MemCore->>Supplement: search({query, maxResults})
Supplement->>API: client.search(query)
API-->>Supplement: results + scores
Supplement->>Supplement: filter by corpusMinScore, map/truncate
Supplement-->>MemCore: MemoryCorpusSearchResult[]
MemCore->>MemCore: three-phase promotion -> MEMORY.md
Note over OpenClaw: regular recall hooks
OpenClaw->>OpenClaw: if cfg.corpusSupplement -> skip search-based recall
OpenClaw->>OpenClaw: inject Working Memory only
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Register Nowledge Mem as a searchable corpus inside memory-core's three-phase dreaming pipeline. Fix WM injection on short messages, null guard in corpus get(), CE version bump, and Codex CHANGELOG historical accuracy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
setup.sh installed to plugins/memory/nowledge-mem/ but Hermes scans direct children of plugins/. Bare imports (from provider, from client) fail under Hermes' importlib loading; switched to relative imports. Added migration step for users who already installed to the old path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Thread identity was session-scoped in-memory state. Plugin restarts,
Alma relaunches, and LRU buffer evictions caused conversations to be
saved as new threads. Now uses deterministic IDs (alma-{sha1[:12]})
and try-append-first to reconnect to existing threads.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All data operations now use fetch() to the Nowledge Mem HTTP API instead of spawning the nmem CLI synchronously. This eliminates event-loop blocking that caused Alma to freeze during recall injection and thread sync, especially on Windows where Python process startup is slow. API key passed via Authorization header. CLI retained only for status tool diagnostics (user-initiated, not on hot path). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Memory search used `query` instead of `q`, causing FastAPI to silently ignore the search term and return browse-mode results. Label filter used `filter_labels` instead of `labels`, time filter sent enum values to `event_date_from` instead of `time_range`. All three broke recall injection, dedup, and search tools without any visible error. Also removes ~83 lines of dead code (saveActiveThread, normalizeThreadMessages, stringifyMessage, phantom tool params) and updates the OpenClaw status tool to present memory-core users with both configuration paths instead of a single "switch to us" warning. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… string
Two bugs found during fresh API audit:
1. Dedup guard reads `memory.score` but the search endpoint returns
`relevance_score`. Result: dedup check always sees 0, never blocks
duplicate saves. Fix: read `relevance_score` with `score` fallback.
2. sourceThreadId extraction reads `source_thread` expecting a string,
but the backend returns `{id, title}` object. Result: downstream
thread_show calls receive "[object Object]" instead of a thread ID.
Fix: extractThreadId() helper handles both string and object shapes.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
OpenClaw stores isolated cron runs as agent:<id>:cron:... (e.g. cron-worker). captureExclude defaulted to empty, so those short automation transcripts were synced like user chat. Add isCronCaptureSessionKey (mirrors OpenClaw isCronSessionKey + bare cron: prefix) and apply it in agent_end and before_reset capture handlers before user glob patterns. Made-with: Cursor
When CE is active, captures run via afterTurn instead of agent_end hooks; apply the same isCronCaptureSessionKey guard so cron-worker runs stay out of Mem threads. Made-with: Cursor
Update CHANGELOG and captureExclude UI copy: cron/isolated-agent keys are always skipped; captureExclude remains for subagents and custom patterns. Made-with: Cursor
…-sdk Delegate agent-scoped cron detection to isCronSessionKey from openclaw/plugin-sdk/routing; keep a narrow bare cron: prefix rule for internal keys the SDK does not parse. Updates plugin changelog and captureExclude copy to match. Made-with: Cursor
…ron exclusion Keeps community/integrations.json aligned with INTEGRATION_UNIFICATION and the plugin implementation. Made-with: Cursor
The import `from "openclaw/plugin-sdk/routing"` crashes the plugin on load with ERR_PACKAGE_PATH_NOT_EXPORTED: the openclaw package only exports `./plugin-sdk` and `./plugin-sdk/account-id`, not `./plugin-sdk/routing`. isCronSessionKey exists in the dist bundle but is not reachable through the exports map. Inlines the detection logic (parse agent:<id>:cron:... format) which is a stable session key contract. Updates changelog and config copy to remove SDK-delegation claims. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous commit inlined isCronSessionKey based on a stale local node_modules (v2026.2.22). The 3pp source confirms openclaw/plugin-sdk/routing exports isCronSessionKey since v2026.3.22, and the plugin already requires >=2026.4.5. Restores the SDK import and updates JSDoc with the minimum version note. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
@codex can you help review our PR? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integrations.json (1)
226-227:⚠️ Potential issue | 🟡 MinorUpdate Alma integration version from 0.6.13 to 0.6.14 in integrations.json.
The Alma plugin version in
integrations.json(line 226) shows0.6.13, but the plugin'sCHANGELOG.mdandmanifest.jsonboth document version0.6.14with the Async HTTP transport migration.integrations.jsonmust remain synchronized with current plugin releases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations.json` around lines 226 - 227, Update the Alma plugin version entry in integrations.json by changing the "version" value for the integration with "directory": "nowledge-mem-alma-plugin" from "0.6.13" to "0.6.14"; locate the object matching that directory and update its "version" field so integrations.json matches the plugin's manifest and CHANGELOG reflecting the Async HTTP transport migration.
🧹 Nitpick comments (1)
nowledge-mem-openclaw-plugin/src/corpus-supplement.js (1)
88-106: Harden mapped search records against malformed upstream fields.
idandscoreare forwarded without validation. Defensive normalization avoids invalid corpus paths andNaNscores entering memory-core ranking.Proposed refactor
- return filtered.map((m) => { + return filtered + .filter((m) => typeof m?.id === "string" && m.id.length > 0) + .map((m) => { + const numericScore = Number(m.score); const snippet = String(m.content || "").slice(0, SNIPPET_MAX_CHARS); const lineCount = Math.max(1, snippet.split(/\r?\n/u).length); const result = { corpus: "nowledge-mem", path: `nowledgemem://memory/${m.id}`, - score: Number(m.score ?? 0), + score: Number.isFinite(numericScore) ? numericScore : 0, snippet, title: m.title || undefined, kind: m.labels?.[0] || undefined, id: m.id, startLine: 1, endLine: lineCount, provenanceLabel: "Nowledge Mem", source: "nowledge-mem", sourceType: "knowledge-graph", }; return result; - }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-openclaw-plugin/src/corpus-supplement.js` around lines 88 - 106, The mapped records must defensively normalize m.id and m.score inside the filtered.map callback: ensure m.id is present and convertible to a safe string (coerce via String(m.id) or skip the record if null/undefined), sanitize it for the corpus path (e.g., encodeURIComponent or remove unsafe chars) before building path `nowledgemem://memory/${...}`, and ensure score is a finite Number (use Number(m.score) and fallback to 0 if NaN or !isFinite). Update the mapping in the filtered.map block that computes snippet/lineCount and builds result so path and score cannot be malformed, and consider omitting or skipping records with totally invalid ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nowledge-mem-alma-plugin/main.js`:
- Around line 1463-1467: deactivate() currently cannot flush per-thread buffers
because those buffers are closed over inside activate() and only flushed by quit
hooks; modify the design so deactivate() can flush and clear buffered messages:
either lift the thread buffer state and a flushAllBuffers() helper to
module-scope (so both activate() and deactivate() can call it), or change
activate() to return a dispose() that both schedules quit hooks and exposes a
flush() method which deactivate() invokes before clearing timers; ensure the
unique symbols deactivate, activate, dispose (and any thread buffer variables
used in activate) are updated so deactivate calls the flush path to flush
buffered thread messages as well as removing timers.
- Around line 98-100: The _fetch() error handling currently throws a generic
stringified message which loses HTTP status info used by cliErrorResult() to
detect 404/403 for nowledge_mem_show, nowledge_mem_delete,
nowledge_mem_thread_show and nowledge_mem_thread_delete; change the throw to
create an Error object that preserves structured details (e.g., set
error.message to the text and attach error.status = resp.status and error.body =
text or truncated text) so cliErrorResult() can inspect err.status (and/or
err.body) to correctly route notFound/permission branches.
In `@nowledge-mem-hermes/setup.sh`:
- Around line 51-57: The script currently deletes the old plugin at
OLD_PLUGIN_DIR before confirming the new install succeeded; change the migration
to first download/install the new plugin to a temporary location (or create a
backup of OLD_PLUGIN_DIR), validate the new package (e.g., check for plugin.yaml
in the temp dir), and only then remove or replace OLD_PLUGIN_DIR and rmdir
"$HERMES_HOME/plugins/memory" if appropriate; in other words, update setup.sh to
perform the download/validation into a temp path, verify success, then
atomically move the new plugin into place (or remove the old after backup)
instead of running rm -rf "$OLD_PLUGIN_DIR" unconditionally.
In `@nowledge-mem-openclaw-plugin/package.json`:
- Line 3: Add a new section header for the 0.8.0 release to CHANGELOG.md
matching the package.json/openclaw.plugin.json version: insert a line like
"[0.8.0] - YYYY-MM-DD" (replace YYYY-MM-DD with the actual release date) above
the changelog notes for this release so the changelog contains the required
release section for version 0.8.0.
In `@nowledge-mem-openclaw-plugin/src/corpus-supplement.js`:
- Around line 46-57: In sliceLines, when from/start is beyond the available
lines selected becomes empty but endLine is set to start (making downstream
lineCount appear as 1); change the endLine calculation so that if
selected.length === 0 you return an explicit empty range (e.g., endLine = start
- 1) otherwise compute endLine = start + selected.length - 1 and clamp to
totalLines; ensure startLine, endLine, text, and totalLines are returned
consistently so downstream lineCount becomes 0 for an empty selection.
In `@nowledge-mem-openclaw-plugin/src/index.js`:
- Around line 84-102: The runtime registration may fail but downstream modules
still read cfg.corpusSupplement; change the code that currently wraps
api.registerMemoryCorpusSupplement(...) and
createNowledgeMemCorpusSupplement(...) so it sets a runtime flag
corpusSupplementActive = true only after successful registration (and false on
catch/failure), and export or attach that flag to the same runtime object (e.g.,
api or client) used by other modules; then update the consumers that currently
check cfg.corpusSupplement (notably the logic in recall.js, context-engine.js,
and tools/status.js) to read this runtime corpusSupplementActive flag instead of
cfg.corpusSupplement so they reflect actual registration state.
In `@nowledge-mem-openclaw-plugin/src/tools/status.js`:
- Around line 65-87: The current info-only branch for when memorySlot is set but
not "openclaw-nowledge-mem" hides the specific misconfiguration of pointing at
"memory-core"; modify the branch that checks memorySlot && memorySlot !==
"openclaw-nowledge-mem" to explicitly detect memorySlot === "memory-core" and
push a warning line (e.g., a clear "WARNING: memory slot is set to
\"memory-core\" — should be \"openclaw-nowledge-mem\" for this plugin") before
the existing corpusSupplement-based messages; keep the existing two-option
guidance and corpusSupplement messaging intact, but ensure the memory-core
warning appears first so it is surfaced prominently by the nowledge_mem_status
diagnostic code that constructs the lines array.
---
Outside diff comments:
In `@integrations.json`:
- Around line 226-227: Update the Alma plugin version entry in integrations.json
by changing the "version" value for the integration with "directory":
"nowledge-mem-alma-plugin" from "0.6.13" to "0.6.14"; locate the object matching
that directory and update its "version" field so integrations.json matches the
plugin's manifest and CHANGELOG reflecting the Async HTTP transport migration.
---
Nitpick comments:
In `@nowledge-mem-openclaw-plugin/src/corpus-supplement.js`:
- Around line 88-106: The mapped records must defensively normalize m.id and
m.score inside the filtered.map callback: ensure m.id is present and convertible
to a safe string (coerce via String(m.id) or skip the record if null/undefined),
sanitize it for the corpus path (e.g., encodeURIComponent or remove unsafe
chars) before building path `nowledgemem://memory/${...}`, and ensure score is a
finite Number (use Number(m.score) and fallback to 0 if NaN or !isFinite).
Update the mapping in the filtered.map block that computes snippet/lineCount and
builds result so path and score cannot be malformed, and consider omitting or
skipping records with totally invalid ids.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f3223b0-cb18-46b2-95c5-85eeba5dc607
⛔ Files ignored due to path filters (1)
nowledge-mem-openclaw-plugin/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
integrations.jsonnowledge-mem-alma-plugin/CHANGELOG.mdnowledge-mem-alma-plugin/CLAUDE.mdnowledge-mem-alma-plugin/main.jsnowledge-mem-alma-plugin/manifest.jsonnowledge-mem-codex-plugin/CHANGELOG.mdnowledge-mem-hermes/CHANGELOG.mdnowledge-mem-hermes/README.mdnowledge-mem-hermes/__init__.pynowledge-mem-hermes/plugin.yamlnowledge-mem-hermes/provider.pynowledge-mem-hermes/setup.shnowledge-mem-openclaw-plugin/CHANGELOG.mdnowledge-mem-openclaw-plugin/CLAUDE.mdnowledge-mem-openclaw-plugin/README.mdnowledge-mem-openclaw-plugin/openclaw.plugin.jsonnowledge-mem-openclaw-plugin/package.jsonnowledge-mem-openclaw-plugin/src/config.jsnowledge-mem-openclaw-plugin/src/context-engine.jsnowledge-mem-openclaw-plugin/src/corpus-supplement.jsnowledge-mem-openclaw-plugin/src/hooks/capture.jsnowledge-mem-openclaw-plugin/src/hooks/recall.jsnowledge-mem-openclaw-plugin/src/index.jsnowledge-mem-openclaw-plugin/src/tools/status.js
![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: 233f3ecb62
ℹ️ 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.
Actionable comments posted: 2
♻️ Duplicate comments (2)
nowledge-mem-alma-plugin/main.js (2)
1465-1467:⚠️ Potential issue | 🟠 Major
deactivate()still allows silent loss of unsaved buffered messages.On plugin reload/disable (without quit hooks), pending thread buffers are dropped. Flush capability needs to be shared with
deactivate().💡 Suggested direction
- // Thread buffers are flushed by quit hooks registered in activate(). - // deactivate() cannot access those buffers (they're scoped to activate's closure). + // Flush pending buffers on deactivate as well (shared flush helper/module-scope state). + await flushAllBuffers?.();Move
threadBuffers+ aflushAllBuffers()helper to shared scope (or exposeflush()fromactivate()and call it here) so both quit hooks anddeactivate()execute the same drain path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-alma-plugin/main.js` around lines 1465 - 1467, deactivate() can silently drop pending threadBuffers; to fix, move the threadBuffers data structure and the drain logic into shared scope (or implement and return a flushAllBuffers/flush function from activate()) so both the quit hooks and deactivate() call the same flush path; update code to reference the shared threadBuffers and call flushAllBuffers() (or the returned flush) from deactivate(), ensuring the same buffer-drain logic used by activate()'s quit hooks is reused.
98-101:⚠️ Potential issue | 🟠 MajorPreserve structured HTTP status on errors so tool-level
notFound/permission handling keeps working.Throwing only a stringified message drops reliable status-based classification, so delete/show/thread tools can miss their
notFoundbranch.💡 Proposed fix
- throw new Error(`HTTP ${resp.status}: ${text.slice(0, 200)}`); + const error = new Error(`HTTP ${resp.status}: ${text.slice(0, 200)}`); + error.status = resp.status; + error.body = text.slice(0, 200); + throw error;// In cliErrorResult(), prefer status when available: const status = err && typeof err === "object" ? err.status : undefined; if (status === 404) code = "not_found"; else if (status === 401 || status === 403) code = "permission_denied";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-alma-plugin/main.js` around lines 98 - 101, The current error handling throws a plain Error containing only a stringified message (from resp.text()), losing the numeric HTTP status needed by higher-level tooling; change the throw to preserve the status as structured data (e.g., throw an Error-like object or custom Error instance that includes a status property and the truncated body message) instead of only a string. Locate the HTTP response branch that uses resp (the block that checks if (!resp.ok)) and replace the throw new Error(...) with something like creating an object { name: "HttpError", status: resp.status, message: `HTTP ${resp.status}: ${text.slice(0,200)}` } or instantiate an Error and assign err.status = resp.status so callers (and cliErrorResult) can read err.status to map 404 → not_found and 401/403 → permission_denied.
🧹 Nitpick comments (1)
nowledge-mem-openclaw-plugin/CLAUDE.md (1)
72-72: Clarify the phrasing about default usage.The sentence "Most users have Nowledge Mem as the memory slot (default
false)" is confusing. It appears to state that most users have Nowledge Mem as the memory slot, but then indicates the default isfalse, which seems contradictory.📝 Suggested clarification
-Enable when memory-core is the memory slot and you want cross-tool knowledge to participate in OpenClaw's native recall and dreaming. Most users have Nowledge Mem as the memory slot (default `false`). +Enable when memory-core is the memory slot and you want cross-tool knowledge to participate in OpenClaw's native recall and dreaming. Most users have Nowledge Mem configured as the memory slot, so corpus supplement should remain disabled (default `false`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-openclaw-plugin/CLAUDE.md` at line 72, Reword the confusing sentence that reads "Most users have Nowledge Mem as the memory slot (default `false`)" so it clearly separates the user behavior from the default setting; for example replace it with something like: "Most users choose Nowledge Mem as their memory slot. This option is disabled by default (`false`) and should only be enabled when memory-core is set as the memory slot and you want cross-tool knowledge to participate in OpenClaw's recall and dreaming." Update the CLAUDE.md sentence to that clearer wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nowledge-mem-alma-plugin/CLAUDE.md`:
- Around line 10-12: Update the Settings section to match the Memory backend
description: change the line that claims `apiKey` is passed via env var only so
it documents that for the HTTP memory backend the API key is transported via an
Authorization header (e.g., "Authorization: Bearer <API_KEY>"), and note that an
env var (e.g., NMEM_API_KEY) can still be used for CLI/diagnostic tools; ensure
the Settings wording references "Memory backend / HTTP" and the `apiKey`
identifier so the file no longer contradicts the header-auth statement in the
Memory backend paragraph.
In `@nowledge-mem-alma-plugin/main.js`:
- Around line 1282-1288: The current catch block for client.appendThread (inside
the try around logger.info and await client.appendThread) creates a new thread
for any appendErr; change it to only fallback to client.createThread when the
appendErr is a definitive "not found" error (e.g., check
appendErr.code/status/message for a 404 or "not found" sentinel) and rethrow or
surface other errors (timeouts, 5xx, auth) instead of attempting createThread;
update the catch to inspect appendErr (the variable used in the diff) and only
call client.createThread(buf.nowledgeThreadId, ...) when the error indicates the
thread is missing, otherwise propagate the error.
---
Duplicate comments:
In `@nowledge-mem-alma-plugin/main.js`:
- Around line 1465-1467: deactivate() can silently drop pending threadBuffers;
to fix, move the threadBuffers data structure and the drain logic into shared
scope (or implement and return a flushAllBuffers/flush function from activate())
so both the quit hooks and deactivate() call the same flush path; update code to
reference the shared threadBuffers and call flushAllBuffers() (or the returned
flush) from deactivate(), ensuring the same buffer-drain logic used by
activate()'s quit hooks is reused.
- Around line 98-101: The current error handling throws a plain Error containing
only a stringified message (from resp.text()), losing the numeric HTTP status
needed by higher-level tooling; change the throw to preserve the status as
structured data (e.g., throw an Error-like object or custom Error instance that
includes a status property and the truncated body message) instead of only a
string. Locate the HTTP response branch that uses resp (the block that checks if
(!resp.ok)) and replace the throw new Error(...) with something like creating an
object { name: "HttpError", status: resp.status, message: `HTTP ${resp.status}:
${text.slice(0,200)}` } or instantiate an Error and assign err.status =
resp.status so callers (and cliErrorResult) can read err.status to map 404 →
not_found and 401/403 → permission_denied.
---
Nitpick comments:
In `@nowledge-mem-openclaw-plugin/CLAUDE.md`:
- Line 72: Reword the confusing sentence that reads "Most users have Nowledge
Mem as the memory slot (default `false`)" so it clearly separates the user
behavior from the default setting; for example replace it with something like:
"Most users choose Nowledge Mem as their memory slot. This option is disabled by
default (`false`) and should only be enabled when memory-core is set as the
memory slot and you want cross-tool knowledge to participate in OpenClaw's
recall and dreaming." Update the CLAUDE.md sentence to that clearer wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9dd87d1-83b4-40da-b0f7-a788a6dfbc93
⛔ Files ignored due to path filters (1)
nowledge-mem-openclaw-plugin/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
integrations.jsonnowledge-mem-alma-plugin/CHANGELOG.mdnowledge-mem-alma-plugin/CLAUDE.mdnowledge-mem-alma-plugin/main.jsnowledge-mem-alma-plugin/manifest.jsonnowledge-mem-codex-plugin/CHANGELOG.mdnowledge-mem-hermes/CHANGELOG.mdnowledge-mem-hermes/README.mdnowledge-mem-hermes/__init__.pynowledge-mem-hermes/plugin.yamlnowledge-mem-hermes/provider.pynowledge-mem-hermes/setup.shnowledge-mem-openclaw-plugin/CHANGELOG.mdnowledge-mem-openclaw-plugin/CLAUDE.mdnowledge-mem-openclaw-plugin/README.mdnowledge-mem-openclaw-plugin/openclaw.plugin.jsonnowledge-mem-openclaw-plugin/package.jsonnowledge-mem-openclaw-plugin/src/config.jsnowledge-mem-openclaw-plugin/src/context-engine.jsnowledge-mem-openclaw-plugin/src/corpus-supplement.jsnowledge-mem-openclaw-plugin/src/hooks/capture.jsnowledge-mem-openclaw-plugin/src/hooks/recall.jsnowledge-mem-openclaw-plugin/src/index.jsnowledge-mem-openclaw-plugin/src/tools/status.js
✅ Files skipped from review due to trivial changes (9)
- nowledge-mem-hermes/provider.py
- nowledge-mem-codex-plugin/CHANGELOG.md
- nowledge-mem-hermes/README.md
- nowledge-mem-hermes/plugin.yaml
- nowledge-mem-hermes/init.py
- nowledge-mem-openclaw-plugin/package.json
- nowledge-mem-hermes/CHANGELOG.md
- nowledge-mem-alma-plugin/manifest.json
- integrations.json
🚧 Files skipped from review as they are similar to previous changes (7)
- nowledge-mem-hermes/setup.sh
- nowledge-mem-openclaw-plugin/src/hooks/capture.js
- nowledge-mem-openclaw-plugin/src/hooks/recall.js
- nowledge-mem-openclaw-plugin/src/tools/status.js
- nowledge-mem-openclaw-plugin/src/context-engine.js
- nowledge-mem-openclaw-plugin/openclaw.plugin.json
- nowledge-mem-openclaw-plugin/src/corpus-supplement.js
Member
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
P1: OpenClaw corpus supplement registration failure now resets cfg.corpusSupplement to false, so recall hooks fall back to normal search instead of silently skipping all memory recall. P2: Alma _fetch() errors now carry a .status property. The append-first thread reconnect only falls back to create on 404 (not-found), rethrowing transient errors (5xx, timeout) instead of masking them with a create attempt. P2: Hermes setup.sh defers old plugin path deletion until after the new files download successfully. P3: Alma createThread removes dead empty if-block for content param (backend POST /threads has no summary field). CLAUDE.md corrects apiKey transport description (Bearer header, not env var only). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
bugbot run |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nowledge-mem-alma-plugin/main.js (1)
1467-1471:⚠️ Potential issue | 🟠 MajorPlugin reload/disable still risks losing buffered messages.
The comment at lines 1469-1470 acknowledges that
deactivate()cannot access thread buffers (scoped insideactivate()). While quit hooks handle normal app exit, a plugin reload/disable bypasses them, potentially losing unflushed messages.Consider lifting
threadBuffersandflushThreadto module scope sodeactivate()can flush before returning:💡 Suggested direction
+// Module-scope for deactivate() access +let threadBuffers = null; +let flushThreadFn = null; + export async function activate(context) { // ... - const threadBuffers = new Map(); + threadBuffers = new Map(); // ... + flushThreadFn = flushThread; // ... } export async function deactivate(context) { const logger = context?.logger ?? console; + if (threadBuffers && flushThreadFn) { + for (const [tid, buf] of threadBuffers) { + if (buf.messages.length >= 2 && buf.messages.length > buf.savedCount) { + await flushThreadFn(tid).catch(() => {}); + } + } + } logger.info?.("nowledge-mem deactivated"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-alma-plugin/main.js` around lines 1467 - 1471, deactivate() cannot access the per-activate thread buffers, so lift the buffer state and flush helper to module scope and call the flush from deactivate to avoid lost messages: move threadBuffers and flushThread out of activate() into top-level module variables/functions, update activate() to reference the shared module-scope threadBuffers/flushThread, and add a flushThread loop call inside deactivate() (before logger.info) to synchronously flush any remaining buffers; ensure function names threadBuffers, flushThread and activate/deactivate are used so you update all references.
🧹 Nitpick comments (1)
nowledge-mem-alma-plugin/main.js (1)
310-326:cliErrorResultshould checkerr.statusfor HTTP error classification.The function inspects
message.toLowerCase()for keywords like "not found", but HTTP errors may return 404 with an empty body or JSON error body that doesn't contain those words. Since_fetch()now attacheserr.status(line 101), this function should prioritize status codes:♻️ Suggested improvement
function cliErrorResult(err, operation) { const message = err instanceof Error ? err.message : String(err); + const status = err && typeof err === "object" ? err.status : undefined; const normalized = message.toLowerCase(); let code = "cli_error"; - if (normalized.includes("not found")) code = "not_found"; - if (normalized.includes("permission")) code = "permission_denied"; + if (status === 404 || normalized.includes("not found")) code = "not_found"; + if (status === 403 || normalized.includes("permission")) code = "permission_denied"; if (normalized.includes("invalid json")) code = "invalid_json";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-alma-plugin/main.js` around lines 310 - 326, cliErrorResult currently classifies errors by inspecting err.message text; instead first check for an HTTP status attached by _fetch (err.status) and map common codes (e.g., 404 -> "not_found", 403 -> "permission_denied", 422 or 400 -> "invalid_json" or a suitable client error code, 500+ -> "model_unavailable" or "cli_error") before falling back to the existing message.toLowerCase() keyword checks; update the function cliErrorResult to prioritize err.status from _fetch and only use message keyword matching as a fallback so HTTP responses with empty/opaque bodies are classified correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nowledge-mem-alma-plugin/main.js`:
- Around line 1467-1471: deactivate() cannot access the per-activate thread
buffers, so lift the buffer state and flush helper to module scope and call the
flush from deactivate to avoid lost messages: move threadBuffers and flushThread
out of activate() into top-level module variables/functions, update activate()
to reference the shared module-scope threadBuffers/flushThread, and add a
flushThread loop call inside deactivate() (before logger.info) to synchronously
flush any remaining buffers; ensure function names threadBuffers, flushThread
and activate/deactivate are used so you update all references.
---
Nitpick comments:
In `@nowledge-mem-alma-plugin/main.js`:
- Around line 310-326: cliErrorResult currently classifies errors by inspecting
err.message text; instead first check for an HTTP status attached by _fetch
(err.status) and map common codes (e.g., 404 -> "not_found", 403 ->
"permission_denied", 422 or 400 -> "invalid_json" or a suitable client error
code, 500+ -> "model_unavailable" or "cli_error") before falling back to the
existing message.toLowerCase() keyword checks; update the function
cliErrorResult to prioritize err.status from _fetch and only use message keyword
matching as a fallback so HTTP responses with empty/opaque bodies are classified
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2e65b4a-b4c9-40f9-a28b-6e9d92b24a78
📒 Files selected for processing (5)
nowledge-mem-alma-plugin/CLAUDE.mdnowledge-mem-alma-plugin/main.jsnowledge-mem-alma-plugin/package.jsonnowledge-mem-hermes/setup.shnowledge-mem-openclaw-plugin/src/index.js
✅ Files skipped from review due to trivial changes (1)
- nowledge-mem-alma-plugin/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- nowledge-mem-hermes/setup.sh
…Memory
BugBot round 2:
1. cliErrorResult now checks err.status (404 -> not_found, 403 ->
permission_denied) in addition to string matching. Fixes 404
responses with empty body being classified as generic errors
instead of not_found.
2. Store tool uses client.createMemory(body) instead of raw
client._fetch("POST", "/memories", ...). Restores encapsulation
after addMemory() was removed in the dead-code cleanup.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
bugbot run |
Member
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 09e8d03. Configure here.
Member
Author
|
@codex kindly review them all? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09e8d0331e
ℹ️ 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".
Member
Author
|
bugbot run |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nowledge-mem-alma-plugin/CLAUDE.md (1)
157-157: Optional: make backend-signature reference more stable.Consider adding a pinned commit/tag or a direct file link in addition to the repo path so this guidance doesn’t silently drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-alma-plugin/CLAUDE.md` at line 157, Update the guidance line in CLAUDE.md that currently points to nowledge-graph-py/src/nowledge_graph_server/api/routers/ so it uses a stable reference: replace or supplement the repo-relative path with a direct permalink to the specific file(s) and/or add a pinned commit SHA or tag (e.g., append @<commit-sha-or-tag>) so the backend-signature reference cannot drift; keep the existing phrasing ("When modifying API calls...") but include the permanent URL and mention the commit/tag so reviewers know which backend version (nowledge-graph-py/src/nowledge_graph_server/api/routers/ at <commit-or-tag>) to verify against.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nowledge-mem-alma-plugin/CLAUDE.md`:
- Line 157: Update the guidance line in CLAUDE.md that currently points to
nowledge-graph-py/src/nowledge_graph_server/api/routers/ so it uses a stable
reference: replace or supplement the repo-relative path with a direct permalink
to the specific file(s) and/or add a pinned commit SHA or tag (e.g., append
@<commit-sha-or-tag>) so the backend-signature reference cannot drift; keep the
existing phrasing ("When modifying API calls...") but include the permanent URL
and mention the commit/tag so reviewers know which backend version
(nowledge-graph-py/src/nowledge_graph_server/api/routers/ at <commit-or-tag>) to
verify against.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ceab9e53-56b4-4ec5-901f-d5c3d8521fa3
📒 Files selected for processing (2)
nowledge-mem-alma-plugin/CLAUDE.mdnowledge-mem-alma-plugin/main.js
🚧 Files skipped from review as they are similar to previous changes (1)
- nowledge-mem-alma-plugin/main.js
Member
Author
|
bugbot run |
Member
Author
|
@codex kindly review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0d42df7. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Note
Medium Risk
Medium risk due to switching Alma from sync CLI calls to async HTTP API calls and changing thread ID/sync behavior, which could affect persistence and recall correctness if API assumptions differ. OpenClaw adds a new optional corpus-supplement integration into memory-core’s recall/dreaming pipeline, which changes how recall is sourced when enabled.
Overview
Alma plugin switches all memory/thread operations from
nmemCLIspawnSyncto async HTTPfetch()withAuthorization: Bearer, updates query param mapping (q/labels/time_range), and simplifies tool schemas by removing CLI-only parameters (contentLimit,force, truncation metadata). Thread syncing is hardened by deriving a stablealma-{sha1(...)}thread id, attempting append-before-create on first flush, snapshotting message counts, and flushing buffers ondispose()to reduce duplicates across restarts/eviction.OpenClaw plugin bumps to
0.8.0, adds optionalcorpusSupplementcapability (plus max/min settings) to register aMemoryCorpusSupplementfor memory-core recall/dreaming and disables duplicate search-based recall when active; it also updates capture behavior/docs to exclude cron/isolated sessions viaisCronSessionKey.Hermes plugin fixes plugin discoverability by installing to
~/.hermes/plugins/nowledge-mem/, migrates old installs, and updates imports to relative; registry/docs/changelogs are updated with version bumps and corrected Codex skill naming.Reviewed by Cursor Bugbot for commit caf6f46. Bugbot is set up for automated code reviews on this repo. Configure here.