Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions internal/workbench/workbench.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand Down Expand Up @@ -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.
Comment thread
piekstra marked this conversation as resolved.
//
// 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 {
Comment thread
piekstra marked this conversation as resolved.
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
}
Expand Down
41 changes: 41 additions & 0 deletions internal/workbench/workbench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -724,3 +724,44 @@
func (s smokeStream) Wait(context.Context) (llm.Response, error) {
Comment thread
piekstra marked this conversation as resolved.
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()

Check failure on line 760 in internal/workbench/workbench_test.go

View workflow job for this annotation

GitHub Actions / lint

G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
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)
}
}
Loading