diff --git a/README.md b/README.md index 3840b95..bda58e8 100644 --- a/README.md +++ b/README.md @@ -677,6 +677,11 @@ Run a live review: cr review https://github.com/OWNER/REPO/pull/123 ``` +The PR URL and selected profile are the repository inputs. `cr review` may be +run outside a Git checkout or from an unrelated repository; it fetches the +PR's exact base and head commits into the run-owned workbench using the +profile's repository credential. + Run a live review and fail the command when a major or blocking finding exists: ```bash diff --git a/docs/architecture.md b/docs/architecture.md index c7b4222..18614b9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -75,6 +75,19 @@ This keeps markers, retries, reconciliation, idempotency, and resume behavior in one place. New commands such as `cr respond` should produce planned thread actions and let the reviewplan/ledger/outbox flow perform provider writes. +## Authenticated Git Reads + +`internal/gitexec` is the narrow transport adapter for non-interactive Git +commands. The application composition roots construct it from the configured +repository/read provider's refreshable token source and inject its command +function into review and selection pipelines. Authentication is host-scoped +and process-only; posting/reviewer credentials are not checkout credentials. + +The pipeline validates the requested, fetched, base, and head repository +identities before invoking Git. `internal/workbench.RunPreparer` then owns only +run-workbench creation, exact-commit fetching, validation, reuse, and metadata. +It does not inspect the caller's current directory or local Git origin. + ## Command And Review Harness Guardrails Command packages should stay thin: parse args and flags, load runtime @@ -85,10 +98,10 @@ packages; shared command infrastructure belongs in packages such as `internal/cmd/root`. Command-independent helpers used to compose review and response application runtimes, such as retention-policy conversion and repository-root resolution, belong in `internal/appruntime` rather than -`internal/cmd/cmdruntime`. Review lifecycle runtime assembly belongs in -`internal/reviewruntime`; `cr review` and `cr respond` command packages should -construct `reviewruntime.OpenRequest` values and keep CLI-only validation, -rendering, config-path selection, and error mapping at the command boundary. +`internal/cmd/cmdruntime`. Production review lifecycle assembly belongs in +`internal/app`; `cr review` and `cr respond` command packages should construct +`app.OpenRequest` values and keep CLI-only validation, rendering, config-path +selection, and error mapping at the command boundary. Application packages outside `internal/cmd` and `internal/view` should not depend on Cobra, command packages, or view packages. Those packages should diff --git a/docs/checkout-native-review-contract.md b/docs/checkout-native-review-contract.md index d406bba..ad10944 100644 --- a/docs/checkout-native-review-contract.md +++ b/docs/checkout-native-review-contract.md @@ -86,11 +86,10 @@ 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` +`workbench/metadata.json` is a versioned durable artifact. Schema version `2` records: - `schema_version` -- `source_repo_root` - `checkout_mode` - normalized PR identity under `pr` - pinned base and head refs @@ -103,13 +102,19 @@ records: downstream tasks can fingerprint workbench semantics without re-deriving them from the checkout tree ad hoc. +Schema version `1` remains readable for exact-byte workbench reuse. Its legacy +`source_repo_root` field is ignored because the invocation checkout is not a +workbench input; a valid reused v1 metadata file is not rewritten, preserving +durable task fingerprints. + `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. -Pinned review entrypoints require base and head refs that can be fetched from -the same repository remote. Fork-backed heads need explicit fetch and -authentication handling before pinned review can accept them. +Normal review entrypoints derive credential-free HTTPS remotes from the +validated PR identity and fetch the exact base and head commits. Same-host fork +heads are supported; cross-host heads are rejected before authenticated Git is +invoked. Pinned dry-run entrypoints retain their stricter same-repository rule. ## Reviewer-Facing Context diff --git a/docs/development.md b/docs/development.md index f84829b..c1fcfcf 100644 --- a/docs/development.md +++ b/docs/development.md @@ -13,15 +13,16 @@ data lifecycle commands. The current Go code is a Cobra command tree in `internal/cmd/*` with a thin `cmd/cr` entrypoint, shared exit-code mapping in `internal/cmd/exitcode`, and -version plumbing in `internal/version`. Command-independent helpers used to -compose review and response application runtimes live in `internal/appruntime`; -command packages keep CLI composition, rendering, config-path selection, and -error mapping. Review orchestration is split across `internal/reviewruntime`, +version plumbing in `internal/version`. Production review and response runtime +assembly lives in `internal/app`; `internal/appruntime` contains only small +command-independent helpers such as retention conversion and invocation-root +resolution. Command packages keep CLI composition, rendering, config-path +selection, and error mapping. Review orchestration is split across `internal/pipeline`, `internal/reviewrun`, `internal/threadrespond`, `internal/reviewplan`, `internal/outbox`, `internal/gate`, and `internal/gateio`. Command packages construct typed -runtime requests, while `internal/reviewruntime` owns the reusable provider, -ledger, adapter, pipeline, live-run, and response-run assembly. +runtime requests, while `internal/app` owns the reusable provider, ledger, +adapter, pipeline, live-run, and response-run assembly. Architecture guardrails for LLM execution, model resolution, Git provider writes, command and review harness boundaries, inline thread lifecycle, and @@ -43,6 +44,12 @@ Within `internal/pipeline`, the public entry points are `DryRun`, `Live`, and and no ledger or posting side effects so benchmark tooling can reuse the real selector implementation. +Review workbench preparation is location-independent. Runtime composition +constructs the authenticated Git command adapter from the repository/read +credential and injects it into the pipeline; workbench code derives remotes +from the validated PR identity and must not resolve or inspect the invocation +checkout. + Structured LLM calls in the review pipeline are durable per-task units. See `docs/llm-task-artifacts.md` for the artifact schema, status taxonomy, and resume invariants. @@ -98,12 +105,12 @@ make clean # remove build artifacts - Current distribution status: GitHub release archives plus standard package channels declared in `packaging/identity.yml`. - Application package layering follows the active codereview implementation: - command glue in `internal/cmd/*`, review/response app runtime helpers in - `internal/appruntime`, presentation in `internal/view`, + command glue in `internal/cmd/*`, production review/response composition in + `internal/app`, small runtime helpers in `internal/appruntime`, presentation in `internal/view`, state/config adapters in `internal/config`, `internal/ledger`, and `internal/statepaths`, provider/LLM adapters in their owning packages, and - review runtime assembly in `internal/reviewruntime`, review posting/gating in - `internal/outbox`, `internal/gate`, and `internal/gateio`, and response-only + review posting/gating in `internal/outbox`, `internal/gate`, and + `internal/gateio`, and response-only inline discussion handling in `internal/threadrespond`. diff --git a/docs/llm-task-artifacts.md b/docs/llm-task-artifacts.md index 3c34d6d..f5e3944 100644 --- a/docs/llm-task-artifacts.md +++ b/docs/llm-task-artifacts.md @@ -86,9 +86,12 @@ reviewer failure must not approve; the final event is clamped to at least `comment`. `failed_blocking` means the task prevents dependent phases from safely running. -Selection and rollup failures are blocking. Once a run exists, blocking LLM task -failures leave the run `incomplete` so the normal resume gate can rerun only the -failed task and downstream work. +Selection and rollup failures are blocking. `reviewrun` is the sole owner of +live planner-error outcomes: blocking LLM failures leave fresh and resumed runs +`incomplete`; transient failures leave resumed runs `incomplete` but fail fresh +runs; terminal failures fail either kind; cancellation leaves the current +outcome untouched. The normal resume gate can therefore rerun only the failed +task and downstream work after an interruption or durable blocking failure. Provider start/wait failures may have empty `attempts` because no structured output existed. When a provider session ID is known, retry should seed the next diff --git a/internal/app/runtime.go b/internal/app/runtime.go index 12cec95..c069795 100644 --- a/internal/app/runtime.go +++ b/internal/app/runtime.go @@ -14,9 +14,11 @@ import ( "github.com/open-cli-collective/cli-common/credstore" "github.com/open-cli-collective/codereview-cli/internal/approvaloverride" + "github.com/open-cli-collective/codereview-cli/internal/appruntime" "github.com/open-cli-collective/codereview-cli/internal/config" "github.com/open-cli-collective/codereview-cli/internal/credentials" "github.com/open-cli-collective/codereview-cli/internal/datalifecycle" + "github.com/open-cli-collective/codereview-cli/internal/gitexec" "github.com/open-cli-collective/codereview-cli/internal/gitprovider" githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" "github.com/open-cli-collective/codereview-cli/internal/hooks" @@ -73,8 +75,6 @@ type OpenRequest struct { MaxConcurrency int Retention datalifecycle.RetentionPolicy RetentionManualOnly bool - ResolveRepoRoot func(context.Context) (string, error) - GitCommand func(context.Context, string, ...string) ([]byte, error) Dependencies Dependencies } @@ -103,6 +103,8 @@ type AdapterFactory func(config.LLMConfig, credentials.Reader) (llm.Adapter, err // defaults. type Dependencies struct { NewGitProvider GitProviderFactory + NewGitCommand func(string, gitexec.TokenSource) (func(context.Context, string, ...string) ([]byte, error), error) + ResolveRepoRoot func(context.Context) (string, error) ResolvePostingIdentity PostingIdentityResolver NewAdapter AdapterFactory RuntimeLayout func() (statepaths.Layout, error) @@ -119,6 +121,18 @@ func (d Dependencies) withDefaults() Dependencies { if d.ResolvePostingIdentity == nil { d.ResolvePostingIdentity = resolvePostingIdentity } + if d.NewGitCommand == nil { + d.NewGitCommand = func(host string, tokens gitexec.TokenSource) (func(context.Context, string, ...string) ([]byte, error), error) { + client, err := gitexec.New(host, tokens) + if err != nil { + return nil, err + } + return client.Run, nil + } + } + if d.ResolveRepoRoot == nil { + d.ResolveRepoRoot = appruntime.ResolveRepoRoot + } if d.NewAdapter == nil { d.NewAdapter = newAdapter } @@ -150,7 +164,12 @@ func Open(ctx context.Context, req OpenRequest) (Runtime, error) { cleanup() return Runtime{}, err } - repoProvider, _, err := deps.NewGitProvider(profile.Git, repoProviderStore, gitProviderOptions(profile, profile.Git, req.PRRef)) + repoProvider, repoCredential, err := deps.NewGitProvider(profile.Git, repoProviderStore, gitProviderOptions(profile, profile.Git, req.PRRef)) + if err != nil { + cleanup() + return Runtime{}, err + } + gitCommand, err := deps.NewGitCommand(profile.Git.Host, repositoryTokenSource(repoProvider, repoCredential)) if err != nil { cleanup() return Runtime{}, err @@ -215,7 +234,7 @@ func Open(ctx context.Context, req OpenRequest) (Runtime, error) { } return withProgressAdapter(req.Progress, command, adapter, string(profile.LLM.Provider), string(profile.LLM.Adapter)), nil }) - runner := buildReviewRunner(ledgerStore, repoProvider, postingProvider, adapter, profile, limiter, layout, req.Warnings, req.Progress, dispatcher, req, command) + runner := buildReviewRunner(ledgerStore, repoProvider, postingProvider, adapter, profile, limiter, layout, req.Warnings, req.Progress, dispatcher, req, command, gitCommand, deps.ResolveRepoRoot) return Runtime{ Runner: runner, Responder: runner, @@ -372,7 +391,7 @@ func runtimeLayout() (statepaths.Layout, error) { return layout, nil } -func buildReviewRunner(ledgerStore *ledger.Store, repoProvider gitprovider.GitProvider, postingProvider gitprovider.GitProvider, adapter llm.Adapter, profile config.Profile, limiter outbox.Limiter, layout statepaths.Layout, warnings io.Writer, logger *progress.Logger, dispatcher *hookDispatcher, req OpenRequest, command string) reviewRunner { +func buildReviewRunner(ledgerStore *ledger.Store, repoProvider gitprovider.GitProvider, postingProvider gitprovider.GitProvider, adapter llm.Adapter, profile config.Profile, limiter outbox.Limiter, layout statepaths.Layout, warnings io.Writer, logger *progress.Logger, dispatcher *hookDispatcher, req OpenRequest, command string, gitCommand func(context.Context, string, ...string) ([]byte, error), resolveRepoRoot func(context.Context) (string, error)) reviewRunner { taskProgress := newPipelineTaskProgress(logger, command, dispatcher) liveProvider := runtimeProvider{read: repoProvider, write: postingProvider} pipelineOpts := pipeline.Options{ @@ -387,8 +406,8 @@ func buildReviewRunner(ledgerStore *ledger.Store, repoProvider gitprovider.GitPr MaxConcurrency: req.MaxConcurrency, Retention: req.Retention, RetentionManualOnly: req.RetentionManualOnly, - ResolveRepoRoot: req.ResolveRepoRoot, - GitCommand: req.GitCommand, + ResolveRepoRoot: resolveRepoRoot, + GitCommand: gitCommand, } return reviewRunner{ pipeline: pipelineOpts, @@ -403,7 +422,7 @@ func buildReviewRunner(ledgerStore *ledger.Store, repoProvider gitprovider.GitPr ApprovalOverride: withProgressApprovalOverrideClassifier(logger, buildApprovalOverrideClassifier(profile, adapter, warnings)), Retention: req.Retention, RetentionManualOnly: req.RetentionManualOnly, - ResolveRepoRoot: req.ResolveRepoRoot, + ResolveRepoRoot: resolveRepoRoot, }, respond: threadrespond.Options{ Store: ledgerStore, @@ -645,6 +664,13 @@ func resolvePostingIdentity(ctx context.Context, provider gitprovider.GitProvide return provider.WhoAmI(ctx, credential) } +func repositoryTokenSource(provider gitprovider.GitProvider, credential gitprovider.Credential) gitexec.TokenSource { + if source, ok := provider.(gitexec.TokenSource); ok { + return source + } + return gitexec.TokenSourceFunc(func(context.Context) (string, error) { return credential.Token, nil }) +} + type adapterConstructor func(config.LLMConfig, credentials.Reader, config.LLMRuntimeSpec) (llm.Adapter, error) var adapterConstructors = map[config.LLMAdapter]adapterConstructor{ diff --git a/internal/app/runtime_selection.go b/internal/app/runtime_selection.go index 5876812..a4a0272 100644 --- a/internal/app/runtime_selection.go +++ b/internal/app/runtime_selection.go @@ -4,16 +4,14 @@ import ( "context" "github.com/open-cli-collective/codereview-cli/internal/config" - "github.com/open-cli-collective/codereview-cli/internal/gitprovider" - "github.com/open-cli-collective/codereview-cli/internal/llm" + "github.com/open-cli-collective/codereview-cli/internal/pipeline" ) // SelectionRuntime contains the dependencies needed for selection-only // execution paths that must match review-command runtime semantics. type SelectionRuntime struct { - Provider gitprovider.GitProvider - Adapter llm.Adapter - Cleanup func() + Select func(context.Context, pipeline.SelectionRequest) (pipeline.SelectionResult, error) + Cleanup func() } // OpenSelection resolves provider and adapter setup using the same @@ -35,7 +33,12 @@ func OpenSelection(ctx context.Context, req SelectionOpenRequest) (SelectionRunt } // Selection-only paths read with the profile git credential while keeping // PR-scoped GitHub App installation lookup aligned with review runs. - provider, _, err := deps.NewGitProvider(profile.Git, gitStore, gitProviderOptions(profile, profile.Git, req.PRRef)) + provider, credential, err := deps.NewGitProvider(profile.Git, gitStore, gitProviderOptions(profile, profile.Git, req.PRRef)) + if err != nil { + cleanup() + return SelectionRuntime{}, err + } + gitCommand, err := deps.NewGitCommand(profile.Git.Host, repositoryTokenSource(provider, credential)) if err != nil { cleanup() return SelectionRuntime{}, err @@ -54,8 +57,15 @@ func OpenSelection(ctx context.Context, req SelectionOpenRequest) (SelectionRunt return SelectionRuntime{}, err } return SelectionRuntime{ - Provider: provider, - Adapter: adapter, - Cleanup: cleanup, + Select: func(ctx context.Context, selection pipeline.SelectionRequest) (pipeline.SelectionResult, error) { + return pipeline.SelectionOnly(ctx, pipeline.Options{ + Provider: provider, + Adapter: adapter, + GitCommand: gitCommand, + ResolveRepoRoot: deps.ResolveRepoRoot, + MaxAgents: selection.MaxAgents, + }, selection) + }, + Cleanup: cleanup, }, nil } diff --git a/internal/app/runtime_test.go b/internal/app/runtime_test.go index bfc6436..c3a2ca8 100644 --- a/internal/app/runtime_test.go +++ b/internal/app/runtime_test.go @@ -4,6 +4,8 @@ import ( "bytes" "context" "errors" + "os" + "os/exec" "path/filepath" "slices" "strings" @@ -16,12 +18,14 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/config" "github.com/open-cli-collective/codereview-cli/internal/config/configtest" "github.com/open-cli-collective/codereview-cli/internal/credentials" + "github.com/open-cli-collective/codereview-cli/internal/gitexec" "github.com/open-cli-collective/codereview-cli/internal/gitprovider" githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" "github.com/open-cli-collective/codereview-cli/internal/ledger" "github.com/open-cli-collective/codereview-cli/internal/llm" "github.com/open-cli-collective/codereview-cli/internal/outbox" "github.com/open-cli-collective/codereview-cli/internal/pipeline" + "github.com/open-cli-collective/codereview-cli/internal/reporoot" "github.com/open-cli-collective/codereview-cli/internal/review" "github.com/open-cli-collective/codereview-cli/internal/reviewrun" "github.com/open-cli-collective/codereview-cli/internal/statepaths" @@ -77,28 +81,43 @@ func TestOpenUsesReviewerCredentialsAsRuntimeProvider(t *testing.T) { reviewerProvider.SetCapabilities(gitprovider.ProviderCaps{NativeFileLevelComments: true}) identity := gitprovider.Identity{Login: "review-bot", ID: "bot-id"} + deps := testDependencies(t, + func(git config.GitConfig, _ credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + providerCalls = append(providerCalls, git) + if git.Credential.Name == "codereview/home-reviewer" { + return reviewerProvider, gitprovider.Credential{Type: "pat", Token: "reviewer-token"}, nil + } + return repoProvider, gitprovider.Credential{Type: "pat", Token: "repo-token"}, nil + }, + func(_ context.Context, provider gitprovider.GitProvider, credential gitprovider.Credential, _ credentials.Reader, _ config.Profile) (gitprovider.Identity, error) { + if provider != reviewerProvider || credential.Token != "reviewer-token" { + t.Fatalf("identity resolver provider=%#v credential=%#v, want reviewer provider/token", provider, credential) + } + return identity, nil + }, + func(config.LLMConfig, credentials.Reader) (llm.Adapter, error) { + return &llm.FakeAdapter{NameValue: "fake-llm"}, nil + }, + ) + deps.NewGitCommand = func(host string, tokens gitexec.TokenSource) (func(context.Context, string, ...string) ([]byte, error), error) { + if host != "github.com" { + t.Fatalf("Git host = %q, want github.com", host) + } + token, err := tokens.AccessToken(context.Background()) + if err != nil { + t.Fatalf("repository token source: %v", err) + } + if token != "repo-token" { + t.Fatalf("checkout token = %q, want repository token and not reviewer token", token) + } + return func(context.Context, string, ...string) ([]byte, error) { return nil, nil }, nil + } + runtime, err := Open(context.Background(), OpenRequest{ - Config: cfg, - Profile: profile, - PRRef: testPRRef(), - Dependencies: testDependencies(t, - func(git config.GitConfig, _ credentials.Reader, _ githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { - providerCalls = append(providerCalls, git) - if git.Credential.Name == "codereview/home-reviewer" { - return reviewerProvider, gitprovider.Credential{Type: "pat", Token: "reviewer-token"}, nil - } - return repoProvider, gitprovider.Credential{Type: "pat", Token: "repo-token"}, nil - }, - func(_ context.Context, provider gitprovider.GitProvider, credential gitprovider.Credential, _ credentials.Reader, _ config.Profile) (gitprovider.Identity, error) { - if provider != reviewerProvider || credential.Token != "reviewer-token" { - t.Fatalf("identity resolver provider=%#v credential=%#v, want reviewer provider/token", provider, credential) - } - return identity, nil - }, - func(config.LLMConfig, credentials.Reader) (llm.Adapter, error) { - return &llm.FakeAdapter{NameValue: "fake-llm"}, nil - }, - ), + Config: cfg, + Profile: profile, + PRRef: testPRRef(), + Dependencies: deps, }) if err != nil { t.Fatalf("Open: %v", err) @@ -471,6 +490,93 @@ func TestOpenSelectionPassesGitHubAppInstallationLookupAndPinnedID(t *testing.T) } } +func TestOpenSelectionReturnsExecutableSelectionRuntime(t *testing.T) { + ctx := context.Background() + repoDir := t.TempDir() + git := func(dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) // #nosec G204 -- test uses structured Git arguments. + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return strings.TrimSpace(string(out)) + } + git(repoDir, "init", "-b", "main") + git(repoDir, "config", "user.name", "Selection Test") + git(repoDir, "config", "user.email", "selection@example.com") + git(repoDir, "commit", "--allow-empty", "-m", "base") + baseSHA := git(repoDir, "rev-parse", "HEAD") + git(repoDir, "commit", "--allow-empty", "-m", "head") + headSHA := git(repoDir, "rev-parse", "HEAD") + + cfg := testConfig() + profile := cfg.Profiles["home"] + ref := testPRRef() + pr := testPR() + pr.Base.SHA = baseSHA + pr.Head.SHA = headSHA + provider := &gitprovider.Fake{} + if err := provider.SetPR(ref, pr); err != nil { + t.Fatalf("SetPR: %v", err) + } + if err := provider.SetDiff(ref, gitprovider.UnifiedDiff{}); err != nil { + t.Fatalf("SetDiff: %v", err) + } + tokenCalls := 0 + runtime, err := OpenSelection(ctx, SelectionOpenRequest{ + Config: cfg, Profile: profile, PRRef: ref, + Dependencies: Dependencies{ + NewGitProvider: func(config.GitConfig, credentials.Reader, githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + return provider, gitprovider.Credential{Type: "pat", Token: "repo-token"}, nil + }, + NewGitCommand: func(_ string, tokens gitexec.TokenSource) (func(context.Context, string, ...string) ([]byte, error), error) { + return func(ctx context.Context, dir string, args ...string) ([]byte, error) { + cmdArgs := append([]string(nil), args...) + if len(cmdArgs) >= 3 && cmdArgs[0] == "fetch" { + token, err := tokens.AccessToken(ctx) + if err != nil || token != "repo-token" { + t.Fatalf("repository token = %q, %v", token, err) + } + tokenCalls++ + cmdArgs[2] = repoDir + } + cmd := exec.CommandContext(ctx, "git", cmdArgs...) // #nosec G204 -- test uses structured Git arguments. + cmd.Dir = dir + return cmd.CombinedOutput() + }, nil + }, + NewAdapter: func(config.LLMConfig, credentials.Reader) (llm.Adapter, error) { + return &llm.FakeAdapter{NameValue: "fake-llm"}, nil + }, + ResolveRepoRoot: func(context.Context) (string, error) { return "", reporoot.ErrUnavailable }, + }, + }) + if err != nil { + t.Fatalf("OpenSelection: %v", err) + } + defer runtime.Cleanup() + artifactDir := t.TempDir() + result, err := runtime.Select(ctx, pipeline.SelectionRequest{ + PRRef: ref, ProfileName: "home", Profile: profile, + PostingIdentity: gitprovider.Identity{Login: "review-bot", ID: "bot-id"}, + ArtifactDir: artifactDir, + }) + if err != nil { + t.Fatalf("Select: %v", err) + } + if tokenCalls == 0 { + t.Fatal("selection runtime did not use repository token source for fetch") + } + if result.Artifacts.Dir != artifactDir { + t.Fatalf("artifact dir = %q, want %q", result.Artifacts.Dir, artifactDir) + } + if _, err := os.Stat(result.Artifacts.WorkbenchMetadataPath()); err != nil { + t.Fatalf("workbench metadata: %v", err) + } +} + func TestOpenRejectsBackendOverrideForNamedSecretsStore(t *testing.T) { cfg := testConfig() cfg.Secrets = config.SecretsConfig{ diff --git a/internal/architecture/git_transport_test.go b/internal/architecture/git_transport_test.go new file mode 100644 index 0000000..d87b4ca --- /dev/null +++ b/internal/architecture/git_transport_test.go @@ -0,0 +1,48 @@ +package architecture_test + +import ( + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "strconv" + "strings" + "testing" +) + +func TestGitTransportConstructionAndWorkbenchDependenciesStayLayered(t *testing.T) { + repoRoot := repoRootFromTest(t) + internalRoot := filepath.Join(repoRoot, "internal") + gitexecImport := "github.com/open-cli-collective/codereview-cli/internal/gitexec" + reporootImport := "github.com/open-cli-collective/codereview-cli/internal/reporoot" + fset := token.NewFileSet() + err := filepath.WalkDir(internalRoot, func(path string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if entry.IsDir() || filepath.Ext(path) != ".go" || strings.HasSuffix(path, "_test.go") { + return nil + } + parsed, err := parser.ParseFile(fset, path, nil, parser.ImportsOnly) + if err != nil { + return err + } + rel := filepath.ToSlash(mustRel(t, repoRoot, path)) + for _, spec := range parsed.Imports { + importPath, err := strconv.Unquote(spec.Path.Value) + if err != nil { + return err + } + if importPath == gitexecImport && !strings.HasPrefix(rel, "internal/app/") { + t.Fatalf("%s imports gitexec; production Git transport construction belongs in internal/app", rel) + } + if importPath == reporootImport && strings.HasPrefix(rel, "internal/workbench/") { + t.Fatalf("%s imports reporoot; workbench preparation must not depend on the invocation checkout", rel) + } + } + return nil + }) + if err != nil { + t.Fatalf("WalkDir(%s): %v", internalRoot, err) + } +} diff --git a/internal/cmd/benchmarkcmd/executor.go b/internal/cmd/benchmarkcmd/executor.go index c738fd0..5a6d545 100644 --- a/internal/cmd/benchmarkcmd/executor.go +++ b/internal/cmd/benchmarkcmd/executor.go @@ -155,7 +155,6 @@ func (e inProcessExecutor) Execute(ctx context.Context, req reviewExecutionReque MaxConcurrency: req.Candidate.MaxConcurrency, Retention: appruntime.RetentionPolicyFromConfig(e.cfg.Data.Retention), RetentionManualOnly: e.cfg.Data.Retention.Enforcement == config.RetentionManualOnly, - ResolveRepoRoot: appruntime.ResolveRepoRoot, }) if err != nil { return inProcessReviewFailure(cmdruntime.MapRunError(err), &stderr) diff --git a/internal/cmd/benchmarkcmd/select.go b/internal/cmd/benchmarkcmd/select.go index 8613769..ce54c7f 100644 --- a/internal/cmd/benchmarkcmd/select.go +++ b/internal/cmd/benchmarkcmd/select.go @@ -37,7 +37,6 @@ var ( PRRef: ref, }) } - runSelectionOnly = pipeline.SelectionOnly ) type selectFlags struct { @@ -284,11 +283,7 @@ func executeBenchmarkSelectRun(ctx context.Context, logger *progress.Logger, sui _ = promptSpan.End(nil) selectionSpan := logger.Start("benchmark.select", "selection_pipeline", runID) - selectionResult, err := runSelectionOnly(ctx, pipeline.Options{ - Provider: state.runtime.Provider, - Adapter: state.runtime.Adapter, - MaxAgents: candidate.MaxAgents, - }, pipeline.SelectionRequest{ + selectionResult, err := state.runtime.Select(ctx, pipeline.SelectionRequest{ PRRef: ref, ProfileName: state.profileName, Profile: state.profile, @@ -297,6 +292,7 @@ func executeBenchmarkSelectRun(ctx context.Context, logger *progress.Logger, sui ArtifactDir: runDir, ReviewBaseSHA: benchCase.ReviewBaseSHA, ReviewHeadSHA: benchCase.ReviewHeadSHA, + MaxAgents: candidate.MaxAgents, SelectionModelOverride: candidate.Stages.Selection.Model, SelectionEffortOverride: candidate.Stages.Selection.Effort, SelectionPromptInstructions: selectionPromptInstructions, diff --git a/internal/cmd/benchmarkcmd/select_test.go b/internal/cmd/benchmarkcmd/select_test.go index ca00018..46c7665 100644 --- a/internal/cmd/benchmarkcmd/select_test.go +++ b/internal/cmd/benchmarkcmd/select_test.go @@ -486,14 +486,19 @@ func withBenchmarkSelectSeams( t.Helper() oldNow := benchmarkNow oldOpener := openSelectionRuntime - oldRunner := runSelectionOnly benchmarkNow = func() time.Time { return fixedBenchmarkTime() } - openSelectionRuntime = runtimeOpener - runSelectionOnly = runner + openSelectionRuntime = func(ctx context.Context, backend string, changed bool, cfg config.File, profile config.Profile, ref gitprovider.PRRef) (app.SelectionRuntime, error) { + runtime, err := runtimeOpener(ctx, backend, changed, cfg, profile, ref) + if err == nil { + runtime.Select = func(ctx context.Context, req pipeline.SelectionRequest) (pipeline.SelectionResult, error) { + return runner(ctx, pipeline.Options{MaxAgents: req.MaxAgents}, req) + } + } + return runtime, err + } t.Cleanup(func() { benchmarkNow = oldNow openSelectionRuntime = oldOpener - runSelectionOnly = oldRunner }) } diff --git a/internal/cmd/noleak/noleak_test.go b/internal/cmd/noleak/noleak_test.go index 29fb8f8..8c4ad82 100644 --- a/internal/cmd/noleak/noleak_test.go +++ b/internal/cmd/noleak/noleak_test.go @@ -37,6 +37,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/cmd/sessionscmd" "github.com/open-cli-collective/codereview-cli/internal/config" "github.com/open-cli-collective/codereview-cli/internal/credentials" + "github.com/open-cli-collective/codereview-cli/internal/gitexec" "github.com/open-cli-collective/codereview-cli/internal/gitprovider" githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" "github.com/open-cli-collective/codereview-cli/internal/identity" @@ -258,6 +259,9 @@ func TestCommandSurfacesDoNotLeakSeededSecrets(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { h := newAuditHarness(t) + if strings.HasPrefix(tc.name, "review ") { + t.Chdir(t.TempDir()) + } if tc.prepare != nil { tc.prepare(t, h) } @@ -439,25 +443,17 @@ func noLeakGitOutput(t *testing.T, dir string, args ...string) string { return string(out) } -func noLeakGitCommand(ref gitprovider.PRRef) func(context.Context, string, ...string) ([]byte, error) { +func noLeakGitCommand(ref gitprovider.PRRef, repoDir string, run func(context.Context, string, ...string) ([]byte, error)) 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() + out, err := run(ctx, dir, args...) if err != nil { - if ctxErr := ctx.Err(); ctxErr != nil { - return nil, ctxErr - } - message := strings.TrimSpace(string(out)) - if message == "" { - message = err.Error() + return nil, err + } + remoteURL := fmt.Sprintf("https://%s/%s/%s.git", ref.Host, ref.Owner, ref.Repo) + if len(args) == 4 && args[0] == "remote" && args[1] == "add" && args[3] == remoteURL { + if _, err := run(ctx, dir, "config", "url."+repoDir+".insteadOf", remoteURL); err != nil { + return nil, err } - return nil, fmt.Errorf("git %s: %s", strings.Join(args, " "), message) } return out, nil } @@ -772,6 +768,11 @@ func (h *auditHarness) reviewRuntimeFactory(ctx context.Context, runtimeOpts app cleanup() return app.Runtime{}, err } + gitClient, err := gitexec.New(profile.Git.Host, readProvider) + if err != nil { + cleanup() + return app.Runtime{}, err + } postingGit := gitConfigForReviewerAuth(profile) postingProvider, credential, err := h.newGitHubProvider(postingGit, store, installationLookup(runtimeOpts.PRRef)) if err != nil { @@ -809,7 +810,7 @@ func (h *auditHarness) reviewRuntimeFactory(ctx context.Context, runtimeOpts app RetentionManualOnly: runtimeOpts.RetentionManualOnly, MaxAgents: runtimeOpts.MaxAgents, MaxConcurrency: runtimeOpts.MaxConcurrency, - GitCommand: noLeakGitCommand(h.prRef), + GitCommand: noLeakGitCommand(h.prRef, h.workbenchRepoDir, gitClient.Run), ResolveRepoRoot: func(context.Context) (string, error) { return h.workbenchRepoDir, nil }, } liveProvider := noLeakRuntimeProvider{read: readProvider, write: postingProvider} diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index 96c2f6e..ef87bfc 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -280,7 +280,6 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact RequireOpinionatedReviewAuthority: !flags.dryRun, Retention: appruntime.RetentionPolicyFromConfig(cfg.Data.Retention), RetentionManualOnly: cfg.Data.Retention.Enforcement == config.RetentionManualOnly, - ResolveRepoRoot: appruntime.ResolveRepoRoot, } runtimeSpan := logger.Start("review", "build_runtime", "runtime") runtime, err := factory(ctx, runtimeReq) diff --git a/internal/cmd/reviewcmd/reviewcmd_test.go b/internal/cmd/reviewcmd/reviewcmd_test.go index 610dff9..71396d6 100644 --- a/internal/cmd/reviewcmd/reviewcmd_test.go +++ b/internal/cmd/reviewcmd/reviewcmd_test.go @@ -8,7 +8,6 @@ import ( "fmt" "os" "path/filepath" - "reflect" "strconv" "strings" "testing" @@ -864,12 +863,6 @@ func TestReviewPassesRetentionConfigToRuntimeFactory(t *testing.T) { if got.Retention.LiveForever != tt.wantForever || got.Retention.LiveMaxAge != tt.wantLiveMaxAge || got.RetentionManualOnly != tt.wantManualOnly { t.Fatalf("runtime retention = %#v manual %v, want forever=%v max_age=%s manual=%v", got.Retention, got.RetentionManualOnly, tt.wantForever, tt.wantLiveMaxAge, tt.wantManualOnly) } - if got.ResolveRepoRoot == nil { - t.Fatal("runtime ResolveRepoRoot is nil") - } - if gotPtr, wantPtr := reflect.ValueOf(got.ResolveRepoRoot).Pointer(), reflect.ValueOf(appruntime.ResolveRepoRoot).Pointer(); gotPtr != wantPtr { - t.Fatalf("ResolveRepoRoot pointer = %x, want %x", gotPtr, wantPtr) - } }) } } diff --git a/internal/gitexec/client.go b/internal/gitexec/client.go new file mode 100644 index 0000000..289d977 --- /dev/null +++ b/internal/gitexec/client.go @@ -0,0 +1,117 @@ +// Package gitexec runs authenticated, non-interactive Git commands. +package gitexec + +import ( + "context" + "encoding/base64" + "fmt" + "net/url" + "os/exec" + "strings" +) + +// TokenSource supplies the current repository access token. +type TokenSource interface { + AccessToken(context.Context) (string, error) +} + +// TokenSourceFunc adapts a function to TokenSource. +type TokenSourceFunc func(context.Context) (string, error) + +// AccessToken returns the current token. +func (f TokenSourceFunc) AccessToken(ctx context.Context) (string, error) { return f(ctx) } + +// Client runs Git with host-scoped HTTPS authentication. +type Client struct { + host string + tokens TokenSource +} + +// New constructs an authenticated Git client. +func New(host string, tokens TokenSource) (*Client, error) { + host = strings.TrimSpace(strings.TrimSuffix(host, "/")) + if host == "" || strings.Contains(host, "://") || strings.Contains(host, "/") { + return nil, fmt.Errorf("gitexec: valid host is required") + } + if tokens == nil { + return nil, fmt.Errorf("gitexec: token source is required") + } + return &Client{host: host, tokens: tokens}, nil +} + +// Run executes Git in dir, or the current directory when dir is empty. +func (c *Client) Run(ctx context.Context, dir string, args ...string) ([]byte, error) { + header := "" + token := "" + if requiresAuthentication(c.host, args) { + var err error + token, err = c.tokens.AccessToken(ctx) + if err != nil { + return nil, fmt.Errorf("gitexec: read access token: %w", err) + } + token = strings.TrimSpace(token) + if token == "" { + return nil, fmt.Errorf("gitexec: access token is empty") + } + header = "Authorization: Basic " + base64.StdEncoding.EncodeToString([]byte("x-access-token:"+token)) + } + cmd := exec.CommandContext(ctx, "git", args...) // #nosec G204 -- fixed git binary with structured arguments. + if strings.TrimSpace(dir) != "" { + cmd.Dir = dir + } + cmd.Env = gitEnvironment(cmd.Environ(), c.host, header) + output, runErr := cmd.CombinedOutput() + if runErr == nil { + return output, nil + } + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + detail := strings.TrimSpace(string(output)) + if detail == "" { + detail = runErr.Error() + } + if token != "" { + detail = strings.ReplaceAll(detail, token, "[REDACTED]") + } + if header != "" { + detail = strings.ReplaceAll(detail, header, "[REDACTED]") + } + return nil, fmt.Errorf("git %s: %s", strings.Join(args, " "), detail) +} + +func requiresAuthentication(host string, args []string) bool { + if len(args) < 2 || args[0] != "fetch" { + return false + } + for _, arg := range args[1:] { + if strings.HasPrefix(arg, "-") { + continue + } + remote, err := url.Parse(arg) + return err == nil && remote.Scheme == "https" && strings.EqualFold(remote.Host, host) + } + return false +} + +func gitEnvironment(base []string, host, header string) []string { + env := make([]string, 0, len(base)+6) + for _, entry := range base { + key, _, _ := strings.Cut(entry, "=") + if key == "GIT_CONFIG_COUNT" || strings.HasPrefix(key, "GIT_CONFIG_KEY_") || strings.HasPrefix(key, "GIT_CONFIG_VALUE_") || key == "GIT_TERMINAL_PROMPT" || key == "LC_ALL" || key == "LANG" { + continue + } + env = append(env, entry) + } + env = append(env, + "GIT_TERMINAL_PROMPT=0", + "LC_ALL=C", + "LANG=C") + if header != "" { + env = append(env, + "GIT_CONFIG_COUNT=1", + "GIT_CONFIG_KEY_0=http.https://"+host+"/.extraHeader", + "GIT_CONFIG_VALUE_0="+header) + } + return env +} diff --git a/internal/gitexec/client_test.go b/internal/gitexec/client_test.go new file mode 100644 index 0000000..58851bf --- /dev/null +++ b/internal/gitexec/client_test.go @@ -0,0 +1,145 @@ +//go:build !windows + +package gitexec + +import ( + "context" + "encoding/base64" + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +type rotatingTokens struct { + values []string + calls int + err error +} + +func (s *rotatingTokens) AccessToken(context.Context) (string, error) { + if s.err != nil { + return "", s.err + } + value := s.values[s.calls] + s.calls++ + return value, nil +} + +func TestClientLocalCommandsDoNotReadToken(t *testing.T) { + dir := t.TempDir() + script := filepath.Join(dir, "git") + if err := os.WriteFile(script, []byte("#!/bin/sh\nexit 0\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(script, 0o700); err != nil { // #nosec G302 -- executable test fixture. + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + tokens := &rotatingTokens{err: errors.New("refresh unavailable")} + client, err := New("github.example.com", tokens) + if err != nil { + t.Fatalf("New: %v", err) + } + for _, args := range [][]string{{"status", "--porcelain"}, {"clone", dir, filepath.Join(dir, "clone")}} { + if _, err := client.Run(context.Background(), "", args...); err != nil { + t.Fatalf("local Run %v: %v", args, err) + } + } + if tokens.calls != 0 { + t.Fatalf("token calls = %d, want none for local Git", tokens.calls) + } +} + +func TestClientPreservesFailingLocalCommandDiagnostic(t *testing.T) { + dir := t.TempDir() + script := filepath.Join(dir, "git") + if err := os.WriteFile(script, []byte("#!/bin/sh\nprintf 'local failure' >&2\nexit 1\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(script, 0o700); err != nil { // #nosec G302 -- executable test fixture. + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + client, err := New("github.example.com", &rotatingTokens{err: errors.New("refresh unavailable")}) + if err != nil { + t.Fatalf("New: %v", err) + } + _, err = client.Run(context.Background(), dir, "status") + if err == nil || !strings.Contains(err.Error(), "local failure") { + t.Fatalf("Run error = %v, want intact local diagnostic", err) + } +} + +func TestClientUsesFreshHostScopedTokenWithoutLeakingArguments(t *testing.T) { + dir := t.TempDir() + record := filepath.Join(dir, "record") + script := filepath.Join(dir, "git") + body := "#!/bin/sh\nprintf '%s\\n' \"$@\" > \"$GITEXEC_RECORD.args\"\nenv > \"$GITEXEC_RECORD.env\"\n" + if err := os.WriteFile(script, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(script, 0o700); err != nil { // #nosec G302 -- executable test fixture. + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("GITEXEC_RECORD", record) + + tokens := &rotatingTokens{values: []string{"first-secret", "second-secret"}} + client, err := New("github.example.com", tokens) + if err != nil { + t.Fatalf("New: %v", err) + } + for range 2 { + if _, err := client.Run(context.Background(), "", "fetch", "https://github.example.com/acme/repo.git"); err != nil { + t.Fatalf("Run: %v", err) + } + } + + args, err := os.ReadFile(record + ".args") // #nosec G304 -- test-controlled temp path. + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(args), "secret") { + t.Fatalf("arguments leaked token: %s", args) + } + env, err := os.ReadFile(record + ".env") // #nosec G304 -- test-controlled temp path. + if err != nil { + t.Fatal(err) + } + got := string(env) + if !strings.Contains(got, "GIT_TERMINAL_PROMPT=0") || !strings.Contains(got, "GIT_CONFIG_KEY_0=http.https://github.example.com/.extraHeader") { + t.Fatalf("git environment missing safety config: %s", got) + } + wantAuth := base64.StdEncoding.EncodeToString([]byte("x-access-token:second-secret")) + if !strings.Contains(got, wantAuth) { + t.Fatalf("git environment did not use refreshed token: %s", got) + } +} + +func TestClientRedactsTokenFromGitError(t *testing.T) { + dir := t.TempDir() + script := filepath.Join(dir, "git") + body := "#!/bin/sh\nenv | grep GIT_CONFIG_VALUE_0 >&2\nexit 1\n" + if err := os.WriteFile(script, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(script, 0o700); err != nil { // #nosec G302 -- executable test fixture. + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + + const token = "distinct-secret-token" + client, err := New("github.example.com", TokenSourceFunc(func(context.Context) (string, error) { return token, nil })) + if err != nil { + t.Fatalf("New: %v", err) + } + _, err = client.Run(context.Background(), "", "fetch", "https://github.example.com/acme/repo.git") + if err == nil { + t.Fatal("Run succeeded, want git failure") + } + if strings.Contains(err.Error(), token) || strings.Contains(err.Error(), base64.StdEncoding.EncodeToString([]byte("x-access-token:"+token))) { + t.Fatalf("Run error leaked token: %v", err) + } +} diff --git a/internal/gitprovider/github/app_auth_test.go b/internal/gitprovider/github/app_auth_test.go index 0825184..49fdc08 100644 --- a/internal/gitprovider/github/app_auth_test.go +++ b/internal/gitprovider/github/app_auth_test.go @@ -87,6 +87,13 @@ func TestNewFromGitConfigBuildsGitHubAppClientAndRefreshesToken(t *testing.T) { t.Fatalf("GetPR first: %v", err) } now = now.Add(59 * time.Minute) + gitToken, err := client.AccessToken(context.Background()) + if err != nil { + t.Fatalf("AccessToken refresh: %v", err) + } + if gitToken != "installation-token-2" { + t.Fatalf("AccessToken = %q, want refreshed installation token", gitToken) + } if _, err := client.GetPR(context.Background(), ref); err != nil { t.Fatalf("GetPR refresh: %v", err) } diff --git a/internal/gitprovider/github/client.go b/internal/gitprovider/github/client.go index 161df5c..586fe6b 100644 --- a/internal/gitprovider/github/client.go +++ b/internal/gitprovider/github/client.go @@ -154,6 +154,12 @@ func (c *Client) Host() string { return c.host } +// AccessToken returns the current repository access token. GitHub App clients +// refresh their installation token through the same path used by API calls. +func (c *Client) AccessToken(ctx context.Context) (string, error) { + return c.bearerToken(ctx, "") +} + // Capabilities returns GitHub feature support. func (c *Client) Capabilities() gitprovider.ProviderCaps { return gitprovider.ProviderCaps{ diff --git a/internal/pipeline/failure.go b/internal/pipeline/failure.go new file mode 100644 index 0000000..179bead --- /dev/null +++ b/internal/pipeline/failure.go @@ -0,0 +1,49 @@ +package pipeline + +import ( + "errors" + "fmt" +) + +// FailureKind describes whether a live planning failure can be resumed safely. +type FailureKind uint8 + +const ( + // FailureTransient is an attempt-scoped failure that may succeed on retry. + FailureTransient FailureKind = iota + // FailureDurableBlocking preserves resumable task state. + FailureDurableBlocking + // FailureTerminal cannot be resumed safely. + FailureTerminal +) + +type classifiedFailure struct { + kind FailureKind + err error +} + +func (e *classifiedFailure) Error() string { return e.err.Error() } +func (e *classifiedFailure) Unwrap() error { return e.err } + +// ClassifyFailure returns the recovery class for err. +func ClassifyFailure(err error) FailureKind { + var classified *classifiedFailure + if errors.As(err, &classified) { + return classified.kind + } + if errors.Is(err, errLLMTaskFailedBlocking) { + return FailureDurableBlocking + } + return FailureTransient +} + +// Failure marks err with a recovery class. +func Failure(kind FailureKind, err error) error { + if err == nil { + return nil + } + if kind > FailureTerminal { + return fmt.Errorf("pipeline: invalid failure kind %d: %w", kind, err) + } + return &classifiedFailure{kind: kind, err: err} +} diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index e8093bd..5ce064d 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -173,6 +173,7 @@ type SelectionRequest struct { ArtifactDir string ReviewBaseSHA string ReviewHeadSHA string + MaxAgents int SelectionModelOverride string SelectionEffortOverride string @@ -392,19 +393,19 @@ func DryRun(ctx context.Context, opts Options, req Request) (Result, error) { // Live executes the review planning phases into a gate-allocated live run. func Live(ctx context.Context, opts Options, req Request, run ledger.Run) (Result, error) { if hasDryRunStageOverrides(req) { - return Result{}, fmt.Errorf("pipeline: selection and reviewer overrides require dry-run review") + return Result{}, Failure(FailureTerminal, fmt.Errorf("pipeline: selection and reviewer overrides require dry-run review")) } if strings.TrimSpace(req.ReviewBaseSHA) != "" || strings.TrimSpace(req.ReviewHeadSHA) != "" { - return Result{}, fmt.Errorf("pipeline: pinned review SHAs require dry-run review") + return Result{}, Failure(FailureTerminal, fmt.Errorf("pipeline: pinned review SHAs require dry-run review")) } if strings.TrimSpace(run.RunID) == "" { - return Result{}, fmt.Errorf("pipeline: live run ID is required") + return Result{}, Failure(FailureTerminal, fmt.Errorf("pipeline: live run ID is required")) } if strings.TrimSpace(run.ArtifactPath) == "" { - return Result{}, fmt.Errorf("pipeline: live artifact path is required") + return Result{}, Failure(FailureTerminal, fmt.Errorf("pipeline: live artifact path is required")) } if run.PostMode != ledger.PostModeLive { - return Result{}, fmt.Errorf("pipeline: live run post mode is required") + return Result{}, Failure(FailureTerminal, fmt.Errorf("pipeline: live run post mode is required")) } return execute(ctx, opts, req, executionMode{ live: true, @@ -425,6 +426,13 @@ func SelectionOnly(ctx context.Context, opts Options, req SelectionRequest) (Sel if err := agents.RequireSafeProfileSources(req.Profile.AgentSources, invocationRoot); err != nil { return SelectionResult{}, err } + reviewCtx, err := resolveReviewPRContext(ctx, opts.Provider, req.PRRef, req.ReviewBaseSHA, req.ReviewHeadSHA) + if err != nil { + return SelectionResult{}, err + } + if err := validateRepositoryBinding(req.PRRef, req.Profile.Git.Host, reviewCtx.reviewPR); err != nil { + return SelectionResult{}, err + } prepared, err := prepareSelectionContext(ctx, opts, selectionSetupRequest{ PRRef: req.PRRef, @@ -434,6 +442,7 @@ func SelectionOnly(ctx context.Context, opts Options, req SelectionRequest) (Sel ReviewBaseSHA: req.ReviewBaseSHA, ReviewHeadSHA: req.ReviewHeadSHA, NoResolveThreads: false, + ResolvedPR: &reviewCtx, InvocationRoot: &invocationRoot, ResolveArtifacts: func(gitprovider.PR) (ArtifactPaths, error) { return ArtifactPathsFromDir(req.ArtifactDir), nil @@ -442,7 +451,6 @@ func SelectionOnly(ctx context.Context, opts Options, req SelectionRequest) (Sel if err != nil { return SelectionResult{}, err } - result := prepared.selectionResult() if err := workbench.Prepare(ctx, workbenchDeps(opts), workbench.Request{ PRRef: req.PRRef, @@ -475,7 +483,7 @@ func SelectionOnly(ctx context.Context, opts Options, req SelectionRequest) (Sel Threads: prepared.threads, ThreadContext: prepared.threadContext, Artifacts: prepared.artifacts, - MaxAgents: opts.maxAgents(), + MaxAgents: selectionMaxAgents(req.MaxAgents, opts.maxAgents()), }) result.SelectionSession = selectionSessionFromDraft(session) if err != nil { @@ -485,8 +493,18 @@ func SelectionOnly(ctx context.Context, opts Options, req SelectionRequest) (Sel return result, nil } +func selectionMaxAgents(requested, fallback int) int { + if requested > 0 { + return requested + } + return fallback +} + func execute(ctx context.Context, opts Options, req Request, mode executionMode) (out Result, err error) { if err := validate(opts, req); err != nil { + if mode.live { + return Result{}, Failure(FailureTerminal, err) + } return Result{}, err } invocationRoot, err := resolveInvocationRootForSafety(ctx, opts) @@ -494,17 +512,13 @@ func execute(ctx context.Context, opts Options, req Request, mode executionMode) return Result{}, err } if err := agents.RequireSafeProfileSources(req.Profile.AgentSources, invocationRoot); err != nil { + if mode.live { + return Result{}, Failure(FailureTerminal, err) + } return Result{}, err } completed := false failureOutcome := ledger.OutcomeFailed - if mode.live { - defer func() { - if !completed && !isContextError(err) { - _ = opts.Store.CompleteRun(context.Background(), mode.run.RunID, failureOutcome, opts.now()) - } - }() - } now := opts.now() maxAgents := opts.maxAgents() maxConcurrency := opts.maxConcurrency(maxAgents) @@ -516,6 +530,12 @@ func execute(ctx context.Context, opts Options, req Request, mode executionMode) if err != nil { return Result{}, err } + if err := validateRepositoryBinding(req.PRRef, req.Profile.Git.Host, reviewCtx.reviewPR); err != nil { + if mode.live { + return Result{}, Failure(FailureTerminal, err) + } + return Result{}, err + } var resumedDryRun *ledger.Run if !mode.live && !req.Rerun { run, ok, err := findIncompleteDryRun(ctx, opts.Store, req, reviewCtx.reviewPR) @@ -597,6 +617,9 @@ func execute(ctx context.Context, opts Options, req Request, mode executionMode) ChangedFiles: prepared.changedFiles, Artifacts: prepared.artifacts, }); err != nil { + if errors.Is(err, workbench.ErrUnsafeFetchRef) || errors.Is(err, workbench.ErrInvalidRepositoryIdentity) { + return Result{}, Failure(FailureTerminal, err) + } return Result{}, err } if err := dossier.Prepare(ctx, dossierEnv(opts), dossier.PreparationRequest{ @@ -667,7 +690,7 @@ func executePlanPhases(ctx context.Context, opts Options, req Request, mode exec func executeLLMPhases(ctx context.Context, opts Options, req Request, mode executionMode, run ledger.Run, prepared preparedSelectionContext, repoSources []agents.SourceInfo, now time.Time, maxAgents, maxConcurrency int, result *Result) (map[review.FindingID]string, bool, error) { runtimeConfig, err := resolveSelectionRuntimeConfig(req.Profile, req.SelectionModelOverride, req.SelectionEffortOverride) if err != nil { - return nil, false, err + return nil, false, Failure(FailureTerminal, err) } namedSession, err := prepareNamedSession(ctx, opts, req, mode.live, runtimeConfig.model, now) if err != nil { @@ -715,15 +738,15 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu rollupRuntimeConfig, err := resolveSynthesisRuntimeConfig(req) if err != nil { - return nil, false, err + return nil, false, Failure(FailureTerminal, err) } rollupModel, rollupEffort := rollupRuntimeConfig.model, rollupRuntimeConfig.effort rollupPrompt, err := buildRollupPrompt(prepared.reviewPR, findings, reviewerFailures, reviewerCoverage) if err != nil { - return nil, false, err + return nil, false, Failure(FailureTerminal, err) } if err := opts.checkPromptBudget("rollup", "", rollupModel, "", rollupPrompt); err != nil { - return nil, false, err + return nil, false, Failure(FailureTerminal, err) } rollupLog, err := prepared.artifacts.AgentLog(orchestratorRollupStage) if err != nil { @@ -856,7 +879,7 @@ func persistedPlanning(ctx context.Context, store Store, runID string) ([]ledger return nil, nil, false, nil } if len(actions) == 0 { - return nil, nil, false, fmt.Errorf("pipeline: persisted planning for run %s has findings but no planned actions", runID) + return nil, nil, false, Failure(FailureTerminal, fmt.Errorf("pipeline: persisted planning for run %s has findings but no planned actions", runID)) } return findings, actions, true, nil } @@ -1077,7 +1100,7 @@ func resolveReviewPRContext(ctx context.Context, provider ReadProvider, ref gitp func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequest) (llm.Selection, sessionDraft, ledger.Session, error) { runtimeConfig, err := resolveSelectionRuntimeConfig(req.Profile, req.SelectionModelOverride, req.SelectionEffortOverride) if err != nil { - return llm.Selection{}, sessionDraft{}, ledger.Session{}, err + return llm.Selection{}, sessionDraft{}, ledger.Session{}, Failure(FailureTerminal, err) } model, effort := runtimeConfig.model, runtimeConfig.effort @@ -1094,10 +1117,10 @@ func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequ fingerprintDeps := append(append([]string(nil), dependencyTaskIDs...), promptDeps...) selectionPrompt, err := buildSelectionPrompt(req.Catalog, promptInput, req.MaxAgents, req.SelectionPromptInstructions) if err != nil { - return llm.Selection{}, sessionDraft{}, ledger.Session{}, err + return llm.Selection{}, sessionDraft{}, ledger.Session{}, Failure(FailureTerminal, err) } if err := opts.checkPromptBudget("selection", "", model, "", selectionPrompt); err != nil { - return llm.Selection{}, sessionDraft{}, ledger.Session{}, err + return llm.Selection{}, sessionDraft{}, ledger.Session{}, Failure(FailureTerminal, err) } selectionLog, err := req.Artifacts.AgentLog(orchestratorSelectionStage) if err != nil { @@ -1198,10 +1221,6 @@ func validatePinnedReviewPR(ref gitprovider.PRRef, pr gitprovider.PR) error { return nil } -func isContextError(err error) bool { - return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) -} - func sessionRowIDForFinding(finding reviewplan.AnchoredFinding, findingSession map[review.FindingID]string) (string, error) { rowID := strings.TrimSpace(findingSession[finding.FindingID]) if rowID == "" { @@ -1303,17 +1322,17 @@ func runReviewers(ctx context.Context, opts Options, req Request, runID string, func runReviewer(ctx context.Context, opts Options, req Request, runID string, pr gitprovider.PR, parsed ParsedDiff, artifacts ArtifactPaths, selected llm.SelectedAgent, agent agents.Agent, dependencyTaskIDs []string) (llm.Findings, sessionDraft, ledger.Session, *ReviewerFailure, error) { runtimeConfig, err := resolveReviewerRuntimeConfig(req, agent) if err != nil { - return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, err + return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, Failure(FailureTerminal, err) } model, effort := runtimeConfig.model, runtimeConfig.effort changedFilePaths := patchPaths(parsed.Patches) assignmentScope := reviewerAssignmentScope(selected, changedFilePaths) prompt, promptDeps, err := buildReviewerPrompt(artifacts, pr, selected, agent, changedFilePaths) if err != nil { - return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, err + return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, Failure(FailureTerminal, err) } if err := opts.checkPromptBudget("reviewer", agent.ID, model, strings.Join(selected.Files, ","), prompt); err != nil { - return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, err + return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, Failure(FailureTerminal, err) } logPath, err := artifacts.AgentLog(agent.ID) if err != nil { @@ -1881,7 +1900,7 @@ func workstreamUsage(name string, draft sessionDraft) reviewplan.WorkstreamUsage func (opts Options) buildPlan(req Request, pr gitprovider.PR, postMode reviewplan.PostMode, caps reviewplan.ProviderCaps, diff reviewplan.Diff, findings []review.Finding, rollup review.Rollup, threadActions []review.ThreadAction, noDiff bool, agentDefsChanged bool, runInputs planRunInputs) (reviewplan.Plan, error) { runSummary, findingReviewers := opts.buildRunSummary(req, runInputs) - return reviewplan.Build(reviewplan.Request{ + plan, err := reviewplan.Build(reviewplan.Request{ PostMode: postMode, ProviderCaps: caps, Diff: diff, @@ -1906,6 +1925,10 @@ func (opts Options) buildPlan(req Request, pr gitprovider.PR, postMode reviewpla Now: opts.now, NewActionID: opts.newActionID, }) + if err != nil { + return reviewplan.Plan{}, Failure(FailureTerminal, err) + } + return plan, nil } func (opts Options) emitWarning(warning string) { @@ -2041,6 +2064,22 @@ func validateReviewSHAs(baseSHA, headSHA string) error { return nil } +func validateRepositoryBinding(ref gitprovider.PRRef, configuredHost string, pr gitprovider.PR) error { + if !strings.EqualFold(strings.TrimSpace(configuredHost), strings.TrimSpace(ref.Host)) { + return fmt.Errorf("pipeline: configured git host %q does not match PR host %q", configuredHost, ref.Host) + } + if !strings.EqualFold(pr.Ref.Host, ref.Host) || !strings.EqualFold(pr.Ref.Owner, ref.Owner) || !strings.EqualFold(pr.Ref.Repo, ref.Repo) || pr.Ref.Number != ref.Number { + return fmt.Errorf("pipeline: fetched PR identity does not match requested PR %s/%s#%d on %s", ref.Owner, ref.Repo, ref.Number, ref.Host) + } + if !strings.EqualFold(pr.Base.Host, ref.Host) || !strings.EqualFold(pr.Base.Owner, ref.Owner) || !strings.EqualFold(pr.Base.Repo, ref.Repo) { + return fmt.Errorf("pipeline: PR base repository does not match requested repository %s/%s on %s", ref.Owner, ref.Repo, ref.Host) + } + if !strings.EqualFold(pr.Head.Host, ref.Host) { + return fmt.Errorf("pipeline: cross-host PR head %s/%s on %s is unsupported", pr.Head.Owner, pr.Head.Repo, pr.Head.Host) + } + return nil +} + // ArtifactPathsForRun returns the artifact paths for a generated run ID. func ArtifactPathsForRun(layout statepaths.Layout, ref gitprovider.PRRef, pr gitprovider.PR, profile, postingIdentity, runID string) (ArtifactPaths, error) { return runartifact.ForRun(layout, ref, pr, profile, postingIdentity, runID) @@ -2165,7 +2204,7 @@ func (opts Options) resolveRepoRoot(ctx context.Context) (string, error) { } func workbenchDeps(opts Options) workbench.Deps { - return workbench.Deps{GitCommand: opts.GitCommand, ResolveRepoRoot: opts.ResolveRepoRoot} + return workbench.Deps{GitCommand: opts.GitCommand} } func dossierEnv(opts Options) dossier.Env { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 1b36836..64f6b1c 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -71,6 +71,150 @@ func TestResolveInvocationRootForSafetyTreatsUnavailableAsUnknownAndOtherErrorsA } } +func TestValidateRepositoryBinding(t *testing.T) { + ref := gitprovider.PRRef{Host: "github.com", Owner: "acme", Repo: "widgets", Number: 42} + valid := gitprovider.PR{ + Ref: ref, + Base: gitprovider.PRBranchRef{Host: ref.Host, Owner: ref.Owner, Repo: ref.Repo}, + Head: gitprovider.PRBranchRef{Host: ref.Host, Owner: "contributor", Repo: ref.Repo}, + } + tests := []struct { + name string + host string + pr gitprovider.PR + }{ + {name: "valid same-host fork", host: ref.Host, pr: valid}, + {name: "configured host mismatch", host: "git.example.com", pr: valid}, + {name: "fetched identity mismatch", host: ref.Host, pr: func() gitprovider.PR { pr := valid; pr.Ref.Number++; return pr }()}, + {name: "base identity mismatch", host: ref.Host, pr: func() gitprovider.PR { pr := valid; pr.Base.Repo = "other"; return pr }()}, + {name: "cross-host head", host: ref.Host, pr: func() gitprovider.PR { pr := valid; pr.Head.Host = "git.example.com"; return pr }()}, + {name: "mixed-case identity", host: "GitHub.com", pr: func() gitprovider.PR { + pr := valid + pr.Ref.Owner, pr.Ref.Repo = "ACME", "Widgets" + pr.Base.Owner, pr.Base.Repo = "ACME", "Widgets" + return pr + }()}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateRepositoryBinding(ref, tt.host, tt.pr) + valid := tt.name == "valid same-host fork" || tt.name == "mixed-case identity" + if valid && err != nil { + t.Fatalf("validateRepositoryBinding: %v", err) + } + if !valid && err == nil { + t.Fatal("validateRepositoryBinding succeeded, want rejection") + } + }) + } +} + +func TestSelectionOnlyRejectsRepositoryMismatchBeforeGit(t *testing.T) { + provider, req := dryRunHarness(t) + req.Profile.Git.Host = "git.example.com" + gitCalls := 0 + artifactDir := t.TempDir() + _, err := selectionOnlyForTest(context.Background(), Options{ + Provider: provider, + Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, + GitCommand: func(context.Context, string, ...string) ([]byte, error) { + gitCalls++ + return nil, errors.New("unexpected git call") + }, + }, selectionRequestFromReview(req, artifactDir)) + if err == nil || !strings.Contains(err.Error(), "configured git host") { + t.Fatalf("SelectionOnly error = %v, want configured host mismatch", err) + } + if gitCalls != 0 { + t.Fatalf("Git calls = %d, want zero before trust binding", gitCalls) + } + entries, readErr := os.ReadDir(artifactDir) + if readErr != nil { + t.Fatalf("ReadDir artifacts: %v", readErr) + } + if len(entries) != 0 { + t.Fatalf("artifacts = %#v, want no writes before trust binding", entries) + } +} + +func TestLiveClassifiesRepositoryBindingFailureAsTerminalBeforeGit(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + run := allocateLiveRun(t, store, provider, req, "terminal-binding") + req.Profile.Git.Host = "git.example.com" + gitCalls := 0 + _, err := Live(ctx, Options{ + Provider: provider, + Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, + Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + ResolveRepoRoot: func(context.Context) (string, error) { return "", reporoot.ErrUnavailable }, + GitCommand: func(context.Context, string, ...string) ([]byte, error) { + gitCalls++ + return nil, errors.New("unexpected git call") + }, + }, req, run) + if err == nil || ClassifyFailure(err) != FailureTerminal { + t.Fatalf("Live error = %v kind %v, want terminal repository binding failure", err, ClassifyFailure(err)) + } + if gitCalls != 0 { + t.Fatalf("Git calls = %d, want zero before trust binding", gitCalls) + } +} + +func TestLiveClassifiesUnsafeFetchRefAsTerminal(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + provider.pr.Base.Ref = "main" + run := allocateLiveRun(t, store, provider, req, "terminal-ref") + _, err := liveForTest(ctx, Options{ + Provider: provider, + Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, + Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + }, req, run) + if err == nil || ClassifyFailure(err) != FailureTerminal { + t.Fatalf("Live error = %v kind %v, want terminal unsafe-ref failure", err, ClassifyFailure(err)) + } +} + +func TestBuildPlanClassifiesActionIDFailureTerminalAcrossPaths(t *testing.T) { + wantErr := errors.New("action ID unavailable") + opts := Options{ + Now: fixedNow, + NewActionID: func(reviewplan.ActionKind) (string, error) { + return "", wantErr + }, + } + req := Request{ + ProfileName: "home", + PostingIdentity: gitprovider.Identity{Login: "review-bot", ID: "bot-id"}, + } + pr := gitprovider.PR{Head: gitprovider.PRBranchRef{SHA: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}} + tests := []struct { + name string + noDiff bool + rollup review.Rollup + sources []agents.SourceInfo + }{ + {name: "no diff", noDiff: true}, + {name: "repo guidance unavailable", sources: []agents.SourceInfo{{Kind: agents.SourceRepo, Status: agents.SourceStatusMissing}}}, + {name: "normal review", rollup: review.Rollup{ReviewEvent: review.ReviewEventComment}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := opts.buildPlan(req, pr, reviewplan.PostModeLive, reviewplan.ProviderCaps{}, reviewplan.Diff{}, nil, tt.rollup, nil, tt.noDiff, false, planRunInputs{repoSources: tt.sources}) + if !errors.Is(err, wantErr) || ClassifyFailure(err) != FailureTerminal { + t.Fatalf("buildPlan error = %v kind %v, want terminal action-ID failure", err, ClassifyFailure(err)) + } + }) + } +} + func TestReviewPipelineAcceptanceHarnessDryRunWithFakes(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) @@ -267,7 +411,7 @@ func TestReviewPipelineAcceptanceHarnessDryRunWithFakes(t *testing.T) { assertFileContains(t, result.Artifacts.DiffPatch, "diff --git a/main.go b/main.go") assertFileContains(t, result.Artifacts.FindingsJSON, `"severity": "major"`) assertFileContains(t, result.Artifacts.RollupMarkdown, "Automated PR Review") - assertFileContains(t, result.Artifacts.WorkbenchMetadataPath(), `"schema_version": 1`) + assertFileContains(t, result.Artifacts.WorkbenchMetadataPath(), `"schema_version": 2`) assertFileContains(t, result.Artifacts.WorkbenchMetadataPath(), `"repo_path":`) assertAgentSourcesArtifact(t, result.Artifacts.AgentSourcesJSON, "harness:reviewer") assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "pr-intent.md"), "Document the checkout-native review contract.") @@ -1095,13 +1239,14 @@ func TestSelectionOnlyCapsMaxAgents(t *testing.T) { "reasoning": "too many" }`, 10, 2)) + selectionReq := selectionRequestFromReview(req, t.TempDir()) + selectionReq.MaxAgents = 1 result, err := selectionOnlyForTest(ctx, Options{ - Provider: provider, - Adapter: adapter, - Now: fixedNow, - MaxAgents: 1, - Warnings: &warnings, - }, selectionRequestFromReview(req, t.TempDir())) + Provider: provider, + Adapter: adapter, + Now: fixedNow, + Warnings: &warnings, + }, selectionReq) if err != nil { t.Fatalf("SelectionOnly: %v", err) } @@ -1781,8 +1926,8 @@ func TestLiveResumeCompletedSelectionAndReviewersRerunsOnlyRollup(t *testing.T) if err != nil { t.Fatalf("GetRun: %v", err) } - if stored.Outcome == nil || *stored.Outcome != ledger.OutcomeIncomplete { - t.Fatalf("run outcome = %#v, want incomplete after rollup task failure", stored.Outcome) + if stored.Outcome != nil { + t.Fatalf("run outcome = %#v, want pipeline to leave live outcome ownership to reviewrun", stored.Outcome) } selectionMeta, ok, err := llmlifecycle.ReadMetadata(lifecyclePaths(ArtifactPathsFromDir(run.ArtifactPath)), orchestratorSelectionStage) if err != nil || !ok || selectionMeta.Status != llmTaskStatusSucceeded { @@ -3327,12 +3472,16 @@ func TestLiveMarksRunIncompleteAfterBlockingLLMTaskError(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "no queued result") { t.Fatalf("Live error = %v, want fake LLM planning error", err) } + plannerErr := err storedRun, err := store.GetRun(ctx, run.RunID) if err != nil { t.Fatalf("GetRun: %v", err) } - if storedRun.Outcome == nil || *storedRun.Outcome != ledger.OutcomeIncomplete { - t.Fatalf("stored outcome = %#v, want incomplete", storedRun.Outcome) + if storedRun.Outcome != nil { + t.Fatalf("stored outcome = %#v, want pipeline to leave live outcome ownership to reviewrun", storedRun.Outcome) + } + if got := ClassifyFailure(plannerErr); got != FailureDurableBlocking { + t.Fatalf("failure kind = %v, want durable blocking", got) } } @@ -4791,16 +4940,17 @@ func configureWorkbenchFixtureForTest(_ context.Context, opts *Options, ref gitp } } if opts.GitCommand == nil { - opts.GitCommand = workbenchGitCommandForTest(ref) + opts.GitCommand = workbenchGitCommandForTest(ref, repoDir) } } -func workbenchGitCommandForTest(ref gitprovider.PRRef) func(context.Context, string, ...string) ([]byte, error) { +func workbenchGitCommandForTest(ref gitprovider.PRRef, repoDir string) 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 + cmdArgs := append([]string(nil), args...) + if len(cmdArgs) >= 3 && cmdArgs[0] == "fetch" && cmdArgs[2] == fmt.Sprintf("https://%s/%s/%s.git", ref.Host, ref.Owner, ref.Repo) { + cmdArgs[2] = repoDir } - cmd := exec.CommandContext(ctx, "git", args...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + 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 } @@ -4813,7 +4963,7 @@ func workbenchGitCommandForTest(ref gitprovider.PRRef) func(context.Context, str if message == "" { message = err.Error() } - return nil, fmt.Errorf("git %s: %s", strings.Join(args, " "), message) + return nil, fmt.Errorf("git %s: %s", strings.Join(cmdArgs, " "), message) } return out, nil } diff --git a/internal/pipeline/workbench_integration_test.go b/internal/pipeline/workbench_integration_test.go index 775e179..a788a70 100644 --- a/internal/pipeline/workbench_integration_test.go +++ b/internal/pipeline/workbench_integration_test.go @@ -11,12 +11,14 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/gitprovider" "github.com/open-cli-collective/codereview-cli/internal/ledger" "github.com/open-cli-collective/codereview-cli/internal/llm" + "github.com/open-cli-collective/codereview-cli/internal/reporoot" "github.com/open-cli-collective/codereview-cli/internal/statepaths" "github.com/open-cli-collective/codereview-cli/internal/workbench" ) func TestSelectionOnlyPreparesWorkbenchInCallerOwnedArtifacts(t *testing.T) { ctx := context.Background() + t.Chdir(t.TempDir()) fixture := newWorkbenchGitFixture(t) provider, req := dryRunHarness(t) provider.pr = fixture.pr @@ -29,12 +31,11 @@ func TestSelectionOnlyPreparesWorkbenchInCallerOwnedArtifacts(t *testing.T) { artifactDir := t.TempDir() result, err := selectionOnlyForTest(ctx, Options{ - Provider: provider, - Adapter: adapter, - Now: fixedNow, - ResolveRepoRoot: func(context.Context) (string, error) { - return fixture.repoDir, nil - }, + Provider: provider, + Adapter: adapter, + Now: fixedNow, + GitCommand: workbenchGitCommandForTest(req.PRRef, fixture.repoDir), + ResolveRepoRoot: func(context.Context) (string, error) { return "", reporoot.ErrUnavailable }, }, selectionRequestFromReview(req, artifactDir)) if err != nil { t.Fatalf("SelectionOnly: %v", err) @@ -53,6 +54,9 @@ func TestSelectionOnlyPreparesWorkbenchInCallerOwnedArtifacts(t *testing.T) { func TestDryRunPreparesWorkbenchInAllocatedRunArtifacts(t *testing.T) { ctx := context.Background() + invocationDir := t.TempDir() + gitCommandMustSucceed(t, invocationDir, "init") + t.Chdir(invocationDir) store := openPipelineStore(t) defer closeStore(t, store) fixture := newWorkbenchGitFixture(t) @@ -73,13 +77,14 @@ func TestDryRunPreparesWorkbenchInAllocatedRunArtifacts(t *testing.T) { 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, + Provider: provider, + Adapter: adapter, + Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + GitCommand: workbenchGitCommandForTest(req.PRRef, fixture.repoDir), ResolveRepoRoot: func(context.Context) (string, error) { - return fixture.repoDir, nil + return invocationDir, nil }, NewRunID: func() string { return "run-workbench" }, NewSessionRowID: sequence("session"), diff --git a/internal/reviewrun/reviewrun.go b/internal/reviewrun/reviewrun.go index 57bd8ea..d1660ff 100644 --- a/internal/reviewrun/reviewrun.go +++ b/internal/reviewrun/reviewrun.go @@ -260,6 +260,9 @@ func continueRun(ctx context.Context, opts Options, req Request, result Result) desired, planResult, err := planOrResume(ctx, opts, req, result) if err != nil { + if run, loadErr := opts.Store.GetRun(context.Background(), result.Run.RunID); loadErr == nil { + result.Run = run + } return result, err } if planResult != nil { @@ -356,6 +359,13 @@ func planOrResume(ctx context.Context, opts Options, req Request, result Result) planned, err := opts.Planner.Live(ctx, req.Pipeline, result.Run) if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return "", nil, err + } + outcome := plannerFailureOutcome(pipeline.ClassifyFailure(err), result.Decision.Kind == gate.DecisionResume) + if completeErr := opts.Store.CompleteRun(context.Background(), result.Run.RunID, outcome, opts.now()); completeErr != nil { + return "", nil, completeErr + } return "", nil, err } desired, err := desiredOutcomeFromPlan(planned.Plan.Outcome) @@ -365,6 +375,13 @@ func planOrResume(ctx context.Context, opts Options, req Request, result Result) return desired, &planned, nil } +func plannerFailureOutcome(kind pipeline.FailureKind, resumed bool) ledger.Outcome { + if kind == pipeline.FailureDurableBlocking || kind == pipeline.FailureTransient && resumed { + return ledger.OutcomeIncomplete + } + return ledger.OutcomeFailed +} + func planningStateEmpty(ctx context.Context, store Store, runID string) (bool, error) { sessions, err := store.ListSessionsForRun(ctx, runID) if err != nil { diff --git a/internal/reviewrun/reviewrun_test.go b/internal/reviewrun/reviewrun_test.go index b4909ab..5e9ba7f 100644 --- a/internal/reviewrun/reviewrun_test.go +++ b/internal/reviewrun/reviewrun_test.go @@ -17,12 +17,15 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/cmd/exitcode" "github.com/open-cli-collective/codereview-cli/internal/config" "github.com/open-cli-collective/codereview-cli/internal/datalifecycle" + "github.com/open-cli-collective/codereview-cli/internal/gate" "github.com/open-cli-collective/codereview-cli/internal/gateio" "github.com/open-cli-collective/codereview-cli/internal/gitprovider" "github.com/open-cli-collective/codereview-cli/internal/ledger" + "github.com/open-cli-collective/codereview-cli/internal/llm" "github.com/open-cli-collective/codereview-cli/internal/marker" "github.com/open-cli-collective/codereview-cli/internal/pipeline" "github.com/open-cli-collective/codereview-cli/internal/plannedactions" + "github.com/open-cli-collective/codereview-cli/internal/reporoot" "github.com/open-cli-collective/codereview-cli/internal/review" "github.com/open-cli-collective/codereview-cli/internal/reviewplan" "github.com/open-cli-collective/codereview-cli/internal/statepaths" @@ -459,6 +462,125 @@ func TestRunResumeExistingActionsSkipsPlanner(t *testing.T) { } } +func TestPlanOrResumePersistsFailureOutcomeMatrix(t *testing.T) { + tests := []struct { + name string + kind pipeline.FailureKind + resume bool + outcome ledger.Outcome + }{ + {name: "fresh transient", kind: pipeline.FailureTransient, outcome: ledger.OutcomeFailed}, + {name: "resumed transient", kind: pipeline.FailureTransient, resume: true, outcome: ledger.OutcomeIncomplete}, + {name: "fresh durable blocking", kind: pipeline.FailureDurableBlocking, outcome: ledger.OutcomeIncomplete}, + {name: "resumed durable blocking", kind: pipeline.FailureDurableBlocking, resume: true, outcome: ledger.OutcomeIncomplete}, + {name: "fresh terminal", kind: pipeline.FailureTerminal, outcome: ledger.OutcomeFailed}, + {name: "resumed terminal", kind: pipeline.FailureTerminal, resume: true, outcome: ledger.OutcomeFailed}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, strings.ReplaceAll(tt.name, " ", "-"), testBaseSHA) + decision := gate.Decision{Kind: gate.DecisionFresh} + if tt.resume { + decision.Kind = gate.DecisionResume + } + plannerErr := errors.New("planner failed") + planner := &fakePlanner{store: fixture.store, err: pipeline.Failure(tt.kind, plannerErr)} + + _, _, err := planOrResume(context.Background(), fixture.opts(planner), Request{Pipeline: fixture.req}, Result{Run: run, Decision: decision}) + if !errors.Is(err, plannerErr) { + t.Fatalf("planOrResume error = %v, want planner failure", err) + } + stored, err := fixture.store.GetRun(context.Background(), run.RunID) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if stored.Outcome == nil || *stored.Outcome != tt.outcome { + t.Fatalf("outcome = %#v, want %q", stored.Outcome, tt.outcome) + } + }) + } +} + +func TestPlanOrResumeLeavesCanceledRunOpen(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, "canceled-plan", testBaseSHA) + planner := &fakePlanner{store: fixture.store, err: context.Canceled} + + _, _, err := planOrResume(context.Background(), fixture.opts(planner), Request{Pipeline: fixture.req}, Result{ + Run: run, + Decision: gate.Decision{Kind: gate.DecisionFresh}, + }) + if !errors.Is(err, context.Canceled) { + t.Fatalf("planOrResume error = %v, want context canceled", err) + } + stored, err := fixture.store.GetRun(context.Background(), run.RunID) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if stored.Outcome != nil { + t.Fatalf("outcome = %#v, want running run for next invocation", stored.Outcome) + } +} + +func TestRunAfterCanceledPlanningResumesSameRun(t *testing.T) { + fixture := newFixture(t) + planner := &fakePlanner{store: fixture.store, outcome: reviewplan.OutcomeComment, err: context.Canceled} + opts := fixture.opts(planner) + opts.NewRunID = sequence("interrupted") + + first, err := Run(context.Background(), opts, Request{Pipeline: fixture.req}) + if !errors.Is(err, context.Canceled) { + t.Fatalf("first Run error = %v, want context canceled", err) + } + if first.Run.RunID == "" || first.Run.Outcome != nil { + t.Fatalf("first run = %#v, want open allocated run", first.Run) + } + + planner.err = nil + second, err := Run(context.Background(), opts, Request{Pipeline: fixture.req}) + if err != nil { + t.Fatalf("second Run: %v", err) + } + if second.Run.RunID != first.Run.RunID { + t.Fatalf("second run ID = %q, want resumed %q", second.Run.RunID, first.Run.RunID) + } + if second.Run.Outcome == nil || *second.Run.Outcome != ledger.OutcomeComment { + t.Fatalf("second run outcome = %#v, want comment", second.Run.Outcome) + } + if planner.calls != 2 { + t.Fatalf("planner calls = %d, want interrupted attempt plus resumed attempt", planner.calls) + } +} + +func TestRunMarksResumedRealPipelineTerminalFailureFailed(t *testing.T) { + fixture := newFixture(t) + run := fixture.allocateRun(t, "real-terminal", testBaseSHA) + fixture.req.Profile.Git.Host = "git.example.com" + planner := pipelinePlanner{opts: pipeline.Options{ + Provider: fixture.provider, + Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, + Store: fixture.store, + Layout: fixture.layout, + ResolveRepoRoot: func(context.Context) (string, error) { return "", reporoot.ErrUnavailable }, + GitCommand: func(context.Context, string, ...string) ([]byte, error) { + t.Fatal("Git called before terminal repository binding failure") + return nil, nil + }, + }} + + result, err := Run(context.Background(), fixture.opts(planner), Request{Pipeline: fixture.req}) + if err == nil || pipeline.ClassifyFailure(err) != pipeline.FailureTerminal { + t.Fatalf("Run error = %v kind %v, want terminal pipeline failure", err, pipeline.ClassifyFailure(err)) + } + if result.Run.RunID != run.RunID { + t.Fatalf("result run ID = %q, want resumed %q", result.Run.RunID, run.RunID) + } + if result.Run.Outcome == nil || *result.Run.Outcome != ledger.OutcomeFailed { + t.Fatalf("result outcome = %#v, want failed", result.Run.Outcome) + } +} + func TestRunResumeInvalidStoredActionsFailsRunWithRerunGuidance(t *testing.T) { ctx := context.Background() fixture := newFixture(t) @@ -1086,6 +1208,7 @@ func (s findingOnlyStore) ListFindings(ctx context.Context, runID string) ([]led type fakePlanner struct { store *ledger.Store outcome reviewplan.Outcome + err error namedCandidate *ledger.NamedSession includeResolve bool calls int @@ -1093,6 +1216,12 @@ type fakePlanner struct { requests []pipeline.Request } +type pipelinePlanner struct{ opts pipeline.Options } + +func (p pipelinePlanner) Live(ctx context.Context, req pipeline.Request, run ledger.Run) (pipeline.Result, error) { + return pipeline.Live(ctx, p.opts, req, run) +} + type retentionProvider struct { *gitprovider.Fake beforeLive func() @@ -1109,6 +1238,9 @@ func (p *fakePlanner) Live(_ context.Context, req pipeline.Request, run ledger.R p.calls++ p.runs = append(p.runs, run) p.requests = append(p.requests, req) + if p.err != nil { + return pipeline.Result{}, p.err + } event := review.ReviewEventComment if p.outcome == reviewplan.OutcomeRequestChanges { event = review.ReviewEventRequestChanges diff --git a/internal/workbench/workbench.go b/internal/workbench/workbench.go index 66094bd..92b8994 100644 --- a/internal/workbench/workbench.go +++ b/internal/workbench/workbench.go @@ -4,11 +4,13 @@ package workbench import ( "context" "encoding/json" + "errors" "fmt" "net/url" "os" "os/exec" "path/filepath" + "slices" "sort" "strings" @@ -16,21 +18,33 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/gitprovider" "github.com/open-cli-collective/codereview-cli/internal/llm" "github.com/open-cli-collective/codereview-cli/internal/prref" - "github.com/open-cli-collective/codereview-cli/internal/reporoot" "github.com/open-cli-collective/codereview-cli/internal/runartifact" "github.com/open-cli-collective/codereview-cli/internal/statepaths" ) const ( - metadataSchemaVersion = 1 + metadataSchemaVersion = 2 checkoutModeArtifactClone = "artifact-clone" defaultReviewerWorkspaceToolOutputBytes = 32 * 1024 ) +// ErrUnsafeFetchRef marks a provider ref that cannot be passed to Git safely. +var ErrUnsafeFetchRef = errors.New("workbench: unsafe fetch ref") + +// ErrInvalidRepositoryIdentity marks repository coordinates that cannot form a safe remote. +var ErrInvalidRepositoryIdentity = errors.New("workbench: invalid repository identity") + // Deps contains the injected repository operations used to prepare workspaces. type Deps struct { - GitCommand func(context.Context, string, ...string) ([]byte, error) - ResolveRepoRoot func(context.Context) (string, error) + GitCommand func(context.Context, string, ...string) ([]byte, error) +} + +// RunPreparer creates and validates durable review workbenches. +type RunPreparer struct{ deps Deps } + +// NewRunPreparer constructs a durable workbench preparer. +func NewRunPreparer(gitCommand func(context.Context, string, ...string) ([]byte, error)) *RunPreparer { + return &RunPreparer{deps: Deps{GitCommand: gitCommand}} } // Request identifies the review checkout and artifact paths to prepare. @@ -43,7 +57,7 @@ type Request struct { type metadataArtifact struct { SchemaVersion int `json:"schema_version"` - SourceRepoRoot string `json:"source_repo_root"` + SourceRepoRoot string `json:"source_repo_root,omitempty"` CheckoutMode string `json:"checkout_mode"` PR prIdentity `json:"pr"` Base branchArtifact `json:"base"` @@ -71,21 +85,25 @@ type branchArtifact struct { } type fingerprintInputs struct { - PR prIdentity `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"` + PR prIdentity `json:"pr"` + BaseSHA string `json:"base_sha"` + HeadSHA string `json:"head_sha"` + CheckoutMode string `json:"checkout_mode"` + ChangedFiles []string `json:"changed_files,omitempty"` } // Prepare creates a clean checkout pinned to the requested review commits. func Prepare(ctx context.Context, deps Deps, req Request) error { - sourceRepoRoot, err := deps.resolveRepoRoot(ctx) - if err != nil { - return fmt.Errorf("pipeline: resolve source repo root: %w", err) + return NewRunPreparer(deps.GitCommand).Prepare(ctx, req) +} + +// Prepare creates or reuses a clean checkout pinned to the requested commits. +func (p *RunPreparer) Prepare(ctx context.Context, req Request) error { + if reusable, err := p.reusable(ctx, req); err != nil { + return err + } else if reusable { + return nil } - sourceRepoRoot = filepath.Clean(sourceRepoRoot) if err := os.RemoveAll(req.Artifacts.WorkbenchDir); err != nil { return fmt.Errorf("pipeline: reset workbench dir: %w", err) @@ -96,38 +114,31 @@ func Prepare(ctx context.Context, deps Deps, req Request) error { } } - baseRemoteURL, err := resolveBaseRemoteURL(ctx, deps, sourceRepoRoot, req.ReviewPR.Base) + baseRemoteURL, err := branchRemoteURL(req.ReviewPR.Base) if err != nil { return err } - if _, err := deps.gitCommand(ctx, "", "clone", "--no-checkout", "--no-hardlinks", sourceRepoRoot, req.Artifacts.WorkbenchRepoDir); err != nil { - return fmt.Errorf("pipeline: clone workbench repo: %w", err) - } - remoteMatchesBaseHost, err := remoteMatchesBaseHost(baseRemoteURL, req.ReviewPR.Base) - if err != nil { - return err + if _, err := p.deps.gitCommand(ctx, "", "init", req.Artifacts.WorkbenchRepoDir); err != nil { + return fmt.Errorf("pipeline: initialize workbench repo: %w", 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 := p.deps.gitCommand(ctx, req.Artifacts.WorkbenchRepoDir, "remote", "add", "origin", baseRemoteURL); err != nil { + return fmt.Errorf("pipeline: configure workbench origin: %w", err) } - if err := ensureCommit(ctx, deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Base, baseRemoteURL); err != nil { + if err := ensureCommit(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Base, baseRemoteURL); err != nil { return err } - headRemoteURL := baseRemoteURL if !sameBranchRepo(req.ReviewPR.Base, req.ReviewPR.Head) { - headRemoteURL, err = deriveRemoteURL(baseRemoteURL, req.ReviewPR.Head) - if err != nil { - return fmt.Errorf("pipeline: derive head remote URL: %w", err) + if err := ensurePullRequestHead(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.PRRef.Number, req.ReviewPR.Head, baseRemoteURL); err != nil { + return err } - } - if err := ensureCommit(ctx, deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Head, headRemoteURL); err != nil { + } else if err := ensureCommit(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Head, baseRemoteURL); err != nil { return err } - if _, err := deps.gitCommand(ctx, req.Artifacts.WorkbenchRepoDir, "checkout", "--detach", req.ReviewPR.Head.SHA); err != nil { + if _, err := p.deps.gitCommand(ctx, req.Artifacts.WorkbenchRepoDir, "checkout", "--detach", req.ReviewPR.Head.SHA); err != nil { return fmt.Errorf("pipeline: checkout workbench head %s: %w", prref.ShortSHA(req.ReviewPR.Head.SHA), err) } - if err := verifyClean(ctx, deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Head.SHA); err != nil { + if err := verifyClean(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Head.SHA); err != nil { return err } @@ -140,55 +151,72 @@ func Prepare(ctx context.Context, deps Deps, req Request) error { Number: req.PRRef.Number, } meta := metadataArtifact{ - SchemaVersion: metadataSchemaVersion, - SourceRepoRoot: sourceRepoRoot, - CheckoutMode: checkoutModeArtifactClone, - PR: prIdentity, - Base: branchArtifactFromRef(req.ReviewPR.Base), - Head: branchArtifactFromRef(req.ReviewPR.Head), - RepoPath: req.Artifacts.WorkbenchRepoDir, - ScratchPath: req.Artifacts.WorkbenchScratch, - ChangedFiles: changedFiles, + SchemaVersion: metadataSchemaVersion, + CheckoutMode: checkoutModeArtifactClone, + PR: prIdentity, + Base: branchArtifactFromRef(req.ReviewPR.Base), + Head: branchArtifactFromRef(req.ReviewPR.Head), + RepoPath: req.Artifacts.WorkbenchRepoDir, + ScratchPath: req.Artifacts.WorkbenchScratch, + ChangedFiles: changedFiles, FingerprintInputs: fingerprintInputs{ - PR: prIdentity, - BaseSHA: req.ReviewPR.Base.SHA, - HeadSHA: req.ReviewPR.Head.SHA, - CheckoutMode: checkoutModeArtifactClone, - ChangedFiles: changedFiles, - SourceRepoRoot: sourceRepoRoot, + PR: prIdentity, + BaseSHA: req.ReviewPR.Base.SHA, + HeadSHA: req.ReviewPR.Head.SHA, + CheckoutMode: checkoutModeArtifactClone, + ChangedFiles: changedFiles, }, } return writeJSONFile(req.Artifacts.WorkbenchMetadataPath(), meta) } -func resolveBaseRemoteURL(ctx context.Context, deps Deps, sourceRepoRoot string, base gitprovider.PRBranchRef) (string, error) { - originOutput, err := deps.gitCommand(ctx, sourceRepoRoot, "remote", "get-url", "origin") - if err != nil { - return "", fmt.Errorf("pipeline: resolve source repo origin: %w", err) +func (p *RunPreparer) reusable(ctx context.Context, req Request) (bool, error) { + data, err := os.ReadFile(req.Artifacts.WorkbenchMetadataPath()) // #nosec G304 -- run-owned artifact path. + if errors.Is(err, os.ErrNotExist) { + return false, nil } - originURL := strings.TrimSpace(string(originOutput)) - if originURL == "" { - return "", fmt.Errorf("pipeline: source repo origin URL is empty") - } - host, owner, repo, _, err := parseRemoteURL(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) + return false, fmt.Errorf("pipeline: read workbench metadata: %w", err) } - if host == "" { - return "", fmt.Errorf("pipeline: source repo origin %q did not include a host", originURL) + var meta metadataArtifact + if err := json.Unmarshal(data, &meta); err != nil { + return false, nil } - return originURL, nil + changedFiles := append([]string(nil), req.ChangedFiles...) + sort.Strings(changedFiles) + wantPR := prIdentity{Host: req.PRRef.Host, Owner: req.PRRef.Owner, Repo: req.PRRef.Repo, Number: req.PRRef.Number} + if (meta.SchemaVersion != 1 && meta.SchemaVersion != metadataSchemaVersion) || + meta.CheckoutMode != checkoutModeArtifactClone || meta.PR != wantPR || + meta.Base != branchArtifactFromRef(req.ReviewPR.Base) || meta.Head != branchArtifactFromRef(req.ReviewPR.Head) || + filepath.Clean(meta.RepoPath) != filepath.Clean(req.Artifacts.WorkbenchRepoDir) || + filepath.Clean(meta.ScratchPath) != filepath.Clean(req.Artifacts.WorkbenchScratch) || + !slices.Equal(meta.ChangedFiles, changedFiles) || + meta.FingerprintInputs.PR != wantPR || meta.FingerprintInputs.BaseSHA != req.ReviewPR.Base.SHA || + meta.FingerprintInputs.HeadSHA != req.ReviewPR.Head.SHA || meta.FingerprintInputs.CheckoutMode != checkoutModeArtifactClone || + !slices.Equal(meta.FingerprintInputs.ChangedFiles, changedFiles) { + return false, nil + } + if !commitPresent(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Base.SHA) || + !commitPresent(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Head.SHA) { + return false, nil + } + if err := verifyClean(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Head.SHA); err != nil { + return false, nil + } + if err := os.MkdirAll(req.Artifacts.WorkbenchScratch, 0o700); err != nil { + return false, fmt.Errorf("pipeline: create workbench scratch dir: %w", err) + } + return true, nil } -func remoteMatchesBaseHost(remoteURL string, base gitprovider.PRBranchRef) (bool, error) { - host, _, _, _, err := parseRemoteURL(remoteURL) - if err != nil { - return false, fmt.Errorf("pipeline: parse source repo origin URL %q: %w", remoteURL, err) +func branchRemoteURL(branch gitprovider.PRBranchRef) (string, error) { + host := strings.TrimSpace(branch.Host) + owner := strings.Trim(strings.TrimSpace(branch.Owner), "/") + repo := strings.TrimSuffix(strings.Trim(strings.TrimSpace(branch.Repo), "/"), ".git") + if host == "" || owner == "" || repo == "" || strings.Contains(host, "://") || strings.Contains(owner, "/") || strings.Contains(repo, "/") { + return "", fmt.Errorf("%w %s/%s on %s", ErrInvalidRepositoryIdentity, owner, repo, host) } - return strings.EqualFold(host, base.Host), nil + return (&url.URL{Scheme: "https", Host: host, Path: "/" + owner + "/" + repo + ".git"}).String(), nil } func ensureCommit(ctx context.Context, deps Deps, repoDir string, branch gitprovider.PRBranchRef, remoteURL string) error { @@ -210,12 +238,20 @@ func ensureCommit(ctx context.Context, deps Deps, repoDir string, branch gitprov return fmt.Errorf("pipeline: fetch commit %s for %s/%s from %q", prref.ShortSHA(branch.SHA), branch.Owner, branch.Repo, remoteURL) } +func ensurePullRequestHead(ctx context.Context, deps Deps, repoDir string, number int, head gitprovider.PRBranchRef, remoteURL string) error { + ref := fmt.Sprintf("refs/pull/%d/head", number) + if _, err := deps.gitCommand(ctx, repoDir, "fetch", "--no-tags", remoteURL, ref); err == nil && commitPresent(ctx, deps, repoDir, head.SHA) { + return nil + } + return fmt.Errorf("pipeline: fetch PR head commit %s from %q ref %q", prref.ShortSHA(head.SHA), remoteURL, ref) +} + func validateFetchRef(ref string) error { if ref == "" { return nil } if !strings.HasPrefix(ref, "refs/") { - return fmt.Errorf("pipeline: reject unsafe fetch ref %q", ref) + return fmt.Errorf("%w %q", ErrUnsafeFetchRef, ref) } return nil } @@ -229,7 +265,7 @@ func commitPresent(ctx context.Context, deps Deps, repoDir, sha string) bool { } func sameBranchRepo(left, right gitprovider.PRBranchRef) bool { - return strings.EqualFold(left.Host, right.Host) && left.Owner == right.Owner && left.Repo == right.Repo + return strings.EqualFold(left.Host, right.Host) && strings.EqualFold(left.Owner, right.Owner) && strings.EqualFold(left.Repo, right.Repo) } func branchArtifactFromRef(ref gitprovider.PRBranchRef) branchArtifact { @@ -385,98 +421,6 @@ func validateReviewerWorkspaceFileTarget(target string, displayPath string) erro return nil } -type remoteStyle struct { - scheme string - user string - host string - scp bool - dotGit bool -} - -func parseRemoteURL(raw string) (host, owner, repo string, style remoteStyle, err error) { - raw = strings.TrimSpace(raw) - switch { - case strings.Contains(raw, "://"): - parsed, parseErr := url.Parse(raw) - if parseErr != nil { - return "", "", "", remoteStyle{}, parseErr - } - owner, repo, dotGit, parseErr := parseRepoPath(parsed.Path) - if parseErr != nil { - return "", "", "", remoteStyle{}, parseErr - } - style = remoteStyle{ - 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 "", "", "", remoteStyle{}, fmt.Errorf("invalid scp-style remote") - } - userHost := parts[0] - pathPart := parts[1] - userHostParts := strings.SplitN(userHost, "@", 2) - if len(userHostParts) != 2 { - return "", "", "", remoteStyle{}, fmt.Errorf("invalid scp-style remote") - } - owner, repo, dotGit, parseErr := parseRepoPath(pathPart) - if parseErr != nil { - return "", "", "", remoteStyle{}, parseErr - } - style = remoteStyle{ - user: userHostParts[0], - host: userHostParts[1], - scp: true, - dotGit: dotGit, - } - return userHostParts[1], owner, repo, style, nil - default: - return "", "", "", remoteStyle{}, fmt.Errorf("unsupported remote URL %q", raw) - } -} - -func parseRepoPath(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 deriveRemoteURL(originURL string, branch gitprovider.PRBranchRef) (string, error) { - _, _, _, style, err := parseRemoteURL(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 writeJSONFile(path string, payload any) error { data, err := json.MarshalIndent(payload, "", " ") if err != nil { @@ -510,10 +454,3 @@ func (deps Deps) gitCommand(ctx context.Context, dir string, args ...string) ([] } return output, nil } - -func (deps Deps) resolveRepoRoot(ctx context.Context) (string, error) { - if deps.ResolveRepoRoot != nil { - return deps.ResolveRepoRoot(ctx) - } - return reporoot.Resolve(ctx, "", deps.GitCommand) -} diff --git a/internal/workbench/workbench_test.go b/internal/workbench/workbench_test.go index ae15de3..06cd9f3 100644 --- a/internal/workbench/workbench_test.go +++ b/internal/workbench/workbench_test.go @@ -26,7 +26,9 @@ func TestPrepareCreatesCleanPinnedCheckoutAndMetadata(t *testing.T) { artifacts := runartifact.FromDir(t.TempDir()) err := Prepare(ctx, Deps{ - ResolveRepoRoot: func(context.Context) (string, error) { return fixture.repoDir, nil }, + GitCommand: testGitRunner(t, map[string]string{ + "https://github.com/open-cli-collective/codereview-cli.git": fixture.repoDir, + }), }, Request{ PRRef: fixture.pr.Ref, ReviewPR: fixture.pr, @@ -65,8 +67,8 @@ func TestPrepareCreatesCleanPinnedCheckoutAndMetadata(t *testing.T) { if meta.PR != (prIdentity{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.SourceRepoRoot != "" { + t.Fatalf("metadata source repo root = %q, want omitted", meta.SourceRepoRoot) } 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) @@ -78,46 +80,25 @@ func TestPrepareCreatesCleanPinnedCheckoutAndMetadata(t *testing.T) { meta.FingerprintInputs.BaseSHA != fixture.baseSHA || meta.FingerprintInputs.HeadSHA != fixture.headSHA || meta.FingerprintInputs.CheckoutMode != checkoutModeArtifactClone || - 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 TestDeriveRemoteURLPreservesRemoteStyle(t *testing.T) { - branch := gitprovider.PRBranchRef{Host: "github.com", Owner: "fork-owner", Repo: "codereview-cli"} - - scpURL, err := deriveRemoteURL("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 := deriveRemoteURL("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 TestPrepareFetchesForkHeadFromDerivedRemote(t *testing.T) { +func TestPrepareFetchesForkHeadThroughBasePullRef(t *testing.T) { ctx := context.Background() fixture := newForkWorkbenchFixture(t) artifacts := runartifact.FromDir(t.TempDir()) var fetchedRemotes []string + var fetchedRefs []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]) + fetchedRefs = append(fetchedRefs, cmdArgs[len(cmdArgs)-1]) switch cmdArgs[2] { - case "git@github.com:open-cli-collective/codereview-cli.git": + case "https://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. @@ -132,8 +113,7 @@ func TestPrepareFetchesForkHeadFromDerivedRemote(t *testing.T) { } err := Prepare(ctx, Deps{ - ResolveRepoRoot: func(context.Context) (string, error) { return fixture.sourceRepoDir, nil }, - GitCommand: gitRunner, + GitCommand: gitRunner, }, Request{ PRRef: fixture.pr.Ref, ReviewPR: fixture.pr, @@ -149,27 +129,8 @@ func TestPrepareFetchesForkHeadFromDerivedRemote(t *testing.T) { 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 TestPrepareRejectsMismatchedBaseHostEvenWhenCommitsExistLocally(t *testing.T) { - ctx := context.Background() - fixture := newWorkbenchGitFixture(t) - artifacts := runartifact.FromDir(t.TempDir()) - pr := fixture.pr - pr.Base.Host = "example.com" - pr.Ref.Host = "example.com" - - err := Prepare(ctx, Deps{ - ResolveRepoRoot: func(context.Context) (string, error) { return fixture.repoDir, nil }, - }, Request{PRRef: pr.Ref, ReviewPR: pr, ChangedFiles: []string{"main.go"}, Artifacts: artifacts}) - if err == nil { - t.Fatal("Prepare 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("Prepare error = %v, want host mismatch", err) + if slices.Contains(fetchedRemotes, "https://github.com/fork-owner/codereview-cli-fork.git") || !slices.Contains(fetchedRefs, "refs/pull/371/head") { + t.Fatalf("fetches = remotes %#v refs %#v, want base remote PR-head ref", fetchedRemotes, fetchedRefs) } } @@ -178,15 +139,13 @@ func TestPrepareRejectsUnsafeFetchRef(t *testing.T) { fixture := newForkWorkbenchFixture(t) artifacts := runartifact.FromDir(t.TempDir()) pr := fixture.pr - pr.Head.Ref = "--upload-pack=/tmp/pwn" + pr.Base.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": + case "https://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. @@ -201,10 +160,9 @@ func TestPrepareRejectsUnsafeFetchRef(t *testing.T) { } err := Prepare(ctx, Deps{ - ResolveRepoRoot: func(context.Context) (string, error) { return fixture.sourceRepoDir, nil }, - GitCommand: gitRunner, + GitCommand: gitRunner, }, Request{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"`) { + if !errors.Is(err, ErrUnsafeFetchRef) { t.Fatalf("Prepare error = %v, want unsafe ref rejection", err) } } @@ -214,7 +172,9 @@ func TestPrepareRefreshesExistingArtifactRoot(t *testing.T) { fixture := newWorkbenchGitFixture(t) artifacts := runartifact.FromDir(t.TempDir()) req := Request{PRRef: fixture.pr.Ref, ReviewPR: fixture.pr, ChangedFiles: []string{"main.go"}, Artifacts: artifacts} - deps := Deps{ResolveRepoRoot: func(context.Context) (string, error) { return fixture.repoDir, nil }} + deps := Deps{GitCommand: testGitRunner(t, map[string]string{ + "https://github.com/open-cli-collective/codereview-cli.git": fixture.repoDir, + })} if err := Prepare(ctx, deps, req); err != nil { t.Fatalf("Prepare first run: %v", err) @@ -242,6 +202,43 @@ func TestPrepareRefreshesExistingArtifactRoot(t *testing.T) { } } +func TestPrepareReusesValidV1WorkbenchWithoutRewritingMetadata(t *testing.T) { + ctx := context.Background() + fixture := newWorkbenchGitFixture(t) + artifacts := runartifact.FromDir(t.TempDir()) + req := Request{PRRef: fixture.pr.Ref, ReviewPR: fixture.pr, ChangedFiles: []string{"main.go"}, Artifacts: artifacts} + remote := "https://github.com/open-cli-collective/codereview-cli.git" + if err := Prepare(ctx, Deps{GitCommand: testGitRunner(t, map[string]string{remote: fixture.repoDir})}, req); err != nil { + t.Fatalf("Prepare first run: %v", err) + } + metadataPath := artifacts.WorkbenchMetadataPath() + data, err := os.ReadFile(metadataPath) // #nosec G304 -- test-controlled artifact path. + if err != nil { + t.Fatal(err) + } + v1 := strings.Replace(string(data), `"schema_version": 2,`, `"schema_version": 1,`+"\n \"source_repo_root\": \"/old/unrelated/checkout\",", 1) + if err := os.WriteFile(metadataPath, []byte(v1), 0o600); err != nil { // #nosec G703 -- test-controlled artifact path. + t.Fatal(err) + } + baseRunner := testGitRunner(t, nil) + reuseRunner := func(ctx context.Context, dir string, args ...string) ([]byte, error) { + if len(args) > 0 && (args[0] == "init" || args[0] == "fetch") { + return nil, fmt.Errorf("unexpected rebuild command: git %s", strings.Join(args, " ")) + } + return baseRunner(ctx, dir, args...) + } + if err := Prepare(ctx, Deps{GitCommand: reuseRunner}, req); err != nil { + t.Fatalf("Prepare reuse: %v", err) + } + got, err := os.ReadFile(metadataPath) // #nosec G304 -- test-controlled artifact path. + if err != nil { + t.Fatal(err) + } + if string(got) != v1 { + t.Fatalf("v1 metadata was rewritten\ngot: %s\nwant: %s", got, v1) + } +} + func TestReviewerWorkspaceSmokeAllowsReadAndWorkspaceWrites(t *testing.T) { ctx := context.Background() fixture, artifacts, deps := prepareReviewerFixture(t) @@ -415,9 +412,7 @@ type workbenchGitFixture struct { } type forkWorkbenchFixture struct { - sourceRepoDir string baseRemotePath string - forkRemotePath string pr gitprovider.PR } @@ -470,9 +465,6 @@ func newForkWorkbenchFixture(t *testing.T) forkWorkbenchFixture { 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") @@ -483,9 +475,10 @@ func newForkWorkbenchFixture(t *testing.T) forkWorkbenchFixture { } gitCommandMustSucceed(t, forkRemotePath, "commit", "-am", "fork head") headSHA := strings.TrimSpace(gitCommandOutput(t, forkRemotePath, "rev-parse", "HEAD")) + gitCommandMustSucceed(t, forkRemotePath, "push", baseRemotePath, "HEAD:refs/pull/371/head") ref := gitprovider.PRRef{Host: "github.com", Owner: "open-cli-collective", Repo: "codereview-cli", Number: 371} return forkWorkbenchFixture{ - sourceRepoDir: sourceRepoDir, baseRemotePath: baseRemotePath, forkRemotePath: forkRemotePath, + baseRemotePath: baseRemotePath, pr: gitprovider.PR{ Ref: ref, Title: "Fork workbench fixture", URL: "https://github.com/open-cli-collective/codereview-cli/pull/371", State: gitprovider.PRStateOpen, Base: gitprovider.PRBranchRef{Host: ref.Host, Owner: ref.Owner, Repo: ref.Repo, Name: "main", Ref: "refs/heads/main", SHA: baseSHA}, @@ -498,13 +491,36 @@ func prepareReviewerFixture(t *testing.T) (workbenchGitFixture, runartifact.Path t.Helper() fixture := newWorkbenchGitFixture(t) artifacts := runartifact.FromDir(t.TempDir()) - deps := Deps{ResolveRepoRoot: func(context.Context) (string, error) { return fixture.repoDir, nil }} + deps := Deps{GitCommand: testGitRunner(t, map[string]string{ + "https://github.com/open-cli-collective/codereview-cli.git": fixture.repoDir, + })} if err := Prepare(context.Background(), deps, Request{PRRef: fixture.pr.Ref, ReviewPR: fixture.pr, ChangedFiles: []string{"main.go"}, Artifacts: artifacts}); err != nil { t.Fatalf("Prepare: %v", err) } return fixture, artifacts, deps } +func testGitRunner(t *testing.T, remotes map[string]string) func(context.Context, string, ...string) ([]byte, error) { + t.Helper() + return func(ctx context.Context, dir string, args ...string) ([]byte, error) { + cmdArgs := append([]string(nil), args...) + if len(cmdArgs) >= 3 && cmdArgs[0] == "fetch" { + if local, ok := remotes[cmdArgs[2]]; ok { + cmdArgs[2] = local + } + } + cmd := exec.CommandContext(ctx, "git", cmdArgs...) // #nosec G204 -- tests invoke git with fixed 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 + } +} + func gitCommandMustSucceed(t *testing.T, dir string, args ...string) string { t.Helper() return strings.TrimSpace(gitCommandOutput(t, dir, args...))