refactor(run): extract shared queued tail + state-init + load helpers (BE-3265) - #526
refactor(run): extract shared queued tail + state-init + load helpers (BE-3265)#526mattmillerai wants to merge 5 commits into
Conversation
… (BE-3265)
execute() and execute_cloud() carried near-verbatim twins of the ~45-line
async-submit "queued" tail (jobs_state.new → item_map → write → _spawn_watcher
→ status_glyph('queued') panel → watcher-failure warning → queued envelope →
_tail_state_file), and the cloud --wait path repeated the state-init sub-block
a third time. The load/preloaded prefix was also byte-identical.
Extract three module-level helpers, keeping each function's own submit/wait
logic and the per-target envelope contract intact:
- _load_workflow_or_exit(): the identical preloaded/_load_workflow_file prefix.
- _init_queued_state(): jobs_state.new + item_map + write, reused by the local
async path, the cloud non-wait path, and the cloud --wait path.
- _emit_queued(): the pretty panel + queued envelope + live tail. Divergent
bits stay at the call sites via params (watch_command, extra_envelope_fields)
so the public agent-mode JSON envelope is preserved verbatim per target —
local keeps host/port + watcher_spawned; cloud keeps base_url, no
watcher_spawned; _journal_run placement unchanged (local earlier, cloud in
the block).
Add two golden-envelope tests pinning the exact queued field set for local and
cloud so the shared helper can never silently drop/add a key.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
✅ No high-signal findings.
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
|
Triage pass — no code changes were needed; both red checks are external to this diff. Merge conflict: none, PR is Review feedback: nothing outstanding. The Cursor panel returned "✅ No high-signal findings" (6/8 reviewers contributed), and there are zero open review threads. CodeRabbit did not review (label-gated: "Review skipped: no matching review label", plus a rate limit). CI — both failures are pre-existing and reproduce on unrelated branches:
So this PR should go green on Self-review: re-read the extraction against all three call sites. Order of operations is preserved in each path (state write → One note for the reviewer: the 287-line |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
Resolves uv.lock conflict (exceptiongroup marker) by regenerating, which lands exactly on main's lockfile state.
skishore23
left a comment
There was a problem hiding this comment.
The refactor itself is good and I'd like it in — but one of the new golden tests is broken, and it's the only thing failing CI right now. One-line fix.
Blocker: test_queued_envelope_field_set_local_golden patches a stale target
test_run_json.py:309 patches:
patch("comfy_cli.command.run.request.urlopen") as mock_open,
mock_open.return_value.read.return_value = json.dumps({"prompt_id": "pL"}).encode()Every other test in this file patches comfy_cli.http._AUTHED_OPENER.open — I counted ~15 of them, including the sibling test_no_wait_emits_prompt_preview_then_queued 20 lines above. run/__init__.py:15 even flags from urllib import request as a legacy patch target (# noqa: F401 — patch target for tests), but submission now goes through comfy_cli.http, so the patch never intercepts and the test makes a real connection:
{"ok": false, "error": {"code": "connection_error",
"message": "Failed to submit workflow: [Errno 61] Connection refused"}}
Fix (verified — the whole file goes to 98 passed):
patch("comfy_cli.http._AUTHED_OPENER.open") as mock_open,
mock_open.return_value.__enter__.return_value.read.return_value = json.dumps({"prompt_id": "pL"}).encode()Note both halves are needed: the opener is used as a context manager, so __enter__.return_value.read is the right attribute (swapping only the patch target still fails).
This is the same stale-HTTP-target class as #521 — worth a quick grep across the other open PRs in this stack for request.urlopen / urllib.request.urlopen patch targets.
The CI triage in the description is out of date
build/ "Run pytest" — 9 failures intest_config_parser.py/test_node_init.py, allValueError: Comment cannot contain line breaks… Fixed by #533
That was true when written, but the tomlkit fix has since landed. The current build run (2026-07-30) reports:
FAILED tests/comfy_cli/command/test_run_json.py::TestSuccessfulRun::test_queued_envelope_field_set_local_golden - assert 1 == 0
===== 1 failed, 3649 passed, 36 skipped =====
So build is red because of this PR's own new test, not a pre-existing issue. Everything else in the suite is green — which is actually good news for the refactor.
(Separately, GitHub currently reports this PR as CONFLICTING/DIRTY. It merged cleanly for me against current main with no conflict markers, so that metadata looks stale — a rebase should clear it.)
The refactor itself checks out
With the test fixed, all 98 pass, and I agree with the substantive judgment calls:
- Keeping the
queuedenvelope field set owned by the call site rather than the helper is the right shape for a public agent-mode contract — and pinning order, not just membership, is the detail that makes the golden tests actually load-bearing. - Scoping
_prepare_workflowdown was correct. The five divergences you list (object_info source, UI-conversion error codes, classify semantics,print_promptbehavior, local-only partner preflight) are real, and awhere-branching unifier would trade duplication for a worse conditional. Right call not to force it. - Leaving the local
--waitcompleted path alone is a genuinely sharp catch — routing it through_init_queued_statewould have turned onecompletedwrite into aqueuedwrite plus a second write. That's the kind of thing a "pure refactor" quietly breaks. uv.lockis no longer in the diff, so that section of the description can go too.
Push the one-line test fix and I'll approve.
…n-queued-tail # Conflicts: # comfy_cli/command/run/__init__.py
test_queued_envelope_field_set_local_golden patched comfy_cli.command.run.request.urlopen, which submission no longer goes through since BE-3274 routed it through the shared no_redirect_urlopen (comfy_cli.http._AUTHED_OPENER.open). Match the sibling test's pattern. Per skishore23's review.
|
Pushed the one-line fix ( |
…n-queued-tail # Conflicts: # comfy_cli/command/run/__init__.py
ELI-5
comfy runhas two doors — one for your local ComfyUI server, one for ComfyCloud. Both doors did the exact same thing at the end after handing off a job:
write a little status file, start a background watcher, print the pretty "⏳
queued" panel, emit the machine-readable JSON line, and briefly tail the job.
That ~45-line "queued tail" was copy-pasted verbatim in both, and the cloud path
even repeated part of it a third time. Copy-paste drift had already started (the
two JSON envelopes had quietly diverged). This PR moves that shared tail into one
helper so there's a single place to change it, without altering what any user or
agent actually sees.
What changed
Pure refactor of
comfy_cli/command/run/__init__.py— no behavior change. Threesmall module-level helpers replace the duplication:
_load_workflow_or_exit()— the byte-identicalpreloaded/_load_workflow_fileprefix that opens both
execute()andexecute_cloud()._init_queued_state()—jobs_state.new→ stashitem_map→write,reused by the local async path, the cloud non-wait path, and (the third copy)
the cloud
--waitpath._emit_queued()— the queued tail: pretty status panel + watcher-failurewarning +
queuedenvelope + live state tail.Each function keeps its own submit/wait logic; only the shared mechanics moved.
Preserving the public JSON envelope (the careful part)
The
queuedenvelope is a public agent-mode contract, so field membership iskept verbatim per target — the call site owns the field set,
_emit_queuedjust renders it:
host,port,state_file,watcher_spawned(in that order)base_url,state_file(nowatcher_spawned, by design)Other deliberately-preserved divergences:
_journal_runstill fires earlier inthe local path and inside the block in the cloud path; the watch hint keeps its
--where cloudsuffix on cloud;_spawn_watcherstill getshost/portonly onlocal. Field order was preserved exactly, not just the set.
Tests
test_run_json.py) that pin the exactqueuedfield set for local and cloud, so the shared helper can neversilently drop or add a key.
runsuites(
test_run.py+test_run_json.py) are 181 passed.ruff check+ruff format --checkclean.Judgment call —
_prepare_workflowscoped down (please note)The ticket also proposed a
_prepare_workflow()over the fullload/convert/classify/dry-run/preflight prefix. I extracted only the genuinely
identical part of that prefix (
_load_workflow_or_exit) and consciously didnot force a single shared helper over the rest, because the two prefixes are
parallel-but-divergent, not verbatim:
fetch_object_infovs cloud snapshot_load_from_target(mode="cloud"));cql_no_graph; hint text differs);emptyvsinvalid; cloud collapsesboth to
workflow_not_api_format, and popscompose_metaunconditionally);print_promptdiffers (localreturn+ emits preview only in the earlierprompt_previewevent; cloud emits its ownprompt_previewevent thenraise typer.Exit(0));A unifying helper would be a
where-branching tree that's larger and riskierthan the duplication it removes, against the "small, conservative" guardrail. The
headline of the ticket — the verbatim ~45-line queued tail — is fully extracted;
the prefix is addressed for its truly-shared slice. Happy to take the fuller
prefix extraction as a follow-up if a reviewer prefers it.
Self-review
extraction (negative-claim falsification trigger does not fire).
--waitcompleted path was intentionally left untouched: itwrites state once as
completed, so routing it through_init_queued_state(which writes once as
queued) would add a second write — a behavior change.CI status
The tomlkit CI break (previously flagged here) is resolved on
main. The onered check this PR had —
test_queued_envelope_field_set_local_goldenpatchinga stale HTTP target (
comfy_cli.command.run.request.urlopeninstead ofcomfy_cli.http._AUTHED_OPENER.open, per skishore23's review) — is fixed.test_run.py+test_run_json.py= 295 passed; full suite green;ruff check+ruff format --checkclean on the changed files.test/ "Windows Specific Commands" (ImportError: cannot import name '__version__' from 'pydantic_core') is repo-wide and unrelated to this diff;tracked as BE-3289.