From 992b1f3e4931fd50abcb88c5d803bcb86a06494d Mon Sep 17 00:00:00 2001 From: piekstra Date: Thu, 13 Aug 2026 14:25:51 -0400 Subject: [PATCH 1/3] fix(workbench): give the workbench a ref so reviewer clones succeed Prepare builds the workbench with init + fetch-by-SHA + checkout --detach, so the directory ends up with FETCH_HEAD and no refs/ at all. Git does not treat such a directory as a repository, so the per-reviewer git clone --no-hardlinks fails with "repository does not exist" for every agent that needs a workspace. The failure is silent and actively misleading. A reviewer that cannot start is recorded as incomplete_failed and contributes **zero findings**, which the rollup then renders as a clean review. On a real PR this produced "0 findings" across four of five reviewers -- only the one agent that reads the diff without a checkout actually ran -- and the result read as an all-clear. Writing refs/heads/cr-review-head at the head SHA after checkout makes the workbench a valid, clonable repository. HEAD stays detached, so nothing else about the pinned checkout changes. The regression test asserts the property that matters -- that the workbench can actually be cloned the way a reviewer workspace is created -- and fails without the fix. --- internal/workbench/workbench.go | 16 +++++++++-- internal/workbench/workbench_test.go | 41 ++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/internal/workbench/workbench.go b/internal/workbench/workbench.go index 707a54c..3272dec 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 } diff --git a/internal/workbench/workbench_test.go b/internal/workbench/workbench_test.go index 2927efe..15f463d 100644 --- a/internal/workbench/workbench_test.go +++ b/internal/workbench/workbench_test.go @@ -724,3 +724,44 @@ 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") + out, err := exec.CommandContext(ctx, "git", "clone", "--no-hardlinks", artifacts.WorkbenchRepoDir, dest).CombinedOutput() + if err != nil { + t.Fatalf("clone workbench: %v: %s", err, out) + } + if got := strings.TrimSpace(gitCommandOutput(t, dest, "rev-parse", "HEAD")); got != fixture.headSHA { + t.Fatalf("cloned HEAD = %q, want %q", got, fixture.headSHA) + } +} From d4b668f4d386b724d7410103e48027863b361452 Mon Sep 17 00:00:00 2001 From: piekstra Date: Thu, 13 Aug 2026 15:14:57 -0400 Subject: [PATCH 2/3] fix(workbench): assert clonability on the reuse path, and document the ref Review raised that Prepare's new postcondition -- the workbench is clonable because it carries a head ref -- was established only on the build path, while the reuse fast path returned early without checking it. Adding the assertion is right regardless, so it is here. But the specific scenario in the finding does not appear reachable: a workbench missing refs/ entirely is already rejected, because commitPresent and verifyClean shell out to git and fail in a directory git does not consider a repository. I tried several ways to construct a reuse that reaches a reviewer clone with a broken workbench and could not; deleting only the ref leaves refs/ in place, which git still accepts and clones fine. So the check stays as belt-and-braces for the narrower case (refs/ present, head ref gone) and to stop a future change to those checks quietly dropping clonability -- and the comment says exactly that rather than claiming to fix a failure I could not reproduce. No test accompanies it: a test that passes with the change reverted verifies nothing, and I would rather ship the guard honestly labelled than a green assertion that discriminates nothing. Also documents the ref in the checkout contract, per the same review. That includes the consequence the reviewer surfaced: only the head is given a ref, so the base commit does not reach reviewer workspaces when base is not an ancestor of head. Recorded as a decision with the one-line fix if it ever needs to change. --- docs/checkout-native-review-contract.md | 10 +++++++++- internal/workbench/workbench.go | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) 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 3272dec..d6ec661 100644 --- a/internal/workbench/workbench.go +++ b/internal/workbench/workbench.go @@ -219,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. This is belt-and-braces for the + // narrower case -- refs/ present but the head ref gone -- and so that a + // future change to those checks cannot quietly drop clonability. + 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), "/") From 4cfc86cd4b9a6d48b3ba0ce27298238004eacc00 Mon Sep 17 00:00:00 2001 From: piekstra Date: Thu, 13 Aug 2026 15:19:11 -0400 Subject: [PATCH 3/3] test(workbench): prove the reuse gate restores the head ref The previous commit added the reuse-path check without a test and said so, because the test I wrote passed with the change reverted. Review pointed out the reason, and it was a flaw in my assertion rather than in the scenario: I was asserting the reused workbench is clonable, which does not discriminate, since deleting only the ref leaves refs/ in place and git still clones from it happily. The discriminating assertion is that the ref comes back. Without the gate, Prepare takes the reuse path and the ref stays deleted; with it, the workbench is rebuilt and the ref resolves to the head SHA again. Confirmed red with the gate removed and green with it. Comment updated to describe what the check actually catches -- refs/ present but the head ref absent, which every other reuse check accepts -- rather than hedging about a scenario I had not managed to construct. --- internal/workbench/workbench.go | 6 +-- internal/workbench/workbench_test.go | 69 ++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/internal/workbench/workbench.go b/internal/workbench/workbench.go index d6ec661..6b654fa 100644 --- a/internal/workbench/workbench.go +++ b/internal/workbench/workbench.go @@ -224,9 +224,9 @@ func (p *RunPreparer) reusable(ctx context.Context, req Request) (bool, error) { // // 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. This is belt-and-braces for the - // narrower case -- refs/ present but the head ref gone -- and so that a - // future change to those checks cannot quietly drop clonability. + // 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 } diff --git a/internal/workbench/workbench_test.go b/internal/workbench/workbench_test.go index 15f463d..79fb4f9 100644 --- a/internal/workbench/workbench_test.go +++ b/internal/workbench/workbench_test.go @@ -757,11 +757,72 @@ func TestPrepareLeavesWorkbenchClonable(t *testing.T) { // The property that actually matters: it can be cloned, the way each // reviewer workspace is created. dest := filepath.Join(t.TempDir(), "reviewer") - out, err := exec.CommandContext(ctx, "git", "clone", "--no-hardlinks", artifacts.WorkbenchRepoDir, dest).CombinedOutput() - if err != nil { - t.Fatalf("clone workbench: %v: %s", err, out) - } + 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 +}