[PR #83] [MERGED] test: check cross-SDK surface parity against the Python SDK #85

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-8506-cross-sdk-surface-parity

📝 Commits (1)

fc7ac22 test: check cross-SDK surface parity against the Python SDK

📊 Changes

9 files changed (+856 additions, -2 deletions)

View changed files

📝 .github/workflows/ci.yml (+23 -0)
📝 .prettierignore (+6 -0)
📝 AGENTS.md (+30 -1)
📝 package.json (+3 -1)
➕ parity/README.md (+35 -0)
➕ parity/python-surface.json (+76 -0)
➕ scripts/python-surface.mjs (+251 -0)
➕ scripts/sync-python-surface.mjs (+125 -0)
➕ src/sdk/surface-parity.test.ts (+307 -0)

📄 Description

ELI-5

We ship two SDKs — Python and TypeScript — that are supposed to expose the same things under the same names. They are maintained by separate pull requests in separate repositories, so they drift, and nobody finds out until a user copies a Python example into TypeScript and the method is not there. This adds a test that compares the two surfaces and fails saying exactly which name went missing and in which direction.

What is compared

Public method names under models — comfy.models here against the Python Models namespace class.
Router error class names — routerErrors.* against comfy_sdk.router_exceptions.
The error_type each of those classes maps to, per class, not just as a set — a class wired to the wrong bucket is caught, not only a missing one.
The error_type set as a whole (ROUTER_ERROR_TYPES on both sides).
The error classes the package root re-exports — the workflow-API hierarchy (ComfyError and friends), which is where the credential asymmetry below actually shows up as a name.

Where it lives, and how the other repository's surface is obtained

A cross-repo check needs a stated mechanism rather than an implicit one, so here it is. The work is split in two, mirroring how this repo already handles the vendored OpenAPI spec:

Where	What it does	Network
`pnpm test` → `src/sdk/surface-parity.test.ts`	Diffs this SDK's live exports against the committed snapshot `parity/python-surface.json`. This is the parity assertion.	no
`sdk-parity` CI job → `pnpm check:sdk-parity`	Re-derives the snapshot from the Python SDK's default branch and fails if it has moved.	yes

scripts/sync-python-surface.mjs reads four files from the public Comfy-Org/comfy-python-sdk repository over plain HTTPS (raw.githubusercontent.com — no token, no clone) and extracts the surface with scripts/python-surface.mjs, which parses those files rather than importing the package: importing would need a Python toolchain, a virtualenv and the Python SDK's runtime dependencies inside a Node repo's CI, which is a lot of moving parts for a check whose only job is to be trustworthy.

The snapshot is committed rather than fetched at test time for the same reason spec/openapi.yaml is vendored: the assertion has to run offline and deterministically on every PR, and a test that reaches the network fails for reasons that have nothing to do with the code under review. The sdk-parity job is what stops the snapshot going quietly stale — without it the offline half would keep passing over a frozen copy.

Documented in-repo at parity/README.md and AGENTS.md section 6.

The intentional asymmetries

Declared in a commented INTENTIONAL_ASYMMETRIES allowlist at the top of the test. Anything not in it fails.

result-envelope — Python returns the provider payload directly; TypeScript returns { data, requestId }. Return types only, no name differs, so the entry suppresses nothing. It is declared anyway so a reviewer who notices the difference finds it listed as a decision rather than wondering whether the check can see it.
credential-resolution — Python resolves credentials per client instance, TypeScript configures them once at module level. The one name this changes is MissingApiKey (Python) versus MissingCredentials (TypeScript), so the entry carries that rename.
no-sync-variant — Python ships Models/AsyncModels, TypeScript is promise-native with one comfy.models. AsyncModels is therefore not expected to have a counterpart here — but it is still checked, against the sync class: the async twin may introduce no method name of its own, which is what stops a suffixed run_async from becoming a Python-only method nobody notices is absent.

The allowlist also fails on an entry that no longer applies (a rename whose Python name is gone), so it cannot rot into a blanket exemption.

Not passing vacuously

A broken introspection step must not read as parity, so this fails loudly in three places instead of returning an empty set: every extractor in scripts/python-surface.mjs throws on an empty or unrecognized result; the snapshot loader rejects any empty section; and the TypeScript side asserts each introspected group is non-empty before diffing. models.py growing a third public class is a hard failure too, rather than a surface the check silently does not look at.

Verification of the failure modes

Each assertion was exercised against a deliberately mutated snapshot, confirming it fails and names the symbol:

Mutation	Failure message
Python gains `Models.submit`	comfy.models: `submit` exists in the Python SDK but not in the TypeScript SDK
Python gains `AsyncModels.run_async`	`AsyncModels declares methods the synchronous class does not: [ 'run_async' ]`
`ProviderError` renamed	routerErrors: `ProviderFailure` exists in the Python SDK but not in the TypeScript SDK + the converse line
`ProviderTimeout` rebucketed	`routerErrors.ProviderTimeout: error_type is "provider_timeout" in the TypeScript SDK but "upstream_timeout" in the Python SDK`
`forbidden` dropped, `rate_limited` added	error_type: `rate_limited` exists in the Python SDK but not in the TypeScript SDK + the converse line
Python renames `MissingApiKey` to match	the allowlist renames `MissingApiKey`, which the Python SDK no longer exports
A snapshot section emptied	parity/python-surface.json has an empty `routerErrorClasses` … an empty section is a broken extraction, not parity

Two of these (the unrecognized-class guard and the empty router set) are pinned as tests in the file; the rest are assertions over the snapshot, exercised by mutating it locally.

Coverage, including the part this does not cover

Compared today: 38 symbols — 1 models method, 12 router error classes, 11 error_type values, 14 root-exported error classes.

Not compared: 24 of the package root's 39 exported symbols (Comfy, Workflow, Job, Asset, Output, their factories, and the base-URL / timeout / retry constants) and the 6 non-class members of routerErrors (the two header-name constants, ROUTER_ERROR_TYPES and its two halves, toRouterError). Those are outside the three criteria this check was written to, and several of them are constants whose values would matter more than their names. Widening it is a deliberate next step, not an oversight — see Residual.

Provenance

Authored by: agent-work loop
Verified: pnpm lint clean; pnpm format:check clean; pnpm typecheck clean; pnpm test 365 passed / 0 failed across 21 files; node scripts/check-spec-drift.mjs in sync; node scripts/check-public-repo-hygiene.mjs clean; node scripts/sync-python-surface.mjs matches the Python SDK's default branch; pnpm build + pnpm pack succeed and the tarball contains no parity/ or surface-parity entries; and the seven failure modes tabled above each reproduced against a mutated snapshot.
Deviations: none against the stated criteria. The scope calls this check surface-only, and the divergences listed under Residual are behavioural, so they are reported rather than fixed here.

Residual

Not fixed by this PR, and each worth its own follow-up:

The two SDKs' status-to-bucket fallback maps genuinely disagree. When a response carries no X-Comfy-Error-Type at all, TypeScript's ERROR_TYPE_BY_STATUS maps 400 and 422 to invalid_input; the Python SDK deliberately omits both, its comment reasoning that the contract pins no single bucket to either and that guessing between invalid_input and content_policy_violation would tell a caller to retry a deterministic refusal. Both sides also handle a 499 the same way, so this is specifically the 400/422 pair. One of the two is wrong and a caller's except/catch branch differs by language today. This is behaviour, not surface, so the check added here does not and will not catch it — it was found by reading both sources while building the check.
RouterError's own default error_type differs — "internal_error" in TypeScript, "" in Python. The base class is excluded from the per-class error_type comparison for exactly this reason, and the exclusion is commented in the test. Also behavioural rather than a name, and it interacts with item 1: an unknown bucket carries a different default value depending on which SDK you are in.
The mirror-side job was not built. The criterion asks for the check to run in at least one of the two repositories and it runs here, but there is no Python-side equivalent reading this SDK's surface. A change landing in this repo is therefore caught by this repo's own pnpm test, while a change landing in the Python repo is caught only the next time this repo's CI runs — so drift originating on the Python side has a detection lag equal to the gap between PRs here. A scheduled run of the sdk-parity job, or a mirror job in the Python repo, would close that.
The sdk-parity job is not a required check. That is deliberate per the scope ("get it running first, then decide"), and it is worth deciding: as configured, a Python-side surface change reddens this job on unrelated PRs in this repo until somebody refreshes the snapshot. That is the intended loudness, but it is the cost that gets a parity check disabled, so it should be a conscious call.
Within-language parity for the Python SDK's two clients is out of scope and is covered by that repo's own test. This check only enforces the one-directional rule that the async models class introduces no method name the sync one lacks.
The design documents that specify the shared namespace and the shared error table were not readable from this run — they live in an internal workspace with no access from here. The shared set was therefore taken from the two SDKs' own source rather than from the specification they were both written against, so this check asserts that the two SDKs agree with each other, not that either agrees with the design document. If the two have drifted from the spec in the same direction, this check passes.
The surface not compared, quantified above (24 of 39 root exports, 6 of 18 routerErrors members): the workflow-API classes Comfy, Workflow, Job, Asset, Output and their factories have no name-parity assertion, so a method added to Workflow in one SDK and not the other is still invisible.

_{🔄 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/83 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/25/2026 **Status:** ✅ Merged **Merged:** 8/26/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8506-cross-sdk-surface-parity` --- ### 📝 Commits (1) - [`fc7ac22`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/fc7ac22c090aa403261dd5a5629f8c16f2218439) test: check cross-SDK surface parity against the Python SDK ### 📊 Changes **9 files changed** (+856 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci.yml` (+23 -0) 📝 `.prettierignore` (+6 -0) 📝 `AGENTS.md` (+30 -1) 📝 `package.json` (+3 -1) ➕ `parity/README.md` (+35 -0) ➕ `parity/python-surface.json` (+76 -0) ➕ `scripts/python-surface.mjs` (+251 -0) ➕ `scripts/sync-python-surface.mjs` (+125 -0) ➕ `src/sdk/surface-parity.test.ts` (+307 -0) </details> ### 📄 Description ## ELI-5 We ship two SDKs — Python and TypeScript — that are supposed to expose the same things under the same names. They are maintained by separate pull requests in separate repositories, so they drift, and nobody finds out until a user copies a Python example into TypeScript and the method is not there. This adds a test that compares the two surfaces and fails saying exactly which name went missing and in which direction. ## What is compared - Public **method names under `models`** — `comfy.models` here against the Python `Models` namespace class. - **Router error class names** — `routerErrors.*` against `comfy_sdk.router_exceptions`. - The **`error_type` each of those classes maps to**, per class, not just as a set — a class wired to the wrong bucket is caught, not only a missing one. - The **`error_type` set** as a whole (`ROUTER_ERROR_TYPES` on both sides). - The **error classes the package root re-exports** — the workflow-API hierarchy (`ComfyError` and friends), which is where the credential asymmetry below actually shows up as a name. ## Where it lives, and how the other repository's surface is obtained A cross-repo check needs a stated mechanism rather than an implicit one, so here it is. The work is split in two, mirroring how this repo already handles the vendored OpenAPI spec: | Where | What it does | Network | | --- | --- | --- | | `pnpm test` → `src/sdk/surface-parity.test.ts` | Diffs this SDK's **live exports** against the committed snapshot `parity/python-surface.json`. This is the parity assertion. | no | | `sdk-parity` CI job → `pnpm check:sdk-parity` | Re-derives the snapshot from the Python SDK's default branch and fails if it has moved. | yes | `scripts/sync-python-surface.mjs` reads four files from the public `Comfy-Org/comfy-python-sdk` repository over plain HTTPS (`raw.githubusercontent.com` — no token, no clone) and extracts the surface with `scripts/python-surface.mjs`, which **parses** those files rather than importing the package: importing would need a Python toolchain, a virtualenv and the Python SDK's runtime dependencies inside a Node repo's CI, which is a lot of moving parts for a check whose only job is to be trustworthy. The snapshot is committed rather than fetched at test time for the same reason `spec/openapi.yaml` is vendored: the assertion has to run offline and deterministically on every PR, and a test that reaches the network fails for reasons that have nothing to do with the code under review. The `sdk-parity` job is what stops the snapshot going quietly stale — without it the offline half would keep passing over a frozen copy. Documented in-repo at `parity/README.md` and `AGENTS.md` section 6. ## The intentional asymmetries Declared in a commented `INTENTIONAL_ASYMMETRIES` allowlist at the top of the test. Anything not in it fails. 1. **`result-envelope`** — Python returns the provider payload directly; TypeScript returns `{ data, requestId }`. Return types only, no name differs, so the entry suppresses nothing. It is declared anyway so a reviewer who notices the difference finds it listed as a decision rather than wondering whether the check can see it. 2. **`credential-resolution`** — Python resolves credentials per client instance, TypeScript configures them once at module level. The one name this changes is `MissingApiKey` (Python) versus `MissingCredentials` (TypeScript), so the entry carries that rename. 3. **`no-sync-variant`** — Python ships `Models`/`AsyncModels`, TypeScript is promise-native with one `comfy.models`. `AsyncModels` is therefore not expected to have a counterpart here — but it is still checked, against the *sync* class: the async twin may introduce no method name of its own, which is what stops a suffixed `run_async` from becoming a Python-only method nobody notices is absent. The allowlist also fails on an entry that **no longer applies** (a rename whose Python name is gone), so it cannot rot into a blanket exemption. ## Not passing vacuously A broken introspection step must not read as parity, so this fails loudly in three places instead of returning an empty set: every extractor in `scripts/python-surface.mjs` throws on an empty or unrecognized result; the snapshot loader rejects any empty section; and the TypeScript side asserts each introspected group is non-empty before diffing. `models.py` growing a third public class is a hard failure too, rather than a surface the check silently does not look at. ## Verification of the failure modes Each assertion was exercised against a deliberately mutated snapshot, confirming it fails and names the symbol: | Mutation | Failure message | | --- | --- | | Python gains `Models.submit` | ``comfy.models: `submit` exists in the Python SDK but not in the TypeScript SDK`` | | Python gains `AsyncModels.run_async` | ``AsyncModels declares methods the synchronous class does not: [ 'run_async' ]`` | | `ProviderError` renamed | ``routerErrors: `ProviderFailure` exists in the Python SDK but not in the TypeScript SDK`` + the converse line | | `ProviderTimeout` rebucketed | ``routerErrors.ProviderTimeout: error_type is "provider_timeout" in the TypeScript SDK but "upstream_timeout" in the Python SDK`` | | `forbidden` dropped, `rate_limited` added | ``error_type: `rate_limited` exists in the Python SDK but not in the TypeScript SDK`` + the converse line | | Python renames `MissingApiKey` to match | ``the allowlist renames `MissingApiKey`, which the Python SDK no longer exports`` | | A snapshot section emptied | ``parity/python-surface.json has an empty `routerErrorClasses` … an empty section is a broken extraction, not parity`` | Two of these (the unrecognized-class guard and the empty router set) are pinned as tests in the file; the rest are assertions over the snapshot, exercised by mutating it locally. ## Coverage, including the part this does not cover Compared today: **38 symbols** — 1 `models` method, 12 router error classes, 11 `error_type` values, 14 root-exported error classes. Not compared: **24 of the package root's 39 exported symbols** (`Comfy`, `Workflow`, `Job`, `Asset`, `Output`, their factories, and the base-URL / timeout / retry constants) and the 6 non-class members of `routerErrors` (the two header-name constants, `ROUTER_ERROR_TYPES` and its two halves, `toRouterError`). Those are outside the three criteria this check was written to, and several of them are constants whose *values* would matter more than their names. Widening it is a deliberate next step, not an oversight — see Residual. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm lint` clean; `pnpm format:check` clean; `pnpm typecheck` clean; `pnpm test` 365 passed / 0 failed across 21 files; `node scripts/check-spec-drift.mjs` in sync; `node scripts/check-public-repo-hygiene.mjs` clean; `node scripts/sync-python-surface.mjs` matches the Python SDK's default branch; `pnpm build` + `pnpm pack` succeed and the tarball contains no `parity/` or `surface-parity` entries; and the seven failure modes tabled above each reproduced against a mutated snapshot. - **Deviations:** none against the stated criteria. The scope calls this check surface-only, and the divergences listed under Residual are behavioural, so they are reported rather than fixed here. ## Residual Not fixed by this PR, and each worth its own follow-up: 1. **The two SDKs' status-to-bucket fallback maps genuinely disagree.** When a response carries no `X-Comfy-Error-Type` at all, TypeScript's `ERROR_TYPE_BY_STATUS` maps `400` and `422` to `invalid_input`; the Python SDK deliberately omits both, its comment reasoning that the contract pins no single bucket to either and that guessing between `invalid_input` and `content_policy_violation` would tell a caller to retry a deterministic refusal. Both sides also handle a `499` the same way, so this is specifically the `400`/`422` pair. One of the two is wrong and a caller's `except`/`catch` branch differs by language today. This is **behaviour, not surface**, so the check added here does not and will not catch it — it was found by reading both sources while building the check. 2. **`RouterError`'s own default `error_type` differs** — `"internal_error"` in TypeScript, `""` in Python. The base class is excluded from the per-class `error_type` comparison for exactly this reason, and the exclusion is commented in the test. Also behavioural rather than a name, and it interacts with item 1: an unknown bucket carries a different default value depending on which SDK you are in. 3. **The mirror-side job was not built.** The criterion asks for the check to run in at least one of the two repositories and it runs here, but there is no Python-side equivalent reading this SDK's surface. A change landing in *this* repo is therefore caught by this repo's own `pnpm test`, while a change landing in the Python repo is caught only the next time this repo's CI runs — so drift originating on the Python side has a detection lag equal to the gap between PRs here. A scheduled run of the `sdk-parity` job, or a mirror job in the Python repo, would close that. 4. **The `sdk-parity` job is not a required check.** That is deliberate per the scope ("get it running first, then decide"), and it is worth deciding: as configured, a Python-side surface change reddens this job on unrelated PRs in this repo until somebody refreshes the snapshot. That is the intended loudness, but it is the cost that gets a parity check disabled, so it should be a conscious call. 5. **Within-language parity for the Python SDK's two clients is out of scope** and is covered by that repo's own test. This check only enforces the one-directional rule that the async `models` class introduces no method name the sync one lacks. 6. **The design documents that specify the shared namespace and the shared error table were not readable from this run** — they live in an internal workspace with no access from here. The shared set was therefore taken from the two SDKs' own source rather than from the specification they were both written against, so this check asserts that the two SDKs agree *with each other*, not that either agrees with the design document. If the two have drifted from the spec in the same direction, this check passes. 7. **The surface not compared, quantified above** (24 of 39 root exports, 6 of 18 `routerErrors` members): the workflow-API classes `Comfy`, `Workflow`, `Job`, `Asset`, `Output` and their factories have no name-parity assertion, so a method added to `Workflow` in one SDK and not the other is still invisible. --- <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:24 +00:00