Skip to content

fix: escape square brackets in Typer help strings so choice lists render#547

Open
mattmillerai wants to merge 1 commit into
mainfrom
matt/be-3321-escape-brackets-typer-help
Open

fix: escape square brackets in Typer help strings so choice lists render#547
mattmillerai wants to merge 1 commit into
mainfrom
matt/be-3321-escape-brackets-typer-help

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

The CLI's --help was 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], decided remote|local|cache must 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 11 node subcommands, plus the nodes argument help on update / disable / enable / fix.
  • comfy_cli/cmdline.py (1): the target argument help on comfy 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:

$ comfy node show --help
│ --mode           TEXT  [remote|local|cache]          │   # was: empty cell

$ comfy node update --help
│ *  nodes  NODES...  [all|List of custom nodes to update] [required]

$ comfy update --help
│    target  [TARGET]  [all|comfy|cli] [default: comfy]

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 -q2471 passed, 13 skipped (matches baseline exactly)
  • ruff format → clean; ruff check14 errors, identical to the pre-change baseline on main (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 broader help=("|')[^"']*\[ sweep found two more:

  • cmdline.py:1133help="Launch ComfyUI: ?[--background] ?[-- <extra args ...>]". Not a bug — verified by rendering. comfy launch --help prints its brackets intact, because Rich only treats [...] as a tag when the contents can parse as a style name; --background starts 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 on main today (renders as Update 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 (update panel help and --mode both 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 ruff W605 (+16 errors over baseline) and is a future SyntaxError. 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.Choice so Typer renders the values itself. Several duplicate an existing autocompletion= choice list, so that dedup is real — but it's a behavior change and should be decided separately.

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.
@mattmillerai mattmillerai added the agent-coded PR authored by the agent-work loop label Jul 17, 2026
@mattmillerai
mattmillerai marked this pull request as ready for review July 17, 2026 12:04
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 41 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 55954b17-2023-43dc-9707-4706688082ba

📥 Commits

Reviewing files that changed from the base of the PR and between a732f5c and 3b97236.

📒 Files selected for processing (2)
  • comfy_cli/cmdline.py
  • comfy_cli/command/custom_nodes/command.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matt/be-3321-escape-brackets-typer-help
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-3321-escape-brackets-typer-help

Comment @coderabbitai help to get the list of available commands.

@mattmillerai mattmillerai added the cursor-review Request Cursor bot review label Jul 17, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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)

@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Review pass: no code changes needed — both CI failures are pre-existing and unrelated

Re-reviewed this PR. No merge conflict (MERGEABLE), no open review threads, Cursor's panel reported ✅ no high-signal findings, and CodeRabbit never actually reviewed (it hit a Fair-Usage rate limit — its green check is Review skipped: no matching review label, not an approval).

The two red checks are not caused by this PR. This branch touches only Typer help= strings — 2 files, 16 single-character escapes, zero runtime behavior — and neither failure involves that code.

build — 9 failures, upstream tomlkit 0.15 regression

ValueError: Comment cannot contain line breaks from config_parser.py, a file this PR does not touch (byte-identical to main). Isolated it to the dependency:

  • tomlkit is unpinned (pyproject.toml:49); uv.lock pins 0.13.3test_config_parser.py 81 passed locally.
  • Force-installing tomlkit==0.15.1 into the same venv reproduces CI's exact 9 failures.
  • Minimal repro: tomlkit.array().comment('\n# x = 1\n')ValueError on 0.15.1, fine on 0.13.3.

tomlkit 0.15 started rejecting comments containing line breaks; config_parser.py:75 and :250 both pass multi-line strings to .comment(). CI installs via pip install -e '.[dev]', which ignores uv.lock and resolves the newest tomlkit — so CI is red while uv-locked local runs are green. main was last green 2026-07-13, before that release; the first red runs are today.

test (Windows) — infra, fails on every PR

Dies in Install Dependencies, before any repo test runs: failed to remove file ..._pydantic_core.cp312-win_amd64.pyd: Access is denied. (os error 5), which corrupts the install and yields ImportError: cannot import name '__version__' from 'pydantic_core'. Windows file-locking on a loaded .pyd. Fails on 8/8 recent runs across unrelated branches.

Both are repo-wide and block every open PR, so I've proposed them as separate follow-ups rather than smuggling a dependency-compat fix into a help-string PR, where it would muddy review and revert.

Verification of this PR's own diff

  • All 16 sites render, checked individually (11 × [remote|local|cache], 4 × nodes argument help, 1 × update target).
  • Counterfactual confirmed — reverting the two files to origin/main renders those cells empty; with the fix they show the choice lists. The fix is real, not cosmetic.
  • pytest tests/comfy_cli2471 passed, 13 skipped (matches baseline).
  • ruff format clean; ruff check → 14 errors, diffed against main and byte-identical — zero new.

@mattmillerai

Copy link
Copy Markdown
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded PR authored by the agent-work loop bug Something isn't working cursor-review Request Cursor bot review size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant