diff --git a/docs/.generated/plugin-sdk-api-baseline.sha256 b/docs/.generated/plugin-sdk-api-baseline.sha256 index 753f3ebaff2..4ce694d7c38 100644 --- a/docs/.generated/plugin-sdk-api-baseline.sha256 +++ b/docs/.generated/plugin-sdk-api-baseline.sha256 @@ -1,2 +1,2 @@ -e5c3d6eb56304164434a8b29670a6f02490f359eb7a5f3f4034e61a1b8421c54 plugin-sdk-api-baseline.json -14d07997a35902bbd3c94d528aafa53185bc66a0ddd9b518f87c85a352feb476 plugin-sdk-api-baseline.jsonl +d08f0e793e66192fdbc377183ce0d94adcbec53cf334522bce8c0c457b90b0a8 plugin-sdk-api-baseline.json +924f20b350a9f1997e95b3d7249cbb6720c9576c63e6c0c15cca0164734fd93d plugin-sdk-api-baseline.jsonl diff --git a/extensions/openai/openai-codex-cli-auth.test.ts b/extensions/openai/openai-codex-cli-auth.test.ts index 267b3202f0d..36e9d40ae7f 100644 --- a/extensions/openai/openai-codex-cli-auth.test.ts +++ b/extensions/openai/openai-codex-cli-auth.test.ts @@ -87,7 +87,116 @@ describe("readOpenAICodexCliOAuthProfile", () => { provider: "openai-codex", access: "local-access", refresh: "local-refresh", + expires: Date.now() + 10 * 60_000, + }, + }, + }, + }); + + expect(parsed).toBeNull(); + }); + + it("does not override explicit local non-oauth auth with Codex CLI bootstrap", () => { + vi.spyOn(fs, "readFileSync").mockReturnValue( + JSON.stringify({ + auth_mode: "chatgpt", + tokens: { + access_token: "access-token", + refresh_token: "refresh-token", + }, + }), + ); + + const parsed = readOpenAICodexCliOAuthProfile({ + store: { + version: 1, + profiles: { + [OPENAI_CODEX_DEFAULT_PROFILE_ID]: { + type: "api_key", + provider: "openai-codex", + key: "sk-local", + }, + }, + }, + }); + + expect(parsed).toBeNull(); + }); + + it("allows Codex CLI bootstrap when the stored default is expired", () => { + const accessToken = buildJwt({ + exp: Math.floor(Date.now() / 1000) + 600, + "https://api.openai.com/profile": { + email: "codex@example.com", + }, + }); + vi.spyOn(fs, "readFileSync").mockReturnValue( + JSON.stringify({ + auth_mode: "chatgpt", + tokens: { + access_token: accessToken, + refresh_token: "refresh-token", + account_id: "acct_123", + }, + }), + ); + + const parsed = readOpenAICodexCliOAuthProfile({ + store: { + version: 1, + profiles: { + [OPENAI_CODEX_DEFAULT_PROFILE_ID]: { + type: "oauth", + provider: "openai-codex", + access: "expired-local-access", + refresh: "expired-local-refresh", + expires: Date.now() - 60_000, + }, + }, + }, + }); + + expect(parsed).toMatchObject({ + profileId: OPENAI_CODEX_DEFAULT_PROFILE_ID, + credential: { + access: accessToken, + refresh: "refresh-token", + accountId: "acct_123", + email: "codex@example.com", + }, + }); + }); + + it("refuses Codex CLI bootstrap when an expired local default belongs to a different account", () => { + const accessToken = buildJwt({ + exp: Math.floor(Date.now() / 1000) + 600, + "https://api.openai.com/profile": { + email: "codex-b@example.com", + }, + }); + vi.spyOn(fs, "readFileSync").mockReturnValue( + JSON.stringify({ + auth_mode: "chatgpt", + tokens: { + access_token: accessToken, + refresh_token: "refresh-token", + account_id: "acct_b", + }, + }), + ); + + const parsed = readOpenAICodexCliOAuthProfile({ + store: { + version: 1, + profiles: { + [OPENAI_CODEX_DEFAULT_PROFILE_ID]: { + type: "oauth", + provider: "openai-codex", + access: "near-expiry-local-access", + refresh: "near-expiry-local-refresh", expires: Date.now() + 60_000, + accountId: "acct_a", + email: "codex-a@example.com", }, }, }, diff --git a/extensions/openai/openai-codex-cli-auth.ts b/extensions/openai/openai-codex-cli-auth.ts index 63c5fd111e5..9b24a1cf471 100644 --- a/extensions/openai/openai-codex-cli-auth.ts +++ b/extensions/openai/openai-codex-cli-auth.ts @@ -1,7 +1,11 @@ import fs from "node:fs"; import path from "node:path"; -import type { AuthProfileStore, OAuthCredential } from "openclaw/plugin-sdk/provider-auth"; -import { resolveRequiredHomeDir } from "openclaw/plugin-sdk/provider-auth"; +import { + hasUsableOAuthCredential, + resolveRequiredHomeDir, + type AuthProfileStore, + type OAuthCredential, +} from "openclaw/plugin-sdk/provider-auth"; import { createSubsystemLogger } from "openclaw/plugin-sdk/runtime-env"; import { resolveCodexAccessTokenExpiry, @@ -76,6 +80,40 @@ function oauthCredentialMatches(a: OAuthCredential, b: OAuthCredential): boolean ); } +function normalizeAuthIdentityToken(value: string | undefined): string | undefined { + const trimmed = value?.trim(); + return trimmed ? trimmed : undefined; +} + +function normalizeAuthEmailToken(value: string | undefined): string | undefined { + return normalizeAuthIdentityToken(value)?.toLowerCase(); +} + +// Keep this overwrite guard aligned with the canonical OAuth identity-copy rule +// in src/agents/auth-profiles/oauth.ts without widening the plugin SDK surface. +function isSafeToReplaceStoredIdentity( + existing: Pick, + incoming: Pick, +): boolean { + const existingAccountId = normalizeAuthIdentityToken(existing.accountId); + const incomingAccountId = normalizeAuthIdentityToken(incoming.accountId); + const existingEmail = normalizeAuthEmailToken(existing.email); + const incomingEmail = normalizeAuthEmailToken(incoming.email); + + if (existingAccountId !== undefined && incomingAccountId !== undefined) { + return existingAccountId === incomingAccountId; + } + if (existingEmail !== undefined && incomingEmail !== undefined) { + return existingEmail === incomingEmail; + } + + const existingHasIdentity = existingAccountId !== undefined || existingEmail !== undefined; + if (existingHasIdentity) { + return false; + } + return true; +} + export function readOpenAICodexCliOAuthProfile(params: { env?: NodeJS.ProcessEnv; store: AuthProfileStore; @@ -104,7 +142,28 @@ export function readOpenAICodexCliOAuthProfile(params: { ...(identity.profileName ? { displayName: identity.profileName } : {}), }; const existing = params.store.profiles[OPENAI_CODEX_DEFAULT_PROFILE_ID]; - if (existing && (existing.type !== "oauth" || !oauthCredentialMatches(existing, credential))) { + const existingOAuth = + existing?.type === "oauth" && existing.provider === PROVIDER_ID ? existing : undefined; + if (existing && !existingOAuth) { + log.debug("kept explicit local auth over Codex CLI bootstrap", { + profileId: OPENAI_CODEX_DEFAULT_PROFILE_ID, + localType: existing.type, + localProvider: existing.provider, + }); + return null; + } + if ( + existingOAuth && + hasUsableOAuthCredential(existingOAuth) && + !oauthCredentialMatches(existingOAuth, credential) + ) { + return null; + } + if ( + existingOAuth && + !oauthCredentialMatches(existingOAuth, credential) && + !isSafeToReplaceStoredIdentity(existingOAuth, credential) + ) { return null; } diff --git a/src/agents/auth-health.test.ts b/src/agents/auth-health.test.ts index 076580ff433..c40d7f9cf6e 100644 --- a/src/agents/auth-health.test.ts +++ b/src/agents/auth-health.test.ts @@ -173,6 +173,63 @@ describe("buildAuthHealthSummary", () => { expect(profile?.expiresAt).toBe(now + DEFAULT_OAUTH_WARN_MS + 10_000); }); + it("marks oauth as expiring when it falls within the shared refresh margin", () => { + vi.spyOn(Date, "now").mockReturnValue(now); + const store = { + version: 1, + profiles: { + "openai-codex:default": { + type: "oauth" as const, + provider: "openai-codex", + access: "near-expiry-access", + refresh: "near-expiry-refresh", + expires: now + 2 * 60_000, + }, + }, + }; + + const summary = buildAuthHealthSummary({ + store, + warnAfterMs: 60_000, + }); + + const profile = summary.profiles.find((entry) => entry.profileId === "openai-codex:default"); + expect(profile?.status).toBe("expiring"); + }); + + it("prefers fresher imported external OAuth when the local credential is near expiry", () => { + vi.spyOn(Date, "now").mockReturnValue(now); + readCodexCliCredentialsCachedMock.mockReturnValue({ + type: "oauth", + provider: "openai-codex", + access: "fresh-cli-access", + refresh: "fresh-cli-refresh", + expires: now + DEFAULT_OAUTH_WARN_MS + 60_000, + accountId: "acct-cli", + }); + const store = { + version: 1, + profiles: { + "openai-codex:default": { + type: "oauth" as const, + provider: "openai-codex", + access: "near-expiry-local-access", + refresh: "near-expiry-local-refresh", + expires: now + 2 * 60_000, + }, + }, + }; + + const summary = buildAuthHealthSummary({ + store, + warnAfterMs: 60_000, + }); + + const profile = summary.profiles.find((entry) => entry.profileId === "openai-codex:default"); + expect(profile?.status).toBe("ok"); + expect(profile?.expiresAt).toBe(now + DEFAULT_OAUTH_WARN_MS + 60_000); + }); + it("marks token profiles with invalid expires as missing with reason code", () => { vi.spyOn(Date, "now").mockReturnValue(now); const store = { diff --git a/src/agents/auth-health.ts b/src/agents/auth-health.ts index ef3217f6444..da68d8c75e6 100644 --- a/src/agents/auth-health.ts +++ b/src/agents/auth-health.ts @@ -1,5 +1,6 @@ import type { OpenClawConfig } from "../config/types.openclaw.js"; import { + DEFAULT_OAUTH_REFRESH_MARGIN_MS, type AuthCredentialReasonCode, evaluateStoredCredentialEligibility, resolveTokenExpiryState, @@ -79,16 +80,22 @@ export function formatRemainingShort( function resolveOAuthStatus( expiresAt: number | undefined, now: number, - warnAfterMs: number, + expiringWithinMs: number, ): { status: AuthProfileHealthStatus; remainingMs?: number } { if (!expiresAt || !Number.isFinite(expiresAt) || expiresAt <= 0) { return { status: "missing" }; } const remainingMs = expiresAt - now; - if (remainingMs <= 0) { + const expiryState = resolveTokenExpiryState(expiresAt, now, { + expiringWithinMs, + }); + if (expiryState === "invalid_expires" || expiryState === "missing") { + return { status: "missing" }; + } + if (expiryState === "expired") { return { status: "expired", remainingMs }; } - if (remainingMs <= warnAfterMs) { + if (expiryState === "expiring") { return { status: "expiring", remainingMs }; } return { status: "ok", remainingMs }; @@ -166,10 +173,11 @@ function buildProfileHealth(params: { profileId, credential, }); + const oauthWarnAfterMs = Math.max(warnAfterMs, DEFAULT_OAUTH_REFRESH_MARGIN_MS); const { status: rawStatus, remainingMs } = resolveOAuthStatus( effectiveCredential.expires, now, - warnAfterMs, + oauthWarnAfterMs, ); return { profileId, diff --git a/src/agents/auth-profiles.external-cli-sync.test.ts b/src/agents/auth-profiles.external-cli-sync.test.ts index 2c58ebf561a..4b46f002c8e 100644 --- a/src/agents/auth-profiles.external-cli-sync.test.ts +++ b/src/agents/auth-profiles.external-cli-sync.test.ts @@ -22,7 +22,7 @@ function makeOAuthCredential( provider: overrides.provider, access: overrides.access ?? `${overrides.provider}-access`, refresh: overrides.refresh ?? `${overrides.provider}-refresh`, - expires: overrides.expires ?? Date.now() + 60_000, + expires: overrides.expires ?? Date.now() + 10 * 60_000, accountId: overrides.accountId, email: overrides.email, enterpriseUrl: overrides.enterpriseUrl, @@ -109,13 +109,13 @@ describe("external cli oauth resolution", () => { }); describe("external cli bootstrap policy", () => { - it("treats only non-expired access tokens as usable local oauth", () => { + it("treats only non-expired and non-near-expiry access tokens as usable local oauth", () => { expect( hasUsableOAuthCredential( makeOAuthCredential({ provider: "openai-codex", access: "live-access", - expires: Date.now() + 60_000, + expires: Date.now() + 10 * 60_000, }), ), ).toBe(true); @@ -128,6 +128,15 @@ describe("external cli oauth resolution", () => { }), ), ).toBe(false); + expect( + hasUsableOAuthCredential( + makeOAuthCredential({ + provider: "openai-codex", + access: "near-expiry-access", + expires: Date.now() + 60_000, + }), + ), + ).toBe(false); expect( hasUsableOAuthCredential( makeOAuthCredential({ @@ -153,7 +162,7 @@ describe("external cli oauth resolution", () => { provider: "openai-codex", access: "healthy-local-access", refresh: "healthy-local-refresh", - expires: Date.now() + 60_000, + expires: Date.now() + 10 * 60_000, }), imported, }), @@ -169,6 +178,40 @@ describe("external cli oauth resolution", () => { imported, }), ).toBe(true); + expect( + shouldBootstrapFromExternalCliCredential({ + existing: makeOAuthCredential({ + provider: "openai-codex", + access: "near-expiry-local-access", + refresh: "near-expiry-local-refresh", + expires: Date.now() + 60_000, + }), + imported, + }), + ).toBe(true); + }); + + it("does not bootstrap across different known oauth identities", () => { + const imported = makeOAuthCredential({ + provider: "openai-codex", + access: "fresh-cli-access", + refresh: "fresh-cli-refresh", + expires: Date.now() + 5 * 24 * 60 * 60_000, + accountId: "acct-external", + }); + + expect( + shouldBootstrapFromExternalCliCredential({ + existing: makeOAuthCredential({ + provider: "openai-codex", + access: "expired-local-access", + refresh: "expired-local-refresh", + expires: Date.now() - 60_000, + accountId: "acct-local", + }), + imported, + }), + ).toBe(false); }); }); @@ -296,7 +339,7 @@ describe("external cli oauth resolution", () => { provider: "openai-codex", access: "healthy-local-access", refresh: "healthy-local-refresh", - expires: Date.now() + 60_000, + expires: Date.now() + 10 * 60_000, }), ), ); diff --git a/src/agents/auth-profiles/constants.ts b/src/agents/auth-profiles/constants.ts index f790f81eefb..9e0ba5207f8 100644 --- a/src/agents/auth-profiles/constants.ts +++ b/src/agents/auth-profiles/constants.ts @@ -33,9 +33,13 @@ export const AUTH_STORE_LOCK_OPTIONS = { // so a legitimate refresh's critical section always finishes well before // peers would treat the lock as reclaimable. Violating this invariant re- // introduces the `refresh_token_reused` race the lock is meant to prevent. +// +// Retry budget note: keep the MINIMUM cumulative retry window comfortably +// above OAUTH_REFRESH_CALL_TIMEOUT_MS so waiters do not give up while a +// legitimate slow refresh is still within its allowed runtime budget. export const OAUTH_REFRESH_LOCK_OPTIONS = { retries: { - retries: 10, + retries: 20, factor: 2, minTimeout: 100, maxTimeout: 10_000, diff --git a/src/agents/auth-profiles/credential-state.test.ts b/src/agents/auth-profiles/credential-state.test.ts index 443519e5b0c..936a7bbe64b 100644 --- a/src/agents/auth-profiles/credential-state.test.ts +++ b/src/agents/auth-profiles/credential-state.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it } from "vitest"; import { + DEFAULT_OAUTH_REFRESH_MARGIN_MS, evaluateStoredCredentialEligibility, + hasUsableOAuthCredential, resolveTokenExpiryState, } from "./credential-state.js"; @@ -25,6 +27,33 @@ describe("resolveTokenExpiryState", () => { it("returns valid when expires is in the future", () => { expect(resolveTokenExpiryState(now + 1, now)).toBe("valid"); }); + + it("returns expiring when expires falls within the configured margin", () => { + expect( + resolveTokenExpiryState(now + DEFAULT_OAUTH_REFRESH_MARGIN_MS - 1, now, { + expiringWithinMs: DEFAULT_OAUTH_REFRESH_MARGIN_MS, + }), + ).toBe("expiring"); + }); +}); + +describe("hasUsableOAuthCredential", () => { + const now = 1_700_000_000_000; + + it("treats near-expiry oauth credentials as no longer usable", () => { + expect( + hasUsableOAuthCredential( + { + type: "oauth", + provider: "openai-codex", + access: "access-token", + refresh: "refresh-token", + expires: now + DEFAULT_OAUTH_REFRESH_MARGIN_MS - 1, + }, + { now }, + ), + ).toBe(false); + }); }); describe("evaluateStoredCredentialEligibility", () => { diff --git a/src/agents/auth-profiles/credential-state.ts b/src/agents/auth-profiles/credential-state.ts index 9b2afcdfe2e..4aeb8c7e6ed 100644 --- a/src/agents/auth-profiles/credential-state.ts +++ b/src/agents/auth-profiles/credential-state.ts @@ -1,5 +1,5 @@ import { coerceSecretRef, normalizeSecretInputString } from "../../config/types.secrets.js"; -import type { AuthProfileCredential } from "./types.js"; +import type { AuthProfileCredential, OAuthCredential } from "./types.js"; export type AuthCredentialReasonCode = | "ok" @@ -8,9 +8,17 @@ export type AuthCredentialReasonCode = | "expired" | "unresolved_ref"; -export type TokenExpiryState = "missing" | "valid" | "expired" | "invalid_expires"; +export const DEFAULT_OAUTH_REFRESH_MARGIN_MS = 5 * 60 * 1000; -export function resolveTokenExpiryState(expires: unknown, now = Date.now()): TokenExpiryState { +export type TokenExpiryState = "missing" | "valid" | "expiring" | "expired" | "invalid_expires"; + +export function resolveTokenExpiryState( + expires: unknown, + now = Date.now(), + opts?: { + expiringWithinMs?: number; + }, +): TokenExpiryState { if (expires === undefined) { return "missing"; } @@ -20,7 +28,37 @@ export function resolveTokenExpiryState(expires: unknown, now = Date.now()): Tok if (!Number.isFinite(expires) || expires <= 0) { return "invalid_expires"; } - return now >= expires ? "expired" : "valid"; + const remainingMs = expires - now; + if (remainingMs <= 0) { + return "expired"; + } + const expiringWithinMs = Math.max(0, opts?.expiringWithinMs ?? 0); + if (expiringWithinMs > 0 && remainingMs <= expiringWithinMs) { + return "expiring"; + } + return "valid"; +} + +export function hasUsableOAuthCredential( + credential: OAuthCredential | undefined, + opts?: { + now?: number; + refreshMarginMs?: number; + }, +): boolean { + if (!credential || credential.type !== "oauth") { + return false; + } + if (typeof credential.access !== "string" || credential.access.trim().length === 0) { + return false; + } + const now = opts?.now ?? Date.now(); + const refreshMarginMs = Math.max(0, opts?.refreshMarginMs ?? DEFAULT_OAUTH_REFRESH_MARGIN_MS); + return ( + resolveTokenExpiryState(credential.expires, now, { + expiringWithinMs: refreshMarginMs, + }) === "valid" + ); } function hasConfiguredSecretRef(value: unknown): boolean { diff --git a/src/agents/auth-profiles/effective-oauth.ts b/src/agents/auth-profiles/effective-oauth.ts index 0624c1908c2..3522dee878c 100644 --- a/src/agents/auth-profiles/effective-oauth.ts +++ b/src/agents/auth-profiles/effective-oauth.ts @@ -1,6 +1,7 @@ import { log } from "./constants.js"; import { hasUsableOAuthCredential, + isSafeToUseExternalCliCredential, readExternalCliBootstrapCredential, shouldBootstrapFromExternalCliCredential, } from "./external-cli-sync.js"; @@ -26,6 +27,13 @@ export function resolveEffectiveOAuthCredential(params: { }); return params.credential; } + if (!isSafeToUseExternalCliCredential(params.credential, imported)) { + log.warn("refused external cli oauth bootstrap: identity mismatch", { + profileId: params.profileId, + provider: params.credential.provider, + }); + return params.credential; + } const shouldBootstrap = shouldBootstrapFromExternalCliCredential({ existing: params.credential, imported, diff --git a/src/agents/auth-profiles/external-cli-sync.ts b/src/agents/auth-profiles/external-cli-sync.ts index 226c32bcd1a..594ec4fc856 100644 --- a/src/agents/auth-profiles/external-cli-sync.ts +++ b/src/agents/auth-profiles/external-cli-sync.ts @@ -8,7 +8,7 @@ import { OPENAI_CODEX_DEFAULT_PROFILE_ID, } from "./constants.js"; import { log } from "./constants.js"; -import { resolveTokenExpiryState } from "./credential-state.js"; +import { hasUsableOAuthCredential as hasUsableOAuthCredentialShared } from "./credential-state.js"; import type { AuthProfileStore, OAuthCredential } from "./types.js"; export type ExternalCliResolvedProfile = { @@ -41,6 +41,46 @@ export function areOAuthCredentialsEquivalent( ); } +function normalizeAuthIdentityToken(value: string | undefined): string | undefined { + const trimmed = value?.trim(); + return trimmed ? trimmed : undefined; +} + +function normalizeAuthEmailToken(value: string | undefined): string | undefined { + return normalizeAuthIdentityToken(value)?.toLowerCase(); +} + +// Keep this gate aligned with the canonical identity-copy rule in oauth.ts. +export function isSafeToUseExternalCliCredential( + existing: OAuthCredential | undefined, + imported: OAuthCredential, +): boolean { + if (!existing) { + return true; + } + if (existing.provider !== imported.provider) { + return false; + } + + const existingAccountId = normalizeAuthIdentityToken(existing.accountId); + const importedAccountId = normalizeAuthIdentityToken(imported.accountId); + const existingEmail = normalizeAuthEmailToken(existing.email); + const importedEmail = normalizeAuthEmailToken(imported.email); + + if (existingAccountId !== undefined && importedAccountId !== undefined) { + return existingAccountId === importedAccountId; + } + if (existingEmail !== undefined && importedEmail !== undefined) { + return existingEmail === importedEmail; + } + + const existingHasIdentity = existingAccountId !== undefined || existingEmail !== undefined; + if (existingHasIdentity) { + return false; + } + return true; +} + function hasNewerStoredOAuthCredential( existing: OAuthCredential | undefined, incoming: OAuthCredential, @@ -70,13 +110,7 @@ export function hasUsableOAuthCredential( credential: OAuthCredential | undefined, now = Date.now(), ): boolean { - if (!credential || credential.type !== "oauth") { - return false; - } - if (typeof credential.access !== "string" || credential.access.trim().length === 0) { - return false; - } - return resolveTokenExpiryState(credential.expires, now) === "valid"; + return hasUsableOAuthCredentialShared(credential, { now }); } export function shouldBootstrapFromExternalCliCredential(params: { @@ -85,6 +119,9 @@ export function shouldBootstrapFromExternalCliCredential(params: { now?: number; }): boolean { const now = params.now ?? Date.now(); + if (!isSafeToUseExternalCliCredential(params.existing, params.imported)) { + return false; + } if (hasUsableOAuthCredential(params.existing, now)) { return false; } @@ -144,7 +181,26 @@ export function resolveExternalCliAuthProfiles( continue; } const existing = store.profiles[providerConfig.profileId]; - const existingOAuth = existing?.type === "oauth" ? existing : undefined; + const existingOAuth = + existing?.type === "oauth" && existing.provider === providerConfig.provider + ? existing + : undefined; + if (existing && !existingOAuth) { + log.debug("kept explicit local auth over external cli bootstrap", { + profileId: providerConfig.profileId, + provider: providerConfig.provider, + localType: existing.type, + localProvider: existing.provider, + }); + continue; + } + if (existingOAuth && !isSafeToUseExternalCliCredential(existingOAuth, creds)) { + log.warn("refused external cli oauth bootstrap: identity mismatch", { + profileId: providerConfig.profileId, + provider: providerConfig.provider, + }); + continue; + } if ( !shouldBootstrapFromExternalCliCredential({ existing: existingOAuth, diff --git a/src/agents/auth-profiles/external-oauth.test.ts b/src/agents/auth-profiles/external-oauth.test.ts index 3f907f376a5..af4de0d251b 100644 --- a/src/agents/auth-profiles/external-oauth.test.ts +++ b/src/agents/auth-profiles/external-oauth.test.ts @@ -34,6 +34,11 @@ function createCredential(overrides: Partial = {}): OAuthCreden }; } +function createUsableOAuthExpiry(): number { + // Keep fixtures comfortably outside the shared near-expiry refresh margin. + return Date.now() + 30 * 60 * 1000; +} + describe("auth external oauth helpers", () => { beforeEach(() => { resolveExternalAuthProfilesWithPluginsMock.mockReset(); @@ -124,7 +129,7 @@ describe("auth external oauth helpers", () => { createCredential({ access: "fresh-cli-access-token", refresh: "fresh-cli-refresh-token", - expires: Date.now() + 60_000, + expires: createUsableOAuthExpiry(), }), ); @@ -159,7 +164,7 @@ describe("auth external oauth helpers", () => { "openai-codex:default": createCredential({ access: "healthy-local-access-token", refresh: "healthy-local-refresh-token", - expires: Date.now() + 60_000, + expires: createUsableOAuthExpiry(), }), }), ); @@ -169,4 +174,58 @@ describe("auth external oauth helpers", () => { refresh: "healthy-local-refresh-token", }); }); + + it("keeps explicit local non-oauth auth over external cli oauth overlays", () => { + readCodexCliCredentialsCachedMock.mockReturnValue( + createCredential({ + access: "fresh-cli-access-token", + refresh: "fresh-cli-refresh-token", + expires: Date.now() + 5 * 24 * 60 * 60_000, + }), + ); + + const overlaid = overlayExternalOAuthProfiles( + createStore({ + "openai-codex:default": { + type: "api_key", + provider: "openai-codex", + key: "sk-local", + }, + }), + ); + + expect(overlaid.profiles["openai-codex:default"]).toMatchObject({ + type: "api_key", + provider: "openai-codex", + key: "sk-local", + }); + }); + + it("keeps expired local oauth when external cli belongs to a different account", () => { + readCodexCliCredentialsCachedMock.mockReturnValue( + createCredential({ + access: "fresh-cli-access-token", + refresh: "fresh-cli-refresh-token", + expires: createUsableOAuthExpiry(), + accountId: "acct-external", + }), + ); + + const overlaid = overlayExternalOAuthProfiles( + createStore({ + "openai-codex:default": createCredential({ + access: "expired-local-access-token", + refresh: "expired-local-refresh-token", + expires: Date.now() - 60_000, + accountId: "acct-local", + }), + }), + ); + + expect(overlaid.profiles["openai-codex:default"]).toMatchObject({ + access: "expired-local-access-token", + refresh: "expired-local-refresh-token", + accountId: "acct-local", + }); + }); }); diff --git a/src/agents/auth-profiles/oauth-lock-timeout-classification.test.ts b/src/agents/auth-profiles/oauth-lock-timeout-classification.test.ts new file mode 100644 index 00000000000..8862b2e27e9 --- /dev/null +++ b/src/agents/auth-profiles/oauth-lock-timeout-classification.test.ts @@ -0,0 +1,208 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { FILE_LOCK_TIMEOUT_ERROR_CODE, type FileLockTimeoutError } from "../../infra/file-lock.js"; +import { captureEnv } from "../../test-utils/env.js"; +import { resolveAuthStorePath, resolveOAuthRefreshLockPath } from "./paths.js"; +import { clearRuntimeAuthProfileStoreSnapshots, saveAuthProfileStore } from "./store.js"; +import type { AuthProfileStore, OAuthCredential } from "./types.js"; + +let resolveApiKeyForProfile: typeof import("./oauth.js").resolveApiKeyForProfile; +let resetOAuthRefreshQueuesForTest: typeof import("./oauth.js").resetOAuthRefreshQueuesForTest; + +async function loadOAuthModuleForTest() { + ({ resolveApiKeyForProfile, resetOAuthRefreshQueuesForTest } = await import("./oauth.js")); +} + +function resolveApiKeyForProfileInTest( + params: Omit[0], "cfg">, +) { + return resolveApiKeyForProfile({ cfg: {}, ...params }); +} + +const { withFileLockMock } = vi.hoisted(() => ({ + withFileLockMock: vi.fn( + async (_filePath: string, _options: unknown, run: () => Promise) => await run(), + ), +})); + +vi.mock("../cli-credentials.js", () => ({ + readCodexCliCredentialsCached: () => null, + readMiniMaxCliCredentialsCached: () => null, + resetCliCredentialCachesForTest: () => undefined, + writeCodexCliCredentials: () => true, +})); + +vi.mock("@mariozechner/pi-ai/oauth", () => ({ + getOAuthApiKey: vi.fn(async () => null), + getOAuthProviders: () => [{ id: "openai-codex" }], +})); + +vi.mock("../../plugins/provider-runtime.runtime.js", () => ({ + formatProviderAuthProfileApiKeyWithPlugin: (params: { context?: { access?: string } }) => + params?.context?.access, + refreshProviderOAuthCredentialWithPlugin: async () => undefined, +})); + +vi.mock("../../infra/file-lock.js", () => ({ + FILE_LOCK_TIMEOUT_ERROR_CODE: "file_lock_timeout", + resetFileLockStateForTest: () => undefined, + withFileLock: withFileLockMock, +})); + +vi.mock("../../plugin-sdk/file-lock.js", () => ({ + FILE_LOCK_TIMEOUT_ERROR_CODE: "file_lock_timeout", + resetFileLockStateForTest: () => undefined, + withFileLock: withFileLockMock, +})); + +vi.mock("./doctor.js", () => ({ + formatAuthDoctorHint: async () => undefined, +})); + +vi.mock("./external-auth.js", () => ({ + overlayExternalAuthProfiles: (store: T) => store, + shouldPersistExternalAuthProfile: () => true, +})); + +vi.mock("./external-cli-sync.js", async () => { + const actual = + await vi.importActual("./external-cli-sync.js"); + return { + ...actual, + syncExternalCliCredentials: () => false, + readManagedExternalCliCredential: () => null, + resolveExternalCliAuthProfiles: () => [], + areOAuthCredentialsEquivalent: (a: unknown, b: unknown) => a === b, + }; +}); + +function createExpiredOauthStore(params: { + profileId: string; + provider: string; +}): AuthProfileStore { + return { + version: 1, + profiles: { + [params.profileId]: { + type: "oauth", + provider: params.provider, + access: "stale-access", + refresh: "stale-refresh", + expires: Date.now() - 60_000, + } satisfies OAuthCredential, + }, + }; +} + +function createLockTimeoutError(lockPath: string): FileLockTimeoutError { + return Object.assign(new Error(`file lock timeout for ${lockPath.slice(0, -5)}`), { + code: FILE_LOCK_TIMEOUT_ERROR_CODE as typeof FILE_LOCK_TIMEOUT_ERROR_CODE, + lockPath, + }); +} + +describe("OAuth refresh lock timeout classification", () => { + const envSnapshot = captureEnv([ + "OPENCLAW_STATE_DIR", + "OPENCLAW_AGENT_DIR", + "PI_CODING_AGENT_DIR", + ]); + let tempRoot = ""; + let agentDir = ""; + + beforeEach(async () => { + withFileLockMock.mockReset(); + withFileLockMock.mockImplementation( + async (_filePath: string, _options: unknown, run: () => Promise) => await run(), + ); + clearRuntimeAuthProfileStoreSnapshots(); + tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-oauth-lock-timeout-")); + process.env.OPENCLAW_STATE_DIR = tempRoot; + agentDir = path.join(tempRoot, "agents", "main", "agent"); + process.env.OPENCLAW_AGENT_DIR = agentDir; + process.env.PI_CODING_AGENT_DIR = agentDir; + await fs.mkdir(agentDir, { recursive: true }); + await loadOAuthModuleForTest(); + resetOAuthRefreshQueuesForTest(); + }); + + afterEach(async () => { + envSnapshot.restore(); + clearRuntimeAuthProfileStoreSnapshots(); + if (resetOAuthRefreshQueuesForTest) { + resetOAuthRefreshQueuesForTest(); + } + if (tempRoot) { + await fs.rm(tempRoot, { recursive: true, force: true }); + } + }); + + it("maps only global refresh lock timeouts to refresh_contention", async () => { + const profileId = "openai-codex:default"; + const provider = "openai-codex"; + const store = createExpiredOauthStore({ profileId, provider }); + saveAuthProfileStore(store, agentDir); + + const refreshLockPath = `${resolveOAuthRefreshLockPath(provider, profileId)}.lock`; + withFileLockMock.mockImplementationOnce(async () => { + throw createLockTimeoutError(refreshLockPath); + }); + + try { + await resolveApiKeyForProfileInTest({ + store, + profileId, + agentDir, + }); + throw new Error("expected refresh contention error"); + } catch (error) { + expect((error as Error).message).toContain("another process is already refreshing"); + expect((error as Error).message).toContain( + "Please wait for the in-flight refresh to finish and retry.", + ); + expect((error as Error & { cause?: unknown }).cause).toMatchObject({ + code: "refresh_contention", + }); + expect( + ((error as Error & { cause?: { cause?: unknown } }).cause as { cause?: unknown }).cause, + ).toMatchObject({ + code: FILE_LOCK_TIMEOUT_ERROR_CODE, + lockPath: refreshLockPath, + }); + } + }); + + it("preserves auth-store lock timeouts instead of remapping them to refresh_contention", async () => { + const profileId = "openai-codex:default"; + const provider = "openai-codex"; + const store = createExpiredOauthStore({ profileId, provider }); + saveAuthProfileStore(store, agentDir); + + const authStoreLockPath = `${resolveAuthStorePath(agentDir)}.lock`; + withFileLockMock + .mockImplementationOnce( + async (_filePath: string, _options: unknown, run: () => Promise) => await run(), + ) + .mockImplementationOnce(async () => { + throw createLockTimeoutError(authStoreLockPath); + }); + + try { + await resolveApiKeyForProfileInTest({ + store, + profileId, + agentDir, + }); + throw new Error("expected auth-store lock timeout"); + } catch (error) { + expect((error as Error).message).toContain("file lock timeout"); + expect((error as Error).message).toContain("Please try again or re-authenticate."); + expect((error as Error & { cause?: unknown }).cause).toMatchObject({ + code: FILE_LOCK_TIMEOUT_ERROR_CODE, + lockPath: authStoreLockPath, + }); + } + }); +}); diff --git a/src/agents/auth-profiles/oauth-refresh-timeout.test.ts b/src/agents/auth-profiles/oauth-refresh-timeout.test.ts index b9827362deb..e7423bdb48a 100644 --- a/src/agents/auth-profiles/oauth-refresh-timeout.test.ts +++ b/src/agents/auth-profiles/oauth-refresh-timeout.test.ts @@ -1,6 +1,21 @@ import { describe, expect, it } from "vitest"; import { OAUTH_REFRESH_CALL_TIMEOUT_MS, OAUTH_REFRESH_LOCK_OPTIONS } from "./constants.js"; +function computeMinimumRetryBudgetMs(): number { + let total = 0; + for (let attempt = 0; attempt < OAUTH_REFRESH_LOCK_OPTIONS.retries.retries; attempt += 1) { + total += Math.min( + OAUTH_REFRESH_LOCK_OPTIONS.retries.maxTimeout, + Math.max( + OAUTH_REFRESH_LOCK_OPTIONS.retries.minTimeout, + OAUTH_REFRESH_LOCK_OPTIONS.retries.minTimeout * + OAUTH_REFRESH_LOCK_OPTIONS.retries.factor ** attempt, + ), + ); + } + return total; +} + // Invariant tests for the two constants that together bound the OAuth // refresh critical section. Behavioural tests for the inner `setTimeout` // mechanics are deliberately omitted: the implementation is a thin @@ -41,4 +56,10 @@ describe("OAuth refresh call timeout (invariants)", () => { // lock during a legitimate slow-but-successful refresh. expect(OAUTH_REFRESH_LOCK_OPTIONS.stale).toBeGreaterThan(60_000); }); + + it("OAUTH_REFRESH_LOCK_OPTIONS retry budget outlasts the refresh call timeout", () => { + // Waiters should not exhaust their retry budget while a legitimate slow + // refresh is still within its allowed runtime budget. + expect(computeMinimumRetryBudgetMs()).toBeGreaterThan(OAUTH_REFRESH_CALL_TIMEOUT_MS); + }); }); diff --git a/src/agents/auth-profiles/oauth.fallback-to-main-agent.test.ts b/src/agents/auth-profiles/oauth.fallback-to-main-agent.test.ts index 956ceb19387..a2173077b96 100644 --- a/src/agents/auth-profiles/oauth.fallback-to-main-agent.test.ts +++ b/src/agents/auth-profiles/oauth.fallback-to-main-agent.test.ts @@ -55,6 +55,10 @@ async function loadFreshOAuthModuleForTest() { ({ resolveApiKeyForProfile } = await import("./oauth.js")); } +function createUsableOAuthExpiry(): number { + return Date.now() + 30 * 60 * 1000; +} + describe("resolveApiKeyForProfile fallback to main agent", () => { const envSnapshot = captureEnv([ "OPENCLAW_STATE_DIR", @@ -139,7 +143,7 @@ describe("resolveApiKeyForProfile fallback to main agent", () => { provider: "anthropic", access: "oauth-token", refresh: "refresh-token", - expires: Date.now() + 60_000, + expires: createUsableOAuthExpiry(), }, }, }; diff --git a/src/agents/auth-profiles/oauth.openai-codex-refresh-fallback.test.ts b/src/agents/auth-profiles/oauth.openai-codex-refresh-fallback.test.ts index 7961394688d..a41fd07a82d 100644 --- a/src/agents/auth-profiles/oauth.openai-codex-refresh-fallback.test.ts +++ b/src/agents/auth-profiles/oauth.openai-codex-refresh-fallback.test.ts @@ -87,6 +87,9 @@ function createExpiredOauthStore(params: { profileId: string; provider: string; access?: string; + refresh?: string; + accountId?: string; + email?: string; }): AuthProfileStore { return { version: 1, @@ -95,8 +98,10 @@ function createExpiredOauthStore(params: { type: "oauth", provider: params.provider, access: params.access ?? "cached-access-token", - refresh: "refresh-token", + refresh: params.refresh ?? "refresh-token", expires: Date.now() - 60_000, + accountId: params.accountId, + email: params.email, }, }, }; @@ -174,6 +179,46 @@ describe("resolveApiKeyForProfile openai-codex refresh fallback", () => { expect(refreshProviderOAuthCredentialWithPluginMock).toHaveBeenCalledTimes(1); }); + it("refreshes near-expiry openai-codex credentials before hard expiry", async () => { + const profileId = "openai-codex:default"; + saveAuthProfileStore( + { + version: 1, + profiles: { + [profileId]: { + type: "oauth", + provider: "openai-codex", + access: "near-expiry-access-token", + refresh: "near-expiry-refresh-token", + expires: Date.now() + 60_000, + }, + }, + }, + agentDir, + ); + refreshProviderOAuthCredentialWithPluginMock.mockResolvedValueOnce({ + type: "oauth", + provider: "openai-codex", + access: "rotated-access-token", + refresh: "rotated-refresh-token", + expires: Date.now() + 86_400_000, + accountId: "acct-rotated", + }); + + const result = await resolveApiKeyForProfile({ + store: ensureAuthProfileStore(agentDir), + profileId, + agentDir, + }); + + expect(result).toEqual({ + apiKey: "rotated-access-token", + provider: "openai-codex", + email: undefined, + }); + expect(refreshProviderOAuthCredentialWithPluginMock).toHaveBeenCalledTimes(1); + }); + it("persists plugin-refreshed openai-codex credentials before returning", async () => { const profileId = "openai-codex:default"; saveAuthProfileStore( @@ -320,6 +365,71 @@ describe("resolveApiKeyForProfile openai-codex refresh fallback", () => { ); }); + it("ignores mismatched fresh Codex CLI credentials when canonical local auth is bound to another account", async () => { + const profileId = "openai-codex:default"; + saveAuthProfileStore( + createExpiredOauthStore({ + profileId, + provider: "openai-codex", + access: "expired-local-access-token", + refresh: "local-refresh-token", + accountId: "acct-local", + }), + agentDir, + ); + readCodexCliCredentialsCachedMock.mockReturnValueOnce({ + type: "oauth", + provider: "openai-codex", + access: "fresh-cli-access-token", + refresh: "fresh-cli-refresh-token", + expires: Date.now() + 86_400_000, + accountId: "acct-external", + }); + refreshProviderOAuthCredentialWithPluginMock.mockImplementationOnce( + async (params?: { context?: unknown }) => { + expect(params?.context).toMatchObject({ + access: "expired-local-access-token", + refresh: "local-refresh-token", + accountId: "acct-local", + }); + return { + type: "oauth", + provider: "openai-codex", + access: "fresh-local-access-token", + refresh: "fresh-local-refresh-token", + expires: Date.now() + 86_400_000, + accountId: "acct-local", + }; + }, + ); + + await expect( + resolveApiKeyForProfile({ + store: ensureAuthProfileStore(agentDir), + profileId, + agentDir, + }), + ).resolves.toEqual({ + apiKey: "fresh-local-access-token", + provider: "openai-codex", + email: undefined, + }); + + const persisted = await readPersistedStore(agentDir); + expect(persisted.profiles[profileId]).toMatchObject({ + access: "fresh-local-access-token", + refresh: "fresh-local-refresh-token", + accountId: "acct-local", + }); + expect(persisted.profiles[profileId]).not.toEqual( + expect.objectContaining({ + access: "fresh-cli-access-token", + refresh: "fresh-cli-refresh-token", + accountId: "acct-external", + }), + ); + }); + it("keeps healthy local Codex OAuth over fresher imported CLI credentials", async () => { const profileId = "openai-codex:default"; saveAuthProfileStore( @@ -331,7 +441,7 @@ describe("resolveApiKeyForProfile openai-codex refresh fallback", () => { provider: "openai-codex", access: "healthy-local-access-token", refresh: "healthy-local-refresh-token", - expires: Date.now() + 60_000, + expires: Date.now() + 10 * 60_000, }, }, }, @@ -427,6 +537,135 @@ describe("resolveApiKeyForProfile openai-codex refresh fallback", () => { ); }); + it("adopts a fresher imported refresh token even when its access token is already expired", async () => { + const profileId = "openai-codex:default"; + saveAuthProfileStore( + { + version: 1, + profiles: { + [profileId]: { + type: "oauth", + provider: "openai-codex", + access: "expired-local-access-token", + refresh: "stale-local-refresh-token", + expires: Date.now() - 120_000, + }, + }, + }, + agentDir, + ); + readCodexCliCredentialsCachedMock.mockReturnValue({ + type: "oauth", + provider: "openai-codex", + access: "newer-but-expired-cli-access-token", + refresh: "fresh-cli-refresh-token", + expires: Date.now() - 30_000, + accountId: "acct-cli", + }); + refreshProviderOAuthCredentialWithPluginMock.mockImplementationOnce( + async (params?: { context?: unknown }) => { + expect(params?.context).toMatchObject({ + access: "newer-but-expired-cli-access-token", + refresh: "fresh-cli-refresh-token", + }); + return { + type: "oauth", + provider: "openai-codex", + access: "fresh-access-token", + refresh: "fresh-refresh-token", + expires: Date.now() + 86_400_000, + }; + }, + ); + + await expect( + resolveApiKeyForProfile({ + store: ensureAuthProfileStore(agentDir), + profileId, + agentDir, + }), + ).resolves.toEqual({ + apiKey: "fresh-access-token", + provider: "openai-codex", + email: undefined, + }); + + const persisted = await readPersistedStore(agentDir); + expect(persisted.profiles[profileId]).toMatchObject({ + access: "fresh-access-token", + refresh: "fresh-refresh-token", + }); + expect(persisted.profiles[profileId]).not.toEqual( + expect.objectContaining({ + refresh: "stale-local-refresh-token", + }), + ); + }); + + it("does not use mismatched imported Codex CLI refresh state as refresh context", async () => { + const profileId = "openai-codex:default"; + saveAuthProfileStore( + createExpiredOauthStore({ + profileId, + provider: "openai-codex", + access: "expired-local-access-token", + refresh: "local-refresh-token", + accountId: "acct-local", + }), + agentDir, + ); + readCodexCliCredentialsCachedMock.mockReturnValue({ + type: "oauth", + provider: "openai-codex", + access: "expired-cli-access-token", + refresh: "external-refresh-token", + expires: Date.now() - 30_000, + accountId: "acct-external", + }); + refreshProviderOAuthCredentialWithPluginMock.mockImplementationOnce( + async (params?: { context?: unknown }) => { + expect(params?.context).toMatchObject({ + access: "expired-local-access-token", + refresh: "local-refresh-token", + accountId: "acct-local", + }); + return { + type: "oauth", + provider: "openai-codex", + access: "fresh-local-access-token", + refresh: "fresh-local-refresh-token", + expires: Date.now() + 86_400_000, + accountId: "acct-local", + }; + }, + ); + + await expect( + resolveApiKeyForProfile({ + store: ensureAuthProfileStore(agentDir), + profileId, + agentDir, + }), + ).resolves.toEqual({ + apiKey: "fresh-local-access-token", + provider: "openai-codex", + email: undefined, + }); + + const persisted = await readPersistedStore(agentDir); + expect(persisted.profiles[profileId]).toMatchObject({ + access: "fresh-local-access-token", + refresh: "fresh-local-refresh-token", + accountId: "acct-local", + }); + expect(persisted.profiles[profileId]).not.toEqual( + expect.objectContaining({ + refresh: "external-refresh-token", + accountId: "acct-external", + }), + ); + }); + it("adopts fresher stored credentials after refresh_token_reused", async () => { const profileId = "openai-codex:default"; saveAuthProfileStore( @@ -446,7 +685,7 @@ describe("resolveApiKeyForProfile openai-codex refresh fallback", () => { provider: "openai-codex", access: "reloaded-access-token", refresh: "reloaded-refresh-token", - expires: Date.now() + 60_000, + expires: Date.now() + 10 * 60_000, }, }, }, @@ -510,7 +749,7 @@ describe("resolveApiKeyForProfile openai-codex refresh fallback", () => { newCredentials: { access: "retried-access-token", refresh: "retried-refresh-token", - expires: Date.now() + 60_000, + expires: Date.now() + 10 * 60_000, }, }; }); diff --git a/src/agents/auth-profiles/oauth.test.ts b/src/agents/auth-profiles/oauth.test.ts index ad6f5b718b2..3d380e55b55 100644 --- a/src/agents/auth-profiles/oauth.test.ts +++ b/src/agents/auth-profiles/oauth.test.ts @@ -116,6 +116,10 @@ afterAll(() => { vi.doUnmock("../../plugins/provider-runtime.runtime.js"); }); +function createUsableOAuthExpiry(): number { + return Date.now() + 30 * 60 * 1000; +} + describe("resolveApiKeyForProfile config compatibility", () => { it("accepts token credentials when config mode is oauth", async () => { const profileId = "anthropic:token"; @@ -183,7 +187,7 @@ describe("resolveApiKeyForProfile config compatibility", () => { provider: "anthropic", access: "access-123", refresh: "refresh-123", - expires: Date.now() + 60_000, + expires: createUsableOAuthExpiry(), }, }, }; diff --git a/src/agents/auth-profiles/oauth.ts b/src/agents/auth-profiles/oauth.ts index fb920ab81fc..3f516dbc85d 100644 --- a/src/agents/auth-profiles/oauth.ts +++ b/src/agents/auth-profiles/oauth.ts @@ -8,7 +8,11 @@ import { loadConfig } from "../../config/config.js"; import type { OpenClawConfig } from "../../config/types.openclaw.js"; import { coerceSecretRef } from "../../config/types.secrets.js"; import { formatErrorMessage } from "../../infra/errors.js"; -import { withFileLock } from "../../infra/file-lock.js"; +import { + FILE_LOCK_TIMEOUT_ERROR_CODE, + type FileLockTimeoutError, + withFileLock, +} from "../../infra/file-lock.js"; import { formatProviderAuthProfileApiKeyWithPlugin, refreshProviderOAuthCredentialWithPlugin, @@ -28,6 +32,7 @@ import { resolveEffectiveOAuthCredential } from "./effective-oauth.js"; import { areOAuthCredentialsEquivalent, hasUsableOAuthCredential, + isSafeToUseExternalCliCredential, readExternalCliBootstrapCredential, shouldReplaceStoredOAuthCredential, } from "./external-cli-sync.js"; @@ -286,6 +291,28 @@ async function withRefreshCallTimeout( } } +function createOAuthRefreshContentionError(params: { + profileId: string; + provider: string; + cause?: unknown; +}): Error & { code: "refresh_contention" } { + const error = new Error( + `OAuth refresh failed (refresh_contention): another process is already refreshing ${params.provider} for ${params.profileId}`, + { cause: params.cause }, + ); + return Object.assign(error, { code: "refresh_contention" as const }); +} + +function isGlobalOAuthRefreshLockTimeoutError( + error: unknown, + refreshLockPath: string, +): error is FileLockTimeoutError { + return ( + (error as { code?: string } | undefined)?.code === FILE_LOCK_TIMEOUT_ERROR_CODE && + (error as { lockPath?: string } | undefined)?.lockPath === `${refreshLockPath}.lock` + ); +} + /** * Drop any in-flight entries in the module-level refresh queue. Intended * exclusively for tests that exercise the concurrent-refresh surface; a @@ -549,183 +576,207 @@ async function doRefreshOAuthTokenWithLock(params: { // paths only take the per-store lock, so no cycle is possible. const globalRefreshLockPath = resolveOAuthRefreshLockPath(params.provider, params.profileId); - return await withFileLock(globalRefreshLockPath, OAUTH_REFRESH_LOCK_OPTIONS, async () => - withFileLock(authPath, AUTH_STORE_LOCK_OPTIONS, async () => { - // Locked refresh must bypass runtime snapshots so we can adopt fresher - // on-disk credentials written by another refresh attempt. - const store = loadAuthProfileStoreForSecretsRuntime(params.agentDir); - const cred = store.profiles[params.profileId]; - if (!cred || cred.type !== "oauth") { - return null; - } + try { + return await withFileLock(globalRefreshLockPath, OAUTH_REFRESH_LOCK_OPTIONS, async () => + withFileLock(authPath, AUTH_STORE_LOCK_OPTIONS, async () => { + // Locked refresh must bypass runtime snapshots so we can adopt fresher + // on-disk credentials written by another refresh attempt. + const store = loadAuthProfileStoreForSecretsRuntime(params.agentDir); + const cred = store.profiles[params.profileId]; + if (!cred || cred.type !== "oauth") { + return null; + } - if (hasUsableOAuthCredential(cred)) { - return { - apiKey: await buildOAuthApiKey(cred.provider, cred), - newCredentials: cred, - }; - } + if (hasUsableOAuthCredential(cred)) { + return { + apiKey: await buildOAuthApiKey(cred.provider, cred), + newCredentials: cred, + }; + } - // Inside-the-lock recheck: a prior agent that already held this lock may - // have completed a refresh and mirrored its fresh credential into the - // main store. If so, adopt into the local store and return without - // issuing another HTTP refresh. This is what turns N serialized - // refreshes into 1 refresh + (N-1) adoptions, preventing the - // `refresh_token_reused` storm reported in #26322. - if (params.agentDir) { - try { - const mainStore = loadAuthProfileStoreForSecretsRuntime(undefined); - const mainCred = mainStore.profiles[params.profileId]; - if ( - mainCred?.type === "oauth" && - mainCred.provider === cred.provider && - hasUsableOAuthCredential(mainCred) && - // Defense-in-depth identity gate. Tolerates the pure upgrade - // case (sub predates identity capture) but refuses positive - // mismatch, identity regression, and non-overlapping fields. - isSafeToCopyOAuthIdentity(cred, mainCred) - ) { - store.profiles[params.profileId] = { ...mainCred }; - saveAuthProfileStore(store, params.agentDir); - log.info("adopted fresh OAuth credential from main store (under refresh lock)", { + // Inside-the-lock recheck: a prior agent that already held this lock may + // have completed a refresh and mirrored its fresh credential into the + // main store. If so, adopt into the local store and return without + // issuing another HTTP refresh. This is what turns N serialized + // refreshes into 1 refresh + (N-1) adoptions, preventing the + // `refresh_token_reused` storm reported in #26322. + if (params.agentDir) { + try { + const mainStore = loadAuthProfileStoreForSecretsRuntime(undefined); + const mainCred = mainStore.profiles[params.profileId]; + if ( + mainCred?.type === "oauth" && + mainCred.provider === cred.provider && + hasUsableOAuthCredential(mainCred) && + // Defense-in-depth identity gate. Tolerates the pure upgrade + // case (sub predates identity capture) but refuses positive + // mismatch, identity regression, and non-overlapping fields. + isSafeToCopyOAuthIdentity(cred, mainCred) + ) { + store.profiles[params.profileId] = { ...mainCred }; + saveAuthProfileStore(store, params.agentDir); + log.info("adopted fresh OAuth credential from main store (under refresh lock)", { + profileId: params.profileId, + agentDir: params.agentDir, + expires: new Date(mainCred.expires).toISOString(), + }); + return { + apiKey: await buildOAuthApiKey(mainCred.provider, mainCred), + newCredentials: mainCred, + }; + } else if ( + mainCred?.type === "oauth" && + mainCred.provider === cred.provider && + hasUsableOAuthCredential(mainCred) && + !isSafeToCopyOAuthIdentity(cred, mainCred) + ) { + // Main has fresh creds but they belong to a DIFFERENT account — + // record the refusal so operators can diagnose, then proceed to + // our own refresh rather than leaking credentials. + log.warn("refused to adopt fresh main-store OAuth credential: identity mismatch", { + profileId: params.profileId, + agentDir: params.agentDir, + }); + } + } catch (err) { + log.debug("inside-lock main-store adoption failed; proceeding to refresh", { profileId: params.profileId, - agentDir: params.agentDir, - expires: new Date(mainCred.expires).toISOString(), + error: formatErrorMessage(err), }); - return { - apiKey: await buildOAuthApiKey(mainCred.provider, mainCred), - newCredentials: mainCred, - }; - } else if ( - mainCred?.type === "oauth" && - mainCred.provider === cred.provider && - hasUsableOAuthCredential(mainCred) && - !isSafeToCopyOAuthIdentity(cred, mainCred) - ) { - // Main has fresh creds but they belong to a DIFFERENT account — - // record the refusal so operators can diagnose, then proceed to - // our own refresh rather than leaking credentials. - log.warn("refused to adopt fresh main-store OAuth credential: identity mismatch", { + } + } + + const externallyManaged = readExternalCliBootstrapCredential({ + profileId: params.profileId, + credential: cred, + }); + let refreshCred = cred; + if (externallyManaged) { + if (isSafeToUseExternalCliCredential(cred, externallyManaged)) { + const hasUsableExternalCredential = hasUsableOAuthCredential(externallyManaged); + const shouldAdoptExternalCredential = + shouldReplaceStoredOAuthCredential(cred, externallyManaged) && + !areOAuthCredentialsEquivalent(cred, externallyManaged); + if (shouldAdoptExternalCredential) { + store.profiles[params.profileId] = externallyManaged; + saveAuthProfileStore(store, params.agentDir); + refreshCred = externallyManaged; + } + if (hasUsableExternalCredential) { + return { + apiKey: await buildOAuthApiKey(externallyManaged.provider, externallyManaged), + newCredentials: externallyManaged, + }; + } + } else { + log.warn("refused to adopt external cli OAuth credential: identity mismatch", { profileId: params.profileId, + provider: params.provider, agentDir: params.agentDir, }); } - } catch (err) { - log.debug("inside-lock main-store adoption failed; proceeding to refresh", { - profileId: params.profileId, - error: formatErrorMessage(err), - }); } - } - const externallyManaged = readExternalCliBootstrapCredential({ - profileId: params.profileId, - credential: cred, - }); - if (externallyManaged) { - if ( - shouldReplaceStoredOAuthCredential(cred, externallyManaged) && - !areOAuthCredentialsEquivalent(cred, externallyManaged) - ) { - store.profiles[params.profileId] = externallyManaged; + const pluginRefreshed = await withRefreshCallTimeout( + `refreshProviderOAuthCredentialWithPlugin(${refreshCred.provider})`, + OAUTH_REFRESH_CALL_TIMEOUT_MS, + () => + refreshProviderOAuthCredentialWithPlugin({ + provider: refreshCred.provider, + context: refreshCred, + }), + ); + if (pluginRefreshed) { + const refreshedCredentials: OAuthCredential = { + ...refreshCred, + ...pluginRefreshed, + type: "oauth", + }; + store.profiles[params.profileId] = refreshedCredentials; saveAuthProfileStore(store, params.agentDir); - } - if (hasUsableOAuthCredential(externallyManaged)) { + if (params.agentDir) { + const mainPath = resolveAuthStorePath(undefined); + if (mainPath !== authPath) { + await mirrorRefreshedCredentialIntoMainStore({ + profileId: params.profileId, + refreshed: refreshedCredentials, + }); + } + } return { - apiKey: await buildOAuthApiKey(externallyManaged.provider, externallyManaged), - newCredentials: externallyManaged, + apiKey: await buildOAuthApiKey(cred.provider, refreshedCredentials), + newCredentials: refreshedCredentials, }; } - } - const pluginRefreshed = await withRefreshCallTimeout( - `refreshProviderOAuthCredentialWithPlugin(${cred.provider})`, - OAUTH_REFRESH_CALL_TIMEOUT_MS, - () => - refreshProviderOAuthCredentialWithPlugin({ - provider: cred.provider, - context: cred, - }), - ); - if (pluginRefreshed) { - const refreshedCredentials: OAuthCredential = { - ...cred, - ...pluginRefreshed, + const oauthCreds: Record = { + [refreshCred.provider]: refreshCred, + }; + const result = + refreshCred.provider === "chutes" + ? await (async () => { + const newCredentials = await withRefreshCallTimeout( + `refreshChutesTokens(${refreshCred.provider})`, + OAUTH_REFRESH_CALL_TIMEOUT_MS, + () => refreshChutesTokens({ credential: refreshCred }), + ); + return { apiKey: newCredentials.access, newCredentials }; + })() + : await (async () => { + const oauthProvider = resolveOAuthProvider(refreshCred.provider); + if (!oauthProvider) { + return null; + } + if (typeof getOAuthApiKey !== "function") { + return null; + } + return await withRefreshCallTimeout( + `getOAuthApiKey(${oauthProvider})`, + OAUTH_REFRESH_CALL_TIMEOUT_MS, + () => getOAuthApiKey(oauthProvider, oauthCreds), + ); + })(); + if (!result) { + return null; + } + const mergedCred: OAuthCredential = { + ...refreshCred, + ...result.newCredentials, type: "oauth", }; - store.profiles[params.profileId] = refreshedCredentials; + store.profiles[params.profileId] = mergedCred; saveAuthProfileStore(store, params.agentDir); + + // Mirror the refreshed credential back into the main-agent store while + // both locks are still held (refresh lock + this agent's store lock) + // plus we'll take main-store lock inside the mirror. Doing this inside + // the refresh lock closes the cross-process race window where a second + // agent could acquire the refresh lock between our lock release and + // our main-store write, see only stale main creds, and redundantly + // refresh (reproducing refresh_token_reused). if (params.agentDir) { const mainPath = resolveAuthStorePath(undefined); if (mainPath !== authPath) { await mirrorRefreshedCredentialIntoMainStore({ profileId: params.profileId, - refreshed: refreshedCredentials, + refreshed: mergedCred, }); } } - return { - apiKey: await buildOAuthApiKey(cred.provider, refreshedCredentials), - newCredentials: refreshedCredentials, - }; - } - const oauthCreds: Record = { [cred.provider]: cred }; - const result = - cred.provider === "chutes" - ? await (async () => { - const newCredentials = await withRefreshCallTimeout( - `refreshChutesTokens(${cred.provider})`, - OAUTH_REFRESH_CALL_TIMEOUT_MS, - () => refreshChutesTokens({ credential: cred }), - ); - return { apiKey: newCredentials.access, newCredentials }; - })() - : await (async () => { - const oauthProvider = resolveOAuthProvider(cred.provider); - if (!oauthProvider) { - return null; - } - if (typeof getOAuthApiKey !== "function") { - return null; - } - return await withRefreshCallTimeout( - `getOAuthApiKey(${oauthProvider})`, - OAUTH_REFRESH_CALL_TIMEOUT_MS, - () => getOAuthApiKey(oauthProvider, oauthCreds), - ); - })(); - if (!result) { - return null; - } - const mergedCred: OAuthCredential = { - ...cred, - ...result.newCredentials, - type: "oauth", - }; - store.profiles[params.profileId] = mergedCred; - saveAuthProfileStore(store, params.agentDir); - - // Mirror the refreshed credential back into the main-agent store while - // both locks are still held (refresh lock + this agent's store lock) - // plus we'll take main-store lock inside the mirror. Doing this inside - // the refresh lock closes the cross-process race window where a second - // agent could acquire the refresh lock between our lock release and - // our main-store write, see only stale main creds, and redundantly - // refresh (reproducing refresh_token_reused). - if (params.agentDir) { - const mainPath = resolveAuthStorePath(undefined); - if (mainPath !== authPath) { - await mirrorRefreshedCredentialIntoMainStore({ - profileId: params.profileId, - refreshed: mergedCred, - }); - } - } - - return result; - }), - ); + return result; + }), + ); + } catch (error) { + if (isGlobalOAuthRefreshLockTimeoutError(error, globalRefreshLockPath)) { + throw createOAuthRefreshContentionError({ + profileId: params.profileId, + provider: params.provider, + cause: error, + }); + } + throw error; + } } async function tryResolveOAuthProfile( @@ -1029,6 +1080,7 @@ export async function resolveApiKeyForProfile( } const message = extractErrorMessage(error); + const errorCode = (error as { code?: string } | undefined)?.code; const hint = await formatAuthDoctorHint({ cfg, store: refreshedStore, @@ -1037,7 +1089,9 @@ export async function resolveApiKeyForProfile( }); throw new Error( `OAuth token refresh failed for ${cred.provider}: ${message}. ` + - "Please try again or re-authenticate." + + (errorCode === "refresh_contention" + ? "Please wait for the in-flight refresh to finish and retry." + : "Please try again or re-authenticate.") + (hint ? `\n\n${hint}` : ""), { cause: error }, ); diff --git a/src/agents/model-auth.profiles.test.ts b/src/agents/model-auth.profiles.test.ts index 1d3f72fca6b..bb6ac3883ad 100644 --- a/src/agents/model-auth.profiles.test.ts +++ b/src/agents/model-auth.profiles.test.ts @@ -92,10 +92,14 @@ afterEach(() => { const envVar = (...parts: string[]) => parts.join("_"); +function createUsableOAuthExpiry(): number { + return Date.now() + 30 * 60 * 1000; +} + const oauthFixture = { access: "access-token", refresh: "refresh-token", - expires: Date.now() + 60_000, + expires: createUsableOAuthExpiry(), accountId: "acct_123", }; diff --git a/src/agents/pi-embedded-helpers.formatassistanterrortext.test.ts b/src/agents/pi-embedded-helpers.formatassistanterrortext.test.ts index 9c69a2bc3cd..606eff5c516 100644 --- a/src/agents/pi-embedded-helpers.formatassistanterrortext.test.ts +++ b/src/agents/pi-embedded-helpers.formatassistanterrortext.test.ts @@ -247,6 +247,22 @@ describe("formatAssistantErrorText", () => { ); }); + it("returns a contention-specific message for OAuth refresh lock timeouts", () => { + const msg = makeAssistantError("file lock timeout for /tmp/openclaw-oauth-refresh.lock"); + expect(formatAssistantErrorText(msg)).toBe( + "Authentication refresh is already in progress elsewhere and this attempt timed out waiting for it. Retry in a moment.", + ); + }); + + it("returns a timeout-specific message for OAuth refresh hard timeouts", () => { + const msg = makeAssistantError( + 'OAuth refresh call "refreshProviderOAuthCredentialWithPlugin(openai-codex)" exceeded hard timeout (120000ms)', + ); + expect(formatAssistantErrorText(msg)).toBe( + "Authentication refresh timed out before the provider completed. Retry in a moment; re-authenticate only if it keeps failing.", + ); + }); + it("returns a missing-scope message for OpenAI Codex scope failures", () => { const msg = makeAssistantError( '401 {"type":"error","error":{"type":"permission_error","message":"Missing scopes: api.responses.write model.request"}}', diff --git a/src/agents/pi-embedded-helpers.isbillingerrormessage.test.ts b/src/agents/pi-embedded-helpers.isbillingerrormessage.test.ts index 0faace3647e..fc1ec760752 100644 --- a/src/agents/pi-embedded-helpers.isbillingerrormessage.test.ts +++ b/src/agents/pi-embedded-helpers.isbillingerrormessage.test.ts @@ -1228,6 +1228,37 @@ describe("classifyProviderRuntimeFailureKind", () => { ).toBe("auth_refresh"); }); + it("classifies OAuth refresh timeouts and lock contention distinctly", () => { + expect( + classifyProviderRuntimeFailureKind( + 'OAuth refresh call "refreshProviderOAuthCredentialWithPlugin(openai-codex)" exceeded hard timeout (120000ms)', + ), + ).toBe("refresh_timeout"); + expect( + classifyProviderRuntimeFailureKind("file lock timeout for /tmp/openclaw-oauth-refresh.lock"), + ).toBe("refresh_contention"); + expect( + classifyProviderRuntimeFailureKind({ + code: "refresh_contention", + message: + "OAuth token refresh failed for openai-codex: OAuth refresh failed (refresh_contention): another process is already refreshing openai-codex for openai-codex:default. Please wait for the in-flight refresh to finish and retry.", + }), + ).toBe("refresh_contention"); + expect( + classifyProviderRuntimeFailureKind( + "OAuth token refresh failed for openai-codex: file lock timeout for /tmp/agent/auth-profiles.json. Please try again or re-authenticate.", + ), + ).toBe("auth_refresh"); + }); + + it("classifies wrapped OpenAI Codex callback validation failures distinctly", () => { + expect( + classifyProviderRuntimeFailureKind( + "OpenAI Codex OAuth failed (callback_validation_failed): State mismatch", + ), + ).toBe("callback_validation"); + }); + it("classifies HTML 403 auth failures", () => { expect( classifyProviderRuntimeFailureKind( diff --git a/src/agents/pi-embedded-helpers/errors.ts b/src/agents/pi-embedded-helpers/errors.ts index 73f50535189..ab78cc8cc0c 100644 --- a/src/agents/pi-embedded-helpers/errors.ts +++ b/src/agents/pi-embedded-helpers/errors.ts @@ -256,6 +256,10 @@ export type FailoverClassification = export type ProviderRuntimeFailureKind = | "auth_scope" | "auth_refresh" + | "refresh_timeout" + | "refresh_contention" + | "callback_timeout" + | "callback_validation" | "auth_html_403" | "upstream_html" | "proxy" @@ -441,6 +445,26 @@ function isTimeoutTransportErrorMessage(raw: string, status?: number): boolean { return false; } +function isOAuthRefreshTimeoutMessage(raw: string): boolean { + return /\boauth refresh call\b.*\bexceeded hard timeout\b/i.test(raw); +} + +function isOAuthRefreshContentionMessage(raw: string): boolean { + return ( + /\brefresh_contention\b/i.test(raw) || + (/\bfile lock timeout\b/i.test(raw) && + /(?:\/|\\|^)(?:oauth-refresh|openclaw-oauth-refresh)[^/\n\\]*?(?:\.lock)?\b/i.test(raw)) + ); +} + +function isOAuthCallbackTimeoutMessage(raw: string): boolean { + return /\bcallback_timeout\b/i.test(raw); +} + +function isOAuthCallbackValidationMessage(raw: string): boolean { + return /\bcallback_validation_failed\b/i.test(raw); +} + function includesAnyHint(text: string, hints: readonly string[]): boolean { return hints.some((hint) => text.includes(hint)); } @@ -825,6 +849,21 @@ export function classifyProviderRuntimeFailureKind( if (!message && typeof status !== "number") { return "unknown"; } + if (normalizedSignal.code === "refresh_contention") { + return "refresh_contention"; + } + if (message && isOAuthRefreshContentionMessage(message)) { + return "refresh_contention"; + } + if (message && isOAuthRefreshTimeoutMessage(message)) { + return "refresh_timeout"; + } + if (message && isOAuthCallbackTimeoutMessage(message)) { + return "callback_timeout"; + } + if (message && isOAuthCallbackValidationMessage(message)) { + return "callback_validation"; + } if (message && classifyOAuthRefreshFailure(message)) { return "auth_refresh"; } @@ -911,6 +950,34 @@ export function formatAssistantErrorText( return "Authentication refresh failed. Re-authenticate this provider and try again."; } + if (providerRuntimeFailureKind === "refresh_contention") { + return ( + "Authentication refresh is already in progress elsewhere and this attempt " + + "timed out waiting for it. Retry in a moment." + ); + } + + if (providerRuntimeFailureKind === "refresh_timeout") { + return ( + "Authentication refresh timed out before the provider completed. " + + "Retry in a moment; re-authenticate only if it keeps failing." + ); + } + + if (providerRuntimeFailureKind === "callback_timeout") { + return ( + "Browser OAuth did not complete before manual fallback kicked in. " + + "Retry the login flow and paste the redirect URL if prompted." + ); + } + + if (providerRuntimeFailureKind === "callback_validation") { + return ( + "Browser OAuth returned an invalid or incomplete callback. " + + "Retry the login flow and make sure the full redirect URL is pasted if prompted." + ); + } + if (providerRuntimeFailureKind === "auth_scope") { return ( "Authentication is missing the required OpenAI Codex scopes. " + diff --git a/src/infra/file-lock.ts b/src/infra/file-lock.ts index 1e544006e72..90f14f390f4 100644 --- a/src/infra/file-lock.ts +++ b/src/infra/file-lock.ts @@ -1,7 +1,12 @@ -export type { FileLockHandle, FileLockOptions } from "../plugin-sdk/file-lock.js"; +export type { + FileLockHandle, + FileLockOptions, + FileLockTimeoutError, +} from "../plugin-sdk/file-lock.js"; export { acquireFileLock, drainFileLockStateForTest, + FILE_LOCK_TIMEOUT_ERROR_CODE, resetFileLockStateForTest, withFileLock, } from "../plugin-sdk/file-lock.js"; diff --git a/src/plugin-sdk/file-lock.test.ts b/src/plugin-sdk/file-lock.test.ts new file mode 100644 index 00000000000..42c91a4a3a9 --- /dev/null +++ b/src/plugin-sdk/file-lock.test.ts @@ -0,0 +1,58 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + acquireFileLock, + drainFileLockStateForTest, + FILE_LOCK_TIMEOUT_ERROR_CODE, + resetFileLockStateForTest, +} from "./file-lock.js"; + +describe("acquireFileLock", () => { + let tempDir = ""; + + beforeEach(async () => { + resetFileLockStateForTest(); + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-file-lock-")); + }); + + afterEach(async () => { + await drainFileLockStateForTest(); + if (tempDir) { + await fs.rm(tempDir, { recursive: true, force: true }); + } + }); + + it("respects the configured retry budget even when stale windows are much larger", async () => { + const filePath = path.join(tempDir, "oauth-refresh"); + const lockPath = `${filePath}.lock`; + const options = { + retries: { + retries: 1, + factor: 1, + minTimeout: 20, + maxTimeout: 20, + }, + stale: 100, + } as const; + + await fs.writeFile( + lockPath, + JSON.stringify({ pid: process.pid, createdAt: new Date().toISOString() }, null, 2), + "utf8", + ); + setTimeout(() => { + void fs.rm(lockPath, { force: true }); + }, 50); + + await expect(acquireFileLock(filePath, options)).rejects.toSatisfy((error) => { + expect(error).toMatchObject({ + code: FILE_LOCK_TIMEOUT_ERROR_CODE, + }); + expect((error as { lockPath?: string }).lockPath).toBeTruthy(); + expect((error as { lockPath?: string }).lockPath).toMatch(/oauth-refresh\.lock$/); + return true; + }); + }, 5_000); +}); diff --git a/src/plugin-sdk/file-lock.ts b/src/plugin-sdk/file-lock.ts index cd91e06ea93..356c4011481 100644 --- a/src/plugin-sdk/file-lock.ts +++ b/src/plugin-sdk/file-lock.ts @@ -123,6 +123,24 @@ export type FileLockHandle = { release: () => Promise; }; +export const FILE_LOCK_TIMEOUT_ERROR_CODE = "file_lock_timeout"; + +export type FileLockTimeoutError = Error & { + code: typeof FILE_LOCK_TIMEOUT_ERROR_CODE; + lockPath: string; +}; + +function createFileLockTimeoutError( + normalizedFile: string, + lockPath: string, +): FileLockTimeoutError { + const error = new Error(`file lock timeout for ${normalizedFile}`); + return Object.assign(error, { + code: FILE_LOCK_TIMEOUT_ERROR_CODE, + lockPath, + }) as FileLockTimeoutError; +} + async function releaseHeldLock(normalizedFile: string): Promise { const current = HELD_LOCKS.get(normalizedFile); if (!current) { @@ -162,8 +180,7 @@ export async function acquireFileLock( }; } - const attempts = Math.max(1, options.retries.retries + 1); - for (let attempt = 0; attempt < attempts; attempt += 1) { + for (let attempt = 0; attempt <= options.retries.retries; attempt += 1) { try { const handle = await fs.open(lockPath, "wx"); await handle.writeFile( @@ -184,14 +201,14 @@ export async function acquireFileLock( await fs.rm(lockPath, { force: true }).catch(() => undefined); continue; } - if (attempt >= attempts - 1) { + if (attempt >= options.retries.retries) { break; } await new Promise((resolve) => setTimeout(resolve, computeDelayMs(options.retries, attempt))); } } - throw new Error(`file lock timeout for ${normalizedFile}`); + throw createFileLockTimeoutError(normalizedFile, lockPath); } /** Run an async callback while holding a file lock, always releasing the lock afterward. */ diff --git a/src/plugin-sdk/provider-auth.ts b/src/plugin-sdk/provider-auth.ts index e1b3de8fa1e..a0d9657a144 100644 --- a/src/plugin-sdk/provider-auth.ts +++ b/src/plugin-sdk/provider-auth.ts @@ -69,6 +69,10 @@ export { } from "../secrets/provider-env-vars.js"; export { buildOauthProviderAuthResult } from "./provider-auth-result.js"; export { generatePkceVerifierChallenge, toFormUrlEncoded } from "./oauth-utils.js"; +export { + DEFAULT_OAUTH_REFRESH_MARGIN_MS, + hasUsableOAuthCredential, +} from "../agents/auth-profiles/credential-state.js"; export function isProviderApiKeyConfigured(params: { provider: string; diff --git a/src/plugins/provider-openai-codex-oauth.test.ts b/src/plugins/provider-openai-codex-oauth.test.ts index 0e265df421f..81c79c0b63f 100644 --- a/src/plugins/provider-openai-codex-oauth.test.ts +++ b/src/plugins/provider-openai-codex-oauth.test.ts @@ -215,6 +215,92 @@ describe("loginOpenAICodexOAuth", () => { }); }); + it("waits briefly before prompting for manual input after the local browser flow starts", async () => { + vi.useFakeTimers(); + const { prompter } = createPrompter(); + const runtime = createRuntime(); + mocks.loginOpenAICodex.mockImplementation( + async (opts: { + onAuth: (event: { url: string }) => Promise; + onManualCodeInput?: () => Promise; + }) => { + await opts.onAuth({ + url: "https://auth.openai.com/oauth/authorize?state=abc", + }); + expect(opts.onManualCodeInput).toBeTypeOf("function"); + const manualPromise = opts.onManualCodeInput?.(); + await vi.advanceTimersByTimeAsync(14_000); + expect(manualPromise).toBeDefined(); + expect(prompter.text).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(1_000); + expect(prompter.text).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(1_000); + return { + provider: "openai-codex" as const, + access: "access-token", + refresh: "refresh-token", + expires: Date.now() + 60_000, + email: "user@example.com", + manualCode: await manualPromise, + }; + }, + ); + + await expect( + loginOpenAICodexOAuth({ + prompter, + runtime, + isRemote: false, + openUrl: async () => {}, + }), + ).resolves.toMatchObject({ + access: "access-token", + refresh: "refresh-token", + }); + + expect(prompter.text).toHaveBeenCalledWith({ + message: "Paste the authorization code (or full redirect URL):", + validate: expect.any(Function), + }); + expect(runtime.log).toHaveBeenCalledWith( + "OpenAI Codex OAuth callback did not arrive within 15000ms; switching to manual entry (callback_timeout).", + ); + vi.useRealTimers(); + }); + + it("clears the local manual fallback timer when browser callback settles first", async () => { + vi.useFakeTimers(); + mocks.loginOpenAICodex.mockImplementation( + async (opts: { + onAuth: (event: { url: string }) => Promise; + onManualCodeInput?: () => Promise; + }) => { + await opts.onAuth({ + url: "https://auth.openai.com/oauth/authorize?state=abc", + }); + expect(opts.onManualCodeInput).toBeTypeOf("function"); + void opts.onManualCodeInput?.(); + return { + provider: "openai-codex" as const, + access: "access-token", + refresh: "refresh-token", + expires: Date.now() + 60_000, + email: "user@example.com", + }; + }, + ); + + await expect(runCodexOAuth({ isRemote: false })).resolves.toMatchObject({ + result: expect.objectContaining({ + access: "access-token", + refresh: "refresh-token", + }), + }); + + expect(vi.getTimerCount()).toBe(0); + vi.useRealTimers(); + }); + it("continues OAuth flow on non-certificate preflight failures", async () => { const creds = { provider: "openai-codex" as const, @@ -238,7 +324,7 @@ describe("loginOpenAICodexOAuth", () => { expect(prompter.note).not.toHaveBeenCalledWith("tls fix", "OAuth prerequisites"); }); - it("surfaces TLS preflight guidance but still attempts OAuth login", async () => { + it("fails fast on TLS certificate preflight failures before starting OAuth login", async () => { mocks.runOpenAIOAuthTlsPreflight.mockResolvedValue({ ok: false, kind: "tls-cert", @@ -265,14 +351,108 @@ describe("loginOpenAICodexOAuth", () => { isRemote: false, openUrl: async () => {}, }), - ).resolves.toEqual(creds); + ).rejects.toThrow(/OAuth prerequisites/i); - expect(mocks.loginOpenAICodex).toHaveBeenCalledOnce(); - expect(runtime.log).toHaveBeenCalledWith("Run brew postinstall openssl@3"); - expect(runtime.error).not.toHaveBeenCalledWith("Run brew postinstall openssl@3"); + expect(mocks.loginOpenAICodex).not.toHaveBeenCalled(); expect(prompter.note).toHaveBeenCalledWith( "Run brew postinstall openssl@3", "OAuth prerequisites", ); }); + + it("prompts for manual input immediately when the local callback flow never starts", async () => { + vi.useFakeTimers(); + const { prompter } = createPrompter(); + const runtime = createRuntime(); + mocks.loginOpenAICodex.mockImplementation( + async (opts: { onManualCodeInput?: () => Promise }) => { + expect(opts.onManualCodeInput).toBeTypeOf("function"); + const manualCode = await opts.onManualCodeInput?.(); + return { + provider: "openai-codex" as const, + access: "access-token", + refresh: "refresh-token", + expires: Date.now() + 60_000, + email: "user@example.com", + manualCode, + }; + }, + ); + + await expect( + loginOpenAICodexOAuth({ + prompter, + runtime, + isRemote: false, + openUrl: async () => {}, + }), + ).resolves.toMatchObject({ + access: "access-token", + refresh: "refresh-token", + }); + + expect(prompter.text).toHaveBeenCalledWith({ + message: "Paste the authorization code (or full redirect URL):", + validate: expect.any(Function), + }); + expect(vi.getTimerCount()).toBe(0); + vi.useRealTimers(); + }); + + it("suppresses the local manual prompt when oauth settles just after the fallback deadline", async () => { + vi.useFakeTimers(); + const { prompter } = createPrompter(); + const runtime = createRuntime(); + mocks.loginOpenAICodex.mockImplementation( + async (opts: { + onAuth: (event: { url: string }) => Promise; + onManualCodeInput?: () => Promise; + }) => { + await opts.onAuth({ + url: "https://auth.openai.com/oauth/authorize?state=abc", + }); + void opts.onManualCodeInput?.(); + await vi.advanceTimersByTimeAsync(15_500); + return { + provider: "openai-codex" as const, + access: "access-token", + refresh: "refresh-token", + expires: Date.now() + 60_000, + email: "user@example.com", + }; + }, + ); + + await expect( + loginOpenAICodexOAuth({ + prompter, + runtime, + isRemote: false, + openUrl: async () => {}, + }), + ).resolves.toMatchObject({ + access: "access-token", + refresh: "refresh-token", + }); + + expect(prompter.text).not.toHaveBeenCalled(); + vi.useRealTimers(); + }); + + it("rewrites callback validation failures with a stable internal code", async () => { + mocks.loginOpenAICodex.mockRejectedValue(new Error("State mismatch")); + + const { prompter, spin } = createPrompter(); + const runtime = createRuntime(); + await expect( + loginOpenAICodexOAuth({ + prompter, + runtime, + isRemote: false, + openUrl: async () => {}, + }), + ).rejects.toThrow(/callback_validation_failed/i); + + expect(spin.stop).toHaveBeenCalledWith("OpenAI OAuth failed"); + }); }); diff --git a/src/plugins/provider-openai-codex-oauth.ts b/src/plugins/provider-openai-codex-oauth.ts index 9cc6fef45df..c94c9a990e5 100644 --- a/src/plugins/provider-openai-codex-oauth.ts +++ b/src/plugins/provider-openai-codex-oauth.ts @@ -1,7 +1,9 @@ import { loginOpenAICodex, type OAuthCredentials } from "@mariozechner/pi-ai/oauth"; +import { formatErrorMessage } from "../infra/errors.js"; import { ensureGlobalUndiciEnvProxyDispatcher } from "../infra/net/undici-global-dispatcher.js"; import type { RuntimeEnv } from "../runtime.js"; import type { WizardPrompter } from "../wizard/prompts.js"; +import type { OAuthPrompt } from "./provider-oauth-flow.js"; import { createVpsAwareOAuthHandlers } from "./provider-oauth-flow.js"; import { formatOpenAIOAuthTlsPreflightFix, @@ -10,6 +12,111 @@ import { const manualInputPromptMessage = "Paste the authorization code (or full redirect URL):"; const openAICodexOAuthOriginator = "openclaw"; +const localManualFallbackDelayMs = 15_000; +const localManualFallbackGraceMs = 1_000; + +type OpenAICodexOAuthFailureCode = "callback_timeout" | "callback_validation_failed"; + +function waitForDelayOrLoginSettle(params: { + delayMs: number; + waitForLoginToSettle: Promise; +}): Promise<"delay" | "settled"> { + return new Promise((resolve) => { + let finished = false; + const finish = (outcome: "delay" | "settled") => { + if (finished) { + return; + } + finished = true; + clearTimeout(timeoutHandle); + resolve(outcome); + }; + const timeoutHandle = setTimeout(() => finish("delay"), params.delayMs); + params.waitForLoginToSettle.then( + () => finish("settled"), + () => finish("settled"), + ); + }); +} + +function createNeverSettlingPromptResult(): Promise { + return new Promise(() => undefined); +} + +function createOpenAICodexOAuthError( + code: OpenAICodexOAuthFailureCode, + message: string, + cause?: unknown, +): Error & { code: OpenAICodexOAuthFailureCode } { + const error = new Error(`OpenAI Codex OAuth failed (${code}): ${message}`, { cause }); + return Object.assign(error, { code }); +} + +function rewriteOpenAICodexOAuthError(error: unknown): Error { + const message = formatErrorMessage(error); + if (/state mismatch|missing authorization code/i.test(message)) { + return createOpenAICodexOAuthError("callback_validation_failed", message, error); + } + return error instanceof Error ? error : new Error(message); +} + +function createManualCodeInputHandler(params: { + isRemote: boolean; + onPrompt: (prompt: OAuthPrompt) => Promise; + runtime: RuntimeEnv; + spin: ReturnType; + waitForLoginToSettle: Promise; + hasBrowserAuthStarted: () => boolean; +}): (() => Promise) | undefined { + if (params.isRemote) { + return async () => + await params.onPrompt({ + message: manualInputPromptMessage, + }); + } + + return async () => { + if (!params.hasBrowserAuthStarted()) { + params.spin.update( + "Local OAuth callback was unavailable. Paste the redirect URL to continue…", + ); + params.runtime.log( + "OpenAI Codex OAuth local callback did not start; switching to manual entry immediately.", + ); + return await params.onPrompt({ + message: manualInputPromptMessage, + }); + } + + const outcome = await waitForDelayOrLoginSettle({ + delayMs: localManualFallbackDelayMs, + waitForLoginToSettle: params.waitForLoginToSettle, + }); + if (outcome === "settled") { + // markLoginSettled() runs in loginOpenAICodexOAuth's finally block, so + // reaching this branch means the outer login call has already completed. + // Return a never-settling promise to suppress an unnecessary manual + // prompt without feeding placeholder input back into the upstream flow. + return await createNeverSettlingPromptResult(); + } + + const settledDuringGraceWindow = await waitForDelayOrLoginSettle({ + delayMs: localManualFallbackGraceMs, + waitForLoginToSettle: params.waitForLoginToSettle, + }); + if (settledDuringGraceWindow === "settled") { + return await createNeverSettlingPromptResult(); + } + + params.spin.update("Browser callback did not finish. Paste the redirect URL to continue…"); + params.runtime.log( + `OpenAI Codex OAuth callback did not arrive within ${localManualFallbackDelayMs}ms; switching to manual entry (callback_timeout).`, + ); + return await params.onPrompt({ + message: manualInputPromptMessage, + }); + }; +} export async function loginOpenAICodexOAuth(params: { prompter: WizardPrompter; @@ -25,8 +132,9 @@ export async function loginOpenAICodexOAuth(params: { const preflight = await runOpenAIOAuthTlsPreflight(); if (!preflight.ok && preflight.kind === "tls-cert") { const hint = formatOpenAIOAuthTlsPreflightFix(preflight); - runtime.log(hint); await prompter.note(hint, "OAuth prerequisites"); + runtime.error(hint); + throw new Error(`OpenAI Codex OAuth prerequisites failed: ${preflight.message}`); } await prompter.note( @@ -45,6 +153,11 @@ export async function loginOpenAICodexOAuth(params: { ); const spin = prompter.progress("Starting OAuth flow…"); + let browserAuthStarted = false; + let markLoginSettled!: () => void; + const waitForLoginToSettle = new Promise((resolve) => { + markLoginSettled = resolve; + }); try { const { onAuth: baseOnAuth, onPrompt } = createVpsAwareOAuthHandlers({ isRemote, @@ -55,25 +168,34 @@ export async function loginOpenAICodexOAuth(params: { localBrowserMessage: localBrowserMessage ?? "Complete sign-in in browser…", manualPromptMessage: manualInputPromptMessage, }); + const onAuth: typeof baseOnAuth = async (event) => { + browserAuthStarted = true; + await baseOnAuth(event); + }; const creds = await loginOpenAICodex({ - onAuth: baseOnAuth, + onAuth, onPrompt, originator: openAICodexOAuthOriginator, - onManualCodeInput: isRemote - ? async () => - await onPrompt({ - message: manualInputPromptMessage, - }) - : undefined, + onManualCodeInput: createManualCodeInputHandler({ + isRemote, + onPrompt, + runtime, + spin, + waitForLoginToSettle, + hasBrowserAuthStarted: () => browserAuthStarted, + }), onProgress: (msg: string) => spin.update(msg), }); spin.stop("OpenAI OAuth complete"); return creds ?? null; } catch (err) { spin.stop("OpenAI OAuth failed"); - runtime.error(String(err)); + const rewrittenError = rewriteOpenAICodexOAuthError(err); + runtime.error(String(rewrittenError)); await prompter.note("Trouble with OAuth? See https://docs.openclaw.ai/start/faq", "OAuth help"); - throw err; + throw rewrittenError; + } finally { + markLoginSettled(); } }