feat: wire MCP server into CLI as evalhub mcp subcommand#99
Open
tarilabs wants to merge 2 commits intoeval-hub:mainfrom
Open
feat: wire MCP server into CLI as evalhub mcp subcommand#99tarilabs wants to merge 2 commits intoeval-hub:mainfrom
evalhub mcp subcommand#99tarilabs wants to merge 2 commits intoeval-hub:mainfrom
Conversation
- Add `evalhub mcp` subcommand reusing CLI profile/config for base-url, token, tenant - Remove standalone `evalhub-mcp` entry point and `src/evalhub/mcp/__main__.py` - MCP extra now depends on CLI extra (`eval-hub-sdk[cli]`) - Lazy-import `mcp` package; show install hint when missing - Add tests for subcommand registration, config resolution, and CLI flag overrides Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: tarilabs <matteo.mortari@gmail.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughConsolidates the EvalHub MCP server entry from a standalone Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as evalhub CLI
participant Config as Config/Profile
participant MCP as mcp package
participant Client as AsyncEvalHubClient
participant Server as MCP Server
User->>CLI: evalhub mcp [--base-url] [--token] [--tenant]
CLI->>Config: Load active profile (EVALHUB_CONFIG/EVALHUB_TENANT)
Config-->>CLI: Profile values (base_url, token, tenant, insecure, timeout)
CLI->>CLI: Resolve final params (CLI flags override profile)
CLI->>MCP: import mcp
alt mcp missing
MCP-->>CLI: ImportError
CLI-->>User: ClickException with pip install guidance
else mcp present
MCP-->>CLI: mcp loaded
CLI->>Client: AsyncEvalHubClient(base_url, token, tenant, insecure, timeout)
Client-->>CLI: client instance
CLI->>Server: set_client(client)
CLI->>Server: asyncio.run(mcp_server.run_stdio_async())
Server-->>User: MCP server running on stdio
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/test_cli_mcp.py (1)
16-22: Environment variable cleanup is not isolated.The fixture directly modifies
os.environ, which could cause issues if tests are run in parallel or if an exception occurs before cleanup. Consider usingCliRunner(env=...)ormonkeypatchfor safer isolation.♻️ Safer alternative using monkeypatch
`@pytest.fixture`() -def config_file(tmp_path: Path) -> Iterator[Path]: +def config_file(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[Path]: """Provide a temporary config file path and set EVALHUB_CONFIG.""" path = tmp_path / "config.yaml" - os.environ["EVALHUB_CONFIG"] = str(path) + monkeypatch.setenv("EVALHUB_CONFIG", str(path)) yield path - os.environ.pop("EVALHUB_CONFIG", None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_cli_mcp.py` around lines 16 - 22, The fixture config_file currently mutates os.environ directly which isn't isolated; update it to use pytest's monkeypatch (add monkeypatch to the fixture signature) and call monkeypatch.setenv("EVALHUB_CONFIG", str(path)) before yield so the environment is automatically restored after each test, or alternatively when invoking the CLI use Click's CliRunner(env={...}) for test-specific envs; keep the fixture name config_file and the tmp_path usage but remove direct os.environ modification and the manual os.environ.pop cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/evalhub/cli/main.py`:
- Around line 902-909: The created AsyncEvalHubClient is stored via
set_client(client) but never closed; either construct it with an async context
manager (use "async with AsyncEvalHubClient(...)" and call set_client inside
that block so the client is auto-closed when mcp_server.run_stdio_async()
returns) or ensure explicit shutdown by awaiting its async close method after
the server run (call await client.aclose() or asyncio.run(client.aclose()) in a
finally/shutdown hook). Update the code around AsyncEvalHubClient creation and
the call to mcp_server.run_stdio_async() to use one of these approaches
(referencing AsyncEvalHubClient, set_client, mcp_server.run_stdio_async, and the
client.aclose()/__aenter__/__aexit__ methods) so the underlying httpx client is
always cleaned up on exit.
In `@tests/unit/test_cli_mcp.py`:
- Around line 99-105: Add the missing assertion that verifies set_client was
invoked in the test_mcp_cli_flags_override_profile test: after the existing
mocks/assertions (including mock_client_cls.assert_called_once_with(...)) add a
call to assert mock_set_client.assert_called_once() to mirror
test_mcp_resolves_from_profile; locate the mock named mock_set_client in the
test function and assert it was called exactly once to ensure the client was set
during the CLI flags override scenario.
---
Nitpick comments:
In `@tests/unit/test_cli_mcp.py`:
- Around line 16-22: The fixture config_file currently mutates os.environ
directly which isn't isolated; update it to use pytest's monkeypatch (add
monkeypatch to the fixture signature) and call
monkeypatch.setenv("EVALHUB_CONFIG", str(path)) before yield so the environment
is automatically restored after each test, or alternatively when invoking the
CLI use Click's CliRunner(env={...}) for test-specific envs; keep the fixture
name config_file and the tmp_path usage but remove direct os.environ
modification and the manual os.environ.pop cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6304f8a8-dca9-40d7-9547-2e62fdd6aaf7
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.tomlsrc/evalhub/cli/main.pysrc/evalhub/mcp/__main__.pytests/unit/test_cli_mcp.py
💤 Files with no reviewable changes (1)
- src/evalhub/mcp/main.py
Member
Author
LIVE DEMO that refactor (wire mcp into cli) works as intendedensure authenticated with for completeness: ensure evalhub command is available: you can notice using: works as intended 
if missing the auth token gets 401 in MCP inspector and on the server: otherwise works as intended: |
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: tarilabs <matteo.mortari@gmail.com>
tarilabs
requested review from
gnaulak-redhat,
julpayne,
mariusdanciu,
nbs-rh,
ppadashe-psp,
ruivieira and
scheruku-rh
ppadashe-psp
approved these changes
Mar 28, 2026
Member
|
One thought (don't feel strongly about it, and I can't recall if this was discussed previously): right now the Would it make sense to flip this?
That way:
The evalhub mcp subcommand would still work exactly as-is; it just means the MCP server code doesn't drag in CLI dependencies when used standalone. A thin |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
evalhub mcpsubcommand reusing CLI profile/config for base-url, token, tenantevalhub-mcpentry point andsrc/evalhub/mcp/__main__.pyeval-hub-sdk[cli])mcppackage; show install hint when missingType
Testing
see thread for manual tests
Breaking changes
Summary by CodeRabbit
New Features
mcpsubcommand to the main CLI to start the MCP server using centralized configuration.Bug Fixes & Improvements
Tests