diff --git a/.github/workflows/ci-cd.yml b/.github/workflows/ci-cd.yml index f4aee899..0621991f 100644 --- a/.github/workflows/ci-cd.yml +++ b/.github/workflows/ci-cd.yml @@ -99,6 +99,8 @@ jobs: apps/api/src/services/cli-conversation.test.ts apps/api/src/services/dataset-search-fitness.test.ts apps/api/src/services/cli-runtime-recovery.test.ts + apps/api/src/services/cli-runtime-liveness.test.ts + apps/api/src/services/cli-run-selection.test.ts apps/api/src/services/pdf-service.test.ts apps/api/src/services/pdf-attachments.test.ts apps/api/src/services/vendor-evidence-lines.test.ts @@ -119,6 +121,8 @@ jobs: packages/mcp-server/src/api-client.test.ts packages/mcp-server/src/modifier-percent.test.ts packages/mcp-server/src/summary-row-contract.test.ts + packages/mcp-server/src/tools/source-refs.test.ts + packages/mcp-server/src/tools/evidence-gate-contract.test.ts apps/api/src/revision-rate-schedule-scoping.test.ts apps/api/src/answer-resumes-run.test.ts apps/api/src/quote-status-source.test.ts diff --git a/apps/api/src/answer-resumes-run.test.ts b/apps/api/src/answer-resumes-run.test.ts index 444780d2..fe8bede7 100644 --- a/apps/api/src/answer-resumes-run.test.ts +++ b/apps/api/src/answer-resumes-run.test.ts @@ -24,7 +24,20 @@ test("answering a question resumes a run that already stopped", () => { // Without this the answer sat in run history with nobody polling for it. const body = answerHandler(); assert.match(body, /startResumedSession\(request, \{/, "must start a resumed session"); - assert.match(body, /if \(session\) \{/, "must not resume when a live session is already polling"); +}); + +test("answering a question does not resume on top of a live agent", () => { + // Deciding this from the in-memory handle was the bug: an agent we had lost + // track of looked stopped, so we resumed over it, the runtime refused the + // second writer on the thread, and the user saw a failure -- while the + // original run had the answer and went on to finish. Liveness must be probed. + // Behaviour is covered in services/cli-runtime-liveness.test.ts. + const body = answerHandler(); + const resumeIndex = body.indexOf("startResumedSession(request, {"); + const guardIndex = body.indexOf("liveAgent.live"); + assert.notEqual(guardIndex, -1, "must probe for a live agent"); + assert.ok(guardIndex < resumeIndex, "the probe must gate the resume"); + assert.match(body, /probeLiveAgent\(projectId, resolveProjectDir\(projectId\)\)/); }); test("the resumed agent is told the question and the answer", () => { diff --git a/apps/api/src/resume-session-guard.test.ts b/apps/api/src/resume-session-guard.test.ts index aac0300c..b514c410 100644 --- a/apps/api/src/resume-session-guard.test.ts +++ b/apps/api/src/resume-session-guard.test.ts @@ -2,6 +2,18 @@ import assert from "node:assert/strict"; import test from "node:test"; import { readFileSync } from "node:fs"; +/** + * Route-level wiring for the "one live agent per project" rule. + * + * The rule itself is exercised for real in + * `services/cli-runtime-liveness.test.ts`. It has to be: this file used to + * assert that `resumeSession` contained `session.status === "running"`, which + * stayed true for three weeks while the guard did nothing in production -- the + * session registry it consulted had been emptied by an unrelated timer, so the + * check never fired and resumes kept colliding with running agents. Matching + * source text cannot tell a working guard from an inert one. + */ + const runtime = readFileSync(new URL("./services/cli-runtime.ts", import.meta.url), "utf8"); const routes = readFileSync(new URL("./routes/cli-routes.ts", import.meta.url), "utf8"); @@ -13,27 +25,16 @@ function functionBody(source: string, signature: string) { return next === -1 ? rest : rest.slice(0, next); } -test("resume refuses to start on top of a running session", () => { - // The real incident: a resume was issued 4 minutes into a session that kept - // running for another 90 seconds. Codex refused the second writer -- - // "thread-store conflict: thread already has an active writer" -- and the - // new run died. spawnSession already guarded this; resume did not. +test("the resume guard probes the process rather than trusting the registry", () => { const body = functionBody(runtime, "export async function resumeSession("); - assert.match(body, /session\.status === "running"/, "must check the live session's status"); - const guardIndex = body.indexOf('session.status === "running"'); - const sessionIdIndex = body.indexOf("let sessionId"); + const guardIndex = body.indexOf("probeLiveAgent("); + assert.notEqual(guardIndex, -1, "must probe for a live agent"); assert.ok( - guardIndex < sessionIdIndex, + guardIndex < body.indexOf("let sessionId"), "the guard must run before resolving a session id to resume", ); }); -test("the guard reports a conflict, not a generic failure", () => { - const body = functionBody(runtime, "export async function resumeSession("); - assert.match(body, /statusCode: 409/, "409 so callers can distinguish it from a crash"); - assert.match(body, /Stop it before resuming/, "tells the user what to do"); -}); - test("spawn and resume agree that one live session per project is the rule", () => { const spawn = functionBody(runtime, "export async function spawnSession("); assert.match(spawn, /existing\.status === "running"/, "spawn already had this guard"); diff --git a/apps/api/src/routes/cli-routes.ts b/apps/api/src/routes/cli-routes.ts index 991025ec..ea7b0337 100644 --- a/apps/api/src/routes/cli-routes.ts +++ b/apps/api/src/routes/cli-routes.ts @@ -5,7 +5,7 @@ */ import type { FastifyInstance, FastifyRequest } from "fastify"; -import { detectCli, checkCliAuth, spawnSession, stopSession, resumeSession, getSession, listSessions, listCliModels, type AgentChatMode, type AgentRuntime } from "../services/cli-runtime.js"; +import { detectCli, checkCliAuth, spawnSession, stopSession, resumeSession, getSession, probeLiveAgent, listSessions, listCliModels, type AgentChatMode, type AgentRuntime } from "../services/cli-runtime.js"; import { startLoginSession, attachLoginSession, @@ -25,6 +25,7 @@ import { writeAgentLibrarySnapshot } from "../services/agent-library-snapshot.js import { stripBlankCredentialEnv } from "../services/agent-host/env-sanitize.js"; import { getAgentRuntimeHost } from "../services/agent-host/index.js"; import { buildModeConversationContext } from "../services/cli-conversation.js"; +import { selectLatestRun } from "../services/cli-run-selection.js"; import { resolveAgentProviderKeys, resolveRuntimeProviderKey, @@ -2057,7 +2058,7 @@ ${message}`; }; } - const latestRun = runs[runs.length - 1]; + const latestRun = selectLatestRun(runs); const latestRunEvents = ((latestRun?.output as any)?.events || []) as Array<{ type?: string; timestamp?: string; @@ -2551,7 +2552,15 @@ Merge tables that span multiple pages. Skip non-data pages. // already stopped there is nobody polling, so the answer would sit in // history forever. Questions have no deadline by design — the answer can // arrive the next day — so restart the run and hand it the answer. - if (session) { + // + // "Still running" has to be decided by probing the process, not by whether + // this API process still holds the handle. Trusting the handle alone meant + // a running agent that we had lost track of looked stopped: we resumed on + // top of it, the runtime refused the second writer on the same thread, and + // the user saw a failure even though the answer had been delivered and the + // original run went on to finish normally. + const liveAgent = await probeLiveAgent(projectId, resolveProjectDir(projectId)); + if (liveAgent.live) { return { ok: true, message: "Answer delivered to agent", resumed: false }; } diff --git a/apps/api/src/services/cli-run-selection.test.ts b/apps/api/src/services/cli-run-selection.test.ts new file mode 100644 index 00000000..6b5261a6 --- /dev/null +++ b/apps/api/src/services/cli-run-selection.test.ts @@ -0,0 +1,86 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { isOpenRun, isStillbornRun, selectLatestRun } from "./cli-run-selection.js"; + +const statusEvent = (status: string) => ({ type: "status", data: { status } }); + +/** A run that started and is still going. */ +const openRun = (id: string) => ({ + id, + status: "running", + output: { events: [statusEvent("running"), { type: "tool_call", data: {} }] }, +}); + +/** A run that started and finished. */ +const finishedRun = (id: string, status = "completed") => ({ + id, + status, + output: { events: [statusEvent("running"), statusEvent(status)] }, +}); + +/** + * A resume that collided with a live agent: it failed without the CLI ever + * reporting "running", because the process never got that far. + */ +const stillbornRun = (id: string) => ({ + id, + status: "failed", + output: { + events: [ + { type: "error", data: { message: "thread-store conflict: thread 01a0 already has an active writer" } }, + { type: "message", data: { role: "assistant", content: "Intake failed (exit code 1)." } }, + statusEvent("failed"), + ], + }, +}); + +test("classifies a collided resume as stillborn and a working run as open", () => { + assert.equal(isStillbornRun(stillbornRun("r2")), true); + assert.equal(isStillbornRun(finishedRun("r1", "failed")), false, "a run that started then failed is a real failure"); + assert.equal(isOpenRun(openRun("r1")), true); + assert.equal(isOpenRun(finishedRun("r1")), false); +}); + +test("a failed resume does not mask the run that is still working", () => { + // The incident: answering a question resumed on top of a live agent, the + // runtime refused the second writer, and that sub-second failure became the + // newest run -- so the whole project reported "failed" while the estimate was + // still being built. + const live = openRun("run-live"); + const latest = selectLatestRun([finishedRun("run-0"), live, stillbornRun("run-collision")]); + assert.equal(latest, live, "status must follow the run that is actually running"); +}); + +test("repeated failed resumes still do not mask it", () => { + // Every Resume click appended another stillborn run, so a single-step lookback + // would have gone straight back to reporting a failure. + const live = openRun("run-live"); + const latest = selectLatestRun([ + live, + stillbornRun("c1"), + stillbornRun("c2"), + stillbornRun("c3"), + stillbornRun("c4"), + ]); + assert.equal(latest, live); +}); + +test("a genuine start-up failure is still reported", () => { + // Nothing else is running, so this failure is the truth about the project and + // must not be hidden behind the previous run's success. + const collision = stillbornRun("run-failed-start"); + const latest = selectLatestRun([finishedRun("run-0"), collision]); + assert.equal(latest, collision); +}); + +test("a failure after the live run ends is reported once it is the truth", () => { + const latest = selectLatestRun([finishedRun("run-0"), stillbornRun("c1")]); + assert.equal((latest as { id: string }).id, "c1"); +}); + +test("the newest run wins in the ordinary case", () => { + const newest = openRun("run-2"); + assert.equal(selectLatestRun([finishedRun("run-1"), newest]), newest); + assert.equal(selectLatestRun([]), undefined); +}); diff --git a/apps/api/src/services/cli-run-selection.ts b/apps/api/src/services/cli-run-selection.ts new file mode 100644 index 00000000..b6b9595f --- /dev/null +++ b/apps/api/src/services/cli-run-selection.ts @@ -0,0 +1,63 @@ +/** + * Choosing which AiRun represents a project's current agent state. + * + * "Newest run wins" is right almost always, and wrong in one case that matters: + * a resume issued against an agent that is still working dies in a few hundred + * milliseconds (the runtime refuses a second writer on the same thread). That + * stillborn run is the newest, so it became the run the status endpoint + * reported — turning a healthy, still-running estimate into a visible failure. + * Each retry appended another one, so every attempt to recover re-applied the + * mask. These helpers skip that tail while a real run is still open. + */ + +export interface SelectableRun { + status?: string; + output?: unknown; +} + +const TERMINAL_RUN_STATUSES = new Set(["completed", "failed", "stopped"]); + +function statusEventValues(run: SelectableRun | undefined): string[] { + const events = ((run?.output as { events?: unknown })?.events || []) as Array<{ + type?: string; + data?: { status?: unknown }; + }>; + if (!Array.isArray(events)) return []; + return events + .filter((event) => event?.type === "status") + .map((event) => String(event?.data?.status ?? "")); +} + +/** + * A run whose process never came up: marked failed, and it never once reported + * "running". A CLI that starts at all emits a running status before anything + * else, so its absence is a reliable "this never began". + */ +export function isStillbornRun(run: SelectableRun | undefined): boolean { + if (run?.status !== "failed") return false; + return !statusEventValues(run).includes("running"); +} + +/** A run the DB still has open, with no terminal status event in its transcript. */ +export function isOpenRun(run: SelectableRun | undefined): boolean { + if (run?.status !== "running") return false; + return !statusEventValues(run).some((status) => TERMINAL_RUN_STATUSES.has(status)); +} + +/** + * Pick the run that represents the project's current state. + * + * Falls back to the newest run whenever the last run that actually started has + * finished, so a genuine start-up failure with nothing else running still + * surfaces to the user rather than silently reporting the previous run. + */ +export function selectLatestRun(runs: T[]): T | undefined { + if (runs.length === 0) return undefined; + const newest = runs[runs.length - 1]; + if (!isStillbornRun(newest)) return newest; + + let index = runs.length - 1; + while (index >= 0 && isStillbornRun(runs[index])) index -= 1; + const lastStarted = index >= 0 ? runs[index] : undefined; + return lastStarted && isOpenRun(lastStarted) ? lastStarted : newest; +} diff --git a/apps/api/src/services/cli-runtime-liveness.test.ts b/apps/api/src/services/cli-runtime-liveness.test.ts new file mode 100644 index 00000000..d5e5d008 --- /dev/null +++ b/apps/api/src/services/cli-runtime-liveness.test.ts @@ -0,0 +1,150 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { mkdtemp, mkdir, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { probeLiveAgent, reapSession, resumeSession } from "./cli-runtime.js"; + +/** + * Above pid_max on both Linux (2^22) and macOS (~99998), so it can never name a + * running process. + */ +const DEAD_PID = 2_147_483_647; + +async function projectDirWithSessionState(state: Record) { + const dir = await mkdtemp(join(tmpdir(), "bidwright-liveness-")); + await mkdir(join(dir, ".bidwright"), { recursive: true }); + await writeFile(join(dir, ".bidwright", "session.json"), JSON.stringify(state)); + return dir; +} + +// ── The eviction that caused the incident ────────────────────────────────── + +test("reaping a finished session does not evict the run that took its slot", () => { + // The exact shape of the failure: run 1 completes and schedules its reap; + // run 2 starts on the same project inside the reap delay and claims the slot; + // run 1's timer then fires. Deleting by key alone left the API blind to run 2, + // which was still working and went on to finish normally. + const registry = new Map(); + const finished = { projectId: "project-1", name: "run-1" }; + const tookOverTheSlot = { projectId: "project-1", name: "run-2" }; + + registry.set("project-1", finished); + registry.set("project-1", tookOverTheSlot); + + assert.equal(reapSession(registry, finished), false, "must not evict a newer session"); + assert.equal(registry.get("project-1"), tookOverTheSlot, "the live run stays reachable"); +}); + +test("reaping a finished session still clears its own slot", () => { + const registry = new Map(); + const finished = { projectId: "project-1" }; + registry.set("project-1", finished); + + assert.equal(reapSession(registry, finished), true); + assert.equal(registry.has("project-1"), false); +}); + +// ── Liveness is decided by probing, not by holding a handle ──────────────── + +test("a project with no persisted session state has no live agent", async () => { + const dir = await mkdtemp(join(tmpdir(), "bidwright-liveness-")); + const probe = await probeLiveAgent("project-none", dir); + assert.equal(probe.live, false); + assert.equal(probe.source, "none"); +}); + +test("a running agent owned by this API process reads as live", async () => { + const dir = await projectDirWithSessionState({ + pid: process.pid, + ownerPid: process.pid, + status: "running", + sessionId: "thread-abc", + runtime: "openrouter", + }); + + const probe = await probeLiveAgent("project-live", dir); + assert.equal(probe.live, true, "lost the handle, but the process is still there"); + assert.equal(probe.source, "disk"); + assert.equal(probe.sessionId, "thread-abc"); +}); + +test("a recorded pid that is no longer running does not read as live", async () => { + const dir = await projectDirWithSessionState({ + pid: DEAD_PID, + ownerPid: process.pid, + status: "running", + }); + + assert.equal((await probeLiveAgent("project-dead", dir)).live, false); +}); + +test("a pid owned by a different API process is never probed", async () => { + // A pid from a previous API process, or restored with a workspace snapshot + // taken on another host, names a process that is not ours -- and pid numbers + // get reused. `process.pid` is very much alive here, so only the ownership + // check can keep this from being reported as a live agent. + const dir = await projectDirWithSessionState({ + pid: process.pid, + ownerPid: process.pid + 1, + status: "running", + }); + + assert.equal((await probeLiveAgent("project-foreign", dir)).live, false); +}); + +test("a session that finished does not read as live", async () => { + const dir = await projectDirWithSessionState({ + pid: process.pid, + ownerPid: process.pid, + status: "completed", + }); + + assert.equal((await probeLiveAgent("project-done", dir)).live, false); +}); + +// ── The guard that the eviction used to disable ──────────────────────────── + +test("resume refuses to start on top of an agent it can still see running", async () => { + // Codex rejects the second writer on a thread outright -- "thread-store + // conflict: thread already has an active writer" -- so the resumed run + // dies instantly while the original keeps going. Refuse it here, with a 409 + // the caller can distinguish from a crash, rather than letting the runtime + // produce a raw error the user reads as "my estimate failed". + const dir = await projectDirWithSessionState({ + pid: process.pid, + ownerPid: process.pid, + status: "running", + sessionId: "thread-abc", + runtime: "openrouter", + }); + + await assert.rejects( + () => resumeSession({ projectId: "project-live", projectDir: dir, prompt: "continue" }), + (err: Error & { statusCode?: number }) => { + assert.equal(err.statusCode, 409, "409, not a generic 500"); + assert.match(err.message, /already running/i); + assert.match(err.message, /Stop it before resuming/, "tells the user what to do"); + return true; + }, + ); +}); + +test("resume proceeds when the recorded agent is gone", async () => { + // Not live -> the guard must not fire. This one gets past the guard and fails + // later, on the session id it cannot resume, which is the correct next error. + const dir = await projectDirWithSessionState({ + pid: DEAD_PID, + ownerPid: process.pid, + status: "running", + }); + + await assert.rejects( + () => resumeSession({ projectId: "project-dead", projectDir: dir, prompt: "continue" }), + (err: Error & { statusCode?: number }) => { + assert.notEqual(err.statusCode, 409, "the liveness guard must not have fired"); + return true; + }, + ); +}); diff --git a/apps/api/src/services/cli-runtime.ts b/apps/api/src/services/cli-runtime.ts index 230a845a..d32bb111 100644 --- a/apps/api/src/services/cli-runtime.ts +++ b/apps/api/src/services/cli-runtime.ts @@ -102,6 +102,8 @@ const sessions = new Map(); const interruptingProjects = new Set(); const lastBackgroundInterruptAtByProject = new Map(); const BACKGROUND_INTERRUPT_COOLDOWN_MS = 2 * 60_000; +/** How long a finished session stays readable in the registry before reaping. */ +const SESSION_REAP_DELAY_MS = 5 * 60_000; /** * Tool-call timing state shared across sessions (preserves prior behavior * where the original `toolStartTimes` was a module-level Map). @@ -114,6 +116,114 @@ function cliRunId(prefix = "cli") { return `${prefix}-${Date.now()}-${Math.random().toString(16).slice(2, 6)}`; } +/** + * Drop a finished session from a registry — but only if its project slot has + * not already been taken over by a newer session. + * + * Deleting by key alone was a live-session bug: a follow-up run started within + * the reap delay takes the slot, and the *previous* run's pending timer then + * evicted the new, running session. The API went blind to its own agent — + * `resumeSession`'s "already running" guard stopped firing, and answering an + * askUser prompt spawned a second process against the same runtime thread, + * which the CLI rejected with "thread-store conflict: ... already has an active + * writer". The original run kept working (its listeners hold the session + * directly, not the map entry), so the UI showed a failure over a healthy run. + * + * Returns whether anything was removed. + */ +export function reapSession( + registry: Map, + finished: T, +): boolean { + if (registry.get(finished.projectId) !== finished) return false; + registry.delete(finished.projectId); + return true; +} + +/** Reap a finished session once its transcript has had time to drain. */ +function scheduleSessionReap(session: CliSession): void { + setTimeout(() => { + reapSession(sessions, session); + }, SESSION_REAP_DELAY_MS).unref?.(); +} + +/** Probe whether a pid is alive. Signal 0 delivers nothing; it only checks. */ +function isPidAlive(pid: unknown): boolean { + if (typeof pid !== "number" || !Number.isInteger(pid) || pid <= 0) return false; + try { + process.kill(pid, 0); + return true; + } catch (err) { + // EPERM means the process exists but we may not signal it — still alive. + return (err as NodeJS.ErrnoException)?.code === "EPERM"; + } +} + +export interface LiveAgentProbe { + live: boolean; + /** Where the answer came from: the registry, the on-disk state, or nothing. */ + source: "memory" | "disk" | "none"; + sessionId?: string; + runtime?: AgentRuntime; + pid?: number; +} + +/** + * Is an agent process still running for this project? + * + * The in-memory registry is not authoritative — it is per-process and can lose + * an entry while the child is still working. `.bidwright/session.json` is the + * second opinion: it carries the child pid plus `ownerPid`, the API process + * that spawned it. We trust the recorded pid only when `ownerPid` is us, + * because a pid from a previous API process (or from a workspace snapshot + * restored on another host) refers to a process that either no longer exists + * or was never ours, and pid numbers get reused. + * + * Callers use this to tell "no agent is running" apart from "we lost the + * handle" — the distinction that decides whether answering a question or + * pressing Resume may spawn a second process on the same runtime thread. + */ +export async function probeLiveAgent( + projectId: string, + projectDir?: string, +): Promise { + const session = sessions.get(projectId); + if (session) { + return { + live: session.status === "running" && isPidAlive(session.pid || session.process?.pid), + source: "memory", + sessionId: session.sessionId, + runtime: session.runtime, + pid: session.pid, + }; + } + + if (!projectDir) return { live: false, source: "none" }; + + const sessionJsonPath = join(projectDir, ".bidwright", "session.json"); + if (!existsSync(sessionJsonPath)) return { live: false, source: "none" }; + + try { + const saved = JSON.parse(await readFile(sessionJsonPath, "utf-8")) as { + pid?: number; + ownerPid?: number; + status?: string; + sessionId?: string; + runtime?: AgentRuntime; + }; + const ownedByThisApi = saved.ownerPid === process.pid; + return { + live: ownedByThisApi && saved.status === "running" && isPidAlive(saved.pid), + source: "disk", + sessionId: saved.sessionId, + runtime: saved.runtime, + pid: saved.pid, + }; + } catch { + return { live: false, source: "none" }; + } +} + function sanitizeRuntimeEventForPersistence(value: unknown): unknown { if (typeof value === "string") { const redacted = value.replace(/[A-Za-z0-9+/=]{2000,}/g, "[large encoded payload omitted]"); @@ -235,6 +345,10 @@ async function persistSessionState( join(sessionJsonDir, "session.json"), JSON.stringify({ pid: session.process.pid, + // The API process that owns that pid. A pid written by an earlier API + // process (or restored with a workspace snapshot from another host) must + // never be probed for liveness — see probeLiveAgent. + ownerPid: process.pid, runtime: session.runtime, sessionId: session.sessionId, startedAt: session.startedAt, @@ -655,12 +769,7 @@ function wireChildProcess( data: { status: session.status, exitCode: code, signal }, }); events.emit("done", session.status); - setTimeout( - () => { - sessions.delete(session.projectId); - }, - 5 * 60 * 1000, - ); + scheduleSessionReap(session); }); child.on("error", (err) => { @@ -668,12 +777,7 @@ function wireChildProcess( session.status = "failed"; events.emit("event", { type: "error", data: { message: err.message } }); events.emit("done", "failed"); - setTimeout( - () => { - sessions.delete(session.projectId); - }, - 5 * 60 * 1000, - ); + scheduleSessionReap(session); }); } @@ -936,7 +1040,12 @@ export async function resumeSession(opts: ResumeSessionOpts): Promise already // has an active writer" — killing the new run. Codex was right to refuse: the // previous session was mid-flight and kept emitting for another 90 seconds. - if (session && session.status === "running") { + // + // The check goes through probeLiveAgent rather than the registry alone: when + // the in-memory entry was missing the guard silently passed and we collided + // anyway, which is the failure it was written to prevent. + const liveAgent = await probeLiveAgent(opts.projectId, opts.projectDir); + if (liveAgent.live) { throw Object.assign( new Error( `A session is already running for this project. Stop it before resuming, or wait for it to finish.`, diff --git a/packages/mcp-server/src/tools/estimate-tools.ts b/packages/mcp-server/src/tools/estimate-tools.ts index 54905407..7f315cb3 100644 --- a/packages/mcp-server/src/tools/estimate-tools.ts +++ b/packages/mcp-server/src/tools/estimate-tools.ts @@ -1,5 +1,6 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; +import { sourceRefArray } from "./source-refs.js"; import { apiGet, apiPost, getProjectId } from "../api-client.js"; @@ -348,7 +349,7 @@ export function registerEstimateTools(server: McpServer) { id: z.string(), name: z.string(), kind: z.string(), - sourceRefs: z.array(z.string()).default([]), + sourceRefs: sourceRefArray(), quantityBasis: z.string().optional(), quantities: z.record(z.union([z.string(), z.coerce.number(), z.boolean()])).optional(), included: z.boolean().default(true), diff --git a/packages/mcp-server/src/tools/evidence-gate-contract.test.ts b/packages/mcp-server/src/tools/evidence-gate-contract.test.ts new file mode 100644 index 00000000..d7ff8e39 --- /dev/null +++ b/packages/mcp-server/src/tools/evidence-gate-contract.test.ts @@ -0,0 +1,146 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { readFileSync } from "node:fs"; + +/** + * Guards the evidence-gate contract against the failure seen on the Birla + * electrical quote, where 60 of 65 createWorksheetItem calls were rejected. + * + * The rules live inside a closure in quote-tools.ts, so these reimplement the + * two predicates from the source rather than importing them. The source is read + * here so a change to the regex that does not update this file is visible. + */ + +const source = readFileSync(new URL("./quote-tools.ts", import.meta.url), "utf8"); + +/** The separator class in looksLikeStructuredSourceRef's id patterns. */ +function structuredRefPatterns() { + const prefixed = source.match(/if \((\/\^\(doc\|document[^)]+?\)\[[^\]]+\]\/i)\.test\(value\)\) return true;/); + const bareId = source.match(/if \((\/\^\[a-z\]\{2,8\}\[?[^/]*?\/i)\.test\(value\)\) return true;/); + assert.ok(prefixed, "prefixed-id pattern not found in looksLikeStructuredSourceRef"); + assert.ok(bareId, "bare-id pattern not found in looksLikeStructuredSourceRef"); + return [prefixed[1], bareId[1]].map((literal) => { + const body = literal.slice(1, literal.lastIndexOf("/")); + return new RegExp(body, "i"); + }); +} + +function looksStructured(value: string) { + return structuredRefPatterns().some((pattern) => pattern.test(value)); +} + +// ── The ids the system actually mints ────────────────────────────────────── + +test("every id format the system mints counts as a structured source ref", () => { + // doc_ and lis_ use an underscore. A hyphen-only rule scored a correctly + // cited source document as zero structured refs, so the row was rejected for + // "needs structured cite" no matter how accurately the agent cited it. + const minted = [ + "doc_b25cb79d-b387-45f1-9248-3eaf2641335c", // SourceDocument + "lis_0001ce9912de0cef8faa8c84a220143e", // LineItemSearchDocument + "ecost-816cd480-1fb8-41d3-bef4-38646887b7ea", // EffectiveCost + "rci-b4124b70-0415-4f01-8bb8-ba4689d37af7", // ResourceCatalogItem + "kb-b32cb26d-362d-45e5-a414-ee3162b74eaf", // KnowledgeBook + "ds-9f87cb2a-c384-4300-a280-6f627db1c0aa", // Dataset + "lu-3229b9dc-1f9b-4313-a911-ec75a619af2b", // LaborUnit + "rsi-4ec986af-dbf3-4674-a5ff-5c5344044cb1", // RateScheduleItem + ]; + for (const id of minted) { + assert.ok(looksStructured(id), `${id} must count as a structured cite`); + } +}); + +test("prose is still not a structured source ref", () => { + // The agent tried these when it could not work out the required format; they + // must keep failing, or the gate stops meaning anything. + for (const value of ["best judgment", "see notes", "similar project", "n/a"]) { + assert.equal(looksStructured(value), false, `${value} must not pass`); + } +}); + +test("the rejection hint only names prefixes that exist", () => { + const hint = source.match(/const STRUCTURED_SOURCE_REF_HINT = "([^"]+)"/); + assert.ok(hint, "hint constant not found"); + assert.ok( + !/\bdoc-; SourceDocument mints doc_", + ); + assert.match(hint[1], /doc_/, "names the real document id form"); + for (const message of [ + "Labour row needs laborUnitId", + "Material/Sub/Equip/Allowance row needs", + "row needs costResourceId/effectiveCostId/itemId", + ]) { + const index = source.indexOf(message); + assert.notEqual(index, -1, `gate message missing: ${message}`); + assert.match( + source.slice(index - 120, index + 320), + /STRUCTURED_SOURCE_REF_HINT/, + `"${message}" must show the agent a usable cite format`, + ); + } +}); + +// ── Truncated tool calls ─────────────────────────────────────────────────── + +/** Mirrors looksLikeTruncatedItemPayload. */ +function looksTruncated(input: Record) { + const fields = JSON.parse( + (source.match(/const WORKSHEET_ITEM_PAYLOAD_FIELDS = \[([\s\S]*?)\] as const;/) as RegExpMatchArray)[1] + .replace(/,\s*$/, "") + .replace(/^/, "[") + .replace(/$/, "]") + .replace(/'/g, '"'), + ) as string[]; + const carries = fields.some((key) => { + const value = input[key]; + if (value === undefined || value === null || value === "") return false; + if (typeof value === "object" && Object.keys(value as object).length === 0) return false; + return true; + }); + if (carries) return false; + return !String(input.description ?? "").trim() && !String(input.sourceNotes ?? "").trim(); +} + +test("a call carrying only the required fields is reported as truncated", () => { + // The exact payload recorded 39 times on the Birla run, which the server + // answered with "Line evidence basis is required" -- true, but it sent the + // agent rewriting evidenceBasis instead of resending a smaller call. + assert.equal( + looksTruncated({ + entityName: 'Cable Tray — Aluminum Ladder 12" (supply)', + worksheetId: "worksheet-a3ca93c9-15a7-4a3a-b4ba-0f97f307735c", + description: "", + sourceNotes: "", + quantity: 1, + uom: "EA", + }), + true, + ); +}); + +test("a real row is never mistaken for a truncated one", () => { + assert.equal( + looksTruncated({ + entityName: "Electrician", + worksheetId: "worksheet-1", + categoryId: "ecat-240a9ccf", + quantity: 1, + uom: "HR", + sourceNotes: "NECA-12233 VFD 25-50 HP 9.5 HR/EA x 2", + }), + false, + ); + assert.equal( + looksTruncated({ entityName: "Row", worksheetId: "w1", evidenceBasis: { type: "mixed" } }), + false, + ); +}); + +test("the truncation message tells the agent to change shape, not to retry", () => { + const message = source.match(/const TRUNCATED_ITEM_PAYLOAD_MESSAGE = \[([\s\S]*?)\]\.join/); + assert.ok(message, "truncation message not found"); + assert.match(message[1], /cut off/i, "names the actual cause"); + assert.match(message[1], /createRateScheduleWorksheetItem/, "points at the smaller tool"); + assert.match(message[1], /Do not retry the same way/i, "stops the retry loop"); +}); diff --git a/packages/mcp-server/src/tools/quote-tools.ts b/packages/mcp-server/src/tools/quote-tools.ts index 33a8a1b5..275ed0d5 100644 --- a/packages/mcp-server/src/tools/quote-tools.ts +++ b/packages/mcp-server/src/tools/quote-tools.ts @@ -1,5 +1,6 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; +import { sourceRefArray } from "./source-refs.js"; import { apiGet, apiPost, apiPatch, apiDelete, projectPath, getRevisionId } from "../api-client.js"; import { rollupWorksheetUnits } from "@bidwright/domain"; @@ -756,14 +757,63 @@ function claimIdsMentionedInLine(input: { return [...new Set([...text.matchAll(/\bclaim-[0-9a-f]{12}\b/gi)].map((match) => match[0]))]; } +/** Fields that carry the substance of a row. A real row sets at least one. */ +const WORKSHEET_ITEM_PAYLOAD_FIELDS = [ + "categoryId", "category", "entityType", "cost", "price", "markup", "tierUnits", + "rateScheduleItemId", "itemId", "costResourceId", "effectiveCostId", "laborUnitId", + "resourceComposition", "sourceEvidence", "evidenceBasis", "classification", + "costCode", "phaseId", +] as const; + +/** + * Did this call arrive carrying nothing but its two required fields? + * + * `worksheetId` and `entityName` are the only parameters without a default, so + * a tool call cut off partway through serialization lands here with exactly + * those two and everything else defaulted. That used to be reported as "Line + * evidence basis is required", which is technically true and diagnostically + * useless: it sent the agent off rewriting evidenceBasis 39 times when the real + * problem was that its arguments never arrived intact. + */ +function looksLikeTruncatedItemPayload(input: Record): boolean { + const carriesPayload = WORKSHEET_ITEM_PAYLOAD_FIELDS.some((key) => { + const value = input[key]; + if (value === undefined || value === null || value === "") return false; + if (typeof value === "object" && Object.keys(value).length === 0) return false; + return true; + }); + if (carriesPayload) return false; + return !String(input.description ?? "").trim() && !String(input.sourceNotes ?? "").trim(); +} + +const TRUNCATED_ITEM_PAYLOAD_MESSAGE = [ + "This call arrived with only worksheetId and entityName — no category, quantity basis, cost/rate, sourceNotes, or evidenceBasis came through.", + "That normally means the tool call was cut off while being written, not that the row is missing evidence. Do not retry the same way; the arguments will be cut off again.", + "Re-send as one row per call with a smaller payload: categoryId + quantity + uom, then either cost/price (freeform categories) or rateScheduleItemId + tierUnits (rate categories), plus sourceNotes and evidenceBasis.", + "For Labour, Equipment, Rental Equipment, and General Conditions rows prefer createRateScheduleWorksheetItem — it takes a much smaller payload for the same result.", +].join(" "); + +/** + * Examples printed in gate rejections. These are real minted id prefixes — + * the old hint said "doc-", which nothing in the system produces, so an agent + * following it literally could never satisfy the gate. + */ +const STRUCTURED_SOURCE_REF_HINT = "plain strings like doc_, lu-, ds-, kb-, lis_, rsi-, ecost-, a URL, or 'File.pdf p.12'"; + function looksLikeStructuredSourceRef(ref: unknown): boolean { if (typeof ref !== "string") return false; const value = ref.trim(); if (value.length < 4) return false; // Accept "doc::", "dataset::", "book::", "lu:", "vendor:", "uri:..." - if (/^(doc|document|dataset|ds|book|kb|knowledge|lu|labor|vendor|invoice|quote|catalog|cat|costres|effcost|sku|standard|spec|page|sheet|cell|row|atlas|claim)[-:]/i.test(value)) return true; - // Accept hyphenated DB ids like ds-, lu-, doc-, etc. - if (/^[a-z]{2,8}-[a-z0-9]{6,}/i.test(value)) return true; + if (/^(doc|document|dataset|ds|book|kb|knowledge|lu|labor|vendor|invoice|quote|catalog|cat|costres|effcost|sku|standard|spec|page|sheet|cell|row|atlas|claim)[-:_]/i.test(value)) return true; + // Accept DB ids like ds-, lu-, rsi-, ecost- — and the + // underscore-separated ones. SourceDocument mints `doc_` and + // LineItemSearchDocument mints `lis_`, so a hyphen-only rule silently + // scored a cited source document as zero structured refs: the row was then + // rejected for "needs structured cite", the agent re-cited the same real + // document id, and the loop repeated. Those two are the ids an agent is most + // likely to have for a material row, which is where this bit hardest. + if (/^[a-z]{2,8}[-_][a-z0-9]{6,}/i.test(value)) return true; // Accept URIs if (/^https?:\/\//i.test(value)) return true; // Accept document filename + page/section "Foo.pdf p.12" or "Foo.xlsx Sheet 'x' row 4" @@ -904,7 +954,7 @@ function validateLineEvidenceBasisForPricing(ws: any, input: { const hasLaborUnit = !!input.laborUnitId; const hasStructuredRef = structuredRefs > 0; if (!hasLaborUnit && !hasStructuredRef && !hasAssumptionIds) { - return "Labour row needs laborUnitId, evidenceBasis.pricing.sourceRefs with structured cite (ds-/lu-/doc-/kb-), or assumptionIds."; + return `Labour row needs laborUnitId, evidenceBasis.pricing.sourceRefs with a structured cite (${STRUCTURED_SOURCE_REF_HINT}), or assumptionIds.`; } } @@ -915,7 +965,7 @@ function validateLineEvidenceBasisForPricing(ws: any, input: { const hasStructuredRef = structuredRefs > 0; const hasComposition = compositionCount > 0; if (!hasStructuredLink && !hasStructuredRef && !hasAssumptionIds && !hasComposition) { - return "Material/Sub/Equip/Allowance row needs costResourceId, effectiveCostId, or itemId; or evidenceBasis.pricing.sourceRefs with structured cite; or assumptionIds; or resourceComposition.resources."; + return `Material/Sub/Equip/Allowance row needs costResourceId, effectiveCostId, or itemId; or evidenceBasis.pricing.sourceRefs with a structured cite (${STRUCTURED_SOURCE_REF_HINT}); or assumptionIds; or resourceComposition.resources.`; } // #3: Composite (LS / high-value) Material/Sub rows need component-level evidence @@ -923,7 +973,7 @@ function validateLineEvidenceBasisForPricing(ws: any, input: { if ((isLumpSum || rowDollar >= compositeMaterialThreshold()) && !hasStructuredLink) { const hasComponentEvidence = compositionCount >= 2 || structuredRefs >= 2; if (!hasComponentEvidence) { - return `Composite LS / >=$${compositeMaterialThreshold().toLocaleString()} row needs costResourceId/effectiveCostId/itemId, or 2+ structured sourceRefs, or 2+ resourceComposition.resources.`; + return `Composite LS / >=$${compositeMaterialThreshold().toLocaleString()} row needs costResourceId/effectiveCostId/itemId, or 2+ structured sourceRefs (${STRUCTURED_SOURCE_REF_HINT}), or 2+ resourceComposition.resources.`; } } } @@ -1782,19 +1832,19 @@ function worksheetTreeSummary(ws: any) { type: z.enum(LINE_EVIDENCE_BASIS_TYPES).describe("Source class that justifies the row quantity, labour hours, duration, or count."), drawingClaimIds: z.array(z.string()).default([]).describe("Required when quantity.type is drawing_quantity, visual_takeoff, drawing_table, or drawing_note."), quantityDriver: z.string().optional().describe("Formula or driver behind quantity/hours/duration."), - sourceRefs: z.array(z.string()).default([]), + sourceRefs: sourceRefArray(), assumptionIds: z.array(z.string()).default([]), rationale: z.string().optional(), }).passthrough().optional(), pricing: z.object({ type: z.enum(LINE_EVIDENCE_BASIS_TYPES).describe("Source class that justifies unit cost, rate, productivity, markup basis, or allowance value."), - sourceRefs: z.array(z.string()).default([]), + sourceRefs: sourceRefArray(), assumptionIds: z.array(z.string()).default([]), rationale: z.string().optional(), }).passthrough().optional(), quantityDriver: z.string().optional().describe("Short explanation of what drives quantity, hours, duration, or allowance."), drawingClaimIds: z.array(z.string()).default([]).describe("Legacy location for drawing quantity claim IDs. Prefer evidenceBasis.quantity.drawingClaimIds."), - sourceRefs: z.array(z.string()).default([]).describe("Document, quote, manual, library, web, schedule, or model refs supporting non-drawing rows."), + sourceRefs: sourceRefArray("Document, quote, manual, library, web, schedule, or model refs supporting non-drawing rows."), assumptionIds: z.array(z.string()).default([]).describe("Saved assumption IDs when the row is assumption-backed."), rationale: z.string().optional().describe("Why this source class is appropriate and how it supports the line."), }).passthrough().optional().describe("Line-level evidence contract. Required when drawings exist. Use quantity/pricing axes when quantity evidence and price/rate evidence differ."), @@ -1806,6 +1856,9 @@ function worksheetTreeSummary(ws: any) { ), }, async (input) => { + if (looksLikeTruncatedItemPayload(input as Record)) { + return { content: [{ type: "text" as const, text: TRUNCATED_ITEM_PAYLOAD_MESSAGE }] }; + } const wsForGate = await getWs(); const targetWorksheet = asArray(wsForGate.worksheets).map(asRecord).find((worksheet) => String(worksheet.id ?? "") === input.worksheetId); const gateError = await checkGate("createWorksheetItem", [ @@ -2021,19 +2074,19 @@ function worksheetTreeSummary(ws: any) { type: z.enum(LINE_EVIDENCE_BASIS_TYPES), drawingClaimIds: z.array(z.string()).default([]), quantityDriver: z.string().optional(), - sourceRefs: z.array(z.string()).default([]), + sourceRefs: sourceRefArray(), assumptionIds: z.array(z.string()).default([]), rationale: z.string().optional(), }).passthrough().optional(), pricing: z.object({ type: z.enum(LINE_EVIDENCE_BASIS_TYPES), - sourceRefs: z.array(z.string()).default([]), + sourceRefs: sourceRefArray(), assumptionIds: z.array(z.string()).default([]), rationale: z.string().optional(), }).passthrough().optional(), quantityDriver: z.string().optional(), drawingClaimIds: z.array(z.string()).default([]), - sourceRefs: z.array(z.string()).default([]), + sourceRefs: sourceRefArray(), assumptionIds: z.array(z.string()).default([]), rationale: z.string().optional(), }).passthrough().describe("Line-level evidence contract. Use quantity/pricing axes."), diff --git a/packages/mcp-server/src/tools/resource-tools.ts b/packages/mcp-server/src/tools/resource-tools.ts index e9406a64..4d6df65b 100644 --- a/packages/mcp-server/src/tools/resource-tools.ts +++ b/packages/mcp-server/src/tools/resource-tools.ts @@ -1,5 +1,6 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; +import { sourceRefArray } from "./source-refs.js"; import { apiGet, apiPost, getProjectId, projectPath } from "../api-client.js"; const sourceTypeSchema = z.enum([ @@ -1214,19 +1215,19 @@ export function registerResourceTools(server: McpServer) { type: lineEvidenceBasisTypeSchema.describe("Source class that justifies the row quantity, hours, duration, or count."), drawingClaimIds: z.array(z.string()).default([]), quantityDriver: z.string().optional(), - sourceRefs: z.array(z.string()).default([]), + sourceRefs: sourceRefArray(), assumptionIds: z.array(z.string()).default([]), rationale: z.string().optional(), }).passthrough().optional(), pricing: z.object({ type: lineEvidenceBasisTypeSchema.describe("Source class that justifies unit cost, rate, productivity, or allowance value."), - sourceRefs: z.array(z.string()).default([]), + sourceRefs: sourceRefArray(), assumptionIds: z.array(z.string()).default([]), rationale: z.string().optional(), }).passthrough().optional(), quantityDriver: z.string().optional(), drawingClaimIds: z.array(z.string()).default([]), - sourceRefs: z.array(z.string()).default([]), + sourceRefs: sourceRefArray(), assumptionIds: z.array(z.string()).default([]), rationale: z.string().optional(), }).passthrough().optional().describe("Line-level evidence contract. Required when drawings exist. Prefer quantity/pricing axes. Drawing/takeoff quantity basis needs quantity.drawingClaimIds; pricing can separately be rate_schedule, knowledge_labor, material_quote, vendor_quote, allowance, indirect, document_quantity, assumption, subcontract, equipment_rental, or mixed."), diff --git a/packages/mcp-server/src/tools/source-refs.test.ts b/packages/mcp-server/src/tools/source-refs.test.ts new file mode 100644 index 00000000..2e77e24a --- /dev/null +++ b/packages/mcp-server/src/tools/source-refs.test.ts @@ -0,0 +1,43 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { z } from "zod"; + +import { normalizeSourceRef, sourceRefArray } from "./source-refs.js"; + +const schema = z.object({ sourceRefs: sourceRefArray("refs") }); + +test("strings pass through, trimmed", () => { + assert.deepEqual( + schema.parse({ sourceRefs: ["doc_abc123", " lu-xyz789 "] }).sourceRefs, + ["doc_abc123", "lu-xyz789"], + ); +}); + +test("an omitted array defaults to empty", () => { + assert.deepEqual(schema.parse({}).sourceRefs, []); +}); + +test("object refs are accepted instead of failing the whole tool call", () => { + // This is what the agent actually sent, and what came back as + // "MCP error -32602: expected string, received object". + const parsed = schema.parse({ + sourceRefs: [ + { documentId: "doc_3b409f90", page: 4 }, + { type: "library", title: "Cable Tray Spec.pdf" }, + ], + }); + assert.deepEqual(parsed.sourceRefs, ["doc_3b409f90 p.4", "Cable Tray Spec.pdf"]); +}); + +test("an object with nothing identifiable is dropped, not stringified", () => { + const parsed = schema.parse({ sourceRefs: [{ noExactMatch: true }, "kb-1a2b3c"] }); + assert.deepEqual(parsed.sourceRefs, ["kb-1a2b3c"], "no '[object Object]' entries"); +}); + +test("normalized object refs satisfy the structured-cite rule", () => { + // The point of accepting objects: the result has to actually pass the gate + // that rejected the call in the first place. + const looksStructured = (value: string) => /^[a-z]{2,8}[-_][a-z0-9]{6,}/i.test(value); + assert.ok(looksStructured(normalizeSourceRef({ documentId: "doc_3b409f90a71c" }))); + assert.ok(looksStructured(normalizeSourceRef({ id: "lis_0001ce9912de0cef" }))); +}); diff --git a/packages/mcp-server/src/tools/source-refs.ts b/packages/mcp-server/src/tools/source-refs.ts new file mode 100644 index 00000000..92c50b9b --- /dev/null +++ b/packages/mcp-server/src/tools/source-refs.ts @@ -0,0 +1,68 @@ +/** + * `sourceRefs` accept either a plain string cite or an object, normalizing both + * to the string form the evidence gates read. + * + * The gates ask for "structured sourceRefs", and an agent reasonably reads that + * as "send me a structure". The array was typed `z.array(z.string())`, so an + * object came back as a raw `MCP error -32602: expected string, received + * object` with no hint about the intended shape -- and the agent alternated + * between object and string forms, each rejected by a different layer for a + * different reason. Accepting both and normalizing removes the guess: the + * "structured" the gates want is a resolvable id inside the string, not a + * nested object. + */ + +import { z } from "zod"; + +/** Keys an agent plausibly puts the resolvable id under. */ +const ID_KEYS = [ + "id", "ref", "sourceRef", "documentId", "docId", "sourceId", "itemId", + "claimId", "effectiveCostId", "costResourceId", "laborUnitId", "labourUnitId", + "datasetId", "bookId", "rateScheduleItemId", "url", "uri", +] as const; + +/** Keys that name the source when no id is present. */ +const LABEL_KEYS = ["title", "name", "document", "file", "fileName", "label", "query"] as const; + +/** Keys that locate the cite within the source. */ +const LOCATOR_KEYS = ["page", "pageNumber", "pageNum", "sheet", "row", "cell", "section"] as const; + +function firstNonEmpty(record: Record, keys: readonly string[]): string { + for (const key of keys) { + const value = record[key]; + if (typeof value === "string" && value.trim()) return value.trim(); + if (typeof value === "number" && Number.isFinite(value)) return String(value); + } + return ""; +} + +/** Flatten one source ref to its string form. Returns "" for unusable input. */ +export function normalizeSourceRef(value: unknown): string { + if (typeof value === "string") return value.trim(); + if (typeof value === "number" && Number.isFinite(value)) return String(value); + if (!value || typeof value !== "object" || Array.isArray(value)) return ""; + + const record = value as Record; + const head = firstNonEmpty(record, ID_KEYS) || firstNonEmpty(record, LABEL_KEYS); + if (!head) return ""; + + const locator = firstNonEmpty(record, LOCATOR_KEYS); + return locator ? `${head} p.${locator}` : head; +} + +/** + * An array of source refs. Accepts strings or objects on the way in, and always + * produces strings. Drops entries that carry no identifiable source rather than + * storing `"[object Object]"`. + */ +export function sourceRefArray(description?: string) { + const schema = z + .array( + z + .union([z.string(), z.number(), z.record(z.unknown())]) + .transform(normalizeSourceRef), + ) + .transform((refs) => refs.filter(Boolean)) + .default([]); + return description ? schema.describe(description) : schema; +}