From 91436d60db7600731f50832b906879b2e70b4d37 Mon Sep 17 00:00:00 2001 From: Braedon Saunders Date: Thu, 10 Sep 2026 14:14:32 -0400 Subject: [PATCH 1/2] fix(agent): stop a stale reap timer from blinding the API to a live run A session that finishes schedules `sessions.delete(projectId)` five minutes later. If a follow-up run starts inside that window it takes the project's slot, and the previous run's timer then evicted the *new*, running session. Everything downstream reads that registry, so the API went blind to its own agent while the child kept working: - `resumeSession`'s "already running" guard consulted the empty map, so it silently passed and let a resume collide with the live agent. Codex refused the second writer -- "thread-store conflict: thread already has an active writer" -- and the resumed run died in ~300ms. - `/answer` concluded nobody was polling the askUser prompt and auto-resumed on top of the running agent, producing that same collision one second after the answer had already been delivered. - Each stillborn resume became the newest AiRun and therefore `latestRun`, so the status endpoint reported the whole project as "failed" while the estimate was still being built. Every Resume click appended another and re-applied the mask. On CB-260910-0001 the run survived all of it and finished normally 32 minutes later, having never stopped -- only the UI ever thought otherwise. Three changes: - `reapSession` deletes only if the slot still holds that same session. Extracted as a pure function over an injected Map so the takeover case is testable without spawning a process. - `probeLiveAgent` decides liveness by probing: registry first, then `.bidwright/session.json` + `kill(pid, 0)`. `persistSessionState` now records `ownerPid`, and a recorded pid is trusted only when this API process spawned it -- a pid from an earlier process, or restored with a workspace snapshot from another host, is not ours and pids get reused. Both the resume guard and `/answer` use it. - `selectLatestRun` skips a tail of stillborn runs (failed, and never emitted `status: running`) while the last run that did start is still open. It walks the whole tail, since each retry added one. When nothing else is running the newest run still wins, so genuine start-up failures surface. `selectLatestRun` lives in its own module because anything defined in cli-routes.ts can only be tested by grepping its source text -- which is how the resume guard stayed inert for three weeks while `resume-session-guard.test.ts` asserted that it contained `session.status === "running"`. That file now checks route wiring only; the rule itself is covered behaviourally in cli-runtime-liveness.test.ts. Co-Authored-By: Claude Opus 5 (1M context) --- apps/api/src/answer-resumes-run.test.ts | 15 +- apps/api/src/resume-session-guard.test.ts | 31 ++-- apps/api/src/routes/cli-routes.ts | 15 +- .../src/services/cli-run-selection.test.ts | 86 ++++++++++ apps/api/src/services/cli-run-selection.ts | 63 ++++++++ .../src/services/cli-runtime-liveness.test.ts | 150 ++++++++++++++++++ apps/api/src/services/cli-runtime.ts | 135 ++++++++++++++-- 7 files changed, 463 insertions(+), 32 deletions(-) create mode 100644 apps/api/src/services/cli-run-selection.test.ts create mode 100644 apps/api/src/services/cli-run-selection.ts create mode 100644 apps/api/src/services/cli-runtime-liveness.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.`, From 2e4fdf63116ce60f669e77af3be094434cd636d8 Mon Sep 17 00:00:00 2001 From: Braedon Saunders Date: Thu, 10 Sep 2026 14:14:56 -0400 Subject: [PATCH 2/2] fix(mcp): stop the evidence gate rejecting the source ids it asks for On the Birla electrical quote 60 of 65 createWorksheetItem calls were rejected. Three separate causes: 1. `looksLikeStructuredSourceRef` accepted only `-` or `:` after the prefix, but SourceDocument mints `doc_` and LineItemSearchDocument mints `lis_`. Both scored zero structured refs, so a row citing a real source document was rejected for "needs structured cite" no matter how accurately it cited it -- and those are precisely the two ids an agent has for a material row. Every other minted id (lu-, ds-, kb-, rsi-, ecost-, rci-) passed, which is why this only bit material and composite rows. The rejection text made it worse by naming `doc-` as the format to use, a prefix nothing in the system produces. Gate messages now interpolate STRUCTURED_SOURCE_REF_HINT, which lists real prefixes. 2. `sourceRefs` was `z.array(z.string())`, so when the agent read "structured sourceRefs" as "send a structure" it got back a bare `MCP error -32602: expected string, received object` with no hint at the intended shape. It then alternated between object and string forms, each rejected by a different layer for a different reason. The new `sourceRefArray()` accepts either and normalizes objects to the string form the gates read (`{documentId, page}` -> `doc_3b409f90 p.4`), dropping entries with nothing identifiable rather than storing "[object Object]". Applied to all ten sourceRefs schemas, which also fixes the saveEstimateScopeGraph scopeItems[].sourceRefs rejection. 3. 39 calls arrived carrying only `worksheetId` and `entityName` -- the two parameters without a default -- because the tool call was cut off while being serialized. The server answered "Line evidence basis is required", which is true and diagnostically useless: it sent the agent rewriting evidenceBasis 39 times over a payload that never arrived intact. It worked this out itself ("I'm repeatedly truncating my own tool call") and eventually delegated the rows to a sub-agent. That shape is now detected and reported as a truncated call, telling the agent not to retry the same way and pointing at createRateScheduleWorksheetItem, whose smaller payload succeeded 13 of 16 times in the same run against 5 of 65. Tests replay the recorded prod payloads and are registered in CI, along with the session-liveness tests from the previous commit. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci-cd.yml | 4 + .../mcp-server/src/tools/estimate-tools.ts | 3 +- .../src/tools/evidence-gate-contract.test.ts | 146 ++++++++++++++++++ packages/mcp-server/src/tools/quote-tools.ts | 77 +++++++-- .../mcp-server/src/tools/resource-tools.ts | 7 +- .../mcp-server/src/tools/source-refs.test.ts | 43 ++++++ packages/mcp-server/src/tools/source-refs.ts | 68 ++++++++ 7 files changed, 332 insertions(+), 16 deletions(-) create mode 100644 packages/mcp-server/src/tools/evidence-gate-contract.test.ts create mode 100644 packages/mcp-server/src/tools/source-refs.test.ts create mode 100644 packages/mcp-server/src/tools/source-refs.ts 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/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; +}