Repository navigation
fix(wework): refine task model switching compatibility - #1229
Merged
qdaxb merged 1 commit intoJun 3, 2026
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR implements runtime model compatibility validation using ChangesRuntime Model Compatibility and Selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/app/services/chat/config/model_compatibility.py (1)
30-49: 💤 Low valueConsider adding a docstring to clarify intent.
The function extracts protocol/provider signals from various nested fields, but the purpose isn't immediately clear from the name alone. A brief docstring would improve maintainability.
📝 Suggested docstring
def _model_signals(model_spec: dict[str, Any]) -> list[str]: + """Extract protocol/provider identifier candidates from model spec fields.""" model_config = model_spec.get("modelConfig") or {}🤖 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 `@backend/app/services/chat/config/model_compatibility.py` around lines 30 - 49, Add a concise docstring to _model_signals(model_spec: dict[str, Any]) explaining that the function collects protocol/provider identification signals from the top-level modelSpec and its nested modelConfig and env maps, list the specific keys returned (protocol, apiFormat, api_format, wire_api, model, model_id, base_url) and note that it normalizes missing or non-dict entries to empty dicts; place the docstring immediately below the def line to clarify intent and expected input/output for future maintainers.wework/src/features/workbench/useWorkbenchModels.ts (1)
96-100: 💤 Low valueConsider renaming
restoreSelection's parameter for clarity.The
restoreSelectioncallback (line 133) has a parameter namedavailableModels, but it's called at line 187 with the filteredmodelslist. While functionally correct (the parameter serves as the search space forfindConfiguredModel), the naming is confusing since there's now a separateavailableModelsstate variable (line 96) that holds the unfiltered list. Consider renaming the parameter tomodelsToSearchorcandidateModelsto avoid confusion.🤖 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 96 - 100, The parameter name of the restoreSelection callback is confusing because it collides with the component state availableModels; rename the parameter (e.g., to modelsToSearch or candidateModels) in the restoreSelection function declaration and update its internal references (where it calls findConfiguredModel) as well as the call site that currently passes the filtered models array (models) so it reads clearly that the filtered list is being searched; ensure any TypeScript types (UnifiedModel[]) and exported references remain consistent after the rename.
🤖 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.
Nitpick comments:
In `@backend/app/services/chat/config/model_compatibility.py`:
- Around line 30-49: Add a concise docstring to _model_signals(model_spec:
dict[str, Any]) explaining that the function collects protocol/provider
identification signals from the top-level modelSpec and its nested modelConfig
and env maps, list the specific keys returned (protocol, apiFormat, api_format,
wire_api, model, model_id, base_url) and note that it normalizes missing or
non-dict entries to empty dicts; place the docstring immediately below the def
line to clarify intent and expected input/output for future maintainers.
In `@wework/src/features/workbench/useWorkbenchModels.ts`:
- Around line 96-100: The parameter name of the restoreSelection callback is
confusing because it collides with the component state availableModels; rename
the parameter (e.g., to modelsToSearch or candidateModels) in the
restoreSelection function declaration and update its internal references (where
it calls findConfiguredModel) as well as the call site that currently passes the
filtered models array (models) so it reads clearly that the filtered list is
being searched; ensure any TypeScript types (UnifiedModel[]) and exported
references remain consistent after the rename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 782c32a7-da22-4c50-81c4-8ff2ba4bb974
📒 Files selected for processing (23)
backend/app/api/ws/chat_namespace.pybackend/app/services/chat/config/model_compatibility.pybackend/app/services/chat/trigger/lifecycle.pybackend/tests/services/chat/test_model_compatibility.pydocs/en/user-guide/chat/README.mddocs/zh/user-guide/chat/README.mdfrontend/src/__tests__/features/tasks/hooks/useModelSelection.test.tsxfrontend/src/__tests__/utils/modelCompatibility.test.tsfrontend/src/__tests__/utils/modelPreferences.test.tsfrontend/src/features/tasks/hooks/useModelSelection.tsfrontend/src/utils/modelCompatibility.tsfrontend/src/utils/modelPreferences.tswework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/useWorkbenchModels.tswework/src/features/workbench/workbenchProjectChatHooks.test.tsxwework/src/features/workbench/workbenchReducer.test.tswework/src/features/workbench/workbenchReducer.tswework/src/lib/model-ui.test.tswework/src/lib/model-ui.ts
icycrystal4
force-pushed
the
codex/task-model-switch-compatibility
branch
from
June 2, 2026 08:55
1ce8c26 to
97882bb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/app/services/chat/config/model_compatibility.py (1)
39-46: ⚡ Quick winAdd
@trace_syncspans for model compatibility validation
get_model_compatibility_family(39-46) andensure_task_model_override_compatible(98-106) are public entry points but contain no@trace_syncinstrumentation, so model switch resolution/rejection details won’t surface in traces.
- Decorate both with
@trace_sync, and record resolved families + rejection reason (unknown family / family mismatch) viaset_span_attribute()/add_span_event().🤖 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 `@backend/app/services/chat/config/model_compatibility.py` around lines 39 - 46, Add tracing spans to the public model-compatibility entrypoints by decorating get_model_compatibility_family and ensure_task_model_override_compatible with `@trace_sync`, and inside each function set span attributes and events: use set_span_attribute to record the resolved family (the value returned from _normalize(_model_env(model_spec).get("model")) in get_model_compatibility_family and the chosen family in ensure_task_model_override_compatible) and use add_span_event to record rejection reasons such as "unknown family" or "family mismatch" when applicable; ensure the span attribute keys and event messages are clear and include the family string and rejection cause.
🤖 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 `@backend/app/services/chat/config/model_compatibility.py`:
- Around line 49-54: The code in _extract_task_model_id (and similarly in
_get_bot_default_model_name) can crash when task.json contains "metadata": None
or "spec": None because task_json.get("metadata", {}) returns None not a dict;
change the lookups to coerce possibly-None values to dicts before .get calls
(e.g., use (task_json.get("metadata") or {}) and (task_json.get("spec") or {})
so you then call .get("labels", {}) or .get("modelId") safely) and apply the
same pattern in _get_bot_default_model_name to guard against spec being None.
---
Nitpick comments:
In `@backend/app/services/chat/config/model_compatibility.py`:
- Around line 39-46: Add tracing spans to the public model-compatibility
entrypoints by decorating get_model_compatibility_family and
ensure_task_model_override_compatible with `@trace_sync`, and inside each function
set span attributes and events: use set_span_attribute to record the resolved
family (the value returned from _normalize(_model_env(model_spec).get("model"))
in get_model_compatibility_family and the chosen family in
ensure_task_model_override_compatible) and use add_span_event to record
rejection reasons such as "unknown family" or "family mismatch" when applicable;
ensure the span attribute keys and event messages are clear and include the
family string and rejection cause.
🪄 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: 8ba2d03c-85d3-4f63-9201-b098d8015148
📒 Files selected for processing (23)
backend/app/api/ws/chat_namespace.pybackend/app/services/chat/config/model_compatibility.pybackend/app/services/chat/trigger/lifecycle.pybackend/tests/services/chat/test_model_compatibility.pydocs/en/user-guide/chat/README.mddocs/zh/user-guide/chat/README.mdfrontend/src/__tests__/features/tasks/hooks/useModelSelection.test.tsxfrontend/src/__tests__/utils/modelCompatibility.test.tsfrontend/src/__tests__/utils/modelPreferences.test.tsfrontend/src/features/tasks/hooks/useModelSelection.tsfrontend/src/utils/modelCompatibility.tsfrontend/src/utils/modelPreferences.tswework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/useWorkbenchModels.tswework/src/features/workbench/workbenchProjectChatHooks.test.tsxwework/src/features/workbench/workbenchReducer.test.tswework/src/features/workbench/workbenchReducer.tswework/src/lib/model-ui.test.tswework/src/lib/model-ui.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- wework/src/components/layout/MobileWorkbenchLayout.tsx
- wework/src/components/chat/composer/ComposerToolbar.tsx
- frontend/src/tests/utils/modelCompatibility.test.ts
- frontend/src/tests/utils/modelPreferences.test.ts
- docs/zh/user-guide/chat/README.md
- docs/en/user-guide/chat/README.md
- wework/src/lib/model-ui.test.ts
- wework/src/features/workbench/WorkbenchProvider.test.tsx
- wework/src/features/workbench/WorkbenchProvider.tsx
- wework/src/features/workbench/workbenchReducer.ts
- wework/src/features/workbench/workbenchReducer.test.ts
- wework/src/features/workbench/useWorkbenchModels.ts
- frontend/src/utils/modelPreferences.ts
- frontend/src/tests/features/tasks/hooks/useModelSelection.test.tsx
- backend/app/api/ws/chat_namespace.py
- frontend/src/features/tasks/hooks/useModelSelection.ts
- frontend/src/utils/modelCompatibility.ts
Comment on lines
+49
to
+54
| def _extract_task_model_id(task: TaskResource) -> Optional[str]: | ||
| task_json = task.json or {} | ||
| labels = task_json.get("metadata", {}).get("labels", {}) or {} | ||
| spec = task_json.get("spec", {}) or {} | ||
| model_id = labels.get("modelId") or spec.get("modelId") | ||
| return model_id if isinstance(model_id, str) and model_id.strip() else None |
Contributor
There was a problem hiding this comment.
Guard against metadata/spec being explicitly None.
task_json.get("metadata", {}) only returns the default {} when the key is absent; if task.json contains "metadata": None, this returns None and the chained .get("labels", {}) raises AttributeError. The trailing or {} is applied too late to help. Note spec on Line 52 already uses the or {} pattern correctly, so the two are inconsistent.
🛡️ Proposed fix
- task_json = task.json or {}
- labels = task_json.get("metadata", {}).get("labels", {}) or {}
- spec = task_json.get("spec", {}) or {}
+ task_json = task.json or {}
+ metadata = task_json.get("metadata") or {}
+ labels = metadata.get("labels") or {}
+ spec = task_json.get("spec") or {}🤖 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 `@backend/app/services/chat/config/model_compatibility.py` around lines 49 -
54, The code in _extract_task_model_id (and similarly in
_get_bot_default_model_name) can crash when task.json contains "metadata": None
or "spec": None because task_json.get("metadata", {}) returns None not a dict;
change the lookups to coerce possibly-None values to dicts before .get calls
(e.g., use (task_json.get("metadata") or {}) and (task_json.get("spec") or {})
so you then call .get("labels", {}) or .get("modelId") safely) and apply the
same pattern in _get_bot_default_model_name to guard against spec being None.
Comment on lines
+184
to
+192
| export function areModelsProtocolCompatible( | ||
| currentModel?: ModelCompatibilitySource | null, | ||
| nextModel?: ModelCompatibilitySource | null, | ||
| ): boolean { | ||
| const currentFamily = getModelCompatibilityFamily(currentModel) | ||
| const nextFamily = getModelCompatibilityFamily(nextModel) | ||
| if (!currentFamily || !nextFamily) return false | ||
| return currentFamily === nextFamily | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how useWorkbenchModels applies areModelsProtocolCompatible,
# especially the branch when the current/base task model has no inferred family.
fd -t f 'useWorkbenchModels.ts' --exec cat {}
rg -nP -C4 'areModelsProtocolCompatible|getModelCompatibilityFamily' --type=tsRepository: wecode-ai/Wegent
Length of output: 23304
Avoid hard-blocking model switching when the current model’s family is unknown
wework/src/features/workbench/useWorkbenchModels.ts’s filterModelsByCompatibility() returns only [currentModel] when getModelCompatibilityFamily(currentModel) is missing, so unknown/custom current models can’t switch to any other candidate.
wework/src/lib/model-ui.test.ts coverage shown for areModelsProtocolCompatible() only demonstrates the “missing family in next model” case (e.g., known current vs unknownKimi), not the “unknown current” short-circuit that blocks alternatives in useWorkbenchModels.
Adjust the filtering logic so unknown current families don’t exclude all alternatives, and add a test for the unknown-current path in useWorkbenchModels.
icycrystal4
force-pushed
the
codex/task-model-switch-compatibility
branch
from
June 2, 2026 09:12
97882bb to
194fc6c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wework/src/lib/model-ui.ts (1)
184-192:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Unknown current models still block all switching
The past review comment on lines 184-192 flagged that unknown/custom current models (those without
config.env.model) cannot switch to any other model. This issue remains unaddressed.When
currentFamilyisnullat line 188 (current model has noenv.model), this function returnsfalsefor every candidate, souseWorkbenchModels.tsmarks all alternatives ascompatibilityDisabled(viagetCompatibilityDisabledReasonreturning'missing_current_env_model'), and line 231 of that hook blocks selection.Users stuck on unknown/custom models cannot switch away.
Recommended fix to allow unknown families
export function areModelsProtocolCompatible( currentModel?: ModelCompatibilitySource | null, nextModel?: ModelCompatibilitySource | null, ): boolean { const currentFamily = getModelCompatibilityFamily(currentModel) const nextFamily = getModelCompatibilityFamily(nextModel) - if (!currentFamily || !nextFamily) return false + if (!currentFamily || !nextFamily) return true return currentFamily === nextFamily }This change allows switches when either model lacks an
env.modelsignal, preserving strict validation only when both families are known.🤖 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/lib/model-ui.ts` around lines 184 - 192, areModelsProtocolCompatible currently returns false when either getModelCompatibilityFamily(currentModel) or getModelCompatibilityFamily(nextModel) is null, which blocks switching away from unknown/custom current models; change areModelsProtocolCompatible to treat a missing family as permissive (i.e., if either currentFamily or nextFamily is null return true) so only when both families are known do you enforce strict equality; update logic in areModelsProtocolCompatible (and ensure any callers like useWorkbenchModels.ts / getCompatibilityDisabledReason that rely on this output will then stop marking alternatives as 'missing_current_env_model').
🤖 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/features/workbench/useWorkbenchModels.ts`:
- Around line 85-100: getCompatibilityDisabledReason currently blocks all
alternatives when getModelCompatibilityFamily(currentModel) is null; change it
so unknown families do not disable switching: keep the isSameModel check, then
fetch currentFamily and nextFamily via getModelCompatibilityFamily, and if
either family is null return null (allow switch). Only when both families are
present should you call areModelsProtocolCompatible and return
'env_model_mismatch' if incompatible; remove the early returns
'missing_current_env_model' and 'missing_target_env_model' so
annotateModelsByCompatibility won't mark all alternatives compatibilityDisabled.
In `@wework/src/features/workbench/workbenchProjectChatHooks.test.tsx`:
- Around line 47-221: Add a new test for the "unknown current model" path in
useWorkbenchModels: create a mock model list where the
selected/compatibilityConfig model has no config.env.model (a custom/unknown
model), call useWorkbenchModels with selectionConfig and compatibilityConfig
pointing to that model, await the models to load (renderHook + waitFor), and
assert the expected compatibility annotations by inspecting
result.current.models for compatibilityDisabled and compatibilityDisabledReason
(use the same pattern as existing tests to map model.name and
compatibilityDisabled flags); ensure the test name and setup reference
useWorkbenchModels, selectionConfig, compatibilityConfig, compatibilityDisabled
and compatibilityDisabledReason so it verifies whether other models are marked
disabled when the current model lacks env.model.
---
Duplicate comments:
In `@wework/src/lib/model-ui.ts`:
- Around line 184-192: areModelsProtocolCompatible currently returns false when
either getModelCompatibilityFamily(currentModel) or
getModelCompatibilityFamily(nextModel) is null, which blocks switching away from
unknown/custom current models; change areModelsProtocolCompatible to treat a
missing family as permissive (i.e., if either currentFamily or nextFamily is
null return true) so only when both families are known do you enforce strict
equality; update logic in areModelsProtocolCompatible (and ensure any callers
like useWorkbenchModels.ts / getCompatibilityDisabledReason that rely on this
output will then stop marking alternatives as 'missing_current_env_model').
🪄 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: 735fd1e8-9a67-44cf-88eb-f418d83a68ae
📒 Files selected for processing (29)
backend/app/api/ws/chat_namespace.pybackend/app/services/chat/config/model_compatibility.pybackend/app/services/chat/trigger/lifecycle.pybackend/tests/services/chat/test_model_compatibility.pydocs/en/user-guide/chat/README.mddocs/zh/user-guide/chat/README.mdfrontend/src/__tests__/features/tasks/hooks/useModelSelection.test.tsxfrontend/src/__tests__/utils/modelCompatibility.test.tsfrontend/src/__tests__/utils/modelPreferences.test.tsfrontend/src/features/tasks/hooks/useModelSelection.tsfrontend/src/utils/modelCompatibility.tsfrontend/src/utils/modelPreferences.tsstart.shtests/test_start_sh.shwework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/chat/composer/ModelSelector.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/useWorkbenchModels.tswework/src/features/workbench/workbenchProjectChatHooks.test.tsxwework/src/features/workbench/workbenchReducer.test.tswework/src/features/workbench/workbenchReducer.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 (2)
- wework/src/i18n/locales/en/common.json
- docs/zh/user-guide/chat/README.md
🚧 Files skipped from review as they are similar to previous changes (14)
- backend/app/services/chat/trigger/lifecycle.py
- frontend/src/tests/utils/modelPreferences.test.ts
- frontend/src/tests/features/tasks/hooks/useModelSelection.test.tsx
- docs/en/user-guide/chat/README.md
- wework/src/lib/model-ui.test.ts
- wework/src/features/workbench/workbenchReducer.test.ts
- wework/src/components/layout/MobileWorkbenchLayout.tsx
- frontend/src/features/tasks/hooks/useModelSelection.ts
- frontend/src/utils/modelPreferences.ts
- frontend/src/utils/modelCompatibility.ts
- backend/tests/services/chat/test_model_compatibility.py
- wework/src/features/workbench/WorkbenchProvider.test.tsx
- backend/app/services/chat/config/model_compatibility.py
- backend/app/api/ws/chat_namespace.py
Comment on lines
+85
to
+100
| function getCompatibilityDisabledReason( | ||
| currentModel: UnifiedModel, | ||
| nextModel: UnifiedModel | ||
| ): ModelCompatibilityDisabledReason | null { | ||
| if (isSameModel(currentModel, nextModel)) return null | ||
|
|
||
| const currentFamily = getModelCompatibilityFamily(currentModel) | ||
| if (!currentFamily) return 'missing_current_env_model' | ||
|
|
||
| const nextFamily = getModelCompatibilityFamily(nextModel) | ||
| if (!nextFamily) return 'missing_target_env_model' | ||
|
|
||
| return areModelsProtocolCompatible(currentModel, nextModel) | ||
| ? null | ||
| : 'env_model_mismatch' | ||
| } |
Contributor
There was a problem hiding this comment.
Logic disables all models when current model family is unknown
When getModelCompatibilityFamily(currentModel) returns null (line 91), this function returns 'missing_current_env_model' for every target model (line 92), causing annotateModelsByCompatibility to mark all alternatives as compatibilityDisabled. Line 231 then blocks their selection.
This prevents users on unknown/custom models from switching to any other model—the same issue flagged in the past review comment on model-ui.ts.
To fix this, adjust the logic to allow switches when the current model has no known family, reserving strict validation for cases where both models have known families.
🤖 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 85 - 100,
getCompatibilityDisabledReason currently blocks all alternatives when
getModelCompatibilityFamily(currentModel) is null; change it so unknown families
do not disable switching: keep the isSameModel check, then fetch currentFamily
and nextFamily via getModelCompatibilityFamily, and if either family is null
return null (allow switch). Only when both families are present should you call
areModelsProtocolCompatible and return 'env_model_mismatch' if incompatible;
remove the early returns 'missing_current_env_model' and
'missing_target_env_model' so annotateModelsByCompatibility won't mark all
alternatives compatibilityDisabled.
icycrystal4
force-pushed
the
codex/task-model-switch-compatibility
branch
from
June 2, 2026 09:39
194fc6c to
90ab574
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@start.sh`:
- Around line 1313-1316: The check in check_port() currently returns success if
our_pid appears among port_pids; instead require that every listener PID equals
our_pid before returning reusable: call get_port_listener_pids("$port"),
split/iterate port_pids and verify each entry equals our_pid (or that port_pids
contains only a single PID equal to our_pid) and only then return 0; otherwise
treat the port as in-use and continue error/return non-zero. Ensure you
reference and update the logic around the variables our_pid and port_pids in the
function that performs this verification.
- Around line 1290-1301: is_port_listening currently returns true only when
get_port_listener_pids finds PIDs, which misses listeners when ss/netstat output
lacks "pid=" metadata; update is_port_listening to detect open/listening sockets
directly instead of relying solely on PID extraction: call the same logic used
in get_port_listener_pids (ss -lntp / ss -lntu / netstat fallback) but test for
any matching listening line for the given port (e.g., "LISTEN" or "tcp.*:$port"
matches) and return true if such a socket exists; keep get_port_listener_pids
as-is for PID needs but ensure is_port_listening uses raw socket matching to
avoid false negatives.
In `@wework/src/lib/model-ui.ts`:
- Around line 163-177: getModelCompatibilityFamily currently returns any
non-empty runtime.family, so update it to reject unknown families by checking
against the canonical allowlist used by the backend validator (e.g. import or
create a shared ALLOWED_COMPATIBILITY_FAMILIES or isKnownCompatibilityFamily
helper) and return null for values not in that allowlist; keep
areModelsProtocolCompatible as-is since it relies on
getModelCompatibilityFamily. Update getModelCompatibilityFamily (and tests) to
reference the allowlist helper, and add a unit test that constructs two models
with the same unknown runtime.family and asserts getModelCompatibilityFamily
returns null (and that areModelsProtocolCompatible returns false) to prevent
unknown-but-equal families from being treated as compatible.
🪄 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: 55cb9844-5868-4141-a44a-d4470a5c5683
📒 Files selected for processing (25)
backend/app/api/ws/chat_namespace.pybackend/app/services/chat/config/model_compatibility.pybackend/app/services/chat/trigger/lifecycle.pybackend/app/services/model_aggregation_service.pybackend/tests/services/chat/test_model_compatibility.pybackend/tests/services/test_model_aggregation_service.pydocs/en/user-guide/chat/README.mddocs/zh/user-guide/chat/README.mdstart.shtests/test_start_sh.shwework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/chat/composer/ModelSelector.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/useWorkbenchModels.tswework/src/features/workbench/workbenchProjectChatHooks.test.tsxwework/src/features/workbench/workbenchReducer.test.tswework/src/features/workbench/workbenchReducer.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)
- wework/src/i18n/locales/en/common.json
🚧 Files skipped from review as they are similar to previous changes (18)
- wework/src/components/chat/composer/ComposerToolbar.tsx
- tests/test_start_sh.sh
- wework/src/types/api.ts
- wework/src/components/layout/MobileWorkbenchLayout.tsx
- docs/zh/user-guide/chat/README.md
- backend/app/services/chat/trigger/lifecycle.py
- wework/src/features/workbench/WorkbenchProvider.tsx
- wework/src/components/chat/composer/ModelSelector.tsx
- wework/src/features/workbench/workbenchProjectChatHooks.test.tsx
- wework/src/features/workbench/workbenchReducer.test.ts
- docs/en/user-guide/chat/README.md
- wework/src/features/workbench/WorkbenchProvider.test.tsx
- wework/src/components/chat/ChatInput.test.tsx
- wework/src/features/workbench/workbenchReducer.ts
- backend/app/services/chat/config/model_compatibility.py
- wework/src/features/workbench/useWorkbenchModels.ts
- backend/tests/services/chat/test_model_compatibility.py
- backend/app/api/ws/chat_namespace.py
Comment on lines
+1290
to
+1301
| if [ -z "$pids" ] && command -v ss >/dev/null 2>&1; then | ||
| pids=$(ss -H -lntp "sport = :$port" 2>/dev/null | sed -nE 's/.*pid=([0-9]+).*/\1/p' | sort -u || true) | ||
| fi | ||
|
|
||
| if [ -n "$pids" ]; then | ||
| echo "$pids" | ||
| fi | ||
| } | ||
|
|
||
| is_port_listening() { | ||
| local port=$1 | ||
| [ -n "$(get_port_listener_pids "$port")" ] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ ! -f start.sh ]; then
echo "start.sh not found at repo root"
exit 1
fi
echo "=== Functions & call sites (quick grep) ==="
rg -n "get_port_listener_pids|is_port_listening|check_port\(" start.sh || true
echo
echo "=== Inspect around helper definitions ==="
nl -ba start.sh | sed -n '1240,1345p'
echo
echo "=== Inspect downstream usage regions (shutdown/status/checks) ==="
nl -ba start.sh | sed -n '1540,1735p'
echo
echo "=== Inspect any port check tests/mocks (if present) ==="
rg -n "stub(ed)? lsof|stub(ed)? ss|ss -H -lnt|lsof .*:|SPORT|LISTEN|mock" start.sh || trueRepository: wecode-ai/Wegent
Length of output: 809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect helpers around 1280-1340 ==="
sed -n '1270,1345p' start.sh
echo
echo "=== Inspect check_port around 1300-1335 ==="
sed -n '1290,1335p' start.sh
echo
echo "=== Inspect downstream usage regions 1540-1740 ==="
sed -n '1540,1745p' start.sh
echo
echo "=== Search for lsof/ss usage related to port listening ==="
rg -n "lsof|ss -H -lnt|LISTEN|sport = :|LISTEN|Pi :|LISTEN -t" start.sh || true
echo
echo "=== Search for any port-selection logic calling check_port/get_port_listener_pids ==="
rg -n "check_port|get_port_listener_pids|is_port_listening" start.sh | head -n 200Repository: wecode-ai/Wegent
Length of output: 11843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect port selection logic around check_port call sites ==="
sed -n '1335,1425p' start.sh
echo
echo "=== Inspect next chunk for any additional port selection branches ==="
sed -n '1425,1465p' start.shRepository: wecode-ai/Wegent
Length of output: 4547
Fix is_port_listening to detect sockets directly (not PID-extraction success)
is_port_listening() currently returns true only when get_port_listener_pids() can extract PIDs, but the ss fallback extracts PIDs from ss -lntp output via sed—so listeners can be present while pid= metadata is unavailable, causing “port is free” misclassification. This affects port auto-selection (check_port → find_available_port / resolve_start_service_port) and shutdown/status flows that gate on is_port_listening() (graceful wait, final port verification, show_status).
Suggested fix
get_port_listener_pids() {
local port=$1
local pids=""
@@
if [ -n "$pids" ]; then
echo "$pids"
fi
}
is_port_listening() {
local port=$1
- [ -n "$(get_port_listener_pids "$port")" ]
+ if command -v lsof >/dev/null 2>&1; then
+ lsof -Pi :"$port" -sTCP:LISTEN -t >/dev/null 2>&1
+ return $?
+ fi
+
+ if command -v ss >/dev/null 2>&1; then
+ ss -H -lnt "sport = :$port" 2>/dev/null | grep -q .
+ return $?
+ fi
+
+ return 1
}📝 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
| if [ -z "$pids" ] && command -v ss >/dev/null 2>&1; then | |
| pids=$(ss -H -lntp "sport = :$port" 2>/dev/null | sed -nE 's/.*pid=([0-9]+).*/\1/p' | sort -u || true) | |
| fi | |
| if [ -n "$pids" ]; then | |
| echo "$pids" | |
| fi | |
| } | |
| is_port_listening() { | |
| local port=$1 | |
| [ -n "$(get_port_listener_pids "$port")" ] | |
| if [ -z "$pids" ] && command -v ss >/dev/null 2>&1; then | |
| pids=$(ss -H -lntp "sport = :$port" 2>/dev/null | sed -nE 's/.*pid=([0-9]+).*/\1/p' | sort -u || true) | |
| fi | |
| if [ -n "$pids" ]; then | |
| echo "$pids" | |
| fi | |
| } | |
| is_port_listening() { | |
| local port=$1 | |
| if command -v lsof >/dev/null 2>&1; then | |
| lsof -Pi :"$port" -sTCP:LISTEN -t >/dev/null 2>&1 | |
| return $? | |
| fi | |
| if command -v ss >/dev/null 2>&1; then | |
| ss -H -lnt "sport = :$port" 2>/dev/null | grep -q . | |
| return $? | |
| fi | |
| return 1 | |
| } |
🤖 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 `@start.sh` around lines 1290 - 1301, is_port_listening currently returns true
only when get_port_listener_pids finds PIDs, which misses listeners when
ss/netstat output lacks "pid=" metadata; update is_port_listening to detect
open/listening sockets directly instead of relying solely on PID extraction:
call the same logic used in get_port_listener_pids (ss -lntp / ss -lntu /
netstat fallback) but test for any matching listening line for the given port
(e.g., "LISTEN" or "tcp.*:$port" matches) and return true if such a socket
exists; keep get_port_listener_pids as-is for PID needs but ensure
is_port_listening uses raw socket matching to avoid false negatives.
Comment on lines
+163
to
+177
| export function getModelCompatibilityFamily( | ||
| model?: ModelCompatibilitySource | null, | ||
| ): ModelCompatibilityFamily | null { | ||
| if (!model) return null | ||
| return normalizeCompatibilitySignal(model.runtime?.family) || null | ||
| } | ||
|
|
||
| export function areModelsProtocolCompatible( | ||
| currentModel?: ModelCompatibilitySource | null, | ||
| nextModel?: ModelCompatibilitySource | null, | ||
| ): boolean { | ||
| const currentFamily = getModelCompatibilityFamily(currentModel) | ||
| const nextFamily = getModelCompatibilityFamily(nextModel) | ||
| if (!currentFamily || !nextFamily) return false | ||
| return currentFamily === nextFamily |
Contributor
There was a problem hiding this comment.
Reject unknown runtime families here, not just missing ones.
getModelCompatibilityFamily() currently treats any non-empty runtime.family as valid, so two models with the same unrecognized value will pass areModelsProtocolCompatible(). That breaks the PR contract for existing-task switches, which is to reject missing or unknown runtime groups, and useWorkbenchModels.ts will surface that as a selectable target because only null/mismatch is disabled downstream.
Please gate this helper against the same allowlist the backend validator uses, and add a unit test for two identical unknown families to keep the frontend behavior aligned.
🤖 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/lib/model-ui.ts` around lines 163 - 177,
getModelCompatibilityFamily currently returns any non-empty runtime.family, so
update it to reject unknown families by checking against the canonical allowlist
used by the backend validator (e.g. import or create a shared
ALLOWED_COMPATIBILITY_FAMILIES or isKnownCompatibilityFamily helper) and return
null for values not in that allowlist; keep areModelsProtocolCompatible as-is
since it relies on getModelCompatibilityFamily. Update
getModelCompatibilityFamily (and tests) to reference the allowlist helper, and
add a unit test that constructs two models with the same unknown runtime.family
and asserts getModelCompatibilityFamily returns null (and that
areModelsProtocolCompatible returns false) to prevent unknown-but-equal families
from being treated as compatible.
icycrystal4
force-pushed
the
codex/task-model-switch-compatibility
branch
from
June 2, 2026 09:54
90ab574 to
55b5a61
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/api/ws/test_chat_namespace_retry.py (1)
47-64: ⚡ Quick winAssert the retry compatibility guard is actually exercised.
These tests now patch
ensure_task_model_override_compatible, but they never verify thaton_chat_retry()called it. If that validation is accidentally removed or bypassed, both tests still pass and this PR’s new retry guard loses coverage.♻️ Minimal test hardening
with ( patch("app.api.ws.chat_namespace.SessionLocal", return_value=db), patch( "app.api.ws.chat_namespace.can_access_task", AsyncMock(return_value=True) ), @@ patch("app.api.ws.chat_namespace.extract_display_prompt", return_value="Hello"), patch("app.api.ws.chat_namespace.get_device_id", return_value=None), - patch("app.api.ws.chat_namespace.ensure_task_model_override_compatible"), + patch( + "app.api.ws.chat_namespace.ensure_task_model_override_compatible" + ) as mock_ensure_compatible, patch( "app.services.chat.trigger.trigger_ai_response_unified", AsyncMock() ) as mock_trigger, patch("sqlalchemy.orm.attributes.flag_modified") as mock_flag_modified, ): @@ assert result == {"success": True} + mock_ensure_compatible.assert_called_once()Also applies to: 118-135
🤖 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 `@backend/tests/api/ws/test_chat_namespace_retry.py` around lines 47 - 64, The tests patch ensure_task_model_override_compatible but never assert it was called; update the tests in test_chat_namespace_retry.py to assert that ensure_task_model_override_compatible was invoked when on_chat_retry() runs (e.g., assert mock_ensure_task_model_override_compatible.called or called_with expected args). Locate the patch for "app.api.ws.chat_namespace.ensure_task_model_override_compatible" in the context managers and add a corresponding assertion after calling on_chat_retry()/the websocket handler to ensure the retry guard is exercised; do this for both occurrences (lines ~47-64 and ~118-135).
🤖 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 `@backend/app/services/model_aggregation_service.py`:
- Around line 90-92: The runtime property is incorrectly deriving runtime.family
from provider; instead thread and use a dedicated runtime_family field on
UnifiedModel (originating from spec.modelConfig.env.model) and return that here;
update the UnifiedModel data structure to carry runtime_family, ensure it's
populated when constructing models, and serialize runtime.family from
UnifiedModel.runtime_family in the runtime() property rather than using
self.provider (also apply the same change at the other occurrence around line
114).
---
Nitpick comments:
In `@backend/tests/api/ws/test_chat_namespace_retry.py`:
- Around line 47-64: The tests patch ensure_task_model_override_compatible but
never assert it was called; update the tests in test_chat_namespace_retry.py to
assert that ensure_task_model_override_compatible was invoked when
on_chat_retry() runs (e.g., assert
mock_ensure_task_model_override_compatible.called or called_with expected args).
Locate the patch for
"app.api.ws.chat_namespace.ensure_task_model_override_compatible" in the context
managers and add a corresponding assertion after calling on_chat_retry()/the
websocket handler to ensure the retry guard is exercised; do this for both
occurrences (lines ~47-64 and ~118-135).
🪄 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: d516d5b8-1327-49a6-8147-2a184168cf65
📒 Files selected for processing (26)
backend/app/api/ws/chat_namespace.pybackend/app/services/chat/config/model_compatibility.pybackend/app/services/chat/trigger/lifecycle.pybackend/app/services/model_aggregation_service.pybackend/tests/api/ws/test_chat_namespace_retry.pybackend/tests/services/chat/test_model_compatibility.pybackend/tests/services/test_model_aggregation_service.pydocs/en/user-guide/chat/README.mddocs/zh/user-guide/chat/README.mdstart.shtests/test_start_sh.shwework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/composer/ComposerToolbar.tsxwework/src/components/chat/composer/ModelSelector.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/useWorkbenchModels.tswework/src/features/workbench/workbenchProjectChatHooks.test.tsxwework/src/features/workbench/workbenchReducer.test.tswework/src/features/workbench/workbenchReducer.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 (4)
- wework/src/features/workbench/workbenchReducer.test.ts
- wework/src/i18n/locales/en/common.json
- docs/zh/user-guide/chat/README.md
- wework/src/features/workbench/WorkbenchProvider.test.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/test_start_sh.sh
- backend/tests/services/test_model_aggregation_service.py
- wework/src/types/api.ts
- wework/src/components/layout/MobileWorkbenchLayout.tsx
- wework/src/lib/model-ui.ts
- wework/src/features/workbench/useWorkbenchModels.ts
- wework/src/features/workbench/workbenchReducer.ts
- wework/src/components/chat/ChatInput.test.tsx
- wework/src/features/workbench/WorkbenchProvider.tsx
- wework/src/features/workbench/workbenchProjectChatHooks.test.tsx
- wework/src/components/chat/composer/ModelSelector.tsx
- backend/app/api/ws/chat_namespace.py
- backend/app/services/chat/trigger/lifecycle.py
- wework/src/i18n/locales/zh-CN/common.json
- wework/src/lib/model-ui.test.ts
- backend/tests/services/chat/test_model_compatibility.py
- docs/en/user-guide/chat/README.md
- backend/app/services/chat/config/model_compatibility.py
icycrystal4
changed the title
icycrystal4
force-pushed
the
codex/task-model-switch-compatibility
branch
7 times, most recently
from
June 3, 2026 04:14
cbc46ad to
bbe3a53
Compare
icycrystal4
force-pushed
the
codex/task-model-switch-compatibility
branch
from
June 3, 2026 04:22
bbe3a53 to
34c1369
Compare
qdaxb
approved these changes
Jun 3, 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
Tests