diff --git a/comfy_cli/comfy_client.py b/comfy_cli/comfy_client.py index 02b8c8eb..69cef1e1 100644 --- a/comfy_cli/comfy_client.py +++ b/comfy_cli/comfy_client.py @@ -533,29 +533,49 @@ def extract_output_entries(record: dict) -> list[dict]: (e.g. ``comfy download`` reading a state file) can join them back to producing nodes on the (filename, subfolder, type) triple — the same one :meth:`Client.view_url` encodes as query params. Ordering is stable: - record ``outputs`` insertion order, then media-key order, then item order. + record ``outputs`` insertion order, then node-output key insertion order, + then item order. + + Detection is shape-based, mirroring ComfyUI core's ``normalize_outputs`` + and the cloud worker's ``isFileOutputArray``: any node-output key whose + value is a list of dicts carrying a ``filename`` is treated as file + outputs. This deliberately covers keys beyond the classic + ``images/gifs/videos/audio/files`` — notably SaveGLB's ``"3d"`` key and + the cloud worker's singular ``"video"`` key — so 3D/mesh jobs resolve + instead of returning ``download_no_outputs`` (BE-4417). The ``"animated"`` + key is skipped explicitly to match core semantics (it emits ``(True,)`` + boolean flags, not file entries). + + Entries are de-duplicated on the full ``(node_id, filename, subfolder, + type)`` tuple, first occurrence wins. A node that surfaces the same + artifact under two keys — e.g. the cloud worker's singular ``"video"`` + alongside the classic plural ``"videos"`` — would otherwise yield two + identical ``/view`` URLs and download the same file twice. """ results: list[dict] = [] + seen: set[tuple[str, str, str, str]] = set() outputs = record.get("outputs") or {} if not isinstance(outputs, dict): return results for node_id, node_output in outputs.items(): if not isinstance(node_output, dict): continue - for key in ("images", "gifs", "videos", "audio", "files"): - items = node_output.get(key) or [] - if not isinstance(items, list): + for key, items in node_output.items(): + if key == "animated" or not isinstance(items, list): continue for item in items: if isinstance(item, dict) and "filename" in item: - results.append( - { - "node_id": str(node_id), - "filename": str(item.get("filename", "")), - "subfolder": str(item.get("subfolder", "")), - "type": str(item.get("type", "output")), - } - ) + entry = { + "node_id": str(node_id), + "filename": str(item.get("filename", "")), + "subfolder": str(item.get("subfolder", "")), + "type": str(item.get("type", "output")), + } + dedup_key = (entry["node_id"], entry["filename"], entry["subfolder"], entry["type"]) + if dedup_key in seen: + continue + seen.add(dedup_key) + results.append(entry) return results diff --git a/tests/comfy_cli/cloud/test_client.py b/tests/comfy_cli/cloud/test_client.py index 1f6c59d3..e0da5b40 100644 --- a/tests/comfy_cli/cloud/test_client.py +++ b/tests/comfy_cli/cloud/test_client.py @@ -641,6 +641,56 @@ def test_extract_output_urls_delegates_to_extract_outputs(self): client = comfy_client.Client(CLOUD) assert client.extract_output_urls(record) == [o["url"] for o in client.extract_outputs(record)] + def test_extract_resolves_saveglb_3d_key(self): + """SaveGLB emits entries under the "3d" key, not the classic media + keys — shape-based detection must still resolve them (BE-4417).""" + record = {"outputs": {"10": {"3d": [{"filename": "abc123.glb", "subfolder": "3d", "type": "output"}]}}} + assert comfy_client.extract_output_entries(record) == [ + {"node_id": "10", "filename": "abc123.glb", "subfolder": "3d", "type": "output"} + ] + urls = comfy_client.Client(CLOUD).extract_output_urls(record) + assert urls == ["https://cloud.example.com/api/view?filename=abc123.glb&subfolder=3d&type=output"] + + def test_extract_resolves_singular_video_key(self): + """The cloud worker's synthesizeMediaTypeFromResult emits a singular + "video" key; shape-based detection covers it too.""" + record = {"outputs": {"7": {"video": [{"filename": "clip.webm", "subfolder": "", "type": "output"}]}}} + assert comfy_client.extract_output_entries(record) == [ + {"node_id": "7", "filename": "clip.webm", "subfolder": "", "type": "output"} + ] + + def test_extract_dedups_same_artifact_under_two_keys(self): + """A node that surfaces the same artifact under both the singular + "video" and plural "videos" keys must emit one entry, not two — else + the /view URL (and the downloaded file) is duplicated.""" + record = { + "outputs": { + "7": { + "video": [{"filename": "clip.webm", "subfolder": "", "type": "output"}], + "videos": [{"filename": "clip.webm", "subfolder": "", "type": "output"}], + } + } + } + assert comfy_client.extract_output_entries(record) == [ + {"node_id": "7", "filename": "clip.webm", "subfolder": "", "type": "output"} + ] + + def test_extract_skips_non_file_shaped_keys(self): + """Keys carrying flags/metadata rather than file entries produce + nothing: "animated" (boolean flags — explicitly skipped), "text" + (strings), "dims" (dicts without a filename).""" + record = { + "outputs": { + "3": { + "animated": [True], + "text": ["hello"], + "dims": [{"width": 512}], + } + } + } + assert comfy_client.extract_output_entries(record) == [] + assert comfy_client.Client(CLOUD).extract_output_urls(record) == [] + class TestGroupOutputs: """_group_outputs: pure grouping of extract_outputs entries by node and diff --git a/tests/comfy_cli/command/test_transfer_download.py b/tests/comfy_cli/command/test_transfer_download.py index 251fc6cb..05dee7e7 100644 --- a/tests/comfy_cli/command/test_transfer_download.py +++ b/tests/comfy_cli/command/test_transfer_download.py @@ -232,6 +232,42 @@ def test_client_extract_outputs_delegates(self, fake_target): assert outputs[0]["url"] == "https://cloud.example.com/api/view?filename=ComfyUI_a.png&subfolder=&type=output" +GLB_RECORD = { + "status": {"completed": True, "status_str": "success"}, + "outputs": {"10": {"3d": [{"filename": "abc123.glb", "subfolder": "3d", "type": "output"}]}}, +} + + +class TestSaveGlb3dDownload: + """Regression (BE-4417): a SaveGLB job emits its output under the "3d" + key. Shape-based extraction must resolve it so `comfy download` saves the + `.glb` instead of erroring `download_no_outputs`.""" + + def test_3d_only_record_downloads_glb(self, fake_target, tmp_path, capsys): + urls = comfy_client.Client(fake_target).extract_output_urls(GLB_RECORD) + assert urls == ["https://cloud.example.com/api/view?filename=abc123.glb&subfolder=3d&type=output"] + state = jobs_state.JobState( + prompt_id=PROMPT_ID, + client_id=None, + workflow="/abs/mesh.json", + where="cloud", + base_url=fake_target.base_url, + status="completed", + outputs=urls, + record=GLB_RECORD, + item_map=None, + ) + assert jobs_state.write(state) is not None + + paths, data = _run_download(fake_target, tmp_path, capsys) + + assert len(paths) == 1 + # Extension derives from the URL's filename query param → `.glb`. + assert Path(paths[0]).suffix == ".glb" + assert Path(paths[0]).is_file() + assert data["files"][0]["node_id"] == "10" + + class TestCollisionSafeNaming: """A retry fan-out reusing the same item ids re-downloads into the same out-dir; attempt 1 must never be silently clobbered (fennec friction #3,