[PR #205] feat(errors): map sso_required to Forbidden and carry organization_id #205

New issue

Open

opened 2026-10-31 22:02:19 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:02:19 +00:00

Owner

📋 Pull Request Information

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

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

📝 Commits (3)

5f4e920 feat(errors): map sso_required to Forbidden and carry organization_id
9f802c7 fix(errors): gate and sanitise organization_id; keep Forbidden's bucket on sso_required
1133ad5 Merge remote-tracking branch 'origin/main' into matt/be-19944-sso-required-forbidden

📊 Changes

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

View changed files

📝 CHANGELOG.md (+4 -0)
📝 src/comfy_low/errors.py (+45 -0)
📝 src/comfy_sdk/_errors.py (+12 -0)
📝 src/comfy_sdk/exceptions.py (+19 -2)
📝 src/comfy_sdk/router_exceptions.py (+12 -1)
📝 tests/conftest.py (+32 -0)
📝 tests/test_error_contract.py (+4 -1)
➕ tests/test_sso_required.py (+206 -0)

📄 Description

ELI5

Some Comfy Cloud organizations require their members to sign in through single sign-on. When a personal key is used on one of those accounts, the service refuses it and says which organization to sign in through. Before this change the SDK raised a generic error and dropped that information. Now it raises the usual "access denied" error, so existing error handling still catches it, and the error also carries the organization, so an app can send the user to the right sign-in page.

What changed

comfy_low.errors: ApiError gains an organization_id keyword and attribute. _BY_CODE["sso_required"] = Forbidden. error_from_envelope reads error.organization_id only when the resolved code is sso_required, and passes it through the new clean_organization_id. That keeps the value only if all of it is a bounded, id-shaped token ([A-Za-z0-9._:-]{1,200}). A missing, empty, non-string or malformed value becomes None.
comfy_sdk: ComfyError gains organization_id as a class-level None default, a constructor keyword and an attribute. RouterError.__init__ accepts it, plus an optional code=, and passes both on to ComfyError. _BY_CODE maps "sso_required" to the existing Forbidden, which is the RouterError subclass. to_sdk_error passes organization_id through all three branches: QueueFull, the RouterError branch (the one sso_required takes) and the generic one. On the RouterError branch, error_type is the class's own bucket, so sso_required raises Forbidden with error_type == "forbidden" and code == "sso_required". For every other mapping the bucket already equals the code.
_STAMPED_ATTRIBUTES now includes organization_id. tests/test_error_contract.py requires every caller-readable ComfyError attribute to be in this list. Without it, exc.organization_id on a stamped httpx.ConnectError or CancelledError would raise AttributeError instead of returning None. _BASE_ATTRIBUTES in that test is extended to match.
Stub server (tests/conftest.py): a new sso_required knob, named after the TypeScript stub's ssoRequired. When it is set, every authenticated DELETE/GET/PUT/POST (not HEAD) answers 403 {"error": {"code": "sso_required", "message": ..., "organization_id"?}}. organization_id is left out when the knob's value is None.
Tests: new tests/test_sso_required.py:
- Low layer: sso_required becomes Forbidden with the org. An absent, empty, blank, non-string, URL-delimited, control-character, bidi-override or overlong organization_id gives None. The field is ignored on not_found/queue_full/forbidden envelopes.
- SDK layer: every to_sdk_error branch forwards the org, translating() keeps it when it stamps a key, and error_type stays inside ROUTER_ERROR_TYPES.
- End to end through the stub, with require_auth on: sync jobs.get, sync submit, and async jobs.get, with the org known and unknown.
CHANGELOG: entry under Unreleased → Added.

Judgment calls

No SsoRequired class, on purpose. comfy_sdk's Forbidden is a Router bucket class, and that surface is one class per error_type, which the TypeScript SDK's parity snapshot checks. I ran the TS repo's python-surface.mjs extractors (router classes and error types, status map, exported error classes) on this branch and on origin/main: the output is identical.
organization_id is read only off an sso_required envelope. This makes the documented "None on every other code" true, and means an unrelated 404/429 body that happens to carry the field cannot lead a caller into starting SSO. (Changed in review; the first version read it off any envelope.)
Sanitising: organization_id is checked as a whole token, not shortened to its valid prefix (unlike clean_request_id) and not run through clean_body_excerpt. Callers put it into a URL, and a shortened org id would start SSO for the wrong organization, so a value that does not match is treated as absent. Callers should still URL-encode it when building the SSO start URL.
The plan named tests/test_errors.py / tests/test_exceptions.py, which do not exist in this repo, so the cases are in a new focused file instead.

Provenance

Authored by: agent-work loop
Verified:
- Tests: 1306 passed, 9 skipped (env-gated integration), local Python 3.12, at the current head (the merge of main into this branch)
- Lint/format/type: ruff check, ruff format --check, mypy src all clean at the current head
- Public-repo hygiene script: clean at the current head
- CI at the current head: the full 3.10–3.13 test matrix, build-check, codegen drift and hygiene all pass
- Mutation-tested (first round): dropping either _BY_CODE entry, or the RouterError → ComfyError hand-off, makes test_sso_required.py fail
- TS parity extractors (first round): router surface output identical to origin/main
Deviations: tests are in tests/test_sso_required.py, because the two file names the plan gave do not exist here. HEAD probes cannot carry the org (no body), and this is documented on the attribute rather than recovered.

Residual

Not tested against a live gateway. tests/integration/test_gateway_e2e.py needs a real key on an SSO-held account, and I did not run it. The real gateway's sso_required body shape is taken from the vendored contract and the stub, not observed directly.
HEAD probes lose the SSO details. A HEAD response has no body, so if the real gateway refuses HEAD /api/v2/assets/by-hash/* with 403, the SDK raises a plain forbidden with organization_id=None. This is now documented on ApiError.organization_id / ComfyError.organization_id. The stub's HEAD handler has no auth check and does not model it.
Spec docs: organization_id is not in the ErrorEnvelope schema in the vendored spec/openapi.yaml on main. It arrives with the next one-way spec sync, which this PR does not depend on: error_from_envelope is hand-written and the generated models are untouched.

Summary by CodeRabbit

New Features
- sso_required errors are reported as Forbidden while retaining their error code.
- Error details now include the organization ID when the server provides a valid value; otherwise, it is None.
Documentation
- Documented the organization ID attribute for SDK errors.

_{🔄 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-python-sdk/pull/205 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `matt/be-19944-sso-required-forbidden` --- ### 📝 Commits (3) - [`5f4e920`](https://github.com/Comfy-Org/comfy-python-sdk/commit/5f4e9201e817679b8fa13d7de5b41b25f5501984) feat(errors): map sso_required to Forbidden and carry organization_id - [`9f802c7`](https://github.com/Comfy-Org/comfy-python-sdk/commit/9f802c7ee1dc97a81f65c1b51fcf04ab6dc9f052) fix(errors): gate and sanitise organization_id; keep Forbidden's bucket on sso_required - [`1133ad5`](https://github.com/Comfy-Org/comfy-python-sdk/commit/1133ad5789173e1706da301a739c6350bf2143b6) Merge remote-tracking branch 'origin/main' into matt/be-19944-sso-required-forbidden ### 📊 Changes **8 files changed** (+334 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+4 -0) 📝 `src/comfy_low/errors.py` (+45 -0) 📝 `src/comfy_sdk/_errors.py` (+12 -0) 📝 `src/comfy_sdk/exceptions.py` (+19 -2) 📝 `src/comfy_sdk/router_exceptions.py` (+12 -1) 📝 `tests/conftest.py` (+32 -0) 📝 `tests/test_error_contract.py` (+4 -1) ➕ `tests/test_sso_required.py` (+206 -0) </details> ### 📄 Description ## ELI5 Some Comfy Cloud organizations require their members to sign in through single sign-on. When a personal key is used on one of those accounts, the service refuses it and says which organization to sign in through. Before this change the SDK raised a generic error and dropped that information. Now it raises the usual "access denied" error, so existing error handling still catches it, and the error also carries the organization, so an app can send the user to the right sign-in page. ## What changed - **`comfy_low.errors`**: `ApiError` gains an `organization_id` keyword and attribute. `_BY_CODE["sso_required"] = Forbidden`. `error_from_envelope` reads `error.organization_id` only when the resolved code is `sso_required`, and passes it through the new `clean_organization_id`. That keeps the value only if all of it is a bounded, id-shaped token (`[A-Za-z0-9._:-]{1,200}`). A missing, empty, non-string or malformed value becomes `None`. - **`comfy_sdk`**: `ComfyError` gains `organization_id` as a class-level `None` default, a constructor keyword and an attribute. `RouterError.__init__` accepts it, plus an optional `code=`, and passes both on to `ComfyError`. `_BY_CODE` maps `"sso_required"` to the existing `Forbidden`, which is the `RouterError` subclass. `to_sdk_error` passes `organization_id` through all three branches: `QueueFull`, the `RouterError` branch (the one `sso_required` takes) and the generic one. On the `RouterError` branch, `error_type` is the class's own bucket, so `sso_required` raises `Forbidden` with `error_type == "forbidden"` and `code == "sso_required"`. For every other mapping the bucket already equals the code. - **`_STAMPED_ATTRIBUTES`** now includes `organization_id`. `tests/test_error_contract.py` requires every caller-readable `ComfyError` attribute to be in this list. Without it, `exc.organization_id` on a stamped `httpx.ConnectError` or `CancelledError` would raise `AttributeError` instead of returning `None`. `_BASE_ATTRIBUTES` in that test is extended to match. - **Stub server** (`tests/conftest.py`): a new `sso_required` knob, named after the TypeScript stub's `ssoRequired`. When it is set, every authenticated DELETE/GET/PUT/POST (not HEAD) answers `403 {"error": {"code": "sso_required", "message": ..., "organization_id"?}}`. `organization_id` is left out when the knob's value is `None`. - **Tests**: new `tests/test_sso_required.py`: - Low layer: `sso_required` becomes `Forbidden` with the org. An absent, empty, blank, non-string, URL-delimited, control-character, bidi-override or overlong `organization_id` gives `None`. The field is ignored on `not_found`/`queue_full`/`forbidden` envelopes. - SDK layer: every `to_sdk_error` branch forwards the org, `translating()` keeps it when it stamps a key, and `error_type` stays inside `ROUTER_ERROR_TYPES`. - End to end through the stub, with `require_auth` on: sync `jobs.get`, sync `submit`, and async `jobs.get`, with the org known and unknown. - **CHANGELOG**: entry under Unreleased → Added. ## Judgment calls - **No `SsoRequired` class**, on purpose. `comfy_sdk`'s `Forbidden` is a Router bucket class, and that surface is one class per `error_type`, which the TypeScript SDK's parity snapshot checks. I ran the TS repo's `python-surface.mjs` extractors (router classes and error types, status map, exported error classes) on this branch and on `origin/main`: the output is identical. - **`organization_id` is read only off an `sso_required` envelope.** This makes the documented "`None` on every other code" true, and means an unrelated 404/429 body that happens to carry the field cannot lead a caller into starting SSO. (Changed in review; the first version read it off any envelope.) - **Sanitising**: `organization_id` is checked as a whole token, not shortened to its valid prefix (unlike `clean_request_id`) and not run through `clean_body_excerpt`. Callers put it into a URL, and a shortened org id would start SSO for the wrong organization, so a value that does not match is treated as absent. Callers should still URL-encode it when building the SSO start URL. - The plan named `tests/test_errors.py` / `tests/test_exceptions.py`, which do not exist in this repo, so the cases are in a new focused file instead. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: 1306 passed, 9 skipped (env-gated integration), local Python 3.12, at the current head (the merge of `main` into this branch) - Lint/format/type: `ruff check`, `ruff format --check`, `mypy src` all clean at the current head - Public-repo hygiene script: clean at the current head - CI at the current head: the full 3.10–3.13 test matrix, `build-check`, codegen drift and hygiene all pass - Mutation-tested (first round): dropping either `_BY_CODE` entry, or the `RouterError` → `ComfyError` hand-off, makes `test_sso_required.py` fail - TS parity extractors (first round): router surface output identical to `origin/main` - **Deviations:** tests are in `tests/test_sso_required.py`, because the two file names the plan gave do not exist here. HEAD probes cannot carry the org (no body), and this is documented on the attribute rather than recovered. ## Residual - **Not tested against a live gateway.** `tests/integration/test_gateway_e2e.py` needs a real key on an SSO-held account, and I did not run it. The real gateway's `sso_required` body shape is taken from the vendored contract and the stub, not observed directly. - **HEAD probes lose the SSO details.** A HEAD response has no body, so if the real gateway refuses `HEAD /api/v2/assets/by-hash/*` with 403, the SDK raises a plain `forbidden` with `organization_id=None`. This is now documented on `ApiError.organization_id` / `ComfyError.organization_id`. The stub's HEAD handler has no auth check and does not model it. - **Spec docs**: `organization_id` is not in the `ErrorEnvelope` schema in the vendored `spec/openapi.yaml` on `main`. It arrives with the next one-way spec sync, which this PR does not depend on: `error_from_envelope` is hand-written and the generated models are untouched.  ## Summary by CodeRabbit * **New Features** * `sso_required` errors are reported as `Forbidden` while retaining their error code. * Error details now include the organization ID when the server provides a valid value; otherwise, it is `None`. * **Documentation** * Documented the organization ID attribute for SDK errors.  --- <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:02:19 +00:00

bsakaguchi referenced this issue

2026-10-31 22:02:21 +00:00

[PR #210] docs(errors): list rate_limited explicitly as the fourth code both surfaces share #211

bsakaguchi referenced this issue

2026-10-31 22:02:22 +00:00

[PR #213] feat(errors): export RateLimited as a fourth shared bucket and map v2 rate_limited to it #213