refactor: extract _enforce_spend_gate from run_template_cmd (BE-4367) - #599
refactor: extract _enforce_spend_gate from run_template_cmd (BE-4367)#599mattmillerai wants to merge 2 commits into
Conversation
Pure code motion of the BE-4113 spend-consent gate (templates.py:891-922) into a module-level _enforce_spend_gate helper next to the other gate helpers. No behavior change: the block is moved verbatim (only a function-local import sys is added; the command's now-unused import sys is removed). Single call site. Adds a TestEnforceSpendGate class with direct unit tests for every branch, including the previously-untested interactive-accept path (pretty + tty + confirm-yes -> run proceeds).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 18 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.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
… (BE-4367) Harden the extracted _enforce_spend_gate per cursor-review: - Escape name/paid_nodes/gallery_signals before rprint so a template name or provider containing Rich markup (e.g. '[test]') no longer raises MarkupError / spoofs the consent warning. The structured error 'details' still carries the raw values. - Guard sys.stdin before .isatty(): when stdin is None (detached process / closed stream) fall through to the non-interactive hard-fail branch instead of crashing with AttributeError. Adds two direct tests covering both paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Heads-up for the move-only review: the extracted
Control flow is otherwise the verbatim BE-4113 gate. Added The third finding (Medium — fail-open money gate when |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
ELI-5
Before running a gallery template,
comfy run-templatechecks a "spend gate":if the template uses partner-API nodes that cost Comfy credits and you didn't
pass
--allow-spend, it either asks you to confirm (in an interactive terminal)or hard-fails with a "you must consent" error. That gate was ~30 lines buried in
the middle of the big
run_template_cmdfunction. This PR lifts those linesout, unchanged, into their own
_enforce_spend_gatehelper so the gate is asingle named thing you can read and test on its own — and adds direct unit tests
for it, including the "user says yes at the prompt" path that nothing tested
before.
What changed
templates.py:891-922(the BE-4113 spend gate) movedverbatim into a new module-level
_enforce_spend_gatehelper placed nextto the other gate helpers (
_detect_paid_nodes,_gallery_paid_signals). Theonly additions are a function-local
import sys(the gate usessys.stdin.isatty()); the command body's now-unusedimport sysis removed(ruff confirms it was unused after the move). The call site is replaced with a
single keyword-arg call; the original
-- Spend gate (BE-4113)comment iskept.
called unconditionally exactly where the inline block ran, returns
Nonewhenthe run may proceed, and raises
typer.Exit(1)otherwise.TestEnforceSpendGateintests/comfy_cli/command/test_run_template.pycalls_enforce_spend_gatedirectly across all six branches: allow-spend proceeds, non-interactive
hard-fail (envelope +
details.partner_nodes), interactive decline, thepreviously-untested interactive ACCEPT path, no-paid-signals (confirm never
consulted), and gallery-signals-only (fail-open
object_info={}+tag:API). No existing test was modified.Review note — please diff move-only
This is a security-sensitive money/consent gate. Per the ticket, this must be
pure code motion: human review that the block was moved with no condition
inverted / no branch dropped is requested before merge. The moved region is
character-for-character identical to the deleted region (only
import sysaddedto the helper).
Testing
uv run pytest tests/comfy_cli/command/test_run_template.py tests/comfy_cli/command/test_templates.py→ 31 passeduv run ruff check comfy_cli/command/templates.py→ All checks passed