fix(run): refresh OAuth session in local partner-credential injection (BE-3361)#552
fix(run): refresh OAuth session in local partner-credential injection (BE-3361)#552mattmillerai wants to merge 2 commits into
Conversation
… (BE-3361) The local partner-node credential injector called resolve_cloud_credential with refresh=False, so a signed-in OAuth-only user whose short-lived access token had lapsed was skipped here and deterministically hit partner_node_requires_credential on any local run — the 'intermittent while signed in' failure from dogfooding. Resolve with refresh=True so the lapsed token is refreshed, and add an allow_clear kwarg to resolve_cloud_credential (forwarded through get_session into ensure_fresh_session) so this best-effort path passes allow_clear=False: a fatal refresh error never clears the shared session, and a transient failure returns the stale session that fails its own expiry check and falls through to env/stored key exactly as before.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 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 (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
dosubot
Bot
added
size:M
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…BE-3361) Address the cursor-review panel findings on the refresh=False→True switch: - Skip _resolve_partner_credential() when an explicit --api-key already populated extra_data: the resolver can perform a synchronous OAuth refresh (network POST + refresh-lock wait), and an explicit key already satisfies the partner node — so an explicit-key run stays network-free. - Make the injector truly best-effort: ensure_fresh_session only swallows transient/timeout cases, so an unexpected OSError (acquiring the refresh lock or persisting the rotated token) previously propagated and aborted the run. Catch it and fall through to a network-free env/stored-key read, exactly the pre-BE-3361 behavior on this path. - Correct the docstring's leeway over-claim: a token inside the 30-60s window is returned as live rather than falling through. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
CI note: the red `build` and Windows `test` checks are the two known repo-wide infra breaks, not this PR:
All partner-cred / run tests pass locally (131 passed), ruff + format clean. Cursor-review threads addressed in 64748c6 and resolved. |
1 participant
ELI-5
When you run a workflow locally that uses a partner-API node (Veo/Kling/BFL/Gemini/…), the CLI has to hand the local server a credential so the node can authenticate. If you're signed in with OAuth (the normal way), it grabs your login token.
The catch: OAuth access tokens expire fast (that's by design). The local injector was reading your session without ever refreshing it, so once your short-lived token lapsed — which happens routinely between commands — it saw an "expired" session, skipped it, and you got a
partner_node_requires_credentialerror even though you were signed in. That was the "intermittent while signed in" failure from the 2026-07-16 dogfooding session.This makes the injector refresh the token when it can (spending the long-lived refresh token, exactly like the cloud path already does), so a signed-in user keeps working. And it does it in a way that can never log you out: if the refresh hits a fatal error, the shared session is left intact and the code just falls through to the env var / stored key like before.
What changed
comfy_cli/command/run/credentials.py—_resolve_partner_credential()now callsresolve_cloud_credential(purpose="cloud", refresh=True, allow_clear=False)instead ofrefresh=False. Module + function docstrings updated to document the new contract (refresh when possible, never clear, fall through on failure).comfy_cli/credentials.py—resolve_cloud_credential()gains anallow_clear: bool = Truekeyword, forwarded intoget_session(refresh=refresh, allow_clear=allow_clear)→ensure_fresh_session.get_sessionalready accepted/forwardedallow_clear, so no change was needed there.Why it's safe
allow_clear=Falsemeans a fatal refresh error (invalid_grant/ reuse-detection) returnsNonewithout clearing the stored session (oauth.pyalready implements this; verified by an existing oauth-layer test + a new end-to-end test here). The foreground command still owns the session lifecycle.ensure_fresh_sessionreturns the stale session on a transient refresh failure; it fails its own expiry check and the resolver falls through to env/stored key — identical to the pre-change behavior.comfy runfan-outs are safe — the cross-process refresh lock (oauth.py) coalesces concurrent refreshes so a rotated refresh token is never replayed.allow_cleardefaults toTrue, andget_sessionalready defaultedallow_clear=True, so the two existing callers (target.py,generate/client.py) keep identical behavior.where.py), with the addedallow_clear=Falsenuance appropriate for a best-effort injector.Tests
tests/comfy_cli/test_credentials.py— newTestResolveAllowClear(end-to-end against a real tmp secret store): expired session + valid refresh token → new access token returned; fatalOAuthRefreshErrorwithallow_clear=False→ session not cleared, falls through to env key; transient refresh failure → stale session kept, falls through; plus boundary checks thatallow_clearis forwarded and defaults toTrue.tests/comfy_cli/command/test_run.py—TestResolvePartnerCredentialupdated to the refresh contract: refreshes-and-uses the OAuth token, passesallow_clear=False, transient-stale falls through, env/stored precedence.test_no_direct_credential_reads_outside_resolver) still passes — no new directos.environ/ensure_fresh_sessionreads were added outsidecredentials.py.Full targeted suite:
tests/comfy_cli/test_credentials.py+tests/comfy_cli/command/test_run.py+tests/comfy_cli/auth/→ 130+ passed;ruff check/ruff format --checkclean.Not in scope
Per the ticket: the
partner_node_requires_credentialerror message/hint text (separate ticket) and any MCP-side changes (BE-3344).