diff --git a/crates/goose-cli/src/cli.rs b/crates/goose-cli/src/cli.rs index 9691320427..e8ace59f1b 100644 --- a/crates/goose-cli/src/cli.rs +++ b/crates/goose-cli/src/cli.rs @@ -988,6 +988,104 @@ enum Command { bin_name: String, }, + /// Local code review. + /// + /// Discovers `**/.agents/checks/*.md` subagent reviewers and + /// `**/.agents/REVIEW.md` scoped prompt overrides, builds a review + /// request from the working tree (or an explicit diff range), and + /// runs the review through goose. + #[command(about = "Review the current diff using goose")] + Review { + /// Diff range to review (e.g. "main...HEAD"). Defaults to the working + /// tree vs HEAD. + #[arg(value_name = "RANGE")] + range: Option, + + /// Path to a Markdown file with a custom base review prompt. Replaces + /// the embedded default prompt. + #[arg(long = "prompt", value_name = "FILE")] + prompt: Option, + + /// Default model used for the main review agent and for any check + /// that does not declare its own `model:` in frontmatter. + #[arg(long = "model", value_name = "MODEL")] + model: Option, + + /// Provider for the main review agent. + #[arg(long = "provider", value_name = "PROVIDER")] + provider: Option, + + /// Force every discovered check to use this model, regardless of + /// the check's own `model:` field. + #[arg(long = "override-model", value_name = "MODEL")] + override_model: Option, + + /// Default `turn-limit` applied to checks that do not declare their + /// own. + #[arg(long = "turn-limit", value_name = "N")] + turn_limit: Option, + + /// Print the assembled review prompt and discovered checks instead of + /// running the review. + #[arg(long = "dry-run")] + dry_run: bool, + + /// Suppress non-result output from the underlying agent. + #[arg(long, short = 'q')] + quiet: bool, + + /// Disable the Rust-driven parallel orchestrator and fall back to + /// the single-prompt path that asks the main agent to delegate + /// each check via `delegate(... async: true ...)`. The default + /// orchestrator dispatches one `goose run` subprocess per check + /// (capped at 4 concurrent), bounding wall-clock to the slowest + /// single check rather than waiting on the model to issue + /// dispatches. + #[arg(long = "no-orchestrate")] + no_orchestrate: bool, + + /// Additional free-form instructions to prepend to the review + /// (e.g. PR intent, commit-message context, "this is a refactor, + /// flag any behavior change"). Mirrors `amp review --instructions` + /// for drop-in compatibility with existing reviewer wrappers. + #[arg(long = "instructions", short = 'i', value_name = "TEXT")] + instructions: Option, + + /// Restrict the review to a specific set of files. Other files in + /// the diff are still passed to the agent for context but are + /// excluded from the assembled diff sent to checks. Mirrors + /// `amp review --files`. + #[arg(long = "files", short = 'f', value_name = "FILE", num_args = 1..)] + files: Vec, + + /// Only run checks whose `name` matches one of these. Other + /// discovered checks are skipped. Mirrors `amp review --check-filter`. + #[arg(long = "check-filter", short = 'c', value_name = "NAME", num_args = 1..)] + check_filter: Vec, + + /// Alternate directory to search for `.agents/checks/*.md` instead + /// of the repo root. Mirrors `amp review --check-scope`. + #[arg(long = "check-scope", short = 's', value_name = "DIR")] + check_scope: Option, + + /// Skip the main correctness pass and only run check subagents. + /// Mirrors `amp review --checks-only`. + #[arg(long = "checks-only")] + checks_only: bool, + + /// Print only the diff summary; skip the full review. + /// Mirrors `amp review --summary-only`. + #[arg(long = "summary-only")] + summary_only: bool, + + /// Minimum severity to display. Findings below this rank are + /// dropped from the output. Default is `medium`, matching + /// Amp's CLI which hides `low` from review output. Pass + /// `--severity low` to surface every finding. + #[arg(long = "severity", value_name = "LEVEL", default_value = "medium")] + severity: String, + }, + #[command( name = "validate-extensions", about = "Validate a bundled-extensions.json file", @@ -1157,6 +1255,7 @@ fn get_command_name(command: &Option) -> &'static str { #[cfg(feature = "local-inference")] Some(Command::LocalModels { .. }) => "local-models", Some(Command::Completion { .. }) => "completion", + Some(Command::Review { .. }) => "review", Some(Command::ValidateExtensions { .. }) => "validate-extensions", None => "default_session", } @@ -1990,6 +2089,45 @@ pub async fn cli() -> anyhow::Result<()> { Some(Command::Term { command }) => handle_term_subcommand(command).await, #[cfg(feature = "local-inference")] Some(Command::LocalModels { command }) => handle_local_models_command(command).await, + Some(Command::Review { + range, + prompt, + model, + provider, + override_model, + turn_limit, + dry_run, + quiet, + no_orchestrate, + instructions, + files, + check_filter, + check_scope, + checks_only, + summary_only, + severity, + }) => { + use crate::commands::review::{handle_review, ReviewOptions}; + handle_review(ReviewOptions { + range, + prompt_file: prompt, + default_model: model, + provider, + override_model, + default_turn_limit: turn_limit, + dry_run, + quiet, + no_orchestrate, + instructions, + files, + check_filter, + check_scope, + checks_only, + summary_only, + severity, + }) + .await + } Some(Command::ValidateExtensions { file }) => { use goose::agents::validate_extensions::validate_bundled_extensions; match validate_bundled_extensions(&file) { diff --git a/crates/goose-cli/src/commands/mod.rs b/crates/goose-cli/src/commands/mod.rs index 3dee8b6372..43005e401b 100644 --- a/crates/goose-cli/src/commands/mod.rs +++ b/crates/goose-cli/src/commands/mod.rs @@ -5,6 +5,7 @@ pub mod info; pub mod plugin; pub mod project; pub mod recipe; +pub mod review; pub mod schedule; pub mod session; pub mod term; diff --git a/crates/goose-cli/src/commands/review/default_review_prompt.md b/crates/goose-cli/src/commands/review/default_review_prompt.md new file mode 100644 index 0000000000..4a83d2cfb5 --- /dev/null +++ b/crates/goose-cli/src/commands/review/default_review_prompt.md @@ -0,0 +1,108 @@ +You are reviewing a code change for **correctness bugs**, security issues, +performance problems, and style violations. Be precise and concrete; cite the +exact line(s) and explain the failure mode. + +## Output + +For every issue you find, emit a single JSON object on its own line with the +fields: + +- `severity` — one of `low`, `medium`, `high`, `critical` +- `path` — repo-relative file path +- `line_start` — first line the comment applies to (1-indexed) +- `line_end` — last line the comment applies to +- `summary` — one-paragraph explanation of the issue and the fix +- `check` — the `name` of the check that produced the finding, or + `main` for findings produced by the main review pass + +If there are no issues, emit a single line containing `[]`. + +## Correctness pass (run this for every diff) + +Before delegating to subagent checks, do a careful correctness pass on the +diff yourself. Walk every changed function and look hard for: + +- **Silent error paths.** Missing-key, missing-row, `None`/`null`, and + exception cases that produce a default value instead of surfacing the + error. Flag every place where a missing record is silently coerced to + `0`, `""`, `[]`, etc. +- **Off-by-one and boundary errors.** Loop bounds, slice indices, + ranges, and inclusive vs. exclusive comparisons. +- **Unhandled error returns.** Functions that return `Result`/`Error`/`err` + whose return value is dropped or ignored. +- **Concurrency hazards.** Shared mutable state without a lock, missing + `await`, blocking I/O on async paths, deadlock-prone lock ordering. +- **Resource lifecycle.** File handles, sockets, threads, or subprocess + handles that are not closed/joined on every path (including error + paths). +- **Input validation.** Untrusted input flowing into SQL, shell, file + paths, deserialization, or template rendering without sanitization. +- **State that leaks across requests.** Module-level mutables, default + arguments, and singleton caches that retain user data across calls. +- **Logic that contradicts the comment, docstring, or function name.** + These signal that one of them is wrong; flag the inconsistency. + +Emit findings from this pass with `"check": "main"`. + +## Code-quality pass + +Alongside the correctness pass, walk every changed hunk and call out: + +- **Bugs and hackiness.** Suspicious workarounds, copy-pasted blocks + that drifted, anything that looks like a fix-as-you-go. +- **Unnecessary code.** Dead branches, unreachable paths, redundant + null checks, work that could be deleted without changing behavior. +- **Too much shared mutable state.** Module-level singletons, globals, + parameters mutated across helpers, structures whose ownership is + unclear. +- **Abstraction fit, in both directions.** Flag *unnecessary + indirection* (factories, wrappers, traits, adapters that have one + caller and add no leverage) and *missing abstractions* (the same + five-line block repeated across the diff, or hard-coded values that + belong behind a name). For each finding, cite concrete locations + and recommend exactly one action — only when it improves the + current code, not because it is a "best practice". + +## Guidelines + +- Only comment on the diff. Do not flag pre-existing code unless the diff + meaningfully changes its behavior. +- Prefer high-signal findings over coverage. A small number of correct, + actionable comments is better than many low-confidence ones. +- Treat style nits as `low` severity; reserve `high`/`critical` for real + bugs, regressions, or security issues. + +## Checks + +If the request below lists subagent **checks**, **dispatch them all in +parallel** before doing anything else. For each check: + +``` +delegate( + instructions = , + async = true, # IMPORTANT: parallelize + model = , + max_turns = , +) +``` + +Do NOT pass the check's `tools` value to `extensions`. The `extensions` +parameter filters by **extension name** (e.g. `developer`, `summon`), +not tool name (e.g. `Read`, `Grep`), so passing a tool list there +silently disables every extension and the subagent ends up with no +tools at all. Treat the per-check `tools` column in the request as +informational guidance for the subagent's prompt, not as an +extensions filter. + +This returns a `taskId` immediately. After dispatching every check, call +`load(taskId)` once per check to wait for the results. **Do not** issue +the next `delegate` call after the previous one has completed — that is +sequential and slow; we want every check executing concurrently. + +Run your own correctness pass while the subagents are in flight, so the +wall-clock time is bounded by the slowest single check rather than by +their sum. + +Each subagent must include the originating check's `name` in the `check` +field of every finding so attribution is preserved end-to-end. +Aggregate all findings (yours and theirs) into the same JSON output. diff --git a/crates/goose-cli/src/commands/review/handler.rs b/crates/goose-cli/src/commands/review/handler.rs new file mode 100644 index 0000000000..1b362f6d8d --- /dev/null +++ b/crates/goose-cli/src/commands/review/handler.rs @@ -0,0 +1,618 @@ +use anyhow::{anyhow, bail, Context, Result}; +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Command; + +use crate::session::{build_session, SessionBuilderConfig}; + +use goose::checks::{discover, DiscoveredReview}; + +use super::orchestrator::{ + emit_findings, run_checks_in_parallel, run_main_pass_in_parallel, Severity, +}; +use super::prompt::{build_review_prompt, DEFAULT_REVIEW_PROMPT}; + +/// Options for `goose review`. +#[derive(Debug, Clone, Default)] +pub struct ReviewOptions { + /// Diff range to review (e.g. `main...HEAD`). When `None`, falls back to + /// the working tree vs. the inferred merge base / default branch. + pub range: Option, + /// Path to a markdown file with a custom base review prompt. Overrides the + /// embedded default prompt entirely. + pub prompt_file: Option, + /// Default model used for the main review agent and for any check that + /// does not declare its own `model:`. + pub default_model: Option, + /// Provider for the main review agent. + pub provider: Option, + /// Force every discovered check to run with this model, regardless of + /// the check's own `model:` field. + pub override_model: Option, + /// Default `turn-limit` applied to checks that do not declare their own. + pub default_turn_limit: Option, + /// Print the assembled prompt and discovered checks instead of dispatching + /// the review. + pub dry_run: bool, + /// Suppress non-result output from the underlying agent. + pub quiet: bool, + /// Disable the Rust-driven parallel orchestrator and fall back to the + /// single-prompt path that asks the main agent to delegate checks via + /// `delegate(... async: true ...)`. Useful when comparing against the + /// in-process behavior or running on a model that handles dispatch + /// reliably on its own. + pub no_orchestrate: bool, + /// Additional free-form instructions to prepend to the review (PR + /// intent, commit-message context, etc.). Surfaced to both the main + /// agent and every check subprocess. + pub instructions: Option, + /// Restrict the review to a specific set of files (repo-relative). + /// When non-empty, the diff sent to the agent is filtered to only + /// include hunks for these paths. + pub files: Vec, + /// Only run checks whose `name` is in this list. Empty means run all + /// discovered checks (the default). + pub check_filter: Vec, + /// Alternate directory to search for `.agents/checks/*.md` instead of + /// the repo root. + pub check_scope: Option, + /// Skip the main correctness pass and only run check subagents. + pub checks_only: bool, + /// Print only the diff summary; skip the full review. + pub summary_only: bool, + /// Minimum severity to display from check findings. Defaults to + /// `medium`, matching Amp's CLI behavior of hiding `low` from + /// the review output. + pub severity: String, +} + +/// Entry point for the `goose review` subcommand. +pub async fn handle_review(opts: ReviewOptions) -> Result<()> { + let repo_root = find_repo_root().context("not inside a git repository")?; + + // Validate `--severity` once, up front, so a bogus value fails fast + // regardless of which orchestration path we end up taking. + let sev_str = if opts.severity.is_empty() { + "medium" + } else { + opts.severity.as_str() + }; + let min_sev: Severity = sev_str + .parse() + .map_err(|e: String| anyhow!("--severity: {e}"))?; + + let mut touched = touched_files(&repo_root, opts.range.as_deref(), &opts.files)?; + let mut diff = collect_diff(&repo_root, opts.range.as_deref(), &opts.files)?; + + // Without an explicit `--range`, `git diff HEAD` excludes untracked + // files entirely — brand-new files would silently miss the review. + // Synthesize a `new file` diff for each so the main pass and the + // checks see them. + if opts.range.is_none() { + let untracked = untracked_files(&repo_root, &opts.files)?; + if !untracked.is_empty() { + let untracked_diff = synthesize_untracked_diff(&repo_root, &untracked)?; + diff.push_str(&untracked_diff); + for u in untracked { + if !touched.contains(&u) { + touched.push(u); + } + } + } + } + + if diff.trim().is_empty() { + eprintln!("goose review: no changes to review"); + return Ok(()); + } + + // `--summary-only` short-circuits everything else: print `git + // diff --stat` and return without calling the agent. Mirrors + // `amp review --summary-only`. + if opts.summary_only { + let summary = collect_diff_stat(&repo_root, opts.range.as_deref(), &opts.files)?; + print!("{}", summary); + return Ok(()); + } + + // `--check-scope` overrides where we look for `.agents/checks/*.md`, + // otherwise discovery walks from the repo root + every directory on + // the path of a touched file. + let discovery_root = opts.check_scope.as_deref().unwrap_or(&repo_root); + // `touched` is repo-relative; rebase to discovery_root so candidate + // scope walking doesn't double-prefix `/api/...` for files + // already living under the scope. + let discovery_touched = rebase_touched_to_scope(&repo_root, discovery_root, &touched); + let discovered = discover(discovery_root, &discovery_touched)?; + let discovered = filter_checks(discovered, &opts.check_filter); + if !opts.quiet { + print_discovered_summary(&discovered); + } + + let base_prompt = match &opts.prompt_file { + Some(path) => fs::read_to_string(path) + .with_context(|| format!("read --prompt file {}", path.display()))?, + None => DEFAULT_REVIEW_PROMPT.to_string(), + }; + + let use_orchestrator = !opts.no_orchestrate; + + // Reviewer instructions are also injected into every per-file + // main-pass subprocess and every per-check subprocess. To avoid + // duplicating them, only prepend to the base prompt for the legacy + // single-prompt (`--no-orchestrate`) path. + let base_prompt = if use_orchestrator { + base_prompt + } else { + prepend_instructions(&base_prompt, opts.instructions.as_deref()) + }; + + // In orchestrator mode, the main pass runs as N parallel subprocesses + // (one per touched file) — checks run as parallel subprocesses too — + // so the assembled prompt only matters for the legacy in-process path. + let main_prompt_discovered = if use_orchestrator { + DiscoveredReview::default() + } else { + discovered.clone() + }; + let prompt = build_review_prompt( + &base_prompt, + &main_prompt_discovered, + &diff, + opts.default_model.as_deref(), + opts.override_model.as_deref(), + opts.default_turn_limit, + ); + + if opts.dry_run { + println!("{}", prompt); + if use_orchestrator { + println!( + "\n# orchestrator: {} check(s) would run as parallel subprocesses", + discovered.checks.len() + ); + println!("# orchestrator: main pass would fan out one subprocess per touched file"); + } + return Ok(()); + } + + if !use_orchestrator { + // Legacy in-process path (--no-orchestrate). Useful for comparing + // against orchestrated wall clock and for models that handle + // delegation reliably on their own. + if opts.checks_only { + // The legacy path runs everything as a single agent prompt, + // so it has no way to "skip the main pass". Fall back to the + // orchestrator's check-runner (which IS able to run checks + // in isolation) instead of silently no-op'ing. + let check_results = run_checks_in_parallel(&discovered.checks, &diff, &opts).await; + let mut total_emitted = 0usize; + let mut total_seen = 0usize; + for findings in &check_results { + total_seen += findings.len(); + total_emitted += emit_findings(findings, min_sev); + } + if !opts.quiet { + let suppressed = total_seen.saturating_sub(total_emitted); + eprintln!( + "goose review: emitted {total_emitted} finding(s) from {} check(s) ({suppressed} hidden below severity={:?})", + discovered.checks.len(), + min_sev + ); + } + return Ok(()); + } + let mut session = build_session(SessionBuilderConfig { + session_id: None, + no_session: true, + no_profile: true, + builtins: vec!["developer".to_string(), "summon".to_string()], + provider: opts.provider.clone(), + model: opts.default_model.clone(), + quiet: opts.quiet, + output_format: "text".to_string(), + ..SessionBuilderConfig::default() + }) + .await; + return session.headless(prompt).await; + } + + // Orchestrated mode: run the main correctness pass (per-file + // parallel subprocesses) and the discovered checks (one subprocess + // each, capped at MAX_WORKERS) concurrently. Wall clock is bounded + // by `max(slowest_main_file, slowest_check)` instead of scaling + // with diff size or check count. + let main_findings_fut = async { + if opts.checks_only { + Vec::new() + } else { + run_main_pass_in_parallel(&diff, &base_prompt, &opts).await + } + }; + let checks_fut = run_checks_in_parallel(&discovered.checks, &diff, &opts); + let (main_findings, check_results) = tokio::join!(main_findings_fut, checks_fut); + + let mut total_emitted = 0usize; + let mut total_seen = main_findings.len(); + total_emitted += emit_findings(&main_findings, min_sev); + for findings in &check_results { + total_seen += findings.len(); + total_emitted += emit_findings(findings, min_sev); + } + if !opts.quiet { + let suppressed = total_seen.saturating_sub(total_emitted); + let main_pass_label = if opts.checks_only { "skipped" } else { "ran" }; + if suppressed == 0 { + eprintln!( + "goose review: orchestrator emitted {total_emitted} finding(s) from {} check(s) (main: {main_pass_label}, {} finding(s))", + discovered.checks.len(), + main_findings.len() + ); + } else { + eprintln!( + "goose review: orchestrator emitted {total_emitted} finding(s) from {} check(s) (main: {main_pass_label}, {} finding(s); {suppressed} hidden below severity={:?})", + discovered.checks.len(), + main_findings.len(), + min_sev + ); + } + } + + Ok(()) +} + +/// Restrict a discovered review to the named checks (no-op when the +/// filter is empty). Mirrors `amp review --check-filter`. +fn filter_checks(discovered: DiscoveredReview, names: &[String]) -> DiscoveredReview { + if names.is_empty() { + return discovered; + } + let allow: std::collections::HashSet<&str> = names.iter().map(String::as_str).collect(); + DiscoveredReview { + checks: discovered + .checks + .into_iter() + .filter(|c| allow.contains(c.name.as_str())) + .collect(), + } +} + +/// Prepend a free-form `--instructions ` block to the base prompt +/// so it is visible to both the main agent and (via the orchestrator) +/// every per-check subprocess. +fn prepend_instructions(base_prompt: &str, instructions: Option<&str>) -> String { + match instructions { + Some(text) if !text.trim().is_empty() => { + format!( + "## Reviewer instructions\n\n{}\n\n{}", + text.trim(), + base_prompt + ) + } + _ => base_prompt.to_string(), + } +} + +fn print_discovered_summary(d: &DiscoveredReview) { + if d.checks.is_empty() { + eprintln!("goose review: no checks or REVIEW.md rules discovered"); + return; + } + eprintln!("goose review: discovered {} check(s):", d.checks.len()); + for c in &d.checks { + let scope = if c.scope_dir.is_empty() { + "" + } else { + &c.scope_dir + }; + eprintln!(" - {} (scope: {})", c.name, scope); + } +} + +fn find_repo_root() -> Result { + let out = Command::new("git") + .args(["rev-parse", "--show-toplevel"]) + .output() + .context("failed to invoke git")?; + if !out.status.success() { + bail!( + "git rev-parse --show-toplevel failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + let path = String::from_utf8(out.stdout)?.trim().to_string(); + Ok(PathBuf::from(path)) +} + +/// Configure a `git` Command to disable quoting of non-ASCII paths. +/// Without this, paths containing non-ASCII bytes come back as quoted +/// C-style escapes (`"dir/\303\251.txt"`), which downstream parsers +/// would have to round-trip-decode just to spell the filename. We turn +/// it off everywhere so callers always get clean UTF-8 paths. +fn git_command(repo_root: &Path) -> Command { + let mut cmd = Command::new("git"); + cmd.current_dir(repo_root) + .args(["-c", "core.quotePath=off"]); + cmd +} + +fn touched_files(repo_root: &Path, range: Option<&str>, files: &[String]) -> Result> { + let mut cmd = git_command(repo_root); + cmd.arg("diff").arg("--name-only"); + match range { + Some(r) => { + cmd.arg(r); + } + None => { + cmd.arg("HEAD"); + } + } + if !files.is_empty() { + cmd.arg("--"); + for f in files { + cmd.arg(f); + } + } + let out = cmd.output().context("git diff --name-only failed")?; + if !out.status.success() { + bail!( + "git diff --name-only failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + Ok(String::from_utf8(out.stdout)? + .lines() + .filter(|l| !l.trim().is_empty()) + .map(|l| l.to_string()) + .collect()) +} + +fn collect_diff(repo_root: &Path, range: Option<&str>, files: &[String]) -> Result { + let mut cmd = git_command(repo_root); + cmd.arg("diff"); + match range { + Some(r) => { + cmd.arg(r); + } + None => { + cmd.arg("HEAD"); + } + } + if !files.is_empty() { + cmd.arg("--"); + for f in files { + cmd.arg(f); + } + } + let out = cmd.output().context("git diff failed")?; + if !out.status.success() { + bail!("git diff failed: {}", String::from_utf8_lossy(&out.stderr)); + } + String::from_utf8(out.stdout).map_err(|e| anyhow!("git diff returned non-UTF8 output: {e}")) +} + +fn collect_diff_stat(repo_root: &Path, range: Option<&str>, files: &[String]) -> Result { + let mut cmd = git_command(repo_root); + cmd.arg("diff").arg("--stat"); + match range { + Some(r) => { + cmd.arg(r); + } + None => { + cmd.arg("HEAD"); + } + } + if !files.is_empty() { + cmd.arg("--"); + for f in files { + cmd.arg(f); + } + } + let out = cmd.output().context("git diff --stat failed")?; + if !out.status.success() { + bail!( + "git diff --stat failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + String::from_utf8(out.stdout) + .map_err(|e| anyhow!("git diff --stat returned non-UTF8 output: {e}")) +} + +/// List untracked-but-not-ignored files in `repo_root`. Used to expose +/// brand-new files to the review when no `--range` is given (default +/// `git diff HEAD` would silently drop them). +fn untracked_files(repo_root: &Path, files: &[String]) -> Result> { + let mut cmd = git_command(repo_root); + cmd.args(["ls-files", "--others", "--exclude-standard"]); + if !files.is_empty() { + cmd.arg("--"); + for f in files { + cmd.arg(f); + } + } + let out = cmd.output().context("git ls-files failed")?; + if !out.status.success() { + bail!( + "git ls-files failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + Ok(String::from_utf8(out.stdout)? + .lines() + .filter(|l| !l.trim().is_empty()) + .map(|l| l.to_string()) + .collect()) +} + +/// Synthesize a unified `new file` diff for each untracked path so +/// downstream parsers and the review prompt can treat them as +/// additions. Binary or unreadable files are skipped (we cannot +/// produce a meaningful textual diff for them). +fn synthesize_untracked_diff(repo_root: &Path, paths: &[String]) -> Result { + let mut out = String::new(); + for path in paths { + let abs = repo_root.join(path); + let content = match fs::read_to_string(&abs) { + Ok(c) => c, + Err(_) => continue, + }; + out.push_str(&format!("diff --git a/{path} b/{path}\n")); + out.push_str("new file mode 100644\n"); + out.push_str("--- /dev/null\n"); + out.push_str(&format!("+++ b/{path}\n")); + let trailing_newline = content.ends_with('\n'); + let line_count = if content.is_empty() { + 0 + } else if trailing_newline { + content.matches('\n').count() + } else { + content.matches('\n').count() + 1 + }; + if line_count > 0 { + out.push_str(&format!("@@ -0,0 +1,{line_count} @@\n")); + for line in content.split_inclusive('\n') { + let body = line.strip_suffix('\n').unwrap_or(line); + out.push('+'); + out.push_str(body); + out.push('\n'); + } + if !trailing_newline { + out.push_str("\\ No newline at end of file\n"); + } + } + } + Ok(out) +} + +/// Convert repo-relative `touched` paths into paths relative to +/// `discovery_root` so [`goose::checks::discover`] doesn't double- +/// prefix `/api/...` when `--check-scope` points at a subtree. +/// Files outside the scope are dropped — they cannot affect any +/// scoped check inside `discovery_root`. +fn rebase_touched_to_scope( + repo_root: &Path, + discovery_root: &Path, + touched: &[String], +) -> Vec { + if discovery_root == repo_root { + return touched.to_vec(); + } + let prefix = match discovery_root.strip_prefix(repo_root) { + Ok(p) => p, + Err(_) => return touched.to_vec(), + }; + let prefix_str = prefix.to_string_lossy().replace('\\', "/"); + if prefix_str.is_empty() { + return touched.to_vec(); + } + let prefix_with_slash = format!("{prefix_str}/"); + touched + .iter() + .filter_map(|p| p.strip_prefix(&prefix_with_slash).map(str::to_string)) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use goose::checks::Check; + use std::path::PathBuf; + + fn ck(name: &str) -> Check { + Check { + name: name.to_string(), + description: None, + model: None, + turn_limit: None, + tools: None, + severity_default: None, + path: PathBuf::from(format!("/.agents/checks/{name}.md")), + scope_dir: String::new(), + body: "body".into(), + } + } + + #[test] + fn filter_checks_passes_through_when_filter_empty() { + let d = DiscoveredReview { + checks: vec![ck("perf"), ck("security")], + }; + let out = filter_checks(d, &[]); + assert_eq!(out.checks.len(), 2); + } + + #[test] + fn filter_checks_keeps_only_named_checks() { + let d = DiscoveredReview { + checks: vec![ck("perf"), ck("security"), ck("idempotency")], + }; + let out = filter_checks(d, &["security".to_string(), "idempotency".to_string()]); + let names: Vec<&str> = out.checks.iter().map(|c| c.name.as_str()).collect(); + assert_eq!(names, vec!["security", "idempotency"]); + } + + #[test] + fn prepend_instructions_noop_when_none_or_empty() { + assert_eq!(prepend_instructions("BASE", None), "BASE"); + assert_eq!(prepend_instructions("BASE", Some(" ")), "BASE"); + } + + #[test] + fn prepend_instructions_adds_block_above_base() { + let out = prepend_instructions("BASE", Some("Refactor only — flag any behavior change.")); + assert!(out.starts_with("## Reviewer instructions\n\nRefactor only")); + assert!(out.ends_with("BASE")); + } + + #[test] + fn synthesize_untracked_diff_emits_new_file_chunk_with_added_lines() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + let path = root.join("new/file.txt"); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(&path, "alpha\nbeta\ngamma\n").unwrap(); + + let diff = synthesize_untracked_diff(root, &["new/file.txt".to_string()]).unwrap(); + assert!(diff.contains("diff --git a/new/file.txt b/new/file.txt")); + assert!(diff.contains("new file mode 100644")); + assert!(diff.contains("--- /dev/null")); + assert!(diff.contains("+++ b/new/file.txt")); + assert!(diff.contains("@@ -0,0 +1,3 @@")); + assert!(diff.contains("+alpha\n+beta\n+gamma\n")); + assert!(!diff.contains("\\ No newline at end of file")); + } + + #[test] + fn synthesize_untracked_diff_marks_missing_trailing_newline() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + fs::write(root.join("a.txt"), "no-newline").unwrap(); + + let diff = synthesize_untracked_diff(root, &["a.txt".to_string()]).unwrap(); + assert!(diff.contains("@@ -0,0 +1,1 @@")); + assert!(diff.contains("+no-newline\n")); + assert!(diff.contains("\\ No newline at end of file")); + } + + #[test] + fn rebase_touched_to_scope_strips_scope_prefix() { + let repo = PathBuf::from("/repo"); + let scope = PathBuf::from("/repo/api/v2"); + let touched = vec![ + "api/v2/foo.rs".to_string(), + "api/v2/bar.rs".to_string(), + "frontend/main.tsx".to_string(), + ]; + let out = rebase_touched_to_scope(&repo, &scope, &touched); + assert_eq!(out, vec!["foo.rs", "bar.rs"]); + } + + #[test] + fn rebase_touched_to_scope_passes_through_when_scope_equals_repo() { + let repo = PathBuf::from("/repo"); + let touched = vec!["a.rs".to_string()]; + let out = rebase_touched_to_scope(&repo, &repo, &touched); + assert_eq!(out, touched); + } +} diff --git a/crates/goose-cli/src/commands/review/mod.rs b/crates/goose-cli/src/commands/review/mod.rs new file mode 100644 index 0000000000..42efde77a9 --- /dev/null +++ b/crates/goose-cli/src/commands/review/mod.rs @@ -0,0 +1,16 @@ +//! `goose review` — local code review tool. +//! +//! Discovers `**/.agents/checks/*.md` subagent reviewers and `**/.agents/REVIEW.md` +//! scoped prompt overrides, builds a review request from the working tree (or an +//! explicit diff range), and dispatches the review to the configured agent. +//! +//! Modeled after Amp's `review` command. +//! +//! Check parsing and discovery live in [`goose::checks`] so they can be reused +//! from other entry points (server, ACP) without depending on this CLI. + +pub mod handler; +pub mod orchestrator; +pub mod prompt; + +pub use handler::{handle_review, ReviewOptions}; diff --git a/crates/goose-cli/src/commands/review/orchestrator.rs b/crates/goose-cli/src/commands/review/orchestrator.rs new file mode 100644 index 0000000000..af2b72e7b3 --- /dev/null +++ b/crates/goose-cli/src/commands/review/orchestrator.rs @@ -0,0 +1,1121 @@ +//! Deterministic, Rust-driven orchestration for `goose review`. +//! +//! The default in-process review path lets the LLM decide whether to +//! dispatch each check as a real subagent (`delegate(... async: true)`) +//! or to inline the work itself. That decision is non-deterministic and +//! is the dominant source of variance we see between runs (16s in the +//! best case, 60s+ when the model dispatches everything as a separate +//! subagent). +//! +//! This module sidesteps that variance by orchestrating checks +//! deterministically from Rust: +//! +//! - One subprocess per check (`goose run -q -t `) +//! - Concurrency capped at [`MAX_WORKERS`] via a Tokio semaphore +//! - Per-check timeout of [`CHECK_TIMEOUT_SECS`] +//! - Each check is given a strict, tool-free prompt and is required to +//! return only `{"findings": [...]}` JSON +//! - Findings are tagged with the originating `check` name in Rust, not +//! by the model +//! +//! Wall-clock for the orchestrated phase is therefore +//! `max(check_latency)` — bounded by the slowest single check — rather +//! than the sum of model-driven dispatch overhead. +//! +//! The main correctness pass still runs in-process via the existing +//! `session.headless()` path; the two phases are awaited concurrently +//! so the user sees both their findings as soon as the slower of the +//! two completes. + +use anyhow::{Context, Result}; +use serde::{Deserialize, Serialize}; +use std::process::Stdio; +use std::sync::Arc; +use std::time::Duration; +use tokio::io::AsyncWriteExt; +use tokio::process::Command; +use tokio::sync::Semaphore; +use tokio::task::JoinSet; +use tokio::time::timeout; + +use super::handler::ReviewOptions; +use goose::checks::Check; + +/// Maximum number of check subprocesses we run concurrently. 4 is +/// empirically the sweet spot before LLM-side rate limits and local +/// resource contention start hurting wall-clock. +pub const MAX_WORKERS: usize = 4; + +/// Hard wall-clock cap for a single check subprocess. A check that +/// takes longer than this is almost always stuck in a tool-call loop +/// or a retry storm; we'd rather surface the timeout than block the +/// whole review. +pub const CHECK_TIMEOUT_SECS: u64 = 5 * 60; + +/// One review finding emitted by a check or by the main correctness +/// pass. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct Finding { + pub severity: String, + pub path: String, + pub line_start: i64, + pub line_end: i64, + pub summary: String, + pub check: String, +} + +/// Schema the check subprocess is required to emit. +#[derive(Debug, Deserialize)] +struct FindingsResponse { + findings: Vec, +} + +#[derive(Debug, Deserialize)] +struct RawFinding { + #[serde(default)] + severity: Option, + #[serde(default)] + path: Option, + #[serde(default)] + line_start: Option, + #[serde(default)] + line_end: Option, + #[serde(default)] + summary: Option, +} + +/// Run all discovered checks concurrently as `goose run` subprocesses. +/// +/// Returns one `Vec` per check, in the same order as `checks`. +/// A failed check (subprocess error, timeout, malformed JSON) yields an +/// empty findings list and a warning on stderr; a single broken check +/// must never block the rest of the review. +pub async fn run_checks_in_parallel( + checks: &[Check], + diff: &str, + opts: &ReviewOptions, +) -> Vec> { + let semaphore = Arc::new(Semaphore::new(MAX_WORKERS)); + let mut set = JoinSet::new(); + + for (idx, check) in checks.iter().enumerate() { + let sem = semaphore.clone(); + let check = check.clone(); + let diff = diff.to_string(); + let provider = opts.provider.clone(); + let model = resolve_check_model(&check, opts); + let max_turns = check.resolved_turn_limit(opts.default_turn_limit); + let quiet = opts.quiet; + let instructions = opts.instructions.clone(); + + set.spawn(async move { + // Bounded concurrency: drop the permit only after the + // subprocess completes. + let _permit = sem.acquire().await.expect("semaphore is never closed"); + let result = run_single_check_subprocess( + &check, + &diff, + provider.as_deref(), + model.as_deref(), + instructions.as_deref(), + Some(max_turns), + ) + .await; + (idx, check, result, quiet) + }); + } + + // Pre-allocate so we can write results in source order. + let mut results: Vec> = vec![Vec::new(); checks.len()]; + + while let Some(joined) = set.join_next().await { + let (idx, check, result, quiet) = match joined { + Ok(v) => v, + Err(e) => { + eprintln!("goose review: check task panicked: {e}"); + continue; + } + }; + + match result { + Ok(findings) => { + if !quiet { + eprintln!( + "goose review: check '{}' completed: {} finding(s)", + check.name, + findings.len() + ); + } + results[idx] = findings; + } + Err(e) => { + // Per-check failure must never abort the review — emit a + // warning and continue with empty findings for this check. + eprintln!("goose review: check '{}' failed: {e}", check.name); + results[idx] = Vec::new(); + } + } + } + + results +} + +/// Resolve which model a check should run on. +/// +/// Precedence (most specific wins): +/// 1. `--override-model` always wins. +/// 2. If the user picked an explicit `--provider` on the CLI, drop the +/// per-check `model:` declaration entirely. The per-check model is +/// almost always pinned to a specific provider (e.g. a check that +/// asks for `goose-claude-4-sonnet` would 404 against Google's API), +/// so silently inheriting it across providers makes targeted reruns +/// fail. Use `--model` if set, otherwise fall through to the +/// selected provider's default. +/// 3. Per-check `model:` from frontmatter. +/// 4. `--model` (or the agent default). +fn resolve_check_model(check: &Check, opts: &ReviewOptions) -> Option { + if let Some(o) = opts.override_model.as_deref() { + return Some(o.to_string()); + } + if opts.provider.is_some() { + return opts.default_model.clone(); + } + if let Some(m) = check.model.as_deref() { + return Some(m.to_string()); + } + opts.default_model.clone() +} + +/// Spawn a single `goose run` subprocess for one check and parse its +/// output into [`Finding`]s. +async fn run_single_check_subprocess( + check: &Check, + diff: &str, + provider: Option<&str>, + model: Option<&str>, + instructions: Option<&str>, + max_turns: Option, +) -> Result> { + let prompt = build_check_prompt(check, diff, instructions); + let raw = run_subprocess_for_findings( + &prompt, + &format!("check '{}'", check.name), + provider, + model, + max_turns, + ) + .await?; + let default_sev = check.severity_default.as_deref().unwrap_or("medium"); + Ok(raw + .into_iter() + .map(|r| Finding { + severity: r.severity.unwrap_or_else(|| default_sev.to_string()), + path: r.path.unwrap_or_default(), + line_start: r.line_start.unwrap_or(0), + line_end: r.line_end.unwrap_or(0), + summary: r.summary.unwrap_or_default(), + check: check.name.clone(), + }) + .collect()) +} + +/// Generic `goose run` subprocess that hands a prompt to the model +/// and parses `{"findings": [...]}` JSON out of the response. Shared +/// by the per-check and per-file main-pass orchestrators so both get +/// the same robust JSON extraction, timeout handling, and error +/// reporting. +async fn run_subprocess_for_findings( + prompt: &str, + label: &str, + provider: Option<&str>, + model: Option<&str>, + max_turns: Option, +) -> Result> { + let goose_bin = std::env::current_exe().context("locate current goose binary")?; + + let mut cmd = Command::new(&goose_bin); + cmd.arg("run") + .arg("--no-session") + .arg("--quiet") + .arg("--no-profile") + .arg("-i") + .arg("-") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + // Drop-on-cancel safety: when the outer `timeout` fires it drops + // this future, which drops the Child handle. Without + // kill_on_drop, the Tokio runtime leaves the subprocess running + // (and racking up tokens) in the background — kill_on_drop sends + // SIGKILL on Drop instead. + .kill_on_drop(true); + + if let Some(p) = provider { + cmd.arg("--provider").arg(p); + } + if let Some(m) = model { + cmd.arg("--model").arg(m); + } + if let Some(t) = max_turns { + cmd.arg("--max-turns").arg(t.to_string()); + } + + let mut child = cmd + .spawn() + .with_context(|| format!("spawn subprocess for {label}"))?; + + if let Some(mut stdin) = child.stdin.take() { + stdin + .write_all(prompt.as_bytes()) + .await + .with_context(|| format!("write prompt to {label} stdin"))?; + // Closing stdin signals EOF to `goose run -i -`. + drop(stdin); + } + + let wait = child.wait_with_output(); + let output = match timeout(Duration::from_secs(CHECK_TIMEOUT_SECS), wait).await { + Ok(o) => o.with_context(|| format!("wait on {label}"))?, + Err(_) => { + anyhow::bail!("{label} timed out after {}s", CHECK_TIMEOUT_SECS); + } + }; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!( + "{label} subprocess exited with status {}: {}", + output.status, + truncate(&stderr, 500) + ); + } + + let stdout = String::from_utf8_lossy(&output.stdout); + parse_findings(&stdout) +} + +/// Run the main correctness pass as N parallel subprocesses, one per +/// touched file. This replaces the older in-process `session.headless()` +/// path which: +/// +/// 1. Streamed text-mode chatter to stdout (not JSONL) so findings were +/// sometimes lost in interleaved output. +/// 2. Sent the entire diff in a single prompt — large diffs (1000+ +/// lines) reliably caused Gemini 3.x to short-circuit with `[]` +/// after ~30s instead of doing the work. +/// +/// File-by-file fan-out keeps each subprocess context small enough that +/// the model actually walks every change, and runs them concurrently +/// so total wall clock stays close to the slowest single file rather +/// than scaling with diff size. Failures on one file never block the +/// others. +pub async fn run_main_pass_in_parallel( + diff: &str, + base_prompt: &str, + opts: &ReviewOptions, +) -> Vec { + let per_file = split_diff_by_file(diff); + if per_file.is_empty() { + return Vec::new(); + } + + let semaphore = Arc::new(Semaphore::new(MAX_WORKERS)); + let mut set: JoinSet<(usize, String, Result>, bool)> = JoinSet::new(); + + for (idx, (path, file_diff)) in per_file.iter().enumerate() { + let sem = semaphore.clone(); + let path = path.clone(); + let file_diff = file_diff.clone(); + let provider = opts.provider.clone(); + let model = opts.default_model.clone(); + let quiet = opts.quiet; + let instructions = opts.instructions.clone(); + let base_prompt = base_prompt.to_string(); + + set.spawn(async move { + let _permit = sem.acquire().await.expect("semaphore is never closed"); + let prompt = + build_main_pass_prompt(&path, &file_diff, &base_prompt, instructions.as_deref()); + let label = format!("main:{path}"); + let result = run_subprocess_for_findings( + &prompt, + &label, + provider.as_deref(), + model.as_deref(), + None, + ) + .await; + (idx, path, result, quiet) + }); + } + + let mut per_file_results: Vec> = vec![Vec::new(); per_file.len()]; + while let Some(joined) = set.join_next().await { + let (idx, path, result, quiet) = match joined { + Ok(v) => v, + Err(e) => { + eprintln!("goose review: main-pass task panicked: {e}"); + continue; + } + }; + match result { + Ok(raw) => { + let findings: Vec = raw + .into_iter() + .map(|r| Finding { + severity: r.severity.unwrap_or_else(|| "medium".to_string()), + path: r.path.unwrap_or_else(|| path.clone()), + line_start: r.line_start.unwrap_or(0), + line_end: r.line_end.unwrap_or(0), + summary: r.summary.unwrap_or_default(), + check: "main".to_string(), + }) + .collect(); + if !quiet { + eprintln!( + "goose review: main pass on '{}' completed: {} finding(s)", + path, + findings.len() + ); + } + per_file_results[idx] = findings; + } + Err(e) => { + // A single broken file must not abort the entire main + // pass; surface a warning and continue. + eprintln!("goose review: main pass on '{}' failed: {e}", path); + per_file_results[idx] = Vec::new(); + } + } + } + + per_file_results.into_iter().flatten().collect() +} + +/// Split a unified `git diff` into one chunk per file. Each chunk +/// starts at its `diff --git a/... b/...` header and runs to the +/// next file boundary. Returns `(repo_relative_path, file_diff)` +/// pairs in source order. +pub fn split_diff_by_file(diff: &str) -> Vec<(String, String)> { + let mut out: Vec<(String, String)> = Vec::new(); + let mut current_path: Option = None; + let mut current_chunk = String::new(); + + for line in diff.split_inclusive('\n') { + if let Some(rest) = line.strip_prefix("diff --git ") { + // Flush previous file. + if let Some(p) = current_path.take() { + if !current_chunk.is_empty() { + out.push((p, std::mem::take(&mut current_chunk))); + } else { + current_chunk.clear(); + } + } + current_chunk.clear(); + current_chunk.push_str(line); + // Header is `a/ b/`. Pull `b/` (post-image) + // because that's the path the model should reference for + // line numbers. + current_path = parse_diff_header_path(rest); + } else { + current_chunk.push_str(line); + } + } + if let Some(p) = current_path { + if !current_chunk.is_empty() { + out.push((p, current_chunk)); + } + } + out +} + +/// Parse the `a/ b/` portion of a `diff --git` header line. +/// Returns the post-image path (`b/...`) when present; falls back to +/// the pre-image path (`a/...`) for deletions. +/// +/// Also handles git's quoted form for paths with non-ASCII bytes, +/// spaces, or special chars (e.g. `"a/dir/\303\251.txt" "b/dir/\303\251.txt"`, +/// emitted whenever `core.quotePath` is on or the path contains +/// whitespace). Without quote handling, files under those paths are +/// silently dropped from the per-file main pass. +#[allow(clippy::string_slice)] +fn parse_diff_header_path(rest: &str) -> Option { + let trimmed = rest.trim_end_matches('\n').trim(); + + if trimmed.starts_with('"') { + let (a_quoted, after_a) = take_quoted(trimmed)?; + let after_a = after_a.trim_start(); + let post = if after_a.starts_with('"') { + let (b_quoted, _) = take_quoted(after_a)?; + b_quoted + } else { + // Mixed form: quoted a/, unquoted b/. + after_a.to_string() + }; + return Some( + strip_diff_prefix(&post) + .unwrap_or_else(|| strip_diff_prefix(&a_quoted).unwrap_or(post)), + ); + } + + if let Some(idx) = trimmed.find(" b/") { + let post = &trimmed[idx + 3..]; + // The post-image path may itself be quoted in mixed-form + // headers like `a/foo.txt "b/with space.txt"`. + if post.starts_with('"') { + let (q, _) = take_quoted(post)?; + return Some(strip_diff_prefix(&q).unwrap_or(q)); + } + return Some(post.to_string()); + } + if let Some(stripped) = trimmed.strip_prefix("a/") { + return Some(stripped.to_string()); + } + None +} + +/// Strip the leading `a/` or `b/` prefix that git puts on diff header +/// paths. Returns `None` if no such prefix exists. +fn strip_diff_prefix(s: &str) -> Option { + s.strip_prefix("a/") + .or_else(|| s.strip_prefix("b/")) + .map(str::to_string) +} + +/// Pull a single C-style quoted token off the start of `s` and return +/// `(decoded, remainder)`. Decodes the escape sequences git uses when +/// `core.quotePath` is on: `\\`, `\"`, `\a`, `\b`, `\t`, `\n`, `\v`, +/// `\f`, `\r`, and octal `\NNN` byte escapes (for non-ASCII paths). +fn take_quoted(s: &str) -> Option<(String, &str)> { + let bytes = s.as_bytes(); + if bytes.first() != Some(&b'"') { + return None; + } + let mut decoded: Vec = Vec::with_capacity(s.len()); + let mut i = 1; + while i < bytes.len() { + match bytes[i] { + b'"' => { + let rest = std::str::from_utf8(&bytes[i + 1..]).ok()?; + let s = String::from_utf8(decoded).ok()?; + return Some((s, rest)); + } + b'\\' => { + if i + 1 >= bytes.len() { + return None; + } + match bytes[i + 1] { + b'\\' => { + decoded.push(b'\\'); + i += 2; + } + b'"' => { + decoded.push(b'"'); + i += 2; + } + b'a' => { + decoded.push(0x07); + i += 2; + } + b'b' => { + decoded.push(0x08); + i += 2; + } + b't' => { + decoded.push(b'\t'); + i += 2; + } + b'n' => { + decoded.push(b'\n'); + i += 2; + } + b'v' => { + decoded.push(0x0b); + i += 2; + } + b'f' => { + decoded.push(0x0c); + i += 2; + } + b'r' => { + decoded.push(b'\r'); + i += 2; + } + c if (b'0'..=b'7').contains(&c) => { + if i + 3 >= bytes.len() { + return None; + } + let octal = &bytes[i + 1..i + 4]; + if !octal.iter().all(|b| (b'0'..=b'7').contains(b)) { + return None; + } + let val = ((octal[0] - b'0') as u16) * 64 + + ((octal[1] - b'0') as u16) * 8 + + (octal[2] - b'0') as u16; + decoded.push(val as u8); + i += 4; + } + _ => return None, + } + } + b => { + decoded.push(b); + i += 1; + } + } + } + None +} + +/// Build the strict, JSON-only prompt sent to one main-pass +/// subprocess. The base prompt (custom or +/// [`DEFAULT_REVIEW_PROMPT`]) supplies the reviewer voice; we then +/// pin the file under review and force a `{"findings": [...]}` +/// response so the orchestrator's parser can pick it up reliably. +fn build_main_pass_prompt( + path: &str, + file_diff: &str, + base_prompt: &str, + instructions: Option<&str>, +) -> String { + let mut s = String::new(); + s.push_str(base_prompt.trim_end()); + s.push_str("\n\n"); + if let Some(text) = instructions { + let trimmed = text.trim(); + if !trimmed.is_empty() { + s.push_str("## Reviewer instructions\n\n"); + s.push_str(trimmed); + s.push_str("\n\n"); + } + } + s.push_str("## File under review\n\n"); + s.push_str(&format!("Path: `{path}`\n\n")); + s.push_str( + "Review ONLY the changes in this file. Walk every added/modified line. \ + Do not flag pre-existing code shown for context (lines beginning with a space). \ + Use post-change line numbers from the diff.\n\n", + ); + s.push_str( + "## Output\n\nReturn ONLY valid JSON with this exact schema:\n\n\ +{\n \"findings\": [\n {\n \"severity\": \"low|medium|high|critical\",\n \"path\": \"relative/path/to/file\",\n \"line_start\": 10,\n \"line_end\": 12,\n \"summary\": \"One-paragraph actionable explanation of the issue and the fix\"\n }\n ]\n}\n\nIf there are no real issues, return:\n{\"findings\":[]}\n\nDo NOT include any text before or after the JSON. Do NOT wrap the JSON in code fences.\n\n", + ); + s.push_str("## Diff\n\n```diff\n"); + s.push_str(file_diff.trim_end_matches('\n')); + s.push_str("\n```\n"); + s +} + +/// Build the strict, tool-free prompt sent to one check subprocess. +/// +/// Shape matches the prompt format Amp-authored checks already expect, +/// so a check written for `amp review` runs the same way under +/// `goose review`. +fn build_check_prompt(check: &Check, diff: &str, instructions: Option<&str>) -> String { + let mut s = String::new(); + s.push_str("You are running an automated code review check.\n\n"); + s.push_str(&format!("Check name: {}\n", check.name)); + if let Some(d) = check.description.as_deref() { + if !d.is_empty() { + s.push_str(&format!("Description: {}\n", d)); + } + } + if let Some(sev) = check.severity_default.as_deref() { + if !sev.is_empty() { + s.push_str(&format!("Default severity: {}\n", sev)); + } + } + if let Some(text) = instructions { + let trimmed = text.trim(); + if !trimmed.is_empty() { + s.push_str("\nReviewer instructions:\n"); + s.push_str(trimmed); + s.push('\n'); + } + } + s.push_str("\nReview ONLY the git diff provided below.\n"); + s.push_str("Do not ask for missing context.\n"); + s.push_str("Use repo-relative file paths.\n"); + s.push_str("Use post-change line numbers from the diff.\n"); + s.push_str("If you cannot map an issue to a specific line range in the diff, use line_start: 0 and line_end: 0.\n"); + // Amp's check prompt emphasizes this twice; without it, models + // routinely flag pre-existing code that just happens to appear in + // the diff context (lines starting with a space, not `+`). + s.push_str( + "Search for patterns described above ONLY in the changed lines (lines beginning with `+` in the diff).\n", + ); + s.push_str("Report issues ONLY for code that was added or modified in this diff.\n"); + s.push_str("Do NOT report issues for unchanged/pre-existing code shown for context.\n\n"); + s.push_str( + "Return ONLY valid JSON with this exact schema:\n\ +{\n \"findings\": [\n {\n \"severity\": \"low|medium|high|critical\",\n \"path\": \"relative/path/to/file\",\n \"line_start\": 10,\n \"line_end\": 12,\n \"summary\": \"One-sentence actionable issue\"\n }\n ]\n}\n\nIf there are no issues, return:\n{\"findings\":[]}\n\nDo NOT include any text before or after the JSON. Do NOT wrap the JSON in code fences.\n\n", + ); + s.push_str("Check instructions:\n\n"); + s.push_str(check.body.trim()); + s.push_str("\n\nDiff:\n\n```diff\n"); + s.push_str(diff.trim_end_matches('\n')); + s.push_str("\n```\n"); + s +} + +/// Pull the `findings` array out of an LLM response, tolerating code +/// fences and stray text the model occasionally inserts. +fn parse_findings(output: &str) -> Result> { + let stripped = strip_code_fences(output.trim()); + let json = extract_json_object(&stripped).unwrap_or(stripped); + let resp: FindingsResponse = serde_json::from_str(&json) + .with_context(|| format!("parse check JSON: {}", truncate(&json, 500)))?; + Ok(resp.findings) +} + +fn strip_code_fences(s: &str) -> String { + let s = s.trim(); + if let Some(after_open) = s.strip_prefix("```") { + let after_first_line = after_open + .split_once('\n') + .map(|(_, rest)| rest) + .unwrap_or(""); + let trimmed_close = after_first_line + .rsplit_once("```") + .map(|(before, _)| before) + .unwrap_or(after_first_line); + return trimmed_close.trim().to_string(); + } + s.to_string() +} + +#[allow(clippy::string_slice)] +fn extract_json_object(s: &str) -> Option { + // The structural characters we scan for (`"`, `\`, `{`, `}`) are + // single-byte ASCII, so iterating char-by-char with byte offsets via + // `char_indices` is safe even when the LLM's chatter around the JSON + // contains multi-byte characters. The two slice operations below + // (`s[start..]` and `s[start..=abs]`) only ever land on UTF-8 char + // boundaries because `start` is from `find('{')` and `abs` is from + // `char_indices`, both of which yield boundary offsets. + let start = s.find('{')?; + let mut depth = 0i32; + let mut in_string = false; + let mut escaped = false; + for (i, ch) in s[start..].char_indices() { + let abs = start + i; + if escaped { + escaped = false; + continue; + } + if in_string { + if ch == '\\' { + escaped = true; + } else if ch == '"' { + in_string = false; + } + continue; + } + match ch { + '"' => in_string = true, + '{' => depth += 1, + '}' => { + depth -= 1; + if depth == 0 { + return Some(s[start..=abs].to_string()); + } + } + _ => {} + } + } + None +} + +#[allow(clippy::string_slice)] +fn truncate(s: &str, max: usize) -> String { + if s.len() <= max { + return s.to_string(); + } + // Find the largest char boundary at or before `max` so we never + // bisect a multi-byte UTF-8 sequence (this string is usually a model + // error / response excerpt). + let mut cut = max; + while cut > 0 && !s.is_char_boundary(cut) { + cut -= 1; + } + format!("{}…", &s[..cut]) +} + +/// Emit findings as JSONL (one object per line) to stdout, matching +/// the format the in-process path produces. Findings whose severity +/// ranks below `min_severity` are suppressed; this mirrors Amp's +/// behavior of hiding `low` from the review output by default. +pub fn emit_findings(findings: &[Finding], min_severity: Severity) -> usize { + let mut emitted = 0usize; + for f in findings { + if Severity::parse(&f.severity) < min_severity { + continue; + } + // serde_json::to_string never fails for these owned strings. + if let Ok(line) = serde_json::to_string(f) { + println!("{line}"); + emitted += 1; + } + } + emitted +} + +/// Severity floor for finding display. Mirrors Amp's CLI behavior of +/// hiding `low` by default; pass `--severity low` to surface them. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub enum Severity { + Low = 0, + Medium = 1, + High = 2, + Critical = 3, +} + +impl Severity { + /// Parse a severity string from a finding (`low`/`medium`/`high`/ + /// `critical`). Unrecognized strings are treated as `Medium` so + /// odd-but-non-trivial findings still surface. + pub fn parse(s: &str) -> Self { + match s.trim().to_ascii_lowercase().as_str() { + "low" | "info" | "note" => Severity::Low, + "high" => Severity::High, + "critical" | "crit" | "blocker" => Severity::Critical, + _ => Severity::Medium, + } + } +} + +impl std::str::FromStr for Severity { + type Err = String; + fn from_str(s: &str) -> Result { + match s.trim().to_ascii_lowercase().as_str() { + "low" => Ok(Severity::Low), + "medium" | "med" => Ok(Severity::Medium), + "high" => Ok(Severity::High), + "critical" => Ok(Severity::Critical), + other => Err(format!( + "unknown severity '{other}' (expected one of: low, medium, high, critical)" + )), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + fn ck(name: &str) -> Check { + Check { + name: name.to_string(), + description: Some("desc".into()), + model: None, + turn_limit: None, + tools: None, + severity_default: None, + path: PathBuf::from(format!("/.agents/checks/{name}.md")), + scope_dir: String::new(), + body: "look for bugs".into(), + } + } + + #[test] + fn check_prompt_is_strict_and_diff_aware() { + let p = build_check_prompt(&ck("perf"), "diff content", None); + assert!(p.contains("automated code review check")); + assert!(p.contains("Check name: perf")); + assert!(p.contains("```diff\ndiff content\n```")); + assert!(p.contains("Return ONLY valid JSON")); + assert!(p.contains("look for bugs")); + assert!(!p.contains("Reviewer instructions")); + } + + #[test] + fn check_prompt_restricts_findings_to_added_or_modified_lines() { + // Mirrors Amp's prompt language; without these the model + // happily flags pre-existing code shown for context. + let p = build_check_prompt(&ck("perf"), "diff content", None); + assert!(p.contains("ONLY in the changed lines")); + assert!(p.contains("lines beginning with `+`")); + assert!(p.contains("ONLY for code that was added or modified")); + assert!(p.contains("Do NOT report issues for unchanged")); + } + + #[test] + fn check_prompt_includes_reviewer_instructions_when_provided() { + let p = build_check_prompt( + &ck("perf"), + "diff content", + Some("This is a refactor; flag any behavior change."), + ); + assert!(p.contains("Reviewer instructions:")); + assert!(p.contains("flag any behavior change")); + } + + #[test] + fn check_prompt_skips_blank_reviewer_instructions() { + let p = build_check_prompt(&ck("perf"), "diff content", Some(" \n ")); + assert!(!p.contains("Reviewer instructions")); + } + + #[test] + fn parse_findings_accepts_bare_json() { + let raw = r#"{"findings":[{"severity":"high","path":"a.py","line_start":1,"line_end":2,"summary":"bad"}]}"#; + let f = parse_findings(raw).unwrap(); + assert_eq!(f.len(), 1); + assert_eq!(f[0].path.as_deref(), Some("a.py")); + } + + #[test] + fn parse_findings_strips_code_fences() { + let raw = "```json\n{\"findings\":[]}\n```"; + let f = parse_findings(raw).unwrap(); + assert!(f.is_empty()); + } + + #[test] + fn parse_findings_extracts_object_when_model_adds_chatter() { + let raw = + "Sure, here are the findings:\n{\"findings\":[]}\n\nLet me know if you need more."; + let f = parse_findings(raw).unwrap(); + assert!(f.is_empty()); + } + + #[test] + fn extract_json_object_respects_string_braces() { + let raw = r#"{"a": "value with } brace", "b": 1}"#; + let extracted = extract_json_object(raw).unwrap(); + assert_eq!(extracted, raw); + } + + #[test] + fn resolve_check_model_prefers_override() { + let check = ck("perf"); + let mut c = check.clone(); + c.model = Some("per-check".into()); + let opts = ReviewOptions { + override_model: Some("OVERRIDE".into()), + default_model: Some("default".into()), + ..ReviewOptions::default() + }; + assert_eq!(resolve_check_model(&c, &opts).as_deref(), Some("OVERRIDE")); + } + + #[test] + fn resolve_check_model_falls_through_to_per_check_then_default() { + let mut c = ck("perf"); + c.model = Some("per-check".into()); + let opts = ReviewOptions { + default_model: Some("default".into()), + ..ReviewOptions::default() + }; + assert_eq!(resolve_check_model(&c, &opts).as_deref(), Some("per-check")); + + let c = ck("perf"); + let opts = ReviewOptions { + default_model: Some("default".into()), + ..ReviewOptions::default() + }; + assert_eq!(resolve_check_model(&c, &opts).as_deref(), Some("default")); + } + + #[test] + fn severity_orders_correctly() { + assert!(Severity::Low < Severity::Medium); + assert!(Severity::Medium < Severity::High); + assert!(Severity::High < Severity::Critical); + } + + #[test] + fn severity_parse_from_finding_string_is_lenient() { + assert_eq!(Severity::parse("low"), Severity::Low); + assert_eq!(Severity::parse("LOW"), Severity::Low); + assert_eq!(Severity::parse("info"), Severity::Low); + assert_eq!(Severity::parse("note"), Severity::Low); + assert_eq!(Severity::parse("medium"), Severity::Medium); + assert_eq!(Severity::parse("high"), Severity::High); + assert_eq!(Severity::parse("critical"), Severity::Critical); + assert_eq!(Severity::parse("blocker"), Severity::Critical); + // Unknown strings default to Medium so they still surface. + assert_eq!(Severity::parse("weird"), Severity::Medium); + assert_eq!(Severity::parse(""), Severity::Medium); + } + + #[test] + fn severity_from_str_strict_for_cli() { + use std::str::FromStr; + assert_eq!(Severity::from_str("low").unwrap(), Severity::Low); + assert_eq!(Severity::from_str("MEDIUM").unwrap(), Severity::Medium); + assert_eq!(Severity::from_str("med").unwrap(), Severity::Medium); + assert_eq!(Severity::from_str("high").unwrap(), Severity::High); + assert_eq!(Severity::from_str("critical").unwrap(), Severity::Critical); + assert!(Severity::from_str("info").is_err()); + assert!(Severity::from_str("").is_err()); + } + + #[test] + fn resolve_check_model_cli_provider_wins_over_per_check_model() { + let mut c = ck("perf"); + c.model = Some("goose-claude-4-sonnet".into()); // wrong provider + let opts = ReviewOptions { + provider: Some("google".into()), + default_model: Some("gemini-3.1-pro-preview".into()), + ..ReviewOptions::default() + }; + assert_eq!( + resolve_check_model(&c, &opts).as_deref(), + Some("gemini-3.1-pro-preview") + ); + } + + #[test] + fn resolve_check_model_cli_provider_alone_drops_per_check_model() { + // `--provider google` without `--model`: a per-check model pinned + // to a Claude/Databricks model would 404 against Google. Drop it. + let mut c = ck("perf"); + c.model = Some("goose-claude-4-sonnet".into()); + let opts = ReviewOptions { + provider: Some("google".into()), + default_model: None, + ..ReviewOptions::default() + }; + assert_eq!(resolve_check_model(&c, &opts), None); + } + + #[test] + fn split_diff_by_file_separates_files_in_source_order() { + let diff = "\ +diff --git a/foo.rs b/foo.rs +index 1111..2222 100644 +--- a/foo.rs ++++ b/foo.rs +@@ -1 +1 @@ +-old foo ++new foo +diff --git a/bar/baz.go b/bar/baz.go +index 3333..4444 100644 +--- a/bar/baz.go ++++ b/bar/baz.go +@@ -10 +10 @@ +-old baz ++new baz +"; + let chunks = split_diff_by_file(diff); + assert_eq!(chunks.len(), 2); + assert_eq!(chunks[0].0, "foo.rs"); + assert!(chunks[0].1.starts_with("diff --git a/foo.rs b/foo.rs")); + assert!(chunks[0].1.contains("+new foo")); + // Each chunk must end at the next `diff --git` boundary, never + // leak into the following file's body. + assert!(!chunks[0].1.contains("baz.go")); + assert_eq!(chunks[1].0, "bar/baz.go"); + assert!(chunks[1].1.contains("+new baz")); + } + + #[test] + fn split_diff_by_file_handles_single_file() { + let diff = "\ +diff --git a/only.py b/only.py +index aaa..bbb 100644 +--- a/only.py ++++ b/only.py +@@ -1 +1 @@ +-x = 1 ++x = 2 +"; + let chunks = split_diff_by_file(diff); + assert_eq!(chunks.len(), 1); + assert_eq!(chunks[0].0, "only.py"); + } + + #[test] + fn split_diff_by_file_returns_empty_for_empty_input() { + assert!(split_diff_by_file("").is_empty()); + assert!(split_diff_by_file("\n").is_empty()); + } + + #[test] + fn split_diff_by_file_handles_quoted_headers_with_octal_escapes() { + // git emits `"a/dir/\303\251.txt" "b/dir/\303\251.txt"` for paths + // containing non-ASCII bytes when core.quotePath is on. Without + // quote handling the chunk gets dropped from the per-file pass. + let diff = "\ +diff --git \"a/dir/\\303\\251.txt\" \"b/dir/\\303\\251.txt\" +index 1111..2222 100644 +--- \"a/dir/\\303\\251.txt\" ++++ \"b/dir/\\303\\251.txt\" +@@ -1 +1 @@ +-old ++new +"; + let chunks = split_diff_by_file(diff); + assert_eq!(chunks.len(), 1); + assert_eq!(chunks[0].0, "dir/\u{e9}.txt"); + } + + #[test] + fn split_diff_by_file_handles_quoted_header_with_space() { + let diff = "\ +diff --git \"a/with space.txt\" \"b/with space.txt\" +index aaa..bbb 100644 +--- \"a/with space.txt\" ++++ \"b/with space.txt\" +@@ -1 +1 @@ +-old ++new +"; + let chunks = split_diff_by_file(diff); + assert_eq!(chunks.len(), 1); + assert_eq!(chunks[0].0, "with space.txt"); + } + + #[test] + fn split_diff_by_file_picks_post_image_path_for_renames() { + let diff = "\ +diff --git a/old/name.rs b/new/name.rs +similarity index 100% +rename from old/name.rs +rename to new/name.rs +"; + let chunks = split_diff_by_file(diff); + assert_eq!(chunks.len(), 1); + // We use the post-image path (b/) so the model references the + // file under its new name when emitting line numbers. + assert_eq!(chunks[0].0, "new/name.rs"); + } + + #[test] + fn main_pass_prompt_pins_file_and_demands_strict_json() { + let p = build_main_pass_prompt( + "src/foo.rs", + "diff --git a/src/foo.rs b/src/foo.rs\n@@ -1 +1 @@\n-old\n+new\n", + "BASE PROMPT", + None, + ); + assert!(p.starts_with("BASE PROMPT")); + assert!(p.contains("Path: `src/foo.rs`")); + assert!(p.contains("Walk every added/modified line")); + assert!(p.contains("\"findings\"")); + assert!(p.contains("Return ONLY valid JSON")); + assert!(p.contains("Do NOT include any text before or after the JSON")); + assert!(p.contains("```diff\ndiff --git a/src/foo.rs")); + assert!(!p.contains("Reviewer instructions")); + } + + #[test] + fn main_pass_prompt_includes_reviewer_instructions_when_provided() { + let p = build_main_pass_prompt( + "src/foo.rs", + "diff body", + "BASE", + Some("PR is a refactor; flag behavior changes."), + ); + assert!(p.contains("## Reviewer instructions")); + assert!(p.contains("flag behavior changes")); + } + + #[test] + fn main_pass_prompt_skips_blank_reviewer_instructions() { + let p = build_main_pass_prompt("src/foo.rs", "diff body", "BASE", Some(" \n \t\n")); + assert!(!p.contains("Reviewer instructions")); + } +} diff --git a/crates/goose-cli/src/commands/review/prompt.rs b/crates/goose-cli/src/commands/review/prompt.rs new file mode 100644 index 0000000000..3c518d271c --- /dev/null +++ b/crates/goose-cli/src/commands/review/prompt.rs @@ -0,0 +1,196 @@ +use std::fmt::Write; + +use goose::checks::{Check, DiscoveredReview}; + +/// The default review prompt embedded in the binary. +pub const DEFAULT_REVIEW_PROMPT: &str = include_str!("default_review_prompt.md"); + +/// Build the full prompt sent to the main review agent. +/// +/// Layout: +/// +/// ```text +/// +/// +/// ## Checks +/// +/// +/// ## Diff +/// ``` +/// +/// `base_prompt` is either the embedded [`DEFAULT_REVIEW_PROMPT`] or a +/// caller-supplied prompt loaded from `--prompt`. Findings derived from a +/// `**/.agents/REVIEW.md` file appear here as virtual `repo-rules`-prefixed +/// checks so the agent can attribute them via the `check` field on each +/// JSON finding. +pub fn build_review_prompt( + base_prompt: &str, + discovered: &DiscoveredReview, + diff: &str, + default_model: Option<&str>, + override_model: Option<&str>, + default_turn_limit: Option, +) -> String { + let mut out = String::new(); + out.push_str(base_prompt.trim_end()); + out.push_str("\n\n"); + + if !discovered.checks.is_empty() { + out.push_str("## Checks\n\n"); + out.push_str("Dispatch one subagent per check below. "); + out.push_str( + "Use the `model`, `turn_limit`, and `tools` columns when invoking each subagent. ", + ); + out.push_str("`tools = *` means the subagent inherits the agent's full toolset. "); + out.push_str( + "Set the `check` field on each finding to the check's `name` so the originating \ + rule can be identified in the output.\n\n", + ); + out.push_str( + "| name | scope | model | turn_limit | tools | severity_default | description |\n", + ); + out.push_str( + "|------|-------|-------|------------|-------|------------------|-------------|\n", + ); + for check in &discovered.checks { + let scope = if check.scope_dir.is_empty() { + "".to_string() + } else { + check.scope_dir.clone() + }; + let model = check + .resolved_model(default_model, override_model) + .unwrap_or(""); + let turn_limit = check.resolved_turn_limit(default_turn_limit); + let tools = match check.tools.as_ref() { + Some(t) if !t.is_empty() => t.join(", "), + _ => "*".to_string(), + }; + let severity = check.severity_default.as_deref().unwrap_or(""); + let description = check.description.as_deref().unwrap_or(""); + let _ = writeln!( + out, + "| {} | {} | {} | {} | {} | {} | {} |", + escape_pipe(&check.name), + escape_pipe(&scope), + escape_pipe(model), + turn_limit, + escape_pipe(&tools), + escape_pipe(severity), + escape_pipe(description), + ); + } + out.push('\n'); + for check in &discovered.checks { + append_check_body(&mut out, check); + } + } + + out.push_str("## Diff\n\n"); + out.push_str("```diff\n"); + out.push_str(diff.trim_end_matches('\n')); + out.push_str("\n```\n"); + out +} + +fn append_check_body(out: &mut String, check: &Check) { + let scope = if check.scope_dir.is_empty() { + "".to_string() + } else { + check.scope_dir.clone() + }; + let _ = writeln!(out, "### Check: {} (scope: {})", check.name, scope); + out.push_str(check.body.trim()); + out.push_str("\n\n"); +} + +fn escape_pipe(s: &str) -> String { + s.replace('|', "\\|") +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + fn check(name: &str, scope: &str, model: Option<&str>, turn_limit: Option) -> Check { + Check { + name: name.to_string(), + description: Some(format!("desc-{name}")), + model: model.map(str::to_string), + turn_limit, + tools: None, + severity_default: None, + path: PathBuf::from(format!("/r/{scope}/.agents/checks/{name}.md")), + scope_dir: scope.to_string(), + body: format!("body-{name}"), + } + } + + #[test] + fn renders_checks_with_resolved_model_and_turn_limit() { + let discovered = DiscoveredReview { + checks: vec![ + check("perf", "", None, None), + check("auth", "api", Some("m1"), Some(7)), + ], + }; + + let prompt = build_review_prompt( + "BASE", + &discovered, + "diff content", + Some("default-model"), + None, + Some(20), + ); + + assert!(prompt.contains("| auth | api | m1 | 7 | * |")); + assert!(prompt.contains("| perf | | default-model | 20 | * |")); + assert!(prompt.contains("body-auth")); + assert!(prompt.contains("```diff\ndiff content\n```")); + } + + #[test] + fn override_model_wins_per_check() { + let discovered = DiscoveredReview { + checks: vec![check("perf", "", Some("per-check"), None)], + }; + let prompt = build_review_prompt( + "BASE", + &discovered, + "", + Some("default"), + Some("OVERRIDE"), + None, + ); + assert!(prompt.contains("| perf | | OVERRIDE |")); + } + + #[test] + fn renders_tool_allowlist_and_severity_when_present() { + let mut perf = check("perf", "", None, None); + perf.tools = Some(vec!["read".to_string(), "grep".to_string()]); + perf.severity_default = Some("high".into()); + let discovered = DiscoveredReview { checks: vec![perf] }; + let prompt = build_review_prompt("BASE", &discovered, "", None, None, None); + assert!(prompt.contains("| perf | | | 25 | read, grep | high |")); + } + + #[test] + fn instructs_agent_to_attribute_findings_via_check_field() { + let discovered = DiscoveredReview { + checks: vec![check("perf", "", None, None)], + }; + let prompt = build_review_prompt("BASE", &discovered, "", None, None, None); + assert!(prompt.contains("Set the `check` field")); + } + + #[test] + fn omits_checks_section_when_empty() { + let discovered = DiscoveredReview::default(); + let prompt = build_review_prompt("BASE", &discovered, "diff", None, None, None); + assert!(!prompt.contains("## Checks")); + assert!(prompt.contains("## Diff")); + } +} diff --git a/crates/goose/src/checks/mod.rs b/crates/goose/src/checks/mod.rs new file mode 100644 index 0000000000..172ca72b51 --- /dev/null +++ b/crates/goose/src/checks/mod.rs @@ -0,0 +1,742 @@ +//! Discovery and parsing for review checks (`.agents/checks/*.md` and +//! `**/.agents/REVIEW.md`). Reuses the frontmatter parser exported by +//! [`crate::sources`] so all source-style files share one YAML pipeline. +//! +//! User-facing CRUD lives in `crate::sources` for parity with skills and +//! projects; `goose review` consumes [`Check`] and [`discover`] directly. + +use crate::sources::parse_frontmatter; +use anyhow::{anyhow, bail, Context, Result}; +use goose_sdk::custom_requests::{SourceEntry, SourceType}; +use serde::Deserialize; +use std::collections::{BTreeMap, HashMap}; +use std::fs; +use std::path::{Path, PathBuf}; + +/// Default maximum number of turns a check subagent may take. +/// +/// Mirrors `goose::agents::subagent_task_config::DEFAULT_SUBAGENT_MAX_TURNS`, +/// duplicated here to keep the checks module self-contained for parsing. +pub const DEFAULT_CHECK_TURN_LIMIT: usize = 25; + +/// Virtual check name prefix for `REVIEW.md`-derived checks. Findings emitted +/// by these checks should be attributed to the originating `REVIEW.md`. +pub const REVIEW_MD_CHECK_PREFIX: &str = "repo-rules"; + +/// Parsed YAML frontmatter for a check file. +#[derive(Debug, Deserialize, Default)] +struct CheckFrontmatter { + name: Option, + description: Option, + model: Option, + #[serde(rename = "turn-limit")] + turn_limit: Option, + tools: Option>, + #[serde(rename = "severity-default")] + severity_default: Option, +} + +/// A parsed check definition from `**/.agents/checks/*.md`. +/// +/// Each check is a Markdown file with YAML frontmatter and a body of +/// natural-language instructions for the subagent reviewer. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Check { + /// Identifier for the check. Must match the file's basename (without `.md`) + /// for repo-local checks; globals are allowed to drift for cross-tool + /// compatibility. + pub name: String, + /// Brief description shown when listing checks. + pub description: Option, + /// Per-check model override. Resolved against CLI flags by [`Check::resolved_model`]. + pub model: Option, + /// Per-check turn limit. Resolved against the global default by + /// [`Check::resolved_turn_limit`]. + pub turn_limit: Option, + /// Optional allowlist of tool names the check subagent may call. + /// `None` (the default) means the subagent inherits the agent's full + /// toolset. Mirrors Amp's `tools:` field for parity with + /// `.agents/checks/*.md`. + pub tools: Option>, + /// Optional default severity for findings emitted by this check + /// (`low`, `medium`, `high`, `critical`). Recognized for parity with + /// Amp's `severity-default:` field. + pub severity_default: Option, + /// Absolute path to the check file on disk. + pub path: PathBuf, + /// Repo-relative scope directory the check applies to. Empty string means + /// the repo root or a global location. + pub scope_dir: String, + /// Markdown content after the closing `---`. + pub body: String, +} + +impl Check { + /// Read and parse a check file from disk. + pub fn from_path(path: &Path) -> Result { + let content = fs::read_to_string(path) + .with_context(|| format!("read check file: {}", path.display()))?; + Self::parse(&content, path) + } + + /// Parse a check from raw content. Reuses [`crate::sources::parse_frontmatter`] + /// so checks share the same YAML pipeline as skills and projects. + pub fn parse(content: &str, path: &Path) -> Result { + let trimmed = content.trim_start(); + if !trimmed.starts_with("---") { + bail!( + "check {}: missing frontmatter (must start with ---)", + path.display() + ); + } + + // Tolerate CRLF line endings before delegating to the shared splitter. + let normalized = trimmed.replace("\r\n", "\n"); + let (frontmatter, body) = match parse_frontmatter::(&normalized) { + Ok(Some(parsed)) => parsed, + Ok(None) => bail!( + "check {}: missing closing --- in frontmatter", + path.display() + ), + Err(e) => { + return Err(anyhow!(e)) + .with_context(|| format!("check {}: invalid frontmatter YAML", path.display())) + } + }; + + let file_stem = path + .file_stem() + .and_then(|s| s.to_str()) + .ok_or_else(|| anyhow!("check {}: invalid filename", path.display()))? + .to_string(); + + let name = match frontmatter.name { + Some(declared) if !declared.is_empty() => declared, + _ => file_stem, + }; + + Ok(Check { + name, + description: frontmatter.description, + model: frontmatter.model, + turn_limit: frontmatter.turn_limit, + tools: frontmatter.tools, + severity_default: frontmatter.severity_default, + path: path.to_path_buf(), + scope_dir: String::new(), + body, + }) + } + + /// Verify the check's `name` matches its filename stem (e.g. `perf` for + /// `perf.md`). Repo-local checks are required to satisfy this rule so + /// authors get a clear error early; checks loaded from global directories + /// are allowed to drift for compatibility with cross-tool conventions. + pub fn validate_name_matches_filename(&self) -> Result<()> { + let stem = self + .path + .file_stem() + .and_then(|s| s.to_str()) + .ok_or_else(|| anyhow!("check {}: invalid filename", self.path.display()))?; + if self.name != stem { + bail!( + "check {}: name '{}' must match filename '{}'", + self.path.display(), + self.name, + stem + ); + } + Ok(()) + } + + /// Resolve which model this check should use. + /// + /// `override_model` (CLI `--override-model`) wins over everything; otherwise + /// the per-check `model` wins; otherwise `default_model` (CLI `--model`). + pub fn resolved_model<'a>( + &'a self, + default_model: Option<&'a str>, + override_model: Option<&'a str>, + ) -> Option<&'a str> { + if let Some(m) = override_model { + return Some(m); + } + if let Some(m) = self.model.as_deref() { + return Some(m); + } + default_model + } + + /// Resolve the turn limit for this check. + pub fn resolved_turn_limit(&self, default_turn_limit: Option) -> usize { + self.turn_limit + .or(default_turn_limit) + .unwrap_or(DEFAULT_CHECK_TURN_LIMIT) + } + + /// Render this check as a generic [`SourceEntry`] so it can flow through + /// the same listing/UI pipeline as skills and projects. Checks surface as + /// [`SourceType::Agent`] entries — they're sub-agent definitions + /// specialized for code review — with `properties["kind"] = "check"` + /// so clients can distinguish them from `.agents/agents/*.md` agents. + /// Per-check tunables (`model`, `turn-limit`, `tools`, `severity-default`, + /// `scope_dir`) live in `properties` so the SDK schema doesn't need + /// check-specific fields. + pub fn to_source_entry(&self, global: bool) -> SourceEntry { + let mut properties: HashMap = HashMap::new(); + properties.insert("kind".into(), serde_json::Value::String("check".into())); + if let Some(m) = &self.model { + properties.insert("model".into(), serde_json::Value::String(m.clone())); + } + if let Some(n) = self.turn_limit { + properties.insert("turnLimit".into(), serde_json::Value::from(n)); + } + if let Some(t) = &self.tools { + properties.insert( + "tools".into(), + serde_json::Value::Array( + t.iter() + .map(|s| serde_json::Value::String(s.clone())) + .collect(), + ), + ); + } + if let Some(sev) = &self.severity_default { + properties.insert( + "severityDefault".into(), + serde_json::Value::String(sev.clone()), + ); + } + if !self.scope_dir.is_empty() { + properties.insert( + "scopeDir".into(), + serde_json::Value::String(self.scope_dir.clone()), + ); + } + SourceEntry { + source_type: SourceType::Agent, + name: self.name.clone(), + description: self.description.clone().unwrap_or_default(), + content: self.body.clone(), + path: self.path.to_string_lossy().into_owned(), + global, + writable: true, + supporting_files: Vec::new(), + properties, + } + } +} + +/// Discovered set of checks for a review request. +/// +/// `checks` contains both author-defined checks (from `.agents/checks/*.md`) +/// and virtual checks auto-derived from `**/.agents/REVIEW.md` files so their +/// findings can be attributed back to that file. +#[derive(Debug, Default, Clone)] +pub struct DiscoveredReview { + /// All applicable checks, sorted by name. Closer scopes shadow same-named + /// checks from broader scopes (project root, then home/global). + pub checks: Vec, +} + +fn review_md_virtual_check_name(scope_dir: &str) -> String { + if scope_dir.is_empty() { + REVIEW_MD_CHECK_PREFIX.to_string() + } else { + format!("{REVIEW_MD_CHECK_PREFIX}:{scope_dir}") + } +} + +fn synthesize_review_md_check(scope_dir: &str, path: &Path, body: &str) -> Check { + let scope_label = if scope_dir.is_empty() { + "the entire repository".to_string() + } else { + format!("files under `{scope_dir}/`") + }; + let intro = format!( + "You are enforcing the project's `REVIEW.md` rules for {scope_label}.\n\n\ + These rules were authored in `{}`. Apply them strictly to the diff.\n\n\ + ---\n\n", + path.display(), + ); + Check { + name: review_md_virtual_check_name(scope_dir), + description: Some(format!("Auto-derived from {}", path.display())), + model: None, + turn_limit: None, + tools: None, + severity_default: None, + path: path.to_path_buf(), + scope_dir: scope_dir.to_string(), + body: format!("{intro}{}", body.trim()), + } +} + +/// Locations searched for global checks, in priority order. +/// +/// The first existing directory wins for a given check name; closer scopes +/// (repo root, then sub-trees) shadow these globals when names collide. +pub fn global_checks_dirs() -> Vec { + let mut dirs = Vec::new(); + if let Some(home) = dirs_home() { + dirs.push(home.join(".config").join("goose").join("checks")); + dirs.push(home.join(".config").join("agents").join("checks")); + } + dirs +} + +fn dirs_home() -> Option { + std::env::var_os("HOME") + .map(PathBuf::from) + .or_else(|| std::env::var_os("USERPROFILE").map(PathBuf::from)) +} + +/// Discover all checks and REVIEW.md files relevant to `touched_files`. +/// +/// `repo_root` is the absolute path to the repository root; `touched_files` is +/// the list of repo-relative file paths changed in the diff. When +/// `touched_files` is empty, only repo-root and global locations are +/// considered. +pub fn discover(repo_root: &Path, touched_files: &[String]) -> Result { + discover_with_globals(repo_root, touched_files, &global_checks_dirs()) +} + +/// Discover checks against an explicit set of global directories. Exposed for +/// tests and for callers that want to override the global search path. +pub fn discover_with_globals( + repo_root: &Path, + touched_files: &[String], + global_dirs: &[PathBuf], +) -> Result { + let scope_dirs = candidate_scope_dirs(touched_files); + + // Collect raw checks keyed by name with explicit per-source priority so + // closer/more-specific sources shadow broader ones. Globals get priority + // 0 so a repo-root check (priority 1) of the same name always wins. + let mut by_name: BTreeMap = BTreeMap::new(); + let mut record = |check: Check, priority: usize| { + by_name + .entry(check.name.clone()) + .and_modify(|(existing_priority, existing)| { + if priority > *existing_priority { + *existing = check.clone(); + *existing_priority = priority; + } + }) + .or_insert((priority, check)); + }; + + for dir in global_dirs { + for check in read_checks_dir(dir, "", LoadMode::Lenient)? { + record(check, 0); + } + } + + let root_dir = repo_root.join(".agents").join("checks"); + for check in read_checks_dir(&root_dir, "", LoadMode::Strict)? { + record(check, scope_priority("")); + } + + for scope in &scope_dirs { + let dir = repo_root.join(scope).join(".agents").join("checks"); + for check in read_checks_dir(&dir, scope, LoadMode::Strict)? { + let p = scope_priority(scope); + record(check, p); + } + } + + let root_review = repo_root.join(".agents").join("REVIEW.md"); + if root_review.is_file() { + let body = fs::read_to_string(&root_review) + .with_context(|| format!("read REVIEW.md {}", root_review.display()))?; + let check = synthesize_review_md_check("", &root_review, &body); + record(check, scope_priority("")); + } + for scope in &scope_dirs { + let path = repo_root.join(scope).join(".agents").join("REVIEW.md"); + if path.is_file() { + let body = fs::read_to_string(&path) + .with_context(|| format!("read REVIEW.md {}", path.display()))?; + let check = synthesize_review_md_check(scope, &path, &body); + let p = scope_priority(scope); + record(check, p); + } + } + + let mut checks: Vec = by_name.into_values().map(|(_, c)| c).collect(); + checks.sort_by(|a, b| a.name.cmp(&b.name)); + + Ok(DiscoveredReview { checks }) +} + +/// Whether parse errors in a check directory should fail the run or be skipped +/// with a warning. Repo-local checks are loaded `Strict` because authors should +/// see their own broken frontmatter; global directories are loaded `Lenient` +/// because they often contain `README.md` and similar non-check Markdown. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum LoadMode { + Strict, + Lenient, +} + +fn read_checks_dir(dir: &Path, scope_dir: &str, mode: LoadMode) -> Result> { + if !dir.is_dir() { + return Ok(Vec::new()); + } + let mut out = Vec::new(); + let entries = + fs::read_dir(dir).with_context(|| format!("read checks dir {}", dir.display()))?; + for entry in entries { + let entry = entry?; + let path = entry.path(); + if !path.is_file() { + continue; + } + if path.extension().and_then(|s| s.to_str()) != Some("md") { + continue; + } + // README.md is conventionally the directory's docs, not a check. + if path.file_name().and_then(|s| s.to_str()) == Some("README.md") { + continue; + } + let parsed = Check::from_path(&path).and_then(|mut check| { + if mode == LoadMode::Strict { + check.validate_name_matches_filename()?; + } + check.scope_dir = scope_dir.to_string(); + Ok(check) + }); + match parsed { + Ok(check) => out.push(check), + Err(e) => match mode { + LoadMode::Strict => return Err(e), + LoadMode::Lenient => { + eprintln!("goose review: skipping {}: {e}", path.display()); + } + }, + } + } + out.sort_by(|a, b| a.name.cmp(&b.name)); + Ok(out) +} + +/// Priority of a scope for shadowing checks/overrides. Higher = closer. +fn scope_priority(scope_dir: &str) -> usize { + if scope_dir.is_empty() { + 1 + } else { + 2 + scope_dir.split('/').count() + } +} + +/// Compute the set of in-repo scope directories whose `.agents/` may contain +/// checks or REVIEW.md applicable to the touched files. +/// +/// For `api/v2/foo.rs` this yields `["api", "api/v2"]`. +pub fn candidate_scope_dirs(touched_files: &[String]) -> Vec { + let mut seen = std::collections::BTreeSet::new(); + for file in touched_files { + let normalized = file.replace('\\', "/"); + let dir = Path::new(&normalized).parent(); + let Some(dir) = dir else { continue }; + let parts: Vec<&str> = dir + .to_str() + .unwrap_or_default() + .split('/') + .filter(|p| !p.is_empty() && *p != ".") + .collect(); + for i in 1..=parts.len() { + seen.insert(parts[..i].join("/")); + } + } + seen.into_iter().collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + use tempfile::tempdir; + + fn p(s: &str) -> PathBuf { + PathBuf::from(s) + } + + fn write(path: &Path, contents: &str) { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).unwrap(); + } + fs::write(path, contents).unwrap(); + } + + #[test] + fn parses_full_frontmatter() { + let content = r#"--- +name: perf +description: Flag perf regressions +model: claude-sonnet-4 +turn-limit: 40 +tools: + - read + - grep +--- +Look for N+1 queries. +"#; + let check = Check::parse(content, &p("/r/.agents/checks/perf.md")).unwrap(); + assert_eq!(check.name, "perf"); + assert_eq!(check.description.as_deref(), Some("Flag perf regressions")); + assert_eq!(check.model.as_deref(), Some("claude-sonnet-4")); + assert_eq!(check.turn_limit, Some(40)); + assert_eq!( + check.tools.as_deref(), + Some(["read".to_string(), "grep".to_string()].as_slice()) + ); + assert_eq!(check.body, "Look for N+1 queries."); + } + + #[test] + fn tools_field_is_optional_for_backward_compatibility() { + let content = "---\nname: legacy\n---\nbody"; + let check = Check::parse(content, &p("/r/.agents/checks/legacy.md")).unwrap(); + assert!(check.tools.is_none()); + assert!(check.severity_default.is_none()); + } + + #[test] + fn parses_extended_frontmatter() { + let content = r#"--- +name: untrusted-pr +description: Reviews PRs as untrusted input +severity-default: high +tools: [Bash, Read, Grep] +--- + +## Purpose +"#; + let check = Check::parse(content, &p("/r/.agents/checks/untrusted-pr.md")).unwrap(); + assert_eq!(check.name, "untrusted-pr"); + assert_eq!(check.severity_default.as_deref(), Some("high")); + assert_eq!( + check.tools.as_deref(), + Some(["Bash".to_string(), "Read".to_string(), "Grep".to_string()].as_slice()) + ); + assert!(check.model.is_none()); + assert!(check.turn_limit.is_none()); + } + + #[test] + fn defaults_name_to_filename_stem() { + let content = "---\ndescription: foo\n---\nbody"; + let check = Check::parse(content, &p("/r/.agents/checks/sql-safety.md")).unwrap(); + assert_eq!(check.name, "sql-safety"); + } + + #[test] + fn parse_keeps_declared_name_for_global_compat() { + let content = "---\nname: meta-review\n---\nbody"; + let check = Check::parse(content, &p("/global/checks/review.md")).unwrap(); + assert_eq!(check.name, "meta-review"); + } + + #[test] + fn validate_name_matches_filename_rejects_mismatch() { + let content = "---\nname: other\n---\nbody"; + let check = Check::parse(content, &p("/r/.agents/checks/perf.md")).unwrap(); + let err = check.validate_name_matches_filename().unwrap_err(); + assert!(err.to_string().contains("must match filename")); + } + + #[test] + fn validate_name_matches_filename_accepts_match() { + let content = "---\nname: perf\n---\nbody"; + let check = Check::parse(content, &p("/r/.agents/checks/perf.md")).unwrap(); + check.validate_name_matches_filename().unwrap(); + } + + #[test] + fn rejects_missing_frontmatter() { + let err = Check::parse("just a body", &p("/r/.agents/checks/x.md")).unwrap_err(); + assert!(err.to_string().contains("missing frontmatter")); + } + + #[test] + fn rejects_unclosed_frontmatter() { + let err = Check::parse("---\nname: x\nno close", &p("/r/.agents/checks/x.md")).unwrap_err(); + assert!(err.to_string().contains("missing closing")); + } + + #[test] + fn parses_crlf_frontmatter() { + let content = "---\r\nname: perf\r\ndescription: ok\r\n---\r\nbody line\r\n"; + let check = Check::parse(content, &p("/r/.agents/checks/perf.md")).unwrap(); + assert_eq!(check.name, "perf"); + assert_eq!(check.description.as_deref(), Some("ok")); + assert_eq!(check.body, "body line"); + } + + #[test] + fn resolves_model_precedence() { + let mut check = Check::parse( + "---\nname: x\nmodel: per-check\n---\n", + &p("/r/.agents/checks/x.md"), + ) + .unwrap(); + assert_eq!( + check.resolved_model(Some("default"), None), + Some("per-check") + ); + assert_eq!( + check.resolved_model(Some("default"), Some("override")), + Some("override") + ); + check.model = None; + assert_eq!(check.resolved_model(Some("default"), None), Some("default")); + assert_eq!(check.resolved_model(None, None), None); + } + + #[test] + fn resolves_turn_limit() { + let mut check = Check::parse( + "---\nname: x\nturn-limit: 7\n---\n", + &p("/r/.agents/checks/x.md"), + ) + .unwrap(); + assert_eq!(check.resolved_turn_limit(None), 7); + assert_eq!(check.resolved_turn_limit(Some(99)), 7); + check.turn_limit = None; + assert_eq!(check.resolved_turn_limit(Some(99)), 99); + assert_eq!(check.resolved_turn_limit(None), DEFAULT_CHECK_TURN_LIMIT); + } + + #[test] + fn candidate_scopes_walks_parents() { + let scopes = candidate_scope_dirs(&[ + "api/v2/foo.rs".into(), + "api/v2/bar.rs".into(), + "README.md".into(), + ]); + assert_eq!(scopes, vec!["api".to_string(), "api/v2".to_string()]); + } + + #[test] + fn discovers_root_and_scoped_checks() { + let dir = tempdir().unwrap(); + let root = dir.path(); + write( + &root.join(".agents/checks/sql.md"), + "---\nname: sql\ndescription: root sql\n---\nbody", + ); + write( + &root.join("api/.agents/checks/auth.md"), + "---\nname: auth\n---\nauth body", + ); + + let result = discover_with_globals(root, &["api/users.rs".to_string()], &[]).unwrap(); + let names: Vec<_> = result.checks.iter().map(|c| c.name.as_str()).collect(); + assert_eq!(names, vec!["auth", "sql"]); + } + + #[test] + fn closer_scope_overrides_same_named_check() { + let dir = tempdir().unwrap(); + let root = dir.path(); + write( + &root.join(".agents/checks/perf.md"), + "---\nname: perf\ndescription: root\n---\nroot body", + ); + write( + &root.join("api/.agents/checks/perf.md"), + "---\nname: perf\ndescription: scoped\n---\nscoped body", + ); + + let result = discover_with_globals(root, &["api/users.rs".to_string()], &[]).unwrap(); + assert_eq!(result.checks.len(), 1); + let perf = &result.checks[0]; + assert_eq!(perf.scope_dir, "api"); + assert_eq!(perf.body, "scoped body"); + } + + #[test] + fn synthesizes_virtual_checks_for_review_md_at_each_scope() { + let dir = tempdir().unwrap(); + let root = dir.path(); + write(&root.join(".agents/REVIEW.md"), "root rules"); + write(&root.join("api/.agents/REVIEW.md"), "api rules"); + write(&root.join("api/v2/.agents/REVIEW.md"), "v2 rules"); + + let result = discover_with_globals(root, &["api/v2/x.rs".to_string()], &[]).unwrap(); + let names: Vec<_> = result.checks.iter().map(|c| c.name.as_str()).collect(); + assert_eq!( + names, + vec!["repo-rules", "repo-rules:api", "repo-rules:api/v2"] + ); + + let root_check = result + .checks + .iter() + .find(|c| c.name == "repo-rules") + .unwrap(); + assert!(root_check.body.contains("the entire repository")); + assert!(root_check.body.contains("root rules")); + + let scoped = result + .checks + .iter() + .find(|c| c.name == "repo-rules:api/v2") + .unwrap(); + assert!(scoped.body.contains("files under `api/v2/`")); + assert!(scoped.body.contains("v2 rules")); + } + + #[test] + fn repo_root_check_overrides_same_named_global_check() { + let dir = tempdir().unwrap(); + let root = dir.path(); + let global = tempdir().unwrap(); + write( + &global.path().join("perf.md"), + "---\nname: perf\ndescription: global\n---\nglobal body", + ); + write( + &root.join(".agents/checks/perf.md"), + "---\nname: perf\ndescription: repo\n---\nrepo body", + ); + + let result = discover_with_globals(root, &[], &[global.path().to_path_buf()]).unwrap(); + assert_eq!(result.checks.len(), 1); + assert_eq!(result.checks[0].body, "repo body"); + assert_eq!(result.checks[0].description.as_deref(), Some("repo")); + } + + #[test] + fn user_check_named_repo_rules_is_not_overwritten_by_root_review_md() { + let dir = tempdir().unwrap(); + let root = dir.path(); + write( + &root.join(".agents/checks/repo-rules.md"), + "---\nname: repo-rules\ndescription: user\n---\nuser body", + ); + write(&root.join(".agents/REVIEW.md"), "review rules"); + + let result = discover_with_globals(root, &[], &[]).unwrap(); + let repo_rules = result + .checks + .iter() + .find(|c| c.name == "repo-rules") + .expect("repo-rules check should exist"); + assert_eq!(repo_rules.body, "user body"); + } + + #[test] + fn skips_non_markdown_in_checks_dir() { + let dir = tempdir().unwrap(); + let root = dir.path(); + write(&root.join(".agents/checks/README.txt"), "ignored"); + write( + &root.join(".agents/checks/perf.md"), + "---\nname: perf\n---\nbody", + ); + let result = discover_with_globals(root, &[], &[]).unwrap(); + assert_eq!(result.checks.len(), 1); + } +} diff --git a/crates/goose/src/lib.rs b/crates/goose/src/lib.rs index e1897cb84c..02696312c9 100644 --- a/crates/goose/src/lib.rs +++ b/crates/goose/src/lib.rs @@ -9,6 +9,7 @@ pub use goose_sdk::custom_requests; pub mod action_required_manager; pub mod agents; pub mod builtin_extension; +pub mod checks; pub mod config; pub mod context_mgmt; pub mod conversation; diff --git a/crates/goose/src/session/session_manager.rs b/crates/goose/src/session/session_manager.rs index 954a561f1a..33d6562f9e 100644 --- a/crates/goose/src/session/session_manager.rs +++ b/crates/goose/src/session/session_manager.rs @@ -642,25 +642,39 @@ impl SessionStorage { } async fn create_schema(pool: &Pool) -> Result<()> { + // Run schema creation under `BEGIN IMMEDIATE` so SQLite serializes + // writers across processes. Combined with `IF NOT EXISTS` on every + // DDL statement and `INSERT OR IGNORE` on the bootstrap version + // row, this makes init safe under concurrent first-run startup — + // the previous flow: + // + // SELECT EXISTS('schema_version') → false + // CREATE TABLE schema_version (...) + // + // raced when two processes both saw "doesn't exist" and the + // second one's CREATE TABLE failed with `table already exists`, + // which surfaced to callers as "Could not create session". + let mut tx = pool.begin_with("BEGIN IMMEDIATE").await?; + sqlx::query( r#" - CREATE TABLE schema_version ( + CREATE TABLE IF NOT EXISTS schema_version ( version INTEGER PRIMARY KEY, applied_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ) "#, ) - .execute(pool) + .execute(&mut *tx) .await?; - sqlx::query("INSERT INTO schema_version (version) VALUES (?)") + sqlx::query("INSERT OR IGNORE INTO schema_version (version) VALUES (?)") .bind(CURRENT_SCHEMA_VERSION) - .execute(pool) + .execute(&mut *tx) .await?; sqlx::query( r#" - CREATE TABLE sessions ( + CREATE TABLE IF NOT EXISTS sessions ( id TEXT PRIMARY KEY, name TEXT NOT NULL DEFAULT '', description TEXT NOT NULL DEFAULT '', @@ -688,12 +702,12 @@ impl SessionStorage { ) "#, ) - .execute(pool) + .execute(&mut *tx) .await?; sqlx::query( r#" - CREATE TABLE messages ( + CREATE TABLE IF NOT EXISTS messages ( id INTEGER PRIMARY KEY AUTOINCREMENT, message_id TEXT, session_id TEXT NOT NULL REFERENCES sessions(id), @@ -706,24 +720,30 @@ impl SessionStorage { ) "#, ) - .execute(pool) + .execute(&mut *tx) .await?; - sqlx::query("CREATE INDEX idx_messages_session ON messages(session_id)") - .execute(pool) + sqlx::query("CREATE INDEX IF NOT EXISTS idx_messages_session ON messages(session_id)") + .execute(&mut *tx) .await?; - sqlx::query("CREATE INDEX idx_messages_timestamp ON messages(timestamp)") - .execute(pool) + sqlx::query("CREATE INDEX IF NOT EXISTS idx_messages_timestamp ON messages(timestamp)") + .execute(&mut *tx) .await?; - sqlx::query("CREATE INDEX idx_messages_message_id ON messages(message_id)") - .execute(pool) + sqlx::query("CREATE INDEX IF NOT EXISTS idx_messages_message_id ON messages(message_id)") + .execute(&mut *tx) .await?; - sqlx::query("CREATE INDEX idx_sessions_updated ON sessions(updated_at DESC)") - .execute(pool) + sqlx::query("CREATE INDEX IF NOT EXISTS idx_sessions_updated ON sessions(updated_at DESC)") + .execute(&mut *tx) .await?; - sqlx::query("CREATE INDEX idx_sessions_type ON sessions(session_type)") - .execute(pool) + sqlx::query("CREATE INDEX IF NOT EXISTS idx_sessions_type ON sessions(session_type)") + .execute(&mut *tx) .await?; + + tx.commit().await?; + + // The inventory tables already use `CREATE TABLE IF NOT EXISTS` + // and run on the shared pool, so they don't need to be inside + // the same transaction. crate::providers::inventory::create_tables(pool).await?; Ok(()) diff --git a/crates/goose/src/sources.rs b/crates/goose/src/sources.rs index 224058e587..9ff2cd1f1c 100644 --- a/crates/goose/src/sources.rs +++ b/crates/goose/src/sources.rs @@ -937,6 +937,31 @@ pub fn list_sources_with_roots( } SourceType::Agent => { sources.extend(list_agent_sources(project_dir, additional_roots)); + + // Surface `.agents/checks/*.md` review checks under the same + // `Agent` source type. They live at a different path on disk + // (Amp-compatible `.agents/checks/`) but are conceptually + // agents: a check is a sub-agent definition specialized for + // code review. `properties["kind"] = "check"` lets clients + // differentiate. + let working_dir = project_dir + .map(str::trim) + .filter(|p| !p.is_empty()) + .map(PathBuf::from); + let discovered = match working_dir.as_deref() { + Some(root) => crate::checks::discover(root, &[]) + .map_err(|e| Error::internal_error().data(e.to_string()))?, + None => crate::checks::DiscoveredReview::default(), + }; + for check in discovered.checks { + let global = check.path.starts_with( + crate::checks::global_checks_dirs() + .first() + .map(PathBuf::as_path) + .unwrap_or_else(|| Path::new("")), + ); + sources.push(check.to_source_entry(global)); + } } SourceType::Recipe | SourceType::Subrecipe => { return Err(Error::invalid_params() @@ -1751,6 +1776,50 @@ mod tests { assert!(exported.0.contains("preferred")); } + #[test] + fn list_agent_sources_includes_review_checks_with_kind_check() { + let tmp = TempDir::new().unwrap(); + let checks_dir = tmp.path().join(".agents").join("checks"); + std::fs::create_dir_all(&checks_dir).unwrap(); + std::fs::write( + checks_dir.join("perf.md"), + "---\nname: perf\ndescription: Flag perf regressions\nmodel: claude-sonnet-4\nturn-limit: 40\ntools: [Read, Grep]\nseverity-default: high\n---\nLook for N+1 queries.", + ) + .unwrap(); + + let listed = list_sources( + Some(SourceType::Agent), + Some(tmp.path().to_str().unwrap()), + false, + ) + .unwrap(); + + let check = listed + .iter() + .find(|s| s.name == "perf") + .expect("perf check should appear in Agent listing"); + assert_eq!(check.source_type, SourceType::Agent); + assert_eq!( + check.properties.get("kind").and_then(|v| v.as_str()), + Some("check") + ); + assert_eq!( + check.properties.get("model").and_then(|v| v.as_str()), + Some("claude-sonnet-4") + ); + assert_eq!( + check.properties.get("turnLimit").and_then(|v| v.as_u64()), + Some(40) + ); + assert_eq!( + check + .properties + .get("severityDefault") + .and_then(|v| v.as_str()), + Some("high") + ); + } + #[test] fn update_rejects_path_traversal() { let tmp = TempDir::new().unwrap();