diff --git a/docs/plans/M5c2/01-wire-ask-memory.md b/docs/plans/M5c2/01-wire-ask-memory.md new file mode 100644 index 0000000..3476dff --- /dev/null +++ b/docs/plans/M5c2/01-wire-ask-memory.md @@ -0,0 +1,351 @@ +# M5c-2 task 01: `ask_memory`'s arguments and their shape checks + +**Branch:** `m5c2`. Check it: `git branch --show-current` prints `m5c2` and `git status --short` +is empty; otherwise stop. Never switch branch. +**Commit subject:** `bxcore: ask_memory's arguments and their shape checks` +**Log row:** the Task cell is `M5c2/01-wire-ask-memory`. + +## Goal + +M5c-2 adds a core tool, `ask_memory`, that asks Honcho, a memory server shared by a fleet of +agents, a question about one or two of its workspaces (M5c spec, section 9). This task adds only +the tool's arguments to `bxcore`, with their shape checks, the way `recall`'s are there +(`RecallArgs`, its name constant `RECALL`, its limit `MAX_QUERY`, and `RecallArgs::check`). Later +tasks use them: task 02 (`brokerd` parses the arguments with them), task 03 (`loopd` declares the +tool), task 07 (`memoryd` answers it). + +```json +{"question":"Who wrote the launch note?","workspaces":["fleet","boxmaker1"]} +{"question":"What does the owner prefer?"} +``` + +- **`question`**: required; 1 to 2,000 characters, counted as characters (`chars().count()`), + never bytes; a question of only spaces or newlines is empty. +- **`workspaces`**: optional; at most two names, none empty, none repeated, each 1 to 100 + characters of ASCII letters, digits, `_` and `-`. Missing, or `[]`, means the one workspace + `own`, the agent's own trusted one. Whether a name is one this agent's `[honcho]` configuration + has is **not** checked here: `memoryd` checks that in task 07. +- There is no other field, and an unknown field is refused, as in every argument type we define. + (An earlier draft of the spec had a `reasoning` field; it was removed, and a given test sends + it and expects a refusal.) + +Each refusal is one sentence that the model is shown after `invalid argument shape: `, so each +says what to do instead. They are exact; the given tests compare them. + +## Every place that changes + +This is the complete list. + +| Place | What happens there | +|---|---| +| `crates/bxcore/src/memory.rs` | the module's doc comment; `ASK_MEMORY`; four constants; `AskMemoryArgs`; its `check` and `asked`; the private `workspace_name`; `MemoryCall`'s doc comment (edits M1 to M6) | +| `crates/bxcore/src/lib.rs` | one added line exports `AskMemoryArgs` (edit X1) | +| `crates/bxcore/tests/ask_memory_args.rs` | new, given | +| everything else | **no change**: not `brokerd` (task 02), not `loopd`, not `memoryd`, no fixture, no `Cargo.toml` | + +## Files + +- Copy, from `docs/plans/M5c2/files/` (the same path under it): the new test + `crates/bxcore/tests/ask_memory_args.rs`. +- Modify: `crates/bxcore/src/memory.rs`, `crates/bxcore/src/lib.rs`, `docs/implementer-log.md`. +- Do not touch: any other file. + +## Facts you need + +- **No wire fixture.** `brokerd` hands a decided memory call to `memoryd` in a `MemoryCall`, + whose `request.arguments` is the model's argument text as a string. No message changes, so no + fixture under `crates/bxcore/tests/fixtures/` is added or changed. The arguments' own tests + hold their JSON inline, as `memory_args.rs` does for `recall` and `memory`. +- **The default.** `#[serde(default)]` on `workspaces: Vec` reads a missing field as an + empty list. An empty list is written out as `"workspaces":[]`: every field is written, as + `RecallArgs` writes its default `k`, so that `{"question":"q"}` and + `{"question":"q","workspaces":[]}` look the same once `brokerd` writes them out again. So: + no `skip_serializing_if`. `asked()` turns the empty list into `["own"]`. +- **The order of the checks** is fixed, and a given test pins it: the question (empty, then too + long), then the number of names, then each name in turn (empty, then its form, then whether an + earlier name is the same). A call with several faults is shown the first one. +- **Names are compared as written**: `fleet` and `Fleet` are two names (a Honcho name is + case-sensitive). +- **Why 100 characters.** `bxcore` has no rule for Honcho's names (`memoryd`'s config has its own, + which `bxcore` cannot use: `bxcore` depends on no other crate). The model names a workspace by + a short word (`own`, `untrusted`, `fleet`) or an agent's name; 100 characters is plenty and + keeps the name short enough to repeat in a refusal. +- **Characters and bytes in a name.** A name's form allows ASCII only, so its length in bytes + (`len()`) is its length in characters; a name with any other character fails the form check + whatever its length. +- **The long strings.** Four `return Err(...)` sentences are longer than a line. `rustfmt` leaves a + string literal as it is, and the gate accepts the long line. Do not split a sentence with `\` + or `concat!`. +- The calls and attributes the edits use. This table is complete; `serde` and `serde_json` are + already dependencies of `bxcore`, and no crate's source (the Cargo registry, + `~/.cargo/registry`) is needed or allowed. + + | Item | What it does | + |---|---| + | `#[serde(deny_unknown_fields)]` | on `AskMemoryArgs` (M4): an unknown key is refused | + | `#[serde(default)]` | on `workspaces` (M4): missing reads as `Vec::new()` | + | `str::trim(&self) -> &str`, `str::is_empty` | the empty-question check | + | `str::chars(&self).count() -> usize` | the question's length in characters | + | `Vec::len`, `slice::iter`, `Iterator::enumerate`, `Iterator::take(i)`, `Iterator::any` | the count and the repeat check: `take(i)` is the names before the `i`th | + | `str::bytes(&self)`, `u8::is_ascii_alphanumeric(&self) -> bool`, `Iterator::all` | a name's form | + | `String::as_str` | `asked` borrows each name | + +## The edits, exactly + +Nothing else in these files changes. Each quoted "before" block occurs exactly once in its file +when its turn comes (make them in order). Keep the fields in the order shown. + +### `crates/bxcore/src/memory.rs` + +**M1.** The module's doc comment, the lines + +```rust +//! Memory: the arguments of `recall` and `memory`, their shape checks, and the messages `memoryd` +//! exchanges with `loopd`, `brokerd` and `bxctl` (M5a spec, sections 5 and 7). Content rules (an +//! entry matching the file, a file's limit, the check on characters) are `memoryd`'s. +``` + +become + +```rust +//! Memory: the arguments of `recall`, `memory` and `ask_memory`, their shape checks, and the +//! messages `memoryd` exchanges with `loopd`, `brokerd` and `bxctl` (M5a spec, sections 5 and 7; +//! M5c spec, section 9). Content rules (an entry matching the file, a file's limit, the check on +//! characters, which workspaces `[honcho]` has) are `memoryd`'s. +``` + +**M2.** The line + +```rust +pub const MEMORY: &str = "memory"; +``` + +becomes + +```rust +pub const MEMORY: &str = "memory"; +pub const ASK_MEMORY: &str = "ask_memory"; +``` + +**M3.** The lines + +```rust +/// The line that separates entries in `memory.md`, `user.md` and `notes.md`. +pub const SEPARATOR: &str = "§"; +``` + +become + +```rust +/// The line that separates entries in `memory.md`, `user.md` and `notes.md`. +pub const SEPARATOR: &str = "§"; +/// The most characters an `ask_memory` question may have. +pub const MAX_QUESTION: usize = 2_000; +/// The most workspaces one `ask_memory` may name: both are asked at once, inside `brokerd`'s wait. +pub const MAX_WORKSPACES: usize = 2; +/// The most characters of a workspace name in `ask_memory`. +pub const MAX_WORKSPACE_NAME: usize = 100; +/// The workspace `ask_memory` asks when it names none: the agent's own trusted one. +pub const OWN_WORKSPACE: &str = "own"; +``` + +**M4.** The lines + +```rust +fn default_k() -> u64 { + DEFAULT_K +} +``` + +become + +```rust +fn default_k() -> u64 { + DEFAULT_K +} + +/// The arguments for `ask_memory` (M5c spec, section 9). An empty `workspaces` asks `own`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct AskMemoryArgs { + pub question: String, + #[serde(default)] + pub workspaces: Vec, +} +``` + +**M5.** The lines (its spaces are checked: every sentence in the new code has single spaces, and +is compared letter for letter by the given tests) + +```rust +impl MemoryArgs { + /// The shape rules of `memory` (M5a spec, section 5): every text is a well-formed entry, and +``` + +become + +```rust +impl AskMemoryArgs { + /// The shape rules of `ask_memory` (M5c spec, section 9), in this order: the question, the + /// number of names, then each name in turn. Only a name's form is checked here. Each sentence + /// reaches the model, so it says what to do instead. + pub fn check(&self) -> Result<(), String> { + if self.question.trim().is_empty() { + return Err("question is empty; write the question to ask".to_string()); + } + let chars = self.question.chars().count(); + if chars > MAX_QUESTION { + return Err(format!( + "question has {chars} characters, over the limit of {MAX_QUESTION}; ask a shorter question" + )); + } + let names = self.workspaces.len(); + if names > MAX_WORKSPACES { + return Err(format!( + "workspaces names {names}, over the limit of {MAX_WORKSPACES}; ask about the others in another call" + )); + } + for (i, name) in self.workspaces.iter().enumerate() { + if name.is_empty() { + return Err( + "a workspace name is empty; name a workspace, or leave workspaces out to ask own" + .to_string(), + ); + } + if !workspace_name(name) { + return Err(format!( + "a workspace name has 1 to {MAX_WORKSPACE_NAME} letters, digits, _ or -; use own, untrusted, fleet or another agent's name" + )); + } + if self.workspaces.iter().take(i).any(|w| w == name) { + return Err(format!( + "workspaces names {name} twice; name each workspace once" + )); + } + } + Ok(()) + } + + /// The workspaces to ask: those named, in order, or `own` when none is. + pub fn asked(&self) -> Vec<&str> { + if self.workspaces.is_empty() { + vec![OWN_WORKSPACE] + } else { + self.workspaces.iter().map(String::as_str).collect() + } + } +} + +/// A workspace name's form: at most `MAX_WORKSPACE_NAME` characters, each an ASCII letter, a +/// digit, `_` or `-`, as Honcho's names are. `check` refuses an empty name first. +fn workspace_name(name: &str) -> bool { + name.len() <= MAX_WORKSPACE_NAME + && name + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-') +} + +impl MemoryArgs { + /// The shape rules of `memory` (M5a spec, section 5): every text is a well-formed entry, and +``` + +**M6.** `MemoryCall`'s doc comment, the lines + +```rust +/// `brokerd` → `memoryd`: a decided `recall` or `memory` call, with the session's state when it +/// was decided. +``` + +become + +```rust +/// `brokerd` → `memoryd`: a decided `recall`, `memory` or `ask_memory` call, with the session's +/// state when it was decided. +``` + +### `crates/bxcore/src/lib.rs` + +**X1.** Add one line. The line + +```rust +pub use log::{LogRecord, ToolCall, Usage}; +``` + +becomes + +```rust +pub use log::{LogRecord, ToolCall, Usage}; +pub use memory::AskMemoryArgs; +``` + +Do not change the `pub use memory::{ ... };` block under it. + +## Steps + +- [ ] **1. Copy the given test.** From the repository root, one command: + + ```sh + cp docs/plans/M5c2/files/crates/bxcore/tests/ask_memory_args.rs crates/bxcore/tests/ + ``` + + Then `git status --short` lists one new file, `crates/bxcore/tests/ask_memory_args.rs`. +- [ ] **2. See it fail.** `cargo build --tests --keep-going -p bxcore`, with a shell timeout of 600000 ms: one + test target, `ask_memory_args`, does not compile, with two errors, both `E0432`: + ``unresolved import `bxcore::AskMemoryArgs` `` and ``unresolved imports + `bxcore::memory::ASK_MEMORY`, `bxcore::memory::MAX_QUESTION`, + `bxcore::memory::MAX_WORKSPACE_NAME`, `bxcore::memory::MAX_WORKSPACES`, + `bxcore::memory::OWN_WORKSPACE` ``. Every other target compiles. +- [ ] **3. The edits.** Make edits M1 to M6 and X1, in order. `cargo check -p bxcore` prints no + error and no warning. +- [ ] **4. The tests.** `cargo test -p bxcore --test ask_memory_args --test memory_args + --test memory_wire`: every test passes, 8 in `ask_memory_args` and the others as before (6 and + 5). +- [ ] **5. Check.** Two commands: + - `git diff --stat` lists exactly `crates/bxcore/src/lib.rs` (1 line added) and + `crates/bxcore/src/memory.rs`. + - `grep -c 'skip_serializing_if' crates/bxcore/src/memory.rs` prints `5`, as before this task: + the five on `ConsolidateEntry`; none on `AskMemoryArgs`. +- [ ] **6. Run the gate.** `cargo fmt --all`, then `make gate`, with a shell timeout of 600000 ms. + Expected last line: `gate: ok`. The test count is the passed total of `cargo test --workspace` + plus one, for the ignored `end_to_end` test that the gate runs on its own: **1372** after this + task, 1364 before it (1363 passed in `cargo test --workspace`, plus 1), and the 8 new ones are + the tests of `ask_memory_args.rs`. To count, add up the `N passed` of every `test result:` line + the gate's `cargo test --workspace` printed, and add 1. Never run the gate again only to count. +- [ ] **7. Log and commit.** Add your row to `docs/implementer-log.md`; its Task cell is + `M5c2/01-wire-ask-memory`. `git status --short` lists `crates/bxcore/src/lib.rs`, + `crates/bxcore/src/memory.rs` and your log as modified, and the test as new. Stage with + `git add crates/bxcore 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 1372 tests. +- `git diff HEAD~1 --stat` lists only `crates/bxcore/src/lib.rs`, `crates/bxcore/src/memory.rs`, + `crates/bxcore/tests/ask_memory_args.rs` and `docs/implementer-log.md`. + +## Stop and report if + +- The branch check at the top fails. +- A "before" block is not in its file exactly as shown when its turn comes, or is there more + than once. +- Step 2 shows a different result: another target failing, or other errors. +- `cargo check` in step 3 prints an error or a warning. Quote it, and stop. +- A test fails in step 4. Do not change the test: stop, and give the failing tests' names and + their messages. +- The fix seems to need a change anywhere else: another crate, a fixture, a `skip_serializing_if`, + a check against `[honcho]`. It does not; stop and say why you think so. +- A test outside `ask_memory_args.rs` fails in the gate: check once whether it fails without your + change (`git stash -u`, run it, `git stash pop`), then stop and report both results. +- The gate passes with a count other than 1372. +- A comment in an edit does not fit the code under it. Quote it and stop. + +## What the given tests pin + +Each of these changes to the finished code makes at least one given test fail (23 tried, all +caught): the question checked without `trim`; its length in bytes; its limit moved by one +either way; the limit on names moved by one, or dropped; the repeat check dropped, comparing a +name with itself, or ignoring case; the empty-name sentence skipped; `.` allowed in a name, `-` +refused, non-ASCII letters allowed; the 100-character limit moved by one, or dropped; `asked` +empty for no names, or out of order; unknown keys accepted; `workspaces` required; the empty list +left out when written; the names checked before their number; the tool's name; any sentence's +words. diff --git a/docs/plans/M5c2/README.md b/docs/plans/M5c2/README.md index 2d889b7..662ed72 100644 --- a/docs/plans/M5c2/README.md +++ b/docs/plans/M5c2/README.md @@ -89,6 +89,15 @@ trusted workspace within its limit; with rift down, `recall` answers from the lo `unavailable` lines. The baseline is measured again against the 3,000-token budget (`make verify-device`). +## Counts, as tasks are written + +The gate count is the passed total of `cargo test --workspace` plus one, for the ignored +`end_to_end` test the gate runs on its own. Before task 01 it is 1364 (talos, rustc 1.98.1). + +| Task | Gate count after | Mutations tried, caught | +|---|---|---| +| 01 | 1372 | 23, 23 | + ## Before hand-over Every task is worked through `docs/plans/CHECKLIST.md` with a reference in a scratch worktree diff --git a/docs/plans/M5c2/files/crates/bxcore/tests/ask_memory_args.rs b/docs/plans/M5c2/files/crates/bxcore/tests/ask_memory_args.rs new file mode 100644 index 0000000..2f1746a --- /dev/null +++ b/docs/plans/M5c2/files/crates/bxcore/tests/ask_memory_args.rs @@ -0,0 +1,181 @@ +//! The arguments of `ask_memory`: decoding, the default workspace, unknown keys refused, and the +//! shape checks with the sentences the model is shown (M5c spec, section 9). Do not edit. + +use bxcore::AskMemoryArgs; +use bxcore::memory::{ASK_MEMORY, MAX_QUESTION, MAX_WORKSPACE_NAME, MAX_WORKSPACES, OWN_WORKSPACE}; + +fn args(json: &str) -> AskMemoryArgs { + serde_json::from_str(json).unwrap_or_else(|e| panic!("{json}: {e}")) +} + +fn ask(question: &str, workspaces: &[&str]) -> AskMemoryArgs { + AskMemoryArgs { + question: question.to_string(), + workspaces: workspaces.iter().map(|w| w.to_string()).collect(), + } +} + +fn refused(value: &AskMemoryArgs) -> String { + value + .check() + .expect_err(&format!("{value:?} must be refused")) +} + +#[test] +fn the_name_and_the_limits() { + assert_eq!(ASK_MEMORY, "ask_memory"); + assert_eq!(MAX_QUESTION, 2_000); + assert_eq!(MAX_WORKSPACES, 2); + assert_eq!(MAX_WORKSPACE_NAME, 100); + assert_eq!(OWN_WORKSPACE, "own"); +} + +/// No `workspaces`, or an empty list, asks the agent's own workspace; both are written out with +/// every field, so two spellings of one call read the same in the audit log. +#[test] +fn without_workspaces_it_asks_own() { + for json in [ + r#"{"question":"What does the owner prefer?"}"#, + r#"{"question":"What does the owner prefer?","workspaces":[]}"#, + ] { + let value = args(json); + assert_eq!(value, ask("What does the owner prefer?", &[]), "{json}"); + assert_eq!(value.check(), Ok(()), "{json}"); + assert_eq!(value.asked(), ["own"], "{json}"); + assert_eq!( + serde_json::to_string(&value).unwrap(), + r#"{"question":"What does the owner prefer?","workspaces":[]}"# + ); + } +} + +#[test] +fn named_workspaces_round_trip_exactly_and_are_asked_in_order() { + let json = r#"{"question":"Who wrote the launch note?","workspaces":["fleet","boxmaker1"]}"#; + let value = args(json); + assert_eq!( + value, + ask("Who wrote the launch note?", &["fleet", "boxmaker1"]) + ); + assert_eq!(value.check(), Ok(())); + assert_eq!(value.asked(), ["fleet", "boxmaker1"]); + assert_eq!(serde_json::to_string(&value).unwrap(), json); + let one = args(r#"{"question":"q","workspaces":["untrusted"]}"#); + assert_eq!(one.check(), Ok(())); + assert_eq!(one.asked(), ["untrusted"]); +} + +/// `reasoning` was in an earlier draft of the spec and is not a field. +#[test] +fn unknown_keys_missing_questions_and_wrong_types_are_refused() { + for json in [ + r#"{"question":"q","reasoning":"low"}"#, + r#"{"question":"q","workspace":"fleet"}"#, + r#"{"question":"q","k":3}"#, + r#"{"workspaces":["own"]}"#, + r#"{"question":"q","workspaces":"fleet"}"#, + r#"{"question":"q","workspaces":[1]}"#, + r#"{"question":null}"#, + ] { + assert!( + serde_json::from_str::(json).is_err(), + "{json}" + ); + } +} + +/// The limit is in characters, not bytes: each `é` is two bytes, each `日` three. +#[test] +fn the_question_has_1_to_2000_characters() { + assert_eq!(ask(&"é".repeat(2_000), &[]).check(), Ok(())); + assert_eq!(ask(&"日".repeat(2_000), &["own"]).check(), Ok(())); + assert_eq!(ask("x", &[]).check(), Ok(())); + assert_eq!( + refused(&ask(&"é".repeat(2_001), &[])), + "question has 2001 characters, over the limit of 2000; ask a shorter question" + ); + assert_eq!( + refused(&ask(&"x".repeat(5_000), &["own"])), + "question has 5000 characters, over the limit of 2000; ask a shorter question" + ); + for empty in ["", " ", " \n\t "] { + assert_eq!( + refused(&ask(empty, &[])), + "question is empty; write the question to ask", + "{empty:?}" + ); + } +} + +#[test] +fn at_most_two_workspaces_each_named_once() { + assert_eq!(ask("q", &["own", "fleet"]).check(), Ok(())); + assert_eq!( + refused(&ask("q", &["own", "untrusted", "fleet"])), + "workspaces names 3, over the limit of 2; ask about the others in another call" + ); + assert_eq!( + refused(&ask("q", &["fleet", "fleet"])), + "workspaces names fleet twice; name each workspace once" + ); + assert_eq!( + refused(&ask("q", &["own", "own"])), + "workspaces names own twice; name each workspace once" + ); + // Names are compared as written: case is part of a Honcho name. + assert_eq!(ask("q", &["fleet", "Fleet"]).check(), Ok(())); +} + +/// Only the form is checked here; whether a name is one of `[honcho]`'s workspaces is `memoryd`'s +/// check. +#[test] +fn a_workspace_name_has_1_to_100_letters_digits_underscores_or_hyphens() { + let form = "a workspace name has 1 to 100 letters, digits, _ or -; use own, untrusted, fleet or \ + another agent's name"; + for good in [ + "own", + "boxmaker1", + "Boxmaker_1-trusted", + "x", + "-_-", + &"a".repeat(100), + ] { + assert_eq!(ask("q", &[good]).check(), Ok(()), "{good}"); + } + for bad in [ + " ", + "own ", + " own", + "a.b", + "a/b", + "a b", + "é", + "own\n", + "a:b", + &"a".repeat(101), + ] { + assert_eq!(refused(&ask("q", &[bad])), form, "{bad:?}"); + assert_eq!(refused(&ask("q", &["own", bad])), form, "second: {bad:?}"); + } + let empty = "a workspace name is empty; name a workspace, or leave workspaces out to ask own"; + assert_eq!(refused(&ask("q", &[""])), empty); + assert_eq!(refused(&ask("q", &["fleet", ""])), empty); +} + +/// The checks run in a fixed order, so a call with several faults is shown the first: the +/// question, then the number of names, then each name in turn. +#[test] +fn the_first_fault_is_the_one_reported() { + assert_eq!( + refused(&ask("", &["a", "b", "c"])), + "question is empty; write the question to ask" + ); + assert_eq!( + refused(&ask("q", &["", "", ""])), + "workspaces names 3, over the limit of 2; ask about the others in another call" + ); + assert_eq!( + refused(&ask("q", &["", "a.b"])), + "a workspace name is empty; name a workspace, or leave workspaces out to ask own" + ); +}