mirror of
https://github.com/aaif-goose/goose.git
synced 2026-07-03 14:10:03 +02:00
feat(cli): add goose review local code review command (#9114)
Signed-off-by: Joah Gerstenberg <joahg@squareup.com> Signed-off-by: Joah Gerstenberg <joah@squareup.com> Co-authored-by: Joah Gerstenberg <joahg@squareup.com> Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
@@ -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<String>,
|
||||
|
||||
/// Path to a Markdown file with a custom base review prompt. Replaces
|
||||
/// the embedded default prompt.
|
||||
#[arg(long = "prompt", value_name = "FILE")]
|
||||
prompt: Option<PathBuf>,
|
||||
|
||||
/// 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<String>,
|
||||
|
||||
/// Provider for the main review agent.
|
||||
#[arg(long = "provider", value_name = "PROVIDER")]
|
||||
provider: Option<String>,
|
||||
|
||||
/// 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<String>,
|
||||
|
||||
/// Default `turn-limit` applied to checks that do not declare their
|
||||
/// own.
|
||||
#[arg(long = "turn-limit", value_name = "N")]
|
||||
turn_limit: Option<usize>,
|
||||
|
||||
/// 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<String>,
|
||||
|
||||
/// 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<String>,
|
||||
|
||||
/// 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<String>,
|
||||
|
||||
/// 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<PathBuf>,
|
||||
|
||||
/// 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<Command>) -> &'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) {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 = <check body>,
|
||||
async = true, # IMPORTANT: parallelize
|
||||
model = <check model>,
|
||||
max_turns = <check turn_limit>,
|
||||
)
|
||||
```
|
||||
|
||||
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.
|
||||
@@ -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<String>,
|
||||
/// Path to a markdown file with a custom base review prompt. Overrides the
|
||||
/// embedded default prompt entirely.
|
||||
pub prompt_file: Option<PathBuf>,
|
||||
/// Default model used for the main review agent and for any check that
|
||||
/// does not declare its own `model:`.
|
||||
pub default_model: Option<String>,
|
||||
/// Provider for the main review agent.
|
||||
pub provider: Option<String>,
|
||||
/// Force every discovered check to run with this model, regardless of
|
||||
/// the check's own `model:` field.
|
||||
pub override_model: Option<String>,
|
||||
/// Default `turn-limit` applied to checks that do not declare their own.
|
||||
pub default_turn_limit: Option<usize>,
|
||||
/// 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<String>,
|
||||
/// 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<String>,
|
||||
/// Only run checks whose `name` is in this list. Empty means run all
|
||||
/// discovered checks (the default).
|
||||
pub check_filter: Vec<String>,
|
||||
/// Alternate directory to search for `.agents/checks/*.md` instead of
|
||||
/// the repo root.
|
||||
pub check_scope: Option<PathBuf>,
|
||||
/// 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 `<scope>/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 <text>` 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() {
|
||||
"<root>"
|
||||
} else {
|
||||
&c.scope_dir
|
||||
};
|
||||
eprintln!(" - {} (scope: {})", c.name, scope);
|
||||
}
|
||||
}
|
||||
|
||||
fn find_repo_root() -> Result<PathBuf> {
|
||||
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<Vec<String>> {
|
||||
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<String> {
|
||||
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<String> {
|
||||
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<Vec<String>> {
|
||||
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<String> {
|
||||
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 `<scope>/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<String> {
|
||||
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);
|
||||
}
|
||||
}
|
||||
@@ -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};
|
||||
File diff suppressed because it is too large
Load Diff
@@ -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
|
||||
/// <base prompt>
|
||||
///
|
||||
/// ## Checks
|
||||
/// <check table + per-check bodies>
|
||||
///
|
||||
/// ## 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<usize>,
|
||||
) -> 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() {
|
||||
"<root>".to_string()
|
||||
} else {
|
||||
check.scope_dir.clone()
|
||||
};
|
||||
let model = check
|
||||
.resolved_model(default_model, override_model)
|
||||
.unwrap_or("<agent default>");
|
||||
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() {
|
||||
"<root>".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<usize>) -> 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 | <root> | 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 | <root> | 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 | <root> | <agent default> | 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"));
|
||||
}
|
||||
}
|
||||
@@ -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<String>,
|
||||
description: Option<String>,
|
||||
model: Option<String>,
|
||||
#[serde(rename = "turn-limit")]
|
||||
turn_limit: Option<usize>,
|
||||
tools: Option<Vec<String>>,
|
||||
#[serde(rename = "severity-default")]
|
||||
severity_default: Option<String>,
|
||||
}
|
||||
|
||||
/// 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<String>,
|
||||
/// Per-check model override. Resolved against CLI flags by [`Check::resolved_model`].
|
||||
pub model: Option<String>,
|
||||
/// Per-check turn limit. Resolved against the global default by
|
||||
/// [`Check::resolved_turn_limit`].
|
||||
pub turn_limit: Option<usize>,
|
||||
/// 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<Vec<String>>,
|
||||
/// 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<String>,
|
||||
/// 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<Self> {
|
||||
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<Self> {
|
||||
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::<CheckFrontmatter>(&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>) -> 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<String, serde_json::Value> = 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<Check>,
|
||||
}
|
||||
|
||||
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<PathBuf> {
|
||||
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<PathBuf> {
|
||||
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<DiscoveredReview> {
|
||||
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<DiscoveredReview> {
|
||||
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<String, (usize, Check)> = 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<Check> = 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<Vec<Check>> {
|
||||
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<String> {
|
||||
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);
|
||||
}
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
@@ -642,25 +642,39 @@ impl SessionStorage {
|
||||
}
|
||||
|
||||
async fn create_schema(pool: &Pool<Sqlite>) -> 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(())
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user