fix(agents): actionable copy for exhausted auth-profile failover#85798
Merged
Conversation
openclaw-barnacle
Bot
added
commands
Contributor
|
Codex review: needs changes before merge. Reviewed June 1, 2026, 1:48 PM ET / 17:48 UTC. Summary PR surface: Source +141, Tests +103, Docs +1. Total +245 across 6 files. Reproducibility: yes. Source inspection shows the current all-profiles-in-cooldown path reaches Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the centralized presenter and helper move, remove the Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows the current all-profiles-in-cooldown path reaches Is this the best way to solve the issue? Yes, with one cleanup. Formatting the message at the auth-profile failover source is a better owner-boundary fix than adding downstream string classifiers, and the command helper move avoids an agents-to-commands dependency; the changelog edit should be dropped. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against ee4802802843. Label changesLabel justifications:
Evidence reviewedPR surface: Source +141, Tests +103, Docs +1. Total +245 across 6 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
Contributor
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Pearl Proofling Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
clawsweeper
Bot
added
P3
altaywtf
self-requested a review
May 26, 2026 21:36
RomneyDa
marked this pull request as draft
May 27, 2026 20:02
The pi-embedded runner threw a generic "No available auth profile for <provider> (all in cooldown or unavailable)" message whenever every configured profile was in cooldown, even though the failover machinery had already resolved a concrete reason (auth, billing, rate_limit, session_expired, etc.). The user-facing copy never used that reason and never told the user how to recover. Route the resolved reason through a single presenter (`formatAuthProfileFailureMessage`) that composes a reason-specific sentence with `buildProviderAuthRecoveryHint`, so FailoverError.message ships with the right `openclaw models auth login --provider <id>` hint when the cause is authentication/session/billing, and falls back to the underlying provider error text otherwise. Helper moved out of `src/commands/` into `src/agents/` because `src/agents/` cannot depend on `src/commands/`.
RomneyDa
force-pushed
the
auth-profile-error-message
branch
from
June 1, 2026 17:40
86c577b to
927d171
Compare
RomneyDa
marked this pull request as ready for review
June 1, 2026 17:59
…-copy tests - Delete `src/commands/provider-auth-guidance.ts` and point doctor-auth, auth-choice.model-check, and models/list.status-command directly at `src/agents/provider-auth-recovery-hint.ts`. The cold-imports test moves with it. - Rewrite `failure-copy.test.ts` to assert behavior (recovery-hint dispatch, provider mention, cause-suffix dedup) instead of pinning exact long copy strings, so wording tweaks no longer require a test update in two places.
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Jun 2, 2026
…nclaw#85798) * fix(agents): actionable copy for exhausted auth-profile failover The pi-embedded runner threw a generic "No available auth profile for <provider> (all in cooldown or unavailable)" message whenever every configured profile was in cooldown, even though the failover machinery had already resolved a concrete reason (auth, billing, rate_limit, session_expired, etc.). The user-facing copy never used that reason and never told the user how to recover. Route the resolved reason through a single presenter (`formatAuthProfileFailureMessage`) that composes a reason-specific sentence with `buildProviderAuthRecoveryHint`, so FailoverError.message ships with the right `openclaw models auth login --provider <id>` hint when the cause is authentication/session/billing, and falls back to the underlying provider error text otherwise. Helper moved out of `src/commands/` into `src/agents/` because `src/agents/` cannot depend on `src/commands/`. * fix(agents): soften auth-profile failure copy for non-technical users * refactor(agents): drop guidance re-export shim and de-brittle failure-copy tests - Delete `src/commands/provider-auth-guidance.ts` and point doctor-auth, auth-choice.model-check, and models/list.status-command directly at `src/agents/provider-auth-recovery-hint.ts`. The cold-imports test moves with it. - Rewrite `failure-copy.test.ts` to assert behavior (recovery-hint dispatch, provider mention, cause-suffix dedup) instead of pinning exact long copy strings, so wording tweaks no longer require a test update in two places.
SYU8384
pushed a commit
to SYU8384/openclaw
that referenced
this pull request
Jun 3, 2026
…nclaw#85798) * fix(agents): actionable copy for exhausted auth-profile failover The pi-embedded runner threw a generic "No available auth profile for <provider> (all in cooldown or unavailable)" message whenever every configured profile was in cooldown, even though the failover machinery had already resolved a concrete reason (auth, billing, rate_limit, session_expired, etc.). The user-facing copy never used that reason and never told the user how to recover. Route the resolved reason through a single presenter (`formatAuthProfileFailureMessage`) that composes a reason-specific sentence with `buildProviderAuthRecoveryHint`, so FailoverError.message ships with the right `openclaw models auth login --provider <id>` hint when the cause is authentication/session/billing, and falls back to the underlying provider error text otherwise. Helper moved out of `src/commands/` into `src/agents/` because `src/agents/` cannot depend on `src/commands/`. * fix(agents): soften auth-profile failure copy for non-technical users * refactor(agents): drop guidance re-export shim and de-brittle failure-copy tests - Delete `src/commands/provider-auth-guidance.ts` and point doctor-auth, auth-choice.model-check, and models/list.status-command directly at `src/agents/provider-auth-recovery-hint.ts`. The cold-imports test moves with it. - Rewrite `failure-copy.test.ts` to assert behavior (recovery-hint dispatch, provider mention, cause-suffix dedup) instead of pinning exact long copy strings, so wording tweaks no longer require a test update in two places.
sablehead
pushed a commit
to sablehead/openclaw
that referenced
this pull request
Jun 10, 2026
…nclaw#85798) * fix(agents): actionable copy for exhausted auth-profile failover The pi-embedded runner threw a generic "No available auth profile for <provider> (all in cooldown or unavailable)" message whenever every configured profile was in cooldown, even though the failover machinery had already resolved a concrete reason (auth, billing, rate_limit, session_expired, etc.). The user-facing copy never used that reason and never told the user how to recover. Route the resolved reason through a single presenter (`formatAuthProfileFailureMessage`) that composes a reason-specific sentence with `buildProviderAuthRecoveryHint`, so FailoverError.message ships with the right `openclaw models auth login --provider <id>` hint when the cause is authentication/session/billing, and falls back to the underlying provider error text otherwise. Helper moved out of `src/commands/` into `src/agents/` because `src/agents/` cannot depend on `src/commands/`. * fix(agents): soften auth-profile failure copy for non-technical users * refactor(agents): drop guidance re-export shim and de-brittle failure-copy tests - Delete `src/commands/provider-auth-guidance.ts` and point doctor-auth, auth-choice.model-check, and models/list.status-command directly at `src/agents/provider-auth-recovery-hint.ts`. The cold-imports test moves with it. - Rewrite `failure-copy.test.ts` to assert behavior (recovery-hint dispatch, provider mention, cause-suffix dedup) instead of pinning exact long copy strings, so wording tweaks no longer require a test update in two places.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When every auth profile for a provider was in cooldown, the TUI surfaced this:
The message gave the user nothing to act on, even though the failover machinery had already resolved a concrete reason (
auth,billing,rate_limit,session_expired, …) and the repo already had a helper that knows how to render a provider-specificopenclaw models auth login --provider <id>recovery hint.This PR routes the resolved reason through a single presenter so the user-facing copy says, e.g.:
…and falls back to the original provider error text when no actionable reason maps.
Logic followed
The smell wasn't the wording — it was that the system already had structured failure data (
FailoverError.reason) but the user-facing layer was either ignoring it or, downstream, regex-matching the message string to try to recover it. Adding another regex would deepen that smell. So the architectural move is to invert: make structured data flow through, and treat strings as fallback, not as the source of truth.Concretely:
Single presenter for auth-profile failures. New
src/agents/auth-profiles/failure-copy.tsexportsformatAuthProfileFailureMessage({ reason, provider, allInCooldown, cause?, config?, workspaceDir?, env? }). Pure function — one place owns the copy for every "auth profiles exhausted / unusable" surface, trivially unit-testable.Use it at the source.
throwAuthProfileFailoverinsrc/agents/pi-embedded-runner/run/auth-controller.tsalready computedreasonviaresolveAuthProfileFailoverReasonand discarded it for the message. It now feedsreason(plus the underlying cause, if any) into the presenter, soFailoverError.messageships with good copy no matter who reads it (TUI, logs, CLI). ExistingFailoverError.reasonmetadata is unchanged.Cross-layer move.
buildProviderAuthRecoveryHintlived insrc/commands/but the new caller is insrc/agents/, and non-test code insrc/agents/must not import fromsrc/commands/. The helper (and its privateresolveProviderAuthLoginCommand) moved tosrc/agents/provider-auth-recovery-hint.ts.src/commands/provider-auth-guidance.tsbecomes a one-line re-export so the existing CLI callers (doctor-auth.ts,auth-choice.model-check.ts,models/list.status-command.ts) keep working without churn.Backward-safe fallback. When the resolved reason has no actionable copy (e.g.
unknown/empty_response) and acauseis present, the presenter returns the cause's text verbatim — so paths that previously produced decentgetApiKeyForModel-style errors are never regressed.What was deliberately not done
agent-runner-execution.ts(buildMissingApiKeyFailureText,classifyOAuthRefreshFailure) still run; they're now redundant for the auth-profile path but they correctly handle errors from third-party transports that arrive as opaque strings. Removing/typing them is a follow-up worth a separate PR.\"no_profile\"reason. TheprofileCandidates = [undefined]path (no profile configured at all) still surfacesgetApiKeyForModel's message. Unifying that path through the presenter is a sibling follow-up.FailoverErrorserialization. This PR fixes the message at the source, where the type is already in scope.Test plan
pnpm vitest run src/agents/auth-profiles/failure-copy.test.ts— new presenter unit tests (reason coverage, hint inclusion rules, cause passthrough, cause-dedup).pnpm vitest run src/agents/pi-embedded-runner/run/auth-controller.test.ts src/commands/provider-auth-guidance.test.ts— direct callers.pnpm vitest run src/commands/models/list.status.test.ts src/commands/auth-choice.model-check.test.ts src/commands/provider-setup-cold-imports.test.ts— all existing consumers of the moved helper.pnpm tsgo:core+pnpm tsgo:core:test— clean.npx oxlinton touched files — clean.openai-codexfailure with all profiles in cooldown and verify the TUI now shows the reason + login command.