[PR #67] [MERGED] feat: add the comfy.models namespace and module-level comfy.config({ credentials }) #69

New issue

Closed

opened 2026-10-31 22:04:16 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:04:16 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/67
Author: @mattmillerai
Created: 8/23/2026
Status: ✅ Merged
Merged: 8/23/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8502-models-namespace-config

📝 Commits (1)

706bcdc feat: add the comfy.models namespace and module-level comfy.config({ credentials })

📊 Changes

11 files changed (+657 additions, -5 deletions)

View changed files

📝 README.md (+61 -0)
📝 src/index.test.ts (+19 -1)
📝 src/low/transport.ts (+11 -4)
➕ src/sdk/comfy.ts (+31 -0)
➕ src/sdk/credentials.test.ts (+186 -0)
➕ src/sdk/credentials.ts (+87 -0)
📝 src/sdk/exceptions.ts (+23 -0)
📝 src/sdk/index.ts (+10 -0)
➕ src/sdk/models.spec-contract.test.ts (+66 -0)
➕ src/sdk/models.test.ts (+98 -0)
➕ src/sdk/models.ts (+65 -0)

📄 Description

ELI-5

There are two ways to hand this SDK an API key. Until now there was one: construct a Comfy client and pass apiKey to it. This adds the other one — configure it once for the whole process (comfy.config({ credentials })), which is the shape a lot of hosted-inference clients use and the shape someone porting an existing integration already has in their fingers. It also adds the comfy.models namespace those calls will hang off. comfy.models.run does not run anything yet: it resolves credentials and fails loudly and locally, pointing you at the workflow API that does work today. Separately, and less pleasantly: console.log(client) used to print your API key. It no longer does.

What changed

New module-level surface (src/sdk/credentials.ts, src/sdk/models.ts, src/sdk/comfy.ts), re-exported from the package root:

comfy.config({ credentials }) sets the credential for every subsequent comfy.* call. Only keys present on the argument are touched, so config({}) is a no-op; credentials: undefined clears; an empty or whitespace-only string is a TypeError rather than a silent clear (a silent clear would fall through to the environment and quietly break the documented precedence).
Credentials also resolve from COMFY_API_KEY, read per call rather than at module load. Explicit config wins. A blank variable counts as unset, and a runtime with no process (a browser) degrades to undefined instead of throwing.
Both documented import shapes reach the same members and are asserted to be identical objects: import { comfy } and import * as comfy.
comfy.models.run(model, input) resolves credentials and throws locally, with no socket opened: MissingCredentials when there is none, ModelRunNotImplemented when there is. Both are ComfyError subclasses.
New contract tripwire src/sdk/models.spec-contract.test.ts: it asserts the vendored spec declares no model-execution route, and fails with a fix-it message the moment one appears.

Credential leak fix (src/low/transport.ts): ComfyLow's apiKey is now an ECMAScript #private field rather than a TypeScript private one. TS private is erased at runtime, so the key was an enumerable own property and showed up in anything that serialized or inspected a client.

Docs: a new README section covering the namespace, the precedence rules, the process-global caveat, the two error cases, and the module-format story.

Why `models.run` throws instead of running (falsification evidence)

This diff ships a capability denial, so the denial was checked rather than assumed. Every surface I can read says model-by-id execution does not exist here yet:

Surface	Result
`spec/openapi.yaml` (the contract this SDK generates from)	10 paths, all under `/api/v2/assets` and `/api/v2/jobs`; no model route
`OPERATION_IDS` in `src/low/transport.ts`	11 operations, none model-related
`src/low/generated/types.gen.ts`	the only occurrence of "model" is a doc example path string
published `@comfyorg/sdk@0.1.7` (`npm pack`, unpacked)	no `models` / `comfy` / `credentials` module; root exports are the class client only
the sibling Python SDK on its default branch	no models namespace

One path I could not settle, stated as such: an unauthenticated read-only probe of the deployed gateway is inconclusive, because it authenticates before it routes — https://cloud.comfy.org/api/v2/<anything> returns 401 unauthorized whether or not the path exists (a deliberately nonsense /api/v2/... path 401s too, while /api/v9/... 404s), and no credential was available in the environment this was written in. That is why the thrown message is scoped to the spec ("the Comfy API v2 spec this SDK is generated from declares no model-execution route") rather than to the server: that claim is verifiable from this repo and stays true regardless of what the gateway is serving behind auth. And the acceptance criteria require the types to come from the spec, so an undocumented route would not be implementable here anyway.

The denial is a signpost, not a dead-end. Running a model's workflow graph works today, so the error redirects there — await new Comfy({ apiKey }).run(workflow) — and a test asserts that sentence stays in the message.

Acceptance criteria

Criterion	State
`comfy.models` exported and reachable as documented	met — both import shapes, root re-export tested, README section added
`comfy.config({ credentials })` sets credentials for subsequent calls	met
Credentials also resolve from the environment, explicit config taking precedence	met — `COMFY_API_KEY`, per-call read, precedence tested
`models.run` with no credentials throws a clear, named error locally, without a network call	met — `MissingCredentials`; a `fetch` spy asserts zero calls
Credentials never in a thrown error's message, a serialized client, or a log line — asserted by test	met, and it was not true before this change (see below)
Types generated from / checked against the spec rather than hand-declared	partially met — see "Not met" below
Works in both ESM and CJS consumers if the package supports both today; otherwise say so	the package is ESM-only — saying so, measured below

Not met: "a contract change surfaces as a type error"

Nothing in this diff hand-declares a wire type — but neither does it generate one, because there is nothing to generate: the vendored spec has no model-execution operation, and the story this one sits under is explicitly blocked on that route contract. So the criterion is met in its "rather than hand-declared" half and not met in its "surfaces as a type error" half.

What I shipped in its place is a mechanical tripwire rather than a compile failure, and it is proven, not asserted: injecting a POST /api/v2/models/{model}/run operation into the spec turns three tests red — both assertions in models.spec-contract.test.ts (each printing "run pnpm generate, wire it through src/low/transport.ts, replace the ModelRunNotImplemented placeholder with a call typed by the generated request/response types") and the pre-existing spec coverage > declared operation IDs match the vendored spec. The spec was restored immediately; no spec change is in this diff. A genuine generated-type binding lands with the story that implements run.

The package is ESM-only

Measured, not inferred, against the built dist/ on Node 24.18.0 and Node 22.23.2:

ESM import (both { comfy } and * as comfy): works.
CommonJS require("@comfyorg/sdk") on Node >= 22.12: works, via Node's built-in require(esm).
CommonJS await import(...): works.
TypeScript consumers: clean under "module": "nodenext" for both "type": "module" and "type": "commonjs" packages. Under "module": "node16", a CommonJS consumer's static import is rejected with TS1479 and needs await import(...).

No CJS build was added — that is a packaging decision (dual-build, exports conditions, "engines" floor) well outside this story. The README now states the above rather than leaving a consumer to discover it.

The credential leak this uncovered

While writing the redaction test I found the criterion was already violated on main and in the published package. Same sweep, run both ways over every consumer-reachable object that transitively holds the key, checking JSON.stringify, util.inspect(depth: 20) and String():

published @comfyorg/sdk@0.1.7: 4 of 6 objects print the API key (Comfy, AssetFactory, JobFactory, Asset; WorkflowFactory and Job hold no client).
this branch: 0 of 7 (the seventh being the new comfy namespace).

Reverting only the #apiKey line turns exactly one test red — credential redaction > keeps the constructor apiKey out of a serialized client — so the fix is pinned red-to-green. The SDK itself emits no log lines at all (grep for console.* / process.stdout over non-test source: zero hits), so the "log line" half of the criterion is a property of the objects, which is what the test measures.

Judgment calls

COMFY_API_KEY is a new public contract I chose. Neither SDK had an environment variable for the key before; this is the obvious sibling to the existing COMFY_BASE_URL. Easy to veto now, expensive to rename later.
new Comfy() deliberately does not consult comfy.config or COMFY_API_KEY. Making it fall back would mean a process with COMFY_API_KEY exported starts attaching a bearer token to a self-hosted proxy the user deliberately gave no key to — and the README's auth table explicitly says not to pass a key there. The fallback is scoped to the new comfy.* surface; the class client is untouched.
Chesterton's fence on the #apiKey change. The auth condition itself (if (apiKey && isSameOrigin(url))) is byte-for-byte unchanged; only the field's storage moved, and both read sites moved together. A test asserts Authorization: Bearer <key> is still sent, and the full pre-existing suite passes on Node 22 and 24. The one behavior change for a consumer: JavaScript code reaching into low.apiKey now reads undefined. TypeScript already forbade that, and it is the point of the fix — flagging it rather than burying it.
Object.freeze on comfy and comfy.models, since they are shared process-wide.
resolveCredentials() is exported, so an app can ask "am I configured?" without a try/catch around a call that throws.
/model/i on operationIds in the tripwire is deliberately broad. A false trip is a loud failure with instructions, and the pre-existing spec-coverage test already fails on any new operation, so the marginal blast radius is nil.
config takes only credentials. COMFY_BASE_URL remains the only base-URL knob; adding comfy.config({ baseUrl }) was not asked for and would need its own precedence story against the existing environment variable.

Unexercised artifacts

Two internal planning documents referenced by the work item are not reachable from the environment this was written in, so the namespace shape was taken from the acceptance criteria and from the existing SDK's conventions rather than from those documents. If either specifies a different member layout for comfy.models, this needs adjusting before release.
The parent epic and the blocking route-contract story were named to me but their contents were not, so nothing here is verified against them.
The deployed gateway's /api/v2/models behaviour, for the pre-routing-401 reason detailed above.

Follow-on work this does not do

Out of scope for this story and left open deliberately: models.run's actual execution, its full error-class taxonomy, retry and abortSignal, and browser-specific credential handling. The ModelRunNotImplemented placeholder and models.spec-contract.test.ts both exist to be deleted by the story that implements run. This work item stays open — its "types generated from the spec" criterion is only half satisfied, as detailed above.

Provenance

Authored by: agent-work loop
Verified: pnpm format:check, pnpm lint, pnpm typecheck clean; pnpm test 224 passed / 19 files (192 was the baseline on main), green on both Node 24.18.0 and Node 22.23.2; node scripts/check-spec-drift.mjs in sync; node scripts/check-public-repo-hygiene.mjs clean; pnpm build + pnpm pack produce dist/index.js and dist/index.d.ts with no test files in the tarball; ESM/CJS/TypeScript consumer matrix exercised against the built dist as tabulated above; redaction sweep run against both the published 0.1.7 and this branch (4/6 leaking vs 0/7); tripwire and redaction fixes each proven red before green.
Deviations: the "surfaces as a type error" half of the generated-types criterion is not met, and a mechanical spec tripwire ships in its place — reasons and evidence in "Not met" above. No CommonJS build was added; the package stays ESM-only and now documents it.

_{🔄 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/67 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/23/2026 **Status:** ✅ Merged **Merged:** 8/23/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8502-models-namespace-config` --- ### 📝 Commits (1) - [`706bcdc`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/706bcdc4ba3325ac2fe891feb22c0037bf1bdea2) feat: add the comfy.models namespace and module-level comfy.config({ credentials }) ### 📊 Changes **11 files changed** (+657 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+61 -0) 📝 `src/index.test.ts` (+19 -1) 📝 `src/low/transport.ts` (+11 -4) ➕ `src/sdk/comfy.ts` (+31 -0) ➕ `src/sdk/credentials.test.ts` (+186 -0) ➕ `src/sdk/credentials.ts` (+87 -0) 📝 `src/sdk/exceptions.ts` (+23 -0) 📝 `src/sdk/index.ts` (+10 -0) ➕ `src/sdk/models.spec-contract.test.ts` (+66 -0) ➕ `src/sdk/models.test.ts` (+98 -0) ➕ `src/sdk/models.ts` (+65 -0) </details> ### 📄 Description ## ELI-5 There are two ways to hand this SDK an API key. Until now there was one: construct a `Comfy` client and pass `apiKey` to it. This adds the other one — configure it once for the whole process (`comfy.config({ credentials })`), which is the shape a lot of hosted-inference clients use and the shape someone porting an existing integration already has in their fingers. It also adds the `comfy.models` namespace those calls will hang off. `comfy.models.run` does not run anything yet: it resolves credentials and fails loudly and locally, pointing you at the workflow API that does work today. Separately, and less pleasantly: `console.log(client)` used to print your API key. It no longer does. ## What changed **New module-level surface** (`src/sdk/credentials.ts`, `src/sdk/models.ts`, `src/sdk/comfy.ts`), re-exported from the package root: - `comfy.config({ credentials })` sets the credential for every subsequent `comfy.*` call. Only keys present on the argument are touched, so `config({})` is a no-op; `credentials: undefined` clears; an empty or whitespace-only string is a `TypeError` rather than a silent clear (a silent clear would fall through to the environment and quietly break the documented precedence). - Credentials also resolve from `COMFY_API_KEY`, read **per call** rather than at module load. Explicit config wins. A blank variable counts as unset, and a runtime with no `process` (a browser) degrades to `undefined` instead of throwing. - Both documented import shapes reach the same members and are asserted to be identical objects: `import { comfy }` and `import * as comfy`. - `comfy.models.run(model, input)` resolves credentials and throws **locally, with no socket opened**: `MissingCredentials` when there is none, `ModelRunNotImplemented` when there is. Both are `ComfyError` subclasses. - New contract tripwire `src/sdk/models.spec-contract.test.ts`: it asserts the vendored spec declares no model-execution route, and fails with a fix-it message the moment one appears. **Credential leak fix** (`src/low/transport.ts`): `ComfyLow`'s `apiKey` is now an ECMAScript `#private` field rather than a TypeScript `private` one. TS `private` is erased at runtime, so the key was an enumerable own property and showed up in anything that serialized or inspected a client. **Docs**: a new README section covering the namespace, the precedence rules, the process-global caveat, the two error cases, and the module-format story. ## Why `models.run` throws instead of running (falsification evidence) This diff ships a capability denial, so the denial was checked rather than assumed. Every surface I can read says model-by-id execution does not exist here yet: | Surface | Result | | --- | --- | | `spec/openapi.yaml` (the contract this SDK generates from) | 10 paths, all under `/api/v2/assets` and `/api/v2/jobs`; no model route | | `OPERATION_IDS` in `src/low/transport.ts` | 11 operations, none model-related | | `src/low/generated/types.gen.ts` | the only occurrence of "model" is a doc example path string | | published `@comfyorg/sdk@0.1.7` (`npm pack`, unpacked) | no `models` / `comfy` / `credentials` module; root exports are the class client only | | the sibling Python SDK on its default branch | no models namespace | **One path I could not settle, stated as such:** an unauthenticated read-only probe of the deployed gateway is inconclusive, because it authenticates before it routes — `https://cloud.comfy.org/api/v2/<anything>` returns `401 unauthorized` whether or not the path exists (a deliberately nonsense `/api/v2/...` path 401s too, while `/api/v9/...` 404s), and no credential was available in the environment this was written in. That is why the thrown message is scoped to **the spec** ("the Comfy API v2 spec this SDK is generated from declares no model-execution route") rather than to the server: that claim is verifiable from this repo and stays true regardless of what the gateway is serving behind auth. And the acceptance criteria require the types to come from the spec, so an undocumented route would not be implementable here anyway. **The denial is a signpost, not a dead-end.** Running a model's workflow graph works today, so the error redirects there — `await new Comfy({ apiKey }).run(workflow)` — and a test asserts that sentence stays in the message. ## Acceptance criteria | Criterion | State | | --- | --- | | `comfy.models` exported and reachable as documented | met — both import shapes, root re-export tested, README section added | | `comfy.config({ credentials })` sets credentials for subsequent calls | met | | Credentials also resolve from the environment, explicit config taking precedence | met — `COMFY_API_KEY`, per-call read, precedence tested | | `models.run` with no credentials throws a clear, named error locally, without a network call | met — `MissingCredentials`; a `fetch` spy asserts zero calls | | Credentials never in a thrown error's message, a serialized client, or a log line — asserted by test | met, and it was **not** true before this change (see below) | | Types generated from / checked against the spec rather than hand-declared | **partially met — see "Not met" below** | | Works in both ESM and CJS consumers if the package supports both today; otherwise say so | **the package is ESM-only — saying so, measured below** | ### Not met: "a contract change surfaces as a type error" Nothing in this diff hand-declares a wire type — but neither does it generate one, because there is nothing to generate: the vendored spec has no model-execution operation, and the story this one sits under is explicitly blocked on that route contract. So the criterion is met in its "rather than hand-declared" half and **not** met in its "surfaces as a type error" half. What I shipped in its place is a mechanical tripwire rather than a compile failure, and it is proven, not asserted: injecting a `POST /api/v2/models/{model}/run` operation into the spec turns three tests red — both assertions in `models.spec-contract.test.ts` (each printing "run `pnpm generate`, wire it through `src/low/transport.ts`, replace the `ModelRunNotImplemented` placeholder with a call typed by the generated request/response types") and the pre-existing `spec coverage > declared operation IDs match the vendored spec`. The spec was restored immediately; no spec change is in this diff. A genuine generated-type binding lands with the story that implements `run`. ### The package is ESM-only Measured, not inferred, against the built `dist/` on **Node 24.18.0 and Node 22.23.2**: - ESM `import` (both `{ comfy }` and `* as comfy`): works. - CommonJS `require("@comfyorg/sdk")` on Node >= 22.12: works, via Node's built-in `require(esm)`. - CommonJS `await import(...)`: works. - TypeScript consumers: clean under `"module": "nodenext"` for both `"type": "module"` and `"type": "commonjs"` packages. Under `"module": "node16"`, a CommonJS consumer's static import is rejected with `TS1479` and needs `await import(...)`. No CJS build was added — that is a packaging decision (dual-build, `exports` conditions, `"engines"` floor) well outside this story. The README now states the above rather than leaving a consumer to discover it. ## The credential leak this uncovered While writing the redaction test I found the criterion was already violated on `main` and in the published package. Same sweep, run both ways over every consumer-reachable object that transitively holds the key, checking `JSON.stringify`, `util.inspect(depth: 20)` and `String()`: - **published `@comfyorg/sdk@0.1.7`: 4 of 6 objects print the API key** (`Comfy`, `AssetFactory`, `JobFactory`, `Asset`; `WorkflowFactory` and `Job` hold no client). - **this branch: 0 of 7** (the seventh being the new `comfy` namespace). Reverting only the `#apiKey` line turns exactly one test red — `credential redaction > keeps the constructor apiKey out of a serialized client` — so the fix is pinned red-to-green. The SDK itself emits no log lines at all (`grep` for `console.*` / `process.stdout` over non-test source: zero hits), so the "log line" half of the criterion is a property of the objects, which is what the test measures. ## Judgment calls - **`COMFY_API_KEY` is a new public contract I chose.** Neither SDK had an environment variable for the key before; this is the obvious sibling to the existing `COMFY_BASE_URL`. Easy to veto now, expensive to rename later. - **`new Comfy()` deliberately does *not* consult `comfy.config` or `COMFY_API_KEY`.** Making it fall back would mean a process with `COMFY_API_KEY` exported starts attaching a bearer token to a self-hosted proxy the user deliberately gave no key to — and the README's auth table explicitly says not to pass a key there. The fallback is scoped to the new `comfy.*` surface; the class client is untouched. - **Chesterton's fence on the `#apiKey` change.** The auth condition itself (`if (apiKey && isSameOrigin(url))`) is byte-for-byte unchanged; only the field's storage moved, and both read sites moved together. A test asserts `Authorization: Bearer <key>` is still sent, and the full pre-existing suite passes on Node 22 and 24. The one behavior change for a consumer: JavaScript code reaching into `low.apiKey` now reads `undefined`. TypeScript already forbade that, and it is the point of the fix — flagging it rather than burying it. - **`Object.freeze` on `comfy` and `comfy.models`**, since they are shared process-wide. - **`resolveCredentials()` is exported**, so an app can ask "am I configured?" without a `try`/`catch` around a call that throws. - **`/model/i` on operationIds in the tripwire is deliberately broad.** A false trip is a loud failure with instructions, and the pre-existing spec-coverage test already fails on *any* new operation, so the marginal blast radius is nil. - **`config` takes only `credentials`.** `COMFY_BASE_URL` remains the only base-URL knob; adding `comfy.config({ baseUrl })` was not asked for and would need its own precedence story against the existing environment variable. ## Unexercised artifacts - Two internal planning documents referenced by the work item are not reachable from the environment this was written in, so the namespace shape was taken from the acceptance criteria and from the existing SDK's conventions rather than from those documents. If either specifies a different member layout for `comfy.models`, this needs adjusting before release. - The parent epic and the blocking route-contract story were named to me but their contents were not, so nothing here is verified against them. - The deployed gateway's `/api/v2/models` behaviour, for the pre-routing-401 reason detailed above. ## Follow-on work this does not do Out of scope for this story and left open deliberately: `models.run`'s actual execution, its full error-class taxonomy, retry and `abortSignal`, and browser-specific credential handling. The `ModelRunNotImplemented` placeholder and `models.spec-contract.test.ts` both exist to be deleted by the story that implements `run`. This work item stays open — its "types generated from the spec" criterion is only half satisfied, as detailed above. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm format:check`, `pnpm lint`, `pnpm typecheck` clean; `pnpm test` 224 passed / 19 files (192 was the baseline on `main`), green on both Node 24.18.0 and Node 22.23.2; `node scripts/check-spec-drift.mjs` in sync; `node scripts/check-public-repo-hygiene.mjs` clean; `pnpm build` + `pnpm pack` produce `dist/index.js` and `dist/index.d.ts` with no test files in the tarball; ESM/CJS/TypeScript consumer matrix exercised against the built `dist` as tabulated above; redaction sweep run against both the published 0.1.7 and this branch (4/6 leaking vs 0/7); tripwire and redaction fixes each proven red before green. - **Deviations:** the "surfaces as a type error" half of the generated-types criterion is not met, and a mechanical spec tripwire ships in its place — reasons and evidence in "Not met" above. No CommonJS build was added; the package stays ESM-only and now documents it. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:04:16 +00:00