From d3c2ec7649080e9a41c095ef509bc6ac5fc880ea Mon Sep 17 00:00:00 2001 From: Aaron Wong <6979793+zzwong@users.noreply.github.com> Date: Thu, 24 Sep 2026 02:49:51 -0400 Subject: [PATCH 1/2] feat(review): replay a PR without its existing discussion Add `cr review --without-discussion` for first-pass replays of pinned dry-run reviews. It requires --dry-run (or --no-post) and both --review-base-sha and --review-head-sha. Pinned reviews already skipped the live thread, review, and issue-comment reads, but a replay still resumed the PR's default orchestrator session and reviewer cohort sessions, which carry the earlier discussion, and it overwrote that cohort. With the flag, the run reads no discussion, resumes and updates no PR-scoped session or cohort, and so selection, reviewer, and rollup prompts see only the PR title, description, and diff. The run marker, dossier discussion artifacts, and JSON run output record without_discussion, and an incomplete run resumes only into a run with the same setting. Reviewer task fingerprints include the setting as well. --- README.md | 1 + internal/cmd/reviewcmd/reviewcmd.go | 11 + internal/cmd/reviewcmd/reviewcmd_test.go | 64 ++++ internal/dossier/dossier.go | 27 +- internal/pipeline/pipeline.go | 103 ++++-- internal/pipeline/without_discussion_test.go | 354 +++++++++++++++++++ internal/runartifact/runartifact.go | 21 +- internal/view/review.go | 19 +- 8 files changed, 554 insertions(+), 46 deletions(-) create mode 100644 internal/pipeline/without_discussion_test.go diff --git a/README.md b/README.md index 422648e..a918f9a 100644 --- a/README.md +++ b/README.md @@ -1227,6 +1227,7 @@ Review selection and execution flags: | `--reviewer-effort ` | Override reviewer-stage effort with `low`, `medium`, `high`, `xhigh`, or `max`, subject to runtime support. Available for dry-run, no-post, and live reviews. | | `--review-base-sha ` | Review this base commit SHA instead of the PR's current base SHA. Requires `--review-head-sha` and `--dry-run` or `--no-post`. | | `--review-head-sha ` | Review this head commit SHA instead of the PR's current head SHA. Requires `--review-base-sha` and `--dry-run` or `--no-post`. | +| `--without-discussion` | Replay the pinned review as a first pass. No review threads, thread outcomes, issue comments, or prior reviews are read, and the PR's orchestrator session and reviewer cohort sessions are neither resumed nor updated, so selection, reviewers, and rollup see only the PR title, description, and diff. The run marker (`review-run.json`), dossier discussion artifacts, and `--json` run output record `without_discussion: true`, and such a run never resumes an incomplete run made with discussion (or the reverse). Requires `--review-base-sha`, `--review-head-sha`, and `--dry-run` or `--no-post`. | | `--session ` | Override the PR's default orchestrator session with a named live-review session. Reviewer cohorts remain PR-scoped. Not allowed with `--dry-run`, `--no-post`, or `--retry-posts`. | Review progress on stderr reports the merged reviewer catalog, final selected diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index 4949407..1d2a2f1 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -80,6 +80,7 @@ type commandFlags struct { reviewerEffort string reviewBaseSHA string reviewHeadSHA string + withoutDiscussion bool maxAgents int maxConcurrency int allowSelfReview bool @@ -124,6 +125,7 @@ func RegisterWithFactory(rootCmd *cobra.Command, opts *root.Options, factory Run cmd.Flags().StringVar(&flags.reviewerEffort, "reviewer-effort", "", "Override reviewer effort for this review") cmd.Flags().StringVar(&flags.reviewBaseSHA, "review-base-sha", "", "Review this base commit SHA instead of the PR's current base SHA; requires --dry-run and --review-head-sha") cmd.Flags().StringVar(&flags.reviewHeadSHA, "review-head-sha", "", "Review this head commit SHA instead of the PR's current head SHA; requires --dry-run and --review-base-sha") + cmd.Flags().BoolVar(&flags.withoutDiscussion, "without-discussion", false, "Replay the pinned review as a first pass, without the PR's existing discussion or review sessions; requires --dry-run, --review-base-sha, and --review-head-sha") cmd.Flags().IntVar(&flags.maxAgents, "max-agents", 0, "Maximum selected reviewer agents") cmd.Flags().IntVar(&flags.maxConcurrency, "max-concurrency", 0, "Maximum concurrent reviewer agents") cmd.Flags().BoolVar(&flags.allowSelfReview, "allow-self-review", false, "Allow reviewer credentials matching the PR author") @@ -202,6 +204,14 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact return exitcode.Usage(fmt.Errorf("--review-base-sha and --review-head-sha require --dry-run or --no-post")) } } + if flags.withoutDiscussion { + if !flags.dryRun { + return exitcode.Usage(fmt.Errorf("--without-discussion requires --dry-run or --no-post")) + } + if !reviewBaseChanged { + return exitcode.Usage(fmt.Errorf("--without-discussion requires --review-base-sha and --review-head-sha")) + } + } if flags.rerun && flags.retryPosts { return exitcode.Usage(fmt.Errorf("--rerun and --retry-posts are mutually exclusive")) } @@ -348,6 +358,7 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact ReviewerFast: reviewerFast, ReviewBaseSHA: reviewBaseSHA, ReviewHeadSHA: reviewHeadSHA, + WithoutDiscussion: flags.withoutDiscussion, Rerun: flags.rerun, FreshSession: flags.freshSession, ToolVersion: version.Version, diff --git a/internal/cmd/reviewcmd/reviewcmd_test.go b/internal/cmd/reviewcmd/reviewcmd_test.go index 0579bfd..d812f1a 100644 --- a/internal/cmd/reviewcmd/reviewcmd_test.go +++ b/internal/cmd/reviewcmd/reviewcmd_test.go @@ -572,6 +572,70 @@ func TestReviewRejectsInvalidReviewSHAOverrides(t *testing.T) { } } +func TestReviewDryRunPassesWithoutDiscussion(t *testing.T) { + for _, mode := range []string{"--dry-run", "--no-post"} { + t.Run(mode, func(t *testing.T) { + runner := &fakeRunner{result: testPipelineResult(false)} + cmd, _ := newTestCommand(t, testConfig(), fakeFactory(runner)) + + err := root.Execute(cmd, []string{ + "review", "https://github.com/open-cli-collective/codereview-cli/pull/29", + mode, + "--review-base-sha", "1111111", + "--review-head-sha", "2222222", + "--without-discussion", + }) + if err != nil { + t.Fatalf("Execute: %v", err) + } + if len(runner.requests) != 1 { + t.Fatalf("runner calls = %d, want 1", len(runner.requests)) + } + if !runner.requests[0].WithoutDiscussion { + t.Fatal("WithoutDiscussion = false, want true") + } + }) + } +} + +func TestReviewRejectsWithoutDiscussionOutsidePinnedDryRun(t *testing.T) { + tests := []struct { + name string + args []string + wantErr string + }{ + {name: "live with SHAs", args: []string{"--review-base-sha", "1111111", "--review-head-sha", "2222222", "--without-discussion"}, wantErr: "require --dry-run or --no-post"}, + {name: "live without SHAs", args: []string{"--without-discussion"}, wantErr: "--without-discussion requires --dry-run or --no-post"}, + {name: "dry run without SHAs", args: []string{"--dry-run", "--without-discussion"}, wantErr: "--without-discussion requires --review-base-sha and --review-head-sha"}, + {name: "no post without SHAs", args: []string{"--no-post", "--without-discussion"}, wantErr: "--without-discussion requires --review-base-sha and --review-head-sha"}, + {name: "dry run with base only", args: []string{"--dry-run", "--review-base-sha", "1111111", "--without-discussion"}, wantErr: "must be set together"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var factoryCalled bool + cmd, _ := newTestCommand(t, testConfig(), func(context.Context, app.OpenRequest) (app.Runtime, error) { + factoryCalled = true + return app.Runtime{Runner: &fakeRunner{result: testPipelineResult(false), liveResult: testLiveResult(false)}}, nil + }) + + args := append([]string{"review", "https://github.com/open-cli-collective/codereview-cli/pull/29"}, tt.args...) + err := root.Execute(cmd, args) + if err == nil { + t.Fatal("Execute error = nil, want usage error") + } + if got := exitcode.FromError(err); got != exitcode.UsageError { + t.Fatalf("exit code = %d, want usage", got) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error = %q, want %q", err.Error(), tt.wantErr) + } + if factoryCalled { + t.Fatal("runtime factory was called for invalid --without-discussion use") + } + }) + } +} + func TestReviewLiveRejectsStageOverridesBeforeRuntimeFactory(t *testing.T) { tests := []struct { name string diff --git a/internal/dossier/dossier.go b/internal/dossier/dossier.go index 153dbb9..b3c21f2 100644 --- a/internal/dossier/dossier.go +++ b/internal/dossier/dossier.go @@ -42,9 +42,12 @@ type ChangedFile struct { // Inputs contains the source material written to raw dossier artifacts. type Inputs struct { - CurrentPR gitprovider.PR - ReviewPR gitprovider.PR - PinnedReview bool + CurrentPR gitprovider.PR + ReviewPR gitprovider.PR + PinnedReview bool + // WithoutDiscussion drops every discussion input, even if a caller + // supplied some, and records that the review ran without discussion. + WithoutDiscussion bool ChangedFiles []ChangedFile Threads []gitprovider.InlineThread ThreadContext []threadcontext.Thread @@ -128,6 +131,7 @@ type dossierPRContextArtifact struct { type dossierDiscussionArtifact struct { PinnedReview bool `json:"pinned_review"` + WithoutDiscussion bool `json:"without_discussion,omitempty"` DiscussionOmittedNote string `json:"discussion_omitted_note,omitempty"` TopLevelComments []dossierTopLevelCommentArtifact `json:"top_level_comments,omitempty"` InlineThreads []dossierInlineThreadArtifact `json:"inline_threads,omitempty"` @@ -138,6 +142,7 @@ type DiscussionSummary struct { SchemaVersion int `json:"schema_version"` SourceFingerprint string `json:"source_fingerprint,omitempty"` PinnedReview bool `json:"pinned_review"` + WithoutDiscussion bool `json:"without_discussion,omitempty"` DiscussionOmittedNote string `json:"discussion_omitted_note,omitempty"` TopLevelOmitted int `json:"top_level_comments_omitted,omitempty"` InlineThreadsOmitted int `json:"inline_threads_omitted,omitempty"` @@ -286,6 +291,12 @@ func WriteRaw(paths runartifact.Paths, in Inputs) error { return fmt.Errorf("pipeline: create dossier dir: %w", err) } } + if in.WithoutDiscussion { + in.Threads = nil + in.ThreadContext = nil + in.Reviews = nil + in.IssueComments = nil + } prContext := dossierPRContextArtifact{ Title: in.CurrentPR.Title, @@ -314,9 +325,10 @@ func WriteRaw(paths runartifact.Paths, in Inputs) error { } discussion := dossierDiscussionArtifact{ PinnedReview: in.PinnedReview, + WithoutDiscussion: in.WithoutDiscussion, DiscussionOmittedNote: strings.TrimSpace(in.DiscussionOmittedNote), } - if !in.PinnedReview { + if !in.PinnedReview && !in.WithoutDiscussion { discussion.TopLevelComments = topLevelComments discussion.InlineThreads = inlineThreads } @@ -395,10 +407,11 @@ func ReadDiscussionSummary(paths runartifact.Paths) (DiscussionSummary, error) { } func summarizeDiscussionArtifacts(ctx context.Context, env Env, req PreparationRequest, discussion dossierDiscussionArtifact) (DiscussionSummary, error) { - if discussion.PinnedReview { + if discussion.PinnedReview || discussion.WithoutDiscussion { return DiscussionSummary{ SchemaVersion: dossierSummarySchemaVersion, - PinnedReview: true, + PinnedReview: discussion.PinnedReview, + WithoutDiscussion: discussion.WithoutDiscussion, DiscussionOmittedNote: strings.TrimSpace(discussion.DiscussionOmittedNote), }, nil } @@ -797,7 +810,7 @@ func renderDossierDiscussionSummaryMarkdown(summary DiscussionSummary, title str var out strings.Builder out.WriteString(title) out.WriteString("\n\n") - if summary.PinnedReview { + if summary.PinnedReview || summary.WithoutDiscussion { note := strings.TrimSpace(summary.DiscussionOmittedNote) if note == "" { note = "Current PR discussion omitted for pinned review." diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 9dd637f..fe10507 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -199,8 +199,12 @@ type Request struct { ReviewerFast bool ReviewBaseSHA string ReviewHeadSHA string - Rerun bool - FreshSession bool + // WithoutDiscussion replays a pinned dry-run review as a first pass: no + // PR discussion is fetched, and no PR-scoped orchestrator or reviewer + // session (which saw earlier discussion) is resumed or updated. + WithoutDiscussion bool + Rerun bool + FreshSession bool FailOn *review.Severity AllowSelfReview bool @@ -290,6 +294,7 @@ type Result struct { CurrentHeadSHA string ReviewBaseSHA string ReviewHeadSHA string + WithoutDiscussion bool ReviewerFailures []ReviewerFailure ReviewerCoverage []reviewplan.ReviewerCoverageSummary reviewerFastDelivered string @@ -376,9 +381,11 @@ type selectionSetupRequest struct { ReviewBaseSHA string ReviewHeadSHA string NoResolveThreads bool - ResolvedPR *reviewPRContext - InvocationRoot *string - ResolveArtifacts func(gitprovider.PR) (ArtifactPaths, error) + // WithoutDiscussion skips every PR discussion read, independent of pinning. + WithoutDiscussion bool + ResolvedPR *reviewPRContext + InvocationRoot *string + ResolveArtifacts func(gitprovider.PR) (ArtifactPaths, error) } type preparedSelectionContext struct { @@ -454,6 +461,9 @@ func Live(ctx context.Context, opts Options, req Request, run ledger.Run) (Resul if strings.TrimSpace(req.ReviewBaseSHA) != "" || strings.TrimSpace(req.ReviewHeadSHA) != "" { return Result{}, Failure(FailureTerminal, fmt.Errorf("pipeline: pinned review SHAs require dry-run review")) } + if req.WithoutDiscussion { + return Result{}, Failure(FailureTerminal, fmt.Errorf("pipeline: review without discussion requires dry-run review")) + } if strings.TrimSpace(run.RunID) == "" { return Result{}, Failure(FailureTerminal, fmt.Errorf("pipeline: live run ID is required")) } @@ -623,15 +633,16 @@ func execute(ctx context.Context, opts Options, req Request, mode executionMode) return Result{}, fmt.Errorf("pipeline: reviewer credentials resolve to PR author %q; pass --allow-self-review to continue", req.PostingIdentity.Login) } prepared, err := prepareSelectionContext(ctx, opts, selectionSetupRequest{ - PRRef: req.PRRef, - Profile: req.Profile, - PostingIdentity: req.PostingIdentity, - AgentDirs: req.AgentDirs, - ReviewBaseSHA: req.ReviewBaseSHA, - ReviewHeadSHA: req.ReviewHeadSHA, - NoResolveThreads: req.NoResolveThreads, - ResolvedPR: &reviewCtx, - InvocationRoot: &invocationRoot, + PRRef: req.PRRef, + Profile: req.Profile, + PostingIdentity: req.PostingIdentity, + AgentDirs: req.AgentDirs, + ReviewBaseSHA: req.ReviewBaseSHA, + ReviewHeadSHA: req.ReviewHeadSHA, + NoResolveThreads: req.NoResolveThreads, + WithoutDiscussion: req.WithoutDiscussion, + ResolvedPR: &reviewCtx, + InvocationRoot: &invocationRoot, ResolveArtifacts: func(reviewPR gitprovider.PR) (ArtifactPaths, error) { if mode.live { return ArtifactPathsFromDir(mode.run.ArtifactPath), nil @@ -655,6 +666,7 @@ func execute(ctx context.Context, opts Options, req Request, mode executionMode) opts.emitWarning(warning) result := prepared.reviewResult() + result.WithoutDiscussion = req.WithoutDiscussion run := mode.run if !mode.live { if resumedDryRun != nil { @@ -675,7 +687,7 @@ func execute(ctx context.Context, opts Options, req Request, mode executionMode) if err != nil { return Result{}, err } - if err := runartifact.WriteMarker(prepared.artifacts.Dir, runartifact.KindReview, run.RunID); err != nil { + if err := runartifact.WriteMarkerWithOptions(prepared.artifacts.Dir, runartifact.KindReview, run.RunID, runartifact.MarkerOptions{WithoutDiscussion: req.WithoutDiscussion}); err != nil { return Result{}, err } } @@ -780,15 +792,25 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu if err != nil { return nil, false, Failure(FailureTerminal, err) } - namedSession, err := prepareNamedSession(ctx, opts, req, mode.live, runtimeConfig.model, now) - if err != nil { - return nil, false, err - } - - cohortScope := ledger.ReviewerCohortScope{PRKey: prepared.prKey, Profile: req.ProfileName, PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity)} - selection, reviewerResumeIDs, reusedCohort, err := loadReviewerCohort(ctx, opts, req, cohortScope, prepared.catalog, reviewablePatchPaths(prepared.parsed.Patches), maxAgents) - if err != nil { - return executionPhaseFailure(err) + // A review without discussion must not resume the PR's orchestrator or + // reviewer sessions: they carry earlier discussion. It also leaves them + // untouched, so a replay never becomes the PR's next live context. The zero + // named-session state and cohort scope make every checkpoint a no-op. + var namedSession namedSessionState + var cohortScope ledger.ReviewerCohortScope + var selection llm.Selection + var reviewerResumeIDs map[string]string + reusedCohort := false + if !req.WithoutDiscussion { + namedSession, err = prepareNamedSession(ctx, opts, req, mode.live, runtimeConfig.model, now) + if err != nil { + return nil, false, err + } + cohortScope = ledger.ReviewerCohortScope{PRKey: prepared.prKey, Profile: req.ProfileName, PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity)} + selection, reviewerResumeIDs, reusedCohort, err = loadReviewerCohort(ctx, opts, req, cohortScope, prepared.catalog, reviewablePatchPaths(prepared.parsed.Patches), maxAgents) + if err != nil { + return executionPhaseFailure(err) + } } if reusedCohort { if err := restoreOrchestratorSessionFromRun(ctx, opts.Store, run.RunID, prepared.artifacts, &namedSession); err != nil { @@ -834,7 +856,7 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu if err != nil { return executionPhaseFailure(err) } - if !reusedCohort { + if !reusedCohort && !req.WithoutDiscussion { if err := persistReviewerCohort(ctx, opts, req, cohortScope, prepared.catalog, selection, now); err != nil { return nil, false, err } @@ -1009,7 +1031,13 @@ func findIncompleteDryRun(ctx context.Context, store Store, req Request, pr gitp } runs = append(runs, legacyRuns...) } - best, found := runlifecycle.NewestCompatibleIncompleteRun(runs, pr.Base.SHA, ledger.PostModeDryRun, runartifact.KindReview, runartifact.MarkerMatches) + // A run resumes only into a run with the same discussion setting, so a + // discussion-free replay never reuses tasks or sessions that saw discussion. + markerMatches := func(artifactPath, kind, runID string) bool { + marker, err := runartifact.ReadMarker(artifactPath, kind) + return err == nil && marker.RunID == runID && marker.WithoutDiscussion == req.WithoutDiscussion + } + best, found := runlifecycle.NewestCompatibleIncompleteRun(runs, pr.Base.SHA, ledger.PostModeDryRun, runartifact.KindReview, markerMatches) return best, found, nil } @@ -1134,7 +1162,9 @@ func prepareSelectionContext(ctx context.Context, opts Options, req selectionSet var threadContext []threadcontext.Thread var reviews []gitprovider.Review var issueComments []gitprovider.IssueComment - if !reviewCtx.pinnedReview { + // Discussion is cut off here, at its only source, so no later consumer + // (dossier, selection, thread analysis, reviewers, planner) can see it. + if !reviewCtx.pinnedReview && !req.WithoutDiscussion { threads, err = opts.Provider.ListInlineThreads(ctx, req.PRRef) if err != nil { return preparedSelectionContext{}, err @@ -1202,6 +1232,7 @@ func prepareSelectionContext(ctx context.Context, opts Options, req selectionSet CurrentPR: pr, ReviewPR: reviewPR, PinnedReview: reviewCtx.pinnedReview, + WithoutDiscussion: req.WithoutDiscussion, ChangedFiles: dossierChangedFiles(parsed.Patches), Threads: threads, ThreadContext: threadContext, @@ -1210,13 +1241,20 @@ func prepareSelectionContext(ctx context.Context, opts Options, req selectionSet Catalog: catalog, CurrentBaseSHA: reviewCtx.currentBaseSHA, CurrentHeadSHA: reviewCtx.currentHeadSHA, - DiscussionOmittedNote: "Current PR discussion omitted because this review is pinned to explicit base/head SHAs.", + DiscussionOmittedNote: discussionOmittedNote(req.WithoutDiscussion), }); err != nil { return preparedSelectionContext{}, err } return out, nil } +func discussionOmittedNote(withoutDiscussion bool) string { + if withoutDiscussion { + return "No PR discussion is provided for this review." + } + return "Current PR discussion omitted because this review is pinned to explicit base/head SHAs." +} + func resolveReviewPRContext(ctx context.Context, provider ReadProvider, ref gitprovider.PRRef, reviewBaseSHA, reviewHeadSHA string) (reviewPRContext, error) { pr, err := provider.GetPR(ctx, ref) if err != nil { @@ -2125,6 +2163,9 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p if req.ReviewerFast { fingerprintDeps = append(fingerprintDeps, "fast=true") } + if req.WithoutDiscussion { + fingerprintDeps = append(fingerprintDeps, "without_discussion=true") + } findings, session, ledgerSession, err := runStructuredTask(ctx, opts, llmTaskSpec{ runID: runID, taskID: taskID, @@ -2231,6 +2272,9 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p if req.ReviewerFast { repairFingerprintDeps = append(repairFingerprintDeps, "fast=true") } + if req.WithoutDiscussion { + repairFingerprintDeps = append(repairFingerprintDeps, "without_discussion=true") + } repair, repairSession, repairLedgerSession, repairErr := runStructuredTask(ctx, opts, llmTaskSpec{ runID: runID, taskID: repairTaskID, @@ -3420,6 +3464,9 @@ func validate(opts Options, req Request) error { if strings.TrimSpace(req.PRURL) == "" { return fmt.Errorf("pipeline: PR URL is required") } + if req.WithoutDiscussion && (strings.TrimSpace(req.ReviewBaseSHA) == "" || strings.TrimSpace(req.ReviewHeadSHA) == "") { + return fmt.Errorf("pipeline: review without discussion requires pinned review base and head SHAs") + } if strings.TrimSpace(runlifecycle.PostingKey(req.PostingIdentity)) == "" { return fmt.Errorf("pipeline: posting identity is required") } diff --git a/internal/pipeline/without_discussion_test.go b/internal/pipeline/without_discussion_test.go new file mode 100644 index 0000000..1c2685c --- /dev/null +++ b/internal/pipeline/without_discussion_test.go @@ -0,0 +1,354 @@ +package pipeline + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/dossier" + "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/review" + "github.com/open-cli-collective/codereview-cli/internal/runartifact" + "github.com/open-cli-collective/codereview-cli/internal/runlifecycle" + "github.com/open-cli-collective/codereview-cli/internal/statepaths" +) + +// Distinct text for each kind of existing discussion, so any leak into a +// prompt or dossier file is caught by a plain substring search. +const ( + discussionThreadMarker = "DISCUSSION-THREAD-MARKER" + discussionCommentMarker = "DISCUSSION-ISSUE-COMMENT-MARKER" + discussionReviewMarker = "DISCUSSION-PRIOR-REVIEW-MARKER" + discussionCRThreadMarker = "Original finding." + priorOrchestratorSession = "prior-orchestrator-session" + priorReviewerSession = "prior-reviewer-session" +) + +var discussionMarkers = []string{discussionThreadMarker, discussionCommentMarker, discussionReviewMarker} + +func addExistingDiscussion(provider *readOnlyProvider, bot gitprovider.Identity) { + human := gitprovider.Identity{Login: "maintainer", ID: "maintainer-id"} + provider.threads = []gitprovider.InlineThread{{ + ID: "thread-human", + Path: "main.go", + Side: review.DiffSideRight, + Line: 2, + SubjectType: review.AnchorKindLine, + Comments: []gitprovider.ThreadComment{{ + ID: "thread-human-1", + Body: discussionThreadMarker + " this looks wrong", + Author: human, + }}, + }} + provider.issueComments = []gitprovider.IssueComment{{ + ID: "issue-1", + Body: discussionCommentMarker + " please also handle the empty case", + Author: human, + }} + provider.reviews = []gitprovider.Review{{ + ID: "review-1", + Body: discussionReviewMarker + " earlier cr review summary", + Author: bot, + Event: review.ReviewEventComment, + }} +} + +// storePriorSessions stores the PR's default orchestrator session and reviewer +// cohort as an earlier live review would have left them. +func storePriorSessions(t *testing.T, ctx context.Context, store *ledger.Store, provider *readOnlyProvider, req Request) (string, ledger.ReviewerCohortScope) { + t.Helper() + // The earlier live review that left these sessions behind. + allocateLiveRun(t, store, provider, req, "run-prior-live") + name, err := defaultSessionName(req) + if err != nil { + t.Fatalf("defaultSessionName: %v", err) + } + stored := namedSessionForRequest(req, priorOrchestratorSession) + stored.Name = name + if err := store.UpsertNamedSession(ctx, stored); err != nil { + t.Fatalf("UpsertNamedSession: %v", err) + } + prKey, err := statepaths.PRKey(req.PRRef.Host, req.PRRef.Owner, req.PRRef.Repo, req.PRRef.Number) + if err != nil { + t.Fatalf("PRKey: %v", err) + } + scope := ledger.ReviewerCohortScope{PRKey: prKey, Profile: req.ProfileName, PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity)} + if err := store.ReplaceReviewerCohort(ctx, ledger.ReviewerCohort{ + Scope: scope, Adapter: "fake-llm", CreatedAt: fixedNow().Add(-time.Hour), UpdatedAt: fixedNow().Add(-time.Hour), + Members: []ledger.ReviewerCohortMember{{ + AgentID: "harness:reviewer", AssignmentMode: ledger.ReviewerAssignmentScoped, Files: []string{"main.go"}, + Model: "claude-sonnet-5", Effort: "medium", ProviderSessionID: priorReviewerSession, + }}, + }); err != nil { + t.Fatalf("ReplaceReviewerCohort: %v", err) + } + return name, scope +} + +func pinnedDiscussionHarness(t *testing.T) (*readOnlyProvider, Request) { + t.Helper() + provider, req := dryRunHarness(t) + fixture, reviewBaseSHA, reviewHeadSHA := newPinnedReviewFixtureForRef(t, req.PRRef) + provider.pr = fixture.pr + provider.pr.Title = "Replay title" + provider.pr.Body = "Replay description stays visible." + addRepoAgentFixture(provider) + provider.fixtureRepoDir = fixture.repoDir + provider.diffBetween = gitprovider.UnifiedDiff{Raw: smallDiff("main.go")} + req.ReviewBaseSHA = reviewBaseSHA + req.ReviewHeadSHA = reviewHeadSHA + addExistingDiscussion(provider, req.PostingIdentity) + provider.threads = append(provider.threads, markedReviewThread(t, "thread-cr", "main.go", 2, req.PostingIdentity, gitprovider.Identity{Login: "maintainer", ID: "maintainer-id"})) + return provider, req +} + +func withoutDiscussionOptions(t *testing.T, provider *readOnlyProvider, adapter llm.Adapter, store *ledger.Store, runID string) Options { + t.Helper() + return Options{ + Provider: provider, + Adapter: adapter, + Store: store, + NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), + Now: fixedNow, + NewRunID: func() string { return runID }, + NewSessionRowID: sequence("session"), + NewFindingID: findingSequence("finding"), + NewActionID: actionSequence(), + MaxConcurrency: 1, + } +} + +// allPrompts returns every prompt the adapter received, started or resumed. +func allPrompts(adapter *llm.FakeAdapter) []string { + var prompts []string + for _, request := range adapter.Requests() { + prompts = append(prompts, request.Prompt) + } + for _, resume := range adapter.Resumes() { + prompts = append(prompts, resume.Request.Prompt) + } + return prompts +} + +// dossierText concatenates every file the run wrote under its dossier. +func dossierText(t *testing.T, dir string) string { + t.Helper() + var out strings.Builder + err := filepath.WalkDir(dir, func(path string, entry os.DirEntry, err error) error { + if err != nil || entry.IsDir() { + return err + } + data, err := os.ReadFile(path) // #nosec G304 -- test reads its own temp artifacts. + if err != nil { + return err + } + out.WriteString(path) + out.WriteString("\n") + out.Write(data) + out.WriteString("\n") + return nil + }) + if err != nil { + t.Fatalf("walk dossier: %v", err) + } + return out.String() +} + +func TestDryRunWithoutDiscussionReplaysAsFirstPass(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := pinnedDiscussionHarness(t) + req.WithoutDiscussion = true + sessionName, cohortScope := storePriorSessions(t, ctx, store, provider, req) + adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} + adapter.Queue(fakeLLMResult("selection-replay", selectionJSON("harness:reviewer", "main.go"), 10, 2)) + adapter.Queue(fakeLLMResult("reviewer-replay", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) + adapter.Queue(fakeLLMResult("rollup-replay", rollupJSON("comment", []string{"finding-1"}), 30, 6)) + + result, err := dryRunForTest(ctx, withoutDiscussionOptions(t, provider, adapter, store, "run-replay"), req) + if err != nil { + t.Fatalf("DryRun: %v", err) + } + + if provider.threadCalls != 0 || provider.reviewCalls != 0 || provider.issueCommentCalls != 0 { + t.Fatalf("discussion reads = threads %d reviews %d comments %d, want none", provider.threadCalls, provider.reviewCalls, provider.issueCommentCalls) + } + if resumes := adapter.Resumes(); len(resumes) != 0 { + t.Fatalf("resumes = %#v, want no prior orchestrator or reviewer session resumed", resumes) + } + prompts := allPrompts(adapter) + if len(prompts) != 3 { + t.Fatalf("prompts = %d, want selection, reviewer, rollup", len(prompts)) + } + for i, prompt := range prompts { + for _, leaked := range append(discussionMarkers, discussionCRThreadMarker, "discussion_outcomes") { + if strings.Contains(prompt, leaked) { + t.Fatalf("prompt %d contains discussion %q:\n%s", i, leaked, prompt) + } + } + } + if !strings.Contains(prompts[0], "Replay title") || !strings.Contains(prompts[0], "Replay description stays visible.") { + t.Fatalf("selection prompt lost PR title/description:\n%s", prompts[0]) + } + dossierFiles := dossierText(t, result.Artifacts.DossierDir) + for _, leaked := range append(discussionMarkers, discussionCRThreadMarker) { + if strings.Contains(dossierFiles, leaked) { + t.Fatalf("dossier contains discussion %q:\n%s", leaked, dossierFiles) + } + } + summary, err := dossier.ReadDiscussionSummary(result.Artifacts) + if err != nil { + t.Fatalf("ReadDiscussionSummary: %v", err) + } + if !summary.WithoutDiscussion || len(summary.TopLevelComments) != 0 || len(summary.InlineThreads) != 0 { + t.Fatalf("discussion summary = %#v, want empty and marked without discussion", summary) + } + + marker, err := runartifact.ReadMarker(result.Artifacts.Dir, runartifact.KindReview) + if err != nil { + t.Fatalf("ReadMarker: %v", err) + } + if !marker.WithoutDiscussion || !result.WithoutDiscussion { + t.Fatalf("marker/result without discussion = %v/%v, want recorded", marker.WithoutDiscussion, result.WithoutDiscussion) + } + if result.NamedSessionCandidate != nil { + t.Fatalf("named session candidate = %#v, want none", result.NamedSessionCandidate) + } + storedSession, err := store.GetNamedSession(ctx, sessionName) + if err != nil { + t.Fatalf("GetNamedSession: %v", err) + } + if storedSession.ProviderSessionID != priorOrchestratorSession { + t.Fatalf("orchestrator session = %q, want untouched %q", storedSession.ProviderSessionID, priorOrchestratorSession) + } + cohort, err := store.GetReviewerCohort(ctx, cohortScope) + if err != nil { + t.Fatalf("GetReviewerCohort: %v", err) + } + if len(cohort.Members) != 1 || cohort.Members[0].ProviderSessionID != priorReviewerSession { + t.Fatalf("reviewer cohort = %#v, want untouched %q", cohort, priorReviewerSession) + } +} + +// Without the flag, a pinned replay still resumes the PR's prior sessions, +// which is the leak the flag closes. +func TestDryRunPinnedWithDiscussionStillResumesPriorSessions(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := pinnedDiscussionHarness(t) + storePriorSessions(t, ctx, store, provider, req) + adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} + adapter.Queue(fakeLLMResult("reviewer-resumed", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) + adapter.Queue(fakeLLMResult("rollup-resumed", rollupJSON("comment", []string{"finding-1"}), 30, 6)) + + result, err := dryRunForTest(ctx, withoutDiscussionOptions(t, provider, adapter, store, "run-pinned"), req) + if err != nil { + t.Fatalf("DryRun: %v", err) + } + resumed := map[string]bool{} + for _, resume := range adapter.Resumes() { + resumed[resume.SessionID] = true + } + if !resumed[priorReviewerSession] || !resumed[priorOrchestratorSession] { + t.Fatalf("resumes = %#v, want prior reviewer and orchestrator sessions", adapter.Resumes()) + } + if result.WithoutDiscussion { + t.Fatal("result.WithoutDiscussion = true, want false") + } + marker, err := runartifact.ReadMarker(result.Artifacts.Dir, runartifact.KindReview) + if err != nil { + t.Fatalf("ReadMarker: %v", err) + } + if marker.WithoutDiscussion { + t.Fatal("marker records without_discussion for a run with discussion") + } +} + +// Without the flag and without pinning, the same discussion fixture reaches +// the dossier and the selection prompt, so the assertions above would see a leak. +func TestDryRunWithDiscussionFeedsExistingDiscussion(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + addExistingDiscussion(provider, req.PostingIdentity) + adapter := &llm.FakeAdapter{NameValue: "fake-llm"} + adapter.Queue(fakeLLMResult("dossier-summary-session", discussionSummaryJSON([]string{discussionCommentMarker}, nil), 8, 2)) + adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) + adapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) + adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) + + result, err := dryRunForTest(ctx, withoutDiscussionOptions(t, provider, adapter, store, "run-with-discussion"), req) + if err != nil { + t.Fatalf("DryRun: %v", err) + } + if provider.threadCalls == 0 || provider.reviewCalls == 0 || provider.issueCommentCalls == 0 { + t.Fatalf("discussion reads = threads %d reviews %d comments %d, want all read", provider.threadCalls, provider.reviewCalls, provider.issueCommentCalls) + } + dossierFiles := dossierText(t, result.Artifacts.DossierDir) + for _, want := range discussionMarkers { + if !strings.Contains(dossierFiles, want) { + t.Fatalf("dossier missing discussion %q", want) + } + } + prompts := allPrompts(adapter) + selectionPrompt := prompts[1] + if !strings.Contains(selectionPrompt, discussionThreadMarker) || !strings.Contains(selectionPrompt, discussionCommentMarker) { + t.Fatalf("selection prompt missing existing discussion:\n%s", selectionPrompt) + } +} + +// An incomplete run made with discussion is never resumed by a replay without +// it, because its tasks and sessions may have seen that discussion. +func TestDryRunWithoutDiscussionDoesNotResumeRunWithDiscussion(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := pinnedDiscussionHarness(t) + opts := withoutDiscussionOptions(t, provider, &llm.FakeAdapter{NameValue: "fake-llm"}, store, "run-fresh-replay") + provider.diffBetween = gitprovider.UnifiedDiff{} + withDiscussion := allocateDryRunForSHAs(t, store, opts.Layout, req, "run-with-discussion", req.ReviewHeadSHA, req.ReviewBaseSHA, fixedNow().Add(-time.Minute)) + req.WithoutDiscussion = true + + result, err := dryRunForTest(ctx, opts, req) + if err != nil { + t.Fatalf("DryRun: %v", err) + } + if result.Run.RunID == withDiscussion.RunID || result.Run.RunID != "run-fresh-replay" { + t.Fatalf("run = %q, want a fresh run instead of resuming %q", result.Run.RunID, withDiscussion.RunID) + } + marker, err := runartifact.ReadMarker(result.Artifacts.Dir, runartifact.KindReview) + if err != nil { + t.Fatalf("ReadMarker: %v", err) + } + if !marker.WithoutDiscussion { + t.Fatal("fresh replay marker does not record without_discussion") + } +} + +func TestPipelineRejectsWithoutDiscussionOutsidePinnedDryRun(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + req.WithoutDiscussion = true + opts := withoutDiscussionOptions(t, provider, &llm.FakeAdapter{NameValue: "fake-llm"}, store, "run-unpinned") + if _, err := dryRunForTest(ctx, opts, req); err == nil || !strings.Contains(err.Error(), "requires pinned review base and head SHAs") { + t.Fatalf("DryRun error = %v, want pinned SHA requirement", err) + } + run := allocateLiveRun(t, store, provider, Request{PRRef: req.PRRef, PRURL: req.PRURL, ProfileName: req.ProfileName, Profile: req.Profile, PostingIdentity: req.PostingIdentity}, "run-live") + if _, err := liveForTest(ctx, opts, req, run); err == nil || !strings.Contains(err.Error(), "requires dry-run review") { + t.Fatalf("Live error = %v, want dry-run requirement", err) + } + if provider.threadCalls != 0 || provider.reviewCalls != 0 || provider.issueCommentCalls != 0 { + t.Fatalf("discussion reads = %d/%d/%d, want none before rejection", provider.threadCalls, provider.reviewCalls, provider.issueCommentCalls) + } +} diff --git a/internal/runartifact/runartifact.go b/internal/runartifact/runartifact.go index b2e01a4..ea4f0fc 100644 --- a/internal/runartifact/runartifact.go +++ b/internal/runartifact/runartifact.go @@ -206,17 +206,32 @@ type Marker struct { SchemaVersion int `json:"schema_version"` Kind string `json:"kind"` RunID string `json:"run_id"` + // WithoutDiscussion records a review run that was given no PR discussion. + WithoutDiscussion bool `json:"without_discussion,omitempty"` +} + +// MarkerOptions records run options that make one run's artifacts +// incompatible with another run's. +type MarkerOptions struct { + WithoutDiscussion bool } // WriteMarker persists the run-kind discriminator for an artifact root. func WriteMarker(artifactPath, kind, runID string) error { + return WriteMarkerWithOptions(artifactPath, kind, runID, MarkerOptions{}) +} + +// WriteMarkerWithOptions persists the run-kind discriminator and run options +// for an artifact root. +func WriteMarkerWithOptions(artifactPath, kind, runID string, options MarkerOptions) error { if _, err := markerFile(kind); err != nil { return err } data, err := json.MarshalIndent(Marker{ - SchemaVersion: markerSchema, - Kind: kind, - RunID: runID, + SchemaVersion: markerSchema, + Kind: kind, + RunID: runID, + WithoutDiscussion: options.WithoutDiscussion, }, "", " ") if err != nil { return err diff --git a/internal/view/review.go b/internal/view/review.go index 818a7f1..8c02964 100644 --- a/internal/view/review.go +++ b/internal/view/review.go @@ -127,6 +127,8 @@ type ReviewRun struct { HeadSHA string `json:"head_sha,omitempty"` CurrentBaseSHA string `json:"current_base_sha,omitempty"` CurrentHeadSHA string `json:"current_head_sha,omitempty"` + // WithoutDiscussion marks a replay that ran without the PR's discussion. + WithoutDiscussion bool `json:"without_discussion,omitempty"` } // ReviewOutbox summarizes live posting state. @@ -187,14 +189,15 @@ func NewReviewDryRun(result pipeline.Result) (ReviewDryRun, error) { } rendered := ReviewDryRun{ Run: ReviewRun{ - RunID: result.Run.RunID, - PRURL: result.PR.URL, - PRKey: result.PRKey, - PostMode: result.Run.PostMode.String(), - Outcome: outcome, - ArtifactPath: result.Run.ArtifactPath, - BaseSHA: result.ReviewBaseSHA, - HeadSHA: result.ReviewHeadSHA, + RunID: result.Run.RunID, + PRURL: result.PR.URL, + PRKey: result.PRKey, + PostMode: result.Run.PostMode.String(), + Outcome: outcome, + ArtifactPath: result.Run.ArtifactPath, + BaseSHA: result.ReviewBaseSHA, + HeadSHA: result.ReviewHeadSHA, + WithoutDiscussion: result.WithoutDiscussion, }, RollupMarkdown: result.Plan.RollupMarkdown, Summary: newReviewSummary(result.Plan.Summary), From 3a622e65a3b87db28e21cff7be0aeb81ccfe5ff3 Mon Sep 17 00:00:00 2001 From: Aaron Wong <6979793+zzwong@users.noreply.github.com> Date: Thu, 24 Sep 2026 03:06:57 -0400 Subject: [PATCH 2/2] fix(review): keep discussion-free replays from looking up the PR A --without-discussion replay kept its discussion out of every prompt, but a reviewer with a shell could still fetch it with gh, curl, or the GitHub API. Such replays now remove the git remotes from the workbench and each reviewer checkout, deny claude_cli reviewers WebFetch, WebSearch, and Bash commands that reach the network (gh, curl, wget, git fetch/pull/ls-remote and similar), and run codex_cli reviewers with web search and sandbox network access off. The adapters' argument validation requires these restrictions exactly when the workspace is marked offline. This narrows the tool surface rather than sandboxing the network, and the README says so. --fresh-session is now rejected with --without-discussion, since a replay never reuses or resets PR sessions. --- README.md | 2 +- internal/cmd/reviewcmd/reviewcmd.go | 5 +- internal/cmd/reviewcmd/reviewcmd_test.go | 1 + internal/llm/adapter.go | 4 + internal/llmadapters/subprocess.go | 76 ++++++++++++++++ internal/llmadapters/subprocess_test.go | 91 ++++++++++++++++++++ internal/pipeline/pipeline.go | 5 +- internal/pipeline/without_discussion_test.go | 8 ++ internal/workbench/workbench.go | 54 +++++++++++- internal/workbench/workbench_test.go | 41 +++++++++ 10 files changed, 279 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index a918f9a..8ebffb2 100644 --- a/README.md +++ b/README.md @@ -1227,7 +1227,7 @@ Review selection and execution flags: | `--reviewer-effort ` | Override reviewer-stage effort with `low`, `medium`, `high`, `xhigh`, or `max`, subject to runtime support. Available for dry-run, no-post, and live reviews. | | `--review-base-sha ` | Review this base commit SHA instead of the PR's current base SHA. Requires `--review-head-sha` and `--dry-run` or `--no-post`. | | `--review-head-sha ` | Review this head commit SHA instead of the PR's current head SHA. Requires `--review-base-sha` and `--dry-run` or `--no-post`. | -| `--without-discussion` | Replay the pinned review as a first pass. No review threads, thread outcomes, issue comments, or prior reviews are read, and the PR's orchestrator session and reviewer cohort sessions are neither resumed nor updated, so selection, reviewers, and rollup see only the PR title, description, and diff. The run marker (`review-run.json`), dossier discussion artifacts, and `--json` run output record `without_discussion: true`, and such a run never resumes an incomplete run made with discussion (or the reverse). Requires `--review-base-sha`, `--review-head-sha`, and `--dry-run` or `--no-post`. | +| `--without-discussion` | Replay the pinned review as a first pass. cr reads no review threads, thread outcomes, issue comments, or prior reviews, and neither resumes nor updates the PR's orchestrator session or reviewer cohort sessions, so the discussion is not in any selection, reviewer, or rollup prompt; the PR title, description, and diff still are. Reviewers also get a narrower tool surface: the review checkouts have no git remotes, `claude_cli` reviewers are denied WebFetch, WebSearch, and Bash commands such as `gh`, `curl`, `wget`, and `git fetch`/`pull`/`ls-remote`, and `codex_cli` reviewers run with web search and sandbox network access off. That blocks the obvious lookups but is not a network sandbox, so a reviewer with a shell could still reach the network another way. The run marker (`review-run.json`), dossier discussion artifacts, and `--json` run output record `without_discussion: true`, and such a run never resumes an incomplete run made with discussion (or the reverse). Requires `--review-base-sha`, `--review-head-sha`, and `--dry-run` or `--no-post`; cannot be combined with `--fresh-session`. | | `--session ` | Override the PR's default orchestrator session with a named live-review session. Reviewer cohorts remain PR-scoped. Not allowed with `--dry-run`, `--no-post`, or `--retry-posts`. | Review progress on stderr reports the merged reviewer catalog, final selected diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index 1d2a2f1..807e55b 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -125,7 +125,7 @@ func RegisterWithFactory(rootCmd *cobra.Command, opts *root.Options, factory Run cmd.Flags().StringVar(&flags.reviewerEffort, "reviewer-effort", "", "Override reviewer effort for this review") cmd.Flags().StringVar(&flags.reviewBaseSHA, "review-base-sha", "", "Review this base commit SHA instead of the PR's current base SHA; requires --dry-run and --review-head-sha") cmd.Flags().StringVar(&flags.reviewHeadSHA, "review-head-sha", "", "Review this head commit SHA instead of the PR's current head SHA; requires --dry-run and --review-base-sha") - cmd.Flags().BoolVar(&flags.withoutDiscussion, "without-discussion", false, "Replay the pinned review as a first pass, without the PR's existing discussion or review sessions; requires --dry-run, --review-base-sha, and --review-head-sha") + cmd.Flags().BoolVar(&flags.withoutDiscussion, "without-discussion", false, "Replay the pinned review as a first pass, without the PR's existing discussion or review sessions, and deny reviewers common network tools; requires --dry-run, --review-base-sha, and --review-head-sha") cmd.Flags().IntVar(&flags.maxAgents, "max-agents", 0, "Maximum selected reviewer agents") cmd.Flags().IntVar(&flags.maxConcurrency, "max-concurrency", 0, "Maximum concurrent reviewer agents") cmd.Flags().BoolVar(&flags.allowSelfReview, "allow-self-review", false, "Allow reviewer credentials matching the PR author") @@ -222,6 +222,9 @@ func runReview(ctx context.Context, cmd *cobra.Command, opts *root.Options, fact if sessionName != "" && flags.retryPosts { return exitcode.Usage(fmt.Errorf("--session cannot be used with --retry-posts")) } + if flags.freshSession && flags.withoutDiscussion { + return exitcode.Usage(fmt.Errorf("--fresh-session cannot be used with --without-discussion, which never reuses or resets PR sessions")) + } if flags.freshSession && flags.retryPosts { return exitcode.Usage(fmt.Errorf("--fresh-session cannot be used with --retry-posts")) } diff --git a/internal/cmd/reviewcmd/reviewcmd_test.go b/internal/cmd/reviewcmd/reviewcmd_test.go index d812f1a..6bcd765 100644 --- a/internal/cmd/reviewcmd/reviewcmd_test.go +++ b/internal/cmd/reviewcmd/reviewcmd_test.go @@ -609,6 +609,7 @@ func TestReviewRejectsWithoutDiscussionOutsidePinnedDryRun(t *testing.T) { {name: "dry run without SHAs", args: []string{"--dry-run", "--without-discussion"}, wantErr: "--without-discussion requires --review-base-sha and --review-head-sha"}, {name: "no post without SHAs", args: []string{"--no-post", "--without-discussion"}, wantErr: "--without-discussion requires --review-base-sha and --review-head-sha"}, {name: "dry run with base only", args: []string{"--dry-run", "--review-base-sha", "1111111", "--without-discussion"}, wantErr: "must be set together"}, + {name: "fresh session", args: []string{"--dry-run", "--review-base-sha", "1111111", "--review-head-sha", "2222222", "--without-discussion", "--fresh-session"}, wantErr: "--fresh-session cannot be used with --without-discussion"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/llm/adapter.go b/internal/llm/adapter.go index 0395d22..60c3cb3 100644 --- a/internal/llm/adapter.go +++ b/internal/llm/adapter.go @@ -64,6 +64,10 @@ type ReviewerWorkspaceRequest struct { Env []string AllowedFiles []string MaxToolOutputBytes int + // NoNetwork asks the adapter to deny reviewer tools that reach the network + // (git hosts, web fetch, web search), so a reviewer cannot look up live PR + // state such as discussion. It narrows the tool surface; it is not a sandbox. + NoNetwork bool } // ErrReviewerWorkspaceUnsupported reports that an adapter cannot use a prepared diff --git a/internal/llmadapters/subprocess.go b/internal/llmadapters/subprocess.go index 5c3b9f3..4e6feae 100644 --- a/internal/llmadapters/subprocess.go +++ b/internal/llmadapters/subprocess.go @@ -12,6 +12,7 @@ import ( "os/exec" "path/filepath" "regexp" + "slices" "strconv" "strings" "sync" @@ -345,6 +346,7 @@ func buildClaudeForegroundArgs(req Request, scratch string, resumeSessionID stri if workspace := req.ReviewerWorkspace; workspace != nil { args = append(args, "--add-dir", workspace.RepoDir) } + args = appendClaudeNoNetworkArgs(args, req) if resumeSessionID != "" { args = append(args, "--resume", resumeSessionID) } @@ -360,6 +362,33 @@ func buildClaudeForegroundArgs(req Request, scratch string, resumeSessionID stri return append(args, "--", claudeBGPositionalPrompt(scratch)) } +// claudeNoNetworkDisallowedTools denies the ways a Claude reviewer could look +// up live PR state from its workspace: web tools, the GitHub CLI, HTTP +// clients, and git commands that talk to a remote. Bash can still reach the +// network by other means, so this narrows the tool surface; it is not a sandbox. +var claudeNoNetworkDisallowedTools = strings.Join([]string{ + "WebFetch", + "WebSearch", + "Bash(gh *)", + "Bash(curl *)", + "Bash(wget *)", + "Bash(git fetch *)", + "Bash(git pull *)", + "Bash(git ls-remote *)", + "Bash(git clone *)", + "Bash(git remote *)", + "Bash(git -C * fetch *)", + "Bash(git -C * pull *)", + "Bash(git -C * ls-remote *)", +}, ",") + +func appendClaudeNoNetworkArgs(args []string, req Request) []string { + if req.ReviewerWorkspace == nil || !req.ReviewerWorkspace.NoNetwork { + return args + } + return append(args, "--disallowedTools", claudeNoNetworkDisallowedTools) +} + // startClaude picks the transport for a Claude task. Background mode is the // default and foreground is the documented-sturdier fallback, so a background // job that fails without producing a result is retried once in foreground @@ -774,6 +803,7 @@ func (a *SubprocessAdapter) buildArgsForSession(req Request, scratch string, res if workspace := req.ReviewerWorkspace; workspace != nil { args = append(args, "--add-dir", workspace.RepoDir) } + args = appendClaudeNoNetworkArgs(args, req) if resumeSessionID != "" { args = append(args, "--resume", resumeSessionID) } @@ -803,6 +833,7 @@ func (a *SubprocessAdapter) buildArgsForSession(req Request, scratch string, res if req.Fast { args = append(args, "-c", `service_tier="fast"`) } + args = appendCodexNoNetworkArgs(args, req) if resumeSessionID != "" { args = subprocessCodexResumeArgs() if req.Model != "" { @@ -814,6 +845,7 @@ func (a *SubprocessAdapter) buildArgsForSession(req Request, scratch string, res if req.Fast { args = append(args, "-c", `service_tier="fast"`) } + args = appendCodexNoNetworkArgs(args, req) args = append(args, resumeSessionID) } return append(args, "--", req.Prompt), nil @@ -822,6 +854,24 @@ func (a *SubprocessAdapter) buildArgsForSession(req Request, scratch string, res } } +// codexNoNetworkConfig turns off Codex web search (on by default, in cached +// mode) and pins the workspace-write sandbox's network access off, so a +// reviewer cannot look up live PR state. +var codexNoNetworkConfig = []string{ + `web_search="disabled"`, + "sandbox_workspace_write.network_access=false", +} + +func appendCodexNoNetworkArgs(args []string, req Request) []string { + if req.ReviewerWorkspace == nil || !req.ReviewerWorkspace.NoNetwork { + return args + } + for _, value := range codexNoNetworkConfig { + args = append(args, "-c", value) + } + return args +} + func subprocessCodexBaseArgs(prefix []string, sandbox, cwd string, durable bool) []string { args := append([]string(nil), prefix...) args = append(args, "--json") @@ -874,9 +924,13 @@ func (a *SubprocessAdapter) validateArgs(args []string, scratch string, req Requ "--model": true, "--effort": true, "--settings": true, + "--disallowedTools": true, }); err != nil { return err } + if err := validateClaudeNoNetworkArgs(checkedArgs, req); err != nil { + return err + } // Exactly one transport: the detached job service (--bg) or headless // foreground print mode (-p, see claudeForegroundEnabled). if containsFlag(checkedArgs, "--bg") == containsFlag(checkedArgs, "-p") { @@ -950,6 +1004,28 @@ func (a *SubprocessAdapter) validateArgs(args []string, scratch string, req Requ return fmt.Errorf("%w: missing %s", ErrUnsafeSubprocessConfig, flag) } } + if workspace != nil && workspace.NoNetwork { + configs := flagValues(checkedArgs, "-c") + for _, want := range codexNoNetworkConfig { + if !slices.Contains(configs, want) { + return fmt.Errorf("%w: codex_cli offline reviewer must pass -c %s", ErrUnsafeSubprocessConfig, want) + } + } + } + } + return nil +} + +func validateClaudeNoNetworkArgs(args []string, req Request) error { + got, ok := flagValueOK(args, "--disallowedTools") + if req.ReviewerWorkspace != nil && req.ReviewerWorkspace.NoNetwork { + if !ok || got != claudeNoNetworkDisallowedTools { + return fmt.Errorf("%w: claude_cli offline reviewer must deny network tools", ErrUnsafeSubprocessConfig) + } + return nil + } + if ok { + return fmt.Errorf("%w: claude_cli must not pass --disallowedTools without an offline reviewer workspace", ErrUnsafeSubprocessConfig) } return nil } diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index 9f789a8..1a3d538 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strconv" "strings" "testing" @@ -2584,3 +2585,93 @@ func TestSubprocessClaudeBackgroundNothingLeftBehindIsRetried(t *testing.T) { // attempted. assertClaudeLaunchCount(t, readHelperRecords(t, recordPath), true) } + +func offlineTestRequest(t *testing.T, noNetwork bool) (Request, string) { + t.Helper() + tempDir := t.TempDir() + scratch := filepath.Join(tempDir, "scratch") + repoRoot := filepath.Join(tempDir, "repo") + for _, dir := range []string{scratch, repoRoot} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll(%s): %v", dir, err) + } + } + return Request{ + Prompt: "prompt", + ReviewerWorkspace: &ReviewerWorkspaceRequest{ + RepoDir: repoRoot, + ScratchDir: scratch, + NoNetwork: noNetwork, + }, + }, scratch +} + +func TestSubprocessClaudeDeniesNetworkToolsOnlyForOfflineReviewer(t *testing.T) { + adapter := NewClaudeCLIAdapter(SubprocessOptions{}) + for _, noNetwork := range []bool{false, true} { + req, scratch := offlineTestRequest(t, noNetwork) + background, err := adapter.buildArgsForSession(req, scratch, "") + if err != nil { + t.Fatalf("buildArgsForSession: %v", err) + } + resumed, err := adapter.buildArgsForSession(req, scratch, "session-1") + if err != nil { + t.Fatalf("buildArgsForSession(resume): %v", err) + } + foreground := buildClaudeForegroundArgs(req, scratch, "") + for name, args := range map[string][]string{"background": background, "resumed": resumed, "foreground": foreground} { + got, ok := flagValueOK(argsBeforePrompt(args), "--disallowedTools") + if noNetwork { + if !ok { + t.Fatalf("%s offline args = %v, want --disallowedTools", name, args) + } + for _, rule := range []string{"WebFetch", "WebSearch", "Bash(gh *)", "Bash(curl *)", "Bash(wget *)", "Bash(git fetch *)", "Bash(git pull *)", "Bash(git ls-remote *)"} { + if !slices.Contains(strings.Split(got, ","), rule) { + t.Fatalf("%s disallowed tools = %q, missing %q", name, got, rule) + } + } + } else if ok { + t.Fatalf("%s args = %v, want no --disallowedTools without the offline flag", name, args) + } + if err := adapter.validateArgs(args, scratch, req); err != nil { + t.Fatalf("%s validateArgs(noNetwork=%v): %v", name, noNetwork, err) + } + } + if noNetwork { + if err := adapter.validateArgs(removeFlagPair(background, "--disallowedTools"), scratch, req); !errors.Is(err, ErrUnsafeSubprocessConfig) { + t.Fatalf("validateArgs without deny rules = %v, want unsafe config", err) + } + } + } +} + +func TestSubprocessCodexDisablesNetworkOnlyForOfflineReviewer(t *testing.T) { + adapter := NewCodexCLIAdapter(SubprocessOptions{AllowBestEffortNoTools: true}) + for _, noNetwork := range []bool{false, true} { + req, scratch := offlineTestRequest(t, noNetwork) + started, err := adapter.buildArgsForSession(req, scratch, "") + if err != nil { + t.Fatalf("buildArgsForSession: %v", err) + } + resumed, err := adapter.buildArgsForSession(req, scratch, "session-1") + if err != nil { + t.Fatalf("buildArgsForSession(resume): %v", err) + } + for name, args := range map[string][]string{"started": started, "resumed": resumed} { + configs := flagValues(argsBeforePrompt(args), "-c") + for _, want := range []string{`web_search="disabled"`, "sandbox_workspace_write.network_access=false"} { + if slices.Contains(configs, want) != noNetwork { + t.Fatalf("%s -c values = %v, want %q present=%v", name, configs, want, noNetwork) + } + } + if err := adapter.validateArgs(args, scratch, req); err != nil { + t.Fatalf("%s validateArgs(noNetwork=%v): %v", name, noNetwork, err) + } + } + if noNetwork { + if err := adapter.validateArgs(removeFlagPair(started, "-c"), scratch, req); !errors.Is(err, ErrUnsafeSubprocessConfig) { + t.Fatalf("validateArgs without network config = %v, want unsafe config", err) + } + } + } +} diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index fe10507..9c5e73a 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -705,6 +705,7 @@ func execute(ctx context.Context, opts Options, req Request, mode executionMode) ChangedFiles: prepared.changedFiles, Artifacts: prepared.artifacts, HeadRefNamespace: opts.Provider.Capabilities().HeadRefNamespace, + Offline: req.WithoutDiscussion, }); err != nil { if errors.Is(err, workbench.ErrUnsafeFetchRef) || errors.Is(err, workbench.ErrInvalidRepositoryIdentity) { return Result{}, Failure(FailureTerminal, err) @@ -2145,7 +2146,7 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p } agentID := agent.ID taskID := reviewerTaskID(agent.ID) - request, cleanupWorkspace, err := workbench.PrepareReviewerRequest(ctx, workbenchDeps(opts), opts.Adapter, artifacts, pr.Head.SHA, agent.ID, selected.AllowedFiles, model, effort, prompt, logPath) + request, cleanupWorkspace, err := workbench.PrepareReviewerRequest(ctx, workbenchDeps(opts), opts.Adapter, artifacts, pr.Head.SHA, agent.ID, selected.AllowedFiles, model, effort, prompt, logPath, workbench.ReviewerOptions{Offline: req.WithoutDiscussion}) if err != nil { return reviewerExecution{}, err } @@ -2253,7 +2254,7 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p return repairSetupFailed(err) } // Its own identity: reusing agent.ID would reset the primary pass's workspace and scratch. - repairRequest, cleanupRepairWorkspace, err := workbench.PrepareReviewerRequest(ctx, workbenchDeps(opts), opts.Adapter, artifacts, pr.Head.SHA, repairIdentity, repairSelected.AllowedFiles, model, effort, repairPrompt, repairLogPath) + repairRequest, cleanupRepairWorkspace, err := workbench.PrepareReviewerRequest(ctx, workbenchDeps(opts), opts.Adapter, artifacts, pr.Head.SHA, repairIdentity, repairSelected.AllowedFiles, model, effort, repairPrompt, repairLogPath, workbench.ReviewerOptions{Offline: req.WithoutDiscussion}) if err != nil { return repairSetupFailed(err) } diff --git a/internal/pipeline/without_discussion_test.go b/internal/pipeline/without_discussion_test.go index 1c2685c..70ae1ae 100644 --- a/internal/pipeline/without_discussion_test.go +++ b/internal/pipeline/without_discussion_test.go @@ -194,6 +194,9 @@ func TestDryRunWithoutDiscussionReplaysAsFirstPass(t *testing.T) { } } } + if reviewer := adapter.Requests()[1]; reviewer.ReviewerWorkspace == nil || !reviewer.ReviewerWorkspace.NoNetwork { + t.Fatalf("reviewer workspace = %#v, want network tools denied", reviewer.ReviewerWorkspace) + } if !strings.Contains(prompts[0], "Replay title") || !strings.Contains(prompts[0], "Replay description stays visible.") { t.Fatalf("selection prompt lost PR title/description:\n%s", prompts[0]) } @@ -260,6 +263,11 @@ func TestDryRunPinnedWithDiscussionStillResumesPriorSessions(t *testing.T) { if !resumed[priorReviewerSession] || !resumed[priorOrchestratorSession] { t.Fatalf("resumes = %#v, want prior reviewer and orchestrator sessions", adapter.Resumes()) } + for _, resume := range adapter.Resumes() { + if workspace := resume.Request.ReviewerWorkspace; workspace != nil && workspace.NoNetwork { + t.Fatalf("reviewer workspace = %#v, want network tools left alone without the flag", workspace) + } + } if result.WithoutDiscussion { t.Fatal("result.WithoutDiscussion = true, want false") } diff --git a/internal/workbench/workbench.go b/internal/workbench/workbench.go index 6b654fa..57eeb65 100644 --- a/internal/workbench/workbench.go +++ b/internal/workbench/workbench.go @@ -59,6 +59,16 @@ type Request struct { // pull-request heads (gitprovider.ProviderCaps.HeadRefNamespace). Empty // means the GitHub "pull" namespace. HeadRefNamespace string + // Offline removes the checkout's remotes once the review commits are + // fetched, so nothing cloned from it points back at the git host. + Offline bool +} + +// ReviewerOptions adjusts one reviewer workspace. +type ReviewerOptions struct { + // Offline removes the workspace clone's remotes and asks the adapter to + // deny reviewer tools that reach the network. + Offline bool } type metadataArtifact struct { @@ -105,6 +115,16 @@ func Prepare(ctx context.Context, deps Deps, req Request) error { // Prepare creates or reuses a clean checkout pinned to the requested commits. func (p *RunPreparer) Prepare(ctx context.Context, req Request) error { + if err := p.prepare(ctx, req); err != nil { + return err + } + if req.Offline { + return removeRemotes(ctx, p.deps, req.Artifacts.WorkbenchRepoDir) + } + return nil +} + +func (p *RunPreparer) prepare(ctx context.Context, req Request) error { if reusable, err := p.reusable(ctx, req); err != nil { return err } else if reusable { @@ -236,6 +256,21 @@ func (p *RunPreparer) reusable(ctx context.Context, req Request) (bool, error) { return true, nil } +// removeRemotes deletes every configured remote. Fetches in Prepare pass the +// remote URL directly, so nothing after the fetch depends on a named remote. +func removeRemotes(ctx context.Context, deps Deps, repoDir string) error { + out, err := deps.gitCommand(ctx, repoDir, "remote") + if err != nil { + return fmt.Errorf("pipeline: list workbench remotes: %w", err) + } + for _, name := range strings.Fields(string(out)) { + if _, err := deps.gitCommand(ctx, repoDir, "remote", "remove", name); err != nil { + return fmt.Errorf("pipeline: remove workbench remote %q: %w", name, err) + } + } + return 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}") @@ -358,11 +393,11 @@ func verifyClean(ctx context.Context, deps Deps, repoDir string, headSHA string) } // PrepareReviewerRequest creates a disposable reviewer workspace and LLM request. -func PrepareReviewerRequest(ctx context.Context, deps Deps, adapter llm.Adapter, artifacts runartifact.Paths, headSHA string, agentID string, allowedFiles []string, model, effort, prompt, logPath string) (llm.Request, func() error, error) { +func PrepareReviewerRequest(ctx context.Context, deps Deps, adapter llm.Adapter, artifacts runartifact.Paths, headSHA string, agentID string, allowedFiles []string, model, effort, prompt, logPath string, options ...ReviewerOptions) (llm.Request, func() error, error) { if err := llm.RequireReviewerWorkspace(adapter); err != nil { return llm.Request{}, nil, fmt.Errorf("pipeline: %w", err) } - workspace, cleanup, err := prepareReviewerWorkspace(ctx, deps, artifacts, headSHA, agentID, allowedFiles, defaultReviewerWorkspaceToolOutputBytes) + workspace, cleanup, err := prepareReviewerWorkspace(ctx, deps, artifacts, headSHA, agentID, allowedFiles, defaultReviewerWorkspaceToolOutputBytes, options...) if err != nil { return llm.Request{}, nil, err } @@ -385,7 +420,7 @@ func PrepareReviewerRequest(ctx context.Context, deps Deps, adapter llm.Adapter, if err := cleanupCurrent(); err != nil { return fmt.Errorf("pipeline: cleanup reviewer workspace before retry: %w", err) } - retryWorkspace, retryCleanup, err := prepareReviewerWorkspace(ctx, deps, artifacts, headSHA, agentID, allowedFiles, defaultReviewerWorkspaceToolOutputBytes) + retryWorkspace, retryCleanup, err := prepareReviewerWorkspace(ctx, deps, artifacts, headSHA, agentID, allowedFiles, defaultReviewerWorkspaceToolOutputBytes, options...) if err != nil { return err } @@ -397,7 +432,11 @@ func PrepareReviewerRequest(ctx context.Context, deps Deps, adapter llm.Adapter, }, cleanupCurrent, nil } -func prepareReviewerWorkspace(ctx context.Context, deps Deps, artifacts runartifact.Paths, headSHA string, agentID string, allowedFiles []string, maxToolOutputBytes int) (llm.ReviewerWorkspaceRequest, func() error, error) { +func prepareReviewerWorkspace(ctx context.Context, deps Deps, artifacts runartifact.Paths, headSHA string, agentID string, allowedFiles []string, maxToolOutputBytes int, options ...ReviewerOptions) (llm.ReviewerWorkspaceRequest, func() error, error) { + offline := false + for _, option := range options { + offline = offline || option.Offline + } if strings.TrimSpace(artifacts.WorkbenchRepoDir) == "" { return llm.ReviewerWorkspaceRequest{}, nil, fmt.Errorf("pipeline: workbench repo dir is required for reviewer workspace") } @@ -442,6 +481,12 @@ func prepareReviewerWorkspace(ctx context.Context, deps Deps, artifacts runartif _ = cleanup() return llm.ReviewerWorkspaceRequest{}, nil, err } + if offline { + if err := removeRemotes(ctx, deps, workspaceRepo); err != nil { + _ = cleanup() + return llm.ReviewerWorkspaceRequest{}, nil, err + } + } if len(allowedFiles) > 0 { for _, path := range allowedFiles { clean := filepath.Clean(strings.TrimSpace(path)) @@ -457,6 +502,7 @@ func prepareReviewerWorkspace(ctx context.Context, deps Deps, artifacts runartif DiffPath: artifacts.DiffPatch, AllowedFiles: append([]string(nil), allowedFiles...), MaxToolOutputBytes: maxToolOutputBytes, + NoNetwork: offline, }, cleanup, nil } diff --git a/internal/workbench/workbench_test.go b/internal/workbench/workbench_test.go index 8de2c17..8ee913e 100644 --- a/internal/workbench/workbench_test.go +++ b/internal/workbench/workbench_test.go @@ -826,3 +826,44 @@ func gitCommandSucceeds(dir string, args ...string) bool { cmd.Dir = dir return cmd.Run() == nil } + +func TestOfflineWorkbenchAndReviewerWorkspaceHaveNoRemotes(t *testing.T) { + ctx := context.Background() + for _, offline := range []bool{false, true} { + 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, Offline: offline} + // The second call takes the reuse path, which must also leave no remotes. + for range 2 { + if err := Prepare(ctx, deps, req); err != nil { + t.Fatalf("Prepare(offline=%v): %v", offline, err) + } + remotes := strings.TrimSpace(gitCommandOutput(t, artifacts.WorkbenchRepoDir, "remote")) + if offline && remotes != "" { + t.Fatalf("offline workbench remotes = %q, want none", remotes) + } + if !offline && remotes != "origin" { + t.Fatalf("workbench remotes = %q, want origin", remotes) + } + } + + workspace, cleanup, err := prepareReviewerWorkspace(ctx, deps, artifacts, fixture.headSHA, "harness:offline", []string{"main.go"}, 1024, ReviewerOptions{Offline: offline}) + if err != nil { + t.Fatalf("prepareReviewerWorkspace(offline=%v): %v", offline, err) + } + t.Cleanup(cleanupForTest(t, cleanup)) + remotes := strings.TrimSpace(gitCommandOutput(t, workspace.RepoDir, "remote")) + if offline && (remotes != "" || !workspace.NoNetwork) { + t.Fatalf("offline reviewer workspace remotes = %q, NoNetwork = %v; want none and true", remotes, workspace.NoNetwork) + } + if !offline && (remotes != "origin" || workspace.NoNetwork) { + t.Fatalf("reviewer workspace remotes = %q, NoNetwork = %v; want origin and false", remotes, workspace.NoNetwork) + } + if got := strings.TrimSpace(gitCommandOutput(t, workspace.RepoDir, "rev-parse", "HEAD")); got != fixture.headSHA { + t.Fatalf("reviewer workspace HEAD = %q, want %q", got, fixture.headSHA) + } + } +}