From 5014d78fab4329b97b80348d2d8e9f02f8d76fce Mon Sep 17 00:00:00 2001 From: nfebe Date: Tue, 6 Oct 2026 10:04:32 +0100 Subject: [PATCH 1/3] feat(review): Add fixed revision and remote reviews --- internal/agent/reviews.go | 2 + internal/command/review.go | 71 ++++- internal/command/review_snapshot.go | 281 ++++++++++++++++++ internal/command/review_snapshot_test.go | 140 +++++++++ internal/command/review_test.go | 23 ++ .../testdata/review-snapshot-response.json | 161 ++++++++++ 6 files changed, 671 insertions(+), 7 deletions(-) create mode 100644 internal/command/review_snapshot.go create mode 100644 internal/command/review_snapshot_test.go create mode 100644 internal/command/testdata/review-snapshot-response.json diff --git a/internal/agent/reviews.go b/internal/agent/reviews.go index 24b940a..7e712c8 100644 --- a/internal/agent/reviews.go +++ b/internal/agent/reviews.go @@ -14,6 +14,7 @@ import ( type Ask struct { Repository string `json:"repository"` Against string `json:"against"` + Head string `json:"head,omitempty"` Title string `json:"title"` Description string `json:"description"` Skills []string `json:"skills"` @@ -106,6 +107,7 @@ type Read struct { Summary Summary `json:"summary"` Suggestions []Suggestion `json:"suggestions"` Notes map[string]string `json:"notes"` + Execution json.RawMessage `json:"execution,omitempty"` } // Review is whether a checkout's work is ready to be proposed to anyone. diff --git a/internal/command/review.go b/internal/command/review.go index a40cb7e..5b78ecb 100644 --- a/internal/command/review.go +++ b/internal/command/review.go @@ -23,11 +23,17 @@ var ( func reviewCommand(opts *options) *cobra.Command { var ( - against string - title string - skills []string - noWait bool - noModel bool + against string + title string + skills []string + noWait bool + noModel bool + repoPath string + base string + head string + descriptionFile string + remote snapshotOptions + format string ) command := &cobra.Command{ Use: "review [path]", @@ -36,10 +42,49 @@ func reviewCommand(opts *options) *cobra.Command { "it is committed or not, and say whether it is ready to propose.", Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + if format != "" && format != "json" && format != "text" { + return fmt.Errorf("format must be json or text") + } + if format != "" { + opts.asJSON = format == "json" + } folder := "." if len(args) == 1 { folder = args[0] } + if repoPath != "" { + if len(args) != 0 { + return fmt.Errorf("use either --repo or a positional path") + } + folder = repoPath + } + if remote.endpoint != "" { + if against != "" || noWait || noModel || len(skills) != 0 || descriptionFile != "" { + return fmt.Errorf("remote snapshots require committed input and --pr-metadata for context") + } + remote.base, remote.head, remote.title = base, head, title + return remoteReview(cmd, opts, folder, remote) + } + if remote.repository != "" || remote.diffFile != "" || remote.metadataFile != "" || len(remote.configuration) != 0 { + return fmt.Errorf("snapshot options require --reviewer or SOURCEANT_REVIEW_URL") + } + if base != "" { + if against != "" { + return fmt.Errorf("use either --base or --against") + } + against = base + } + if head != "" && base == "" { + return fmt.Errorf("--head requires --base") + } + description := "" + if descriptionFile != "" { + content, err := os.ReadFile(descriptionFile) + if err != nil { + return fmt.Errorf("the description file could not be read") + } + description = string(content) + } folder, err := filepath.Abs(folder) if err != nil { return err @@ -50,8 +95,10 @@ func reviewCommand(opts *options) *cobra.Command { return err } started, err := client.Review(cmd.Context(), agent.Ask{ - Repository: repository, - Against: against, + Repository: repository, + Against: against, + Head: head, + Description: description, // Named, so a list of reviews says where each came from. Title: or(title, "From the terminal"), Skills: skills, @@ -99,6 +146,16 @@ func reviewCommand(opts *options) *cobra.Command { command.Flags().StringArrayVar(&skills, "skill", nil, "Read it against this skill, repeatable") command.Flags().BoolVar(&noWait, "no-wait", false, "Print the link and leave it running") command.Flags().BoolVar(&noModel, "no-model", false, "Say what changed without judging it") + command.Flags().StringVar(&repoPath, "repo", "", "The checkout to review") + command.Flags().StringVar(&base, "base", "", "Compare against this commit") + command.Flags().StringVar(&head, "head", "", "Require a clean checkout at this full commit SHA") + command.Flags().StringVar(&descriptionFile, "description-file", "", "Read the change description from this file") + command.Flags().StringVar(&remote.endpoint, "reviewer", os.Getenv("SOURCEANT_REVIEW_URL"), "The remote snapshot review API URL") + command.Flags().StringVar(&remote.repository, "repository", "", "Repository identity as owner/name") + command.Flags().StringVar(&remote.diffFile, "diff-file", "", "Use a patch matching the committed comparison") + command.Flags().StringVar(&remote.metadataFile, "pr-metadata", "", "Read title and body from pull request JSON") + command.Flags().StringArrayVar(&remote.configuration, "review-option", nil, "Remote review option as label=value, repeatable") + command.Flags().StringVar(&format, "format", "", "Output format: json or text") return command } diff --git a/internal/command/review_snapshot.go b/internal/command/review_snapshot.go new file mode 100644 index 0000000..0fc6d6f --- /dev/null +++ b/internal/command/review_snapshot.go @@ -0,0 +1,281 @@ +package command + +import ( + "bufio" + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net" + "net/http" + "net/url" + "os" + "os/exec" + "path" + "regexp" + "strconv" + "strings" + "time" + "unicode/utf8" + + "github.com/sourceant/cli/internal/agent" + "github.com/spf13/cobra" +) + +const snapshotLimit = 64 * 1024 * 1024 + +type snapshotOptions struct { + endpoint, repository, base, head, diffFile, metadataFile, title string + configuration []string +} + +type reviewSnapshot struct { + Repository string `json:"repository"` + Base string `json:"base"` + Head string `json:"head"` + Diff string `json:"diff"` + Files map[string]string `json:"files"` + Omitted []string `json:"omitted"` + Title string `json:"title"` + Description string `json:"description"` + Configuration map[string]string `json:"configuration"` +} + +type snapshotIdentity struct { + Repository string `json:"repository"` + Base string `json:"base"` + Head string `json:"head"` + Omitted []string `json:"omitted"` +} + +type snapshotResult struct { + Status string `json:"status"` + Review agent.Review `json:"review"` + Snapshot snapshotIdentity `json:"snapshot"` + Configuration map[string]string `json:"configuration"` +} + +func remoteReview(cmd *cobra.Command, opts *options, folder string, settings snapshotOptions) error { + endpoint, err := url.Parse(settings.endpoint) + if err != nil || endpoint.Host == "" || endpoint.User != nil || endpoint.RawQuery != "" || endpoint.Fragment != "" { + return fmt.Errorf("reviewer must be an API URL without credentials, query or fragment") + } + loopback := endpoint.Hostname() == "localhost" + if ip := net.ParseIP(endpoint.Hostname()); ip != nil { + loopback = ip.IsLoopback() + } + if endpoint.Scheme != "https" && !(endpoint.Scheme == "http" && loopback) { + return fmt.Errorf("the reviewer API requires HTTPS") + } + token := os.Getenv("SOURCEANT_REVIEW_TOKEN") + if token == "" { + return fmt.Errorf("SOURCEANT_REVIEW_TOKEN is required for a remote review") + } + limit := 14 * time.Minute + if cmd.Flags().Changed("timeout") { + limit = opts.timeout + } + ctx, cancel := context.WithTimeout(cmd.Context(), limit) + defer cancel() + snapshot, err := checkoutSnapshot(ctx, folder, settings) + if err != nil { + return err + } + body, err := json.Marshal(snapshot) + if err != nil { + return fmt.Errorf("the snapshot could not be encoded") + } + if len(body) > snapshotLimit { + return fmt.Errorf("the review snapshot exceeds 64 MiB") + } + request, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint.String(), bytes.NewReader(body)) + if err != nil { + return fmt.Errorf("the review request could not be created") + } + request.Header.Set("Authorization", "Bearer "+token) + request.Header.Set("Content-Type", "application/json") + request.Header.Set("Accept", "application/json") + if workspace := os.Getenv("SOURCEANT_REVIEW_WORKSPACE"); workspace != "" { + request.Header.Set("X-SourceAnt-Workspace", workspace) + } + client := &http.Client{CheckRedirect: func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse }} + response, err := client.Do(request) + if err != nil { + return fmt.Errorf("the remote review request did not complete") + } + defer response.Body.Close() + if response.StatusCode < 200 || response.StatusCode >= 300 { + return fmt.Errorf("the reviewer API refused the request (HTTP %d)", response.StatusCode) + } + encoded, err := io.ReadAll(io.LimitReader(response.Body, snapshotLimit+1)) + if err != nil || len(encoded) > snapshotLimit { + return fmt.Errorf("the reviewer API returned an unreadable response") + } + var envelope struct { + Status string `json:"status"` + Data snapshotResult `json:"data"` + } + if json.Unmarshal(encoded, &envelope) != nil || envelope.Status != "success" || envelope.Data.Status != agent.Done { + return fmt.Errorf("the reviewer API did not return a completed review") + } + result := envelope.Data + if result.Snapshot.Repository != snapshot.Repository || result.Snapshot.Base != snapshot.Base || result.Snapshot.Head != snapshot.Head || result.Review.Base != snapshot.Base { + return fmt.Errorf("the reviewer API returned a different snapshot") + } + if opts.asJSON { + if err := writeJSON(cmd.OutOrStdout(), result); err != nil { + return err + } + } else { + report(cmd.OutOrStdout(), agent.Reading{Status: result.Status, Review: result.Review}) + } + if !result.Review.Ready { + return &unready{} + } + return nil +} + +func checkoutGit(ctx context.Context, folder string, args ...string) ([]byte, error) { + command := exec.CommandContext(ctx, "git", append([]string{"-C", folder}, args...)...) + command.Env = append(os.Environ(), "GIT_NO_REPLACE_OBJECTS=1", "GIT_OPTIONAL_LOCKS=0") + output, err := command.Output() + if err != nil { + return nil, fmt.Errorf("the checkout could not supply the requested commit comparison") + } + return output, nil +} + +func checkoutSnapshot(ctx context.Context, folder string, settings snapshotOptions) (reviewSnapshot, error) { + snapshot := reviewSnapshot{Repository: settings.repository, Base: settings.base, Head: settings.head, Files: map[string]string{}, Omitted: []string{}, Configuration: map[string]string{}, Title: settings.title} + sha := regexp.MustCompile(`^[a-f0-9]{40}$`) + if !sha.MatchString(settings.base) || !sha.MatchString(settings.head) || !regexp.MustCompile(`^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$`).MatchString(settings.repository) { + return snapshot, fmt.Errorf("remote review requires --repository owner/name and full --base and --head commit SHAs") + } + for _, option := range settings.configuration { + label, value, found := strings.Cut(option, "=") + if !found || label == "" || value == "" { + return snapshot, fmt.Errorf("review options must be label=value") + } + if _, duplicate := snapshot.Configuration[label]; duplicate { + return snapshot, fmt.Errorf("a review option was specified more than once") + } + snapshot.Configuration[label] = value + } + head, err := checkoutGit(ctx, folder, "rev-parse", "HEAD") + if err != nil { + return snapshot, err + } + if strings.TrimSpace(string(head)) != settings.head { + return snapshot, fmt.Errorf("the checkout HEAD does not match --head") + } + status, err := checkoutGit(ctx, folder, "status", "--porcelain", "--untracked-files=no") + if err != nil { + return snapshot, err + } + if len(status) != 0 { + return snapshot, fmt.Errorf("commit tracked changes before submitting a remote snapshot") + } + diff, err := checkoutGit(ctx, folder, "diff", "--no-ext-diff", "--no-textconv", settings.base+"..."+settings.head) + if err != nil { + return snapshot, err + } + if len(diff) == 0 || len(diff) > 8*1024*1024 || !utf8.Valid(diff) { + return snapshot, fmt.Errorf("the snapshot requires a nonempty UTF-8 diff of at most 8 MiB") + } + if settings.diffFile != "" { + mounted, err := os.ReadFile(settings.diffFile) + if err != nil || !bytes.Equal(mounted, diff) { + return snapshot, fmt.Errorf("the supplied patch does not match the committed comparison") + } + } + snapshot.Diff = string(diff) + if settings.metadataFile != "" { + encoded, err := os.ReadFile(settings.metadataFile) + if err != nil { + return snapshot, fmt.Errorf("pull request metadata could not be read") + } + var metadata struct{ NWO, Base, Head, Title, Body string } + if json.Unmarshal(encoded, &metadata) != nil || metadata.NWO != settings.repository || metadata.Base != settings.base || metadata.Head != settings.head { + return snapshot, fmt.Errorf("pull request metadata does not match the snapshot") + } + if snapshot.Title == "" { + snapshot.Title = metadata.Title + } + snapshot.Description = metadata.Body + } + tree, err := checkoutGit(ctx, folder, "ls-tree", "-r", "-z", settings.head) + if err != nil { + return snapshot, err + } + batchContext, stopBatch := context.WithCancel(ctx) + defer stopBatch() + batch := exec.CommandContext(batchContext, "git", "-C", folder, "cat-file", "--batch") + batch.Env = append(os.Environ(), "GIT_NO_REPLACE_OBJECTS=1", "GIT_OPTIONAL_LOCKS=0") + input, err := batch.StdinPipe() + if err != nil { + return snapshot, err + } + output, err := batch.StdoutPipe() + if err != nil { + return snapshot, err + } + if err := batch.Start(); err != nil { + return snapshot, fmt.Errorf("the snapshot objects could not be read") + } + defer func() { _ = input.Close(); stopBatch(); _ = batch.Wait() }() + reader := bufio.NewReader(output) + total := len(diff) + entries := bytes.Split(tree, []byte{0}) + if len(entries) > 50001 { + return snapshot, fmt.Errorf("the snapshot exceeds 50000 files") + } + for _, entry := range entries { + if len(entry) == 0 { + continue + } + info, file, ok := strings.Cut(string(entry), "\t") + fields := strings.Fields(info) + if !ok || len(fields) != 3 || !utf8.ValidString(file) || path.Clean(file) != file || strings.HasPrefix(file, "/") || strings.Contains(file, "\\") || strings.Contains("/"+file+"/", "/.git/") { + return snapshot, fmt.Errorf("the snapshot contains an unsupported file path") + } + if fields[0] != "100644" && fields[0] != "100755" { + snapshot.Omitted = append(snapshot.Omitted, file) + continue + } + if _, err := io.WriteString(input, fields[2]+"\n"); err != nil { + return snapshot, fmt.Errorf("a snapshot object could not be read") + } + header, err := reader.ReadString('\n') + parts := strings.Fields(header) + if err != nil || len(parts) != 3 || parts[1] != "blob" || parts[0] != fields[2] { + return snapshot, fmt.Errorf("a snapshot object could not be read") + } + size, err := strconv.ParseInt(parts[2], 10, 64) + if err != nil || size < 0 { + return snapshot, fmt.Errorf("a snapshot object has an invalid size") + } + if size > 1_000_000 { + if _, err := io.CopyN(io.Discard, reader, size+1); err != nil { + return snapshot, fmt.Errorf("a snapshot object could not be read") + } + snapshot.Omitted = append(snapshot.Omitted, file) + continue + } + content := make([]byte, int(size)+1) + if _, err := io.ReadFull(reader, content); err != nil || content[len(content)-1] != '\n' { + return snapshot, fmt.Errorf("a snapshot object could not be read") + } + content = content[:size] + if bytes.ContainsRune(content, 0) || !utf8.Valid(content) { + snapshot.Omitted = append(snapshot.Omitted, file) + continue + } + total += len(content) + if total > snapshotLimit { + return snapshot, fmt.Errorf("the review snapshot exceeds 64 MiB") + } + snapshot.Files[file] = string(content) + } + return snapshot, nil +} diff --git a/internal/command/review_snapshot_test.go b/internal/command/review_snapshot_test.go new file mode 100644 index 0000000..41369fe --- /dev/null +++ b/internal/command/review_snapshot_test.go @@ -0,0 +1,140 @@ +package command + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func snapshotCheckout(t *testing.T) (string, string, string) { + t.Helper() + folder := t.TempDir() + git := func(args ...string) string { + t.Helper() + command := exec.Command("git", append([]string{"-C", folder}, args...)...) + encoded, err := command.CombinedOutput() + if err != nil { + t.Fatalf("git failed: %s", encoded) + } + return strings.TrimSpace(string(encoded)) + } + git("init", "-q") + git("config", "user.email", "review@example.com") + git("config", "user.name", "Reviewer") + if err := os.WriteFile(filepath.Join(folder, "changed.go"), []byte("package example\nvar Value = 1\n"), 0600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(folder, "context.go"), []byte("package example\nvar Context = Value\n"), 0600); err != nil { + t.Fatal(err) + } + git("add", "-A") + git("commit", "-q", "-m", "feat: Add the example") + base := git("rev-parse", "HEAD") + if err := os.WriteFile(filepath.Join(folder, "changed.go"), []byte("package example\nvar Value = 2\n"), 0600); err != nil { + t.Fatal(err) + } + git("add", "-A") + git("commit", "-q", "-m", "feat: Change the example") + head := git("rev-parse", "HEAD") + if err := os.WriteFile(filepath.Join(folder, "untracked.env"), []byte("PRIVATE=local\n"), 0600); err != nil { + t.Fatal(err) + } + return folder, base, head +} + +func TestRemoteReviewUploadsCommittedContextAndUsesWorkspaceCredentials(t *testing.T) { + folder, base, head := snapshotCheckout(t) + t.Setenv("SOURCEANT_REVIEW_TOKEN", "your-api-key-here") + t.Setenv("SOURCEANT_REVIEW_WORKSPACE", "benchmark") + var captured reviewSnapshot + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.Path != "/api/reviews/snapshots" { + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + } + if r.Header.Get("Authorization") != "Bearer your-api-key-here" || r.Header.Get("X-SourceAnt-Workspace") != "benchmark" { + t.Error("workspace credentials did not reach the API") + } + if err := json.NewDecoder(r.Body).Decode(&captured); err != nil { + t.Error(err) + } + encoded, err := os.ReadFile(filepath.Join("testdata", "review-snapshot-response.json")) + if err != nil { + t.Fatal(err) + } + var response struct { + Status string `json:"status"` + Data snapshotResult `json:"data"` + } + if err := json.Unmarshal(encoded, &response); err != nil { + t.Fatal(err) + } + response.Data.Review.Base = base + response.Data.Snapshot = snapshotIdentity{Repository: "acme/example", Base: base, Head: head} + _ = json.NewEncoder(w).Encode(response) + })) + defer server.Close() + var stdout, stderr bytes.Buffer + code := Run([]string{"review", "--repo", folder, "--base", base, "--head", head, "--repository", "acme/example", "--reviewer", server.URL + "/api/reviews/snapshots", "--format", "json", "--review-option", "discovery-passes=3"}, &stdout, &stderr) + if code != 2 { + t.Fatalf("exited %d: %s", code, stderr.String()) + } + if captured.Files["context.go"] != "package example\nvar Context = Value\n" { + t.Error("unchanged context was not uploaded") + } + if captured.Files["changed.go"] != "package example\nvar Value = 2\n" || !strings.Contains(captured.Diff, "+var Value = 2") { + t.Error("head source or committed diff was lost") + } + if _, found := captured.Files["untracked.env"]; found { + t.Error("an untracked file was uploaded") + } + for file := range captured.Files { + if strings.HasPrefix(file, ".git/") { + t.Error("Git metadata was uploaded") + } + } + if captured.Configuration["discovery-passes"] != "3" { + t.Error("review settings were lost") + } + if !strings.Contains(stdout.String(), "review-0") { + t.Error("execution metadata from the API was lost") + } + if !json.Valid(stdout.Bytes()) { + t.Fatalf("invalid JSON: %s", stdout.String()) + } +} + +func TestRemoteReviewRefusesWrongHeadBeforeUploading(t *testing.T) { + folder, base, _ := snapshotCheckout(t) + t.Setenv("SOURCEANT_REVIEW_TOKEN", "your-api-key-here") + called := false + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { called = true })) + defer server.Close() + var stdout, stderr bytes.Buffer + code := Run([]string{"review", "--repo", folder, "--base", base, "--head", strings.Repeat("a", 40), "--repository", "acme/example", "--reviewer", server.URL}, &stdout, &stderr) + if code != 1 || called || stdout.Len() != 0 { + t.Fatalf("invalid input was uploaded: code=%d called=%v", code, called) + } +} + +func TestRemoteReviewDoesNotFollowRedirects(t *testing.T) { + folder, base, head := snapshotCheckout(t) + t.Setenv("SOURCEANT_REVIEW_TOKEN", "your-api-key-here") + forwarded := false + destination := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { forwarded = true })) + defer destination.Close() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, destination.URL, http.StatusTemporaryRedirect) + })) + defer server.Close() + var stdout, stderr bytes.Buffer + code := Run([]string{"review", "--repo", folder, "--base", base, "--head", head, "--repository", "acme/example", "--reviewer", server.URL}, &stdout, &stderr) + if code != 1 || forwarded || stdout.Len() != 0 { + t.Fatalf("the request followed a redirect: code=%d forwarded=%v", code, forwarded) + } +} diff --git a/internal/command/review_test.go b/internal/command/review_test.go index a374888..a8ea750 100644 --- a/internal/command/review_test.go +++ b/internal/command/review_test.go @@ -77,6 +77,29 @@ func TestReviewPrintsTheLinkAndWhatWasMadeOfTheChange(t *testing.T) { } } +func TestCommittedReviewFlagsReachTheAPI(t *testing.T) { + folder := t.TempDir() + run, _, sent := reviewing(t, map[string][]answer{ + "/api/repositories": {{body: indexing(t, folder)}}, + "/api/reviews": {{status: http.StatusAccepted, body: fixture(t, "review-started.json")}}, + "/api/reviews/": {{body: fixture(t, "review-done.json")}}, + }) + stdout, stderr, code := run("review", "--repo", folder, "--base", strings.Repeat("a", 40), "--head", strings.Repeat("b", 40), "--format", "json") + if code != 0 { + t.Fatalf("exited %d: %s", code, stderr) + } + var request map[string]any + if err := json.Unmarshal(*sent, &request); err != nil { + t.Fatal(err) + } + if request["against"] != strings.Repeat("a", 40) || request["head"] != strings.Repeat("b", 40) { + t.Fatalf("commit comparison was lost: %s", *sent) + } + if !json.Valid([]byte(stdout)) { + t.Fatalf("invalid JSON output: %s", stdout) + } +} + func TestABlockingFindingIsPrintedAndExitsNonZero(t *testing.T) { folder := t.TempDir() run, _, _ := reviewing(t, map[string][]answer{ diff --git a/internal/command/testdata/review-snapshot-response.json b/internal/command/testdata/review-snapshot-response.json new file mode 100644 index 0000000..09df4ef --- /dev/null +++ b/internal/command/testdata/review-snapshot-response.json @@ -0,0 +1,161 @@ +{ + "status": "success", + "message": "Request was successful", + "data": { + "status": "done", + "configuration": { + "discovery-passes": "3", + "evaluation-passes": "2", + "concurrency": "6", + "minimum-support": "1", + "maximum-rejections": "0", + "reading-budget": "15000", + "include-nitpicks": "False" + }, + "review": { + "base": "3712b5d70f9c924d9a088268027aa15d8bff769d", + "ready": false, + "review": { + "execution": { + "reviews": [ + { + "participant": "review-0", + "repetition": 1, + "error": "" + }, + { + "participant": "review-0", + "repetition": 2, + "error": "" + }, + { + "participant": "review-0", + "repetition": 3, + "error": "" + } + ], + "evaluations": [ + { + "participant": "deterministic", + "repetition": 1, + "error": "", + "judgments": [] + }, + { + "participant": "evaluation-0", + "repetition": 1, + "error": "", + "judgments": [ + { + "candidate": 0, + "status": "supported", + "reason": "The migration changes an existing applied operation.", + "evidence": [ + "db/0001_charges.py: return 1" + ] + } + ] + }, + { + "participant": "evaluation-0", + "repetition": 2, + "error": "", + "judgments": [ + { + "candidate": 0, + "status": "supported", + "reason": "The migration changes an existing applied operation.", + "evidence": [ + "db/0001_charges.py: return 1" + ] + } + ] + } + ], + "analysis_coverage": [ + { + "tool": "semgrep", + "paths": [], + "unsupported": [], + "unavailable": [ + "db/0001_charges.py" + ], + "languages": [], + "checks": [ + "static-analysis" + ], + "skipped": [] + } + ], + "analysis_findings": [], + "file_languages": { + "db/0001_charges.py": "python" + }, + "unchecked": [ + "db/0001_charges.py" + ], + "producers": [ + [ + "review-0:1", + "review-0:2", + "review-0:3" + ] + ], + "candidates": [ + { + "file_name": "db/0001_charges.py", + "position": 3, + "start_line": 2, + "end_line": 2, + "side": "RIGHT", + "comment": "Add a new migration instead.", + "category": "BUG", + "trigger": null, + "blast": null, + "impact": null, + "certainty": null, + "comment_only": false, + "suggested_code": " pass\n", + "existing_code": " return 1\n", + "claims": [] + } + ] + }, + "verdict": "REQUEST_CHANGES", + "summary": { + "overview": "Review found 1 actionable issue(s).", + "key_improvements": [], + "minor_suggestions": [ + "Review coverage is incomplete. Inspect the execution details before approving." + ], + "critical_issues": [ + "Add a new migration instead." + ] + }, + "suggestions": [ + { + "path": "db/0001_charges.py", + "start_line": 2, + "end_line": 2, + "side": "RIGHT", + "comment": "Add a new migration instead.", + "category": "BUG", + "existing_code": " return 1\n", + "suggested_code": " pass\n" + } + ], + "notes": {} + }, + "where": { + "base": "3712b5d70f9c924d9a088268027aa15d8bff769d", + "branch": "11db3957bcf923e86fe4d98857e86a1182ecf03d" + } + }, + "snapshot": { + "repository": "acme/billing", + "base": "3712b5d70f9c924d9a088268027aa15d8bff769d", + "head": "11db3957bcf923e86fe4d98857e86a1182ecf03d", + "omitted": [] + } + } +} From 713dfcd11b5f3a1e7b84dfe2af892b13a648c415 Mon Sep 17 00:00:00 2001 From: nfebe Date: Tue, 6 Oct 2026 20:19:25 +0100 Subject: [PATCH 2/3] refactor(cli): Simplify review flags and reviewer host selection --- README.md | 20 ++++++++++++++++++++ internal/command/review.go | 18 +++++++++--------- internal/command/review_snapshot.go | 6 ++++-- internal/command/review_snapshot_test.go | 20 +++++++++++++++++--- internal/command/review_test.go | 2 +- 5 files changed, 51 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index ca9b120..7b884f6 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,26 @@ Any command that needs the agent starts one, so nothing has to be started by han `--json` prints the agent's own answer, for anything that wants to read it rather than look at it. +For a remote review, `--dir` (`-d`) selects the local checkout and +`--repository` (`-r`) supplies its `owner/name` identity. `--host` (`-H`) +selects the reviewer server; the CLI supplies the API path. Set +`SOURCEANT_REVIEW_TOKEN` for authentication and optionally +`SOURCEANT_REVIEW_HOST` as the default server. + +```bash +sourceant review \ + --dir /workspace/repo \ + --repository acme/example \ + --base "$(git -C /workspace/repo rev-parse HEAD~1)" \ + --head "$(git -C /workspace/repo rev-parse HEAD)" \ + --host https://review.example.com \ + --option discovery-passes=3 \ + --option evaluation-passes=2 \ + --format json +``` + +`--option` (`-o`) can be repeated for different reviewer settings. + ## Building ```bash diff --git a/internal/command/review.go b/internal/command/review.go index 5b78ecb..4d88148 100644 --- a/internal/command/review.go +++ b/internal/command/review.go @@ -28,7 +28,7 @@ func reviewCommand(opts *options) *cobra.Command { skills []string noWait bool noModel bool - repoPath string + folderPath string base string head string descriptionFile string @@ -52,11 +52,11 @@ func reviewCommand(opts *options) *cobra.Command { if len(args) == 1 { folder = args[0] } - if repoPath != "" { + if folderPath != "" { if len(args) != 0 { - return fmt.Errorf("use either --repo or a positional path") + return fmt.Errorf("use either --dir or a positional path") } - folder = repoPath + folder = folderPath } if remote.endpoint != "" { if against != "" || noWait || noModel || len(skills) != 0 || descriptionFile != "" { @@ -66,7 +66,7 @@ func reviewCommand(opts *options) *cobra.Command { return remoteReview(cmd, opts, folder, remote) } if remote.repository != "" || remote.diffFile != "" || remote.metadataFile != "" || len(remote.configuration) != 0 { - return fmt.Errorf("snapshot options require --reviewer or SOURCEANT_REVIEW_URL") + return fmt.Errorf("snapshot options require --host or SOURCEANT_REVIEW_HOST") } if base != "" { if against != "" { @@ -146,15 +146,15 @@ func reviewCommand(opts *options) *cobra.Command { command.Flags().StringArrayVar(&skills, "skill", nil, "Read it against this skill, repeatable") command.Flags().BoolVar(&noWait, "no-wait", false, "Print the link and leave it running") command.Flags().BoolVar(&noModel, "no-model", false, "Say what changed without judging it") - command.Flags().StringVar(&repoPath, "repo", "", "The checkout to review") + command.Flags().StringVarP(&folderPath, "dir", "d", "", "The local checkout directory to review") command.Flags().StringVar(&base, "base", "", "Compare against this commit") command.Flags().StringVar(&head, "head", "", "Require a clean checkout at this full commit SHA") command.Flags().StringVar(&descriptionFile, "description-file", "", "Read the change description from this file") - command.Flags().StringVar(&remote.endpoint, "reviewer", os.Getenv("SOURCEANT_REVIEW_URL"), "The remote snapshot review API URL") - command.Flags().StringVar(&remote.repository, "repository", "", "Repository identity as owner/name") + command.Flags().StringVarP(&remote.endpoint, "host", "H", os.Getenv("SOURCEANT_REVIEW_HOST"), "The reviewer server URL, including scheme and optional port") + command.Flags().StringVarP(&remote.repository, "repository", "r", "", "Repository identity as owner/name") command.Flags().StringVar(&remote.diffFile, "diff-file", "", "Use a patch matching the committed comparison") command.Flags().StringVar(&remote.metadataFile, "pr-metadata", "", "Read title and body from pull request JSON") - command.Flags().StringArrayVar(&remote.configuration, "review-option", nil, "Remote review option as label=value, repeatable") + command.Flags().StringArrayVarP(&remote.configuration, "option", "o", nil, "Remote review option as label=value, repeatable") command.Flags().StringVar(&format, "format", "", "Output format: json or text") return command } diff --git a/internal/command/review_snapshot.go b/internal/command/review_snapshot.go index 0fc6d6f..4c4d9ac 100644 --- a/internal/command/review_snapshot.go +++ b/internal/command/review_snapshot.go @@ -58,8 +58,8 @@ type snapshotResult struct { func remoteReview(cmd *cobra.Command, opts *options, folder string, settings snapshotOptions) error { endpoint, err := url.Parse(settings.endpoint) - if err != nil || endpoint.Host == "" || endpoint.User != nil || endpoint.RawQuery != "" || endpoint.Fragment != "" { - return fmt.Errorf("reviewer must be an API URL without credentials, query or fragment") + if err != nil || endpoint.Host == "" || endpoint.User != nil || endpoint.RawQuery != "" || endpoint.Fragment != "" || (endpoint.Path != "" && endpoint.Path != "/") { + return fmt.Errorf("--host must be a server URL without credentials, an API path, query or fragment") } loopback := endpoint.Hostname() == "localhost" if ip := net.ParseIP(endpoint.Hostname()); ip != nil { @@ -68,6 +68,8 @@ func remoteReview(cmd *cobra.Command, opts *options, folder string, settings sna if endpoint.Scheme != "https" && !(endpoint.Scheme == "http" && loopback) { return fmt.Errorf("the reviewer API requires HTTPS") } + endpoint.Path = "/api/reviews/snapshots" + token := os.Getenv("SOURCEANT_REVIEW_TOKEN") if token == "" { return fmt.Errorf("SOURCEANT_REVIEW_TOKEN is required for a remote review") diff --git a/internal/command/review_snapshot_test.go b/internal/command/review_snapshot_test.go index 41369fe..6e239a5 100644 --- a/internal/command/review_snapshot_test.go +++ b/internal/command/review_snapshot_test.go @@ -80,7 +80,13 @@ func TestRemoteReviewUploadsCommittedContextAndUsesWorkspaceCredentials(t *testi })) defer server.Close() var stdout, stderr bytes.Buffer - code := Run([]string{"review", "--repo", folder, "--base", base, "--head", head, "--repository", "acme/example", "--reviewer", server.URL + "/api/reviews/snapshots", "--format", "json", "--review-option", "discovery-passes=3"}, &stdout, &stderr) + code := Run([]string{"review", "--dir", folder, "--base", base, "--head", head, "--repository", "acme/example", "--host", server.URL, "--format", "json", "--option", "discovery-passes=3"}, &stdout, &stderr) + if code != 2 { + t.Fatalf("exited %d: %s", code, stderr.String()) + } + stdout.Reset() + stderr.Reset() + code = Run([]string{"review", "-d", folder, "--base", base, "--head", head, "-r", "acme/example", "-H", server.URL + "/", "--format", "json", "-o", "discovery-passes=3"}, &stdout, &stderr) if code != 2 { t.Fatalf("exited %d: %s", code, stderr.String()) } @@ -116,7 +122,7 @@ func TestRemoteReviewRefusesWrongHeadBeforeUploading(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { called = true })) defer server.Close() var stdout, stderr bytes.Buffer - code := Run([]string{"review", "--repo", folder, "--base", base, "--head", strings.Repeat("a", 40), "--repository", "acme/example", "--reviewer", server.URL}, &stdout, &stderr) + code := Run([]string{"review", "--dir", folder, "--base", base, "--head", strings.Repeat("a", 40), "--repository", "acme/example", "--host", server.URL}, &stdout, &stderr) if code != 1 || called || stdout.Len() != 0 { t.Fatalf("invalid input was uploaded: code=%d called=%v", code, called) } @@ -133,8 +139,16 @@ func TestRemoteReviewDoesNotFollowRedirects(t *testing.T) { })) defer server.Close() var stdout, stderr bytes.Buffer - code := Run([]string{"review", "--repo", folder, "--base", base, "--head", head, "--repository", "acme/example", "--reviewer", server.URL}, &stdout, &stderr) + code := Run([]string{"review", "--dir", folder, "--base", base, "--head", head, "--repository", "acme/example", "--host", server.URL}, &stdout, &stderr) if code != 1 || forwarded || stdout.Len() != 0 { t.Fatalf("the request followed a redirect: code=%d forwarded=%v", code, forwarded) } } + +func TestRemoteReviewHostRejectsAnEndpointPath(t *testing.T) { + var stdout, stderr bytes.Buffer + code := Run([]string{"review", "--host", "https://review.example.com/api/reviews/snapshots"}, &stdout, &stderr) + if code == 0 || !strings.Contains(stderr.String(), "--host must be a server URL") { + t.Fatalf("exited %d: %s", code, stderr.String()) + } +} diff --git a/internal/command/review_test.go b/internal/command/review_test.go index a8ea750..7cb3e8d 100644 --- a/internal/command/review_test.go +++ b/internal/command/review_test.go @@ -84,7 +84,7 @@ func TestCommittedReviewFlagsReachTheAPI(t *testing.T) { "/api/reviews": {{status: http.StatusAccepted, body: fixture(t, "review-started.json")}}, "/api/reviews/": {{body: fixture(t, "review-done.json")}}, }) - stdout, stderr, code := run("review", "--repo", folder, "--base", strings.Repeat("a", 40), "--head", strings.Repeat("b", 40), "--format", "json") + stdout, stderr, code := run("review", "--dir", folder, "--base", strings.Repeat("a", 40), "--head", strings.Repeat("b", 40), "--format", "json") if code != 0 { t.Fatalf("exited %d: %s", code, stderr) } From 72a558d37b42111f7980ea683d20728e1c3409be Mon Sep 17 00:00:00 2001 From: nfebe Date: Tue, 6 Oct 2026 21:07:04 +0100 Subject: [PATCH 3/3] fix(cli): Report omitted files and upload plain review diffs --- internal/command/review_snapshot.go | 8 +++++++- internal/command/review_snapshot_test.go | 24 ++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/internal/command/review_snapshot.go b/internal/command/review_snapshot.go index 4c4d9ac..6129661 100644 --- a/internal/command/review_snapshot.go +++ b/internal/command/review_snapshot.go @@ -131,6 +131,12 @@ func remoteReview(cmd *cobra.Command, opts *options, folder string, settings sna } } else { report(cmd.OutOrStdout(), agent.Reading{Status: result.Status, Review: result.Review}) + if len(result.Snapshot.Omitted) != 0 { + _, _ = fmt.Fprintln(cmd.OutOrStdout(), "\nFiles omitted from the review snapshot:") + for _, file := range result.Snapshot.Omitted { + _, _ = fmt.Fprintln(cmd.OutOrStdout(), " "+file) + } + } } if !result.Review.Ready { return &unready{} @@ -178,7 +184,7 @@ func checkoutSnapshot(ctx context.Context, folder string, settings snapshotOptio if len(status) != 0 { return snapshot, fmt.Errorf("commit tracked changes before submitting a remote snapshot") } - diff, err := checkoutGit(ctx, folder, "diff", "--no-ext-diff", "--no-textconv", settings.base+"..."+settings.head) + diff, err := checkoutGit(ctx, folder, "diff", "--no-color", "--no-ext-diff", "--no-textconv", settings.base+"..."+settings.head) if err != nil { return snapshot, err } diff --git a/internal/command/review_snapshot_test.go b/internal/command/review_snapshot_test.go index 6e239a5..c95901c 100644 --- a/internal/command/review_snapshot_test.go +++ b/internal/command/review_snapshot_test.go @@ -33,6 +33,9 @@ func snapshotCheckout(t *testing.T) (string, string, string) { if err := os.WriteFile(filepath.Join(folder, "context.go"), []byte("package example\nvar Context = Value\n"), 0600); err != nil { t.Fatal(err) } + if err := os.WriteFile(filepath.Join(folder, "binary.bin"), []byte{0xff, 0x00}, 0600); err != nil { + t.Fatal(err) + } git("add", "-A") git("commit", "-q", "-m", "feat: Add the example") base := git("rev-parse", "HEAD") @@ -50,6 +53,17 @@ func snapshotCheckout(t *testing.T) (string, string, string) { func TestRemoteReviewUploadsCommittedContextAndUsesWorkspaceCredentials(t *testing.T) { folder, base, head := snapshotCheckout(t) + if encoded, err := exec.Command("git", "-C", folder, "config", "color.ui", "always").CombinedOutput(); err != nil { + t.Fatalf("git config failed: %s", encoded) + } + patch, err := exec.Command("git", "-C", folder, "diff", "--no-color", base+"..."+head).Output() + if err != nil { + t.Fatal(err) + } + patchFile := filepath.Join(t.TempDir(), "diff.patch") + if err := os.WriteFile(patchFile, patch, 0600); err != nil { + t.Fatal(err) + } t.Setenv("SOURCEANT_REVIEW_TOKEN", "your-api-key-here") t.Setenv("SOURCEANT_REVIEW_WORKSPACE", "benchmark") var captured reviewSnapshot @@ -75,12 +89,12 @@ func TestRemoteReviewUploadsCommittedContextAndUsesWorkspaceCredentials(t *testi t.Fatal(err) } response.Data.Review.Base = base - response.Data.Snapshot = snapshotIdentity{Repository: "acme/example", Base: base, Head: head} + response.Data.Snapshot = snapshotIdentity{Repository: "acme/example", Base: base, Head: head, Omitted: captured.Omitted} _ = json.NewEncoder(w).Encode(response) })) defer server.Close() var stdout, stderr bytes.Buffer - code := Run([]string{"review", "--dir", folder, "--base", base, "--head", head, "--repository", "acme/example", "--host", server.URL, "--format", "json", "--option", "discovery-passes=3"}, &stdout, &stderr) + code := Run([]string{"review", "--dir", folder, "--base", base, "--head", head, "--repository", "acme/example", "--host", server.URL, "--format", "json", "--option", "discovery-passes=3", "--diff-file", patchFile}, &stdout, &stderr) if code != 2 { t.Fatalf("exited %d: %s", code, stderr.String()) } @@ -113,6 +127,12 @@ func TestRemoteReviewUploadsCommittedContextAndUsesWorkspaceCredentials(t *testi if !json.Valid(stdout.Bytes()) { t.Fatalf("invalid JSON: %s", stdout.String()) } + stdout.Reset() + stderr.Reset() + code = Run([]string{"review", "--dir", folder, "--base", base, "--head", head, "--repository", "acme/example", "--host", server.URL}, &stdout, &stderr) + if code != 2 || !strings.Contains(stdout.String(), "Files omitted from the review snapshot:") || !strings.Contains(stdout.String(), "binary.bin") { + t.Fatalf("omitted file is hidden: exited %d, stdout=%s, stderr=%s", code, stdout.String(), stderr.String()) + } } func TestRemoteReviewRefusesWrongHeadBeforeUploading(t *testing.T) {