From fc5021c83ad772a25f3e43ba98cb79387e41b703 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 15 Oct 2026 16:15:52 -0500 Subject: [PATCH] Scope git-review running to the current branch Report activity only when the current branch owns a live review lock. This prevents agents from deferring work or feedback because another branch has an active review. Keep the lock repository-wide to prevent concurrent reviews from using the shared review checkout. --- README.md | 7 ++++--- internal/cli/activity_test.go | 24 +++++++++++++++--------- internal/cli/cli.go | 2 +- internal/review/run.go | 10 ++++++++++ internal/review/service.go | 8 ++++++-- internal/review/worktree.go | 13 +++++++++++-- 6 files changed, 47 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 3a7c12f..26b0caf 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ Remove any manually installed copies of the git-review extension or skill to avo The extension source lives in [`pi/extensions/git-review/`](pi/extensions/git-review/), with `index.ts` as its entry point and tests in `tests/`. It publishes review counts for other extensions and asks the model to use the git-review skill when feedback exists after an agent run. -It defers model notices while a review process holds the shared worktree lock. +It defers model notices while a review is active for the current branch. Deferred feedback remains eligible for notification after the review ends. Activity-query failures suppress notices rather than assume that review ended. It runs after the mise on-stop hook when the mise-hooks extension is present. @@ -155,10 +155,11 @@ git-review running --json ``` Successful output is `{"version":1,"running":true}` or `{"version":1,"running":false}`. -The command checks the repository-wide review worktree lock without changing review state or creating files. +The command reports whether the current branch owns the live review worktree lock without changing review state or creating files. A running automatic review holds this lock across all selected stacks. A persisted `reviewing` attempt alone does not indicate a running process. -Linked worktrees share the same activity signal. +A review on another branch returns `running: false`, including when that branch is in a linked worktree. +The lock remains repository-wide to prevent concurrent reviews from using the shared review checkout. Command failures exit with status 1 and emit the standard versioned JSON error object. ### Collect saved feedback diff --git a/internal/cli/activity_test.go b/internal/cli/activity_test.go index da913ed..80b4806 100644 --- a/internal/cli/activity_test.go +++ b/internal/cli/activity_test.go @@ -1,9 +1,9 @@ package cli import ( + "context" "os" "path/filepath" - "syscall" "testing" "github.com/stretchr/testify/require" @@ -37,17 +37,23 @@ func TestReviewRunningReportsLiveLockNotPersistedAttempt(t *testing.T) { return nil })) check(dir, false) - require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) - lock, err := os.OpenFile(path+".git-review.lock", os.O_CREATE|os.O_RDWR, 0o600) - require.NoError(t, err) - t.Cleanup(func() { require.NoError(t, lock.Close()) }) - require.NoError(t, syscall.Flock(int(lock.Fd()), syscall.LOCK_EX|syscall.LOCK_NB)) - check(dir, true) other := filepath.Join(t.TempDir(), "checkout") git(t, dir, "worktree", "add", "-b", "other", other, "HEAD") - check(other, true) - require.NoError(t, syscall.Flock(int(lock.Fd()), syscall.LOCK_UN)) + called := false + reviewer := review.Reviewer{ + Service: review.Service{Repository: review.Repository{Dir: dir}}, + Choose: func(context.Context, *review.Decision) (string, error) { + called = true + check(dir, true) + check(other, false) + return "cancel", nil + }, + } + _, err = reviewer.Review(t.Context(), review.ReviewOptions{}) + require.NoError(t, err) + require.True(t, called) check(dir, false) + check(other, false) } func TestReviewRunningOutsideRepository(t *testing.T) { diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 911cb56..4f364ce 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -82,7 +82,7 @@ func (a *application) command() *cobra.Command { }} status.Flags().BoolVarP(&a.verbose, "verbose", "v", false, "Show saved review details before the table") status.Flags().BoolVar(&onlyNeedsReview, "needs-review", false, "Show unresolved stacks and active review attempts") - activity := &cobra.Command{Use: "running", Short: "Check whether a review process holds the shared worktree lock", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { + activity := &cobra.Command{Use: "running", Short: "Check whether a review is running for the current branch", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { running, err := a.service.ReviewRunning(cmd.Context()) if err != nil { return err diff --git a/internal/review/run.go b/internal/review/run.go index c901cc6..d5a43be 100644 --- a/internal/review/run.go +++ b/internal/review/run.go @@ -47,11 +47,21 @@ func (r Reviewer) Review(ctx context.Context, options ReviewOptions) (result Res if err != nil { return result, err } + branch, err := r.Service.Repository.git(ctx, "symbolic-ref", "--quiet", "HEAD") + if err != nil { + return result, err + } lock, err := lockWorktree(path) if err != nil { return result, err } defer func() { err = errors.Join(err, lock.Close()) }() + if err := lock.Truncate(0); err != nil { + return result, err + } + if _, err := lock.WriteString(branch); err != nil { + return result, err + } result, err = r.reviewSequence(ctx, options, path, lock) if err != nil { return result, err diff --git a/internal/review/service.go b/internal/review/service.go index 6304376..3656244 100644 --- a/internal/review/service.go +++ b/internal/review/service.go @@ -121,14 +121,18 @@ func (s Service) Feedback(ctx context.Context) (Result, error) { return result, nil } -// ReviewRunning reports whether a process holds the shared review worktree lock. +// ReviewRunning reports whether the current branch owns the live review lock. // It does not create the lock file or infer activity from persisted attempts. func (s Service) ReviewRunning(ctx context.Context) (bool, error) { path, err := WorktreePath(s.Repository.Dir) if err != nil { return false, err } - return reviewRunning(ctx, path) + branch, err := s.Repository.git(ctx, "symbolic-ref", "--quiet", "HEAD") + if err != nil { + return false, err + } + return reviewRunning(ctx, path, branch) } // Status returns current stacks and ordered commit counts without changing review state. diff --git a/internal/review/worktree.go b/internal/review/worktree.go index 68c9c33..3798652 100644 --- a/internal/review/worktree.go +++ b/internal/review/worktree.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "os" "path/filepath" "slices" @@ -25,7 +26,7 @@ func WorktreePath(repo string) (string, error) { } // reviewRunning probes an existing lock without creating it. -func reviewRunning(ctx context.Context, path string) (running bool, err error) { +func reviewRunning(ctx context.Context, path, branch string) (running bool, err error) { file, err := os.OpenFile(path+".git-review.lock", os.O_RDONLY, 0) if errors.Is(err, os.ErrNotExist) { return false, nil @@ -39,7 +40,15 @@ func reviewRunning(ctx context.Context, path string) (running bool, err error) { } err = syscall.Flock(int(file.Fd()), syscall.LOCK_SH|syscall.LOCK_NB) if errors.Is(err, syscall.EWOULDBLOCK) { - return true, nil + owner, err := io.ReadAll(file) + if err != nil { + return false, err + } + // Metadata left after process exit is not evidence of a live review. + if err := syscall.Flock(int(file.Fd()), syscall.LOCK_SH|syscall.LOCK_NB); !errors.Is(err, syscall.EWOULDBLOCK) { + return false, err + } + return string(owner) == branch, nil } return false, err }