Tool rules: forward metadata through wrappers, close review gaps, pin tinytools#57

crates/tinyagents-harness/src/agent_loop/nested_tests.rs

@@ -2048,3 +2048,58 @@ async fn a_parent_dropped_while_a_nested_result_is_observed_still_closes_the_cal
     assert_eq!(nested_started(&recorder), 1);
     every_start_has_one_terminal_event(&recorder);
 }
+
+fn with_tool_rules(harness: &mut AgentHarness<()>, rules: serde_json::Value) {
+    let mut policy = harness.policy().clone();
+    policy.tool_rules = crate::tool::ToolRulePolicy::new(
+        serde_json::from_value::<tinytools::ToolRules>(rules).unwrap(),
+    );
+    harness.with_policy(policy);
+}
+
+#[tokio::test]
+async fn a_tool_rule_refuses_a_nested_call() {
+    let (outcome, runs) = nested_refusal(Leaf::new("leaf"), |harness| {
+        with_tool_rules(
+            harness,
+            json!({ "rules": [ { "id": "no-leaf", "effect": "deny", "match": { "name": "leaf" } } ] }),
+        );
+    })
+    .await;
+
+    assert!(
+        outcome.unwrap_err().contains("rule 'no-leaf'"),
+        "tool rules must bind nested calls"
+    );
+    assert_eq!(runs, 0);
+}
+
+#[tokio::test]
+async fn a_require_approval_rule_fails_a_nested_call_instead_of_deferring() {
+    let (outcome, runs) = nested_refusal(Leaf::new("leaf"), |harness| {
+        with_tool_rules(
+            harness,
+            json!({ "rules": [ { "effect": "require_approval", "match": { "name": "leaf" } } ] }),
+        );
+    })
+    .await;
+
+    assert!(outcome.unwrap_err().contains("requires approval"));
+    assert_eq!(runs, 0);
+}
+
+#[tokio::test]
+async fn an_auto_approve_rule_waives_a_nested_declared_approval() {
+    let mut policy = ToolPolicy::classified();
+    policy.access.approval_required = true;
+    let (outcome, runs) = nested_refusal(leaf_with("gated", policy), |harness| {
+        with_tool_rules(
+            harness,
+            json!({ "rules": [ { "effect": "auto_approve", "match": { "name": "gated" } } ] }),
+        );
+    })
+    .await;
+
+    assert!(outcome.is_ok(), "{outcome:?}");
+    assert_eq!(runs, 1);
+}

crates/tinyagents-harness/src/agent_loop/tool_rules_tests.rs

@@ -2,22 +2,21 @@
 //! `tool_search` results and every call, including approval, nested calls,
 //! indirect targets and a hosted definition's own rules.
 
-use std::sync::{Arc, Mutex};
+use super::*;
+
+use std::sync::Mutex;
 
 use async_trait::async_trait;
-use serde_json::{Value, json};
+use serde_json::json;
 
-use crate::context::{RunConfig, RunContext};
 use crate::host::{
     AllowAllSecurityGate, FixedModelResolver, HostCapabilities, StaticContextComposer,
 };
-use crate::runtime::{AgentHarness, AgentInvocation, AgentTurnRequest, RunPolicy};
+use crate::runtime::{AgentInvocation, AgentTurnRequest, RunPolicy};
 use crate::testkit::ScriptedModel;
 use crate::tool::ToolRulePolicy;
 use tinyagents_definition::{AgentDefinition, InMemoryDefinitionRegistry};
-use tinyinference_llm::message::Message;
 use tinyinference_llm::model::ModelResponse;
-use tinyinference_llm::tool::ToolCall;
 use tinytools::{
     RuleContext, Tool, ToolExposure, ToolPolicy, ToolResult, ToolRuleSet, ToolRules, ToolSubject,
 };
@@ -92,11 +91,13 @@ impl Tool for RuleTool {
     fn policy(&self) -> ToolPolicy {
         self.policy.clone()
     }
-    fn indirect_target(&self, args: &Value) -> Option<ToolSubject> {
+    fn indirect_target(&self, args: &Value) -> Option<tinytools::IndirectCall> {
         if !self.dispatches {
             return None;
         }
-        args.get("action")?.as_str().map(ToolSubject::named)
+        args.get("action")?
+            .as_str()
+            .map(|name| ToolSubject::named(name).into())
     }
     async fn execute(&self, arguments: Value) -> anyhow::Result<ToolResult> {
         self.seen.lock().unwrap().push(arguments);
@@ -366,3 +367,193 @@ async fn a_hosted_definition_stacks_its_rules_on_the_policy() {
     assert_eq!(web.calls(), 0);
     assert!(tool_text(&run.messages, "c1").contains("not permitted by tool rules"));
 }
+
+// ── Review follow-ups ───────────────────────────────────────────────────────
+
+#[tokio::test]
+async fn a_rewrite_target_carries_its_own_approval_rule() {
+    let model = Arc::new(ScriptedModel::new(vec![
+        calls(vec![("c1", "missing", json!({}))]),
+        ModelResponse::assistant("never reached"),
+    ]));
+    let mut harness: AgentHarness<()> = AgentHarness::new();
+    harness.register_model("scripted", model as _);
+    harness.with_policy(RunPolicy {
+        unknown_tool: UnknownToolPolicy::Rewrite {
+            tool_name: "send".to_string(),
+        },
+        tool_rules: ToolRulePolicy::new(rules(json!({ "rules": [
+            { "effect": "require_approval", "match": { "name": "send" } },
+        ] }))),
+        ..RunPolicy::default()
+    });
+    let send = RuleTool::new("send");
+    harness.register_tool(send.clone());
+
+    let run = run(&harness, "rewrite").await;
+
+    assert_eq!(send.calls(), 0, "the rewritten call waits for approval");
+    let deferred = run.deferred.expect("the run waits for approval");
+    assert_eq!(deferred.approvals.len(), 1);
+}
+
+#[tokio::test]
+async fn repaired_arguments_are_checked_against_the_rules_again() {
+    let mut malformed = ToolCall::new(
+        "c1",
+        "execute",
+        Value::String("{action: \"GMAIL_DELETE_EMAIL\"}".to_string()),
+    );
+    malformed.invalid = Some("unquoted key".to_string());
+    let mut response = ModelResponse::assistant("");
+    response.message.tool_calls.push(malformed);
+    let model = Arc::new(ScriptedModel::new(vec![
+        response,
+        ModelResponse::assistant("done"),
+    ]));
+    let policy = ToolRulePolicy::new(rules(json!({ "rules": [
+        { "id": "no-delete", "effect": "deny", "match": { "name": "*_delete_*" } },
+    ] })));
+    let execute = RuleTool::dispatcher("execute");
+    let mut harness = harness_with(model, policy);
+    harness.register_tool(execute.clone());
+
+    let run = run(&harness, "repaired").await;
+
+    assert_eq!(
+        execute.calls(),
+        0,
+        "the repaired call names a denied target"
+    );
+    assert!(tool_text(&run.messages, "c1").contains("rule 'no-delete'"));
+}
+
+#[tokio::test]
+async fn a_rule_can_withhold_tool_search_itself() {
+    let model = Arc::new(ScriptedModel::new(vec![
+        calls(vec![("s1", "tool_search", json!({"query": "deferred"}))]),
+        ModelResponse::assistant("done"),
+    ]));
+    let policy = ToolRulePolicy::new(rules(json!({ "rules": [
+        { "id": "no-discovery", "effect": "deny", "match": { "name": "tool_search" } },
+    ] })));
+    let mut harness = harness_with(model.clone(), policy);
+    harness.register_tool(RuleTool::deferred("deferred_open"));
+
+    let run = run(&harness, "no-search").await;
+
+    assert!(!tool_names(&model.requests()[0]).contains(&"tool_search".to_string()));
+    let answer = tool_text(&run.messages, "s1");
+    assert!(answer.contains("rule 'no-discovery'"), "{answer}");
+    assert!(!answer.contains("deferred_open"), "{answer}");
+}
+
+struct FamilyTool(&'static str);
+
+#[async_trait]
+impl Tool for FamilyTool {
+    fn name(&self) -> &str {
+        "dup"
+    }
+    fn description(&self) -> &str {
+        "same name, different family"
+    }
+    fn parameters_schema(&self) -> Value {
+        json!({ "type": "object" })
+    }
+    fn family(&self) -> Option<&str> {
+        Some(self.0)
+    }
+    async fn execute(&self, _arguments: Value) -> anyhow::Result<ToolResult> {
+        Ok(ToolResult::success(self.0))
+    }
+}
+
+#[tokio::test]
+async fn a_toolset_tool_never_takes_a_denied_registered_tools_name() {
+    let model = Arc::new(ScriptedModel::new(vec![ModelResponse::assistant("done")]));
+    let policy = ToolRulePolicy::new(rules(json!({ "rules": [
+        { "effect": "deny", "match": { "family": "registered" } },
+    ] })));
+    let mut harness = harness_with(model.clone(), policy);
+    harness.register_tool(Arc::new(FamilyTool("registered")));
+    let mut extra: crate::tool::ToolRegistry<(), ()> = crate::tool::ToolRegistry::new();
+    extra.register(Arc::new(FamilyTool("toolset")));
+    harness.with_toolset(Arc::new(extra));
+
+    run(&harness, "collision").await;
+
+    assert!(!tool_names(&model.requests()[0]).contains(&"dup".to_string()));
+}
+
+#[tokio::test]
+async fn normalized_arguments_are_checked_against_the_rules_again() {
+    // A JSON-encoded object is decoded by normalization before dispatch; the
+    // final rule check sees the decoded target.
+    let model = Arc::new(ScriptedModel::new(vec![
+        calls(vec![(
+            "c1",
+            "execute",
+            Value::String("{\"action\":\"GMAIL_DELETE_EMAIL\"}".to_string()),
+        )]),
+        ModelResponse::assistant("done"),
+    ]));
+    let policy = ToolRulePolicy::new(rules(json!({ "rules": [
+        { "id": "no-delete", "effect": "deny", "match": { "name": "*_delete_*" } },
+    ] })));
+    let execute = RuleTool::dispatcher("execute");
+    let mut harness: AgentHarness<()> = AgentHarness::new();
+    harness.register_model("scripted", model as _);
+    harness.with_policy(RunPolicy {
+        invalid_args: crate::runtime::InvalidArgsPolicy::NormalizeThenReturnToolError,
+        tool_rules: policy,
+        ..RunPolicy::default()
+    });
+    harness.register_tool(execute.clone());
+
+    let run = run(&harness, "normalized").await;
+
+    assert_eq!(execute.calls(), 0);
+    let text = tool_text(&run.messages, "c1");
+    assert!(text.contains("rule 'no-delete'"), "{text}");
+}
+
+#[tokio::test]
+async fn a_require_approval_rule_on_tool_search_refuses_discovery() {
+    let model = Arc::new(ScriptedModel::new(vec![
+        calls(vec![("s1", "tool_search", json!({"query": "deferred"}))]),
+        ModelResponse::assistant("done"),
+    ]));
+    let policy = ToolRulePolicy::new(rules(json!({ "rules": [
+        { "effect": "require_approval", "match": { "name": "tool_search" } },
+    ] })));
+    let mut harness = harness_with(model.clone(), policy);
+    harness.register_tool(RuleTool::deferred("deferred_open"));
+
+    let run = run(&harness, "search-approval").await;
+
+    let answer = tool_text(&run.messages, "s1");
+    assert!(answer.contains("requires approval"), "{answer}");
+    assert!(
+        !tool_names(&model.requests()[0]).contains(&"tool_search".to_string()),
+        "an approval-gated bridge is not advertised"
+    );
+    assert!(!answer.contains("deferred_open"), "{answer}");
+}
+
+#[tokio::test]
+async fn unknown_tool_recovery_does_not_point_at_a_denied_tool_search() {
+    let model = Arc::new(ScriptedModel::new(vec![
+        calls(vec![("c1", "nope", json!({}))]),
+        ModelResponse::assistant("done"),
+    ]));
+    let policy = ToolRulePolicy::new(rules(json!({ "rules": [
+        { "effect": "deny", "match": { "name": "tool_search" } },
+    ] })));
+    let mut harness = harness_with(model, policy);
+    harness.register_tool(RuleTool::deferred("deferred_open"));
+
+    let run = run(&harness, "unknown").await;
+
+    assert!(!tool_text(&run.messages, "c1").contains("tool_search"));
+}

crates/tinyagents-harness/src/agent_loop/tool_surface.rs

@@ -42,8 +42,11 @@ impl<State: Send + Sync, Ctx: Send + Sync> AgentHarness<State, Ctx> {
             })
             .collect::<Vec<_>>();
         if let Some(toolset) = &self.toolset {
-            let existing: HashSet<&str> =
-                schemas.iter().map(|schema| schema.name.as_str()).collect();
+            // Every registered name, listed or not: a registered tool owns its
+            // name even when the rules keep it off this catalogue, so a
+            // toolset tool may never take its place on the wire.
+            let registered = self.tools.names();
+            let existing: HashSet<&str> = registered.iter().map(String::as_str).collect();
             let extra: Vec<_> = toolset
                 .tools(ctx)
                 .await?
@@ -102,7 +105,21 @@ impl<State: Send + Sync, Ctx: Send + Sync> AgentHarness<State, Ctx> {
                 .collect();
         let promoted_names: BTreeSet<String> = promoted_schemas.keys().cloned().collect();
         let recorded_promotions = promoted_names.clone();
-        if !deferred_catalog.is_empty() {
+        // Listed only when a call to it would be answered: hidden or denied
+        // on the catalogue, or approval-gated (the bridge is answered in place
+        // and cannot be deferred to an approver), keeps it off the wire.
+        let search = crate::tool::discover::TOOL_SEARCH_NAME;
+        let answerable = |surface| {
+            matches!(
+                gate.admits_intrinsic(search, surface),
+                crate::tool::CallGate::Admit(
+                    tinytools::ApprovalDirective::Default | tinytools::ApprovalDirective::Waived
+                )
+            )
+        };
+        let bridge_listed =
+            answerable(tinytools::Surface::Catalog) && answerable(tinytools::Surface::Call);
+        if bridge_listed && !deferred_catalog.is_empty() {
             // A host-registered `tool_search` keeps its slot: the intrinsic
             // bridge only fills a name nobody registered. Check the full
             // registry (`self.tools.dispatch`), not just the direct set — a