feat(copilot): add Nowledge Mem plugin for Copilot CLI#201
feat(copilot): add Nowledge Mem plugin for Copilot CLI#201wey-gu merged 5 commits intonowledge-co:mainfrom
Conversation
Full-featured plugin with auto-capture, working memory, search, and distill. - Plugin manifest (.claude-plugin/plugin.json) v0.1.0 - 4 lifecycle hooks (SessionStart×2, UserPromptSubmit, Stop) - 4 skills (read-working-memory, search-memory, distill-memory, save-thread) - 4 commands (/save, /search, /sum, /status) - Session capture script (copilot-stop-save.py) with: - Incremental append via nmem t import + nmem t append fallback - Secret filtering (6 patterns) and sensitive content detection - Auto-distill with cooldown, content hash dedup, min thresholds - Cross-platform locking (fcntl/msvcrt) - Incomplete turn detection (background tasks, ask_user, questions) - Idempotent installer (install-hooks.sh) - Registry entry in integrations.json (plugin-capture transport) - Full test suite (14 tests covering all capture script logic) - Documentation: README, CHANGELOG, RELEASING, AGENTS.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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 14 minutes and 48 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)
📝 WalkthroughWalkthroughAdds a Copilot CLI integration and a Nowledge Mem Copilot plugin that loads working memory, nudges per-turn memory actions, captures sessions on stop via hooks, imports threads into nmem, and provides skills, install scripts, docs, and tests. Changes
Sequence DiagramsequenceDiagram
participant Copilot as Copilot CLI
participant Hook as Hook System
participant PyScript as copilot-stop-save.py
participant StateFile as Session State (lock/file)
participant Transcript as Transcript JSON(.l)
participant NmemCLI as nmem CLI
Copilot->>Hook: Trigger Stop hook (stdin JSON)
Hook->>PyScript: Invoke script with payload
PyScript->>PyScript: Parse payload (sessionId, transcriptPath, stopReason)
PyScript->>StateFile: Acquire per-session lock
StateFile-->>PyScript: Lock acquired
PyScript->>Transcript: Read/parse transcript events
PyScript->>PyScript: Select user→assistant turn window
PyScript->>PyScript: Redact secrets & detect incomplete/background work
alt Turn invalid or sensitive
PyScript->>PyScript: Log skip and persist state
else Valid turn
PyScript->>NmemCLI: `nmem t import` (temp file, thread id copilot-{session})
NmemCLI-->>PyScript: Import result (or exists)
PyScript->>NmemCLI: `nmem t append` fallback or `nmem t triage`/`t distill` as guarded
PyScript->>PyScript: Update state (last_saved_turn_end_id, last_distill_ts, hash)
end
PyScript->>StateFile: Write JSONL log entry and release lock
StateFile-->>PyScript: Lock released
PyScript-->>Hook: Exit (0 / non-zero)
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 |
![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: eee5555b54
ℹ️ 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".
| return [json.loads(line) for line in fh] | ||
|
|
||
|
|
||
| def find_index(events: list[dict], event_id: str | None) -> int | None: |
There was a problem hiding this comment.
This script uses 3.10-only type syntax (str | None / -> int | None), but the installer only checks for python3 and does not require 3.10+, so users on Python 3.8/3.9 will hit a SyntaxError before the Stop hook can run. In those environments, session capture silently never works (the hook command ends with || true). Either enforce a minimum Python version during install or replace these annotations with older-compatible typing syntax.
Useful? React with 👍 / 👎.
| triage = run_json( | ||
| build_nmem_command( | ||
| nmem_bin, "--json", "t", "triage", thread_id | ||
| ) |
There was a problem hiding this comment.
A failure in nmem t triage (for example, older nmem versions without this subcommand or transient backend errors) raises out of main() before last_saved_turn_end_id is updated, because state is saved only later. After that, each Stop hook reprocesses the same turn and keeps trying to import/append it again, causing repeated duplicate appends and noisy errors. Triage/distill should be best-effort and must not block state advancement once thread import/append succeeded.
Useful? React with 👍 / 👎.
![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 (10)
nowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.md (1)
10-12: Include/savein explicit trigger examples.Please add
/saveto the explicit “when to save” trigger list so the skill aligns with command-driven usage.As per coding guidelines: Use slash commands (
/save,/search <query>,/sum,/status) to manage Nowledge Mem integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.md` around lines 10 - 12, Update the explicit trigger examples in SKILL.md's "Only when user explicitly says:" list to include the slash command `/save` alongside the existing phrases ("Save this session", "Checkpoint this", "Record conversation") so the skill supports command-driven usage; edit the trigger list in SKILL.md to add `/save` and ensure consistency with other command examples (`/search <query>`, `/sum`, `/status`) mentioned in the guidelines.nowledge-mem-copilot-cli-plugin/commands/status.md (1)
9-13: Prefer/statusas the command shown to users.Please make
/statusthe top-level invocation in this page, and keepnmem statusas implementation/troubleshooting detail.As per coding guidelines: Use slash commands (
/save,/search <query>,/sum,/status) to manage Nowledge Mem integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/commands/status.md` around lines 9 - 13, Update the example in status.md to present "/status" as the primary invocation and move "nmem status" into a secondary implementation/troubleshooting note; specifically replace the top-level code block showing "nmem status" with "/status" and add a small following paragraph or code block that notes the equivalent CLI command "nmem status" for debugging or local runs so both usages appear but "/status" is shown as the canonical user-facing command.nowledge-mem-copilot-cli-plugin/commands/search.md (1)
6-14: Add/search <query>as the primary command example.The current section is technically correct, but this command page should lead with the slash command and then optionally show the underlying
nmem --json ...call.As per coding guidelines: Use slash commands (
/save,/search <query>,/sum,/status) to manage Nowledge Mem integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/commands/search.md` around lines 6 - 14, The documentation should lead with the slash command example; update the Search Memory section to present "/search <query>" as the primary command example (e.g., show a code block with "/search <query>") and then optionally include the underlying CLI invocation "nmem --json m search \"$ARGUMENTS\"" afterwards; ensure you keep the existing heading "Search Memory" and the explanatory sentence, and follow the guideline to use slash commands (`/save`, `/search <query>`, `/sum`, `/status`) as the primary examples.nowledge-mem-copilot-cli-plugin/commands/sum.md (1)
7-30: Make/sumthe primary invocation in this command page.This page currently teaches direct
nmemusage first; for consistency with plugin UX, show/sumas the primary action and keep raw CLI as implementation detail/reference.As per coding guidelines: Use slash commands (
/save,/search <query>,/sum,/status) to manage Nowledge Mem integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/commands/sum.md` around lines 7 - 30, The page should present the slash command /sum as the primary invocation and move the raw nmem CLI example (nmem m add ...) to a secondary "Implementation/CLI reference" section; update the introduction and the top-level example to show "/sum" (and related slash commands /save, /search <query>, /status) as the recommended workflow, then retain the existing nmem CLI snippet only as an optional reference and label it accordingly; modify headings and order in sum.md and adjust any examples or usage steps that currently teach direct nmem usage first so they now demonstrate slash-command usage first.nowledge-mem-copilot-cli-plugin/commands/save.md (1)
9-17: Show/savefirst in the Usage section.Since this is the
/savecommand doc, lead with slash-command usage and keepnmem t create ...as the underlying execution detail.As per coding guidelines: Use slash commands (
/save,/search <query>,/sum,/status) to manage Nowledge Mem integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/commands/save.md` around lines 9 - 17, Reorder the Usage section to lead with the slash command `/save` (showing the exact slash-command usage first) and then present the `nmem t create "..." --title "..." -s copilot-cli` example as the underlying execution detail; update the text to state that `/save` triggers an immediate save and that `nmem t create` is the equivalent lower-level invocation, and ensure the slash-command conforms to the established style used elsewhere (`/save`, `/search <query>`, `/sum`, `/status`) so readers see the slash command first and the `nmem t create` form as supplemental.nowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py (3)
324-345: Test isolation: mutating module-levelSTATE_DIRcan cause flaky tests.Directly assigning
copilot_stop_save.STATE_DIR = Path(tmpdir)mutates global state that persists across tests. If test ordering changes or tests run in parallel, this can cause failures. Also, the unpackedpathandpath2variables are unused (per static analysis).♻️ Proposed fix using `patch` and underscore prefix
class TestStateManagement: def test_load_state_new_session(self): with tempfile.TemporaryDirectory() as tmpdir: - copilot_stop_save.STATE_DIR = Path(tmpdir) - path, state = copilot_stop_save.load_state("new-session") + with patch.object(copilot_stop_save, "STATE_DIR", Path(tmpdir)): + _path, state = copilot_stop_save.load_state("new-session") assert state["active_start_event_id"] is None assert state["last_saved_turn_end_id"] is None assert state["last_distill_ts"] == 0 assert state["last_content_hash"] is None def test_save_and_load_state(self): with tempfile.TemporaryDirectory() as tmpdir: - copilot_stop_save.STATE_DIR = Path(tmpdir) - path, state = copilot_stop_save.load_state("test-session") - state["last_saved_turn_end_id"] = "e4" - state["last_distill_ts"] = 1000 - state["last_content_hash"] = "abc123" - copilot_stop_save.save_state(path, state) - - path2, state2 = copilot_stop_save.load_state("test-session") - assert state2["last_saved_turn_end_id"] == "e4" - assert state2["last_distill_ts"] == 1000 - assert state2["last_content_hash"] == "abc123" + with patch.object(copilot_stop_save, "STATE_DIR", Path(tmpdir)): + path, state = copilot_stop_save.load_state("test-session") + state["last_saved_turn_end_id"] = "e4" + state["last_distill_ts"] = 1000 + state["last_content_hash"] = "abc123" + copilot_stop_save.save_state(path, state) + + _path2, state2 = copilot_stop_save.load_state("test-session") + assert state2["last_saved_turn_end_id"] == "e4" + assert state2["last_distill_ts"] == 1000 + assert state2["last_content_hash"] == "abc123"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py` around lines 324 - 345, The tests mutate the module-level copilot_stop_save.STATE_DIR directly which can leak between tests; update both test_load_state_new_session and test_save_and_load_state to patch STATE_DIR only for the test scope (use unittest.mock.patch or pytest monkeypatch to set copilot_stop_save.STATE_DIR = Path(tmpdir) inside the context) and avoid assigning unused variables by renaming path and path2 to _path/_path2 or not capturing them when calling load_state; keep calls to load_state and save_state unchanged.
383-392: Test leaves temporary file behind on assertion failure.If an assertion fails before
os.unlink(f.name), the temp file remains. Using a context manager withdelete_on_close=False(Python 3.12+) or atry/finallyblock ensures cleanup:♻️ Proposed fix for reliable cleanup
def test_load_events_from_jsonl(self): events = basic_transcript() - with tempfile.NamedTemporaryFile("w", suffix=".jsonl", delete=False) as f: - for event in events: - f.write(json.dumps(event) + "\n") - f.flush() - loaded = copilot_stop_save.load_events(f.name) - assert len(loaded) == len(events) - assert loaded[0]["type"] == "user.message" - os.unlink(f.name) + with tempfile.TemporaryDirectory() as tmpdir: + fpath = Path(tmpdir) / "transcript.jsonl" + with fpath.open("w") as f: + for event in events: + f.write(json.dumps(event) + "\n") + loaded = copilot_stop_save.load_events(str(fpath)) + assert len(loaded) == len(events) + assert loaded[0]["type"] == "user.message"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py` around lines 383 - 392, The test test_load_events_from_jsonl leaves the temp file behind if an assertion fails; wrap the body that writes, calls copilot_stop_save.load_events and asserts in a try/finally so os.unlink(f.name) is always called (or use a context manager that ensures deletion), locating the tempfile usage in test_load_events_from_jsonl and the call to copilot_stop_save.load_events to add the try/finally cleanup around those operations.
151-174: Secret redaction tests use weak assertions.The assertions like
assert "ghp_" not in result or "[REDACTED]" in resultpass even if the token is partially present. A stricter check ensures the secret is fully redacted:♻️ Proposed stricter assertions
def test_redacts_github_token(self): text = "Use token ghp_1234567890abcdefghijklmn" result = copilot_stop_save.redact(text) - assert "ghp_" not in result or "[REDACTED]" in result + assert "ghp_1234567890abcdefghijklmn" not in result + assert "[REDACTED]" in result def test_redacts_github_pat(self): text = "Token: github_pat_1234567890abcdefghijklmn" result = copilot_stop_save.redact(text) - assert "github_pat_" not in result or "[REDACTED]" in result + assert "github_pat_1234567890abcdefghijklmn" not in result + assert "[REDACTED]" in result def test_redacts_openai_key(self): text = "API key is sk-1234567890abcdefgh" result = copilot_stop_save.redact(text) - assert "sk-" not in result or "[REDACTED]" in result + assert "sk-1234567890abcdefgh" not in result + assert "[REDACTED]" in result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py` around lines 151 - 174, The tests (test_redacts_github_token, test_redacts_github_pat, test_redacts_openai_key, test_redacts_bearer_token) use weak OR assertions that allow partial secrets to remain; update each to assert the secret prefix is absent AND that "[REDACTED]" appears (e.g. replace `assert "ghp_" not in result or "[REDACTED]" in result` with `assert "ghp_" not in result and "[REDACTED]" in result`), and do the same for "github_pat_", "sk-" and the bearer case when calling copilot_stop_save.redact to ensure full redaction while keeping test_preserves_normal_text unchanged.nowledge-mem-copilot-cli-plugin/README.md (1)
32-37: WSL shim may break on arguments containing spaces or special characters.The quoting approach
q="$q \"$a\""doesn't handle arguments with embedded quotes, backslashes, or other special characters. Consider usingprintf '%q 'for proper shell escaping:♻️ Proposed fix for robust argument handling
mkdir -p ~/.local/bin && cat > ~/.local/bin/nmem << 'SHIMEOF' #!/bin/bash -q=""; for a in "$@"; do q="$q \"$a\""; done -cmd.exe /s /c "\"nmem.cmd\"$q" +args=() +for a in "$@"; do args+=("$(printf '%q' "$a")"); done +cmd.exe /s /c "nmem.cmd ${args[*]}" SHIMEOF chmod +x ~/.local/bin/nmem🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/README.md` around lines 32 - 37, The WSL shim in the nmem script builds q with q="$q \"$a\"" which fails on embedded quotes/backslashes; update the argument-quoting loop in the nmem shim to use a robust shell-escaping approach (e.g., build q with printf '%q' for each "$a" or otherwise use an array and proper quoting) so arguments containing spaces/special chars are escaped correctly; look for the q variable and the for a in "$@" loop in the nmem shim and replace the naive concatenation with printf '%q' "$a" (or an equivalent safe-escaping strategy) before invoking cmd.exe.nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py (1)
314-321: File locking on Windows locks only 1 byte of an empty file.
msvcrt.locking(lock_fh.fileno(), msvcrt.LK_LOCK, 1)locks 1 byte, but the file is empty (just opened with"w"). While this works as a mutex (any concurrent lock attempt will block), the lock is never released explicitly—relying on file handle close. Also, if the file grows beyond 1 byte from another process writing, the lock scope is ambiguous.Consider writing a sentinel byte before locking, or document this is intentional mutex-style locking:
♻️ Proposed improvement
with lock_path.open("w", encoding="utf-8") as lock_fh: + lock_fh.write("L") # Sentinel byte for msvcrt locking + lock_fh.flush() if fcntl: fcntl.flock(lock_fh, fcntl.LOCK_EX) else: import msvcrt msvcrt.locking(lock_fh.fileno(), msvcrt.LK_LOCK, 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py` around lines 314 - 321, The Windows branch currently calls msvcrt.locking(lock_fh.fileno(), msvcrt.LK_LOCK, 1) on an empty file which leads to ambiguous locking; change the Windows path (around lock_path, lock_fh, session_id, and the fcntl/ mscrt branch) to first write a sentinel byte (e.g., b"\0") to the opened file, flush() and seek(0) so the file actually contains that byte, then call msvcrt.locking(..., msvcrt.LK_LOCK, 1) to lock that byte; keep the existing behavior for fcntl, and rely on closing lock_fh to release the lock as before.
🤖 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/CHANGELOG.md`:
- Line 8: The release heading "## [0.1.0] - 2026-07-14" uses a future date;
update that heading in CHANGELOG.md (the "## [0.1.0] - 2026-07-14" line) to
either the actual release date (e.g., "2026-04-16") or replace the date with
"Unreleased" until the real release date is known, ensuring the header stays
exactly "## [0.1.0] - <date or Unreleased>".
In `@nowledge-mem-copilot-cli-plugin/RELEASING.md`:
- Around line 5-7: Reorder the three release steps so the `integrations.json`
update happens before the `.claude-plugin/plugin.json` update: first bump the
`copilot-cli` version in integrations.json (the canonical source), then update
the `version` field in `.claude-plugin/plugin.json`, and finally add the new
section to `CHANGELOG.md`; ensure the RELEASING.md steps reflect that exact
sequence and phrasing.
In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py`:
- Around line 240-254: The load_state function (and any code that builds
lock_path or other filesystem paths from session_id) currently embeds raw
session_id into STATE_DIR / f"{session_id}.json" which allows path traversal;
add a sanitization/normalization step (e.g., a sanitize_session_id(session_id)
helper called from main() and before any use in load_state or when creating
lock_path) that either (a) validates allowed characters and rejects or
canonicalizes input (strip path separators, dots, and directory components), or
(b) replaces the session_id with a safe deterministic token (e.g., hex of
sha256(session_id) or a UUID derived from it) and use that token for filenames;
update load_state, any lock_path construction, and callers in main() to use the
sanitized token instead of the raw session_id.
- Around line 125-134: The redact function currently discards group 1 prefixes
by replacing the entire match when pattern.groups == 1; update the logic in
redact (which loops over SECRET_PATTERNS and checks pattern.groups) so that for
pattern.groups == 1 you preserve the first capture and append “[REDACTED]”
(i.e., perform the same substitution used for 2+ groups such as using the
"\1[REDACTED]" replacement), keep 0-group behavior unchanged, and ensure
SECRET_PATTERNS and pattern.groups are referenced as-is to locate the change.
In `@nowledge-mem-copilot-cli-plugin/skills/read-working-memory/SKILL.md`:
- Around line 34-36: Replace the plain-shell example command "nmem wm read" with
the JSON-mode command "nmem --json wm read" in SKILL.md (update the code block
that contains the command), and scan the same MD file for any other occurrences
of "nmem wm read" to change them to the "--json" variant so recall examples
follow the structured parsing convention.
In `@nowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.md`:
- Around line 33-37: Update the fenced code block that contains the "✓ Thread
saved" message so it declares a language identifier (e.g., change the opening
``` to ```text) to satisfy markdown linting; locate the fenced block that wraps
"✓ Thread saved / Summary: {summary} / Thread ID: {thread_id from nmem output}"
in SKILL.md and add the language token immediately after the opening backticks.
---
Nitpick comments:
In `@nowledge-mem-copilot-cli-plugin/commands/save.md`:
- Around line 9-17: Reorder the Usage section to lead with the slash command
`/save` (showing the exact slash-command usage first) and then present the `nmem
t create "..." --title "..." -s copilot-cli` example as the underlying execution
detail; update the text to state that `/save` triggers an immediate save and
that `nmem t create` is the equivalent lower-level invocation, and ensure the
slash-command conforms to the established style used elsewhere (`/save`,
`/search <query>`, `/sum`, `/status`) so readers see the slash command first and
the `nmem t create` form as supplemental.
In `@nowledge-mem-copilot-cli-plugin/commands/search.md`:
- Around line 6-14: The documentation should lead with the slash command
example; update the Search Memory section to present "/search <query>" as the
primary command example (e.g., show a code block with "/search <query>") and
then optionally include the underlying CLI invocation "nmem --json m search
\"$ARGUMENTS\"" afterwards; ensure you keep the existing heading "Search Memory"
and the explanatory sentence, and follow the guideline to use slash commands
(`/save`, `/search <query>`, `/sum`, `/status`) as the primary examples.
In `@nowledge-mem-copilot-cli-plugin/commands/status.md`:
- Around line 9-13: Update the example in status.md to present "/status" as the
primary invocation and move "nmem status" into a secondary
implementation/troubleshooting note; specifically replace the top-level code
block showing "nmem status" with "/status" and add a small following paragraph
or code block that notes the equivalent CLI command "nmem status" for debugging
or local runs so both usages appear but "/status" is shown as the canonical
user-facing command.
In `@nowledge-mem-copilot-cli-plugin/commands/sum.md`:
- Around line 7-30: The page should present the slash command /sum as the
primary invocation and move the raw nmem CLI example (nmem m add ...) to a
secondary "Implementation/CLI reference" section; update the introduction and
the top-level example to show "/sum" (and related slash commands /save, /search
<query>, /status) as the recommended workflow, then retain the existing nmem CLI
snippet only as an optional reference and label it accordingly; modify headings
and order in sum.md and adjust any examples or usage steps that currently teach
direct nmem usage first so they now demonstrate slash-command usage first.
In `@nowledge-mem-copilot-cli-plugin/README.md`:
- Around line 32-37: The WSL shim in the nmem script builds q with q="$q \"$a\""
which fails on embedded quotes/backslashes; update the argument-quoting loop in
the nmem shim to use a robust shell-escaping approach (e.g., build q with printf
'%q' for each "$a" or otherwise use an array and proper quoting) so arguments
containing spaces/special chars are escaped correctly; look for the q variable
and the for a in "$@" loop in the nmem shim and replace the naive concatenation
with printf '%q' "$a" (or an equivalent safe-escaping strategy) before invoking
cmd.exe.
In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py`:
- Around line 314-321: The Windows branch currently calls
msvcrt.locking(lock_fh.fileno(), msvcrt.LK_LOCK, 1) on an empty file which leads
to ambiguous locking; change the Windows path (around lock_path, lock_fh,
session_id, and the fcntl/ mscrt branch) to first write a sentinel byte (e.g.,
b"\0") to the opened file, flush() and seek(0) so the file actually contains
that byte, then call msvcrt.locking(..., msvcrt.LK_LOCK, 1) to lock that byte;
keep the existing behavior for fcntl, and rely on closing lock_fh to release the
lock as before.
In `@nowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.md`:
- Around line 10-12: Update the explicit trigger examples in SKILL.md's "Only
when user explicitly says:" list to include the slash command `/save` alongside
the existing phrases ("Save this session", "Checkpoint this", "Record
conversation") so the skill supports command-driven usage; edit the trigger list
in SKILL.md to add `/save` and ensure consistency with other command examples
(`/search <query>`, `/sum`, `/status`) mentioned in the guidelines.
In `@nowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py`:
- Around line 324-345: The tests mutate the module-level
copilot_stop_save.STATE_DIR directly which can leak between tests; update both
test_load_state_new_session and test_save_and_load_state to patch STATE_DIR only
for the test scope (use unittest.mock.patch or pytest monkeypatch to set
copilot_stop_save.STATE_DIR = Path(tmpdir) inside the context) and avoid
assigning unused variables by renaming path and path2 to _path/_path2 or not
capturing them when calling load_state; keep calls to load_state and save_state
unchanged.
- Around line 383-392: The test test_load_events_from_jsonl leaves the temp file
behind if an assertion fails; wrap the body that writes, calls
copilot_stop_save.load_events and asserts in a try/finally so os.unlink(f.name)
is always called (or use a context manager that ensures deletion), locating the
tempfile usage in test_load_events_from_jsonl and the call to
copilot_stop_save.load_events to add the try/finally cleanup around those
operations.
- Around line 151-174: The tests (test_redacts_github_token,
test_redacts_github_pat, test_redacts_openai_key, test_redacts_bearer_token) use
weak OR assertions that allow partial secrets to remain; update each to assert
the secret prefix is absent AND that "[REDACTED]" appears (e.g. replace `assert
"ghp_" not in result or "[REDACTED]" in result` with `assert "ghp_" not in
result and "[REDACTED]" in result`), and do the same for "github_pat_", "sk-"
and the bearer case when calling copilot_stop_save.redact to ensure full
redaction while keeping test_preserves_normal_text unchanged.
🪄 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: afb203e7-f806-40ad-87b3-00520c2d7682
⛔ Files ignored due to path filters (1)
nowledge-mem-copilot-cli-plugin/icon.pngis excluded by!**/*.png
📒 Files selected for processing (19)
integrations.jsonnowledge-mem-copilot-cli-plugin/.claude-plugin/plugin.jsonnowledge-mem-copilot-cli-plugin/AGENTS.mdnowledge-mem-copilot-cli-plugin/CHANGELOG.mdnowledge-mem-copilot-cli-plugin/README.mdnowledge-mem-copilot-cli-plugin/RELEASING.mdnowledge-mem-copilot-cli-plugin/commands/save.mdnowledge-mem-copilot-cli-plugin/commands/search.mdnowledge-mem-copilot-cli-plugin/commands/status.mdnowledge-mem-copilot-cli-plugin/commands/sum.mdnowledge-mem-copilot-cli-plugin/hooks/hooks.jsonnowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pynowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.shnowledge-mem-copilot-cli-plugin/scripts/install-hooks.shnowledge-mem-copilot-cli-plugin/skills/distill-memory/SKILL.mdnowledge-mem-copilot-cli-plugin/skills/read-working-memory/SKILL.mdnowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.mdnowledge-mem-copilot-cli-plugin/skills/search-memory/SKILL.mdnowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py
| def redact(text: str) -> str: | ||
| redacted = text or "" | ||
| for pattern in SECRET_PATTERNS: | ||
| if pattern.groups == 0: | ||
| redacted = pattern.sub("[REDACTED]", redacted) | ||
| elif pattern.groups == 1: | ||
| redacted = pattern.sub("[REDACTED]", redacted) | ||
| else: | ||
| redacted = pattern.sub(r"\1[REDACTED]", redacted) | ||
| return redacted |
There was a problem hiding this comment.
Redaction logic discards intended prefixes for single-group patterns.
When pattern.groups == 1, the current code replaces the entire match with [REDACTED], losing the prefix captured in group 1. Patterns like the Bearer token pattern (line 56) capture "Bearer " in group 1 specifically to preserve it. The logic should be:
- 0 groups → replace whole match
- 1 group → preserve group 1 as prefix
- 2+ groups → already handled with
\1[REDACTED]
🐛 Proposed fix
def redact(text: str) -> str:
redacted = text or ""
for pattern in SECRET_PATTERNS:
if pattern.groups == 0:
redacted = pattern.sub("[REDACTED]", redacted)
elif pattern.groups == 1:
- redacted = pattern.sub("[REDACTED]", redacted)
+ redacted = pattern.sub(r"\1[REDACTED]", redacted)
else:
redacted = pattern.sub(r"\1[REDACTED]", redacted)
return redactedOr simplify since groups >= 1 now have the same behavior:
def redact(text: str) -> str:
redacted = text or ""
for pattern in SECRET_PATTERNS:
if pattern.groups == 0:
redacted = pattern.sub("[REDACTED]", redacted)
else:
redacted = pattern.sub(r"\1[REDACTED]", redacted)
return redacted📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def redact(text: str) -> str: | |
| redacted = text or "" | |
| for pattern in SECRET_PATTERNS: | |
| if pattern.groups == 0: | |
| redacted = pattern.sub("[REDACTED]", redacted) | |
| elif pattern.groups == 1: | |
| redacted = pattern.sub("[REDACTED]", redacted) | |
| else: | |
| redacted = pattern.sub(r"\1[REDACTED]", redacted) | |
| return redacted | |
| def redact(text: str) -> str: | |
| redacted = text or "" | |
| for pattern in SECRET_PATTERNS: | |
| if pattern.groups == 0: | |
| redacted = pattern.sub("[REDACTED]", redacted) | |
| elif pattern.groups == 1: | |
| redacted = pattern.sub(r"\1[REDACTED]", redacted) | |
| else: | |
| redacted = pattern.sub(r"\1[REDACTED]", redacted) | |
| return redacted |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py` around lines
125 - 134, The redact function currently discards group 1 prefixes by replacing
the entire match when pattern.groups == 1; update the logic in redact (which
loops over SECRET_PATTERNS and checks pattern.groups) so that for pattern.groups
== 1 you preserve the first capture and append “[REDACTED]” (i.e., perform the
same substitution used for 2+ groups such as using the "\1[REDACTED]"
replacement), keep 0-group behavior unchanged, and ensure SECRET_PATTERNS and
pattern.groups are referenced as-is to locate the change.
Clarify that Copilot CLI currently ships bundled command docs without exposing them as interactive slash commands, and update the install path examples accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep the Copilot plugin user-facing surface skill-only so command docs no longer create overlapping save/search/status entries alongside skills. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
nowledge-mem-copilot-cli-plugin/CHANGELOG.md (1)
8-8:⚠️ Potential issue | 🟡 MinorUpdate release date or mark as Unreleased.
The release date
2026-07-14is approximately three months in the future (today is April 17, 2026). Use the actual release date or change toUnreleaseduntil the package ships.📝 Suggested fix
-## [0.1.0] - 2026-07-14 +## [0.1.0] - UnreleasedOr use today's date if releasing now:
-## [0.1.0] - 2026-07-14 +## [0.1.0] - 2026-04-17🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/CHANGELOG.md` at line 8, The changelog entry header "## [0.1.0] - 2026-07-14" uses a future release date; update the date to the actual release date (today's date) or replace the date with "Unreleased" for the 0.1.0 entry; edit the heading text in CHANGELOG.md (the "## [0.1.0] - 2026-07-14" line) to either "## [0.1.0] - Unreleased" or "## [0.1.0] - YYYY-MM-DD" with the correct release date.nowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.md (1)
33-37:⚠️ Potential issue | 🟡 MinorAdd language identifier to fenced code block.
The fenced block should specify a language (e.g.,
text) to satisfy markdown linting.📝 Suggested fix
-``` +```text ✓ Thread saved Summary: {summary} Thread ID: {thread_id from nmem output}</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@nowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.mdaround lines 33
- 37, Update the fenced code block in SKILL.md (the block that currently shows
"✓ Thread saved\nSummary: {summary}\nThread ID: {thread_id from nmem output}")
to include a language identifier (for example "text") after the opening triple
backticks so Markdown linting passes; ensure the opening fence is changed from
totext and do not alter the block contents.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In@nowledge-mem-copilot-cli-plugin/CHANGELOG.md:
- Line 8: The changelog entry header "## [0.1.0] - 2026-07-14" uses a future
release date; update the date to the actual release date (today's date) or
replace the date with "Unreleased" for the 0.1.0 entry; edit the heading text in
CHANGELOG.md (the "## [0.1.0] - 2026-07-14" line) to either "## [0.1.0] -
Unreleased" or "## [0.1.0] - YYYY-MM-DD" with the correct release date.In
@nowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.md:
- Around line 33-37: Update the fenced code block in SKILL.md (the block that
currently shows "✓ Thread saved\nSummary: {summary}\nThread ID: {thread_id from
nmem output}") to include a language identifier (for example "text") after the
opening triple backticks so Markdown linting passes; ensure the opening fence is
changed fromtotext and do not alter the block contents.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `53f25813-660f-43f6-996f-7b40a66f9d6c` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between eee5555b54b28499bcf40d2387d43479d8515ea2 and 02c3b71ee1697a8a8b7b2ae58cf1575348f2d593. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `integrations.json` * `nowledge-mem-copilot-cli-plugin/AGENTS.md` * `nowledge-mem-copilot-cli-plugin/CHANGELOG.md` * `nowledge-mem-copilot-cli-plugin/README.md` * `nowledge-mem-copilot-cli-plugin/RELEASING.md` * `nowledge-mem-copilot-cli-plugin/skills/distill-memory/SKILL.md` * `nowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.md` * `nowledge-mem-copilot-cli-plugin/skills/search-memory/SKILL.md` </details> <details> <summary>✅ Files skipped from review due to trivial changes (3)</summary> * nowledge-mem-copilot-cli-plugin/skills/distill-memory/SKILL.md * nowledge-mem-copilot-cli-plugin/RELEASING.md * nowledge-mem-copilot-cli-plugin/skills/search-memory/SKILL.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * integrations.json </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
bugbot run |
|
@codex review, please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02c3b71ee1
ℹ️ 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".
| session_id = hook_input.get("sessionId") | ||
| transcript_path = hook_input.get("transcriptPath") | ||
| stop_reason = hook_input.get("stopReason") |
There was a problem hiding this comment.
The Stop hook is configured with the PascalCase event name ("Stop" in nowledge-mem-copilot-cli-plugin/hooks/hooks.json), but this parser only reads camelCase keys (sessionId, transcriptPath, stopReason). In Copilot CLI, PascalCase hook events provide VS Code-compatible snake_case fields (session_id, transcript_path, stop_reason) per the official hook payload docs, so this code will treat valid Stop payloads as missing required fields and return early without saving any thread data. See GitHub’s payload format reference: https://docs.github.com/en/copilot/reference/copilot-cli-reference/cli-command-reference#hook-event-input-payloads.
Useful? React with 👍 / 👎.
|
@codex review |
|
bugbot run |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py (1)
125-133:⚠️ Potential issue | 🟡 MinorPreserve prefixes for captured-secret patterns.
Line 130 still drops the captured prefix for one-group patterns like
Bearerand assignment prefixes, reducing captured context. Treat all grouped patterns as prefix-preserving.🛡️ Proposed fix
def redact(text: str) -> str: redacted = text or "" for pattern in SECRET_PATTERNS: if pattern.groups == 0: redacted = pattern.sub("[REDACTED]", redacted) - elif pattern.groups == 1: - redacted = pattern.sub("[REDACTED]", redacted) else: redacted = pattern.sub(r"\1[REDACTED]", redacted) return redacted🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py` around lines 125 - 133, The redact function currently drops captured prefixes when pattern.groups == 1 (e.g., "Bearer ") by replacing the whole match with "[REDACTED]"; update the logic in redact to treat any pattern with one or more capture groups as prefix-preserving by using the first-group backreference (e.g., pattern.sub(r"\1[REDACTED]", redacted)) for pattern.groups >= 1 so SECRET_PATTERNS that capture prefixes keep the prefix while the secret portion is redacted; change the conditional handling in redact (the branches referencing pattern.groups) accordingly.
🤖 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/README.md`:
- Around line 106-112: The README shows NMEM_SPACE assigned but not applied to
the Copilot launch; update the snippet so the environment variable is actually
exported or used when invoking the Copilot binary (e.g., either export
NMEM_SPACE="Research Agent" before running or run the command with the variable
in-line) so sessions are launched into the specified NMEM_SPACE; reference the
NMEM_SPACE variable and the copilot command in the updated example.
In `@nowledge-mem-copilot-cli-plugin/RELEASING.md`:
- Around line 13-15: Update the three release validation commands in
RELEASING.md so they are cwd-safe by referencing the plugin directory explicitly
(use the plugin path prefix with integrations.json, hooks/hooks.json and
tests/), e.g. run the json.tool check against
nowledge-mem-copilot-cli-plugin/integrations.json and
nowledge-mem-copilot-cli-plugin/hooks/hooks.json and run pytest against
nowledge-mem-copilot-cli-plugin/tests/ so the checks succeed regardless of the
current working directory.
In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py`:
- Around line 230-237: The run_json function currently calls subprocess.run
without a timeout which can hang the worker; modify run_json (the subprocess.run
invocation) to pass a reasonable timeout (e.g., a few seconds) and catch
subprocess.TimeoutExpired to raise a RuntimeError with a clear message including
the command and that it timed out; ensure normal error handling still applies
(use proc.stderr/proc.stdout if available) and that an expired timeout does not
leave resources open.
- Around line 579-599: The triage/distill block (using should_try_distill,
run_json and build_nmem_command) can raise and prevent saving state after the
thread append; wrap the calls to run_json for the "t triage" and "t distill"
steps in try/except so any exception is caught, logged, and does not propagate —
still set distill_attempted only when triage returns should_distill or when
distill actually ran successfully, and always allow execution to continue to the
state update (state["last_distill_ts"]) and subsequent save; reference the
triage variable, distill_attempted flag, run_json, build_nmem_command and
state["last_distill_ts"] when adding the error handling.
- Around line 183-240: Add the future annotations import at the top of the
script to enable PEP 563-style postponed evaluation so union types like those
used in find_index(event_id: str | None) and load_state(session_id: str) ->
tuple[Path, dict] (and other annotations using |) work on Python 3.9 and
earlier; specifically insert from __future__ import annotations as the first
non-shebang/import line in
nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py so the existing
type hints require no other syntax changes.
- Around line 304-348: The hook compares hook_timestamp (raw JSON) to
event_timestamp_ms() (int ms) causing type errors when hook_timestamp is an ISO
string; normalize hook_timestamp to an integer millisecond timestamp before the
comparison in the block that locates current_turn_end. Add a small helper (e.g.,
normalize_hook_timestamp) used where hook_timestamp is read (variable
hook_timestamp) to convert ISO strings or numeric seconds/ms to an int ms value
(or None if absent/invalid), then use that normalized value in the comparison
with event_timestamp_ms(event) inside the current_turn_end selection; keep
existing functions load_state, load_events and state_token unchanged.
---
Duplicate comments:
In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py`:
- Around line 125-133: The redact function currently drops captured prefixes
when pattern.groups == 1 (e.g., "Bearer ") by replacing the whole match with
"[REDACTED]"; update the logic in redact to treat any pattern with one or more
capture groups as prefix-preserving by using the first-group backreference
(e.g., pattern.sub(r"\1[REDACTED]", redacted)) for pattern.groups >= 1 so
SECRET_PATTERNS that capture prefixes keep the prefix while the secret portion
is redacted; change the conditional handling in redact (the branches referencing
pattern.groups) accordingly.
🪄 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: 7ad46be1-f6a5-4a08-aa54-3ad9e3b6bcec
📒 Files selected for processing (8)
nowledge-mem-copilot-cli-plugin/CHANGELOG.mdnowledge-mem-copilot-cli-plugin/README.mdnowledge-mem-copilot-cli-plugin/RELEASING.mdnowledge-mem-copilot-cli-plugin/hooks/hooks.jsonnowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pynowledge-mem-copilot-cli-plugin/skills/read-working-memory/SKILL.mdnowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.mdnowledge-mem-copilot-cli-plugin/tests/test_copilot_plugin.py
✅ Files skipped from review due to trivial changes (2)
- nowledge-mem-copilot-cli-plugin/skills/save-thread/SKILL.md
- nowledge-mem-copilot-cli-plugin/skills/read-working-memory/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- nowledge-mem-copilot-cli-plugin/hooks/hooks.json
| Spaces are optional. If one Copilot CLI process naturally belongs to one project or agent lane, launch Copilot CLI with: | ||
|
|
||
| ```bash | ||
| NMEM_SPACE="Research Agent" | ||
| ``` | ||
|
|
||
| The session-start Working Memory read, per-turn guidance, skills, and background capture will then stay in that lane automatically. |
There was a problem hiding this comment.
Show NMEM_SPACE attached to the Copilot launch.
The snippet currently sets an environment variable but does not launch Copilot CLI or export it, so copying it as-is will not put a session into that space.
🛠️ Proposed docs fix
-NMEM_SPACE="Research Agent"
+NMEM_SPACE="Research Agent" copilotOr, for a shell session:
export NMEM_SPACE="Research Agent"
copilot🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nowledge-mem-copilot-cli-plugin/README.md` around lines 106 - 112, The README
shows NMEM_SPACE assigned but not applied to the Copilot launch; update the
snippet so the environment variable is actually exported or used when invoking
the Copilot binary (e.g., either export NMEM_SPACE="Research Agent" before
running or run the command with the variable in-line) so sessions are launched
into the specified NMEM_SPACE; reference the NMEM_SPACE variable and the copilot
command in the updated example.
| 1. Verify `integrations.json` is valid JSON: `python3 -m json.tool integrations.json > /dev/null` | ||
| 2. Verify `hooks/hooks.json` is valid JSON: `python3 -m json.tool hooks/hooks.json > /dev/null` | ||
| 3. Run fixture tests: `uv run --with pytest pytest tests/ -v` |
There was a problem hiding this comment.
Make the release validation commands cwd-safe.
Line 13 is repo-root relative, while Lines 14-15 are plugin-directory relative. As written, maintainers will fail either integrations.json or hooks/hooks.json/tests/ depending on where they run the checklist.
🛠️ Proposed docs fix
-1. Verify `integrations.json` is valid JSON: `python3 -m json.tool integrations.json > /dev/null`
-2. Verify `hooks/hooks.json` is valid JSON: `python3 -m json.tool hooks/hooks.json > /dev/null`
-3. Run fixture tests: `uv run --with pytest pytest tests/ -v`
+1. From the repository root, verify `integrations.json` is valid JSON: `python3 -m json.tool integrations.json > /dev/null`
+2. Verify `hooks/hooks.json` is valid JSON: `python3 -m json.tool nowledge-mem-copilot-cli-plugin/hooks/hooks.json > /dev/null`
+3. Run fixture tests: `uv run --with pytest pytest nowledge-mem-copilot-cli-plugin/tests/ -v`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nowledge-mem-copilot-cli-plugin/RELEASING.md` around lines 13 - 15, Update
the three release validation commands in RELEASING.md so they are cwd-safe by
referencing the plugin directory explicitly (use the plugin path prefix with
integrations.json, hooks/hooks.json and tests/), e.g. run the json.tool check
against nowledge-mem-copilot-cli-plugin/integrations.json and
nowledge-mem-copilot-cli-plugin/hooks/hooks.json and run pytest against
nowledge-mem-copilot-cli-plugin/tests/ so the checks succeed regardless of the
current working directory.
| def build_nmem_command(nmem_bin: str, *args: str) -> list[str]: | ||
| if nmem_bin.lower().endswith(".cmd"): | ||
| return [ | ||
| "cmd.exe", | ||
| "/s", | ||
| "/c", | ||
| subprocess.list2cmdline([nmem_bin, *args]), | ||
| ] | ||
| return [nmem_bin, *args] | ||
|
|
||
|
|
||
| def load_events(transcript_path: str) -> list[dict]: | ||
| with open(transcript_path, encoding="utf-8") as fh: | ||
| return [json.loads(line) for line in fh] | ||
|
|
||
|
|
||
| def find_index(events: list[dict], event_id: str | None) -> int | None: | ||
| if not event_id: | ||
| return None | ||
| for idx, event in enumerate(events): | ||
| if event.get("id") == event_id: | ||
| return idx | ||
| return None | ||
|
|
||
|
|
||
| def collect_messages(events: list[dict]) -> list[dict]: | ||
| messages: list[dict] = [] | ||
| for event in events: | ||
| etype = event.get("type") | ||
| if etype == "user.message": | ||
| text = clean_user_text(event) | ||
| role = "user" | ||
| elif etype == "assistant.message": | ||
| text = clean_assistant_text(event) | ||
| role = "assistant" | ||
| else: | ||
| continue | ||
|
|
||
| if not text: | ||
| continue | ||
| if messages and messages[-1]["role"] == role: | ||
| messages[-1]["content"] += "\n\n" + text | ||
| else: | ||
| messages.append({"role": role, "content": text}) | ||
| return messages | ||
|
|
||
|
|
||
| def run_json(args: list[str]) -> dict: | ||
| proc = subprocess.run(args, capture_output=True, text=True) | ||
| if proc.returncode != 0: | ||
| raise RuntimeError( | ||
| proc.stderr.strip() or proc.stdout.strip() or "command failed" | ||
| ) | ||
| output = proc.stdout.strip() | ||
| return json.loads(output) if output else {} | ||
|
|
||
|
|
||
| def load_state(session_id: str) -> tuple[Path, dict]: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the repo declares/enforces a Python 3.10+ requirement for this hook.
rg -n "Python 3\\.10|Python >=3\\.10|requires-python|python_requires|from __future__ import annotations|str \\| None|int \\| None" nowledge-mem-copilot-cli-plugin README.md pyproject.toml setup.cfg setup.py 2>/dev/null || trueRepository: nowledge-co/community
Length of output: 317
🏁 Script executed:
#!/bin/bash
# Get the full copilot-stop-save.py file to examine the syntax usage
wc -l nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Search for all instances of union type syntax and future imports in the file
rg -n "from __future__|str \| |int \| |list \| |dict \| " nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 195
🏁 Script executed:
#!/bin/bash
# Check the shebang and opening lines to understand the Python environment requirement
head -20 nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 620
🏁 Script executed:
#!/bin/bash
# Check if there's any Python version specification in the installer or README
cat nowledge-mem-copilot-cli-plugin/README.md 2>/dev/null | head -50Repository: nowledge-co/community
Length of output: 1391
🏁 Script executed:
#!/bin/bash
# Check setup.py or pyproject.toml for Python version constraints
find nowledge-mem-copilot-cli-plugin -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" | xargs cat 2>/dev/null | grep -i "python" | head -20Repository: nowledge-co/community
Length of output: 47
Add from __future__ import annotations to support Python 3.9 and earlier.
The hook is invoked as plain python3 without a version constraint, but the script uses union type syntax (str | None, int | None) that requires Python 3.10+ or postponed annotation evaluation. The repository has no documented Python 3.10+ requirement. Add the future import to make the script compatible with Python 3.9 and earlier:
Proposed fix
#!/usr/bin/env python3
"""Copilot CLI session capture for Nowledge Mem.
@@ -9,6 +9,8 @@ thread ID: ``copilot-{session_id}`` (stable per-session, enables incremental
append). State management with file locking for concurrent session safety.
"""
+from __future__ import annotations
+
import hashlib
import json
import osAffects lines 199 and 273.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def build_nmem_command(nmem_bin: str, *args: str) -> list[str]: | |
| if nmem_bin.lower().endswith(".cmd"): | |
| return [ | |
| "cmd.exe", | |
| "/s", | |
| "/c", | |
| subprocess.list2cmdline([nmem_bin, *args]), | |
| ] | |
| return [nmem_bin, *args] | |
| def load_events(transcript_path: str) -> list[dict]: | |
| with open(transcript_path, encoding="utf-8") as fh: | |
| return [json.loads(line) for line in fh] | |
| def find_index(events: list[dict], event_id: str | None) -> int | None: | |
| if not event_id: | |
| return None | |
| for idx, event in enumerate(events): | |
| if event.get("id") == event_id: | |
| return idx | |
| return None | |
| def collect_messages(events: list[dict]) -> list[dict]: | |
| messages: list[dict] = [] | |
| for event in events: | |
| etype = event.get("type") | |
| if etype == "user.message": | |
| text = clean_user_text(event) | |
| role = "user" | |
| elif etype == "assistant.message": | |
| text = clean_assistant_text(event) | |
| role = "assistant" | |
| else: | |
| continue | |
| if not text: | |
| continue | |
| if messages and messages[-1]["role"] == role: | |
| messages[-1]["content"] += "\n\n" + text | |
| else: | |
| messages.append({"role": role, "content": text}) | |
| return messages | |
| def run_json(args: list[str]) -> dict: | |
| proc = subprocess.run(args, capture_output=True, text=True) | |
| if proc.returncode != 0: | |
| raise RuntimeError( | |
| proc.stderr.strip() or proc.stdout.strip() or "command failed" | |
| ) | |
| output = proc.stdout.strip() | |
| return json.loads(output) if output else {} | |
| def load_state(session_id: str) -> tuple[Path, dict]: | |
| #!/usr/bin/env python3 | |
| """Copilot CLI session capture for Nowledge Mem. | |
| This module is invoked by the Copilot CLI stop hook to capture and persist | |
| session state (messages, metadata, transcript) for incremental thread updates via | |
| Nowledge Mem. Session state is persisted to `.copilot-cli/session/{session_id}.json`; | |
| thread ID: ``copilot-{session_id}`` (stable per-session, enables incremental | |
| append). State management with file locking for concurrent session safety. | |
| """ | |
| from __future__ import annotations | |
| import hashlib | |
| import json | |
| import os |
🧰 Tools
🪛 Ruff (0.15.10)
[error] 231-231: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py` around lines
183 - 240, Add the future annotations import at the top of the script to enable
PEP 563-style postponed evaluation so union types like those used in
find_index(event_id: str | None) and load_state(session_id: str) -> tuple[Path,
dict] (and other annotations using |) work on Python 3.9 and earlier;
specifically insert from __future__ import annotations as the first
non-shebang/import line in
nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py so the existing
type hints require no other syntax changes.
| def run_json(args: list[str]) -> dict: | ||
| proc = subprocess.run(args, capture_output=True, text=True) | ||
| if proc.returncode != 0: | ||
| raise RuntimeError( | ||
| proc.stderr.strip() or proc.stdout.strip() or "command failed" | ||
| ) | ||
| output = proc.stdout.strip() | ||
| return json.loads(output) if output else {} |
There was a problem hiding this comment.
Put a timeout on nmem subprocess calls.
The Stop hook runs after each response; a hung nmem command can keep the worker and session lock alive indefinitely, blocking later captures for that session.
⏱️ Proposed fix
DISTILL_COOLDOWN_SECS = 120
+NMEM_COMMAND_TIMEOUT_SECS = 30
@@
-def run_json(args: list[str]) -> dict:
- proc = subprocess.run(args, capture_output=True, text=True)
+def run_json(args: list[str]) -> dict:
+ try:
+ proc = subprocess.run(
+ args,
+ capture_output=True,
+ text=True,
+ timeout=NMEM_COMMAND_TIMEOUT_SECS,
+ )
+ except subprocess.TimeoutExpired as exc:
+ raise RuntimeError(
+ f"nmem command timed out after {NMEM_COMMAND_TIMEOUT_SECS}s"
+ ) from exc
if proc.returncode != 0:
raise RuntimeError(
proc.stderr.strip() or proc.stdout.strip() or "command failed"
)🧰 Tools
🪛 Ruff (0.15.10)
[error] 231-231: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py` around lines
230 - 237, The run_json function currently calls subprocess.run without a
timeout which can hang the worker; modify run_json (the subprocess.run
invocation) to pass a reasonable timeout (e.g., a few seconds) and catch
subprocess.TimeoutExpired to raise a RuntimeError with a clear message including
the command and that it timed out; ensure normal error handling still applies
(use proc.stderr/proc.stdout if available) and that an expired timeout does not
leave resources open.
| session_id = hook_input.get("sessionId") | ||
| transcript_path = hook_input.get("transcriptPath") | ||
| stop_reason = hook_input.get("stopReason") | ||
| hook_timestamp = hook_input.get("timestamp") | ||
|
|
||
| if not session_id or not transcript_path or stop_reason != "end_turn": | ||
| return 0 | ||
| if not Path(transcript_path).exists(): | ||
| return 0 | ||
|
|
||
| nmem_bin = shutil.which("nmem") or shutil.which("nmem.cmd") | ||
| if not nmem_bin: | ||
| log({"session_id": session_id, "action": "skip", "reason": "nmem_missing"}) | ||
| return 0 | ||
|
|
||
| lock_path = STATE_DIR / f"{state_token(session_id)}.lock" | ||
| lock_path.parent.mkdir(parents=True, exist_ok=True) | ||
| with lock_path.open("w+", encoding="utf-8") as lock_fh: | ||
| if fcntl: | ||
| fcntl.flock(lock_fh, fcntl.LOCK_EX) | ||
| else: | ||
| import msvcrt | ||
| lock_fh.write("\0") | ||
| lock_fh.flush() | ||
| lock_fh.seek(0) | ||
| msvcrt.locking(lock_fh.fileno(), msvcrt.LK_LOCK, 1) | ||
|
|
||
| state_path, state = load_state(session_id) | ||
| events = load_events(transcript_path) | ||
|
|
||
| # --- Locate current turn end --- | ||
| turn_end_events = [ | ||
| e for e in events if e.get("type") == "assistant.turn_end" | ||
| ] | ||
| if not turn_end_events: | ||
| return 0 | ||
| current_turn_end = next( | ||
| ( | ||
| event | ||
| for event in reversed(turn_end_events) | ||
| if hook_timestamp is None | ||
| or ( | ||
| event_timestamp_ms(event) is not None | ||
| and event_timestamp_ms(event) <= hook_timestamp | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "copilot-stop-save.py" -type fRepository: nowledge-co/community
Length of output: 127
🏁 Script executed:
wc -l ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 131
🏁 Script executed:
sed -n '304,348p' ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 1671
🏁 Script executed:
grep -n "def event_timestamp_ms" ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 119
🏁 Script executed:
sed -n '273,290p' ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 644
🏁 Script executed:
grep -n "hook_input" ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py | head -20Repository: nowledge-co/community
Length of output: 320
🏁 Script executed:
grep -n "def extract_input" ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 110
🏁 Script executed:
sed -n '106,130p' ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 861
🏁 Script executed:
grep -n "hook_timestamp" ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 235
🏁 Script executed:
sed -n '340,360p' ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 783
🏁 Script executed:
grep -n "event_timestamp_ms" ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 253
🏁 Script executed:
head -50 ./nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.pyRepository: nowledge-co/community
Length of output: 1502
Normalize the hook timestamp before comparing it to event milliseconds.
event_timestamp_ms() returns an integer (milliseconds), but hook_timestamp at line 307 is extracted raw from JSON. If the Stop payload supplies an ISO timestamp string, the comparison at line 347 raises a type error and capture is skipped by the hook wrapper.
Extract the proposed helper function and apply normalization:
🐛 Proposed fix
def event_timestamp_ms(event: dict) -> int | None:
- raw = event.get("timestamp")
+ return timestamp_ms(event.get("timestamp"))
+
+
+def timestamp_ms(raw) -> int | None:
if not raw:
return None
+ if isinstance(raw, (int, float)):
+ value = int(raw)
+ return value if value > 10_000_000_000 else value * 1000
+ if not isinstance(raw, str):
+ return None
return int(
datetime.fromisoformat(raw.replace("Z", "+00:00")).timestamp() * 1000
)
@@
- hook_timestamp = hook_input.get("timestamp")
+ hook_timestamp = timestamp_ms(hook_input.get("timestamp"))
@@
if hook_timestamp is None
or (
event_timestamp_ms(event) is not None
and event_timestamp_ms(event) <= hook_timestamp
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py` around lines
304 - 348, The hook compares hook_timestamp (raw JSON) to event_timestamp_ms()
(int ms) causing type errors when hook_timestamp is an ISO string; normalize
hook_timestamp to an integer millisecond timestamp before the comparison in the
block that locates current_turn_end. Add a small helper (e.g.,
normalize_hook_timestamp) used where hook_timestamp is read (variable
hook_timestamp) to convert ISO strings or numeric seconds/ms to an int ms value
(or None if absent/invalid), then use that normalized value in the comparison
with event_timestamp_ms(event) inside the current_turn_end selection; keep
existing functions load_state, load_events and state_token unchanged.
| triage = None | ||
| distill_attempted = False | ||
| if should_try_distill: | ||
| triage = run_json( | ||
| build_nmem_command( | ||
| nmem_bin, "--json", "t", "triage", thread_id | ||
| ) | ||
| ) | ||
| if triage.get("should_distill"): | ||
| distill_attempted = True | ||
| run_json( | ||
| build_nmem_command( | ||
| nmem_bin, | ||
| "--json", | ||
| "t", | ||
| "distill", | ||
| thread_id, | ||
| "--triage", | ||
| ) | ||
| ) | ||
| state["last_distill_ts"] = now_ts |
There was a problem hiding this comment.
Do not let auto-distill failures roll back capture state.
The thread import/append has already succeeded before Line 581. If t triage or t distill fails, the exception prevents Lines 601-604 from saving state, so the next Stop hook can append the same slice again.
🐛 Proposed fix
triage = None
distill_attempted = False
if should_try_distill:
- triage = run_json(
- build_nmem_command(
- nmem_bin, "--json", "t", "triage", thread_id
+ try:
+ triage = run_json(
+ build_nmem_command(
+ nmem_bin, "--json", "t", "triage", thread_id
+ )
)
- )
- if triage.get("should_distill"):
- distill_attempted = True
- run_json(
- build_nmem_command(
- nmem_bin,
- "--json",
- "t",
- "distill",
- thread_id,
- "--triage",
+ if triage.get("should_distill"):
+ distill_attempted = True
+ run_json(
+ build_nmem_command(
+ nmem_bin,
+ "--json",
+ "t",
+ "distill",
+ thread_id,
+ "--triage",
+ )
)
+ state["last_distill_ts"] = now_ts
+ except Exception as exc:
+ log(
+ {
+ "session_id": session_id,
+ "action": "distill_skip",
+ "reason": str(exc),
+ }
)
- state["last_distill_ts"] = now_ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py` around lines
579 - 599, The triage/distill block (using should_try_distill, run_json and
build_nmem_command) can raise and prevent saving state after the thread append;
wrap the calls to run_json for the "t triage" and "t distill" steps in
try/except so any exception is caught, logged, and does not propagate — still
set distill_attempted only when triage returns should_distill or when distill
actually ran successfully, and always allow execution to continue to the state
update (state["last_distill_ts"]) and subsequent save; reference the triage
variable, distill_attempted flag, run_json, build_nmem_command and
state["last_distill_ts"] when adding the error handling.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d097ce3023
ℹ️ 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".
| session_id = hook_input.get("sessionId") | ||
| transcript_path = hook_input.get("transcriptPath") | ||
| stop_reason = hook_input.get("stopReason") |
There was a problem hiding this comment.
With this plugin configured to use PascalCase hook names (Stop, UserPromptSubmit, SessionStart), Copilot CLI can send VS Code-compatible payloads (session_id, transcript_path, stop_reason, ISO timestamp) rather than camelCase. The script only reads camelCase keys here, so session_id/transcript_path resolve to None and the early guard exits without saving any thread data. GitHub’s CLI reference documents both payload shapes for agentStop / Stop and notes snake_case for PascalCase events, so this causes silent loss of session capture in that mode.
Useful? React with 👍 / 👎.
|
bugbot run |
|
@HamsteRider-m thanks a lot for your amazing work!!! |
2 participants
Summary
nowledge-mem-copilot-cli-pluginpackage with Copilot-native hooks, skills, installer, docs, and testsintegrations.jsonso it can ship through the Nowledge community marketplaceWhy this exists
GitHub Copilot CLI can load Claude Code-style plugins, but using the Claude Code integration directly is brittle in practice because some lifecycle assumptions, command surfaces, and config paths are Claude-specific.
This PR therefore treats the Copilot plugin as a real adaptation, not a straight copy:
Copilot surface note
This plugin is now intentionally skill-only on the user-facing surface.
Earlier iterations shipped both command docs and skills, which could make Copilot expose overlapping entries for the same underlying action. That was especially confusing for explicit save flows.
The current shape is:
read-working-memory,search-memory,distill-memory, andsave-threadsave/search/sum/statusFor direct terminal troubleshooting outside the skill surface, users should run the
nmemCLI directly, for examplenmem status.Implementation approach
The recorded build session shows an orchestrator-worker approach rather than a role-play pipeline:
The captured planning artifacts also show three design corrections made before/during implementation:
copilot-{session_id}What the reviewer should look at first
If you want to review from the source of the design rather than only from the final diff, start here:
nowledge-mem-copilot-cli-plugin/scripts/copilot-stop-save.py— core Copilot transcript capture and incremental thread save logicnowledge-mem-copilot-cli-plugin/hooks/hooks.json— lifecycle wiring forSessionStart,UserPromptSubmit, and asyncStopnowledge-mem-copilot-cli-plugin/README.mdandAGENTS.md— user-facing and agent-facing behavior model, now documented as skill-onlyintegrations.json— registry entry for the newcopilot-cliintegrationSession evidence and validation notes
The Copilot session artifacts I reviewed show:
claude-opus-4.6claude-haiku-4.5gpt-5.4I did not find reliable evidence for a specific reasoning-effort tier such as
high, so I am intentionally not claiming that here.The session artifacts clearly preserve the planning and review flow. They are less consistent on which validation steps were fully completed versus merely planned, so I would treat the recorded validation narrative as implementation context rather than as the authoritative release checklist.
Test plan
scripts/install-hooks.sh🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests