diff --git a/docs/checkout-native-review-contract.md b/docs/checkout-native-review-contract.md index 3c08b2a..c6a3e09 100644 --- a/docs/checkout-native-review-contract.md +++ b/docs/checkout-native-review-contract.md @@ -74,7 +74,15 @@ Notes: - `dossier/summary/` holds durable normalized discussion artifacts. - `dossier/final/` holds the reviewer-facing dossier files used by the orchestrator and specialists. -- `workbench/repo/` is a clean pinned checkout at the PR head SHA. +- `workbench/repo/` is a clean pinned checkout at the PR head SHA, carrying + `refs/heads/cr-review-head` at that SHA. The ref is load-bearing: the + per-reviewer workspace is created with `git clone` from this directory, and + git does not treat a directory without `refs/` as a repository. Only the head + is given a ref, so the base commit is not transferred into reviewer + workspaces when base is not an ancestor of head; reviewers are handed the + provider-generated `diff.patch` and nothing in the pipeline resolves the base + SHA inside the workspace. That is a decision, not an oversight -- add + `refs/heads/cr-review-base` if a reviewer ever needs `git log base..HEAD`. - `workbench/reviewers//repo/` is a disposable reviewer checkout. - `workbench/scratch//` holds reviewer-owned scratch, temp, and cache roots. diff --git a/internal/workbench/workbench.go b/internal/workbench/workbench.go index 707a54c..6b654fa 100644 --- a/internal/workbench/workbench.go +++ b/internal/workbench/workbench.go @@ -23,8 +23,10 @@ import ( ) const ( - metadataSchemaVersion = 2 - checkoutModeArtifactClone = "artifact-clone" + metadataSchemaVersion = 2 + checkoutModeArtifactClone = "artifact-clone" + // workbenchHeadRef gives the workbench a ref so it is a clonable repository. + workbenchHeadRef = "refs/heads/cr-review-head" defaultReviewerWorkspaceToolOutputBytes = 32 * 1024 ) @@ -142,6 +144,16 @@ func (p *RunPreparer) Prepare(ctx context.Context, req Request) error { 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) } + // Give the workbench at least one ref. + // + // Everything above fetches by SHA and checks out detached, so the repo ends + // up with FETCH_HEAD and no refs/ at all. Git does not consider such a + // directory a repository, so the per-reviewer `git clone` of this workbench + // fails with "repository does not exist" -- and a reviewer that cannot start + // reports zero findings, which the rollup renders as a clean review. + if _, err := p.deps.gitCommand(ctx, req.Artifacts.WorkbenchRepoDir, "update-ref", workbenchHeadRef, req.ReviewPR.Head.SHA); err != nil { + return fmt.Errorf("pipeline: record workbench head ref: %w", err) + } if err := verifyClean(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Head.SHA); err != nil { return err } @@ -207,12 +219,29 @@ func (p *RunPreparer) reusable(ctx context.Context, req Request) (bool, error) { if err := verifyClean(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, req.ReviewPR.Head.SHA); err != nil { return false, nil } + // Both exits from Prepare must leave a clonable workbench, so the reuse + // path asserts the same postcondition the build path establishes. + // + // A workbench missing refs/ entirely is already rejected above, because + // commitPresent and verifyClean shell out to git and fail in a directory + // git does not consider a repository. What this catches is the narrower + // and likelier case: refs/ present but the head ref absent, which every + // other check happily accepts. + if !refPresent(ctx, p.deps, req.Artifacts.WorkbenchRepoDir, workbenchHeadRef) { + 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 } +// refPresent reports whether ref resolves to a commit in repoDir. +func refPresent(ctx context.Context, deps Deps, repoDir, ref string) bool { + _, err := deps.gitCommand(ctx, repoDir, "rev-parse", "--verify", "--quiet", ref+"^{commit}") + return err == nil +} + func branchRemoteURL(branch gitprovider.PRBranchRef) (string, error) { host := strings.TrimSpace(branch.Host) owner := strings.Trim(strings.TrimSpace(branch.Owner), "/") diff --git a/internal/workbench/workbench_test.go b/internal/workbench/workbench_test.go index 2927efe..79fb4f9 100644 --- a/internal/workbench/workbench_test.go +++ b/internal/workbench/workbench_test.go @@ -724,3 +724,105 @@ func (smokeStream) SessionID() string { return "workspace-smoke-session" } func (s smokeStream) Wait(context.Context) (llm.Response, error) { return llm.Response{StructuredOutput: []byte(s.output)}, nil } + +// The workbench is cloned once per reviewer. Everything that builds it fetches +// by SHA and checks out detached, so without an explicit ref the directory has +// no refs/ at all -- git then refuses to call it a repository and every +// per-reviewer clone fails. A reviewer that cannot start reports zero findings, +// which a rollup renders as a clean review, so this failure is silent and +// actively misleading. +func TestPrepareLeavesWorkbenchClonable(t *testing.T) { + ctx := context.Background() + fixture := newWorkbenchGitFixture(t) + artifacts := runartifact.FromDir(t.TempDir()) + + if err := Prepare(ctx, Deps{ + 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, + ChangedFiles: []string{"main.go"}, + Artifacts: artifacts, + }); err != nil { + t.Fatalf("Prepare: %v", err) + } + + // The head must be reachable through a real ref, not only FETCH_HEAD. + if got := strings.TrimSpace(gitCommandOutput(t, artifacts.WorkbenchRepoDir, "rev-parse", workbenchHeadRef)); got != fixture.headSHA { + t.Fatalf("%s = %q, want head %q", workbenchHeadRef, got, fixture.headSHA) + } + + // The property that actually matters: it can be cloned, the way each + // reviewer workspace is created. + dest := filepath.Join(t.TempDir(), "reviewer") + cloneWorkbench(t, artifacts.WorkbenchRepoDir, dest) + if got := strings.TrimSpace(gitCommandOutput(t, dest, "rev-parse", "HEAD")); got != fixture.headSHA { + t.Fatalf("cloned HEAD = %q, want %q", got, fixture.headSHA) + } +} + +// The reuse fast path must re-establish the head ref rather than skip past a +// workbench that lacks it. +// +// The discriminating assertion is that the ref comes *back*, not that the repo +// is clonable: deleting only the ref leaves refs/ in place, which git still +// accepts, so a clonability check here passes with or without the gate and +// proves nothing. Without the gate, Prepare takes the reuse path and the ref +// stays deleted. +func TestPrepareRestoresHeadRefOnReuse(t *testing.T) { + ctx := context.Background() + fixture := newWorkbenchGitFixture(t) + artifacts := runartifact.FromDir(t.TempDir()) + deps := Deps{ + GitCommand: testGitRunner(t, map[string]string{ + "https://github.com/open-cli-collective/codereview-cli.git": fixture.repoDir, + }), + } + req := Request{ + PRRef: fixture.pr.Ref, + ReviewPR: fixture.pr, + ChangedFiles: []string{"main.go"}, + Artifacts: artifacts, + } + if err := Prepare(ctx, deps, req); err != nil { + t.Fatalf("Prepare: %v", err) + } + + // Simulate a workbench written by a binary that predates the ref, leaving + // everything else the reuse gate inspects intact and matching. + gitCommandOutput(t, artifacts.WorkbenchRepoDir, "update-ref", "-d", workbenchHeadRef) + if gitCommandSucceeds(artifacts.WorkbenchRepoDir, "rev-parse", "--verify", "--quiet", workbenchHeadRef+"^{commit}") { + t.Fatal("premise broken: head ref still resolves after deletion") + } + + if err := Prepare(ctx, deps, req); err != nil { + t.Fatalf("Prepare (reuse): %v", err) + } + + if got := strings.TrimSpace(gitCommandOutput(t, artifacts.WorkbenchRepoDir, "rev-parse", workbenchHeadRef)); got != fixture.headSHA { + t.Fatalf("%s = %q after reuse, want head %q -- the reuse path skipped past a workbench with no head ref", workbenchHeadRef, got, fixture.headSHA) + } + cloneWorkbench(t, artifacts.WorkbenchRepoDir, filepath.Join(t.TempDir(), "reviewer")) +} + +// cloneWorkbench performs the clone a reviewer workspace is created with, so +// tests assert the property rather than a proxy for it. +func cloneWorkbench(t *testing.T, src, dest string) { + t.Helper() + cmd := exec.Command("git", "clone", "--no-hardlinks", src, dest) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + cmd.Env = gittest.Env() + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("clone workbench: %v: %s", err, out) + } +} + +// gitCommandSucceeds reports whether a git command exits zero, for assertions +// about a command that is expected to fail. +func gitCommandSucceeds(dir string, args ...string) bool { + cmd := exec.Command("git", args...) // #nosec G204 -- tests invoke git with fixed command names and structured arguments. + cmd.Env = gittest.Env() + cmd.Dir = dir + return cmd.Run() == nil +}