[PR #50] [MERGED] fix: expose job_id and expires_at on the public wrappers #58

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/50
Author: @wei-hai
Created: 8/13/2026
Status: ✅ Merged
Merged: 8/13/2026
Merged by: @wei-hai

Base: main ← Head: wei/expose-job-id

📝 Commits (1)

75ef44d fix: expose job_id and expires_at on the public wrappers

📊 Changes

5 files changed (+67 additions, -1 deletions)

View changed files

📝 src/comfy_sdk/assets.py (+17 -0)
📝 src/comfy_sdk/outputs.py (+12 -0)
📝 tests/conftest.py (+6 -1)
📝 tests/test_assets.py (+23 -0)
📝 tests/test_jobs.py (+9 -0)

📄 Description

The recent spec sync added three fields to the generated models — Output.job_id, Asset.job_id, Asset.expires_at — but the hand-written wrapper classes never surfaced any of them. A caller couldn't reach the field the change exists to provide without touching a private attribute.

How it was found

End-to-end testing against Comfy Cloud staging and a self-hosted comfy-api-proxy, driving a real workflow. The value was correct on the wire from both:

wire job_id: 94d5c5a3-db99-46e7-8dc2-90d911f8d856
SDK wrapper: <NOT EXPOSED>

Existing tests missed it because they assert on request and response bodies, never on the wrapper's surface — so the field was verified at the HTTP layer while being unreachable through the public API.

The fix

job_id on Output/AsyncOutput and Asset/AsyncAsset, plus expires_at on the asset. Asset doesn't retain the full model, so those two are captured in _apply alongside the existing fields rather than passed through.

Typed str | None — genuinely absent for an uploaded asset with no producing job, so it isn't faked.

Tests assert the wrapper, not the wire

That's the gap that let this ship. New tests: output.job_id equals the producing job's id after a real run; an uploaded asset's job_id is None; a committed asset's expires_at parses to a timezone-aware datetime. The shared fixture now stamps job_id onto polled outputs, which it never carried before — previously there was no way to test the real value flowing through, only its absence.

Verified end to end, not just in unit tests

Re-ran both environments against this branch:

Cloud staging — job succeeded, output.job_id and asset.job_id both match, PNG downloaded and verified, GET /jobs/{id}/workflow returns format: api with the submitted graph and no extra_data
Local ComfyUI 0.30.0 via comfy-api-proxy — same five steps pass

ruff, ruff format, mypy, drift and hygiene clean; 140 tests pass.

Noted, not fixed

Asset also omits size_bytes, content_type, created_at and url from its wrapper — but those predate this sync and aren't part of the same regression, so widening the surface further belongs in its own change.

🤖 Generated with Claude Code

_{🔄 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-python-sdk/pull/50 **Author:** [@wei-hai](https://github.com/wei-hai) **Created:** 8/13/2026 **Status:** ✅ Merged **Merged:** 8/13/2026 **Merged by:** [@wei-hai](https://github.com/wei-hai) **Base:** `main` ← **Head:** `wei/expose-job-id` --- ### 📝 Commits (1) - [`75ef44d`](https://github.com/Comfy-Org/comfy-python-sdk/commit/75ef44dacc4656a73d53c9a60cd71b0ae261173d) fix: expose job_id and expires_at on the public wrappers ### 📊 Changes **5 files changed** (+67 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/comfy_sdk/assets.py` (+17 -0) 📝 `src/comfy_sdk/outputs.py` (+12 -0) 📝 `tests/conftest.py` (+6 -1) 📝 `tests/test_assets.py` (+23 -0) 📝 `tests/test_jobs.py` (+9 -0) </details> ### 📄 Description The recent spec sync added three fields to the generated models — `Output.job_id`, `Asset.job_id`, `Asset.expires_at` — but the hand-written wrapper classes never surfaced any of them. A caller couldn't reach the field the change exists to provide without touching a private attribute. ## How it was found End-to-end testing against **Comfy Cloud staging** and a **self-hosted comfy-api-proxy**, driving a real workflow. The value was correct on the wire from both: ``` wire job_id: 94d5c5a3-db99-46e7-8dc2-90d911f8d856 SDK wrapper: <NOT EXPOSED> ``` Existing tests missed it because they assert on request and response **bodies**, never on the wrapper's surface — so the field was verified at the HTTP layer while being unreachable through the public API. ## The fix `job_id` on `Output`/`AsyncOutput` and `Asset`/`AsyncAsset`, plus `expires_at` on the asset. `Asset` doesn't retain the full model, so those two are captured in `_apply` alongside the existing fields rather than passed through. Typed `str | None` — genuinely absent for an uploaded asset with no producing job, so it isn't faked. ## Tests assert the wrapper, not the wire That's the gap that let this ship. New tests: `output.job_id` equals the producing job's id after a real run; an uploaded asset's `job_id` is `None`; a committed asset's `expires_at` parses to a timezone-aware datetime. The shared fixture now stamps `job_id` onto polled outputs, which it never carried before — previously there was no way to test the real value flowing through, only its absence. ## Verified end to end, not just in unit tests Re-ran both environments against this branch: - **Cloud staging** — job succeeded, `output.job_id` and `asset.job_id` both match, PNG downloaded and verified, `GET /jobs/{id}/workflow` returns `format: api` with the submitted graph and no `extra_data` - **Local ComfyUI 0.30.0 via comfy-api-proxy** — same five steps pass `ruff`, `ruff format`, `mypy`, drift and hygiene clean; **140 tests pass**. ## Noted, not fixed `Asset` also omits `size_bytes`, `content_type`, `created_at` and `url` from its wrapper — but those predate this sync and aren't part of the same regression, so widening the surface further belongs in its own change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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