diff --git a/docs/plans/M5c2/09-memoryd-recall-honcho.md b/docs/plans/M5c2/09-memoryd-recall-honcho.md new file mode 100644 index 0000000..c03b5d6 --- /dev/null +++ b/docs/plans/M5c2/09-memoryd-recall-honcho.md @@ -0,0 +1,358 @@ +# M5c-2 task 09: `recall` asks Honcho, outside the state lock + +**Branch:** `m5c2`. Check it: `git branch --show-current` prints `m5c2` and `git status --short` +is empty; otherwise stop. Never switch branch. +**Commit subject:** `memoryd: recall asks Honcho after the local index, outside the state lock` +**Log row:** the Task cell is `M5c2/09-memoryd-recall-honcho`. + +## Goal + +Task 08 wrote `recall`'s Honcho part (`honcho/search.rs`): `search` asks every source at once, +each with its own limit, and `answer` makes the answer from the local index's items and +Honcho's. This task makes `memoryd`'s `recall` use it (M5c spec, section 8). + +- **With `[honcho]`**, a `recall` call: + 1. takes the state lock, runs `recall::local` (task 07: the search, the recall log, the local + items and their label), and releases the lock; + 2. then, with no lock held, runs the Honcho part and makes the answer: `search::Reading::recall`, + a new method that is `search` then `answer`. + + So a source that hangs for its 5 s holds up no `memory` call, no memory block and no other + `recall`: before this task, `call` held the lock for the whole of any call. +- **Without `[honcho]`**, `recall` is exactly as before: `recall::recall` under the lock. +- **`memory`** calls are unchanged: under the lock for the whole call. +- **At start**, with `[honcho]`, `start` keeps what `recall` reads, `search::Reading { readers, + owner, wait }`: the readers task 05 built (`opened.readers`, taken before the worker's thread + takes the rest of `Opened`), the owner's peer from `[honcho]` (`honcho.owner`), and + `search::SOURCE_WAIT` (5 s). + +`serve.rs` is near the 500-line limit (495 lines after this task), so `Reading` and its method go +into `search.rs`, not `serve.rs`. + +## Every place that changes + +This is the complete list. + +| Place | What happens there | +|---|---| +| `crates/memoryd/src/honcho/search.rs` | `Reading` and `Reading::recall` (edit S1) | +| `crates/memoryd/src/serve.rs` | a `use` line; `Memoryd::reading`; `start` builds it; `call` and `recall` (edits E1 to E7) | +| `crates/memoryd/tests/honcho_recall_serve.rs` | new, given | +| everything else | **no change**: not `recall.rs`, not `start.rs`, not the worker, no `Cargo.toml` | + +## Files + +- Copy, from `docs/plans/M5c2/files/` (the same path under it): the new test + `crates/memoryd/tests/honcho_recall_serve.rs`. +- Modify: `crates/memoryd/src/honcho/search.rs`, `crates/memoryd/src/serve.rs`, + `docs/implementer-log.md`. +- Do not touch: any other file. + +## Facts you need + +- **`serve.rs` today.** `start` opens the Honcho side (`worker::open`, returning `Opened`) when + `config.honcho_config()?` is `Some(honcho)`, and later moves `opened` into the worker's thread + (`worker::spawn`). `Opened::readers` is an `Arc` (task 05). `call` takes the state + lock (`self.lock()`) first, then runs `recall` or `memory` with it. +- **Taking the lock for a block only**: `let local = { let state = self.lock(); … };` releases + the lock at the block's closing brace, when `state` is dropped. The Honcho part runs after it. +- **`let … else`**: `let Some(reading) = &self.reading else { …; return …; };` runs the `else` + block (which must return) when there is no `[honcho]`. +- **What `recall` answers with `[honcho]`** (task 08's `answer`): the local items, then Honcho's + up to `k` in all, numbered after them, then any `{source}: unavailable` line; + `(no matches for "…")` when there is nothing. A failed or late source is logged by + `search`, never with the query. +- **The given test** `honcho_recall_serve.rs` (4 tests; about 5 s), through `memoryd`'s `brokerd` + socket, against the fake Honcho (task 08's version): + - a local hit, then `boxmaker1`'s (labelled trusted, among them a conclusion about the owner's + peer, `kyle`) and the fleet's; the answer untrusted because a fleet hit is shown (T58); the + agent's own workspaces never searched; with the fleet missing, no fleet hit and a trusted + answer; + - while the fleet hangs, a `memory` call answered within 2 s and the `recall` still waiting; + then the `recall` answered with `fleet: unavailable`, and the line logged; + - nothing found anywhere: `(no matches for "zebra")`; + - without `[honcho]`: the local answer exactly as before, and nothing logged. +- The items the edits use. This table is complete. + + | Item | Where | + |---|---| + | `search::search`, `search::answer`, `search::SOURCE_WAIT` (task 08) | S1, E4 | + | `recall::local`, `recall::recall` (task 07) | E7 | + | `Arc::clone` | E4 | + +## 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/memoryd/src/honcho/search.rs` + +**S1.** The line + +```rust +/// What the Honcho part found. +``` + +becomes + +```rust +/// What `recall` reads in Honcho: the sources, the owner's peer, and each source's limit +/// (`SOURCE_WAIT` in `memoryd`). +pub struct Reading { + pub readers: Arc, + pub owner: String, + pub wait: Duration, +} + +impl Reading { + /// `recall`'s answer from the local index's items and Honcho's: `search`, then `answer`. + pub fn recall(&self, local: Local, args: &RecallArgs, log: &Log) -> Recalled { + let searched = search(&self.readers, &self.owner, args, self.wait, log); + answer(local, &args.query, args.k, searched) + } +} + +/// What the Honcho part found. +``` + +### `crates/memoryd/src/serve.rs` + +**E1.** Add one line. The line + +```rust +use crate::honcho::report; +``` + +becomes + +```rust +use crate::honcho::report; +use crate::honcho::search; +``` + +**E2.** In `struct Memoryd`, the lines + +```rust + mirror: Option, + log: Log, +} +``` + +become + +```rust + mirror: Option, + /// With `[honcho]`: what `recall` reads beside the local index, outside `state`. + reading: Option, + log: Log, +} +``` + +**E3.** In `start`, the lines + +```rust + let opened = match config.honcho_config()? { + Some(honcho) => Some(worker::open(&config, &honcho, &log)?), + None => None, + }; +``` + +become + +```rust + let (opened, owner) = match config.honcho_config()? { + Some(honcho) => ( + Some(worker::open(&config, &honcho, &log)?), + honcho.owner.clone(), + ), + None => (None, String::new()), + }; +``` + +**E4.** In `start`, the lines + +```rust + let (honcho, mirror) = match opened { + Some(opened) => { + let sync = Arc::clone(&opened.sync); + let mirror = opened.mirror(); + worker::spawn(opened, &config, Arc::clone(&wake), Arc::clone(&log))?; + (Some(sync), Some(mirror)) + } + None => (None, None), + }; +``` + +become + +```rust + let (honcho, mirror, reading) = match opened { + Some(opened) => { + let sync = Arc::clone(&opened.sync); + let mirror = opened.mirror(); + let reading = search::Reading { + readers: Arc::clone(&opened.readers), + owner, + wait: search::SOURCE_WAIT, + }; + worker::spawn(opened, &config, Arc::clone(&wake), Arc::clone(&log))?; + (Some(sync), Some(mirror), Some(reading)) + } + None => (None, None, None), + }; +``` + +**E5.** In `start`, the lines + +```rust + wake, + mirror, + log, + }), +``` + +become + +```rust + wake, + mirror, + reading, + log, + }), +``` + +**E6.** The lines + +```rust + /// A decided call from `brokerd`: `recall` or `memory`, under the one lock. `Failed` carries the + /// sentence for the model. + fn call(&self, call: MemoryCall) -> ToolResponse { + let state = self.lock(); + let result = match call.request.tool.as_str() { + bxcore::memory::RECALL => self.recall(&state, &call), + bxcore::memory::MEMORY => self.memory(&state, &call), +``` + +become + +```rust + /// A decided call from `brokerd`: `recall` or `memory`. `memory` runs under the one lock; + /// `recall` takes it for the local index only (`recall`). `Failed` carries the sentence for + /// the model. + fn call(&self, call: MemoryCall) -> ToolResponse { + let result = match call.request.tool.as_str() { + bxcore::memory::RECALL => self.recall(&call), + bxcore::memory::MEMORY => self.memory(&self.lock(), &call), +``` + +**E7.** The lines + +```rust + /// `recall`: search, labelled by what it found. + fn recall(&self, state: &State, call: &MemoryCall) -> Result { + let args: RecallArgs = match serde_json::from_str(&call.request.arguments) { + Ok(args) => args, + Err(e) => return Err(format!("recall's arguments: {e}")), + }; + recall::recall( + &state.index, + &self.store, + &args, + call.request.session.as_str(), + Timestamp::now(), + ) + } +``` + +become + +```rust + /// `recall`: search, labelled by what it found. Without `[honcho]`, the local index under the + /// lock, as before. With it, the local index under the lock, then, with the lock released, + /// Honcho's sources (M5c spec, section 8): a source that hangs holds up no other request. + fn recall(&self, call: &MemoryCall) -> Result { + let args: RecallArgs = match serde_json::from_str(&call.request.arguments) { + Ok(args) => args, + Err(e) => return Err(format!("recall's arguments: {e}")), + }; + let session = call.request.session.as_str(); + let Some(reading) = &self.reading else { + let state = self.lock(); + return recall::recall(&state.index, &self.store, &args, session, Timestamp::now()); + }; + let local = { + let state = self.lock(); + recall::local(&state.index, &self.store, &args, session, Timestamp::now())? + }; + Ok(reading.recall(local, &args, &self.log)) + } +``` + +## Steps + +- [ ] **1. Copy the given test.** From the repository root, one command: + + ```sh + cp docs/plans/M5c2/files/crates/memoryd/tests/honcho_recall_serve.rs crates/memoryd/tests/ + ``` + +- [ ] **2. See it fail.** `cargo test --no-fail-fast -p memoryd --test honcho_recall_serve`, with + a shell timeout of 600000 ms: it compiles, and two tests fail: + `a_fleet_hit_makes_the_answer_untrusted` (no Honcho hit is shown yet) and + `a_hanging_source_holds_up_no_memory_call` (the fleet is never searched: ``the fleet was never + searched``). The other two pass already. +- [ ] **3. The edits.** Make edits S1 and E1 to E7, in order. `cargo check -p memoryd` prints no + error and no warning. +- [ ] **4. The tests.** With a shell timeout of 600000 ms, every test passes: 4 in + `honcho_recall_serve`, and as before 12 in `honcho_search`, 6 in `honcho_serve`, 8 in `serve` + and 5 in `serve_sanitize`: + + ```sh + cargo test -p memoryd --test honcho_recall_serve --test honcho_serve --test serve \ + --test serve_sanitize --test honcho_search + ``` +- [ ] **5. Check.** Two commands: + - `git diff --stat` lists exactly `crates/memoryd/src/honcho/search.rs` and + `crates/memoryd/src/serve.rs`. + - `wc -l crates/memoryd/src/serve.rs` prints 495. +- [ ] **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: **1434** after this + task, 1430 before it; the 4 new ones are the tests of `honcho_recall_serve.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/09-memoryd-recall-honcho`. Stage with + `git add crates/memoryd 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 1434 tests. +- `git diff HEAD~1 --stat` lists only `search.rs`, `serve.rs`, `honcho_recall_serve.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: a compile error, or other tests failing. +- `cargo check` in step 3 prints an error or a warning. Quote it, and stop. +- A test fails in step 4. Do not change a test: stop, and give the failing tests' names and + their messages. +- The fix seems to need a change anywhere else: `recall.rs`, `start.rs`, `read.rs`, the worker, + a configuration key for the limit. It does not; stop and say why you think so. +- `serve.rs` passes 500 lines, or the gate's line check fails. +- A test outside the given file 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 1434. +- 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 (8 tried, all +caught): the state lock held across the Honcho part; Honcho not asked though configured; no +`Reading` built at start; the agent's peer passed as the owner's; the path without `[honcho]` +going through the Honcho answer; `Reading::recall` dropping Honcho's label; a zero limit; the +local items left out of the answer. diff --git a/docs/plans/M5c2/README.md b/docs/plans/M5c2/README.md index ff0d9b9..63fec3c 100644 --- a/docs/plans/M5c2/README.md +++ b/docs/plans/M5c2/README.md @@ -107,6 +107,7 @@ The gate count is the passed total of `cargo test --workspace` plus one, for the | 06 | 1414 | 22, 22 | | 07 | 1418 | 15, 15 | | 08 | 1430 | 19, 19 | +| 09 | 1434 | 8, 8 | **A flaky test, fixed before the run:** `bxctl`'s `chat_approvals` test `json_prints_only_json_and_never_asks_brokerd` failed once in 5 loaded runs of the whole workspace diff --git a/docs/plans/M5c2/files/crates/memoryd/tests/honcho_recall_serve.rs b/docs/plans/M5c2/files/crates/memoryd/tests/honcho_recall_serve.rs new file mode 100644 index 0000000..b28bc25 --- /dev/null +++ b/docs/plans/M5c2/files/crates/memoryd/tests/honcho_recall_serve.rs @@ -0,0 +1,241 @@ +//! `memoryd` answering `recall` with `[honcho]` (M5c spec, section 8), through its `brokerd` +//! socket, against a fake Honcho: the local index's hits, then the read workspace's and the +//! fleet's, the answer untrusted when a fleet hit is shown (T58); a hanging source holds up no +//! memory call, because the state lock is released before Honcho is asked; without `[honcho]`, +//! `recall` answers as before. Every wait has a deadline. Do not edit. + +#[path = "support/honcho.rs"] +mod honcho; +#[path = "support/logs.rs"] +mod logs; +#[path = "support/tmp.rs"] +mod tmp; + +use std::os::unix::net::UnixStream; +use std::path::Path; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; + +use bxcore::{ + CallId, DataClass, Envelope, MemoryCall, Message, SessionId, ToolRequest, ToolResponse, + read_frame, write_frame, +}; +use honcho::{FakeHoncho, Mode}; +use logs::{user, write_log}; +use memoryd::config::Config; +use memoryd::serve; +use tmp::TempDir; + +/// `[honcho]` for `url`, reading `boxmaker1`'s workspace, labelled trusted. +fn section(url: &str) -> String { + format!( + "[honcho]\nurl = \"{url}\"\nagent = \"boxmaker0\"\nowner = \"kyle\"\ntrusted = \"boxmaker0-trusted\"\nuntrusted = \"boxmaker0-untrusted\"\nfleet = \"fleet\"\n[[honcho.read]]\nagent = \"boxmaker1\"\nworkspace = \"boxmaker1-trusted\"\ntrust = \"trusted\"\n" + ) +} + +struct Home { + config: Config, + /// Removed when the test ends; declared last, so it outlives what writes into it (T44). + _dir: TempDir, +} + +impl Home { + /// A home whose index holds one line the owner said, `tea time`; with `[honcho]` at `url` + /// when there is one. + fn new(tag: &str, url: Option<&str>) -> Home { + let dir = TempDir::new(tag); + let soul = dir.write("etc/soul.md", "You are Boxmaker.\n"); + write_log(dir.path(), "mm-a", &[user(1, "tea time")]); + let text = format!( + "[paths]\nhome = \"{}\"\n[memory]\nsoul_starter = \"{}\"\n{}", + dir.path().display(), + soul.display(), + url.map(section).unwrap_or_default() + ); + Home { + config: Config::parse(&text).unwrap(), + _dir: dir, + } + } + + /// `serve::start`, run on a thread; its log lines are collected. + fn serve(&self) -> Arc>> { + let lines = Arc::new(Mutex::new(Vec::new())); + let sink = Arc::clone(&lines); + let log: serve::Log = Arc::new(move |l: &str| sink.lock().unwrap().push(l.to_string())); + let running = serve::start(self.config.clone(), log).unwrap(); + std::thread::spawn(move || running.run()); + lines + } +} + +/// One call on `brokerd`'s socket and its answer, within 20 s; how long it took. +fn call(socket: &Path, tool: &str, arguments: &str) -> (ToolResponse, Duration) { + let started = Instant::now(); + let mut stream = UnixStream::connect(socket).unwrap(); + stream + .set_read_timeout(Some(Duration::from_secs(20))) + .unwrap(); + let env = Envelope { + v: 1, + id: 3, + r#final: true, + msg: Message::MemoryCall(MemoryCall { + request: ToolRequest { + session: SessionId::new("mm-now").unwrap(), + call: CallId(3), + tool: tool.to_string(), + arguments: arguments.to_string(), + }, + taint: DataClass::Private, + untrusted: false, + }), + }; + write_frame(&mut stream, &env).unwrap(); + match read_frame(&mut stream).unwrap().msg { + Message::ToolResponse(response) => (response, started.elapsed()), + other => panic!("{other:?}"), + } +} + +fn result(response: ToolResponse) -> (String, DataClass, bool) { + match response { + ToolResponse::Result { + content, + class, + untrusted, + .. + } => (content, class, untrusted), + other => panic!("{other:?}"), + } +} + +/// The paths of the fake's search and conclusion-query requests. +fn reads(fake: &FakeHoncho) -> Vec { + fake.requests() + .into_iter() + .map(|r| r.path) + .filter(|p| p.ends_with("/search") || p.ends_with("/conclusions/query")) + .collect() +} + +/// T58, end to end: a fleet hit shown makes `memoryd`'s answer untrusted, as `brokerd` will label +/// the session (task 02); hits only from a workspace labelled trusted do not. +#[test] +fn a_fleet_hit_makes_the_answer_untrusted() { + let fake = FakeHoncho::start(); + let home = Home::new("recall-serve-fleet", Some(&fake.url)); + let _lines = home.serve(); + let socket = home.config.broker_socket(); + let (content, class, untrusted) = + result(call(&socket, "recall", r#"{"query":"tea","k":10}"#).0); + assert!( + content.starts_with("[1] memory · session mm-a, 2026-09-20 14:01 UTC, the owner said · private\n tea time\n[2] memory · boxmaker1: kyle said, "), + "{content}" + ); + assert!( + content.contains("\n[3] memory-tainted · fleet: kyle said, "), + "{content}" + ); + // The conclusions about the owner's peer, `[honcho] owner`, are asked for and shown. + assert!( + content.contains("memory · boxmaker1: a conclusion about kyle, "), + "{content}" + ); + assert_eq!((class, untrusted), (DataClass::Private, true)); + assert!( + !reads(&fake).iter().any(|p| p.contains("boxmaker0")), + "{:?}", + reads(&fake) + ); + fake.set_workspace("fleet", Mode::Missing); + let (content, _, untrusted) = result(call(&socket, "recall", r#"{"query":"tea","k":10}"#).0); + assert!(!content.contains("fleet"), "{content}"); + assert!(!untrusted, "{content}"); +} + +/// `recall` releases the state lock before it asks Honcho: while a source hangs, a memory call is +/// answered at once, and the `recall` answers after the source's limit with its `unavailable` +/// line. +#[test] +fn a_hanging_source_holds_up_no_memory_call() { + let fake = FakeHoncho::start(); + fake.set_workspace("fleet", Mode::Hang); + let home = Home::new("recall-serve-hang", Some(&fake.url)); + let lines = home.serve(); + let socket = home.config.broker_socket(); + let for_recall = socket.clone(); + let recall = std::thread::spawn(move || call(&for_recall, "recall", r#"{"query":"tea"}"#)); + let deadline = Instant::now() + Duration::from_secs(10); + while !reads(&fake) + .iter() + .any(|p| p == "/v3/workspaces/fleet/search") + { + assert!(Instant::now() < deadline, "the fleet was never searched"); + std::thread::sleep(Duration::from_millis(10)); + } + let (answer, took) = call( + &socket, + "memory", + r#"{"action":"note","text":"tea is kept in the blue tin"}"#, + ); + assert!(matches!(answer, ToolResponse::Result { .. }), "{answer:?}"); + assert!( + took < Duration::from_secs(2), + "the memory call waited {took:?}" + ); + assert!(!recall.is_finished(), "the recall is still waiting"); + let (answer, took) = recall.join().unwrap(); + let (content, _, untrusted) = result(answer); + assert!(content.ends_with("\nfleet: unavailable"), "{content}"); + assert!(took < Duration::from_secs(15), "{took:?}"); + assert!(!untrusted, "no fleet hit was shown: {content}"); + assert!( + lines + .lock() + .unwrap() + .iter() + .any(|l| l.starts_with("memoryd: recall: fleet unavailable: ")), + "{:?}", + lines.lock().unwrap() + ); +} + +/// Nothing found anywhere: the sentence local `recall` gives. +#[test] +fn nothing_found_anywhere() { + let fake = FakeHoncho::start(); + let home = Home::new("recall-serve-nothing", Some(&fake.url)); + let _lines = home.serve(); + let (content, class, untrusted) = result( + call( + &home.config.broker_socket(), + "recall", + r#"{"query":"zebra"}"#, + ) + .0, + ); + assert_eq!(content, "(no matches for \"zebra\")"); + assert_eq!((class, untrusted), (DataClass::Private, false)); +} + +/// Without `[honcho]`, `recall` is the local index's, and nothing else is asked. +#[test] +fn without_honcho_recall_answers_as_before() { + let home = Home::new("recall-serve-off", None); + let lines = home.serve(); + let socket = home.config.broker_socket(); + let (content, class, untrusted) = result(call(&socket, "recall", r#"{"query":"tea"}"#).0); + assert_eq!( + content, + "[1] memory · session mm-a, 2026-09-20 14:01 UTC, the owner said · private\n tea time" + ); + assert_eq!((class, untrusted), (DataClass::Private, false)); + let (content, _, _) = result(call(&socket, "recall", r#"{"query":"zebra"}"#).0); + assert_eq!(content, "(no matches for \"zebra\")"); + assert!( + lines.lock().unwrap().is_empty(), + "{:?}", + lines.lock().unwrap() + ); +}