diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bf5e0c..a9c03ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,8 +17,8 @@ and the project adheres to [Semantic Versioning 2.0.0](https://semver.org/spec/v `--provider codex-cli`; no API key needed (reuses the host CLI's auth). Driven through `codex exec --sandbox read-only --skip-git-repo-check` (non-interactive, read-only). Like the other CLI providers it is a plain-text emitter — - no structured findings, so `--json` / `--markdown` / `remote pr` do not - apply. + no structured findings, so `--json` / `--markdown` and `remote pr`'s + posting mode don't apply (but `remote pr --no-post`, added below, does). - **SPDX-header CI guard.** `make spdx-check` (`scripts/spdx-check.sh`, folded into `make check` and a dedicated CI job) fails the build if any Go source — tracked or newly added — is missing its @@ -57,6 +57,14 @@ and the project adheres to [Semantic Versioning 2.0.0](https://semver.org/spec/v the truly bare invocation — any explicit flag or subcommand bypasses it. Expanded git-alias style before argument parsing (ADR-0005). Set via `config set -- command.default "…"` or by editing `config.yml`. +- **`remote pr --no-post` — review a PR locally, no GitHub writes.** + Fetches the PR diff via `gh` and renders the review to your terminal + like a local review, posting nothing to GitHub (no inline comments, no + verdict). Because output is local, the flags posting mode rejects now + apply: `--json`, `--markdown`, `--output`, `--copy`, `--compact`, + `--cli` (CLI providers), and `--fail-on` (exit code). No self-PR block; + results are cached like a local review. `--request-changes-on` / + `--with-context` are noted-and-ignored in this mode (ADR-0016 §Update). ## [1.2.1] diff --git a/README.md b/README.md index ebc82c2..2276751 100644 --- a/README.md +++ b/README.md @@ -242,9 +242,10 @@ subscription and don't want to manage a second API key. Adding a provider is one new package under `internal/provider//`. -> The `remote pr` subcommand (below) requires an **API provider** — -> `claude-cli` / `gemini-cli` / `codex-cli` are incompatible because they -> don't produce structured findings. +> The `remote pr` subcommand (below) requires an **API provider** when it +> posts to GitHub — `claude-cli` / `gemini-cli` / `codex-cli` don't +> produce structured findings to anchor comments. (In `--no-post` mode it +> only prints locally, so CLI providers work there.) ## Reviewing pull requests from the terminal @@ -258,6 +259,8 @@ auth. commitbrief remote pr 42 # PR #42 in the current repo commitbrief remote pr CommitBrief/web#10 # cross-repo (owner/repo#N) commitbrief remote pr 42 --request-changes-on=high +commitbrief remote pr 42 --no-post # review locally, write nothing to GitHub +commitbrief remote pr 42 --no-post --output review.md # …or --json / --cli gemini, etc. ``` `--request-changes-on=` (default `critical`) @@ -266,6 +269,15 @@ sets the severity at or above which the verdict becomes request-changes; provider. `--fail-on` is ignored here — the GitHub verdict replaces the exit-code gate. +**`--no-post`** turns `remote pr` into a read-only review: it fetches the +PR diff via `gh` and renders the result to your terminal exactly like a +local review, **writing nothing to GitHub** (no comments, no verdict). +Because the output is local, the flags posting mode rejects all apply — +`--json`, `--markdown`, `--output`, `--copy`, `--compact`, `--cli`, and +`--fail-on` — and there's no self-PR restriction (you can review your own +PR). Results are cached like any local review. Handy for triaging a PR, +piping findings into another tool, or reviewing with a CLI provider. + Each comment is anchored to the diff side its line lives on — `RIGHT` (new file) for added/context lines, `LEFT` (old file) for removed ones. A finding whose line falls outside the diff (or whose POST is rejected) diff --git a/internal/cli/remote_pr.go b/internal/cli/remote_pr.go index 37168b5..71020c1 100644 --- a/internal/cli/remote_pr.go +++ b/internal/cli/remote_pr.go @@ -3,9 +3,13 @@ package cli import ( + "bufio" "context" + "crypto/sha256" + "encoding/hex" "errors" "fmt" + "os" "sort" "strings" "time" @@ -27,6 +31,7 @@ import ( type remotePRFlags struct { requestChangesOn string repo string + noPost bool } func newRemotePRCmd() *cobra.Command { @@ -46,6 +51,8 @@ func newRemotePRCmd() *cobra.Command { cmd.Flags().StringVar(&f.requestChangesOn, "request-changes-on", "critical", "severity at/above which the verdict becomes request-changes (critical|high|medium|low)") cmd.Flags().StringVar(&f.repo, "repo", "", "target repository owner/repo (overrides git context)") + cmd.Flags().BoolVar(&f.noPost, "no-post", false, + "review the PR diff and print locally (no GitHub writes); enables --json/--markdown/--output/--copy/--cli like a local review") return cmd } @@ -70,6 +77,14 @@ func parseRequestChangesOn(raw string) (render.Severity, error) { } func runRemotePR(cmd *cobra.Command, prID string, f remotePRFlags, runner remote.Runner) error { + // --no-post (ADR-0016 §Update): use the PR diff purely as a review + // source and render to the terminal like a local review — no GitHub + // writes (no comments, no verdict), so the local-render and CLI flags + // apply. Diverges enough from the posting flow to warrant its own path. + if f.noPost { + return runRemotePRLocal(cmd, prID, f, runner) + } + ctx := cmd.Context() app, err := resolveContext(false) @@ -182,6 +197,278 @@ func runRemotePR(cmd *cobra.Command, prID string, f remotePRFlags, runner remote return nil } +// runRemotePRLocal is the `remote pr --no-post` path: fetch the PR +// diff via gh and run it through the same review+render pipeline a local +// review uses, printing to the terminal instead of posting to GitHub. +// No GitHub writes happen, so the local-render flags (--json/--markdown/ +// --output/--copy/--compact), CLI providers (--cli), and --fail-on all +// apply, and there is no self-PR restriction. It mirrors runReview's +// core (cache → cost preflight → call → render); the differences are the +// diff source (gh, not git) and that the secret scanner WARNS rather than +// aborts (you can't fix another author's PR locally, and aborting a +// read-only review is unhelpful), matching the posting path's posture. +func runRemotePRLocal(cmd *cobra.Command, prID string, f remotePRFlags, runner remote.Runner) error { + ctx := cmd.Context() + app, err := resolveContext(false) + if err != nil { + return err + } + cat := app.Catalog + + if _, _, err := parseMinSeverity(global.minSeverity); err != nil { + return err + } + if f.repo == "" && app.RepoRoot == "" { + return errors.New(cat.T("remote.repo_required")) + } + // --request-changes-on only drives a GitHub verdict, which --no-post + // never submits. Note it only when the user explicitly set it. + if cmd.Flags().Changed("request-changes-on") { + infof("%s", cat.T("remote.no_post_request_changes_ignored")) + } + if err := remote.EnsureGH(); err != nil { + return errors.New(cat.T("remote.gh_missing")) + } + + prov, err := provider.New(app.Config.Provider, app.Config.Providers[app.Config.Provider]) + if err != nil { + return err + } + _, plainText := prov.(provider.PlainTextEmitter) + // --with-context grounds a CLI review in the LOCAL working tree, which + // need not match the PR's branch — combining it with a remote diff is + // misleading, so it is not wired here. Note it if the user set it. + if global.withContext { + infof("%s", cat.T("remote.no_post_context_ignored")) + } + model := app.Config.Providers[app.Config.Provider].Model + if model == "" { + model = prov.DefaultModel() + } + + loaded, err := rules.Load(app.RepoRoot) + if err != nil { + return err + } + if loaded.Source == rules.SourceDefault { + infof("%s", cat.T("rules.using_default")) + } + outputLoaded, err := rules.LoadOutput(app.RepoRoot, userHome()) + if err != nil { + return err + } + if outputLoaded.Source == rules.SourceDefault { + infof("%s", cat.T("rules.output.using_default")) + } else if verr := render.ValidateOutputTemplate(outputLoaded.Content); verr != nil { + return errors.New(cat.T("output.template.invalid", outputLoaded.Path, verr.Error())) + } + + if !global.quiet { + _, _ = fmt.Fprintln(cmd.ErrOrStderr(), render.HeaderLine(render.Meta{Provider: prov.Name(), Model: model})) + } + prog := ui.NewProgress(cmd.ErrOrStderr(), ui.ParseColorMode(global.color), global.quiet) + defer prog.Close() + + prog.Start(cat.T("remote.fetching_pr", prID)) + rawDiff, err := remote.FetchDiff(ctx, runner, prID, f.repo) + if err != nil { + prog.Fail(err) + return err + } + parsed, err := diff.Parse(git.Diff{Content: rawDiff, Origin: git.OriginDiff}) + if err != nil { + prog.Fail(err) + return err + } + parsed = diff.Filter(parsed, buildMatcher(app.RepoRoot)) + parsed = diff.KeepPaths(parsed, global.files, global.dirs) + if parsed.Empty() { + prog.Finish() + prog.Close() + infof("%s", cat.T("review.no_changes")) + return nil + } + prog.Info(render.StatusLine(render.Meta{ + Files: parsed.FileCount(), + LinesAdded: parsed.AddedLines(), + LinesRemoved: parsed.DeletedLines(), + RulesLoaded: loaded.Source != rules.SourceDefault, + })) + diffText := parsed.String() + numbered := parsed.NumberedString() + + // Secret scanner warns (does not abort) — the diff is a remote PR's. + if app.Config.Guard.SecretScan && !global.allowSecrets { + if hits := guard.ScanForSecrets(diffText); len(hits) > 0 { + prog.Info(cat.T("remote.secret_warn", len(hits))) + } + } + + var p prompt.Prompt + if plainText { + p = prompt.BuildPlainText(loaded, app.Lang, numbered, false) + } else { + p = prompt.Build(loaded, app.Lang, numbered) + } + + cacheKey := cache.Compute(cache.ComputeArgs{ + Diff: diffText, + SystemPrompt: p.System, + Provider: prov.Name(), + Model: model, + Lang: app.Lang.Code, + }) + cacheStore, cerr := openCache(app.RepoRoot, app.Config.Cache) + if cerr != nil { + infof("%s", cat.T("review.cache_disabled", cerr)) + } + + if !global.noCache && cacheStore != nil { + if entry, hit := cacheStore.Get(cacheKey); hit { + prog.Finish() + prog.Clear() + usage := provider.Usage{ + InputTokens: entry.Result.Tokens.Input, + OutputTokens: entry.Result.Tokens.Output, + CachedInputTokens: entry.Result.Tokens.Cached, + } + meta := render.Meta{ + Provider: prov.Name(), + Model: model, + Lang: app.Lang.Code, + Cached: true, + Timestamp: entry.CreatedAt, + Usage: usage, + Cost: resolvePricing(app.Config, prov, model).Cost(usage), + Files: parsed.FileCount(), + LinesAdded: parsed.AddedLines(), + LinesRemoved: parsed.DeletedLines(), + RulesLoaded: loaded.Source != rules.SourceDefault, + } + var findings []render.Finding + switch entry.Result.Format { + case cache.FormatJSON, "": + findings, _ = render.ParseFindings(entry.Result.Content) + } + if entry.Result.Format == cache.FormatPlainText { + if err := emitPlainText(cmd, entry.Result.Content); err != nil { + return err + } + } else if err := renderResult(cmd, entry.Result.Content, outputLoaded.Content, findings, meta); err != nil { + return err + } + handleCopyFlag(cmd, app, findings) + return applyFailOn(cmd, app, findings) + } + } + prog.Finish() + + // Cost preflight, same as a local review (no-op for zero-priced CLI + // providers). --no-cost-check bypasses; --yes deliberately does not. + if !global.noCostCheck { + estUsage := provider.Usage{ + InputTokens: p.EstimatedTokens(), + OutputTokens: estimateOutputTokens(p.EstimatedTokens()), + } + estCost := resolvePricing(app.Config, prov, model).Cost(estUsage) + prog.Pause() + if abort := handleCostPreflight(cmd, app, estCost, bufio.NewReader(os.Stdin)); abort { + return errors.New(cat.T("cost.aborted_user")) + } + prog.Resume() + } + + prog.Start(cat.T("remote.reviewing")) + start := time.Now() + req := provider.Request{ + Model: model, + SystemPrompt: p.System, + UserPrompt: p.User, + Lang: app.Lang.Code, + } + var ( + content string + usage provider.Usage + format string + ) + if plainText { + resp, callErr := prov.Review(ctx, req) + if callErr != nil { + prog.Fail(callErr) + return fmt.Errorf("provider %s: %w", prov.Name(), callErr) + } + content, usage, format = resp.Content, resp.Usage, cache.FormatPlainText + } else { + var callErr error + content, usage, format, callErr = tryStructuredReview(ctx, prov, req, func() { + prog.Soft() + prog.Start(cat.T("progress.retrying")) + }) + if callErr != nil { + prog.Fail(callErr) + return fmt.Errorf("provider %s: %w", prov.Name(), callErr) + } + } + prog.Finish() + prog.Clear() + latency := time.Since(start) + + var findings []render.Finding + switch format { + case cache.FormatJSON: + findings, _ = render.ParseFindings(content) + case cache.FormatMarkdownFallback: + _, _ = fmt.Fprintln(cmd.ErrOrStderr(), cat.T("review.degraded")) + } + + meta := render.Meta{ + Provider: prov.Name(), + Model: model, + Lang: app.Lang.Code, + Usage: usage, + Cost: resolvePricing(app.Config, prov, model).Cost(usage), + Latency: latency, + Timestamp: time.Now().UTC(), + Files: parsed.FileCount(), + LinesAdded: parsed.AddedLines(), + LinesRemoved: parsed.DeletedLines(), + RulesLoaded: loaded.Source != rules.SourceDefault, + } + + if !global.noCache && cacheStore != nil { + diffSum := sha256.Sum256([]byte(diffText)) + promptSum := sha256.Sum256([]byte(p.System)) + _ = cacheStore.Put(cacheKey, cache.Entry{ + Key: cache.KeyMeta{ + DiffHash: "sha256:" + hex.EncodeToString(diffSum[:]), + SystemPromptHash: "sha256:" + hex.EncodeToString(promptSum[:]), + Provider: prov.Name(), + Model: model, + Lang: app.Lang.Code, + }, + Result: cache.Result{ + Content: content, + Format: format, + Tokens: cache.Tokens{ + Input: usage.InputTokens, + Output: usage.OutputTokens, + Cached: usage.CachedInputTokens, + }, + }, + }) + } + + if format == cache.FormatPlainText { + if err := emitPlainText(cmd, content); err != nil { + return err + } + } else if err := renderResult(cmd, content, outputLoaded.Content, findings, meta); err != nil { + return err + } + handleCopyFlag(cmd, app, findings) + return applyFailOn(cmd, app, findings) +} + // prReviewResult bundles everything one PR review produces that the // caller needs downstream: the findings, the anchor index to place them, // and the provider usage + latency that feed the terminal footer line. diff --git a/internal/cli/remote_pr_test.go b/internal/cli/remote_pr_test.go index 756454e..a642e82 100644 --- a/internal/cli/remote_pr_test.go +++ b/internal/cli/remote_pr_test.go @@ -327,6 +327,45 @@ func TestRemotePRPrintsHeaderAndFooter(t *testing.T) { } } +func TestRemotePRNoPostRendersLocallyWithoutWrites(t *testing.T) { + e := newCLIEnv(t) + stubGHOnPath(t) + // --no-post fetches the diff but performs NO GitHub writes. whoami / + // prMeta are deliberately left set to the SAME login to prove the + // self-PR block does not apply in this read-only mode (it never calls + // whoami / pr view at all). + r := &fakeGH{whoami: "tester", prMeta: prMetaJSON("tester", "stable"), diff: sampleDiff} + + oldWd, _ := os.Getwd() + _ = os.Chdir(e.repoRoot) + t.Cleanup(func() { _ = os.Chdir(oldWd) }) + + var out, errBuf bytes.Buffer + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + cmd.SetOut(&out) + cmd.SetErr(&errBuf) + + if err := runRemotePR(cmd, "42", remotePRFlags{noPost: true, requestChangesOn: "critical"}, r); err != nil { + t.Fatalf("--no-post: %v", err) + } + + // The diff is still fetched... + if r.callCount("diff") == 0 { + t.Errorf("--no-post should fetch the PR diff; calls=%v", r.calls) + } + // ...but nothing is written to GitHub. + for _, write := range []string{"/comments", "review", "--approve", "--request-changes", "--comment"} { + if n := r.callCount(write); n != 0 { + t.Errorf("--no-post must not write to GitHub (%q seen %d times); calls=%v", write, n, r.calls) + } + } + // And the review is rendered locally to stdout. + if out.Len() == 0 { + t.Errorf("--no-post should render the review to stdout; stderr=%s", errBuf.String()) + } +} + func TestRemotePRAbortsOnDoubleRace(t *testing.T) { e := newCLIEnv(t) stubGHOnPath(t) diff --git a/internal/i18n/messages.en.yml b/internal/i18n/messages.en.yml index 3a74cc8..018cf3d 100644 --- a/internal/i18n/messages.en.yml +++ b/internal/i18n/messages.en.yml @@ -142,8 +142,10 @@ remote.request_changes_on_invalid: "remote pr: invalid --request-changes-on=%q ( remote.self_pr_blocked: "remote pr: you are the author of this PR; GitHub does not allow self-review." remote.too_volatile: "remote pr: the PR head changed twice during review; aborted — try again once it settles." remote.repo_required: "remote pr: not inside a git repository and --repo was not given; pass --repo owner/repo." -remote.output_flags: "remote pr: --json / --markdown / --output / --copy / --compact don't apply (output goes to GitHub)." +remote.output_flags: "remote pr: --json / --markdown / --output / --copy / --compact don't apply when posting to GitHub. Add --no-post to review the PR locally and print instead." remote.fail_on_ignored: "ℹ --fail-on is ignored in remote-pr mode; the GitHub verdict is controlled by --request-changes-on." +remote.no_post_request_changes_ignored: "ℹ --request-changes-on is ignored with --no-post (no GitHub verdict is submitted)." +remote.no_post_context_ignored: "ℹ --with-context is ignored with --no-post (it would read the local tree, not the PR's branch)." remote.degraded: "remote pr: the provider did not return structured findings; no review submitted." remote.secret_warn: "⚠ secret scanner flagged %d line(s) in the PR diff; continuing (remote-pr mode warns, does not abort)." remote.fetching_pr: "Fetching PR %s…" diff --git a/internal/i18n/messages.tr.yml b/internal/i18n/messages.tr.yml index f2d9f56..dfb0755 100644 --- a/internal/i18n/messages.tr.yml +++ b/internal/i18n/messages.tr.yml @@ -140,8 +140,10 @@ remote.request_changes_on_invalid: "remote pr: geçersiz --request-changes-on=%q remote.self_pr_blocked: "remote pr: bu PR'ın yazarı sizsiniz; GitHub kendi PR'ınızı incelemenize izin vermez." remote.too_volatile: "remote pr: review sırasında PR head'i iki kez değişti; iptal edildi — sakinleşince tekrar deneyin." remote.repo_required: "remote pr: bir git deposu içinde değilsiniz ve --repo verilmedi; --repo owner/repo geçin." -remote.output_flags: "remote pr: --json / --markdown / --output / --copy / --compact burada geçerli değil (çıktı GitHub'a gider)." +remote.output_flags: "remote pr: GitHub'a gönderirken --json / --markdown / --output / --copy / --compact geçerli değil. PR'ı lokal review edip basmak için --no-post ekleyin." remote.fail_on_ignored: "ℹ remote-pr modunda --fail-on yok sayılır; GitHub verdict'i --request-changes-on ile belirlenir." +remote.no_post_request_changes_ignored: "ℹ --no-post ile --request-changes-on yok sayılır (GitHub verdict'i gönderilmez)." +remote.no_post_context_ignored: "ℹ --no-post ile --with-context yok sayılır (PR'ın branch'ini değil lokal ağacı okurdu)." remote.degraded: "remote pr: provider yapılandırılmış bulgu döndürmedi; review gönderilmedi." remote.secret_warn: "⚠ secret tarayıcı PR diff'inde %d satır işaretledi; devam ediliyor (remote-pr modu uyarır, durdurmaz)." remote.fetching_pr: "PR %s getiriliyor…" diff --git a/man/commitbrief-remote-pr.1 b/man/commitbrief-remote-pr.1 index 23fc34a..1130b5a 100644 --- a/man/commitbrief-remote-pr.1 +++ b/man/commitbrief-remote-pr.1 @@ -20,6 +20,10 @@ or a full URL. See ADR-0016. \fB-h\fP, \fB--help\fP[=false] help for pr +.PP +\fB--no-post\fP[=false] + review the PR diff and print locally (no GitHub writes); enables --json/--markdown/--output/--copy/--cli like a local review + .PP \fB--repo\fP="" target repository owner/repo (overrides git context)