From 14b63012fadce375329b9f0e46e86f22ad8e7684 Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 17 Jun 2026 09:00:17 -0400 Subject: [PATCH 1/2] feat(respond): conversational replies on review-comment threads Add a `cr respond ` command that closes the conversational loop on cr's own open review-comment threads. It lists the PR's inline review threads, selects the ones cr authored (its own findings, matched by posting identity plus a codereview marker) that have a newer unaddressed human reply, and asks a small-tier LLM classifier whether to acknowledge and resolve, reply only, or skip. Replies post in-thread via the existing ReplyToThread plumbing and the thread is resolved via ResolveThread when the finding is addressed. The engine lives in a new internal/threadreply package modeled on internal/approvaloverride: a strict structured-output schema, a deterministic prompt builder, and a pure thread-selection step that keeps repeated runs idempotent without persisted per-thread state. The command supports --dry-run/--no-post, --json, and --no-resolve-threads, and respects the profile resolve_threads policy. This is the engine + CLI-trigger slice of the feature. The menu-bar watcher trigger (polling for new thread replies and invoking cr) is a separate tool and is intentionally out of scope. Closes #326 --- README.md | 41 +++ cmd/cr/main.go | 3 +- cmd/cr/main_test.go | 1 + internal/cmd/respondcmd/respondcmd.go | 385 +++++++++++++++++++++ internal/cmd/respondcmd/respondcmd_test.go | 259 ++++++++++++++ internal/threadreply/responder.go | 120 +++++++ internal/threadreply/threadreply.go | 228 ++++++++++++ internal/threadreply/threadreply_test.go | 249 +++++++++++++ internal/view/respond.go | 90 +++++ 9 files changed, 1375 insertions(+), 1 deletion(-) create mode 100644 internal/cmd/respondcmd/respondcmd.go create mode 100644 internal/cmd/respondcmd/respondcmd_test.go create mode 100644 internal/threadreply/responder.go create mode 100644 internal/threadreply/threadreply.go create mode 100644 internal/threadreply/threadreply_test.go create mode 100644 internal/view/respond.go diff --git a/README.md b/README.md index 61b766e..bd8e6af 100644 --- a/README.md +++ b/README.md @@ -998,6 +998,47 @@ Live text output includes run ID, gate status, decision, outcome, PR, artifacts, and post counts. JSON output includes `run`, `status`, `decision`, `message`, `outbox`, `artifacts`, and `fail_on_triggered`. +### `cr respond` + +```text +cr respond [flags] +``` + +Responds to human replies on `cr`'s own open review-comment threads, closing the +conversational loop on a finding without forcing a full re-review. The PR host +must match the active profile's `git.host`. + +`cr respond` lists the pull request's inline review threads and selects the ones +that meet all of: + +- the first comment is a `cr` finding (it matches the posting identity and + carries a codereview marker), +- the thread is still open (unresolved), and +- the latest comment is a human reply that `cr` has not already answered. + +For each selected thread, a small-tier LLM classifier reads the original finding +plus the full thread and chooses one of `acknowledge_and_resolve` (post a brief +acknowledgement and resolve the thread because the finding is addressed or +conceded), `reply_only` (post a contextual answer and keep the thread open), or +`skip` (take no action). Threads with no new human reply never reach the +classifier, so repeated runs are idempotent without persisted per-thread state. + +| Flag | Semantics | +|------|-----------| +| `--dry-run` | Plan thread replies and print the plan without posting. | +| `--no-post` | Alias for `--dry-run`. | +| `--no-resolve-threads` | Post replies but never resolve threads. Also implied by profile `resolve_threads: never`. | +| `--json` | Emit JSON. | + +Text output includes the PR, mode, considered/replied/resolved/skipped counts, +and a per-thread breakdown. JSON output includes `pr_url`, `dry_run`, the same +counts, and a `threads` array with each thread's `decision`, `reply`, and +`resolved` state. + +The classifier uses the profile's `small` model tier (falling back to `medium` +when `small` is unset, with a warning); a profile with neither configured is +rejected. + ### `cr benchmark` ```text diff --git a/cmd/cr/main.go b/cmd/cr/main.go index d28d8a5..a3a4c1e 100644 --- a/cmd/cr/main.go +++ b/cmd/cr/main.go @@ -18,6 +18,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/cmd/datacmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/exitcode" "github.com/open-cli-collective/codereview-cli/internal/cmd/mecmd" + "github.com/open-cli-collective/codereview-cli/internal/cmd/respondcmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/reviewcmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/root" "github.com/open-cli-collective/codereview-cli/internal/cmd/sessionscmd" @@ -47,6 +48,6 @@ func buildRootCommand(stdin io.Reader, stdout, stderr io.Writer) (*cobra.Command Stdout: stdout, Stderr: stderr, }) - root.RegisterAll(cmd, opts, configcmd.Register, credentialcmd.Register, mecmd.Register, agentscmd.Register, reviewcmd.Register, sessionscmd.Register, datacmd.Register, benchmarkcmd.Register) + root.RegisterAll(cmd, opts, configcmd.Register, credentialcmd.Register, mecmd.Register, agentscmd.Register, reviewcmd.Register, respondcmd.Register, sessionscmd.Register, datacmd.Register, benchmarkcmd.Register) return cmd, opts } diff --git a/cmd/cr/main_test.go b/cmd/cr/main_test.go index b9f8284..7ebb8f5 100644 --- a/cmd/cr/main_test.go +++ b/cmd/cr/main_test.go @@ -34,6 +34,7 @@ func TestRun(t *testing.T) { {name: "me command wired", args: []string{"me", "--help"}, wantCode: 0, wantStdout: "Resolve and cache", wantStdoutSubstring: true}, {name: "agents command wired", args: []string{"agents", "--help"}, wantCode: 0, wantStdout: "Inspect trusted review agents", wantStdoutSubstring: true}, {name: "review command wired", args: []string{"review", "--help"}, wantCode: 0, wantStdout: "Run an automated pull-request review", wantStdoutSubstring: true}, + {name: "respond command wired", args: []string{"respond", "--help"}, wantCode: 0, wantStdout: "Respond to human replies", wantStdoutSubstring: true}, {name: "sessions command wired", args: []string{"sessions", "--help"}, wantCode: 0, wantStdout: "Manage named LLM sessions", wantStdoutSubstring: true}, {name: "data command wired", args: []string{"data", "--help"}, wantCode: 0, wantStdout: "Manage local review data", wantStdoutSubstring: true}, {name: "benchmark command wired", args: []string{"benchmark", "--help"}, wantCode: 0, wantStdout: "Validate, inspect, and run benchmark suites", wantStdoutSubstring: true}, diff --git a/internal/cmd/respondcmd/respondcmd.go b/internal/cmd/respondcmd/respondcmd.go new file mode 100644 index 0000000..3b13d4c --- /dev/null +++ b/internal/cmd/respondcmd/respondcmd.go @@ -0,0 +1,385 @@ +// Package respondcmd wires the `cr respond` command surface. +package respondcmd + +import ( + "context" + "errors" + "fmt" + "io" + "sync" + + "github.com/open-cli-collective/cli-common/credstore" + "github.com/spf13/cobra" + + "github.com/open-cli-collective/codereview-cli/internal/cmd/cmderr" + "github.com/open-cli-collective/codereview-cli/internal/cmd/exitcode" + "github.com/open-cli-collective/codereview-cli/internal/cmd/root" + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/credentials" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + "github.com/open-cli-collective/codereview-cli/internal/llm" + "github.com/open-cli-collective/codereview-cli/internal/prref" + "github.com/open-cli-collective/codereview-cli/internal/threadreply" + "github.com/open-cli-collective/codereview-cli/internal/view" +) + +const respondLong = `Respond to human replies on cr's own open review-comment threads. + +cr lists the pull request's inline review threads, finds the ones it authored +(its own findings) that have a newer human reply, and asks a small-tier LLM +whether to acknowledge, answer, or skip. It posts a contextual in-thread reply +and resolves the thread when the finding has been addressed. + +Use --dry-run (or --no-post) to plan replies without posting. Use +--no-resolve-threads to keep threads open even when the reply indicates the +finding is addressed.` + +// Runtime contains the per-command dependencies for `cr respond`. +type Runtime struct { + Provider gitprovider.GitProvider + PostingIdentity gitprovider.Identity + Classifier threadreply.Classifier + Cleanup func() +} + +// RuntimeFactory builds the concrete runtime used by `cr respond`. +type RuntimeFactory func(cmd *cobra.Command, opts *root.Options, cfg config.File, profile config.Profile, ref gitprovider.PRRef) (Runtime, error) + +type commandFlags struct { + dryRun bool + noPost bool + jsonOutput bool + noResolveThreads bool +} + +// Register attaches the respond command to rootCmd. +func Register(rootCmd *cobra.Command, opts *root.Options) { + RegisterWithFactory(rootCmd, opts, newRuntime) +} + +// RegisterWithFactory attaches the respond command with an injected runtime factory. +func RegisterWithFactory(rootCmd *cobra.Command, opts *root.Options, factory RuntimeFactory) { + var flags commandFlags + cmd := &cobra.Command{ + Use: "respond ", + Short: "Reply to human replies on cr's own review-comment threads", + Long: respondLong, + Args: func(_ *cobra.Command, args []string) error { + if len(args) != 1 { + return exitcode.Usage(fmt.Errorf("respond requires one PR URL")) + } + return nil + }, + RunE: func(cmd *cobra.Command, args []string) error { + return runRespond(cmd.Context(), cmd, opts, factory, flags, args[0]) + }, + } + cmd.Flags().BoolVar(&flags.dryRun, "dry-run", false, "Plan thread replies without posting") + cmd.Flags().BoolVar(&flags.noPost, "no-post", false, "Alias for --dry-run") + cmd.Flags().BoolVar(&flags.jsonOutput, "json", false, "Emit JSON") + cmd.Flags().BoolVar(&flags.noResolveThreads, "no-resolve-threads", false, "Reply but never resolve threads") + rootCmd.AddCommand(cmd) +} + +func runRespond(ctx context.Context, cmd *cobra.Command, opts *root.Options, factory RuntimeFactory, flags commandFlags, prArg string) error { + if flags.noPost { + flags.dryRun = true + } + path, err := configPath(opts) + if err != nil { + return exitcode.AuthConfig(err) + } + cfg, err := config.Load(path) + if err != nil { + return cmderr.Config(err) + } + ref, err := prref.ParseGitHubPullURL(prArg) + if err != nil { + return exitcode.Usage(err) + } + _, profile, err := config.ResolveProfileForRepository(cfg, opts.Profile, root.ProfileFlagChanged(cmd), config.RepositoryTarget{ + Host: ref.Host, + Namespace: ref.Owner, + Repo: ref.Repo, + }) + if err != nil { + return cmderr.Config(err) + } + if !prref.SameHost(ref.Host, profile.Git.Host) { + return exitcode.Usage(fmt.Errorf("PR host %q must match configured git host %q", ref.Host, profile.Git.Host)) + } + noResolve := flags.noResolveThreads || profile.ReviewPolicy.ResolveThreads == config.ResolveThreadsNever + + runtime, err := factory(cmd, opts, cfg, profile, ref) + if err != nil { + return err + } + if runtime.Cleanup != nil { + defer runtime.Cleanup() + } + if runtime.Provider == nil { + return fmt.Errorf("respond: runtime provider is required") + } + if runtime.Classifier == nil { + return fmt.Errorf("respond: runtime classifier is required") + } + + result, err := respond(ctx, runtime, ref, prArg, flags.dryRun, noResolve) + if err != nil { + return mapRunError(err) + } + if flags.jsonOutput { + return view.RenderRespondJSON(opts.Stdout, result) + } + return view.RenderRespondText(opts.Stdout, result) +} + +func respond(ctx context.Context, runtime Runtime, ref gitprovider.PRRef, prURL string, dryRun, noResolve bool) (view.RespondResult, error) { + pr, err := runtime.Provider.GetPR(ctx, ref) + if err != nil { + return view.RespondResult{}, err + } + threads, err := runtime.Provider.ListInlineThreads(ctx, ref) + if err != nil { + return view.RespondResult{}, err + } + candidates := threadreply.SelectCandidates(pr, runtime.PostingIdentity, threads) + result := view.RespondResult{ + PRURL: prURL, + DryRun: dryRun, + Considered: len(candidates), + Threads: make([]view.RespondThread, 0, len(candidates)), + } + for _, candidate := range candidates { + decision, err := runtime.Classifier.ClassifyThreadReply(ctx, candidate.Request) + if err != nil { + return view.RespondResult{}, err + } + rendered, err := applyDecision(ctx, runtime, ref, candidate, decision, dryRun, noResolve) + if err != nil { + return view.RespondResult{}, err + } + switch { + case rendered.Resolved: + result.Resolved++ + result.Replied++ + case rendered.Decision == string(threadreply.DecisionSkip): + result.Skipped++ + default: + result.Replied++ + } + result.Threads = append(result.Threads, rendered) + } + return result, nil +} + +func applyDecision(ctx context.Context, runtime Runtime, ref gitprovider.PRRef, candidate threadreply.Candidate, decision threadreply.Result, dryRun, noResolve bool) (view.RespondThread, error) { + thread := view.RespondThread{ + ThreadID: string(candidate.Thread.ID), + Path: candidate.Thread.Path, + Line: candidate.Thread.Line, + Decision: string(decision.Decision), + Reply: decision.Reply, + } + if decision.Decision == threadreply.DecisionSkip { + return thread, nil + } + willResolve := decision.Decision == threadreply.DecisionAcknowledgeAndResolve && !noResolve + if dryRun { + thread.Resolved = willResolve + return thread, nil + } + if _, err := runtime.Provider.ReplyToThread(ctx, ref, candidate.Thread.ID, decision.Reply); err != nil { + return view.RespondThread{}, err + } + thread.Posted = true + if willResolve { + if err := runtime.Provider.ResolveThread(ctx, ref, candidate.Thread.ID); err != nil { + return view.RespondThread{}, err + } + thread.Resolved = true + } + return thread, nil +} + +func configPath(opts *root.Options) (string, error) { + if opts != nil && opts.ConfigPath != "" { + return opts.ConfigPath, nil + } + return config.Path() +} + +func mapRunError(err error) error { + switch { + case errors.Is(err, config.ErrInvalid), + errors.Is(err, config.ErrNotConfigured), + errors.Is(err, config.ErrProfileNotFound), + errors.Is(err, config.ErrSecretsProfileNotFound), + errors.Is(err, config.ErrUnsupported): + return cmderr.Config(err) + case errors.Is(err, gitprovider.ErrAuth), + errors.Is(err, gitprovider.ErrPermission), + errors.Is(err, gitprovider.ErrRetryable), + errors.Is(err, gitprovider.ErrNotFound), + errors.Is(err, gitprovider.ErrConflict), + errors.Is(err, gitprovider.ErrStaleSHA): + return cmderr.Provider(err) + case errors.Is(err, credentials.ErrInvalidBackendSelection), + errors.Is(err, credentials.ErrWrongService), + errors.Is(err, credstore.ErrRefEmpty), + errors.Is(err, credstore.ErrRefSegmentCount), + errors.Is(err, credstore.ErrRefInvalidChar), + errors.Is(err, credstore.ErrKeyNotAllowed), + errors.Is(err, credstore.ErrExists), + errors.Is(err, credstore.ErrFilePassphraseRequired), + errors.Is(err, credstore.ErrSecretServiceFailClosed), + errors.Is(err, credstore.ErrStoreClosed), + errors.Is(err, credstore.ErrBackendNotImplemented): + return cmderr.Credential(err) + default: + return err + } +} + +var ( + newGitProvider = func(git config.GitConfig, store githubprovider.TokenStore, opts githubprovider.Options) (gitprovider.GitProvider, gitprovider.Credential, error) { + return githubprovider.NewFromGitConfig(git, store, opts) + } + newAdapterForRuntime = newAdapter +) + +func newRuntime(cmd *cobra.Command, opts *root.Options, cfg config.File, profile config.Profile, ref gitprovider.PRRef) (Runtime, error) { + resolvedSecretsProfile, err := credentials.ResolveSecretsProfileForProfile(cfg, profile) + if err != nil { + return Runtime{}, mapRunError(err) + } + store, err := credentials.OpenResolvedStore(opts.Backend, cmderr.BackendFlagChanged(cmd), cfg, resolvedSecretsProfile) + if err != nil { + return Runtime{}, mapRunError(err) + } + cleanup := func() { _ = store.Close() } + providerGit := gitConfigForReviewerAuth(profile) + provider, credential, err := newGitProvider(providerGit, store, gitProviderOptions(ref)) + if err != nil { + cleanup() + return Runtime{}, mapRunError(err) + } + postingIdentity, err := provider.WhoAmI(cmd.Context(), credential) + if err != nil { + cleanup() + return Runtime{}, mapRunError(err) + } + classifier, err := buildClassifier(profile, store, opts.Stderr) + if err != nil { + cleanup() + return Runtime{}, err + } + return Runtime{ + Provider: provider, + PostingIdentity: postingIdentity, + Classifier: classifier, + Cleanup: cleanup, + }, nil +} + +func buildClassifier(profile config.Profile, store *credstore.Store, warnings io.Writer) (threadreply.Classifier, error) { + return &lazyClassifier{profile: profile, store: store, warnings: warnings}, nil +} + +// lazyClassifier defers LLM adapter construction until the first classification +// so commands with no candidate threads never touch the LLM provider. +type lazyClassifier struct { + mu sync.Mutex + profile config.Profile + store *credstore.Store + warnings io.Writer + resolved threadreply.Classifier + err error + loaded bool +} + +func (c *lazyClassifier) ClassifyThreadReply(ctx context.Context, req threadreply.Request) (threadreply.Result, error) { + classifier, err := c.get() + if err != nil { + return threadreply.Result{}, err + } + return classifier.ClassifyThreadReply(ctx, req) +} + +func (c *lazyClassifier) get() (threadreply.Classifier, error) { + c.mu.Lock() + defer c.mu.Unlock() + if c.loaded { + return c.resolved, c.err + } + c.loaded = true + adapter, err := newAdapterForRuntime(c.profile.LLM, c.store) + if err != nil { + c.err = fmt.Errorf("respond: initialize LLM adapter: %w", err) + return nil, c.err + } + model, effort, ok := resolveClassifierModel(c.profile, c.warnings) + if !ok { + c.err = fmt.Errorf("%w: respond requires a small or medium model tier", config.ErrUnsupported) + return nil, c.err + } + c.resolved = threadreply.NewLLMClassifier(adapter, model, effort) + return c.resolved, nil +} + +func resolveClassifierModel(profile config.Profile, warnings io.Writer) (string, string, bool) { + if resolved, ok := config.ResolveModelTier(profile.LLM, config.ModelTierSmall); ok { + return resolved.Model, "low", true + } + if resolved, ok := config.ResolveModelTier(profile.LLM, config.ModelTierMedium); ok { + if warnings != nil { + _, _ = fmt.Fprintf(warnings, "warning: respond classifier small model is not configured; falling back to medium tier model %s\n", resolved.Model) + } + return resolved.Model, "low", true + } + return "", "", false +} + +func gitConfigForReviewerAuth(profile config.Profile) config.GitConfig { + if profile.ReviewerCredentials == nil { + return profile.Git + } + return config.GitConfig{ + Host: profile.Git.Host, + AuthMode: profile.ReviewerCredentials.AuthMode, + CredentialRef: profile.ReviewerCredentials.CredentialRef, + IdentityCache: profile.ReviewerCredentials.IdentityCache, + } +} + +func gitProviderOptions(ref gitprovider.PRRef) githubprovider.Options { + if ref.Owner == "" || ref.Repo == "" { + return githubprovider.Options{} + } + return githubprovider.Options{InstallationLookup: &githubprovider.InstallationLookup{ + Owner: ref.Owner, + Repo: ref.Repo, + }} +} + +func newAdapter(llmConfig config.LLMConfig, store *credstore.Store) (llm.Adapter, error) { + switch llmConfig.Adapter { + case config.LLMAdapterClaudeCLI: + return llm.NewClaudeCLIAdapter(llm.SubprocessOptions{}), nil + case config.LLMAdapterCodexCLI: + if llmConfig.Provider != config.LLMProviderOpenAI || llmConfig.Auth != config.LLMAuthSubscription { + return nil, fmt.Errorf("%w: codex_cli requires provider openai with subscription auth", config.ErrUnsupported) + } + return llm.NewCodexCLIAdapter(llm.SubprocessOptions{AllowBestEffortNoTools: true}), nil + case config.LLMAdapterPiRPC: + if llmConfig.Provider != config.LLMProviderPi || llmConfig.Auth != config.LLMAuthSubscription { + return nil, fmt.Errorf("%w: pi_rpc requires provider pi with subscription auth", config.ErrUnsupported) + } + return llm.NewPiRPCAdapter(llm.PiRPCOptions{}), nil + case config.LLMAdapterAnthropicAPI, config.LLMAdapterOpenAIAPI: + return llm.NewAPIAdapterFromConfig(llmConfig, store, llm.APIOptions{}) + default: + return nil, fmt.Errorf("%w: unsupported LLM adapter %q", config.ErrUnsupported, llmConfig.Adapter) + } +} diff --git a/internal/cmd/respondcmd/respondcmd_test.go b/internal/cmd/respondcmd/respondcmd_test.go new file mode 100644 index 0000000..dae8931 --- /dev/null +++ b/internal/cmd/respondcmd/respondcmd_test.go @@ -0,0 +1,259 @@ +package respondcmd + +import ( + "bytes" + "context" + "encoding/json" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/open-cli-collective/codereview-cli/internal/cmd/root" + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/threadreply" + "github.com/open-cli-collective/codereview-cli/internal/view" +) + +const prURL = "https://github.com/open-cli-collective/codereview-cli/pull/29" + +var testRef = gitprovider.PRRef{Host: "github.com", Owner: "open-cli-collective", Repo: "codereview-cli", Number: 29} + +// stubClassifier returns a canned decision per request in order. +type stubClassifier struct { + results []threadreply.Result + calls int +} + +func (s *stubClassifier) ClassifyThreadReply(_ context.Context, _ threadreply.Request) (threadreply.Result, error) { + result := s.results[s.calls] + s.calls++ + return result, nil +} + +func markerComment(login, body string) gitprovider.ThreadComment { + return gitprovider.ThreadComment{ + Author: gitprovider.Identity{Login: login}, + Body: "\n" + body, + } +} + +func plainComment(login, body string) gitprovider.ThreadComment { + return gitprovider.ThreadComment{Author: gitprovider.Identity{Login: login}, Body: body} +} + +func newFake(t *testing.T) *gitprovider.Fake { + t.Helper() + fake := &gitprovider.Fake{} + fake.SetIdentity(gitprovider.Identity{Login: "review-bot"}) + if err := fake.SetPR(testRef, gitprovider.PR{Ref: testRef, Title: "CR-20", URL: prURL}); err != nil { + t.Fatalf("SetPR: %v", err) + } + if err := fake.SetInlineThreads(testRef, []gitprovider.InlineThread{ + { + ID: "thread-addressed", + Resolved: false, + Path: "main.go", + Line: 10, + Comments: []gitprovider.ThreadComment{ + markerComment("review-bot", "Nil check missing."), + plainComment("author", "Fixed in latest commit."), + }, + }, + { + ID: "thread-human-only", + Resolved: false, + Comments: []gitprovider.ThreadComment{ + plainComment("author", "Unrelated human discussion."), + plainComment("reviewer-2", "agreed"), + }, + }, + }); err != nil { + t.Fatalf("SetInlineThreads: %v", err) + } + return fake +} + +func newTestCommand(t *testing.T, factory RuntimeFactory) (*cobra.Command, *bytes.Buffer) { + t.Helper() + path := filepath.Join(t.TempDir(), "config.yml") + if err := config.Save(path, testConfig()); err != nil { + t.Fatalf("Save config: %v", err) + } + var out bytes.Buffer + cmd, opts := root.NewCommandWithOptions(&root.Options{ + ConfigPath: path, + Stdin: strings.NewReader(""), + Stdout: &out, + Stderr: &out, + }) + RegisterWithFactory(cmd, opts, factory) + return cmd, &out +} + +func testConfig() config.File { + return config.File{ + DefaultProfile: "home", + Keyring: config.KeyringConfig{Backend: "memory"}, + Profiles: map[string]config.Profile{ + "home": { + Git: config.GitConfig{ + Host: "github.com", + AuthMode: config.GitAuthModePAT, + CredentialRef: "codereview/home", + }, + LLM: config.LLMConfig{ + Provider: config.LLMProviderAnthropic, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterClaudeCLI, + }, + }, + }, + } +} + +func factoryFor(fake *gitprovider.Fake, classifier threadreply.Classifier) RuntimeFactory { + return func(_ *cobra.Command, _ *root.Options, _ config.File, _ config.Profile, _ gitprovider.PRRef) (Runtime, error) { + return Runtime{ + Provider: fake, + PostingIdentity: gitprovider.Identity{Login: "review-bot"}, + Classifier: classifier, + }, nil + } +} + +func TestRespondLivePostsReplyAndResolves(t *testing.T) { + fake := newFake(t) + classifier := &stubClassifier{results: []threadreply.Result{ + {Decision: threadreply.DecisionAcknowledgeAndResolve, Reply: "Thanks, resolving."}, + }} + cmd, out := newTestCommand(t, factoryFor(fake, classifier)) + + if err := root.Execute(cmd, []string{"respond", prURL}); err != nil { + t.Fatalf("Execute: %v", err) + } + replies := fake.RecordedThreadReplies(testRef) + if len(replies) != 1 || replies[0].ThreadID != "thread-addressed" || replies[0].Body != "Thanks, resolving." { + t.Fatalf("replies = %#v, want one reply on thread-addressed", replies) + } + resolved := fake.RecordedResolvedThreads(testRef) + if len(resolved) != 1 || resolved[0] != "thread-addressed" { + t.Fatalf("resolved = %#v, want thread-addressed resolved", resolved) + } + if classifier.calls != 1 { + t.Fatalf("classifier calls = %d, want 1 (only cr-authored thread with reply)", classifier.calls) + } + if got := out.String(); !strings.Contains(got, "Replied: 1") || !strings.Contains(got, "Resolved: 1") { + t.Fatalf("stdout = %q, want replied/resolved counts", got) + } +} + +func TestRespondDryRunDoesNotPost(t *testing.T) { + fake := newFake(t) + classifier := &stubClassifier{results: []threadreply.Result{ + {Decision: threadreply.DecisionAcknowledgeAndResolve, Reply: "Thanks."}, + }} + cmd, out := newTestCommand(t, factoryFor(fake, classifier)) + + if err := root.Execute(cmd, []string{"respond", "--dry-run", prURL}); err != nil { + t.Fatalf("Execute: %v", err) + } + if replies := fake.RecordedThreadReplies(testRef); len(replies) != 0 { + t.Fatalf("replies = %#v, want none in dry-run", replies) + } + if resolved := fake.RecordedResolvedThreads(testRef); len(resolved) != 0 { + t.Fatalf("resolved = %#v, want none in dry-run", resolved) + } + if got := out.String(); !strings.Contains(got, "Mode: dry-run") || !strings.Contains(got, "Resolved: 1") { + t.Fatalf("stdout = %q, want dry-run plan with resolved count", got) + } +} + +func TestRespondNoResolveThreadsRepliesWithoutResolving(t *testing.T) { + fake := newFake(t) + classifier := &stubClassifier{results: []threadreply.Result{ + {Decision: threadreply.DecisionAcknowledgeAndResolve, Reply: "Thanks."}, + }} + cmd, _ := newTestCommand(t, factoryFor(fake, classifier)) + + if err := root.Execute(cmd, []string{"respond", "--no-resolve-threads", prURL}); err != nil { + t.Fatalf("Execute: %v", err) + } + if replies := fake.RecordedThreadReplies(testRef); len(replies) != 1 { + t.Fatalf("replies = %#v, want one reply", replies) + } + if resolved := fake.RecordedResolvedThreads(testRef); len(resolved) != 0 { + t.Fatalf("resolved = %#v, want no resolutions with --no-resolve-threads", resolved) + } +} + +func TestRespondSkipDecisionDoesNothing(t *testing.T) { + fake := newFake(t) + classifier := &stubClassifier{results: []threadreply.Result{{Decision: threadreply.DecisionSkip}}} + cmd, out := newTestCommand(t, factoryFor(fake, classifier)) + + if err := root.Execute(cmd, []string{"respond", prURL}); err != nil { + t.Fatalf("Execute: %v", err) + } + if replies := fake.RecordedThreadReplies(testRef); len(replies) != 0 { + t.Fatalf("replies = %#v, want none for skip", replies) + } + if got := out.String(); !strings.Contains(got, "Skipped: 1") { + t.Fatalf("stdout = %q, want skipped count", got) + } +} + +func TestRespondJSONOutput(t *testing.T) { + fake := newFake(t) + classifier := &stubClassifier{results: []threadreply.Result{ + {Decision: threadreply.DecisionReplyOnly, Reply: "Here is why."}, + }} + cmd, out := newTestCommand(t, factoryFor(fake, classifier)) + + if err := root.Execute(cmd, []string{"respond", "--json", prURL}); err != nil { + t.Fatalf("Execute: %v", err) + } + var got view.RespondResult + if err := json.Unmarshal(out.Bytes(), &got); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if got.Considered != 1 || got.Replied != 1 || got.Resolved != 0 { + t.Fatalf("result counts = %#v, want considered=1 replied=1 resolved=0", got) + } + if len(got.Threads) != 1 || got.Threads[0].Decision != string(threadreply.DecisionReplyOnly) { + t.Fatalf("threads = %#v, want one reply_only thread", got.Threads) + } +} + +func TestRespondNoCandidatesSkipsClassifier(t *testing.T) { + fake := &gitprovider.Fake{} + fake.SetIdentity(gitprovider.Identity{Login: "review-bot"}) + if err := fake.SetPR(testRef, gitprovider.PR{Ref: testRef, Title: "t", URL: prURL}); err != nil { + t.Fatalf("SetPR: %v", err) + } + if err := fake.SetInlineThreads(testRef, nil); err != nil { + t.Fatalf("SetInlineThreads: %v", err) + } + classifier := &stubClassifier{} + cmd, out := newTestCommand(t, factoryFor(fake, classifier)) + + if err := root.Execute(cmd, []string{"respond", prURL}); err != nil { + t.Fatalf("Execute: %v", err) + } + if classifier.calls != 0 { + t.Fatalf("classifier calls = %d, want 0", classifier.calls) + } + if got := out.String(); !strings.Contains(got, "Threads considered: 0") { + t.Fatalf("stdout = %q, want zero considered", got) + } +} + +func TestRespondRejectsMissingPR(t *testing.T) { + fake := newFake(t) + cmd, _ := newTestCommand(t, factoryFor(fake, &stubClassifier{})) + if err := root.Execute(cmd, []string{"respond"}); err == nil { + t.Fatal("Execute error = nil, want usage error for missing PR argument") + } +} diff --git a/internal/threadreply/responder.go b/internal/threadreply/responder.go new file mode 100644 index 0000000..2c56641 --- /dev/null +++ b/internal/threadreply/responder.go @@ -0,0 +1,120 @@ +package threadreply + +import ( + "strings" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/marker" +) + +// Candidate is one cr-authored thread that has an unaddressed human reply. +type Candidate struct { + Thread gitprovider.InlineThread + Request Request +} + +// SelectCandidates returns the threads that cr should consider replying to: +// unresolved threads whose first comment cr authored (its own finding) and +// whose latest comment is a human reply that cr has not answered yet. +// +// Threads whose latest comment is cr's own are skipped: cr already had the last +// word, so there is no new reply to respond to. This keeps repeated runs +// idempotent without persisted per-thread state. +func SelectCandidates(pr gitprovider.PR, postingIdentity gitprovider.Identity, threads []gitprovider.InlineThread) []Candidate { + login := strings.TrimSpace(postingIdentity.Login) + var candidates []Candidate + for _, thread := range threads { + if thread.Resolved || len(thread.Comments) == 0 { + continue + } + first := thread.Comments[0] + if !authoredByCR(first, login) { + continue + } + last := thread.Comments[len(thread.Comments)-1] + if commentLogin(last) == login && login != "" { + // cr posted the latest comment; nothing new to respond to. + continue + } + candidates = append(candidates, Candidate{ + Thread: thread, + Request: buildRequest(pr, postingIdentity, login, thread), + }) + } + return candidates +} + +func buildRequest(pr gitprovider.PR, postingIdentity gitprovider.Identity, login string, thread gitprovider.InlineThread) Request { + comments := make([]Comment, 0, len(thread.Comments)) + original := "" + for i, c := range thread.Comments { + fromCR := authoredByCR(c, login) + body := marker.SanitizeModelContent(c.Body) + if i == 0 { + original = stripMarkers(c.Body) + } + comments = append(comments, Comment{ + Author: displayLogin(c), + Body: stripMarkers(body), + FromCR: fromCR, + CreatedAt: c.CreatedAt, + }) + } + return Request{ + PR: pr, + PostingIdentity: postingIdentity, + Path: thread.Path, + Line: thread.Line, + OriginalFinding: original, + Comments: comments, + } +} + +// authoredByCR reports whether comment c was authored by cr. A comment counts as +// cr's when it matches the posting login and carries a codereview action marker, +// so a human posting from the same account is not mistaken for a finding. +func authoredByCR(c gitprovider.ThreadComment, login string) bool { + if login == "" { + return false + } + if commentLogin(c) != login { + return false + } + return len(marker.FindActions(c.Body)) > 0 +} + +func commentLogin(c gitprovider.ThreadComment) string { + return strings.TrimSpace(c.Author.Login) +} + +func displayLogin(c gitprovider.ThreadComment) string { + if login := commentLogin(c); login != "" { + return login + } + return "unknown" +} + +// stripMarkers removes codereview HTML-comment markers so the model sees only +// human-meaningful text. +func stripMarkers(body string) string { + const openTag = "" + var b strings.Builder + rest := body + for { + start := strings.Index(rest, openTag) + if start == -1 { + b.WriteString(rest) + break + } + b.WriteString(rest[:start]) + after := rest[start+len(openTag):] + end := strings.Index(after, closeTag) + if end == -1 { + // Unterminated marker: drop the remainder of the opening token. + break + } + rest = after[end+len(closeTag):] + } + return strings.TrimSpace(b.String()) +} diff --git a/internal/threadreply/threadreply.go b/internal/threadreply/threadreply.go new file mode 100644 index 0000000..14249e4 --- /dev/null +++ b/internal/threadreply/threadreply.go @@ -0,0 +1,228 @@ +// Package threadreply classifies how cr should respond to human replies on its +// own open review-comment threads and whether the thread is now addressed. +package threadreply + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/llm" +) + +const schemaVersion = 1 + +// Decision is the responder's chosen action for one review-comment thread. +type Decision string + +// Decision values. +const ( + // DecisionSkip leaves the thread untouched. + DecisionSkip Decision = "skip" + // DecisionReplyOnly posts a contextual reply but keeps the thread open + // (for example to answer a question or push back on a reply). + DecisionReplyOnly Decision = "reply_only" + // DecisionAcknowledgeAndResolve posts an acknowledgement reply and resolves + // the thread because the finding has been addressed or conceded. + DecisionAcknowledgeAndResolve Decision = "acknowledge_and_resolve" +) + +// Valid reports whether d is one of the known decisions. +func (d Decision) Valid() bool { + switch d { + case DecisionSkip, DecisionReplyOnly, DecisionAcknowledgeAndResolve: + return true + default: + return false + } +} + +// ParseDecision parses a responder decision. +func ParseDecision(value string) (Decision, error) { + decision := Decision(strings.ToLower(strings.TrimSpace(value))) + if !decision.Valid() { + return "", fmt.Errorf("invalid thread reply decision %q", value) + } + return decision, nil +} + +// Comment is one comment in a review-comment thread. +type Comment struct { + Author string + Body string + FromCR bool + CreatedAt time.Time +} + +// Request is the classifier input for one review-comment thread. +type Request struct { + PR gitprovider.PR + PostingIdentity gitprovider.Identity + Path string + Line int + OriginalFinding string + Comments []Comment + LogPath string +} + +// Result is the responder's decision for one thread. +type Result struct { + Decision Decision + Reply string +} + +// Classifier decides how to respond to replies on a cr-authored thread. +type Classifier interface { + ClassifyThreadReply(context.Context, Request) (Result, error) +} + +// LLMClassifier implements Classifier with structured output. +type LLMClassifier struct { + adapter llm.Adapter + model string + effort string +} + +// NewLLMClassifier builds an LLM-backed thread-reply classifier. +func NewLLMClassifier(adapter llm.Adapter, model, effort string) *LLMClassifier { + return &LLMClassifier{ + adapter: adapter, + model: strings.TrimSpace(model), + effort: strings.TrimSpace(effort), + } +} + +// ClassifyThreadReply decides how cr should respond to a thread's latest reply. +func (c *LLMClassifier) ClassifyThreadReply(ctx context.Context, req Request) (Result, error) { + if c == nil || c.adapter == nil { + return Result{}, fmt.Errorf("threadreply: adapter is required") + } + if strings.TrimSpace(c.model) == "" { + return Result{}, fmt.Errorf("threadreply: model is required") + } + if err := ensureLogDir(req.LogPath); err != nil { + return Result{}, err + } + value, _, err := llm.RunStructured(ctx, c.adapter, llm.Request{ + Model: c.model, + Effort: c.effort, + Prompt: BuildPrompt(req), + LogPath: req.LogPath, + }, DecodeResponse) + if err != nil { + return Result{}, err + } + decision, err := ParseDecision(value.Decision) + if err != nil { + return Result{}, err + } + reply := strings.TrimSpace(value.Reply) + if decision != DecisionSkip && reply == "" { + return Result{}, fmt.Errorf("threadreply: decision %q requires a non-empty reply", decision) + } + if decision == DecisionSkip { + reply = "" + } + return Result{Decision: decision, Reply: reply}, nil +} + +// Response is the strict classifier schema. +type Response struct { + SchemaVersion int `json:"schema_version"` + Decision string `json:"decision"` + Reply string `json:"reply"` +} + +// DecodeResponse validates a classifier structured-output payload. +func DecodeResponse(data []byte) (Response, error) { + var raw struct { + SchemaVersion *int `json:"schema_version"` + Decision *string `json:"decision"` + Reply *string `json:"reply"` + } + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&raw); err != nil { + return Response{}, err + } + var extra any + if err := decoder.Decode(&extra); !errors.Is(err, io.EOF) { + return Response{}, fmt.Errorf("threadreply: trailing JSON tokens") + } + if raw.SchemaVersion == nil { + return Response{}, fmt.Errorf("threadreply: schema_version is required") + } + if *raw.SchemaVersion != schemaVersion { + return Response{}, fmt.Errorf("threadreply: schema_version = %d, want %d", *raw.SchemaVersion, schemaVersion) + } + if raw.Decision == nil { + return Response{}, fmt.Errorf("threadreply: decision is required") + } + if _, err := ParseDecision(*raw.Decision); err != nil { + return Response{}, err + } + reply := "" + if raw.Reply != nil { + reply = *raw.Reply + } + return Response{ + SchemaVersion: *raw.SchemaVersion, + Decision: *raw.Decision, + Reply: reply, + }, nil +} + +// BuildPrompt returns the deterministic prompt for one thread-reply classification. +func BuildPrompt(req Request) string { + var b strings.Builder + b.WriteString("You are a code-review assistant deciding how to respond to a human reply on a review-comment thread that you (the reviewer) opened.\n") + b.WriteString("Read the original finding and the full thread, then choose exactly one decision:\n") + b.WriteString("- \"acknowledge_and_resolve\": the reply shows the finding is fixed, already handled, or you concede the point; post a brief acknowledgement and the thread will be resolved.\n") + b.WriteString("- \"reply_only\": the reply asks a question or pushes back and still needs a contextual answer; post a reply and keep the thread open.\n") + b.WriteString("- \"skip\": no useful response is possible or the latest comment is your own; take no action.\n\n") + b.WriteString("Return JSON only with schema_version=1, decision, and reply. For skip, reply must be empty. For the other decisions, reply must be a concise, courteous in-thread message.\n\n") + b.WriteString("Pull request:\n") + fmt.Fprintf(&b, "- title: %q\n", req.PR.Title) + fmt.Fprintf(&b, "- url: %q\n", req.PR.URL) + fmt.Fprintf(&b, "- reviewer_login: %q\n", req.PostingIdentity.Login) + if strings.TrimSpace(req.Path) != "" { + fmt.Fprintf(&b, "- file: %q\n", req.Path) + } + if req.Line > 0 { + fmt.Fprintf(&b, "- line: %d\n", req.Line) + } + b.WriteString("\nOriginal finding:\n") + b.WriteString(strings.TrimSpace(req.OriginalFinding)) + b.WriteString("\n\nThread (oldest first):\n") + for i, comment := range req.Comments { + speaker := comment.Author + if comment.FromCR { + speaker = comment.Author + " (reviewer)" + } + fmt.Fprintf(&b, "\nComment %d by %s:\n", i+1, speaker) + b.WriteString(strings.TrimSpace(comment.Body)) + b.WriteByte('\n') + } + b.WriteString("\nReturn exactly: {\"schema_version\":1,\"decision\":\"...\",\"reply\":\"...\"}\n") + return b.String() +} + +func ensureLogDir(path string) error { + path = strings.TrimSpace(path) + if path == "" { + return nil + } + dir := filepath.Dir(path) + if dir == "." || dir == "" { + return nil + } + return os.MkdirAll(dir, 0o750) +} diff --git a/internal/threadreply/threadreply_test.go b/internal/threadreply/threadreply_test.go new file mode 100644 index 0000000..acb5e7c --- /dev/null +++ b/internal/threadreply/threadreply_test.go @@ -0,0 +1,249 @@ +package threadreply + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/llm" +) + +func TestDecodeResponseStrictSchema(t *testing.T) { + valid, err := DecodeResponse([]byte(`{"schema_version":1,"decision":"reply_only","reply":"thanks"}`)) + if err != nil { + t.Fatalf("DecodeResponse valid: %v", err) + } + if valid.Decision != "reply_only" || valid.Reply != "thanks" { + t.Fatalf("DecodeResponse = %#v, want reply_only/thanks", valid) + } + + for _, raw := range []string{ + `{"schema_version":2,"decision":"skip","reply":""}`, + `{"schema_version":1,"decision":"skip","reply":"","why":"because"}`, + `{"schema_version":1,"reply":"hi"}`, + `{"schema_version":1,"decision":"bogus","reply":"hi"}`, + `{"decision":"skip","reply":""}`, + } { + if _, err := DecodeResponse([]byte(raw)); err == nil { + t.Fatalf("DecodeResponse(%s) error = nil, want error", raw) + } + } +} + +func TestParseDecision(t *testing.T) { + for _, value := range []string{"skip", "reply_only", "ACKNOWLEDGE_AND_RESOLVE", " reply_only "} { + if _, err := ParseDecision(value); err != nil { + t.Fatalf("ParseDecision(%q): %v", value, err) + } + } + if _, err := ParseDecision("resolve"); err == nil { + t.Fatal("ParseDecision(resolve) error = nil, want error") + } +} + +func TestBuildPromptIncludesThreadContext(t *testing.T) { + req := Request{ + PR: gitprovider.PR{ + Title: "Add retries", + URL: "https://example.test/pr/7", + Author: gitprovider.Identity{Login: "author"}, + }, + PostingIdentity: gitprovider.Identity{Login: "review-bot"}, + Path: "internal/api/client.go", + Line: 42, + OriginalFinding: "This nil check is missing.", + Comments: []Comment{ + {Author: "review-bot", Body: "This nil check is missing.", FromCR: true}, + {Author: "author", Body: "Fixed in latest commit."}, + }, + } + prompt := BuildPrompt(req) + for _, want := range []string{ + "Return JSON only", + "Add retries", + "internal/api/client.go", + "line: 42", + "This nil check is missing.", + "Comment 1 by review-bot (reviewer)", + "Comment 2 by author", + "Fixed in latest commit.", + } { + if !strings.Contains(prompt, want) { + t.Fatalf("prompt missing %q:\n%s", want, prompt) + } + } +} + +func TestLLMClassifierReturnsDecisionAndReply(t *testing.T) { + adapter := &llm.FakeAdapter{} + adapter.Queue(llm.FakeResult{ + SessionID: "session-1", + Response: llm.Response{StructuredOutput: []byte(`{"schema_version":1,"decision":"acknowledge_and_resolve","reply":"Thanks, resolving."}`)}, + }) + classifier := NewLLMClassifier(adapter, "small-model", "low") + + result, err := classifier.ClassifyThreadReply(context.Background(), Request{ + PR: gitprovider.PR{Title: "t", URL: "u"}, + Comments: []Comment{{Author: "author", Body: "done"}}, + }) + if err != nil { + t.Fatalf("ClassifyThreadReply: %v", err) + } + if result.Decision != DecisionAcknowledgeAndResolve || result.Reply != "Thanks, resolving." { + t.Fatalf("result = %#v, want acknowledge_and_resolve with reply", result) + } + requests := adapter.Requests() + if len(requests) != 1 { + t.Fatalf("adapter requests = %d, want 1", len(requests)) + } + if requests[0].Model != "small-model" || requests[0].Effort != "low" { + t.Fatalf("request = %#v, want configured model/effort", requests[0]) + } +} + +func TestLLMClassifierSkipClearsReply(t *testing.T) { + adapter := &llm.FakeAdapter{} + adapter.Queue(llm.FakeResult{ + Response: llm.Response{StructuredOutput: []byte(`{"schema_version":1,"decision":"skip","reply":""}`)}, + }) + classifier := NewLLMClassifier(adapter, "small-model", "low") + result, err := classifier.ClassifyThreadReply(context.Background(), Request{Comments: []Comment{{Author: "a", Body: "b"}}}) + if err != nil { + t.Fatalf("ClassifyThreadReply: %v", err) + } + if result.Decision != DecisionSkip || result.Reply != "" { + t.Fatalf("result = %#v, want skip with empty reply", result) + } +} + +func TestLLMClassifierRejectsEmptyReplyForAction(t *testing.T) { + adapter := &llm.FakeAdapter{} + adapter.Queue(llm.FakeResult{ + Response: llm.Response{StructuredOutput: []byte(`{"schema_version":1,"decision":"reply_only","reply":" "}`)}, + }) + adapter.Queue(llm.FakeResult{ + Response: llm.Response{StructuredOutput: []byte(`{"schema_version":1,"decision":"reply_only","reply":" "}`)}, + }) + classifier := NewLLMClassifier(adapter, "small-model", "low") + if _, err := classifier.ClassifyThreadReply(context.Background(), Request{Comments: []Comment{{Author: "a", Body: "b"}}}); err == nil { + t.Fatal("ClassifyThreadReply error = nil, want error for empty reply") + } +} + +func markerComment(login, body string) gitprovider.ThreadComment { + return gitprovider.ThreadComment{ + Author: gitprovider.Identity{Login: login}, + Body: "\n" + body, + CreatedAt: time.Now().UTC(), + } +} + +func plainComment(login, body string) gitprovider.ThreadComment { + return gitprovider.ThreadComment{ + Author: gitprovider.Identity{Login: login}, + Body: body, + CreatedAt: time.Now().UTC(), + } +} + +func TestSelectCandidatesPicksReplyOnCRThread(t *testing.T) { + pr := gitprovider.PR{Title: "t", URL: "u"} + posting := gitprovider.Identity{Login: "review-bot"} + threads := []gitprovider.InlineThread{ + { + ID: "open-with-reply", + Resolved: false, + Path: "a.go", + Line: 10, + Comments: []gitprovider.ThreadComment{ + markerComment("review-bot", "Nil check missing."), + plainComment("author", "Fixed it."), + }, + }, + { + ID: "already-resolved", + Resolved: true, + Comments: []gitprovider.ThreadComment{ + markerComment("review-bot", "Finding."), + plainComment("author", "done"), + }, + }, + { + ID: "no-human-reply", + Resolved: false, + Comments: []gitprovider.ThreadComment{ + markerComment("review-bot", "Finding only."), + }, + }, + { + ID: "cr-had-last-word", + Resolved: false, + Comments: []gitprovider.ThreadComment{ + markerComment("review-bot", "Finding."), + plainComment("author", "why?"), + plainComment("review-bot", "Because X."), + }, + }, + { + ID: "human-thread", + Resolved: false, + Comments: []gitprovider.ThreadComment{ + plainComment("author", "Question from a human."), + plainComment("other", "answer"), + }, + }, + } + + candidates := SelectCandidates(pr, posting, threads) + if len(candidates) != 1 { + t.Fatalf("candidates = %d, want 1 (%+v)", len(candidates), candidateIDs(candidates)) + } + got := candidates[0] + if got.Thread.ID != "open-with-reply" { + t.Fatalf("candidate thread = %q, want open-with-reply", got.Thread.ID) + } + if got.Request.Path != "a.go" || got.Request.Line != 10 { + t.Fatalf("request anchor = %q:%d, want a.go:10", got.Request.Path, got.Request.Line) + } + if got.Request.OriginalFinding != "Nil check missing." { + t.Fatalf("original finding = %q, want marker stripped", got.Request.OriginalFinding) + } + if len(got.Request.Comments) != 2 || !got.Request.Comments[0].FromCR || got.Request.Comments[1].FromCR { + t.Fatalf("comments = %+v, want first from cr and second from human", got.Request.Comments) + } +} + +func candidateIDs(candidates []Candidate) []string { + ids := make([]string, 0, len(candidates)) + for _, c := range candidates { + ids = append(ids, string(c.Thread.ID)) + } + return ids +} + +func TestSelectCandidatesIgnoresSameLoginWithoutMarker(t *testing.T) { + pr := gitprovider.PR{Title: "t", URL: "u"} + posting := gitprovider.Identity{Login: "shared-account"} + threads := []gitprovider.InlineThread{ + { + ID: "human-from-shared-account", + Resolved: false, + Comments: []gitprovider.ThreadComment{ + plainComment("shared-account", "A human comment, no marker."), + plainComment("other", "reply"), + }, + }, + } + if got := SelectCandidates(pr, posting, threads); len(got) != 0 { + t.Fatalf("candidates = %d, want 0 for unmarked thread", len(got)) + } +} + +func TestStripMarkersRemovesAllMarkers(t *testing.T) { + body := "\nReal text here." + if got := stripMarkers(body); got != "Real text here." { + t.Fatalf("stripMarkers = %q, want %q", got, "Real text here.") + } +} diff --git a/internal/view/respond.go b/internal/view/respond.go new file mode 100644 index 0000000..f8527e8 --- /dev/null +++ b/internal/view/respond.go @@ -0,0 +1,90 @@ +package view + +import ( + "encoding/json" + "fmt" + "io" +) + +// RespondResult is the presentation model for `cr respond`. +type RespondResult struct { + PRURL string `json:"pr_url"` + DryRun bool `json:"dry_run"` + Considered int `json:"considered"` + Replied int `json:"replied"` + Resolved int `json:"resolved"` + Skipped int `json:"skipped"` + Threads []RespondThread `json:"threads"` +} + +// RespondThread describes one thread the responder evaluated. +type RespondThread struct { + ThreadID string `json:"thread_id"` + Path string `json:"path,omitempty"` + Line int `json:"line,omitempty"` + Decision string `json:"decision"` + Reply string `json:"reply,omitempty"` + Resolved bool `json:"resolved"` + Posted bool `json:"posted"` +} + +// RenderRespondText writes a stable human-readable responder summary. +func RenderRespondText(w io.Writer, result RespondResult) error { + mode := "live" + if result.DryRun { + mode = "dry-run" + } + if err := writeKV(w, "PR", result.PRURL); err != nil { + return err + } + if err := writeKV(w, "Mode", mode); err != nil { + return err + } + if err := writeKV(w, "Threads considered", fmt.Sprint(result.Considered)); err != nil { + return err + } + if err := writeKV(w, "Replied", fmt.Sprint(result.Replied)); err != nil { + return err + } + if err := writeKV(w, "Resolved", fmt.Sprint(result.Resolved)); err != nil { + return err + } + if err := writeKV(w, "Skipped", fmt.Sprint(result.Skipped)); err != nil { + return err + } + for _, thread := range result.Threads { + if _, err := fmt.Fprintln(w); err != nil { + return err + } + anchor := thread.Path + if anchor != "" && thread.Line > 0 { + anchor = fmt.Sprintf("%s:%d", thread.Path, thread.Line) + } + if err := writeKV(w, "Thread", thread.ThreadID); err != nil { + return err + } + if err := writeOptionalKV(w, "Location", anchor); err != nil { + return err + } + if err := writeKV(w, "Decision", thread.Decision); err != nil { + return err + } + if err := writeKV(w, "Resolved", fmt.Sprint(thread.Resolved)); err != nil { + return err + } + if err := writeOptionalKV(w, "Reply", thread.Reply); err != nil { + return err + } + } + return nil +} + +// RenderRespondJSON writes the responder summary as indented JSON. +func RenderRespondJSON(w io.Writer, result RespondResult) error { + if result.Threads == nil { + result.Threads = []RespondThread{} + } + encoder := json.NewEncoder(w) + encoder.SetIndent("", " ") + return encoder.Encode(result) +} From b92a5a9ea955bc3243eec95cf139ba448b7da2e1 Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 17 Jun 2026 09:44:21 -0400 Subject: [PATCH 2/2] refactor(respond): address review feedback on conversational replies - threadreply: strip cr markers before sanitizing in buildRequest so the first comment's Body and OriginalFinding derive from one consistent representation and no live/escaped marker leaks to the model - threadreply: drop dead duplicate adapter.Queue in the empty-reply test - respond: hide --no-post (keep it as a working legacy alias for --dry-run) and stop advertising both flags side-by-side in the long help - llm: extract shared NewAdapterFromConfig and have reviewcmd/respondcmd delegate to it, removing the duplicated adapter dispatch --- internal/cmd/respondcmd/respondcmd.go | 29 +++++------------- internal/cmd/reviewcmd/reviewcmd.go | 19 +----------- internal/llm/factory.go | 35 ++++++++++++++++++++++ internal/threadreply/responder.go | 11 +++++-- internal/threadreply/threadreply_test.go | 38 ++++++++++++++++++++++-- 5 files changed, 86 insertions(+), 46 deletions(-) create mode 100644 internal/llm/factory.go diff --git a/internal/cmd/respondcmd/respondcmd.go b/internal/cmd/respondcmd/respondcmd.go index 3b13d4c..8eec0a5 100644 --- a/internal/cmd/respondcmd/respondcmd.go +++ b/internal/cmd/respondcmd/respondcmd.go @@ -31,9 +31,8 @@ cr lists the pull request's inline review threads, finds the ones it authored whether to acknowledge, answer, or skip. It posts a contextual in-thread reply and resolves the thread when the finding has been addressed. -Use --dry-run (or --no-post) to plan replies without posting. Use ---no-resolve-threads to keep threads open even when the reply indicates the -finding is addressed.` +Use --dry-run to plan replies without posting. Use --no-resolve-threads to keep +threads open even when the reply indicates the finding is addressed.` // Runtime contains the per-command dependencies for `cr respond`. type Runtime struct { @@ -76,7 +75,10 @@ func RegisterWithFactory(rootCmd *cobra.Command, opts *root.Options, factory Run }, } cmd.Flags().BoolVar(&flags.dryRun, "dry-run", false, "Plan thread replies without posting") - cmd.Flags().BoolVar(&flags.noPost, "no-post", false, "Alias for --dry-run") + cmd.Flags().BoolVar(&flags.noPost, "no-post", false, "Deprecated alias for --dry-run") + // --no-post is a legacy alias for --dry-run; keep it working but hide it so + // --help advertises a single flag for the plan-only behavior. + _ = cmd.Flags().MarkHidden("no-post") cmd.Flags().BoolVar(&flags.jsonOutput, "json", false, "Emit JSON") cmd.Flags().BoolVar(&flags.noResolveThreads, "no-resolve-threads", false, "Reply but never resolve threads") rootCmd.AddCommand(cmd) @@ -364,22 +366,5 @@ func gitProviderOptions(ref gitprovider.PRRef) githubprovider.Options { } func newAdapter(llmConfig config.LLMConfig, store *credstore.Store) (llm.Adapter, error) { - switch llmConfig.Adapter { - case config.LLMAdapterClaudeCLI: - return llm.NewClaudeCLIAdapter(llm.SubprocessOptions{}), nil - case config.LLMAdapterCodexCLI: - if llmConfig.Provider != config.LLMProviderOpenAI || llmConfig.Auth != config.LLMAuthSubscription { - return nil, fmt.Errorf("%w: codex_cli requires provider openai with subscription auth", config.ErrUnsupported) - } - return llm.NewCodexCLIAdapter(llm.SubprocessOptions{AllowBestEffortNoTools: true}), nil - case config.LLMAdapterPiRPC: - if llmConfig.Provider != config.LLMProviderPi || llmConfig.Auth != config.LLMAuthSubscription { - return nil, fmt.Errorf("%w: pi_rpc requires provider pi with subscription auth", config.ErrUnsupported) - } - return llm.NewPiRPCAdapter(llm.PiRPCOptions{}), nil - case config.LLMAdapterAnthropicAPI, config.LLMAdapterOpenAIAPI: - return llm.NewAPIAdapterFromConfig(llmConfig, store, llm.APIOptions{}) - default: - return nil, fmt.Errorf("%w: unsupported LLM adapter %q", config.ErrUnsupported, llmConfig.Adapter) - } + return llm.NewAdapterFromConfig(llmConfig, store) } diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index dc50322..13246b6 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -982,24 +982,7 @@ func resolvePostingIdentity(ctx context.Context, provider gitprovider.GitProvide } func newAdapter(llmConfig config.LLMConfig, store *credstore.Store) (llm.Adapter, error) { - switch llmConfig.Adapter { - case config.LLMAdapterClaudeCLI: - return llm.NewClaudeCLIAdapter(llm.SubprocessOptions{}), nil - case config.LLMAdapterCodexCLI: - if llmConfig.Provider != config.LLMProviderOpenAI || llmConfig.Auth != config.LLMAuthSubscription { - return nil, fmt.Errorf("%w: codex_cli requires provider openai with subscription auth", config.ErrUnsupported) - } - return llm.NewCodexCLIAdapter(llm.SubprocessOptions{AllowBestEffortNoTools: true}), nil - case config.LLMAdapterPiRPC: - if llmConfig.Provider != config.LLMProviderPi || llmConfig.Auth != config.LLMAuthSubscription { - return nil, fmt.Errorf("%w: pi_rpc requires provider pi with subscription auth", config.ErrUnsupported) - } - return llm.NewPiRPCAdapter(llm.PiRPCOptions{}), nil - case config.LLMAdapterAnthropicAPI, config.LLMAdapterOpenAIAPI: - return llm.NewAPIAdapterFromConfig(llmConfig, store, llm.APIOptions{}) - default: - return nil, fmt.Errorf("%w: unsupported LLM adapter %q", config.ErrUnsupported, llmConfig.Adapter) - } + return llm.NewAdapterFromConfig(llmConfig, store) } var _ Runner = reviewRunner{} diff --git a/internal/llm/factory.go b/internal/llm/factory.go new file mode 100644 index 0000000..7f4d071 --- /dev/null +++ b/internal/llm/factory.go @@ -0,0 +1,35 @@ +package llm + +import ( + "fmt" + + "github.com/open-cli-collective/codereview-cli/internal/config" +) + +// NewAdapterFromConfig constructs the LLM adapter selected by an LLM config, +// dispatching across the subprocess (Claude/Codex/Pi) and direct-API +// (Anthropic/OpenAI) adapters. The store is only consulted for API-key adapters +// to resolve the credential; subprocess adapters ignore it. +// +// This is the single source of truth for adapter selection shared by the review +// and respond commands. +func NewAdapterFromConfig(llmConfig config.LLMConfig, store APITokenStore) (Adapter, error) { + switch llmConfig.Adapter { + case config.LLMAdapterClaudeCLI: + return NewClaudeCLIAdapter(SubprocessOptions{}), nil + case config.LLMAdapterCodexCLI: + if llmConfig.Provider != config.LLMProviderOpenAI || llmConfig.Auth != config.LLMAuthSubscription { + return nil, fmt.Errorf("%w: codex_cli requires provider openai with subscription auth", config.ErrUnsupported) + } + return NewCodexCLIAdapter(SubprocessOptions{AllowBestEffortNoTools: true}), nil + case config.LLMAdapterPiRPC: + if llmConfig.Provider != config.LLMProviderPi || llmConfig.Auth != config.LLMAuthSubscription { + return nil, fmt.Errorf("%w: pi_rpc requires provider pi with subscription auth", config.ErrUnsupported) + } + return NewPiRPCAdapter(PiRPCOptions{}), nil + case config.LLMAdapterAnthropicAPI, config.LLMAdapterOpenAIAPI: + return NewAPIAdapterFromConfig(llmConfig, store, APIOptions{}) + default: + return nil, fmt.Errorf("%w: unsupported LLM adapter %q", config.ErrUnsupported, llmConfig.Adapter) + } +} diff --git a/internal/threadreply/responder.go b/internal/threadreply/responder.go index 2c56641..4880e11 100644 --- a/internal/threadreply/responder.go +++ b/internal/threadreply/responder.go @@ -49,13 +49,18 @@ func buildRequest(pr gitprovider.PR, postingIdentity gitprovider.Identity, login original := "" for i, c := range thread.Comments { fromCR := authoredByCR(c, login) - body := marker.SanitizeModelContent(c.Body) + // Strip cr's own markers first, then escape any remaining marker + // openings the comment text may contain, so the model never sees a + // live marker yet a single representation is used everywhere. Deriving + // both Body and OriginalFinding from this one value keeps the first + // comment consistent between the two fields. + body := marker.SanitizeModelContent(stripMarkers(c.Body)) if i == 0 { - original = stripMarkers(c.Body) + original = body } comments = append(comments, Comment{ Author: displayLogin(c), - Body: stripMarkers(body), + Body: body, FromCR: fromCR, CreatedAt: c.CreatedAt, }) diff --git a/internal/threadreply/threadreply_test.go b/internal/threadreply/threadreply_test.go index acb5e7c..c775f51 100644 --- a/internal/threadreply/threadreply_test.go +++ b/internal/threadreply/threadreply_test.go @@ -123,9 +123,6 @@ func TestLLMClassifierRejectsEmptyReplyForAction(t *testing.T) { adapter.Queue(llm.FakeResult{ Response: llm.Response{StructuredOutput: []byte(`{"schema_version":1,"decision":"reply_only","reply":" "}`)}, }) - adapter.Queue(llm.FakeResult{ - Response: llm.Response{StructuredOutput: []byte(`{"schema_version":1,"decision":"reply_only","reply":" "}`)}, - }) classifier := NewLLMClassifier(adapter, "small-model", "low") if _, err := classifier.ClassifyThreadReply(context.Background(), Request{Comments: []Comment{{Author: "a", Body: "b"}}}); err == nil { t.Fatal("ClassifyThreadReply error = nil, want error for empty reply") @@ -215,6 +212,41 @@ func TestSelectCandidatesPicksReplyOnCRThread(t *testing.T) { } } +func TestSelectCandidatesFirstCommentBodyMatchesOriginalFinding(t *testing.T) { + pr := gitprovider.PR{Title: "t", URL: "u"} + posting := gitprovider.Identity{Login: "review-bot"} + threads := []gitprovider.InlineThread{ + { + ID: "open-with-reply", + Resolved: false, + Path: "a.go", + Line: 10, + Comments: []gitprovider.ThreadComment{ + markerComment("review-bot", "Nil check missing."), + plainComment("author", "Fixed it."), + }, + }, + } + + candidates := SelectCandidates(pr, posting, threads) + if len(candidates) != 1 { + t.Fatalf("candidates = %d, want 1", len(candidates)) + } + req := candidates[0].Request + // OriginalFinding and the first comment body are derived from a single + // stripped-then-sanitized representation, so they must be identical and + // must not leak the cr marker (live or HTML-escaped). + if req.OriginalFinding != req.Comments[0].Body { + t.Fatalf("OriginalFinding %q != Comments[0].Body %q", req.OriginalFinding, req.Comments[0].Body) + } + if req.OriginalFinding != "Nil check missing." { + t.Fatalf("OriginalFinding = %q, want marker stripped", req.OriginalFinding) + } + if strings.Contains(req.OriginalFinding, "codereview:") { + t.Fatalf("OriginalFinding = %q, leaks codereview marker", req.OriginalFinding) + } +} + func candidateIDs(candidates []Candidate) []string { ids := make([]string, 0, len(candidates)) for _, c := range candidates {