fix(generate): emit envelope/1 errors under global --json and reject a flag-shaped model target - #601
fix(generate): emit envelope/1 errors under global --json and reject a flag-shaped model target#601mattmillerai wants to merge 2 commits into
Conversation
…a flag-shaped model target `comfy --json generate --prompt=x` exited 1 with no JSON on stdout: the flag token was swallowed by the model-alias positional, `spec.get_endpoint` raised, and the handler printed via raw `rich.print`. Any envelope-consuming caller got a blank failure. Every error path in the `generate` module now routes through a `_fail` helper: when the global renderer is in JSON / JSON-stream mode it emits exactly one `envelope/1` error with a stable, registered `code`; otherwise it keeps the historical rich-red output byte-for-byte. Success/result output is unchanged -- the module's non-migration rationale still stands for results. A flag-shaped first positional now fails immediately with `generate_target_required` and points at both `comfy generate list` and `comfy run-template` (the local text-to-image alternative), instead of dying deep inside the spec lookup. Also closes three adjacent silent failures: unhandled SchemaError in `generate list` / `generate schema`, and unreported poll failures behind the transient spinner in the submit and resume paths.
dosubot
Bot
added
the
size:L
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 26 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 (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…ion (BE-4577) All seven consolidated-panel findings, each with a regression test that fails without its fix: - HIGH: `generate resume … --timeout nope` parsed with a bare `float()`, so the ValueError escaped `main()` (which traps only KeyboardInterrupt / typer.Exit / SystemExit) as a traceback — exit 1 with a blank stdout, the exact failure this PR exists to remove. Guarded like the submit path. - MED: the pollers called `resp.json()` unguarded on a 200, so a proxy/CDN HTML error page raised a bare ValueError that neither caller's `(ApiError, HTTPError)` tuple caught. `_decode_poll_body` now raises ApiError (and rejects a non-object body, which would have blown up on `.get()` a line later). - MED: the spend prompt was gated on the command-local `--json` only. With a TTY stdin and a machine stdout (`comfy generate … | jq`) the notice and the prompt went to the piped stdout — invisible to the human being asked, and spliced ahead of the caller's JSON. They go to stderr when the renderer is in JSON mode; the prompt itself is kept (a TTY user can still answer it, and reads it on the same terminal). Pretty mode is untouched. - LOW: `httpx.HTTPStatusError` subclasses HTTPError, so `upload_remote_url`'s `raise_for_status` on a 404 source URL was reported as `generate_network_error` — "check connectivity and retry", the wrong advice for a 4xx that reached the server. Now `generate_api_error`, with the tracking bucket following suit. - LOW: server-controlled text (an ApiError body, a response preview, a partner's failure reason) was interpolated into Rich markup, so a bracketed token either got swallowed (`[link]`) or raised MarkupError (`[/path]`), turning a clean error into a traceback. Escaped at every such site. - LOW: the poll failure was reported inside `with _spinner()`, where a transient Progress is still auto-refreshing stdout — on a TTY the envelope would interleave with spinner control codes. Reported after the block exits. - NIT: `_fail`'s docstring claimed byte-compatibility for legacy_json where `code` is in fact newly added; reworded. Also lists `generate_job_failed` in the SKILL.md error-code set, which the PR had omitted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Needs a rebase before I can review it — and this one deserves a careful one rather than a mechanical Merging current Both come from #621 ( The
For Everything else about the PR reads well from here — the negative-claim falsification, reusing |
|
This PR currently conflicts with Please rebase (or merge |
2 participants
ELI-5
If you ask the CLI for JSON and then make a mistake, it should tell you what went wrong in JSON. It didn't:
comfy --json generate --prompt=xexited 1 and printed nothing a machine could read. Now everycomfy generatefailure comes back as one machine-readable error object with a stable code — and the specific mistake above ("you forgot the model name") now says exactly that, and points you at the local alternative.What changed
1. Structured errors under global
--json/--json-stream. Every error path incomfy_cli/command/generate/app.pynow goes through a small_fail(...)helper. When the global renderer is in a machine mode it callsrenderer.error(code=…, message=…, hint=…, details=…)— exactly oneenvelope/1error line on stdout with a stable code. When it isn't, the historical rich-red line is printed unchanged (and the one path that already printed a dim follow-up hint still does). Success/result output is not migrated — the module's existing non-migration rationale (stdout-redirection contract for job ids / image URLs) still stands for results.New codes, all added to
comfy_cli/error_codes.REGISTRY(which the two-way registry test pins against real call sites):generate_target_required,generate_unknown_model,generate_bad_args,generate_timeout_invalid,generate_api_error,generate_network_error,generate_job_failed,generate_spec_invalid. The spend gate reuses the already-registeredspend_consent_required.2. Actionable flag-shaped target.
_generate_entryrejects atargetthat starts with-(other than-h/--help) before dispatch, withgenerate_target_required: "comfy generaterequires a partner model alias as its first argument … it is a cloud/partner verb that spends credits", hintingcomfy generate listandcomfy run-templatefor local text-to-image.details.receivedcarries the offending token.3. Exit codes stay 1 on every one of these paths.
Tests
New
tests/comfy_cli/command/generate/test_json_errors.py(24 cases) covers the four required scenarios plus the rest of the surface:--jsontarget-required / unknown-model / bad-args / bad-timeout / api-error (withdetails.status+details.body) / network-error / failed-job envelopes; the--json-streamterminal line;-h/--helpstill short-circuiting to help; and pretty-mode regression guards. A shared_sole_envelope()asserts stdout is exactly one line — a second JSON object would break naive callers, so that count is load-bearing. Full suite: 2792 passed, 37 skipped;ruff check+ruff format --diffclean under the CI-pinned 0.15.15.Negative-claim falsification
The diff's user-facing outcome refuses one invocation shape, so per the falsification discipline I went and looked before shipping the refusal, rather than asserting it in a test:
main:python -m comfy_cli --json generate --prompt=x→ exit 1,Unknown model: '--prompt=x'as bare rich text, no envelope. After the change: a singleenvelope/1line withgenerate_target_required.comfy generateworks without a model alias? No. The only non-alias first positionals are the six reserved actions (list/schema/refresh/upload/resume/consent), none of which start with-, andNone/-h/--helpalready short-circuit to help one line above the new guard. So this is a genuine usage error, not a capability being removed.comfy run-template --helpexists (merged in feat: comfy run-template — fetch, fill params, spend-gate, run to completion (BE-4131) #578) andcomfy --json templates lsreturns 578 gallery templates, the first beingimage_z_image_turbo/ "Z-Image-Turbo: Text to Image" — a local text-to-image path.comfy generate list(the other half of the hint) also returns the model table. The error redirects to a working capability rather than denying one.Judgment calls
_generate_entry/_generate/_resume/_list_models/_schema, but the acceptance criterion says any failedcomfy generate._upload,_refresh, and_consentare reachable ascomfy generate …and had the same bare-rich-print failures, so they're migrated too.spend_consent_requiredinstead of addinggenerate_spend_not_confirmed. The suggested new code would be a second contract entry for a condition that already has a registered, documented one (its registry text explicitly covers "comfy generateraises it when a credit-spending call runs non-interactively"). Splitting it would be contract pollution for agents branching onerror.code._emit_result's failed-job branch is migrated (newgenerate_job_failed). The plan says don't migrate result output, and the command-local--jsonraw-response path is untouched. But a terminal non-succeeded job under global--json(without the command-local--json) was landing red text + a raw payload on stdout with no envelope — a failure, not a result, and a real hole in the acceptance criterion.SchemaErrorin_list_models/_schema(a bare traceback for e.g.generate list --download), and unreported poll failures behind the transient spinner in both the submit and the resume path (the submit path swallowed the exception message in every mode; resume let it escape as a traceback). Each is the same "blank failure" class this ticket exists to close.Behavior changes reviewers should weigh
Renderer.resolveprecedence rule 6), socomfy generate bogus | catnow yields an envelope where it previously yielded rich text. That is the renderer's documented contract for every other command;generate's error paths are now consistent with it. Interactive TTY output is unchanged.--jsonflag is deliberately not the trigger. With a real TTY plusgenerate … --json, the pre-existing flat{"error": …, "code": …}object is preserved (pinned by a new test), because that flag's documented meaning is "raw API response", and its success payload is unmigrated. Only the global renderer's mode switches the error shape.comfy_cli/skills/comfy/SKILL.mdgains a bullet documenting the error contract, since the bundled skill is read by exactly the agents that consume these envelopes.Not covered
An unexpected exception (outside the handled families) still escapes as a traceback with no envelope — closing that needs a top-level handler in
cmdline.entry(), which is a separate change with a much wider blast radius.generate listwith zero matches still prints a yellow line and exits 0 with no envelope; it's a success path, so it's outside this ticket.