PR #410 Review: fix: replace python3 -m mempalace with CLI entry point (PEP 668)

URL: #410
Author: armujahid | Status: Draft | Branch: fix/pep668-cli-entrypoint → main
Scope: 14 files | +52 / -24

Executive Summary

This PR adds a mempalace mcp-serve CLI subcommand and migrates all python3 -m mempalace invocations to the mempalace entry point across plugin configs, hook scripts, and documentation. It also updates install instructions to recommend uv tool install / pipx install for PEP 668 compliance. The change is well-scoped and mostly mechanical, but contains one medium-severity bug and one fragile implementation pattern that should be addressed before merge.

Recommendation: Request changes (2 issues to fix)

Ratings

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 — works
• mempalace 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-serve

Fix (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_server is 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 --palace is silently lost
• It mutates global sys.argv state, 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 mempalace still works via __main__.py which imports cli.main()
• Entry point is correct: pyproject.toml has mempalace = "mempalace:main" which resolves through __init__.py to cli.main()
• Consistent migration: All 6 plugin/hook files are updated from python3 -m mempalace to mempalace entry point
• PEP 668 guidance: instructions/init.md now correctly recommends uv tool install / pipx install and explains the externally-managed-environment error
• Test follows existing patterns: test_main_mcp_serve_dispatches mirrors the dispatch test pattern used by other subcommands

Minor Observations (Non-blocking)

• The test only validates dispatch routing (patches cmd_mcp_serve and checks it's called). A test for the actual --palace forwarding 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.sh and hooks/mempal_save_hook.sh now call mempalace mine instead of python3 -m mempalace mine — this requires mempalace to be on PATH, which is guaranteed by uv tool install / pipx install but not by pip install -e . in a venv unless the venv is activated.

Flow Impact

Blast radius is low — this is additive (new subcommand) with string replacements in configs/hooks.

@bgauryy Thanks for the thorough review! All issues have been addressed:

Fixed:

1. --palace position bug (Medium) — Added --palace to the mcp-serve subparser with default=argparse.SUPPRESS so both forms work: mempalace --palace /path mcp-serve and mempalace mcp-serve --palace /path. Tests cover both argument orders (test_main_mcp_serve_palace_after_subcommand, test_main_mcp_serve_palace_before_subcommand).

2. ~ expansion for non-shell launchers (Medium) — cmd_mcp_serve now calls os.path.expanduser() before forwarding --palace, so tilde paths from MCP JSON configs resolve correctly. Covered by test_cmd_mcp_serve_expands_tilde.

3. Install detection misses uv/pipx (Low) — instructions/init.md now uses mempalace status instead of pip show mempalace to detect existing installs.

Not changed (by design):

• sys.argv rewrite pattern in cmd_mcp_serve — acknowledged as fragile but low risk since mcp_server is never pre-imported in any code path. The env var alternative has its own downsides (leaks to child processes).

Current head: 928f610. Would appreciate a re-review when you get a chance.

Hi, thanks for the contribution.

This PR has merge conflicts with develop, and the branch has not been updated in over 7 days, which puts it before our most recent release. The conflicts are likely against work that landed in that release.

Could you rebase onto develop so we can take another look?

If this change is no longer relevant, feel free to close the PR.

(This message is part of a periodic backlog pass, sent to all open PRs that match this state.)