From c6356cbd771a093e83cb69cf32adeaf219390460 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Fri, 22 May 2026 09:39:20 -0400 Subject: [PATCH] feat: improve run board, thread, and MCP create flows (#347) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This branch improves several run-management surfaces that agents and users rely on: archived runs now stay visible and ordered correctly in the board view, pair-session messages appear in the stage Thread tab, and the `fabro_run_create` MCP tool accepts the workflow-string shorthand it advertises. ## Changes - Updates the web board cache invalidation and archived-column handling so archive/unarchive actions refresh both active and archived board queries and keep archived runs in a predictable column position. - Adds pair user/system message events to stage activity parsing, Thread rendering, search, details, and DNA timeline items. - Aligns `fabro_run_create` MCP runtime deserialization and `tools/list` schema so each run entry may be either a workflow string or a full create spec object. ## Test Plan - `cargo nextest run -p fabro-tool -p fabro-mcp-server` - `cargo nextest run -p fabro-cli stdio_server_initializes_and_lists_run_tools mcp_create_string_shorthand_deserializes_before_auth mcp_create_validation_errors_happen_before_auth_or_network mcp_create_and_search_manage_real_runs_with_cli_auth` - `cargo +nightly-2026-04-14 fmt --check --all` --- [![Compound Engineering](https://img.shields.io/badge/Compound_Engineering-6366f1)](https://github.com/EveryInc/compound-engineering-plugin) 🤖 Generated with GPT-5 via [Codex](https://openai.com/codex) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- apps/fabro-web/app/lib/board-cache.ts | 15 + apps/fabro-web/app/lib/board-events.ts | 3 +- apps/fabro-web/app/lib/mutations.test.ts | 57 +++ apps/fabro-web/app/lib/mutations.ts | 6 +- apps/fabro-web/app/lib/run-events.test.tsx | 9 + apps/fabro-web/app/lib/run-events.ts | 2 + apps/fabro-web/app/routes/run-stages.test.ts | 43 ++ apps/fabro-web/app/routes/run-stages.tsx | 65 ++- apps/fabro-web/app/routes/runs.test.tsx | 40 ++ apps/fabro-web/app/routes/runs.tsx | 13 +- docs/internal/mcp-server-qa-test-plan.md | 2 + docs/public/agents/mcp.mdx | 22 + lib/crates/fabro-cli/tests/it/cmd/mcp.rs | 73 ++++ lib/crates/fabro-mcp-server/src/server.rs | 44 ++ lib/crates/fabro-tool/src/create.rs | 380 +++++++++++++++--- lib/crates/fabro-tool/src/lib.rs | 6 +- .../fabro-workflow/src/handler/llm/api.rs | 6 +- 17 files changed, 724 insertions(+), 62 deletions(-) create mode 100644 apps/fabro-web/app/lib/board-cache.ts create mode 100644 apps/fabro-web/app/lib/mutations.test.ts diff --git a/apps/fabro-web/app/lib/board-cache.ts b/apps/fabro-web/app/lib/board-cache.ts new file mode 100644 index 000000000..db95dceeb --- /dev/null +++ b/apps/fabro-web/app/lib/board-cache.ts @@ -0,0 +1,15 @@ +import type { Key } from "swr"; + +import { queryKeys } from "./query-keys"; + +type MutateBoardRuns = (key: Key) => unknown; + +export function boardRunCacheKeys(): Key[] { + return [queryKeys.boards.runs(false), queryKeys.boards.runs(true)]; +} + +export function mutateBoardRunCaches(mutate: MutateBoardRuns) { + for (const key of boardRunCacheKeys()) { + void mutate(key); + } +} diff --git a/apps/fabro-web/app/lib/board-events.ts b/apps/fabro-web/app/lib/board-events.ts index 77e44f520..1ef425314 100644 --- a/apps/fabro-web/app/lib/board-events.ts +++ b/apps/fabro-web/app/lib/board-events.ts @@ -5,6 +5,7 @@ import { subscribeToCrossTabSse, type CrossTabSseCoordinator, } from "./cross-tab-sse"; +import { boardRunCacheKeys } from "./board-cache"; import { queryKeys } from "./query-keys"; import { createBrowserEventSource, @@ -85,7 +86,7 @@ function boardInvalidation(payload: EventPayload) { } function boardRunKeys() { - return [queryKeys.boards.runs(false), queryKeys.boards.runs(true)]; + return boardRunCacheKeys(); } export function useBoardEvents() { diff --git a/apps/fabro-web/app/lib/mutations.test.ts b/apps/fabro-web/app/lib/mutations.test.ts new file mode 100644 index 000000000..186ae2c42 --- /dev/null +++ b/apps/fabro-web/app/lib/mutations.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, mock, test, beforeEach } from "bun:test"; + +import { queryKeys } from "./query-keys"; + +const mutateMock = mock((..._args: unknown[]) => Promise.resolve(undefined)); +let lastMutationOptions: { onSuccess?: (result: unknown) => void } | null = null; + +const useSWRMutationMock = mock((_key: unknown, _fetcher: unknown, options: unknown) => { + lastMutationOptions = options as { onSuccess?: (result: unknown) => void }; + return {}; +}); + +mock.module("swr", () => ({ + useSWRConfig: () => ({ mutate: mutateMock }), +})); + +mock.module("swr/mutation", () => ({ + default: useSWRMutationMock, +})); + +mock.module("./api-client", () => ({ + apiData: mock(), + authApi: {}, + humanInTheLoopApi: {}, + runsApi: {}, +})); + +mock.module("./run-actions", () => ({ + archiveRun: mock(), + cancelRun: mock(), + isLifecycleActionError: () => false, + unarchiveRun: mock(), +})); + +const { useArchiveRun } = await import("./mutations"); + +beforeEach(() => { + mutateMock.mockClear(); + useSWRMutationMock.mockClear(); + lastMutationOptions = null; +}); + +describe("lifecycle mutations", () => { + test("successful archive invalidates both board run caches", () => { + useArchiveRun("run-1"); + + lastMutationOptions?.onSuccess?.({ + intent: "archive", + ok: true, + run: {}, + }); + + const keys = mutateMock.mock.calls.map((call) => call[0]); + expect(keys).toContainEqual(queryKeys.boards.runs(false)); + expect(keys).toContainEqual(queryKeys.boards.runs(true)); + }); +}); diff --git a/apps/fabro-web/app/lib/mutations.ts b/apps/fabro-web/app/lib/mutations.ts index 5852a7544..889245bba 100644 --- a/apps/fabro-web/app/lib/mutations.ts +++ b/apps/fabro-web/app/lib/mutations.ts @@ -14,6 +14,7 @@ import { humanInTheLoopApi, runsApi, } from "./api-client"; +import { mutateBoardRunCaches } from "./board-cache"; import { queryKeys } from "./query-keys"; import type { LifecycleAction, LifecycleActionError } from "./run-actions"; import { @@ -97,7 +98,7 @@ function useLifecycleMutation( onSuccess: (result) => { if (!id || !result.ok) return; void mutate(queryKeys.runs.detail(id)); - void mutate(queryKeys.boards.runs()); + mutateBoardRunCaches(mutate); void mutate(queryKeys.runs.billing(id)); }, }, @@ -116,8 +117,7 @@ export function useUpdateRunTitle(id: string | undefined) { onSuccess: (run) => { if (!id) return; void mutate(queryKeys.runs.detail(id), run, { revalidate: false }); - void mutate(queryKeys.boards.runs()); - void mutate(queryKeys.boards.runs(true)); + mutateBoardRunCaches(mutate); }, }, ); diff --git a/apps/fabro-web/app/lib/run-events.test.tsx b/apps/fabro-web/app/lib/run-events.test.tsx index e6f7f48d7..812035d37 100644 --- a/apps/fabro-web/app/lib/run-events.test.tsx +++ b/apps/fabro-web/app/lib/run-events.test.tsx @@ -77,6 +77,15 @@ describe("queryKeysForRunEvent", () => { queryKeys.runs.stageEvents("run-1", "nap@1"), ]); }); + + test("pair messages invalidate the stage events query", () => { + expect(queryKeysForRunEvent("run-1", "agent.pair.user_message", "nap@1")).toEqual([ + queryKeys.runs.stageEvents("run-1", "nap@1"), + ]); + expect(queryKeysForRunEvent("run-1", "agent.pair.system_message", "nap@1")).toEqual([ + queryKeys.runs.stageEvents("run-1", "nap@1"), + ]); + }); }); describe("subscribeToRunEvents", () => { diff --git a/apps/fabro-web/app/lib/run-events.ts b/apps/fabro-web/app/lib/run-events.ts index 6ac518451..54599a065 100644 --- a/apps/fabro-web/app/lib/run-events.ts +++ b/apps/fabro-web/app/lib/run-events.ts @@ -73,6 +73,8 @@ export const STAGE_ACTIVITY_EVENT_TYPES = [ "agent.tool.completed", "agent.steering.injected", "agent.interrupt.injected", + "agent.pair.user_message", + "agent.pair.system_message", "command.started", "command.completed", ] as const; diff --git a/apps/fabro-web/app/routes/run-stages.test.ts b/apps/fabro-web/app/routes/run-stages.test.ts index 57f447e90..20a36c9cd 100644 --- a/apps/fabro-web/app/routes/run-stages.test.ts +++ b/apps/fabro-web/app/routes/run-stages.test.ts @@ -228,6 +228,49 @@ describe("eventsToActivity", () => { ]); }); + test("renders pair messages as transcript turns for the matching stage", () => { + const events: EventEnvelope[] = [ + envelope(1, { + event: "agent.pair.system_message", + ts: "2026-04-09T12:00:00Z", + stage_id: "nap@1", + node_id: "nap", + properties: { + text: "A human has joined this workflow run for live pairing.", + kind: "human_joined", + visit: 1, + }, + }), + envelope(2, { + event: "agent.pair.user_message", + ts: "2026-04-09T12:00:05Z", + stage_id: "nap@1", + node_id: "nap", + properties: { text: "try a smaller diff", visit: 1 }, + }), + envelope(3, { + event: "agent.pair.user_message", + ts: "2026-04-09T12:00:06Z", + stage_id: "other@1", + node_id: "other", + properties: { text: "wrong stage", visit: 1 }, + }), + ]; + + expect(eventsToActivity(events, "nap@1")).toEqual([ + { + kind: "pair_system", + ts: "2026-04-09T12:00:00Z", + content: "A human has joined this workflow run for live pairing.", + }, + { + kind: "pair_user", + ts: "2026-04-09T12:00:05Z", + content: "try a smaller diff", + }, + ]); + }); + test("renders prompt.completed as an assistant turn for prompt-shape stages", () => { const events: EventEnvelope[] = [ envelope(1, { diff --git a/apps/fabro-web/app/routes/run-stages.tsx b/apps/fabro-web/app/routes/run-stages.tsx index 6087955d5..35e8af1f6 100644 --- a/apps/fabro-web/app/routes/run-stages.tsx +++ b/apps/fabro-web/app/routes/run-stages.tsx @@ -69,6 +69,8 @@ type TurnType = | { kind: "system"; ts: string; content: string } | { kind: "steer"; ts: string; content: string } | { kind: "interrupt"; ts: string; content: string } + | { kind: "pair_user"; ts: string; content: string } + | { kind: "pair_system"; ts: string; content: string } | { kind: "assistant"; ts: string; content: string; inputTokens: number; outputTokens: number } | { kind: "tool"; ts: string; toolName: string; input: string; result: string; isError: boolean; durationMs: number } | { @@ -97,13 +99,24 @@ type PanelSelection = ThreadDnaSelection; const STAGE_ACTIVITY_EVENT_SET = new Set(STAGE_ACTIVITY_EVENT_TYPES); -const EVENT_KINDS = ["system", "steer", "interrupt", "assistant", "tool", "command"] as const; +const EVENT_KINDS = [ + "system", + "steer", + "interrupt", + "pair_user", + "pair_system", + "assistant", + "tool", + "command", +] as const; type EventKind = (typeof EVENT_KINDS)[number]; const EVENT_KIND_LABEL: Record = { system: "System", steer: "Steer", interrupt: "Interrupt", + pair_user: "Human", + pair_system: "System", assistant: "Agent", tool: "Tool", command: "Command", @@ -231,6 +244,20 @@ export function eventsToActivity(events: EventEnvelope[], stageId: string): Turn case "agent.interrupt.injected": turns.push({ kind: "interrupt", ts: e.ts, content: "Agent interrupted" }); break; + case "agent.pair.user_message": { + const text = getString(props, "text") ?? e.text ?? ""; + if (text) { + turns.push({ kind: "pair_user", ts: e.ts, content: text }); + } + break; + } + case "agent.pair.system_message": { + const text = getString(props, "text") ?? e.text ?? ""; + if (text) { + turns.push({ kind: "pair_system", ts: e.ts, content: text }); + } + break; + } case "agent.tool.started": { const callId = getString(props, "tool_call_id") ?? e.tool_call_id ?? ""; const args = props.arguments ?? e.arguments; @@ -423,6 +450,26 @@ export function buildThreadDnaItems( }); prevEndMs = tsMs; break; + case "pair_user": + out.push({ + category: "user", + label: "pair.user", + startMs: Math.max(0, tsMs - anchorMs), + durationMs: 0, + selection, + }); + prevEndMs = tsMs; + break; + case "pair_system": + out.push({ + category: "system", + label: "pair.system", + startMs: Math.max(0, tsMs - anchorMs), + durationMs: 0, + selection, + }); + prevEndMs = tsMs; + break; case "assistant": { // turn.ts is the moment the assistant message arrived (end of // generation). Its bar represents the gap from the last activity @@ -519,6 +566,10 @@ function turnLabel(turn: TurnType): string { return "Steer"; case "interrupt": return "Interrupt"; + case "pair_user": + return "Human"; + case "pair_system": + return "System"; case "assistant": return "Agent"; case "tool": @@ -536,6 +587,10 @@ function turnTone(turn: TurnType): string { return "bg-overlay-strong text-fg-2"; case "interrupt": return "bg-coral/15 text-coral"; + case "pair_user": + return "bg-overlay-strong text-fg-2"; + case "pair_system": + return "bg-amber/15 text-amber"; case "assistant": return "bg-teal-500/15 text-teal-500"; case "tool": @@ -582,6 +637,8 @@ export function turnSummary(turn: TurnType): string { case "system": case "steer": case "interrupt": + case "pair_user": + case "pair_system": case "assistant": return oneLine(turn.content); case "tool": @@ -611,6 +668,8 @@ export function turnMetric(turn: TurnType): string | null { case "steer": case "interrupt": case "system": + case "pair_user": + case "pair_system": return null; } } @@ -620,6 +679,8 @@ export function searchableText(turn: TurnType): string { case "system": case "steer": case "interrupt": + case "pair_user": + case "pair_system": case "assistant": return turn.content; case "tool": @@ -775,6 +836,8 @@ function EventDetails({ {(turn.kind === "system" || turn.kind === "steer" || turn.kind === "interrupt" || + turn.kind === "pair_user" || + turn.kind === "pair_system" || turn.kind === "assistant") && ( diff --git a/apps/fabro-web/app/routes/runs.test.tsx b/apps/fabro-web/app/routes/runs.test.tsx index 54722ae0d..aab1c7abe 100644 --- a/apps/fabro-web/app/routes/runs.test.tsx +++ b/apps/fabro-web/app/routes/runs.test.tsx @@ -3,6 +3,7 @@ import type { BoardColumn, Run } from "@qltysh/fabro-api-client"; import { buildBoardColumns, + placeArchivedColumnLast, runsQuickStartCommands, shouldRefreshBoardForEvent, } from "./runs"; @@ -131,6 +132,45 @@ describe("runs route board mapping", () => { expect(columns.some((column) => column.id === "archived")).toBe(false); }); + test("puts archived last when the archived view is active", () => { + const columns = buildBoardColumns({ + columns: [ + { id: "queued", name: "Queued" }, + { id: "initializing", name: "Initializing" }, + { id: "running", name: "Running" }, + { id: "blocked", name: "Blocked" }, + { id: "succeeded", name: "Succeeded" }, + { id: "failed", name: "Failed" }, + { id: "archived", name: "Archived" }, + ], + data: [ + boardRun("running-run", "running"), + boardRun("succeeded-run", "succeeded"), + boardRun("archived-run", "archived"), + ], + meta: { has_more: false }, + }); + + expect(placeArchivedColumnLast(columns, true).map((column) => column.id)).toEqual([ + "queued", + "initializing", + "running", + "blocked", + "succeeded", + "failed", + "archived", + ]); + expect(placeArchivedColumnLast(columns, false).map((column) => column.id)).toEqual([ + "queued", + "initializing", + "running", + "blocked", + "succeeded", + "failed", + "archived", + ]); + }); + test("refreshes for blocked status and interview events", () => { expect(shouldRefreshBoardForEvent("run.queued")).toBe(true); expect(shouldRefreshBoardForEvent("run.blocked")).toBe(true); diff --git a/apps/fabro-web/app/routes/runs.tsx b/apps/fabro-web/app/routes/runs.tsx index 6eb1652cd..f24d48edc 100644 --- a/apps/fabro-web/app/routes/runs.tsx +++ b/apps/fabro-web/app/routes/runs.tsx @@ -28,9 +28,9 @@ import { EmptyState } from "../components/state"; import { InlineMarkdown } from "../components/inline-markdown"; import { PullRequestChip } from "../components/pull-request-chip"; import { useToast } from "../components/toast"; +import { mutateBoardRunCaches } from "../lib/board-cache"; import { shouldRefreshBoardForEvent, useBoardEvents } from "../lib/board-events"; import { useAuthConfig, useBoardsRuns, useSystemInfo } from "../lib/queries"; -import { queryKeys } from "../lib/query-keys"; import { archiveRun, canArchive } from "../lib/run-actions"; import type { BoardColumn, PaginatedBoardRunList } from "@qltysh/fabro-api-client"; @@ -114,6 +114,13 @@ export function buildBoardColumns(response: BoardRunsResponse): Column[] { }); } +export function placeArchivedColumnLast(columns: Column[], includeArchived: boolean): Column[] { + if (!includeArchived) return columns; + const archived = columns.find((column) => column.id === "archived"); + if (archived == null) return columns; + return [...columns.filter((column) => column.id !== "archived"), archived]; +} + function boardLifecycleStatusLabel(run: Pick): string | null { if (run.lifecycleStatusLabel == null) return null; if (run.column === "initializing") return null; @@ -459,7 +466,7 @@ function ColumnActionsMenu({ column }: { column: Column }) { } } finally { setPending(false); - void mutate(queryKeys.boards.runs()); + mutateBoardRunCaches(mutate); } } @@ -871,7 +878,7 @@ export default function Runs() { (sum, col) => sum + col.items.length, 0, ); - const visibleColumns = filteredColumns.filter( + const visibleColumns = placeArchivedColumnLast(filteredColumns, includeArchived).filter( (col) => col.id !== "queued" || col.items.length > 0, ); diff --git a/docs/internal/mcp-server-qa-test-plan.md b/docs/internal/mcp-server-qa-test-plan.md index c3f6a6b5e..d81ebd5db 100644 --- a/docs/internal/mcp-server-qa-test-plan.md +++ b/docs/internal/mcp-server-qa-test-plan.md @@ -12,6 +12,7 @@ Live list of bugs and notable observations surfaced during the sweep. Each entry None currently open. ### Rechecked / no longer open +- **C18 — `fabro_run_create` shorthand schema/runtime mismatch**: fixed on 2026-05-22. `runs: ["sleeper"]` is now accepted as workflow-selector shorthand, object-form specs remain supported for create options, and `tools/list` advertises both item shapes. Covered by `fabro_run_create_tool_advertises_string_and_object_run_specs`, `stdio_server_initializes_and_lists_run_tools`, and `mcp_create_string_shorthand_deserializes_before_auth`. - **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`. @@ -64,6 +65,7 @@ Source: `run_tools/create.rs:124` - [x] **C14** 51 entries → `runs must contain no more than 50 item(s)`. — **PASS**. - [x] **C15** Missing required `workflow` → MCP layer `-32602: missing field 'workflow'`. — **PASS**. - [x] **C16** Unknown workflow slug → `Unknown workflow 'X'\n\nAvailable workflows: ...`. — **PASS** (very helpful — lists available workflows). +- [x] **C18** String shorthand `runs: ["gh-list"]` → accepted as the workflow selector. `tools/list` should show `runs.items.anyOf` with both a string branch and an object branch requiring `workflow`. — **COVERED by automated regression tests added 2026-05-22**. ### Failure semantics - [x] **C17** Invalid sandbox name → `failed to resolve manifest settings: run.sandbox.provider: invalid value - unknown sandbox provider: this-sandbox-does-not-exist`. — **PASS**. Error raised at manifest-resolve time before any run record is created (no orphaned submitted run). diff --git a/docs/public/agents/mcp.mdx b/docs/public/agents/mcp.mdx index a3bf91216..e44976a1b 100644 --- a/docs/public/agents/mcp.mdx +++ b/docs/public/agents/mcp.mdx @@ -34,6 +34,28 @@ Pass `--server` when the MCP client should connect to a specific Fabro server, o | `fabro_run_gather` | Wait for runs to reach terminal states, returning current state on timeout. | | `fabro_run_events` | List, inspect, or search stored events for a run. | +For a simple create call, `fabro_run_create` accepts a workflow selector string: + +```json +{ "runs": ["sleeper"] } +``` + +Use the object form when you need create options: + +```json +{ + "runs": [ + { + "workflow": "sleeper", + "auto_approve": true, + "dry_run": true, + "labels": { "source": "mcp" }, + "start": true + } + ] +} +``` + Run summaries returned by the MCP server include parent metadata. Use `parent_id` on `fabro_run_create` to create a child run, `parent_id` on `fabro_run_search` to list direct children, and the `link_parent` or `unlink_parent` actions on `fabro_run_interact` to change an existing run's parent. ## Fabro agents as MCP clients diff --git a/lib/crates/fabro-cli/tests/it/cmd/mcp.rs b/lib/crates/fabro-cli/tests/it/cmd/mcp.rs index b24796bf6..b48ac90fd 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/mcp.rs @@ -454,6 +454,12 @@ async fn stdio_server_initializes_and_lists_run_tools() { .is_some_and(serde_json::Value::is_object), "fabro_run_interact.answer should have an object JSON Schema: {interact_schema}" ); + let create_schema = tools + .iter() + .find(|(name, _, _)| name == "fabro_run_create") + .map(|(_, _, schema)| schema) + .expect("fabro_run_create tool should be listed"); + assert_create_schema_accepts_string_and_object_specs(create_schema); client .shutdown() .await @@ -1494,6 +1500,44 @@ async fn mcp_create_validation_errors_happen_before_auth_or_network() { .expect("MCP client should shut down"); } +#[tokio::test(flavor = "multi_thread")] +async fn mcp_create_string_shorthand_deserializes_before_auth() { + let context = test_context!(); + let harness = + RealAuthHarness::start_with_dev_token(fabro_test::GitHubAppState::default()).await; + let target_url = harness.api_target(); + let workflow = context.install_fixture("simple.fabro"); + let client = spawn_mcp_client(&context, &["--server", &target_url]).await; + + let result = client + .call_tool( + "fabro_run_create", + serde_json::json!({ "runs": [workflow] }), + std::time::Duration::from_secs(30), + ) + .await + .expect("string shorthand should deserialize and return a tool-level auth error"); + assert_eq!(result.is_error, Some(true), "tool should return error"); + let error = result + .content + .first() + .and_then(|content| serde_json::to_value(content).ok()) + .and_then(|content| content["text"].as_str().map(ToOwned::to_owned)) + .expect("tool error should include text"); + assert!(!error.contains("CreateRunSpec"), "{error}"); + assert!( + error.contains("Run `fabro auth login` to authenticate."), + "{error}" + ); + assert_eq!(client.list_tools().await.unwrap().len(), 6); + + client + .shutdown() + .await + .expect("MCP client should shut down"); + harness.shutdown().await; +} + #[tokio::test(flavor = "multi_thread")] async fn mcp_interact_answer_validation_happens_before_auth_or_network() { let context = test_context!(); @@ -2203,6 +2247,35 @@ async fn call_tool_error_text( .expect("tool error should include text") } +fn assert_create_schema_accepts_string_and_object_specs(schema: &serde_json::Value) { + let variants = schema + .pointer("/properties/runs/items/anyOf") + .and_then(serde_json::Value::as_array) + .expect("fabro_run_create runs items should use anyOf"); + + assert!( + variants.iter().any(|variant| variant["type"] == "string"), + "fabro_run_create should advertise workflow string shorthand: {schema}" + ); + let object_variant = variants + .iter() + .find(|variant| variant["type"] == "object") + .unwrap_or_else(|| { + panic!("fabro_run_create should advertise object create specs: {schema}") + }); + assert!( + object_variant.pointer("/properties/workflow").is_some(), + "object create spec should include workflow property: {schema}" + ); + assert!( + object_variant + .get("required") + .and_then(serde_json::Value::as_array) + .is_some_and(|required| required.iter().any(|field| field == "workflow")), + "object create spec should require workflow: {schema}" + ); +} + async fn create_mcp_run(client: &McpClient, workflow: PathBuf, start: bool) -> String { let create = call_tool_json( client, diff --git a/lib/crates/fabro-mcp-server/src/server.rs b/lib/crates/fabro-mcp-server/src/server.rs index 4208591e3..1735ed2a4 100644 --- a/lib/crates/fabro-mcp-server/src/server.rs +++ b/lib/crates/fabro-mcp-server/src/server.rs @@ -257,4 +257,48 @@ mod tests { assert!(!schema_text.contains("\"node_id\"")); assert!(!schema_text.contains("\"visit\"")); } + + #[test] + fn fabro_run_create_tool_advertises_string_and_object_run_specs() { + let settings = FabroMcpServerSettings { + cwd: PathBuf::from("."), + config_path: PathBuf::from("fabro.toml"), + client_factory: Arc::new(|| { + Box::pin(async { panic!("client should not be constructed while listing tools") }) + }), + }; + let server = FabroMcpServer::new(Arc::new(settings)); + let tools = server.tool_router.list_all(); + let tool = tools + .iter() + .find(|tool| tool.name.as_ref() == "fabro_run_create") + .expect("fabro_run_create should be registered"); + let schema = Value::Object(tool.input_schema.as_ref().clone()); + let variants = schema + .pointer("/properties/runs/items/anyOf") + .and_then(Value::as_array) + .expect("runs items should advertise string and object variants"); + + assert!( + variants.iter().any(|variant| variant["type"] == "string"), + "runs items should include workflow string shorthand: {schema}" + ); + let object_variant = variants + .iter() + .find(|variant| variant["type"] == "object") + .unwrap_or_else(|| { + panic!("runs items should include object create spec variant: {schema}") + }); + assert!( + object_variant.pointer("/properties/workflow").is_some(), + "object create spec should expose workflow property: {schema}" + ); + assert!( + object_variant + .get("required") + .and_then(Value::as_array) + .is_some_and(|required| required.iter().any(|name| name == "workflow")), + "object create spec should require workflow: {schema}" + ); + } } diff --git a/lib/crates/fabro-tool/src/create.rs b/lib/crates/fabro-tool/src/create.rs index 85b8ae69f..7c50fd8f7 100644 --- a/lib/crates/fabro-tool/src/create.rs +++ b/lib/crates/fabro-tool/src/create.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use fabro_types::RunId; use schemars::{JsonSchema, Schema, SchemaGenerator, json_schema}; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize, de}; use serde_json::Value; use super::common::{self, FabroToolBackend, ToolError, ToolResult}; @@ -13,7 +13,178 @@ use super::manifest; #[derive(Debug, Deserialize, JsonSchema)] pub struct FabroRunCreateParams { - pub runs: Vec, + pub runs: Vec, +} + +#[derive(Debug)] +pub enum CreateRunSpecInput { + Workflow(String), + Spec(Box), +} + +impl<'de> Deserialize<'de> for CreateRunSpecInput { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let value = Value::deserialize(deserializer)?; + match value { + Value::String(workflow) => Ok(Self::Workflow(workflow)), + Value::Object(_) => CreateRunSpec::deserialize(value) + .map(Box::new) + .map(Self::Spec) + .map_err(de::Error::custom), + other => Err(de::Error::custom(format!( + "expected workflow string shorthand or create spec object, got {}", + json_value_kind(&other) + ))), + } + } +} + +fn json_value_kind(value: &Value) -> &'static str { + match value { + Value::Null => "null", + Value::Bool(_) => "boolean", + Value::Number(_) => "number", + Value::String(_) => "string", + Value::Array(_) => "array", + Value::Object(_) => "object", + } +} + +impl From for CreateRunSpecInput { + fn from(spec: CreateRunSpec) -> Self { + Self::Spec(Box::new(spec)) + } +} + +impl JsonSchema for CreateRunSpecInput { + fn inline_schema() -> bool { + true + } + + fn schema_name() -> Cow<'static, str> { + "CreateRunSpecInput".into() + } + + fn json_schema(_: &mut SchemaGenerator) -> Schema { + json_schema!({ + "description": "Fabro run create specification. Use a workflow string shorthand, or an object when setting create options.", + "anyOf": [ + { + "type": "string", + "description": "Workflow selector shorthand. Equivalent to an object with only the workflow field set." + }, + { + "type": "object", + "description": "Full create-run specification.", + "required": ["workflow"], + "properties": { + "workflow": { + "type": "string", + "description": "Workflow selector, such as a workflow name or workflow file path." + }, + "cwd": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "Working directory used to resolve relative workflow paths." + }, + "run_id": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "Optional run id to use for the created run." + }, + "parent_id": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "Optional parent run id or selector." + }, + "goal": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "Optional goal override for the run." + }, + "inputs": { + "type": "object", + "description": "Workflow input overrides keyed by input name.", + "additionalProperties": { + "description": "Run input override value. Inputs are TOML-compatible scalar values: string, boolean, integer, or float.", + "anyOf": [ + { "type": "string" }, + { "type": "boolean" }, + { "type": "integer" }, + { "type": "number" } + ] + } + }, + "labels": { + "type": "object", + "description": "Labels to attach to the created run.", + "additionalProperties": { "type": "string" } + }, + "dry_run": { + "anyOf": [ + { "type": "boolean" }, + { "type": "null" } + ], + "description": "Whether the run should use dry-run mode." + }, + "auto_approve": { + "anyOf": [ + { "type": "boolean" }, + { "type": "null" } + ], + "description": "Whether agent approval prompts should be auto-approved." + }, + "model": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "Model override for the run." + }, + "provider": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "Provider override for the run." + }, + "sandbox": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "Sandbox provider override for the run." + }, + "preserve_sandbox": { + "anyOf": [ + { "type": "boolean" }, + { "type": "null" } + ], + "description": "Whether to preserve the sandbox after the run." + }, + "start": { + "anyOf": [ + { "type": "boolean" }, + { "type": "null" } + ], + "description": "Whether to start the run immediately after creation. Defaults to true." + } + } + } + ] + }) + } } #[derive(Debug, Deserialize, JsonSchema)] @@ -111,6 +282,38 @@ impl TryFrom for ValidatedCreateRuns { } } +impl TryFrom for ValidatedCreateRunSpec { + type Error = ToolError; + + fn try_from(spec: CreateRunSpecInput) -> Result { + match spec { + CreateRunSpecInput::Workflow(workflow) => { + let workflow = workflow.trim(); + if workflow.is_empty() { + return Err(ToolError::message("workflow must not be blank")); + } + Self::try_from(CreateRunSpec { + workflow: workflow.to_string(), + cwd: None, + run_id: None, + parent_id: None, + goal: None, + inputs: HashMap::new(), + labels: HashMap::new(), + dry_run: None, + auto_approve: None, + model: None, + provider: None, + sandbox: None, + preserve_sandbox: None, + start: None, + }) + } + CreateRunSpecInput::Spec(spec) => Self::try_from(*spec), + } + } +} + impl TryFrom for ValidatedCreateRunSpec { type Error = ToolError; @@ -329,6 +532,75 @@ mod tests { assert_eq!(spec.parent_id.as_deref(), Some("nightly-parent")); } + #[test] + fn create_params_accept_string_shorthand() { + let params: FabroRunCreateParams = serde_json::from_value(json!({ + "runs": ["simple.fabro"] + })) + .expect("string shorthand should deserialize"); + + let params = ValidatedCreateRuns::try_from(params) + .expect("string shorthand should validate as workflow selector"); + let spec = ¶ms.runs[0]; + assert_eq!(spec.workflow, "simple.fabro"); + assert_eq!(spec.cwd, None); + assert_eq!(spec.run_id, None); + assert_eq!(spec.parent_id, None); + assert!(spec.inputs.is_empty()); + assert!(spec.labels.is_empty()); + assert_eq!(spec.start, None); + } + + #[test] + fn create_params_preserve_object_form_options() { + let params: FabroRunCreateParams = serde_json::from_value(json!({ + "runs": [{ + "workflow": "simple.fabro", + "dry_run": true, + "auto_approve": true, + "labels": { "source": "mcp-test" }, + "start": false + }] + })) + .expect("object form should deserialize"); + + let params = + ValidatedCreateRuns::try_from(params).expect("object form should still validate"); + let spec = ¶ms.runs[0]; + assert_eq!(spec.workflow, "simple.fabro"); + assert_eq!(spec.dry_run, Some(true)); + assert_eq!(spec.auto_approve, Some(true)); + assert_eq!( + spec.labels.get("source").map(String::as_str), + Some("mcp-test") + ); + assert_eq!(spec.start, Some(false)); + } + + #[test] + fn create_params_reject_blank_string_shorthand_workflow() { + let params: FabroRunCreateParams = serde_json::from_value(json!({ + "runs": [" "] + })) + .expect("blank shorthand should deserialize before validation"); + + let err = ValidatedCreateRuns::try_from(params).expect_err("blank workflow should fail"); + assert!(err.to_string().contains("workflow"), "{err}"); + } + + #[test] + fn create_params_missing_object_workflow_keeps_field_error() { + let err = serde_json::from_value::(json!({ + "runs": [{ "dry_run": true }] + })) + .expect_err("object form without workflow should fail deserialization"); + + assert!( + err.to_string().contains("missing field `workflow`"), + "{err}" + ); + } + #[tokio::test] async fn create_runs_resolves_parent_selector_and_sends_parent_id_to_backend() { let temp = tempfile::tempdir().expect("tempdir should be created"); @@ -342,22 +614,25 @@ mod tests { resolved_selectors: Mutex::new(Vec::new()), }); let params = ValidatedCreateRuns::try_from(FabroRunCreateParams { - runs: vec![CreateRunSpec { - workflow: "simple.fabro".to_string(), - cwd: None, - run_id: None, - parent_id: Some("nightly-parent".to_string()), - goal: None, - inputs: HashMap::new(), - labels: HashMap::new(), - dry_run: Some(true), - auto_approve: Some(true), - model: None, - provider: None, - sandbox: None, - preserve_sandbox: None, - start: Some(false), - }], + runs: vec![ + CreateRunSpec { + workflow: "simple.fabro".to_string(), + cwd: None, + run_id: None, + parent_id: Some("nightly-parent".to_string()), + goal: None, + inputs: HashMap::new(), + labels: HashMap::new(), + dry_run: Some(true), + auto_approve: Some(true), + model: None, + provider: None, + sandbox: None, + preserve_sandbox: None, + start: Some(false), + } + .into(), + ], }) .expect("create params should validate"); @@ -387,22 +662,24 @@ mod tests { created_parent_ids: Mutex::new(Vec::new()), resolved_selectors: Mutex::new(Vec::new()), }); - let runs = (0..2) - .map(|_| CreateRunSpec { - workflow: "simple.fabro".to_string(), - cwd: None, - run_id: None, - parent_id: Some("nightly-parent".to_string()), - goal: None, - inputs: HashMap::new(), - labels: HashMap::new(), - dry_run: Some(true), - auto_approve: Some(true), - model: None, - provider: None, - sandbox: None, - preserve_sandbox: None, - start: Some(false), + let runs: Vec = (0..2) + .map(|_| { + CreateRunSpecInput::from(CreateRunSpec { + workflow: "simple.fabro".to_string(), + cwd: None, + run_id: None, + parent_id: Some("nightly-parent".to_string()), + goal: None, + inputs: HashMap::new(), + labels: HashMap::new(), + dry_run: Some(true), + auto_approve: Some(true), + model: None, + provider: None, + sandbox: None, + preserve_sandbox: None, + start: Some(false), + }) }) .collect(); let params = ValidatedCreateRuns::try_from(FabroRunCreateParams { runs }) @@ -434,22 +711,25 @@ mod tests { resolved_selectors: Mutex::new(Vec::new()), }); let params = ValidatedCreateRuns::try_from(FabroRunCreateParams { - runs: vec![CreateRunSpec { - workflow: "simple.fabro".to_string(), - cwd: None, - run_id: None, - parent_id: Some(parent_id.to_string()), - goal: None, - inputs: HashMap::new(), - labels: HashMap::new(), - dry_run: Some(true), - auto_approve: Some(true), - model: None, - provider: None, - sandbox: None, - preserve_sandbox: None, - start: Some(false), - }], + runs: vec![ + CreateRunSpec { + workflow: "simple.fabro".to_string(), + cwd: None, + run_id: None, + parent_id: Some(parent_id.to_string()), + goal: None, + inputs: HashMap::new(), + labels: HashMap::new(), + dry_run: Some(true), + auto_approve: Some(true), + model: None, + provider: None, + sandbox: None, + preserve_sandbox: None, + start: Some(false), + } + .into(), + ], }) .expect("create params should validate"); diff --git a/lib/crates/fabro-tool/src/lib.rs b/lib/crates/fabro-tool/src/lib.rs index 4b9c720eb..7f5026a57 100644 --- a/lib/crates/fabro-tool/src/lib.rs +++ b/lib/crates/fabro-tool/src/lib.rs @@ -21,9 +21,9 @@ pub use common::{ tool_definitions, }; pub use create::{ - CreateRunOptions, CreateRunSpec, CreateRunsResult, CreatedRunResult, FabroRunCreateParams, - RunInputValue, ValidatedCreateRunSpec, ValidatedCreateRuns, create_runs, create_runs_text, - create_runs_with_options, + CreateRunOptions, CreateRunSpec, CreateRunSpecInput, CreateRunsResult, CreatedRunResult, + FabroRunCreateParams, RunInputValue, ValidatedCreateRunSpec, ValidatedCreateRuns, create_runs, + create_runs_text, create_runs_with_options, }; pub use events::{ FabroRunEventsParams, RunEventResult, RunEventsAction, RunEventsResult, ValidatedRunEvents, diff --git a/lib/crates/fabro-workflow/src/handler/llm/api.rs b/lib/crates/fabro-workflow/src/handler/llm/api.rs index ed8643ef4..884a6e230 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/api.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/api.rs @@ -306,7 +306,11 @@ fn ensure_current_run_parent( ) -> fabro_tool::ToolResult<()> { let current_parent = current_run_id.to_string(); for run in ¶ms.runs { - match run.parent_id.as_deref().map(str::trim) { + let parent_id = match run { + fabro_tool::CreateRunSpecInput::Workflow(_) => None, + fabro_tool::CreateRunSpecInput::Spec(spec) => spec.parent_id.as_deref().map(str::trim), + }; + match parent_id { None => {} Some("") => { return Err(fabro_tool::ToolError::message(