Skip to content

fix(worktrees): open existing worktree when branch is already attached#906

Merged
pedramamini merged 4 commits intoRunMaestro:rcfrom
chr1syy:fix/worktree-in-use
Apr 27, 2026
Merged

fix(worktrees): open existing worktree when branch is already attached#906
pedramamini merged 4 commits intoRunMaestro:rcfrom
chr1syy:fix/worktree-in-use

Conversation

@chr1syy
Copy link
Copy Markdown
Contributor

@chr1syy chr1syy commented Apr 26, 2026

Summary

When git worktree add fails because the branch is already attached to another worktree on disk (e.g., a stale registration that survives disk/UI cleanup), Maestro currently surfaces an opaque "Failed to Create Worktree" toast and there's no path to recover. This change resolves the existing worktree path via git worktree list --porcelain and opens it as a session instead, with a clear "Worktree Already Existed" toast.

  • Detects the "already used / already checked out" stderr in worktreeSetup (local + SSH) and resolves the existing path from porcelain output.
  • Threads alreadyExisted / existingPath through the IPC result type (preload/git.ts, renderer/global.d.ts).
  • Renderer flows (handleCreateWorktreeFromConfig, handleCreateWorktree, Auto Run spawnWorktreeAgentAndDispatch) open or focus the resolved worktree, reusing an already-open child session when one matches and keeping createPROnCompletion config intact.

Behavior

Scenario Before After
User clicks Create New Worktree, branch already attached at another path "Failed to Create Worktree" toast, blocked Session opened against the existing path with info toast
Existing worktree already open as a child session "Failed to Create Worktree" toast Existing session focused, no duplicate created
Unrelated git worktree add failure (permission, etc.) Error toast Error toast (unchanged — porcelain lookup is skipped)

Test plan

  • npx vitest run for: shared/gitUtils.test.ts, main/ipc/handlers/git.test.ts, main/utils/remote-git.test.ts, renderer/hooks/useWorktreeHandlers.test.ts, renderer/hooks/batch/useAutoRunHandlers.worktree.test.ts, renderer/hooks/useAutoRunHandlers.test.ts, main/preload/git.test.ts — 426 passing.
  • New tests cover: porcelain parser (CRLF, detached worktrees, multiple branches), "already used" detection (modern + legacy + case-insensitive), local + SSH alreadyExisted recovery, porcelain miss falls through to error toast, unrelated git failures don't trigger porcelain lookup, renderer paths build sessions against existingPath, dedup against an already-open session.
  • npm run lint — 0 new TypeScript errors in changed files (the 34 pre-existing @xterm/* / js-yaml errors also exist on main and are unrelated).
  • Prettier check clean across all changed files.
  • Manual: in a repo with a stale worktree registration, open Create New Worktree with that branch name and confirm the existing worktree opens as a session.

Notes

The companion bug "all worktree children disappear after app restart" was investigated but is out of scope here — it likely lives in useWorktreeHandlers.scanWorktreeConfigs stale-removal logic (useWorktreeHandlers.ts:545-553) when the parent's worktreeConfig.basePath is missing/mismatched. Tracking separately.

Summary by CodeRabbit

  • Bug Fixes

    • Improved recovery when creating a worktree already checked out elsewhere (local or remote): the app can reuse the existing worktree and avoid surfacing failures.
    • Ignored stale worktree locations to prevent incorrect recovery.
  • New Features

    • Prevent duplicate sessions by detecting and reusing already-open worktree-backed sessions.
    • Added info toasts: “Worktree Already Existed” and “Worktree Already Open”.
  • Chores

    • Standardized API return shapes for worktree operations.

When `git worktree add` fails because the branch is already attached to
another worktree on disk (e.g., a stale registration that survives
disk/UI cleanup), Maestro now resolves the existing worktree path via
`git worktree list --porcelain` and opens it as a session instead of
surfacing an opaque "Failed to Create Worktree" error.

Adds `isWorktreeAlreadyUsedError` and `parseWorktreePathForBranch` to
shared gitUtils, threads `alreadyExisted` / `existingPath` through the
local + SSH worktreeSetup result, and updates the renderer flows
(handleCreateWorktreeFromConfig, handleCreateWorktree, and
spawnWorktreeAgentAndDispatch) to open or focus the resolved worktree
with an info toast. Reuses an already-open child session when one
matches the resolved path, keeping PR-on-completion config intact.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 26, 2026

📝 Walkthrough

Walkthrough

Adds recoverable handling for "git worktree add" failures that indicate a branch is already checked out: detect those errors, parse git worktree list --porcelain, validate discovered paths (local or remote), and return { success: true, created: false, alreadyExisted, existingPath } for reuse; renderer hooks reuse matching sessions and emit contextual toasts.

Changes

Cohort / File(s) Summary
IPC Handler & Remote Utils
src/main/ipc/handlers/git.ts, src/main/utils/remote-git.ts
On "already used/checked out" failures, run git worktree list --porcelain, parse candidate worktree path, validate existence (local fs.access or remote test -d), and return recoverable result with alreadyExisted/existingPath instead of propagating the error.
Shared Git Utilities
src/shared/gitUtils.ts
Add isWorktreeAlreadyUsedError(stderr) (case-insensitive detection) and parseWorktreePathForBranch(stdout, branchName) to parse porcelain output and extract worktree paths.
Preload & Public Types
src/main/preload/git.ts, src/renderer/global.d.ts
Introduce/export GitWorktreeSetupResult and GitWorktreeCheckoutResult types; update worktreeSetup/worktreeCheckout Promise return types to include optional alreadyExisted/existingPath.
Renderer Hooks (logic changes)
src/renderer/hooks/batch/useAutoRunHandlers.ts, src/renderer/hooks/worktree/useWorktreeHandlers.ts
When worktreeSetup returns alreadyExisted, prefer normalized existingPath, clear recent-create marks, detect/reuse existing sessions by normalized projectRoot/cwd, skip creating new sessions if matched (unless busy/connecting), and show contextual info/warning toasts.
Tests — IPC / Remote
src/__tests__/main/ipc/handlers/git.test.ts, src/__tests__/main/utils/remote-git.test.ts
Expand tests for recovery flows: porcelain parsing, staleness handling (fs.access/test -d failures), branch-not-found, and ensuring unrelated git errors are propagated unchanged.
Tests — Renderer Hooks
src/__tests__/renderer/hooks/batch/useAutoRunHandlers.worktree.test.ts, src/__tests__/renderer/hooks/useWorktreeHandlers.test.ts
Add integration/unit tests for create-new flows when alreadyExisted is returned: session reuse, normalization (trailing slashes/subdir drift), dispatch behavior, immediate start dispatch semantics, and expected info/warning toasts.
Tests — Shared Utils
src/__tests__/shared/gitUtils.test.ts
Add tests for isWorktreeAlreadyUsedError (modern/legacy strings, case-insensitive) and parseWorktreePathForBranch (porcelain parsing, CRLF, detached/no-match cases).

Sequence Diagram

sequenceDiagram
    participant Renderer as Renderer (Hook)
    participant IPC as IPC Handler (Main)
    participant Git as Git CLI
    participant Store as Session Store

    Renderer->>IPC: worktreeSetup(requestedBranch, requestedPath)
    IPC->>Git: git worktree add requestedPath requestedBranch
    Git-->>IPC: ❌ stderr ("already checked out..." / "already used by worktree...")
    IPC->>IPC: isWorktreeAlreadyUsedError(stderr)?
    alt recoverable pattern matches
        IPC->>Git: git worktree list --porcelain
        Git-->>IPC: porcelain stdout
        IPC->>IPC: parseWorktreePathForBranch(stdout, requestedBranch)
        IPC->>IPC: fs.access(existingPath) / remote test -d
        alt existingPath valid
            IPC-->>Renderer: {success: true, created: false, alreadyExisted: true, existingPath}
            Renderer->>Store: normalize(existingPath) -> find session by cwd/projectRoot
            alt session found and not busy
                Store-->>Renderer: existingSessionId
                Renderer->>Store: setActiveSession(existingSessionId)
                Renderer->>Renderer: show toast "Worktree Already Open"
            else no session found or not eligible
                Renderer->>Store: create session with existingPath
                Renderer->>Renderer: show toast "Worktree Already Existed"
            end
        else stale/missing -> fallback
            IPC-->>Renderer: original git error (failure)
        end
    else not a matched error
        IPC-->>Renderer: original git error (failure)
    end
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly Related PRs

Suggested labels

ready to merge

Poem

🐰
Porcelain maps a path I've known,
A rabbit finds an open home.
No duplicate burrows, one session to keep,
Hop in quick — the code reuses what we reap.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately and concisely describes the primary change: enabling Maestro to open an existing worktree when a branch is already attached elsewhere, rather than surfacing an error. It reflects the main improvement across the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 26, 2026

Greptile Summary

This PR adds recovery logic for the git worktree add "already checked out / already used" failure: the main process now runs git worktree list --porcelain to resolve the existing path and returns it to the renderer, which opens a session against it instead of surfacing an opaque error. Both local and SSH/remote paths are covered, with a clean fallback to the original error when the porcelain lookup finds no match.

Confidence Score: 4/5

Safe to merge — all findings are P2 quality/consistency issues with no correctness impact on the happy path.

No P0 or P1 issues found. Two P2 logic-consistency findings: (1) handleCreateWorktreeFromConfig calls fetchGitInfo before the early-return existing-session check, wasting an async git round-trip; (2) spawnWorktreeAgentAndDispatch compares paths without normalizePath, inconsistent with useWorktreeHandlers. One trivial P2 style note on the unnecessary alias. The core recovery logic (porcelain parsing, IPC threading, renderer branching) is correct and well-tested.

src/renderer/hooks/worktree/useWorktreeHandlers.ts (fetchGitInfo ordering), src/renderer/hooks/batch/useAutoRunHandlers.ts (path normalization)

Important Files Changed

Filename Overview
src/shared/gitUtils.ts Adds isWorktreeAlreadyUsedError (regex detector) and parseWorktreePathForBranch (porcelain parser with CRLF support); both are well-tested and correctly handle edge cases.
src/main/ipc/handlers/git.ts Adds findLocalWorktreeForBranch helper and 'already used' recovery in the worktree setup IPC handler; recovery logic is correct but contains an unnecessary isAlreadyUsedError alias.
src/main/utils/remote-git.ts Mirrors the local recovery logic for SSH/remote worktrees; findRemoteWorktreeForBranch follows the same pattern and falls through cleanly when porcelain finds no match.
src/renderer/hooks/worktree/useWorktreeHandlers.ts Handles alreadyExisted in both handleCreateWorktreeFromConfig and handleCreateWorktree, but the former calls fetchGitInfo before the existing-session check, wasting an async round-trip on the early-return path.
src/renderer/hooks/batch/useAutoRunHandlers.ts Adds alreadyExisted recovery and session dedup to spawnWorktreeAgentAndDispatch; the existing-session lookup uses raw string equality instead of normalizePath, inconsistent with useWorktreeHandlers.
src/main/preload/git.ts Adds alreadyExisted and existingPath optional fields to the IPC result type; straightforward and correctly marked optional.
src/renderer/global.d.ts Mirrors the preload type update in the renderer global declaration; in sync with preload/git.ts.

Sequence Diagram

sequenceDiagram
    participant UI as Renderer (UI)
    participant IPC as IPC Handler (git.ts)
    participant Git as git CLI

    UI->>IPC: worktreeSetup(mainRepoCwd, worktreePath, branchName)
    IPC->>Git: git worktree add [worktreePath] [branchName]
    Git-->>IPC: exitCode=128, stderr="already checked out at '/existing/path'"

    alt isWorktreeAlreadyUsedError(stderr)
        IPC->>Git: git worktree list --porcelain
        Git-->>IPC: porcelain output
        IPC->>IPC: parseWorktreePathForBranch(stdout, branchName)
        alt existingPath found
            IPC-->>UI: { success:true, alreadyExisted:true, existingPath }
            UI->>UI: Check sessions for existingPath (normalizePath match)
            alt session already open
                UI->>UI: setActiveSessionId(existingSession.id) + Worktree Already Open toast
            else no open session
                UI->>UI: buildWorktreeSession(existingPath) + Worktree Already Existed toast
            end
        else no match in porcelain
            IPC-->>UI: { success:false, error: stderr }
            UI->>UI: Failed to Create Worktree error toast
        end
    else unrelated git error
        IPC-->>UI: { success:false, error: stderr }
        UI->>UI: Failed to Create Worktree error toast
    end
Loading

Comments Outside Diff (1)

  1. src/main/ipc/handlers/git.ts, line 532 (link)

    P2 Unnecessary function alias

    isAlreadyUsedError is defined solely as const isAlreadyUsedError = isWorktreeAlreadyUsedError. There is no need for the alias — isWorktreeAlreadyUsedError can be called directly at the single call site on line 545.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "fix(worktrees): recover from "already us..." | Re-trigger Greptile

Comment thread src/renderer/hooks/worktree/useWorktreeHandlers.ts Outdated
Comment thread src/renderer/hooks/batch/useAutoRunHandlers.ts Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/main/preload/git.ts (1)

229-246: Extract this worktreeSetup response shape into a shared type.

This contract is now duplicated in preload and renderer global declarations; centralizing it will reduce type drift on future IPC field changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/preload/git.ts` around lines 229 - 246, Extract the inline response
shape used in worktreeSetup into a shared exported type (e.g.,
WorktreeSetupResult) and replace the inline Promise<{...}> return type in the
worktreeSetup declaration with Promise<WorktreeSetupResult>; then import that
shared type into the preload (where worktreeSetup calls ipcRenderer.invoke) and
the renderer global declaration that currently duplicates the shape so both
reference the same exported type and avoid drift, preserving all existing
property names (success, created, currentBranch, requestedBranch,
branchMismatch, alreadyExisted, existingPath, error).
src/main/utils/remote-git.ts (1)

454-475: Recovery path is well-scoped.

Limiting recovery to isWorktreeAlreadyUsedError and falling through to the original error when porcelain lookup returns no match keeps unrelated git worktree add failures unchanged. Setting currentBranch: branchName is safe here because parseWorktreePathForBranch only returns a path whose branch line matches branchName.

One optional consideration: a debug log when the recovery branch fires would help diagnose future "already used" reports without changing behavior — e.g.

📝 Optional: log recovery
 		if (isWorktreeAlreadyUsedError(errMsg)) {
 			const existingPath = await findRemoteWorktreeForBranch(mainRepoCwd, branchName, sshRemote);
 			if (existingPath) {
+				logger.debug(
+					`Recovered from worktree-already-used: branch=${branchName} existingPath=${existingPath}`,
+					LOG_CONTEXT
+				);
 				return {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/utils/remote-git.ts` around lines 454 - 475, Add a debug log when
the "already used" recovery path runs: inside the error handling that checks
isWorktreeAlreadyUsedError(errMsg) and before returning the
success-with-existingPath result (where you call
findRemoteWorktreeForBranch(mainRepoCwd, branchName, sshRemote) and build the
returned object), emit a debug/processLogger.trace or similar message that
includes the branchName, sshRemote (or remote id), and the found existingPath
(or that none was found) along with createResult.stderr so the recovery event is
visible in logs for diagnosis without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/ipc/handlers/git.ts`:
- Around line 74-80: The helper findLocalWorktreeForBranch must validate that
the parsed worktree path is actually usable before returning success: after
calling parseWorktreePathForBranch (and before returning), check that the
returned path exists and is a valid git worktree (e.g., directory exists and
contains a .git directory or gitdir file, or run a quick git -C <path> rev-parse
--git-dir to confirm); if the check fails, return null so callers (lines
~705-723) do not treat stale registrations as success. Use the existing helpers
execFileNoThrow or fs/stat to perform the validation and keep the same return
type Promise<string | null>.

In `@src/renderer/hooks/batch/useAutoRunHandlers.ts`:
- Around line 160-164: The session lookup uses a raw equality check which can
miss matches due to differing slashes/trailing separators; update the
existingSession lookup in useAutoRunHandlers.ts to compare normalized paths
(e.g., use path.normalize or the app's normalizePath utility) by normalizing
both s.cwd and worktreePath before the find, so reuse of an open session works
the same way as useWorktreeHandlers.

---

Nitpick comments:
In `@src/main/preload/git.ts`:
- Around line 229-246: Extract the inline response shape used in worktreeSetup
into a shared exported type (e.g., WorktreeSetupResult) and replace the inline
Promise<{...}> return type in the worktreeSetup declaration with
Promise<WorktreeSetupResult>; then import that shared type into the preload
(where worktreeSetup calls ipcRenderer.invoke) and the renderer global
declaration that currently duplicates the shape so both reference the same
exported type and avoid drift, preserving all existing property names (success,
created, currentBranch, requestedBranch, branchMismatch, alreadyExisted,
existingPath, error).

In `@src/main/utils/remote-git.ts`:
- Around line 454-475: Add a debug log when the "already used" recovery path
runs: inside the error handling that checks isWorktreeAlreadyUsedError(errMsg)
and before returning the success-with-existingPath result (where you call
findRemoteWorktreeForBranch(mainRepoCwd, branchName, sshRemote) and build the
returned object), emit a debug/processLogger.trace or similar message that
includes the branchName, sshRemote (or remote id), and the found existingPath
(or that none was found) along with createResult.stderr so the recovery event is
visible in logs for diagnosis without changing behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4f934630-8c2b-4c03-8e6b-0ded62603d78

📥 Commits

Reviewing files that changed from the base of the PR and between 5fc4cf7 and 16741bd.

📒 Files selected for processing (12)
  • src/__tests__/main/ipc/handlers/git.test.ts
  • src/__tests__/main/utils/remote-git.test.ts
  • src/__tests__/renderer/hooks/batch/useAutoRunHandlers.worktree.test.ts
  • src/__tests__/renderer/hooks/useWorktreeHandlers.test.ts
  • src/__tests__/shared/gitUtils.test.ts
  • src/main/ipc/handlers/git.ts
  • src/main/preload/git.ts
  • src/main/utils/remote-git.ts
  • src/renderer/global.d.ts
  • src/renderer/hooks/batch/useAutoRunHandlers.ts
  • src/renderer/hooks/worktree/useWorktreeHandlers.ts
  • src/shared/gitUtils.ts

Comment thread src/main/ipc/handlers/git.ts Outdated
Comment thread src/renderer/hooks/batch/useAutoRunHandlers.ts Outdated
- Validate recovered worktree path with fs.access (local) and remote
  test -d (SSH) so stale `git worktree list` registrations no longer
  surface as success.
- Reorder handleCreateWorktreeFromConfig to run the existing-session
  check before fetchGitInfo, matching handleCreateWorktree and
  avoiding a wasted async round-trip when the session is already open.
- Normalize path comparison in useAutoRunHandlers so trailing slashes /
  duplicate separators don't cause duplicate sessions.
- Extract GitWorktreeSetupResult / GitWorktreeCheckoutResult shared
  types so the preload bridge and renderer global stay in sync.
- Log a debug entry when the SSH "already used" recovery path fires.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/main/ipc/handlers/git.ts (1)

94-94: Nit: redundant local alias.

const isAlreadyUsedError = isWorktreeAlreadyUsedError; is a one-liner rename that doesn’t add information at the call site (line 721). You can drop it and call isWorktreeAlreadyUsedError(errMsg) directly to avoid two names for the same predicate.

♻️ Suggested cleanup
-const isAlreadyUsedError = isWorktreeAlreadyUsedError;
-
@@
-				if (isAlreadyUsedError(errMsg)) {
+				if (isWorktreeAlreadyUsedError(errMsg)) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/ipc/handlers/git.ts` at line 94, Remove the redundant local alias by
deleting the line that defines const isAlreadyUsedError =
isWorktreeAlreadyUsedError; and update any call sites that use
isAlreadyUsedError(errMsg) to call isWorktreeAlreadyUsedError(errMsg) directly
(keep the original predicate name everywhere to avoid duplicate identifiers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/renderer/hooks/batch/useAutoRunHandlers.ts`:
- Around line 165-172: The session lookup only compares normalizePath(s.cwd) to
normalizedWorktreePath and misses sessions that have cd'd into subdirs; update
the find predicate used in the useSessionStore.getState().sessions.find call
(and the similar lookup around lines 188-212) to also compare a stable project
root field (e.g., s.projectRoot or s.fullPath) after normalizing — return the
session if normalizePath(s.projectRoot) === normalizedWorktreePath ||
normalizePath(s.fullPath) === normalizedWorktreePath || normalizePath(s.cwd) ===
normalizedWorktreePath so existing worktree-root sessions are matched before
falling back to cwd.

In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts`:
- Around line 338-355: The fast-path duplicate-session check using
reusedExisting only normalizes s.cwd and compares it to actualPath, which breaks
if an existing session's cwd has drifted (e.g., from project root to a child
folder); update the match to compare a stable root path instead: derive the
stable identifier (the worktree root, e.g., projectRoot or fullPath used when
creating sessions) and compare normalizePath(s.<stableRootProperty> or
storedRoot) against normalizePath(projectRoot || fullPath) rather than
s.cwd/actualPath; update both occurrences (the block around reusedExisting and
the similar block at lines 443-468) to use
useSessionStore.getState().sessions.find(...) matching the stable root property
so the check reliably detects and focuses the already-open worktree session.

---

Nitpick comments:
In `@src/main/ipc/handlers/git.ts`:
- Line 94: Remove the redundant local alias by deleting the line that defines
const isAlreadyUsedError = isWorktreeAlreadyUsedError; and update any call sites
that use isAlreadyUsedError(errMsg) to call isWorktreeAlreadyUsedError(errMsg)
directly (keep the original predicate name everywhere to avoid duplicate
identifiers).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b5899aee-74dc-457e-8a3a-1bac2dbbe145

📥 Commits

Reviewing files that changed from the base of the PR and between 16741bd and 85ba142.

📒 Files selected for processing (9)
  • src/__tests__/main/ipc/handlers/git.test.ts
  • src/__tests__/main/utils/remote-git.test.ts
  • src/__tests__/renderer/hooks/batch/useAutoRunHandlers.worktree.test.ts
  • src/main/ipc/handlers/git.ts
  • src/main/preload/git.ts
  • src/main/utils/remote-git.ts
  • src/renderer/global.d.ts
  • src/renderer/hooks/batch/useAutoRunHandlers.ts
  • src/renderer/hooks/worktree/useWorktreeHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/utils/remote-git.ts
  • src/main/preload/git.ts

Comment thread src/renderer/hooks/batch/useAutoRunHandlers.ts Outdated
Comment thread src/renderer/hooks/worktree/useWorktreeHandlers.ts
…ust cwd

A child session whose `cwd` has drifted into a subdirectory of the
worktree (e.g. `cd src/components`) was missed by the recovery dedup
check, causing a duplicate session to be built for the same worktree.
Match against the stable `projectRoot` set at session creation, falling
back to `cwd`. Applied to both reuse sites in useWorktreeHandlers and
the spawn flow in useAutoRunHandlers.

Also drops the redundant `isAlreadyUsedError` alias in git.ts in favor
of calling `isWorktreeAlreadyUsedError` directly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/renderer/hooks/batch/useAutoRunHandlers.ts (1)

184-226: ⚠️ Potential issue | 🟡 Minor

Reused session may be busy/connecting — batch dispatch will land on a busy agent.

When spawnWorktreeAgentAndDispatch finds an existingSession (either via the alreadyExisted recovery path or because a session already matched the requested worktree root), it returns existingSession.id directly to handleStartBatchRun, which immediately calls startBatchRun(targetSessionId, ...). Unlike the explicit existing-open branch above (lines 392–405), there is no guard for state === 'busy' | 'connecting' here, so a recovery into a busy worktree session will silently queue the batch onto an in-flight agent.

Consider mirroring the existing-open guard before returning the reused session id:

♻️ Suggested guard
 	let dispatchSessionId: string;
 	if (existingSession) {
+		if (existingSession.state === 'busy' || existingSession.state === 'connecting') {
+			notifyToast({
+				type: 'warning',
+				title: 'Target Agent Busy',
+				message: 'Existing worktree agent is busy. Please try again.',
+			});
+			return null;
+		}
 		dispatchSessionId = existingSession.id;
 	} else {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/hooks/batch/useAutoRunHandlers.ts` around lines 184 - 226, The
code returns an existingSession.id without guarding for existingSession.state,
which can be 'busy' or 'connecting' and cause the batch to be queued onto an
in-flight agent; update spawnWorktreeAgentAndDispatch (or the block that returns
existingSession.id) to mirror the existing-open guard logic by checking
existingSession.state and if it is 'busy' or 'connecting' either wait for or
re-spawn an open agent (e.g., call the same helper used in the existing-open
branch or invoke spawnWorktreeAgentAndDispatch to ensure/open a session) before
returning a session id so handleStartBatchRun/startBatchRun always receives an
'open' session id rather than one in flight.
🧹 Nitpick comments (1)
src/renderer/hooks/batch/useAutoRunHandlers.ts (1)

16-33: Extract the duplicated normalizePath / sessionMatchesWorktreeRoot helpers.

These two helpers are byte-identical to the ones added in src/renderer/hooks/worktree/useWorktreeHandlers.ts (lines 86–103). Keeping two copies means future tweaks (e.g., Windows drive-letter normalization, additional fallback fields like fullPath) have to be made in lockstep. Move them to a shared module (e.g., src/renderer/utils/worktreePath.ts) and import from both hooks.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/hooks/batch/useAutoRunHandlers.ts` around lines 16 - 33, The two
byte-identical helpers normalizePath and sessionMatchesWorktreeRoot are
duplicated; extract them into a single shared module that exports both functions
(and imports/exports the Session type if needed), replace the local
implementations in useAutoRunHandlers and the other hook with imports from that
shared module, and update both hooks to import Session/type dependencies from
the same place so future changes (e.g., drive-letter normalization or extra
fallback fields) are made in one location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/renderer/hooks/batch/useAutoRunHandlers.ts`:
- Around line 184-226: The code returns an existingSession.id without guarding
for existingSession.state, which can be 'busy' or 'connecting' and cause the
batch to be queued onto an in-flight agent; update spawnWorktreeAgentAndDispatch
(or the block that returns existingSession.id) to mirror the existing-open guard
logic by checking existingSession.state and if it is 'busy' or 'connecting'
either wait for or re-spawn an open agent (e.g., call the same helper used in
the existing-open branch or invoke spawnWorktreeAgentAndDispatch to ensure/open
a session) before returning a session id so handleStartBatchRun/startBatchRun
always receives an 'open' session id rather than one in flight.

---

Nitpick comments:
In `@src/renderer/hooks/batch/useAutoRunHandlers.ts`:
- Around line 16-33: The two byte-identical helpers normalizePath and
sessionMatchesWorktreeRoot are duplicated; extract them into a single shared
module that exports both functions (and imports/exports the Session type if
needed), replace the local implementations in useAutoRunHandlers and the other
hook with imports from that shared module, and update both hooks to import
Session/type dependencies from the same place so future changes (e.g.,
drive-letter normalization or extra fallback fields) are made in one location.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cb27e626-a96c-4055-971e-50be69f6b7b2

📥 Commits

Reviewing files that changed from the base of the PR and between 85ba142 and 69fab4f.

📒 Files selected for processing (5)
  • src/__tests__/renderer/hooks/batch/useAutoRunHandlers.worktree.test.ts
  • src/__tests__/renderer/hooks/useWorktreeHandlers.test.ts
  • src/main/ipc/handlers/git.ts
  • src/renderer/hooks/batch/useAutoRunHandlers.ts
  • src/renderer/hooks/worktree/useWorktreeHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/ipc/handlers/git.ts
  • src/tests/renderer/hooks/batch/useAutoRunHandlers.worktree.test.ts

… busy

The spawnWorktreeAgentAndDispatch reuse path returned existingSession.id
straight to handleStartBatchRun without checking state, so a recovery into
a worktree session whose agent was already mid-run would silently queue
the batch on top of the in-flight agent. Mirrors the existing-open guard
in handleStartBatchRun: when the matched session is 'busy' or
'connecting', surface the "Target Agent Busy" toast and abort.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/renderer/hooks/batch/useAutoRunHandlers.ts (1)

16-33: Extract worktree path-matching helpers into a shared renderer utility.

normalizePath and sessionMatchesWorktreeRoot now mirror logic in src/renderer/hooks/worktree/useWorktreeHandlers.ts (Line 87-102). Keeping these duplicated in two hooks increases drift risk and can reintroduce dedup mismatches later.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/hooks/batch/useAutoRunHandlers.ts` around lines 16 - 33, The
normalizePath and sessionMatchesWorktreeRoot functions are duplicated from
useWorktreeHandlers.ts; extract them into a shared renderer utility and import
that utility in both hooks to avoid drift. Create a new utility (e.g., export
normalizePath and sessionMatchesWorktreeRoot from a shared module), replace the
local implementations in useAutoRunHandlers.ts and useWorktreeHandlers.ts with
imports of those functions, and update any references to call the imported
helpers to ensure a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/renderer/hooks/batch/useAutoRunHandlers.ts`:
- Around line 16-33: The normalizePath and sessionMatchesWorktreeRoot functions
are duplicated from useWorktreeHandlers.ts; extract them into a shared renderer
utility and import that utility in both hooks to avoid drift. Create a new
utility (e.g., export normalizePath and sessionMatchesWorktreeRoot from a shared
module), replace the local implementations in useAutoRunHandlers.ts and
useWorktreeHandlers.ts with imports of those functions, and update any
references to call the imported helpers to ensure a single source of truth.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0c6b62e9-72f8-40d7-bffd-d16903496b21

📥 Commits

Reviewing files that changed from the base of the PR and between 69fab4f and f4a6a1f.

📒 Files selected for processing (2)
  • src/__tests__/renderer/hooks/batch/useAutoRunHandlers.worktree.test.ts
  • src/renderer/hooks/batch/useAutoRunHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/tests/renderer/hooks/batch/useAutoRunHandlers.worktree.test.ts

@chr1syy chr1syy added ready to merge This PR is ready to merge labels Apr 27, 2026
@pedramamini pedramamini merged commit fd5db57 into RunMaestro:rc Apr 27, 2026
3 checks passed
pedramamini added a commit that referenced this pull request Apr 27, 2026
…eRoot

Both useAutoRunHandlers and useWorktreeHandlers (and worktreeDedup itself)
defined identical copies of normalizePath; the path-matching helper was
duplicated across the two hooks. CodeRabbit flagged this on #906 and it
matches the canonical "no duplicated helpers" rule in CLAUDE.md.

Move both helpers into worktreeDedup.ts (already the canonical home for
worktree path comparison) and import from a single source. Add unit tests
covering both helpers, including the projectRoot vs. cwd-drift case that
motivated sessionMatchesWorktreeRoot in the first place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge This PR is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants