[PR #100] [MERGED] test: gate the Router run route and default host against the vendored spec #103

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-9936-router-spec-contract

📝 Commits (1)

6c23f26 test: gate the Router run route and default host against the vendored spec

📊 Changes

9 files changed (+492 additions, -21 deletions)

View changed files

📝 .github/workflows/ci.yml (+1 -1)
📝 AGENTS.md (+23 -6)
📝 CHANGELOG.md (+10 -0)
📝 README.md (+2 -0)
📝 scripts/check-spec-drift.mjs (+100 -9)
➕ scripts/router-route-contract.mjs (+189 -0)
📝 spec/README.md (+6 -1)
📝 src/sdk/models.ts (+55 -4)
➕ src/sdk/router-spec-contract.test.ts (+106 -0)

📄 Description

ELI-5

comfy.models.run knows two things by heart: the path it posts to (/v1/models/{provider}/{model}) and the host it posts to (https://api.comfy.org). This repo also carries a vendored copy of the Comfy Router contract that declares both — but nothing read the route out of it, so if a sync PR ever moved the route, CI would stay green and run would quietly start 404ing. This PR makes the two agree by force: the path becomes a named constant, and two checks now compare it (and the host) against the vendored spec. Nothing about the request changes.

What changed

src/sdk/models.ts — the inline path becomes export const RUN_ROUTE_TEMPLATE = "/v1/models/{provider}/{model}", filled by a module-private fillRoute(template, values) that runs encodeURIComponent per segment, exactly as the interpolated string did. An unknown placeholder throws rather than shipping a literal {...} in a URL. ModelId became a type so it carries the implicit index signature fillRoute needs (no cast).
src/sdk/router-spec-contract.test.ts (new) — compares the live constants against spec/router-openapi.yaml: the path declared for post.operationId: runRouterModel vs RUN_ROUTE_TEMPLATE, servers[0].url vs COMFY_ROUTER_BASE_URL, and the path parameters (provider then model, order included, $refs resolved) vs the template's placeholders. Two further cases drive a real request through the stub server so the constant is proved to be the one runUrl actually uses, and that per-segment percent-encoding survived the extraction.
scripts/check-spec-drift.mjs — makes the same comparisons inside pnpm check:spec-drift, so the drift job reddens by name rather than only the unit suite. That job is plain Node with no TypeScript loader, so it reads the two constants out of their source text. Its two checks (codegen diff, Router route) now both run and both report on every invocation, so one failure cannot hide the other.
scripts/router-route-contract.mjs (new) — the shared spec reader, mirroring how scripts/python-surface.mjs is shared with the parity test. Every extractor in it throws rather than returning nothing: a drift check that silently reads nothing reports agreement, which is the one failure mode it must not have.
Docs — a sentence under comfy.models.run in the README naming the file and the two constants to update; a CHANGELOG entry under Unreleased → Changed (marked no-behaviour-change); spec/README.md and AGENTS.md §1–2 updated, since both stated that the error-bucket table was the only thing coupled to this contract and that check-spec-drift.mjs read nothing from it. The CI step name changed from Check generated code is in sync with spec/openapi.yaml to Check the code coupled to spec/ is in sync with it, since it now covers both specs (AGENTS.md quoted the old name and was updated with it).

Acceptance, exercised

Both halves of the stated criterion were run, in both directions, on the real vendored spec.

Green on the unmodified tree: pnpm test 406 passed / 23 files; pnpm check:spec-drift OK: src/low/generated is in sync with spec/openapi.yaml + OK: the Router route matches spec/router-openapi.yaml; pnpm lint, pnpm format:check, pnpm typecheck, pnpm build, node scripts/check-public-repo-hygiene.mjs, pnpm check:sdk-parity (network) all clean.

Red when spec/router-openapi.yaml's runRouterModel path is temporarily edited to /v2/models/{provider}/{model} — the spec was restored afterwards and git status on spec/ is clean:

pnpm test → 1 failed / 23 files, AssertionError: the vendored Router contract moved the runRouterModel path — update RUN_ROUTE_TEMPLATE in src/sdk/models.ts to match it (comfy.models.run 404s until you do).
pnpm check:spec-drift → exit 1, RUN_ROUTE_TEMPLATE (src/sdk/models.ts) is "/v1/models/{provider}/{model}", but spec/router-openapi.yaml declares "/v2/models/{provider}/{model}" for post.operationId: runRouterModel.

The other two gates were exercised the same way and each fires alone: editing servers[0].url names COMFY_ROUTER_BASE_URL, and swapping the two $refd path parameters names RUN_ROUTE_TEMPLATE and prints the order it expected.

Sweep

The vendored Router contract declares 4 operations. This SDK calls exactly 1 of them (runRouterModel) and this PR pins that one. The other 3 — listRouterModels, getRouterModel, getRouterModelInputSchema — have no call site in this repo, so there is no constant to drift and nothing to pin; if the SDK grows one, it needs the same treatment. A grep across src/, test/ and scripts/ finds 1 production spelling of the route (now RUN_ROUTE_TEMPLATE) and 0 other call sites; the remaining hits are prose, a test's expected path, and the stub server's header comment. The stub answers any path, so it is not coupled to the route.

Judgment calls

RUN_ROUTE_TEMPLATE is exported from src/sdk/models.ts but deliberately NOT re-exported from src/sdk/index.ts. It is the anchor for a drift gate, not a knob a caller turns — the route is the SDK's business and comfy.config({ baseUrl }) is how a caller retargets the host. Publishing it would make an internal coupling point semver-relevant and would put a TypeScript-only name onto the cross-SDK surface. The package's public surface is therefore unchanged, the parity check needed no allowlist entry, and pnpm check:sdk-parity is green. Say the word if you'd rather it were public.
The drift script reads the constants by regex over source text, not by importing them. check:spec-drift runs under plain Node in CI with no TypeScript loader and no build ahead of it. The regex requires the export const X = "…"; form on one line and throws with a message naming the constant if it does not find it, so a future reformat fails loudly rather than silently passing. The unit test imports the real values, which is the half that proves the runtime value.
The plan this came from pointed at a YAML reader in scripts/check-spec-drift.mjs; that file had none. The existing reader lives in src/sdk/router-spec-coverage.test.ts. I used the yaml dev dependency the same way it does — the alternative the plan named.
No capability is denied by this change. The one new throw (fillRoute on an unknown placeholder) is unreachable from any caller: RUN_ROUTE_TEMPLATE declares exactly {provider} and {model}, and ModelId is typechecked to carry exactly those two keys. I verified empirically rather than by assertion that the capability it could conceivably block still works: comfy.models.run completes end to end against the stub server in the pre-existing suite and in two new cases that read the request path off the wire (/v1/models/fal-ai/flux-pro, and /v1/models/fal%20ai/flux%23pro for the encoding). The new CI gate only blocks a spec sync that moves the route without updating the constant, and it is green against today's real vendored spec.

Residual

Not fixed here, and worth its own tracking:

The route move itself is not made. This PR only makes it fail loudly. When the upstream Router route actually moves and the automated spec sync updates spec/router-openapi.yaml, that sync PR will be red until RUN_ROUTE_TEMPLATE in src/sdk/models.ts is updated in the same PR. That is the intended outcome, but it does mean the sync is no longer a merge-on-green change for this repo, and whoever handles it needs to know that. The README, spec/README.md and AGENTS.md §2 all name the constant so the failure is self-servicing.
Unexercised artifact: the spec-sync workflow. The automation that would deliver such a change (push-v2-spec-to-sdks.yml, its typescript-sdk-router row) lives in a different repository and is not present in this tree, so I could not read or run it. I have therefore not verified that a real sync PR rewrites spec/router-openapi.yaml in the shape this gate assumes — a change to the paths key and/or servers[0].url. If a sync instead, say, adds a /v2 path alongside the /v1 one and leaves runRouterModel on the old key, this gate stays green and the SDK keeps calling the old route. Worth confirming against that workflow.
servers[0] only. The host comparison reads the first server entry. A contract that grows a second entry, or reorders them, is not covered.
The contract's other three operations are unpinned, as described under Sweep — nothing is wrong today, but the gate does not generalize to them on its own.

Provenance

Authored by: agent-work loop
Verified: pnpm test 406 passed / 0 failed (23 files); pnpm check:spec-drift both checks OK, and exit 1 naming RUN_ROUTE_TEMPLATE under a simulated /v2 move; pnpm lint, pnpm format:check, pnpm typecheck, pnpm build clean; node scripts/check-public-repo-hygiene.mjs OK; pnpm check:sdk-parity matches the Python SDK's main.
Deviations: none of the plan's five steps were skipped. Step 5 (cross-SDK parity) needed no allowlist entry, because the new constant was kept off the package's public export surface — see Judgment calls.

_{🔄 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/100 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/27/2026 **Status:** ✅ Merged **Merged:** 8/27/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9936-router-spec-contract` --- ### 📝 Commits (1) - [`6c23f26`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/6c23f263bf60049f83f30646e6d0d58295406320) test: gate the Router run route and default host against the vendored spec ### 📊 Changes **9 files changed** (+492 additions, -21 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci.yml` (+1 -1) 📝 `AGENTS.md` (+23 -6) 📝 `CHANGELOG.md` (+10 -0) 📝 `README.md` (+2 -0) 📝 `scripts/check-spec-drift.mjs` (+100 -9) ➕ `scripts/router-route-contract.mjs` (+189 -0) 📝 `spec/README.md` (+6 -1) 📝 `src/sdk/models.ts` (+55 -4) ➕ `src/sdk/router-spec-contract.test.ts` (+106 -0) </details> ### 📄 Description ## ELI-5 `comfy.models.run` knows two things by heart: the path it posts to (`/v1/models/{provider}/{model}`) and the host it posts to (`https://api.comfy.org`). This repo also carries a vendored copy of the Comfy Router contract that declares both — but nothing read the route out of it, so if a sync PR ever moved the route, CI would stay green and `run` would quietly start 404ing. This PR makes the two agree by force: the path becomes a named constant, and two checks now compare it (and the host) against the vendored spec. Nothing about the request changes. ## What changed - **`src/sdk/models.ts`** — the inline path becomes `export const RUN_ROUTE_TEMPLATE = "/v1/models/{provider}/{model}"`, filled by a module-private `fillRoute(template, values)` that runs `encodeURIComponent` per segment, exactly as the interpolated string did. An unknown placeholder throws rather than shipping a literal `{...}` in a URL. `ModelId` became a `type` so it carries the implicit index signature `fillRoute` needs (no cast). - **`src/sdk/router-spec-contract.test.ts`** (new) — compares the **live** constants against `spec/router-openapi.yaml`: the path declared for `post.operationId: runRouterModel` vs `RUN_ROUTE_TEMPLATE`, `servers[0].url` vs `COMFY_ROUTER_BASE_URL`, and the path parameters (`provider` then `model`, order included, `$ref`s resolved) vs the template's placeholders. Two further cases drive a **real request** through the stub server so the constant is proved to be the one `runUrl` actually uses, and that per-segment percent-encoding survived the extraction. - **`scripts/check-spec-drift.mjs`** — makes the same comparisons inside `pnpm check:spec-drift`, so the drift job reddens by name rather than only the unit suite. That job is plain Node with no TypeScript loader, so it reads the two constants out of their source text. Its two checks (codegen diff, Router route) now both run and both report on every invocation, so one failure cannot hide the other. - **`scripts/router-route-contract.mjs`** (new) — the shared spec reader, mirroring how `scripts/python-surface.mjs` is shared with the parity test. Every extractor in it throws rather than returning nothing: a drift check that silently reads nothing reports agreement, which is the one failure mode it must not have. - **Docs** — a sentence under `comfy.models.run` in the README naming the file and the two constants to update; a CHANGELOG entry under Unreleased → Changed (marked no-behaviour-change); `spec/README.md` and `AGENTS.md` §1–2 updated, since both stated that the error-bucket table was the *only* thing coupled to this contract and that `check-spec-drift.mjs` read nothing from it. The CI step name changed from `Check generated code is in sync with spec/openapi.yaml` to `Check the code coupled to spec/ is in sync with it`, since it now covers both specs (`AGENTS.md` quoted the old name and was updated with it). ## Acceptance, exercised Both halves of the stated criterion were run, in both directions, on the real vendored spec. Green on the unmodified tree: `pnpm test` 406 passed / 23 files; `pnpm check:spec-drift` `OK: src/low/generated is in sync with spec/openapi.yaml` + `OK: the Router route matches spec/router-openapi.yaml`; `pnpm lint`, `pnpm format:check`, `pnpm typecheck`, `pnpm build`, `node scripts/check-public-repo-hygiene.mjs`, `pnpm check:sdk-parity` (network) all clean. Red when `spec/router-openapi.yaml`'s `runRouterModel` path is temporarily edited to `/v2/models/{provider}/{model}` — the spec was restored afterwards and `git status` on `spec/` is clean: - `pnpm test` → 1 failed / 23 files, `AssertionError: the vendored Router contract moved the runRouterModel path — update RUN_ROUTE_TEMPLATE in src/sdk/models.ts to match it (comfy.models.run 404s until you do)`. - `pnpm check:spec-drift` → exit 1, `RUN_ROUTE_TEMPLATE (src/sdk/models.ts) is "/v1/models/{provider}/{model}", but spec/router-openapi.yaml declares "/v2/models/{provider}/{model}" for post.operationId: runRouterModel.` The other two gates were exercised the same way and each fires alone: editing `servers[0].url` names `COMFY_ROUTER_BASE_URL`, and swapping the two `$ref`d path parameters names `RUN_ROUTE_TEMPLATE` and prints the order it expected. ## Sweep The vendored Router contract declares **4** operations. This SDK calls exactly **1** of them (`runRouterModel`) and this PR pins that one. The other **3** — `listRouterModels`, `getRouterModel`, `getRouterModelInputSchema` — have no call site in this repo, so there is no constant to drift and nothing to pin; if the SDK grows one, it needs the same treatment. A grep across `src/`, `test/` and `scripts/` finds **1** production spelling of the route (now `RUN_ROUTE_TEMPLATE`) and **0** other call sites; the remaining hits are prose, a test's expected path, and the stub server's header comment. The stub answers any path, so it is not coupled to the route. ## Judgment calls - **`RUN_ROUTE_TEMPLATE` is exported from `src/sdk/models.ts` but deliberately NOT re-exported from `src/sdk/index.ts`.** It is the anchor for a drift gate, not a knob a caller turns — the route is the SDK's business and `comfy.config({ baseUrl })` is how a caller retargets the host. Publishing it would make an internal coupling point semver-relevant and would put a TypeScript-only name onto the cross-SDK surface. The package's public surface is therefore unchanged, the parity check needed no allowlist entry, and `pnpm check:sdk-parity` is green. Say the word if you'd rather it were public. - **The drift script reads the constants by regex over source text, not by importing them.** `check:spec-drift` runs under plain Node in CI with no TypeScript loader and no build ahead of it. The regex requires the `export const X = "…";` form on one line and throws with a message naming the constant if it does not find it, so a future reformat fails loudly rather than silently passing. The unit test imports the real values, which is the half that proves the runtime value. - **The plan this came from pointed at a YAML reader in `scripts/check-spec-drift.mjs`; that file had none.** The existing reader lives in `src/sdk/router-spec-coverage.test.ts`. I used the `yaml` dev dependency the same way it does — the alternative the plan named. - **No capability is denied by this change.** The one new `throw` (`fillRoute` on an unknown placeholder) is unreachable from any caller: `RUN_ROUTE_TEMPLATE` declares exactly `{provider}` and `{model}`, and `ModelId` is typechecked to carry exactly those two keys. I verified empirically rather than by assertion that the capability it could conceivably block still works: `comfy.models.run` completes end to end against the stub server in the pre-existing suite and in two new cases that read the request path off the wire (`/v1/models/fal-ai/flux-pro`, and `/v1/models/fal%20ai/flux%23pro` for the encoding). The new CI gate only blocks a spec sync that moves the route without updating the constant, and it is green against today's real vendored spec. ## Residual Not fixed here, and worth its own tracking: - **The route move itself is not made.** This PR only makes it fail loudly. When the upstream Router route actually moves and the automated spec sync updates `spec/router-openapi.yaml`, that sync PR will be red until `RUN_ROUTE_TEMPLATE` in `src/sdk/models.ts` is updated in the same PR. That is the intended outcome, but it does mean the sync is no longer a merge-on-green change for this repo, and whoever handles it needs to know that. The README, `spec/README.md` and `AGENTS.md` §2 all name the constant so the failure is self-servicing. - **Unexercised artifact: the spec-sync workflow.** The automation that would deliver such a change (`push-v2-spec-to-sdks.yml`, its `typescript-sdk-router` row) lives in a different repository and is not present in this tree, so I could not read or run it. I have therefore **not** verified that a real sync PR rewrites `spec/router-openapi.yaml` in the shape this gate assumes — a change to the `paths` key and/or `servers[0].url`. If a sync instead, say, adds a `/v2` path alongside the `/v1` one and leaves `runRouterModel` on the old key, this gate stays green and the SDK keeps calling the old route. Worth confirming against that workflow. - **`servers[0]` only.** The host comparison reads the first server entry. A contract that grows a second entry, or reorders them, is not covered. - **The contract's other three operations are unpinned**, as described under Sweep — nothing is wrong today, but the gate does not generalize to them on its own. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm test` 406 passed / 0 failed (23 files); `pnpm check:spec-drift` both checks OK, and exit 1 naming `RUN_ROUTE_TEMPLATE` under a simulated `/v2` move; `pnpm lint`, `pnpm format:check`, `pnpm typecheck`, `pnpm build` clean; `node scripts/check-public-repo-hygiene.mjs` OK; `pnpm check:sdk-parity` matches the Python SDK's `main`. - **Deviations:** none of the plan's five steps were skipped. Step 5 (cross-SDK parity) needed no allowlist entry, because the new constant was kept off the package's public export surface — see Judgment calls. --- <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:35 +00:00