feat(generate): spend-gate — consent before credit spend (BE-4103)#577
feat(generate): spend-gate — consent before credit spend (BE-4103)#577mattmillerai wants to merge 2 commits into
Conversation
…alls (BE-4103) A generation call spends Comfy credits on invocation with no confirmation. Put the proxy call behind a consent interlock, ahead of every network side effect (auth refresh, asset uploads, the generation request): - interactive TTY: confirm prompt (default No) before spending - --yes flag bypasses (automation / evals) - spend.auto_confirm config persists always-proceed; new `comfy generate consent [show|always|ask]` action manages it - --json / non-TTY with neither: fail closed with spend_consent_required (exit 1, nothing spent) — never hang on a prompt no machine caller can answer Ungated: list/schema/refresh/upload/resume/--emit-workflow (spend nothing).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 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 (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
Shell execution is blocked here, but the file contents plus static analysis let me adjudicate confidently. Key verifications:
- **Interpolation crash (real):** `ConfigManager.get_bool` → `configparser.getboolean`, which applies interpolation before boolean conversion. A `%` in the value raises `configparser.InterpolationError` (subclass of `configparser.Error`, **not** `ValueError`), so `except ValueError` misses it. Confirmed real; 3 reviewers across labs/types.
- **`--yes` fail-open (real):**
Re-trigger by removing and re-adding the cursor-review label.
ELI-5
comfy generate flux-pro --prompt "a cat"costs real money (Comfy credits) the moment you run it — and until now it never asked. This PR makes it ask first. At a terminal you get a "this spends credits — proceed?" prompt; scripts and agents must pass--yes(or the user can persist "always proceed" once withcomfy generate consent always); and a machine call (--jsonor no terminal) with neither errors out cleanly instead of spending silently or hanging on a question nobody can answer.What
The money interlock for
comfy generate(launch blocker for exposing generation through the local MCP, parent BE-4102):default No) before any credit-spending proxy call.--yes— explicit bypass for automation/evals (new meta flag, stripped like--async/--json).spend.auto_confirmconfig — persistent always-proceed, managed by a newcomfy generate consent [show|always|ask]action; stored inconfig.iniviaConfigManager(see judgment calls).--jsonor no stdin TTY with neither consent source → errorspend_consent_required(registered inerror_codes.REGISTRY), exit 1, nothing spent, never a hanging prompt.The gate sits in
_generatebefore every network side effect — before OAuth refresh (resolve_api_key), before asset-upload transforms, before the generation request. Ungated because they spend nothing:list/schema/refresh/upload/resume(polls an already-paid job) /--emit-workflow(local artifact, no proxy call).Consent failures emit
generate:errorwitherror_kind="consent"; theconsentaction emitsgenerate:consent. Docs updated: top help, per-model schema help, and the agent-facingskills/comfy/SKILL.md(explicitly tells agents not to reflexively add--yes— it must represent real human approval).Acceptance criteria
comfy generate <model> --jsonwith no consent → errors, spends nothing ✅ (test + live CLI run: exits 1 withspend_consent_requiredbefore any network call)--yesorspend.auto_confirm=true→ proceeds ✅ (tests for both, plus config-persistence-to-disk check)Judgment calls
spend.auto_confirmlives inconfig.ini(ConfigManager), notsecrets.json. The ticket says "same persistence that backscomfy cloud set-key", which literally is the auth-record store (secrets.json, provider→key schema). A boolean preference doesn't fit that schema;config.iniis the CLI's canonical settings persistence (same durability, same directory). Easy to move if the literal store was intended.comfy generate consent [show|always|ask]— the ticket asks for the config but no setter; without one, users would hand-editconfig.ini. Small (~40 lines), makes the acceptance path actually usable, and gives the fail-closed error a concrete remediation to name.stdin=TTY, stdout=redirectedwill prompt into the redirect — consistent with every other prompt in this CLI (typer.confirmdefaults); the no-input-TTY hang case fails closed.comfy generateinvocations now require--yes— that is the ticket. Fallback for garbagespend.auto_confirmvalues is fail-closed.--yes,consent always), and each of those paths is empirically proven by tests + a live CLI run.Tests
tests/comfy_cli/command/generate/test_spend_gate.py— 19 tests: fail-closed (json / pretty-non-TTY / async / pre-auth), bypasses (flag, config true/false/garbage), interactive prompt (accept/decline/default), ungated paths (emit-workflow, schema errors), lifecycle telemetry, and theconsentaction incl. on-disk persistence.conftest.pyautouse fixture pre-authorizes the gate for the existing 177 generate tests (they exercise post-gate behavior; CliRunner has no TTY), overridden by name in the gate's own tests.test_validate_commandreds from main (BE-3357×BE-3359, fix in test: fix repo-wide pytest failure — reconcile validate-command fixtures with the no-outputs check (BE-4097) #576). Ruff format clean; the 15 localruff checkUP038 hits are the known ruff-0.12.7-vs-CI-0.15.15 false flags in untouched files.Safety note (from the ticket): human review required before merge — this is the money interlock. Do not auto-merge.