fix: remove auto v1 migration prompt - #265
Conversation
WalkthroughRemoved interactive v1 migration from the CLI, hardened cross-platform single-key input in the v1 migrator, expanded run command help and execution-mode options, deleted the AI-agent CLI design doc, and updated a test to call CLI help directly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as mcpm CLI
participant Help as Help Renderer
participant Migrator as v1_migrator
User->>CLI: run `mcpm` or `mcpm --help`
activate CLI
Note over CLI: v1 migration detection/prompt removed
CLI->>Help: print header
CLI->>Help: print help text (global footer suppressed)
Help-->>CLI: composed help output
CLI-->>User: display help
deactivate CLI
Note right of Migrator: `_wait_for_keypress` now:\n- tries Unix `termios`/`tty` raw read\n- tries Windows `msvcrt.getch()`\n- falls back to `input()` for non-tty/errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (1)
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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_global_config.py (1)
43-50: Stabilize help assertions (case-insensitive) to reduce brittleness.Lowercase once and assert against that to avoid future styling/casing flakes.
- assert result.exit_code == 0 - assert "global configuration" in result.output.lower() - assert "profile" in result.output.lower() - assert "install" in result.output - assert "run" in result.output + assert result.exit_code == 0 + out = result.output.lower() + assert "global configuration" in out + assert "profile" in out + assert "install" in out + assert "run" in outAdditionally, consider adding a separate test invoking
mainwith no args ([]) to assert header shows once and the global footer isn’t duplicated. I can draft it if helpful.src/mcpm/migration/v1_migrator.py (2)
46-67: Don’t block in non‑interactive contexts; flush prompt first.Without a TTY (CI, piped input), this can hang waiting for input. Flush the prompt and short‑circuit when stdin isn’t a TTY.
- console.print(message, end="") + console.print(message, end="") + # Ensure prompt is visible before blocking + try: + console.file.flush() + except Exception: + pass + # In non-interactive contexts (e.g., CI/pipes), don't wait + try: + is_tty = sys.stdin.isatty() + except Exception: + is_tty = False + if not is_tty: + console.print() + return
48-66: Leverage Click’s cross‑platform pause to simplify.You can replace the platform branches with
click.pause()which already handles Windows/Unix, echo, and TTY checks.Example replacement inside this method:
import click # at top of file def _wait_for_keypress(self, message: str): console.print(message, end="") try: console.file.flush() except Exception: pass try: click.pause("") # empty string to avoid duplicate message except (EOFError, KeyboardInterrupt): pass console.print()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mcpm/cli.py(0 hunks)src/mcpm/migration/v1_migrator.py(1 hunks)tests/test_global_config.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/mcpm/cli.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Always format Python code with
ruff.
Files:
tests/test_global_config.pysrc/mcpm/migration/v1_migrator.py
🧬 Code graph analysis (1)
tests/test_global_config.py (1)
src/mcpm/cli.py (1)
main(80-103)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcpm/commands/run.py (1)
66-71: Port auto-discovery ignores --host; can mis-detect availabilityBinding check is hardcoded to 127.0.0.1. If users pass --host 0.0.0.0 (or another IP/IPv6), a port may be reported “free” yet fail on uvicorn bind (or vice versa). Bind against the requested host (and support IPv6) to avoid false positives/negatives.
Apply this diff:
- actual_port = await find_available_port(port) + actual_port = await find_available_port(port, host)-async def find_available_port(preferred_port, max_attempts=10): - """Find an available port starting from preferred_port.""" - import socket +async def find_available_port(preferred_port, host, max_attempts=10): + """Find an available port for the given host starting from preferred_port.""" + import socket, ipaddress for attempt in range(max_attempts): port_to_try = preferred_port + attempt - # Check if port is available + # Check if port is available for the specified host (IPv4/IPv6 aware) try: - with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - s.bind(("127.0.0.1", port_to_try)) + try: + is_ipv6 = ipaddress.ip_address(host).version == 6 + except ValueError: + is_ipv6 = ":" in host # simple fallback for hostnames with IPv6 literals + family = socket.AF_INET6 if is_ipv6 else socket.AF_INET + bind_host = host + with socket.socket(family, socket.SOCK_STREAM) as s: + # For detection we only need to bind; the context manager closes immediately + s.bind((bind_host, port_to_try)) return port_to_try except OSError: continue # Port is busy, try next oneAlso applies to: 113-129
🧹 Nitpick comments (2)
src/mcpm/utils/rich_click_config.py (1)
167-169: Run option group updated with --sse/--host — alignment looks goodThe option group now mirrors the run command’s flags.
Optional: rename the group to “Execution & Network” (or “Serving Options”) since it now includes network params, not just “mode”.
- { - "name": "Execution Mode", + { + "name": "Execution & Network", "options": ["--http", "--sse", "--port", "--host"], },src/mcpm/commands/run.py (1)
73-82: Friendlier URL when binding to all interfacesWhen host is 0.0.0.0 or ::, the printed URL isn’t directly reachable. Show localhost in the hint to reduce confusion.
- if sse_mode: - server_url = f"http://{host}:{actual_port}/sse/" + display_host = "localhost" if host in {"0.0.0.0", "::"} else host + if sse_mode: + server_url = f"http://{display_host}:{actual_port}/sse/" title = "📡 SSE Server Running" else: - server_url = f"http://{host}:{actual_port}/mcp/" + server_url = f"http://{display_host}:{actual_port}/mcp/" title = "🌐 Local Server Running"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
AI_AGENT_FRIENDLY_CLI_PLAN.md(0 hunks)src/mcpm/commands/run.py(1 hunks)src/mcpm/utils/rich_click_config.py(1 hunks)
💤 Files with no reviewable changes (1)
- AI_AGENT_FRIENDLY_CLI_PLAN.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Always format Python code with
ruff.
Files:
src/mcpm/utils/rich_click_config.pysrc/mcpm/commands/run.py
🧬 Code graph analysis (1)
src/mcpm/commands/run.py (1)
src/mcpm/commands/profile/run.py (1)
run(141-207)
🔇 Additional comments (2)
src/mcpm/commands/run.py (2)
145-177: Reworked usage doc is clear and actionableExamples and tips read well; defaults and exclusivity are obvious.
134-143: New flags (--sse, --host) and clearer help — LGTMOptions --http, --sse, --port, --host and -h are present in src/mcpm/commands/run.py and the mutual-exclusivity/help text is explicit.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mcpm/migration/v1_migrator.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Always format Python code with
ruff.
Files:
src/mcpm/migration/v1_migrator.py
niechen
changed the title
## [2.8.2](v2.8.1...v2.8.2) (2025-09-18) ### Bug Fixes * remove auto v1 migration prompt ([#265](#265)) ([8630fc8](8630fc8))
|
🎉 This PR is included in version 2.8.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
PR Type
Enhancement
Description
Remove automatic v1 migration prompt from CLI startup
Improve cross-platform keypress handling in migration utility
Simplify test setup by removing migration mocks
Diagram Walkthrough
File Walkthrough
cli.py
Remove automatic migration prompt logicsrc/mcpm/cli.py
v1_migrator.py
Enhance cross-platform keypress handlingsrc/mcpm/migration/v1_migrator.py
msvcrt.getch()fallbacktest_global_config.py
Simplify tests by removing migration mockstests/test_global_config.py
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests