fix(workflow): emit an envelope for set-slot --stdout and vary under --json (BE-4215) - #579
fix(workflow): emit an envelope for set-slot --stdout and vary under --json (BE-4215)#579mattmillerai wants to merge 2 commits into
Conversation
…--json (BE-4215) `comfy workflow set-slot --stdout` wrote the modified workflow bare to stdout and returned WITHOUT calling `renderer.emit(...)`, so under `--json` it produced no `envelope/1` at all. Every machine caller — including the local Comfy MCP's `set_workflow_slot`, whose default is `stdout=True` — failed the whole parameterize step with "comfy-cli returned no JSON". stdout belongs to the envelope in JSON mode (docs/json-output.md), so: - set-slot --stdout: human mode still prints the raw workflow (pipeable); JSON mode emits an envelope whose `data.workflow_json` carries the modified workflow, with `wrote: null` / `out: "stdout"` and `changed: false`. - vary without --out-dir: same split — raw NDJSON in human mode, variants in `data.variants` under --json (previously the variants were dumped onto stdout ahead of the envelope and never appeared in `data` at all). - schemas/workflow.json documents the new keys and widens `wrote` to allow null.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes 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 (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 1 |
| 🟢 Low | 2 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…s (BE-4215)
Cursor review follow-ups on the BE-4215 envelope split:
- vary (High): in human mode without --out-dir the NDJSON loop fell through to
the pretty summary, so `comfy workflow vary ... --no-json > out.ndjson` ended
with a non-JSON `✓ produced N variation(s)` line and broke strict
line-delimited consumers. The summary/warnings now go to stderr whenever
stdout carries the stream.
- set-slot/vary help + comment (Medium): a redirect makes stdout a non-TTY,
which Renderer.resolve reads as JSON mode, so `--stdout > new.json` yields an
envelope, not a raw workflow. Both now say --no-json is required for the raw
form.
- set-slot (Nit): the workflow was serialized unconditionally but unused on the
--stdout + JSON path; it is now serialized only where it is consumed.
- Both commands fold the object_info stale-cache note into `warnings` up front
so the --stdout early return reports it (on stderr) instead of dropping it.
- The human-mode vary test asserted only on lines starting with `{`, which hid
the trailing summary; it now requires every stdout line to parse as JSON and
checks the summary lands on stderr.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ELI-5
comfy workflow set-slot --stdoutis how a tool asks "apply this override and hand theworkflow back without touching my file." Under
--jsonit handed back a bare workflowobject and never printed the
envelope/1wrapper that every machine caller looks for —so the caller saw "no JSON" and the whole parameterize step failed. This makes that path
speak the envelope contract: the modified workflow now rides inside the envelope's
data.Piping by hand is unchanged.
What changed
comfy_cli/command/workflow.pyset-slot --stdout— theif stdout:branch wrotesys.stdout.write(serialized)andreturned beforerenderer.emit(...), so no envelope was ever emitted in JSON mode.Now:
--no-json/ TTY): unchanged — the raw workflow on stdout, pipeable.envelope/1on stdout whosedatacarriesworkflow_json(the modified workflow), plusout: "stdout",wrote: null,the existing
applied/warnings/ stale block, andchanged: false(nothing was written, so the envelope no longer claims a change).
varywithout--out-dir— the sibling raw-stdout path the ticket asked me to audithad the same gap in a quieter form: it dumped N raw workflow lines onto stdout ahead of
the envelope, and the envelope's
datanever contained the variants at all (written: [],no workflows). Same split applied: raw NDJSON in human mode,
data.variantsunder--json.--stdout/--out-dir/varyhelp strings now state where the result lands in each mode.comfy_cli/schemas/workflow.json— documentsout,workflow_json,variants,written,out_dir, and widenswroteto["string", "null"](it can legitimately be null now).Why the mode split is the right seam
docs/json-output.mdand the renderer's own contract are explicit that in JSON mode stdoutcarries only the envelope;
workflow get --where localin this same file already usesexactly this
if renderer.is_pretty(): write rawshape. The two stdout branches wereintroduced in #470 — the commit that added the envelope contract — with no comment or test
asserting the bypass, so this reads as an oversight in that commit rather than a deliberate
carve-out.
Downstream verification (acceptance criterion 4)
Not asserted from my own tests — I ran the real comfy-cli binary and fed its stdout
through comfy-local-mcp's own
_last_json_object+_unwrap_envelope, extracted verbatimfrom that repo's
origin/main(the exact code that raises the reported error):set_workflow_slot(path, [...])(defaultstdout=True) doesorigin/main(before)ComfyCliError: comfy-cli returned no JSON (exit 0)— the reported bug, reproduceddatawith keysapplied, out, warnings, workflow, workflow_json, wrote;workflow_jsoncontains the applied override (3.seed→4215),wrote=None,out='stdout'Also verified against the real binary:
--no-json ... --stdoutstill prints the bare workflow;--json workflow varyemits exactly one line (the envelope) with both variants indata.variants;--no-json workflow varystill emits 2 NDJSON lines.(The comfy-cli pin bump in comfy-local-mcp is explicitly out of scope per the ticket, so the
MCP won't pick this up until that pin moves. Its
set_workflow_slotdocstring — "writemodifiedto disk" — will want a one-line update to saymodified["workflow_json"]when thepin lands.)
Judgment calls
data.workflow_json(not makingdatabe the workflow). Keeping the payloadstructured preserves
applied,warnings, and thestale/object_info_stalesignal thatan agent needs;
workflow_jsonmatches how this file already names a workflow document on thecloud path. The cost is that a consumer reads
data["workflow_json"]rather thandata.comfy workflow set-slot f.json a=b --stdout > out.jsonwith no flag is non-TTY, and therenderer auto-selects JSON there — so that shell redirect now yields an envelope instead of a
bare workflow. That is the documented machine contract (a non-TTY consumer is a machine), and
--no-jsonis the escape hatch for a raw pipe; both are now spelled out in--stdout's help.Flagging it explicitly because it is the one behavior change beyond the bug itself.
varywas in the ticket's "audit" list, and I fixed it rather than only noting it — theenvelope it emitted was structurally valid but semantically empty (no variants anywhere in
data), which is the same contract gap. Same 3-line shape, covered by its own two tests.changed: falseon the--stdoutpath.--stdoutwrites nothing. The oldchanged=Truewas only ever reachable on the in-place path (the stdout branch returnedearly), so no existing behavior flips.
Not a capability-denying change — it restores a broken default; no deny/dead-end path,
"not supported" string, or dead-end test was added.
Testing
tests/comfy_cli/command/test_workflow_slots.py:--json --stdoutemits asingle
envelope/1whosedataround-trips the applied override (and leaves the fileuntouched); human-mode
--stdoutstill prints the raw workflow;varyvariants land indata.variantsunder--json;varystill emits NDJSON in human mode;variants is Nonewhen
--out-dirwas used. Red→green confirmed: 3 of them fail on the pre-fix source.ruff check+ruff format --diff(0.15.15, the CI pin) clean.