diff --git a/docs/checkout-native-review-contract.md b/docs/checkout-native-review-contract.md index e24bf93..975ca38 100644 --- a/docs/checkout-native-review-contract.md +++ b/docs/checkout-native-review-contract.md @@ -69,6 +69,23 @@ Notes: The workbench is run-owned, not cache-owned. Shared clone or fetch caches are a possible future optimization but are not part of the correctness contract. +`workbench/metadata.json` is a versioned durable artifact. Schema version `1` +records: + +- `schema_version` +- `source_repo_root` +- `checkout_mode` +- normalized PR identity under `pr` +- pinned base and head refs +- `repo_path` and `scratch_path` +- sorted changed file paths +- `fingerprint_inputs`, which restates the semantic inputs downstream resumable + tasks use to detect stale workbench state + +`fingerprint_inputs` is intentionally redundant with the top-level metadata so +downstream tasks can fingerprint workbench semantics without re-deriving them +from the checkout tree ad hoc. + `SelectionOnly` and benchmark callers still own their artifact directory choice. Checkout-native additions must work with caller-owned artifact roots instead of assuming that every selector run is ledger-backed. diff --git a/internal/cmd/noleak/noleak_test.go b/internal/cmd/noleak/noleak_test.go index f938fc0..e957d48 100644 --- a/internal/cmd/noleak/noleak_test.go +++ b/internal/cmd/noleak/noleak_test.go @@ -13,6 +13,7 @@ import ( "net/http" "net/http/httptest" "os" + "os/exec" "path/filepath" "strings" "sync" @@ -283,21 +284,22 @@ func TestCommandSurfacesDoNotLeakSeededSecrets(t *testing.T) { } type auditHarness struct { - t *testing.T - configPath string - configRoot string - layout statepaths.Layout - agentDir string - githubURL string - graphQLURL string - llmURL string - prRef gitprovider.PRRef - prURL string - prKey string - headSHA string - baseSHA string - now time.Time - llmCalls atomic.Int32 + t *testing.T + configPath string + configRoot string + layout statepaths.Layout + agentDir string + githubURL string + graphQLURL string + llmURL string + prRef gitprovider.PRRef + prURL string + prKey string + headSHA string + baseSHA string + workbenchRepoDir string + now time.Time + llmCalls atomic.Int32 gitSecret string reviewerSecret string @@ -327,14 +329,17 @@ func newAuditHarness(t *testing.T) *auditHarness { if err != nil { t.Fatalf("DefaultLayoutEnsured: %v", err) } + workbench := newNoLeakWorkbenchFixture(t) + baseSHA, headSHA := workbench.baseSHA, workbench.headSHA h := &auditHarness{ // #nosec G101 -- these are distinctive test canaries, not real credentials. t: t, configPath: configPath, configRoot: filepath.Dir(configPath), layout: layout, agentDir: filepath.Join(rootDir, "agents"), - headSHA: strings.Repeat("a", 40), - baseSHA: strings.Repeat("b", 40), + headSHA: headSHA, + baseSHA: baseSHA, + workbenchRepoDir: workbench.repoDir, now: time.Date(2026, 6, 2, 12, 0, 0, 0, time.UTC), gitSecret: "cr-noleak-git-token-0001", reviewerSecret: "cr-noleak-reviewer-token-0002", @@ -376,6 +381,74 @@ func newAuditHarness(t *testing.T) *auditHarness { return h } +type noLeakWorkbenchFixture struct { + repoDir string + baseSHA string + headSHA string +} + +func newNoLeakWorkbenchFixture(t *testing.T) noLeakWorkbenchFixture { + t.Helper() + repoDir := t.TempDir() + noLeakGitMustSucceed(t, repoDir, "init", "-b", "main") + noLeakGitMustSucceed(t, repoDir, "config", "user.name", "NoLeak Test") + noLeakGitMustSucceed(t, repoDir, "config", "user.email", "noleak@example.com") + noLeakGitMustSucceed(t, repoDir, "remote", "add", "origin", "git@github.com:open-cli-collective/codereview-cli.git") + if err := os.WriteFile(filepath.Join(repoDir, "main.go"), []byte("package main\n\nvar changed = false\n"), 0o600); err != nil { + t.Fatalf("write main.go: %v", err) + } + noLeakGitMustSucceed(t, repoDir, "add", "main.go") + noLeakGitMustSucceed(t, repoDir, "commit", "-m", "base") + baseSHA := strings.TrimSpace(noLeakGitOutput(t, repoDir, "rev-parse", "HEAD")) + noLeakGitMustSucceed(t, repoDir, "checkout", "-b", "feature") + if err := os.WriteFile(filepath.Join(repoDir, "main.go"), []byte("package main\n\nvar changed = true\n"), 0o600); err != nil { + t.Fatalf("update main.go: %v", err) + } + noLeakGitMustSucceed(t, repoDir, "commit", "-am", "head") + headSHA := strings.TrimSpace(noLeakGitOutput(t, repoDir, "rev-parse", "HEAD")) + return noLeakWorkbenchFixture{repoDir: repoDir, baseSHA: baseSHA, headSHA: headSHA} +} + +func noLeakGitMustSucceed(t *testing.T, dir string, args ...string) string { + t.Helper() + return strings.TrimSpace(noLeakGitOutput(t, dir, args...)) +} + +func noLeakGitOutput(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return string(out) +} + +func noLeakGitCommand(ref gitprovider.PRRef) func(context.Context, string, ...string) ([]byte, error) { + return func(ctx context.Context, dir string, args ...string) ([]byte, error) { + if len(args) == 3 && args[0] == "remote" && args[1] == "get-url" && args[2] == "origin" { + return []byte(fmt.Sprintf("https://%s/%s/%s.git\n", ref.Host, ref.Owner, ref.Repo)), nil + } + cmd := exec.CommandContext(ctx, "git", args...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + if strings.TrimSpace(dir) != "" { + cmd.Dir = dir + } + out, err := cmd.CombinedOutput() + if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + message := strings.TrimSpace(string(out)) + if message == "" { + message = err.Error() + } + return nil, fmt.Errorf("git %s: %s", strings.Join(args, " "), message) + } + return out, nil + } +} + func (h *auditHarness) run(args []string) (string, string, error) { var stdout, stderr bytes.Buffer args = auditArgsWithExplicitProfile(args) @@ -648,17 +721,20 @@ func (h *auditHarness) reviewRuntimeFactory(cmd *cobra.Command, opts *root.Optio _ = store.Close() } pipelineOpts := pipeline.Options{ - Provider: provider, - Adapter: adapter, - Store: ledgerStore, - NamedSessions: ledgerStore, - Layout: h.layout, - Warnings: opts.Stderr, - Now: func() time.Time { return h.now }, - Retention: runtimeOpts.Retention, - RetentionManualOnly: runtimeOpts.RetentionManualOnly, - MaxAgents: runtimeOpts.MaxAgents, - MaxConcurrency: runtimeOpts.MaxConcurrency, + Provider: provider, + Adapter: adapter, + Store: ledgerStore, + NamedSessions: ledgerStore, + Layout: h.layout, + Warnings: opts.Stderr, + Now: func() time.Time { return h.now }, + Retention: runtimeOpts.Retention, + RetentionManualOnly: runtimeOpts.RetentionManualOnly, + MaxAgents: runtimeOpts.MaxAgents, + MaxConcurrency: runtimeOpts.MaxConcurrency, + AutoUnlockWorkbenchOnExit: true, + GitCommand: noLeakGitCommand(h.prRef), + ResolveRepoRoot: func(context.Context) (string, error) { return h.workbenchRepoDir, nil }, } runner := realReviewRunner{ pipeline: pipelineOpts, @@ -1057,11 +1133,15 @@ func (h *auditHarness) assertOwnedFilesDoNotLeak(t *testing.T) { } func shouldSkipOwnedPath(path string, entry os.DirEntry) bool { + clean := filepath.Clean(path) base := filepath.Base(path) if base == "keyring" { return true } - if strings.Contains(filepath.Clean(path), string(filepath.Separator)+"keyring"+string(filepath.Separator)) { + if strings.Contains(clean, string(filepath.Separator)+"keyring"+string(filepath.Separator)) { + return true + } + if strings.Contains(clean, string(filepath.Separator)+"workbench"+string(filepath.Separator)+"repo") { return true } if entry.Type()&os.ModeSymlink != 0 { diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index ce238db..9f8d96c 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -78,11 +78,14 @@ type Runtime struct { // RuntimeOptions carries command flags that affect runtime construction. type RuntimeOptions struct { - MaxAgents int - MaxConcurrency int - PRRef gitprovider.PRRef - Retention datalifecycle.RetentionPolicy - RetentionManualOnly bool + MaxAgents int + MaxConcurrency int + PRRef gitprovider.PRRef + Retention datalifecycle.RetentionPolicy + RetentionManualOnly bool + AutoUnlockWorkbenchOnExit bool + ResolveRepoRoot func(context.Context) (string, error) + GitCommand func(context.Context, string, ...string) ([]byte, error) } // RuntimeFactory builds the concrete runtime used by `cr review`. @@ -858,17 +861,20 @@ func runtimeLayout() (statepaths.Layout, error) { func buildReviewRunner(ledgerStore *ledger.Store, provider gitprovider.GitProvider, adapter llm.Adapter, profile config.Profile, limiter outbox.Limiter, layout statepaths.Layout, warnings io.Writer, logger *progress.Logger, runtimeOpts RuntimeOptions) reviewRunner { pipelineOpts := pipeline.Options{ - Provider: provider, - Adapter: adapter, - Store: ledgerStore, - NamedSessions: ledgerStore, - Layout: layout, - Warnings: warnings, - TaskProgress: newPipelineTaskProgress(logger), - MaxAgents: runtimeOpts.MaxAgents, - MaxConcurrency: runtimeOpts.MaxConcurrency, - Retention: runtimeOpts.Retention, - RetentionManualOnly: runtimeOpts.RetentionManualOnly, + Provider: provider, + Adapter: adapter, + Store: ledgerStore, + NamedSessions: ledgerStore, + Layout: layout, + Warnings: warnings, + TaskProgress: newPipelineTaskProgress(logger), + MaxAgents: runtimeOpts.MaxAgents, + MaxConcurrency: runtimeOpts.MaxConcurrency, + Retention: runtimeOpts.Retention, + RetentionManualOnly: runtimeOpts.RetentionManualOnly, + AutoUnlockWorkbenchOnExit: runtimeOpts.AutoUnlockWorkbenchOnExit, + ResolveRepoRoot: runtimeOpts.ResolveRepoRoot, + GitCommand: runtimeOpts.GitCommand, } return reviewRunner{ pipeline: pipelineOpts, diff --git a/internal/cmd/reviewcmd/reviewcmd_test.go b/internal/cmd/reviewcmd/reviewcmd_test.go index b524e20..63a1e71 100644 --- a/internal/cmd/reviewcmd/reviewcmd_test.go +++ b/internal/cmd/reviewcmd/reviewcmd_test.go @@ -8,9 +8,11 @@ import ( "fmt" "io" "os" + "os/exec" "path/filepath" "strconv" "strings" + "sync" "testing" "time" @@ -746,7 +748,7 @@ func TestNewRuntimeLiveApprovedFastPathDoesNotInitializeAdapter(t *testing.T) { statedirtest.Hermetic(t) cfg := testConfig() profile := cfg.Profiles["home"] - ref, pr := reviewCommandPR() + ref, pr := reviewCommandPR(t) provider := &gitprovider.Fake{} if err := provider.SetPR(ref, pr); err != nil { t.Fatalf("SetPR: %v", err) @@ -1288,7 +1290,7 @@ func TestBuildReviewRunnerWiresNamedSessionDependencies(t *testing.T) { statepaths.NewLayout(t.TempDir(), t.TempDir()), &warnings, nil, - RuntimeOptions{MaxAgents: 3, MaxConcurrency: 2, Retention: retention, RetentionManualOnly: true}, + runtimeOptsWithWorkbench(t, RuntimeOptions{MaxAgents: 3, MaxConcurrency: 2, Retention: retention, RetentionManualOnly: true}), ) if runner.pipeline.NamedSessions != store { @@ -1407,7 +1409,7 @@ func TestReviewLiveRealRunnerHonorsConfiguredRetention(t *testing.T) { MaxAgeDays: &maxAgeDays, Enforcement: config.RetentionAtWrite, } - ref, pr := reviewCommandPR() + ref, pr := reviewCommandPR(t) provider := &gitprovider.Fake{} if err := provider.SetPR(ref, pr); err != nil { t.Fatalf("SetPR: %v", err) @@ -1446,7 +1448,7 @@ func TestReviewLiveRealRunnerHonorsConfiguredRetention(t *testing.T) { layout, opts.Stderr, nil, - runtimeOpts, + runtimeOptsWithWorkbench(t, runtimeOpts), ) return Runtime{Runner: runner, PostingIdentity: gitprovider.Identity{Login: "review-bot", ID: "bot-id"}}, nil }) @@ -1470,31 +1472,9 @@ func TestReviewLiveSessionThroughRealRunnerPersistsNamedSession(t *testing.T) { profile.AgentSources = []string{agentDir} cfg.Profiles["home"] = profile - ref := gitprovider.PRRef{Host: "github.com", Owner: "open-cli-collective", Repo: "codereview-cli", Number: 29} - baseSHA := strings.Repeat("b", 40) - headSHA := strings.Repeat("a", 40) - pr := gitprovider.PR{ - Ref: ref, - URL: "https://github.com/open-cli-collective/codereview-cli/pull/29", - State: gitprovider.PRStateOpen, - Author: gitprovider.Identity{Login: "author", ID: "author-id"}, - Base: gitprovider.PRBranchRef{ - Host: ref.Host, - Owner: ref.Owner, - Repo: ref.Repo, - Name: "main", - Ref: "refs/heads/main", - SHA: baseSHA, - }, - Head: gitprovider.PRBranchRef{ - Host: ref.Host, - Owner: ref.Owner, - Repo: ref.Repo, - Name: "feature", - Ref: "refs/heads/feature", - SHA: headSHA, - }, - } + fixture := newReviewCommandWorkbenchFixture(t) + reviewCommandFixtures.Store(reviewCommandFixtureKey(fixture.ref), fixture) + ref, pr := fixture.ref, fixture.pr provider := &gitprovider.Fake{} if err := provider.SetPR(ref, pr); err != nil { t.Fatalf("SetPR: %v", err) @@ -1529,7 +1509,7 @@ func TestReviewLiveSessionThroughRealRunnerPersistsNamedSession(t *testing.T) { statepaths.NewLayout(t.TempDir(), t.TempDir()), opts.Stderr, nil, - runtimeOpts, + runtimeOptsWithWorkbench(t, runtimeOpts), ) return Runtime{Runner: runner, PostingIdentity: gitprovider.Identity{Login: "review-bot", ID: "bot-id"}}, nil }) @@ -1562,7 +1542,7 @@ func TestReviewDryRunRealRunnerHonorsConfiguredRetention(t *testing.T) { MaxAgeDays: &maxAgeDays, Enforcement: config.RetentionAtWrite, } - ref, pr := reviewCommandPR() + ref, pr := reviewCommandPR(t) provider := &gitprovider.Fake{} if err := provider.SetPR(ref, pr); err != nil { t.Fatalf("SetPR: %v", err) @@ -1602,7 +1582,7 @@ func TestReviewDryRunRealRunnerHonorsConfiguredRetention(t *testing.T) { layout, opts.Stderr, nil, - runtimeOpts, + runtimeOptsWithWorkbench(t, runtimeOpts), ) return Runtime{Runner: runner, PostingIdentity: gitprovider.Identity{Login: "review-bot", ID: "bot-id"}}, nil }) @@ -1633,7 +1613,7 @@ func TestReviewDryRunRealRunnerHonorsConfiguredKeepLiveForever(t *testing.T) { MaxAgeDays: &maxAgeDays, Enforcement: config.RetentionAtWrite, } - ref, pr := reviewCommandPR() + ref, pr := reviewCommandPR(t) provider := &gitprovider.Fake{} if err := provider.SetPR(ref, pr); err != nil { t.Fatalf("SetPR: %v", err) @@ -1672,7 +1652,7 @@ func TestReviewDryRunRealRunnerHonorsConfiguredKeepLiveForever(t *testing.T) { layout, opts.Stderr, nil, - runtimeOpts, + runtimeOptsWithWorkbench(t, runtimeOpts), ) return Runtime{Runner: runner, PostingIdentity: gitprovider.Identity{Login: "review-bot", ID: "bot-id"}}, nil }) @@ -1696,7 +1676,7 @@ func TestReviewDryRunRealRunnerHonorsManualOnlyRetention(t *testing.T) { profile.AgentSources = []string{agentDir} cfg.Profiles["home"] = profile cfg.Data.Retention = config.RetentionConfig{Enforcement: config.RetentionManualOnly} - ref, pr := reviewCommandPR() + ref, pr := reviewCommandPR(t) provider := &gitprovider.Fake{} if err := provider.SetPR(ref, pr); err != nil { t.Fatalf("SetPR: %v", err) @@ -1735,7 +1715,7 @@ func TestReviewDryRunRealRunnerHonorsManualOnlyRetention(t *testing.T) { layout, opts.Stderr, nil, - runtimeOpts, + runtimeOptsWithWorkbench(t, runtimeOpts), ) return Runtime{Runner: runner, PostingIdentity: gitprovider.Identity{Login: "review-bot", ID: "bot-id"}}, nil }) @@ -1886,7 +1866,7 @@ func TestReviewQuietSuppressesProgressOnlyForTextOutput(t *testing.T) { func TestReviewDryRunRealRunnerQuietSuppressesProgressOnly(t *testing.T) { cfg := testConfig() - ref, pr := reviewCommandPR() + ref, pr := reviewCommandPR(t) provider := &gitprovider.Fake{} if err := provider.SetPR(ref, pr); err != nil { t.Fatalf("SetPR: %v", err) @@ -1922,7 +1902,7 @@ func TestReviewDryRunRealRunnerQuietSuppressesProgressOnly(t *testing.T) { statepaths.NewLayout(t.TempDir(), t.TempDir()), opts.Stderr, logger, - runtimeOpts, + runtimeOptsWithWorkbench(t, runtimeOpts), ) return Runtime{Runner: runner, PostingIdentity: gitprovider.Identity{Login: "review-bot", ID: "bot-id"}}, nil }, true) @@ -1944,7 +1924,7 @@ func TestReviewDryRunRealRunnerQuietSuppressesProgressOnly(t *testing.T) { func TestReviewDryRunRealRunnerWritesGitHubProgressToStderr(t *testing.T) { cfg := testConfig() - ref, pr := reviewCommandPR() + ref, pr := reviewCommandPR(t) provider := &gitprovider.Fake{} if err := provider.SetPR(ref, pr); err != nil { t.Fatalf("SetPR: %v", err) @@ -1980,7 +1960,7 @@ func TestReviewDryRunRealRunnerWritesGitHubProgressToStderr(t *testing.T) { statepaths.NewLayout(t.TempDir(), t.TempDir()), opts.Stderr, logger, - runtimeOpts, + runtimeOptsWithWorkbench(t, runtimeOpts), ) return Runtime{Runner: runner, PostingIdentity: gitprovider.Identity{Login: "review-bot", ID: "bot-id"}}, nil }, false) @@ -2122,7 +2102,7 @@ func TestProgressPlannerWritesRunIDBreadcrumb(t *testing.T) { } }) provider := &gitprovider.Fake{} - ref, pr := reviewCommandPR() + ref, pr := reviewCommandPR(t) if err := provider.SetPR(ref, pr); err != nil { t.Fatalf("SetPR: %v", err) } @@ -2147,7 +2127,7 @@ func TestProgressPlannerWritesRunIDBreadcrumb(t *testing.T) { statepaths.NewLayout(t.TempDir(), t.TempDir()), &errOut, logger, - RuntimeOptions{}, + runtimeOptsWithWorkbench(t, RuntimeOptions{}), ) run, err := store.AllocateRun(context.Background(), ledger.AllocateRunParams{ @@ -2670,34 +2650,132 @@ func approvalOverrideClassifierTestRequest() approvaloverride.Request { } } -func reviewCommandPR() (gitprovider.PRRef, gitprovider.PR) { +type reviewCommandWorkbenchFixture struct { + repoDir string + ref gitprovider.PRRef + pr gitprovider.PR +} + +var reviewCommandFixtures sync.Map + +func reviewCommandPR(t *testing.T) (gitprovider.PRRef, gitprovider.PR) { + t.Helper() + fixture := newReviewCommandWorkbenchFixture(t) + reviewCommandFixtures.Store(reviewCommandFixtureKey(fixture.ref), fixture) + return fixture.ref, fixture.pr +} + +func reviewCommandFixtureKey(ref gitprovider.PRRef) string { + return fmt.Sprintf("%s/%s/%s/%d", ref.Host, ref.Owner, ref.Repo, ref.Number) +} + +func newReviewCommandWorkbenchFixture(t *testing.T) reviewCommandWorkbenchFixture { + t.Helper() ref := gitprovider.PRRef{Host: "github.com", Owner: "open-cli-collective", Repo: "codereview-cli", Number: 29} - baseSHA := strings.Repeat("b", 40) - headSHA := strings.Repeat("a", 40) - return ref, gitprovider.PR{ - Ref: ref, - URL: "https://github.com/open-cli-collective/codereview-cli/pull/29", - State: gitprovider.PRStateOpen, - Author: gitprovider.Identity{Login: "author", ID: "author-id"}, - Base: gitprovider.PRBranchRef{ - Host: ref.Host, - Owner: ref.Owner, - Repo: ref.Repo, - Name: "main", - Ref: "refs/heads/main", - SHA: baseSHA, - }, - Head: gitprovider.PRBranchRef{ - Host: ref.Host, - Owner: ref.Owner, - Repo: ref.Repo, - Name: "feature", - Ref: "refs/heads/feature", - SHA: headSHA, + repoDir := t.TempDir() + reviewCommandGitMustSucceed(t, repoDir, "init", "-b", "main") + reviewCommandGitMustSucceed(t, repoDir, "config", "user.name", "ReviewCmd Test") + reviewCommandGitMustSucceed(t, repoDir, "config", "user.email", "reviewcmd@example.com") + reviewCommandGitMustSucceed(t, repoDir, "remote", "add", "origin", "git@github.com:open-cli-collective/codereview-cli.git") + writeReviewCommandFile(t, filepath.Join(repoDir, "main.go"), "package main\n\nvar changed = false\n") + reviewCommandGitMustSucceed(t, repoDir, "add", "main.go") + reviewCommandGitMustSucceed(t, repoDir, "commit", "-m", "base") + baseSHA := strings.TrimSpace(reviewCommandGitOutput(t, repoDir, "rev-parse", "HEAD")) + reviewCommandGitMustSucceed(t, repoDir, "checkout", "-b", "feature") + writeReviewCommandFile(t, filepath.Join(repoDir, "main.go"), "package main\n\nvar changed = true\n") + reviewCommandGitMustSucceed(t, repoDir, "commit", "-am", "head") + headSHA := strings.TrimSpace(reviewCommandGitOutput(t, repoDir, "rev-parse", "HEAD")) + return reviewCommandWorkbenchFixture{ + repoDir: repoDir, + ref: ref, + pr: gitprovider.PR{ + Ref: ref, + URL: "https://github.com/open-cli-collective/codereview-cli/pull/29", + State: gitprovider.PRStateOpen, + Author: gitprovider.Identity{Login: "author", ID: "author-id"}, + Base: gitprovider.PRBranchRef{ + Host: ref.Host, + Owner: ref.Owner, + Repo: ref.Repo, + Name: "main", + Ref: "refs/heads/main", + SHA: baseSHA, + }, + Head: gitprovider.PRBranchRef{ + Host: ref.Host, + Owner: ref.Owner, + Repo: ref.Repo, + Name: "feature", + Ref: "refs/heads/feature", + SHA: headSHA, + }, }, } } +func reviewCommandGitMustSucceed(t *testing.T, dir string, args ...string) string { + t.Helper() + return strings.TrimSpace(reviewCommandGitOutput(t, dir, args...)) +} + +func reviewCommandGitOutput(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return string(out) +} + +func reviewCommandGitCommand(ref gitprovider.PRRef) func(context.Context, string, ...string) ([]byte, error) { + return func(ctx context.Context, dir string, args ...string) ([]byte, error) { + if len(args) == 3 && args[0] == "remote" && args[1] == "get-url" && args[2] == "origin" { + return []byte(fmt.Sprintf("https://%s/%s/%s.git\n", ref.Host, ref.Owner, ref.Repo)), nil + } + cmd := exec.CommandContext(ctx, "git", args...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + if strings.TrimSpace(dir) != "" { + cmd.Dir = dir + } + out, err := cmd.CombinedOutput() + if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + message := strings.TrimSpace(string(out)) + if message == "" { + message = err.Error() + } + return nil, fmt.Errorf("git %s: %s", strings.Join(args, " "), message) + } + return out, nil + } +} + +func writeReviewCommandFile(t *testing.T, path, contents string) { + t.Helper() + if err := os.WriteFile(path, []byte(contents), 0o600); err != nil { + t.Fatalf("WriteFile(%s): %v", path, err) + } +} + +func runtimeOptsWithWorkbench(t *testing.T, opts RuntimeOptions) RuntimeOptions { + t.Helper() + opts.AutoUnlockWorkbenchOnExit = true + fixture := newReviewCommandWorkbenchFixture(t) + if opts.PRRef != (gitprovider.PRRef{}) { + if cached, ok := reviewCommandFixtures.Load(reviewCommandFixtureKey(opts.PRRef)); ok { + fixture = cached.(reviewCommandWorkbenchFixture) + } + } + opts.ResolveRepoRoot = func(context.Context) (string, error) { + return fixture.repoDir, nil + } + opts.GitCommand = reviewCommandGitCommand(fixture.ref) + return opts +} + func allocateReviewCommandRun(t *testing.T, store *ledger.Store, layout statepaths.Layout, runID string, mode ledger.PostMode, started time.Time) ledger.Run { t.Helper() prKey := "github_open-cli-collective_codereview-cli_29" @@ -2706,17 +2784,20 @@ func allocateReviewCommandRun(t *testing.T, store *ledger.Store, layout statepat func allocateReviewCommandRunForPRKey(t *testing.T, store *ledger.Store, layout statepaths.Layout, runID, prKey string, mode ledger.PostMode, started time.Time) ledger.Run { t.Helper() + fixture := newReviewCommandWorkbenchFixture(t) + baseSHA := fixture.pr.Base.SHA + headSHA := fixture.pr.Head.SHA run, err := store.AllocateRun(context.Background(), ledger.AllocateRunParams{ PRKey: prKey, PRURL: "https://github.com/open-cli-collective/codereview-cli/pull/29", RunID: runID, - SHA: strings.Repeat("a", 40), - BaseSHA: strings.Repeat("b", 40), + SHA: headSHA, + BaseSHA: baseSHA, Profile: "home", PostingIdentity: "review-bot", PostMode: mode, StartedAt: started, - ArtifactPath: filepath.Join(layout.DataRoot, "runs", prKey, strings.Repeat("a", 40), strings.Repeat("b", 40), "home__review-bot", runID), + ArtifactPath: filepath.Join(layout.DataRoot, "runs", prKey, headSHA, baseSHA, "home__review-bot", runID), }) if err != nil { t.Fatalf("AllocateRun: %v", err) diff --git a/internal/datalifecycle/datalifecycle.go b/internal/datalifecycle/datalifecycle.go index c61f05f..0d45b66 100644 --- a/internal/datalifecycle/datalifecycle.go +++ b/internal/datalifecycle/datalifecycle.go @@ -599,5 +599,49 @@ func (opts Options) removeAll() RemoveAllFunc { if opts.RemoveAll != nil { return opts.RemoveAll } - return os.RemoveAll + return removeAllWritable +} + +func removeAllWritable(path string) error { + if err := makeTreeWritable(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + return os.RemoveAll(path) +} + +func makeTreeWritable(root string) error { + err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, walkErr error) error { + if errors.Is(walkErr, fs.ErrNotExist) { + return nil + } + if walkErr != nil { + return walkErr + } + if entry.Type()&os.ModeSymlink != 0 { + return nil + } + info, err := entry.Info() + if err != nil { + return err + } + mode := info.Mode().Perm() + if entry.IsDir() { + mode |= 0o700 + mode |= 0o055 + } else { + mode |= 0o200 + mode |= 0o444 + } + if err := os.Chmod(path, mode); err != nil { // #nosec G122 -- artifact paths are validated run-owned trees; symlinks are skipped during the walk. + return err + } + return nil + }) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("datalifecycle: chmod tree %s writable: %w", root, err) + } + return nil } diff --git a/internal/datalifecycle/datalifecycle_test.go b/internal/datalifecycle/datalifecycle_test.go index 2e308b5..717b76b 100644 --- a/internal/datalifecycle/datalifecycle_test.go +++ b/internal/datalifecycle/datalifecycle_test.go @@ -154,6 +154,35 @@ func TestPruneDeletesRowBeforeBestEffortArtifactRemoval(t *testing.T) { } } +func TestDefaultRemoveAllUnlocksReadOnlyArtifactTree(t *testing.T) { + root := filepath.Join(t.TempDir(), "artifacts") + workbenchRepo := filepath.Join(root, "workbench", "repo") + if err := os.MkdirAll(filepath.Join(workbenchRepo, "nested"), 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + writeFile(t, filepath.Join(workbenchRepo, "main.go"), "package main\n") + writeFile(t, filepath.Join(workbenchRepo, "nested", "file.txt"), "x") + if err := os.Chmod(workbenchRepo, 0o555); err != nil { // #nosec G302 -- test fixture intentionally simulates a read-only workbench tree. + t.Fatalf("Chmod repo dir: %v", err) + } + if err := os.Chmod(filepath.Join(workbenchRepo, "nested"), 0o555); err != nil { // #nosec G302 -- test fixture intentionally simulates a read-only workbench tree. + t.Fatalf("Chmod nested dir: %v", err) + } + if err := os.Chmod(filepath.Join(workbenchRepo, "main.go"), 0o444); err != nil { // #nosec G302 -- test fixture intentionally simulates a read-only workbench tree. + t.Fatalf("Chmod main.go: %v", err) + } + if err := os.Chmod(filepath.Join(workbenchRepo, "nested", "file.txt"), 0o444); err != nil { // #nosec G302 -- test fixture intentionally simulates a read-only workbench tree. + t.Fatalf("Chmod nested file: %v", err) + } + + if err := (Options{}).removeAll()(root); err != nil { + t.Fatalf("removeAll writable: %v", err) + } + if _, err := os.Stat(root); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("artifact root stat error = %v, want not exist", err) + } +} + func TestPruneSkipsUnsafeArtifactPathsAfterDeletingRows(t *testing.T) { layout := testLayout(t) now := testNow() diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index a69070e..a596b1d 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -10,7 +10,9 @@ import ( "fmt" "io" "io/fs" + "net/url" "os" + "os/exec" "path/filepath" "regexp" "sort" @@ -138,6 +140,10 @@ type Options struct { Retention datalifecycle.RetentionPolicy RetentionManualOnly bool + + GitCommand func(context.Context, string, ...string) ([]byte, error) + ResolveRepoRoot func(context.Context) (string, error) + AutoUnlockWorkbenchOnExit bool } // Request identifies one dry-run review. @@ -197,6 +203,9 @@ type ArtifactPaths struct { AgentLogsDir string `json:"agent_logs_dir"` LLMTasksDir string `json:"llm_tasks_dir"` DossierDir string `json:"dossier_dir"` + WorkbenchDir string `json:"workbench_dir"` + WorkbenchRepoDir string `json:"workbench_repo_dir"` + WorkbenchScratch string `json:"workbench_scratch_dir"` } // SlicePatch returns the artifact path for an agent/file diff slice. @@ -277,6 +286,11 @@ func (p ArtifactPaths) DossierIndexPath() string { return filepath.Join(p.DossierDir, "index.json") } +// WorkbenchMetadataPath returns the workbench metadata artifact path. +func (p ArtifactPaths) WorkbenchMetadataPath() string { + return filepath.Join(p.WorkbenchDir, "metadata.json") +} + func dossierChildPath(dir, name string) (string, error) { name = strings.TrimSpace(name) if name == "" { @@ -571,6 +585,19 @@ func SelectionOnly(ctx context.Context, opts Options, req SelectionRequest) (Sel } result := prepared.selectionResult() + if err := prepareWorkbenchArtifacts(ctx, opts, workbenchPreparationRequest{ + PRRef: req.PRRef, + ReviewPR: prepared.reviewPR, + ChangedFiles: prepared.changedFiles, + Artifacts: prepared.artifacts, + }); err != nil { + return SelectionResult{}, err + } + if shouldAutoUnlockWorkbenchOnExit(opts) { + defer func() { + _ = unlockWorkbenchRepo(prepared.artifacts.WorkbenchRepoDir) + }() + } if err := prepareDossierArtifacts(ctx, opts, dossierPreparationRequest{ Profile: req.Profile, SelectionModelOverride: req.SelectionModelOverride, @@ -678,6 +705,19 @@ func execute(ctx context.Context, opts Options, req Request, mode executionMode) } result.Run = run + if err := prepareWorkbenchArtifacts(ctx, opts, workbenchPreparationRequest{ + PRRef: req.PRRef, + ReviewPR: prepared.reviewPR, + ChangedFiles: prepared.changedFiles, + Artifacts: prepared.artifacts, + }); err != nil { + return Result{}, err + } + if shouldAutoUnlockWorkbenchOnExit(opts) { + defer func() { + _ = unlockWorkbenchRepo(prepared.artifacts.WorkbenchRepoDir) + }() + } if err := prepareDossierArtifacts(ctx, opts, dossierPreparationRequest{ RunID: run.RunID, Profile: req.Profile, @@ -2795,6 +2835,51 @@ type dossierPreparationRequest struct { Artifacts ArtifactPaths } +type workbenchPreparationRequest struct { + PRRef gitprovider.PRRef + ReviewPR gitprovider.PR + ChangedFiles []string + Artifacts ArtifactPaths +} + +type workbenchMetadataArtifact struct { + SchemaVersion int `json:"schema_version"` + SourceRepoRoot string `json:"source_repo_root"` + CheckoutMode string `json:"checkout_mode"` + PR workbenchPRIdentity `json:"pr"` + Base workbenchBranchArtifact `json:"base"` + Head workbenchBranchArtifact `json:"head"` + RepoPath string `json:"repo_path"` + ScratchPath string `json:"scratch_path"` + ChangedFiles []string `json:"changed_files,omitempty"` + FingerprintInputs workbenchFingerprintInputs `json:"fingerprint_inputs"` +} + +type workbenchPRIdentity struct { + Host string `json:"host"` + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` +} + +type workbenchBranchArtifact struct { + Host string `json:"host,omitempty"` + Owner string `json:"owner,omitempty"` + Repo string `json:"repo,omitempty"` + Name string `json:"name,omitempty"` + Ref string `json:"ref,omitempty"` + SHA string `json:"sha"` +} + +type workbenchFingerprintInputs struct { + PR workbenchPRIdentity `json:"pr"` + BaseSHA string `json:"base_sha"` + HeadSHA string `json:"head_sha"` + CheckoutMode string `json:"checkout_mode"` + ChangedFiles []string `json:"changed_files,omitempty"` + SourceRepoRoot string `json:"source_repo_root"` +} + type dossierIndexArtifact struct { HashAlgorithm string `json:"hash_algorithm"` Files []dossierIndexFileArtifact `json:"files"` @@ -2806,16 +2891,18 @@ type dossierIndexFileArtifact struct { } const ( - dossierFinalMaxTopLevelComments = 20 - dossierFinalMaxInlineThreads = 20 - dossierFinalMaxThreadComments = 5 - dossierFinalExcerptRunes = 240 - dossierSummarySchemaVersion = 1 - dossierSummaryTaskID = "dossier-discussion-summary" - dossierSummaryMaxTopLevel = 20 - dossierSummaryMaxInlineThreads = 20 - dossierSummaryMaxThreadComments = 5 - dossierSummaryExcerptRunes = 480 + dossierFinalMaxTopLevelComments = 20 + dossierFinalMaxInlineThreads = 20 + dossierFinalMaxThreadComments = 5 + dossierFinalExcerptRunes = 240 + dossierSummarySchemaVersion = 1 + dossierSummaryTaskID = "dossier-discussion-summary" + dossierSummaryMaxTopLevel = 20 + dossierSummaryMaxInlineThreads = 20 + dossierSummaryMaxThreadComments = 5 + dossierSummaryExcerptRunes = 480 + workbenchMetadataSchemaVersion = 1 + workbenchCheckoutModeArtifactClone = "artifact-clone" ) var forbiddenDiscussionSummaryPatterns = []*regexp.Regexp{ @@ -2838,6 +2925,340 @@ var forbiddenDiscussionSummaryPatterns = []*regexp.Regexp{ regexp.MustCompile(`\bcheck(s)? failed\b`), } +func prepareWorkbenchArtifacts(ctx context.Context, opts Options, req workbenchPreparationRequest) error { + sourceRepoRoot, err := opts.resolveRepoRoot(ctx) + if err != nil { + return fmt.Errorf("pipeline: resolve source repo root: %w", err) + } + sourceRepoRoot = filepath.Clean(sourceRepoRoot) + + _ = unlockWorkbenchRepo(req.Artifacts.WorkbenchRepoDir) + if err := os.RemoveAll(req.Artifacts.WorkbenchDir); err != nil { + return fmt.Errorf("pipeline: reset workbench dir: %w", err) + } + for _, dir := range []string{req.Artifacts.WorkbenchDir, req.Artifacts.WorkbenchScratch} { + if err := os.MkdirAll(dir, 0o700); err != nil { + return fmt.Errorf("pipeline: create workbench dir: %w", err) + } + } + + baseRemoteURL, err := resolveWorkbenchBaseRemoteURL(ctx, opts, sourceRepoRoot, req.ReviewPR.Base) + if err != nil { + return err + } + if _, err := opts.gitCommand(ctx, "", "clone", "--no-checkout", "--no-hardlinks", sourceRepoRoot, req.Artifacts.WorkbenchRepoDir); err != nil { + return fmt.Errorf("pipeline: clone workbench repo: %w", err) + } + remoteMatchesBaseHost, err := workbenchRemoteMatchesBaseHost(baseRemoteURL, req.ReviewPR.Base) + if err != nil { + return err + } + if !remoteMatchesBaseHost { + return fmt.Errorf("pipeline: source repo origin %q does not match PR base repo %s/%s on %s", baseRemoteURL, req.ReviewPR.Base.Owner, req.ReviewPR.Base.Repo, req.ReviewPR.Base.Host) + } + + if err := ensureWorkbenchCommit(ctx, opts, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Base, baseRemoteURL); err != nil { + return err + } + headRemoteURL := baseRemoteURL + if !sameBranchRepo(req.ReviewPR.Base, req.ReviewPR.Head) { + headRemoteURL, err = deriveWorkbenchRemoteURL(baseRemoteURL, req.ReviewPR.Head) + if err != nil { + return fmt.Errorf("pipeline: derive head remote URL: %w", err) + } + } + if err := ensureWorkbenchCommit(ctx, opts, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Head, headRemoteURL); err != nil { + return err + } + if _, err := opts.gitCommand(ctx, req.Artifacts.WorkbenchRepoDir, "checkout", "--detach", req.ReviewPR.Head.SHA); err != nil { + return fmt.Errorf("pipeline: checkout workbench head %s: %w", shortSHA(req.ReviewPR.Head.SHA), err) + } + if err := makeWorkbenchRepoReadOnly(req.Artifacts.WorkbenchRepoDir); err != nil { + return err + } + + changedFiles := append([]string(nil), req.ChangedFiles...) + sort.Strings(changedFiles) + meta := workbenchMetadataArtifact{ + SchemaVersion: workbenchMetadataSchemaVersion, + SourceRepoRoot: sourceRepoRoot, + CheckoutMode: workbenchCheckoutModeArtifactClone, + PR: workbenchPRIdentity{ + Host: req.PRRef.Host, + Owner: req.PRRef.Owner, + Repo: req.PRRef.Repo, + Number: req.PRRef.Number, + }, + Base: workbenchBranchArtifactFromRef(req.ReviewPR.Base), + Head: workbenchBranchArtifactFromRef(req.ReviewPR.Head), + RepoPath: req.Artifacts.WorkbenchRepoDir, + ScratchPath: req.Artifacts.WorkbenchScratch, + ChangedFiles: changedFiles, + FingerprintInputs: workbenchFingerprintInputs{ + PR: workbenchPRIdentity{ + Host: req.PRRef.Host, + Owner: req.PRRef.Owner, + Repo: req.PRRef.Repo, + Number: req.PRRef.Number, + }, + BaseSHA: req.ReviewPR.Base.SHA, + HeadSHA: req.ReviewPR.Head.SHA, + CheckoutMode: workbenchCheckoutModeArtifactClone, + ChangedFiles: changedFiles, + SourceRepoRoot: sourceRepoRoot, + }, + } + return writeJSONFile(req.Artifacts.WorkbenchMetadataPath(), meta) +} + +func resolveWorkbenchBaseRemoteURL(ctx context.Context, opts Options, sourceRepoRoot string, base gitprovider.PRBranchRef) (string, error) { + originOutput, err := opts.gitCommand(ctx, sourceRepoRoot, "remote", "get-url", "origin") + if err != nil { + return "", fmt.Errorf("pipeline: resolve source repo origin: %w", err) + } + originURL := strings.TrimSpace(string(originOutput)) + if originURL == "" { + return "", fmt.Errorf("pipeline: source repo origin URL is empty") + } + host, owner, repo, _, err := parseWorkbenchRemoteURL(originURL) + if err != nil { + return "", fmt.Errorf("pipeline: parse source repo origin URL %q: %w", originURL, err) + } + if owner != base.Owner || repo != base.Repo { + return "", fmt.Errorf("pipeline: source repo origin %q does not match PR base repo %s/%s", originURL, base.Owner, base.Repo) + } + if host == "" { + return "", fmt.Errorf("pipeline: source repo origin %q did not include a host", originURL) + } + return originURL, nil +} + +func workbenchRemoteMatchesBaseHost(remoteURL string, base gitprovider.PRBranchRef) (bool, error) { + host, _, _, _, err := parseWorkbenchRemoteURL(remoteURL) + if err != nil { + return false, fmt.Errorf("pipeline: parse source repo origin URL %q: %w", remoteURL, err) + } + return strings.EqualFold(host, base.Host), nil +} + +func ensureWorkbenchCommit(ctx context.Context, opts Options, repoDir string, branch gitprovider.PRBranchRef, remoteURL string) error { + ref := strings.TrimSpace(branch.Ref) + if err := validateWorkbenchFetchRef(ref); err != nil { + return err + } + if workbenchCommitPresent(ctx, opts, repoDir, branch.SHA) { + return nil + } + if _, err := opts.gitCommand(ctx, repoDir, "fetch", "--no-tags", remoteURL, branch.SHA); err == nil && workbenchCommitPresent(ctx, opts, repoDir, branch.SHA) { + return nil + } + if ref != "" { + if _, err := opts.gitCommand(ctx, repoDir, "fetch", "--no-tags", remoteURL, ref); err == nil && workbenchCommitPresent(ctx, opts, repoDir, branch.SHA) { + return nil + } + } + return fmt.Errorf("pipeline: fetch commit %s for %s/%s from %q", shortSHA(branch.SHA), branch.Owner, branch.Repo, remoteURL) +} + +func validateWorkbenchFetchRef(ref string) error { + if ref == "" { + return nil + } + if !strings.HasPrefix(ref, "refs/") { + return fmt.Errorf("pipeline: reject unsafe fetch ref %q", ref) + } + return nil +} + +func workbenchCommitPresent(ctx context.Context, opts Options, repoDir, sha string) bool { + if strings.TrimSpace(sha) == "" { + return false + } + _, err := opts.gitCommand(ctx, repoDir, "cat-file", "-e", sha+"^{commit}") + return err == nil +} + +func makeWorkbenchRepoReadOnly(root string) error { + err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if entry.Type()&os.ModeSymlink != 0 { + return nil + } + info, err := entry.Info() + if err != nil { + return err + } + mode := info.Mode().Perm() & 0o555 + if !entry.IsDir() { + mode |= 0o444 + } + if err := os.Chmod(path, mode); err != nil { // #nosec G122 -- workbench paths are pipeline-owned artifact files; symlinks are skipped during the walk. + return fmt.Errorf("chmod %s: %w", path, err) + } + return nil + }) + if err != nil { + return fmt.Errorf("pipeline: lock workbench repo read-only: %w", err) + } + return nil +} + +func unlockWorkbenchRepo(root string) error { + if strings.TrimSpace(root) == "" { + return nil + } + if _, err := os.Stat(root); errors.Is(err, os.ErrNotExist) { + return nil + } else if err != nil { + return err + } + err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if entry.Type()&os.ModeSymlink != 0 { + return nil + } + info, err := entry.Info() + if err != nil { + return err + } + mode := info.Mode().Perm() + if entry.IsDir() { + mode |= 0o700 + mode |= 0o055 + } else { + mode |= 0o200 + mode |= 0o444 + } + if err := os.Chmod(path, mode); err != nil { // #nosec G122 -- workbench paths are pipeline-owned artifact files; symlinks are skipped during the walk. + return fmt.Errorf("chmod %s: %w", path, err) + } + return nil + }) + if err != nil { + return fmt.Errorf("pipeline: unlock workbench repo: %w", err) + } + return nil +} + +func sameBranchRepo(left, right gitprovider.PRBranchRef) bool { + return strings.EqualFold(left.Host, right.Host) && left.Owner == right.Owner && left.Repo == right.Repo +} + +func workbenchBranchArtifactFromRef(ref gitprovider.PRBranchRef) workbenchBranchArtifact { + return workbenchBranchArtifact{ + Host: ref.Host, + Owner: ref.Owner, + Repo: ref.Repo, + Name: ref.Name, + Ref: ref.Ref, + SHA: ref.SHA, + } +} + +type workbenchRemoteStyle struct { + scheme string + user string + host string + scp bool + dotGit bool +} + +func parseWorkbenchRemoteURL(raw string) (host, owner, repo string, style workbenchRemoteStyle, err error) { + raw = strings.TrimSpace(raw) + switch { + case strings.Contains(raw, "://"): + parsed, parseErr := url.Parse(raw) + if parseErr != nil { + return "", "", "", workbenchRemoteStyle{}, parseErr + } + owner, repo, dotGit, parseErr := parseWorkbenchRepoPath(parsed.Path) + if parseErr != nil { + return "", "", "", workbenchRemoteStyle{}, parseErr + } + style = workbenchRemoteStyle{ + scheme: parsed.Scheme, + host: parsed.Host, + dotGit: dotGit, + } + if parsed.User != nil { + style.user = parsed.User.Username() + } + return parsed.Host, owner, repo, style, nil + case strings.Contains(raw, "@") && strings.Contains(raw, ":"): + parts := strings.SplitN(raw, ":", 2) + if len(parts) != 2 { + return "", "", "", workbenchRemoteStyle{}, fmt.Errorf("invalid scp-style remote") + } + userHost := parts[0] + pathPart := parts[1] + userHostParts := strings.SplitN(userHost, "@", 2) + if len(userHostParts) != 2 { + return "", "", "", workbenchRemoteStyle{}, fmt.Errorf("invalid scp-style remote") + } + owner, repo, dotGit, parseErr := parseWorkbenchRepoPath(pathPart) + if parseErr != nil { + return "", "", "", workbenchRemoteStyle{}, parseErr + } + style = workbenchRemoteStyle{ + user: userHostParts[0], + host: userHostParts[1], + scp: true, + dotGit: dotGit, + } + return userHostParts[1], owner, repo, style, nil + default: + return "", "", "", workbenchRemoteStyle{}, fmt.Errorf("unsupported remote URL %q", raw) + } +} + +func parseWorkbenchRepoPath(path string) (owner, repo string, dotGit bool, err error) { + path = strings.Trim(strings.TrimSpace(path), "/") + if strings.HasSuffix(path, ".git") { + dotGit = true + path = strings.TrimSuffix(path, ".git") + } + parts := strings.Split(path, "/") + if len(parts) < 2 { + return "", "", false, fmt.Errorf("unsupported repo path %q", path) + } + return parts[len(parts)-2], parts[len(parts)-1], dotGit, nil +} + +func deriveWorkbenchRemoteURL(originURL string, branch gitprovider.PRBranchRef) (string, error) { + _, _, _, style, err := parseWorkbenchRemoteURL(originURL) + if err != nil { + return "", err + } + repoPath := branch.Owner + "/" + branch.Repo + if style.dotGit { + repoPath += ".git" + } + if style.scp { + return style.user + "@" + branch.Host + ":" + repoPath, nil + } + u := &url.URL{ + Scheme: style.scheme, + Host: branch.Host, + Path: "/" + repoPath, + } + if style.user != "" { + u.User = url.User(style.user) + } + return u.String(), nil +} + +func shortSHA(sha string) string { + sha = strings.TrimSpace(sha) + if len(sha) > 12 { + return sha[:12] + } + return sha +} + func writeRawDossierArtifacts(paths ArtifactPaths, in dossierInputs) error { rawDir := filepath.Join(paths.DossierDir, "raw") summaryDir := filepath.Join(paths.DossierDir, "summary") @@ -3948,6 +4369,9 @@ func ArtifactPathsForRun(layout statepaths.Layout, ref gitprovider.PRRef, pr git AgentLogsDir: filepath.Join(dir, "agent-logs"), LLMTasksDir: filepath.Join(dir, "llm-tasks"), DossierDir: filepath.Join(dir, "dossier"), + WorkbenchDir: filepath.Join(dir, "workbench"), + WorkbenchRepoDir: filepath.Join(dir, "workbench", "repo"), + WorkbenchScratch: filepath.Join(dir, "workbench", "scratch"), }, nil } @@ -3963,6 +4387,9 @@ func ArtifactPathsFromDir(dir string) ArtifactPaths { AgentLogsDir: filepath.Join(dir, "agent-logs"), LLMTasksDir: filepath.Join(dir, "llm-tasks"), DossierDir: filepath.Join(dir, "dossier"), + WorkbenchDir: filepath.Join(dir, "workbench"), + WorkbenchRepoDir: filepath.Join(dir, "workbench", "repo"), + WorkbenchScratch: filepath.Join(dir, "workbench", "scratch"), } } @@ -4072,6 +4499,48 @@ func (opts Options) maxConcurrency(maxAgents int) int { return opts.MaxConcurrency } +func (opts Options) gitCommand(ctx context.Context, dir string, args ...string) ([]byte, error) { + if opts.GitCommand != nil { + return opts.GitCommand(ctx, dir, args...) + } + cmdArgs := append([]string{}, args...) + cmd := exec.CommandContext(ctx, "git", cmdArgs...) // #nosec G204 -- pipeline invokes git with fixed command names and structured arguments. + if strings.TrimSpace(dir) != "" { + cmd.Dir = dir + } + output, err := cmd.CombinedOutput() + if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + message := strings.TrimSpace(string(output)) + if message == "" { + message = err.Error() + } + return nil, fmt.Errorf("git %s: %s", strings.Join(args, " "), message) + } + return output, nil +} + +func (opts Options) resolveRepoRoot(ctx context.Context) (string, error) { + if opts.ResolveRepoRoot != nil { + return opts.ResolveRepoRoot(ctx) + } + out, err := opts.gitCommand(ctx, "", "rev-parse", "--show-toplevel") + if err != nil { + return "", err + } + root := strings.TrimSpace(string(out)) + if root == "" { + return "", fmt.Errorf("git rev-parse --show-toplevel returned empty output") + } + return root, nil +} + +func shouldAutoUnlockWorkbenchOnExit(opts Options) bool { + return opts.AutoUnlockWorkbenchOnExit +} + func knownAgents(catalog agents.Catalog) map[string]bool { out := make(map[string]bool, len(catalog.Agents)) for _, agent := range catalog.Agents { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index ded5f65..009f1f2 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -8,9 +8,11 @@ import ( "fmt" "io/fs" "os" + "os/exec" "path/filepath" "reflect" "regexp" + "slices" "strings" "sync" "testing" @@ -27,6 +29,24 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/statepaths" ) +func dryRunForTest(ctx context.Context, opts Options, req Request) (Result, error) { + opts.AutoUnlockWorkbenchOnExit = true + configureWorkbenchFixtureForTest(ctx, &opts, req.PRRef) + return DryRun(ctx, opts, req) +} + +func selectionOnlyForTest(ctx context.Context, opts Options, req SelectionRequest) (SelectionResult, error) { + opts.AutoUnlockWorkbenchOnExit = true + configureWorkbenchFixtureForTest(ctx, &opts, req.PRRef) + return SelectionOnly(ctx, opts, req) +} + +func liveForTest(ctx context.Context, opts Options, req Request, run ledger.Run) (Result, error) { + opts.AutoUnlockWorkbenchOnExit = true + configureWorkbenchFixtureForTest(ctx, &opts, req.PRRef) + return Live(ctx, opts, req, run) +} + func TestDryRunPlansAndPersistsWithoutProviderWrites(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) @@ -79,7 +99,7 @@ func TestDryRunPlansAndPersistsWithoutProviderWrites(t *testing.T) { } } - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -199,7 +219,7 @@ func TestDryRunNormalizesReviewerFindingsFileAlias(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsFileAliasJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -242,9 +262,9 @@ func TestDryRunWithPinnedReviewSHAsUsesCompareDiffAndPinnedFileRefs(t *testing.T defer closeStore(t, store) provider, req := dryRunHarness(t) writeAgentFullContent(t, req.Profile.AgentSources[0], "harness", "reviewer") - currentBaseSHA := provider.pr.Base.SHA - reviewBaseSHA := strings.Repeat("1", 40) - reviewHeadSHA := strings.Repeat("2", 40) + fixture, reviewBaseSHA, reviewHeadSHA := newPinnedReviewFixtureForRef(t, req.PRRef) + provider.pr = fixture.pr + provider.fixtureRepoDir = fixture.repoDir req.ReviewBaseSHA = reviewBaseSHA req.ReviewHeadSHA = reviewHeadSHA provider.diffBetween = gitprovider.UnifiedDiff{Raw: smallDiff("main.go")} @@ -256,7 +276,7 @@ func TestDryRunWithPinnedReviewSHAsUsesCompareDiffAndPinnedFileRefs(t *testing.T adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) layout := statepaths.NewLayout(t.TempDir(), t.TempDir()) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -275,7 +295,7 @@ func TestDryRunWithPinnedReviewSHAsUsesCompareDiffAndPinnedFileRefs(t *testing.T if len(provider.diffBetweenCalls) != 1 || provider.diffBetweenCalls[0].baseSHA != reviewBaseSHA || provider.diffBetweenCalls[0].headSHA != reviewHeadSHA { t.Fatalf("diff between calls = %#v, want pinned base/head", provider.diffBetweenCalls) } - if result.CurrentBaseSHA != strings.Repeat("b", 40) || result.CurrentHeadSHA != strings.Repeat("a", 40) || + if result.CurrentBaseSHA != provider.pr.Base.SHA || result.CurrentHeadSHA != provider.pr.Head.SHA || result.ReviewBaseSHA != reviewBaseSHA || result.ReviewHeadSHA != reviewHeadSHA { t.Fatalf("result SHAs = current %s/%s review %s/%s", result.CurrentBaseSHA, result.CurrentHeadSHA, result.ReviewBaseSHA, result.ReviewHeadSHA) } @@ -294,7 +314,7 @@ func TestDryRunWithPinnedReviewSHAsUsesCompareDiffAndPinnedFileRefs(t *testing.T if !strings.Contains(selectionPrompt, reviewBaseSHA) || !strings.Contains(selectionPrompt, reviewHeadSHA) { t.Fatalf("selection prompt missing pinned review SHAs: %s", selectionPrompt) } - if strings.Contains(selectionPrompt, currentBaseSHA) || strings.Contains(selectionPrompt, provider.pr.Head.SHA) { + if strings.Contains(selectionPrompt, provider.pr.Head.SHA) { t.Fatalf("selection prompt contains current PR SHAs: %s", selectionPrompt) } if provider.threadCalls != 0 { @@ -303,7 +323,7 @@ func TestDryRunWithPinnedReviewSHAsUsesCompareDiffAndPinnedFileRefs(t *testing.T if provider.reviewCalls != 0 || provider.issueCommentCalls != 0 { t.Fatalf("review/comment calls = %d/%d, want no live discussion reads for pinned review", provider.reviewCalls, provider.issueCommentCalls) } - if !containsFileCall(provider.treeCalls, fileKey{gitRef: currentBaseSHA, path: ".codereview/agents"}) { + if !containsFileCall(provider.treeCalls, fileKey{gitRef: provider.pr.Base.SHA, path: ".codereview/agents"}) { t.Fatalf("tree calls = %#v, want repo agents loaded from current base SHA", provider.treeCalls) } if containsFileCall(provider.treeCalls, fileKey{gitRef: reviewBaseSHA, path: ".codereview/agents"}) { @@ -323,7 +343,7 @@ func TestDryRunWithPinnedReviewSHAsRejectsForkHeads(t *testing.T) { provider.pr.Head.Owner = "fork-owner" provider.pr.Head.Repo = "codereview-cli-fork" - _, err := DryRun(ctx, Options{ + _, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, Store: store, @@ -354,7 +374,7 @@ func TestDryRunSelectionPromptInstructionsStayInsideStructuredPayload(t *testing adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - if _, err := DryRun(ctx, Options{ + if _, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -401,7 +421,7 @@ func TestSelectionOnlyRunsSingleSelectionPhaseWithoutReviewArtifacts(t *testing. adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) artifactDir := t.TempDir() - result, err := SelectionOnly(ctx, Options{ + result, err := selectionOnlyForTest(ctx, Options{ Provider: provider, Adapter: adapter, Now: fixedNow, @@ -504,7 +524,7 @@ func TestSelectionOnlyPromptPreservesRoutingContractWithoutReviewerPromptBodies( adapter.Queue(fakeLLMResult("dossier-summary-session", discussionSummaryJSON(nil, []threadSummary{{path: "main.go", line: 2, status: "unresolved", summary: "Open thread at main.go:2"}}), 8, 2)) adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:alpha", "main.go"), 10, 2)) - if _, err := SelectionOnly(ctx, Options{ + if _, err := selectionOnlyForTest(ctx, Options{ Provider: provider, Adapter: adapter, Now: fixedNow, @@ -590,7 +610,7 @@ func TestSelectionOnlyReusesCachedDossierSummaryTask(t *testing.T) { firstAdapter.Queue(fakeLLMResult("dossier-summary-session", discussionSummaryJSON([]string{"Compact concern"}, nil), 8, 2)) firstAdapter.Queue(fakeLLMResult("selection-session-1", selectionJSON("harness:reviewer", "main.go"), 10, 2)) firstProgress := &fakeTaskProgress{} - if _, err := SelectionOnly(ctx, Options{ + if _, err := selectionOnlyForTest(ctx, Options{ Provider: provider, Adapter: firstAdapter, TaskProgress: firstProgress, @@ -606,7 +626,7 @@ func TestSelectionOnlyReusesCachedDossierSummaryTask(t *testing.T) { secondAdapter := &llm.FakeAdapter{NameValue: "fake-llm"} secondAdapter.Queue(fakeLLMResult("selection-session-2", selectionJSON("harness:reviewer", "main.go"), 10, 2)) secondProgress := &fakeTaskProgress{} - if _, err := SelectionOnly(ctx, Options{ + if _, err := selectionOnlyForTest(ctx, Options{ Provider: provider, Adapter: secondAdapter, TaskProgress: secondProgress, @@ -631,7 +651,7 @@ func TestSelectionOnlyReusesCachedDossierSummaryTask(t *testing.T) { thirdAdapter.Queue(fakeLLMResult("dossier-summary-session-2", discussionSummaryJSON([]string{"Updated concern"}, nil), 8, 2)) thirdAdapter.Queue(fakeLLMResult("selection-session-3", selectionJSON("harness:reviewer", "main.go"), 10, 2)) thirdProgress := &fakeTaskProgress{} - if _, err := SelectionOnly(ctx, Options{ + if _, err := selectionOnlyForTest(ctx, Options{ Provider: provider, Adapter: thirdAdapter, TaskProgress: thirdProgress, @@ -1243,7 +1263,7 @@ func TestSelectionOnlyRejectsInvalidSelection(t *testing.T) { adapter.Queue(fakeLLMResult("selection-session-retry", selectionJSON("missing:agent", "main.go"), 10, 2)) artifactDir := t.TempDir() - result, err := SelectionOnly(ctx, Options{ + result, err := selectionOnlyForTest(ctx, Options{ Provider: provider, Adapter: adapter, Now: fixedNow, @@ -1298,7 +1318,7 @@ func TestSelectionOnlyCapsMaxAgents(t *testing.T) { "reasoning": "too many" }`, 10, 2)) - result, err := SelectionOnly(ctx, Options{ + result, err := selectionOnlyForTest(ctx, Options{ Provider: provider, Adapter: adapter, Now: fixedNow, @@ -1332,7 +1352,7 @@ func TestSelectionOnlyContextBudgetFailure(t *testing.T) { req.Profile.AgentSources = []string{dir} adapter := &llm.FakeAdapter{NameValue: "fake-llm"} - _, err := SelectionOnly(ctx, Options{ + _, err := selectionOnlyForTest(ctx, Options{ Provider: provider, Adapter: adapter, Now: fixedNow, @@ -1353,7 +1373,7 @@ func TestSelectionOnlyNoDiffSkipsLLMAndReturnsPreparedContext(t *testing.T) { adapter := &llm.FakeAdapter{NameValue: "fake-llm"} artifactDir := t.TempDir() - result, err := SelectionOnly(ctx, Options{ + result, err := selectionOnlyForTest(ctx, Options{ Provider: provider, Adapter: adapter, Now: fixedNow, @@ -1378,6 +1398,360 @@ func TestSelectionOnlyNoDiffSkipsLLMAndReturnsPreparedContext(t *testing.T) { } } +func TestPrepareWorkbenchArtifactsCreatesPinnedReadOnlyCheckoutAndMetadata(t *testing.T) { + ctx := context.Background() + fixture := newWorkbenchGitFixture(t) + artifacts := ArtifactPathsFromDir(t.TempDir()) + + err := prepareWorkbenchArtifacts(ctx, Options{ + ResolveRepoRoot: func(context.Context) (string, error) { return fixture.repoDir, nil }, + }, workbenchPreparationRequest{ + PRRef: fixture.pr.Ref, + ReviewPR: fixture.pr, + ChangedFiles: []string{"main.go"}, + Artifacts: artifacts, + }) + if err != nil { + t.Fatalf("prepareWorkbenchArtifacts: %v", err) + } + t.Cleanup(func() { + if err := unlockWorkbenchRepo(artifacts.WorkbenchRepoDir); err != nil { + t.Fatalf("unlockWorkbenchRepo: %v", err) + } + }) + + if got := strings.TrimSpace(gitCommandOutput(t, artifacts.WorkbenchRepoDir, "rev-parse", "HEAD")); got != fixture.headSHA { + t.Fatalf("workbench HEAD = %q, want %q", got, fixture.headSHA) + } + if got := strings.TrimSpace(gitCommandOutput(t, artifacts.WorkbenchRepoDir, "diff", "--name-only", fixture.baseSHA+"...HEAD")); got != "main.go" { + t.Fatalf("workbench diff names = %q, want main.go", got) + } + if err := os.WriteFile(filepath.Join(artifacts.WorkbenchRepoDir, "main.go"), []byte("blocked"), 0o600); err == nil { + t.Fatalf("overwrite of read-only workbench file unexpectedly succeeded") + } + if err := os.WriteFile(filepath.Join(artifacts.WorkbenchRepoDir, "new-file.txt"), []byte("blocked"), 0o600); err == nil { + t.Fatalf("new file creation in read-only workbench unexpectedly succeeded") + } + if err := os.WriteFile(filepath.Join(artifacts.WorkbenchScratch, "note.txt"), []byte("ok"), 0o600); err != nil { + t.Fatalf("write scratch: %v", err) + } + + var meta workbenchMetadataArtifact + if err := readJSONFile(artifacts.WorkbenchMetadataPath(), &meta); err != nil { + t.Fatalf("read metadata: %v", err) + } + if meta.SchemaVersion != workbenchMetadataSchemaVersion || meta.CheckoutMode != workbenchCheckoutModeArtifactClone { + t.Fatalf("metadata = %#v, want schema %d and checkout mode %q", meta, workbenchMetadataSchemaVersion, workbenchCheckoutModeArtifactClone) + } + if meta.Base.SHA != fixture.baseSHA || meta.Head.SHA != fixture.headSHA { + t.Fatalf("metadata refs = %#v/%#v, want base/head fixture SHAs", meta.Base, meta.Head) + } + if meta.PR != (workbenchPRIdentity{Host: fixture.pr.Ref.Host, Owner: fixture.pr.Ref.Owner, Repo: fixture.pr.Ref.Repo, Number: fixture.pr.Ref.Number}) { + t.Fatalf("metadata PR = %#v, want fixture PR identity", meta.PR) + } + if meta.SourceRepoRoot != fixture.repoDir { + t.Fatalf("metadata source repo root = %q, want %q", meta.SourceRepoRoot, fixture.repoDir) + } + if meta.RepoPath != artifacts.WorkbenchRepoDir || meta.ScratchPath != artifacts.WorkbenchScratch { + t.Fatalf("metadata paths = repo %q scratch %q, want artifact workbench paths", meta.RepoPath, meta.ScratchPath) + } + if !reflect.DeepEqual(meta.ChangedFiles, []string{"main.go"}) { + t.Fatalf("metadata changed files = %#v, want main.go", meta.ChangedFiles) + } + if meta.FingerprintInputs.PR != meta.PR || + meta.FingerprintInputs.BaseSHA != fixture.baseSHA || + meta.FingerprintInputs.HeadSHA != fixture.headSHA || + meta.FingerprintInputs.CheckoutMode != workbenchCheckoutModeArtifactClone || + meta.FingerprintInputs.SourceRepoRoot != fixture.repoDir || + !reflect.DeepEqual(meta.FingerprintInputs.ChangedFiles, []string{"main.go"}) { + t.Fatalf("fingerprint inputs = %#v, want deterministic metadata inputs", meta.FingerprintInputs) + } +} + +func TestDeriveWorkbenchRemoteURLPreservesRemoteStyle(t *testing.T) { + branch := gitprovider.PRBranchRef{Host: "github.com", Owner: "fork-owner", Repo: "codereview-cli"} + + scpURL, err := deriveWorkbenchRemoteURL("git@github.com:open-cli-collective/codereview-cli.git", branch) + if err != nil { + t.Fatalf("derive scp remote: %v", err) + } + if scpURL != "git@github.com:fork-owner/codereview-cli.git" { + t.Fatalf("scp remote = %q, want fork-style scp URL", scpURL) + } + + httpsURL, err := deriveWorkbenchRemoteURL("https://github.com/open-cli-collective/codereview-cli.git", branch) + if err != nil { + t.Fatalf("derive https remote: %v", err) + } + if httpsURL != "https://github.com/fork-owner/codereview-cli.git" { + t.Fatalf("https remote = %q, want fork-style https URL", httpsURL) + } +} + +func TestPrepareWorkbenchArtifactsFetchesForkHeadFromDerivedRemote(t *testing.T) { + ctx := context.Background() + fixture := newForkWorkbenchFixture(t) + artifacts := ArtifactPathsFromDir(t.TempDir()) + var fetchedRemotes []string + gitRunner := func(ctx context.Context, dir string, args ...string) ([]byte, error) { + cmdArgs := append([]string(nil), args...) + if len(cmdArgs) >= 3 && cmdArgs[0] == "fetch" { + fetchedRemotes = append(fetchedRemotes, cmdArgs[2]) + switch cmdArgs[2] { + case "git@github.com:open-cli-collective/codereview-cli.git": + cmdArgs[2] = fixture.baseRemotePath + case "git@github.com:fork-owner/codereview-cli-fork.git": + cmdArgs[2] = fixture.forkRemotePath + } + } + cmd := exec.CommandContext(ctx, "git", cmdArgs...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + if strings.TrimSpace(dir) != "" { + cmd.Dir = dir + } + out, err := cmd.CombinedOutput() + if err != nil { + return nil, fmt.Errorf("git %s: %s", strings.Join(cmdArgs, " "), strings.TrimSpace(string(out))) + } + return out, nil + } + + err := prepareWorkbenchArtifacts(ctx, Options{ + ResolveRepoRoot: func(context.Context) (string, error) { return fixture.sourceRepoDir, nil }, + GitCommand: gitRunner, + }, workbenchPreparationRequest{ + PRRef: fixture.pr.Ref, + ReviewPR: fixture.pr, + ChangedFiles: []string{"main.go"}, + Artifacts: artifacts, + }) + if err != nil { + t.Fatalf("prepareWorkbenchArtifacts: %v", err) + } + t.Cleanup(func() { + if err := unlockWorkbenchRepo(artifacts.WorkbenchRepoDir); err != nil { + t.Fatalf("unlockWorkbenchRepo: %v", err) + } + }) + + if got := strings.TrimSpace(gitCommandOutput(t, artifacts.WorkbenchRepoDir, "rev-parse", "HEAD")); got != fixture.pr.Head.SHA { + t.Fatalf("workbench HEAD = %q, want fork head %q", got, fixture.pr.Head.SHA) + } + if got := strings.TrimSpace(gitCommandOutput(t, artifacts.WorkbenchRepoDir, "diff", "--name-only", fixture.pr.Base.SHA+"...HEAD")); got != "main.go" { + t.Fatalf("workbench diff names = %q, want main.go", got) + } + if !slices.Contains(fetchedRemotes, "git@github.com:fork-owner/codereview-cli-fork.git") { + t.Fatalf("fetched remotes = %#v, want derived fork remote fetch", fetchedRemotes) + } +} + +func TestPrepareWorkbenchArtifactsRejectsMismatchedBaseHostEvenWhenCommitsExistLocally(t *testing.T) { + ctx := context.Background() + fixture := newWorkbenchGitFixture(t) + artifacts := ArtifactPathsFromDir(t.TempDir()) + pr := fixture.pr + pr.Base.Host = "example.com" + pr.Ref.Host = "example.com" + + err := prepareWorkbenchArtifacts(ctx, Options{ + ResolveRepoRoot: func(context.Context) (string, error) { return fixture.repoDir, nil }, + }, workbenchPreparationRequest{ + PRRef: pr.Ref, + ReviewPR: pr, + ChangedFiles: []string{"main.go"}, + Artifacts: artifacts, + }) + if err == nil { + t.Fatal("prepareWorkbenchArtifacts unexpectedly succeeded for mismatched base host") + } + if !strings.Contains(err.Error(), `source repo origin "git@github.com:open-cli-collective/codereview-cli.git" does not match PR base repo open-cli-collective/codereview-cli on example.com`) { + t.Fatalf("prepareWorkbenchArtifacts error = %v, want host mismatch", err) + } +} + +func TestPrepareWorkbenchArtifactsRejectsUnsafeFetchRef(t *testing.T) { + ctx := context.Background() + fixture := newForkWorkbenchFixture(t) + artifacts := ArtifactPathsFromDir(t.TempDir()) + pr := fixture.pr + pr.Head.Ref = "--upload-pack=/tmp/pwn" + gitRunner := func(ctx context.Context, dir string, args ...string) ([]byte, error) { + cmdArgs := append([]string(nil), args...) + if len(cmdArgs) >= 3 && cmdArgs[0] == "fetch" { + switch cmdArgs[2] { + case "git@github.com:open-cli-collective/codereview-cli.git": + cmdArgs[2] = fixture.baseRemotePath + case "git@github.com:fork-owner/codereview-cli-fork.git": + cmdArgs[2] = fixture.forkRemotePath + } + } + cmd := exec.CommandContext(ctx, "git", cmdArgs...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + if strings.TrimSpace(dir) != "" { + cmd.Dir = dir + } + out, err := cmd.CombinedOutput() + if err != nil { + return nil, fmt.Errorf("git %s: %s", strings.Join(cmdArgs, " "), strings.TrimSpace(string(out))) + } + return out, nil + } + + err := prepareWorkbenchArtifacts(ctx, Options{ + ResolveRepoRoot: func(context.Context) (string, error) { return fixture.sourceRepoDir, nil }, + GitCommand: gitRunner, + }, workbenchPreparationRequest{ + PRRef: pr.Ref, + ReviewPR: pr, + ChangedFiles: []string{"main.go"}, + Artifacts: artifacts, + }) + if err == nil || !strings.Contains(err.Error(), `reject unsafe fetch ref "--upload-pack=/tmp/pwn"`) { + t.Fatalf("prepareWorkbenchArtifacts error = %v, want unsafe ref rejection", err) + } +} + +func TestPrepareWorkbenchArtifactsRefreshesExistingArtifactRoot(t *testing.T) { + ctx := context.Background() + fixture := newWorkbenchGitFixture(t) + artifacts := ArtifactPathsFromDir(t.TempDir()) + req := workbenchPreparationRequest{ + PRRef: fixture.pr.Ref, + ReviewPR: fixture.pr, + ChangedFiles: []string{"main.go"}, + Artifacts: artifacts, + } + opts := Options{ + ResolveRepoRoot: func(context.Context) (string, error) { return fixture.repoDir, nil }, + } + + if err := prepareWorkbenchArtifacts(ctx, opts, req); err != nil { + t.Fatalf("prepareWorkbenchArtifacts first run: %v", err) + } + if err := unlockWorkbenchRepo(artifacts.WorkbenchRepoDir); err != nil { + t.Fatalf("unlockWorkbenchRepo: %v", err) + } + stalePath := filepath.Join(artifacts.WorkbenchDir, "stale.txt") + if err := os.WriteFile(stalePath, []byte("stale"), 0o600); err != nil { + t.Fatalf("write stale artifact: %v", err) + } + if err := os.WriteFile(artifacts.WorkbenchMetadataPath(), []byte(`{"schema_version":999}`), 0o600); err != nil { + t.Fatalf("overwrite stale metadata: %v", err) + } + + if err := prepareWorkbenchArtifacts(ctx, opts, req); err != nil { + t.Fatalf("prepareWorkbenchArtifacts second run: %v", err) + } + t.Cleanup(func() { + if err := unlockWorkbenchRepo(artifacts.WorkbenchRepoDir); err != nil { + t.Fatalf("unlockWorkbenchRepo cleanup: %v", err) + } + }) + + if _, err := os.Stat(stalePath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("stale artifact stat error = %v, want not exist", err) + } + var meta workbenchMetadataArtifact + if err := readJSONFile(artifacts.WorkbenchMetadataPath(), &meta); err != nil { + t.Fatalf("read refreshed metadata: %v", err) + } + if meta.SchemaVersion != workbenchMetadataSchemaVersion || meta.Head.SHA != fixture.headSHA { + t.Fatalf("refreshed metadata = %#v, want current workbench metadata", meta) + } +} + +func TestSelectionOnlyPreparesWorkbenchInCallerOwnedArtifacts(t *testing.T) { + ctx := context.Background() + fixture := newWorkbenchGitFixture(t) + provider, req := dryRunHarness(t) + provider.pr = fixture.pr + provider.diff = gitprovider.UnifiedDiff{Raw: smallDiff("main.go")} + req.PRRef = fixture.pr.Ref + req.PRURL = fixture.pr.URL + adapter := &llm.FakeAdapter{NameValue: "fake-llm"} + adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) + artifactDir := t.TempDir() + + result, err := selectionOnlyForTest(ctx, Options{ + Provider: provider, + Adapter: adapter, + Now: fixedNow, + ResolveRepoRoot: func(context.Context) (string, error) { + return fixture.repoDir, nil + }, + }, selectionRequestFromReview(req, artifactDir)) + if err != nil { + t.Fatalf("SelectionOnly: %v", err) + } + + if !reflect.DeepEqual(result.Artifacts, ArtifactPathsFromDir(artifactDir)) { + t.Fatalf("artifacts = %#v, want caller-owned dir %q", result.Artifacts, artifactDir) + } + if got := strings.TrimSpace(gitCommandOutput(t, result.Artifacts.WorkbenchRepoDir, "rev-parse", "HEAD")); got != fixture.headSHA { + t.Fatalf("workbench HEAD = %q, want %q", got, fixture.headSHA) + } + if _, err := os.Stat(result.Artifacts.WorkbenchMetadataPath()); err != nil { + t.Fatalf("stat workbench metadata: %v", err) + } +} + +func TestDryRunPreparesWorkbenchInAllocatedRunArtifacts(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + fixture := newWorkbenchGitFixture(t) + provider, req := dryRunHarness(t) + provider.pr = fixture.pr + provider.diff = gitprovider.UnifiedDiff{Raw: smallDiff("main.go")} + req.PRRef = fixture.pr.Ref + req.PRURL = fixture.pr.URL + req.Profile.AgentSources = nil + adapter := &llm.FakeAdapter{NameValue: "fake-llm"} + adapter.Queue(fakeLLMResult("selection-session", `{ + "schema_version": 1, + "selected_agents": [], + "thread_actions": [], + "reasoning": "no specialist needed" + }`, 10, 2)) + adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", nil), 30, 6)) + + result, err := dryRunForTest(ctx, Options{ + Provider: provider, + Adapter: adapter, + Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + ResolveRepoRoot: func(context.Context) (string, error) { + return fixture.repoDir, nil + }, + NewRunID: func() string { return "run-workbench" }, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, + }, req) + if err != nil { + t.Fatalf("DryRun: %v", err) + } + t.Cleanup(func() { + if err := unlockWorkbenchRepo(result.Artifacts.WorkbenchRepoDir); err != nil { + t.Fatalf("unlockWorkbenchRepo cleanup: %v", err) + } + }) + + if got := strings.TrimSpace(gitCommandOutput(t, result.Artifacts.WorkbenchRepoDir, "rev-parse", "HEAD")); got != fixture.headSHA { + t.Fatalf("workbench HEAD = %q, want %q", got, fixture.headSHA) + } + if got := strings.TrimSpace(gitCommandOutput(t, result.Artifacts.WorkbenchRepoDir, "diff", "--name-only", fixture.baseSHA+"...HEAD")); got != "main.go" { + t.Fatalf("workbench diff names = %q, want main.go", got) + } + var meta workbenchMetadataArtifact + if err := readJSONFile(result.Artifacts.WorkbenchMetadataPath(), &meta); err != nil { + t.Fatalf("read metadata: %v", err) + } + if meta.RepoPath != result.Artifacts.WorkbenchRepoDir || meta.ScratchPath != result.Artifacts.WorkbenchScratch { + t.Fatalf("metadata paths = %#v, want allocated run workbench paths", meta) + } +} + func TestDryRunNoDiffDoesNotResolveUnmappedModelTier(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) @@ -1391,7 +1765,7 @@ func TestDryRunNoDiffDoesNotResolveUnmappedModelTier(t *testing.T) { } adapter := &llm.FakeAdapter{NameValue: "fake-llm"} - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -1425,7 +1799,7 @@ func TestDryRunAgentModelTierUsesProfileModelMapOverride(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -1469,7 +1843,7 @@ func TestDryRunReviewerBaselineTierRaisesReviewerModelFloor(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -1542,7 +1916,7 @@ func TestDryRunSelectionOverridesApplyOnlyToSelection(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -1603,7 +1977,7 @@ func TestDryRunReviewerOverridesApplyOnlyToReviewers(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -1648,7 +2022,7 @@ func TestDryRunReviewerFailureIsolation(t *testing.T) { writeAgent(t, req.Profile.AgentSources[0], "harness", "gamma", "gamma desc", "Review gamma.") adapter := &reviewerIsolationAdapter{reviewerBarrier: newReviewerStartBarrier(3)} - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -1725,7 +2099,7 @@ func TestDryRunReviewerProviderFailureIsolation(t *testing.T) { betaErr := errors.New("provider wait failed") adapter := &reviewerIsolationAdapter{betaProviderErr: betaErr, reviewerBarrier: newReviewerStartBarrier(3)} - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -1799,17 +2173,18 @@ func TestLiveResumeCompletedSelectionAndReviewersRerunsOnlyRollup(t *testing.T) firstAdapter.Queue(fakeLLMResult("rollup-retry-session", rollupJSON("comment", []string{"missing-finding"}), 30, 6)) findingID := func() (review.FindingID, error) { return review.FindingID("finding-1"), nil } - _, err := Live(ctx, Options{ - Provider: provider, - Adapter: firstAdapter, - Store: store, - NamedSessions: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingID, - NewActionID: actionSequence(), - MaxConcurrency: 1, + _, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: firstAdapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingID, + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err == nil || !errors.Is(err, ErrStructuredOutputInvalidAfterRetry) { t.Fatalf("first Live error = %v, want invalid rollup after retry", err) @@ -1836,17 +2211,18 @@ func TestLiveResumeCompletedSelectionAndReviewersRerunsOnlyRollup(t *testing.T) secondAdapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} secondAdapter.Queue(fakeLLMResult("rollup-fixed-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := Live(ctx, Options{ - Provider: provider, - Adapter: secondAdapter, - Store: store, - NamedSessions: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Now: fixedNow, - NewSessionRowID: sequence("resume-session"), - NewFindingID: findingID, - NewActionID: actionSequence(), - MaxConcurrency: 1, + result, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: secondAdapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("resume-session"), + NewFindingID: findingID, + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err != nil { t.Fatalf("second Live: %v", err) @@ -2185,7 +2561,7 @@ func TestDryRunReviewerModelTierOverrideAppliesOnlyToReviewers(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -2228,7 +2604,7 @@ func TestDryRunAgentModelIDBypassesModelMapForReviewer(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -2282,7 +2658,7 @@ func TestDryRunReviewerBaselineDoesNotAffectAgentModelID(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -2363,7 +2739,7 @@ func TestDryRunReviewerModelOverrideBypassesAgentModelID(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -2419,7 +2795,7 @@ func TestDryRunPrunesConfiguredRetentionBeforeFetch(t *testing.T) { } } - if _, err := DryRun(ctx, Options{ + if _, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -2457,7 +2833,7 @@ func TestDryRunManualOnlySkipsRetentionBeforeFetch(t *testing.T) { } } - if _, err := DryRun(ctx, Options{ + if _, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -2509,16 +2885,17 @@ func TestLivePlansPendingActionsWithoutCompletingRun(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := Live(ctx, Options{ - Provider: provider, - Adapter: adapter, - Store: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + result, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: adapter, + Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err != nil { t.Fatalf("Live: %v", err) @@ -2576,16 +2953,17 @@ func TestLiveRejectsStageRuntimeOverrides(t *testing.T) { run := allocateLiveRun(t, store, provider, req, "run-live-override-"+strings.ReplaceAll(tt.name, " ", "-")) adapter := &llm.FakeAdapter{NameValue: "fake-llm"} - _, err := Live(ctx, Options{ - Provider: provider, - Adapter: adapter, - Store: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + _, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: adapter, + Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err == nil { t.Fatal("Live error = nil, want stage override rejection") @@ -2619,17 +2997,18 @@ func TestLiveNamedSessionResumesOrchestratorOnlyAndReturnsCandidate(t *testing.T adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-new", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := Live(ctx, Options{ - Provider: provider, - Adapter: adapter, - Store: store, - NamedSessions: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + result, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: adapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err != nil { t.Fatalf("Live: %v", err) @@ -2676,17 +3055,18 @@ func TestLiveNamedSessionMissingRowStartsFreshAndReturnsCandidate(t *testing.T) adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) adapter.Queue(fakeLLMResult("rollup-new", rollupJSON("comment", []string{"finding-1"}), 30, 6)) - result, err := Live(ctx, Options{ - Provider: provider, - Adapter: adapter, - Store: store, - NamedSessions: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + result, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: adapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err != nil { t.Fatalf("Live: %v", err) @@ -2733,17 +3113,18 @@ func TestLiveNamedSessionScopeMismatchRefusesBeforeLLM(t *testing.T) { } adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} - _, err := Live(ctx, Options{ - Provider: provider, - Adapter: adapter, - Store: store, - NamedSessions: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + _, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: adapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err == nil || !strings.Contains(err.Error(), tt.wantErr) { t.Fatalf("Live error = %v, want %q", err, tt.wantErr) @@ -2770,17 +3151,18 @@ func TestLiveNamedSessionResumeFailureLeavesStoredSessionUnchanged(t *testing.T) resumeErr := errors.New("resume failed") adapter.Queue(llm.FakeResult{StartErr: resumeErr}) - _, err := Live(ctx, Options{ - Provider: provider, - Adapter: adapter, - Store: store, - NamedSessions: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + _, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: adapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if !errors.Is(err, resumeErr) { t.Fatalf("Live error = %v, want resume failure", err) @@ -2815,18 +3197,19 @@ func TestLiveNamedSessionCrossHostWarnsAndContinues(t *testing.T) { adapter.Queue(fakeLLMResult("rollup-new", rollupJSON("comment", []string{"finding-1"}), 30, 6)) var warnings bytes.Buffer - result, err := Live(ctx, Options{ - Provider: provider, - Adapter: adapter, - Store: store, - NamedSessions: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Warnings: &warnings, - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + result, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: adapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Warnings: &warnings, + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err != nil { t.Fatalf("Live: %v", err) @@ -2860,18 +3243,19 @@ func TestLiveNamedSessionUnsupportedResumeStartsFreshAndReturnsCandidate(t *test adapter.Queue(fakeLLMResult("rollup-fresh", rollupJSON("comment", []string{"finding-1"}), 30, 6)) var warnings bytes.Buffer - result, err := Live(ctx, Options{ - Provider: provider, - Adapter: adapter, - Store: store, - NamedSessions: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Warnings: &warnings, - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + result, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: adapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Warnings: &warnings, + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err != nil { t.Fatalf("Live: %v", err) @@ -2905,18 +3289,19 @@ func TestLiveNamedSessionNoDiffLeavesCandidateEmpty(t *testing.T) { adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} var warnings bytes.Buffer - result, err := Live(ctx, Options{ - Provider: provider, - Adapter: adapter, - Store: store, - NamedSessions: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Warnings: &warnings, - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + result, err := liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: adapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Warnings: &warnings, + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err != nil { t.Fatalf("Live: %v", err) @@ -2964,16 +3349,17 @@ func TestLiveMarksRunIncompleteAfterBlockingLLMTaskError(t *testing.T) { t.Fatalf("AllocateRun: %v", err) } - _, err = Live(ctx, Options{ - Provider: provider, - Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, - Store: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + _, err = liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, + Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if err == nil || !strings.Contains(err.Error(), "no queued result") { t.Fatalf("Live error = %v, want fake LLM planning error", err) @@ -3013,16 +3399,17 @@ func TestLiveLeavesRunIncompleteAfterContextCancellation(t *testing.T) { t.Fatalf("AllocateRun: %v", err) } - _, err = Live(ctx, Options{ - Provider: provider, - Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, - Store: store, - Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), - Now: fixedNow, - NewSessionRowID: sequence("session"), - NewFindingID: findingSequence("finding"), - NewActionID: actionSequence(), - MaxConcurrency: 1, + _, err = liveForTest(ctx, Options{ + AutoUnlockWorkbenchOnExit: true, + Provider: provider, + Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, + Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, }, req, run) if !errors.Is(err, context.Canceled) { t.Fatalf("Live error = %v, want context.Canceled", err) @@ -3067,7 +3454,7 @@ func TestDryRunNoResolveThreadsKeepsSummaryReplyOnly(t *testing.T) { }`, 1, 1)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("approve", nil), 1, 1)) - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -3113,7 +3500,7 @@ func TestDryRunMultiAgentSessionsMapFindingsToReviewerSessions(t *testing.T) { provider.diff.Raw = smallDiff("main.go") + smallDiff("other.go") adapter := &promptAwareAdapter{} - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -3233,7 +3620,7 @@ func TestDryRunPlanSummaryNamesWorkstreamsInSelectionOrder(t *testing.T) { req.ToolVersion = "0.0.0-test" provider.diff.Raw = smallDiff("main.go") + smallDiff("other.go") - result, err := DryRun(ctx, Options{ + result, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: &promptAwareAdapter{}, Store: store, @@ -3415,7 +3802,7 @@ func TestDryRunRejectsUnsafeProfileAgentSourcesBeforeRunAllocation(t *testing.T) provider, req := dryRunHarness(t) req.Profile.AgentSources = []string{tt.source(t)} - _, err := DryRun(ctx, Options{ + _, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, Store: store, @@ -3445,7 +3832,7 @@ func TestDryRunMarksRunFailedAfterPostAllocationError(t *testing.T) { adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 1, 1)) adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 1, 1)) - _, err := DryRun(ctx, Options{ + _, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -3727,7 +4114,7 @@ func TestDryRunContextBudgetFailures(t *testing.T) { if tt.queue != nil { tt.queue(adapter) } - _, err := DryRun(ctx, Options{ + _, err := dryRunForTest(ctx, Options{ Provider: provider, Adapter: adapter, Store: store, @@ -3781,6 +4168,7 @@ type readOnlyProvider struct { issueCommentCalls int caps gitprovider.ProviderCaps onGetPR func() + fixtureRepoDir string } type shaPair struct { @@ -4083,44 +4471,269 @@ func containsFileCall(calls []fileKey, want fileKey) bool { return false } -func dryRunHarness(t *testing.T) (*readOnlyProvider, Request) { +type workbenchGitFixture struct { + repoDir string + baseSHA string + headSHA string + pr gitprovider.PR +} + +type forkWorkbenchFixture struct { + sourceRepoDir string + baseRemotePath string + forkRemotePath string + pr gitprovider.PR +} + +func newWorkbenchGitFixture(t *testing.T) workbenchGitFixture { t.Helper() - ref := gitprovider.PRRef{Host: "github.com", Owner: "open-cli-collective", Repo: "codereview-cli", Number: 29} - baseSHA := strings.Repeat("b", 40) - headSHA := strings.Repeat("a", 40) - pr := gitprovider.PR{ - Ref: ref, - Title: "CR-20 dry-run", - Body: "Default PR body.", - URL: prURL(ref), - State: gitprovider.PRStateOpen, - Author: gitprovider.Identity{Login: "author", ID: "author-id"}, - Base: gitprovider.PRBranchRef{ - Host: ref.Host, - Owner: ref.Owner, - Repo: ref.Repo, - Name: "main", - Ref: "refs/heads/main", - SHA: baseSHA, + ref := gitprovider.PRRef{Host: "github.com", Owner: "open-cli-collective", Repo: "codereview-cli", Number: 370} + return newWorkbenchGitFixtureForRef(t, ref) +} + +func newWorkbenchGitFixtureForRef(t *testing.T, ref gitprovider.PRRef) workbenchGitFixture { + t.Helper() + repoDir := t.TempDir() + gitCommandMustSucceed(t, repoDir, "init", "-b", "main") + gitCommandMustSucceed(t, repoDir, "config", "user.name", "Workbench Test") + gitCommandMustSucceed(t, repoDir, "config", "user.email", "workbench@example.com") + gitCommandMustSucceed(t, repoDir, "remote", "add", "origin", "git@github.com:open-cli-collective/codereview-cli.git") + if err := os.WriteFile(filepath.Join(repoDir, "main.go"), []byte("package main\n\nvar changed = false\n"), 0o600); err != nil { + t.Fatalf("write main.go: %v", err) + } + gitCommandMustSucceed(t, repoDir, "add", "main.go") + gitCommandMustSucceed(t, repoDir, "commit", "-m", "base") + baseSHA := strings.TrimSpace(gitCommandOutput(t, repoDir, "rev-parse", "HEAD")) + gitCommandMustSucceed(t, repoDir, "checkout", "-b", "feature") + if err := os.WriteFile(filepath.Join(repoDir, "main.go"), []byte("package main\n\nvar changed = true\n"), 0o600); err != nil { + t.Fatalf("update main.go: %v", err) + } + gitCommandMustSucceed(t, repoDir, "commit", "-am", "head") + headSHA := strings.TrimSpace(gitCommandOutput(t, repoDir, "rev-parse", "HEAD")) + + return workbenchGitFixture{ + repoDir: repoDir, + baseSHA: baseSHA, + headSHA: headSHA, + pr: gitprovider.PR{ + Ref: ref, + Title: "Workbench fixture", + URL: prURL(ref), + State: gitprovider.PRStateOpen, + Base: gitprovider.PRBranchRef{ + Host: ref.Host, + Owner: ref.Owner, + Repo: ref.Repo, + Name: "main", + Ref: "refs/heads/main", + SHA: baseSHA, + }, + Head: gitprovider.PRBranchRef{ + Host: ref.Host, + Owner: ref.Owner, + Repo: ref.Repo, + Name: "feature", + Ref: "refs/heads/feature", + SHA: headSHA, + }, }, - Head: gitprovider.PRBranchRef{ - Host: ref.Host, - Owner: ref.Owner, - Repo: ref.Repo, - Name: "feature", - Ref: "refs/heads/feature", - SHA: headSHA, + } +} + +func newPinnedReviewFixtureForRef(t *testing.T, ref gitprovider.PRRef) (workbenchGitFixture, string, string) { + t.Helper() + repoDir := t.TempDir() + gitCommandMustSucceed(t, repoDir, "init", "-b", "main") + gitCommandMustSucceed(t, repoDir, "config", "user.name", "Workbench Test") + gitCommandMustSucceed(t, repoDir, "config", "user.email", "workbench@example.com") + gitCommandMustSucceed(t, repoDir, "remote", "add", "origin", "git@github.com:open-cli-collective/codereview-cli.git") + if err := os.WriteFile(filepath.Join(repoDir, "main.go"), []byte("package main\n\nvar changed = false\n"), 0o600); err != nil { + t.Fatalf("write main.go: %v", err) + } + gitCommandMustSucceed(t, repoDir, "add", "main.go") + gitCommandMustSucceed(t, repoDir, "commit", "-m", "base") + reviewBaseSHA := strings.TrimSpace(gitCommandOutput(t, repoDir, "rev-parse", "HEAD")) + if err := os.WriteFile(filepath.Join(repoDir, "main.go"), []byte("package main\n\nvar changed = maybe\n"), 0o600); err != nil { + t.Fatalf("update main.go for review head: %v", err) + } + gitCommandMustSucceed(t, repoDir, "commit", "-am", "review head") + reviewHeadSHA := strings.TrimSpace(gitCommandOutput(t, repoDir, "rev-parse", "HEAD")) + gitCommandMustSucceed(t, repoDir, "checkout", "-b", "feature") + if err := os.WriteFile(filepath.Join(repoDir, "main.go"), []byte("package main\n\nvar changed = true\n"), 0o600); err != nil { + t.Fatalf("update main.go for current head: %v", err) + } + gitCommandMustSucceed(t, repoDir, "commit", "-am", "current head") + headSHA := strings.TrimSpace(gitCommandOutput(t, repoDir, "rev-parse", "HEAD")) + + return workbenchGitFixture{ + repoDir: repoDir, + baseSHA: reviewHeadSHA, + headSHA: headSHA, + pr: gitprovider.PR{ + Ref: ref, + Title: "Pinned review fixture", + URL: prURL(ref), + State: gitprovider.PRStateOpen, + Base: gitprovider.PRBranchRef{ + Host: ref.Host, + Owner: ref.Owner, + Repo: ref.Repo, + Name: "main", + Ref: "refs/heads/main", + SHA: reviewHeadSHA, + }, + Head: gitprovider.PRBranchRef{ + Host: ref.Host, + Owner: ref.Owner, + Repo: ref.Repo, + Name: "feature", + Ref: "refs/heads/feature", + SHA: headSHA, + }, + }, + }, reviewBaseSHA, reviewHeadSHA +} + +func newForkWorkbenchFixture(t *testing.T) forkWorkbenchFixture { + t.Helper() + baseSeedDir := t.TempDir() + gitCommandMustSucceed(t, baseSeedDir, "init", "-b", "main") + gitCommandMustSucceed(t, baseSeedDir, "config", "user.name", "Workbench Test") + gitCommandMustSucceed(t, baseSeedDir, "config", "user.email", "workbench@example.com") + if err := os.WriteFile(filepath.Join(baseSeedDir, "main.go"), []byte("package main\n\nvar changed = false\n"), 0o600); err != nil { + t.Fatalf("write main.go: %v", err) + } + gitCommandMustSucceed(t, baseSeedDir, "add", "main.go") + gitCommandMustSucceed(t, baseSeedDir, "commit", "-m", "base") + baseSHA := strings.TrimSpace(gitCommandOutput(t, baseSeedDir, "rev-parse", "HEAD")) + + baseRemotePath := filepath.Join(t.TempDir(), "base-remote.git") + gitCommandMustSucceed(t, "", "clone", "--bare", baseSeedDir, baseRemotePath) + + sourceRepoDir := filepath.Join(t.TempDir(), "source") + gitCommandMustSucceed(t, "", "clone", baseRemotePath, sourceRepoDir) + gitCommandMustSucceed(t, sourceRepoDir, "remote", "set-url", "origin", "git@github.com:open-cli-collective/codereview-cli.git") + + forkRemotePath := filepath.Join(t.TempDir(), "fork-remote.git") + gitCommandMustSucceed(t, "", "clone", baseRemotePath, forkRemotePath) + gitCommandMustSucceed(t, forkRemotePath, "checkout", "-b", "feature") + gitCommandMustSucceed(t, forkRemotePath, "config", "user.name", "Fork Workbench Test") + gitCommandMustSucceed(t, forkRemotePath, "config", "user.email", "fork@example.com") + if err := os.WriteFile(filepath.Join(forkRemotePath, "main.go"), []byte("package main\n\nvar changed = true\n"), 0o600); err != nil { + t.Fatalf("update fork main.go: %v", err) + } + gitCommandMustSucceed(t, forkRemotePath, "commit", "-am", "fork head") + headSHA := strings.TrimSpace(gitCommandOutput(t, forkRemotePath, "rev-parse", "HEAD")) + + ref := gitprovider.PRRef{Host: "github.com", Owner: "open-cli-collective", Repo: "codereview-cli", Number: 371} + return forkWorkbenchFixture{ + sourceRepoDir: sourceRepoDir, + baseRemotePath: baseRemotePath, + forkRemotePath: forkRemotePath, + pr: gitprovider.PR{ + Ref: ref, + Title: "Fork workbench fixture", + URL: prURL(ref), + State: gitprovider.PRStateOpen, + Base: gitprovider.PRBranchRef{ + Host: ref.Host, + Owner: ref.Owner, + Repo: ref.Repo, + Name: "main", + Ref: "refs/heads/main", + SHA: baseSHA, + }, + Head: gitprovider.PRBranchRef{ + Host: ref.Host, + Owner: "fork-owner", + Repo: "codereview-cli-fork", + Name: "feature", + Ref: "refs/heads/feature", + SHA: headSHA, + }, }, } +} + +func gitCommandMustSucceed(t *testing.T, dir string, args ...string) string { + t.Helper() + return strings.TrimSpace(gitCommandOutput(t, dir, args...)) +} + +func gitCommandOutput(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + if strings.TrimSpace(dir) != "" { + cmd.Dir = dir + } + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return string(out) +} + +func configureWorkbenchFixtureForTest(_ context.Context, opts *Options, ref gitprovider.PRRef) { + if opts.ResolveRepoRoot != nil && opts.GitCommand != nil { + return + } + provider, ok := opts.Provider.(*readOnlyProvider) + if !ok || strings.TrimSpace(provider.fixtureRepoDir) == "" { + return + } + repoDir := provider.fixtureRepoDir + if opts.ResolveRepoRoot == nil { + opts.ResolveRepoRoot = func(context.Context) (string, error) { + return repoDir, nil + } + } + if opts.GitCommand == nil { + opts.GitCommand = workbenchGitCommandForTest(ref) + } +} + +func workbenchGitCommandForTest(ref gitprovider.PRRef) func(context.Context, string, ...string) ([]byte, error) { + return func(ctx context.Context, dir string, args ...string) ([]byte, error) { + if len(args) == 3 && args[0] == "remote" && args[1] == "get-url" && args[2] == "origin" { + return []byte(fmt.Sprintf("https://%s/%s/%s.git\n", ref.Host, ref.Owner, ref.Repo)), nil + } + cmd := exec.CommandContext(ctx, "git", args...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + if strings.TrimSpace(dir) != "" { + cmd.Dir = dir + } + out, err := cmd.CombinedOutput() + if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + message := strings.TrimSpace(string(out)) + if message == "" { + message = err.Error() + } + return nil, fmt.Errorf("git %s: %s", strings.Join(args, " "), message) + } + return out, nil + } +} + +func dryRunHarness(t *testing.T) (*readOnlyProvider, Request) { + t.Helper() + ref := gitprovider.PRRef{Host: "github.com", Owner: "open-cli-collective", Repo: "codereview-cli", Number: 29} + fixture := newWorkbenchGitFixtureForRef(t, ref) + pr := fixture.pr + pr.Title = "CR-20 dry-run" + pr.Body = "Default PR body." + pr.Author = gitprovider.Identity{Login: "author", ID: "author-id"} dir := t.TempDir() writeAgent(t, dir, "harness", "reviewer", "reviewer desc", "Review carefully.") trustCurrentTempFixtures(t) provider := &readOnlyProvider{ - pr: pr, - diff: gitprovider.UnifiedDiff{Raw: smallDiff("main.go")}, - files: map[fileKey][]byte{}, - trees: map[fileKey][]gitprovider.TreeEntry{}, - caps: gitprovider.ProviderCaps{NativeFileLevelComments: true, ThreadResolution: true}, + pr: pr, + diff: gitprovider.UnifiedDiff{Raw: smallDiff("main.go")}, + files: map[fileKey][]byte{}, + trees: map[fileKey][]gitprovider.TreeEntry{}, + caps: gitprovider.ProviderCaps{NativeFileLevelComments: true, ThreadResolution: true}, + fixtureRepoDir: fixture.repoDir, } req := Request{ PRRef: ref,