diff --git a/CHANGELOG.md b/CHANGELOG.md index 178a222..fd50636 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,19 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/), and this project adheres to [Semantic Versioning](https://semver.org/). +## [Unreleased] + +### Fixed + +- `fetch_node` and `explore_rpg` now accept batch-only payloads + (`entity_ids` without `entity_id`). Previously serde rejected such + calls with `missing field 'entity_id'` before the handlers' existing + either-or fallback could run, making the documented batch mode + unreachable. `entity_id` is now `Option` on both param + structs; handlers return `"either entity_id or entity_ids is + required"` when both are absent, and reject empty `entity_ids` + batches. (#91) + ## [0.8.3] - 2026-04-14 ### Added diff --git a/crates/rpg-mcp/src/params.rs b/crates/rpg-mcp/src/params.rs index c4a6b00..ade9a20 100644 --- a/crates/rpg-mcp/src/params.rs +++ b/crates/rpg-mcp/src/params.rs @@ -25,8 +25,8 @@ pub(crate) struct SearchNodeParams { /// Parameters for the `fetch_node` tool. #[derive(Debug, Deserialize, JsonSchema)] pub(crate) struct FetchNodeParams { - /// The entity ID to fetch (e.g., 'src/auth.rs:validate_token') - pub(crate) entity_id: String, + /// The entity ID to fetch (e.g., 'src/auth.rs:validate_token'). Either entity_id or entity_ids is required. + pub(crate) entity_id: Option, /// Multiple entity IDs to fetch in batch (overrides entity_id when provided) pub(crate) entity_ids: Option>, /// Comma-separated fields to include: "features", "source", "deps", "hierarchy". Omit for all fields. @@ -38,8 +38,8 @@ pub(crate) struct FetchNodeParams { /// Parameters for the `explore_rpg` tool. #[derive(Debug, Deserialize, JsonSchema)] pub(crate) struct ExploreRpgParams { - /// The entity ID to start exploration from - pub(crate) entity_id: String, + /// The entity ID to start exploration from. Either entity_id or entity_ids is required. + pub(crate) entity_id: Option, /// Multiple entity IDs to explore from in batch (overrides entity_id when provided) pub(crate) entity_ids: Option>, /// Traversal direction: 'upstream', 'downstream', or 'both' @@ -301,3 +301,47 @@ impl std::fmt::Debug for AutoLiftParams { .finish() } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn fetch_node_batch_only_payload_deserializes() { + // Reproduces the bug: caller supplies entity_ids (batch) without entity_id. + // Before the fix, serde rejected this with `missing field 'entity_id'` + // before the handler could use entity_ids. + let payload = serde_json::json!({ + "entity_ids": [ + "docker/fetch-runtime-config.mjs:main", + "docker/fetch-runtime-config.mjs:fetchSsmEnv", + "docker/fetch-runtime-config.mjs:fetchSecrets" + ], + "fields": "source,deps,features", + "source_max_lines": 220 + }); + + let result: Result = serde_json::from_value(payload); + assert!( + result.is_ok(), + "batch-only payload should deserialize, got: {:?}", + result.err() + ); + } + + #[test] + fn explore_rpg_batch_only_payload_deserializes() { + // Same bug, same fix needed on ExploreRpgParams. + let payload = serde_json::json!({ + "entity_ids": ["foo.rs:bar", "baz.rs:qux"], + "direction": "downstream" + }); + + let result: Result = serde_json::from_value(payload); + assert!( + result.is_ok(), + "batch-only payload should deserialize, got: {:?}", + result.err() + ); + } +} diff --git a/crates/rpg-mcp/src/tools.rs b/crates/rpg-mcp/src/tools.rs index 74d8190..f7eb351 100644 --- a/crates/rpg-mcp/src/tools.rs +++ b/crates/rpg-mcp/src/tools.rs @@ -170,11 +170,7 @@ impl RpgServer { let guard = self.graph.read().await; let graph = guard.as_ref().unwrap(); - let ids: Vec<&str> = if let Some(ref batch) = params.entity_ids { - batch.iter().map(|s| s.as_str()).collect() - } else { - vec![params.entity_id.as_str()] - }; + let ids = requested_entity_ids(params.entity_ids.as_deref(), params.entity_id.as_ref())?; let projection = rpg_nav::toon::FetchProjection::from_params( params.fields.as_deref(), @@ -228,11 +224,7 @@ impl RpgServer { .map(parse_entity_type_filter) .filter(|v| !v.is_empty()); - let ids: Vec<&str> = if let Some(ref batch) = params.entity_ids { - batch.iter().map(|s| s.as_str()).collect() - } else { - vec![params.entity_id.as_str()] - }; + let ids = requested_entity_ids(params.entity_ids.as_deref(), params.entity_id.as_ref())?; let use_compact = matches!(params.format.as_deref(), Some("compact")); @@ -3622,6 +3614,18 @@ impl RpgServer { } } +fn requested_entity_ids<'a>( + entity_ids: Option<&'a [String]>, + entity_id: Option<&'a String>, +) -> Result, String> { + match (entity_ids, entity_id) { + (Some([]), _) => Err("entity_ids must not be empty".to_string()), + (Some(batch), _) => Ok(batch.iter().map(|s| s.as_str()).collect()), + (None, Some(id)) => Ok(vec![id.as_str()]), + (None, None) => Err("either entity_id or entity_ids is required".to_string()), + } +} + /// Parse an edge_filter string into an EdgeKind. Exposed for testing. pub fn parse_edge_filter(filter: &str) -> Option { match filter { @@ -3649,6 +3653,36 @@ mod tests { assert_eq!(parse_edge_filter("data_flow"), Some(EdgeKind::DataFlow)); } + #[test] + fn requested_entity_ids_requires_single_or_batch() { + assert_eq!( + requested_entity_ids(None, None).unwrap_err(), + "either entity_id or entity_ids is required" + ); + } + + #[test] + fn requested_entity_ids_rejects_empty_batch() { + let batch = Vec::new(); + let entity_id = "fallback.rs:item".to_string(); + + assert_eq!( + requested_entity_ids(Some(&batch), Some(&entity_id)).unwrap_err(), + "entity_ids must not be empty" + ); + } + + #[test] + fn requested_entity_ids_prefers_non_empty_batch() { + let batch = vec!["a.rs:first".to_string(), "b.rs:second".to_string()]; + let entity_id = "fallback.rs:item".to_string(); + + assert_eq!( + requested_entity_ids(Some(&batch), Some(&entity_id)).unwrap(), + vec!["a.rs:first", "b.rs:second"] + ); + } + #[test] fn test_parse_edge_filter_all_kinds() { assert_eq!(parse_edge_filter("imports"), Some(EdgeKind::Imports));