From a12b3e15be7862785ae26fad36dd9d2dd25fb062 Mon Sep 17 00:00:00 2001 From: Yash Datta Date: Wed, 29 Jul 2026 22:29:23 +0800 Subject: [PATCH] fix: make multi-writer artifacts readable, and stop destroying codex error causes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found by the first live end-to-end run of a collaborative session with real models — a mixed fleet (claude opus 4.8 / sonnet 5, codex gpt-5.6-sol) where an orchestrator drove a cross-vendor panel. Neither was reachable from the test suite as written. ## blackboard_read could not address a multi-writer artifact `findings` is a MULTI_WRITER kind: each reviewer writes its own slot. So `blackboard_index` correctly reported `findings` in slot `review` — and `blackboard_read` had no `slot` parameter, read the singleton slot, and answered "No findings has been written on this goal yet." Live consequence: the orchestrator saw the artifact in the index, tried to read it six times, was told it did not exist, concluded "Blackboard still unreadable on a third attempt", and abandoned the board — routing around the entire typed-handoff mechanism the feature exists for. The index advertised something the reader could not fetch. `read` now takes an optional `slot`, and a slotless read that finds nothing POINTS AT the slots that do exist instead of claiming the artifact is absent. The asymmetry with `write` is deliberate and unchanged: §4 gives `write` no slot so a writer cannot name a peer's and overwrite it. The service always supported `read(kind, slot)`; only the tool schema was missing it. ### A test was enforcing the bug `blackboard-mcp.test.ts` asserted that NO tool accepts a `slot` — stricter than the design, which forbids it on `write` only. That over-assertion is why reads were slotless, and it would have rejected this fix. It now states the invariant per tool: no tool takes a goal or session id (scope rides the token), `write` never takes a slot, `read` does. ## codex errors arrived as "[object Object]" The `error` notification carries an object, and the handler did `(params.message ?? params.error) as string` then `String(...)` — so every codex failure reported the literal `[object Object]`. The cause was destroyed at the one place whose job was to report it. Live consequence: a codex role-child failed twice with `Error: [object Object]`. Nothing in the transcript, the audit log, or the daemon log said why, and the orchestrator could only report "identical bare `worker turn ended in error`, no diagnostic detail". `describeCodexError` now handles a direct string, a bare string error, a JSON-RPC-shaped `{code, message}`, and falls back to bounded JSON — chosen to tolerate shape rather than assert one, because a wrong assumption here costs the whole diagnostic, which is exactly what happened. With it, the same run reported: codex: {"type":"error","status":400,"error":{"type":"invalid_request_error", "message":"The 'gpt-5.6-sol' model requires a newer version of Codex. Please upgrade to the latest app or CLI and try again."}} The codex failure itself is environmental — the installed CLI (client 0.145.0) predates the model, which its own cache still advertises. Not a codeoid bug, and now diagnosable in one read instead of not at all. ## Verification Daemon suite 2172 to 2191. Both fixes mutation-checked: removing `slot` from the read schema fails the multi-writer read test, and reverting the error extraction fails three of the five codex cases. The codex error path had NO test before this — which is how `[object Object]` shipped. It has five now, including one that asserts the string `[object Object]` never appears for any input shape. ## What the live run confirmed working Not part of this fix, but it is the first time any of it ran with real models, so worth recording: - an orchestrator on opus chose `fleet_panel` itself and fanned one brief to two children on different vendors - the R3 gate fired per dispatch with the cost roll-up attached, and the roll-up accumulated across dispatches ($0 to $0.68 to $1.39) - the barrier behaved exactly as designed, twice: `group_waiting done=1/2` then `group_joined members=2 failed=1` — it absorbed the first completion, did not fire early, and joined on all-terminal with the failed member reported rather than hanging on it - after the slot fix, the reviewer's `findings` artifact read back through the orchestrator's mount, closing the write-then-read handoff --- src/daemon/blackboard/mcp-http.ts | 39 +++++++++++++++--- src/daemon/providers/codex/index.ts | 42 ++++++++++++++++++- src/tests/blackboard-mcp.test.ts | 64 +++++++++++++++++++++++++---- src/tests/provider-codex.test.ts | 51 ++++++++++++++++++++++- 4 files changed, 181 insertions(+), 15 deletions(-) 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); + } + }); +});