fix: add mcp subcommand to CLI so mempalace mcp works for Claude Desktop (#355) - #372
fix: add mcp subcommand to CLI so mempalace mcp works for Claude Desktop (#355)#372RhettOP wants to merge 5 commits into
mcp subcommand to CLI so mempalace mcp works for Claude Desktop (#355)#372Conversation
PR Review: fix: add
|
| Aspect | Value |
|---|---|
| PR Goal | Wire existing mcp_server.main() into the CLI as mempalace mcp subcommand |
| Files Changed | 1 (mempalace/cli.py) |
| Risk Level | LOW — purely additive, no existing behaviour modified |
| Review Mode | Quick |
| Review Effort | 1 — small, focused bug fix |
| Recommendation | APPROVE |
Affected Areas: mempalace/cli.py → new cmd_mcp() → calls mempalace/mcp_server.py:main()
Business Impact: Running mempalace mcp (as documented in Claude Desktop setup) was broken — raised invalid choice: 'mcp'. This fix makes the documented command work.
Flow Changes: New CLI entry point mempalace mcp → lazy-imports mcp_server.main() → starts JSON-RPC stdio loop. No existing commands affected.
Ratings
| Aspect | Score |
|---|---|
| Correctness | 5/5 |
| Security | 5/5 |
| Performance | 5/5 |
| Maintainability | 5/5 |
PR Health
- Has clear description — explains the error, root cause, and fix
- References ticket/issue — Fixes pip package does not include mcp subcommand.. the Claude Desktop setup instructions fail :( #355
- Appropriate size — 14-line additive patch, well-scoped
- Follows established patterns — matches all other
cmd_*/dispatchconventions
Consistency Analysis
The new subcommand follows the exact pattern established by all other CLI commands:
| Check | Existing pattern (cmd_repair) |
PR (cmd_mcp) |
Match? |
|---|---|---|---|
def cmd_*(args): signature |
def cmd_repair(args): |
def cmd_mcp(args): |
✅ |
| Lazy import inside function body | import chromadb |
from .mcp_server import main as mcp_main |
✅ |
sub.add_parser(name, help=...) |
sub.add_parser("repair", help=...) |
sub.add_parser("mcp", help=...) |
✅ |
| Dispatch dict entry | "repair": cmd_repair |
"mcp": cmd_mcp |
✅ |
Integration Verification
mcp_server.main()confirmed: Exists atmempalace/mcp_server.py, takes zero arguments, runs astdin.readline()loop with JSON-RPC protocol handling. HandlesKeyboardInterrupt,json.JSONDecodeError, and genericExceptioninternally.- No argument mismatch:
cmd_mcp(args)callsmcp_main()with no arguments, matching thedef main():signature exactly. - Lazy import: MCP server dependencies are only loaded when
mempalace mcpis invoked, avoiding startup cost for other commands.
Issues
No HIGH or MEDIUM issues found.
Low Priority (Optional)
| # | Location | Confidence | Finding |
|---|---|---|---|
| 1 | cli.py:229 |
MED | args parameter is accepted but unused in cmd_mcp. Not a bug (dispatch pattern requires it), but if the project lints for unused params, consider # noqa: ARG001 or a future --verbose/--transport flag. |
Flow Impact Analysis
main() [cli.py]
└─ parser.parse_args() → args.command == "mcp"
└─ dispatch["mcp"] → cmd_mcp(args)
└─ from .mcp_server import main as mcp_main
└─ mcp_main() # blocks on stdin JSON-RPC loop
Blast radius: Zero. Purely additive — no existing commands, signatures, or dispatch paths are modified. cmd_mcp is a new leaf function with no callers other than the dispatch dict.
Created by Octocode MCP https://octocode.ai
|
🫡🙏🙏 |
web3guru888
left a comment
There was a problem hiding this comment.
Nice fix — this has been a real UX gap. The mempalace mcp command is documented in the Claude Desktop setup but wasn't actually wired in, so new users hit an immediate wall.
The implementation follows the existing CLI pattern correctly:
- Lazy import via
from .mcp_server import maininside the function ✅ - Subparser registration in the right place ✅
- Dispatch entry added ✅
A couple of minor observations:
- No
--portor--transportflags —mcp_server.main()is stdio-only today, but if a future PR adds SSE/streamable-HTTP transport, having the subparser ready for arguments would be nice. Not a blocker for this PR though. - Positional placement — the
mcpsubparser is added betweeninstructionsandrepair, which is fine, but grouping it near the other server-y commands (if any exist later) might read better in--helpoutput.
Overall this is a straightforward, correct fix. Tested verification in the PR description looks good.
🔭 Reviewed as part of the MemPalace-AGI integration project — autonomous research with perfect memory. Community interaction updates are posted regularly on the dashboard.
…gistration
The merge of fix/issue-355-add-mcp-subcommand onto develop introduced two
cmd_mcp() definitions and two sub.add_parser('mcp', ...) calls, causing:
argparse.ArgumentError: argument command: conflicting subparser: mcp
which broke all test_cli.py tests on CI.
Fix: keep the upstream cmd_mcp (setup guidance with --palace flag), remove
the simpler launcher-only version added in the original PR commit.
|
Fixed. The merge introduced two ...and broke all Fix: removed the duplicate simpler launcher-only |
Problem
Running
mempalace mcp(as instructed in the Claude Desktop setup docs) gives:The MCP server code exists at
mempalace/mcp_server.pyand has a workingmain()function, but themcpsubcommand was not wired intomempalace/cli.py.Fix
Added an
mcpsubcommand tomempalace/cli.pyfollowing the established pattern:cmd_mcp()function that callsmcp_server.main()p_mcpsubparser with descriptive help text"mcp": cmd_mcpentry to the dispatch dictVerification
Fixes #355