Conversation
Add an embedded Model Context Protocol (MCP) server to the skit application, gated behind an optional `mcp` Cargo feature flag. When enabled and configured, the server exposes a Streamable HTTP endpoint at `/api/v1/mcp` (configurable) that reuses existing auth, CORS, tracing, and metrics middleware. Phase 1 provides six read/discover/validate/manage tools aimed at automated testing, debugging, and pipeline design use cases: - list_nodes: permission-filtered node definitions with schemas - validate_pipeline: stateless YAML dry-run returning diagnostics - create_session: create a dynamic session from YAML - list_sessions: list active sessions (permission-filtered) - get_pipeline: runtime pipeline snapshot including node states - destroy_session: stop and remove a session Implementation details: - rmcp 1.5 with Streamable HTTP transport and tool_router macro - McpConfig struct (enabled + endpoint) added to server config - StreamKitMcp service with ServerHandler trait implementation - Auth extracted from HTTP request parts injected by rmcp - Public MCP helpers in server module avoid exposing private types Closes: #358 (Phase 1) Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Prevent misconfiguration where setting mcp.endpoint to a path outside /api/ would silently bypass auth_guard_middleware and origin_guard_middleware. The server now panics at startup if the endpoint is not under /api/. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| fn extract_auth( | ||
| ctx: &RequestContext<RoleServer>, | ||
| app_state: &Arc<AppState>, | ||
| ) -> Result<(String, Permissions), McpError> { | ||
| let parts = ctx | ||
| .extensions | ||
| .get::<http::request::Parts>() | ||
| .ok_or_else(|| McpError::internal_error("missing HTTP request parts", None))?; | ||
| Ok(crate::role_extractor::get_role_and_permissions(&parts.headers, app_state)) | ||
| } |
There was a problem hiding this comment.
📝 Info: extract_auth relies on http crate type identity between rmcp and the MCP module
At apps/skit/src/mcp/mod.rs:62, extract_auth does ctx.extensions.get::<http::request::Parts>(). This requires that the http::request::Parts type in the MCP module (from http v1) is identical to the type rmcp injects into the extensions.
Since both rmcp and this crate depend on http v1, Cargo resolves them to the same version, making the types identical. If a future dependency update caused a version split (e.g. rmcp pins http 1.1 while this crate uses http 1.2), the get::<Parts>() would silently return None, and the MCP HTTP transport would fall back to the STDIO auth path (unauthenticated admin). The integration tests verify this works correctly today.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Correct — this is an implicit contract with rmcp's StreamableHttpService. The behavior is documented in rmcp's transport-streamable-http-server module: it extracts http::request::Parts from the incoming Axum request and inserts them into the RequestContext extensions. If rmcp ever stops doing this, the failure mode is safe (all tool calls fail with "missing HTTP request parts" rather than silently falling back to unauthenticated access). Worth adding an integration test that exercises this path — tracked for Phase 3 (#361).
| pub fn streamable_http_service( | ||
| app_state: Arc<AppState>, | ||
| ) -> StreamableHttpService<StreamKitMcp, LocalSessionManager> { | ||
| StreamableHttpService::new( | ||
| move || Ok(StreamKitMcp::new(Arc::clone(&app_state))), | ||
| Arc::new(LocalSessionManager::default()), | ||
| StreamableHttpServerConfig::default(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🚩 StreamableHttpServerConfig::default() may need tuning for production
At mcp.rs:555, StreamableHttpServerConfig::default() is used without any customization. The defaults of the rmcp library control session limits, timeouts, and other parameters for the MCP protocol. In production, unbounded MCP sessions could be a resource concern. However, the auth middleware limits access to authenticated users, and MCP sessions are lightweight protocol sessions (not pipeline sessions), so this is likely acceptable. Worth reviewing the rmcp defaults if this sees heavy use.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Agreed — the defaults are acceptable for Phase 1 since MCP protocol sessions are lightweight (just SSE connections) and already gated behind auth. Worth tuning session limits and timeouts once this sees real usage. Can be addressed as part of production hardening in a follow-up.
- Deduplicate validation: refactor validate_pipeline_handler to call validate_pipeline_yaml, eliminating near-identical implementations. - Return typed ValidateResponse instead of serde_json::Value, removing the unnecessary double-serialization pass. - Document StreamableHttpServerConfig defaults (rmcp 1.5) and disable allowed_hosts since auth/origin middleware already guards the endpoint. - Add comment explaining why create_session hard-codes dynamic-mode rules rather than going through check_mode. - Remove racy pre-flight is_name_taken/can_accept_session check in create_session; add_session rechecks under the lock. - Change pub fn wrappers to pub (effectively pub(crate) in this private module) per clippy redundant_pub_crate lint. - Add MCP integration tests: unauthenticated request rejection, authenticated initialization, and validate_pipeline tool call. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
MUST FIX: - Move endpoint validation from runtime assert! to config::load time, returning a config error instead of panicking the binary - Tighten endpoint validator: only accept /api/v<digits>/mcp(/...), reject /api/v1/auth/ prefix (auth bypass), path traversal (..) SHOULD FIX: - Warn when mcp.enabled=true but binary compiled without mcp feature - Document SessionConfig keep_alive TTL (5 min) that evicts idle MCP sessions, preventing unbounded growth - Add mcp.allowed_hosts config for DNS rebinding protection on the Host header; disabled by default (acceptable behind auth middleware) - Factor inline oneshot-node check to shared is_synthetic_kind() used by both HTTP and MCP create_session paths - Integration test: panic on PermissionDenied instead of silent skip - Add tests: config validation, permission denial, destroy nonexistent NITS: - Propagate add_session error message instead of hardcoded string - check_file_path_security now accumulates all errors like HTTP path - Set explicit ServerInfo name/version (streamkit + CARGO_PKG_VERSION) - Restore debug! traces in validate_pipeline_yaml for parse/compile - Comment rmcp feature flags in Cargo.toml Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| router = router.nest_service( | ||
| &app_state.config.mcp.endpoint, | ||
| crate::mcp::streamable_http_service(Arc::clone(&app_state)), | ||
| ); |
There was a problem hiding this comment.
🚩 MCP sessions have no body-size limit unlike HTTP handlers
The HTTP create_session_handler and validate_pipeline_handler have DefaultBodyLimit::max(app_state.config.server.max_body_size) applied. The MCP endpoint, mounted via nest_service, does not have an explicit body-size limit. Axum's default body limit (2 MiB) applies, and the rmcp library may impose its own limits on JSON-RPC message size. For MCP tool calls carrying pipeline YAML, the default limit is likely sufficient since pipeline definitions are typically small. However, if very large pipelines are expected, this could be a consideration.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Acknowledged — Axum's default 2 MiB body limit applies to the nested MCP service. Pipeline YAML definitions are well within this limit in practice. If very large pipelines become a use case, we can apply DefaultBodyLimit::max(config.server.max_body_size) as a layer on the nested service.
The catch-all arm silently mapped misspelled modes (e.g. 'Dynamic') to None, skipping mode-specific validation and producing false-positive results. Explicitly reject unknown mode values with an error. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…ers, add positive-path tests - Add pre-flight session-limit and name-uniqueness checks before Session::create to avoid wasted engine allocation (Finding 1) - Use McpError::invalid_request instead of internal_error for session limit and name collision errors (Finding 2) - Remove thin mcp_ wrapper functions; make originals pub and call them directly from the MCP module (Findings 3+4) - Add positive-path tests: list_nodes, create→list→get→destroy round trip covering all 6 MCP tools (Finding 5) - Replace flake-prone 100ms startup sleep with /healthz polling in integration tests (Finding 5) - Convert call_tool to async fn to satisfy clippy::manual_async_fn Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
* feat(mcp): add generate_oneshot_command tool Add a new MCP tool that generates curl or skit-cli commands for executing oneshot (batch processing) pipelines. The tool validates the pipeline YAML before generating the command, returning diagnostics if validation fails. Key details: - Supports 'curl' (default) and 'skit-cli' output formats - Validates YAML with mode=oneshot before generating commands - Uses 'config' as the multipart field name (matching the server API) - Includes integration tests for both formats, invalid YAML, and permission denial Signed-off-by: Devin AI <devin@streamkit.dev> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> * fix(mcp): fix duplicate --input flags and add shell quoting - Fix duplicate --input flags in generate_skit_cli_command when no input has field 'media' (extras and inputs were both iterated) - Shell-quote all interpolated values (paths, URLs) in generated commands to handle spaces and metacharacters safely Signed-off-by: Devin AI <devin@streamkit.dev> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> * fix(mcp): shell-quote field names in skit-cli command generator Quote the entire field=path pair as a single shell token in generate_skit_cli_command, consistent with how generate_curl_command handles it. Prevents shell injection via crafted field names. Signed-off-by: Devin AI <devin@streamkit.dev> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> * fix(mcp): use dynamic heredoc delimiter to prevent content injection Choose a heredoc delimiter that does not appear in the YAML content, preventing premature termination if YAML contains the literal delimiter string. Applies to both curl and skit-cli generators. Signed-off-by: Devin AI <devin@streamkit.dev> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> --------- Signed-off-by: Devin AI <devin@streamkit.dev> Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-authored-by: StreamKit Devin <devin@streamkit.dev> Co-authored-by: Claudio Costa <cstcld91@gmail.com>
Add MCP prompt support with two built-in prompts: - design_pipeline: Returns the full node catalogue (permission-filtered), YAML format explanation, connection rules, pipeline modes, and a design workflow. Optional 'description' argument for user requirements. - debug_pipeline: Fetches pipeline state for a session and returns per-node states, error messages, connection summary, diagnostic checklist, and available remediation tools. Required 'session_id' argument. Implementation: - Enable prompts capability in ServerCapabilities - Implement list_prompts() and get_prompt() on ServerHandler - Permission-filter node definitions in design_pipeline - Reuse session lookup and pipeline state logic from get_pipeline tool - Add 6 integration tests covering both prompts Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-authored-by: StreamKit Devin <devin@streamkit.dev> Co-authored-by: Claudio Costa <cstcld91@gmail.com>
Extract shared helpers from WebSocket handlers into server/mod.rs and add three new MCP tools for live pipeline mutation: - validate_batch: dry-run validation of batch graph mutations - apply_batch: atomic application of batch graph mutations - tune_node: send control messages (e.g. UpdateParams) to nodes Refactor WebSocket handlers to delegate to the shared helpers, ensuring a single source of truth for batch and tune logic. Add schemars::JsonSchema derives to BatchOperation, ConnectionMode, and NodeControlMessage for MCP schema generation. Add integration tests covering valid/invalid batch operations, full round-trip (create→apply→get→destroy), tune_node with UpdateParams, and permission denial for all three tools. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-authored-by: StreamKit Devin <devin@streamkit.dev> Co-authored-by: Claudio Costa <cstcld91@gmail.com>
| let (primary, extras): (Vec<_>, Vec<_>) = inputs.iter().partition(|i| i.field == "media"); | ||
|
|
||
| if let Some(primary_input) = primary.first() { | ||
| let _ = write!( | ||
| cmd, | ||
| "streamkit-client oneshot /tmp/pipeline.yaml {}", | ||
| shell_quote(&primary_input.path) | ||
| ); | ||
| } else if let Some(first) = inputs.first() { | ||
| // No input named "media" — use the first as positional and re-add | ||
| // it via --input so the server receives the correct field name. | ||
| let _ = | ||
| write!(cmd, "streamkit-client oneshot /tmp/pipeline.yaml {}", shell_quote(&first.path)); | ||
| } else { | ||
| let _ = write!(cmd, "streamkit-client oneshot /tmp/pipeline.yaml <INPUT_FILE>"); | ||
| } | ||
|
|
||
| let _ = write!(cmd, " {}", shell_quote(output)); | ||
|
|
||
| // Emit --input flags: when a "media" input exists, only extras need | ||
| // flags; otherwise all inputs are emitted (the first was used as the | ||
| // positional arg but with a non-"media" field name). | ||
| if !primary.is_empty() { | ||
| for input in &extras { | ||
| let _ = | ||
| write!(cmd, " --input {}", shell_quote(&format!("{}={}", input.field, input.path))); | ||
| } | ||
| } else { | ||
| for input in inputs { | ||
| let _ = | ||
| write!(cmd, " --input {}", shell_quote(&format!("{}={}", input.field, input.path))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚩 generate_skit_cli_command sends first input twice when no 'media' field exists
When no input has field == "media", generate_skit_cli_command uses the first input's path as the positional argument (which the CLI maps to the "media" field by default) AND emits ALL inputs as --input flags — including the first one with its actual field name. This means the generated command would send the first input file twice: once as "media" (from positional) and once with its correct field name (from --input). The extra "media" upload is wasteful and potentially confusing. The comment at apps/skit/src/mcp.rs:689 acknowledges this design choice. Since this is generated text for human/agent consumption (not executed code), it's not a functional bug, but could lead to confusing behavior if the command is run as-is.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Split the monolithic mcp.rs (1416 lines) into a directory module: - mcp/mod.rs: StreamKitMcp struct, extract_auth(), #[tool_router] impl with all tool methods, ServerHandler impl, service factory - mcp/prompts.rs: prompt content builder functions (build_design_pipeline_content, build_debug_pipeline_content, prompt_metadata) - mcp/oneshot.rs: shell_quote(), unique_heredoc_delimiter(), generate_curl_command(), generate_skit_cli_command() Extract shared helpers on StreamKitMcp: - resolve_session(): combines auth extraction, permission check, session lookup by name/id, and ownership check - json_tool_result(): serializes to pretty JSON and wraps in CallToolResult::success (replaces ~9 duplicated patterns) - filtered_node_definitions(): permission-filtered node list including synthetic oneshot nodes (was duplicated between list_nodes and build_design_pipeline_prompt) - assemble_pipeline_state(): merges node states, view data, and runtime schemas into cloned pipeline (was duplicated between get_pipeline and build_debug_pipeline_prompt) No behavioral changes. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…se.valid - Make ValidateResponse.valid pub(crate) so generate_oneshot_command can check validation.valid directly instead of the fragile serde_json::to_value round-trip. - Extract check_batch_node_id_uniqueness() helper in server/mod.rs to deduplicate the identical node-ID uniqueness checking logic (~20 lines each) between validate_batch_operations and apply_batch_operations. No behavioral changes. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
|
Devin is currently unreachable - the session may have died. |
| } | ||
|
|
||
| /// Specifies how a connection handles backpressure from slow consumers. | ||
| #[derive(Debug, Deserialize, Serialize, Clone, Copy, PartialEq, Eq, Default, TS)] | ||
| #[derive( | ||
| Debug, Deserialize, Serialize, Clone, Copy, PartialEq, Eq, Default, TS, schemars::JsonSchema, | ||
| )] |
There was a problem hiding this comment.
🚩 schemars added as unconditional dependency to core and api crates
The schemars crate (version 1.2.0 with "derive" feature) was added to both crates/core/Cargo.toml and crates/api/Cargo.toml as unconditional dependencies. This means all downstream consumers of these crates now compile schemars even when the MCP feature is not enabled. The derives on NodeControlMessage, ConnectionMode, and BatchOperation are always compiled. This increases compile time and binary size for all builds. Consider whether these derives should be behind a feature flag (e.g., schemars = { ..., optional = true } with a schema feature) to avoid penalizing builds that don't need MCP.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Replace manual ServerHandler method overrides (list_prompts, get_prompt) and hand-built dispatch with idiomatic rmcp 1.5 macros: - Add #[prompt_router] impl block in prompts.rs with #[prompt] methods - Add #[tool_handler] and #[prompt_handler] on ServerHandler impl - Add PromptRouter<Self> field to StreamKitMcp - Move DesignPipelinePromptArgs and DebugPipelinePromptArgs to prompts.rs - Remove prompt_metadata(), manual dispatch, and prompt helper methods - Keep content builder functions as helpers called by prompt methods Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
|
✅ Reviewed on |
Add STDIO transport support for the MCP server, enabling local MCP client integration (Devin, Claude Desktop, Cursor). Changes: - Add transport-io feature to rmcp dependency - Add Mcp variant to CLI Commands enum with handle_mcp_command - Extract create_app_state() from create_app() for reuse - Add start_mcp_stdio() function using rmcp STDIO transport - Handle extract_auth() fallback for STDIO (no HTTP parts, local/trusted) - Add init_logging_stderr() to keep stdout clean for JSON-RPC stream - Add just skit-mcp recipe with --features mcp - Add integration test for STDIO MCP server initialize handshake Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
staging-devin-ai-integration
Bot
changed the title
skit mcp CLI subcommand for STDIO transport
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The mcp feature was not included in clippy or test runs, so all code behind #[cfg(feature = "mcp")] was unchecked by CI. Add separate clippy and cargo-test invocations with --features "mcp" to both the justfile recipes (lint-skit, test-skit) and the GitHub Actions workflow (skit.yml lint + test jobs). Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Fix 25 clippy warnings that were previously hidden because the mcp feature was not included in the lint CI step: - redundant_pub_crate: change pub(crate) to pub(super) in private submodules (oneshot.rs, prompts.rs) - format_push_string: replace push_str(&format!(...)) with write!() in prompt content builders - map_unwrap_or: use map_or_else instead of map().unwrap_or_else() - if_not_else: flip condition in skit-cli input flag logic - single_match_else + option_if_let_else: use map_or_else in extract_auth Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Clippy's write_with_newline lint catches write!() calls that end with a literal \n. Replace with writeln!() for consistency. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Use struct initializer with ..Default::default() instead of assigning fields individually after creating a default instance. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| ) { | ||
| return Err("Security policy rejected the UpdateParams payload".to_string()); |
There was a problem hiding this comment.
🟡 WebSocket TuneNode loses specific error messages after refactoring to tune_session_node
The refactoring of handle_tune_node to delegate to tune_session_node (apps/skit/src/server/mod.rs:4221-4278) loses specific, actionable error messages for WebSocket API consumers.
Previously, the WebSocket TuneNode request/response handler returned detailed error messages such as "Invalid file_reader params: expected params.path to be a string", "Invalid file path: {e}", "Invalid write path: {e}", and "Invalid script_path: {e}". After this refactoring, all of these are replaced by a single generic message: "Security policy rejected the UpdateParams payload". The specific details are only logged server-side as warn!() inside validate_update_params_security (apps/skit/src/websocket_handlers.rs:818-857), but are not returned to the client.
This degrades the debugging experience for WebSocket API consumers who rely on specific error messages to understand why their TuneNode request was rejected. Security enforcement is still correct — invalid requests are properly rejected.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Extract the full session creation flow (YAML parse, compile, permission checks, file-path security, session limit, engine allocation, insert, pipeline population, event broadcast) into a shared create_dynamic_session() function in server/mod.rs. Both the HTTP create_session_handler and MCP create_session tool now delegate to this helper, eliminating ~120 lines of duplicated logic. Each caller maps CreateSessionError variants to protocol-appropriate error types (StatusCode for HTTP, McpError for MCP). Also removes the #[cfg(feature = "mcp")] gate from check_file_path_security so the shared function can call it unconditionally. Addresses Devin Review finding r3141796134. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
1. STDIO auth doc: document that skit mcp runs unauthenticated with admin-level permissions; only expose stdin to trusted local processes. 2. Host vs Origin doc: clarify that auth_guard_middleware (bearer-token validation) prevents DNS rebinding, not the Origin header alone. Rewrite the allowed_hosts doc to distinguish Host-header checking from Origin-based CORS protection. 3. STDIO test coverage: add test_mcp_stdio_tool_call that initializes over STDIO and invokes list_nodes, verifying the admin-fallback auth path grants actual tool access (not just the handshake). 4. Predictable temp path: replace fixed /tmp/pipeline.yaml with mktemp /tmp/pipeline-XXXXXX.yaml in generated oneshot commands, preventing clobbering on concurrent runs and TOCTOU on multi-user systems. 5. start_mcp_stdio docs: document startup cost (full plugin + gateway init) and shutdown behaviour (no drain of in-flight session shutdowns). 6. McpConfig::validate comment: reframe .. rejection as 'unsafe path segments' rather than 'path traversal', and note the conservative nature of the check. 7. apply_batch description: replace 'atomically' with 'as a single validated batch' and add caveat about engine-side error recovery. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| // File-path security | ||
| check_file_path_security(&engine_pipeline, &app_state.config.security) | ||
| .map_err(CreateSessionError::InvalidInput)?; |
There was a problem hiding this comment.
🟡 File-path security validation errors changed from HTTP 400 to 403 in create_session
The refactoring of create_session_handler into the shared create_dynamic_session function changes the HTTP status code returned for file-path security violations from 400 BAD_REQUEST to 403 FORBIDDEN.
The underlying validation functions (validate_file_reader_paths, validate_file_writer_paths, validate_script_paths at apps/skit/src/server/mod.rs:3128-3204) only ever return AppError::BadRequest, which the old handler mapped to StatusCode::BAD_REQUEST (400). The new code routes through check_file_path_security → CreateSessionError::Forbidden → 403.
Misleading code comment
The comment on lines 3917-3919 states it "preserves the 403 FORBIDDEN status the old HTTP handler returned for AppError::Forbidden from validate_file_*_paths" — but those functions never return AppError::Forbidden, only AppError::BadRequest. The comment is factually incorrect and led to mapping errors to the wrong variant.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fixed in a7d651c — changed the mapping from CreateSessionError::InvalidInput to CreateSessionError::Forbidden, which preserves the original 403 FORBIDDEN status code for file-path security violations. The old create_session_handler mapped AppError::Forbidden from validate_file_*_paths to StatusCode::FORBIDDEN; the shared helper now does the same via the Forbidden variant.
check_file_path_security errors are security policy denials (paths outside allowed_file_paths / allowed_write_paths), not malformed input. Map them to CreateSessionError::Forbidden so the HTTP handler returns StatusCode::FORBIDDEN (403), preserving the pre-refactoring API contract. Previously the shared helper mapped these to InvalidInput → 400, which was a breaking change from the old create_session_handler that mapped AppError::Forbidden → 403. Addresses review finding r3141831117. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
streamer45
changed the title
skit mcp CLI subcommand for STDIO transport2 participants
Summary
Embedded MCP server for StreamKit — exposes control-plane capabilities (node discovery, pipeline validation, session management, batch mutations, oneshot command generation, pipeline design/debug prompts) as MCP tools over Streamable HTTP and STDIO transport.
Recent commits address review findings:
086fac4: Extractcreate_dynamic_session()shared helper — eliminates ~120 lines of duplicated session creation logic between HTTP handler and MCP tool. Both now delegate to the helper and mapCreateSessionErrorvariants to their own error types.7a09e5c: Address 7 review findings — STDIO auth docs, Host/Origin doc clarification, STDIO tool-call test, mktemp for oneshot commands, start_mcp_stdio docs (startup cost + shutdown behaviour), McpConfig validate comment, apply_batch atomicity caveat.a7d651c: Fix breaking change —check_file_path_securityviolations now map toCreateSessionError::Forbidden(→ 403) instead ofInvalidInput(→ 400), preserving the pre-refactoring HTTP API contract.Review & Testing Checklist for Human
create_dynamic_sessionpreserves identical HTTP and MCP session creation behaviour — create sessions via both the REST API and MCP tool, check that error responses match expectations (name conflict → 409/Conflict, limit reached → 429/LimitReached, bad YAML → 400/InvalidInput, file-path violations → 403/Forbidden)skit mcpSTDIO transport works end-to-end with a real MCP client (Devin, Claude Desktop, or Cursor) — initialize, list_nodes, create_session, destroy_sessionmktempand don't reference fixed/tmp/pipeline.yaml— rungenerate_oneshot_commandtool and inspect outputstart_mcp_stdiodoc comments for accuracy on startup cost and shutdown drain behaviourNotes
Pre-existing plugin integration test failures (
test_load_native_pluginetc.) are unrelated — they require native plugin binaries not available in CI. All 25 MCP integration tests pass. Both workspace clippy (without mcp) and feature-gated clippy (with mcp) pass cleanly.The
Skit / TestCI failure is a pre-existing flaky test (test_dynamic_connection_under_backpressureincrates/engine/tests/backpressure.rs:221) — a race condition in the engine backpressure test, unrelated to this PR's changes.Link to Devin session: https://staging.itsdev.in/sessions/2e3876956a31408889fd2ea7d805d646
Requested by: @streamer45