[PR #206] feat(errors): map sso_required to Forbidden and carry organizationId #209

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/206
Author: @mattmillerai
Created: 10/9/2026
Status: 🔄 Open

Base: main ← Head: matt/be-19943-sso-required-forbidden

📝 Commits (2)

af39985 feat(errors): map sso_required to Forbidden and carry organizationId
345993f fix(errors): read organizationId only on sso_required, and scope the claim

📊 Changes

8 files changed (+189 additions, -4 deletions)

View changed files

📝 CHANGELOG.md (+11 -0)
📝 README.md (+6 -1)
📝 src/low/errors.test.ts (+45 -0)
📝 src/low/errors.ts (+29 -2)
📝 src/sdk/client.test.ts (+25 -0)
📝 src/sdk/exceptions.test.ts (+37 -0)
📝 src/sdk/exceptions.ts (+17 -1)
📝 test/support/stub-server.ts (+19 -0)

📄 Description

ELI-5

When a personal API key belongs to an account whose organization requires single sign-on, the server refuses every call with 403 sso_required and names the organization. The SDK used to turn that into a generic error and drop the organization. Now it raises Forbidden (so an auth catch sees it), keeps code === "sso_required", and exposes the organization as err.organizationId, which is the value needed to start the SSO flow.

What changed

src/low/errors.ts: ApiError gains organizationId: string | null. BY_CODE maps sso_required → Forbidden. errorFromEnvelope reads error.organization_id from the envelope only when the resolved code is sso_required. A missing, non-string, empty or whitespace-only value becomes null. The code lookup goes through Object.hasOwn, so a wire code such as constructor falls back to ApiError.
src/sdk/exceptions.ts: ComfyError gains the same organizationId field, and BY_CODE maps sso_required → Forbidden (also looked up with Object.hasOwn). toSdkError forwards organizationId on both branches (queue_full and generic), so neither path can lose it.
test/support/stub-server.ts: new ServerState.ssoRequired scenario. It refuses every authenticated request with 403 sso_required. When organizationId is null it omits organization_id, as the gateway does when it does not know the org.
Tests: mapping-table rows in both layers, envelope parsing (present / absent / empty / whitespace / organization_id ignored on other codes), inherited-name codes falling back to the bare class, forwarding on both toSdkError branches, and an end-to-end comfy.jobs.get case against the stub server, with an org and with null.
Docs: a CHANGELOG entry under Unreleased → Changed, and a note on sso_required in the README's typed-errors list. Both say which paths cannot surface the code: the bodiless HEAD asset-dedup probe, and comfy.models.* (Router).

Judgment calls:

No new exception class. A subclass would trip the cross-SDK parity gates and give callers nothing extra. surface-parity.test.ts passes unchanged.
CHANGELOG section. The entry goes under Changed, not Fixed or Added, because the class a caller sees is different now. It is not breaking: Forbidden extends ComfyError, so existing catch (e instanceof ComfyError) code still matches.

Provenance

Authored by: agent-work loop
Verified:
- Tests: all green, pnpm test (801 passed, 5 skipped; the skips were already there)
- Lint / format / typecheck: clean (pnpm lint, pnpm format:check, pnpm typecheck)
- Spec drift: OK (pnpm check:spec-drift); build: OK (pnpm build, at the original head)
- Mutation-tested (original head): removing sso_required from the sdk BY_CODE fails 4 of the new tests, including both end-to-end stub-server cases
- Not run locally: pnpm check:sdk-parity (network; this change adds no class or error_type), the shared hygiene workflow, the Node 22/24 CI matrix
Deviations:
- Review round: organizationId now reads only on sso_required; both code maps use Object.hasOwn; the docs no longer claim "every operation"

Residual

Router path not covered. The comfy.models.* (Router) path is untouched. Router failures go through routerErrors and are keyed on error_type, not on the v2 envelope's code. If the gateway also answers Router calls with sso_required, those calls do not yet surface organizationId. Neither the ticket nor the vendored Router contract declares this, so it was left alone.
Gateway message not checked. The stub's message text ("This account signs in with your organization's single sign-on") is copied from the change request. I could not check it against the gateway's own message constant, which is not in this repo. No test asserts on the message.
Gateway response not exercised. The gateway's real 403 sso_required response was not exercised against a live deployment. Verification is against the stub server only.
Python twin pending. The matching change in the Python SDK is tracked separately and is not part of this PR.

_{🔄 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/206 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-19943-sso-required-forbidden` --- ### 📝 Commits (2) - [`af39985`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/af3998500d47adb435999775844c014b36f3cdc0) feat(errors): map sso_required to Forbidden and carry organizationId - [`345993f`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/345993f8fcbad56b0e987b645bcca4fe223799a3) fix(errors): read organizationId only on sso_required, and scope the claim ### 📊 Changes **8 files changed** (+189 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+11 -0) 📝 `README.md` (+6 -1) 📝 `src/low/errors.test.ts` (+45 -0) 📝 `src/low/errors.ts` (+29 -2) 📝 `src/sdk/client.test.ts` (+25 -0) 📝 `src/sdk/exceptions.test.ts` (+37 -0) 📝 `src/sdk/exceptions.ts` (+17 -1) 📝 `test/support/stub-server.ts` (+19 -0) </details> ### 📄 Description ## ELI-5 When a personal API key belongs to an account whose organization requires single sign-on, the server refuses every call with `403 sso_required` and names the organization. The SDK used to turn that into a generic error and drop the organization. Now it raises `Forbidden` (so an auth `catch` sees it), keeps `code === "sso_required"`, and exposes the organization as `err.organizationId`, which is the value needed to start the SSO flow. ## What changed - **`src/low/errors.ts`**: `ApiError` gains `organizationId: string | null`. `BY_CODE` maps `sso_required → Forbidden`. `errorFromEnvelope` reads `error.organization_id` from the envelope only when the resolved code is `sso_required`. A missing, non-string, empty or whitespace-only value becomes `null`. The code lookup goes through `Object.hasOwn`, so a wire code such as `constructor` falls back to `ApiError`. - **`src/sdk/exceptions.ts`**: `ComfyError` gains the same `organizationId` field, and `BY_CODE` maps `sso_required → Forbidden` (also looked up with `Object.hasOwn`). `toSdkError` forwards `organizationId` on both branches (`queue_full` and generic), so neither path can lose it. - **`test/support/stub-server.ts`**: new `ServerState.ssoRequired` scenario. It refuses every authenticated request with `403 sso_required`. When `organizationId` is `null` it omits `organization_id`, as the gateway does when it does not know the org. - **Tests**: mapping-table rows in both layers, envelope parsing (present / absent / empty / whitespace / `organization_id` ignored on other codes), inherited-name codes falling back to the bare class, forwarding on both `toSdkError` branches, and an end-to-end `comfy.jobs.get` case against the stub server, with an org and with `null`. - **Docs**: a CHANGELOG entry under Unreleased → Changed, and a note on `sso_required` in the README's typed-errors list. Both say which paths cannot surface the code: the bodiless `HEAD` asset-dedup probe, and `comfy.models.*` (Router). Judgment calls: - **No new exception class.** A subclass would trip the cross-SDK parity gates and give callers nothing extra. `surface-parity.test.ts` passes unchanged. - **CHANGELOG section.** The entry goes under **Changed**, not Fixed or Added, because the class a caller sees is different now. It is not breaking: `Forbidden` extends `ComfyError`, so existing `catch (e instanceof ComfyError)` code still matches. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: all green, `pnpm test` (801 passed, 5 skipped; the skips were already there) - Lint / format / typecheck: clean (`pnpm lint`, `pnpm format:check`, `pnpm typecheck`) - Spec drift: OK (`pnpm check:spec-drift`); build: OK (`pnpm build`, at the original head) - Mutation-tested (original head): removing `sso_required` from the sdk `BY_CODE` fails 4 of the new tests, including both end-to-end stub-server cases - Not run locally: `pnpm check:sdk-parity` (network; this change adds no class or `error_type`), the shared hygiene workflow, the Node 22/24 CI matrix - **Deviations:** - Review round: `organizationId` now reads only on `sso_required`; both code maps use `Object.hasOwn`; the docs no longer claim "every operation" ## Residual - **Router path not covered.** The `comfy.models.*` (Router) path is untouched. Router failures go through `routerErrors` and are keyed on `error_type`, not on the v2 envelope's `code`. If the gateway also answers Router calls with `sso_required`, those calls do not yet surface `organizationId`. Neither the ticket nor the vendored Router contract declares this, so it was left alone. - **Gateway message not checked.** The stub's message text ("This account signs in with your organization's single sign-on") is copied from the change request. I could not check it against the gateway's own message constant, which is not in this repo. No test asserts on the message. - **Gateway response not exercised.** The gateway's real `403 sso_required` response was not exercised against a live deployment. Verification is against the stub server only. - **Python twin pending.** The matching change in the Python SDK is tracked separately and is not part of this PR. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

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

bsakaguchi referenced this issue

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

[PR #210] feat(models): expose the queued-submit cost estimate as RequestHandle.estimate #210