diff --git a/CHANGELOG.md b/CHANGELOG.md index f9102ee9..1647a5c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,19 @@ stays a decision somebody makes rather than a side effect of a vendor's bad afte Nothing changes for a connector whose grants all match its tool list, which is the normal case — the section is not drawn and no row is written. +### Opening a new chat no longer logs a server error + +A thread id is minted before the thread exists — the platform creates it on the first run — so reading +history on a brand-new conversation asks about a thread nothing has heard of yet. The platform answered +404, the runtime reported that as `500 Failed to fetch thread messages`, and every new chat left one in +the log with a stack trace behind it. Nothing was visibly broken, because the browser only reads a +history it got a 200 for; what was missing was any way to tell a thread that does not exist yet from a +history store that is down. + +A thread the platform does not have now reads as having no messages. A 404 and only a 404: a 500 stays +a 500, because an outage answered with an empty history would tell the browser the conversation is gone +and invite somebody to start it over. + ## 0.0.4 ### A click citing a ref this deployment cannot resolve is refused diff --git a/server/src/copilot.ts b/server/src/copilot.ts index c8f1c6e3..3527c69a 100644 --- a/server/src/copilot.ts +++ b/server/src/copilot.ts @@ -839,6 +839,75 @@ export function createRequestAgents( * reachable on the next request. Resolving once at boot would mean every new Bot needed a restart, * which is not a property you can explain to somebody who just created one. */ +/** + * Whether this failure means "the platform has never heard of that thread". + * + * A thread id is minted before the thread exists — the platform creates it on the first run — so + * reading history on a brand-new conversation is the normal opening move, and the platform answers + * `THREAD_NOT_FOUND` with a 404. The runtime's own handler catches everything and returns a bare 500, + * so every new chat produced one, with a stack trace behind it. + * + * Matched on the shape rather than with `instanceof`. The class is `PlatformRequestError` and it + * carries `.status` for exactly this — its own documentation gives `error.status === 404` as the + * example — but it is not re-exported from `@copilotkit/runtime/v2`, and the package's `exports` map + * offers no subpath that reaches it, so there is no type to test against. The name is set by the + * constructor and the status is a number on the instance; both are checked, so an unrelated error + * carrying a `status` of 404 does not qualify. + * + * 404 ONLY, and nothing wider. A 500 from the platform means an outage or a bad key, and answering + * that with an empty history would tell the browser the conversation is gone and invite somebody to + * start it over. That is the failure this must not introduce while removing the noisy one. + */ +export function isMissingThread(error: unknown): boolean { + return ( + error instanceof Error && + error.name === "PlatformRequestError" && + (error as { status?: unknown }).status === 404 + ); +} + +/** + * Read a thread's history, treating a thread the platform does not know about as having none. + * + * Takes the read as a function rather than being folded into the class below, so the decision can be + * exercised against a function that really throws. The previous attempt at this fix + * (#71) was tested by re-implementing its middleware inside the test file, which passes with the real + * code deleted; this is the actual code path in both places. + */ +export async function historyOrEmpty( + read: () => Promise, + whenMissing: T, +): Promise { + try { + return await read(); + } catch (error) { + if (isMissingThread(error)) return whenMissing; + throw error; + } +} + +/** + * The platform client, with one answer corrected. + * + * A subclass rather than a wrapper. The runtime is handed this object and calls many methods on it, + * and the base class keeps its state in `#private` fields — which a `Proxy` cannot forward, because a + * method invoked with the proxy as `this` cannot reach them. Extending keeps every other method + * exactly as it was, on the instance that owns those fields. + * + * `getThreadMessages` is the only override. `handleGetThreadMessages` in the runtime calls it and + * returns `Response.json` of whatever comes back, so an empty history here is the `{ messages: [] }` + * the browser expects and a 200 instead of a 500. + */ +class IntelligenceKnowingANewThread extends CopilotKitIntelligence { + override getThreadMessages( + params: Parameters[0], + ) { + return historyOrEmpty(() => super.getThreadMessages(params), { + messages: [], + }); + } +} + export function mountCopilotRuntime( config: DeploymentConfig, model: RuntimeModel, @@ -869,7 +938,9 @@ export function mountCopilotRuntime( // returns, so omitting it puts every person in the deployment in the same thread space and one // person's conversations become another's. identifyUser, - intelligence: new CopilotKitIntelligence({ + // The subclass, not the base: a thread nobody has run yet reads as empty rather than as a 500. + // See IntelligenceKnowingANewThread. + intelligence: new IntelligenceKnowingANewThread({ apiUrl: intelligence.apiUrl, wsUrl: intelligence.gatewayWsUrl, apiKey: intelligence.apiKey, diff --git a/server/tests/thread-history.test.ts b/server/tests/thread-history.test.ts new file mode 100644 index 00000000..eb7e70f7 --- /dev/null +++ b/server/tests/thread-history.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, test } from "bun:test"; +import { historyOrEmpty, isMissingThread } from "../src/copilot"; + +/** + * Reading history on a thread the platform has never seen. + * + * A thread id is minted before the thread exists, so this is the opening move of every new + * conversation and it was answering 500. The decision is the whole of the change, and it is tested + * here against the real functions rather than a copy of them: the previous attempt (#71) tested a + * re-implementation of its own middleware, which passes with the shipped code deleted. + */ + +/** + * A `PlatformRequestError` as the platform client constructs one. + * + * Built by hand because the class is not re-exported from `@copilotkit/runtime/v2` and the package's + * `exports` map reaches nothing that holds it — which is also why the code under test matches on the + * shape. The constructor sets the message, then `status`, then `name`, so this is the same object. + */ +function platformError(status: number): Error { + const error = new Error(`Intelligence platform error ${status}`); + error.name = "PlatformRequestError"; + (error as Error & { status: number }).status = status; + return error; +} + +describe("recognising a thread the platform does not have", () => { + test("a 404 from the platform is a missing thread", () => { + expect(isMissingThread(platformError(404))).toBe(true); + }); + + test("a 500 from the platform is not", () => { + // The one that matters. An outage answered with an empty history tells the browser the + // conversation is gone and invites somebody to start it over. + expect(isMissingThread(platformError(500))).toBe(false); + }); + + test("a 403 from the platform is not", () => { + // A bad key is not an absent thread, and reading it as one would hide a misconfiguration behind + // a conversation that looks new. + expect(isMissingThread(platformError(403))).toBe(false); + }); + + test("an unrelated error carrying a 404 is not", () => { + /* + * Both halves are checked, so something else with a `status` of 404 on it — a fetch wrapper, a + * vendor SDK — does not get a thread's history replaced with nothing. + */ + const other = new Error("some other failure"); + (other as Error & { status: number }).status = 404; + expect(isMissingThread(other)).toBe(false); + }); + + test("a plain object shaped like one is not", () => { + expect(isMissingThread({ name: "PlatformRequestError", status: 404 })).toBe( + false, + ); + }); + + test("nothing thrown at all is not", () => { + expect(isMissingThread(undefined)).toBe(false); + expect(isMissingThread(null)).toBe(false); + }); +}); + +describe("reading a history that may not exist yet", () => { + const empty = { messages: [] as string[] }; + + test("a thread with history returns it", async () => { + const history = { messages: ["hello"] }; + expect(await historyOrEmpty(async () => history, empty)).toBe(history); + }); + + test("a thread the platform does not have reads as empty", async () => { + expect( + await historyOrEmpty(async () => { + throw platformError(404); + }, empty), + ).toEqual({ messages: [] }); + }); + + test("a platform outage still throws", async () => { + // Not swallowed, not turned into an empty conversation. This is the assertion that would fail if + // the branch were widened to any failure. + await expect( + historyOrEmpty(async () => { + throw platformError(500); + }, empty), + ).rejects.toThrow("Intelligence platform error 500"); + }); + + test("an error that is not the platform's still throws", async () => { + await expect( + historyOrEmpty(async () => { + throw new Error("the network went away"); + }, empty), + ).rejects.toThrow("the network went away"); + }); +});