From 8774abd786eded060094fcd8d564a66f1b95c682 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 11 May 2026 16:58:46 -0400 Subject: [PATCH] fix(mcp): hide archived runs by default --- docs/internal/mcp-server-qa-test-plan.md | 5 +- .../fabro-mcp-server/src/run_tools/search.rs | 49 ++++++++++++++++--- 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/docs/internal/mcp-server-qa-test-plan.md b/docs/internal/mcp-server-qa-test-plan.md index 422a38225..ddd613cf8 100644 --- a/docs/internal/mcp-server-qa-test-plan.md +++ b/docs/internal/mcp-server-qa-test-plan.md @@ -9,12 +9,13 @@ This plan is **not** a template for adding automated test coverage — it exists Live list of bugs and notable observations surfaced during the sweep. Each entry links back to the scenario where it was found. ### Bugs / mismatches -- **I10 — Archived runs not filtered from default search**: `fabro_run_search` with no `archived` filter returns archived runs alongside active ones. Most systems hide archived by default; consider flipping the default. +None currently open. ### Rechecked / no longer open - **C4 — `inputs` schema/runtime mismatch**: fixed by narrowing MCP input values to scalar JSON (`string`, `boolean`, `integer`, `number`) and rejecting arrays/objects locally with scalar-only errors. Re-tested on 2026-05-11 against `127.0.0.1:32276`; `tools/list` now advertises scalar-only `inputs.additionalProperties`. - **C5 — Misleading null-input error message**: fixed. Re-tested on 2026-05-11; null now returns ``input `maybe` cannot be null; use a string, boolean, or number``. - **I7 / I9 — Misleading "Run not found." on terminal runs**: fixed on 2026-05-11 in the server API layer. `message`/steer against a durable terminal run that no longer has a live managed engine now returns `409` with `run_not_steerable`; `cancel` returns `409` with `Run is already terminal and cannot be cancelled.` True missing runs still return `404`. +- **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. - **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. @@ -191,7 +192,7 @@ Source: `run_tools/interact.rs:201` - [x] **I9** Cancel an already-terminal run → initially returned `Run not found.`. — **FIXED**: durable terminal runs without a live managed engine now return `409` with `Run is already terminal and cannot be cancelled.`; true missing runs remain `404`. #### `archive` / `unarchive` -- [x] **I10** Archive terminal run → `archived=true` in summary; visible via `search archived=true`. — **PASS**. **BUT FINDING**: archived runs are **not** filtered out of default search (no archive filter applied unless explicitly requested). +- [x] **I10** Archive terminal run → `archived=true` in summary; visible via `search archived=true`. — **FIXED**: default search now hides archived runs to match `/api/v1/runs`; `archived=true` still surfaces archived runs explicitly. - [x] **I11** Unarchive → reverses (`archived=false`). — **PASS**. - [x] **I12** Archive an active run → `run must be terminal (succeeded, failed, or dead) to archive; current status is starting`. — **PASS** (excellent error). 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 88efe65cf..2066164f0 100644 --- a/lib/crates/fabro-mcp-server/src/run_tools/search.rs +++ b/lib/crates/fabro-mcp-server/src/run_tools/search.rs @@ -123,9 +123,8 @@ fn filter_sort_and_page_runs( .any(|status| *status == run.lifecycle.status.kind()) }); } - if let Some(archived) = raw.archived { - runs.retain(|run| run.lifecycle.archived == archived); - } + let archived = raw.archived.unwrap_or(false); + runs.retain(|run| run.lifecycle.archived == archived); if let Some(created_after) = raw.created_after.as_deref() { let cutoff = common::parse_datetime_filter("created_after", created_after)?; runs.retain(|run| run.timestamps.created_at >= cutoff); @@ -222,7 +221,41 @@ mod tests { assert_eq!(ids, vec![matching_newer.id, matching_older.id]); } + #[test] + fn omitted_archived_filter_hides_archived_runs_by_default() { + let active = run("01KRBZW5C00000000000000001", "keep", 30); + let archived = archived_run("01KRBZW4DW0000000000000002", "keep", 20); + + let result = filter_sort_and_page_runs( + vec![archived.clone(), active.clone()], + &FabroRunSearchParams { + run_ids: None, + workflow: None, + labels: None, + status: None, + archived: None, + created_after: None, + created_before: None, + first: Some(10), + after: None, + }, + None, + ) + .expect("filtering should succeed"); + + let ids = result.runs.iter().map(|run| run.id).collect::>(); + assert_eq!(ids, vec![active.id]); + } + fn run(id: &str, group: &str, seconds: u32) -> Run { + run_with_archived(id, group, seconds, false) + } + + fn archived_run(id: &str, group: &str, seconds: u32) -> Run { + run_with_archived(id, group, seconds, true) + } + + fn run_with_archived(id: &str, group: &str, seconds: u32, archived: bool) -> Run { let created_at = Utc.with_ymd_and_hms(2026, 5, 11, 12, 0, seconds).unwrap(); Run { id: id.parse().expect("test run id should parse"), @@ -238,12 +271,12 @@ mod tests { origin: RunOrigin::default(), labels: HashMap::from([("group".to_string(), group.to_string())]), lifecycle: RunLifecycle { - status: RunStatus::Submitted, + status: RunStatus::Submitted, pending_control: None, - queue_position: None, - error: None, - archived: false, - archived_at: None, + queue_position: None, + error: None, + archived, + archived_at: None, }, sandbox: None, models: Vec::new(),