From 38debd74209af3f9c8da266527aaba33cabada30 Mon Sep 17 00:00:00 2001
From: Jasper <jasper@jasperhugo.com>
Date: Tue, 11 Aug 2026 07:28:12 -0600
Subject: [PATCH] fix: enforce review check tool policy (#11128)

---
 crates/goose-cli/src/cli.rs                   |  1 +
 .../goose-cli/src/commands/review/handler.rs  | 55 ++++++++++++++++++-
 .../docs/guides/goose-cli-commands.md         |  2 +-
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/crates/goose-cli/src/cli.rs b/crates/goose-cli/src/cli.rs
index b87210055..fb94620c7 100644
--- a/crates/goose-cli/src/cli.rs
+++ b/crates/goose-cli/src/cli.rs
@@ -1137,6 +1137,7 @@ enum Command {
         /// (capped at 4 concurrent), bounding wall-clock to the slowest
         /// single check rather than waiting on the model to issue
         /// dispatches.
+        /// Checks with an explicit tool allowlist require the default orchestrator.
         #[arg(long = "no-orchestrate")]
         no_orchestrate: bool,
 
diff --git a/crates/goose-cli/src/commands/review/handler.rs b/crates/goose-cli/src/commands/review/handler.rs
index 32d579c44..9af2a1ff7 100644
--- a/crates/goose-cli/src/commands/review/handler.rs
+++ b/crates/goose-cli/src/commands/review/handler.rs
@@ -43,7 +43,8 @@ pub struct ReviewOptions {
     /// 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.
+    /// reliably on its own. Checks with an explicit tool allowlist require
+    /// the default orchestrator and are rejected on this path.
     pub no_orchestrate: bool,
     /// Additional free-form instructions to prepend to the review (PR
     /// intent, commit-message context, etc.). Surfaced to both the main
@@ -205,6 +206,7 @@ pub async fn handle_review(opts: ReviewOptions) -> Result<()> {
             }
             return Ok(());
         }
+        ensure_legacy_check_tools_are_unrestricted(&discovered)?;
         let mut session = build_session(SessionBuilderConfig {
             session_id: None,
             no_session: true,
@@ -280,6 +282,23 @@ fn filter_checks(discovered: DiscoveredReview, names: &[String]) -> DiscoveredRe
     }
 }
 
+fn ensure_legacy_check_tools_are_unrestricted(discovered: &DiscoveredReview) -> Result<()> {
+    let restricted: Vec<&str> = discovered
+        .checks
+        .iter()
+        .filter(|check| check.tools.is_some())
+        .map(|check| check.name.as_str())
+        .collect();
+    if restricted.is_empty() {
+        return Ok(());
+    }
+
+    bail!(
+        "--no-orchestrate cannot enforce per-check tool allowlists for: {}; rerun without --no-orchestrate",
+        restricted.join(", ")
+    )
+}
+
 /// 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.
@@ -555,6 +574,40 @@ mod tests {
         assert_eq!(names, vec!["security", "idempotency"]);
     }
 
+    #[test]
+    fn legacy_review_allows_checks_without_tool_policy() {
+        let discovered = DiscoveredReview {
+            checks: vec![ck("security")],
+        };
+
+        ensure_legacy_check_tools_are_unrestricted(&discovered).unwrap();
+    }
+
+    #[test]
+    fn legacy_review_rejects_nonempty_tool_allowlists() {
+        let mut restricted = ck("security");
+        restricted.tools = Some(vec!["read".to_string()]);
+        let discovered = DiscoveredReview {
+            checks: vec![restricted],
+        };
+
+        let error = ensure_legacy_check_tools_are_unrestricted(&discovered).unwrap_err();
+        assert!(error.to_string().contains("security"));
+        assert!(error.to_string().contains("without --no-orchestrate"));
+    }
+
+    #[test]
+    fn legacy_review_rejects_explicit_empty_tool_allowlists() {
+        let mut restricted = ck("no-tools");
+        restricted.tools = Some(Vec::new());
+        let discovered = DiscoveredReview {
+            checks: vec![restricted],
+        };
+
+        let error = ensure_legacy_check_tools_are_unrestricted(&discovered).unwrap_err();
+        assert!(error.to_string().contains("no-tools"));
+    }
+
     #[test]
     fn prepend_instructions_noop_when_none_or_empty() {
         assert_eq!(prepend_instructions("BASE", None), "BASE");
diff --git a/documentation/docs/guides/goose-cli-commands.md b/documentation/docs/guides/goose-cli-commands.md
index 553eb7b3b..05f8a79e7 100644
--- a/documentation/docs/guides/goose-cli-commands.md
+++ b/documentation/docs/guides/goose-cli-commands.md
@@ -479,7 +479,7 @@ Review the current git diff using goose. By default, `goose review` reviews the
 - **`--turn-limit <N>`**: Set the default turn limit for orchestrated review subprocesses and checks
 - **`--dry-run`**: Print the assembled review prompt and discovered checks without running the review
 - **`-q, --quiet`**: Suppress non-result output from the underlying agent
-- **`--no-orchestrate`**: Disable the default Rust-driven parallel orchestrator and use the single-prompt path
+- **`--no-orchestrate`**: Disable the default Rust-driven parallel orchestrator and use the single-prompt path. Checks that declare `tools` are rejected because this path cannot enforce per-check tool allowlists.
 - **`-i, --instructions <TEXT>`**: Add free-form review instructions
 - **`-f, --files <FILE>...`**: Restrict the review to specific files
 - **`-c, --check-filter <NAME>...`**: Run only checks with matching names