[PR #191] [MERGED] feat(models): carry refusalSubject on ComfyError so models.run names the refused input/output #193

New issue

Closed

opened 2026-10-31 22:05:33 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:05:33 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/191
Author: @mattmillerai
Created: 9/27/2026
Status: ✅ Merged
Merged: 10/9/2026
Merged by: @mattmillerai

Base: be-17174-refusal-subject ← Head: be-17424-run-refusal-subject

📝 Commits (4)

47c8022 feat(models): carry refusalSubject on ComfyError so models.run names the refused input/output
d9d6595 Merge be-17174-refusal-subject: read a repeated identical refusal subject
392075d fix(models): guard the error-class lookup to own keys, and say what refusalSubject's doc can promise
9f9a1e9 Merge origin/be-17174-refusal-subject into be-17424-run-refusal-subject

📊 Changes

7 files changed (+210 additions, -13 deletions)

View changed files

📝 CHANGELOG.md (+6 -2)
📝 src/sdk/exceptions.test.ts (+9 -0)
📝 src/sdk/exceptions.ts (+25 -0)
📝 src/sdk/models.test.ts (+140 -0)
📝 src/sdk/models.ts (+17 -3)
📝 src/sdk/routerErrors.ts (+10 -6)
📝 test/support/router-stub-server.ts (+3 -2)

📄 Description

STACKED — merging lands on be-17174-refusal-subject (owned by @mattmillerai, PR #187), NOT main. This builds on #187's REFUSAL_SUBJECT_HEADER and subject normalizer; merge #187 first, then this retargets to main.

ELI-5

When a model refuses a request on content-policy grounds, Router can say which part it refused (for example, "the audio output"). The queued submit / subscribe / handle calls already pass that on (#187). comfy.models.run did not: its error had nowhere to put it. After this change the error models.run throws carries it as err.refusalSubject, so a caller can change just that part and try again.

What changed

ComfyError.refusalSubject: string | null (and refusalSubject? on ComfyErrorOptions), on the base class next to retryAfter / idempotencyKey, defaulting to null. It's on the base because models.run raises a refusal as a bare ComfyError (BY_ERROR_TYPE has no content_policy_violation entry). The value passes through unnarrowed, the same as code.
errorFromResponse (src/sdk/models.ts) reads X-Comfy-Refusal-Subject first and falls back to the body's refusal_subject. That is the same order toRouterError uses, and the value goes through the same normalizer: trimmed, and null if blank, not a string, or comma-joined from two different subjects (a repeated header). After #187's latest commit, a header that repeats the same subject still names it. Like #187 and the Python SDK, it isn't gated on code.
readRefusalSubject is now exported from src/sdk/routerErrors.ts, alongside parseRetryAfter, which models.ts already imports from there.
Docs: the RouterError.refusalSubject docstring and #187's CHANGELOG bullet no longer say models.run lacks the subject. There is a new Unreleased / Added entry, and one sentence on the RunOptions.retry docstring where content_policy_violation is listed.
Test support: ScriptedResponse.headers accepts string[], which sends a repeated header line.
errorFromResponse looks up the error class by own property only (Object.hasOwn, as toRouterError does). Before, a server bucket of constructor made models.run throw a String wrapper instead of a ComfyError, dropping refusalSubject, and toString / __proto__ raised a bare TypeError. That lookup predates this PR, but it would have dropped the subject this PR adds, so it is fixed here.

Judgment call: where the normalizer lives

The plan offered two options: export the normalizer from routerErrors.ts, or move it to a small internal module that both files import. I tried the internal module first, and it broke routerErrors.test.ts's ES5 down-level suite. That suite transpiles routerErrors.ts in isolation and fails if the module requires anything, on purpose, because it proves the instanceof chain survives a consumer's ES5 build. So the helper had to stay inside routerErrors.ts, and I exported it the way parseRetryAfter is exported. The side effect: it becomes reachable as routerErrors.readRefusalSubject (and via @comfyorg/sdk/errors), since that module is re-exported whole. The surface-parity check compares error classes, models methods and error_type mappings, not helper functions, so parity is unaffected.

Tests

A new describe("refusalSubject on models.run") block in models.test.ts. Each case drives comfy.models.run(MODEL, {}, { retry: FAST }) against a scripted 400 content_policy_violation and asserts code plus:

header only → "output_audio"
header and body disagree → header wins
body only → "input_text"
neither → null
blank/whitespace header → falls back to the body value; blank header with no body value → null
repeated header, two different subjects → null. This sends two real header lines, not a pre-joined string. I checked separately that Node's writeHead with an array value reaches fetch as Headers.get(...) === "input_image, output_text".
repeated header, same subject (output_audio twice) → "output_audio". This matches #187's normalizer change.
non-string body value (3) → null
unknown well-formed value (input_3d_mesh) → passed through
422 detail[] validation shape → null
models.schema() and models.list().page() failures → refusalSubject === null
bucket constructor / toString / __proto__ with a subject header → still a ComfyError carrying code and refusalSubject. This fails without the own-property guard.

In exceptions.test.ts, new ComfyError("x").refusalSubject === null and the option round-trips.

Mutation check: with refusalSubject hard-coded to null in errorFromResponse, the five cases that expect a value fail. (That count predates the same-subject case added since.) The null-expecting cases pass either way by design.

Residual

Not verified against live Router. The server-side producer of X-Comfy-Refusal-Subject / refusal_subject hasn't shipped to production yet, so production responses don't carry the header. The stub-server tests above are the only verification. Once the producer is live, run a real content_policy_violation through comfy.models.run and confirm err.refusalSubject is populated.
REFUSAL_SUBJECTS and the header name are still not pinned to spec/router-openapi.yaml. The vendored spec doesn't carry them yet. #187's rot guard in router-spec-contract.test.ts flags the sync that brings them in; that sync PR should pin them. Out of scope here.
models.run still raises the ComfyError family rather than RouterError. Switching it is separate backlog work and not attempted here.

Provenance

Authored by: agent-work loop
Verified: (at the head that merges #187's cdb627a)
- pnpm lint, pnpm format:check, pnpm typecheck clean
- pnpm check:spec-drift OK (both checks)
- pnpm test: 812 passed, 5 skipped; pnpm build ok
- pnpm check:sdk-parity OK against the Python SDK's main
- mutation check as above (5 value-expecting cases fail without the fix); the prototype-key case fails with the old bare lookup
Deviations:
- the normalizer is exported from routerErrors.ts (reachable as routerErrors.readRefusalSubject); the internal-module option broke the module's import-free ES5 test (see "Judgment call")
- the pre-existing bare BY_ERROR_TYPE[code] lookup was switched to an own-property one, and the refusalSubject doc says what Router sends rather than claiming discovery paths are always null
- #187's later commits were merged in; conflicts in CHANGELOG.md, the RouterError.refusalSubject doc and exceptions.test.ts kept both sides (#187's queued-surface wording and stampIdempotencyKey tests, plus this branch's models.run subject)

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-typescript-sdk/pull/191 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/27/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `be-17174-refusal-subject` ← **Head:** `be-17424-run-refusal-subject` --- ### 📝 Commits (4) - [`47c8022`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/47c802243c9d73075eee36c96bbf87eb9f9b2065) feat(models): carry refusalSubject on ComfyError so models.run names the refused input/output - [`d9d6595`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/d9d6595fb4d624e975741ea6acd767ad079cbfb1) Merge be-17174-refusal-subject: read a repeated identical refusal subject - [`392075d`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/392075d663232364ebf56500ba435ef8b150c2a4) fix(models): guard the error-class lookup to own keys, and say what refusalSubject's doc can promise - [`9f9a1e9`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/9f9a1e92336583175950f38a754b2bcf460fef0f) Merge origin/be-17174-refusal-subject into be-17424-run-refusal-subject ### 📊 Changes **7 files changed** (+210 additions, -13 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+6 -2) 📝 `src/sdk/exceptions.test.ts` (+9 -0) 📝 `src/sdk/exceptions.ts` (+25 -0) 📝 `src/sdk/models.test.ts` (+140 -0) 📝 `src/sdk/models.ts` (+17 -3) 📝 `src/sdk/routerErrors.ts` (+10 -6) 📝 `test/support/router-stub-server.ts` (+3 -2) </details> ### 📄 Description **STACKED — merging lands on `be-17174-refusal-subject` (owned by @mattmillerai, PR #187), NOT `main`.** This builds on #187's `REFUSAL_SUBJECT_HEADER` and subject normalizer; merge #187 first, then this retargets to `main`. ## ELI-5 When a model refuses a request on content-policy grounds, Router can say *which* part it refused (for example, "the audio output"). The queued `submit` / `subscribe` / `handle` calls already pass that on (#187). `comfy.models.run` did not: its error had nowhere to put it. After this change the error `models.run` throws carries it as `err.refusalSubject`, so a caller can change just that part and try again. ## What changed - **`ComfyError.refusalSubject: string | null`** (and `refusalSubject?` on `ComfyErrorOptions`), on the **base** class next to `retryAfter` / `idempotencyKey`, defaulting to `null`. It's on the base because `models.run` raises a refusal as a bare `ComfyError` (`BY_ERROR_TYPE` has no `content_policy_violation` entry). The value passes through unnarrowed, the same as `code`. - **`errorFromResponse` (`src/sdk/models.ts`)** reads `X-Comfy-Refusal-Subject` first and falls back to the body's `refusal_subject`. That is the same order `toRouterError` uses, and the value goes through the same normalizer: trimmed, and `null` if blank, not a string, or comma-joined from two *different* subjects (a repeated header). After #187's latest commit, a header that repeats the *same* subject still names it. Like #187 and the Python SDK, it isn't gated on `code`. - **`readRefusalSubject` is now exported from `src/sdk/routerErrors.ts`**, alongside `parseRetryAfter`, which `models.ts` already imports from there. - Docs: the `RouterError.refusalSubject` docstring and #187's CHANGELOG bullet no longer say `models.run` lacks the subject. There is a new `Unreleased / Added` entry, and one sentence on the `RunOptions.retry` docstring where `content_policy_violation` is listed. - Test support: `ScriptedResponse.headers` accepts `string[]`, which sends a repeated header line. - **`errorFromResponse` looks up the error class by own property only** (`Object.hasOwn`, as `toRouterError` does). Before, a server bucket of `constructor` made `models.run` throw a `String` wrapper instead of a `ComfyError`, dropping `refusalSubject`, and `toString` / `__proto__` raised a bare `TypeError`. That lookup predates this PR, but it would have dropped the subject this PR adds, so it is fixed here. ## Judgment call: where the normalizer lives The plan offered two options: export the normalizer from `routerErrors.ts`, or move it to a small internal module that both files import. I tried the internal module first, and it broke `routerErrors.test.ts`'s ES5 down-level suite. That suite transpiles `routerErrors.ts` in isolation and fails if the module `require`s anything, on purpose, because it proves the `instanceof` chain survives a consumer's ES5 build. So the helper had to stay inside `routerErrors.ts`, and I exported it the way `parseRetryAfter` is exported. The side effect: it becomes reachable as `routerErrors.readRefusalSubject` (and via `@comfyorg/sdk/errors`), since that module is re-exported whole. The surface-parity check compares error classes, `models` methods and `error_type` mappings, not helper functions, so parity is unaffected. ## Tests A new `describe("refusalSubject on models.run")` block in `models.test.ts`. Each case drives `comfy.models.run(MODEL, {}, { retry: FAST })` against a scripted 400 `content_policy_violation` and asserts `code` plus: - header only → `"output_audio"` - header and body disagree → header wins - body only → `"input_text"` - neither → `null` - blank/whitespace header → falls back to the body value; blank header with no body value → `null` - **repeated header, two different subjects** → `null`. This sends two real header lines, not a pre-joined string. I checked separately that Node's `writeHead` with an array value reaches `fetch` as `Headers.get(...) === "input_image, output_text"`. - **repeated header, same subject** (`output_audio` twice) → `"output_audio"`. This matches #187's normalizer change. - non-string body value (`3`) → `null` - unknown well-formed value (`input_3d_mesh`) → passed through - 422 `detail[]` validation shape → `null` - `models.schema()` and `models.list().page()` failures → `refusalSubject === null` - bucket `constructor` / `toString` / `__proto__` with a subject header → still a `ComfyError` carrying `code` and `refusalSubject`. This fails without the own-property guard. In `exceptions.test.ts`, `new ComfyError("x").refusalSubject === null` and the option round-trips. Mutation check: with `refusalSubject` hard-coded to `null` in `errorFromResponse`, the five cases that expect a value fail. (That count predates the same-subject case added since.) The null-expecting cases pass either way by design. ## Residual - **Not verified against live Router.** The server-side producer of `X-Comfy-Refusal-Subject` / `refusal_subject` hasn't shipped to production yet, so production responses don't carry the header. The stub-server tests above are the only verification. Once the producer is live, run a real `content_policy_violation` through `comfy.models.run` and confirm `err.refusalSubject` is populated. - **`REFUSAL_SUBJECTS` and the header name are still not pinned to `spec/router-openapi.yaml`.** The vendored spec doesn't carry them yet. #187's rot guard in `router-spec-contract.test.ts` flags the sync that brings them in; that sync PR should pin them. Out of scope here. - `models.run` still raises the `ComfyError` family rather than `RouterError`. Switching it is separate backlog work and not attempted here. ## Provenance - **Authored by:** agent-work loop - **Verified:** (at the head that merges #187's `cdb627a`) - `pnpm lint`, `pnpm format:check`, `pnpm typecheck` clean - `pnpm check:spec-drift` OK (both checks) - `pnpm test`: 812 passed, 5 skipped; `pnpm build` ok - `pnpm check:sdk-parity` OK against the Python SDK's `main` - mutation check as above (5 value-expecting cases fail without the fix); the prototype-key case fails with the old bare lookup - **Deviations:** - the normalizer is exported from `routerErrors.ts` (reachable as `routerErrors.readRefusalSubject`); the internal-module option broke the module's import-free ES5 test (see "Judgment call") - the pre-existing bare `BY_ERROR_TYPE[code]` lookup was switched to an own-property one, and the `refusalSubject` doc says what Router sends rather than claiming discovery paths are always `null` - #187's later commits were merged in; conflicts in `CHANGELOG.md`, the `RouterError.refusalSubject` doc and `exceptions.test.ts` kept both sides (#187's queued-surface wording and `stampIdempotencyKey` tests, plus this branch's `models.run` subject) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:05:33 +00:00