fix(agents): prefer ordered eligible credentials in discovery map#110542
fix(agents): prefer ordered eligible credentials in discovery map#110542wuqxuan wants to merge 2 commits into
Conversation
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs maintainer review before merge. Reviewed July 18, 2026, 5:41 AM ET / 09:41 UTC. Summary PR surface: Source +38, Tests +146. Total +184 across 3 files. Reproducibility: yes. in source: current main retains the first convertible same-provider profile during raw store iteration, while the shared resolver already supplies the expected ordered eligible candidates. The contributor also supplied a redacted after-fix runtime run against a temporary SQLite auth store. Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Merge the discovery bridge only after ordinary owner review confirms that background callers should share the canonical auth-profile ordering policy, preserving the focused regressions for OAuth expiry and read-only SecretRef handling. Do we have a high-confidence way to reproduce the issue? Yes in source: current main retains the first convertible same-provider profile during raw store iteration, while the shared resolver already supplies the expected ordered eligible candidates. The contributor also supplied a redacted after-fix runtime run against a temporary SQLite auth store. Is this the best way to solve the issue? Yes. Reusing AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 39528edd745a. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +38, Tests +146. Total +184 across 3 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
@clawsweeper re-review Strengthened the SecretRef read-only regression coverage: ordered discovery now asserts the configured |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
1 participant
Fixes #110488
What Problem This Solves
Fixes an issue where automatic/background credential discovery would pick the first same-provider auth profile in raw store insertion order, ignoring
auth.order, OAuth-over-api_key preference, and OAuth expiry. That caused ChatGPT-mode background paths (for example automatic compaction) to fail with unusable credentials while the same session still worked through profile-aware interactive resolution.Why This Change Was Made
Background discovery now builds its one-credential-per-provider map through the same canonical
resolveAuthProfileOrderranking used for interactive profile selection, and OAuth conversion rejects expired access tokens the same way tokens already do. Discovery still does not prompt for secrets or persist resolved credentials; session-specificauthProfileOverrideplumbing for every background caller remains a follow-up if needed beyond global ordering.Compatibility: no config/schema changes; reuses existing order/eligibility helpers. Performance: one order resolve per provider present in the store. Usability: automatic paths fail less often when a usable OAuth profile exists after an earlier api_key. Security: expired OAuth is no longer surfaced as a usable discovery credential.
User Impact
Automatic compaction and other non-interactive model calls for a provider should select the same preferred eligible credential as interactive resolution when profiles are ordered by type or
auth.order, instead of silently binding the first raw store entry.Evidence
Verification host: local macOS (focused Vitest; isolated state dir for discovery runtime proof).
Coverage includes: OAuth preferred over earlier same-provider api_key;
auth.orderhonored; expired OAuth skipped with fallthrough; configured SecretRef markers kept under ordered read-only discovery (openclaw-secret-ref-configured); keyRef-only providers visible todiscoverAuthStorage({ readOnly: true })and omitted at runtime.Real behavior proof
auth.order) over raw store insertion order; expired OAuth is not converted as usable; read-only discovery keeps configured SecretRef markers after routing throughresolveAuthProfileOrderwithreadinessMode: "read-only".fbd95eaf28; focused Vitest agents lane; isolatedOPENCLAW_STATE_DIRfor a realdiscoverAuthStoragepath against a temporary agent SQLite auth store (no live cloud secrets).expires > 0still converted; this patch orders via the shared resolver and rejects expired OAuth.node scripts/run-vitest.mjs src/agents/agent-model-discovery.auth.test.ts(10/10 pass), including SecretRef marker + ordered keyRef-only cases.fixture-refkeyRef-only profile; callresolveAgentCredentialMapFromStoreanddiscoverAuthStoragewith/withoutreadOnly: true.fbd95eaf28):{ "head": "fbd95eaf28", "orderedDiscoveryMap": { "fixture-ref": { "type": "api_key", "key": "openclaw-secret-ref-configured" }, "openai": { "type": "oauth", "access": "oauth-access-REDACTED", "refresh": "oauth-refresh-REDACTED" } }, "executionMap": { "openai": { "type": "oauth", "access": "oauth-access-REDACTED", "refresh": "oauth-refresh-REDACTED" } }, "readOnlyHasAuth": { "openai": true, "fixture-ref": true }, "runtimeHasAuth": { "openai": true, "fixture-ref": false } }authProfileOverridethreading through every background caller.This PR was authored with AI assistance (Grok).