-
Notifications
You must be signed in to change notification settings - Fork 0
feat: make rig work project-aware #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
thoreinstein
commented
Jan 26, 2026
- Support [project:]ticket format in 'rig work' and 'rig session'
- Auto-detect repository from ticket prefix if not in a git repo
- Update tmux session naming to include project prefix when specified
- Make 'rig merge' session killing project-aware
- Allow WorktreeManager to be initialized at an explicit path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds project-aware functionality to the rig work and rig session commands, enabling users to work with tickets across multiple repositories without being in a specific git directory.
Changes:
- Added support for
[project:]ticketformat in work and session commands - Implemented auto-detection of repositories from ticket prefix or explicit project name
- Updated tmux session naming to include project prefix when specified
- Made the merge workflow's session killing project-aware
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/steps.go | Added project-aware tmux session killing that tries both plain and project-prefixed session names; added getRepoName helper method |
| pkg/workflow/merge.go | Added projectPath field to Engine struct to support project-aware operations |
| pkg/git/worktree.go | Added NewWorktreeManagerAtPath constructor to allow working with repos at explicit paths |
| cmd/work.go | Implemented ticket parsing with optional project prefix, repository location logic, and helper functions for git repo detection |
| cmd/session.go | Updated session attach and kill commands to handle project-prefixed tickets |
| cmd/work_project_aware_test.go | Added comprehensive tests for new project-aware functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/work.go
Outdated
| } | ||
|
|
||
| // 2. Current directory check | ||
| cwd, _ := os.Getwd() |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error from os.Getwd() is silently ignored. If getting the current working directory fails, the code will use an empty string for cwd, potentially leading to incorrect behavior when checking if it's a git repository. Consider handling this error explicitly.
| cwd, _ := os.Getwd() | |
| cwd, err := os.Getwd() | |
| if err != nil { | |
| return "", errors.Wrap(err, "failed to get current working directory") | |
| } |
cmd/work.go
Outdated
| func locateRepo(name string, cfg *config.Config) (string, error) { | ||
| basePath := cfg.Clone.BasePath | ||
| if basePath == "" { | ||
| home, _ := os.UserHomeDir() |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error from os.UserHomeDir() is silently ignored. If getting the home directory fails, basePath will be set to "/src" (empty home + "/src"), which is likely not the intended behavior. Consider handling this error explicitly or at minimum logging it.
| home, _ := os.UserHomeDir() | |
| home, err := os.UserHomeDir() | |
| if err != nil { | |
| return "", errors.Wrap(err, "failed to determine user home directory") | |
| } |
cmd/work.go
Outdated
| if _, err := os.Stat(headPath); err == nil { | ||
| if _, err := os.Stat(configPath); err == nil { | ||
| return true |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bare repository detection only checks for HEAD and config files, but doesn't verify the objects directory, which is essential for a valid git repository. Consider also checking for the objects directory to ensure more robust bare repository detection, or use git commands for validation.
| if _, err := os.Stat(headPath); err == nil { | |
| if _, err := os.Stat(configPath); err == nil { | |
| return true | |
| objectsPath := filepath.Join(path, "objects") | |
| if _, err := os.Stat(headPath); err == nil { | |
| if _, err := os.Stat(configPath); err == nil { | |
| if info, err := os.Stat(objectsPath); err == nil && info.IsDir() { | |
| return true | |
| } |
pkg/workflow/steps.go
Outdated
| } else { | ||
| absPath = filepath.Join(e.projectPath, commonDir) | ||
| } | ||
| absPath, _ = filepath.Abs(absPath) |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error returned by filepath.Abs is being silently ignored. If filepath.Abs fails, it returns an empty string and an error. This could lead to incorrect behavior when determining the repository name. Consider handling the error explicitly or at least logging it if it's expected to never fail in practice.
| absPath, _ = filepath.Abs(absPath) | |
| absPath, err = filepath.Abs(absPath) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to resolve git common dir %q to absolute path: %w", absPath, err) | |
| } |
cmd/work.go
Outdated
| sessionID := ticketInfo.ID | ||
| if ticketInfo.Project != "" { | ||
| sessionID = ticketInfo.Project + "-" + ticketInfo.ID | ||
| } |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for building sessionID from ticketInfo (lines 282-285 in work.go, 104-107 in session.go, 136-139 in session.go) is duplicated across multiple locations. Consider extracting this into a helper function to improve maintainability and ensure consistency.
| // NewWorktreeManagerAtPath creates a new WorktreeManager for a specific repository path | ||
| func NewWorktreeManagerAtPath(repoPath, baseBranchConfig string, verbose bool) *WorktreeManager { | ||
| return &WorktreeManager{ | ||
| RepoPath: repoPath, | ||
| Verbose: verbose, | ||
| BaseBranchConfig: baseBranchConfig, | ||
| runner: &RealCommandRunner{Verbose: verbose}, | ||
| getwd: os.Getwd, | ||
| } | ||
| } |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new constructor NewWorktreeManagerAtPath lacks test coverage. Given that the codebase has comprehensive test coverage for git operations (see worktree_test.go with 1164 lines), consider adding tests for this new function to verify it correctly initializes the RepoPath field and that GetRepoRoot works properly with explicit paths.
pkg/git/worktree.go
Outdated
| cwd := dir | ||
| if !filepath.IsAbs(cwd) { | ||
| var err error | ||
| cwd, err = wm.getwd() | ||
| if err != nil { | ||
| return "", errors.Wrap(err, "failed to get working directory") | ||
| } | ||
| cwd = filepath.Join(cwd, dir) |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path resolution logic here may have an issue. When dir is a relative path (e.g., "../../some/repo"), the code checks if it's absolute (it's not), then calls getwd() and joins them with filepath.Join(cwd, dir). However, this may not produce the correct absolute path if dir is already a complex relative path. Consider using filepath.Abs(dir) instead to properly resolve the directory before joining it with commonDir.
| path := filepath.Join(basePath, name) | ||
| if isGitRepo(path) { | ||
| return path, nil | ||
| } |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When name contains a slash (owner/repo format), the function tries the direct path but doesn't return an error if it fails. It then continues to search one level deep, which will never match because it's looking for "basePath/<entry>/owner/repo". Consider returning an explicit error when the owner/repo format is detected but not found at the expected path, rather than falling through to a search that will never succeed.
| } | |
| } | |
| return "", errors.Newf("could not find repository %q at %s", name, path) |
- Support [project:]ticket format in 'rig work' and 'rig session' - Auto-detect repository from ticket prefix if not in a git repo - Update tmux session naming to include project prefix when specified - Make 'rig merge' session killing project-aware - Allow WorktreeManager to be initialized at an explicit path
e07670d to
2c02458
Compare
Refactored session ID generation into a method on TicketInfo to remove duplication across session and work commands.
Improved robustness of repository discovery and file system operations: - Enhanced isGitRepo to check for the 'objects' directory in bare repos - Improved path resolution in GetRepoRoot for relative and absolute paths - Added explicit error checking for home directory and path resolution - Added unit tests for WorktreeManager with explicit paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 3. Auto-detect based on ticket type | ||
| path, err := locateRepo(ticketType, cfg) | ||
| if err == nil { | ||
| if verbose { |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'verbose' is referenced here but it's a global variable from the root command package. This function doesn't have direct access to it in its local scope, which could lead to incorrect behavior. The function should either accept verbose as a parameter or retrieve it from the config object to ensure it works correctly in all contexts.
| if err == nil { | ||
| return path, nil | ||
| } | ||
| return "", err |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling in this block is problematic. When err is nil (meaning the path was found), you correctly return it. However, when err is not nil, you immediately return the error. This prevents the code from attempting fallback option #2 (current directory check). The logic should continue to the next check instead of returning immediately, so that all three priority levels are attempted.
| return "", err | |
| if verbose { | |
| fmt.Printf("Failed to locate repository for project %q: %v\n", project, err) | |
| } | |
| // Fall through to other detection methods |
| // locateRepo searches for a repository by name in the configured base path. | ||
| func locateRepo(name string, cfg *config.Config) (string, error) { | ||
| basePath := cfg.Clone.BasePath | ||
| if basePath == "" { | ||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return "", errors.Wrap(err, "failed to determine user home directory") | ||
| } | ||
| basePath = filepath.Join(home, "src") | ||
| } | ||
|
|
||
| // Try direct path if name contains a slash (owner/repo) | ||
| if strings.Contains(name, "/") { | ||
| path := filepath.Join(basePath, name) | ||
| if isGitRepo(path) { | ||
| return path, nil | ||
| } | ||
| return "", errors.Newf("could not find repository %q at %s", name, path) | ||
| } | ||
|
|
||
| // Search one level deep for the repo name | ||
| entries, err := os.ReadDir(basePath) | ||
| if err != nil { | ||
| return "", errors.Wrapf(err, "failed to read base path: %s", basePath) | ||
| } | ||
|
|
||
| for _, entry := range entries { | ||
| if entry.IsDir() { | ||
| path := filepath.Join(basePath, entry.Name(), name) | ||
| if isGitRepo(path) { | ||
| return path, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return "", errors.Newf("could not find repository %q in %s", name, basePath) |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The locateRepo function searches for repositories in the base path, but when it finds a match using the pattern basePath/entry.Name()/name, the result is an absolute path. However, in findRepoPath, when CWD is a git repo, it returns "." (a relative path). This inconsistency could cause issues in NewWorktreeManagerAtPath if it expects consistent path types. Consider either always returning absolute paths or documenting that both relative and absolute paths are acceptable.
| // SessionID returns a sanitized ticket identifier suitable for tmux session names | ||
| func (t *TicketInfo) SessionID() string { | ||
| if t.Project != "" { | ||
| return t.Project + "-" + t.ID |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SessionID method concatenates project and ID with a hyphen, which could create ambiguous session names. For example, if project="foo-bar" and ID="baz-123", the result would be "foo-bar-baz-123", which is indistinguishable from project="foo" and ID="bar-baz-123". Consider using a different separator that's not allowed in ticket IDs, or documenting this limitation clearly.
| return t.Project + "-" + t.ID | |
| // Use a separator that cannot appear in ticket IDs (which are [a-zA-Z]+-[a-zA-Z0-9]+) | |
| // to avoid ambiguous session names when projects or IDs contain hyphens. | |
| return t.Project + "::" + t.ID |
| func TestGetRepoRoot_ExplicitPath(t *testing.T) { | ||
| mock := &MockCommandRunner{ | ||
| OutputFunc: func(dir string, name string, args ...string) ([]byte, error) { | ||
| if dir != "/custom/path/to/repo" { | ||
| return nil, errors.New("called with wrong directory") | ||
| } | ||
| return []byte(".\n"), nil | ||
| }, | ||
| } | ||
| wm := NewWorktreeManagerWithRunner("", false, mock) | ||
| wm.RepoPath = "/custom/path/to/repo" | ||
| wm.getwd = func() (string, error) { | ||
| return "/custom/path/to/repo", nil | ||
| } | ||
|
|
||
| root, err := wm.GetRepoRoot() | ||
| if err != nil { | ||
| t.Fatalf("GetRepoRoot() error = %v, want nil", err) | ||
| } | ||
|
|
||
| if root != "/custom/path/to/repo" { | ||
| t.Errorf("GetRepoRoot() = %q, want %q", root, "/custom/path/to/repo") | ||
| } | ||
| } |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test creates a WorktreeManager with NewWorktreeManagerWithRunner but doesn't set the RepoPath field. This means it's testing the GetRepoRoot method with an empty RepoPath field rather than the explicit path functionality that NewWorktreeManagerAtPath provides. To properly test the explicit path feature, you should either use NewWorktreeManagerAtPath or set wm.RepoPath before calling GetRepoRoot.
| var lastErr error | ||
| for _, name := range sessionNames { | ||
| // Construct full session name using configured prefix | ||
| fullSessionName := name | ||
| if e.cfg.Tmux.SessionPrefix != "" { | ||
| fullSessionName = e.cfg.Tmux.SessionPrefix + name | ||
| } | ||
|
|
||
| cmd := exec.Command("tmux", "kill-session", "-t", fullSessionName) | ||
| if err := cmd.Run(); err == nil { | ||
| return nil // Successfully killed a session | ||
| } else { | ||
| lastErr = err | ||
| } | ||
| } | ||
|
|
||
| return lastErr |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When killTmuxSession fails to kill any session, it returns lastErr which is the error from the last attempt. However, this error message won't indicate that multiple session names were tried. For better debugging, consider wrapping the error with context about which session names were attempted, or only trying the project-prefixed version if the plain ticket ID fails (to avoid misleading error messages).
| // getRepoName attempts to determine the repository name from the project path. | ||
| func (e *Engine) getRepoName() (string, error) { | ||
| // Use git rev-parse --git-common-dir to find the repo root | ||
| cmd := exec.Command("git", "rev-parse", "--git-common-dir") | ||
| cmd.Dir = e.projectPath | ||
| output, err := cmd.Output() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| commonDir := strings.TrimSpace(string(output)) | ||
| if commonDir == "" { | ||
| return "", errors.New("empty git common dir") | ||
| } | ||
|
|
||
| // Resolve to absolute path | ||
| var absPath string | ||
| if filepath.IsAbs(commonDir) { | ||
| absPath = commonDir | ||
| } else { | ||
| absPath = filepath.Join(e.projectPath, commonDir) | ||
| } | ||
| absPath, err = filepath.Abs(absPath) | ||
| if err != nil { | ||
| return "", errors.Wrapf(err, "failed to resolve absolute path for %s", absPath) | ||
| } | ||
|
|
||
| cmd := exec.Command("tmux", "kill-session", "-t", sessionName) | ||
| return cmd.Run() | ||
| // For bare repos, commonDir might be ".", so basename is the repo name | ||
| // For worktrees, commonDir points to the .git dir of the main repo | ||
| return filepath.Base(absPath), nil | ||
| } |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation duplicates the logic from WorktreeManager.GetRepoRoot in pkg/git/worktree.go. Instead of reimplementing git repository name resolution, consider reusing the existing WorktreeManager.GetRepoName method. This would improve maintainability and ensure consistent behavior across the codebase. If the workflow package needs this functionality, the Engine could use a WorktreeManager instance or delegate to shared utility functions.
| func TestRunWorkCommand_ProjectAware(t *testing.T) { | ||
| // Skip if git is not available | ||
| if _, err := exec.LookPath("git"); err != nil { | ||
| t.Skip("git not found in PATH, skipping test") | ||
| } | ||
|
|
||
| tmpDir := t.TempDir() | ||
| srcDir := filepath.Join(tmpDir, "src") | ||
| if err := os.MkdirAll(srcDir, 0755); err != nil { | ||
| t.Fatalf("failed to create src dir: %v", err) | ||
| } | ||
|
|
||
| notesDir := t.TempDir() | ||
| setupWorkTestConfig(t, notesDir) | ||
| viper.Set("clone.base_path", srcDir) | ||
| defer viper.Reset() | ||
|
|
||
| // Helper to setup a bare git repo | ||
| setupBareRepo := func(t *testing.T, path string) { | ||
| if err := exec.Command("git", "init", "--bare", path).Run(); err != nil { | ||
| t.Fatalf("git init --bare failed: %v", err) | ||
| } | ||
| // We need at least one commit for worktree add to work. | ||
| // So we create a temporary clone, commit, and push. | ||
| tmpClone := filepath.Join(t.TempDir(), "rig-test-clone") | ||
| if err := exec.Command("git", "clone", path, tmpClone).Run(); err != nil { | ||
| t.Fatalf("git clone failed: %v", err) | ||
| } | ||
| if err := exec.Command("git", "-C", tmpClone, "config", "user.email", "test@example.com").Run(); err != nil { | ||
| t.Fatalf("git config email failed: %v", err) | ||
| } | ||
| if err := exec.Command("git", "-C", tmpClone, "config", "user.name", "Test User").Run(); err != nil { | ||
| t.Fatalf("git config name failed: %v", err) | ||
| } | ||
| if err := exec.Command("git", "-C", tmpClone, "commit", "--allow-empty", "-m", "Initial commit").Run(); err != nil { | ||
| t.Fatalf("git commit failed: %v", err) | ||
| } | ||
| if err := exec.Command("git", "-C", tmpClone, "push", "origin", "HEAD").Run(); err != nil { | ||
| t.Fatalf("git push failed: %v", err) | ||
| } | ||
| } | ||
|
|
||
| repo1Path := filepath.Join(srcDir, "owner1", "repo1") | ||
| if err := os.MkdirAll(repo1Path, 0755); err != nil { | ||
| t.Fatalf("failed to create repo1 dir: %v", err) | ||
| } | ||
| setupBareRepo(t, repo1Path) | ||
|
|
||
| // Run from a neutral directory | ||
| t.Chdir(tmpDir) | ||
|
|
||
| err := runWorkCommand("repo1:proj-123") | ||
| if err != nil { | ||
| t.Logf("runWorkCommand warning (likely tmux): %v", err) | ||
| } | ||
|
|
||
| // Check if worktree was created in repo1 | ||
| worktreePath := filepath.Join(repo1Path, "proj", "proj-123") | ||
| if _, err := os.Stat(worktreePath); os.IsNotExist(err) { | ||
| t.Errorf("Worktree should be created at %s", worktreePath) | ||
| } | ||
|
|
||
| // Verify note was created | ||
| notePath := filepath.Join(notesDir, "proj", "proj-123.md") | ||
| if _, err := os.Stat(notePath); os.IsNotExist(err) { | ||
| t.Errorf("Note should be created at %s", notePath) | ||
| } | ||
| } |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test coverage for the new project-aware functionality appears incomplete. The TestRunWorkCommand_ProjectAware test only verifies the happy path (creating a worktree with a project prefix). Consider adding tests for error scenarios such as: 1) invalid project names, 2) project not found in base path, 3) fallback behavior when a project is specified but doesn't exist, 4) conflict scenarios where both a project prefix and being in a git repo apply. This would ensure the priority logic in findRepoPath works correctly across all cases.
| return nil, errors.New("invalid ticket format. Project name cannot be empty when using ':'") | ||
| } | ||
| project = p | ||
| ticket = t |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parseTicket function allows colons in project names (e.g., "owner/repo:ticket-123"), but the colon character is used as the delimiter between project and ticket. This means inputs like "a:b:c-123" would be ambiguous - should it be project="a" with ticket="b:c-123", or is it an error? The current implementation would parse it as project="a" and ticket="b:c-123", but then the ticket regex would fail. Consider either explicitly validating that the project portion doesn't contain colons, or documenting this behavior clearly.
| ticket = t | |
| ticket = t | |
| // Disallow additional ':' in the ticket portion to avoid ambiguous inputs like "a:b:c-123" | |
| if strings.Contains(ticket, ":") { | |
| return nil, errors.New("invalid ticket format. Only a single ':' is allowed to separate project and ticket (e.g., project:TYPE-ID)") | |
| } |