diff --git a/src/daemon/blackboard/mcp-http.ts b/src/daemon/blackboard/mcp-http.ts index 3d8d0d4..c9f6999 100644 --- a/src/daemon/blackboard/mcp-http.ts +++ b/src/daemon/blackboard/mcp-http.ts @@ -102,19 +102,48 @@ const TOOLS: ToolDef[] = [ { name: "blackboard_read", description: - "Read the latest version of one artifact. Denied if your role's read scope doesn't include it.", + "Read the latest version of one artifact. Pass `slot` for a multi-writer kind — blackboard_index shows each entry's slot. Denied if your role's read scope doesn't include the kind.", jsonSchema: { type: "object", - properties: { kind: { type: "string", description: KIND_DESC } }, + properties: { + kind: { type: "string", description: KIND_DESC }, + // `read` takes a slot; `write` deliberately does NOT (§4 — a writer must + // not be able to name a peer's slot and overwrite it). Without this, + // every multi-writer artifact was unreachable: the index advertised + // `findings` in slot "review", and a slotless read looked in the + // singleton slot and reported it did not exist. Observed live — an + // orchestrator retried six times and concluded the board was broken. + slot: { + type: "string", + description: + "Writer slot, exactly as blackboard_index reports it (e.g. \"review\" or \"review#2\"). Omit for a single-writer kind.", + }, + }, required: ["kind"], additionalProperties: false, }, run: (args, bb) => { - const r = bb.read(str(args.kind)); + const kind = str(args.kind); + const slot = args.slot === undefined ? null : str(args.slot); + const r = bb.read(kind, slot); if (!r.ok) throw new Error(r.error); - if (!r.value) return `No "${str(args.kind)}" has been written on this goal yet.`; + if (!r.value) { + // Point at the slots that DO exist rather than a bare "not written": + // the caller usually asked for the right kind with the wrong slot, and + // the index already knows the answer. + const slots = bb + .index() + .filter((e) => e.kind === kind) + .map((e) => e.slot) + .filter((sl): sl is string => sl !== null); + if (slot === null && slots.length > 0) { + return `"${kind}" has no single-writer entry, but ${slots.length} writer slot(s) exist: ${slots.join(", ")}. Re-read with \`slot\`, or use blackboard_read_all to get every writer's entry at once.`; + } + return `No "${kind}"${slot ? ` in slot "${slot}"` : ""} has been written on this goal yet.`; + } const a = r.value; - return `${a.kind} v${a.version} (by ${a.authorRole ?? a.authorSub}):\n\n${a.content}`; + const where = a.slot ? ` slot ${a.slot}` : ""; + return `${a.kind}${where} v${a.version} (by ${a.authorRole ?? a.authorSub}):\n\n${a.content}`; }, }, { diff --git a/src/daemon/providers/codex/index.ts b/src/daemon/providers/codex/index.ts index 5d20718..3cc7697 100644 --- a/src/daemon/providers/codex/index.ts +++ b/src/daemon/providers/codex/index.ts @@ -199,6 +199,40 @@ function sandboxPolicyWire(mode: SandboxMode, workdir: string): Record): string { + const direct = params.message; + if (typeof direct === "string" && direct.length > 0) return direct; + + const err = params.error; + if (typeof err === "string" && err.length > 0) return err; + if (err && typeof err === "object") { + const o = err as { message?: unknown; code?: unknown; data?: unknown }; + const msg = typeof o.message === "string" ? o.message : undefined; + const code = o.code !== undefined ? ` (code ${String(o.code)})` : ""; + if (msg) return `${msg}${code}`; + try { + return JSON.stringify(err).slice(0, 500); + } catch { + return "unserializable error payload"; + } + } + try { + const dump = JSON.stringify(params); + return dump && dump !== "{}" ? dump.slice(0, 500) : "unspecified codex error"; + } catch { + return "unspecified codex error"; + } +} + export class CodexProvider implements SessionProvider { readonly id = "codex"; readonly displayName = "Codex (OpenAI)"; @@ -652,8 +686,12 @@ export class CodexProvider implements SessionProvider { break; } case "error": { - const message = (params.message ?? params.error ?? "codex error") as string; - this.#push({ type: "error", message: String(message) }); + // `params.error` is an OBJECT on this wire ({code, message, data}), so + // the previous `as string` + String() turned every codex failure into + // the literal "[object Object]" — the cause destroyed at the one place + // it was supposed to be reported. Observed live: a role-child failed + // twice with `Error: [object Object]` and nothing anywhere said why. + this.#push({ type: "error", message: `codex: ${describeCodexError(params)}` }); break; } default: diff --git a/src/tests/blackboard-mcp.test.ts b/src/tests/blackboard-mcp.test.ts index bc267d6..c767468 100644 --- a/src/tests/blackboard-mcp.test.ts +++ b/src/tests/blackboard-mcp.test.ts @@ -170,19 +170,69 @@ describe("the token carries the role's scope", () => { // No tool takes a goal id, so a child cannot address another goal even by // guessing one — the mount is the boundary. - test("no tool accepts a goal id, a session id, or a slot", async () => { + // + // This test used to forbid `slot` on EVERY tool, which was stricter than the + // design and made the feature wrong: §4 forbids a slot on `write` (so a writer + // cannot name a peer's slot and overwrite it), but a READER must be able to + // address the slot the index just showed it. With reads slotless, every + // multi-writer artifact was unreachable — observed live, an orchestrator read + // `findings` six times, was told it did not exist, and gave up on the board. + // The invariant is now stated per-tool instead of as one blanket rule. + test("no tool accepts a goal id or a session id", async () => { const token = mcp.mint(bb.forRole(GOAL, ident("review"))); const list = await rpc(token, "tools/list"); // Assert on PARAMETER names, not the serialized blob — the descriptions // legitimately mention "this goal" in prose. - const params = (list.body.result.tools as Array<{ inputSchema: { properties: object } }>) - .flatMap((t) => Object.keys(t.inputSchema.properties ?? {})) - .sort(); - // The full parameter vocabulary of the surface is exactly two names. - expect([...new Set(params)]).toEqual(["content", "kind"]); - for (const forbidden of ["goal", "goalSessionId", "sessionId", "slot", "accountId"]) { + const tools = list.body.result.tools as Array<{ + name: string; + inputSchema: { properties?: object }; + }>; + const params = tools.flatMap((t) => Object.keys(t.inputSchema.properties ?? {})).sort(); + // Scope is carried by the TOKEN, never named by the caller. + for (const forbidden of ["goal", "goalSessionId", "sessionId", "accountId", "projectId"]) { expect(params).not.toContain(forbidden); } + // The whole parameter vocabulary, so a new knob cannot appear unnoticed. + expect([...new Set(params)].sort()).toEqual(["content", "kind", "slot"]); + }); + + test("`slot` is readable but never writable — a writer cannot name a peer's slot", async () => { + const token = mcp.mint(bb.forRole(GOAL, ident("review"))); + const list = await rpc(token, "tools/list"); + const byName = new Map( + (list.body.result.tools as Array<{ name: string; inputSchema: { properties?: object } }>).map( + (t) => [t.name, Object.keys(t.inputSchema.properties ?? {})], + ), + ); + // The asymmetry IS the security model (§4): the service picks a writer's + // slot from its own identity, so exposing one on write would hand a + // reviewer the ability to overwrite another reviewer's findings. + expect(byName.get("blackboard_write")).not.toContain("slot"); + expect(byName.get("blackboard_read")).toContain("slot"); + }); + + test("a reader can fetch a multi-writer artifact by the slot the index reports", async () => { + // The end-to-end shape of the live failure: two reviewers write `findings`, + // the index reports their slots, and a read must be able to use them. + bb.forRole(GOAL, ident("review", 1)).write("findings", "FIRST OPINION"); + bb.forRole(GOAL, ident("review", 2)).write("findings", "SECOND OPINION"); + const token = mcp.mint(bb.forRole(GOAL, ident("orchestrator"))); + + const idx = await call(token, "blackboard_index", {}); + expect(textOf(idx.body)).toContain("review#2"); + + const first = await call(token, "blackboard_read", { kind: "findings", slot: "review" }); + expect(textOf(first.body)).toContain("FIRST OPINION"); + const second = await call(token, "blackboard_read", { kind: "findings", slot: "review#2" }); + expect(textOf(second.body)).toContain("SECOND OPINION"); + + // And a slotless read now POINTS AT the slots instead of claiming the + // artifact does not exist — the message that misled a live orchestrator. + const slotless = await call(token, "blackboard_read", { kind: "findings" }); + const text = textOf(slotless.body); + expect(text).toMatch(/writer slot\(s\) exist/); + expect(text).toContain("review#2"); + expect(text).not.toMatch(/has been written on this goal yet/); }); // Two goals, two tokens: neither can see the other's artifacts. diff --git a/src/tests/provider-codex.test.ts b/src/tests/provider-codex.test.ts index d76a3b9..2e55e21 100644 --- a/src/tests/provider-codex.test.ts +++ b/src/tests/provider-codex.test.ts @@ -27,7 +27,7 @@ import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:f import { tmpdir } from "node:os"; import { join } from "node:path"; import { CodexRpcProcess } from "../daemon/providers/codex/rpc.js"; -import { CodexProvider } from "../daemon/providers/codex/index.js"; +import { CodexProvider, describeCodexError } from "../daemon/providers/codex/index.js"; import { MemoryMcpHttp, MEMORY_MCP_SERVER_NAME } from "../daemon/memory/mcp-http.js"; import { McpRegistry } from "../daemon/mcp/registry.js"; import type { RawMcpServerConfig } from "../config.js"; @@ -764,3 +764,52 @@ describe("codex resolution + registry", () => { expect(compareNodeVersionsDesc("v18.1.0", "v18.1.0")).toBe(0); }); }); + +// ── error reporting ───────────────────────────────────────────────────────── + +// This exists because a live role-child failed twice with the literal +// `Error: [object Object]` and nothing anywhere said why. The `error` +// notification carries an OBJECT on this wire, and the handler cast it +// `as string` then String()'d it — destroying the cause at the one place whose +// entire job was to report it. A backend whose failures are unreadable is a +// backend nobody can debug. +describe("describeCodexError", () => { + it("extracts a JSON-RPC-style error object rather than stringifying it", () => { + const out = describeCodexError({ + error: { code: -32603, message: "model gpt-5.6-sol is not available to this account" }, + }); + expect(out).toContain("not available to this account"); + expect(out).toContain("-32603"); + expect(out).not.toContain("[object Object]"); + }); + + it("prefers a direct message when present", () => { + expect(describeCodexError({ message: "stream closed" })).toBe("stream closed"); + }); + + it("accepts a bare string error", () => { + expect(describeCodexError({ error: "rate limited" })).toBe("rate limited"); + }); + + it("falls back to bounded JSON for an unrecognised shape", () => { + // Tolerating shape beats asserting it: a wrong assumption here costs the + // whole diagnostic, which is exactly what happened. + const out = describeCodexError({ error: { unexpected: { nested: true } } }); + expect(out).toContain("unexpected"); + expect(out).not.toContain("[object Object]"); + }); + + it("never returns [object Object], whatever it is handed", () => { + for (const params of [ + {}, + { error: {} }, + { error: null }, + { message: 42 }, + { error: { message: null, code: 7 } }, + ] as Array>) { + const out = describeCodexError(params); + expect(out).not.toContain("[object Object]"); + expect(out.length).toBeGreaterThan(0); + } + }); +});