diff --git a/crates/brokerd/src/args.rs b/crates/brokerd/src/args.rs index ba8b183..e052297 100644 --- a/crates/brokerd/src/args.rs +++ b/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 diff --git a/crates/brokerd/src/broker.rs b/crates/brokerd/src/broker.rs index 375d0ab..1c04c39 100644 --- a/crates/brokerd/src/broker.rs +++ b/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, + }), } } diff --git a/crates/brokerd/src/ledger/mod.rs b/crates/brokerd/src/ledger/mod.rs index 18c962d..a4010bd 100644 --- a/crates/brokerd/src/ledger/mod.rs +++ b/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)), } } } diff --git a/crates/brokerd/src/policy/mod.rs b/crates/brokerd/src/policy/mod.rs index aa43850..08bae2e 100644 --- a/crates/brokerd/src/policy/mod.rs +++ b/crates/brokerd/src/policy/mod.rs @@ -120,6 +120,9 @@ pub struct Denial { pub grant: Option, /// `Some` only for `DeniedByGrant`: the deny grant's file hash. pub grant_sha256: Option, + /// `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, } 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(), diff --git a/crates/brokerd/tests/admin.rs b/crates/brokerd/tests/admin.rs index cee1522..7af5e87 100644 --- a/crates/brokerd/tests/admin.rs +++ b/crates/brokerd/tests/admin.rs @@ -83,7 +83,10 @@ fn error_code(msg: &Message) -> Option { } fn denied(reason: DenyReason) -> ToolResponse { - ToolResponse::Denied { reason } + ToolResponse::Denied { + reason, + detail: None, + } } #[test] diff --git a/crates/brokerd/tests/broker.rs b/crates/brokerd/tests/broker.rs index a7b2bea..fc31dcb 100644 --- a/crates/brokerd/tests/broker.rs +++ b/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) { diff --git a/crates/brokerd/tests/broker_pending.rs b/crates/brokerd/tests/broker_pending.rs index d0dc540..81ea335 100644 --- a/crates/brokerd/tests/broker_pending.rs +++ b/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); diff --git a/crates/brokerd/tests/courier.rs b/crates/brokerd/tests/courier.rs index 9480625..625de1f 100644 --- a/crates/brokerd/tests/courier.rs +++ b/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); diff --git a/crates/brokerd/tests/invalid_detail.rs b/crates/brokerd/tests/invalid_detail.rs new file mode 100644 index 0000000..a482cdc --- /dev/null +++ b/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, + } + ); +} diff --git a/crates/brokerd/tests/ledger.rs b/crates/brokerd/tests/ledger.rs index 66dd7eb..b9b8e59 100644 --- a/crates/brokerd/tests/ledger.rs +++ b/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"), ¬es(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"), ¬es(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"), ¬es(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"), ¬es(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); diff --git a/crates/brokerd/tests/serve.rs b/crates/brokerd/tests/serve.rs index 54ad05d..bcdffac 100644 --- a/crates/brokerd/tests/serve.rs +++ b/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. diff --git a/crates/bxcore/src/wire.rs b/crates/bxcore/src/wire.rs index 4e0ca8a..e8456de 100644 --- a/crates/bxcore/src/wire.rs +++ b/crates/bxcore/src/wire.rs @@ -104,6 +104,10 @@ pub enum ToolResponse { }, Denied { reason: DenyReason, + /// Why the arguments were not valid, for `invalid_arguments` only. Written only when + /// present, so a denial without one has the same bytes as before M5b. + #[serde(default, skip_serializing_if = "Option::is_none")] + detail: Option, }, } diff --git a/crates/bxcore/tests/fixtures/wire/tool_response_denied_detail.json b/crates/bxcore/tests/fixtures/wire/tool_response_denied_detail.json new file mode 100644 index 0000000..7bffeb0 --- /dev/null +++ b/crates/bxcore/tests/fixtures/wire/tool_response_denied_detail.json @@ -0,0 +1 @@ +{"v":1,"id":9,"final":true,"msg":{"kind":"tool_response","body":{"status":"denied","reason":"invalid_arguments","detail":"invalid argument shape: unknown field `file`, expected `text`"}}} diff --git a/crates/bxcore/tests/strict.rs b/crates/bxcore/tests/strict.rs index dc847e4..c71c325 100644 --- a/crates/bxcore/tests/strict.rs +++ b/crates/bxcore/tests/strict.rs @@ -68,6 +68,7 @@ fn envelopes_reject_unknown_keys_at_every_depth() { "tool_response_result.json", "tool_response_failed.json", "tool_response_denied.json", + "tool_response_denied_detail.json", "error.json", "turn.json", "turn_event_tool_result.json", diff --git a/crates/bxcore/tests/wire.rs b/crates/bxcore/tests/wire.rs index d926348..9d2e977 100644 --- a/crates/bxcore/tests/wire.rs +++ b/crates/bxcore/tests/wire.rs @@ -94,10 +94,35 @@ fn tool_response_failed() { fn tool_response_denied() { let body = ToolResponse::Denied { reason: DenyReason::TaintTooHigh, + detail: None, }; check("tool_response_denied.json", response(9, true, body)); } +/// A detail is written only when there is one, so the fixture above has no `detail` key. +#[test] +fn tool_response_denied_with_a_detail() { + let body = ToolResponse::Denied { + reason: DenyReason::InvalidArguments, + detail: Some("invalid argument shape: unknown field `file`, expected `text`".to_string()), + }; + check("tool_response_denied_detail.json", response(9, true, body)); +} + +#[test] +fn a_detail_must_be_a_string() { + let good = fixture("tool_response_denied_detail.json"); + assert!(serde_json::from_str::(&good).is_ok()); + let at = good.find("\"detail\":").unwrap(); + for value in ["7", "[\"a\"]", "{}", "true"] { + let bad = format!("{}\"detail\":{value}}}}}}}", &good[..at]); + assert!( + serde_json::from_str::(&bad).is_err(), + "accepted {bad}" + ); + } +} + #[test] fn error_message() { let body = WireError { diff --git a/crates/loopd/src/tools.rs b/crates/loopd/src/tools.rs index a019c6d..04fcc81 100644 --- a/crates/loopd/src/tools.rs +++ b/crates/loopd/src/tools.rs @@ -378,6 +378,19 @@ pub fn denial_text(reason: bxcore::DenyReason) -> &'static str { } } +/// What a denial reaches the model as: the reason's sentence, and the broker's detail, if it sent +/// one, after a colon (`Denied: the arguments are not valid for this tool: .`). +pub fn denial_message(reason: bxcore::DenyReason, detail: Option<&str>) -> String { + let sentence = denial_text(reason); + match detail { + None => sentence.to_string(), + Some(detail) => { + let stem = sentence.strip_suffix('.').unwrap_or(sentence); + format!("{stem}: {detail}.") + } + } +} + pub struct FakeTools { calls: Mutex>, } @@ -432,6 +445,7 @@ impl ToolPort for FakeTools { }, _ => bxcore::ToolResponse::Denied { reason: bxcore::DenyReason::NoGrant, + detail: None, }, } } diff --git a/crates/loopd/src/turn.rs b/crates/loopd/src/turn.rs index fc79dcc..3c13b7a 100644 --- a/crates/loopd/src/turn.rs +++ b/crates/loopd/src/turn.rs @@ -7,7 +7,7 @@ use crate::config::Config; use crate::llama::info::{CacheOutcome, cache_outcome}; use crate::llama::{ChatEvent, ChatRequest, Client, InferError}; use crate::session::{Session, SessionError}; -use crate::tools::{Registry, ToolPort, cap_result, denial_text, dispatch}; +use crate::tools::{Registry, ToolPort, cap_result, denial_message, dispatch}; use bxcore::{ CallId, DataClass, LogRecord, Timestamp, ToolRequest, ToolResponse, TurnEvent, Usage, }; @@ -292,12 +292,16 @@ fn run_call( DataClass::Public, false, ), - ToolResponse::Denied { reason } => { + ToolResponse::Denied { reason, detail } => { on_event(&TurnEvent::ToolDenied { name: request.tool.clone(), reason, }); - (denial_text(reason).to_string(), DataClass::Public, false) + ( + denial_message(reason, detail.as_deref()), + DataClass::Public, + false, + ) } ToolResponse::PendingApproval { .. } => ( "The tool failed: the tool broker gave no final answer".to_string(), diff --git a/crates/loopd/tests/broker_port.rs b/crates/loopd/tests/broker_port.rs index 4ba2ad3..b1981bf 100644 --- a/crates/loopd/tests/broker_port.rs +++ b/crates/loopd/tests/broker_port.rs @@ -48,8 +48,15 @@ fn every_denial_and_a_failure_come_back_as_they_were_sent() { DenyReason::StateUnreadable, ] .into_iter() - .map(|reason| ToolResponse::Denied { reason }) + .map(|reason| ToolResponse::Denied { + reason, + detail: None, + }) .collect(); + answers.push(ToolResponse::Denied { + reason: DenyReason::InvalidArguments, + detail: Some("invalid path: n/a".to_string()), + }); answers.push(ToolResponse::Failed { message: "the runner arrives in M3b".to_string(), }); diff --git a/crates/loopd/tests/tools.rs b/crates/loopd/tests/tools.rs index ab3c73f..2278fa2 100644 --- a/crates/loopd/tests/tools.rs +++ b/crates/loopd/tests/tools.rs @@ -5,7 +5,8 @@ mod support; use bxcore::{CallId, DenyReason, SessionId, ToolRequest, ToolResponse}; use loopd::tools::{ - Dispatch, FakeTools, Pending, Registry, ToolPort, cap_result, denial_text, dispatch, + Dispatch, FakeTools, Pending, Registry, ToolPort, cap_result, denial_message, denial_text, + dispatch, }; fn req(tool: &str, arguments: &str) -> ToolRequest { @@ -252,9 +253,35 @@ fn every_deny_reason_has_its_sentence() { ]; for (reason, want) in table { assert_eq!(denial_text(reason), want, "{reason:?}"); + assert_eq!( + denial_message(reason, None), + want, + "{reason:?}: without a detail, the sentence alone" + ); } } +/// The broker's detail goes after the sentence, before its full stop. +#[test] +fn a_denial_with_a_detail_appends_it_to_the_sentence() { + assert_eq!( + denial_message( + DenyReason::InvalidArguments, + Some("invalid argument shape: unknown field `file`, expected `text`") + ), + "Denied: the arguments are not valid for this tool: invalid argument shape: unknown field `file`, expected `text`." + ); + assert_eq!( + denial_message(DenyReason::InvalidArguments, Some("invalid path: n/a")), + "Denied: the arguments are not valid for this tool: invalid path: n/a." + ); + assert_eq!( + denial_message(DenyReason::NoGrant, Some("x")), + "Denied: no grant allows this call: x.", + "any reason's detail is shown the same way" + ); +} + #[test] fn results_are_cut_on_a_character_boundary_and_marked() { assert_eq!(cap_result("short", 100), ("short".to_string(), false)); @@ -296,7 +323,8 @@ fn fake_tools_answer_echo_deny_the_rest_and_record_calls() { assert_eq!( fake.call(&req(tool, "{}"), &mut on_pending), ToolResponse::Denied { - reason: DenyReason::NoGrant + reason: DenyReason::NoGrant, + detail: None, }, "{tool}: the clock is loopd's own now, not the port's" ); diff --git a/crates/loopd/tests/turn_broker.rs b/crates/loopd/tests/turn_broker.rs index 61a2236..66e9672 100644 --- a/crates/loopd/tests/turn_broker.rs +++ b/crates/loopd/tests/turn_broker.rs @@ -31,6 +31,7 @@ fn tool_events(events: &[TurnEvent]) -> Vec { fn a_denial_is_a_fixed_sentence_for_the_model_and_an_event_for_the_owner() { let s = setup(vec![ToolResponse::Denied { reason: DenyReason::TaintTooHigh, + detail: None, }]); s.server.route( CHAT, @@ -78,10 +79,55 @@ fn a_denial_is_a_fixed_sentence_for_the_model_and_an_event_for_the_owner() { ); } +/// The broker's detail reaches the model, in the log and in the next request; the owner's event +/// is the same as for any denial. +#[test] +fn an_invalid_call_tells_the_model_why() { + let s = setup(vec![ToolResponse::Denied { + reason: DenyReason::InvalidArguments, + detail: Some("invalid path: n/a".to_string()), + }]); + s.server.route( + CHAT, + vec![Reply::fixture("tool_call"), Reply::fixture("plain")], + ); + let mut session = s.session("a"); + let (result, events) = s.turn(&mut session, "x"); + assert!(result.is_ok(), "{result:?}"); + let want = "Denied: the arguments are not valid for this tool: invalid path: n/a."; + match &session.records()[4] { + LogRecord::ToolResult { + content, + class, + untrusted, + truncated, + .. + } => { + assert_eq!(content, want); + assert_eq!( + (*class, *untrusted, *truncated), + (DataClass::Public, false, false) + ); + } + other => panic!("{other:?}"), + } + assert_eq!( + tool_events(&events), + [ + "started read_file", + "denied read_file InvalidArguments", + "result read_file" + ] + ); + let m2 = s.server.requests_to(CHAT)[1].json(); + assert_eq!(m2["messages"][3]["content"], want); +} + #[test] fn a_denied_call_tool_names_the_target_tool_in_the_denial() { let s = setup(vec![ToolResponse::Denied { reason: DenyReason::NoGrant, + detail: None, }]); s.server.route( CHAT, @@ -158,6 +204,7 @@ fn a_pending_approval_that_ends_in_a_refusal() { pending(7, "2026-09-18T12:15:00.000Z"), ToolResponse::Denied { reason: DenyReason::ApprovalRefused, + detail: None, }, ]); s.server.route( diff --git a/docs/implementer-log.md b/docs/implementer-log.md index b63147b..0aab80b 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -6,6 +6,7 @@ reviewer adds findings under "Reviews" once per milestone. | Task | Date | Status | Gate runs | First gate | Deviations | Notes | Model | |---|---|---|---|---|---|---|---| +| M5b/01-invalid-arguments-detail | 2026-09-26 | done | 1 | pass | none | Copied the 13 given files from `docs/plans/M5b/files/` (two new: `bxcore/tests/fixtures/wire/tool_response_denied_detail.json` and `brokerd/tests/invalid_detail.rs`; new versions of `bxcore/tests/wire.rs` and `strict.rs`; new versions of `brokerd/tests/{admin,broker,broker_pending,courier,ledger,serve}.rs` where `broker.rs`/`serve.rs` are the `-task01` files; new versions of `loopd/tests/{broker_port,tools,turn_broker}.rs`); step 2 showed exactly the named failures (`strict::envelopes_reject_unknown_keys_at_every_depth` on the new fixture, two `E0559` in `wire`, and ten non-compiling brokerd/loopd test targets with the named error kinds). Made all the edits exactly as the task gives: `bxcore/src/wire.rs` `ToolResponse::Denied` gains `detail: Option` with the skip/serde attributes; `brokerd/src/args.rs` adds `MAX_DETAIL=300` and `ArgsError::detail` (control chars and U+2028/U+2029 -> space, cut to 300 chars); `brokerd/src/policy/mod.rs` `Denial` gains `detail`, `Denial::new` sets `None`, `decide` sets it for `InvalidArguments` only, the two `DeniedByGrant` arms set `None`; `brokerd/src/ledger/mod.rs` `Decided::Denied` now holds a `Denial` (every arm rebuilt through `Denial::new` except the `RecordPayload::Denial` arm which passes the `Denial` through); `brokerd/src/broker.rs` `handle` sends the denial's detail, the two `pending` arms send `None`; `loopd/src/tools.rs` adds `denial_message(reason, detail)` before `FakeTools` and `FakeTools::call` sends `detail: None`; `loopd/src/turn.rs` imports `denial_message` and reads `detail` into it, the `TurnEvent::ToolDenied` event keeping only the reason. Did not touch `gatewayd`, `bxctl` or `bxcore/src/audit.rs`. `cargo fmt --all`; `cargo test -p bxcore -p brokerd -p loopd` every suite passes (including the eight `invalid_detail` tests); step 6 `grep -rn "Decided::Denied(DenyReason"` prints nothing and `git diff --stat -- crates/gatewayd crates/bxctl crates/bxcore/src/audit.rs` prints nothing; `cargo test --workspace` reports 955 passed and the gate's separate loopd `end_to_end` ignored test adds 1, for 956 total. No new dependency, so `Cargo.lock` is unchanged. | ornith-1.5-35b-a3b | | M5a/22-plain-note-answers | 2026-09-25 | done | 1 | pass | none | The edits are all in `crates/memoryd/src/edit.rs` and match the task exactly. Replaced the `SAVED_AS_NOTE` const with a `saved_as_note(file)` fn ("not added to {file.name()}: this session has read untrusted content, so it was saved in notes.md as an untrusted note") plus the `NOTED` const "saved in notes.md" and `NOTED_UNTRUSTED` const "saved in notes.md, marked untrusted: this session has read untrusted content"; the `format!` string stays one line past 100 chars and `cargo fmt` left it so. Updated the comment in `apply` ("replaced by saved_as_note(file)") and the `answer:` line (`saved_as_note(file)`). In `noted` replaced the two string literals with `NOTED_UNTRUSTED`/`NOTED`. Copied the new `crates/memoryd/tests/edit.rs` and `serve.rs` from `docs/plans/M5a/files/` (not the `-task06`/`-task10` versions). Step 2 showed exactly the four named tests failing (`a_note_is_labelled_by_the_session` and `an_untrusted_session_can_take_notes_but_not_change_identity` in edit, `an_untrusted_session_writes_notes_not_identity` and `review_holds_identity_writes_but_not_notes` in serve); after the edits the step-2 command and `cargo test -p memoryd` every suite pass; `make gate` printed `gate: ok` with 944 tests, unchanged; `grep -rn SAVED_AS_NOTE crates/` prints nothing. No new dependency, so `Cargo.lock` is unchanged. | Ornith-1.5-35B-A3B | | M5a/21-session-follows-result | 2026-09-25 | done (by the design model) | many | fail | The session added and then deleted diagnostic test files (`crates/bxctl/tests/zz_diag.rs`, `zz_diag2.rs`) and ran about 80 minutes of experiments instead of stopping when a test outside the task failed, as the task says. | The session's edits match the task file exactly and the copied `ledger_label.rs` is identical to the plan's; step 2 showed the two named tests failing, and after the edits `ledger_label`, `cargo test -p brokerd` and `-p bxctl` passed. The gate then failed in `bxctl`'s `args_os` with `NotFound`. The cause was the design model's: its reference worktree for tasks 21 and 22 shared this `target/` through a symlink, so test binaries built there had the worktree's absolute paths compiled in (`CARGO_BIN_EXE_*`, `CARGO_MANIFEST_DIR`), and Cargo, whose freshness check does not include the workspace's absolute path, kept them after the worktree was deleted. After `cargo clean`, `make gate` printed `gate: ok`, 944 tests. The owner stopped the run. | Ornith-1.5-35B-A3B (the session); design model (the commit) | | M5a/20-review-ids | 2026-09-25 | done | 1 | pass | none | The two edits exactly as the task gives, both in `submit` in `crates/memoryd/src/review.rs`: the comment block "how many Waiting lines have an id starting format!(\"p-{day}-\"). The id is format!(\"p-{day}-{:04}\", taken + 1)." became "how many Waiting lines of `lines(store)?`, done or not, have an id starting format!(\"p-{day}-\"): an id is never used twice. The id is format!(\"p-{day}-{:04}\", taken + 1)."; and `let taken = waiting(store)?` became `let taken = lines(store)?`, the `.iter()`/`.filter(..)`/`.count()` after it unchanged. `waiting` untouched, since `list`/`accept`/`reject` still use it. Copied `crates/memoryd/tests/review_ids.rs` from `docs/plans/M5a/files/`. `cargo fmt --all`; step 2 showed exactly the two named tests failing (`a_write_staged_after_an_accepted_one_gets_a_new_id_and_can_be_accepted` expected `p-2026-09-24-0002` got `p-2026-09-24-0001`; `ids_count_every_write_staged_that_day_waiting_or_done` expected `p-2026-09-24-0004` got `p-2026-09-24-0002`); after the edits both pass and `cargo test -p memoryd` every suite passes; `make gate` printed `gate: ok`. `cargo test --workspace` reports 940 passed, plus the gate's separate loopd `end_to_end` ignored test = 941 total. No new dependency, so `Cargo.lock` is unchanged. | Ornith-1.5-35B-A3B |