feat(cli): add goose review local code review command - #9114
Conversation
a114767 to
0ecbf32
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ecbf32b2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if scope_priority(&check.scope_dir) > scope_priority(&existing.scope_dir) { | ||
| *existing = check.clone(); |
There was a problem hiding this comment.
The dedupe logic only replaces an existing check when the new scope priority is strictly greater, but both global checks and repo-root checks are assigned scope_dir = "" (same priority). Because globals are loaded first, a ~/.config/.../checks/foo.md entry permanently masks <repo>/.agents/checks/foo.md, which contradicts the documented precedence (repo should shadow global) and silently runs the wrong rules for projects that intentionally override shared checks.
Useful? React with 👍 / 👎.
| cmd.arg(r); | ||
| } | ||
| None => { | ||
| cmd.arg("HEAD"); |
There was a problem hiding this comment.
When no explicit range is provided, both file discovery and prompt diff use git diff HEAD, which excludes untracked files. In practice this means newly created files are completely omitted from goose review until they are staged/tracked, so correctness or security issues in brand-new code are silently missed in the default workflow.
Useful? React with 👍 / 👎.
| })? | ||
| .trim_start_matches(['\r', '\n']); | ||
|
|
||
| let (frontmatter_raw, body_raw) = after_open.split_once("\n---").ok_or_else(|| { |
There was a problem hiding this comment.
Frontmatter parsing searches for the closing delimiter using split_once("\n---"), so check files saved with Windows CRLF (\r\n---) fail with “missing closing ---”. Since these .md check files are commonly edited cross-platform, this makes valid checks unreadable on Windows-style line endings and can abort review discovery in strict mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0db9cbda77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async = true, # IMPORTANT: parallelize | ||
| model = <check model>, | ||
| max_turns = <check turn_limit>, | ||
| extensions = <check tools or omit>, |
There was a problem hiding this comment.
The default review prompt tells the main agent to pass each check’s tools value directly as delegate(... extensions = <check tools>) (default_review_prompt.md), but delegate.extensions is an extension-name filter, not a tool-name filter (see summon::DelegateParams schema and extensions.retain(|ext| filter.contains(&ext.name())) in crates/goose/src/agents/platform_extensions/summon.rs). In practice, Amp-style values like tools: [Read, Grep] do not match extension names (e.g. developer), so the subagent can end up with no usable extensions and silently miss findings whenever a check declares tools.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d439e7e988
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cmd.arg("run") | ||
| .arg("--no-session") | ||
| .arg("--quiet") | ||
| .arg("--no-profile") | ||
| .arg("-i") | ||
| .arg("-") |
There was a problem hiding this comment.
The orchestrated review path parses turn-limit metadata and --turn-limit, but the spawned goose run command never receives a --max-turns argument, so each check falls back to the run-command default turn budget instead of the configured per-check limit. In practice this can let a misbehaving check loop far longer than intended (until the 5-minute timeout), increasing latency and token spend and violating the documented turn-limit precedence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6ffe0bd10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if opts.checks_only { | ||
| // No checks discovered (or `--no-orchestrate` set with checks | ||
| // empty after filter). With `--checks-only` there is nothing | ||
| // left to run, so return cleanly instead of dispatching the | ||
| // main agent. | ||
| return Ok(()); |
There was a problem hiding this comment.
When --checks-only is used together with --no-orchestrate, this branch returns early and skips session.headless(prompt), so no checks are executed at all even if checks were discovered. This silently produces an empty review for a valid flag combination and contradicts the documented behavior of --checks-only (run only subagent checks).
Useful? React with 👍 / 👎.
| let output = match timeout(Duration::from_secs(CHECK_TIMEOUT_SECS), wait).await { | ||
| Ok(o) => o.with_context(|| format!("wait on check '{}'", check.name))?, | ||
| Err(_) => { | ||
| anyhow::bail!( | ||
| "check '{}' timed out after {}s", | ||
| check.name, | ||
| CHECK_TIMEOUT_SECS | ||
| ); |
There was a problem hiding this comment.
On timeout, the code immediately returns an error without terminating the spawned goose run child process. In Tokio, dropped Child handles do not kill the process by default, so timed-out checks can continue running in the background, wasting resources and undermining the timeout/concurrency guarantees.
Useful? React with 👍 / 👎.
| let discovery_root = opts.check_scope.as_deref().unwrap_or(&repo_root); | ||
| let discovered = discover(discovery_root, &touched)?; |
There was a problem hiding this comment.
Discovery receives touched paths that are repo-relative even when --check-scope points at a subdirectory, so downstream scope walking joins that prefix twice (e.g., <scope>/ + api/...), which causes scoped .agents/checks and REVIEW.md under the custom scope to be missed. This makes --check-scope unreliable for monorepo subtrees.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35a6dbeda2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if opts.provider.is_some() && opts.default_model.is_some() { | ||
| return opts.default_model.clone(); | ||
| } | ||
| if let Some(m) = check.model.as_deref() { |
There was a problem hiding this comment.
resolve_check_model only suppresses a check’s model: when both --provider and --model are set. With --provider alone, the per-check model is still forwarded, which can be invalid for the selected provider (for example, a check pinned to a Claude model while running with --provider google), causing that check subprocess to fail and return no findings. An explicit provider should not silently inherit incompatible per-check model IDs.
Useful? React with 👍 / 👎.
| for findings in &check_results { | ||
| total_seen += findings.len(); | ||
| total_emitted += emit_findings(findings, min_sev); |
There was a problem hiding this comment.
Severity filtering is only applied to check_results via emit_findings, while main-pass findings are streamed directly by session.headless(prompt). In default orchestrated runs, this means --severity high can still print lower-severity check: main items, which contradicts the CLI contract that findings below the threshold are dropped and makes severity behavior inconsistent across finding sources.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecf918a6db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ..SessionBuilderConfig::default() | ||
| }) | ||
| .await; | ||
| return session.headless(prompt).await; |
There was a problem hiding this comment.
In the --no-orchestrate path, execution returns directly from session.headless(prompt) before the later severity parsing/filtering block runs, so --severity is effectively ignored for this flag combination. Users running goose review --no-orchestrate --severity high can still receive low/medium findings, which breaks the documented severity contract and makes output inconsistent between orchestration modes.
Useful? React with 👍 / 👎.
| 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); | ||
| by_name.insert(check.name.clone(), check); |
There was a problem hiding this comment.
Discovery stores checks in a single map keyed only by name, and this unconditional insert for synthesized REVIEW.md checks can overwrite an existing user-defined check with the same name (for example .agents/checks/repo-rules.md). That silently drops authored rules whenever names collide, so users can lose check coverage without any warning. Consider reserving the prefix or rejecting collisions explicitly.
Useful? React with 👍 / 👎.
alexhancock
left a comment
alexhancock
left a comment
There was a problem hiding this comment.
I'd like to eventually see this land in a plugin as our support for all the components of plugins gets more mature, but this seems really useful and a high value for the code review use-case so stamping to go into goose-cli!
We can revisit alternative implementations as plugin support gets better.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f62ce6dc7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(idx) = trimmed.find(" b/") { | ||
| let post = &trimmed[idx + 3..]; | ||
| return Some(post.to_string()); | ||
| } | ||
| if let Some(stripped) = trimmed.strip_prefix("a/") { | ||
| return Some(stripped.to_string()); |
There was a problem hiding this comment.
parse_diff_header_path only recognizes unquoted a/... b/... headers, but git diff emits quoted headers for non-ASCII paths (for example diff --git "a/dir/\303\251.txt" "b/dir/\303\251.txt"). In that case this function returns None, split_diff_by_file drops the file chunk, and run_main_pass_in_parallel silently skips reviewing that changed file entirely.
Useful? React with 👍 / 👎.
| Ok(String::from_utf8(out.stdout)? | ||
| .lines() | ||
| .filter(|l| !l.trim().is_empty()) | ||
| .map(|l| l.to_string()) | ||
| .collect()) |
There was a problem hiding this comment.
touched_files consumes git diff --name-only as plain newline text and forwards each line verbatim. For quoted path output (e.g. "dir/\303\251.txt"), downstream scope walking treats the quote/escape text as literal directory names, so scoped .agents/checks and scoped REVIEW.md files are not discovered for those changes. This causes valid scoped checks to be silently skipped on repos with quoted filenames.
Useful? React with 👍 / 👎.
|
Hey @joahg — the Check Rust Code Format check is failing. Could you run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c6dcd717d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let discovery_root = opts.check_scope.as_deref().unwrap_or(&repo_root); | ||
| let discovered = discover(discovery_root, &touched)?; | ||
| let discovered = filter_checks(discovered, &opts.check_filter); | ||
| print_discovered_summary(&discovered); |
There was a problem hiding this comment.
--quiet is documented as suppressing non-result output, but the command always calls print_discovered_summary before any quiet check. Running goose review --quiet therefore still prints discovery chatter to stderr (discovered N check(s), scope lines), which breaks quiet-mode scripting expectations and makes output inconsistent with the flag contract.
Useful? React with 👍 / 👎.
| .with_context(|| format!("read --prompt file {}", path.display()))?, | ||
| None => DEFAULT_REVIEW_PROMPT.to_string(), | ||
| }; | ||
| let base_prompt = prepend_instructions(&base_prompt, opts.instructions.as_deref()); |
There was a problem hiding this comment.
Reviewer instructions are prepended into base_prompt unconditionally, then orchestrated main-pass prompts add the same instructions again via run_main_pass_in_parallel(..., &opts) → build_main_pass_prompt(..., instructions). With --instructions set, every per-file main-pass subprocess receives duplicated instruction blocks, which inflates token usage and can skew model behavior on long instructions without adding signal.
Useful? React with 👍 / 👎.
|
ok, sorry for taking so long I think you should add this as a new type to crates/goose/src/sources.rs and then reuse the discover and parsing stuff we have going on there. how does that sound? |
|
happy to discuss in person what that would look like. sources.rs is pretty new and can do with some refactoring itself |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47989e3d1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let prefix = match discovery_root.strip_prefix(repo_root) { | ||
| Ok(p) => p, | ||
| Err(_) => return touched.to_vec(), |
There was a problem hiding this comment.
When --check-scope is passed as a relative path (for example --check-scope api), rebase_touched_to_scope hits the strip_prefix(repo_root) fallback and returns repo-relative touched paths unchanged, so discover() later joins them under the relative root and searches api/api/... instead of api/.... Fresh evidence: this code now handles absolute scope paths, but relative scopes still skip scoped checks/REVIEW.md silently for nested files.
Useful? React with 👍 / 👎.
|
🤖 Sent by Joah's AI agent: Done in 71805be — moved
Net: -719 / +33 in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71805beae5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let discovered = match working_dir.as_deref() { | ||
| Some(root) => crate::checks::discover(root, &[]) | ||
| .map_err(|e| Error::internal_error().data(e.to_string()))?, |
There was a problem hiding this comment.
list_sources currently calls discover(root, &[]) for SourceType::Check, but discover with an empty touched-file set only includes repo-root/global checks and skips subtree scopes like api/.agents/checks/*.md and scoped REVIEW.md. In a repo that defines scoped checks, those entries never appear in source listings, so ACP/UI consumers cannot see or manage them even though goose review can use them when files are touched under that scope.
Useful? React with 👍 / 👎.
| let global = check.path.starts_with( | ||
| crate::checks::global_checks_dirs() | ||
| .first() | ||
| .map(PathBuf::as_path) | ||
| .unwrap_or_else(|| Path::new("")), |
There was a problem hiding this comment.
The global flag is derived by checking only global_checks_dirs().first(), so checks loaded from the second supported global directory (~/.config/agents/checks) are incorrectly marked non-global. Also, when no global dir is returned, the fallback Path::new("") makes starts_with true for every path, marking repo-local checks as global. This mislabels check provenance in source listings and can break downstream UI behavior that depends on correct global/project classification.
Useful? React with 👍 / 👎.
Adds a new `goose review` subcommand that performs local code review.
Discovery:
- Parses `**/.agents/checks/*.md` subagent reviewers with YAML
frontmatter: `name`, `description`, `model`, `turn-limit`,
`tools` (per-check tool allowlist, optional and backwards
compatible), and `severity-default`
- Discovers `**/.agents/REVIEW.md` files and synthesizes a virtual
`repo-rules` (root) or `repo-rules:<scope>` (sub-tree) check from
each so its findings can be attributed via the `check` field
- Looks in repo root, all sub-trees touched by the diff, and global
locations:
`~/.config/goose/checks`
`~/.config/agents/checks`
- Closer scopes shadow same-named checks; checks loaded from global
directories are loaded leniently (parse errors are warned and skipped,
`README.md` is always skipped, and `name:` is allowed to differ
from filename for cross-tool compatibility)
CLI flags:
- `--prompt <FILE>` — replace the embedded default prompt
- `--model <MODEL>` — default model for the main agent and for any
check that does not declare its own
- `--override-model <MODEL>` — force every check to use this model,
ignoring per-check declarations
- `--turn-limit <N>` — default per-check turn limit
- `--provider <PROVIDER>`, `--dry-run`, `-q/--quiet`
Default prompt:
- Strong, explicit "correctness pass" the main agent runs before
delegating to subagents, calling out silent error paths, off-by-one
bugs, dropped errors, concurrency hazards, resource lifecycle, input
validation, leaked state, and logic/comment contradictions
- Instructs the main agent to dispatch all check delegates in parallel
via `delegate(... async: true ...)` and gather results with `load(taskId)`
- Findings are emitted as one JSON object per line with a `check`
attribution field (`main` for the correctness pass, the check name
for delegated subagents)
AGNTOPS-23
Signed-off-by: Joah Gerstenberg <joahg@squareup.com>
Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e09d6-63d3-705a-bf93-437cafa504f7
Skip the user's configured extension profile (--no-profile equivalent) and explicitly load only the two extensions review actually uses: - developer: shell, edit, write, tree (file inspection) - summon: delegate, load (parallel subagent dispatch) Brings the model's tools list from ~35 to 6, cutting the per-request prompt by roughly 50% on a typical Square setup with github, blockcell, blockchain, and various MCPs configured. No behavior change — review still discovers and dispatches checks the same way. AGNTOPS-23 Signed-off-by: Joah Gerstenberg <joahg@squareup.com> Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Joah Gerstenberg <joahg@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-019e09d6-63d3-705a-bf93-437cafa504f7
Mirrors the proven `sq-agents` review pattern (`agents/cli/pkg/checks/run.go`)
to eliminate the dominant source of run-to-run variance: the model
deciding on each turn whether to dispatch each check as a real subagent
or to inline the work itself.
When checks are discovered, the new orchestrator:
- Fans out one `goose run -i - --no-session --quiet --no-profile`
subprocess per check, reading the prompt from stdin
- Caps concurrency at MAX_WORKERS (4), matching sq-agents'
empirically-tuned ceiling
- Enforces a 5-minute per-check timeout
- Sends each check a strict, tool-free prompt that requires
`{"findings": [...]}` JSON output, mirroring sq-agents'
buildCheckPrompt
- Tags every finding with the originating `check` name in Rust, so
attribution no longer depends on the model following instructions
- Strips code fences and extracts the JSON object defensively, so
occasional model chatter around the JSON does not drop the check
The main correctness pass keeps running in-process via
`session.headless()`, with the checks table stripped from its prompt
(checks are dispatched separately). Both phases are awaited
concurrently with `tokio::join!`, so wall-clock for the orchestrated
phase is bounded by `max(main_pass, slowest_check)` instead of
serialized model-driven dispatch.
Per-check failures (subprocess error, timeout, malformed JSON) emit a
warning on stderr and contribute zero findings; one broken check must
never fail the whole review.
Also adjusts the model-resolution precedence so an explicit CLI
`--provider`+`--model` combo wins over a per-check `model:` field
that may belong to a different provider (e.g. a check pinned to
`goose-claude-4-sonnet` would 404 against Google).
Adds a `--no-orchestrate` escape hatch for the legacy single-prompt
behavior.
Benchmark on the same fixture (Gemini 3.1 Pro on both sides):
| run | wall | findings | checks attributed |
|-----------------------|-------|----------|-------------------|
| amp review | 62 s | 23 | perf+security+meta+README+untrusted-pr |
| goose review (orch) | 17 s | 20 | main+meta-review+repo-rules+security+perf |
| goose review (orch) | 14 s | 20 | same |
| goose review (orch) | 22 s | 20 | same |
| goose review (orch) | 16 s | 20 | same |
| goose review (orch) | 24 s | 20 | same |
Median ~17s vs amp's ~44-62s (~2.5-3.5x faster). Findings count and
check attribution are now stable across runs (was 0-15 findings before
this commit when the model didn't reliably dispatch).
29 unit tests pass (21 existing + 7 orchestrator + 1 model-precedence).
AGNTOPS-23
Signed-off-by: Joah Gerstenberg <joahg@squareup.com>
Co-authored-by: Amp <amp@ampcode.com>
Signed-off-by: Joah Gerstenberg <joahg@squareup.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e09d6-63d3-705a-bf93-437cafa504f7
… checks Adds the remaining flag surface needed for drop-in compatibility with `amp review`-style wrappers: - `-i`, `--instructions <TEXT>` — free-form reviewer guidance prepended to the base prompt and threaded through to every check subprocess so checks see the same context as the main agent. - `-f`, `--files <FILE>...` — restrict the diff (and therefore what the agent and checks see) to specific paths. - `-c`, `--check-filter <NAME>...` — only run named checks. - `-s`, `--check-scope <DIR>` — discover `.agents/checks/*.md` from this directory instead of the repo root. - `--checks-only` — skip the main correctness pass. - `--summary-only` — print `git diff --stat` and exit. Tests: 35 passing (was 29). Adds coverage for reviewer-instruction threading into per-check prompts and the `filter_checks` / `prepend_instructions` helpers. Amp-Thread-ID: https://ampcode.com/threads/T-019e09d6-63d3-705a-bf93-437cafa504f7 Co-authored-by: Amp <amp@ampcode.com>
… floor
Three improvements informed by the prompt structure that Amp-authored
checks already expect:
1. Per-check prompt now restricts findings to lines beginning with
'+' in the diff and explicitly forbids reporting on
unchanged/pre-existing context. Without this the model routinely
flags surrounding context lines.
2. Default review prompt grows a 'code-quality pass' alongside the
existing correctness pass: hackiness/unnecessary code, too much
shared mutable state, and abstraction fit in both directions
(unnecessary indirection AND missing abstractions). Each finding
must cite a concrete location and recommend exactly one action,
only when it improves current code.
3. New `--severity <low|medium|high|critical>` flag for the
orchestrator output, defaulting to `medium` so `low` findings
are hidden from review output unless explicitly requested. Counts
suppressed findings on stderr for triage. End-to-end:
--severity high -> 9 emitted, 3 hidden (only critical/high)
--severity low -> 13 emitted (every finding shown)
Tests: 39 passing (was 35). Adds coverage for the new check-prompt
guardrails and the Severity ordering / parsing.
Amp-Thread-ID: https://ampcode.com/threads/T-019e09d6-63d3-705a-bf93-437cafa504f7
Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e09d6-63d3-705a-bf93-437cafa504f7 Co-authored-by: Amp <amp@ampcode.com>
The orchestrated main pass used to run as a single in-process
session.headless() call:
- text-mode chatter to stdout (not JSONL), so findings could be
interleaved with banner output instead of cleanly parseable
- the entire diff was sent in one prompt, which on long PRs
(1000+ lines) reliably caused gemini-3.x to short-circuit with
a literal '[]' after ~30s — the model declined to do the work
instead of returning real findings
Concretely, on a 1533-line agents PR Amp's reviewer found 5 real
correctness bugs and goose review's main pass returned 0.
This change replaces the in-process main pass with N parallel
'goose run' subprocesses, one per touched file, reusing the same
strict JSON contract and parser the per-check orchestrator already
uses:
- per-file context is small enough that the model walks every
added/modified line instead of bailing
- subprocesses run concurrently (capped at MAX_WORKERS), so
wall clock stays close to the slowest single file rather
than scaling with diff size
- a failure on one file warns and continues; it never aborts
the whole review
- main-pass findings flow through emit_findings() with the same
severity floor as check findings, so '--severity low' surfaces
them and the stderr summary reflects the suppressed counts
The orchestrator now also runs even when no .agents/checks/*.md
are discovered, so single-repo reviews get the same per-file
parallelism as multi-check ones. '--no-orchestrate' still falls
back to the original in-process session.headless() path for
comparison and for models that handle delegation reliably on
their own.
Also adds tests for the per-file diff splitter, the post-image
path extraction (renames), and the strict JSON main-pass prompt.
Amp-Thread-ID: https://ampcode.com/threads/T-019e09d6-63d3-705a-bf93-437cafa504f7
Co-authored-by: Amp <amp@ampcode.com>
…-run
When N goose run processes start simultaneously on a fresh container
(e.g. the parallel orchestrator in goose review fanning out one
subprocess per touched file + per check), the schema-bootstrap path
in SessionManager raced:
SELECT EXISTS('schema_version') -> false (in process A and B)
CREATE TABLE schema_version (...) -> ok (process A)
CREATE TABLE schema_version (...) -> error: 'table already exists' (process B)
Process B then surfaced the failure to its caller as
'Could not create session: ... table schema_version already exists',
which propagated as a non-zero exit even with --no-session. The
result was that 4-of-5 review subprocesses on a cold workstation
exited 1 immediately, leaving the orchestrator with effectively
zero coverage.
Fix:
* Run create_schema under BEGIN IMMEDIATE so SQLite serializes
writers across processes.
* Add IF NOT EXISTS to every CREATE TABLE / CREATE INDEX in the
bootstrap.
* Use INSERT OR IGNORE for the bootstrap version row.
After this change, a process that raced into create_schema after
another process completed it sees the schema already in place and
no-ops cleanly instead of failing.
The existing test_concurrent_session_creation still passes; this
fix specifically targets the schema-creation race that fired
*before* any session was opened.
Amp-Thread-ID: https://ampcode.com/threads/T-019e09d6-63d3-705a-bf93-437cafa504f7
Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e22c3-9469-7741-810e-fd85193d56b5 Co-authored-by: Amp <amp@ampcode.com>
P1 correctness fixes:
- discover: repo-root checks now properly override same-named globals
(both used scope_priority("") = 1; globals now get priority 0)
- handler: include untracked files in default review diff so brand-new
files get reviewed (git diff HEAD silently dropped them)
- default_review_prompt: stop telling the agent to pass per-check tools
as delegate.extensions — that filter is by extension name, not tool
name, so it disabled every extension and the subagent ran tool-less
- orchestrator: forward per-check turn-limit to spawned 'goose run'
subprocesses via --max-turns (was parsed and dropped)
- handler: --checks-only --no-orchestrate no longer no-ops; falls
through to orchestrator's check runner so checks actually execute
- orchestrator: kill_on_drop(true) on subprocess so timed-out
'goose run' children are SIGKILLed instead of leaking
- orchestrator: --provider alone now drops per-check model: a
Claude-pinned per-check model would 404 against Google/etc.; only
the per-check model from the matching provider was safe to inherit
- orchestrator: parse_diff_header_path handles git's quoted form
("a/dir/\\303\\251.txt" "b/dir/...") so non-ASCII / spaced
paths aren't silently dropped from the per-file main pass
P2 robustness fixes:
- check: tolerate CRLF line endings in frontmatter terminator
- handler: pass -c core.quotePath=off to all git invocations so
non-ASCII paths come back as clean UTF-8 (matters for both diff
parsing and scope discovery)
- handler: --check-scope rebases touched paths to the discovery root
so scope walking doesn't double-prefix <scope>/api/...
- handler: --severity is parsed up front and applied to --checks-only
--no-orchestrate output too
- handler: --quiet honored before print_discovered_summary (no
discovery chatter to stderr)
- handler: reviewer instructions only prepended to base_prompt for the
legacy single-prompt path; orchestrated mode injects per-subprocess
to avoid duplication
- discover: synthesized REVIEW.md checks now use the priority-aware
recorder so a user-authored 'repo-rules' check isn't overwritten
Adds 9 new tests covering the above. Full suite (56 tests in
commands::review) passes; cargo fmt and clippy --all-targets
-D warnings clean.
Refs: AGNTOPS-23
Signed-off-by: Joah Gerstenberg <joah@squareup.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e22eb-2b5a-7504-815d-3360284d51fa
Co-authored-by: Amp <amp@ampcode.com>
Address @aaif-goose review feedback by lifting Check parsing and discovery out of `goose-cli` into a new `goose::checks` module that sits alongside `sources` / `skills`. - Add `SourceType::Check` to the SDK so checks can flow through the same listing/UI pipeline as skills and projects. - Create `crates/goose/src/checks/mod.rs` containing `Check`, `DiscoveredReview`, `discover`, `global_checks_dirs`, and `candidate_scope_dirs`. Reuses `sources::parse_frontmatter` so checks share the YAML pipeline with skills and projects. - Wire `SourceType::Check` into `sources::list_sources` via a new `Check::to_source_entry` adapter that stuffs per-check tunables (`model`, `turn-limit`, `tools`, `severity-default`, `scope_dir`) into `SourceEntry.properties`. - Delete `goose-cli/src/commands/review/{check,discover}.rs` and point the CLI's review pipeline (`handler`, `orchestrator`, `prompt`) at `goose::checks` instead. Net: -719 / +33 in goose-cli plus the new module in goose. All 19 `checks` tests and 37 `commands::review` tests pass; `cargo fmt` / `cargo clippy --all-targets -- -D warnings` clean. Tracking: AGNTOPS-23 Signed-off-by: Joah Gerstenberg <joah@squareup.com> Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Joah Gerstenberg <joah@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-019e282e-50fe-7072-9130-24ddbb6bb852
71805be to
e65d3d3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e65d3d3637
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cmd.arg(r); | ||
| } | ||
| None => { | ||
| cmd.arg("HEAD"); |
There was a problem hiding this comment.
When --range is omitted, the default path hard-codes git diff ... HEAD; in a newly initialized repo with no commits, HEAD is undefined and Git exits with fatal: ambiguous argument 'HEAD', so goose review fails before it can synthesize untracked-file diffs. This blocks the command for first-commit workflows; the default mode should fall back to an empty-tree comparison (or a HEAD-less diff path) instead of requiring an existing commit.
Useful? React with 👍 / 👎.
…variant
Per reviewer feedback (lifeizhou-ap), drop the new SourceType::Check
variant and have review checks present as SourceType::Agent entries
with properties["kind"] = "check" for differentiation.
Checks are conceptually sub-agents specialized for code review, so they
naturally fit under the existing Agent source type. The on-disk path
(.agents/checks/) stays unchanged for Amp portability, and the review
orchestrator continues to consume goose::checks::{discover, Check}
directly so the per-check tunables (model, turn-limit, tools,
severity-default) and the discovery semantics (scope walking,
REVIEW.md -> virtual repo-rules) are unaffected.
list_sources(SourceType::Agent) now also walks .agents/checks/ and
returns checks alongside .agents/agents/ entries; clients can filter on
properties.kind to differentiate.
This aligns the source schema with the planned ACP "start agent in
new session or as sub-agent" surface so review checks can be a first
consumer once that lands.
Refs AGNTOPS-23
Signed-off-by: Joah Gerstenberg <joah@squareup.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e3b8b-6a77-71bd-8c56-2c6650137a6a
Co-authored-by: Amp <amp@ampcode.com>
|
🤖 Sent by Joah's AI agent: Thanks @lifeizhou-ap! Done in 767a718 — dropped Kept the on-disk path at This lines the source schema up with the upcoming ACP "start agent in new session or as sub-agent" RPC so review checks can be a first consumer once that lands — without blocking on it. Happy to also migrate the orchestrator off subprocesses onto the ACP path as a follow-up once the RPC is ready; just give me a heads up. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 767a718a7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(c) => c, | ||
| Err(_) => continue, | ||
| }; | ||
| out.push_str(&format!("diff --git a/{path} b/{path}\n")); |
There was a problem hiding this comment.
Emit git-style quoted paths when synthesizing untracked-file diffs; the current unquoted header (diff --git a/{path} b/{path}) can be misparsed by split_diff_by_file for names containing the sequence " b/" (for example new b/file.rs). In that case parse_diff_header_path splits at the wrong b/, producing a corrupted path and causing the main-pass subprocess to review/attribute findings against an invalid filename for those new files.
Useful? React with 👍 / 👎.
* origin/main: (160 commits) Add Linux musl CLI builds (#9240) feat(acp): paginate session list (#9199) docs: reorganize (#9310) Structured per-provider config block, non-destructive provider switching (#8977) feat(cli): add `goose review` local code review command (#9114) feat(tui): diff viewer (#9260) fix(otel): emit trace_output as span attribute instead of event (#9255) docs: add guide for connecting goose Desktop to a remote goosed server (#9275) fix(config): check file fallback when keyring has no entry (#9279) fix(desktop): ScheduleModal error message styling (#9278) fix(ui): align sidebar hamburger in macOS fullscreen (#9257) Add documentation for new provider SaladCloud AI Gateway (#9253) fix: use current_exe() instead of PATH lookup when spawning goose (#9236) fix(extension_manager): set TCP_USER_TIMEOUT on streamable HTTP clients (#9207) fix: activate custom provider after adding via configure (#9213) Flush OTLP traces reliably on exit with configurable timeout (#9228) fix: reduce excessive MISSING_TRANSLATION warnings for fallback locales (#9294) feat(acp): pass session cwd param to acp providers (#9229) fix(desktop): eliminate cross-window deep link contamination (#9273) fix: improve Telegram gateway error reporting and connection reliability (#9223) ... Signed-off-by: Michael Neale <michael.neale@gmail.com> # Conflicts: # crates/goose/src/agents/agent.rs # crates/goose/tests/agent.rs
* main: (70 commits) Feat/summon subagent instructions (#9325) feat: open-plugins generalization + skills (#9112) feat(hooks): PreToolUse denial (#9304) Add support for optional api_key configuration for declarative openai-engine providers (#9202) fix(cli): use plain '> ' prompt instead of goose emoji (#9305) flag for login shell PATH (#9313) Remove popular chat topics from new chat screen (#9307) fix: stop killing goosed when a window closes (#9302) Remove vendored Windows binaries (#9318) Add Linux musl CLI builds (#9240) feat(acp): paginate session list (#9199) docs: reorganize (#9310) Structured per-provider config block, non-destructive provider switching (#8977) feat(cli): add `goose review` local code review command (#9114) feat(tui): diff viewer (#9260) fix(otel): emit trace_output as span attribute instead of event (#9255) docs: add guide for connecting goose Desktop to a remote goosed server (#9275) fix(config): check file fallback when keyring has no entry (#9279) fix(desktop): ScheduleModal error message styling (#9278) fix(ui): align sidebar hamburger in macOS fullscreen (#9257) ...
4 participants
Summary
Adds
goose review, a local code review CLI inspired by Amp's Code Review > Checks. Compatible with the same.agents/checks/*.mdfrontmatter shape so existing checks port over without modification.What it does
goose review [<RANGE>]collects the diff for the working tree (or an explicit range likemain...HEAD), discovers project + global review configuration, fans out a per-file main correctness pass plus N parallel check subagents, and emits findings as JSON-per-line carrying acheckfield for end-to-end attribution.Discovery
Subagent reviewers live in Markdown files with YAML frontmatter under any
.agents/checks/directory:All frontmatter fields except
nameare optional. Thetoolsandseverity-defaultfields are optional and backwards compatible — checks without them continue to work unchanged.Search order, with closer scopes shadowing same-named checks:
~/.config/goose/checks/,~/.config/agents/checks/(global)<repo>/.agents/checks/<repo>/<scope>/.agents/checks/for every directory above a touched fileGlobal directories are loaded leniently: parse errors are warned and skipped,
README.mdis always skipped, andname:is allowed to differ from the filename for cross-tool compatibility.REVIEW.md → virtual
repo-ruleschecks**/.agents/REVIEW.mdfiles are auto-derived into virtual checks namedrepo-rules(root) orrepo-rules:<scope>(sub-tree), so their findings carry their own provenance:{"severity": "critical", "path": "api/handlers/users.py", "line_start": 12, "line_end": 12, "summary": "...", "check": "repo-rules"}CLI flags
--prompt <FILE>--model <MODEL>model:--override-model <MODEL>--turn-limit <N>25)--provider <PROVIDER>--dry-run-q,--quiet--no-orchestrateAmp-compatible flags
The CLI also accepts the same flag surface as
amp review, so existing review wrappers can switch tools by swapping the binary name:-i,--instructions <TEXT>-f,--files <FILE>...-c,--check-filter <NAME>...-s,--check-scope <DIR>.agents/checks/*.mdfrom this directory instead of the repo root.--checks-only--summary-onlygit diff --statand exit; do not call the agent.--severity <LEVEL>mediummatches Amp's CLI behavior of hidinglowfrom review output. Suppressed findings are counted on stderr.Precedence
--override-model> per-check frontmattermodel:>--model> agent defaultturn-limit:>--turn-limit>25Default prompt
The embedded default prompt does two passes before (or alongside) delegating to checks:
Findings from this pass are tagged with
"check": "main".Per-check prompt
Each check subprocess receives a strict, tool-free prompt that (a) requires
{"findings": [...]}JSON output, (b) embeds the check body and any--instructionstext, and (c) explicitly restricts findings to lines beginning with+in the diff and forbids reporting on unchanged/pre-existing context. Without that restriction the model routinely flags surrounding context lines that just happen to appear in the hunk.Override the entire base prompt with
--prompt my-prompt.md.Tests
Coverage includes frontmatter parsing (full, minimal, invalid, extended with
severity-default+tools), filename/name validation (parse keeps declared name; strict validation only enforced for repo-local checks), default-stem behavior, model + turn-limit + tools precedence, scope walking from touched files, closer-scope shadowing, virtualrepo-ruleschecks synthesized from REVIEW.md at every scope, the assembled prompt layout (including thetoolsandseverity_defaultcolumns and the attribution instruction), the orchestrator (strict per-check prompt, JSON parsing with code-fence stripping and chatter tolerance, model-resolution precedence including the CLI--provider+--modeloverride), the per-file diff splitter (multi-file, single-file, empty input, renames using post-image path), the strict main-pass prompt (file-pinned, JSON-only, optional reviewer instructions block), reviewer-instructions threading into per-check prompts, the--check-filter/--instructionshelpers, the per-check guardrails that restrict findings to+lines, andSeverityordering / lenient parsing / strict CLI parsing.Out of scope (follow-ups)
goose reviewinto the goose 2.0 UI (the CLI is the priority for this PR).orchestrator spawns a
goose runsubprocess per check + per touched file;subprocess startup is ~50ms × N and can be eliminated by replacing the
subprocess with a direct provider call).
Performance + recall
Both running on Gemini 3.1 Pro (the same model Amp's review subagent uses on
ampcode.com/models).Real PR (squareup/agents#2978, +1500-ish lines across 8 files)
amp reviewgoose review(orchestrated)Direct
goose reviewprobe (bypassing the wrapper) on the same head emitted 4 findings in 26s, two of which matched Amp's findings exactly (humanInt(-N)formatting bug, strict branch-equality false negative) plus aSessionID-overwrite bug Amp missed. The remaining gap is a model-quality / non-determinism gap that lands well within the same neighborhood as Amp on Gemini 3.1 Pro.Synthetic fixtures (small files, planted bugs)
amp reviewgoose review(orchestrated)amp reviewgoose review(orchestrated)What changed under the hood
Rust-driven parallel orchestrator for both checks and the main pass (the big win).
The original review path relied on the LLM to (a) walk the entire diff in one in-process
session.headless()call and (b) decide whether to dispatch each check as a real subagent. Both are non-deterministic and were the dominant source of variance — and on long real-world diffs (1000+ lines) the in-process main pass reliably short-circuited with[]after ~30s instead of doing the work.The orchestrator now fans out from Rust:
goose run -i - --no-session --no-profile --quietsubprocess per discovered check, capped atMAX_WORKERS = 4via a Tokio semaphore, with a 5-minute per-check timeout. Each check receives a strict, tool-free prompt requiring{"findings": [...]}JSON output, and findings are attributed to the originatingcheckin Rust so attribution no longer depends on the model."check": "main"so the orchestrator's emitter can attribute them like any other check.tokio::join!, so total wall clock is bounded bymax(slowest_main_file, slowest_check)instead of scaling with diff size or check count.Add
--no-orchestrateto fall back to the legacy single-prompt path that asks the main agent to dispatch checks viadelegate(... async: true ...).Idempotent SQLite schema bootstrap.
SessionManager::create_schemapreviously had a TOCTOU race (SELECT EXISTS('schema_version')→CREATE TABLE schema_version) that surfaced asCould not create session: ... table schema_version already existswhenever 4+ subprocesses started simultaneously on a fresh container — exactly the workload the orchestrator generates. The path now runs underBEGIN IMMEDIATEwithIF NOT EXISTSon every DDL statement andINSERT OR IGNOREon the bootstrap version row, so concurrent first-run startup is safe.Trimmed the agent's toolset. Skip the user's configured extension profile and load only
developer+summon. On a typical setup that drops the tool list from ~35 functions (github, blockcell, MCPs, etc.) to 6, cutting the per-request prompt by ~50%.Stronger correctness prompt that walks the diff for the bug classes subagents miss (silent error paths, off-by-one, dropped error returns, concurrency hazards, resource lifecycle, input validation, leaked state, comment/code contradictions).
Update: Codex review feedback addressed (commit 47989e3)
Codex flagged 16 issues across 8 reviews; this revision fixes all 16.
P1 correctness/security:
discover.rs: repo-root checks now properly override same-named global checks (both usedscope_priority("") = 1; globals demoted to priority 0).handler.rs: synthesizenew filediff chunks for untracked files in default-range mode so brand-new files actually get reviewed (git diff HEADsilently dropped them).default_review_prompt.md: stop telling the agent to pass per-checktoolsasdelegate.extensions— that filter matches extension names (developer,summon), not tool names (Read,Grep), so the subagent ended up tool-less.orchestrator.rs: forward per-checkturn-limitto spawnedgoose runsubprocesses via--max-turns(was parsed and dropped on the floor).handler.rs:--checks-only --no-orchestrateno longer no-ops; falls through to the orchestrator's check runner so checks actually execute.orchestrator.rs:kill_on_drop(true)on the spawnedCommandso timed-out children get SIGKILLed instead of leaking and racking up tokens.orchestrator.rs:--provideralone (without--model) now drops the per-checkmodel:. A Claude-pinned per-check model would 404 against Google's API; only the explicit-CLI model is safe to inherit across providers.orchestrator.rs:parse_diff_header_pathhandles git's quoted-header form (e.g."a/dir/\303\251.txt" "b/dir/...") so non-ASCII or spaced paths aren't silently dropped from the per-file main pass. Also paired with-c core.quotePath=offon the source side.P2 robustness/UX:
check.rs: tolerate CRLF line endings in the frontmatter terminator.handler.rs: pass-c core.quotePath=offto allgitinvocations so non-ASCII paths return as clean UTF-8 (fixes scope discovery on quoted paths).handler.rs:--check-scoperebases touched paths to the discovery root so candidate scope walking doesn't double-prefix<scope>/api/....handler.rs:--severityis parsed up front and applied to--checks-only --no-orchestrateoutput too.handler.rs:--quiethonored beforeprint_discovered_summary(nodiscovered N check(s)chatter to stderr).handler.rs: reviewer instructions only prepended tobase_promptfor the legacy single-prompt path; orchestrated mode injects per-subprocess to avoid duplication on every per-file main-pass call.discover.rs: synthesizedREVIEW.mdchecks now use the priority-aware recorder so a user-authored check namedrepo-rules.mdisn't silently overwritten.Adds 9 new tests; full
commands::reviewsuite is 56/56.cargo fmt --all -- --checkandcargo clippy -p goose-cli --all-targets -- -D warningsclean.🤖 Sent by Joah's AI agent — tracking ticket: AGNTOPS-23
Update: address @lifeizhou-ap review feedback — checks are
SourceType::Agent, not a new variant (commit 767a718)Per the suggestion to use
agentas the surface for the review sub-agents:SourceType::Checkenum variant entirely.Check::to_source_entrynow returnsSourceType::Agentwithproperties["kind"] = "check"so clients can differentiate review checks from.agents/agents/*.mdagents while sharing one source type.list_sources(SourceType::Agent)now walks.agents/checks/in addition to the existing agent directories, so checks flow through the same listing pipeline as agents..agents/checks/for Amp portability — thegoose::checks::{discover, Check}discovery layer (scope walking, REVIEW.md → virtualrepo-rules, closer-scope shadowing) is unchanged, and the review orchestrator continues to consumeCheckdirectly.This keeps the source schema aligned with the planned ACP "start agent in new session or as sub-agent" RPC so review checks can be a first consumer once that lands, without blocking on it.
Adds 1 new test covering check → Agent listing. Full suite:
cargo test -p goose --lib1467/1467,cargo test -p goose-cli --lib236/236,cargo clippy -p goose -p goose-sdk -p goose-cli --all-targets -- -D warningsclean.