Repository navigation
fix(node): Fix mcp-client sse and streamable-http - #267
Merged
Merged
Conversation
ryan-t-christensen
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
March 17, 2026 06:26
github-actions
Bot
added
module:nodes
Contributor
📝 WalkthroughWalkthroughAdded transport-aware config resolution and validation helpers to the MCP client core, tightened transport-specific validation (stdio/sse/streamable-http), standardized tools/list RPC params to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
nodes/src/nodes/mcp_client/IGlobal.py (3)
43-51: Consider moving the helper function after all imports.Defining
_is_mappingbetween import groups is unconventional and may cause confusion during maintenance. Typically, helper functions are placed after all imports.♻️ Suggested reordering
from ai.common.config import Config from rocketlib import IGlobalBase, OPEN_MODE, warning - -def _is_mapping(obj: Any) -> bool: - """Check if obj is dict-like (supports .get and .items), including IJson.""" - return hasattr(obj, 'get') and hasattr(obj, 'items') - from .mcp_stdio_client import McpStdioClient, McpToolDef from .mcp_sse_client import McpSseClient from .mcp_streamable_http_client import McpStreamableHttpClient from .mcp_driver import McpDriver + + +def _is_mapping(obj: Any) -> bool: + """Check if obj is dict-like (supports .get and .items), including IJson.""" + return hasattr(obj, 'get') and hasattr(obj, 'items')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/mcp_client/IGlobal.py` around lines 43 - 51, The helper function _is_mapping is placed between import statements which breaks conventional ordering; move the _is_mapping definition so that all imports (McpStdioClient, McpToolDef, McpSseClient, McpStreamableHttpClient, McpDriver) appear together at the top, then place the _is_mapping helper after those imports (and after any module-level constants) so imports are grouped and helpers follow them.
78-83: Falsy check may not distinguish explicit empty values from missing keys.
if not val:treats empty strings,0, andFalseas falsy and falls through to the top-level config. If a transport subconfig explicitly sets a key to an empty string to override a top-level default, this logic will ignore it and use the top-level value instead.If this is intentional for backward compatibility (prefer non-empty values from either location), consider adding a brief comment. Otherwise, use
if val is None:to only fall back when the key is truly absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/mcp_client/IGlobal.py` around lines 78 - 83, The _get function currently treats any falsy transport value (empty string, 0, False) as missing and falls back to cfg; change the conditional to only fall back when the transport lookup actually returns None (i.e., use "if val is None:" instead of "if not val:") so explicit empty-string or zero overrides are preserved, and optionally add a brief comment above the check explaining the intended precedence between tcfg and cfg.
172-212: Consider extracting shared config resolution logic.The
_vgethelper andtcfgresolution invalidateConfiglargely duplicate the pattern frombeginGlobal. For maintainability, consider extracting the transport-subconfig resolution to a shared helper method or module-level function.That said, keeping validation logic self-contained has its merits for clarity, so this is optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/mcp_client/IGlobal.py` around lines 172 - 212, The config-resolution logic (tcfg resolution and the _vget helper) in validateConfig duplicates the pattern used in beginGlobal; extract this into a shared helper function (e.g., get_transport_config and vget_from_config) at module scope and replace the inline tcfg/_vget usage in validateConfig with calls to those helpers so both beginGlobal and validateConfig reuse the same logic; ensure the helper accepts the original cfg and transport name, returns the resolved tcfg mapping and a vget(key) closure or function that applies the same fallback/stripping behavior, and update validation branches (checks for commandLine/command/args, sse_endpoint, endpoint) to call the new vget/getters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nodes/src/nodes/mcp_client/IGlobal.py`:
- Around line 43-51: The helper function _is_mapping is placed between import
statements which breaks conventional ordering; move the _is_mapping definition
so that all imports (McpStdioClient, McpToolDef, McpSseClient,
McpStreamableHttpClient, McpDriver) appear together at the top, then place the
_is_mapping helper after those imports (and after any module-level constants) so
imports are grouped and helpers follow them.
- Around line 78-83: The _get function currently treats any falsy transport
value (empty string, 0, False) as missing and falls back to cfg; change the
conditional to only fall back when the transport lookup actually returns None
(i.e., use "if val is None:" instead of "if not val:") so explicit empty-string
or zero overrides are preserved, and optionally add a brief comment above the
check explaining the intended precedence between tcfg and cfg.
- Around line 172-212: The config-resolution logic (tcfg resolution and the
_vget helper) in validateConfig duplicates the pattern used in beginGlobal;
extract this into a shared helper function (e.g., get_transport_config and
vget_from_config) at module scope and replace the inline tcfg/_vget usage in
validateConfig with calls to those helpers so both beginGlobal and
validateConfig reuse the same logic; ensure the helper accepts the original cfg
and transport name, returns the resolved tcfg mapping and a vget(key) closure or
function that applies the same fallback/stripping behavior, and update
validation branches (checks for commandLine/command/args, sse_endpoint,
endpoint) to call the new vget/getters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5b4caa8-91d2-4961-be4b-82def56c1fa4
📒 Files selected for processing (6)
nodes/src/nodes/mcp_client/IGlobal.pynodes/src/nodes/mcp_client/mcp_sse_client.pynodes/src/nodes/mcp_client/mcp_stdio_client.pynodes/src/nodes/mcp_client/mcp_streamable_http_client.pynodes/src/nodes/mcp_client/services.jsonpackages/shared-ui/src/components/rjsf-theme/select-widget/SelectWidget.tsx
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nodes/src/nodes/mcp_client/IGlobal.py (1)
98-102:⚠️ Potential issue | 🟡 MinorAddress inconsistent type validation between dict and list arguments.
The code uses
_is_mapping()to duck-type validate dict-like objects (line 110-113) to accommodate C++ IJson objects that failisinstance(obj, dict). However,argsvalidation still relies onisinstance(args, list)(line 101), creating an inconsistency. If IJson objects behave the same way for lists, this check could reject valid IJson list arguments.For consistency and robustness, add a duck-type check for list-like objects similar to
_is_mapping():`@staticmethod` def _is_sequence(obj: Any) -> bool: """Check if obj is list-like (supports iteration), including IJson.""" return hasattr(obj, '__iter__') and not isinstance(obj, (str, dict))Then apply it to the args validation at line 101.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/mcp_client/IGlobal.py` around lines 98 - 102, Add a duck-typed sequence check and use it for args validation: implement a static method _is_sequence(obj) (similar to existing _is_mapping) that returns True for list-like objects (e.g., hasattr(obj, '__iter__') and not isinstance(obj, (str, dict))), then replace the current isinstance(args, list) check in the block that sets args = tcfg.get('args') or cfg.get('args') or [] with a check that uses _is_sequence(args) so IJson sequence-like objects are accepted and the Exception('mcp_client args must be a list of strings') is only raised when args is not sequence-like.
🧹 Nitpick comments (1)
nodes/src/nodes/mcp_client/IGlobal.py (1)
75-84: Consider extracting transport config resolution to a shared helper.The transport key mapping (line 75-77) and the
_get/_vgetlookup pattern are duplicated invalidateConfig(lines 173-181). Extracting this to a reusable method would reduce duplication and ensure both paths stay in sync.♻️ Possible refactor
+ `@staticmethod` + def _resolve_transport_config(cfg: Any, transport: str) -> tuple[Any, callable]: + """Return (transport_sub_config, getter_fn) for nested vs flat config lookup.""" + _transport_key = {'stdio': 'stdio', 'sse': 'sse', 'streamable-http': 'http'}.get(transport) + _sub = cfg.get(_transport_key) if _transport_key else None + tcfg = _sub if IGlobal._is_mapping(_sub) else {} + + def _get(key: str, default: str = '') -> str: + val = tcfg.get(key) if tcfg else None + if not val: + val = cfg.get(key) + return str(val).strip() if val else default + + return tcfg, _getThen in both
beginGlobalandvalidateConfig:tcfg, _get = self._resolve_transport_config(cfg, self.transport)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nodes/src/nodes/mcp_client/IGlobal.py` around lines 75 - 84, Extract the transport-key mapping and the transport-specific lookup logic into a shared helper (suggest name _resolve_transport_config) on the IGlobal class, moving the map {'stdio': 'stdio', 'sse': 'sse', 'streamable-http': 'http'} and the inner _get/_vget lookup behavior into that helper; have it accept (cfg, transport) and return the resolved tcfg and a callable get(key, default) so you can replace the duplicated code in beginGlobal (the _transport_key/_get block) and in validateConfig with tcfg, _get = self._resolve_transport_config(cfg, self.transport). Ensure the helper preserves existing semantics (prefers transport sub-object keys, falls back to top-level, returns str().strip() or default) and is used from both validateConfig and beginGlobal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@nodes/src/nodes/mcp_client/IGlobal.py`:
- Around line 98-102: Add a duck-typed sequence check and use it for args
validation: implement a static method _is_sequence(obj) (similar to existing
_is_mapping) that returns True for list-like objects (e.g., hasattr(obj,
'__iter__') and not isinstance(obj, (str, dict))), then replace the current
isinstance(args, list) check in the block that sets args = tcfg.get('args') or
cfg.get('args') or [] with a check that uses _is_sequence(args) so IJson
sequence-like objects are accepted and the Exception('mcp_client args must be a
list of strings') is only raised when args is not sequence-like.
---
Nitpick comments:
In `@nodes/src/nodes/mcp_client/IGlobal.py`:
- Around line 75-84: Extract the transport-key mapping and the
transport-specific lookup logic into a shared helper (suggest name
_resolve_transport_config) on the IGlobal class, moving the map {'stdio':
'stdio', 'sse': 'sse', 'streamable-http': 'http'} and the inner _get/_vget
lookup behavior into that helper; have it accept (cfg, transport) and return the
resolved tcfg and a callable get(key, default) so you can replace the duplicated
code in beginGlobal (the _transport_key/_get block) and in validateConfig with
tcfg, _get = self._resolve_transport_config(cfg, self.transport). Ensure the
helper preserves existing semantics (prefers transport sub-object keys, falls
back to top-level, returns str().strip() or default) and is used from both
validateConfig and beginGlobal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1001ffb2-38d7-4ce0-9d0d-d1e3b57424a8
📒 Files selected for processing (1)
nodes/src/nodes/mcp_client/IGlobal.py
stepmikhaylov
approved these changes
Mar 17, 2026
ryan-t-christensen
added a commit
that referenced
this pull request
Mar 18, 2026
* fix mcp client sse http-stream, adjust styling * move _is_mapping into IGlobal as a staticmethod
Rod-Christensen
added a commit
that referenced
this pull request
Mar 18, 2026
* feat(vscode): better engine control
Local mode no longer uses a hardcoded port (previously defaulted to 5565).
The engine now starts with --port=0 so the OS assigns a free port, and
the extension parses the actual port from Uvicorn's startup output.
Why: Multiple VS Code windows running local engines would collide on the
same port. Dynamic allocation lets each window get its own engine process
without conflicts.
Affected: engine-manager.ts, connection.ts, config.ts
Engine installs are now placed in versioned directories
(<enginesRoot>/engines/<tag>--<hash>/) instead of a single shared folder.
A cross-process lockfile (proper-lockfile) coordinates installs so multiple
VS Code windows don't race. Channel pointer files (current-stable.json,
current-pre.json) track which version is active. PID files allow safe
cleanup of old version directories that aren't in use.
Why: The previous single-directory model meant that upgrading the engine
while another window was running it would clobber the in-use binary.
On Windows this caused EBUSY errors; on all platforms it was a race
condition between download/extract and running processes.
Also moves the engine storage root from context.extensionPath to
context.globalStorageUri.fsPath so engines persist across extension
updates instead of being re-downloaded each time.
Affected: engine-installer.ts, engine-manager.ts, extension.ts,
connection.ts, package.json (added proper-lockfile)
Removed debugger registration from package.json (activation events,
breakpoints contribution, debuggers contribution) and commented out
the registerDebugger() call in extension.ts. The adapter.ts and
session.ts files are still compiled but not wired up; adapter.ts now
contains a restoration guide in its header comment.
Why: The debugger is being reworked and was adding activation overhead
and user confusion while non-functional.
Affected: package.json, extension.ts, adapter.ts
- Moved API key input from a top-level "RocketRide account" section into
the On-prem connection mode panel (it's only relevant there).
- Cloud mode now shows a "Coming Soon" placeholder in both Settings and
Welcome pages.
- Mode-specific fields are wrapped in a bordered config box for clarity.
- Replaced the array-based engine arguments editor with a single text
input (engineArgs changed from string[] to string).
- Added a "Full debug output" checkbox (local.debugOutput) that passes
--trace=servicePython, shown instead of the manual args field when
enabled.
- Removed the local port input — port is now dynamic and not user-facing.
- Renamed "Engine Version" to "Server Version" throughout the UI.
- Hidden internal-only commands from the command palette (file ops,
status, setup credentials).
- Added two new commands: rocketride.provider.connection.focus and
rocketride.provider.files.focus.
Added an IntegrationSettings component with toggle checkboxes for
GitHub Copilot, Claude Code, Cursor, and Windsurf. Backed by new
VS Code configuration keys (rocketride.integrations.*).
Why: Preparing for AI assistant integrations; the toggles are
persisted but not yet wired to functionality.
Affected: IntegrationSettings.tsx (new), PageSettings.tsx,
PageSettingsProvider.ts, package.json
When the parent engine was started with --trace=..., child pipeline
processes now inherit that flag automatically (unless the user explicitly
set one). Uses the new startup_args() import from rocketlib.
Why: Without this, debugging child processes required manually adding
trace args to every pipeline launch configuration.
- Welcome page text colors changed from VS Code theme variables to
explicit rgba(255,255,255,...) values for consistent appearance on
the dark branded background.
- Mode description moved inside a config box with border.
- MUI InputLabel style overrides added for smaller italic labels.
- Removed padding from MuiInputBase-input.
- Placeholder text styled with italic/light/reduced opacity.
- Reconnection backoff cap reduced from 15s to 5s.
* feat(vscode,ai): debug trace propagation, DAP logging, and settings refinements
Add a "Full debug output" checkbox to Settings (local + on-prem modes) that
injects --trace=servicePython into engine args. This gives users a single
toggle for detailed trace logging without manually editing server arguments.
The trace flag is sent both to the local engine command line and via DAP
execute/launch requests, so task subprocesses receive it too.
The trace flag respects user overrides: if the user types their own --trace=
in the Server Arguments field, the checkbox's value is skipped to avoid
duplicates — the engine's C++ argparser rejects duplicate --trace entries.
On the server side, task_engine.py now inherits the parent engine's --trace
setting from sys.argv when the DAP args don't include one, ensuring
subprocesses always get trace config regardless of how the engine was started.
User-provided args containing spaces are safely split using shlex.split()
to handle quoted paths like --path='C:\Program Files'.
DAP request/response logging added to ConnectionManager so all commands
(execute, launch, etc.) and their full payloads are visible in the
RocketRide: Extension output panel — previously these were completely silent,
making debugging difficult.
Other refinements:
- Server Arguments field now only shown in local mode (not on-prem)
- "when VS Code starts" → "when extension starts" for accuracy
- Suppress Python 3.12 frozen modules debugger warning in node.py
- Reduce reconnection backoff cap from 15s to 5s
* feat(vscode): add local OS service deployment for RocketRide engine
Add the ability to install, update, start, stop, and remove the
RocketRide engine as a native OS service directly from the Deploy page.
This supplements the existing Cloud and Docker deployment options with a
third "Local Service" path that runs the engine as a background service
that starts automatically on boot.
Key changes:
- New `deploy/` module with platform-specific service managers:
- Windows: uses NSSM (Non-Sucking Service Manager)
- Linux: uses systemd unit files
- macOS: uses launchd plist files
- Common `ServiceManager` abstract base class with port-check utility
- PageDeployProvider: orchestrates service lifecycle by combining
EngineInstaller (download/version management) with ServiceManager
(OS service registration). Tracks installed version in a config file
at ProgramData/RocketRide/config.json. Adds status polling (3s
interval) and a waitForServiceRunning loop with 60s timeout.
- PageDeploy.tsx: new service status display (state indicator, version,
install path), split-button UI with version dropdown for install and
update actions, start/stop/remove controls, progress and error
feedback via messages from the provider.
- styles.css: service status indicators (color-coded states), split
button with dropdown, progress/error message styles.
- engine-installer.ts: extract `getLatestPointer()` to deduplicate the
channel-fallback logic across getExecutablePath, getInstalledDir,
getInstalledVersion, and getInstalledPublishedAt.
- engine-manager.ts: respect configured engineVersion setting when
reading version info (prerelease vs stable channel).
- onprem.svg: replaced chip icon with a server/gear icon that better
represents local service deployment.
* feat(vscode): replace terminal-based Docker deploy with full container lifecycle management
The previous Docker deployment was a fire-and-forget approach that opened a
terminal, ran `docker pull` and `docker create`, and gave the user no
feedback or control afterward. This meant users had no way to start, stop,
update, or remove the container from within VS Code — they had to drop to
the CLI for everything after initial setup.
This commit replaces that with a proper Docker container manager that
mirrors the existing OS service (NSSM/systemd/launchd) deployment panel:
- Add `DockerManager` class (`docker-manager.ts`) using the dockerode SDK
to provide install, start, stop, remove, update, and status operations.
Handles Windows named-pipe detection (Docker Desktop vs standalone) and
maps Docker API errors to user-friendly messages.
- Add `ssh2` stub (`src/stubs/ssh2.js`) and esbuild alias so that
docker-modem (a dockerode dependency) doesn't pull in native .node
binaries — we only use local socket connections, never SSH tunnels.
- Extend `PageDeployProvider` backend with Docker lifecycle commands
(dockerInstall, dockerRemove, dockerUpdate, dockerStart, dockerStop),
Docker status polling, and GHCR tag fetching via the Docker Registry V2
anonymous token flow to populate the version dropdown.
- Add Docker config persistence (`docker-config.json` in ProgramData /
/Library/Application Support / /etc) to track which version was
installed, since Docker container inspect doesn't carry that metadata.
- Rework the React `PageDeploy` UI to give Docker its own status display,
progress/error feedback, and split-button version selector — the same
UX pattern already used by the OS service panel. Refactor shared UI
elements (renderSplitButton, renderInstalledActions, renderStatusIndicator)
to be reusable across both panels instead of duplicated.
- Update progress bar styling to use monospace font with text-overflow
ellipsis, better suited for streaming Docker pull output.
- Add `dockerode` and `@types/dockerode` as dependencies; sort
existing deps alphabetically in package.json.
* Potential fix for code scanning alert no. 422: Unused variable, import, function or class
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
* Potential fix for code scanning alert no. 423: Unused variable, import, function or class
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
* Potential fix for code scanning alert no. 424: Unused import
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
* * Coderabbit fixes
* *Resolve issues with Linux service manager and Linux docker
* feat(builder): check dependencies of overlay root (#253)
* fix(builder): minor fix in client-typescript build
* fix(node): Fix mcp-client sse and streamable-http (#267)
* fix mcp client sse http-stream, adjust styling
* move _is_mapping into IGlobal as a staticmethod
* test: Add fulltest profiles for model-downloading nodes sentence transformer, NER, OCR, embedding_image (#273)
* fix adding full test context
* Add missing line
* feat(nodes): add fulltest support for all-profile node testing (#272)
* feat(nodes): add fulltest support for all-profile node testing
Add a 'fulltest' test key to service*.json (anonymize as first node)
listing all available profiles. Extend the test framework to discover
and parametrize tests from this key, and add test_dynamic_fulltest.py
that exercises every profile without the skip_nodes filter.
- anonymize/services.json: add 'fulltest' with all 17 GLiNER/Gretel profiles
- discovery.py: add test_key param to discover_testable_nodes/_parse_test_config
- conftest.py: refactor pytest_generate_tests; add node_fulltest_config parametrization
- test_dynamic_fulltest.py: new test class TestDynamicNodesFull (opt-in, no skip filter)
Fulltest is invoked via builder model_server:fulltest, which runs
model_server:test first and then exercises all profile variants.
* fix: small fixes for fulltest
* fix(ci): add explicit token permissions to workflows (#261)
Co-authored-by: Dmitrii Karataev <dmitriikarataev@Dmitriis-MacBook-Pro.local>
* break out tooling connections in agent nodes for user clarity and ease of use (#263)
* Match Linux deploy patterns (sudo -S, launchd status, port conflict handling, auto-connect). Fix Docker pull on ARM Macs and use user-writable config paths
* * Updates from coderabbit
---------
Co-authored-by: Rod.Christensen <rod.christensen@aparavi.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: stepmik <stepmikhaylov@yandex.ru>
Co-authored-by: Ryan Christensen <ryan.christensen@rocketride.ai>
Co-authored-by: Ariel Vernaza <ariel@lazyracoon.tech>
Co-authored-by: Alexandru Sclearuc <asclearuc@gmail.com>
Co-authored-by: Dmitrii Karataev <dmitry.karataev@gmail.com>
Co-authored-by: Dmitrii Karataev <dmitriikarataev@Dmitriis-MacBook-Pro.local>
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
back to top-level for back-compat with flat preconfig profiles.
Type
Testing
./builder testpassesChecklist
Related Issues
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements
Configuration Changes