fix(auth): skip OAuth refresh adapter when credential has no refresh token by RomneyDa · Pull Request #85028 · openclaw/openclaw

RomneyDa · 2026-05-21T16:42:33Z

Summary

OAuth credentials that loaded without their sidecar material (no access, no refresh) still entered the refresh path inside the per-profile lock. The adapter call there is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS = 120_000 (src/agents/auth-profiles/constants.ts:59), so the eventual No API key found for provider "..." surface to the user only landed after a long stall — even though the resolver had no usable refresh material to attempt with in the first place.

Short-circuit doRefreshOAuthTokenWithLock (src/agents/auth-profiles/oauth-manager.ts) to return null when there is no refresh token. The check is placed after the in-lock main-store adoption (oauth-manager.ts:455) and external bootstrap-credential checks (:498), so sub-agents inheriting fresh tokens from the main store and externally-managed credentials still resolve normally. Only the dead-end "nothing to refresh with" case short-circuits.

Companion to #84752 (root cause: re-enabling legacy sidecar rehydration) — this change is the fail-fast layer that improves the diagnostic surface when something else leaves a credential record token-less.

Verification

node scripts/run-vitest.mjs src/agents/auth-profiles/oauth-manager.test.ts — 38 passed.
node scripts/run-vitest.mjs src/agents/model-auth.test.ts src/agents/auth-profiles/store-oauth-sidecar.test.ts — 104 passed.
New test skips the refresh adapter when the credential has no refresh token asserts refreshCredential is not called and the resolver returns null.

Real behavior proof

Behavior addressed: No API key found for provider "openai-codex" surfacing only after several minutes of silent waiting on the bounded OAuth refresh adapter call when the stored credential has no usable refresh token.
Real environment tested: not exercised end-to-end on a live gateway — the change is gated behind the in-lock refresh adapter and has unit-test coverage that exercises the short-circuit. The matching slow-fail behavior is reproducible by editing an auth-profiles.json openai-codex entry to drop access/refresh, but that requires a real OAuth profile to start from.
Exact steps or command run after this patch: node scripts/run-vitest.mjs src/agents/auth-profiles/oauth-manager.test.ts src/agents/model-auth.test.ts src/agents/auth-profiles/store-oauth-sidecar.test.ts.
Evidence after fix: 142 tests passed across the touched files; new short-circuit case passes (refreshCredential not invoked, resolver returns null immediately).
Observed result after fix: the resolver returns null for credentials without refresh material without entering the bounded-timeout refresh adapter.
What was not tested: live gateway turn against a real broken openai-codex profile (no live OAuth profile available in this environment).

Test plan

Unit: new short-circuit test in oauth-manager.test.ts.
Regression: oauth-manager + model-auth + store-oauth-sidecar suites green.
Live: confirm that an openai-codex profile with missing access/refresh now fails fast on the user-perceived embedded-runner turn (left for someone with a reproducible live env).

@RomneyDa

…token OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

RomneyDa · 2026-05-21T16:42:47Z

Two related follow-ups intentionally not in this PR, kept separate to keep this one a one-line short-circuit:

Follow-up B — Surface per-candidate failure timing in the resolver

resolveApiKeyForProvider (src/agents/model-auth.ts:733) currently swallows per-candidate failures with log.debug?.(...). With multiple stale candidates the user has no way to see where time is going. Upgrade that to log.warn with the candidate id and a one-line error class so the slow-fail shape is visible without enabling debug logging.

Follow-up C — Per-candidate refresh budget for non-primary candidates

OAUTH_REFRESH_CALL_TIMEOUT_MS = 120_000 (src/agents/auth-profiles/constants.ts:59) is the hard ceiling per refresh. The intent is "let a legit slow refresh finish," which is reasonable for the primary credential. For non-primary fallback candidates inside the resolver loop, a much smaller per-attempt budget (e.g. 5–10s) would better match the "fast failover" intent — the alternative today is N candidates × 120s before the user sees an error. The shape of this would be an optional timeoutMs override on resolveOAuthAccess that the resolver loop passes for fallback candidates only.

Doing C is more invasive than B; do B first.

cc @RomneyDa

clawsweeper · 2026-05-21T16:44:08Z

Codex review: needs maintainer review before merge.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
The PR adds an OAuth-manager guard that returns null before invoking the refresh adapter when the selected OAuth credential has no refresh token, plus a regression test and changelog entry.

Reproducibility: yes. from source inspection: current main reaches adapter.refreshCredential under the 120s timeout even when the selected OAuth credential has no refresh material. I did not run a live gateway repro because this review is read-only and the PR body says a real OAuth profile would be needed.

PR rating
Overall: 🐚 platinum hermit
Proof: 🦐 gold shrimp
Patch quality: 🐚 platinum hermit
Summary: Small, source-aligned patch with targeted regression coverage and no blocking findings, with live runtime proof left as an optional maintainer confidence check.

Rank-up moves:

Have auth owners accept the token-less fail-fast contract, or run the live broken-profile gateway check if they want runtime confirmation before merge.

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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.

Real behavior proof
Not applicable: The external-contributor proof gate does not apply to this MEMBER/maintainer-labeled PR; the body provides unit-test evidence and explicitly notes no live gateway run.

Risk before merge

Merging changes an auth-provider failure path: any provider refresh adapter that could recover without a refresh token will no longer be called for that token-less record; the current OAuthCredential contract and bootstrap ordering make that look intentional, but owner acceptance is still needed.
The PR body reports targeted unit suites but no live gateway run against a broken openai-codex profile.

Maintainer options:

Accept the fail-fast OAuth contract (recommended)
Because OAuthCredential requires refresh material and the patch preserves main-store plus external bootstrap recovery first, maintainers can intentionally accept immediate null for token-less stored OAuth records.
Pause for live broken-profile proof
If maintainers want runtime confidence beyond unit coverage, reproduce with a real OAuth profile whose stored access and refresh material were removed and confirm the embedded turn reaches the missing-key error promptly.
Add adapter-contract coverage first
If any supported provider adapter is expected to refresh without credential.refresh, add explicit tests or an adapter contract note before landing this guard.

Next step before merge
No narrow automation repair is needed; this protected maintainer/auth-provider PR needs normal owner review and merge judgment.

Security
Cleared: The diff touches OAuth credential flow but does not add dependency, workflow, secret-storage, or logging behavior that exposes credentials; it only returns before invoking refresh without material.

Review details

Best possible solution:

Land the narrow guard if auth owners agree that token-less stored OAuth records should fail fast after main-store and bootstrap recovery opportunities, with the larger resolver timing diagnostics handled separately.

Do we have a high-confidence way to reproduce the issue?

Yes, from source inspection: current main reaches adapter.refreshCredential under the 120s timeout even when the selected OAuth credential has no refresh material. I did not run a live gateway repro because this review is read-only and the PR body says a real OAuth profile would be needed.

Is this the best way to solve the issue?

Yes, the proposed insertion point is the narrowest maintainable fix because it runs after main-store adoption and external bootstrap checks but before the bounded refresh adapter call. The broader resolver warning and fallback-budget ideas are correctly separated as follow-ups.

Label changes:

add P2: This is a focused auth-provider bug fix for a slow failure path with limited blast radius and targeted coverage.
add merge-risk: 🚨 auth-provider: The diff deliberately changes when OAuth refresh adapters are called for malformed token-less credentials, which can affect provider auth resolution behavior.
add rating: 🐚 platinum hermit: Current PR rating is 🐚 platinum hermit because proof is 🦐 gold shrimp, patch quality is 🐚 platinum hermit, and Small, source-aligned patch with targeted regression coverage and no blocking findings, with live runtime proof left as an optional maintainer confidence check.
add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply to this MEMBER/maintainer-labeled PR; the body provides unit-test evidence and explicitly notes no live gateway run.

Label justifications:

P2: This is a focused auth-provider bug fix for a slow failure path with limited blast radius and targeted coverage.
merge-risk: 🚨 auth-provider: The diff deliberately changes when OAuth refresh adapters are called for malformed token-less credentials, which can affect provider auth resolution behavior.
rating: 🐚 platinum hermit: Current PR rating is 🐚 platinum hermit because proof is 🦐 gold shrimp, patch quality is 🐚 platinum hermit, and Small, source-aligned patch with targeted regression coverage and no blocking findings, with live runtime proof left as an optional maintainer confidence check.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply to this MEMBER/maintainer-labeled PR; the body provides unit-test evidence and explicitly notes no live gateway run.

What I checked:

Current main refresh path: Current main enters the OAuth refresh adapter under the 120s timeout after main-store adoption and external bootstrap checks, with no refresh-token presence check before adapter.refreshCredential. (src/agents/auth-profiles/oauth-manager.ts:536, b33deb41594e)
Timeout contract: OAUTH_REFRESH_CALL_TIMEOUT_MS is 120000ms, matching the PR's claimed slow-fail ceiling for a single refresh attempt. (src/agents/auth-profiles/constants.ts:59, b33deb41594e)
OAuth credential shape: OAuthCredentials currently requires access, refresh, and expires strings/numbers, so a missing or blank refresh token is outside the normal usable refresh contract. (src/agents/auth-profiles/types.ts:6, b33deb41594e)
Secret normalization helper: normalizeSecretInputString treats non-strings and blank strings as missing, which matches the proposed guard's use for empty persisted refresh values. (src/config/types.secrets.ts:151, b33deb41594e)
PR diff: The branch adds the guard immediately before withRefreshCallTimeout and adds a regression test asserting refreshCredential is not called for an expired OAuth credential with blank access and refresh. (src/agents/auth-profiles/oauth-manager.ts:534, 544acf24413f)
Review and discussion state: GitHub shows no PR review comments, a maintainer label, and a maintainer comment explicitly splitting follow-up diagnostics and fallback timeout work out of this one-line short-circuit.

Likely related people:

Josh Avant: git blame in the available shallow checkout attributes the current OAuth manager and model-auth source lines around this refresh path to commit 40db92f. (role: recent area contributor / current implementation introducer; confidence: medium; commits: 40db92f6097c; files: src/agents/auth-profiles/oauth-manager.ts, src/agents/model-auth.ts)
@openclaw/openclaw-secops: CODEOWNERS explicitly owns src/agents auth and auth-profiles paths, and the PR timeline requested security-ops review. (role: CODEOWNERS reviewer team; confidence: high; files: .github/CODEOWNERS, src/agents/auth-profiles/oauth-manager.ts)
RomneyDa: The PR is maintainer-labeled, authored by a MEMBER, and the author supplied the scoped follow-up plan; GitHub commit lookup also shows prior merged OpenClaw work, though not clear source ownership of this exact auth path. (role: current maintainer contributor / likely follow-up owner; confidence: medium; commits: 544acf24413f, b79effefee92; files: src/agents/auth-profiles/oauth-manager.ts, src/agents/auth-profiles/oauth-manager.test.ts, CHANGELOG.md)

Codex review notes: model gpt-5.5, reasoning high; reviewed against b33deb41594e.

clawsweeper · 2026-05-21T16:49:37Z

ClawSweeper PR egg

✨ Hatched: 🥚 common Pearl Crabkin

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

Merged PRs are hatchable.
Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: polishes edge cases.
Image traits: location flaky test forest; accessory lint brush; palette violet, aqua, and starlight; mood watchful; pose leaning over a miniature review desk; shell smooth pearl shell; lighting gentle morning glow; background small green status lights.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Pearl Crabkin in ClawSweeper.

What is this egg doing here?

Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

@RomneyDa

…token (openclaw#85028) OAuth credentials that loaded without their sidecar material (no access, no refresh) would still enter the refresh path inside the per-profile lock, where the adapter call is bounded by OAUTH_REFRESH_CALL_TIMEOUT_MS (120s). That made the eventual "No API key found for provider" surface to the user only after a long stall, even though the resolver had no usable material to attempt with. Short-circuit doRefreshOAuthTokenWithLock to return null when there is no refresh token to use, after the in-lock main-store adoption and external bootstrap-credential checks have already had a chance to recover. Thanks @RomneyDa.

RomneyDa requested a review from a team as a code owner May 21, 2026 16:42

openclaw-barnacle Bot added agents Agent runtime and tooling size: XS maintainer Maintainer-authored PR labels May 21, 2026

RomneyDa merged commit 205c595 into main May 21, 2026
150 of 153 checks passed

RomneyDa deleted the fix-oauth-refresh-skip-when-no-token branch May 21, 2026 17:00

github-actions Bot mentioned this pull request May 21, 2026

📡 Upstream Digest — 2026-05-21 17:51 UTC curtismercier/openclaw-mods#909

Open

This was referenced May 21, 2026

fix(auth): load legacy Codex OAuth sidecars in embedded secrets-runtime loaders #85074

Merged

fix(auth): auto-migrate keychain-only legacy Codex OAuth profiles on first interactive CLI run #85163

Closed

RomneyDa mentioned this pull request May 24, 2026

fix(auth): point Keychain-only legacy Codex OAuth users at doctor instead of auto-prompting #86220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(auth): skip OAuth refresh adapter when credential has no refresh token#85028

fix(auth): skip OAuth refresh adapter when credential has no refresh token#85028
RomneyDa merged 1 commit into
mainfrom
fix-oauth-refresh-skip-when-no-token

RomneyDa commented May 21, 2026

Uh oh!

RomneyDa commented May 21, 2026

Uh oh!

clawsweeper Bot commented May 21, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

RomneyDa commented May 21, 2026

Summary

Verification

Real behavior proof

Test plan

Uh oh!

RomneyDa commented May 21, 2026

Follow-up B — Surface per-candidate failure timing in the resolver

Follow-up C — Per-candidate refresh budget for non-primary candidates

Uh oh!

clawsweeper Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clawsweeper Bot commented May 21, 2026

Hatch command

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented May 21, 2026 •

edited

Loading