Repository navigation
ci(pullfrog): retry the review with the fallback subscription token - #40
fabiodalez-dev wants to merge 4 commits into
Conversation
PULLFROG_OAUTH_FALLBACK can hold a second subscription token, but the workflow only ever passed the standard names — all unset here — so the agent's single credential was the one Pullfrog holds at account scope. That limit is per account and shared with interactive local use of the same subscription, and an afternoon of local work exhausts it: in the Pinakes repository every review from 16:08 onward failed with `429 ... would exceed your account's rate limit` followed by `no other model or provider was selected`, after two that had succeeded earlier. A per-account limit is not fixed by a second expression — the first credential has to be tried and seen to fail — so this is a second attempt, not an `||`. Written as `secrets.A || secrets.B` it would be worse than nothing: with no repository-level token set, that resolves to the spare one on every run and drains both quotas at once. Three details that are not obvious from the diff: - Only the presence flag is hoisted to job level, not the token. A step-level `if:` cannot read the `secrets` context at all — GitHub refuses to parse the whole workflow with `Unrecognized named-value: 'secrets'`, which I confirmed by dispatching a throwaway workflow rather than assuming. `env` is readable there, hence the flag. - The provider keys stay on the agent steps. Moving them to job level would put every token in the environment of the checkout and of the final `run:` too, which cuts against `persist-credentials: false` and `shell: restricted`. - The fallback step passes only the fallback token. If another provider were configured and usable, the first step would already have reached it, since Pullfrog walks its credentials in order. Degrades safely: with the secret unset, HAS_OAUTH_FALLBACK is false, the second step is skipped and behaviour is exactly as before. The final step restores the failure the first step's continue-on-error held back, so a review that genuinely cannot run still reports red instead of passing silently. This repository still needs the secret itself to be set for the fallback to do anything.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Aggiornamento: il secret è ora impostato su questo repository e il fallback è stato verificato in esecuzione, non solo nella forma. Run su
Nel log si vede il passaggio: il primo step chiude con Questo risolve anche il dubbio che avevo lasciato aperto: un run salvato dalla riserva riporta verde, non rosso. Il timore era che il primo step marcasse il check dell'action come fallito prima che Resta vera la distinzione |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No defects found in the workflow logic. Two open questions about how the Pullfrog action behaves at runtime are below. I could not settle them from this repository, so I am not approving.
Reviewed changes
This review covers the single commit that adds a second-attempt fallback to .github/workflows/pullfrog.yml.
- Presence flag at job level:
HAS_OAUTH_FALLBACKis set fromsecrets.PULLFROG_OAUTH_FALLBACK != ''. Only the boolean is hoisted, so a stepif:can read it without exposing the token. - Primary attempt made non-fatal:
Run agentgetsid: agentandcontinue-on-error: true. - Fallback step: a second run of the same pinned action receives only
CLAUDE_CODE_OAUTH_TOKENfrom the fallback secret. It runs only when the first attempt failed and the flag is'true'. - Failure restored: a final
run:step exits 1 when the primary attempt failed and the fallback did not succeed.
I traced the conditions and they behave as the PR description says:
continue-on-errorkeeps the job status at success, so the implicitsuccess()on the later steps holds, whilesteps.agent.outcomestaysfailure.- A skipped fallback has
outcome == 'skipped', so the final step fires. - A fallback that fails is not
continue-on-error. The job is then already red, so skipping the final step loses nothing. - The token never reaches the checkout step or the final
run:step.
ℹ️ Unclear whether the fallback token takes priority over Pullfrog's account-level credential
The fallback step still runs with id-token: write, so Pullfrog can mint its account-level credential there too. Nothing in this repository shows whether an env-provided CLAUDE_CODE_OAUTH_TOKEN is used before that credential. If the account-level credential still comes first, the fallback hits the same 429 and adds nothing. Your dispatch test could not cover this because the secret was absent.
Technical details
# Confirm that the fallback step actually uses PULLFROG_OAUTH_FALLBACK
## Affected sites
- .github/workflows/pullfrog.yml:150-159 — the fallback step sets only `CLAUDE_CODE_OAUTH_TOKEN`, while job-level `id-token: write` (line 45) still lets Pullfrog mint its account-level credential
- .github/workflows/pullfrog.yml:146-149 — the comment says "Pullfrog walks its credentials in order", but the order between an env OAuth token and the OIDC-minted account credential is not established
## Required outcome
- Evidence that, with `CLAUDE_CODE_OAUTH_TOKEN` set in env, the agent authenticates with that token and not the account-level one. Possible sources: a run log after the secret is added, the `pullfrog` npm package source, or docs.pullfrog.com.
## Open questions for the human
- After adding the secret, does the fallback run's log show the env OAuth token being used?ℹ️ The fallback runs after any failure of the first attempt, not only a rate limit
The fallback condition checks steps.agent.outcome == 'failure', which is true for any failure. A deterministic failure (a bad prompt, a tool or MCP error, an action bug) therefore re-runs the whole review and spends the second subscription's quota on a run that will fail the same way. It is also unverified whether a first attempt that fails late can leave side effects that the second run duplicates or contradicts. Examples would be a submitted review, a progress comment marked as errored, or a check run already concluded as failure.
Technical details
# Retry scope and first-attempt side effects
## Affected sites
- .github/workflows/pullfrog.yml:152 — `if: steps.agent.outcome == 'failure' && ...` does not distinguish a 429 from other failures
## Required outcome
- Either accept the trade-off and say so in the step comment, or narrow the retry to rate-limit failures if the action exposes an output or log signal that makes this possible.
- Confirm that a second run in the same job reuses the dispatch's progress comment and check run cleanly after the first run has failed, with no duplicate review and no stale red check left behind.
## Open questions for the human
- Is retrying on non-rate-limit failures acceptable given the shared quota on the fallback subscription?claude-opus-5-5 ｜ 𝕏
|
Correction to my review. The first open question, whether the fallback token takes priority over Pullfrog's account-level credential, is now answered from the Pullfrog source at the pinned SHA. The concern does not hold — please disregard it.
The same source reading added two caveats. Neither blocks the PR:
The second open question — the fallback also runs on failures unrelated to a rate limit — still stands. The source confirms it: 5xx and network errors in the pre-flight check count as usable, so a run that fails for those reasons still triggers the fallback step.
|
|
Follow-up on the second open question: what a failed first attempt leaves behind. I read the Pullfrog source at the pinned SHA. The short answer: retrying after a pre-flight failure is safe; retrying after the first attempt has already posted its review is not.
My suggestion: keep the retry, but note this narrow failure mode in the step comment. A duplicate review is a minor cost next to a run that never happens, and the action offers nothing that would let the workflow skip the retry once a review is already posted. None of this blocks merging.
|
There was a problem hiding this comment.
✅ No new issues found. Both open questions from the previous review are now settled.
Reviewed changes
This run covers the two commits pushed after the review of 7766cd0. Both commits add time limits and document the retry trade-offs.
- Bounded attempts: added
timeout: 20mto both agent steps andtimeout-minutes: 45to the job.timeoutis a declared input inaction.ymlat the pinned SHA0d318bef, with a default of1h. If the job limit is reached during the fallback, the job is cancelled and shows red, so this cannot become a green no-op. - Retry scope documented: the fallback step comment now says the retry fires on any failure. It also says a deterministic error can spend fallback quota and a late failure can repeat a review. This addresses the second question as an accepted, explicit trade-off.
- Credential order cited: added the claim that the workflow OAuth token is tried before account OAuth tokens, with a link to the source. I checked it against
utils/credentialPool.tsatf0684f2. Theoptionsarray lists workflow credentials before account candidates, and the stable subscription sort keeps that order. This addresses the first question.
One note, not a blocker: 20m replaces the 1h default for each attempt. A review that legitimately runs longer than 20 minutes will time out, retry, and probably fail red. The PR description says this is intended.
claude-opus-5-5 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The fallback step now has the same provider setup as the primary step, except for the OAuth token.
Reviewed changes
This run covers one commit, 16d0afc, pushed after the review of 8da49fe.
- Restored provider keys on the fallback step: added the same 12 provider keys the primary step has (
ANTHROPIC_API_KEY,OPENAI_API_KEY,GOOGLE_GENERATIVE_AI_API_KEY,GEMINI_API_KEY,XAI_API_KEY,DEEPSEEK_API_KEY,MOONSHOT_API_KEY,KIMI_API_KEY,META_MODEL_API_KEY,OPENROUTER_API_KEY,AI_GATEWAY_API_KEY,OPENCODE_API_KEY) in the same order. OnlyCLAUDE_CODE_OAUTH_TOKENdiffers, and it now comes fromPULLFROG_OAUTH_FALLBACK. I compared the two blocks line by line and the lists are identical. A run configured for a non-Claude model therefore works the same way on the retry. - Rewrote the step comment: removed the old reasoning ("the step above would already have reached it"). The new comment says only the OAuth token is replaced. The keys are still set only on the two agent steps. Checkout and the final
run:step get none of them, which matches the job-level comment at lines 38–39.
The 13-key block now appears in both agent steps. If one block changes, the other must be changed too. A YAML anchor cannot remove the duplication, because GitHub Actions does not support merge keys (<<:) and the token has to be overridden. Moving the keys to job level would put them in the checkout step's environment, which this PR avoids on purpose. The duplication is the right trade-off.
claude-opus-5-5 ｜ 𝕏
This branch has not been deployed
1 participant
When the primary review attempt fails, Pullfrog now makes one additional attempt using
PULLFROG_OAUTH_FALLBACK, if configured. A review that cannot run still fails the check. Provider keys remain scoped to the two agent steps. The fallback retains the same provider keys and replaces only the OAuth token; job-level environment contains only its presence flag.The runtime 0.1.97 credential selector tries the workflow OAuth token before account OAuth tokens of the same kind; its stable subscription sort retains that order. This is verified in the published npm package and its matching credentialPool.ts source. API keys are not claimed to outrank subscriptions. Read-only repository permissions, disabled push, restricted shell and checkout without persisted credentials are retained.
The action exposes
result, not a structured failure reason, so the retry applies to any primary failure rather than only 429s. Both attempts are capped at 20 minutes and the job at 45 minutes. A deterministic error may spend fallback quota, and an error after posting may duplicate a review; the workflow comment makes this bounded trade-off explicit. A clean replay after late side effects is not claimed.Validation: actionlint and diff whitespace checks pass. The previously quota-failed review was rerun. All 3 checks pass on final head
16d0afc7102f8f69f9de1386144814048681c214; the latest Pullfrog review reports no new issues. The fallback secret is present (name/presence verified only, no secret value read). This infrastructure PR stays separate from the application changes and takes effect when merged into the default branch.CodeRabbit automatic review is skipped for this Android repository due to its stars threshold.