feat(models): add MiniMax OAuth provider flow #549
Merged
Conversation
Deploying nexu-docs with
|
| Latest commit: |
69ecd38
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://deb012d6.nexu-docs.pages.dev |
| Branch Preview URL: | https://feat-minimax-coding-plan.nexu-docs.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MiniMax OAuth endpoints and UI flows; extends provider schemas and stored provider records with authMode/oauth fields; removes the legacy "custom" provider flow; wires OAuth lifecycle into ModelProviderService, config store, auth-profile generation, desktop IPC, SDK/types, OpenClaw sync, and runtime restart handling. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Desktop as Desktop UI
participant Controller as Controller API
participant MiniMax as MiniMax OAuth
participant Store as Config Store
participant Sync as OpenClaw Sync
participant Runtime as OpenClaw Runtime
User->>Desktop: Click "Connect MiniMax" (region)
Desktop->>Controller: POST /api/v1/providers/minimax/oauth/login { region }
Controller->>MiniMax: Create auth request (PKCE/state) → returns browserUrl
Controller-->>Desktop: 200 { browserUrl, started: true }
Desktop->>MiniMax: User authorizes in browser
Note over Controller,MiniMax: Controller polls token endpoint until token received or aborted
MiniMax-->>Controller: Returns access token & metadata
Controller->>Store: setProviderOauthCredentials(minimax, oauthCredential)
Store-->>Controller: Persisted provider record
Controller->>Sync: openclawSyncService.syncAll()
Sync->>Runtime: write auth-profiles (derived api_key + oauth profile)
Controller->>Runtime: restart runtime if required
Desktop-->>User: Show connected status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69ecd38953
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared/src/schemas/provider.ts (1)
28-34:⚠️ Potential issue | 🟠 MajorDon't expose OAuth on every provider update contract.
upsertProviderBodySchemabacks the generic/providers/{providerId}update route, so this makes the SDK/OpenAPI advertiseauthMode: "oauth"for every provider even though this PR only implements MiniMax OAuth. Split out a MiniMax-specific body schema, or add a route-level refinement keyed byproviderId, so unsupported providers cannot accept an impossible auth mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/schemas/provider.ts` around lines 28 - 34, The generic upsertProviderBodySchema currently exposes providerAuthModeSchema (which includes "oauth") for every provider and thus advertises OAuth support on the generic /providers/{providerId} update route; fix by removing OAuth from the generic contract and scoping OAuth to MiniMax-specific updates: create a new minimaxUpsertProviderBodySchema that mirrors upsertProviderBodySchema but allows authMode: providerAuthModeSchema (including "oauth"), keep the existing upsertProviderBodySchema but constrain its authMode to the non-OAuth subset (or make authMode optional without "oauth"), and update the route handlers to use minimaxUpsertProviderBodySchema only for the MiniMax providerId (or add a route-level refinement that validates providerId === "minimax" before accepting authMode "oauth").
🧹 Nitpick comments (5)
apps/controller/src/store/nexu-config-store.ts (1)
977-980: Redundant OAuth field clearing.Lines 952-955 already conditionally set
oauthRegionandoauthCredentialtonullwheninput.authMode === "apiKey". The clearing at lines 977-980 duplicates this for the same condition. Consider removing this redundant block.♻️ Proposed fix
- if (nextProvider.authMode === "apiKey") { - nextProvider.oauthRegion = null; - nextProvider.oauthCredential = null; - } - created = existing === undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/controller/src/store/nexu-config-store.ts` around lines 977 - 980, Remove the duplicate OAuth-clearing block by deleting the redundant conditional that sets nextProvider.oauthRegion = null and nextProvider.oauthCredential = null for nextProvider.authMode === "apiKey"; keep the original conditional that handles input.authMode === "apiKey" (which already clears these fields) so OAuth fields are cleared in one place only (refer to nextProvider and input.authMode to locate the duplicates).apps/controller/src/runtime/openclaw-auth-profiles-writer.ts (1)
195-213: Verify necessity of dual auth profile writes.This writes auth profiles to both the workspace path and
~/.openclaw/agents/{agentId}/agent/auth-profiles.json. The conditiondefaultUserAgentAuthProfilesPath !== authProfilesPathwill almost always be true since they're derived from different base paths.Is the dual write intentional to support different OpenClaw resolution paths? If so, consider adding a brief comment explaining why both locations are needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/controller/src/runtime/openclaw-auth-profiles-writer.ts` around lines 195 - 213, The code is writing auth profiles to two locations via defaultUserAgentAuthProfilesPath and authProfilesPath and the condition defaultUserAgentAuthProfilesPath !== authProfilesPath will almost always be true; either add a short clarifying comment above the block explaining that the dual write to ~/.openclaw/agents/{agent.id}/agent/auth-profiles.json is intentional to support alternate OpenClaw resolution paths (referencing defaultUserAgentAuthProfilesPath, authProfilesPath, and agent.id), or if the duplicate write is not required remove the mkdir/writeFile block (the mkdir, writeFile calls and the defaultUserAgentAuthProfilesPath variable) so we only write to authProfilesPath.apps/controller/src/services/model-provider-service.ts (2)
766-766: Unbounded poll interval growth could delay final attempts.The interval grows via
pollIntervalMs * 1.5up to 10 seconds. While capped, if the user completes authorization just before expiry, the long interval could cause a missed window. Consider whether a tighter cap (e.g., 5 seconds) would improve UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/controller/src/services/model-provider-service.ts` at line 766, The poll interval growth currently caps at 10000ms causing potentially long waits; change the cap to a tighter value (e.g., 5000ms) or make it configurable to improve UX by replacing the hardcoded 10000 with a new constant or config (e.g., POLL_INTERVAL_MAX_MS = 5000) where pollIntervalMs is updated (the line using pollIntervalMs = Math.min(pollIntervalMs * 1.5, 10000)); update any related docs/comments and tests to reflect the new cap or config parameter.
174-235: Consider simplifying OpenClaw command resolution.The
getOpenClawCommandSpecfunction has complex path resolution logic with multiple fallbacks (Electron executable, workspace root detection, various entry points). While functional, this could be fragile if directory structures change.Consider documenting the expected directory layouts or consolidating the resolution paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/controller/src/services/model-provider-service.ts` around lines 174 - 235, getOpenClawCommandSpec: simplify and harden OpenClaw resolution by consolidating fallbacks and adding clear expectations: update getOpenClawCommandSpec to first check OPENCLAW_ELECTRON_EXECUTABLE and return electronExec with ELECTRON_RUN_AS_NODE, then if env.openclawBin appears to be a path (absolute or contains path.sep) return it directly; if not, attempt workspace-based resolution by computing workspaceRoot via NEXU_WORKSPACE_ROOT or findWorkspaceRoot(process.cwd()) and then check for a single canonical runtime entry (runtimeEntryPath) and a single canonical wrapper (wrapperPath) in a clear priority order (runtimeEntryPath -> wrapperPath), and finally fall back to returning env.openclawBin; document expected directory layout for runtimeEntryPath and wrapperPath in a short comment inside the function and remove any duplicate/fragile path constructions to keep paths deterministic.apps/web/src/pages/models.tsx (1)
119-142: Host bridge helper duplicates pattern from desktop-links.ts.This helper duplicates the pattern from
apps/web/src/lib/desktop-links.ts. Consider extracting a sharedgetHostInvokeBridgeutility to avoid duplication, though this is a minor refactor opportunity.♻️ Optional: Extract shared host bridge helper
The
getModelsHostInvokeBridgecould be consolidated with the similar pattern indesktop-links.tsinto a shared utility inapps/web/src/lib/that returns a properly typed bridge based on the channels needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/pages/models.tsx` around lines 119 - 142, The function getModelsHostInvokeBridge duplicates the host-invoke detection logic; extract a shared utility (e.g. getHostInvokeBridge) that encapsulates the window check, candidate retrieval, Reflect.get("invoke") validation, and typed invoke wrapper, then replace getModelsHostInvokeBridge with a thin wrapper that calls getHostInvokeBridge and narrows the returned bridge to ModelsHostInvokeBridge (preserving the same invoke signature); update the other duplicating implementation (desktop-links' equivalent) to reuse getHostInvokeBridge as well so both callers share the single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/controller/src/lib/byok-providers.ts`:
- Around line 1-11: The allowlist supportedByokProviderIds is missing "moonshot"
and "zai", which breaks validation (z.enum(supportedByokProviderIds)) and
runtime checks (isSupportedByokProviderId()) while those providers remain
configured elsewhere (model-provider-service.ts, openclaw-config-compiler.ts,
apps/web pages). Fix by adding "moonshot" and "zai" to the exported
supportedByokProviderIds const so the enum and checks accept them, or
alternatively implement a migration in model-provider-service (or config
compiler) to map existing "moonshot" -> "kimi" and "zai" -> "glm" before
validation; update references to isSupportedByokProviderId() and any validation
sites to use the chosen approach so existing configs are preserved.
In `@apps/controller/src/routes/model-routes.ts`:
- Around line 18-20: providerIdParamSchema was tightened to
z.enum(supportedByokProviderIds) removing "custom", so update the tests that
still expect providerId="custom" (tests named openclaw-config-compiler.test and
openclaw-auth-service.test) to use one of the supported provider IDs from
supportedByokProviderIds or adjust the tests to expect a 422 validation error;
locate occurrences of "custom" in those test files and replace with a valid
provider constant or change assertions accordingly to match
providerIdParamSchema behavior.
In `@apps/controller/src/services/model-provider-service.ts`:
- Around line 571-586: The code passes OAuth's expired_in (a relative duration
in seconds) directly to pollMiniMaxOAuthToken causing an immediate timeout;
update finishMiniMaxOauthLogin to convert auth.expired_in into an absolute
expiration timestamp in milliseconds (e.g., Date.now() + auth.expired_in * 1000)
before passing it as the expiresAt argument so pollMiniMaxOAuthToken receives a
proper epoch-ms expiry value.
In `@apps/desktop/src/components/surface-frame.tsx`:
- Around line 97-98: Replace the `void title; void description;` pattern by
renaming the unused props in the SurfaceFrame component signature to use an
underscore prefix (e.g. _title and _description) and remove those void
statements; update any references accordingly so the props are clearly marked
unused (or optionally used for aria-label) while avoiding the void statements.
In `@packages/shared/src/schemas/provider.ts`:
- Around line 73-75: The minimaxOauthStartBodySchema currently allows region:
"cn" but downstream code (the minimax sync path used in
openclaw-config-compiler.ts) is hard-coded to the global endpoint; update
minimaxOauthStartBodySchema (which uses minimaxOauthRegionSchema) to prevent
acceptance of region === "cn" until a region-aware sync path is implemented,
e.g., add a schema refinement that throws a validation error when region ===
"cn" (or alternately require an explicit region-specific syncPath field and
validate it) so CN OAuth logins cannot proceed with the wrong sync endpoint.
---
Outside diff comments:
In `@packages/shared/src/schemas/provider.ts`:
- Around line 28-34: The generic upsertProviderBodySchema currently exposes
providerAuthModeSchema (which includes "oauth") for every provider and thus
advertises OAuth support on the generic /providers/{providerId} update route;
fix by removing OAuth from the generic contract and scoping OAuth to
MiniMax-specific updates: create a new minimaxUpsertProviderBodySchema that
mirrors upsertProviderBodySchema but allows authMode: providerAuthModeSchema
(including "oauth"), keep the existing upsertProviderBodySchema but constrain
its authMode to the non-OAuth subset (or make authMode optional without
"oauth"), and update the route handlers to use minimaxUpsertProviderBodySchema
only for the MiniMax providerId (or add a route-level refinement that validates
providerId === "minimax" before accepting authMode "oauth").
---
Nitpick comments:
In `@apps/controller/src/runtime/openclaw-auth-profiles-writer.ts`:
- Around line 195-213: The code is writing auth profiles to two locations via
defaultUserAgentAuthProfilesPath and authProfilesPath and the condition
defaultUserAgentAuthProfilesPath !== authProfilesPath will almost always be
true; either add a short clarifying comment above the block explaining that the
dual write to ~/.openclaw/agents/{agent.id}/agent/auth-profiles.json is
intentional to support alternate OpenClaw resolution paths (referencing
defaultUserAgentAuthProfilesPath, authProfilesPath, and agent.id), or if the
duplicate write is not required remove the mkdir/writeFile block (the mkdir,
writeFile calls and the defaultUserAgentAuthProfilesPath variable) so we only
write to authProfilesPath.
In `@apps/controller/src/services/model-provider-service.ts`:
- Line 766: The poll interval growth currently caps at 10000ms causing
potentially long waits; change the cap to a tighter value (e.g., 5000ms) or make
it configurable to improve UX by replacing the hardcoded 10000 with a new
constant or config (e.g., POLL_INTERVAL_MAX_MS = 5000) where pollIntervalMs is
updated (the line using pollIntervalMs = Math.min(pollIntervalMs * 1.5, 10000));
update any related docs/comments and tests to reflect the new cap or config
parameter.
- Around line 174-235: getOpenClawCommandSpec: simplify and harden OpenClaw
resolution by consolidating fallbacks and adding clear expectations: update
getOpenClawCommandSpec to first check OPENCLAW_ELECTRON_EXECUTABLE and return
electronExec with ELECTRON_RUN_AS_NODE, then if env.openclawBin appears to be a
path (absolute or contains path.sep) return it directly; if not, attempt
workspace-based resolution by computing workspaceRoot via NEXU_WORKSPACE_ROOT or
findWorkspaceRoot(process.cwd()) and then check for a single canonical runtime
entry (runtimeEntryPath) and a single canonical wrapper (wrapperPath) in a clear
priority order (runtimeEntryPath -> wrapperPath), and finally fall back to
returning env.openclawBin; document expected directory layout for
runtimeEntryPath and wrapperPath in a short comment inside the function and
remove any duplicate/fragile path constructions to keep paths deterministic.
In `@apps/controller/src/store/nexu-config-store.ts`:
- Around line 977-980: Remove the duplicate OAuth-clearing block by deleting the
redundant conditional that sets nextProvider.oauthRegion = null and
nextProvider.oauthCredential = null for nextProvider.authMode === "apiKey"; keep
the original conditional that handles input.authMode === "apiKey" (which already
clears these fields) so OAuth fields are cleared in one place only (refer to
nextProvider and input.authMode to locate the duplicates).
In `@apps/web/src/pages/models.tsx`:
- Around line 119-142: The function getModelsHostInvokeBridge duplicates the
host-invoke detection logic; extract a shared utility (e.g. getHostInvokeBridge)
that encapsulates the window check, candidate retrieval, Reflect.get("invoke")
validation, and typed invoke wrapper, then replace getModelsHostInvokeBridge
with a thin wrapper that calls getHostInvokeBridge and narrows the returned
bridge to ModelsHostInvokeBridge (preserving the same invoke signature); update
the other duplicating implementation (desktop-links' equivalent) to reuse
getHostInvokeBridge as well so both callers share the single implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32884e05-63ad-4f57-8b06-c2489ab4f55d
⛔ Files ignored due to path filters (2)
apps/controller/static/runtime-plugins/openclaw-weixin/package-lock.jsonis excluded by!**/package-lock.jsonopenclaw-runtime/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
apps/controller/openapi.jsonapps/controller/src/app/container.tsapps/controller/src/lib/byok-providers.tsapps/controller/src/lib/openclaw-config-compiler.tsapps/controller/src/routes/model-routes.tsapps/controller/src/runtime/openclaw-auth-profiles-writer.tsapps/controller/src/services/model-provider-service.tsapps/controller/src/services/openclaw-sync-service.tsapps/controller/src/store/nexu-config-store.tsapps/controller/src/store/schemas.tsapps/controller/static/runtime-plugins/nexu-runtime-model/index.jsapps/controller/tests/openclaw-config-compiler.test.tsapps/desktop/main/ipc.tsapps/desktop/shared/host.tsapps/desktop/src/components/surface-frame.tsxapps/desktop/src/lib/host-api.tsapps/web/lib/api/sdk.gen.tsapps/web/lib/api/types.gen.tsapps/web/src/components/model-picker-dropdown.tsxapps/web/src/i18n/locales/en.tsapps/web/src/i18n/locales/zh-CN.tsapps/web/src/lib/desktop-links.tsapps/web/src/pages/home.tsxapps/web/src/pages/models.tsxapps/web/src/pages/welcome.tsxdocs/en/guide/models.mdpackages/shared/src/schemas/provider.tstests/desktop/nexu-runtime-model-plugin.test.ts
💤 Files with no reviewable changes (4)
- docs/en/guide/models.md
- apps/controller/static/runtime-plugins/nexu-runtime-model/index.js
- tests/desktop/nexu-runtime-model-plugin.test.ts
- apps/web/src/pages/welcome.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd53715b86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/controller/src/services/model-provider-service.ts (3)
738-741: Consider wrapping JSON.parse for clearer error messages.
JSON.parse(text)will throw a generic syntax error if the server returns malformed JSON (e.g., HTML error page). While the outer try-catch infinishMiniMaxOauthLoginhandles this, wrapping the parse would provide a more descriptive error.♻️ Suggested improvement
const text = await response.text(); - const payload = - text.length > 0 ? (JSON.parse(text) as Record<string, unknown>) : {}; + let payload: Record<string, unknown> = {}; + if (text.length > 0) { + try { + payload = JSON.parse(text) as Record<string, unknown>; + } catch { + throw new Error("MiniMax OAuth returned invalid JSON response"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/controller/src/services/model-provider-service.ts` around lines 738 - 741, Wrap the JSON.parse call that builds payload in a small try-catch to provide a clearer error message that includes the raw response body; inside finishMiniMaxOauthLogin (where const text = await response.text() and const payload = ... JSON.parse(text) ... are used) replace the direct JSON.parse with a guarded parse that catches SyntaxError and throws/returns a new error describing "failed to parse JSON response" and include the text (or attach it) so callers can see the malformed server response.
786-810: Consider adding a timeout to prevent indefinite hangs.
execOpenClawCommandhas no timeout. If the OpenClaw process hangs (e.g., waiting for input or encountering a deadlock), this promise will never resolve, potentially blocking the OAuth flow indefinitely.♻️ Suggested improvement
+const OPENCLAW_COMMAND_TIMEOUT_MS = 30_000; + private async execOpenClawCommand(args: string[]): Promise<void> { const spec = getOpenClawCommandSpec(this.env); await new Promise<void>((resolve, reject) => { - execFile( + const child = execFile( spec.command, [...spec.argsPrefix, ...args], { cwd: this.env.openclawStateDir, env: { ...process.env, ...spec.extraEnv, OPENCLAW_CONFIG_PATH: this.env.openclawConfigPath, OPENCLAW_STATE_DIR: this.env.openclawStateDir, }, + timeout: OPENCLAW_COMMAND_TIMEOUT_MS, }, (error) => { if (error) { reject(error); return; } resolve(); }, ); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/controller/src/services/model-provider-service.ts` around lines 786 - 810, The execOpenClawCommand function can hang because execFile is called without a timeout; modify execOpenClawCommand to enforce a configurable timeout (e.g., from this.env or a constant) and reject when exceeded, ensuring the child process is killed: use execFile's options.timeout or switch to spawn to track the ChildProcess, set a timer that calls child.kill() and rejects with a clear TimeoutError, and clear the timer on normal exit; update any call-sites if you add a timeout parameter and keep references to getOpenClawCommandSpec and execOpenClawCommand when locating the code.
90-95: Order-sensitive comparison may be fragile.
hasSameModelscompares arrays by index, which will returnfalseif the same models appear in different order. This works currently since it compares against constant arrays (MINI_MAX_OAUTH_MODELS), but could cause unexpected refresh loops if the stored model order ever diverges.♻️ Alternative using sets for order-insensitive comparison
function hasSameModels(current: string[], expected: string[]): boolean { - return ( - current.length === expected.length && - current.every((model, index) => model === expected[index]) - ); + if (current.length !== expected.length) { + return false; + } + const expectedSet = new Set(expected); + return current.every((model) => expectedSet.has(model)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/controller/src/services/model-provider-service.ts` around lines 90 - 95, The hasSameModels function is currently order-sensitive and can falsely return false if the same model names are shuffled; update hasSameModels to perform an order-insensitive comparison by comparing either sets or by sorting both arrays before comparison (e.g., convert current and expected to sets and check sizes and membership, or sort and then compare lengths and elements). Change the implementation inside hasSameModels (used when comparing against constants like MINI_MAX_OAUTH_MODELS) so it first normalizes order (or uses a Set) and then compares equality to avoid refresh loops when stored model order differs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/desktop/openclaw-auth-profiles-writer.test.ts`:
- Around line 58-59: The test mock for ControllerEnv is missing the
analyticsStatePath property used by createEnv; update the test env object used
in openclaw-auth-profiles-writer.test (the mock passed to createEnv) to include
analyticsStatePath (e.g., a string path or undefined if allowed) so the mock
conforms to the ControllerEnv shape and satisfies pnpm typecheck.
---
Nitpick comments:
In `@apps/controller/src/services/model-provider-service.ts`:
- Around line 738-741: Wrap the JSON.parse call that builds payload in a small
try-catch to provide a clearer error message that includes the raw response
body; inside finishMiniMaxOauthLogin (where const text = await response.text()
and const payload = ... JSON.parse(text) ... are used) replace the direct
JSON.parse with a guarded parse that catches SyntaxError and throws/returns a
new error describing "failed to parse JSON response" and include the text (or
attach it) so callers can see the malformed server response.
- Around line 786-810: The execOpenClawCommand function can hang because
execFile is called without a timeout; modify execOpenClawCommand to enforce a
configurable timeout (e.g., from this.env or a constant) and reject when
exceeded, ensuring the child process is killed: use execFile's options.timeout
or switch to spawn to track the ChildProcess, set a timer that calls
child.kill() and rejects with a clear TimeoutError, and clear the timer on
normal exit; update any call-sites if you add a timeout parameter and keep
references to getOpenClawCommandSpec and execOpenClawCommand when locating the
code.
- Around line 90-95: The hasSameModels function is currently order-sensitive and
can falsely return false if the same model names are shuffled; update
hasSameModels to perform an order-insensitive comparison by comparing either
sets or by sorting both arrays before comparison (e.g., convert current and
expected to sets and check sizes and membership, or sort and then compare
lengths and elements). Change the implementation inside hasSameModels (used when
comparing against constants like MINI_MAX_OAUTH_MODELS) so it first normalizes
order (or uses a Set) and then compares equality to avoid refresh loops when
stored model order differs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d45c9ea-1b7c-4659-8b39-7b6caafc3f00
📒 Files selected for processing (6)
apps/controller/src/lib/byok-providers.tsapps/controller/src/lib/openclaw-config-compiler.tsapps/controller/src/runtime/openclaw-auth-profiles-writer.tsapps/controller/src/services/model-provider-service.tsapps/controller/tests/openclaw-config-compiler.test.tstests/desktop/openclaw-auth-profiles-writer.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/controller/src/lib/byok-providers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/controller/src/runtime/openclaw-auth-profiles-writer.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41c455416a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lefarcen
approved these changes
Mar 25, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/controller/src/services/model-provider-service.ts (1)
419-435:⚠️ Potential issue | 🟠 MajorDon't mark arbitrary MiniMax
/modelsfailures as valid.Line 420 promotes any 404 to
valid: true, and Lines 433-435 do the same for any 200 response without adataarray. A mistyped or custom base URL can therefore pass verification and only fail later at runtime. Restrict the static-model fallback to the canonical MiniMax hosts, or to some other positive MiniMax-specific probe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/controller/src/services/model-provider-service.ts` around lines 419 - 435, The current verification in model-provider-service.ts incorrectly treats any MiniMax 404 or missing-data 200 as valid; update the validation in the code paths around the fetch response handling (the block that returns { valid: true, models: MINI_MAX_API_MODELS } and the payload.data fallback) to only allow the static MINI_MAX_API_MODELS fallback when the request is actually against a canonical MiniMax host (e.g., inspect response.url or the provider's configured baseUrl for an authorized MiniMax hostname) or after a dedicated MiniMax-specific probe succeeds; otherwise return valid: false with the HTTP error or an explicit invalid-provider error to avoid accepting mistyped/custom endpoints. Ensure the checks are applied both in the !response.ok branch and the payload.data absence branch (the logic that currently maps payload.data or falls back to MINI_MAX_API_MODELS).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/controller/src/services/model-provider-service.ts`:
- Around line 463-517: Concurrent startMiniMaxOauth calls can race when earlier
attempts clear shared fields; fix by tagging each attempt and validating before
any mutation: in startMiniMaxOauth create a unique attempt id (e.g., const
attemptId = Symbol() or timestamp) and store it on the instance
(this.currentMiniMaxAttemptId = attemptId) right after setting
this.miniMaxOauthAbortController, then pass/close-over that id when calling
requestMiniMaxOAuthCode and finishMiniMaxOauthLogin; inside the catch/finally of
startMiniMaxOauth and inside finishMiniMaxOauthLogin (the code referenced around
633-662), check that this.currentMiniMaxAttemptId === attemptId (or that
this.miniMaxOauthAbortController === abortController) before clearing or
updating this.miniMaxOauthAbortController, this.miniMaxOauthBrowserUrl, or
this.miniMaxOauthState so a newer attempt cannot be overwritten by an older one.
- Around line 573-588: In refreshMiniMaxOauthModelsIfNeeded, only backfill
MINI_MAX_OAUTH_MODELS when the provider is not just authMode: "oauth" but also
actually has stored OAuth credentials; after fetching provider via
this.configStore.getProvider("minimax") add a guard that returns unless the
provider contains the expected credential fields (e.g., provider.clientId &&
provider.clientSecret or provider.credentials?.accessToken — match your actual
provider shape), keeping the rest of the logic and the upsertProvider call
unchanged; reference finishMiniMaxOauthLogin for the canonical place where
credentials are saved so the helper only mirrors that successful state.
- Around line 665-686: Add hard timeouts to the new MiniMax/OpenClaw I/O by
combining an AbortSignal.timeout(timeoutMs) with the existing signal for each
external call: wrap the fetch in requestMiniMaxOAuthCode (the POST to
`${getMiniMaxOauthHost(region)}/oauth/code`) with a merged signal (e.g.,
AbortSignal.any or manual race) that enforces a reasonable timeout, do the same
for the fetch in the MiniMax OAuth token polling loop (the polling request that
checks for access_token), and apply a timeout to execOpenClawCommand's execFile
call so the child process is killed if it hangs; update the calls to use the
combined signal (or abort the child) and ensure proper cleanup of timers/abort
controllers.
In `@tests/desktop/model-provider-service.test.ts`:
- Around line 141-155: The test still calls the removed two-argument
ModelProviderService constructor; update the old call sites (the ones using
ModelProviderService(store, env.nodeEnv) at the earlier specs) to the new
four-argument signature used later in this file: pass the NexuConfigStore
instance and the env returned from createEnv, plus a mock openclawSyncService
(implement syncAll: async () => ({ configPushed: false }) or similar) and a mock
openclawProcess (implement stop/start/enableAutoRestart stubs). Mirror the shape
of the mocks used in the existing four-arg instantiation in this file so the
constructor and types align with ModelProviderService.
---
Outside diff comments:
In `@apps/controller/src/services/model-provider-service.ts`:
- Around line 419-435: The current verification in model-provider-service.ts
incorrectly treats any MiniMax 404 or missing-data 200 as valid; update the
validation in the code paths around the fetch response handling (the block that
returns { valid: true, models: MINI_MAX_API_MODELS } and the payload.data
fallback) to only allow the static MINI_MAX_API_MODELS fallback when the request
is actually against a canonical MiniMax host (e.g., inspect response.url or the
provider's configured baseUrl for an authorized MiniMax hostname) or after a
dedicated MiniMax-specific probe succeeds; otherwise return valid: false with
the HTTP error or an explicit invalid-provider error to avoid accepting
mistyped/custom endpoints. Ensure the checks are applied both in the
!response.ok branch and the payload.data absence branch (the logic that
currently maps payload.data or falls back to MINI_MAX_API_MODELS).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 667b8164-75b1-4a77-b1c0-67684ded22ec
📒 Files selected for processing (2)
apps/controller/src/services/model-provider-service.tstests/desktop/model-provider-service.test.ts
lefarcen
reviewed
Mar 25, 2026
Collaborator
lefarcen
left a comment
There was a problem hiding this comment.
Code Review
几个需要修复的问题：
1. home.starnexu 翻译 key 错误 — UI 会显示 raw key
apps/web/src/pages/home.tsx 中 t("home.starNexu") 被改成了 t("home.starnexu")，但 locale 文件里没有对应更新，会导致首页按钮显示未翻译的原始 key。
2. OpenAPI spec 未完全重新生成
supportedByokProviderIds 包含 moonshot 和 zai（共 11 个），但生成的 openapi.json / types.gen.ts 只有 9 个 provider。需要重新 pnpm generate-types。
3. browserUrl 打开前未校验 URL scheme — XSS 风险
models.tsx 中 window.open(browserUrl, "_blank", "noopener,noreferrer") 的 browserUrl 来自 MiniMax API 响应。如果被篡改为 javascript: URL 会产生 XSS。建议加个前缀校验：
if (browserUrl && /^https?:\/\//.test(browserUrl)) {
window.open(browserUrl, "_blank", "noopener,noreferrer");
}4. custom provider 移除无迁移路径
现有配置了 custom provider 的用户升级后会被静默过滤掉，无警告。需要考虑迁移策略或至少 log 一条 warning。
5. API title 意外改名
openapi.json 中 "Nexu Controller API" → "nexu Controller API"，看起来是无意的。
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e39b2ee52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/pages/models.tsx (1)
415-427:⚠️ Potential issue | 🟠 MajorKeep
moonshotandzaireachable until they are migrated.The sidebar/type union now excludes
moonshotandzai, but the controller still accepts both provider IDs and this file still has metadata/default models for them. Existing saved configs for those providers will disappear from the UI and become impossible to edit or remove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/pages/models.tsx` around lines 415 - 427, The BYOK_PROVIDER_IDS constant and resulting ByokProviderId union currently omit "moonshot" and "zai", which hides existing saved configs; restore those providers by adding "moonshot" and "zai" back into the BYOK_PROVIDER_IDS array so the ByokProviderId type and sidebar/type union include them, and verify any related metadata/default model entries for moonshot and zai remain referenced (e.g., in the same file's metadata/defaults) so existing configs remain editable/removable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/pages/models.tsx`:
- Around line 1798-1808: The dismiss button for the MiniMax OAuth error uses a
hard-coded aria-label ("Dismiss error") which bypasses localization; update the
button to use the i18n translation function (the same t(...) used elsewhere) for
its aria-label so screen readers get locale-specific text, e.g., replace the
static aria-label on the element associated with visibleMiniMaxOauthError and
setDismissedMiniMaxOauthError to a translated string via t('...') consistent
with other MiniMax copy.
- Around line 1284-1288: The current defaulting logic for authMode and
oauthRegion uses providerId === "minimax" to pick "oauth", which misclassifies
legacy MiniMax entries; instead, update the initialization of authMode (used
with setAuthMode) and oauthRegion (used with setOauthRegion) to first inspect
saved credential flags on dbProvider (e.g., whether an API key or OAuth token is
present) and derive the fallback from those flags when dbProvider.authMode is
missing; apply the same change at the other occurrence (around the other
authMode/oauthRegion initialization) so legacy API-key-only MiniMax configs open
the API-key panel and avoid exposing OAuth-only UI.
- Around line 1546-1570: Add onError handlers to the existing
minimaxOauthMutation (and the equivalent cancel mutation) so HTTP/IPC failures
are surfaced to the UI: in minimaxOauthMutation (which calls hostBridge.invoke
or postApiV1ProvidersMinimaxOauthLogin) add an onError callback that takes the
error, invalidates the ["minimax-oauth-status"] query via
queryClient.invalidateQueries, and write the error message into the status cache
(e.g. queryClient.setQueryData(["minimax-oauth-status"], prev => ({
...(prev||{}), error: error?.message || String(error) }))) so
minimaxOauthStatus.error is populated; apply the same pattern to the cancel
mutation.
- Around line 1451-1460: The effect that loads stored MiniMax models should also
clear verifiedModels when switching away from the OAuth MiniMax path; inside the
useEffect that currently returns early when (!isMiniMax || authMode !==
"oauth"), call setVerifiedModels([]) before returning so the API-key
verifiedModels do not persist into the OAuth panel. Update the effect that reads
dbProvider?.modelsJson to only setVerifiedModels(stored) when stored.length > 0
and ensure the early-return branch clears the state via setVerifiedModels([]).
---
Outside diff comments:
In `@apps/web/src/pages/models.tsx`:
- Around line 415-427: The BYOK_PROVIDER_IDS constant and resulting
ByokProviderId union currently omit "moonshot" and "zai", which hides existing
saved configs; restore those providers by adding "moonshot" and "zai" back into
the BYOK_PROVIDER_IDS array so the ByokProviderId type and sidebar/type union
include them, and verify any related metadata/default model entries for moonshot
and zai remain referenced (e.g., in the same file's metadata/defaults) so
existing configs remain editable/removable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d12fb44-a34f-4494-841a-d804d8288039
📒 Files selected for processing (1)
apps/web/src/pages/models.tsx
lefarcen
reviewed
Mar 25, 2026
Collaborator
lefarcen
left a comment
There was a problem hiding this comment.
Code Review (updated)
三个需要修复的问题：
1. home.starnexu 翻译 key 错误 — UI 会显示 raw key
apps/web/src/pages/home.tsx 中 t("home.starNexu") 被改成了 t("home.starnexu")，但 locale 文件里没有对应更新，会导致首页按钮显示未翻译的原始 key。
2. OpenAPI spec 未完全重新生成
supportedByokProviderIds 包含 moonshot 和 zai（共 11 个），但生成的 openapi.json / types.gen.ts 只有 9 个 provider。需要重新 pnpm generate-types。
3. browserUrl 打开前未校验 URL scheme — XSS 风险
models.tsx 中 window.open(browserUrl, "_blank", "noopener,noreferrer") 的 browserUrl 来自 MiniMax API 响应。如果被篡改为 javascript: URL 会产生 XSS。建议加个前缀校验：
if (browserUrl && /^https?:\/\//.test(browserUrl)) {
window.open(browserUrl, "_blank", "noopener,noreferrer");
}
lefarcen
approved these changes
Mar 25, 2026
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0268870058
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This was referenced Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #437
Summary
Summary by CodeRabbit
New Features
Chores