fix(discovery): list sessions from git worktrees#191
Merged
matt1398 merged 6 commits intoMay 6, 2026
Conversation
GitIdentityResolver previously only looked for a `.git` file/directory exactly at `projectPath` instead of searching upwards to the project root. This caused discovery to fail when Claude was run inside a subdirectory of a git worktree or normal repository. Without a resolved identity, `WorktreeGrouper` fell back to heuristic path parsing and incorrectly split the session into a phantom repository group named after the subdirectory, preventing it from appearing in the worktree/session dropdown under the main repository. Fix: - Modify `resolveIdentity`, `isWorktree`, `getBranch`, `detectWorktreeSource`, and `getGitWorktreeName` to search upwards for the `.git` file/directory. - Add test coverage for `GitIdentityResolver` and `WorktreeGrouper` to ensure worktrees and subdirectories group correctly. Fixes matt1398#188.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements upward searching for .git directories in GitIdentityResolver to better handle git worktrees and subdirectories. It introduces a findGitPath helper and updates several resolution methods, supported by new test cases. The feedback suggests consolidating the logic in getBranch and getRepoType by utilizing the new helper method to improve maintainability and ensure consistent path resolution.
Comment on lines
465
to
486
| let currentPath = projectPath; | ||
| let gitPathExists = false; | ||
| let gitPath = ''; | ||
|
|
||
| try { | ||
| await fs.promises.access(gitPath); | ||
| } catch { | ||
| return null; | ||
| while (currentPath) { | ||
| gitPath = path.join(currentPath, '.git'); | ||
| try { | ||
| await fs.promises.access(gitPath); | ||
| gitPathExists = true; | ||
| break; // Found it | ||
| } catch { | ||
| const parentDir = path.dirname(currentPath); | ||
| if (parentDir === currentPath) { | ||
| break; | ||
| } | ||
| currentPath = parentDir; | ||
| } | ||
| } | ||
|
|
||
| if (!gitPathExists) return null; | ||
|
|
||
| const stats = await fs.promises.stat(gitPath); |
Contributor
There was a problem hiding this comment.
Consolidate the upward search logic by using the findGitPath helper method to improve maintainability and ensure consistent path resolution behavior.
Suggested change
| let currentPath = projectPath; | |
| let gitPathExists = false; | |
| let gitPath = ''; | |
| try { | |
| await fs.promises.access(gitPath); | |
| } catch { | |
| return null; | |
| while (currentPath) { | |
| gitPath = path.join(currentPath, '.git'); | |
| try { | |
| await fs.promises.access(gitPath); | |
| gitPathExists = true; | |
| break; // Found it | |
| } catch { | |
| const parentDir = path.dirname(currentPath); | |
| if (parentDir === currentPath) { | |
| break; | |
| } | |
| currentPath = parentDir; | |
| } | |
| } | |
| if (!gitPathExists) return null; | |
| const stats = await fs.promises.stat(gitPath); | |
| const gitInfo = await this.findGitPath(projectPath); | |
| if (!gitInfo) return null; | |
| const { gitPath, stats } = gitInfo; |
Comment on lines
587
to
600
| let currentPath = projectPath; | ||
| while (currentPath) { | ||
| const gitPath = path.join(currentPath, '.git'); | ||
| try { | ||
| await fs.promises.access(gitPath); | ||
| return 'git'; | ||
| } catch { | ||
| const parentDir = path.dirname(currentPath); | ||
| if (parentDir === currentPath) { | ||
| break; | ||
| } | ||
| currentPath = parentDir; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Consolidate the upward search logic by using the findGitPath helper method.
if (await this.findGitPath(projectPath)) {
return 'git';
}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an upward-search helper to find the nearest ChangesWorktree Detection & Identity Resolution
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/services/parsing/GitIdentityResolver.ts (1)
82-100: ⚡ Quick winRefactor to use
findGitPath()helper to avoid code duplication.
resolveIdentity()has an inline upward-search loop (lines 87-100) that duplicates the logic infindGitPath()(lines 39-64). The same duplication exists ingetBranch()(lines 469-482) anddetectWorktreeSource()(lines 588-600). Consider refactoring these methods to use the helper for consistency and maintainability.♻️ Proposed refactor for resolveIdentity()
async resolveIdentity(projectPath: string): Promise<RepositoryIdentity | null> { try { - // Search upwards for .git - let currentPath = projectPath; - let gitPathExists = false; - let gitPath = ''; - - while (currentPath) { - gitPath = path.join(currentPath, '.git'); - try { - await fs.promises.access(gitPath); - gitPathExists = true; - break; // Found it - } catch { - const parentDir = path.dirname(currentPath); - if (parentDir === currentPath) { - break; - } - currentPath = parentDir; - } - } + const gitInfo = await this.findGitPath(projectPath); - if (gitPathExists) { - const stats = await fs.promises.stat(gitPath); + if (gitInfo) { + const { repoRoot: currentPath, gitPath, stats } = gitInfo; let mainGitDir: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/parsing/GitIdentityResolver.ts` around lines 82 - 100, The upward-search loop in resolveIdentity() duplicates logic already implemented in findGitPath(); replace the inline loop with a call to findGitPath(projectPath) and use the returned git path (or handle not-found) so resolveIdentity() reuses the helper; apply the same refactor pattern to getBranch() and detectWorktreeSource() by removing their inline directory-walk loops and invoking findGitPath() (or its exported equivalent) to obtain the .git location, then adapt any downstream variable names and error/exists handling to match findGitPath()'s return contract.test/main/services/parsing/GitIdentityResolver.test.ts (1)
1-1: 💤 Low valueRemove unused
viimport.The
viimport is not used in this test file.♻️ Fix
-import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { describe, expect, it, beforeEach, afterEach } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/services/parsing/GitIdentityResolver.test.ts` at line 1, The import list at the top of GitIdentityResolver.test.ts includes an unused symbol `vi`; remove `vi` from the named imports in the line importing from 'vitest' (so leave only the used symbols `describe`, `expect`, `it`, `beforeEach`, `afterEach`) to eliminate the unused import warning.test/main/services/discovery/WorktreeGrouper.test.ts (1)
50-61: 💤 Low valueMove
vi.mock()calls to module level.
vi.mock()is hoisted by Vitest at compile time, so placing it insideit()doesn't scope it to that test—it applies to the entire file. While this works with a single test, it's misleading and may cause issues if more tests are added. Move mocks to the top level (after imports) or usevi.doMock()if per-test mocking is intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/services/discovery/WorktreeGrouper.test.ts` around lines 50 - 61, The vi.mock calls for SubprojectRegistry and SessionContentFilter are inside a test scope and must be moved to module-level so Vitest's hoisting behavior doesn't make them global/misleading; relocate the two vi.mock(...) blocks (mocking subprojectRegistry.getSessionFilter and SessionContentFilter.hasNonNoiseMessages) to the top of the test file immediately after imports, or if you need per-test behavior change them to vi.doMock(...) within the specific it()/describe() block and use vi.importMock/vi.clearAllMocks as appropriate.
🤖 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/main/services/parsing/GitIdentityResolver.ts`:
- Around line 82-100: The upward-search loop in resolveIdentity() duplicates
logic already implemented in findGitPath(); replace the inline loop with a call
to findGitPath(projectPath) and use the returned git path (or handle not-found)
so resolveIdentity() reuses the helper; apply the same refactor pattern to
getBranch() and detectWorktreeSource() by removing their inline directory-walk
loops and invoking findGitPath() (or its exported equivalent) to obtain the .git
location, then adapt any downstream variable names and error/exists handling to
match findGitPath()'s return contract.
In `@test/main/services/discovery/WorktreeGrouper.test.ts`:
- Around line 50-61: The vi.mock calls for SubprojectRegistry and
SessionContentFilter are inside a test scope and must be moved to module-level
so Vitest's hoisting behavior doesn't make them global/misleading; relocate the
two vi.mock(...) blocks (mocking subprojectRegistry.getSessionFilter and
SessionContentFilter.hasNonNoiseMessages) to the top of the test file
immediately after imports, or if you need per-test behavior change them to
vi.doMock(...) within the specific it()/describe() block and use
vi.importMock/vi.clearAllMocks as appropriate.
In `@test/main/services/parsing/GitIdentityResolver.test.ts`:
- Line 1: The import list at the top of GitIdentityResolver.test.ts includes an
unused symbol `vi`; remove `vi` from the named imports in the line importing
from 'vitest' (so leave only the used symbols `describe`, `expect`, `it`,
`beforeEach`, `afterEach`) to eliminate the unused import warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7ff0f77-300f-4875-837a-bc26c14d519c
📒 Files selected for processing (4)
src/main/services/parsing/GitIdentityResolver.tstest/main/services/discovery/WorktreeGrouper.test.tstest/main/services/parsing/GitIdentityResolver.test.tstest/main/services/parsing/GitIdentityResolver.upwards.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/services/parsing/GitIdentityResolver.ts (1)
453-466:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing relative path resolution for
gitdir:ingetBranch.When the
.gitfile contains a relativegitdir:path (e.g.,gitdir: ../../.git/worktrees/foo), this code uses it directly without resolving it against the repo root. This is inconsistent withresolveIdentity(line 103) andgetGitWorktreeName(lines 691-693), which both resolve relative paths.🐛 Proposed fix to handle relative gitdir paths
if (stats.isFile()) { // Worktree - read .git file to find the HEAD location const gitFileContent = (await fs.promises.readFile(gitPath, 'utf-8')).trim(); const gitDirMatch = /^gitdir:\s*(\S[^\r\n]*)$/.exec(gitFileContent); if (!gitDirMatch) { return null; } - headPath = path.join(gitDirMatch[1], 'HEAD'); + let worktreeGitDir = gitDirMatch[1].trim(); + if (!path.isAbsolute(worktreeGitDir)) { + worktreeGitDir = path.resolve(gitInfo.repoRoot, worktreeGitDir); + } + headPath = path.join(worktreeGitDir, 'HEAD'); } else {Note: You'll need to access
repoRootfromgitInfoby adjusting the destructuring at line 450:- const { gitPath, stats } = gitInfo; + const { repoRoot, gitPath, stats } = gitInfo;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/parsing/GitIdentityResolver.ts` around lines 453 - 466, getBranch currently uses the raw gitdir value from gitFileContent without resolving relative paths; update getBranch to destructure repoRoot from gitInfo (add repoRoot to the existing destructuring near where gitPath is obtained) and, when gitDirMatch is present, compute the resolvedDir as path.isAbsolute(gitDirMatch[1]) ? gitDirMatch[1] : path.resolve(repoRoot, gitDirMatch[1]) and then set headPath = path.join(resolvedDir, 'HEAD') (replacing the direct gitDirMatch[1] join) so relative gitdir entries (e.g., ../../.git/...) resolve the same way as resolveIdentity and getGitWorktreeName.
🤖 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/main/services/parsing/GitIdentityResolver.ts`:
- Around line 453-466: getBranch currently uses the raw gitdir value from
gitFileContent without resolving relative paths; update getBranch to destructure
repoRoot from gitInfo (add repoRoot to the existing destructuring near where
gitPath is obtained) and, when gitDirMatch is present, compute the resolvedDir
as path.isAbsolute(gitDirMatch[1]) ? gitDirMatch[1] : path.resolve(repoRoot,
gitDirMatch[1]) and then set headPath = path.join(resolvedDir, 'HEAD')
(replacing the direct gitDirMatch[1] join) so relative gitdir entries (e.g.,
../../.git/...) resolve the same way as resolveIdentity and getGitWorktreeName.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23afd5a6-8348-48a0-9d0c-fd3e527533f1
📒 Files selected for processing (1)
src/main/services/parsing/GitIdentityResolver.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/main/services/parsing/GitIdentityResolver.upwards.test.ts (1)
42-54: 💤 Low valueConsider asserting
identity.remoteUrlto cover the config-parsing path.Both tests already have a working
.git/configwith a[remote "origin"]URL in the fixture. Adding an assertion onidentity.remoteUrlwould exercise thegetRemoteUrl → parseConfigpath end-to-end at no extra setup cost, and would catch regressions in that path independently of the upward-search logic.💡 Suggested addition to each `it` block
it('resolves identity when path is a subdirectory of main repo', async () => { const identity = await gitIdentityResolver.resolveIdentity(path.join(mainRepoDir, 'src')); expect(identity).toBeDefined(); expect(identity?.mainGitDir).toBe(fs.realpathSync(path.join(mainRepoDir, '.git'))); + expect(identity?.remoteUrl).toBe('git@github.com:matt1398/claude-devtools.git'); expect(await gitIdentityResolver.isWorktree(path.join(mainRepoDir, 'src'))).toBe(false); }); it('resolves identity when path is a subdirectory of a worktree (with relative gitdir)', async () => { const identity = await gitIdentityResolver.resolveIdentity(path.join(worktreeDir, 'src')); expect(identity).toBeDefined(); expect(identity?.mainGitDir).toBe(fs.realpathSync(path.join(mainRepoDir, '.git'))); + expect(identity?.remoteUrl).toBe('git@github.com:matt1398/claude-devtools.git'); expect(await gitIdentityResolver.isWorktree(path.join(worktreeDir, 'src'))).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/services/parsing/GitIdentityResolver.upwards.test.ts` around lines 42 - 54, Add assertions to exercise the config-parsing path by checking the resolved identity's remote URL: after calling gitIdentityResolver.resolveIdentity(...) in each test, assert that identity.remoteUrl equals the expected remote (the `[remote "origin"]` URL present in the fixture). This will exercise getRemoteUrl → parseConfig end-to-end; update the two it blocks that call gitIdentityResolver.resolveIdentity(...) and gitIdentityResolver.isWorktree(...) to include expect(identity?.remoteUrl).toBe(...) using the fixture's origin URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/main/services/parsing/GitIdentityResolver.upwards.test.ts`:
- Around line 42-54: Add assertions to exercise the config-parsing path by
checking the resolved identity's remote URL: after calling
gitIdentityResolver.resolveIdentity(...) in each test, assert that
identity.remoteUrl equals the expected remote (the `[remote "origin"]` URL
present in the fixture). This will exercise getRemoteUrl → parseConfig
end-to-end; update the two it blocks that call
gitIdentityResolver.resolveIdentity(...) and gitIdentityResolver.isWorktree(...)
to include expect(identity?.remoteUrl).toBe(...) using the fixture's origin URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffa3787c-86b8-43a2-a9fa-1375d7963255
📒 Files selected for processing (2)
test/main/services/parsing/GitIdentityResolver.test.tstest/main/services/parsing/GitIdentityResolver.upwards.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/main/services/parsing/GitIdentityResolver.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitIdentityResolver previously only looked for a
.gitfile/directory exactly atprojectPathinstead of searching upwards to the project root. This caused discovery to fail when Claude was run inside a subdirectory of a git worktree or normal repository. Without a resolved identity,WorktreeGrouperfell back to heuristic path parsing and incorrectly split the session into a phantom repository group named after the subdirectory, preventing it from appearing in the worktree/session dropdown under the main repository.Fix:
resolveIdentity,isWorktree,getBranch,detectWorktreeSource, andgetGitWorktreeNameto search upwards for the.gitfile/directory.GitIdentityResolverandWorktreeGrouperto ensure worktrees and subdirectories group correctly.Fixes #188.
Summary by CodeRabbit
Bug Fixes
Tests