fix(sdd): detect git worktrees in findProjectRoot and resolve main repo root#1180
fix(sdd): detect git worktrees in findProjectRoot and resolve main repo root#1180blak0p wants to merge 1 commit into
Conversation
…po root In a git worktree, .git is a regular file (not a directory) containing 'gitdir: /path/to/main/.git/worktrees/name'. findProjectRoot() treated it as a strong marker and returned the worktree root, causing Engram to create duplicate projects per worktree and the SDD orchestrator to re-trigger sdd-init. Now when .git is a file, the function reads the gitdir: line, resolves the shared .git directory, and returns the main repo root. Falls back to 'git rev-parse --git-common-dir' if file parsing fails. Normal .git directories are unaffected. Closes Gentleman-Programming#702
📝 WalkthroughWalkthroughProject-root detection now handles Git worktrees whose ChangesGit worktree root resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/components/sdd/inject_test.go`:
- Around line 5210-5211: Update the test around the findProjectRoot assertion to
resolve symlinks on mainRoot before comparing it with got, using
filepath.EvalSymlinks and handling the returned error. Compare the resolved
expected root against got while preserving the existing failure message and test
intent.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a872e091-9cca-4121-b172-3544b463b48f
⛔ Files ignored due to path filters (1)
openspec/specs/sdd-orchestrator-assets/spec.mdis excluded by!openspec/**
📒 Files selected for processing (2)
internal/components/sdd/inject.gointernal/components/sdd/inject_test.go
| if got != mainRoot { | ||
| t.Fatalf("findProjectRoot = %q, want main repo root %q", got, mainRoot) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test will fail on macOS due to symlink resolution mismatch.
resolveWorktreeRoot calls filepath.EvalSymlinks on the computed commonGitDir and returns filepath.Dir(resolved). On macOS, t.TempDir() returns a path under /var/folders/... where /var is a symlink to /private/var. After EvalSymlinks, got becomes /private/var/folders/.../T/mainRoot while mainRoot remains /var/folders/.../T/mainRoot, causing the assertion to fail.
🛡️ Proposed fix: resolve symlinks on mainRoot before comparison
func TestFindProjectRootWorktreeGitFile(t *testing.T) {
mainRoot := t.TempDir()
+ // Resolve symlinks so the comparison matches filepath.EvalSymlinks
+ // called inside resolveWorktreeRoot (macOS /var → /private/var).
+ resolvedMainRoot, err := filepath.EvalSymlinks(mainRoot)
+ if err != nil {
+ t.Fatalf("EvalSymlinks(mainRoot): %v", err)
+ }
// Main repo has a real .git directory with a worktrees subdirectory.
- mainGitDir := filepath.Join(mainRoot, ".git")
+ mainGitDir := filepath.Join(resolvedMainRoot, ".git")Then update the assertion:
- if got != mainRoot {
- t.Fatalf("findProjectRoot = %q, want main repo root %q", got, mainRoot)
+ if got != resolvedMainRoot {
+ t.Fatalf("findProjectRoot = %q, want main repo root %q", got, resolvedMainRoot)
}📝 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.
| if got != mainRoot { | |
| t.Fatalf("findProjectRoot = %q, want main repo root %q", got, mainRoot) | |
| if got != resolvedMainRoot { | |
| t.Fatalf("findProjectRoot = %q, want main repo root %q", got, resolvedMainRoot) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/components/sdd/inject_test.go` around lines 5210 - 5211, Update the
test around the findProjectRoot assertion to resolve symlinks on mainRoot before
comparing it with got, using filepath.EvalSymlinks and handling the returned
error. Compare the resolved expected root against got while preserving the
existing failure message and test intent.
1 participant
🔗 Linked Issue
Closes #702
🏷️ PR Type
type:bug— Bug fix (non-breaking change that fixes an issue)📝 Summary
findProjectRoot()now detects when.gitis a regular file (git worktree) vs a directory (normal repo).gitis a file, it reads thegitdir:line and resolves the main repository rootgit rev-parse --git-common-dirif file parsing fails.gitdirectories are completely unaffected📂 Changes
internal/components/sdd/inject.goresolveWorktreeRoot()andresolveGitCommonDir()helpers; modifiedfindProjectRoot()to special-case.gitfile vs directoryinternal/components/sdd/inject_test.go.gitfile resolution, malformed fallback, normal.gitdir regression guardopenspec/specs/sdd-orchestrator-assets/spec.md🧪 Test Plan
Unit Tests
go test ./...go test ./...) — 13/13 findProjectRoot tests pass (10 existing + 3 new)cd e2e && ./docker-test.sh)✅ Contributor Checklist
status:approvedsize:exceptionwith rationale documentedtype:*label to this PRgo test ./...)cd e2e && ./docker-test.sh)Co-Authored-Bytrailers💬 Notes for Reviewers
The change is ~270 lines total (~120 code, ~99 tests, ~55 spec). Well under the 400-line budget. All existing tests pass unchanged.
Summary by CodeRabbit
Bug Fixes
.gitfile.Tests