[PR #167] [MERGED] fix(parity): track the Python SDK's refactors, and retry the sdk-parity fetch #167

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/sdk-parity-track-python-refactors

📝 Commits (2)

c1fc9c2 fix(parity): track the Python SDK's refactors instead of reading them as drift
fb4d665 fix(ci): retry the sdk-parity fetch so a network blip is not read as drift

📊 Changes

6 files changed (+457 additions, -56 deletions)

View changed files

📝 AGENTS.md (+26 -16)
📝 parity/python-surface.json (+17 -3)
📝 scripts/python-surface.mjs (+81 -11)
📝 scripts/sync-python-surface.mjs (+65 -10)
📝 src/sdk/surface-parity.test.ts (+171 -16)
➕ src/sdk/sync-python-surface.test.ts (+97 -0)

📄 Description

ELI-5

The sdk-parity job checks that this SDK and the Python SDK still look alike. It does that by reading the Python SDK's source over the network and comparing names.

It has been red for four runs, and neither cause was a real difference between the two SDKs:

The Python SDK moved some code around. Our reader didn't understand the new arrangement, so it reported "Python lost four error classes" — which looks exactly like a genuine problem.
The most recent run failed because the network hiccuped while downloading the file.

Both are fixed here: the reader now understands the new arrangement, and the download retries before giving up. Along the way the refreshed comparison surfaced three real differences, which are now written down instead of being invisible.

What was actually broken

1. `RouterRunResult` was read as a third namespace

models.py grew RouterRunResult — the envelope Models.run_detailed returns. extractModelsMethods's guard exists to catch a third namespace class appearing, which a result record is not, so it failed hard.

Adding it to PYTHON_MODELS_CLASSES would have been wrong (it has no methods, so the extractor's own "yielded zero public methods" guard would fire). Data classes are recognized separately now, via PYTHON_MODELS_DATA_CLASSES, with a rot guard so an entry the Python module no longer defines fails rather than lingering.

2. The extractor couldn't see re-exports — and the miss looked like drift

exceptions.py stopped defining ComfyError, Forbidden, InsufficientCredits and Unauthorized, and started re-exporting them from _errors / router_exceptions. That is deliberate on the Python side — it is how the two modules came to share one class object per name instead of two classes wearing one name.

extractExportedErrorClasses only read class statements, so it missed all four. The failure mode is the important part: it did not look broken, it looked like the Python root had dropped four error classes — the same shape a real divergence takes. The extractor now follows relative re-exports too.

Three real differences this surfaced

Refreshing the snapshot made three names visible for the first time, so they go in the allowlist as what they are. Three new fields, pointing the opposite way to the existing LEAD fields:

Name	Field	What it means
`run_detailed`	`pythonOnlyModelsMethods`	Divergence. Our `run` already returns an envelope, so the disclosure rides the ordinary return value. A second method would be the same data behind a longer name.
`RouterError`	`pythonOnlyExportedErrorClasses`	Divergence. Same class, different address — we namespace it as `routerErrors.RouterError`. The guard proves it is really there.
`AlreadyCompleted`, `CancelRefused`	`exportedErrorClassesBehindPython`	A genuine lag. We have neither class, at the root or in `routerErrors`.

That last row is a real gap worth a follow-up: RequestHandle.cancel exists here, but its refusals surface untyped, so a caller cannot tell "already finished" from "refused" without reading a status. It is recorded rather than filtered so it stays visible, and its guard is the strictest of the three — it insists the name is missing from both addresses, so the entry can never decay into a general exemption.

Also deletes queue-error-buckets-land-first-in-typescript: its rot guard fired on the refreshed snapshot because the Python twin landed. That is the mechanism working, not a gap.

The retry

Second commit, separable if you'd rather split it — kept together because both changes serve the same property: a red from sdk-parity should mean the sibling SDK moved.

The 2026-09-20 run died on TypeError: fetch failed / ECONNRESET from raw.githubusercontent.com, which is indistinguishable from a real rename at a glance. fetchSource now makes four attempts over a widening backoff (under two seconds on a healthy run). A transport failure or a 5xx retries; a 4xx does not — that is a definite answer the caller needs promptly, and retrying a 404 just spends four attempts arriving at the same "the file moved" while making it look flaky.

To test this rather than assert it, fetchSource is exported and await main() is guarded so the module only self-runs as a CLI — importing it must not reach the network or rewrite the snapshot. CLI behavior is unchanged.

Consumer impact

None. Nothing under src/ ships differently: the only src/ changes are a test file and a new test file. No export, type, or default changes. This is tooling, tests and docs, so per CONTRIBUTING.md there is no CHANGELOG entry.

Test plan

Every gate the CI runs, locally, on Node 26:

pnpm lint
pnpm format:check
pnpm typecheck
pnpm check:spec-drift
pnpm check:sdk-parity — now OK against comfy-python-sdk@main
pnpm test — 650 passed, 5 skipped (was 645; +5 new retry tests)
pnpm build
node scripts/check-public-repo-hygiene.mjs

New tests cover the happy path with no retry, riding out three transport blips, giving up after the fourth attempt, retrying a 503, and not retrying a 404 — the transport failure uses the real undici shape (TypeError: fetch failed with an ECONNRESET cause).

One note on process: the router-errors-namespaced entry originally claimed all three of RouterError, AlreadyCompleted and CancelRefused were namespaced. The new rot guard rejected that claim, which is how the lag in row three got found rather than papered over.

_{🔄 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/167 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/20/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/sdk-parity-track-python-refactors` --- ### 📝 Commits (2) - [`c1fc9c2`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/c1fc9c202bfd5bdc503cf1b3a2e10def8dcc8dc1) fix(parity): track the Python SDK's refactors instead of reading them as drift - [`fb4d665`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/fb4d665f0e1999de3271600199466db1f9accf26) fix(ci): retry the sdk-parity fetch so a network blip is not read as drift ### 📊 Changes **6 files changed** (+457 additions, -56 deletions) <details> <summary>View changed files</summary> 📝 `AGENTS.md` (+26 -16) 📝 `parity/python-surface.json` (+17 -3) 📝 `scripts/python-surface.mjs` (+81 -11) 📝 `scripts/sync-python-surface.mjs` (+65 -10) 📝 `src/sdk/surface-parity.test.ts` (+171 -16) ➕ `src/sdk/sync-python-surface.test.ts` (+97 -0) </details> ### 📄 Description ## ELI-5 The `sdk-parity` job checks that this SDK and the Python SDK still look alike. It does that by reading the Python SDK's source over the network and comparing names. It has been red for four runs, and **neither cause was a real difference between the two SDKs**: 1. The Python SDK moved some code around. Our reader didn't understand the new arrangement, so it reported "Python lost four error classes" — which looks exactly like a genuine problem. 2. The most recent run failed because the network hiccuped while downloading the file. Both are fixed here: the reader now understands the new arrangement, and the download retries before giving up. Along the way the refreshed comparison surfaced three real differences, which are now written down instead of being invisible. ## What was actually broken ### 1. `RouterRunResult` was read as a third namespace `models.py` grew `RouterRunResult` — the envelope `Models.run_detailed` returns. `extractModelsMethods`'s guard exists to catch a third *namespace* class appearing, which a result record is not, so it failed hard. Adding it to `PYTHON_MODELS_CLASSES` would have been wrong (it has no methods, so the extractor's own "yielded zero public methods" guard would fire). Data classes are recognized separately now, via `PYTHON_MODELS_DATA_CLASSES`, with a rot guard so an entry the Python module no longer defines fails rather than lingering. ### 2. The extractor couldn't see re-exports — and the miss looked like drift `exceptions.py` stopped *defining* `ComfyError`, `Forbidden`, `InsufficientCredits` and `Unauthorized`, and started re-exporting them from `_errors` / `router_exceptions`. That is deliberate on the Python side — it is how the two modules came to share one class object per name instead of two classes wearing one name. `extractExportedErrorClasses` only read `class` statements, so it missed all four. **The failure mode is the important part: it did not look broken, it looked like the Python root had dropped four error classes** — the same shape a real divergence takes. The extractor now follows relative re-exports too. ## Three real differences this surfaced Refreshing the snapshot made three names visible for the first time, so they go in the allowlist as what they are. Three new fields, pointing the opposite way to the existing LEAD fields: | Name | Field | What it means | |---|---|---| | `run_detailed` | `pythonOnlyModelsMethods` | **Divergence.** Our `run` already returns an envelope, so the disclosure rides the ordinary return value. A second method would be the same data behind a longer name. | | `RouterError` | `pythonOnlyExportedErrorClasses` | **Divergence.** Same class, different address — we namespace it as `routerErrors.RouterError`. The guard proves it is really there. | | `AlreadyCompleted`, `CancelRefused` | `exportedErrorClassesBehindPython` | **A genuine lag.** We have neither class, at the root or in `routerErrors`. | That last row is a real gap worth a follow-up: `RequestHandle.cancel` exists here, but its refusals surface untyped, so a caller cannot tell "already finished" from "refused" without reading a status. It is recorded rather than filtered so it stays visible, and its guard is the strictest of the three — it insists the name is missing from *both* addresses, so the entry can never decay into a general exemption. Also deletes `queue-error-buckets-land-first-in-typescript`: its rot guard fired on the refreshed snapshot because the Python twin landed. That is the mechanism working, not a gap. ## The retry Second commit, separable if you'd rather split it — kept together because both changes serve the same property: **a red from `sdk-parity` should mean the sibling SDK moved.** The 2026-09-20 run died on `TypeError: fetch failed` / `ECONNRESET` from `raw.githubusercontent.com`, which is indistinguishable from a real rename at a glance. `fetchSource` now makes four attempts over a widening backoff (under two seconds on a healthy run). A transport failure or a 5xx retries; **a 4xx does not** — that is a definite answer the caller needs promptly, and retrying a 404 just spends four attempts arriving at the same "the file moved" while making it look flaky. To test this rather than assert it, `fetchSource` is exported and `await main()` is guarded so the module only self-runs as a CLI — importing it must not reach the network or rewrite the snapshot. CLI behavior is unchanged. ## Consumer impact **None.** Nothing under `src/` ships differently: the only `src/` changes are a test file and a new test file. No export, type, or default changes. This is tooling, tests and docs, so per CONTRIBUTING.md there is no CHANGELOG entry. ## Test plan Every gate the CI runs, locally, on Node 26: - [x] `pnpm lint` - [x] `pnpm format:check` - [x] `pnpm typecheck` - [x] `pnpm check:spec-drift` - [x] `pnpm check:sdk-parity` — now **OK** against `comfy-python-sdk@main` - [x] `pnpm test` — 650 passed, 5 skipped (was 645; +5 new retry tests) - [x] `pnpm build` - [x] `node scripts/check-public-repo-hygiene.mjs` New tests cover the happy path with no retry, riding out three transport blips, giving up after the fourth attempt, retrying a 503, and *not* retrying a 404 — the transport failure uses the real `undici` shape (`TypeError: fetch failed` with an `ECONNRESET` cause). One note on process: the `router-errors-namespaced` entry originally claimed all three of `RouterError`, `AlreadyCompleted` and `CancelRefused` were namespaced. The new rot guard rejected that claim, which is how the lag in row three got found rather than papered over. --- <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:15 +00:00