diff --git a/.changeset/gateway-internal-error-one-code.md b/.changeset/gateway-internal-error-one-code.md new file mode 100644 index 0000000000..d59b863d99 --- /dev/null +++ b/.changeset/gateway-internal-error-one-code.md @@ -0,0 +1,43 @@ +--- +"@agent-native/core": patch +--- + +Stop the Builder gateway's 500 envelope from ending chats three different ways, and stop emitting the malformed tool schema that provokes it. + +The gateway answers 200 and then delivers its own internal-error envelope ("Sorry, +we ran into an issue processing your request. ERROR ID: …") as an in-stream frame, +so nothing structured reaches the layer that catches it. `runAgentLoopWithResume` +dropped straight to `internal_error` without asking the shared classifier, so one +upstream failure was persisted under three codes depending on which layer caught it +— `builder_gateway_internal_error`, `internal_error`, `unknown` — and only one of +those is on the client's recoverable list. Identical gateway failures therefore +ended some chats on the first attempt and sent others into a re-dispatch chain. +Both catch sites now classify the message first; `internal_error` stays the last +resort for a message nothing recognises. + +That envelope is also no longer auto-recoverable at the run level. Measured across +three production databases, 7 turns reached it and 0 of 7 ever finished, across 97 +runs — one turn burned 28 runs over 16 minutes on a single "Hey", with overlapping +concurrent runs on the same turn. The engine's in-request retry still covers the +genuinely transient case, and the recovery card keeps a deliberate Retry, because +its own copy ends with "Retry in a moment". + +The fallback Zod-to-JSON-Schema converter read a literal's value from `def.value`, +which Zod v4 does not define — it stores `values`. Every literal therefore emitted +`{"type":"undefined"}`, a type keyword no JSON Schema dialect defines, and an +`enum` of `[null]` instead of the real value. Literals are most often a +discriminated union's discriminator, so ordinary action schemas were affected. The +Builder gateway validates only a tool's top-level `input_schema.type`, so an +invented type on a nested field passes validation, reaches the provider, and comes +back as the opaque ERROR ID envelope — on every retry, because the same malformed +schema is resent verbatim. Literal values are now read from `values`, and no +literal can emit a type outside the seven JSON Schema names. + +An action schema carrying a literal that JSON cannot represent is now rejected when +the action is defined, rather than producing a broken request later. A `bigint` +literal made `JSON.stringify` throw while the request was being built, and +`undefined`, `NaN`, and `±Infinity` serialized to `null` — advertising a value the +Zod validator still rejects, so the schema and the validator disagreed with nothing +in the output left to detect it from. The check runs against the Zod def as well as +the emitted schema, because Zod's own converter turns `z.literal(NaN)` into +`const: null` before the emitted schema can be inspected. diff --git a/packages/core/src/action.spec.ts b/packages/core/src/action.spec.ts index 226085bd80..4fc502371f 100644 --- a/packages/core/src/action.spec.ts +++ b/packages/core/src/action.spec.ts @@ -353,6 +353,174 @@ describe("defineAction schema mode — tool parameter JSON Schema", () => { expect(params.properties.title.description).toBe("Form title"); }); + // JSON Schema has exactly seven type names and JavaScript's `typeof` is not a + // way to spell them. The Builder gateway validates only a tool's TOP-LEVEL + // `input_schema.type`, so an invented type on a NESTED field passes validation, + // reaches the provider, and comes back as the gateway's opaque "ERROR ID" 500 + // envelope — on every retry, because the same malformed schema is resent. + it("never emits a non-JSON-Schema type for a literal", () => { + const legalTypes = new Set([ + "string", + "number", + "integer", + "boolean", + "object", + "array", + "null", + ]); + const action = defineAction({ + description: "literal types", + schema: z.object({ + nothing: z.literal(null), + name: z.literal("fixed"), + count: z.literal(7), + ratio: z.literal(1.5), + flag: z.literal(true), + nested: z.object({ inner: z.literal(null) }), + items: z.array(z.literal(null)), + }), + run: async () => "ok", + }); + + // The fallback converter is what runs for a schema that exposes no + // standard-schema `jsonSchema` hook, so exercise that path explicitly rather + // than only Zod's own converter — the bug lived exclusively in the fallback. + const withoutJsonSchemaHook = (schema: T): T => { + const standard = (schema as any)["~standard"]; + (schema as any)["~standard"] = { ...standard, jsonSchema: undefined }; + return schema; + }; + const fallbackAction = defineAction({ + description: "literal types via fallback converter", + schema: withoutJsonSchemaHook( + z.object({ + kind: z.literal("weekly"), + nested: z.object({ inner: z.literal(3) }), + mode: z.union([z.literal("a"), z.literal("b")]), + }), + ), + run: async () => "ok", + }); + + const illegal: string[] = []; + const walk = (node: unknown) => { + if (!node || typeof node !== "object") return; + if (Array.isArray(node)) { + node.forEach(walk); + return; + } + const record = node as Record; + const type = record.type; + if (typeof type === "string" && !legalTypes.has(type)) illegal.push(type); + Object.values(record).forEach(walk); + }; + walk(action.tool.parameters); + walk(fallbackAction.tool.parameters); + + expect(illegal).toEqual([]); + const props = (action.tool.parameters as any).properties; + expect(props.nothing).toMatchObject({ type: "null" }); + expect(props.name).toMatchObject({ type: "string" }); + expect(props.flag).toMatchObject({ type: "boolean" }); + + // The fallback path must also carry the literal's VALUE. Reading the + // singular `def.value` returned undefined, which serialized to `[null]` and + // told the model the discriminator accepts null. + const fallbackProps = (fallbackAction.tool.parameters as any).properties; + expect(fallbackProps.kind).toMatchObject({ + type: "string", + enum: ["weekly"], + }); + expect(fallbackProps.nested.properties.inner).toMatchObject({ + type: "integer", + enum: [3], + }); + expect(fallbackProps.mode).toMatchObject({ + type: "string", + enum: ["a", "b"], + }); + }); + + // A tool schema only ever travels as JSON, so a value JSON cannot carry + // describes a contract no caller can satisfy. `bigint` makes `JSON.stringify` + // THROW while the request is being built; `undefined`/`NaN`/`±Infinity` + // serialize to `null`, advertising a value the Zod validator still rejects. + // Both are worse than failing at definition time. + it.each([ + ["a BigInt", z.literal(1n), "a BigInt"], + ["undefined", z.literal(undefined as any), "undefined"], + ["NaN", z.literal(NaN), "NaN"], + ["negative Infinity", z.literal(-Infinity), "Infinity"], + ])("rejects a literal of %s", (_label, literal, expected) => { + expect(() => + defineAction({ + description: "unrepresentable literal", + schema: z.object({ field: literal as any }), + run: async () => "ok", + }), + ).toThrow(new RegExp(`field is a literal of ${expected}`)); + }); + + // The value must be found wherever it hides, including the places a converter + // would otherwise launder it before anything could inspect the output. + it.each([ + [ + "nested in an object", + z.object({ outer: z.object({ field: z.literal(1n) }) }), + "outer.field", + ], + ["inside an array", z.object({ field: z.array(z.literal(1n)) }), "field"], + [ + "behind optional", + z.object({ field: z.literal(NaN).optional() }), + "field", + ], + [ + "in a union branch", + z.object({ field: z.union([z.literal("s"), z.literal(NaN)]) }), + "field|1", + ], + ])("rejects an unrepresentable literal %s", (_label, schema, path) => { + let caught: any; + try { + defineAction({ + description: "d", + schema: schema as any, + run: async () => "ok", + }); + } catch (err) { + caught = err; + } + expect(caught?.errorCode).toBe("SCHEMA_NOT_JSON_REPRESENTABLE"); + expect(caught.details.problems.join(";")).toContain(path); + }); + + // The failure this guards against was a throw during serialization, so assert + // on serialization itself rather than only on the object graph. + it("emits tool parameters that survive JSON.stringify", () => { + const action = defineAction({ + description: "serializable", + schema: z.object({ + title: z.string(), + count: z.number().int().optional(), + kind: z.literal("weekly"), + nothing: z.literal(null), + ratio: z.literal(1.5), + mode: z.union([z.literal("a"), z.literal("b")]), + choice: z.enum(["x", "y"]), + }), + run: async () => "ok", + }); + + expect(() => JSON.stringify(action.tool.parameters)).not.toThrow(); + const roundTripped = JSON.parse(JSON.stringify(action.tool.parameters)); + // A silent `null` here is the divergence case: it would mean a value was + // dropped or coerced on the way to JSON. + expect(JSON.stringify(roundTripped)).not.toContain('"const":null,"type"'); + expect(roundTripped.properties.kind).toMatchObject({ const: "weekly" }); + expect(roundTripped.properties.nothing).toMatchObject({ type: "null" }); + }); + it("strips the $schema key so the Claude API (draft 2020-12) does not reject it", () => { const action = defineAction({ description: "with schema key", @@ -389,93 +557,6 @@ describe("defineAction schema mode — tool parameter JSON Schema", () => { expect(params.properties.cfg.default).toEqual({ propertyNames: "x" }); }); - // OpenAI answers a `oneOf` anywhere in a function schema with - // "Invalid schema for function 'x': ... 'oneOf' is not permitted" and 400s - // the whole request before a token streams. Zod emits `oneOf` for every - // discriminated union, so this was 178k errors across 786 users over seven - // weeks from one action. - it("rewrites oneOf to anyOf so OpenAI does not reject the function schema", () => { - const action = defineAction({ - description: "with a discriminated union", - schema: z.object({ - operations: z.array( - z.discriminatedUnion("op", [ - z.object({ op: z.literal("add"), panelId: z.string() }), - z.object({ - op: z.literal("remove"), - panelIds: z.array(z.string()), - }), - ]), - ), - }), - run: async () => "ok", - }); - const json = JSON.stringify(action.tool.parameters); - expect(json).not.toContain('"oneOf"'); - expect(json).toContain('"anyOf"'); - }); - - it("keeps every branch when rewriting a nested union", () => { - const action = defineAction({ - description: "nested union", - schema: z.object({ - outer: z.object({ - inner: z.discriminatedUnion("kind", [ - z.object({ kind: z.literal("a"), a: z.string() }), - z.object({ kind: z.literal("b"), b: z.string() }), - z.object({ kind: z.literal("c"), c: z.string() }), - ]), - }), - }), - run: async () => "ok", - }); - const params = action.tool.parameters as any; - const inner = params.properties.outer.properties.inner; - expect(inner.oneOf).toBeUndefined(); - expect(inner.anyOf).toHaveLength(3); - }); - - // OpenAI rejects a schema position with no `type` — "schema must have a - // 'type' key" — and 400s the whole request, exactly like `oneOf` did. This - // surfaced only after the oneOf fix let the validator reach the next layer. - it("gives z.unknown() a typed value union so OpenAI accepts it", () => { - const action = defineAction({ - description: "typeless field", - schema: z.object({ value: z.unknown() }), - run: async () => "ok", - }); - const value = (action.tool.parameters as any).properties.value; - expect(Array.isArray(value.anyOf)).toBe(true); - expect(value.anyOf.map((b: any) => b.type)).toContain("string"); - expect(value.anyOf.map((b: any) => b.type)).toContain("object"); - }); - - it("types the value schema inside a record so nothing is left bare", () => { - const action = defineAction({ - description: "record of unknown", - schema: z.object({ patch: z.record(z.string(), z.unknown()) }), - run: async () => "ok", - }); - const patch = (action.tool.parameters as any).properties.patch; - expect(patch.type).toBe("object"); - const extra = patch.additionalProperties; - if (extra && typeof extra === "object") { - expect(Array.isArray(extra.anyOf)).toBe(true); - } - }); - - // An enum carries its own shape; adding a value union would widen it. - it("leaves an enum-only schema alone", () => { - const action = defineAction({ - description: "enum field", - schema: z.object({ mode: z.enum(["a", "b"]) }), - run: async () => "ok", - }); - const mode = (action.tool.parameters as any).properties.mode; - expect(mode.enum).toEqual(["a", "b"]); - expect(mode.anyOf).toBeUndefined(); - }); - it("stores the original schema on the entry for downstream re-validation", () => { const schema = z.object({ x: z.string() }); const action = defineAction({ diff --git a/packages/core/src/action.ts b/packages/core/src/action.ts index 9048644878..e181c88b66 100644 --- a/packages/core/src/action.ts +++ b/packages/core/src/action.ts @@ -1277,6 +1277,160 @@ const PROVIDER_REJECTED_KEYWORDS = [ "not", ] as const; +/** + * Why a value that JSON cannot carry must fail here rather than later. + * + * A tool schema is only ever delivered as JSON, so a value JSON cannot represent + * describes a contract no caller can satisfy. There is no safe way to smuggle it + * through, and the two failure modes it produces are both worse than an error at + * definition time: + * + * - `bigint` makes `JSON.stringify` THROW, so the turn dies while building the + * request, before any provider sees it. + * - `undefined`, `NaN`, and `±Infinity` serialize to `null`. The advertised + * schema then says `null` is the accepted value while the Zod validator still + * demands the original, so the model is told to send something that can never + * validate — the silent-divergence case this repo treats as a bug, not a guard. + * + * Checked on the FINAL schema from either converter, because Zod's own converter + * reaches this too: it emits `const: NaN` for `z.literal(NaN)` without + * complaint, and only the fallback path is reached for bigint literals (Zod + * throws "BigInt literals cannot be represented in JSON Schema", which is what + * sends us to the fallback in the first place). + */ +function describeJsonUnsafeValue(value: unknown): string | null { + if (typeof value === "bigint") return "a BigInt"; + if (typeof value === "symbol") return "a symbol"; + if (typeof value === "function") return "a function"; + if (value === undefined) return "undefined"; + if (typeof value === "number" && !Number.isFinite(value)) { + return Number.isNaN(value) ? "NaN" : "Infinity"; + } + return null; +} + +function collectJsonUnsafeSchemaValues( + node: unknown, + path: string, + found: string[], + seen: Set, +): void { + const unsafe = describeJsonUnsafeValue(node); + if (unsafe) { + found.push(`${path || "schema"} is ${unsafe}`); + return; + } + if (!node || typeof node !== "object") return; + if (seen.has(node)) return; + seen.add(node); + if (Array.isArray(node)) { + node.forEach((item, index) => + collectJsonUnsafeSchemaValues(item, `${path}[${index}]`, found, seen), + ); + return; + } + for (const [key, value] of Object.entries(node as Record)) { + collectJsonUnsafeSchemaValues( + value, + path ? `${path}.${key}` : key, + found, + seen, + ); + } +} + +/** + * The same check one layer earlier, against the Zod def rather than the emitted + * schema, because a converter can launder the problem before this file sees it: + * Zod's own converter turns `z.literal(NaN)` into `const: null` without + * complaint, which reads as a legitimate "null is the accepted value" and is + * indistinguishable from an author writing `z.literal(null)`. The validator still + * demands NaN, so the advertised contract and the enforced one disagree — with + * nothing left in the output to detect it from. + * + * Deliberately tolerant of shape: an unrecognised def is skipped rather than + * treated as a problem, so this can only ever reject a literal it positively + * identified. + */ +const LITERAL_DEF_CHILD_KEYS = [ + "element", + "innerType", + "in", + "out", + "valueType", + "keyType", +] as const; + +function collectJsonUnsafeLiteralDefs( + node: any, + path: string, + found: string[], + seen: Set, +): void { + const def = node?._zod?.def ?? node; + if (!def || typeof def !== "object") return; + if (seen.has(def)) return; + seen.add(def); + + if (def.type === "literal") { + for (const [index, value] of literalValuesOfDef(def).entries()) { + const unsafe = describeJsonUnsafeValue(value); + if (unsafe) { + found.push(`${path || "schema"} is a literal of ${unsafe}`); + } + void index; + } + return; + } + + if (def.shape && typeof def.shape === "object") { + for (const [key, child] of Object.entries(def.shape)) { + collectJsonUnsafeLiteralDefs( + child, + path ? `${path}.${key}` : key, + found, + seen, + ); + } + } + if (Array.isArray(def.options)) { + def.options.forEach((child: unknown, index: number) => + collectJsonUnsafeLiteralDefs(child, `${path}|${index}`, found, seen), + ); + } + for (const key of LITERAL_DEF_CHILD_KEYS) { + if (def[key]) collectJsonUnsafeLiteralDefs(def[key], path, found, seen); + } +} + +function assertJsonSafeLiteralDefs(schema: unknown): void { + const found: string[] = []; + collectJsonUnsafeLiteralDefs(schema, "", found, new Set()); + if (found.length > 0) throw jsonUnrepresentableSchemaError(found); +} + +function jsonUnrepresentableSchemaError( + problems: string[], +): ActionContractError { + return new ActionContractError( + `Action schema cannot be represented as a JSON tool schema: ${problems.join("; ")}. ` + + "Tool schemas are sent as JSON, so use a JSON-representable literal " + + "(string, finite number, boolean, or null) instead.", + { + errorCode: "SCHEMA_NOT_JSON_REPRESENTABLE", + details: { problems }, + statusCode: 500, + }, + ); +} + +function assertJsonSafeToolSchema(schema: T): T { + const found: string[] = []; + collectJsonUnsafeSchemaValues(schema, "", found, new Set()); + if (found.length > 0) throw jsonUnrepresentableSchemaError(found); + return schema; +} + export function stripUnsupportedSchemaKeywords(node: T): T { if (!node || typeof node !== "object" || Array.isArray(node)) return node; const obj = node as Record; @@ -1339,6 +1493,10 @@ function schemaToJsonSchema( ): ActionTool["parameters"] { const s = schema as any; + // Before either converter runs, so a value one of them would launder into a + // plausible-looking `null` is still identifiable. + assertJsonSafeLiteralDefs(s); + // Prefer Zod's own JSON Schema output — it handles descriptions, // enums, coerce, and all type wrappers correctly. if (s["~standard"]?.jsonSchema?.input) { @@ -1351,21 +1509,87 @@ function schemaToJsonSchema( if (result && typeof result === "object") { delete result.$schema; } - return stripUnsupportedSchemaKeywords(result) as ActionTool["parameters"]; - } catch { + return assertJsonSafeToolSchema( + stripUnsupportedSchemaKeywords(result), + ) as ActionTool["parameters"]; + } catch (err) { + // A schema this converter produced but JSON cannot carry is a real contract + // error, not a "try the other converter" signal — the fallback would emit + // the same unusable value. Only a conversion failure falls through. + if (isActionContractError(err)) throw err; // Fall through to manual converter } } // Fallback: manual conversion from Zod v4 internal defs if (s._zod?.def) { - return stripUnsupportedSchemaKeywords(zodDefToJsonSchema(s._zod.def)); + return assertJsonSafeToolSchema( + stripUnsupportedSchemaKeywords(zodDefToJsonSchema(s._zod.def)), + ); } // Last resort: empty object schema return { type: "object" as const, properties: {} }; } +/** + * JSON Schema recognises exactly seven type names, and JavaScript's `typeof` is + * not one of the ways to spell them: `typeof null` is `"object"`, and `bigint`, + * `undefined`, `symbol`, and `function` have no JSON Schema equivalent at all. + * Emitting one of those as a `type` produces a schema the Builder gateway's + * request validator accepts — it only checks a tool's TOP-LEVEL + * `input_schema.type` — and the upstream provider then rejects, which comes back + * as the gateway's opaque "ERROR ID" 500 envelope rather than a typed 400. A + * nested field is therefore able to take down a whole turn with an error that + * names nothing, on every retry, because the malformed schema is resent verbatim. + * + * Return no `type` at all rather than an invented one: `enum` alone still pins + * the value, and an absent keyword is valid where a bogus one is not. + */ +/** + * Zod v4 stores a literal's value in `def.values` (an array — `z.literal` accepts + * a set of values), NOT in `def.value`. Reading the singular key returns + * `undefined` for every literal, which is how `typeof def.value` came to emit + * `{"type":"undefined"}`: a type keyword no JSON Schema dialect defines. A + * literal is most often a discriminated union's discriminator, so this was + * reached by ordinary action schemas, not an exotic corner. + * + * Keep reading the singular key as a fallback so a schema from a different + * standard-schema implementation still resolves. + * + * Returns `[]` when neither key is PRESENT, which is different from a literal + * whose value happens to be `undefined`: an unrecognised def shape must degrade + * to a permissive schema, while a real `undefined` literal is a contract error + * `assertJsonSafeToolSchema` is entitled to reject. + */ +function literalValuesOfDef(def: any): unknown[] { + if (Array.isArray(def?.values)) return def.values; + if (def && "values" in def) return [def.values]; + if (def && "value" in def) return [def.value]; + return []; +} + +function jsonSchemaTypeOfLiteral(value: unknown): { type?: string } { + if (value === null) return { type: "null" }; + switch (typeof value) { + case "string": + return { type: "string" }; + case "number": + // NaN and ±Infinity have no JSON form; `assertJsonSafeToolSchema` rejects + // them, so do not hand back a `number` that implies they are usable. + return Number.isFinite(value) + ? { type: Number.isInteger(value) ? "integer" : "number" } + : {}; + case "boolean": + return { type: "boolean" }; + // `bigint` is deliberately absent: JSON has no bigint, so there is no honest + // type to claim. Mapping it to `integer` would have advertised a value that + // makes `JSON.stringify` throw while building the request. + default: + return {}; + } +} + /** * Convert a Zod v4 internal def to JSON Schema. * Handles the common types used in action parameters. @@ -1434,7 +1658,22 @@ function zodDefToJsonSchema(def: any): any { } if (type === "literal") { - return { type: typeof def.value, enum: [def.value] }; + const values = literalValuesOfDef(def); + // An unrecognised literal def shape yields no values at all. Emit a + // permissive schema rather than an `enum: []`, which would advertise a field + // that accepts nothing. + if (values.length === 0) return {}; + const jsonTypes = [ + ...new Set( + values.map((value: unknown) => jsonSchemaTypeOfLiteral(value).type), + ), + ]; + return { + ...(jsonTypes.length === 1 && jsonTypes[0] !== undefined + ? { type: jsonTypes[0] } + : {}), + enum: values, + }; } if (type === "array") { @@ -1479,13 +1718,11 @@ function zodDefToJsonSchema(def: any): any { (o: any) => o?._zod?.def?.type === "literal", ); if (allLiterals) { - const values = def.options.map((o: any) => o._zod.def.value); + const values = def.options.flatMap((o: any) => + literalValuesOfDef(o._zod.def), + ); const jsonTypeOf = (v: any) => - typeof v === "number" - ? "number" - : typeof v === "boolean" - ? "boolean" - : "string"; + jsonSchemaTypeOfLiteral(v).type ?? "string"; const uniqueTypes = [...new Set(values.map(jsonTypeOf))]; if (uniqueTypes.length === 1) { // Homogeneous literal union (e.g. all numbers) — derive the JSON diff --git a/packages/core/src/agent/run-loop-with-resume.spec.ts b/packages/core/src/agent/run-loop-with-resume.spec.ts index 41005a72fc..a062d7d9cf 100644 --- a/packages/core/src/agent/run-loop-with-resume.spec.ts +++ b/packages/core/src/agent/run-loop-with-resume.spec.ts @@ -895,6 +895,104 @@ describe("runAgentLoopDirectWithSoftTimeout", () => { ]); }); + // The gateway answers 200 and then emits its 500 envelope as an in-stream + // frame, so nothing structured reaches this catch — only the sentence. Falling + // straight to `internal_error` is what made one upstream failure arrive under + // three different codes depending on which layer caught it, and only one of + // the three is on the client's recoverable list. Production showed the same + // envelope persisted as `internal_error` in calendar and clips while analytics + // recorded it as `builder_gateway_internal_error`. + it("names the Builder gateway 500 envelope when the thrown error carries no code", async () => { + const outcomes: AgentLoopOutcome[] = []; + const message = + "Sorry, we ran into an issue processing your request. " + + "ERROR ID: 4dbb6f30593c44d093090a37a99012a2"; + mockRunAgentLoop.mockImplementation(async () => { + throw new Error(message); + }); + + await expect( + runAgentLoopDirectWithSoftTimeout( + makeOpts( + [{ role: "user", content: [{ type: "text", text: "Hey" }] }], + new AbortController().signal, + undefined, + undefined, + outcomes, + ), + 60_000, + ), + ).rejects.toThrow(message); + + expect(outcomes).toEqual([ + { + state: "failed", + code: "builder_gateway_internal_error", + retryable: false, + message, + }, + ]); + }); + + // `timeoutMs <= 0` takes a separate direct-loop branch with its OWN catch, so + // the loop-path test above does not exercise it. Both catches had the same + // blind fallback, so both need the same proof. + it("names the gateway 500 envelope on the direct path too (timeoutMs <= 0)", async () => { + const outcomes: AgentLoopOutcome[] = []; + const message = + "Sorry, we ran into an issue processing your request. " + + "ERROR ID: 39a8c319984746da9c7f351f3067913d"; + mockRunAgentLoop.mockImplementation(async () => { + throw new Error(message); + }); + + await expect( + runAgentLoopDirectWithSoftTimeout( + makeOpts( + [{ role: "user", content: [{ type: "text", text: "Hey" }] }], + new AbortController().signal, + undefined, + undefined, + outcomes, + ), + 0, + ), + ).rejects.toThrow(message); + + expect(outcomes).toEqual([ + { + state: "failed", + code: "builder_gateway_internal_error", + retryable: false, + message, + }, + ]); + }); + + // `internal_error` must stay meaningful: it is the code for a message nothing + // recognises, not the code for every uncoded failure. + it("still falls back to internal_error for an unrecognisable message", async () => { + const outcomes: AgentLoopOutcome[] = []; + mockRunAgentLoop.mockImplementation(async () => { + throw new Error("something nobody has classified"); + }); + + await expect( + runAgentLoopDirectWithSoftTimeout( + makeOpts( + [{ role: "user", content: [{ type: "text", text: "go" }] }], + new AbortController().signal, + undefined, + undefined, + outcomes, + ), + 60_000, + ), + ).rejects.toThrow("something nobody has classified"); + + expect(outcomes[0]).toMatchObject({ code: "internal_error" }); + }); + it("bails out after MAX_RUN_LOOP_CONTINUATIONS to prevent infinite loops", async () => { let attempts = 0; mockRunAgentLoop.mockImplementation(async () => { diff --git a/packages/core/src/agent/run-loop-with-resume.ts b/packages/core/src/agent/run-loop-with-resume.ts index 2b251425f8..dcc9df5e81 100644 --- a/packages/core/src/agent/run-loop-with-resume.ts +++ b/packages/core/src/agent/run-loop-with-resume.ts @@ -21,6 +21,10 @@ * uniform "continue" instruction regardless of which recovery fired. */ +import { + classifyTerminalErrorCode, + describeErrorWithCauses, +} from "./engine/error-detail.js"; import type { EngineMessage } from "./engine/types.js"; import { runAgentLoop, @@ -317,6 +321,37 @@ export const RUN_BUDGET_EXHAUSTED_MESSAGE = "I stopped rather than keep retrying silently. " + "Check any completed tool cards above before retrying, ideally as one smaller follow-up."; +/** + * The code a failed attempt is persisted and reported under. + * + * A thrown error that carries no `errorCode` is not automatically anonymous: + * its message is frequently one the shared classifier can name, and the Builder + * gateway's own 500 envelope is the case that matters most in production. The + * gateway answers 200 and then emits the envelope as an in-stream frame, so + * there is no HTTP status to read and no structured code attached — falling + * straight to a generic code here is what made one upstream failure arrive as + * three different codes depending on which layer caught it + * (`builder_gateway_internal_error` when the engine classified it, + * `internal_error` here, `unknown` at persistence). Only one of those three is + * on the client's recoverable list, so identical gateway failures ended some + * chats instantly and sent others into a re-dispatch chain. + * + * Classify the message before giving up on it, so every path through this + * function reports the same code for the same upstream failure. `internal_error` + * stays the last resort for a message nothing recognises — it must remain + * distinguishable from a named failure rather than becoming the name for all of + * them. + */ +function resolveAttemptErrorCode(err: unknown): string { + const candidate = err as { errorCode?: unknown } | null; + if (typeof candidate?.errorCode === "string" && candidate.errorCode) { + return candidate.errorCode; + } + return ( + classifyTerminalErrorCode(describeErrorWithCauses(err)) ?? "internal_error" + ); +} + /** * Internal entry point used by the agent-chat plugin's run handler. Wraps * `runAgentLoop` with soft-timeout + resumable-error continuation recovery. @@ -389,10 +424,7 @@ export async function runAgentLoopDirectWithSoftTimeout( ? { state: "canceled", message: "Agent run was aborted." } : { state: "failed", - code: - typeof candidate?.errorCode === "string" && candidate.errorCode - ? candidate.errorCode - : "internal_error", + code: resolveAttemptErrorCode(err), retryable: isResumableEngineError(err), message: typeof candidate?.message === "string" @@ -649,10 +681,7 @@ export async function runAgentLoopDirectWithSoftTimeout( const candidate = err as { errorCode?: unknown; message?: unknown }; reportFinalOutcome({ state: "failed", - code: - typeof candidate?.errorCode === "string" && candidate.errorCode - ? candidate.errorCode - : "internal_error", + code: resolveAttemptErrorCode(err), retryable: false, message: typeof candidate?.message === "string" diff --git a/packages/core/src/client/chat/run-recovery.spec.tsx b/packages/core/src/client/chat/run-recovery.spec.tsx index d3648b7155..94d7b409de 100644 --- a/packages/core/src/client/chat/run-recovery.spec.tsx +++ b/packages/core/src/client/chat/run-recovery.spec.tsx @@ -229,6 +229,55 @@ describe("run recovery surfaces", () => { expect(container.textContent).toContain("Retry"); }); + // The gateway 500 is no longer auto-recoverable (0 recoveries in 97 production + // runs), so `recoverable` is absent and Continue is correctly gone. Its own copy + // still ends with "Retry in a moment", so without an explicit branch the card + // would be the dead end that sentence points at. + it("offers a working Retry for the gateway internal-error envelope", async () => { + const onRetry = vi.fn(); + const onContinue = vi.fn(); + + await act(async () => { + root.render( + + + , + ); + }); + + const retry = [...container.querySelectorAll("button")].find( + (button) => button.textContent?.trim() === "Retry", + ); + expect(retry).toBeTruthy(); + + await act(async () => { + retry!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + expect(onRetry).toHaveBeenCalledTimes(1); + + // Not recoverable, so no Continue — auto-continuing is what burned 28 runs. + const labels = [...container.querySelectorAll("button")].map((button) => + button.textContent?.trim(), + ); + expect(labels).not.toContain("Continue"); + expect(onContinue).not.toHaveBeenCalled(); + }); + it("dismisses the recovery card after saving a provider key", async () => { const onDismiss = vi.fn(); diff --git a/packages/core/src/client/chat/run-recovery.tsx b/packages/core/src/client/chat/run-recovery.tsx index fa37481e4c..73bbc95498 100644 --- a/packages/core/src/client/chat/run-recovery.tsx +++ b/packages/core/src/client/chat/run-recovery.tsx @@ -21,6 +21,7 @@ import { } from "@tabler/icons-react"; import { useState, useEffect, useCallback, useRef } from "react"; +import { BUILDER_GATEWAY_INTERNAL_ERROR_CODE } from "../../agent/engine/error-detail.js"; import { agentNativePath } from "../api-path.js"; import { writeClipboardText } from "../clipboard.js"; import { @@ -497,7 +498,19 @@ export function RunErrorRecoveryCard({ // points at a button that isn't there. const isUnblockableExternally = info.errorCode === "email_verification_required"; - const canRetry = canRecover || isProviderAuthError || isUnblockableExternally; + // Automatic re-dispatch of this one is futile — the same payload fails the + // same way, measured at 0 recoveries in 97 production runs — so it is not on + // the client's auto-recoverable list. A retry the USER chooses is a different + // decision: minutes have passed, the upstream may have recovered, and its own + // copy ends with "Retry in a moment". Without this the card is the dead end + // that copy points at. + const isDeliberateRetryGatewayFailure = + info.errorCode === BUILDER_GATEWAY_INTERNAL_ERROR_CODE; + const canRetry = + canRecover || + isProviderAuthError || + isUnblockableExternally || + isDeliberateRetryGatewayFailure; const builderReconnectResolved = shouldShowBuilderReconnect && builderReconnect.hasFetchedStatus && diff --git a/packages/core/src/client/sse-event-processor.spec.ts b/packages/core/src/client/sse-event-processor.spec.ts index b6aa491eea..cc520b22e1 100644 --- a/packages/core/src/client/sse-event-processor.spec.ts +++ b/packages/core/src/client/sse-event-processor.spec.ts @@ -3713,42 +3713,60 @@ describe("SSE event processor error classification", () => { }); }); - it("auto-continues the Builder gateway internal-error envelope", async () => { + // Measured across the analytics/clips/calendar production databases: 7 turns + // reached this code and 0 of 7 ever finished, over 97 runs — one turn burned + // 28 runs across 16 minutes on a single "Hey", with overlapping concurrent + // runs. The envelope is emitted for deterministic upstream rejections, so a + // fresh run resends the request that just failed. `providerRetryable: true` + // is present on the real production event and must NOT revive it here: the + // engine's in-request retry already covers the transient case. + it("does not re-dispatch a run for the Builder gateway internal-error envelope", async () => { + const dispatchEvent = vi.fn(); + vi.stubGlobal("window", { dispatchEvent }); + vi.stubGlobal( + "CustomEvent", + class { + type: string; + detail: unknown; + constructor(type: string, init?: { detail?: unknown }) { + this.type = type; + this.detail = init?.detail; + } + }, + ); const message = "Sorry, we ran into an issue processing your request. " + "ERROR ID: bebaeb5da13441539790834b63ff955a"; - const err = await readSSEStream( + const results = []; + for await (const result of readSSEStream( eventStream([ { type: "error", error: message, errorCode: "builder_gateway_internal_error", + providerRetryable: true, }, ]), [], { value: 0 }, "tab-gateway-internal", - ) - [Symbol.asyncIterator]() - .next() - .then( - () => undefined, - (caught) => caught, - ); + )) { + results.push(result); + } - expect(err).toBeInstanceOf(AgentAutoContinueSignal); - expect((err as AgentAutoContinueSignal).errorInfo).toMatchObject({ - errorCode: "builder_gateway_internal_error", - recoverable: true, + const terminal = results.at(-1); + expect(terminal?.status).toMatchObject({ + type: "incomplete", + reason: "error", }); + const runError = terminal?.metadata?.custom?.runError as + | { errorCode?: string; message?: string; details?: string } + | undefined; + expect(runError?.errorCode).toBe("builder_gateway_internal_error"); // The correlation id is the only part support can act on, so it stays — // just not as the whole sentence the user reads. - expect((err as AgentAutoContinueSignal).errorInfo?.details).toContain( - "bebaeb5da13441539790834b63ff955a", - ); - expect((err as AgentAutoContinueSignal).errorInfo?.message).not.toContain( - "ERROR ID", - ); + expect(runError?.details).toContain("bebaeb5da13441539790834b63ff955a"); + expect(runError?.message).not.toContain("ERROR ID"); }); it("surfaces run_budget_exhausted as a loud terminal error without auto-continuing", async () => { diff --git a/packages/core/src/client/sse-event-processor.ts b/packages/core/src/client/sse-event-processor.ts index 8ec43d1b95..d4433ab969 100644 --- a/packages/core/src/client/sse-event-processor.ts +++ b/packages/core/src/client/sse-event-processor.ts @@ -820,6 +820,21 @@ function isAutoRecoverableError(ev: SSEEvent, errMsg: string): boolean { // (each turn cleared+regenerated visible content) for users hitting a // misbehaving Builder route. Surface the error instead. code === "builder_gateway_error" || + // The gateway's unhandled-500 envelope ("Sorry, we ran into an issue + // processing your request. ERROR ID: "), for the same reason as + // `builder_gateway_error` directly above: the production-agent already + // retries it synchronously up to MAX_RETRIES before it can reach here, so a + // fresh run only resends the identical request. Measured across the + // analytics/clips/calendar production databases, 7 turns reached this code + // and 0 of 7 ever reached `done` — 97 runs, one of them 28 runs over 16 + // minutes on a single "Hey", with overlapping concurrent runs on the same + // turn. The envelope is emitted for deterministic upstream rejections (a + // malformed nested tool-schema `type` reproduces it 6/6), which is why + // retrying cannot help: the same payload fails the same way every time. + // It stays `providerRetryable` for the ENGINE's in-request retry, which is + // cheap and does catch the genuinely transient case; this list governs + // whole-run re-dispatch, which does not. + code === BUILDER_GATEWAY_INTERNAL_ERROR_CODE || // The hosted run exhausted its in-invocation continuation budget without // finishing (run-loop-with-resume.ts). It's flagged `recoverable: true` so // the recovery banner reads "stopped before finishing", but it must NOT @@ -848,9 +863,6 @@ function isAutoRecoverableError(ev: SSEEvent, errMsg: string): boolean { code === "http_408" || code === "http_429" || code === "http_500" || - // The gateway's unhandled-500 envelope delivered in-stream instead of as a - // status. Recoverable for the same reason `http_500` is. - code === BUILDER_GATEWAY_INTERNAL_ERROR_CODE || code === "http_502" || code === "http_503" || code === "http_504" ||