diff --git a/docs/internal/mcp-server-qa-test-plan.md b/docs/internal/mcp-server-qa-test-plan.md index 3d1079ddb..422a38225 100644 --- a/docs/internal/mcp-server-qa-test-plan.md +++ b/docs/internal/mcp-server-qa-test-plan.md @@ -9,12 +9,12 @@ 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 -- **I7 / I9 — Misleading "Run not found." on terminal runs**: `message` or `cancel` against a terminal run returns `Run not found.` (which means "not in managed-runs map"), not a status-appropriate error like "run is already terminal". Confusing to operators. - **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. ### 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`. - **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. @@ -184,11 +184,11 @@ Source: `run_tools/interact.rs:201` - [ ] **I4** Steer a running LLM agent — **DEFERRED** (requires an active LLM agent stage; would burn LLM tokens; can be exercised manually once the answer bug below is resolved). - [ ] **I5** `interrupt=true` — **DEFERRED** along with I4. - [x] **I6** Missing `message` → `message is required for action message`. — **PASS**. -- [x] **I7** Message a terminal run → `Run not found.` — **FINDING / BUG**: should distinguish "run is terminal / no managed engine" from "run doesn't exist". +- [x] **I7** Message a terminal run → initially returned `Run not found.`. — **FIXED**: durable terminal runs without a live managed engine now return `409 run_not_steerable`; true missing runs remain `404`. #### `cancel` - [x] **I8** Cancel a `gh-list` run during `starting`. Returns summary at request time (status=`starting`). Subsequent `gather` returned terminal `failed` within 5s; `get` projection shows `status: {kind: "failed", reason: "cancelled"}` and `conclusion.failure_reason: "Pipeline cancelled"`. — **PASS** + **observation**: `cancel`'s returned summary is a snapshot at request time, not the eventual terminal status. -- [x] **I9** Cancel an already-terminal run → `Run not found.` — **FINDING / BUG**: same misleading error as I7. Should say "run is terminal" instead. +- [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). diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index e099f8d75..ae6d9779f 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -2322,6 +2322,15 @@ async fn load_pending_control( .and_then(|summary| summary.lifecycle.pending_control)) } +async fn durable_run_status(state: &AppState, run_id: RunId) -> anyhow::Result> { + Ok(state + .store + .runs() + .find(&run_id) + .await? + .map(|summary| summary.lifecycle.status)) +} + fn fail_managed_run(state: &Arc, run_id: RunId, reason: FailureReason, message: String) { let mut runs = state.runs.lock().expect("runs lock poisoned"); if let Some(managed_run) = runs.get_mut(&run_id) { diff --git a/lib/crates/fabro-server/src/server/handler/lifecycle.rs b/lib/crates/fabro-server/src/server/handler/lifecycle.rs index 469f74bc1..80e5d2918 100644 --- a/lib/crates/fabro-server/src/server/handler/lifecycle.rs +++ b/lib/crates/fabro-server/src/server/handler/lifecycle.rs @@ -5,7 +5,7 @@ use super::super::{ Principal, RequiredUser, Response, RewindRequest, RewindResponse, Router, RunAnswerTransport, RunControlAction, RunExecutionMode, RunId, RunStatus, StartRunRequest, State, StatusCode, Storage, TimelineEntryResponse, WORKER_CANCEL_GRACE, WorkflowError, append_control_request, - get, load_pending_control, managed_run, operations, parse_run_id_path, + durable_run_status, get, load_pending_control, managed_run, operations, parse_run_id_path, persist_cancelled_run_status, post, reject_if_archived, sleep, update_live_run_from_event, workflow_event, }; @@ -171,7 +171,7 @@ async fn cancel_run( .into_response(); } }; - let (persist_cancelled_status, answer_transport, cancel_token, cancel_tx, worker_pid) = { + let cancel_target = { let mut runs = state.runs.lock().expect("runs lock poisoned"); match runs.get_mut(&id) { Some(managed_run) => match managed_run.status { @@ -192,7 +192,7 @@ async fn cancel_run( reason: FailureReason::Cancelled, }; } - ( + Some(( persist_cancelled_status, managed_run.answer_transport.clone(), managed_run.cancel_token.clone(), @@ -200,16 +200,21 @@ async fn cancel_run( .then(|| managed_run.cancel_tx.take()) .flatten(), managed_run.worker_pid, - ) + )) } _ => { return ApiError::new(StatusCode::CONFLICT, "Run is not cancellable.") .into_response(); } }, - None => return ApiError::not_found("Run not found.").into_response(), + None => None, } }; + let Some((persist_cancelled_status, answer_transport, cancel_token, cancel_tx, worker_pid)) = + cancel_target + else { + return unmanaged_cancel_response(state.as_ref(), id).await; + }; if pending_control != Some(RunControlAction::Cancel) { if let Err(err) = append_control_request( @@ -256,6 +261,23 @@ async fn cancel_run( run_response(state.as_ref(), id, StatusCode::OK).await } +async fn unmanaged_cancel_response(state: &AppState, id: RunId) -> Response { + match durable_run_status(state, id).await { + Ok(Some(status)) if status.is_terminal() => ApiError::new( + StatusCode::CONFLICT, + "Run is already terminal and cannot be cancelled.", + ) + .into_response(), + Ok(Some(_)) => { + ApiError::new(StatusCode::CONFLICT, "Run is not cancellable.").into_response() + } + Ok(None) => ApiError::not_found("Run not found.").into_response(), + Err(err) => { + ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() + } + } +} + /// How `pause_run` should enact the transition, chosen from the current run /// status. enum PauseMode { diff --git a/lib/crates/fabro-server/src/server/handler/steer.rs b/lib/crates/fabro-server/src/server/handler/steer.rs index 6ee20497c..da7b30f58 100644 --- a/lib/crates/fabro-server/src/server/handler/steer.rs +++ b/lib/crates/fabro-server/src/server/handler/steer.rs @@ -9,7 +9,9 @@ use fabro_api::types::SteerRunRequest; use fabro_types::Principal; use fabro_workflow::run_status::RunStatus; -use super::super::{AnswerTransportError, AppState, parse_run_id_path, reject_if_archived}; +use super::super::{ + AnswerTransportError, AppState, durable_run_status, parse_run_id_path, reject_if_archived, +}; use crate::error::ApiError; use crate::principal_middleware::RequiredUser; @@ -77,73 +79,72 @@ async fn control_run( // Status + steerability gate. Take the answer_transport snapshot under // the same lock so we can hand it off without further state races. - let answer_transport = { + let managed_answer_transport = { let runs = state.runs.lock().expect("runs lock poisoned"); - let Some(managed_run) = runs.get(&id) else { - return ApiError::not_found("Run not found.").into_response(); - }; - match managed_run.status { - RunStatus::Blocked { .. } => { - return ApiError::with_code( - StatusCode::CONFLICT, - "Run is blocked on a question; use the interview-answer endpoint instead.", - "use_answer_endpoint", - ) - .into_response(); + match runs.get(&id) { + Some(managed_run) => { + match managed_run.status { + RunStatus::Blocked { .. } => { + return ApiError::with_code( + StatusCode::CONFLICT, + "Run is blocked on a question; use the interview-answer endpoint \ + instead.", + "use_answer_endpoint", + ) + .into_response(); + } + RunStatus::Submitted + | RunStatus::Queued + | RunStatus::Starting + | RunStatus::Paused { .. } => { + return ApiError::with_code( + StatusCode::CONFLICT, + "Run is not currently running.", + "run_not_steerable", + ) + .into_response(); + } + RunStatus::Failed { .. } + | RunStatus::Succeeded { .. } + | RunStatus::Removing + | RunStatus::Dead => { + return terminal_control_response(&control); + } + RunStatus::Running => {} + } + // Steerability predicate. Best-effort, target-oriented: + // - If at least one API-mode session is active → forward. + // - Else if no agent stages are active at all → forward (worker hub buffers + // for the next session). + // - Else (active agents exist but all are CLI-mode) → 409. + if managed_run.active_api_stages.is_empty() + && !managed_run.active_cli_stages.is_empty() + { + return ApiError::with_code( + StatusCode::CONFLICT, + "All currently running agent stages are CLI-mode and cannot be steered.", + "cli_agent_not_steerable", + ) + .into_response(); + } + if managed_run.active_api_stages.is_empty() && control.requires_active_api_session() + { + return ApiError::with_code( + StatusCode::CONFLICT, + "Run has no active API-mode agent session.", + "no_active_api_session", + ) + .into_response(); + } + Some(managed_run.answer_transport.clone()) } - RunStatus::Submitted - | RunStatus::Queued - | RunStatus::Starting - | RunStatus::Paused { .. } => { - return ApiError::with_code( - StatusCode::CONFLICT, - "Run is not currently running.", - "run_not_steerable", - ) - .into_response(); - } - RunStatus::Failed { .. } - | RunStatus::Succeeded { .. } - | RunStatus::Removing - | RunStatus::Dead => { - let code = if matches!(&control, RunControlRequest::Interrupt) { - "run_not_interruptible" - } else { - "run_not_steerable" - }; - return ApiError::with_code( - StatusCode::CONFLICT, - "Run is no longer steerable.", - code, - ) - .into_response(); - } - RunStatus::Running => {} + None => None, } - // Steerability predicate. Best-effort, target-oriented: - // - If at least one API-mode session is active → forward. - // - Else if no agent stages are active at all → forward (worker hub buffers - // for the next session). - // - Else (active agents exist but all are CLI-mode) → 409. - if managed_run.active_api_stages.is_empty() && !managed_run.active_cli_stages.is_empty() { - return ApiError::with_code( - StatusCode::CONFLICT, - "All currently running agent stages are CLI-mode and cannot be steered.", - "cli_agent_not_steerable", - ) - .into_response(); - } - if managed_run.active_api_stages.is_empty() && control.requires_active_api_session() { - return ApiError::with_code( - StatusCode::CONFLICT, - "Run has no active API-mode agent session.", - "no_active_api_session", - ) - .into_response(); - } - managed_run.answer_transport.clone() }; + let Some(answer_transport) = managed_answer_transport else { + return unmanaged_control_response(state.as_ref(), id, &control).await; + }; let Some(answer_transport) = answer_transport else { return ApiError::with_code( StatusCode::SERVICE_UNAVAILABLE, @@ -178,3 +179,32 @@ async fn control_run( .into_response(), } } + +fn terminal_control_response(control: &RunControlRequest) -> Response { + let code = if matches!(control, RunControlRequest::Interrupt) { + "run_not_interruptible" + } else { + "run_not_steerable" + }; + ApiError::with_code(StatusCode::CONFLICT, "Run is no longer steerable.", code).into_response() +} + +async fn unmanaged_control_response( + state: &AppState, + id: fabro_types::RunId, + control: &RunControlRequest, +) -> Response { + match durable_run_status(state, id).await { + Ok(Some(status)) if status.is_terminal() => terminal_control_response(control), + Ok(Some(_)) => ApiError::with_code( + StatusCode::SERVICE_UNAVAILABLE, + "Run has no live worker control channel.", + "worker_control_unavailable", + ) + .into_response(), + Ok(None) => ApiError::not_found("Run not found.").into_response(), + Err(err) => { + ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() + } + } +} diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index 0e0d7aa0a..7f717f93f 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -6877,6 +6877,40 @@ async fn cancel_nonexistent_run_returns_not_found() { assert_status!(response, StatusCode::NOT_FOUND).await; } +#[tokio::test] +async fn cancel_terminal_durable_run_returns_conflict() { + let state = test_app_state(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let run_id = fixtures::RUN_1; + create_durable_run_with_events(&state, run_id, &[ + workflow_event::Event::WorkflowRunCompleted { + duration_ms: 1000, + artifact_count: 0, + status: "succeeded".to_string(), + reason: SuccessReason::Completed, + total_usd_micros: None, + final_git_commit_sha: None, + final_patch: None, + diff_summary: None, + billing: None, + }, + ]) + .await; + + let req = Request::builder() + .method("POST") + .uri(api(&format!("/runs/{run_id}/cancel"))) + .body(Body::empty()) + .unwrap(); + + let response = app.oneshot(req).await.unwrap(); + let body = response_json!(response, StatusCode::CONFLICT).await; + assert_eq!( + body["errors"][0]["detail"], + "Run is already terminal and cannot be cancelled." + ); +} + #[tokio::test] async fn steer_nonexistent_run_returns_not_found() { let app = test_app_with(); @@ -6893,6 +6927,39 @@ async fn steer_nonexistent_run_returns_not_found() { assert_status!(response, StatusCode::NOT_FOUND).await; } +#[tokio::test] +async fn steer_terminal_durable_run_returns_run_not_steerable() { + let state = test_app_state(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let run_id = fixtures::RUN_1; + create_durable_run_with_events(&state, run_id, &[ + workflow_event::Event::WorkflowRunCompleted { + duration_ms: 1000, + artifact_count: 0, + status: "succeeded".to_string(), + reason: SuccessReason::Completed, + total_usd_micros: None, + final_git_commit_sha: None, + final_patch: None, + diff_summary: None, + billing: None, + }, + ]) + .await; + + let req = Request::builder() + .method("POST") + .uri(api(&format!("/runs/{run_id}/steer"))) + .header("content-type", "application/json") + .body(Body::from(r#"{"text":"try again"}"#)) + .unwrap(); + + let response = app.oneshot(req).await.unwrap(); + let body = response_json!(response, StatusCode::CONFLICT).await; + assert_eq!(body["errors"][0]["code"], "run_not_steerable"); + assert_eq!(body["errors"][0]["detail"], "Run is no longer steerable."); +} + #[tokio::test] async fn steer_empty_text_returns_bad_request() { let state = test_app_state();