diff --git a/Cargo.lock b/Cargo.lock index 6ace7aec3..e3e5e630d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1991,7 +1991,7 @@ dependencies = [ [[package]] name = "fabro-api" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "chrono", "fabro-automation", @@ -2017,7 +2017,7 @@ dependencies = [ [[package]] name = "fabro-auth" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2042,7 +2042,7 @@ dependencies = [ [[package]] name = "fabro-automation" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -2063,11 +2063,11 @@ dependencies = [ [[package]] name = "fabro-build-support" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" [[package]] name = "fabro-checkpoint" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "fabro-config", "fabro-types", @@ -2075,7 +2075,7 @@ dependencies = [ [[package]] name = "fabro-cli" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -2171,7 +2171,7 @@ dependencies = [ [[package]] name = "fabro-client" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "bytes", @@ -2200,7 +2200,7 @@ dependencies = [ [[package]] name = "fabro-config" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -2229,7 +2229,7 @@ dependencies = [ [[package]] name = "fabro-db" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -2242,7 +2242,7 @@ dependencies = [ [[package]] name = "fabro-dev" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -2261,7 +2261,7 @@ dependencies = [ [[package]] name = "fabro-dot" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "fabro-template", "fabro-types", @@ -2273,7 +2273,7 @@ dependencies = [ [[package]] name = "fabro-dump" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "bytes", @@ -2287,7 +2287,7 @@ dependencies = [ [[package]] name = "fabro-environment" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -2309,7 +2309,7 @@ dependencies = [ [[package]] name = "fabro-github" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2334,7 +2334,7 @@ dependencies = [ [[package]] name = "fabro-graphviz" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "fabro-dot", @@ -2344,7 +2344,7 @@ dependencies = [ [[package]] name = "fabro-http" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "fabro-static", "http 1.4.0", @@ -2354,7 +2354,7 @@ dependencies = [ [[package]] name = "fabro-install" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "fabro-config", @@ -2371,7 +2371,7 @@ dependencies = [ [[package]] name = "fabro-interview" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "async-trait", "fabro-types", @@ -2383,7 +2383,7 @@ dependencies = [ [[package]] name = "fabro-llm" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "async-trait", "bytes", @@ -2408,7 +2408,7 @@ dependencies = [ [[package]] name = "fabro-macros" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "clap", "fabro-options-metadata", @@ -2419,7 +2419,7 @@ dependencies = [ [[package]] name = "fabro-manifest" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2448,7 +2448,7 @@ dependencies = [ [[package]] name = "fabro-mcp-server" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "fabro-client", @@ -2468,7 +2468,7 @@ dependencies = [ [[package]] name = "fabro-mcp-store" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "chrono", "fabro-db", @@ -2486,7 +2486,7 @@ dependencies = [ [[package]] name = "fabro-oauth" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "axum", @@ -2508,7 +2508,7 @@ dependencies = [ [[package]] name = "fabro-options-metadata" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "serde", "serde_json", @@ -2516,7 +2516,7 @@ dependencies = [ [[package]] name = "fabro-petri" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2571,7 +2571,7 @@ dependencies = [ [[package]] name = "fabro-proc" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "libc", "tempfile", @@ -2583,7 +2583,7 @@ dependencies = [ [[package]] name = "fabro-redact" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "aho-corasick", "pebble-coding-agent", @@ -2595,12 +2595,13 @@ dependencies = [ "toml 0.8.23", "tracing", "tracing-subscriber", + "ulid", "url", ] [[package]] name = "fabro-server" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2695,7 +2696,7 @@ dependencies = [ [[package]] name = "fabro-slack" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "fabro-http", "fabro-interview", @@ -2716,18 +2717,18 @@ dependencies = [ [[package]] name = "fabro-spa" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "rust-embed", ] [[package]] name = "fabro-static" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" [[package]] name = "fabro-store" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "bytes", "chrono", @@ -2755,7 +2756,7 @@ dependencies = [ [[package]] name = "fabro-telemetry" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "base64", @@ -2781,7 +2782,7 @@ dependencies = [ [[package]] name = "fabro-template" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "fabro-types", @@ -2795,7 +2796,7 @@ dependencies = [ [[package]] name = "fabro-test" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "assert_cmd", @@ -2820,7 +2821,7 @@ dependencies = [ [[package]] name = "fabro-tool" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2844,7 +2845,7 @@ dependencies = [ [[package]] name = "fabro-tracker" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2858,7 +2859,7 @@ dependencies = [ [[package]] name = "fabro-types" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "chrono", "clap", @@ -2885,7 +2886,7 @@ dependencies = [ [[package]] name = "fabro-util" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "console 0.15.11", @@ -2908,7 +2909,7 @@ dependencies = [ [[package]] name = "fabro-variable" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -2925,7 +2926,7 @@ dependencies = [ [[package]] name = "fabro-vault" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "chrono", @@ -2944,7 +2945,7 @@ dependencies = [ [[package]] name = "fabro-workflow" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "anyhow", "async-trait", @@ -2989,7 +2990,7 @@ dependencies = [ [[package]] name = "fabro-workflow-version" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "fabro-config", "fabro-dot", @@ -7925,7 +7926,7 @@ dependencies = [ [[package]] name = "twin-github" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" dependencies = [ "axum", "base64", diff --git a/Cargo.toml b/Cargo.toml index f85830d59..5cef8b415 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,7 +10,7 @@ resolver = "2" [workspace.package] edition = "2021" -version = "0.389.0-nightly.0" +version = "0.392.0-nightly.0" license = "MIT" [workspace.dependencies] diff --git a/apps/fabro-web/app/components/run-waterfall.tsx b/apps/fabro-web/app/components/run-waterfall.tsx index 3e4959c72..62f4308ef 100644 --- a/apps/fabro-web/app/components/run-waterfall.tsx +++ b/apps/fabro-web/app/components/run-waterfall.tsx @@ -3,6 +3,7 @@ import { Link } from "react-router"; import { StageState, type RunStage } from "@qltysh/fabro-api-client"; import { HoverCard, PopoverHeader, PopoverRow, PopoverRows } from "./ui"; +import { stagePath } from "../lib/stage-routes"; import { isVisibleStage } from "../data/runs"; import { formatAbsoluteTs, formatDurationMs } from "../lib/format"; import { @@ -150,7 +151,7 @@ function stageRow(runId: string, stage: RunStage, nowMs: number): Row | null { endMs, durationMs, barClass: stageBarClass(stage.status), - href: `/runs/${runId}/stages/${encodeURIComponent(stage.id)}`, + href: stagePath(runId, stage.id), popover: stagePopover(stage, durationMs, inFlight), }; } diff --git a/apps/fabro-web/app/components/stage-popover.tsx b/apps/fabro-web/app/components/stage-popover.tsx index 0ea060770..450cbafe9 100644 --- a/apps/fabro-web/app/components/stage-popover.tsx +++ b/apps/fabro-web/app/components/stage-popover.tsx @@ -1,6 +1,7 @@ import { Link } from "react-router"; import type { StageState } from "@qltysh/fabro-api-client"; +import { stagePath } from "../lib/stage-routes"; import { formatTokenCount } from "../lib/format"; import { formatStageLabel, @@ -106,7 +107,7 @@ export function StagePopover({ runId, stage, duration }: StagePopoverProps) { {stage.resumedFromStageId && ( {stage.resumedFromStageId} diff --git a/apps/fabro-web/app/components/stage-renderers/conditional-decision.tsx b/apps/fabro-web/app/components/stage-renderers/conditional-decision.tsx index 13727e1d5..18a859b04 100644 --- a/apps/fabro-web/app/components/stage-renderers/conditional-decision.tsx +++ b/apps/fabro-web/app/components/stage-renderers/conditional-decision.tsx @@ -5,6 +5,7 @@ import { ArrowsRightLeftIcon, } from "@heroicons/react/20/solid"; +import { stagePath } from "../../lib/stage-routes"; import type { Stage } from "../stage-sidebar"; import { StageMetaBar } from "./meta-bar"; import type { EdgeSelection } from "./helpers"; @@ -67,7 +68,7 @@ export function ConditionalDecision({ {edge ? ( targetStage ? (
diff --git a/apps/fabro-web/app/components/stage-renderers/parallel-children.test.tsx b/apps/fabro-web/app/components/stage-renderers/parallel-children.test.tsx index a36305025..36016136f 100644 --- a/apps/fabro-web/app/components/stage-renderers/parallel-children.test.tsx +++ b/apps/fabro-web/app/components/stage-renderers/parallel-children.test.tsx @@ -119,8 +119,8 @@ describe("ParallelChildren", () => { expect(rows[1]).toContain("Running"); expect(rows[1]).toContain("review_opus"); expect(hrefs(renderer)).toEqual([ - "/runs/run-1/stages/review_glm@1", - "/runs/run-1/stages/review_opus@1", + "/runs/run-1/stages/review_glm%401", + "/runs/run-1/stages/review_opus%401", ]); expect(statValue(renderer, "Succeeded")).toBe("1"); expect(statValue(renderer, "Failed")).toBe("0"); @@ -154,7 +154,7 @@ describe("ParallelChildren", () => { ], ); - expect(hrefs(renderer)).toEqual(["/runs/run-1/stages/review_glm@1"]); + expect(hrefs(renderer)).toEqual(["/runs/run-1/stages/review_glm%401"]); }); test("shows a late-starting branch when lower indexes have no stage yet", () => { @@ -168,7 +168,7 @@ describe("ParallelChildren", () => { "PendingBranch 2", "Runningreview_opus", ]); - expect(hrefs(renderer)).toEqual(["/runs/run-1/stages/review_opus@1"]); + expect(hrefs(renderer)).toEqual(["/runs/run-1/stages/review_opus%401"]); expect(statValue(renderer, "Branches")).toBe("3"); }); @@ -192,7 +192,7 @@ describe("ParallelChildren", () => { ); expect(branchRowText(renderer)).toEqual(["Runningreview_glm@2"]); - expect(hrefs(renderer)).toEqual(["/runs/run-1/stages/review_glm@2"]); + expect(hrefs(renderer)).toEqual(["/runs/run-1/stages/review_glm%402"]); }); test("keeps duplicate branch targets in index order and only links recorded stages", () => { @@ -208,7 +208,7 @@ describe("ParallelChildren", () => { expect(rows).toHaveLength(2); expect(rows[0]).toContain("Succeeded"); expect(rows[1]).toContain("Failed"); - expect(hrefs(renderer)).toEqual(["/runs/run-1/stages/review@1"]); + expect(hrefs(renderer)).toEqual(["/runs/run-1/stages/review%401"]); }); test("renders a completed result without a matching stage as an unlinked row", () => { diff --git a/apps/fabro-web/app/components/stage-renderers/parallel-children.tsx b/apps/fabro-web/app/components/stage-renderers/parallel-children.tsx index be7bff3f9..03b918002 100644 --- a/apps/fabro-web/app/components/stage-renderers/parallel-children.tsx +++ b/apps/fabro-web/app/components/stage-renderers/parallel-children.tsx @@ -3,6 +3,7 @@ import { Link } from "react-router"; import { ArrowTopRightOnSquareIcon } from "@heroicons/react/20/solid"; import { StageState } from "@qltysh/fabro-api-client"; +import { stagePath } from "../../lib/stage-routes"; import type { Stage } from "../stage-sidebar"; import { formatStageLabel, stageStatusLabel, stageStatusTone } from "../../lib/stage-sidebar"; import { StageMetaBar } from "./meta-bar"; @@ -82,7 +83,7 @@ function ChildRow({
  • {row.stageId ? ( {inner} diff --git a/apps/fabro-web/app/components/stage-sidebar.tsx b/apps/fabro-web/app/components/stage-sidebar.tsx index 897fb0d53..9e37e4176 100644 --- a/apps/fabro-web/app/components/stage-sidebar.tsx +++ b/apps/fabro-web/app/components/stage-sidebar.tsx @@ -18,6 +18,7 @@ import { MapIcon, PaperClipIcon, } from "@heroicons/react/24/outline"; +import { stagePath } from "../lib/stage-routes"; import { formatDurationSecs } from "../lib/format"; import { ACTIVE_STAGE_STATES, formatStageLabel, type Stage } from "../lib/stage-sidebar"; import { elapsedSecsSince, useTickingNow } from "../lib/time"; @@ -207,7 +208,7 @@ export function StageSidebar({ stages, runId, selectedStageId, activeLink }: Sta return ( (null); const openStage = useCallback( - (stageId: string) => navigate(`/runs/${id}/stages/${stageId}`), + (stageId: string) => { + if (id) navigate(stagePath(id, stageId)); + }, [id, navigate], ); useAnnotatedRunGraphSvg({ diff --git a/apps/fabro-web/app/routes/run-petri.render.test.tsx b/apps/fabro-web/app/routes/run-petri.render.test.tsx index 0da4e46a2..5c620be94 100644 --- a/apps/fabro-web/app/routes/run-petri.render.test.tsx +++ b/apps/fabro-web/app/routes/run-petri.render.test.tsx @@ -177,8 +177,8 @@ describe("a two-branch parallel run", () => { />, ); expect(html).toContain("Branches"); - expect(html).toContain("/runs/run-1/stages/a@1"); - expect(html).toContain("/runs/run-1/stages/b@1"); + expect(html).toContain("/runs/run-1/stages/a%401"); + expect(html).toContain("/runs/run-1/stages/b%401"); expect((html.match(/Succeeded/g) ?? []).length).toBeGreaterThanOrEqual(2); }); @@ -274,7 +274,7 @@ describe("a human gate answered through the API", () => { runId="run-1" />, ); - expect(html).toContain("/runs/run-1/stages/no@1"); + expect(html).toContain("/runs/run-1/stages/no%401"); expect(html).not.toContain("No target"); }); diff --git a/apps/fabro-web/app/routes/run-stages.navigation.test.tsx b/apps/fabro-web/app/routes/run-stages.navigation.test.tsx new file mode 100644 index 000000000..8f3d83c76 --- /dev/null +++ b/apps/fabro-web/app/routes/run-stages.navigation.test.tsx @@ -0,0 +1,331 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import type { ReactNode } from "react"; +import TestRenderer, { act } from "react-test-renderer"; +import { createMemoryRouter, Outlet, RouterProvider } from "react-router"; +import { loadPetriFixture } from "../lib/petri-fixtures"; +import { makeStage, setupReactTestEnv, textContent } from "../lib/test-utils"; + +import { ParallelChildren } from "../components/stage-renderers/parallel-children"; +import { ConditionalDecision } from "../components/stage-renderers/conditional-decision"; +import { StagePopover } from "../components/stage-popover"; +import { RunWaterfall } from "../components/run-waterfall"; + +const ids = [ + "reviewer@1", + "reviewer/e2@1", + "reviewer/50%@1", + "reviewer%20x@1", + "reviewer%2Fe2@1", + "reviewer%252Fe2@1", + "reviewer?x#y@1", + "réview/日本@1", +]; +const fixture = loadPetriFixture("command"); +const stages = ids.map((id) => ({ + id, + name: id.slice(0, -2), + node_id: id.slice(0, -2), + visit: 1, + handler: "command", + status: "succeeded", + usage: fixture.projection.stages["say@1"].usage, +})); +const projection = { + ...fixture.projection, + stages: Object.fromEntries( + ids.map((id) => [id, fixture.projection.stages["say@1"]]), + ), +}; +// Keep real Petri records and give each occurrence a visibly distinct command. +const stream = ids.flatMap((id) => + fixture.stream + .filter( + (entry) => + entry.kind === "petri" && entry.item.subject?.node?.name === "say", + ) + .map((entry) => { + const copy = structuredClone(entry); + copy.item.subject.node.name = id.slice(0, -2); + copy.item.subject.node.meta.script = `echo selected:${id}`; + return copy; + }), +); +let loading = false; +let available = stages; +const logRequests: string[] = []; +mock.module("../lib/queries", () => ({ + useRunGraph: () => ({ data: undefined }), + useRunGraphSource: () => ({ data: undefined }), + useRun: () => ({ data: undefined }), + useRunStages: () => ({ + data: loading ? undefined : { data: available }, + isLoading: loading, + }), + useRunState: () => ({ data: projection }), + useRunStream: () => ({ data: stream }), + useRunStageContextWindow: () => ({ data: undefined }), + useRunStageLog: (_runId: string, stageId: string) => { + logRequests.push(stageId); + const output = Buffer.from(`output:${stageId}`, "utf8"); + return { + data: { + bytes_base64: output.toString("base64"), + next_offset: output.length, + total_bytes: output.length, + }, + }; + }, +})); +mock.module("../components/run-summary-panel", () => ({ + RunSummaryPanel: () => null, +})); +let graphClick: (id: string) => void; +mock.module("../hooks/use-annotated-run-graph-svg", () => ({ + useAnnotatedRunGraphSvg: ({ + onStageClick, + }: { + onStageClick: (id: string) => void; + }) => { + graphClick = onStageClick; + }, +})); +const { default: RunOverview } = await import("./run-overview"); +const { default: RunStages } = await import("./run-stages"); +let renderer: TestRenderer.ReactTestRenderer; +let teardown: () => void; +beforeEach(() => { + teardown = setupReactTestEnv(); + loading = false; + available = stages; + logRequests.length = 0; +}); +afterEach(() => { + if (renderer) act(() => renderer.unmount()); + teardown(); +}); + +function render(entry = "/runs/run-1/stages", source?: ReactNode) { + const router = createMemoryRouter( + [ + { + path: "/runs/:id", + element: , + children: [ + { index: true, element: source ?? }, + { path: "stages", element: }, + { path: "stages/:stageId", element: }, + ], + }, + ], + { initialEntries: [entry] }, + ); + act(() => { + renderer = TestRenderer.create(); + }); + return router; +} +async function clickStage(id: string) { + const link = renderer.root + .findAllByType("a") + .find( + (node) => + node.props.href.includes("/stages/") && + node + .findAllByType("span") + .some((span) => textContent(span) === id.slice(0, -2)), + ); + expect(link).toBeDefined(); + await act(async () => + link!.props.onClick({ + button: 0, + defaultPrevented: false, + preventDefault() { + this.defaultPrevented = true; + }, + }), + ); +} +function expectSelected(id: string) { + const text = textContent(renderer.root); + expect(text).toContain(`echo selected:${id}`); + expect(text).toContain(`output:${id}`); + expect(logRequests.at(-1)).toBe(id); + const active = renderer.root + .findAllByType("a") + .filter((node) => node.props.className?.includes("bg-overlay text-fg")); + expect(active).toHaveLength(1); + expect(active[0].props.href).toBe( + `/runs/run-1/stages/${encodeURIComponent(id)}`, + ); + for (const other of ids.filter((value) => value !== id)) { + expect(text).not.toContain(`echo selected:${other}`); + expect(text).not.toContain(`output:${other}`); + } +} + +describe("stage occurrence navigation", () => { + test.each(ids)("clicking the sidebar opens exactly %s", async (id) => { + const router = render(); + await clickStage(id); + expect(router.state.location.pathname).toBe( + `/runs/run-1/stages/${encodeURIComponent(id)}`, + ); + expectSelected(id); + }); + test.each(ids)("direct links preserve %s through the nested route", (id) => { + render(`/runs/run-1/stages/${encodeURIComponent(id)}/?view=detail#output`); + expectSelected(id); + }); + test("back and forward restore the exact occurrence", async () => { + const router = render(); + await clickStage(ids[1]); + await clickStage(ids[4]); + await act(async () => { + await router.navigate(-1); + }); + expectSelected(ids[1]); + await act(async () => { + await router.navigate(1); + }); + expectSelected(ids[4]); + }); + test("the stage index still selects the first stage", () => { + render(); + expectSelected(ids[0]); + }); + test.each(["missing%401", "bad%ZZ"])( + "an explicit unavailable ID %s never selects another stage", + (segment) => { + render(`/runs/run-1/stages/${segment}`); + expect(textContent(renderer.root)).toContain("Stage not found"); + expect(logRequests).toEqual([]); + }, + ); + test("an explicit ID waits for the stage list then selects it", async () => { + loading = true; + const router = render(`/runs/run-1/stages/${encodeURIComponent(ids[1])}`); + expect(textContent(renderer.root)).toContain("Loading stages"); + expect(textContent(renderer.root)).not.toContain("Stage not found"); + loading = false; + await act(async () => { + await router.navigate(router.state.location.pathname, { replace: true }); + }); + expectSelected(ids[1]); + }); +}); + +describe("other stage navigation surfaces", () => { + const target = makeStage({ + id: ids[1], + name: "reviewer/e2", + nodeId: "reviewer/e2", + handler: "command", + status: "succeeded", + parallelGroupId: "fork@1", + parallelBranchIndex: 0, + }); + const source = makeStage({ + id: "fork@1", + nodeId: "fork", + status: "succeeded", + }); + const producers = [ + [ + "parallel child", + , + ], + [ + "conditional target", + , + ], + [ + "resumed-stage popover", + , + ], + [ + "waterfall", + , + ], + ] as const; + test.each(producers)( + "%s opens the offered occurrence", + async (_name, element) => { + const router = render("/runs/run-1", element); + const link = renderer.root + .findAllByType("a") + .find((node) => node.props.href.includes("/stages/")); + expect(link).toBeDefined(); + await act(async () => + link!.props.onClick({ + button: 0, + defaultPrevented: false, + preventDefault() { + this.defaultPrevented = true; + }, + }), + ); + expect(router.state.location.pathname).toBe( + `/runs/run-1/stages/${encodeURIComponent(target.id)}`, + ); + expectSelected(target.id); + }, + ); + test("the resumed-stage banner opens the source occurrence", async () => { + available = stages.map((stage, index) => + index === 0 ? { ...stage, resumed_from_stage_id: ids[1] } : stage, + ); + render(); + const link = renderer.root + .findAllByType("a") + .find((node) => textContent(node) === ids[1]); + await act(async () => + link!.props.onClick({ + button: 0, + defaultPrevented: false, + preventDefault() { + this.defaultPrevented = true; + }, + }), + ); + expectSelected(ids[1]); + }); + test("the graph stage-click handler navigates to the offered occurrence", async () => { + const router = render("/runs/run-1"); + await act(async () => graphClick(ids[1])); + expect(router.state.location.pathname).toBe( + `/runs/run-1/stages/${encodeURIComponent(ids[1])}`, + ); + expectSelected(ids[1]); + }); +}); diff --git a/apps/fabro-web/app/routes/run-stages.tsx b/apps/fabro-web/app/routes/run-stages.tsx index 49271cd4e..28110f83b 100644 --- a/apps/fabro-web/app/routes/run-stages.tsx +++ b/apps/fabro-web/app/routes/run-stages.tsx @@ -5,7 +5,7 @@ import { useState, type ReactNode, } from "react"; -import { Link, useParams } from "react-router"; +import { Link, useLocation, useParams } from "react-router"; import { ArrowDownTrayIcon, ChevronDownIcon, @@ -38,6 +38,7 @@ import type { ThreadDnaItem, ThreadDnaSelection, } from "../components/event-debug"; +import { stageIdFromPathname, stagePath } from "../lib/stage-routes"; import { StageContext } from "../components/stage-context"; import { StageInsightsSidebar } from "../components/stage-insights-sidebar"; import { StageSidebar } from "../components/stage-sidebar"; @@ -2374,7 +2375,7 @@ function RunStageActivityStage({

    Resumed from{" "} {selectedStage.resumedFromStageId} @@ -2526,7 +2527,9 @@ function RunStageActivity({ } export default function RunStages() { - const { id, stageId } = useParams(); + const { id, stageId: stageParam } = useParams(); + const { pathname } = useLocation(); + const stageId = stageIdFromPathname(pathname); const runQuery = useRun(id); const stagesQuery = useRunStages(id); const stages = useMemo( @@ -2535,7 +2538,9 @@ export default function RunStages() { ); const selectedStage = - stages.find((s: Stage) => s.id === stageId) ?? stages[0]; + stageParam === undefined + ? stages[0] + : stages.find((s: Stage) => s.id === stageId); const selectedStageId = selectedStage?.id; const runStart = selectedStage?.startedAt ?? @@ -2563,6 +2568,25 @@ export default function RunStages() { ? runStateQuery.data?.stages[selectedStageId] : undefined; + if (stagesQuery.isLoading && !stagesQuery.data) { + return ( +

    + +
    + ); + } + + if (id && stageParam !== undefined && !selectedStage) { + return ( +
    + +
    + ); + } + if (!id || !selectedStage) { return (
    diff --git a/docs/public/agents/hooks.mdx b/docs/public/agents/hooks.mdx index e8b83f483..5d36b7bb8 100644 --- a/docs/public/agents/hooks.mdx +++ b/docs/public/agents/hooks.mdx @@ -5,31 +5,43 @@ description: "Run custom logic in response to workflow lifecycle events" Hooks let you run custom logic at key points during a workflow — before a stage starts, after a run completes, when a sandbox is ready, and more. Use them for validation, notifications, guardrails, and orchestration without modifying the workflow graph itself. +The examples below are configuration fragments for `workflow.toml`. Add them to a workflow configuration using the current [run configuration schema](/execution/run-configuration). + ## Hook types Fabro supports four hook types, from simple shell commands to full agent sessions: ### Command -Run a shell command via `sh -c`. The simplest and most common hook type. +Use `script` for a shell command string. This is the simplest and most common hook type. -```toml title="run.toml" -[[hooks]] +```toml title="workflow.toml" +[[run.hooks]] event = "stage_start" -command = "./scripts/pre-check.sh" +script = "./scripts/pre-check.sh" ``` +Alternatively, `command` accepts an array of strings: + +```toml title="workflow.toml" +[[run.hooks]] +event = "post_tool_use" +command = ["cargo", "fmt"] +matcher = "write_file|edit_file|apply_patch" +``` + +Use either `script` or `command` for a command hook. + ### HTTP POST the event context as JSON to an HTTP endpoint. Useful for webhooks, external APIs, and notification services. -```toml title="run.toml" -[[hooks]] +```toml title="workflow.toml" +[[run.hooks]] event = "run_complete" -type = "http" url = "https://hooks.example.com/done" -[hooks.headers] +[run.hooks.headers] X-Deployment-Environment = "{{ vars.DEPLOY_ENV }}" ``` @@ -45,10 +57,9 @@ X-Deployment-Environment = "{{ vars.DEPLOY_ENV }}" A single-turn LLM call that evaluates the event context and returns an `ok`/`block` decision. The model responds with structured JSON. -```toml title="run.toml" -[[hooks]] +```toml title="workflow.toml" +[[run.hooks]] event = "stage_start" -type = "prompt" prompt = "Should this stage proceed given the current context? Check for any red flags." model = "haiku" blocking = true @@ -63,10 +74,10 @@ blocking = true A multi-turn agent session with full tool access (shell, file read/write, grep, glob). The agent can investigate the workspace before making a decision. -```toml title="run.toml" -[[hooks]] +```toml title="workflow.toml" +[[run.hooks]] event = "run_complete" -type = "agent" +agent = "enabled" prompt = "Verify that all tests pass and the code compiles. Run the test suite." model = "sonnet" max_tool_rounds = 10 @@ -75,10 +86,13 @@ blocking = true | Field | Description | |---|---| +| `agent` | Set to `"enabled"` to select an agent hook. | | `prompt` | Task instructions for the agent. | | `model` | Model alias or ID. Defaults to `haiku`. | | `max_tool_rounds` | Maximum tool call rounds before the agent gives up. Default: `50`. | +The `agent = "enabled"` marker selects an agent hook; `prompt` supplies its instructions. Without that marker, `prompt` selects a single-turn prompt hook. + ## Lifecycle events Each hook fires on a specific lifecycle event: @@ -104,34 +118,41 @@ Each hook fires on a specific lifecycle event: ## Configuration -Hooks are defined as `[[hooks]]` entries in any of these TOML config files: +Hooks are defined as `[[run.hooks]]` entries in any of these TOML config files: - **`.fabro/project.toml`** — project-level hooks, apply to all workflows in the project - **`workflow.toml`** — per-workflow hooks - **`~/.fabro/settings.toml`** — global defaults for all runs +Each entry selects one transport: `script` or `command`, `url`, `prompt`, or `agent = "enabled"` together with `prompt`. Do not add a `type` field; the transport fields select the hook type. + See [Merging hook configs](#merging-hook-configs) for how these layers combine. -```toml title="run.toml" -[[hooks]] -name = "pre-check" +```toml title="workflow.toml" +[[run.hooks]] +id = "pre-check" +name = "Pre-check script" event = "stage_start" -command = "./scripts/pre-check.sh" +script = "./scripts/pre-check.sh" matcher = "agent" blocking = true -timeout_ms = 30000 +timeout = "30s" sandbox = false ``` | Field | Description | |---|---| -| `name` | Optional display name. Auto-generated from event and type if omitted. | +| `id` | Optional merge identity. Hooks with the same `id` replace each other across layers. | +| `name` | Optional display name; does not control merging. If omitted, the hook uses `id` or an auto-generated label. | | `event` | The lifecycle event to listen for (required). | -| `command` | Shell command shorthand — implies `type = "command"`. | -| `type` | Explicit hook type: `"command"`, `"http"`, `"prompt"`, or `"agent"`. | +| `script` | Shell command string. | +| `command` | Command and arguments as an array of strings, such as `["cargo", "fmt"]`; an alternative to `script`. | +| `url` | Selects an HTTP hook. | +| `prompt` | Instructions for a prompt hook, or for an agent hook when `agent` is set. | +| `agent` | Set to `"enabled"` to select an agent hook. | | `matcher` | Regex pattern to filter which stages trigger this hook. | | `blocking` | Whether the hook must complete before execution continues. Defaults vary by event. | -| `timeout_ms` | Hook timeout in milliseconds. Default: `60000` (60s) for most types, `30000` (30s) for prompt hooks. | +| `timeout` | Duration string with a unit, such as `"500ms"`, `"30s"`, or `"2m"`. Default: `"30s"` for prompt hooks, `"60s"` for other types. | | `sandbox` | Run inside the sandbox (`true`, default) or on the host (`false`). | ## Blocking vs. non-blocking @@ -211,34 +232,33 @@ For tool events, the matcher is tested against the tool's internal name (e.g., ` This example auto-formats Rust code whenever the agent writes or edits a file: -```toml -[[hooks]] +```toml title="workflow.toml" +[[run.hooks]] name = "cargo-fmt" event = "post_tool_use" -command = "cargo fmt" +script = "cargo fmt" matcher = "write_file|edit_file|apply_patch" blocking = true ``` More examples: -```toml +```toml title="workflow.toml" # Only fire for agent handler nodes -[[hooks]] +[[run.hooks]] event = "stage_start" -command = "./scripts/agent-guard.sh" +script = "./scripts/agent-guard.sh" matcher = "^agent$" # Fire for any node with "test" in its ID -[[hooks]] +[[run.hooks]] event = "stage_complete" -command = "./scripts/report-test.sh" +script = "./scripts/report-test.sh" matcher = "test" # Gate shell commands with a guardrail -[[hooks]] +[[run.hooks]] event = "pre_tool_use" -type = "prompt" prompt = "Is this shell command safe to run?" matcher = "^shell$" model = "haiku" @@ -327,7 +347,7 @@ Each hook type has a default timeout: | Prompt | 30 seconds | | Agent | 60 seconds | -Override with `timeout_ms` on any hook definition. Prompt and agent hooks **fail open** on timeout — execution proceeds as if the hook returned `ok: true`. +Override with `timeout` on any hook definition, using a duration string such as `timeout = "30s"` or `timeout = "2m"`. Supported units are `ms`, `s`, `m`, `h`, and `d`. Prompt and agent hooks **fail open** on timeout — execution proceeds as if the hook returned `ok: true`. ## Fail-open behavior @@ -343,61 +363,61 @@ Command hooks do **not** fail open. A non-zero exit code (other than 0 or 2) pro ## Merging hook configs -Hooks from multiple config files are merged in this order (later layers win on name collisions): +Hooks from multiple config files are merged in this order (later layers replace hooks with the same `id`): 1. **`~/.fabro/settings.toml`** — global defaults 2. **`.fabro/project.toml`** — project-level overrides 3. **`workflow.toml`** — per-workflow overrides -This lets you define global hooks at the server level, project-wide hooks in `.fabro/project.toml`, and override or extend them per workflow. +Hooks with matching `id` values replace in place. Hooks without an `id` append after inherited hooks, even when their `name` matches. Use `id` when a workflow should override an inherited hook. + +This lets you define global hooks at the server level, project-wide hooks in `.fabro/project.toml`, and override or extend them per workflow. See [configuration precedence](/execution/run-configuration#precedence) for the complete layer order. ## Full example -```toml title="run.toml" +```toml title="workflow.toml" # Validate environment before the run starts -[[hooks]] +[[run.hooks]] name = "env-check" event = "run_start" -command = "./scripts/check-env.sh" +script = "./scripts/check-env.sh" blocking = true sandbox = false # Notify Slack when a stage completes -[[hooks]] +[[run.hooks]] name = "slack-notify" event = "stage_complete" -type = "http" url = "https://hooks.slack.com/workflows/T00/B00/abc123" tls = "verify" blocking = false # LLM guardrail before agent stages -[[hooks]] +[[run.hooks]] name = "safety-check" event = "stage_start" -type = "prompt" prompt = "Review the event context. Is there any reason this stage should not proceed?" model = "haiku" matcher = "^agent$" blocking = true -timeout_ms = 15000 +timeout = "15s" # Post-run verification agent -[[hooks]] +[[run.hooks]] name = "verify" event = "run_complete" -type = "agent" +agent = "enabled" prompt = "Run the test suite and verify all tests pass. Report any failures." model = "sonnet" max_tool_rounds = 20 blocking = true -timeout_ms = 120000 +timeout = "2m" # Auto-format Rust files after writes -[[hooks]] +[[run.hooks]] name = "cargo-fmt" event = "post_tool_use" -command = "cargo fmt" +script = "cargo fmt" matcher = "write_file|edit_file|apply_patch" blocking = true ``` diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index e5319e4f4..13f3bbc0c 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -9012,12 +9012,16 @@ components: model: type: string description: >- - Model selector: `provider/model`, a model id or alias, or a - provider id. The server picks a ready-provider default when - omitted. + Model selector: `provider/model`, or a model id or alias. A bare + provider name is rejected; pin it with `provider` instead. When + omitted, the server picks the pinned provider's default model, or + a ready-provider default when `provider` is also omitted. provider: type: string - description: Optional provider pin for a bare model selector. + description: >- + Optional provider pin. Scopes a bare model selector to that + provider, which also lets an uncatalogued model id reach a + provider that allows passthrough. system: type: string description: System prompt (convenience; prepended as a system message). diff --git a/docs/public/core-concepts/models.mdx b/docs/public/core-concepts/models.mdx index 5ab6914f4..149294276 100644 --- a/docs/public/core-concepts/models.mdx +++ b/docs/public/core-concepts/models.mdx @@ -130,7 +130,7 @@ capabilities = { text = true, tools = true } `api_model` is the model name sent to that provider's API. It defaults to the exact model id, so omit it when the two strings match. Fabro does not infer vendor prefixes or rewrite the value. -A `provider/model` selector such as `openai/gpt-5.6-sol` pins the provider and names the model by id, alias, or wire id. A bare selector with no provider pin picks the highest-priority ready offering; a separate `provider = "openrouter"` pin selects the OpenRouter offering. Providers with `allow_passthrough = true` also accept `provider/model` selectors for models the catalog does not list. +A `provider/model` selector such as `openai/gpt-5.6-sol` pins the provider and names the model by id, alias, or wire id. A bare selector with no provider pin picks the highest-priority ready offering; a separate `provider = "openrouter"` pin selects the OpenRouter offering. Providers with `allow_passthrough = true` also accept `provider/model` selectors for models the catalog does not list. A bare selector the catalog does not know is an error rather than a passthrough, and a bare provider name such as `gemini` is an error too: pin the provider and omit the model to use its default. Model roles are separate: the provider's `default_model` controls normal model selection for workflow execution, while `small_default = true` on a model row marks the provider's small utility model for metadata tasks such as generated run titles. If a provider has no small default, Fabro falls back to that provider's default model. diff --git a/docs/public/execution/run-configuration.mdx b/docs/public/execution/run-configuration.mdx index b4bc7b4f5..d7e127e5d 100644 --- a/docs/public/execution/run-configuration.mdx +++ b/docs/public/execution/run-configuration.mdx @@ -582,11 +582,11 @@ sandbox = false | `id` | Optional merge identity. Hooks with the same `id` replace each other across layers. | | `name` | Optional display name for the hook. | | `event` | Lifecycle event: `run_start`, `run_complete`, `stage_start`, `stage_complete`, etc. | -| `script` | Shell-evaluated command (equivalent to the old `type = "command"` shorthand). | -| `command` | Argv-style command (alternative to `script`). | +| `script` | Shell command string. | +| `command` | Command and arguments as an array of strings, such as `["cargo", "fmt"]`; an alternative to `script`. | | `matcher` | Regex matched against node ID or handler type. Limits which stages trigger this hook. | | `blocking` | Whether the hook must complete before execution continues. Defaults vary by event. | -| `timeout` | Human-readable hook timeout (e.g. `"30s"`, `"1m"`). Default: `"60s"`. | +| `timeout` | Human-readable hook timeout (e.g. `"30s"`, `"1m"`). Default: `"30s"` for prompt hooks, `"60s"` for other types. | | `sandbox` | Run inside the sandbox (`true`, default) or on the host (`false`). | Hook merge semantics: hooks with matching `id` values replace in place. Hooks without an `id` from a higher-precedence layer append after the fully merged inherited hook list. diff --git a/lib/apps/fabro-cli/src/commands/run/petri_stream.rs b/lib/apps/fabro-cli/src/commands/run/petri_stream.rs index 28c374b78..2748f5779 100644 --- a/lib/apps/fabro-cli/src/commands/run/petri_stream.rs +++ b/lib/apps/fabro-cli/src/commands/run/petri_stream.rs @@ -1329,7 +1329,8 @@ mod tests { #[test] fn redacted_raw_lines_keep_the_typed_run_id() { let run_id = "01M5228EHCT0P73Z4BYGDRWJNS"; - let mut event = petri(1, json!({"record": {"body": {"value": run_id}}})); + let secret = "xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6p"; + let mut event = petri(1, json!({"record": {"body": {"value": secret}}})); event.run_id = run_id.parse().expect("a valid run ID"); let line = raw_line(&event).expect("a redacted line"); @@ -1338,4 +1339,25 @@ mod tests { assert_eq!(decoded.stream_seq, event.stream_seq); assert_eq!(decoded.item["record"]["body"]["value"], "REDACTED"); } + + #[test] + fn raw_lines_preserve_run_ids_that_cross_the_entropy_threshold() { + // This ID has enough distinct characters to read as a secret to the + // entropy scanner; the Web UI link and run branch must survive. + let run_id = "01M5228EHCT0P73Z4BYGDRWJNS"; + for (kind, field, text) in [ + ( + "run.created", + "web_url", + format!("http://localhost:3000/runs/{run_id}"), + ), + ("run.branch", "run_branch", format!("fabro/run/{run_id}")), + ] { + let mut event = platform(1, &json!({"kind": kind, field: text, "content": run_id})); + event.run_id = run_id.parse().expect("a valid run ID"); + let line = raw_line(&event).expect("a redacted line"); + let decoded: RunStreamItem = serde_json::from_str(&line).expect("a valid stream item"); + assert_eq!(decoded, event); + } + } } diff --git a/lib/apps/fabro-server/src/server/handler/models.rs b/lib/apps/fabro-server/src/server/handler/models.rs index 02c5f92b8..53070acfc 100644 --- a/lib/apps/fabro-server/src/server/handler/models.rs +++ b/lib/apps/fabro-server/src/server/handler/models.rs @@ -299,7 +299,8 @@ fn model_selection_response(error: &ModelSelectionError) -> Response { match error { ModelSelectionError::UnknownProvider { .. } | ModelSelectionError::UnknownSelector { .. } - | ModelSelectionError::UnknownSelectorOnProvider { .. } => { + | ModelSelectionError::UnknownSelectorOnProvider { .. } + | ModelSelectionError::ProviderUsedAsModel { .. } => { ApiError::not_found(error.to_string()).into_response() } ModelSelectionError::ProviderUnavailable { .. } diff --git a/lib/apps/fabro-server/src/server/handler/sessions.rs b/lib/apps/fabro-server/src/server/handler/sessions.rs index b8da8740b..5d6d79db3 100644 --- a/lib/apps/fabro-server/src/server/handler/sessions.rs +++ b/lib/apps/fabro-server/src/server/handler/sessions.rs @@ -1523,10 +1523,8 @@ enabled = true .unwrap(), (openrouter, "future-model".to_string()) ); - assert_eq!( - canonical_session_model(&catalog, &both, Some("future-model"), None).unwrap(), - (openai, "future-model".to_string()) - ); + // Without a pin there is no provider to pass through on. + assert!(canonical_session_model(&catalog, &both, Some("future-model"), None).is_err()); } /// A colon in a model ID does not make it provider-qualified. Ollama @@ -1548,9 +1546,8 @@ enabled = true .unwrap(), (openrouter, "future-model:latest".to_string()) ); - assert_eq!( - canonical_session_model(&catalog, &both, Some("future-model:latest"), None).unwrap(), - (openai, "future-model:latest".to_string()) + assert!( + canonical_session_model(&catalog, &both, Some("future-model:latest"), None).is_err() ); } diff --git a/lib/apps/fabro-server/tests/it/api/sessions.rs b/lib/apps/fabro-server/tests/it/api/sessions.rs index 0605d8989..3c5dd0333 100644 --- a/lib/apps/fabro-server/tests/it/api/sessions.rs +++ b/lib/apps/fabro-server/tests/it/api/sessions.rs @@ -222,11 +222,10 @@ async fn unknown_session_models_preserve_passthrough_on_the_selected_provider() let app = fabro_server::test_support::build_test_router(test_app_state()); let (run_id, _run_id_workspace) = create_run(&app).await; - for model in ["not-a-real-model", "openai/not-a-real-model"] { - let created = create_session_with_model(&app, &run_id, "Ask Fabro", model).await; - assert_eq!(created["model"], "not-a-real-model"); - assert_eq!(created["provider"], "openai"); - } + let created = + create_session_with_model(&app, &run_id, "Ask Fabro", "openai/not-a-real-model").await; + assert_eq!(created["model"], "not-a-real-model"); + assert_eq!(created["provider"], "openai"); } #[tokio::test] @@ -234,7 +233,7 @@ async fn invalid_session_model_refs_are_rejected_at_creation() { let app = fabro_server::test_support::build_test_router(test_app_state()); let (run_id, _run_id_workspace) = create_run(&app).await; - for model in ["openai", "openai/", "anthropic/gpt-5.4"] { + for model in ["openai", "openai/", "anthropic/gpt-5.4", "not-a-real-model"] { let response = create_session_response( &app, &run_id, diff --git a/lib/apps/fabro-server/tests/it/scenario/dry_run.rs b/lib/apps/fabro-server/tests/it/scenario/dry_run.rs index 9ec6aedec..e7b3312c2 100644 --- a/lib/apps/fabro-server/tests/it/scenario/dry_run.rs +++ b/lib/apps/fabro-server/tests/it/scenario/dry_run.rs @@ -167,6 +167,48 @@ async fn completion_non_streaming_returns_valid_json() { assert!(body["usage"]["output"].is_number()); } +/// A provider name in the model field, or a selector the catalog does not +/// know, is refused before any provider is called. Neither may pass through +/// to whichever ready provider wins priority. +#[tokio::test] +async fn completion_with_a_bare_provider_or_unknown_selector_is_rejected_before_any_call() { + let mock_server = MockServer::start_async().await; + let anthropic = mock_server + .mock_async(|when, then| { + when.method("POST").path("/v1/messages"); + then.status(500); + }) + .await; + let app = test_app_with_mock_anthropic(&format!("{}/v1", mock_server.url(""))); + + for (model, expected_detail) in [ + ("gemini", "set provider = \"gemini\""), + ("anthropic", "set provider = \"anthropic\""), + ( + "not-a-real-model", + "unknown model selector 'not-a-real-model'", + ), + ] { + let response = app + .clone() + .oneshot(completion_request_with_model(false, model)) + .await + .unwrap(); + let body = response_json( + response, + StatusCode::BAD_REQUEST, + &format!("POST /api/v1/completions with model {model}"), + ) + .await; + let detail = body["errors"][0]["detail"].as_str().unwrap_or_default(); + assert!( + detail.contains(expected_detail), + "model {model}: unexpected detail {detail:?}" + ); + } + assert_eq!(anthropic.calls_async().await, 0); +} + #[tokio::test] async fn completion_streaming_returns_sse() { let mock_server = MockServer::start_async().await; diff --git a/lib/components/fabro-llm/src/lib.rs b/lib/components/fabro-llm/src/lib.rs index 7d6d6c0fd..d1bbebb55 100644 --- a/lib/components/fabro-llm/src/lib.rs +++ b/lib/components/fabro-llm/src/lib.rs @@ -5,9 +5,10 @@ //! //! - building the catalog from the lithos built-ins and the operator `[llm]` //! overlay, and reading the agent harness a model expects ([`catalog`]); -//! - Fabro's passthrough policy for selections made before a request exists -//! ([`selection`]); at request time the lithos resolver enforces `enabled` -//! and `stands_in_for` itself; +//! - Fabro's selection policy for choices made before a request exists +//! ([`selection`]), including when an uncatalogued model may pass through; at +//! request time the lithos resolver enforces `enabled` and `stands_in_for` +//! itself; //! - constructing a client from a Fabro credential store ([`client`]); //! - model and provider probes ([`probe`]), and the API views of the catalog //! ([`api`]); diff --git a/lib/components/fabro-llm/src/selection.rs b/lib/components/fabro-llm/src/selection.rs index 63ab7aee4..548980388 100644 --- a/lib/components/fabro-llm/src/selection.rs +++ b/lib/components/fabro-llm/src/selection.rs @@ -9,7 +9,10 @@ //! - `provider/model` pins the provider, as the lithos resolver reads it. //! - An unknown selector pinned to a provider passes through verbatim on that //! provider. -//! - An unqualified unknown selector passes through on the default provider. +//! - An unqualified unknown selector is an error. Passthrough needs a provider +//! pin, so a typo never lands on whichever provider happens to win priority. +//! - A bare provider name is an error that points at the provider pin; it is +//! never a model name. //! - No selector picks the default offering (of the pinned provider, when one //! is given). //! @@ -73,6 +76,14 @@ pub enum ModelSelectionError { selector: String, provider: ProviderId, }, + #[error( + "'{selector}' names the provider '{provider}', not a model; set provider = \"{provider}\" \ + and omit the model to use its default" + )] + ProviderUsedAsModel { + selector: String, + provider: ProviderId, + }, #[error( "model selector '{selector}' is known but has no offering on an eligible provider; available providers: {providers:?}" )] @@ -155,6 +166,14 @@ pub fn select<'a>( } let matches = catalog.offerings_matching(selector); if matches.is_empty() { + // A provider name in the model position is the one unknown selector + // with an obvious intent, so say what the caller meant to write. + if let Some(provider) = catalog.enabled_provider(selector) { + return Err(ModelSelectionError::ProviderUsedAsModel { + selector: selector.to_string(), + provider: provider.id().clone(), + }); + } return Err(ModelSelectionError::UnknownSelector { selector: selector.to_string(), }); @@ -230,13 +249,6 @@ pub fn resolve_selection( model: selector, }) } - Err(ModelSelectionError::UnknownSelector { .. }) => { - let default = select_default(catalog, eligible)?; - Ok(SelectedModel { - provider: default.provider.id().clone(), - model: selector.to_string(), - }) - } Err(error) => Err(error), } } @@ -296,17 +308,53 @@ mod tests { } #[test] - fn unknown_selector_passes_through_on_the_default_provider() { + fn unknown_unqualified_selector_is_an_error_not_a_passthrough() { let catalog = test_catalog(); - let selected = resolve_selection( + let error = resolve_selection( &catalog, Some("totally-new-model"), None, &eligible(&["openai", "anthropic"]), ) + .unwrap_err(); + assert_eq!(error, ModelSelectionError::UnknownSelector { + selector: "totally-new-model".to_string(), + }); + } + + #[test] + fn bare_provider_name_is_rejected_with_a_pin_hint() { + let catalog = test_catalog(); + for (selector, provider) in [ + ("openai", builtin::openai()), + ("anthropic", builtin::anthropic()), + ] { + let error = resolve_selection( + &catalog, + Some(selector), + None, + &eligible(&["openai", "anthropic"]), + ) + .unwrap_err(); + assert_eq!(error, ModelSelectionError::ProviderUsedAsModel { + selector: selector.to_string(), + provider: provider.clone(), + }); + assert!( + error + .to_string() + .contains(&format!("provider = \"{provider}\"")) + ); + } + // The pin alone still means that provider's default. + let selected = resolve_selection( + &catalog, + None, + Some(&builtin::openai()), + &eligible(&["openai"]), + ) .unwrap(); - assert_eq!(selected.provider, builtin::anthropic()); - assert_eq!(selected.model, "totally-new-model"); + assert_eq!(selected.provider, builtin::openai()); } #[test] diff --git a/lib/foundation/fabro-redact/Cargo.toml b/lib/foundation/fabro-redact/Cargo.toml index 168c0d74f..a1b6deca1 100644 --- a/lib/foundation/fabro-redact/Cargo.toml +++ b/lib/foundation/fabro-redact/Cargo.toml @@ -22,6 +22,8 @@ ref-cast.workspace = true regex.workspace = true serde_json.workspace = true thiserror.workspace = true +# Recognizes ULID-shaped tokens so Fabro's own IDs are never scanned as secrets. +ulid.workspace = true url.workspace = true [build-dependencies] diff --git a/lib/foundation/fabro-redact/src/entropy.rs b/lib/foundation/fabro-redact/src/entropy.rs index ec6029c3c..fa906f525 100644 --- a/lib/foundation/fabro-redact/src/entropy.rs +++ b/lib/foundation/fabro-redact/src/entropy.rs @@ -1,6 +1,7 @@ use std::sync::LazyLock; use regex::Regex; +use ulid::Ulid; use super::Region; @@ -11,6 +12,24 @@ static SECRET_PATTERN: LazyLock = const ENTROPY_THRESHOLD: f64 = 4.5; +/// The length of a ULID's canonical text form. +const ULID_LEN: usize = 26; + +/// The timestamp window a token must fall in to be recognized as a ULID: +/// 2020-01-01T00:00:00Z through 2100-01-01T00:00:00Z. +/// +/// Fabro's run, session, turn, message and pair IDs are ULIDs, and a +/// ULID occasionally has enough distinct characters to cross the entropy +/// threshold. Exempting the format keeps those IDs intact everywhere they +/// appear (URLs, branch names, commit messages, free text) instead of +/// protecting one field at a time. The window bounds what the exemption +/// can miss: a secret would have to be exactly 26 uppercase Crockford +/// base32 characters whose leading 48 bits decode to a date in this +/// range, which is roughly one in a hundred of the already rare tokens +/// that take that shape. The gitleaks patterns still run on every token. +const ULID_MIN_TIMESTAMP_MS: u64 = 1_577_836_800_000; +const ULID_MAX_TIMESTAMP_MS: u64 = 4_102_444_800_000; + /// Compute Shannon entropy (bits per byte) of a string. pub(super) fn shannon_entropy(s: &str) -> f64 { if s.is_empty() { @@ -68,6 +87,13 @@ pub(super) fn find_entropy_regions(s: &str) -> Vec { start += offset; } + // Identifiers are not secrets: a token that is a ULID, or ends in + // `-ULID`, `_ULID` or `=ULID` segments, is measured without them. + let end = start + ulid_free_len(&s[start..end]); + if end - start < 10 { + continue; + } + if shannon_entropy(&s[start..end]) > ENTROPY_THRESHOLD { regions.push(Region { start, end }); } @@ -97,10 +123,152 @@ fn assignment_value_offset(token: &str) -> Option { Some(eq + 1) } +/// Whether `token` is a ULID in canonical form (26 uppercase Crockford +/// base32 characters) with a timestamp inside the recognized window. +/// +/// The alphabet is checked by hand because the `ulid` crate's decoder is +/// lenient about case; the exemption is meant to be strict. +fn is_ulid(token: &str) -> bool { + token.len() == ULID_LEN + && token.bytes().all(|b| { + matches!( + b, + b'0'..=b'9' | b'A'..=b'H' | b'J' | b'K' | b'M' | b'N' | b'P'..=b'T' | b'V'..=b'Z' + ) + }) + && Ulid::from_string(token).is_ok_and(|ulid| { + (ULID_MIN_TIMESTAMP_MS..ULID_MAX_TIMESTAMP_MS).contains(&ulid.timestamp_ms()) + }) +} + +/// The length of `token` once every trailing ULID segment is removed. +/// +/// A token that is itself a ULID shrinks to nothing. A trailing ULID +/// preceded by `-`, `_` or `=` is stripped together with its separator, +/// repeatedly, so `20261020-` leaves the date and +/// `-` leaves the secret to be measured on its own. Twenty-six +/// trailing characters that are not set off by a separator stay part of the +/// token: they could be the tail of a longer secret. +fn ulid_free_len(token: &str) -> usize { + let mut len = token.len(); + loop { + let Some(id_start) = len.checked_sub(ULID_LEN) else { + return len; + }; + if !is_ulid(&token[id_start..len]) { + return len; + } + if id_start == 0 { + return 0; + } + if !matches!(token.as_bytes()[id_start - 1], b'-' | b'_' | b'=') { + return len; + } + len = id_start - 1; + } +} + #[cfg(test)] mod tests { use super::*; + /// Run IDs that cross the entropy threshold: each has at least 23 + /// distinct characters out of 26. + const HIGH_ENTROPY_ULIDS: [&str; 2] = + ["01M5228EHCT0P73Z4BYGDRWJNS", "01M5228EHCT0P73Z4BYGDRWJNV"]; + + const SECRET: &str = "xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6p"; + + #[test] + fn high_entropy_ulids_cross_the_threshold_on_their_own() { + for id in HIGH_ENTROPY_ULIDS { + let e = shannon_entropy(id); + assert!(e > ENTROPY_THRESHOLD, "{id}: expected > 4.5, got {e}"); + } + } + + #[test] + fn ulids_are_not_secrets() { + for id in HIGH_ENTROPY_ULIDS { + for text in [ + id.to_string(), + format!("fabro/run/{id}"), + format!("http://localhost:3000/runs/{id}"), + format!("run_id={id}"), + format!("20261020-{id}"), + format!("fabro-run-{id}"), + format!("checkpoint_{id}_{id}"), + ] { + let regions = find_entropy_regions(&text); + assert!(regions.is_empty(), "{text}: got regions {regions:?}"); + } + } + } + + #[test] + fn secrets_beside_a_ulid_are_still_found() { + let id = HIGH_ENTROPY_ULIDS[0]; + let text = format!("{SECRET}-{id}"); + assert_eq!(find_entropy_regions(&text), vec![Region { + start: 0, + end: SECRET.len(), + }]); + let text = format!("https://{SECRET}@example.com/runs/{id}"); + assert_eq!(find_entropy_regions(&text), vec![Region { + start: "https://".len(), + end: "https://".len() + SECRET.len(), + }]); + } + + #[test] + fn a_ulid_glued_to_other_characters_is_scanned_as_a_whole() { + // No separator sets the ID off, so the token is measured intact. + let text = format!("x{}", HIGH_ENTROPY_ULIDS[0]); + assert!(shannon_entropy(&text) > ENTROPY_THRESHOLD); + assert_eq!(find_entropy_regions(&text), vec![Region { + start: 0, + end: text.len(), + }]); + } + + #[test] + fn ulid_recognition_is_strict() { + let id = HIGH_ENTROPY_ULIDS[0]; + assert!(is_ulid(id)); + // Lowercase is not canonical, even though the crate would decode it. + assert!(!is_ulid(&id.to_ascii_lowercase())); + assert!(!find_entropy_regions(&id.to_ascii_lowercase()).is_empty()); + // Wrong length or alphabet. + assert!(!is_ulid(&id[1..])); + assert!(!is_ulid(&format!("{id}0"))); + assert!(!is_ulid(&format!("I{}", &id[1..]))); + // A random secret that happens to be 26 base32 characters but whose + // timestamp falls outside the window is still a secret. + let ulid = Ulid::from_string(id).unwrap(); + let year_2010 = Ulid::from_parts(1_262_304_000_000, ulid.random()).to_string(); + let year_2150 = Ulid::from_parts(5_680_281_600_000, ulid.random()).to_string(); + assert!(!is_ulid(&year_2010), "{year_2010}"); + assert!(!is_ulid(&year_2150), "{year_2150}"); + // The window edges. + assert!(is_ulid( + &Ulid::from_parts(ULID_MIN_TIMESTAMP_MS, ulid.random()).to_string() + )); + assert!(!is_ulid( + &Ulid::from_parts(ULID_MAX_TIMESTAMP_MS, ulid.random()).to_string() + )); + } + + #[test] + fn ulid_free_len_strips_trailing_segments_only() { + let id = HIGH_ENTROPY_ULIDS[0]; + assert_eq!(ulid_free_len(id), 0); + assert_eq!(ulid_free_len(&format!("abc-{id}")), 3); + assert_eq!(ulid_free_len(&format!("abc_{id}-{id}")), 3); + assert_eq!(ulid_free_len(&format!("{id}-abc")), id.len() + 4); + assert_eq!(ulid_free_len(&format!("x{id}")), id.len() + 1); + assert_eq!(ulid_free_len("short"), 5); + } + #[test] fn entropy_empty_string() { assert!(shannon_entropy("").abs() < f64::EPSILON); diff --git a/lib/foundation/fabro-redact/src/jsonl.rs b/lib/foundation/fabro-redact/src/jsonl.rs index 84c48457c..f83faf62f 100644 --- a/lib/foundation/fabro-redact/src/jsonl.rs +++ b/lib/foundation/fabro-redact/src/jsonl.rs @@ -209,19 +209,50 @@ mod tests { #[test] fn redact_jsonl_line_preserves_ids_repeated_in_scanned_values() { - // This run ID from a failed CLI attach exceeds the entropy threshold. - // Redacting an event's context value must not also rewrite its envelope. + // Redacting an event's context value must not also rewrite its + // envelope, even when the envelope's ID field holds the same text. + let input = serde_json::json!({ + "run_id": HIGH_ENTROPY_SECRET, + "item": { "context": { "value": HIGH_ENTROPY_SECRET } }, + }); + let result = redact_jsonl_line(&input.to_string()); + let redacted: Value = serde_json::from_str(&result).unwrap(); + + assert_eq!(redacted["run_id"], HIGH_ENTROPY_SECRET); + assert_eq!(redacted["item"]["context"]["value"], "REDACTED"); + assert_eq!(redacted, redact_json_value(input)); + } + + #[test] + fn redact_jsonl_line_keeps_run_ids_wherever_they_appear() { + // This run ID from a failed CLI attach exceeds the entropy threshold + // on its own; the ULID exemption keeps it intact in scanned values, + // not only in the skipped `run_id` envelope field. let run_id = "01M5228EHCT0P73Z4BYGDRWJNS"; let input = serde_json::json!({ "run_id": run_id, - "item": { "context": { "value": run_id } }, + "item": { "record": { + "web_url": format!("http://localhost:3000/runs/{run_id}"), + "run_branch": format!("fabro/run/{run_id}"), + "message": format!("checkpoint for run {run_id}: {HIGH_ENTROPY_SECRET}"), + } }, }); let result = redact_jsonl_line(&input.to_string()); let redacted: Value = serde_json::from_str(&result).unwrap(); assert_eq!(redacted["run_id"], run_id); - assert_eq!(redacted["item"]["context"]["value"], "REDACTED"); - assert_eq!(redacted, redact_json_value(input)); + assert_eq!( + redacted["item"]["record"]["web_url"], + format!("http://localhost:3000/runs/{run_id}") + ); + assert_eq!( + redacted["item"]["record"]["run_branch"], + format!("fabro/run/{run_id}") + ); + assert_eq!( + redacted["item"]["record"]["message"], + format!("checkpoint for run {run_id}: REDACTED") + ); } #[test] diff --git a/lib/foundation/fabro-redact/src/lib.rs b/lib/foundation/fabro-redact/src/lib.rs index 84d8bca28..f07496f0e 100644 --- a/lib/foundation/fabro-redact/src/lib.rs +++ b/lib/foundation/fabro-redact/src/lib.rs @@ -1,8 +1,12 @@ //! Secret and credential redaction utilities. //! //! `redact_string`, `redact_json_value`, and `redact_jsonl_line` provide -//! generic secret scanning. [`DisplaySafeUrl`] provides deterministic URL -//! redaction for logging and error-message boundaries. +//! generic secret scanning: Shannon entropy over alphanumeric tokens plus +//! gitleaks credential patterns. Tokens that are ULIDs (Fabro's run, session, +//! turn, message and pair IDs) are exempt from the entropy layer, so a run ID +//! that happens to have many distinct characters is never turned into +//! `REDACTED`. [`DisplaySafeUrl`] provides deterministic URL redaction for +//! logging and error-message boundaries. mod entropy; mod gitleaks; diff --git a/lib/packages/fabro-api-client/src/models/create-completion-request.ts b/lib/packages/fabro-api-client/src/models/create-completion-request.ts index c6f60ca40..e9646276c 100644 --- a/lib/packages/fabro-api-client/src/models/create-completion-request.ts +++ b/lib/packages/fabro-api-client/src/models/create-completion-request.ts @@ -41,11 +41,11 @@ export interface CreateCompletionRequest { */ 'messages': Array; /** - * Model selector: `provider/model`, a model id or alias, or a provider id. The server picks a ready-provider default when omitted. + * Model selector: `provider/model`, or a model id or alias. A bare provider name is rejected; pin it with `provider` instead. When omitted, the server picks the pinned provider\'s default model, or a ready-provider default when `provider` is also omitted. */ 'model'?: string; /** - * Optional provider pin for a bare model selector. + * Optional provider pin. Scopes a bare model selector to that provider, which also lets an uncatalogued model id reach a provider that allows passthrough. */ 'provider'?: string; /**