Repository navigation
fix(wework): preserve protocol tool lifecycle state - #2213
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe local model proxy normalizes tool-call identifiers across request and streaming protocol conversions. Runtime correlation uses call IDs, while Wework adds per-file streaming statistics, duration selection, animations, stable merged IDs, and expanded protocol-switch and UI test coverage. ChangesLocal model protocol correlation
File-change rendering
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant LocalModelClient
participant LocalModelProxy
participant ProtocolConverter
participant RuntimeTimeline
participant ToolBlocksDisplay
LocalModelClient->>LocalModelProxy: send tool history or provider SSE
LocalModelProxy->>LocalModelProxy: normalize request and stream IDs
LocalModelProxy->>ProtocolConverter: convert canonical history and events
ProtocolConverter-->>RuntimeTimeline: emit correlated tool blocks
RuntimeTimeline->>ToolBlocksDisplay: provide processing blocks
ToolBlocksDisplay-->>RuntimeTimeline: render selected duration and streaming statistics
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/e2e/desktop/task-flow.e2e.mjs (1)
3927-3940: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle prewarm requests in model-switch states.
handleModelProtocolMatrixResponseaccepts prompt-lessprewarmrequests, butmodel_switch_sourceandmodel_switch_targetrequire the initial prompt immediately. HandlecodexRequestKind(body) === 'prewarm'in these states by returning an empty successful assistant response without advancing the stage; otherwise tool-less model selection can fail during prewarm.Also applies to:
wework/e2e/desktop/task-flow.e2e.mjs:3962-3988🤖 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/e2e/desktop/task-flow.e2e.mjs` around lines 3927 - 3940, Update handleModelProtocolMatrixResponse in the model_switch_source and model_switch_target state branches to detect codexRequestKind(body) === 'prewarm', return an empty successful assistant response, and leave the current stage unchanged; preserve the existing initial-prompt and tool-call behavior for non-prewarm requests.
🧹 Nitpick comments (4)
wework/src/styles/globals.css (1)
557-584: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHard-coded colors bypass the neutral-first token system and read incorrectly in dark mode.
The stat block mixes tokens (
--color-border,--color-bg-surface) with a fixed white inner highlight (rgb(255 255 255 / 0.85)) and literal pastel gradients, so the chip will look inverted on dark surfaces. Also, the deletion bar is orange while the deletion count next to it istext-red-500— same semantic state rendered in two different hues. Consider driving all four values from theme tokens.As per coding guidelines: "Follow the neutral-first visual system ... Restrict green and teal to semantic success/addition states."
🤖 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/styles/globals.css` around lines 557 - 584, The file-change stat styles use hard-coded highlight and addition/deletion colors that bypass theme tokens and render inconsistently in dark mode. Update the file-change stat container and .file-change-stat-segment.is-addition/.is-deletion rules to use the existing neutral-first theme tokens for the border, highlight, and backgrounds, while keeping addition green and matching deletion to the red semantic color used by the deletion count.Source: Coding guidelines
wework/src/components/chat/blocks/ToolBlocksDisplay.test.tsx (1)
677-684: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
requestAnimationFramespy inafterEachso a failing assertion cannot leak it.
animationFrame.mockRestore()only runs when every precedingexpectpasses; otherwisewindow.requestAnimationFramestays stubbed on a fake-timer implementation for the remaining tests in this file.♻️ Proposed cleanup
afterEach(() => { vi.useRealTimers() + vi.restoreAllMocks() })Also applies to: 727-728
🤖 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/blocks/ToolBlocksDisplay.test.tsx` around lines 677 - 684, Restore the requestAnimationFrame spy in the test cleanup path, such as the file’s afterEach hook, so it runs even when assertions fail. Use the existing animationFrame spy from the streaming duration tests and ensure fake timers and system time are also cleaned up consistently without relying on assertions reaching the end of the test.executor/src/server/local_model_proxy/chat.rs (1)
932-972: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared "normalize-id-field-if-changed" helper. Both sites reuse
normalized_responses_api_idcorrectly but each re-implements the same small "get str field → normalize → replace if changed" wrapper independently, risking future drift between the two copies.
executor/src/server/local_model_proxy/chat.rs#L932-L972: keepnormalize_id_field's signature but move it (or an equivalent generic helper) intomod.rs(or a shared submodule) so both files call one implementation.executor/src/server/local_model_proxy/mod.rs#L616-L679: replace the inlinefor field in ["id", "call_id"] { ... }body innormalize_responses_request_idswith a call to the shared helper (adjusting to track the changed-count via the helper's boolean return).♻️ Suggested shared helper
// in mod.rs, usable from both mod.rs and chat.rs via `super::normalize_id_field` pub(super) fn normalize_id_field(object: &mut serde_json::Map<String, Value>, field: &str) -> bool { let Some(raw) = object.get(field).and_then(Value::as_str) else { return false; }; let normalized = normalized_responses_api_id(raw); if normalized != raw { object.insert(field.to_owned(), Value::String(normalized)); return true; } 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 `@executor/src/server/local_model_proxy/chat.rs` around lines 932 - 972, Extract the shared normalize-id-field-if-changed logic into a reusable helper in executor/src/server/local_model_proxy/mod.rs, preserving the normalize_id_field behavior and boolean return. Update executor/src/server/local_model_proxy/chat.rs lines 932-972 to call the shared helper, and replace the inline id/call_id normalization loop in normalize_responses_request_ids at executor/src/server/local_model_proxy/mod.rs lines 616-679 with that helper while preserving changed-count tracking.executor/src/server/local_model_proxy/mod.rs (1)
1010-1090: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for
convert_custom_tools=truecombined with invalid cross-protocol ids.The new tests exercise
prepare_request("openai-responses"/"openai-chat-completions"/"anthropic-messages", false, ...), but none exercise theconvert_custom_tools=truebranch (which routes throughchat::responses_to_responsesbefore serialization) together with invalid ids. Since that conversion runs on the already-normalizedresponses_body, it should carry the normalized ids through unchanged, but this path is currently untested — worth a follow-up test to lock in the guarantee this PR is establishing.🤖 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/server/local_model_proxy/mod.rs` around lines 1010 - 1090, Extend the existing cross-protocol ID normalization tests to cover prepare_request for openai-responses, openai-chat-completions, and anthropic-messages with convert_custom_tools=true. Reuse cross_protocol_history_with_invalid_ids, verify the converted requests preserve valid normalized call and output IDs, and assert the expected conversion result for each protocol.
🤖 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/e2e/desktop/task-flow.e2e.mjs`:
- Around line 6176-6189: Before the model-switch evidence is written in the
MODEL_SWITCH_ONLY branch, sanitize control.modelRequests by removing or
redacting each request’s authorization header while preserving all non-sensitive
request fields; then serialize the sanitized data to model-requests.json and
leave the verification flow unchanged.
In `@wework/src/styles/globals.css`:
- Line 497: Update the affected keyframe selectors in the relevant CSS animation
block to use percentage notation as required by keyframe-selector-notation, and
change the color value from currentColor to lowercase currentcolor to satisfy
value-keyword-case. Apply these corrections to the referenced declarations and
all additionally affected selectors.
---
Outside diff comments:
In `@wework/e2e/desktop/task-flow.e2e.mjs`:
- Around line 3927-3940: Update handleModelProtocolMatrixResponse in the
model_switch_source and model_switch_target state branches to detect
codexRequestKind(body) === 'prewarm', return an empty successful assistant
response, and leave the current stage unchanged; preserve the existing
initial-prompt and tool-call behavior for non-prewarm requests.
---
Nitpick comments:
In `@executor/src/server/local_model_proxy/chat.rs`:
- Around line 932-972: Extract the shared normalize-id-field-if-changed logic
into a reusable helper in executor/src/server/local_model_proxy/mod.rs,
preserving the normalize_id_field behavior and boolean return. Update
executor/src/server/local_model_proxy/chat.rs lines 932-972 to call the shared
helper, and replace the inline id/call_id normalization loop in
normalize_responses_request_ids at executor/src/server/local_model_proxy/mod.rs
lines 616-679 with that helper while preserving changed-count tracking.
In `@executor/src/server/local_model_proxy/mod.rs`:
- Around line 1010-1090: Extend the existing cross-protocol ID normalization
tests to cover prepare_request for openai-responses, openai-chat-completions,
and anthropic-messages with convert_custom_tools=true. Reuse
cross_protocol_history_with_invalid_ids, verify the converted requests preserve
valid normalized call and output IDs, and assert the expected conversion result
for each protocol.
In `@wework/src/components/chat/blocks/ToolBlocksDisplay.test.tsx`:
- Around line 677-684: Restore the requestAnimationFrame spy in the test cleanup
path, such as the file’s afterEach hook, so it runs even when assertions fail.
Use the existing animationFrame spy from the streaming duration tests and ensure
fake timers and system time are also cleaned up consistently without relying on
assertions reaching the end of the test.
In `@wework/src/styles/globals.css`:
- Around line 557-584: The file-change stat styles use hard-coded highlight and
addition/deletion colors that bypass theme tokens and render inconsistently in
dark mode. Update the file-change stat container and
.file-change-stat-segment.is-addition/.is-deletion rules to use the existing
neutral-first theme tokens for the border, highlight, and backgrounds, while
keeping addition green and matching deletion to the red semantic color used by
the deletion count.
🪄 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 Plus
Run ID: 9816d5dc-c8dc-4316-877b-0715a141f076
📒 Files selected for processing (14)
docs/en/wegent/developer-guide/local-device-architecture.mddocs/zh/wegent/developer-guide/local-device-architecture.mdexecutor/src/runtime_work/events.rsexecutor/src/runtime_work/transcript.rsexecutor/src/server/local_model_proxy/chat.rsexecutor/src/server/local_model_proxy/mod.rswework/e2e/desktop/task-flow.e2e.mjswework/src/components/chat/MessageList.tsxwework/src/components/chat/blocks/ToolBlockItem.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.test.tsxwework/src/components/chat/blocks/ToolBlocksDisplay.tsxwework/src/components/chat/blocks/toolBlockActivity.test.tswework/src/components/chat/blocks/toolBlockActivity.tswework/src/styles/globals.css
| inset-inline-start: 100%; | ||
| inset-block-start: -0.8em; | ||
| margin-inline-start: 2px; | ||
| color: currentColor; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Stylelint violations so the lint gate passes.
keyframe-selector-notation requires percentage selectors, and value-keyword-case requires lowercase currentcolor.
🔧 Proposed fix
- color: currentColor;
+ color: currentcolor; `@keyframes` file-change-number-roll {
- from {
+ 0% {
opacity: 0.2;
transform: translateY(-0.9em);
}
58% {
opacity: 1;
transform: translateY(0.08em);
}
- to {
+ 100% {
transform: translateY(0);
}
}Also applies to: 506-520
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 497-497: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 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/styles/globals.css` at line 497, Update the affected keyframe
selectors in the relevant CSS animation block to use percentage notation as
required by keyframe-selector-notation, and change the color value from
currentColor to lowercase currentcolor to satisfy value-keyword-case. Apply
these corrections to the referenced declarations and all additionally affected
selectors.
Source: Linters/SAST tools
29def0b to
690c9b6
Compare
Summary
Verification
pnpm --filter wework test(2119 passed)pnpm --filter wework typecheckpnpm --filter wework lintcargo test --manifest-path executor/Cargo.toml --lib(388 passed, 1 ignored)node wework/e2e/desktop/task-flow.e2e.mjs --model-switch-onlyai:verifyedit flow with streaming and completed screenshotsSummary by CodeRabbit