diff --git a/CHANGELOG.md b/CHANGELOG.md index d945a921..650b551a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ npm release are grouped under the in-development version that introduced them. ### Added +- **`makeLlmSurface` is exported from `stitchapi/llm`**, with its `LlmDefaults` argument. ([#699](https://github.com/rejifald/StitchAPI/issues/699)) + The exported `llmSurface` is only the `{ id: 'llm' }` identity — nothing to wrap — and the real + factory, which closes over the provider and defaults, had no `export`, so layering behaviour over + the llm surface ([P21](docs/CONTRACT.md#p21--every-contract-has-an-extension-seam)) meant forking core. + - **`throttle.concurrency` goes fleet-wide too, by lease.** ([ADR 0025](docs/adr/0025-fleet-wide-concurrency-by-lease.md)) [ADR 0024](docs/adr/0024-the-fleet-wide-pacing-cell.md) made the rate budget fleet-wide and left the concurrency cap per-process, so `concurrency: 10` across eight workers was really a fleet cap @@ -566,6 +571,10 @@ npm release are grouped under the in-development version that introduced them. ### Fixed +- **A truncated LLM completion is no longer a silent success.** ([#699](https://github.com/rejifald/StitchAPI/issues/699)) + `finishReason` was lifted by both provider mappings and read by nothing, so a completion cut short + at the token cap resolved `ok: true` with `findings: []`. It now sets a normalised `truncated` on + `LlmResult` and emits a `warn` drift finding — non-fatal; failing the call is a follow-up. - **An accepted non-2xx no longer becomes a cached absence.** ([#704](https://github.com/rejifald/StitchAPI/issues/704)) The store gate was `out.ok` alone, so `verdict: { accept: [404] }` cached the absence behind the `404` — masking a record created after diff --git a/packages/core/src/engine.ts b/packages/core/src/engine.ts index fbf859a6..cd1f1bbb 100644 --- a/packages/core/src/engine.ts +++ b/packages/core/src/engine.ts @@ -1231,7 +1231,17 @@ async function* runFrom( // It reads the RAW response body, not `value` — the flag indexes what the server sent, before // transform/pick reshaped it. const flagged = flagFinding(res, cfg); - const findings = flagged ? [flagged, ...outputFindings] : outputFindings; + // Three diagnostic sources, merged in PIPELINE order so a reader of `.inspect().findings` walks + // the call the way it ran: what the surface noticed while interpreting the response + // (`SurfaceOutcome.findings` — llm's truncated completion, issue #699), then the verdict + // config's inert flag, then the `output` contract's drift. None of the first two is levelled by + // `drift.severity`: that resolves inside the diff, over the kinds the diff produces, and these + // two are authored at a fixed level rather than derived from a comparison. + const findings = [ + ...(outcome.findings ?? []), + ...(flagged ? [flagged] : []), + ...outputFindings, + ]; let fatal = false; for (const finding of findings) { yield { type: 'drift', finding, at: now() }; diff --git a/packages/core/src/llm.ts b/packages/core/src/llm.ts index 89625fd0..9e10f1b7 100644 --- a/packages/core/src/llm.ts +++ b/packages/core/src/llm.ts @@ -17,6 +17,7 @@ import { makeStitch } from './stitch'; import { verdictOf } from './surface'; import type { Surface, SurfaceOutcome } from './surface'; import { + type DriftFinding, type NoRequestShapeOnLlm, type NoUnknownKeys, type NoUnknownNestedKeys, @@ -52,10 +53,60 @@ export interface LlmResult { text: string; model?: string; usage?: { inputTokens?: number; outputTokens?: number }; + /** Why the provider stopped generating, in the PROVIDER's own vocabulary — anthropic's + * `stop_reason` (`end_turn`, `max_tokens`, …), openai's `finish_reason` (`stop`, `length`, …). + * The field name is normalised; the VALUE is not, because there is no cross-vendor standard to + * normalise it to. {@link LlmResult.truncated} is the one question worth answering portably. */ finishReason?: string; + /** + * Did the completion stop because it hit the token cap — i.e. is `text` a PARTIAL answer? + * + * The surface derives this from {@link LlmResult.finishReason} so a caller never has to know + * that anthropic spells it `max_tokens` and openai spells it `length`. Three-state on purpose: + * `true` truncated, `false` finished on its own terms, and **absent** = unknown, because a + * provider that lifted no `finishReason` gave no grounds for either answer. A confident `false` + * over silence would be the same failure this field exists to fix, one level up. + * + * A BYO provider whose stop vocabulary is neither vendor's may set this in its own `parse`; the + * surface defers to that rather than guessing (CONTRACT.md P21 — the seam is the provider). + */ + truncated?: boolean; raw: unknown; } +// The finish reasons that mean THE CAP STOPPED IT, across the two first-party mappings: anthropic's +// `stop_reason: 'max_tokens'` and openai's `finish_reason: 'length'`. Compared lower-cased so a BYO +// provider passing its vendor's string through verbatim is not defeated by case alone (Gemini-family +// APIs shout `MAX_TOKENS`) — a cheap widening that can only ever recognise MORE truncation. +const CAP_REASONS = new Set(['max_tokens', 'length']); + +// Read truncation off the NORMALISED finish reason. `undefined` in ⇒ `undefined` out: no reason +// lifted is UNKNOWN, never "fine" (see LlmResult.truncated). +const truncatedBy = (finishReason?: string): boolean | undefined => + finishReason === undefined + ? undefined + : CAP_REASONS.has(finishReason.toLowerCase()); + +// The `warn` finding for a completion the cap cut short (issue #699). Non-fatal by construction — +// only `level: 'error'` fails a call — because whether a partial answer is acceptable is the +// CALLER's call, not the surface's. The surface's whole job here is to make sure the caller is in a +// position to make it, which it was not while a truncated completion resolved `ok: true` with an +// empty `findings` array on all eight observers. +// +// It reuses the `coerced` change kind rather than minting one, following `flagFinding`'s precedent: +// a new kind would widen `SoftDriftChange`, the per-kind severity map and its documented defaults +// for a diagnostic that reads the same either way. `coerced` is the honest fit of the three soft +// kinds — the value that reached you is not the value that was meant — and its default level is +// already `warn`, so kind and severity agree instead of arguing. +const truncationFinding = (reason: string | undefined): DriftFinding => ({ + level: 'warn', + path: 'finishReason', + change: 'coerced', + detail: + `the completion stopped at the token cap (\`${reason}\`), so \`text\` is a PARTIAL answer, ` + + `not a short one. Raise \`tokens\`, or branch on \`truncated\` on the result.`, +}); + /** * The provider-mapping CONTRACT (ADR 0008) — the contract-not-dependency primitive for LLMs, the * same shape the fingerprint contract and `axiosAdapter` follow. A provider declares its url + @@ -77,8 +128,10 @@ export interface LlmProvider { parse: (body: unknown) => LlmResult; } -/** Per-stitch llm defaults baked into the surface (the call may override via `body`). */ -interface LlmDefaults { +/** Per-stitch llm defaults baked into the surface (the call may override via `body`). Exported + * alongside {@link makeLlmSurface}, whose argument it is — a public factory taking a private + * parameter type is not actually constructible by a consumer. */ +export interface LlmDefaults { provider: LlmProvider; model?: string; system?: string; @@ -116,9 +169,37 @@ function toRequest(d: LlmDefaults, input: StitchInput): LlmRequest { */ export const llmSurface: Surface = { id: 'llm' }; -// The live llm surface for one provider + defaults: pack the request via `provider.buildBody` as a -// JSON POST (provider headers under the user's), and `interpret` lifts the result via `provider.parse`. -// +/** + * The live llm surface for one provider + defaults: pack the request via `provider.buildBody` as a + * JSON POST (provider headers under the user's), and `interpret` lifts the result via + * `provider.parse`. + * + * EXPORTED (issue #699) because {@link llmSurface} — the identity — has nothing to wrap: it is a + * bare `{ id: 'llm' }`, so a caller who wanted to layer behaviour over the real llm surface had no + * object to layer it over and no way to build one, since this factory closes over the provider and + * defaults that `llm(config)` assembles internally. Wrapping a surface is the documented way to + * extend one (CONTRACT.md P21 — `Surface` is the seam), and that door was shut on this surface + * alone. With the factory public, composing over `interpret`/`buildRequest` is ordinary code: + * + * ```ts + * import { makeLlmSurface, anthropic } from 'stitchapi/llm'; + * import { stitch } from 'stitchapi'; + * + * const base = makeLlmSurface({ provider: anthropic, model: 'claude-opus-4-8' }); + * const strict = { + * ...base, + * interpret: (res, cfg) => { + * const out = base.interpret!(res, cfg); + * return out.ok && out.data.truncated + * ? { ok: false as const, message: 'llm: truncated at the token cap' } + * : out; + * }, + * }; + * ``` + * + * That example is deliberate: making truncation FATAL is a caller-side policy this surface does not + * impose, and exporting the factory is what makes it a five-line wrapper instead of a fork. + */ // `method` and the body encoding are the surface's, not the caller's — `NoRequestShapeOnLlm` makes // authoring `method` or `wire.body` a compile error so the override is never silent. `headers` and // `wire.response` are NOT overridden (base headers win over the provider's; the response decoding @@ -127,7 +208,9 @@ export const llmSurface: Surface = { id: 'llm' }; // The flat `bodyType: 'json'` below is the `AdapterRequest` spelling, one layer under the authoring // config: that transport contract keeps the flat wire-format fields (CONTRACT.md P22), and the // engine converts `wire` into them when it builds the request. -function makeLlmSurface(d: LlmDefaults): Surface { +export function makeLlmSurface( + d: LlmDefaults, +): Surface { const { provider } = d; return { id: 'llm', @@ -142,10 +225,35 @@ function makeLlmSurface(d: LlmDefaults): Surface { // is what keeps `parse` seeing a successful body now that the engine no longer guarantees a // non-2xx never reaches here. A provider's "200 with an error envelope" is still `parse`'s // to handle. + // + // A completion that hit the token cap is then REPORTED, not thrown (issue #699). The + // provider mappings already lifted `finishReason`; until now nothing read it, so the one + // response shape a caller most needs to notice — "your answer is cut off" — arrived as + // `ok: true` with `findings: []`, indistinguishable from a model that simply finished. It + // resolves as a success because it IS one at every layer this surface owns (the transport + // succeeded, the body is well-formed, `text` holds real tokens); whether a partial answer + // is usable is the caller's question, and `truncated` + the `warn` finding are what let it + // be asked. Failing the call instead would be a policy — see {@link makeLlmSurface} for the + // wrapper that adopts it. interpret: (res, cfg): SurfaceOutcome => { const failure = verdictOf(res, cfg); if (failure) return failure; - return { ok: true, data: provider.parse(res.body) }; + const parsed = provider.parse(res.body); + // The provider's own answer wins: a BYO provider that already decided this in `parse` + // knows its vendor's stop vocabulary better than a two-entry set does. + const truncated = + parsed.truncated ?? truncatedBy(parsed.finishReason); + // Unknown stays ABSENT rather than being written as `false` — `compact`'s discipline, + // and the reason `truncated` is three-state at all. + const data = + truncated === undefined ? parsed : { ...parsed, truncated }; + return truncated === true + ? { + ok: true, + data, + findings: [truncationFinding(parsed.finishReason)], + } + : { ok: true, data }; }, }; } diff --git a/packages/core/src/surface.ts b/packages/core/src/surface.ts index 6a30b492..06c8075b 100644 --- a/packages/core/src/surface.ts +++ b/packages/core/src/surface.ts @@ -21,7 +21,22 @@ import { getPath } from './util'; /** * The result a surface's {@link Surface.interpret} produces from a buffered response. The success - * arm carries `data` (CONTRACT.md P5). + * arm carries `data` (CONTRACT.md P5), plus optional `findings` — the DIAGNOSTIC channel for a + * surface that noticed something about a response it is nonetheless returning as a success. + * + * `findings` exists because the alternative is a false choice. A surface reading a body can meet a + * condition that is neither a failure nor a non-event — an LLM completion cut short at the token + * cap is the motivating case (issue #699): the call worked, the bytes are well-formed, and the + * `text` is still a partial answer. Without a diagnostic channel `interpret` must either fail the + * call (wrong — the caller may not care, and it is the caller's decision to make) or say nothing + * (wrong — the caller cannot make that decision if it never hears about it). The engine merges + * these into the same drift stream the `output` contract and {@link flagFinding} feed, so they + * reach `.inspect().findings`, the `drift` event, and the trace with no extra wiring. + * + * Findings are DIAGNOSTIC, never control flow (ADR 0015/0016) — with one edge the engine already + * owns: a `level: 'error'` finding fails the call. A surface reporting a non-fatal condition + * therefore uses `warn`/`info`/`verbose`, which is the whole point of reporting it here rather + * than in the failure arm. * * The middle arm is the BODY-AWARE RETRY (ADR 0022 Decision 5, issue #529): a surface that has read * the body can ask for another attempt — a `200` carrying `{ status: 'PENDING' }`, an in-payload @@ -32,7 +47,7 @@ import { getPath } from './util'; * author writes it. Exhausting the budget surfaces the outcome as an ordinary failure. */ export type SurfaceOutcome = - | { ok: true; data: T } + | { ok: true; data: T; findings?: DriftFinding[] } | { ok: false; retry: true; message: string; after?: number | string } | { ok: false; message: string; status?: number }; diff --git a/packages/core/test/llm-truncation.spec.ts b/packages/core/test/llm-truncation.spec.ts new file mode 100644 index 00000000..d6e8e6e4 --- /dev/null +++ b/packages/core/test/llm-truncation.spec.ts @@ -0,0 +1,275 @@ +// A truncated completion is no longer a silent success (issue #699). The provider mappings have +// always lifted `finishReason` — anthropic's `stop_reason`, openai's `finish_reason` — and nothing +// read it, so a completion the token cap cut off resolved `ok: true` with an empty `findings` array, +// identical on every observer to a model that finished on its own terms. These pin the two halves of +// the fix: the normalised `truncated` flag on the result, and the `warn` drift finding that carries +// it into `.inspect()` / the drift event / the trace. +// +// Style follows llm.spec.ts — a capturing adapter, no network. `trace: false` keeps the findings +// assertions from writing a trace file. +import { anthropic, llm, makeLlmSurface, openai } from '../src/llm'; +import type { LlmProvider, LlmResult } from '../src/llm'; +import { stitch } from '../src/stitch'; +import type { Adapter, AdapterResponse } from '../src/types'; + +const respond = + (body: unknown): Adapter => + async () => ({ status: 200, headers: {}, body }); + +const ASK = { body: { messages: [{ role: 'user' as const, content: 'hi' }] } }; + +// The two vendor bodies, each carrying the finish reason that means "the cap stopped it". +const anthropicBody = (stop_reason: string) => ({ + content: [{ text: 'the first half of an ans' }], + model: 'claude-opus-4-8', + usage: { input_tokens: 5, output_tokens: 256 }, + stop_reason, +}); +const openaiBody = (finish_reason: string) => ({ + choices: [ + { message: { content: 'the first half of an ans' }, finish_reason }, + ], + model: 'gpt-4o', + usage: { prompt_tokens: 5, completion_tokens: 256 }, +}); + +describe('a completion cut off at the token cap is reported', () => { + test('anthropic `stop_reason: max_tokens` → truncated, with a warn finding', async () => { + const chat = llm({ + provider: anthropic, + model: 'claude-opus-4-8', + adapter: respond(anthropicBody('max_tokens')), + trace: false, + }); + + // 1. On the RESULT — the normalised flag, so a caller never has to know that anthropic + // spells the cap `max_tokens` and openai spells it `length`. + const out = await chat(ASK); + expect(out.truncated).toBe(true); + expect(out.finishReason).toBe('max_tokens'); // the vendor string survives alongside it + expect(out.text).toBe('the first half of an ans'); // the partial answer is still served + + // 2. As a FINDING — the diagnostic channel, so it reaches `.inspect()`, the `drift` event + // and the trace with no extra wiring. + const r = await chat.inspect(ASK); + const finding = r.findings.find((f) => f.path === 'finishReason'); + expect(finding).toBeDefined(); + expect(finding!.level).toBe('warn'); + expect(finding!.detail).toMatch(/token cap/); + // Diagnostic, never control flow: the call still succeeded. + expect(r.error).toBeNull(); + }); + + test('openai `finish_reason: length` → truncated, with a warn finding', async () => { + const chat = llm({ + provider: openai, + model: 'gpt-4o', + adapter: respond(openaiBody('length')), + trace: false, + }); + + const out = await chat(ASK); + expect(out.truncated).toBe(true); + expect(out.finishReason).toBe('length'); + + const r = await chat.inspect(ASK); + expect( + r.findings.some( + (f) => f.path === 'finishReason' && f.level === 'warn', + ), + ).toBe(true); + expect(r.error).toBeNull(); + }); + + // The point of the whole change: it is REPORTED, not thrown. Making it fatal is a caller-side + // policy (see the wrapper test below), deliberately not this surface's default — flipping it + // would be a semver-major behaviour change. + test('truncation does NOT fail the call', async () => { + const chat = llm({ + provider: anthropic, + model: 'claude-opus-4-8', + adapter: respond(anthropicBody('max_tokens')), + trace: false, + }); + await expect(chat(ASK)).resolves.toMatchObject({ truncated: true }); + }); +}); + +describe('a completion that finished on its own terms reports nothing', () => { + test('anthropic `end_turn` → truncated false, no finding', async () => { + const chat = llm({ + provider: anthropic, + model: 'claude-opus-4-8', + adapter: respond(anthropicBody('end_turn')), + trace: false, + }); + + const out = await chat(ASK); + expect(out.truncated).toBe(false); + + const r = await chat.inspect(ASK); + expect(r.findings).toEqual([]); + expect(r.error).toBeNull(); + }); + + test('openai `stop` → truncated false, no finding', async () => { + const chat = llm({ + provider: openai, + model: 'gpt-4o', + adapter: respond(openaiBody('stop')), + trace: false, + }); + + expect((await chat(ASK)).truncated).toBe(false); + expect((await chat.inspect(ASK)).findings).toEqual([]); + }); + + // Three-state, and this is the third: a provider that lifted NO finish reason gave no grounds + // for either answer, so `truncated` is absent rather than a confident `false` — which would be + // the same "silence read as success" this change exists to remove, one level up. + test('no finishReason at all → `truncated` is absent (unknown), not false', async () => { + const chat = llm({ + provider: anthropic, + model: 'claude-opus-4-8', + adapter: respond({ content: [{ text: 'ok' }] }), // no stop_reason + trace: false, + }); + + const out = await chat(ASK); + expect(out.finishReason).toBeUndefined(); + expect('truncated' in out).toBe(false); + expect((await chat.inspect(ASK)).findings).toEqual([]); + }); +}); + +describe('the provider is the extension seam (CONTRACT.md P21)', () => { + // A BYO provider whose stop vocabulary is neither vendor's decides for itself in `parse`, and + // the surface defers rather than overwriting it with a guess from an unfamiliar string. + const byo = (parse: LlmProvider['parse']): LlmProvider => ({ + id: 'byo', + url: 'https://byo.test/v1/complete', + defaultModel: 'byo-1', + buildBody: openai.buildBody, + parse, + }); + + test("a provider's own `truncated: true` wins over an unrecognised finishReason", async () => { + const chat = llm({ + provider: byo((body): LlmResult => ({ + text: 'cut', + finishReason: 'BUDGET_EXHAUSTED', // in neither shipped vocabulary + truncated: true, // ...so the provider says so itself + raw: body, + })), + adapter: respond({}), + trace: false, + }); + + const out = await chat(ASK); + expect(out.truncated).toBe(true); + expect( + (await chat.inspect(ASK)).findings.some( + (f) => f.path === 'finishReason', + ), + ).toBe(true); + }); + + test("a provider's own `truncated: false` wins over a cap-looking finishReason", async () => { + const chat = llm({ + provider: byo((body): LlmResult => ({ + text: 'done', + finishReason: 'length', // looks like openai's cap... + truncated: false, // ...but on THIS API it means something else + raw: body, + })), + adapter: respond({}), + trace: false, + }); + + expect((await chat(ASK)).truncated).toBe(false); + expect((await chat.inspect(ASK)).findings).toEqual([]); + }); + + // A BYO provider that only lifts a finishReason still gets the signal free, as long as it + // passes one of the two shipped spellings through. Case is not load-bearing (Gemini-family + // APIs shout `MAX_TOKENS`). + test('a BYO provider gets truncation free from a shipped spelling, case-insensitively', async () => { + const chat = llm({ + provider: byo((body): LlmResult => ({ + text: 'cut', + finishReason: 'MAX_TOKENS', + raw: body, + })), + adapter: respond({}), + trace: false, + }); + + expect((await chat(ASK)).truncated).toBe(true); + }); +}); + +// `makeLlmSurface` was the unblocker (issue #699 §3): the exported `llmSurface` is a bare +// `{ id: 'llm' }` identity with nothing to wrap, and the real factory — which closes over the +// provider and defaults — had no `export`, so a caller could not implement any of this themselves. +// This pins that it is importable, that it builds a working surface, and that wrapping it is +// ordinary code: the "make truncation fatal" policy this surface deliberately does NOT adopt is a +// few lines on the caller's side. +describe('makeLlmSurface is exported and wrappable', () => { + test('is importable and builds a live surface with the llm identity', () => { + expect(typeof makeLlmSurface).toBe('function'); + const surface = makeLlmSurface({ + provider: anthropic, + model: 'claude-opus-4-8', + }); + expect(surface.id).toBe('llm'); + expect(typeof surface.buildRequest).toBe('function'); + expect(typeof surface.interpret).toBe('function'); + }); + + test('drives a stitch through the generic `kind` slot', async () => { + const surface = makeLlmSurface({ + provider: anthropic, + model: 'claude-opus-4-8', + }); + const chat = stitch({ + kind: surface, + url: anthropic.url, + adapter: respond(anthropicBody('end_turn')), + trace: false, + }); + + const out = await chat(ASK); + expect(out.text).toBe('the first half of an ans'); + expect(out.truncated).toBe(false); + }); + + test('a wrapper can make truncation FATAL — the opt-in this PR leaves to the caller', async () => { + const base = makeLlmSurface({ + provider: anthropic, + model: 'claude-opus-4-8', + }); + const strict = { + ...base, + interpret: ( + res: AdapterResponse, + cfg: Parameters>[1], + ) => { + const out = base.interpret!(res, cfg); + return out.ok && out.data.truncated + ? { + ok: false as const, + message: 'llm: truncated at the token cap', + } + : out; + }, + }; + const chat = stitch({ + kind: strict, + url: anthropic.url, + adapter: respond(anthropicBody('max_tokens')), + trace: false, + }); + + await expect(chat(ASK)).rejects.toThrow(/truncated/); + }); +});