docs: clarify update/lifecycle entry points via help text (BE-3001) - #520
docs: clarify update/lifecycle entry points via help text (BE-3001)#520mattmillerai wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThe pull request clarifies help text for update, standalone, and manager commands. It distinguishes ComfyUI, CLI, and custom-node updates and describes standalone downloads and manager lifecycle operations. ChangesCommand help clarification
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ 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)
|
The failing |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
…E-3001)
Rich parses square brackets in Typer `help=` as console markup, so
`[all|comfy|cli]` was silently stripped from the rendered help. With this
PR's appended cross-reference the panel read:
Update ComfyUI Environment . To update custom nodes, use `comfy node update`.
— the target list gone and a stray " ." left behind. Escaping the opening
bracket makes it render literally:
Update ComfyUI Environment [all|comfy|cli]. To update custom nodes, use
`comfy node update`.
Self-review finding; verified by rendering `comfy update --help`.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🤖 Self-review pass — one real finding, fixed in d97d9d4. Rich was eating this PR's own help text. Square brackets in a Typer …target list gone, stray Verified by rendering the real Also verified the negative claims are accurate, since two of them deny a capability:
Deferred to a follow-up: 16 other Status: |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
|
ℹ️ Reviews-loop triage pass — no code change made; both red checks are pre-existing infra, not this PR. This PR is help-text-only (4 Typer
Confirmed red on #541, #543, #544, #545 as well. Deferred to a follow-up rather than fixed here — it is a cross-cutting dependency fix in an unrelated module, and folding it into a docs PR would bury it (and race the other open PRs hitting the same check).
Neither failure is attributable to this diff. Ready to merge once the |
|
✏️ Correction to my previous comment: the So the triage for #520 stands unchanged, with both red checks tracked elsewhere and neither caused by this diff:
Nothing actionable remains on this PR: no code change, no new ticket. |
|
This PR currently conflicts with Please rebase (or merge |
Resolves the conflict in cmdline.py: main added _switch_comfy_version() (comfy update comfy --version X) while this branch escaped the Typer help-string brackets; both kept.
|
Rebased on `main` in 6b5b23a — the only conflict was in `cmdline.py` (main's new `_switch_comfy_version`/`comfy update comfy --version X` path vs. this branch's escaped Typer help-string brackets on the same `update` command decorator); both are preserved. Verified post-merge: full suite green (3410 passed, 37 skipped), `ruff format`/`check` clean on the touched file, and all four `--help` outputs re-rendered correctly (brackets show literally, e.g. `Update ComfyUI Environment [all|comfy|cli]. ...`). No unresolved review threads. Ready for another pass. |
skishore23
left a comment
There was a problem hiding this comment.
The cross-references are a genuine improvement and I want them in. But there's a one-character regression in the comfy update help that's worth fixing first — in a help-text-only PR, the rendered output is the deliverable.
The \[ escape renders literally in this app
This line adds an escape that wasn't there before:
-@app.command(help="Update ComfyUI Environment [all|comfy|cli]")
+@app.command(help="Update ComfyUI Environment \\[all|comfy|cli]. To update custom nodes, use `comfy node update`.")Rendered comfy --help, before and after (merged origin/main into the head first):
main: │ update Update ComfyUI Environment [all|comfy|cli]
this: │ update Update ComfyUI Environment \[all|comfy|cli]. To update custom
The backslash reaches the user. This app runs typer 0.16.0 with rich_markup_mode never set (grepped — no occurrence anywhere in comfy_cli/), so Rich markup is not parsed in help strings and \[ is emitted verbatim rather than collapsing to [. The unescaped form on main renders correctly today.
Fix: drop the \\, keeping the rest of the sentence:
@app.command(help="Update ComfyUI Environment [all|comfy|cli]. To update custom nodes, use `comfy node update`.")Why the precedent is misleading
There is prior art for \\[ in the codebase — 17 occurrences — so I want to be clear it wasn't an unreasonable thing to copy. But those live on typer.Argument/typer.Option help, not @app.command(help=…), and they're broken too. Same file, current main, comfy update --help:
Update ComfyUI Environment [all|comfy|cli] <- command help, unescaped, correct
╭─ Arguments ─────────────────────────────────────╮
│ target [TARGET] \[all|comfy|cli] ... <- argument help, escaped, stray backslash
╰─────────────────────────────────────────────────╯
So the escape is a latent cosmetic bug throughout (cmdline.py ×2, custom_nodes/command.py ×15), and this PR would propagate it into the one place that was still right.
Bonus, entirely optional: since this PR is already about help-text quality, stripping all 17 \\[ → [ would be a tidy, mechanical follow-up (or an extra commit here). Every one of them currently shows a stray backslash. Your call whether to scope it in — the blocking ask is just the single line above.
Everything else checks out
- The other three strings are correct and render cleanly (
standalone,manager,node update) — verified incomfy --help. - Clean merge with
origin/main; CI green. - Your judgment call on keeping "or ComfyUI" in
node updateis right —comfy node update alldoes dispatch the same CM-CLIupdate allpath, so removing it would have made the help less accurate. - Agreed there's no snapshot to regenerate:
comfy discoverbuilds from the live Typer tree viabuild_help_json, andtest_command_mentions.pyresolves the new backtickedcomfy node update/comfy updatementions against the real tree — both are real commands, so the lint stays green.
Happy to approve as soon as the backslash comes off.
|
Re-checked this against the repo's actual locked dependency, and the escape is correct — please don't drop it.
Typer's default No stray backslash, target list intact — same as what merged in d97d9d4. If your test environment resolved typer 0.16.0, that's likely a stale/unlocked install ( Keeping the escape as-is. Happy to be wrong if you're seeing something different — if so, could you share your |
…update-lifecycle-help # Conflicts: # comfy_cli/cmdline.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/command/custom_nodes/command.py`:
- Line 790: Update the help text on the custom-node update command decorator to
describe only updating custom nodes; remove the claim that it updates ComfyUI
while retaining the guidance to use `comfy update` for ComfyUI or CLI updates.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bd174bb1-5d2e-4f5b-8532-6d21d0807c38
📒 Files selected for processing (2)
comfy_cli/cmdline.pycomfy_cli/command/custom_nodes/command.py
|
Rebased onto current Re-verified the Also replied to and resolved the new CodeRabbit thread on @skishore23 re-requesting your review — the merge conflict that was blocking you is resolved and CI is clean; the one open ask (the escape) is addressed above with reproducible evidence. Let me know if you're still seeing the backslash on your end and I'll dig into the env difference. |
ELI-5
comfyhas four commands that sound like they might all "update" things, and it's confusing which does what:comfy update— updates ComfyUI itself (or comfy-cli).comfy node update— updates custom nodes.comfy manager— turns ComfyUI-Manager on/off and configures it. Not an updater.comfy standalone— downloads a standalone Python interpreter bundle. Not an updater.This PR is help-text only — no renames, no aliases, no behavior change. It adds cross-references so
--helpsteers you to the right command, and relabels the two that were mislabeled as "update-ish".What changed
comfy updateUpdate ComfyUI Environment [all|comfy|cli]To update custom nodes, usecomfy node update.comfy node updateUpdate custom nodes or ComfyUITo update ComfyUI itself or the CLI, usecomfy update.comfy managerManage ComfyUI-Manager.Enable/disable and configure ComfyUI-Manager (lifecycle, not updates).comfy standaloneDownload a standalone Python interpreter…This bundles an interpreter; it does not update ComfyUI or custom nodes.Notes / judgment calls
node updatekept its "or ComfyUI" wording (appended, not replaced).comfy node update alldispatches the same CM-CLIupdate allcall thatcomfy update alluses, so "or ComfyUI" is factually accurate — removing it would be a regression. Followed the ticket's "append" instruction rather than rewording.comfy discoverself-describing surface builds its command tree from the live Typerappat runtime (build_help_json), and its tests validate against JSON schemas, not exact help strings — so the new text flows through automatically. Full unit suite is green (2471 passed, 13 skipped).comfy update,comfy node update) exist and are explicitly cross-referenced.Testing
ruff format+ruff checkclean on both touched files.comfy update|node update|manager|standalone --helprender the new strings correctly.pytest tests/comfy_cli→ 2471 passed, 13 skipped.