From d0b3672bcc1f73197af7083768886961e8344884 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Mon, 27 Jul 2026 18:04:50 +0200 Subject: [PATCH 1/4] docs: sync user-facing docs and MSRV/version refs for v0.9.0 Bump per-crate README dependency snippets and root README's dependency example from 0.8 to 0.9; add breaking-change/removal callouts for the --http/--sse exclusivity change and the removed mcp-execution-runtime npm package. Align CONTRIBUTING.md's MSRV references with the actual 1.91 toolchain and update SECURITY.md's supported-versions table. --- CONTRIBUTING.md | 4 ++-- README.md | 12 +++++++++--- SECURITY.md | 5 ++++- crates/mcp-codegen/README.md | 2 +- crates/mcp-core/README.md | 2 +- crates/mcp-files/README.md | 2 +- crates/mcp-introspector/README.md | 2 +- crates/mcp-skill/README.md | 2 +- 8 files changed, 20 insertions(+), 11 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3139563..2e681bc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -32,7 +32,7 @@ This project adheres to a code of conduct that we expect all contributors to fol ### Prerequisites **Required:** -- Rust 1.89 or higher (Edition 2024) +- Rust 1.91 or higher (Edition 2024) - Cargo (comes with Rust) - Node.js 18+ (for testing generated TypeScript code) - Git @@ -429,7 +429,7 @@ All PRs must pass: - Tests on Linux, macOS, Windows - Tests on stable and beta Rust - Code coverage upload to codecov -- MSRV check (Rust 1.89) +- MSRV check (Rust 1.91) - Security audit (cargo-deny) - Documentation build - Benchmark build diff --git a/README.md b/README.md index 983d40c..e40bf2d 100644 --- a/README.md +++ b/README.md @@ -82,11 +82,14 @@ Or add to `Cargo.toml`: ```toml [dependencies] -mcp-execution-core = "0.8" -mcp-execution-introspector = "0.8" -mcp-execution-codegen = "0.8" +mcp-execution-core = "0.9" +mcp-execution-introspector = "0.9" +mcp-execution-codegen = "0.9" ``` +> [!WARNING] +> The standalone `mcp-execution-runtime` npm package has been removed as of v0.9.0. If you were using it, switch to the TypeScript bridge generated by `mcp-execution-cli generate`, which is actively maintained and receives all security updates. + > [!IMPORTANT] @@ -184,6 +187,9 @@ mcp-execution-cli completions bash > [!TIP] > Use `--from-config` to load server settings from `~/.claude/mcp.json` instead of manual arguments. +> [!IMPORTANT] +> **v0.9 breaking change:** `generate --http ` and `introspect --http ` now require either `--from-config`, a positional command, or `--http`/`--sse`, not a combination. If you were combining a positional command with `--http` or `--sse`, use one or the other exclusively. + ## Performance | Metric | Target | Achieved | diff --git a/SECURITY.md b/SECURITY.md index ee84079..7e5c630 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -4,7 +4,10 @@ | Version | Supported | | ------- | ------------------ | -| 0.6.x | :white_check_mark: | +| 0.9.x | :white_check_mark: | +| 0.8.x | :white_check_mark: | +| 0.7.x | :x: | +| 0.6.x | :x: | ## Reporting a Vulnerability diff --git a/crates/mcp-codegen/README.md b/crates/mcp-codegen/README.md index 48643a2..3153224 100644 --- a/crates/mcp-codegen/README.md +++ b/crates/mcp-codegen/README.md @@ -12,7 +12,7 @@ Progressive loading TypeScript code generation for MCP tools. Achieves **98% tok ```toml [dependencies] -mcp-execution-codegen = "0.8" +mcp-execution-codegen = "0.9" ``` Or with cargo-add: diff --git a/crates/mcp-core/README.md b/crates/mcp-core/README.md index c447da0..477f1c9 100644 --- a/crates/mcp-core/README.md +++ b/crates/mcp-core/README.md @@ -12,7 +12,7 @@ Foundation types, traits, and error handling for MCP Code Execution. ```toml [dependencies] -mcp-execution-core = "0.8" +mcp-execution-core = "0.9" ``` Or with cargo-add: diff --git a/crates/mcp-files/README.md b/crates/mcp-files/README.md index fe5f547..a57fa87 100644 --- a/crates/mcp-files/README.md +++ b/crates/mcp-files/README.md @@ -12,7 +12,7 @@ In-memory virtual filesystem for MCP tools organization and export. ```toml [dependencies] -mcp-execution-files = "0.8" +mcp-execution-files = "0.9" ``` Or with cargo-add: diff --git a/crates/mcp-introspector/README.md b/crates/mcp-introspector/README.md index 36a3fb2..99c1f5e 100644 --- a/crates/mcp-introspector/README.md +++ b/crates/mcp-introspector/README.md @@ -12,7 +12,7 @@ MCP server introspection using the official [rmcp](https://docs.rs/rmcp) SDK. ```toml [dependencies] -mcp-execution-introspector = "0.8" +mcp-execution-introspector = "0.9" ``` Or with cargo-add: diff --git a/crates/mcp-skill/README.md b/crates/mcp-skill/README.md index d223156..f51bf7c 100644 --- a/crates/mcp-skill/README.md +++ b/crates/mcp-skill/README.md @@ -18,7 +18,7 @@ Or add to your `Cargo.toml`: ```toml [dependencies] -mcp-execution-skill = "0.8" +mcp-execution-skill = "0.9" ``` > [!IMPORTANT] From 27d10fdc2fde27fa3bdb2a89dde516ade5bffeec Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Mon, 27 Jul 2026 18:05:03 +0200 Subject: [PATCH 2/4] docs(specs): sync per-crate specifications with changes since v0.8.0 Bring specs/{codegen,core,files,introspector,server,skill}/spec.md back in line with the current code after the type-safety, redaction, and path-confinement work merged since v0.8.0 (ServerId/ToolName/ServerConfig closed-type invariants, Transport enum, ResourceKind/FilesResourceKind, runtime bridge hardening, and related fixes). --- specs/codegen/spec.md | 124 +++++++++++++++++++++++++++++++------ specs/core/spec.md | 15 ++++- specs/files/spec.md | 59 ++++++++++++++++-- specs/introspector/spec.md | 48 ++++++++++++-- specs/server/spec.md | 70 +++++++++++++++++---- specs/skill/spec.md | 38 +++++++++++- 6 files changed, 309 insertions(+), 45 deletions(-) diff --git a/specs/codegen/spec.md b/specs/codegen/spec.md index 049a184..5c87666 100644 --- a/specs/codegen/spec.md +++ b/specs/codegen/spec.md @@ -44,6 +44,12 @@ pub use common::types::{GeneratedCode, GeneratedFile, TemplateContext, ToolDefin pub use progressive::ProgressiveGenerator; pub use template_engine::TemplateEngine; +impl GeneratedCode { + // Returns Error::DuplicateGeneratedFilePath if `file.path` is already present, instead + // of silently overwriting the existing entry (issue #312). + pub fn add_file(&mut self, file: GeneratedFile) -> Result<()>; +} + // mcp_execution_codegen::progressive pub struct ProgressiveGenerator<'a> { /* engine: TemplateEngine<'a> */ } impl<'a> ProgressiveGenerator<'a> { @@ -51,19 +57,29 @@ impl<'a> ProgressiveGenerator<'a> { pub fn generate(&self, server_info: &ServerInfo) -> Result; pub fn generate_with_categories(&self, server_info: &ServerInfo, categorizations: &HashMap) -> Result; } -pub struct ToolCategorization { pub category: String, pub keywords: String, pub short_description: String } -pub struct BridgeContext { pub forbidden_chars: Vec, pub forbidden_env_names: Vec, pub forbidden_env_prefix: String } -// BridgeContext::default() populates from mcp_execution_core::forbidden_chars()/forbidden_env_names()/forbidden_env_prefix() — hand-written, no derive(Default), so it can never render a fail-open (empty) bridge. +pub struct ToolCategorization { pub category: String, pub keywords: Vec, pub short_description: String } + +// BridgeContext's three fields are private with read-only accessor methods of the same +// name; `BridgeContext::default()` is the only construction path (no `pub` fields, no +// `derive(Default)`, no `Deserialize`) — see [[#Runtime bridge]] and issue #315. +pub struct BridgeContext { /* forbidden_chars, forbidden_env_names, forbidden_env_prefix: private */ } +impl BridgeContext { + pub fn forbidden_chars(&self) -> &[String]; + pub fn forbidden_env_names(&self) -> &[String]; + pub fn forbidden_env_prefix(&self) -> &str; +} +impl Default for BridgeContext { /* populates from mcp_execution_core::forbidden_chars()/forbidden_env_names()/forbidden_env_prefix() — hand-written, so it can never render a fail-open (empty) bridge */ } pub const MAX_GENERATED_FILES: usize; // = introspector::MAX_TOOL_COUNT + 5 (fixed files) pub const MAX_GENERATED_BYTES: usize; // = 2 * MAX_TOOL_COUNT * (MAX_TOOL_NAME_LEN + MAX_TOOL_DESCRIPTION_LEN + MAX_SCHEMA_SIZE_BYTES) // mcp_execution_codegen::common::typescript +pub const MAX_SCHEMA_RECURSION_DEPTH: usize = 128; // see [[#Recursion Depth Bound]] pub fn to_camel_case(s: &str) -> String; pub fn to_pascal_case(s: &str) -> String; pub fn sanitize_ts_identifier(s: &str) -> String; // collapses invalid-char runs to one '_'; prefixes '_' if leading digit/empty pub fn json_type_to_typescript(json_type: &str) -> &'static str; -pub fn json_schema_to_typescript(schema: &serde_json::Value) -> String; +pub fn json_schema_to_typescript(schema: &serde_json::Value) -> String; // depth-capped at MAX_SCHEMA_RECURSION_DEPTH pub fn extract_properties(schema: &serde_json::Value) -> Vec; // mcp_execution_codegen::template_engine @@ -84,15 +100,27 @@ of the sanitizers documented below, run **before** rendering. `generate`/`generate_with_categories` accept `&ServerInfo` (from [[../introspector/spec]]) and an optional `HashMap` -keyed by raw tool name (`ToolCategorization.category`/`.keywords` -comma-separated/`.short_description` — supplied by Claude via -`mcp-server`'s `save_categorized_tools`, or absent for the CLI's plain -`generate` command). `generate` delegates to `generate_with_categories` with +keyed by the tool's **raw** name (`ToolInfo.name`), not its +display-sanitized `typescript_name` — every lookup in +`generate_with_categories` indexes the map by `tool.name.as_str()` before any +sanitization, so a `categorizations` map keyed by a display name would +silently miss. `ToolCategorization.keywords` is a `Vec` (not a +comma-joined `String`); JSDoc rendering joins it via the internal +`render_keywords_for_jsdoc` helper (`keywords.join(", ")`, then +`sanitize_jsdoc`) whenever a display string is needed — supplied by Claude +via `mcp-server`'s `save_categorized_tools`, or absent for the CLI's plain +`generate` command. `generate` delegates to `generate_with_categories` with an **empty** map — this must produce byte-identical `index.ts` category behavior to "no categorization at all" (regression-tested: an empty-but- `Some` map must not synthesize a spurious "uncategorized" `CategoryInfo` group). +`ToolContext::short_description` (populated for each tool's JSDoc header) is +a plain `String`, not `Option`: `create_tool_context` always falls +back to the tool's own (sanitized) `description` when no categorization +short description is supplied, so the type reflects that a tool file's +header always has a short description to render. + ## 4. Output: Generated File Set For a server with N tools, exactly `N + 5` files: @@ -106,6 +134,16 @@ For a server with N tools, exactly `N + 5` files: | `tsconfig.json` | `target: ES2022`, `module`/`moduleResolution: NodeNext`, `strict: true`, `noEmit: true`, `allowImportingTsExtensions: true`, `skipLibCheck: true`, `types: ["node"]` | | `_meta.json` | `mcp_execution_core::metadata::ServerMetadata` (schema_version, server_id/name/version, per-tool metadata incl. **raw, unsanitized** parameter descriptions) | +Each tool's `{Name}Params` is emitted as a `type` alias, not an `interface` — +only a `type` alias gets the implicit `Record`-compatible +index signature `callMCPTool`'s parameter requires; an `interface` is not +structurally assignable there. Each tool's `{Name}Result` is +`Record | unknown[] | string` — a union, not a bare +`interface { [key: string]: unknown }` — reflecting that `callMCPTool` +resolves to a parsed JSON object/array, plain text, or (for a non-text +content item) the content object itself; callers must narrow the type +before accessing a field on the result. + `package.json`/`tsconfig.json` are regenerated on every `generate` call — documented as **read-only, not meant to be extended** (e.g. via `tsconfig.json`'s `"extends"`, which would silently inherit `noEmit: true` @@ -136,16 +174,55 @@ into a consumer's own build). sibling property names that sanitize to the same identifier (e.g. `a-b` and `a.b` both → `a_b`) — the second becomes `a_b_2`. -## 6. Injection Defense (Sanitization Pipeline) +## 6. Recursion Depth Bound + +`common::typescript::MAX_SCHEMA_RECURSION_DEPTH` (`= 128`) bounds how far +`json_schema_to_typescript` and the JSDoc description sanitizer +(`sanitize_schema_jsdoc_descriptions`/`sanitize_schema_jsdoc_value` in +`progressive::generator`) will descend into a nested `object`/`array` +schema before treating the remaining branch as opaque (`unknown`/ +`unknown[]`, or leaving a `description` unsanitized) rather than recursing +further. Both functions log a single `tracing::warn!` per call the first +time any branch trips the cap (not once per clipped branch). + +This is **defense-in-depth for direct callers of these two `pub` functions +with a hand-built `serde_json::Value`**, not a fix for a reachable +wire-path denial of service: a schema arriving over the wire from an MCP +server's `tools/list` response is deserialized by `serde_json`, which +enforces its own default recursion limit (128) before +`mcp-execution-introspector` ever constructs a `ToolInfo` — that ceiling +caps *reachable* nesting at 122 levels for an array-shaped schema and ~61 +levels for `properties`-nested objects, well under this constant's value, +so no schema that actually clears introspection is ever clipped here. `128` +is chosen to sit at that wire-path ceiling and is coupled to `serde_json`'s +own default staying at 128; a future change to either needs re-verifying +this property. + +This cap does **not** cover the rest of the generation pipeline's other +unconditionally-recursive touches on the same schema — e.g. +`create_tool_context`'s `tool.input_schema.clone()`, the schema's later +re-serialization and Handlebars rendering, or `serde_json::Value`'s own +recursive `Drop` impl. + +## 7. Injection Defense (Sanitization Pipeline) All of the following run **before** any Handlebars render, and are the actual injection-safety mechanism given `no_escape` is set: | Function | Neutralizes | Applied to | |---|---|---| -| `sanitize_jsdoc(s, max_len)` | `*/` (JSDoc comment terminator, escaped to `*\/`), all newline-class chars (incl. U+2028/U+2029) flattened to space, truncated to `max_len` chars | tool/server descriptions, categories, keywords rendered into `.ts` JSDoc comments | +| `sanitize_jsdoc(s, max_len)` | Every control character (C0, DEL, C1 — everything `char::is_control` reports — plus U+2028/U+2029, which ECMAScript treats as line terminators) is *replaced with a space* by delegating to `mcp_execution_core::untrusted::sanitize_untrusted_text`; **then** `*/` (JSDoc comment terminator) is escaped to `*\/`; truncation to `max_len` chars runs last | tool/server descriptions, categories, keywords rendered into `.ts` JSDoc comments | | `sanitize_ts_string_literal(s)` | backslash, single-quote, `\r`/`\n`, U+2028/U+2029 | tool name / server id embedded as single-quoted TS string literals (`callMCPTool('{{{server_id_literal}}}', ...)`) | -| `sanitize_schema_jsdoc_descriptions(value)` | recursively applies `sanitize_jsdoc` to every `"description"` key in the input JSON Schema before it's embedded in a tool's JSDoc | `input_schema` field of `ToolContext` | +| `sanitize_schema_jsdoc_descriptions(value)` | recursively applies `sanitize_jsdoc` to every `"description"` key in the input JSON Schema before it's embedded in a tool's JSDoc, up to [[#Recursion Depth Bound]] | `input_schema` field of `ToolContext` | + +> [!warning] +> `sanitize_jsdoc`'s ordering is load-bearing: control-character +> neutralization must run **before** the `*/` escape step. A control +> character sitting between `*` and `/` would otherwise prevent the `*/` +> match during escaping, and neutralizing/removing it afterward could +> collapse the two characters back together into a live, comment-closing +> `*/` — reopening the JSDoc block comment (issue #300). Replacing with a +> space (not deleting) also avoids gluing adjacent words together. > [!warning] > The `_meta.json` sidecar deliberately uses the **raw, unsanitized** MCP @@ -154,7 +231,7 @@ actual injection-safety mechanism given `no_escape` is set: > would only lose fidelity (regression-tested against issue #141, where the > old regex-based parser could not recover parameter descriptions at all). -## 7. Runtime Bridge (`_runtime/mcp-bridge.ts`) +## 8. Runtime Bridge (`_runtime/mcp-bridge.ts`) Generated once per server (identical content regardless of tool count, except for the forbidden-char/env-name lists rendered from `BridgeContext`). @@ -186,7 +263,7 @@ Responsibilities: `structuredContent`) success response is currently indistinguishable from a misbehaving server and is surfaced as a thrown error. -## 8. Resource-Exhaustion Bounds (CWE-400) +## 9. Resource-Exhaustion Bounds (CWE-400) - `enforce_tool_count_bound` rejects before any per-tool rendering if `tools.len() + 5 > MAX_GENERATED_FILES`. @@ -203,7 +280,7 @@ Responsibilities: deterministically rejected here for simply being "as large as introspection already allows." -## 9. Error Conditions +## 10. Error Conditions | Condition | `Error` variant | |---|---| @@ -212,9 +289,10 @@ Responsibilities: | Malformed property schema (`name`/`type` not a string) | `ValidationError` | | Handlebars render failure | `SerializationError` (message embeds Handlebars' own error text) | | `_meta.json` serialization failure | `SerializationError` | +| `GeneratedCode::add_file` called with a `path` already present in the collection | `DuplicateGeneratedFilePath { path }` — the original entry is left untouched, not overwritten; defense-in-depth once `resolve_typescript_names` already seeds its own collision set with this generator's reserved output filenames, not a path this generator expects to hit in practice (issue #312) | | Any per-tool failure above | Re-wrapped as `ScriptGenerationError { tool, message, source: Some(...) }` via `wrap_tool_generation_error`, preserving the original error's own classification (e.g. a wrapped `ResourceLimitExceeded` still reports as such downstream — see [[../cli/spec#classify_core_error]]) | -## 10. Cross-Crate Contracts +## 11. Cross-Crate Contracts - **Consumes**: `mcp-core::metadata`/error types/forbidden-char constants; `mcp-introspector::{ServerInfo, ToolInfo}` and its `MAX_TOOL_COUNT`/ @@ -227,7 +305,7 @@ Responsibilities: - **Produced for** `mcp-skill`/`mcp-server`: the `_meta.json` sidecar (schema owned by `mcp-core::metadata`). -## 11. Edge Cases & Notable Behaviors +## 12. Edge Cases & Notable Behaviors - A tool name containing only invalid identifier characters (e.g. all non-ASCII) sanitizes to a bare `_`, then gets disambiguated if colliding. @@ -239,8 +317,18 @@ Responsibilities: two independent nested objects can each reuse the same disambiguated name without conflict (verified by `test_disambiguate_identifier_reuses_base_across_independent_scopes`). - -## 12. See Also +- `sanitize_ts_identifier` collapses an entire run of consecutive + invalid/underscore-producing characters into a single `_`, not one `_` + per character — a name like `café_menu_日本語` sanitizes to `caf_menu_`, + and regenerating a server whose tools have such names can produce + different identifiers than an older generated package did. +- `ProgressiveGenerator::create_tool_context`/`create_tool_metadata` both + consume a single `extract_property_data` call per tool rather than + extracting twice — the JSDoc-sanitized `PropertyInfo` half feeds the + `.ts` template, the raw-description half feeds `_meta.json`, sharing one + schema walk (issue #295). + +## 13. See Also - [[../introspector/spec]] — source of `ServerInfo`/`ToolInfo` - [[../files/spec]] — consumer of `GeneratedCode` diff --git a/specs/core/spec.md b/specs/core/spec.md index d6a749b..bdc2a47 100644 --- a/specs/core/spec.md +++ b/specs/core/spec.md @@ -95,6 +95,7 @@ pub enum Error { ValidationError { field: String, reason: String }, ScriptGenerationError { tool: String, message: String, source: Option> }, ResourceLimitExceeded { resource: ResourceKind, actual: usize, limit: usize }, + DuplicateGeneratedFilePath { path: String }, } pub type Result = std::result::Result; @@ -119,12 +120,22 @@ via `mcp-execution-codegen`), so sharing this enum would mean adding a new direc `mcp-core` for a single error variant — see [[../files/spec#7. Error Conditions]]. Each variant has an `is_*` predicate (`is_connection_error`, `is_security_error`, `is_timeout`, `is_validation_error`, -`is_script_generation_error`, `is_resource_limit_exceeded`). No predicate -exists for `InvalidArgument`/`SerializationError` — callers match directly. +`is_script_generation_error`, `is_resource_limit_exceeded`, +`is_duplicate_generated_file_path`). No predicate exists for +`InvalidArgument`/`SerializationError` — callers match directly. `ScriptGenerationError.source` is the vehicle for preserving an inner `Error`'s own classification through wrapping (see `mcp-cli`'s `classify_core_error`, which recurses into it — [[../cli/spec]]). +`Error::DuplicateGeneratedFilePath { path: String }` (issue #312) is raised by +`mcp-execution-codegen`'s `GeneratedCode::add_file` when a second file is added at a path +already present in the same generated-code collection (e.g. a sanitized tool name colliding +with a generator's own reserved output filename like `index`) — a silent overwrite that used +to lose a generated file with no signal to the caller now fails loudly instead. `add_file`'s +signature changed from `fn add_file(..)` to `fn add_file(..) -> Result<()>` accordingly; every +in-tree call site (`mcp-codegen`'s own generator, `mcp-files`'s `FilesBuilder`) was updated to +handle the new `Result`. + ### `ServerConfig` / `ServerConfigBuilder` / `Transport` (`src/server_config.rs`) ```rust diff --git a/specs/files/spec.md b/specs/files/spec.md index 386f8e6..ad404bd 100644 --- a/specs/files/spec.md +++ b/specs/files/spec.md @@ -72,7 +72,12 @@ impl FileSystem { pub fn export_to_filesystem_parallel(&self, base_path: impl AsRef) -> Result<()>; } -pub struct ExportOptions { pub atomic: bool } // default: atomic=true +pub struct ExportOptions { pub atomic: bool, confine_to: Option } // atomic default true; confine_to default None +impl ExportOptions { + pub const fn new() -> Self; + pub const fn with_atomic_writes(mut self, atomic: bool) -> Self; + pub fn with_confine_to(mut self, base_dir: impl Into) -> Self; +} pub struct FilesBuilder { /* vfs: FileSystem, errors: Vec */ } impl FilesBuilder { @@ -113,7 +118,16 @@ single-file "group" swap the shared base directory wholesale in 1. `check_export_bounds()` — file count ≤ `MAX_EXPORT_FILES`, total content bytes ≤ `MAX_EXPORT_BYTES` (CWE-400). -2. `stage_export(target)`: +2. `stage_export(target, confine_to)`: + - Confirms `target`'s parent directory exists. + - If `options.confine_to` is set (via `ExportOptions::with_confine_to`), + canonicalizes both the parent and `confine_to` and rejects the export + with `FilesError::PathEscapesBase` unless the canonicalized parent + starts with the canonicalized `confine_to` — see + [[#Confinement check (`with_confine_to`)]]. A canonicalization failure + on either path (missing directory, permission denied) is a plain + `FilesError::IoError`, not `PathEscapesBase`, since the confinement + check itself never ran. - Sweeps stale sibling artifacts from a **previous crashed** export of the *same* target (`sweep_stale_artifacts`, age-gated — see [[#Stale-artifact sweep]]). @@ -157,6 +171,21 @@ name-only match could delete a concurrent in-flight export's staging *or* displaced-backup directory, defeating rollback and permanently losing the target. +### Confinement check (`with_confine_to`) + +`ExportOptions::with_confine_to(base_dir)` is opt-in, defense-in-depth +against a caller-built `target` that was assembled by joining untrusted +input (e.g. a server id) onto a base directory: `PathBuf::join` silently +discards `base_dir` entirely if the joined component is absolute, and a +`..`-bearing component can walk back out of it even for a relative join. +Only `export_to_filesystem_with_options` accepts `ExportOptions`, so this +check is unavailable through `export_to_filesystem` (which always uses +`ExportOptions::default()`, `confine_to: None`) or +`export_to_filesystem_parallel` (which passes `None` directly to +`stage_export`). `mcp-cli`'s `generate` command wires this in as a second +layer behind its primary guard (sanitizing the server-id-derived directory +name) — see [[../cli/spec#generate]]. + ### Non-goals / accepted gaps - Concurrent exports of the **same** `base_path` from different processes @@ -232,6 +261,14 @@ avoids that coupling. Each variant's `Display` reproduces the same wording `chec used to build by hand (`"export file count"` / `"export total size"`), so `FilesError::ResourceLimitExceeded`'s message is unchanged in substance. +`PathEscapesBase` is returned only by `export_to_filesystem_with_options` when +`ExportOptions::with_confine_to` is set and the confinement check fails (see +[[#Confinement check (`with_confine_to`)]]); `path` is the canonicalized +export-target parent, `base` the canonicalized confinement directory. A +canonicalization failure on either side is `FilesError::IoError` instead — +`PathEscapesBase` specifically means "both paths resolved, and the target +is outside the base" (#311). + ## 8. Cross-Crate Contracts - **Consumes** `mcp-codegen::GeneratedCode`/`GeneratedFile`; derives @@ -245,8 +282,12 @@ used to build by hand (`"export file count"` / `"export total size"`), so `vfs.export_to_filesystem(&output_dir)` inside `spawn_blocking`, guarded by a per-`output_dir` lock — see [[../server/spec#save_categorized_tools]]). - **Used by** `mcp-cli generate` (via - `FilesBuilder::from_generated_code(code, "/").build_and_export(base_dir)` - — see [[../cli/spec#generate]]). + `FilesBuilder::from_generated_code(code, "/").build()` then + `vfs.export_to_filesystem_with_options(output_path, &ExportOptions::new().with_confine_to(base_dir))` + — one server's tree per call, not the shared-root `build_and_export` path, + with the confinement check as a second defense-in-depth layer behind + sanitizing the server-id-derived directory name — see + [[../cli/spec#generate]]). ## 9. Edge Cases & Notable Behaviors @@ -265,9 +306,17 @@ used to build by hand (`"export file count"` / `"export total size"`), so exact same staging/atomic-rename mechanism, writing files via `rayon` instead — faster for >50 files, may not preserve write order (order doesn't matter for a flat file tree). +- `vfs_to_disk_path`'s defense-in-depth `..`-traversal check (a safety net + behind `FilePath::new`'s own validation) used to `assert!` on a match, + panicking the whole process if a `..` ever reached it. It now returns + `Result`, surfacing `FilesError::InvalidPathComponent` + instead. All three call sites — `collect_directories`, `write_files`, and + `export_to_filesystem_parallel`'s own inline call inside its `rayon` + closure — propagate this via `?` (#318), so a bug that would have reached + this check is now a normal `Err`, not a crash. ## 10. See Also - [[../codegen/spec]] — source of `GeneratedCode` and derived resource bounds - [[../server/spec#Per-resource locking]] — higher-layer serialization for concurrent exports to the same target -- [[../cli/spec#generate]] — CLI-side consumer via `build_and_export` +- [[../cli/spec#generate]] — CLI-side consumer, via `export_to_filesystem_with_options` with `ExportOptions::with_confine_to` diff --git a/specs/introspector/spec.md b/specs/introspector/spec.md index c118ceb..1805718 100644 --- a/specs/introspector/spec.md +++ b/specs/introspector/spec.md @@ -67,6 +67,13 @@ derived budgets proportionally without touching their own formulas. ## 3. Discovery Flow (`discover_server`) +The whole method carries a `#[tracing::instrument(skip_all, fields(server_id = +%server_id))]` span (issue #211): every `tracing` call made during the steps +below runs inside that span, so under the default `fmt` subscriber each log +line gets a `discover_server{server_id=...}:` prefix. No existing log +message's text changed — only this added span context, which lets +concurrently-discovered servers' log output be correlated. + 1. `validate_server_config(config)` — defense in depth, even though a builder-constructed `ServerConfig` is already validated (see [[../core/spec#Defense in depth]]). @@ -83,6 +90,14 @@ derived budgets proportionally without touching their own formulas. with caller headers converted to `http::HeaderValue`/`HeaderName`. **No response-size bound exists on this path** — see [[#Known gap HTTP response size]]. + + Both branches converge on a single private `connect_and_list_tools` + helper, generic over the connect future each transport builds (issue + #294): `discover_via_stdio` and `discover_via_http` used to copy-paste the + same connect/list-tools/timeout/error-mapping pipeline for their shared + `RunningService` client, and now both delegate to it. + Steps 3–5 below are that shared pipeline — timeout durations, error + types/messages, and success output are unchanged from before the dedup. 3. `client.serve(transport)` bounded by `config.connect_timeout()` → `Error::Timeout { operation: "connect to {id}", .. }` on expiry. 4. `list_tools_bounded(&client)` — pages via `list_tools`, bailing out as @@ -92,14 +107,28 @@ derived budgets proportionally without touching their own formulas. Bounded overall by `config.discover_timeout()` → `Error::Timeout { operation: "list_all_tools for {id}", .. }`. 5. `extract_peer_meta` — pulls server name/version/capability flags from the - handshake `InitializeResult`; falls back to - `config.command`/`config.url()` and `"unknown"` version if the server - sent no peer info. + handshake `InitializeResult` into a private `PeerMeta` struct + (`server_name`, `server_version`, `has_resources`, `has_prompts`), + replacing a positional `(String, String, bool, bool)` tuple whose two + trailing `bool`s could previously be transposed without a type error + (issue #207); falls back to `config.command`/`config.url()` and + `"unknown"` version if the server sent no peer info. The tool list and + `PeerMeta` are combined into a private `DiscoveryResult { tools, peer_meta + }`, also replacing a positional tuple, and threaded from + `discover_via_stdio_process`/`discover_via_stdio`/`discover_via_http` up + through `discover_server`. 6. `build_server_info` → per-tool `build_tool_info`, enforcing `MAX_TOOL_NAME_LEN`/`MAX_TOOL_DESCRIPTION_LEN`/`MAX_SCHEMA_SIZE_BYTES` and the overall `MAX_TOOL_COUNT` — returns `Error::ResourceLimitExceeded` on - any violation, naming the specific tool. -7. Cache result in `self.servers`, return `ServerInfo`. + any violation, naming the specific tool. Also enforces + [`ToolName::new`]'s invariant on every tool's `name` — see the error table + below. +7. Cache result in `self.servers`, keyed by the just-built `ServerInfo`'s own + `info.id.clone()` — **not** the `server_id` parameter threaded through + steps 1–6 — so the map key is structurally derived from the value's own + identity and cannot drift from it, even though both are sourced from the + same identifier in every call today (issue #317, no public API change). + Return `ServerInfo`. ## 4. Response-Line Bounding (stdio) @@ -147,6 +176,7 @@ part of `rmcp`'s HTTP transport client-side; `rmcp` 3.0.0-beta.2 adds a | `tools/list` exceeds `discover_timeout` | `Timeout { operation: "list_all_tools for {id}", duration_secs }` | | Accumulated tool count > `MAX_TOOL_COUNT` during paging | `ResourceLimitExceeded { resource: ResourceKind::ToolCount { server_id }, .. }` | | Single tool's name/description/schema exceeds its bound | `ResourceLimitExceeded { resource: ResourceKind::ToolNameLength \| DescriptionLength { tool_name } \| InputSchemaSize { tool_name } \| OutputSchemaSize { tool_name }, .. }` | +| Tool name fails `ToolName::new`'s invariant (e.g. contains `/`, empty) | `ValidationError { field: "tool name", reason }` — hard-fails the *entire* `discover_server` call via `?`-propagation, exactly like an oversized name/description/schema above: a single malformed tool name is not skipped-with-a-warning while the rest of the server's tools are returned (#287) | | HTTP header name/value invalid (introspection-time) | `ConnectionFailed` (header construction failure) | ## 7. Edge Cases & Notable Behaviors @@ -166,6 +196,14 @@ part of `rmcp`'s HTTP transport client-side; `rmcp` 3.0.0-beta.2 adds a the running total over `MAX_TOOL_COUNT` is fetched — at most one page's worth of tools beyond the limit is ever held in memory at once, not the server's entire (potentially huge) full response. +- `ServerInfo`/`ToolInfo` derive `Deserialize` and hold `ServerId`/`ToolName` + fields directly. Both newtypes are constructible only through their + fallible `new`, enforced even through their `#[serde(try_from = "String")]`- + backed `Deserialize` impl (see [[../core/spec]]) — so deserializing a + `ServerInfo`/`ToolInfo` from untrusted JSON with a hostile `id`/tool `name` + (e.g. containing `..` or a path separator) now fails outright instead of + silently producing an unvalidated value, closing a gap that predates this + invariant. ## 8. Cross-Crate Contracts diff --git a/specs/server/spec.md b/specs/server/spec.md index d09f431..406ed45 100644 --- a/specs/server/spec.md +++ b/specs/server/spec.md @@ -137,6 +137,13 @@ introspect→categorize→save workflow. wrapped via `mcp_execution_core::untrusted::wrap_untrusted_block` (`wrap_introspect_result`) since the tool summaries it contains are server-reported, attacker-controlled text now shown to Claude. +- Errors from building the `ServerConfig` or from discovery are classified via + `caller_or_internal_error`: a `ValidationError` **or** `SecurityViolation` + (shell metacharacters, a forbidden env var, etc.) reports as `invalid_params` + — the caller's fault — while anything else reports as `internal_error`. + `SecurityViolation` previously fell through to `internal_error`, misreporting + hostile caller input as a server-side fault; it is now handled identically to + `ValidationError`. ### `save_categorized_tools` @@ -145,18 +152,37 @@ introspect→categorize→save workflow. 1. `state.take(session_id)` — session must exist and not be expired, or `invalid_params`. -2. Validates `categorized_tools` against the session's own introspected - tool names, **sanitized the same way `introspect_server` sanitized them - for Claude** (`sanitize_untrusted_text`) — so a well-behaved caller - echoing back exactly what it was shown never fails a raw-vs-sanitized - name mismatch. Rejects: more entries than `min(introspected count, - MAX_TOOL_FILES)` (reusing `mcp_execution_skill::MAX_TOOL_FILES` so this - stage can never generate more tool files than `generate_skill` will - later accept); a name not in the introspected set; a duplicate name; - any field (`name`/`category`/`keywords`/`short_description`) over its - own byte cap (`MAX_CATEGORIZED_TOOL_NAME_LEN`=128, - `MAX_CATEGORY_LEN`=100, `MAX_KEYWORDS_LEN`=500, - `MAX_SHORT_DESCRIPTION_LEN`=320). +2. Builds a display-name→raw-name lookup (`display_to_raw`) before validating + any entry, since a caller can only ever echo back the *display* form of a + tool name `introspect_server` showed it, never the raw one. For each + introspected tool, both plausible display forms are computed (`display_forms`): + the fully escaped form actually shown (`sanitize_untrusted_text` followed by + `&`/`<`/`>` → `&`/`<`/`>` entity-escaping, mirroring + `wrap_untrusted_block`'s own escaping) and the same text with those entities + decoded back, since `wrap_untrusted_block`'s preamble explicitly invites the + reader to do so. If two **distinct** raw tool names collide on the same + display key under either form, that key is dropped from the lookup + entirely — genuinely ambiguous, so a caller using it hits "not found" rather + than silently having one raw tool's categorization misattributed to another's. + Each `categorized_tools` entry's `name` is resolved through this lookup to a + raw tool name once; both the duplicate check and the codegen categorization + map are keyed by that **resolved raw name**, not the submitted string. This + fixes a categorization-lookup desync (issue #307): an earlier version built + the categorization map keyed by the submitted display string while codegen + looked up tools by their raw name, silently dropping category/keywords/ + description for any tool name containing a control character, line + terminator, or `&`/`<`/`>`. Rejects: more entries than `min(introspected + count, MAX_TOOL_FILES)` (reusing `mcp_execution_skill::MAX_TOOL_FILES` so this + stage can never generate more tool files than `generate_skill` will later + accept — bounded by the true introspected tool count, not by the lookup's + size, since one raw tool can legitimately own two display keys and an + ambiguous key is excluded from the map); a name that doesn't resolve to any + raw tool (unknown, or an ambiguous display key); a name that resolves to a + raw tool a previous entry in the same call already claimed; any field + (`name`/`category`/`keywords`/`short_description`) over its own byte cap + (`MAX_CATEGORIZED_TOOL_NAME_LEN`=128, `MAX_CATEGORY_LEN`=100, + `MAX_KEYWORDS_LEN`=500, `MAX_SHORT_DESCRIPTION_LEN`=320 — each checked via a + single private `check_categorized_field_length` helper called once per field). 3. `ProgressiveGenerator::generate_with_categories` → `FilesBuilder::from_generated_code(code, "/")` → `vfs.file_count()` captured. 4. **Resolves `output_dir` fresh, right here** — not from any value cached on the session — via `output_dir::resolve_output_dir` (see @@ -261,6 +287,14 @@ grows unboundedly; an unconditional `remove` keyed by value alone is a TOCTOU bug (it could evict a fresh handle a *third*, concurrent caller already inserted after a *second* caller's own eviction). +Every lock helper (`introspector_for`/`evict_introspector`/`export_lock_for`/ +`evict_export_lock`) and every tool handler except `list_generated_servers` +(`introspect_server`/`save_categorized_tools`/`generate_skill`/`save_skill`) +carries a `#[tracing::instrument(skip_all, fields(server_id = ...))]` (or +`output_dir = ...` for the lock helpers) span (issue #211) — this changes the +shape of stderr output (nested spans, structured `server_id`/`output_dir` +fields) but not log message text. + ## 6. Output Directory Resolution (`output_dir.rs`) Mirrors `mcp-skill`'s `resolve_skill_output_path` for a directory target @@ -313,6 +347,18 @@ request" stall (issue #273) that could otherwise leave a valid, already-buffered request undelivered forever if it shared a chunk with a preceding bad line. +The acquired permit for an admitted request is attached to that request's +`Extensions` (`attach_permit`) and released only when `rmcp`'s +`RequestContext` for it is dropped — on handler completion or panic, not on +cancellation alone, since `rmcp`'s own cancel path never aborts the handler +task (issue #227). `RecoveringCodec`'s blank-line handling was later fixed +(issue #284) to peek the next buffered line and silently fold a +blank/whitespace-only line to `DecodedFrame::Skipped` rather than +`Malformed`, avoiding a `tracing::warn!` per blank line — the same +log-volume-amplification class already fixed for the introspector's +symmetric decoder (#275/#282); a genuinely malformed non-blank line still +warns. + ## 9. Error Conditions `StateError`: `AtCapacity { limit }`, `MemoryBudgetExceeded { limit }`. diff --git a/specs/skill/spec.md b/specs/skill/spec.md index 63b88b3..499d2a3 100644 --- a/specs/skill/spec.md +++ b/specs/skill/spec.md @@ -85,8 +85,14 @@ Handlebars render context for both templates), `SkillCategory`/`SkillTool`/ Replaces a historical regex-based `.ts`-file scanner (issue #141: it could never recover parameter descriptions). Now: -1. Canonicalizes `dir`, then canonicalizes `dir/_meta.json` and requires it - to `starts_with(canonical_base)` — defends against a symlinked +1. Canonicalizes `dir`: an `io::ErrorKind::NotFound` maps to + `ScanError::DirectoryNotFound`; any other I/O error (e.g. a symlink loop, + a permission failure) now propagates as `ScanError::Io` with the real + underlying cause, instead of being collapsed into `DirectoryNotFound` as + before (issue #302's fix). Then canonicalizes `dir/_meta.json` under the + same split — `NotFound` → `ScanError::MissingMetadata`, any other kind → + `ScanError::Io` — and requires the resolved sidecar path to + `starts_with(canonical_base)` — defends against a symlinked `_meta.json` escaping the directory (path-traversal via symlink). 2. Size-checks the sidecar (`MAX_FILE_SIZE`), parses as `mcp_core::metadata::ServerMetadata`, checks @@ -103,7 +109,16 @@ never recover parameter descriptions). Now: drift must be visible to a structured caller, not just server-side tracing). `index.ts` (the aggregator) is never treated as an "extra" file. -4. Tools returned sorted by name. +4. Each surviving sidecar entry is converted to a `ParsedToolFile` via the + private `parsed_tool_file_from_metadata(meta, server_id)` function, which + takes the scanned `server_id` directly. This replaced a public + `impl From for ParsedToolFile` that could only ever set + `server_id: String::new()` as a placeholder, relying on this function to + patch it in afterward — a representable-but-wrong intermediate state. + Removing the `From` impl is a breaking change for any external caller + that used it directly; `ParsedToolFile`'s own fields stay public, so it + remains constructible directly (e.g. by test fixtures) (issue #342). +5. Tools returned sorted by name. ## 4. `build_skill_context` — Sanitization & Grouping @@ -171,6 +186,23 @@ after the file's opening `---`). > evidence ledger, the reasons the swap is deferred rather than rejected, > and the measurable gate/review date (2026-10-27) for revisiting it. +Regression tests pin an incidental parser characteristic worth preserving: +`RawFrontmatter`'s `name`/`description` fields are declared as plain +`Option`, not a buffering type. Because of that, a YAML "alias +bomb" (a handful of anchors each referencing the previous several times, +expanding to millions of nodes if fully materialized) placed under an +undeclared key — or under `description` itself, since deserializing a +sequence into `Option` short-circuits on an immediate type +mismatch — is discarded by serde's derived visitor today without expanding +nested aliases, so parsing stays cheap. Retyping either field to a +buffering shape (`serde_norway::Value`, an untagged enum, or a buffering +`#[serde(deserialize_with)]`) would force alias expansion before per-field +routing and trip `serde_norway`'s own repetition-limit guard instead — +still bounded, but no longer free. This is a currently-true property of +`RawFrontmatter`'s field shape, not a designed defense in its own right; +see [[../decisions/ADR-341-serde-saphyr-vs-serde-norway]] for the +evaluation this characteristic factored into. + ## 8. `resolve_skill_output_path` — Path Confinement Confines `save_skill`'s optional `output_path` to `base_dir/server_id` From 3fdb25e86104953586007b32eccceec209bc6416 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Mon, 27 Jul 2026 18:05:18 +0200 Subject: [PATCH 3/4] release: prepare v0.9.0 Bump workspace version and internal path-dependency version pins from 0.8.0 to 0.9.0, refresh Cargo.lock, and finalize the CHANGELOG's Unreleased section under the 0.9.0 release header. --- CHANGELOG.md | 5 ++++- Cargo.lock | 14 +++++++------- Cargo.toml | 14 +++++++------- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63263ab..7e7f4d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.9.0] - 2026-07-27 + ### Security - **`mcp-execution-core`**: added `redact_urls_in_text`, which scans arbitrary already-assembled @@ -3106,7 +3108,8 @@ Phase 6 (Optimization) is currently OPTIONAL and DEFERRED because: --- -[Unreleased]: https://github.com/bug-ops/mcp-execution/compare/v0.8.0...HEAD +[Unreleased]: https://github.com/bug-ops/mcp-execution/compare/v0.9.0...HEAD +[0.9.0]: https://github.com/bug-ops/mcp-execution/compare/v0.8.0...v0.9.0 [0.8.0]: https://github.com/bug-ops/mcp-execution/compare/v0.7.2...v0.8.0 [0.7.2]: https://github.com/bug-ops/mcp-execution/compare/v0.7.1...v0.7.2 [0.7.1]: https://github.com/bug-ops/mcp-execution/compare/v0.7.0...v0.7.1 diff --git a/Cargo.lock b/Cargo.lock index e2b804c..b7aa05c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1290,7 +1290,7 @@ checksum = "47e1ffaa40ddd1f3ed91f717a33c8c0ee23fff369e3aa8772b9605cc1d22f4c3" [[package]] name = "mcp-execution-cli" -version = "0.8.0" +version = "0.9.0" dependencies = [ "anyhow", "clap", @@ -1315,7 +1315,7 @@ dependencies = [ [[package]] name = "mcp-execution-codegen" -version = "0.8.0" +version = "0.9.0" dependencies = [ "criterion", "handlebars", @@ -1329,7 +1329,7 @@ dependencies = [ [[package]] name = "mcp-execution-core" -version = "0.8.0" +version = "0.9.0" dependencies = [ "async-trait", "chrono", @@ -1345,7 +1345,7 @@ dependencies = [ [[package]] name = "mcp-execution-files" -version = "0.8.0" +version = "0.9.0" dependencies = [ "criterion", "dhat", @@ -1358,7 +1358,7 @@ dependencies = [ [[package]] name = "mcp-execution-introspector" -version = "0.8.0" +version = "0.9.0" dependencies = [ "axum", "criterion", @@ -1376,7 +1376,7 @@ dependencies = [ [[package]] name = "mcp-execution-server" -version = "0.8.0" +version = "0.9.0" dependencies = [ "anyhow", "chrono", @@ -1403,7 +1403,7 @@ dependencies = [ [[package]] name = "mcp-execution-skill" -version = "0.8.0" +version = "0.9.0" dependencies = [ "dirs", "handlebars", diff --git a/Cargo.toml b/Cargo.toml index 6068ed8..7241060 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ default-members = [ "crates/*" ] resolver = "3" [workspace.package] -version = "0.8.0" +version = "0.9.0" edition = "2024" rust-version = "1.91" authors = ["Andrei G "] @@ -26,12 +26,12 @@ dirs = "6.0" futures-util = "0.3" handlebars = "6.4" http = "1" -mcp-execution-codegen = { path = "crates/mcp-codegen", version = "0.8.0" } -mcp-execution-core = { path = "crates/mcp-core", version = "0.8.0" } -mcp-execution-files = { path = "crates/mcp-files", version = "0.8.0" } -mcp-execution-introspector = { path = "crates/mcp-introspector", version = "0.8.0" } -mcp-execution-server = { path = "crates/mcp-server", version = "0.8.0" } -mcp-execution-skill = { path = "crates/mcp-skill", version = "0.8.0" } +mcp-execution-codegen = { path = "crates/mcp-codegen", version = "0.9.0" } +mcp-execution-core = { path = "crates/mcp-core", version = "0.9.0" } +mcp-execution-files = { path = "crates/mcp-files", version = "0.9.0" } +mcp-execution-introspector = { path = "crates/mcp-introspector", version = "0.9.0" } +mcp-execution-server = { path = "crates/mcp-server", version = "0.9.0" } +mcp-execution-skill = { path = "crates/mcp-skill", version = "0.9.0" } rayon = "1.12" regex = "1.12" rmcp = "2.1" From a67adc76b03aafc9cfa0438c1bff3014eb344109 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Mon, 27 Jul 2026 18:10:02 +0200 Subject: [PATCH 4/4] docs(specs): sync CLI specification with changes since v0.8.0 Bring specs/cli/spec.md in line with the library/binary split, typed --format handling, ServerSource/ServerStatus enums, generate's --name validation and export path, get_mcp_server_entry split, and build_command_string redaction behavior merged since v0.8.0. --- specs/cli/spec.md | 127 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 115 insertions(+), 12 deletions(-) diff --git a/specs/cli/spec.md b/specs/cli/spec.md index dec00c6..3141fbf 100644 --- a/specs/cli/spec.md +++ b/specs/cli/spec.md @@ -35,6 +35,17 @@ do, without going through the MCP-server layer at all: read server, generate progressive-loading TypeScript, render SKILL.md, and manage/validate server configuration and the local runtime environment. +The crate is a library (`src/lib.rs`) with a thin binary entry point +(`src/main.rs`: parse `Cli`, call `runner::init_logging`, call +`runner::execute_command`, then `std::process::exit` on the returned +`ExitCode`). `lib.rs` exposes `pub mod cli`, `pub mod runner`, `pub mod +actions`, `pub mod commands`, and `pub mod formatters` (plus a re-exported +`ServerAction`) — `cli::{Cli, Commands}` and `runner`'s +command-execution/exit-code-classification entry points are genuine public +library API, not merely an implementation detail of the compiled binary, so +an external crate or integration test can drive command parsing/execution +in-process (issue #188). + ## 2. Subcommands (`Commands` enum, `src/cli.rs`) | Subcommand | Purpose | Key flags | @@ -48,6 +59,15 @@ manage/validate server configuration and the local runtime environment. Global flags on `Cli` (apply to every subcommand): `-v/--verbose` (DEBUG log level), `--format {json,text,pretty}` (default `pretty`, case-insensitive). +`--format` is typed as `mcp_execution_core::cli::OutputFormat` directly via +clap's `PossibleValuesParser` (mapped through `OutputFormat::from_str`), +not a raw `String` parsed post-hoc: `--help` lists the three possible +values, an invalid value (e.g. `--format xml`) is rejected by clap itself +before any command handler runs (routed through the same +`runner::report_and_classify`/`ExitCode::INVALID_INPUT` path as a +handler-level failure — see [[#5. runner.rs]]), and +`completions`-generated shell scripts complete `--format` from the same +three values (issue #206). `introspect`/`generate` flatten a shared `ServerFlags` (`#[derive(Args)]`, private fields, `cli.rs`) holding `from_config`/`server`/`args`/`env`/`cwd`/ @@ -226,29 +246,65 @@ command. `run(source: ServerSource, name: Option, output_dir: Option, dry_run: bool, output_format) -> Result`: -1. `resolve_server_config` → `discover_server_info` (introspects; applies - `--name` override to `ServerInfo.id` if given, so generated - directory/literals use the custom name rather than the raw command). +1. `resolve_server_config` → `discover_server_info`: if `--name` is given, + it is validated via `mcp_execution_skill::validate_server_id` **before** + the connection attempt and only overrides `ServerInfo.id` once valid — + an invalid `--name` (traversal shape, absolute path, or simply outside + `validate_server_id`'s `[a-z0-9-]` charset, e.g. `My Server!`) is now a + hard `INVALID_INPUT` error instead of being silently slugified into a + different id. **Breaking behavior change** from the pre-#311 CLI, which + constructed `ServerId::new(custom_name)` directly with no validation. 2. If the server has zero tools: logs a warning and returns `ExitCode::SUCCESS` (not an error) without generating anything. -3. `ProgressiveGenerator::generate` (uncategorized — no LLM step in this +3. `resolve_server_dir_name` turns `server_info.id` into the directory + name, re-validating it via `validate_server_id` regardless of which arm + produced the id: `derive_server_id_from_path_or_name` (stdio command), + `derive_server_id_from_url` (http/sse — see [[#3. common.rs]]), or the + already-validated `--name` override. This check is a redundant backstop + for those three arms, but the **sole** enforcement point when the id + came straight from an unvalidated `--from-config` `mcp.json` key with + no `--name` override — + the error message differs accordingly (names `mcp.json` and suggests a + ready-to-use `--name` slug vs. framing the failure as an internal error + for the other arms, since reaching it there would mean one of their own + checks has a bug) (issue #311). +4. `ProgressiveGenerator::generate` (uncategorized — no LLM step in this path, unlike `mcp-server`'s `save_categorized_tools`). -4. `resolve_base_dir(output_dir)` — defaults to `~/.claude/servers`. -5. **`--dry-run`**: renders a `DryRunResult` (`FilePreview` per file: path + +5. `resolve_base_dir(output_dir)` — defaults to `~/.claude/servers`. +6. **`--dry-run`**: renders a `DryRunResult` (`FilePreview` per file: path + size, human-readable `format_size`) **without writing anything to disk** — the only place in this workspace that previews generated output without ever touching the filesystem. -6. Otherwise: `FilesBuilder::from_generated_code(code, "/").build_and_export(&base_dir)` - — see [[../files/spec#FilesBuilder::build_and_export]] for the - per-top-level-group atomicity this implies (a re-run with fewer tools - deletes stale tool files in that server's own directory, but never - touches sibling servers under the same `base_dir`). -7. Success output names the required post-export step +7. Otherwise: `FilesBuilder::from_generated_code(code, "/").build()`, then + `FileSystem::export_to_filesystem_with_options(output_path, + &ExportOptions::new().with_confine_to(base_dir))` — **not** + `FilesBuilder::build_and_export`, which treats its target as a + shared multi-server root; `generate` instead publishes one server's + whole directory (`output_path = base_dir.join(server_dir_name)`) per + call, getting `export_to_filesystem_with_options`'s own per-call atomic + staging/swap (a re-run with fewer tools deletes stale tool files in that + server's own directory, but never touches sibling servers under the + same `base_dir`). `with_confine_to(base_dir)` is a second, + defense-in-depth layer behind the id-sanitization in step 3: a future + caller that skipped that sanitization fails loudly instead of writing + outside `base_dir`. See [[../files/spec#5. Atomic Export + (`export_to_filesystem_with_options`)]] and + [[../files/spec#8. Cross-Crate Contracts]]. +8. Success output names the required post-export step (`NPM_INSTALL_HINT`: run `npm install` before type-checking the generated package — issue #257's fix, since the generated `package.json` declares `@types/node` as a `devDependency` that isn't installed by `generate` itself). +`Text`/`Pretty` output (both the success report and the `--dry-run` +preview) escapes the MCP server's handshake-supplied `server_name` via +`formatters::escape_display` before interpolating it into a freeform +`"Server: {name} ({id})"` line — always JSON-quoting the value, even when +it contains no control characters, so a benign name like `Test Server` +renders as `Server: "Test Server" (id)`, not just a malicious one (issue +#299). `Json` output is unaffected, since `serde_json` already escapes +string values. + ## 8. `skill` Command (`commands/skill.rs`) `run(server, servers_dir, output_path, skill_name, hints, overwrite, output_format) -> Result`: @@ -287,6 +343,53 @@ single source of truth: handshake (the entry's own full configured timeout applies), the authoritative single-target check. +`ServerEntry.status`/`ServerInfo.status` are typed as a closed +`ServerStatus` enum (`Available`/`Unavailable`, +`#[serde(rename_all = "lowercase")]`), not a bare `String` — a **breaking** +type change from the pre-#318 CLI, though it serializes identically +(`"available"`/`"unavailable"`) so `--format json` consumers are +unaffected. + +`info`/`validate` distinguish "entry absent from `mcp.json`" from "entry +present but invalid" via `get_mcp_server_entry` (looks up the raw +`McpServerEntry` only) rather than `get_mcp_server` (which also eagerly ran +`build_core_config`'s security validation — see [[#3. common.rs]] — +making the two cases indistinguishable through one `with_context` "not +found" wrapping): + +- `server info` on an entry that is present but fails `build_core_config` + (e.g. an invalid URL scheme) or fails introspection now reports a + structured `ServerInfo` with `"status": "unavailable"` through the + normal `output_format` path — not a raw, unformatted `anyhow` error — + while still returning `ExitCode::ERROR` (issue #305). Only a genuinely + absent entry propagates as `Err` (and thus a raw error report). A known + gap: `ServerInfo` has no field naming *why* the server is unavailable + (invalid config vs. failed handshake). +- `server validate` on a present-but-invalid entry reports the actual + `build_core_config`/precheck failure message in `ValidationResult` (e.g. + `"Server '{name}' has an invalid configuration: {e}"`), not the generic + `"Server not found"` message reserved for a genuinely absent entry (issue + #304). + +`build_command_string` (feeds `list`/`info`/`validate` output, printed +unconditionally, never gated behind `--verbose`) redacts the same way +`ServerConfig`'s own `Debug` impl does (issue #346): stdio `command` is +routed through `sanitize_path_for_error` (home directory/username scrub); +stdio `args` are replaced **wholesale** with +`mcp_execution_core::REDACTED_PLACEHOLDER` per entry, rendered as a +space-joined shell-shaped string (`"docker "`), since +a single argument routinely holds an entire secret with no key/value half +worth preserving — unlike `mcp_execution_core::RedactedItems`'s +Rust-`Debug`-list rendering, which would be awkward to embed in +`--format json` output; http/sse `url` is redacted via `RedactedUrl` +(strips userinfo credentials and any query string, keeps scheme/host/path +readable), falling back to redacting the whole string if it fails to +parse. `validate_command`'s own "URL is not well-formed" precheck message +(`url_precheck_message`, built *before* `build_command_string` runs) is +redacted the same way, closing a gap where a malformed-but-credentialed +URL could leak via the precheck message even though `build_command_string` +itself was already safe (issue #346, S1). + ## 10. `setup` Command (`commands/setup.rs`) Validates the local runtime is ready to execute generated tools: