Cherrypick/application shell#1
Open
soyboyscout wants to merge 6 commits intomainfrom
Open
Conversation
New crate for CherryPick git client features within Zed. Module stubs for sidebar panel, view tab, branch list, commit graph, staging, worktree/stash lists, remote toolbar, repo state banner, and settings. Wired into Zed's init path.
CherryPickSidebar implementing Panel trait with dock right, focus handle, ToggleSidebar action, and placeholder render. Registered in workspace via observe_new pattern.
BranchList component with placeholder render. CherryPickView implementing Item trait with split layout (graph placeholder top, staging placeholder bottom), tab content, and tooltip.
Port cherrypick_pr and cherrypick_agent into the Zed workspace as cherrypick_pr and cherrypick_agent crates. Wire cherrypick_ui sidebar to live git repository data: branch listing with checkout, file staging with stage/unstage, remote fetch/pull/push, local PR creation with SQLite persistence, and unified diff viewing with colored +/- lines.
Add a "Graph" button to the CherryPick sidebar that opens Zed's built-in git graph view as a workspace tab. Reuses the full GitGraph implementation rather than reimplementing commit visualization from scratch.
📝 WalkthroughWalkthroughAdds three new workspace crates: an LLM-driven agent (chat, tools, MCP, persistence), a PR service with SQLite-backed store and merge tooling, and a GPUI-based UI exposing PR/cherrypick workflows; wires them into the existing workspace and editor initialization. ChangesWorkspace Configuration
cherrypick_agent (Agent Core, Providers, Tools, Storage, Skills)
cherrypick_pr (PR domain, store, services, watcher, diff)
cherrypick_ui (GPUI components & state)
Sequence DiagramssequenceDiagram
participant User
participant Agent as AgentService
participant Store as ChatStore
participant Context as ContextEngine
participant Provider as LLM Provider
participant Tools as ToolExecutor
User->>Agent: send_message(user_message, repo_path)
Agent->>Store: append user message
Agent->>Context: truncate_history(...)
Agent->>Provider: stream_completion(request)
loop streaming
Provider-->>Agent: StreamChunk (TextDelta / ToolCall events / Usage)
Agent->>Agent: accumulate text and tool args
Agent-->>User: emit AgentEvent (TextDelta / ToolCallStarted)
end
Provider-->>Agent: Done
Agent->>Store: append assistant message
alt tool calls present
Agent->>Tools: execute(tool_name, args, repo_path)
Tools-->>Agent: result
Agent->>Store: append tool result
Agent-->>User: emit ToolCallCompleted
Agent->>Provider: (possibly) stream next turn
else no tools
Agent-->>User: emit TurnComplete
end
sequenceDiagram
participant User
participant PrSvc as PrService
participant Git as git2
participant Store as PrStore
participant Diff as DiffService
participant Merge as MergeService
User->>PrSvc: create_pr(title, source_branch, target_branch)
PrSvc->>Git: discover repo, resolve branch OIDs
PrSvc->>Git: find_first_commit_oid, compute remotes hash
PrSvc->>Store: ensure_repo(first_oid, remote_hash)
Store-->>PrSvc: repo_id
PrSvc->>Store: create_pr(...)
Store-->>PrSvc: pr_id
PrSvc->>Store: record_snapshot(pr_id, ...)
PrSvc-->>User: LocalPr
User->>PrSvc: get_branch_diff(source_oid, target_oid)
PrSvc->>Diff: get_branch_diff(...)
Diff->>Git: compute diff or return cache
Diff-->>PrSvc: BranchDiff
PrSvc-->>User: BranchDiff
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Kilo Code Review could not run — your account is out of credits. Add credits or switch to a free model to enable reviews on this change. |
|
🐕 Corgea found the following new SCA issues in the codebase:
Showing 10 out of 41 findings. See full results |
Review Summary by QodoImplement cherry-pick application shell with agent, PR management, and UI layers
WalkthroughsDescription• Implements comprehensive cherry-pick application shell with three new crates: cherrypick_agent, cherrypick_pr, and cherrypick_ui • **Agent system**: Implements AgentService for multi-turn LLM conversations with tool orchestration, MCP client for external tool integration, and skill system for composable AI capabilities • **PR management**: Provides PrService, MergeService, DiffService, and PrStore for PR lifecycle, conflict resolution, branch diffing, and persistent storage • **Git operations**: Implements 8 git tool handlers with security checks (path traversal prevention, sensitive path detection) and comprehensive error handling • **UI components**: Builds complete UI layer with sidebar panel, PR list, branch list, staging view, PR detail view, diff display, and remote operations toolbar • **Data persistence**: Adds SQLite-backed storage for PR metadata, conversations, and repository tracking with proper schema and indexes • **LLM integration**: Implements Anthropic provider with streaming support, context budget management, and token estimation • **Security**: Includes secure path resolution, API key management with pluggable backends, and safety policies for git operations • **Workspace integration**: Registers CherryPick sidebar panel and initializes UI components in main application Diagramflowchart LR
A["User Interface<br/>Sidebar, PR List,<br/>Branch List"]
B["PR Management<br/>Service, Store,<br/>Merge, Diff"]
C["Agent System<br/>LLM Provider,<br/>Tool Executor,<br/>Skills"]
D["Git Operations<br/>Git Tools,<br/>Branch Watcher"]
E["Data Layer<br/>SQLite Storage<br/>Chat, PR, Repo"]
F["External<br/>Anthropic API<br/>MCP Servers"]
A -->|"manages"| B
A -->|"orchestrates"| C
B -->|"uses"| D
C -->|"executes"| D
C -->|"persists"| E
C -->|"calls"| F
B -->|"stores"| E
File Changes1. crates/cherrypick_agent/src/tools/git_tools.rs
|
Code Review by Qodo
1. MCP buffered output loss
|
|
Summary By MatterAI
🔄 What ChangedThis PR introduces a massive overhaul of the AI Agent infrastructure and CI/CD pipelines. Key changes include the introduction of the 🔍 Impact of the ChangeThe agentic capabilities of Zed are now significantly more robust, allowing for complex, multi-phase plans and sub-agent spawning. Security posture is improved by preventing shell injection via terminal tools. The release process is more automated with integrated compliance checks and Slack notifications for unreviewed hotfixes. 📁 Total Files ChangedClick to Expand
🧪 Test Added/RecommendedAdded
Recommended
🔒Security Vulnerabilities
Self-Review Checklist:
Closes #N/A Release Notes:
⏳ Estimated code review effortHIGH (~50 minutes) Tip Quality Recommendations
♫ Tanka Poem
Sequence DiagramsequenceDiagram
participant U as User
participant T as AcpThread
participant A as NativeAgent
participant LM as LanguageModel
participant TS as ToolSystem
U->>T: Send Message
T->>A: run_turn()
A->>LM: build_completion_request(intent)
LM-->>A: stream(CompletionEvent)
alt Tool Use Detected
A->>TS: run_tool(tool_call)
TS->>TS: validate_terminal_command(input)
opt Permission Required
TS->>T: authorize(context)
T-->>U: Show Permission UI
U->>T: Approve (Always/Once)
T-->>TS: SelectedPermissionOutcome
end
TS-->>A: ToolResult
A->>LM: Follow-up Request (ToolResult)
end
A->>T: handle_session_update(Usage/Plan)
T-->>U: Update UI (Cost/Plan/Message)
|
![matterai-app[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧪 PR Review is completed: The PR introduces a robust architecture for the CherryPick git client. A few issues were identified regarding data integrity in the chat store, incorrect OID validation in the diff service, and handling of empty tool arguments.
Skipped files
Cargo.toml: Skipped file patterncrates/cherrypick_agent/Cargo.toml: Skipped file patterncrates/cherrypick_pr/Cargo.toml: Skipped file patterncrates/cherrypick_ui/Cargo.toml: Skipped file patterncrates/zed/Cargo.toml: Skipped file pattern
Comment on lines
+122
to
+136
| let is_lfs = if !is_binary { | ||
| if let Some(oid) = new_file.id().as_bytes().first() { | ||
| if *oid != 0 { | ||
| let blob = repo.find_blob(new_file.id()); | ||
| blob.map(|b| FileContent::is_lfs_pointer(b.content())) | ||
| .unwrap_or(false) | ||
| } else { | ||
| false | ||
| } | ||
| } else { | ||
| false | ||
| } | ||
| } else { | ||
| false | ||
| }; |
There was a problem hiding this comment.
🟠 Logic Error
Issue: Checking if *oid != 0 only checks if the first byte of the OID is non-zero. A valid git blob OID can legitimately start with a zero byte, which would cause this code to incorrectly skip the LFS pointer check for those files.
Fix: Use !new_file.id().is_zero() to correctly check if the entire OID is non-zero (which indicates a valid object ID).
Impact: Ensures LFS pointers are correctly detected even if their blob OID happens to start with a zero byte.
Suggested change
| let is_lfs = if !is_binary { | |
| if let Some(oid) = new_file.id().as_bytes().first() { | |
| if *oid != 0 { | |
| let blob = repo.find_blob(new_file.id()); | |
| blob.map(|b| FileContent::is_lfs_pointer(b.content())) | |
| .unwrap_or(false) | |
| } else { | |
| false | |
| } | |
| } else { | |
| false | |
| } | |
| } else { | |
| false | |
| }; | |
| let is_lfs = if !is_binary && !new_file.id().is_zero() { | |
| let blob = repo.find_blob(new_file.id()); | |
| blob.map(|b| FileContent::is_lfs_pointer(b.content())) | |
| .unwrap_or(false) | |
| } else { | |
| false | |
| }; |
Comment on lines
+137
to
+146
| conn.execute( | ||
| "INSERT INTO messages (conversation_id, role, content_json, created_at) | ||
| VALUES (?1, ?2, ?3, ?4)", | ||
| params![conversation_id, role, content, now], | ||
| )?; | ||
| conn.execute( | ||
| "UPDATE conversations SET updated_at = ?1 WHERE id = ?2", | ||
| params![now, conversation_id], | ||
| )?; | ||
| Ok(conn.last_insert_rowid()) |
There was a problem hiding this comment.
🟠 Data Integrity
Issue: Inserting a message and updating the conversation's updated_at timestamp are performed as separate queries without a transaction. If the second query fails, the database will be left in an inconsistent state.
Fix: Wrap both operations in a database transaction.
Impact: Prevents partial updates and ensures database consistency.
Suggested change
| conn.execute( | |
| "INSERT INTO messages (conversation_id, role, content_json, created_at) | |
| VALUES (?1, ?2, ?3, ?4)", | |
| params![conversation_id, role, content, now], | |
| )?; | |
| conn.execute( | |
| "UPDATE conversations SET updated_at = ?1 WHERE id = ?2", | |
| params![now, conversation_id], | |
| )?; | |
| Ok(conn.last_insert_rowid()) | |
| let tx = conn.transaction()?; | |
| tx.execute( | |
| "INSERT INTO messages (conversation_id, role, content_json, created_at) | |
| VALUES (?1, ?2, ?3, ?4)", | |
| params![conversation_id, role, content, now], | |
| )?; | |
| let id = tx.last_insert_rowid(); | |
| tx.execute( | |
| "UPDATE conversations SET updated_at = ?1 WHERE id = ?2", | |
| params![now, conversation_id], | |
| )?; | |
| tx.commit()?; | |
| Ok(id) |
Comment on lines
+176
to
+179
| StreamChunk::ToolCallEnd => { | ||
| let args: serde_json::Value = | ||
| serde_json::from_str(¤t_tool_args).unwrap_or_default(); | ||
| tool_calls.push(ToolCall { |
There was a problem hiding this comment.
🟡 Logic Error
Issue: When current_tool_args is an empty string (which can happen if the LLM calls a tool with no arguments), serde_json::from_str will fail and unwrap_or_default() will return Value::Null. If the tool expects an empty object {}, this might cause parsing errors downstream.
Fix: Default to an empty JSON object json!({}) instead of Null.
Impact: Improves robustness when handling tool calls without arguments.
Suggested change
| StreamChunk::ToolCallEnd => { | |
| let args: serde_json::Value = | |
| serde_json::from_str(¤t_tool_args).unwrap_or_default(); | |
| tool_calls.push(ToolCall { | |
| StreamChunk::ToolCallEnd => { | |
| let args: serde_json::Value = | |
| serde_json::from_str(¤t_tool_args).unwrap_or_else(|_| serde_json::json!({})); | |
| tool_calls.push(ToolCall { |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new cherrypick feature set, including an agent for AI-assisted git operations and a UI for managing pull requests and staging changes. The changes include new crates for the agent and PR management, as well as integration into the main Zed application. I have reviewed the code and provided feedback on error handling and path safety. Please address the suggestions regarding robust error handling and path validation to ensure the stability and security of the new functionality.
Comment on lines
+396
to
+397
| index.add_path(Path::new(path)) | ||
| .map_err(|e| AgentError::ToolExecution(format!("Failed to stage {path}: {e}")))?; |
There was a problem hiding this comment.
Using Path::new(path) directly inside index.add_path might be risky if path is not properly sanitized. Ensure path is validated against the repository root.
Suggested change
| index.add_path(Path::new(path)) | |
| .map_err(|e| AgentError::ToolExecution(format!("Failed to stage {path}: {e}")))?; | |
| let safe_path = resolve_safe_path(repo_path, path)?; | |
| index.add_path(safe_path.strip_prefix(repo_path).unwrap_or(Path::new(path))) |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+181
to
+205
| let stdout = process | ||
| .stdout | ||
| .as_mut() | ||
| .ok_or_else(|| AgentError::Mcp("No stdout".into()))?; | ||
| let mut reader = BufReader::new(stdout); | ||
| let mut line = String::new(); | ||
|
|
||
| let read_result = timeout(REQUEST_TIMEOUT, reader.read_line(&mut line)).await; | ||
|
|
||
| match read_result { | ||
| Ok(Ok(0)) => Err(AgentError::Mcp("Server closed connection".into())), | ||
| Ok(Ok(_)) => { | ||
| if line.len() > MAX_MESSAGE_SIZE { | ||
| return Err(AgentError::Mcp("Response too large".into())); | ||
| } | ||
| let response: JsonRpcResponse = serde_json::from_str(&line) | ||
| .map_err(|e| AgentError::Mcp(format!("Invalid response: {e}")))?; | ||
| if let Some(error) = response.error { | ||
| Err(AgentError::Mcp(format!( | ||
| "RPC error {}: {}", | ||
| error.code, error.message | ||
| ))) | ||
| } else { | ||
| Ok(response.result) | ||
| } |
There was a problem hiding this comment.
1. Mcp buffered output loss 🐞 Bug ☼ Reliability
McpClient::send_request constructs a new BufReader around the child stdout on every request; if the reader buffers beyond the newline, dropping it discards unread bytes and can desynchronize subsequent JSON-RPC reads. The function also doesn’t validate the response id, so notifications/out-of-order replies can be misattributed to the wrong request.
Agent Prompt
### Issue description
`McpClient::send_request` creates a new `BufReader` on `ChildStdout` for each request and drops it after reading one line. If the reader performs read-ahead (or a timeout happens mid-read), buffered bytes can be lost, corrupting the request/response stream. Additionally, the parsed JSON-RPC `id` is not checked, so notifications or out-of-order responses can be associated with the wrong request.
### Issue Context
This is the core transport for MCP `tools/list` and `tools/call`. Stream corruption will manifest as intermittent “Invalid response” / timeouts or wrong tool results.
### Fix Focus Areas
- crates/cherrypick_agent/src/mcp.rs[150-209]
### What to change
- Store a persistent stdout reader in `McpClient` (e.g., `Option<BufReader<ChildStdout>>`) initialized in `start()`, and reuse it across calls.
- Implement response routing by `id`:
- After sending request with `id`, read lines until a response with matching `id` is received.
- Optionally handle/ignore JSON-RPC notifications (messages without `id`).
- Ensure timeouts/partial reads do not discard buffered data (reusing a single `BufReader` is the simplest way).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+146
to
+175
| let mut merge_opts = git2::MergeOptions::new(); | ||
| let mut index = repo.merge_commits(&source, &target, Some(&mut merge_opts))?; | ||
|
|
||
| for (path, content) in &session.resolutions { | ||
| let blob_oid = repo.blob(content)?; | ||
| let entry = git2::IndexEntry { | ||
| ctime: git2::IndexTime::new(0, 0), | ||
| mtime: git2::IndexTime::new(0, 0), | ||
| dev: 0, | ||
| ino: 0, | ||
| mode: 0o100644, | ||
| uid: 0, | ||
| gid: 0, | ||
| file_size: content.len() as u32, | ||
| id: blob_oid, | ||
| flags: 0, | ||
| flags_extended: 0, | ||
| path: path.as_bytes().to_vec(), | ||
| }; | ||
| index.add(&entry)?; | ||
| } | ||
|
|
||
| let tree_oid = index.write_tree_to(&repo)?; | ||
| let tree = repo.find_tree(tree_oid)?; | ||
| let sig = repo.signature()?; | ||
|
|
||
| let merge_oid = match strategy { | ||
| MergeStrategy::MergeCommit => { | ||
| repo.commit(None, &sig, &sig, message, &tree, &[&target, &source])? | ||
| } |
There was a problem hiding this comment.
2. Merge direction mismatch 🐞 Bug ≡ Correctness
MergeService calls repo.merge_commits(&source, &target, ...) while elsewhere treating target as “ours” (e.g., conflict content uses target.tree() as ours and merge commit parents are [target, source]). This inconsistent ordering can produce a merge result in the wrong direction and incorrect conflict semantics.
Agent Prompt
### Issue description
`MergeService` computes merge results via `repo.merge_commits(&source, &target, ...)` but the rest of the code treats `target` as “ours” and `source` as “theirs” (conflict content + merge commit parents). This inconsistency can generate an inverted merge tree/conflicts.
### Issue Context
- `get_conflict_content` uses `target.tree()` as `ours` and `source.tree()` as `theirs`.
- Merge commit parents are `[&target, &source]`.
These imply the intended merge is: merge `source` into `target`.
### Fix Focus Areas
- crates/cherrypick_pr/src/merge_service.rs[22-44]
- crates/cherrypick_pr/src/merge_service.rs[130-177]
### What to change
- Update `merge_commits` calls to use `(&target, &source)` if the intent is “merge source into target”.
- Ensure any “ours/theirs” naming in conflict handling stays consistent with that ordering.
- Add/extend tests to assert the produced merge tree content matches expectations (not just that conflicts exist).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+392
to
+398
| for path in &paths { | ||
| if super::is_sensitive_path(path) { | ||
| return Err(AgentError::ToolExecution(format!("Cannot stage sensitive file: {path}"))); | ||
| } | ||
| index.add_path(Path::new(path)) | ||
| .map_err(|e| AgentError::ToolExecution(format!("Failed to stage {path}: {e}")))?; | ||
| } |
There was a problem hiding this comment.
3. Unsafe staging path handling 🐞 Bug ⛨ Security
StageFilesTool forwards caller-provided strings directly to git2::Index::add_path without using resolve_safe_path or rejecting traversal/absolute paths. This can attempt to stage unintended paths and is inconsistent with the stricter path safety used by read_file/search_code.
Agent Prompt
### Issue description
`StageFilesTool` does not validate that `paths` are inside the repository root before calling `index.add_path(Path::new(path))`. Unlike `read_file`/`search_code`, it does not use `resolve_safe_path`, so traversal or unexpected inputs can cause staging failures or staging unintended files.
### Issue Context
`resolve_safe_path` already exists and enforces canonical-root containment and symlink escape rejection.
### Fix Focus Areas
- crates/cherrypick_agent/src/tools/git_tools.rs[383-401]
- crates/cherrypick_agent/src/tools/safe_path.rs[1-66]
### What to change
- For each requested path:
- Reject absolute paths.
- Use `resolve_safe_path(repo_path, path)` to get an absolute safe path.
- Convert it back to a repo-relative path for git index operations (strip the canonical repo root).
- Reject if stripping fails.
- Keep the sensitive-path check, but run it on the repo-relative path you will stage.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “CherryPick” feature set into the Zed application shell, adding a dedicated sidebar/panel UI plus supporting crates for local PR tracking, diffs, merging, and an agent/tooling layer.
Changes:
- Integrates
cherrypick_uiinto Zed startup and panel initialization. - Adds
cherrypick_uicrate with sidebar + views for branches/PRs/diffs/staging. - Adds
cherrypick_pr(SQLite-backed local PR store + diff/merge utilities) andcherrypick_agent(tool executor + provider + MCP client).
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/zed/src/zed.rs | Loads the CherryPick sidebar panel alongside existing panels. |
| crates/zed/src/main.rs | Initializes cherrypick_ui during app startup. |
| crates/zed/Cargo.toml | Adds cherrypick_ui dependency to the Zed crate. |
| crates/cherrypick_ui/src/worktree_list.rs | Placeholder module stub. |
| crates/cherrypick_ui/src/stash_list.rs | Placeholder module stub. |
| crates/cherrypick_ui/src/staging_view.rs | Adds a staging UI for staged/unstaged file toggling. |
| crates/cherrypick_ui/src/settings.rs | Placeholder module stub. |
| crates/cherrypick_ui/src/repo_state_banner.rs | Placeholder module stub. |
| crates/cherrypick_ui/src/remote_toolbar.rs | Adds fetch/pull/push toolbar UI for repository remotes. |
| crates/cherrypick_ui/src/pr_state.rs | Introduces PR state management bridging UI to cherrypick_pr. |
| crates/cherrypick_ui/src/pr_list.rs | Adds UI list for local PRs. |
| crates/cherrypick_ui/src/pr_detail.rs | Adds UI for PR details and actions (close/reopen). |
| crates/cherrypick_ui/src/lib.rs | Exposes modules and registers the sidebar on workspace creation. |
| crates/cherrypick_ui/src/diff_file_list.rs | Adds UI list for files in a branch diff. |
| crates/cherrypick_ui/src/create_pr_form.rs | Adds UI form to create a local PR from/to selected branches. |
| crates/cherrypick_ui/src/commit_graph_embed.rs | Adds helper to open the existing Git graph from CherryPick UI. |
| crates/cherrypick_ui/src/cherrypick_view.rs | Adds main CherryPick view for staging/diff rendering. |
| crates/cherrypick_ui/src/cherrypick_sidebar.rs | Implements the dockable CherryPick sidebar panel and actions. |
| crates/cherrypick_ui/src/branch_list.rs | Adds UI for displaying and checking out branches. |
| crates/cherrypick_ui/Cargo.toml | Defines the new cherrypick_ui crate and dependencies. |
| crates/cherrypick_pr/src/watcher.rs | Adds filesystem watcher for branch ref changes + force-push detection helper. |
| crates/cherrypick_pr/src/types.rs | Defines PR/domain types (status, snapshots, file content helpers). |
| crates/cherrypick_pr/src/store.rs | Adds SQLite-backed PR storage and queries. |
| crates/cherrypick_pr/src/service.rs | Adds higher-level PR service operations (create/close/reopen/retarget). |
| crates/cherrypick_pr/src/merge_service.rs | Adds merge/conflict services and merge session tracking. |
| crates/cherrypick_pr/src/lib.rs | Exposes cherrypick_pr public API surface. |
| crates/cherrypick_pr/src/error.rs | Adds error types for PR/merge/storage operations. |
| crates/cherrypick_pr/src/diff_service.rs | Adds cached diff computation between branch tips. |
| crates/cherrypick_pr/Cargo.toml | Defines the new cherrypick_pr crate and dependencies. |
| crates/cherrypick_agent/src/tools/safe_path.rs | Adds safe path resolution to prevent traversal/symlink escapes. |
| crates/cherrypick_agent/src/tools/mod.rs | Adds tool executor framework and builtin tool registration. |
| crates/cherrypick_agent/src/tools/git_tools.rs | Implements git-related tools (status/diff/log/search/stage/commit). |
| crates/cherrypick_agent/src/skills/mod.rs | Adds skill definitions and markdown-based skill loading. |
| crates/cherrypick_agent/src/provider/types.rs | Defines provider-agnostic message/tool streaming types. |
| crates/cherrypick_agent/src/provider/mod.rs | Adds provider trait and module wiring. |
| crates/cherrypick_agent/src/provider/anthropic.rs | Implements Anthropic streaming integration (SSE parsing). |
| crates/cherrypick_agent/src/mcp.rs | Adds MCP client/manager for external tool servers. |
| crates/cherrypick_agent/src/lib.rs | Exposes cherrypick_agent public API surface. |
| crates/cherrypick_agent/src/keys.rs | Adds key management abstraction + env/in-memory backends. |
| crates/cherrypick_agent/src/error.rs | Adds agent error types. |
| crates/cherrypick_agent/src/context/mod.rs | Adds context budgeting and history truncation logic. |
| crates/cherrypick_agent/src/chat/store.rs | Adds SQLite-backed chat storage for conversations/messages. |
| crates/cherrypick_agent/src/chat/mod.rs | Implements agent loop: provider streaming + tool execution. |
| crates/cherrypick_agent/Cargo.toml | Defines the new cherrypick_agent crate and dependencies. |
| Cargo.toml | Registers new workspace members and adds workspace dependencies. |
| Cargo.lock | Records dependency graph updates for the added crates/deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+194
to
+196
| "content_block_stop" => { | ||
| let _ = tx.send(StreamChunk::ToolCallEnd); | ||
| } |
There was a problem hiding this comment.
content_block_stop is mapped to StreamChunk::ToolCallEnd unconditionally. Anthropic sends content_block_stop for text blocks too, so this will generate spurious tool calls (often with empty id/name/args) and can lead to executing a nonexistent tool or mis-parsing subsequent tool JSON. Track the active block type/index and only emit ToolCallEnd for tool_use blocks (and likewise only buffer JSON deltas for tool_use).
Comment on lines
+153
to
+155
| let text = response.text().await?; | ||
|
|
||
| for line in text.lines() { |
There was a problem hiding this comment.
This implementation reads the entire response body via response.text().await? and then iterates over lines. That defeats SSE streaming (no incremental UI updates), can increase latency, and may allocate large buffers for long generations. Consider consuming response.bytes_stream() and parsing SSE incrementally.
Comment on lines
+238
to
+240
| // For now, auto-approve in the agentic loop. | ||
| // In the full UI implementation, this would wait for user input | ||
| // via a oneshot channel before proceeding. |
There was a problem hiding this comment.
Tools that require confirmation (risk.requires_confirmation()) are still executed automatically after emitting ConfirmationNeeded. This contradicts the stated safety policy and makes it possible for write-capable tools to run without explicit user approval. The agent loop should block and wait for a user response (e.g., via a oneshot) before executing these tools, or deny by default.
Suggested change
| // For now, auto-approve in the agentic loop. | |
| // In the full UI implementation, this would wait for user input | |
| // via a oneshot channel before proceeding. | |
| let err_msg = | |
| "Tool execution blocked pending explicit user approval".to_string(); | |
| let _ = event_tx.send(AgentEvent::ToolCallCompleted { | |
| id: tc.id.clone(), | |
| result: err_msg.clone(), | |
| is_error: true, | |
| }); | |
| self.history | |
| .push(Message::tool_result(&tc.id, &err_msg, true)); | |
| continue; |
Comment on lines
+39
to
+71
| let refs_dir = git_dir.join("refs").join("heads"); | ||
| let head_file = git_dir.join("HEAD"); | ||
|
|
||
| let tx_clone = tx.clone(); | ||
| let watcher_result = notify::recommended_watcher(move |res: std::result::Result<Event, notify::Error>| { | ||
| match res { | ||
| Ok(event) => { | ||
| if matches!( | ||
| event.kind, | ||
| EventKind::Modify(_) | EventKind::Create(_) | EventKind::Remove(_) | ||
| ) { | ||
| for path in &event.paths { | ||
| if let Some(branch) = extract_branch_name(path, &refs_dir) { | ||
| match event.kind { | ||
| EventKind::Remove(_) => { | ||
| let _ = tx_clone.send(BranchEvent::BranchDeleted { | ||
| branch, | ||
| }); | ||
| } | ||
| _ => { | ||
| let new_oid = std::fs::read_to_string(path) | ||
| .unwrap_or_default() | ||
| .trim() | ||
| .to_string(); | ||
| let _ = tx_clone.send(BranchEvent::BranchTipChanged { | ||
| branch, | ||
| old_oid: None, | ||
| new_oid, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
HEAD is added to the watcher list, but the event handler only extracts branch names under refs/heads. As a result, updates to HEAD (e.g., switching branches) will be ignored. Handle HEAD path changes explicitly (e.g., read and resolve the new ref) or remove the unused watch.
| nanoid = "0.4" | ||
| nbformat = "1.2.0" | ||
| nix = "0.29" | ||
| notify = "7" |
There was a problem hiding this comment.
The workspace pins notify = "7", but the existing fs crate depends on notify = "8.2.0" directly. This introduces two major versions of notify into the build (see Cargo.lock), increasing compile times and binary size and potentially complicating platform watcher behavior. Prefer aligning on a single notify major version (e.g., update the new crates to notify 8.x and set the workspace dependency accordingly).
Suggested change
| notify = "7" | |
| notify = "8.2.0" |
| let mut merge_opts = git2::MergeOptions::new(); | ||
| merge_opts.file_favor(git2::FileFavor::Normal); | ||
|
|
||
| let index = repo.merge_commits(&source, &target, Some(&merge_opts))?; |
There was a problem hiding this comment.
merge_commits takes (our, their) commits; for “merge source into target”, target should be our and source should be their. Passing them reversed will compute conflicts and the merge result in the opposite direction. Swap the argument order consistently in conflict checking and merge execution.
Suggested change
| let index = repo.merge_commits(&source, &target, Some(&merge_opts))?; | |
| let index = repo.merge_commits(&target, &source, Some(&merge_opts))?; |
Comment on lines
+123
to
+130
| if let Some(oid) = new_file.id().as_bytes().first() { | ||
| if *oid != 0 { | ||
| let blob = repo.find_blob(new_file.id()); | ||
| blob.map(|b| FileContent::is_lfs_pointer(b.content())) | ||
| .unwrap_or(false) | ||
| } else { | ||
| false | ||
| } |
There was a problem hiding this comment.
LFS pointer detection checks only the first byte of the blob OID (new_file.id().as_bytes().first()) to decide whether the OID is non-zero. A valid OID can start with 0x00, so this can incorrectly skip LFS detection. Compare against git2::Oid::zero() (or use is_zero() if available) instead of inspecting a single byte.
Suggested change
| if let Some(oid) = new_file.id().as_bytes().first() { | |
| if *oid != 0 { | |
| let blob = repo.find_blob(new_file.id()); | |
| blob.map(|b| FileContent::is_lfs_pointer(b.content())) | |
| .unwrap_or(false) | |
| } else { | |
| false | |
| } | |
| if new_file.id() != git2::Oid::zero() { | |
| let blob = repo.find_blob(new_file.id()); | |
| blob.map(|b| FileContent::is_lfs_pointer(b.content())) | |
| .unwrap_or(false) |
| let pr = service | ||
| .create_pr(&path, "My PR", "feature", "master") | ||
| .await; | ||
| assert!(pr.is_ok() || pr.is_err()); |
There was a problem hiding this comment.
The test create_pr_validates_branches ends with assert!(pr.is_ok() || pr.is_err()), which is tautologically true and doesn’t validate any behavior. Replace this with a concrete assertion (e.g., that creating a PR succeeds when both branches exist, or returns BranchNotFound when the target branch is missing).
Suggested change
| assert!(pr.is_ok() || pr.is_err()); | |
| assert!(matches!(pr, Err(PrError::BranchNotFound(_)))); |
Comment on lines
+61
to
+66
| use sha2::Digest; | ||
| let hash = format!("{:x}", sha2::Sha256::digest(repo_path.to_string_lossy().as_bytes())); | ||
| let canonical = repo_path.to_string_lossy().to_string(); | ||
|
|
||
| Ok((db_path, first_oid, hash, canonical)) | ||
| }); |
There was a problem hiding this comment.
ensure_repo expects a remote_urls_hash, but initialize computes hash from the repo path string (Sha256(repo_path)), not from remote URLs. This can cause the same repo to be treated as different identities when cloned in multiple locations, and it’s inconsistent with PrService::ensure_repo_identity which hashes remote URLs. Consider extracting the same remote-URL hashing logic (or renaming the column/variable if path-hash is intended).
Comment on lines
+388
to
+429
| let mut container = div() | ||
| .id("unified-diff") | ||
| .flex() | ||
| .flex_col() | ||
| .flex_grow() | ||
| .overflow_y_scroll() | ||
| .p_1() | ||
| .text_xs() | ||
| .font_family("monospace"); | ||
|
|
||
| for line in diff_text.lines() { | ||
| let color = if line.starts_with('+') && !line.starts_with("+++") { | ||
| added_color | ||
| } else if line.starts_with('-') && !line.starts_with("---") { | ||
| deleted_color | ||
| } else if line.starts_with("@@") { | ||
| muted | ||
| } else if line.starts_with("diff ") || line.starts_with("index ") { | ||
| muted | ||
| } else { | ||
| text_color | ||
| }; | ||
|
|
||
| let bg = if line.starts_with('+') && !line.starts_with("+++") { | ||
| Some(cx.theme().colors().version_control_added.opacity(0.1)) | ||
| } else if line.starts_with('-') && !line.starts_with("---") { | ||
| Some(cx.theme().colors().version_control_deleted.opacity(0.1)) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let mut line_div = div() | ||
| .text_color(color) | ||
| .px_1() | ||
| .child(line.to_string()); | ||
|
|
||
| if let Some(bg_color) = bg { | ||
| line_div = line_div.bg(bg_color); | ||
| } | ||
|
|
||
| container = container.child(line_div); | ||
| } |
There was a problem hiding this comment.
Rendering the unified diff by creating a separate UI element for every line (for line in diff_text.lines()) can become very slow and memory-heavy for large diffs. Consider virtualizing the diff view, collapsing unchanged sections, or imposing a line limit with a “load more” affordance to keep rendering responsive.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
crates/cherrypick_agent/src/tools/safe_path.rs-6-13 (1)
6-13:⚠️ Potential issue | 🟡 MinorEmpty-path branch bypasses canonical root validation.
Line 6 returns
repo_rootdirectly, skipping the canonicalization/error path at Lines 10–12. This makes empty-path behavior inconsistent and can hide invalid root paths.🔧 Suggested fix
pub fn resolve_safe_path(repo_root: &Path, relative_path: &str) -> Result<PathBuf> { - if relative_path.is_empty() { - return Ok(repo_root.to_path_buf()); - } - let canonical_root = repo_root .canonicalize() .map_err(|e| AgentError::ToolExecution(format!("Cannot canonicalize repo root: {e}")))?; + + if relative_path.is_empty() { + return Ok(canonical_root); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/tools/safe_path.rs` around lines 6 - 13, The early return when relative_path.is_empty() skips validating repo_root; change the logic in safe_path.rs so repo_root is always canonicalized via repo_root.canonicalize() (the canonical_root variable) before returning, and then if relative_path.is_empty() return that canonical_root (or its PathBuf) instead of the original repo_root; ensure errors from canonicalize still map to AgentError::ToolExecution as done currently.crates/cherrypick_ui/src/remote_toolbar.rs-110-114 (1)
110-114:⚠️ Potential issue | 🟡 MinorPush with empty branch name when no branch is available.
If
repositoryisNoneor has no branch,branch_namebecomes an empty string viaunwrap_or_default(). Pushing with an empty branch name could lead to unexpected git behavior or errors.Consider returning early or disabling the push button when no branch is available, similar to how fetch/pull/push already guard against missing repository.
🛡️ Suggested guard
fn do_push(&mut self, window: &mut Window, cx: &mut Context<Self>) { let Some(repo) = self.repository.clone() else { return; }; + let Some(branch_name) = self + .repository + .as_ref() + .and_then(|r| r.read(cx).branch.as_ref().map(|b| b.name().to_string())) + else { + // No branch to push + return; + }; let askpass = Self::noop_askpass(cx); - let branch_name = self - .repository - .as_ref() - .and_then(|r| r.read(cx).branch.as_ref().map(|b| b.name().to_string())) - .unwrap_or_default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_ui/src/remote_toolbar.rs` around lines 110 - 114, The code computes branch_name with unwrap_or_default(), producing an empty string when repository or branch is missing; change this to guard explicitly: check self.repository.as_ref().and_then(|r| r.read(cx).branch.as_ref()). If that returns None, either return early from the handler or set the Push UI control disabled (mirroring the existing fetch/pull/push guards) instead of using branch_name; use the real branch via .name().to_string() only when present so Push logic never runs with an empty branch_name.crates/cherrypick_agent/src/provider/anthropic.rs-194-196 (1)
194-196:⚠️ Potential issue | 🟡 Minor
content_block_stopunconditionally sendsToolCallEnd, even for text blocks.This could confuse consumers that track tool call state. Consider tracking the current block type and only emitting
ToolCallEndwhen a tool_use block ends.💡 Suggested approach
Track the current block type when
content_block_startis received (using theindexfield), and only sendToolCallEndwhen the stopped block was a tool_use block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/provider/anthropic.rs` around lines 194 - 196, The handler currently sends StreamChunk::ToolCallEnd unconditionally on "content_block_stop", which mislabels ended text blocks as tool calls; modify the logic to track the active block type when "content_block_start" events arrive (use the provided index field) and store whether that index corresponds to a "tool_use" block, then on "content_block_stop" only send StreamChunk::ToolCallEnd via tx if the stopped block's stored type is tool_use; update any state map/variable used to track block index → block_type and clear it when the block stops.crates/cherrypick_pr/src/diff_service.rs-138-146 (1)
138-146:⚠️ Potential issue | 🟡 MinorPer-file insertion/deletion counts are always zero.
DiffFileEntryhasinsertionsanddeletionsfields, but they're hardcoded to0in the constructor. The total stats are computed (lines 154-155), but per-file stats require iterating hunks viadiff.foreach()or usingdiff.stats_format_text(). This may mislead UI consumers expecting per-file metrics.💡 Suggested approach to compute per-file stats
Use
diff.foreach()with a file callback to accumulate per-file line counts, or compute from patch hunks. Alternatively, document that per-file stats are not available and remove the fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/diff_service.rs` around lines 138 - 146, DiffFileEntry instances are being constructed with insertions and deletions hardcoded to 0 (see the files.push(DiffFileEntry { ... }) call), which hides per-file stats; update the diff processing to compute per-file insertion/deletion counts by iterating the diff hunks (e.g., use diff.foreach() with the file/patc h/hunk callbacks or parse diff.stats_format_text()) and set the insertions/deletions fields on each DiffFileEntry accordingly, or if per-file stats cannot be derived, remove those fields or document they are unavailable; reference the DiffFileEntry struct and the diff.foreach()/diff.stats_format_text() helpers when implementing the change.crates/cherrypick_ui/src/create_pr_form.rs-98-110 (1)
98-110:⚠️ Potential issue | 🟡 MinorCycling source/target can result in source == target.
After cycling,
selected_sourceandselected_targetcan become equal, but this is only validated at submit time (lines 121-124). Consider either preventing the cycle that would make them equal, or showing a visual warning when they match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_ui/src/create_pr_form.rs` around lines 98 - 110, The cycle_source and cycle_target methods currently allow selected_source == selected_target; change them to skip any index that would make source and target equal by advancing until you find a different index (or stop if branch_names.len() <= 1), update selected_source/selected_target to that non-equal index, and still call cx.notify(); reference the methods cycle_source and cycle_target and the fields selected_source, selected_target, and branch_names when making the change so the UI never transitions into an equal-state (alternatively add a visual warning in the form submit/validation code where equality is checked).crates/cherrypick_ui/src/cherrypick_sidebar.rs-208-215 (1)
208-215:⚠️ Potential issue | 🟡 MinorEarly return after async initialization may cause PR creation to be silently dropped.
When
pr_stateis not initialized, you callinitialize().detach()and immediately return. This means the PR creation request is lost. Consider queuing the request or awaiting initialization completion before returning.💡 Suggested approach
fn handle_create_pr(&mut self, title: String, source: String, target: String, cx: &mut Context<Self>) { log::info!("cherrypick: handle_create_pr called: '{}' {} → {}", title, source, target); if !self.pr_state.is_initialized() { log::error!("cherrypick: PR state not initialized, attempting init"); - self.pr_state.initialize().detach(); - return; + // Queue the PR creation after initialization completes + let task = self.pr_state.initialize(); + let pr_list = self.pr_list.clone(); + let create_form = self.create_pr_form.clone(); + let title_clone = title.clone(); + let source_clone = source.clone(); + let target_clone = target.clone(); + cx.spawn(async move |this, cx| { + if task.await.is_ok() { + let _ = this.update(cx, |this, cx| { + this.handle_create_pr(title_clone, source_clone, target_clone, cx); + }); + } + }).detach(); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_ui/src/cherrypick_sidebar.rs` around lines 208 - 215, handle_create_pr currently calls self.pr_state.initialize().detach() and returns, which drops the incoming PR request; instead, ensure the request is retried after initialization completes by either queuing the PR or awaiting init completion. Modify handle_create_pr (and add a pending container like self.pending_prs: Vec<(String,String,String)> if needed) so that when !self.pr_state.is_initialized() you push (title, source, target) into the pending queue, kick off initialization if not already running (replace initialize().detach() with a start-if-needed call), and ensure initialization completion logic processes the pending queue (or have initialize() return a Future you await and then continue handling the PR). Reference: handle_create_pr, pr_state, initialize().detach(), and add processing in the pr_state init-completion handler to flush pending PRs.crates/cherrypick_pr/src/service.rs-284-287 (1)
284-287:⚠️ Potential issue | 🟡 MinorMake this test assert a real outcome.
pr.is_ok() || pr.is_err()is tautologically true, so Line 287 can never fail and won't catch branch-resolution regressions. Assert success plus the returned branches/OIDs, or assert the expected error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/service.rs` around lines 284 - 287, The test currently asserts pr.is_ok() || pr.is_err(), which is always true; replace it with a concrete expectation for service.create_pr(&path, "My PR", "feature", "master") — either assert that pr.is_ok(), then unwrap the result from create_pr and assert the returned PR fields (branch names, base branch, and OIDs) match expected values, or assert that pr.is_err() and validate the specific error variant/message; locate the call to create_pr and the pr variable in this test and update the assertion accordingly to validate a real outcome.crates/cherrypick_agent/src/tools/git_tools.rs-68-70 (1)
68-70:⚠️ Potential issue | 🟡 Minor
ListFilesTooldoesn't check sensitive paths for directory listing.While
ReadFileToolblocks sensitive files,ListFilesToolallows listing any directory, potentially revealing the existence and names of sensitive files (e.g.,.env,.ssh/). Consider applyingis_sensitive_pathto the directory path as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/tools/git_tools.rs` around lines 68 - 70, ListFilesTool's async fn execute currently resolves dir_path via resolve_safe_path without checking for sensitive directories; update ListFilesTool::execute to call is_sensitive_path on the resolved full_path (or on dir_path before resolving) and return an error/abort the listing when it is sensitive, similar to ReadFileTool's behavior, so attempts to list paths like ".env" or ".ssh/" are blocked; ensure you use the same error type/result pattern used elsewhere in execute to keep behavior consistent.crates/cherrypick_pr/src/watcher.rs-63-67 (1)
63-67:⚠️ Potential issue | 🟡 Minor
old_oidis alwaysNoneinBranchTipChangedevents.The
old_oidfield is never populated with the previous commit OID. To provide meaningful branch tip change events, consider caching the previous OID per branch (e.g., in aHashMap<String, String>) before reading the new value, then passing the cached value asold_oid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/watcher.rs` around lines 63 - 67, BranchTipChanged events currently always set old_oid to None; cache the latest OID per branch (e.g., maintain a HashMap<String, String> named old_oid_map accessible in the watcher task) and look up the previous value before sending BranchEvent::BranchTipChanged (use the cached value as old_oid and the just-read commit as new_oid), then update the cache with the new OID after sending; ensure the cache is shared safely (Arc<Mutex<...>> or similar) if this runs across tasks/threads and reference the existing BranchEvent::BranchTipChanged construction site (the tx_clone.send call using branch and new_oid) to replace None with the cached previous OID.crates/cherrypick_pr/src/store.rs-158-165 (1)
158-165:⚠️ Potential issue | 🟡 MinorTimestamp parse failures silently default to current time.
When
created_atorupdated_atfails to parse, the code defaults toUtc::now(). This silently corrupts the data timeline - a PR from yesterday would appear to be created now. Consider logging a warning or returning an error.♻️ Proposed fix with logging
created_at: row .get::<_, String>(10)? .parse() - .unwrap_or_else(|_| Utc::now()), + .unwrap_or_else(|e| { + log::warn!("Failed to parse created_at for PR {}: {e}", pr_id); + Utc::now() + }),Also applies to: 347-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/store.rs` around lines 158 - 165, The created_at/updated_at parsing silently falls back to Utc::now() (via unwrap_or_else) which corrupts timestamps; replace the silent fallback with explicit error handling or a logged warning: when parsing the strings obtained by row.get::<_, String>(10/11) for created_at and updated_at fails, return a parsing error (propagate Err) or call log::warn with the row/context (e.g., PR id) and the raw timestamp before deciding a fallback, and apply the same change to the repeated parsing at the other occurrence (the block at lines ~347-354); update the function that maps DB rows to your struct (the codepath doing row.get(...).parse() for created_at/updated_at) to propagate or log parse failures instead of using unwrap_or_else(Utc::now()) so timestamp corruption is avoided.crates/cherrypick_agent/src/chat/mod.rs-177-178 (1)
177-178:⚠️ Potential issue | 🟡 MinorMalformed tool call arguments silently default to empty object.
If the LLM returns invalid JSON in
current_tool_args,serde_json::from_strfails andunwrap_or_default()produces an empty{}. This could cause tool execution to fail unexpectedly or behave incorrectly. Consider logging a warning or emitting an error event.♻️ Proposed fix
StreamChunk::ToolCallEnd => { - let args: serde_json::Value = - serde_json::from_str(¤t_tool_args).unwrap_or_default(); + let args: serde_json::Value = match serde_json::from_str(¤t_tool_args) { + Ok(v) => v, + Err(e) => { + let _ = event_tx.send(AgentEvent::Error(format!( + "Invalid tool arguments for {}: {e}", + current_tool_name + ))); + serde_json::Value::Object(Default::default()) + } + }; tool_calls.push(ToolCall {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/chat/mod.rs` around lines 177 - 178, The code silently swallows JSON parse errors by using serde_json::from_str(¤t_tool_args).unwrap_or_default(), which turns malformed tool args into an empty object; instead, parse with match/Result handling on serde_json::from_str(¤t_tool_args) used to assign args and on Err(log and propagate) — log a warning/error including current_tool_args and the serde_json::Error (e.g., via tracing::warn! or your crate's logger) and either return/emit an error event to the caller or propagate the Err so tool execution can fail fast rather than using the default empty args.
🧹 Nitpick comments (26)
crates/cherrypick_ui/src/diff_file_list.rs (1)
140-155: Consider usingif letpattern instead ofis_none()+unwrap().The current pattern checks
is_none()and then usesunwrap(). While safe, anif let Somepattern is more idiomatic and avoids theunwrap()call.♻️ Suggested refactor
impl Render for DiffFileList { fn render(&mut self, _window: &mut Window, cx: &mut Context<Self>) -> impl IntoElement { - if self.diff.is_none() { - return div() + let Some(diff) = self.diff.as_ref() else { + return div() .id("diff-file-list-empty") .p_2() .text_sm() .text_color(cx.theme().colors().text_muted) .child("No diff to display"); - } + }; - let diff = self.diff.as_ref().unwrap(); let summary = self.render_summary(diff, cx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_ui/src/diff_file_list.rs` around lines 140 - 155, The code checks self.diff.is_none() then later calls self.diff.as_ref().unwrap(), which is safe but unidiomatic; change the block to use an if let Some(diff) = self.diff.as_ref() { ... } else { ... } so you bind diff directly and remove unwrap; move the existing render_summary(diff, cx) and file_entries = diff.files.iter().map(|f| self.render_file_entry(f, cx)).collect() inside the Some arm and keep the empty div() return in the else arm to preserve behavior.crates/cherrypick_ui/src/remote_toolbar.rs (1)
11-17: Unusedworkspacefield.The
workspace: WeakEntity<Workspace>field is stored but never read. If it's intended for future use, consider adding a TODO comment; otherwise, remove it to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_ui/src/remote_toolbar.rs` around lines 11 - 17, The RemoteToolbar struct currently defines a stored but unused field workspace: WeakEntity<Workspace>; either remove this field from the RemoteToolbar definition (and any associated constructor/initialization in new/Default impls) if it's not needed, or retain it and add a clear TODO comment above the field (e.g., "// TODO: keep workspace for future focus/navigation features") and reference it wherever appropriate; update any code that constructs RemoteToolbar (functions/methods that set workspace) to match the change and run cargo check to ensure no remaining references to workspace exist.crates/cherrypick_ui/src/branch_list.rs (1)
13-14:show_localandshow_remotetoggles are never modified.These fields are initialized to
truebut there's no UI or method to toggle them. If this is placeholder state for a future feature, consider adding a TODO comment. Otherwise, remove them and simplifyrender_section_itemsto always render when branches exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_ui/src/branch_list.rs` around lines 13 - 14, The fields show_local and show_remote in the BranchList state are initialized but never changed; either mark them as intentional placeholders or remove them—if they're placeholders, add a clear TODO comment next to the declarations of show_local and show_remote indicating planned UI toggles and keep existing render_section_items logic; otherwise delete show_local and show_remote from the struct and update render_section_items to unconditionally render sections when branches exist (remove any checks that reference show_local/show_remote and simplify conditional branches in render_section_items and any callers such as render or update methods to rely only on branch existence).crates/cherrypick_ui/src/staging_view.rs (1)
50-52: Unused fields:commit_messageand_subscriptions.Both fields are initialized but never used. If these are placeholders for future functionality, consider adding a
// TODOcomment or removing them to reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_ui/src/staging_view.rs` around lines 50 - 52, The struct field declarations commit_message and _subscriptions in staging_view.rs are unused; either remove these fields from the struct or mark intent with a TODO comment and/or convert them to underscore-prefixed names to indicate deliberate unused placeholders. Locate the struct (fields commit_message: String and _subscriptions: Vec<Subscription>), then either delete those two fields if not needed or add a clear TODO comment above each (e.g., "// TODO: will be used for X") or rename commit_message to _commit_message to silence unused warnings while keeping intent.crates/cherrypick_agent/src/keys.rs (2)
65-71:EnvVarBackend::setanddeleteare silent no-ops.These methods succeed without actually modifying environment variables, which could mislead callers into thinking keys were persisted. Consider returning an error or documenting this limitation.
📝 Document the limitation or return an error
+ /// Note: Environment variables cannot be modified at runtime. + /// This method is a no-op and always succeeds. fn set(&self, _key: &str, _value: &str) -> Result<()> { Ok(()) } + /// Note: Environment variables cannot be modified at runtime. + /// This method is a no-op and always succeeds. fn delete(&self, _key: &str) -> Result<()> { Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/keys.rs` around lines 65 - 71, EnvVarBackend::set and EnvVarBackend::delete are currently silent no-ops; update them to actually modify the process environment using std::env::set_var(key, value) in set and std::env::remove_var(key) in delete, then return Ok(()), and ensure errors (if you use fallible APIs) are mapped into the crate's Result type; include a short doc comment on EnvVarBackend noting this only affects the current process environment (not persistent system-wide storage).
86-102: Mutex.unwrap()can panic on poisoned lock.If a thread panics while holding the mutex, subsequent
.unwrap()calls will panic. Consider using.lock().ok()or.lock().unwrap_or_else(|e| e.into_inner())to handle poisoned locks gracefully.🔒 Suggested fix
impl KeyBackend for InMemoryBackend { fn get(&self, key: &str) -> Result<Option<String>> { - Ok(self.data.lock().unwrap().get(key).cloned()) + Ok(self.data.lock().unwrap_or_else(|e| e.into_inner()).get(key).cloned()) } fn set(&self, key: &str, value: &str) -> Result<()> { self.data .lock() - .unwrap() + .unwrap_or_else(|e| e.into_inner()) .insert(key.to_string(), value.to_string()); Ok(()) } fn delete(&self, key: &str) -> Result<()> { - self.data.lock().unwrap().remove(key); + self.data.lock().unwrap_or_else(|e| e.into_inner()).remove(key); Ok(()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/keys.rs` around lines 86 - 102, The impl of KeyBackend for InMemoryBackend uses self.data.lock().unwrap() in get/set/delete which can panic on a poisoned mutex; replace those unwraps with a poisoned-lock recovery such as using .lock().unwrap_or_else(|e| e.into_inner()) (or equivalent handling) in the get, set, and delete methods so the code acquires the mutex guard even if poisoned and proceeds to call get/insert/remove on the underlying map.crates/cherrypick_agent/src/provider/anthropic.rs (1)
139-142: Rate limit retry_after is hardcoded instead of parsing the Retry-After header.The 429 handling hardcodes
retry_after_secs: 60. Anthropic's API returns aRetry-Afterheader with the actual backoff duration. Parse and use that value for more responsive rate limit handling.💡 Suggested fix
if status.as_u16() == 429 { + let retry_after = response + .headers() + .get("retry-after") + .and_then(|v| v.to_str().ok()) + .and_then(|s| s.parse::<u64>().ok()) + .unwrap_or(60); return Err(AgentError::RateLimited { - retry_after_secs: 60, + retry_after_secs: retry_after, }); }Note: You'll need to read the header before consuming the response body with
.text().await.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/provider/anthropic.rs` around lines 139 - 142, When handling HTTP 429 in the Anthropic client (the block that returns AgentError::RateLimited), read the response's "Retry-After" header before consuming the body and parse it to determine retry_after_secs: if the header is a plain integer, use that value; if it's an HTTP-date, compute seconds until that date; if missing or parsing fails, fall back to 60 seconds. Update the code around the status check that currently hardcodes retry_after_secs: 60 to extract and parse resp.headers().get("Retry-After") and pass the computed seconds into AgentError::RateLimited.crates/cherrypick_ui/src/pr_detail.rs (1)
8-8: Unused import:PrStateThe
PrStateimport is not referenced anywhere in this file.🧹 Suggested fix
-use crate::pr_state::PrState;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_ui/src/pr_detail.rs` at line 8, Remove the unused import PrState from pr_detail.rs: the statement `use crate::pr_state::PrState;` is not referenced in this file, so delete that line to eliminate the unused import warning; if PrState is intended to be used later, instead ensure any functions or structs in pr_detail.rs reference PrState explicitly (e.g., in function signatures or struct fields) before keeping the import.crates/cherrypick_agent/src/context/mod.rs (2)
58-60: Token estimation heuristic is very rough.
len / 4is a common approximation for English text but can be inaccurate for code, non-Latin scripts, or structured content. This may cause unexpected truncation behavior. Consider documenting this limitation or using a more accurate tokenizer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/context/mod.rs` around lines 58 - 60, The token estimation in function estimate_tokens (pub fn estimate_tokens) uses a fixed text.len() / 4 heuristic which is unreliable for code, non‑Latin scripts, or structured content; update this by either replacing the heuristic with a real tokenizer (e.g., integrate a tokenizer crate such as tiktoken_rs or another model-specific tokenizer and call it from estimate_tokens) or, if adding a tokenizer is out of scope, add a clear doc comment on estimate_tokens documenting the heuristic's limitations and the risk of incorrect truncation, and consider exposing a configurable multiplier constant so callers can tune the heuristic.
10-27: Budget tier fields (hot,warm,cold) are computed but appear unused.
ContextEnginestores aContextBudgetbut only exposes it viabudget(). The tier values aren't used for any logic in this module. If these are planned for future use, consider adding a// TODOcomment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/context/mod.rs` around lines 10 - 27, ContextBudget defines tier fields hot/warm/cold that are computed but never used; update the code to either remove these unused fields or annotate them with a clear TODO explaining planned future use so linters/reviewers understand intent. Locate the ContextBudget struct and its impl::new (and any usages like ContextEngine::budget()) and either delete hot/warm/cold from the struct and adjust new() to only set total, or add a comment above each field or above the struct (e.g., "// TODO: reserved for future per-tier eviction/placement logic") to document why they are present.crates/cherrypick_ui/src/create_pr_form.rs (1)
147-257: PR title is not editable in the UI.The form stores a
titlefield (initialized from source branch name on line 77-79), but the rendered UI doesn't include a text input for editing it. Users cannot customize the PR title before creation.💡 Consider adding a title input
Add a text input element between the header and branch pickers to allow users to customize the PR title before submission.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_ui/src/create_pr_form.rs` around lines 147 - 257, The UI is missing an editable PR title input in CreatePrForm::render: insert a text input element (e.g., div().child(input()...)) between the "Create Local PR" header child and the branch pickers that binds to the form's title field (self.title) so users can edit it; use an on_input listener to call a setter (e.g., this.set_title(cx, new_value) or update this.title directly) and give the input an id like "create-pr-title" and a placeholder (e.g., source branch name) so submit (this.submit) will use the updated title when creating the PR.crates/cherrypick_ui/src/cherrypick_sidebar.rs (1)
233-235: Failed PR creation errors are logged but not surfaced to the user.When PR creation fails, the error is only logged. Consider showing a toast notification or inline error message to inform the user.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_ui/src/cherrypick_sidebar.rs` around lines 233 - 235, The Err(e) branch that currently only calls log::error!("cherrypick: failed to create PR: {}", e) should also surface the failure to the user UI; update the PR-creation error handling to display a toast or inline error message (e.g., call your existing toast/notification helper or emit a UI event) with a concise message including e.to_string(), so users see "Failed to create PR: <error>". Locate the error branch around the PR creation code (the log::error! call) and add a UI notification call (or send a message to the component state) so the client shows the error to the user in addition to logging it.crates/cherrypick_pr/src/diff_service.rs (1)
122-136: Useis_zero()method for null OID detection.The Rust
git2crate provides anis_zero()method onOidspecifically for this check. Replace the manual byte inspection withnew_file.id().is_zero()for clarity and correctness.💡 Suggested fix
- let is_lfs = if !is_binary { - if let Some(oid) = new_file.id().as_bytes().first() { - if *oid != 0 { + let is_lfs = if !is_binary && !new_file.id().is_zero() { let blob = repo.find_blob(new_file.id()); blob.map(|b| FileContent::is_lfs_pointer(b.content())) .unwrap_or(false) - } else { - false - } - } else { - false - } } else { false };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/diff_service.rs` around lines 122 - 136, The is_lfs detection currently inspects the first byte of new_file.id().as_bytes() to detect a null OID; replace that manual byte check with the git2-provided Oid::is_zero() by calling new_file.id().is_zero() when deciding whether to call repo.find_blob and FileContent::is_lfs_pointer; update the is_lfs block (the variable computed from new_file.id(), repo.find_blob, and FileContent::is_lfs_pointer) to use new_file.id().is_zero() for the null OID check and keep the rest of the logic the same.crates/cherrypick_pr/src/types.rs (2)
80-89:BranchHealth::default()setsexists: truewhich may be misleading.Defaulting
existstotruecould mask cases where branch existence wasn't actually checked. Consider whetherfalsewould be a safer default, requiring explicit confirmation of existence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/types.rs` around lines 80 - 89, BranchHealth::default() currently sets exists: true; change the Default impl in BranchHealth::default to set exists: false so the default represents "not confirmed" rather than present, and update any callers or tests that relied on the previous default to explicitly set exists when appropriate (refer to the Default impl for BranchHealth and any places constructing BranchHealth via Default or derived Default).
22-29: Consider implementingstd::str::FromStrtrait instead of customfrom_strmethods.The custom
from_strmethods shadow the standard trait, which prevents using.parse()on strings. ImplementingFromStrwould provide better ergonomics and trait-based interoperability.♻️ Example for PrStatus
+impl std::str::FromStr for PrStatus { + type Err = (); + + fn from_str(s: &str) -> std::result::Result<Self, Self::Err> { + match s { + "open" => Ok(Self::Open), + "merged" => Ok(Self::Merged), + "closed" => Ok(Self::Closed), + _ => Err(()), + } + } +}Also applies to: 46-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/types.rs` around lines 22 - 29, The file defines a custom from_str(&str) -> Option<Self> (e.g., the PrStatus::from_str function) which shadows the std trait and prevents using .parse(); implement std::str::FromStr for the enum(s) (e.g., impl FromStr for PrStatus and the other enum at lines 46-52) returning Result<Self, ParseErrorType> (you can use std::string::String or a custom error type) and move the matching logic there, then update or keep the existing from_str helper to delegate to str::parse if desired so callers can use "s".parse::<PrStatus>().crates/cherrypick_pr/src/merge_service.rs (2)
111-113: No validation that resolved paths are inconflicted_paths.
resolve_conflictallows inserting resolutions for arbitrary paths, even those not inconflicted_paths. This could lead to unexpected behavior if a caller resolves a non-conflicted file. Consider validating thatpathexists insession.conflicted_paths.♻️ Proposed fix
- pub fn resolve_conflict(session: &mut MergeSession, path: &str, content: Vec<u8>) { - session.resolutions.insert(path.to_string(), content); + pub fn resolve_conflict(session: &mut MergeSession, path: &str, content: Vec<u8>) -> bool { + if session.conflicted_paths.contains(&path.to_string()) { + session.resolutions.insert(path.to_string(), content); + true + } else { + false + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/merge_service.rs` around lines 111 - 113, The resolve_conflict function currently inserts resolutions for any path; update it to validate that the provided path exists in session.conflicted_paths before inserting into session.resolutions. Change resolve_conflict (and its callers) to return a Result (or bool) indicating success/failure, and if the path is not found return an Err (or false) rather than inserting; reference the MergeSession type and its conflicted_paths and resolutions fields so callers can handle invalid-path attempts appropriately.
32-32: Unused variablemerge_base_oid.The
merge_base_oidis computed but never used incheck_conflicts. Either remove it or use it if it was intended for something.♻️ Proposed fix
- let merge_base_oid = repo.merge_base(source.id(), target.id())?; - let mut merge_opts = git2::MergeOptions::new();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/merge_service.rs` at line 32, The local variable merge_base_oid returned from repo.merge_base(source.id(), target.id()) in check_conflicts is computed but never used; remove the unused assignment or explicitly ignore it to silence the warning—e.g., call repo.merge_base(...)?; without assigning the result or assign to _merge_base_oid/_ to indicate it's intentionally unused; locate the call to repo.merge_base in the check_conflicts function and either delete the let merge_base_oid = ... line or change it to let _ = repo.merge_base(source.id(), target.id())?;crates/cherrypick_pr/src/watcher.rs (4)
118-143:detect_force_pushreturnsfalseon all errors, masking failures.The function silently returns
falsefor repository discovery failures, invalid OIDs, andgraph_descendant_oferrors. This makes it impossible for callers to distinguish "not a force push" from "error occurred." Consider returningResult<bool>orOption<bool>to surface errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/watcher.rs` around lines 118 - 143, The detect_force_push function currently swallows all errors; change its signature from fn detect_force_push(...) -> bool to return a Result<bool, E> (or Option<bool>) so callers can distinguish errors from "not a force push"; use git2::Repository::discover(...) and git2::Oid::from_str(...) with ? (or map_err) to propagate errors instead of returning false, and propagate the Result from repo.graph_descendant_of(new, old) rather than using unwrap_or(true) so any git errors are returned to the caller (referencing detect_force_push, git2::Repository::discover, git2::Oid::from_str, and repo.graph_descendant_of).
39-40: Duplicaterefs_dircomputation.
refs_diris computed identically on line 39 and again on line 82. Extract this path computation once to avoid redundancy.♻️ Proposed fix
let refs_dir = git_dir.join("refs").join("heads"); let head_file = git_dir.join("HEAD"); let tx_clone = tx.clone(); + let refs_dir_clone = refs_dir.clone(); let watcher_result = notify::recommended_watcher(move |res: std::result::Result<Event, notify::Error>| { match res { Ok(event) => { if matches!( event.kind, EventKind::Modify(_) | EventKind::Create(_) | EventKind::Remove(_) ) { for path in &event.paths { - if let Some(branch) = extract_branch_name(path, &refs_dir) { + if let Some(branch) = extract_branch_name(path, &refs_dir_clone) {Then on lines 82-84:
let watcher = match watcher_result { Ok(mut w) => { - let refs_path = git_dir.join("refs").join("heads"); - if refs_path.exists() { - let _ = w.watch(&refs_path, RecursiveMode::Recursive); + if refs_dir.exists() { + let _ = w.watch(&refs_dir, RecursiveMode::Recursive); }Also applies to: 82-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/watcher.rs` around lines 39 - 40, Refactor the duplicated refs_dir computation by computing it once from git_dir (e.g., let refs_dir = git_dir.join("refs").join("heads");) and reusing that single refs_dir variable wherever currently recomputed (locations around the refs_dir declarations in watcher.rs, including the usages near the earlier head_file and later code around lines where refs_dir is recomputed). Remove the second identical assignment and update any subsequent code to reference the extracted refs_dir variable to avoid redundancy.
177-187: Test coverage gap fordetect_force_pushwith valid divergent commits.The tests cover nonexistent repos and same-OID cases, but there's no test verifying the core logic: detecting when
new_oidis not a descendant ofold_oid(actual force push detection). Consider adding a test that creates two divergent commits and verifies the function correctly identifies a force push.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/watcher.rs` around lines 177 - 187, Add a unit test for detect_force_push that creates a repo with two divergent commits and asserts detection returns true: initialize a temp repo (tempfile::tempdir), create an initial commit and record its OID (old_oid), then create a new commit on a different root/branch that does not descend from old_oid (new_oid) by making a separate tree/blob and commit via git2::Repository::commit, and finally assert detect_force_push(tmp.path(), &old_oid.to_string(), &new_oid.to_string()) is true; reference detect_force_push and use git2::Repository, Oid, and commit creation helpers to implement this.
84-86: Silently ignoringwatch()errors.Both
w.watch()calls discard their results withlet _ = .... If watching fails (e.g., due to permissions or inotify limits), the watcher will silently not monitor the intended paths. Consider sending aWatcherErrorevent or logging on failure.♻️ Proposed fix
if refs_path.exists() { - let _ = w.watch(&refs_path, RecursiveMode::Recursive); + if let Err(e) = w.watch(&refs_path, RecursiveMode::Recursive) { + let _ = tx.send(BranchEvent::WatcherError(format!( + "Failed to watch refs/heads: {e}" + ))); + } } - let _ = w.watch(&head_file, RecursiveMode::NonRecursive); + if let Err(e) = w.watch(&head_file, RecursiveMode::NonRecursive) { + let _ = tx.send(BranchEvent::WatcherError(format!( + "Failed to watch HEAD: {e}" + ))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/watcher.rs` around lines 84 - 86, The two calls to w.watch(&refs_path, RecursiveMode::Recursive) and w.watch(&head_file, RecursiveMode::NonRecursive) currently ignore Result values; change them to handle Err by logging the error and/or sending a watcher error into the existing watcher event channel (e.g., construct and send a WatcherError event) instead of using let _ = ...; implement this by matching the Result from w.watch(...) and on Err call the existing logger (or tx.send(WatcherEvent::WatcherError(err.into()))/similar) so failures to register watches are visible and handled.crates/cherrypick_agent/src/tools/git_tools.rs (2)
303-357: Unbounded recursion insearch_recursivecould cause stack overflow.The recursive directory traversal has no depth limit. A repository with deeply nested directories (e.g., symlink loops if the symlink check fails, or just very deep nesting) could exhaust the stack. Consider adding a max depth parameter or converting to iterative traversal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/tools/git_tools.rs` around lines 303 - 357, The search_recursive function lacks a depth limit and can overflow the stack; modify search_recursive to accept a max_depth (and a current_depth or depth_remaining) parameter, check the depth at the top of the function and return Ok(()) when the depth limit is reached, and increment the depth when recursing (i.e., call search_recursive(&path, pattern, repo_root, results, max_results, current_depth + 1) or decrement depth_remaining on each recursive call); update all callers to pass the initial depth (0 or the configured limit) and ensure behavior and early returns (max_results) remain unchanged—this adds a simple max-depth guard without changing the file traversal logic.
40-41: Blocking filesystem operations in async context.
std::fs::read_to_string,std::fs::read_dir, and similar operations block the async runtime thread. For a tool executor that may handle multiple concurrent requests, consider usingtokio::fsor spawning blocking work viatokio::task::spawn_blocking.♻️ Example for ReadFileTool
- std::fs::read_to_string(&full_path) - .map_err(|e| AgentError::ToolExecution(format!("Failed to read file: {e}"))) + tokio::fs::read_to_string(&full_path) + .await + .map_err(|e| AgentError::ToolExecution(format!("Failed to read file: {e}")))Also applies to: 68-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/tools/git_tools.rs` around lines 40 - 41, The code uses blocking std::fs operations (e.g., std::fs::read_to_string(&full_path)) inside async/tool executor code which can block the async runtime; replace these with async equivalents or offload to a blocking task: either call tokio::fs::read_to_string(&full_path).await and propagate errors into AgentError::ToolExecution, or wrap the blocking call in tokio::task::spawn_blocking(|| std::fs::read_to_string(full_path)) and .await the JoinHandle, doing the same error mapping; apply the same change to the other blocking usages referenced (e.g., read_dir/read_to_string calls around lines 68-86).crates/cherrypick_agent/src/chat/mod.rs (2)
194-194:JoinErrorfrom provider task is silently discarded.If the spawned provider task panics,
provider_handle.awaitreturnsErr(JoinError)which is ignored. Consider handling this to surface task panics.♻️ Proposed fix
- let _ = provider_handle.await; + if let Err(e) = provider_handle.await { + let _ = event_tx.send(AgentEvent::Error(format!("Provider task failed: {e}"))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/chat/mod.rs` at line 194, The JoinError from awaiting the spawned provider task is currently ignored at the call to provider_handle.await; change this to handle the Result by matching or using if let to detect Err(e) and surface it (log the error with context or propagate the panic), e.g. replace the discard with a check like if let Err(e) = provider_handle.await { /* log e with context or re-panic/resume_unwind */ } so task panics are not silently swallowed; reference the provider_handle.await and handle the JoinError returned by it.
149-192: No timeout on chunk reception; agent can hang indefinitely.The
while let Some(chunk) = chunk_rx.recv().awaitloop has no timeout. If the provider stops sending chunks without emittingDoneorError, the agent will hang forever. Consider usingtokio::time::timeoutaround the receive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_agent/src/chat/mod.rs` around lines 149 - 192, The receive loop on chunk_rx (while let Some(chunk) = chunk_rx.recv().await) lacks a timeout and can hang; wrap the recv() call with tokio::time::timeout (e.g., timeout(Duration::from_secs(N), chunk_rx.recv()).await), handle the timeout branch by sending an AgentEvent::Error (via event_tx.send(...)) and returning an appropriate AgentError (either add/return AgentError::Timeout or return AgentError::Provider with a timeout message), and ensure you still check the cancel flag (cancel.borrow()) between attempts; keep existing handling for StreamChunk::Done and StreamChunk::Error unchanged (preserve ToolCall assembly using current_tool_id/current_tool_args and risk lookup via self.tool_executor.risk_level).crates/cherrypick_pr/src/store.rs (1)
88-103:ensure_repocould be simplified to single SQL statement for clarity, though current implementation works correctly.The separate
INSERT OR IGNOREfollowed bySELECTstatements create a non-atomic sequence. While the UNIQUE constraint on(first_commit_oid, remote_urls_hash)ensures both concurrent calls will consistently retrieve the same ID, consolidating to a single statement improves clarity and reduces database round-trips.♻️ Alternative using RETURNING (SQLite 3.35+)
.call(move |conn| { - conn.execute( - "INSERT OR IGNORE INTO repos (first_commit_oid, remote_urls_hash, canonical_path) - VALUES (?1, ?2, ?3)", - params![fco, ruh, cp], - )?; - let id: i64 = conn.query_row( - "SELECT id FROM repos WHERE first_commit_oid = ?1 AND remote_urls_hash = ?2", - params![fco, ruh], + let id: i64 = conn.query_row( + "INSERT INTO repos (first_commit_oid, remote_urls_hash, canonical_path) + VALUES (?1, ?2, ?3) + ON CONFLICT(first_commit_oid, remote_urls_hash) DO UPDATE SET canonical_path = canonical_path + RETURNING id", + params![fco, ruh, cp], |row| row.get(0), )?; Ok(id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cherrypick_pr/src/store.rs` around lines 88 - 103, The current ensure_repo implementation uses separate INSERT OR IGNORE then SELECT, causing an extra round-trip; simplify it in the conn.call closure by replacing the two-statement sequence with a single SQL that returns the id via RETURNING, e.g. run "INSERT INTO repos (first_commit_oid, remote_urls_hash, canonical_path) VALUES (?1, ?2, ?3) ON CONFLICT(first_commit_oid, remote_urls_hash) DO UPDATE SET canonical_path=repos.canonical_path RETURNING id" (so conflicts don't change data but still return the existing row id) and use conn.query_row to fetch that returned id instead of the separate SELECT; update the closure in ensure_repo to bind fco/ruh/cp and map the returned row to Ok(id) and propagate errors as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5884120-d455-4e4d-bc22-c9cb73b8b4ea
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
Cargo.tomlcrates/cherrypick_agent/Cargo.tomlcrates/cherrypick_agent/src/chat/mod.rscrates/cherrypick_agent/src/chat/store.rscrates/cherrypick_agent/src/context/mod.rscrates/cherrypick_agent/src/error.rscrates/cherrypick_agent/src/keys.rscrates/cherrypick_agent/src/lib.rscrates/cherrypick_agent/src/mcp.rscrates/cherrypick_agent/src/provider/anthropic.rscrates/cherrypick_agent/src/provider/mod.rscrates/cherrypick_agent/src/provider/types.rscrates/cherrypick_agent/src/skills/mod.rscrates/cherrypick_agent/src/tools/git_tools.rscrates/cherrypick_agent/src/tools/mod.rscrates/cherrypick_agent/src/tools/safe_path.rscrates/cherrypick_pr/Cargo.tomlcrates/cherrypick_pr/src/diff_service.rscrates/cherrypick_pr/src/error.rscrates/cherrypick_pr/src/lib.rscrates/cherrypick_pr/src/merge_service.rscrates/cherrypick_pr/src/service.rscrates/cherrypick_pr/src/store.rscrates/cherrypick_pr/src/types.rscrates/cherrypick_pr/src/watcher.rscrates/cherrypick_ui/Cargo.tomlcrates/cherrypick_ui/src/branch_list.rscrates/cherrypick_ui/src/cherrypick_sidebar.rscrates/cherrypick_ui/src/cherrypick_view.rscrates/cherrypick_ui/src/commit_graph_embed.rscrates/cherrypick_ui/src/create_pr_form.rscrates/cherrypick_ui/src/diff_file_list.rscrates/cherrypick_ui/src/lib.rscrates/cherrypick_ui/src/pr_detail.rscrates/cherrypick_ui/src/pr_list.rscrates/cherrypick_ui/src/pr_state.rscrates/cherrypick_ui/src/remote_toolbar.rscrates/cherrypick_ui/src/repo_state_banner.rscrates/cherrypick_ui/src/settings.rscrates/cherrypick_ui/src/staging_view.rscrates/cherrypick_ui/src/stash_list.rscrates/cherrypick_ui/src/worktree_list.rscrates/zed/Cargo.tomlcrates/zed/src/main.rscrates/zed/src/zed.rs
Comment on lines
+228
to
+241
| if risk.requires_confirmation() { | ||
| let preview = self | ||
| .tool_executor | ||
| .preview(tc) | ||
| .unwrap_or_else(|| tc.name.clone()); | ||
| let _ = event_tx.send(AgentEvent::ConfirmationNeeded { | ||
| tool_name: tc.name.clone(), | ||
| risk_level: risk, | ||
| preview, | ||
| }); | ||
| // For now, auto-approve in the agentic loop. | ||
| // In the full UI implementation, this would wait for user input | ||
| // via a oneshot channel before proceeding. | ||
| } |
There was a problem hiding this comment.
High-risk tool operations are auto-approved despite confirmation event.
The code emits ConfirmationNeeded for destructive operations but immediately proceeds to execute them (lines 243+). The comment acknowledges this is a placeholder, but this defeats the safety rules stated in SYSTEM_POLICY. Users expecting confirmation before force-push or branch deletion will not get it.
Would you like me to help implement a proper confirmation gate using a oneshot channel that blocks until user approval is received?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cherrypick_agent/src/chat/mod.rs` around lines 228 - 241, The current
logic emits AgentEvent::ConfirmationNeeded but then continues without waiting;
change it to create a tokio::sync::oneshot channel, include the oneshot::Sender
in the ConfirmationNeeded event (or a separate ConfirmationRequest struct) when
sending via event_tx.send(...), then await the oneshot::Receiver (e.g.,
receiver.await) before proceeding with executing the risky tool; handle the
receiver result (Ok(approved) => continue, Ok(false) or Err(_) => skip/abort the
operation) and consider a timeout fallback to deny by default. Ensure the
symbols referenced are risk.requires_confirmation(),
self.tool_executor.preview(tc), event_tx.send(AgentEvent::ConfirmationNeeded {
... }) and use oneshot sender/receiver to block until explicit approval is
returned.
Comment on lines
+95
to
+100
| Some(ToolDefinition { | ||
| name: format!( | ||
| "mcp_{}__{}", | ||
| self.config.name, | ||
| t.get("name")?.as_str()? | ||
| ), |
There was a problem hiding this comment.
Strip the client-side namespace before tools/call.
discover_tools() rewrites each tool name to mcp_<server>__<tool>, but call_tool() forwards tool_name unchanged. If the caller passes one of the discovered names back in, the server will see an unknown tool instead of the raw MCP name it advertised.
Also applies to: 124-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cherrypick_agent/src/mcp.rs` around lines 95 - 100, discover_tools()
currently rewrites advertised names to "mcp_<server>__<tool>" (ToolDefinition
name), but call_tool() forwards the client-side names as-is, causing the server
to see unknown names; update call_tool() (and any other forwarders around the
block mirrored at the later section) to strip the client-side namespace prefix
"mcp_<server>__" before sending to the server so the original raw MCP tool name
is used, e.g., detect and remove the "mcp_<server>__" prefix from the incoming
tool_name and forward the remainder; keep the prefix logic centralized so
discover_tools() still advertises rewritten names while call_tool() converts
them back.
Comment on lines
+153
to
+155
| let text = response.text().await?; | ||
|
|
||
| for line in text.lines() { |
There was a problem hiding this comment.
Critical: Response is fully buffered before SSE parsing, defeating streaming.
response.text().await consumes the entire response body into memory before the SSE parsing loop begins. This negates the purpose of streaming and will cause memory issues with large responses. Use response.bytes_stream() or similar to process chunks as they arrive.
🐛 Suggested streaming approach
- let text = response.text().await?;
-
- for line in text.lines() {
+ use futures::StreamExt;
+ let mut stream = response.bytes_stream();
+ let mut buffer = String::new();
+
+ while let Some(chunk) = stream.next().await {
+ let chunk = chunk?;
+ buffer.push_str(&String::from_utf8_lossy(&chunk));
+
+ while let Some(pos) = buffer.find('\n') {
+ let line = buffer[..pos].trim().to_string();
+ buffer = buffer[pos + 1..].to_string();
+
+ if line.is_empty() || line.starts_with(':') {
+ continue;
+ }
+ // ... rest of parsing logic
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cherrypick_agent/src/provider/anthropic.rs` around lines 153 - 155,
The code currently calls response.text().await and iterates over text.lines(),
which buffers the entire response and breaks streaming; change the logic in the
Anthropc provider where response.text().await and text.lines() are used to
instead consume response.bytes_stream() (or the async chunk stream from your
HTTP client) and parse SSE events as chunks arrive, feeding each chunk into your
SSE parser/accumulator and handling complete events incrementally; update the
function handling the response (the block using response.text().await and the
for loop over text.lines()) to read from response.bytes_stream(), handle partial
lines across chunks, and emit parsed SSE messages as they are completed to
preserve streaming and avoid buffering the whole body.
Comment on lines
+76
to
+88
| pub fn load_from_directory(&mut self, dir: &Path) -> Result<usize> { | ||
| let mut count = 0; | ||
| let entries = std::fs::read_dir(dir).map_err(|e| AgentError::Io(e))?; | ||
|
|
||
| for entry in entries { | ||
| let entry = entry.map_err(|e| AgentError::Io(e))?; | ||
| let path = entry.path(); | ||
|
|
||
| if path.extension().and_then(|e| e.to_str()) == Some("md") { | ||
| if let Ok(skill) = parse_skill_file(&path) { | ||
| self.skills.insert(skill.name.clone(), skill); | ||
| count += 1; | ||
| } |
There was a problem hiding this comment.
Validate and reject conflicting skill files.
The current load path silently skips failing files, overwrites duplicate names, and leaves duplicate triggers to HashMap iteration in resolve_slash_command(). A bad skill pack can therefore change which prompt executes without surfacing any error.
Also applies to: 99-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cherrypick_agent/src/skills/mod.rs` around lines 76 - 88,
load_from_directory currently skips parse failures, overwrites duplicate skill
names in self.skills and allows duplicate triggers which later causes
non-deterministic behavior in resolve_slash_command; change load_from_directory
to fail-fast and validate conflicts: collect parsed Skill objects (from
parse_skill_file) first and if any parse error occurs return Err, then detect
and reject duplicate skill.name values and duplicate trigger strings across all
parsed skills (do not insert or overwrite into self.skills), returning an
AgentError with context about the conflicting file/name/trigger so the caller
can surface the error; apply the same validation logic to the code paths around
the later insert loop (the duplicate handling at lines ~99-104) so no duplicate
names or triggers are ever silently accepted.
Comment on lines
+392
to
+398
| for path in &paths { | ||
| if super::is_sensitive_path(path) { | ||
| return Err(AgentError::ToolExecution(format!("Cannot stage sensitive file: {path}"))); | ||
| } | ||
| index.add_path(Path::new(path)) | ||
| .map_err(|e| AgentError::ToolExecution(format!("Failed to stage {path}: {e}")))?; | ||
| } |
There was a problem hiding this comment.
StageFilesTool bypasses safe path resolution.
Unlike ReadFileTool which uses resolve_safe_path, StageFilesTool only checks is_sensitive_path but doesn't validate that paths are within the repository. A malicious LLM could potentially stage files outside the repo using path traversal (e.g., ../../../somewhere). Use resolve_safe_path for each path before staging.
🔒 Proposed fix
for path in &paths {
if super::is_sensitive_path(path) {
return Err(AgentError::ToolExecution(format!("Cannot stage sensitive file: {path}")));
}
- index.add_path(Path::new(path))
+ // Validate path is within repo before staging
+ let _ = resolve_safe_path(repo_path, path)?;
+ index.add_path(Path::new(path))
.map_err(|e| AgentError::ToolExecution(format!("Failed to stage {path}: {e}")))?;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cherrypick_agent/src/tools/git_tools.rs` around lines 392 - 398, The
loop in StageFilesTool currently only checks is_sensitive_path and then calls
index.add_path with the raw path, allowing path traversal; before staging each
entry call resolve_safe_path(path) and use the returned safe PathBuf (or return
an AgentError if resolution fails) and then pass that resolved path to
index.add_path; also ensure you run is_sensitive_path against the resolved path
(or after resolution) and update error messages to include the resolved path and
the underlying error when index.add_path fails (refer to functions
resolve_safe_path, is_sensitive_path, and index.add_path in git_tools.rs).
Comment on lines
+146
to
+170
| fn show_pr(&mut self, pr: LocalPr, cx: &mut Context<Self>) { | ||
| let source_oid = pr.source_oid.clone(); | ||
| let target_oid = pr.target_oid.clone(); | ||
| self.mode = ViewMode::PrDetail(pr); | ||
| self.load_diff(source_oid, target_oid, cx); | ||
| cx.emit(CherryPickViewEvent::UpdateTab); | ||
| cx.notify(); | ||
| } | ||
|
|
||
| fn show_compare( | ||
| &mut self, | ||
| source_branch: String, | ||
| target_branch: String, | ||
| source_oid: String, | ||
| target_oid: String, | ||
| cx: &mut Context<Self>, | ||
| ) { | ||
| self.mode = ViewMode::BranchCompare { | ||
| source_branch, | ||
| target_branch, | ||
| source_oid: source_oid.clone(), | ||
| target_oid: target_oid.clone(), | ||
| }; | ||
| self.load_diff(source_oid, target_oid, cx); | ||
| cx.emit(CherryPickViewEvent::UpdateTab); |
There was a problem hiding this comment.
Ignore stale diff loads when the selection changes.
show_pr() / show_compare() kick off detached work without clearing the current diff or tagging the request. If the user switches PRs or repositories quickly, an older task can finish last and overwrite diff_file_list / unified_diff_text with the wrong comparison.
Also applies to: 174-192, 195-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cherrypick_ui/src/cherrypick_view.rs` around lines 146 - 170, The
PR/compare handlers (show_pr, show_compare and the other similar handlers) start
asynchronous load_diff tasks but don't clear or tag the current diff state, so a
stale async result can overwrite diff_file_list/unified_diff_text when the
selection changes; fix by adding a per-request nonce or request_id field on the
view struct that you increment each time you call load_diff, clear the current
diff state (set diff_file_list and unified_diff_text to empty) before kicking
off the task, and have the async completion callback compare the captured
request_id/nonce to the current one and only apply results when they match (do
the same tagging in the other similar methods that call load_diff).
Comment on lines
+44
to
+58
| div() | ||
| .id(SharedString::from(format!("pr-{}", pr_id))) | ||
| .flex() | ||
| .flex_col() | ||
| .gap(px(2.0)) | ||
| .px_2() | ||
| .py_1() | ||
| .rounded_sm() | ||
| .cursor_pointer() | ||
| .hover(|style| style.bg(cx.theme().colors().ghost_element_hover)) | ||
| .active(|style| style.bg(cx.theme().colors().ghost_element_active)) | ||
| .on_click(cx.listener(move |this, _event, _window, cx| { | ||
| log::info!("cherrypick: PR clicked: id={}", pr_clone.id); | ||
| cx.emit(PrListEvent::Selected(pr_clone.clone())); | ||
| })) |
There was a problem hiding this comment.
PR row selection is mouse-only; keyboard users cannot trigger selection.
On Line 55, selection is bound only to click on a div, with no per-row keyboard activation path. Please add keyboard-operable interaction (e.g., focusable row semantics + Enter/Space activation) so PR selection is accessible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cherrypick_ui/src/pr_list.rs` around lines 44 - 58, The PR row is only
selectable via mouse because the div built for each row only registers an
on_click handler; make the row keyboard-accessible by making the row focusable
and invoking the same selection logic on Enter/Space key activation. Update the
div builder (the element with id(format!("pr-{}", pr_id))) to include
focusability (e.g., set a tabindex or call the framework's tab_index/focusable
API), add appropriate semantic role/focus styles, and add an on_key_down or
on_key_press handler that checks for Enter or Space and calls the same code path
that emits PrListEvent::Selected(pr_clone.clone()) (reuse the same closure body
or factor selection into a small helper function invoked by both on_click and
the key handler).
Comment on lines
+27
to
+29
| pub fn set_repo_path(&mut self, path: PathBuf) { | ||
| self.repo_path = Some(path); | ||
| } |
There was a problem hiding this comment.
Version initialize() so an old repo load can't win the race.
set_repo_path() only changes the path; the previous store and repo_id stay live until the detached init finishes. If repo A initializes after repo B, it overwrites the shared state and subsequent PR operations run against the wrong database.
Also applies to: 39-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cherrypick_ui/src/pr_state.rs` around lines 27 - 29, The bug:
set_repo_path() only updates repo_path leaving old store/repo_id live so a
slower initialize() can overwrite state; fix by introducing a generation/version
counter (e.g., repo_generation: u64) in the PR state and bump it inside
set_repo_path(), then have initialize() capture the generation at start and only
commit its store/repo_id changes if the captured generation still matches the
current repo_generation; update all detached init logic in initialize() (and
related code between the existing set_repo_path and initialize logic) to check
this generation before writing shared fields to prevent older inits from winning
the race.
Comment on lines
+47
to
+80
| let git_info = std::thread::spawn(move || -> anyhow::Result<(String, String, String, String)> { | ||
| let db_dir = repo_path.join(".cherrypick"); | ||
| std::fs::create_dir_all(&db_dir)?; | ||
| let db_path = db_dir.join("prs.db").to_string_lossy().to_string(); | ||
|
|
||
| let repo = git2::Repository::discover(&repo_path)?; | ||
| let mut revwalk = repo.revwalk()?; | ||
| revwalk.push_head()?; | ||
| revwalk.set_sorting(git2::Sort::TOPOLOGICAL | git2::Sort::REVERSE)?; | ||
| let first_oid = revwalk | ||
| .next() | ||
| .ok_or_else(|| anyhow::anyhow!("no commits"))?? | ||
| .to_string(); | ||
|
|
||
| use sha2::Digest; | ||
| let hash = format!("{:x}", sha2::Sha256::digest(repo_path.to_string_lossy().as_bytes())); | ||
| let canonical = repo_path.to_string_lossy().to_string(); | ||
|
|
||
| Ok((db_path, first_oid, hash, canonical)) | ||
| }); | ||
|
|
||
| self.executor.spawn(async move { | ||
| let (db_path, first_oid, hash, canonical) = git_info | ||
| .join() | ||
| .map_err(|_| anyhow::anyhow!("git info thread panicked"))??; | ||
|
|
||
| let store = PrStore::open(&db_path) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("{e}"))?; | ||
|
|
||
| let id = store | ||
| .ensure_repo(&first_oid, &hash, &canonical) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("{e}"))?; |
There was a problem hiding this comment.
Don't bypass PrService from the UI state layer.
This file recomputes repo identity in initialize() and writes PrStore directly in create_pr() / update_pr_status(). That diverges from PrService's invariants: stable repo identity, same-branch/status validation, and initial snapshot recording. UI-created PRs can therefore land under a different repo id and miss the snapshot data used for branch health.
Also applies to: 147-175, 195-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cherrypick_ui/src/pr_state.rs` around lines 47 - 80, The UI layer must
not open PrStore or recompute repo identity directly (git_info, PrStore::open,
ensure_repo) in initialize()/create_pr()/update_pr_status(); instead call the
existing PrService API that centralizes repo identity, same-branch/status checks
and snapshot recording. Replace the git_info thread + PrStore::open/ensure_repo
usage with a single call into PrService (e.g., PrService::initialize_repo or the
appropriate constructor/ensure method) to obtain the repo id and a
PrService-backed handle, and then invoke PrService::create_pr and
PrService::update_pr_status so all invariants (stable repo id, branch/status
validation, initial snapshot) are enforced by PrService rather than bypassing
it. Ensure any repo path hashing/canonicalization logic is moved into or
delegated to PrService, and remove direct references to PrStore from this
module.
Comment on lines
+63
to
+77
| fn do_fetch(&mut self, window: &mut Window, cx: &mut Context<Self>) { | ||
| let Some(repo) = self.repository.clone() else { | ||
| return; | ||
| }; | ||
| let askpass = Self::noop_askpass(cx); | ||
| window | ||
| .spawn(cx, async move |cx| { | ||
| let rx = repo.update(cx, |repo, cx| { | ||
| repo.fetch(FetchOptions::All, askpass, cx) | ||
| }); | ||
| let _ = rx.await; | ||
| anyhow::Ok(()) | ||
| }) | ||
| .detach(); | ||
| } |
There was a problem hiding this comment.
Git operation errors are silently discarded.
All three operations (do_fetch, do_pull, do_push) use let _ = rx.await which discards any errors. Users receive no feedback when operations fail (network issues, auth failures, conflicts, etc.).
Consider surfacing errors to the user via notifications or status indicators.
🛠️ Example pattern for error handling
window
.spawn(cx, async move |cx| {
let rx = repo.update(cx, |repo, cx| {
repo.fetch(FetchOptions::All, askpass, cx)
});
- let _ = rx.await;
- anyhow::Ok(())
+ if let Err(e) = rx.await {
+ // Surface error to user via notification
+ log::error!("Fetch failed: {e}");
+ // Consider: cx.update(|cx| show_notification(cx, format!("Fetch failed: {e}")));
+ }
+ anyhow::Ok(())
})
.detach();Also applies to: 79-103, 105-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cherrypick_ui/src/remote_toolbar.rs` around lines 63 - 77,
do_fetch/do_pull/do_push currently discard repo.update results with `let _ =
rx.await`; change these spawn closures to match the example pattern: await the
rx result, check for Err and map the error from the repo operation (e.g., result
from repo.fetch/repo.pull/repo.push), and surface failures via the UI (use
Window or Context to emit a notification or set a status indicator). Concretely,
in the async closure passed to window.spawn (where you call repo.update(cx,
|repo, cx| { repo.fetch(...) })), replace the `let _ = rx.await` with code that
awaits rx, matches Ok/Err, logs/internal-returns the error, and calls your
notification method (or Context/Window API) to show the error message to the
user so network/auth/conflict errors are not silently dropped; apply the same
change to the closures in do_pull and do_push.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
27 issues found across 46 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/cherrypick_pr/src/watcher.rs">
<violation number="1" location="crates/cherrypick_pr/src/watcher.rs:51">
P2: This path filter ignores `HEAD` changes, so branch switches can be missed even though `HEAD` is being watched. Handle `HEAD` events explicitly (or stop watching `HEAD`).</violation>
<violation number="2" location="crates/cherrypick_pr/src/watcher.rs:59">
P1: Do not emit `BranchTipChanged` when reading the ref file fails or yields an empty OID.</violation>
</file>
<file name="crates/cherrypick_agent/src/chat/mod.rs">
<violation number="1" location="crates/cherrypick_agent/src/chat/mod.rs:177">
P2: `unwrap_or_default()` silently converts malformed tool-call JSON into `Value::Null`, which will then be passed to the tool executor. This hides LLM output errors and can cause confusing downstream failures. Parse errors should be surfaced to the user or recorded as a tool error.</violation>
<violation number="2" location="crates/cherrypick_agent/src/chat/mod.rs:194">
P2: The result of `provider_handle.await` is silently discarded. If the spawned provider task panics, the `JoinError` is lost, making debugging difficult. At minimum, log the error or propagate it.</violation>
<violation number="3" location="crates/cherrypick_agent/src/chat/mod.rs:238">
P1: The `ConfirmationNeeded` event is sent but tool execution proceeds immediately without waiting for a response, making the confirmation a no-op. This directly contradicts the safety rules defined in `SYSTEM_POLICY` in the same file. Even for a temporary implementation, consider blocking destructive tool execution or skipping high-risk tools entirely until the confirmation flow is wired up.</violation>
</file>
<file name="crates/cherrypick_ui/src/remote_toolbar.rs">
<violation number="1" location="crates/cherrypick_ui/src/remote_toolbar.rs:114">
P1: `do_push` defaults missing branch name to `""`, which can trigger an invalid push attempt with empty ref names.</violation>
<violation number="2" location="crates/cherrypick_ui/src/remote_toolbar.rs:155">
P2: Ahead/behind indicators are rendered from cached state that is never refreshed after fetch/pull/push, so the toolbar can show stale tracking counts.</violation>
</file>
<file name="crates/cherrypick_pr/src/types.rs">
<violation number="1" location="crates/cherrypick_pr/src/types.rs:6">
P2: `MergeStrategy` has mismatched Serde vs helper-method string formats (`MergeCommit/Squash` vs `merge/squash`).</violation>
<violation number="2" location="crates/cherrypick_pr/src/types.rs:6">
P2: `PrStatus` has inconsistent string representations between Serde and `as_str`/`from_str`; Serde currently uses `Open/Merged/Closed` while helper methods use lowercase.</violation>
</file>
<file name="crates/cherrypick_ui/src/cherrypick_sidebar.rs">
<violation number="1" location="crates/cherrypick_ui/src/cherrypick_sidebar.rs:149">
P2: PR loading is triggered before async PR-state initialization completes, so initial list fetch can fail silently and not retry.</violation>
<violation number="2" location="crates/cherrypick_ui/src/cherrypick_sidebar.rs:153">
P2: Switching to no active repository can keep and show stale PR data because PR state is never reset before refreshing.</violation>
</file>
<file name="crates/cherrypick_pr/src/merge_service.rs">
<violation number="1" location="crates/cherrypick_pr/src/merge_service.rs:147">
P1: Swap the `merge_commits` arguments so `target` is treated as ours and `source` as theirs; the current order inverts merge/conflict semantics relative to the rest of this merge flow.</violation>
</file>
<file name="crates/cherrypick_pr/src/store.rs">
<violation number="1" location="crates/cherrypick_pr/src/store.rs:143">
P3: `get_pr` duplicates `LocalPr` row-mapping logic already defined in `map_pr_row`, which risks divergence and inconsistent behavior.</violation>
</file>
<file name="crates/cherrypick_ui/src/pr_state.rs">
<violation number="1" location="crates/cherrypick_ui/src/pr_state.rs:40">
P2: Reject PR creation when source and target branches are the same.</violation>
</file>
<file name="Cargo.toml">
<violation number="1" location="Cargo.toml:608">
P3: Align the workspace `notify` version with the existing major used elsewhere to avoid pulling in two major versions and the extra build/runtime overhead that comes with that split.</violation>
</file>
<file name="crates/cherrypick_pr/src/diff_service.rs">
<violation number="1" location="crates/cherrypick_pr/src/diff_service.rs:123">
P2: Avoid checking only the first byte of the blob OID to determine whether it is zero. Compare the full OID against `git2::Oid::zero()` so LFS pointer detection is not skipped for valid IDs with a leading `0x00` byte.</violation>
<violation number="2" location="crates/cherrypick_pr/src/diff_service.rs:141">
P2: Per-file insertion/deletion counts are always set to 0, so `DiffFileEntry` reports incorrect file-level diff stats.</violation>
</file>
<file name="crates/cherrypick_agent/src/tools/git_tools.rs">
<violation number="1" location="crates/cherrypick_agent/src/tools/git_tools.rs:342">
P1: Apply sensitive-path filtering in `search_recursive` before reading file contents; otherwise `search_code` can exfiltrate secrets that other tools explicitly block.</violation>
</file>
<file name="crates/cherrypick_agent/src/mcp.rs">
<violation number="1" location="crates/cherrypick_agent/src/mcp.rs:83">
P1: Missing MCP `initialize` handshake. The MCP protocol requires clients to send an `initialize` request (with `protocolVersion`, `clientInfo`, and `capabilities`) followed by an `initialized` notification before any other method. Without this, conformant MCP servers will reject the `tools/list` call.</violation>
</file>
<file name="crates/cherrypick_agent/src/tools/mod.rs">
<violation number="1" location="crates/cherrypick_agent/src/tools/mod.rs:106">
P1: Truncating `String` at a fixed byte index can panic on non-ASCII output. Adjust the truncation point to a valid UTF-8 char boundary before calling `truncate`.</violation>
</file>
<file name="crates/cherrypick_agent/src/context/mod.rs">
<violation number="1" location="crates/cherrypick_agent/src/context/mod.rs:24">
P2: Budget allocation can under-allocate tokens due to per-tier float truncation. Ensure the final tier absorbs rounding remainder so `hot + warm + cold == total` for all totals.</violation>
</file>
<file name="crates/cherrypick_ui/src/branch_list.rs">
<violation number="1" location="crates/cherrypick_ui/src/branch_list.rs:29">
P2: Clear cached branches when switching repositories to avoid rendering stale branch entries from the previous repo.</violation>
</file>
<file name="crates/cherrypick_ui/src/create_pr_form.rs">
<violation number="1" location="crates/cherrypick_ui/src/create_pr_form.rs:100">
P2: Cycling the source branch does not update `title`, so submitted PR titles can reference a previously selected branch.</violation>
</file>
<file name="crates/cherrypick_pr/src/service.rs">
<violation number="1" location="crates/cherrypick_pr/src/service.rs:287">
P2: This assertion is tautological (`Result` is always either `Ok` or `Err`), so the test cannot catch regressions.</violation>
</file>
<file name="crates/cherrypick_agent/src/keys.rs">
<violation number="1" location="crates/cherrypick_agent/src/keys.rs:29">
P1: The hardcoded `std::env::var` fallback in `get_key` bypasses the backend abstraction, making tests non-hermetic. If `ANTHROPIC_API_KEY` is set in the environment (e.g. in CI), the `get_nonexistent_key` and `has_key_check` tests will fail because `get_key` returns a value from the real environment even though `InMemoryBackend` is empty.
Consider moving the env-var fallback into the `EnvVarBackend` implementation (or a wrapper) so the backend abstraction isn't leaked.</violation>
</file>
<file name="crates/cherrypick_agent/src/provider/anthropic.rs">
<violation number="1" location="crates/cherrypick_agent/src/provider/anthropic.rs:153">
P1: This buffers the full SSE response before parsing, so `stream_completion` does not actually stream and can add large latency/memory overhead on long outputs.</violation>
<violation number="2" location="crates/cherrypick_agent/src/provider/anthropic.rs:195">
P1: `content_block_stop` always sends `ToolCallEnd`, which can emit incorrect tool lifecycle events for non-tool blocks (e.g. text blocks).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }); | ||
| } | ||
| _ => { | ||
| let new_oid = std::fs::read_to_string(path) |
There was a problem hiding this comment.
P1: Do not emit BranchTipChanged when reading the ref file fails or yields an empty OID.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/cherrypick_pr/src/watcher.rs, line 59:
<comment>Do not emit `BranchTipChanged` when reading the ref file fails or yields an empty OID.</comment>
<file context>
@@ -0,0 +1,188 @@
+ });
+ }
+ _ => {
+ let new_oid = std::fs::read_to_string(path)
+ .unwrap_or_default()
+ .trim()
</file context>
| risk_level: risk, | ||
| preview, | ||
| }); | ||
| // For now, auto-approve in the agentic loop. |
There was a problem hiding this comment.
P1: The ConfirmationNeeded event is sent but tool execution proceeds immediately without waiting for a response, making the confirmation a no-op. This directly contradicts the safety rules defined in SYSTEM_POLICY in the same file. Even for a temporary implementation, consider blocking destructive tool execution or skipping high-risk tools entirely until the confirmation flow is wired up.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/cherrypick_agent/src/chat/mod.rs, line 238:
<comment>The `ConfirmationNeeded` event is sent but tool execution proceeds immediately without waiting for a response, making the confirmation a no-op. This directly contradicts the safety rules defined in `SYSTEM_POLICY` in the same file. Even for a temporary implementation, consider blocking destructive tool execution or skipping high-risk tools entirely until the confirmation flow is wired up.</comment>
<file context>
@@ -0,0 +1,295 @@
+ risk_level: risk,
+ preview,
+ });
+ // For now, auto-approve in the agentic loop.
+ // In the full UI implementation, this would wait for user input
+ // via a oneshot channel before proceeding.
</file context>
| .repository | ||
| .as_ref() | ||
| .and_then(|r| r.read(cx).branch.as_ref().map(|b| b.name().to_string())) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
P1: do_push defaults missing branch name to "", which can trigger an invalid push attempt with empty ref names.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/cherrypick_ui/src/remote_toolbar.rs, line 114:
<comment>`do_push` defaults missing branch name to `""`, which can trigger an invalid push attempt with empty ref names.</comment>
<file context>
@@ -0,0 +1,203 @@
+ .repository
+ .as_ref()
+ .and_then(|r| r.read(cx).branch.as_ref().map(|b| b.name().to_string()))
+ .unwrap_or_default();
+
+ window
</file context>
| let target = repo.find_commit(git2::Oid::from_str(&session.target_oid)?)?; | ||
|
|
||
| let mut merge_opts = git2::MergeOptions::new(); | ||
| let mut index = repo.merge_commits(&source, &target, Some(&mut merge_opts))?; |
There was a problem hiding this comment.
P1: Swap the merge_commits arguments so target is treated as ours and source as theirs; the current order inverts merge/conflict semantics relative to the rest of this merge flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/cherrypick_pr/src/merge_service.rs, line 147:
<comment>Swap the `merge_commits` arguments so `target` is treated as ours and `source` as theirs; the current order inverts merge/conflict semantics relative to the rest of this merge flow.</comment>
<file context>
@@ -0,0 +1,337 @@
+ let target = repo.find_commit(git2::Oid::from_str(&session.target_oid)?)?;
+
+ let mut merge_opts = git2::MergeOptions::new();
+ let mut index = repo.merge_commits(&source, &target, Some(&mut merge_opts))?;
+
+ for (path, content) in &session.resolutions {
</file context>
Suggested change
| let mut index = repo.merge_commits(&source, &target, Some(&mut merge_opts))?; | |
| let mut index = repo.merge_commits(&target, &source, Some(&mut merge_opts))?; |
| if path.is_dir() { | ||
| search_recursive(&path, pattern, repo_root, results, max_results)?; | ||
| } else if path.is_file() { | ||
| if let Ok(content) = std::fs::read_to_string(&path) { |
There was a problem hiding this comment.
P1: Apply sensitive-path filtering in search_recursive before reading file contents; otherwise search_code can exfiltrate secrets that other tools explicitly block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/cherrypick_agent/src/tools/git_tools.rs, line 342:
<comment>Apply sensitive-path filtering in `search_recursive` before reading file contents; otherwise `search_code` can exfiltrate secrets that other tools explicitly block.</comment>
<file context>
@@ -0,0 +1,517 @@
+ if path.is_dir() {
+ search_recursive(&path, pattern, repo_root, results, max_results)?;
+ } else if path.is_file() {
+ if let Ok(content) = std::fs::read_to_string(&path) {
+ for (lineno, line) in content.lines().enumerate() {
+ if line.contains(pattern) {
</file context>
| let pr = service | ||
| .create_pr(&path, "My PR", "feature", "master") | ||
| .await; | ||
| assert!(pr.is_ok() || pr.is_err()); |
There was a problem hiding this comment.
P2: This assertion is tautological (Result is always either Ok or Err), so the test cannot catch regressions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/cherrypick_pr/src/service.rs, line 287:
<comment>This assertion is tautological (`Result` is always either `Ok` or `Err`), so the test cannot catch regressions.</comment>
<file context>
@@ -0,0 +1,349 @@
+ let pr = service
+ .create_pr(&path, "My PR", "feature", "master")
+ .await;
+ assert!(pr.is_ok() || pr.is_err());
+ }
+
</file context>
| EventKind::Modify(_) | EventKind::Create(_) | EventKind::Remove(_) | ||
| ) { | ||
| for path in &event.paths { | ||
| if let Some(branch) = extract_branch_name(path, &refs_dir) { |
There was a problem hiding this comment.
P2: This path filter ignores HEAD changes, so branch switches can be missed even though HEAD is being watched. Handle HEAD events explicitly (or stop watching HEAD).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/cherrypick_pr/src/watcher.rs, line 51:
<comment>This path filter ignores `HEAD` changes, so branch switches can be missed even though `HEAD` is being watched. Handle `HEAD` events explicitly (or stop watching `HEAD`).</comment>
<file context>
@@ -0,0 +1,188 @@
+ EventKind::Modify(_) | EventKind::Create(_) | EventKind::Remove(_)
+ ) {
+ for path in &event.paths {
+ if let Some(branch) = extract_branch_name(path, &refs_dir) {
+ match event.kind {
+ EventKind::Remove(_) => {
</file context>
| let is_binary = delta.flags().is_binary(); | ||
|
|
||
| let is_lfs = if !is_binary { | ||
| if let Some(oid) = new_file.id().as_bytes().first() { |
There was a problem hiding this comment.
P2: Avoid checking only the first byte of the blob OID to determine whether it is zero. Compare the full OID against git2::Oid::zero() so LFS pointer detection is not skipped for valid IDs with a leading 0x00 byte.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/cherrypick_pr/src/diff_service.rs, line 123:
<comment>Avoid checking only the first byte of the blob OID to determine whether it is zero. Compare the full OID against `git2::Oid::zero()` so LFS pointer detection is not skipped for valid IDs with a leading `0x00` byte.</comment>
<file context>
@@ -0,0 +1,237 @@
+ let is_binary = delta.flags().is_binary();
+
+ let is_lfs = if !is_binary {
+ if let Some(oid) = new_file.id().as_bytes().first() {
+ if *oid != 0 {
+ let blob = repo.find_blob(new_file.id());
</file context>
| status, merge_strategy, merged_oid, created_at, updated_at | ||
| FROM prs WHERE id = ?1", | ||
| params![pr_id], | ||
| |row| { |
There was a problem hiding this comment.
P3: get_pr duplicates LocalPr row-mapping logic already defined in map_pr_row, which risks divergence and inconsistent behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/cherrypick_pr/src/store.rs, line 143:
<comment>`get_pr` duplicates `LocalPr` row-mapping logic already defined in `map_pr_row`, which risks divergence and inconsistent behavior.</comment>
<file context>
@@ -0,0 +1,436 @@
+ status, merge_strategy, merged_oid, created_at, updated_at
+ FROM prs WHERE id = ?1",
+ params![pr_id],
+ |row| {
+ Ok(LocalPr {
+ id: row.get(0)?,
</file context>
EntelligenceAI PR SummaryThis PR introduces a full AI-assisted local pull request feature for Zed, implemented across three new workspace crates integrated at application startup.
Confidence Score: 1/5 - Blocking IssuesNot safe to merge — this PR introduces multiple compile-breaking duplicate declarations in Key Findings:
Files requiring special attention
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
This PR introduces three new workspace crates — cherrypick_agent, cherrypick_pr, and cherrypick_ui — implementing a full AI-assisted local pull request workflow inside Zed. cherrypick_agent provides an agentic LLM chat loop with tool execution and context management. cherrypick_pr handles local PR lifecycle, Git diffs, merges, and filesystem watching. cherrypick_ui delivers GPUI panels for branch listing, PR management, staging, and diff viewing, integrated into the Zed workspace at startup.
Changes
| File(s) | Summary |
|---|---|
Cargo.lock |
Adds three new internal crates and their dependency trees; introduces rusqlite, tokio-rusqlite, notify, hashlink, fallible-streaming-iterator, inotify, notify-types; upgrades darling to 0.21.3 |
Cargo.toml |
Registers cherrypick_ui, cherrypick_pr, cherrypick_agent as workspace members and dependencies; adds lru, notify, rusqlite (bundled), and tokio-rusqlite to shared dependency table |
crates/cherrypick_agent/Cargo.toml |
New crate manifest for cherrypick_agent (v0.1.0, GPL-3.0-or-later) with dependencies on git2, tokio, serde, reqwest, rusqlite, tokio-rusqlite, tiktoken-rs, sha2, and others |
crates/cherrypick_agent/src/lib.rs |
Root library file declaring eight public submodules and re-exporting primary types (AgentService, ContextEngine, McpManager, ToolExecutor, Message, RiskLevel, StreamChunk) |
crates/cherrypick_agent/src/chat/mod.rs |
Implements AgentService agentic chat loop with streaming LLM completions, tool execution, history truncation, AgentConfig (10-iteration/120s cap), AgentEvent enum, and hardcoded SYSTEM_POLICY safety guardrails |
crates/cherrypick_agent/src/chat/store.rs |
SQLite-backed ChatStore with async CRUD for conversations and messages (create_conversation, list_conversations, delete_conversation, save_message, load_messages); WAL mode; cascade deletes; integration tests |
crates/cherrypick_agent/src/context/mod.rs |
ContextBudget (hot/warm/cold tier partitioning) and ContextEngine (repo file map builder, token estimation via len/4, history truncation); unit tests |
crates/cherrypick_agent/src/error.rs |
AgentError enum via thiserror covering provider, rate limit, context, tool, skill, MCP, DB, IO, JSON, and HTTP variants; Result<T> alias; display tests |
crates/cherrypick_agent/src/keys.rs |
KeyBackend trait with EnvVarBackend and InMemoryBackend implementations; KeyManager with provider:profile composite key resolution and {PROVIDER}_API_KEY fallback; unit tests |
crates/cherrypick_agent/src/mcp.rs |
JSON-RPC over stdio MCP client (McpClient) with tool discovery, invocation, size validation, 30s timeout; McpManager for multi-server lifecycle; namespaced tool names (mcp_<server>__<tool>); unit tests |
crates/cherrypick_agent/src/provider/mod.rs |
LlmProvider async trait with name() and stream_completion() via unbounded mpsc; declares types and anthropic submodules |
crates/cherrypick_agent/src/provider/anthropic.rs |
AnthropicProvider implementing LlmProvider against Anthropic Messages API with SSE parsing (text deltas, tool call lifecycle, usage, errors) into StreamChunk variants; unit tests |
crates/cherrypick_agent/src/provider/types.rs |
Core LLM types: Role, MessageContent, Message (factory methods), ToolDefinition, ToolCall, CompletionRequest, StreamChunk, Usage, RiskLevel; serde support; unit tests |
crates/cherrypick_agent/src/skills/mod.rs |
Skill and SkillLoader with four built-in skills, Markdown/YAML frontmatter user skill loading, slash-command resolution, directory discovery; unit and tempfile integration tests |
crates/cherrypick_agent/src/tools/mod.rs |
ToolHandler async trait; ToolExecutor with registry dispatch, 50k-char output truncation, repo exclusion, risk lookup, preview; ConfirmationRequest; is_sensitive_path guard; unit tests |
crates/cherrypick_agent/src/tools/git_tools.rs |
Eight ToolHandler implementations (ReadFileTool, ListFilesTool, GitStatusTool, GitDiffTool, GitLogTool, SearchCodeTool, StageFilesTool, CreateCommitTool) using git2; path traversal and sensitive file protections; unit tests |
crates/cherrypick_agent/src/tools/safe_path.rs |
resolve_safe_path utility validating paths against repo root, blocking ../ traversal and symlink escapes via canonicalization; four unit tests |
crates/cherrypick_pr/Cargo.toml |
New crate manifest for cherrypick_pr with dependencies on git2, rusqlite, tokio-rusqlite, tokio, serde, chrono, lru, notify, sha2, thiserror |
crates/cherrypick_pr/src/lib.rs |
Root library file declaring and re-exporting seven submodules and key domain types (DiffService, MergeService, PrService, PrStore, BranchWatcher, PrError, LocalPr, MergeStrategy, etc.) |
crates/cherrypick_pr/src/diff_service.rs |
DiffService computing and LRU-caching Git branch diffs via git2; BranchDiff/DiffFileEntry structs; invalidate/invalidate_all; unit tests |
crates/cherrypick_pr/src/error.rs |
PrError enum via thiserror with domain variants (NotFound, BranchNotFound, MergeConflicts, InvalidStatusTransition) and #[from] conversions for rusqlite, git2, io; Result<T> alias; unit tests |
crates/cherrypick_pr/src/merge_service.rs |
MergeService with three-way merge workflow: conflict detection, content retrieval, stateful MergeSession, merge/squash commit creation, TargetMoved stale-OID guard; read_file_from_tree helper; unit tests |
crates/cherrypick_pr/src/service.rs |
PrService business logic: create_pr, close_pr, reopen_pr, retarget_pr with branch validation and status transition guards; check_branch_health for force-push detection; private OID/hash helpers; test suite |
crates/cherrypick_pr/src/store.rs |
PrStore async SQLite CRUD: schema (repos, prs, pr_snapshots), ensure_repo, PR create/retrieve/list/delete, status/branch-tip/target updates, snapshot recording, cascade deletes; async test suite |
crates/cherrypick_pr/src/types.rs |
Domain types: PrStatus, MergeStrategy (string round-trip), LocalPr, PrSnapshot, BranchHealth, FileContent (encoding/LFS detection), ThreeWayContent, RepoRecord; unit tests |
crates/cherrypick_pr/src/watcher.rs |
BranchWatcher using notify to monitor refs/heads for BranchTipChanged/BranchDeleted/WatcherError events; detect_force_push via git2 ancestry check; unit tests |
crates/cherrypick_ui/Cargo.toml |
New crate manifest for cherrypick_ui with dependencies on gpui, git, git_ui, project, ui, settings, askpass, cherrypick_pr, cherrypick_agent, git_graph, sha2, git2, tokio |
crates/cherrypick_ui/src/lib.rs |
Root module declaring all cherrypick_ui submodules and init function registering CherryPickSidebar via GPUI observe_new lifecycle hook |
crates/cherrypick_ui/src/cherrypick_sidebar.rs |
CherryPickSidebar GPUI Panel (right-docked, 260px default) integrating BranchList, PrList, CreatePrForm; GitStoreEvent subscriptions; ToggleSidebar/OpenCherryPick actions |
crates/cherrypick_ui/src/cherrypick_view.rs |
CherryPickView panel with Staging, PrDetail, and BranchCompare modes; git store subscriptions; async diff loading via PrState; implements Item, Focusable, EventEmitter, Render |
crates/cherrypick_ui/src/pr_state.rs |
PrState central UI state manager: initialize, list_open_prs, create_pr, get_branch_diff, update_pr_status, get_unified_diff, get_pr; all dispatched via GPUI BackgroundExecutor; DiffService with LRU-32 |
crates/cherrypick_ui/src/branch_list.rs |
BranchList GPUI component rendering scrollable local/remote branches with checkout-on-click, priority sorting, active branch highlighting, and ahead/behind tracking display |
crates/cherrypick_ui/src/create_pr_form.rs |
CreatePrForm collapsible UI panel for PR creation with branch auto-selection, visibility management, source/target validation, and CreatePrFormEvent::Submit emission |
crates/cherrypick_ui/src/diff_file_list.rs |
DiffFileList GPUI component rendering BranchDiff file list with summary bar, status indicators (A/D/M/R/C/T), rename arrows, per-file stats, and binary file handling |
crates/cherrypick_ui/src/pr_list.rs |
PrList GPUI component rendering scrollable LocalPr items with color-coded status, branch info, hover/active states, click-to-select via PrListEvent::Selected, and empty-state message |
crates/cherrypick_ui/src/pr_detail.rs |
PrDetail GPUI component displaying PR title, color-coded status badge, branch direction, conflict count, and context-aware action buttons emitting PrAction (Close/Reopen/StatusChanged) |
crates/cherrypick_ui/src/staging_view.rs |
StagingView two-panel git staging UI with staged/unstaged file lists, per-file stage/unstage actions, bulk Stage All/Unstage All, Repository entity subscription, and GPUI integration |
crates/cherrypick_ui/src/remote_toolbar.rs |
RemoteToolbar GPUI component displaying ahead/behind counts with Fetch/Pull/Push buttons dispatched asynchronously via window.spawn using AskPassDelegate |
crates/cherrypick_ui/src/commit_graph_embed.rs |
open_git_graph function activating existing or creating new GitGraph panel in the active workspace pane with repo presence validation |
crates/cherrypick_ui/src/repo_state_banner.rs |
|
crates/cherrypick_ui/src/settings.rs |
|
crates/cherrypick_ui/src/stash_list.rs |
|
crates/cherrypick_ui/src/worktree_list.rs |
Placeholder stub files with no functional implementation, marked for future sections |
crates/zed/Cargo.toml |
Adds cherrypick_ui as a workspace dependency to the zed crate |
crates/zed/src/main.rs |
Calls cherrypick_ui::init(cx) during application startup between git_ui and git_graph initialization |
crates/zed/src/zed.rs |
Imports and registers CherryPickSidebar in initialize_panels alongside other workspace panels |
Sequence Diagram
This diagram shows the interactions between components:
sequenceDiagram
participant Caller
participant AgentService
participant ContextEngine
participant LlmProvider
participant ToolExecutor
Caller->>AgentService: send_message(user_message, repo_path, event_tx, cancel)
activate AgentService
AgentService->>AgentService: history.push(Message::user)
AgentService->>ToolExecutor: definitions()
ToolExecutor-->>AgentService: tool_defs[]
loop Agentic Loop (max 10 iterations / 120s)
AgentService->>ContextEngine: truncate_history(history, budget.warm)
ContextEngine-->>AgentService: truncated messages
AgentService->>AgentService: build CompletionRequest(model, messages, system_policy, tools)
AgentService->>LlmProvider: stream_completion(request, chunk_tx) [spawned task]
activate LlmProvider
loop Stream chunks from LLM
LlmProvider-->>AgentService: StreamChunk::TextDelta(text)
AgentService->>Caller: event_tx <- AgentEvent::TextDelta(text)
LlmProvider-->>AgentService: StreamChunk::ToolCallStart{id, name}
AgentService->>ToolExecutor: risk_level(name)
ToolExecutor-->>AgentService: RiskLevel
AgentService->>Caller: event_tx <- AgentEvent::ToolCallStarted{id, name, risk_level}
LlmProvider-->>AgentService: StreamChunk::ToolCallDelta(json)
AgentService->>AgentService: accumulate tool_args
LlmProvider-->>AgentService: StreamChunk::ToolCallEnd
AgentService->>AgentService: parse args -> push to tool_calls[]
LlmProvider-->>AgentService: StreamChunk::Done
end
deactivate LlmProvider
AgentService->>AgentService: push assistant message(text + tool_uses) to history
alt No tool calls
AgentService->>Caller: event_tx <- AgentEvent::TurnComplete
AgentService-->>Caller: Ok(())
else Tool calls present
loop For each ToolCall
AgentService->>ToolExecutor: risk_level(tc.name)
ToolExecutor-->>AgentService: RiskLevel
opt risk.requires_confirmation()
AgentService->>ToolExecutor: preview(tc)
ToolExecutor-->>AgentService: preview_text
AgentService->>Caller: event_tx <- AgentEvent::ConfirmationNeeded{tool_name, risk_level, preview}
end
AgentService->>ToolExecutor: execute(tc, repo_path)
ToolExecutor-->>AgentService: Ok(output) / Err(e)
AgentService->>Caller: event_tx <- AgentEvent::ToolCallCompleted{id, result, is_error}
AgentService->>AgentService: push Message::tool_result to history
end
Note over AgentService: Continue loop -> send tool results back to LLM
end
end
deactivate AgentService
🔗 Cross-Repository Impact Analysis
Enable automatic detection of breaking changes across your dependent repositories. → Set up now
Learn more about Cross-Repository Analysis
What It Does
- Automatically identifies repositories that depend on this code
- Analyzes potential breaking changes across your entire codebase
- Provides risk assessment before merging to prevent cross-repo issues
How to Enable
- Visit Settings → Code Management
- Configure repository dependencies
- Future PRs will automatically include cross-repo impact analysis!
Benefits
- 🛡️ Prevent breaking changes across repositories
- 🔍 Catch integration issues before they reach production
- 📊 Better visibility into your multi-repo architecture
Comment on lines
+278
to
+282
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
There was a problem hiding this comment.
Correctness: The diff introduces a second mod tests block, but the complete file already contains one at line ~242. Rust does not allow two modules with the same name in the same scope — this will cause a compile error (error[E0428]: the name 'tests' is defined multiple times).
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `crates/cherrypick_agent/src/chat/mod.rs`, the diff adds a new `#[cfg(test)] mod tests { use super::*;` block starting at line 278, but the file already has a `#[cfg(test)] mod tests` block beginning around line 242. Rust forbids duplicate module names in the same scope and this will fail to compile. Remove the duplicate `mod tests` declaration added by the diff and instead merge any new test functions into the existing `mod tests` block.
| format!("{index}{wt}") | ||
| } | ||
|
|
||
| pub struct GitDiffTool; |
There was a problem hiding this comment.
Correctness: GitDiffTool is already defined later in this file — this duplicate pub struct GitDiffTool; declaration will cause a Rust compile error (error[E0428]: the name 'GitDiffTool' is defined multiple times).
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `crates/cherrypick_agent/src/tools/git_tools.rs`, lines 151-153 introduce a duplicate `pub struct GitDiffTool;` declaration. The struct is already defined later in the same file. Remove the three added lines (the blank line, `pub struct GitDiffTool;`, and the trailing blank line) introduced by this diff to fix the compile error.
| } | ||
| } | ||
|
|
||
| pub struct SearchCodeTool; |
There was a problem hiding this comment.
Correctness: SearchCodeTool is already defined earlier in this file — this duplicate declaration will cause a compile error (error[E0428]: the name 'SearchCodeTool' is defined multiple times).
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `crates/cherrypick_agent/src/tools/git_tools.rs` at lines 260-262, a duplicate `pub struct SearchCodeTool;` declaration was introduced. The struct is already defined earlier in the same file. Remove the duplicate declaration at lines 260-262 entirely to fix the compile error.
| Ok(()) | ||
| } | ||
|
|
||
| pub struct StageFilesTool; |
There was a problem hiding this comment.
Correctness: StageFilesTool is already defined in this file with a full ToolHandler implementation — this duplicate pub struct StageFilesTool; declaration will cause a Rust compilation error (error[E0428]: the name 'StageFilesTool' is defined multiple times).
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `crates/cherrypick_agent/src/tools/git_tools.rs`, lines 358-360 introduce a duplicate `pub struct StageFilesTool;` declaration. The struct is already defined earlier in the file with a complete `ToolHandler` implementation. Remove the three added lines (the blank line, the `pub struct StageFilesTool;` declaration, and the trailing blank line) to fix the duplicate definition compile error.
| } | ||
| } | ||
|
|
||
| pub struct CreateCommitTool; |
There was a problem hiding this comment.
Correctness: CreateCommitTool is already defined in this file — this duplicate pub struct CreateCommitTool; declaration will cause a compile error (error[E0428]: the name CreateCommitTool is defined multiple times).
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `crates/cherrypick_agent/src/tools/git_tools.rs` around lines 412-414, there is a duplicate `pub struct CreateCommitTool;` declaration being added. The struct `CreateCommitTool` is already defined earlier in the file (with its full `ToolHandler` implementation). Remove the three added lines (the blank line, `pub struct CreateCommitTool;`, and the trailing blank line) to fix the duplicate definition compile error.
| revwalk.push_head().map_err(|_| { | ||
| PrError::Other("Cannot find first commit: no HEAD".to_string()) | ||
| })?; | ||
| revwalk.set_sorting(git2::Sort::TOPOLOGICAL | git2::Sort::REVERSE)?; |
There was a problem hiding this comment.
Correctness: revwalk.set_sorting(...) resets the revwalk in libgit2 (documented: "Changing the sorting method will cause the revwalk to be reset"), so calling it after push_head() discards the pushed start point — revwalk.next() will always return None, causing every call to find_first_commit_oid to fail with "Empty repository" even on a normal repo. set_sorting must be called before push_head().
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `crates/cherrypick_pr/src/service.rs`, function `find_first_commit_oid` (lines 191–202), the call to `revwalk.set_sorting(git2::Sort::TOPOLOGICAL | git2::Sort::REVERSE)` at line 196 must be moved to before the `revwalk.push_head()` call at line 193. The libgit2 `git_revwalk_sorting` function resets the revwalk when called, which discards any previously pushed starting points. As written, push_head succeeds but set_sorting immediately resets the walk, so next() always returns None and the function always errors with 'Empty repository'. Fix: swap the order so set_sorting is called first, then push_head.
Comment on lines
+104
to
+106
|
|
||
| if output.len() > OUTPUT_TRUNCATION_LIMIT { | ||
| output.truncate(OUTPUT_TRUNCATION_LIMIT); |
There was a problem hiding this comment.
Correctness: String::truncate panics if OUTPUT_TRUNCATION_LIMIT (50,000) falls in the middle of a multi-byte UTF-8 character — any git diff or file content with Unicode near the boundary will crash the process.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `crates/cherrypick_agent/src/tools/mod.rs`, lines 104-106, `output.truncate(OUTPUT_TRUNCATION_LIMIT)` will panic if byte offset 50,000 falls in the middle of a multi-byte UTF-8 character. Fix by finding the last valid char boundary at or before `OUTPUT_TRUNCATION_LIMIT` using `char_indices()` before calling `truncate()`.
Comment on lines
+392
to
+396
| for path in &paths { | ||
| if super::is_sensitive_path(path) { | ||
| return Err(AgentError::ToolExecution(format!("Cannot stage sensitive file: {path}"))); | ||
| } | ||
| index.add_path(Path::new(path)) |
There was a problem hiding this comment.
Correctness: Unlike ReadFileTool which calls both is_sensitive_path(path) AND resolve_safe_path(repo_path, path)?, this implementation only calls is_sensitive_path before passing the raw path to index.add_path(Path::new(path)). A traversal path like ../../.env would pass is_sensitive_path (which checks file names, not traversal) and get fed directly to git2, bypassing the traversal guard entirely.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `crates/cherrypick_agent/src/tools/git_tools.rs`, inside `StageFilesTool::execute` (lines 392-396), add a call to `resolve_safe_path(repo_path, path)?` before `index.add_path(Path::new(path))` for each path in the loop. This mirrors the pattern used in `ReadFileTool` and prevents directory traversal attacks where a path like `../../.env` bypasses `is_sensitive_path` (which only checks file names) and reaches `git2` directly.
| @@ -0,0 +1,295 @@ | |||
| pub mod store; | |||
There was a problem hiding this comment.
Correctness: CLAUDE.md / AGENTS.md): This file is created at src/chat/mod.rs, which explicitly violates the rule "Never create files with mod.rs paths - prefer src/some_module.rs instead of src/some_module/mod.rs". The file should be src/chat.rs (or the contents split into src/chat/agent_service.rs etc. without a mod.rs).
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `crates/cherrypick_agent/src/chat/mod.rs` (lines 1–57): the file is placed at `src/chat/mod.rs`, violating the project guideline that forbids `mod.rs` paths. Rename the file to `src/chat.rs` (moving it out of the `chat/` subdirectory), keeping `store` as a sibling module `src/chat/store.rs` if needed, or restructure so no `mod.rs` file exists. Update any `mod chat;` declarations in `src/lib.rs` accordingly.
Comment on lines
+222
to
+234
| for tc in &tool_calls { | ||
| let risk = self | ||
| .tool_executor | ||
| .risk_level(&tc.name) | ||
| .unwrap_or(RiskLevel::ReadOnly); | ||
|
|
||
| if risk.requires_confirmation() { | ||
| let preview = self | ||
| .tool_executor | ||
| .preview(tc) | ||
| .unwrap_or_else(|| tc.name.clone()); | ||
| let _ = event_tx.send(AgentEvent::ConfirmationNeeded { | ||
| tool_name: tc.name.clone(), |
There was a problem hiding this comment.
Correctness: 🚨 ConfirmationNeeded is emitted but execution immediately proceeds — high-risk tools (branch delete, force-push) are always auto-approved, directly violating the SYSTEM_POLICY safety rules that prohibit these operations without explicit user confirmation.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `crates/cherrypick_agent/src/chat/mod.rs`, the `send_message` method at the `ConfirmationNeeded` block (around line 222-234) sends a `ConfirmationNeeded` event but immediately proceeds to execute the tool without waiting for user approval. Replace the auto-approval with a real blocking mechanism — pass a `oneshot::Sender<bool>` inside `ConfirmationNeeded` and `await` the receiver here before calling `self.tool_executor.execute`. If the confirmation is rejected, skip the tool call and push an appropriate tool_result indicating rejection.
Brings 1419 commits from zed-industries/zed:main onto our fork. Conflicts resolved: - Cargo.toml: kept cherrypick deps (lru, notify, rusqlite), took upstream's lsp-types rev bump, nucleo addition, rust-embed 8.11 - Cargo.lock: took upstream's darling 0.23.0 - Re-added tiktoken-rs to workspace deps (upstream removed; still used by cherrypick_agent)
|
🐕 Corgea found the following new SCA issues in the codebase:
Showing 6 out of 13 findings. See full results |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Cargo.toml (1)
745-745: Remove unusedtiktoken-rsdependency to reduce build overhead.The
tiktoken-rsgit dependency at line 745 is not used anywhere in the codebase. No Rust files reference it, and it appears in no feature flags or conditional code paths. Removing it eliminates unnecessary git-fetch and compile costs during builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` at line 745, Remove the unused git dependency entry for tiktoken-rs from Cargo.toml: locate the dependency stanza referencing tiktoken-rs = { git = "https://github.com/zed-industries/tiktoken-rs", rev = "2570c4387a8505fb8f1d3f3557454b474f1e8271" } and delete it, then run a cargo clean && cargo update (or regenerate Cargo.lock) to ensure the lockfile and build artifacts no longer reference tiktoken-rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Cargo.toml`:
- Line 745: Remove the unused git dependency entry for tiktoken-rs from
Cargo.toml: locate the dependency stanza referencing tiktoken-rs = { git =
"https://github.com/zed-industries/tiktoken-rs", rev =
"2570c4387a8505fb8f1d3f3557454b474f1e8271" } and delete it, then run a cargo
clean && cargo update (or regenerate Cargo.lock) to ensure the lockfile and
build artifacts no longer reference tiktoken-rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 513c6607-933d-4061-a710-053ca049feb1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Cargo.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #ISSUE
Before you mark this PR as ready for review, make sure that you have:
Release Notes:
Summary by cubic
Add the CherryPick application shell to Zed: a Git-focused sidebar and view with local PR management and an extensible agent backend. Rebased on upstream
main; kept CherryPick deps and restoredtiktoken-rsto support the agent.New Features
cherrypick_ui: Sidebar and view for branches, staging, unified diff, PR list/detail/creation; fetch/pull/push; opens the built-in Git graph.cherrypick_pr: SQLite local PR store (create/list/update), diff service with LRU cache, merge/conflict helpers, and a branch watcher.cherrypick_agent: Chat loop with streaming; Anthropic provider; Git/file tools with safe-path and risk gating; context budgeting; MCP client; key manager; SQLite chat store.cherrypick_ui,cherrypick_pr, andcherrypick_agentto workspace members and dependencies.Migration
ANTHROPIC_API_KEYto enable the agent with the Anthropic provider.Written for commit 73d7b5e. Summary will update on new commits.
Summary by CodeRabbit