From 91d6606868d0e9c5f78ab48a592ef95ffaeeca61 Mon Sep 17 00:00:00 2001 From: Alexander Petric Date: Fri, 17 Jul 2026 00:15:34 +0200 Subject: [PATCH] fix(mcp): push granular scope patterns into SQL so scoped scripts/flows aren't truncated (#10140) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): push granular scope patterns into SQL so scoped scripts/flows aren't truncated MCP `list_tools` fetched scripts/flows capped at the 100 newest by `created_at` and only *then* filtered by the token's granular folder/custom scope in Rust. In a workspace with more than 100 scripts/flows, in-scope items outside that newest-100 window were truncated before the scope filter ran, so a folder- or custom-scoped token could see zero tools even though matching items existed. Push the scope patterns into the query via a new `PathFilter::Patterns` (mirroring `is_resource_allowed`: `*` disables filtering, exact paths match by equality, `x/*` matches the folder or its subtree, empty grants nothing) so the filter applies before the `ITEMS_FETCH_MAX_LIMIT` cap. The existing hashed-name resolution path keeps its prefix behavior via `PathFilter::Prefix`, and the Rust post-filter stays as defense in depth. Co-Authored-By: Claude Opus 4.8 (1M context) * feat(mcp): warn in scope selector when a scope exceeds the MCP tool cap The server exposes at most ITEMS_FETCH_MAX_LIMIT (100) scripts and 100 flows per token; a scope matching more silently drops the overflow, which bloats the assistant's context with a partial, arbitrary tool set. McpScopeSelector now computes how many scripts/flows the current scope would expose (per type, mirroring the backend's is_resource_allowed) and shows a warning Alert when either exceeds the cap, so the user can narrow the scope before generating the URL/token. An async sequence guard keeps rapid scope changes from applying stale counts. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(mcp): address review — dedup count fetches, boundary-aware folder counting, fix copy Follow-up to the MCP scope-selector truncation warning: - Reuse a single per-type (scripts/flows) cache for both the preview list and the exposed count, instead of a second concurrent fetch of the same rows. - Count a folder scope against the `f/{folder}/*` subtree (via the same boundary-aware matcher), so a folder like `team` no longer over-counts a sibling like `team2` and falsely warns. - Custom-mode counts are derived synchronously from the already-loaded scripts/flows — no fetch. - Reword the warning to "most recent" (flows are ordered by edited_at, not created_at). Co-Authored-By: Claude Opus 4.8 (1M context) * fix(mcp): anchor folder count fetch at the folder boundary Follow-up to review: the folder count fetched the unbounded prefix `f/{folder}` (backend `path LIKE 'f/{folder}%'`), so a prefix-sharing sibling like `f/team2` shared the page. With a page limit, enough newer sibling rows could fill the first page ahead of the target folder's older rows; the client-side boundary filter then dropped them all, wrongly suppressing the warning and emptying the preview. Fetch `f/{folder}/` instead so the backend prefix (`LIKE 'f/{folder}/%'`) is anchored at the folder boundary and never returns siblings. The client-side matcher stays as a backstop for folder names whose LIKE wildcards (`_`, `%`) can still let the backend prefix over-match. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: Ruben Fiszel --- backend/windmill-api/src/mcp/core.rs | 10 +- backend/windmill-api/src/mcp/utils.rs | 114 ++++++++++- backend/windmill-mcp/src/server/backend.rs | 26 ++- backend/windmill-mcp/src/server/mod.rs | 2 +- backend/windmill-mcp/src/server/runner.rs | 23 ++- .../components/mcp/McpScopeSelector.svelte | 186 ++++++++++++------ 6 files changed, 283 insertions(+), 78 deletions(-) diff --git a/backend/windmill-api/src/mcp/core.rs b/backend/windmill-api/src/mcp/core.rs index db1644379d..cdf1f0b3a3 100644 --- a/backend/windmill-api/src/mcp/core.rs +++ b/backend/windmill-api/src/mcp/core.rs @@ -12,7 +12,7 @@ use windmill_mcp::common::transform::apply_key_transformation; use windmill_mcp::common::types::{ FlowInfo, HubScriptInfo, ResourceInfo, ResourceType, SchemaType, ScriptInfo, WorkspaceInfo, }; -use windmill_mcp::server::{BackendResult, EndpointTool, ErrorData, McpBackend}; +use windmill_mcp::server::{BackendResult, EndpointTool, ErrorData, McpBackend, PathFilter}; use crate::auth::AuthCache; use crate::db::ApiAuthed; @@ -78,7 +78,7 @@ impl McpBackend for WindmillBackend { auth: &ApiAuthed, workspace_id: &str, favorites_only: bool, - path_prefix: Option<&str>, + path_filter: Option>, ) -> BackendResult> { let scope_type = if favorites_only { "favorites" } else { "all" }; get_items::( @@ -87,7 +87,7 @@ impl McpBackend for WindmillBackend { workspace_id, scope_type, "script", - path_prefix, + path_filter, ) .await .map_err(|e| ErrorData::internal_error(e.message, None)) @@ -98,7 +98,7 @@ impl McpBackend for WindmillBackend { auth: &ApiAuthed, workspace_id: &str, favorites_only: bool, - path_prefix: Option<&str>, + path_filter: Option>, ) -> BackendResult> { let scope_type = if favorites_only { "favorites" } else { "all" }; get_items::( @@ -107,7 +107,7 @@ impl McpBackend for WindmillBackend { workspace_id, scope_type, "flow", - path_prefix, + path_filter, ) .await .map_err(|e| ErrorData::internal_error(e.message, None)) diff --git a/backend/windmill-api/src/mcp/utils.rs b/backend/windmill-api/src/mcp/utils.rs index de0ad9d4da..70a4c6c1f4 100644 --- a/backend/windmill-api/src/mcp/utils.rs +++ b/backend/windmill-api/src/mcp/utils.rs @@ -15,7 +15,7 @@ use windmill_common::scripts::{get_full_hub_script_by_path, Schema}; use windmill_common::utils::{query_elems_from_hub, StripPath}; use windmill_common::worker::to_raw_value; use windmill_common::{DB, HUB_BASE_URL}; -use windmill_mcp::server::{BackendResult, ErrorData}; +use windmill_mcp::server::{BackendResult, ErrorData, PathFilter}; use windmill_mcp::{HubResponse, HubScriptInfo, ItemSchema, ResourceInfo, ResourceType}; use crate::db::ApiAuthed; @@ -24,6 +24,43 @@ use crate::HTTP_CLIENT; // items max limit const ITEMS_FETCH_MAX_LIMIT: usize = 100; +/// Escape LIKE wildcards so a literal path is matched as a prefix, not a pattern. +fn escape_like(s: &str) -> String { + s.replace('\\', "\\\\") + .replace('%', "\\%") + .replace('_', "\\_") +} + +/// Build the SQL condition matching any MCP scope pattern, mirroring +/// `is_resource_allowed`. Returns `None` when no filter should be applied (a `*` +/// pattern grants everything); `Some("false")` when the list is empty (grants +/// nothing); otherwise an OR of per-pattern `o.path` conditions. +fn scope_patterns_condition(patterns: &[String]) -> Option { + if patterns.iter().any(|p| p == "*") { + return None; + } + if patterns.is_empty() { + return Some("false".to_string()); + } + let conds: Vec = patterns + .iter() + .map(|p| { + if let Some(prefix) = p.strip_suffix("/*") { + // A subtree pattern matches the folder itself or anything under it. + let subtree = format!("{}/%", escape_like(prefix)); + format!( + "({} OR {})", + "o.path = ?".bind(&prefix), + "o.path LIKE ? ESCAPE '\\'".bind(&subtree), + ) + } else { + "o.path = ?".bind(p) + } + }) + .collect(); + Some(format!("({})", conds.join(" OR "))) +} + // ============================================================================ // Database utilities // ============================================================================ @@ -135,7 +172,7 @@ pub async fn get_items sqlx::FromRow<'a, sqlx::postgres::PgRow> + Sen workspace_id: &str, scope_type: &str, item_type: &str, - path_prefix: Option<&str>, + path_filter: Option>, ) -> Result, ErrorData> { let mut sqlb = SqlBuilder::select_from(&format!("{} as o", item_type)); let fields = vec!["o.path", "o.summary", "o.description", "o.schema"]; @@ -155,12 +192,17 @@ pub async fn get_items sqlx::FromRow<'a, sqlx::postgres::PgRow> + Sen sqlb.and_where("(o.auto_kind IS NULL OR o.auto_kind <> 'lib')"); } - if let Some(prefix) = path_prefix { - let escaped = prefix - .replace('\\', "\\\\") - .replace('%', "\\%") - .replace('_', "\\_"); - sqlb.and_where("o.path LIKE ? ESCAPE '\\'".bind(&format!("{}%", escaped))); + match path_filter { + None => {} + Some(PathFilter::Prefix(prefix)) => { + let escaped = format!("{}%", escape_like(prefix)); + sqlb.and_where("o.path LIKE ? ESCAPE '\\'".bind(&escaped)); + } + Some(PathFilter::Patterns(patterns)) => { + if let Some(cond) = scope_patterns_condition(patterns) { + sqlb.and_where(cond); + } + } } sqlb.order_by( @@ -781,4 +823,60 @@ mod tests { "?path=u%2Falice%2Fmy%20script" ); } + + fn strings(v: &[&str]) -> Vec { + v.iter().map(|s| s.to_string()).collect() + } + + #[test] + fn scope_patterns_condition_wildcard_disables_filter() { + // A `*` pattern grants everything, so no SQL condition should be added. + assert_eq!(scope_patterns_condition(&strings(&["*"])), None); + assert_eq!(scope_patterns_condition(&strings(&["f/team/*", "*"])), None); + } + + #[test] + fn scope_patterns_condition_empty_matches_nothing() { + // An empty pattern list grants no items of this type. + assert_eq!(scope_patterns_condition(&[]), Some("false".to_string())); + } + + #[test] + fn scope_patterns_condition_exact_path() { + assert_eq!( + scope_patterns_condition(&strings(&["u/admin/my_script"])), + Some("(o.path = 'u/admin/my_script')".to_string()) + ); + } + + #[test] + fn scope_patterns_condition_subtree() { + // `f/team/*` matches the folder itself or anything beneath it, mirroring + // resource_matches_pattern. Underscores in the prefix are LIKE-escaped. + assert_eq!( + scope_patterns_condition(&strings(&["f/team/*"])), + Some("((o.path = 'f/team' OR o.path LIKE 'f/team/%' ESCAPE '\\'))".to_string()) + ); + } + + #[test] + fn scope_patterns_condition_mixed_ored() { + assert_eq!( + scope_patterns_condition(&strings(&["u/admin/one", "f/team/*"])), + Some( + "(o.path = 'u/admin/one' OR (o.path = 'f/team' OR o.path LIKE 'f/team/%' ESCAPE '\\'))" + .to_string() + ) + ); + } + + #[test] + fn scope_patterns_condition_escapes_like_wildcards() { + // A subtree prefix containing `%`/`_` must be escaped so it isn't treated + // as a LIKE pattern; the exact-match arm is quoted verbatim by bind. + assert_eq!( + scope_patterns_condition(&strings(&["f/a_b/*"])), + Some("((o.path = 'f/a_b' OR o.path LIKE 'f/a\\_b/%' ESCAPE '\\'))".to_string()) + ); + } } diff --git a/backend/windmill-mcp/src/server/backend.rs b/backend/windmill-mcp/src/server/backend.rs index 96a2bed8df..e42c0dacfc 100644 --- a/backend/windmill-mcp/src/server/backend.rs +++ b/backend/windmill-mcp/src/server/backend.rs @@ -16,6 +16,24 @@ use crate::server::endpoints::EndpointTool; /// Result type for backend operations using rmcp's ErrorData directly pub type BackendResult = Result; +/// How a script/flow listing is narrowed by path at the SQL layer, *before* the +/// `ITEMS_FETCH_MAX_LIMIT` cap applies. +/// +/// This must be pushed into the query, not applied in Rust after the fetch: the +/// listing is capped to the newest N rows, so a granular token whose in-scope +/// items are not among those N would have them truncated away before any Rust +/// filter ran (returning zero tools even though the items exist). +#[derive(Debug, Clone, Copy)] +pub enum PathFilter<'a> { + /// Raw `LIKE '{prefix}%'` prefix — used to resolve a hashed tool name back to + /// its full path. + Prefix(&'a str), + /// MCP scope patterns (`*`, an exact path, or an `x/*` subtree). Mirrors + /// `is_resource_allowed`: a `*` pattern matches everything, an empty list + /// matches nothing. + Patterns(&'a [String]), +} + /// Authentication context required by the MCP server pub trait McpAuth: Send + Sync + Clone + 'static { /// Get the username @@ -62,22 +80,22 @@ pub trait McpBackend: Send + Sync + Clone + 'static { // Listing Operations // ───────────────────────────────────────────────────────────────── - /// List scripts, optionally filtered to favorites only and/or by path prefix + /// List scripts, optionally filtered to favorites only and/or by path async fn list_scripts( &self, auth: &Self::Auth, workspace_id: &str, favorites_only: bool, - path_prefix: Option<&str>, + path_filter: Option>, ) -> BackendResult>; - /// List flows, optionally filtered to favorites only and/or by path prefix + /// List flows, optionally filtered to favorites only and/or by path async fn list_flows( &self, auth: &Self::Auth, workspace_id: &str, favorites_only: bool, - path_prefix: Option<&str>, + path_filter: Option>, ) -> BackendResult>; /// List resource types in workspace diff --git a/backend/windmill-mcp/src/server/mod.rs b/backend/windmill-mcp/src/server/mod.rs index d5d49bc746..da7032418b 100644 --- a/backend/windmill-mcp/src/server/mod.rs +++ b/backend/windmill-mcp/src/server/mod.rs @@ -12,7 +12,7 @@ pub mod tools; // Re-export main types pub use crate::common::types::{McpToken, MultiWorkspaceMcp, WorkspaceInfo}; -pub use backend::{BackendResult, McpAuth, McpBackend}; +pub use backend::{BackendResult, McpAuth, McpBackend, PathFilter}; pub use endpoints::{ endpoint_tool_to_mcp_tool, endpoint_tool_to_mcp_tool_multi, is_endpoint_read_only, list_workspaces_tool, EndpointTool, diff --git a/backend/windmill-mcp/src/server/runner.rs b/backend/windmill-mcp/src/server/runner.rs index dd810269d9..5db32faedb 100644 --- a/backend/windmill-mcp/src/server/runner.rs +++ b/backend/windmill-mcp/src/server/runner.rs @@ -10,7 +10,7 @@ use crate::common::transform::{ reverse_transform, reverse_transform_key, }; use crate::common::types::{McpToken, MultiWorkspaceMcp, ResourceInfo, ToolableItem, WorkspaceId}; -use crate::server::backend::{McpAuth, McpBackend}; +use crate::server::backend::{McpAuth, McpBackend, PathFilter}; use crate::server::endpoints::{ endpoint_tool_to_mcp_tool, endpoint_tool_to_mcp_tool_multi, list_workspaces_tool, }; @@ -273,11 +273,23 @@ impl Runner { // mutating action), so skip the script/flow/hub/resource fetches // entirely — they would only be discarded below. if !read_only { + // For granular tokens, push the scope patterns into the SQL query so + // in-scope items survive the fetch cap (see `PathFilter`). The Rust + // filter below still runs as a defense-in-depth check. Non-granular + // (favorites/all) tokens are already narrowed by the favorites join + // or intentionally unfiltered. + let script_filter = scope_config + .granular + .then(|| PathFilter::Patterns(scope_config.scripts.as_slice())); + let flow_filter = scope_config + .granular + .then(|| PathFilter::Patterns(scope_config.flows.as_slice())); + let (scripts, flows, resource_types, hub_scripts) = tokio::try_join!( self.backend - .list_scripts(auth, workspace_id, favorites_only, None), + .list_scripts(auth, workspace_id, favorites_only, script_filter), self.backend - .list_flows(auth, workspace_id, favorites_only, None), + .list_flows(auth, workspace_id, favorites_only, flow_filter), self.backend.list_resource_types(auth, workspace_id), async { if let Some(ref apps) = scope_config.hub_apps { @@ -456,11 +468,12 @@ impl Runner { (type_str, version_id, true) } else { let path_prefix = extract_path_prefix_from_hashed(name.as_ref()); + let path_filter = path_prefix.as_deref().map(PathFilter::Prefix); let favorites_only = scope_config.favorites; let matched_path = if type_str == "script" { find_matching_path( self.backend - .list_scripts(auth, workspace_id, favorites_only, path_prefix.as_deref()) + .list_scripts(auth, workspace_id, favorites_only, path_filter) .await .map_err(|e| ErrorData::internal_error(e.message, None))?, name.as_ref(), @@ -468,7 +481,7 @@ impl Runner { } else { find_matching_path( self.backend - .list_flows(auth, workspace_id, favorites_only, path_prefix.as_deref()) + .list_flows(auth, workspace_id, favorites_only, path_filter) .await .map_err(|e| ErrorData::internal_error(e.message, None))?, name.as_ref(), diff --git a/frontend/src/lib/components/mcp/McpScopeSelector.svelte b/frontend/src/lib/components/mcp/McpScopeSelector.svelte index cf95617790..3ff1ab3d95 100644 --- a/frontend/src/lib/components/mcp/McpScopeSelector.svelte +++ b/frontend/src/lib/components/mcp/McpScopeSelector.svelte @@ -1,5 +1,5 @@