fix: replace python3 -m mempalace with CLI entry point (PEP 668)#410
fix: replace python3 -m mempalace with CLI entry point (PEP 668)#410armujahid wants to merge 6 commits intoMemPalace:developfrom
Conversation
f14f4ba to
258fc91
Compare
The plugin assumed python3 -m mempalace works, which breaks on modern Linux (PEP 668), uv tool install, and pipx installs where the CLI binary is on PATH but the module isn't importable by system python3. - Add `mempalace mcp-serve` CLI subcommand to replace `python3 -m mempalace.mcp_server` - Update MCP server configs in both .claude-plugin and .codex-plugin - Update all hook scripts to use `mempalace` CLI directly - Update docs to recommend uv/pipx install methods - Backward compat preserved: python -m mempalace still works Closes MemPalace#408
258fc91 to
fa28be6
Compare
PR #410 Review:
|
| Dimension | Score | Notes |
|---|---|---|
| Correctness | 3/5 | --palace passthrough via docstring is wrong for argparse subparsers |
| Security | 5/5 | No new attack surface |
| Performance | 5/5 | No performance implications |
| Maintainability | 3/5 | sys.argv manipulation pattern is fragile |
Issues
1. [MEDIUM / HIGH confidence] Documentation bug: --palace position incorrect for argparse subparsers
Location: mempalace/mcp_server.py:5
The updated docstring says:
Install: claude mcp add mempalace -- mempalace mcp-serve [--palace /path/to/palace]
With the current argparse setup, --palace is defined on the parent parser (cli.py:~372), not on the mcp-serve subparser. Argparse requires parent-level optional arguments to appear before the subcommand:
mempalace --palace /path mcp-serve— worksmempalace mcp-serve --palace /path— raises "unrecognized arguments"
Any user following the docstring instruction would get a CLI error.
Fix (option A — fix the docs):
-Install: claude mcp add mempalace -- mempalace mcp-serve [--palace /path/to/palace]
+Install: claude mcp add mempalace -- mempalace --palace /path/to/palace mcp-serveFix (option B — better UX, add --palace to the subparser):
p_mcp_serve = sub.add_parser(
"mcp-serve",
help="Start the MCP server (JSON-RPC over stdin/stdout)",
)
p_mcp_serve.add_argument("--palace", default=None, help="Path to palace directory")Option B is preferred since users naturally expect mempalace mcp-serve --palace /path.
2. [MEDIUM / HIGH confidence] Fragile sys.argv manipulation pattern in cmd_mcp_serve
Location: mempalace/cli.py:236-240 (on PR branch)
def cmd_mcp_serve(args):
"""Start the MCP server (JSON-RPC over stdin/stdout)."""
sys.argv = ["mempalace-mcp-server"] + (["--palace", args.palace] if args.palace else [])
from .mcp_server import main as mcp_main
mcp_main()This works by overwriting sys.argv before the import, relying on mcp_server.py's module-level _parse_args() executing at import time to pick up the modified sys.argv.
This is fragile because:
- If
mcp_serveris already imported (e.g., by a test harness, a pre-import, or a future refactor), the module-level_parse_args()won't re-run, and--palaceis silently lost - It mutates global
sys.argvstate, which is a side-effect hazard
Fix — set the env var directly (same mechanism mcp_server._parse_args uses internally):
def cmd_mcp_serve(args):
"""Start the MCP server (JSON-RPC over stdin/stdout)."""
if args.palace:
os.environ["MEMPALACE_PALACE_PATH"] = os.path.abspath(args.palace)
from .mcp_server import main as mcp_main
mcp_main()This is idempotent, doesn't rely on import ordering, and uses the same MEMPALACE_PALACE_PATH env var that mcp_server.py already respects.
What Looks Good
- Backward compat preserved:
python -m mempalacestill works via__main__.pywhich importscli.main() - Entry point is correct:
pyproject.tomlhasmempalace = "mempalace:main"which resolves through__init__.pytocli.main() - Consistent migration: All 6 plugin/hook files are updated from
python3 -m mempalacetomempalaceentry point - PEP 668 guidance:
instructions/init.mdnow correctly recommendsuv tool install/pipx installand explains theexternally-managed-environmenterror - Test follows existing patterns:
test_main_mcp_serve_dispatchesmirrors the dispatch test pattern used by other subcommands
Minor Observations (Non-blocking)
- The test only validates dispatch routing (patches
cmd_mcp_serveand checks it's called). A test for the actual--palaceforwarding behavior would catch issue Integrating MemPalace with SoulForge's code intelligence system #2 above, but this is consistent with how all other commands are tested. hooks/mempal_precompact_hook.shandhooks/mempal_save_hook.shnow callmempalace mineinstead ofpython3 -m mempalace mine— this requiresmempalaceto be onPATH, which is guaranteed byuv tool install/pipx installbut not bypip install -e .in a venv unless the venv is activated.
Flow Impact
| Changed Symbol | Type | Callers Affected |
|---|---|---|
cmd_mcp_serve (new) |
New function | Called only from main() dispatch table |
mcp-serve subparser (new) |
New CLI command | User-facing; no internal callers |
| Hook scripts (modified) | Shell scripts | Called by Claude Code / Codex harnesses |
Blast radius is low — this is additive (new subcommand) with string replacements in configs/hooks.
Use argparse.SUPPRESS as default for the mcp-serve --palace argument
so it doesn't overwrite the parent parser's value when omitted.
Both forms now work: mempalace --palace /path mcp-serve
mempalace mcp-serve --palace /path
- Apply os.path.expanduser() to --palace in cmd_mcp_serve so tilde paths from MCP JSON configs resolve correctly - Use mempalace status instead of pip show to detect existing installs (pip show misses uv/pipx installs)
- Add test_cmd_mcp_serve_expands_tilde to verify ~ is expanded before forwarding --palace to the MCP server - Fix init.md wording: say "report that mempalace is installed" instead of "report the version" since there is no --version flag
|
@bgauryy Thanks for the thorough review! All issues have been addressed: Fixed:
Not changed (by design):
Current head: |
…hon -m The mcp guidance command was still telling users to use python -m mempalace.mcp_server. Update to use the new CLI entry point.
web3guru888
left a comment
There was a problem hiding this comment.
This overlaps significantly with #340 (messelink) — both replace python3 -m mempalace.mcp_server for pipx/uv compatibility. Worth noting the differences:
| #340 (messelink) | #410 (this PR) | |
|---|---|---|
| MCP entry | mempalace-mcp (separate entry point) |
mempalace mcp-serve (subcommand) |
| Hook scripts | Use mempalace CLI |
Use mempalace CLI |
| Docs | Updated | Updated + recommends uv/pipx |
| Tests | None added | 4 new CLI tests |
| pyproject.toml | Adds entry point | No entry point added |
This PR is more thorough (tests, --palace forwarding both before/after subcommand, tilde expansion for non-shell launchers). The subcommand approach has the advantage of keeping everything under one binary.
The cmd_mcp_serve implementation is well thought out — especially the sys.argv rewriting to pass --palace through to mcp_server.main(). The tilde expansion test caught a real edge case for MCP JSON configs where the shell doesn't expand ~.
Since these two PRs conflict, the maintainers should pick one approach. Both work; this one has better test coverage.
🔭 Reviewed as part of the MemPalace-AGI integration project — autonomous research with perfect memory. Community interaction updates are posted regularly on the dashboard.
Summary
mempalace mcp-serveCLI subcommand to replacepython3 -m mempalace.mcp_servermcp-serveaccepts--palaceboth before and after the subcommand (mempalace --palace /path mcp-serveandmempalace mcp-serve --palace /path)python3 -m mempalaceinvocations with themempalaceCLI entry point across both.claude-plugin/and.codex-plugin/(MCP configs, hook scripts, standalone hooks)uv tool install/pipx installas primary install methodspython -m mempalacestill works via__main__.pyCloses #408
Test plan
uv run pytest tests/test_cli.py— all 44 tests pass including newtest_main_mcp_serve_dispatchesandtest_main_mcp_serve_palace_after_subcommandmempalace mcp-servestarts the MCP server (reads JSON-RPC from stdin)mempalace --palace /tmp/test mcp-serveandmempalace mcp-serve --palace /tmp/testboth pass--palacethrough correctlypython3 -m mempalacein plugin/hook filesuv tool install mempalace