Skip to content

test: lint comfy hint-strings + snapshot the command inventory - #516

Open
mattmillerai wants to merge 8 commits into
mainfrom
matt/be-2979-hint-lint-discover-snapshot
Open

test: lint comfy hint-strings + snapshot the command inventory#516
mattmillerai wants to merge 8 commits into
mainfrom
matt/be-2979-hint-lint-discover-snapshot

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

Some of the CLI's help messages and error hints told you to run commands that
don't exist — like comfy auth login (the real one is comfy cloud login).
Nobody noticed for a long time. This adds two tests that act like a spell-checker
for command names:

  1. A lint that reads every comfy … command mentioned in help text, hints,
    and error messages and checks each one is a real registered command. If a
    string points at a command that doesn't exist, the test fails and names the
    exact string + file.
  2. A snapshot of the full command list, so adding or removing a command shows
    up as an obvious diff in review (mirrors the MCP server's EXPECTED_TOOLS
    pattern).

Turning the lint on immediately caught live rot — five hints pointed at the
removed comfy auth whoami — so those are corrected to comfy cloud whoami too.

What & why

The MCP server prevents help/hint rot with EXPECTED_TOOLS + description-budget
snapshot tests; the CLI had no equivalent, which is how seven strings referenced
the nonexistent comfy auth login for who knows how long.

1. Hint-string lint (tests/comfy_cli/test_cli_surface.py)

  • Extracts every recognizable comfy … invocation — backtick-quoted,
    run:/$-prefixed shell hints, and whole-string command lines — from all
    Python string literals (docstrings, help=/hint=/message= kwargs,
    examples) and bundled skill docs, scanned via AST so comments and code
    identifiers are never mistaken for references.
  • Validates each against the registered command set by walking the Typer app
    tree (build_help_json). Correctly accepts groups, leaves-plus-args
    (comfy generate list), and hidden commands (comfy agent-review); flags the
    "valid group + nonexistent subcommand" rot class (comfy auth login/whoami).
  • Fails naming the offending string + file. Extraction is marker-gated, so prose
    like "the comfy skills into Claude Code" is not mistaken for a command
    (verified: zero false positives across the whole package).

2. Command-inventory snapshot — snapshots every command path (groups +
leaves, with a [hidden] marker) to tests/comfy_cli/fixtures/cli_command_inventory.txt.
Regenerate after an intentional surface change with one command:

UPDATE_CLI_SNAPSHOT=1 pytest tests/comfy_cli/test_cli_surface.py

Both run under the normal pytest invocation, so they gate every PR in CI.

Rot the lint surfaced (and what this PR does about it)

  • comfy auth whoami (5 hints) → comfy cloud whoami. auth whoami was
    deliberately removed (there is an existing test_legacy_auth_whoami_is_gone);
    the hints were stale. All five are cloud-auth contexts, so they route to the
    real comfy cloud whoami. Fixed here.
  • comfy query (2 refs — an example + a comfy run-cli demo step). The CQL
    query surface is documented but no query command is registered on the app.
    Whether to wire one up or rewrite the examples is a product call, so it is
    baselined (BASELINE_INVALID_REFS) for a separate fix rather than bundled
    into this guard. test_baseline_is_not_stale fails the moment it's fixed,
    forcing the baseline entry to be removed so the allowlist can't outlive the bug.
  • comfy version is allowlisted — it's the --version flag's output-envelope
    command name (in discovery.COMMAND_SCHEMAS), not an invokable subcommand.

Acceptance

  1. Would have caught all 7 auth login refs — pinned by the regression
    fixture in test_lint_catches_auth_login_and_friends (backtick, run:, and
    whole-string forms), per the ticket's "add one as a regression fixture" note.
  2. Snapshot update path documented — one command, above and in the test.
  3. Runs in CI on every PR — plain pytest, picked up by the existing pytest
    workflow.

Notes for the reviewer

  • Stacked PR. Based on the branch that routes the seven auth login hints to
    comfy cloud login; the live lint depends on those being fixed to stay green.
    GitHub will retarget this to main when that PR merges.
  • The two source-file kinds of change are: the new tests/snapshot, and the
    auth whoami → cloud whoami corrections the lint immediately surfaced.
  • Full tests/comfy_cli suite green (2475 passed, 13 skipped); ruff check +
    ruff format clean.

Judgment calls

  • Fixed the auth whoami rot inline (clean, unambiguous, test-backed) but
    baselined comfy query (needs a product decision) — kept this PR focused on
    the guard plus the rot that's mechanically safe to fix.
  • The lint scans bundled *.md skill docs too (agent-facing help); all current
    refs there are valid.

mattmillerai and others added 4 commits July 14, 2026 12:23
…comfy auth login`

The `auth` command group manages third-party model-host API tokens
(Civitai, Hugging Face) and has no `login` subcommand. Comfy Cloud
sign-in is `comfy cloud login`. Seven help/hint/error strings (plus a
stale comment) pointed users and agents at the dead `comfy auth login`,
including the agent-facing `jobs --where` option help. Correct them all
and tighten the signed-out banner tests to guard the hint.
The interactive demo's sign-in step invoked `comfy auth whoami`, which was
deliberately removed (test_legacy_auth_whoami_is_gone) — the command now lives
at `comfy cloud whoami`. Update the step title, both invocations, and the
capabilities summary so the walkthrough runs a command that actually exists.
Adjust the step-coverage test needle from "auth" to "whoami" to match.

Addresses cursor-review (gemini-3.1-pro edge-case) thread on PR #512.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Four cloud error paths (job cancel, workflow HTTP errors, cloud model
list/search) still pointed users at `comfy auth whoami`, which does not
exist — the `auth` group only has list/set/remove, and sign-in status
lives under `comfy cloud whoami`. Same class of bug this PR set out to
fix for `auth login`.

Also strengthen the run_cli demo test per CodeRabbit: assert the whoami
step's argv/labels target `cloud whoami` and never `auth whoami`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add two anti-rot CLI-surface guards, mirroring the MCP server's
EXPECTED_TOOLS + description-budget snapshot pattern:

- Hint-string lint: extract every backtick/shell-hint `comfy …`
  invocation from help text, docstrings, hint/error strings, examples,
  and bundled skill docs, then validate each against the registered
  command set by walking the Typer app tree. Fails naming the offending
  string + file. A regression fixture pins that it flags the historical
  `comfy auth login` rot (and the sibling `auth whoami`).
- Command-inventory snapshot: snapshot the full command tree so surface
  additions/removals/hides are explicit diffs in review. Regenerate with
  UPDATE_CLI_SNAPSHOT=1 pytest tests/comfy_cli/test_cli_surface.py.

The lint immediately surfaced live rot: five hint strings pointed at the
nonexistent `comfy auth whoami` (deliberately removed — see
test_legacy_auth_whoami_is_gone). Route them to the real `comfy cloud
whoami`. The remaining `comfy query` rot (CQL surface documented but no
command registered) is baselined for a separate fix.
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a1d8a266-c7e3-43ba-a06a-1d8cf59debea

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matt/be-2979-hint-lint-discover-snapshot
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-2979-hint-lint-discover-snapshot

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

@mattmillerai
mattmillerai marked this pull request as ready for review July 14, 2026 20:07
@mattmillerai mattmillerai added cursor-review Request Cursor bot review agent-coded PR authored by the agent-work loop labels Jul 14, 2026
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jul 14, 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.

⚠️ Review failed

Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
I have enough to adjudicate. The shell/web tools are blocked in this environment, so I verified everything by reading the actual file and reasoning from Python internals.

Key adjudication decisions:

- **Dropped the gemini "high" AttributeError claim (line 172).** The claim that `ast.Constant` nodes inside f-strings lack `.lineno` in Python 3.10/3.11 is false — parsed f-string literal parts carry valid (if historically imprecise) position info; `.lineno` never raises `AttributeError` on parsed 

Re-trigger by removing and re-adding the cursor-review label.

mattmillerai and others added 4 commits July 17, 2026 04:49
This PR is scoped to routing user-facing `comfy auth login`/`auth whoami`
hints at the real `comfy cloud login`/`cloud whoami` commands. It had also
picked up a 287-line `uv.lock` re-sync that locks `anthropic`, `pydantic`,
and `pydantic-core` — unrelated to the hint strings and not load-bearing
for any test here.

main's lockfile is genuinely stale (`uv lock --check` fails on main: the
`bench` extra declares `anthropic>=0.40` but was never locked), so the
re-sync is real work — just not this PR's. No CI job consumes `uv.lock`
(ci-cursor-review explicitly excludes it), so reverting restores main's
status quo with no regression. Tracked as a follow-up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both conflicts were cosmetic overlaps from main's own auth-copy cleanup
(BE-2996, #616) landing alongside this branch's identical intent:
- test_run_cli.py: keep main's "cloud whoami" needle (matches the actual
  step title produced by run_cli.py)
- test_branding.py: keep both assertions — main's tightened "comfy cloud
  login" check plus this branch's "comfy auth login" not in out guard
…' into matt/be-2979-hint-lint-discover-snapshot

# Conflicts:
#	tests/comfy_cli/command/test_run_cli.py
Merging in the base branch pulled in a wave of new commands from main
(model downloads, node deps, run-template, system-stats, workflow
notes, etc.) that weren't in this PR's committed inventory snapshot.
Separately, the base's run-cli rewrite already replaced the invalid
`comfy query` reference with `comfy nodes ls`, so that BASELINE_INVALID_REFS
entry is now stale per test_baseline_is_not_stale's own contract.
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jul 30, 2026
Base automatically changed from matt/be-2975-fix-auth-login-refs to main August 4, 2026 20:26

@skishore23 skishore23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The command-inventory snapshot here is genuinely valuable and I'd like to see it landmain has no equivalent. But the lint half is now a duplicate of a stronger guard that already shipped, and I think it should come out before merge. Details below; it's a small, surgical cut.

The overlap

This PR was opened 2026-07-14. Since then #616 ("fix: point CLI copy at commands that exist + lint it", BE-2996) landed on main on 2026-07-28, adding comfy_cli/command_mentions.py + tests/comfy_cli/test_command_mentions.py — the same hint-string lint, same ticket family. After merging origin/main into this head, the branch carries both.

That alone would just be redundancy. The problem is which one is weaker.

The lint in this PR has a blind spot main's has already closed

_leading_command_tokens stops at the first token that isn't [a-z][a-z0-9-]*, so a leading global option swallows the whole reference. main's resolver deliberately skips root-callback options and keeps scanning. Ran both against the same strings on the merged tree:

reference this PR flags main flags
comfy --json auth login False True
comfy auth login True True
comfy --where cloud auth whoami False True
comfy --json cloud singin False True

main has an explicit regression test for exactly this (test_invalid_mentions_are_flagged[comfy --json auth login-login]) with a comment noting that otherwise "prefixing the bad command with --json would hide it from the lint entirely."

Two further gaps: this lint scans comfy_cli/**/*.py + comfy_cli/**/*.md but not README.md (main scans it, and has test_scan_reaches_the_readmes_fenced_examples), and _WHOLE_LINE's [a-z0-9 _-]*$ can't match a bare line carrying a path, so comfy run --workflow ./workflow.json isn't extracted.

To be clear: merging this does not weaken anything — main's lint still catches all of the above, so combined coverage is unchanged. The cost is maintenance, and it's real: ~200 lines of parallel extraction logic, and two separate allowlists (ALLOWED_NONINVOKABLE_REFS / BASELINE_INVALID_REFS here vs PROSE_SCAN_EXCLUDED there). A contributor who adds a command and gets flagged has to know both exist and update the right one. The weaker implementation is also the one with the more inviting name (test_cli_surface.py), so it's the one people will reach for.

What I'd suggest

Keep test_command_inventory_snapshot + _command_inventory() + the fixture; drop test_no_invalid_command_references, test_baseline_is_not_stale, test_lint_catches_auth_login_and_friends, and the extraction helpers (_refs_in_text, _leading_command_tokens, _ref_is_valid, _command_tree, _iter_source_strings, _scan_invalid_references, both allowlists, the three regexes). The snapshot test only needs build_help_json, so it's cleanly separable — no dependency on any of the lint machinery.

If any of the three lint tests cover a case main's doesn't, the better move is to port that case into tests/comfy_cli/test_command_mentions.py rather than keep a second scanner alive.

What I verified (all green)

  • Merged origin/main into the head — clean merge, no conflicts.
  • test_cli_surface.py + test_command_mentions.py35 passed together.
  • The snapshot is not stale: regenerated it with UPDATE_CLI_SNAPSHOT=1 against current main and diffed — zero drift, 152 entries. Nice job reconciling that in 21a890f.
  • BASELINE_INVALID_REFS is now empty, so the comfy query rot mentioned in the description is already resolved — the description's "baselined for a separate fix" paragraph is stale and should come out of the squash message.

Happy to flip to approve on a push that drops the duplicate lint — or, if you'd rather keep both and reconcile later, say so and I'll approve as-is; the coverage argument above is about maintenance cost, not correctness.

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 cursor-review Request Cursor bot review size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants