feat(codex): bundle host-level stop hook installer#178
Open
HamsteRider-m wants to merge 8 commits intonowledge-co:mainfrom
Open
feat(codex): bundle host-level stop hook installer#178HamsteRider-m wants to merge 8 commits intonowledge-co:mainfrom
HamsteRider-m wants to merge 8 commits intonowledge-co:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces automatic Codex transcript capture via a bundled host-level Stop hook installer. It adds hook installation, configuration management, and thread refresh scripts; updates the Codex CLI integration to enable Changes
Sequence DiagramsequenceDiagram
participant Codex as Codex CLI
participant Hook as Stop Hook<br/>(nmem-stop-save.py)
participant Lock as State Lock<br/>(fcntl)
participant Parser as nmem_cli<br/>Parser
participant API as nmem API
Codex->>Hook: Invoke on session stop<br/>(session_id, transcript_path)
Hook->>Hook: Read & validate payload
Hook->>Parser: parse_codex_session_streaming<br/>(transcript_path)
Parser-->>Hook: Parsed messages & metadata
Hook->>Lock: Acquire lock<br/>(short timeout)
Lock-->>Hook: Lock acquired
Hook->>Hook: Load prior state<br/>(content_hash, message_count)
alt Content unchanged
Hook-->>Codex: Skip (content_hash match)
else New/delta content
Hook->>Hook: Compute new content_hash<br/>& derive thread_id/title
Hook->>API: Check thread exists<br/>(api_get_optional)
alt Thread exists
API-->>Hook: Existing thread
Hook->>API: Append messages<br/>(deduplicate=True)
API-->>Hook: Append result
else Thread missing
API-->>Hook: Not found
Hook->>API: Create thread<br/>(title + messages)
API-->>Hook: New thread_id
end
Hook->>Hook: Update & persist state<br/>(content_hash, message_count, etc.)
Hook-->>Codex: Return (exit 0)
end
Lock->>Lock: Release lock
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span multiple technologies (Python, Node.js, TOML, JSON) and introduce intricate logic across installer/hook/refresh scripts with state management, file I/O, API interaction, and configuration merging. While individual files follow consistent patterns, the heterogeneous nature of implementations (shell shebang rewriting, fcntl-based locking, config normalization, title derivation heuristics) and interconnected workflows demand separate reasoning for each component, pushing toward the higher end of complexity. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR upgrades the Codex integration to support automatic session capture by bundling a host-level Stop hook installer and a runtime hook that imports Codex transcripts directly via transcript_path.
Changes:
- Adds a bundled installer (
scripts/install_hooks.py) that installs/merges~/.codex/hooks.json, copies the runtime hook into~/.codex/hooks/, and enablescodex_hooks. - Adds a
Stopruntime hook (hooks/nmem-stop-save.py) that parses/imports the current Codex transcript usingnmem_cli.session_import. - Updates docs, release notes, validation script, and install commands to preserve hidden files and bump Codex plugin version to
0.1.3.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates top-level integration table to mention running the bundled Codex hook installer. |
| nowledge-mem-codex-prompts/README.md | Deprecation notice updated to reflect the new hook installer capability in the plugin. |
| nowledge-mem-codex-plugin/scripts/validate-plugin.mjs | Adds a Codex plugin package validator (required files, version checks, docs/install command assertions). |
| nowledge-mem-codex-plugin/scripts/install_hooks.py | Adds host-level hook installer to copy hook, merge hooks.json, and enable codex_hooks. |
| nowledge-mem-codex-plugin/RELEASING.md | Adds release checklist and smoke test steps for the Codex plugin + hook surfaces. |
| nowledge-mem-codex-plugin/README.md | Documents hook support/installer usage and troubleshooting for automatic capture. |
| nowledge-mem-codex-plugin/hooks/nmem-stop-save.py | Adds the Codex Stop hook runtime that imports the transcript via transcript_path. |
| nowledge-mem-codex-plugin/CHANGELOG.md | Adds 0.1.3 entry describing the hook installer/runtime and install command fixes. |
| nowledge-mem-codex-plugin/.codex-plugin/plugin.json | Bumps plugin version from 0.1.2 to 0.1.3. |
| integrations.json | Bumps Codex integration version and fixes install/update commands to copy hidden files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 0593a57530
ℹ️ 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".
![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 (2)
nowledge-mem-codex-plugin/RELEASING.md (1)
20-26: Expand checklist to include all version-sync surfaces.Lines 20-26 miss two release-critical sync points used by your validator:
integrations.json(codex-cli.version) andscripts/validate-plugin.mjsexpected version. Adding both prevents avoidable release-time failures.Suggested checklist additions
- bump `.codex-plugin/plugin.json` version - add a top entry to `CHANGELOG.md` +- bump `integrations.json` -> `integrations[id="codex-cli"].version` +- update expected version checks in `scripts/validate-plugin.mjs` - keep install examples on `cp -r .../. ...` so `.codex-plugin/` is copied - keep `scripts/install_hooks.py` idempotent - keep `hooks/nmem-stop-save.py` focused on direct transcript import via `transcript_path` - re-run `node nowledge-mem-codex-plugin/scripts/validate-plugin.mjs`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-codex-plugin/RELEASING.md` around lines 20 - 26, Update the release checklist to also sync version strings in integrations.json (update the codex-cli.version field) and to update the expected version constant inside scripts/validate-plugin.mjs so the validator and plugin.json/.codex-plugin/plugin.json are consistent; edit RELEASING.md to list these two steps and ensure the same semantic version value is applied to .codex-plugin/plugin.json, integrations.json (codex-cli.version) and the version check inside scripts/validate-plugin.mjs.nowledge-mem-codex-plugin/scripts/install_hooks.py (1)
21-24: Handle malformedhooks.jsongracefully.If
~/.codex/hooks.jsonis invalid JSON, installation aborts immediately. Consider catchingjson.JSONDecodeError, backing up the invalid file, and continuing with an empty structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-codex-plugin/scripts/install_hooks.py` around lines 21 - 24, load_json currently raises on malformed JSON which aborts install; update load_json to catch json.JSONDecodeError, log/warn about the invalid hooks file, make a backup copy or rename the bad file (e.g., append a timestamp and ".bak"), and then return an empty dict so installation continues; ensure this change touches the load_json function and uses the provided Path variable to perform the backup and to return {} on failure.
🤖 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-codex-plugin/hooks/nmem-stop-save.py`:
- Around line 116-119: The subprocess.run call that executes nmem (built from
nmem_bin with args in variable command) can block indefinitely; add a timeout
argument (e.g. timeout=SECONDS) to the subprocess.run invocation and catch
subprocess.TimeoutExpired around that call so you can kill/cleanup if it exceeds
the timeout and log/report the timeout; update the try/finally block that
surrounds the run to handle the TimeoutExpired exception and ensure
temp_path/thread_id cleanup still occurs (refer to the variables command,
nmem_bin, temp_path, thread_id and the subprocess.run call).
- Around line 59-70: The code constructs transcript_path =
Path(payload.get("transcript_path") or "") which turns an empty value into
Path("") (the current directory), allowing the later check (not transcript_path
or not transcript_path.exists()) to pass wrongly; instead, first read the raw
value (e.g., raw_transcript = payload.get("transcript_path")), if it's falsy
return the skip message, then construct transcript_path = Path(raw_transcript)
and validate both transcript_path.exists() and transcript_path.is_file() (or use
not (transcript_path.exists() and transcript_path.is_file()) to return the skip
error). Update any logs (log(f"transcript=...")) to use the validated
transcript_path and ensure the function (the early-return logic around
session_id and transcript_path) uses these checks.
In `@nowledge-mem-codex-plugin/scripts/install_hooks.py`:
- Around line 81-84: The current regex replacement for codex_hooks can modify
keys outside the [features] section (symbols: codex_hooks and [features]), so
change the implementation in install_hooks.py to parse the file as TOML (e.g.,
using a toml or tomli/tomllib library), set data["features"]["codex_hooks"] =
True (creating the features table if missing), and then write the serialized
TOML back instead of doing blind regex replacements; ensure you preserve or
minimally transform existing formatting/ordering if possible by using a TOML
library that supports round-trip or write back a clean TOML representation.
- Line 42: The suffix construction uses an unnecessary f-string: change the call
that sets backup (the expression assigning to backup which uses
GLOBAL_HOOKS_FILE.with_suffix(f".json.bak")) to use a plain string literal
instead (GLOBAL_HOOKS_FILE.with_suffix(".json.bak")) so the f-prefix is removed
and Ruff F541 is resolved while leaving the same suffix value.
In `@nowledge-mem-codex-plugin/scripts/validate-plugin.mjs`:
- Around line 53-75: The script currently calls readFileSync/JSON.parse for the
manifest, changelog, and README unconditionally which can throw and abort
validation early; modify validate-plugin.mjs so you first check for file
presence and safely parse the manifest (wrap JSON.parse/readFileSync in
try/catch or use fs.existsSync) and call fail() on errors but continue
execution, and only read CHANGELOG.md into the changelog variable and README.md
into readme if the prior presence/manifest checks passed; ensure you still call
ok()/fail() for manifest.name, manifest.version, the changelog entry, and README
phrases but guard each read/parse behind the corresponding check to avoid
throwing and allow reporting all violations in one run.
In `@nowledge-mem-codex-prompts/README.md`:
- Line 3: The README currently implies automatic session capture is enabled by
default; update the deprecation note sentence referencing the "bundled
host-level hook installer" / "automatic session capture" to explicitly state
that automatic capture begins only after running the bundled hook installer
(i.e., mention that users must run the installer to enable auto-capture), and
adjust wording around "provides the same capabilities as composable Codex
skills, plus a bundled host-level hook installer for automatic session capture"
to make the dependency explicit.
---
Nitpick comments:
In `@nowledge-mem-codex-plugin/RELEASING.md`:
- Around line 20-26: Update the release checklist to also sync version strings
in integrations.json (update the codex-cli.version field) and to update the
expected version constant inside scripts/validate-plugin.mjs so the validator
and plugin.json/.codex-plugin/plugin.json are consistent; edit RELEASING.md to
list these two steps and ensure the same semantic version value is applied to
.codex-plugin/plugin.json, integrations.json (codex-cli.version) and the version
check inside scripts/validate-plugin.mjs.
In `@nowledge-mem-codex-plugin/scripts/install_hooks.py`:
- Around line 21-24: load_json currently raises on malformed JSON which aborts
install; update load_json to catch json.JSONDecodeError, log/warn about the
invalid hooks file, make a backup copy or rename the bad file (e.g., append a
timestamp and ".bak"), and then return an empty dict so installation continues;
ensure this change touches the load_json function and uses the provided Path
variable to perform the backup and to return {} on failure.
🪄 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: 70a4b43b-0b12-4333-aa84-90526091f22f
📒 Files selected for processing (10)
README.mdintegrations.jsonnowledge-mem-codex-plugin/.codex-plugin/plugin.jsonnowledge-mem-codex-plugin/CHANGELOG.mdnowledge-mem-codex-plugin/README.mdnowledge-mem-codex-plugin/RELEASING.mdnowledge-mem-codex-plugin/hooks/nmem-stop-save.pynowledge-mem-codex-plugin/scripts/install_hooks.pynowledge-mem-codex-plugin/scripts/validate-plugin.mjsnowledge-mem-codex-prompts/README.md
Member
|
bugbot run |
Member
|
@codex kindly review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0593a57530
ℹ️ 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".
Author
|
Pushed follow-up commits to Additional changes on top of the original PR:
Current validation run:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
nowledge-mem-codex-plugin/hooks/nmem-stop-save.py (1)
48-56: Config loading pattern duplicates existing code in bub-plugin.This config-loading logic is nearly identical to
nowledge-mem-bub-plugin/src/nowledge_mem_bub/client.py:37-64. Consider extracting to a shared utility in a future refactor to maintain consistency and reduce duplication. For now, the implementation is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-codex-plugin/hooks/nmem-stop-save.py` around lines 48 - 56, The configure_nmem_env function duplicates config-loading logic found elsewhere; extract the shared behavior into a new helper (e.g., a function named load_nmem_config that reads HOME/.nowledge-mem/config.json and normalizes keys to api_url and api_key) and have configure_nmem_env call that helper to set NMEM_API_URL/NMEM_API_KEY; update the other module to use the same helper so both modules share the single implementation and avoid duplication.
🤖 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-codex-plugin/hooks/nmem-stop-save.py`:
- Around line 173-176: The list comprehension building messages can include
items with message.get("role") == None which breaks downstream consumers (e.g.,
nmem t import); update the construction of messages (the comprehension that
iterates parsed.get("messages", [])) to filter out any message dicts missing a
truthy role (and/or empty content if desired) so only entries with a valid role
are included, e.g., skip messages where message.get("role") is None or falsy
before creating the {"role": ..., "content": ...} dict and ensure the variable
messages contains only well-formed role-bearing entries.
In `@nowledge-mem-codex-plugin/scripts/refresh_thread_titles.py`:
- Around line 40-56: The refresh_thread implementation performs a non-atomic
delete (cli.api_delete) followed by import (cli.api_post), which can leave a
thread deleted if the import fails; change refresh_thread to either (A) attempt
the import first and then delete only if import confirms it can replace (using
api_post semantics that support overwrite), or (B) wrap the api_post in a
try/except and on failure attempt recovery: log an error with clear context
(thread_id, title) and attempt to re-create the original thread by retrying the
import or restoring from a saved backup of messages; specifically update the
refresh_thread function to save the messages payload before deleting, perform
the safer-import-first flow if supported, otherwise perform delete then import
inside a try/except and on exception call cli.api_post to restore the original
messages and surface the failure.
---
Nitpick comments:
In `@nowledge-mem-codex-plugin/hooks/nmem-stop-save.py`:
- Around line 48-56: The configure_nmem_env function duplicates config-loading
logic found elsewhere; extract the shared behavior into a new helper (e.g., a
function named load_nmem_config that reads HOME/.nowledge-mem/config.json and
normalizes keys to api_url and api_key) and have configure_nmem_env call that
helper to set NMEM_API_URL/NMEM_API_KEY; update the other module to use the same
helper so both modules share the single implementation and avoid duplication.
🪄 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: d3f88cec-5940-44ab-a4a0-f552492844a2
📒 Files selected for processing (7)
nowledge-mem-codex-plugin/RELEASING.mdnowledge-mem-codex-plugin/hooks/nmem-stop-save.pynowledge-mem-codex-plugin/scripts/install_hooks.pynowledge-mem-codex-plugin/scripts/refresh_thread_titles.pynowledge-mem-codex-plugin/scripts/validate-plugin.mjsnowledge-mem-codex-plugin/tests/test_codex_plugin.pynowledge-mem-codex-prompts/README.md
✅ Files skipped from review due to trivial changes (1)
- nowledge-mem-codex-prompts/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- nowledge-mem-codex-plugin/scripts/validate-plugin.mjs
- nowledge-mem-codex-plugin/scripts/install_hooks.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nowledge-mem-codex-plugin/hooks/nmem-stop-save.py (1)
185-188:⚠️ Potential issue | 🟡 MinorFilter malformed parsed messages before import.
The message list currently allows entries with missing roles (or non-dict items), which can produce invalid
t importpayloads. Filter to valid role-bearing messages.Suggested fix
messages = [ - {"role": message.get("role"), "content": message.get("content", "")} + {"role": message.get("role"), "content": message.get("content", "")} for message in parsed.get("messages", []) + if isinstance(message, dict) and message.get("role") ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-codex-plugin/hooks/nmem-stop-save.py` around lines 185 - 188, The comprehension building messages allows non-dict or role-less entries; update the list construction that assigns messages (from parsed.get("messages", [])) to filter out items that are not dicts or that lack a non-empty "role" (and optionally validate role is one of expected roles) before mapping to {"role": ..., "content": ...}; i.e., replace the current list comprehension with one that checks isinstance(message, dict) and "role" in message and message["role"] truthy (and normalize content with message.get("content", "")).
🤖 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-codex-plugin/hooks/nmem-stop-save.py`:
- Around line 198-255: The STATE_FILE read-modify-write is not concurrency-safe:
wrap the load/update/save sequence with a file-level lock to prevent parallel
Stop hooks from clobbering state; specifically acquire a lock (e.g., using
filelock.FileLock or portalocker/fcntl) before calling load_json(STATE_FILE),
perform the read and your mutation of state (the state variable and the block
that assigns state[session_id] after nmem import succeeds), then atomically
persist the file (save_json to a temp file and os.replace or save_json under the
lock) and finally release the lock; ensure the lock scope includes the earlier
check that computes import_messages so you read the latest state, and add
retries or a short timeout if the lock cannot be acquired immediately.
- Around line 34-40: The load_json function can return non-dict JSON (e.g.,
list/string) causing downstream .get(...) calls to fail; update load_json (the
function named load_json that reads Path and uses json.loads) to ensure it
always returns a dict by validating the parsed value and returning {} whenever
the parsed value is not an instance of dict (also preserve the existing behavior
for missing files and JSON parse errors).
---
Duplicate comments:
In `@nowledge-mem-codex-plugin/hooks/nmem-stop-save.py`:
- Around line 185-188: The comprehension building messages allows non-dict or
role-less entries; update the list construction that assigns messages (from
parsed.get("messages", [])) to filter out items that are not dicts or that lack
a non-empty "role" (and optionally validate role is one of expected roles)
before mapping to {"role": ..., "content": ...}; i.e., replace the current list
comprehension with one that checks isinstance(message, dict) and "role" in
message and message["role"] truthy (and normalize content with
message.get("content", "")).
🪄 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: bac2f8c1-11f3-49bb-ab4a-f969bfbc02c8
📒 Files selected for processing (7)
docs/PLUGIN_DEVELOPMENT_GUIDE.mdintegrations.jsonnowledge-mem-codex-plugin/README.mdnowledge-mem-codex-plugin/hooks/nmem-stop-save.pynowledge-mem-codex-plugin/scripts/validate-plugin.mjsnowledge-mem-codex-plugin/tests/test_codex_plugin.pynowledge-mem-npx-skills/skills/check-integration/SKILL.md
✅ Files skipped from review due to trivial changes (2)
- docs/PLUGIN_DEVELOPMENT_GUIDE.md
- nowledge-mem-codex-plugin/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- integrations.json
- nowledge-mem-codex-plugin/scripts/validate-plugin.mjs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
nowledge-mem-codex-plugin/hooks/nmem-stop-save.py (1)
73-80: Consider chaining the exception for better debugging context.Static analysis flags that the
TimeoutErroris raised without chaining from theBlockingIOError. While the message is descriptive, chaining preserves the full exception context.♻️ Proposed fix
except BlockingIOError: if time.monotonic() >= deadline: - raise TimeoutError(f"timed out waiting for lock {lock_path}") + raise TimeoutError(f"timed out waiting for lock {lock_path}") from None time.sleep(0.05)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-codex-plugin/hooks/nmem-stop-save.py` around lines 73 - 80, The TimeoutError raised inside the lock acquisition loop should be chained to the caught BlockingIOError to preserve context; update the except block (where fcntl.flock(handle.fileno(), ...) is retried and BlockingIOError is caught) to re-raise the TimeoutError using "from e" (i.e., raise TimeoutError(f"timed out waiting for lock {lock_path}") from e) so the original BlockingIOError is preserved for debugging.nowledge-mem-codex-plugin/scripts/refresh_thread_titles.py (1)
20-26: Consider replacingassertwith explicit error handling.Line 24 uses
assert spec.loader is not Nonewhich could raiseAssertionErrorwith no useful context. While this is unlikely to fail in practice, explicit error handling would be clearer.♻️ Proposed fix
def load_hook_module(): hook_path = Path(__file__).resolve().parent.parent / "hooks" / "nmem-stop-save.py" spec = importlib.util.spec_from_file_location("nmem_stop_save", hook_path) - module = importlib.util.module_from_spec(spec) - assert spec.loader is not None + if spec is None or spec.loader is None: + raise RuntimeError(f"Failed to load hook module from {hook_path}") + module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) return module🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-codex-plugin/scripts/refresh_thread_titles.py` around lines 20 - 26, The use of assert in load_hook_module is unsafe; replace the assertion with explicit error checks that raise a descriptive exception if the importspec or loader is missing. In load_hook_module, after creating spec = importlib.util.spec_from_file_location(...) verify spec is not None and then verify spec.loader is not None; if either is missing raise an ImportError (or RuntimeError) with a message that includes the hook_path and which part failed, then proceed to module = importlib.util.module_from_spec(spec) and spec.loader.exec_module(module) as before. Ensure the raised error gives clear context about failing to load nmem-stop-save so callers can understand the cause.
🤖 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-codex-plugin/scripts/refresh_thread_titles.py`:
- Around line 145-158: Add a broad exception handler around the refresh_thread
call: keep the existing except SystemExit as-is, and add an except Exception as
exc block that increments errors and prints a clear error message (same format
as the SystemExit branch) so runtime errors from refresh_thread (e.g.,
RuntimeError from API calls) are counted and logged instead of crashing the
script; reference the try block that calls refresh_thread(thread_id,
desired_title, parsed.get("messages", []), args.dry_run,
original_title=current_title, original_messages=thread_payload.get("messages",
[]), thread=thread) and update the exception handling there.
---
Nitpick comments:
In `@nowledge-mem-codex-plugin/hooks/nmem-stop-save.py`:
- Around line 73-80: The TimeoutError raised inside the lock acquisition loop
should be chained to the caught BlockingIOError to preserve context; update the
except block (where fcntl.flock(handle.fileno(), ...) is retried and
BlockingIOError is caught) to re-raise the TimeoutError using "from e" (i.e.,
raise TimeoutError(f"timed out waiting for lock {lock_path}") from e) so the
original BlockingIOError is preserved for debugging.
In `@nowledge-mem-codex-plugin/scripts/refresh_thread_titles.py`:
- Around line 20-26: The use of assert in load_hook_module is unsafe; replace
the assertion with explicit error checks that raise a descriptive exception if
the importspec or loader is missing. In load_hook_module, after creating spec =
importlib.util.spec_from_file_location(...) verify spec is not None and then
verify spec.loader is not None; if either is missing raise an ImportError (or
RuntimeError) with a message that includes the hook_path and which part failed,
then proceed to module = importlib.util.module_from_spec(spec) and
spec.loader.exec_module(module) as before. Ensure the raised error gives clear
context about failing to load nmem-stop-save so callers can understand the
cause.
🪄 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: 5b77c963-ca3f-4427-abf6-433ba2b212e4
📒 Files selected for processing (4)
AGENTS.mdnowledge-mem-codex-plugin/hooks/nmem-stop-save.pynowledge-mem-codex-plugin/scripts/refresh_thread_titles.pynowledge-mem-codex-plugin/tests/test_codex_plugin.py
- Replace subprocess nmem CLI calls with direct nmem_cli API imports - Add fcntl-based file locking to prevent race conditions - Implement atomic state file writes with temp file + os.replace - Add thread existence check and use create vs append endpoints - Filter malformed messages before sync - Add rollback mechanism in refresh_thread_titles on failure - Update tests to mock nmem_cli modules instead of subprocess - Add AGENTS.md with project agent collaboration guidelines
wey-gu
force-pushed
the
codex/codex-hooks-for-nmem
branch
from
33553da to
c121917
Compare
Member
|
bugbot run |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
nowledge-mem-codex-plugin/tests/test_codex_plugin.py (1)
467-471: Also assert that the installed hook stays executable.This currently proves the shebang rewrite, but a regression in the chmod step would still pass here while leaving Codex unable to invoke the hook.
Suggested assertion
def test_install_runtime_hook_rewrites_shebang_to_current_python(self): self.module.install_runtime_hook() installed = self.module.INSTALLED_HOOK.read_text(encoding="utf-8") self.assertTrue(installed.startswith(f"#!{self.module.sys.executable}\n")) + self.assertNotEqual(self.module.INSTALLED_HOOK.stat().st_mode & 0o111, 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nowledge-mem-codex-plugin/tests/test_codex_plugin.py` around lines 467 - 471, The test test_install_runtime_hook_rewrites_shebang_to_current_python currently only checks the shebang; add an assertion that the installed hook file (self.module.INSTALLED_HOOK) is executable after module.install_runtime_hook() so a chmod regression is caught—use an OS-level executable check (e.g., os.access(path, os.X_OK) or checking stat.S_IXUSR/IXGRP/IXOTH on INSTALLED_HOOK) to assert it is executable.
🤖 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-codex-plugin/hooks/nmem-stop-save.py`:
- Around line 254-296: The api_get_optional and api_post calls in the sync block
should include an explicit timeout (e.g., timeout=...) to avoid hanging; update
the calls to api_get_optional(f"/threads/{encoded_thread_id}", timeout=...) and
both api_post(...) invocations to pass a timeout argument, and ensure you use
the same sensible timeout used elsewhere (see refresh_thread_titles.py). Also
broaden the error handling to catch SystemExit so the hook returns the skip
tuple on CLI exits—modify the final excepts to handle SystemExit (or catch
BaseException and re-raise KeyboardInterrupt if needed) while preserving the
existing returned skip messages that reference session_id and thread_id; keep
state_lock(), load_json(STATE_FILE), and save_json(STATE_FILE) behavior
unchanged.
In `@nowledge-mem-codex-plugin/RELEASING.md`:
- Around line 42-46: Update the smoke-test expectations in RELEASING.md (and the
duplicate checklist in README Verify) to match the actual hook log emitted by
nmem-stop-save.py: remove references to "hook: Stop", "hook: Stop Completed",
and a raw "nmem t import" result and instead check for the real log tokens such
as "start event=", "nmem_exit=0", and the create/append/skip outcome lines that
indicate import results; ensure the checklist items and example log snippets are
replaced accordingly so maintainers look for the actual strings produced by
nmem-stop-save.py.
In `@nowledge-mem-codex-plugin/scripts/install_hooks.py`:
- Around line 107-113: In merge_hooks_json, create the timestamped backup of
GLOBAL_HOOKS_FILE before calling load_json() (which may move/rename the original
file) rather than after; specifically, compute backup =
GLOBAL_HOOKS_FILE.with_suffix(".json.bak") and if GLOBAL_HOOKS_FILE.exists() and
not backup.exists() call shutil.copy2(GLOBAL_HOOKS_FILE, backup) first, then
call hooks_doc = load_json(GLOBAL_HOOKS_FILE) so shutil.copy2 won't raise
FileNotFoundError when load_json has already moved the file.
- Around line 128-135: The loop currently overwrites the entire
stop_hooks[index] when a matching hook command (hook.get("command") ==
str(INSTALLED_HOOK)) is found, which discards other sibling hooks and matcher
settings; instead, either treat the matching hook as already installed (skip
replacing) or replace only that specific hook dict in entry["hooks"] in place
while leaving other keys on entry intact—locate the matching hook inside
stop_hooks -> entry["hooks"], and assign the updated hook dict (from
desired["hooks"] or constructed equivalent) to entry["hooks"][hook_index] rather
than setting stop_hooks[index] = desired; ensure replaced is set appropriately.
In `@nowledge-mem-codex-plugin/scripts/refresh_thread_titles.py`:
- Around line 50-65: The build_thread_payload function currently assumes every
message dict has "role" and "content" keys; update the messages normalization in
build_thread_payload to match the Stop hook behavior by iterating messages,
skipping any entries without a role, and defaulting missing content to an empty
string before building the "messages" list; target the messages processing logic
inside build_thread_payload (the list comprehension) and replace it with
explicit normalization that uses m.get("role") and m.get("content", "") and only
includes messages with a truthy role.
- Around line 113-115: Before calling ensure_nmem_modules() / importing
nmem_cli, load the saved Mem client config so NMEM_API_URL and NMEM_API_KEY are
populated from ~/.nowledge-mem/config.json; update the startup sequence around
load_hook_module and ensure_nmem_modules to invoke the config loader (or inline
read-and-export of NMEM_API_URL/NMEM_API_KEY) prior to importing nmem_cli so
remote users who rely on saved client config can refresh titles without manually
exporting env vars.
In `@nowledge-mem-codex-plugin/tests/test_codex_plugin.py`:
- Around line 515-545: The test forces refresh_thread to raise but main
currently always returns 0; update main() to propagate a non-zero exit status
when any refresh_thread() calls fail (i.e., track failures during the
iter_codex_rollouts loop that uses
parse_codex_session_streaming/get_thread/refresh_thread and, if any exceptions
occur, set a failure flag and return a non-zero integer instead of 0), and
ensure printed error lines remain the same while the final return value reflects
whether any errors occurred.
---
Nitpick comments:
In `@nowledge-mem-codex-plugin/tests/test_codex_plugin.py`:
- Around line 467-471: The test
test_install_runtime_hook_rewrites_shebang_to_current_python currently only
checks the shebang; add an assertion that the installed hook file
(self.module.INSTALLED_HOOK) is executable after module.install_runtime_hook()
so a chmod regression is caught—use an OS-level executable check (e.g.,
os.access(path, os.X_OK) or checking stat.S_IXUSR/IXGRP/IXOTH on INSTALLED_HOOK)
to assert it is executable.
🪄 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: 01770bcd-dbb4-4bed-a102-96c66d320688
📒 Files selected for processing (14)
AGENTS.mdREADME.mddocs/PLUGIN_DEVELOPMENT_GUIDE.mdintegrations.jsonnowledge-mem-codex-plugin/CHANGELOG.mdnowledge-mem-codex-plugin/README.mdnowledge-mem-codex-plugin/RELEASING.mdnowledge-mem-codex-plugin/hooks/nmem-stop-save.pynowledge-mem-codex-plugin/scripts/install_hooks.pynowledge-mem-codex-plugin/scripts/refresh_thread_titles.pynowledge-mem-codex-plugin/scripts/validate-plugin.mjsnowledge-mem-codex-plugin/tests/test_codex_plugin.pynowledge-mem-codex-prompts/README.mdnowledge-mem-npx-skills/skills/check-integration/SKILL.md
✅ Files skipped from review due to trivial changes (3)
- nowledge-mem-npx-skills/skills/check-integration/SKILL.md
- docs/PLUGIN_DEVELOPMENT_GUIDE.md
- nowledge-mem-codex-plugin/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- nowledge-mem-codex-prompts/README.md
- README.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.
Summary
Stophook and enablescodex_hookstranscript_pathvianmem_cli.session_importWhy
Codex currently reads hooks from
~/.codex/hooks.json, not from assets nested inside the plugin directory. The existing Codex plugin only shipped skills, so it had no automatic session capture even though adjacent integrations in this repo already ship lifecycle hooks or hook validators.This change keeps the plugin honest and practical:
Stophook imports directly from Codex'stranscript_path, avoiding brittle session discovery by filename parsingValidation
node nowledge-mem-codex-plugin/scripts/validate-plugin.mjspython3 -m py_compile nowledge-mem-codex-plugin/hooks/nmem-stop-save.py nowledge-mem-codex-plugin/scripts/install_hooks.pyscripts/install_hooks.pyagainst a temporaryHOMEto verify:~/.codex/hooks.jsonis created/merged correctly~/.codex/hooks/nowledge-mem-stop-save.pyis installed and executablecodex_hooks = trueis added under[features]in~/.codex/config.tomlSummary by CodeRabbit
New Features
Documentation
Tests