Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughAdds a new stdio-based MCP server package ( Changes
Sequence Diagram(s)sequenceDiagram
participant IDE as IDE / AI Assistant
participant MCP as MCP Server (jmp mcp serve)
participant ConnMgr as ConnectionManager
participant Exporter as Exporter / Device
IDE->>MCP: jmp_create_lease(exporter, duration)
MCP->>ConnMgr: connect(config, lease_name, exporter)
ConnMgr->>Exporter: acquire lease & serve_unix_async()
Exporter-->>ConnMgr: unix socket path + driver client
ConnMgr-->>MCP: connection_id, socket_path
IDE->>MCP: jmp_explore(connection_id)
MCP->>ConnMgr: get connection -> client.cli
ConnMgr-->>MCP: CLI tree / metadata
MCP-->>IDE: CLI metadata
IDE->>MCP: jmp_run(connection_id, ["..."], timeout)
MCP->>Exporter: spawn subprocess with env (socket path)
Exporter-->>MCP: stdout, exit code
MCP-->>IDE: command result
IDE->>MCP: jmp_disconnect(connection_id)
MCP->>ConnMgr: disconnect(connection_id)
ConnMgr->>Exporter: end lease / cleanup
ConnMgr-->>MCP: disconnected
MCP-->>IDE: status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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: 1
🧹 Nitpick comments (1)
python/packages/jumpstarter-mcp/PLAN.md (1)
73-104: Add a language to the fenced code block.Line 73 opens a fenced block without a language tag, which triggers markdownlint MD040. Use
```text(or another explicit language) for that block.Suggested doc fix
-``` +```text Jumpstarter provides remote access to physical hardware devices through a controller that manages leases and exporters. ... - Use jmp_driver_methods to get exact method signatures for Python code</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@python/packages/jumpstarter-mcp/PLAN.mdaround lines 73 - 104, The fenced
code block showing the Jumpstarter workflow in PLAN.md is missing a language tag
(causing markdownlint MD040); update the opening fence fromtotext (or
another explicit language like "markdown" or "md") for the block that begins
with "Jumpstarter provides remote access..." so the linter recognizes the code
block language.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@python/packages/jumpstarter-mcp/PLAN.md:
- Line 110: Update the package path references in PLAN.md to match the
repository layout by replacing occurrences of "packages/jumpstarter-mcp/" with
"python/packages/jumpstarter-mcp/" (including the text that describes creating
the package and any entry-point notes like "jmp mcp serve"); ensure all other
mentions (e.g., at line 332) are normalized as well so the doc consistently
points to the actual repo location.
Nitpick comments:
In@python/packages/jumpstarter-mcp/PLAN.md:
- Around line 73-104: The fenced code block showing the Jumpstarter workflow in
PLAN.md is missing a language tag (causing markdownlint MD040); update the
opening fence fromtotext (or another explicit language like "markdown"
or "md") for the block that begins with "Jumpstarter provides remote access..."
so the linter recognizes the code block language.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `62860ac2-ffcb-4f27-809a-11de3a0f6c8c` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 0cf0d61d6188281bffd3737c9202d1d40f49efd6 and 8c178e29eb83798a057c5d9fa76fbf80751260ac. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `python/uv.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (1)</summary> * `python/packages/jumpstarter-mcp/PLAN.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
mangelajo
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/jmp.py`:
- Around line 41-46: The try/except block around "from jumpstarter_mcp.cli
import mcp" swallows real import errors and hides broken installs; replace it by
importing mcp unconditionally so failures surface (remove the try/except and
keep "from jumpstarter_mcp.cli import mcp" and "jmp.add_command(mcp)"), or if
you intentionally want to only handle missing package installs, narrow the
handler to ModuleNotFoundError and verify the exception.name equals
"jumpstarter_mcp" before suppressing; update the code around the import and
jmp.add_command(mcp) accordingly.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/connections.py`:
- Around line 228-246: The connection entry is left in self._connections after
the background tasks finish; update the teardown in the async with
anyio.create_task_group() block (around notify_tg.cancel_scope.cancel(), after
await notify_send.aclose() and after the socket/client teardown) to remove the
dead connection from the manager state by doing a safe removal like
self._connections.pop(connection_id, None) (or equivalent) so that
get_connection(), list_connections(), get_env(), and run_command() no longer see
stale entries; ensure removal is done unconditionally when that background
teardown completes and do it in the same scope as where
_forward_lease_notifications/_watch_lease_transfer and notify_send.aclose() are
handled to avoid races.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/introspect.py`:
- Around line 103-120: list_drivers() is producing dot-prefixed paths like
"client.power" that are incompatible with
get_driver_methods()/jmp_driver_methods which expect child-key lists like
["power"]; change list_drivers to emit child-relative paths (no leading
"client") and avoid producing empty segments for the root driver by: start with
an empty prefix (prefix="" default) or when composing child paths use
f"{prefix}.{name}".lstrip(".") / conditional join so the root entry becomes ""
or the driver name (not "client" or "client.."), and ensure any example or
caller that expects jmp_driver_methods gets the split path by doing
path.split(".") while skipping empty segments; update references to
list_drivers, get_driver_methods, and jmp_driver_methods accordingly.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py`:
- Around line 360-378: _wrap the file-logging setup in _setup_logging with a
broad try/cept so failures in Path.home(), log_dir.mkdir(), or
logging.FileHandler(...) cannot raise and stop the server; catch Exception
around the block that computes log_dir, creates the directory, creates the
FileHandler, and calls root.addHandler, and on failure skip adding the file
handler (optionally call logging.getLogger(__name__).warning or print a short
message) so the stdio server always starts even if file logging is unavailable.
Use the existing _setup_logging function and the symbols Path.home,
log_dir.mkdir, logging.FileHandler, and root.addHandler to locate where to wrap
the try/except.
- Around line 82-103: The current _load_config() creates a
ClientConfigV1Alpha1() instance that can be empty, so change the logic to detect
an unusable config and fall back to the saved user config: after constructing or
attempting to construct ClientConfigV1Alpha1, check its required fields (e.g.,
config.endpoint and config.token) for truthy values; if either is
missing/None/empty, call
UserConfigV1Alpha1.load_or_create().config.current_client and use that instead
(instead of relying solely on catching ValidationError). Ensure this check
references ClientConfigV1Alpha1 and
UserConfigV1Alpha1.load_or_create().config.current_client and returns a valid
config or raises the existing RuntimeError if still None.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/tools/commands.py`:
- Around line 44-45: The logs currently print the full CLI argv which can leak
secrets; create a small sanitizer (e.g., redact_argv or redact_args) that
returns only the subcommand name (command[0]) or a redacted representation (mask
all subsequent tokens as "<redacted>" or replace with count) and use it in every
logger call that currently interpolates " ".join(command) (the Running
command/info, success, timeout, and failure logs) and in any exception handlers
in the same module so no positional or flag values are logged verbatim.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9909b028-dbfb-4526-9e1f-bbc1141a21bf
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
python/docs/source/getting-started/guides/ai-agent-integration.mdpython/docs/source/getting-started/guides/index.mdpython/docs/source/getting-started/guides/integration-patterns.mdpython/docs/source/reference/package-apis/index.mdpython/docs/source/reference/package-apis/mcp.mdpython/packages/jumpstarter-cli/jumpstarter_cli/jmp.pypython/packages/jumpstarter-cli/pyproject.tomlpython/packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.pypython/packages/jumpstarter-mcp/README.mdpython/packages/jumpstarter-mcp/jumpstarter_mcp/__init__.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/cli.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/connections.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/introspect.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/__init__.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/commands.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/connections.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/leases.pypython/packages/jumpstarter-mcp/pyproject.tomlpython/pyproject.toml
✅ Files skipped from review due to trivial changes (8)
- python/docs/source/reference/package-apis/index.md
- python/docs/source/getting-started/guides/index.md
- python/docs/source/reference/package-apis/mcp.md
- python/docs/source/getting-started/guides/integration-patterns.md
- python/packages/jumpstarter-mcp/README.md
- python/docs/source/getting-started/guides/ai-agent-integration.md
- python/packages/jumpstarter-mcp/pyproject.toml
- python/packages/jumpstarter-cli/pyproject.toml
Comment on lines
+82
to
+103
| def _load_config() -> ClientConfigV1Alpha1: | ||
| """Load client config using the same resolution as jmp CLI.""" | ||
| from pydantic import ValidationError | ||
|
|
||
| from jumpstarter.config.user import UserConfigV1Alpha1 | ||
|
|
||
| config = None | ||
| try: | ||
| config = ClientConfigV1Alpha1() | ||
| except ValidationError: | ||
| pass | ||
|
|
||
| if config is None: | ||
| config = UserConfigV1Alpha1.load_or_create().config.current_client | ||
|
|
||
| if config is None: | ||
| raise RuntimeError( | ||
| "No jumpstarter client config found. " | ||
| "Run 'jmp config set-client <name>' or set JUMPSTARTER_* environment variables." | ||
| ) | ||
|
|
||
| return config |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'def _load_config|ClientConfigV1Alpha1\(|endpoint: str \| None|token: str \| None' \
python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py \
python/packages/jumpstarter/jumpstarter/config/client.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 2859
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full definition of ClientConfigV1Alpha1
fd -t f "client.py" python/packages/jumpstarter/jumpstarter/config/ --exec cat -n {} \; | \
head -200Repository: jumpstarter-dev/jumpstarter
Length of output: 7832
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are any validators or model_config that enforce required fields in ClientConfigV1Alpha1
rg -A 30 "class ClientConfigV1Alpha1" python/packages/jumpstarter/jumpstarter/config/client.py | head -50Repository: jumpstarter-dev/jumpstarter
Length of output: 1691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how _load_config() return value is actually used in the MCP server
rg -n "_load_config\(\)|endpoint|token" python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py | head -30Repository: jumpstarter-dev/jumpstarter
Length of output: 297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the actual usage context where _load_config() is called in server.py
rg -B 3 -A 10 "_load_config\(\)" python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py | head -60Repository: jumpstarter-dev/jumpstarter
Length of output: 1931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are any validators in ClientConfigV1Alpha1 that might validate endpoint/token
rg -A 50 "class ClientConfigV1Alpha1\(BaseSettings\)" python/packages/jumpstarter/jumpstarter/config/client.py | grep -A 50 "endpoint\|token\|validator"Repository: jumpstarter-dev/jumpstarter
Length of output: 1585
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify that lease_tools functions use config.channel()
rg -n "def list_exporters|def list_leases|def create_lease" \
python/packages/jumpstarter-mcp/jumpstarter_mcp/tools/*.py | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what lease_tools.list_exporters does with the config
rg -A 10 "async def list_exporters|config.channel\(\)" \
python/packages/jumpstarter-mcp/jumpstarter_mcp/tools/*.py | head -30Repository: jumpstarter-dev/jumpstarter
Length of output: 1141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Final check: verify config.list_exporters eventually calls config.channel()
rg -A 5 "async def list_exporters" python/packages/jumpstarter/jumpstarter/config/client.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full list_exporters implementation
rg -A 20 "async def list_exporters" python/packages/jumpstarter/jumpstarter/config/client.py | head -30Repository: jumpstarter-dev/jumpstarter
Length of output: 779
_load_config() never reaches the saved-user-config fallback when no environment variables are set.
ClientConfigV1Alpha1() is constructible with default None values for endpoint and token, so the ValidationError catch never triggers. The function returns an unusable config instead of falling back to UserConfigV1Alpha1.load_or_create().config.current_client. This breaks the standard workflow where users run jmp config set-client <name> and expect it to work. At runtime, any MCP tool invocation fails with ConfigurationError when config.channel() is called.
Suggested fix
def _load_config() -> ClientConfigV1Alpha1:
"""Load client config using the same resolution as jmp CLI."""
from pydantic import ValidationError
from jumpstarter.config.user import UserConfigV1Alpha1
- config = None
- try:
- config = ClientConfigV1Alpha1()
- except ValidationError:
- pass
-
- if config is None:
- config = UserConfigV1Alpha1.load_or_create().config.current_client
+ try:
+ env_config = ClientConfigV1Alpha1()
+ except ValidationError:
+ env_config = None
+
+ config = env_config
+ if config is None or config.endpoint is None or config.token is None:
+ config = UserConfigV1Alpha1.load_or_create().config.current_client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py` around lines 82 -
103, The current _load_config() creates a ClientConfigV1Alpha1() instance that
can be empty, so change the logic to detect an unusable config and fall back to
the saved user config: after constructing or attempting to construct
ClientConfigV1Alpha1, check its required fields (e.g., config.endpoint and
config.token) for truthy values; if either is missing/None/empty, call
UserConfigV1Alpha1.load_or_create().config.current_client and use that instead
(instead of relying solely on catching ValidationError). Ensure this check
references ClientConfigV1Alpha1 and
UserConfigV1Alpha1.load_or_create().config.current_client and returns a valid
config or raises the existing RuntimeError if still None.
Member
Author
There was a problem hiding this comment.
That's incorrect, this is actually working.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify once more whether there are any validators in ClientConfigV1Alpha1 that I may have missed.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete ClientConfigV1Alpha1 definition including validators
cat -n python/packages/jumpstarter/jumpstarter/config/client.py | head -120
echo "---"
# Also check if there are model_validators or field_validators
rg -n "model_validator|field_validator|@validator" python/packages/jumpstarter/jumpstarter/config/client.py
Length of output: 4449
Let me check a few more lines to see the full ClientConfigV1Alpha1 field and validator definitions.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get lines 120-250 of client.py to see endpoint/token fields and any validators
sed -n '120,260p' python/packages/jumpstarter/jumpstarter/config/client.py | cat -n
Length of output: 6168
@mangelajo, thanks for the clarification. I can see it works correctly when JMP_ENDPOINT/JMP_TOKEN environment variables are set — in that case ClientConfigV1Alpha1() reads them via pydantic-settings and returns a fully configured instance.
My concern is specifically about users who configure Jumpstarter only via jmp config set-client <name> without any JMP_* env vars. In that scenario:
ClientConfigV1Alpha1()succeeds without raisingValidationError(since bothendpointandtokenarestr | None = Field(default=None)— no validators on the class).configis therefore neverNone, so theUserConfigV1Alpha1.load_or_create().config.current_clientfallback is never reached.- The returned config has
endpoint=None/token=None, and every subsequent MCP tool call fails atchannel()withConfigurationError("endpoint or token not set in client config").
If you've verified that the named-client config path (jmp config set-client) also works correctly in your testing, I'm happy to accept that and note it as a learning. Could you clarify whether that path was tested, or whether the current design intentionally targets only the env-var-based configuration?
🧠 Learnings used
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 320
File: python/packages/jumpstarter-cli/jumpstarter_cli/shell.py:308-341
Timestamp: 2026-03-14T11:11:29.265Z
Learning: In `python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`, the `--tls-grpc` path (`_shell_direct_async`) intentionally does NOT use `ClientConfigV1Alpha1`. It constructs a synthetic config with `allow=[]`, `unsafe=True`, and `use_profiles=False` because this is a direct, controller-free connection mode. Do not flag the absence of a real client config here as an issue.
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 365
File: python/packages/jumpstarter-mcp/jumpstarter_mcp/tools/commands.py:44-45
Timestamp: 2026-03-23T10:38:49.265Z
Learning: In the `jumpstarter-mcp` package (`python/packages/jumpstarter-mcp/jumpstarter_mcp/tools/commands.py`), the `j` CLI binary does not generally accept sensitive arguments such as tokens or passwords. Logging the full `j` command argv (via `" ".join(command)`) is acceptable because the arguments are not sensitive and the MCP server log is local (`~/.jumpstarter/logs/mcp-server.log`). Do not flag this as a secret-leakage issue in future reviews.
Comment on lines
+360
to
+378
| def _setup_logging() -> None: | ||
| """Configure logging with both stderr and file output. | ||
|
|
||
| Adds a file handler to the root logger regardless of whether | ||
| other handlers (e.g. from the mcp library) are already present. | ||
| """ | ||
| from pathlib import Path | ||
|
|
||
| log_dir = Path.home() / ".jumpstarter" / "logs" | ||
| log_dir.mkdir(parents=True, exist_ok=True) | ||
| log_file = log_dir / "mcp-server.log" | ||
|
|
||
| fmt = "%(asctime)s %(levelname)s %(name)s: %(message)s" | ||
| file_handler = logging.FileHandler(log_file, mode="a") | ||
| file_handler.setLevel(logging.DEBUG) | ||
| file_handler.setFormatter(logging.Formatter(fmt)) | ||
|
|
||
| root = logging.getLogger() | ||
| root.addHandler(file_handler) |
Contributor
There was a problem hiding this comment.
File logging shouldn’t be able to prevent the MCP server from starting.
Right now Path.home(), mkdir(), or logging.FileHandler(...) can raise before the stdio server comes up. In restricted environments with a read-only or missing home directory, jmp mcp serve will fail even though file logging is only a convenience.
Suggested fix
def _setup_logging() -> None:
@@
- log_dir = Path.home() / ".jumpstarter" / "logs"
- log_dir.mkdir(parents=True, exist_ok=True)
- log_file = log_dir / "mcp-server.log"
-
fmt = "%(asctime)s %(levelname)s %(name)s: %(message)s"
- file_handler = logging.FileHandler(log_file, mode="a")
- file_handler.setLevel(logging.DEBUG)
- file_handler.setFormatter(logging.Formatter(fmt))
-
root = logging.getLogger()
- root.addHandler(file_handler)
+ try:
+ log_dir = Path.home() / ".jumpstarter" / "logs"
+ log_dir.mkdir(parents=True, exist_ok=True)
+ file_handler = logging.FileHandler(log_dir / "mcp-server.log", mode="a")
+ except (OSError, RuntimeError):
+ logger.warning("Disabling MCP file logging", exc_info=True)
+ else:
+ file_handler.setLevel(logging.DEBUG)
+ file_handler.setFormatter(logging.Formatter(fmt))
+ root.addHandler(file_handler)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py` around lines 360 -
378, _wrap the file-logging setup in _setup_logging with a broad try/cept so
failures in Path.home(), log_dir.mkdir(), or logging.FileHandler(...) cannot
raise and stop the server; catch Exception around the block that computes
log_dir, creates the directory, creates the FileHandler, and calls
root.addHandler, and on failure skip adding the file handler (optionally call
logging.getLogger(__name__).warning or print a short message) so the stdio
server always starts even if file logging is unavailable. Use the existing
_setup_logging function and the symbols Path.home, log_dir.mkdir,
logging.FileHandler, and root.addHandler to locate where to wrap the try/except.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/docs/source/getting-started/guides/ai-agent-integration.md`:
- Around line 33-34: The sentence mentioning the MCP server package is a run-on
and has a formatting typo (missing space after inline code): split into two
clear sentences and insert a space after `jumpstarter-mcp`, e.g. state that the
MCP server package is normally provided via a full install and then separately
note that the `jumpstarter-mcp` package provides the `jmp mcp serve` CLI
subcommand; update the phrase containing `jumpstarter-mcp` and `jmp mcp serve`
to ensure there is a space after the inline code and the wording is concise and
readable.
- Around line 175-199: Add a language identifier to the fenced code block that
begins with "$ claude" (the example console transcript) to satisfy markdownlint
MD040 by changing the opening fence from ``` to something like ```text (or
```console/```bash if more appropriate), and also remove any stray diff-markup
hyphens inside the block (e.g., the leading "-" before "The board is running
Fedora...") so the block contains only the transcript text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb8638f3-4385-4aab-9102-c7a22e970c37
📒 Files selected for processing (1)
python/docs/source/getting-started/guides/ai-agent-integration.md
Add an MCP (Model Context Protocol) server that exposes Jumpstarter's lease management, device connections, and command execution as structured tools accessible by AI coding agents (Cursor, Claude Code, Claude Desktop, or any MCP-compatible host). The server enables natural-language-driven hardware interaction from IDEs: - Lease & exporter management (list, create, delete) - Persistent device connections with CLI command execution - Driver introspection (discover methods, signatures, docstrings) - Environment export for Python scripting with the env() helper Also includes: - Documentation guide with setup instructions for Cursor, Claude Code, and Claude Desktop, plus usage examples and workflow diagrams - Symlink in docs reference section for package API docs - Unit tests (24 tests covering introspection, connections, commands) - CompositeClient.__getattr__ fix to raise AttributeError instead of KeyError
- Remove connection from manager state in _run_connection finally block so expired/transferred leases don't leave stale entries - Add driver_path field to list_drivers output (e.g. ["power"]) so jmp_drivers results can be passed directly to jmp_driver_methods - Fix docs cross-reference to use doc-tree path instead of filesystem path Made-with: Cursor
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
python/docs/source/getting-started/guides/ai-agent-integration.md (2)
175-199:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
The code block at line 175 is missing a language identifier, which triggers markdownlint MD040.
Suggested fix
-``` +```text $ claude > /mcp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/docs/source/getting-started/guides/ai-agent-integration.md` around lines 175 - 199, The fenced code block that begins with the shell interaction starting with "$ claude" is missing a language identifier (triggering markdownlint MD040); update the opening fence to include a language tag (e.g., ```text) so the block becomes ```text ... ```; locate the block that contains the lines starting with "$ claude" and the MCP interaction (commands like "/mcp", "Connected MCP servers:", and the subsequent output) and add the language identifier to the opening triple backticks.
33-34:⚠️ Potential issue | 🟡 MinorFix the run-on sentence in prerequisites.
The sentence is awkward and has redundant phrasing ("which...which").
Suggested fix
-The MCP server package, which is normally provided when you perform a full install -through the `jumpstarter-mcp` package which provides the `jmp mcp serve` subcommand on the CLI. +The MCP server is provided by the `jumpstarter-mcp` package, which adds the +`jmp mcp serve` subcommand. This package is included in a full Jumpstarter installation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/docs/source/getting-started/guides/ai-agent-integration.md` around lines 33 - 34, Rewrite the run-on prerequisites sentence to remove the redundant "which...which" and improve flow; replace the current line that references "The MCP server package, which is normally provided when you perform a full install through the `jumpstarter-mcp` package which provides the `jmp mcp serve` subcommand on the CLI." with a concise sentence that names the MCP server package, states it is installed by the `jumpstarter-mcp` package, and mentions the `jmp mcp serve` subcommand once (e.g., "The MCP server package is installed by the `jumpstarter-mcp` package and provides the `jmp mcp serve` CLI subcommand."). Ensure the phrase "MCP server package", "`jumpstarter-mcp`", and "`jmp mcp serve`" remain present for clarity.python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py (2)
82-103:⚠️ Potential issue | 🟠 Major
_load_config()fallback logic is still incomplete.
ClientConfigV1Alpha1()can be constructed with defaultNonevalues forendpointandtokenwithout raisingValidationError. The function returns this unusable config instead of falling back to the user config. This breaks the standard workflow where users runjmp config set-client <name>.Suggested fix
def _load_config() -> ClientConfigV1Alpha1: """Load client config using the same resolution as jmp CLI.""" from pydantic import ValidationError from jumpstarter.config.user import UserConfigV1Alpha1 config = None try: config = ClientConfigV1Alpha1() except ValidationError: pass - if config is None: + if config is None or config.endpoint is None or config.token is None: config = UserConfigV1Alpha1.load_or_create().config.current_client if config is None: raise RuntimeError(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py` around lines 82 - 103, _load_config currently accepts a ClientConfigV1Alpha1 instance even when required fields like endpoint or token are None because ClientConfigV1Alpha1() doesn't raise ValidationError; update _load_config to detect unusable defaults by checking the created config's essential fields (e.g., config.endpoint and config.token) and if either is missing, treat it as None and fall back to UserConfigV1Alpha1.load_or_create().config.current_client; after falling back, re-check that the final config has both endpoint and token and raise the RuntimeError if still missing so callers of _load_config get a valid usable client config.
360-386:⚠️ Potential issue | 🟠 MajorFile logging setup can prevent the MCP server from starting.
Path.home(),mkdir(), orlogging.FileHandler()can raise exceptions in restricted environments (read-only or missing home directory). This will crash the server before it comes up, even though file logging is only a convenience feature.Suggested fix
def _setup_logging() -> None: """Configure logging with both stderr and file output.""" from pathlib import Path + fmt = "%(asctime)s %(levelname)s %(name)s: %(message)s" + root = logging.getLogger() + + try: - log_dir = Path.home() / ".jumpstarter" / "logs" - log_dir.mkdir(parents=True, exist_ok=True) - log_file = log_dir / "mcp-server.log" - - fmt = "%(asctime)s %(levelname)s %(name)s: %(message)s" - file_handler = logging.FileHandler(log_file, mode="a") - file_handler.setLevel(logging.DEBUG) - file_handler.setFormatter(logging.Formatter(fmt)) - - root = logging.getLogger() - root.addHandler(file_handler) + log_dir = Path.home() / ".jumpstarter" / "logs" + log_dir.mkdir(parents=True, exist_ok=True) + file_handler = logging.FileHandler(log_dir / "mcp-server.log", mode="a") + file_handler.setLevel(logging.DEBUG) + file_handler.setFormatter(logging.Formatter(fmt)) + root.addHandler(file_handler) + except (OSError, RuntimeError): + logger.warning("File logging disabled (cannot create log file)") logging.getLogger("jumpstarter_mcp").setLevel(logging.DEBUG)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py` around lines 360 - 386, The _setup_logging function currently calls Path.home(), log_dir.mkdir(...), and logging.FileHandler(...) without guarding against IO/OS errors which can abort startup; wrap the file-log creation and addition in a try/except that catches OSError/IOError (and optionally PermissionError) around Path.home(), mkdir, and logging.FileHandler so failures fall back to continuing without a file handler, emit a warning to the existing stderr StreamHandler via logging.getLogger().warning or print if no handlers exist, and only call root.addHandler(file_handler) if the handler was successfully created; reference _setup_logging, Path.home, mkdir, logging.FileHandler, root.addHandler, and the file_handler variable when making the change.python/packages/jumpstarter-mcp/jumpstarter_mcp/introspect.py (1)
151-167:⚠️ Potential issue | 🟡 MinorMalformed call_example for root-level drivers.
When
driver_pathis empty (root client),attr_pathbecomes an empty string, producing call examples likeclient..method(...)with a double dot.Suggested fix
attr_path = ".".join(driver_path) call_args = ", ".join(f"{p['name']}=..." for p in params) - method_call = f"client.{attr_path}.{name}({call_args})" + target = f"client.{attr_path}" if attr_path else "client" + method_call = f"{target}.{name}({call_args})"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/introspect.py` around lines 151 - 167, The call_example generation can produce a double-dot when driver_path is empty because attr_path is joined into an empty string; update the construction of method_call used in call_example so it conditionally includes attr_path only when driver_path is non-empty (i.e., if driver_path: method_call = f"client.{attr_path}.{name}({call_args})" else: method_call = f"client.{name}({call_args})"), and keep attr_path and call_args logic as-is; ensure call_example uses this corrected method_call variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@python/docs/source/getting-started/guides/ai-agent-integration.md`:
- Around line 175-199: The fenced code block that begins with the shell
interaction starting with "$ claude" is missing a language identifier
(triggering markdownlint MD040); update the opening fence to include a language
tag (e.g., ```text) so the block becomes ```text ... ```; locate the block that
contains the lines starting with "$ claude" and the MCP interaction (commands
like "/mcp", "Connected MCP servers:", and the subsequent output) and add the
language identifier to the opening triple backticks.
- Around line 33-34: Rewrite the run-on prerequisites sentence to remove the
redundant "which...which" and improve flow; replace the current line that
references "The MCP server package, which is normally provided when you perform
a full install through the `jumpstarter-mcp` package which provides the `jmp mcp
serve` subcommand on the CLI." with a concise sentence that names the MCP server
package, states it is installed by the `jumpstarter-mcp` package, and mentions
the `jmp mcp serve` subcommand once (e.g., "The MCP server package is installed
by the `jumpstarter-mcp` package and provides the `jmp mcp serve` CLI
subcommand."). Ensure the phrase "MCP server package", "`jumpstarter-mcp`", and
"`jmp mcp serve`" remain present for clarity.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/introspect.py`:
- Around line 151-167: The call_example generation can produce a double-dot when
driver_path is empty because attr_path is joined into an empty string; update
the construction of method_call used in call_example so it conditionally
includes attr_path only when driver_path is non-empty (i.e., if driver_path:
method_call = f"client.{attr_path}.{name}({call_args})" else: method_call =
f"client.{name}({call_args})"), and keep attr_path and call_args logic as-is;
ensure call_example uses this corrected method_call variable.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py`:
- Around line 82-103: _load_config currently accepts a ClientConfigV1Alpha1
instance even when required fields like endpoint or token are None because
ClientConfigV1Alpha1() doesn't raise ValidationError; update _load_config to
detect unusable defaults by checking the created config's essential fields
(e.g., config.endpoint and config.token) and if either is missing, treat it as
None and fall back to UserConfigV1Alpha1.load_or_create().config.current_client;
after falling back, re-check that the final config has both endpoint and token
and raise the RuntimeError if still missing so callers of _load_config get a
valid usable client config.
- Around line 360-386: The _setup_logging function currently calls Path.home(),
log_dir.mkdir(...), and logging.FileHandler(...) without guarding against IO/OS
errors which can abort startup; wrap the file-log creation and addition in a
try/except that catches OSError/IOError (and optionally PermissionError) around
Path.home(), mkdir, and logging.FileHandler so failures fall back to continuing
without a file handler, emit a warning to the existing stderr StreamHandler via
logging.getLogger().warning or print if no handlers exist, and only call
root.addHandler(file_handler) if the handler was successfully created; reference
_setup_logging, Path.home, mkdir, logging.FileHandler, root.addHandler, and the
file_handler variable when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef90b2ae-1a60-449c-aa12-dc45ccf5535b
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
python/docs/source/getting-started/guides/ai-agent-integration.mdpython/docs/source/getting-started/guides/index.mdpython/docs/source/getting-started/guides/integration-patterns.mdpython/docs/source/reference/package-apis/index.mdpython/docs/source/reference/package-apis/mcp.mdpython/packages/jumpstarter-cli/jumpstarter_cli/jmp.pypython/packages/jumpstarter-cli/pyproject.tomlpython/packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.pypython/packages/jumpstarter-mcp/README.mdpython/packages/jumpstarter-mcp/jumpstarter_mcp/__init__.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/cli.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/connections.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/introspect.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/__init__.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/commands.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/connections.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/leases.pypython/packages/jumpstarter-mcp/pyproject.tomlpython/pyproject.toml
✅ Files skipped from review due to trivial changes (6)
- python/docs/source/reference/package-apis/mcp.md
- python/docs/source/reference/package-apis/index.md
- python/packages/jumpstarter-cli/pyproject.toml
- python/docs/source/getting-started/guides/integration-patterns.md
- python/packages/jumpstarter-mcp/README.md
- python/packages/jumpstarter-mcp/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- python/packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py
- python/docs/source/getting-started/guides/index.md
- python/pyproject.toml
- python/packages/jumpstarter-cli/jumpstarter_cli/jmp.py
- python/packages/jumpstarter-mcp/jumpstarter_mcp/tools/connections.py
- python/packages/jumpstarter-mcp/jumpstarter_mcp/connections.py
When the access token is expired or within 30s of expiry, the MCP server now automatically attempts a refresh using the stored refresh_token via OIDC before making gRPC calls. This avoids forcing users to manually re-authenticate with `jmp login` during long MCP sessions. Made-with: Cursor
Member
Author
|
I have more ideas to improve it, like opening and streaming serial ports/stream ports in the background, or creating TCP/UDP redirections in the background. But I will do if we agree to merge this one, don't want to make this huge for review. |
bennyz
approved these changes
Mar 24, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py (2)
428-438:⚠️ Potential issue | 🟠 MajorFile logging failures should not prevent MCP startup.
Lines [428]-[438] can raise in constrained environments (home dir/mkdir/file handler), which would abort
jmp mcp servebefore stdio is available.Proposed fix
def _setup_logging() -> None: @@ - log_dir = Path.home() / ".jumpstarter" / "logs" - log_dir.mkdir(parents=True, exist_ok=True) - log_file = log_dir / "mcp-server.log" - fmt = "%(asctime)s %(levelname)s %(name)s: %(message)s" - file_handler = logging.FileHandler(log_file, mode="a") - file_handler.setLevel(logging.DEBUG) - file_handler.setFormatter(logging.Formatter(fmt)) - root = logging.getLogger() - root.addHandler(file_handler) + try: + log_dir = Path.home() / ".jumpstarter" / "logs" + log_dir.mkdir(parents=True, exist_ok=True) + file_handler = logging.FileHandler(log_dir / "mcp-server.log", mode="a") + except Exception: + logger.warning("MCP file logging disabled", exc_info=True) + else: + file_handler.setLevel(logging.DEBUG) + file_handler.setFormatter(logging.Formatter(fmt)) + root.addHandler(file_handler)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py` around lines 428 - 438, Wrap the file-logging setup (the log_dir, log_file, file_handler creation and root.addHandler call) in a try/except that catches Exception and silently falls back to not using a FileHandler so startup cannot be aborted by IO errors; specifically, enclose the existing statements that create log_dir, call mkdir, create logging.FileHandler, setLevel/setFormatter and root.addHandler(file_handler) in a try block and in the except block do not re-raise (optionally call logging.getLogger().debug or pass to record the failure) so jmp mcp serve continues even when file-handler creation fails.
90-99:⚠️ Potential issue | 🔴 CriticalFallback logic should also handle incomplete env config.
Line [92] can produce a config object with missing runtime essentials (e.g., endpoint/token), so Line [96] never falls back to saved
current_client, and downstream controller calls fail.Proposed fix
def _load_config() -> ClientConfigV1Alpha1: @@ - config = None + config: ClientConfigV1Alpha1 | None = None try: config = ClientConfigV1Alpha1() except ValidationError: pass - if config is None: + if config is None or not config.endpoint or not config.token: config = UserConfigV1Alpha1.load_or_create().config.current_client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py` around lines 90 - 99, The code initializing `config` with `ClientConfigV1Alpha1()` may create an incomplete object missing critical runtime details; therefore, after the try-except block in the server initialization, add validation logic to check if required fields like endpoint and token are present in `config`. If the config is incomplete or missing these essentials, fallback to loading the saved `current_client` from `UserConfigV1Alpha1.load_or_create()`. This ensures that fallback logic correctly handles partial environment configs to avoid downstream failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.py`:
- Around line 529-538: The test currently checks that ClientConfigV1Alpha1.save
wasn't called but doesn't assert the OIDC refresh path was skipped; update
test_token_without_exp_claim_skips_refresh to patch/mock the actual refresh
helper used by _ensure_fresh_token (e.g., the module-level refresh function
called inside _ensure_fresh_token) and assert that this mock was not called,
while keeping the existing assertions on result and ClientConfigV1Alpha1.save;
reference the test name test_token_without_exp_claim_skips_refresh, the function
under test _ensure_fresh_token, and the ClientConfigV1Alpha1 class so the patch
targets the correct symbol.
---
Duplicate comments:
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py`:
- Around line 428-438: Wrap the file-logging setup (the log_dir, log_file,
file_handler creation and root.addHandler call) in a try/except that catches
Exception and silently falls back to not using a FileHandler so startup cannot
be aborted by IO errors; specifically, enclose the existing statements that
create log_dir, call mkdir, create logging.FileHandler, setLevel/setFormatter
and root.addHandler(file_handler) in a try block and in the except block do not
re-raise (optionally call logging.getLogger().debug or pass to record the
failure) so jmp mcp serve continues even when file-handler creation fails.
- Around line 90-99: The code initializing `config` with
`ClientConfigV1Alpha1()` may create an incomplete object missing critical
runtime details; therefore, after the try-except block in the server
initialization, add validation logic to check if required fields like endpoint
and token are present in `config`. If the config is incomplete or missing these
essentials, fallback to loading the saved `current_client` from
`UserConfigV1Alpha1.load_or_create()`. This ensures that fallback logic
correctly handles partial environment configs to avoid downstream failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89aca7ac-8b2d-4c06-8aa2-a7385beb5940
📒 Files selected for processing (2)
python/packages/jumpstarter-mcp/jumpstarter_mcp/server.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.py
Comment on lines
+529
to
+538
| async def test_token_without_exp_claim_skips_refresh(self): | ||
| config = MagicMock() | ||
| config.token = _make_jwt_payload(exp=None) | ||
| config.refresh_token = "some-refresh" | ||
|
|
||
| with patch("jumpstarter_mcp.server.ClientConfigV1Alpha1") as mock_cls: | ||
| result = await _ensure_fresh_token(config) | ||
|
|
||
| assert result is config | ||
| mock_cls.save.assert_not_called() |
Contributor
There was a problem hiding this comment.
test_token_without_exp_claim_skips_refresh does not verify “skip refresh”.
Line [529] names a skip-path test, but Lines [534]-[538] never assert that OIDC refresh was not invoked; the test can still pass if refresh is attempted and fails.
Proposed test tightening
`@pytest.mark.asyncio`
async def test_token_without_exp_claim_skips_refresh(self):
config = MagicMock()
config.token = _make_jwt_payload(exp=None)
config.refresh_token = "some-refresh"
- with patch("jumpstarter_mcp.server.ClientConfigV1Alpha1") as mock_cls:
+ with (
+ patch("jumpstarter_mcp.server.ClientConfigV1Alpha1") as mock_cls,
+ patch("jumpstarter_cli_common.oidc.Config") as mock_oidc_cls,
+ ):
result = await _ensure_fresh_token(config)
assert result is config
mock_cls.save.assert_not_called()
+ mock_oidc_cls.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server_test.py` around lines
529 - 538, The test currently checks that ClientConfigV1Alpha1.save wasn't
called but doesn't assert the OIDC refresh path was skipped; update
test_token_without_exp_claim_skips_refresh to patch/mock the actual refresh
helper used by _ensure_fresh_token (e.g., the module-level refresh function
called inside _ensure_fresh_token) and assert that this mock was not called,
while keeping the existing assertions on result and ClientConfigV1Alpha1.save;
reference the test name test_token_without_exp_claim_skips_refresh, the function
under test _ensure_fresh_token, and the ClientConfigV1Alpha1 class so the patch
targets the correct symbol.
mangelajo
commented
Mar 24, 2026
Member
Author
|
thank you @bennyz ! :) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py (2)
429-439:⚠️ Potential issue | 🟠 MajorFile logging setup can prevent the MCP server from starting.
Lines 429-435 can raise in restricted environments (e.g., unwritable home dir), which aborts
jmp mcp servebefore stdio is up. Logging should be best-effort here.Proposed fix
def _setup_logging() -> None: @@ - log_dir = Path.home() / ".jumpstarter" / "logs" - log_dir.mkdir(parents=True, exist_ok=True) - log_file = log_dir / "mcp-server.log" - fmt = "%(asctime)s %(levelname)s %(name)s: %(message)s" - file_handler = logging.FileHandler(log_file, mode="a") - file_handler.setLevel(logging.DEBUG) - file_handler.setFormatter(logging.Formatter(fmt)) - root = logging.getLogger() - root.addHandler(file_handler) + try: + log_dir = Path.home() / ".jumpstarter" / "logs" + log_dir.mkdir(parents=True, exist_ok=True) + file_handler = logging.FileHandler(log_dir / "mcp-server.log", mode="a") + file_handler.setLevel(logging.DEBUG) + file_handler.setFormatter(logging.Formatter(fmt)) + root.addHandler(file_handler) + except Exception: + logger.warning("MCP file logging disabled; continuing without file handler", exc_info=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py` around lines 429 - 439, The file-based logging setup using Path.home(), log_dir.mkdir(...), and logging.FileHandler(log_file) can raise exceptions in restricted environments and abort startup; wrap the entire block that creates log_dir, creates log_file, instantiates file_handler and calls root.addHandler(file_handler) in a broad try/except that logs a warning to stderr (or the existing console/stdio logger) and silently continues on failure so startup does not abort; ensure variables referenced are log_dir, log_file, file_handler and root, and do not remove or change the existing stdio/console handlers—only add the file handler when the operations succeed.
91-99:⚠️ Potential issue | 🔴 Critical
_load_config()can return an unusable config and skip the intended fallback.On Line 93,
ClientConfigV1Alpha1()may succeed with empty defaults, so the fallback on Line 98 is never reached. In that case, tools fail later when a real endpoint/token is required.Proposed fix
def _load_config() -> ClientConfigV1Alpha1: @@ - config = None + config: ClientConfigV1Alpha1 | None = None try: config = ClientConfigV1Alpha1() except ValidationError: pass - if config is None: + if config is None or not config.endpoint or not config.token: config = UserConfigV1Alpha1.load_or_create().config.current_client - if config is None: + if config is None or not config.endpoint or not config.token: raise RuntimeError( "No jumpstarter client config found. " "Run 'jmp config client use <name>' or set JUMPSTARTER_* environment variables." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py` around lines 91 - 99, ClientConfigV1Alpha1() can succeed with empty defaults and thus prevent the intended fallback; change _load_config() so after constructing ClientConfigV1Alpha1 (and catching ValidationError) you validate that required fields (e.g. endpoint/token or whatever fields ClientConfigV1Alpha1 requires) are present and non-empty, and if they are missing then load the fallback using UserConfigV1Alpha1.load_or_create().config.current_client; reference ClientConfigV1Alpha1, ValidationError, and UserConfigV1Alpha1.load_or_create().config.current_client to locate where to add the presence check and fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py`:
- Around line 243-257: The current logic prevents rebinding a log callback
(check of manager._log_callback) causing the one-shot _log closure to capture a
stale session; remove the early return and instead register a stable callback
via manager.set_log_callback that, on each call, calls mcp.get_context() to
fetch the current request_context/session and then forwards the message via
session.send_log_message (handling exceptions), so logs always go to the active
session; update references around manager.set_log_callback,
manager._log_callback, the _log closure and use
mcp.get_context()/request_context.session for each invocation.
---
Duplicate comments:
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py`:
- Around line 429-439: The file-based logging setup using Path.home(),
log_dir.mkdir(...), and logging.FileHandler(log_file) can raise exceptions in
restricted environments and abort startup; wrap the entire block that creates
log_dir, creates log_file, instantiates file_handler and calls
root.addHandler(file_handler) in a broad try/except that logs a warning to
stderr (or the existing console/stdio logger) and silently continues on failure
so startup does not abort; ensure variables referenced are log_dir, log_file,
file_handler and root, and do not remove or change the existing stdio/console
handlers—only add the file handler when the operations succeed.
- Around line 91-99: ClientConfigV1Alpha1() can succeed with empty defaults and
thus prevent the intended fallback; change _load_config() so after constructing
ClientConfigV1Alpha1 (and catching ValidationError) you validate that required
fields (e.g. endpoint/token or whatever fields ClientConfigV1Alpha1 requires)
are present and non-empty, and if they are missing then load the fallback using
UserConfigV1Alpha1.load_or_create().config.current_client; reference
ClientConfigV1Alpha1, ValidationError, and
UserConfigV1Alpha1.load_or_create().config.current_client to locate where to add
the presence check and fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ffa8803-692e-4624-aa2b-baac829b18ed
📒 Files selected for processing (1)
python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py
Comment on lines
+243
to
+257
| if manager._log_callback is not None: | ||
| return | ||
| try: | ||
| ctx = mcp.get_context() | ||
| if ctx.request_context: | ||
| session = ctx.request_context.session | ||
|
|
||
| async def _log(level: str, message: str) -> None: | ||
| try: | ||
| await session.send_log_message(level=level, data=message, logger="jumpstarter") | ||
| except Exception: | ||
| pass | ||
|
|
||
| manager.set_log_callback(_log) | ||
| except (LookupError, AttributeError): |
Contributor
There was a problem hiding this comment.
Notification callback capture is one-shot and can go stale.
Line 243 prevents rebinding once a callback exists, but this manager stores only one callback and replaces it on set_log_callback(). That means later sessions may never receive logs.
Proposed fix
def _capture_session_for_notifications(mcp: FastMCP, manager: ConnectionManager) -> None:
"""Try to capture the MCP session so background tasks can send log notifications."""
- if manager._log_callback is not None:
- return
try:
ctx = mcp.get_context()
if ctx.request_context:
session = ctx.request_context.session
@@
manager.set_log_callback(_log)
except (LookupError, AttributeError):
pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py` around lines 243 -
257, The current logic prevents rebinding a log callback (check of
manager._log_callback) causing the one-shot _log closure to capture a stale
session; remove the early return and instead register a stable callback via
manager.set_log_callback that, on each call, calls mcp.get_context() to fetch
the current request_context/session and then forwards the message via
session.send_log_message (handling exceptions), so logs always go to the active
session; update references around manager.set_log_callback,
manager._log_callback, the _log closure and use
mcp.get_context()/request_context.session for each invocation.
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
2 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.
Summary
jumpstarter-mcp, a new package providing an MCP (Model Context Protocol) server that exposes Jumpstarter's lease management, device connections, and command execution as structured tools for AI coding agentsjmp mcp serveWhat's included
New package:
jumpstarter-mcpserver.py): Registers tools for lease/exporter management, device connections, CLI command execution, and driver introspectionconnections.py): Manages persistent device connections with graceful error handling for client disconnectsintrospect.py): Walk Click CLI trees and inspect Python driver method signatures/docstringscli.py): Addsjmp mcp servesubcommandDocumentation
ai-agent-integration.mdwith setup for Cursor, Claude Code, and Claude Desktopintegration-patterns.mdreference/package-apis/mcp.mdOther changes
CompositeClient.__getattr__now raisesAttributeErrorinstead ofKeyErrorfor missing attributes (fixes introspection compatibility)jumpstarter-clinow optionally loads the MCP subcommandTest plan
make lint-fixpasses clean (0 errors)make pkg-test-jumpstarter-mcppasses (24/24 tests)jmp mcp servestarts and tools are accessible from Cursorclaude mcp add jumpstarter -- jmp mcp serveworks in Claude Code