bxcore, brokerd, loopd: an invalid-arguments denial says why

Co-Authored-By: ornith-1.5-35b-a3b <agent@wntrmute.dev>

crates/brokerd/src/args.rs

@@ -12,6 +12,8 @@ use std::fmt;
 pub const MAX_PATH: usize = 4096;
 /// Maximum length, in bytes, of a URL.
 pub const MAX_URL: usize = 2048;
+/// Maximum length, in characters, of the detail an invalid call is denied with.
+pub const MAX_DETAIL: usize = 300;
 
 /// The tools the broker can dispatch to: four that run in a container, and the two memory
 /// tools that `memoryd` runs (M5a spec, section 8).
@@ -190,6 +192,25 @@ impl fmt::Display for ArgsError {
     }
 }
 
+impl ArgsError {
+    /// The error as the model is shown it: the `Display` text on one line, each control character
+    /// and each Unicode line or paragraph separator replaced by a space, cut to `MAX_DETAIL`
+    /// characters. The text holds the model's own arguments, which may be long or hold newlines.
+    pub fn detail(&self) -> String {
+        self.to_string()
+            .chars()
+            .map(|c| {
+                if c.is_control() || c == '\u{2028}' || c == '\u{2029}' {
+                    ' '
+                } else {
+                    c
+                }
+            })
+            .take(MAX_DETAIL)
+            .collect()
+    }
+}
+
 impl std::error::Error for ArgsError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         None

crates/brokerd/src/broker.rs

@@ -251,7 +251,10 @@ pub fn handle(mut stream: UnixStream, broker: &Broker) {
     let now = Timestamp::now();
     let grants = broker.grants();
     let answer = match broker.ledger().decide(request, &grants, now) {
-        Decided::Denied(reason) => Some(ToolResponse::Denied { reason }), // 3a. denied
+        Decided::Denied(denial) => Some(ToolResponse::Denied {
+            reason: denial.reason,
+            detail: denial.detail,
+        }), // 3a. denied
         Decided::Allowed { decision, seq } => Some(run(broker, decision, seq)), // 3b. allowed
         Decided::Ask { ask, seq, state } => {
             pending(&mut stream, broker, id, ask, seq, state, now) // 3c. ask
@@ -334,7 +337,10 @@ fn pending(
     // 5. Answer from the verdict.
     match outcome {
         Ok(verdict) => match verdict {
-            Verdict::Denied(reason) => Some(ToolResponse::Denied { reason }),
+            Verdict::Denied(reason) => Some(ToolResponse::Denied {
+                reason,
+                detail: None,
+            }),
             Verdict::Run(decision) => {
                 let decision = *decision;
                 if !alive(stream) {
@@ -350,6 +356,9 @@ fn pending(
                 Some(run(broker, decision, seq))
             }
         },
-        Err(reason) => Some(ToolResponse::Denied { reason }),
+        Err(reason) => Some(ToolResponse::Denied {
+            reason,
+            detail: None,
+        }),
     }
 }

crates/brokerd/src/ledger/mod.rs

@@ -62,7 +62,8 @@ impl Call {
     }
 }
 
-/// How a call was decided.
+/// How a call was decided. A denial carries the policy's `Denial`, so an invalid call keeps its
+/// detail on the way to the answer.
 #[derive(Debug)]
 pub enum Decided {
     Allowed {
@@ -74,7 +75,7 @@ pub enum Decided {
         seq: u64,
         state: SessionState,
     },
-    Denied(DenyReason),
+    Denied(Denial),
 }
 
 /// The value a decided call carries past the record, or the denial reason when there is none.
@@ -210,7 +211,7 @@ impl Ledger {
         // 1. Lock fails → denied, nothing written.
         let mut guard = match self.lock() {
             Some(guard) => guard,
-            None => return Decided::Denied(DenyReason::AuditUnavailable),
+            None => return Decided::Denied(Denial::new(DenyReason::AuditUnavailable)),
         };
 
         // 2. Read the state, keeping the Result. The recorded state is "unknown" when unreadable.
@@ -281,16 +282,16 @@ impl Ledger {
         };
         match self.append(&mut guard, now, event) {
             Ok(seq) => match payload {
-                RecordPayload::Simple(reason) => Decided::Denied(reason),
+                RecordPayload::Simple(reason) => Decided::Denied(Denial::new(reason)),
                 RecordPayload::Allowed(decision) => Decided::Allowed { decision, seq },
                 RecordPayload::Ask(ask) => Decided::Ask {
                     ask,
                     seq,
                     state: recorded,
                 },
-                RecordPayload::Denial(denial) => Decided::Denied(denial.reason),
+                RecordPayload::Denial(denial) => Decided::Denied(denial),
             },
-            Err(()) => Decided::Denied(DenyReason::AuditUnavailable),
+            Err(()) => Decided::Denied(Denial::new(DenyReason::AuditUnavailable)),
         }
     }
 }

crates/brokerd/src/policy/mod.rs

@@ -120,6 +120,9 @@ pub struct Denial {
     pub grant: Option<String>,
     /// `Some` only for `DeniedByGrant`: the deny grant's file hash.
     pub grant_sha256: Option<Hash32>,
+    /// `Some` only for `InvalidArguments`: why, from `ArgsError::detail`. It goes to the model in
+    /// the `ToolResponse`, never into the audit record, which holds the arguments already.
+    pub detail: Option<String>,
 }
 
 impl Denial {
@@ -128,6 +131,7 @@ impl Denial {
             reason,
             grant: None,
             grant_sha256: None,
+            detail: None,
         }
     }
 }
@@ -244,9 +248,16 @@ pub fn decide(
     let Some(tool) = ToolName::parse(&request.tool) else {
         return Outcome::Denied(Denial::new(DenyReason::NoGrant));
     };
-    // 2. Malformed arguments are refused before matching, whatever the grants say.
-    let Ok(args) = args::parse(tool, &request.arguments) else {
-        return Outcome::Denied(Denial::new(DenyReason::InvalidArguments));
+    // 2. Malformed arguments are refused before matching, whatever the grants say, and the
+    //    model is told why, so that it can correct the call.
+    let args = match args::parse(tool, &request.arguments) {
+        Ok(args) => args,
+        Err(error) => {
+            return Outcome::Denied(Denial {
+                detail: Some(error.detail()),
+                ..Denial::new(DenyReason::InvalidArguments)
+            });
+        }
     };
 
     let MatchResult {
@@ -273,6 +284,7 @@ pub fn decide(
             reason: DenyReason::DeniedByGrant,
             grant: Some(winner.id.clone()),
             grant_sha256: Some(winner.grant_sha256),
+            detail: None,
         }),
         Mode::Ask => Outcome::Ask(Ask {
             request,
@@ -316,6 +328,7 @@ pub fn redecide(
             reason: DenyReason::DeniedByGrant,
             grant: Some(winner.id.clone()),
             grant_sha256: Some(winner.grant_sha256),
+            detail: None,
         }),
         Mode::Ask | Mode::Auto => Ok(Decision {
             request: ask.request().clone(),

crates/brokerd/tests/admin.rs

@@ -83,7 +83,10 @@ fn error_code(msg: &Message) -> Option<ErrorCode> {
 }
 
 fn denied(reason: DenyReason) -> ToolResponse {
-    ToolResponse::Denied { reason }
+    ToolResponse::Denied {
+        reason,
+        detail: None,
+    }
 }
 
 #[test]

crates/brokerd/tests/broker.rs

@@ -45,7 +45,10 @@ fn only(frames: &[Envelope]) -> &ToolResponse {
 }
 
 fn denied(reason: DenyReason) -> ToolResponse {
-    ToolResponse::Denied { reason }
+    ToolResponse::Denied {
+        reason,
+        detail: None,
+    }
 }
 
 fn auto_notes(rig: &Rig, class: &str) {

crates/brokerd/tests/broker_pending.rs

@@ -174,7 +174,8 @@ fn a_denied_verdict_reaches_the_requester_and_nothing_runs() {
     assert_eq!(
         final_answer(&mut stream),
         ToolResponse::Denied {
-            reason: DenyReason::ApprovalRefused
+            reason: DenyReason::ApprovalRefused,
+            detail: None,
         }
     );
     assert_eq!(rt.count(), 0);

crates/brokerd/tests/courier.rs

@@ -154,7 +154,8 @@ fn refused_in_mattermost_it_is_denied_with_the_reason() {
     assert_eq!(
         final_answer(&mut stream),
         ToolResponse::Denied {
-            reason: DenyReason::ApprovalRefused
+            reason: DenyReason::ApprovalRefused,
+            detail: None,
         }
     );
     assert_eq!(rt.count(), 0);
@@ -188,7 +189,8 @@ fn approved_after_the_grant_is_gone_it_is_denied_on_the_recheck() {
     assert_eq!(
         final_answer(&mut stream),
         ToolResponse::Denied {
-            reason: DenyReason::NoGrant
+            reason: DenyReason::NoGrant,
+            detail: None,
         }
     );
     assert_eq!(rt.count(), 0);

crates/brokerd/tests/invalid_detail.rs

@@ -0,0 +1,258 @@
+//! An invalid call is denied with the parser's reason: `ArgsError::detail` makes it one short
+//! line, `policy::decide` keeps it, and `broker.sock` sends it. The audit record is unchanged.
+//! Do not edit.
+
+#[path = "support/build.rs"]
+mod build;
+#[path = "support/client.rs"]
+mod client;
+#[path = "support/rig.rs"]
+mod rig;
+#[path = "support/runtime.rs"]
+mod runtime;
+#[path = "support/sink.rs"]
+mod sink;
+#[path = "support/tmp.rs"]
+mod tmp;
+
+use brokerd::args::{ArgsError, MAX_DETAIL};
+use brokerd::policy::decide;
+use build::{denied, fetch, grant, now, private, read, set, shell};
+use bxcore::{AuditEvent, DecisionRecord, DenyReason, Envelope, Message, Mode, ToolResponse};
+use client::{Serve, call};
+use rig::{Rig, grant_text};
+use runtime::Recording;
+
+#[test]
+fn the_detail_is_the_error_text() {
+    assert_eq!(MAX_DETAIL, 300);
+    let cases = [
+        (
+            ArgsError::Shape("missing field `path`".to_string()),
+            "invalid argument shape: missing field `path`",
+        ),
+        (
+            ArgsError::Path("notes/a.md".to_string()),
+            "invalid path: notes/a.md",
+        ),
+        (
+            ArgsError::Url("http://example.com/".to_string()),
+            "invalid url: http://example.com/",
+        ),
+    ];
+    for (error, want) in cases {
+        assert_eq!(error.detail(), want);
+        assert_eq!(
+            error.detail(),
+            error.to_string(),
+            "a short clean text is kept as it is"
+        );
+    }
+}
+
+/// Every control character, and U+2028 and U+2029 (the line and paragraph separators), becomes
+/// one space each; other characters, spaces and non-ASCII letters included, are kept.
+#[test]
+fn the_detail_is_one_line() {
+    let error = ArgsError::Path(
+        "a\nb\r\nc\td\u{7}e\u{1b}[31mf\u{7f}g\u{85}h\u{2028}i\u{2029}j k \u{e9}\u{65e5}"
+            .to_string(),
+    );
+    assert_eq!(
+        error.detail(),
+        "invalid path: a b  c d e [31mf g h i j k \u{e9}\u{65e5}"
+    );
+    let error = ArgsError::Shape("line one\nline two\n".to_string());
+    assert_eq!(error.detail(), "invalid argument shape: line one line two ");
+}
+
+/// At most `MAX_DETAIL` characters, counted as characters, not bytes: a cut never splits one.
+#[test]
+fn the_detail_is_cut_to_300_characters() {
+    // "invalid path: " is 14 characters.
+    let exact = ArgsError::Path("a".repeat(286));
+    assert_eq!(
+        exact.detail(),
+        exact.to_string(),
+        "300 characters are kept whole"
+    );
+    let over = ArgsError::Path("a".repeat(287));
+    assert_eq!(over.detail(), format!("invalid path: {}", "a".repeat(286)));
+
+    let wide = ArgsError::Path("\u{e9}".repeat(1000));
+    let detail = wide.detail();
+    assert_eq!(detail.chars().count(), 300);
+    assert_eq!(detail, format!("invalid path: {}", "\u{e9}".repeat(286)));
+
+    // Newlines past the cut do not matter; newlines before it are spaces.
+    let long = ArgsError::Shape(format!("{}\n{}", "x".repeat(10), "y\n".repeat(500)));
+    let detail = long.detail();
+    assert_eq!(detail.chars().count(), 300);
+    assert!(!detail.contains('\n'), "{detail:?}");
+    assert!(detail.starts_with("invalid argument shape: xxxxxxxxxx y y"));
+}
+
+/// Each kind of `ArgsError`, from the model's own argument text, reaches the denial as its detail,
+/// with no grant and with a grant that would cover the call: arguments are checked first. (The
+/// memory tools take no grant: their policy is built in.)
+#[test]
+fn each_kind_of_invalid_arguments_carries_its_detail() {
+    let covering = set(vec![
+        grant("r", "read_file", Mode::Auto).paths(&["/n"]),
+        grant("s", "shell", Mode::Auto),
+        grant("f", "http_fetch", Mode::Auto).hosts(&["example.com"]),
+    ]);
+    let none = set(vec![]);
+    let cases = [
+        (
+            build::request(
+                "memory",
+                r#"{"action":"note","file":"notes.md","text":"x"}"#,
+            ),
+            "invalid argument shape: unknown field `file`, expected `text`",
+        ),
+        (
+            build::request("read_file", r#"{"path":"/n/a","file":"x"}"#),
+            "invalid argument shape: unknown field `file`, expected `path` at line 1 column 21",
+        ),
+        (
+            build::request("read_file", "not json"),
+            "invalid argument shape: expected ident at line 1 column 2",
+        ),
+        (
+            build::request("read_file", "{}"),
+            "invalid argument shape: missing field `path` at line 1 column 2",
+        ),
+        (
+            build::request("recall", r#"{"query":""}"#),
+            "invalid argument shape: query must have 1 to 500 characters",
+        ),
+        (read("n/a"), "invalid path: n/a"),
+        (shell(Some("a\\nb")), "invalid path: a b"),
+        (
+            fetch("http://example.com/"),
+            "invalid url: http://example.com/",
+        ),
+    ];
+    for grants in [&none, &covering] {
+        for (req, want) in &cases {
+            let text = req.arguments.clone();
+            let denial = denied(decide(req.clone(), grants, private(), now()));
+            assert_eq!(denial.reason, DenyReason::InvalidArguments, "{text}");
+            assert_eq!(denial.detail.as_deref(), Some(*want), "{text}");
+            assert_eq!(denial.grant, None, "{text}");
+        }
+    }
+}
+
+#[test]
+fn a_long_invalid_path_is_cut_in_the_denial() {
+    let path = format!("/{}", "a".repeat(5000));
+    let denial = denied(decide(read(&path), &set(vec![]), private(), now()));
+    assert_eq!(denial.reason, DenyReason::InvalidArguments);
+    let detail = denial.detail.unwrap();
+    assert_eq!(detail.chars().count(), 300);
+    assert_eq!(detail, format!("invalid path: /{}", "a".repeat(285)));
+}
+
+/// Only an invalid-arguments denial has a detail.
+#[test]
+fn other_denials_have_no_detail() {
+    let grants = set(vec![
+        grant("no", "read_file", Mode::Deny).paths(&["/n"]),
+        grant("old", "shell", Mode::Auto).expires("2026-09-01T00:00:00.000Z"),
+    ]);
+    let cases = [
+        (build::request("echo", "not json"), DenyReason::NoGrant),
+        (fetch("https://example.com/"), DenyReason::NoGrant),
+        (read("/n/a"), DenyReason::DeniedByGrant),
+        (shell(None), DenyReason::GrantExpired),
+    ];
+    for (req, reason) in cases {
+        let text = req.arguments.clone();
+        let denial = denied(decide(req, &grants, private(), now()));
+        assert_eq!(denial.reason, reason, "{text}");
+        assert_eq!(denial.detail, None, "{text}");
+    }
+}
+
+fn only(frames: &[Envelope]) -> &ToolResponse {
+    assert_eq!(frames.len(), 1, "{frames:?}");
+    assert!(frames[0].r#final);
+    match &frames[0].msg {
+        Message::ToolResponse(r) => r,
+        other => panic!("{other:?}"),
+    }
+}
+
+#[test]
+fn broker_sock_sends_the_detail_and_the_audit_record_does_not_hold_it() {
+    let rig = Rig::new("broker-invalid-detail");
+    rig.grant(
+        "notes",
+        &grant_text("read_file", "auto", "", "paths = [\"/n\"]"),
+    );
+    let rt = Recording::answering("x");
+    let broker = rig.broker(&rt);
+    let arguments = r#"{"path":"/n/a","file":"x"}"#;
+    let frames = call(&broker, rig::request("s1", 5, "read_file", arguments));
+    let detail =
+        "invalid argument shape: unknown field `file`, expected `path` at line 1 column 21";
+    assert_eq!(
+        only(&frames),
+        &ToolResponse::Denied {
+            reason: DenyReason::InvalidArguments,
+            detail: Some(detail.to_string()),
+        }
+    );
+    assert_eq!(rt.count(), 0, "nothing ran");
+    match rig.events().as_slice() {
+        [
+            AuditEvent::Decision {
+                outcome,
+                arguments: recorded,
+                ..
+            },
+        ] => {
+            assert_eq!(
+                *outcome,
+                DecisionRecord::Denied {
+                    reason: DenyReason::InvalidArguments
+                }
+            );
+            assert_eq!(recorded, arguments, "the record keeps the arguments");
+        }
+        other => panic!("{other:?}"),
+    }
+    let audit = rig.cfg.audit_dir();
+    let mut logs = 0;
+    for entry in std::fs::read_dir(&audit).unwrap() {
+        let path = entry.unwrap().path();
+        if path.extension().is_none_or(|e| e != "jsonl") {
+            continue;
+        }
+        logs += 1;
+        let text = std::fs::read_to_string(&path).unwrap();
+        assert!(!text.contains("unknown field"), "{text}");
+        assert!(!text.contains("\"detail\""), "{text}");
+    }
+    assert_eq!(logs, 1, "one audit log file");
+}
+
+#[test]
+fn a_call_no_grant_allows_is_denied_on_broker_sock_without_a_detail() {
+    let rig = Rig::new("broker-no-detail");
+    let rt = Recording::answering("x");
+    let broker = rig.broker(&rt);
+    let frames = call(
+        &broker,
+        rig::request("s1", 6, "read_file", r#"{"path":"/n/a"}"#),
+    );
+    assert_eq!(
+        only(&frames),
+        &ToolResponse::Denied {
+            reason: DenyReason::NoGrant,
+            detail: None,
+        }
+    );
+}

crates/brokerd/tests/ledger.rs

@@ -14,7 +14,7 @@ use std::os::unix::fs::PermissionsExt;
 use std::panic::AssertUnwindSafe;
 
 use brokerd::ledger::{Call, Decided, Grants, NOT_RECORDED, POISONED, STOPPED};
-use brokerd::policy::{Label, SessionState};
+use brokerd::policy::{Denial, Label, SessionState};
 use build::{grant, now, read, set};
 use bxcore::{
     AuditEvent, CallId, DataClass, DecisionRecord, DenyReason, GrantProblem, Mode, ResultStatus,
@@ -113,13 +113,25 @@ fn a_denial_names_a_grant_only_when_a_grant_denied() {
     let ledger = rig.ledger();
     let none = ledger.decide(read("/n/a"), &one(Vec::new()), now());
     assert!(
-        matches!(none, Decided::Denied(DenyReason::NoGrant)),
+        matches!(
+            none,
+            Decided::Denied(Denial {
+                reason: DenyReason::NoGrant,
+                ..
+            })
+        ),
         "{none:?}"
     );
     let deny = one(vec![grant("d", "read_file", Mode::Deny).paths(&["/n"])]);
     let by = ledger.decide(read("/n/a"), &deny, now());
     assert!(
-        matches!(by, Decided::Denied(DenyReason::DeniedByGrant)),
+        matches!(
+            by,
+            Decided::Denied(Denial {
+                reason: DenyReason::DeniedByGrant,
+                ..
+            })
+        ),
         "{by:?}"
     );
     let events = rig.events();
@@ -162,12 +174,24 @@ fn invalid_grants_come_first_then_an_unreadable_state() {
     }]);
     let first = ledger.decide(read("/n/a"), &invalid, now());
     assert!(
-        matches!(first, Decided::Denied(DenyReason::GrantsInvalid)),
+        matches!(
+            first,
+            Decided::Denied(Denial {
+                reason: DenyReason::GrantsInvalid,
+                ..
+            })
+        ),
         "{first:?}"
     );
     let second = ledger.decide(read("/n/a"), &notes(Mode::Auto), now());
     assert!(
-        matches!(second, Decided::Denied(DenyReason::StateUnreadable)),
+        matches!(
+            second,
+            Decided::Denied(Denial {
+                reason: DenyReason::StateUnreadable,
+                ..
+            })
+        ),
         "{second:?}"
     );
     // A state brokerd cannot read is recorded as the most sensitive one.
@@ -190,7 +214,13 @@ fn a_failed_append_denies_this_call_and_every_later_one() {
     rig.switch.fail(true);
     let first = ledger.decide(read("/n/a"), &notes(Mode::Auto), now());
     assert!(
-        matches!(first, Decided::Denied(DenyReason::AuditUnavailable)),
+        matches!(
+            first,
+            Decided::Denied(Denial {
+                reason: DenyReason::AuditUnavailable,
+                ..
+            })
+        ),
         "{first:?}"
     );
     assert_eq!(rig.switch.attempts(), 1);
@@ -198,7 +228,13 @@ fn a_failed_append_denies_this_call_and_every_later_one() {
     rig.switch.fail(false);
     let later = ledger.decide(read("/n/a"), &notes(Mode::Auto), now());
     assert!(
-        matches!(later, Decided::Denied(DenyReason::AuditUnavailable)),
+        matches!(
+            later,
+            Decided::Denied(Denial {
+                reason: DenyReason::AuditUnavailable,
+                ..
+            })
+        ),
         "{later:?}"
     );
     let finished = ledger.finish(&call(secret()), result("x", secret()), now());
@@ -233,7 +269,13 @@ fn a_panic_while_holding_the_ledger_denies_every_later_call() {
     assert!(panicked.is_err());
     let later = ledger.decide(read("/n/a"), &notes(Mode::Auto), now());
     assert!(
-        matches!(later, Decided::Denied(DenyReason::AuditUnavailable)),
+        matches!(
+            later,
+            Decided::Denied(Denial {
+                reason: DenyReason::AuditUnavailable,
+                ..
+            })
+        ),
         "{later:?}"
     );
     assert_eq!(rig.switch.attempts(), 1);

crates/brokerd/tests/serve.rs

@@ -210,7 +210,8 @@ fn a_stale_socket_is_replaced() {
     assert_eq!(
         last_response(&frames),
         &ToolResponse::Denied {
-            reason: DenyReason::NoGrant
+            reason: DenyReason::NoGrant,
+            detail: None,
         }
     );
 }
@@ -287,7 +288,8 @@ fn an_approval_nobody_answers_expires() {
     assert_eq!(
         last_response(&frames),
         &ToolResponse::Denied {
-            reason: DenyReason::ApprovalExpired
+            reason: DenyReason::ApprovalExpired,
+            detail: None,
         }
     );
     // The expiry thread looks every second.