fix: support runtime-managed OAuth providers (fix OAuth Codex due to Anthropic no longer supporting usage plans)#273
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Lossless Claw’s model-backed summarizer flow to better support providers whose credentials are resolved at runtime via OAuth/auth-profiles (not static API keys), and adds a configurable per-call summarizer timeout.
Changes:
- Add
summaryTimeoutMsto LCM config resolution and expose it inopenclaw.plugin.json. - Introduce
deps.isRuntimeManagedAuthProviderand use it in the summarizer path to skip API-key prefetch + skip direct-credential retry for runtime-managed providers. - Wire summarizer logging to report the configured timeout value.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Extends LcmDependencies with an optional runtime-managed-auth predicate. |
| src/summarize.ts | Makes summarizer timeout configurable and adapts auth handling for runtime-managed providers. |
| src/plugin/index.ts | Provides isRuntimeManagedAuthProvider implementation in plugin dependencies. |
| src/db/config.ts | Adds summaryTimeoutMs to resolved LCM config (env + plugin config). |
| openclaw.plugin.json | Exposes summaryTimeoutMs in UI hints + config schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| delegationTimeoutMs: envDelegationTimeoutMs ?? toNumber(pc.delegationTimeoutMs) ?? 120000, | ||
| summaryTimeoutMs: | ||
| parseFiniteInt(env.LCM_SUMMARY_TIMEOUT_MS) | ||
| ?? toNumber(pc.summaryTimeoutMs) ?? 120000, |
There was a problem hiding this comment.
summaryTimeoutMs now defaults to 120000 in resolveLcmConfig, but the summarizer’s documented default is 60_000 (DEFAULT_SUMMARIZER_TIMEOUT_MS). Because deps.config.summaryTimeoutMs will always be set, this silently changes the default summarizer timeout from 60s to 120s for users who don’t configure it, which can increase gateway stall risk. Consider defaulting this to 60_000 (or leaving it undefined unless configured) to preserve previous behavior.
| ?? toNumber(pc.summaryTimeoutMs) ?? 120000, | |
| ?? toNumber(pc.summaryTimeoutMs) ?? 60_000, |
| const runtimeManagedAuth = params.deps.isRuntimeManagedAuthProvider?.(provider, providerApi) === true; | ||
| const apiKey = runtimeManagedAuth | ||
| ? undefined | ||
| : await params.deps.getApiKey(provider, model, lookupOptions); |
There was a problem hiding this comment.
This new runtimeManagedAuth branch changes behavior by skipping getApiKey and passing apiKey: undefined into deps.complete. There are existing summarizer tests, but none cover the isRuntimeManagedAuthProvider === true path (e.g., openai-codex). Add a focused test asserting deps.getApiKey is not called and that the direct-credentials retry path is skipped for runtime-managed providers.
| "summaryTimeoutMs": { | ||
| "label": "Summary Timeout (ms)", | ||
| "help": "Maximum time to wait for a single model-backed LCM summarizer call before timing out" | ||
| }, |
There was a problem hiding this comment.
New config/env surface area was added (summaryTimeoutMs / LCM_SUMMARY_TIMEOUT_MS). Repo convention appears to be to document user-facing config options in README alongside the plugin schema; please update the README configuration section (plugin config example + env var table) to include this new timeout so users can discover it.
| if (normalizedProvider === "openai-codex" || normalizedProvider === "github-copilot") { | ||
| return true; | ||
| } | ||
| return isCodexResponsesApi(providerApi); |
There was a problem hiding this comment.
isCodexResponsesApi is called here but doesn't appear to be defined or imported anywhere in the repo, which will break builds. Either implement this helper locally (e.g., normalize/compare providerApi against the codex responses API id) or replace it with an existing predicate (such as shouldOmitTemperatureForApi(providerApi) if that’s the intended check).
|
Correction / scope-restoration update: I restored the actual runtime-managed OAuth behavior work so this PR is again about Codex OAuth support in the summarizer path, not just the timeout/docs add-on. What's now in the branch again
Additional fix includedThe previously flaky registration test was updated with an explicit timeout so the suite no longer fails on the same 5s timing issue. ValidationRe-ran the test suite after restoring the OAuth behavior patch and the focused tests. This should now better reflect the real goal of the PR: making runtime-managed OAuth-backed summarizer providers (especially |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| isRuntimeManagedAuthProvider: (provider: string, providerApi?: string) => { | ||
| const normalizedProvider = normalizeProviderId(provider); | ||
| if (normalizedProvider === "openai-codex" || normalizedProvider === "github-copilot") { | ||
| return true; | ||
| } | ||
| return isCodexResponsesApi(providerApi); | ||
| }, |
There was a problem hiding this comment.
isRuntimeManagedAuthProvider references isCodexResponsesApi(providerApi), but isCodexResponsesApi is not defined or imported in this module (search shows only this callsite). This will fail to compile at runtime/tsc. Define this helper (e.g., based on known Codex Responses API ids) or replace the call with an existing predicate in this file.
|
Fresh-pass root-cause update after adversarial testing: What we now know with high confidenceThe remaining issue is not that Lossless Claw needs to bypass the plugin bridge or call the gateway manually. The plugin bridge in
So the bridge contract for Codex OAuth is:
—not—
Why this matters for the PRThe restored summarizer patch was directionally correct in trying to avoid the wrong fallback path for runtime-managed providers, but the first bridge-level interpretation was too blunt. Adversarial bridge testing showed that a naive “skip credential resolution for runtime-managed providers” approach breaks existing Correct patch direction from hereKeep
Change
Important scope clarificationOur earlier live logs proved that LCM/provider routing is path-sensitive and that OAuth/API paths behave very differently (MiniMax portal 404s vs API-key timeouts/successes), but those logs did not directly prove the Codex OAuth seam by themselves. The Codex-specific diagnosis here comes from:
Patch plan
Practical implicationThis PR should still be about Codex OAuth support, but the fix needs to be centered on the summarizer retry policy, not a broad plugin-bridge bypass. |
|
Fresh update after the focused auth-seam debugging pass: Root cause resolved more preciselyThe correct contract is:
In other words:
Why this is the right fixThe plugin bridge already has explicit The updated patch now preserves the bridge/auth resolution path and narrows the behavior change to the summarizer retry policy. Focused validationRan the relevant focused suite: npm test -- test/summarize.test.ts test/index-complete-model-auth.test.ts test/index-complete-provider-config.test.ts test/lcm-expand-query-tool.test.tsResult:
This includes:
Practical statusThis PR now much more accurately reflects the intended Codex OAuth support work:
|
|
Additional follow-up: expansion override coverage has now been strengthened as well. What was added
Focused validationRan: npm test -- test/lcm-expand-query-tool.test.ts test/summarize.test.ts test/index-complete-model-auth.test.ts test/index-complete-provider-config.test.tsResult:
So the focused matrix now covers:
This makes the PR much closer to a full OAuth-support update for both:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const initialAuthError = new LcmProviderAuthError({ provider, model, failure }); | ||
| const runtimeManagedAuth = params.deps.isRuntimeManagedAuthProvider?.(provider, providerApi) === true; | ||
| if (runtimeManagedAuth) { | ||
| throw initialAuthError; | ||
| } | ||
| console.warn(initialAuthError.message); |
There was a problem hiding this comment.
The new runtimeManagedAuth handling only skips the direct-credential retry path, but the initial call still performs deps.getApiKey(...) and passes that value through to deps.complete(...). If the intent is to avoid direct API-key prefetch for runtime-managed OAuth providers, consider bypassing the initial getApiKey lookup (and always passing apiKey: undefined) when isRuntimeManagedAuthProvider(provider, providerApi) is true.
| it("skips direct api-key lookup and direct-credential retry for runtime-managed auth providers", async () => { | ||
| const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}); | ||
| try { | ||
| const deps = makeDeps({ | ||
| config: { summaryTimeoutMs: 60_000 }, | ||
| resolveModel: vi.fn(() => ({ | ||
| provider: "openai-codex", | ||
| model: "gpt-5.4", | ||
| })), | ||
| getApiKey: vi.fn(async () => "should-not-be-used"), | ||
| isRuntimeManagedAuthProvider: vi.fn(() => true), | ||
| complete: vi.fn(async () => ({ | ||
| content: [], | ||
| error: { | ||
| kind: "provider_auth", | ||
| statusCode: 401, | ||
| message: "Missing required scope: model.request", | ||
| }, | ||
| })), | ||
| }); | ||
|
|
||
| const result = await createLcmSummarizeFromLegacyParams({ | ||
| deps, | ||
| legacyParams: { provider: "openai-codex", model: "gpt-5.4" }, | ||
| }); | ||
|
|
||
| await expect(result!.fn("R".repeat(8_000), false)).rejects.toBeInstanceOf( | ||
| LcmProviderAuthError, | ||
| ); | ||
| expect(vi.mocked(deps.getApiKey)).toHaveBeenCalledTimes(1); | ||
| expect(vi.mocked(deps.getApiKey)).toHaveBeenCalledWith("openai-codex", "gpt-5.4", { | ||
| profileId: undefined, | ||
| agentDir: undefined, | ||
| runtimeConfig: undefined, | ||
| }); |
There was a problem hiding this comment.
This test name says it "skips direct api-key lookup", but it asserts deps.getApiKey is called once and the mock even returns a sentinel value. Either update the behavior under test to truly bypass API-key lookup for runtime-managed providers, or rename/adjust the assertions so the test matches the actual intended behavior (e.g. only verifying the direct-credential retry is skipped).
|
Final coverage pass update: I re-read the configuration/docs and scanned the remaining model-calling surfaces to make sure we hadn't missed anything beyond:
The one additional model-backed surface worth explicitly covering was:
That path reuses the same summarizer creation logic, but it did not have explicit Codex/OAuth-oriented coverage before. Added
Focused validation rerunnpm test -- test/engine.test.ts test/summarize.test.ts test/lcm-expand-query-tool.test.ts test/index-complete-model-auth.test.ts test/index-complete-provider-config.test.tsResult:
Net effectThe focused OAuth-support matrix now explicitly covers all of the meaningful model-calling surfaces I could find in the plugin:
That makes this PR much stronger as a full OAuth-support update rather than a narrow fix. |
|
Follow-up on the review points around runtime-managed auth behavior: Test wording cleanupOne review point correctly noted that the earlier test name implied “skip all API-key lookup,” while the actual implementation preserves first-pass credential resolution. I pushed a follow-up rename so the test now reflects the intended contract more precisely:
Why we are not implementing “always pass apiKey: undefined”After a focused debugging pass, I’m now highly confident the bridge contract here is:
This conclusion is grounded in:
Focused verificationnpm test -- test/summarize.test.ts test/index-complete-model-auth.test.ts test/index-secret-ref-auth-profiles.test.ts test/index-complete-provider-config.test.tsResult:
So the current branch keeps the bridge’s Codex OAuth credential resolution intact while narrowing the behavior change to the summarizer retry policy. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| isRuntimeManagedAuthProvider: (provider: string, providerApi?: string) => { | ||
| const normalizedProvider = normalizeProviderId(provider); | ||
| if (normalizedProvider === "openai-codex" || normalizedProvider === "github-copilot") { | ||
| return true; | ||
| } | ||
| return isCodexResponsesApi(providerApi); |
There was a problem hiding this comment.
isRuntimeManagedAuthProvider calls isCodexResponsesApi(providerApi), but isCodexResponsesApi is not defined or imported anywhere in this module. If this function is invoked for providers other than the explicit openai-codex/github-copilot fast path, it will throw a ReferenceError at runtime.
Define this helper locally (or reuse an existing API-normalization predicate, e.g. comparing providerApi to openai-codex-responses) and/or import it from the appropriate module so the predicate is always safe to call.
|
Responding to the remaining review concern around provider scope: This is a fair observation: the current special-case treatment is intentionally scoped to the runtime-managed provider family we can identify confidently in the current codebase ( That scoping is deliberate. Why the PR is targeted instead of fully genericThis PR is fixing a concrete compatibility gap that showed up when switching LCM summarization onto Codex OAuth. Anthropic already worked through the existing summarizer path because it follows a simpler/normal provider auth flow in this stack. Codex is different in this codebase:
So the purpose of this PR is to make the summarizer logic cooperate correctly with that existing Codex OAuth bridge path, not to redefine all OAuth-provider semantics across the plugin in one shot. Why we are not implementing “always pass apiKey: undefined”I tested that direction during the debugging pass and it turned out to be the wrong abstraction for this codebase. The bridge already expects to resolve Codex OAuth into the effective credential/token used by Verification behind this conclusionFocused verification runs covered:
Most relevant focused auth/bridge verification: npm test -- test/summarize.test.ts test/index-complete-model-auth.test.ts test/index-secret-ref-auth-profiles.test.ts test/index-complete-provider-config.test.tsResult:
Expanded focused matrix: npm test -- test/engine.test.ts test/summarize.test.ts test/lcm-expand-query-tool.test.ts test/index-complete-model-auth.test.ts test/index-complete-provider-config.test.tsResult:
Future directionIf maintainers want a generic abstraction for “runtime-managed OAuth providers”, I agree that would be useful — but it likely belongs in a follow-up change with a runtime/provider capability flag rather than more hardcoded provider-family heuristics. For this PR, the narrower scope is intentional: fix the concrete Codex OAuth compatibility gap without broadening auth behavior in ways that could regress other providers. |
|
Final cleanup note: I found and fixed one remaining legitimate review item in the branch before considering it ready for internal application:
Verification rerunnpm test -- test/summarize.test.ts test/index-complete-model-auth.test.ts test/index-secret-ref-auth-profiles.test.ts test/index-complete-provider-config.test.ts test/lcm-expand-query-tool.test.ts test/engine.test.tsResult:
So at this point the legitimate review comments have been addressed, and the focused model/auth/expansion/large-file matrix is green. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thank you! |
When the user has a ChatGPT Plus/Pro subscription (no raw OPENAI_API_KEY but a populated ~/.codex/auth.json), route openai-codex summarization through the local `codex` CLI so it can use its stored OAuth credentials. Mirrors the existing Anthropic OAuth delegate pattern (PR Martian-Engineering#58). Changes in tui/repair.go: - Hoist cliSummarizationSystemPrompt constant, reused by both CLI delegates. - Add hasCodexOAuth() that probes ~/.codex/auth.json presence. - Add summarizeViaCodexCLI() that runs `codex exec` with a restrictive sandbox (--sandbox read-only), --ephemeral, --output-last-message, and streams the prompt on stdin. The system directive is prepended to the stdin payload because codex has no --system-prompt flag. - Add filteredOpenAIChildEnv() that strips every OPENAI_* env var from the child environment, preventing OPENAI_BASE_URL or proxy vars from re-routing OAuth traffic to an unintended endpoint. - Gate in summarize(): allow an empty apiKey when provider is openai-codex and hasCodexOAuth() returns true. - Branch in summarizeOpenAI(): delegate to summarizeViaCodexCLI() when the Codex OAuth path applies. - resolveProviderAPIKey(): when Codex OAuth is available, return an empty string with nil error so the caller can route to the CLI delegate instead of failing hard. When OAuth is absent, the error now hints at `codex login`. Tests in tui/codex_oauth_test.go cover: - hasCodexOAuth: file absent, empty, present, directory. - OAuth path delegates to CLI (HTTP transport never called). - API-key path still uses direct HTTP (regression). - Empty key without OAuth still errors. - resolveProviderAPIKey returns empty sentinel under OAuth; hints at `codex login` without OAuth. - CLI missing from PATH yields an actionable error. - Non-zero exit surfaces stderr. - Empty and oversized CLI output are rejected. - filteredOpenAIChildEnv removes every OPENAI_* var and preserves unrelated env. Closes Martian-Engineering#469 Refs Martian-Engineering#58 (Anthropic delegate pattern), Martian-Engineering#273 (plugin-side Codex OAuth).
* feat(tui): support Codex OAuth via codex CLI delegate When the user has a ChatGPT Plus/Pro subscription (no raw OPENAI_API_KEY but a populated ~/.codex/auth.json), route openai-codex summarization through the local `codex` CLI so it can use its stored OAuth credentials. Mirrors the existing Anthropic OAuth delegate pattern (PR #58). Changes in tui/repair.go: - Hoist cliSummarizationSystemPrompt constant, reused by both CLI delegates. - Add hasCodexOAuth() that probes ~/.codex/auth.json presence. - Add summarizeViaCodexCLI() that runs `codex exec` with a restrictive sandbox (--sandbox read-only), --ephemeral, --output-last-message, and streams the prompt on stdin. The system directive is prepended to the stdin payload because codex has no --system-prompt flag. - Add filteredOpenAIChildEnv() that strips every OPENAI_* env var from the child environment, preventing OPENAI_BASE_URL or proxy vars from re-routing OAuth traffic to an unintended endpoint. - Gate in summarize(): allow an empty apiKey when provider is openai-codex and hasCodexOAuth() returns true. - Branch in summarizeOpenAI(): delegate to summarizeViaCodexCLI() when the Codex OAuth path applies. - resolveProviderAPIKey(): when Codex OAuth is available, return an empty string with nil error so the caller can route to the CLI delegate instead of failing hard. When OAuth is absent, the error now hints at `codex login`. Tests in tui/codex_oauth_test.go cover: - hasCodexOAuth: file absent, empty, present, directory. - OAuth path delegates to CLI (HTTP transport never called). - API-key path still uses direct HTTP (regression). - Empty key without OAuth still errors. - resolveProviderAPIKey returns empty sentinel under OAuth; hints at `codex login` without OAuth. - CLI missing from PATH yields an actionable error. - Non-zero exit surfaces stderr. - Empty and oversized CLI output are rejected. - filteredOpenAIChildEnv removes every OPENAI_* var and preserves unrelated env. Closes #469 Refs #58 (Anthropic delegate pattern), #273 (plugin-side Codex OAuth). * docs: clarify tui codex oauth usage --------- Co-authored-by: Josh Lehman <josh@martian.engineering>
Summary
This patch improves Lossless Claw's model-backed summarizer path so runtime-managed OAuth providers (especially
openai-codex) can be used more cleanly for LCM summarization without assuming a direct static API key.It also adds a configurable
summaryTimeoutMsfor slower/larger summarization loads.What changed
summaryTimeoutMsto LCM config + plugin schemaisRuntimeManagedAuthProviderto injected LCM dependenciesdeps.complete(...)runtime pathWhy
Lossless Claw already uses OpenClaw's runtime completion path (
deps.complete) rather than manual gateway HTTP calls. The gap is in auth/credential handling for providers whose auth is runtime-managed OAuth rather than a normal static key.This especially matters for:
summaryModel/summaryProvideropenai-codex/gpt-5.4Notes
This PR is intentionally surgical and focused on the summarizer path first.
Expansion-model support likely deserves the same treatment next / in follow-up if maintainers want that included here.
Validation
npm testtest/plugin-config-registration.test.ts:inherits OpenClaw's default model for summarization when no LCM model override is setRelated