MASK socket #160

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-15792-asset-handle-filename-docs

📝 Commits (5)

4aaeca9 docs(assets): state that an asset handle binds only to a filename input, never an IMAGE/VIDEO/AUDIO socket
37a69f4 docs(assets): list MASK too, and say the socket rule is unenforced
3ee8b33 docs(changelog): drop the entry, matching how docs-only changes are recorded
44ba0a7 docs(quickstart): say node "10" is a LoadImage node in the three quickstart snippets
8844916 docs: say what to bind a handle to in every quickstart copy

📊 Changes

5 files changed (+31 additions, -5 deletions)

View changed files

📝 README.md (+17 -1)
📝 src/index.ts (+1 -1)
📝 src/sdk/assets.ts (+6 -1)
📝 src/sdk/client.ts (+1 -1)
📝 src/sdk/workflows.ts (+6 -1)

📄 Description

ELI-5

If you hand the SDK a photo, it turns into a filename by the time it reaches the server — not an actual decoded image. So you have to give that filename to a node that expects a filename (a loader like LoadImage), and then wire the loader's picture output into whatever node actually needs the picture. If you instead shove the handle straight onto a node's "image" slot, the server says "OK" when you submit but the job blows up later because that node was handed a piece of text where it wanted a picture. This PR writes that rule down in the docs so nobody hits the trap again.

Summary

Docs-only change. The README's Assets example bound an asset handle to a LoadImage node but never said that a loader's file widget is the only kind of input a handle may go on. An external team bound handles directly to a partner node's grouped model.reference_images.image_1 / model.reference_audios.audio_1 inputs, got HTTP 200 at submit, and the job died at execution with 'str' object has no attribute 'shape'.

The reason is server-side: the public API rewrites every core/ASSET to a filename string, and inference stages files only for loader (class, field) pairs — any other literal is passed straight through to the node as a string. This PR documents the constraint in the places a user would look (README Assets section and quickstart, Workflow.setInput, Asset, and the package-level quickstart TSDoc).

Changes

README.md — new paragraph in the "Assets" section, directly after the wf.setInput("10", "image", asset) example, plus a two-node LoadImage → consumer example showing the handle bound to the loader's image widget and the loader's output (["10", 0]) linked into the consumer.
src/sdk/workflows.ts — setInput JSDoc now notes a handle must be set on a filename input, not an IMAGE/VIDEO/AUDIO/MASK socket, and states plainly that nothing validates this: the binding is accepted at submit and fails at execution.
src/sdk/assets.ts — the Asset doc comment carries the same rule.
Quickstart snippets (README.md quickstart, and the TSDoc copies in src/sdk/client.ts and src/index.ts, which ship in the package) — wf.setInput("10", "image", asset) now carries a comment saying node "10" is a LoadImage node (44ba0a7); the two shipped TSDoc copies add "bind handles to a loader's file widget", and the first bare setInput in the README Assets section points to the explanation below it (8844916).
CHANGELOG.md — unchanged. A ### Docs entry was added on the first
commit and removed again in 3ee8b33: Docs is not one of the six sections
CONTRIBUTING and the file's own comment enumerate, and every docs-only PR
since the changelog was introduced in #61 (#113, #116, #119) added no entry
at all. The README and TSDoc changes above are the record.

Review round (37a69f4) addressed two findings raised on the first commit: MASK was listed in the README but missing from both doc comments, so the README and both doc comments now name the same sockets (the CHANGELOG entry that also carried the mismatch has since been dropped, see above); and "it cannot be set on an IMAGE/VIDEO/AUDIO socket" claimed an enforcement the SDK does not perform — setInput writes straight to node.inputs[field] and substituteAssetHandles recurses the whole graph with no socket awareness — so the wording is now "must be set on …" plus the unenforced / fails-at-execution caveat.

No runtime behaviour changes. The SDK has no node schema (the v2 API exposes none), so client-side rejection of a mis-bound handle is deliberately out of scope — this is documentation of an existing server-side constraint, not new enforcement.

Testing

Re-run at 44ba0a7 in a clean worktree after pnpm install --frozen-lockfile:

pnpm format:check — clean (85 files)
pnpm lint — clean (exit 0)
pnpm typecheck — clean
pnpm test — 642 passed, 5 skipped (27 files passed, 1 skipped)
pnpm build — clean
node scripts/check-public-repo-hygiene.mjs — OK

Provenance

Authored by: agent-work loop
Verified: re-run at 8844916 on 2026-09-27 in a clean worktree: format:check clean (85 files); lint exit 0; typecheck clean; test 642 passed / 5 skipped; public-repo-hygiene OK. The build was last run at 44ba0a7; 8844916 changes only comments and README text. check:sdk-parity fails the same way on this branch and on main (see Deviations).
Deviations: The ### Docs heading previously flagged here is resolved — the entry was removed in 3ee8b33 and CHANGELOG.md is now net-unchanged by this PR. Remaining: the sdk-parity (python surface freshness) check is red on this PR, but it is not caused by this docs-only diff — this branch touches no Python, extractor or parity file. That job re-derives the Python SDK's public surface from Comfy-Org/comfy-python-sdk@main, and that repo landed a public RouterRunResult class in src/comfy_sdk/models.py which this repo's extractor rejects by design (scripts/python-surface.mjs pins PYTHON_MODELS_CLASSES to Models / AsyncModels so an unknown third class is a hard failure rather than a silently unchecked surface). The same ExtractionError fails main's own daily scheduled run — every day since 2026-09-18, still red on 2026-09-27 (run 36303209408) — and it is tracked by issue #156 (with an open fix in #167), where the fix belongs (deciding whether RouterRunResult is part of the models namespace surface or an intentional asymmetry is a call for that issue, not for a docs PR). sdk-parity is not a required status check: the five required checks — Test (node 22), Test (node 24), build-check (publish dry run), public-repo-hygiene and cla-assistant — all pass on this PR (re-read 2026-10-04; main's scheduled run was still red the same way on 2026-10-03, run 37106824538).

Residual

The negative claim ("fails at execution because the node receives a string") was not independently reproduced here. It rests on the reported repro: an external team bound handles to a partner node's grouped sockets, got a 200 at submit, and the job died with 'str' object has no attribute 'shape'. Reproducing it requires a live Comfy Cloud server and the partner node, which this SDK repo and worktree cannot exercise; no code path in this diff enforces the claim, so it is documentation of server behaviour grounded in that report.
The server-side rule was not exercised. The rewrite of core/ASSET → filename and the loader-only file-staging logic live in the cloud backend (cloud/common/file_extractor.go, FileNodeFieldName), a different repo not accessible from this sandbox. The paragraph paraphrases that behaviour from the sourcing investigation rather than from reading the code here.
The equivalent docs update in Comfy-Org/docs is not included. The same example appears in development/api-development/sdks.mdx (and its ja/ko/zh mirrors) and needs the same paragraph. That repo is out of scope for this change and is a manual follow-on.
The originating investigation ticket could not be read. The bug context originates from a related tracker issue that is not reachable from this environment; the change relies on the description and root cause carried into this task rather than on re-reading that issue.

Summary by CodeRabbit

Documentation
- Clarified that asset handles resolve to server-side filenames and should be assigned to filename inputs on loader nodes.
- Explained that assigning an asset handle directly to an image, video, audio, or mask input may be accepted at submission but fail during execution.
- Updated examples to show how to connect a loader’s output and pass an asset to an input.

_{🔄 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/159 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15792-asset-handle-filename-docs` --- ### 📝 Commits (5) - [`4aaeca9`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/4aaeca9ceb83047cb1960ee247f35d8d43320c19) docs(assets): state that an asset handle binds only to a filename input, never an IMAGE/VIDEO/AUDIO socket - [`37a69f4`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/37a69f4cda43ceec4b589fbd5343fda999ee0429) docs(assets): list MASK too, and say the socket rule is unenforced - [`3ee8b33`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/3ee8b33a23e11640013836170745d9d24a5dc17c) docs(changelog): drop the entry, matching how docs-only changes are recorded - [`44ba0a7`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/44ba0a715a255e63135529111a5e017c00f55e64) docs(quickstart): say node "10" is a LoadImage node in the three quickstart snippets - [`8844916`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/884491682f4cf26fa0c554c22b171b3bce4462c0) docs: say what to bind a handle to in every quickstart copy ### 📊 Changes **5 files changed** (+31 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+17 -1) 📝 `src/index.ts` (+1 -1) 📝 `src/sdk/assets.ts` (+6 -1) 📝 `src/sdk/client.ts` (+1 -1) 📝 `src/sdk/workflows.ts` (+6 -1) </details> ### 📄 Description ## ELI-5 If you hand the SDK a photo, it turns into a *filename* by the time it reaches the server — not an actual decoded image. So you have to give that filename to a node that expects a filename (a loader like `LoadImage`), and then wire the loader's picture output into whatever node actually needs the picture. If you instead shove the handle straight onto a node's "image" slot, the server says "OK" when you submit but the job blows up later because that node was handed a piece of text where it wanted a picture. This PR writes that rule down in the docs so nobody hits the trap again. ## Summary Docs-only change. The README's Assets example bound an asset handle to a `LoadImage` node but never said that a loader's file widget is the *only* kind of input a handle may go on. An external team bound handles directly to a partner node's grouped `model.reference_images.image_1` / `model.reference_audios.audio_1` inputs, got HTTP 200 at submit, and the job died at execution with `'str' object has no attribute 'shape'`. The reason is server-side: the public API rewrites every `core/ASSET` to a filename string, and inference stages files only for loader `(class, field)` pairs — any other literal is passed straight through to the node as a string. This PR documents the constraint in the places a user would look (README Assets section and quickstart, `Workflow.setInput`, `Asset`, and the package-level quickstart TSDoc). ## Changes - **`README.md`** — new paragraph in the "Assets" section, directly after the `wf.setInput("10", "image", asset)` example, plus a two-node `LoadImage` → consumer example showing the handle bound to the loader's `image` widget and the loader's output (`["10", 0]`) linked into the consumer. - **`src/sdk/workflows.ts`** — `setInput` JSDoc now notes a handle must be set on a filename input, not an IMAGE/VIDEO/AUDIO/MASK socket, and states plainly that nothing validates this: the binding is accepted at submit and fails at execution. - **`src/sdk/assets.ts`** — the `Asset` doc comment carries the same rule. - **Quickstart snippets** (`README.md` quickstart, and the TSDoc copies in `src/sdk/client.ts` and `src/index.ts`, which ship in the package) — `wf.setInput("10", "image", asset)` now carries a comment saying node `"10"` is a `LoadImage` node (44ba0a7); the two shipped TSDoc copies add "bind handles to a loader's file widget", and the first bare `setInput` in the README Assets section points to the explanation below it (8844916). - **`CHANGELOG.md`** — unchanged. A `### Docs` entry was added on the first commit and removed again in 3ee8b33: `Docs` is not one of the six sections CONTRIBUTING and the file's own comment enumerate, and every docs-only PR since the changelog was introduced in #61 (#113, #116, #119) added no entry at all. The README and TSDoc changes above are the record. Review round (37a69f4) addressed two findings raised on the first commit: `MASK` was listed in the README but missing from both doc comments, so the README and both doc comments now name the same sockets (the CHANGELOG entry that also carried the mismatch has since been dropped, see above); and "it cannot be set on an IMAGE/VIDEO/AUDIO socket" claimed an enforcement the SDK does not perform — `setInput` writes straight to `node.inputs[field]` and `substituteAssetHandles` recurses the whole graph with no socket awareness — so the wording is now "must be set on …" plus the unenforced / fails-at-execution caveat. No runtime behaviour changes. The SDK has no node schema (the v2 API exposes none), so client-side rejection of a mis-bound handle is deliberately out of scope — this is documentation of an existing server-side constraint, not new enforcement. ## Testing Re-run at 44ba0a7 in a clean worktree after `pnpm install --frozen-lockfile`: - `pnpm format:check` — clean (85 files) - `pnpm lint` — clean (exit 0) - `pnpm typecheck` — clean - `pnpm test` — 642 passed, 5 skipped (27 files passed, 1 skipped) - `pnpm build` — clean - `node scripts/check-public-repo-hygiene.mjs` — OK ## Provenance - **Authored by:** agent-work loop - **Verified:** re-run at 8844916 on 2026-09-27 in a clean worktree: format:check clean (85 files); lint exit 0; typecheck clean; test 642 passed / 5 skipped; public-repo-hygiene OK. The build was last run at 44ba0a7; 8844916 changes only comments and README text. `check:sdk-parity` fails the same way on this branch and on `main` (see Deviations). - **Deviations:** The `### Docs` heading previously flagged here is resolved — the entry was removed in 3ee8b33 and `CHANGELOG.md` is now net-unchanged by this PR. Remaining: the `sdk-parity (python surface freshness)` check is red on this PR, but it is **not caused by this docs-only diff** — this branch touches no Python, extractor or parity file. That job re-derives the Python SDK's public surface from `Comfy-Org/comfy-python-sdk@main`, and that repo landed a public `RouterRunResult` class in `src/comfy_sdk/models.py` which this repo's extractor rejects by design (`scripts/python-surface.mjs` pins `PYTHON_MODELS_CLASSES` to `Models` / `AsyncModels` so an unknown third class is a hard failure rather than a silently unchecked surface). The same `ExtractionError` fails `main`'s own daily scheduled run — every day since 2026-09-18, still red on 2026-09-27 ([run 36303209408](https://github.com/Comfy-Org/comfy-typescript-sdk/actions/runs/36303209408)) — and it is tracked by issue #156 (with an open fix in #167), where the fix belongs (deciding whether `RouterRunResult` is part of the `models` namespace surface or an intentional asymmetry is a call for that issue, not for a docs PR). `sdk-parity` is **not** a required status check: the five required checks — Test (node 22), Test (node 24), build-check (publish dry run), public-repo-hygiene and cla-assistant — all pass on this PR (re-read 2026-10-04; `main`'s scheduled run was still red the same way on 2026-10-03, [run 37106824538](https://github.com/Comfy-Org/comfy-typescript-sdk/actions/runs/37106824538)). ## Residual - **The negative claim ("fails at execution because the node receives a string") was not independently reproduced here.** It rests on the reported repro: an external team bound handles to a partner node's grouped sockets, got a 200 at submit, and the job died with `'str' object has no attribute 'shape'`. Reproducing it requires a live Comfy Cloud server and the partner node, which this SDK repo and worktree cannot exercise; no code path in this diff enforces the claim, so it is documentation of server behaviour grounded in that report. - **The server-side rule was not exercised.** The rewrite of `core/ASSET` → filename and the loader-only file-staging logic live in the cloud backend (`cloud/common/file_extractor.go`, `FileNodeFieldName`), a different repo not accessible from this sandbox. The paragraph paraphrases that behaviour from the sourcing investigation rather than from reading the code here. - **The equivalent docs update in `Comfy-Org/docs` is not included.** The same example appears in `development/api-development/sdks.mdx` (and its ja/ko/zh mirrors) and needs the same paragraph. That repo is out of scope for this change and is a manual follow-on. - **The originating investigation ticket could not be read.** The bug context originates from a related tracker issue that is not reachable from this environment; the change relies on the description and root cause carried into this task rather than on re-reading that issue.  ## Summary by CodeRabbit * **Documentation** * Clarified that asset handles resolve to server-side filenames and should be assigned to filename inputs on loader nodes. * Explained that assigning an asset handle directly to an image, video, audio, or mask input may be accepted at submission but fail during execution. * Updated examples to show how to connect a loader’s output and pass an asset to an input.  --- <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:09 +00:00