feat：支持上传反馈、支持钉钉多维表格 - #2283
Conversation
…nto human/gecko-20260728-064933 # Conflicts: # executor/src/task_runtime/store.rs # wework/src/api/deliveries.ts # wework/src/features/workbench/workbenchServices.ts
|
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 (10)
📝 WalkthroughWalkthroughThe PR adds DingTalk AI Table support across backend, executor, desktop APIs, and workbench UI; introduces authenticated feedback submission with bundle handling; migrates task MCP interactions to scoped WeWork space operations; and updates attachment routing, runtime project context, and desktop sidecar packaging. ChangesPlatform services and feedback
DingTalk AITable
Space MCP migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/src/features/todo/TodoEditor.tsx (1)
568-579: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDeleted attachments can reappear via the markdown fallback in
visibleAttachments.
removeAttachmentdeletes the attachment via the API and removes it fromattachmentsstate, but never strips its reference fromdescription. SinceuploadAttachments/appendAttachmentMarkdownembed the same attachment id intodescription's markdown (via thewegent-attachment:marker orwegent://attachments/link),visibleAttachments'smarkdownAttachmentRows(description)fallback (Lines 309-314) will keep resurfacing the deleted attachment as a phantom row once it's no longer present inattachmentsto override it. Opening that phantom row callsdownloadLoopItemAttachmentfor an id that no longer exists server-side.🐛 Proposed fix
async function removeAttachment(attachment: AttachmentRow) { setAttachmentBusy(true) setAttachmentError(null) try { await api.deleteLoopItemAttachment(attachment.id) setAttachments(current => current.filter(entry => entry.id !== attachment.id)) + setDescription(current => stripAttachmentMarkdown(current, attachment.id)) } catch (cause) { setAttachmentError(cause instanceof Error ? cause.message : '附件删除失败') } finally { setAttachmentBusy(false) } }(
stripAttachmentMarkdownwould remove the matching[...](...)+ marker/wegent://reference for the given id fromdescription, mirroringattachmentMarkdown.ts's match patterns.)Also applies to: 309-314
🤖 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/todo/TodoEditor.tsx` around lines 568 - 579, Update removeAttachment to remove the deleted attachment’s markdown references from description using the existing stripAttachmentMarkdown helper or equivalent attachmentMarkdown patterns before updating state. Ensure both wegent-attachment markers and wegent://attachments links for attachment.id are stripped so visibleAttachments cannot recreate the deleted row from markdownAttachmentRows(description).
🟡 Minor comments (4)
executor/src/task_runtime/mcp.rs-590-592 (1)
590-592: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTask-era naming left behind by the space MCP rename. The
task-mcp-server→space-mcp-servermigration renamed the symbols but not the surrounding strings and file name, so agent-facing errors and operator logs still say "task".
executor/src/task_runtime/mcp.rs#L590-L592: change thecreate_space/update_spacerejection text from "not available through task MCP" to referencewework_space.executor/src/task_runtime/mcp.rs#L507-L512: the closure readsitem_idbut errors with"task_id is required"; change the message toitem_id.executor/src/bin/wegent-executor.rs#L25-L29: the gate is nowis_space_mcp_command(), so change theeprintln!to "space MCP server failed".executor/tests/local_task_mcp_contract.rs#L18-L22: the file now covers only the space MCP contract; rename it tolocal_space_mcp_contract.rs.🤖 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 590 - 592, Update the space MCP rename leftovers: in executor/src/task_runtime/mcp.rs lines 590-592, change the create_space/update_space rejection text to reference wework_space; in executor/src/task_runtime/mcp.rs lines 507-512, change the item_id validation message from task_id to item_id; in executor/src/bin/wegent-executor.rs lines 25-29, change the is_space_mcp_command() failure output to “space MCP server failed”; rename executor/tests/local_task_mcp_contract.rs to executor/tests/local_space_mcp_contract.rs.executor/src/runtime_work/handler/tasks.rs-214-216 (1)
214-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
cloudProjectIdis not restored from a recovered link.
send_messagefalls back torecovered_linkfor session id (Line 242) andephemeral(Line 283), but the cloud project id is only restored whenexisting_linkis present. A follow-up turn on a link that had to be recovered will lose the id, and the MCP/space tooling in the spawned turn won't be scoped to the cloud project.🛠️ Restore after the link recovery step
- if let Some(link) = existing_link.as_ref() { - restore_cloud_project_id(&mut request, &link.runtime_handle); - } request.new_session = false;Then after
recovered_linkis computed (around Line 235):if let Some(link) = existing_link.as_ref().or(recovered_link.as_ref()) { restore_cloud_project_id(&mut request, &link.runtime_handle); }🤖 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/tasks.rs` around lines 214 - 216, Update the link restoration logic in send_message to run after recovered_link is computed, and select existing_link or recovered_link via the existing-link fallback. Pass the selected link’s runtime_handle to restore_cloud_project_id so recovered-link follow-up turns retain cloudProjectId.wework/src/api/local/localDelivery.ts-454-459 (1)
454-459: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThread
filenamethroughdownloadLoopItemAttachmentin the local implementation.
TodoEditorpassesattachment.display_name, and the cloud API uses that value for the downloaded/saved filename.openLocalFile()only receivesaccess.path, so it opens whateverattachments.accessreturns and ignores the requested name. If the local path is not already the original filename, add afilenameparameter and apply it consistently with the cloud implementation.🤖 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/localDelivery.ts` around lines 454 - 459, The local download flow should preserve the requested attachment filename. Update downloadLoopItemAttachment to accept the filename passed by TodoEditor, then use it when opening or saving the file through the local implementation so the behavior matches the cloud API rather than relying only on access.path; update any callers and related signatures consistently.wework/src/features/todo/attachmentMarkdown.ts-7-20 (1)
7-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
markdownAttachmentRowsreturns duplicate rows when both patterns match the same attachment.The two patterns aren't mutually exclusive: the markdown actually generated by the upload flow (
[name](wegent://attachments/id)\n<!-- wegent-attachment:id -->, per the mock inCloudTodoWorkspace.test.tsxLines 76-79) matches both patterns, so this function returns two duplicate rows for the same id. Currently the one consumer (TodoEditor.tsx'svisibleAttachments) happens to dedupe by id via aMap, masking the issue, but the function's own contract should guarantee unique rows.♻️ Proposed fix: dedupe within the function
export function markdownAttachmentRows(markdown: string): MarkdownAttachmentRow[] { - return attachmentMarkdownPatterns.flatMap(pattern => - Array.from(markdown.matchAll(pattern), match => ({ - id: match[2], - display_name: match[1], - size_bytes: 0, - })) - ) + const rows = new Map<string, MarkdownAttachmentRow>() + for (const pattern of attachmentMarkdownPatterns) { + for (const match of markdown.matchAll(pattern)) { + if (!rows.has(match[2])) { + rows.set(match[2], { id: match[2], display_name: match[1], size_bytes: 0 }) + } + } + } + return Array.from(rows.values()) }Consider also adding a test in
TodoEditor.test.tsfor the combined marker+link case to lock in the fix.🤖 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/todo/attachmentMarkdown.ts` around lines 7 - 20, Update markdownAttachmentRows to deduplicate attachment rows by their id after collecting matches from both attachmentMarkdownPatterns, while preserving the existing row fields and match behavior. Ensure combined marker-and-link markdown produces one row, and add coverage in TodoEditor.test.ts for that case if the existing test structure supports it.
🧹 Nitpick comments (16)
backend/app/services/runtime_work_service.py (1)
3516-3522: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression assertions for the new space-MCP guidance.
At Lines 3907-3926, assert that
wework_space,list_spaces,get_board_item, andread_item_attachmentare included, and stalewegent_delivery/wegent_tasksguidance is absent.🤖 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/runtime_work_service.py` around lines 3516 - 3522, Add regression assertions in the relevant test block around the runtime work guidance to verify the prompt includes wework_space, list_spaces, get_board_item, and read_item_attachment, while confirming stale wegent_delivery and wegent_tasks guidance is absent. Use the existing assertion style and target the generated guidance text.executor/tests/local_task_mcp_contract.rs (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming the file alongside the tests.
local_task_mcp_contract.rsnow exclusively covers the space MCP contract. Renaming tolocal_space_mcp_contract.rskeeps the migration complete.🤖 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/tests/local_task_mcp_contract.rs` around lines 18 - 22, Rename the test file local_task_mcp_contract.rs to local_space_mcp_contract.rs so its filename matches the space MCP tests it exclusively contains. Update any references to the old filename if present, without changing the test behavior.executor/src/task_runtime/store.rs (1)
986-990: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
write_executor_error_linefor this cached-project warning.
executor::logging::write_executor_error_line()still writes to stderr, but also writes the line to the executor rolling log; this diagnostic should go through the same logging path as other executor error messages.🤖 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/store.rs` around lines 986 - 990, Replace the eprintln! call in the incompatible cached external project handling with executor::logging::write_executor_error_line, preserving the existing project ID and error details in the diagnostic message and the subsequent return None behavior.executor/src/task_runtime/aitable_provider.rs (1)
852-865: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
source_statusparameter is never used by any caller.Both call sites (Lines 524, 567) and the tests pass
None, so thecustombranch always falls back tostatus. Either wire the record's currentsource_statusthrough fromupdate_board, or drop the parameter.As per coding guidelines: "Delete dead code and do not add compatibility shims or fallback paths without agreement; correct the primary path."
🤖 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/aitable_provider.rs` around lines 852 - 865, Remove the unused source_status parameter from source_status_for_write and update both callers, including update_board and the tests, to use the revised signature. Preserve the existing custom-mode fallback to status and mapped-status behavior; do not add compatibility shims or unused plumbing.Source: Coding guidelines
executor/src/task_runtime/aitable_provider_tests.rs (1)
76-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the pagination heuristic.
The subtlest logic in the provider is the
items.len() >= page_limitguard that suppresses DWS's trailing cursor (aitable_provider.rsLines 142-151) — getting it wrong causes either infinite paging or truncated boards. Extracting that cursor decision into a small pure helper would make it directly testable alongside these cases.🤖 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/aitable_provider_tests.rs` around lines 76 - 95, The pagination cursor decision in the provider, including the items.len() >= page_limit heuristic, is not directly tested. Extract the trailing-cursor decision from the provider flow into a small pure helper, then add cases alongside resolves_parent_tasks_from_link_ids_or_parent_titles covering full pages and short final pages, preserving behavior that suppresses DWS’s trailing cursor when the page is full.executor/src/local/app_ipc.rs (1)
747-855: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the
aitable.*arms into a dedicated dispatcher.
handle_task_runtime_requestis now a ~550-line match in a file past 1250 lines. Splitting the new domain (e.g.handle_aitable_request(method, ¶ms, &runtime)) keeps it cohesive and mirrors how the provider module is organized.Also, unlike
cells/field, theconfig/propertypayload at Line 820 is passed through without a type check, so a non-object value reaches the DWS--configflag and only fails at the CLI boundary. Adding.filter(Value::is_object)would keep validation consistent.As per coding guidelines: "Favor cohesive modules, explicit interfaces, and standard practices; split files over 1000 lines."
🤖 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/local/app_ipc.rs` around lines 747 - 855, Extract the aitable.* branches from handle_task_runtime_request into a dedicated handle_aitable_request dispatcher with an explicit method, params, and runtime interface, then delegate to it from the main match. In the aitable_create_field handling, validate config/property with an object-type filter before accepting it, preserving the existing default empty object for missing values and returning the same bad_request error for invalid payloads.Source: Coding guidelines
executor/src/task_runtime/credentials.rs (1)
211-227: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or exercise the
DingtalkAitablecredential pathDingTalk AI Table configs reject
tokenin project store/configuration paths, socredential_contextcannot be reached through encrypt/decrypt/preserve flow with a DWS-managed project credential. Theprovider_config.table_id is requiredbranch is therefore dead/uncovered unless there is another code path that legitimately decrypts this provider’s credentials.🤖 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/credentials.rs` around lines 211 - 227, The DingtalkAitable branch in the credential-context construction flow is currently unreachable through supported credential handling. Remove this dead branch, including its domain, base_id, table_id, and sheet_id parsing, unless an existing legitimate decrypt/preserve path can be wired to exercise it; preserve the surrounding provider credential behavior.backend/app/services/loop_items/external_provider.py (2)
275-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
urlfrom the decode.Ruff (RUF059) flags it; rename to
_urlto keep the tuple unpack explicit.🤖 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/loop_items/external_provider.py` around lines 275 - 296, In delete_attachment, rename the unused url variable returned by _decode_attachment_id to _url while keeping the tuple unpacking and all subsequent behavior unchanged.Source: Linters/SAST tools
108-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
attach_gitlab_uploadis ~100 lines and mixes legacy migration, upload, marker rewriting, and storage.Consider splitting the legacy-marker migration branch (Lines 124-152) and the upload+persist branch (Lines 154-205) into private helpers.
As per coding guidelines, "keep functions focused, preferably under 50 lines".
🤖 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/loop_items/external_provider.py` around lines 108 - 205, The attach_gitlab_upload method is too large and combines legacy attachment migration with new upload and persistence flows. Extract the legacy-marker handling around the existing attachment lookup, markdown rewrite, and external storage into a focused private helper, and extract the staged upload, issue update, and persistence flow into another private helper; keep attach_gitlab_upload responsible for project validation, authorization, issue retrieval, and dispatching between these helpers.Source: Coding guidelines
backend/tests/schemas/test_cloud_project_schema.py (1)
11-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the "only" part of
keeps_only_non_sensitive_locator_config.The test verifies trimming but never asserts which keys survive, so a regression that starts persisting
source_url(or any future sensitive key) would still pass.💚 Suggested assertion
assert project.provider_config["base_id"] == "base-1" assert project.provider_config["table_id"] == "table-1" + assert set(project.provider_config) == {"base_id", "table_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 `@backend/tests/schemas/test_cloud_project_schema.py` around lines 11 - 23, Update test_aitable_project_keeps_only_non_sensitive_locator_config to assert the complete provider_config key set contains only the permitted non-sensitive locator keys, base_id and table_id, ensuring source_url and any other sensitive keys are excluded while preserving the existing trimming assertions.backend/app/api/endpoints/feedback.py (1)
31-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
dictcheck before buildingFeedbackCreate.As written, a non-object JSON body (e.g.
"[1,2]") is first fed to the schema; ifFeedbackCreate.contextis typed as a mapping this branch is unreachable dead code, and if it is permissive the ordering is just confusing. Validating shape first keeps one clear failure path.♻️ Proposed reorder
try: parsed_context = json.loads(context) + if not isinstance(parsed_context, dict): + raise HTTPException( + status.HTTP_422_UNPROCESSABLE_ENTITY, "context must be an object" + ) values = FeedbackCreate( report_id=report_id, title=title, description=description, context=parsed_context, ) except (json.JSONDecodeError, ValidationError) as error: raise HTTPException(status.HTTP_422_UNPROCESSABLE_ENTITY, str(error)) from error - if not isinstance(parsed_context, dict): - raise HTTPException( - status.HTTP_422_UNPROCESSABLE_ENTITY, "context must be an object" - ) return feedback_service.submit(db, current_user, values, bundle)🤖 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/api/endpoints/feedback.py` around lines 31 - 44, In the feedback endpoint’s parsing flow, validate that parsed_context is a dict immediately after json.loads and before constructing FeedbackCreate. Keep the existing 422 response for non-object JSON, then build FeedbackCreate only after the shape check; preserve the current handling of JSONDecodeError and ValidationError.wework/src/api/aitable.ts (1)
13-39: 📐 Maintainability & Code Quality | 🔵 TrivialMixed snake_case/camelCase on normalized response shapes.
ai_config,active_table, andhas_moreuse snake_case while the rest of the TS surface (methods,LocalRequest) is camelCase. Sincerawalready exists specifically to preserve unprocessed DingTalk payloads, the normalized fields could consistently use camelCase, matching idiomatic TS conventions for the sake of downstream UI consumers.
[optional_and_nitpick]🤖 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/aitable.ts` around lines 13 - 39, Rename the normalized response fields ai_config, active_table, and has_more to aiConfig, activeTable, and hasMore in the AITableField, AITableDescription, and AITableRecordPage interfaces. Keep raw unchanged so the original DingTalk payload retains its snake_case fields, and update dependent consumers to use the camelCase names.wework/src/api/deliveries.ts (1)
88-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared
CloudProjectProviderConfigtype.The same ~11-field
provider_configshape (repository/domain/api_base/base_id/table_id/sheet_id/source_url/view_id/board_mapping/status_mode/status_mapping/custom_statuses) is duplicated verbatim acrossCloudProject,createCloudProject's request, andupdateCloudProject's request. Any future provider-config field will need to be added in three places, which already appears to have happened once in this PR.♻️ Proposed refactor
+export interface CloudProjectProviderConfig { + repository?: string + domain?: string + api_base?: string + credential_configured?: boolean + token?: string + base_id?: string + table_id?: string + sheet_id?: string + source_url?: string + view_id?: string + board_mapping?: Record<string, string> + status_mode?: 'mapped' | 'custom' + status_mapping?: Record<string, CloudLoopItem['status']> + custom_statuses?: string[] +} export interface CloudProject { ... - provider_config: { - repository?: string - ... - } + provider_config: CloudProjectProviderConfig }Then reuse
CloudProjectProviderConfig(minuscredential_configured, since it's server-only) in thecreateCloudProject/updateCloudProjectrequest shapes.Also applies to: 211-227, 243-252
🤖 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/deliveries.ts` around lines 88 - 104, Extract the duplicated provider_config fields into a shared CloudProjectProviderConfig type, including the existing optional provider fields and excluding server-only credential_configured. Update CloudProject, createCloudProject, and updateCloudProject request shapes to reference this shared type while preserving their current behavior and field optionality.wework/src/features/feedback/TaskFeedbackDialog.tsx (1)
146-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging the underlying submit error before showing the generic message.
The bare
catch {}discards the actual error entirely (network failure, auth issue, server error, etc.), which matches the test expectations for the user-facing message, but leaves zero diagnostic trail for debugging real submission failures in production.🔧 Proposed fix
- } catch { + } catch (submitError) { + console.error('Feedback submission failed', submitError) setError( reportId ? t('workbench.feedback_contact_developer_with_report', { reportId }) : t('workbench.feedback_contact_developer') )Based on coding guidelines:
**/*: "Diagnose problems using logs, actual code, and concrete evidence; when evidence is insufficient, add focused diagnostic logging before changing behavior and do not guess."🤖 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/feedback/TaskFeedbackDialog.tsx` around lines 146 - 178, Update the catch block in submitFeedback to capture the underlying submission error and log it with the component’s established logging mechanism before setting the existing generic user-facing error message. Preserve the current reportId-based messages and finally cleanup behavior.Source: Coding guidelines
wework/src/api/hybrid/cloudProjectSpaceApi.test.ts (1)
12-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead mock:
externalIssueApiassertions are now vacuous.Since
createCloudProjectSpaceApiis called with onlystoreApi(line 20), theexternalIssueApimock built at lines 12-19 is never wired to the function under test. The assertions at line 27 and 32-34 (not.toHaveBeenCalled()) are trivially true regardless of implementation correctness, since nothing connects this mock to the SUT. Consider removing the unused mock and its assertions to avoid implying they verify real behavior.♻️ Suggested cleanup
- const externalIssueApi = { - retainProjects: vi.fn(async () => { - throw new Error('local executor unavailable') - }), - configureProject: vi.fn(), - listLoopItems: vi.fn(), - createLoopItem: vi.fn(), - } as unknown as ExternalIssueApi const api = createCloudProjectSpaceApi(storeApi) await api.listCloudProjects() await api.listLoopItems('cloud-1') await api.createLoopItem('cloud-1', { title: 'Backend routed' }) expect(storeApi.listCloudProjects).toHaveBeenCalled() - expect(externalIssueApi.retainProjects).not.toHaveBeenCalled() expect(storeApi.listLoopItems).toHaveBeenCalledWith('cloud-1') expect(storeApi.createLoopItem).toHaveBeenCalledWith('cloud-1', { title: 'Backend routed', }) - expect(externalIssueApi.configureProject).not.toHaveBeenCalled() - expect(externalIssueApi.listLoopItems).not.toHaveBeenCalled() - expect(externalIssueApi.createLoopItem).not.toHaveBeenCalled()Also applies to: 27-34
🤖 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/hybrid/cloudProjectSpaceApi.test.ts` around lines 12 - 19, Remove the unused externalIssueApi mock and the associated not.toHaveBeenCalled assertions from the createCloudProjectSpaceApi test, since the SUT is constructed only with storeApi and cannot invoke that mock.wework/src/api/hybrid/cloudProjectSpaceApi.ts (1)
7-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFunction is now a no-op identity wrapper.
createCloudProjectSpaceApisimply returns{ ...storeApi }with no logic, yet the comment still frames it as an enforced "authorization boundary." Since there's no behavior to test or enforce, consider inliningstoreApidirectly at call sites (or documenting why the indirection is retained as a future extension point).🤖 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/hybrid/cloudProjectSpaceApi.ts` around lines 7 - 10, Remove the no-op createCloudProjectSpaceApi wrapper and pass storeApi directly at its call sites, unless the indirection is intentionally retained; if retained, document its future extension-point purpose and update the authorization-boundary comment to reflect that no enforcement occurs.
🤖 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/api/endpoints/deliveries.py`:
- Around line 384-397: Update read_loop_item_attachment to serve the file with
Content-Disposition attachment instead of inline, encode the provider-supplied
filename using RFC 5987 rather than interpolating it directly, and add
X-Content-Type-Options: nosniff to the response headers while preserving the
returned content and media type.
In `@backend/app/services/feedback_service.py`:
- Around line 32-65: Make feedback submission idempotent under concurrent
requests by enforcing uniqueness for the project, creator, and feedback report
identifier at the data layer, including the required Alembic migration. Update
submit() to catch IntegrityError from the create path, roll back the failed
transaction, re-fetch the existing item via _find_existing, and continue with
duplicate=True; preserve the current response and bundle behavior for both newly
created and existing feedback.
In `@backend/app/services/loop_items/external_provider.py`:
- Around line 257-273: The attachment_content method must enforce per-item
visibility before returning provider bytes, matching the local _get_attachment
path. Reuse the existing self.get(...) or equivalent visibility-checking flow to
validate item access, while retaining the project role check and storage
retrieval behavior for authorized callers.
- Around line 118-152: The new GitLab attachment methods must enforce per-item
permissions in addition to the project role. In
backend/app/services/loop_items/external_provider.py lines 118-152, capture the
access returned by require_cloud_project_role in the attachment rewrite/upload
method and reject when self._response(project, issue, access,
user_id)["can_edit"] is false before modifying or storing the attachment; in
lines 257-273, apply the same pattern in the attachment-byte retrieval method
using ["can_view_detail"] before returning bytes.
In `@backend/app/services/loop_items/service.py`:
- Around line 472-480: Update attachment_content to avoid unbounded whole-object
buffering: expose the attachment content through a streaming iterator and
StreamingResponse, or at minimum pass an appropriate max_bytes bound to
delivery_storage.get_bytes. Preserve the existing content type and display name
headers while ensuring large or concurrent downloads cannot load multi-GB blobs
into worker memory.
In `@executor/src/task_runtime/aitable_provider.rs`:
- Around line 456-499: Update get_board to fetch the single task through the
existing get_record method and project that record into a LoopItem instead of
calling list_board; preserve TaskNotFound behavior when the record is
unavailable and ensure update_board’s no-op path uses this efficient lookup. In
list_board, detect when the 50-page pagination cap is reached while a cursor
remains and emit a warning through the existing logging mechanism rather than
silently returning a truncated board.
- Around line 380-419: Update the AITable provider’s run method to use
tokio::fs::create_dir_all and invoke the DWS child with kill-on-drop enabled,
while wrapping the async output wait in an appropriate timeout so hung or
interactive commands cannot block indefinitely. Use a longer or separate timeout
for auth login if the existing command classification exposes it, and ensure
timeout handling returns TaskRuntimeError::ProviderRequest with clear context
while cleaning up and reaping the child.
In `@executor/src/task_runtime/mcp.rs`:
- Around line 611-642: Update the backend arms for create_table_record and
update_table_record to obtain cells through cells_argument instead of defaulting
missing values to an empty object, and serialize the validated result. Update
create_table_field to require name and field_type through string_argument,
preserving its existing type fallback if supported, so backend requests reject
the same invalid inputs as the local path.
In `@executor/src/task_runtime/router.rs`:
- Around line 83-86: Update the external task and attachment routing methods,
including get_external_task, create_external_task, update_external_task, and any
attachment handlers, so DingtalkAitable never reaches issue_provider
Get/Create/Update operations. Route it through the DingTalk AITable provider
path where supported; otherwise return UnsupportedProvider explicitly, matching
the existing list_board handling in the provider dispatch.
In `@wework/src-tauri/src/feedback.rs`:
- Around line 252-267: Update submit_feedback_bundle_blocking to validate the
full bundle_path against the app-owned feedback/downloads directories, matching
discard_feedback_bundle’s parent-directory check, before reading or uploading
the file. Keep the existing filename pattern validation, and reject paths whose
parent is not one of the permitted directories.
In `@wework/src/api/deliveries.ts`:
- Around line 332-352: Update downloadLoopItemAttachment to obtain content
through readLoopItemAttachment instead of repeating the client.getBlob request,
then encode the attachment bytes as base64 before passing them to
save_local_attachment_file. Update the corresponding Rust handler to accept the
base64 string and decode it into bytes before saving, preserving the existing
filename and file-opening behavior.
In `@wework/src/api/http.ts`:
- Around line 195-206: Update getBlob to use the same 401 handling as request,
including clearing the token and redirecting to login before throwing or
returning the error. Reuse the existing request authentication/redirect behavior
rather than introducing a separate flow, while preserving successful Blob
responses.
In `@wework/src/api/local/localServices.ts`:
- Around line 314-318: Update buildLocalRuntimeExecutionRequest to populate
backend_url, auth_token, and mcp_servers whenever input.cloudProjectId is
present, even when CloudModelGateway.backendUrl is unset. Fall back to
localExecutorBackendConnection or the existing local configuration source for
the gateway credentials and MCP server definitions before returning the request,
while preserving the current cloud gateway values when available.
In `@wework/src/features/feedback/TaskFeedbackDialog.tsx`:
- Around line 260-263: Replace the direct setPendingBundle(null) calls in the
checkbox and note onChange handlers with discardPendingBundle(), ensuring the
existing pending bundle is discarded before clearing it when either selection or
note content changes.
In `@wework/src/features/todo/AITableTaskFields.tsx`:
- Around line 119-127: Add error handling to the field-save flow in `save` and
the `updateField` path so rejected `onSave`/`api.updateRecord` promises are
caught rather than propagated from `void save(...)`. Introduce local error
state, set a user-facing message in each catch, and render it near the editor
while preserving the existing `saving` reset and successful-save behavior.
In `@wework/src/features/todo/CloudProjectManageView.tsx`:
- Around line 165-184: Update the AITable field-loading flow around the existing
configureProject/describe effect and DWS auth flow so a successful
dwsApi.login() triggers configureProject(project) followed by a fresh
aitableApi.describe(project.id), then updates aitableFields. Preserve the
existing active/unmount guard and error handling, and ensure the refetch occurs
only after login succeeds.
In `@wework/src/features/todo/CloudTodoWorkspace.tsx`:
- Around line 1088-1118: Update the custom-status branch in moveItem so
itemApi.updateLoopItem persists sourceStatus through the source_status field
rather than the enum status field. Keep the optimistic source_status update,
rollback behavior, and response reconciliation unchanged, while preserving
status as the fixed supported enum value.
---
Outside diff comments:
In `@wework/src/features/todo/TodoEditor.tsx`:
- Around line 568-579: Update removeAttachment to remove the deleted
attachment’s markdown references from description using the existing
stripAttachmentMarkdown helper or equivalent attachmentMarkdown patterns before
updating state. Ensure both wegent-attachment markers and wegent://attachments
links for attachment.id are stripped so visibleAttachments cannot recreate the
deleted row from markdownAttachmentRows(description).
---
Minor comments:
In `@executor/src/runtime_work/handler/tasks.rs`:
- Around line 214-216: Update the link restoration logic in send_message to run
after recovered_link is computed, and select existing_link or recovered_link via
the existing-link fallback. Pass the selected link’s runtime_handle to
restore_cloud_project_id so recovered-link follow-up turns retain
cloudProjectId.
In `@executor/src/task_runtime/mcp.rs`:
- Around line 590-592: Update the space MCP rename leftovers: in
executor/src/task_runtime/mcp.rs lines 590-592, change the
create_space/update_space rejection text to reference wework_space; in
executor/src/task_runtime/mcp.rs lines 507-512, change the item_id validation
message from task_id to item_id; in executor/src/bin/wegent-executor.rs lines
25-29, change the is_space_mcp_command() failure output to “space MCP server
failed”; rename executor/tests/local_task_mcp_contract.rs to
executor/tests/local_space_mcp_contract.rs.
In `@wework/src/api/local/localDelivery.ts`:
- Around line 454-459: The local download flow should preserve the requested
attachment filename. Update downloadLoopItemAttachment to accept the filename
passed by TodoEditor, then use it when opening or saving the file through the
local implementation so the behavior matches the cloud API rather than relying
only on access.path; update any callers and related signatures consistently.
In `@wework/src/features/todo/attachmentMarkdown.ts`:
- Around line 7-20: Update markdownAttachmentRows to deduplicate attachment rows
by their id after collecting matches from both attachmentMarkdownPatterns, while
preserving the existing row fields and match behavior. Ensure combined
marker-and-link markdown produces one row, and add coverage in
TodoEditor.test.ts for that case if the existing test structure supports it.
---
Nitpick comments:
In `@backend/app/api/endpoints/feedback.py`:
- Around line 31-44: In the feedback endpoint’s parsing flow, validate that
parsed_context is a dict immediately after json.loads and before constructing
FeedbackCreate. Keep the existing 422 response for non-object JSON, then build
FeedbackCreate only after the shape check; preserve the current handling of
JSONDecodeError and ValidationError.
In `@backend/app/services/loop_items/external_provider.py`:
- Around line 275-296: In delete_attachment, rename the unused url variable
returned by _decode_attachment_id to _url while keeping the tuple unpacking and
all subsequent behavior unchanged.
- Around line 108-205: The attach_gitlab_upload method is too large and combines
legacy attachment migration with new upload and persistence flows. Extract the
legacy-marker handling around the existing attachment lookup, markdown rewrite,
and external storage into a focused private helper, and extract the staged
upload, issue update, and persistence flow into another private helper; keep
attach_gitlab_upload responsible for project validation, authorization, issue
retrieval, and dispatching between these helpers.
In `@backend/app/services/runtime_work_service.py`:
- Around line 3516-3522: Add regression assertions in the relevant test block
around the runtime work guidance to verify the prompt includes wework_space,
list_spaces, get_board_item, and read_item_attachment, while confirming stale
wegent_delivery and wegent_tasks guidance is absent. Use the existing assertion
style and target the generated guidance text.
In `@backend/tests/schemas/test_cloud_project_schema.py`:
- Around line 11-23: Update
test_aitable_project_keeps_only_non_sensitive_locator_config to assert the
complete provider_config key set contains only the permitted non-sensitive
locator keys, base_id and table_id, ensuring source_url and any other sensitive
keys are excluded while preserving the existing trimming assertions.
In `@executor/src/local/app_ipc.rs`:
- Around line 747-855: Extract the aitable.* branches from
handle_task_runtime_request into a dedicated handle_aitable_request dispatcher
with an explicit method, params, and runtime interface, then delegate to it from
the main match. In the aitable_create_field handling, validate config/property
with an object-type filter before accepting it, preserving the existing default
empty object for missing values and returning the same bad_request error for
invalid payloads.
In `@executor/src/task_runtime/aitable_provider_tests.rs`:
- Around line 76-95: The pagination cursor decision in the provider, including
the items.len() >= page_limit heuristic, is not directly tested. Extract the
trailing-cursor decision from the provider flow into a small pure helper, then
add cases alongside resolves_parent_tasks_from_link_ids_or_parent_titles
covering full pages and short final pages, preserving behavior that suppresses
DWS’s trailing cursor when the page is full.
In `@executor/src/task_runtime/aitable_provider.rs`:
- Around line 852-865: Remove the unused source_status parameter from
source_status_for_write and update both callers, including update_board and the
tests, to use the revised signature. Preserve the existing custom-mode fallback
to status and mapped-status behavior; do not add compatibility shims or unused
plumbing.
In `@executor/src/task_runtime/credentials.rs`:
- Around line 211-227: The DingtalkAitable branch in the credential-context
construction flow is currently unreachable through supported credential
handling. Remove this dead branch, including its domain, base_id, table_id, and
sheet_id parsing, unless an existing legitimate decrypt/preserve path can be
wired to exercise it; preserve the surrounding provider credential behavior.
In `@executor/src/task_runtime/store.rs`:
- Around line 986-990: Replace the eprintln! call in the incompatible cached
external project handling with executor::logging::write_executor_error_line,
preserving the existing project ID and error details in the diagnostic message
and the subsequent return None behavior.
In `@executor/tests/local_task_mcp_contract.rs`:
- Around line 18-22: Rename the test file local_task_mcp_contract.rs to
local_space_mcp_contract.rs so its filename matches the space MCP tests it
exclusively contains. Update any references to the old filename if present,
without changing the test behavior.
In `@wework/src/api/aitable.ts`:
- Around line 13-39: Rename the normalized response fields ai_config,
active_table, and has_more to aiConfig, activeTable, and hasMore in the
AITableField, AITableDescription, and AITableRecordPage interfaces. Keep raw
unchanged so the original DingTalk payload retains its snake_case fields, and
update dependent consumers to use the camelCase names.
In `@wework/src/api/deliveries.ts`:
- Around line 88-104: Extract the duplicated provider_config fields into a
shared CloudProjectProviderConfig type, including the existing optional provider
fields and excluding server-only credential_configured. Update CloudProject,
createCloudProject, and updateCloudProject request shapes to reference this
shared type while preserving their current behavior and field optionality.
In `@wework/src/api/hybrid/cloudProjectSpaceApi.test.ts`:
- Around line 12-19: Remove the unused externalIssueApi mock and the associated
not.toHaveBeenCalled assertions from the createCloudProjectSpaceApi test, since
the SUT is constructed only with storeApi and cannot invoke that mock.
In `@wework/src/api/hybrid/cloudProjectSpaceApi.ts`:
- Around line 7-10: Remove the no-op createCloudProjectSpaceApi wrapper and pass
storeApi directly at its call sites, unless the indirection is intentionally
retained; if retained, document its future extension-point purpose and update
the authorization-boundary comment to reflect that no enforcement occurs.
In `@wework/src/features/feedback/TaskFeedbackDialog.tsx`:
- Around line 146-178: Update the catch block in submitFeedback to capture the
underlying submission error and log it with the component’s established logging
mechanism before setting the existing generic user-facing error message.
Preserve the current reportId-based messages and finally cleanup 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: ea743cf9-f295-498f-ae8d-791574bb5917
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlwework/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (93)
backend/.env.examplebackend/app/api/api.pybackend/app/api/endpoints/deliveries.pybackend/app/api/endpoints/feedback.pybackend/app/core/config.pybackend/app/main.pybackend/app/mcp_server/server.pybackend/app/mcp_server/tools/delivery.pybackend/app/models/delivery.pybackend/app/schemas/cloud_project.pybackend/app/schemas/delivery.pybackend/app/schemas/feedback.pybackend/app/services/feedback_service.pybackend/app/services/loop_items/external_provider.pybackend/app/services/loop_items/provider_router.pybackend/app/services/loop_items/service.pybackend/app/services/runtime_work_service.pybackend/tests/api/test_cloud_projects_api.pybackend/tests/api/test_deliveries_api.pybackend/tests/api/test_feedback_api.pybackend/tests/mcp_server/test_delivery_todo_tools.pybackend/tests/mcp_server/test_delivery_tools.pybackend/tests/schemas/test_cloud_project_schema.pybackend/tests/services/test_runtime_work_service.pyexecutor/src/agents/mod.rsexecutor/src/bin/wegent-executor.rsexecutor/src/local/app_ipc.rsexecutor/src/local/backend.rsexecutor/src/runtime_work/handler.rsexecutor/src/runtime_work/handler/tasks.rsexecutor/src/runtime_work/handler/turns.rsexecutor/src/runtime_work/util.rsexecutor/src/task_runtime/aitable_provider.rsexecutor/src/task_runtime/aitable_provider_tests.rsexecutor/src/task_runtime/credentials.rsexecutor/src/task_runtime/issue_provider.rsexecutor/src/task_runtime/mcp.rsexecutor/src/task_runtime/mod.rsexecutor/src/task_runtime/model.rsexecutor/src/task_runtime/router.rsexecutor/src/task_runtime/store.rsexecutor/tests/local_task_mcp_contract.rspnpm-workspace.yamlwework/package.jsonwework/scripts/build-mac-app.shwework/scripts/build-windows-app.shwework/scripts/dev-mac-app.shwework/scripts/prepare-dws-binary.mjswework/src-tauri/Cargo.tomlwework/src-tauri/src/feedback.rswework/src-tauri/src/lib.rswework/src-tauri/tauri.conf.jsonwework/src/api/aitable.tswework/src/api/backend/backendServices.tswework/src/api/deliveries.tswework/src/api/dws.tswework/src/api/feedback.test.tswework/src/api/feedback.tswework/src/api/http.tswework/src/api/hybrid/cloudProjectSpaceApi.test.tswework/src/api/hybrid/cloudProjectSpaceApi.tswework/src/api/hybrid/hybridServices.test.tswework/src/api/hybrid/hybridServices.tswework/src/api/local/localDelivery.tswework/src/api/local/localServices.test.tswework/src/api/local/localServices.tswework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/useWorkbenchPaneSession.tswework/src/features/feedback/TaskFeedbackDialog.test.tsxwework/src/features/feedback/TaskFeedbackDialog.tsxwework/src/features/todo/AITableTaskFields.test.tsxwework/src/features/todo/AITableTaskFields.tsxwework/src/features/todo/AITableView.test.tsxwework/src/features/todo/AITableView.tsxwework/src/features/todo/CloudProjectManageView.tsxwework/src/features/todo/CloudProjectsHome.tsxwework/src/features/todo/CloudTodoWorkspace.test.tsxwework/src/features/todo/CloudTodoWorkspace.tsxwework/src/features/todo/TaskDescriptionEditor.test.tsxwework/src/features/todo/TaskDescriptionEditor.tsxwework/src/features/todo/TodoEditor.test.tswework/src/features/todo/TodoEditor.tsxwework/src/features/todo/TodoNavigation.tsxwework/src/features/todo/attachmentMarkdown.tswework/src/features/todo/projectProviderConfig.test.tswework/src/features/todo/projectProviderConfig.tswework/src/features/workbench/WorkbenchProvider.test.tsxwework/src/features/workbench/useWorkbenchRuntimeMessaging.tswework/src/features/workbench/workbenchContextTypes.tswework/src/features/workbench/workbenchServices.test.tswework/src/features/workbench/workbenchServices.tswework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.json
💤 Files with no reviewable changes (6)
- backend/tests/mcp_server/test_delivery_todo_tools.py
- wework/src/api/hybrid/hybridServices.test.ts
- backend/tests/mcp_server/test_delivery_tools.py
- backend/app/main.py
- backend/app/mcp_server/tools/delivery.py
- backend/app/mcp_server/server.py
| @router.get("/loop-item-attachments/{attachment_id}/content") | ||
| def read_loop_item_attachment( | ||
| attachment_id: str, | ||
| db: Session = Depends(get_db), | ||
| current_user: User = Depends(get_current_user), | ||
| ) -> Response: | ||
| content, content_type, filename = loop_item_attachment_provider_router.content( | ||
| db, attachment_id, current_user.id | ||
| ) | ||
| return Response( | ||
| content=content, | ||
| media_type=content_type, | ||
| headers={"Content-Disposition": f'inline; filename="{filename}"'}, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Serving attachment bytes inline with a provider-supplied media_type and unescaped filename is risky.
Two concerns on this new endpoint:
content_typeoriginates from the uploaded file (file.content_typeat Line 359 for local attachments). Servingtext/htmlorimage/svg+xmlinlinefrom the API origin allows stored XSS against any session-authenticated browser context.filenameis unsanitized user input; a"or CR/LF in the display name corrupts or injects response headers.
Prefer Content-Disposition: attachment with RFC 5987 encoding, plus X-Content-Type-Options: nosniff.
🔒 Proposed fix
+from urllib.parse import quote as _url_quote content, content_type, filename = loop_item_attachment_provider_router.content(
db, attachment_id, current_user.id
)
+ safe_name = _url_quote(filename or "attachment")
return Response(
content=content,
media_type=content_type,
- headers={"Content-Disposition": f'inline; filename="{filename}"'},
+ headers={
+ "Content-Disposition": f"attachment; filename*=UTF-8''{safe_name}",
+ "X-Content-Type-Options": "nosniff",
+ },
)📝 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.
| @router.get("/loop-item-attachments/{attachment_id}/content") | |
| def read_loop_item_attachment( | |
| attachment_id: str, | |
| db: Session = Depends(get_db), | |
| current_user: User = Depends(get_current_user), | |
| ) -> Response: | |
| content, content_type, filename = loop_item_attachment_provider_router.content( | |
| db, attachment_id, current_user.id | |
| ) | |
| return Response( | |
| content=content, | |
| media_type=content_type, | |
| headers={"Content-Disposition": f'inline; filename="{filename}"'}, | |
| ) | |
| from urllib.parse import quote as _url_quote | |
| `@router.get`("/loop-item-attachments/{attachment_id}/content") | |
| def read_loop_item_attachment( | |
| attachment_id: str, | |
| db: Session = Depends(get_db), | |
| current_user: User = Depends(get_current_user), | |
| ) -> Response: | |
| content, content_type, filename = loop_item_attachment_provider_router.content( | |
| db, attachment_id, current_user.id | |
| ) | |
| safe_name = _url_quote(filename or "attachment") | |
| return Response( | |
| content=content, | |
| media_type=content_type, | |
| headers={ | |
| "Content-Disposition": f"attachment; filename*=UTF-8''{safe_name}", | |
| "X-Content-Type-Options": "nosniff", | |
| }, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 387-387: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 388-388: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 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/api/endpoints/deliveries.py` around lines 384 - 397, Update
read_loop_item_attachment to serve the file with Content-Disposition attachment
instead of inline, encode the provider-supplied filename using RFC 5987 rather
than interpolating it directly, and add X-Content-Type-Options: nosniff to the
response headers while preserving the returned content and media type.
| project = self._configured_project(db) | ||
| existing = self._find_existing(db, project, user.id, values.report_id) | ||
| if existing is None: | ||
| created = loop_item_provider_router.create( | ||
| db, | ||
| project, | ||
| user, | ||
| LoopItemCreate( | ||
| title=values.title, | ||
| description=self._description(values), | ||
| tags=["feedback", "wework"], | ||
| ), | ||
| ) | ||
| item_id = str(created.values["id"]) | ||
| internal_item = created.internal_item | ||
| if internal_item is not None: | ||
| internal_item.metadata_json = { | ||
| **internal_item.metadata_json, | ||
| "feedback_report_id": values.report_id, | ||
| } | ||
| db.commit() | ||
| else: | ||
| item_id, internal_item = existing | ||
|
|
||
| self._ensure_bundle( | ||
| db, project, item_id, internal_item, user.id, values.report_id, bundle | ||
| ) | ||
| return FeedbackResponse( | ||
| report_id=values.report_id, | ||
| project_id=str(project.id), | ||
| item_id=item_id, | ||
| created_by_user_id=user.id, | ||
| duplicate=existing is not None, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Duplicate-feedback race: check-then-act dedup has no synchronization.
_find_existing (read) and the create call (write) in submit() are not atomic. Two concurrent submissions with the same report_id (e.g., a client retry after a timeout) can both see existing is None and each create a separate GitHub/GitLab issue or LoopItem, defeating the intended idempotency (FeedbackResponse.duplicate).
Consider enforcing idempotency at the data layer, e.g. a unique constraint/index on (cloud_project_id, created_by_user_id, feedback_report_id) for internal items (would need an Alembic migration per this repo's backend model-change policy) combined with a try/except IntegrityError fallback to re-fetch the existing item, rather than relying solely on an in-memory dedup scan.
🤖 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/feedback_service.py` around lines 32 - 65, Make feedback
submission idempotent under concurrent requests by enforcing uniqueness for the
project, creator, and feedback report identifier at the data layer, including
the required Alembic migration. Update submit() to catch IntegrityError from the
create path, roll back the failed transaction, re-fetch the existing item via
_find_existing, and continue with duplicate=True; preserve the current response
and bundle behavior for both newly created and existing feedback.
| project, number = self._resolve_project(db, item_id) | ||
| if project.task_provider != "gitlab": | ||
| return None | ||
| require_cloud_project_role(db, project.id, user_id, BaseRole.RestrictedAnalyst) | ||
| issue = self._get_issue(project, number) | ||
| description = str(issue.get("description") or "") | ||
| if filename in description: | ||
| attachment = next( | ||
| ( | ||
| attachment | ||
| for attachment in self._gitlab_attachments(project, item_id, issue) | ||
| if attachment["display_name"] == filename | ||
| ), | ||
| None, | ||
| ) | ||
| if attachment is not None and "wegent://attachments/" in description: | ||
| native_markdown = ( | ||
| f"[{filename}]({self._decode_attachment_id(str(attachment['id']))[1]})\n" | ||
| f"<!-- wegent-attachment:{attachment['id']} -->" | ||
| ) | ||
| description = LEGACY_WEGENT_ATTACHMENT_PATTERN.sub( | ||
| lambda match: ( | ||
| native_markdown | ||
| if match.group("id") == attachment["id"] | ||
| else match.group(0) | ||
| ), | ||
| description, | ||
| ) | ||
| self._update_issue(project, number, {"description": description}) | ||
| attachment["markdown"] = native_markdown | ||
| if attachment is not None: | ||
| self._store_external_attachment( | ||
| str(attachment["id"]), source, max_size_bytes | ||
| ) | ||
| return attachment |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
New GitLab attachment methods authorize by project role only, skipping the per-item can_view_detail/can_edit gate. Every pre-existing path (get, update, delete_attachment) derives permissions from self._response(project, issue, access, user_id) after the role check; the two new methods stop at require_cloud_project_role(..., RestrictedAnalyst), which is the lowest role and also admits public visitors.
backend/app/services/loop_items/external_provider.py#L118-L152: capture theaccessreturned byrequire_cloud_project_roleand reject when_response(...)["can_edit"]is false, before uploading or rewriting the issue description.backend/app/services/loop_items/external_provider.py#L257-L273: apply the same pattern with_response(...)["can_view_detail"]before returning attachment bytes.
📍 Affects 1 file
backend/app/services/loop_items/external_provider.py#L118-L152(this comment)backend/app/services/loop_items/external_provider.py#L257-L273
🤖 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/loop_items/external_provider.py` around lines 118 - 152,
The new GitLab attachment methods must enforce per-item permissions in addition
to the project role. In backend/app/services/loop_items/external_provider.py
lines 118-152, capture the access returned by require_cloud_project_role in the
attachment rewrite/upload method and reject when self._response(project, issue,
access, user_id)["can_edit"] is false before modifying or storing the
attachment; in lines 257-273, apply the same pattern in the attachment-byte
retrieval method using ["can_view_detail"] before returning bytes.
| def attachment_content( | ||
| self, db: Session, attachment_id: str, user_id: int | ||
| ) -> tuple[bytes, str, str]: | ||
| item_id, url = self._decode_attachment_id(attachment_id) | ||
| project, _ = self._resolve_project(db, item_id) | ||
| require_cloud_project_role(db, project.id, user_id, BaseRole.RestrictedAnalyst) | ||
| try: | ||
| content = delivery_storage.get_bytes( | ||
| self._external_attachment_key(attachment_id) | ||
| ) | ||
| except Exception as exc: | ||
| raise HTTPException( | ||
| status.HTTP_404_NOT_FOUND, | ||
| "Provider attachment is not available in unified storage", | ||
| ) from exc | ||
| filename = url.rstrip("/").rsplit("/", 1)[-1] or "attachment" | ||
| return content, "application/octet-stream", filename |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
attachment_content skips the per-item visibility check.
Only project role is verified; _response(...)["can_view_detail"] is never consulted, so a public visitor who does not own the item can still fetch the raw bytes of another user's attachment (this is the path the new /loop-item-attachments/{id}/content endpoint calls). The local service path enforces this via self.get(...) in _get_attachment; the external path should match.
🤖 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/loop_items/external_provider.py` around lines 257 - 273,
The attachment_content method must enforce per-item visibility before returning
provider bytes, matching the local _get_attachment path. Reuse the existing
self.get(...) or equivalent visibility-checking flow to validate item access,
while retaining the project role check and storage retrieval behavior for
authorized callers.
| def attachment_content( | ||
| self, db: Session, attachment_id: str, user_id: int | ||
| ) -> tuple[bytes, str, str]: | ||
| attachment = self._get_attachment(db, attachment_id, user_id) | ||
| return ( | ||
| delivery_storage.get_bytes(attachment.object_key), | ||
| attachment.content_type or "application/octet-stream", | ||
| attachment.display_name, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
attachment_content buffers the entire object in memory.
DELIVERY_MAX_ASSET_SIZE_MB defaults to 2048, so a single request to the new /loop-item-attachments/{id}/content endpoint can pull a multi-GB blob into the worker process (and FastAPI then holds a second copy in the Response body). A handful of concurrent downloads will OOM the backend.
Consider exposing a streaming variant (delivery_storage iterator + StreamingResponse), or at minimum bound the read with the existing max_bytes argument that get_bytes already accepts.
🤖 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/loop_items/service.py` around lines 472 - 480, Update
attachment_content to avoid unbounded whole-object buffering: expose the
attachment content through a streaming iterator and StreamingResponse, or at
minimum pass an appropriate max_bytes bound to delivery_storage.get_bytes.
Preserve the existing content type and display name headers while ensuring large
or concurrent downloads cannot load multi-GB blobs into worker memory.
| interface CloudModelGateway { | ||
| baseUrl: string | ||
| apiKey: string | ||
| mcpUrl?: string | ||
| backendUrl?: string | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'localServices|hybridServices' . | sed 's#^\./##'
echo "== localServices outline =="
ast-grep outline wework/src/api/local/localServices.ts --view compact || true
echo "== hybridServices outline =="
ast-grep outline wework/src/api/hybrid/hybridServices.ts --view compact || true
echo "== relevant localServices sections =="
sed -n '280,340p' wework/src/api/local/localServices.ts
echo "---"
sed -n '1190,1240p' wework/src/api/local/localServices.ts
echo "== relevant hybridServices sections =="
sed -n '45,70p' wework/src/api/hybrid/hybridServices.ts
echo "---"
sed -n '270,300p' wework/src/api/hybrid/hybridServices.tsRepository: wecode-ai/Wegent
Length of output: 6517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== localServices relevant functions =="
rg -n "function createLocalAppServices|createLocalAppServices|CloudModelGateway|messageWithApplicationContext|messageWithApplicationContext\\(" wework/src/api/local/localServices.ts
echo "--- localServices sections around create and helpers ---"
sed -n '340,520p' wework/src/api/local/localServices.ts
echo "--- localServices sections 1120-1190 ---"
sed -n '1120,1190p' wework/src/api/local/localServices.ts
echo "--- localServices section 1240-1320 ---"
sed -n '1240,1320p' wework/src/api/local/localServices.ts
echo "== hybridServices relevant functions =="
rg -n "function createHybridWorkbenchServices|createHybridWorkbenchServices|createBackendWorkbenchServices|runtimeProjectKey|backendUrl|modelOptions|workspacePath|additionalContext" wework/src/api/hybrid/hybridServices.ts
echo "--- hybridServices sections 70-180 ---"
sed -n '70,180p' wework/src/api/hybrid/hybridServices.ts
echo "--- hybridServices sections 250-350 ---"
sed -n '250,350p' wework/src/api/hybrid/hybridServices.ts
echo "--- hybridServices sections 350-560 ---"
sed -n '350,560p' wework/src/api/hybrid/hybridServices.tsRepository: wecode-ai/Wegent
Length of output: 28697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== localServices createLocalAppServices call graph =="
rg -n "buildLocalRuntimeExecutionRequest|createRuntimeExecutionIdsFromSeed|localRuntimeModelConfig|applyRuntimeModelOptions|executeLocalRuntime|runtimeProjectKey|cloudProjectId|HybridWorkbenchServicesOptions|runtimeProjectKey|runtime_workspace_roots" wework/src/api/local/localServices.ts wework/src/api/hybrid/hybridServices.ts
echo "--- localServices createLocalAppServices 2160-2360 ---"
sed -n '2160,2280p' wework/src/api/local/localServices.ts
echo "--- hybridServices runtimeProjectKey create section 760-880 ---"
sed -n '760,880p' wework/src/api/hybrid/hybridServices.ts
echo "== search for createHybridWorkbenchServices options from caller side =="
rg -n "createHybridWorkbenchServices\\(|createLocalAppServices\\(|HybridWorkbenchServicesOptions|backendUrl" wework/src apps | head -200Repository: wecode-ai/Wegent
Length of output: 27463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WorkbenchProvider relevant sections =="
sed -n '150,210p' wework/src/features/workbench/WorkbenchProvider.tsx
sed -n '1930,2010p' wework/src/features/workbench/WorkbenchProvider.tsx
echo "== workbenchServices createHybrid condition =="
sed -n '130,175p' wework/src/features/workbench/workbenchServices.ts
echo "== localServices runtime create branches =="
sed -n '1350,1495p' wework/src/api/local/localServices.ts
echo "== localServices tests around cloudModelGateway/backend_url/mcp_servers =="
sed -n '1250,1310p' wework/src/api/local/localServices.test.tsRepository: wecode-ai/Wegent
Length of output: 11345
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== localExecutor.ts =="
sed -n '1,110p' wework/src/tauri/localExecutor.ts
echo "== LocalRuntimeInitializer.tsx =="
sed -n '100,155p' wework/src/features/local-runtime/LocalRuntimeInitializer.tsx
echo "== App initial provider/callers =="
sed -n '240,300p' wework/src/App.tsx
echo "== static verifier for backendUrl unset payload behavior =="
python3 - <<'PY'
from pathlib import Path
import re
text = Path('wework/src/api/local/localServices.ts').read_text()
build = re.search(r'function buildLocalRuntimeExecutionRequest[^(]*\([^)]*\):[^\n]+\n(?:.*\n)*?\n}', text)
if not build:
print('buildLocalRuntimeExecutionRequest not found')
raise SystemExit(1)
body = build.group(0)
checks = {
'backendUrl conditional adds backend_url/auth_token': '...(input.cloudModelGateway?.backendUrl' in body,
'mcp_servers is hardcoded empty array': 'mcp_servers: [],' in body,
'has cloudProjectId passed through unconditionally': '...(input.cloudProjectId ?' in body,
'has runtimeProjectKey passed through unconditionally': '...input.runtimeProjectKey ? { runtime_project_key:' in body,
'has projectSpaceCapability added when cloudProjectId/set model options': bool(re.search(r'cloudProjectId.*projectSpaceCapability|cloudProjectId \? \{.*projectSpaceCapability', body, re.S)),
}
for name, result in checks.items():
print(f'{name}: {result}')
if checks['backendUrl conditional adds backend_url/auth_token'] and checks['mcp_servers is hardcoded empty array'] and checks['has cloudProjectId passed through unconditionally']:
print('behavior invariant: payload depends on input.cloudModelGateway.backendUrl for backend auth and mcp_servers')
PYRepository: wecode-ai/Wegent
Length of output: 7914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== localServices buildLocalRuntimeExecutionRequest exact search =="
rg -n -C 3 "function buildLocalRuntimeExecutionRequest|mcp_servers: \\[\\]|cloudModelGateway\\?\\.backendUrl|cloudProjectId|runtime_project_key|mcp_servers" wework/src/api/local/localServices.ts
echo "== localServices lines 1150-1255 =="
sed -n '1150,1255p' wework/src/api/local/localServices.tsRepository: wecode-ai/Wegent
Length of output: 6530
Backfill missing gateway credentials when a cloud project is referenced
buildLocalRuntimeExecutionRequest currently omits backend_url/auth_token and leaves mcp_servers: [] when cloudModelGateway.backendUrl is unset, while input.cloudProjectId can still be included. Since messageWithApplicationContext also injects wework_space guidance whenever cloud://projects is present and project-space capability is missing, tasks can be sent with a cloud project reference but no credentials or MCP servers to execute it. Add a fallback to construct these values from localExecutorBackendConnection or another available local config before returning this request.
🤖 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 314 - 318, Update
buildLocalRuntimeExecutionRequest to populate backend_url, auth_token, and
mcp_servers whenever input.cloudProjectId is present, even when
CloudModelGateway.backendUrl is unset. Fall back to
localExecutorBackendConnection or the existing local configuration source for
the gateway credentials and MCP server definitions before returning the request,
while preserving the current cloud gateway values when available.
| async function save(next: unknown = draft) { | ||
| setSaving(true) | ||
| try { | ||
| await onSave(normalizeValue(field, value, next)) | ||
| onCancel() | ||
| } finally { | ||
| setSaving(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Field-save failures are silently swallowed (unhandled promise rejection, no user feedback).
save() (lines 119-127) has no catch, and it's invoked as void save(...) from the button's onClick (line 176). updateField() (lines 376-383) also has no error handling. If api.updateRecord rejects (network error, validation failure, etc.), the rejection becomes unhandled: saving resets to false via finally, but the user gets zero indication that their edit failed to persist — the editor just sits there with no error message.
Add a catch that surfaces an error message (e.g., a local error state rendered near the editor) instead of letting the rejection go unhandled.
🐛 Suggested fix
- const [draft, setDraft] = useState(textValue(value))
- const [saving, setSaving] = useState(false)
+ const [draft, setDraft] = useState(textValue(value))
+ const [saving, setSaving] = useState(false)
+ const [saveError, setSaveError] = useState<string | null>(null)
const options = fieldOptions(field)
async function save(next: unknown = draft) {
setSaving(true)
+ setSaveError(null)
try {
await onSave(normalizeValue(field, value, next))
onCancel()
+ } catch (cause) {
+ setSaveError(cause instanceof Error ? cause.message : '保存失败')
} finally {
setSaving(false)
}
}Also applies to: 166-180, 376-383
🤖 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/todo/AITableTaskFields.tsx` around lines 119 - 127, Add
error handling to the field-save flow in `save` and the `updateField` path so
rejected `onSave`/`api.updateRecord` promises are caught rather than propagated
from `void save(...)`. Introduce local error state, set a user-facing message in
each catch, and render it near the editor while preserving the existing `saving`
reset and successful-save behavior.
| useEffect(() => { | ||
| if (!isAITableProvider || !aitableApi) return | ||
| let active = true | ||
| void aitableApi | ||
| .configureProject(project) | ||
| .then(() => aitableApi.describe(project.id)) | ||
| .then(value => active && setAitableFields(value.fields)) | ||
| .catch(cause => active && setError(cause instanceof Error ? cause.message : '加载字段失败')) | ||
| return () => { | ||
| active = false | ||
| } | ||
| }, [aitableApi, isAITableProvider, project]) | ||
|
|
||
| useEffect(() => { | ||
| if (!isAITableProvider || !dwsApi) return | ||
| void dwsApi | ||
| .authStatus() | ||
| .then(setDwsStatus) | ||
| .catch(() => setDwsStatus(null)) | ||
| }, [dwsApi, isAITableProvider]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether AITable describe/list calls require DWS auth to succeed,
# and whether any other code path refetches aitableFields after dws login.
rg -n "authStatus|describe\(" wework/src/api/aitable.ts wework/src/api/dws.tsRepository: wecode-ai/Wegent
Length of output: 436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate files =="
fd -a 'CloudProjectManageView\.tsx$|aitable\.ts$|dws\.ts$' . | sed 's#^\./##'
echo
echo "== CloudProjectManageView outline =="
ast-grep outline wework/src/features/todo/CloudProjectManageView.tsx || true
echo
echo "== Relevant CloudProjectManageView sections =="
sed -n '1,240p' wework/src/features/todo/CloudProjectManageView.tsx
echo "---"
sed -n '640,900p' wework/src/features/todo/CloudProjectManageView.tsx
echo
echo "== API relevant sections =="
sed -n '1,160p' wework/src/api/aitable.ts
echo "---"
sed -n '1,100p' wework/src/api/dws.ts
echo
echo "== Search for dwsStatus refs and aitableFields refresh triggers =="
rg -n "dwsStatus|setDwsStatus|setAitableFields|aitableFields|authStatus|连接钉钉|configureProject|describe\\(" wework/src/features/todo/CloudProjectManageView.tsx wework/src -g '*.tsx' -2Repository: wecode-ai/Wegent
Length of output: 27369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All dwsStatus refs =="
rg -n "dwsStatus|setDwsStatus|setAitableFields|refresh|reload|fields" wework/src/features/todo/CloudProjectManageView.tsx
echo
echo "== All AITable describe/configure/search docs/tests =="
rg -n "aitable\.describe|configureProject|describe\\(|DWS|dws|钉钉多维表格|连接钉钉|field mapping|board_mapping|aitable.*field|字段映射" -S .
echo
echo "== Inspect DOWORK/AGENTS if present =="
for f in wework/AGENTS.md DESIGN.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
doneRepository: wecode-ai/Wegent
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
executor/src/task_runtime/aitable_provider.rs \
executor/src/task_runtime/credentials.rs \
executor/src/task_runtime/router.rs
do
echo "--- $f ---"
sed -n '1,160p' "$f"
done
echo "--- focused dws/aitable auth sections ---"
rg -n "pub async fn dws_auth_status|pub async fn dws_auth_login|pub async fn aitable_describe|DingTalk authentication|authentication|requires|authenticated|auth_required|login|token|cors|dws" executor/src/task_runtime/aitable_provider.rs executor/src/task_runtime/credentials.rs executor/src/task_runtime/router.rs wework/src/api -g '*.rs' -g '*.ts'Repository: wecode-ai/Wegent
Length of output: 34835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Test around DWS auth requirement for aitable =="
sed -n '1490,1525p' executor/src/task_runtime/router.rs
sed -n '340,390p' executor/src/task_runtime/aitable_provider.rs
echo
echo "== CloudProjectManageView AITable config/save/update sections =="
sed -n '430,620p' wework/src/features/todo/CloudProjectManageView.tsx
sed -n '900,1020p' wework/src/features/todo/CloudProjectManageView.tsxRepository: wecode-ai/Wegent
Length of output: 15030
Refetch AITable fields after DWS login.
aitableApi.describe() runs through isolated DWS, but the field-loading effect only depends on [aitableApi, isAITableProvider, project], so successfully clicking “连接钉钉” does not update aitableFields and the mapping dropdowns remain empty. Add a DWS-authenticated-refetch path after dwsApi.login() succeeds.
🤖 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/todo/CloudProjectManageView.tsx` around lines 165 - 184,
Update the AITable field-loading flow around the existing
configureProject/describe effect and DWS auth flow so a successful
dwsApi.login() triggers configureProject(project) followed by a fresh
aitableApi.describe(project.id), then updates aitableFields. Preserve the
existing active/unmount guard and error handling, and ensure the refetch occurs
only after login succeeds.
| async function moveItem( | ||
| itemId: string, | ||
| status: CloudLoopItem['status'], | ||
| beforeItemId: string | null = null | ||
| beforeItemId: string | null = null, | ||
| sourceStatus: string | null = null | ||
| ) { | ||
| const item = items.find(candidate => candidate.id === itemId) | ||
| if (usesCustomStatusLanes && item && sourceStatus) { | ||
| if (item.can_edit === false || item.source_status === sourceStatus) return | ||
| const previousItems = items | ||
| setItems(current => | ||
| current.map(candidate => | ||
| candidate.id === item.id ? { ...candidate, source_status: sourceStatus } : candidate | ||
| ) | ||
| ) | ||
| try { | ||
| const itemApi = apiForProjectId(item.cloud_project_id) | ||
| if (!itemApi) throw new Error('项目空间当前不可用') | ||
| const updated = await itemApi.updateLoopItem(item.id, { | ||
| version: item.version, | ||
| status: sourceStatus as CloudLoopItem['status'], | ||
| }) | ||
| setItems(current => | ||
| current.map(candidate => (candidate.id === updated.id ? updated : candidate)) | ||
| ) | ||
| } catch (cause) { | ||
| setItems(previousItems) | ||
| setBoardError(cause instanceof Error ? cause.message : '移动任务失败') | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Custom-lane move persists to the wrong field (status instead of source_status).
The custom-lane branch optimistically updates source_status locally (Line 1100) but sends status: sourceStatus to the backend (Lines 1106-1109). Elsewhere in this same file, source_status is clearly the field meant to carry the lane's raw string (columns are built with a fixed status: 'inbox' and a separate sourceStatus, and finishBoardDrop passes column.sourceStatus/target.source_status through). Sending the custom lane value as status means:
- The real lane change is never persisted as
source_status, so the optimistic update will revert on the next 15s poll/refresh. - The enum-typed
statusfield gets overwritten with an arbitrary DingTalk field value, which is likely invalid for that field.
🐛 Proposed fix
const itemApi = apiForProjectId(item.cloud_project_id)
if (!itemApi) throw new Error('项目空间当前不可用')
const updated = await itemApi.updateLoopItem(item.id, {
version: item.version,
- status: sourceStatus as CloudLoopItem['status'],
+ source_status: sourceStatus,
})📝 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.
| async function moveItem( | |
| itemId: string, | |
| status: CloudLoopItem['status'], | |
| beforeItemId: string | null = null | |
| beforeItemId: string | null = null, | |
| sourceStatus: string | null = null | |
| ) { | |
| const item = items.find(candidate => candidate.id === itemId) | |
| if (usesCustomStatusLanes && item && sourceStatus) { | |
| if (item.can_edit === false || item.source_status === sourceStatus) return | |
| const previousItems = items | |
| setItems(current => | |
| current.map(candidate => | |
| candidate.id === item.id ? { ...candidate, source_status: sourceStatus } : candidate | |
| ) | |
| ) | |
| try { | |
| const itemApi = apiForProjectId(item.cloud_project_id) | |
| if (!itemApi) throw new Error('项目空间当前不可用') | |
| const updated = await itemApi.updateLoopItem(item.id, { | |
| version: item.version, | |
| status: sourceStatus as CloudLoopItem['status'], | |
| }) | |
| setItems(current => | |
| current.map(candidate => (candidate.id === updated.id ? updated : candidate)) | |
| ) | |
| } catch (cause) { | |
| setItems(previousItems) | |
| setBoardError(cause instanceof Error ? cause.message : '移动任务失败') | |
| } | |
| return | |
| } | |
| async function moveItem( | |
| itemId: string, | |
| status: CloudLoopItem['status'], | |
| beforeItemId: string | null = null, | |
| sourceStatus: string | null = null | |
| ) { | |
| const item = items.find(candidate => candidate.id === itemId) | |
| if (usesCustomStatusLanes && item && sourceStatus) { | |
| if (item.can_edit === false || item.source_status === sourceStatus) return | |
| const previousItems = items | |
| setItems(current => | |
| current.map(candidate => | |
| candidate.id === item.id ? { ...candidate, source_status: sourceStatus } : candidate | |
| ) | |
| ) | |
| try { | |
| const itemApi = apiForProjectId(item.cloud_project_id) | |
| if (!itemApi) throw new Error('项目空间当前不可用') | |
| const updated = await itemApi.updateLoopItem(item.id, { | |
| version: item.version, | |
| source_status: sourceStatus, | |
| }) | |
| setItems(current => | |
| current.map(candidate => (candidate.id === updated.id ? updated : candidate)) | |
| ) | |
| } catch (cause) { | |
| setItems(previousItems) | |
| setBoardError(cause instanceof Error ? cause.message : '移动任务失败') | |
| } | |
| return | |
| } |
🤖 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/todo/CloudTodoWorkspace.tsx` around lines 1088 - 1118,
Update the custom-status branch in moveItem so itemApi.updateLoopItem persists
sourceStatus through the source_status field rather than the enum status field.
Keep the optimistic source_status update, rollback behavior, and response
reconciliation unchanged, while preserving status as the fixed supported enum
value.
…064933 # Conflicts: # wework/src-tauri/src/feedback.rs # wework/src-tauri/src/lib.rs # wework/src/components/layout/DesktopWorkbenchMain.tsx # wework/src/features/feedback/TaskFeedbackDialog.tsx # wework/src/i18n/locales/en/common.json # wework/src/i18n/locales/zh-CN/common.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/scripts/release-mac-app.sh`:
- Line 591: Update the release flow around prepare:dws to handle
MACOS_BUILD_TARGET=universal-apple-darwin before invoking prepare:dws: prepare
both supported concrete targets, aarch64-apple-darwin and x86_64-apple-darwin,
while preserving the existing single-target behavior for concrete
MACOS_BUILD_TARGET values.
🪄 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: 716aa547-d6ce-41ce-b386-395fd4880ce8
📒 Files selected for processing (3)
.github/workflows/wework-app.yml.github/workflows/wework-e2e.ymlwework/scripts/release-mac-app.sh
| fi | ||
| TAURI_BUILD_ARGS+=(--config "$config_override") | ||
| WEWORK_CODEX_TARGET="${MACOS_BUILD_TARGET:-}" pnpm run prepare:codex | ||
| WEWORK_DWS_TARGET="${MACOS_BUILD_TARGET:-}" pnpm run prepare:dws |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Handle the universal macOS target before preparing DWS.
MACOS_BUILD_TARGET defaults to universal-apple-darwin, but prepare:dws only supports concrete targets such as aarch64-apple-darwin and x86_64-apple-darwin. The default release therefore fails with Unsupported DWS target before Tauri builds. (raw.githubusercontent.com)
Prepare both concrete DWS binaries for universal releases, or add equivalent universal-target handling to prepare-dws-binary.mjs.
Proposed fix
-WEWORK_DWS_TARGET="${MACOS_BUILD_TARGET:-}" pnpm run prepare:dws
+if [ "$MACOS_BUILD_TARGET" = "universal-apple-darwin" ]; then
+ WEWORK_DWS_TARGET=aarch64-apple-darwin pnpm run prepare:dws
+ WEWORK_DWS_TARGET=x86_64-apple-darwin pnpm run prepare:dws
+else
+ WEWORK_DWS_TARGET="${MACOS_BUILD_TARGET:-}" pnpm run prepare:dws
+fi📝 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.
| WEWORK_DWS_TARGET="${MACOS_BUILD_TARGET:-}" pnpm run prepare:dws | |
| if [ "$MACOS_BUILD_TARGET" = "universal-apple-darwin" ]; then | |
| WEWORK_DWS_TARGET=aarch64-apple-darwin pnpm run prepare:dws | |
| WEWORK_DWS_TARGET=x86_64-apple-darwin pnpm run prepare:dws | |
| else | |
| WEWORK_DWS_TARGET="${MACOS_BUILD_TARGET:-}" pnpm run prepare:dws | |
| fi |
🤖 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/scripts/release-mac-app.sh` at line 591, Update the release flow
around prepare:dws to handle MACOS_BUILD_TARGET=universal-apple-darwin before
invoking prepare:dws: prepare both supported concrete targets,
aarch64-apple-darwin and x86_64-apple-darwin, while preserving the existing
single-target behavior for concrete MACOS_BUILD_TARGET values.
Source: MCP tools
2 participants
Summary by CodeRabbit