Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR adds Grok as a supported runtime alongside Claude Code in the nowledge-mem plugin. Hook scripts now detect Grok from environment variables and route ChangesGrok Integration for nowledge-mem Plugin
Hermes Installer Version and Endpoint Reporting
Docker Configuration and Documentation Tag Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 |
The headless image default must track the release so fresh / NAS-app installs pull this version (the preflight guard enforces it). Switch the cosign-verify and docker-pull example commands to :latest so they no longer need a per-release bump. Co-Authored-By: Wey Gu <weyl.gu@gmail.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 90285a1. Configure here.
| or os.environ.get("GROK_WORKSPACE_ROOT") | ||
| ): | ||
| return "grok" | ||
| return "claude-code" |
There was a problem hiding this comment.
Runtime detection ignores plugin root
Medium Severity
Hook commands resolve scripts via GROK_PLUGIN_ROOT, but _host_runtime() and SOURCE_APP only flip to Grok when GROK_SESSION_ID, GROK_HOOK_EVENT, or GROK_WORKSPACE_ROOT are set. A Grok hook run that exports the plugin root without those three vars would still call nmem with --from claude-code and --source-app claude-code, mislabeling sessions again.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 90285a1. Configure here.
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ 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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nowledge-mem-hermes/setup.sh (1)
141-161: ⚡ Quick winHarden helper parsing to avoid blank/false-unknown diagnostics.
On Line 147 and Lines 152/156, the parser is brittle: version can print empty, and endpoint detection depends on double quotes only. A tiny fallback makes post-install diagnostics more reliable.
Suggested patch
plugin_version_for_dir() { local target_dir="$1" if [ ! -f "$target_dir/plugin.yaml" ]; then printf 'unknown' return fi - sed -n "s/^version:[[:space:]]*//p" "$target_dir/plugin.yaml" | head -n 1 | tr -d "\"'" + local v + v="$(sed -n "s/^version:[[:space:]]*//p" "$target_dir/plugin.yaml" | head -n 1 | tr -d "\"'")" + if [ -n "$v" ]; then + printf '%s' "$v" + else + printf 'unknown' + fi } thread_endpoint_for_dir() { local target_dir="$1" - if grep -qF '"/threads/import"' "$target_dir/client.py" 2>/dev/null; then + if grep -qE '["'"'"']/threads/import["'"'"']' "$target_dir/client.py" 2>/dev/null; then printf '/threads/import' return fi - if grep -qF '"/threads"' "$target_dir/client.py" 2>/dev/null; then + if grep -qE '["'"'"']/threads["'"'"']' "$target_dir/client.py" 2>/dev/null; then printf '/threads' return fi printf 'unknown' }🤖 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-hermes/setup.sh` around lines 141 - 161, The plugin_version_for_dir function and thread_endpoint_for_dir function have brittle parsing logic that can produce empty or unreliable results. In plugin_version_for_dir, the sed pipeline may extract an empty version string instead of defaulting to "unknown". Add a fallback check after the version extraction to ensure that if the extracted version is empty, "unknown" is output instead. In thread_endpoint_for_dir, the grep patterns are overly strict and depend only on double-quote formatting, making endpoint detection fragile. Make the grep patterns more flexible to handle variations (such as single quotes or no quotes) in the client.py file, or add additional fallback grep patterns to check for alternative endpoint formats before defaulting to "unknown".
🤖 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-hermes/setup.sh`:
- Around line 141-161: The plugin_version_for_dir function and
thread_endpoint_for_dir function have brittle parsing logic that can produce
empty or unreliable results. In plugin_version_for_dir, the sed pipeline may
extract an empty version string instead of defaulting to "unknown". Add a
fallback check after the version extraction to ensure that if the extracted
version is empty, "unknown" is output instead. In thread_endpoint_for_dir, the
grep patterns are overly strict and depend only on double-quote formatting,
making endpoint detection fragile. Make the grep patterns more flexible to
handle variations (such as single quotes or no quotes) in the client.py file, or
add additional fallback grep patterns to check for alternative endpoint formats
before defaulting to "unknown".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d364aba0-c8f4-48c5-a49c-a7fa79631f67
📒 Files selected for processing (23)
.claude-plugin/marketplace.jsonREADME.mddocs/USER_OVERRIDE_GUIDE.mdintegrations.jsonnowledge-mem-claude-code-plugin/.claude-plugin/plugin.jsonnowledge-mem-claude-code-plugin/CHANGELOG.mdnowledge-mem-claude-code-plugin/README.mdnowledge-mem-claude-code-plugin/commands/save.mdnowledge-mem-claude-code-plugin/hooks/hooks.jsonnowledge-mem-claude-code-plugin/scripts/nmem-hook-read.shnowledge-mem-claude-code-plugin/scripts/nmem-hook-save.pynowledge-mem-claude-code-plugin/skills/read-working-memory/SKILL.mdnowledge-mem-claude-code-plugin/skills/save-thread/SKILL.mdnowledge-mem-claude-code-plugin/tests/test_nmem_hook_read.pynowledge-mem-claude-code-plugin/tests/test_nmem_hook_save.pynowledge-mem-hermes/CHANGELOG.mdnowledge-mem-hermes/README.mdnowledge-mem-hermes/plugin.yamlnowledge-mem-hermes/setup.shnowledge-mem-hermes/tests/test_setup.shnowledge-mem-npx-skills/CHANGELOG.mdnowledge-mem-npx-skills/skills/check-integration/SKILL.mdnowledge-mem-npx-skills/skills/read-working-memory/SKILL.md
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
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.
Note
Medium Risk
Grok hook routing changes how sessions are labeled and captured in Mem; incorrect detection could still mis-tag threads. Hermes changes are mostly install/diagnostic but affect which API path users run after updates.
Overview
Grok Build is now a first-class integration on the same
nowledge-mem-claude-code-pluginpackage (bumped to 0.7.11). Registry, README, marketplace, and user-override docs describe install viagrok plugin install …and Grok-specificnmemusage.The shared hooks resolve
GROK_PLUGIN_ROOTahead ofCLAUDE_PLUGIN_ROOT, detect Grok fromGROK_SESSION_ID/GROK_HOOK_EVENT/GROK_WORKSPACE_ROOT, and route Context Bundle reads with--source-app grokand transcript capture withnmem t save --from grok(plus Grok session/workspace env for ids and project paths). That fixes Grok sessions being stored as Claude Code. Skills, slash-command docs, and hook tests cover the Grok path.Hermes (0.5.18):
setup.shprints installed version and thread import endpoint from the files it wrote, overwrites stale copies (e.g. oldclient.pystill pointing at/threads), and tests assert reinstall behavior. npx-skillscheck-integrationdocuments verifyingThread import endpoint: /threads/importand the correctHERMES_HOME.Reviewed by Cursor Bugbot for commit 90285a1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit