Skip to content

Commit bcaf63c

Browse files
steipetesteipete
committed
fix(auth): use canonical profile order in discovery
1 parent ee5ab7a commit bcaf63c

10 files changed

Lines changed: 191 additions & 54 deletions

File tree

‎src/agents/agent-auth-credentials.ts‎

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@
22
import { normalizeProviderId } from "@openclaw/model-catalog-core/provider-id";
33
import { asDateTimestampMs } from "@openclaw/normalization-core/number-coercion";
44
import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce";
5+
import type { OpenClawConfig } from "../config/types.openclaw.js";
56
import { coerceSecretRef } from "../config/types.secrets.js";
6-
import type { AuthProfileCredential, AuthProfileStore } from "./auth-profiles.js";
7+
import { resolveAuthProfileOrder } from "./auth-profiles/order.js";
8+
import type { AuthProfileCredential, AuthProfileStore } from "./auth-profiles/types.js";
79

810
// Converts auth-profile credentials into the compact credential map consumed by
911
// agent runtimes. Secret refs can be represented by markers without reading
@@ -22,6 +24,7 @@ export type AgentCredentialMap = Record<string, AgentCredential>;
2224

2325
type ResolveAgentCredentialMapOptions = {
2426
includeSecretRefPlaceholders?: boolean;
27+
config?: OpenClawConfig;
2528
};
2629

2730
const AGENT_SECRET_REF_CONFIGURED_MARKER = "openclaw-secret-ref-configured";
@@ -85,7 +88,7 @@ function convertAuthProfileCredentialToAgent(
8588
return null;
8689
}
8790

88-
/** Build one credential per normalized provider from an auth profile store. */
91+
/** Build one canonically selected credential per normalized provider. */
8992
export function resolveAgentCredentialMapFromStore(
9093
store: AuthProfileStore,
9194
options?: ResolveAgentCredentialMapOptions,
@@ -96,27 +99,27 @@ export function resolveAgentCredentialMapFromStore(
9699
if (!provider) {
97100
continue;
98101
}
99-
const converted = convertAuthProfileCredentialToAgent(credential, options);
100-
if (!converted) {
102+
if (credentials[provider]) {
101103
continue;
102104
}
103-
const existing = credentials[provider];
104-
if (!existing) {
105-
credentials[provider] = converted;
106-
continue;
107-
}
108-
// Prefer a non-expired OAuth credential over an expired one when the
109-
// store holds multiple profiles for the same provider. Background
110-
// operations still receive an expired credential as a last resort
111-
// (the refresh path may recover it), but the map picks the fresher
112-
// profile when one is available.
113-
if (
114-
existing.type === "oauth" &&
115-
converted.type === "oauth" &&
116-
Date.now() >= existing.expires &&
117-
Date.now() < converted.expires
118-
) {
119-
credentials[provider] = converted;
105+
// Discovery must not grow a second auth policy: explicit order, provider
106+
// aliases, eligibility, and automatic preference all belong to this resolver.
107+
const profileIds = resolveAuthProfileOrder({
108+
cfg: options?.config,
109+
store,
110+
provider,
111+
...(options?.includeSecretRefPlaceholders === true ? { readinessMode: "read-only" } : {}),
112+
});
113+
for (const profileId of profileIds) {
114+
const profile = store.profiles[profileId];
115+
if (!profile) {
116+
continue;
117+
}
118+
const converted = convertAuthProfileCredentialToAgent(profile, options);
119+
if (converted) {
120+
credentials[provider] = converted;
121+
break;
122+
}
120123
}
121124
}
122125
return credentials;

‎src/agents/agent-auth-discovery.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ export function resolveAgentCredentialsForDiscovery(
5252
const credentials = addEnvBackedAgentCredentials(
5353
resolveAgentCredentialMapFromStore(store, {
5454
includeSecretRefPlaceholders: options?.readOnly === true,
55+
config: options?.config,
5556
}),
5657
{
5758
config: options?.config,

‎src/agents/agent-model-discovery.auth.test.ts‎

Lines changed: 52 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -130,11 +130,6 @@ describe("discoverAuthStorage", () => {
130130
expect(credentials.openai).toBeUndefined();
131131
});
132132

133-
// Regression: the oauth branch did not check past expiry at all, so an
134-
// expired OAuth credential silently took the one-per-provider slot ahead
135-
// of any valid profile. The conversion layer keeps expired-but-refreshable
136-
// OAuth (the auth resolution path may refresh it), but the map builder now
137-
// prefers a non-expired credential when one is available.
138133
it("keeps expired OAuth when it is the sole profile for a provider", () => {
139134
const credentials = resolveAgentCredentialMapFromStore({
140135
version: 1,
@@ -149,21 +144,23 @@ describe("discoverAuthStorage", () => {
149144
},
150145
});
151146

152-
// Sole profile — the expired credential is kept so the refresh
153-
// path has something to work with.
154-
const openai = credentials.openai as
155-
| { type?: string; access?: string; refresh?: string }
156-
| undefined;
157-
expect(openai?.type).toBe("oauth");
158-
expect(openai?.access).toBe("sole-access");
147+
expect(credentials.openai).toEqual({
148+
type: "oauth",
149+
access: "sole-access",
150+
refresh: "sole-refresh",
151+
expires: expect.any(Number),
152+
});
159153
});
160154

161-
// A same-provider valid credential after an expired one must still fill
162-
// the per-provider slot: the map skips expired entries and keeps scanning.
163-
it("skips an expired first credential for a provider and picks the next valid one", () => {
155+
it("uses canonical mode and expiry ordering instead of profile insertion order", () => {
164156
const credentials = resolveAgentCredentialMapFromStore({
165157
version: 1,
166158
profiles: {
159+
"openai:key": {
160+
type: "api_key",
161+
provider: "openai",
162+
key: "insertion-order-key",
163+
},
167164
"openai:expired": {
168165
type: "oauth",
169166
provider: "openai",
@@ -181,11 +178,46 @@ describe("discoverAuthStorage", () => {
181178
},
182179
});
183180

184-
const openai = credentials.openai as
185-
| { type?: string; access?: string; refresh?: string }
186-
| undefined;
187-
expect(openai?.type).toBe("oauth");
188-
expect(openai?.access).toBe("valid-access");
181+
expect(credentials.openai).toEqual({
182+
type: "oauth",
183+
access: "valid-access",
184+
refresh: "valid-refresh",
185+
expires: expect.any(Number),
186+
});
187+
});
188+
189+
it("passes configured auth order through discovery selection", async () => {
190+
await withAgentDir(async (agentDir) => {
191+
writeAuthProfilesSqlite(agentDir, {
192+
version: 1,
193+
profiles: {
194+
"openai:oauth": {
195+
type: "oauth",
196+
provider: "openai",
197+
access: "valid-access",
198+
refresh: "valid-refresh",
199+
expires: Date.now() + 3600_000,
200+
},
201+
"openai:key": {
202+
type: "api_key",
203+
provider: "openai",
204+
key: "configured-order-key",
205+
},
206+
},
207+
});
208+
const authStorage = discoverAuthStorage(agentDir, {
209+
skipExternalAuthProfiles: true,
210+
env: {},
211+
config: {
212+
auth: { order: { openai: ["openai:key", "openai:oauth"] } },
213+
},
214+
});
215+
216+
expect(authStorage.get("openai")).toEqual({
217+
type: "api_key",
218+
key: "configured-order-key",
219+
});
220+
});
189221
});
190222

191223
it("keeps keyRef and tokenRef profiles visible only for read-only agent discovery", () => {

‎src/agents/auth-profiles/order.test.ts‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,79 @@ describe("resolveAuthProfileOrder", () => {
254254
expect(order).toStrictEqual(["fixture-provider:oauth", "fixture-provider:key"]);
255255
});
256256

257+
it.each([
258+
["expired first", ["openai:expired", "openai:valid"]],
259+
["valid first", ["openai:valid", "openai:expired"]],
260+
])("prefers live OAuth before expired OAuth when %s", (_caseName, profileIds) => {
261+
const now = Date.now();
262+
const profiles: AuthProfileStore["profiles"] = {
263+
"openai:expired": {
264+
type: "oauth",
265+
provider: "openai",
266+
access: "expired-access",
267+
refresh: "expired-refresh",
268+
expires: now - 60_000,
269+
},
270+
"openai:valid": {
271+
type: "oauth",
272+
provider: "openai",
273+
access: "valid-access",
274+
refresh: "valid-refresh",
275+
expires: now + 60_000,
276+
},
277+
};
278+
const orderedProfiles: AuthProfileStore["profiles"] = {};
279+
for (const profileId of profileIds) {
280+
const profile = profiles[profileId];
281+
if (profile) {
282+
orderedProfiles[profileId] = profile;
283+
}
284+
}
285+
286+
expect(
287+
resolveAuthProfileOrder({
288+
store: {
289+
version: 1,
290+
profiles: orderedProfiles,
291+
usageStats: {
292+
"openai:expired": { lastUsed: 0 },
293+
"openai:valid": { lastUsed: 10_000 },
294+
},
295+
},
296+
provider: "openai",
297+
}),
298+
).toStrictEqual(["openai:valid", "openai:expired"]);
299+
});
300+
301+
it("keeps an explicit order authoritative across OAuth expiry state", () => {
302+
const now = Date.now();
303+
const store: AuthProfileStore = {
304+
version: 1,
305+
profiles: {
306+
"openai:expired": {
307+
type: "oauth",
308+
provider: "openai",
309+
access: "expired-access",
310+
refresh: "expired-refresh",
311+
expires: now - 60_000,
312+
},
313+
"openai:valid": {
314+
type: "oauth",
315+
provider: "openai",
316+
access: "valid-access",
317+
refresh: "valid-refresh",
318+
expires: now + 60_000,
319+
},
320+
},
321+
order: { openai: ["openai:expired", "openai:valid"] },
322+
};
323+
324+
expect(resolveAuthProfileOrder({ store, provider: "openai" })).toStrictEqual([
325+
"openai:expired",
326+
"openai:valid",
327+
]);
328+
});
329+
257330
it("does not fall back past an explicit configured auth order", async () => {
258331
const store: AuthProfileStore = {
259332
version: 1,

‎src/agents/auth-profiles/order.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
} from "../provider-auth-aliases.js";
1515
import {
1616
evaluateStoredCredentialEligibility,
17+
resolveTokenExpiryState,
1718
type AuthCredentialReasonCode,
1819
} from "./credential-state.js";
1920
import { dedupeProfileIds, listProfilesForProvider } from "./profile-list.js";
@@ -487,22 +488,31 @@ function orderProfilesByMode(
487488
}
488489
}
489490

490-
// Sort available profiles by type preference, then by lastUsed (oldest first = round-robin within type)
491+
// Sort by type, OAuth expiry state, then lastUsed for round-robin within each tier.
491492
const scored = available.map((profileId) => {
492-
const type = store.profiles[profileId]?.type;
493+
const profile = store.profiles[profileId];
494+
const type = profile?.type;
493495
const typeScore = type === "oauth" ? 0 : type === "token" ? 1 : type === "api_key" ? 2 : 3;
496+
// A refreshable expired OAuth profile remains eligible, but refreshing an
497+
// obsolete profile can rotate a one-time refresh token while a live peer exists.
498+
const expiryScore =
499+
profile?.type === "oauth" && resolveTokenExpiryState(profile.expires, now) === "expired"
500+
? 1
501+
: 0;
494502
const lastUsed = store.usageStats?.[profileId]?.lastUsed ?? 0;
495-
return { profileId, typeScore, lastUsed };
503+
return { profileId, typeScore, expiryScore, lastUsed };
496504
});
497505

498506
// Primary sort: type preference (oauth > token > api_key).
499-
// Secondary sort: lastUsed (oldest first for round-robin within type).
500507
const sorted = scored
501508
.toSorted((a, b) => {
502509
// First by type (oauth > token > api_key)
503510
if (a.typeScore !== b.typeScore) {
504511
return a.typeScore - b.typeScore;
505512
}
513+
if (a.expiryScore !== b.expiryScore) {
514+
return a.expiryScore - b.expiryScore;
515+
}
506516
// Then by lastUsed (oldest first)
507517
return a.lastUsed - b.lastUsed;
508518
})

‎src/agents/btw.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,10 @@ async function resolveRuntimeModel(params: {
488488
}> {
489489
const modelsOptions = params.workspaceDir ? { workspaceDir: params.workspaceDir } : undefined;
490490
await ensureOpenClawModelsJson(params.cfg, params.agentDir, modelsOptions);
491-
const authStorage = discoverAuthStorage(params.agentDir);
491+
const authStorage = discoverAuthStorage(params.agentDir, {
492+
config: params.cfg,
493+
...(params.workspaceDir ? { workspaceDir: params.workspaceDir } : {}),
494+
});
492495
const modelRegistry = discoverModels(authStorage, params.agentDir, {
493496
config: params.cfg,
494497
...modelsOptions,

‎src/agents/embedded-agent-runner/model-discovery-cache.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,10 @@ function discoverFreshAgentStores(
142142
options: Pick<DiscoverCachedAgentStoresOptions, "config" | "workspaceDir">,
143143
pluginMetadataSnapshot: PluginMetadataSnapshot | undefined,
144144
): DiscoveryStores {
145-
const authStorage = discoverAuthStorage(agentDir);
145+
const authStorage = discoverAuthStorage(agentDir, {
146+
...(options.config ? { config: options.config } : {}),
147+
...(options.workspaceDir ? { workspaceDir: options.workspaceDir } : {}),
148+
});
146149
const modelRegistry = discoverModels(authStorage, agentDir, {
147150
...(options.config ? { config: options.config } : {}),
148151
...(pluginMetadataSnapshot ? { pluginMetadataSnapshot } : {}),

‎src/agents/embedded-agent-runner/model.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1614,7 +1614,12 @@ export function resolveModel(
16141614
? discoverCachedAgentStoresForAgent(resolvedAgentDir, cfg, workspaceDir)
16151615
: undefined;
16161616
const authStorage =
1617-
options?.authStorage ?? cachedStores?.authStorage ?? discoverAuthStorage(resolvedAgentDir);
1617+
options?.authStorage ??
1618+
cachedStores?.authStorage ??
1619+
discoverAuthStorage(resolvedAgentDir, {
1620+
...(cfg ? { config: cfg } : {}),
1621+
...(workspaceDir ? { workspaceDir } : {}),
1622+
});
16181623
const modelRegistry =
16191624
options?.modelRegistry ??
16201625
cachedStores?.modelRegistry ??
@@ -1695,7 +1700,10 @@ export async function resolveModelAsync(
16951700
options?.authStorage ??
16961701
emptyDiscoveryStores?.authStorage ??
16971702
cachedStores?.authStorage ??
1698-
discoverAuthStorage(resolvedAgentDir);
1703+
discoverAuthStorage(resolvedAgentDir, {
1704+
...(cfg ? { config: cfg } : {}),
1705+
...(workspaceDir ? { workspaceDir } : {}),
1706+
});
16991707
const modelRegistry =
17001708
options?.modelRegistry ??
17011709
emptyDiscoveryStores?.modelRegistry ??

‎src/agents/model-catalog.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -791,10 +791,11 @@ export async function loadModelCatalogSnapshot(
791791
logStage("agent-discovery-imported");
792792
const { buildShouldSuppressBuiltInModel } = await loadModelSuppression();
793793
logStage("catalog-deps-ready");
794-
const authStorage = agentDiscovery.discoverAuthStorage(
795-
agentDir,
796-
readOnly ? { readOnly: true } : undefined,
797-
);
794+
const authStorage = agentDiscovery.discoverAuthStorage(agentDir, {
795+
...(readOnly ? { readOnly: true } : {}),
796+
config: cfg,
797+
workspaceDir,
798+
});
798799
logStage("auth-storage-ready");
799800
const registry = agentDiscovery.discoverModels(authStorage, agentDir, {
800801
config: cfg,

‎src/agents/tools/pdf-tool.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,10 @@ async function runPdfPrompt(params: {
162162

163163
const modelsOptions = params.workspaceDir ? { workspaceDir: params.workspaceDir } : undefined;
164164
await ensureOpenClawModelsJson(effectiveCfg, params.agentDir, modelsOptions);
165-
const authStorage = discoverAuthStorage(params.agentDir);
165+
const authStorage = discoverAuthStorage(params.agentDir, {
166+
config: effectiveCfg,
167+
...(params.workspaceDir ? { workspaceDir: params.workspaceDir } : {}),
168+
});
166169
const modelRegistry = discoverModels(authStorage, params.agentDir, {
167170
config: effectiveCfg,
168171
...modelsOptions,

0 commit comments

Comments
 (0)