refactor: streamline error messages and command execution in NmemClient#134
Merged
wey-gu merged 2 commits intonowledge-co:mainfrom Mar 18, 2026
Merged
refactor: streamline error messages and command execution in NmemClient#134wey-gu merged 2 commits intonowledge-co:mainfrom
wey-gu merged 2 commits intonowledge-co:mainfrom
Conversation
Signed-off-by: Frost Ming <me@frostming.com>
📝 WalkthroughWalkthroughClient formatting and error-decoding were adjusted to prefer Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Plugin
participant Nmem as Nmem/MemoryStore
rect rgba(200,200,255,0.5)
Client->>Plugin: call save_state(message)
Plugin->>Plugin: slice user content, build messages
alt thread in known_threads
Plugin->>Nmem: append to thread (append call)
Nmem-->>Plugin: success / error
else thread not known
Plugin->>Nmem: append to thread (append call)
alt append success
Plugin->>Plugin: record thread in known_threads
else append NmemError (404-like)
Plugin->>Nmem: create thread (create call)
Nmem-->>Plugin: thread created
Plugin->>Plugin: record thread in known_threads
end
end
Plugin-->>Client: return (no exception), log warnings on failures
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nowledge-mem-bub-plugin/src/nowledge_mem_bub/client.py (1)
71-71: Include the preferreduvxinstall path in the missing-binary hint.The message is correct, but including both
uvx(preferred) andpippaths would reduce setup friction across environments.💡 Suggested tweak
- raise NmemError("nmem not found in PATH. Install with: pip install nmem-cli") + raise NmemError( + "nmem not found in PATH. Use `uvx --from nmem-cli nmem` (recommended) " + "or install with `pip install nmem-cli`." + )Based on learnings: Install Nowledge Mem CLI with either
uvx --from nmem-cli nmem(recommended) orpip install nmem-cli.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-bub-plugin/src/nowledge_mem_bub/client.py` at line 71, Update the missing-binary error message raised via NmemError to include the preferred uvx install command alongside the pip option; locate the raise NmemError(...) in client.py (the check that currently raises "nmem not found in PATH. Install with: pip install nmem-cli") and change the message to mention both "uvx --from nmem-cli nmem" (recommended) and "pip install nmem-cli" so users see both installation paths.
🤖 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-bub-plugin/src/nowledge_mem_bub/client.py`:
- Around line 104-109: The timeout and FileNotFoundError handlers in the async
subprocess block should reap the killed process and chain exceptions: in the
asyncio.TimeoutError except clause (around the await asyncio.wait_for call
handling proc.communicate) capture the exception (e.g., as err), call
proc.kill() and then await proc.wait() (or proc.communicate()) to ensure the
process is reaped before raising, and raise NmemError(f"nmem timed out after
{timeout}s") from err; similarly, in the FileNotFoundError except clause capture
the exception and raise NmemError("nmem not found in PATH") from err so the
original traceback is preserved. Use the existing symbols proc, timeout,
NmemError and the surrounding asyncio.wait_for/proc.communicate logic to locate
and update the code.
---
Nitpick comments:
In `@nowledge-mem-bub-plugin/src/nowledge_mem_bub/client.py`:
- Line 71: Update the missing-binary error message raised via NmemError to
include the preferred uvx install command alongside the pip option; locate the
raise NmemError(...) in client.py (the check that currently raises "nmem not
found in PATH. Install with: pip install nmem-cli") and change the message to
mention both "uvx --from nmem-cli nmem" (recommended) and "pip install nmem-cli"
so users see both installation paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43ca51c2-135b-4a22-a29d-01fe63e95b64
📒 Files selected for processing (1)
nowledge-mem-bub-plugin/src/nowledge_mem_bub/client.py
Comment on lines
+104
to
109
| stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=timeout) | ||
| except asyncio.TimeoutError: | ||
| proc.kill() # type: ignore[union-attr] | ||
| raise NmemError(f"nmem timed out after {timeout}s") | ||
| except FileNotFoundError: | ||
| raise NmemError("nmem not found in PATH") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'except asyncio\.TimeoutError|proc\.kill\(|await proc\.wait\(|except FileNotFoundError|raise NmemError' nowledge-mem-bub-plugin/src/nowledge_mem_bub/client.pyRepository: nowledge-co/community
Length of output: 1269
Reap timed-out subprocess and chain caught exceptions.
After proc.kill(), wait for process termination before raising. Also preserve causal traceback with raise ... from err in both except blocks.
🔧 Proposed fix
- try:
+ proc: asyncio.subprocess.Process | None = None
+ try:
proc = await asyncio.create_subprocess_exec(
*cmd,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
env=self._build_env(),
)
stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=timeout)
- except asyncio.TimeoutError:
- proc.kill() # type: ignore[union-attr]
- raise NmemError(f"nmem timed out after {timeout}s")
- except FileNotFoundError:
- raise NmemError("nmem not found in PATH")
+ except asyncio.TimeoutError as err:
+ if proc is not None and proc.returncode is None:
+ proc.kill()
+ await proc.wait()
+ raise NmemError(f"nmem timed out after {timeout}s") from err
+ except FileNotFoundError as err:
+ raise NmemError("nmem not found in PATH") from err🧰 Tools
🪛 Ruff (0.15.6)
[warning] 107-107: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 109-109: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nowledge-mem-bub-plugin/src/nowledge_mem_bub/client.py` around lines 104 -
109, The timeout and FileNotFoundError handlers in the async subprocess block
should reap the killed process and chain exceptions: in the asyncio.TimeoutError
except clause (around the await asyncio.wait_for call handling proc.communicate)
capture the exception (e.g., as err), call proc.kill() and then await
proc.wait() (or proc.communicate()) to ensure the process is reaped before
raising, and raise NmemError(f"nmem timed out after {timeout}s") from err;
similarly, in the FileNotFoundError except clause capture the exception and
raise NmemError("nmem not found in PATH") from err so the original traceback is
preserved. Use the existing symbols proc, timeout, NmemError and the surrounding
asyncio.wait_for/proc.communicate logic to locate and update the code.
…ility Signed-off-by: Frost Ming <me@frostming.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nowledge-mem-bub-plugin/src/nowledge_mem_bub/plugin.py (1)
186-198:⚠️ Potential issue | 🟠 MajorStale known thread IDs bypass the 404 recovery path.
On Line 186, if a thread is in
self._known_threadsbut was deleted remotely,append_threadfails and never attemptscreate_thread. Since the ID stays inself._known_threads, future captures keep failing.Suggested fix
- if thread_id in self._known_threads: - await self.client.append_thread(thread_id, messages_json) - else: - try: - await self.client.append_thread(thread_id, messages_json) - self._known_threads.add(thread_id) - except NmemError as exc: - err_msg = str(exc).lower() - if "not found" not in err_msg and "404" not in err_msg: - raise # timeout, auth, network — don't mask - title = f"Bub Session ({session_id[:30]})" - await self.client.create_thread(thread_id, title, messages_json) - self._known_threads.add(thread_id) + try: + await self.client.append_thread(thread_id, messages_json) + self._known_threads.add(thread_id) + except NmemError as exc: + err_msg = str(exc).lower() + if "not found" not in err_msg and "404" not in err_msg: + raise + title = f"Bub Session ({session_id[:30]})" + await self.client.create_thread(thread_id, title, messages_json) + self._known_threads.add(thread_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-bub-plugin/src/nowledge_mem_bub/plugin.py` around lines 186 - 198, The current logic skips the 404 recovery when thread_id is present in self._known_threads because append_thread is only wrapped in a try/except in the else branch; update the append flow so that any call to self.client.append_thread(thread_id, messages_json) (both when thread is known and when not) catches NmemError, inspects the error string (lowercased) for "not found" or "404", and on that case removes the stale thread_id from self._known_threads, calls self.client.create_thread(thread_id, f"Bub Session ({session_id[:30]})", messages_json) and re-adds the id; for other NmemError cases re-raise. Target symbols: self._known_threads, append_thread, create_thread, NmemError, session_id, messages_json.
🤖 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-bub-plugin/src/nowledge_mem_bub/plugin.py`:
- Around line 176-177: The thread_id generation uses
hashlib.sha1(session_id).hexdigest()[:10], which yields only 40 bits of entropy
and can collide; update the code that computes digest (the session_id -> digest
-> thread_id flow) to use hashlib.sha256(session_id.encode()).hexdigest() and
slice the first 16 hex characters ([:16]) so thread_id = f"bub-{digest}"
provides ~64 bits of entropy; ensure the change is applied where digest and
thread_id are created and that hashlib remains imported.
---
Outside diff comments:
In `@nowledge-mem-bub-plugin/src/nowledge_mem_bub/plugin.py`:
- Around line 186-198: The current logic skips the 404 recovery when thread_id
is present in self._known_threads because append_thread is only wrapped in a
try/except in the else branch; update the append flow so that any call to
self.client.append_thread(thread_id, messages_json) (both when thread is known
and when not) catches NmemError, inspects the error string (lowercased) for "not
found" or "404", and on that case removes the stale thread_id from
self._known_threads, calls self.client.create_thread(thread_id, f"Bub Session
({session_id[:30]})", messages_json) and re-adds the id; for other NmemError
cases re-raise. Target symbols: self._known_threads, append_thread,
create_thread, NmemError, session_id, messages_json.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82b4d789-58c1-40b9-9de6-148129b7cd46
📒 Files selected for processing (1)
nowledge-mem-bub-plugin/src/nowledge_mem_bub/plugin.py
Comment on lines
+176
to
+177
| digest = hashlib.sha1(session_id.encode()).hexdigest()[:10] | ||
| thread_id = f"bub-{digest}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect thread-id construction and client command wiring.
rg -n -C3 'thread_id|bub-|create_thread\(|append_thread\(|"--id"|--id' nowledge-mem-bub-plugin/src/nowledge_mem_bubRepository: nowledge-co/community
Length of output: 10049
🏁 Script executed:
#!/bin/bash
# Search for any thread_id validation, format checks, or constraints
rg -n 'thread_id|thread.?id' nowledge-mem-bub-plugin/src/nowledge_mem_bub --type py -A2 | grep -E '(len|validate|format|match|pattern|constraint|max|min)' || echo "No format constraints found"
# Check if there's any documentation or config about thread_id
fd -e txt -e md nowledge-mem-bub-plugin | xargs grep -l 'thread' 2>/dev/null || echo "No relevant docs"
# Look for session_id structure to understand collision risk
rg -n 'session_id' nowledge-mem-bub-plugin/src/nowledge_mem_bub/plugin.py -B2 -A2Repository: nowledge-co/community
Length of output: 1639
Increase digest entropy for thread_id to prevent session collisions.
SHA-1 truncated to 10 hex characters (40 bits) can collide across different sessions, causing unrelated transcripts to mix in the same thread. No length or format constraints exist in the codebase, so expanding to SHA-256 with 16 hex characters (64 bits) is safe.
Suggested fix
- digest = hashlib.sha1(session_id.encode()).hexdigest()[:10]
+ digest = hashlib.sha256(session_id.encode("utf-8")).hexdigest()[:16]
thread_id = f"bub-{digest}"🧰 Tools
🪛 Ruff (0.15.6)
[error] 176-176: Probable use of insecure hash functions in hashlib: sha1
(S324)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nowledge-mem-bub-plugin/src/nowledge_mem_bub/plugin.py` around lines 176 -
177, The thread_id generation uses hashlib.sha1(session_id).hexdigest()[:10],
which yields only 40 bits of entropy and can collide; update the code that
computes digest (the session_id -> digest -> thread_id flow) to use
hashlib.sha256(session_id.encode()).hexdigest() and slice the first 16 hex
characters ([:16]) so thread_id = f"bub-{digest}" provides ~64 bits of entropy;
ensure the change is applied where digest and thread_id are created and that
hashlib remains imported.
Member
|
Thanks! |
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
2 participants
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.
Signed-off-by: Frost Ming me@frostming.com
The errors of nmem-cli are output to stdout instead of stderr, causing the client to not get the error text correctly
Summary by CodeRabbit
Bug Fixes
Refactor