From d6281055031a9177ac052b91396fec8662fb6932 Mon Sep 17 00:00:00 2001 From: backryun Date: Sun, 26 Jul 2026 15:52:00 +0900 Subject: [PATCH] refactor(sse): retag the designer-web result unions with string discriminants (#8531) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All 10 diagnostics in this file are the `strictNullChecks: false` narrowing limitation: a boolean-literal discriminant narrows the positive branch but leaves the negative one as the full union, so `if (!resolved.ok)` and the code after `if (outcome.success)` could not see `status`/`error`. Three unions, all module-private and untouched by any test, so per the rule recorded on #8499 these are retagged rather than fixed with predicates — predicates are for exported unions whose shape callers depend on: resolveDesignerWebRequest ok: true|false -> state: "resolved"|"invalid" DesignerWebStepResult done + success -> state: "pending"|"ready"|"failed" The step union collapsed two booleans into one discriminant; `done`/`success` encoded three states across two flags, which is also why the pending arm had no `success` property for `outcome.success` to read. Also narrowed runDesignerWebPollLoop's declared return from `DesignerWebStepResult | {…504…}` to a new DesignerWebOutcome (ready | failed). The loop returns a step only after confirming it is terminal, and otherwise synthesizes a 504 — it can never return a pending step, and the old signature claiming it could is what made `.success` unreadable on the union at all. 280 -> 270, zero new, on a line-number-agnostic diff of the full tsc error set. Tests: unlike the previous slices this rewrote real control flow (three conditionals), so the existing suite is doing actual work here — microsoft-designer-web-6672.test.ts drives the handler end-to-end through 400, 401, immediate-ready, poll-then-ready, non-OK upstream and 504-timeout, i.e. every arm but one. The "empty" arm (unrecognized 200 body -> terminal 502) was tested only at the parser level, never through the handler, so the 502 itself was unasserted. Added designer-web-empty-response-502.test.ts (2 tests) pinning that it is terminal (exactly one fetch, no polling) and distinct from the 504 deadline path. Both suites pass against the parent commit too — the tests are black-box through the exported handler, so they are agnostic to the discriminant rename and prove the retag is behavior-preserving. 61/61 across the 3 suites. Co-authored-by: backryun --- .../imageGeneration/providers/designerWeb.ts | 76 ++++++++++++++----- .../designer-web-empty-response-502.test.ts | 67 ++++++++++++++++ 2 files changed, 123 insertions(+), 20 deletions(-) create mode 100644 tests/unit/designer-web-empty-response-502.test.ts diff --git a/open-sse/handlers/imageGeneration/providers/designerWeb.ts b/open-sse/handlers/imageGeneration/providers/designerWeb.ts index f32d478d36..f362eecd14 100644 --- a/open-sse/handlers/imageGeneration/providers/designerWeb.ts +++ b/open-sse/handlers/imageGeneration/providers/designerWeb.ts @@ -104,15 +104,26 @@ interface DesignerWebRequestConfig { pollIntervalMs: number; } +/** + * Outcome of request validation. String-discriminated rather than `ok: boolean` + * because `open-sse` compiles with `strictNullChecks: false`, where a + * boolean-literal discriminant narrows the positive branch but leaves the + * negative one as the full union — so `if (!resolved.ok)` would not expose + * `status`/`error`. All three unions in this file shared that root cause. + */ +type DesignerWebRequestResolution = + | { state: "resolved"; config: DesignerWebRequestConfig } + | { state: "invalid"; status: number; error: string }; + /** Validates the request and resolves auth + poll timing. Returns an error status/message on failure. */ function resolveDesignerWebRequest( body: { prompt?: unknown; size?: unknown; timeout_ms?: unknown; poll_interval_ms?: unknown }, credentials: { apiKey?: string; accessToken?: string } -): { ok: true; config: DesignerWebRequestConfig } | { ok: false; status: number; error: string } { +): DesignerWebRequestResolution { const prompt = typeof body.prompt === "string" ? body.prompt.trim() : ""; if (!prompt) { return { - ok: false, + state: "invalid", status: 400, error: "Prompt is required for Microsoft Designer image generation", }; @@ -120,7 +131,11 @@ function resolveDesignerWebRequest( const accessToken = credentials?.apiKey || credentials?.accessToken; if (!accessToken) { - return { ok: false, status: 401, error: "Microsoft Designer credentials missing access_token" }; + return { + state: "invalid", + status: 401, + error: "Microsoft Designer credentials missing access_token", + }; } const timeoutMs = normalizePositiveNumber( @@ -139,7 +154,7 @@ function resolveDesignerWebRequest( ); return { - ok: true, + state: "resolved", config: { prompt, accessToken, @@ -151,10 +166,20 @@ function resolveDesignerWebRequest( }; } -type DesignerWebStepResult = - | { done: false; waitMs: number } - | { done: true; success: true; imageUrls: string[] } - | { done: true; success: false; status: number; error: string }; +type DesignerWebPending = { state: "pending"; waitMs: number }; +type DesignerWebReady = { state: "ready"; imageUrls: string[] }; +type DesignerWebFailed = { state: "failed"; status: number; error: string }; + +/** One poll cycle: still working, finished with images, or finished with an error. */ +type DesignerWebStepResult = DesignerWebPending | DesignerWebReady | DesignerWebFailed; + +/** + * What the poll loop hands back. Deliberately excludes the pending arm — the + * loop either returns a terminal step or synthesizes a 504, and never surfaces + * `pending` to its caller. The previous signature admitted it, which is why + * `outcome.success` did not exist on every member of that union. + */ +type DesignerWebOutcome = DesignerWebReady | DesignerWebFailed; /** Runs one submit/poll fetch cycle and classifies the outcome. */ async function stepDesignerWebPoll( @@ -167,23 +192,29 @@ async function stepDesignerWebPoll( const resp = await fetchImpl(baseUrl, { method: "POST", headers, body: formBody }); if (!resp.ok) { - return { done: true, success: false, status: resp.status, error: sanitizeErrorMessage(await resp.text()) }; + return { + state: "failed", + status: resp.status, + error: sanitizeErrorMessage(await resp.text()), + }; } const parsed = parseDesignerWebResponse(await resp.json()); if (parsed.status === "ready") { - return { done: true, success: true, imageUrls: parsed.imageUrls }; + return { state: "ready", imageUrls: parsed.imageUrls }; } if (parsed.status === "empty") { return { - done: true, - success: false, + state: "failed", status: 502, error: "Microsoft Designer response did not contain image data or polling metadata", }; } - return { done: false, waitMs: Math.min(parsed.pollIntervalMs ?? pollIntervalMs, pollIntervalMs) }; + return { + state: "pending", + waitMs: Math.min(parsed.pollIntervalMs ?? pollIntervalMs, pollIntervalMs), + }; } /** Drives the submit-then-poll loop to completion, timeout, or a terminal error. */ @@ -192,7 +223,7 @@ async function runDesignerWebPollLoop( config: DesignerWebRequestConfig, fetchImpl: typeof fetch, log?: { info?: (...args: unknown[]) => void } -): Promise { +): Promise { const deadline = Date.now() + config.timeoutMs; let attempt = 0; @@ -205,14 +236,13 @@ async function runDesignerWebPollLoop( config.pollIntervalMs, fetchImpl ); - if (step.done) return step; + if (step.state !== "pending") return step; log?.info?.("IMAGE", `designer-web pending, poll #${attempt} in ${step.waitMs}ms`); await new Promise((resolve) => setTimeout(resolve, step.waitMs)); } return { - done: true, - success: false, + state: "failed", status: 504, error: "Microsoft Designer image generation timed out waiting for a result", }; @@ -237,13 +267,19 @@ export async function handleDesignerWebImageGeneration({ }) { const startTime = Date.now(); const resolved = resolveDesignerWebRequest(body, credentials); - if (!resolved.ok) { - return saveImageErrorResult({ provider, model, status: resolved.status, startTime, error: resolved.error }); + if (resolved.state === "invalid") { + return saveImageErrorResult({ + provider, + model, + status: resolved.status, + startTime, + error: resolved.error, + }); } try { const outcome = await runDesignerWebPollLoop(providerConfig.baseUrl, resolved.config, fetchImpl, log); - if (outcome.success) { + if (outcome.state === "ready") { return saveImageSuccessResult({ provider, model, diff --git a/tests/unit/designer-web-empty-response-502.test.ts b/tests/unit/designer-web-empty-response-502.test.ts new file mode 100644 index 0000000000..32df360c50 --- /dev/null +++ b/tests/unit/designer-web-empty-response-502.test.ts @@ -0,0 +1,67 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { handleDesignerWebImageGeneration } = await import( + "../../open-sse/handlers/imageGeneration/providers/designerWeb.ts" +); + +/** + * `stepDesignerWebPoll` classifies an unrecognized upstream body as a terminal + * 502 — the "empty" arm of the step union, alongside pending / ready / upstream + * failure. + * + * `microsoft-designer-web-6672.test.ts` covers every other arm end-to-end + * (400 no prompt, 401 no token, immediate ready, poll-then-ready, non-OK + * upstream, 504 timeout) but tests "empty" only at the parser level + * (`parseDesignerWebResponse: unrecognized shape is 'empty'`) — it never drives + * the handler with one, so the 502 the handler synthesizes from it was + * unasserted. + */ + +function jsonResponse(status: number, body: unknown) { + return { + ok: status >= 200 && status < 300, + status, + json: async () => body, + text: async () => JSON.stringify(body), + } as Response; +} + +const BASE = { + model: "dall-e-3", + provider: "microsoft-designer-web", + providerConfig: { baseUrl: "https://designerapp.officeapps.live.com/designerapp/DallE.ashx" }, + credentials: { apiKey: "tok-abc" }, +}; + +test("a 200 with an unrecognized body is a terminal 502, not a retry", async () => { + let calls = 0; + const result = await handleDesignerWebImageGeneration({ + ...BASE, + body: { prompt: "a cat astronaut", timeout_ms: 5_000, poll_interval_ms: 1 }, + fetchImpl: async () => { + calls += 1; + return jsonResponse(200, { unexpected: true }); + }, + }); + + assert.equal(result.success, false); + assert.equal(result.status, 502, "an unparseable success body is a bad-gateway, not a timeout"); + assert.match(String(result.error), /did not contain image data or polling metadata/); + assert.equal(calls, 1, "the empty classification is terminal — it must not keep polling"); +}); + +test("a 200 with neither images nor polling metadata does not fall through to 504", async () => { + // The distinction matters: 502 says "the upstream answered with something we + // cannot use", 504 says "the upstream never finished". A timeout_ms generous + // enough to allow several polls proves the 502 came from classification, not + // from the deadline. + const result = await handleDesignerWebImageGeneration({ + ...BASE, + body: { prompt: "a cat astronaut", timeout_ms: 10_000, poll_interval_ms: 1 }, + fetchImpl: async () => jsonResponse(200, { polling_response: {} }), + }); + + assert.equal(result.status, 502); + assert.notEqual(result.status, 504); +});