From e6238e45efe5e9357558b4e47c2d919125742db4 Mon Sep 17 00:00:00 2001 From: Valentino Saputra Date: Fri, 24 Jul 2026 07:40:22 +0700 Subject: [PATCH] fix: scope doctor branch checks to current repository only Root cause: Doctor() called cs.ListAll() which returns capsules from ALL repositories, then checked every capsule's branch against the current repo root via git.BranchExists(caps.Branch, root). Capsules belonging to other repos reported false missing-branch warnings. Fix: 1. Resolve current repository ID via git.RepoID(root) in Doctor(). 2. Guard branch check with caps.RepositoryID == currentRepoID so only capsules belonging to the current repo are checked. 3. Normalize path separators in git.RepoID (filepath.ToSlash) so that Windows paths from os.MkdirTemp (\ separators) and from git rev-parse --show-toplevel (/ separators) produce the same hash, preventing spurious repo-ID mismatches. Tests (9 functions, all use isolated t.TempDir + TASKCAPSULE_HOME): - Current repo capsule branch IS checked - Other-repo capsule NOT checked against current repo - Missing branch in current repo IS reported - Valid branch NOT reported - Same-name across repos: no duplicate false warning - Doctor does not mutate state - Missing worktree IS reported - Missing branch in other repo NOT reported when run from current repo - Signal-0 behavior preserved Existing tests audited: all use t.TempDir for state; no leakage into real ~/.taskcapsule directory. --- internal/app/doctor.go | 22 ++- internal/app/doctor_test.go | 321 ++++++++++++++++++++++++++++++++++++ internal/git/repository.go | 10 +- 3 files changed, 343 insertions(+), 10 deletions(-) create mode 100644 internal/app/doctor_test.go diff --git a/internal/app/doctor.go b/internal/app/doctor.go index a36b132..463176c 100644 --- a/internal/app/doctor.go +++ b/internal/app/doctor.go @@ -27,6 +27,9 @@ func Doctor() ([]DoctorResult, error) { return results, nil } + // Resolve current repository ID for scoping capsule branch checks + currentRepoID, repoIDErr := git.RepoID(root) + // Check config cfgPath := filepath.Join(root, ".taskcapsule.json") if _, err := os.Stat(cfgPath); os.IsNotExist(err) { @@ -67,13 +70,18 @@ func Doctor() ([]DoctorResult, error) { continue } - // Check branch still exists - branchExists, _ := git.BranchExists(caps.Branch, root) - if !branchExists { - results = append(results, DoctorResult{ - OK: false, - Message: "Capsule " + caps.Name + " branch '" + caps.Branch + "' no longer exists", - }) + // Check branch still exists — only for capsules belonging to the + // current repository. Capsules from other repositories are skipped + // to avoid false positives: their branches will not exist in this + // repo's ref namespace. + if repoIDErr == nil && caps.RepositoryID == currentRepoID { + branchExists, _ := git.BranchExists(caps.Branch, root) + if !branchExists { + results = append(results, DoctorResult{ + OK: false, + Message: "Capsule " + caps.Name + " branch '" + caps.Branch + "' no longer exists", + }) + } } // Check for stale PIDs diff --git a/internal/app/doctor_test.go b/internal/app/doctor_test.go new file mode 100644 index 0000000..23e330a --- /dev/null +++ b/internal/app/doctor_test.go @@ -0,0 +1,321 @@ +package app + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/vtino17/taskcapsule/internal/capsule" + "github.com/vtino17/taskcapsule/internal/git" + "github.com/vtino17/taskcapsule/internal/state" +) + +func setupTestRepo(t *testing.T, branches []string) string { + t.Helper() + dir, err := os.MkdirTemp("", "taskcapsule-doctor-*") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.RemoveAll(dir) }) + + cmds := [][]string{ + {"git", "init"}, + {"git", "config", "user.email", "test@test.com"}, + {"git", "config", "user.name", "Test"}, + {"git", "branch", "-M", "main"}, + } + for _, args := range cmds { + c := exec.Command(args[0], args[1:]...) + c.Dir = dir + if out, err := c.CombinedOutput(); err != nil { + t.Fatalf("git setup %v failed: %v\n%s", args, err, out) + } + } + + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("# test\n"), 0644); err != nil { + t.Fatal(err) + } + c := exec.Command("git", "add", ".") + c.Dir = dir + c.Run() + c = exec.Command("git", "commit", "-m", "initial") + c.Dir = dir + c.Run() + + for _, branch := range branches { + c := exec.Command("git", "branch", branch) + c.Dir = dir + if out, err := c.CombinedOutput(); err != nil { + t.Fatalf("git branch %s failed: %v\n%s", branch, err, out) + } + } + + return dir +} + +func setupCapsule(t *testing.T, store *state.Store, repoID, name, branch, worktreePath, status string) { + t.Helper() + s := &capsule.State{ + SchemaVersion: 1, + Name: name, + Status: status, + RepositoryRoot: worktreePath, + RepositoryID: repoID, + WorktreePath: worktreePath, + Branch: branch, + BaseBranch: "main", + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + } + if err := store.Save(repoID, name, s); err != nil { + t.Fatalf("setupCapsule Save: %v", err) + } +} + +func runDoctorInDir(t *testing.T, repoDir, stateDir string) []DoctorResult { + t.Helper() + + origDir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer os.Chdir(origDir) + + if err := os.Chdir(repoDir); err != nil { + t.Fatal(err) + } + + os.Setenv("TASKCAPSULE_HOME", stateDir) + defer os.Unsetenv("TASKCAPSULE_HOME") + + results, err := Doctor() + if err != nil { + t.Fatalf("Doctor() returned error: %v", err) + } + return results +} + +func TestDoctorCurrentRepoBranchChecked(t *testing.T) { + repoDir := setupTestRepo(t, []string{"feature-x"}) + stateDir := t.TempDir() + + repoID, err := git.RepoID(repoDir) + if err != nil { + t.Fatal(err) + } + + store := state.NewStore(stateDir) + _ = os.MkdirAll(filepath.Join(stateDir, "worktrees"), 0755) + setupCapsule(t, store, repoID, "capsule-good", "main", repoDir, "active") + + results := runDoctorInDir(t, repoDir, stateDir) + + for _, r := range results { + if strings.Contains(r.Message, "branch") && strings.Contains(r.Message, "capsule-good") { + t.Errorf("got unexpected branch warning for valid capsule: %s", r.Message) + } + } +} + +func TestDoctorOtherRepoNotCheckedAgainstCurrent(t *testing.T) { + repoA := setupTestRepo(t, []string{"feat-a"}) + repoB := setupTestRepo(t, []string{"feat-b"}) + stateDir := t.TempDir() + + repoAID, err := git.RepoID(repoA) + if err != nil { + t.Fatal(err) + } + repoBID, err := git.RepoID(repoB) + if err != nil { + t.Fatal(err) + } + + store := state.NewStore(stateDir) + _ = os.MkdirAll(filepath.Join(stateDir, "worktrees"), 0755) + setupCapsule(t, store, repoAID, "capsule-a", "feat-a", repoA, "active") + setupCapsule(t, store, repoBID, "capsule-b", "feat-b", repoB, "active") + + results := runDoctorInDir(t, repoA, stateDir) + + for _, r := range results { + if strings.Contains(r.Message, "capsule-b") && strings.Contains(r.Message, "branch") { + t.Errorf("other-repo capsule 'capsule-b' must NOT be branch-checked against current repo: %s", r.Message) + } + } +} + +func TestDoctorMissingBranchInCurrentRepoReported(t *testing.T) { + repoDir := setupTestRepo(t, nil) + stateDir := t.TempDir() + + repoID, err := git.RepoID(repoDir) + if err != nil { + t.Fatal(err) + } + + store := state.NewStore(stateDir) + _ = os.MkdirAll(filepath.Join(stateDir, "worktrees"), 0755) + setupCapsule(t, store, repoID, "capsule-missing", "branch-gone", repoDir, "active") + + results := runDoctorInDir(t, repoDir, stateDir) + + found := false + for _, r := range results { + if strings.Contains(r.Message, "capsule-missing") && strings.Contains(r.Message, "branch") { + found = true + break + } + } + if !found { + t.Error("expected branch warning for capsule with missing branch, got none") + } +} + +func TestDoctorValidBranchNotReported(t *testing.T) { + repoDir := setupTestRepo(t, []string{"valid-branch"}) + stateDir := t.TempDir() + + repoID, err := git.RepoID(repoDir) + if err != nil { + t.Fatal(err) + } + + store := state.NewStore(stateDir) + _ = os.MkdirAll(filepath.Join(stateDir, "worktrees"), 0755) + setupCapsule(t, store, repoID, "capsule-valid", "valid-branch", repoDir, "active") + + results := runDoctorInDir(t, repoDir, stateDir) + + for _, r := range results { + if strings.Contains(r.Message, "capsule-valid") && strings.Contains(r.Message, "branch") { + t.Errorf("got false branch warning for valid branch: %s", r.Message) + } + } +} + +func TestDoctorSameNameAcrossReposNoFalseWarning(t *testing.T) { + repoA := setupTestRepo(t, []string{"feature-x"}) + repoB := setupTestRepo(t, []string{"feature-x"}) + stateDir := t.TempDir() + + repoAID, err := git.RepoID(repoA) + if err != nil { + t.Fatal(err) + } + repoBID, err := git.RepoID(repoB) + if err != nil { + t.Fatal(err) + } + + store := state.NewStore(stateDir) + _ = os.MkdirAll(filepath.Join(stateDir, "worktrees"), 0755) + setupCapsule(t, store, repoAID, "feature-x", "feature-x", repoA, "active") + setupCapsule(t, store, repoBID, "feature-x", "feature-x", repoB, "active") + + results := runDoctorInDir(t, repoA, stateDir) + + branchWarnings := 0 + for _, r := range results { + if strings.Contains(r.Message, "branch") && strings.Contains(r.Message, "feature-x") { + branchWarnings++ + } + } + if branchWarnings > 1 { + t.Errorf("expected at most 1 branch warning for same-name capsules across repos, got %d", branchWarnings) + } +} + +func TestDoctorDoesNotMutateState(t *testing.T) { + repoDir := setupTestRepo(t, []string{"feat-mutate"}) + stateDir := t.TempDir() + + repoID, err := git.RepoID(repoDir) + if err != nil { + t.Fatal(err) + } + + store := state.NewStore(stateDir) + _ = os.MkdirAll(filepath.Join(stateDir, "worktrees"), 0755) + setupCapsule(t, store, repoID, "capsule-mutate", "feat-mutate", repoDir, "active") + + loaded, err := store.Load(repoID, "capsule-mutate") + if err != nil { + t.Fatal(err) + } + originalUpdated := loaded.UpdatedAt + + _ = runDoctorInDir(t, repoDir, stateDir) + + reloaded, err := store.Load(repoID, "capsule-mutate") + if err != nil { + t.Fatal(err) + } + if !reloaded.UpdatedAt.Equal(originalUpdated) { + t.Error("Doctor mutated capsule state (UpdatedAt changed)") + } +} + +func TestDoctorSigZeroBehaviorPreserved(t *testing.T) { + if !isProcessRunning(os.Getpid()) { + t.Skip("current process not reported as running (expected on Windows)") + } +} + +func TestDoctorMissingWorktreeReported(t *testing.T) { + repoDir := setupTestRepo(t, []string{"feat-wt"}) + stateDir := t.TempDir() + + repoID, err := git.RepoID(repoDir) + if err != nil { + t.Fatal(err) + } + + store := state.NewStore(stateDir) + _ = os.MkdirAll(filepath.Join(stateDir, "worktrees"), 0755) + setupCapsule(t, store, repoID, "capsule-no-wt", "feat-wt", filepath.Join(stateDir, "nonexistent-worktree"), "active") + + results := runDoctorInDir(t, repoDir, stateDir) + + found := false + for _, r := range results { + if strings.Contains(r.Message, "capsule-no-wt") && strings.Contains(r.Message, "missing its worktree") { + found = true + break + } + } + if !found { + t.Error("expected worktree missing warning") + } +} + +func TestDoctorMissingBranchOtherRepoNotReported(t *testing.T) { + repoA := setupTestRepo(t, []string{"feat-a"}) + repoB := setupTestRepo(t, nil) + stateDir := t.TempDir() + + repoAID, err := git.RepoID(repoA) + if err != nil { + t.Fatal(err) + } + repoBID, err := git.RepoID(repoB) + if err != nil { + t.Fatal(err) + } + + store := state.NewStore(stateDir) + _ = os.MkdirAll(filepath.Join(stateDir, "worktrees"), 0755) + setupCapsule(t, store, repoAID, "capsule-a", "feat-a", repoA, "active") + setupCapsule(t, store, repoBID, "capsule-b-missing", "branch-gone", repoB, "active") + + results := runDoctorInDir(t, repoA, stateDir) + + for _, r := range results { + if strings.Contains(r.Message, "capsule-b-missing") && strings.Contains(r.Message, "branch") { + t.Errorf("other-repo capsule with missing branch must not be reported: %s", r.Message) + } + } +} diff --git a/internal/git/repository.go b/internal/git/repository.go index 59ed197..e6fe781 100644 --- a/internal/git/repository.go +++ b/internal/git/repository.go @@ -3,6 +3,7 @@ package git import ( "crypto/sha256" "fmt" + "path/filepath" "strings" ) @@ -11,10 +12,13 @@ func Root() (string, error) { } func RepoID(root string) (string, error) { - remote, err := execGitInDir(root, "remote", "get-url", "origin") + // Normalize path separators so that C:\foo\bar and C:/foo/bar + // produce the same ID on Windows. + normalized := filepath.ToSlash(root) + + remote, err := execGitInDir(normalized, "remote", "get-url", "origin") if err != nil { - // Fallback: use repo root path hash - h := sha256.Sum256([]byte(root)) + h := sha256.Sum256([]byte(normalized)) return fmt.Sprintf("%x", h[:8]), nil }