fix(workbench): give the workbench a ref so reviewer clones succeed

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/<reviewer-id>/repo/` is a disposable reviewer checkout.
 - `workbench/scratch/<reviewer-id>/` holds reviewer-owned scratch, temp, and
   cache roots.

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), "/")

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
+}