feat: 看板mcp修复bug - #2291
Conversation
📝 WalkthroughWalkthroughMCP project routing now uses project metadata and requested project IDs to select local or backend handling. Space listing merges local and cloud projects when available, cloud project creation and updates call backend APIs, and tests cover the revised routing and listing behavior. ChangesMCP routing and backend integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant call_tool
participant ProjectMetadata
participant LocalProvider
participant call_backend_tool
MCPClient->>call_tool: Call MCP tool with project id
call_tool->>ProjectMetadata: Check project_store and task_provider
ProjectMetadata-->>call_tool: Return routing decision
alt Local routing
call_tool->>LocalProvider: Execute board or table operation
else Backend routing
call_tool->>call_backend_tool: Execute cloud operation
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
executor/src/task_runtime/mcp.rs (1)
225-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate project-id fallback logic.
requested_space_id.or_else(|| default_project_id.clone())recomputes exactly whatrequested_project_idalready holds (computed at the top ofcall_toolfrom the samearguments.get("space_id")/default_project_idfallback). Reuse the existing variable instead of re-deriving it.As per coding guidelines, "Before adding code, search for and reuse existing components, services, utilities, and patterns; extract shared logic instead of duplicating it."♻️ Proposed fix
- "search_board_items" => { - let requested_space_id = arguments - .get("space_id") - .and_then(Value::as_str) - .map(ToOwned::to_owned); - match parse::<TaskSearch>(arguments) { - Ok(mut input) => { - input.project_id = requested_space_id.or_else(|| default_project_id.clone()); + "search_board_items" => { + let project_id = requested_project_id.clone(); + match parse::<TaskSearch>(arguments) { + Ok(mut input) => { + input.project_id = project_id;🤖 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/task_runtime/mcp.rs` around lines 225 - 232, In the search_board_items branch of call_tool, assign input.project_id from the existing requested_project_id variable instead of recomputing the space_id/default_project_id fallback locally. Remove the duplicate requested_space_id extraction while preserving the current TaskSearch parsing and project-id behavior.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/task_runtime/mcp.rs`:
- Around line 159-170: Update the Err branch in the list-spaces backend call
within the surrounding task runtime function so backend failures preserve and
return the already-fetched local_projects alongside an error indication. Keep
the successful backend merge and no-credentials behavior unchanged, and ensure
the response still uses the existing text_result format.
- Around line 191-197: Update the backend-required guard in the task runtime to
exempt the create_space operation, matching the existing special case in
should_use_backend. Ensure create_space proceeds locally regardless of
requested_project_id or is_locally_routed, while preserving the guard for other
operations.
---
Nitpick comments:
In `@executor/src/task_runtime/mcp.rs`:
- Around line 225-232: In the search_board_items branch of call_tool, assign
input.project_id from the existing requested_project_id variable instead of
recomputing the space_id/default_project_id fallback locally. Remove the
duplicate requested_space_id extraction while preserving the current TaskSearch
parsing and project-id behavior.
🪄 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: 0ea5eef6-40f5-48f4-80e3-c25c405565e5
📒 Files selected for processing (2)
executor/src/task_runtime/mcp.rsexecutor/tests/local_task_mcp_contract.rs
| let Some((backend_url, auth_token)) = backend_url.as_deref().zip(auth_token.as_deref()) | ||
| else { | ||
| return text_result(Value::Array(local_projects).to_string(), false); | ||
| }; | ||
| return match call_backend_tool(backend_url, auth_token, "", name, &arguments).await { | ||
| Ok(Value::Array(mut cloud_projects)) => { | ||
| cloud_projects.extend(local_projects); | ||
| text_result(Value::Array(cloud_projects).to_string(), false) | ||
| } | ||
| Ok(value) => text_result(value.to_string(), false), | ||
| Err(error) => text_result(error, true), | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Backend failure silently discards already-fetched local spaces.
When backend credentials are present but the backend list_spaces call fails, the function returns only the backend error and discards local_projects that were already successfully retrieved (line 169: Err(error) => text_result(error, true)). This defeats the goal of this change — keeping spaces available locally when the backend connection is unreliable — since a transient backend outage now hides the user's local projects entirely.
🛡️ Proposed fix
Ok(value) => text_result(value.to_string(), false),
- Err(error) => text_result(error, true),
+ Err(_error) => text_result(Value::Array(local_projects).to_string(), 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.
| let Some((backend_url, auth_token)) = backend_url.as_deref().zip(auth_token.as_deref()) | |
| else { | |
| return text_result(Value::Array(local_projects).to_string(), false); | |
| }; | |
| return match call_backend_tool(backend_url, auth_token, "", name, &arguments).await { | |
| Ok(Value::Array(mut cloud_projects)) => { | |
| cloud_projects.extend(local_projects); | |
| text_result(Value::Array(cloud_projects).to_string(), false) | |
| } | |
| Ok(value) => text_result(value.to_string(), false), | |
| Err(error) => text_result(error, true), | |
| }; | |
| let Some((backend_url, auth_token)) = backend_url.as_deref().zip(auth_token.as_deref()) | |
| else { | |
| return text_result(Value::Array(local_projects).to_string(), false); | |
| }; | |
| return match call_backend_tool(backend_url, auth_token, "", name, &arguments).await { | |
| Ok(Value::Array(mut cloud_projects)) => { | |
| cloud_projects.extend(local_projects); | |
| text_result(Value::Array(cloud_projects).to_string(), false) | |
| } | |
| Ok(value) => text_result(value.to_string(), false), | |
| Err(_error) => text_result(Value::Array(local_projects).to_string(), 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/task_runtime/mcp.rs` around lines 159 - 170, Update the Err
branch in the list-spaces backend call within the surrounding task runtime
function so backend failures preserve and return the already-fetched
local_projects alongside an error indication. Keep the successful backend merge
and no-credentials behavior unchanged, and ensure the response still uses the
existing text_result format.
| if requested_project_id.is_some() && !is_locally_routed { | ||
| return text_result( | ||
| "The current project space requires the WeWork Backend connection. Retry through wework_space after the connection is restored; do not use git or provider APIs." | ||
| .to_owned(), | ||
| true, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
create_space can be incorrectly blocked by the backend-required guard.
should_use_backend (line 175) special-cases name == "create_space" so creation is attempted unconditionally when backend creds exist. But this guard doesn't mirror that special case: if backend creds are missing and the bound/default project (requested_project_id, which for create_space falls back to default_project_id/WEWORK_SPACE_ID) happens to resolve to a non-locally-routed (backend) project, create_space is rejected with "requires the WeWork Backend connection" — even though creating a brand-new space has nothing to do with the currently bound project and should proceed locally.
🐛 Proposed fix
- if requested_project_id.is_some() && !is_locally_routed {
+ if name != "create_space" && requested_project_id.is_some() && !is_locally_routed {📝 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.
| if requested_project_id.is_some() && !is_locally_routed { | |
| return text_result( | |
| "The current project space requires the WeWork Backend connection. Retry through wework_space after the connection is restored; do not use git or provider APIs." | |
| .to_owned(), | |
| true, | |
| ); | |
| } | |
| if name != "create_space" && requested_project_id.is_some() && !is_locally_routed { | |
| return text_result( | |
| "The current project space requires the WeWork Backend connection. Retry through wework_space after the connection is restored; do not use git or provider APIs." | |
| .to_owned(), | |
| true, | |
| ); | |
| } |
🤖 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/task_runtime/mcp.rs` around lines 191 - 197, Update the
backend-required guard in the task runtime to exempt the create_space operation,
matching the existing special case in should_use_backend. Ensure create_space
proceeds locally regardless of requested_project_id or is_locally_routed, while
preserving the guard for other operations.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes