fix: expose job_id and expires_at on the public wrappers - #50
Conversation
The recent spec sync added Asset.job_id, Asset.expires_at and Output.job_id to the generated models, but the hand-written wrapper classes never surfaced them — so a caller could not reach the field the change exists to provide without touching a private attribute. Found by end-to-end testing against Comfy Cloud staging and a self-hosted proxy: the value was correct on the wire from both, and unreachable through the public API. Existing tests missed it because they assert on the request and response bodies, never on the wrapper's surface; the new tests assert on the wrapper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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:
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_idonOutput/AsyncOutputandAsset/AsyncAsset, plusexpires_aton the asset.Assetdoesn't retain the full model, so those two are captured in_applyalongside 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_idequals the producing job's id after a real run; an uploaded asset'sjob_idisNone; a committed asset'sexpires_atparses to a timezone-aware datetime. The shared fixture now stampsjob_idonto 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:
output.job_idandasset.job_idboth match, PNG downloaded and verified,GET /jobs/{id}/workflowreturnsformat: apiwith the submitted graph and noextra_dataruff,ruff format,mypy, drift and hygiene clean; 140 tests pass.Noted, not fixed
Assetalso omitssize_bytes,content_type,created_atandurlfrom 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