Conversation
|
bugbot run |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 31 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBundles and runs Copilot CLI hook scripts from the plugin package (plugin-relative Changes
Sequence DiagramsequenceDiagram
participant CopilotCLI as Copilot CLI
participant Hook as Stop Hook\n(copilot-stop-save.py)
participant FileSystem as File System\n(transcripts, state, hook-log.jsonl)
participant Nmem as nmem\n(memory system)
CopilotCLI->>Hook: Emit stop event (JSON on stdin)
Hook->>Hook: Parse sessionId, transcriptPath, stopReason, timestamp
Hook->>FileSystem: Acquire per-session lock\n(~/.copilot/nowledge-mem-hooks/state/)
alt invalid stop or transcript missing
Hook->>Hook: Exit early
else valid stop
Hook->>FileSystem: Read transcript events
Hook->>Hook: Locate assistant.turn_end event
Hook->>Hook: Determine slice (start → turn_end)
alt safeguards triggered (incomplete/sensitive/thresholds)
Hook->>FileSystem: Log rejection to hook-log.jsonl
else pass safeguards
Hook->>Nmem: Create temp import JSON
Hook->>Nmem: Run `nmem t import` or `nmem t append`
Nmem->>FileSystem: Consume temp file
alt auto-distill conditions met
Hook->>Nmem: `nmem t triage` then `nmem t distill`
end
Hook->>FileSystem: Update per-session state
Hook->>FileSystem: Append structured log to hook-log.jsonl
end
end
Hook->>FileSystem: Release lock
Hook->>CopilotCLI: Return exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
|
@codex review |
|
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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
nowledge-mem-copilot-cli-plugin/scripts/install-hooks.sh (1)
11-21: ValidateSOURCE_DIRbefore copying.If a user runs this script from an unexpected layout (or via a symlink where
dirname "$0"doesn't resolve the real path),cp -fwill fail underset -ewith a less-than-obvious error. Consider resolving the real script path and assertingSOURCE_DIRcontains the expected hook files before proceeding.♻️ Suggested guard
-PLUGIN_ROOT="$(cd "$(dirname "$0")/.." && pwd)" +SCRIPT_PATH="$(cd "$(dirname "$(readlink -f "$0" 2>/dev/null || echo "$0")")" && pwd)" +PLUGIN_ROOT="$(cd "${SCRIPT_PATH}/.." && pwd)" SOURCE_DIR="${PLUGIN_ROOT}/hooks" + +if [[ ! -f "${SOURCE_DIR}/copilot-stop-save.py" || ! -f "${SOURCE_DIR}/copilot-stop-save.sh" ]]; then + echo "ERROR: Expected hook sources under ${SOURCE_DIR}. Run this script from the plugin's scripts/ directory." >&2 + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/scripts/install-hooks.sh` around lines 11 - 21, The install script doesn't validate SOURCE_DIR or resolve the script's real path, which can make cp -f fail silently under set -e; update PLUGIN_ROOT/SOURCE_DIR resolution (use a realpath/resolve approach for "$0" and symlinks) and add explicit existence checks for the expected hook files (e.g., ensure "${SOURCE_DIR}/copilot-stop-save.py" and "${SOURCE_DIR}/copilot-stop-save.sh" exist) before attempting the cp commands, and if missing print a clear error via echo and exit non‑zero so the installer fails with a helpful message.
🤖 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-copilot-cli-plugin/hooks/copilot-stop-save.py`:
- Around line 230-237: Add a timeout to the nmem subprocess invocation in
run_json: call subprocess.run with a timeout (e.g., timeout=30 or a configurable
constant) and wrap the call in a try/except that catches
subprocess.TimeoutExpired; on timeout raise a RuntimeError that includes any
captured stdout/stderr (use e.stdout/e.stderr or proc output if available) so
the per-session lock won’t hang indefinitely. Update run_json to use
subprocess.run(..., timeout=TIMEOUT, capture_output=True, text=True) and handle
both non-zero return codes and subprocess.TimeoutExpired with clear error
messages.
- Around line 581-604: The triage/distill step can raise and prevent state
updates, causing duplicate imports; wrap the triage/distill block (the calls to
run_json(build_nmem_command(...)) that set distill_attempted and
state["last_distill_ts"]) in a try/except that catches exceptions from
run_json/build_nmem_command, logs the error, and continues so that the
subsequent state updates (state["active_start_event_id"],
state["last_saved_turn_end_id"], state["last_content_hash"]) and
save_state(state_path, state) always run regardless of triage/distill failure;
keep the existing distill_attempted behavior but ensure failures do not
short-circuit the save_state call.
- Around line 340-348: Normalize and parse hook_timestamp before it's compared
to event_timestamp_ms(event): add a helper (e.g., parse_hook_timestamp or
normalize_hook_timestamp) that accepts None, int/float (epoch seconds or ms),
numeric strings, and ISO timestamp strings and returns an int millisecond epoch
or None; call this helper once before the current_turn_end generator and replace
direct uses of hook_timestamp in the comparison with the normalized ms value;
follow the same conventions as event_timestamp_ms() for None handling and
invalid inputs so comparisons work correctly across formats.
In `@nowledge-mem-copilot-cli-plugin/hooks/hooks.json`:
- Line 39: The stop-hook command currently runs python3 on copilot-stop-save.py
in PLUGIN_HOOK_DIR or FALLBACK_HOOK_DIR but does not mask non-zero exits; change
the command in hooks.json so any failure is swallowed (best-effort) by ensuring
the python3 invocation(s) are followed by logical OR true (e.g., make both
branch executions end with "|| true" or wrap the entire if/elif/else in a
subshell that ends with "|| true") so that failures from copilot-stop-save.py do
not produce a nonzero exit; refer to the existing PLUGIN_HOOK_DIR,
FALLBACK_HOOK_DIR and copilot-stop-save.py symbols to locate and update the
command string.
In `@nowledge-mem-copilot-cli-plugin/RELEASING.md`:
- Line 19: Update the checklist text so the diagnostic path
~/.copilot/nowledge-mem-hooks/hook-log.jsonl is described as being written by
copilot-stop-save.py regardless of whether the script ran from hooks/ or the
compatibility copy; change the sentence that currently reserves that path for
the "compatibility fallback" to mention the packaged runtime as well
(referencing copilot-stop-save.py and the hook-log.jsonl path).
In `@nowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py`:
- Around line 38-47: The test
test_packaged_hook_runtime_matches_compatibility_copy is comparing hook files
with read_text(), which can normalize newlines and hide byte-level differences;
change the comparisons to read the files as raw bytes using Path.read_bytes()
for both pairs (the files referenced via the repo_root / "hooks" /
"copilot-stop-save.py" and repo_root / "scripts" / "copilot-stop-save.py", and
similarly for the .sh files) so the assertions perform exact byte-for-byte
parity checks.
---
Nitpick comments:
In `@nowledge-mem-copilot-cli-plugin/scripts/install-hooks.sh`:
- Around line 11-21: The install script doesn't validate SOURCE_DIR or resolve
the script's real path, which can make cp -f fail silently under set -e; update
PLUGIN_ROOT/SOURCE_DIR resolution (use a realpath/resolve approach for "$0" and
symlinks) and add explicit existence checks for the expected hook files (e.g.,
ensure "${SOURCE_DIR}/copilot-stop-save.py" and
"${SOURCE_DIR}/copilot-stop-save.sh" exist) before attempting the cp commands,
and if missing print a clear error via echo and exit non‑zero so the installer
fails with a helpful message.
🪄 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: 45ea3a1f-9637-44a5-9a17-d83d8a146f50
📒 Files selected for processing (10)
integrations.jsonnowledge-mem-copilot-cli-plugin/.claude-plugin/plugin.jsonnowledge-mem-copilot-cli-plugin/CHANGELOG.mdnowledge-mem-copilot-cli-plugin/README.mdnowledge-mem-copilot-cli-plugin/RELEASING.mdnowledge-mem-copilot-cli-plugin/hooks/copilot-stop-save.pynowledge-mem-copilot-cli-plugin/hooks/copilot-stop-save.shnowledge-mem-copilot-cli-plugin/hooks/hooks.jsonnowledge-mem-copilot-cli-plugin/scripts/install-hooks.shnowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 b59ab08. Configure here.
|
bugbot run |
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-copilot-cli-plugin/hooks/copilot-stop-save.py`:
- Around line 73-75: The regex in the re.compile call uses `mongodb(?:\\+srv)?`
which, inside the raw string, treats `\\+` as a backslash plus and fails to
match `mongodb+srv://`; locate the re.compile invocation containing the pattern
r"\b(?:postgres|postgresql|mysql|mongodb(?:\\+srv)?|redis|amqp|kafka)://\S+" and
replace `mongodb(?:\\+srv)?` with `mongodb(?:\+srv)?` so the `+` is correctly
escaped and SRV URLs like `mongodb+srv://...` are detected; apply the same
replacement to the other identical occurrence of this regex in the repo.
In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py`:
- Around line 239-246: In the except subprocess.TimeoutExpired handler replace
the current isinstance(..., str) checks for exc.stdout/exc.stderr with logic
that first checks for bytes and decodes them (e.g., decode with utf-8 and
errors='replace') then falls back to str(), so stdout and stderr become decoded
strings before stripping; keep the fallback to f"command timed out after
{NMEM_TIMEOUT_SECS}s" and raise RuntimeError from exc as before, and apply the
identical change to the copy in the hooks/ version of the script so both
handlers (the TimeoutExpired except block) preserve diagnostic output.
🪄 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: d1f3c348-d392-4263-ad59-33f89f8b79b8
📒 Files selected for processing (6)
nowledge-mem-copilot-cli-plugin/RELEASING.mdnowledge-mem-copilot-cli-plugin/hooks/copilot-stop-save.pynowledge-mem-copilot-cli-plugin/hooks/hooks.jsonnowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pynowledge-mem-copilot-cli-plugin/scripts/install-hooks.shnowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py
✅ Files skipped from review due to trivial changes (1)
- nowledge-mem-copilot-cli-plugin/RELEASING.md
🚧 Files skipped from review as they are similar to previous changes (3)
- nowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py
- nowledge-mem-copilot-cli-plugin/hooks/hooks.json
- nowledge-mem-copilot-cli-plugin/scripts/install-hooks.sh
![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: 9cce1c1f5c
ℹ️ 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": "HOOK_DIR=\"${HOME}/.copilot/nowledge-mem-hooks\"; [ -f \"${HOOK_DIR}/copilot-stop-save.py\" ] && python3 \"${HOOK_DIR}/copilot-stop-save.py\" || true", | ||
| "command": "PLUGIN_HOOK_DIR=\"${COPILOT_PLUGIN_ROOT:-${PLUGIN_ROOT:-${CLAUDE_PLUGIN_ROOT:-}}}/hooks\"; FALLBACK_HOOK_DIR=\"${HOME}/.copilot/nowledge-mem-hooks\"; if [ -f \"${PLUGIN_HOOK_DIR}/copilot-stop-save.py\" ]; then python3 \"${PLUGIN_HOOK_DIR}/copilot-stop-save.py\" || true; elif [ -f \"${FALLBACK_HOOK_DIR}/copilot-stop-save.py\" ]; then python3 \"${FALLBACK_HOOK_DIR}/copilot-stop-save.py\" || true; else true; fi", |
There was a problem hiding this comment.
This command now depends on COPILOT_PLUGIN_ROOT/PLUGIN_ROOT/CLAUDE_PLUGIN_ROOT to find the packaged runtime, but Copilot CLI only added those plugin-root env vars in v1.0.26 (per github/copilot-cli changelog). On v1.0.25 and earlier, a fresh marketplace install has none of these vars and (after this change) no required installer step to populate ~/.copilot/nowledge-mem-hooks, so the Stop hook falls through to true and session capture never runs. Consider either enforcing a minimum Copilot CLI version or retaining an automatic compatibility path for older CLIs.
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 9cce1c1. Configure here.
1 participant
Note
Medium Risk
Changes affect the
Stophook execution path and background capture/distill behavior, which could impact whether sessions are saved or skipped across environments.Overview
Copilot CLI plugin session capture is moved to run from the packaged
hooks/runtime viaCOPILOT_PLUGIN_ROOT, with a fallback to~/.copilot/nowledge-mem-hooks/for older installs, removing reliance on marketplace-installedscripts/paths.The capture script is duplicated into
hooks/(plus a shell launcher), and the compatibility installer is reframed to copy fromhooks/into the legacy location. The capture logic is hardened withnmemtimeouts, more robust timestamp parsing, and non-fatal distill errors; docs/testing are updated accordingly and versions are bumped to0.1.1.Reviewed by Cursor Bugbot for commit 9cce1c1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests