fix(worktree): filter worktrees by branch pattern and handle orphaned state #941
Conversation
|
Warning Rate limit exceeded@Orinks has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a shared BASE_BRANCHES constant, filters base branches from worktree discovery, implements frontend worktree cleanup flows with skipCleanup support, extends IPC/preload/store/UI signatures and behavior, updates cross-platform tests, and adds a .gitignore entry for temporary Claude files. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer UI
participant Store as Task Store
participant IPC as Main (IPC)
participant Git as Git ops
UI->>Store: complete task (forceCleanup?, skipCleanup?)
Store->>IPC: persistTaskStatus(taskId, "done", {forceCleanup?, skipCleanup?})
IPC->>Git: check worktree existence & getWorktreeBranch(worktreePath)
alt git_error
IPC-->>Store: {success:false, error, canSkipCleanup:false}
else orphaned/base branch
IPC->>Git: removeWorktreeWithRetry
IPC->>Git: pruneWorktrees (best-effort)
IPC->>Git: deleteBranch (best-effort)
IPC-->>Store: {success:false, canSkipCleanup:true, worktreePath}
else valid feature branch
alt forceCleanup
IPC->>Git: removeWorktreeWithRetry + deleteBranch
IPC-->>Store: {success:true}
else
IPC-->>Store: {success:false, canSkipCleanup:false, worktreeExists:true, worktreePath}
end
end
Store-->>UI: result (includes canSkipCleanup)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @Orinks, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the worktree management experience by introducing intelligent filtering, automatic cleanup for orphaned worktrees, and robust error handling during worktree removal. It provides users with more control and flexibility, especially when encountering issues like file locks or worktrees left in an inconsistent state after external merges, ultimately streamlining the task completion workflow. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 several valuable improvements to worktree management, including filtering worktrees by branch, handling orphaned worktrees, and adding retry logic for cleanup operations. The changes are well-structured and address real-world edge cases, particularly around merged PRs and file locks on Windows. My review includes suggestions for reducing code duplication, improving error logging for easier debugging, and a few minor consistency fixes in the frontend state management. Overall, this is a solid enhancement to the application's stability.
apps/backend/core/worktree.py
Outdated
| base_branches = {"main", "master", "develop", "HEAD"} | ||
| if info and info.branch not in base_branches: |
There was a problem hiding this comment.
The base_branches set is defined here again, which is a duplication of the definition on line 702. To improve maintainability and avoid potential inconsistencies, it would be better to define this set once at the beginning of the list_all_worktrees method and reuse it in both loops.
| } catch { | ||
| // Git command failed - worktree is in invalid state | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This catch block currently swallows the error. For better debugging, it would be helpful to log the error that occurs when execFileSync fails. This can provide valuable context about why the git command failed, for example, if git is not found or if there are permission issues.
} catch (error) {
// Git command failed - worktree is in invalid state
console.warn(`[getWorktreeBranch] Failed to get branch for worktree at ${worktreePath}:`, error);
return null;
}| console.warn(`[TASK_UPDATE_STATUS] Could not get branch name, using fallback pattern: ${branch}`, branchError); | ||
| }); | ||
| console.warn(`[TASK_UPDATE_STATUS] Orphaned worktree removed: ${worktreePath}`); | ||
| } catch { |
There was a problem hiding this comment.
This catch block is empty, which means the error from git worktree remove is swallowed. It would be beneficial to log this error to understand why the command failed before falling back to manual directory deletion. This can help diagnose issues where the worktree is in a strange state.
} catch (gitRemoveError) {
console.warn(`[TASK_UPDATE_STATUS] 'git worktree remove' failed, falling back to directory deletion. Error:`, gitRemoveError);| const fs = require('fs'); | ||
| fs.rmSync(worktreePath, { recursive: true, force: true }); |
There was a problem hiding this comment.
The fs module is required here and again on line 596. It's better to import rmSync from fs at the top of the file (e.g., import { rmSync } from 'fs';) and use it directly. This avoids repeated require calls and keeps imports organized. If you prefer to keep it inline, you can use require('fs').rmSync(...) to avoid declaring a new variable.
require('fs').rmSync(worktreePath, { recursive: true, force: true });| // Skip if HEAD is detached or pointing to a main/base branch | ||
| // (this can happen if worktree is orphaned after GitHub merge) | ||
| // But allow any feature branch pattern (auto-claude/*, feature/*, etc.) | ||
| const baseBranches = ['main', 'master', 'develop', 'HEAD']; |
There was a problem hiding this comment.
The baseBranches array is also defined in apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts. To avoid duplication and ensure consistency, consider moving this to a shared constants file, for example in apps/frontend/src/shared/constants/task.ts.
| const handleWorktreeSkipCleanup = async () => { | ||
| if (!worktreeCleanupDialog.taskId) return; | ||
|
|
||
| setWorktreeCleanupDialog(prev => ({ ...prev, isProcessing: true, error: undefined })); |
There was a problem hiding this comment.
For consistency with handleWorktreeCleanupConfirm, the canSkipCleanup state should also be reset to false here when starting the processing.
| setWorktreeCleanupDialog(prev => ({ ...prev, isProcessing: true, error: undefined })); | |
| setWorktreeCleanupDialog(prev => ({ ...prev, isProcessing: true, error: undefined, canSkipCleanup: false })); |
| setWorktreeCleanupDialog({ | ||
| open: false, | ||
| taskId: null, | ||
| taskTitle: '', | ||
| worktreePath: undefined, | ||
| isProcessing: false, | ||
| error: undefined, | ||
| canSkipCleanup: false | ||
| }); |
There was a problem hiding this comment.
This block of code to reset the worktreeCleanupDialog state is identical to the one in handleWorktreeCleanupConfirm (lines 505-513). To reduce duplication and improve maintainability, you could extract this logic into a helper function, e.g., resetWorktreeDialog, and call it in both places.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/backend/core/worktree.py (1)
700-716: Refactor: Extract duplicatedbase_branchesset to avoid DRY violation.The
base_branchesset is defined identically at lines 702 and 714. Define it once at the start of the method for maintainability.♻️ Proposed fix
def list_all_worktrees(self) -> list[WorktreeInfo]: """List all spec worktrees (includes legacy .worktrees/ location).""" worktrees = [] seen_specs = set() + # Skip worktrees on base branches (orphaned after GitHub merge) + base_branches = {"main", "master", "develop", "HEAD"} # Check new location first if self.worktrees_dir.exists(): for item in self.worktrees_dir.iterdir(): if item.is_dir(): info = self.get_worktree_info(item.name) - # Skip if worktree is on a base branch (orphaned after GitHub merge) - # But allow any feature branch pattern (auto-claude/*, feature/*, etc.) - base_branches = {"main", "master", "develop", "HEAD"} if info and info.branch not in base_branches: worktrees.append(info) seen_specs.add(item.name) # Check legacy location (.worktrees/) legacy_dir = self.project_dir / ".worktrees" if legacy_dir.exists(): for item in legacy_dir.iterdir(): if item.is_dir() and item.name not in seen_specs: info = self.get_worktree_info(item.name) - # Skip if worktree is on a base branch - base_branches = {"main", "master", "develop", "HEAD"} if info and info.branch not in base_branches: worktrees.append(info) return worktreesapps/frontend/src/renderer/stores/task-store.ts (1)
505-520: Update the shared IPC type definition to match the preload API implementation.The
updateTaskStatusinapps/frontend/src/preload/api/task-api.tshas been correctly updated withcanSkipCleanupandskipCleanupsupport. However, the corresponding type definition inapps/frontend/src/shared/types/ipc.tshas not been updated. The shared type is missing bothcanSkipCleanupin the return type andskipCleanupin the options parameter. Updateapps/frontend/src/shared/types/ipc.tsto include:
- Add
canSkipCleanup?: booleanto the return type- Add
skipCleanup?: booleanto the options parameter
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:
- Around line 526-528: The dynamic require('fs') usage should be removed and
rmSync added to the existing top-level fs imports; update the import that
currently brings in fs functions to also export rmSync, then replace the local
require and calls that use fs.rmSync(worktreePath, { recursive: true, force:
true }) (the occurrences adjacent to the console.warn(`[TASK_UPDATE_STATUS]
Orphaned worktree directory deleted: ${worktreePath}`) and the similar block
later) to call rmSync(worktreePath, { recursive: true, force: true }) directly;
ensure no other require('fs') remains and that worktreePath and the console.warn
usages are unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
.gitignoreapps/backend/core/worktree.pyapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/renderer/components/KanbanBoard.tsxapps/frontend/src/renderer/components/WorktreeCleanupDialog.tsxapps/frontend/src/renderer/stores/task-store.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsxapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/renderer/components/KanbanBoard.tsx
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsxapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/renderer/components/KanbanBoard.tsx
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsxapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/renderer/components/KanbanBoard.tsx
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/worktree.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/worktree.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: When submitting PRs to the upstream AndyMik90/Auto-Claude repository, always target the `develop` branch, not `main`
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Use git worktrees for isolated agent builds with branches following the naming pattern `auto-claude/{spec-name}`
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsxapps/frontend/src/renderer/components/KanbanBoard.tsx
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Store project-specific data including specs, plans, and QA reports in `.auto-claude/specs/` directory and add to .gitignore
Applied to files:
.gitignore
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (`claude-agent-sdk` package) for all AI interactions, never use the Anthropic API directly
Applied to files:
.gitignore
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Use git worktrees for isolated agent builds with branches following the naming pattern `auto-claude/{spec-name}`
Applied to files:
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/backend/core/worktree.py
🧬 Code graph analysis (2)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (3)
apps/frontend/src/main/cli-tool-manager.ts (2)
getToolPath(294-319)getToolPath(1711-1713)apps/frontend/src/shared/types/task.ts (1)
TaskStatus(8-8)apps/frontend/src/main/worktree-paths.ts (1)
findTaskWorktree(50-78)
apps/frontend/src/renderer/components/KanbanBoard.tsx (1)
apps/frontend/src/renderer/stores/task-store.ts (1)
skipCleanupCompleteTask(576-578)
🪛 GitHub Actions: CI
apps/frontend/src/renderer/stores/task-store.ts
[error] 540-540: TS2339: Property 'canSkipCleanup' does not exist on type 'IPCResult & { worktreeExists?: boolean | undefined; worktreePath?: string | undefined; }'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (16)
.gitignore (1)
64-64: LGTM!The
tmpclaude-*pattern is appropriately placed in the Auto Claude Generated section and will correctly ignore temporary files created during worktree operations.apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts (1)
2681-2688: LGTM!The base branch filtering logic is correctly implemented and consistent with the Python backend. The warning log provides useful debugging information when orphaned worktrees are skipped.
apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx (1)
87-98: LGTM!The skip-cleanup button is correctly conditionally rendered only when an error exists and skipping is available. The muted styling appropriately differentiates it from the primary action, and the i18n fallback follows project conventions.
apps/frontend/src/renderer/stores/task-store.ts (2)
571-578: LGTM!The
skipCleanupCompleteTaskhelper follows the same pattern asforceCompleteTaskand provides a clean API for the skip-cleanup workflow. The documentation clearly explains the use case.
539-548: No action needed. The code is correctly typed. Thewindow.electronAPI.updateTaskStatus()return type defined inapps/frontend/src/preload/api/task-api.ts(line 40-44) correctly includes thecanSkipCleanupproperty. The shared types fileapps/frontend/src/shared/types/ipc.tsis not used by the renderer and does not affect this code. No TypeScript error should occur.Likely an incorrect or invalid review comment.
apps/frontend/src/renderer/components/KanbanBoard.tsx (5)
30-30: LGTM!The import is correctly added to support the new skip-cleanup workflow.
343-359: LGTM!The state extension for
canSkipCleanupis well-integrated. The initial state correctly defaults tofalse, and the type annotation properly reflects the optional boolean nature.
497-523: LGTM!The cleanup confirmation handler properly resets
canSkipCleanup: falsewhen starting processing, and correctly propagatesresult.canSkipCleanupon failure to enable the skip option in the UI. The state transitions are consistent.
526-553: LGTM!The
handleWorktreeSkipCleanupfunction correctly mirrors the structure ofhandleWorktreeCleanupConfirm, properly managing the processing state and resetting dialog state on success. The error handling path appropriately shows the error without enabling skip again (avoiding recursive skip attempts).
646-660: LGTM!The
WorktreeCleanupDialogprops are correctly wired:canSkipCleanupis passed from state,onSkipCleanupis bound to the handler, and theonOpenChangecallback properly resetscanSkipCleanupwhen the dialog closes.apps/frontend/src/preload/api/task-api.ts (2)
40-44: LGTM!The interface extension correctly adds
skipCleanupto the options andcanSkipCleanupto the return type, enabling the full skip-cleanup workflow through the IPC layer.
119-124: LGTM!The implementation signature matches the interface, and the options are correctly passed through to the IPC handler.
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (4)
26-52: LGTM!The
getWorktreeBranchhelper is well-designed: it usesexecFileSyncsafely with a timeout, correctly identifies orphaned worktrees by checking for base branches, and returnsnullon any failure to indicate invalid state. The base branch list (main, master, develop, HEAD) covers common scenarios.
570-587: LGTM!The retry logic with 3 attempts and 1-second delays is appropriate for handling Windows file-lock scenarios. The error tracking via
lastErrorensures meaningful error messages if all retries fail.
589-629: LGTM!Good handling of the "not a working tree" edge case where the directory exists but isn't registered with git. The fallback to
rmSyncwith subsequentgit worktree pruneis the correct approach. ReturningcanSkipCleanup: trueon failure allows users to proceed without blocking.
494-667: LGTM on the overall control flow.The structure handles all edge cases correctly:
skipCleanup→ bypass all worktree operations- Orphaned worktree → auto-cleanup and proceed to status update
- Valid worktree +
forceCleanup→ cleanup with retry logic- Valid worktree + no flags → return for user confirmation
The
canSkipCleanupflag is correctly propagated on cleanup failures to enable the UI escape hatch.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:
- Around line 32-52: The base branch comparison in getWorktreeBranch is
case-sensitive, so normalize the branch name before checking; after obtaining
and trimming branch, convert it to a single case (e.g., const normalized =
branch.toLowerCase()) and compare against a lowercased baseBranches array
(['main','master','develop','head']) or lowercased entries using the same
toLowerCase() call to ensure case-insensitive matching and still return null for
base branches.
- Around line 629-639: Add a defensive null/undefined check around the branch
deletion so worktreeBranch is validated before calling execFileSync; inside the
forceCleanup block (where isValidWorktree is true) wrap the
execFileSync(getToolPath('git'), ['branch', '-D', worktreeBranch'], ...) call
with an if (worktreeBranch) guard (or return/continue if missing) and log a
clear warning when worktreeBranch is absent to avoid using an undefined value.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: When submitting PRs to the upstream AndyMik90/Auto-Claude repository, always target the `develop` branch, not `main`
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Use git worktrees for isolated agent builds with branches following the naming pattern `auto-claude/{spec-name}`
🧬 Code graph analysis (1)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (2)
apps/frontend/src/main/cli-tool-manager.ts (2)
getToolPath(294-319)getToolPath(1711-1713)apps/frontend/src/main/worktree-paths.ts (1)
findTaskWorktree(50-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (6)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (6)
5-5: LGTM!The
rmSyncimport is appropriate for the forceful directory removal needed in orphaned worktree cleanup scenarios.
468-481: LGTM!The handler signature changes appropriately extend the API to support the skip cleanup workflow. The
canSkipCleanupresponse field enables the UI to show the skip option when cleanup fails.
494-499: LGTM!The
skipCleanuppath correctly bypasses all worktree checks, allowing users to mark tasks as done when files are locked, as described in the PR objectives.
560-627: Retry logic for Windows file locks is well-implemented.The 3-retry approach with 1-second delays appropriately handles transient file locks. The special handling for "not a working tree" errors (falling back to direct
rmSync) covers edge cases where worktrees are partially created or corrupted.Returning
canSkipCleanup: trueon failures allows the UI to offer the skip option, which aligns with the PR objectives.
641-650: Error message is user-friendly and actionable.The error message at line 648 clearly explains what happened and what the user can do (skip cleanup and remove worktree manually later). This aligns well with the PR's "Skip Cleanup" feature objective.
504-559: Orphaned worktree cleanup is well-structured and correctly implements the branch naming convention.The auto-cleanup logic for orphaned worktrees is sound:
- Tries
git worktree remove --forcefirst- Falls back to
rmSyncfor non-git directories- Attempts branch deletion with graceful fallback
- Prunes stale references
- Doesn't block task completion on cleanup errors
The expected branch pattern
auto-claude/${task.specId}at line 531 correctly matches the naming convention used throughout the codebase for task worktree branches (confirmed at worktree-handlers.ts lines 2112, 2189, and documented in the CLAUDE.md learning note).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/backend/core/worktree.py (1)
694-713: Case-insensitive branch matching not applied.The PR objectives mention "Address case-insensitive branch matching," but the comparison here is case-sensitive. While conventional branch names are lowercase, edge cases (e.g., "Main" or "Develop" on Windows) would bypass this filter.
🔧 Suggested fix for case-insensitive matching
# Skip worktrees on base branches (orphaned after GitHub merge) # But allow any feature branch pattern (auto-claude/*, feature/*, etc.) base_branches = {"main", "master", "develop", "HEAD"} # Check new location first if self.worktrees_dir.exists(): for item in self.worktrees_dir.iterdir(): if item.is_dir(): info = self.get_worktree_info(item.name) - if info and info.branch not in base_branches: + if info and info.branch.lower() not in base_branches: worktrees.append(info) seen_specs.add(item.name) # Check legacy location (.worktrees/) legacy_dir = self.project_dir / ".worktrees" if legacy_dir.exists(): for item in legacy_dir.iterdir(): if item.is_dir() and item.name not in seen_specs: info = self.get_worktree_info(item.name) - if info and info.branch not in base_branches: + if info and info.branch.lower() not in base_branches: worktrees.append(info)Note: "HEAD" is returned verbatim by git for detached HEAD state, so keep it uppercase in the set while using
.lower()only for the branch name comparison. Alternatively, handle "HEAD" separately:if info and info.branch != "HEAD" and info.branch.lower() not in {"main", "master", "develop"}:apps/frontend/src/renderer/components/KanbanBoard.tsx (1)
535-562: InconsistentcanSkipCleanupreset on skip failure.In
handleWorktreeSkipCleanup, when the skip operation fails (lines 556-560),canSkipCleanupis not explicitly reset in the error state update. This differs fromhandleWorktreeCleanupConfirmwhich setscanSkipCleanup: result.canSkipCleanup || falseon failure.If skipping cleanup fails, the "Skip Cleanup" button would still be visible (since
canSkipCleanupretains its previoustruevalue), allowing repeated failed attempts. Consider whether this is intentional behavior or ifcanSkipCleanupshould be reset tofalseon skip failure.🔧 Suggested fix for consistency
} else { setWorktreeCleanupDialog(prev => ({ ...prev, isProcessing: false, - error: result.error || t('dialogs:worktreeCleanup.errorDescription') + error: result.error || t('dialogs:worktreeCleanup.errorDescription'), + canSkipCleanup: false // Reset after skip failed })); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
apps/backend/core/worktree.pyapps/frontend/src/main/__tests__/insights-config.test.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.tsapps/frontend/src/renderer/components/KanbanBoard.tsxapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/shared/types/ipc.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/__tests__/insights-config.test.tsapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/renderer/components/KanbanBoard.tsxapps/frontend/src/shared/types/ipc.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/__tests__/insights-config.test.tsapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/renderer/components/KanbanBoard.tsxapps/frontend/src/shared/types/ipc.ts
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Use Graphiti memory system (mandatory) for cross-session context and knowledge graph management. Access via get_graphiti_memory() from integrations.graphiti.memory module.
Files:
apps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/worktree.py
apps/backend/core/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Files:
apps/backend/core/worktree.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: When submitting PRs to AndyMik90/Auto-Claude upstream, always target the develop branch, NOT main. Commits must include sign-off (git commit -s).
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/backend/core/worktree.py
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/KanbanBoard.tsx
🧬 Code graph analysis (2)
apps/frontend/src/renderer/components/KanbanBoard.tsx (1)
apps/frontend/src/renderer/stores/task-store.ts (1)
skipCleanupCompleteTask(586-588)
apps/backend/core/worktree.py (1)
apps/backend/runners/github/services/pr_worktree_manager.py (1)
get_worktree_info(231-270)
🪛 ast-grep (0.40.4)
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts
[warning] 241-241: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${normalizedTmpDir}${pathSep}\\.claude-token-1234-[0-9a-f]{16}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 286-286: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${normalizedTmpDir}${pathSep}\\.claude-token-5678-[0-9a-f]{16}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (20)
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts (4)
3-3: LGTM!Standard import addition for cross-platform path handling utilities.
104-106: LGTM!Good cross-platform adaptation. The conditional quoting correctly accounts for
buildCdCommand's platform-specific behavior.
239-242: LGTM — static analysis warning is a false positive here.The ReDoS warning from static analysis doesn't apply in this context:
- This is test code with controlled inputs (
tmpdir()is mocked to/tmp).escapeForRegexproperly sanitizes special characters before regex construction.- The pattern structure is simple with no nested quantifiers.
284-287: LGTM!Same cross-platform normalization pattern as the earlier test. The static analysis warning is similarly a false positive for this controlled test context.
apps/frontend/src/main/__tests__/insights-config.test.ts (3)
62-63: LGTM!The use of
path.resolve()for platform-specific path normalization combined withpath.delimiterfor the separator correctly handles cross-platform differences. The added comment clarifies the intent.
88-89: LGTM!Consistent application of
path.resolve()for the single-path case.
99-100: LGTM!Completes the consistent cross-platform path normalization pattern across all test cases.
apps/frontend/src/shared/types/ipc.ts (1)
162-162: LGTM!The type extension correctly adds
skipCleanupto the options parameter andcanSkipCleanupto the return type. This aligns with the backend IPC handler changes and maintains backward compatibility via optional fields.apps/frontend/src/renderer/stores/task-store.ts (3)
515-521: LGTM!The
PersistStatusResultinterface is correctly extended with the optionalcanSkipCleanupfield, maintaining consistency with the IPC return type.
527-570: LGTM!The
persistTaskStatusfunction correctly handles the newcanSkipCleanupcase from the IPC result, returning appropriate failure information to allow the UI to offer the skip cleanup option.
581-588: LGTM!Clean implementation of
skipCleanupCompleteTaskthat delegates topersistTaskStatuswith the appropriate option. The JSDoc clearly explains when this function should be used.apps/frontend/src/renderer/components/KanbanBoard.tsx (4)
30-30: LGTM!Import of
skipCleanupCompleteTaskis correctly added to support the new skip cleanup flow.
351-367: LGTM!Dialog state correctly extended with
canSkipCleanupfield, initialized tofalse. The state shape properly supports the new skip cleanup workflow.
506-532: LGTM!The
handleWorktreeCleanupConfirmhandler properly resetscanSkipCleanupat the start and on success, and correctly propagates thecanSkipCleanupvalue from the result on failure.
654-669: LGTM!The
WorktreeCleanupDialogintegration correctly passes the newcanSkipCleanupandonSkipCleanupprops, and properly resetscanSkipCleanupwhen the dialog is closed.apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (5)
26-54: LGTM!The
getWorktreeBranchhelper is well-implemented:
- Uses
execFileSyncwith a reasonable timeout (10s) to prevent hangs- Case-insensitive comparison for base branches handles edge cases robustly
- Returns
nullfor any git command failure, correctly identifying orphaned/invalid worktrees- Clear JSDoc explaining the purpose and edge case handling
Based on learnings, this aligns with the
auto-claude/{spec-name}branch naming convention.
470-501: LGTM!The handler signature is correctly updated to accept
skipCleanupoption and returncanSkipCleanupin the result. The early return path forskipCleanup(lines 498-500) properly bypasses all worktree operations while still allowing the status update to proceed.
513-562: LGTM - Orphaned worktree cleanup.The orphaned worktree detection and cleanup logic handles the GitHub merge edge case well:
- Detects orphaned state via
getWorktreeBranchreturningnull- Falls back to
rmSyncifgit worktree removefails (line 529)- Attempts branch deletion with graceful handling if branch doesn't exist locally (lines 542-545)
- Prunes stale worktree references
- Doesn't block marking as done even if cleanup fails (correct for orphaned state)
563-656: Solid retry logic and error handling for valid worktree cleanup.The implementation handles Windows file-lock scenarios well:
- Retry loop with 3 attempts and 1-second delays between retries
- Fallback to
rmSyncfor "not a working tree" errors- Returns
canSkipCleanup: trueon cleanup failures, enabling the UI skip flow- Defensive null check for
worktreeBranchbefore deletion (line 634)One minor observation: the error message on line 654 includes a newline and instructions which is good UX.
657-671: LGTM!The remaining branches correctly handle:
- Valid worktree without
forceCleanup: returns confirmation request to UI- No worktree (limbo state): allows marking as done
- End of
!skipCleanupblock properly closes the conditional
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:
- Around line 2681-2689: Extract the inline baseBranches array into a
module-level constant (e.g., BASE_BRANCHES) and replace the local variable
baseBranches in the worktree filter with that constant so all handlers can reuse
the same list; update the check that currently calls
baseBranches.includes(branch.toLowerCase()) to use
BASE_BRANCHES.includes(branch.toLowerCase()) and keep the existing
console.warn([... entry, branch]) behavior unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/components/KanbanBoard.tsx (1)
535-551: Potential issue: Skip button hidden after failed skip attempt.On line 538,
canSkipCleanupis set tofalsebefore processing. IfskipCleanupCompleteTaskfails (lines 544-550),canSkipCleanupis not restored, so the "Skip Cleanup" button will disappear even though the user might want to retry.🐛 Suggested fix
if (result.success) { setWorktreeCleanupDialog(INITIAL_WORKTREE_DIALOG_STATE); } else { setWorktreeCleanupDialog(prev => ({ ...prev, isProcessing: false, - error: result.error || t('dialogs:worktreeCleanup.errorDescription') + error: result.error || t('dialogs:worktreeCleanup.errorDescription'), + canSkipCleanup: true // Preserve ability to retry skip })); }
🤖 Fix all issues with AI agents
In @apps/frontend/src/renderer/components/KanbanBoard.tsx:
- Around line 351-371: Move the static WorktreeCleanupDialogState type and
INITIAL_WORKTREE_DIALOG_STATE constant out of the KanbanBoard component so they
are defined once at module scope; update the component to keep using
useState<WorktreeCleanupDialogState>(INITIAL_WORKTREE_DIALOG_STATE) unchanged,
ensuring referential stability and avoiding recreating
INITIAL_WORKTREE_DIALOG_STATE on every render.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/backend/core/worktree.pyapps/frontend/src/renderer/components/KanbanBoard.tsx
🧰 Additional context used
📓 Path-based instructions (4)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/renderer/components/KanbanBoard.tsx
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/renderer/components/KanbanBoard.tsx
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Use Graphiti memory system (mandatory) for cross-session context and knowledge graph management. Access via get_graphiti_memory() from integrations.graphiti.memory module.
Files:
apps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/worktree.py
apps/backend/core/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Files:
apps/backend/core/worktree.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/KanbanBoard.tsx
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/backend/core/worktree.py
🧬 Code graph analysis (2)
apps/frontend/src/renderer/components/KanbanBoard.tsx (1)
apps/frontend/src/renderer/stores/task-store.ts (2)
forceCompleteTask(577-579)skipCleanupCompleteTask(586-588)
apps/backend/core/worktree.py (1)
apps/backend/runners/github/services/pr_worktree_manager.py (1)
get_worktree_info(231-270)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Security Summary
🔇 Additional comments (6)
apps/backend/core/worktree.py (1)
694-716: LGTM! Well-implemented filtering for orphaned worktrees.The filtering logic correctly:
- Uses a
setfor O(1) membership testing- Applies case-insensitive comparison to handle variations like "Main" or "MASTER"
- Includes "head" to filter out detached HEAD states (common when worktrees become orphaned after remote branch deletion)
- Guards against
Nonewithif info and ...before accessinginfo.branch- Is consistently applied to both new and legacy worktree locations
The comments clearly document the intent, aligning with the PR objective of handling orphaned worktrees after GitHub web UI merges.
apps/frontend/src/renderer/components/KanbanBoard.tsx (5)
30-30: LGTM!The import of
skipCleanupCompleteTaskis correctly added alongside existing task-store functions.
488-496: LGTM!The dialog state is correctly initialized with
error: undefinedandcanSkipCleanup: falsewhen a worktree cleanup is needed.
511-530: LGTM!The
handleWorktreeCleanupConfirmfunction correctly:
- Resets error state and
canSkipCleanupbefore processing- Resets to initial state on success
- Preserves error and propagates
canSkipCleanupfrom the result on failure
644-658: LGTM!The
WorktreeCleanupDialogprops are correctly wired:
canSkipCleanupis passed from stateonSkipCleanupis connected to the handler- Dialog close properly resets
canSkipCleanupalong with other state
1-2: i18n compliance verified.All user-facing strings in the changed code use translation keys via
useTranslation(). No new hardcoded strings were introduced.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:
- Around line 2681-2688: The current includes() check uses a type cast because
branch.toLowerCase() is a string; replace this with a precomputed Set to avoid
casting and get O(1) lookups: create BASE_BRANCHES_SET by mapping BASE_BRANCHES
to lowercased values (at module-level or once in the handler), then change the
guard to use BASE_BRANCHES_SET.has(branch.toLowerCase()) and keep the same
console.warn and return behavior referencing entry and branch.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
apps/backend/core/worktree.pyapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/shared/constants/git.tsapps/frontend/src/shared/constants/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Use Graphiti memory system (mandatory) for cross-session context and knowledge graph management. Access via get_graphiti_memory() from integrations.graphiti.memory module.
Files:
apps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/worktree.py
apps/backend/core/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Files:
apps/backend/core/worktree.py
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/shared/constants/git.tsapps/frontend/src/shared/constants/index.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/shared/constants/git.tsapps/frontend/src/shared/constants/index.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/backend/core/worktree.pyapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧬 Code graph analysis (2)
apps/backend/core/worktree.py (1)
apps/backend/runners/github/services/pr_worktree_manager.py (1)
get_worktree_info(231-270)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (2)
apps/frontend/src/main/cli-tool-manager.ts (2)
getToolPath(294-319)getToolPath(1710-1712)apps/frontend/src/main/worktree-paths.ts (1)
findTaskWorktree(50-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (10)
apps/backend/core/worktree.py (2)
694-698: LGTM: Base branch filtering with cross-language sync note.The filtering logic correctly excludes base branches from worktree listings using case-insensitive comparison. The sync note referencing the TypeScript constant is helpful for maintainability.
One minor consideration: This inline set could be extracted to a module-level constant (e.g.,
BASE_BRANCHES = {"main", "master", "develop", "head"}) for potential reuse in other methods, but the current implementation is functionally correct.
705-707: Verify null-safety ofinfo.branch.The code accesses
info.branch.lower()without a null check. Whileget_worktree_infoshould returnNonefor invalid worktrees (checked on line 705), verify thatinfo.branchis always populated wheninfois notNone.Based on the
WorktreeInfodataclass at lines 150-165,branchis a requiredstrfield with no default, so this should be safe. However, ifget_worktree_infoever returns aWorktreeInfowith an empty branch, the filter would still work correctly (empty string won't match base branches).apps/frontend/src/shared/constants/git.ts (1)
1-15: LGTM: Well-documented cross-language constant.The constant is properly defined with
as constfor type inference, and the derivedBaseBranchtype is idiomatic. The sync note and case-insensitivity guidance in the comments are helpful for maintainers.apps/frontend/src/shared/constants/index.ts (1)
38-40: LGTM: Consistent barrel export.The re-export follows the established pattern for other domain-specific constant modules.
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (6)
26-53: Well-designed helper function for worktree branch detection.The
getWorktreeBranchfunction correctly handles the edge case of orphaned worktrees after GitHub PR merges. Key strengths:
- Proper timeout to prevent hangs
- Returns
nullfor any error/invalid state (defensive)- Case-insensitive base branch comparison
- Appropriate logging for debugging
The type cast on line 44 is consistent with the same pattern used in
worktree-handlers.ts.
493-500: Verify control flow forskipCleanuppath.The
skipCleanupbranch at line 497 logs a warning and has a comment "Fall through to status update - don't check or cleanup worktree". The control flow then exits theif (options?.skipCleanup)block and continues to line 670 which closes theelseblock for!skipCleanup.This means the
skipCleanuppath correctly bypasses all the worktree checking/cleanup logic (lines 501-669) and falls through to the status update logic after line 671. The nesting is complex but correct.Consider adding a brief comment at line 670 to clarify the control flow:
- } // end of else block for !skipCleanup + } // end of worktree cleanup logic (skipped when options.skipCleanup is true)Also applies to: 670-670
512-561: Robust orphaned worktree cleanup with appropriate fallbacks.The orphaned worktree handling is well-designed:
- Tries
git worktree remove --forcefirst (preferred)- Falls back to
rmSyncif git command fails (handles corrupted state)- Attempts to delete the expected branch (may already be gone)
- Runs
git worktree pruneto clean stale referencesThe fallback chain handles various edge cases gracefully. Errors during cleanup don't block marking the task as done, which is appropriate since the worktree was orphaned anyway.
567-629: Retry logic handles Windows file locks and edge cases well.The retry implementation addresses real-world issues:
- 3 retries with 1-second delays for file lock release
- Special handling for "not a working tree" error (partial creation/corruption)
- Returns
canSkipCleanup: trueon persistent failures, allowing UI to offer alternativeOne minor observation: the
await new Promise(resolve => setTimeout(resolve, 1000))at line 585 works correctly in this async handler context.
631-644: Defensive null check for branch deletion is appropriate.The null check on line 633 is defensive programming - at this point
worktreeBranchshould be non-null (since we're in theisValidWorktree === truebranch), but the explicit check prevents potential runtime errors if the code is refactored in the future.The branch deletion ignores errors appropriately since the branch may not exist locally (already merged via GitHub) or may be checked out elsewhere.
648-655: Error message includes actionable guidance for users.The error message at line 653 clearly explains what happened and offers the skip option:
"You can skip cleanup and mark the task as done. The worktree will remain for manual deletion."
This provides good UX by explaining the situation and the available alternative action.
| // Skip if HEAD is detached or pointing to a main/base branch | ||
| // (this can happen if worktree is orphaned after GitHub merge) | ||
| // But allow any feature branch pattern (auto-claude/*, feature/*, etc.) | ||
| // Use case-insensitive comparison for robustness | ||
| if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) { | ||
| console.warn(`[TASK_LIST_WORKTREES] Skipping worktree ${entry} - on base branch: ${branch}`); | ||
| return; // Skip - likely orphaned or misconfigured | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type cast for includes() check is safe but could be cleaner.
The type cast as typeof BASE_BRANCHES[number] is required because toLowerCase() returns string, but includes() on a readonly array expects the exact union type. This is a common TypeScript pattern, though an alternative is to use a type predicate helper or check membership differently.
The early return with warning log is appropriate for filtering orphaned worktrees.
♻️ Optional: Use a Set for O(1) lookup and cleaner type handling
// At module level or within the handler:
const BASE_BRANCHES_SET = new Set(BASE_BRANCHES.map(b => b.toLowerCase()));
// Then in the guard:
if (BASE_BRANCHES_SET.has(branch.toLowerCase())) {
console.warn(`[TASK_LIST_WORKTREES] Skipping worktree ${entry} - on base branch: ${branch}`);
return;
}This avoids the type cast and provides O(1) lookup instead of O(n).
🤖 Prompt for AI Agents
In @apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts around lines
2681 - 2688, The current includes() check uses a type cast because
branch.toLowerCase() is a string; replace this with a precomputed Set to avoid
casting and get O(1) lookups: create BASE_BRANCHES_SET by mapping BASE_BRANCHES
to lowercased values (at module-level or once in the handler), then change the
guard to use BASE_BRANCHES_SET.has(branch.toLowerCase()) and keep the same
console.warn and return behavior referencing entry and branch.
Orinks
left a comment
Orinks
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: ### Merge Verdict: ✅ READY TO MERGE
✅ Ready to merge - All checks passing, no blocking issues found.
No blocking issues. 3 non-blocking suggestion(s) to consider
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Medium | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Low: 3 issue(s)
Generated by Auto Claude PR Review
💡 Suggestions (3)
These are non-blocking suggestions for consideration:
🔵 [7661612f3385] [LOW] Cross-language constant requires manual synchronization
📁 apps/backend/core/worktree.py:698
The base_branches set is defined inline in Python while the TypeScript equivalent BASE_BRANCHES is in a shared constants file. This requires manual synchronization between languages. The code includes a NOTE comment acknowledging this, but there's no build-time validation to ensure they stay in sync.
Suggested fix:
Consider adding a test that validates the Python constant matches the TypeScript constant, or document the manual sync process more formally.
🔵 [7f758f9b377e] [LOW] Best-effort prune operation silently ignores errors
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:135
The pruneWorktrees() function has an empty catch block that silently ignores errors. While this is intentional for best-effort cleanup, adding debug-level logging would help with troubleshooting if prune operations consistently fail.
Suggested fix:
Add debug logging: `catch (e) { console.debug('[pruneWorktrees] Prune failed (non-blocking):', e); }`
🔵 [3b836146924c] [LOW] Consider extracting worktree helpers to dedicated utility module
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:28
The new helper functions (getWorktreeBranch, removeWorktreeWithRetry, pruneWorktrees, deleteBranch, cleanupOrphanedWorktree, cleanupValidWorktree) are defined in execution-handlers.ts. The codebase already has worktree-paths.ts for worktree utilities. Consider consolidating these for better reusability and testability.
Suggested fix:
In a future PR, consider moving these helpers to apps/frontend/src/main/worktree-paths.ts or a new worktree-operations.ts file.
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
MikeeBuilds
added
bug
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 1 CI check(s) failing. Fix CI before merge.
Blocked: 1 CI check(s) failing. Fix CI before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- CI Failed: Require Re-review on Push
Findings Summary
- High: 1 issue(s)
- Medium: 3 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (5 selected of 5 total)
🟠 [b00deac191ad] [HIGH] getWorktreeBranch cannot distinguish git errors from base branches
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:44
The function returns null for both 'on base branch' (line 44-46) and 'git command failed' (line 48-52) cases. If git fails transiently (e.g., lock file, disk I/O error), the code incorrectly treats a valid worktree as orphaned and triggers cleanup. This could delete a working worktree with uncommitted changes.
Suggested fix:
Return a discriminated union: { valid: true, branch: string } vs { valid: false, reason: 'base_branch' | 'git_error', error?: Error }. Only treat 'base_branch' as orphaned, not git errors.
🟡 [b92a830df8e7] [MEDIUM] cleanupOrphanedWorktree silently swallows errors unlike cleanupValidWorktree
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:160
cleanupOrphanedWorktree is a void function that catches all errors (lines 184-186) without notifying the caller. In contrast, cleanupValidWorktree returns WorktreeCleanupResult with canSkipCleanup for user notification. When orphaned cleanup fails (e.g., permission denied), the task is marked 'done' but filesystem resources are leaked with no user notification.
Suggested fix:
Change cleanupOrphanedWorktree to return WorktreeCleanupResult. Update caller at line 678 to check result and potentially show cleanup dialog with canSkipCleanup option.
🟡 [fbd28989bfc3] [MEDIUM] Branch deletion proceeds even if directory cleanup failed
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:174
In cleanupOrphanedWorktree, if rmSync fails (line 176), the catch block at 184 logs the error but code continues to lines 181-182 calling deleteBranch and pruneWorktrees. This leaves inconsistent state: branch deleted but directory still exists. Subsequent worktree creation for same task will fail.
Suggested fix:
Add existsSync check after rmSync to verify directory was deleted. Only call deleteBranch if directory removal succeeded. Consider early return on failure.
🟡 [a7b3bbed1488] [MEDIUM] WorktreeCleanupResult interface should be in shared types
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:56
WorktreeCleanupResult is defined locally in execution-handlers.ts, but the codebase pattern is to define Worktree*Result interfaces in apps/frontend/src/shared/types/task.ts. Examples include WorktreeMergeResult, WorktreeDiscardResult, WorktreeCreatePRResult, and WorktreeListResult. This breaks established patterns and prevents reuse.
Suggested fix:
Move WorktreeCleanupResult to apps/frontend/src/shared/types/task.ts alongside other Worktree*Result types, then import it.
🔵 [4dfef67a1d47] [LOW] handleWorktreeSkipCleanup error case doesn't reset canSkipCleanup
📁 apps/frontend/src/renderer/components/KanbanBoard.tsx:545
In handleWorktreeSkipCleanup, the error state update (lines 545-549) doesn't include canSkipCleanup. If a previous error set canSkipCleanup=true and the skip attempt fails with a different error, the flag retains its stale value. Compare to handleWorktreeCleanupConfirm at line 527 which correctly includes canSkipCleanup.
Suggested fix:
Add canSkipCleanup: result.canSkipCleanup || false to the error state update at line 545-549.
This review was generated by Auto Claude.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:
- Around line 72-128: The duplicate synchronous deletion logic in
removeWorktreeWithRetry and cleanupOrphanedWorktree should be extracted into a
single helper (e.g., deleteWorktreeDirectory or cleanupWorktreeDir) that
encapsulates rmSync removal, logging, pruneWorktrees call, and normalized error
handling; update removeWorktreeWithRetry to call this helper when git reports
"not a working tree" and replace the identical synchronous block in
cleanupOrphanedWorktree with the same helper call so both code paths share
behavior and error messages.
- Around line 166-223: cleanupOrphanedWorktree lacks the Windows file-lock retry
behavior present in removeWorktreeWithRetry; update cleanupOrphanedWorktree to
reuse removeWorktreeWithRetry (or implement the same retry/backoff + clearer
Windows-specific error messages) instead of directly calling
execFileSync/rmSync, so deletion attempts retry on transient locks, and preserve
existing post-delete steps (deleteBranch, pruneWorktrees) only after successful
removal; ensure errors returned include the underlying Error.message and mark
canSkipCleanup=true for lock-related failures.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (2)
apps/frontend/src/shared/constants/git.ts (1)
BASE_BRANCHES(13-13)apps/frontend/src/shared/types/task.ts (1)
WorktreeCleanupResult(443-448)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (6)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (6)
26-33: Well-designed discriminated union type.The
GetWorktreeBranchResulttype properly uses a discriminated union to distinguish between valid branches, orphaned worktrees (on base branch), and git errors. This prevents accidental data loss by ensuring git errors don't trigger cleanup.
45-66: Solid implementation with appropriate error handling.The function correctly:
- Uses case-insensitive comparison for branch matching
- Returns a discriminated union to prevent accidental cleanup on transient git errors
- Sets a reasonable timeout (10s) for the git command
130-159: Best-effort cleanup utilities are appropriate.Both functions correctly handle failures gracefully -
pruneWorktreeslogs debug info, anddeleteBranchlogs a warning. This prevents cleanup failures from blocking the main workflow while still providing diagnostic information.
229-252: LGTM!This function properly delegates to
removeWorktreeWithRetryfor the retry logic and provides a clear error message with actionable guidance when cleanup fails.
694-747: Robust worktree state handling with good edge case coverage.The discriminated union approach effectively handles:
skipCleanup- user-requested bypassgit_error- prevents accidental cleanup on transient failuresbase_branch- auto-cleans orphaned worktrees after GitHub merge- Valid branch with/without
forceCleanup- proper user confirmation flowOne potential issue: on line 720, if
cleanupOrphanedWorktreeis refactored to be async (per earlier suggestion), this call would needawait. Currently it's sync, so this is fine, but keep in mind for future changes.
674-681: Handler signature and return type properly extended.The handler now correctly accepts
skipCleanupoption and returnscanSkipCleanupin the result, enabling the UI to offer the "Skip Cleanup" button when cleanup fails.
| async function removeWorktreeWithRetry( | ||
| projectPath: string, | ||
| worktreePath: string, | ||
| maxRetries = 3 | ||
| ): Promise<WorktreeCleanupResult> { | ||
| let removed = false; | ||
| let lastError: Error | null = null; | ||
|
|
||
| for (let attempt = 1; attempt <= maxRetries && !removed; attempt++) { | ||
| try { | ||
| execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], { | ||
| cwd: projectPath, | ||
| encoding: 'utf-8', | ||
| timeout: 30000 | ||
| }); | ||
| removed = true; | ||
| console.warn(`[removeWorktreeWithRetry] Worktree removed: ${worktreePath}`); | ||
| } catch (err) { | ||
| lastError = err instanceof Error ? err : new Error(String(err)); | ||
| if (attempt < maxRetries) { | ||
| console.warn(`[removeWorktreeWithRetry] Retry ${attempt}/${maxRetries} - waiting before retry...`); | ||
| await new Promise(resolve => setTimeout(resolve, 1000)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!removed && lastError) { | ||
| const errorMessage = lastError.message || String(lastError); | ||
| // Check for "not a working tree" error - directory exists but isn't registered with git | ||
| if (errorMessage.includes('not a working tree') || errorMessage.includes('is not a valid working tree')) { | ||
| console.warn(`[removeWorktreeWithRetry] Directory exists but isn't a git worktree - deleting directly`); | ||
| try { | ||
| rmSync(worktreePath, { recursive: true, force: true }); | ||
| removed = true; | ||
| console.warn(`[removeWorktreeWithRetry] Directory deleted: ${worktreePath}`); | ||
| pruneWorktrees(projectPath); | ||
| } catch (rmError) { | ||
| return { | ||
| success: false, | ||
| canSkipCleanup: true, | ||
| worktreePath, | ||
| error: `Cannot delete worktree directory: ${rmError instanceof Error ? rmError.message : String(rmError)}` | ||
| }; | ||
| } | ||
| } | ||
| if (!removed) { | ||
| return { | ||
| success: false, | ||
| canSkipCleanup: true, | ||
| worktreePath, | ||
| error: `Cannot delete worktree: ${errorMessage}` | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| return { success: true }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good retry logic for handling Windows file locks.
The retry mechanism with exponential backoff (fixed 1s delay) handles transient file lock issues. The fallback to rmSync when git reports "not a working tree" is appropriate for orphaned directory cleanup.
Minor observation: This function is async while cleanupOrphanedWorktree (lines 166-223) has similar deletion logic but is synchronous. Consider extracting the common deletion logic into a shared helper for consistency.
🤖 Prompt for AI Agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts around lines
72 - 128, The duplicate synchronous deletion logic in removeWorktreeWithRetry
and cleanupOrphanedWorktree should be extracted into a single helper (e.g.,
deleteWorktreeDirectory or cleanupWorktreeDir) that encapsulates rmSync removal,
logging, pruneWorktrees call, and normalized error handling; update
removeWorktreeWithRetry to call this helper when git reports "not a working
tree" and replace the identical synchronous block in cleanupOrphanedWorktree
with the same helper call so both code paths share behavior and error messages.
| function cleanupOrphanedWorktree( | ||
| projectPath: string, | ||
| worktreePath: string, | ||
| expectedBranch: string | ||
| ): WorktreeCleanupResult { | ||
| try { | ||
| // Try git worktree remove first, fall back to direct deletion | ||
| try { | ||
| execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], { | ||
| cwd: projectPath, | ||
| encoding: 'utf-8', | ||
| timeout: 30000 | ||
| }); | ||
| console.warn(`[cleanupOrphanedWorktree] Orphaned worktree removed: ${worktreePath}`); | ||
| } catch (gitRemoveError) { | ||
| console.warn(`[cleanupOrphanedWorktree] 'git worktree remove' failed, falling back to directory deletion. Error:`, gitRemoveError); | ||
| try { | ||
| rmSync(worktreePath, { recursive: true, force: true }); | ||
| // Verify the directory was actually deleted | ||
| if (!existsSync(worktreePath)) { | ||
| console.warn(`[cleanupOrphanedWorktree] Orphaned worktree directory deleted: ${worktreePath}`); | ||
| } else { | ||
| console.error(`[cleanupOrphanedWorktree] Directory still exists after rmSync: ${worktreePath}`); | ||
| return { | ||
| success: false, | ||
| canSkipCleanup: true, | ||
| worktreePath, | ||
| error: `Failed to delete worktree directory: ${worktreePath}. Files may be in use.` | ||
| }; | ||
| } | ||
| } catch (rmError) { | ||
| console.error(`[cleanupOrphanedWorktree] rmSync failed:`, rmError); | ||
| return { | ||
| success: false, | ||
| canSkipCleanup: true, | ||
| worktreePath, | ||
| error: `Cannot delete worktree directory: ${rmError instanceof Error ? rmError.message : String(rmError)}` | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // Directory was successfully removed (all failure paths return early above) | ||
| // Now safe to delete the branch and prune stale worktree references | ||
| deleteBranch(projectPath, expectedBranch); | ||
| pruneWorktrees(projectPath); | ||
|
|
||
| console.warn(`[cleanupOrphanedWorktree] Orphaned worktree cleanup completed`); | ||
| return { success: true }; | ||
| } catch (cleanupError) { | ||
| console.error(`[cleanupOrphanedWorktree] Failed to cleanup orphaned worktree:`, cleanupError); | ||
| return { | ||
| success: false, | ||
| canSkipCleanup: true, | ||
| worktreePath, | ||
| error: `Cleanup failed: ${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}` | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing retry logic for Windows file locks.
Unlike removeWorktreeWithRetry, this function doesn't retry on failure. On Windows, files may be temporarily locked by indexers or the application itself. The PR objectives mention "retry logic and clearer error messages for Windows file-lock scenarios" but this function lacks that pattern.
Consider reusing removeWorktreeWithRetry here or adding similar retry logic for consistency:
♻️ Suggested refactor to use async with retry
-function cleanupOrphanedWorktree(
+async function cleanupOrphanedWorktree(
projectPath: string,
worktreePath: string,
expectedBranch: string
-): WorktreeCleanupResult {
+): Promise<WorktreeCleanupResult> {
try {
- // Try git worktree remove first, fall back to direct deletion
- try {
- execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], {
- cwd: projectPath,
- encoding: 'utf-8',
- timeout: 30000
- });
- console.warn(`[cleanupOrphanedWorktree] Orphaned worktree removed: ${worktreePath}`);
- } catch (gitRemoveError) {
- // ... fallback logic
- }
+ // Reuse retry logic from removeWorktreeWithRetry
+ const result = await removeWorktreeWithRetry(projectPath, worktreePath);
+ if (!result.success) {
+ return result;
+ }
+
// Directory was successfully removed
deleteBranch(projectPath, expectedBranch);
pruneWorktrees(projectPath);🤖 Prompt for AI Agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts around lines
166 - 223, cleanupOrphanedWorktree lacks the Windows file-lock retry behavior
present in removeWorktreeWithRetry; update cleanupOrphanedWorktree to reuse
removeWorktreeWithRetry (or implement the same retry/backoff + clearer
Windows-specific error messages) instead of directly calling
execFileSync/rmSync, so deletion attempts retry on transient locks, and preserve
existing post-delete steps (deleteBranch, pruneWorktrees) only after successful
removal; ensure errors returned include the underlying Error.message and mark
canSkipCleanup=true for lock-related failures.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| if (!cleanupResult.success) { | ||
| // Orphaned cleanup failed - notify user but don't block marking as done | ||
| console.warn(`[TASK_UPDATE_STATUS] Orphaned worktree cleanup failed: ${cleanupResult.error}`); | ||
| // Continue to mark as done - orphaned worktree cleanup failure is non-blocking | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (1)
700-751: Consider extracting the worktree handling logic into a helper function.The nested if-else structure for
skipCleanupand worktree state handling adds significant cyclomatic complexity to this handler. Extracting lines 704-750 into a dedicated function likehandleWorktreeCleanupForDone(projectPath, worktreePath, task, options)would improve readability and testability.
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:
- Around line 55-59: The includes check is using an unsafe cast
(branch.toLowerCase() as typeof BASE_BRANCHES[number]); instead, make the check
type-safe by comparing strings: ensure BASE_BRANCHES is treated as a string[]
(or create a lowercased array like const BASE_BRANCHES_LOWER =
BASE_BRANCHES.map(b => b.toLowerCase())) and then use
BASE_BRANCHES_LOWER.includes(branch.toLowerCase()) (remove the type assertion).
Update references to BASE_BRANCHES in this module accordingly and keep the
return behavior the same in the conditional that uses BASE_BRANCHES.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: When submitting PRs to AndyMik90/Auto-Claude upstream, always target the develop branch, NOT main. Commits must include sign-off (git commit -s).
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (3)
apps/frontend/src/shared/constants/git.ts (1)
BASE_BRANCHES(13-13)apps/frontend/src/shared/types/task.ts (2)
WorktreeCleanupResult(443-448)TaskStatus(8-8)apps/frontend/src/main/worktree-paths.ts (1)
findTaskWorktree(50-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (7)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (7)
26-34: LGTM — Good use of discriminated union for type safety.The
GetWorktreeBranchResulttype cleanly distinguishes between valid feature branches, orphaned base branches, and git errors. This enables precise handling in the caller.
72-128: LGTM — Robust retry logic with appropriate fallback.The retry mechanism with 1-second delays addresses Windows file lock scenarios effectively. The fallback to
rmSyncfor directories not registered as git worktrees handles edge cases cleanly, andcanSkipCleanup: trueon failures enables the UI skip-cleanup flow.
130-159: LGTM — Best-effort cleanup helpers.The non-blocking, best-effort approach for
pruneWorktreesanddeleteBranchis appropriate. Force-deleting with-Dis correct here since the branch was likely merged remotely.
184-195: Good defensive verification after rmSync.Verifying the directory was actually deleted after
rmSyncis a valuable safeguard against silent failures where files may remain due to locks or permissions.
229-252: LGTM — Clean async wrapper with helpful error messaging.The error message guidance about skipping cleanup and manual deletion aligns with the PR's UX goals.
694-697: Good fix — Stopping file watcher before cleanup prevents Windows file locks.Moving
fileWatcher.unwatch(taskId)to the start of thedonestatus handler ensures chokidar releases file handles beforermSyncruns, addressing the Windows file-lock scenario mentioned in the commit message.
712-729: Appropriate conservative handling of git errors vs orphaned state.The distinction between
git_error(blocks cleanup to prevent data loss) andbase_branch(auto-cleanup since PR was merged) is correct. Making orphaned cleanup failure non-blocking (line 728) aligns with the skip-cleanup UX goal.
| // Allow any feature branch pattern (auto-claude/*, feature/*, etc.) | ||
| // Use case-insensitive comparison for robustness | ||
| if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) { | ||
| return { valid: false, reason: 'base_branch', branch }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a cleaner type-safe approach for the includes check.
The current type assertion casts an arbitrary string to the literal union type, which suppresses type checking. A safer pattern is to widen the array type instead:
♻️ Suggested improvement
- if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) {
+ if ((BASE_BRANCHES as readonly string[]).includes(branch.toLowerCase())) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Allow any feature branch pattern (auto-claude/*, feature/*, etc.) | |
| // Use case-insensitive comparison for robustness | |
| if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) { | |
| return { valid: false, reason: 'base_branch', branch }; | |
| } | |
| // Allow any feature branch pattern (auto-claude/*, feature/*, etc.) | |
| // Use case-insensitive comparison for robustness | |
| if ((BASE_BRANCHES as readonly string[]).includes(branch.toLowerCase())) { | |
| return { valid: false, reason: 'base_branch', branch }; | |
| } |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts around lines
55 - 59, The includes check is using an unsafe cast (branch.toLowerCase() as
typeof BASE_BRANCHES[number]); instead, make the check type-safe by comparing
strings: ensure BASE_BRANCHES is treated as a string[] (or create a lowercased
array like const BASE_BRANCHES_LOWER = BASE_BRANCHES.map(b => b.toLowerCase()))
and then use BASE_BRANCHES_LOWER.includes(branch.toLowerCase()) (remove the type
assertion). Update references to BASE_BRANCHES in this module accordingly and
keep the return behavior the same in the conditional that uses BASE_BRANCHES.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.gitignoreapps/backend/core/worktree.pyapps/frontend/src/shared/types/ipc.tstests/test_worktree.py
🧰 Additional context used
📓 Path-based instructions (5)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/shared/types/ipc.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/shared/types/ipc.ts
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_worktree.py
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Use Graphiti memory system (mandatory) for cross-session context and knowledge graph management. Access via get_graphiti_memory() from integrations.graphiti.memory module.
Files:
apps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/worktree.py
apps/backend/core/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Files:
apps/backend/core/worktree.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: When submitting PRs to AndyMik90/Auto-Claude upstream, always target the develop branch, NOT main. Commits must include sign-off (git commit -s).
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Store memory data and spec artifacts in .auto-claude/specs/XXX/ directory (gitignored) with structure: spec.md, requirements.json, context.json, implementation_plan.json, qa_report.md, QA_FIX_REQUEST.md, and graphiti/ subdirectory.
Applied to files:
.gitignore
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/backend/core/*.py : Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Applied to files:
.gitignore
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Applied to files:
.gitignore
📚 Learning: 2026-01-11T10:48:12.867Z
Learnt from: g1331
Repo: AndyMik90/Auto-Claude PR: 912
File: tests/test_issue_884_plan_schema.py:287-296
Timestamp: 2026-01-11T10:48:12.867Z
Learning: In test files under tests/, prefer keeping small helper classes (e.g., DummyClient) local to individual test functions when duplication is minimal, tests are integration-style and benefit from self-containment, and tests/conftest.py is already large. Extract helpers to conftest only when duplication across tests is significant or when shared setup is truly reusable across many tests.
Applied to files:
tests/test_worktree.py
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/backend/core/worktree.py
🧬 Code graph analysis (3)
apps/frontend/src/shared/types/ipc.ts (1)
apps/frontend/src/shared/types/task.ts (1)
TaskStatus(8-8)
tests/test_worktree.py (2)
apps/backend/core/worktree.py (1)
WorktreeManager(173-1382)apps/frontend/src/shared/constants/git.ts (1)
BASE_BRANCHES(13-13)
apps/backend/core/worktree.py (1)
apps/frontend/src/shared/constants/git.ts (1)
BASE_BRANCHES(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Seer Code Review
🔇 Additional comments (5)
.gitignore (1)
64-64: LGTM!The
tmpclaude-*pattern correctly ignores temporary Claude-generated files and is appropriately placed in the "Auto Claude Generated" section.apps/backend/core/worktree.py (2)
34-38: LGTM! Well-documented cross-language constant.The
BASE_BRANCHESconstant is correctly defined as afrozensetfor immutability, and the comment clearly documents the synchronization requirement with the TypeScript counterpart. The case-insensitivity note is helpful for maintainers.
728-740: LGTM! Consistent base branch filtering.The filtering logic correctly excludes worktrees on base branches (orphaned after GitHub merge) using case-insensitive comparison. The null guard on
infoprevents potential errors, and the same filtering is applied consistently to both new and legacy worktree locations.tests/test_worktree.py (1)
16-23: LGTM!The new imports (
reandBASE_BRANCHES) are correctly added and used by the cross-language validation test.apps/frontend/src/shared/types/ipc.ts (1)
162-162: LGTM! Well-designed API extension.The
updateTaskStatussignature extension correctly adds:
skipCleanup?: booleanoption to allow marking tasks done without cleanupcanSkipCleanup?: booleanin the return type to signal UI when the skip button should be shownAll new fields are optional, maintaining backward compatibility. This aligns with the PR objective to add a "Skip Cleanup" option when cleanup fails.
| if (!cleanupResult.success) { | ||
| // Orphaned cleanup failed - notify user but don't block marking as done | ||
| console.warn(`[TASK_UPDATE_STATUS] Orphaned worktree cleanup failed: ${cleanupResult.error}`); | ||
| // Continue to mark as done - orphaned worktree cleanup failure is non-blocking | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // No worktree - allow marking as done (limbo state recovery) | ||
| console.warn(`[TASK_UPDATE_STATUS] Allowing status 'done' for task ${taskId} (no worktree found - limbo state)`); | ||
| } | ||
| } // end of else block for !skipCleanup | ||
| } | ||
|
|
||
| // Validate status transition - 'human_review' requires actual work to have been done |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (1)
700-753: Control flow clarity: consider adding a comment for the skip path continuation.The
skipCleanuppath at line 700-702 falls through to the status persistence logic after theelseblock ends at line 753. While functionally correct, a brief comment indicating where execution continues after the emptyif (skipCleanup)block could improve readability for future maintainers.💡 Optional: Add clarifying comment
if (options?.skipCleanup) { console.warn(`[TASK_UPDATE_STATUS] Skipping worktree cleanup for task ${taskId} (user requested)`); - // Fall through to status update - don't check or cleanup worktree + // Skip worktree operations entirely - execution continues after line 753 + // to persist status and handle other transitions } else {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (4)
apps/frontend/src/main/cli-tool-manager.ts (2)
getToolPath(304-329)getToolPath(1720-1722)apps/frontend/src/shared/constants/git.ts (1)
BASE_BRANCHES(13-13)apps/frontend/src/shared/types/task.ts (2)
WorktreeCleanupResult(443-448)TaskStatus(8-8)apps/frontend/src/main/worktree-paths.ts (1)
findTaskWorktree(50-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (9)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (9)
26-66: Well-designed discriminated union for worktree state.The
GetWorktreeBranchResulttype correctly uses a discriminated union pattern to distinguish between valid branches, orphaned state (base branch), and transient git errors. The decision to NOT treat git errors as orphaned state is critical for preventing accidental data loss, which is documented well in the comments.
72-128: Retry logic with fallback looks solid.The retry mechanism with exponential backoff (fixed 1s delay) and fallback to direct
rmSyncfor orphaned directories handles Windows file-lock scenarios well. The error detection for "not a working tree" covers the edge case where the directory exists but isn't registered with git.
130-159: Best-effort cleanup helpers are appropriately non-blocking.Using
console.debugfor prune failures andconsole.warnfor branch deletion failures provides appropriate visibility without halting the cleanup flow. The 30-second timeout is reasonable for these operations.
166-223: Defensive verification after rmSync is a good practice.The check at line 185 verifying the directory was actually deleted after
rmSynccatches edge cases where the deletion might silently fail (e.g., permission issues on some systems). This prevents false-positive success reports.
225-252: LGTM!Clean separation between orphaned and valid worktree cleanup paths. The async wrapper correctly chains the branch deletion after successful worktree removal.
694-753: Early file watcher stop addresses Windows file-locking, but consider the failure path.Stopping the file watcher at line 697 before cleanup is crucial for preventing file-lock issues on Windows. However, if the status update subsequently fails (e.g., due to
git_errorat line 712-719), the watcher remains stopped while the task stays in its previous state.This is likely acceptable since:
- Tasks being marked 'done' are typically not actively running
- The user will see the error and can take corrective action
- Recovery handlers can restart the watcher if needed
If this edge case becomes problematic, consider conditionally re-watching on error paths.
803-809: Proper resource cleanup on auto-stop.Adding
fileWatcher.unwatch(taskId)when auto-stopping tasks ensures consistent resource cleanup across all stop paths. This aligns with the explicit TASK_STOP handler behavior and prevents resource leaks.
680-681: Return type extension is backward compatible.The addition of optional
canSkipCleanupproperty in the return type allows the UI to conditionally render the "Skip Cleanup" option without breaking existing consumers of this IPC handler.
720-731: Orphaned worktree auto-cleanup handles GitHub merge edge case correctly.The detection of orphaned state (worktree on base branch after PR merge) and automatic cleanup addresses the PR's main objective. Returning the cleanup error with
canSkipCleanup: trueenables the UI to offer manual recovery when automatic cleanup fails.
Orinks
left a comment
Orinks
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: ### Merge Verdict: ✅ READY TO MERGE
✅ Ready to merge - All checks passing, no blocking issues found.
No blocking issues. 5 non-blocking suggestion(s) to consider
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | Low | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Low: 5 issue(s)
Generated by Auto Claude PR Review
💡 Suggestions (5)
These are non-blocking suggestions for consideration:
🔵 [8a1b543f597f] [LOW] Duplicated state reset pattern in dialog handlers
📁 apps/frontend/src/renderer/components/KanbanBoard.tsx:511
The state reset logic in handleWorktreeCleanupConfirm and handleWorktreeSkipCleanup is nearly identical, resetting isProcessing, error, and canSkipCleanup to the same values. This could be extracted to a shared helper function for DRY compliance.
Suggested fix:
Extract state reset logic to a shared helper: const resetDialogError = (error, canSkip) => setWorktreeCleanupDialog(prev => ({...prev, isProcessing: false, error, canSkipCleanup: canSkip}))
🔵 [8e3e0aee1ecb] [LOW] Type assertion pattern could be more explicit
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:57
The pattern branch.toLowerCase() as typeof BASE_BRANCHES[number] is used in multiple files to make TypeScript accept the includes() check. While safe, a type guard function would be cleaner and more reusable.
Suggested fix:
Create a type guard: function isBaseBranch(branch: string): branch is BaseBranch { return BASE_BRANCHES.includes(branch.toLowerCase() as BaseBranch); }
🔵 [1593596f0b44] [LOW] Large file growing larger - consider module extraction
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:1
This file has grown by +277 lines. The new worktree cleanup functions (getWorktreeBranch, removeWorktreeWithRetry, cleanupOrphanedWorktree, cleanupValidWorktree, deleteBranch, pruneWorktrees) could be extracted to a dedicated worktree-cleanup.ts module for better maintainability.
Suggested fix:
Extract cleanup-related functions to apps/frontend/src/main/ipc-handlers/task/worktree-cleanup.ts
🔵 [7c704fd7c2aa] [LOW] Consider defense-in-depth validation before rmSync
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:110
The rmSync call with recursive:true relies on upstream path validation in findTaskWorktree. While the upstream validation is sound, adding a secondary check that the path is within the expected .worktrees directory would provide defense-in-depth against future refactoring bugs.
Suggested fix:
Add validation: if (!worktreePath.includes('.worktrees') && !worktreePath.includes('worktrees')) { throw new Error('Invalid worktree path'); }
🔵 [f42e4e450cd7] [LOW] Empty catch without comment in deleteBranch
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:155
The deleteBranch function has a catch block that only logs a warning but does not indicate this is intentional silent failure. Adding a comment or using underscore parameter convention would clarify intent.
Suggested fix:
Add comment: } catch { // Intentional: branch may not exist or be checked out elsewhere
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
|
Interesting. Claude says with the receive code superpowers skill that these five suggestions don't actually worent any changes, so we're ready to merge here. |
|
@Orinks interesting, because there are merge conflicts that needs to be solved. |
|
|
| if (status === 'done') { | ||
| // Stop file watcher BEFORE any cleanup operations to release file handles | ||
| // This prevents file-locking issues on Windows where chokidar holds handles open | ||
| fileWatcher.unwatch(taskId); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… state - Filter worktree listings to only include feature branches (not main/master/develop) - Auto-cleanup orphaned worktrees when marking task as done - Handle "not a working tree" error by deleting directory directly - Add retry logic with better error messages for Windows file locks - Add "Skip Cleanup" option when files are in use (marks done without deleting worktree) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- insights-config.test.ts: Use path.resolve() in expected values to handle platform-specific path normalization - claude-integration-handler.test.ts: Handle platform-specific quoting in buildCdCommand (double quotes on Windows, single on Unix) and use path.join/path.sep for temp file path assertions Co-Authored-By: Claude Opus 4.5 <[email protected]>
…tatus IPC type The updateTaskStatus function returns canSkipCleanup and accepts skipCleanup option but these were missing from the TypeScript type definition. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- worktree.py: Extract duplicated base_branches set to avoid DRY violation - execution-handlers.ts: Add error logging in catch blocks for better debugging - execution-handlers.ts: Use case-insensitive branch matching for robustness - execution-handlers.ts: Add defensive null check for worktreeBranch - KanbanBoard.tsx: Reset canSkipCleanup state for consistency Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Create shared/constants/git.ts with BASE_BRANCHES constant - Update execution-handlers.ts to import from shared constants - Update worktree-handlers.ts to import from shared constants - Add cross-language sync note in Python worktree.py This reduces duplication and makes it easier to maintain the list of base branches across the codebase. Co-Authored-By: Claude Opus 4.5 <[email protected]>
… scope Move the static type and INITIAL_WORKTREE_DIALOG_STATE constant out of the KanbanBoard component to module scope for referential stability, avoiding recreation on every render. Co-Authored-By: Claude Opus 4.5 <[email protected]>
…exity Extract the following helper functions from TASK_UPDATE_STATUS handler: - removeWorktreeWithRetry(): Removes worktree with retry logic for Windows file locks - pruneWorktrees(): Prunes stale worktree references - deleteBranch(): Deletes a git branch with logging - cleanupOrphanedWorktree(): Handles orphaned worktrees after GitHub PR merge - cleanupValidWorktree(): Handles valid worktree cleanup with user confirmation Each function handles a single concern with clear inputs/outputs, improving testability and maintainability. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Extract BASE_BRANCHES to module-level constant in worktree.py - Add debug logging to pruneWorktrees() instead of silent catch - Add TestCrossLanguageConstantValidation test class to validate Python BASE_BRANCHES matches TypeScript BASE_BRANCHES at build time This ensures the cross-language constants stay in sync and provides better debugging visibility for prune operations. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- HIGH: getWorktreeBranch now returns discriminated union to distinguish git errors from base branches, preventing accidental cleanup of valid worktrees with uncommitted changes during transient git failures - MEDIUM: cleanupOrphanedWorktree returns WorktreeCleanupResult instead of void, allowing caller to handle failures appropriately - MEDIUM: Branch deletion only proceeds if directory was successfully removed, preventing inconsistent state - MEDIUM: Move WorktreeCleanupResult to shared types for consistency with other Worktree*Result interfaces - LOW: handleWorktreeSkipCleanup now properly resets canSkipCleanup on error for UI state consistency Co-Authored-By: Claude Opus 4.5 <[email protected]>
The directoryDeleted variable was always true when reaching the conditional because all failure paths return early. Simplified the code by removing the unnecessary variable and conditional check. Co-Authored-By: Claude Opus 4.5 <[email protected]>
…e locks When marking a task as 'done', the file watcher (chokidar) was not stopped before worktree cleanup, causing rmSync() to fail on Windows due to open file handles. This led to cleanup failure dialogs and accumulated orphaned worktree directories. Now fileWatcher.unwatch() is called at the start of the 'done' status handler, consistent with TASK_STOP and recovery handlers. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Previously, orphaned worktree cleanup failures were silently logged and execution continued. This caused: - Users unaware that cleanup failed - Orphaned directories accumulating on disk - Future "already exists" errors when creating new tasks Now orphaned cleanup failures return the error with canSkipCleanup=true, consistent with valid worktree cleanup failures. The UI shows the "Skip Cleanup" option, giving users visibility and control. Co-Authored-By: Claude Opus 4.5 <[email protected]>
When a task's status changes away from 'in_progress' while running, the auto-stop logic killed the process but failed to stop the file watcher. This caused resource leaks (memory, file handles) and potential Windows file locking issues. Now matches the cleanup pattern used in the TASK_STOP handler. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- KanbanBoard.tsx: Extract dialog state to INITIAL_WORKTREE_DIALOG_STATE constant - KanbanBoard.tsx: Add canSkipCleanup: false when opening dialog to prevent stale state - KanbanBoard.tsx: Use constant for dialog reset to avoid duplication - worktree.py: Use case-insensitive branch comparison for consistency Co-Authored-By: Claude Opus 4.5 <[email protected]>
Orinks
force-pushed
the
fix/worktree-github-merge-edge-case
branch
from
35b9e22 to
a4a2ccb
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @apps/backend/core/worktree.py:
- Around line 694-706: Extract the inline base_branches set into a module-level
constant (e.g., BASE_BRANCHES) containing lowercase branch names and a comment
noting it is synced with the TypeScript BASE_BRANCHES; then replace the local
base_branches usage inside list_all_worktrees() with that constant and keep the
existing case-insensitive check (info.branch.lower() not in BASE_BRANCHES).
Ensure the constant name is unique and referenced wherever the same base-branch
logic is used.
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:
- Around line 45-66: The type assertion in getWorktreeBranch masks type-safety
when comparing branch.toLowerCase() to BASE_BRANCHES; replace it with a
type-safe comparison by normalizing BASE_BRANCHES to lowercase (or building a
Set of lowercase base names) and then checking includes using
branch.toLowerCase(), e.g., compute a lowercaseBaseSet = new
Set(BASE_BRANCHES.map(b => b.toLowerCase())) (or lowercaseBaseArray) and use
lowercaseBaseSet.has(branch.toLowerCase()) inside getWorktreeBranch to remove
the unsafe assertion.
- Around line 772-773: Update the handler's declared return type to use the
existing WorktreeCleanupResult shape instead of a separate optional
worktreeExists field; replace the current Promise<IPCResult & { worktreeExists?:
boolean; worktreePath?: string; canSkipCleanup?: boolean }> with
Promise<IPCResult & WorktreeCleanupResult & { worktreePath?: string;
canSkipCleanup?: boolean }> (or simply Promise<IPCResult &
WorktreeCleanupResult> if those extra fields are already covered) so the
signature matches the actual return values from the cleanup functions (which
return WorktreeCleanupResult).
In @apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:
- Around line 2680-2688: The current check uses a type assertion with
BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number]);
replace this with a simple, type-safe helper (e.g., export isBaseBranch(branch:
string): branch is BaseBranch or at minimum isBaseBranch(branch: string):
boolean) that performs a case-insensitive includes against a pre-normalized
BASE_BRANCHES_LOWER array, then call if (isBaseBranch(branch)) {
console.warn(...); return; } to remove the inline type assertion and make the
logic clearer and safer while preserving case-insensitive comparison.
In @apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx:
- Around line 87-98: The UI references a missing translation key used in
WorktreeCleanupDialog: add "skipCleanup": "Mark Done (Keep Worktree)" to the
worktreeCleanup object in apps/frontend/src/shared/i18n/locales/en/dialogs.json
and add the French equivalent (e.g., "skipCleanup": "Marquer comme terminé
(conserver l'arbre de travail)") to the worktreeCleanup object in
apps/frontend/src/shared/i18n/locales/fr/dialogs.json so the
t('dialogs:worktreeCleanup.skipCleanup') lookup resolves in both locales.
In @tests/test_worktree.py:
- Around line 716-761: TestCrossLanguageConstantValidation currently fails when
the Python constant BASE_BRANCHES is missing and uses a fragile
Path(__file__).parent.parent to find the repo root; update the test to attempt
importing BASE_BRANCHES from the Python module (e.g., from
apps.backend.core.worktree import BASE_BRANCHES) and wrap that import with
pytest.importorskip or mark the test pytest.mark.xfail if import fails, and
replace the fragile repo root resolution with
Path(__file__).resolve().parents[2] (or another robust resolution) while keeping
the existing TS regex/compare logic intact.
- Line 23: Add the missing BASE_BRANCHES constant to the worktree module so
imports succeed: define BASE_BRANCHES = ['main', 'master', 'develop', 'head']
near the top of the module (include a short comment like "Cross-language
constant - keep in sync with apps/frontend/src/shared/constants/git.ts") so
WorktreeManager and tests can import it and stay version-synced with the
TypeScript constant.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
.gitignoreapps/backend/core/worktree.pyapps/frontend/src/main/__tests__/insights-config.test.tsapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.tsapps/frontend/src/preload/api/task-api.tsapps/frontend/src/renderer/components/KanbanBoard.tsxapps/frontend/src/renderer/components/WorktreeCleanupDialog.tsxapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/shared/constants/git.tsapps/frontend/src/shared/constants/index.tsapps/frontend/src/shared/types/ipc.tsapps/frontend/src/shared/types/task.tstests/test_worktree.py
🧰 Additional context used
📓 Path-based instructions (5)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/shared/types/task.tsapps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.tsapps/frontend/src/renderer/components/WorktreeCleanupDialog.tsxapps/frontend/src/main/__tests__/insights-config.test.tsapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/shared/types/ipc.tsapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/shared/constants/index.tsapps/frontend/src/shared/constants/git.tsapps/frontend/src/renderer/components/KanbanBoard.tsxapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/preload/api/task-api.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/shared/types/task.tsapps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.tsapps/frontend/src/renderer/components/WorktreeCleanupDialog.tsxapps/frontend/src/main/__tests__/insights-config.test.tsapps/frontend/src/renderer/stores/task-store.tsapps/frontend/src/shared/types/ipc.tsapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/shared/constants/index.tsapps/frontend/src/shared/constants/git.tsapps/frontend/src/renderer/components/KanbanBoard.tsxapps/frontend/src/main/ipc-handlers/task/execution-handlers.tsapps/frontend/src/preload/api/task-api.ts
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_worktree.py
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Use Graphiti memory system (mandatory) for cross-session context and knowledge graph management. Access via get_graphiti_memory() from integrations.graphiti.memory module.
Files:
apps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/worktree.py
apps/backend/core/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Files:
apps/backend/core/worktree.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: When submitting PRs to AndyMik90/Auto-Claude upstream, always target the develop branch, NOT main. Commits must include sign-off (git commit -s).
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/frontend/src/shared/types/task.tsapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/backend/core/worktree.pyapps/frontend/src/main/ipc-handlers/task/execution-handlers.ts
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Store memory data and spec artifacts in .auto-claude/specs/XXX/ directory (gitignored) with structure: spec.md, requirements.json, context.json, implementation_plan.json, qa_report.md, QA_FIX_REQUEST.md, and graphiti/ subdirectory.
Applied to files:
.gitignore
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/backend/core/*.py : Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Applied to files:
.gitignore
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Applied to files:
.gitignore
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsxapps/frontend/src/renderer/components/KanbanBoard.tsx
📚 Learning: 2026-01-11T10:48:12.867Z
Learnt from: g1331
Repo: AndyMik90/Auto-Claude PR: 912
File: tests/test_issue_884_plan_schema.py:287-296
Timestamp: 2026-01-11T10:48:12.867Z
Learning: In test files under tests/, prefer keeping small helper classes (e.g., DummyClient) local to individual test functions when duplication is minimal, tests are integration-style and benefit from self-containment, and tests/conftest.py is already large. Extract helpers to conftest only when duplication across tests is significant or when shared setup is truly reusable across many tests.
Applied to files:
tests/test_worktree.py
🧬 Code graph analysis (4)
apps/frontend/src/shared/types/ipc.ts (2)
scripts/bump-version.js (1)
status(106-106)apps/frontend/src/shared/types/task.ts (1)
TaskStatus(8-8)
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts (1)
apps/frontend/src/shared/constants/git.ts (1)
BASE_BRANCHES(13-13)
tests/test_worktree.py (1)
apps/frontend/src/shared/constants/git.ts (1)
BASE_BRANCHES(13-13)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (2)
apps/frontend/src/shared/constants/git.ts (1)
BASE_BRANCHES(13-13)apps/frontend/src/shared/types/task.ts (2)
WorktreeCleanupResult(443-448)TaskStatus(8-8)
🪛 ast-grep (0.40.5)
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts
[warning] 241-241: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${normalizedTmpDir}${pathSep}\\.claude-token-1234-[0-9a-f]{16}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 286-286: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${normalizedTmpDir}${pathSep}\\.claude-token-5678-[0-9a-f]{16}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: CI
tests/test_worktree.py
[error] 23-23: ImportError: cannot import name 'BASE_BRANCHES' from 'worktree' during test collection.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (33)
apps/frontend/src/shared/types/task.ts (1)
440-448: LGTM!The new
WorktreeCleanupResultinterface is well-structured and follows the established pattern of other result interfaces in this file (e.g.,WorktreeDiscardResult,WorktreeCreatePRResult). ThecanSkipCleanupfield appropriately supports the PR objective for allowing users to skip cleanup when it fails..gitignore (1)
64-64: LGTM! Appropriate pattern for temporary files.The addition of
tmpclaude-*to ignore temporary Claude-related files is well-placed under the "Auto Claude Generated" section and follows the project's naming conventions. The pattern is specific enough to avoid false positives while covering all temporary files with that prefix.apps/frontend/src/main/__tests__/insights-config.test.ts (3)
62-63: LGTM!The use of
path.resolve()for both path components andpath.delimiterfor joining ensures this test works correctly across platforms (Windows uses;, Unix uses:). The comment helpfully documents the normalization behavior.
88-89: LGTM!The comment clarifies the path normalization expectation for this test case.
99-100: LGTM!Consistent with the other test cases; the comment documents the path normalization behavior.
apps/frontend/src/shared/constants/index.ts (1)
38-40: LGTM!The re-export follows the established pattern and correctly exposes the Git-related constants to downstream imports.
apps/backend/core/worktree.py (1)
649-652: Simplified merge failure handling removes nuanced error detection.The previous logic likely checked for specific messages like "already up to date" or "conflict" to provide more targeted responses. The new unconditional abort is simpler but may lose helpful context for users.
If this simplification is intentional (per the AI summary), the implementation is correct. The
--abortensures the repository returns to a clean state after any merge failure.apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts (3)
104-106: LGTM!The platform-aware quoting correctly handles the difference between Windows (double quotes) and Unix (single quotes) for the
cdcommand.
239-242: Static analysis false positive - regex input is properly escaped and trusted.The static analysis warning about ReDoS can be safely dismissed. The
escapeForRegex()helper at Line 9 properly escapes all regex metacharacters ([.*+?^${}()|[\]\\]), and the input values (tmpdir()andpath.sep) are system-controlled, deterministic values—not user input.The cross-platform path handling is correct.
284-287: Same pattern as above - static analysis false positive.The regex construction is safe for the same reasons: properly escaped system values.
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts (1)
2-2: LGTM!The import correctly includes
BASE_BRANCHESfrom the shared constants, enabling consistent filtering across the codebase.apps/frontend/src/shared/constants/git.ts (1)
1-15: Well-documented constants with confirmed cross-language sync.The constants are well-structured with clear documentation and proper TypeScript type inference via
as const. TheBaseBranchtype correctly derives from the const array.The
'head'value is intentional and verified to match the Python backend version inapps/backend/core/worktree.py. The case-insensitive comparison via.toLowerCase()correctly handles matching the detachedHEADstate, ensuring worktrees in detached HEAD state are properly filtered out.apps/frontend/src/shared/types/ipc.ts (1)
163-163: LGTM!The IPC signature update is well-designed:
- Both
skipCleanupin options andcanSkipCleanupin the result are optional, maintaining backward compatibility.- The extended return type correctly mirrors the preload API and task-store implementations.
apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx (1)
20-23: LGTM!The new optional props
canSkipCleanupandonSkipCleanupare correctly typed and follow the existing pattern for optional dialog callbacks.apps/frontend/src/renderer/stores/task-store.ts (4)
516-522: LGTM!The
PersistStatusResultinterface is cleanly extended withcanSkipCleanupto support the new skip-cleanup workflow.
528-532: LGTM!The function signature correctly accepts both
forceCleanupandskipCleanupoptions, aligning with the IPC layer.
550-559: LGTM!The new
canSkipCleanuphandling branch follows the same pattern as the existingworktreeExistscase, providing consistent error handling with actionable information for the UI.
582-589: LGTM!The
skipCleanupCompleteTaskhelper is a clean, single-purpose wrapper that delegates topersistTaskStatuswith the appropriate option. The JSDoc clearly explains the use case.apps/frontend/src/preload/api/task-api.ts (2)
42-46: LGTM!The
TaskAPIinterface correctly extendsupdateTaskStatus:
- Options now include optional
skipCleanupalongsideforceCleanup- Return type includes
canSkipCleanupin the result payloadThis aligns with the IPC type definition in
ipc.ts.
122-127: LGTM!The implementation mirrors the interface changes exactly, ensuring type safety between the preload API and the main process IPC handler.
apps/frontend/src/renderer/components/KanbanBoard.tsx (6)
30-30: LGTM!The import of
skipCleanupCompleteTaskaligns with the new skip cleanup workflow and the function exists in the task-store.
351-371: LGTM!The structured
WorktreeCleanupDialogStatetype andINITIAL_WORKTREE_DIALOG_STATEconstant improve state management clarity. Centralizing the initial state prevents inconsistencies when resetting the dialog.
511-530: LGTM!The updated handler correctly:
- Resets
canSkipCleanupwhen starting a new attempt- Uses the centralized initial state on success
- Propagates
canSkipCleanupfrom the result on failure
532-551: LGTM!The
handleWorktreeSkipCleanupfunction correctly implements the skip cleanup workflow. The omission ofcanSkipCleanupin the failure path is appropriate since there's no further skip option if the skip itself fails.
644-658: LGTM!The dialog integration correctly passes
canSkipCleanupandonSkipCleanupprops. The close handler properly resets all state flags includingcanSkipCleanup.
488-496: LGTM!The initial dialog state correctly sets
canSkipCleanup: false. The skip option should only appear after a cleanup attempt fails withcanSkipCleanup: truein the result.apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (7)
2-5: LGTM!The imports are correctly updated to include
BASE_BRANCHESfor branch filtering,WorktreeCleanupResultfor typed cleanup results, andrmSyncfor directory deletion fallback.
68-128: LGTM!The
removeWorktreeWithRetryfunction implements a robust cleanup strategy:
- Retry loop for transient failures (Windows file locks)
- Fallback to direct
rmSyncfor orphaned directories- Proper
canSkipCleanuppropagation for UI handlingThe fixed 1-second delay between retries is acceptable for file-lock scenarios.
161-223: LGTM!The
cleanupOrphanedWorktreefunction handles the edge case where a PR was merged via GitHub web UI well:
- Tries
git worktree removefirst- Falls back to
rmSyncwith verification- Only proceeds to branch/prune cleanup after confirmed directory removal
- Propagates
canSkipCleanup: trueon all failure paths
225-252: LGTM!The
cleanupValidWorktreefunction properly composes withremoveWorktreeWithRetryand provides clear error messages with guidance about the skip option.
786-845: LGTM!The refactored worktree handling logic is well-structured:
- File watcher unwatch before cleanup - Critical fix for Windows file-lock issues
- skipCleanup path - Clean bypass of all worktree operations
- Three-way branch state handling:
git_error: Returns error without cleanup (prevents data loss on transient failures)base_branch: Auto-cleans orphaned worktrees (PR merged via GitHub)valid: Prompts UI or cleans based onforceCleanupThe discriminated union approach ensures all cases are handled explicitly.
897-901: LGTM!The
fileWatcher.unwatchcall here handles the general case of stopping tasks when status changes away fromin_progress, complementing the early unwatch at line 789 that specifically handles thedonestatus with cleanup.
148-159: Consider logging the error for debugging.The empty
catchblock silently swallows errors. While best-effort is appropriate here, logging the error would help diagnose issues.Add error logging
} catch { - console.warn(`[deleteBranch] Could not delete branch ${branch} (may not exist or be checked out elsewhere)`); + // Intentionally empty - branch may not exist or be checked out elsewhere }Actually, looking again, the warning is already logged. The issue is the catch doesn't capture the error. This is fine as-is since you're logging a generic message. No change needed.
Likely an incorrect or invalid review comment.
| function getWorktreeBranch(worktreePath: string): GetWorktreeBranchResult { | ||
| try { | ||
| const branch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], { | ||
| cwd: worktreePath, | ||
| encoding: 'utf-8', | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| timeout: 10000 | ||
| }).trim(); | ||
|
|
||
| // Skip if on a base branch (orphaned after GitHub merge) | ||
| // Allow any feature branch pattern (auto-claude/*, feature/*, etc.) | ||
| // Use case-insensitive comparison for robustness | ||
| if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) { | ||
| return { valid: false, reason: 'base_branch', branch }; | ||
| } | ||
| return { valid: true, branch }; | ||
| } catch (error) { | ||
| // Git command failed - could be transient (lock file, disk I/O) - do NOT treat as orphaned | ||
| console.warn(`[getWorktreeBranch] Failed to get branch for worktree at ${worktreePath}:`, error); | ||
| return { valid: false, reason: 'git_error', error: error instanceof Error ? error : new Error(String(error)) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type assertion concern on base branch check.
Line 57 uses a type assertion that could mask type errors. The toLowerCase() returns string, but you're asserting it as typeof BASE_BRANCHES[number]. This works because BASE_BRANCHES contains lowercase values, but the assertion bypasses type safety.
Consider a type-safe alternative
- if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) {
+ const lowerBranch = branch.toLowerCase();
+ if ((BASE_BRANCHES as readonly string[]).includes(lowerBranch)) {This avoids the assertion while maintaining the same behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getWorktreeBranch(worktreePath: string): GetWorktreeBranchResult { | |
| try { | |
| const branch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], { | |
| cwd: worktreePath, | |
| encoding: 'utf-8', | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| timeout: 10000 | |
| }).trim(); | |
| // Skip if on a base branch (orphaned after GitHub merge) | |
| // Allow any feature branch pattern (auto-claude/*, feature/*, etc.) | |
| // Use case-insensitive comparison for robustness | |
| if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) { | |
| return { valid: false, reason: 'base_branch', branch }; | |
| } | |
| return { valid: true, branch }; | |
| } catch (error) { | |
| // Git command failed - could be transient (lock file, disk I/O) - do NOT treat as orphaned | |
| console.warn(`[getWorktreeBranch] Failed to get branch for worktree at ${worktreePath}:`, error); | |
| return { valid: false, reason: 'git_error', error: error instanceof Error ? error : new Error(String(error)) }; | |
| } | |
| } | |
| function getWorktreeBranch(worktreePath: string): GetWorktreeBranchResult { | |
| try { | |
| const branch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], { | |
| cwd: worktreePath, | |
| encoding: 'utf-8', | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| timeout: 10000 | |
| }).trim(); | |
| // Skip if on a base branch (orphaned after GitHub merge) | |
| // Allow any feature branch pattern (auto-claude/*, feature/*, etc.) | |
| // Use case-insensitive comparison for robustness | |
| const lowerBranch = branch.toLowerCase(); | |
| if ((BASE_BRANCHES as readonly string[]).includes(lowerBranch)) { | |
| return { valid: false, reason: 'base_branch', branch }; | |
| } | |
| return { valid: true, branch }; | |
| } catch (error) { | |
| // Git command failed - could be transient (lock file, disk I/O) - do NOT treat as orphaned | |
| console.warn(`[getWorktreeBranch] Failed to get branch for worktree at ${worktreePath}:`, error); | |
| return { valid: false, reason: 'git_error', error: error instanceof Error ? error : new Error(String(error)) }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts around lines
45 - 66, The type assertion in getWorktreeBranch masks type-safety when
comparing branch.toLowerCase() to BASE_BRANCHES; replace it with a type-safe
comparison by normalizing BASE_BRANCHES to lowercase (or building a Set of
lowercase base names) and then checking includes using branch.toLowerCase(),
e.g., compute a lowercaseBaseSet = new Set(BASE_BRANCHES.map(b =>
b.toLowerCase())) (or lowercaseBaseArray) and use
lowercaseBaseSet.has(branch.toLowerCase()) inside getWorktreeBranch to remove
the unsafe assertion.
| options?: { forceCleanup?: boolean; skipCleanup?: boolean } | ||
| ): Promise<IPCResult & { worktreeExists?: boolean; worktreePath?: string; canSkipCleanup?: boolean }> => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the WorktreeCleanupResult type definition
rg "interface WorktreeCleanupResult|type WorktreeCleanupResult" -A 5Repository: AndyMik90/Auto-Claude
Length of output: 432
🏁 Script executed:
# Check the file and examine the specific lines mentioned
wc -l apps/frontend/src/main/ipc-handlers/task/execution-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 132
🏁 Script executed:
# Look at the return type declaration and surrounding context
sed -n '770,775p' apps/frontend/src/main/ipc-handlers/task/execution-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 414
🏁 Script executed:
# Check the actual return statements at lines 822 and 829
sed -n '820,835p' apps/frontend/src/main/ipc-handlers/task/execution-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 1031
🏁 Script executed:
# Find IPCResult type definition
rg "interface IPCResult|type IPCResult" -A 5Repository: AndyMik90/Auto-Claude
Length of output: 942
🏁 Script executed:
# Get the complete return statement in the UI prompt case (lines 835-850)
sed -n '835,850p' apps/frontend/src/main/ipc-handlers/task/execution-handlers.tsRepository: AndyMik90/Auto-Claude
Length of output: 832
🏁 Script executed:
# Also check what cleanupOrphanedWorktree and cleanupValidWorktree return
rg "function cleanupOrphanedWorktree|function cleanupValidWorktree|const cleanupOrphanedWorktree|const cleanupValidWorktree" -A 3Repository: AndyMik90/Auto-Claude
Length of output: 778
🏁 Script executed:
# Get the full function signatures for the cleanup functions
sed -n '/^function cleanupOrphanedWorktree/,/^}/p' apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts | head -20Repository: AndyMik90/Auto-Claude
Length of output: 889
🏁 Script executed:
# Get return type annotation for cleanupOrphanedWorktree
rg "function cleanupOrphanedWorktree" -A 20 | grep -E "function cleanupOrphanedWorktree|Promise|return" | head -10Repository: AndyMik90/Auto-Claude
Length of output: 161
🏁 Script executed:
# Check the full function to see what it returns
rg "cleanupOrphanedWorktree\(|cleanupValidWorktree\(" -B 2 -A 15 | head -50Repository: AndyMik90/Auto-Claude
Length of output: 5270
Align handler return type with WorktreeCleanupResult structure.
The handler declares a return type including worktreeExists?: boolean, but the cleanup functions return WorktreeCleanupResult directly (lines 822, 829), which doesn't include that field. While this works because worktreeExists is optional, the type signature doesn't accurately reflect what WorktreeCleanupResult contains. Either update the return type to match WorktreeCleanupResult or ensure all return paths include worktreeExists.
🤖 Prompt for AI Agents
In @apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts around lines
772 - 773, Update the handler's declared return type to use the existing
WorktreeCleanupResult shape instead of a separate optional worktreeExists field;
replace the current Promise<IPCResult & { worktreeExists?: boolean;
worktreePath?: string; canSkipCleanup?: boolean }> with Promise<IPCResult &
WorktreeCleanupResult & { worktreePath?: string; canSkipCleanup?: boolean }> (or
simply Promise<IPCResult & WorktreeCleanupResult> if those extra fields are
already covered) so the signature matches the actual return values from the
cleanup functions (which return WorktreeCleanupResult).
|
|
||
| // Skip if HEAD is detached or pointing to a main/base branch | ||
| // (this can happen if worktree is orphaned after GitHub merge) | ||
| // But allow any feature branch pattern (auto-claude/*, feature/*, etc.) | ||
| // Use case-insensitive comparison for robustness | ||
| if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) { | ||
| console.warn(`[TASK_LIST_WORKTREES] Skipping worktree ${entry} - on base branch: ${branch}`); | ||
| return; // Skip - likely orphaned or misconfigured | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Correct filtering logic for orphaned worktrees.
The guard correctly skips worktrees on base branches using case-insensitive comparison. The console.warn provides helpful debugging information.
One minor type observation: The type assertion as typeof BASE_BRANCHES[number] works but could be simplified by using a type-safe approach:
✨ Optional: Avoid type assertion
- if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) {
+ const lowerBranch = branch.toLowerCase();
+ if ((BASE_BRANCHES as readonly string[]).includes(lowerBranch)) {Or create a type guard helper in the constants file:
export const isBaseBranch = (branch: string): branch is BaseBranch =>
BASE_BRANCHES.includes(branch.toLowerCase() as BaseBranch);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Skip if HEAD is detached or pointing to a main/base branch | |
| // (this can happen if worktree is orphaned after GitHub merge) | |
| // But allow any feature branch pattern (auto-claude/*, feature/*, etc.) | |
| // Use case-insensitive comparison for robustness | |
| if (BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number])) { | |
| console.warn(`[TASK_LIST_WORKTREES] Skipping worktree ${entry} - on base branch: ${branch}`); | |
| return; // Skip - likely orphaned or misconfigured | |
| } | |
| const lowerBranch = branch.toLowerCase(); | |
| if ((BASE_BRANCHES as readonly string[]).includes(lowerBranch)) { | |
| console.warn(`[TASK_LIST_WORKTREES] Skipping worktree ${entry} - on base branch: ${branch}`); | |
| return; // Skip - likely orphaned or misconfigured | |
| } |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts around lines
2680 - 2688, The current check uses a type assertion with
BASE_BRANCHES.includes(branch.toLowerCase() as typeof BASE_BRANCHES[number]);
replace this with a simple, type-safe helper (e.g., export isBaseBranch(branch:
string): branch is BaseBranch or at minimum isBaseBranch(branch: string):
boolean) that performs a case-insensitive includes against a pre-normalized
BASE_BRANCHES_LOWER array, then call if (isBaseBranch(branch)) {
console.warn(...); return; } to remove the inline type assertion and make the
logic clearer and safer while preserving case-insensitive comparison.
| {error && canSkipCleanup && onSkipCleanup && ( | ||
| <AlertDialogAction | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| onSkipCleanup(); | ||
| }} | ||
| disabled={isProcessing} | ||
| className="bg-muted text-muted-foreground hover:bg-muted/80" | ||
| > | ||
| <CheckCircle2 className="mr-2 h-4 w-4" /> | ||
| {t('dialogs:worktreeCleanup.skipCleanup', 'Mark Done (Keep Worktree)')} | ||
| </AlertDialogAction> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the structure of the i18n/locale directories
find apps/frontend/src/shared/i18n -type d -name "locales" 2>/dev/null | head -5Repository: AndyMik90/Auto-Claude
Length of output: 102
🏁 Script executed:
# Search for the specific translation key in JSON files
rg '"skipCleanup"' --type json apps/frontend/src/shared/i18n/ 2>/dev/null || echo "Key not found"Repository: AndyMik90/Auto-Claude
Length of output: 78
🏁 Script executed:
# Also search for worktreeCleanup in dialogs locale files
rg 'worktreeCleanup' --type json apps/frontend/src/shared/i18n/ 2>/dev/null || echo "worktreeCleanup not found"Repository: AndyMik90/Auto-Claude
Length of output: 1246
🏁 Script executed:
# List all locale files to understand the structure
fd -e json . apps/frontend/src/shared/i18n/ | head -20Repository: AndyMik90/Auto-Claude
Length of output: 1155
🏁 Script executed:
# View the worktreeCleanup section in English dialogs
rg -A 10 '"worktreeCleanup"' apps/frontend/src/shared/i18n/locales/en/dialogs.jsonRepository: AndyMik90/Auto-Claude
Length of output: 655
🏁 Script executed:
# View the worktreeCleanup section in French dialogs
rg -A 10 '"worktreeCleanup"' apps/frontend/src/shared/i18n/locales/fr/dialogs.jsonRepository: AndyMik90/Auto-Claude
Length of output: 717
Add missing translation key dialogs:worktreeCleanup.skipCleanup to all language files.
The translation key referenced in the code does not exist in the locale files. Add "skipCleanup": "Mark Done (Keep Worktree)" to apps/frontend/src/shared/i18n/locales/en/dialogs.json under the worktreeCleanup object, and provide the French translation in apps/frontend/src/shared/i18n/locales/fr/dialogs.json.
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/WorktreeCleanupDialog.tsx around lines
87 - 98, The UI references a missing translation key used in
WorktreeCleanupDialog: add "skipCleanup": "Mark Done (Keep Worktree)" to the
worktreeCleanup object in apps/frontend/src/shared/i18n/locales/en/dialogs.json
and add the French equivalent (e.g., "skipCleanup": "Marquer comme terminé
(conserver l'arbre de travail)") to the worktreeCleanup object in
apps/frontend/src/shared/i18n/locales/fr/dialogs.json so the
t('dialogs:worktreeCleanup.skipCleanup') lookup resolves in both locales.
| class TestCrossLanguageConstantValidation: | ||
| """Tests to ensure Python and TypeScript constants stay in sync.""" | ||
|
|
||
| def test_base_branches_matches_typescript(self): | ||
| """Validate Python BASE_BRANCHES matches TypeScript BASE_BRANCHES constant. | ||
| This test ensures the cross-language constant synchronization documented in | ||
| apps/backend/core/worktree.py and apps/frontend/src/shared/constants/git.ts | ||
| is maintained. | ||
| """ | ||
| # Navigate from tests/ to repo root, then to frontend constants | ||
| repo_root = Path(__file__).parent.parent | ||
| ts_file = repo_root / "apps" / "frontend" / "src" / "shared" / "constants" / "git.ts" | ||
|
|
||
| assert ts_file.exists(), ( | ||
| f"TypeScript constants file not found: {ts_file}\n" | ||
| f"Expected path relative to repo root: apps/frontend/src/shared/constants/git.ts" | ||
| ) | ||
|
|
||
| # Read and parse TypeScript file | ||
| ts_content = ts_file.read_text() | ||
|
|
||
| # Extract BASE_BRANCHES array from TypeScript using regex | ||
| # Matches: export const BASE_BRANCHES = ['main', 'master', 'develop', 'head'] as const; | ||
| match = re.search( | ||
| r"export\s+const\s+BASE_BRANCHES\s*=\s*\[(.*?)\]\s*as\s+const", | ||
| ts_content, | ||
| re.DOTALL | ||
| ) | ||
|
|
||
| assert match, "Could not find BASE_BRANCHES constant in TypeScript file" | ||
|
|
||
| # Parse the array contents (extract quoted strings) | ||
| array_content = match.group(1) | ||
| ts_branches = set(re.findall(r"['\"](\w+)['\"]", array_content)) | ||
|
|
||
| # Compare with Python constant (case-insensitive since both use .toLowerCase()/.lower()) | ||
| py_branches = set(BASE_BRANCHES) | ||
|
|
||
| assert ts_branches == py_branches, ( | ||
| f"BASE_BRANCHES mismatch between Python and TypeScript!\n" | ||
| f"Python: {sorted(py_branches)}\n" | ||
| f"TypeScript: {sorted(ts_branches)}\n" | ||
| f"Missing in Python: {sorted(ts_branches - py_branches)}\n" | ||
| f"Missing in TypeScript: {sorted(py_branches - ts_branches)}" | ||
| ) |
There was a problem hiding this comment.
Test class logic is sound but depends on missing Python constant.
The cross-language validation approach is excellent for ensuring TypeScript and Python constants stay synchronized. However:
- The regex pattern correctly extracts the TypeScript array from
export const BASE_BRANCHES = [...] as const; - The set comparison with detailed error messages is well-structured
- The path calculation
Path(__file__).parent.parentmay be fragile if test location changes
Consider using pytest.importorskip or a fixture to gracefully handle the missing import, or mark this test as xfail until the Python constant is added.
🔧 Suggested improvements
+import pytest
+
class TestCrossLanguageConstantValidation:
"""Tests to ensure Python and TypeScript constants stay in sync."""
+ @pytest.mark.skipif(
+ not hasattr(__import__('worktree'), 'BASE_BRANCHES'),
+ reason="BASE_BRANCHES not yet exported from worktree module"
+ )
def test_base_branches_matches_typescript(self):Or use a more robust path resolution:
- repo_root = Path(__file__).parent.parent
+ # More robust: find repo root by looking for a known marker file
+ test_dir = Path(__file__).parent
+ repo_root = test_dir.parent
+ # Validate we found the correct root
+ assert (repo_root / "apps").exists(), f"Could not find repo root from {test_dir}"🤖 Prompt for AI Agents
In @tests/test_worktree.py around lines 716 - 761,
TestCrossLanguageConstantValidation currently fails when the Python constant
BASE_BRANCHES is missing and uses a fragile Path(__file__).parent.parent to find
the repo root; update the test to attempt importing BASE_BRANCHES from the
Python module (e.g., from apps.backend.core.worktree import BASE_BRANCHES) and
wrap that import with pytest.importorskip or mark the test pytest.mark.xfail if
import fails, and replace the fragile repo root resolution with
Path(__file__).resolve().parents[2] (or another robust resolution) while keeping
the existing TS regex/compare logic intact.
Add module-level BASE_BRANCHES constant to match the frontend TypeScript constant. This fixes the test import error and enables cross-language constant validation. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/backend/core/worktree.py:
- Around line 654-657: The merge failure branch in the method that runs git
(checking result.returncode) currently prints only "Merge conflict! Aborting
merge..." and discards result.stderr; update this branch to preserve and surface
the original git error by including result.stderr (or result.stdout if stderr is
empty) in the message or log, and then call self._run_git(["merge", "--abort"])
and return False as before; reference the variables result.returncode and
result.stderr and the helper method self._run_git to locate the change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/core/worktree.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Use Graphiti memory system (mandatory) for cross-session context and knowledge graph management. Access via get_graphiti_memory() from integrations.graphiti.memory module.
Files:
apps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/worktree.py
apps/backend/core/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Files:
apps/backend/core/worktree.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: When submitting PRs to AndyMik90/Auto-Claude upstream, always target the develop branch, NOT main. Commits must include sign-off (git commit -s).
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/backend/core/worktree.py
🧬 Code graph analysis (1)
apps/backend/core/worktree.py (1)
apps/frontend/src/shared/constants/git.ts (1)
BASE_BRANCHES(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (3)
apps/backend/core/worktree.py (3)
34-38: LGTM!The
frozensetis appropriate for an immutable module-level constant, and the values match the frontendBASE_BRANCHESconstant exactly. The comment documenting the cross-file sync requirement is helpful for maintainability. Based on learnings, the test suite validates cross-language constant consistency.
705-708: LGTM!The case-insensitive filtering correctly excludes orphaned worktrees (those pointing to base branches after a GitHub PR merge). The
.lower()call ensures branch names like "Main" or "MAIN" are handled correctly.
716-718: LGTM!Consistent filtering applied to the legacy worktree location, ensuring both paths behave identically.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/backend/core/worktree.py:
- Around line 654-658: The long line constructing error_detail in the merge
failure branch violates Ruff formatting; refactor the assignment in the if block
inside the function that checks result.returncode (refer to result.returncode,
error_detail and the subsequent self._run_git(["merge", "--abort"]) call) by
breaking the expression into multiple shorter lines (e.g., compute stderr_trim =
result.stderr.strip(), stdout_trim = result.stdout.strip() and then set
error_detail = stderr_trim or stdout_trim or "Unknown error") so the line length
conforms to Ruff; run ruff format or apply the multiline change and ensure the
print(f"Merge failed: {error_detail}") remains unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/core/worktree.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Use Graphiti memory system (mandatory) for cross-session context and knowledge graph management. Access via get_graphiti_memory() from integrations.graphiti.memory module.
Files:
apps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/worktree.py
apps/backend/core/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Files:
apps/backend/core/worktree.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: When submitting PRs to AndyMik90/Auto-Claude upstream, always target the develop branch, NOT main. Commits must include sign-off (git commit -s).
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use git worktrees for isolated spec builds with branch naming convention auto-claude/{spec-name}. Keep all branches LOCAL until user explicitly pushes.
Applied to files:
apps/backend/core/worktree.py
🧬 Code graph analysis (1)
apps/backend/core/worktree.py (1)
apps/frontend/src/shared/constants/git.ts (1)
BASE_BRANCHES(13-13)
🪛 GitHub Actions: Lint
apps/backend/core/worktree.py
[error] 652-654: Ruff formatting check failed. Run 'ruff format apps/backend/' to fix code style issues in this file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
apps/backend/core/worktree.py (2)
34-37: LGTM! Well-structured constant with cross-language sync note.Using
frozensetis a good choice for an immutable constant with O(1) membership testing. The sync comment with the TypeScript location is helpful for maintainability.
706-719: LGTM! Correct handling of orphaned worktrees.The filtering logic correctly excludes worktrees that have reverted to base branches after their feature branch was deleted (e.g., via GitHub web UI merge). The case-insensitive comparison with
.lower()is appropriate, and both new and legacy location paths are handled consistently.
Previously the merge failure handler printed only "Merge conflict! Aborting merge..." which discarded the actual git error. Now extracts and displays the error from stderr (or stdout as fallback). Co-Authored-By: Claude Opus 4.5 <[email protected]>
Orinks
force-pushed
the
fix/worktree-github-merge-edge-case
branch
from
943ca1f to
62ca32f
Compare
Orinks
left a comment
Orinks
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 1 CI check(s) failing. Fix CI before merge.
Blocked: 1 CI check(s) failing. Fix CI before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- CI Failed: Require Re-review on Push
Findings Summary
- Low: 3 issue(s)
Generated by Auto Claude PR Review
Findings (3 selected of 3 total)
🔵 [13725e7089dc] [LOW] Constant recreated on every render
📁 apps/frontend/src/renderer/components/KanbanBoard.tsx:361
INITIAL_WORKTREE_DIALOG_STATE is defined inside the KanbanBoard component, causing it to be recreated on every render. While functionally correct, this is wasteful and inconsistent with React best practices for static constants.
Suggested fix:
Move INITIAL_WORKTREE_DIALOG_STATE and WorktreeCleanupDialogState type definition outside the component to module level.
🔵 [97ac76b04481] [LOW] cleanupOrphanedWorktree lacks retry logic unlike cleanupValidWorktree
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:166
The cleanupOrphanedWorktree function does not have retry logic for handling Windows file locks, while cleanupValidWorktree uses removeWorktreeWithRetry which has 3 retries with 1 second delay. This inconsistency means orphaned worktree cleanup is more likely to fail on Windows when files are temporarily locked.
Suggested fix:
Consider extracting the retry logic from removeWorktreeWithRetry into a shared helper, or call removeWorktreeWithRetry from cleanupOrphanedWorktree.
🔵 [2260159d75f5] [LOW] Inconsistent log level in pruneWorktrees
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:141
pruneWorktrees uses console.debug() while all other helper functions (getWorktreeBranch, removeWorktreeWithRetry, deleteBranch, cleanupOrphanedWorktree, cleanupValidWorktree) use console.warn() for their log messages. This inconsistency may make debugging harder if prune failures need investigation.
Suggested fix:
Change console.debug to console.warn for consistency with other helper functions in this file.
This review was generated by Auto Claude.
4 participants
Summary
Problem
When a PR is merged via the GitHub web UI:
Solution
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.