From f318232d6cb2e553974dc5f734957c888a5e41c4 Mon Sep 17 00:00:00 2001 From: Sohan Kshirsagar Date: Tue, 21 Apr 2026 16:01:51 -0700 Subject: [PATCH 1/7] feat: add command for local code review --- cmd/errors.go | 35 +++ cmd/review.go | 345 ++++++++++++++++++++++++ cmd/review_status.go | 83 ++++++ cmd/short_docs/review/overview.md | 63 +++++ go.mod | 2 + go.sum | 2 - internal/api/client.go | 6 + internal/api/code_review.go | 110 ++++++++ internal/review/filter.go | 149 +++++++++++ internal/review/filter_test.go | 70 +++++ internal/review/patch.go | 426 ++++++++++++++++++++++++++++++ internal/review/poll.go | 118 +++++++++ internal/review/preflight.go | 152 +++++++++++ main.go | 2 +- 14 files changed, 1560 insertions(+), 3 deletions(-) create mode 100644 cmd/review.go create mode 100644 cmd/review_status.go create mode 100644 cmd/short_docs/review/overview.md create mode 100644 internal/api/code_review.go create mode 100644 internal/review/filter.go create mode 100644 internal/review/filter_test.go create mode 100644 internal/review/patch.go create mode 100644 internal/review/poll.go create mode 100644 internal/review/preflight.go diff --git a/cmd/errors.go b/cmd/errors.go index 1d666014..e5514e65 100644 --- a/cmd/errors.go +++ b/cmd/errors.go @@ -7,6 +7,41 @@ import ( "github.com/Use-Tusk/tusk-cli/internal/api" ) +// ExitCodeError wraps an error with a specific process exit code. main.go +// unwraps this to pick the right os.Exit value; without it, Cobra-returned +// errors map to exit 1. +type ExitCodeError struct { + Code int + Err error +} + +func (e *ExitCodeError) Error() string { + if e == nil || e.Err == nil { + return "" + } + return e.Err.Error() +} + +func (e *ExitCodeError) Unwrap() error { + if e == nil { + return nil + } + return e.Err +} + +// ExitCodeOf returns the exit code embedded in err (or any wrapper in its +// chain), defaulting to 1 if none is present. +func ExitCodeOf(err error) int { + if err == nil { + return 0 + } + var ec *ExitCodeError + if errors.As(err, &ec) { + return ec.Code + } + return 1 +} + // formatApiError converts raw API errors into user-friendly messages with // actionable guidance. Non-API errors pass through unchanged. func formatApiError(err error) error { diff --git a/cmd/review.go b/cmd/review.go new file mode 100644 index 00000000..43f8d2f9 --- /dev/null +++ b/cmd/review.go @@ -0,0 +1,345 @@ +package cmd + +import ( + "context" + _ "embed" + "encoding/json" + "errors" + "fmt" + "os" + "strings" + "time" + + "github.com/spf13/cobra" + + "github.com/Use-Tusk/tusk-cli/internal/api" + "github.com/Use-Tusk/tusk-cli/internal/log" + "github.com/Use-Tusk/tusk-cli/internal/review" + "github.com/Use-Tusk/tusk-cli/internal/utils" + "github.com/Use-Tusk/tusk-cli/internal/version" + backend "github.com/Use-Tusk/tusk-drift-schemas/generated/go/backend" +) + +//go:embed short_docs/review/overview.md +var reviewOverviewContent string + +var ( + reviewRepo string + reviewBase string + reviewMinSeverity string + reviewExcludes []string + reviewIncludes []string + reviewJSON bool + reviewOutput string + reviewQuiet bool + // Used by the status subcommand only. + reviewStatusWatch bool +) + +var reviewCmd = &cobra.Command{ + Use: "review", + Short: "Run Tusk code review on your local working tree", + Long: utils.RenderMarkdown(reviewOverviewContent), + SilenceUsage: true, + RunE: runReview, +} + +func init() { + rootCmd.AddCommand(reviewCmd) + bindReviewFlags(reviewCmd) +} + +func bindReviewFlags(cmd *cobra.Command) { + cmd.Flags().StringVar(&reviewRepo, "repo", "", "Repository in owner/name format (defaults to git origin remote)") + cmd.Flags().StringVar(&reviewBase, "base", "", "Base ref or SHA to diff against (defaults to merge-base with origin/HEAD)") + cmd.Flags().StringVar(&reviewMinSeverity, "min-severity", "", "Minimum severity to surface: low|medium|high|critical") + cmd.Flags().StringArrayVar(&reviewExcludes, "exclude", nil, "Extra path glob(s) to exclude from the patch (repeatable)") + cmd.Flags().StringArrayVar(&reviewIncludes, "include", nil, "Cancel a default skip for matching files (repeatable)") + cmd.Flags().BoolVar(&reviewJSON, "json", false, "Write the result as JSON (to stdout or --output)") + cmd.Flags().StringVar(&reviewOutput, "output", "", "Write the result to a file instead of stdout") + cmd.Flags().BoolVar(&reviewQuiet, "quiet", false, "Suppress stderr progress output") + cmd.Flags().SortFlags = false +} + +// setupReviewCloud resolves auth (JWT or API key) and returns a client. +// Mirrors setupUnitCloud. +func setupReviewCloud() (*api.TuskClient, api.AuthOptions, error) { + client, authOptions, _, err := api.SetupCloud(context.Background(), false) + if err != nil { + return nil, api.AuthOptions{}, err + } + return client, authOptions, nil +} + +// resolveReviewRepo returns (owner, name). If repoFlag is set, it's parsed +// as "owner/name"; otherwise the origin remote is used. +func resolveReviewRepo(repoFlag string) (string, string, error) { + slug := repoFlag + if slug == "" { + detected, err := getOriginRepoSlug() + if err != nil { + return "", "", err + } + slug = detected + } + parts := strings.Split(slug, "/") + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return "", "", fmt.Errorf("invalid repo %q; expected owner/name", slug) + } + return parts[0], parts[1], nil +} + +func runReview(cmd *cobra.Command, args []string) error { + setupSignalHandling() + + ctx := context.Background() + + log.Debug("Starting tusk review", + "repo", reviewRepo, + "base", reviewBase, + "min-severity", reviewMinSeverity, + "json", reviewJSON, + "output", reviewOutput, + "quiet", reviewQuiet, + ) + + repoRoot, err := review.RepoRoot() + if err != nil { + return &ExitCodeError{Code: 2, Err: err} + } + + if err := review.Preflight(repoRoot); err != nil { + if review.IsPreflightError(err) { + return &ExitCodeError{Code: 2, Err: err} + } + return err + } + + owner, name, err := resolveReviewRepo(reviewRepo) + if err != nil { + return &ExitCodeError{Code: 2, Err: err} + } + + client, authOptions, err := setupReviewCloud() + if err != nil { + return err + } + + patch, err := review.BuildPatch(ctx, review.PatchOptions{ + RepoRoot: repoRoot, + Base: reviewBase, + ExtraExcludes: reviewExcludes, + Includes: reviewIncludes, + RegisterCleanup: RegisterCleanup, + }) + if err != nil { + if errors.Is(err, review.ErrEmptyPatch) { + log.Println("Nothing to review: all changed files were filtered out (lockfiles, build artifacts, etc.).\nPass --include '' to override the default skip list.") + return nil + } + return mapPatchError(err) + } + + // Quick stderr header for non-TTY callers so they know something's happening + // even before the first progress poll. The backend will replace this with + // richer phase text once it starts rendering. + if !reviewQuiet { + baseLabel := patch.BaseRef + if baseLabel == "" { + baseLabel = patch.BaseSha + } + shortSha := patch.BaseSha + if len(shortSha) > 7 { + shortSha = shortSha[:7] + } + log.Stderrln(fmt.Sprintf("Reviewing %d lines across %d files (base: %s @ %s)", + patch.ChangedLines, patch.FileCount, baseLabel, shortSha)) + } + + createReq := &backend.CreateLocalCodeReviewRunRequest{ + OwnerName: owner, + RepoName: name, + BaseSha: patch.BaseSha, + Patch: patch.Patch, + CliVersion: fmt.Sprintf("tusk-cli/%s", version.Version), + } + if reviewMinSeverity != "" { + s := reviewMinSeverity + createReq.MinSeverity = &s + } + + runID, err := client.CreateLocalCodeReviewRun(ctx, createReq, authOptions) + if err != nil { + if api.IsRateLimitError(err) { + return &ExitCodeError{Code: 2, Err: err} + } + if api.IsRepoNotFoundError(err) { + // SOHAN-TODO: audit this error message + return &ExitCodeError{Code: 2, Err: fmt.Errorf( + "this repo (%s/%s) is not connected to Tusk.\nConnect it at https://app.usetusk.ai/onboarding, or pass --repo to target a different connected repo.", + owner, name)} + } + if api.IsPatchInvalidError(err) { + return &ExitCodeError{Code: 2, Err: err} + } + return formatApiError(err) + } + + // Cancellation cleanup MUST be registered before the poll loop so that + // Ctrl+C fires a backend cancel. Keep the timeout short — if the cancel + // RPC itself hangs, we don't want to block process exit. + RegisterCleanup(func() { + cancelCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := client.CancelCodeReviewRun(cancelCtx, &backend.CancelCodeReviewRunRequest{RunId: runID}, authOptions); err != nil { + log.Debug("Failed to cancel code review run", "runId", runID, "error", err) + return + } + if !reviewQuiet { + log.Stderrln(fmt.Sprintf("Cancelled run %s.", runID)) + } + }) + + final, err := review.Poll(ctx, client, authOptions, runID, review.PollOptions{ + Quiet: reviewQuiet, + }) + if err != nil { + return formatApiError(err) + } + + if err := writeResult(final, reviewJSON, reviewOutput); err != nil { + return err + } + + switch final.GetStatus() { + case backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_FAILED: + // Backend already rendered the failure reason into display_message/ + // display_json which we just wrote to stdout. Bubble up a sentinel + // error (no duplicate stderr) purely so the process exits with code 1. + return errSilentFail + case backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_CANCELLED: + return nil + default: + return nil + } +} + +// errSilentFail lets runReview signal "exit code 1 but don't print anything" +// — the failure text is already on stdout/stderr via the result writer. +var errSilentFail = &silentErr{} + +type silentErr struct{} + +func (*silentErr) Error() string { return "" } + +// mapPatchError turns BuildPatch errors into the right user-facing error and +// exit code. Plain errors fall through unchanged. +func mapPatchError(err error) error { + if review.IsBaseResolutionError(err) { + return &ExitCodeError{Code: 2, Err: err} + } + if review.IsPreflightError(err) { + return &ExitCodeError{Code: 2, Err: err} + } + var tooLarge *review.PatchTooLargeError + if errors.As(err, &tooLarge) { + return &ExitCodeError{Code: 2, Err: fmt.Errorf("%s\n\n%s", tooLarge.LimitMessage, formatTopContributors(tooLarge.TopFiles))} + } + var submodule *review.SubmoduleError + if errors.As(err, &submodule) { + lines := []string{"submodule changes are not supported.\n\nFound submodule update(s) in the generated patch:"} + for _, p := range submodule.Paths { + lines = append(lines, " "+p) + } + lines = append(lines, "\nCommit submodule updates separately, or exclude them via:\n tusk review --exclude '/**'") + return &ExitCodeError{Code: 2, Err: errors.New(strings.Join(lines, "\n"))} + } + return err +} + +func formatTopContributors(files []review.FileSummary) string { + if len(files) == 0 { + return "" + } + var sb strings.Builder + sb.WriteString("Top contributors:\n") + for _, f := range files { + total := f.AddedLines + f.DelLines + fmt.Fprintf(&sb, " %s (+%d/-%d, %d lines)\n", f.Path, f.AddedLines, f.DelLines, total) + } + sb.WriteString("\nAdd these to .tuskignore or pass --exclude '' to skip them.") + return sb.String() +} + +// writeResult writes the backend-rendered final output to the selected sink +// (stdout by default, or --output file). JSON mode writes display_json; +// default mode writes display_message. +// +// If the backend did not set display_json (e.g. FAILED with no JSON renderer), +// a minimal CLI-assembled JSON object is written so callers piping to jq +// never receive empty output. +func writeResult(resp *backend.GetCodeReviewRunStatusResponseSuccess, jsonMode bool, outputPath string) error { + var out *os.File + var err error + if outputPath != "" { + out, err = os.Create(outputPath) //nolint:gosec // user-specified path + if err != nil { + return fmt.Errorf("open --output: %w", err) + } + defer func() { _ = out.Close() }() + } else { + out = os.Stdout + } + + if jsonMode { + if resp.GetDisplayJson() != "" { + _, err := out.WriteString(resp.GetDisplayJson()) + if err != nil { + return err + } + if !strings.HasSuffix(resp.GetDisplayJson(), "\n") { + _, _ = out.WriteString("\n") + } + return nil + } + // Fallback: minimal JSON so downstream scripts never see empty output. + fallback := map[string]any{ + "run_id": resp.GetRunId(), + "status": protoStatusToString(resp.GetStatus()), + "message": resp.GetDisplayMessage(), + } + enc := json.NewEncoder(out) + enc.SetIndent("", " ") + enc.SetEscapeHTML(false) + return enc.Encode(fallback) + } + + msg := resp.GetDisplayMessage() + if msg == "" { + msg = fmt.Sprintf("Run %s completed (status: %s).", resp.GetRunId(), protoStatusToString(resp.GetStatus())) + } + if _, err := out.WriteString(msg); err != nil { + return err + } + if !strings.HasSuffix(msg, "\n") { + _, _ = out.WriteString("\n") + } + return nil +} + +func protoStatusToString(s backend.CodeReviewRunStatus) string { + switch s { + case backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_PENDING: + return "PENDING" + case backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_RUNNING: + return "RUNNING" + case backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_SUCCESS: + return "SUCCESS" + case backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_FAILED: + return "FAILED" + case backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_CANCELLED: + return "CANCELLED" + default: + return "UNKNOWN" + } +} diff --git a/cmd/review_status.go b/cmd/review_status.go new file mode 100644 index 00000000..b2d29024 --- /dev/null +++ b/cmd/review_status.go @@ -0,0 +1,83 @@ +package cmd + +import ( + "context" + "time" + + "github.com/spf13/cobra" + + "github.com/Use-Tusk/tusk-cli/internal/log" + "github.com/Use-Tusk/tusk-cli/internal/review" + backend "github.com/Use-Tusk/tusk-drift-schemas/generated/go/backend" +) + +var reviewStatusCmd = &cobra.Command{ + Use: "status ", + Short: "Show status of a previously-started code review run", + Args: cobra.ExactArgs(1), + SilenceUsage: true, + RunE: runReviewStatus, +} + +func init() { + reviewCmd.AddCommand(reviewStatusCmd) + + // Reuse the main review command's --json, --output, --quiet globals so + // `tusk review status ... --json` behaves identically to `tusk review --json`. + reviewStatusCmd.Flags().BoolVar(&reviewJSON, "json", false, "Write the result as JSON (to stdout or --output)") + reviewStatusCmd.Flags().StringVar(&reviewOutput, "output", "", "Write the result to a file instead of stdout") + reviewStatusCmd.Flags().BoolVar(&reviewQuiet, "quiet", false, "Suppress stderr progress output") + reviewStatusCmd.Flags().BoolVar(&reviewStatusWatch, "watch", false, "Block on running runs until they reach a terminal state") + reviewStatusCmd.Flags().SortFlags = false +} + +func runReviewStatus(cmd *cobra.Command, args []string) error { + setupSignalHandling() + ctx := context.Background() + runID := args[0] + + log.Debug("tusk review status", "runId", runID, "watch", reviewStatusWatch, "json", reviewJSON) + + client, authOptions, err := setupReviewCloud() + if err != nil { + return err + } + + if reviewStatusWatch { + // Cancellation cleanup mirrors the main command: if the user hits + // Ctrl+C while --watch is blocking, cancel the run server-side. + RegisterCleanup(func() { + cancelCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := client.CancelCodeReviewRun(cancelCtx, &backend.CancelCodeReviewRunRequest{RunId: runID}, authOptions); err != nil { + log.Debug("Failed to cancel code review run", "runId", runID, "error", err) + } + }) + + final, err := review.Poll(ctx, client, authOptions, runID, review.PollOptions{Quiet: reviewQuiet}) + if err != nil { + return formatApiError(err) + } + if err := writeResult(final, reviewJSON, reviewOutput); err != nil { + return err + } + if final.GetStatus() == backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_FAILED { + return errSilentFail + } + return nil + } + + // Single snapshot (default). Non-terminal runs exit 0 — the user asked for + // a snapshot, not a verdict. Pair with --watch to block. + resp, err := client.GetCodeReviewRunStatus(ctx, &backend.GetCodeReviewRunStatusRequest{RunId: runID}, authOptions) + if err != nil { + return formatApiError(err) + } + if err := writeResult(resp, reviewJSON, reviewOutput); err != nil { + return err + } + if resp.GetStatus() == backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_FAILED { + return errSilentFail + } + return nil +} diff --git a/cmd/short_docs/review/overview.md b/cmd/short_docs/review/overview.md new file mode 100644 index 00000000..54035c0e --- /dev/null +++ b/cmd/short_docs/review/overview.md @@ -0,0 +1,63 @@ +# Tusk Review + +`tusk review` runs the Tusk AI code review against your local working tree — before you push or open a PR. It uploads a git patch (not the whole repo), polls for completion, and prints the result to stdout. + +The CLI never posts comments to GitHub or GitLab. All output is local. + +## Typical workflow + +1. Make some changes in a repo that's connected to Tusk. +2. Run `tusk review` from the repo directory. +3. Stderr shows progress; stdout shows the final review when it's done. + +## Authentication + +Run `tusk auth login`, or set `TUSK_API_KEY` for non-interactive use. + +## Repo identity + +By default, the repo is detected from the `origin` remote (`owner/name`). Override with `--repo owner/name`. The repo must already be connected to Tusk. + +## Base branch resolution + +The "base" is the commit your changes sit on top of. By default, `tusk review` uses `git merge-base origin/HEAD HEAD` — the point your branch diverged from origin's default branch. + +Pass `--base ` to override. This is the right thing to do on **stacked branches** (feature-2 branched off feature-1 branched off main): without it, your review will critique feature-1's changes as if they were yours. + +``` +main: A ─ B ─ C + \ +feature-1: D ─ E (open PR, not merged) + \ +feature-2: F ─ G (current branch) +``` + +Here, run `tusk review --base origin/feature-1` so the diff is just F–G. + +## Output + +- Default: human-readable text to stdout, progress on stderr. +- `--json`: backend-rendered JSON document to stdout, suitable for `| jq`. +- `--output `: write the result to a file (format follows `--json`). +- `--quiet`: suppress stderr progress (final output unchanged). + +## Filtering + +Locked files and common build output are skipped automatically (same list the server-side review uses). To tweak: + +- `.tuskignore` at the repo root: `.gitignore`-style globs, additive to the defaults. +- `--exclude `: one-off add. Repeatable. +- `--include `: cancel a default skip (e.g. `--include 'package-lock.json'`). Repeatable. + +## Exit codes + +- `0` — review completed (issues may or may not be found) +- `1` — run failed, or network / auth error +- `2` — user-actionable pre-flight error (mid-rebase, rate limit, patch too large, repo not connected, couldn't determine base) + +## Status subcommand + +``` +tusk review status # print current status snapshot +tusk review status --watch # block until run reaches a terminal state +``` diff --git a/go.mod b/go.mod index 72a46ac5..23eec606 100644 --- a/go.mod +++ b/go.mod @@ -2,6 +2,8 @@ module github.com/Use-Tusk/tusk-cli go 1.25.0 +replace github.com/Use-Tusk/tusk-drift-schemas => ../tusk-drift-schemas + require ( github.com/Use-Tusk/fence v0.1.36 github.com/Use-Tusk/tusk-drift-schemas v0.1.34 diff --git a/go.sum b/go.sum index 5f5769c6..7c9f59a4 100644 --- a/go.sum +++ b/go.sum @@ -33,8 +33,6 @@ github.com/STARRY-S/zip v0.2.3 h1:luE4dMvRPDOWQdeDdUxUoZkzUIpTccdKdhHHsQJ1fm4= github.com/STARRY-S/zip v0.2.3/go.mod h1:lqJ9JdeRipyOQJrYSOtpNAiaesFO6zVDsE8GIGFaoSk= github.com/Use-Tusk/fence v0.1.36 h1:8S15y8cp3X+xXukx6AN0Ky/aX9/dZyW3fLw5XOQ8YtE= github.com/Use-Tusk/fence v0.1.36/go.mod h1:YkowBDzXioVKJE16vg9z3gSVC6vhzkIZZw2dFf7MW/o= -github.com/Use-Tusk/tusk-drift-schemas v0.1.34 h1:OUXsA4sfBMA/HCuPqYdfl5EP9+Jq+hYenAmw4wwrEVo= -github.com/Use-Tusk/tusk-drift-schemas v0.1.34/go.mod h1:pa3EvTj9kKxl9f904RVFkj9YK1zB75QogboKi70zalM= github.com/agnivade/levenshtein v1.0.3 h1:M5ZnqLOoZR8ygVq0FfkXsNOKzMCk0xRiow0R5+5VkQ0= github.com/agnivade/levenshtein v1.0.3/go.mod h1:4SFRZbbXWLF4MU1T9Qg0pGgH3Pjs+t6ie5efyrwRJXs= github.com/alecthomas/assert/v2 v2.7.0 h1:QtqSACNS3tF7oasA8CU6A6sXZSBDqnm7RfpLl9bZqbE= diff --git a/internal/api/client.go b/internal/api/client.go index 09de83ae..86f82044 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -83,6 +83,7 @@ const ( TestRunServiceAPIPath = "/api/drift/test_run_service" ClientServiceAPIPath = "/api/drift/client_service" SpanExportServiceAPIPath = "/api/drift/tusk.drift.backend.v1.SpanExportService" + CodeReviewServiceAPIPath = "/api/drift/code_review_service" ) // GetBaseURL returns the API base URL with the following priority: @@ -236,6 +237,11 @@ func (c *TuskClient) makeClientServiceRequest(ctx context.Context, endpoint stri return c.makeProtoRequestWithRetryConfig(ctx, fullServiceAPIPath, endpoint, req, resp, auth, DefaultRetryConfig(0)) } +func (c *TuskClient) makeCodeReviewServiceRequest(ctx context.Context, endpoint string, req proto.Message, resp proto.Message, auth AuthOptions, config RetryConfig) error { + fullServiceAPIPath := c.baseURL + CodeReviewServiceAPIPath + return c.makeProtoRequestWithRetryConfig(ctx, fullServiceAPIPath, endpoint, req, resp, auth, config) +} + // SkippableError is returned for errors that should be treated as a no-op in CI mode // (e.g. feature disabled after trial expiry, repo disabled, repo not found) type SkippableError struct { diff --git a/internal/api/code_review.go b/internal/api/code_review.go new file mode 100644 index 00000000..888b0604 --- /dev/null +++ b/internal/api/code_review.go @@ -0,0 +1,110 @@ +package api + +import ( + "context" + "errors" + "fmt" + + backend "github.com/Use-Tusk/tusk-drift-schemas/generated/go/backend" +) + +// RateLimitError is returned by CreateLocalCodeReviewRun when the backend +// indicates a per-user rate limit has been hit. Carries the human-readable +// message (for surfacing verbatim) and an ISO-8601 retry time when available. +type RateLimitError struct { + Message string + RetryAfterIso8601 string +} + +func (e *RateLimitError) Error() string { return e.Message } + +// IsRateLimitError reports whether err (or anything it wraps) is a *RateLimitError. +func IsRateLimitError(err error) bool { + var r *RateLimitError + return errors.As(err, &r) +} + +// RepoNotFoundError means the (owner/name) isn't connected to the caller's org. +// Surfaced as a distinct type so the CLI can show the onboarding link. +type RepoNotFoundError struct { + Message string +} + +func (e *RepoNotFoundError) Error() string { return e.Message } + +// IsRepoNotFoundError reports whether err (or anything it wraps) is a *RepoNotFoundError. +func IsRepoNotFoundError(err error) bool { + var r *RepoNotFoundError + return errors.As(err, &r) +} + +// PatchInvalidError signals the backend rejected the uploaded patch +// (bad bytes, empty after filtering, etc.). +type PatchInvalidError struct { + Message string +} + +func (e *PatchInvalidError) Error() string { return e.Message } + +// IsPatchInvalidError reports whether err (or anything it wraps) is a *PatchInvalidError. +func IsPatchInvalidError(err error) bool { + var p *PatchInvalidError + return errors.As(err, &p) +} + +func (c *TuskClient) CreateLocalCodeReviewRun(ctx context.Context, in *backend.CreateLocalCodeReviewRunRequest, auth AuthOptions) (string, error) { + var out backend.CreateLocalCodeReviewRunResponse + if err := c.makeCodeReviewServiceRequest(ctx, "create_local_code_review_run", in, &out, auth, DefaultRetryConfig(3)); err != nil { + return "", err + } + + if s := out.GetSuccess(); s != nil { + return s.GetRunId(), nil + } + if e := out.GetError(); e != nil { + switch e.GetCode() { + case backend.CreateLocalCodeReviewRunResponseErrorCode_CREATE_LOCAL_CODE_REVIEW_RUN_RESPONSE_ERROR_CODE_RATE_LIMITED: + return "", &RateLimitError{ + Message: e.GetMessage(), + RetryAfterIso8601: e.GetRetryAfterIso8601(), + } + case backend.CreateLocalCodeReviewRunResponseErrorCode_CREATE_LOCAL_CODE_REVIEW_RUN_RESPONSE_ERROR_CODE_REPO_NOT_FOUND: + return "", &RepoNotFoundError{Message: e.GetMessage()} + case backend.CreateLocalCodeReviewRunResponseErrorCode_CREATE_LOCAL_CODE_REVIEW_RUN_RESPONSE_ERROR_CODE_PATCH_INVALID: + return "", &PatchInvalidError{Message: e.GetMessage()} + } + return "", fmt.Errorf("%s: %s", e.GetCode().String(), e.GetMessage()) + } + return "", fmt.Errorf("invalid response") +} + +func (c *TuskClient) GetCodeReviewRunStatus(ctx context.Context, in *backend.GetCodeReviewRunStatusRequest, auth AuthOptions) (*backend.GetCodeReviewRunStatusResponseSuccess, error) { + var out backend.GetCodeReviewRunStatusResponse + if err := c.makeCodeReviewServiceRequest(ctx, "get_code_review_run_status", in, &out, auth, DefaultRetryConfig(3)); err != nil { + return nil, err + } + + if s := out.GetSuccess(); s != nil { + return s, nil + } + if e := out.GetError(); e != nil { + return nil, fmt.Errorf("%s: %s", e.GetCode().String(), e.GetMessage()) + } + return nil, fmt.Errorf("invalid response") +} + +func (c *TuskClient) CancelCodeReviewRun(ctx context.Context, in *backend.CancelCodeReviewRunRequest, auth AuthOptions) error { + var out backend.CancelCodeReviewRunResponse + if err := c.makeCodeReviewServiceRequest(ctx, "cancel_code_review_run", in, &out, auth, DefaultRetryConfig(0)); err != nil { + return err + } + + if s := out.GetSuccess(); s != nil { + _ = s + return nil + } + if e := out.GetError(); e != nil { + return fmt.Errorf("%s: %s", e.GetCode().String(), e.GetMessage()) + } + return fmt.Errorf("invalid response") +} diff --git a/internal/review/filter.go b/internal/review/filter.go new file mode 100644 index 00000000..f594b26c --- /dev/null +++ b/internal/review/filter.go @@ -0,0 +1,149 @@ +// Package review contains helpers used by the `tusk review` command: +// file-filter lists mirroring the backend, pre-flight git checks, patch +// generation, and status polling. +package review + +import ( + "bufio" + "os" + "path/filepath" + "strings" + + "github.com/bmatcuk/doublestar/v4" +) + +var ExtensionsToSkip = []string{ + // Images + ".png", ".jpg", ".jpeg", ".gif", ".bmp", ".tif", ".tiff", ".ico", ".svg", ".webp", ".heic", + // Audio + ".mp3", ".wav", ".wma", ".ogg", ".flac", ".m4a", ".aac", ".midi", ".mid", + // Video + ".mp4", ".avi", ".mkv", ".mov", ".wmv", ".m4v", ".3gp", ".3g2", ".rm", ".swf", ".flv", ".webm", ".mpg", ".mpeg", + // Fonts + ".otf", ".ttf", + // Documents + ".pdf", ".doc", ".docx", ".xls", ".xlsx", ".ppt", ".pptx", ".rtf", ".odt", ".ods", ".odp", + // Archives + ".iso", ".bin", ".tar", ".zip", ".7z", ".gz", ".rar", ".bz2", ".xz", + // Minified and source maps + ".min.js", ".min.js.map", ".js.map", ".min.css", ".min.css.map", + // Data and configuration + ".tfstate", ".tfstate.backup", ".parquet", ".pyc", ".pub", ".pem", ".lock", ".sqlite", ".db", ".env", ".log", + // Compiled code + ".class", ".dll", ".exe", + // Design files + ".psd", ".ai", ".sketch", + // 3D and CAD + ".stl", ".obj", ".dwg", + // Backup files + ".bak", ".old", ".tmp", +} + +var FilesToSkip = []string{ + "pnpm-lock.yaml", + "package-lock.json", + ".DS_Store", + ".gitignore", + "bun.lockb", + "npm-debug.log", + "yarn-error.log", + "Thumbs.db", + "Gemfile.lock", +} + +var DirectoriesToSkip = []string{ + // Version control & IDE + ".git", ".vscode", ".idea", + // JavaScript/Node + "node_modules", "dist", "build", "out", ".next", ".nuxt", ".turbo", + ".parcel-cache", ".svelte-kit", ".vercel", ".angular", ".nx", "bower_components", + // Python + "__pycache__", ".venv", "venv", "env", ".pytest_cache", ".mypy_cache", ".tox", + // Java/JVM + "target", ".gradle", + // Ruby + ".bundle", + // Go/General vendor + "vendor", + // Infrastructure + ".terraform", ".serverless", + // General + "assets", "coverage", "tmp", "temp", "logs", "generated", ".cache", ".sass-cache", +} + +// BuildPathspecExclusions returns a list of git pathspec strings (each prefixed +// with `:(exclude,glob)`) that can be passed to `git diff` to filter out files +// the backend code-review pipeline would skip anyway, plus any user-supplied +// extras. +// +// includes cancels individual default exclusions: any default pattern that +// doublestar-matches an include glob is dropped. This gives the user an +// escape hatch without needing git pathspec's more awkward include form. +func BuildPathspecExclusions(extraExcludes []string, includes []string) []string { + var defaults []string + for _, name := range FilesToSkip { + defaults = append(defaults, "**/"+name) + } + for _, ext := range ExtensionsToSkip { + defaults = append(defaults, "**/*"+ext) + } + for _, dir := range DirectoriesToSkip { + defaults = append(defaults, "**/"+dir+"/**") + } + + filtered := defaults[:0] + for _, pattern := range defaults { + drop := false + for _, inc := range includes { + if match, _ := doublestar.Match(inc, pattern); match { + drop = true + break + } + // Also drop if any file path matched by the default could be + // matched by the include. Cheap heuristic: treat include as + // a path and check if the default would match it. + if match, _ := doublestar.Match(pattern, inc); match { + drop = true + break + } + } + if !drop { + filtered = append(filtered, pattern) + } + } + + all := make([]string, 0, len(filtered)+len(extraExcludes)) + for _, p := range filtered { + all = append(all, ":(exclude,glob)"+p) + } + for _, p := range extraExcludes { + all = append(all, ":(exclude,glob)"+p) + } + return all +} + +// ReadTuskignore parses `.tuskignore` at the given repo root if present. +// Returns a list of glob patterns; comments and blanks are ignored. +// Missing file returns (nil, nil). +func ReadTuskignore(repoRoot string) ([]string, error) { + path := filepath.Join(repoRoot, ".tuskignore") + f, err := os.Open(path) //nolint:gosec // reading user repo file is intended + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, err + } + defer func() { _ = f.Close() }() + + var patterns []string + scanner := bufio.NewScanner(f) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + patterns = append(patterns, line) + } + return patterns, scanner.Err() +} diff --git a/internal/review/filter_test.go b/internal/review/filter_test.go new file mode 100644 index 00000000..523f3ac5 --- /dev/null +++ b/internal/review/filter_test.go @@ -0,0 +1,70 @@ +package review + +import ( + "strings" + "testing" +) + +// TestFilterListSizes pins list lengths so a CI-side check flags drift +// relative to backend/src/utils/repoUtils.ts. Update these numbers (and +// the lists themselves) when the backend list changes. +func TestFilterListSizes(t *testing.T) { + if got, want := len(ExtensionsToSkip), 84; got != want { + t.Errorf("ExtensionsToSkip size = %d; want %d (update list or test if backend changed)", got, want) + } + if got, want := len(FilesToSkip), 9; got != want { + t.Errorf("FilesToSkip size = %d; want %d", got, want) + } + if got, want := len(DirectoriesToSkip), 37; got != want { + t.Errorf("DirectoriesToSkip size = %d; want %d", got, want) + } +} + +func TestBuildPathspecExclusions_IncludesDefaults(t *testing.T) { + specs := BuildPathspecExclusions(nil, nil) + if len(specs) == 0 { + t.Fatal("expected at least some default pathspec exclusions") + } + // Every entry should be prefixed with the pathspec magic. + for _, s := range specs { + if !strings.HasPrefix(s, ":(exclude,glob)") { + t.Errorf("pathspec not properly prefixed: %q", s) + } + } + // Sanity-check a handful of well-known defaults made it in. + joined := strings.Join(specs, "\n") + for _, want := range []string{ + "**/package-lock.json", + "**/*.png", + "**/node_modules/**", + } { + if !strings.Contains(joined, want) { + t.Errorf("expected pathspec list to contain %q", want) + } + } +} + +func TestBuildPathspecExclusions_ExtraExcludes(t *testing.T) { + specs := BuildPathspecExclusions([]string{"docs/**", "*.generated.go"}, nil) + joined := strings.Join(specs, "\n") + if !strings.Contains(joined, "docs/**") { + t.Errorf("missing extra exclude docs/**") + } + if !strings.Contains(joined, "*.generated.go") { + t.Errorf("missing extra exclude *.generated.go") + } +} + +func TestBuildPathspecExclusions_IncludeCancelsDefault(t *testing.T) { + // Ask to keep package-lock.json — it should be removed from the default + // exclusion list rather than forcing a separate include pathspec. + specs := BuildPathspecExclusions(nil, []string{"package-lock.json"}) + joined := strings.Join(specs, "\n") + if strings.Contains(joined, "**/package-lock.json") { + t.Errorf("expected --include 'package-lock.json' to drop the default exclusion; got %q", joined) + } + // Other defaults should remain. + if !strings.Contains(joined, "**/*.png") { + t.Errorf("unrelated default was unexpectedly dropped") + } +} diff --git a/internal/review/patch.go b/internal/review/patch.go new file mode 100644 index 00000000..f4ccd0af --- /dev/null +++ b/internal/review/patch.go @@ -0,0 +1,426 @@ +package review + +import ( + "bytes" + "context" + "errors" + "fmt" + "os" + "os/exec" + "regexp" + "sort" + "strconv" + "strings" +) + +// Hard and soft caps. Hard caps abort with an error listing top contributors. +// Soft caps emit a stderr warning and continue. +const ( + SoftLineCap = 2_000 + SoftFileCap = 50 + HardLineCap = 10_000 + HardFileCap = 200 + HardBytesCap = 1 << 20 // 1 MiB +) + +// FileSummary describes a single file's contribution to the patch, used to +// build "top contributors" lists for size-cap error messages. +type FileSummary struct { + Path string + AddedLines int + DelLines int +} + +// PatchResult is what BuildPatch returns on success. +type PatchResult struct { + Patch []byte + BaseSha string + BaseRef string // The ref the base resolved from (e.g. "origin/main"), informational. + ChangedFiles []FileSummary + ChangedLines int + FileCount int +} + +// ErrEmptyPatch is returned when the working tree diff against the base is +// empty (or empty after filtering). +var ErrEmptyPatch = errors.New("empty patch") + +// SubmoduleError is returned when the generated patch contains submodule +// updates — these are not supported in v1. +type SubmoduleError struct { + Paths []string +} + +func (e *SubmoduleError) Error() string { + return fmt.Sprintf("submodule changes are not supported (%d path(s))", len(e.Paths)) +} + +// PatchTooLargeError is returned when the patch exceeds a hard cap. +type PatchTooLargeError struct { + Reason string // "lines", "files", or "bytes" + Bytes int + Lines int + Files int + TopFiles []FileSummary + LimitMessage string // human-readable summary, e.g. "1.4MB (limit: 1MB)" +} + +func (e *PatchTooLargeError) Error() string { + return "patch too large: " + e.LimitMessage +} + +// BaseResolutionError wraps merge-base or ref-resolution failures with the +// structured remediation UX from the plan. +type BaseResolutionError struct { + Message string +} + +func (e *BaseResolutionError) Error() string { return e.Message } + +// IsBaseResolutionError reports whether err (or anything it wraps) is a +// *BaseResolutionError. +func IsBaseResolutionError(err error) bool { + var b *BaseResolutionError + return errors.As(err, &b) +} + +// PatchOptions drives BuildPatch. +type PatchOptions struct { + RepoRoot string + Base string // user-provided --base ref/sha; empty → auto-detect via origin/HEAD + ExtraExcludes []string + Includes []string + RegisterCleanup func(fn func()) // typically cmd.RegisterCleanup; may be nil for tests + // Stderr is where soft-warnings are written. Defaults to os.Stderr when nil. + Stderr *os.File +} + +// BuildPatch generates a binary-clean git patch of the current working tree +// against a resolved base SHA. Untracked files are added with `git add -N` +// (and the intent-to-add is undone both via a normal return path and via a +// signal-safe cleanup registered through opts.RegisterCleanup). +// +// Filtering is always applied (EXTENSIONS/FILES/DIRECTORIES skip lists + +// .tuskignore + --exclude/--include). Returns ErrEmptyPatch if nothing +// survives filtering. +func BuildPatch(ctx context.Context, opts PatchOptions) (*PatchResult, error) { + stderr := opts.Stderr + if stderr == nil { + stderr = os.Stderr + } + + untracked, err := listUntracked(opts.RepoRoot) + if err != nil { + return nil, err + } + + if len(untracked) > 0 { + if err := gitRun(opts.RepoRoot, append([]string{"add", "-N", "--"}, untracked...)...); err != nil { + return nil, fmt.Errorf("git add -N: %w", err) + } + restore := func() { + // Idempotent — if a path is no longer staged, this is a no-op. + _ = gitRun(opts.RepoRoot, append([]string{"restore", "--staged", "--"}, untracked...)...) + } + if opts.RegisterCleanup != nil { + opts.RegisterCleanup(restore) + } + // Best-effort restore on the normal return path as well. + defer restore() + } + + baseSha, baseRef, err := resolveBase(opts.RepoRoot, opts.Base) + if err != nil { + return nil, err + } + + // Merge .tuskignore entries with --exclude flags. + tuskignoreExtras, err := ReadTuskignore(opts.RepoRoot) + if err != nil { + return nil, fmt.Errorf("read .tuskignore: %w", err) + } + extraExcludes := append([]string{}, opts.ExtraExcludes...) + extraExcludes = append(extraExcludes, tuskignoreExtras...) + + pathspecs := BuildPathspecExclusions(extraExcludes, opts.Includes) + + // Generate binary patch. + diffArgs := append([]string{"diff", "--binary", baseSha, "--"}, pathspecs...) + patchBytes, err := gitOutput(opts.RepoRoot, diffArgs...) + if err != nil { + return nil, fmt.Errorf("git diff --binary: %w", err) + } + + // Generate numstat for per-file line counts. + numstatArgs := append([]string{"diff", "--numstat", baseSha, "--"}, pathspecs...) + numstatBytes, err := gitOutput(opts.RepoRoot, numstatArgs...) + if err != nil { + return nil, fmt.Errorf("git diff --numstat: %w", err) + } + files := parseNumstat(numstatBytes) + + if len(files) == 0 { + return nil, ErrEmptyPatch + } + + // Submodule check on raw patch bytes. Two markers: `160000` mode lines or + // `Subproject commit ` lines. + if paths := detectSubmodulePaths(patchBytes); len(paths) > 0 { + return nil, &SubmoduleError{Paths: paths} + } + + totalLines := 0 + for _, f := range files { + totalLines += f.AddedLines + f.DelLines + } + fileCount := len(files) + byteLen := len(patchBytes) + + // Hard caps — most specific wins for the error reason, but all checks are + // equivalent: patch is too big to upload. + if byteLen > HardBytesCap { + return nil, &PatchTooLargeError{ + Reason: "bytes", + Bytes: byteLen, + Lines: totalLines, + Files: fileCount, + TopFiles: topContributors(files, 5), + LimitMessage: fmt.Sprintf("%s (limit: 1MB)", humanBytes(byteLen)), + } + } + if totalLines > HardLineCap { + return nil, &PatchTooLargeError{ + Reason: "lines", + Bytes: byteLen, + Lines: totalLines, + Files: fileCount, + TopFiles: topContributors(files, 5), + LimitMessage: fmt.Sprintf("%d lines changed (limit: %d)", totalLines, HardLineCap), + } + } + if fileCount > HardFileCap { + return nil, &PatchTooLargeError{ + Reason: "files", + Bytes: byteLen, + Lines: totalLines, + Files: fileCount, + TopFiles: topContributors(files, 5), + LimitMessage: fmt.Sprintf("%d files changed (limit: %d)", fileCount, HardFileCap), + } + } + + // Soft caps — warn but continue. + if totalLines > SoftLineCap { + _, _ = fmt.Fprintf(stderr, "warning: %d lines changed (soft limit: %d). Review quality may degrade on large diffs.\n", + totalLines, SoftLineCap) + } + if fileCount > SoftFileCap { + _, _ = fmt.Fprintf(stderr, "warning: %d files changed (soft limit: %d). Review quality may degrade on large diffs.\n", + fileCount, SoftFileCap) + } + + return &PatchResult{ + Patch: patchBytes, + BaseSha: baseSha, + BaseRef: baseRef, + ChangedFiles: files, + ChangedLines: totalLines, + FileCount: fileCount, + }, nil +} + +// listUntracked returns the set of untracked (and not-ignored) paths under +// repoRoot, suitable for `git add -N`. +func listUntracked(repoRoot string) ([]string, error) { + out, err := gitOutput(repoRoot, "status", "--porcelain=v1", "--untracked-files=normal") + if err != nil { + return nil, fmt.Errorf("git status: %w", err) + } + var untracked []string + for _, line := range strings.Split(string(out), "\n") { + // Porcelain v1: "?? " for untracked. + if strings.HasPrefix(line, "?? ") { + p := strings.TrimPrefix(line, "?? ") + // Unquote if git quoted a path containing special chars. + if strings.HasPrefix(p, "\"") && strings.HasSuffix(p, "\"") { + if unq, err := strconv.Unquote(p); err == nil { + p = unq + } + } + if p != "" { + untracked = append(untracked, p) + } + } + } + return untracked, nil +} + +// resolveBase turns the user-provided base (or empty → origin/HEAD auto-detect) +// into a resolved commit SHA. Returns (sha, ref) where ref is the original +// input (informational) or "origin/HEAD" for auto-detection. +func resolveBase(repoRoot string, userBase string) (string, string, error) { + if userBase != "" { + out, err := gitOutput(repoRoot, "rev-parse", "--verify", userBase+"^{commit}") + if err != nil { + return "", "", &BaseResolutionError{ + Message: fmt.Sprintf("couldn't resolve --base %q to a commit: %s\n\nTry:\n tusk review --base origin/main", + userBase, strings.TrimSpace(string(out))), + } + } + return strings.TrimSpace(string(out)), userBase, nil + } + + // Auto-detect via origin/HEAD. + if err := CheckOriginHead(repoRoot); err != nil { + return "", "", err + } + baseOut, err := gitOutput(repoRoot, "merge-base", "origin/HEAD", "HEAD") + if err != nil { + shallow := isShallow(repoRoot) + msg := "couldn't determine base commit for this branch.\n\n" + msg += "Cause: `git merge-base origin/HEAD HEAD` failed — this branch may not share history with origin's default branch." + if shallow { + msg += " Also: this is a shallow clone." + } + msg += "\n\nThings to try:" + if shallow { + msg += "\n • git fetch --unshallow" + } + msg += "\n • Pass the base explicitly: tusk review --base " + return "", "", &BaseResolutionError{Message: msg} + } + return strings.TrimSpace(string(baseOut)), "origin/HEAD", nil +} + +// parseNumstat parses the output of `git diff --numstat` into per-file +// summaries. Binary files show "- -" for their counts; we record them as +// zero changed lines but still include them in the file count. +func parseNumstat(out []byte) []FileSummary { + var summaries []FileSummary + for _, line := range strings.Split(string(out), "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + parts := strings.Fields(line) + if len(parts) < 3 { + continue + } + added := 0 + del := 0 + if n, err := strconv.Atoi(parts[0]); err == nil { + added = n + } + if n, err := strconv.Atoi(parts[1]); err == nil { + del = n + } + // Path may be a single token or a rename "old => new"; use the last + // token which is the post-rename path for size-cap reporting. + path := parts[len(parts)-1] + summaries = append(summaries, FileSummary{ + Path: path, + AddedLines: added, + DelLines: del, + }) + } + return summaries +} + +// topContributors returns up to n file summaries sorted by (added+deleted) +// lines descending, for inclusion in size-cap error messages. +func topContributors(files []FileSummary, n int) []FileSummary { + sorted := append([]FileSummary{}, files...) + sort.Slice(sorted, func(i, j int) bool { + return (sorted[i].AddedLines + sorted[i].DelLines) > (sorted[j].AddedLines + sorted[j].DelLines) + }) + if len(sorted) > n { + sorted = sorted[:n] + } + return sorted +} + +// submoduleModeRe matches `:160000 160000 ...` or `new mode 160000` etc. in +// raw git-diff output. Conservative: we just look for a 160000 octal mode +// token on its own, which only appears for submodule entries. +var ( + submoduleModeRe = regexp.MustCompile(`(?m)^(?:new file mode|deleted file mode|old mode|new mode|index [^\s]+|similarity index)\b[^\n]*\b160000\b`) + submoduleHeaderRe = regexp.MustCompile(`(?m)^diff --git a/(.+?) b/(.+)$`) + submoduleIndexLineRe = regexp.MustCompile(`(?m)^index [^\s]+\s+160000\b`) + submoduleSubprojectRe = regexp.MustCompile(`(?m)^Subproject commit [0-9a-f]+`) +) + +// detectSubmodulePaths scans the raw git patch for submodule markers. +// Returns up to ~10 offending paths (deduped) so the error message stays +// readable; the user only needs one example to know what to exclude. +func detectSubmodulePaths(patch []byte) []string { + // Fast path: if no markers at all, skip the per-file walk. + if !submoduleModeRe.Match(patch) && + !submoduleIndexLineRe.Match(patch) && + !submoduleSubprojectRe.Match(patch) { + return nil + } + + // Walk file headers; for each file section, scan for a submodule marker. + var paths []string + seen := map[string]struct{}{} + headerIdx := submoduleHeaderRe.FindAllSubmatchIndex(patch, -1) + for i, idx := range headerIdx { + start := idx[1] + end := len(patch) + if i+1 < len(headerIdx) { + end = headerIdx[i+1][0] + } + section := patch[idx[0]:end] + if submoduleModeRe.Match(section) || + submoduleIndexLineRe.Match(section) || + submoduleSubprojectRe.Match(section) { + path := string(patch[idx[2]:idx[3]]) + if _, ok := seen[path]; !ok { + seen[path] = struct{}{} + paths = append(paths, path) + if len(paths) >= 10 { + break + } + } + } + _ = start + } + return paths +} + +// humanBytes formats a byte count as a short human-readable string +// ("1.4MB", "912KB"). Rounded to one decimal for KB/MB. +func humanBytes(n int) string { + const ( + kb = 1 << 10 + mb = 1 << 20 + ) + switch { + case n >= mb: + return fmt.Sprintf("%.1fMB", float64(n)/mb) + case n >= kb: + return fmt.Sprintf("%.1fKB", float64(n)/kb) + default: + return fmt.Sprintf("%dB", n) + } +} + +// gitOutput runs git in repoRoot and returns combined stdout (stderr is +// captured and included in the error on failure). +func gitOutput(repoRoot string, args ...string) ([]byte, error) { + cmd := exec.Command("git", args...) //nolint:gosec // args are controlled + cmd.Dir = repoRoot + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return stdout.Bytes(), fmt.Errorf("%w: %s", err, strings.TrimSpace(stderr.String())) + } + return stdout.Bytes(), nil +} + +// gitRun runs git and returns any error (with stderr captured in the message). +func gitRun(repoRoot string, args ...string) error { + _, err := gitOutput(repoRoot, args...) + return err +} diff --git a/internal/review/poll.go b/internal/review/poll.go new file mode 100644 index 00000000..0120a05d --- /dev/null +++ b/internal/review/poll.go @@ -0,0 +1,118 @@ +package review + +import ( + "context" + "fmt" + "io" + "os" + "time" + + "github.com/mattn/go-isatty" + + "github.com/Use-Tusk/tusk-cli/internal/api" + backend "github.com/Use-Tusk/tusk-drift-schemas/generated/go/backend" +) + +// PollOptions tunes the status-polling loop. +type PollOptions struct { + // Interval between polls. Default: 2s. + Interval time.Duration + // Quiet suppresses all stderr progress output entirely. + Quiet bool + // Stderr is where progress lines are written. Defaults to os.Stderr. + // Primarily for testing. + Stderr *os.File +} + +// spinnerFrames is cycled for TTY spinner animation. +var spinnerFrames = []rune{'⠋', '⠙', '⠹', '⠸', '⠼', '⠴', '⠦', '⠧', '⠇', '⠏'} + +// Poll blocks, polling the backend for the given runId, rendering progress +// to stderr as it goes, until the run reaches a terminal status (SUCCESS, +// FAILED, CANCELLED). +// +// TTY mode: animated spinner, message repainted on each tick with \r. +// Non-TTY: one line per change (avoids spamming CI logs). +// Quiet: nothing to stderr; polling still happens so the backend heartbeat +// is kept fresh. +func Poll(ctx context.Context, client *api.TuskClient, auth api.AuthOptions, runId string, opts PollOptions) (*backend.GetCodeReviewRunStatusResponseSuccess, error) { + if opts.Interval <= 0 { + opts.Interval = 2 * time.Second + } + var stderr io.Writer = os.Stderr + if opts.Stderr != nil { + stderr = opts.Stderr + } + stderrIsTTY := !opts.Quiet && isatty.IsTerminal(os.Stderr.Fd()) + + ticker := time.NewTicker(opts.Interval) + defer ticker.Stop() + + var lastMsg string + frame := 0 + renderedTTY := false + + for { + resp, err := client.GetCodeReviewRunStatus(ctx, + &backend.GetCodeReviewRunStatusRequest{RunId: runId}, auth) + if err != nil { + // Clear the spinner line before bubbling up so the caller's error + // text starts on a clean line. + if renderedTTY { + _, _ = fmt.Fprint(stderr, "\r\033[K") + } + return nil, err + } + + status := resp.GetStatus() + msg := resp.GetDisplayMessage() + + // Terminal-status check runs BEFORE rendering: on SUCCESS/FAILED/ + // CANCELLED the backend packs the full final output into + // display_message, and the caller writes that to stdout via + // writeResult. Rendering it here as well would duplicate the output + // on stderr (and \r\033[K can only clear one row, so a multi-line + // final message leaves visible leftovers). + if isTerminal(status) { + if renderedTTY { + _, _ = fmt.Fprint(stderr, "\r\033[K") + } + return resp, nil + } + + if !opts.Quiet { + if stderrIsTTY { + char := spinnerFrames[frame%len(spinnerFrames)] + frame++ + _, _ = fmt.Fprintf(stderr, "\r\033[K%c %s", char, msg) + renderedTTY = true + } else if msg != "" && msg != lastMsg { + _, _ = fmt.Fprintln(stderr, msg) + lastMsg = msg + } + } + + select { + case <-ctx.Done(): + if renderedTTY { + _, _ = fmt.Fprint(stderr, "\r\033[K") + } + return nil, ctx.Err() + case <-ticker.C: + } + } +} + +// isTerminal reports whether the given status value indicates a terminal +// state. UNSPECIFIED is treated as still-running (defensive — server should +// never send it). +func isTerminal(s backend.CodeReviewRunStatus) bool { + switch s { + case backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_SUCCESS, + backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_FAILED, + backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_CANCELLED: + return true + default: + return false + } +} diff --git a/internal/review/preflight.go b/internal/review/preflight.go new file mode 100644 index 00000000..94e05e41 --- /dev/null +++ b/internal/review/preflight.go @@ -0,0 +1,152 @@ +package review + +import ( + "bytes" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" +) + +// PreflightError is returned for user-actionable pre-flight failures (e.g. +// mid-rebase, no origin/HEAD, shallow). The CLI renders the Message +// verbatim and exits with code 2. +type PreflightError struct { + Message string +} + +func (e *PreflightError) Error() string { return e.Message } + +// IsPreflightError reports whether err (or anything it wraps) is a +// *PreflightError. +func IsPreflightError(err error) bool { + var p *PreflightError + return errors.As(err, &p) +} + +// GitDir returns the path to the .git directory (or worktree gitdir file) for +// the repository at repoRoot. +func GitDir(repoRoot string) (string, error) { + cmd := exec.Command("git", "rev-parse", "--git-dir") //nolint:gosec + cmd.Dir = repoRoot + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("git rev-parse --git-dir: %w", err) + } + gitDir := strings.TrimSpace(string(out)) + if !filepath.IsAbs(gitDir) { + gitDir = filepath.Join(repoRoot, gitDir) + } + return gitDir, nil +} + +// RepoRoot returns the top-level directory of the git repository that the +// current working directory is inside, or an error if cwd isn't a repo. +func RepoRoot() (string, error) { + out, err := exec.Command("git", "rev-parse", "--show-toplevel").Output() //nolint:gosec + if err != nil { + return "", fmt.Errorf("not inside a git repository: %w", err) + } + return strings.TrimSpace(string(out)), nil +} + +// Preflight runs the quick local checks that must pass before we try to +// generate a patch. It returns a *PreflightError for user-actionable failures +// and a plain error for unexpected ones (e.g. git not on PATH). +// +// Emits a stderr warning (not an error) for detached HEAD. Detached HEAD +// doesn't prevent patch generation; the user is just told their results may +// look off. +func Preflight(repoRoot string) error { + gitDir, err := GitDir(repoRoot) + if err != nil { + return err + } + + // Mid-operation detection. Each of these produces a specific, user-actionable error. + midOps := []struct { + path string + name string + next string + }{ + {filepath.Join(gitDir, "rebase-merge"), "rebase", "git rebase --continue` or `git rebase --abort"}, + {filepath.Join(gitDir, "rebase-apply"), "rebase", "git rebase --continue` or `git rebase --abort"}, + {filepath.Join(gitDir, "MERGE_HEAD"), "merge", "git merge --continue` or `git merge --abort"}, + {filepath.Join(gitDir, "CHERRY_PICK_HEAD"), "cherry-pick", "git cherry-pick --continue` or `git cherry-pick --abort"}, + {filepath.Join(gitDir, "REVERT_HEAD"), "revert", "git revert --continue` or `git revert --abort"}, + } + for _, op := range midOps { + if _, err := os.Stat(op.path); err == nil { + return &PreflightError{ + Message: fmt.Sprintf( + "working tree is mid-%s. Finish (`%s`) before running `tusk review`.", + op.name, op.next), + } + } + } + + // `origin` presence is NOT checked here — either: + // - the user passed --repo + --base (no origin needed at all), OR + // - the repo-identity step (resolveReviewRepo) will fail with a + // specific "run inside a git repo with an origin remote" message, OR + // - the base-resolution step will call CheckOriginHead for a targeted + // "origin/HEAD not set" error. + // Firing a generic origin-missing error in preflight would block the + // --repo + --base bypass that this command is designed to allow. + + // Detached HEAD is a warning, not a refusal. + if err := runGitSilent(repoRoot, "symbolic-ref", "-q", "HEAD"); err != nil { + _, _ = fmt.Fprintln(os.Stderr, "warning: HEAD is detached; review results may look odd.") + } + + return nil +} + +// CheckOriginHead confirms that `origin/HEAD` is set on this clone, which +// `git merge-base` relies on for auto-detecting the base branch. Returns a +// *PreflightError with remediation text when missing. +// +// Only called when the user did NOT pass `--base` — explicit base bypasses +// the origin/HEAD requirement. +func CheckOriginHead(repoRoot string) error { + if err := runGitSilent(repoRoot, "symbolic-ref", "refs/remotes/origin/HEAD"); err != nil { + shallow := isShallow(repoRoot) + msg := "couldn't determine base commit for this branch.\n\n" + msg += "Cause: this clone has no `origin/HEAD` ref." + if shallow { + msg += " Also: this is a shallow clone." + } + msg += "\n\nFix: git remote set-head origin --auto" + if shallow { + msg += "\n git fetch --unshallow" + } + msg += "\n\nOr pass --base explicitly:\n tusk review --base main" + return &PreflightError{Message: msg} + } + return nil +} + +func isShallow(repoRoot string) bool { + gitDir, err := GitDir(repoRoot) + if err != nil { + return false + } + _, err = os.Stat(filepath.Join(gitDir, "shallow")) + return err == nil +} + +// runGitSilent runs git with the given args in repoRoot and returns an error +// if the process exits non-zero (or fails to start). stderr is captured but +// not printed. +func runGitSilent(repoRoot string, args ...string) error { + cmd := exec.Command("git", args...) //nolint:gosec // args are controlled + cmd.Dir = repoRoot + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return fmt.Errorf("git %s: %w (%s)", strings.Join(args, " "), err, strings.TrimSpace(stderr.String())) + } + return nil +} diff --git a/main.go b/main.go index 144e9259..eed96a97 100644 --- a/main.go +++ b/main.go @@ -16,6 +16,6 @@ func main() { analytics.GlobalTracker.Close() if err != nil { - os.Exit(1) + os.Exit(cmd.ExitCodeOf(err)) } } From 9a52f204e3957ee9b29e94f702432138433d1b2c Mon Sep 17 00:00:00 2001 From: Sohan Kshirsagar Date: Tue, 21 Apr 2026 21:58:37 -0700 Subject: [PATCH 2/7] address comments --- cmd/review.go | 14 ++++-- cmd/review_status.go | 3 ++ go.mod | 4 +- go.sum | 2 + internal/review/patch.go | 97 +++++++++++++++++++++++++----------- internal/review/poll.go | 11 ++-- internal/review/preflight.go | 36 ++++++++++++- 7 files changed, 127 insertions(+), 40 deletions(-) diff --git a/cmd/review.go b/cmd/review.go index 43f8d2f9..c2d34b11 100644 --- a/cmd/review.go +++ b/cmd/review.go @@ -120,11 +120,6 @@ func runReview(cmd *cobra.Command, args []string) error { return &ExitCodeError{Code: 2, Err: err} } - client, authOptions, err := setupReviewCloud() - if err != nil { - return err - } - patch, err := review.BuildPatch(ctx, review.PatchOptions{ RepoRoot: repoRoot, Base: reviewBase, @@ -140,6 +135,11 @@ func runReview(cmd *cobra.Command, args []string) error { return mapPatchError(err) } + client, authOptions, err := setupReviewCloud() + if err != nil { + return err + } + // Quick stderr header for non-TTY callers so they know something's happening // even before the first progress poll. The backend will replace this with // richer phase text once it starts rendering. @@ -216,6 +216,10 @@ func runReview(cmd *cobra.Command, args []string) error { // Backend already rendered the failure reason into display_message/ // display_json which we just wrote to stdout. Bubble up a sentinel // error (no duplicate stderr) purely so the process exits with code 1. + // SilenceErrors prevents Cobra from printing a stray "Error: \n" line + // for errSilentFail (whose Error() returns ""); normal errors from + // earlier returns are still printed by Cobra. + cmd.SilenceErrors = true return errSilentFail case backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_CANCELLED: return nil diff --git a/cmd/review_status.go b/cmd/review_status.go index b2d29024..0a8daf47 100644 --- a/cmd/review_status.go +++ b/cmd/review_status.go @@ -62,6 +62,8 @@ func runReviewStatus(cmd *cobra.Command, args []string) error { return err } if final.GetStatus() == backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_FAILED { + // Silence Cobra's "Error:" prefix for the empty errSilentFail. + cmd.SilenceErrors = true return errSilentFail } return nil @@ -77,6 +79,7 @@ func runReviewStatus(cmd *cobra.Command, args []string) error { return err } if resp.GetStatus() == backend.CodeReviewRunStatus_CODE_REVIEW_RUN_STATUS_FAILED { + cmd.SilenceErrors = true return errSilentFail } return nil diff --git a/go.mod b/go.mod index 23eec606..857ebb10 100644 --- a/go.mod +++ b/go.mod @@ -2,11 +2,9 @@ module github.com/Use-Tusk/tusk-cli go 1.25.0 -replace github.com/Use-Tusk/tusk-drift-schemas => ../tusk-drift-schemas - require ( github.com/Use-Tusk/fence v0.1.36 - github.com/Use-Tusk/tusk-drift-schemas v0.1.34 + github.com/Use-Tusk/tusk-drift-schemas v0.1.35 github.com/agnivade/levenshtein v1.0.3 github.com/aymanbagabas/go-osc52/v2 v2.0.1 github.com/bmatcuk/doublestar/v4 v4.10.0 diff --git a/go.sum b/go.sum index 7c9f59a4..78e84f28 100644 --- a/go.sum +++ b/go.sum @@ -33,6 +33,8 @@ github.com/STARRY-S/zip v0.2.3 h1:luE4dMvRPDOWQdeDdUxUoZkzUIpTccdKdhHHsQJ1fm4= github.com/STARRY-S/zip v0.2.3/go.mod h1:lqJ9JdeRipyOQJrYSOtpNAiaesFO6zVDsE8GIGFaoSk= github.com/Use-Tusk/fence v0.1.36 h1:8S15y8cp3X+xXukx6AN0Ky/aX9/dZyW3fLw5XOQ8YtE= github.com/Use-Tusk/fence v0.1.36/go.mod h1:YkowBDzXioVKJE16vg9z3gSVC6vhzkIZZw2dFf7MW/o= +github.com/Use-Tusk/tusk-drift-schemas v0.1.35 h1:s8Rknx8zMMRWCoe+YLQWmlB58T45lcZwI8TBK4+8Mx4= +github.com/Use-Tusk/tusk-drift-schemas v0.1.35/go.mod h1:pa3EvTj9kKxl9f904RVFkj9YK1zB75QogboKi70zalM= github.com/agnivade/levenshtein v1.0.3 h1:M5ZnqLOoZR8ygVq0FfkXsNOKzMCk0xRiow0R5+5VkQ0= github.com/agnivade/levenshtein v1.0.3/go.mod h1:4SFRZbbXWLF4MU1T9Qg0pGgH3Pjs+t6ie5efyrwRJXs= github.com/alecthomas/assert/v2 v2.7.0 h1:QtqSACNS3tF7oasA8CU6A6sXZSBDqnm7RfpLl9bZqbE= diff --git a/internal/review/patch.go b/internal/review/patch.go index f4ccd0af..da0a7413 100644 --- a/internal/review/patch.go +++ b/internal/review/patch.go @@ -109,18 +109,21 @@ func BuildPatch(ctx context.Context, opts PatchOptions) (*PatchResult, error) { stderr = os.Stderr } - untracked, err := listUntracked(opts.RepoRoot) + untracked, err := listUntracked(ctx, opts.RepoRoot) if err != nil { return nil, err } if len(untracked) > 0 { - if err := gitRun(opts.RepoRoot, append([]string{"add", "-N", "--"}, untracked...)...); err != nil { + if err := gitRun(ctx, opts.RepoRoot, append([]string{"add", "-N", "--"}, untracked...)...); err != nil { return nil, fmt.Errorf("git add -N: %w", err) } restore := func() { - // Idempotent — if a path is no longer staged, this is a no-op. - _ = gitRun(opts.RepoRoot, append([]string{"restore", "--staged", "--"}, untracked...)...) + // Cleanup must use a fresh context — ctx may already be cancelled + // (Ctrl+C) at the time this runs, and we still need the restore + // to succeed. Idempotent — if a path is no longer staged, this is + // a no-op. + _ = gitRun(context.Background(), opts.RepoRoot, append([]string{"restore", "--staged", "--"}, untracked...)...) } if opts.RegisterCleanup != nil { opts.RegisterCleanup(restore) @@ -129,7 +132,7 @@ func BuildPatch(ctx context.Context, opts PatchOptions) (*PatchResult, error) { defer restore() } - baseSha, baseRef, err := resolveBase(opts.RepoRoot, opts.Base) + baseSha, baseRef, err := resolveBase(ctx, opts.RepoRoot, opts.Base) if err != nil { return nil, err } @@ -146,14 +149,15 @@ func BuildPatch(ctx context.Context, opts PatchOptions) (*PatchResult, error) { // Generate binary patch. diffArgs := append([]string{"diff", "--binary", baseSha, "--"}, pathspecs...) - patchBytes, err := gitOutput(opts.RepoRoot, diffArgs...) + patchBytes, err := gitOutput(ctx, opts.RepoRoot, diffArgs...) if err != nil { return nil, fmt.Errorf("git diff --binary: %w", err) } - // Generate numstat for per-file line counts. - numstatArgs := append([]string{"diff", "--numstat", baseSha, "--"}, pathspecs...) - numstatBytes, err := gitOutput(opts.RepoRoot, numstatArgs...) + // Generate numstat for per-file line counts. `-z` writes NUL-terminated + // records so paths containing spaces or other whitespace survive intact. + numstatArgs := append([]string{"diff", "--numstat", "-z", baseSha, "--"}, pathspecs...) + numstatBytes, err := gitOutput(ctx, opts.RepoRoot, numstatArgs...) if err != nil { return nil, fmt.Errorf("git diff --numstat: %w", err) } @@ -231,8 +235,8 @@ func BuildPatch(ctx context.Context, opts PatchOptions) (*PatchResult, error) { // listUntracked returns the set of untracked (and not-ignored) paths under // repoRoot, suitable for `git add -N`. -func listUntracked(repoRoot string) ([]string, error) { - out, err := gitOutput(repoRoot, "status", "--porcelain=v1", "--untracked-files=normal") +func listUntracked(ctx context.Context, repoRoot string) ([]string, error) { + out, err := gitOutput(ctx, repoRoot, "status", "--porcelain=v1", "--untracked-files=normal") if err != nil { return nil, fmt.Errorf("git status: %w", err) } @@ -258,13 +262,17 @@ func listUntracked(repoRoot string) ([]string, error) { // resolveBase turns the user-provided base (or empty → origin/HEAD auto-detect) // into a resolved commit SHA. Returns (sha, ref) where ref is the original // input (informational) or "origin/HEAD" for auto-detection. -func resolveBase(repoRoot string, userBase string) (string, string, error) { +func resolveBase(ctx context.Context, repoRoot string, userBase string) (string, string, error) { if userBase != "" { - out, err := gitOutput(repoRoot, "rev-parse", "--verify", userBase+"^{commit}") + out, err := gitOutput(ctx, repoRoot, "rev-parse", "--verify", userBase+"^{commit}") if err != nil { + // `git rev-parse --verify` writes its failure reason to stderr + // (e.g. "fatal: Needed a single revision"), which `gitOutput` + // has already captured into err. `out` (stdout) is empty on + // failure — using it here would produce an empty reason. return "", "", &BaseResolutionError{ Message: fmt.Sprintf("couldn't resolve --base %q to a commit: %s\n\nTry:\n tusk review --base origin/main", - userBase, strings.TrimSpace(string(out))), + userBase, strings.TrimSpace(err.Error())), } } return strings.TrimSpace(string(out)), userBase, nil @@ -274,7 +282,7 @@ func resolveBase(repoRoot string, userBase string) (string, string, error) { if err := CheckOriginHead(repoRoot); err != nil { return "", "", err } - baseOut, err := gitOutput(repoRoot, "merge-base", "origin/HEAD", "HEAD") + baseOut, err := gitOutput(ctx, repoRoot, "merge-base", "origin/HEAD", "HEAD") if err != nil { shallow := isShallow(repoRoot) msg := "couldn't determine base commit for this branch.\n\n" @@ -292,18 +300,37 @@ func resolveBase(repoRoot string, userBase string) (string, string, error) { return strings.TrimSpace(string(baseOut)), "origin/HEAD", nil } -// parseNumstat parses the output of `git diff --numstat` into per-file +// parseNumstat parses the output of `git diff --numstat -z` into per-file // summaries. Binary files show "- -" for their counts; we record them as // zero changed lines but still include them in the file count. +// +// With `-z`, records are NUL-terminated. The added/deleted counts are +// tab-delimited, followed by either: +// - a single path (non-renamed), OR +// - three NUL-terminated tokens: "\t\t" + "\0\0" +// for renames/copies (the `-M`/`-C` case — safe to handle even if we +// don't explicitly enable those flags, since users can via git config). +// +// NUL-terminated parsing is mandatory for correctness: `git diff --numstat` +// without `-z` double-quotes paths containing special characters, and paths +// with embedded whitespace would otherwise be split by any field-based +// parser. func parseNumstat(out []byte) []FileSummary { var summaries []FileSummary - for _, line := range strings.Split(string(out), "\n") { - line = strings.TrimSpace(line) - if line == "" { + records := strings.Split(string(out), "\x00") + i := 0 + for i < len(records) { + rec := records[i] + if rec == "" { + i++ continue } - parts := strings.Fields(line) + // The first record contains "\t\t". + // If the trailing path is empty, the next two records are the + // rename's old and new paths. + parts := strings.SplitN(rec, "\t", 3) if len(parts) < 3 { + i++ continue } added := 0 @@ -314,9 +341,22 @@ func parseNumstat(out []byte) []FileSummary { if n, err := strconv.Atoi(parts[1]); err == nil { del = n } - // Path may be a single token or a rename "old => new"; use the last - // token which is the post-rename path for size-cap reporting. - path := parts[len(parts)-1] + path := parts[2] + if path == "" { + // Rename/copy: next two records are "old\0new\0". Use the new path. + if i+2 < len(records) { + path = records[i+2] + i += 3 + } else { + i++ + continue + } + } else { + i++ + } + if path == "" { + continue + } summaries = append(summaries, FileSummary{ Path: path, AddedLines: added, @@ -406,9 +446,10 @@ func humanBytes(n int) string { } // gitOutput runs git in repoRoot and returns combined stdout (stderr is -// captured and included in the error on failure). -func gitOutput(repoRoot string, args ...string) ([]byte, error) { - cmd := exec.Command("git", args...) //nolint:gosec // args are controlled +// captured and included in the error on failure). The process is launched +// with ctx so callers can cancel or time out in-flight git operations. +func gitOutput(ctx context.Context, repoRoot string, args ...string) ([]byte, error) { + cmd := exec.CommandContext(ctx, "git", args...) //nolint:gosec // args are controlled cmd.Dir = repoRoot var stdout, stderr bytes.Buffer cmd.Stdout = &stdout @@ -420,7 +461,7 @@ func gitOutput(repoRoot string, args ...string) ([]byte, error) { } // gitRun runs git and returns any error (with stderr captured in the message). -func gitRun(repoRoot string, args ...string) error { - _, err := gitOutput(repoRoot, args...) +func gitRun(ctx context.Context, repoRoot string, args ...string) error { + _, err := gitOutput(ctx, repoRoot, args...) return err } diff --git a/internal/review/poll.go b/internal/review/poll.go index 0120a05d..fe97bf95 100644 --- a/internal/review/poll.go +++ b/internal/review/poll.go @@ -39,11 +39,16 @@ func Poll(ctx context.Context, client *api.TuskClient, auth api.AuthOptions, run if opts.Interval <= 0 { opts.Interval = 2 * time.Second } - var stderr io.Writer = os.Stderr + // TTY detection must track whichever file descriptor we're actually + // writing to — checking os.Stderr while writing to an overridden + // opts.Stderr (e.g. a test pipe) would emit spinner escape sequences + // into the wrong destination. + stderrFile := os.Stderr if opts.Stderr != nil { - stderr = opts.Stderr + stderrFile = opts.Stderr } - stderrIsTTY := !opts.Quiet && isatty.IsTerminal(os.Stderr.Fd()) + var stderr io.Writer = stderrFile + stderrIsTTY := !opts.Quiet && isatty.IsTerminal(stderrFile.Fd()) ticker := time.NewTicker(opts.Interval) defer ticker.Stop() diff --git a/internal/review/preflight.go b/internal/review/preflight.go index 94e05e41..b0cead9e 100644 --- a/internal/review/preflight.go +++ b/internal/review/preflight.go @@ -8,6 +8,8 @@ import ( "os/exec" "path/filepath" "strings" + + "github.com/Use-Tusk/tusk-cli/internal/log" ) // PreflightError is returned for user-actionable pre-flight failures (e.g. @@ -97,13 +99,45 @@ func Preflight(repoRoot string) error { // --repo + --base bypass that this command is designed to allow. // Detached HEAD is a warning, not a refusal. - if err := runGitSilent(repoRoot, "symbolic-ref", "-q", "HEAD"); err != nil { + // + // `git symbolic-ref -q HEAD` exits 1 *only* when HEAD is not a symbolic + // ref (i.e. detached); other failure modes (corrupt repo, git missing) + // exit 128. We only want to surface the detached-head warning in the + // exit-1 case — treating any non-zero exit as "detached" would + // mis-diagnose those other failures. + if detached, err := isDetachedHEAD(repoRoot); err != nil { + // Don't block the command on an unexpected symbolic-ref failure; + // the real patch-generation step will surface a better error if + // the repo is broken. Do log for debuggability. + log.Debug("symbolic-ref HEAD check failed", "error", err) + } else if detached { _, _ = fmt.Fprintln(os.Stderr, "warning: HEAD is detached; review results may look odd.") } return nil } +// isDetachedHEAD reports whether HEAD is currently detached. Returns +// (false, err) for unexpected git failures so callers can distinguish +// "definitely not detached" from "can't tell". +func isDetachedHEAD(repoRoot string) (bool, error) { + cmd := exec.Command("git", "symbolic-ref", "-q", "HEAD") //nolint:gosec + cmd.Dir = repoRoot + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + // Exit 1 with `-q` is the documented "not a symbolic ref" signal. + if exitErr.ExitCode() == 1 { + return true, nil + } + } + return false, fmt.Errorf("git symbolic-ref -q HEAD: %w (%s)", err, strings.TrimSpace(stderr.String())) + } + return false, nil +} + // CheckOriginHead confirms that `origin/HEAD` is set on this clone, which // `git merge-base` relies on for auto-detecting the base branch. Returns a // *PreflightError with remediation text when missing. From 0a05e62b25085d50cd0a5b3df33ec9bf0e381671 Mon Sep 17 00:00:00 2001 From: Sohan Kshirsagar Date: Wed, 22 Apr 2026 11:09:36 -0700 Subject: [PATCH 3/7] polling every 5 seconds --- internal/review/poll.go | 156 +++++++++++++++++++++++++++------------- 1 file changed, 108 insertions(+), 48 deletions(-) diff --git a/internal/review/poll.go b/internal/review/poll.go index fe97bf95..92d79428 100644 --- a/internal/review/poll.go +++ b/internal/review/poll.go @@ -14,9 +14,20 @@ import ( ) // PollOptions tunes the status-polling loop. +// +// Spinner animation and backend polling are intentionally decoupled: the +// spinner ticks on its own ~100ms schedule for smooth visual feedback, while +// the backend is hit only every Interval seconds. A stale spinner frame +// remains smooth to the eye; a stale backend poll costs money. type PollOptions struct { - // Interval between polls. Default: 2s. + // Interval between backend status polls. Default: 5s. + // + // Must stay well under the backend's heartbeat-abandonment window + // (currently 5 minutes) so live runs don't get reaped. Interval time.Duration + // SpinnerInterval controls how often the TTY spinner redraws. Ignored + // when stderr is not a TTY or when Quiet is set. Default: 100ms. + SpinnerInterval time.Duration // Quiet suppresses all stderr progress output entirely. Quiet bool // Stderr is where progress lines are written. Defaults to os.Stderr. @@ -31,14 +42,19 @@ var spinnerFrames = []rune{'⠋', '⠙', '⠹', '⠸', '⠼', '⠴', '⠦', '⠧ // to stderr as it goes, until the run reaches a terminal status (SUCCESS, // FAILED, CANCELLED). // -// TTY mode: animated spinner, message repainted on each tick with \r. -// Non-TTY: one line per change (avoids spamming CI logs). +// TTY mode: animated spinner redraws every ~100ms using the most recent +// display_message; backend is hit every Interval. +// Non-TTY: one line per message change (avoids spamming CI logs). // Quiet: nothing to stderr; polling still happens so the backend heartbeat // is kept fresh. func Poll(ctx context.Context, client *api.TuskClient, auth api.AuthOptions, runId string, opts PollOptions) (*backend.GetCodeReviewRunStatusResponseSuccess, error) { if opts.Interval <= 0 { - opts.Interval = 2 * time.Second + opts.Interval = 5 * time.Second + } + if opts.SpinnerInterval <= 0 { + opts.SpinnerInterval = 100 * time.Millisecond } + // TTY detection must track whichever file descriptor we're actually // writing to — checking os.Stderr while writing to an overridden // opts.Stderr (e.g. a test pipe) would emit spinner escape sequences @@ -50,60 +66,104 @@ func Poll(ctx context.Context, client *api.TuskClient, auth api.AuthOptions, run var stderr io.Writer = stderrFile stderrIsTTY := !opts.Quiet && isatty.IsTerminal(stderrFile.Fd()) - ticker := time.NewTicker(opts.Interval) - defer ticker.Stop() + // Cached state shared between the poll ticker (writer) and the spinner + // ticker (reader). Both live on the same goroutine so no mutex needed. + var ( + latestMsg string // most recent display_message from the backend + lastLoggedMsg string // last message we actually printed in non-TTY mode (dedup) + frame int + renderedTTY bool + ) - var lastMsg string - frame := 0 - renderedTTY := false + clearSpinner := func() { + if renderedTTY { + _, _ = fmt.Fprint(stderr, "\r\033[K") + } + } - for { - resp, err := client.GetCodeReviewRunStatus(ctx, + fetchOnce := func() (*backend.GetCodeReviewRunStatusResponseSuccess, error) { + return client.GetCodeReviewRunStatus(ctx, &backend.GetCodeReviewRunStatusRequest{RunId: runId}, auth) - if err != nil { - // Clear the spinner line before bubbling up so the caller's error - // text starts on a clean line. - if renderedTTY { - _, _ = fmt.Fprint(stderr, "\r\033[K") - } - return nil, err - } + } - status := resp.GetStatus() - msg := resp.GetDisplayMessage() - - // Terminal-status check runs BEFORE rendering: on SUCCESS/FAILED/ - // CANCELLED the backend packs the full final output into - // display_message, and the caller writes that to stdout via - // writeResult. Rendering it here as well would duplicate the output - // on stderr (and \r\033[K can only clear one row, so a multi-line - // final message leaves visible leftovers). - if isTerminal(status) { - if renderedTTY { - _, _ = fmt.Fprint(stderr, "\r\033[K") - } - return resp, nil - } + // Initial poll so the very first spinner frame shows a real message + // rather than an empty string for the entire first Interval. + resp, err := fetchOnce() + if err != nil { + return nil, err + } + if isTerminal(resp.GetStatus()) { + return resp, nil + } + latestMsg = resp.GetDisplayMessage() - if !opts.Quiet { - if stderrIsTTY { - char := spinnerFrames[frame%len(spinnerFrames)] - frame++ - _, _ = fmt.Fprintf(stderr, "\r\033[K%c %s", char, msg) - renderedTTY = true - } else if msg != "" && msg != lastMsg { - _, _ = fmt.Fprintln(stderr, msg) - lastMsg = msg - } + // Paint the first frame (TTY) or first line (non-TTY) immediately so the + // user sees something before the first spinner/poll tick fires. + if !opts.Quiet { + if stderrIsTTY { + _, _ = fmt.Fprintf(stderr, "\r\033[K%c %s", + spinnerFrames[frame%len(spinnerFrames)], latestMsg) + renderedTTY = true + } else if latestMsg != "" && latestMsg != lastLoggedMsg { + _, _ = fmt.Fprintln(stderr, latestMsg) + lastLoggedMsg = latestMsg } + } + + pollTicker := time.NewTicker(opts.Interval) + defer pollTicker.Stop() + + // Spinner channel only wired up when we're actually going to animate — + // a nil channel blocks forever in select, which is what we want for + // non-TTY / quiet mode. + var spinnerC <-chan time.Time + if stderrIsTTY { + spinnerTicker := time.NewTicker(opts.SpinnerInterval) + defer spinnerTicker.Stop() + spinnerC = spinnerTicker.C + } + for { select { case <-ctx.Done(): - if renderedTTY { - _, _ = fmt.Fprint(stderr, "\r\033[K") - } + clearSpinner() return nil, ctx.Err() - case <-ticker.C: + + case <-spinnerC: + // Redraw with the cached latest message. Cheap local tick — + // no backend call. This is why we can afford 100ms cadence. + frame++ + _, _ = fmt.Fprintf(stderr, "\r\033[K%c %s", + spinnerFrames[frame%len(spinnerFrames)], latestMsg) + renderedTTY = true + + case <-pollTicker.C: + resp, err := fetchOnce() + if err != nil { + clearSpinner() + return nil, err + } + + // Terminal-status check before any render, same reasoning as the + // initial poll: on SUCCESS/FAILED/CANCELLED the backend packs the + // full output into display_message, which the caller writes to + // stdout. Rendering it to stderr would duplicate it. + if isTerminal(resp.GetStatus()) { + clearSpinner() + return resp, nil + } + + latestMsg = resp.GetDisplayMessage() + + // Non-TTY mode prints only on change — there's no spinner ticker + // to pick up message updates. TTY mode just updates the cache + // and lets the spinner ticker redraw at its own cadence. + if !opts.Quiet && !stderrIsTTY { + if latestMsg != "" && latestMsg != lastLoggedMsg { + _, _ = fmt.Fprintln(stderr, latestMsg) + lastLoggedMsg = latestMsg + } + } } } } From 36b6d133263f9951f53146e1e3b59287e5fe1787 Mon Sep 17 00:00:00 2001 From: Sohan Kshirsagar Date: Wed, 22 Apr 2026 11:39:26 -0700 Subject: [PATCH 4/7] address todo --- cmd/review.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cmd/review.go b/cmd/review.go index c2d34b11..9daf4b9f 100644 --- a/cmd/review.go +++ b/cmd/review.go @@ -174,9 +174,17 @@ func runReview(cmd *cobra.Command, args []string) error { return &ExitCodeError{Code: 2, Err: err} } if api.IsRepoNotFoundError(err) { - // SOHAN-TODO: audit this error message return &ExitCodeError{Code: 2, Err: fmt.Errorf( - "this repo (%s/%s) is not connected to Tusk.\nConnect it at https://app.usetusk.ai/onboarding, or pass --repo to target a different connected repo.", + "repo %s/%s is not connected to Tusk under your current org.\n\n"+ + "If this is the repo you meant to review:\n"+ + " • Connect it at https://app.usetusk.ai/repos\n"+ + " (installs the GitHub/GitLab app and grants access)\n"+ + " • Or, if you belong to multiple Tusk orgs, switch:\n"+ + " tusk auth select-org\n\n"+ + "If this is not the repo you meant:\n"+ + " • Pass --repo owner/name to target a different connected repo\n"+ + " • Check whether origin is a fork (git remote -v); you likely\n"+ + " want the upstream, not the fork", owner, name)} } if api.IsPatchInvalidError(err) { From 29899bf191106379742bbd539c240e0d8630d295 Mon Sep 17 00:00:00 2001 From: Sohan Kshirsagar Date: Thu, 23 Apr 2026 00:55:01 -0700 Subject: [PATCH 5/7] address comments --- cmd/review.go | 93 +++++++++++++++++--- cmd/review_run.go | 25 ++++++ cmd/short_docs/review/overview.md | 59 ++----------- cmd/short_docs/review/run.md | 53 ++++++++++++ go.mod | 2 +- go.sum | 4 +- internal/api/code_review.go | 48 ++++++++++- internal/review/patch.go | 139 ++++++++++++++++++++++++------ internal/review/preflight.go | 2 +- 9 files changed, 324 insertions(+), 101 deletions(-) create mode 100644 cmd/review_run.go create mode 100644 cmd/short_docs/review/run.md diff --git a/cmd/review.go b/cmd/review.go index 9daf4b9f..d70b98e0 100644 --- a/cmd/review.go +++ b/cmd/review.go @@ -32,32 +32,36 @@ var ( reviewJSON bool reviewOutput string reviewQuiet bool + reviewNoPoll bool // Used by the status subcommand only. reviewStatusWatch bool ) +// reviewCmd is a parent group only — subcommands (run, status) do the work. +// This matches the tusk unit / tusk drift pattern and leaves room for future +// subcommands (cancel, list, resolve, ...) without the ambiguity of a +// top-level command that's both runnable and a group. var reviewCmd = &cobra.Command{ Use: "review", - Short: "Run Tusk code review on your local working tree", + Short: "Commands for Tusk code review workflows", Long: utils.RenderMarkdown(reviewOverviewContent), SilenceUsage: true, - RunE: runReview, } func init() { rootCmd.AddCommand(reviewCmd) - bindReviewFlags(reviewCmd) } func bindReviewFlags(cmd *cobra.Command) { cmd.Flags().StringVar(&reviewRepo, "repo", "", "Repository in owner/name format (defaults to git origin remote)") - cmd.Flags().StringVar(&reviewBase, "base", "", "Base ref or SHA to diff against (defaults to merge-base with origin/HEAD)") + cmd.Flags().StringVar(&reviewBase, "base", "", "Override the clone pivot (defaults to your branch's upstream @{u}, then merge-base with origin/HEAD)") cmd.Flags().StringVar(&reviewMinSeverity, "min-severity", "", "Minimum severity to surface: low|medium|high|critical") cmd.Flags().StringArrayVar(&reviewExcludes, "exclude", nil, "Extra path glob(s) to exclude from the patch (repeatable)") cmd.Flags().StringArrayVar(&reviewIncludes, "include", nil, "Cancel a default skip for matching files (repeatable)") cmd.Flags().BoolVar(&reviewJSON, "json", false, "Write the result as JSON (to stdout or --output)") cmd.Flags().StringVar(&reviewOutput, "output", "", "Write the result to a file instead of stdout") cmd.Flags().BoolVar(&reviewQuiet, "quiet", false, "Suppress stderr progress output") + cmd.Flags().BoolVar(&reviewNoPoll, "no-poll", false, "Submit the review and exit immediately; check status later via 'tusk review status '") cmd.Flags().SortFlags = false } @@ -142,26 +146,30 @@ func runReview(cmd *cobra.Command, args []string) error { // Quick stderr header for non-TTY callers so they know something's happening // even before the first progress poll. The backend will replace this with - // richer phase text once it starts rendering. + // richer phase text once it starts rendering. Note the "last pushed" label + // — this is the clone pivot (what the sandbox fetches), NOT the review + // base. The backend picks the review base branch server-side. if !reviewQuiet { baseLabel := patch.BaseRef if baseLabel == "" { - baseLabel = patch.BaseSha + baseLabel = patch.LastPushedSha } - shortSha := patch.BaseSha + shortSha := patch.LastPushedSha if len(shortSha) > 7 { shortSha = shortSha[:7] } - log.Stderrln(fmt.Sprintf("Reviewing %d lines across %d files (base: %s @ %s)", + log.Stderrln(fmt.Sprintf("Reviewing %d lines across %d files (last pushed: %s @ %s)", patch.ChangedLines, patch.FileCount, baseLabel, shortSha)) } createReq := &backend.CreateLocalCodeReviewRunRequest{ - OwnerName: owner, - RepoName: name, - BaseSha: patch.BaseSha, - Patch: patch.Patch, - CliVersion: fmt.Sprintf("tusk-cli/%s", version.Version), + OwnerName: owner, + RepoName: name, + LastPushedSha: patch.LastPushedSha, + Patch: patch.Patch, + CliVersion: fmt.Sprintf("tusk-cli/%s", version.Version), + BranchName: patch.BranchName, + LocalHeadSha: patch.LocalHeadSha, } if reviewMinSeverity != "" { s := reviewMinSeverity @@ -190,9 +198,33 @@ func runReview(cmd *cobra.Command, args []string) error { if api.IsPatchInvalidError(err) { return &ExitCodeError{Code: 2, Err: err} } + if api.IsNoSeatError(err) { + // Backend already produced the tailored remediation text + // (JWT-without-linked-username vs API-key-without-PR vs + // no-active-seat). Render verbatim. + return &ExitCodeError{Code: 2, Err: err} + } + if api.IsNotAuthorizedError(err) { + // Client's plan doesn't have the code-review feature. Backend + // message already points at the billing page — render verbatim. + return &ExitCodeError{Code: 2, Err: err} + } return formatApiError(err) } + // --no-poll: submit and exit. Skip cancellation-cleanup registration + // entirely — the backend run is now independent of this CLI process, + // and Ctrl+C shouldn't reach back and cancel it. + if reviewNoPoll { + if err := writeRunIdResult(runID, reviewJSON, reviewOutput); err != nil { + return err + } + if !reviewQuiet { + log.Stderrln(fmt.Sprintf("Run submitted. Check status with: tusk review status %s", runID)) + } + return nil + } + // Cancellation cleanup MUST be registered before the poll loop so that // Ctrl+C fires a backend cancel. Keep the timeout short — if the cancel // RPC itself hangs, we don't want to block process exit. @@ -263,7 +295,7 @@ func mapPatchError(err error) error { for _, p := range submodule.Paths { lines = append(lines, " "+p) } - lines = append(lines, "\nCommit submodule updates separately, or exclude them via:\n tusk review --exclude '/**'") + lines = append(lines, "\nCommit submodule updates separately, or exclude them via:\n tusk review run --exclude '/**'") return &ExitCodeError{Code: 2, Err: errors.New(strings.Join(lines, "\n"))} } return err @@ -283,6 +315,39 @@ func formatTopContributors(files []review.FileSummary) string { return sb.String() } +// writeRunIdResult is used by --no-poll: write just the run id to the +// selected sink so consumers can pipe it into `tusk review status`. In +// --json mode, wraps it in `{"runId": "..."}` for programmatic callers. +func writeRunIdResult(runID string, jsonMode bool, outputPath string) error { + out, cleanup, err := openOutputSink(outputPath) + if err != nil { + return err + } + defer cleanup() + + if jsonMode { + payload, _ := json.Marshal(map[string]string{"runId": runID}) + _, err := fmt.Fprintln(out, string(payload)) + return err + } + _, err = fmt.Fprintln(out, runID) + return err +} + +// openOutputSink returns a writer (plus a cleanup fn) for the given output +// path. Empty path → os.Stdout with a no-op cleanup. Non-empty → create + +// truncate + return a cleanup that closes the file. +func openOutputSink(outputPath string) (*os.File, func(), error) { + if outputPath == "" { + return os.Stdout, func() {}, nil + } + f, err := os.Create(outputPath) //nolint:gosec // user-specified path + if err != nil { + return nil, nil, fmt.Errorf("open --output: %w", err) + } + return f, func() { _ = f.Close() }, nil +} + // writeResult writes the backend-rendered final output to the selected sink // (stdout by default, or --output file). JSON mode writes display_json; // default mode writes display_message. diff --git a/cmd/review_run.go b/cmd/review_run.go new file mode 100644 index 00000000..01f95834 --- /dev/null +++ b/cmd/review_run.go @@ -0,0 +1,25 @@ +package cmd + +import ( + _ "embed" + + "github.com/spf13/cobra" + + "github.com/Use-Tusk/tusk-cli/internal/utils" +) + +//go:embed short_docs/review/run.md +var reviewRunContent string + +var reviewRunCmd = &cobra.Command{ + Use: "run", + Short: "Submit a code review for your local working tree", + Long: utils.RenderMarkdown(reviewRunContent), + SilenceUsage: true, + RunE: runReview, +} + +func init() { + reviewCmd.AddCommand(reviewRunCmd) + bindReviewFlags(reviewRunCmd) +} diff --git a/cmd/short_docs/review/overview.md b/cmd/short_docs/review/overview.md index 54035c0e..6767b312 100644 --- a/cmd/short_docs/review/overview.md +++ b/cmd/short_docs/review/overview.md @@ -1,63 +1,14 @@ # Tusk Review -`tusk review` runs the Tusk AI code review against your local working tree — before you push or open a PR. It uploads a git patch (not the whole repo), polls for completion, and prints the result to stdout. +Tusk Review runs the Tusk AI code review against your local working tree — before you push or open a PR. It uploads a git patch (not the whole repo), surfaces issues in the terminal, and never posts comments to GitHub or GitLab. -The CLI never posts comments to GitHub or GitLab. All output is local. +## Subcommands -## Typical workflow - -1. Make some changes in a repo that's connected to Tusk. -2. Run `tusk review` from the repo directory. -3. Stderr shows progress; stdout shows the final review when it's done. +- `tusk review run` — submit a review of your current working tree. +- `tusk review status ` — check the status of a previously submitted run (pair with `--watch` to block until it finishes). ## Authentication Run `tusk auth login`, or set `TUSK_API_KEY` for non-interactive use. -## Repo identity - -By default, the repo is detected from the `origin` remote (`owner/name`). Override with `--repo owner/name`. The repo must already be connected to Tusk. - -## Base branch resolution - -The "base" is the commit your changes sit on top of. By default, `tusk review` uses `git merge-base origin/HEAD HEAD` — the point your branch diverged from origin's default branch. - -Pass `--base ` to override. This is the right thing to do on **stacked branches** (feature-2 branched off feature-1 branched off main): without it, your review will critique feature-1's changes as if they were yours. - -``` -main: A ─ B ─ C - \ -feature-1: D ─ E (open PR, not merged) - \ -feature-2: F ─ G (current branch) -``` - -Here, run `tusk review --base origin/feature-1` so the diff is just F–G. - -## Output - -- Default: human-readable text to stdout, progress on stderr. -- `--json`: backend-rendered JSON document to stdout, suitable for `| jq`. -- `--output `: write the result to a file (format follows `--json`). -- `--quiet`: suppress stderr progress (final output unchanged). - -## Filtering - -Locked files and common build output are skipped automatically (same list the server-side review uses). To tweak: - -- `.tuskignore` at the repo root: `.gitignore`-style globs, additive to the defaults. -- `--exclude `: one-off add. Repeatable. -- `--include `: cancel a default skip (e.g. `--include 'package-lock.json'`). Repeatable. - -## Exit codes - -- `0` — review completed (issues may or may not be found) -- `1` — run failed, or network / auth error -- `2` — user-actionable pre-flight error (mid-rebase, rate limit, patch too large, repo not connected, couldn't determine base) - -## Status subcommand - -``` -tusk review status # print current status snapshot -tusk review status --watch # block until run reaches a terminal state -``` +Run `tusk review --help` for flags, exit codes, and behavior specific to each subcommand. diff --git a/cmd/short_docs/review/run.md b/cmd/short_docs/review/run.md new file mode 100644 index 00000000..120c3319 --- /dev/null +++ b/cmd/short_docs/review/run.md @@ -0,0 +1,53 @@ +# Tusk Review Run + +`tusk review run` submits a code review for your local working tree. The CLI builds a git patch, uploads it, polls for completion, and prints the result to stdout. Nothing is pushed to your remote, and the CLI never posts comments to GitHub or GitLab — all output is local. + +## Typical workflow + +1. Make some changes in a repo that's connected to Tusk. +2. Run `tusk review run` from the repo directory. +3. Stderr shows progress; stdout shows the final review when it's done. + +## Repo identity + +By default, the repo is detected from the `origin` remote (`owner/name`). Override with `--repo owner/name`. The repo must already be connected to Tusk. + +## Output + +- Default: human-readable text to stdout, progress on stderr. +- `--json`: backend-rendered JSON document to stdout, suitable for `| jq`. +- `--output `: write the result to a file (format follows `--json`). +- `--quiet`: suppress stderr progress (final output unchanged). + +## Filtering + +Lockfiles and common build output are skipped automatically (same list the server-side review uses). To tweak: + +- `.tuskignore` at the repo root: `.gitignore`-style globs, additive to the defaults. +- `--exclude `: one-off add. Repeatable. +- `--include `: cancel a default skip (e.g. `--include 'package-lock.json'`). Repeatable. + +## Fire-and-forget (`--no-poll`) + +`tusk review run --no-poll` uploads the patch, prints the run id to stdout, and exits immediately — no polling, no blocking. Useful for CI scripts and for kicking off a review from one machine and reading it on another. + +``` +$ tusk review run --no-poll +cr_01JK7... +$ tusk review status cr_01JK7... --watch +``` + +With `--json`: + +``` +$ tusk review run --no-poll --json +{"runId":"cr_01JK7..."} +``` + +Ctrl+C on a `--no-poll` run has no effect on the backend (the run is already submitted and independent of the CLI process). + +## Exit codes + +- `0` — review completed (issues may or may not be found) +- `1` — run failed, or network / auth error +- `2` — user-actionable pre-flight error (mid-rebase, rate limit, patch too large, repo not connected, couldn't determine clone pivot, no active seat) diff --git a/go.mod b/go.mod index 857ebb10..249a190a 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.25.0 require ( github.com/Use-Tusk/fence v0.1.36 - github.com/Use-Tusk/tusk-drift-schemas v0.1.35 + github.com/Use-Tusk/tusk-drift-schemas v0.1.36 github.com/agnivade/levenshtein v1.0.3 github.com/aymanbagabas/go-osc52/v2 v2.0.1 github.com/bmatcuk/doublestar/v4 v4.10.0 diff --git a/go.sum b/go.sum index 78e84f28..1d18344e 100644 --- a/go.sum +++ b/go.sum @@ -33,8 +33,8 @@ github.com/STARRY-S/zip v0.2.3 h1:luE4dMvRPDOWQdeDdUxUoZkzUIpTccdKdhHHsQJ1fm4= github.com/STARRY-S/zip v0.2.3/go.mod h1:lqJ9JdeRipyOQJrYSOtpNAiaesFO6zVDsE8GIGFaoSk= github.com/Use-Tusk/fence v0.1.36 h1:8S15y8cp3X+xXukx6AN0Ky/aX9/dZyW3fLw5XOQ8YtE= github.com/Use-Tusk/fence v0.1.36/go.mod h1:YkowBDzXioVKJE16vg9z3gSVC6vhzkIZZw2dFf7MW/o= -github.com/Use-Tusk/tusk-drift-schemas v0.1.35 h1:s8Rknx8zMMRWCoe+YLQWmlB58T45lcZwI8TBK4+8Mx4= -github.com/Use-Tusk/tusk-drift-schemas v0.1.35/go.mod h1:pa3EvTj9kKxl9f904RVFkj9YK1zB75QogboKi70zalM= +github.com/Use-Tusk/tusk-drift-schemas v0.1.36 h1:baojaWiEFEdRU61CLYAbFievXxDLlWTFW/ijL4IpdiE= +github.com/Use-Tusk/tusk-drift-schemas v0.1.36/go.mod h1:pa3EvTj9kKxl9f904RVFkj9YK1zB75QogboKi70zalM= github.com/agnivade/levenshtein v1.0.3 h1:M5ZnqLOoZR8ygVq0FfkXsNOKzMCk0xRiow0R5+5VkQ0= github.com/agnivade/levenshtein v1.0.3/go.mod h1:4SFRZbbXWLF4MU1T9Qg0pGgH3Pjs+t6ie5efyrwRJXs= github.com/alecthomas/assert/v2 v2.7.0 h1:QtqSACNS3tF7oasA8CU6A6sXZSBDqnm7RfpLl9bZqbE= diff --git a/internal/api/code_review.go b/internal/api/code_review.go index 888b0604..75b64f1f 100644 --- a/internal/api/code_review.go +++ b/internal/api/code_review.go @@ -52,6 +52,38 @@ func IsPatchInvalidError(err error) bool { return errors.As(err, &p) } +// NoSeatError signals the caller isn't entitled to run a review. The +// backend tailors the message per cause (JWT without linked code-hosting +// username; API key with no PR on the branch; user with no active seat). +// The CLI renders the message verbatim. +type NoSeatError struct { + Message string +} + +func (e *NoSeatError) Error() string { return e.Message } + +// IsNoSeatError reports whether err (or anything it wraps) is a *NoSeatError. +func IsNoSeatError(err error) bool { + var n *NoSeatError + return errors.As(err, &n) +} + +// NotAuthorizedError signals the caller's org (or client plan) doesn't +// have the code-review feature enabled. Distinct from NoSeatError — no +// per-user remediation; requires a plan / admin change. Backend-supplied +// message is rendered verbatim. +type NotAuthorizedError struct { + Message string +} + +func (e *NotAuthorizedError) Error() string { return e.Message } + +// IsNotAuthorizedError reports whether err (or anything it wraps) is a *NotAuthorizedError. +func IsNotAuthorizedError(err error) bool { + var n *NotAuthorizedError + return errors.As(err, &n) +} + func (c *TuskClient) CreateLocalCodeReviewRun(ctx context.Context, in *backend.CreateLocalCodeReviewRunRequest, auth AuthOptions) (string, error) { var out backend.CreateLocalCodeReviewRunResponse if err := c.makeCodeReviewServiceRequest(ctx, "create_local_code_review_run", in, &out, auth, DefaultRetryConfig(3)); err != nil { @@ -72,8 +104,14 @@ func (c *TuskClient) CreateLocalCodeReviewRun(ctx context.Context, in *backend.C return "", &RepoNotFoundError{Message: e.GetMessage()} case backend.CreateLocalCodeReviewRunResponseErrorCode_CREATE_LOCAL_CODE_REVIEW_RUN_RESPONSE_ERROR_CODE_PATCH_INVALID: return "", &PatchInvalidError{Message: e.GetMessage()} + case backend.CreateLocalCodeReviewRunResponseErrorCode_CREATE_LOCAL_CODE_REVIEW_RUN_RESPONSE_ERROR_CODE_NO_SEAT: + return "", &NoSeatError{Message: e.GetMessage()} + case backend.CreateLocalCodeReviewRunResponseErrorCode_CREATE_LOCAL_CODE_REVIEW_RUN_RESPONSE_ERROR_CODE_NOT_AUTHORIZED: + return "", &NotAuthorizedError{Message: e.GetMessage()} } - return "", fmt.Errorf("%s: %s", e.GetCode().String(), e.GetMessage()) + // Fallback for unmapped codes: surface the backend's human-readable + // message only. The proto enum name is not user-facing. + return "", fmt.Errorf("%s", e.GetMessage()) } return "", fmt.Errorf("invalid response") } @@ -88,7 +126,9 @@ func (c *TuskClient) GetCodeReviewRunStatus(ctx context.Context, in *backend.Get return s, nil } if e := out.GetError(); e != nil { - return nil, fmt.Errorf("%s: %s", e.GetCode().String(), e.GetMessage()) + // Surface only the backend's human-readable message; the proto enum + // name is not user-facing. + return nil, fmt.Errorf("%s", e.GetMessage()) } return nil, fmt.Errorf("invalid response") } @@ -104,7 +144,9 @@ func (c *TuskClient) CancelCodeReviewRun(ctx context.Context, in *backend.Cancel return nil } if e := out.GetError(); e != nil { - return fmt.Errorf("%s: %s", e.GetCode().String(), e.GetMessage()) + // Surface only the backend's human-readable message; the proto enum + // name is not user-facing. + return fmt.Errorf("%s", e.GetMessage()) } return fmt.Errorf("invalid response") } diff --git a/internal/review/patch.go b/internal/review/patch.go index da0a7413..b304fb03 100644 --- a/internal/review/patch.go +++ b/internal/review/patch.go @@ -32,13 +32,25 @@ type FileSummary struct { } // PatchResult is what BuildPatch returns on success. +// +// Terminology (see v2-plan.md "Three roles of SHAs / refs"): +// - LastPushedSha is the commit the sandbox will clone. It's guaranteed +// reachable on origin because the CLI resolves it from a remote-tracking +// ref (@{u}) or, for branches with no upstream, from merge-base with +// origin/HEAD. The uploaded patch is the diff between this SHA and the +// working tree. +// - Review scope is NOT set from this SHA. The backend picks the review +// base branch (open PR's base, or repo default) server-side and diffs +// the reconstructed working tree against that. type PatchResult struct { - Patch []byte - BaseSha string - BaseRef string // The ref the base resolved from (e.g. "origin/main"), informational. - ChangedFiles []FileSummary - ChangedLines int - FileCount int + Patch []byte + LastPushedSha string + BaseRef string // The ref label the pivot resolved from (e.g. "@{u}", "origin/HEAD", or the user's --base value). + BranchName string // Current branch (empty string on detached HEAD). + LocalHeadSha string // git rev-parse HEAD — informational, sent to backend for audit. + ChangedFiles []FileSummary + ChangedLines int + FileCount int } // ErrEmptyPatch is returned when the working tree diff against the base is @@ -132,11 +144,23 @@ func BuildPatch(ctx context.Context, opts PatchOptions) (*PatchResult, error) { defer restore() } - baseSha, baseRef, err := resolveBase(ctx, opts.RepoRoot, opts.Base) + lastPushedSha, baseRef, err := resolveBase(ctx, opts.RepoRoot, opts.Base) if err != nil { return nil, err } + // Branch name and local HEAD SHA — sent to the backend so it can look up + // the open PR for this branch (seat resolution, base-branch defaulting, + // PR context). Detached HEAD → empty branch name; backend tolerates it. + branchName, err := currentBranchName(ctx, opts.RepoRoot) + if err != nil { + return nil, fmt.Errorf("resolve current branch: %w", err) + } + localHeadSha, err := resolveLocalHead(ctx, opts.RepoRoot) + if err != nil { + return nil, fmt.Errorf("resolve HEAD: %w", err) + } + // Merge .tuskignore entries with --exclude flags. tuskignoreExtras, err := ReadTuskignore(opts.RepoRoot) if err != nil { @@ -148,7 +172,7 @@ func BuildPatch(ctx context.Context, opts PatchOptions) (*PatchResult, error) { pathspecs := BuildPathspecExclusions(extraExcludes, opts.Includes) // Generate binary patch. - diffArgs := append([]string{"diff", "--binary", baseSha, "--"}, pathspecs...) + diffArgs := append([]string{"diff", "--binary", lastPushedSha, "--"}, pathspecs...) patchBytes, err := gitOutput(ctx, opts.RepoRoot, diffArgs...) if err != nil { return nil, fmt.Errorf("git diff --binary: %w", err) @@ -156,7 +180,7 @@ func BuildPatch(ctx context.Context, opts PatchOptions) (*PatchResult, error) { // Generate numstat for per-file line counts. `-z` writes NUL-terminated // records so paths containing spaces or other whitespace survive intact. - numstatArgs := append([]string{"diff", "--numstat", "-z", baseSha, "--"}, pathspecs...) + numstatArgs := append([]string{"diff", "--numstat", "-z", lastPushedSha, "--"}, pathspecs...) numstatBytes, err := gitOutput(ctx, opts.RepoRoot, numstatArgs...) if err != nil { return nil, fmt.Errorf("git diff --numstat: %w", err) @@ -224,12 +248,14 @@ func BuildPatch(ctx context.Context, opts PatchOptions) (*PatchResult, error) { } return &PatchResult{ - Patch: patchBytes, - BaseSha: baseSha, - BaseRef: baseRef, - ChangedFiles: files, - ChangedLines: totalLines, - FileCount: fileCount, + Patch: patchBytes, + LastPushedSha: lastPushedSha, + BaseRef: baseRef, + BranchName: branchName, + LocalHeadSha: localHeadSha, + ChangedFiles: files, + ChangedLines: totalLines, + FileCount: fileCount, }, nil } @@ -259,17 +285,26 @@ func listUntracked(ctx context.Context, repoRoot string) ([]string, error) { return untracked, nil } -// resolveBase turns the user-provided base (or empty → origin/HEAD auto-detect) -// into a resolved commit SHA. Returns (sha, ref) where ref is the original -// input (informational) or "origin/HEAD" for auto-detection. +// resolveBase picks the commit the sandbox will clone (and that the upload +// patch is relative to) — NOT the review base branch. v2 semantics: per the +// v2-plan "Three roles of SHAs" table, this is the `last_pushed_sha`. +// +// Resolution order: +// 1. Explicit --base : user override. Resolved via `git rev-parse`. +// 2. @{u} (upstream tracking ref): preferred auto-detect. If set, this is +// the commit this branch currently points at on origin, guaranteed +// reachable for the sandbox clone. +// 3. Fallback: `merge-base origin/HEAD HEAD` — used when the branch has no +// upstream (never pushed / tracking not configured). Requires +// origin/HEAD to be set on the clone; this is the only path that +// invokes CheckOriginHead. +// +// Returns (sha, refLabel) where refLabel is a short human-readable label +// for the stderr header: the user's --base string, "@{u}", or "origin/HEAD". func resolveBase(ctx context.Context, repoRoot string, userBase string) (string, string, error) { if userBase != "" { out, err := gitOutput(ctx, repoRoot, "rev-parse", "--verify", userBase+"^{commit}") if err != nil { - // `git rev-parse --verify` writes its failure reason to stderr - // (e.g. "fatal: Needed a single revision"), which `gitOutput` - // has already captured into err. `out` (stdout) is empty on - // failure — using it here would produce an empty reason. return "", "", &BaseResolutionError{ Message: fmt.Sprintf("couldn't resolve --base %q to a commit: %s\n\nTry:\n tusk review --base origin/main", userBase, strings.TrimSpace(err.Error())), @@ -278,7 +313,13 @@ func resolveBase(ctx context.Context, repoRoot string, userBase string) (string, return strings.TrimSpace(string(out)), userBase, nil } - // Auto-detect via origin/HEAD. + // Preferred: upstream tracking ref. Only works if the branch was pushed + // (or configured to track a remote branch). + if sha, err := resolveUpstream(ctx, repoRoot); err == nil { + return sha, "@{u}", nil + } + + // Fallback: merge-base with origin's default branch. Requires origin/HEAD. if err := CheckOriginHead(repoRoot); err != nil { return "", "", err } @@ -286,20 +327,66 @@ func resolveBase(ctx context.Context, repoRoot string, userBase string) (string, if err != nil { shallow := isShallow(repoRoot) msg := "couldn't determine base commit for this branch.\n\n" - msg += "Cause: `git merge-base origin/HEAD HEAD` failed — this branch may not share history with origin's default branch." + msg += "Cause: your branch has no upstream (try `git push -u origin `) and `git merge-base origin/HEAD HEAD` also failed." if shallow { - msg += " Also: this is a shallow clone." + msg += " Additionally, this is a shallow clone." } msg += "\n\nThings to try:" if shallow { msg += "\n • git fetch --unshallow" } - msg += "\n • Pass the base explicitly: tusk review --base " + msg += "\n • Push this branch so its upstream is set: git push -u origin " + msg += "\n • Or pass the base explicitly: tusk review --base " return "", "", &BaseResolutionError{Message: msg} } return strings.TrimSpace(string(baseOut)), "origin/HEAD", nil } +// resolveUpstream returns the commit SHA that this branch's upstream +// (`@{u}`) currently points at. Non-nil error = no upstream configured or +// the ref has gone away; callers treat that as "fall through to +// merge-base." Never produces a BaseResolutionError — the failure here is +// expected whenever the branch is unpushed. +func resolveUpstream(ctx context.Context, repoRoot string) (string, error) { + out, err := gitOutput(ctx, repoRoot, "rev-parse", "--verify", "@{u}") + if err != nil { + return "", err + } + sha := strings.TrimSpace(string(out)) + if sha == "" { + return "", fmt.Errorf("empty @{u} resolution") + } + return sha, nil +} + +// currentBranchName returns the current branch (e.g. "feature/foo"), or an +// empty string on detached HEAD. Anything else (git missing, broken repo) +// is returned as an error. +// +// Kept in `internal/review` so this package stays self-contained — the +// cmd-package `getCurrentGitBranch` helper in unit_helpers.go does similar +// work but treats detached HEAD as a hard error, which we don't want here: +// v2 spec explicitly tolerates detached HEAD on the CLI side (the backend +// resolves PR context via branch name when available, repo default otherwise). +func currentBranchName(ctx context.Context, repoRoot string) (string, error) { + out, err := gitOutput(ctx, repoRoot, "branch", "--show-current") + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} + +// resolveLocalHead returns `git rev-parse HEAD` — the full SHA of whatever +// commit the working tree is currently checked out on. Used only for audit; +// the backend records it on LocalCheckCommit. +func resolveLocalHead(ctx context.Context, repoRoot string) (string, error) { + out, err := gitOutput(ctx, repoRoot, "rev-parse", "HEAD") + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} + // parseNumstat parses the output of `git diff --numstat -z` into per-file // summaries. Binary files show "- -" for their counts; we record them as // zero changed lines but still include them in the file count. diff --git a/internal/review/preflight.go b/internal/review/preflight.go index b0cead9e..e0b44797 100644 --- a/internal/review/preflight.go +++ b/internal/review/preflight.go @@ -83,7 +83,7 @@ func Preflight(repoRoot string) error { if _, err := os.Stat(op.path); err == nil { return &PreflightError{ Message: fmt.Sprintf( - "working tree is mid-%s. Finish (`%s`) before running `tusk review`.", + "working tree is mid-%s. Finish (`%s`) before running `tusk review run`.", op.name, op.next), } } From 89b76b5c5c294c41b227a45e8508883a7b5f3286 Mon Sep 17 00:00:00 2001 From: Sohan Kshirsagar Date: Thu, 23 Apr 2026 10:36:17 -0700 Subject: [PATCH 6/7] address pr comment feedback --- cmd/review_status.go | 2 +- internal/review/patch.go | 4 ++-- internal/review/preflight.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/review_status.go b/cmd/review_status.go index 0a8daf47..0d8c4d38 100644 --- a/cmd/review_status.go +++ b/cmd/review_status.go @@ -23,7 +23,7 @@ func init() { reviewCmd.AddCommand(reviewStatusCmd) // Reuse the main review command's --json, --output, --quiet globals so - // `tusk review status ... --json` behaves identically to `tusk review --json`. + // `tusk review status ... --json` behaves identically to `tusk review run --json`. reviewStatusCmd.Flags().BoolVar(&reviewJSON, "json", false, "Write the result as JSON (to stdout or --output)") reviewStatusCmd.Flags().StringVar(&reviewOutput, "output", "", "Write the result to a file instead of stdout") reviewStatusCmd.Flags().BoolVar(&reviewQuiet, "quiet", false, "Suppress stderr progress output") diff --git a/internal/review/patch.go b/internal/review/patch.go index b304fb03..f2d909f6 100644 --- a/internal/review/patch.go +++ b/internal/review/patch.go @@ -306,7 +306,7 @@ func resolveBase(ctx context.Context, repoRoot string, userBase string) (string, out, err := gitOutput(ctx, repoRoot, "rev-parse", "--verify", userBase+"^{commit}") if err != nil { return "", "", &BaseResolutionError{ - Message: fmt.Sprintf("couldn't resolve --base %q to a commit: %s\n\nTry:\n tusk review --base origin/main", + Message: fmt.Sprintf("couldn't resolve --base %q to a commit: %s\n\nTry:\n tusk review run --base origin/main", userBase, strings.TrimSpace(err.Error())), } } @@ -336,7 +336,7 @@ func resolveBase(ctx context.Context, repoRoot string, userBase string) (string, msg += "\n • git fetch --unshallow" } msg += "\n • Push this branch so its upstream is set: git push -u origin " - msg += "\n • Or pass the base explicitly: tusk review --base " + msg += "\n • Or pass the base explicitly: tusk review run --base " return "", "", &BaseResolutionError{Message: msg} } return strings.TrimSpace(string(baseOut)), "origin/HEAD", nil diff --git a/internal/review/preflight.go b/internal/review/preflight.go index e0b44797..f2b5487b 100644 --- a/internal/review/preflight.go +++ b/internal/review/preflight.go @@ -156,7 +156,7 @@ func CheckOriginHead(repoRoot string) error { if shallow { msg += "\n git fetch --unshallow" } - msg += "\n\nOr pass --base explicitly:\n tusk review --base main" + msg += "\n\nOr pass --base explicitly:\n tusk review run --base main" return &PreflightError{Message: msg} } return nil From 1358862db8348e7cdb41865592e51095f5025851 Mon Sep 17 00:00:00 2001 From: Sohan Kshirsagar Date: Thu, 23 Apr 2026 14:38:08 -0700 Subject: [PATCH 7/7] add doc, remove --base --- cmd/review.go | 4 -- docs/review/README.md | 123 +++++++++++++++++++++++++++++++++++ internal/review/patch.go | 32 +++------ internal/review/preflight.go | 17 ++--- 4 files changed, 139 insertions(+), 37 deletions(-) create mode 100644 docs/review/README.md diff --git a/cmd/review.go b/cmd/review.go index d70b98e0..731c4c7d 100644 --- a/cmd/review.go +++ b/cmd/review.go @@ -25,7 +25,6 @@ var reviewOverviewContent string var ( reviewRepo string - reviewBase string reviewMinSeverity string reviewExcludes []string reviewIncludes []string @@ -54,7 +53,6 @@ func init() { func bindReviewFlags(cmd *cobra.Command) { cmd.Flags().StringVar(&reviewRepo, "repo", "", "Repository in owner/name format (defaults to git origin remote)") - cmd.Flags().StringVar(&reviewBase, "base", "", "Override the clone pivot (defaults to your branch's upstream @{u}, then merge-base with origin/HEAD)") cmd.Flags().StringVar(&reviewMinSeverity, "min-severity", "", "Minimum severity to surface: low|medium|high|critical") cmd.Flags().StringArrayVar(&reviewExcludes, "exclude", nil, "Extra path glob(s) to exclude from the patch (repeatable)") cmd.Flags().StringArrayVar(&reviewIncludes, "include", nil, "Cancel a default skip for matching files (repeatable)") @@ -100,7 +98,6 @@ func runReview(cmd *cobra.Command, args []string) error { log.Debug("Starting tusk review", "repo", reviewRepo, - "base", reviewBase, "min-severity", reviewMinSeverity, "json", reviewJSON, "output", reviewOutput, @@ -126,7 +123,6 @@ func runReview(cmd *cobra.Command, args []string) error { patch, err := review.BuildPatch(ctx, review.PatchOptions{ RepoRoot: repoRoot, - Base: reviewBase, ExtraExcludes: reviewExcludes, Includes: reviewIncludes, RegisterCleanup: RegisterCleanup, diff --git a/docs/review/README.md b/docs/review/README.md new file mode 100644 index 00000000..d9b33d86 --- /dev/null +++ b/docs/review/README.md @@ -0,0 +1,123 @@ +# Tusk Review + +Tusk Review provides CLI commands for running the [Tusk](https://usetusk.ai) AI code review against your local working tree — before you push or open a pull request. Surfaces issues in the terminal; never posts comments to GitHub or GitLab. + +## Prerequisites + +Authenticate with Tusk: + +```bash +tusk auth login +``` + +Or set the `TUSK_API_KEY` environment variable. See [onboarding docs](https://docs.usetusk.ai/onboarding) for details. + +The repo you're in must already be connected to Tusk (one-time setup by any user in your org). The CLI detects the repo from the `origin` remote; override with `--repo owner/name`. + +## Commands + +### `tusk review run` + +Submit a code review for your current working tree. Generates a git patch (including uncommitted and untracked changes), uploads it, polls for completion, and prints the result. + +```bash +tusk review run +tusk review run --json | jq . +tusk review run --output review.txt +tusk review run --min-severity high +``` + +Common flags: + +- `--repo owner/name` — override the repo (defaults to `origin` remote). +- `--min-severity low|medium|high|critical` — severity floor for surfaced issues. +- `--exclude ` / `--include ` — extra path excludes, or re-include a default skip. Repeatable. +- `--json` — backend-rendered JSON to stdout (pipe to `jq`). +- `--output ` — write the result to a file instead of stdout. +- `--quiet` — suppress stderr progress. +- `--no-poll` — submit and exit immediately with the run id (see [Fire-and-forget](#fire-and-forget) below). + +### `tusk review status ` + +Check the status of a previously submitted run. Defaults to a one-shot snapshot. + +```bash +tusk review status cr_01JK7... +tusk review status cr_01JK7... --watch +tusk review status cr_01JK7... --json +``` + +Pair with `--watch` to block until the run reaches a terminal state (SUCCESS, FAILED, or CANCELLED). Honors the same `--json`, `--output`, and `--quiet` flags as `run`. + +## Typical workflow + +1. Make changes in a repo connected to Tusk. + +2. Run the review: + + ```bash + tusk review run + ``` + + Stderr shows progress; stdout prints the final review when it's done (usually 3–5 min). + +3. Fix issues locally. + +## Fire-and-forget + +Use `--no-poll` to submit the review and exit immediately. Useful for kicking off a review from one machine and reading it elsewhere or whenever you don't want to block a terminal: + +```bash +$ tusk review run --no-poll +cr_01JK7... +$ tusk review status cr_01JK7... --watch +``` + +With `--json`, the run id is wrapped for programmatic consumers: + +```bash +$ tusk review run --no-poll --json +{"runId":"cr_01JK7..."} +``` + +Ctrl+C on a `--no-poll` run has no effect on the backend — the run is independent of the CLI process. + +## Filtering + +Lockfiles, binaries, and common build output are skipped automatically (same list the server-side review uses). To tweak: + +- `.tuskignore` at the repo root — `.gitignore`-style globs, additive to the defaults. +- `--exclude ` — one-off add. Repeatable. +- `--include ` — cancel a default skip (e.g. `--include 'package-lock.json'`). Repeatable. + +If everything you changed is filtered out (e.g. a pure `package-lock.json` bump), the CLI exits 0 with "Nothing to review" — no wasted server-side run. + +## Size limits + +The CLI enforces patch size limits _before_ upload: + +- **Hard caps**: 10,000 changed lines / 200 files / 1 MiB patch bytes. The CLI aborts with a top-contributors list. +- **Soft warnings** (stderr, continues): 2,000 lines / 50 files. + +If you hit a hard cap, the error points at the biggest contributing files — usually unfiltered generated code or lockfiles not caught by the default filter. Add them to `.tuskignore` and rerun. + +## Exit codes + +- `0` — review completed (issues may or may not be found), or nothing to review after filtering. +- `1` — run failed (sandbox error, patch-apply failure, timeout, auth, network). +- `2` — user-actionable pre-flight or request failure: + - mid-rebase / merge / cherry-pick / revert in the working tree + - patch too large + - submodule changes in the patch (not supported in v1) + - repo not connected to Tusk under your current org + - rate limit hit (with next-allowed timestamp) + - no active seat for your code-hosting identity + +## JSON output + +The JSON schema is backend-rendered and documented server-side. Pipe through `jq` for filtering: + +```bash +# Pretty-print +tusk review run --json --quiet | jq . +``` diff --git a/internal/review/patch.go b/internal/review/patch.go index f2d909f6..cbd93790 100644 --- a/internal/review/patch.go +++ b/internal/review/patch.go @@ -45,7 +45,7 @@ type FileSummary struct { type PatchResult struct { Patch []byte LastPushedSha string - BaseRef string // The ref label the pivot resolved from (e.g. "@{u}", "origin/HEAD", or the user's --base value). + BaseRef string // The ref label the pivot resolved from ("@{u}" or "origin/HEAD"). BranchName string // Current branch (empty string on detached HEAD). LocalHeadSha string // git rev-parse HEAD — informational, sent to backend for audit. ChangedFiles []FileSummary @@ -99,7 +99,6 @@ func IsBaseResolutionError(err error) bool { // PatchOptions drives BuildPatch. type PatchOptions struct { RepoRoot string - Base string // user-provided --base ref/sha; empty → auto-detect via origin/HEAD ExtraExcludes []string Includes []string RegisterCleanup func(fn func()) // typically cmd.RegisterCleanup; may be nil for tests @@ -144,7 +143,7 @@ func BuildPatch(ctx context.Context, opts PatchOptions) (*PatchResult, error) { defer restore() } - lastPushedSha, baseRef, err := resolveBase(ctx, opts.RepoRoot, opts.Base) + lastPushedSha, baseRef, err := resolveBase(ctx, opts.RepoRoot) if err != nil { return nil, err } @@ -290,29 +289,17 @@ func listUntracked(ctx context.Context, repoRoot string) ([]string, error) { // v2-plan "Three roles of SHAs" table, this is the `last_pushed_sha`. // // Resolution order: -// 1. Explicit --base : user override. Resolved via `git rev-parse`. -// 2. @{u} (upstream tracking ref): preferred auto-detect. If set, this is -// the commit this branch currently points at on origin, guaranteed -// reachable for the sandbox clone. -// 3. Fallback: `merge-base origin/HEAD HEAD` — used when the branch has no +// 1. @{u} (upstream tracking ref): preferred. If set, this is the commit +// this branch currently points at on origin, guaranteed reachable for +// the sandbox clone. +// 2. Fallback: `merge-base origin/HEAD HEAD` — used when the branch has no // upstream (never pushed / tracking not configured). Requires // origin/HEAD to be set on the clone; this is the only path that // invokes CheckOriginHead. // // Returns (sha, refLabel) where refLabel is a short human-readable label -// for the stderr header: the user's --base string, "@{u}", or "origin/HEAD". -func resolveBase(ctx context.Context, repoRoot string, userBase string) (string, string, error) { - if userBase != "" { - out, err := gitOutput(ctx, repoRoot, "rev-parse", "--verify", userBase+"^{commit}") - if err != nil { - return "", "", &BaseResolutionError{ - Message: fmt.Sprintf("couldn't resolve --base %q to a commit: %s\n\nTry:\n tusk review run --base origin/main", - userBase, strings.TrimSpace(err.Error())), - } - } - return strings.TrimSpace(string(out)), userBase, nil - } - +// for the stderr header: "@{u}" or "origin/HEAD". +func resolveBase(ctx context.Context, repoRoot string) (string, string, error) { // Preferred: upstream tracking ref. Only works if the branch was pushed // (or configured to track a remote branch). if sha, err := resolveUpstream(ctx, repoRoot); err == nil { @@ -327,7 +314,7 @@ func resolveBase(ctx context.Context, repoRoot string, userBase string) (string, if err != nil { shallow := isShallow(repoRoot) msg := "couldn't determine base commit for this branch.\n\n" - msg += "Cause: your branch has no upstream (try `git push -u origin `) and `git merge-base origin/HEAD HEAD` also failed." + msg += "Cause: your branch has no upstream and `git merge-base origin/HEAD HEAD` failed." if shallow { msg += " Additionally, this is a shallow clone." } @@ -336,7 +323,6 @@ func resolveBase(ctx context.Context, repoRoot string, userBase string) (string, msg += "\n • git fetch --unshallow" } msg += "\n • Push this branch so its upstream is set: git push -u origin " - msg += "\n • Or pass the base explicitly: tusk review run --base " return "", "", &BaseResolutionError{Message: msg} } return strings.TrimSpace(string(baseOut)), "origin/HEAD", nil diff --git a/internal/review/preflight.go b/internal/review/preflight.go index f2b5487b..d3d70841 100644 --- a/internal/review/preflight.go +++ b/internal/review/preflight.go @@ -90,13 +90,13 @@ func Preflight(repoRoot string) error { } // `origin` presence is NOT checked here — either: - // - the user passed --repo + --base (no origin needed at all), OR + // - the user passed --repo (no origin needed for repo identity), OR // - the repo-identity step (resolveReviewRepo) will fail with a // specific "run inside a git repo with an origin remote" message, OR // - the base-resolution step will call CheckOriginHead for a targeted - // "origin/HEAD not set" error. - // Firing a generic origin-missing error in preflight would block the - // --repo + --base bypass that this command is designed to allow. + // "origin/HEAD not set" error when the fallback is needed. + // Firing a generic origin-missing error in preflight would produce + // unhelpful errors for users who provided --repo. // Detached HEAD is a warning, not a refusal. // @@ -139,11 +139,9 @@ func isDetachedHEAD(repoRoot string) (bool, error) { } // CheckOriginHead confirms that `origin/HEAD` is set on this clone, which -// `git merge-base` relies on for auto-detecting the base branch. Returns a -// *PreflightError with remediation text when missing. -// -// Only called when the user did NOT pass `--base` — explicit base bypasses -// the origin/HEAD requirement. +// `git merge-base` relies on for auto-detecting the clone pivot when the +// branch has no upstream. Returns a *PreflightError with remediation text +// when missing. func CheckOriginHead(repoRoot string) error { if err := runGitSilent(repoRoot, "symbolic-ref", "refs/remotes/origin/HEAD"); err != nil { shallow := isShallow(repoRoot) @@ -156,7 +154,6 @@ func CheckOriginHead(repoRoot string) error { if shallow { msg += "\n git fetch --unshallow" } - msg += "\n\nOr pass --base explicitly:\n tusk review run --base main" return &PreflightError{Message: msg} } return nil