Skip to content

fix(download): resolve SaveGLB 3d/mesh outputs in extract_output_entries (BE-4417) - #600

Open
mattmillerai wants to merge 2 commits into
mainfrom
matt/be-4417-glb-3d-outputs
Open

fix(download): resolve SaveGLB 3d/mesh outputs in extract_output_entries (BE-4417)#600
mattmillerai wants to merge 2 commits into
mainfrom
matt/be-4417-glb-3d-outputs

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

When you run a workflow that makes a 3D model (a SaveGLB node, e.g. Tripo 3.1 or Rodin Gen 2.5), ComfyUI files the finished .glb under a bucket labelled "3d". But comfy download <prompt_id> --where cloud only knew to look in the buckets labelled images, gifs, videos, audio, and files — so it never found the mesh and gave up with download_no_outputs, even though the file was sitting right there in Media Assets. This teaches the downloader to recognise a file by its shape (it's a list of records that have a filename) instead of by a fixed list of bucket names, so 3D/mesh outputs — and any future output key — resolve and download.

What changed

extract_output_entries in comfy_cli/comfy_client.py previously scanned a hardcoded allowlist of node-output keys: ("images", "gifs", "videos", "audio", "files"). SaveGLB emits its entries under "3d" (ui={"3d": results} in ComfyUI core's nodes_save_3d.py), so those entries were dropped, extract_output_urls returned [], and transfer.py reported download_no_outputs.

The allowlist loop is replaced with shape-based detection, mirroring ComfyUI core's normalize_outputs and the cloud worker's isFileOutputArray: iterate every node-output key, and for any key whose value is a list, collect the items that are dicts carrying a "filename". The "animated" key is skipped explicitly (core emits it as (True,) boolean flags, not file entries — the dict guard would already drop it, but the explicit skip matches core semantics and guards against future dict-shaped animated metadata). Keys holding non-file lists ("text" strings, "dims" dicts without a filename, etc.) are naturally filtered by the isinstance(item, dict) and "filename" in item guard.

This is a strict superset of the prior behaviour for the five existing keys — the (filename, subfolder, type) join-triple contract is unchanged. The only observable difference is intra-node ordering: entries within a single node now follow node-output key insertion order rather than the old fixed key order (the docstring's ordering contract is updated accordingly). No test depends on cross-key ordering within a single node.

Because both the job-watcher state-file path (job_watcher.py) and the API fallback (transfer.py) share this one extractor, this fixes both.

Tests

  • test_extract_resolves_saveglb_3d_key — a "3d"-keyed record resolves to the entry and builds /view?filename=abc123.glb&subfolder=3d&type=output.
  • test_extract_resolves_singular_video_key — the cloud worker's singular "video" key (from synthesizeMediaTypeFromResult) resolves.
  • test_extract_skips_non_file_shaped_keys"animated": [True], "text": ["hello"], "dims": [{"width": 512}] produce no entries.
  • TestSaveGlb3dDownload.test_3d_only_record_downloads_glb — regression through comfy download: a 3d-only history record downloads a file with a .glb extension instead of erroring.
  • Existing mixed-media tests (images/gifs/videos/audio/files) unchanged and green.

Full suite: 2773 passed, 37 skipped. ruff check + ruff format --check clean.

Judgment call (out of scope, noted for transparency)

Two other copies of the same hardcoded allowlist live in comfy_cli/command/jobs.py — the comfy jobs list output-count display (~L325) and the comfy jobs watch WebSocket executed handler (~L1195). Both are on the local-server command surface, not the cloud comfy download path this ticket targets, so they are deliberately left untouched to keep the diff scoped. They would exhibit the same 3D-blindness for local jobs; a follow-up can converge them on the same shape-based detection if desired.

Closes #508 (spike BE-4278).

…ies (BE-4417)

comfy download <prompt_id> --where cloud returned download_no_outputs for
succeeded partner-3D jobs (Tripo 3.1, Rodin Gen 2.5): the SaveGLB node emits
its entries under the "3d" key (ui={"3d": results}), but extract_output_entries
only scanned the fixed allowlist (images, gifs, videos, audio, files), so the
.glb was never resolved and transfer.py emitted download_no_outputs.

Replace the hardcoded allowlist with shape-based detection 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, with "animated" explicitly skipped (it emits (True,) flags).
This is a strict superset of the prior five keys and fixes both the
job-watcher state-file path and the API fallback, which share the extractor.
@mattmillerai mattmillerai added agent-coded PR authored by the agent-work loop cursor-review Request Cursor bot review labels Jul 24, 2026
@mattmillerai
mattmillerai marked this pull request as ready for review July 24, 2026 21:18
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 33 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 127e0bb2-b683-402f-858d-372c6ef73168

📥 Commits

Reviewing files that changed from the base of the PR and between 85b62da and ae140b1.

📒 Files selected for processing (3)
  • comfy_cli/comfy_client.py
  • tests/comfy_cli/cloud/test_client.py
  • tests/comfy_cli/command/test_transfer_download.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matt/be-4417-glb-3d-outputs
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-4417-glb-3d-outputs

Comment @coderabbitai help to get the list of available commands.

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Jul 24, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 2 finding(s).

Severity Count
🟡 Medium 1
🟢 Low 1

Panel: 6/8 reviewers contributed findings.

Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)

Comment thread comfy_cli/comfy_client.py
Comment thread comfy_cli/comfy_client.py
…lder,type) tuple (BE-4417)

A node that surfaces the same artifact under two keys — e.g. the cloud
worker's singular "video" alongside the classic plural "videos" — would
emit two identical /view URLs and download the same file twice. De-dup on
the full entry tuple, first occurrence wins, preserving order. Addresses
the Cursor review de-dup finding on #600.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded PR authored by the agent-work loop bug Something isn't working cursor-review Request Cursor bot review size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

comfy cloud 3d partner nodes issue

1 participant