kyle/boxmaker
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

M5b plan: task 01, the invalid-arguments detail

Browse Source 
Task file and 13 given files (2 new: the wire fixture with a detail and
brokerd/tests/invalid_detail.rs; 11 new versions of existing tests, most
only adding `detail: None`).

Checked against a reference in a scratch worktree with its own target/,
on titan (rustc 1.98.1, clippy 0.1.98): make gate prints gate: ok with
956 tests (944 + 12 new); /tmp has no new entries after the gate. All 21
quoted before-blocks occur once in master, and applying the edits to
master gives the reference byte for byte. The given files are the
reference's, formatted, none over 500 lines; no chat-template tag in
the plan.

Before the edits, strict's envelopes_reject_unknown_keys_at_every_depth
fails and ten test targets do not compile, as step 2 lists.

Mutations: 21 one-line breaks of the reference; 20 caught by the given
tests. The one not caught (dropping `default` from the detail attribute)
cannot change behaviour: serde reads a missing Option field as None.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
kyle
2026-09-26 02:35:35 -07:00
co-authored by Claude Opus 5.5
parent 56b969d55b
commit fa5bcae503
14 changed files with 4338 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/plans/M5b/01-invalid-arguments-detail.md
+551
View File
@@ -0,0 +1,551 @@
# M5b task 01: an invalid call is denied with its reason
**Branch:** `m5b` (run `git switch m5b`; `git status --short` must be empty, otherwise stop)
**Commit subject:** `bxcore, brokerd, loopd: an invalid-arguments denial says why`
## Goal
`brokerd` refuses a tool call whose arguments do not parse (`invalid_arguments`). Its parser
explains why (`ArgsError`), but `policy::decide` throws the explanation away, so the model reads
only a fixed sentence and has to guess. On the device (2026-09-25) it spent two calls finding that
`note` takes no `file`. After this task the explanation reaches the model (M5b spec, section 4):
| The model's call to `memory` | What it reads back |
|---|---|
| `{"action":"note","file":"notes.md","text":"x"}`, before | `Denied: the arguments are not valid for this tool.` |
| the same, after | ``Denied: the arguments are not valid for this tool: invalid argument shape: unknown field `file`, expected `text`.`` |
Four parts:
1. **`bxcore`**: `ToolResponse::Denied` gains a field `detail: Option<String>`, written only when
it is `Some`. A denial without a detail has exactly the bytes it had before, so every existing
fixture stays valid. One new fixture has a detail.
2. **`brokerd`**: `ArgsError` gains `detail()`: its `Display` text on one line (each control
character, and U+2028 and U+2029, becomes a space), cut to 300 characters. `policy::Denial`
gains `detail`, set by `decide` for `InvalidArguments` only. The ledger passes the whole
`Denial` on (`Decided::Denied(Denial)`), and `broker.sock` sends its detail.
3. **`loopd`**: a new `denial_message(reason, detail)` puts the detail after the sentence, before
its full stop. Without a detail it is exactly today's sentence.
4. **Unchanged**: the audit record (`DecisionRecord::Denied { reason }` has no detail: the record
holds the arguments already), `TurnEvent::ToolDenied`, and every text `gatewayd` and `bxctl`
show the owner. Do not touch `crates/gatewayd`, `crates/bxctl` or `crates/bxcore/src/audit.rs`.
## Every place `ToolResponse::Denied` appears
This is the complete list. Each one is covered by an edit below or by a given file.
| Place | What happens there |
|---|---|
| `crates/bxcore/src/wire.rs`, the enum | gains `detail` (edit 1) |
| `crates/brokerd/src/broker.rs`, `handle`, step 3a | sends the `Denial`'s detail (edit B1) |
| `crates/brokerd/src/broker.rs`, `pending`, `Verdict::Denied` and `Err(reason)` | `detail: None`: an approval's denial has no detail (edits B2, B3) |
| `crates/brokerd/src/ledger/answer.rs`, `finish` | a pattern with `..`; no change |
| `crates/loopd/src/turn.rs`, `run_call` | reads `detail` and calls `denial_message` (edit T2) |
| `crates/loopd/src/tools.rs`, `FakeTools::call` | `detail: None` (edit L2) |
| `crates/loopd/src/broker_port.rs` | a pattern with `..`; no change |
| tests in `bxcore`, `brokerd`, `loopd` | new versions are given; `brokerd/tests/broker_sequence.rs` uses `..` and is not changed |
## Files
- Copy, from `docs/plans/M5b/files/` (the same path under it):
- new: `crates/bxcore/tests/fixtures/wire/tool_response_denied_detail.json`,
`crates/brokerd/tests/invalid_detail.rs`
- **new versions** of: `crates/bxcore/tests/wire.rs`, `crates/bxcore/tests/strict.rs`;
`crates/brokerd/tests/admin.rs`, `broker.rs`, `broker_pending.rs`, `courier.rs`, `ledger.rs`,
`serve.rs`; `crates/loopd/tests/broker_port.rs`, `tools.rs`, `turn_broker.rs`
- Modify: `crates/bxcore/src/wire.rs`, `crates/brokerd/src/args.rs`,
`crates/brokerd/src/policy/mod.rs`, `crates/brokerd/src/ledger/mod.rs`,
`crates/brokerd/src/broker.rs`, `crates/loopd/src/tools.rs`, `crates/loopd/src/turn.rs`,
`docs/implementer-log.md`
## Facts you need
- `serde`: `#[serde(default, skip_serializing_if = "Option::is_none")]` on a field of a variant of
this enum (internally tagged, `deny_unknown_fields`) does what it says: a missing `detail` reads
as `None`, `None` is not written, and an unknown key is still refused. Checked against every
fixture; `strict.rs` walks the new one too.
- `char::is_control(self) -> bool` is true for Unicode's control characters only: U+0000 to
U+001F and U+007F to U+009F (tab, line feed, carriage return, escape and NEL among them). It is
false for U+2028 (line separator) and U+2029 (paragraph separator), so `detail` names those two.
- The 300 is characters (`chars()`), not bytes, so a cut never splits a character.
- `str::strip_suffix(&self, '.') -> Option<&str>`; `Option::as_deref(&self) -> Option<&str>` for an
`Option<String>`.
- `Denial { detail: Some(..), ..Denial::new(reason) }` is Rust's struct update syntax: the other
fields come from `Denial::new`.
- `crates/brokerd/src/ledger/mod.rs` already imports `Denial` and `DenyReason`; keep both.
## The edits, exactly
Nothing else in these files changes. Each quoted "before" block occurs exactly once in its file.
### `crates/bxcore/src/wire.rs`
**1.** In `enum ToolResponse`, the lines
```rust
Denied {
reason: DenyReason,
},
}
```
become
```rust
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<String>,
},
}
```
### `crates/brokerd/src/args.rs`
**A1.** The lines
```rust
/// Maximum length, in bytes, of a URL.
pub const MAX_URL: usize = 2048;
```
become
```rust
/// 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;
```
**A2.** The line
```rust
impl std::error::Error for ArgsError {
```
becomes
```rust
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 {
```
### `crates/brokerd/src/policy/mod.rs`
**P1.** In `struct Denial`, the lines
```rust
/// `Some` only for `DeniedByGrant`: the deny grant's file hash.
pub grant_sha256: Option<Hash32>,
}
```
become
```rust
/// `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>,
}
```
**P2.** In `Denial::new`, the lines
```rust
grant: None,
grant_sha256: None,
}
```
become
```rust
grant: None,
grant_sha256: None,
detail: None,
}
```
**P3.** In `decide`, the lines
```rust
// 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));
};
```
become
```rust
// 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)
});
}
};
```
**P4.** In `decide`, the lines
```rust
Mode::Deny => Outcome::Denied(Denial {
reason: DenyReason::DeniedByGrant,
grant: Some(winner.id.clone()),
grant_sha256: Some(winner.grant_sha256),
}),
```
become
```rust
Mode::Deny => Outcome::Denied(Denial {
reason: DenyReason::DeniedByGrant,
grant: Some(winner.id.clone()),
grant_sha256: Some(winner.grant_sha256),
detail: None,
}),
```
**P5.** In `redecide`, the lines
```rust
Mode::Deny => Err(Denial {
reason: DenyReason::DeniedByGrant,
grant: Some(winner.id.clone()),
grant_sha256: Some(winner.grant_sha256),
}),
```
become
```rust
Mode::Deny => Err(Denial {
reason: DenyReason::DeniedByGrant,
grant: Some(winner.id.clone()),
grant_sha256: Some(winner.grant_sha256),
detail: None,
}),
```
### `crates/brokerd/src/ledger/mod.rs`
**D1.** The lines
```rust
/// How a call was decided.
#[derive(Debug)]
pub enum Decided {
```
become
```rust
/// 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 {
```
**D2.** At the end of `enum Decided`, the lines
```rust
Denied(DenyReason),
}
```
become
```rust
Denied(Denial),
}
```
**D3.** In `Ledger::decide`, the line
```rust
None => return Decided::Denied(DenyReason::AuditUnavailable),
```
becomes
```rust
None => return Decided::Denied(Denial::new(DenyReason::AuditUnavailable)),
```
**D4.** The line
```rust
RecordPayload::Simple(reason) => Decided::Denied(reason),
```
becomes
```rust
RecordPayload::Simple(reason) => Decided::Denied(Denial::new(reason)),
```
**D5.** The line
```rust
RecordPayload::Denial(denial) => Decided::Denied(denial.reason),
```
becomes
```rust
RecordPayload::Denial(denial) => Decided::Denied(denial),
```
**D6.** The line
```rust
Err(()) => Decided::Denied(DenyReason::AuditUnavailable),
```
becomes
```rust
Err(()) => Decided::Denied(Denial::new(DenyReason::AuditUnavailable)),
```
### `crates/brokerd/src/broker.rs`
**B1.** In `handle`, the line
```rust
Decided::Denied(reason) => Some(ToolResponse::Denied { reason }), // 3a. denied
```
becomes
```rust
Decided::Denied(denial) => Some(ToolResponse::Denied {
reason: denial.reason,
detail: denial.detail,
}), // 3a. denied
```
**B2.** In `pending`, the line
```rust
Verdict::Denied(reason) => Some(ToolResponse::Denied { reason }),
```
becomes
```rust
Verdict::Denied(reason) => Some(ToolResponse::Denied {
reason,
detail: None,
}),
```
**B3.** At the end of `pending`, the line
```rust
Err(reason) => Some(ToolResponse::Denied { reason }),
```
becomes
```rust
Err(reason) => Some(ToolResponse::Denied {
reason,
detail: None,
}),
```
### `crates/loopd/src/tools.rs`
**L1.** The line
```rust
pub struct FakeTools {
```
becomes
```rust
/// 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: <detail>.`).
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 {
```
`denial_text` itself does not change.
**L2.** In `FakeTools::call`, the lines
```rust
_ => bxcore::ToolResponse::Denied {
reason: bxcore::DenyReason::NoGrant,
},
```
become
```rust
_ => bxcore::ToolResponse::Denied {
reason: bxcore::DenyReason::NoGrant,
detail: None,
},
```
### `crates/loopd/src/turn.rs`
**T1.** The line
```rust
use crate::tools::{Registry, ToolPort, cap_result, denial_text, dispatch};
```
becomes
```rust
use crate::tools::{Registry, ToolPort, cap_result, denial_message, dispatch};
```
**T2.** In `run_call`, the lines
```rust
ToolResponse::Denied { reason } => {
on_event(&TurnEvent::ToolDenied {
name: request.tool.clone(),
reason,
});
(denial_text(reason).to_string(), DataClass::Public, false)
}
```
become
```rust
ToolResponse::Denied { reason, detail } => {
on_event(&TurnEvent::ToolDenied {
name: request.tool.clone(),
reason,
});
(
denial_message(reason, detail.as_deref()),
DataClass::Public,
false,
)
}
```
The event keeps only the reason: the owner's texts do not change.
## Steps
- [ ] **1. Copy the given files.** From the repository root, thirteen commands:
```sh
cp docs/plans/M5b/files/crates/bxcore/tests/fixtures/wire/tool_response_denied_detail.json \
crates/bxcore/tests/fixtures/wire/
cp docs/plans/M5b/files/crates/bxcore/tests/wire.rs \
crates/bxcore/tests/
cp docs/plans/M5b/files/crates/bxcore/tests/strict.rs \
crates/bxcore/tests/
cp docs/plans/M5b/files/crates/brokerd/tests/invalid_detail.rs \
crates/brokerd/tests/
cp docs/plans/M5b/files/crates/brokerd/tests/admin.rs \
crates/brokerd/tests/
cp docs/plans/M5b/files/crates/brokerd/tests/broker.rs \
crates/brokerd/tests/
cp docs/plans/M5b/files/crates/brokerd/tests/broker_pending.rs \
crates/brokerd/tests/
cp docs/plans/M5b/files/crates/brokerd/tests/courier.rs \
crates/brokerd/tests/
cp docs/plans/M5b/files/crates/brokerd/tests/ledger.rs \
crates/brokerd/tests/
cp docs/plans/M5b/files/crates/brokerd/tests/serve.rs \
crates/brokerd/tests/
cp docs/plans/M5b/files/crates/loopd/tests/broker_port.rs \
crates/loopd/tests/
cp docs/plans/M5b/files/crates/loopd/tests/tools.rs \
crates/loopd/tests/
cp docs/plans/M5b/files/crates/loopd/tests/turn_broker.rs \
crates/loopd/tests/
```
Then `git status --short` lists 11 modified files and 2 new ones, all under `crates/*/tests/`.
- [ ] **2. See them fail.**
- `cargo test -p bxcore --test strict`: one test fails,
`envelopes_reject_unknown_keys_at_every_depth`, with
`tool_response_denied_detail.json: fixture must decode`.
- `cargo test -p bxcore --test wire`: does not compile, with two errors `E0559`
(``variant `bxcore::ToolResponse::Denied` has no field named `detail` ``).
- `cargo build --tests --keep-going -p brokerd -p loopd`: ten test targets do not compile:
`admin`, `broker`, `broker_pending`, `courier`, `invalid_detail`, `ledger` and `serve` in
`brokerd`; `broker_port`, `tools` and `turn_broker` in `loopd`. Every error is about a name
this task adds: `E0559` (no field named `detail` on `ToolResponse::Denied`), `E0609` (no field
`detail` on `Denial`), `E0599` (no method named `detail` for `ArgsError`), `E0432`
(unresolved import `MAX_DETAIL` or `denial_message`), or `E0308` (mismatched types, in
`ledger.rs`, where `Decided::Denied` will hold a `Denial`).
- [ ] **3. `bxcore`.** Make edit 1. `cargo test -p bxcore`: every suite passes.
- [ ] **4. `brokerd`.** Make edits A1, A2, P1 to P5, D1 to D6, and B1 to B3.
`cargo test -p brokerd`: every suite passes, among them the eight tests of `invalid_detail`.
- [ ] **5. `loopd`.** Make edits L1, L2, T1 and T2. `cargo test -p loopd`: every suite passes.
- [ ] **6. Check.** `grep -rn "Decided::Denied(DenyReason" crates/` prints nothing, and
`git diff --stat -- crates/gatewayd crates/bxctl crates/bxcore/src/audit.rs` prints nothing.
- [ ] **7. Run the gate.** `cargo fmt --all`, then `make gate`, with a shell timeout of 600000 ms.
Expected last line: `gate: ok`, with 956 tests: 944 before this task, plus 12 new ones (two in
`bxcore/tests/wire.rs`, eight in `brokerd/tests/invalid_detail.rs`, one in
`loopd/tests/tools.rs`, one in `loopd/tests/turn_broker.rs`).
- [ ] **8. Log and commit.**
`git add crates/bxcore crates/brokerd crates/loopd docs/implementer-log.md Cargo.lock`, then
commit as `AGENTS.md` says. `Cargo.lock` does not change in this task; the `git add` is harmless.
## Done when
- `make gate` prints `gate: ok` with 956 tests.
- `grep -rn "Decided::Denied(DenyReason" crates/` prints nothing.
## Stop and report if
- Any quoted "before" block is not in its file exactly as shown, or is there more than once.
- Step 2 shows a different result: another test failing in `strict`, or an error of another kind.
- After step 3, 4 or 5, a test fails. Do not change a test or a fixture.
- A test in `gatewayd`, `bxctl`, `memoryd` or `toolkit` fails in the gate: check once whether it
fails without your change, then stop and report both results.
- The gate passes with a count other than 956.
- A comment in an edit does not fit the code under it. Quote it and stop.