From 7335c7df5f3ddffec210b398a06e0ce1de4c8075 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Wed, 17 Jun 2026 16:19:37 +0200 Subject: [PATCH] trail finding: populate selected_text for line/range findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `entire trail finding add --line/--start-line` always failed against the control plane with `location.granularity=line requires non-empty selected_text`: the CLI built a line/range location but never set `selected_text`, and there was no flag to supply it. Only `--file` (file-granularity, no selected_text requirement) worked. selected_text is the diff anchor, so it must be the source text at those lines. Two ways to provide it now: - `--selected-text` flag — explicit, always wins. - Auto-read from the local working tree when the flag is omitted, behind a safety gate so the anchor text provably matches the trail's reviewed code: the finding's branch must equal the checked-out branch, the target file must have no uncommitted changes, and the trail head SHA must equal local HEAD. The user-supplied --file is validated (no absolute paths, no `..` escape, no .git) and the file is read through os.Root, which confines the open to the repo root at the kernel level and rejects symlinks that escape it. Any gate failure (or out-of-range lines) is a hard error pointing at `--selected-text`. Lines are joined with "\n" and capped at 16384 chars, matching the server's own selectedTextFromPatch derivation. The branch and clean-file checks run before the review is started, so those misconfigurations fail without side effects. The trail head SHA is only returned by starting the review, so `createTrailReviewFinding` finalizes the input via a callback that runs after the review is opened, where the HEAD-match check happens — a HEAD mismatch there does leave an empty review session (rare: requires the right branch and a clean tree but a different commit than the trail tip). Also factor out two shared helpers while here: `compareLocalHead` (now used by both the new head check and the existing `verifyTrailReviewHead`) and `gitStatusDirty` (now backing `HasUncommittedChanges` and the file-scoped clean check). Co-Authored-By: Claude Opus 4.8 (1M context) Entire-Checkpoint: 3e51051232cb --- cmd/entire/cli/git_operations.go | 28 +++- cmd/entire/cli/trail_review_cmd.go | 213 +++++++++++++++++++++--- cmd/entire/cli/trail_review_cmd_test.go | 194 ++++++++++++++++++++- 3 files changed, 399 insertions(+), 36 deletions(-) diff --git a/cmd/entire/cli/git_operations.go b/cmd/entire/cli/git_operations.go index 65c95b20c2..81a0699464 100644 --- a/cmd/entire/cli/git_operations.go +++ b/cmd/entire/cli/git_operations.go @@ -244,15 +244,33 @@ func GetMergeBase(ctx context.Context, branch1, branch2 string) (*plumbing.Hash, // HasUncommittedChanges checks if there are any uncommitted changes in the repository. // This includes staged changes, unstaged changes, and untracked files. -// Uses git CLI instead of go-git because go-git doesn't respect global gitignore -// (core.excludesfile) which can cause false positives for globally ignored files. func HasUncommittedChanges(ctx context.Context) (bool, error) { - cmd := exec.CommandContext(ctx, "git", "status", "--porcelain") - output, err := cmd.Output() + return gitStatusDirty(ctx, "") +} + +// gitStatusDirty reports whether `git status --porcelain` shows any staged, +// unstaged, or untracked changes. An empty root runs in the current directory; +// a non-empty root runs via `git -C root` (needed when pathspecs are +// repo-root-relative but the process CWD is a subdirectory). With pathspecs the +// check is scoped to those paths; untracked paths count as changes too. +// +// Uses the git CLI instead of go-git because go-git doesn't respect global +// gitignore (core.excludesfile), which can cause false positives for globally +// ignored files. +func gitStatusDirty(ctx context.Context, root string, pathspecs ...string) (bool, error) { + args := make([]string, 0, 4+len(pathspecs)) + if root != "" { + args = append(args, "-C", root) + } + args = append(args, "status", "--porcelain") + if len(pathspecs) > 0 { + args = append(args, "--") + args = append(args, pathspecs...) + } + output, err := exec.CommandContext(ctx, "git", args...).Output() if err != nil { return false, fmt.Errorf("failed to get git status: %w", err) } - // If output is empty, there are no changes return len(strings.TrimSpace(string(output))) > 0, nil } diff --git a/cmd/entire/cli/trail_review_cmd.go b/cmd/entire/cli/trail_review_cmd.go index 7c247ffea9..f5b5cb90b2 100644 --- a/cmd/entire/cli/trail_review_cmd.go +++ b/cmd/entire/cli/trail_review_cmd.go @@ -12,6 +12,7 @@ import ( "os" "os/exec" "path" + "path/filepath" "sort" "strconv" "strings" @@ -39,6 +40,12 @@ const ( trailReviewSeverityHigh = "high" trailReviewSeverityMedium = "medium" trailReviewSeverityLow = "low" + // Finding location granularities, mirroring the server's accepted values. + // line/range additionally require a non-empty selected_text. + trailReviewGranularityWholeChange = "whole_change" + trailReviewGranularityFile = "file" + trailReviewGranularityLine = "line" + trailReviewGranularityRange = "range" ) var errTrailReviewDefaultTargetNotFound = errors.New("default trail finding target not found") @@ -138,18 +145,19 @@ func newTrailFindingListCmd(targetOpts *trailReviewTargetOptions) *cobra.Command } type trailReviewCommentAddOptions struct { - Body string - Severity string - Confidence float64 - FilePath string - Line int - StartLine int - EndLine int - ClientID string - Patch string - PatchFile string - Instruction string - JSON bool + Body string + Severity string + Confidence float64 + FilePath string + Line int + StartLine int + EndLine int + SelectedText string + ClientID string + Patch string + PatchFile string + Instruction string + JSON bool } func newTrailFindingAddCmd(targetOpts *trailReviewTargetOptions) *cobra.Command { @@ -173,6 +181,7 @@ func newTrailFindingAddCmd(targetOpts *trailReviewTargetOptions) *cobra.Command cmd.Flags().IntVar(&opts.Line, "line", 0, "Line number for the finding location") cmd.Flags().IntVar(&opts.StartLine, "start-line", 0, "Start line for the finding location") cmd.Flags().IntVar(&opts.EndLine, "end-line", 0, "End line for the finding location") + cmd.Flags().StringVar(&opts.SelectedText, "selected-text", "", "Exact source text the finding anchors to (required by the server for line/range findings; auto-read from the local file when the working tree matches the trail head)") cmd.Flags().StringVar(&opts.ClientID, "client-id", "", "Client-provided idempotency key for this finding") cmd.Flags().StringVar(&opts.Patch, "patch", "", "Unified-diff suggested change to attach") cmd.Flags().StringVar(&opts.PatchFile, "patch-file", "", "Read unified-diff suggested change from file; use '-' for stdin") @@ -318,7 +327,29 @@ func runTrailReviewCommentAdd(cmd *cobra.Command, selector string, opts trailRev if err != nil { return err } - created, err := createTrailReviewFinding(cmd.Context(), client, target.Trail.ID, input) + // Line/range findings require a non-empty selected_text the server uses to + // anchor the finding to the diff. When --selected-text wasn't supplied, read + // it from the local working tree — but only when that tree provably matches + // the trail head, so the anchor text is correct. The branch/clean checks are + // local and run before the review is started; the head-SHA check needs the + // started review and runs inside the finalize callback below. + autoReadSelected := locationNeedsSelectedText(input.Location) && input.Location.SelectedText == nil + var selectedText string + if autoReadSelected { + selectedText, err = readSelectedTextFromWorktree(cmd.Context(), target.Trail, input.Location) + if err != nil { + return err + } + } + created, err := createTrailReviewFinding(cmd.Context(), client, target.Trail.ID, func(review api.TrailReviewStartResponse) (api.TrailReviewCommentInput, error) { + if autoReadSelected { + if err := verifyTrailHeadMatchesLocal(cmd.Context(), review.HeadSHA); err != nil { + return api.TrailReviewCommentInput{}, err + } + input.Location.SelectedText = stringPtr(selectedText) + } + return input, nil + }) if err != nil { return err } @@ -687,33 +718,172 @@ func buildTrailReviewCommentLocation(opts trailReviewCommentAddOptions) (api.Tra return api.TrailReviewLocationCreateRequest{}, errors.New("--end-line must be greater than or equal to the start line") } - loc := api.TrailReviewLocationCreateRequest{Granularity: "whole_change"} + loc := api.TrailReviewLocationCreateRequest{Granularity: trailReviewGranularityWholeChange} if filePath == "" { return loc, nil } - loc.Granularity = "file" + loc.Granularity = trailReviewGranularityFile loc.FilePath = stringPtr(filePath) if line > 0 { - loc.Granularity = "line" + loc.Granularity = trailReviewGranularityLine loc.StartLine = &line if opts.EndLine > 0 { loc.EndLine = &opts.EndLine if opts.EndLine != line { - loc.Granularity = "range" + loc.Granularity = trailReviewGranularityRange } } + // An explicit --selected-text always wins for line/range findings. + // When omitted, the caller auto-reads it from the local working tree + // (see readSelectedTextFromWorktree), but only after verifying the tree + // matches the trail head — which needs git/trail context this pure + // builder does not have. selected_text is meaningless for file + // granularity, so it is only attached here, inside the line branch. + if opts.SelectedText != "" { + loc.SelectedText = stringPtr(opts.SelectedText) + } } return loc, nil } +// locationNeedsSelectedText reports whether the server requires a non-empty +// selected_text for this location's granularity. The control-plane rejects +// line/range findings without it (planetscale/code-review.ts:validateLocationShape); +// file and whole_change findings never carry selected_text. +func locationNeedsSelectedText(loc api.TrailReviewLocationCreateRequest) bool { + return loc.Granularity == trailReviewGranularityLine || loc.Granularity == trailReviewGranularityRange +} + +// maxSelectedTextLen mirrors the server's selected_text cap +// (MAX_SELECTED_TEXT_LENGTH in review-publication.ts and the create-comment +// zod schema). Reading past it would be rejected, so truncate to match. +const maxSelectedTextLen = 16_384 + +// readSelectedTextFromWorktree reads the source lines a line/range finding +// anchors to from the local working tree. It runs the *local* halves of the +// safety gate — the finding must be raised against the trail's own branch, and +// the target file must have no uncommitted changes — so the text we read +// matches the trail's committed code. The remaining gate (local HEAD == trail +// head) needs the started review and is checked separately by +// verifyTrailHeadMatchesLocal. Any failure is fatal: the user can always pass +// --selected-text to anchor the finding explicitly. +func readSelectedTextFromWorktree(ctx context.Context, t api.TrailResource, loc api.TrailReviewLocationCreateRequest) (string, error) { + if loc.FilePath == nil || loc.StartLine == nil { + return "", errors.New("internal: line finding missing file or start line") + } + relPath := *loc.FilePath + start := *loc.StartLine + end := start + if loc.EndLine != nil { + end = *loc.EndLine + } + // --file is user-supplied and used below both as an on-disk read path and a + // git pathspec. Constrain it to a repo-relative path (no absolute paths, no + // .. escape, no .git) before touching the filesystem; validatePatchPath also + // normalizes backslashes so a Windows-style path doesn't slip through. + if err := validatePatchPath(relPath); err != nil { + return "", fmt.Errorf("cannot auto-read selected text: %w; pass --selected-text to anchor the finding explicitly", err) + } + + branch, err := GetCurrentBranch(ctx) + if err != nil { + return "", fmt.Errorf("cannot auto-read selected text for %s: %w; pass --selected-text to anchor the finding explicitly", relPath, err) + } + if t.Branch != "" && branch != t.Branch { + return "", fmt.Errorf("cannot auto-read selected text: trail is on branch %q but %q is checked out; switch branches or pass --selected-text", t.Branch, branch) + } + + root, err := paths.WorktreeRoot(ctx) + if err != nil { + return "", fmt.Errorf("resolve worktree root: %w", err) + } + dirty, err := gitStatusDirty(ctx, root, relPath) + if err != nil { + return "", fmt.Errorf("cannot auto-read selected text for %s: %w; pass --selected-text to anchor the finding explicitly", relPath, err) + } + if dirty { + return "", fmt.Errorf("cannot auto-read selected text: %s has uncommitted changes; commit them or pass --selected-text", relPath) + } + + // Read through os.Root so the open is confined to the repo root by the + // kernel — a symlink inside the repo that points outside it can't be used + // to escape, which the string checks in validatePatchPath can't catch. + osRoot, err := os.OpenRoot(root) + if err != nil { + return "", fmt.Errorf("open repo root for selected text: %w", err) + } + defer osRoot.Close() + data, err := osRoot.ReadFile(filepath.FromSlash(relPath)) + if err != nil { + return "", fmt.Errorf("read %s for selected text: %w; pass --selected-text", relPath, err) + } + lines := strings.Split(string(data), "\n") + // A trailing newline yields a final empty element that is not a real line; + // drop it so line counts match the file's actual lines. + if n := len(lines); n > 0 && lines[n-1] == "" { + lines = lines[:n-1] + } + if start < 1 || end > len(lines) { + return "", fmt.Errorf("lines %d-%d are out of range for %s (%d lines); fix the range or pass --selected-text", start, end, relPath, len(lines)) + } + selected := strings.Join(lines[start-1:end], "\n") + if selected == "" { + return "", fmt.Errorf("selected text for %s lines %d-%d is empty; pass --selected-text", relPath, start, end) + } + if len(selected) > maxSelectedTextLen { + selected = selected[:maxSelectedTextLen] + } + return selected, nil +} + +// verifyTrailHeadMatchesLocal is the head-SHA half of the auto-read safety gate: +// the trail head the finding attaches to must equal the local HEAD, otherwise +// the working-tree lines we read may not correspond to the reviewed code. A +// trail with no resolved head SHA (headSHA == nil/empty) skips the check. +func verifyTrailHeadMatchesLocal(ctx context.Context, headSHA *string) error { + want := strings.TrimSpace(stringPtrValue(headSHA)) + got, matched, err := compareLocalHead(ctx, want) + if err != nil { + return err + } + if !matched { + return fmt.Errorf("cannot auto-read selected text: trail head is %s but local HEAD is %s; check out that commit or pass --selected-text", abbreviate12(want), abbreviate12(got)) + } + return nil +} + +// compareLocalHead resolves the local HEAD and compares it to want (a trail or +// review head SHA). An empty want means "no head pinned" and reports matched +// with an empty got. Callers format their own mismatch message from want/got. +func compareLocalHead(ctx context.Context, want string) (got string, matched bool, err error) { + want = strings.TrimSpace(want) + if want == "" { + return "", true, nil + } + got, err = resolveGitRev(ctx, "HEAD") + if err != nil { + return "", false, err + } + return got, got == want, nil +} + // createTrailReviewFinding posts a single finding through the current API flow: // start a review session, then submit a one-item comment batch under it. It // returns the created (or already-existing) finding. -func createTrailReviewFinding(ctx context.Context, client *api.Client, trailID string, input api.TrailReviewCommentInput) (api.TrailReviewComment, error) { +// +// finalize receives the started review (whose HeadSHA pins the code version the +// finding attaches to) and returns the input to post. It runs after the review +// is opened so callers that need the trail head — e.g. auto-reading selected +// text from a working tree that must match that head — can verify it first. +func createTrailReviewFinding(ctx context.Context, client *api.Client, trailID string, finalize func(api.TrailReviewStartResponse) (api.TrailReviewCommentInput, error)) (api.TrailReviewComment, error) { review, err := startTrailReview(ctx, client, trailID) if err != nil { return api.TrailReviewComment{}, err } + input, err := finalize(review) + if err != nil { + return api.TrailReviewComment{}, err + } resp, err := client.Post(ctx, trailReviewBatchCommentsPath(trailID, review.ReviewID), api.TrailReviewCommentBatchRequest{ Comments: []api.TrailReviewCommentInput{input}, }) @@ -897,14 +1067,11 @@ func trailReviewStatePath(trailID, reviewID, cursor string) string { func verifyTrailReviewHead(ctx context.Context, state api.TrailReviewStateResponse) error { want := strings.TrimSpace(stringPtrValue(state.CodeVersion.HeadSHA)) - if want == "" { - return nil - } - got, err := resolveGitRev(ctx, "HEAD") + got, matched, err := compareLocalHead(ctx, want) if err != nil { return err } - if got != want { + if !matched { return fmt.Errorf("finding was created for head %s, but current HEAD is %s; check out that commit before applying", abbreviate12(want), abbreviate12(got)) } return nil diff --git a/cmd/entire/cli/trail_review_cmd_test.go b/cmd/entire/cli/trail_review_cmd_test.go index b45379670d..6b27801217 100644 --- a/cmd/entire/cli/trail_review_cmd_test.go +++ b/cmd/entire/cli/trail_review_cmd_test.go @@ -179,6 +179,180 @@ func TestBuildTrailReviewCommentInput(t *testing.T) { } } +func TestBuildTrailReviewCommentLocationExplicitSelectedText(t *testing.T) { + t.Parallel() + loc, err := buildTrailReviewCommentLocation(trailReviewCommentAddOptions{ + FilePath: "src/auth/session.ts", + Line: 88, + SelectedText: "const token = mint()", + }) + if err != nil { + t.Fatalf("buildTrailReviewCommentLocation: %v", err) + } + if loc.Granularity != trailReviewGranularityLine { + t.Fatalf("Granularity = %q, want line", loc.Granularity) + } + if loc.SelectedText == nil || *loc.SelectedText != "const token = mint()" { + t.Fatalf("SelectedText = %#v", loc.SelectedText) + } +} + +func TestBuildTrailReviewCommentLocationFileGranularityIgnoresSelectedText(t *testing.T) { + t.Parallel() + // --selected-text without line flags is a file-level finding; selected_text + // is meaningless there and must not be attached. + loc, err := buildTrailReviewCommentLocation(trailReviewCommentAddOptions{ + FilePath: "src/auth/session.ts", + SelectedText: "ignored", + }) + if err != nil { + t.Fatalf("buildTrailReviewCommentLocation: %v", err) + } + if loc.Granularity != trailReviewGranularityFile { + t.Fatalf("Granularity = %q, want file", loc.Granularity) + } + if loc.SelectedText != nil { + t.Fatalf("SelectedText = %#v, want nil for file granularity", loc.SelectedText) + } +} + +func TestLocationNeedsSelectedText(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + granularity string + want bool + }{ + {trailReviewGranularityLine, true}, + {trailReviewGranularityRange, true}, + {trailReviewGranularityFile, false}, + {trailReviewGranularityWholeChange, false}, + } { + if got := locationNeedsSelectedText(api.TrailReviewLocationCreateRequest{Granularity: tc.granularity}); got != tc.want { + t.Errorf("locationNeedsSelectedText(%q) = %v, want %v", tc.granularity, got, tc.want) + } + } +} + +// Not parallel: uses t.Chdir() so git/worktree resolution targets the temp repo. +func TestReadSelectedTextFromWorktree(t *testing.T) { + repoDir := t.TempDir() + testutil.InitRepo(t, repoDir) + testutil.WriteFile(t, repoDir, "src/app.go", "package app\n\nfunc Run() {}\n\nvar X = 1\n") + testutil.GitAdd(t, repoDir, "src/app.go") + testutil.GitCommit(t, repoDir, "init") + t.Chdir(repoDir) + + ctx := context.Background() + branch, err := GetCurrentBranch(ctx) + if err != nil { + t.Fatalf("GetCurrentBranch: %v", err) + } + trail := api.TrailResource{Branch: branch} + + startLine := 3 + endLine := 5 + rangeLoc := api.TrailReviewLocationCreateRequest{ + Granularity: trailReviewGranularityRange, + FilePath: trailReviewStrPtr("src/app.go"), + StartLine: &startLine, + EndLine: &endLine, + } + + got, err := readSelectedTextFromWorktree(ctx, trail, rangeLoc) + if err != nil { + t.Fatalf("readSelectedTextFromWorktree: %v", err) + } + want := "func Run() {}\n\nvar X = 1" + if got != want { + t.Fatalf("selected text = %q, want %q", got, want) + } + + // Branch mismatch is fatal. + if _, err := readSelectedTextFromWorktree(ctx, api.TrailResource{Branch: "other-branch"}, rangeLoc); err == nil || + !strings.Contains(err.Error(), "other-branch") { + t.Fatalf("branch mismatch error = %v, want mention of trail branch", err) + } + + // Paths that escape the repo root are rejected before any filesystem access. + for _, bad := range []string{"/etc/passwd", "../../secret.txt"} { + if _, err := readSelectedTextFromWorktree(ctx, trail, api.TrailReviewLocationCreateRequest{ + Granularity: trailReviewGranularityRange, + FilePath: trailReviewStrPtr(bad), + StartLine: &startLine, + EndLine: &endLine, + }); err == nil { + t.Fatalf("expected error for escaping path %q", bad) + } + } + + // os.Root rejects a tracked symlink whose target escapes the repo, even + // though the path itself looks repo-relative and the tree is clean — + // something validatePatchPath's string checks can't catch. + if err := os.Symlink("/etc/hosts", filepath.Join(repoDir, "src/escape.txt")); err != nil { + t.Skipf("symlink unsupported on this platform: %v", err) + } + testutil.GitAdd(t, repoDir, "src/escape.txt") + testutil.GitCommit(t, repoDir, "add escaping symlink") + one := 1 + if _, err := readSelectedTextFromWorktree(ctx, trail, api.TrailReviewLocationCreateRequest{ + Granularity: trailReviewGranularityLine, + FilePath: trailReviewStrPtr("src/escape.txt"), + StartLine: &one, + }); err == nil { + t.Fatal("expected error reading a symlink that escapes the repo root") + } + + // Out-of-range lines are fatal. + bad := startLine + huge := 999 + if _, err := readSelectedTextFromWorktree(ctx, trail, api.TrailReviewLocationCreateRequest{ + Granularity: trailReviewGranularityRange, + FilePath: trailReviewStrPtr("src/app.go"), + StartLine: &bad, + EndLine: &huge, + }); err == nil || !strings.Contains(err.Error(), "out of range") { + t.Fatalf("out-of-range error = %v, want 'out of range'", err) + } + + // Uncommitted changes to the target file are fatal. + testutil.WriteFile(t, repoDir, "src/app.go", "package app\n\nfunc Run() { panic(1) }\n\nvar X = 1\n") + if _, err := readSelectedTextFromWorktree(ctx, trail, rangeLoc); err == nil || + !strings.Contains(err.Error(), "uncommitted changes") { + t.Fatalf("dirty-file error = %v, want 'uncommitted changes'", err) + } +} + +// Not parallel: uses t.Chdir() so resolveGitRev targets the temp repo. +func TestVerifyTrailHeadMatchesLocal(t *testing.T) { + repoDir := t.TempDir() + testutil.InitRepo(t, repoDir) + testutil.WriteFile(t, repoDir, "f.txt", "x\n") + testutil.GitAdd(t, repoDir, "f.txt") + testutil.GitCommit(t, repoDir, "init") + t.Chdir(repoDir) + + ctx := context.Background() + head, err := resolveGitRev(ctx, "HEAD") + if err != nil { + t.Fatalf("resolveGitRev: %v", err) + } + + // nil head SHA: nothing to compare, no error. + if err := verifyTrailHeadMatchesLocal(ctx, nil); err != nil { + t.Fatalf("verifyTrailHeadMatchesLocal(nil) = %v, want nil", err) + } + // Matching head: ok. + if err := verifyTrailHeadMatchesLocal(ctx, &head); err != nil { + t.Fatalf("verifyTrailHeadMatchesLocal(head) = %v, want nil", err) + } + // Divergent head: fatal. + other := "0000000000000000000000000000000000000000" + if err := verifyTrailHeadMatchesLocal(ctx, &other); err == nil || + !strings.Contains(err.Error(), "local HEAD") { + t.Fatalf("verifyTrailHeadMatchesLocal(other) = %v, want HEAD-mismatch error", err) + } +} + func TestBuildTrailReviewCommentInputGeneratesClientID(t *testing.T) { t.Parallel() input, err := buildTrailReviewCommentInput(trailReviewCommentAddOptions{Body: "finding body"}) @@ -219,10 +393,12 @@ func TestCreateTrailReviewFindingStartsReviewThenPostsBatch(t *testing.T) { t.Setenv(api.BaseURLEnvVar, srv.URL) client := api.NewClient("tok") - created, err := createTrailReviewFinding(context.Background(), client, "trl_1", api.TrailReviewCommentInput{ - ClientID: "agent-run-1:finding-1", - Body: trailReviewStrPtr("body"), - Location: api.TrailReviewLocationCreateRequest{Granularity: "whole_change"}, + created, err := createTrailReviewFinding(context.Background(), client, "trl_1", func(api.TrailReviewStartResponse) (api.TrailReviewCommentInput, error) { + return api.TrailReviewCommentInput{ + ClientID: "agent-run-1:finding-1", + Body: trailReviewStrPtr("body"), + Location: api.TrailReviewLocationCreateRequest{Granularity: "whole_change"}, + }, nil }) if err != nil { t.Fatalf("createTrailReviewFinding: %v", err) @@ -263,10 +439,12 @@ func TestCreateTrailReviewFindingSurfacesBatchError(t *testing.T) { t.Setenv(api.BaseURLEnvVar, srv.URL) client := api.NewClient("tok") - _, err := createTrailReviewFinding(context.Background(), client, "trl_1", api.TrailReviewCommentInput{ - ClientID: "c1", - Body: trailReviewStrPtr("body"), - Location: api.TrailReviewLocationCreateRequest{Granularity: "whole_change"}, + _, err := createTrailReviewFinding(context.Background(), client, "trl_1", func(api.TrailReviewStartResponse) (api.TrailReviewCommentInput, error) { + return api.TrailReviewCommentInput{ + ClientID: "c1", + Body: trailReviewStrPtr("body"), + Location: api.TrailReviewLocationCreateRequest{Granularity: "whole_change"}, + }, nil }) if err == nil { t.Fatal("expected an error when the batch result reports status=error")