Repository navigation
[codex] add Wework local model settings - #1658
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds local model configuration storage and UI, routes model settings through the desktop and mobile settings surfaces, updates runtime model selection and labeling across local/cloud Codex models, tightens executor ChangesLocal model settings and runtime wiring
Estimated code review effort: 5 (Critical) | ~120 minutes Model selection and compatibility
Executor executionRequest contract tightening
Request user input typing refinement
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
qdaxb
marked this pull request as ready for review
July 1, 2026 04:50
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
executor/src/runtime_work/handler.rs (1)
751-763: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse
executionRequestas the workspace fallback insend_message().This path still derives
workspace_pathonly from the top-level/address payload or an existing link. A caller that sends a complete nestedexecutionRequestplus a runtime handle, but omits the duplicatedworkspacePath, will be accepted and then persisted with""as its workspace. That breaks the tightened request contract and corrupts follow-up task metadata.Proposed fix
- let workspace_path = workspace_path(&payload) - .or_else(|| { - existing_link - .as_ref() - .map(|link| link.workspace_path.clone()) - }) - .unwrap_or_default(); let mut request = payload_execution_request .ok_or_else(|| AppIpcError::new("bad_request", "executionRequest is required"))?; + let workspace_path = workspace_path(&payload) + .or_else(|| { + existing_link + .as_ref() + .map(|link| link.workspace_path.clone()) + }) + .or_else(|| request.cwd().map(str::to_owned)) + .unwrap_or_default(); apply_runtime_payload_metadata(&mut request, &payload);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/runtime_work/handler.rs` around lines 751 - 763, The workspace fallback logic in send_message()/handler currently ignores the nested executionRequest when deriving workspace_path, so requests with only executionRequest can be persisted with an empty workspace. Update the workspace_path selection to also fall back to the workspace value already present on payload_execution_request before defaulting to empty, and keep the existing request setup in handler::send_message consistent with the executionRequest contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/zh/developer-guide/wework-cloud-connection.md`:
- Line 7: The opening sentence in the document introduction sounds unnatural in
Chinese; revise the first sentence in the WeWork cloud connection guide to use
more idiomatic user-facing wording while keeping the meaning that Wework is a
fully local application by default. Update the introductory copy near the
opening paragraph so it reads smoothly, preserving the later distinction between
local features and the optional Backend/cloud connection flow.
In `@wework/src/api/local/localServices.ts`:
- Around line 432-469: Unresolved local-model names are falling through to the
built-in Codex path in localRuntimeModelConfig. Update the
localRuntimeModelConfig flow to detect when modelName matches a local-model:
identifier but findLocalModelConfigByModelName() returns nothing, and throw a
clear “model no longer configured” error instead of returning the built-in
branch. Keep the existing enabled check for resolved local models, and only use
builtInCodexModelId() for non-local Codex models.
- Around line 919-925: The request-user-input early return in localServices.send
still omits the new executionRequest field, so follow-up sends fail the updated
executor contract. Update the requestUserInputResponse branch in
runtime.tasks.send handling to include executionRequest in the returned payload,
matching the normal send path and preserving any existing fields like address,
message_id, and collaborationMode.
In `@wework/src/components/settings/ConnectionsSettingsPage.tsx`:
- Around line 148-150: The path mapping in getSettingsNavFromPath() no longer
handles the legacy `/settings/personal/codex` deep link, so add an alias check
alongside the existing `/settings/personal` handling to route that old URL to
`'model-settings'` instead of falling back to `'connections'`. Update the
navigation mapping in ConnectionsSettingsPage so both the new and old
personal-settings paths resolve correctly, preserving bookmarks and history
entries.
In `@wework/src/components/settings/ModelSettingsPage.tsx`:
- Around line 472-478: The delete handler in ModelSettingsPage’s local model
action flow calls deleteLocalModelConfig without any error handling, unlike the
save/clear paths. Wrap the onClick logic for the delete button in try/catch, and
on failure route the error into the same inline error state used by the other
storage mutations instead of letting the exception escape; keep the existing
resetForm behavior only on success and reference deleteLocalModelConfig and
resetForm when updating the handler.
In `@wework/src/features/model-settings/localModelSettings.test.ts`:
- Around line 63-103: The test in localModelSettings.test.ts leaves the
LOCAL_MODEL_SETTINGS_CHANGED_EVENT listener attached if an assertion fails,
which can leak state into later tests. Update the test around
saveLocalModelConfig, deleteLocalModelConfig, and clearLocalModelConfigs to
guarantee window.removeEventListener is always called, either by wrapping the
test body in try/finally or moving listener cleanup into afterEach. Keep the
listener setup and teardown isolated to this test so it does not affect other
cases.
In `@wework/src/features/model-settings/localModelSettings.ts`:
- Around line 50-60: The local model settings flow is persisting and
rebroadcasting plaintext API keys through saveLocalModelConfig,
writeStoredConfigs, and dispatchChanged. Move apiKey storage out of LocalStorage
into a keychain-backed secret store, and update writeStoredConfigs to save only
non-secret config fields. Also change dispatchChanged so
LOCAL_MODEL_SETTINGS_CHANGED_EVENT carries redacted configs without apiKey, and
make any read/merge paths in localModelSettings.ts reconstruct the full config
by fetching the secret separately.
In `@wework/src/pages/AppsPage.tsx`:
- Around line 445-453: The “Codex” app card is still using the old title while
the description and navigation now point to Model Settings/local models. Update
the card configuration in AppsPage’s app definition so the title matches the
renamed feature, and keep it consistent with the existing codexConfigured state,
description, and onClick target in the same object.
---
Outside diff comments:
In `@executor/src/runtime_work/handler.rs`:
- Around line 751-763: The workspace fallback logic in send_message()/handler
currently ignores the nested executionRequest when deriving workspace_path, so
requests with only executionRequest can be persisted with an empty workspace.
Update the workspace_path selection to also fall back to the workspace value
already present on payload_execution_request before defaulting to empty, and
keep the existing request setup in handler::send_message consistent with the
executionRequest contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b187000-1259-403a-b2d5-ddf1ef6b5a89
📒 Files selected for processing (20)
docs/en/developer-guide/wework-cloud-connection.mddocs/zh/developer-guide/wework-cloud-connection.mdexecutor/src/runtime_work/handler.rsexecutor/src/runtime_work/util.rsexecutor/tests/app_runtime_work_send_contract.rswework/src/api/local/localServices.test.tswework/src/api/local/localServices.tswework/src/components/chat/blocks/ToolBlocksDisplay.tsxwework/src/components/chat/requestUserInputMessages.tswework/src/components/settings/ConnectionsSettingsPage.test.tsxwework/src/components/settings/ConnectionsSettingsPage.tsxwework/src/components/settings/MobileSettingsPage.test.tsxwework/src/components/settings/MobileSettingsPage.tsxwework/src/components/settings/ModelSettingsPage.tsxwework/src/features/model-settings/localModelSettings.test.tswework/src/features/model-settings/localModelSettings.tswework/src/features/workbench/useWorkbenchRuntimeMessaging.tswework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/pages/AppsPage.tsx
| # 本地优先云端连接 | ||
|
|
||
| Wework 默认是完整本地应用。本机 Codex、本地 executor、本地工作区和本地会话不依赖 Backend 登录或云端设备。云端连接是一个可选能力层：用户在侧栏输入 Backend 地址并完成 WeWork 登录后，服务端模型、云设备和云端 Codex 认证同步会加入同一个工作台。 | ||
| Wework 默认是完整本地应用。本机 Codex、本地模型配置、本地 executor、本地工作区和本地会话不依赖 Backend 登录或云端设备。云端连接是一个可选能力层：用户在侧栏输入 Backend 地址并完成 WeWork 登录后，服务端模型、云设备和云端 Codex 认证同步会加入同一个工作台。 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Polish the opening sentence.
Line 7 reads awkwardly in Chinese. “默认是完整本地应用” is not natural phrasing for user-facing docs.
Suggested fix
-Wework 默认是完整本地应用。本机 Codex、本地模型配置、本地 executor、本地工作区和本地会话不依赖 Backend 登录或云端设备。
+Wework 默认就是一个完整的本地应用。本机 Codex、本地模型配置、本地 executor、本地工作区和本地会话不依赖 Backend 登录或云端设备。📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Wework 默认是完整本地应用。本机 Codex、本地模型配置、本地 executor、本地工作区和本地会话不依赖 Backend 登录或云端设备。云端连接是一个可选能力层：用户在侧栏输入 Backend 地址并完成 WeWork 登录后，服务端模型、云设备和云端 Codex 认证同步会加入同一个工作台。 | |
| Wework 默认就是一个完整的本地应用。本机 Codex、本地模型配置、本地 executor、本地工作区和本地会话不依赖 Backend 登录或云端设备。云端连接是一个可选能力层：用户在侧栏输入 Backend 地址并完成 WeWork 登录后，服务端模型、云设备和云端 Codex 认证同步会加入同一个工作台。 |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: 您不可用“是”来表达个形容词。大部分人都平常用“很”来代替“是”。您是不是想表达"很完整"？
Context: ..._position: 32 --- # 本地优先云端连接 Wework 默认是完整本地应用。本机 Codex、本地模型配置、本地 executor、本地工作区和本...
(SHI_ADHECTIVE_ERROR)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/zh/developer-guide/wework-cloud-connection.md` at line 7, The opening
sentence in the document introduction sounds unnatural in Chinese; revise the
first sentence in the WeWork cloud connection guide to use more idiomatic
user-facing wording while keeping the meaning that Wework is a fully local
application by default. Update the introductory copy near the opening paragraph
so it reads smoothly, preserving the later distinction between local features
and the optional Backend/cloud connection flow.
Source: Linters/SAST tools
Comment on lines
+445
to
+453
| title: 'Codex', | ||
| description: '管理 Codex auth.json、代理开关与默认 GPT 模型，可从在线设备导入认证。', | ||
| description: '管理本机模型与 Codex auth.json 云端同步，可从在线设备导入认证。', | ||
| icon: Cpu, | ||
| iconClassName: 'bg-gradient-to-br from-slate-900 to-slate-500 text-white', | ||
| status: codexConfigured ? '已认证' : '需认证', | ||
| statusTone: codexConfigured ? 'online' : 'warning', | ||
| meta: codexConfigured ? 'auth.json 已保存' : 'auth.json 缺失', | ||
| action: codexConfigured ? '打开' : '去配置', | ||
| onClick: () => navigateTo('/settings/personal/codex'), | ||
| onClick: () => navigateTo('/settings/personal/models'), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stale "Codex" card title after Model Settings rename.
The description and onClick target were updated to reflect the local-model/Model-Settings rename, but the card title at Line 445 still reads 'Codex'. This creates an inconsistent user-facing label: the card says "Codex" while its description talks about local models and its action navigates to /settings/personal/models.
💚 Proposed fix
{
- title: 'Codex',
+ title: '模型设置',
description: '管理本机模型与 Codex auth.json 云端同步，可从在线设备导入认证。',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| title: 'Codex', | |
| description: '管理 Codex auth.json、代理开关与默认 GPT 模型，可从在线设备导入认证。', | |
| description: '管理本机模型与 Codex auth.json 云端同步，可从在线设备导入认证。', | |
| icon: Cpu, | |
| iconClassName: 'bg-gradient-to-br from-slate-900 to-slate-500 text-white', | |
| status: codexConfigured ? '已认证' : '需认证', | |
| statusTone: codexConfigured ? 'online' : 'warning', | |
| meta: codexConfigured ? 'auth.json 已保存' : 'auth.json 缺失', | |
| action: codexConfigured ? '打开' : '去配置', | |
| onClick: () => navigateTo('/settings/personal/codex'), | |
| onClick: () => navigateTo('/settings/personal/models'), | |
| title: '模型设置', | |
| description: '管理本机模型与 Codex auth.json 云端同步，可从在线设备导入认证。', | |
| icon: Cpu, | |
| iconClassName: 'bg-gradient-to-br from-slate-900 to-slate-500 text-white', | |
| status: codexConfigured ? '已认证' : '需认证', | |
| statusTone: codexConfigured ? 'online' : 'warning', | |
| meta: codexConfigured ? 'auth.json 已保存' : 'auth.json 缺失', | |
| action: codexConfigured ? '打开' : '去配置', | |
| onClick: () => navigateTo('/settings/personal/models'), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/pages/AppsPage.tsx` around lines 445 - 453, The “Codex” app card
is still using the old title while the description and navigation now point to
Model Settings/local models. Update the card configuration in AppsPage’s app
definition so the title matches the renamed feature, and keep it consistent with
the existing codexConfigured state, description, and onClick target in the same
object.
Contributor
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 (3)
executor/src/runtime_work/util.rs (1)
10-22: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
execution_requestnow the sole path but still swallows deserialize errors.Since
execution_request_from_payloadwas removed,execution_request(&payload)(lines 14-20) is the only way to obtain anExecutionRequest. It uses.and_then(|value| serde_json::from_value::<ExecutionRequest>(value).ok()), discarding the actual deserialize error. Both call sites inhandler.rsthen report the generic"executionRequest is required"even when the field is present but malformed (wrong types, invalid nested shape, etc.), which will mislead clients debugging integration issues with this newly-tightened, fail-fast contract.
serde_json::Errorcarries structured diagnostics (line/column, category) that are useful for exactly this kind of debugging, and discarding them here removes that value.🩹 Proposed fix to distinguish missing vs. invalid executionRequest
-pub(crate) fn execution_request(payload: &Value) -> Option<ExecutionRequest> { - payload - .get("executionRequest") - .or_else(|| payload.get("execution_request")) - .cloned() - .and_then(|value| serde_json::from_value::<ExecutionRequest>(value).ok()) -} +pub(crate) fn execution_request(payload: &Value) -> Result<ExecutionRequest, String> { + let raw = payload + .get("executionRequest") + .or_else(|| payload.get("execution_request")) + .cloned() + .ok_or_else(|| "executionRequest is required".to_owned())?; + serde_json::from_value::<ExecutionRequest>(raw) + .map_err(|error| format!("executionRequest is invalid: {error}")) +}Then in
handler.rs:- let mut request = execution_request(&payload) - .ok_or_else(|| AppIpcError::new("bad_request", "executionRequest is required"))?; + let mut request = execution_request(&payload) + .map_err(|message| AppIpcError::new("bad_request", message))?;(and similarly for the
send_messagecall site).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/runtime_work/util.rs` around lines 10 - 22, The execution_request helper currently hides deserialization failures by converting serde_json::from_value to Option, so malformed executionRequest payloads are reported as missing. Update execution_request in util.rs to return the deserialize error (or a Result/explicit error marker) and distinguish “field absent” from “field present but invalid.” Then in the handler.rs call sites that use execution_request, surface the serde_json::Error details instead of the generic “executionRequest is required” message for malformed payloads.wework/src/features/workbench/useWorkbenchModels.ts (1)
223-252: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConcurrent
loadModelscalls can overwrite fresher data with stale results.
loadModelsis now invoked both on mount and on everyLOCAL_MODEL_SETTINGS_CHANGED_EVENT. Both invocations share the samecancelledflag, which only guards against unmount — it does not prevent an older, slower in-flight call from overwriting a newer, faster-resolving one. If the initialapi.listModels()call is still pending when the change event fires and resolves after the event-triggered call,setAvailableModelswill apply the stale initial result last, silently reverting the model list.🔧 Proposed fix: guard against out-of-order responses with a request id
useEffect(() => { let cancelled = false + let requestId = 0 async function loadModels() { + const currentRequestId = ++requestId setIsLoading(true) setError(null) try { const response = await api.listModels() - if (!cancelled) { + if (!cancelled && currentRequestId === requestId) { const filtered = response.data.filter(isSupportedModelFamily) setAvailableModels(filtered) } } catch (nextError) { - if (!cancelled) { + if (!cancelled && currentRequestId === requestId) { setError(nextError instanceof Error ? nextError : new Error('Failed to load models')) } } finally { - if (!cancelled) { + if (!cancelled && currentRequestId === requestId) { setIsLoading(false) } } } void loadModels() window.addEventListener(LOCAL_MODEL_SETTINGS_CHANGED_EVENT, loadModels) return () => { cancelled = true window.removeEventListener(LOCAL_MODEL_SETTINGS_CHANGED_EVENT, loadModels) } }, [api])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/features/workbench/useWorkbenchModels.ts` around lines 223 - 252, The async model reload flow in useWorkbenchModels allows out-of-order responses to overwrite newer data because the shared cancelled flag only handles unmounting, not multiple concurrent loadModels invocations. Update loadModels to track each request with a unique request id (or similar monotonically increasing token) and only call setAvailableModels, setError, and setIsLoading for the latest in-flight request. Keep the existing LOCAL_MODEL_SETTINGS_CHANGED_EVENT listener and useEffect cleanup, but make the response-handling in loadModels ignore stale results based on that request token.wework/src/features/workbench/useWorkbenchRuntimeMessaging.ts (1)
278-350: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLocal-model/non-local-device guard runs after a device side effect in the standalone flow.
In the standalone (non-project) branch,
createConversationWorkspace(lines 319-331) executes againstactiveDeviceIdbefore the local-model/device guard at lines 343-350 has a chance to block the send. If a local model is selected butactiveDeviceIdis a non-local device, a workspace will already be created on that device before the send is rejected, leaving unwanted/orphaned state. Move the check earlier — right afteractiveDeviceId/optimisticDeviceIdis known for each branch — so no device work happens before validation.🔧 Proposed fix
} else { let workspacePath = state.standaloneWorkspacePath + if ( + isConfiguredLocalModel(selectedModel) && + !isLocalDeviceTarget(state.devices, activeDeviceId) + ) { + reportSendBlocked(i18n.t('workbench.local_model_cloud_device_blocked')) + return false + } if (!workspacePath && activeDeviceId) { try { workspacePath = await createConversationWorkspace(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/features/workbench/useWorkbenchRuntimeMessaging.ts` around lines 278 - 350, The local-model/non-local-device validation in sendPreparedRuntimeMessage is running too late in the standalone flow, after createConversationWorkspace may already create device state. Move the isConfiguredLocalModel/isLocalDeviceTarget check earlier in both branches, right after optimisticDeviceId is determined, so the send is rejected before any workspace creation or other device side effects happen. Keep the existing blocked-message path and reuse sendPreparedRuntimeMessage, createConversationWorkspace, and reportSendBlocked for the early return.
🧹 Nitpick comments (4)
executor/src/runtime_work/handler.rs (1)
734-735: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
has_execution_requestlog field is now alwaystrue.Since
send_messagereturns early via.ok_or_else(...)?at line 758-759 wheneverpayload_execution_requestisNone, by the time execution reaches thelog_executor_eventcall at line 783,has_execution_requestcan no longer be anything buttrue. The field is now dead weight in the log output and could be removed (or the log point moved earlier if the pre-check state is still useful for diagnostics).Also applies to: 783-784
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/runtime_work/handler.rs` around lines 734 - 735, The `has_execution_request` field in `send_message` is no longer meaningful at the `log_executor_event` call because the earlier `ok_or_else(...)?` return guarantees `payload_execution_request` is always `Some` by then. Remove the dead-weight `has_execution_request` logging from `handler.rs` (around `send_message` and `log_executor_event`), or move the log before the early return if you still need to capture the pre-check state for diagnostics.wework/src/features/model-settings/localModelSettings.test.ts (1)
63-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for
apiKeypreservation semantics on partial updates.None of the current tests assert whether
apiKeysurvives an update that omits it (theupdatedcall at Line 75 doesn't passapiKey). Given the ambiguity flagged inlocalModelSettings.ts(Lines 112-134) around whether omittingapiKeyclears vs. preserves it, a dedicated test would pin down the intended contract and catch regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/features/model-settings/localModelSettings.test.ts` around lines 63 - 113, Add a focused test in localModelSettings.test.ts around saveLocalModelConfig to pin down partial update behavior for apiKey. Use the existing saveLocalModelConfig and listLocalModelConfigs helpers to create a config with apiKey, then update the same id without passing apiKey and assert the intended contract explicitly (preserved or cleared). Keep the check near the existing updates/deletes/clears test so future changes to localModelSettings.ts cannot silently change apiKey semantics.wework/src/features/model-settings/localModelConnectionTest.test.ts (1)
1-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for happy-path and HTTP-error cases; consider adding a timeout test.
The suite covers the dummy-token and provided-API-key/HTTP-error paths well, but doesn't exercise the
AbortController/timeout branch ('Model test timed out') intestLocalModelConnection. Consider a test using a fetcher that respects the abort signal (or a shorttimeoutMs) to cover that edge case.As per coding guidelines,
**/*.{py,ts,tsx,js,jsx}files should "follow the project's testing principles: AAA, mock external services, test edge cases, and keep tests isolated."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/features/model-settings/localModelConnectionTest.test.ts` around lines 1 - 80, Add a test that exercises the timeout/AbortController path in testLocalModelConnection, since the current localModelConnectionTest suite only covers success and HTTP error cases. Use a mocked fetcher that observes the passed abort signal (or set a very short timeoutMs) so the request times out and assert it rejects with the Model test timed out error. Keep the test isolated and follow the existing AAA style alongside the current testLocalModelConnection coverage.Source: Coding guidelines
wework/src/pages/AppsPage.tsx (1)
428-455: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCard status still reflects only cloud Codex auth, not the broadened "模型设置" scope.
The card title/description now cover both local models and cloud Codex sync (consistent with the rename, and the earlier stale-"Codex"-title issue is fixed). However
status/meta(Line 449-451) are still derived solely fromcodexConfigured, so a user with local models configured but no cloud auth still sees "需认证" / "auth.json 缺失" — understating what's actually configured.Consider factoring in
listLocalModelConfigs().length > 0(or similar) alongsidecodexConfiguredwhen computing this card's status/meta.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/pages/AppsPage.tsx` around lines 428 - 455, The “模型设置” card in buildRecommendedApps still bases its status/meta only on codexConfig, so it can show “需认证” and “auth.json 缺失” even when local model configs already exist. Update the logic in buildRecommendedApps to combine codexConfigured with local model state (for example by checking listLocalModelConfigs().length > 0 or equivalent) when computing the second card’s status, statusTone, and meta, so the card reflects the broader Models Settings scope rather than only cloud Codex auth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wework/src/api/local/localServices.ts`:
- Around line 911-974: `createLocalRuntimeSendPayload` has a duplicated
conditional branch for `requestUserInputResponse`/`request_user_input_response`
that returns the same payload shape as the default path. Collapse the logic into
a single return path in `createLocalRuntimeSendPayload`, keeping the shared
`payload`, `normalizedAddress`, `message_id`, optional `collaborationMode`, and
`buildLocalRuntimeExecutionRequest` construction, since the special-case branch
is now redundant.
In `@wework/src/components/settings/ModelSettingsPage.tsx`:
- Around line 291-360: The model settings flows in handleSubmit,
clearEditingApiKey, and handleTestModel are showing raw English validation
messages via getErrorMessage(), which can leak into the Chinese UI. Update
ModelSettingsPage to intercept the known errors from normalizeLocalModelBaseUrl
and normalizeLocalModelId before displaying them, and map them to localized
fallback strings using the existing t(...) keys. Keep the current
save/test/clear-key behavior unchanged, but ensure the displayed message is
translated whenever those validation failures occur.
In `@wework/src/features/model-settings/localModelSettings.ts`:
- Around line 112-134: The update path in saveLocalModelConfig is wiping the
stored apiKey when callers omit it, unlike enabled which preserves the previous
value. Move the apiKey handling to after previous is read and default it to
previous?.apiKey when input.apiKey is undefined, while still allowing an
explicit empty/null-like value to clear it if that is the intended behavior.
Keep the change localized to saveLocalModelConfig and its
SaveLocalModelConfigInput flow so future callers don’t need to re-pass existing
credentials.
In `@wework/src/i18n/locales/en/common.json`:
- Around line 767-790: Update the stale copy in the locale entry that still
references “Codex Auth” so it matches the renamed settings UI. In common.json,
revise proxy_config_description (and any nearby related text in the same
section) to point users to the new “Model Settings” / “Cloud Codex Sync” page
instead of the old page name. Keep the wording consistent with the other
runtime_config_* strings in this block so the renamed flow reads uniformly
across the page.
---
Outside diff comments:
In `@executor/src/runtime_work/util.rs`:
- Around line 10-22: The execution_request helper currently hides
deserialization failures by converting serde_json::from_value to Option, so
malformed executionRequest payloads are reported as missing. Update
execution_request in util.rs to return the deserialize error (or a
Result/explicit error marker) and distinguish “field absent” from “field present
but invalid.” Then in the handler.rs call sites that use execution_request,
surface the serde_json::Error details instead of the generic “executionRequest
is required” message for malformed payloads.
In `@wework/src/features/workbench/useWorkbenchModels.ts`:
- Around line 223-252: The async model reload flow in useWorkbenchModels allows
out-of-order responses to overwrite newer data because the shared cancelled flag
only handles unmounting, not multiple concurrent loadModels invocations. Update
loadModels to track each request with a unique request id (or similar
monotonically increasing token) and only call setAvailableModels, setError, and
setIsLoading for the latest in-flight request. Keep the existing
LOCAL_MODEL_SETTINGS_CHANGED_EVENT listener and useEffect cleanup, but make the
response-handling in loadModels ignore stale results based on that request
token.
In `@wework/src/features/workbench/useWorkbenchRuntimeMessaging.ts`:
- Around line 278-350: The local-model/non-local-device validation in
sendPreparedRuntimeMessage is running too late in the standalone flow, after
createConversationWorkspace may already create device state. Move the
isConfiguredLocalModel/isLocalDeviceTarget check earlier in both branches, right
after optimisticDeviceId is determined, so the send is rejected before any
workspace creation or other device side effects happen. Keep the existing
blocked-message path and reuse sendPreparedRuntimeMessage,
createConversationWorkspace, and reportSendBlocked for the early return.
---
Nitpick comments:
In `@executor/src/runtime_work/handler.rs`:
- Around line 734-735: The `has_execution_request` field in `send_message` is no
longer meaningful at the `log_executor_event` call because the earlier
`ok_or_else(...)?` return guarantees `payload_execution_request` is always
`Some` by then. Remove the dead-weight `has_execution_request` logging from
`handler.rs` (around `send_message` and `log_executor_event`), or move the log
before the early return if you still need to capture the pre-check state for
diagnostics.
In `@wework/src/features/model-settings/localModelConnectionTest.test.ts`:
- Around line 1-80: Add a test that exercises the timeout/AbortController path
in testLocalModelConnection, since the current localModelConnectionTest suite
only covers success and HTTP error cases. Use a mocked fetcher that observes the
passed abort signal (or set a very short timeoutMs) so the request times out and
assert it rejects with the Model test timed out error. Keep the test isolated
and follow the existing AAA style alongside the current testLocalModelConnection
coverage.
In `@wework/src/features/model-settings/localModelSettings.test.ts`:
- Around line 63-113: Add a focused test in localModelSettings.test.ts around
saveLocalModelConfig to pin down partial update behavior for apiKey. Use the
existing saveLocalModelConfig and listLocalModelConfigs helpers to create a
config with apiKey, then update the same id without passing apiKey and assert
the intended contract explicitly (preserved or cleared). Keep the check near the
existing updates/deletes/clears test so future changes to localModelSettings.ts
cannot silently change apiKey semantics.
In `@wework/src/pages/AppsPage.tsx`:
- Around line 428-455: The “模型设置” card in buildRecommendedApps still bases its
status/meta only on codexConfig, so it can show “需认证” and “auth.json 缺失” even
when local model configs already exist. Update the logic in buildRecommendedApps
to combine codexConfigured with local model state (for example by checking
listLocalModelConfigs().length > 0 or equivalent) when computing the second
card’s status, statusTone, and meta, so the card reflects the broader Models
Settings scope rather than only cloud Codex auth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b6fc9a6-160f-426b-ad69-69190af9b518
📒 Files selected for processing (25)
docs/en/developer-guide/wework-cloud-connection.mddocs/zh/developer-guide/wework-cloud-connection.mdexecutor/src/runtime_work/handler.rsexecutor/src/runtime_work/util.rsexecutor/tests/app_runtime_work_send_contract.rswework/src/App.apps.test.tsxwework/src/api/local/localServices.test.tswework/src/api/local/localServices.tswework/src/components/chat/blocks/ToolBlocksDisplay.tsxwework/src/components/chat/requestUserInputMessages.tswework/src/components/settings/ConnectionsSettingsPage.test.tsxwework/src/components/settings/ConnectionsSettingsPage.tsxwework/src/components/settings/MobileSettingsPage.test.tsxwework/src/components/settings/MobileSettingsPage.tsxwework/src/components/settings/ModelSettingsPage.tsxwework/src/features/model-settings/localModelConnectionTest.test.tswework/src/features/model-settings/localModelConnectionTest.tswework/src/features/model-settings/localModelSettings.test.tswework/src/features/model-settings/localModelSettings.tswework/src/features/workbench/useWorkbenchModels.tswework/src/features/workbench/useWorkbenchRuntimeMessaging.tswework/src/features/workbench/workbenchProjectChatHooks.test.tsxwework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/pages/AppsPage.tsx
Comment on lines
+911
to
974
| function createLocalRuntimeSendPayload( | ||
| data: RuntimeSendRequest, | ||
| localDeviceId: string | ||
| ): Record<string, unknown> { | ||
| const messageId = data.message_id ?? createRuntimeMessageId() | ||
| const collaborationMode = runtimeCollaborationMode(data.modelOptions) | ||
| const workspacePath = stringValue(data.address.workspacePath) | ||
| const normalizedAddress: RuntimeTaskAddress = { | ||
| ...data.address, | ||
| deviceId: localDeviceId, | ||
| ...(workspacePath ? { workspacePath } : {}), | ||
| } | ||
|
|
||
| if (data.requestUserInputResponse || data.request_user_input_response) { | ||
| const payload = { ...data } as Record<string, unknown> | ||
| delete payload.modelId | ||
| delete payload.modelType | ||
| return { | ||
| ...payload, | ||
| address: normalizedAddress, | ||
| message_id: messageId, | ||
| ...(collaborationMode ? { collaborationMode } : {}), | ||
| executionRequest: buildLocalRuntimeExecutionRequest({ | ||
| localTaskId: data.address.localTaskId, | ||
| runtime: 'codex', | ||
| teamId: LOCAL_WORKBENCH_TEAM.id, | ||
| title: data.address.localTaskId, | ||
| message: data.message, | ||
| messageId, | ||
| modelId: data.modelId, | ||
| modelOptions: data.modelOptions, | ||
| attachments: data.attachments, | ||
| localDeviceId, | ||
| workspacePath, | ||
| workspaceSource: 'local_path', | ||
| newSession: false, | ||
| }), | ||
| } as unknown as Record<string, unknown> | ||
| } | ||
|
|
||
| const payload = { ...data } as Record<string, unknown> | ||
| delete payload.modelId | ||
| delete payload.modelType | ||
| return { | ||
| ...payload, | ||
| address: normalizedAddress, | ||
| message_id: messageId, | ||
| ...(collaborationMode ? { collaborationMode } : {}), | ||
| executionRequest: buildLocalRuntimeExecutionRequest({ | ||
| localTaskId: data.address.localTaskId, | ||
| runtime: 'codex', | ||
| teamId: LOCAL_WORKBENCH_TEAM.id, | ||
| title: data.address.localTaskId, | ||
| message: data.message, | ||
| messageId, | ||
| modelId: data.modelId, | ||
| modelOptions: data.modelOptions, | ||
| attachments: data.attachments, | ||
| localDeviceId, | ||
| workspacePath, | ||
| workspaceSource: 'local_path', | ||
| newSession: false, | ||
| }), | ||
| } as unknown as Record<string, unknown> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Redundant duplicated branch in createLocalRuntimeSendPayload.
The requestUserInputResponse/request_user_input_response branch (lines 924-949) and the default branch (lines 951-974) build and return an identical shape — same spread of payload, same buildLocalRuntimeExecutionRequest args. Since payload = { ...data } already preserves requestUserInputResponse/request_user_input_response fields regardless of which branch runs, the if guard is now dead code left over from the earlier fix for the missing executionRequest. Consider collapsing to a single path.
♻️ Proposed simplification
- if (data.requestUserInputResponse || data.request_user_input_response) {
- const payload = { ...data } as Record<string, unknown>
- delete payload.modelId
- delete payload.modelType
- return {
- ...payload,
- address: normalizedAddress,
- message_id: messageId,
- ...(collaborationMode ? { collaborationMode } : {}),
- executionRequest: buildLocalRuntimeExecutionRequest({
- localTaskId: data.address.localTaskId,
- runtime: 'codex',
- teamId: LOCAL_WORKBENCH_TEAM.id,
- title: data.address.localTaskId,
- message: data.message,
- messageId,
- modelId: data.modelId,
- modelOptions: data.modelOptions,
- attachments: data.attachments,
- localDeviceId,
- workspacePath,
- workspaceSource: 'local_path',
- newSession: false,
- }),
- } as unknown as Record<string, unknown>
- }
-
const payload = { ...data } as Record<string, unknown>
delete payload.modelId
delete payload.modelType
return {
...payload,
address: normalizedAddress,
message_id: messageId,
...(collaborationMode ? { collaborationMode } : {}),
executionRequest: buildLocalRuntimeExecutionRequest({
localTaskId: data.address.localTaskId,
runtime: 'codex',
teamId: LOCAL_WORKBENCH_TEAM.id,
title: data.address.localTaskId,
message: data.message,
messageId,
modelId: data.modelId,
modelOptions: data.modelOptions,
attachments: data.attachments,
localDeviceId,
workspacePath,
workspaceSource: 'local_path',
newSession: false,
}),
} as unknown as Record<string, unknown>
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function createLocalRuntimeSendPayload( | |
| data: RuntimeSendRequest, | |
| localDeviceId: string | |
| ): Record<string, unknown> { | |
| const messageId = data.message_id ?? createRuntimeMessageId() | |
| const collaborationMode = runtimeCollaborationMode(data.modelOptions) | |
| const workspacePath = stringValue(data.address.workspacePath) | |
| const normalizedAddress: RuntimeTaskAddress = { | |
| ...data.address, | |
| deviceId: localDeviceId, | |
| ...(workspacePath ? { workspacePath } : {}), | |
| } | |
| if (data.requestUserInputResponse || data.request_user_input_response) { | |
| const payload = { ...data } as Record<string, unknown> | |
| delete payload.modelId | |
| delete payload.modelType | |
| return { | |
| ...payload, | |
| address: normalizedAddress, | |
| message_id: messageId, | |
| ...(collaborationMode ? { collaborationMode } : {}), | |
| executionRequest: buildLocalRuntimeExecutionRequest({ | |
| localTaskId: data.address.localTaskId, | |
| runtime: 'codex', | |
| teamId: LOCAL_WORKBENCH_TEAM.id, | |
| title: data.address.localTaskId, | |
| message: data.message, | |
| messageId, | |
| modelId: data.modelId, | |
| modelOptions: data.modelOptions, | |
| attachments: data.attachments, | |
| localDeviceId, | |
| workspacePath, | |
| workspaceSource: 'local_path', | |
| newSession: false, | |
| }), | |
| } as unknown as Record<string, unknown> | |
| } | |
| const payload = { ...data } as Record<string, unknown> | |
| delete payload.modelId | |
| delete payload.modelType | |
| return { | |
| ...payload, | |
| address: normalizedAddress, | |
| message_id: messageId, | |
| ...(collaborationMode ? { collaborationMode } : {}), | |
| executionRequest: buildLocalRuntimeExecutionRequest({ | |
| localTaskId: data.address.localTaskId, | |
| runtime: 'codex', | |
| teamId: LOCAL_WORKBENCH_TEAM.id, | |
| title: data.address.localTaskId, | |
| message: data.message, | |
| messageId, | |
| modelId: data.modelId, | |
| modelOptions: data.modelOptions, | |
| attachments: data.attachments, | |
| localDeviceId, | |
| workspacePath, | |
| workspaceSource: 'local_path', | |
| newSession: false, | |
| }), | |
| } as unknown as Record<string, unknown> | |
| function createLocalRuntimeSendPayload( | |
| data: RuntimeSendRequest, | |
| localDeviceId: string | |
| ): Record<string, unknown> { | |
| const messageId = data.message_id ?? createRuntimeMessageId() | |
| const collaborationMode = runtimeCollaborationMode(data.modelOptions) | |
| const workspacePath = stringValue(data.address.workspacePath) | |
| const normalizedAddress: RuntimeTaskAddress = { | |
| ...data.address, | |
| deviceId: localDeviceId, | |
| ...(workspacePath ? { workspacePath } : {}), | |
| } | |
| const payload = { ...data } as Record<string, unknown> | |
| delete payload.modelId | |
| delete payload.modelType | |
| return { | |
| ...payload, | |
| address: normalizedAddress, | |
| message_id: messageId, | |
| ...(collaborationMode ? { collaborationMode } : {}), | |
| executionRequest: buildLocalRuntimeExecutionRequest({ | |
| localTaskId: data.address.localTaskId, | |
| runtime: 'codex', | |
| teamId: LOCAL_WORKBENCH_TEAM.id, | |
| title: data.address.localTaskId, | |
| message: data.message, | |
| messageId, | |
| modelId: data.modelId, | |
| modelOptions: data.modelOptions, | |
| attachments: data.attachments, | |
| localDeviceId, | |
| workspacePath, | |
| workspaceSource: 'local_path', | |
| newSession: false, | |
| }), | |
| } as unknown as Record<string, unknown> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/api/local/localServices.ts` around lines 911 - 974,
`createLocalRuntimeSendPayload` has a duplicated conditional branch for
`requestUserInputResponse`/`request_user_input_response` that returns the same
payload shape as the default path. Collapse the logic into a single return path
in `createLocalRuntimeSendPayload`, keeping the shared `payload`,
`normalizedAddress`, `message_id`, optional `collaborationMode`, and
`buildLocalRuntimeExecutionRequest` construction, since the special-case branch
is now redundant.
Comment on lines
+291
to
+360
| const handleSubmit = (event: React.FormEvent<HTMLFormElement>) => { | ||
| event.preventDefault() | ||
| setError(null) | ||
| try { | ||
| saveLocalModelConfig({ | ||
| id: editingId, | ||
| displayName: form.displayName, | ||
| modelId: form.modelId, | ||
| baseUrl: form.baseUrl, | ||
| apiKey: form.apiKey.trim() ? form.apiKey : editingModel?.apiKey, | ||
| enabled: form.enabled, | ||
| }) | ||
| resetForm() | ||
| } catch (saveError) { | ||
| setError(getErrorMessage(saveError, t('workbench.local_model_save_failed', '保存模型失败'))) | ||
| } | ||
| } | ||
|
|
||
| const clearEditingApiKey = () => { | ||
| if (!editingModel) return | ||
| try { | ||
| saveLocalModelConfig({ | ||
| id: editingModel.id, | ||
| displayName: editingModel.displayName, | ||
| modelId: editingModel.modelId, | ||
| baseUrl: editingModel.baseUrl, | ||
| apiKey: null, | ||
| enabled: editingModel.enabled, | ||
| }) | ||
| startEditing({ ...editingModel, apiKey: undefined }) | ||
| } catch (clearError) { | ||
| setError( | ||
| getErrorMessage( | ||
| clearError, | ||
| t('workbench.local_model_clear_key_failed', '清除 API Key 失败') | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| const handleTestModel = async () => { | ||
| setError(null) | ||
| setTestResult(null) | ||
| setTestingModel(true) | ||
| try { | ||
| await testLocalModelConnection({ | ||
| baseUrl: form.baseUrl, | ||
| modelId: form.modelId, | ||
| apiKey: form.apiKey.trim() ? form.apiKey : editingModel?.apiKey, | ||
| }) | ||
| setTestResult({ | ||
| kind: 'success', | ||
| message: t('workbench.local_model_test_success', '模型连接正常'), | ||
| }) | ||
| } catch (testError) { | ||
| const message = getErrorMessage( | ||
| testError, | ||
| t('workbench.local_model_test_failed', '模型测试失败') | ||
| ) | ||
| setTestResult({ | ||
| kind: 'error', | ||
| message: t('workbench.local_model_test_failed_with_message', { | ||
| defaultValue: '模型测试失败：{{message}}', | ||
| message, | ||
| }), | ||
| }) | ||
| } finally { | ||
| setTestingModel(false) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify getErrorMessage's precedence (message vs fallback) and confirm
# normalizeLocalModelBaseUrl/normalizeLocalModelId throw English-only messages.
rg -n "function getErrorMessage" -A 15 --type=ts wework/src
rg -n "throw new Error" -B2 -A2 wework/src/features/model-settings/localModelSettings.tsRepository: wecode-ai/Wegent
Length of output: 6436
Localize model-setting validation errors before displaying them.
getErrorMessage() surfaces error.message directly, so the English errors thrown by normalizeLocalModelBaseUrl/normalizeLocalModelId can leak into this Chinese UI on save/test/clear-key paths. Map those known messages to translated fallbacks before showing them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/components/settings/ModelSettingsPage.tsx` around lines 291 - 360,
The model settings flows in handleSubmit, clearEditingApiKey, and
handleTestModel are showing raw English validation messages via
getErrorMessage(), which can leak into the Chinese UI. Update ModelSettingsPage
to intercept the known errors from normalizeLocalModelBaseUrl and
normalizeLocalModelId before displaying them, and map them to localized fallback
strings using the existing t(...) keys. Keep the current save/test/clear-key
behavior unchanged, but ensure the displayed message is translated whenever
those validation failures occur.
Comment on lines
+112
to
+134
| export function saveLocalModelConfig(input: SaveLocalModelConfigInput): LocalModelConfig { | ||
| const modelId = normalizeLocalModelId(input.modelId) | ||
| const baseUrl = normalizeLocalModelBaseUrl(input.baseUrl) | ||
| const displayName = input.displayName?.trim() || modelId | ||
| const apiKey = input.apiKey?.trim() || undefined | ||
| const id = input.id?.trim() || nextConfigId() | ||
| const existing = readStoredConfigs() | ||
| const previous = existing.find(config => config.id === id) | ||
| const next: LocalModelConfig = { | ||
| id, | ||
| displayName, | ||
| modelId, | ||
| baseUrl, | ||
| apiKey, | ||
| enabled: input.enabled ?? previous?.enabled ?? true, | ||
| updatedAt: new Date().toISOString(), | ||
| } | ||
| const index = existing.findIndex(config => config.id === id) | ||
| const configs = | ||
| index >= 0 ? existing.map(config => (config.id === id ? next : config)) : [...existing, next] | ||
| writeStoredConfigs(configs) | ||
| return next | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
apiKey isn't preserved on update, unlike enabled.
enabled falls back to previous?.enabled ?? true when omitted, but apiKey (Line 116) has no equivalent fallback to previous?.apiKey — it's computed as input.apiKey?.trim() || undefined before previous is even declared (Line 119). Any caller that updates a config (e.g. toggling enabled, renaming, or changing baseUrl) without also passing the existing apiKey will silently wipe the stored key. This is currently masked only by ModelSettingsPage.tsx's handleSubmit explicitly re-passing editingModel?.apiKey as a workaround — any other/future caller (e.g. a bare "enable/disable" toggle) that omits apiKey will lose the credential with no error or warning.
🐛 Proposed fix: preserve apiKey by default, require explicit `null` to clear
const modelId = normalizeLocalModelId(input.modelId)
const baseUrl = normalizeLocalModelBaseUrl(input.baseUrl)
const displayName = input.displayName?.trim() || modelId
- const apiKey = input.apiKey?.trim() || undefined
const id = input.id?.trim() || nextConfigId()
const existing = readStoredConfigs()
const previous = existing.find(config => config.id === id)
+ const apiKey =
+ input.apiKey === undefined ? previous?.apiKey : input.apiKey?.trim() || undefined
const next: LocalModelConfig = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function saveLocalModelConfig(input: SaveLocalModelConfigInput): LocalModelConfig { | |
| const modelId = normalizeLocalModelId(input.modelId) | |
| const baseUrl = normalizeLocalModelBaseUrl(input.baseUrl) | |
| const displayName = input.displayName?.trim() || modelId | |
| const apiKey = input.apiKey?.trim() || undefined | |
| const id = input.id?.trim() || nextConfigId() | |
| const existing = readStoredConfigs() | |
| const previous = existing.find(config => config.id === id) | |
| const next: LocalModelConfig = { | |
| id, | |
| displayName, | |
| modelId, | |
| baseUrl, | |
| apiKey, | |
| enabled: input.enabled ?? previous?.enabled ?? true, | |
| updatedAt: new Date().toISOString(), | |
| } | |
| const index = existing.findIndex(config => config.id === id) | |
| const configs = | |
| index >= 0 ? existing.map(config => (config.id === id ? next : config)) : [...existing, next] | |
| writeStoredConfigs(configs) | |
| return next | |
| } | |
| export function saveLocalModelConfig(input: SaveLocalModelConfigInput): LocalModelConfig { | |
| const modelId = normalizeLocalModelId(input.modelId) | |
| const baseUrl = normalizeLocalModelBaseUrl(input.baseUrl) | |
| const displayName = input.displayName?.trim() || modelId | |
| const id = input.id?.trim() || nextConfigId() | |
| const existing = readStoredConfigs() | |
| const previous = existing.find(config => config.id === id) | |
| const apiKey = | |
| input.apiKey === undefined ? previous?.apiKey : input.apiKey?.trim() || undefined | |
| const next: LocalModelConfig = { | |
| id, | |
| displayName, | |
| modelId, | |
| baseUrl, | |
| apiKey, | |
| enabled: input.enabled ?? previous?.enabled ?? true, | |
| updatedAt: new Date().toISOString(), | |
| } | |
| const index = existing.findIndex(config => config.id === id) | |
| const configs = | |
| index >= 0 ? existing.map(config => (config.id === id ? next : config)) : [...existing, next] | |
| writeStoredConfigs(configs) | |
| return next | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/features/model-settings/localModelSettings.ts` around lines 112 -
134, The update path in saveLocalModelConfig is wiping the stored apiKey when
callers omit it, unlike enabled which preserves the previous value. Move the
apiKey handling to after previous is read and default it to previous?.apiKey
when input.apiKey is undefined, while still allowing an explicit empty/null-like
value to clear it if that is the intended behavior. Keep the change localized to
saveLocalModelConfig and its SaveLocalModelConfigInput flow so future callers
don’t need to re-pass existing credentials.
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/src/api/local/localServices.ts (1)
801-805: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude a unique seed for new local sessions.
When
input.localTaskIdis absent, new sessions with the same runtime, workspace, and first message generate the same task/subtask IDs. IncludemessageIdin the new-session seed to avoid transcript/task collisions.Suggested fix
- const baseSeed = - input.localTaskId || `${input.runtime}:${input.workspacePath ?? ''}:${input.message}` + const baseSeed = + input.localTaskId || `${input.runtime}:${input.workspacePath ?? ''}:${input.messageId}` const [taskId, turnId] = createRuntimeExecutionIdsFromSeed( input.newSession ? baseSeed : `${baseSeed}:${input.messageId}` )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/api/local/localServices.ts` around lines 801 - 805, The seed used in local session ID generation is not unique enough when input.localTaskId is missing, causing new sessions with the same runtime/workspace/first message to reuse task and turn IDs. Update the seed logic in localServices around createRuntimeExecutionIdsFromSeed so that the new-session path also incorporates input.messageId into the base seed, and keep the existing non-new-session behavior unchanged.
🧹 Nitpick comments (2)
wework/src/components/chat/composer/ModelSelector.tsx (1)
649-651: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a selector test for the
unavailablebranch.This is a new user-visible edge case, and without a focused test it can easily drift back to the generic mismatch fallback. As per coding guidelines, "Run tests before committing and follow the project’s testing principles: AAA, mock external services, test edge cases, and keep tests isolated."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/chat/composer/ModelSelector.tsx` around lines 649 - 651, Add a focused selector test covering the `reason === 'unavailable'` branch in `ModelSelector` so this path stays distinct from the generic mismatch fallback. Extend the existing selector tests around the relevant mapping/helper that drives `t('workbench.model_disabled_unavailable', ...)`, using AAA style and isolated mocks, and assert that an unavailable model returns the unavailable message instead of the default fallback.Source: Coding guidelines
wework/src/features/workbench/runtimeModelSelection.test.ts (1)
65-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover disabled-model default selection.
The helper filters
compatibilityDisabled, but this priority test never places a disabled high-priority model before an enabled fallback. Add that assertion to prevent blocked local models from becoming defaults.Proposed test addition
expect(defaultNewChatModelSelection([officialWithoutAuth, cloudModel])).toMatchObject({ modelName: 'cloud-gpt', modelType: 'public', }) + expect( + defaultNewChatModelSelection([ + officialWithoutAuth, + { ...providerModel, compatibilityDisabled: true }, + interfaceModel, + cloudModel, + ]) + ).toMatchObject({ + modelName: 'local-model:ollama', + modelType: 'runtime', + })As per coding guidelines,
**/*.{py,ts,tsx,js,jsx}: Run tests before committing and follow the project’s testing principles: AAA, mock external services, test edge cases, and keep tests isolated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/features/workbench/runtimeModelSelection.test.ts` around lines 65 - 86, Add a test case in runtimeModelSelection.test.ts for defaultNewChatModelSelection that includes a higher-priority model marked compatibilityDisabled before an enabled fallback, and assert the disabled model is skipped in favor of the next eligible selection. Keep the new assertion alongside the existing priority cases so the behavior of defaultNewChatModelSelection and its compatibilityDisabled filtering is covered for blocked local models.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@executor/src/runtime_work/handler.rs`:
- Around line 298-300: The limit parsing in handler.rs currently casts oversized
positive values with `value as u32`, which can silently wrap; update the
`integer_field(...).filter(...).map(...)` handling to validate the parsed value
fits within `u32` before converting. Use the existing `integer_field` flow in
`handler::...` and either reject out-of-range values with an error or clamp them
explicitly, but do not use a direct narrowing cast.
In `@wework/src/api/local/localServices.ts`:
- Around line 123-128: The UI config in the model metadata builder is always
exposing the speed selector even when a model does not support fast mode. Update
the `ui` object in `localServices.ts` to gate `controls: ['speed']` on
`CodexOfficialModel.supportsFastMode` (or the parsed model capability), so
unsupported models do not advertise the control. Keep the existing
`modelFamily`, `providerFamilyLabel`, and `sortOrder` logic unchanged while
making the control list conditional.
- Around line 1599-1626: The `listModels` flow is still probing auth without the
resolved local device, so update it to resolve the local device once and pass
that device into `loadLocalCodexAuthConfigured` instead of calling it with only
`request`. Use the `listModels` async handler in `localServices.ts` and the
`loadLocalCodexAuthConfigured` helper as the key symbols to update, and make
sure the auth-status probe uses the same device instance that was resolved for
the model request.
- Around line 892-901: The runtime_auth_status request in
loadLocalCodexAuthConfigured is bypassing the same device-id normalization used
elsewhere in localServices, which can fail on executors that require a
device_id. Update this device.execute_command call to go through
normalizeLocalDeviceRecord (or otherwise attach the normalized local device id)
before invoking request, so the auth-status lookup uses the local device context
consistently and avoids falling back to false on transport errors.
In `@wework/src/components/settings/ModelSettingsPage.tsx`:
- Around line 1181-1195: The Codex official model loader updates consumers only
on success, so the error path in loadCodexOfficialModels should also notify
listeners. Update the catch branch in ModelSettingsPage’s
loadCodexOfficialModels callback to dispatch LOCAL_MODEL_SETTINGS_CHANGED_EVENT
after setting codexOfficialModels to null and codexOfficialError, so the
workbench selector can refresh when loading fails.
In `@wework/src/features/workbench/runtimeModelSelection.ts`:
- Around line 55-63: The local/cloud detection in modelExecutionSource and
isLocalModel is inconsistent when an explicit execution override is present.
Update isLocalModel so an explicit weworkExecution.source value of cloud (or any
non-local source) takes precedence over the provider fallback, and only fall
back to model.provider === 'local' when no explicit source is set. Keep the
logic aligned with isCloudModel so the same UnifiedModel cannot be both local
and cloud based on conflicting signals.
- Around line 93-104: The default selection logic in
defaultNewChatModelSelection is picking Codex-kind models without guaranteeing
they stay on the Codex runtime path. Update the selection criteria so
codex-official and codex-provider candidates are only chosen when
inferRuntimeName will resolve them as Codex, or add an explicit Codex runtime
guard using the same runtime-identifying fields that inferRuntimeName checks.
Keep the change localized to defaultNewChatModelSelection and any helper used by
inferRuntimeName so the default cannot resolve to claude_code for Codex-shaped
models.
---
Outside diff comments:
In `@wework/src/api/local/localServices.ts`:
- Around line 801-805: The seed used in local session ID generation is not
unique enough when input.localTaskId is missing, causing new sessions with the
same runtime/workspace/first message to reuse task and turn IDs. Update the seed
logic in localServices around createRuntimeExecutionIdsFromSeed so that the
new-session path also incorporates input.messageId into the base seed, and keep
the existing non-new-session behavior unchanged.
---
Nitpick comments:
In `@wework/src/components/chat/composer/ModelSelector.tsx`:
- Around line 649-651: Add a focused selector test covering the `reason ===
'unavailable'` branch in `ModelSelector` so this path stays distinct from the
generic mismatch fallback. Extend the existing selector tests around the
relevant mapping/helper that drives `t('workbench.model_disabled_unavailable',
...)`, using AAA style and isolated mocks, and assert that an unavailable model
returns the unavailable message instead of the default fallback.
In `@wework/src/features/workbench/runtimeModelSelection.test.ts`:
- Around line 65-86: Add a test case in runtimeModelSelection.test.ts for
defaultNewChatModelSelection that includes a higher-priority model marked
compatibilityDisabled before an enabled fallback, and assert the disabled model
is skipped in favor of the next eligible selection. Keep the new assertion
alongside the existing priority cases so the behavior of
defaultNewChatModelSelection and its compatibilityDisabled filtering is covered
for blocked local models.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d69487ff-b76b-456a-9191-07ebaa3ed851
📒 Files selected for processing (25)
docs/en/developer-guide/wework-cloud-connection.mddocs/zh/developer-guide/wework-cloud-connection.mdexecutor/src/agents/codex.rsexecutor/src/runtime_work/handler.rswework/src/api/hybrid/hybridServices.test.tswework/src/api/hybrid/hybridServices.tswework/src/api/local/codexOfficialModels.tswework/src/api/local/localServices.test.tswework/src/api/local/localServices.tswework/src/components/chat/composer/ModelSelector.tsxwework/src/components/settings/ConnectionsSettingsPage.test.tsxwework/src/components/settings/ConnectionsSettingsPage.tsxwework/src/components/settings/ModelSettingsPage.tsxwework/src/features/model-settings/codexOfficialModels.tswework/src/features/model-settings/localModelSettings.test.tswework/src/features/model-settings/localModelSettings.tswework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/runtimeModelSelection.test.tswework/src/features/workbench/runtimeModelSelection.tswework/src/features/workbench/workbenchProviderHelpers.tswework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/lib/model-ui.test.tswework/src/lib/model-ui.tswework/src/types/api.ts
✅ Files skipped from review due to trivial changes (1)
- docs/zh/developer-guide/wework-cloud-connection.md
🚧 Files skipped from review as they are similar to previous changes (7)
- wework/src/components/settings/ConnectionsSettingsPage.tsx
- wework/src/i18n/locales/en/common.json
- wework/src/i18n/locales/zh-CN/common.json
- docs/en/developer-guide/wework-cloud-connection.md
- wework/src/features/model-settings/localModelSettings.ts
- wework/src/api/local/localServices.test.ts
- wework/src/features/model-settings/localModelSettings.test.ts
Comment on lines
+298
to
+300
| let limit = integer_field(&payload, "limit") | ||
| .filter(|value| *value > 0) | ||
| .map(|value| value as u32); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid truncating oversized model-list limits.
value as u32 silently wraps large positive JSON integers. Reject or clamp values that do not fit in u32 before forwarding them.
Proposed fix
let limit = integer_field(&payload, "limit")
.filter(|value| *value > 0)
- .map(|value| value as u32);
+ .and_then(|value| u32::try_from(value).ok());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let limit = integer_field(&payload, "limit") | |
| .filter(|value| *value > 0) | |
| .map(|value| value as u32); | |
| let limit = integer_field(&payload, "limit") | |
| .filter(|value| *value > 0) | |
| .and_then(|value| u32::try_from(value).ok()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/runtime_work/handler.rs` around lines 298 - 300, The limit
parsing in handler.rs currently casts oversized positive values with `value as
u32`, which can silently wrap; update the
`integer_field(...).filter(...).map(...)` handling to validate the parsed value
fits within `u32` before converting. Use the existing `integer_field` flow in
`handler::...` and either reject out-of-range values with an error or clamp them
explicitly, but do not use a direct narrowing cast.
Comment on lines
+123
to
+128
| ui: { | ||
| family: modelFamily, | ||
| ...(providerFamilyLabel ? { familyLabel: providerFamilyLabel } : {}), | ||
| modelLabel: model.modelId, | ||
| controls: ['speed'], | ||
| sortOrder: model.providerType === 'provider' ? 15 : 10, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate the speed control by model capability.
CodexOfficialModel.supportsFastMode is parsed but ignored here, so models that do not advertise fast-mode support still expose the speed selector and can produce unsupported execution options.
Suggested fix
ui: {
family: modelFamily,
...(providerFamilyLabel ? { familyLabel: providerFamilyLabel } : {}),
modelLabel: model.modelId,
- controls: ['speed'],
+ controls: model.supportsFastMode ? ['speed'] : [],
sortOrder: model.providerType === 'provider' ? 15 : 10,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ui: { | |
| family: modelFamily, | |
| ...(providerFamilyLabel ? { familyLabel: providerFamilyLabel } : {}), | |
| modelLabel: model.modelId, | |
| controls: ['speed'], | |
| sortOrder: model.providerType === 'provider' ? 15 : 10, | |
| ui: { | |
| family: modelFamily, | |
| ...(providerFamilyLabel ? { familyLabel: providerFamilyLabel } : {}), | |
| modelLabel: model.modelId, | |
| controls: model.supportsFastMode ? ['speed'] : [], | |
| sortOrder: model.providerType === 'provider' ? 15 : 10, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/api/local/localServices.ts` around lines 123 - 128, The UI config
in the model metadata builder is always exposing the speed selector even when a
model does not support fast mode. Update the `ui` object in `localServices.ts`
to gate `controls: ['speed']` on `CodexOfficialModel.supportsFastMode` (or the
parsed model capability), so unsupported models do not advertise the control.
Keep the existing `modelFamily`, `providerFamilyLabel`, and `sortOrder` logic
unchanged while making the control list conditional.
Comment on lines
+892
to
+901
| async function loadLocalCodexAuthConfigured( | ||
| request: LocalAppServicesDeps['request'] | ||
| ): Promise<boolean> { | ||
| if (!request) return false | ||
| try { | ||
| const response = await request<DeviceCommandResponse>('device.execute_command', { | ||
| command_key: 'runtime_auth_status', | ||
| timeout_seconds: 10, | ||
| max_output_bytes: 4096, | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize the auth-status command with the local device id.
Other device.execute_command calls in this service go through normalizeLocalDeviceRecord; this raw request can fail on executors that require device_id, then silently reports codexAuthConfigured = false.
Suggested fix
async function loadLocalCodexAuthConfigured(
- request: LocalAppServicesDeps['request']
+ request: LocalAppServicesDeps['request'],
+ localDeviceId: string
): Promise<boolean> {
if (!request) return false
try {
- const response = await request<DeviceCommandResponse>('device.execute_command', {
- command_key: 'runtime_auth_status',
- timeout_seconds: 10,
- max_output_bytes: 4096,
- })
+ const response = await request<DeviceCommandResponse>(
+ 'device.execute_command',
+ normalizeLocalDeviceRecord(
+ {
+ command_key: 'runtime_auth_status',
+ timeout_seconds: 10,
+ max_output_bytes: 4096,
+ },
+ localDeviceId
+ )
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function loadLocalCodexAuthConfigured( | |
| request: LocalAppServicesDeps['request'] | |
| ): Promise<boolean> { | |
| if (!request) return false | |
| try { | |
| const response = await request<DeviceCommandResponse>('device.execute_command', { | |
| command_key: 'runtime_auth_status', | |
| timeout_seconds: 10, | |
| max_output_bytes: 4096, | |
| }) | |
| async function loadLocalCodexAuthConfigured( | |
| request: LocalAppServicesDeps['request'], | |
| localDeviceId: string | |
| ): Promise<boolean> { | |
| if (!request) return false | |
| try { | |
| const response = await request<DeviceCommandResponse>( | |
| 'device.execute_command', | |
| normalizeLocalDeviceRecord( | |
| { | |
| command_key: 'runtime_auth_status', | |
| timeout_seconds: 10, | |
| max_output_bytes: 4096, | |
| }, | |
| localDeviceId | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/api/local/localServices.ts` around lines 892 - 901, The
runtime_auth_status request in loadLocalCodexAuthConfigured is bypassing the
same device-id normalization used elsewhere in localServices, which can fail on
executors that require a device_id. Update this device.execute_command call to
go through normalizeLocalDeviceRecord (or otherwise attach the normalized local
device id) before invoking request, so the auth-status lookup uses the local
device context consistently and avoids falling back to false on transport
errors.
Comment on lines
+1599
to
+1626
| listModels: async () => { | ||
| let codexOfficialModels: CodexOfficialModel[] | ||
| let codexOfficialError: string | null | ||
| let codexAuthConfigured: boolean | ||
| try { | ||
| await ensureStatus() | ||
| const [codexOfficialResult, nextCodexAuthConfigured] = await Promise.all([ | ||
| requestLocalCodexOfficialModels(request).then( | ||
| value => ({ value, error: null }), | ||
| error => ({ | ||
| value: null, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }) | ||
| ), | ||
| loadLocalCodexAuthConfigured(request), | ||
| ]) | ||
| codexOfficialModels = codexOfficialResult.value?.models ?? [] | ||
| codexOfficialError = codexOfficialResult.error | ||
| codexAuthConfigured = nextCodexAuthConfigured | ||
| } catch (error) { | ||
| codexOfficialModels = [] | ||
| codexOfficialError = error instanceof Error ? error.message : String(error) | ||
| codexAuthConfigured = false | ||
| } | ||
| return { | ||
| data: localRuntimeModels(codexOfficialModels, codexOfficialError, codexAuthConfigured), | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the resolved local device id into the auth probe.
After updating loadLocalCodexAuthConfigured, resolve the local device once and pass it to the auth-status probe; otherwise model-list metadata can still report auth as unavailable.
Suggested fix
- await ensureStatus()
+ const localDeviceId = await getLocalDeviceId()
const [codexOfficialResult, nextCodexAuthConfigured] = await Promise.all([
requestLocalCodexOfficialModels(request).then(
value => ({ value, error: null }),
@@
- loadLocalCodexAuthConfigured(request),
+ loadLocalCodexAuthConfigured(request, localDeviceId),
])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| listModels: async () => { | |
| let codexOfficialModels: CodexOfficialModel[] | |
| let codexOfficialError: string | null | |
| let codexAuthConfigured: boolean | |
| try { | |
| await ensureStatus() | |
| const [codexOfficialResult, nextCodexAuthConfigured] = await Promise.all([ | |
| requestLocalCodexOfficialModels(request).then( | |
| value => ({ value, error: null }), | |
| error => ({ | |
| value: null, | |
| error: error instanceof Error ? error.message : String(error), | |
| }) | |
| ), | |
| loadLocalCodexAuthConfigured(request), | |
| ]) | |
| codexOfficialModels = codexOfficialResult.value?.models ?? [] | |
| codexOfficialError = codexOfficialResult.error | |
| codexAuthConfigured = nextCodexAuthConfigured | |
| } catch (error) { | |
| codexOfficialModels = [] | |
| codexOfficialError = error instanceof Error ? error.message : String(error) | |
| codexAuthConfigured = false | |
| } | |
| return { | |
| data: localRuntimeModels(codexOfficialModels, codexOfficialError, codexAuthConfigured), | |
| } | |
| }, | |
| listModels: async () => { | |
| let codexOfficialModels: CodexOfficialModel[] | |
| let codexOfficialError: string | null | |
| let codexAuthConfigured: boolean | |
| try { | |
| const localDeviceId = await getLocalDeviceId() | |
| const [codexOfficialResult, nextCodexAuthConfigured] = await Promise.all([ | |
| requestLocalCodexOfficialModels(request).then( | |
| value => ({ value, error: null }), | |
| error => ({ | |
| value: null, | |
| error: error instanceof Error ? error.message : String(error), | |
| }) | |
| ), | |
| loadLocalCodexAuthConfigured(request, localDeviceId), | |
| ]) | |
| codexOfficialModels = codexOfficialResult.value?.models ?? [] | |
| codexOfficialError = codexOfficialResult.error | |
| codexAuthConfigured = nextCodexAuthConfigured | |
| } catch (error) { | |
| codexOfficialModels = [] | |
| codexOfficialError = error instanceof Error ? error.message : String(error) | |
| codexAuthConfigured = false | |
| } | |
| return { | |
| data: localRuntimeModels(codexOfficialModels, codexOfficialError, codexAuthConfigured), | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/api/local/localServices.ts` around lines 1599 - 1626, The
`listModels` flow is still probing auth without the resolved local device, so
update it to resolve the local device once and pass that device into
`loadLocalCodexAuthConfigured` instead of calling it with only `request`. Use
the `listModels` async handler in `localServices.ts` and the
`loadLocalCodexAuthConfigured` helper as the key symbols to update, and make
sure the auth-status probe uses the same device instance that was resolved for
the model request.
Comment on lines
+1181
to
+1195
| const loadCodexOfficialModels = useCallback(async () => { | ||
| setCodexOfficialLoading(true) | ||
| setCodexOfficialError(null) | ||
| try { | ||
| const nextModels = await getLocalCodexOfficialModels() | ||
| setCodexOfficialModels(nextModels) | ||
| window.dispatchEvent(new CustomEvent(LOCAL_MODEL_SETTINGS_CHANGED_EVENT)) | ||
| } catch (loadError) { | ||
| setCodexOfficialModels(null) | ||
| setCodexOfficialError( | ||
| getErrorMessage(loadError, t('workbench.codex_official_models_load_failed')) | ||
| ) | ||
| } finally { | ||
| setCodexOfficialLoading(false) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Refresh model consumers when Codex model loading fails too.
The success path dispatches LOCAL_MODEL_SETTINGS_CHANGED_EVENT, but the error path does not. That can leave the workbench selector showing stale official Codex models while this page shows the list as unavailable.
Suggested fix
try {
const nextModels = await getLocalCodexOfficialModels()
setCodexOfficialModels(nextModels)
- window.dispatchEvent(new CustomEvent(LOCAL_MODEL_SETTINGS_CHANGED_EVENT))
} catch (loadError) {
setCodexOfficialModels(null)
setCodexOfficialError(
getErrorMessage(loadError, t('workbench.codex_official_models_load_failed'))
)
} finally {
+ window.dispatchEvent(new CustomEvent(LOCAL_MODEL_SETTINGS_CHANGED_EVENT))
setCodexOfficialLoading(false)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const loadCodexOfficialModels = useCallback(async () => { | |
| setCodexOfficialLoading(true) | |
| setCodexOfficialError(null) | |
| try { | |
| const nextModels = await getLocalCodexOfficialModels() | |
| setCodexOfficialModels(nextModels) | |
| window.dispatchEvent(new CustomEvent(LOCAL_MODEL_SETTINGS_CHANGED_EVENT)) | |
| } catch (loadError) { | |
| setCodexOfficialModels(null) | |
| setCodexOfficialError( | |
| getErrorMessage(loadError, t('workbench.codex_official_models_load_failed')) | |
| ) | |
| } finally { | |
| setCodexOfficialLoading(false) | |
| } | |
| const loadCodexOfficialModels = useCallback(async () => { | |
| setCodexOfficialLoading(true) | |
| setCodexOfficialError(null) | |
| try { | |
| const nextModels = await getLocalCodexOfficialModels() | |
| setCodexOfficialModels(nextModels) | |
| } catch (loadError) { | |
| setCodexOfficialModels(null) | |
| setCodexOfficialError( | |
| getErrorMessage(loadError, t('workbench.codex_official_models_load_failed')) | |
| ) | |
| } finally { | |
| window.dispatchEvent(new CustomEvent(LOCAL_MODEL_SETTINGS_CHANGED_EVENT)) | |
| setCodexOfficialLoading(false) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/components/settings/ModelSettingsPage.tsx` around lines 1181 -
1195, The Codex official model loader updates consumers only on success, so the
error path in loadCodexOfficialModels should also notify listeners. Update the
catch branch in ModelSettingsPage’s loadCodexOfficialModels callback to dispatch
LOCAL_MODEL_SETTINGS_CHANGED_EVENT after setting codexOfficialModels to null and
codexOfficialError, so the workbench selector can refresh when loading fails.
Comment on lines
+55
to
+63
| function modelExecutionSource(model: UnifiedModel): string { | ||
| const override = getObjectConfigValue(model.config, MODEL_EXECUTION_CONFIG_KEY) | ||
| const source = override?.source | ||
| return typeof source === 'string' ? source : '' | ||
| } | ||
|
|
||
| function isLocalModel(model: UnifiedModel): boolean { | ||
| return modelExecutionSource(model) === 'local' || model.provider === 'local' | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor explicit cloud execution overrides for local detection.
isLocalModel still returns true when weworkExecution.source === 'cloud' and provider === 'local', while isCloudModel also returns true for the same model. Let the explicit source override the provider fallback.
Proposed fix
function isLocalModel(model: UnifiedModel): boolean {
- return modelExecutionSource(model) === 'local' || model.provider === 'local'
+ const source = modelExecutionSource(model)
+ if (source === 'local') return true
+ if (source === 'cloud') return false
+ return model.provider === 'local'
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function modelExecutionSource(model: UnifiedModel): string { | |
| const override = getObjectConfigValue(model.config, MODEL_EXECUTION_CONFIG_KEY) | |
| const source = override?.source | |
| return typeof source === 'string' ? source : '' | |
| } | |
| function isLocalModel(model: UnifiedModel): boolean { | |
| return modelExecutionSource(model) === 'local' || model.provider === 'local' | |
| } | |
| function modelExecutionSource(model: UnifiedModel): string { | |
| const override = getObjectConfigValue(model.config, MODEL_EXECUTION_CONFIG_KEY) | |
| const source = override?.source | |
| return typeof source === 'string' ? source : '' | |
| } | |
| function isLocalModel(model: UnifiedModel): boolean { | |
| const source = modelExecutionSource(model) | |
| if (source === 'local') return true | |
| if (source === 'cloud') return false | |
| return model.provider === 'local' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/features/workbench/runtimeModelSelection.ts` around lines 55 - 63,
The local/cloud detection in modelExecutionSource and isLocalModel is
inconsistent when an explicit execution override is present. Update isLocalModel
so an explicit weworkExecution.source value of cloud (or any non-local source)
takes precedence over the provider fallback, and only fall back to
model.provider === 'local' when no explicit source is set. Keep the logic
aligned with isCloudModel so the same UnifiedModel cannot be both local and
cloud based on conflicting signals.
Comment on lines
+93
to
+104
| export function defaultNewChatModelSelection(models: UnifiedModel[]): ModelSelectionConfig | null { | ||
| const candidates = models.filter(model => !model.compatibilityDisabled) | ||
| const selected = | ||
| candidates.find( | ||
| model => | ||
| isLocalModel(model) && | ||
| modelKind(model) === 'codex-official' && | ||
| getBooleanConfigValue(model.config, 'codexAuthConfigured') | ||
| ) ?? | ||
| candidates.find(model => isLocalModel(model) && modelKind(model) === 'codex-provider') ?? | ||
| candidates.find(model => isLocalModel(model) && modelKind(model) === 'model-interface') ?? | ||
| candidates.find(isCloudModel) ?? |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep Codex-kind defaults on the Codex runtime path.
This now defaults to codex-official and codex-provider models, but inferRuntimeName only treats protocol/api-format compatibility as Codex. Models shaped like the new tests can be selected by default and then inferred as claude_code.
Proposed fix
function isCodexCompatibleModel(model: UnifiedModel): boolean {
+ const kind = modelKind(model)
return (
+ kind === 'codex-official' ||
+ kind === 'codex-provider' ||
getModelCompatibilityFamily(model) === OPENAI_RESPONSES_RUNTIME_FAMILY ||
getStringConfigValue(model.config, 'protocol') === OPENAI_RESPONSES_PROTOCOL ||
getStringConfigValue(model.config, 'apiFormat') === RESPONSES_API_FORMAT ||🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/features/workbench/runtimeModelSelection.ts` around lines 93 -
104, The default selection logic in defaultNewChatModelSelection is picking
Codex-kind models without guaranteeing they stay on the Codex runtime path.
Update the selection criteria so codex-official and codex-provider candidates
are only chosen when inferRuntimeName will resolve them as Codex, or add an
explicit Codex runtime guard using the same runtime-identifying fields that
inferRuntimeName checks. Keep the change localized to
defaultNewChatModelSelection and any helper used by inferRuntimeName so the
default cannot resolve to claude_code for Codex-shaped models.
This was referenced Jul 5, 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
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.
Summary
runtime.codex.models.list; provider groups use the provider name shown in Codex config.model/listonce, and reports an error when that catalog is unavailable instead of falling back to official models.executionRequestpayloads and updated executor validation.Verification
pnpm --dir wework test src/lib/model-ui.test.ts src/api/local/localServices.test.ts src/features/workbench/runtimeModelSelection.test.ts -- --runInBandpnpm --dir wework exec tsc --noEmitcargo test --manifest-path executor/Cargo.toml --lib current_codex_model_provider -qcargo check --manifest-path executor/Cargo.toml -qcargo test --manifest-path executor/Cargo.toml --lib -qcargo fmt --check,cargo test --all-features, andcargo clippySummary by CodeRabbit
executionRequestas required for runtime task create/send.