From 954d51f91981aaf66a2dedc774cd0d17491b737c Mon Sep 17 00:00:00 2001 From: dip Date: Mon, 25 May 2026 14:19:54 +0200 Subject: [PATCH 1/6] feat: add trail review CLI Entire-Checkpoint: 0fd32e9ceae9 --- cmd/entire/cli/api/client.go | 7 +- cmd/entire/cli/api/trail_review_types.go | 179 ++++ cmd/entire/cli/trail_cmd.go | 1 + cmd/entire/cli/trail_review_cmd.go | 1065 ++++++++++++++++++++++ cmd/entire/cli/trail_review_cmd_test.go | 154 ++++ cmd/entire/cli/trail_watch_cmd.go | 4 + 6 files changed, 1409 insertions(+), 1 deletion(-) create mode 100644 cmd/entire/cli/api/trail_review_types.go create mode 100644 cmd/entire/cli/trail_review_cmd.go create mode 100644 cmd/entire/cli/trail_review_cmd_test.go diff --git a/cmd/entire/cli/api/client.go b/cmd/entire/cli/api/client.go index 17365ff31..bff4372f5 100644 --- a/cmd/entire/cli/api/client.go +++ b/cmd/entire/cli/api/client.go @@ -73,6 +73,11 @@ func (c *Client) GetStream(ctx context.Context, path string, headers http.Header // Post sends an authenticated POST request with a JSON body to the given API-relative path. func (c *Client) Post(ctx context.Context, path string, body any) (*http.Response, error) { + return c.PostWithHeaders(ctx, path, body, nil) +} + +// PostWithHeaders sends an authenticated POST request with a JSON body and extra headers. +func (c *Client) PostWithHeaders(ctx context.Context, path string, body any, headers http.Header) (*http.Response, error) { var reader io.Reader if body != nil { data, err := json.Marshal(body) @@ -81,7 +86,7 @@ func (c *Client) Post(ctx context.Context, path string, body any) (*http.Respons } reader = bytes.NewReader(data) } - return c.do(ctx, http.MethodPost, path, reader, nil) + return c.do(ctx, http.MethodPost, path, reader, headers) } // Put sends an authenticated PUT request with a JSON body to the given API-relative path. diff --git a/cmd/entire/cli/api/trail_review_types.go b/cmd/entire/cli/api/trail_review_types.go new file mode 100644 index 000000000..33fc33c9e --- /dev/null +++ b/cmd/entire/cli/api/trail_review_types.go @@ -0,0 +1,179 @@ +package api + +import "time" + +// TrailReviewStartRequest is the body for POST /api/v1/trails/{trail_id}/reviews. +type TrailReviewStartRequest struct { + HeadSHA *string `json:"head_sha,omitempty"` + BaseSHA *string `json:"base_sha,omitempty"` + BaseRef *string `json:"base_ref,omitempty"` + HeadRef *string `json:"head_ref,omitempty"` +} + +// TrailReviewStartResponse is returned after creating or reusing a code review. +type TrailReviewStartResponse struct { + ReviewID string `json:"review_id"` + TrailID string `json:"trail_id"` + RepositoryID string `json:"repository_id"` + CodeVersionID string `json:"code_version_id"` + BaseSHA *string `json:"base_sha"` + HeadSHA *string `json:"head_sha"` + EventStreamURL string `json:"event_stream_url"` + DiffURL string `json:"diff_url"` + FilesURL string `json:"files_url"` + Limits TrailReviewStartLimits `json:"limits"` +} + +// TrailReviewStartLimits describes per-review API limits. +type TrailReviewStartLimits struct { + MaxCommentsPerBatch int `json:"max_comments_per_batch"` +} + +// TrailReviewStateResponse is returned by GET /api/v1/trails/{trail_id}/reviews/{id}. +type TrailReviewStateResponse struct { + Review TrailReview `json:"review"` + CodeVersion TrailReviewCodeVersion `json:"code_version"` + Counts TrailReviewCounts `json:"counts"` + Comments []TrailReviewComment `json:"comments"` + NextCursor *string `json:"next_cursor"` + EventCursor string `json:"event_cursor"` +} + +// TrailReview represents a review session. +type TrailReview struct { + ID string `json:"id"` + TrailID string `json:"trail_id"` + CodeVersionID string `json:"code_version_id"` + ActorID string `json:"actor_id"` + Summary *string `json:"summary"` + StartedAt time.Time `json:"started_at"` +} + +// TrailReviewCodeVersion pins the base/head that a review covers. +type TrailReviewCodeVersion struct { + ID string `json:"id"` + TrailID string `json:"trail_id"` + RepositoryID string `json:"repository_id"` + BaseRef *string `json:"base_ref"` + HeadRef *string `json:"head_ref"` + BaseSHA *string `json:"base_sha"` + HeadSHA *string `json:"head_sha"` + CapturedAt time.Time `json:"captured_at"` +} + +// TrailReviewCounts are review-scoped comment counts. +type TrailReviewCounts struct { + Open int `json:"open"` + Resolved int `json:"resolved"` + Dismissed int `json:"dismissed"` + Stale int `json:"stale"` + Total int `json:"total"` +} + +// TrailReviewCommentsResponse is returned by trail/review comment list endpoints. +type TrailReviewCommentsResponse struct { + Comments []TrailReviewComment `json:"comments"` + HasMore bool `json:"has_more"` + NextOffset *int `json:"next_offset"` +} + +// TrailReviewComment is a single agent-native review finding. +type TrailReviewComment struct { + ID string `json:"id"` + TrailID string `json:"trail_id"` + RepositoryID string `json:"repository_id"` + ReviewID string `json:"review_id"` + CodeVersionID string `json:"code_version_id"` + ActorID string `json:"actor_id"` + Title *string `json:"title"` + Body *string `json:"body"` + Severity *string `json:"severity"` + Confidence *float64 `json:"confidence"` + Status string `json:"status"` + StatusReason *string `json:"status_reason"` + StaleOutcome string `json:"stale_outcome"` + StaleCheckedAt *time.Time `json:"stale_checked_at"` + StaleCheckedCodeVersionID *string `json:"stale_checked_code_version_id"` + ClientID *string `json:"client_id"` + ClientIDHash *string `json:"client_id_hash"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + Location TrailReviewLocation `json:"location"` + SuggestedChanges []TrailReviewSuggestedChange `json:"suggested_changes,omitempty"` + ThreadID *string `json:"thread_id,omitempty"` + ThreadMessageCount int `json:"thread_message_count,omitempty"` + OutgoingLinks []TrailReviewOutgoingLink `json:"outgoing_links,omitempty"` +} + +// TrailReviewLocation identifies where a finding applies. +type TrailReviewLocation struct { + ID string `json:"id"` + ReviewCommentID string `json:"review_comment_id"` + CodeVersionID string `json:"code_version_id"` + Granularity string `json:"granularity"` + FilePath *string `json:"file_path"` + StartLine *int `json:"start_line"` + StartColumn *int `json:"start_column"` + EndLine *int `json:"end_line"` + EndColumn *int `json:"end_column"` + SelectedText *string `json:"selected_text"` + NearbyText *string `json:"nearby_text"` + Language *string `json:"language"` +} + +// TrailReviewSuggestedChange describes a machine-applicable or manual fix. +type TrailReviewSuggestedChange struct { + ID string `json:"id"` + ReviewCommentID string `json:"review_comment_id"` + CodeVersionID string `json:"code_version_id"` + ChangeType string `json:"change_type"` + Patch *string `json:"patch"` + Instruction *string `json:"instruction"` + ExpectedFilePath *string `json:"expected_file_path"` + ExpectedFileHash *string `json:"expected_file_hash"` + ExpectedStartLine *int `json:"expected_start_line"` + ExpectedEndLine *int `json:"expected_end_line"` + ExpectedLines *string `json:"expected_lines"` + CreatedBy string `json:"created_by"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` +} + +// TrailReviewOutgoingLink relates two review comments. +type TrailReviewOutgoingLink struct { + SourceCommentID string `json:"source_comment_id"` + TargetCommentID string `json:"target_comment_id"` + LinkType string `json:"link_type"` +} + +// TrailReviewCommentPatchRequest updates a review finding. +type TrailReviewCommentPatchRequest struct { + Title *string `json:"title,omitempty"` + Body *string `json:"body,omitempty"` + Severity *string `json:"severity,omitempty"` + Confidence *float64 `json:"confidence,omitempty"` + Status string `json:"status,omitempty"` + StatusReason *string `json:"status_reason,omitempty"` +} + +// TrailSubmitReviewRequest is the body for POST /api/v1/trails/{host}/{owner}/{repo}/{number}/review. +type TrailSubmitReviewRequest struct { + Event string `json:"event"` + Body string `json:"body,omitempty"` +} + +// TrailSubmitReviewResponse is returned after approving or requesting changes on a trail. +type TrailSubmitReviewResponse struct { + OK bool `json:"ok"` + Review TrailSubmitReview `json:"review"` +} + +// TrailSubmitReview is a human trail review verdict. +type TrailSubmitReview struct { + ID string `json:"id"` + Author string `json:"author"` + Event string `json:"event"` + Body *string `json:"body"` + CommitSHA string `json:"commit_sha"` + CreatedAt time.Time `json:"created_at"` +} diff --git a/cmd/entire/cli/trail_cmd.go b/cmd/entire/cli/trail_cmd.go index 353e1c72e..6aba0a87b 100644 --- a/cmd/entire/cli/trail_cmd.go +++ b/cmd/entire/cli/trail_cmd.go @@ -58,6 +58,7 @@ branch, or lists recent trails if no trail exists for the current branch.`, cmd.AddCommand(newTrailListCmd()) cmd.AddCommand(newTrailCreateCmd()) cmd.AddCommand(newTrailUpdateCmd()) + cmd.AddCommand(newTrailReviewCmd()) cmd.AddCommand(newTrailWatchCmd()) return cmd diff --git a/cmd/entire/cli/trail_review_cmd.go b/cmd/entire/cli/trail_review_cmd.go new file mode 100644 index 000000000..747eae610 --- /dev/null +++ b/cmd/entire/cli/trail_review_cmd.go @@ -0,0 +1,1065 @@ +package cli + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "os/exec" + "sort" + "strconv" + "strings" + "text/tabwriter" + + "github.com/entireio/cli/cmd/entire/cli/api" + "github.com/entireio/cli/cmd/entire/cli/gitremote" + "github.com/entireio/cli/cmd/entire/cli/paths" + + "github.com/spf13/cobra" +) + +const ( + defaultTrailReviewLimit = 100 + trailReviewStatusAny = "any" + trailReviewStaleCurrent = "current" + trailReviewStaleAny = "any" + trailReviewStatusOpen = "open" + trailReviewStatusResolved = "resolved" + trailReviewStatusDismissed = "dismissed" +) + +type trailReviewListOptions struct { + Status string + Severity string + Stale string + IncludeDismissed bool + Limit int + Offset int + JSON bool +} + +type trailReviewTarget struct { + Host string + Owner string + Repo string + Trail api.TrailResource +} + +func newTrailReviewCmd() *cobra.Command { + opts := defaultTrailReviewListOptions() + + cmd := &cobra.Command{ + Use: "review []", + Short: "Review a trail's agent findings", + Long: `Review a trail's agent-native code-review findings. + +Running 'entire trail review' shows the review dashboard for the current +branch's trail. Pass a trail number to review another trail in the same repo.`, + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + number, err := parseOptionalTrailNumber(args) + if err != nil { + return err + } + return runTrailReviewDashboard(cmd, number, opts) + }, + } + addTrailReviewListFlags(cmd, &opts) + + cmd.AddCommand(newTrailReviewStartCmd()) + cmd.AddCommand(newTrailReviewCommentsCmd()) + cmd.AddCommand(newTrailReviewShowCmd()) + cmd.AddCommand(newTrailReviewApplyCmd()) + cmd.AddCommand(newTrailReviewStatusCmd("resolve", trailReviewStatusResolved, "Resolve a review finding")) + cmd.AddCommand(newTrailReviewStatusCmd("dismiss", trailReviewStatusDismissed, "Dismiss a review finding")) + cmd.AddCommand(newTrailReviewStatusCmd("reopen", trailReviewStatusOpen, "Reopen a review finding")) + cmd.AddCommand(newTrailReviewWatchCmd()) + cmd.AddCommand(newTrailReviewSubmitCmd("approve", "APPROVE", "Approve a trail")) + cmd.AddCommand(newTrailReviewSubmitCmd("request-changes", "REQUEST_CHANGES", "Request changes on a trail")) + + return cmd +} + +func defaultTrailReviewListOptions() trailReviewListOptions { + return trailReviewListOptions{ + Status: trailReviewStatusOpen, + Stale: trailReviewStaleCurrent, + Limit: defaultTrailReviewLimit, + } +} + +func addTrailReviewListFlags(cmd *cobra.Command, opts *trailReviewListOptions) { + cmd.Flags().StringVar(&opts.Status, "status", opts.Status, "Filter by comma-separated status(es): open,resolved,dismissed; use 'any' for all") + cmd.Flags().StringVar(&opts.Severity, "severity", "", "Filter by comma-separated severity value(s): high,medium,low") + cmd.Flags().StringVar(&opts.Stale, "stale", opts.Stale, "Filter stale state: current,stale,any") + cmd.Flags().BoolVar(&opts.IncludeDismissed, "include-dismissed", false, "Include dismissed findings") + cmd.Flags().IntVarP(&opts.Limit, "limit", "n", opts.Limit, "Maximum number of findings to show") + cmd.Flags().IntVar(&opts.Offset, "offset", 0, "Pagination offset") + cmd.Flags().BoolVar(&opts.JSON, "json", false, "Output as JSON") +} + +type trailReviewStartOptions struct { + BaseRef string + HeadRef string + BaseSHA string + HeadSHA string + JSON bool + Watch bool +} + +func newTrailReviewStartCmd() *cobra.Command { + var opts trailReviewStartOptions + cmd := &cobra.Command{ + Use: "start []", + Short: "Start an agent-native review for a trail", + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + number, err := parseOptionalTrailNumber(args) + if err != nil { + return err + } + return runTrailReviewStart(cmd, number, opts) + }, + } + cmd.Flags().StringVar(&opts.BaseRef, "base-ref", "", "Base ref covered by the review (defaults to trail base)") + cmd.Flags().StringVar(&opts.HeadRef, "head-ref", "", "Head ref covered by the review (defaults to current branch)") + cmd.Flags().StringVar(&opts.BaseSHA, "base-sha", "", "Base SHA covered by the review") + cmd.Flags().StringVar(&opts.HeadSHA, "head-sha", "", "Head SHA covered by the review (defaults to HEAD)") + cmd.Flags().BoolVar(&opts.JSON, "json", false, "Output as JSON") + cmd.Flags().BoolVar(&opts.Watch, "watch", false, "After starting, stream trail review events") + return cmd +} + +func newTrailReviewCommentsCmd() *cobra.Command { + opts := defaultTrailReviewListOptions() + cmd := &cobra.Command{ + Use: "comments []", + Short: "List review findings for a trail", + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + number, err := parseOptionalTrailNumber(args) + if err != nil { + return err + } + return runTrailReviewComments(cmd, number, opts) + }, + } + addTrailReviewListFlags(cmd, &opts) + return cmd +} + +func newTrailReviewShowCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "show [] ", + Short: "Show a review finding", + Args: cobra.RangeArgs(1, 2), + RunE: func(cmd *cobra.Command, args []string) error { + number, commentID, err := parseTrailNumberAndCommentID(args) + if err != nil { + return err + } + return runTrailReviewShow(cmd, number, commentID) + }, + } + return cmd +} + +type trailReviewApplyOptions struct { + Resolve bool + Check bool +} + +func newTrailReviewApplyCmd() *cobra.Command { + var opts trailReviewApplyOptions + cmd := &cobra.Command{ + Use: "apply [] ", + Short: "Apply a review finding's unified-diff suggestion", + Args: cobra.RangeArgs(1, 2), + RunE: func(cmd *cobra.Command, args []string) error { + number, commentID, err := parseTrailNumberAndCommentID(args) + if err != nil { + return err + } + return runTrailReviewApply(cmd, number, commentID, opts) + }, + } + cmd.Flags().BoolVar(&opts.Resolve, "resolve", false, "Mark the finding resolved after applying") + cmd.Flags().BoolVar(&opts.Check, "check", false, "Only check whether the patch applies; do not modify files") + return cmd +} + +func newTrailReviewStatusCmd(use, status, short string) *cobra.Command { + var message string + cmd := &cobra.Command{ + Use: use + " [] ", + Short: short, + Args: cobra.RangeArgs(1, 2), + RunE: func(cmd *cobra.Command, args []string) error { + number, commentID, err := parseTrailNumberAndCommentID(args) + if err != nil { + return err + } + return runTrailReviewSetStatus(cmd, number, commentID, status, message) + }, + } + cmd.Flags().StringVarP(&message, "message", "m", "", "Status reason to record") + return cmd +} + +func newTrailReviewWatchCmd() *cobra.Command { + var ( + jsonOutput bool + showPings bool + once bool + ) + cmd := &cobra.Command{ + Use: "watch []", + Short: "Tail a trail's code-review events live", + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + number, err := parseOptionalTrailNumber(args) + if err != nil { + return err + } + return runTrailWatchWithOptions(cmd, number, jsonOutput, showPings, once) + }, + } + cmd.Flags().BoolVar(&jsonOutput, "json", false, "Print each event as a single JSON line") + cmd.Flags().BoolVar(&showPings, "show-pings", false, "Print SSE keepalive pings (otherwise suppressed)") + cmd.Flags().BoolVar(&once, "once", false, "Open one SSE connection then exit instead of reconnecting") + return cmd +} + +func newTrailReviewSubmitCmd(use, event, short string) *cobra.Command { + var body string + cmd := &cobra.Command{ + Use: use + " []", + Short: short, + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + number, err := parseOptionalTrailNumber(args) + if err != nil { + return err + } + return runTrailReviewSubmit(cmd, number, event, body) + }, + } + cmd.Flags().StringVarP(&body, "message", "m", "", "Review message") + return cmd +} + +func runTrailReviewDashboard(cmd *cobra.Command, number int, opts trailReviewListOptions) error { + client, target, err := authenticatedTrailReviewTarget(cmd, number) + if err != nil { + return err + } + comments, hasMore, err := fetchTrailReviewComments(cmd.Context(), client, target.Trail.ID, opts) + if err != nil { + return err + } + if opts.JSON { + return encodeTrailReviewJSON(cmd.OutOrStdout(), target, comments, hasMore) + } + printTrailReviewDashboard(cmd.OutOrStdout(), target, comments, hasMore, opts) + return nil +} + +func runTrailReviewComments(cmd *cobra.Command, number int, opts trailReviewListOptions) error { + client, target, err := authenticatedTrailReviewTarget(cmd, number) + if err != nil { + return err + } + comments, hasMore, err := fetchTrailReviewComments(cmd.Context(), client, target.Trail.ID, opts) + if err != nil { + return err + } + if opts.JSON { + return encodeTrailReviewJSON(cmd.OutOrStdout(), target, comments, hasMore) + } + printTrailReviewComments(cmd.OutOrStdout(), comments, hasMore) + return nil +} + +func runTrailReviewShow(cmd *cobra.Command, number int, commentID string) error { + client, target, err := authenticatedTrailReviewTarget(cmd, number) + if err != nil { + return err + } + comment, err := resolveTrailReviewComment(cmd.Context(), client, target.Trail.ID, commentID) + if err != nil { + return err + } + comment, _ = hydrateTrailReviewCommentSuggestions(cmd.Context(), client, target.Trail.ID, comment) // best-effort detail enrichment + printTrailReviewCommentDetail(cmd.OutOrStdout(), comment) + return nil +} + +func runTrailReviewApply(cmd *cobra.Command, number int, commentID string, opts trailReviewApplyOptions) error { + client, target, err := authenticatedTrailReviewTarget(cmd, number) + if err != nil { + return err + } + comment, err := resolveTrailReviewComment(cmd.Context(), client, target.Trail.ID, commentID) + if err != nil { + return err + } + comment, state, err := hydrateTrailReviewCommentWithState(cmd.Context(), client, target.Trail.ID, comment) + if err != nil { + return err + } + if err := verifyTrailReviewHead(cmd.Context(), state); err != nil { + return err + } + applied, err := applyTrailReviewSuggestions(cmd.Context(), comment, opts.Check, cmd.OutOrStdout()) + if err != nil { + return err + } + if opts.Check { + fmt.Fprintf(cmd.OutOrStdout(), "%d suggested change(s) apply cleanly.\n", applied) + return nil + } + fmt.Fprintf(cmd.OutOrStdout(), "Applied %d suggested change(s).\n", applied) + if opts.Resolve { + updated, err := patchTrailReviewCommentStatus(cmd.Context(), client, target.Trail.ID, comment, trailReviewStatusResolved, "Applied via Entire CLI") + if err != nil { + return err + } + fmt.Fprintf(cmd.OutOrStdout(), "Updated comment %s: %s → %s\n", updated.ID, comment.Status, updated.Status) + } + return nil +} + +func runTrailReviewSetStatus(cmd *cobra.Command, number int, commentID, status, message string) error { + client, target, err := authenticatedTrailReviewTarget(cmd, number) + if err != nil { + return err + } + comment, err := resolveTrailReviewComment(cmd.Context(), client, target.Trail.ID, commentID) + if err != nil { + return err + } + oldStatus := comment.Status + if message == "" { + message = defaultTrailReviewStatusReason(status) + } + updated, err := patchTrailReviewCommentStatus(cmd.Context(), client, target.Trail.ID, comment, status, message) + if err != nil { + return err + } + fmt.Fprintf(cmd.OutOrStdout(), "Updated comment %s: %s → %s\n", updated.ID, oldStatus, updated.Status) + return nil +} + +func runTrailReviewStart(cmd *cobra.Command, number int, opts trailReviewStartOptions) error { + client, target, err := authenticatedTrailReviewTarget(cmd, number) + if err != nil { + return err + } + request, err := buildTrailReviewStartRequest(cmd.Context(), target, opts) + if err != nil { + return err + } + idempotencyKey := trailReviewStartIdempotencyKey(target.Trail.ID, request) + started, err := startTrailReview(cmd.Context(), client, target.Trail.ID, request, idempotencyKey) + if err != nil { + return err + } + if opts.JSON { + enc := json.NewEncoder(cmd.OutOrStdout()) + enc.SetIndent("", " ") + if err := enc.Encode(started); err != nil { + return fmt.Errorf("encode review start response: %w", err) + } + } else { + printTrailReviewStart(cmd.OutOrStdout(), target, started) + } + if opts.Watch { + return runTrailWatchWithOptions(cmd, number, false, false, false) + } + return nil +} + +func runTrailReviewSubmit(cmd *cobra.Command, number int, event, body string) error { + client, target, err := authenticatedTrailReviewTarget(cmd, number) + if err != nil { + return err + } + if target.Trail.Number <= 0 { + return fmt.Errorf("trail %s has no number; cannot submit review", target.Trail.ID) + } + if event == "REQUEST_CHANGES" && strings.TrimSpace(body) == "" { + return errors.New("request-changes requires --message") + } + resp, err := submitTrailReview(cmd.Context(), client, target, event, body) + if err != nil { + return err + } + fmt.Fprintf(cmd.OutOrStdout(), "%s submitted by %s on %s\n", resp.Review.Event, resp.Review.Author, abbreviateMaybe(resp.Review.CommitSHA, 12)) + return nil +} + +func authenticatedTrailReviewTarget(cmd *cobra.Command, number int) (*api.Client, trailReviewTarget, error) { + client, err := NewAuthenticatedAPIClient(trailInsecureHTTP(cmd)) + if err != nil { + return nil, trailReviewTarget{}, fmt.Errorf("authentication required: %w", err) + } + target, err := resolveTrailReviewTarget(cmd.Context(), client, number) + if err != nil { + return nil, trailReviewTarget{}, err + } + return client, target, nil +} + +func resolveTrailReviewTarget(ctx context.Context, client *api.Client, number int) (trailReviewTarget, error) { + host, owner, repo, err := gitremote.ResolveRemoteRepo(ctx, "origin") + if err != nil { + return trailReviewTarget{}, fmt.Errorf("failed to resolve repository: %w", err) + } + + var found *api.TrailResource + if number > 0 { + found, err = findTrailByNumber(ctx, client, host, owner, repo, number) + if err != nil { + return trailReviewTarget{}, err + } + if found == nil { + return trailReviewTarget{}, fmt.Errorf("no trail #%d found in %s/%s/%s", number, host, owner, repo) + } + } else { + branch, branchErr := GetCurrentBranch(ctx) + if branchErr != nil { + return trailReviewTarget{}, fmt.Errorf("no trail number given and current branch is unknown: %w", branchErr) + } + found, err = findTrailByBranch(ctx, client, host, owner, repo, branch) + if err != nil { + return trailReviewTarget{}, err + } + if found == nil { + return trailReviewTarget{}, fmt.Errorf("no trail found for branch %q (run 'entire trail create' or pass a trail number)", branch) + } + } + if found.ID == "" { + return trailReviewTarget{}, errors.New("trail has no id yet") + } + return trailReviewTarget{Host: host, Owner: owner, Repo: repo, Trail: *found}, nil +} + +func fetchTrailReviewComments(ctx context.Context, client *api.Client, trailID string, opts trailReviewListOptions) ([]api.TrailReviewComment, bool, error) { + if opts.Limit <= 0 { + return nil, false, errors.New("limit must be greater than 0") + } + if opts.Offset < 0 { + return nil, false, errors.New("offset must be non-negative") + } + resp, err := client.Get(ctx, trailReviewCommentsPath(trailID, opts)) + if err != nil { + return nil, false, fmt.Errorf("list review comments: %w", err) + } + defer resp.Body.Close() + if err := checkTrailResponse(resp); err != nil { + return nil, false, err + } + var out api.TrailReviewCommentsResponse + if err := api.DecodeJSON(resp, &out); err != nil { + return nil, false, fmt.Errorf("decode review comments: %w", err) + } + return out.Comments, out.HasMore, nil +} + +func trailReviewCommentsPath(trailID string, opts trailReviewListOptions) string { + q := url.Values{} + if opts.Status != "" && opts.Status != trailReviewStatusAny { + q.Set("status", opts.Status) + } + if opts.Severity != "" { + q.Set("severity", opts.Severity) + } + if opts.Stale != "" && opts.Stale != trailReviewStaleAny { + q.Set("stale", opts.Stale) + } + if opts.IncludeDismissed { + q.Set("include_dismissed", "true") + } + if opts.Limit > 0 { + q.Set("limit", strconv.Itoa(opts.Limit)) + } + if opts.Offset > 0 { + q.Set("offset", strconv.Itoa(opts.Offset)) + } + path := "/api/v1/trails/" + url.PathEscape(trailID) + "/reviews/comments" + if encoded := q.Encode(); encoded != "" { + path += "?" + encoded + } + return path +} + +func resolveTrailReviewComment(ctx context.Context, client *api.Client, trailID, commentID string) (api.TrailReviewComment, error) { + opts := trailReviewListOptions{ + Status: trailReviewStatusAny, + Stale: trailReviewStaleAny, + IncludeDismissed: true, + Limit: defaultTrailReviewLimit, + } + var matches []api.TrailReviewComment + for { + comments, hasMore, err := fetchTrailReviewComments(ctx, client, trailID, opts) + if err != nil { + return api.TrailReviewComment{}, err + } + for _, comment := range comments { + if comment.ID == commentID { + return comment, nil + } + if strings.HasPrefix(comment.ID, commentID) { + matches = append(matches, comment) + } + } + if !hasMore { + break + } + opts.Offset += opts.Limit + } + switch len(matches) { + case 0: + return api.TrailReviewComment{}, fmt.Errorf("no review comment %q found", commentID) + case 1: + return matches[0], nil + default: + ids := make([]string, len(matches)) + for i := range matches { + ids[i] = matches[i].ID + } + sort.Strings(ids) + return api.TrailReviewComment{}, fmt.Errorf("ambiguous review comment %q (matches: %s)", commentID, strings.Join(ids, ", ")) + } +} + +func hydrateTrailReviewCommentSuggestions(ctx context.Context, client *api.Client, trailID string, comment api.TrailReviewComment) (api.TrailReviewComment, error) { + hydrated, _, err := hydrateTrailReviewCommentWithState(ctx, client, trailID, comment) + return hydrated, err +} + +func hydrateTrailReviewCommentWithState(ctx context.Context, client *api.Client, trailID string, comment api.TrailReviewComment) (api.TrailReviewComment, api.TrailReviewStateResponse, error) { + state, err := fetchTrailReviewState(ctx, client, trailID, comment.ReviewID) + if err != nil { + return api.TrailReviewComment{}, api.TrailReviewStateResponse{}, err + } + for _, candidate := range state.Comments { + if candidate.ID == comment.ID { + return candidate, state, nil + } + } + return api.TrailReviewComment{}, api.TrailReviewStateResponse{}, fmt.Errorf("review %s did not include comment %s", comment.ReviewID, comment.ID) +} + +func fetchTrailReviewState(ctx context.Context, client *api.Client, trailID, reviewID string) (api.TrailReviewStateResponse, error) { + resp, err := client.Get(ctx, trailReviewStatePath(trailID, reviewID)) + if err != nil { + return api.TrailReviewStateResponse{}, fmt.Errorf("get review state: %w", err) + } + defer resp.Body.Close() + if err := checkTrailResponse(resp); err != nil { + return api.TrailReviewStateResponse{}, err + } + var out api.TrailReviewStateResponse + if err := api.DecodeJSON(resp, &out); err != nil { + return api.TrailReviewStateResponse{}, fmt.Errorf("decode review state: %w", err) + } + return out, nil +} + +func trailReviewStatePath(trailID, reviewID string) string { + q := url.Values{} + q.Set("include_dismissed", "true") + q.Set("stale", trailReviewStaleAny) + q.Set("limit", strconv.Itoa(defaultTrailReviewLimit)) + return "/api/v1/trails/" + url.PathEscape(trailID) + "/reviews/" + url.PathEscape(reviewID) + "?" + q.Encode() +} + +func verifyTrailReviewHead(ctx context.Context, state api.TrailReviewStateResponse) error { + want := strings.TrimSpace(optionalStringValue(state.CodeVersion.HeadSHA)) + if want == "" { + return nil + } + got, err := resolveGitRev(ctx, "HEAD") + if err != nil { + return err + } + if got != want { + return fmt.Errorf("review was created for head %s, but current HEAD is %s; check out the reviewed commit or start a new review", abbreviateMaybe(want, 12), abbreviateMaybe(got, 12)) + } + return nil +} + +func applyTrailReviewSuggestions(ctx context.Context, comment api.TrailReviewComment, checkOnly bool, w io.Writer) (int, error) { + if len(comment.SuggestedChanges) == 0 { + return 0, fmt.Errorf("comment %s has no suggested changes", comment.ID) + } + applied := 0 + for _, change := range comment.SuggestedChanges { + patch := strings.TrimSpace(stringPtrValue(change.Patch)) + if change.ChangeType != "unified_diff" || patch == "" { + fmt.Fprintf(w, "Skipping suggested change %s (%s): only unified_diff patches are supported.\n", change.ID, change.ChangeType) + continue + } + if err := runGitApply(ctx, patch, true); err != nil { + return applied, fmt.Errorf("suggested change %s does not apply cleanly: %w", change.ID, err) + } + if !checkOnly { + if err := runGitApply(ctx, patch, false); err != nil { + return applied, fmt.Errorf("apply suggested change %s: %w", change.ID, err) + } + } + applied++ + } + if applied == 0 { + return 0, fmt.Errorf("comment %s has no supported unified_diff suggested changes", comment.ID) + } + return applied, nil +} + +func runGitApply(ctx context.Context, patch string, checkOnly bool) error { + root, err := paths.WorktreeRoot(ctx) + if err != nil { + return fmt.Errorf("resolve worktree root: %w", err) + } + args := []string{"-C", root, "apply"} + if checkOnly { + args = append(args, "--check") + } + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Stdin = bytes.NewBufferString(patch + "\n") + out, err := cmd.CombinedOutput() + if err != nil { + msg := strings.TrimSpace(string(out)) + if msg != "" { + return fmt.Errorf("git apply: %w: %s", err, msg) + } + return fmt.Errorf("git apply: %w", err) + } + return nil +} + +func patchTrailReviewCommentStatus(ctx context.Context, client *api.Client, trailID string, comment api.TrailReviewComment, status, reason string) (api.TrailReviewComment, error) { + body := api.TrailReviewCommentPatchRequest{Status: status} + if strings.TrimSpace(reason) != "" { + body.StatusReason = stringPtr(reason) + } + resp, err := client.Patch(ctx, trailReviewCommentPath(trailID, comment.ReviewID, comment.ID), body) + if err != nil { + return api.TrailReviewComment{}, fmt.Errorf("update review comment: %w", err) + } + defer resp.Body.Close() + if err := checkTrailResponse(resp); err != nil { + return api.TrailReviewComment{}, err + } + var updated api.TrailReviewComment + if err := api.DecodeJSON(resp, &updated); err != nil { + return api.TrailReviewComment{}, fmt.Errorf("decode updated review comment: %w", err) + } + return updated, nil +} + +func trailReviewCommentPath(trailID, reviewID, commentID string) string { + return "/api/v1/trails/" + url.PathEscape(trailID) + "/reviews/" + url.PathEscape(reviewID) + "/comments/" + url.PathEscape(commentID) +} + +func startTrailReview(ctx context.Context, client *api.Client, trailID string, request api.TrailReviewStartRequest, idempotencyKey string) (api.TrailReviewStartResponse, error) { + headers := http.Header{} + if idempotencyKey != "" { + headers.Set("Idempotency-Key", idempotencyKey) + } + resp, err := client.PostWithHeaders(ctx, trailReviewStartPath(trailID), request, headers) + if err != nil { + return api.TrailReviewStartResponse{}, fmt.Errorf("start review: %w", err) + } + defer resp.Body.Close() + if err := checkTrailResponse(resp); err != nil { + return api.TrailReviewStartResponse{}, err + } + var out api.TrailReviewStartResponse + if err := api.DecodeJSON(resp, &out); err != nil { + return api.TrailReviewStartResponse{}, fmt.Errorf("decode review start response: %w", err) + } + return out, nil +} + +func trailReviewStartPath(trailID string) string { + return "/api/v1/trails/" + url.PathEscape(trailID) + "/reviews" +} + +func buildTrailReviewStartRequest(ctx context.Context, target trailReviewTarget, opts trailReviewStartOptions) (api.TrailReviewStartRequest, error) { + baseRef := strings.TrimSpace(opts.BaseRef) + if baseRef == "" { + baseRef = target.Trail.Base + } + headRef := strings.TrimSpace(opts.HeadRef) + if headRef == "" { + if branch, err := GetCurrentBranch(ctx); err == nil { + headRef = branch + } + } + headSHA := strings.TrimSpace(opts.HeadSHA) + if headSHA == "" { + sha, err := resolveGitRev(ctx, "HEAD") + if err != nil { + return api.TrailReviewStartRequest{}, err + } + headSHA = sha + } + baseSHA := strings.TrimSpace(opts.BaseSHA) + if baseSHA == "" && baseRef != "" { + if sha, err := resolveGitRev(ctx, baseRef); err == nil { + baseSHA = sha + } + } + return api.TrailReviewStartRequest{ + HeadSHA: optionalStringPtr(headSHA), + BaseSHA: optionalStringPtr(baseSHA), + BaseRef: optionalStringPtr(baseRef), + HeadRef: optionalStringPtr(headRef), + }, nil +} + +func resolveGitRev(ctx context.Context, ref string) (string, error) { + root, err := paths.WorktreeRoot(ctx) + if err != nil { + return "", fmt.Errorf("resolve worktree root: %w", err) + } + cmd := exec.CommandContext(ctx, "git", "-C", root, "rev-parse", ref) + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("git rev-parse %s: %w", ref, err) + } + sha := strings.TrimSpace(string(out)) + if sha == "" { + return "", fmt.Errorf("git rev-parse %s: empty output", ref) + } + return sha, nil +} + +func trailReviewStartIdempotencyKey(trailID string, request api.TrailReviewStartRequest) string { + parts := []string{"trail-review-start", trailID, optionalStringValue(request.BaseSHA), optionalStringValue(request.HeadSHA), optionalStringValue(request.BaseRef), optionalStringValue(request.HeadRef)} + return strings.Join(parts, ":") +} + +func submitTrailReview(ctx context.Context, client *api.Client, target trailReviewTarget, event, body string) (api.TrailSubmitReviewResponse, error) { + path := trailsBasePath(target.Host, target.Owner, target.Repo) + "/" + strconv.Itoa(target.Trail.Number) + "/review" + resp, err := client.Post(ctx, path, api.TrailSubmitReviewRequest{Event: event, Body: body}) + if err != nil { + return api.TrailSubmitReviewResponse{}, fmt.Errorf("submit trail review: %w", err) + } + defer resp.Body.Close() + if err := checkTrailResponse(resp); err != nil { + return api.TrailSubmitReviewResponse{}, err + } + var out api.TrailSubmitReviewResponse + if err := api.DecodeJSON(resp, &out); err != nil { + return api.TrailSubmitReviewResponse{}, fmt.Errorf("decode trail review response: %w", err) + } + return out, nil +} + +func encodeTrailReviewJSON(w io.Writer, target trailReviewTarget, comments []api.TrailReviewComment, hasMore bool) error { + enc := json.NewEncoder(w) + enc.SetIndent("", " ") + return enc.Encode(map[string]any{ + "trail": target.Trail, + "comments": comments, + "has_more": hasMore, + }) +} + +func printTrailReviewDashboard(w io.Writer, target trailReviewTarget, comments []api.TrailReviewComment, hasMore bool, opts trailReviewListOptions) { + trail := target.Trail + fmt.Fprintf(w, "Trail #%d %s\n", trail.Number, trail.Title) + fmt.Fprintf(w, "Status: %s Branch: %s Base: %s\n", trail.Status, trail.Branch, trail.Base) + fmt.Fprintf(w, "ID: %s\n\n", trail.ID) + + counts := countTrailReviewComments(comments) + fmt.Fprintf(w, "Open findings: %d high %d medium %d low %d\n", counts.Open, counts.OpenHigh, counts.OpenMedium, counts.OpenLow) + fmt.Fprintf(w, "Resolved: %d Dismissed: %d Stale: %d\n", counts.Resolved, counts.Dismissed, counts.Stale) + if hasMore { + fmt.Fprintf(w, "Showing first %d findings; rerun with --offset for more.\n", opts.Limit) + } + fmt.Fprintln(w) + + if len(comments) == 0 { + fmt.Fprintln(w, "No review findings found.") + return + } + + for _, severity := range []string{"high", "medium", "low", ""} { + group := filterCommentsBySeverity(comments, severity) + if len(group) == 0 { + continue + } + title := severityTitle(severity) + fmt.Fprintln(w, title) + for _, comment := range group { + fmt.Fprintf(w, " %s %s %s %s\n", severityInitial(comment.Severity), trailReviewLocationDisplay(comment.Location), abbreviateMaybe(comment.ID, 12), trailReviewCommentTitle(comment)) + } + fmt.Fprintln(w) + } + + fmt.Fprintln(w, "Actions:") + fmt.Fprintln(w, " entire trail review show ") + fmt.Fprintln(w, " entire trail review apply --resolve") + fmt.Fprintln(w, " entire trail review resolve -m \"fixed in \"") + fmt.Fprintln(w, " entire trail review dismiss -m \"not applicable\"") + fmt.Fprintln(w, " entire trail review watch") +} + +func printTrailReviewComments(w io.Writer, comments []api.TrailReviewComment, hasMore bool) { + if len(comments) == 0 { + fmt.Fprintln(w, "No review findings found.") + return + } + tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) + fmt.Fprintln(tw, "ID\tSEV\tSTATUS\tLOCATION\tTITLE") + for _, comment := range comments { + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\n", abbreviateMaybe(comment.ID, 12), severityDisplay(comment.Severity), comment.Status, trailReviewLocationDisplay(comment.Location), trailReviewCommentTitle(comment)) + } + _ = tw.Flush() + if hasMore { + fmt.Fprintln(w, "More findings available; rerun with --offset for the next page.") + } +} + +func printTrailReviewCommentDetail(w io.Writer, comment api.TrailReviewComment) { + fmt.Fprintf(w, "Comment: %s\n", comment.ID) + fmt.Fprintf(w, "Review: %s\n", comment.ReviewID) + fmt.Fprintf(w, "Status: %s\n", comment.Status) + fmt.Fprintf(w, "Severity: %s\n", severityDisplay(comment.Severity)) + if comment.Confidence != nil { + fmt.Fprintf(w, "Confidence: %.2f\n", *comment.Confidence) + } + fmt.Fprintf(w, "Location: %s\n", trailReviewLocationDisplay(comment.Location)) + if title := trailReviewCommentTitle(comment); title != "" { + fmt.Fprintf(w, "Title: %s\n", title) + } + if body := stringPtrValue(comment.Body); body != "" { + fmt.Fprintf(w, "\n%s\n", strings.TrimSpace(body)) + } + if len(comment.SuggestedChanges) > 0 { + fmt.Fprintln(w, "\nSuggested changes:") + for _, change := range comment.SuggestedChanges { + fmt.Fprintf(w, "- %s (%s)\n", change.ID, change.ChangeType) + if change.ExpectedFilePath != nil && *change.ExpectedFilePath != "" { + fmt.Fprintf(w, " file: %s\n", *change.ExpectedFilePath) + } + if patch := stringPtrValue(change.Patch); patch != "" { + fmt.Fprintf(w, " patch:\n%s\n", strings.TrimSpace(patch)) + } + if instruction := stringPtrValue(change.Instruction); instruction != "" { + fmt.Fprintf(w, " instruction: %s\n", instruction) + } + } + } +} + +func printTrailReviewStart(w io.Writer, target trailReviewTarget, started api.TrailReviewStartResponse) { + fmt.Fprintf(w, "Started review %s for trail #%d (%s)\n", started.ReviewID, target.Trail.Number, target.Trail.Title) + fmt.Fprintf(w, " Code version: %s\n", started.CodeVersionID) + if started.BaseSHA != nil || started.HeadSHA != nil { + fmt.Fprintf(w, " Base: %s\n", abbreviateMaybe(optionalStringValue(started.BaseSHA), 12)) + fmt.Fprintf(w, " Head: %s\n", abbreviateMaybe(optionalStringValue(started.HeadSHA), 12)) + } + fmt.Fprintln(w) + fmt.Fprintln(w, "Next:") + fmt.Fprintf(w, " entire trail review watch %d\n", target.Trail.Number) + fmt.Fprintf(w, " entire trail review comments %d\n", target.Trail.Number) +} + +type trailReviewCommentCounts struct { + Open int + OpenHigh int + OpenMedium int + OpenLow int + Resolved int + Dismissed int + Stale int +} + +func countTrailReviewComments(comments []api.TrailReviewComment) trailReviewCommentCounts { + var counts trailReviewCommentCounts + for _, comment := range comments { + switch comment.Status { + case trailReviewStatusResolved: + counts.Resolved++ + case trailReviewStatusDismissed: + counts.Dismissed++ + case trailReviewStatusOpen: + counts.Open++ + switch strings.ToLower(stringPtrValue(comment.Severity)) { + case "high": + counts.OpenHigh++ + case "medium": + counts.OpenMedium++ + case "low": + counts.OpenLow++ + } + } + if comment.StaleOutcome == "stale" { + counts.Stale++ + } + } + return counts +} + +func filterCommentsBySeverity(comments []api.TrailReviewComment, severity string) []api.TrailReviewComment { + var out []api.TrailReviewComment + for _, comment := range comments { + got := strings.ToLower(stringPtrValue(comment.Severity)) + if severity == "" { + if got != "high" && got != "medium" && got != "low" { + out = append(out, comment) + } + continue + } + if got == severity { + out = append(out, comment) + } + } + return out +} + +func trailReviewLocationDisplay(loc api.TrailReviewLocation) string { + file := stringPtrValue(loc.FilePath) + if file == "" { + return loc.Granularity + } + if loc.StartLine == nil { + return file + } + if loc.EndLine != nil && *loc.EndLine != *loc.StartLine { + return fmt.Sprintf("%s:%d-%d", file, *loc.StartLine, *loc.EndLine) + } + return fmt.Sprintf("%s:%d", file, *loc.StartLine) +} + +func trailReviewCommentTitle(comment api.TrailReviewComment) string { + if title := strings.TrimSpace(stringPtrValue(comment.Title)); title != "" { + return title + } + body := strings.TrimSpace(stringPtrValue(comment.Body)) + if body == "" { + return "(untitled finding)" + } + return truncateOneLine(body, 80) +} + +func severityTitle(severity string) string { + switch severity { + case "high": + return "High" + case "medium": + return "Medium" + case "low": + return "Low" + default: + return "Other" + } +} + +func severityDisplay(severity *string) string { + if severity == nil || strings.TrimSpace(*severity) == "" { + return "-" + } + return *severity +} + +func severityInitial(severity *string) string { + switch strings.ToLower(stringPtrValue(severity)) { + case "high": + return "H" + case "medium": + return "M" + case "low": + return "L" + default: + return "-" + } +} + +func defaultTrailReviewStatusReason(status string) string { + switch status { + case trailReviewStatusResolved: + return "Resolved via Entire CLI" + case trailReviewStatusDismissed: + return "Dismissed via Entire CLI" + case trailReviewStatusOpen: + return "Reopened via Entire CLI" + default: + return "Updated via Entire CLI" + } +} + +func parseOptionalTrailNumber(args []string) (int, error) { + if len(args) == 0 { + return 0, nil + } + n, err := strconv.Atoi(args[0]) + if err != nil || n <= 0 { + return 0, fmt.Errorf("invalid trail number %q", args[0]) + } + return n, nil +} + +func parseTrailNumberAndCommentID(args []string) (int, string, error) { + if len(args) == 1 { + return 0, args[0], nil + } + n, err := parseOptionalTrailNumber(args[:1]) + if err != nil { + return 0, "", err + } + return n, args[1], nil +} + +func optionalStringPtr(s string) *string { + if strings.TrimSpace(s) == "" { + return nil + } + return &s +} + +func stringPtr(s string) *string { + return &s +} + +func stringPtrValue(s *string) string { + if s == nil { + return "" + } + return *s +} + +func optionalStringValue(s *string) string { + if s == nil { + return "" + } + return *s +} + +func abbreviateMaybe(s string, n int) string { + if len(s) <= n || n <= 0 { + return s + } + return s[:n] +} + +func truncateOneLine(s string, maxRunes int) string { + s = strings.ReplaceAll(s, "\r\n", " ") + s = strings.ReplaceAll(s, "\n", " ") + s = strings.Join(strings.Fields(s), " ") + runes := []rune(s) + if len(runes) <= maxRunes { + return s + } + return string(runes[:maxRunes]) + "…" +} diff --git a/cmd/entire/cli/trail_review_cmd_test.go b/cmd/entire/cli/trail_review_cmd_test.go new file mode 100644 index 000000000..0e0e3b770 --- /dev/null +++ b/cmd/entire/cli/trail_review_cmd_test.go @@ -0,0 +1,154 @@ +package cli + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/api" +) + +func TestTrailReviewCommentsPath(t *testing.T) { + got := trailReviewCommentsPath("trail id/with slash", trailReviewListOptions{ + Status: "open,resolved", + Severity: "high,medium", + Stale: "any", + IncludeDismissed: true, + Limit: 25, + Offset: 50, + }) + want := "/api/v1/trails/trail%20id%2Fwith%20slash/reviews/comments?include_dismissed=true&limit=25&offset=50&severity=high%2Cmedium&status=open%2Cresolved" + if got != want { + t.Fatalf("trailReviewCommentsPath = %q, want %q", got, want) + } +} + +func TestPrintTrailReviewDashboard(t *testing.T) { + high := "high" + medium := "medium" + path := "src/auth/session.ts" + line := 88 + comments := []api.TrailReviewComment{ + { + ID: "comment-high-123", + ReviewID: "review-1", + Title: trailReviewStrPtr("Missing expiry skew handling"), + Severity: &high, + Status: trailReviewStatusOpen, + Location: api.TrailReviewLocation{ + Granularity: "line", + FilePath: &path, + StartLine: &line, + }, + }, + { + ID: "comment-medium-123", + ReviewID: "review-1", + Title: trailReviewStrPtr("Retry loop can spin forever"), + Severity: &medium, + Status: trailReviewStatusResolved, + Location: api.TrailReviewLocation{Granularity: "whole_change"}, + }, + } + var out strings.Builder + printTrailReviewDashboard(&out, trailReviewTarget{Trail: api.TrailResource{ + ID: "trl_1", + Number: 42, + Title: "Add token refresh", + Status: "in_review", + Branch: "feat/token-refresh", + Base: "main", + }}, comments, false, defaultTrailReviewListOptions()) + text := out.String() + for _, want := range []string{ + "Trail #42 Add token refresh", + "Open findings: 1 high 1 medium 0 low 0", + "Resolved: 1", + "High", + "src/auth/session.ts:88", + "Missing expiry skew handling", + "Actions:", + } { + if !strings.Contains(text, want) { + t.Fatalf("dashboard missing %q:\n%s", want, text) + } + } +} + +func TestFetchTrailReviewCommentsAndPatchStatus(t *testing.T) { + var gotPatchBody api.TrailReviewCommentPatchRequest + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/trails/trl_1/reviews/comments": + if got := r.URL.Query().Get("status"); got != "open" { + t.Fatalf("status query = %q, want open", got) + } + _ = json.NewEncoder(w).Encode(api.TrailReviewCommentsResponse{Comments: []api.TrailReviewComment{ + {ID: "cmt_1", TrailID: "trl_1", ReviewID: "rvw_1", Status: trailReviewStatusOpen, Location: api.TrailReviewLocation{Granularity: "whole_change"}}, + }}) + case r.Method == http.MethodPatch && r.URL.Path == "/api/v1/trails/trl_1/reviews/rvw_1/comments/cmt_1": + if err := json.NewDecoder(r.Body).Decode(&gotPatchBody); err != nil { + t.Fatalf("decode patch body: %v", err) + } + _ = json.NewEncoder(w).Encode(api.TrailReviewComment{ID: "cmt_1", TrailID: "trl_1", ReviewID: "rvw_1", Status: trailReviewStatusResolved}) + default: + t.Fatalf("unexpected request %s %s", r.Method, r.URL.String()) + } + })) + defer srv.Close() + t.Setenv(api.BaseURLEnvVar, srv.URL) + client := api.NewClient("tok") + + comments, hasMore, err := fetchTrailReviewComments(context.Background(), client, "trl_1", defaultTrailReviewListOptions()) + if err != nil { + t.Fatalf("fetchTrailReviewComments: %v", err) + } + if hasMore || len(comments) != 1 || comments[0].ID != "cmt_1" { + t.Fatalf("comments = %#v, hasMore=%v", comments, hasMore) + } + updated, err := patchTrailReviewCommentStatus(context.Background(), client, "trl_1", comments[0], trailReviewStatusResolved, "fixed") + if err != nil { + t.Fatalf("patchTrailReviewCommentStatus: %v", err) + } + if updated.Status != trailReviewStatusResolved { + t.Fatalf("updated status = %q", updated.Status) + } + if gotPatchBody.Status != trailReviewStatusResolved || gotPatchBody.StatusReason == nil || *gotPatchBody.StatusReason != "fixed" { + t.Fatalf("patch body = %#v", gotPatchBody) + } +} + +func TestStartTrailReviewSendsIdempotencyKey(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.Path != "/api/v1/trails/trl_1/reviews" { + t.Fatalf("unexpected request %s %s", r.Method, r.URL.String()) + } + if got := r.Header.Get("Idempotency-Key"); got != "key-1" { + t.Fatalf("Idempotency-Key = %q, want key-1", got) + } + var body api.TrailReviewStartRequest + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decode body: %v", err) + } + if body.HeadSHA == nil || *body.HeadSHA != "abc" { + t.Fatalf("HeadSHA = %#v", body.HeadSHA) + } + _ = json.NewEncoder(w).Encode(api.TrailReviewStartResponse{ReviewID: "rvw_1", TrailID: "trl_1", CodeVersionID: "cv_1"}) + })) + defer srv.Close() + t.Setenv(api.BaseURLEnvVar, srv.URL) + client := api.NewClient("tok") + + started, err := startTrailReview(context.Background(), client, "trl_1", api.TrailReviewStartRequest{HeadSHA: trailReviewStrPtr("abc")}, "key-1") + if err != nil { + t.Fatalf("startTrailReview: %v", err) + } + if started.ReviewID != "rvw_1" { + t.Fatalf("ReviewID = %q", started.ReviewID) + } +} + +func trailReviewStrPtr(s string) *string { return &s } diff --git a/cmd/entire/cli/trail_watch_cmd.go b/cmd/entire/cli/trail_watch_cmd.go index 55e8acd4e..1c36a0f55 100644 --- a/cmd/entire/cli/trail_watch_cmd.go +++ b/cmd/entire/cli/trail_watch_cmd.go @@ -85,6 +85,10 @@ Events emitted by the server: } func runTrailWatch(cmd *cobra.Command, number int, jsonOutput, showPings, once bool) error { + return runTrailWatchWithOptions(cmd, number, jsonOutput, showPings, once) +} + +func runTrailWatchWithOptions(cmd *cobra.Command, number int, jsonOutput, showPings, once bool) error { ctx := cmd.Context() w := cmd.OutOrStdout() errW := cmd.ErrOrStderr() From 6f7133924072ecd5a96bffeec7374ded8f890e37 Mon Sep 17 00:00:00 2001 From: dip Date: Tue, 26 May 2026 13:21:18 +0200 Subject: [PATCH 2/6] fix: address trail review feedback Entire-Checkpoint: 012879bddbd5 --- cmd/entire/cli/trail_review_cmd.go | 105 +++++++++++++++++++----- cmd/entire/cli/trail_review_cmd_test.go | 73 +++++++++++++++- 2 files changed, 157 insertions(+), 21 deletions(-) diff --git a/cmd/entire/cli/trail_review_cmd.go b/cmd/entire/cli/trail_review_cmd.go index 747eae610..b0c5bd2e6 100644 --- a/cmd/entire/cli/trail_review_cmd.go +++ b/cmd/entire/cli/trail_review_cmd.go @@ -261,10 +261,15 @@ func runTrailReviewDashboard(cmd *cobra.Command, number int, opts trailReviewLis if err != nil { return err } + summaryComments, err := fetchAllTrailReviewComments(cmd.Context(), client, target.Trail.ID, trailReviewSummaryOptions()) + if err != nil { + return err + } + counts := countTrailReviewComments(summaryComments) if opts.JSON { - return encodeTrailReviewJSON(cmd.OutOrStdout(), target, comments, hasMore) + return encodeTrailReviewJSON(cmd.OutOrStdout(), target, comments, hasMore, counts) } - printTrailReviewDashboard(cmd.OutOrStdout(), target, comments, hasMore, opts) + printTrailReviewDashboard(cmd.OutOrStdout(), target, comments, hasMore, opts, counts) return nil } @@ -278,7 +283,7 @@ func runTrailReviewComments(cmd *cobra.Command, number int, opts trailReviewList return err } if opts.JSON { - return encodeTrailReviewJSON(cmd.OutOrStdout(), target, comments, hasMore) + return encodeTrailReviewJSON(cmd.OutOrStdout(), target, comments, hasMore, countTrailReviewComments(comments)) } printTrailReviewComments(cmd.OutOrStdout(), comments, hasMore) return nil @@ -470,6 +475,34 @@ func fetchTrailReviewComments(ctx context.Context, client *api.Client, trailID s return out.Comments, out.HasMore, nil } +func fetchAllTrailReviewComments(ctx context.Context, client *api.Client, trailID string, opts trailReviewListOptions) ([]api.TrailReviewComment, error) { + if opts.Limit <= 0 { + opts.Limit = defaultTrailReviewLimit + } + var all []api.TrailReviewComment + for { + comments, hasMore, err := fetchTrailReviewComments(ctx, client, trailID, opts) + if err != nil { + return nil, err + } + all = append(all, comments...) + if !hasMore { + break + } + opts.Offset += opts.Limit + } + return all, nil +} + +func trailReviewSummaryOptions() trailReviewListOptions { + return trailReviewListOptions{ + Status: trailReviewStatusAny, + Stale: trailReviewStaleAny, + IncludeDismissed: true, + Limit: defaultTrailReviewLimit, + } +} + func trailReviewCommentsPath(trailID string, opts trailReviewListOptions) string { q := url.Values{} if opts.Status != "" && opts.Status != trailReviewStatusAny { @@ -557,26 +590,58 @@ func hydrateTrailReviewCommentWithState(ctx context.Context, client *api.Client, } func fetchTrailReviewState(ctx context.Context, client *api.Client, trailID, reviewID string) (api.TrailReviewStateResponse, error) { - resp, err := client.Get(ctx, trailReviewStatePath(trailID, reviewID)) - if err != nil { - return api.TrailReviewStateResponse{}, fmt.Errorf("get review state: %w", err) - } - defer resp.Body.Close() - if err := checkTrailResponse(resp); err != nil { - return api.TrailReviewStateResponse{}, err - } - var out api.TrailReviewStateResponse - if err := api.DecodeJSON(resp, &out); err != nil { - return api.TrailReviewStateResponse{}, fmt.Errorf("decode review state: %w", err) + var merged api.TrailReviewStateResponse + cursor := "" + seenCursors := map[string]bool{} + for { + resp, err := client.Get(ctx, trailReviewStatePath(trailID, reviewID, cursor)) + if err != nil { + return api.TrailReviewStateResponse{}, fmt.Errorf("get review state: %w", err) + } + var page api.TrailReviewStateResponse + decodeErr := func() error { + defer resp.Body.Close() + if err := checkTrailResponse(resp); err != nil { + return err + } + if err := api.DecodeJSON(resp, &page); err != nil { + return fmt.Errorf("decode review state: %w", err) + } + return nil + }() + if decodeErr != nil { + return api.TrailReviewStateResponse{}, decodeErr + } + + if cursor == "" { + merged = page + } else { + merged.Comments = append(merged.Comments, page.Comments...) + merged.NextCursor = page.NextCursor + merged.EventCursor = page.EventCursor + } + + if page.NextCursor == nil || strings.TrimSpace(*page.NextCursor) == "" { + merged.NextCursor = nil + break + } + cursor = strings.TrimSpace(*page.NextCursor) + if seenCursors[cursor] { + return api.TrailReviewStateResponse{}, fmt.Errorf("review state pagination repeated cursor %q", cursor) + } + seenCursors[cursor] = true } - return out, nil + return merged, nil } -func trailReviewStatePath(trailID, reviewID string) string { +func trailReviewStatePath(trailID, reviewID, cursor string) string { q := url.Values{} q.Set("include_dismissed", "true") q.Set("stale", trailReviewStaleAny) q.Set("limit", strconv.Itoa(defaultTrailReviewLimit)) + if strings.TrimSpace(cursor) != "" { + q.Set("cursor", strings.TrimSpace(cursor)) + } return "/api/v1/trails/" + url.PathEscape(trailID) + "/reviews/" + url.PathEscape(reviewID) + "?" + q.Encode() } @@ -764,23 +829,23 @@ func submitTrailReview(ctx context.Context, client *api.Client, target trailRevi return out, nil } -func encodeTrailReviewJSON(w io.Writer, target trailReviewTarget, comments []api.TrailReviewComment, hasMore bool) error { +func encodeTrailReviewJSON(w io.Writer, target trailReviewTarget, comments []api.TrailReviewComment, hasMore bool, counts trailReviewCommentCounts) error { enc := json.NewEncoder(w) enc.SetIndent("", " ") return enc.Encode(map[string]any{ "trail": target.Trail, + "counts": counts, "comments": comments, "has_more": hasMore, }) } -func printTrailReviewDashboard(w io.Writer, target trailReviewTarget, comments []api.TrailReviewComment, hasMore bool, opts trailReviewListOptions) { +func printTrailReviewDashboard(w io.Writer, target trailReviewTarget, comments []api.TrailReviewComment, hasMore bool, opts trailReviewListOptions, counts trailReviewCommentCounts) { trail := target.Trail fmt.Fprintf(w, "Trail #%d %s\n", trail.Number, trail.Title) fmt.Fprintf(w, "Status: %s Branch: %s Base: %s\n", trail.Status, trail.Branch, trail.Base) fmt.Fprintf(w, "ID: %s\n\n", trail.ID) - counts := countTrailReviewComments(comments) fmt.Fprintf(w, "Open findings: %d high %d medium %d low %d\n", counts.Open, counts.OpenHigh, counts.OpenMedium, counts.OpenLow) fmt.Fprintf(w, "Resolved: %d Dismissed: %d Stale: %d\n", counts.Resolved, counts.Dismissed, counts.Stale) if hasMore { @@ -789,7 +854,7 @@ func printTrailReviewDashboard(w io.Writer, target trailReviewTarget, comments [ fmt.Fprintln(w) if len(comments) == 0 { - fmt.Fprintln(w, "No review findings found.") + fmt.Fprintln(w, "No review findings match the current filters.") return } diff --git a/cmd/entire/cli/trail_review_cmd_test.go b/cmd/entire/cli/trail_review_cmd_test.go index 0e0e3b770..b4394b875 100644 --- a/cmd/entire/cli/trail_review_cmd_test.go +++ b/cmd/entire/cli/trail_review_cmd_test.go @@ -61,7 +61,7 @@ func TestPrintTrailReviewDashboard(t *testing.T) { Status: "in_review", Branch: "feat/token-refresh", Base: "main", - }}, comments, false, defaultTrailReviewListOptions()) + }}, comments, false, defaultTrailReviewListOptions(), countTrailReviewComments(comments)) text := out.String() for _, want := range []string{ "Trail #42 Add token refresh", @@ -78,6 +78,32 @@ func TestPrintTrailReviewDashboard(t *testing.T) { } } +func TestPrintTrailReviewDashboard_UsesSeparateCountsWhenFilteredCommentsEmpty(t *testing.T) { + var out strings.Builder + counts := countTrailReviewComments([]api.TrailReviewComment{ + {ID: "resolved-1", Status: trailReviewStatusResolved}, + {ID: "dismissed-1", Status: trailReviewStatusDismissed, StaleOutcome: "stale"}, + }) + printTrailReviewDashboard(&out, trailReviewTarget{Trail: api.TrailResource{ + ID: "trl_1", + Number: 42, + Title: "Add token refresh", + Status: "in_review", + Branch: "feat/token-refresh", + Base: "main", + }}, nil, false, defaultTrailReviewListOptions(), counts) + text := out.String() + for _, want := range []string{ + "Open findings: 0 high 0 medium 0 low 0", + "Resolved: 1 Dismissed: 1 Stale: 1", + "No review findings match the current filters.", + } { + if !strings.Contains(text, want) { + t.Fatalf("dashboard missing %q:\n%s", want, text) + } + } +} + func TestFetchTrailReviewCommentsAndPatchStatus(t *testing.T) { var gotPatchBody api.TrailReviewCommentPatchRequest srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -121,6 +147,51 @@ func TestFetchTrailReviewCommentsAndPatchStatus(t *testing.T) { } } +func TestFetchTrailReviewStateFollowsCursor(t *testing.T) { + requests := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != "/api/v1/trails/trl_1/reviews/rvw_1" { + t.Fatalf("unexpected request %s %s", r.Method, r.URL.String()) + } + requests++ + switch r.URL.Query().Get("cursor") { + case "": + next := "cursor-2" + _ = json.NewEncoder(w).Encode(api.TrailReviewStateResponse{ + Review: api.TrailReview{ID: "rvw_1"}, + CodeVersion: api.TrailReviewCodeVersion{ID: "cv_1"}, + Comments: []api.TrailReviewComment{{ID: "cmt_1"}}, + NextCursor: &next, + }) + case "cursor-2": + _ = json.NewEncoder(w).Encode(api.TrailReviewStateResponse{ + Review: api.TrailReview{ID: "rvw_1"}, + CodeVersion: api.TrailReviewCodeVersion{ID: "cv_1"}, + Comments: []api.TrailReviewComment{{ID: "cmt_2"}}, + }) + default: + t.Fatalf("unexpected cursor %q", r.URL.Query().Get("cursor")) + } + })) + defer srv.Close() + t.Setenv(api.BaseURLEnvVar, srv.URL) + client := api.NewClient("tok") + + state, err := fetchTrailReviewState(context.Background(), client, "trl_1", "rvw_1") + if err != nil { + t.Fatalf("fetchTrailReviewState: %v", err) + } + if requests != 2 { + t.Fatalf("requests = %d, want 2", requests) + } + if len(state.Comments) != 2 || state.Comments[0].ID != "cmt_1" || state.Comments[1].ID != "cmt_2" { + t.Fatalf("comments = %#v", state.Comments) + } + if state.NextCursor != nil { + t.Fatalf("NextCursor = %#v, want nil after final page", state.NextCursor) + } +} + func TestStartTrailReviewSendsIdempotencyKey(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost || r.URL.Path != "/api/v1/trails/trl_1/reviews" { From 7f4593ff48c35f91ed4923681ed44a601b977df6 Mon Sep 17 00:00:00 2001 From: dip Date: Tue, 26 May 2026 21:44:37 +0200 Subject: [PATCH 3/6] fix: harden trail review patch apply Entire-Checkpoint: 58d44d708365 --- cmd/entire/cli/trail_review_cmd.go | 115 +++++++++++++++++-- cmd/entire/cli/trail_review_cmd_test.go | 143 ++++++++++++++++++++++++ 2 files changed, 248 insertions(+), 10 deletions(-) diff --git a/cmd/entire/cli/trail_review_cmd.go b/cmd/entire/cli/trail_review_cmd.go index b0c5bd2e6..341fb5945 100644 --- a/cmd/entire/cli/trail_review_cmd.go +++ b/cmd/entire/cli/trail_review_cmd.go @@ -1,6 +1,7 @@ package cli import ( + "bufio" "bytes" "context" "encoding/json" @@ -10,6 +11,7 @@ import ( "net/http" "net/url" "os/exec" + "path" "sort" "strconv" "strings" @@ -664,27 +666,120 @@ func applyTrailReviewSuggestions(ctx context.Context, comment api.TrailReviewCom if len(comment.SuggestedChanges) == 0 { return 0, fmt.Errorf("comment %s has no suggested changes", comment.ID) } - applied := 0 + combinedPatch, supported, err := combinedSafeUnifiedDiffPatch(comment, w) + if err != nil { + return 0, err + } + if supported == 0 { + return 0, fmt.Errorf("comment %s has no supported unified_diff suggested changes", comment.ID) + } + if err := runGitApply(ctx, combinedPatch, true); err != nil { + return 0, fmt.Errorf("suggested changes for comment %s do not apply cleanly: %w", comment.ID, err) + } + if checkOnly { + return supported, nil + } + if err := runGitApply(ctx, combinedPatch, false); err != nil { + return 0, fmt.Errorf("apply suggested changes for comment %s: %w", comment.ID, err) + } + return supported, nil +} + +func combinedSafeUnifiedDiffPatch(comment api.TrailReviewComment, w io.Writer) (string, int, error) { + var combined strings.Builder + supported := 0 for _, change := range comment.SuggestedChanges { patch := strings.TrimSpace(stringPtrValue(change.Patch)) if change.ChangeType != "unified_diff" || patch == "" { fmt.Fprintf(w, "Skipping suggested change %s (%s): only unified_diff patches are supported.\n", change.ID, change.ChangeType) continue } - if err := runGitApply(ctx, patch, true); err != nil { - return applied, fmt.Errorf("suggested change %s does not apply cleanly: %w", change.ID, err) + if err := validateUnifiedDiffPatchPaths(patch); err != nil { + return "", 0, fmt.Errorf("suggested change %s has unsafe patch path: %w", change.ID, err) } - if !checkOnly { - if err := runGitApply(ctx, patch, false); err != nil { - return applied, fmt.Errorf("apply suggested change %s: %w", change.ID, err) + if combined.Len() > 0 { + combined.WriteByte('\n') + } + combined.WriteString(patch) + combined.WriteByte('\n') + supported++ + } + return combined.String(), supported, nil +} + +func validateUnifiedDiffPatchPaths(patchText string) error { + scanner := bufio.NewScanner(strings.NewReader(patchText)) + scanner.Buffer(make([]byte, 0, 64*1024), 1<<20) + for scanner.Scan() { + line := scanner.Text() + for _, p := range patchHeaderPaths(line) { + if err := validatePatchPath(p); err != nil { + return err } } - applied++ } - if applied == 0 { - return 0, fmt.Errorf("comment %s has no supported unified_diff suggested changes", comment.ID) + if err := scanner.Err(); err != nil { + return fmt.Errorf("scan patch: %w", err) } - return applied, nil + return nil +} + +func patchHeaderPaths(line string) []string { + switch { + case strings.HasPrefix(line, "diff --git "): + fields := strings.Fields(strings.TrimPrefix(line, "diff --git ")) + if len(fields) >= 2 { + return []string{fields[0], fields[1]} + } + case strings.HasPrefix(line, "--- ") || strings.HasPrefix(line, "+++ "): + return []string{patchHeaderPath(line[4:])} + case strings.HasPrefix(line, "rename from "): + return []string{strings.TrimSpace(strings.TrimPrefix(line, "rename from "))} + case strings.HasPrefix(line, "rename to "): + return []string{strings.TrimSpace(strings.TrimPrefix(line, "rename to "))} + case strings.HasPrefix(line, "copy from "): + return []string{strings.TrimSpace(strings.TrimPrefix(line, "copy from "))} + case strings.HasPrefix(line, "copy to "): + return []string{strings.TrimSpace(strings.TrimPrefix(line, "copy to "))} + } + return nil +} + +func patchHeaderPath(raw string) string { + raw = strings.TrimSpace(raw) + if beforeTab, _, ok := strings.Cut(raw, "\t"); ok { + raw = beforeTab + } + return raw +} + +func validatePatchPath(raw string) error { + p := strings.TrimSpace(raw) + if p == "" || p == "/dev/null" { + return nil + } + if unquoted, err := strconv.Unquote(p); err == nil { + p = unquoted + } + p = strings.TrimPrefix(p, "a/") + p = strings.TrimPrefix(p, "b/") + p = strings.ReplaceAll(p, "\\", "/") + if strings.HasPrefix(p, "/") { + return fmt.Errorf("absolute path %q is not allowed", raw) + } + clean := path.Clean(p) + if clean == "." || clean == "" { + return nil + } + if clean == ".." || strings.HasPrefix(clean, "../") { + return fmt.Errorf("path %q escapes the repository", raw) + } + for _, part := range strings.Split(clean, "/") { + if part == ".git" { + return fmt.Errorf("path %q targets .git metadata", raw) + } + } + return nil } func runGitApply(ctx context.Context, patch string, checkOnly bool) error { diff --git a/cmd/entire/cli/trail_review_cmd_test.go b/cmd/entire/cli/trail_review_cmd_test.go index b4394b875..33cb2379c 100644 --- a/cmd/entire/cli/trail_review_cmd_test.go +++ b/cmd/entire/cli/trail_review_cmd_test.go @@ -3,12 +3,17 @@ package cli import ( "context" "encoding/json" + "io" "net/http" "net/http/httptest" + "os" + "os/exec" + "path/filepath" "strings" "testing" "github.com/entireio/cli/cmd/entire/cli/api" + "github.com/entireio/cli/cmd/entire/cli/paths" ) func TestTrailReviewCommentsPath(t *testing.T) { @@ -192,6 +197,144 @@ func TestFetchTrailReviewStateFollowsCursor(t *testing.T) { } } +func TestApplyTrailReviewSuggestions_AppliesUnifiedDiff(t *testing.T) { + repo := newTrailReviewApplyRepo(t) + writeTrailReviewApplyFile(t, repo, "file.txt", "hello\nold\n") + comment := trailReviewApplyComment(trailReviewPatch("file.txt", "old", "new")) + + applied, err := applyTrailReviewSuggestions(context.Background(), comment, false, io.Discard) + if err != nil { + t.Fatalf("applyTrailReviewSuggestions: %v", err) + } + if applied != 1 { + t.Fatalf("applied = %d, want 1", applied) + } + if got := readTrailReviewApplyFile(t, repo, "file.txt"); got != "hello\nnew\n" { + t.Fatalf("file content = %q", got) + } +} + +func TestApplyTrailReviewSuggestions_CheckDoesNotModifyWorktree(t *testing.T) { + repo := newTrailReviewApplyRepo(t) + writeTrailReviewApplyFile(t, repo, "file.txt", "hello\nold\n") + comment := trailReviewApplyComment(trailReviewPatch("file.txt", "old", "new")) + + applied, err := applyTrailReviewSuggestions(context.Background(), comment, true, io.Discard) + if err != nil { + t.Fatalf("applyTrailReviewSuggestions --check: %v", err) + } + if applied != 1 { + t.Fatalf("applied = %d, want 1", applied) + } + if got := readTrailReviewApplyFile(t, repo, "file.txt"); got != "hello\nold\n" { + t.Fatalf("file content = %q", got) + } +} + +func TestApplyTrailReviewSuggestions_FailureDoesNotPartiallyApply(t *testing.T) { + repo := newTrailReviewApplyRepo(t) + writeTrailReviewApplyFile(t, repo, "a.txt", "hello\nold\n") + writeTrailReviewApplyFile(t, repo, "b.txt", "hello\nold\n") + comment := trailReviewApplyComment( + trailReviewPatch("a.txt", "old", "new"), + trailReviewPatch("b.txt", "missing", "new"), + ) + + applied, err := applyTrailReviewSuggestions(context.Background(), comment, false, io.Discard) + if err == nil { + t.Fatal("applyTrailReviewSuggestions expected error") + } + if applied != 0 { + t.Fatalf("applied = %d, want 0", applied) + } + if got := readTrailReviewApplyFile(t, repo, "a.txt"); got != "hello\nold\n" { + t.Fatalf("a.txt content = %q", got) + } + if got := readTrailReviewApplyFile(t, repo, "b.txt"); got != "hello\nold\n" { + t.Fatalf("b.txt content = %q", got) + } +} + +func TestApplyTrailReviewSuggestions_RejectsGitMetadataPaths(t *testing.T) { + _ = newTrailReviewApplyRepo(t) + comment := trailReviewApplyComment(`diff --git a/.git/config b/.git/config +--- a/.git/config ++++ b/.git/config +@@ -1,1 +1,1 @@ +-old ++new +`) + + _, err := applyTrailReviewSuggestions(context.Background(), comment, false, io.Discard) + if err == nil { + t.Fatal("applyTrailReviewSuggestions expected unsafe path error") + } + if !strings.Contains(err.Error(), ".git") { + t.Fatalf("error = %v, want .git mention", err) + } +} + +func newTrailReviewApplyRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + runTrailReviewApplyGit(t, dir, "init") + paths.ClearWorktreeRootCache() + t.Chdir(dir) + t.Cleanup(paths.ClearWorktreeRootCache) + return dir +} + +func writeTrailReviewApplyFile(t *testing.T, repo, rel, content string) { + t.Helper() + path := filepath.Join(repo, rel) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", filepath.Dir(path), err) + } + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write %s: %v", rel, err) + } +} + +func readTrailReviewApplyFile(t *testing.T, repo, rel string) string { + t.Helper() + data, err := os.ReadFile(filepath.Join(repo, rel)) + if err != nil { + t.Fatalf("read %s: %v", rel, err) + } + return string(data) +} + +func runTrailReviewApplyGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } +} + +func trailReviewApplyComment(patches ...string) api.TrailReviewComment { + changes := make([]api.TrailReviewSuggestedChange, len(patches)) + for i, patch := range patches { + changes[i] = api.TrailReviewSuggestedChange{ + ID: "change-" + string(rune('a'+i)), + ChangeType: "unified_diff", + Patch: trailReviewStrPtr(patch), + } + } + return api.TrailReviewComment{ID: "cmt_1", SuggestedChanges: changes} +} + +func trailReviewPatch(file, oldText, newText string) string { + return "diff --git a/" + file + " b/" + file + "\n" + + "--- a/" + file + "\n" + + "+++ b/" + file + "\n" + + "@@ -1,2 +1,2 @@\n" + + " hello\n" + + "-" + oldText + "\n" + + "+" + newText + "\n" +} + func TestStartTrailReviewSendsIdempotencyKey(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost || r.URL.Path != "/api/v1/trails/trl_1/reviews" { From ddb88c67c7b0aadfa685c72c54d3a845e480a3f5 Mon Sep 17 00:00:00 2001 From: dip Date: Tue, 26 May 2026 22:03:24 +0200 Subject: [PATCH 4/6] fix: satisfy trail review lint Entire-Checkpoint: 1d403658033b --- cmd/entire/cli/trail_review_cmd.go | 53 ++++++++++++++---------- cmd/entire/cli/trail_review_cmd_test.go | 55 ++++++++++++++----------- 2 files changed, 63 insertions(+), 45 deletions(-) diff --git a/cmd/entire/cli/trail_review_cmd.go b/cmd/entire/cli/trail_review_cmd.go index 341fb5945..cb3a961f8 100644 --- a/cmd/entire/cli/trail_review_cmd.go +++ b/cmd/entire/cli/trail_review_cmd.go @@ -32,6 +32,9 @@ const ( trailReviewStatusOpen = "open" trailReviewStatusResolved = "resolved" trailReviewStatusDismissed = "dismissed" + trailReviewSeverityHigh = "high" + trailReviewSeverityMedium = "medium" + trailReviewSeverityLow = "low" ) type trailReviewListOptions struct { @@ -300,7 +303,9 @@ func runTrailReviewShow(cmd *cobra.Command, number int, commentID string) error if err != nil { return err } - comment, _ = hydrateTrailReviewCommentSuggestions(cmd.Context(), client, target.Trail.ID, comment) // best-effort detail enrichment + if hydrated, hydrateErr := hydrateTrailReviewCommentSuggestions(cmd.Context(), client, target.Trail.ID, comment); hydrateErr == nil { + comment = hydrated + } printTrailReviewCommentDetail(cmd.OutOrStdout(), comment) return nil } @@ -405,7 +410,7 @@ func runTrailReviewSubmit(cmd *cobra.Command, number int, event, body string) er if err != nil { return err } - fmt.Fprintf(cmd.OutOrStdout(), "%s submitted by %s on %s\n", resp.Review.Event, resp.Review.Author, abbreviateMaybe(resp.Review.CommitSHA, 12)) + fmt.Fprintf(cmd.OutOrStdout(), "%s submitted by %s on %s\n", resp.Review.Event, resp.Review.Author, abbreviate12(resp.Review.CommitSHA)) return nil } @@ -657,7 +662,7 @@ func verifyTrailReviewHead(ctx context.Context, state api.TrailReviewStateRespon return err } if got != want { - return fmt.Errorf("review was created for head %s, but current HEAD is %s; check out the reviewed commit or start a new review", abbreviateMaybe(want, 12), abbreviateMaybe(got, 12)) + return fmt.Errorf("review was created for head %s, but current HEAD is %s; check out the reviewed commit or start a new review", abbreviate12(want), abbreviate12(got)) } return nil } @@ -927,12 +932,15 @@ func submitTrailReview(ctx context.Context, client *api.Client, target trailRevi func encodeTrailReviewJSON(w io.Writer, target trailReviewTarget, comments []api.TrailReviewComment, hasMore bool, counts trailReviewCommentCounts) error { enc := json.NewEncoder(w) enc.SetIndent("", " ") - return enc.Encode(map[string]any{ + if err := enc.Encode(map[string]any{ "trail": target.Trail, "counts": counts, "comments": comments, "has_more": hasMore, - }) + }); err != nil { + return fmt.Errorf("encode trail review JSON: %w", err) + } + return nil } func printTrailReviewDashboard(w io.Writer, target trailReviewTarget, comments []api.TrailReviewComment, hasMore bool, opts trailReviewListOptions, counts trailReviewCommentCounts) { @@ -953,7 +961,7 @@ func printTrailReviewDashboard(w io.Writer, target trailReviewTarget, comments [ return } - for _, severity := range []string{"high", "medium", "low", ""} { + for _, severity := range []string{trailReviewSeverityHigh, trailReviewSeverityMedium, trailReviewSeverityLow, ""} { group := filterCommentsBySeverity(comments, severity) if len(group) == 0 { continue @@ -961,7 +969,7 @@ func printTrailReviewDashboard(w io.Writer, target trailReviewTarget, comments [ title := severityTitle(severity) fmt.Fprintln(w, title) for _, comment := range group { - fmt.Fprintf(w, " %s %s %s %s\n", severityInitial(comment.Severity), trailReviewLocationDisplay(comment.Location), abbreviateMaybe(comment.ID, 12), trailReviewCommentTitle(comment)) + fmt.Fprintf(w, " %s %s %s %s\n", severityInitial(comment.Severity), trailReviewLocationDisplay(comment.Location), abbreviate12(comment.ID), trailReviewCommentTitle(comment)) } fmt.Fprintln(w) } @@ -982,7 +990,7 @@ func printTrailReviewComments(w io.Writer, comments []api.TrailReviewComment, ha tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) fmt.Fprintln(tw, "ID\tSEV\tSTATUS\tLOCATION\tTITLE") for _, comment := range comments { - fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\n", abbreviateMaybe(comment.ID, 12), severityDisplay(comment.Severity), comment.Status, trailReviewLocationDisplay(comment.Location), trailReviewCommentTitle(comment)) + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\n", abbreviate12(comment.ID), severityDisplay(comment.Severity), comment.Status, trailReviewLocationDisplay(comment.Location), trailReviewCommentTitle(comment)) } _ = tw.Flush() if hasMore { @@ -1026,8 +1034,8 @@ func printTrailReviewStart(w io.Writer, target trailReviewTarget, started api.Tr fmt.Fprintf(w, "Started review %s for trail #%d (%s)\n", started.ReviewID, target.Trail.Number, target.Trail.Title) fmt.Fprintf(w, " Code version: %s\n", started.CodeVersionID) if started.BaseSHA != nil || started.HeadSHA != nil { - fmt.Fprintf(w, " Base: %s\n", abbreviateMaybe(optionalStringValue(started.BaseSHA), 12)) - fmt.Fprintf(w, " Head: %s\n", abbreviateMaybe(optionalStringValue(started.HeadSHA), 12)) + fmt.Fprintf(w, " Base: %s\n", abbreviate12(optionalStringValue(started.BaseSHA))) + fmt.Fprintf(w, " Head: %s\n", abbreviate12(optionalStringValue(started.HeadSHA))) } fmt.Fprintln(w) fmt.Fprintln(w, "Next:") @@ -1056,11 +1064,11 @@ func countTrailReviewComments(comments []api.TrailReviewComment) trailReviewComm case trailReviewStatusOpen: counts.Open++ switch strings.ToLower(stringPtrValue(comment.Severity)) { - case "high": + case trailReviewSeverityHigh: counts.OpenHigh++ - case "medium": + case trailReviewSeverityMedium: counts.OpenMedium++ - case "low": + case trailReviewSeverityLow: counts.OpenLow++ } } @@ -1076,7 +1084,7 @@ func filterCommentsBySeverity(comments []api.TrailReviewComment, severity string for _, comment := range comments { got := strings.ToLower(stringPtrValue(comment.Severity)) if severity == "" { - if got != "high" && got != "medium" && got != "low" { + if got != trailReviewSeverityHigh && got != trailReviewSeverityMedium && got != trailReviewSeverityLow { out = append(out, comment) } continue @@ -1115,11 +1123,11 @@ func trailReviewCommentTitle(comment api.TrailReviewComment) string { func severityTitle(severity string) string { switch severity { - case "high": + case trailReviewSeverityHigh: return "High" - case "medium": + case trailReviewSeverityMedium: return "Medium" - case "low": + case trailReviewSeverityLow: return "Low" default: return "Other" @@ -1135,11 +1143,11 @@ func severityDisplay(severity *string) string { func severityInitial(severity *string) string { switch strings.ToLower(stringPtrValue(severity)) { - case "high": + case trailReviewSeverityHigh: return "H" - case "medium": + case trailReviewSeverityMedium: return "M" - case "low": + case trailReviewSeverityLow: return "L" default: return "-" @@ -1206,8 +1214,9 @@ func optionalStringValue(s *string) string { return *s } -func abbreviateMaybe(s string, n int) string { - if len(s) <= n || n <= 0 { +func abbreviate12(s string) string { + const n = 12 + if len(s) <= n { return s } return s[:n] diff --git a/cmd/entire/cli/trail_review_cmd_test.go b/cmd/entire/cli/trail_review_cmd_test.go index 33cb2379c..9c46f929e 100644 --- a/cmd/entire/cli/trail_review_cmd_test.go +++ b/cmd/entire/cli/trail_review_cmd_test.go @@ -16,6 +16,8 @@ import ( "github.com/entireio/cli/cmd/entire/cli/paths" ) +const trailReviewApplyOriginalContent = "hello\nold\n" + func TestTrailReviewCommentsPath(t *testing.T) { got := trailReviewCommentsPath("trail id/with slash", trailReviewListOptions{ Status: "open,resolved", @@ -32,8 +34,8 @@ func TestTrailReviewCommentsPath(t *testing.T) { } func TestPrintTrailReviewDashboard(t *testing.T) { - high := "high" - medium := "medium" + high := trailReviewSeverityHigh + medium := trailReviewSeverityMedium path := "src/auth/session.ts" line := 88 comments := []api.TrailReviewComment{ @@ -117,14 +119,14 @@ func TestFetchTrailReviewCommentsAndPatchStatus(t *testing.T) { if got := r.URL.Query().Get("status"); got != "open" { t.Fatalf("status query = %q, want open", got) } - _ = json.NewEncoder(w).Encode(api.TrailReviewCommentsResponse{Comments: []api.TrailReviewComment{ + encodeTrailReviewTestJSON(t, w, api.TrailReviewCommentsResponse{Comments: []api.TrailReviewComment{ {ID: "cmt_1", TrailID: "trl_1", ReviewID: "rvw_1", Status: trailReviewStatusOpen, Location: api.TrailReviewLocation{Granularity: "whole_change"}}, }}) case r.Method == http.MethodPatch && r.URL.Path == "/api/v1/trails/trl_1/reviews/rvw_1/comments/cmt_1": if err := json.NewDecoder(r.Body).Decode(&gotPatchBody); err != nil { t.Fatalf("decode patch body: %v", err) } - _ = json.NewEncoder(w).Encode(api.TrailReviewComment{ID: "cmt_1", TrailID: "trl_1", ReviewID: "rvw_1", Status: trailReviewStatusResolved}) + encodeTrailReviewTestJSON(t, w, api.TrailReviewComment{ID: "cmt_1", TrailID: "trl_1", ReviewID: "rvw_1", Status: trailReviewStatusResolved}) default: t.Fatalf("unexpected request %s %s", r.Method, r.URL.String()) } @@ -162,14 +164,14 @@ func TestFetchTrailReviewStateFollowsCursor(t *testing.T) { switch r.URL.Query().Get("cursor") { case "": next := "cursor-2" - _ = json.NewEncoder(w).Encode(api.TrailReviewStateResponse{ + encodeTrailReviewTestJSON(t, w, api.TrailReviewStateResponse{ Review: api.TrailReview{ID: "rvw_1"}, CodeVersion: api.TrailReviewCodeVersion{ID: "cv_1"}, Comments: []api.TrailReviewComment{{ID: "cmt_1"}}, NextCursor: &next, }) case "cursor-2": - _ = json.NewEncoder(w).Encode(api.TrailReviewStateResponse{ + encodeTrailReviewTestJSON(t, w, api.TrailReviewStateResponse{ Review: api.TrailReview{ID: "rvw_1"}, CodeVersion: api.TrailReviewCodeVersion{ID: "cv_1"}, Comments: []api.TrailReviewComment{{ID: "cmt_2"}}, @@ -199,8 +201,8 @@ func TestFetchTrailReviewStateFollowsCursor(t *testing.T) { func TestApplyTrailReviewSuggestions_AppliesUnifiedDiff(t *testing.T) { repo := newTrailReviewApplyRepo(t) - writeTrailReviewApplyFile(t, repo, "file.txt", "hello\nold\n") - comment := trailReviewApplyComment(trailReviewPatch("file.txt", "old", "new")) + writeTrailReviewApplyFile(t, repo, "file.txt") + comment := trailReviewApplyComment(trailReviewPatch("file.txt", "old")) applied, err := applyTrailReviewSuggestions(context.Background(), comment, false, io.Discard) if err != nil { @@ -216,8 +218,8 @@ func TestApplyTrailReviewSuggestions_AppliesUnifiedDiff(t *testing.T) { func TestApplyTrailReviewSuggestions_CheckDoesNotModifyWorktree(t *testing.T) { repo := newTrailReviewApplyRepo(t) - writeTrailReviewApplyFile(t, repo, "file.txt", "hello\nold\n") - comment := trailReviewApplyComment(trailReviewPatch("file.txt", "old", "new")) + writeTrailReviewApplyFile(t, repo, "file.txt") + comment := trailReviewApplyComment(trailReviewPatch("file.txt", "old")) applied, err := applyTrailReviewSuggestions(context.Background(), comment, true, io.Discard) if err != nil { @@ -226,18 +228,18 @@ func TestApplyTrailReviewSuggestions_CheckDoesNotModifyWorktree(t *testing.T) { if applied != 1 { t.Fatalf("applied = %d, want 1", applied) } - if got := readTrailReviewApplyFile(t, repo, "file.txt"); got != "hello\nold\n" { + if got := readTrailReviewApplyFile(t, repo, "file.txt"); got != trailReviewApplyOriginalContent { t.Fatalf("file content = %q", got) } } func TestApplyTrailReviewSuggestions_FailureDoesNotPartiallyApply(t *testing.T) { repo := newTrailReviewApplyRepo(t) - writeTrailReviewApplyFile(t, repo, "a.txt", "hello\nold\n") - writeTrailReviewApplyFile(t, repo, "b.txt", "hello\nold\n") + writeTrailReviewApplyFile(t, repo, "a.txt") + writeTrailReviewApplyFile(t, repo, "b.txt") comment := trailReviewApplyComment( - trailReviewPatch("a.txt", "old", "new"), - trailReviewPatch("b.txt", "missing", "new"), + trailReviewPatch("a.txt", "old"), + trailReviewPatch("b.txt", "missing"), ) applied, err := applyTrailReviewSuggestions(context.Background(), comment, false, io.Discard) @@ -247,10 +249,10 @@ func TestApplyTrailReviewSuggestions_FailureDoesNotPartiallyApply(t *testing.T) if applied != 0 { t.Fatalf("applied = %d, want 0", applied) } - if got := readTrailReviewApplyFile(t, repo, "a.txt"); got != "hello\nold\n" { + if got := readTrailReviewApplyFile(t, repo, "a.txt"); got != trailReviewApplyOriginalContent { t.Fatalf("a.txt content = %q", got) } - if got := readTrailReviewApplyFile(t, repo, "b.txt"); got != "hello\nold\n" { + if got := readTrailReviewApplyFile(t, repo, "b.txt"); got != trailReviewApplyOriginalContent { t.Fatalf("b.txt content = %q", got) } } @@ -284,13 +286,13 @@ func newTrailReviewApplyRepo(t *testing.T) string { return dir } -func writeTrailReviewApplyFile(t *testing.T, repo, rel, content string) { +func writeTrailReviewApplyFile(t *testing.T, repo, rel string) { t.Helper() path := filepath.Join(repo, rel) if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { t.Fatalf("mkdir %s: %v", filepath.Dir(path), err) } - if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + if err := os.WriteFile(path, []byte(trailReviewApplyOriginalContent), 0o600); err != nil { t.Fatalf("write %s: %v", rel, err) } } @@ -306,7 +308,7 @@ func readTrailReviewApplyFile(t *testing.T, repo, rel string) string { func runTrailReviewApplyGit(t *testing.T, dir string, args ...string) { t.Helper() - cmd := exec.Command("git", args...) + cmd := exec.CommandContext(context.Background(), "git", args...) cmd.Dir = dir if out, err := cmd.CombinedOutput(); err != nil { t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) @@ -325,14 +327,21 @@ func trailReviewApplyComment(patches ...string) api.TrailReviewComment { return api.TrailReviewComment{ID: "cmt_1", SuggestedChanges: changes} } -func trailReviewPatch(file, oldText, newText string) string { +func trailReviewPatch(file, oldText string) string { return "diff --git a/" + file + " b/" + file + "\n" + "--- a/" + file + "\n" + "+++ b/" + file + "\n" + "@@ -1,2 +1,2 @@\n" + " hello\n" + "-" + oldText + "\n" + - "+" + newText + "\n" + "+new\n" +} + +func encodeTrailReviewTestJSON(t *testing.T, w http.ResponseWriter, v any) { + t.Helper() + if err := json.NewEncoder(w).Encode(v); err != nil { + t.Fatalf("encode response: %v", err) + } } func TestStartTrailReviewSendsIdempotencyKey(t *testing.T) { @@ -350,7 +359,7 @@ func TestStartTrailReviewSendsIdempotencyKey(t *testing.T) { if body.HeadSHA == nil || *body.HeadSHA != "abc" { t.Fatalf("HeadSHA = %#v", body.HeadSHA) } - _ = json.NewEncoder(w).Encode(api.TrailReviewStartResponse{ReviewID: "rvw_1", TrailID: "trl_1", CodeVersionID: "cv_1"}) + encodeTrailReviewTestJSON(t, w, api.TrailReviewStartResponse{ReviewID: "rvw_1", TrailID: "trl_1", CodeVersionID: "cv_1"}) })) defer srv.Close() t.Setenv(api.BaseURLEnvVar, srv.URL) From 43067b86ddb480cedd0f5164c32adf838d5cee51 Mon Sep 17 00:00:00 2001 From: dip Date: Fri, 5 Jun 2026 19:30:59 +0200 Subject: [PATCH 5/6] feat: simplify trail finding CLI --- cmd/entire/cli/api/client.go | 7 +- cmd/entire/cli/api/trail_review_types.go | 85 +-- .../checkpoint/committed_reader_resolve.go | 2 +- cmd/entire/cli/strategy/manual_commit.go | 2 +- cmd/entire/cli/trail_cmd.go | 21 +- cmd/entire/cli/trail_review_cmd.go | 663 +++++++++++------- cmd/entire/cli/trail_review_cmd_test.go | 199 +++++- cmd/entire/cli/trail_watch_cmd.go | 38 +- cmd/entire/cli/trail_watch_cmd_test.go | 2 +- e2e/README.md | 2 + e2e/examples/README.md | 27 + e2e/examples/trail-finding-golden-path.sh | 77 ++ 12 files changed, 738 insertions(+), 387 deletions(-) create mode 100644 e2e/examples/README.md create mode 100755 e2e/examples/trail-finding-golden-path.sh diff --git a/cmd/entire/cli/api/client.go b/cmd/entire/cli/api/client.go index cd79bb914..453207b36 100644 --- a/cmd/entire/cli/api/client.go +++ b/cmd/entire/cli/api/client.go @@ -111,11 +111,6 @@ func (c *Client) GetStream(ctx context.Context, path string, headers http.Header // Post sends an authenticated POST request with a JSON body to the given API-relative path. func (c *Client) Post(ctx context.Context, path string, body any) (*http.Response, error) { - return c.PostWithHeaders(ctx, path, body, nil) -} - -// PostWithHeaders sends an authenticated POST request with a JSON body and extra headers. -func (c *Client) PostWithHeaders(ctx context.Context, path string, body any, headers http.Header) (*http.Response, error) { var reader io.Reader if body != nil { data, err := json.Marshal(body) @@ -124,7 +119,7 @@ func (c *Client) PostWithHeaders(ctx context.Context, path string, body any, hea } reader = bytes.NewReader(data) } - return c.do(ctx, http.MethodPost, path, reader, headers) + return c.do(ctx, http.MethodPost, path, reader, nil) } // Put sends an authenticated PUT request with a JSON body to the given API-relative path. diff --git a/cmd/entire/cli/api/trail_review_types.go b/cmd/entire/cli/api/trail_review_types.go index 33fc33c9e..60bbe75cd 100644 --- a/cmd/entire/cli/api/trail_review_types.go +++ b/cmd/entire/cli/api/trail_review_types.go @@ -2,33 +2,6 @@ package api import "time" -// TrailReviewStartRequest is the body for POST /api/v1/trails/{trail_id}/reviews. -type TrailReviewStartRequest struct { - HeadSHA *string `json:"head_sha,omitempty"` - BaseSHA *string `json:"base_sha,omitempty"` - BaseRef *string `json:"base_ref,omitempty"` - HeadRef *string `json:"head_ref,omitempty"` -} - -// TrailReviewStartResponse is returned after creating or reusing a code review. -type TrailReviewStartResponse struct { - ReviewID string `json:"review_id"` - TrailID string `json:"trail_id"` - RepositoryID string `json:"repository_id"` - CodeVersionID string `json:"code_version_id"` - BaseSHA *string `json:"base_sha"` - HeadSHA *string `json:"head_sha"` - EventStreamURL string `json:"event_stream_url"` - DiffURL string `json:"diff_url"` - FilesURL string `json:"files_url"` - Limits TrailReviewStartLimits `json:"limits"` -} - -// TrailReviewStartLimits describes per-review API limits. -type TrailReviewStartLimits struct { - MaxCommentsPerBatch int `json:"max_comments_per_batch"` -} - // TrailReviewStateResponse is returned by GET /api/v1/trails/{trail_id}/reviews/{id}. type TrailReviewStateResponse struct { Review TrailReview `json:"review"` @@ -105,6 +78,42 @@ type TrailReviewComment struct { OutgoingLinks []TrailReviewOutgoingLink `json:"outgoing_links,omitempty"` } +// TrailReviewCommentCreateRequest creates an agent-native review finding on a trail. +type TrailReviewCommentCreateRequest struct { + Title *string `json:"title,omitempty"` + Body string `json:"body"` + Severity *string `json:"severity,omitempty"` + Confidence *float64 `json:"confidence,omitempty"` + ClientID *string `json:"client_id,omitempty"` + Location TrailReviewLocationCreateRequest `json:"location"` + SuggestedChanges []TrailReviewSuggestedChangeCreateRequest `json:"suggested_changes,omitempty"` +} + +// TrailReviewLocationCreateRequest identifies where a new finding applies. +type TrailReviewLocationCreateRequest struct { + Granularity string `json:"granularity"` + FilePath *string `json:"file_path,omitempty"` + StartLine *int `json:"start_line,omitempty"` + StartColumn *int `json:"start_column,omitempty"` + EndLine *int `json:"end_line,omitempty"` + EndColumn *int `json:"end_column,omitempty"` + SelectedText *string `json:"selected_text,omitempty"` + NearbyText *string `json:"nearby_text,omitempty"` + Language *string `json:"language,omitempty"` +} + +// TrailReviewSuggestedChangeCreateRequest attaches a suggested fix to a new finding. +type TrailReviewSuggestedChangeCreateRequest struct { + ChangeType string `json:"change_type"` + Patch *string `json:"patch,omitempty"` + Instruction *string `json:"instruction,omitempty"` + ExpectedFilePath *string `json:"expected_file_path,omitempty"` + ExpectedFileHash *string `json:"expected_file_hash,omitempty"` + ExpectedStartLine *int `json:"expected_start_line,omitempty"` + ExpectedEndLine *int `json:"expected_end_line,omitempty"` + ExpectedLines *string `json:"expected_lines,omitempty"` +} + // TrailReviewLocation identifies where a finding applies. type TrailReviewLocation struct { ID string `json:"id"` @@ -155,25 +164,3 @@ type TrailReviewCommentPatchRequest struct { Status string `json:"status,omitempty"` StatusReason *string `json:"status_reason,omitempty"` } - -// TrailSubmitReviewRequest is the body for POST /api/v1/trails/{host}/{owner}/{repo}/{number}/review. -type TrailSubmitReviewRequest struct { - Event string `json:"event"` - Body string `json:"body,omitempty"` -} - -// TrailSubmitReviewResponse is returned after approving or requesting changes on a trail. -type TrailSubmitReviewResponse struct { - OK bool `json:"ok"` - Review TrailSubmitReview `json:"review"` -} - -// TrailSubmitReview is a human trail review verdict. -type TrailSubmitReview struct { - ID string `json:"id"` - Author string `json:"author"` - Event string `json:"event"` - Body *string `json:"body"` - CommitSHA string `json:"commit_sha"` - CreatedAt time.Time `json:"created_at"` -} diff --git a/cmd/entire/cli/checkpoint/committed_reader_resolve.go b/cmd/entire/cli/checkpoint/committed_reader_resolve.go index c5aeb665a..7ca60c8a7 100644 --- a/cmd/entire/cli/checkpoint/committed_reader_resolve.go +++ b/cmd/entire/cli/checkpoint/committed_reader_resolve.go @@ -54,7 +54,7 @@ type CommittedReaderOptions struct { BlobFetcher BlobFetchFunc } -func NewCommittedReader(ctx context.Context, repo *git.Repository, opts CommittedReaderOptions) (CommittedStore, error) { //nolint:ireturn // Factory selects between v1 and dual reader implementations. +func NewCommittedReader(ctx context.Context, repo *git.Repository, opts CommittedReaderOptions) (CommittedStore, error) { if repo == nil { return nil, errors.New("git repository is required") } diff --git a/cmd/entire/cli/strategy/manual_commit.go b/cmd/entire/cli/strategy/manual_commit.go index d83e49456..9e3364876 100644 --- a/cmd/entire/cli/strategy/manual_commit.go +++ b/cmd/entire/cli/strategy/manual_commit.go @@ -48,7 +48,7 @@ func (s *ManualCommitStrategy) getCheckpointStore(repo *git.Repository) *checkpo return store } -func (s *ManualCommitStrategy) committedCheckpointStore(ctx context.Context, repo *git.Repository) (checkpoint.CommittedListReader, error) { //nolint:ireturn // Strategy callers need the selected committed checkpoint reader implementation. +func (s *ManualCommitStrategy) committedCheckpointStore(ctx context.Context, repo *git.Repository) (checkpoint.CommittedListReader, error) { WarnIfMetadataDisconnected() store, err := checkpoint.NewCommittedReader(ctx, repo, checkpoint.CommittedReaderOptions{ BlobFetcher: s.blobFetcher, diff --git a/cmd/entire/cli/trail_cmd.go b/cmd/entire/cli/trail_cmd.go index 6ac126e3a..0a771eb16 100644 --- a/cmd/entire/cli/trail_cmd.go +++ b/cmd/entire/cli/trail_cmd.go @@ -9,6 +9,7 @@ import ( "net/http" "os/exec" "sort" + "strconv" "strings" "text/tabwriter" "time" @@ -39,6 +40,7 @@ func newTrailCmd() *cobra.Command { Use: "trail", Short: "Manage trails for your branches", Hidden: true, + Args: cobra.NoArgs, Long: `Trails are branch-centric work tracking abstractions. They describe the "why" and "what" of your work, while checkpoints capture the "how" and "when". @@ -58,7 +60,7 @@ branch, or lists recent trails if no trail exists for the current branch.`, cmd.AddCommand(newTrailListCmd()) cmd.AddCommand(newTrailCreateCmd()) cmd.AddCommand(newTrailUpdateCmd()) - cmd.AddCommand(newTrailReviewCmd()) + cmd.AddCommand(newTrailFindingCmd()) cmd.AddCommand(newTrailWatchCmd()) return cmd @@ -419,12 +421,25 @@ func printTrailRows(w io.Writer, trails []*trail.Metadata, showAuthor bool) { // tabwriter aligns by display columns instead of bytes, so multi-byte // branch names or logins don't throw off the table. tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) + if showAuthor { + fmt.Fprintln(tw, " NUM\tBRANCH\tTITLE\tAUTHOR\tUPDATED") + } else { + fmt.Fprintln(tw, " NUM\tBRANCH\tTITLE\tUPDATED") + } for _, t := range trails { + number := "-" + if t.Number > 0 { + number = strconv.Itoa(t.Number) + } + title := truncateOneLine(t.Title, 60) + if title == "" { + title = "(untitled)" + } if showAuthor { - fmt.Fprintf(tw, " %s\t%s\t%s\n", t.Branch, t.AuthorLogin(), timeAgo(t.UpdatedAt)) + fmt.Fprintf(tw, " %s\t%s\t%s\t%s\t%s\n", number, t.Branch, title, t.AuthorLogin(), timeAgo(t.UpdatedAt)) continue } - fmt.Fprintf(tw, " %s\t%s\n", t.Branch, timeAgo(t.UpdatedAt)) + fmt.Fprintf(tw, " %s\t%s\t%s\t%s\n", number, t.Branch, title, timeAgo(t.UpdatedAt)) } _ = tw.Flush() } diff --git a/cmd/entire/cli/trail_review_cmd.go b/cmd/entire/cli/trail_review_cmd.go index 2f1f53a93..a66c3ffa2 100644 --- a/cmd/entire/cli/trail_review_cmd.go +++ b/cmd/entire/cli/trail_review_cmd.go @@ -8,8 +8,8 @@ import ( "errors" "fmt" "io" - "net/http" "net/url" + "os" "os/exec" "path" "sort" @@ -37,6 +37,8 @@ const ( trailReviewSeverityLow = "low" ) +var errTrailReviewDefaultTargetNotFound = errors.New("default trail finding target not found") + type trailReviewListOptions struct { Status string Severity string @@ -47,6 +49,10 @@ type trailReviewListOptions struct { JSON bool } +type trailReviewTargetOptions struct { + Selector string +} + type trailReviewTarget struct { Host string Owner string @@ -54,37 +60,39 @@ type trailReviewTarget struct { Trail api.TrailResource } -func newTrailReviewCmd() *cobra.Command { +func newTrailFindingCmd() *cobra.Command { opts := defaultTrailReviewListOptions() + targetOpts := trailReviewTargetOptions{} cmd := &cobra.Command{ - Use: "review []", - Short: "Review a trail's agent findings", - Long: `Review a trail's agent-native code-review findings. - -Running 'entire trail review' shows the review dashboard for the current -branch's trail. Pass a trail number to review another trail in the same repo.`, + Use: "finding []", + Short: "Manage a trail's agent findings", + Long: `Manage a trail's agent-native findings. + +Running 'entire trail finding' shows the finding dashboard for the current +branch's trail. Pass a trail selector (number, id, or branch) to inspect another +trail in the same repo. Use 'entire trail list --status any' when you need to +discover a trail selector first.`, Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - number, err := parseOptionalTrailNumber(args) + selector, err := parseOptionalTrailSelector(args, targetOpts.Selector) if err != nil { return err } - return runTrailReviewDashboard(cmd, number, opts) + return runTrailReviewDashboard(cmd, selector, opts) }, } + cmd.PersistentFlags().StringVar(&targetOpts.Selector, "trail", "", "Trail selector (number, id, or branch); defaults to the current branch's trail") addTrailReviewListFlags(cmd, &opts) - cmd.AddCommand(newTrailReviewStartCmd()) - cmd.AddCommand(newTrailReviewCommentsCmd()) - cmd.AddCommand(newTrailReviewShowCmd()) - cmd.AddCommand(newTrailReviewApplyCmd()) - cmd.AddCommand(newTrailReviewStatusCmd("resolve", trailReviewStatusResolved, "Resolve a review finding")) - cmd.AddCommand(newTrailReviewStatusCmd("dismiss", trailReviewStatusDismissed, "Dismiss a review finding")) - cmd.AddCommand(newTrailReviewStatusCmd("reopen", trailReviewStatusOpen, "Reopen a review finding")) - cmd.AddCommand(newTrailReviewWatchCmd()) - cmd.AddCommand(newTrailReviewSubmitCmd("approve", "APPROVE", "Approve a trail")) - cmd.AddCommand(newTrailReviewSubmitCmd("request-changes", "REQUEST_CHANGES", "Request changes on a trail")) + cmd.AddCommand(newTrailFindingListCmd(&targetOpts)) + cmd.AddCommand(newTrailFindingAddCmd(&targetOpts)) + cmd.AddCommand(newTrailReviewShowCmd(&targetOpts)) + cmd.AddCommand(newTrailReviewApplyCmd(&targetOpts)) + cmd.AddCommand(newTrailReviewStatusCmd(&targetOpts, "resolve", trailReviewStatusResolved, "Resolve a finding")) + cmd.AddCommand(newTrailReviewStatusCmd(&targetOpts, "dismiss", trailReviewStatusDismissed, "Dismiss a finding")) + cmd.AddCommand(newTrailReviewStatusCmd(&targetOpts, "reopen", trailReviewStatusOpen, "Reopen a finding")) + cmd.AddCommand(newTrailReviewWatchCmd(&targetOpts)) return cmd } @@ -107,67 +115,81 @@ func addTrailReviewListFlags(cmd *cobra.Command, opts *trailReviewListOptions) { cmd.Flags().BoolVar(&opts.JSON, "json", false, "Output as JSON") } -type trailReviewStartOptions struct { - BaseRef string - HeadRef string - BaseSHA string - HeadSHA string - JSON bool - Watch bool -} - -func newTrailReviewStartCmd() *cobra.Command { - var opts trailReviewStartOptions +func newTrailFindingListCmd(targetOpts *trailReviewTargetOptions) *cobra.Command { + opts := defaultTrailReviewListOptions() cmd := &cobra.Command{ - Use: "start []", - Short: "Start an agent-native review for a trail", + Use: "list []", + Short: "List findings for a trail", Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - number, err := parseOptionalTrailNumber(args) + selector, err := parseOptionalTrailSelector(args, targetOpts.Selector) if err != nil { return err } - return runTrailReviewStart(cmd, number, opts) + return runTrailReviewComments(cmd, selector, opts) }, } - cmd.Flags().StringVar(&opts.BaseRef, "base-ref", "", "Base ref covered by the review (defaults to trail base)") - cmd.Flags().StringVar(&opts.HeadRef, "head-ref", "", "Head ref covered by the review (defaults to current branch)") - cmd.Flags().StringVar(&opts.BaseSHA, "base-sha", "", "Base SHA covered by the review") - cmd.Flags().StringVar(&opts.HeadSHA, "head-sha", "", "Head SHA covered by the review (defaults to HEAD)") - cmd.Flags().BoolVar(&opts.JSON, "json", false, "Output as JSON") - cmd.Flags().BoolVar(&opts.Watch, "watch", false, "After starting, stream trail review events") + addTrailReviewListFlags(cmd, &opts) return cmd } -func newTrailReviewCommentsCmd() *cobra.Command { - opts := defaultTrailReviewListOptions() +type trailReviewCommentAddOptions struct { + Title string + Body string + Severity string + Confidence float64 + FilePath string + Line int + StartLine int + EndLine int + ClientID string + Patch string + PatchFile string + Instruction string + JSON bool +} + +func newTrailFindingAddCmd(targetOpts *trailReviewTargetOptions) *cobra.Command { + var opts trailReviewCommentAddOptions cmd := &cobra.Command{ - Use: "comments []", - Short: "List review findings for a trail", + Use: "add []", + Short: "Create a finding on a trail", Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - number, err := parseOptionalTrailNumber(args) + selector, err := parseOptionalTrailSelector(args, targetOpts.Selector) if err != nil { return err } - return runTrailReviewComments(cmd, number, opts) + return runTrailReviewCommentAdd(cmd, selector, opts) }, } - addTrailReviewListFlags(cmd, &opts) + cmd.Flags().StringVar(&opts.Title, "title", "", "Finding title") + cmd.Flags().StringVarP(&opts.Body, "body", "m", "", "Finding body") + cmd.Flags().StringVar(&opts.Severity, "severity", "", "Finding severity: high,medium,low") + cmd.Flags().Float64Var(&opts.Confidence, "confidence", -1, "Finding confidence from 0.0 to 1.0") + cmd.Flags().StringVar(&opts.FilePath, "file", "", "File path for the finding location") + 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.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") + cmd.Flags().StringVar(&opts.Instruction, "instruction", "", "Manual suggested-change instruction to attach") + cmd.Flags().BoolVar(&opts.JSON, "json", false, "Output as JSON") return cmd } -func newTrailReviewShowCmd() *cobra.Command { +func newTrailReviewShowCmd(targetOpts *trailReviewTargetOptions) *cobra.Command { cmd := &cobra.Command{ - Use: "show [] ", - Short: "Show a review finding", + Use: "show [] ", + Short: "Show a finding", Args: cobra.RangeArgs(1, 2), RunE: func(cmd *cobra.Command, args []string) error { - number, commentID, err := parseTrailNumberAndCommentID(args) + selector, commentID, err := parseTrailSelectorAndCommentID(args, targetOpts.Selector) if err != nil { return err } - return runTrailReviewShow(cmd, number, commentID) + return runTrailReviewShow(cmd, selector, commentID) }, } return cmd @@ -178,18 +200,18 @@ type trailReviewApplyOptions struct { Check bool } -func newTrailReviewApplyCmd() *cobra.Command { +func newTrailReviewApplyCmd(targetOpts *trailReviewTargetOptions) *cobra.Command { var opts trailReviewApplyOptions cmd := &cobra.Command{ - Use: "apply [] ", - Short: "Apply a review finding's unified-diff suggestion", + Use: "apply [] ", + Short: "Apply a finding's unified-diff suggestion", Args: cobra.RangeArgs(1, 2), RunE: func(cmd *cobra.Command, args []string) error { - number, commentID, err := parseTrailNumberAndCommentID(args) + selector, commentID, err := parseTrailSelectorAndCommentID(args, targetOpts.Selector) if err != nil { return err } - return runTrailReviewApply(cmd, number, commentID, opts) + return runTrailReviewApply(cmd, selector, commentID, opts) }, } cmd.Flags().BoolVar(&opts.Resolve, "resolve", false, "Mark the finding resolved after applying") @@ -197,40 +219,40 @@ func newTrailReviewApplyCmd() *cobra.Command { return cmd } -func newTrailReviewStatusCmd(use, status, short string) *cobra.Command { +func newTrailReviewStatusCmd(targetOpts *trailReviewTargetOptions, use, status, short string) *cobra.Command { var message string cmd := &cobra.Command{ - Use: use + " [] ", + Use: use + " [] ", Short: short, Args: cobra.RangeArgs(1, 2), RunE: func(cmd *cobra.Command, args []string) error { - number, commentID, err := parseTrailNumberAndCommentID(args) + selector, commentID, err := parseTrailSelectorAndCommentID(args, targetOpts.Selector) if err != nil { return err } - return runTrailReviewSetStatus(cmd, number, commentID, status, message) + return runTrailReviewSetStatus(cmd, selector, commentID, status, message) }, } cmd.Flags().StringVarP(&message, "message", "m", "", "Status reason to record") return cmd } -func newTrailReviewWatchCmd() *cobra.Command { +func newTrailReviewWatchCmd(targetOpts *trailReviewTargetOptions) *cobra.Command { var ( jsonOutput bool showPings bool once bool ) cmd := &cobra.Command{ - Use: "watch []", - Short: "Tail a trail's code-review events live", + Use: "watch []", + Short: "Tail a trail's finding events live", Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - number, err := parseOptionalTrailNumber(args) + selector, err := parseOptionalTrailSelector(args, targetOpts.Selector) if err != nil { return err } - return runTrailWatchWithOptions(cmd, number, jsonOutput, showPings, once) + return runTrailReviewWatch(cmd, selector, jsonOutput, showPings, once) }, } cmd.Flags().BoolVar(&jsonOutput, "json", false, "Print each event as a single JSON line") @@ -239,27 +261,14 @@ func newTrailReviewWatchCmd() *cobra.Command { return cmd } -func newTrailReviewSubmitCmd(use, event, short string) *cobra.Command { - var body string - cmd := &cobra.Command{ - Use: use + " []", - Short: short, - Args: cobra.MaximumNArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - number, err := parseOptionalTrailNumber(args) - if err != nil { - return err - } - return runTrailReviewSubmit(cmd, number, event, body) - }, - } - cmd.Flags().StringVarP(&body, "message", "m", "", "Review message") - return cmd -} - -func runTrailReviewDashboard(cmd *cobra.Command, number int, opts trailReviewListOptions) error { - client, target, err := authenticatedTrailReviewTarget(cmd, number) +func runTrailReviewDashboard(cmd *cobra.Command, selector string, opts trailReviewListOptions) error { + client, target, err := authenticatedTrailReviewTarget(cmd, selector) if err != nil { + if strings.TrimSpace(selector) == "" && errors.Is(err, errTrailReviewDefaultTargetNotFound) { + fmt.Fprintln(cmd.OutOrStdout(), "No trail found for the current branch; showing trails in this repo.") + fmt.Fprintln(cmd.OutOrStdout()) + return runTrailListAll(cmd.Context(), cmd.OutOrStdout(), trailListOptions{Status: trailListStatusAny, Limit: defaultTrailListLimit, InsecureHTTP: trailInsecureHTTP(cmd)}) + } return err } comments, hasMore, err := fetchTrailReviewComments(cmd.Context(), client, target.Trail.ID, opts) @@ -278,8 +287,8 @@ func runTrailReviewDashboard(cmd *cobra.Command, number int, opts trailReviewLis return nil } -func runTrailReviewComments(cmd *cobra.Command, number int, opts trailReviewListOptions) error { - client, target, err := authenticatedTrailReviewTarget(cmd, number) +func runTrailReviewComments(cmd *cobra.Command, selector string, opts trailReviewListOptions) error { + client, target, err := authenticatedTrailReviewTarget(cmd, selector) if err != nil { return err } @@ -294,8 +303,37 @@ func runTrailReviewComments(cmd *cobra.Command, number int, opts trailReviewList return nil } -func runTrailReviewShow(cmd *cobra.Command, number int, commentID string) error { - client, target, err := authenticatedTrailReviewTarget(cmd, number) +func runTrailReviewCommentAdd(cmd *cobra.Command, selector string, opts trailReviewCommentAddOptions) error { + client, target, err := authenticatedTrailReviewTarget(cmd, selector) + if err != nil { + return err + } + opts, err = loadTrailReviewCommentPatchFile(opts, cmd.InOrStdin()) + if err != nil { + return err + } + request, err := buildTrailReviewCommentCreateRequest(opts) + if err != nil { + return err + } + created, err := createTrailReviewComment(cmd.Context(), client, target.Trail.ID, request) + if err != nil { + return err + } + if opts.JSON { + enc := json.NewEncoder(cmd.OutOrStdout()) + enc.SetIndent("", " ") + if err := enc.Encode(created); err != nil { + return fmt.Errorf("encode created finding: %w", err) + } + return nil + } + printTrailReviewCommentCreated(cmd.OutOrStdout(), target, created) + return nil +} + +func runTrailReviewShow(cmd *cobra.Command, selector string, commentID string) error { + client, target, err := authenticatedTrailReviewTarget(cmd, selector) if err != nil { return err } @@ -310,8 +348,8 @@ func runTrailReviewShow(cmd *cobra.Command, number int, commentID string) error return nil } -func runTrailReviewApply(cmd *cobra.Command, number int, commentID string, opts trailReviewApplyOptions) error { - client, target, err := authenticatedTrailReviewTarget(cmd, number) +func runTrailReviewApply(cmd *cobra.Command, selector string, commentID string, opts trailReviewApplyOptions) error { + client, target, err := authenticatedTrailReviewTarget(cmd, selector) if err != nil { return err } @@ -340,13 +378,13 @@ func runTrailReviewApply(cmd *cobra.Command, number int, commentID string, opts if err != nil { return err } - fmt.Fprintf(cmd.OutOrStdout(), "Updated comment %s: %s → %s\n", updated.ID, comment.Status, updated.Status) + fmt.Fprintf(cmd.OutOrStdout(), "Updated finding %s: %s → %s\n", updated.ID, comment.Status, updated.Status) } return nil } -func runTrailReviewSetStatus(cmd *cobra.Command, number int, commentID, status, message string) error { - client, target, err := authenticatedTrailReviewTarget(cmd, number) +func runTrailReviewSetStatus(cmd *cobra.Command, selector string, commentID, status, message string) error { + client, target, err := authenticatedTrailReviewTarget(cmd, selector) if err != nil { return err } @@ -362,96 +400,65 @@ func runTrailReviewSetStatus(cmd *cobra.Command, number int, commentID, status, if err != nil { return err } - fmt.Fprintf(cmd.OutOrStdout(), "Updated comment %s: %s → %s\n", updated.ID, oldStatus, updated.Status) + fmt.Fprintf(cmd.OutOrStdout(), "Updated finding %s: %s → %s\n", updated.ID, oldStatus, updated.Status) return nil } -func runTrailReviewStart(cmd *cobra.Command, number int, opts trailReviewStartOptions) error { - client, target, err := authenticatedTrailReviewTarget(cmd, number) - if err != nil { - return err - } - request, err := buildTrailReviewStartRequest(cmd.Context(), target, opts) +func runTrailReviewWatch(cmd *cobra.Command, selector string, jsonOutput, showPings, once bool) error { + client, target, err := authenticatedTrailReviewTarget(cmd, selector) if err != nil { return err } - idempotencyKey := trailReviewStartIdempotencyKey(target.Trail.ID, request) - started, err := startTrailReview(cmd.Context(), client, target.Trail.ID, request, idempotencyKey) - if err != nil { - return err - } - if opts.JSON { - enc := json.NewEncoder(cmd.OutOrStdout()) - enc.SetIndent("", " ") - if err := enc.Encode(started); err != nil { - return fmt.Errorf("encode review start response: %w", err) - } - } else { - printTrailReviewStart(cmd.OutOrStdout(), target, started) - } - if opts.Watch { - return runTrailWatchWithOptions(cmd, number, false, false, false) - } - return nil + return runTrailReviewWatchWithClient(cmd, client, target, jsonOutput, showPings, once) } -func runTrailReviewSubmit(cmd *cobra.Command, number int, event, body string) error { - client, target, err := authenticatedTrailReviewTarget(cmd, number) - if err != nil { - return err - } - if target.Trail.Number <= 0 { - return fmt.Errorf("trail %s has no number; cannot submit review", target.Trail.ID) - } - if event == "REQUEST_CHANGES" && strings.TrimSpace(body) == "" { - return errors.New("request-changes requires --message") - } - resp, err := submitTrailReview(cmd.Context(), client, target, event, body) - if err != nil { - return err +func runTrailReviewWatchWithClient(cmd *cobra.Command, client *api.Client, target trailReviewTarget, jsonOutput, showPings, once bool) error { + if target.Trail.ID == "" { + return fmt.Errorf("trail %s has no id yet", trailReviewTargetDisplay(target)) } - fmt.Fprintf(cmd.OutOrStdout(), "%s submitted by %s on %s\n", resp.Review.Event, resp.Review.Author, abbreviate12(resp.Review.CommitSHA)) - return nil + description := trailWatchDescription(target.Host, target.Owner, target.Repo, target.Trail.Number, target.Trail.ID) + return runTrailWatchResolved(cmd, client, target.Trail.ID, description, jsonOutput, showPings, once) } -func authenticatedTrailReviewTarget(cmd *cobra.Command, number int) (*api.Client, trailReviewTarget, error) { +func authenticatedTrailReviewTarget(cmd *cobra.Command, selector string) (*api.Client, trailReviewTarget, error) { client, err := NewAuthenticatedAPIClient(cmd.Context(), trailInsecureHTTP(cmd)) if err != nil { return nil, trailReviewTarget{}, fmt.Errorf("authentication required: %w", err) } - target, err := resolveTrailReviewTarget(cmd.Context(), client, number) + target, err := resolveTrailReviewTarget(cmd.Context(), client, selector) if err != nil { return nil, trailReviewTarget{}, err } return client, target, nil } -func resolveTrailReviewTarget(ctx context.Context, client *api.Client, number int) (trailReviewTarget, error) { +func resolveTrailReviewTarget(ctx context.Context, client *api.Client, selector string) (trailReviewTarget, error) { host, owner, repo, err := gitremote.ResolveRemoteRepo(ctx, "origin") if err != nil { return trailReviewTarget{}, fmt.Errorf("failed to resolve repository: %w", err) } + selector = strings.TrimSpace(selector) var found *api.TrailResource - if number > 0 { - found, err = findTrailByNumber(ctx, client, host, owner, repo, number) + if selector != "" { + found, err = findTrailBySelector(ctx, client, host, owner, repo, selector) if err != nil { return trailReviewTarget{}, err } if found == nil { - return trailReviewTarget{}, fmt.Errorf("no trail #%d found in %s/%s/%s", number, host, owner, repo) + return trailReviewTarget{}, fmt.Errorf("no trail %q found in %s/%s/%s (run 'entire trail list --status any')", selector, host, owner, repo) } } else { branch, branchErr := GetCurrentBranch(ctx) if branchErr != nil { - return trailReviewTarget{}, fmt.Errorf("no trail number given and current branch is unknown: %w", branchErr) + return trailReviewTarget{}, fmt.Errorf("%w: no trail selector given and current branch is unknown: %w\nhint: run 'entire trail list --status any' or pass --trail ", errTrailReviewDefaultTargetNotFound, branchErr) } found, err = findTrailByBranch(ctx, client, host, owner, repo, branch) if err != nil { return trailReviewTarget{}, err } if found == nil { - return trailReviewTarget{}, fmt.Errorf("no trail found for branch %q (run 'entire trail create' or pass a trail number)", branch) + return trailReviewTarget{}, fmt.Errorf("%w: no trail found for current branch %q\nhint: run 'entire trail create', 'entire trail list --status any', or pass --trail ", errTrailReviewDefaultTargetNotFound, branch) } } if found.ID == "" { @@ -460,6 +467,30 @@ func resolveTrailReviewTarget(ctx context.Context, client *api.Client, number in return trailReviewTarget{Host: host, Owner: owner, Repo: repo, Trail: *found}, nil } +func findTrailBySelector(ctx context.Context, client *api.Client, host, owner, repo, selector string) (*api.TrailResource, error) { + selector = strings.TrimSpace(selector) + if selector == "" { + return nil, nil //nolint:nilnil // empty selector means not found for this helper + } + if n, ok := parseTrailReviewNumberSelector(selector); ok { + found, err := findTrailByNumber(ctx, client, host, owner, repo, n) + if err != nil || found != nil { + return found, err + } + } + return findTrail(ctx, client, host, owner, repo, func(t api.TrailResource) bool { + return t.ID == selector || t.Branch == selector + }) +} + +func parseTrailReviewNumberSelector(selector string) (int, bool) { + n, err := strconv.Atoi(strings.TrimSpace(selector)) + if err != nil || n <= 0 { + return 0, false + } + return n, true +} + func fetchTrailReviewComments(ctx context.Context, client *api.Client, trailID string, opts trailReviewListOptions) ([]api.TrailReviewComment, bool, error) { if opts.Limit <= 0 { return nil, false, errors.New("limit must be greater than 0") @@ -469,7 +500,7 @@ func fetchTrailReviewComments(ctx context.Context, client *api.Client, trailID s } resp, err := client.Get(ctx, trailReviewCommentsPath(trailID, opts)) if err != nil { - return nil, false, fmt.Errorf("list review comments: %w", err) + return nil, false, fmt.Errorf("list findings: %w", err) } defer resp.Body.Close() if err := checkTrailResponse(resp); err != nil { @@ -477,7 +508,7 @@ func fetchTrailReviewComments(ctx context.Context, client *api.Client, trailID s } var out api.TrailReviewCommentsResponse if err := api.DecodeJSON(resp, &out); err != nil { - return nil, false, fmt.Errorf("decode review comments: %w", err) + return nil, false, fmt.Errorf("decode findings: %w", err) } return out.Comments, out.HasMore, nil } @@ -530,13 +561,155 @@ func trailReviewCommentsPath(trailID string, opts trailReviewListOptions) string if opts.Offset > 0 { q.Set("offset", strconv.Itoa(opts.Offset)) } - path := "/api/v1/trails/" + url.PathEscape(trailID) + "/reviews/comments" + path := trailReviewCreateCommentPath(trailID) if encoded := q.Encode(); encoded != "" { path += "?" + encoded } return path } +func loadTrailReviewCommentPatchFile(opts trailReviewCommentAddOptions, stdin io.Reader) (trailReviewCommentAddOptions, error) { + patchFile := strings.TrimSpace(opts.PatchFile) + if patchFile == "" { + return opts, nil + } + if strings.TrimSpace(opts.Patch) != "" { + return opts, errors.New("pass either --patch or --patch-file, not both") + } + var ( + data []byte + err error + ) + if patchFile == "-" { + data, err = io.ReadAll(stdin) + } else { + data, err = os.ReadFile(patchFile) //nolint:gosec // --patch-file is an explicit user-selected input path. + } + if err != nil { + return opts, fmt.Errorf("read patch file %q: %w", patchFile, err) + } + opts.Patch = string(data) + return opts, nil +} + +func buildTrailReviewCommentCreateRequest(opts trailReviewCommentAddOptions) (api.TrailReviewCommentCreateRequest, error) { + body := strings.TrimSpace(opts.Body) + if body == "" { + return api.TrailReviewCommentCreateRequest{}, errors.New("finding body is required (pass --body)") + } + severity := strings.ToLower(strings.TrimSpace(opts.Severity)) + if severity != "" { + switch severity { + case trailReviewSeverityHigh, trailReviewSeverityMedium, trailReviewSeverityLow: + default: + return api.TrailReviewCommentCreateRequest{}, fmt.Errorf("invalid severity %q: valid values are high, medium, low", opts.Severity) + } + } + confidence, hasConfidence, err := buildTrailReviewCommentConfidence(opts.Confidence) + if err != nil { + return api.TrailReviewCommentCreateRequest{}, err + } + loc, err := buildTrailReviewCommentLocation(opts) + if err != nil { + return api.TrailReviewCommentCreateRequest{}, err + } + request := api.TrailReviewCommentCreateRequest{ + Title: optionalStringPtr(strings.TrimSpace(opts.Title)), + Body: body, + Severity: optionalStringPtr(severity), + ClientID: optionalStringPtr(strings.TrimSpace(opts.ClientID)), + Location: loc, + } + if hasConfidence { + request.Confidence = &confidence + } + if patch := strings.TrimSpace(opts.Patch); patch != "" { + change := api.TrailReviewSuggestedChangeCreateRequest{ + ChangeType: "unified_diff", + Patch: stringPtr(patch), + Instruction: optionalStringPtr(strings.TrimSpace(opts.Instruction)), + } + request.SuggestedChanges = append(request.SuggestedChanges, change) + } else if instruction := strings.TrimSpace(opts.Instruction); instruction != "" { + request.SuggestedChanges = append(request.SuggestedChanges, api.TrailReviewSuggestedChangeCreateRequest{ + ChangeType: "manual_instruction", + Instruction: stringPtr(instruction), + }) + } + return request, nil +} + +func buildTrailReviewCommentConfidence(confidence float64) (float64, bool, error) { + if confidence < 0 { + return 0, false, nil + } + if confidence > 1 { + return 0, false, errors.New("--confidence must be between 0.0 and 1.0") + } + return confidence, true, nil +} + +func buildTrailReviewCommentLocation(opts trailReviewCommentAddOptions) (api.TrailReviewLocationCreateRequest, error) { + filePath := strings.TrimSpace(opts.FilePath) + line := opts.Line + if line < 0 || opts.StartLine < 0 || opts.EndLine < 0 { + return api.TrailReviewLocationCreateRequest{}, errors.New("line numbers must be non-negative") + } + if opts.StartLine > 0 { + if line > 0 { + return api.TrailReviewLocationCreateRequest{}, errors.New("pass either --line or --start-line, not both") + } + line = opts.StartLine + } + if filePath == "" && (line > 0 || opts.EndLine > 0) { + return api.TrailReviewLocationCreateRequest{}, errors.New("--line/--start-line/--end-line require --file") + } + if opts.EndLine > 0 && line == 0 { + return api.TrailReviewLocationCreateRequest{}, errors.New("--end-line requires --line or --start-line") + } + if opts.EndLine > 0 && opts.EndLine < line { + return api.TrailReviewLocationCreateRequest{}, errors.New("--end-line must be greater than or equal to the start line") + } + + loc := api.TrailReviewLocationCreateRequest{Granularity: "whole_change"} + if filePath == "" { + return loc, nil + } + loc.Granularity = "file" + loc.FilePath = stringPtr(filePath) + if line > 0 { + loc.Granularity = "line" + loc.StartLine = &line + if opts.EndLine > 0 { + loc.EndLine = &opts.EndLine + if opts.EndLine != line { + loc.Granularity = "range" + } + } + } + return loc, nil +} + +func createTrailReviewComment(ctx context.Context, client *api.Client, trailID string, request api.TrailReviewCommentCreateRequest) (api.TrailReviewComment, error) { + resp, err := client.Post(ctx, trailReviewCreateCommentPath(trailID), request) + if err != nil { + return api.TrailReviewComment{}, fmt.Errorf("create finding: %w", err) + } + defer resp.Body.Close() + if err := checkTrailResponse(resp); err != nil { + return api.TrailReviewComment{}, err + } + var created api.TrailReviewComment + if err := api.DecodeJSON(resp, &created); err != nil { + return api.TrailReviewComment{}, fmt.Errorf("decode created finding: %w", err) + } + return created, nil +} + +func trailReviewCreateCommentPath(trailID string) string { + return "/api/v1/trails/" + url.PathEscape(trailID) + "/reviews/comments" +} + func resolveTrailReviewComment(ctx context.Context, client *api.Client, trailID, commentID string) (api.TrailReviewComment, error) { opts := trailReviewListOptions{ Status: trailReviewStatusAny, @@ -565,7 +738,7 @@ func resolveTrailReviewComment(ctx context.Context, client *api.Client, trailID, } switch len(matches) { case 0: - return api.TrailReviewComment{}, fmt.Errorf("no review comment %q found", commentID) + return api.TrailReviewComment{}, fmt.Errorf("no finding %q found", commentID) case 1: return matches[0], nil default: @@ -574,7 +747,7 @@ func resolveTrailReviewComment(ctx context.Context, client *api.Client, trailID, ids[i] = matches[i].ID } sort.Strings(ids) - return api.TrailReviewComment{}, fmt.Errorf("ambiguous review comment %q (matches: %s)", commentID, strings.Join(ids, ", ")) + return api.TrailReviewComment{}, fmt.Errorf("ambiguous finding %q (matches: %s)", commentID, strings.Join(ids, ", ")) } } @@ -593,7 +766,7 @@ func hydrateTrailReviewCommentWithState(ctx context.Context, client *api.Client, return candidate, state, nil } } - return api.TrailReviewComment{}, api.TrailReviewStateResponse{}, fmt.Errorf("review %s did not include comment %s", comment.ReviewID, comment.ID) + return api.TrailReviewComment{}, api.TrailReviewStateResponse{}, fmt.Errorf("finding %s details were not returned by the API", comment.ID) } func fetchTrailReviewState(ctx context.Context, client *api.Client, trailID, reviewID string) (api.TrailReviewStateResponse, error) { @@ -662,30 +835,30 @@ func verifyTrailReviewHead(ctx context.Context, state api.TrailReviewStateRespon return err } if got != want { - return fmt.Errorf("review was created for head %s, but current HEAD is %s; check out the reviewed commit or start a new review", abbreviate12(want), abbreviate12(got)) + 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 } func applyTrailReviewSuggestions(ctx context.Context, comment api.TrailReviewComment, checkOnly bool, w io.Writer) (int, error) { if len(comment.SuggestedChanges) == 0 { - return 0, fmt.Errorf("comment %s has no suggested changes", comment.ID) + return 0, fmt.Errorf("finding %s has no suggested changes", comment.ID) } combinedPatch, supported, err := combinedSafeUnifiedDiffPatch(comment, w) if err != nil { return 0, err } if supported == 0 { - return 0, fmt.Errorf("comment %s has no supported unified_diff suggested changes", comment.ID) + return 0, fmt.Errorf("finding %s has no supported unified_diff suggested changes", comment.ID) } if err := runGitApply(ctx, combinedPatch, true); err != nil { - return 0, fmt.Errorf("suggested changes for comment %s do not apply cleanly: %w", comment.ID, err) + return 0, fmt.Errorf("suggested changes for finding %s do not apply cleanly: %w", comment.ID, err) } if checkOnly { return supported, nil } if err := runGitApply(ctx, combinedPatch, false); err != nil { - return 0, fmt.Errorf("apply suggested changes for comment %s: %w", comment.ID, err) + return 0, fmt.Errorf("apply suggested changes for finding %s: %w", comment.ID, err) } return supported, nil } @@ -816,7 +989,7 @@ func patchTrailReviewCommentStatus(ctx context.Context, client *api.Client, trai } resp, err := client.Patch(ctx, trailReviewCommentPath(trailID, comment.ReviewID, comment.ID), body) if err != nil { - return api.TrailReviewComment{}, fmt.Errorf("update review comment: %w", err) + return api.TrailReviewComment{}, fmt.Errorf("update finding: %w", err) } defer resp.Body.Close() if err := checkTrailResponse(resp); err != nil { @@ -824,7 +997,7 @@ func patchTrailReviewCommentStatus(ctx context.Context, client *api.Client, trai } var updated api.TrailReviewComment if err := api.DecodeJSON(resp, &updated); err != nil { - return api.TrailReviewComment{}, fmt.Errorf("decode updated review comment: %w", err) + return api.TrailReviewComment{}, fmt.Errorf("decode updated finding: %w", err) } return updated, nil } @@ -833,63 +1006,6 @@ func trailReviewCommentPath(trailID, reviewID, commentID string) string { return "/api/v1/trails/" + url.PathEscape(trailID) + "/reviews/" + url.PathEscape(reviewID) + "/comments/" + url.PathEscape(commentID) } -func startTrailReview(ctx context.Context, client *api.Client, trailID string, request api.TrailReviewStartRequest, idempotencyKey string) (api.TrailReviewStartResponse, error) { - headers := http.Header{} - if idempotencyKey != "" { - headers.Set("Idempotency-Key", idempotencyKey) - } - resp, err := client.PostWithHeaders(ctx, trailReviewStartPath(trailID), request, headers) - if err != nil { - return api.TrailReviewStartResponse{}, fmt.Errorf("start review: %w", err) - } - defer resp.Body.Close() - if err := checkTrailResponse(resp); err != nil { - return api.TrailReviewStartResponse{}, err - } - var out api.TrailReviewStartResponse - if err := api.DecodeJSON(resp, &out); err != nil { - return api.TrailReviewStartResponse{}, fmt.Errorf("decode review start response: %w", err) - } - return out, nil -} - -func trailReviewStartPath(trailID string) string { - return "/api/v1/trails/" + url.PathEscape(trailID) + "/reviews" -} - -func buildTrailReviewStartRequest(ctx context.Context, target trailReviewTarget, opts trailReviewStartOptions) (api.TrailReviewStartRequest, error) { - baseRef := strings.TrimSpace(opts.BaseRef) - if baseRef == "" { - baseRef = target.Trail.Base - } - headRef := strings.TrimSpace(opts.HeadRef) - if headRef == "" { - if branch, err := GetCurrentBranch(ctx); err == nil { - headRef = branch - } - } - headSHA := strings.TrimSpace(opts.HeadSHA) - if headSHA == "" { - sha, err := resolveGitRev(ctx, "HEAD") - if err != nil { - return api.TrailReviewStartRequest{}, err - } - headSHA = sha - } - baseSHA := strings.TrimSpace(opts.BaseSHA) - if baseSHA == "" && baseRef != "" { - if sha, err := resolveGitRev(ctx, baseRef); err == nil { - baseSHA = sha - } - } - return api.TrailReviewStartRequest{ - HeadSHA: optionalStringPtr(headSHA), - BaseSHA: optionalStringPtr(baseSHA), - BaseRef: optionalStringPtr(baseRef), - HeadRef: optionalStringPtr(headRef), - }, nil -} - func resolveGitRev(ctx context.Context, ref string) (string, error) { root, err := paths.WorktreeRoot(ctx) if err != nil { @@ -907,45 +1023,27 @@ func resolveGitRev(ctx context.Context, ref string) (string, error) { return sha, nil } -func trailReviewStartIdempotencyKey(trailID string, request api.TrailReviewStartRequest) string { - parts := []string{"trail-review-start", trailID, optionalStringValue(request.BaseSHA), optionalStringValue(request.HeadSHA), optionalStringValue(request.BaseRef), optionalStringValue(request.HeadRef)} - return strings.Join(parts, ":") -} - -func submitTrailReview(ctx context.Context, client *api.Client, target trailReviewTarget, event, body string) (api.TrailSubmitReviewResponse, error) { - path := trailsBasePath(target.Host, target.Owner, target.Repo) + "/" + strconv.Itoa(target.Trail.Number) + "/review" - resp, err := client.Post(ctx, path, api.TrailSubmitReviewRequest{Event: event, Body: body}) - if err != nil { - return api.TrailSubmitReviewResponse{}, fmt.Errorf("submit trail review: %w", err) - } - defer resp.Body.Close() - if err := checkTrailResponse(resp); err != nil { - return api.TrailSubmitReviewResponse{}, err - } - var out api.TrailSubmitReviewResponse - if err := api.DecodeJSON(resp, &out); err != nil { - return api.TrailSubmitReviewResponse{}, fmt.Errorf("decode trail review response: %w", err) - } - return out, nil -} - func encodeTrailReviewJSON(w io.Writer, target trailReviewTarget, comments []api.TrailReviewComment, hasMore bool, counts trailReviewCommentCounts) error { enc := json.NewEncoder(w) enc.SetIndent("", " ") if err := enc.Encode(map[string]any{ "trail": target.Trail, "counts": counts, - "comments": comments, + "findings": comments, "has_more": hasMore, }); err != nil { - return fmt.Errorf("encode trail review JSON: %w", err) + return fmt.Errorf("encode trail findings JSON: %w", err) } return nil } func printTrailReviewDashboard(w io.Writer, target trailReviewTarget, comments []api.TrailReviewComment, hasMore bool, opts trailReviewListOptions, counts trailReviewCommentCounts) { trail := target.Trail - fmt.Fprintf(w, "Trail #%d %s\n", trail.Number, trail.Title) + if trail.Number > 0 { + fmt.Fprintf(w, "Trail #%d %s\n", trail.Number, trail.Title) + } else { + fmt.Fprintf(w, "Trail %s %s\n", trail.ID, trail.Title) + } fmt.Fprintf(w, "Status: %s Branch: %s Base: %s\n", trail.Status, trail.Branch, trail.Base) fmt.Fprintf(w, "ID: %s\n\n", trail.ID) @@ -957,7 +1055,7 @@ func printTrailReviewDashboard(w io.Writer, target trailReviewTarget, comments [ fmt.Fprintln(w) if len(comments) == 0 { - fmt.Fprintln(w, "No review findings match the current filters.") + fmt.Fprintln(w, "No findings match the current filters.") return } @@ -975,16 +1073,17 @@ func printTrailReviewDashboard(w io.Writer, target trailReviewTarget, comments [ } fmt.Fprintln(w, "Actions:") - fmt.Fprintln(w, " entire trail review show ") - fmt.Fprintln(w, " entire trail review apply --resolve") - fmt.Fprintln(w, " entire trail review resolve -m \"fixed in \"") - fmt.Fprintln(w, " entire trail review dismiss -m \"not applicable\"") - fmt.Fprintln(w, " entire trail review watch") + fmt.Fprintln(w, " entire trail finding show ") + fmt.Fprintln(w, " entire trail finding add -m \"finding body\" --file path --line 42") + fmt.Fprintln(w, " entire trail finding apply --resolve") + fmt.Fprintln(w, " entire trail finding resolve -m \"fixed in \"") + fmt.Fprintln(w, " entire trail finding dismiss -m \"not applicable\"") + fmt.Fprintln(w, " entire trail finding watch") } func printTrailReviewComments(w io.Writer, comments []api.TrailReviewComment, hasMore bool) { if len(comments) == 0 { - fmt.Fprintln(w, "No review findings found.") + fmt.Fprintln(w, "No findings found.") return } tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) @@ -998,9 +1097,18 @@ func printTrailReviewComments(w io.Writer, comments []api.TrailReviewComment, ha } } +func printTrailReviewCommentCreated(w io.Writer, target trailReviewTarget, comment api.TrailReviewComment) { + fmt.Fprintf(w, "Created finding %s on %s\n", comment.ID, trailReviewTargetDisplay(target)) + fmt.Fprintf(w, "Status: %s\n", comment.Status) + fmt.Fprintf(w, "Severity: %s\n", severityDisplay(comment.Severity)) + fmt.Fprintf(w, "Location: %s\n", trailReviewLocationDisplay(comment.Location)) + if title := trailReviewCommentTitle(comment); title != "" { + fmt.Fprintf(w, "Title: %s\n", title) + } +} + func printTrailReviewCommentDetail(w io.Writer, comment api.TrailReviewComment) { - fmt.Fprintf(w, "Comment: %s\n", comment.ID) - fmt.Fprintf(w, "Review: %s\n", comment.ReviewID) + fmt.Fprintf(w, "Finding: %s\n", comment.ID) fmt.Fprintf(w, "Status: %s\n", comment.Status) fmt.Fprintf(w, "Severity: %s\n", severityDisplay(comment.Severity)) if comment.Confidence != nil { @@ -1030,17 +1138,14 @@ func printTrailReviewCommentDetail(w io.Writer, comment api.TrailReviewComment) } } -func printTrailReviewStart(w io.Writer, target trailReviewTarget, started api.TrailReviewStartResponse) { - fmt.Fprintf(w, "Started review %s for trail #%d (%s)\n", started.ReviewID, target.Trail.Number, target.Trail.Title) - fmt.Fprintf(w, " Code version: %s\n", started.CodeVersionID) - if started.BaseSHA != nil || started.HeadSHA != nil { - fmt.Fprintf(w, " Base: %s\n", abbreviate12(optionalStringValue(started.BaseSHA))) - fmt.Fprintf(w, " Head: %s\n", abbreviate12(optionalStringValue(started.HeadSHA))) +func trailReviewTargetDisplay(target trailReviewTarget) string { + if target.Trail.Number > 0 { + return fmt.Sprintf("trail #%d (%s)", target.Trail.Number, target.Trail.Title) } - fmt.Fprintln(w) - fmt.Fprintln(w, "Next:") - fmt.Fprintf(w, " entire trail review watch %d\n", target.Trail.Number) - fmt.Fprintf(w, " entire trail review comments %d\n", target.Trail.Number) + if target.Trail.Branch != "" { + return fmt.Sprintf("trail %s on %s", target.Trail.ID, target.Trail.Branch) + } + return "trail " + target.Trail.ID } type trailReviewCommentCounts struct { @@ -1167,26 +1272,42 @@ func defaultTrailReviewStatusReason(status string) string { } } -func parseOptionalTrailNumber(args []string) (int, error) { +func parseOptionalTrailSelector(args []string, flagSelector string) (string, error) { + flagSelector = strings.TrimSpace(flagSelector) if len(args) == 0 { - return 0, nil + return flagSelector, nil } - n, err := strconv.Atoi(args[0]) - if err != nil || n <= 0 { - return 0, fmt.Errorf("invalid trail number %q", args[0]) + if flagSelector != "" { + return "", errors.New("pass a trail either positionally or with --trail, not both") } - return n, nil + selector := strings.TrimSpace(args[0]) + if selector == "" { + return "", errors.New("trail selector cannot be empty") + } + return selector, nil } -func parseTrailNumberAndCommentID(args []string) (int, string, error) { +func parseTrailSelectorAndCommentID(args []string, flagSelector string) (string, string, error) { + flagSelector = strings.TrimSpace(flagSelector) if len(args) == 1 { - return 0, args[0], nil + commentID := strings.TrimSpace(args[0]) + if commentID == "" { + return "", "", errors.New("finding id cannot be empty") + } + return flagSelector, commentID, nil } - n, err := parseOptionalTrailNumber(args[:1]) - if err != nil { - return 0, "", err + if flagSelector != "" { + return "", "", errors.New("pass a trail either positionally or with --trail, not both") + } + selector := strings.TrimSpace(args[0]) + commentID := strings.TrimSpace(args[1]) + if selector == "" { + return "", "", errors.New("trail selector cannot be empty") + } + if commentID == "" { + return "", "", errors.New("finding id cannot be empty") } - return n, args[1], nil + return selector, commentID, nil } func optionalStringPtr(s string) *string { diff --git a/cmd/entire/cli/trail_review_cmd_test.go b/cmd/entire/cli/trail_review_cmd_test.go index 9c46f929e..5a3114a6d 100644 --- a/cmd/entire/cli/trail_review_cmd_test.go +++ b/cmd/entire/cli/trail_review_cmd_test.go @@ -14,9 +14,54 @@ import ( "github.com/entireio/cli/cmd/entire/cli/api" "github.com/entireio/cli/cmd/entire/cli/paths" + + "github.com/spf13/cobra" +) + +const ( + trailReviewApplyOriginalContent = "hello\nold\n" + trailReviewTestCommentID = "cmt_1" ) -const trailReviewApplyOriginalContent = "hello\nold\n" +func TestTrailCommandSurfaceUsesFindings(t *testing.T) { + trailCmd := newTrailCmd() + children := map[string]*cobra.Command{} + for _, child := range trailCmd.Commands() { + children[child.Name()] = child + } + findingCmd := children["finding"] + if findingCmd == nil { + t.Fatal("trail command did not register finding subcommand") + } + if children["review"] != nil { + t.Fatal("trail command should not register review subcommand") + } + + subcommands := map[string]bool{} + for _, child := range findingCmd.Commands() { + subcommands[child.Name()] = true + } + for _, required := range []string{"list", "add", "show", "apply", "resolve", "dismiss", "reopen", "watch"} { + if !subcommands[required] { + t.Fatalf("trail finding missing %q subcommand", required) + } + } + for _, removed := range []string{"start", "comments", "approve", "request-changes"} { + if subcommands[removed] { + t.Fatalf("trail finding should not register removed %q subcommand", removed) + } + } +} + +func TestTrailCommandRejectsRemovedReviewCommand(t *testing.T) { + cmd := newTrailCmd() + cmd.SetArgs([]string{"review"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + if err := cmd.Execute(); err == nil { + t.Fatal("expected removed trail review command to error") + } +} func TestTrailReviewCommentsPath(t *testing.T) { got := trailReviewCommentsPath("trail id/with slash", trailReviewListOptions{ @@ -33,6 +78,114 @@ func TestTrailReviewCommentsPath(t *testing.T) { } } +func TestParseTrailSelectorAndCommentID(t *testing.T) { + selector, commentID, err := parseTrailSelectorAndCommentID([]string{trailReviewTestCommentID}, "425") + if err != nil { + t.Fatalf("parseTrailSelectorAndCommentID with --trail: %v", err) + } + if selector != "425" || commentID != trailReviewTestCommentID { + t.Fatalf("selector=%q commentID=%q, want 425/cmt_1", selector, commentID) + } + + selector, commentID, err = parseTrailSelectorAndCommentID([]string{"feat/review", "cmt_2"}, "") + if err != nil { + t.Fatalf("parseTrailSelectorAndCommentID positional: %v", err) + } + if selector != "feat/review" || commentID != "cmt_2" { + t.Fatalf("selector=%q commentID=%q, want feat/review/cmt_2", selector, commentID) + } + + if _, _, err := parseTrailSelectorAndCommentID([]string{"425", trailReviewTestCommentID}, "trl_1"); err == nil { + t.Fatal("expected error when both positional trail and --trail are provided") + } +} + +func TestLoadTrailReviewCommentPatchFile(t *testing.T) { + opts, err := loadTrailReviewCommentPatchFile(trailReviewCommentAddOptions{PatchFile: "-"}, strings.NewReader("diff --git a/file.txt b/file.txt\n")) + if err != nil { + t.Fatalf("loadTrailReviewCommentPatchFile: %v", err) + } + if opts.Patch != "diff --git a/file.txt b/file.txt\n" { + t.Fatalf("Patch = %q", opts.Patch) + } + + if _, err := loadTrailReviewCommentPatchFile(trailReviewCommentAddOptions{Patch: "inline", PatchFile: "-"}, strings.NewReader("patch")); err == nil { + t.Fatal("expected error when --patch and --patch-file are both provided") + } +} + +func TestBuildTrailReviewCommentCreateRequest(t *testing.T) { + req, err := buildTrailReviewCommentCreateRequest(trailReviewCommentAddOptions{ + Title: "Missing expiry skew handling", + Body: "Token refresh should allow clock skew.", + Severity: "HIGH", + Confidence: 0.94, + FilePath: "src/auth/session.ts", + StartLine: 88, + EndLine: 91, + ClientID: "agent-run-1:finding-7", + Instruction: "Allow a five minute skew.", + }) + if err != nil { + t.Fatalf("buildTrailReviewCommentCreateRequest: %v", err) + } + if req.Body != "Token refresh should allow clock skew." { + t.Fatalf("Body = %q", req.Body) + } + if req.Title == nil || *req.Title != "Missing expiry skew handling" { + t.Fatalf("Title = %#v", req.Title) + } + if req.Severity == nil || *req.Severity != trailReviewSeverityHigh { + t.Fatalf("Severity = %#v", req.Severity) + } + if req.Confidence == nil || *req.Confidence != 0.94 { + t.Fatalf("Confidence = %#v", req.Confidence) + } + if req.ClientID == nil || *req.ClientID != "agent-run-1:finding-7" { + t.Fatalf("ClientID = %#v", req.ClientID) + } + if req.Location.Granularity != "range" || req.Location.FilePath == nil || *req.Location.FilePath != "src/auth/session.ts" { + t.Fatalf("Location = %#v", req.Location) + } + if req.Location.StartLine == nil || *req.Location.StartLine != 88 || req.Location.EndLine == nil || *req.Location.EndLine != 91 { + t.Fatalf("Location lines = %#v", req.Location) + } + if len(req.SuggestedChanges) != 1 || req.SuggestedChanges[0].ChangeType != "manual_instruction" { + t.Fatalf("SuggestedChanges = %#v", req.SuggestedChanges) + } +} + +func TestCreateTrailReviewCommentPostsTrailScopedPath(t *testing.T) { + var gotBody api.TrailReviewCommentCreateRequest + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.Path != "/api/v1/trails/trl_1/reviews/comments" { + t.Fatalf("unexpected request %s %s", r.Method, r.URL.String()) + } + if err := json.NewDecoder(r.Body).Decode(&gotBody); err != nil { + t.Fatalf("decode body: %v", err) + } + encodeTrailReviewTestJSON(t, w, api.TrailReviewComment{ID: trailReviewTestCommentID, TrailID: "trl_1", Status: trailReviewStatusOpen}) + })) + defer srv.Close() + t.Setenv(api.BaseURLEnvVar, srv.URL) + client := api.NewClient("tok") + + created, err := createTrailReviewComment(context.Background(), client, "trl_1", api.TrailReviewCommentCreateRequest{ + Body: "body", + ClientID: trailReviewStrPtr("agent-run-1:finding-1"), + Location: api.TrailReviewLocationCreateRequest{Granularity: "whole_change"}, + }) + if err != nil { + t.Fatalf("createTrailReviewComment: %v", err) + } + if created.ID != trailReviewTestCommentID { + t.Fatalf("created.ID = %q", created.ID) + } + if gotBody.Body != "body" || gotBody.ClientID == nil || *gotBody.ClientID != "agent-run-1:finding-1" { + t.Fatalf("request body = %#v", gotBody) + } +} + func TestPrintTrailReviewDashboard(t *testing.T) { high := trailReviewSeverityHigh medium := trailReviewSeverityMedium @@ -103,7 +256,7 @@ func TestPrintTrailReviewDashboard_UsesSeparateCountsWhenFilteredCommentsEmpty(t for _, want := range []string{ "Open findings: 0 high 0 medium 0 low 0", "Resolved: 1 Dismissed: 1 Stale: 1", - "No review findings match the current filters.", + "No findings match the current filters.", } { if !strings.Contains(text, want) { t.Fatalf("dashboard missing %q:\n%s", want, text) @@ -120,13 +273,13 @@ func TestFetchTrailReviewCommentsAndPatchStatus(t *testing.T) { t.Fatalf("status query = %q, want open", got) } encodeTrailReviewTestJSON(t, w, api.TrailReviewCommentsResponse{Comments: []api.TrailReviewComment{ - {ID: "cmt_1", TrailID: "trl_1", ReviewID: "rvw_1", Status: trailReviewStatusOpen, Location: api.TrailReviewLocation{Granularity: "whole_change"}}, + {ID: trailReviewTestCommentID, TrailID: "trl_1", ReviewID: "rvw_1", Status: trailReviewStatusOpen, Location: api.TrailReviewLocation{Granularity: "whole_change"}}, }}) case r.Method == http.MethodPatch && r.URL.Path == "/api/v1/trails/trl_1/reviews/rvw_1/comments/cmt_1": if err := json.NewDecoder(r.Body).Decode(&gotPatchBody); err != nil { t.Fatalf("decode patch body: %v", err) } - encodeTrailReviewTestJSON(t, w, api.TrailReviewComment{ID: "cmt_1", TrailID: "trl_1", ReviewID: "rvw_1", Status: trailReviewStatusResolved}) + encodeTrailReviewTestJSON(t, w, api.TrailReviewComment{ID: trailReviewTestCommentID, TrailID: "trl_1", ReviewID: "rvw_1", Status: trailReviewStatusResolved}) default: t.Fatalf("unexpected request %s %s", r.Method, r.URL.String()) } @@ -139,7 +292,7 @@ func TestFetchTrailReviewCommentsAndPatchStatus(t *testing.T) { if err != nil { t.Fatalf("fetchTrailReviewComments: %v", err) } - if hasMore || len(comments) != 1 || comments[0].ID != "cmt_1" { + if hasMore || len(comments) != 1 || comments[0].ID != trailReviewTestCommentID { t.Fatalf("comments = %#v, hasMore=%v", comments, hasMore) } updated, err := patchTrailReviewCommentStatus(context.Background(), client, "trl_1", comments[0], trailReviewStatusResolved, "fixed") @@ -167,7 +320,7 @@ func TestFetchTrailReviewStateFollowsCursor(t *testing.T) { encodeTrailReviewTestJSON(t, w, api.TrailReviewStateResponse{ Review: api.TrailReview{ID: "rvw_1"}, CodeVersion: api.TrailReviewCodeVersion{ID: "cv_1"}, - Comments: []api.TrailReviewComment{{ID: "cmt_1"}}, + Comments: []api.TrailReviewComment{{ID: trailReviewTestCommentID}}, NextCursor: &next, }) case "cursor-2": @@ -191,7 +344,7 @@ func TestFetchTrailReviewStateFollowsCursor(t *testing.T) { if requests != 2 { t.Fatalf("requests = %d, want 2", requests) } - if len(state.Comments) != 2 || state.Comments[0].ID != "cmt_1" || state.Comments[1].ID != "cmt_2" { + if len(state.Comments) != 2 || state.Comments[0].ID != trailReviewTestCommentID || state.Comments[1].ID != "cmt_2" { t.Fatalf("comments = %#v", state.Comments) } if state.NextCursor != nil { @@ -324,7 +477,7 @@ func trailReviewApplyComment(patches ...string) api.TrailReviewComment { Patch: trailReviewStrPtr(patch), } } - return api.TrailReviewComment{ID: "cmt_1", SuggestedChanges: changes} + return api.TrailReviewComment{ID: trailReviewTestCommentID, SuggestedChanges: changes} } func trailReviewPatch(file, oldText string) string { @@ -344,34 +497,4 @@ func encodeTrailReviewTestJSON(t *testing.T, w http.ResponseWriter, v any) { } } -func TestStartTrailReviewSendsIdempotencyKey(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost || r.URL.Path != "/api/v1/trails/trl_1/reviews" { - t.Fatalf("unexpected request %s %s", r.Method, r.URL.String()) - } - if got := r.Header.Get("Idempotency-Key"); got != "key-1" { - t.Fatalf("Idempotency-Key = %q, want key-1", got) - } - var body api.TrailReviewStartRequest - if err := json.NewDecoder(r.Body).Decode(&body); err != nil { - t.Fatalf("decode body: %v", err) - } - if body.HeadSHA == nil || *body.HeadSHA != "abc" { - t.Fatalf("HeadSHA = %#v", body.HeadSHA) - } - encodeTrailReviewTestJSON(t, w, api.TrailReviewStartResponse{ReviewID: "rvw_1", TrailID: "trl_1", CodeVersionID: "cv_1"}) - })) - defer srv.Close() - t.Setenv(api.BaseURLEnvVar, srv.URL) - client := api.NewClient("tok") - - started, err := startTrailReview(context.Background(), client, "trl_1", api.TrailReviewStartRequest{HeadSHA: trailReviewStrPtr("abc")}, "key-1") - if err != nil { - t.Fatalf("startTrailReview: %v", err) - } - if started.ReviewID != "rvw_1" { - t.Fatalf("ReviewID = %q", started.ReviewID) - } -} - func trailReviewStrPtr(s string) *string { return &s } diff --git a/cmd/entire/cli/trail_watch_cmd.go b/cmd/entire/cli/trail_watch_cmd.go index 287295694..23e48334c 100644 --- a/cmd/entire/cli/trail_watch_cmd.go +++ b/cmd/entire/cli/trail_watch_cmd.go @@ -19,7 +19,7 @@ import ( "github.com/spf13/cobra" ) -// SSE control events for the agent-native code-review stream. Domain events +// SSE control events for the agent-native finding stream. Domain events // (for example "session.started" or "comment.created") are emitted as their // code_review_events.event_type values. const ( @@ -48,8 +48,8 @@ func newTrailWatchCmd() *cobra.Command { cmd := &cobra.Command{ Use: "watch []", - Short: "Tail a trail's code-review events live", - Long: `Subscribe to the trail-scoped agent-native code-review SSE stream and + Short: "Tail a trail's finding events live", + Long: `Subscribe to the trail-scoped agent-native finding SSE stream and print events as they arrive. Reconnects automatically when the server caps the connection (~50s) and on transient network errors. @@ -60,7 +60,7 @@ GET /api/v1/trails//reviews/events with Accept: text/event-stream. Events emitted by the server: ready initial frame, includes trail and cursor - code-review domain event (session.started, comment.created, ...) + finding domain event (sessions, findings, suggested changes, ...) reconnect server cap reached; re-establishing forbidden access was revoked; stream ends error server-side error; treated as reconnect`, @@ -90,9 +90,6 @@ func runTrailWatch(cmd *cobra.Command, number int, jsonOutput, showPings, once b func runTrailWatchWithOptions(cmd *cobra.Command, number int, jsonOutput, showPings, once bool) error { ctx := cmd.Context() - w := cmd.OutOrStdout() - errW := cmd.ErrOrStderr() - client, err := NewAuthenticatedAPIClient(ctx, trailInsecureHTTP(cmd)) if err != nil { return fmt.Errorf("authentication required: %w", err) @@ -102,6 +99,13 @@ func runTrailWatchWithOptions(cmd *cobra.Command, number int, jsonOutput, showPi if err != nil { return err } + return runTrailWatchResolved(cmd, client, trailID, description, jsonOutput, showPings, once) +} + +func runTrailWatchResolved(cmd *cobra.Command, client *api.Client, trailID, description string, jsonOutput, showPings, once bool) error { + ctx := cmd.Context() + w := cmd.OutOrStdout() + errW := cmd.ErrOrStderr() streamPath := reviewEventsPath(trailID) fmt.Fprintf(errW, "Watching %s — Ctrl+C to stop\n", description) @@ -525,34 +529,34 @@ func printReviewStreamEvent(w io.Writer, ev reviewStreamEvent) { severity := payloadString(ev.Payload, "severity") switch { case file != "" && severity != "": - fmt.Fprintf(w, "%scomment created by %s on %s (%s) — %s\n", prefix, actor, file, severity, ev.TargetID) + fmt.Fprintf(w, "%sfinding created by %s on %s (%s) — %s\n", prefix, actor, file, severity, ev.TargetID) case file != "": - fmt.Fprintf(w, "%scomment created by %s on %s — %s\n", prefix, actor, file, ev.TargetID) + fmt.Fprintf(w, "%sfinding created by %s on %s — %s\n", prefix, actor, file, ev.TargetID) default: - fmt.Fprintf(w, "%scomment created by %s — %s\n", prefix, actor, ev.TargetID) + fmt.Fprintf(w, "%sfinding created by %s — %s\n", prefix, actor, ev.TargetID) } case "comment.status_changed": - fmt.Fprintf(w, "%scomment %s status %s → %s\n", prefix, ev.TargetID, payloadString(ev.Payload, "from"), payloadString(ev.Payload, "to")) + fmt.Fprintf(w, "%sfinding %s status %s → %s\n", prefix, ev.TargetID, payloadString(ev.Payload, "from"), payloadString(ev.Payload, "to")) case "comment.updated": - fmt.Fprintf(w, "%scomment %s updated by %s\n", prefix, ev.TargetID, actor) + fmt.Fprintf(w, "%sfinding %s updated by %s\n", prefix, ev.TargetID, actor) case "comment.stale_checked": - fmt.Fprintf(w, "%scomment %s marked %s (%s)\n", prefix, ev.TargetID, payloadString(ev.Payload, "outcome"), payloadString(ev.Payload, "reason")) + fmt.Fprintf(w, "%sfinding %s marked %s (%s)\n", prefix, ev.TargetID, payloadString(ev.Payload, "outcome"), payloadString(ev.Payload, "reason")) case "suggested_change.created": - fmt.Fprintf(w, "%ssuggested change %s created for comment %s (%s)\n", prefix, ev.TargetID, payloadString(ev.Payload, "review_comment_id"), payloadString(ev.Payload, "change_type")) + fmt.Fprintf(w, "%ssuggested change %s created for finding %s (%s)\n", prefix, ev.TargetID, payloadString(ev.Payload, "review_comment_id"), payloadString(ev.Payload, "change_type")) case "suggested_change.updated": fmt.Fprintf(w, "%ssuggested change %s updated by %s\n", prefix, ev.TargetID, actor) case "suggested_change.check_result", "suggested_change.apply_result": fmt.Fprintf(w, "%s%s for %s: %s\n", prefix, ev.EventType, payloadString(ev.Payload, "suggested_change_id"), payloadString(ev.Payload, "status")) case "thread.created": - fmt.Fprintf(w, "%sthread %s created for comment %s\n", prefix, ev.TargetID, payloadString(ev.Payload, "review_comment_id")) + fmt.Fprintf(w, "%sthread %s created for finding %s\n", prefix, ev.TargetID, payloadString(ev.Payload, "review_comment_id")) case "thread.message_added": fmt.Fprintf(w, "%sthread message %s added by %s\n", prefix, ev.TargetID, actor) case "thread.message_edited": fmt.Fprintf(w, "%sthread message %s edited by %s\n", prefix, ev.TargetID, actor) case "comment.linked": - fmt.Fprintf(w, "%scomment link created: %s → %s\n", prefix, payloadString(ev.Payload, "source_comment_id"), payloadString(ev.Payload, "target_comment_id")) + fmt.Fprintf(w, "%sfinding link created: %s → %s\n", prefix, payloadString(ev.Payload, "source_comment_id"), payloadString(ev.Payload, "target_comment_id")) case "comment.unlinked": - fmt.Fprintf(w, "%scomment link removed: %s → %s\n", prefix, payloadString(ev.Payload, "source_comment_id"), payloadString(ev.Payload, "target_comment_id")) + fmt.Fprintf(w, "%sfinding link removed: %s → %s\n", prefix, payloadString(ev.Payload, "source_comment_id"), payloadString(ev.Payload, "target_comment_id")) default: fmt.Fprintf(w, "%s%s %s/%s by %s\n", prefix, ev.EventType, ev.TargetType, ev.TargetID, actor) } diff --git a/cmd/entire/cli/trail_watch_cmd_test.go b/cmd/entire/cli/trail_watch_cmd_test.go index 6d7528a81..511aad8a5 100644 --- a/cmd/entire/cli/trail_watch_cmd_test.go +++ b/cmd/entire/cli/trail_watch_cmd_test.go @@ -75,7 +75,7 @@ func TestStreamOnce_PrintsReadyAndReviewEvents(t *testing.T) { t.Errorf("lastID = %q, want %q", lastID, "3") } out := stdout.String() - for _, want := range []string{"trail trl_1", "session started", "comment created", "src/foo.ts", "session ended"} { + for _, want := range []string{"trail trl_1", "session started", "finding created", "src/foo.ts", "session ended"} { if !strings.Contains(out, want) { t.Errorf("expected %q in output, got: %q", want, out) } diff --git a/e2e/README.md b/e2e/README.md index 84e556086..abcb0539c 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -25,6 +25,7 @@ e2e/ ├── agents/ # Agent abstraction (Agent interface, tmux sessions, concurrency gates) ├── bootstrap/ # CI pre-test setup (auth config, warmup) ├── entire/ # `entire` CLI wrapper (enable, rewind, etc.) +├── examples/ # Manual golden-path scripts, not run by CI ├── exploratory/ # Experimental tests, not run by CI ├── tests/ # Blessed test files (run by CI) └── testutil/ # Repo setup, assertions, artifact capture @@ -36,6 +37,7 @@ e2e/ - All operations go through `RepoState` (`s.RunPrompt`, `s.Git`) so they're logged to `console.log`. - Use the `entire` package for CLI interactions, not raw `exec.Command`. - Skip tests pending CLI fixes with `t.Skip("ENT-XXX: reason")`. +- Put manual, service-backed golden paths under `e2e/examples/`; they are documentation and release-verification scripts, not CI tests. ## Adding a New Agent diff --git a/e2e/examples/README.md b/e2e/examples/README.md new file mode 100644 index 000000000..a71f5ec82 --- /dev/null +++ b/e2e/examples/README.md @@ -0,0 +1,27 @@ +# E2E examples + +Manual end-to-end examples for workflows that touch real Entire services. + +These are **not run by CI**. They are intended as copy/pasteable golden paths for release verification and debugging. Run them from a git repository with an `origin` remote that resolves to a repo with trails, and authenticate first with `entire login`. + +## Trail finding golden path + +```bash +./e2e/examples/trail-finding-golden-path.sh +``` + +Optional environment variables: + +- `ENTIRE_BIN`: path to the CLI binary, defaults to `entire` +- `ENTIRE_TRAIL_FINDING_SELECTOR`: trail number, id, or branch; defaults to the current branch's trail +- `ENTIRE_TRAIL_FINDING_FILE`: file path for the example finding, defaults to `README.md` +- `ENTIRE_TRAIL_FINDING_LINE`: line for the example finding, defaults to `1` + +The script exercises the intended user/agent path: + +1. discover trails with `entire trail list --status any` +2. show finding dashboard for current branch/default target +3. create a trail-scoped finding with a `client_id` +4. list findings +5. show the created finding +6. resolve the created finding diff --git a/e2e/examples/trail-finding-golden-path.sh b/e2e/examples/trail-finding-golden-path.sh new file mode 100755 index 000000000..a314b9756 --- /dev/null +++ b/e2e/examples/trail-finding-golden-path.sh @@ -0,0 +1,77 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Golden path for manual trail-finding CLI verification. +# Run from a git repo with an origin remote and an authenticated Entire CLI. + +ENTIRE_BIN="${ENTIRE_BIN:-entire}" +TARGET_SELECTOR="${ENTIRE_TRAIL_FINDING_SELECTOR:-${ENTIRE_TRAIL_REVIEW_SELECTOR:-}}" +FINDING_FILE="${ENTIRE_TRAIL_FINDING_FILE:-${ENTIRE_TRAIL_REVIEW_FILE:-README.md}}" +FINDING_LINE="${ENTIRE_TRAIL_FINDING_LINE:-${ENTIRE_TRAIL_REVIEW_LINE:-1}}" +CLIENT_ID="trail-finding-e2e:$(date +%s):$$" + +target_args=() +if [[ -n "$TARGET_SELECTOR" ]]; then + target_args+=("$TARGET_SELECTOR") +fi + +run() { + printf '\n$ %q' "$ENTIRE_BIN" + printf ' %q' "$@" + printf '\n' + "$ENTIRE_BIN" "$@" +} + +need() { + if ! command -v "$1" >/dev/null 2>&1; then + echo "missing required command: $1" >&2 + exit 1 + fi +} + +need "$ENTIRE_BIN" +need jq + +echo "Trail finding golden path" +echo "Target: ${TARGET_SELECTOR:-current branch trail}" +echo "Finding location: ${FINDING_FILE}:${FINDING_LINE}" + +run trail list --status any --limit 10 +run trail finding "${target_args[@]}" + +printf '\n$ %q' "$ENTIRE_BIN" +printf ' %q' trail finding add "${target_args[@]}" \ + --json \ + --title "E2E finding" \ + --body "Golden-path finding created by the trail finding E2E example." \ + --severity medium \ + --confidence 0.9 \ + --file "$FINDING_FILE" \ + --line "$FINDING_LINE" \ + --client-id "$CLIENT_ID" +printf '\n' + +finding_json=$("$ENTIRE_BIN" trail finding add "${target_args[@]}" \ + --json \ + --title "E2E finding" \ + --body "Golden-path finding created by the trail finding E2E example." \ + --severity medium \ + --confidence 0.9 \ + --file "$FINDING_FILE" \ + --line "$FINDING_LINE" \ + --client-id "$CLIENT_ID") + +printf '%s\n' "$finding_json" | jq . +finding_id=$(printf '%s\n' "$finding_json" | jq -r '.id') +if [[ -z "$finding_id" || "$finding_id" == "null" ]]; then + echo "create finding response did not include .id" >&2 + exit 1 +fi + +run trail finding list "${target_args[@]}" --status any --include-dismissed +run trail finding show "${target_args[@]}" "$finding_id" +run trail finding resolve "${target_args[@]}" "$finding_id" -m "resolved by trail finding golden-path e2e example" +run trail finding list "${target_args[@]}" --status any --include-dismissed + +echo +printf 'Golden path complete. Created and resolved finding: %s\n' "$finding_id" From edaadf399fba83d91aaca9b69e2726a015f357c0 Mon Sep 17 00:00:00 2001 From: dip Date: Fri, 5 Jun 2026 20:17:18 +0200 Subject: [PATCH 6/6] refactor: address trail finding CLI review feedback - Remove duplicate optionalStringValue helper; use stringPtrValue - Inline pointless runTrailWatchWithOptions wrapper into runTrailWatch - Add t.Parallel() to pure-function trail finding tests - Guard .git path check with EqualFold for case-insensitive filesystems - Document why the apply tests use bare git init over testutil.InitRepo Co-Authored-By: Claude Opus 4.8 (1M context) Entire-Checkpoint: 138a8d212cff --- cmd/entire/cli/trail_review_cmd.go | 13 ++++--------- cmd/entire/cli/trail_review_cmd_test.go | 11 +++++++++++ cmd/entire/cli/trail_watch_cmd.go | 4 ---- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/cmd/entire/cli/trail_review_cmd.go b/cmd/entire/cli/trail_review_cmd.go index a66c3ffa2..064c9c504 100644 --- a/cmd/entire/cli/trail_review_cmd.go +++ b/cmd/entire/cli/trail_review_cmd.go @@ -826,7 +826,7 @@ func trailReviewStatePath(trailID, reviewID, cursor string) string { } func verifyTrailReviewHead(ctx context.Context, state api.TrailReviewStateResponse) error { - want := strings.TrimSpace(optionalStringValue(state.CodeVersion.HeadSHA)) + want := strings.TrimSpace(stringPtrValue(state.CodeVersion.HeadSHA)) if want == "" { return nil } @@ -953,7 +953,9 @@ func validatePatchPath(raw string) error { return fmt.Errorf("path %q escapes the repository", raw) } for _, part := range strings.Split(clean, "/") { - if part == ".git" { + // EqualFold guards case-insensitive filesystems (default macOS/Windows) + // where ".GIT/config" would otherwise slip past an exact match. + if strings.EqualFold(part, ".git") { return fmt.Errorf("path %q targets .git metadata", raw) } } @@ -1328,13 +1330,6 @@ func stringPtrValue(s *string) string { return *s } -func optionalStringValue(s *string) string { - if s == nil { - return "" - } - return *s -} - func abbreviate12(s string) string { const n = 12 if len(s) <= n { diff --git a/cmd/entire/cli/trail_review_cmd_test.go b/cmd/entire/cli/trail_review_cmd_test.go index 5a3114a6d..b53e99924 100644 --- a/cmd/entire/cli/trail_review_cmd_test.go +++ b/cmd/entire/cli/trail_review_cmd_test.go @@ -24,6 +24,7 @@ const ( ) func TestTrailCommandSurfaceUsesFindings(t *testing.T) { + t.Parallel() trailCmd := newTrailCmd() children := map[string]*cobra.Command{} for _, child := range trailCmd.Commands() { @@ -54,6 +55,7 @@ func TestTrailCommandSurfaceUsesFindings(t *testing.T) { } func TestTrailCommandRejectsRemovedReviewCommand(t *testing.T) { + t.Parallel() cmd := newTrailCmd() cmd.SetArgs([]string{"review"}) cmd.SetOut(io.Discard) @@ -64,6 +66,7 @@ func TestTrailCommandRejectsRemovedReviewCommand(t *testing.T) { } func TestTrailReviewCommentsPath(t *testing.T) { + t.Parallel() got := trailReviewCommentsPath("trail id/with slash", trailReviewListOptions{ Status: "open,resolved", Severity: "high,medium", @@ -79,6 +82,7 @@ func TestTrailReviewCommentsPath(t *testing.T) { } func TestParseTrailSelectorAndCommentID(t *testing.T) { + t.Parallel() selector, commentID, err := parseTrailSelectorAndCommentID([]string{trailReviewTestCommentID}, "425") if err != nil { t.Fatalf("parseTrailSelectorAndCommentID with --trail: %v", err) @@ -101,6 +105,7 @@ func TestParseTrailSelectorAndCommentID(t *testing.T) { } func TestLoadTrailReviewCommentPatchFile(t *testing.T) { + t.Parallel() opts, err := loadTrailReviewCommentPatchFile(trailReviewCommentAddOptions{PatchFile: "-"}, strings.NewReader("diff --git a/file.txt b/file.txt\n")) if err != nil { t.Fatalf("loadTrailReviewCommentPatchFile: %v", err) @@ -115,6 +120,7 @@ func TestLoadTrailReviewCommentPatchFile(t *testing.T) { } func TestBuildTrailReviewCommentCreateRequest(t *testing.T) { + t.Parallel() req, err := buildTrailReviewCommentCreateRequest(trailReviewCommentAddOptions{ Title: "Missing expiry skew handling", Body: "Token refresh should allow clock skew.", @@ -187,6 +193,7 @@ func TestCreateTrailReviewCommentPostsTrailScopedPath(t *testing.T) { } func TestPrintTrailReviewDashboard(t *testing.T) { + t.Parallel() high := trailReviewSeverityHigh medium := trailReviewSeverityMedium path := "src/auth/session.ts" @@ -239,6 +246,7 @@ func TestPrintTrailReviewDashboard(t *testing.T) { } func TestPrintTrailReviewDashboard_UsesSeparateCountsWhenFilteredCommentsEmpty(t *testing.T) { + t.Parallel() var out strings.Builder counts := countTrailReviewComments([]api.TrailReviewComment{ {ID: "resolved-1", Status: trailReviewStatusResolved}, @@ -432,6 +440,9 @@ func TestApplyTrailReviewSuggestions_RejectsGitMetadataPaths(t *testing.T) { func newTrailReviewApplyRepo(t *testing.T) string { t.Helper() dir := t.TempDir() + // Bare `git init` rather than testutil.InitRepo: these tests apply patches to + // the working tree without committing, so no user/GPG config is needed, and we + // must avoid testutil's core.autocrlf=true which rewrites patched LF to CRLF. runTrailReviewApplyGit(t, dir, "init") paths.ClearWorktreeRootCache() t.Chdir(dir) diff --git a/cmd/entire/cli/trail_watch_cmd.go b/cmd/entire/cli/trail_watch_cmd.go index 97a5ed276..907a3e34d 100644 --- a/cmd/entire/cli/trail_watch_cmd.go +++ b/cmd/entire/cli/trail_watch_cmd.go @@ -84,10 +84,6 @@ Events emitted by the server: } func runTrailWatch(cmd *cobra.Command, number int, jsonOutput, showPings, once bool) error { - return runTrailWatchWithOptions(cmd, number, jsonOutput, showPings, once) -} - -func runTrailWatchWithOptions(cmd *cobra.Command, number int, jsonOutput, showPings, once bool) error { ctx := cmd.Context() client, err := NewAuthenticatedAPIClient(ctx, trailInsecureHTTP(cmd)) if err != nil {