From 1bd1af2f48041dad1266f537c9970e2990a14d02 Mon Sep 17 00:00:00 2001 From: backryun Date: Sun, 26 Jul 2026 15:51:45 +0900 Subject: [PATCH] refactor(sse): narrow three result unions via type predicates (#8499) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(sse): narrow three result unions via type predicates Eight diagnostics across three executors, all the same root cause already recorded in #8483: this workspace compiles with `strictNullChecks: false`, where a boolean-literal discriminant narrows the positive branch but not the negative one. Reading a failure-only field after `!result.ok` therefore leaves the full union. auggie.ts 2 .error on AuggieModelResolution muse-spark-web 4 .error on GraphqlResult notion-web 2 .retryable / .errorResult on the runOnce union #8483 retagged its union with a string discriminant. That is the better shape when the union is module-private and small, but it does not fit here: `resolveAuggieModel` is exported and its tests deep-equal the literal `{ ok: true, model }` object, so retagging would churn public API and assertions to fix a checker limitation. Each union instead gets an explicit type predicate, which narrows correctly under these compiler settings while leaving the shape, every call site, and the tests untouched. notion-web's inline union is named `NotionAttempt` first so it has something to `Extract` from. Validation: full tsc error-set diff against the base config — 335 -> 327, zero new errors (line-number-agnostic). `typecheck:core` clean; the 6 existing test files importing a touched executor pass. Coverage: each predicate is a one-liner whose control flow inverts on a stray `!`, and all three failure branches already have behavioral guards — `auggie-executor.test.ts` (400 + /Unknown Auggie model/), `muse-spark-web-continuation.test.ts` ("Warmup failed: …"), and `executor-notion-web.test.ts` (nested temporarily-unavailable → retried). The two assertions added here pin the arm that the predicate unlocks on the one union that is exported and directly reachable. * chore(quality): rebaseline muse-spark-web.ts for #8499 own growth The new isGraphqlFailure() type-predicate helper (TS7 strictNullChecks:false narrowing fix) grows the frozen file 1396->1405 (+9), irreducible per the justification recorded in config/quality/file-size-baseline.json. Co-authored-by: diegosouzapw --------- Co-authored-by: ikelvingo Co-authored-by: diegosouzapw --- config/quality/file-size-baseline.json | 3 ++- open-sse/executors/auggie.ts | 15 +++++++++++++- open-sse/executors/muse-spark-web.ts | 13 ++++++++++-- open-sse/executors/notion-web.ts | 25 +++++++++++++++++------ tests/unit/auggie-executor.test.ts | 28 ++++++++++++++++++++++++++ 5 files changed, 74 insertions(+), 10 deletions(-) diff --git a/config/quality/file-size-baseline.json b/config/quality/file-size-baseline.json index a76747350b..a0e0c33518 100644 --- a/config/quality/file-size-baseline.json +++ b/config/quality/file-size-baseline.json @@ -1,4 +1,5 @@ { + "_rebaseline_2026_07_25_8499_ts7_result_union_predicates": "PR #8499 (backryun, chore/ts7-types-executor-scattered) own growth: muse-spark-web.ts 1396->1405 (+9, irreducible). Under this workspace's `strictNullChecks: false`, the boolean-literal discriminant on `GraphqlResult` (`{ ok: true } | { ok: false; error: string }`) narrows the positive `.ok===true` branch but leaves `!result.ok` at the full union under TS7, making `.error` unreachable to the checker at the two call sites (warmup, mode-switch). Fixed by adding a single `isGraphqlFailure()` type-predicate helper (doc comment + 3-line body) reused at both call sites instead of duplicating the predicate inline — not extractable to a shared module without splitting a single-file executor's local narrowing helper out of its own file. Covered by the existing muse-spark-web executor test suite (no behavior change, pure narrowing fix).", "_rebaseline_2026_07_22_8131_windowshide_cloudflared_spawn": "PR #8167 (Dingding-leo, fix/windows-hide-child-process, #8131) own growth: src/lib/cloudflaredTunnel.ts 934->935 (+1, irreducible call-site wiring — the single `windowsHide: true` option added to the existing cloudflared spawn() options object so no transient conhost.exe/cmd console window flashes open on Windows). Covered by the pre-merge-fix regression test tests/unit/windows-hide-child-process-spawns-8131.test.ts (added for the two additional spawn() sites the PR missed: ServiceSupervisor.ts, versionManager/processManager.ts) plus the windowsHide assertion added to tests/unit/services/installers/runNpm-shell-5379.test.ts (installers/utils.ts buildNpmExecOptions).", "_rebaseline_2026_07_22_8006_adobe_firefly_media_provider": "PR #8006 (artickc, feat/adobe-firefly-media) own growth: adds Adobe Firefly as a media-only (image + video) provider — unofficial IMS/cookie-session bridge for firefly.adobe.com covering IMS cookie->access_token exchange, discovery-catalog fallback, credits/balance usage, and submit+poll dispatch for both image (nano-banana/gpt-image families) and video (Sora 2/Veo 3.1/Kling 3.0) generation, with 408-under-load retry handling. New leaf open-sse/services/adobeFireflyClient.ts frozen at 1958 (>>cap 800) — a single self-contained upstream client (mirrors the qoderCli.ts precedent for a new provider client that is legitimately large on day one: IMS auth, cookie/JWT normalization, payload builders for 2 media types x multiple model families, SSE-less submit/poll state machine, error sanitization); not extractable without scattering a single upstream integration across artificial module boundaries mid-PR. open-sse/config/imageRegistry.ts (existing, previously under cap) grows 800->821 (+21, the new adobe-firefly IMAGE_PROVIDERS entry + models list, additive registry data at the existing registry chokepoint). src/lib/usage/providerLimits.ts 1000->1003 (+3, adobe-firefly/firefly added to the existing apikey-usage-fetcher allowlist, irreducible call-site wiring mirroring the sibling #7994 PromptQL/HyperAgent entries in the same PR group). Covered by tests/unit/adobe-firefly.test.ts (35/35). Structural shrink tracked in #3501.", "_rebaseline_2026_07_22_7994_hyperagent_web_provider": "PR #7994 (artickc, feat/hyperagent-web) own growth: adds HyperAgent (hyperagent.com) as a new unofficial web-cookie chat provider, reverse-engineered from live SPA captures (thread/session SSE flow, credits/usage endpoint). New leaf open-sse/executors/hyperagent.ts frozen at 937 (>cap 800) — single self-contained executor covering cookie auth, SSE parsing (text/session_start/session_end/done events), and a sticky thread/session cache for multi-turn continuity; not extractable without splitting the executor mid-request-flow (mirrors the sseParser.ts/muse-spark-web.ts precedent for new provider executors that exceed cap on day one). src/lib/usage/providerLimits.ts 1000->1003 (+3, irreducible call-site wiring adding hyperagent/ha to the existing USAGE_FETCHER_PROVIDERS-style allowlist at the chokepoint other web-cookie providers already extend). Covered by tests/unit/executor-hyperagent.test.ts (16/16). Structural shrink tracked in #3501.", @@ -175,7 +176,7 @@ "open-sse/executors/duckduckgo-web.ts": 925, "open-sse/executors/grok-web.ts": 1873, "open-sse/executors/hyperagent.ts": 937, - "open-sse/executors/muse-spark-web.ts": 1396, + "open-sse/executors/muse-spark-web.ts": 1405, "open-sse/executors/perplexity-web.ts": 1032, "open-sse/handlers/audioSpeech.ts": 1061, "open-sse/handlers/chatCore.ts": 5125, diff --git a/open-sse/executors/auggie.ts b/open-sse/executors/auggie.ts index f3c98c8d8f..6443f73162 100644 --- a/open-sse/executors/auggie.ts +++ b/open-sse/executors/auggie.ts @@ -149,6 +149,19 @@ export async function initAuggieModels( type AuggieModelResolution = { ok: true; model: string } | { ok: false; error: string }; +/** + * This workspace compiles with `strictNullChecks: false`, where a boolean-literal + * discriminant narrows the positive branch but not the negative one — so `!r.ok` alone + * leaves `r` as the full union and reading `.error` fails. An explicit type predicate + * narrows under those settings without retagging the union (which is public API here: + * `resolveAuggieModel` is exported and its tests deep-equal the `{ ok: true, ... }` shape). + */ +function isAuggieModelFailure( + resolution: AuggieModelResolution +): resolution is Extract { + return !resolution.ok; +} + /** * Validate + resolve the requested model against the registry allowlist. * Rejects flag-smuggling (leading "-") and any id not declared in the registry. @@ -384,7 +397,7 @@ export class AuggieExecutor extends BaseExecutor { await initAuggieModels(signal); // Argument-injection defense: never forward an unvalidated model into the argv. const modelResolution = resolveAuggieModel(model); - if (!modelResolution.ok) { + if (isAuggieModelFailure(modelResolution)) { const response = wantsStream ? buildAuggieSseError(modelResolution.error) : errorResponse(400, modelResolution.error); diff --git a/open-sse/executors/muse-spark-web.ts b/open-sse/executors/muse-spark-web.ts index 8c7d9617ff..66a2e819ef 100644 --- a/open-sse/executors/muse-spark-web.ts +++ b/open-sse/executors/muse-spark-web.ts @@ -906,6 +906,15 @@ function buildWsUrl(authorization: string, requestId: string): string { type GraphqlResult = { ok: true } | { ok: false; error: string }; +/** + * Narrows the failure arm. Under this workspace's `strictNullChecks: false`, a + * boolean-literal discriminant narrows the positive branch but not the negative one, so + * `!result.ok` leaves the full union and `.error` is unreachable to the checker. + */ +function isGraphqlFailure(result: GraphqlResult): result is Extract { + return !result.ok; +} + async function graphqlPost( docId: string, variables: Record, @@ -1315,7 +1324,7 @@ export class MuseSparkWebExecutor extends BaseExecutor { "Warmup", signal ); - if (!warmupResult.ok) { + if (isGraphqlFailure(warmupResult)) { evictContinuationIfNeeded(cached, continuationCacheKey); log?.error?.("MUSE-SPARK-WEB", `Warmup failed: ${warmupResult.error}`); return errorResult(502, warmupResult.error, "meta_ai_warmup_failed", {}, body); @@ -1329,7 +1338,7 @@ export class MuseSparkWebExecutor extends BaseExecutor { "Mode switch", signal ); - if (!modeResult.ok) { + if (isGraphqlFailure(modeResult)) { evictContinuationIfNeeded(cached, continuationCacheKey); log?.error?.("MUSE-SPARK-WEB", `Mode switch failed: ${modeResult.error}`); return errorResult(502, modeResult.error, "meta_ai_mode_switch_failed", {}, body); diff --git a/open-sse/executors/notion-web.ts b/open-sse/executors/notion-web.ts index 19bd0f3026..57ca5b4e96 100644 --- a/open-sse/executors/notion-web.ts +++ b/open-sse/executors/notion-web.ts @@ -606,13 +606,26 @@ export class NotionWebExecutor extends BaseExecutor { const reqHeaders = buildNotionExecuteHeaders({ cookie, spaceId, userId, agent }); + type NotionAttempt = + | { ok: true; finalText: string; reqBody: Record } + | { + ok: false; + errorResult: ReturnType; + retryable: boolean; + reqBody: Record; + }; + + // `strictNullChecks: false` narrows a boolean-literal discriminant on the positive + // branch only, so `!attempt.ok` leaves the full union and the failure-only fields are + // unreachable to the checker. An explicit predicate narrows under those settings. + const isFailedAttempt = ( + attempt: NotionAttempt + ): attempt is Extract => !attempt.ok; + const runOnce = async (opts: { createThread: boolean; threadId: string; - }): Promise< - | { ok: true; finalText: string; reqBody: Record } - | { ok: false; errorResult: ReturnType; retryable: boolean; reqBody: Record } - > => { + }): Promise => { const transcript = buildNotionTranscript(messages, { notionModel: notionCodename || undefined, spaceId, @@ -681,13 +694,13 @@ export class NotionWebExecutor extends BaseExecutor { let attempt = await runOnce({ createThread, threadId }); // One automatic retry for transient Notion faults — same threadId, never create again - if (!attempt.ok && attempt.retryable) { + if (isFailedAttempt(attempt) && attempt.retryable) { const delayMs = process.env.NODE_ENV === "test" || process.env.VITEST ? 20 : 700 + Math.floor(Math.random() * 400); await new Promise((r) => setTimeout(r, delayMs)); attempt = await runOnce({ createThread: false, threadId }); } - if (!attempt.ok) { + if (isFailedAttempt(attempt)) { return attempt.errorResult; } diff --git a/tests/unit/auggie-executor.test.ts b/tests/unit/auggie-executor.test.ts index f8fcd25ea5..b44c3ec94f 100644 --- a/tests/unit/auggie-executor.test.ts +++ b/tests/unit/auggie-executor.test.ts @@ -293,6 +293,34 @@ test("resolveAuggieModel resolves every pre-v0.32.0 alias to an allowlisted v0.3 } }); +// The failure arm is read through an `isAuggieModelFailure()` type predicate — this +// workspace compiles with `strictNullChecks: false`, where `!result.ok` narrows the +// success branch but not the failure one, so `.error` was unreachable to the checker. +// The predicate is a one-liner whose control flow inverts on a stray `!`, so pin both +// arms: the failure arm must carry a usable message, the success arm must not claim one. +test("resolveAuggieModel's failure arm carries a readable error message", () => { + const result = resolveAuggieModel("totally-not-a-real-model"); + assert.equal(result.ok, false); + if (!result.ok) { + assert.equal(typeof result.error, "string"); + assert.ok(result.error.length > 0, "the rejection must explain itself"); + assert.match(result.error, /Unknown Auggie model/); + } +}); + +test("resolveAuggieModel's success arm resolves a model and reports no error", () => { + const result = resolveAuggieModel("haiku4.5"); + assert.equal(result.ok, true); + if (result.ok) { + assert.equal(result.model, "haiku4.5"); + assert.equal( + (result as unknown as { error?: unknown }).error, + undefined, + "the success arm must not carry a failure message" + ); + } +}); + // ─── execute(): model allowlist (argument-injection defense) ────────────── test("execute() rejects a model not in the registry allowlist and never spawns", async () => {