diff --git a/CHANGELOG.md b/CHANGELOG.md index 4df3bfd3f..52b0731b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A Bot's question to a person survives a route that fails + +Who "a person" is, is a seam a deployment fills in with its own on-call rota or duty desk. If that +route failed by throwing rather than by returning a refusal — a timeout, a 502, a name that does not +resolve — the error came straight back out of the tool, so the Bot's run ended with nothing said to +the person waiting, and no audit row recorded that the question had reached nobody. The Bot is now +told, in a sentence it can say, that nobody could be asked and that it must not claim otherwise, and +an `agent.escalation_failed` row goes down carrying what the route actually threw. Deployments using +the shipped in-conversation route are unaffected: it cannot fail. ### Resetting a Bot's computer while the Bot is acting signs it out On a deployment with one shared computer, which is what the published image and the Helm chart run by diff --git a/server/src/agents/escalation.ts b/server/src/agents/escalation.ts index 590071bcf..07fdf3976 100644 --- a/server/src/agents/escalation.ts +++ b/server/src/agents/escalation.ts @@ -121,14 +121,58 @@ export function escalationTool(options: { return "That was not put to anybody: say what you need a person to answer."; } - const outcome = await route({ - actorId: from.actorId, - botId: from.botId, - ...(from.threadId ? { threadId: from.threadId } : {}), - runId: from.runId, - question, - ...(parsed.data.why ? { why: parsed.data.why } : {}), - }); + /* + * A route that throws is a route that refused, and the run has to survive it. + * + * `handoff.ts` says the rule for both of these tools at the top of its module: every refusal is + * an answer, not an error, because the asking Bot is mid-run with a person waiting and a thrown + * error ends the run with nothing said. This tool competes with that one for the same decision + * and did not follow it — the throw came straight back out of `execute`, which is the failure + * that reads to the person as the Bot ignoring them, on the tool whose entire job is to stop + * ignoring them. + * + * It looks unreachable and is not. `askTheirOwnPerson` is a pure function that cannot throw, so + * nothing in this repo or its tests ever takes this path — but the module comment above says + * WHO "A PERSON" IS, IS A SEAM, and every route a company actually hands in is a duty desk, a + * rota, a queue: a network call that times out, 502s, or resolves DNS to nothing. The one route + * that cannot fail is the one that ships, so the guard was missing exactly where the + * documentation invites a deployment to go. + * + * Recorded as `agent.escalation_failed`, which is what the comment below already promises for + * an escalation that could not be delivered and previously only delivered for a route polite + * enough to return its refusal. + */ + /** What the route threw, if it did, for the row and never for the answer. */ + let thrown: string | undefined; + let outcome: { reached: string } | { refusal: string }; + try { + outcome = await route({ + actorId: from.actorId, + botId: from.botId, + ...(from.threadId ? { threadId: from.threadId } : {}), + runId: from.runId, + question, + ...(parsed.data.why ? { why: parsed.data.why } : {}), + }); + } catch (error) { + /* + * The thrown text goes in the trail and not into the answer. + * + * What a route throws is written for whoever operates the rota — a connection reset, a status + * line, a stack — and the answer here is paraphrased to the person who asked the question. + * `handoff-runner.ts` keeps the two apart for the same reason; this keeps the whole thing on + * the row, capped, and gives the Bot a sentence that is true whatever went wrong. + */ + thrown = (error instanceof Error ? error.message : String(error)).slice( + 0, + 400, + ); + outcome = { + refusal: + "That did not reach anybody: nobody could be asked just now. Say so plainly, answer " + + "only what you can settle yourself, and do not tell them a person has been asked.", + }; + } /* * Recorded either way. An escalation that could not be delivered is the one worth finding @@ -152,6 +196,9 @@ export function escalationTool(options: { ...("reached" in outcome ? { reached: outcome.reached } : { reason: outcome.refusal }), + // The route's own words, only when it threw them. `reason` above is the sentence the Bot + // was given; this is what actually went wrong, which is the pair `store.ts` keeps too. + ...(thrown ? { failure: thrown } : {}), }, }); } diff --git a/server/tests/agent-escalation.test.ts b/server/tests/agent-escalation.test.ts index 80ecaae6f..8b87a4125 100644 --- a/server/tests/agent-escalation.test.ts +++ b/server/tests/agent-escalation.test.ts @@ -117,6 +117,76 @@ describe("asking a person", () => { expect(written[0]?.eventType).toBe("agent.escalation_failed"); }); + /* + * The route above refused politely. A real one fails by throwing. + * + * `askTheirOwnPerson` cannot throw, so every route this repo runs takes the happy path and the + * gap was invisible — but the module says who a person is is a seam, and every route a company + * hands in is a duty desk or a rota reached over a network. A timeout there used to come straight + * back out of `execute`, ending the run with nothing said on the one tool whose job is to stop + * the Bot falling silent, and leaving no row behind to say the person was never asked. + */ + test("a route that throws is an answer, not the end of the run", async () => { + const { written, store } = recorder(); + const tool = escalationTool({ + from: FROM, + route: async () => { + throw new Error("connect ETIMEDOUT rota.internal:443"); + }, + auditStore: store, + }); + + const said = await tool.execute({ question: "which account?" }); + + expect(said).toContain("did not reach anybody"); + // The Bot must not go on to tell the person their question is with somebody. + expect(said).not.toContain(PUT_TO); + expect(written[0]?.eventType).toBe("agent.escalation_failed"); + }); + + test("what the route threw is on the row and not in the answer", async () => { + const { written, store } = recorder(); + const tool = escalationTool({ + from: FROM, + route: async () => { + throw new Error("connect ETIMEDOUT rota.internal:443"); + }, + auditStore: store, + }); + + const said = await tool.execute({ question: "which account?" }); + + // An internal address and error code are for whoever operates the rota, not for the person who + // asked the question and will read whatever the Bot paraphrases. + expect(said).not.toContain("rota.internal"); + expect(written[0]?.payload).toMatchObject({ + question: "which account?", + failure: "connect ETIMEDOUT rota.internal:443", + }); + }); + + /* + * A route can throw something that is not an Error, and a run must survive that too rather than + * failing inside the handler written to keep it alive. + */ + test("a route that throws something that is not an Error is still an answer", async () => { + const { written, store } = recorder(); + const tool = escalationTool({ + from: FROM, + route: async () => { + throw "the desk is closed"; + }, + auditStore: store, + }); + + const said = await tool.execute({ question: "which account?" }); + + expect(said).toContain("did not reach anybody"); + expect(written[0]?.payload).toMatchObject({ + failure: "the desk is closed", + }); + }); + /* * Mid-run with a person waiting: a throw ends the run with nothing said, which reads as the Bot * ignoring them.