fix: escape square brackets in Typer help strings so choice lists render#547
fix: escape square brackets in Typer help strings so choice lists render#547mattmillerai wants to merge 1 commit into
Conversation
Rich parses square brackets in Typer help= strings as console markup, so help text of the form [a|b|c] was silently stripped and the accepted values never appeared in --help output. Escape the opening bracket at the 16 affected argument/option help strings. Verified by rendering the real help for each affected command.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 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 (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)
Review pass: no code changes needed — both CI failures are pre-existing and unrelatedRe-reviewed this PR. No merge conflict ( The two red checks are not caused by this PR. This branch touches only Typer
|
|
Correction to my comment above: both failures already have open fix PRs — no new follow-ups needed, and they independently corroborate the diagnosis:
This PR needs no changes and stays ready to merge; its own CI should go green once those land. I did not merge anything. |
ELI-5
The CLI's
--helpwas supposed to tell you which values an option accepts —[remote|local|cache],[all|comfy|cli]. It didn't. Rich (the library that draws those pretty help panels) reads square brackets as styling markup, the same way[bold]means "make this bold". So it looked at[remote|local|cache], decidedremote|local|cachemust be a style name, and quietly deleted the whole thing. You got an empty cell.This escapes the opening bracket at the 16 places it happened, so Rich prints the brackets instead of eating them.
What changed
One character per site —
help="[a|b|c]"→help="\\[a|b|c]"— across 16 argument/option help strings:comfy_cli/command/custom_nodes/command.py(15): the--mode[remote|local|cache]help on 11nodesubcommands, plus thenodesargument help onupdate/disable/enable/fix.comfy_cli/cmdline.py(1): thetargetargument help oncomfy update.No runtime behavior changes — these are help strings only. Only the opening
[needs escaping; the closing]is already literal.Verification
Since no test asserts on these strings, the real check is rendering the actual help. All 16 sites confirmed rendering, not just the 3 named in the issue:
All 11
[remote|local|cache]sites verified by rendering each owning command (show,simple-show,install,reinstall,uninstall,update,disable,enable,fix,install-deps,deps-in-workflow) — 11/11 render.pytest tests/comfy_cli -q→ 2471 passed, 13 skipped (matches baseline exactly)ruff format→ clean;ruff check→ 14 errors, identical to the pre-change baseline onmain(all pre-existing, none in touched lines)Judgment calls / notes
1. Two additional bracket sites exist that the issue's grep (
help="\[) can't see — both deliberately left alone. That pattern anchors to a leading bracket; a broaderhelp=("|')[^"']*\[sweep found two more:cmdline.py:1133—help="Launch ComfyUI: ?[--background] ?[-- <extra args ...>]". Not a bug — verified by rendering.comfy launch --helpprints its brackets intact, because Rich only treats[...]as a tag when the contents can parse as a style name;--backgroundstarts with-, so Rich leaves it literal. This is also why the 16 sites in this PR do break:[remote|...],[all|...]start with a letter. I nearly "fixed" this one before checking.cmdline.py:611—@app.command(help="Update ComfyUI Environment [all|comfy|cli]"). This one is genuinely stripped onmaintoday (renders asUpdate ComfyUI Environment), but it is the exact site docs: clarify update/lifecycle entry points via help text (BE-3001) #520 already fixes, so touching it here would collide. Left to docs: clarify update/lifecycle entry points via help text (BE-3001) #520.2. Verified this does not conflict with #520, which is open and touches both of these files with hunks landing within a few lines of two of mine. Test-merged #520 into this branch locally: merges cleanly, no conflicts, and both fixes render correctly side by side (
updatepanel help and--modeboth show their choice lists). No coordination needed on merge order.3. Escape spelling. Written
"\\[..."(normal string → the string holds a literal\[), not"\[". The latter renders identically but is an invalid escape sequence — it trips ruffW605(+16 errors over baseline) and is a futureSyntaxError. Worth noting since"\["is the tempting shorter form.4. No Linear ref in this title/body, per the public-repo convention for this repo; the ticket autolinks via the branch name. Flagging because sibling PRs (#490, #520) do carry
(BE-…)suffixes, so this is an inconsistency someone may want to settle either way.5. Not done (explicitly out of scope per the issue): converting these to a real
enum/click.Choiceso Typer renders the values itself. Several duplicate an existingautocompletion=choice list, so that dedup is real — but it's a behavior change and should be decided separately.