fix: enforce review check tool policy (#11128)

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,
 

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");

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