fix(tools): Prevent crash in non-interactive environments

[diego-coder]

Fixes #3163

Title

fix(tools): Harden MCPServerAdapter initialization and fix tests

Description

Summary

This pull request resolves an issue where the MCPServerAdapter would crash in non-interactive environments. It hardens the adapter's initialization logic, adds explicit test coverage for missing dependencies, and fixes existing bugs in the test suite itself.

Changes Made

1. Hardened MCPServerAdapter:

   • Replaced the interactive click.confirm() prompt with a non-interactive ImportError to prevent crashes.
   • Added critical level logging for missing dependencies.
   • Improved the stop() method to be fully idempotent and to ensure all resources are released.

2. Fixed the Test Suite:

   • Resolved a ModuleNotFoundError in the test suite's subprocesses by changing the test command from uv run to python for test subprocesses to ensure tests use the same environment as pytest. This prevents ModuleNotFoundError for optional dependencies like mcp and guarantees test reliability across all developer setups and CI.

3. Added New Test Coverage:

   • A new unit test now confirms that an ImportError is correctly raised if the mcp package is missing.

Motivation

• Prevents fatal crashes in production environments (e.g., FastAPI, Docker).
• Strengthens the reliability and maintainability of the adapter.
• Improves the stability of the project's own test suite.

Checklist

• Code is formatted (ruff format .)
• Lint checks pass (ruff check .)
• All local tests pass (pytest)

Impact

• No breaking changes; improves error handling and test robustness only.

How to Test

pytest tests/adapters/mcp_adapter_test.py

[lucasgomide]

@diego-coder Welcome and thank you for your collaboration!

What do you think about properly handling the exceptions raised by click or subprocess instead?
Removing this could negatively impact the developer experience, as it would drop a "magical" feature that automatically installs missing packages.

[diego-coder]

@diego-coder Welcome and thank you for your collaboration!

What do you think about properly handling the exceptions raised by click or subprocess instead? Removing this could negatively impact the developer experience, as it would drop a "magical" feature that automatically installs missing packages.

Honestly my pleasure, big fan. I think you raise a really good point, and I agree with it because I like the install. So I tried to find a work around, copy/pasted the issue, PR, and code change to ChatGPT and Gemini, and when I ran it by them they both about lost their minds over it. The issue for them both is that they claim this is really about production and production use for autonomous agents.

They both claim the majority of users will use this in a non-interactive environments (docker containers, fastAPI servers, ci/cd pipelines) after setting up it up (dev, prototyping, debugging) in their consoles, so that the UX of creating truly autonomous agents that perform without human supervision (backend APIs, data pipelines, scheduled tasks, chatbot integrations) trumps the ux of package install.

ChatGPT used charged words in its description, saying that magic installs break silently (security, dependency conflicts, CI/CD instability) and that "in non-interactive environments, this is not just bad—it’s catastrophic." Ok ChatGPT - calm down. It does raise a point that in mature projects silent installs are discouraged (hence pip, poetry, uv) so that users are aware of what's installed.

I think if it were me and for my own use I prefer the UX with ease of use. But I really think that this code, this entire app is awesome and really is production grade already and is being used by production professionals who already know how to 'uv pip install -e' in a flash to be ready to go. The ml ops folks all want stability in production, they absolutely do not care if it comes at the cost of installing a few packages.

What you are running into is a good problem to have, IMO. It really is an excellent problem to have. This app is getting into the hands of people who are using it in a production way, as it should be used in production, for production use case - and that's broad. So PR solves all issues across all those broad production environments (also hardens stop() to prevent resource leaks in unattended environments, fixes a few other existing tests).

I would say it should be merged as is. It's a very serious bug preventing it from being used by the people you want using it, and it probably should be merged quickly.

BUT. After the fix is in, my thinking is - as I said above - I like silent installs... so why can't we have both? Can there be a solution introduced where we can have it both ways? A few caveats: 1) what I'm proposing is not a prominent feature in production apps (don't think I've seen it?) 2) this was not suggested by ChatGPT or Gemini mind you 3) obviously this introduces something less brittle, but it may still fail 4) some devs are fickle and are easily annoyed, could be annoyed by this feature. But! I came up with my own idea, and it's at least plausible, so...:

10 second prompt, if user fails to input 'y' to proceed it flips to non-interactive. I think something like that could work, but it would have to work across all platforms and environments natively without external dependencies which makes it more challenging. Could look like:

import sys
import threading

def timed_input(prompt, timeout=10):
    """Prompt the user for input, but return None if they don't respond in time."""
    result = [None]
    def ask():
        try:
            result[0] = input(prompt)
        except Exception:
            result[0] = None

    thread = threading.Thread(target=ask)
    thread.start()
    thread.join(timeout)
    if thread.is_alive():
        print(f"\nNo input detected after {timeout} seconds. Defaulting to 'no'.")
        return None
    return result[0]

# Example usage:
answer = timed_input("Would you like to install the missing package? [y/N]: ", timeout=10)
if answer is not None and answer.lower() == 'y':
    print("Proceeding with install...")
else:
    print("Not installing. Exiting or raising ImportError.")

The good: it checks for tty, it's not a static prompt, has a timeout mechanism, it uses python threading to create the prompt and uses a separate thread to prevent the main program from freezing, creates the input in a separate thread, no dependencies, cross platform, robust.

The bad: complexity, it's more complex than raising an import error, input is dangling (eg if someone enters something after timeout, that input could be used in the next input), someone will find a reason to complain about it ^_^, it will inevitably cause issues somewhere.

I probably wouldn't implement this on a major repo where my job is putting out a lot of fires in said repo, but I would love it for myself and smaller groups of users. This was just the first idea I thought of, I was trying to think of some ways you could expand your options to show that there might be alternatives. So yeah there might be other, more stable features that do the same thing as this? Maybe, but I would start with the critical bug fix and work from there while you think about it, imo.