支持看板公开、私有 - #2265
Conversation
📝 WalkthroughWalkthroughThe PR adds public/private cloud-project visibility, caller-specific project and loop-item capabilities, provider-aware GitLab task attachments, MCP and IPC attachment APIs, projects-home and task-search workspace flows, creator metadata propagation, and WeWork account switching. ChangesCloud access and workspace experience
Provider-backed task attachments
Authorization account switching
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Visitor
participant CloudProjectAPI
participant CloudProjectService
participant LoopItemService
Visitor->>CloudProjectAPI: Request public project and loop items
CloudProjectAPI->>CloudProjectService: Resolve project access
CloudProjectService-->>CloudProjectAPI: RestrictedAnalyst access
CloudProjectAPI->>LoopItemService: Build capability-aware responses
LoopItemService-->>CloudProjectAPI: Own items editable, other details hidden
CloudProjectAPI-->>Visitor: Project and loop-item responses
sequenceDiagram
participant MCPClient
participant MCPServer
participant TaskRuntime
participant IssueProvider
MCPClient->>MCPServer: Upload task attachment
MCPServer->>TaskRuntime: Add provider-aware attachment
TaskRuntime->>IssueProvider: Upload and update GitLab manifest
IssueProvider-->>TaskRuntime: Attachment record
TaskRuntime-->>MCPServer: Attachment result
MCPServer-->>MCPClient: Upload response
🚥 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
executor/src/local/app_ipc.rs (1)
964-1005: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the new attachment handlers with an updated payload.
wework/src/api/local/localDelivery.tsstill sendslistLoopItemAttachments(),removeLoopItemAttachment()viaattachments.delete, andaccessLoopItemAttachment()withoutproject_id, while the Rust handler is readingrequired_task_string(¶ms, "project_id"). Update both sides so existing UI calls don’t receivebad_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 `@executor/src/local/app_ipc.rs` around lines 964 - 1005, Update the attachment IPC contract across the Rust handlers and the TypeScript local delivery methods: ensure listLoopItemAttachments, removeLoopItemAttachment, and accessLoopItemAttachment provide the project_id expected by the attachments.list, attachments.delete, and attachments.access branches, while preserving the existing item and attachment identifiers. Align payload field names and forwarding consistently so these UI calls no longer fail required_task_string validation.backend/app/api/endpoints/deliveries.py (1)
133-159: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winDuplicate access resolution — same root cause as
cloud_projects.py's_project_response.
find_cloud_context(called at line 139) already resolves the caller's project role viarequire_cloud_project_role, then discards it. This handler then recomputes it viacloud_project_service.access(...)to build theaccess_role/project dict — an avoidable extra DB round-trip, and near-duplicate of the dict-shaping logic incloud_projects.py::_project_response. See consolidated comment.🤖 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 133 - 159, Update find_runtime_task_cloud_context to retain and reuse the project role returned by loop_item_service.find_cloud_context instead of calling cloud_project_service.access again. Use that resolved role when constructing the project response, preserving the existing access_role value while eliminating the duplicate database lookup.backend/app/services/cloud_projects/service.py (1)
135-154: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winCritical: restrict provider credential retrieval to Developer+ memberships.
get_provider_credential()now acceptsBaseRole.RestrictedAnalyst, and public non-members are assigned that role byrequire_cloud_project_role. Since this endpoint returns the decrypted provider token stored inprovider_configand the frontend calls it while listing cloud projects, any authenticated visitor to a public external project can retrieve that credential. RequireBaseRole.Developeror a member-only role, decoupled from public-project visibility.🤖 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/cloud_projects/service.py` around lines 135 - 154, Update get_provider_credential to require BaseRole.Developer or another member-only role instead of BaseRole.RestrictedAnalyst when calling require_cloud_project_role, ensuring public non-members cannot retrieve decrypted provider credentials while preserving the existing token decryption and error handling.wework/src/features/todo/TodoEditor.tsx (1)
1034-1044: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSave button isn't gated by
item.can_edit.A
Reporterrole member hascan_view_detail=true, can_edit=falseper the backend (_item_permissions), so they can open this editor but nothing here (fields or the Save button) reflects that they can't actually persist changes — the save only fails after the round trip, surfaced as a generic error. Gating the Save button (as already done for drag/open on the board cards) would give faster, clearer feedback.♻️ Proposed fix
- {dirty && ( + {dirty && item?.can_edit !== false && ( <button type="button" data-testid="cloud-todo-save"🤖 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 1034 - 1044, Update the Save button rendering and disabled state in TodoEditor around saveDetails so it is gated by item.can_edit, preventing Reporter users from attempting to persist changes when editing is not permitted. Preserve the existing dirty, title validation, and saving conditions for users who can edit.wework/src/api/hybrid/cloudProjectSpaceApi.ts (1)
47-84: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire membership before exposing provider credentials.
get_cloud_project_provider_credential()only authenticatescurrent_user, thencloud_project_service.get_provider_credential()allowsRestrictedAnalyst. WithlistCloudProjects()now returning public external projects to non-members, this path lets a public visitor decrypt and configure the real GitHub/GitLab token viaexternalIssueApi.configureProject(), bypassing the localrequireTaskPermissionguard. Require at leastReportermembership for provider 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 `@wework/src/api/hybrid/cloudProjectSpaceApi.ts` around lines 47 - 84, Require at least Reporter project membership before retrieving provider credentials in the listCloudProjects flow. Update the credential-loading path around storeApi.getCloudProjectProviderCredential and the underlying get_cloud_project_provider_credential authorization so public non-members cannot decrypt or pass tokens to externalIssueApi.configureProject; preserve credential configuration for authorized project members.
🧹 Nitpick comments (5)
executor/src/task_runtime/issue_provider.rs (1)
477-510: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid buffering the attachment twice, and validate before decoding.
Part::bytes(bytes.clone())holds a second full copy of the payload for the whole upload. Hashing/sizing first lets the original buffer move into the part. Thedisplay_namecheck is also cheaper before the base64 decode.♻️ Proposed refactor
+ if input.display_name.trim().is_empty() { + return Err(invalid("attachment display_name is required")); + } let bytes = STANDARD .decode(input.base64.as_bytes()) .map_err(|error| invalid(format!("attachment base64 is invalid: {error}")))?; - if input.display_name.trim().is_empty() { - return Err(invalid("attachment display_name is required")); - } + let size_bytes = bytes.len() as i64; + let sha256 = sha256_hex(&bytes); let repository = encode_path_segment(&config.repository); let url = format!("{}/projects/{repository}/uploads", config.api_base); - let mut part = Part::bytes(bytes.clone()).file_name(input.display_name.clone()); + let mut part = Part::bytes(bytes).file_name(input.display_name.clone());Then use
size_bytes/sha256when building the record.🤖 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/issue_provider.rs` around lines 477 - 510, In the attachment upload flow, validate input.display_name before decoding the base64 payload, then compute the payload size and SHA-256 from the decoded bytes before constructing the multipart request. Update Part::bytes to consume the original bytes without cloning, and use the precomputed size and hash when building GitlabAttachmentRecord.executor/src/task_runtime/router.rs (2)
415-441: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
item_idis ignored on the local/GitHub branches.
task_attachment_pathanddelete_task_attachmentnow takeitem_idbut pass onlyattachment_idto the local store, so a caller supplying a mismatched item can still read or delete another task's attachment. The new signature reads as if ownership were checked. Either scope the local-store lookups byitem_idor document that it is GitLab-routing-only.🤖 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/router.rs` around lines 415 - 441, The local and GitHub branches of the attachment operations do not validate item ownership. Update the local-store calls in the task attachment path and delete flow to scope lookups/deletion by item_id, ensuring mismatched item and attachment IDs cannot access another task’s attachment; preserve the existing GitLab provider behavior.
1124-1239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the rollback and corrupted-manifest paths.
The happy path is well covered. The compensating logic in
upload_attachment(delete the blob when the manifest PUT fails) anddelete_attachment(restore the manifest when the remote delete fails) is the part most likely to silently regress, and a description whose manifest block is present but undecodable is the failure mode flagged inissue_provider.rs. A handler that returns 500 on the second PUT would exercise both.🤖 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/router.rs` around lines 1124 - 1239, Extend stores_gitlab_task_attachments_in_project_uploads with failure-path coverage using a handler that returns HTTP 500 on the second manifest PUT: verify upload_attachment deletes the uploaded blob when manifest persistence fails, verify delete_attachment restores the manifest when remote deletion fails, and add a corrupted-but-present manifest case that returns the expected error without silently losing attachment data.backend/app/services/loop_items/service.py (1)
356-357: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant double authorization checks — follow the
restore()pattern.
add_attachment,delete_attachment,update,delete, andbind_taskall callself.get(...)(which performs a view-mode_require_item_accesscheck) and then immediately call_require_item_access(..., edit=True)again — eachrequire_cloud_project_rolecall is a project+membership DB round-trip.delete_attachmentcompounds this further via_get_attachment's ownself.get()call, resulting in three checks for one delete.restore()(480-481) already shows the efficient alternative: fetch the row via_get_item_rowdirectly and perform a single_require_item_access(..., edit=True)call.♻️ Suggested pattern (mirrors `restore()`)
def add_attachment(self, db, item_id, user_id, ...): - item = self.get(db, item_id, user_id) - self._require_item_access(db, item, user_id, edit=True) + item = self._get_item_row(db, item_id) + self._require_item_access(db, item, user_id, edit=True)Also applies to: 408-415, 432-433, 469-470, 548-549
🤖 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 356 - 357, Update add_attachment, delete_attachment, update, delete, and bind_task to fetch the item through _get_item_row instead of self.get, then perform only the existing _require_item_access(..., edit=True) check. In delete_attachment, avoid the nested self.get performed by _get_attachment as well, following restore()’s single-check pattern while preserving the current attachment behavior.backend/tests/api/test_cloud_projects_api.py (1)
126-206: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage for public-visitor access boundaries. Once
get_provider_credential's required role is restored to a higher tier (seeservices/cloud_projects/service.py), consider adding a regression test asserting aRestrictedAnalyst/public-visitor caller cannot fetch the provider credential for a public external project. Happy to draft that test if useful.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/api/test_cloud_projects_api.py` around lines 126 - 206, The existing public-visitor coverage should also verify provider-credential access boundaries. Extend test_public_project_visitors_only_access_their_own_todo_details to create or reference a public external project, request its provider credential as the RestrictedAnalyst visitor, and assert the request is rejected after get_provider_credential restores the higher required role.
🤖 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/cloud_projects.py`:
- Around line 40-51: Eliminate duplicate CloudProjectAccess lookups: update
_project_response and the create/get/update service flows in cloud_projects.py
to propagate and reuse the access already resolved, and batch-resolve roles for
list_cloud_projects instead of per-project require_cloud_project_role calls. In
deliveries.py, update loop_item_service.find_cloud_context to return its
resolved CloudProjectAccess and reuse it at the endpoint rather than calling
cloud_project_service.access again; apply these changes at
backend/app/api/endpoints/cloud_projects.py lines 40-51 and
backend/app/api/endpoints/deliveries.py lines 133-159.
In `@executor/src/task_runtime/issue_provider.rs`:
- Around line 1072-1078: Update the GitlabUpload deserialization struct so
full_path and markdown are optional or have serde defaults, while keeping url
required for deriving the secret. Ensure upload_attachment can successfully
parse responses that omit those fields and proceed to its existing rollback
handling on later failures.
- Around line 666-683: The delete_gitlab_upload method currently builds an
incomplete GitLab upload-deletion URL. Update it to include the stored
display/original filename after the encoded secret, or use the supported
upload_id deletion endpoint, while preserving the existing repository and
authorization handling.
- Around line 1334-1349: Update split_gitlab_attachment_manifest so a present
but malformed attachment block is removed from the returned description instead
of returning the original text with an empty record list. Preserve the
block-removal behavior for valid manifests, and surface the decode/parse failure
through the existing error-reporting mechanism rather than treating corruption
as no attachments; ensure subsequent render_gitlab_attachment_manifest calls
cannot round-trip the damaged marker text.
In `@executor/src/task_runtime/mcp.rs`:
- Around line 474-528: Restrict binary_input_from_path and
copy_attachment_if_requested to the task/project workspace root, resolving and
canonicalizing both requested paths and rejecting any path that escapes the
root, including symlink traversal. Preserve the existing read/copy flow only for
confined paths, and enforce a size limit before fs::read so oversized
attachments are rejected without excessive buffering or base64 expansion.
In `@frontend/src/app/auth/wework/authorize/page.tsx`:
- Around line 76-85: Update handleSwitchAccount to set the submission state to
'submitting' before calling removeToken, storing the redirect target, or
navigating with router.replace, so repeated account-switch submissions are
prevented.
In `@wework/src/features/todo/CloudTodoWorkspace.tsx`:
- Around line 1251-1279: Restrict the manage-view render boundary in
CloudTodoWorkspace so CloudProjectManageView is shown only when
selectedProject.access_role is Owner or Maintainer, preventing navigation from
CloudProjectsHome from bypassing the header-tab gate. Update the
selectedProject.access_role fallback used by the manage-tab condition from the
privileged Owner role to a restrictive non-manageable role.
- Around line 1200-1215: The CloudProjectsHome onSelectItem wiring opens
restricted items without checking can_view_detail. Update the onSelectItem
handler in CloudTodoWorkspace so it only calls setSelectedItem when
item.can_view_detail !== false, matching the guards used by CloudMyWorkView and
the board card click path.
In `@wework/src/features/todo/TodoEditor.tsx`:
- Around line 385-390: Update the creator selection logic near the creator
render in TodoEditor so the current-user name branch is used only when both user
IDs are truthy, preventing the 0-as-unset sentinel from matching. Preserve
member-name lookup for valid creator IDs and allow the existing render fallback
(`creator ?? (...)`) to produce `#0` or — when no creator label exists.
---
Outside diff comments:
In `@backend/app/api/endpoints/deliveries.py`:
- Around line 133-159: Update find_runtime_task_cloud_context to retain and
reuse the project role returned by loop_item_service.find_cloud_context instead
of calling cloud_project_service.access again. Use that resolved role when
constructing the project response, preserving the existing access_role value
while eliminating the duplicate database lookup.
In `@backend/app/services/cloud_projects/service.py`:
- Around line 135-154: Update get_provider_credential to require
BaseRole.Developer or another member-only role instead of
BaseRole.RestrictedAnalyst when calling require_cloud_project_role, ensuring
public non-members cannot retrieve decrypted provider credentials while
preserving the existing token decryption and error handling.
In `@executor/src/local/app_ipc.rs`:
- Around line 964-1005: Update the attachment IPC contract across the Rust
handlers and the TypeScript local delivery methods: ensure
listLoopItemAttachments, removeLoopItemAttachment, and accessLoopItemAttachment
provide the project_id expected by the attachments.list, attachments.delete, and
attachments.access branches, while preserving the existing item and attachment
identifiers. Align payload field names and forwarding consistently so these UI
calls no longer fail required_task_string validation.
In `@wework/src/api/hybrid/cloudProjectSpaceApi.ts`:
- Around line 47-84: Require at least Reporter project membership before
retrieving provider credentials in the listCloudProjects flow. Update the
credential-loading path around storeApi.getCloudProjectProviderCredential and
the underlying get_cloud_project_provider_credential authorization so public
non-members cannot decrypt or pass tokens to externalIssueApi.configureProject;
preserve credential configuration for authorized project members.
In `@wework/src/features/todo/TodoEditor.tsx`:
- Around line 1034-1044: Update the Save button rendering and disabled state in
TodoEditor around saveDetails so it is gated by item.can_edit, preventing
Reporter users from attempting to persist changes when editing is not permitted.
Preserve the existing dirty, title validation, and saving conditions for users
who can edit.
---
Nitpick comments:
In `@backend/app/services/loop_items/service.py`:
- Around line 356-357: Update add_attachment, delete_attachment, update, delete,
and bind_task to fetch the item through _get_item_row instead of self.get, then
perform only the existing _require_item_access(..., edit=True) check. In
delete_attachment, avoid the nested self.get performed by _get_attachment as
well, following restore()’s single-check pattern while preserving the current
attachment behavior.
In `@backend/tests/api/test_cloud_projects_api.py`:
- Around line 126-206: The existing public-visitor coverage should also verify
provider-credential access boundaries. Extend
test_public_project_visitors_only_access_their_own_todo_details to create or
reference a public external project, request its provider credential as the
RestrictedAnalyst visitor, and assert the request is rejected after
get_provider_credential restores the higher required role.
In `@executor/src/task_runtime/issue_provider.rs`:
- Around line 477-510: In the attachment upload flow, validate
input.display_name before decoding the base64 payload, then compute the payload
size and SHA-256 from the decoded bytes before constructing the multipart
request. Update Part::bytes to consume the original bytes without cloning, and
use the precomputed size and hash when building GitlabAttachmentRecord.
In `@executor/src/task_runtime/router.rs`:
- Around line 415-441: The local and GitHub branches of the attachment
operations do not validate item ownership. Update the local-store calls in the
task attachment path and delete flow to scope lookups/deletion by item_id,
ensuring mismatched item and attachment IDs cannot access another task’s
attachment; preserve the existing GitLab provider behavior.
- Around line 1124-1239: Extend
stores_gitlab_task_attachments_in_project_uploads with failure-path coverage
using a handler that returns HTTP 500 on the second manifest PUT: verify
upload_attachment deletes the uploaded blob when manifest persistence fails,
verify delete_attachment restores the manifest when remote deletion fails, and
add a corrupted-but-present manifest case that returns the expected error
without silently losing attachment data.
🪄 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: c7595b44-c6cd-409e-b6f4-3f2a408c3b0e
⛔ Files ignored due to path filters (1)
executor/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
backend/app/api/endpoints/cloud_projects.pybackend/app/api/endpoints/deliveries.pybackend/app/models/delivery.pybackend/app/schemas/cloud_project.pybackend/app/schemas/delivery.pybackend/app/services/cloud_projects/access.pybackend/app/services/cloud_projects/service.pybackend/app/services/delivery/access.pybackend/app/services/loop_items/service.pybackend/tests/api/test_cloud_projects_api.pyexecutor/Cargo.tomlexecutor/src/local/app_ipc.rsexecutor/src/task_runtime/issue_provider.rsexecutor/src/task_runtime/mcp.rsexecutor/src/task_runtime/model.rsexecutor/src/task_runtime/router.rsexecutor/src/task_runtime/store.rsexecutor/tests/local_app_ipc_contract.rsexecutor/tests/local_task_mcp_contract.rsfrontend/src/app/auth/wework/authorize/page.tsxfrontend/src/i18n/locales/en/common.jsonfrontend/src/i18n/locales/zh-CN/common.jsonwework/src/api/deliveries.tswework/src/api/hybrid/cloudProjectSpaceApi.test.tswework/src/api/hybrid/cloudProjectSpaceApi.tswework/src/api/local/localDelivery.test.tswework/src/api/local/localDelivery.tswework/src/features/todo/CloudProjectManageView.tsxwework/src/features/todo/CloudProjectsHome.tsxwework/src/features/todo/CloudTodoWorkspace.test.tsxwework/src/features/todo/CloudTodoWorkspace.tsxwework/src/features/todo/TodoEditor.tsxwework/src/features/todo/todoShared.tswework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.json
| async fn delete_gitlab_upload( | ||
| &self, | ||
| config: &ProviderConfig, | ||
| provider: TaskProviderKind, | ||
| secret: &str, | ||
| ) -> Result<(), TaskRuntimeError> { | ||
| let repository = encode_path_segment(&config.repository); | ||
| let secret = encode_path_segment(secret); | ||
| let url = format!("{}/projects/{repository}/uploads/{secret}", config.api_base); | ||
| self.send( | ||
| provider, | ||
| "delete_attachment", | ||
| &config.repository, | ||
| config.authorize(self.client.delete(url), provider), | ||
| ) | ||
| .await?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
GitLab REST API delete project upload by secret and filename endpoint
💡 Result:
Yes, GitLab provides a REST API endpoint to delete a project upload using its secret and filename [1][2]. The endpoint is: DELETE /projects/:id/uploads/:secret/:filename Parameters: - id: The integer ID or URL-encoded path of the project. - secret: The 32-character secret string associated with the upload. - filename: The filename of the upload [1][2]. If the deletion is successful, the API returns a 204 status code with no response body [1][2]. Example request using cURL: curl --request DELETE \ --header "PRIVATE-TOKEN: <your_access_token>" \ --url "https://gitlab.example.com/api/v4/projects/<project_id>/uploads//" [1][2]
Citations:
- 1: https://docs.gitlab.com/api/project_markdown_uploads/
- 2: https://gitlab.com/gitlab-org/gitlab/-/blob/cca85fba528c912901e50499be46734c6cc134a4/doc/api/project_markdown_uploads.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Matching files:\n'
fd -a 'issue_provider\.rs$|.*task_runtime.*' . | sed 's#^\./##' | head -50
printf '\nRelevant source section:\n'
sed -n '620,710p' executor/src/task_runtime/issue_provider.rs
printf '\nRecord/upload field usage:\n'
rg -n "upload_id|secret|delete_gitlab_upload|update_gitlab_attachment_manifest|attachment" executor/src -SRepository: wecode-ai/Wegent
Length of output: 50372
Use the full GitLab upload-deletion endpoint shape.
GitLab’s REST upload deletion endpoint requires DELETE /projects/:id/uploads/:secret/:filename; DELETE /projects/:id/uploads/:secret can return 404 and be mapped to TaskRuntimeError::TaskNotFound during attachment cleanup. Store and use the display/original filename, or use the stored upload_id deletion path if that endpoint is available.
🤖 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/issue_provider.rs` around lines 666 - 683, The
delete_gitlab_upload method currently builds an incomplete GitLab
upload-deletion URL. Update it to include the stored display/original filename
after the encoded secret, or use the supported upload_id deletion endpoint,
while preserving the existing repository and authorization handling.
| #[derive(Deserialize)] | ||
| struct GitlabUpload { | ||
| id: Option<i64>, | ||
| url: String, | ||
| full_path: String, | ||
| markdown: String, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Deserialization of full_path/markdown is a hard requirement on the upload response.
Only url is actually needed to derive the secret. If a GitLab version omits full_path or markdown, .json::<GitlabUpload>() fails after the blob is already stored, and the rollback in upload_attachment never runs because the secret was never parsed — leaving an orphaned upload. Defaulting these keeps the parse resilient.
🛡️ Proposed fix
#[derive(Deserialize)]
struct GitlabUpload {
id: Option<i64>,
url: String,
- full_path: String,
- markdown: String,
+ #[serde(default)]
+ full_path: String,
+ #[serde(default)]
+ markdown: String,
}📝 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.
| #[derive(Deserialize)] | |
| struct GitlabUpload { | |
| id: Option<i64>, | |
| url: String, | |
| full_path: String, | |
| markdown: String, | |
| } | |
| #[derive(Deserialize)] | |
| struct GitlabUpload { | |
| id: Option<i64>, | |
| url: String, | |
| #[serde(default)] | |
| full_path: String, | |
| #[serde(default)] | |
| markdown: String, | |
| } |
🤖 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/issue_provider.rs` around lines 1072 - 1078, Update
the GitlabUpload deserialization struct so full_path and markdown are optional
or have serde defaults, while keeping url required for deriving the secret.
Ensure upload_attachment can successfully parse responses that omit those fields
and proceed to its existing rollback handling on later failures.
| fn split_gitlab_attachment_manifest(description: &str) -> (String, Vec<GitlabAttachmentRecord>) { | ||
| let Some(start) = description.find(ATTACHMENT_BLOCK_START) else { | ||
| return (description.to_owned(), Vec::new()); | ||
| }; | ||
| let block_tail = &description[start + ATTACHMENT_BLOCK_START.len()..]; | ||
| let Some(relative_end) = block_tail.find(ATTACHMENT_BLOCK_END) else { | ||
| return (description.to_owned(), Vec::new()); | ||
| }; | ||
| let end = start + ATTACHMENT_BLOCK_START.len() + relative_end + ATTACHMENT_BLOCK_END.len(); | ||
| let block = &description[start..end]; | ||
| let Some(attachments) = attachment_manifest_payload(block) | ||
| .and_then(|encoded| URL_SAFE_NO_PAD.decode(encoded.as_bytes()).ok()) | ||
| .and_then(|bytes| serde_json::from_slice(&bytes).ok()) | ||
| else { | ||
| return (description.to_owned(), Vec::new()); | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Manifest parse failure silently orphans every attachment.
When the block markers are present but the payload doesn't decode — e.g. a human edited the issue description in GitLab and clipped the comment — this returns the original description (block text included) plus an empty record list. The next update then calls render_gitlab_attachment_manifest, whose internal split fails the same way, so the stale marker text stays embedded in the description while the record list is written as empty: every previously uploaded file is dropped from the manifest and can no longer be listed, downloaded, or deleted.
Consider stripping the block on the failure path too (so at least the description is clean), and surfacing the corruption rather than treating it as "no attachments".
🛠️ Sketch
- let Some(attachments) = attachment_manifest_payload(block)
- .and_then(|encoded| URL_SAFE_NO_PAD.decode(encoded.as_bytes()).ok())
- .and_then(|bytes| serde_json::from_slice(&bytes).ok())
- else {
- return (description.to_owned(), Vec::new());
- };
+ let attachments = attachment_manifest_payload(block)
+ .and_then(|encoded| URL_SAFE_NO_PAD.decode(encoded.as_bytes()).ok())
+ .and_then(|bytes| serde_json::from_slice(&bytes).ok())
+ .unwrap_or_default();with the block still removed from the visible text below, so a damaged manifest never round-trips back into the description.
📝 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.
| fn split_gitlab_attachment_manifest(description: &str) -> (String, Vec<GitlabAttachmentRecord>) { | |
| let Some(start) = description.find(ATTACHMENT_BLOCK_START) else { | |
| return (description.to_owned(), Vec::new()); | |
| }; | |
| let block_tail = &description[start + ATTACHMENT_BLOCK_START.len()..]; | |
| let Some(relative_end) = block_tail.find(ATTACHMENT_BLOCK_END) else { | |
| return (description.to_owned(), Vec::new()); | |
| }; | |
| let end = start + ATTACHMENT_BLOCK_START.len() + relative_end + ATTACHMENT_BLOCK_END.len(); | |
| let block = &description[start..end]; | |
| let Some(attachments) = attachment_manifest_payload(block) | |
| .and_then(|encoded| URL_SAFE_NO_PAD.decode(encoded.as_bytes()).ok()) | |
| .and_then(|bytes| serde_json::from_slice(&bytes).ok()) | |
| else { | |
| return (description.to_owned(), Vec::new()); | |
| }; | |
| fn split_gitlab_attachment_manifest(description: &str) -> (String, Vec<GitlabAttachmentRecord>) { | |
| let Some(start) = description.find(ATTACHMENT_BLOCK_START) else { | |
| return (description.to_owned(), Vec::new()); | |
| }; | |
| let block_tail = &description[start + ATTACHMENT_BLOCK_START.len()..]; | |
| let Some(relative_end) = block_tail.find(ATTACHMENT_BLOCK_END) else { | |
| return (description.to_owned(), Vec::new()); | |
| }; | |
| let end = start + ATTACHMENT_BLOCK_START.len() + relative_end + ATTACHMENT_BLOCK_END.len(); | |
| let block = &description[start..end]; | |
| let attachments = attachment_manifest_payload(block) | |
| .and_then(|encoded| URL_SAFE_NO_PAD.decode(encoded.as_bytes()).ok()) | |
| .and_then(|bytes| serde_json::from_slice(&bytes).ok()) | |
| .unwrap_or_default(); |
🤖 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/issue_provider.rs` around lines 1334 - 1349, Update
split_gitlab_attachment_manifest so a present but malformed attachment block is
removed from the returned description instead of returning the original text
with an empty record list. Preserve the block-removal behavior for valid
manifests, and surface the decode/parse failure through the existing
error-reporting mechanism rather than treating corruption as no attachments;
ensure subsequent render_gitlab_attachment_manifest calls cannot round-trip the
damaged marker text.
| fn binary_input_from_path( | ||
| arguments: &Value, | ||
| file_path: &str, | ||
| ) -> Result<BinaryInput, super::TaskRuntimeError> { | ||
| let bytes = fs::read(file_path).map_err(|error| { | ||
| super::TaskRuntimeError::Invalid(format!("cannot read attachment file: {error}")) | ||
| })?; | ||
| let display_name = arguments | ||
| .get("display_name") | ||
| .and_then(Value::as_str) | ||
| .filter(|value| !value.trim().is_empty()) | ||
| .map(ToOwned::to_owned) | ||
| .or_else(|| { | ||
| Path::new(file_path) | ||
| .file_name() | ||
| .and_then(|value| value.to_str()) | ||
| .map(ToOwned::to_owned) | ||
| }) | ||
| .ok_or_else(|| { | ||
| super::TaskRuntimeError::Invalid("attachment display_name is required".to_owned()) | ||
| })?; | ||
| Ok(BinaryInput { | ||
| display_name, | ||
| content_type: arguments | ||
| .get("content_type") | ||
| .and_then(Value::as_str) | ||
| .filter(|value| !value.trim().is_empty()) | ||
| .map(ToOwned::to_owned), | ||
| base64: STANDARD.encode(bytes), | ||
| }) | ||
| } | ||
|
|
||
| fn copy_attachment_if_requested( | ||
| arguments: &Value, | ||
| source_path: &str, | ||
| ) -> Result<String, super::TaskRuntimeError> { | ||
| let Some(output_path) = arguments | ||
| .get("output_path") | ||
| .and_then(Value::as_str) | ||
| .filter(|value| !value.trim().is_empty()) | ||
| else { | ||
| return Ok(source_path.to_owned()); | ||
| }; | ||
| if let Some(parent) = Path::new(output_path).parent() { | ||
| fs::create_dir_all(parent).map_err(|error| { | ||
| super::TaskRuntimeError::Invalid(format!( | ||
| "cannot create attachment output directory: {error}" | ||
| )) | ||
| })?; | ||
| } | ||
| fs::copy(source_path, output_path).map_err(|error| { | ||
| super::TaskRuntimeError::Invalid(format!("cannot copy attachment file: {error}")) | ||
| })?; | ||
| Ok(output_path.to_owned()) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Unbounded, unsandboxed filesystem access from an agent-controlled tool.
file_path and output_path come straight from the model's tool arguments with no confinement: binary_input_from_path will read any file the executor process can read (including credential and session files outside the workspace) and ship it to a GitLab project, and copy_attachment_if_requested will create_dir_all + copy to any writable location. Confining both to the task/project workspace root — and rejecting paths that escape it after canonicalization — closes this without changing the intended flow.
Also worth a size ceiling on fs::read: the bytes are held in memory and then base64-expanded ~4/3, so a large file is buffered several times over before the upload even starts.
As per coding guidelines: "Keep the local runtime and desktop UI isolated from a developer's normal Codex home; do not copy or log 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/mcp.rs` around lines 474 - 528, Restrict
binary_input_from_path and copy_attachment_if_requested to the task/project
workspace root, resolving and canonicalizing both requested paths and rejecting
any path that escapes the root, including symlink traversal. Preserve the
existing read/copy flow only for confined paths, and enforce a size limit before
fs::read so oversized attachments are rejected without excessive buffering or
base64 expansion.
Source: Coding guidelines
| function handleSwitchAccount() { | ||
| // Drop the current Web session and bounce through the login page so the | ||
| // authorization can be granted by a different account. The login page | ||
| // reads POST_LOGIN_REDIRECT_KEY and returns here after a successful login. | ||
| removeToken() | ||
| const redirectTarget = currentRedirectTarget() | ||
| sessionStorage.setItem(POST_LOGIN_REDIRECT_KEY, redirectTarget) | ||
| router.replace(`${paths.auth.login.getHref()}?redirect=${encodeURIComponent(redirectTarget)}`) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate target file:"
fd -a 'page\.tsx$' frontend/src/app/auth/wework/authorize || true
echo
echo "Relevant lines and nearby state/imports:"
if [ -f frontend/src/app/auth/wework/authorize/page.tsx ]; then
nl -ba frontend/src/app/auth/wework/authorize/page.tsx | sed -n '1,140p'
fi
echo
echo "Search for related constants/usages:"
rg -n "handleSwitchAccount|SETTLED|submitting|POST_LOGIN_REDIRECT_KEY|removeToken" frontend/src/app/auth/wework/authorize/page.tsx frontend/src -g '*.tsx' -g '*.ts' | head -200 || true
echo
echo "Git diff stat/name-status for context:"
git diff --stat || true
git diff -- frontend/src/app/auth/wework/authorize/page.tsx | sed -n '1,220p' || trueRepository: wecode-ai/Wegent
Length of output: 324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
export LC_ALL=C
TARGET='frontend/src/app/auth/wework/authorize/page.tsx'
echo "Relevant page lines:"
awk '{printf "%6d %s\n", NR, $0}' "$TARGET" | sed -n '1,140p'
echo
echo "Search for state handling in page:"
grep -nE "useState|setState|submitting|handleSwitchAccount|disabled|button|auth/login|POST_LOGIN_REDIRECT_KEY|removeToken" "$TARGET" || trueRepository: wecode-ai/Wegent
Length of output: 7176
Prevent repeated account-switch submissions.
handleSwitchAccount navigates without setting state to 'submitting', so the next button remains enabled until the nav occurs. Set the state before clearing credentials or redirecting.
Proposed fix
function handleSwitchAccount() {
+ setState('submitting')
removeToken()📝 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.
| function handleSwitchAccount() { | |
| // Drop the current Web session and bounce through the login page so the | |
| // authorization can be granted by a different account. The login page | |
| // reads POST_LOGIN_REDIRECT_KEY and returns here after a successful login. | |
| removeToken() | |
| const redirectTarget = currentRedirectTarget() | |
| sessionStorage.setItem(POST_LOGIN_REDIRECT_KEY, redirectTarget) | |
| router.replace(`${paths.auth.login.getHref()}?redirect=${encodeURIComponent(redirectTarget)}`) | |
| } | |
| function handleSwitchAccount() { | |
| setState('submitting') | |
| // Drop the current Web session and bounce through the login page so the | |
| // authorization can be granted by a different account. The login page | |
| // reads POST_LOGIN_REDIRECT_KEY and returns here after a successful login. | |
| removeToken() | |
| const redirectTarget = currentRedirectTarget() | |
| sessionStorage.setItem(POST_LOGIN_REDIRECT_KEY, redirectTarget) | |
| router.replace(`${paths.auth.login.getHref()}?redirect=${encodeURIComponent(redirectTarget)}`) | |
| } |
🤖 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 `@frontend/src/app/auth/wework/authorize/page.tsx` around lines 76 - 85, Update
handleSwitchAccount to set the submission state to 'submitting' before calling
removeToken, storing the redirect target, or navigating with router.replace, so
repeated account-switch submissions are prevented.
| {selectedProject.access_role !== 'RestrictedAnalyst' && ( | ||
| <button | ||
| type="button" | ||
| onClick={() => setProjectView('files')} | ||
| className={cn( | ||
| 'rounded-md px-3.5 py-1 text-sm', | ||
| projectView === 'files' | ||
| ? 'bg-background font-medium text-text-primary shadow-sm' | ||
| : 'text-text-secondary hover:text-text-primary' | ||
| )} | ||
| > | ||
| 文件 | ||
| </button> | ||
| )} | ||
| {['Owner', 'Maintainer'].includes(selectedProject.access_role ?? 'Owner') && ( | ||
| <button | ||
| type="button" | ||
| data-testid="cloud-project-manage-view" | ||
| onClick={() => setProjectView('manage')} | ||
| className={cn( | ||
| 'rounded-md px-3.5 py-1 text-sm', | ||
| projectView === 'manage' | ||
| ? 'bg-background font-medium text-text-primary shadow-sm' | ||
| : 'text-text-secondary hover:text-text-primary' | ||
| )} | ||
| > | ||
| 管理 | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Manage view is reachable for any role via the projects-home manage modal, bypassing the header-tab gate.
The "管理" header tab is correctly restricted to Owner/Maintainer (lines 1265-1279), but CloudProjectManageView is rendered (lines 1320-1333) whenever projectView === 'manage', with no independent check. CloudProjectsHome's "全部空间 → 管理" modal calls onManageProject (wired here at lines 1209-1212) for every listed project regardless of role — its ProjectsHomeProject type doesn't even carry access_role, so that path can't be gated upstream. A Reporter/Developer/public visitor can open the manage panel for a project they don't administer this way.
Also note: selectedProject.access_role ?? 'Owner' (line 1265) fails open — if access_role is ever missing, this treats the caller as the most-privileged role instead of the least-privileged one.
Recommend re-checking the role at the render boundary itself (defense-in-depth), and defaulting to a restrictive role on the fallback:
🔒 Proposed fix
- {['Owner', 'Maintainer'].includes(selectedProject.access_role ?? 'Owner') && (
+ {['Owner', 'Maintainer'].includes(selectedProject.access_role ?? 'RestrictedAnalyst') && (- ) : projectView === 'manage' && selectedProjectApi ? (
+ ) : projectView === 'manage' &&
+ selectedProjectApi &&
+ ['Owner', 'Maintainer'].includes(selectedProject.access_role ?? 'RestrictedAnalyst') ? (
<CloudProjectManageViewAlso applies to: 1320-1333
🤖 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 1251 - 1279,
Restrict the manage-view render boundary in CloudTodoWorkspace so
CloudProjectManageView is shown only when selectedProject.access_role is Owner
or Maintainer, preventing navigation from CloudProjectsHome from bypassing the
header-tab gate. Update the selectedProject.access_role fallback used by the
manage-tab condition from the privileged Owner role to a restrictive
non-manageable role.
| const creator = | ||
| item && item.created_by_user_id === editProps?.project?.current_user_id | ||
| ? editProps.project.current_user_name | ||
| : item | ||
| ? memberNameById(projectMembers, item.created_by_user_id) | ||
| : null |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Creator chip renders blank instead of the fallback when current_user_id/created_by_user_id are both 0.
creator can resolve to '' (not null/undefined) whenever editProps.project.current_user_name is empty — which is always true for local (device-only) projects (localProject() hardcodes current_user_id: 0, current_user_name: '') and for any pre-existing external issue without a creator label (created_by_user_id defaults to 0). Since the render uses creator ?? (...) (line 735), ?? doesn't treat '' as missing, so the chip shows no value at all instead of #0/—. Elsewhere in this same PR (localDelivery.ts's ownsTask) the 0-as-unset sentinel is guarded with Boolean(...) before comparing — worth applying the same guard here.
🐛 Proposed fix
const creator =
- item && item.created_by_user_id === editProps?.project?.current_user_id
- ? editProps.project.current_user_name
+ item &&
+ Boolean(editProps?.project?.current_user_id) &&
+ item.created_by_user_id === editProps?.project?.current_user_id
+ ? editProps.project.current_user_name || null
: item
? memberNameById(projectMembers, item.created_by_user_id)
: nullAlso applies to: 727-738
🤖 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 385 - 390, Update the
creator selection logic near the creator render in TodoEditor so the
current-user name branch is used only when both user IDs are truthy, preventing
the 0-as-unset sentinel from matching. Preserve member-name lookup for valid
creator IDs and allow the existing render fallback (`creator ?? (...)`) to
produce `#0` or — when no creator label exists.
… env - drop needless borrows flagged by clippy in task MCP attachment tools - replace Option::is_none_or (stable since 1.82) with map_or to respect the declared 1.77 MSRV - scrub inherited WEGENT_* variables in the unit test binary so tests stay hermetic when spawned from the desktop app
The workbench services test still expected cloud GitLab tasks to route through the local executor, but createCloudProjectSpaceApi now delegates execution to the backend and only uses externalIssueApi to prune stale local credential entries.
- narrow the WEGENT_* scrub to host-state variables that leak machine paths (bundled/managed hooks dirs, executor home) instead of wiping every WEGENT_* variable, so runner configuration like WEGENT_EXTRA_PATHS survives - raise the fallback shell test timeout from 1s to 30s; the 1s budget flakes when the shared test thread pool is saturated by the full suite
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
executor/src/local/app_ipc.rs (1)
988-1028: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate local attachment IPC clients to match the required params.
attachments.list/access/deletenow requireproject_id, andaccess/deletealso requireitem_id, butwework/src/api/local/localDelivery.tsstill sends onlyitem_idfor list,attachment_idfor access, andattachment_idfor delete. These calls returnbad_request: project_id is requiredand break local attachment listing, opening, and deletion.🤖 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 988 - 1028, Update the local attachment IPC client methods in localDelivery.ts to send the required parameters: include project_id and item_id for attachments.list, and include project_id, item_id, and attachment_id for attachments.access and attachments.delete. Preserve the existing attachment operation behavior while aligning each request payload with the IPC handlers.
🧹 Nitpick comments (12)
backend/app/services/loop_items/external_provider.py (2)
288-297: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEach provider call re-decrypts the token and opens a fresh connection.
httpx.requestbuilds and discards a client per call, so a fulllistcan perform up to 100 sequential requests with 100 TLS handshakes, and_repository+_requesteach invoke_config→decrypt_provider_token. Resolve the config once per operation and reuse anhttpx.Client(context-managed) across the pagination loop.Also applies to: 342-349
🤖 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 288 - 297, Refactor the provider operation around _request and the related _repository flow to resolve _config only once per operation, then create one context-managed httpx.Client and reuse it throughout the pagination loop. Replace per-call httpx.request usage with the shared client, while preserving request parameters, pagination behavior, and response handling.
161-189: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
ensure_shadowhas a check-then-insert race on the primary key.Two concurrent requests for the same external item both see
existing is Noneand bothdb.addthe sameid, so one commit fails with anIntegrityErrorsurfaced as a 500. Wrap the insert in a try/except that rolls back and re-reads, or use an upsert.🤖 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 161 - 189, The ensure_shadow method has a check-then-insert race that can surface a duplicate-primary-key IntegrityError. Make the insert/commit path concurrency-safe by catching the commit IntegrityError, rolling back the session, and re-reading LoopItem by item_id to return the concurrently created record; preserve the existing creation path when no conflict occurs.executor/src/task_runtime/store.rs (1)
1251-1254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing
store()helper in the new tests.Both tests re-create the temp dir and store inline instead of using the
store()helper defined at Line 1146. Also consider assertingproject_provider_credentialsis pruned in the retain test — right now onlyremove_external_projectcovers that DELETE.♻️ Suggested change
- let directory = tempfile::tempdir().unwrap(); - let store = LocalTaskStore::open(directory.path().join("tasks.sqlite")).unwrap(); + let (_directory, store) = store();Also applies to: 1290-1293
🤖 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 1251 - 1254, Update both new tests, including removes_backend_external_project_credentials and the test around the second referenced range, to use the existing store() helper instead of creating a temporary directory and opening LocalTaskStore inline. In the retain test, also assert that project_provider_credentials is pruned, covering the retained-project cleanup path rather than relying only on remove_external_project.Source: Coding guidelines
executor/src/task_runtime/mcp.rs (3)
992-1004: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese
call_tooltests depend on ambient environment state.
call_toolreadsWEGENT_TASK_PROJECT_ID,WEGENT_TASK_BACKEND_URL, andWEGENT_TASK_AUTH_TOKENfrom the process env. Since unit tests share a process and run in parallel, any test that sets those vars would push these cases onto the scope-check or backend route and fail them. The env-guard pattern inexecutor/tests/local_app_ipc_contract.rs(theDrop-based restore helper) would make this deterministic.Also applies to: 1051-1054
🤖 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 992 - 1004, Make the call_tool tests searches_todos_by_text_and_structured_filters and the test at the referenced second location deterministic by applying the Drop-based environment restore guard pattern from local_app_ipc_contract.rs. Isolate and restore WEGENT_TASK_PROJECT_ID, WEGENT_TASK_BACKEND_URL, and WEGENT_TASK_AUTH_TOKEN around each test so parallel tests cannot leak ambient environment state into these cases.
371-382: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffBackend
search_todospulls the whole board before filtering.Every search fetches all loop items for the project and filters in memory, so cost grows with board size regardless of
limit. If the backend endpoint accepts query/status/priority parameters, pushing the filters server-side would keep this bounded.Also applies to: 526-541
🤖 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 371 - 382, Update the search_todos request in the task runtime, along with the analogous request referenced at lines 526-541, to pass the search, status, priority, and limit filters as supported query parameters to the loop-items backend endpoint. Preserve the existing authentication, response parsing, and task-return behavior while removing reliance on fetching and filtering the entire board in memory.
353-361: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet an explicit connect timeout before fetching backend/attachment URLs.
reqwest::Client::new()gets a default request timeout, but the connection phase can still block with no limit. A stuck TCP connection for backend calls or the externally supplied attachment URL can stall the MCP tool call/agent turn.🛡️ Suggested change
- let client = reqwest::Client::new(); + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(30)) + .connect_timeout(std::time::Duration::from_secs(10)) + .build() + .map_err(|error| error.to_string())?;Also applies to: 460-467
🤖 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 353 - 361, Update the HTTP client construction in call_backend_tool and the corresponding client used for externally supplied attachment URLs to configure an explicit connection timeout. Preserve the existing request behavior while ensuring both backend and attachment fetch connections cannot block indefinitely.backend/app/services/cloud_projects/service.py (1)
108-120: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider indexing
metadata_json->>'visibility'if thecloud_projectstable grows.Filtering on
CloudProject.metadata_json["visibility"].as_string() == "public"scans/parses JSON per row with no supporting index. Fine at current scale, but worth a functional/GIN index later if project counts grow significantly.🤖 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/cloud_projects/service.py` around lines 108 - 120, Consider adding a database index for the JSON visibility expression used in the CloudProject query, specifically CloudProject.metadata_json["visibility"].as_string(), using the project’s migration/model indexing conventions. Ensure the index supports filtering for the "public" value without changing the existing service query behavior.wework/src/features/todo/TaskSearchPanel.tsx (2)
26-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew panel hardcodes Chinese copy instead of using the i18n wrapper.
GlobalTodoSearch.tsx, added in the same PR, routes all of its strings throughuseTranslation('common'). This component hardcodes every label (placeholder,aria-labels, the option labels at Lines 82-189, and the empty/no-result text at Lines 193-198), so the panel stays untranslated.♻️ Suggested direction
+import { useTranslation } from '`@/hooks/useTranslation`' ... }: TaskSearchPanelProps) { + const { t } = useTranslation('common') const results = searchTasks(items, query, filters, members)then replace the literals with
t('workbench.task_search_*')keys added to bothwework/src/i18n/locales/en/common.jsonandwework/src/i18n/locales/zh-CN/common.json.As per coding guidelines: "Use the local
@/hooks/useTranslationwrapper for new Wework code" and "Add new copy to the appropriate Wework namespace in both the English and Simplified Chinese locale directories".🤖 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/TaskSearchPanel.tsx` around lines 26 - 68, Update TaskSearchPanel and its remaining hardcoded labels, placeholders, aria-labels, option text, and empty/no-result messages to use the local useTranslation('common') wrapper and workbench.task_search_* keys. Add every new key with English and Simplified Chinese values to the corresponding common.json locale files, then replace the literals throughout the component with t(...) calls.Source: Coding guidelines
69-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeven near-identical
<select>blocks could collapse into one helper.Each block differs only in test id, aria-label, current value, options, and the patched filter key. A small local
FilterSelectcomponent (or an options-driven array) would remove most of this block and keep future filters consistent.🤖 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/TaskSearchPanel.tsx` around lines 69 - 190, The filter controls in TaskSearchPanel are duplicated across seven near-identical select blocks. Introduce a small local FilterSelect helper or configuration-driven rendering for status, priority, tag, assignee, creator, due, and children, while preserving each control’s test id, aria-label, value, options, and filter-key-specific onChange behavior.backend/tests/api/test_cloud_projects_api.py (1)
456-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe no-token-exposure assertion is effectively vacuous.
Provider credentials travel in the
Authorizationheader, not in the JSON body, soall("server-only-secret" not in str(payload) ...)can't fail regardless of behavior. Assert on what the client actually receives instead (and/or on the captured headers).💚 Suggested strengthening
assert any(method == "POST" for method, _, _ in requests) assert all("server-only-secret" not in str(payload) for _, _, payload in requests) + assert "server-only-secret" not in listed.text + assert "server-only-secret" not in created.textSame pattern applies to the assertion at Line 618.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/api/test_cloud_projects_api.py` around lines 456 - 457, Update the assertions in the cloud-project API tests around the POST request checks, including the corresponding assertion near the later occurrence, to inspect the captured Authorization headers or the client-facing response rather than request payloads. Verify that server-only-secret is not exposed through the actual credential transport while preserving the existing POST-request assertion.wework/src/features/todo/CloudTodoWorkspace.test.tsx (1)
509-509: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the PR's headline path: creating a
publicproject.Both
createCloudProjectassertions pinvisibility: 'private'(the default); the new visibility radio group inProjectDialog(CloudTodoWorkspace.tsxLines 438-486) has no test that selects 公开 and assertsvisibility: 'public'reaches the API. Worth one added case in this file.🤖 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.test.tsx` at line 509, Add a test case in CloudTodoWorkspace.test.tsx that uses the ProjectDialog visibility radio group to select 公开 and then creates the project, asserting createCloudProject receives visibility: 'public'. Keep the existing private-visibility assertions unchanged.wework/src/features/todo/CloudTodoWorkspace.tsx (1)
1111-1137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a
data-testidon the sidebar project rows.These project buttons are the primary navigation affordance but carry no test id, so E2E/unit selection has to fall back to project name text.
♻️ Suggested addition
<button key={project.id} type="button" + data-testid={`cloud-project-row-${project.id}`} onClick={() => {As per coding guidelines: "give every new interactive element a descriptive
data-testid".🤖 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 1111 - 1137, The project navigation buttons rendered in the projects map need a descriptive test identifier. Add a data-testid to each button in the project row, using the project identity so individual rows can be selected reliably in tests while preserving the existing navigation 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 `@backend/app/api/endpoints/deliveries.py`:
- Line 337: Make external_loop_item_provider.ensure_shadow atomic and
conflict-safe so concurrent requests cannot fail on duplicate shadow insertion,
preserving its existing behavior when the shadow already exists. Apply the
helper fix to all six call sites in backend/app/api/endpoints/deliveries.py:
lines 337, 353, 400, 415, 430, and 446; each site requires conflict-safe shadow
creation for its respective endpoint.
- Around line 265-274: Update the external-provider branch in the delivery
reorder endpoint to avoid silently ignoring the submitted values: either
implement GitHub/GitLab ordering through the provider or reject the request with
a clear 4xx response. If rejecting, ensure the corresponding caller/UI does not
offer reorder for projects whose task_provider is "github" or "gitlab".
In `@backend/app/services/loop_items/external_provider.py`:
- Around line 354-371: Update the pagination loop around the `batch` handling to
retain the API response count separately from the pull-request-filtered results.
Use the unfiltered page length for the `len(...) < 100` termination check, while
continuing to extend `results` with the filtered GitHub issues.
- Around line 214-230: The external-provider item mapping around can_view
currently exposes metadata for non-viewable issues. Update this flow so public
visitors who cannot view an item receive no sensitive fields, either by
filtering those items from list results or consistently redacting the full
payload, including title, tags, status, priority, and creator name; preserve
full details for authorized viewers.
- Around line 72-80: Update the permission guard in the external issue creation
flow before _labels_for_write and _create_issue so every is_public_visitor is
rejected with HTTP 403, rather than bypassing the Reporter requirement. Preserve
the existing Reporter permission check for non-public users and prevent
restricted public visitors from creating provider-backed issues.
- Around line 496-506: Update _labels_for_write to remove caller-supplied labels
beginning with CREATOR_PREFIX in addition to priority and status labels, and
accept the trusted creator label separately so it is appended after sanitized
tags. Update create and update call sites to pass the server-generated creator
label through this trusted parameter, ensuring deduplication preserves the
trusted label rather than a forged user tag.
In `@executor/src/lib.rs`:
- Around line 55-65: Replace the Mach-O-specific __mod_init_func registration in
SCRUB_WEGENT_ENV with a portable initializer mechanism that runs scrub before
application startup across supported platforms, such as the project’s ctor-based
initializer or an equivalent platform-aware registration. Preserve the existing
scrub function’s iteration over HOST_STATE_VARS and its unsafe environment
removal behavior.
In `@executor/src/local/app_ipc.rs`:
- Around line 712-728: The external_projects.retain handler must no longer
perform destructive project refresh cleanup through retain_external_projects.
Remove or reroute this IPC operation so createCloudProjectSpaceApi’s
retaintProjects([]) cannot delete backend project_provider_credentials or
external_project_catalog entries; provide an explicit migration/cleanup
operation for intentional deletion instead.
In `@wework/src/api/hybrid/cloudProjectSpaceApi.ts`:
- Around line 16-21: Move the legacy externalIssueApi.retainProjects cleanup out
of the per-call listCloudProjects path and run it once during construction or
through a memoized promise. Handle or swallow cleanup failures so they cannot
reject listCloudProjects; preserve listing through storeApi.listCloudProjects
regardless of cleanup availability.
In `@wework/src/features/todo/CloudTodoWorkspace.tsx`:
- Line 770: Update canCreateBoardTask in CloudTodoWorkspace to derive from
selectedProject.access_role or the project’s existing can_create capability
rather than merely checking for a selected project, and apply the same
permission gate to every openTodoCreation entry point, including the “新建任务” UI,
column-header add, child creation, and GlobalTodoSearch selection path. If
role-based creation is not required, remove canCreateBoardTask and its
misleading gating.
---
Outside diff comments:
In `@executor/src/local/app_ipc.rs`:
- Around line 988-1028: Update the local attachment IPC client methods in
localDelivery.ts to send the required parameters: include project_id and item_id
for attachments.list, and include project_id, item_id, and attachment_id for
attachments.access and attachments.delete. Preserve the existing attachment
operation behavior while aligning each request payload with the IPC handlers.
---
Nitpick comments:
In `@backend/app/services/cloud_projects/service.py`:
- Around line 108-120: Consider adding a database index for the JSON visibility
expression used in the CloudProject query, specifically
CloudProject.metadata_json["visibility"].as_string(), using the project’s
migration/model indexing conventions. Ensure the index supports filtering for
the "public" value without changing the existing service query behavior.
In `@backend/app/services/loop_items/external_provider.py`:
- Around line 288-297: Refactor the provider operation around _request and the
related _repository flow to resolve _config only once per operation, then create
one context-managed httpx.Client and reuse it throughout the pagination loop.
Replace per-call httpx.request usage with the shared client, while preserving
request parameters, pagination behavior, and response handling.
- Around line 161-189: The ensure_shadow method has a check-then-insert race
that can surface a duplicate-primary-key IntegrityError. Make the insert/commit
path concurrency-safe by catching the commit IntegrityError, rolling back the
session, and re-reading LoopItem by item_id to return the concurrently created
record; preserve the existing creation path when no conflict occurs.
In `@backend/tests/api/test_cloud_projects_api.py`:
- Around line 456-457: Update the assertions in the cloud-project API tests
around the POST request checks, including the corresponding assertion near the
later occurrence, to inspect the captured Authorization headers or the
client-facing response rather than request payloads. Verify that
server-only-secret is not exposed through the actual credential transport while
preserving the existing POST-request assertion.
In `@executor/src/task_runtime/mcp.rs`:
- Around line 992-1004: Make the call_tool tests
searches_todos_by_text_and_structured_filters and the test at the referenced
second location deterministic by applying the Drop-based environment restore
guard pattern from local_app_ipc_contract.rs. Isolate and restore
WEGENT_TASK_PROJECT_ID, WEGENT_TASK_BACKEND_URL, and WEGENT_TASK_AUTH_TOKEN
around each test so parallel tests cannot leak ambient environment state into
these cases.
- Around line 371-382: Update the search_todos request in the task runtime,
along with the analogous request referenced at lines 526-541, to pass the
search, status, priority, and limit filters as supported query parameters to the
loop-items backend endpoint. Preserve the existing authentication, response
parsing, and task-return behavior while removing reliance on fetching and
filtering the entire board in memory.
- Around line 353-361: Update the HTTP client construction in call_backend_tool
and the corresponding client used for externally supplied attachment URLs to
configure an explicit connection timeout. Preserve the existing request behavior
while ensuring both backend and attachment fetch connections cannot block
indefinitely.
In `@executor/src/task_runtime/store.rs`:
- Around line 1251-1254: Update both new tests, including
removes_backend_external_project_credentials and the test around the second
referenced range, to use the existing store() helper instead of creating a
temporary directory and opening LocalTaskStore inline. In the retain test, also
assert that project_provider_credentials is pruned, covering the
retained-project cleanup path rather than relying only on
remove_external_project.
In `@wework/src/features/todo/CloudTodoWorkspace.test.tsx`:
- Line 509: Add a test case in CloudTodoWorkspace.test.tsx that uses the
ProjectDialog visibility radio group to select 公开 and then creates the project,
asserting createCloudProject receives visibility: 'public'. Keep the existing
private-visibility assertions unchanged.
In `@wework/src/features/todo/CloudTodoWorkspace.tsx`:
- Around line 1111-1137: The project navigation buttons rendered in the projects
map need a descriptive test identifier. Add a data-testid to each button in the
project row, using the project identity so individual rows can be selected
reliably in tests while preserving the existing navigation behavior.
In `@wework/src/features/todo/TaskSearchPanel.tsx`:
- Around line 26-68: Update TaskSearchPanel and its remaining hardcoded labels,
placeholders, aria-labels, option text, and empty/no-result messages to use the
local useTranslation('common') wrapper and workbench.task_search_* keys. Add
every new key with English and Simplified Chinese values to the corresponding
common.json locale files, then replace the literals throughout the component
with t(...) calls.
- Around line 69-190: The filter controls in TaskSearchPanel are duplicated
across seven near-identical select blocks. Introduce a small local FilterSelect
helper or configuration-driven rendering for status, priority, tag, assignee,
creator, due, and children, while preserving each control’s test id, aria-label,
value, options, and filter-key-specific onChange 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: 9d3daf49-87e2-4b6b-95ce-0cb622bf8578
📒 Files selected for processing (33)
backend/app/api/endpoints/cloud_projects.pybackend/app/api/endpoints/deliveries.pybackend/app/schemas/cloud_project.pybackend/app/schemas/delivery.pybackend/app/services/cloud_projects/service.pybackend/app/services/loop_items/external_provider.pybackend/app/services/loop_items/service.pybackend/tests/api/test_cloud_projects_api.pyexecutor/src/lib.rsexecutor/src/local/app_ipc.rsexecutor/src/process_environment.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.rswework/src/api/deliveries.tswework/src/api/hybrid/cloudProjectSpaceApi.test.tswework/src/api/hybrid/cloudProjectSpaceApi.tswework/src/api/local/localDelivery.tswework/src/features/todo/CloudProjectsHome.tsxwework/src/features/todo/CloudTodoWorkspace.test.tsxwework/src/features/todo/CloudTodoWorkspace.tsxwework/src/features/todo/GlobalTodoSearch.tsxwework/src/features/todo/TaskSearchPanel.tsxwework/src/features/todo/TaskSearchPermissions.test.tsxwework/src/features/todo/TodoEditor.tsxwework/src/features/todo/taskSearch.test.tswework/src/features/todo/taskSearch.tswework/src/features/workbench/workbenchServices.test.tswework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.json
💤 Files with no reviewable changes (1)
- backend/app/schemas/cloud_project.py
🚧 Files skipped from review as they are similar to previous changes (9)
- wework/src/i18n/locales/en/common.json
- wework/src/features/todo/CloudProjectsHome.tsx
- wework/src/api/deliveries.ts
- backend/app/api/endpoints/cloud_projects.py
- wework/src/features/todo/TodoEditor.tsx
- executor/src/task_runtime/router.rs
- backend/app/services/loop_items/service.py
- wework/src/api/local/localDelivery.ts
- executor/src/task_runtime/issue_provider.rs
| project = cloud_project_service.get(db, project_id, current_user.id) | ||
| if project.task_provider in {"github", "gitlab"}: | ||
| return LoopItemListResponse( | ||
| items=[ | ||
| LoopItemResponse.model_validate(item) | ||
| for item in external_loop_item_provider.list( | ||
| db, project_id, current_user.id | ||
| ) | ||
| ] | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject or implement external reorder requests.
Lines 265-274 ignore values and only reload the external items, so the endpoint returns success while leaving GitHub/GitLab ordering unchanged. Implement provider-specific ordering or return a clear 4xx response and prevent the caller from offering this action.
🤖 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 265 - 274, Update the
external-provider branch in the delivery reorder endpoint to avoid silently
ignoring the submitted values: either implement GitHub/GitLab ordering through
the provider or reject the request with a clear 4xx response. If rejecting,
ensure the corresponding caller/UI does not offer reorder for projects whose
task_provider is "github" or "gitlab".
| db: Session = Depends(get_db), | ||
| current_user: User = Depends(get_current_user), | ||
| ) -> list[LoopItemAttachmentResponse]: | ||
| external_loop_item_provider.ensure_shadow(db, item_id, current_user.id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make ensure_shadow atomic before using it from these concurrent endpoints.
The helper checks for a shadow and then inserts it in a separate step. Concurrent requests for the same external item can therefore collide on the primary key and return 500.
backend/app/api/endpoints/deliveries.py#L337-L337: make shadow creation conflict-safe for attachment listing.backend/app/api/endpoints/deliveries.py#L353-L353: make shadow creation conflict-safe for attachment creation.backend/app/api/endpoints/deliveries.py#L400-L400: make shadow creation conflict-safe for task listing.backend/app/api/endpoints/deliveries.py#L415-L415: make shadow creation conflict-safe for task unbinding.backend/app/api/endpoints/deliveries.py#L430-L430: make shadow creation conflict-safe for task binding.backend/app/api/endpoints/deliveries.py#L446-L446: make shadow creation conflict-safe before delivery creation.
📍 Affects 1 file
backend/app/api/endpoints/deliveries.py#L337-L337(this comment)backend/app/api/endpoints/deliveries.py#L353-L353backend/app/api/endpoints/deliveries.py#L400-L400backend/app/api/endpoints/deliveries.py#L415-L415backend/app/api/endpoints/deliveries.py#L430-L430backend/app/api/endpoints/deliveries.py#L446-L446
🤖 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` at line 337, Make
external_loop_item_provider.ensure_shadow atomic and conflict-safe so concurrent
requests cannot fail on duplicate shadow insertion, preserving its existing
behavior when the shadow already exists. Apply the helper fix to all six call
sites in backend/app/api/endpoints/deliveries.py: lines 337, 353, 400, 415, 430,
and 446; each site requires conflict-safe shadow creation for its respective
endpoint.
| if not access.is_public_visitor and not has_permission( | ||
| access.role, BaseRole.Reporter | ||
| ): | ||
| raise HTTPException(status.HTTP_403_FORBIDDEN, "Insufficient permission") | ||
| labels = self._labels_for_write( | ||
| values.tags + [f"{CREATOR_PREFIX}{user_id}:{self._safe_name(user_name)}"], | ||
| values.priority, | ||
| values.status, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect CloudProjectAccess semantics: who becomes is_public_visitor and what role they get.
fd -g 'access.py' backend/app/services/cloud_projects | xargs -r cat -n
rg -n 'is_public_visitor' backend --type=py -C3Repository: wecode-ai/Wegent
Length of output: 7511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== external_provider outline =="
ast-grep outline backend/app/services/loop_items/external_provider.py --view expanded | sed -n '1,220p' || true
echo
echo "== relevant external_provider lines =="
cat -n backend/app/services/loop_items/external_provider.py | sed -n '1,130p'
cat -n backend/app/services/loop_items/external_provider.py | sed -n '220,270p'
echo
echo "== BaseRole / has_permission =="
fd -g 'base_role.py' backend/app/schemas backend/app --type=py | xargs -r cat -n
echo
echo "== API/schema references around external_provider service =="
rg -n 'ExternalProvider|loop_items|external_provider|require_cloud_project_role\(' backend/app --type=py -C2Repository: wecode-ai/Wegent
Length of output: 10626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== base_role.py files =="
fd -g 'base_role.py' backend/app | while read -r f; do
echo "--- $f"
cat -n "$f"
done
echo
echo "== access.py base role refs =="
rg -n 'BaseRole|has_permission|is_public_visitor|RestrictedAnalyst|Owner|Developer|Reporter|RestrictedObserver|RestrictedManager|Admin' backend/app -g '*.py' -C2Repository: wecode-ai/Wegent
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== RestrictedAnalyst exact definitions/usages =="
rg -n 'RestrictedAnalyst|is_public_visitor|allow_public_visitor|external_loop_item|LoopItemProvider|create\(' backend/app --type=py -C2 | sed -n '1,220p'
echo
echo "== loop_items service relevant create path =="
cat -n backend/app/services/loop_items/service.py | sed -n '40,105p'
cat -n backend/app/services/loop_items/service.py | sed -n '480,515p'
echo
echo "== read-only verifier of current permission expression =="
python3 - <<'PY'
from enum import Enum
class BaseRole(str, Enum):
Owner = "Owner"
Maintainer = "Maintainer"
Developer = "Developer"
Reporter = "Read-only access"
RestrictedAnalyst = "Limited read access"
ROLE_HIERARCHY = {
"Owner": 0,
"Maintainer": 1,
"Developer": 2,
"Reporter": 3,
"RestrictedAnalyst": 4,
}
def has_permission(user_role, required_role):
user_level = ROLE_HIERARCHY.get(user_role, 999)
required_level = ROLE_HIERARCHY.get(required_role, 999)
return user_level <= required_level
roles = ["Owner", "Maintainer", "Developer", "Reporter", "RestrictedAnalyst"]
print("current gate condition:")
print("access.is_public_visitor", "role_has_reporter", "gated")
for role in roles:
is_public_visitor = role == "RestrictedAnalyst"
role_has_reporter = has_permission(role, "Reporter")
current_gate = not (not is_public_visitor and not role_has_reporter)
print(f"{role:20} {is_public_visitor!s:6} {role_has_reporter!s:8} {current_gate!s:6}")
print("\nproposed gate condition:")
for role in roles:
is_public_visitor = role == "RestrictedAnalyst"
role_has_reporter = has_permission(role, "Reporter")
proposed_gate = not (is_public_visitor or not role_has_reporter)
print(f"{role:20} {is_public_visitor!s:6} {role_has_reporter!s:8} {proposed_gate!s:6}")
PYRepository: wecode-ai/Wegent
Length of output: 21363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find API endpoints around cloud and delivery =="
fd '(.*)\.(py)$' backend/app/api -d 2 -x sh -c 'echo "--- $1"; rg -n "create.*loop|loop_items|Issue|issues|cloud_project|delivery|Provider|POST|PutRequest" "$1" -C3' sh {} | sed -n '1,260p'
echo
echo "== focused references to ExternalLoopItemProvider =="
rg -n 'external_loop_item_provider|ExternalLoopItemProvider|LoopItemCreate|LoopItemProvider' backend/app -C2Repository: wecode-ai/Wegent
Length of output: 9540
Block public visitors before creating external issues.
is_public_visitor is set for RestrictedAnalyst, which is defined as limited read access, but lines 72-74 let those visitors skip the Reporter requirement and create GitHub/GitLab issues through the provider credential. Reject public visitors here before _create_issue; any accepted public access should be explicit, attributed, and rate-limited.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 77-77: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
🤖 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 72 - 80,
Update the permission guard in the external issue creation flow before
_labels_for_write and _create_issue so every is_public_visitor is rejected with
HTTP 403, rather than bypassing the Reporter requirement. Preserve the existing
Reporter permission check for non-public users and prevent restricted public
visitors from creating provider-backed issues.
| return { | ||
| "id": f"{project.project_key}-{number}", | ||
| "cloud_project_id": str(project.id), | ||
| "sequence_number": number, | ||
| "parent_id": parent_id, | ||
| "title": str(issue.get("title") or ""), | ||
| "description": description if can_view else "", | ||
| "status": item_status, | ||
| "assignee_user_id": None, | ||
| "priority": self._priority(labels), | ||
| "due_at": None, | ||
| "sort_order": number, | ||
| "tags": self._public_tags(labels), | ||
| "created_by_user_id": creator_id, | ||
| "created_by_user_name": creator_name, | ||
| "can_view_detail": can_view, | ||
| "can_edit": can_edit, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
can_view=False redacts only the description; title and tags still leak.
For public visitors that don't own the item, title, tags, status, priority, and creator name are all returned in full, and list returns every issue in the repository. If can_view_detail is meant to hide other people's items, redact the whole payload (or filter non-viewable items out of list) rather than just the body.
🔒️ Sketch
- "title": str(issue.get("title") or ""),
+ "title": str(issue.get("title") or "") if can_view else "",
"description": description if can_view else "",
...
- "tags": self._public_tags(labels),
+ "tags": self._public_tags(labels) if can_view else [],📝 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.
| return { | |
| "id": f"{project.project_key}-{number}", | |
| "cloud_project_id": str(project.id), | |
| "sequence_number": number, | |
| "parent_id": parent_id, | |
| "title": str(issue.get("title") or ""), | |
| "description": description if can_view else "", | |
| "status": item_status, | |
| "assignee_user_id": None, | |
| "priority": self._priority(labels), | |
| "due_at": None, | |
| "sort_order": number, | |
| "tags": self._public_tags(labels), | |
| "created_by_user_id": creator_id, | |
| "created_by_user_name": creator_name, | |
| "can_view_detail": can_view, | |
| "can_edit": can_edit, | |
| return { | |
| "id": f"{project.project_key}-{number}", | |
| "cloud_project_id": str(project.id), | |
| "sequence_number": number, | |
| "parent_id": parent_id, | |
| "title": str(issue.get("title") or "") if can_view else "", | |
| "description": description if can_view else "", | |
| "status": item_status, | |
| "assignee_user_id": None, | |
| "priority": self._priority(labels), | |
| "due_at": None, | |
| "sort_order": number, | |
| "tags": self._public_tags(labels) if can_view else [], | |
| "created_by_user_id": creator_id, | |
| "created_by_user_name": creator_name, | |
| "can_view_detail": can_view, | |
| "can_edit": can_edit, |
🤖 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 214 - 230,
The external-provider item mapping around can_view currently exposes metadata
for non-viewable issues. Update this flow so public visitors who cannot view an
item receive no sensitive fields, either by filtering those items from list
results or consistently redacting the full payload, including title, tags,
status, priority, and creator name; preserve full details for authorized
viewers.
| for page in range(1, 101): | ||
| path = ( | ||
| f"/repos/{repository}/issues" | ||
| if project.task_provider == "github" | ||
| else f"/projects/{quote(repository, safe='')}/issues" | ||
| ) | ||
| batch = self._request( | ||
| project, | ||
| "GET", | ||
| path, | ||
| params={"state": "all", "per_page": 100, "page": page}, | ||
| ) | ||
| if project.task_provider == "github": | ||
| batch = [issue for issue in batch if "pull_request" not in issue] | ||
| results.extend(batch) | ||
| if len(batch) < 100: | ||
| break | ||
| return results |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pagination stops early on GitHub when a page contains pull requests.
batch is reassigned to the PR-filtered list before the len(batch) < 100 check, so a page of 100 items containing even one PR yields 99 and the loop breaks — silently dropping every later page of issues.
🐛 Proposed fix
batch = self._request(
project,
"GET",
path,
params={"state": "all", "per_page": 100, "page": page},
)
+ page_size = len(batch)
if project.task_provider == "github":
batch = [issue for issue in batch if "pull_request" not in issue]
results.extend(batch)
- if len(batch) < 100:
+ if page_size < 100:
break📝 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.
| for page in range(1, 101): | |
| path = ( | |
| f"/repos/{repository}/issues" | |
| if project.task_provider == "github" | |
| else f"/projects/{quote(repository, safe='')}/issues" | |
| ) | |
| batch = self._request( | |
| project, | |
| "GET", | |
| path, | |
| params={"state": "all", "per_page": 100, "page": page}, | |
| ) | |
| if project.task_provider == "github": | |
| batch = [issue for issue in batch if "pull_request" not in issue] | |
| results.extend(batch) | |
| if len(batch) < 100: | |
| break | |
| return results | |
| for page in range(1, 101): | |
| path = ( | |
| f"/repos/{repository}/issues" | |
| if project.task_provider == "github" | |
| else f"/projects/{quote(repository, safe='')}/issues" | |
| ) | |
| batch = self._request( | |
| project, | |
| "GET", | |
| path, | |
| params={"state": "all", "per_page": 100, "page": page}, | |
| ) | |
| page_size = len(batch) | |
| if project.task_provider == "github": | |
| batch = [issue for issue in batch if "pull_request" not in issue] | |
| results.extend(batch) | |
| if page_size < 100: | |
| break | |
| return results |
🤖 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 354 - 371,
Update the pagination loop around the `batch` handling to retain the API
response count separately from the pull-request-filtered results. Use the
unfiltered page length for the `len(...) < 100` termination check, while
continuing to extend `results` with the filtered GitHub issues.
| @staticmethod | ||
| def _labels_for_write( | ||
| tags: list[str], priority: str, item_status: str | ||
| ) -> list[str]: | ||
| labels = [ | ||
| tag for tag in tags if not tag.startswith((PRIORITY_PREFIX, STATUS_PREFIX)) | ||
| ] | ||
| if priority != "none": | ||
| labels.append(f"{PRIORITY_PREFIX}{priority}") | ||
| labels.append(f"{STATUS_PREFIX}{item_status}") | ||
| return list(dict.fromkeys(labels)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Creator labels are forgeable through user-supplied tags, which defeats the ownership check.
_labels_for_write strips only PRIORITY_PREFIX and STATUS_PREFIX, so a caller can pass tags=["wegent:creator:42:victim"] and have it written to the issue. _creator_label (Line 466) returns the first matching label, and in create (Line 77) the user tags precede the server-generated creator label — so the forged value wins. _permissions (Lines 246-248) then grants can_view/can_edit to any public visitor whose user_id matches the forged id, and _response reports a spoofed created_by_user_name. update (Lines 126-139) re-writes attacker tags the same way.
Strip CREATOR_PREFIX from caller-supplied tags and append the trusted label last.
🔒️ Proposed fix
`@staticmethod`
def _labels_for_write(
tags: list[str], priority: str, item_status: str
) -> list[str]:
labels = [
- tag for tag in tags if not tag.startswith((PRIORITY_PREFIX, STATUS_PREFIX))
+ tag
+ for tag in tags
+ if not tag.startswith((PRIORITY_PREFIX, STATUS_PREFIX, CREATOR_PREFIX))
]
if priority != "none":
labels.append(f"{PRIORITY_PREFIX}{priority}")
labels.append(f"{STATUS_PREFIX}{item_status}")
return list(dict.fromkeys(labels))Then pass the trusted creator separately, e.g. in create:
- labels = self._labels_for_write(
- values.tags + [f"{CREATOR_PREFIX}{user_id}:{self._safe_name(user_name)}"],
- values.priority,
- values.status,
- )
+ labels = [
+ *self._labels_for_write(values.tags, values.priority, values.status),
+ f"{CREATOR_PREFIX}{user_id}:{self._safe_name(user_name)}",
+ ]🤖 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 496 - 506,
Update _labels_for_write to remove caller-supplied labels beginning with
CREATOR_PREFIX in addition to priority and status labels, and accept the trusted
creator label separately so it is appended after sanitized tags. Update create
and update call sites to pass the server-generated creator label through this
trusted parameter, ensuring deduplication preserves the trusted label rather
than a forged user tag.
| #[used] | ||
| #[link_section = "__DATA,__mod_init_func"] | ||
| static SCRUB_WEGENT_ENV: extern "C" fn() = { | ||
| extern "C" fn scrub() { | ||
| for key in HOST_STATE_VARS { | ||
| // SAFETY: constructors run single-threaded before main. | ||
| unsafe { std::env::remove_var(key) }; | ||
| } | ||
| } | ||
| scrub | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm target platforms/CI runners and whether any portable ctor mechanism already exists.
fd -e toml -g 'Cargo.toml' | xargs -r rg -n 'edition|ctor|\[package\]'
fd . .github/workflows -e yml -e yaml 2>/dev/null | xargs -r rg -n 'runs-on|cargo test'
rg -n 'link_section|mod_init_func|init_array|CRT\$XCU' --type=rustRepository: wecode-ai/Wegent
Length of output: 146
🌐 Web query:
Rust link_section __DATA,__mod_init_func macOS vs .init_array Linux constructor portability
💡 Result:
The use of link_section for static constructors is highly platform-dependent, as macOS (Mach-O) and Linux (ELF) utilize entirely different object file formats and linker mechanisms [1][2][3]. You cannot use a single link_section attribute to achieve cross-platform behavior; instead, you must use conditional compilation [3]. On Linux (ELF), static constructors are traditionally placed in the.init_array section [3]. The linker collects pointers from this section and executes them at startup [3]. Rust code can target this by using #[link_section = ".init_array"] on a static function pointer [3]. On macOS (Mach-O), the equivalent mechanism typically uses the DATA,mod_init_func section [1][4][3]. Specifically, the correct approach requires identifying the section with the mod_init_funcs flag, often specified as #[link_section = "DATA,mod_init_func,mod_init_funcs"] [4][3]. Note that simply using DATA,mod_init_func may not always be treated as a GC root or valid initialization section by the linker if the proper flags are missing [1][4]. Because of these differences—as well as the additional complexities of Windows (.CRT$XCU) and other platforms—it is not recommended to manually manage these sections [3][5]. The standard practice in the Rust ecosystem is to use established crates that abstract these platform-specific differences [3][6][5]. The ctor crate is the most widely used solution for this purpose, as it correctly handles the conditional compilation and necessary linker attributes for Linux, macOS, Windows, and other targets [3][7]. For more advanced use cases involving linker-managed data collections, the link-section crate provides additional abstractions [3][8][5]. Using these abstractions is critical because code that runs before main is inherently fragile [8][7]. It must avoid relying on standard library features that are not yet initialized (such as thread-local storage or complex I/O) and must generally minimize or eliminate the possibility of panicking [8][7].
Citations:
- 1: Only compile #[used] as llvm.compiler.used for ELF targets rust-lang/rust#93718
- 2: https://docs.rs/link-section/latest/link_section/
- 3: https://docs.rs/ctor/latest/ctor/
- 4: add support for mod_init_funcs (was compat: rust-ctor fails to trigger with Cranelift on MacOS, but works with LLVM) rust-lang/rustc_codegen_cranelift#1588
- 5: https://grack.com/blog/2026/06/11/life-before-main/
- 6: https://github.com/thomcc/startup
- 7: https://docs.rs/ctor/latest/ctor/index.html
- 8: https://docs.rs/link-section/latest/link_section/life_before_main/index.html
🏁 Script executed:
#!/usr/bin/env bash
set -u
echo "== files =="
git ls-files | sed -n '1,200p'
echo "== locate executor lib =="
fd -a 'lib\.rs$' . | sed -n '1,80p'
echo "== search relevant symbols =="
rg -n 'SCRUB_WEGENT_ENV|HOST_STATE_VARS|link_section|mod_init_func|init_array|CRT\$XCU|crate::test_env|test_env::lock|i' . || true
echo "== workflow snippets =="
if [ -d .github/workflows ]; then
fd . .github/workflows -e yml -e yaml 2>/dev/null | while read -r f; do
echo "--- $f"
rg -n 'runs-on|cargo test|test' "$f" || true
done
fiRepository: wecode-ai/Wegent
Length of output: 146
🏁 Script executed:
#!/usr/bin/env bash
set -u
echo "== tracked files (first 200) =="
(git ls-files 2>/dev/null || true) | sed -n '1,200p'
echo "== rust files containing relevant identifiers =="
for pat in SCRUB_WEGENT_ENV HOST_STATE_VARS test_env::lock link_section mod_init_func init_array CRT\$XCU; do
echo "--- $pat"
rg -n --fixed-strings "$pat" -g '*.rs' . || true
done
echo "== workflow runs/cargo test snippets =="
if [ -d .github/workflows ]; then
find .github/workflows -type f \( -name '*.yaml' -o -name '*.yml' \) -print | sort | while read -r f; do
echo "--- $f"
rg -n 'runs-on|cargo test|name:|uses:' "$f" || true
done
else
echo "no .github/workflows"
fiRepository: wecode-ai/Wegent
Length of output: 146
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== repo files =="
git ls-files | sed -n '1,200p'
echo "== executor/lib.rs outline/sections =="
sed -n '1,120p' executor/src/lib.rs 2>/dev/null || true
echo "== CI/workflow files =="
find .github/workflows -type f 2>/dev/null | sort
echo "== workflow snippets =="
while IFS= read -r f; do
echo "--- $f"
rg -n 'runs-on|cargo test' "$f" || true
done < <(find .github/workflows -type f 2>/dev/null)
echo "== constructor/linking search =="
rg -n --glob '*.rs' --glob '*.toml' --glob '*.yaml' --glob '*.yml' \
'SCRUB_WEGENT_ENV|HOST_STATE_VARS|link_section|mod_init_func|init_array|CRT\$XCU|ctor|test_env::lock' . || trueRepository: wecode-ai/Wegent
Length of output: 146
🌐 Web query:
site:wirced.ai Wegent executor/src/lib.rs SCRUB_WEGENT_ENV HOST_STATE_VARS
💡 Result:
I checked wecode-ai/Wegent’s executor/src/lib.rs on main: the file exists, is only 48 lines, and just re-exports modules. It does not contain SCRUB_WEGENT_ENV or HOST_STATE_VARS. The executor/src tree does include process_environment.rs and hooks/, which look more relevant if you’re tracing env handling. (github.com)
Citations:
Run the environment scrub from a portable initializer
__DATA,__mod_init_func is a Mach-O section; on Linux/ELF the equivalent initializer section is .init_array (.CRT$XCU on MSVC), so this static does not guarantee the host env variables are cleared where CI runs. Use a portable initializer approach instead, such as ctor/dtor or a call-site initializer guarded behind env lock.
🤖 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/lib.rs` around lines 55 - 65, Replace the Mach-O-specific
__mod_init_func registration in SCRUB_WEGENT_ENV with a portable initializer
mechanism that runs scrub before application startup across supported platforms,
such as the project’s ctor-based initializer or an equivalent platform-aware
registration. Preserve the existing scrub function’s iteration over
HOST_STATE_VARS and its unsafe environment removal behavior.
| "external_projects.retain" => { | ||
| let project_ids = params | ||
| .get("project_ids") | ||
| .and_then(Value::as_array) | ||
| .ok_or_else(|| AppIpcError::new("bad_request", "project_ids must be an array"))? | ||
| .iter() | ||
| .map(|value| { | ||
| value.as_str().map(ToOwned::to_owned).ok_or_else(|| { | ||
| AppIpcError::new("bad_request", "project_ids must contain strings") | ||
| }) | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| runtime | ||
| .retain_external_projects(&project_ids) | ||
| .map_err(task_runtime_error)?; | ||
| Ok(json!({})) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C6 'retainProjects|external_projects\.retain' --type=ts --type=tsxRepository: wecode-ai/Wegent
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked files matching app_ipc.ts/tsx or retainProject:\n'
git ls-files | rg '(^|/)(app_ipc|.*ipc|.*external.*project).*\.(ts|tsx|js|jsx)$|project|credential' | head -n 200
printf '\nRepository-wide search retainProjects/external_projects.retain/retain_external_projects:\n'
rg -n -C 5 'retainProjects|external_projects\.retain|retain_external_projects|retainProjects\(' . || true
printf '\napp_ipc.rs relevant functions:\n'
rg -n -C 8 'external_projects\.retain|retain_external_projects|project_store|project_provider_credentials|external_project_catalog' executor/src/local/app_ipc.rs || trueRepository: wecode-ai/Wegent
Length of output: 16740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'executor/src/task_runtime/store.rs retain_external_projects implementation:\n'
sed -n '190,240p' executor/src/task_runtime/store.rs
printf '\nExecutor tests around retain_external_projects:\n'
sed -n '1260,1355p' executor/src/task_runtime/store.rs
printf '\nwework/src/api/hybrid/cloudProjectSpaceApi.ts:\n'
sed -n '1,45p' wework/src/api/hybrid/cloudProjectSpaceApi.ts
printf '\nwework/src/features/workbench/workbenchServices.test.ts relevant part:\n'
sed -n '70,105p' wework/src/features/workbench/workbenchServices.test.ts
printf '\nRead-only semantic/model check for DELETE/INSERT behavior with empty retained set:\n'
python3 - <<'PY'
from pathlib import Path
p = Path("executor/src/task_runtime/store.rs")
s = p.read_text()
start = s.index("pub fn retain_external_projects")
end = s.index("pub fn remove_external_project", start)
fn = s[start:end]
checks = {
"deletes_project_provider_credentials": "DELETE FROM project_provider_credentials" in fn and "WHERE project_id NOT IN" in fn and "retained" in fn,
"deletes_external_project_catalog": "DELETE FROM external_project_catalog" in fn and "WHERE cloud_project_id NOT IN" in fn and "retained" in fn,
"empty_retained_set_logic": "WHERE (<SELECT" in fn or "NOT IN" in fn,
"empty_retained_set_select": "SELECT '2568b5cb-d512-45da-ae5d-c6974188290f' WHERE false" in fn or ("not_retained" in fn and "SELECT" in fn),
}
print("Function:")
print(fn)
print("Checks:")
for k, v in checks.items():
print(f"{k}: {v}")
# Simulate the simplest SQL-case model where retention IDs are passed as a list.
retained = []
all_projects = ["cloud-a", "cloud-b"]
deleted_credentials = [p for p in all_projects if p not in retained]
deleted_catalog = [p for p in all_projects if p not in retained]
print("\nEmpty retain payload deletes:", {"credentials": deleted_credentials, "catalog": deleted_catalog})
PYRepository: wecode-ai/Wegent
Length of output: 8087
Do not send the project refresh cleanup via external_projects.retain.
createCloudProjectSpaceApi calls retaintProjects([]) before listCloudProjects, and retain_external_projects deletes every backend project_provider_credentials / external_project_catalog entry not in the retained list. A failed or empty projects list can wipe all cached backend project credentials; remove this destructive call path and use an explicit migration/cleanup operation instead.
🤖 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 712 - 728, The
external_projects.retain handler must no longer perform destructive project
refresh cleanup through retain_external_projects. Remove or reroute this IPC
operation so createCloudProjectSpaceApi’s retaintProjects([]) cannot delete
backend project_provider_credentials or external_project_catalog entries;
provide an explicit migration/cleanup operation for intentional deletion
instead.
| async listCloudProjects() { | ||
| const response = await storeApi.listCloudProjects() | ||
| await Promise.all( | ||
| response.items.map(async project => { | ||
| rememberProject(project) | ||
| if ( | ||
| isExternalProject(project) && | ||
| project.provider_config.credential_configured !== false | ||
| ) { | ||
| let credential | ||
| try { | ||
| credential = await storeApi.getCloudProjectProviderCredential(project.id) | ||
| } catch { | ||
| return | ||
| } | ||
| try { | ||
| await externalIssueApi.configureProject(project, credential.token) | ||
| } catch { | ||
| return | ||
| } | ||
| } | ||
| }) | ||
| ) | ||
| return response | ||
| }, | ||
| async createCloudProject(data) { | ||
| const { token, ...providerConfig } = data.provider_config ?? {} | ||
| const storedProject = await storeApi.createCloudProject({ | ||
| ...data, | ||
| provider_config: data.provider_config, | ||
| }) | ||
| const project = rememberProject({ | ||
| ...storedProject, | ||
| project_store: 'backend', | ||
| task_provider: data.task_provider ?? 'local', | ||
| provider_config: providerConfig, | ||
| }) | ||
| if (isExternalProject(project)) { | ||
| await externalIssueApi.configureProject(project, token) | ||
| } | ||
| return project | ||
| }, | ||
| async updateCloudProject(projectId, data) { | ||
| const current = requireProject(projectId) | ||
| const updated = await storeApi.updateCloudProject(projectId, data) | ||
| const project = rememberProject({ | ||
| ...updated, | ||
| project_store: current.project_store, | ||
| task_provider: current.task_provider, | ||
| provider_config: updated.provider_config ?? current.provider_config, | ||
| }) | ||
| if (isExternalProject(project)) { | ||
| const credential = await storeApi.getCloudProjectProviderCredential(project.id) | ||
| await externalIssueApi.configureProject(project, credential.token) | ||
| } | ||
| return project | ||
| }, | ||
| async listLoopItems(projectId) { | ||
| const project = requireProject(projectId) | ||
| const response = isExternalProject(project) | ||
| ? await externalIssueApi.listLoopItems(project) | ||
| : await storeApi.listLoopItems(projectId) | ||
| rememberTasks(project.id, response.items) | ||
| return response | ||
| }, | ||
| async getLoopItem(itemId) { | ||
| const project = requireTaskProject(itemId) | ||
| const item = isExternalProject(project) | ||
| ? await externalIssueApi.getLoopItem(project, itemId) | ||
| : await storeApi.getLoopItem(itemId) | ||
| taskProjects.set(item.id, project.id) | ||
| return item | ||
| }, | ||
| async createLoopItem(projectId, data) { | ||
| const project = requireProject(projectId) | ||
| const item = isExternalProject(project) | ||
| ? await externalIssueApi.createLoopItem(project, data) | ||
| : await storeApi.createLoopItem(projectId, data) | ||
| taskProjects.set(item.id, project.id) | ||
| return item | ||
| }, | ||
| async updateLoopItem(itemId, data) { | ||
| const project = requireTaskProject(itemId) | ||
| const item = isExternalProject(project) | ||
| ? await externalIssueApi.updateLoopItem(project, itemId, data) | ||
| : await storeApi.updateLoopItem(itemId, data) | ||
| taskProjects.set(item.id, project.id) | ||
| return item | ||
| }, | ||
| async reorderLoopItems(projectId, data) { | ||
| const project = requireProject(projectId) | ||
| if (!isExternalProject(project)) { | ||
| return storeApi.reorderLoopItems(projectId, data) | ||
| } | ||
| const response = await externalIssueApi.listLoopItems(project) | ||
| rememberTasks(project.id, response.items) | ||
| return response | ||
| }, | ||
| async listDeliveries(itemId) { | ||
| return isExternalProject(requireTaskProject(itemId)) | ||
| ? { items: [] } | ||
| : storeApi.listDeliveries(itemId) | ||
| }, | ||
| async listTaskBindings(itemId) { | ||
| return isExternalProject(requireTaskProject(itemId)) ? [] : storeApi.listTaskBindings(itemId) | ||
| }, | ||
| async listLoopItemAttachments(itemId) { | ||
| return isExternalProject(requireTaskProject(itemId)) | ||
| ? [] | ||
| : storeApi.listLoopItemAttachments(itemId) | ||
| }, | ||
| async listLoopItemCollaborators(itemId) { | ||
| return isExternalProject(requireTaskProject(itemId)) | ||
| ? [] | ||
| : storeApi.listLoopItemCollaborators(itemId) | ||
| // Remove credentials/catalog entries written by older Wework versions. | ||
| // Backend-owned projects never execute through the local task runtime. | ||
| await externalIssueApi.retainProjects?.([]) | ||
| return storeApi.listCloudProjects() | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Legacy cleanup runs on every listCloudProjects call and can fail the whole listing.
retainProjects?.([]) is a one-time migration cleanup, but it's awaited on each project list. Two consequences: every listing pays an extra IPC round-trip, and if the local runtime is unavailable the rejection propagates — CloudTodoWorkspace's per-api catch then yields an empty project list, so a cleanup hiccup silently hides all cloud projects. Run it once (at construction / behind a memoized promise) and don't let it reject the listing.
🛡️ Proposed fix
export function createCloudProjectSpaceApi(
storeApi: DeliveryApi,
externalIssueApi: ExternalIssueApi
): DeliveryApi {
+ // Remove credentials/catalog entries written by older Wework versions.
+ // Backend-owned projects never execute through the local task runtime.
+ const legacyCleanup = Promise.resolve(externalIssueApi.retainProjects?.([])).catch(error => {
+ console.error('[Wework project space] legacy external project cleanup failed', error)
+ })
return {
...storeApi,
async listCloudProjects() {
- // Remove credentials/catalog entries written by older Wework versions.
- // Backend-owned projects never execute through the local task runtime.
- await externalIssueApi.retainProjects?.([])
+ await legacyCleanup
return storeApi.listCloudProjects()
},
}
}📝 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 listCloudProjects() { | |
| const response = await storeApi.listCloudProjects() | |
| await Promise.all( | |
| response.items.map(async project => { | |
| rememberProject(project) | |
| if ( | |
| isExternalProject(project) && | |
| project.provider_config.credential_configured !== false | |
| ) { | |
| let credential | |
| try { | |
| credential = await storeApi.getCloudProjectProviderCredential(project.id) | |
| } catch { | |
| return | |
| } | |
| try { | |
| await externalIssueApi.configureProject(project, credential.token) | |
| } catch { | |
| return | |
| } | |
| } | |
| }) | |
| ) | |
| return response | |
| }, | |
| async createCloudProject(data) { | |
| const { token, ...providerConfig } = data.provider_config ?? {} | |
| const storedProject = await storeApi.createCloudProject({ | |
| ...data, | |
| provider_config: data.provider_config, | |
| }) | |
| const project = rememberProject({ | |
| ...storedProject, | |
| project_store: 'backend', | |
| task_provider: data.task_provider ?? 'local', | |
| provider_config: providerConfig, | |
| }) | |
| if (isExternalProject(project)) { | |
| await externalIssueApi.configureProject(project, token) | |
| } | |
| return project | |
| }, | |
| async updateCloudProject(projectId, data) { | |
| const current = requireProject(projectId) | |
| const updated = await storeApi.updateCloudProject(projectId, data) | |
| const project = rememberProject({ | |
| ...updated, | |
| project_store: current.project_store, | |
| task_provider: current.task_provider, | |
| provider_config: updated.provider_config ?? current.provider_config, | |
| }) | |
| if (isExternalProject(project)) { | |
| const credential = await storeApi.getCloudProjectProviderCredential(project.id) | |
| await externalIssueApi.configureProject(project, credential.token) | |
| } | |
| return project | |
| }, | |
| async listLoopItems(projectId) { | |
| const project = requireProject(projectId) | |
| const response = isExternalProject(project) | |
| ? await externalIssueApi.listLoopItems(project) | |
| : await storeApi.listLoopItems(projectId) | |
| rememberTasks(project.id, response.items) | |
| return response | |
| }, | |
| async getLoopItem(itemId) { | |
| const project = requireTaskProject(itemId) | |
| const item = isExternalProject(project) | |
| ? await externalIssueApi.getLoopItem(project, itemId) | |
| : await storeApi.getLoopItem(itemId) | |
| taskProjects.set(item.id, project.id) | |
| return item | |
| }, | |
| async createLoopItem(projectId, data) { | |
| const project = requireProject(projectId) | |
| const item = isExternalProject(project) | |
| ? await externalIssueApi.createLoopItem(project, data) | |
| : await storeApi.createLoopItem(projectId, data) | |
| taskProjects.set(item.id, project.id) | |
| return item | |
| }, | |
| async updateLoopItem(itemId, data) { | |
| const project = requireTaskProject(itemId) | |
| const item = isExternalProject(project) | |
| ? await externalIssueApi.updateLoopItem(project, itemId, data) | |
| : await storeApi.updateLoopItem(itemId, data) | |
| taskProjects.set(item.id, project.id) | |
| return item | |
| }, | |
| async reorderLoopItems(projectId, data) { | |
| const project = requireProject(projectId) | |
| if (!isExternalProject(project)) { | |
| return storeApi.reorderLoopItems(projectId, data) | |
| } | |
| const response = await externalIssueApi.listLoopItems(project) | |
| rememberTasks(project.id, response.items) | |
| return response | |
| }, | |
| async listDeliveries(itemId) { | |
| return isExternalProject(requireTaskProject(itemId)) | |
| ? { items: [] } | |
| : storeApi.listDeliveries(itemId) | |
| }, | |
| async listTaskBindings(itemId) { | |
| return isExternalProject(requireTaskProject(itemId)) ? [] : storeApi.listTaskBindings(itemId) | |
| }, | |
| async listLoopItemAttachments(itemId) { | |
| return isExternalProject(requireTaskProject(itemId)) | |
| ? [] | |
| : storeApi.listLoopItemAttachments(itemId) | |
| }, | |
| async listLoopItemCollaborators(itemId) { | |
| return isExternalProject(requireTaskProject(itemId)) | |
| ? [] | |
| : storeApi.listLoopItemCollaborators(itemId) | |
| // Remove credentials/catalog entries written by older Wework versions. | |
| // Backend-owned projects never execute through the local task runtime. | |
| await externalIssueApi.retainProjects?.([]) | |
| return storeApi.listCloudProjects() | |
| }, | |
| export function createCloudProjectSpaceApi( | |
| storeApi: DeliveryApi, | |
| externalIssueApi: ExternalIssueApi | |
| ): DeliveryApi { | |
| // Remove credentials/catalog entries written by older Wework versions. | |
| // Backend-owned projects never execute through the local task runtime. | |
| const legacyCleanup = Promise.resolve(externalIssueApi.retainProjects?.([])).catch(error => { | |
| console.error('[Wework project space] legacy external project cleanup failed', error) | |
| }) | |
| return { | |
| ...storeApi, | |
| async listCloudProjects() { | |
| await legacyCleanup | |
| return storeApi.listCloudProjects() | |
| }, | |
| } | |
| } |
🤖 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 16 - 21, Move the
legacy externalIssueApi.retainProjects cleanup out of the per-call
listCloudProjects path and run it once during construction or through a memoized
promise. Handle or swallow cleanup failures so they cannot reject
listCloudProjects; preserve listing through storeApi.listCloudProjects
regardless of cleanup availability.
| [availableProjectSpaceApis, projectSpaceApis, projects, services.deliveryApi] | ||
| ) | ||
| const selectedProjectApi = selectedProject ? apiForProjectId(selectedProject.id) : undefined | ||
| const canCreateBoardTask = selectedProject !== null |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Which creation affordances exist and how they are gated
rg -nP --type=tsx -C3 'canCreateBoardTask|openTodoCreation\(' wework/src/features/todo/CloudTodoWorkspace.tsx
# Does the backend allow RestrictedAnalyst/public visitors to create loop items?
rg -nP -C4 'allow_public_visitor' backend/app/services/loop_items/service.pyRepository: wecode-ai/Wegent
Length of output: 1475
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and lines =="
wc -l wework/src/features/todo/CloudTodoWorkspace.tsx
sed -n '750,785p' wework/src/features/todo/CloudTodoWorkspace.tsx
sed -n '1270,1315p' wework/src/features/todo/CloudTodoWorkspace.tsx
sed -n '1325,1360p' wework/src/features/todo/CloudTodoWorkspace.tsx
sed -n '1450,1510p' wework/src/features/todo/CloudTodoWorkspace.tsx
sed -n '1520,1560p' wework/src/features/todo/CloudTodoWorkspace.tsx
echo "== all access_role/permissions references in file =="
rg -n 'access_role|can_create|RestrictedAnalyst|openTodoCreation|canCreateBoardTask' wework/src/features/todo/CloudTodoWorkspace.tsx || true
echo "== selection branch outline around render =="
sed -n '1,1285p' wework/src/features/todo/CloudTodoWorkspace.tsx | tail -n 120Repository: wecode-ai/Wegent
Length of output: 17356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate CloudTodoWorkspace.tsx references =="
rg -n 'CloudTodoWorkspace|cloud-todo-add|cloud-todo-column-add-|cloud-todo-column-bottom-add-|canCreateBoardTask|RestrictedAnalyst|allow_public_visitor|allow_public' -S .
echo "== search loop-item create services for public_visitor behavior =="
rg -n -C5 'cloud_project_id|allow_public_visitor|create|LoopItem|parent_id|parent_item' backend -S | head -n 220
echo "== inspect create endpoints in backend =="
rg -n -C6 'router\.(post|put|delete)|/cycles|/items|item|loop' backend -S | head -n 260Repository: wecode-ai/Wegent
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
path = Path('wework/src/features/todo/CloudTodoWorkspace.tsx')
text = path.read_text()
lines = text.splitlines()
def context(i, n=4):
start = max(1, i-n)
end = min(len(lines), i+n)
return '\n'.join(f"{j+1}: {lines[j]}" for j in range(start, end+1))
events = []
pat = re.compile(r'\b(canCreateBoardTask|openTodoCreation|selectedProject)\b')
for j, line in enumerate(lines, start=1):
if pat.search(line):
events.append((j, line.rstrip()))
print("EVENTS:")
for j, line in events:
print(f"{j}: {line}")
print("\nCAN CREATE DEFINITION:")
print(context(lines.index(next(l for l in lines if 'const canCreateBoardTask' in l)) if any('const canCreateBoardTask' in l for l in lines) else 770-1, n=2))
print("\nSELECTED PROJECT CONDITIONAL BLOCK:")
i = next(j for j, l in enumerate(lines, start=1) if l.strip().startswith(': ('))
print(context(i, n=25))
print("\nGATED CONDITIONAL BLOCKS:")
for i, line in enumerate(lines, start=1):
if line.strip().startswith('{'):
# simple brace matcher for this line's condition is hard; just print if relevant nearby
window = ' '.join(lines[i-2:i+8])
if 'canCreateBoardTask' in line or 'openTodoCreation' in line:
print(f"\nAround line {i}:")
print('\n'.join(lines[i-5:i+12]))
PY
echo "== openTodoCreation implementation =="
sed -n '810,850p' wework/src/features/todo/CloudTodoWorkspace.tsx
echo "== backend loop item relevant services snippets =="
sed -n '1,110p' backend/app/services/loop_items/service.py
sed -n '480,520p' backend/app/services/loop_items/service.py
sed -n '640,670p' backend/app/services/loop_items/service.pyRepository: wecode-ai/Wegent
Length of output: 2387
Make the creation gate match the intended permission rule.
canCreateBoardTask is defined as selectedProject !== null, but all canCreateBoardTask-gated UI is already only within the selected-project branch, so it does not hide “新建任务” for roles that shouldn’t create. The column header add, child creation, and GlobalTodoSearch selection path also call openTodoCreation without this gate, so the restriction is tautological/incomplete if creation is meant to be role-gated. Derive it from selectedProject.access_role / a project capability like backend can_create, and apply it consistently to every creation entry point; otherwise drop the flag.
🤖 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` at line 770, Update
canCreateBoardTask in CloudTodoWorkspace to derive from
selectedProject.access_role or the project’s existing can_create capability
rather than merely checking for a selected project, and apply the same
permission gate to every openTodoCreation entry point, including the “新建任务” UI,
column-header add, child creation, and GlobalTodoSearch selection path. If
role-based creation is not required, remove canCreateBoardTask and its
misleading gating.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes