diff --git a/docs/internal/mcp-server-qa-test-plan.md b/docs/internal/mcp-server-qa-test-plan.md index ddd613cf8..c3f6a6b5e 100644 --- a/docs/internal/mcp-server-qa-test-plan.md +++ b/docs/internal/mcp-server-qa-test-plan.md @@ -18,13 +18,13 @@ None currently open. - **I10 — Archived runs not filtered from default search**: fixed on 2026-05-11 by aligning MCP search with the HTTP API. `fabro_run_search` now hides archived runs when `archived` is omitted, while `archived=true` still searches archived runs explicitly. - **I15 / I16 — yes/no answer flow**: re-tested on 2026-05-11 against `fabro server` `0.230.0-nightly.0` at `127.0.0.1:32276`. `answer=true` and `answer=false` both submit successfully for the bundled `interview` workflow's first `yes_no` question. `true` advanced the run to the next `confirmation` question. - **I22 — numeric answer local validation**: re-tested on 2026-05-11 against the same server. `answer=42` now returns `unsupported answer value: 42; expected boolean, string, or object` from the MCP layer before reaching the API. +- **Section 2 side observation — Search payloads include full `goal` text**: fixed on 2026-05-11. `fabro_run_search` now returns bounded `goal_preview` plus `goal_truncated` instead of the full `goal`, keeping list responses compact while preserving full summaries on other run interactions. - **X6 — Cursor/filter ordering**: simplified on 2026-05-11 by applying search filters before sorting and applying the `after` cursor. This prevents unrelated runs outside the filtered result set from trimming the page. Pagination is explicitly not snapshot-isolated; a new matching run inserted before the cursor during traversal appears when the client starts a new search. ### UX / polish - **C12 — `cwd` errors don't distinguish "directory missing" from "workflow not in directory"**: both return `workflow not found: `. - **S9 (bonus) — Undocumented date format**: error message reveals `YYYY-MM-DD` is accepted alongside RFC3339, but the schema only says RFC3339. - **S17 — `run_ids` accepts more than IDs**: error message reveals it also matches ID prefixes and workflow names. Either rename the field or document. -- **Section 2 side observation — Search payloads include full `goal` text**: a single long-goal run (e.g. `ImplementPlan`) inflates every search response by ~30 KB. Consider truncation or excluding `goal` from list responses. - **E4 — Events `search` is whole-envelope substring match**: search includes embedded payloads (workflow definitions, settings, sandbox dockerfile, etc.), so a search like `query="list_prs"` legitimately matches the `run.created` event because that event embeds the workflow JSON. Easy to misinterpret. Consider documenting or scoping search to event body only. ### Nice-to-haves diff --git a/lib/crates/fabro-mcp-server/src/run_tools/search.rs b/lib/crates/fabro-mcp-server/src/run_tools/search.rs index 2066164f0..0a5b2bb15 100644 --- a/lib/crates/fabro-mcp-server/src/run_tools/search.rs +++ b/lib/crates/fabro-mcp-server/src/run_tools/search.rs @@ -10,6 +10,8 @@ use serde::{Deserialize, Serialize}; use super::common; use super::common::{RunSummaryResult, ToolError, ToolResult}; +const SEARCH_GOAL_PREVIEW_CHARS: usize = 240; + #[derive(Debug, Deserialize, JsonSchema)] pub(crate) struct FabroRunSearchParams { pub(crate) run_ids: Option>, @@ -68,10 +70,27 @@ impl TryFrom for ValidatedSearchRuns { #[derive(Debug, Serialize, JsonSchema)] pub(crate) struct SearchRunsResult { - pub(crate) runs: Vec, + pub(crate) runs: Vec, pub(crate) next_cursor: Option, } +#[derive(Debug, Serialize, JsonSchema)] +pub(crate) struct SearchRunSummaryResult { + pub(crate) run_id: String, + pub(crate) workflow_name: String, + pub(crate) workflow_slug: Option, + pub(crate) status: String, + pub(crate) archived: bool, + pub(crate) created_at: String, + pub(crate) started_at: Option, + pub(crate) completed_at: Option, + pub(crate) labels: HashMap, + pub(crate) source_directory: Option, + pub(crate) repo_origin_url: Option, + pub(crate) goal_preview: String, + pub(crate) goal_truncated: bool, +} + pub(crate) async fn search_runs( client: Arc, params: ValidatedSearchRuns, @@ -89,11 +108,58 @@ pub(crate) async fn search_runs( let page = filter_sort_and_page_runs(runs, &raw, status.as_deref())?; Ok(SearchRunsResult { - runs: page.runs.iter().map(common::run_summary_result).collect(), + runs: page.runs.iter().map(search_run_summary_result).collect(), next_cursor: page.next_cursor, }) } +fn search_run_summary_result(run: &Run) -> SearchRunSummaryResult { + let RunSummaryResult { + run_id, + workflow_name, + workflow_slug, + status, + archived, + created_at, + started_at, + completed_at, + labels, + source_directory, + repo_origin_url, + goal, + } = common::run_summary_result(run); + let (goal_preview, goal_truncated) = goal_preview(&goal); + + SearchRunSummaryResult { + run_id, + workflow_name, + workflow_slug, + status, + archived, + created_at, + started_at, + completed_at, + labels, + source_directory, + repo_origin_url, + goal_preview, + goal_truncated, + } +} + +fn goal_preview(goal: &str) -> (String, bool) { + let mut chars = goal.chars(); + let mut preview = chars + .by_ref() + .take(SEARCH_GOAL_PREVIEW_CHARS) + .collect::(); + let truncated = chars.next().is_some(); + if truncated { + preview.push_str("..."); + } + (preview, truncated) +} + struct RunSearchPage { runs: Vec, next_cursor: Option, @@ -247,6 +313,18 @@ mod tests { assert_eq!(ids, vec![active.id]); } + #[test] + fn search_summary_uses_bounded_goal_preview() { + let mut run = run("01KRBZW5C00000000000000001", "keep", 30); + run.goal = format!("{}tail-marker", "a".repeat(300)); + + let summary = search_run_summary_result(&run); + + assert!(summary.goal_truncated); + assert!(summary.goal_preview.len() < run.goal.len()); + assert!(!summary.goal_preview.contains("tail-marker")); + } + fn run(id: &str, group: &str, seconds: u32) -> Run { run_with_archived(id, group, seconds, false) }