mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
feat: improve run board, thread, and MCP create flows (#347)
## 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` --- [](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) <noreply@anthropic.com>
This commit is contained in:
parent
3831c157fb
commit
c6356cbd77
17 changed files with 724 additions and 62 deletions
15
apps/fabro-web/app/lib/board-cache.ts
Normal file
15
apps/fabro-web/app/lib/board-cache.ts
Normal file
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
@ -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() {
|
||||
|
|
|
|||
57
apps/fabro-web/app/lib/mutations.test.ts
Normal file
57
apps/fabro-web/app/lib/mutations.test.ts
Normal file
|
|
@ -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));
|
||||
});
|
||||
});
|
||||
|
|
@ -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);
|
||||
},
|
||||
},
|
||||
);
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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, {
|
||||
|
|
|
|||
|
|
@ -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<string>(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<EventKind, string> = {
|
||||
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") && (
|
||||
<DetailField label="Content">
|
||||
<Markdown content={turn.content} />
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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<RunItem, "column" | "lifecycleStatusLabel">): 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,
|
||||
);
|
||||
|
||||
|
|
|
|||
|
|
@ -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).
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<CreateRunSpec>,
|
||||
pub runs: Vec<CreateRunSpecInput>,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
pub enum CreateRunSpecInput {
|
||||
Workflow(String),
|
||||
Spec(Box<CreateRunSpec>),
|
||||
}
|
||||
|
||||
impl<'de> Deserialize<'de> for CreateRunSpecInput {
|
||||
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
|
||||
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<CreateRunSpec> 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<FabroRunCreateParams> for ValidatedCreateRuns {
|
|||
}
|
||||
}
|
||||
|
||||
impl TryFrom<CreateRunSpecInput> for ValidatedCreateRunSpec {
|
||||
type Error = ToolError;
|
||||
|
||||
fn try_from(spec: CreateRunSpecInput) -> Result<Self, Self::Error> {
|
||||
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<CreateRunSpec> 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::<FabroRunCreateParams>(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<CreateRunSpecInput> = (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");
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue