diff --git a/internal/cli/app.go b/internal/cli/app.go index 80854beb4..61ed32b1c 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -81,6 +81,7 @@ type appDeps struct { runSandboxSetupHelper func(path string, args []string, stdout io.Writer, stderr io.Writer) error registerMCPTools func(context.Context, *tools.Registry, config.MCPConfig, mcp.RegisterOptions) (mcpToolRuntime, error) prepareWorktree func(context.Context, worktrees.Options) (worktrees.Result, error) + releaseWorktree func(context.Context, worktrees.Options, string) error detectVerifyPlan func(string) (verify.Plan, error) runVerify func(context.Context, verify.Plan, verify.RunOptions) verify.Report runSelfVerify func(context.Context, verify.Plan, selfverify.Options) selfverify.Report @@ -187,6 +188,7 @@ func defaultAppDeps() appDeps { return mcp.RegisterTools(ctx, registry, cfg, options) }, prepareWorktree: worktrees.Prepare, + releaseWorktree: worktrees.Release, detectVerifyPlan: verify.DetectPlan, runVerify: verify.Run, runSelfVerify: selfverify.Run, @@ -532,6 +534,9 @@ func fillAppDeps(deps appDeps) appDeps { if deps.prepareWorktree == nil { deps.prepareWorktree = defaults.prepareWorktree } + if deps.releaseWorktree == nil { + deps.releaseWorktree = defaults.releaseWorktree + } if deps.detectVerifyPlan == nil { deps.detectVerifyPlan = defaults.detectVerifyPlan } diff --git a/internal/cli/backends_test.go b/internal/cli/backends_test.go index 3a1f7fb4d..d86dfaf74 100644 --- a/internal/cli/backends_test.go +++ b/internal/cli/backends_test.go @@ -23,7 +23,7 @@ import ( func TestRunBackendsJSONUsesLifecycleSnapshotWithoutConnectingMCP(t *testing.T) { cwd := t.TempDir() - secret := "sk-proj-" + strings.Repeat("a", 24) + secret := "sk-proj-" + strings.Repeat("a", 23) + "0" deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { @@ -161,7 +161,7 @@ func TestRunBackendsTextAndHelp(t *testing.T) { func TestRunBackendsDoctorJSONAndTextWithoutConnectingMCP(t *testing.T) { cwd := t.TempDir() - secret := "sk-proj-" + strings.Repeat("b", 24) + secret := "sk-proj-" + strings.Repeat("b", 23) + "0" deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { diff --git a/internal/cli/completions.go b/internal/cli/completions.go index 3620ad2ac..b11fcfe4c 100644 --- a/internal/cli/completions.go +++ b/internal/cli/completions.go @@ -84,7 +84,7 @@ var completionRoot = completionNode{ }}, {names: []string{"update"}}, {names: []string{"upgrade"}}, - {names: []string{"worktrees", "worktree"}, children: leafNodes("prepare")}, + {names: []string{"worktrees", "worktree"}, children: leafNodes("prepare", "release")}, {names: []string{"verify"}}, {names: []string{"trust"}, children: leafNodes("list", "remove")}, {names: []string{"eval"}, children: leafNodes("validate", "run", "bench")}, diff --git a/internal/cli/completions_test.go b/internal/cli/completions_test.go index fc2acdec6..c129a6af5 100644 --- a/internal/cli/completions_test.go +++ b/internal/cli/completions_test.go @@ -155,6 +155,8 @@ func TestCompletionTreeCoversAliasesNestingAndCommonFlags(t *testing.T) { assertCandidates(t, byPath[""], "sessions", "session", "plugins", "plugin", "worktrees", "worktree", "--add-dir", "--theme", "-p", "--prompt") assertCandidates(t, byPath["exec"], "--model", "--cwd", "--worktree", "--output-format", "--resume", "--skip-permissions-unsafe") + assertCandidates(t, byPath["worktrees"], "prepare", "release") + assertCandidates(t, byPath["worktree"], "prepare", "release") assertCandidates(t, byPath["daemon"], "start", "stop", "status", "run", "attach") assertCandidates(t, byPath["mcp oauth"], "login", "logout", "status") assertCandidates(t, byPath["sandbox grants"], "list", "allow", "deny", "revoke", "clear") diff --git a/internal/cli/exec.go b/internal/cli/exec.go index 2d1fe542a..0a34b9a1f 100644 --- a/internal/cli/exec.go +++ b/internal/cli/exec.go @@ -199,11 +199,33 @@ func runExec(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in Name: options.worktreeName, BaseDir: options.worktreeDir, Now: deps.now, + // The worktree's lifetime is bound to this process (the deferred + // release below), so record the PID: if this process dies without + // releasing, Clean can expire the lease instead of skipping the + // locked worktree forever. + LeasePID: os.Getpid(), }) if err != nil { return writeExecFormatUsageError(stdout, stderr, options.outputFormat, err.Error()) } workspaceRoot = preparedWorktree.Path + // When this run's own Prepare call took the worktree lock, its + // lifetime is bound to this function: release the lock once it returns + // so Clean can reclaim the worktree later if it goes stale. A reused + // worktree whose lock an external `zero worktrees prepare` caller + // still holds reports LockAcquired=false; releasing it here would + // clear that caller's lease and expose its workspace to Clean, so the + // matching release stays that caller's responsibility. A failed unlock + // leaves a lock Clean will permanently skip, so it must not pass + // silently; the run's primary result has already been emitted by the + // time the defer runs, so surface it as a diagnostic. + if preparedWorktree.LockAcquired { + defer func() { + if releaseErr := deps.releaseWorktree(context.Background(), worktrees.Options{Cwd: trustRoot}, preparedWorktree.Path); releaseErr != nil { + fmt.Fprintf(stderr, "zero: failed to release worktree lock on %s: %s\n", redactCLIString(preparedWorktree.Path), redactCLIString(releaseErr.Error())) + } + }() + } } registry := newCoreRegistry(workspaceRoot) diff --git a/internal/cli/extensions_test.go b/internal/cli/extensions_test.go index 00af4dd9b..0593ab576 100644 --- a/internal/cli/extensions_test.go +++ b/internal/cli/extensions_test.go @@ -78,7 +78,7 @@ func TestRunPluginsListsJSONAndText(t *testing.T) { } func TestRunHooksListsRedactedJSONAndText(t *testing.T) { - secret := "sk-proj-" + strings.Repeat("a", 24) + secret := "sk-proj-" + strings.Repeat("a", 23) + "0" result := hooks.LoadResult{ Config: hooks.Config{ Enabled: true, diff --git a/internal/cli/hooks_manage_test.go b/internal/cli/hooks_manage_test.go index 2fb08489c..96ea14f7a 100644 --- a/internal/cli/hooks_manage_test.go +++ b/internal/cli/hooks_manage_test.go @@ -87,7 +87,7 @@ func TestRunHooksAddRejectsUnknownEvent(t *testing.T) { func TestRunHooksAddJSONRedactsSecretArgs(t *testing.T) { cwd := t.TempDir() - secret := "sk-proj-" + strings.Repeat("z", 24) + secret := "sk-proj-" + strings.Repeat("z", 23) + "0" var stdout, stderr bytes.Buffer code := runHooksAdd([]string{"h1", "--event", "beforeTool", "--command", "sh", "--arg", "-c", "--arg", "echo " + secret, "--json"}, &stdout, &stderr, hooksManageDeps(cwd)) if code != exitSuccess { diff --git a/internal/cli/mcp_commands_test.go b/internal/cli/mcp_commands_test.go index df2b0c228..b9155a33c 100644 --- a/internal/cli/mcp_commands_test.go +++ b/internal/cli/mcp_commands_test.go @@ -516,7 +516,7 @@ func TestRunMCPRemovePreservesUnrelatedConfigFields(t *testing.T) { func TestRunMCPListRedactsURLCredentialsAndSensitiveQueryParams(t *testing.T) { cwd := t.TempDir() serverURL := "https://user:password@remote.example/mcp?access_token=secret-token&api_key=secret-key&safe=value#access_token=fragment-secret" - commandSecret := "sk-proj-" + strings.Repeat("a", 24) + commandSecret := "sk-proj-" + strings.Repeat("a", 23) + "0" deps := appDeps{ getwd: func() (string, error) { return cwd, nil }, resolveMCPConfig: func(workspaceRoot string, _ bool) (config.MCPConfig, error) { diff --git a/internal/cli/workflow_test.go b/internal/cli/workflow_test.go index 75b844f47..f47052393 100644 --- a/internal/cli/workflow_test.go +++ b/internal/cli/workflow_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "os" "path/filepath" "strings" @@ -92,7 +93,7 @@ func TestRunWorktreesPrepareReportsErrors(t *testing.T) { } func TestRunWorktreesPrepareRedactsPathsInOutput(t *testing.T) { - secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz0" cwd := filepath.Join(t.TempDir(), secret, "repo") if err := os.MkdirAll(cwd, 0o700); err != nil { t.Fatal(err) @@ -153,6 +154,194 @@ func TestRunWorktreesPrepareRejectsDuplicateNames(t *testing.T) { } } +func TestRunWorktreesRelease(t *testing.T) { + worktreeDir := t.TempDir() + var releasedPath string + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"worktrees", "release", worktreeDir}, &stdout, &stderr, appDeps{ + releaseWorktree: func(ctx context.Context, options worktrees.Options, path string) error { + releasedPath = path + return nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + if releasedPath != worktreeDir { + t.Fatalf("released path = %q, want %q", releasedPath, worktreeDir) + } + if !strings.Contains(stdout.String(), worktreeDir) { + t.Fatalf("expected confirmation output to mention path, got %q", stdout.String()) + } +} + +func TestRunWorktreesReleaseNormalizesRelativePath(t *testing.T) { + // git worktree unlock matches against the path git recorded when the + // worktree was created, so a relative argument (resolved against + // whatever directory the caller happens to be running `zero` from) can + // fail to match. Chdir into a known directory and pass a relative + // argument to confirm it reaches releaseWorktree as an absolute path. + origWd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + parent := t.TempDir() + worktreeDir := filepath.Join(parent, "task-a") + if err := os.Mkdir(worktreeDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Chdir(parent); err != nil { + t.Fatal(err) + } + defer func() { + if err := os.Chdir(origWd); err != nil { + t.Fatal(err) + } + }() + + // filepath.Abs resolves against os.Getwd, whose spelling of the temp dir + // can differ from t.TempDir()'s (macOS reports /private/var for /var), so + // derive the expected value through the same resolution instead of + // joining onto the lexical parent path. + expectedPath, err := filepath.Abs("task-a") + if err != nil { + t.Fatal(err) + } + + var releasedPath string + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"worktrees", "release", "task-a"}, &stdout, &stderr, appDeps{ + releaseWorktree: func(ctx context.Context, options worktrees.Options, path string) error { + releasedPath = path + return nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + if releasedPath != expectedPath { + t.Fatalf("released path = %q, want absolute %q", releasedPath, expectedPath) + } +} + +func TestRunWorktreesReleaseRequiresPath(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"worktrees", "release"}, &stdout, &stderr, appDeps{ + releaseWorktree: func(context.Context, worktrees.Options, string) error { + t.Fatal("releaseWorktree should not be called without a path") + return nil + }, + }) + + if exitCode != exitUsage { + t.Fatalf("expected usage exit %d, got %d", exitUsage, exitCode) + } + if !strings.Contains(stderr.String(), "requires a worktree path") { + t.Fatalf("expected missing-path error, got %q", stderr.String()) + } +} + +func TestRunWorktreesReleaseWiresWorkspaceCwd(t *testing.T) { + // Release only consults Options.Cwd when the worktree directory itself is + // already gone (deleted by hand instead of released); git then has to run + // from the source repository to clear the orphaned lock. The CLI must + // wire the resolved workspace root through, or that advertised recovery + // path runs git from a possibly non-repository directory and the lock is + // never cleared (Clean skips locked entries). + root := t.TempDir() + var gotCwd string + + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"worktrees", "release", filepath.Join(root, "already-deleted")}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return root, nil }, + releaseWorktree: func(ctx context.Context, options worktrees.Options, path string) error { + gotCwd = options.Cwd + return nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + if gotCwd != root { + t.Fatalf("release Options.Cwd = %q, want workspace root %q", gotCwd, root) + } +} + +func TestRunWorktreesReleaseHonorsExplicitCwd(t *testing.T) { + // The deleted-path recovery cannot derive the source repository from the + // worktree path (the directory is gone and its name is a one-way hash), + // so -C names it explicitly; the resolved root must reach Release as + // Options.Cwd regardless of where the command was launched. + launchDir := t.TempDir() + repoDir := t.TempDir() + var gotCwd string + + var stdout, stderr bytes.Buffer + exitCode := runWithDeps([]string{"worktrees", "release", "-C", repoDir, filepath.Join(repoDir, "already-deleted")}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return launchDir, nil }, + releaseWorktree: func(ctx context.Context, options worktrees.Options, path string) error { + gotCwd = options.Cwd + return nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + if gotCwd != repoDir { + t.Fatalf("release Options.Cwd = %q, want explicit -C root %q", gotCwd, repoDir) + } +} + +func TestRunWorktreesReleaseReportsErrors(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"worktrees", "release", "/no/such/worktree"}, &stdout, &stderr, appDeps{ + releaseWorktree: func(context.Context, worktrees.Options, string) error { + return errors.New("unlock git worktree: not a valid worktree") + }, + }) + + if exitCode != exitUsage { + t.Fatalf("expected usage exit %d, got %d", exitUsage, exitCode) + } + if !strings.Contains(stderr.String(), "not a valid worktree") { + t.Fatalf("expected underlying error, got %q", stderr.String()) + } +} + +func TestRunWorktreesReleaseRedactsErrorText(t *testing.T) { + // Release errors interpolate the caller-supplied path (see + // verifyZeroOwnedWorktree's "refusing to release %s" messages); unlike the + // success path, which redacts before printing, the error was previously + // forwarded to stderr verbatim, so a rejected path containing a key-shaped + // segment would reach terminal/model-visible output unredacted. + secret := "sk-proj-abcDEF123_ghiJKL456-mnoPQR789stu" + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"worktrees", "release", "/tmp/" + secret + "/task"}, &stdout, &stderr, appDeps{ + releaseWorktree: func(context.Context, worktrees.Options, string) error { + return fmt.Errorf("refusing to release /tmp/%s/task: not a registered worktree of this repository", secret) + }, + }) + + if exitCode != exitUsage { + t.Fatalf("expected usage exit %d, got %d", exitUsage, exitCode) + } + if strings.Contains(stderr.String(), secret) { + t.Fatalf("release error leaked unredacted key-shaped path: %q", stderr.String()) + } + if !strings.Contains(stderr.String(), "[REDACTED]") { + t.Fatalf("expected redaction placeholder in error output, got %q", stderr.String()) + } +} + func TestRunVerifyTextAndJSON(t *testing.T) { cwd := t.TempDir() plan := verify.Plan{Root: cwd, Checks: []verify.Check{{ID: "go.test", Name: "Go tests", Command: []string{"go", "test", "./..."}}}} @@ -238,7 +427,7 @@ func TestRunVerifyTextAndJSON(t *testing.T) { } func TestRunVerifyRedactsWorkspacePathsInOutput(t *testing.T) { - secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz0" cwd := filepath.Join(t.TempDir(), secret, "workspace") if err := os.MkdirAll(cwd, 0o700); err != nil { t.Fatal(err) @@ -621,6 +810,157 @@ func TestRunExecWorktreeUsesPreparedWorkspace(t *testing.T) { } } +func TestRunExecWorktreeReleasesLockAfterRun(t *testing.T) { + root := t.TempDir() + worktreeDir := t.TempDir() + var releasedPath string + releaseCalls := 0 + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"exec", "--worktree", "task-a", "hello"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return root, nil }, + prepareWorktree: func(ctx context.Context, options worktrees.Options) (worktrees.Result, error) { + return worktrees.Result{Name: "task-a", Path: worktreeDir, RepoRoot: root, SourceBranch: "main", SourceCommit: "abc1234", LockAcquired: true}, nil + }, + releaseWorktree: func(ctx context.Context, options worktrees.Options, path string) error { + releaseCalls++ + releasedPath = path + return nil + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return execResolvedConfig(), nil + }, + newProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return echoExecProvider{}, nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + // A `zero exec --worktree` run is the only user of the worktree it prepares: + // its own process created it and is now exiting, so it must release the + // Prepare lock itself rather than leaving it locked forever (unlike `zero + // worktrees prepare`, which hands the path to a longer-lived external + // caller and has no such end-of-life signal to act on). + if releaseCalls != 1 { + t.Fatalf("releaseWorktree call count = %d, want 1", releaseCalls) + } + if releasedPath != worktreeDir { + t.Fatalf("released path = %q, want %q", releasedPath, worktreeDir) + } +} + +func TestRunExecWorktreeKeepsLockItDidNotAcquire(t *testing.T) { + // Defense in depth: Prepare now rejects an in-use lease outright, so a + // successful result always reports LockAcquired=true. Should that ever + // change, exec must still only release the ownership its own invocation + // established; releasing a lock it did not acquire would clear another + // caller's lease and let a later Clean force-delete a live workspace. + root := t.TempDir() + worktreeDir := t.TempDir() + releaseCalls := 0 + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"exec", "--worktree", "task-a", "hello"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return root, nil }, + prepareWorktree: func(ctx context.Context, options worktrees.Options) (worktrees.Result, error) { + return worktrees.Result{Name: "task-a", Path: worktreeDir, RepoRoot: root, SourceBranch: "main", SourceCommit: "abc1234", Reused: true, LockAcquired: false}, nil + }, + releaseWorktree: func(ctx context.Context, options worktrees.Options, path string) error { + releaseCalls++ + return nil + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return execResolvedConfig(), nil + }, + newProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return echoExecProvider{}, nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + if releaseCalls != 0 { + t.Fatalf("releaseWorktree call count = %d, want 0 for a lock this run did not acquire", releaseCalls) + } +} + +func TestRunExecWorktreeSurfacesReleaseFailure(t *testing.T) { + // A failed unlock leaves a lock Clean permanently skips, recreating the + // disk leak silently; the failure must reach the user with the affected + // path so the leaked lock can be cleared by hand. + root := t.TempDir() + worktreeDir := t.TempDir() + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"exec", "--worktree", "task-a", "hello"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return root, nil }, + prepareWorktree: func(ctx context.Context, options worktrees.Options) (worktrees.Result, error) { + return worktrees.Result{Name: "task-a", Path: worktreeDir, RepoRoot: root, SourceBranch: "main", SourceCommit: "abc1234", LockAcquired: true}, nil + }, + releaseWorktree: func(ctx context.Context, options worktrees.Options, path string) error { + return errors.New("unlock git worktree: boom") + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return execResolvedConfig(), nil + }, + newProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return echoExecProvider{}, nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + if !strings.Contains(stderr.String(), worktreeDir) || !strings.Contains(stderr.String(), "boom") { + t.Fatalf("expected release failure with path on stderr, got %q", stderr.String()) + } +} + +func TestRunExecWorktreeRedactsReleaseFailureText(t *testing.T) { + // The release path argument was already redacted before this diagnostic + // was added, but the error's own text was forwarded verbatim; Release's + // ownership errors interpolate the caller-supplied path (see + // verifyZeroOwnedWorktree), so a key-shaped path reaching this message + // leaked unredacted onto stderr. + root := t.TempDir() + worktreeDir := t.TempDir() + secret := "sk-proj-abcDEF123_ghiJKL456-mnoPQR789stu" + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"exec", "--worktree", "task-a", "hello"}, &stdout, &stderr, appDeps{ + getwd: func() (string, error) { return root, nil }, + prepareWorktree: func(ctx context.Context, options worktrees.Options) (worktrees.Result, error) { + return worktrees.Result{Name: "task-a", Path: worktreeDir, RepoRoot: root, SourceBranch: "main", SourceCommit: "abc1234", LockAcquired: true}, nil + }, + releaseWorktree: func(ctx context.Context, options worktrees.Options, path string) error { + return fmt.Errorf("refusing to release /tmp/%s/task: not a registered worktree of this repository", secret) + }, + resolveConfig: func(workspaceRoot string, overrides config.Overrides) (config.ResolvedConfig, error) { + return execResolvedConfig(), nil + }, + newProvider: func(config.ProviderProfile) (zeroruntime.Provider, error) { + return echoExecProvider{}, nil + }, + }) + + if exitCode != exitSuccess { + t.Fatalf("expected exit code %d, got %d: %s", exitSuccess, exitCode, stderr.String()) + } + if strings.Contains(stderr.String(), secret) { + t.Fatalf("deferred release diagnostic leaked unredacted key-shaped path: %q", stderr.String()) + } + if !strings.Contains(stderr.String(), "[REDACTED]") { + t.Fatalf("expected redaction placeholder in deferred release diagnostic, got %q", stderr.String()) + } +} + func TestRunExecRejectsForkWithWorktree(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer @@ -720,7 +1060,7 @@ func TestRunChangesCommitAuto(t *testing.T) { Root: cwd, Branch: "main", Files: []zerogit.FileChange{{Path: "README.md", Status: "modified"}}, - Diff: "some diff content with ghp_SECRETKEYHERE", + Diff: "some diff content with ghp_1234567890abcdefghijklmnopqrstuvwxyz", } mockProv := &mockCommitMsgProvider{ @@ -768,7 +1108,7 @@ func TestRunChangesCommitAuto(t *testing.T) { } // Verify that secret in the diff was redacted promptContent := mockProv.req.Messages[0].Content - if strings.Contains(promptContent, "ghp_SECRETKEYHERE") { + if strings.Contains(promptContent, "ghp_1234567890abcdefghijklmnopqrstuvwxyz") { t.Fatal("expected secret in diff to be redacted, but it was found in the prompt") } if !strings.Contains(promptContent, "[REDACTED]") && !strings.Contains(promptContent, "REDACTED") { diff --git a/internal/cli/workflows.go b/internal/cli/workflows.go index bc13bc7f8..854d71c3f 100644 --- a/internal/cli/workflows.go +++ b/internal/cli/workflows.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "path/filepath" "strconv" "strings" "time" @@ -63,6 +64,9 @@ func runWorktrees(args []string, stdout io.Writer, stderr io.Writer, deps appDep } return exitSuccess } + if command == "release" { + return runWorktreesRelease(args, stdout, stderr, deps) + } if command != "prepare" { return writeExecUsageError(stderr, fmt.Sprintf("unknown worktrees command %q", command)) } @@ -102,6 +106,85 @@ func runWorktrees(args []string, stdout io.Writer, stderr io.Writer, deps appDep return exitSuccess } +// runWorktreesRelease unlocks a worktree `zero worktrees prepare` created, so +// Zero's automatic Clean pass can reclaim it once it goes stale. Prepare locks +// every worktree it creates and nothing else in that command's own lifetime +// unlocks it (its process exits right after printing the path, long before +// whatever external caller actually finishes using the worktree), so that +// caller is responsible for running this once it is done. +func runWorktreesRelease(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { + var path string + var cwdFlag string + for index := 0; index < len(args); index++ { + arg := args[index] + switch { + case arg == "-h" || arg == "--help" || arg == "help": + if err := writeWorktreesHelp(stdout); err != nil { + return exitCrash + } + return exitSuccess + case arg == "-C" || arg == "--cwd": + value, next, err := nextFlagValue(args, index, arg) + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + cwdFlag = value + index = next + case strings.HasPrefix(arg, "--cwd="): + cwdFlag = strings.TrimSpace(strings.TrimPrefix(arg, "--cwd=")) + case strings.HasPrefix(arg, "-"): + return writeExecUsageError(stderr, fmt.Sprintf("unknown worktrees release flag %q", arg)) + default: + if path != "" { + return writeExecUsageError(stderr, "worktree path was provided more than once") + } + path = arg + } + } + path = strings.TrimSpace(path) + if path == "" { + return writeExecUsageError(stderr, "worktrees release requires a worktree path") + } + // git worktree unlock matches against the path git recorded when the + // worktree was created, so a relative argument here (resolved against + // whatever directory the caller happens to be running in, not + // necessarily the worktree's own) can fail to match. Resolve to absolute + // up front so the unlock target is unambiguous regardless of cwd. + absPath, err := filepath.Abs(path) + if err != nil { + return writeExecUsageError(stderr, redactCLIString(err.Error())) + } + // Release falls back to Options.Cwd as git's working directory when the + // worktree directory itself was deleted by hand instead of released; with + // no Cwd wired in, that recovery path runs `git worktree unlock` from + // wherever the caller happens to be, which outside the source repository + // leaves the orphaned lock behind forever (Clean skips locked entries). + // -C/--cwd names the source repository explicitly for exactly that case, + // since the deleted worktree path itself carries no way back to the repo; + // without the flag, resolve the launch directory best-effort (the + // ordinary existing-path case does not need it, so a resolution failure + // is not an error here). + releaseOptions := worktrees.Options{} + if cwdFlag != "" { + workspaceRoot, err := resolveWorkspaceRoot(cwdFlag, deps) + if err != nil { + return writeExecUsageError(stderr, redactCLIString(err.Error())) + } + releaseOptions.Cwd = workspaceRoot + } else if workspaceRoot, rootErr := resolveWorkspaceRoot("", deps); rootErr == nil { + releaseOptions.Cwd = workspaceRoot + } + if err := deps.releaseWorktree(context.Background(), releaseOptions, absPath); err != nil { + return writeExecUsageError(stderr, redactCLIString(err.Error())) + } + // Print the absolute path that was released so relative arguments still + // show an unambiguous target in the confirmation line. + if _, err := fmt.Fprintf(stdout, "released %s\n", redactCLIString(absPath)); err != nil { + return exitCrash + } + return exitSuccess +} + func runVerifyCommand(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { options, help, err := parseVerifyCommandArgs(args) if err != nil { @@ -786,15 +869,27 @@ func formatCommitResult(result zerogit.CommitResult) string { func writeWorktreesHelp(w io.Writer) error { _, err := fmt.Fprint(w, `Usage: zero worktrees prepare [flags] [name] + zero worktrees release [flags] Prepares an isolated git worktree for a Zero task. -Flags: +prepare locks the worktree it creates so Zero's automatic cleanup never +removes it out from under you. release unlocks a worktree prepare created, +once you are done with it, so cleanup can reclaim it later if it goes stale. +If the worktree directory was deleted by hand, run release with -C pointing +at the source repository so the orphaned lock can still be cleared. + +prepare flags: --name Worktree name; defaults to a timestamped task name --dir Base directory for Zero worktrees -C, --cwd Source repository directory --json Print JSON output -h, --help Show this help + +release flags: + -C, --cwd Source repository directory (required if the + worktree directory was already deleted) + -h, --help Show this help `) return err } diff --git a/internal/redaction/audit_fixes_test.go b/internal/redaction/audit_fixes_test.go index c4431904f..28f9fc0f9 100644 --- a/internal/redaction/audit_fixes_test.go +++ b/internal/redaction/audit_fixes_test.go @@ -61,6 +61,19 @@ func TestRedactString_CompoundKeyForms(t *testing.T) { } } +func TestRedactString_OverlappingExtraSecrets(t *testing.T) { + opts := Options{ + ExtraSecretValues: []string{"secret", "secret_token"}, + } + out := RedactString("my secret_token value", opts) + if strings.Contains(out, "_token") { + t.Fatalf("RedactString left partial fragment '_token': got %q", out) + } + if !strings.Contains(out, RedactedSecret) { + t.Fatalf("expected RedactedSecret in output: got %q", out) + } +} + func TestRedactString_AuthHeaderSchemes(t *testing.T) { o := Options{} // Opaque values (no token-format prefix) so only the header/colon logic can @@ -135,6 +148,117 @@ func TestRedactString_PreservesFileLineColons(t *testing.T) { } } +func TestRedactString_OpenAIKeyDigitFilterAndVendorPrefixes(t *testing.T) { + o := Options{} + // Vendor / fixture shapes that the enumerated-prefix approach missed. + for _, secret := range []string{ + "sk-test-secret1234567890", + "sk-fw-1SENTINEL_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "sk-live-1SENTINELaaaaaaaaaa_bbbbbbbbbb-cccc", + } { + out := RedactString("approved with "+secret, o) + if strings.Contains(out, secret) { + t.Errorf("RedactString leaked %q: %q", secret, out) + } + } + // Known OpenAI prefixes redact even with an alphabet-only body. + for _, secret := range []string{ + "sk-proj-abcdefghijklmnopqrstuvwxyz", + "sk-svcacct-abcdefghijklmnopqrstuvwx", + "sk-admin-abcdefghijklmnopqrstuvwxyz", + } { + out := RedactString("token="+secret, o) + if strings.Contains(out, secret) { + t.Errorf("alphabet-only known OpenAI form leaked %q: %q", secret, out) + } + } + // No-digit kebab phrase must survive (parity with secrets.Scan). + phrase := "sk-learn-machine-learning-model" + out := RedactString("testing "+phrase+" in text", o) + if !strings.Contains(out, phrase) { + t.Fatalf("digit filter over-redacted kebab phrase: %q", out) + } +} + +func TestRedactString_LooseJWTForms(t *testing.T) { + o := Options{} + for _, token := range []string{ + "eyJhbGciOiJIUzI1NiJ9.U0VOVElORUxwYXlsb2Fk.SENTINELsignature123", + "eyJhbGciOiJkaXIiLCJlbmMiOiJBMjU2R0NNIn0.SENTINELencryptedkey.SENTINELiv", + } { + out := RedactString("auth="+token, o) + if strings.Contains(out, "eyJhbGci") { + t.Errorf("RedactString leaked jwt material from %q: %q", token, out) + } + } +} + +// Patterns whose body class allows "-" must redact a secret that ends in "-" +// right before a delimiter: a trailing \b anchor would force the engine to +// drop that last character and leave the hyphen visible (parity with +// secrets.Scan). +func TestRedactString_TrailingHyphenWithoutTailLeak(t *testing.T) { + o := Options{} + cases := []string{ + "xoxb-1234567890-abcdefghi-", + "AIzaSyA1234567890abcdefghijklmnopqrstu-", + "sk-proj-abcDEF123_ghiJKL456-mnoPQR789st-", + "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.SflKxwRJSMeKKF2QT4fw-", + } + for _, secret := range cases { + in := "secret is " + secret + " end" + out := RedactString(in, o) + if strings.Contains(out, secret) { + t.Errorf("RedactString leaked trailing-hyphen secret %q: got %q", secret, out) + } + if strings.Contains(out, "- end") || strings.Contains(out, "-] end") { + t.Errorf("trailing hyphen leaked for %q: %q", secret, out) + } + want := "secret is " + RedactedSecret + " end" + if out != want { + t.Errorf("RedactString(%q) = %q, want %q", in, out, want) + } + } +} + +// A credential with more word characters appended right after its body must +// still have its real secret material redacted; the appended run itself is +// outside the body class and stays. A trailing \b would fail the whole match +// (parity with secrets.Scan). +func TestRedactString_CredentialWithAppendedSuffix(t *testing.T) { + o := Options{} + cases := []struct { + secret string + suffix string + }{ + {"AKIAIOSFODNN7EXAMPLE", "EXTRA"}, + {"ghp_1234567890abcdefghijklmnopqrstuvwxyz", "_suffix"}, + } + for _, tc := range cases { + in := "token=" + tc.secret + tc.suffix + " end" + out := RedactString(in, o) + if strings.Contains(out, tc.secret) { + t.Errorf("RedactString leaked credential %q: got %q", tc.secret, out) + } + // Bare "token" is a sensitive key, so the assignment/query path may + // redact the whole value including the suffix. Either outcome is fine + // as long as the credential itself is gone and a marker is present. + if !strings.Contains(out, RedactedSecret) { + t.Errorf("expected redaction marker for %q: got %q", in, out) + } + } + // Standalone (no sensitive-key assignment) so only textSecretPatterns run. + in := "aws key AKIAIOSFODNN7EXAMPLEEXTRA tail" + out := RedactString(in, o) + if strings.Contains(out, "AKIAIOSFODNN7EXAMPLE") { + t.Fatalf("appended-suffix AWS key leaked: %q", out) + } + want := "aws key " + RedactedSecret + "EXTRA tail" + if out != want { + t.Fatalf("RedactString(%q) = %q, want %q", in, out, want) + } +} + func TestRedactValue_CompoundKeys(t *testing.T) { o := Options{} in := map[string]any{ diff --git a/internal/redaction/redaction.go b/internal/redaction/redaction.go index 310d28af9..24685eca9 100644 --- a/internal/redaction/redaction.go +++ b/internal/redaction/redaction.go @@ -6,6 +6,7 @@ import ( "net/url" "reflect" "regexp" + "sort" "strings" "unicode" ) @@ -67,16 +68,32 @@ var sensitiveKeys = map[string]struct{}{ "zero_api_key": {}, } +// openaiKeyPattern mirrors secrets.Scan's broad sk- body. Known OpenAI +// prefixes (sk-proj-/sk-svcacct-/sk-admin-) are always redacted; other sk- +// digit-free matches with an interior hyphen are left alone (kebab-case false +// positives), while digit-free legacy sk- credentials are still redacted. +// Applied via ReplaceAllStringFunc rather than the plain list below. +var openaiKeyPattern = regexp.MustCompile(`\bsk-[A-Za-z0-9_-]{20,}`) + +// textSecretPatterns mirror secrets.Scan for end-boundary behavior and the +// shared high-confidence shapes. A leading \b keeps each pattern from firing +// mid-word; a trailing \b is omitted so a secret followed by more word +// characters outside its body class (e.g. AKIA…EXAMPLEEXTRA) still matches, +// and a secret that ends in "-" (allowed by some body classes) is fully +// redacted rather than leaving the hyphen behind. glpat is redaction-only +// (not in secrets.Scan); ASIA temporary access keys are kept alongside AKIA. +// openai keys are handled separately (digit filter). JWT has a strict form +// (both segments start with eyJ) and a looser three-segment form. var textSecretPatterns = []*regexp.Regexp{ - regexp.MustCompile(`\bsk-(?:proj-)?[A-Za-z0-9._-]{12,}\b`), - regexp.MustCompile(`\bsk-ant-api\d{2}-[A-Za-z0-9._-]{12,}\b`), - regexp.MustCompile(`\bgithub_pat_[A-Za-z0-9_]{12,}\b`), - regexp.MustCompile(`\bgh[pousr]_[A-Za-z0-9_]{12,}\b`), - regexp.MustCompile(`\bglpat-[A-Za-z0-9_-]{12,}\b`), - regexp.MustCompile(`\bAIza[0-9A-Za-z_-]{12,}\b`), - regexp.MustCompile(`\bxox[baprs]-[A-Za-z0-9-]{12,}\b`), - regexp.MustCompile(`\b(?:AKIA|ASIA)[A-Z0-9]{16}\b`), - regexp.MustCompile(`\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\b`), + regexp.MustCompile(`\bsk-ant-(?:api\d{2}-)?[A-Za-z0-9_-]{20,}`), + regexp.MustCompile(`\bgithub_pat_[A-Za-z0-9_]{22,}`), + regexp.MustCompile(`\bgh[pousr]_[A-Za-z0-9]{36,}`), + regexp.MustCompile(`\bglpat-[A-Za-z0-9_-]{12,}`), + regexp.MustCompile(`\bAIza[0-9A-Za-z\-_]{35,}`), + regexp.MustCompile(`\bxox[baprs]-[A-Za-z0-9-]{10,}`), + regexp.MustCompile(`\b(?:AKIA|ASIA)[A-Z0-9]{16}`), + regexp.MustCompile(`\beyJ[A-Za-z0-9_-]{10,}\.eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}`), + regexp.MustCompile(`\beyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}`), } var ( @@ -156,9 +173,15 @@ func keyLooksSensitive(normalized string) bool { func RedactString(value string, options Options) string { replacement := replacement(options) redacted := value - for _, secret := range options.ExtraSecretValues { - if strings.TrimSpace(secret) != "" { - redacted = strings.ReplaceAll(redacted, secret, replacement) + if len(options.ExtraSecretValues) > 0 { + secrets := append([]string{}, options.ExtraSecretValues...) + sort.SliceStable(secrets, func(i, j int) bool { + return len(secrets[i]) > len(secrets[j]) + }) + for _, secret := range secrets { + if strings.TrimSpace(secret) != "" { + redacted = strings.ReplaceAll(redacted, secret, replacement) + } } } @@ -201,12 +224,39 @@ func RedactString(value string, options Options) string { } return parts[1] + parts[2] + "=" + replacement }) + // openai keys first so the filter can drop kebab-case false positives + // before any other pattern rewrites nearby text. + redacted = openaiKeyPattern.ReplaceAllStringFunc(redacted, func(match string) string { + if !knownOpenAIKeyPrefix(match) && !secretMatchHasDigit(match) && + strings.Contains(strings.TrimPrefix(match, "sk-"), "-") { + return match + } + return replacement + }) for _, pattern := range textSecretPatterns { redacted = pattern.ReplaceAllString(redacted, replacement) } return redacted } +// knownOpenAIKeyPrefix is the redaction-side twin of secrets.knownOpenAIKeyPrefix: +// known OpenAI-issued forms redact even with an alphabet-only body. +func knownOpenAIKeyPrefix(match string) bool { + return strings.HasPrefix(match, "sk-proj-") || + strings.HasPrefix(match, "sk-svcacct-") || + strings.HasPrefix(match, "sk-admin-") +} + +// secretMatchHasDigit is the redaction-side twin of secrets.containsDigit. +func secretMatchHasDigit(s string) bool { + for _, r := range s { + if r >= '0' && r <= '9' { + return true + } + } + return false +} + func RedactValue(value any, options Options) any { return redactReflect(reflect.ValueOf(value), redactionContext{ options: options, diff --git a/internal/secrets/scanner.go b/internal/secrets/scanner.go index e3c612e9b..50886992d 100644 --- a/internal/secrets/scanner.go +++ b/internal/secrets/scanner.go @@ -30,19 +30,35 @@ type pattern struct { // mid-word — e.g. "sk-" inside "task-management-and-coordination" must NOT match // as an openai_key. Real secrets are preceded by a delimiter (space, quote, =, :, // start-of-string), all of which satisfy \b. +// +// A trailing \b is omitted on every pattern: \b only matches at a word/non-word +// transition, so a secret immediately followed by more word characters that +// fall outside its body class (an appended suffix like "...EXAMPLEEXTRA" or +// "..._suffix") has no such transition there, and a fixed or greedy quantifier +// cannot backtrack its way to one — the whole match fails and the credential +// leaks un-redacted. The body character class is itself the real stopping +// boundary once the input runs out of allowed characters, so the anchor is +// unnecessary; the leading \b already keeps these from firing mid-word. var patterns = []pattern{ {"aws_access_key_id", regexp.MustCompile(`\bAKIA[0-9A-Z]{16}`)}, - {"github_token", regexp.MustCompile(`\bgh[pousr]_[A-Za-z0-9]{36}`)}, + {"github_token", regexp.MustCompile(`\bgh[pousr]_[A-Za-z0-9]{36,}`)}, {"github_pat", regexp.MustCompile(`\bgithub_pat_[A-Za-z0-9_]{22,}`)}, {"slack_token", regexp.MustCompile(`\bxox[baprs]-[A-Za-z0-9-]{10,}`)}, - {"google_api_key", regexp.MustCompile(`\bAIza[0-9A-Za-z\-_]{35}`)}, - // Body allows - and _ so modern prefixed keys (sk-proj-…, sk-svcacct-…) match, - // not just the legacy sk- shape. + {"google_api_key", regexp.MustCompile(`\bAIza[0-9A-Za-z\-_]{35,}`)}, + {"anthropic_key", regexp.MustCompile(`\bsk-ant-(?:api\d{2}-)?[A-Za-z0-9_-]{20,}`)}, + // Broad body (allows - and _) so sk-proj-…, sk-or-v1-…, sk-fw-…, and + // legacy sk- all match. Scan drops digit-free matches only when + // the body contains an interior hyphen, which preserves kebab-case false + // positives without excluding digit-free legacy credentials. {"openai_key", regexp.MustCompile(`\bsk-[A-Za-z0-9_-]{20,}`)}, // Match the ENTIRE PEM/OpenSSH block (header THROUGH the END marker, body // included) so redaction removes the key material, not just the header. {"private_key_block", regexp.MustCompile(`(?s)-----BEGIN (?:[A-Z0-9]+ )*PRIVATE KEY-----.*?-----END (?:[A-Z0-9]+ )*PRIVATE KEY-----`)}, + // Strict JWS (both header and payload are JSON, so both start with eyJ) + // plus a looser three-segment form for non-JSON payloads and the first + // three segments of a compact JWE. {"jwt", regexp.MustCompile(`\beyJ[A-Za-z0-9_-]{10,}\.eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}`)}, + {"jwt", regexp.MustCompile(`\beyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}`)}, } // Scan returns the distinct secrets found in text (deduplicated by match, @@ -54,6 +70,13 @@ func Scan(text string) []Finding { seen := map[string]Finding{} for _, p := range patterns { for _, m := range p.re.FindAllString(text, -1) { + // Drop pure kebab FPs (sk-learn-…) unless the match is a known + // OpenAI-issued prefix form (sk-proj-/sk-svcacct-/sk-admin-), which + // we always treat as credentials even when the body has no digit. + if p.typ == "openai_key" && !knownOpenAIKeyPrefix(m) && !containsDigit(m) && + strings.Contains(strings.TrimPrefix(m, "sk-"), "-") { + continue + } if _, ok := seen[m]; !ok { seen[m] = Finding{Type: p.typ, Match: m} } @@ -75,6 +98,27 @@ func Scan(text string) []Finding { return out } +// containsDigit reports whether s has at least one ASCII digit. Together with +// an interior-hyphen check, it distinguishes kebab-case false positives from +// digit-free legacy sk- credentials. +func containsDigit(s string) bool { + for _, r := range s { + if r >= '0' && r <= '9' { + return true + } + } + return false +} + +// knownOpenAIKeyPrefix reports whether match is a known OpenAI-issued sk- +// form. Those prefixes are redacted even when the body has no digit; unknown +// sk- vendor forms still need a digit so kebab phrases stay un-redacted. +func knownOpenAIKeyPrefix(match string) bool { + return strings.HasPrefix(match, "sk-proj-") || + strings.HasPrefix(match, "sk-svcacct-") || + strings.HasPrefix(match, "sk-admin-") +} + // Redact replaces every detected secret in text with a typed placeholder and // returns the redacted text plus the findings. When nothing matches it returns // the input unchanged and a nil slice. diff --git a/internal/secrets/scanner_test.go b/internal/secrets/scanner_test.go index 6ad4521e9..c5a1c96f8 100644 --- a/internal/secrets/scanner_test.go +++ b/internal/secrets/scanner_test.go @@ -50,11 +50,11 @@ func TestRedactNestedSecretStillRemovesWholeBlock(t *testing.T) { // AWS key shape, which sorts before private_key_block). Redaction must remove // the WHOLE block: if the inner match were replaced first it would corrupt the // block's exact string and leave the BEGIN/END header in the output. - key := "-----BEGIN PRIVATE KEY-----\nAKIAABCDEFGHIJKLMNOP\nMIIEowIBAAKCAQEAbody\n-----END PRIVATE KEY-----" + key := "-----BEGIN PRIVATE KEY-----\nAKIAIOSFODNN7EXAMPLE\nMIIEowIBAAKCAQEAbody\n-----END PRIVATE KEY-----" text := "leaked:\n" + key + "\ndone" redacted, _ := Redact(text) - for _, leaked := range []string{"PRIVATE KEY", "AKIAABCDEFGHIJKLMNOP", "MIIEowIBAAKCAQEAbody"} { + for _, leaked := range []string{"PRIVATE KEY", "AKIAIOSFODNN7EXAMPLE", "MIIEowIBAAKCAQEAbody"} { if strings.Contains(redacted, leaked) { t.Fatalf("redaction leaked %q from a nested-secret block: %q", leaked, redacted) } @@ -102,11 +102,12 @@ func TestRedactNoMatchReturnsInputUnchanged(t *testing.T) { } func TestScanDetectsModernPrefixedOpenAIKeys(t *testing.T) { - // Modern keys carry sk-proj-/sk-svcacct- prefixes and use - and _ in the body; - // the legacy sk- pattern would have missed them. + // Modern keys carry sk-proj-/sk-svcacct-/sk-admin- prefixes and use - and _ in + // the body; the legacy sk- pattern would have missed them. for _, key := range []string{ "sk-proj-abcDEF123_ghiJKL456-mnoPQR789stu", "sk-svcacct-abcDEF123_ghiJKL456-mnoPQR789", + "sk-admin-abcDEF123_ghiJKL456-mnoPQR789stu", } { redacted, findings := Redact("token=" + key) if len(findings) != 1 || findings[0].Type != "openai_key" { @@ -120,3 +121,183 @@ func TestScanDetectsModernPrefixedOpenAIKeys(t *testing.T) { } } } + +func TestScanDetectsHyphenatedOpenAICompatibleKeys(t *testing.T) { + // Vendor-prefixed keys (OpenRouter sk-or-v1-, Fireworks sk-fw-, live keys) + // use hyphens beyond the enumerated prefixes. The broad sk-[A-Za-z0-9_-] + // pattern plus the digit filter must catch them without listing every vendor. + for _, key := range []string{ + "sk-or-v1-1234567890abcdef1234567890abcdef1234567890abcdef1234", + // Vendor prefixes not in any allow-list; digit filter still accepts them. + "sk-fw-1SENTINEL_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "sk-live-1SENTINELaaaaaaaaaa_bbbbbbbbbb-cccc", + "sk-test-secret1234567890", + } { + redacted, findings := Redact("token=" + key) + if len(findings) != 1 || findings[0].Type != "openai_key" { + t.Fatalf("expected one openai_key finding for %q, got %#v", key, findings) + } + if strings.Contains(redacted, key) { + t.Fatalf("key leaked after redaction: %q", redacted) + } + if !strings.Contains(redacted, "[REDACTED:openai_key]") { + t.Fatalf("missing typed placeholder for %q: %q", key, redacted) + } + } +} + +func TestScanDetectsLooseJWTForms(t *testing.T) { + // Strict form requires the second segment to start with eyJ (JSON payload). + // Compact JWS with a non-JSON payload and the first three segments of a + // compact JWE still need to redact. + cases := []string{ + "eyJhbGciOiJIUzI1NiJ9.U0VOVElORUxwYXlsb2Fk.SENTINELsignature123", + "eyJhbGciOiJkaXIiLCJlbmMiOiJBMjU2R0NNIn0.SENTINELencryptedkey.SENTINELiv.SENTINELciphertext.SENTINELtag12345", + } + for _, token := range cases { + redacted, findings := Redact("auth=" + token) + if len(findings) == 0 { + t.Fatalf("expected jwt finding for %q, got none", token) + } + if findings[0].Type != "jwt" { + t.Fatalf("expected jwt type for %q, got %#v", token, findings) + } + // At least the leading three segments (or the full three-part JWS) must go. + if strings.Contains(redacted, "eyJhbGci") { + t.Fatalf("jwt header leaked after redaction: %q", redacted) + } + } +} + +func TestScanDetectsAnthropicKeys(t *testing.T) { + for _, key := range []string{ + "sk-ant-api03-1234567890abcdefghijklmnopqrstuvwxyz-12345", + "sk-ant-1234567890abcdefghijklmnopqrstuvwxyz-12345", + } { + redacted, findings := Redact("token=" + key) + if len(findings) != 1 || findings[0].Type != "anthropic_key" { + t.Fatalf("expected one anthropic_key finding for %q, got %#v", key, findings) + } + if strings.Contains(redacted, key) { + t.Fatalf("key leaked after redaction: %q", redacted) + } + if !strings.Contains(redacted, "[REDACTED:anthropic_key]") { + t.Fatalf("missing typed placeholder for %q: %q", key, redacted) + } + } +} + +func TestScanRedactsLongerKeysWithoutTailLeak(t *testing.T) { + cases := []struct { + wantType string + secret string + }{ + {"github_token", "ghp_1234567890abcdefghijklmnopqrstuvwxyz1234567890abcdef"}, // 50 chars instead of 40 + {"google_api_key", "AIzaSyA1234567890abcdefghijklmnopqrstuv12345678"}, // 50 chars instead of 39 + } + for _, tc := range cases { + text := "longer key is " + tc.secret + " in text" + redacted, findings := Redact(text) + if len(findings) != 1 || findings[0].Type != tc.wantType { + t.Errorf("expected one %s finding, got %#v", tc.wantType, findings) + continue + } + wantRedacted := "longer key is [REDACTED:" + tc.wantType + "] in text" + if redacted != wantRedacted { + t.Errorf("redacted = %q, want %q", redacted, wantRedacted) + } + } +} + +// Patterns whose body class allows "-" (slack_token, google_api_key, the +// modern openai_key branch, jwt) must redact a secret that ends in "-" right +// before a delimiter: a trailing \b anchor would have no word/non-word +// transition to match there, forcing the engine to drop that last character +// from the match and leaking it. +func TestScanRedactsTrailingHyphenWithoutTailLeak(t *testing.T) { + cases := []struct { + wantType string + secret string + }{ + {"slack_token", "xoxb-1234567890-abcdefghi-"}, + {"google_api_key", "AIzaSyA1234567890abcdefghijklmnopqrstu-"}, + {"openai_key", "sk-proj-abcDEF123_ghiJKL456-mnoPQR789st-"}, + {"jwt", "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.SflKxwRJSMeKKF2QT4fw-"}, + } + for _, tc := range cases { + text := "secret is " + tc.secret + " end" + redacted, findings := Redact(text) + if len(findings) != 1 || findings[0].Type != tc.wantType { + t.Errorf("%s: expected one finding for %q, got %#v", tc.wantType, tc.secret, findings) + continue + } + if strings.Contains(redacted, "- end") || strings.Contains(redacted, "-] end") { + t.Errorf("%s: trailing hyphen leaked: %q", tc.wantType, redacted) + } + wantRedacted := "secret is [REDACTED:" + tc.wantType + "] end" + if redacted != wantRedacted { + t.Errorf("%s: redacted = %q, want %q", tc.wantType, redacted, wantRedacted) + } + } +} + +// A credential with more word characters appended right after its body (a +// copy-paste artifact, an env-var-style suffix) must still have its real +// secret material redacted, even though the appended run itself is outside +// the body class and so is left un-redacted. A trailing \b anchor would find +// no word/non-word transition there and, for a fixed or unbounded-greedy +// quantifier, fail the whole match instead of backtracking, leaking the +// credential entirely. +func TestScanRedactsCredentialWithAppendedSuffix(t *testing.T) { + cases := []struct { + wantType string + secret string + suffix string + }{ + {"aws_access_key_id", "AKIAIOSFODNN7EXAMPLE", "EXTRA"}, + {"github_token", "ghp_1234567890abcdefghijklmnopqrstuvwxyz", "_suffix"}, + } + for _, tc := range cases { + text := "token=" + tc.secret + tc.suffix + " end" + redacted, findings := Redact(text) + if len(findings) != 1 || findings[0].Type != tc.wantType { + t.Fatalf("%s: expected one finding for %q, got %#v", tc.wantType, tc.secret+tc.suffix, findings) + } + if findings[0].Match != tc.secret { + t.Fatalf("%s: matched %q, want the credential prefix %q", tc.wantType, findings[0].Match, tc.secret) + } + if strings.Contains(redacted, tc.secret) { + t.Fatalf("%s: credential leaked after redaction: %q", tc.wantType, redacted) + } + wantRedacted := "token=[REDACTED:" + tc.wantType + "]" + tc.suffix + " end" + if redacted != wantRedacted { + t.Fatalf("%s: redacted = %q, want %q", tc.wantType, redacted, wantRedacted) + } + } +} + +func TestScanIgnoresKebabCaseStartingWithSk(t *testing.T) { + phrase := "sk-learn-machine-learning-model" + findings := Scan("testing " + phrase + " in text") + if len(findings) != 0 { + t.Errorf("expected no match for non-secret kebab-case phrase %q, got: %#v", phrase, findings) + } +} + +func TestScanDetectsAlphabetOnlyKnownOpenAIPrefixes(t *testing.T) { + // Known OpenAI-issued prefixes redact even when the body has no digit. + // Unknown sk- forms still require a digit (see TestScanIgnoresKebabCase…). + for _, key := range []string{ + "sk-proj-abcdefghijklmnopqrstuvwxyz", + "sk-svcacct-abcdefghijklmnopqrstuvwx", + "sk-admin-abcdefghijklmnopqrstuvwxyz", + } { + redacted, findings := Redact("token=" + key) + if len(findings) != 1 || findings[0].Type != "openai_key" { + t.Fatalf("expected one openai_key for alphabet-only %q, got %#v", key, findings) + } + if strings.Contains(redacted, key) { + t.Fatalf("alphabet-only known OpenAI form leaked: %q", redacted) + } + } +} diff --git a/internal/selfverify/contracts_test.go b/internal/selfverify/contracts_test.go index c6f8237cd..c87ce12f8 100644 --- a/internal/selfverify/contracts_test.go +++ b/internal/selfverify/contracts_test.go @@ -9,7 +9,7 @@ import ( ) func TestSnapshotFromReportPreservesAttemptsAndRedactsRemediation(t *testing.T) { - secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz0" report := Report{ Root: "/repo/" + secret, StartedAt: "2026-06-06T11:00:00Z", diff --git a/internal/selfverify/selfverify_test.go b/internal/selfverify/selfverify_test.go index a53301220..323cbfac9 100644 --- a/internal/selfverify/selfverify_test.go +++ b/internal/selfverify/selfverify_test.go @@ -110,7 +110,7 @@ func TestRunDefaultsToOneAttempt(t *testing.T) { func TestRunStopsOnRemediatorErrorAndRedacts(t *testing.T) { root := t.TempDir() - secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz0" plan := verify.Plan{Root: root, Checks: []verify.Check{{ID: "go.test", Name: "Go tests", Command: []string{"go", "test", "./..."}}}} runner := &fakeVerifyRunner{results: []verify.CommandResult{{ExitCode: 1, Stdout: "FAIL\n"}}} diff --git a/internal/sessions/replay_test.go b/internal/sessions/replay_test.go index dc7ee4b1b..2a36196e3 100644 --- a/internal/sessions/replay_test.go +++ b/internal/sessions/replay_test.go @@ -131,7 +131,7 @@ func TestStorePlansCompactionWindow(t *testing.T) { } func TestStoreCompactionShapesSensitivePermissionEvents(t *testing.T) { - secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz0" store := NewStore(StoreOptions{RootDir: t.TempDir()}) session, err := store.Create(CreateInput{SessionID: "compactsafe"}) if err != nil { diff --git a/internal/tools/bash_secrets_test.go b/internal/tools/bash_secrets_test.go index b9491aa4d..b263acb4a 100644 --- a/internal/tools/bash_secrets_test.go +++ b/internal/tools/bash_secrets_test.go @@ -24,3 +24,14 @@ func TestFormatBashOutputLeavesCleanOutputAlone(t *testing.T) { t.Fatalf("clean output should not be altered, got %q", out) } } + +func TestFormatBashOutputRedactsAnthropicKey(t *testing.T) { + key := "sk-ant-api03-1234567890abcdefghijklmnopqrstuvwxyz-12345" + out := formatBashOutput("anthropic key: "+key+"\n", "", 0) + if strings.Contains(out, key) { + t.Fatalf("bash output leaked an Anthropic key: %q", out) + } + if !strings.Contains(out, "[REDACTED:anthropic_key]") { + t.Fatalf("expected typed redaction placeholder, got %q", out) + } +} diff --git a/internal/tui/command_output_test.go b/internal/tui/command_output_test.go index ba36235c0..018f11185 100644 --- a/internal/tui/command_output_test.go +++ b/internal/tui/command_output_test.go @@ -98,7 +98,7 @@ func TestFormatCommandCardRedactsTokenLikeText(t *testing.T) { {Key: "api key", Value: "sk-ant-api03-abcdefghijklmnopqrstuvwxyz"}, }, Lines: []string{ - "google token AIza1234567890abcdef", + "google token AIza1234567890abcdefghijklmnopqrstuvwxyz", }, Rows: []commandRow{ {Text: "shell used sk-proj-row-secret-value"}, @@ -111,7 +111,7 @@ func TestFormatCommandCardRedactsTokenLikeText(t *testing.T) { for _, secret := range []string{ "sk-proj-summary-secret-value", "sk-ant-api03-abcdefghijklmnopqrstuvwxyz", - "AIza1234567890abcdef", + "AIza1234567890abcdefghijklmnopqrstuvwxyz", "sk-proj-row-secret-value", "sk-proj-action-secret-value", } { @@ -187,7 +187,7 @@ func TestFormatCommandOutputRedactsTokenLikeText(t *testing.T) { Lines: []string{ "bash [allow] - sk-proj-sensitive-token-value approved shell", "anthropic: sk-ant-api03-abcdefghijklmnopqrstuvwxyz", - "google: AIza1234567890abcdef", + "google: AIza1234567890abcdefghijklmnopqrstuvwxyz", }, }}, }) @@ -195,7 +195,7 @@ func TestFormatCommandOutputRedactsTokenLikeText(t *testing.T) { for _, secret := range []string{ "sk-proj-sensitive-token-value", "sk-ant-api03-abcdefghijklmnopqrstuvwxyz", - "AIza1234567890abcdef", + "AIza1234567890abcdefghijklmnopqrstuvwxyz", } { if strings.Contains(got, secret) { t.Fatalf("expected token-like text to be redacted, got:\n%s", got) diff --git a/internal/tui/command_polish_test.go b/internal/tui/command_polish_test.go index 2758171fe..bfaf10def 100644 --- a/internal/tui/command_polish_test.go +++ b/internal/tui/command_polish_test.go @@ -253,7 +253,7 @@ func TestContextAndPermissionsCommandsRenderProductState(t *testing.T) { if _, err := store.Grant(sandbox.GrantInput{ ToolName: "bash", Decision: sandbox.GrantAllow, - Reason: "sk-proj-sensitive approved shell", + Reason: "sk-proj-sensitive-credential-value approved shell", }); err != nil { t.Fatalf("Grant returned error: %v", err) } @@ -312,7 +312,7 @@ func TestContextAndPermissionsCommandsRenderProductState(t *testing.T) { } { assertContains(t, permissionText, want) } - assertNotContains(t, permissionText, "sk-proj-sensitive") + assertNotContains(t, permissionText, "sk-proj-sensitive-credential-value") assertNotContains(t, permissionText, "status: ok") assertNotContains(t, permissionText, "Permission mode:") } diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index 2c4dbfb13..622e5caad 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -372,7 +372,7 @@ func TestPermissionsCommandListsPersistentSandboxGrants(t *testing.T) { if _, err := store.Grant(sandbox.GrantInput{ ToolName: "bash", Decision: sandbox.GrantAllow, - Reason: "sk-proj-sensitive trusted shell", + Reason: "sk-proj-sensitive-credential-value trusted shell", }); err != nil { t.Fatalf("Grant bash returned error: %v", err) } @@ -406,7 +406,7 @@ func TestPermissionsCommandListsPersistentSandboxGrants(t *testing.T) { } { assertContains(t, text, want) } - assertNotContains(t, text, "sk-proj-sensitive") + assertNotContains(t, text, "sk-proj-sensitive-credential-value") assertNotContains(t, text, "status: ok") assertNotContains(t, text, "Permission mode:") } diff --git a/internal/verify/contracts_test.go b/internal/verify/contracts_test.go index 9826b189b..6bd8d3a54 100644 --- a/internal/verify/contracts_test.go +++ b/internal/verify/contracts_test.go @@ -8,7 +8,7 @@ import ( ) func TestSnapshotFromReportRedactsLogsAndBuildsEvents(t *testing.T) { - secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz0" report := Report{ Root: "/workspace/" + secret, StartedAt: "2026-06-06T10:00:00Z", diff --git a/internal/verify/verify_test.go b/internal/verify/verify_test.go index 5a9291614..7db1b2e21 100644 --- a/internal/verify/verify_test.go +++ b/internal/verify/verify_test.go @@ -91,7 +91,7 @@ func TestRunParsesStructuredFailureSummary(t *testing.T) { ExitCode: 1, Stdout: strings.Join([]string{ "--- FAIL: TestSecret (0.00s)", - " secret_test.go:12: token sk-proj-abcdefghijklmnopqrstuvwxyz", + " secret_test.go:12: token sk-proj-abcdefghijklmnopqrstuvwxyz0", "FAIL", }, "\n"), }}} @@ -108,7 +108,7 @@ func TestRunParsesStructuredFailureSummary(t *testing.T) { if !strings.Contains(lines, "TestSecret") || !strings.Contains(lines, "[REDACTED]") { t.Fatalf("expected redacted failure summary lines, got %q", lines) } - if strings.Contains(lines, "sk-proj-abcdefghijklmnopqrstuvwxyz") { + if strings.Contains(lines, "sk-proj-abcdefghijklmnopqrstuvwxyz0") { t.Fatalf("failure summary leaked secret: %q", lines) } if report.Results[0].TestSummary == nil { diff --git a/internal/worktrees/worktrees.go b/internal/worktrees/worktrees.go index 1ab9eca81..b42d2ee7a 100644 --- a/internal/worktrees/worktrees.go +++ b/internal/worktrees/worktrees.go @@ -5,12 +5,15 @@ import ( "context" "crypto/sha1" "encoding/hex" + "errors" "fmt" + "io/fs" "os" "os/exec" "path/filepath" "regexp" "runtime" + "strconv" "strings" "time" ) @@ -30,6 +33,14 @@ type Options struct { Env map[string]string Now func() time.Time RunGit GitRunner + // LeasePID, when positive, records the owning process in the worktree + // lock reason. A lease carrying a PID is recoverable: if that process + // dies without releasing (SIGKILL, crash, power loss), Clean can expire + // the lease instead of skipping the locked worktree forever. Callers + // whose worktree outlives their process (zero worktrees prepare hands + // the path to an external owner) leave it zero for a persistent lock + // that only an explicit release clears. + LeasePID int } type Result struct { @@ -39,6 +50,10 @@ type Result struct { SourceBranch string `json:"sourceBranch,omitempty"` SourceCommit string `json:"sourceCommit,omitempty"` Reused bool `json:"reused"` + // LockAcquired reports whether this Prepare call took the worktree lock. + // It is false when a reused worktree was already locked by another live + // caller; releasing that lease is that caller's responsibility, not ours. + LockAcquired bool `json:"lockAcquired"` } var worktreeNamePattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]{0,80}$`) @@ -64,11 +79,32 @@ func Prepare(ctx context.Context, options Options) (Result, error) { return Result{}, err } + // Clean up stale worktrees (older than 24 hours) automatically to prevent + // disk space leaks. Only after the request itself validated: a rejected + // command (for example an invalid --name) must not have destructive + // cleanup side effects before reporting its error. + if options.RunGit == nil { + _ = Clean(ctx, options, 24*time.Hour) + } + repoRoot, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--show-toplevel") if err != nil { return Result{}, fmt.Errorf("not a git repository: %w", err) } repoRoot = filepath.Clean(repoRoot) + // Key the per-repository worktree bucket off the MAIN worktree's root, not + // repoRoot (which is whichever worktree - main or a linked one - this call + // happened to run from): git worktree list --porcelain always reports the + // main worktree first, regardless of invocation location, so this keeps + // Prepare and Release's ownership key derivation in agreement (see + // verifyZeroOwnedWorktree) even when Prepare runs from a linked worktree. + // Without it, a worktree prepared from a linked checkout hashes a + // different repoKey than Release computes, and its lease can never be + // cleared. + primaryRoot, err := primaryWorktreeRoot(ctx, runGit, repoRoot) + if err != nil { + return Result{}, err + } branch, _ := gitOutput(ctx, runGit, repoRoot, "rev-parse", "--abbrev-ref", "HEAD") commit, _ := gitOutput(ctx, runGit, repoRoot, "rev-parse", "--short", "HEAD") @@ -84,12 +120,12 @@ func Prepare(ctx context.Context, options Options) (Result, error) { return Result{}, fmt.Errorf("resolve worktree dir: %w", err) } - repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(primaryRoot)) target := filepath.Join(repoDir, name) result := Result{ Name: name, Path: target, - RepoRoot: repoRoot, + RepoRoot: primaryRoot, SourceBranch: branch, SourceCommit: commit, } @@ -106,6 +142,47 @@ func Prepare(ctx context.Context, options Options) (Result, error) { return Result{}, fmt.Errorf("worktree path already exists for a different git repository: %s", target) } result.Reused = true + // A reused worktree may have been released by a prior run's exit, and + // an unlocked target is exposed to Clean's staleness heuristic while + // this caller is still using it, so re-establish the lease here. A + // lock already held by a live owner means another run is still using + // the path: two live runs must not share one supposedly isolated + // checkout, so reject it. A Zero lease whose recorded PID is dead + // (SIGKILL, crash) is reclaimable the same way Clean recovers it — + // otherwise a crashed `zero exec --worktree --worktree-name X` bricks + // that name until someone runs release by hand. + acquired, err := lockWorktree(ctx, runGit, repoRoot, target, options.LeasePID) + if err != nil { + return Result{}, err + } + if !acquired { + reclaimed, reclaimErr := reclaimDeadOwnerLease(ctx, runGit, repoRoot, target) + if reclaimErr != nil { + return Result{}, reclaimErr + } + if reclaimed { + acquired, err = lockWorktree(ctx, runGit, repoRoot, target, options.LeasePID) + if err != nil { + return Result{}, err + } + } + } + if !acquired { + return Result{}, fmt.Errorf("worktree %s is locked by another active run; release it with `zero worktrees release %s` if that run is finished, or use a different --name", target, target) + } + result.LockAcquired = true + // Touch the worktree directory so Clean's mtime-based staleness check + // doesn't prune a worktree that's actively in use by the current process. + // Long-running tasks that don't write new files (e.g. waiting on a model) + // would otherwise accumulate stale mtimes and be wrongly force-removed. + if err := os.Chtimes(target, time.Now(), time.Now()); err != nil { + _, unlockErr := gitOutput(ctx, runGit, repoRoot, "worktree", "unlock", target) + return Result{}, errors.Join(err, unlockErr) + } + if err := writeOwnershipMarker(ctx, runGit, target); err != nil { + _, unlockErr := gitOutput(ctx, runGit, repoRoot, "worktree", "unlock", target) + return Result{}, errors.Join(err, unlockErr) + } return result, nil } if err := os.MkdirAll(repoDir, 0o700); err != nil { @@ -122,9 +199,388 @@ func Prepare(ctx context.Context, options Options) (Result, error) { } return Result{}, fmt.Errorf("create git worktree: %s", message) } + // Lock the worktree so Clean's mtime+dirty staleness heuristic (which + // cannot tell "abandoned" apart from "clean but waiting on a model, + // network, or user for a long time") never force-removes a worktree zero + // itself is still using. entry.locked already makes Clean skip a worktree + // a human locked by hand; locking here extends that same protection to + // the worktrees zero creates. An "already locked" answer on a worktree + // this call just created means another process raced us to it: treat that + // exactly like the reuse collision above rather than claiming a lease + // this call never acquired. + acquired, err := lockWorktree(ctx, runGit, repoRoot, target, options.LeasePID) + if err != nil { + // This call created target and nothing else can own it yet (a + // concurrent racer would have made lockWorktree return acquired=false, + // not an error), so a failure here (not "already locked", an actual + // git failure) must not leave an unleased, newly created worktree + // behind: Clean cannot distinguish it from live-but-not-yet-touched + // work, so it would sit as a disk leak until the staleness window + // passes. Best-effort: report a removal failure alongside the + // original lock error rather than letting it mask the leak. + // gitOutput (not a raw runGit call) so a nonzero exit code is treated + // as a failure: defaultRunGit returns a nil error alongside a nonzero + // CommandResult.ExitCode for a failed git invocation. + if _, removeErr := gitOutput(ctx, runGit, repoRoot, "worktree", "remove", "--force", target); removeErr != nil { + return Result{}, errors.Join(err, fmt.Errorf("clean up worktree after failed lock: %w", removeErr)) + } + return Result{}, err + } + if !acquired { + return Result{}, fmt.Errorf("worktree %s is locked by another active run; release it with `zero worktrees release %s` if that run is finished, or use a different --name", target, target) + } + result.LockAcquired = true + if err := writeOwnershipMarker(ctx, runGit, target); err != nil { + _, unlockErr := gitOutput(ctx, runGit, repoRoot, "worktree", "unlock", target) + _, removeErr := gitOutput(ctx, runGit, repoRoot, "worktree", "remove", "--force", target) + return Result{}, errors.Join(err, unlockErr, removeErr) + } return result, nil } +// leaseReasonPrefix marks a lock Zero itself created (vs a human `git +// worktree lock`); a "(pid N)" suffix makes the lease recoverable by Clean +// once the owning process is gone. +const leaseReasonPrefix = "zero: active task worktree" + +// leaseReason renders the lock reason for a Zero lease, embedding the owning +// PID when the caller's worktree lifetime is bound to its process. +func leaseReason(pid int) string { + if pid > 0 { + return fmt.Sprintf("%s (pid %d)", leaseReasonPrefix, pid) + } + return leaseReasonPrefix +} + +// leasePID extracts the owning PID from a Zero lease reason. ok=false for +// human locks and PID-less Zero leases (external prepare callers), which +// only an explicit release may clear. +func leasePID(reason string) (int, bool) { + rest, found := strings.CutPrefix(strings.TrimSpace(reason), leaseReasonPrefix+" (pid ") + if !found { + return 0, false + } + digits, found := strings.CutSuffix(rest, ")") + if !found { + return 0, false + } + pid, err := strconv.Atoi(digits) + if err != nil || pid <= 0 { + return 0, false + } + return pid, true +} + +// processAlive reports whether pid is a live process. It fails closed: any +// ambiguity (permission denied, platform limits) counts as alive, so an +// uncertain answer can only keep a lease, never expire one. Detection is +// platform-specific (see osProcessAlive in worktrees_posix.go / +// worktrees_windows.go): os.FindProcess alone cannot tell a dead PID from a +// live one on Windows, since OpenProcess can still succeed for a process that +// has already exited but whose handle has not been fully released. +func processAlive(pid int) bool { + if pid <= 0 { + return true + } + return osProcessAlive(pid) +} + +// lockWorktree takes the Clean-protection lease on target via `git worktree +// lock`. It reports whether this call acquired the lease: a lock already held +// by someone else is left in place and reported as not acquired, so the +// caller knows the matching Release belongs to the lease's original owner. +func lockWorktree(ctx context.Context, runGit GitRunner, repoRoot string, target string, pid int) (bool, error) { + lockResult, err := runGit(ctx, repoRoot, "worktree", "lock", "--reason", leaseReason(pid), target) + if err != nil { + return false, fmt.Errorf("lock git worktree: %w", err) + } + if lockResult.ExitCode == 0 { + return true, nil + } + message := strings.TrimSpace(firstNonEmpty(lockResult.Stderr, lockResult.Stdout)) + if strings.Contains(message, "already locked") { + return false, nil + } + if message == "" { + message = fmt.Sprintf("git worktree lock exited with code %d", lockResult.ExitCode) + } + return false, fmt.Errorf("lock git worktree: %s", message) +} + +// reclaimDeadOwnerLease unlocks target when its porcelain entry is a Zero +// lease whose recorded PID is provably dead. Live owners, human locks, and +// PID-less Zero leases are left alone. Returns true only when an unlock ran +// successfully so the caller can retry lockWorktree. +func reclaimDeadOwnerLease(ctx context.Context, runGit GitRunner, repoRoot string, target string) (bool, error) { + output, err := gitOutput(ctx, runGit, repoRoot, "worktree", "list", "--porcelain") + if err != nil { + return false, fmt.Errorf("list git worktrees for lease reclaim: %w", err) + } + want := canonicalizePath(target) + for _, entry := range parseWorktreeList(output) { + if canonicalizePath(entry.path) != want { + continue + } + if !entry.locked { + return false, nil + } + pid, ok := leasePID(entry.lockReason) + if !ok || processAlive(pid) { + return false, nil + } + if _, err := gitOutput(ctx, runGit, repoRoot, "worktree", "unlock", entry.path); err != nil { + return false, fmt.Errorf("reclaim dead-owner lease on %s: %w", entry.path, err) + } + return true, nil + } + return false, nil +} + +// Release unlocks a worktree that Prepare locked, via `git worktree unlock`, +// making it eligible for Clean's staleness check again. Zero itself only +// knows a worktree's use is over when its own process created it and is now +// exiting (zero exec --worktree calls this via defer); zero worktrees +// prepare hands the path to an external caller with no defined end-of-life, +// so that caller must run `zero worktrees release ` itself once done. +// Until it does, the worktree stays locked and Clean will never touch it, +// which is the safe default over silently guessing at liveness from mtimes. +func Release(ctx context.Context, options Options, path string) error { + runGit := options.RunGit + if runGit == nil { + runGit = defaultRunGit + } + // git needs a working directory that still exists to run in. path itself + // normally works (it's the worktree being unlocked), but if a caller + // manually deleted a locked worktree directory instead of releasing it + // first, path is gone; fall back to options.Cwd (the main repo) so the + // leaked, now-orphaned lock can still be unlocked and later pruned. + dir := path + if _, err := os.Stat(path); os.IsNotExist(err) { + if cwd, cwdErr := resolveCwd(options.Cwd); cwdErr == nil { + dir = cwd + } + } + matchedPath, err := verifyZeroOwnedWorktree(ctx, runGit, dir, path) + if err != nil { + if errors.Is(err, errAlreadyUnlocked) { + return nil + } + return err + } + if _, err := gitOutput(ctx, runGit, dir, "worktree", "unlock", matchedPath); err != nil { + return fmt.Errorf("unlock git worktree: %w", err) + } + return nil +} + +// zeroOwnerMarkerFile is the name of the ownership marker Prepare writes into +// a worktree's own private git admin directory (`git rev-parse +// --absolute-git-dir`), never into the working tree itself: the working tree +// is what `git status` inspects for staleness/dirtiness, and a marker living +// there would either show up as untracked noise (defeating Clean's dirty +// check) or need a .gitignore entry this package has no business adding to a +// caller's repository. The admin directory survives even after the worktree +// directory itself is deleted by hand (git only forgets it on `worktree +// prune`), and is not something `git worktree add` populates on its own, so +// its presence is what actually proves Zero's own Prepare created a given +// worktree - unlike the public zero-worktree- path convention and +// the leaseReasonPrefix lock-reason string, both of which a user can +// reproduce by hand for a worktree of the same repository. +const zeroOwnerMarkerFile = "zero-owner" + +// zeroOwnerMarkerContent is the marker's fixed body. Its value carries no +// meaning beyond "Prepare wrote this"; the file's mere presence at the +// expected location is the signal Release and Clean check. +const zeroOwnerMarkerContent = "zero: this worktree was created by `zero worktrees prepare`\n" + +// writeOwnershipMarker persists the ownership marker for target, a worktree +// path that must still exist on disk (Prepare calls this immediately after +// creating or re-locking it). Overwriting an existing marker is harmless and +// lets a worktree Prepare created before this marker existed self-heal the +// next time Prepare reuses it. +func writeOwnershipMarker(ctx context.Context, runGit GitRunner, target string) error { + gitDir, err := gitOutput(ctx, runGit, target, "rev-parse", "--absolute-git-dir") + if err != nil { + return fmt.Errorf("resolve worktree git dir: %w", err) + } + if err := os.WriteFile(filepath.Join(gitDir, zeroOwnerMarkerFile), []byte(zeroOwnerMarkerContent), 0o600); err != nil { + return fmt.Errorf("write worktree ownership marker: %w", err) + } + return nil +} + +// hasOwnershipMarker reports whether target's own git admin directory carries +// the marker writeOwnershipMarker persists. It fails closed: any error other +// than the marker simply not existing is returned rather than treated as +// "not owned so it's fine to skip," since callers otherwise use false to mean +// "safe to leave alone." +func hasOwnershipMarker(ctx context.Context, runGit GitRunner, target string) (bool, error) { + gitDir, err := gitOutput(ctx, runGit, target, "rev-parse", "--absolute-git-dir") + if err != nil { + return false, fmt.Errorf("resolve worktree git dir: %w", err) + } + content, err := os.ReadFile(filepath.Join(gitDir, zeroOwnerMarkerFile)) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("read worktree ownership marker: %w", err) + } + return string(content) == zeroOwnerMarkerContent, nil +} + +// isLegacyZeroWorktree verifies whether a worktree lacking zero-owner was +// created by a pre-upgrade version of Zero for this repository. +func isLegacyZeroWorktree(ctx context.Context, runGit GitRunner, target string, repoDir string, entry worktreeEntry) bool { + if !isUnderDir(canonicalizePath(target), repoDir) { + return false + } + if entry.locked { + if !strings.HasPrefix(strings.TrimSpace(entry.lockReason), leaseReasonPrefix) { + return false + } + } + gitDir, err := gitOutput(ctx, runGit, target, "rev-parse", "--absolute-git-dir") + if err != nil || strings.TrimSpace(gitDir) == "" { + return false + } + return true +} + +// verifyZeroOwnedWorktree confirms path has a zero-worktree- ancestor +// directory component, is a registered worktree of the repository, and (when +// locked) carries a lock reason Zero itself set, so Release cannot be used to +// clear the lock on a worktree a user (or another tool) manages by hand: the +// command is documented as releasing a worktree `prepare` created, not an +// arbitrary git worktree lock. The ancestor-component check stands in for +// reconstructing and comparing a full repoDir, because Release has no +// reliable way to know which --dir a long-gone Prepare call used (the CLI +// never threads BaseDir through to Release, and a custom --dir is not +// recorded anywhere the lock/unlock path can read back); the repoKey +// component is Prepare's actual ownership signature regardless of which +// directory it was created under. The repository root for that key, and the +// lock reason for the ownership check, both come from the same `git worktree +// list --porcelain` call, which works whether dir is the worktree itself or +// the main repository (its first entry is always the main working tree, from +// any worktree, regardless of git-dir layout), so this needs no branching on +// which of Release's two cwd cases is in play. +func verifyZeroOwnedWorktree(ctx context.Context, runGit GitRunner, dir string, path string) (string, error) { + output, err := gitOutput(ctx, runGit, dir, "worktree", "list", "--porcelain") + if err != nil { + return "", fmt.Errorf("resolve repository for %s: %w", path, err) + } + entries := parseWorktreeList(output) + if len(entries) == 0 { + return "", fmt.Errorf("resolve repository for %s: git worktree list returned no entries", path) + } + // entries[0].path is always the main worktree (see primaryWorktreeRoot), + // which is what Prepare now also keys its repoKey off, so this and Prepare + // agree regardless of which worktree either call ran from. + want := "zero-worktree-" + repoKey(entries[0].path) + + target := canonicalizePath(path) + + hasZeroComponent := false + for _, component := range strings.Split(target, string(filepath.Separator)) { + if component == want { + hasZeroComponent = true + break + } + } + if !hasZeroComponent { + return "", fmt.Errorf("refusing to release %s: not a zero-managed worktree (expected an ancestor directory named %q)", path, want) + } + + // Require a registered porcelain entry before unlocking. Matching only + // the public zero-worktree- path component would let Release + // clear a manual lock on any same-named path under that directory. + // Path comparison uses canonicalizePath so a lexical user argument + // (macOS /var vs /private/var, symlink --worktree-dir) still matches + // the physical spelling git worktree list reports. + var matchedPath string + for _, entry := range entries { + if canonicalizePath(entry.path) != target { + continue + } + matchedPath = entry.path + if !entry.locked { + // Already unlocked: nothing to release. Treat as success so a + // double release is a no-op rather than a git "not locked" error. + return "", errAlreadyUnlocked + } + if !strings.HasPrefix(entry.lockReason, leaseReasonPrefix) { + return "", fmt.Errorf("refusing to release %s: locked with reason %q, not a zero lease", path, entry.lockReason) + } + if info, statErr := os.Stat(entry.path); statErr == nil && info.IsDir() { + owned, err := hasOwnershipMarker(ctx, runGit, entry.path) + if err != nil { + return "", fmt.Errorf("verify worktree ownership for %s: %w", path, err) + } + if !owned { + return "", fmt.Errorf("refusing to release %s: missing zero ownership marker (not created by `zero worktrees prepare`)", path) + } + } + break + } + if matchedPath == "" { + return "", fmt.Errorf("refusing to release %s: not a registered worktree of this repository", path) + } + return matchedPath, nil +} + +// errAlreadyUnlocked is a sentinel for a Zero-managed path whose git lock is +// already clear; Release returns nil without calling unlock. +var errAlreadyUnlocked = errors.New("worktree already unlocked") + +// canonicalizePath returns the physical, cleaned form of path when it can be +// resolved (EvalSymlinks), otherwise Abs+Clean. Used so comparisons against +// `git worktree list --porcelain` (which reports physical paths) succeed for +// lexical or symlinked user spellings of the same location. +func canonicalizePath(path string) string { + if resolved, err := filepath.EvalSymlinks(path); err == nil { + return filepath.Clean(resolved) + } + abs, err := filepath.Abs(path) + if err != nil { + return filepath.Clean(path) + } + abs = filepath.Clean(abs) + // path itself does not exist (the documented release -C recovery: the + // worktree directory was deleted by hand before releasing it), so + // EvalSymlinks above had nothing to resolve. If an ANCESTOR of path is + // itself a symlink (a symlinked --worktree-dir), the plain Abs+Clean + // fallback keeps that ancestor's logical spelling, which never matches + // git's physical-path porcelain entry, and the lock can never be + // released. Resolve symlinks through the nearest existing ancestor + // instead and reattach the missing tail. + return resolveThroughNearestExistingAncestor(abs) +} + +// resolveThroughNearestExistingAncestor walks up from path until it finds an +// existing ancestor directory, resolves that ancestor's symlinks, and +// reattaches path's missing remainder onto the resolved ancestor. +func resolveThroughNearestExistingAncestor(path string) string { + var missing []string + current := path + for { + if resolved, err := filepath.EvalSymlinks(current); err == nil { + result := filepath.Clean(resolved) + for i := len(missing) - 1; i >= 0; i-- { + result = filepath.Join(result, missing[i]) + } + return result + } + parent := filepath.Dir(current) + if parent == current { + // Reached the filesystem root without finding an existing + // ancestor; nothing left to resolve symlinks through. + return path + } + missing = append(missing, filepath.Base(current)) + current = parent + } +} + func DefaultBaseDir(env map[string]string) (string, error) { if runtime.GOOS == "windows" { if localAppData := strings.TrimSpace(envValue(env, "LOCALAPPDATA")); localAppData != "" { @@ -214,6 +670,24 @@ func gitOutput(ctx context.Context, runGit GitRunner, dir string, args ...string return strings.TrimSpace(result.Stdout), nil } +// primaryWorktreeRoot returns the repository's main worktree path. `git +// worktree list --porcelain` always reports the main worktree first (its +// first entry), regardless of which linked worktree the command runs from - +// which is what lets Prepare and verifyZeroOwnedWorktree agree on one +// repoKey for the same repository even when invoked from different +// worktrees of it. +func primaryWorktreeRoot(ctx context.Context, runGit GitRunner, dir string) (string, error) { + output, err := gitOutput(ctx, runGit, dir, "worktree", "list", "--porcelain") + if err != nil { + return "", fmt.Errorf("resolve repository root: %w", err) + } + entries := parseWorktreeList(output) + if len(entries) == 0 { + return "", fmt.Errorf("resolve repository root: git worktree list returned no entries") + } + return entries[0].path, nil +} + func sameGitCommonDir(ctx context.Context, runGit GitRunner, sourceDir string, targetDir string) (bool, error) { sourceCommonDir, err := gitCommonDir(ctx, runGit, sourceDir) if err != nil { @@ -298,3 +772,336 @@ func firstNonEmpty(values ...string) string { } return "" } + +// Clean prunes any zero-owned git worktrees older than maxAge to prevent disk space leaks. +func Clean(ctx context.Context, options Options, maxAge time.Duration) error { + cwd, err := resolveCwd(options.Cwd) + if err != nil { + return err + } + runGit := options.RunGit + if runGit == nil { + runGit = defaultRunGit + } + repoRoot, err := gitOutput(ctx, runGit, cwd, "rev-parse", "--show-toplevel") + if err != nil { + return fmt.Errorf("not a git repository: %w", err) + } + repoRoot = filepath.Clean(repoRoot) + + baseDir := strings.TrimSpace(options.BaseDir) + if baseDir == "" { + baseDir, err = DefaultBaseDir(options.Env) + if err != nil { + return err + } + } + baseDir, err = filepath.Abs(baseDir) + if err != nil { + return err + } + // git worktree list --porcelain reports each worktree's PHYSICAL location, + // resolving any symlink component (for example a --worktree-dir that is + // itself a symlink, or a symlinked ancestor). Comparing entry paths against + // a merely-absolute (but not symlink-resolved) baseDir would then reject + // every worktree Prepare actually created under a symlinked base, leaving + // them permanently unprunable. EvalSymlinks failing (most commonly because + // the directory does not exist yet) just means there is nothing under it + // to prune, so falling back to the plain absolute path is safe. + if resolved, err := filepath.EvalSymlinks(baseDir); err == nil { + baseDir = resolved + } + + output, err := gitOutput(ctx, runGit, repoRoot, "worktree", "list", "--porcelain") + if err != nil { + return fmt.Errorf("list git worktrees: %w", err) + } + entries := parseWorktreeList(output) + if len(entries) == 0 { + return fmt.Errorf("list git worktrees: git worktree list returned no entries") + } + // Prepare only ever creates worktrees under this per-repository subtree + // (mirroring the repoDir it computes). Scoping pruning to baseDir itself + // would authorize deleting a worktree a user manages by hand in the same + // directory, which Zero never created and has no business force-removing. + // + // The bucket must be keyed off the MAIN worktree's root + // (entries[0].path, per primaryWorktreeRoot), not repoRoot: repoRoot is + // --show-toplevel for whichever worktree Clean itself runs from, which is + // a linked checkout's own root when Clean (or the Prepare call that + // auto-invokes it) runs there. Prepare keys repoDir off the same + // entries[0].path, so using repoRoot here instead would compute a + // different repoDir than Prepare's and filter out every actual + // zero-owned worktree for this repository whenever either call runs from + // a linked worktree. + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(entries[0].path)) + + cutoff := time.Now().Add(-maxAge) + var lastErr error + for _, entry := range entries { + // Compare against the physical spelling of the entry path. Git's + // porcelain listing is normally already physical, but test fixtures + // and some symlink layouts can leave a logical spelling that would + // otherwise fail the containment check against the resolved repoDir. + // Git commands still receive entry.path (the registered spelling). + entryPath := canonicalizePath(entry.path) + // Only prune worktrees zero created for this repository (i.e. inside + // repoDir), using a path-boundary-safe comparison so a sibling + // directory that merely shares repoDir as a string prefix (e.g. + // "-other") can't match. + if !isUnderDir(entryPath, repoDir) { + continue + } + + // A locked worktree is never a prune candidate - with one recovery + // carve-out: a Zero lease whose recorded owner process is provably + // dead (SIGKILL, crash, power loss skipped the deferred release). + // Without it, an abnormal exit leaves the lock in place forever and + // Clean can never reclaim the disk. Human locks and PID-less Zero + // leases (external prepare callers) are always honored. + expiredLease := false + if entry.locked { + pid, ok := leasePID(entry.lockReason) + if !ok || processAlive(pid) { + continue + } + expiredLease = true + } + + // Prefer the physical path for filesystem probes when the entry + // resolves; fall back to the registered spelling if it does not + // (for example a prunable entry whose directory is already gone). + statPath := entryPath + info, err := os.Stat(statPath) + if err != nil && statPath != entry.path { + statPath = entry.path + info, err = os.Stat(statPath) + } + if err != nil { + if os.IsNotExist(err) { + if expiredLease { + _, _ = gitOutput(ctx, runGit, repoRoot, "worktree", "unlock", entry.path) + } + _, _ = runGit(ctx, repoRoot, "worktree", "prune") + } + continue + } + if !info.IsDir() { + continue + } + + if worktreeIsStale(statPath, cutoff) { + // Ownership / legacy status first: the dirty probe's includeIgnored + // flag depends on it. An explicit release (unlocked, marker present) + // is a completion signal, so ignored residue is reclaimable. An + // expired lease is not (task may have died mid-work). A legacy + // pre-upgrade worktree never had Release at all — unlocked was its + // only state — so it gets the same benefit of the doubt as a + // crashed lease: ignored files block removal. Probing dirty before + // this would treat every legacy unlocked worktree as released and + // delete gitignored contents (e.g. .env) on first post-upgrade Clean. + owned, err := hasOwnershipMarker(ctx, runGit, statPath) + if err != nil { + continue + } + legacy := false + if !owned { + legacy = isLegacyZeroWorktree(ctx, runGit, statPath, repoDir, entry) + } + includeIgnored := expiredLease || legacy + if worktreeIsDirty(ctx, runGit, statPath, includeIgnored) { + // A stale mtime only means nothing changed at the worktree's + // top level or below recently; it does not mean the task + // holding it is done. Uncommitted or untracked changes are + // still live work waiting on a model, network, or user, so + // force-removing here would discard it. Skip until it either + // gets committed/cleaned (no longer dirty) or unlocked. + continue + } + if !owned { + if legacy { + if writeErr := writeOwnershipMarker(ctx, runGit, statPath); writeErr == nil { + owned = true + } + } + } + if !owned { + continue + } + if err := preserveUnreachableWorktreeHead(ctx, runGit, repoRoot, statPath); err != nil { + lastErr = errors.Join(lastErr, fmt.Errorf("preserve worktree HEAD %s: %w", entry.path, err)) + continue + } + if expiredLease { + // Recover the dead owner's lease only once every removal + // guard has passed, so a failed unlock (or a later guard) + // leaves the lock in place rather than half-recovered. + if _, err := gitOutput(ctx, runGit, repoRoot, "worktree", "unlock", entry.path); err != nil { + lastErr = errors.Join(lastErr, fmt.Errorf("recover expired lease %s: %w", entry.path, err)) + continue + } + } + // gitOutput (not a raw runGit call) so a nonzero exit code is + // reported as a failure: defaultRunGit deliberately returns a nil + // error alongside a nonzero CommandResult.ExitCode for a failed git + // invocation, so checking only the returned error would silently + // treat a failed removal (busy, permission denied) as success. + if _, err := gitOutput(ctx, runGit, repoRoot, "worktree", "remove", "--force", entry.path); err != nil { + // errors.Join, not a plain overwrite: multiple stale worktrees can + // fail removal in the same Clean pass (locking, permissions), and + // only reporting the last one would hide the others from the caller. + lastErr = errors.Join(lastErr, fmt.Errorf("remove worktree %s: %w", entry.path, err)) + } + } + } + + _, _ = runGit(ctx, repoRoot, "worktree", "prune") + return lastErr +} + +type worktreeEntry struct { + path string + locked bool + lockReason string +} + +// parseWorktreeList reads `git worktree list --porcelain` output into one +// entry per worktree. Entries are delimited by their own "worktree " +// line rather than by blank-line blocks, so this tolerates both real git +// output (attribute lines plus a blank-line separator) and a minimal listing +// with no separators. +func parseWorktreeList(output string) []worktreeEntry { + var entries []worktreeEntry + var current *worktreeEntry + for _, line := range strings.Split(output, "\n") { + switch { + case strings.HasPrefix(line, "worktree "): + if current != nil { + entries = append(entries, *current) + } + current = &worktreeEntry{path: canonicalizePath(strings.TrimPrefix(line, "worktree "))} + case current != nil && (line == "locked" || strings.HasPrefix(line, "locked ")): + current.locked = true + current.lockReason = strings.TrimSpace(strings.TrimPrefix(line, "locked")) + } + } + if current != nil { + entries = append(entries, *current) + } + return entries +} + +// isUnderDir reports whether path is dir itself or a descendant of it. Unlike +// a bare strings.HasPrefix(path, dir), this rejects a sibling that merely +// shares dir as a string prefix (e.g. dir "/a/base" must not match +// "/a/base-other"), and filepath.Rel makes the comparison Windows-correct. +func isUnderDir(path, dir string) bool { + rel, err := filepath.Rel(dir, path) + if err != nil { + return false + } + if rel == "." { + return true + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + +// preserveUnreachableWorktreeHead guards against `git worktree remove --force` +// silently discarding a commit: Prepare creates every worktree with `worktree +// add --detach`, so its HEAD is a plain commit, not a branch, and nothing +// outside that worktree's own administrative files points at it. If a task +// committed its result there and the worktree goes stale before that commit +// is otherwise referenced (merged, pushed, cherry-picked), force-removing it +// deletes the only ref keeping the commit reachable - it becomes immediately +// eligible for git gc, exactly as if it had never been committed. This checks +// whether some OTHER ref in the repository already contains worktreePath's +// HEAD; if none does, it creates a durable ref for it in the main +// repository's refs namespace before the caller proceeds to remove the +// worktree, so the commit survives (visible under refs/zero/orphaned-worktree) +// even after the worktree itself is gone. +func preserveUnreachableWorktreeHead(ctx context.Context, runGit GitRunner, repoRoot, worktreePath string) error { + head, err := gitOutput(ctx, runGit, worktreePath, "rev-parse", "HEAD") + if err != nil { + // No commit to preserve (an empty/unborn worktree, or one already + // gone) - nothing for this guard to do; let the caller proceed. + return nil + } + head = strings.TrimSpace(head) + if head == "" { + return nil + } + contained, err := gitOutput(ctx, runGit, repoRoot, "for-each-ref", "--contains="+head, "--count=1", "--format=%(refname)") + if err != nil { + return fmt.Errorf("check ref reachability for %s: %w", head, err) + } + if strings.TrimSpace(contained) != "" { + // Already reachable from some branch/tag; the worktree's own HEAD is + // redundant and removal cannot orphan the commit. + return nil + } + refName := "refs/zero/orphaned-worktree/" + head + if _, err := gitOutput(ctx, runGit, repoRoot, "update-ref", refName, head); err != nil { + return fmt.Errorf("preserve unreachable commit %s: %w", head, err) + } + return nil +} + +// worktreeIsStale reports whether every file under root was last modified +// before cutoff. A directory's own mtime only changes when an entry is +// added, removed, or renamed directly inside it, not when a long-running +// task edits files deeper in the tree, so checking root's mtime alone can +// mistake an actively-used worktree for stale. Walking the tree and bailing +// out on the first recent entry avoids that false positive. +// +// Any inspection failure (a WalkDir error, or a DirEntry that can't report its +// own info) fails closed: it's treated the same as "not stale," never as +// "stale," so an incomplete inspection can't authorize a forced removal. +func worktreeIsStale(root string, cutoff time.Time) bool { + stale := true + walkErr := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + stale = false + return filepath.SkipAll + } + info, err := d.Info() + if err != nil { + stale = false + return filepath.SkipAll + } + if info.ModTime().After(cutoff) { + stale = false + return filepath.SkipAll + } + return nil + }) + return stale && walkErr == nil +} + +// worktreeIsDirty reports whether a worktree has changes that block a forced +// removal, via `git status --porcelain` run inside it. A task can hold a +// worktree with live, unpushed work while it waits on a model, network, or +// user for far longer than the staleness window, without writing to the tree +// again in that time; mtime alone can't distinguish that from an abandoned +// one, but a dirty working tree still can. +// +// includeIgnored additionally counts files matched by .gitignore +// (credentials, generated drafts, task artifacts) as live data. It is set +// when the worktree's owner never signaled completion (an expired crashed +// lease): such files may be all a dead task left behind. It is clear for an +// explicitly released worktree, where ignored residue like node_modules or +// build output would otherwise make the released checkout unreclaimable at +// every age - release is the owner's statement that the task is done. +// +// An inspection failure fails closed, treating it as dirty rather than clean: +// an incomplete check must not authorize a forced removal. +func worktreeIsDirty(ctx context.Context, runGit GitRunner, path string, includeIgnored bool) bool { + args := []string{"status", "--porcelain"} + if includeIgnored { + args = append(args, "--ignored") + } + output, err := gitOutput(ctx, runGit, path, args...) + if err != nil { + return true + } + return output != "" +} diff --git a/internal/worktrees/worktrees_posix.go b/internal/worktrees/worktrees_posix.go new file mode 100644 index 000000000..0605ddf42 --- /dev/null +++ b/internal/worktrees/worktrees_posix.go @@ -0,0 +1,32 @@ +//go:build !windows + +package worktrees + +import ( + "errors" + "os" + "syscall" +) + +// osProcessAlive reports whether pid is a live process on POSIX. Signal 0 +// does not deliver a signal; it only checks existence/permission. Only ESRCH +// and os.ErrProcessDone mean the process is gone; EPERM means it exists but +// we may not signal it (alive). Any other error is treated as alive +// (fail-closed) so an ambiguous answer can only keep a lease, never expire +// one. +func osProcessAlive(pid int) bool { + proc, err := os.FindProcess(pid) + if err != nil { + // FindProcess rarely fails on Unix; fail closed if it does. + return true + } + err = proc.Signal(syscall.Signal(0)) + if err == nil { + return true + } + if errors.Is(err, os.ErrProcessDone) || errors.Is(err, syscall.ESRCH) { + return false + } + // EPERM and any other error: process may still exist. + return true +} diff --git a/internal/worktrees/worktrees_test.go b/internal/worktrees/worktrees_test.go index 90df4f76f..f824eb72d 100644 --- a/internal/worktrees/worktrees_test.go +++ b/internal/worktrees/worktrees_test.go @@ -29,7 +29,7 @@ func TestDefaultRunGitSeparatesStdoutAndStderr(t *testing.T) { t.Fatalf("Stderr should be empty on success, got %q", ok.Stderr) } - // A failing command's diagnostic must land on Stderr, not Stdout — the prior + // A failing command's diagnostic must land on Stderr, not Stdout - the prior // CombinedOutput merged them and left Stderr empty. bad, err := defaultRunGit(context.Background(), dir, "not-a-real-subcommand") if err != nil { @@ -44,14 +44,24 @@ func TestDefaultRunGitSeparatesStdoutAndStderr(t *testing.T) { } func TestPrepareCreatesDetachedGitWorktree(t *testing.T) { - root := t.TempDir() - base := t.TempDir() + root := physicalTestPath(t, t.TempDir()) + base := physicalTestPath(t, t.TempDir()) + // autoAbsoluteGitDir answers Prepare's post-lock ownership-marker write + // (`git rev-parse --absolute-git-dir`) for the newly created worktree + // without a canned result: without it, the fake runner falls off the end + // of results and returns an empty CommandResult, so writeOwnershipMarker + // resolves gitDir to "" and os.WriteFile writes "zero-owner" as a + // relative path into the test process's actual working directory instead + // of failing loudly. runner := &fakeRunner{ + autoAbsoluteGitDir: true, results: []CommandResult{ {Stdout: root + "\n"}, + {Stdout: "worktree " + root + "\n"}, {Stdout: "main\n"}, {Stdout: "abc1234\n"}, {}, + {}, }, } @@ -75,14 +85,244 @@ func TestPrepareCreatesDetachedGitWorktree(t *testing.T) { if !strings.HasPrefix(result.Path, filepath.Join(base, "zero-worktree-")) { t.Fatalf("Path = %q, want under base %q", result.Path, base) } - if got := runner.commandLine(3); got != "git worktree add --detach "+result.Path+" HEAD" { + if got := runner.commandLine(4); got != "git worktree add --detach "+result.Path+" HEAD" { t.Fatalf("git worktree command = %q", got) } + // Prepare must lock every worktree it creates: this is what makes + // entry.locked inside Clean protect zero's own worktrees, not just ones a + // human locked by hand (see TestCleanSkipsLockedZeroOwnedWorktree). + if got := runner.commandLine(5); got != "git worktree lock --reason zero: active task worktree "+result.Path { + t.Fatalf("git worktree lock command = %q", got) + } + if !result.LockAcquired { + t.Fatalf("LockAcquired = false, want true for a worktree this call created") + } + // Prepare must also persist the ownership marker so Release/Clean can + // tell this worktree apart from one a user created and locked by hand + // under the same predictable zero-worktree- path convention. + if got := runner.commandLine(6); got != "git rev-parse --absolute-git-dir" { + t.Fatalf("marker write command = %q", got) + } + if len(runner.calls) != 7 { + t.Fatalf("expected metadata calls plus lock and marker write, got %#v", runner.calls) + } +} + +func TestReleaseUnlocksWorktree(t *testing.T) { + // repoRoot must be resolved to its physical spelling up front: on a + // platform where the temp dir is itself a symlink (macOS /var -> + // /private/var), real `git worktree list --porcelain` reports the + // physical spelling, so computing repoKey from the lexical spelling here + // would produce a different key than verifyZeroOwnedWorktree derives in + // production, and Release would reject this genuinely Zero-owned fixture + // as not-zero-managed. The worktree path itself is physicalized for the + // same reason: Release compares the registered entry against the + // canonical user path, and a lexical /var spelling would not match. + repoRoot := physicalTestPath(t, t.TempDir()) + // path must carry the zero-worktree- ancestor component Prepare + // actually creates: Release now refuses to unlock anything else. + base := physicalTestPath(t, t.TempDir()) + path := filepath.Join(base, "zero-worktree-"+repoKey(repoRoot), "task-a") + if err := os.MkdirAll(path, 0o700); err != nil { + t.Fatal(err) + } + plantOwnershipMarker(t, path) + // The target entry carries Zero's own lease reason (as Prepare's lock + // call sets it), so the lock-reason check added alongside the ancestor + // check must let this release through rather than treating every locked + // entry as a manual, non-zero lock (see + // TestReleaseRejectsManuallyLockedWorktree for the rejecting case). + runner := &fakeRunner{ + autoAbsoluteGitDir: true, + results: []CommandResult{ + {Stdout: "worktree " + repoRoot + "\nworktree " + path + "\nlocked " + leaseReasonPrefix + "\n"}, + {}, + }, + } + + if err := Release(context.Background(), Options{RunGit: runner.Run}, path); err != nil { + t.Fatalf("Release returned error: %v", err) + } + // list + absolute-git-dir (marker) + unlock + if len(runner.calls) != 3 { + t.Fatalf("expected list, marker check, then unlock, got %#v", runner.calls) + } + if got := runner.commandLine(0); got != "git worktree list --porcelain" { + t.Fatalf("ownership-check command = %q", got) + } + if got := runner.commandLine(1); got != "git rev-parse --absolute-git-dir" { + t.Fatalf("marker-check command = %q", got) + } + if runner.calls[2].dir != path { + t.Fatalf("git worktree unlock dir = %q, want %q", runner.calls[2].dir, path) + } + if got := runner.commandLine(2); got != "git worktree unlock "+path { + t.Fatalf("git worktree unlock command = %q", got) + } +} + +// TestReleaseRejectsForgedZeroLeaseWithoutOwnershipMarker pins the fix for +// ownership being inferred from the public zero-worktree- path +// convention plus a lock reason that merely starts with leaseReasonPrefix: a +// user (or another tool) can create a same-repository worktree at that +// predictable path and lock it by hand with a reason like "zero: active task +// worktree manually-owned", which passes both the ancestor-component and +// HasPrefix checks. Only the ownership marker Prepare itself writes can tell +// that apart from a genuine Zero lease, so Release must still refuse to +// unlock it when the worktree directory exists but carries no marker. +func TestReleaseRejectsForgedZeroLeaseWithoutOwnershipMarker(t *testing.T) { + repoRoot := physicalTestPath(t, t.TempDir()) + base := physicalTestPath(t, t.TempDir()) + path := filepath.Join(base, "zero-worktree-"+repoKey(repoRoot), "manually-owned") + if err := os.MkdirAll(path, 0o700); err != nil { + t.Fatal(err) + } + // Deliberately no plantOwnershipMarker call: this worktree exists and is + // locked with a reason that starts with leaseReasonPrefix, exactly what a + // user could reproduce by hand, but Prepare never created it. + runner := &fakeRunner{ + autoAbsoluteGitDir: true, + results: []CommandResult{ + {Stdout: "worktree " + repoRoot + "\nworktree " + path + "\nlocked " + leaseReasonPrefix + " manually-owned\n"}, + }, + } + + err := Release(context.Background(), Options{RunGit: runner.Run}, path) + if err == nil || !strings.Contains(err.Error(), "missing zero ownership marker") { + t.Fatalf("Release error = %v, want a missing-ownership-marker rejection", err) + } + // list + marker check, no unlock call. + if len(runner.calls) != 2 { + t.Fatalf("expected only the ownership check and marker read, no unlock call, got %#v", runner.calls) + } +} + +func TestReleaseFallsBackToCwdWhenWorktreeDirMissing(t *testing.T) { + // A caller who deletes a locked worktree directory by hand instead of + // releasing it first leaves path itself gone; Release must still be able + // to run `git worktree unlock` (from the main repo, via options.Cwd) so + // the orphaned lock can be cleared and the entry later pruned. + // repoRoot is resolved to its physical spelling for the same reason as + // TestReleaseUnlocksWorktree: real `git worktree list --porcelain` + // reports the physical spelling, so a lexical temp-dir spelling here + // would derive a different repoKey than production and reject this + // fixture. The deleted path must still appear in the porcelain list + // (git keeps a prunable entry after a manual rm -rf) with Zero's lease + // reason, or the ownership check refuses the unlock. + repoRoot := physicalTestPath(t, t.TempDir()) + missingPath := filepath.Join(physicalTestPath(t, t.TempDir()), "zero-worktree-"+repoKey(repoRoot), "already-deleted") + runner := &fakeRunner{results: []CommandResult{ + {Stdout: "worktree " + repoRoot + "\nworktree " + missingPath + "\nlocked " + leaseReasonPrefix + "\n"}, + {}, + }} + + if err := Release(context.Background(), Options{RunGit: runner.Run, Cwd: repoRoot}, missingPath); err != nil { + t.Fatalf("Release returned error: %v", err) + } + if len(runner.calls) != 2 { + t.Fatalf("expected exactly two git calls (ownership check, then unlock), got %#v", runner.calls) + } + if runner.calls[0].dir != repoRoot || runner.calls[1].dir != repoRoot { + t.Fatalf("git calls = %#v, want both to fall back to Cwd %q", runner.calls, repoRoot) + } + if got := runner.commandLine(1); got != "git worktree unlock "+missingPath { + t.Fatalf("git worktree unlock command = %q, want the original path as the unlock target", got) + } +} + +// TestReleaseRejectsNonZeroOwnedWorktree pins the fix for Release being +// usable to clear the lock on a worktree a user (or another tool) manages by +// hand: the command is documented as releasing a worktree `prepare` created, +// not an arbitrary git worktree lock, so a path with no zero-worktree- +// ancestor component must be refused before any unlock is attempted. +func TestReleaseRejectsNonZeroOwnedWorktree(t *testing.T) { + repoRoot := t.TempDir() + manualWorktree := filepath.Join(t.TempDir(), "my-manual-worktree") + if err := os.MkdirAll(manualWorktree, 0o700); err != nil { + t.Fatal(err) + } + runner := &fakeRunner{results: []CommandResult{ + {Stdout: "worktree " + repoRoot + "\n"}, + }} + + err := Release(context.Background(), Options{RunGit: runner.Run}, manualWorktree) + if err == nil || !strings.Contains(err.Error(), "not a zero-managed worktree") { + t.Fatalf("Release error = %v, want a not-zero-managed rejection", err) + } + if len(runner.calls) != 1 { + t.Fatalf("expected only the ownership check, no unlock call, got %#v", runner.calls) + } +} + +// TestReleaseRejectsManuallyLockedWorktree pins the fix for Release being +// usable to clear a lock a user (or another tool) applied by hand to a +// worktree that otherwise sits inside Zero's zero-worktree- +// subtree: the ancestor-component check alone can't tell that lock apart +// from one of Zero's own leases, so Release must also refuse to unlock an +// entry whose recorded lock reason doesn't carry Zero's lease prefix. +func TestReleaseRejectsManuallyLockedWorktree(t *testing.T) { + repoRoot := physicalTestPath(t, t.TempDir()) + // Use a physical base so the porcelain entry path matches the + // canonicalizePath comparison Release uses against git's physical + // spelling (macOS /var vs /private/var, symlink TMPDIR layouts). + base := physicalTestPath(t, t.TempDir()) + path := filepath.Join(base, "zero-worktree-"+repoKey(repoRoot), "task-a") + if err := os.MkdirAll(path, 0o700); err != nil { + t.Fatal(err) + } + runner := &fakeRunner{results: []CommandResult{ + {Stdout: "worktree " + repoRoot + "\nworktree " + path + "\nlocked do not touch, in use\n"}, + }} + + err := Release(context.Background(), Options{RunGit: runner.Run}, path) + if err == nil || !strings.Contains(err.Error(), "not a zero lease") { + t.Fatalf("Release error = %v, want a not-a-zero-lease rejection", err) + } + if len(runner.calls) != 1 { + t.Fatalf("expected only the ownership check, no unlock call, got %#v", runner.calls) + } +} + +// TestReleaseRejectsUnregisteredWorktree pins the requirement that Release +// only unlocks a path git currently has registered for the repository. A +// same-looking zero-worktree- path that never appeared in +// `git worktree list` must not reach unlock. +func TestReleaseRejectsUnregisteredWorktree(t *testing.T) { + repoRoot := physicalTestPath(t, t.TempDir()) + base := physicalTestPath(t, t.TempDir()) + path := filepath.Join(base, "zero-worktree-"+repoKey(repoRoot), "not-registered") + if err := os.MkdirAll(path, 0o700); err != nil { + t.Fatal(err) + } + runner := &fakeRunner{results: []CommandResult{ + {Stdout: "worktree " + repoRoot + "\n"}, + }} + + err := Release(context.Background(), Options{RunGit: runner.Run}, path) + if err == nil || !strings.Contains(err.Error(), "not a registered worktree") { + t.Fatalf("Release error = %v, want a not-registered rejection", err) + } + if len(runner.calls) != 1 { + t.Fatalf("expected only the ownership check, no unlock call, got %#v", runner.calls) + } +} + +func TestReleasePropagatesGitFailure(t *testing.T) { + path := filepath.Join(t.TempDir(), "task-a") + if err := os.Mkdir(path, 0o700); err != nil { + t.Fatal(err) + } + runner := &fakeRunner{results: []CommandResult{{ExitCode: 1, Stderr: "fatal: not a working tree"}}} + + err := Release(context.Background(), Options{RunGit: runner.Run}, path) + if err == nil || !strings.Contains(err.Error(), "not a working tree") { + t.Fatalf("Release error = %v, want it to surface the git failure", err) + } } func TestPrepareReusesExistingGitWorktree(t *testing.T) { - root := t.TempDir() - base := t.TempDir() + root := physicalTestPath(t, t.TempDir()) + base := physicalTestPath(t, t.TempDir()) sourceGit := filepath.Join(root, ".git") if err := os.MkdirAll(sourceGit, 0o700); err != nil { t.Fatal(err) @@ -92,12 +332,15 @@ func TestPrepareReusesExistingGitWorktree(t *testing.T) { t.Fatal(err) } runner := &fakeRunner{ + autoAbsoluteGitDir: true, results: []CommandResult{ {Stdout: root + "\n"}, + {Stdout: "worktree " + root + "\n"}, {Stdout: "main\n"}, {Stdout: "abc1234\n"}, {Stdout: sourceGit + "\n"}, {Stdout: sourceGit + "\n"}, + {}, }, } @@ -117,16 +360,462 @@ func TestPrepareReusesExistingGitWorktree(t *testing.T) { if result.Path != existing { t.Fatalf("Path = %q, want existing %q", result.Path, existing) } - if len(runner.calls) != 5 { - t.Fatalf("expected metadata git calls only, got %#v", runner.calls) + // metadata calls + lock + ownership marker write (absolute-git-dir) + if len(runner.calls) != 8 { + t.Fatalf("expected metadata git calls plus lock and marker write, got %#v", runner.calls) + } + // The original lock may have been released by a prior run's exit, which + // would leave the reused worktree exposed to Clean's staleness heuristic + // while this caller is still using it: reuse must re-establish the lease. + if got := runner.commandLine(6); got != "git worktree lock --reason zero: active task worktree "+existing { + t.Fatalf("git worktree lock command = %q", got) + } + if got := runner.commandLine(7); got != "git rev-parse --absolute-git-dir" { + t.Fatalf("marker write command = %q", got) + } + if !result.LockAcquired { + t.Fatalf("LockAcquired = false, want true for a lease this call took") } } -func TestPrepareRejectsExistingWorktreeFromDifferentRepo(t *testing.T) { +func TestPrepareRejectsWorktreeLockedByAnotherRun(t *testing.T) { + // A reused worktree that is still locked belongs to another live run. + // Handing that checkout to a second caller would let two runs edit one + // supposedly isolated tree, and whichever exits first would release the + // single Git lock out from under the other, so Prepare must reject the + // in-use lease instead of returning the path. + root := physicalTestPath(t, t.TempDir()) + base := physicalTestPath(t, t.TempDir()) + sourceGit := filepath.Join(root, ".git") + if err := os.MkdirAll(sourceGit, 0o700); err != nil { + t.Fatal(err) + } + existing := filepath.Join(base, "zero-worktree-"+repoKey(root), "reuse-me") + if err := os.MkdirAll(filepath.Join(existing, ".git"), 0o700); err != nil { + t.Fatal(err) + } + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "worktree " + root + "\n"}, + {Stdout: "main\n"}, + {Stdout: "abc1234\n"}, + {Stdout: sourceGit + "\n"}, + {Stdout: sourceGit + "\n"}, + {ExitCode: 128, Stderr: "fatal: '" + existing + "' is already locked, reason: zero: active task worktree"}, + // reclaimDeadOwnerLease must preserve a lease owned by a live PID. + {Stdout: "worktree " + root + "\nworktree " + existing + "\nlocked " + leaseReason(os.Getpid()) + "\n"}, + }, + } + + _, err := Prepare(context.Background(), Options{ + Cwd: root, + Name: "reuse-me", + BaseDir: base, + RunGit: runner.Run, + }) + if err == nil || !strings.Contains(err.Error(), "locked by another active run") { + t.Fatalf("Prepare must reject an in-use lease, got %v", err) + } + for _, call := range runner.calls { + if len(call.args) >= 2 && call.args[0] == "worktree" && call.args[1] == "unlock" { + t.Fatalf("Prepare unlocked a live owner's lease: %#v", runner.calls) + } + } +} + +// TestPrepareReclaimsDeadOwnerLeaseOnReuse: a Zero lease whose recorded PID is +// dead must not brick the worktree name. Prepare's reuse path unlocks and +// re-locks so a SIGKILLed prior run does not force a manual release. +func TestPrepareReclaimsDeadOwnerLeaseOnReuse(t *testing.T) { + root := physicalTestPath(t, t.TempDir()) + base := physicalTestPath(t, t.TempDir()) + sourceGit := filepath.Join(root, ".git") + if err := os.MkdirAll(sourceGit, 0o700); err != nil { + t.Fatal(err) + } + existing := filepath.Join(base, "zero-worktree-"+repoKey(root), "reuse-me") + if err := os.MkdirAll(filepath.Join(existing, ".git"), 0o700); err != nil { + t.Fatal(err) + } + deadPID := deadProcessPID(t) + runner := &fakeRunner{ + autoAbsoluteGitDir: true, + results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "worktree " + root + "\n"}, + {Stdout: "main\n"}, + {Stdout: "abc1234\n"}, + {Stdout: sourceGit + "\n"}, + {Stdout: sourceGit + "\n"}, + {ExitCode: 128, Stderr: "fatal: '" + existing + "' is already locked"}, + // reclaimDeadOwnerLease: list shows a dead-owner Zero lease + {Stdout: "worktree " + root + "\nworktree " + existing + "\nlocked " + leaseReason(deadPID) + "\n"}, + {ExitCode: 0}, // unlock + {ExitCode: 0}, // re-lock + }, + } + + result, err := Prepare(context.Background(), Options{ + Cwd: root, + Name: "reuse-me", + BaseDir: base, + LeasePID: os.Getpid(), + RunGit: runner.Run, + }) + if err != nil { + t.Fatalf("Prepare should reclaim a dead-owner lease, got %v", err) + } + if !result.Reused || !result.LockAcquired { + t.Fatalf("expected reused+locked result, got %#v", result) + } + unlocked := false + locked := 0 + for _, call := range runner.calls { + if len(call.args) >= 2 && call.args[0] == "worktree" && call.args[1] == "unlock" { + unlocked = true + } + if len(call.args) >= 2 && call.args[0] == "worktree" && call.args[1] == "lock" { + locked++ + } + } + if !unlocked || locked < 2 { + t.Fatalf("expected unlock then re-lock, unlocked=%v lockCalls=%d calls=%#v", unlocked, locked, runner.calls) + } +} + +// TestPrepareRollsBackWorktreeOnLockFailure pins the fix for a newly created +// worktree being left behind, unleased, when the lock call after `git +// worktree add` fails for a reason other than a concurrent racer (an actual +// git failure, not "already locked"): nothing else can own a worktree this +// call just created, so Prepare must remove it rather than leaking it until +// Clean's staleness window eventually reclaims it. +func TestPrepareRollsBackWorktreeOnLockFailure(t *testing.T) { root := t.TempDir() base := t.TempDir() + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: root + "\n"}, + {Stdout: "worktree " + root + "\n"}, + {Stdout: "main\n"}, + {Stdout: "abc1234\n"}, + {}, // worktree add + {ExitCode: 1, Stderr: "fatal: disk full"}, // worktree lock + {}, // worktree remove --force (rollback) + }, + } + + _, err := Prepare(context.Background(), Options{ + Cwd: root, + Name: "review-api", + BaseDir: base, + RunGit: runner.Run, + }) + if err == nil || !strings.Contains(err.Error(), "disk full") { + t.Fatalf("Prepare must surface the lock failure, got %v", err) + } + if len(runner.calls) != 7 { + t.Fatalf("expected add, lock, and a rollback removal, got %#v", runner.calls) + } + lastCall := runner.calls[len(runner.calls)-1] + if lastCall.args[0] != "worktree" || lastCall.args[1] != "remove" { + t.Fatalf("last call = %#v, want a rollback `git worktree remove`", lastCall) + } +} + +// physicalTestPath resolves a test directory to its physical spelling +// (symlinks and Windows 8.3 short names), matching how git records paths. +func physicalTestPath(t *testing.T, path string) string { + t.Helper() + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + t.Fatalf("resolve %s: %v", path, err) + } + return resolved +} + +// TestPrepareFromLinkedWorktreeCanBeReleased pins the fix for Prepare and +// Release deriving different repoKeys when Prepare is invoked from a linked +// worktree instead of the main one. Before the fix, Prepare hashed +// --show-toplevel (the linked checkout's own root when run there), while +// Release always hashes the main worktree's root (via git worktree list +// --porcelain, whose first entry is always the main worktree); a worktree +// prepared from a linked checkout therefore failed its own ownership check +// on release and its lease could never be cleared. +func TestPrepareFromLinkedWorktreeCanBeReleased(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + ctx := context.Background() + mainRepo := physicalTestPath(t, t.TempDir()) + mustGit := func(dir string, args ...string) string { + t.Helper() + out, err := gitOutput(ctx, defaultRunGit, dir, args...) + if err != nil { + t.Skipf("git unavailable or failed (%v): %v", args, err) + } + return out + } + mustGit(mainRepo, "init") + mustGit(mainRepo, "-c", "user.email=t@example.invalid", "-c", "user.name=t", "commit", "--allow-empty", "-m", "seed") + + // A linked worktree of mainRepo, checked out elsewhere on disk. Its own + // --show-toplevel is this directory, not mainRepo. + linkedWorktree := filepath.Join(t.TempDir(), "linked") + mustGit(mainRepo, "worktree", "add", "--detach", linkedWorktree, "HEAD") + + base := physicalTestPath(t, t.TempDir()) + result, err := Prepare(ctx, Options{Cwd: linkedWorktree, BaseDir: base, Name: "from-linked"}) + if err != nil { + t.Fatalf("Prepare from a linked worktree: %v", err) + } + wantComponent := "zero-worktree-" + repoKey(mainRepo) + if !strings.Contains(result.Path, wantComponent) { + t.Fatalf("Path = %q, want it keyed off the main worktree %q (component %q)", result.Path, mainRepo, wantComponent) + } + + if err := Release(ctx, Options{}, result.Path); err != nil { + t.Fatalf("Release of a worktree Prepare created from a linked worktree: %v", err) + } +} + +// TestCleanFromLinkedWorktreePrunesStaleWorktree pins the fix for Clean +// keying its owned-subtree bucket off repoRoot (--show-toplevel for whatever +// worktree Clean itself runs from) instead of the main worktree's root: +// Prepare and Clean must agree on repoKey regardless of which checkout +// either call runs from, or a Clean invoked from a linked worktree computes +// a different repoDir than Prepare used and silently skips every +// zero-owned worktree for the repository. +func TestCleanFromLinkedWorktreePrunesStaleWorktree(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + ctx := context.Background() + mainRepo := physicalTestPath(t, t.TempDir()) + mustGit := func(dir string, args ...string) string { + t.Helper() + out, err := gitOutput(ctx, defaultRunGit, dir, args...) + if err != nil { + t.Skipf("git unavailable or failed (%v): %v", args, err) + } + return out + } + mustGit(mainRepo, "init") + mustGit(mainRepo, "-c", "user.email=t@example.invalid", "-c", "user.name=t", "commit", "--allow-empty", "-m", "seed") + + // A linked worktree of mainRepo, checked out elsewhere on disk. Its own + // --show-toplevel is this directory, not mainRepo, so Clean run from here + // would compute repoKey(linkedWorktree) unless it instead keys off the + // main worktree's root the way Prepare does. + linkedWorktree := filepath.Join(t.TempDir(), "linked") + mustGit(mainRepo, "worktree", "add", "--detach", linkedWorktree, "HEAD") + + base := physicalTestPath(t, t.TempDir()) + result, err := Prepare(ctx, Options{Cwd: linkedWorktree, BaseDir: base, Name: "stale-from-linked"}) + if err != nil { + t.Fatalf("Prepare from a linked worktree: %v", err) + } + if err := Release(ctx, Options{Cwd: linkedWorktree}, result.Path); err != nil { + t.Fatalf("Release: %v", err) + } + old := time.Now().Add(-48 * time.Hour) + if err := filepath.WalkDir(result.Path, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, old, old) + }); err != nil { + t.Fatal(err) + } + + // Clean itself also runs from the linked worktree here, mirroring a + // Prepare/Clean invocation started from a linked checkout. + if err := Clean(ctx, Options{Cwd: linkedWorktree, BaseDir: base}, 24*time.Hour); err != nil { + t.Fatalf("Clean: %v", err) + } + if _, err := os.Stat(result.Path); !os.IsNotExist(err) { + t.Fatalf("Clean run from a linked worktree should have pruned the stale zero-owned worktree, stat err: %v", err) + } +} + +// TestCleanPrunesStaleWorktreeUnderSymlinkedBaseDir pins the fix for a +// symlinked --worktree-dir: git worktree list --porcelain reports each +// worktree's PHYSICAL location, resolving any symlink component, so Clean +// comparing entries against a merely-absolute (not symlink-resolved) baseDir +// would reject every worktree created under a symlinked base and never prune +// it. Unlike the other Clean tests, base is deliberately handed to +// Prepare/Clean via its symlinked spelling, not its physical one (which +// physicalTestPath would normally produce), so this actually exercises the +// mismatch. +func TestCleanPrunesStaleWorktreeUnderSymlinkedBaseDir(t *testing.T) { + ctx := context.Background() + repo := physicalTestPath(t, t.TempDir()) + mustGit := func(args ...string) string { + t.Helper() + out, err := gitOutput(ctx, defaultRunGit, repo, args...) + if err != nil { + t.Skipf("git unavailable or failed (%v): %v", args, err) + } + return out + } + mustGit("init") + mustGit("-c", "user.email=t@example.invalid", "-c", "user.name=t", "commit", "--allow-empty", "-m", "seed") + + realBase := physicalTestPath(t, t.TempDir()) + base := filepath.Join(t.TempDir(), "base-link") + if err := os.Symlink(realBase, base); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + + if _, err := Prepare(ctx, Options{Cwd: repo, BaseDir: base, Name: "stale-task"}); err != nil { + t.Fatalf("Prepare: %v", err) + } + staleDir := filepath.Join(realBase, "zero-worktree-"+repoKey(repo), "stale-task") + if _, err := os.Stat(staleDir); err != nil { + t.Fatalf("expected worktree created at physical base path %s: %v", staleDir, err) + } + // Release the lock Prepare took and age every entry past the cutoff, so + // the worktree is both unlocked and stale. + if err := Release(ctx, Options{Cwd: repo}, staleDir); err != nil { + t.Fatalf("Release: %v", err) + } + old := time.Now().Add(-48 * time.Hour) + if err := filepath.WalkDir(staleDir, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, old, old) + }); err != nil { + t.Fatal(err) + } + + if err := Clean(ctx, Options{Cwd: repo, BaseDir: base}, 24*time.Hour); err != nil { + t.Fatalf("Clean: %v", err) + } + if _, err := os.Stat(staleDir); !os.IsNotExist(err) { + t.Fatalf("Clean should have pruned the stale worktree under the symlinked base dir, stat err: %v", err) + } +} + +// TestReleaseRecoversDeletedWorktreeUnderSymlinkedBaseDir pins the fix for +// the documented `release -C` recovery path failing when --worktree-dir was +// a symlink and the worktree itself was then deleted by hand: EvalSymlinks +// has nothing to resolve on a path that no longer exists, so the prior +// Abs+Clean fallback kept the symlinked (logical) spelling, which never +// matched git's physical-path porcelain entry, and the orphaned lock could +// never be released. canonicalizePath must resolve through the nearest +// existing ancestor (the symlink itself, since everything under it is gone) +// and reattach the missing tail. +func TestReleaseRecoversDeletedWorktreeUnderSymlinkedBaseDir(t *testing.T) { + ctx := context.Background() + repo := physicalTestPath(t, t.TempDir()) + realBase := physicalTestPath(t, t.TempDir()) + base := filepath.Join(t.TempDir(), "base-link") + if err := os.Symlink(realBase, base); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + + // The worktree directory under the symlinked base was deleted by hand; + // nothing on disk exists below base itself. + deletedPath := filepath.Join(base, "zero-worktree-"+repoKey(repo), "already-deleted") + physicalPath := filepath.Join(realBase, "zero-worktree-"+repoKey(repo), "already-deleted") + + runner := &fakeRunner{results: []CommandResult{ + {Stdout: "worktree " + repo + "\nworktree " + physicalPath + "\nlocked " + leaseReasonPrefix + "\n"}, + {}, + }} + + if err := Release(ctx, Options{RunGit: runner.Run, Cwd: repo}, deletedPath); err != nil { + t.Fatalf("Release must recover a deleted worktree under a symlinked base dir: %v", err) + } + if got := runner.commandLine(1); got != "git worktree unlock "+physicalPath { + t.Fatalf("git worktree unlock command = %q", got) + } +} + +// TestPrepareValidatesRequestBeforeCleanup pins the order of validation and +// the automatic stale-worktree pruning: a rejected request (an invalid +// --name) must not have destructive cleanup side effects before it reports +// its error. The second half proves the assertion has teeth: the same stale +// worktree IS pruned once a valid request runs. +func TestPrepareValidatesRequestBeforeCleanup(t *testing.T) { + ctx := context.Background() + // Canonicalize both roots up front: git records worktree paths in + // physical spelling, so a lexically different spelling of the same + // directory (macOS /var vs /private/var, Windows 8.3 short names on CI + // runners) would make Clean's containment check skip the entry. + repo := physicalTestPath(t, t.TempDir()) + mustGit := func(args ...string) string { + t.Helper() + out, err := gitOutput(ctx, defaultRunGit, repo, args...) + if err != nil { + t.Skipf("git unavailable or failed (%v): %v", args, err) + } + return out + } + mustGit("init") + mustGit("-c", "user.email=t@example.invalid", "-c", "user.name=t", "commit", "--allow-empty", "-m", "seed") + toplevel := filepath.Clean(mustGit("rev-parse", "--show-toplevel")) + + base := physicalTestPath(t, t.TempDir()) + staleDir := filepath.Join(base, "zero-worktree-"+repoKey(toplevel), "stale-task") + if err := os.MkdirAll(filepath.Dir(staleDir), 0o700); err != nil { + t.Fatal(err) + } + mustGit("worktree", "add", "--detach", staleDir) + // Plant the ownership marker Clean requires before it will force-remove a + // path: without it, a hand-created worktree under the predictable + // zero-worktree-* layout must not be pruned. Real linked worktrees keep + // their admin dir behind a .git file (gitdir: ...), so resolve it the + // same way writeOwnershipMarker does rather than mkdir path/.git. + gitDir, err := gitOutput(ctx, defaultRunGit, staleDir, "rev-parse", "--absolute-git-dir") + if err != nil { + t.Fatalf("resolve stale worktree git dir: %v", err) + } + if err := os.WriteFile(filepath.Join(gitDir, zeroOwnerMarkerFile), []byte(zeroOwnerMarkerContent), 0o600); err != nil { + t.Fatalf("plant ownership marker: %v", err) + } + // Age every filesystem entry past the 24h staleness cutoff. + old := time.Now().Add(-48 * time.Hour) + if err := filepath.WalkDir(staleDir, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, old, old) + }); err != nil { + t.Fatal(err) + } + // Also age the admin dir (outside the worktree tree) so nested activity + // under the marker file does not keep the worktree "fresh". + if err := filepath.WalkDir(gitDir, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, old, old) + }); err != nil { + t.Fatal(err) + } + + if _, err := Prepare(ctx, Options{Cwd: repo, BaseDir: base, Name: "../escape"}); err == nil { + t.Fatal("expected invalid-name error") + } + if _, err := os.Stat(staleDir); err != nil { + t.Fatalf("rejected request must not prune worktrees, stale dir: %v", err) + } + + if _, err := Prepare(ctx, Options{Cwd: repo, BaseDir: base, Name: "fresh-task"}); err != nil { + t.Fatalf("valid Prepare: %v", err) + } + if _, err := os.Stat(staleDir); !os.IsNotExist(err) { + t.Fatalf("valid request should have pruned the stale worktree, stat err: %v", err) + } +} + +func TestPrepareRejectsExistingWorktreeFromDifferentRepo(t *testing.T) { + root := physicalTestPath(t, t.TempDir()) + base := physicalTestPath(t, t.TempDir()) sourceGit := filepath.Join(root, ".git") - otherGit := filepath.Join(t.TempDir(), ".git") + otherGit := filepath.Join(physicalTestPath(t, t.TempDir()), ".git") for _, dir := range []string{sourceGit, otherGit} { if err := os.MkdirAll(dir, 0o700); err != nil { t.Fatal(err) @@ -139,6 +828,7 @@ func TestPrepareRejectsExistingWorktreeFromDifferentRepo(t *testing.T) { runner := &fakeRunner{ results: []CommandResult{ {Stdout: root + "\n"}, + {Stdout: "worktree " + root + "\n"}, {Stdout: "main\n"}, {Stdout: "abc1234\n"}, {Stdout: sourceGit + "\n"}, @@ -158,11 +848,12 @@ func TestPrepareRejectsExistingWorktreeFromDifferentRepo(t *testing.T) { } func TestPrepareValidatesNameAndExistingDirectory(t *testing.T) { - root := t.TempDir() - base := t.TempDir() + root := physicalTestPath(t, t.TempDir()) + base := physicalTestPath(t, t.TempDir()) runner := &fakeRunner{ results: []CommandResult{ {Stdout: root + "\n"}, + {Stdout: "worktree " + root + "\n"}, {Stdout: "main\n"}, {Stdout: "abc1234\n"}, }, @@ -217,10 +908,22 @@ func TestDefaultBaseDirFallsBackForWindowsUserProfile(t *testing.T) { type fakeRunner struct { calls []gitCall results []CommandResult + // autoAbsoluteGitDir answers `rev-parse --absolute-git-dir` without + // consuming a queued result: it ensures dir/.git exists and returns that + // path. Tests plant zero-owner markers under that dir when they want + // Release/Clean to treat the worktree as Prepare-created. + autoAbsoluteGitDir bool } func (runner *fakeRunner) Run(ctx context.Context, dir string, args ...string) (CommandResult, error) { runner.calls = append(runner.calls, gitCall{dir: dir, args: append([]string{}, args...)}) + if runner.autoAbsoluteGitDir && len(args) >= 2 && args[0] == "rev-parse" && args[1] == "--absolute-git-dir" { + gitDir := filepath.Join(dir, ".git") + if err := os.MkdirAll(gitDir, 0o700); err != nil { + return CommandResult{}, err + } + return CommandResult{Stdout: gitDir + "\n"}, nil + } if len(runner.results) == 0 { return CommandResult{}, nil } @@ -236,6 +939,41 @@ func (runner *fakeRunner) commandLine(index int) string { return "git " + strings.Join(runner.calls[index].args, " ") } +// firstStatusCommand returns the first `git status ...` call, if any. Clean +// inserts ownership-marker checks before the dirty probe, so status is no +// longer at a fixed call index. +func (runner *fakeRunner) firstStatusCommand() string { + for i := range runner.calls { + if line := runner.commandLine(i); strings.HasPrefix(line, "git status ") { + return line + } + } + return "" +} + +// hasGitCommand reports whether any recorded call equals want (full "git …" form). +func (runner *fakeRunner) hasGitCommand(want string) bool { + for i := range runner.calls { + if runner.commandLine(i) == want { + return true + } + } + return false +} + +// plantOwnershipMarker writes Prepare's ownership marker under path's .git +// admin dir so Release/Clean treat the fixture as zero-created. +func plantOwnershipMarker(t *testing.T, path string) { + t.Helper() + gitDir := filepath.Join(path, ".git") + if err := os.MkdirAll(gitDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(gitDir, zeroOwnerMarkerFile), []byte(zeroOwnerMarkerContent), 0o600); err != nil { + t.Fatal(err) + } +} + type gitCall struct { dir string args []string @@ -248,3 +986,1102 @@ func fixedTime(value string) func() time.Time { } return func() time.Time { return parsed } } + +func TestCleanPrunesStaleWorktrees(t *testing.T) { + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + // Create directories representing two worktrees: one young, one stale. + youngPath := filepath.Join(repoDir, "young-task") + stalePath := filepath.Join(repoDir, "stale-task") + if err := os.MkdirAll(youngPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(stalePath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + // Markers first, then age the whole tree: planting after Chtimes would + // refresh the worktree mtime and make worktreeIsStale skip the prune. + plantOwnershipMarker(t, youngPath) + plantOwnershipMarker(t, stalePath) + + // Change mtime of stale-task (and its marker files) to be in the past. + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := filepath.WalkDir(stalePath, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, twoDaysAgo, twoDaysAgo) + }); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + autoAbsoluteGitDir: true, + results: []CommandResult{ + {Stdout: repoRoot}, // rev-parse --show-toplevel + // Main worktree must be listed first: Clean keys repoDir off entries[0]. + {Stdout: "worktree " + repoRoot + "\nworktree " + youngPath + "\nworktree " + stalePath + "\n"}, + {ExitCode: 0}, // status --porcelain (clean) + {Stdout: "deadbeef"}, // rev-parse HEAD + {Stdout: "refs/heads/main"}, // for-each-ref --contains=deadbeef (already reachable) + {ExitCode: 0}, // worktree remove --force + {ExitCode: 0}, // worktree prune + }, + } + + options := Options{ + Cwd: repoRoot, + BaseDir: baseDir, + RunGit: runner.Run, + } + + err := Clean(context.Background(), options, 24*time.Hour) + if err != nil { + t.Fatalf("Clean failed: %v", err) + } + + // young is skipped as not-stale before any status/marker work. + // For the stale path: marker check, then status, then HEAD preservation, remove, prune. + if runner.commandLine(0) != "git rev-parse --show-toplevel" { + t.Errorf("call 0 = %q", runner.commandLine(0)) + } + if runner.commandLine(1) != "git worktree list --porcelain" { + t.Errorf("call 1 = %q", runner.commandLine(1)) + } + if got, want := runner.firstStatusCommand(), "git status --porcelain"; got != want { + t.Errorf("status call = %q, want %q", got, want) + } + expectedRemoveCall := "git worktree remove --force " + filepath.Clean(stalePath) + if !runner.hasGitCommand(expectedRemoveCall) { + t.Errorf("missing remove call %q in %#v", expectedRemoveCall, runner.calls) + } + if !runner.hasGitCommand("git worktree prune") { + t.Errorf("missing prune call in %#v", runner.calls) + } +} + +// defaultRunGit deliberately returns a nil error alongside a nonzero +// CommandResult.ExitCode for a failed git invocation (see its comment), so +// Clean must check ExitCode itself rather than trusting a nil error to mean +// the removal succeeded. +func TestCleanReportsErrorOnFailedRemoval(t *testing.T) { + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + stalePath := filepath.Join(repoDir, "stale-task") + if err := os.MkdirAll(stalePath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + plantOwnershipMarker(t, stalePath) + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := filepath.WalkDir(stalePath, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, twoDaysAgo, twoDaysAgo) + }); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + autoAbsoluteGitDir: true, + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + repoRoot + "\nworktree " + stalePath + "\n"}, + {ExitCode: 0}, // status --porcelain (clean) + {Stdout: "deadbeef"}, // rev-parse HEAD + {Stdout: "refs/heads/main"}, // for-each-ref --contains=deadbeef (already reachable) + {ExitCode: 1, Stderr: "fatal: unable to remove worktree: in use"}, // worktree remove --force + {ExitCode: 0}, + }, + } + + err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour) + if err == nil { + t.Fatal("expected Clean to report the failed removal") + } + if !strings.Contains(err.Error(), "in use") { + t.Errorf("error = %q, want it to include the git failure message", err.Error()) + } +} + +func TestCleanAggregatesMultipleFailedRemovals(t *testing.T) { + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + stalePathA := filepath.Join(repoDir, "stale-task-a") + stalePathB := filepath.Join(repoDir, "stale-task-b") + for _, path := range []string{stalePathA, stalePathB} { + if err := os.MkdirAll(path, 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + for _, path := range []string{stalePathA, stalePathB} { + plantOwnershipMarker(t, path) + } + twoDaysAgo := time.Now().Add(-48 * time.Hour) + for _, path := range []string{stalePathA, stalePathB} { + if err := filepath.WalkDir(path, func(p string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(p, twoDaysAgo, twoDaysAgo) + }); err != nil { + t.Fatal(err) + } + } + + runner := &fakeRunner{ + autoAbsoluteGitDir: true, + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + repoRoot + "\nworktree " + stalePathA + "\n\nworktree " + stalePathB + "\n"}, + {ExitCode: 0}, // status --porcelain (clean) + {Stdout: "deadbeefa"}, // rev-parse HEAD + {Stdout: "refs/heads/main"}, // for-each-ref --contains=deadbeefa (already reachable) + {ExitCode: 1, Stderr: "fatal: unable to remove worktree A"}, // remove stalePathA + {ExitCode: 0}, // status --porcelain (clean) + {Stdout: "deadbeefb"}, // rev-parse HEAD + {Stdout: "refs/heads/main"}, // for-each-ref --contains=deadbeefb (already reachable) + {ExitCode: 1, Stderr: "fatal: unable to remove worktree B"}, // remove stalePathB + {ExitCode: 0}, // final prune + }, + } + + err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour) + if err == nil { + t.Fatal("expected Clean to report both failed removals") + } + // Both failures must survive in the returned error, not just the last one + // to occur - overwriting lastErr instead of joining would silently drop + // worktree A's failure once worktree B's removal is also attempted. + if !strings.Contains(err.Error(), "unable to remove worktree A") { + t.Errorf("error = %q, missing worktree A's failure", err.Error()) + } + if !strings.Contains(err.Error(), "unable to remove worktree B") { + t.Errorf("error = %q, missing worktree B's failure", err.Error()) + } +} + +// An inspection failure (the root can't be stat'd or walked) must fail +// closed: worktreeIsStale reports false rather than true, so an incomplete +// inspection can never authorize a forced removal. +func TestWorktreeIsStaleFailsClosedOnInspectionError(t *testing.T) { + missing := filepath.Join(t.TempDir(), "does-not-exist") + if worktreeIsStale(missing, time.Now()) { + t.Fatal("expected worktreeIsStale to fail closed for an uninspectable root") + } +} + +func TestWorktreeIsStaleTrueForOldUntouchedTree(t *testing.T) { + dir := t.TempDir() + old := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(dir, old, old); err != nil { + t.Fatal(err) + } + if !worktreeIsStale(dir, time.Now().Add(-24*time.Hour)) { + t.Fatal("expected an old, untouched directory to be reported stale") + } +} + +// A worktree with a stale top-level mtime but a file that was written deep +// inside the tree more recently must not be pruned: the directory's own mtime +// only changes when an entry is added/removed/renamed directly inside it, not +// when a long-running task edits an existing nested file. +func TestCleanSkipsWorktreeWithRecentNestedActivity(t *testing.T) { + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + activePath := filepath.Join(repoDir, "active-task") + nestedDir := filepath.Join(activePath, "internal", "pkg") + if err := os.MkdirAll(nestedDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(activePath, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(nestedDir, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + + // The file itself is freshly written (default mtime is now), simulating a + // task actively editing code deep in the worktree. + nestedFile := filepath.Join(nestedDir, "handler.go") + if err := os.WriteFile(nestedFile, []byte("package pkg"), 0o644); err != nil { + t.Fatal(err) + } + + // activePath/internal was created alongside nestedDir by the MkdirAll + // above and never backdated, so it would otherwise still carry a fresh + // mtime; WalkDir would hit it and report "not stale" before ever reaching + // nestedFile, so the test would pass even if the recursive walk stopped + // checking after the first directory level. + if err := os.Chtimes(filepath.Join(activePath, "internal"), twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: repoRoot}, + // Main worktree must be listed first: Clean keys repoDir off entries[0]. + {Stdout: "worktree " + repoRoot + "\nworktree " + activePath + "\n"}, + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + + // Nested activity makes worktreeIsStale false before any status/remove. + if got, want := runner.commandLine(1), "git worktree list --porcelain"; got != want { + t.Fatalf("call 1 = %q, want %q", got, want) + } + if got, want := runner.commandLine(2), "git worktree prune"; got != want { + t.Fatalf("call 2 = %q, want %q (not-stale skip before status/remove)", got, want) + } + for _, call := range runner.calls { + if len(call.args) > 0 && call.args[0] == "remove" { + t.Fatalf("Clean removed an actively-edited worktree: %v", call.args) + } + if len(call.args) > 0 && call.args[0] == "status" { + t.Fatalf("Clean probed status on a not-stale worktree: %v", call.args) + } + } +} + +// A worktree explicitly locked via `git worktree lock` must never be pruned, +// regardless of how stale its mtime looks. +func TestCleanSkipsLockedWorktree(t *testing.T) { + tempDir := t.TempDir() + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + + lockedPath := filepath.Join(baseDir, "locked-task") + if err := os.MkdirAll(lockedPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(lockedPath, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + lockedPath + "\nHEAD abc1234\nlocked in use by zero\n"}, + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + + for _, call := range runner.calls { + if len(call.args) > 0 && call.args[0] == "remove" { + t.Fatalf("Clean removed a locked worktree: %v", call.args) + } + } +} + +// A worktree zero created and locked at Prepare time (see the "worktree lock" +// call added there) must survive Clean even though it looks idle-but-clean: a +// task can finish committing and then sit waiting on a model, network, or +// user for far longer than the staleness window without touching the tree +// again, and mtime alone can't distinguish that from an abandoned worktree. +// Before this fix, Prepare never locked its own worktrees, so entry.locked +// only ever protected worktrees a human locked by hand. +func TestCleanSkipsLockedZeroOwnedWorktree(t *testing.T) { + tempDir := t.TempDir() + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + idlePath := filepath.Join(repoDir, "idle-task") + if err := os.MkdirAll(idlePath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(idlePath, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + idlePath + "\nlocked zero: active task worktree\n"}, + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + + for _, call := range runner.calls { + if len(call.args) > 0 && (call.args[0] == "remove" || call.args[0] == "status") { + t.Fatalf("Clean touched a locked zero-owned worktree: %v", call.args) + } + } +} + +// A worktree whose only content is matched by .gitignore (credentials, +// generated drafts, task artifacts) must still block force-removal: plain +// `git status --porcelain` reports such a worktree as clean, but +// worktreeIsDirty now also passes --ignored, so Clean must treat it as dirty. +// TestCleanPreservesUnreachableCommitBeforeRemoval pins the fix for a real +// data-loss case: Prepare creates every worktree with `worktree add --detach`, +// so a commit made there is reachable only through that worktree's own HEAD. +// If the worktree goes stale and clean before the commit is otherwise +// referenced, force-removing it must not let the commit become unreachable - +// Clean has to preserve it under a durable ref first. +func TestCleanPreservesUnreachableCommitBeforeRemoval(t *testing.T) { + ctx := context.Background() + repo := physicalTestPath(t, t.TempDir()) + mustGit := func(dir string, args ...string) string { + t.Helper() + out, err := gitOutput(ctx, defaultRunGit, dir, args...) + if err != nil { + t.Skipf("git unavailable or failed (%v): %v", args, err) + } + return out + } + mustGit(repo, "init") + mustGit(repo, "-c", "user.email=t@example.invalid", "-c", "user.name=t", "commit", "--allow-empty", "-m", "seed") + + base := physicalTestPath(t, t.TempDir()) + if _, err := Prepare(ctx, Options{Cwd: repo, BaseDir: base, Name: "orphan-task"}); err != nil { + t.Fatalf("Prepare: %v", err) + } + staleDir := filepath.Join(base, "zero-worktree-"+repoKey(repo), "orphan-task") + + // Commit inside the worktree: this HEAD is not on any branch, so nothing + // outside the worktree itself points at it yet. + mustGit(staleDir, "-c", "user.email=t@example.invalid", "-c", "user.name=t", "commit", "--allow-empty", "-m", "orphaned work") + orphanSHA := mustGit(staleDir, "rev-parse", "HEAD") + + if err := Release(ctx, Options{Cwd: repo}, staleDir); err != nil { + t.Fatalf("Release: %v", err) + } + old := time.Now().Add(-48 * time.Hour) + if err := filepath.WalkDir(staleDir, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, old, old) + }); err != nil { + t.Fatal(err) + } + + if err := Clean(ctx, Options{Cwd: repo, BaseDir: base}, 24*time.Hour); err != nil { + t.Fatalf("Clean: %v", err) + } + if _, err := os.Stat(staleDir); !os.IsNotExist(err) { + t.Fatalf("Clean should have pruned the stale worktree, stat err: %v", err) + } + if _, err := gitOutput(ctx, defaultRunGit, repo, "cat-file", "-e", orphanSHA); err != nil { + t.Fatalf("commit %s is no longer reachable after Clean: %v", orphanSHA, err) + } + preserved := mustGit(repo, "for-each-ref", "--contains="+orphanSHA, "--count=1", "--format=%(refname)") + if strings.TrimSpace(preserved) == "" { + t.Fatalf("commit %s survived only by luck (not yet GC'd); expected a durable ref to contain it", orphanSHA) + } +} + +func TestCleanReclaimsReleasedWorktreeWithOnlyIgnoredFiles(t *testing.T) { + // An explicit release is the owner's completion signal: an unlocked, + // stale worktree holding only gitignored residue (node_modules, build + // output) must be reclaimable, or every released worktree with such + // artifacts leaks disk forever. The dirty probe for unlocked entries + // therefore omits --ignored. + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + ignoredOnlyPath := filepath.Join(repoDir, "ignored-only-task") + if err := os.MkdirAll(ignoredOnlyPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + plantOwnershipMarker(t, ignoredOnlyPath) + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := filepath.WalkDir(ignoredOnlyPath, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, twoDaysAgo, twoDaysAgo) + }); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + autoAbsoluteGitDir: true, + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + repoRoot + "\nworktree " + ignoredOnlyPath + "\n"}, + {ExitCode: 0}, // status --porcelain: ignored files invisible => clean + {Stdout: "deadbeef"}, // rev-parse HEAD + {Stdout: "refs/heads/main"}, // for-each-ref --contains (reachable) + {ExitCode: 0}, // worktree remove --force + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + + if got, want := runner.firstStatusCommand(), "git status --porcelain"; got != want { + t.Fatalf("status call = %q, want %q (released worktrees must not count ignored residue)", got, want) + } + removed := false + for _, call := range runner.calls { + if len(call.args) > 1 && call.args[0] == "worktree" && call.args[1] == "remove" { + removed = true + } + } + if !removed { + t.Fatal("Clean did not reclaim a released, stale worktree holding only ignored residue") + } +} + +// TestCleanRecoversExpiredLease: a Zero lease that records its owning PID is +// recoverable - if that process died without releasing (SIGKILL, crash), the +// lock must not protect the worktree forever. A stale, clean worktree behind +// a dead-owner lease is unlocked and removed; the dirty probe there keeps +// --ignored because a crashed task never signaled completion. +func TestCleanRecoversExpiredLease(t *testing.T) { + deadPID := deadProcessPID(t) + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + crashedPath := filepath.Join(repoDir, "crashed-task") + if err := os.MkdirAll(crashedPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + plantOwnershipMarker(t, crashedPath) + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := filepath.WalkDir(crashedPath, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, twoDaysAgo, twoDaysAgo) + }); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + autoAbsoluteGitDir: true, + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + repoRoot + "\nworktree " + crashedPath + "\nlocked " + leaseReason(deadPID) + "\n"}, + {ExitCode: 0}, // status --porcelain --ignored: clean + {Stdout: "deadbeef"}, // rev-parse HEAD + {Stdout: "refs/heads/main"}, // for-each-ref --contains (reachable) + {ExitCode: 0}, // worktree unlock (lease recovery) + {ExitCode: 0}, // worktree remove --force + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + + if got, want := runner.firstStatusCommand(), "git status --porcelain --ignored"; got != want { + t.Fatalf("status call = %q, want %q (a crashed lease never signaled completion)", got, want) + } + if !runner.hasGitCommand("git worktree unlock " + filepath.Clean(crashedPath)) { + t.Fatalf("missing lease recovery unlock, calls=%#v", runner.calls) + } + if !runner.hasGitCommand("git worktree remove --force " + filepath.Clean(crashedPath)) { + t.Fatalf("missing remove after lease recovery, calls=%#v", runner.calls) + } +} + +// TestCleanSkipsExpiredLeaseWithIgnoredData: even behind a dead-owner lease, +// ignored files (credentials, generated drafts) may be all the crashed task +// left behind, so they still block removal. +func TestCleanSkipsExpiredLeaseWithIgnoredData(t *testing.T) { + deadPID := deadProcessPID(t) + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + crashedPath := filepath.Join(repoDir, "crashed-task") + if err := os.MkdirAll(crashedPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(crashedPath, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: repoRoot}, + // Main worktree must be listed first: Clean keys repoDir off entries[0]. + {Stdout: "worktree " + repoRoot + "\nworktree " + crashedPath + "\nlocked " + leaseReason(deadPID) + "\n"}, + {Stdout: "!! ignored-data\n"}, // status --porcelain --ignored + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + // Prove the dirty probe with --ignored actually ran (the guard under test), + // not that Clean skipped the entry for an unrelated ownership reason. + if got, want := runner.firstStatusCommand(), "git status --porcelain --ignored"; got != want { + t.Fatalf("status call = %q, want %q", got, want) + } + if !runner.hasGitCommand("git worktree prune") { + t.Fatalf("missing prune call, calls=%#v", runner.calls) + } + for _, call := range runner.calls { + if len(call.args) > 1 && call.args[0] == "worktree" && (call.args[1] == "remove" || call.args[1] == "unlock") { + t.Fatalf("Clean touched a crashed worktree holding ignored data: %v", call.args) + } + } +} + +// TestCleanHonorsLiveLease: a lease whose recorded owner is still running is +// never expired, regardless of staleness. +func TestCleanHonorsLiveLease(t *testing.T) { + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + livePath := filepath.Join(repoDir, "live-task") + if err := os.MkdirAll(livePath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(livePath, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: repoRoot}, + // Main worktree must be listed first: Clean keys repoDir off entries[0]. + {Stdout: "worktree " + repoRoot + "\nworktree " + livePath + "\nlocked " + leaseReason(os.Getpid()) + "\n"}, + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + // Live lease short-circuits before status/remove: list then prune only. + if got, want := runner.commandLine(1), "git worktree list --porcelain"; got != want { + t.Fatalf("call 1 = %q, want %q", got, want) + } + if got, want := runner.commandLine(2), "git worktree prune"; got != want { + t.Fatalf("call 2 = %q, want %q (live lease skips status/remove)", got, want) + } + for _, call := range runner.calls { + if len(call.args) > 0 && (call.args[0] == "remove" || call.args[0] == "status") { + t.Fatalf("Clean touched a worktree behind a live lease: %v", call.args) + } + } +} + +// deadProcessPID returns the PID of a process that has already exited, for +// exercising lease expiry. PID reuse in the instant between exit and the +// assertion is vanishingly unlikely. +func deadProcessPID(t *testing.T) int { + t.Helper() + gitPath, err := exec.LookPath("git") + if err != nil { + t.Skip("git not available") + } + cmd := exec.Command(gitPath, "version") + if err := cmd.Run(); err != nil { + t.Fatalf("run git version: %v", err) + } + return cmd.Process.Pid +} + +// TestCleanHonorsTouchLiveness verifies that a stale worktree (mtime older +// than maxAge) is skipped by Clean when it was recently touched by Prepare, +// so a long-running but idle task that Prepare is still associated with is +// not wrongly force-removed. Fresh mtime alone is enough; no lock is required. +func TestCleanHonorsTouchLiveness(t *testing.T) { + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + touchedPath := filepath.Join(repoDir, "touched-task") + if err := os.MkdirAll(touchedPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + // Age the worktree past maxAge so it would be pruned without the touch. + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(touchedPath, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + plantOwnershipMarker(t, touchedPath) + // Re-age the marker files planted above so only the subsequent touch + // refreshes liveness. + if err := filepath.WalkDir(touchedPath, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, twoDaysAgo, twoDaysAgo) + }); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: repoRoot}, + // Main worktree must be listed first: Clean keys repoDir off entries[0]. + {Stdout: "worktree " + repoRoot + "\nworktree " + touchedPath + "\n"}, + {ExitCode: 0}, // worktree prune (no-op) + }, + } + + // Touch the directory now, simulating Prepare being called for this worktree. + if err := os.Chtimes(touchedPath, time.Now(), time.Now()); err != nil { + t.Fatal(err) + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean: %v", err) + } + // Fresh touch makes worktreeIsStale false before status/remove. + if got, want := runner.commandLine(1), "git worktree list --porcelain"; got != want { + t.Fatalf("call 1 = %q, want %q", got, want) + } + if got, want := runner.commandLine(2), "git worktree prune"; got != want { + t.Fatalf("call 2 = %q, want %q (fresh mtime skips status/remove)", got, want) + } + for _, call := range runner.calls { + if len(call.args) > 0 && (call.args[0] == "remove" || call.args[0] == "status") { + t.Fatalf("Clean touched a freshly-touched worktree: %v", call.args) + } + } + if _, err := os.Stat(touchedPath); err != nil { + t.Fatalf("worktree %s was incorrectly removed: %v", touchedPath, err) + } +} + +// worktreeIsDirty must count files matched by .gitignore as dirty content: a +// worktree holding only ignored task data (credentials, generated drafts) has +// nothing to show in plain `git status --porcelain` and would otherwise pass +// as clean and be force-removed by Clean's staleness heuristic. This exercises +// the real git binary rather than the fake runner, so it also verifies +// --ignored actually changes git's answer, not just the command we send. +func TestWorktreeIsDirtyCountsIgnoredFilesAsDirty(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + dir := t.TempDir() + run := func(args ...string) { + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + run("init", "--quiet") + run("config", "user.email", "zero@example.com") + run("config", "user.name", "zero") + if err := os.WriteFile(filepath.Join(dir, ".gitignore"), []byte("ignored-data\n"), 0o644); err != nil { + t.Fatal(err) + } + run("add", ".gitignore") + run("commit", "--quiet", "-m", "initial") + + if worktreeIsDirty(context.Background(), defaultRunGit, dir, true) { + t.Fatal("expected a clean worktree with no ignored files present to report clean") + } + + if err := os.WriteFile(filepath.Join(dir, "ignored-data"), []byte("secret task artifact"), 0o644); err != nil { + t.Fatal(err) + } + + if !worktreeIsDirty(context.Background(), defaultRunGit, dir, true) { + t.Fatal("expected an ignored-but-present file to count as dirty") + } +} + +// A sibling directory that merely shares the per-repository repoDir as a +// string prefix (e.g. "-other") must not be treated as zero-owned. +// This also covers a manually managed worktree for the SAME repository that a +// user placed directly under a shared baseDir rather than inside zero's +// repoDir subtree: it must not be treated as zero-owned either. +func TestCleanRejectsSiblingDirWithSharedPrefix(t *testing.T) { + tempDir := t.TempDir() + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + siblingDir := repoDir + "-other" + + siblingPath := filepath.Join(siblingDir, "not-ours") + if err := os.MkdirAll(siblingPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(siblingPath, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + siblingPath + "\n"}, + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + + for _, call := range runner.calls { + if len(call.args) > 0 && call.args[0] == "remove" { + t.Fatalf("Clean removed a sibling directory outside baseDir: %v", call.args) + } + } +} + +// A manually managed worktree that a user placed directly under a shared +// baseDir, outside zero's own "zero-worktree-" subtree, must never +// be pruned even though it is technically inside baseDir. +func TestCleanIgnoresWorktreeOutsideOwnedSubtree(t *testing.T) { + tempDir := t.TempDir() + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + + manualPath := filepath.Join(baseDir, "hand-managed-checkout") + if err := os.MkdirAll(manualPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(manualPath, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + manualPath + "\n"}, + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + + for _, call := range runner.calls { + if len(call.args) > 0 && (call.args[0] == "remove" || call.args[0] == "status") { + t.Fatalf("Clean touched a worktree outside its owned subtree: %v", call.args) + } + } +} + +// A worktree with a stale top-level mtime but uncommitted or untracked +// changes must not be force-removed: a task can hold live work in a worktree +// while waiting on a model, network, or user for far longer than the +// staleness window, without ever writing to the tree again in that time. +func TestCleanSkipsDirtyStaleWorktree(t *testing.T) { + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + dirtyPath := filepath.Join(repoDir, "dirty-task") + if err := os.MkdirAll(dirtyPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := os.Chtimes(dirtyPath, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: repoRoot}, + // Main worktree must be listed first: Clean keys repoDir off entries[0]. + {Stdout: "worktree " + repoRoot + "\nworktree " + dirtyPath + "\n"}, + {Stdout: " M internal/pkg/handler.go\n"}, // status --porcelain: dirty + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + + // Prove the dirty status probe actually ran (the guard under test). + if got, want := runner.firstStatusCommand(), "git status --porcelain"; got != want { + t.Fatalf("status call = %q, want %q", got, want) + } + if !runner.hasGitCommand("git worktree prune") { + t.Fatalf("missing prune call, calls=%#v", runner.calls) + } + for _, call := range runner.calls { + if len(call.args) > 1 && call.args[0] == "worktree" && call.args[1] == "remove" { + t.Fatalf("Clean removed a dirty worktree: %v", call.args) + } + } +} + +// An inspection failure (git status errors out) must fail closed: treat the +// worktree as dirty rather than clean, so a broken status check can never +// authorize a forced removal. +func TestWorktreeIsDirtyFailsClosedOnInspectionError(t *testing.T) { + runner := &fakeRunner{results: []CommandResult{{ExitCode: 1, Stderr: "fatal: not a git repository"}}} + if !worktreeIsDirty(context.Background(), runner.Run, t.TempDir(), true) { + t.Fatal("expected worktreeIsDirty to fail closed on a status error") + } +} + +func TestIsUnderDir(t *testing.T) { + base := filepath.Join(string(filepath.Separator), "a", "base") + cases := []struct { + path string + want bool + }{ + {filepath.Join(base, "child"), true}, + {filepath.Join(base, "nested", "deeper"), true}, + {base, true}, + {base + "-other", false}, + {base + "-other" + string(filepath.Separator) + "child", false}, + {filepath.Join(string(filepath.Separator), "a", "elsewhere"), false}, + } + for _, c := range cases { + if got := isUnderDir(c.path, base); got != c.want { + t.Errorf("isUnderDir(%q, %q) = %v, want %v", c.path, base, got, c.want) + } + } +} + +func TestParseWorktreeListTracksLockedState(t *testing.T) { + output := "worktree /a/one\nHEAD abc\nbranch refs/heads/main\n\n" + + "worktree /a/two\nHEAD def\nlocked\n\n" + + "worktree /a/three\nHEAD ghi\nlocked some reason\ndetached\n" + + entries := parseWorktreeList(output) + if len(entries) != 3 { + t.Fatalf("expected 3 entries, got %d: %#v", len(entries), entries) + } + if entries[0].path != canonicalizePath("/a/one") || entries[0].locked { + t.Errorf("entries[0] = %#v", entries[0]) + } + if entries[1].path != canonicalizePath("/a/two") || !entries[1].locked { + t.Errorf("entries[1] = %#v", entries[1]) + } + if entries[2].path != canonicalizePath("/a/three") || !entries[2].locked { + t.Errorf("entries[2] = %#v", entries[2]) + } +} + +func TestCleanUnlocksExpiredLeaseBeforePruningMissingDir(t *testing.T) { + deadPID := deadProcessPID(t) + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + missingPath := filepath.Join(repoDir, "missing-dead-task") + + runner := &fakeRunner{ + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + repoRoot + "\nworktree " + missingPath + "\nlocked " + leaseReason(deadPID) + "\n"}, + {ExitCode: 0}, // unlock + {ExitCode: 0}, // prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + + unlocked := false + pruned := false + for _, call := range runner.calls { + if len(call.args) >= 2 && call.args[0] == "worktree" { + if call.args[1] == "unlock" { + unlocked = true + } + if call.args[1] == "prune" { + pruned = true + } + } + } + if !unlocked || !pruned { + t.Errorf("expected unlock and prune for missing expired lease, unlocked=%v, pruned=%v, calls=%#v", unlocked, pruned, runner.calls) + } +} + +func TestCleanMigratesAndReclaimsLegacyZeroWorktrees(t *testing.T) { + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + legacyPath := filepath.Join(repoDir, "legacy-task") + if err := os.MkdirAll(legacyPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + // Note: plantOwnershipMarker is explicitly NOT called here, simulating a pre-upgrade worktree + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := filepath.WalkDir(legacyPath, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, twoDaysAgo, twoDaysAgo) + }); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + autoAbsoluteGitDir: true, + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + repoRoot + "\nworktree " + legacyPath + "\n"}, + {ExitCode: 0}, // status --porcelain --ignored: clean (legacy gets includeIgnored) + {Stdout: "deadbeef"}, // rev-parse HEAD + {Stdout: "refs/heads/main"}, // for-each-ref --contains (reachable) + {ExitCode: 0}, // worktree remove --force + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + + // Legacy unlocked worktrees must use --ignored: they predate Release, so + // unlocked is not a completion signal. + if got, want := runner.firstStatusCommand(), "git status --porcelain --ignored"; got != want { + t.Fatalf("status call = %q, want %q (legacy dirty probe must count ignored files)", got, want) + } + removed := false + for _, call := range runner.calls { + if len(call.args) >= 2 && call.args[0] == "worktree" && call.args[1] == "remove" { + removed = true + } + } + if !removed { + t.Fatal("Clean did not reclaim a legacy pre-upgrade Zero worktree lacking an ownership marker") + } +} + +// TestCleanSkipsLegacyWorktreeWithIgnoredData: a pre-upgrade unlocked worktree +// with only gitignored contents (.env, node_modules) must not be force-removed. +// Clean used to probe without --ignored first and treated legacy as released. +func TestCleanSkipsLegacyWorktreeWithIgnoredData(t *testing.T) { + tempDir := physicalTestPath(t, t.TempDir()) + baseDir := filepath.Join(tempDir, "zero-worktrees") + repoRoot := filepath.Join(tempDir, "repo") + repoDir := filepath.Join(baseDir, "zero-worktree-"+repoKey(repoRoot)) + + legacyPath := filepath.Join(repoDir, "legacy-task") + if err := os.MkdirAll(legacyPath, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(repoRoot, 0o755); err != nil { + t.Fatal(err) + } + twoDaysAgo := time.Now().Add(-48 * time.Hour) + if err := filepath.WalkDir(legacyPath, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, twoDaysAgo, twoDaysAgo) + }); err != nil { + t.Fatal(err) + } + + runner := &fakeRunner{ + autoAbsoluteGitDir: true, + results: []CommandResult{ + {Stdout: repoRoot}, + {Stdout: "worktree " + repoRoot + "\nworktree " + legacyPath + "\n"}, + {Stdout: "!! .env\n"}, // status --porcelain --ignored: ignored residue + {ExitCode: 0}, // worktree prune + }, + } + + if err := Clean(context.Background(), Options{Cwd: repoRoot, BaseDir: baseDir, RunGit: runner.Run}, 24*time.Hour); err != nil { + t.Fatalf("Clean failed: %v", err) + } + if got, want := runner.firstStatusCommand(), "git status --porcelain --ignored"; got != want { + t.Fatalf("status call = %q, want %q", got, want) + } + for _, call := range runner.calls { + if len(call.args) >= 2 && call.args[0] == "worktree" && call.args[1] == "remove" { + t.Fatalf("Clean must not remove a legacy worktree with ignored data, calls=%#v", runner.calls) + } + } +} diff --git a/internal/worktrees/worktrees_windows.go b/internal/worktrees/worktrees_windows.go new file mode 100644 index 000000000..ae7c90fd7 --- /dev/null +++ b/internal/worktrees/worktrees_windows.go @@ -0,0 +1,40 @@ +//go:build windows + +package worktrees + +import ( + "errors" + + "golang.org/x/sys/windows" +) + +// osProcessAlive reports whether pid is a live process on Windows. It opens +// the process for limited query access; a successful open with a +// non-exited status means the PID is live. OpenProcess succeeding is not +// enough on its own: a handle can remain valid briefly after the process it +// named has exited, so GetExitCodeProcess must confirm STILL_ACTIVE before +// treating the PID as live. +func osProcessAlive(pid int) bool { + handle, err := windows.OpenProcess(windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid)) + if err != nil { + return openProcessErrorMeansAlive(err) + } + defer windows.CloseHandle(handle) + var code uint32 + if err := windows.GetExitCodeProcess(handle, &code); err != nil { + // Could not query; treat the open as proof of life (conservative: do + // not reclaim a lock we are unsure about). + return true + } + const stillActive = 259 // STILL_ACTIVE + return code == stillActive +} + +// openProcessErrorMeansAlive classifies an OpenProcess failure. Only +// ERROR_INVALID_PARAMETER means the PID names no running process (dead). +// Every other error - access denied under another user, handle pressure, +// out-of-memory, and any other ambiguity - fails closed as alive, matching +// processAlive's contract and the POSIX sibling. +func openProcessErrorMeansAlive(err error) bool { + return !errors.Is(err, windows.ERROR_INVALID_PARAMETER) +} diff --git a/internal/worktrees/worktrees_windows_test.go b/internal/worktrees/worktrees_windows_test.go new file mode 100644 index 000000000..6dcb194c0 --- /dev/null +++ b/internal/worktrees/worktrees_windows_test.go @@ -0,0 +1,58 @@ +//go:build windows + +package worktrees + +import ( + "os" + "os/exec" + "testing" + + "golang.org/x/sys/windows" +) + +func TestOpenProcessErrorMeansAliveOnAccessDenied(t *testing.T) { + if !openProcessErrorMeansAlive(windows.ERROR_ACCESS_DENIED) { + t.Fatal("ERROR_ACCESS_DENIED must be treated as alive: the PID could belong to a live process this caller lacks rights to query") + } +} + +func TestOpenProcessErrorMeansAliveOnInvalidParameter(t *testing.T) { + if openProcessErrorMeansAlive(windows.ERROR_INVALID_PARAMETER) { + t.Fatal("ERROR_INVALID_PARAMETER means no such process; must be treated as dead") + } +} + +func TestOpenProcessErrorMeansAliveOnAmbiguousErrors(t *testing.T) { + // Any error other than INVALID_PARAMETER is ambiguity under + // processAlive's fail-closed contract (handle pressure, OOM, etc.). + for _, err := range []error{ + windows.ERROR_ACCESS_DENIED, + windows.ERROR_NOT_ENOUGH_MEMORY, + windows.ERROR_NO_SYSTEM_RESOURCES, + windows.ERROR_TOO_MANY_OPEN_FILES, + } { + if !openProcessErrorMeansAlive(err) { + t.Fatalf("%v must be treated as alive", err) + } + } +} + +func TestOsProcessAliveReportsLiveSelf(t *testing.T) { + if !osProcessAlive(os.Getpid()) { + t.Fatal("current process must report alive") + } +} + +func TestOsProcessAliveReportsDeadAfterExit(t *testing.T) { + cmd := exec.Command("cmd", "/C", "exit", "0") + if err := cmd.Start(); err != nil { + t.Fatalf("start child process: %v", err) + } + pid := cmd.Process.Pid + if err := cmd.Wait(); err != nil { + t.Fatalf("wait for child process: %v", err) + } + if osProcessAlive(pid) { + t.Fatal("exited process must not report alive") + } +} diff --git a/internal/zerocommands/backend_doctor_test.go b/internal/zerocommands/backend_doctor_test.go index 1ea9db56e..845f40be2 100644 --- a/internal/zerocommands/backend_doctor_test.go +++ b/internal/zerocommands/backend_doctor_test.go @@ -11,7 +11,7 @@ import ( ) func TestNewBackendDoctorReportSurfacesDiagnosticsAndActions(t *testing.T) { - secret := "sk-proj-" + strings.Repeat("a", 24) + secret := "sk-proj-" + strings.Repeat("a", 23) + "0" report := NewBackendDoctorReport(BackendDoctorInput{ MCP: config.MCPConfig{Servers: map[string]config.MCPServerConfig{ "remote": { diff --git a/internal/zerocommands/backend_snapshots_test.go b/internal/zerocommands/backend_snapshots_test.go index ff3951ecc..a0be71d87 100644 --- a/internal/zerocommands/backend_snapshots_test.go +++ b/internal/zerocommands/backend_snapshots_test.go @@ -89,7 +89,7 @@ func TestMCPServerSnapshotWithCountsMergesRuntimeCounts(t *testing.T) { } func TestMCPServerSnapshotRedactsSecretCommand(t *testing.T) { - secret := "sk-proj-" + strings.Repeat("d", 24) + secret := "sk-proj-" + strings.Repeat("d", 23) + "0" server := mcp.Server{ Name: "leaky", Type: mcp.ServerTypeStdio, @@ -201,7 +201,7 @@ func TestHookSnapshotFromDefinitionPreservesPositionForRedactedArgs(t *testing.T } func TestHookSnapshotFromDefinitionRedactsSecretCommand(t *testing.T) { - secret := "sk-proj-" + strings.Repeat("c", 24) + secret := "sk-proj-" + strings.Repeat("c", 23) + "0" def := hooks.Definition{ ID: "hook-secret-command", Event: hooks.EventAfterTool, @@ -400,7 +400,7 @@ func TestPluginSnapshotFromPluginCollapsesSlicesToCounts(t *testing.T) { } func TestPluginSnapshotRedactsOperatorFacingStrings(t *testing.T) { - secret := "sk-proj-" + strings.Repeat("e", 24) + secret := "sk-proj-" + strings.Repeat("e", 23) + "0" plugin := plugins.LoadedPlugin{ ID: "plugin-" + secret, Name: "Docs " + secret, diff --git a/internal/zerocommands/contracts_test.go b/internal/zerocommands/contracts_test.go index 3da774376..4e5479f77 100644 --- a/internal/zerocommands/contracts_test.go +++ b/internal/zerocommands/contracts_test.go @@ -52,7 +52,7 @@ func TestConfigSnapshotRedactsProviderURLsAndResolvesAPIModels(t *testing.T) { } func TestConfigSnapshotRedactsProviderWarnings(t *testing.T) { - secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz0" resolved := config.ResolvedConfig{ Providers: []config.ProviderProfile{ { diff --git a/internal/zerogit/contracts_test.go b/internal/zerogit/contracts_test.go index b8aee3981..672c232ee 100644 --- a/internal/zerogit/contracts_test.go +++ b/internal/zerogit/contracts_test.go @@ -6,7 +6,7 @@ import ( ) func TestSnapshotFromSummaryRedactsDiffAndBuildsEvents(t *testing.T) { - secret := "sk-proj-abcdefghijklmnopqrstuvwxyz" + secret := "sk-proj-abcdefghijklmnopqrstuvwxyz0" summary := ChangeSummary{ Root: "/repo/" + secret, Branch: "feature/" + secret, diff --git a/internal/zerogit/zerogit_test.go b/internal/zerogit/zerogit_test.go index cfb479e6e..1e177faaf 100644 --- a/internal/zerogit/zerogit_test.go +++ b/internal/zerogit/zerogit_test.go @@ -23,7 +23,7 @@ func TestInspectSummarizesChangesAndRedactsDiff(t *testing.T) { {}, {}, {Stdout: " internal/verify/verify.go | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n"}, - {Stdout: "diff --git a/internal/verify/verify.go b/internal/verify/verify.go\n+token sk-proj-abcdefghijklmnopqrstuvwxyz\n"}, + {Stdout: "diff --git a/internal/verify/verify.go b/internal/verify/verify.go\n+token sk-proj-abcdefghijklmnopqrstuvwxyz0\n"}, }} summary, err := Inspect(context.Background(), InspectOptions{ @@ -50,7 +50,7 @@ func TestInspectSummarizesChangesAndRedactsDiff(t *testing.T) { if summary.Files[1].Path != "internal/zerogit/zerogit.go" || summary.Files[1].Status != "untracked" || !summary.Files[1].Untracked { t.Fatalf("unexpected untracked file summary: %#v", summary.Files[1]) } - if strings.Contains(summary.Diff, "sk-proj-abcdefghijklmnopqrstuvwxyz") || !strings.Contains(summary.Diff, "[REDACTED]") { + if strings.Contains(summary.Diff, "sk-proj-abcdefghijklmnopqrstuvwxyz0") || !strings.Contains(summary.Diff, "[REDACTED]") { t.Fatalf("expected redacted diff, got %q", summary.Diff) } if !summary.Truncated { @@ -279,8 +279,8 @@ func TestInspectBaseRefUsesThreeDotDiff(t *testing.T) { {Stdout: "feature/m5\n"}, // rev-parse --abbrev-ref HEAD {Stdout: "abc1234\n"}, // rev-parse --short HEAD {Stdout: "M\ta.txt\nA\tb.txt\n"}, // diff --name-status main...HEAD - {Stdout: " a.txt | 1 +\n b.txt | 1 +\n 2 files changed, 2 insertions(+)\n"}, // diff --stat main...HEAD - {Stdout: "diff --git a/internal/changes/changes.go b/internal/changes/changes.go\n+token sk-proj-abcdefghijklmnopqrstuvwxyz\n"}, // diff main...HEAD + {Stdout: " a.txt | 1 +\n b.txt | 1 +\n 2 files changed, 2 insertions(+)\n"}, // diff --stat main...HEAD + {Stdout: "diff --git a/internal/changes/changes.go b/internal/changes/changes.go\n+token sk-proj-abcdefghijklmnopqrstuvwxyz0\n"}, // diff main...HEAD }} summary, err := Inspect(context.Background(), InspectOptions{ @@ -311,7 +311,7 @@ func TestInspectBaseRefUsesThreeDotDiff(t *testing.T) { if summary.Files[1].Path != "b.txt" || summary.Files[1].Status != "added" { t.Fatalf("unexpected second file: %#v", summary.Files[1]) } - if strings.Contains(summary.Diff, "sk-proj-abcdefghijklmnopqrstuvwxyz") || !strings.Contains(summary.Diff, "[REDACTED]") { + if strings.Contains(summary.Diff, "sk-proj-abcdefghijklmnopqrstuvwxyz0") || !strings.Contains(summary.Diff, "[REDACTED]") { t.Fatalf("expected redacted diff, got %q", summary.Diff) } if !summary.Truncated {