Skip to content

Conversation

niechen
Copy link
Contributor

@niechen niechen commented Sep 18, 2025

User description

Summary

  • lazily instantiate the install PromptSession to avoid NoConsoleScreenBufferError during import
  • fall back to plain input() when no interactive console is available
  • add logging so the fallback path is visible when running with --verbose

Testing

  • uv run python -m mcpm.cli run timer

PR Type

Bug fix


Description

  • Fix install prompt crashes on headless Windows environments

  • Add lazy PromptSession instantiation with fallback to basic input

  • Include debug logging for fallback scenarios

  • Handle EOFError exceptions in prompt handling


Diagram Walkthrough

flowchart LR
  A["PromptSession Creation"] --> B{"Console Available?"}
  B -->|Yes| C["Use Rich Prompt"]
  B -->|No| D["Log Debug Message"]
  D --> E["Use Basic input()"]
  C --> F["Handle User Input"]
  E --> F
Loading

File Walkthrough

Relevant files
Bug fix
install.py
Fix headless Windows prompt handling                                         

src/mcpm/commands/install.py

  • Add lazy PromptSession instantiation with error handling
  • Implement fallback to basic input() for headless environments
  • Add logging import and debug messages for fallback scenarios
  • Handle EOFError exceptions alongside KeyboardInterrupt
+47/-13 

Summary by CodeRabbit

  • Bug Fixes
    • Installer prompts are now more reliable across environments, including headless/CI and systems without an interactive console.
    • Automatically falls back to a basic input prompt when advanced console features aren’t available.
    • Prevents repeated initialization failures to reduce flakiness and unexpected crashes.
    • Gracefully handles cancellations and end-of-input (e.g., Ctrl+C/Ctrl+D), aborting cleanly.

Copy link

coderabbitai bot commented Sep 18, 2025

Walkthrough

Implements lazy, environment-aware PromptSession creation with error caching and a fallback to input() in headless contexts, adds a module logger, updates typings for prompt-related globals, broadens exception handling in prompt_with_default, and retains the core install flow while adjusting prompt acquisition and logging.

Changes

Cohort / File(s) Summary of Changes
Prompt handling & logging
src/mcpm/commands/install.py
Added logger; introduced _get_prompt_session factory with cached initialization error; changed prompt_session to Optional and added _prompt_session_error; prompt_with_default now uses lazy session or falls back to input(); expanded exception handling to include EOFError; minor docstring/style adjustments.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant InstallCommand as InstallCommand
  participant PromptFactory as _get_prompt_session
  participant PromptSession as PromptSession
  participant Stdin as input()

  User->>InstallCommand: Run install
  InstallCommand->>PromptFactory: Request PromptSession
  alt Console available
    PromptFactory-->>InstallCommand: PromptSession instance
    InstallCommand->>PromptSession: prompt_with_default(...)
    PromptSession-->>InstallCommand: user input
  else No console / init error
    PromptFactory-->>InstallCommand: None (error cached)
    InstallCommand->>Stdin: input(prompt)
    Stdin-->>InstallCommand: user input
  end
  InstallCommand-->>User: Continue install with value

  rect rgba(240,250,255,0.6)
  note over InstallCommand: KeyboardInterrupt/EOFError are caught and abort gracefully
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A rabbit taps the console keys, hop-hop—
If screens go dark, I won’t just stop.
I sniff a fallback, input’s scent,
Cache the bumps where errors went.
With logs to guide my burrowed trail,
The install hops on—without a fail. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title is concise and directly reflects the primary change described in the diff and PR objectives: it fixes the install prompt behavior for headless Windows environments by using lazy PromptSession initialization and an input() fallback, so it accurately summarizes the main change.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/windows-headless-install

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Logging Setup

A module-level logger is used, but no guarantee that logging is configured elsewhere; ensure verbose flag actually enables the debug message so the fallback path is visible as intended.

logger = logging.getLogger(__name__)
repo_manager = RepositoryManager()
profile_config_manager = ProfileConfigManager()
global_config_manager = GlobalConfigManager()

# Create a prompt session with custom styling (lazy to support headless Windows runs)
prompt_session: Optional[PromptSession] = None
_prompt_session_error: Optional[Exception] = None


def _get_prompt_session() -> Optional[PromptSession]:
    """Lazily create a PromptSession, tolerating environments without a console."""

    global prompt_session, _prompt_session_error  # noqa: PLW0603 - shared module state

    if prompt_session is not None:
        return prompt_session

    if _prompt_session_error is not None:
        return None

    try:
        prompt_session = PromptSession()
    except Exception as exc:  # prompt_toolkit raises NoConsoleScreenBufferError on Windows
        _prompt_session_error = exc
        logger.debug("Falling back to basic input prompts: %s", exc)
        prompt_session = None

    return prompt_session
Fallback UX

The plain input() fallback shows defaults like "[default]"; confirm this aligns with CLI UX and that hidden input is handled appropriately when hide_input=True in non-interactive environments.

try:
    if session is not None:
        result = session.prompt(
            message=HTML(f"<prompt>{prompt_text}</prompt> > "),
            style=style,
            default=default,
            is_password=hide_input,
            key_bindings=kb,
        )
    else:
        # Basic fallback for environments without an interactive console (e.g., headless Windows)
        prompt_parts = [prompt_text]
        if default:
            prompt_parts.append(f"[{default}]")
        prompt_display = " ".join(prompt_parts) + " > "
        result = input(prompt_display)  # noqa: PLW1513 - suppressed by KeyboardInterrupt handling
Recursive Prompt

On required empty input, function recurses; consider iterative loop to avoid deep recursion in persistent invalid input scenarios.

    return default

# Empty result for required field without default is not allowed
if not result.strip() and required and not default:
    console.print("[yellow]Warning: Required value cannot be empty.[/]")
    return prompt_with_default(prompt_text, default, hide_input, required)

return result

Copy link
Contributor

qodo-merge-pro bot commented Sep 18, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Catch specific exceptions instead of Exception

In _get_prompt_session, replace the broad except Exception with specific
exceptions (OSError and NoConsoleScreenBufferError) to avoid masking unexpected
errors in headless environments.

src/mcpm/commands/install.py [37-55]

 def _get_prompt_session() -> Optional[PromptSession]:
     """Lazily create a PromptSession, tolerating environments without a console."""
 
     global prompt_session, _prompt_session_error  # noqa: PLW0603 - shared module state
 
     if prompt_session is not None:
         return prompt_session
 
     if _prompt_session_error is not None:
         return None
 
+    # Define a tuple of expected exceptions for headless environments.
+    expected_exceptions = [OSError]
+    try:
+        # NoConsoleScreenBufferError is Windows-specific.
+        from prompt_toolkit.output.win32 import NoConsoleScreenBufferError
+        expected_exceptions.append(NoConsoleScreenBufferError)
+    except ImportError:
+        pass  # Not on Windows.
+
     try:
         prompt_session = PromptSession()
-    except Exception as exc:  # prompt_toolkit raises NoConsoleScreenBufferError on Windows
+    except tuple(expected_exceptions) as exc:
         _prompt_session_error = exc
         logger.debug("Falling back to basic input prompts: %s", exc)
         prompt_session = None
 
     return prompt_session
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that catching a generic Exception is a bad practice as it can hide unrelated bugs, and proposes a more robust implementation by catching only specific, expected exceptions.

Medium
High-level
Encapsulate prompt session state management

Replace the module-level global variables prompt_session and
_prompt_session_error with a dedicated class to encapsulate the state and logic
for lazy initialization of the prompt session. This change would improve
modularity and testability.

Examples:

src/mcpm/commands/install.py [33-55]
prompt_session: Optional[PromptSession] = None
_prompt_session_error: Optional[Exception] = None


def _get_prompt_session() -> Optional[PromptSession]:
    """Lazily create a PromptSession, tolerating environments without a console."""

    global prompt_session, _prompt_session_error  # noqa: PLW0603 - shared module state

    if prompt_session is not None:

 ... (clipped 13 lines)

Solution Walkthrough:

Before:

prompt_session: Optional[PromptSession] = None
_prompt_session_error: Optional[Exception] = None

def _get_prompt_session() -> Optional[PromptSession]:
    global prompt_session, _prompt_session_error
    if prompt_session is not None:
        return prompt_session
    if _prompt_session_error is not None:
        return None
    try:
        prompt_session = PromptSession()
    except Exception as exc:
        _prompt_session_error = exc
        prompt_session = None
    return prompt_session

def prompt_with_default(...):
    session = _get_prompt_session()
    # ...

After:

class PromptSessionFactory:
    _session: Optional[PromptSession] = None
    _error: Optional[Exception] = None

    def get_session(self) -> Optional[PromptSession]:
        if self._session or self._error:
            return self._session
        try:
            self._session = PromptSession()
        except Exception as exc:
            self._error = exc
        return self._session

prompt_factory = PromptSessionFactory()

def prompt_with_default(...):
    session = prompt_factory.get_session()
    # ...
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies the use of module-level global state and proposes a valid design improvement by encapsulating it, which would enhance modularity and testability.

Low
Organization
best practice
Send prompts to stderr

When prompting via input(), write the prompt to stderr to avoid mixing with data
output on stdout.

src/mcpm/commands/install.py [127-132]

 # Basic fallback for environments without an interactive console (e.g., headless Windows)
 prompt_parts = [prompt_text]
 if default:
     prompt_parts.append(f"[{default}]")
 prompt_display = " ".join(prompt_parts) + " > "
-result = input(prompt_display)  # noqa: PLW1513 - suppressed by KeyboardInterrupt handling
+print(prompt_display, end="", file=sys.stderr, flush=True)
+result = input()
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Prefer reading subprocess stderr for logs/status instead of stdout.

Low
Add Click help option and examples

Add a Click help option decorator and a descriptive docstring with an example
block to align with the CLI help pattern.

src/mcpm/commands/install.py [152]

 @click.command()
+@click.help_option("-h", "--help")
+def install(...):
+    """Install command for adding MCP servers to the global configuration.
+    
+    Example:
+    
+    \b
+        mcpm install <server>
+    """
+    ...

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Provide consistent Click help options with both -h and --help and include structured help text with examples.

Low
  • Update

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/mcpm/commands/install.py (2)

32-35: Module-level state: tiny typing nit

Optional: widen _prompt_session_error to Optional[BaseException] to future‑proof if you ever decide to cache non-Exception errors.


127-133: Harden fallback input: hide secrets and strip HTML tags in plain mode

When no console is available, input() will echo secrets and the HTML tags show literally. Use getpass for hidden input and strip tags for readability.

Apply this diff:

-            # Basic fallback for environments without an interactive console (e.g., headless Windows)
-            prompt_parts = [prompt_text]
+            # Basic fallback for environments without an interactive console (e.g., headless Windows)
+            # Strip prompt_toolkit HTML tags for plain input
+            plain_text = re.sub(r"<[^>]+>", "", prompt_text)
+            prompt_parts = [plain_text]
             if default:
                 prompt_parts.append(f"[{default}]")
             prompt_display = " ".join(prompt_parts) + " > "
-            result = input(prompt_display)  # noqa: PLW1513 - suppressed by KeyboardInterrupt handling
+            if hide_input:
+                import getpass  # local import to avoid global dependency when unused
+                result = getpass.getpass(prompt_display)  # noqa: PLW1513
+            else:
+                result = input(prompt_display)  # noqa: PLW1513
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf2dc92 and fcd6d87.

📒 Files selected for processing (1)
  • src/mcpm/commands/install.py (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Always format Python code with ruff.

Files:

  • src/mcpm/commands/install.py
🧬 Code graph analysis (1)
src/mcpm/commands/install.py (3)
src/mcpm/utils/repository.py (1)
  • RepositoryManager (25-163)
src/mcpm/profile/profile_config.py (1)
  • ProfileConfigManager (13-207)
src/mcpm/global_config.py (1)
  • GlobalConfigManager (23-381)
🔇 Additional comments (4)
src/mcpm/commands/install.py (4)

1-1: LGTM: docstring and logging setup

Docstring reads well; logger initialization is appropriate.

Also applies to: 4-4, 27-27


115-126: Session gating in prompt_with_default: good

Using the lazy getter and preserving styling when a session exists is correct.


148-149: EOFError handling: nice catch

Catching EOFError avoids crashes in truly non-interactive contexts.


37-56: Approve: lazy PromptSession + error caching; verify other interactive prompts

Lazy PromptSession init and error caching are good and approved. Found other interactive prompt callsites that can still fail in headless/non‑TTY runs — ensure each has a safe fallback or is guarded.

  • src/mcpm/commands/install.py — Confirm.ask(...) and click.prompt(...) (installation confirmation & method selection).
  • src/mcpm/commands/uninstall.py — Confirm.ask("Proceed with removal?").
  • src/mcpm/commands/profile/remove.py — Confirm.ask(...).
  • src/mcpm/commands/inspect.py — click.confirm("Launch raw MCP Inspector").
  • src/mcpm/commands/edit.py — multiple inquirer.* prompts (text/select/confirm).
  • src/mcpm/commands/client.py — many inquirer.* calls (checkbox/confirm/text); some helpers already catch OSError (errno 22) — ensure consistent handling.
  • src/mcpm/commands/profile/interactive.py — inquirer.* prompts.
  • tests/test_add.py — tests patch PromptSession.prompt; verify tests still valid if prompt behavior changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant