fix(claude-code): centralize working memory hook#231
Conversation
|
@codex review |
|
bugbot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6feee2d25b
ℹ️ 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".
|
@codex review |
|
bugbot review |
📝 WalkthroughWalkthroughThe pull request extracts the working-memory read logic from inline hook commands into a standalone shell script ( ChangesHook Read Script Extraction
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR centralizes Claude Code “Working Memory” injection by moving the duplicated nmem wm read shell pipeline out of hooks.json into a shared script (scripts/nmem-hook-read.sh), while preserving the existing fallback order (project space → default space → ~/ai-now/memory.md). It also bumps the Claude Code plugin version to 0.7.9 and adds regression/static-contract tests to ensure the new wiring stays in place.
Changes:
- Replace inline
wm readpipelines in Claude CodeSessionStarthooks with a sharednmem-hook-read.shscript. - Add unit + e2e static-contract coverage to validate hook wiring, space resolution, and fallback behavior.
- Bump Claude Code plugin metadata/version references to
0.7.9and document in the changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/plugin_e2e/test_key_plugins_e2e.py |
Adds static assertions that Claude hooks reference the new read script and no longer embed wm read. |
nowledge-mem-claude-code-plugin/tests/test_nmem_hook_read.py |
New regression tests for space resolution and fallback order in the read hook script. |
nowledge-mem-claude-code-plugin/scripts/nmem-hook-read.sh |
New shared implementation of the Working Memory read/fallback logic (including Windows nmem.cmd handling). |
nowledge-mem-claude-code-plugin/hooks/hooks.json |
Updates SessionStart hook commands to call the shared read script. |
nowledge-mem-claude-code-plugin/CHANGELOG.md |
Documents the refactor and added coverage for version 0.7.9. |
nowledge-mem-claude-code-plugin/.claude-plugin/plugin.json |
Bumps plugin version to 0.7.9. |
integrations.json |
Updates the Claude Code integration version to 0.7.9. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = _run_hook( | ||
| tmp_path, | ||
| cwd=tmp_path, | ||
| env={"PATH": "/bin:/usr/bin", "NMEM_SPACE": ""}, | ||
| ) |
|
bugbot review |
|
@codex review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nowledge-mem-claude-code-plugin/scripts/nmem-hook-read.sh (1)
6-12: 💤 Low valueConsider more robust argument escaping for the Windows wrapper.
The argument quoting in the
nmem()function may not handle edge cases correctly when arguments contain embedded quotes, backslashes, or cmd.exe metacharacters. While nmem arguments are typically simple, a space name likeproject"2024could break the command string passed tocmd.exe.Consider escaping arguments more defensively, or document the limitation.
♻️ More robust quoting approach
- nmem() { - q="" - for a in "$@"; do - q="$q \"$a\"" - done - cmd.exe /s /c "\"nmem.cmd\"$q" - } + nmem() { + # Escape each argument for cmd.exe by doubling quotes + local args="" + for a in "$@"; do + # Replace " with "" for cmd.exe escaping + local escaped="${a//\"/\"\"}" + args="$args \"$escaped\"" + done + cmd.exe /s /c "\"nmem.cmd\"$args" + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nowledge-mem-claude-code-plugin/scripts/nmem-hook-read.sh` around lines 6 - 12, The nmem() wrapper builds a command string without escaping backslashes or embedded double-quotes, which can break cmd.exe invocation for args like project"2024; update nmem to defensively escape each argument before adding it to q (e.g., for each a: first escape backslashes then escape double quotes via parameter expansion esc=${a//\\/\\\\}; esc=${esc//\"/\\\"}; then append q="$q \"$esc\""), continue to wrap each escaped arg in quotes and pass the final string to cmd.exe /s /c "\"nmem.cmd\"$q"; reference: function name nmem in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@nowledge-mem-claude-code-plugin/scripts/nmem-hook-read.sh`:
- Around line 6-12: The nmem() wrapper builds a command string without escaping
backslashes or embedded double-quotes, which can break cmd.exe invocation for
args like project"2024; update nmem to defensively escape each argument before
adding it to q (e.g., for each a: first escape backslashes then escape double
quotes via parameter expansion esc=${a//\\/\\\\}; esc=${esc//\"/\\\"}; then
append q="$q \"$esc\""), continue to wrap each escaped arg in quotes and pass
the final string to cmd.exe /s /c "\"nmem.cmd\"$q"; reference: function name
nmem in the diff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66d5c732-28dd-41cd-b9a8-6670b851b9b0
📒 Files selected for processing (7)
integrations.jsonnowledge-mem-claude-code-plugin/.claude-plugin/plugin.jsonnowledge-mem-claude-code-plugin/CHANGELOG.mdnowledge-mem-claude-code-plugin/hooks/hooks.jsonnowledge-mem-claude-code-plugin/scripts/nmem-hook-read.shnowledge-mem-claude-code-plugin/tests/test_nmem_hook_read.pytests/plugin_e2e/test_key_plugins_e2e.py
|
@codex review |
|
bugbot review |
|
Addressed the actionable review comments in the latest head:
Final-head validation passed locally: focused hook E2E, Claude live E2E, plugin static gate, Codex plugin validator, Hermes tests, |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
|
@codex review |
|
bugbot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d3baa8e5e
ℹ️ 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".
| { | ||
| "type": "command", | ||
| "command": "command -v nmem >/dev/null 2>&1 || { command -v nmem.cmd >/dev/null 2>&1 && nmem(){ local q=''; for a in \"$@\"; do q=\"$q \\\"$a\\\"\"; done; cmd.exe /s /c \"\\\"nmem.cmd\\\"$q\"; }; }; PY=\"$(command -v python3 || command -v python || true)\"; SPACE=\"${NMEM_SPACE:-$(basename \"$(cd \"$(dirname \"$(git rev-parse --git-common-dir 2>/dev/null)\")\" 2>/dev/null && pwd)\" 2>/dev/null | tr '[:upper:]' '[:lower:]')}\"; { [ -n \"$PY\" ] && [ -n \"$SPACE\" ] && nmem --json wm read --space \"$SPACE\" 2>/dev/null | \"$PY\" -c \"import sys,json;d=json.load(sys.stdin);c=d.get('content','');print(c) if d.get('exists') and c else sys.exit(1)\" 2>/dev/null; } || { [ -n \"$PY\" ] && nmem --json wm read 2>/dev/null | \"$PY\" -c \"import sys,json;d=json.load(sys.stdin);c=d.get('content','');print(c) if c else sys.exit(1)\" 2>/dev/null; } || cat ~/ai-now/memory.md 2>/dev/null || true" | ||
| "command": "SCRIPT=\"${CLAUDE_PLUGIN_ROOT:-}/scripts/nmem-hook-read.sh\"; { [ -f \"$SCRIPT\" ] && sh \"$SCRIPT\"; } || cat \"$HOME/ai-now/memory.md\" 2>/dev/null || true" |
There was a problem hiding this comment.
Keep nmem lookup when plugin root is unset
The new SessionStart hook command now only runs WM loading through scripts/nmem-hook-read.sh when that file path exists, and otherwise falls straight to ~/ai-now/memory.md. If CLAUDE_PLUGIN_ROOT is unset/empty (which can happen in direct hook execution), SCRIPT becomes /scripts/nmem-hook-read.sh, so the hook never attempts nmem --json wm read even when nmem is installed; users lose project/default working-memory injection unless the fallback file exists. The previous inline command still performed nmem wm read in this scenario, so this is a behavior regression.
Useful? React with 👍 / 👎.
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 3d3baa8. Configure here.
Summary
scripts/nmem-hook-read.shso both hook matchers share the same behavior.nmem wm read, default-spacenmem wm read, then~/ai-now/memory.md.Verification
uv run --with pytest pytest nowledge-mem-claude-code-plugin/tests/test_nmem_hook_read.py nowledge-mem-claude-code-plugin/tests/test_nmem_hook_save.py tests/plugin_e2e/test_key_plugins_e2e.py -quv run --with pytest pytest tests/plugin_e2e -quv run --with pytest pytest nowledge-mem-hermes/tests -qnode nowledge-mem-codex-plugin/scripts/validate-plugin.mjssh -n nowledge-mem-claude-code-plugin/scripts/nmem-hook-read.shgit diff --checkNote
Low Risk
Low risk refactor of Claude Code hook wiring plus added tests; main risk is regressions in shell hook execution and fallback behavior across environments (git/non-git, missing
CLAUDE_PLUGIN_ROOT, Windowsnmem.cmd).Overview
Claude Code’s
SessionStartandcompactWorking Memory injection is refactored to call a new shared script (scripts/nmem-hook-read.sh) instead of duplicating a long inline shell pipeline inhooks.json, while preserving the same fallback order (space-awarewm read→ defaultwm read→$HOME/ai-now/memory.md).Adds focused regression tests for space resolution (
git --git-common-dirandNMEM_SPACEoverride), fallback behavior when space/default WM is empty ornmemis missing, and Windowsnmem.cmdinvocation/escaping; updates plugin registry/manifest versions to0.7.9and asserts in E2E static checks that hooks no longer embedwm readdirectly and that file fallback works withoutCLAUDE_PLUGIN_ROOT.Reviewed by Cursor Bugbot for commit 3d3baa8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Chores
Tests