Skip to content

feat: add Flow credit balance commands - #671

Merged
ffroliva merged 2 commits into
ffroliva:developfrom
ziyedbe:feature/flow-credits
Sep 5, 2026
Merged

ffroliva merged 2 commits into
ffroliva:developfrom
ziyedbe:feature/flow-credits

Conversation

@ziyedbe

@ziyedbe ziyedbe commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • add explicit gflow credits user / gflow credits list commands and the matching gflow_get_credits MCP tool
  • make the primary path genuinely browser-free: the labs.google cookie client is closed before a separate cookie-free, Bearer-only credits request
  • share the retrying Flow session probe with auth verification, preserving profile-boundary validation and 429/503/504/network retries
  • preserve partial multi-profile results and safe diagnostics without logging credentials or response values
  • document the command across user and agent surfaces, including that credits fund Veo video while image generation has separate per-model daily quotas

Review fixes

  • rebased onto current develop after v0.68.0 and kept the changelog entry under [Unreleased]
  • removed ambiguous bare aliases and swallowed group flags; the public CLI is now only credits user and credits list
  • added human-readable output, default UX, HTTP error-branch, typed failure, security-boundary, retry, cookie-isolation, and parity coverage
  • added a marker-gated live e2e test that hard-fails if Playwright fallback is reached

Verification

  • uv run ruff check src tests
  • uv run ruff format --check src tests
  • uv run pyright src — 0 errors, 0 warnings
  • repository hygiene, documentation links, website mirror, and council-memory gates
  • full suite — 3,947 passed, 7 skipped, 100 deselected; 92.03% coverage
  • live HTTP-only e2e — status 200; redacted response keys: credits, serviceTier, sku, subscriptionCredits, userPaygateTier; unknown key count 0; 1 passed in 1.49s
  • exact-commit council review — GREEN at a46188d1e1c6ef416c6780f05cebcd4688da4b4b

The live e2e patches the Playwright cookie fallback to raise immediately, so its passing result confirms the credits request completed through the HTTP fast path without launching a browser.

@ziyedbe
ziyedbe requested a review from ffroliva as a code owner September 5, 2026 13:38
@github-actions github-actions Bot added external-contribution Pull request from a non-maintainer or fork needs-copilot-review Ask GitHub Copilot for advisory code review needs-maintainer-review Requires maintainer review before merge labels Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

Thanks for the contribution. This PR has been marked as an external contribution and routed for maintainer review.

Before merge, please make sure:

  • The PR targets develop.
  • Commits use a real Git identity or GitHub noreply email.
  • External commits include a DCO sign-off: git commit -s.
  • The PR does not include secrets, cookies, account tokens, signed URLs, or private captured data.
  • Focused tests/docs are included for behavior changes.

GitHub Copilot code review may be requested as an advisory first pass, but maintainer approval is still required.

@ffroliva

ffroliva commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Maintainer council review — verdict 🔴 RED (reviewed at 73c57db)

Thanks for this — it's a well-shaped contribution: the DTO fails closed instead of coercing bad values to zero, partial-result aggregation across profiles is right, the BDD stubs mirror the real signatures exactly, and the MCP parity mapping covers all three leaves. Local CI gates (ruff, format, doc-links, repo hygiene, website-docs mirror, council-memory) are all green at head.

The blocker is that the HTTP fast path — the feature's whole premise — does not work on this machine. Details below so you can reproduce rather than guess.

Must-fix

1. The HTTP fast path fails and falls back to a browser on every profile.

Running gflow credits list here against real saved profiles (zero credits — read-only GET) logged, for both profiles:

{"profile":"<redacted>","error_type":"AisandboxAuthError","event":"credits.http_fallback_to_browser"}

each followed by browser.engine_selected engine=playwright and a real chrome.exe launch. So gflow credits list is currently N sequential Chrome launches, not a browser-free HTTP call.

AisandboxAuthError is raised at only two sites in api/credits.py — "no access_token in Flow session" and "credits endpoint returned 401/403" — so the labs.google session call succeeded and the failure is downstream of it. Finding #2 is the leading suspect.

2. The cookie jar is sent cross-host. src/gflow_cli/api/credits.py:53-94

httpx.AsyncClient(cookies=...) is built from a plain dict[str, str], so httpx attaches no domain scoping and sends the whole jar to every host the client touches. The same client then makes the credits call to a different host:

https://labs.google/fx/api/auth/session         -> SIDCC=<value>
https://aisandbox-pa.googleapis.com/v1/credits  -> SIDCC=<value>   # same client

This contradicts the function's own docstring ("Neither credential is returned, persisted, or logged", cookies scoped to labs.google) and the intent stated at auth/cookies.py:30. follow_redirects=False doesn't mitigate it. Fix: use a separate bare client for the credits request (header-only), or client.cookies.clear() between the two calls. Worth checking whether this is also what causes #1.

3. No retry on the session probe. src/gflow_cli/api/credits.py:57-70

auth/verification.py:295-335 performs the identical cookie→session httpx call — same four headers, same client kwargs — but with _MAX_ATTEMPTS = 3, _RETRYABLE_STATUSES = {429, 503, 504} and exponential backoff, and documents why: "so the httpx fast path and the Playwright path have identical durability characteristics. A single transient 429/503/504 or network blip will not reject a valid login." The new copy has none of it — one transient 429 drops straight into a Chrome launch. Extracting one shared helper for that block resolves both this and the duplication.

4. Docs contradiction + a broken anchor shipped to the public site.

docs/USER_GUIDE.md:409-415 (untouched here) still says "Flow does not expose a credit-balance API today… There is no programmatic way to fetch this from gflow-cli yet" and links KNOWN_ISSUES.md#no-in-cli-quota-visibility. Both sentences are now false, and renaming that heading to ### No in-CLI quota visibility — resolved changes its slug, so the fragment no longer resolves — in the repo and in the mirrored website/docs/USER_GUIDE.md:415. scripts/ci/check_doc_links.py checks anchors for file existence only (see its lines 4-5), so it cannot catch this.

5. Agent-facing surfaces omit the command. AGENTS.md:47's "Command surface" bullet lists all 16 other groups but not credits; the "Project at a glance" module inventory omits cli_credits.py; llms.txt and docs/INDEX.md have no mention. These are the surfaces no test covers.

6. Test gaps on the default UX path.

  • Bare gflow credits and gflow credits --all (cli_credits.py:62-72) are never invoked by any test.
  • _render_one / _render_many (cli_credits.py:18-44) are 0% covered — every test passes --json, so the human-readable output is entirely unverified.
  • 4 of 8 HTTP error branches in api/credits.py are untested, including the exact branch firing in docs: add autonomous bug-fix and lessons-capture rules to agent reasoning style #1.
  • _failure's GFlowError branch (exc.title) is untested; the one failure test raises a bare RuntimeError.

7. No e2e test. All eight new test files are offline — every HTTP and browser call is faked, and pytest.mark.live / pytest.mark.e2e appear zero times across them. tests/e2e/ already holds the closest siblings to this code (test_aisandbox_auth_live.py, test_auth_verification_e2e.py exercise the same cookie→session→Bearer path), gated on GFLOW_CLI_E2E_PROFILE. Since credits is a read-only GET that spends nothing, an e2e test there is cheap — and it would have caught #1 before this PR was opened.

Nice-to-have

  1. services/credits.py:32-38 vs :70-83 — _success spreads **asdict(info) while _failure hardcodes the same five keys. Derive both from dataclasses.fields(CreditsInfo) so a new DTO field can't silently skip the error path.
  2. Three spellings for two operations: credits ≡ credits user, credits --all ≡ credits list, and credits user takes the profile as both a positional and an option with a hand-written conflict check that exists nowhere else in the repo. gflow auth is real precedent for invoke_without_command=True, but its bare form does something distinct rather than aliasing a subcommand. Trimming to credits user [--profile] + credits list removes ~20-30 lines.
  3. Group-level flags are silently swallowed when a subcommand is given — gflow credits --json user prints a Rich table, because the callback returns at cli_credits.py:65-66 without storing to ctx.obj. Either propagate them or drop the group options.
  4. credits.http_fallback_to_browser records only error_type, which is precisely why docs: add autonomous bug-fix and lessons-capture rules to agent reasoning style #1 can't be diagnosed from its own log. Including the HTTP status would distinguish "wire drifted" from "cookies expired".
  5. verification.py:233,305 calls _validate_profile_in_home before touching a profile dir; fetch_credits_http doesn't. Not exploitable today (paths come from profile_store), but it's an asymmetry in a security boundary.
  6. cli_credits.py:49,54 use a conditional expression as a statement; all six sibling call sites use if/else blocks.
  7. Nothing in the new prose notes that this balance is drawn down by video only — image generation consumes a separate per-model daily quota. A reader will assume one unified balance.

Merge-order note

The CHANGELOG hunk correctly targets [Unreleased], but the in-flight v0.68.0 release moves the gflow update block (this hunk's context) into a versioned section. Once v0.68.0 back-merges to develop, this will need a rebase.

Verification

Because this is a fork PR, SonarCloud is skipped — no server-side gate runs on it at all. The test-plan boxes for the two live runs carry no artifacts; given #1, please include the HTTP status and the redacted response keys from a successful run when you next update, so the fast path can be confirmed rather than assumed.

ffroliva added a commit that referenced this pull request Sep 5, 2026
…er clause to the merge

Council review of #677 found the substitution still reachable -- not through
the wording, but through the verdict colour. D4 said "must-fix" while scoring
YELLOW, and a YELLOW is dismissable under section 5 step 8 with a one-line
justification. "The live runs in the PR body are good enough" is precisely the
justification someone writes, which reopens the ledger-for-e2e substitution
through the escape valve rather than through the text.

- Absent with no reason given now scores D4 RED, forcing consensus RED under
  section 5 step 3, and the skill says why it is not YELLOW so nobody softens
  it back later.
- Absent WITH a stated reason stays YELLOW, blocked on a maintainer run -- that
  is the documented exemption, not an evasion.
- Present is now verified rather than trusted: cite the tests/e2e file:line,
  confirm it exists at REVIEWED_SHA, confirm it exercises the changed surface.
  A passing e2e test that never touches the changed path is
  pr-must-verify-on-affected-surface wearing an e2e label.
- The grandfather clause said "PRs opened before 2026-09-05", which excluded
  the only two PRs it exists for: #669 opened 11:28Z and #671 at 13:38Z, both
  hours before #675 merged at 19:15Z the same day. Now anchored to the merge
  commit, not the calendar day.

The published slug carries the same RED wording, so the memory a future
reviewer loads cannot disagree with the skill that cites it.
@ziyedbe
ziyedbe force-pushed the feature/flow-credits branch from 73c57db to a46188d Compare September 5, 2026 21:11
ziyedbe pushed a commit to ziyedbe/gflow-cli-fork that referenced this pull request Sep 5, 2026
E2E is the layer that proves a change works against the real Flow blackbox;
offline-green only proves our code does what we think it does. Nothing in the
contributor path asked for it: the lifecycle table routed only generation-path
changes to live-verify, "Test categories" documented how to run e2e but framed
it as opt-in maintainer regression, and the PR template's only live-Flow line
was scoped the same way. External PR ffroliva#671 followed all of it to the letter and
arrived with eight offline test files, zero e2e -- and its HTTP fast path failed
on the first real run.

Also records what live-verify is not: it drives CLI commands by hand into a
gitignored note and never runs `pytest -m e2e`. An e2e test is a re-runnable
regression; a live-verify ledger is a narrative record. Both, not either.

- CONTRIBUTING.md: new lifecycle row for any behavior change; fourth pipeline
  rule; "E2E is required, not optional" in the test-categories section
- AGENTS.md: same rule where agents read it, plus the cost sub-marker list
- .github/PULL_REQUEST_TEMPLATE.md: an e2e line that has to be answered

Drive-by: AGENTS.md said "six gates above" over a nine-command block.
@ffroliva

ffroliva commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Maintainer council review — verdict 🟢 GREEN (reviewed at a46188d1e1c6ef416c6780f05cebcd4688da4b4b)

All 7 must-fixes and all 7 nice-to-haves from the previous RED review are addressed. Merging.

Must-fix #1 — reproduced as fixed on the same machine that produced the RED

The blocker was that the HTTP fast path fell back to a real chrome.exe launch on every profile here. It no longer does:

{"status_code": 200,
 "response_keys": ["credits", "serviceTier", "sku", "subscriptionCredits", "userPaygateTier"],
 "unknown_key_count": 0,
 "event": "credits.http_fast_path_succeeded",
 "cli_command": "credits user"}

gflow credits user returned credits: 875, sku: G1_TIER1 with no browser launch. gflow credits list read 3 profiles over HTTP and preserved partial results for the 6 stale ones (expired cookies / newer-Chromium guard), Total credits: 1566 — the aggregation and the human-readable table both behave.

The --json run also confirms the diagnostic gap from nice-to-have #4 is closed: credits.http_fallback_to_browser now carries status_code and route, so a future regression is diagnosable from its own log.

Verification of the remaining items

Item Evidence at head
#2 cookie jar sent cross-host The labs.google session client is closed before a separate bare, header-only client makes the aisandbox call (api/credits.py:71). No jar crosses the origin boundary.
#3 no retry on the session probe Both callers now share fetch_flow_session_httpx, which keeps _MAX_ATTEMPTS = 3 and the 429/503/504 backoff. The duplication is gone as well.
#4 docs contradiction + broken anchor docs/USER_GUIDE.md:409 rewritten; the dead #no-in-cli-quota-visibility fragment removed; website/docs mirror regenerated and in sync.
#5 agent-facing surfaces credits added to the AGENTS.md command surface, cli_credits.py to the module inventory, plus llms.txt and docs/INDEX.md.
#6 test gaps on the default UX path api/credits.py 100%, cli_credits.py 100%, services/credits.py 98% (43 tests). _render_one / _render_many were 0% and are now fully covered.
#7 no e2e test tests/e2e/test_credits_e2e.py, marked e2e + e2e_auth, gated on GFLOW_CLI_E2E_PROFILE. Ran live here: 1 passed in 1.48s. It patches the Playwright cookie fallback to fail immediately, so a green result is the browser-free proof — exactly the check that would have caught #1 before the PR was opened.

Nice-to-haves 1, 2, 3, 5, 6 and 7 are all in as well: _failure now derives its keys from dataclasses.fields(CreditsInfo), the CLI is trimmed to credits user [--profile] + credits list with no swallowed group flags, _validate_profile_in_home guards the shared probe, the conditional-expression statements are if/else, and the video-only nature of this balance is stated in the user guide, KNOWN_ISSUES and llms.txt.

Gates at head

check_repo_hygiene · check_doc_links · check_website_docs_pii · generate_website_docs --check · check_council_memory · ruff check · ruff format --check — all green. pyright src — 0 errors, 0 warnings. Rebased cleanly onto develop after v0.68.0 with the CHANGELOG entry still under [Unreleased], so the merge-order note is resolved.

SonarCloud is skipped on fork PRs, so this maintainer review is the substitute server-side gate.

One residual, not a blocker

verify_flow_profile now validates the profile path twice — once itself, once inside the extracted helper. Harmless and cheap, and the outer call deliberately stays outside the fail-closed except Exception so a SecurityError still propagates rather than being downgraded to VERIFICATION_ERROR.

Thanks for the thorough turnaround — particularly for adding the e2e test that makes the central claim self-proving rather than asserted.

@ffroliva
ffroliva merged commit e2c69be into ffroliva:develop Sep 5, 2026
18 checks passed
@ffroliva

ffroliva commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Merged as e2c69be — gflow credits user|list and gflow_get_credits are on develop, and the post-merge CI run is green across all 11 jobs including SonarCloud.

Thank you for this. A few things about how you handled it are worth naming, because they are not the norm:

You fixed the blocker rather than arguing with it. The RED said the HTTP fast path fell back to a real Chrome launch on every profile on my machine, which is a hard thing to receive when it passed on yours. You went and found the cross-host cookie leak underneath it instead of asking me for more logs. Re-tested here on the same machine that produced the RED: status_code=200, no browser, 875 credits.

You extracted the shared session probe instead of copying the retry block. fetch_flow_session_httpx now serves both credits and auth verification, so the durability characteristics cannot drift apart again, and the _validate_profile_in_home asymmetry I flagged as a nice-to-have disappeared for free. That is the fix that makes the next bug not happen.

The e2e test is the part I will be pointing other contributors at. Patching the Playwright cookie fallback to fail immediately means a green run is the browser-free proof, not an assertion about one. This project reverse-engineers a blackbox — offline-green only ever proves our code does what we think it does, never that Flow still behaves as captured — so a test that makes its own central claim falsifiable is exactly the right shape. It is also, precisely, the test that would have caught the blocker before the PR was opened.

And you took all seven nice-to-haves, including the one that was really a documentation problem: the note that this balance funds Veo video while image generation draws on separate per-model daily quotas. Two currencies get conflated constantly and now the docs say so in three places.

KNOWN_ISSUES.md § "No in-CLI quota visibility" has been open since v0.5 was a plan. Nice to close it properly.

Ships in the next release. Thanks again — genuinely good work.

@ffroliva ffroliva mentioned this pull request Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request from a non-maintainer or fork needs-copilot-review Ask GitHub Copilot for advisory code review needs-maintainer-review Requires maintainer review before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants