From 5b39b7ffc344ba87057b0d9cdf52f31147d24b30 Mon Sep 17 00:00:00 2001 From: "K. Isom" Date: Mon, 28 Sep 2026 21:07:18 -0700 Subject: [PATCH] M5c-2 plan: task 11, the admin socket's requests move out of serve.rs Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/plans/M5c2/11-memoryd-serve-admin.md | 283 ++++++++++++++++++ docs/plans/M5c2/README.md | 6 +- .../files/crates/memoryd/src/serve/admin.rs | 75 +++++ .../files/crates/memoryd/tests/serve_admin.rs | 173 +++++++++++ 4 files changed, 535 insertions(+), 2 deletions(-) create mode 100644 docs/plans/M5c2/11-memoryd-serve-admin.md create mode 100644 docs/plans/M5c2/files/crates/memoryd/src/serve/admin.rs create mode 100644 docs/plans/M5c2/files/crates/memoryd/tests/serve_admin.rs diff --git a/docs/plans/M5c2/11-memoryd-serve-admin.md b/docs/plans/M5c2/11-memoryd-serve-admin.md new file mode 100644 index 0000000..ae47ab4 --- /dev/null +++ b/docs/plans/M5c2/11-memoryd-serve-admin.md @@ -0,0 +1,283 @@ +# M5c-2 task 11: `serve.rs` makes room: the admin socket's requests in a module of their own + +**Branch:** `m5c2`. Check it: `git branch --show-current` prints `m5c2` and `git status --short` +is empty; otherwise stop. Never switch branch. +**Commit subject:** `memoryd: the admin socket's requests move to serve/admin.rs` +**Log row:** the Task cell is `M5c2/11-memoryd-serve-admin`. + +## Goal + +Task 12 adds `ask_memory` to `memoryd`'s serving, in `crates/memoryd/src/serve.rs`, which is at +495 lines; the gate refuses any source file over 500. This task makes room, and changes no +behaviour: the two functions that answer the admin socket, `reindex` and `admin`, move +unchanged into a new module, `crates/memoryd/src/serve/admin.rs`, a child of `serve`. + +- **The new file is given whole**, and holds the two functions exactly as they are in `serve.rs` + today, with one change each: `fn` becomes `pub(super) fn`, so that `serve.rs` can still call + them. It has its own `use` lines for what they use. +- **In `serve.rs`**: the two functions are deleted, `mod admin;` is added, and the imports only + they used go (`MemoryAdminReply`, `MemoryCommand` and `Reindexed` from `bxcore`, and + `crate::honcho::report`). `serve.rs` is then 433 lines. +- **A child module sees its parent's private items**: `admin.rs` uses `Memoryd`'s private fields, + its private methods (`lock`, `mirror`, `index_notes`), the private `State` and the private + function `error`, as the code in `serve.rs` did, without making anything else public. +- **New tests pin the moved code.** Breaking the moved code on purpose showed that no test + covered four of its behaviours: `log` without a count shows ten edits; `reject` discards a + held write (routing it to `accept` passed every test); what an accepted write moves to + `notes.md` is indexed at once; a reindex counts the notes. A new test file, `serve_admin.rs`, + covers them. It passes before and after the move. + +## Every place that changes + +This is the complete list. + +| Place | What happens there | +|---|---| +| `crates/memoryd/src/serve/admin.rs` | new, given whole: `reindex` and `admin`, moved | +| `crates/memoryd/src/serve.rs` | the two functions deleted; `mod admin;`; two `use` lines (edits E1 to E4) | +| `crates/memoryd/tests/serve_admin.rs` | new, given | +| everything else | **no change**: no other source, no test, no `Cargo.toml` | + +## Files + +- Copy, from `docs/plans/M5c2/files/` (the same path under it): the new source + `crates/memoryd/src/serve/admin.rs` (a new directory, `crates/memoryd/src/serve/`, beside + `serve.rs`) and the new test `crates/memoryd/tests/serve_admin.rs`. +- Modify: `crates/memoryd/src/serve.rs`, `docs/implementer-log.md`. +- Do not touch: any other file. Do not change `admin.rs`. + +## Facts you need + +- **A module file with a directory of the same name.** `serve.rs` declares `mod admin;`, and + Rust reads it from `serve/admin.rs`. `serve.rs` stays where it is; no `mod.rs` is needed. +- **Why `pub(super)`.** A method in `admin.rs` without it is private to `admin`; `pub(super)` + lets `serve` (its parent), and nothing further, call it. `handle` in `serve.rs` calls + `self.reindex()` and `self.admin(..)` as before. +- **The given test** `serve_admin.rs` (4 tests), through the admin and `brokerd` sockets of a + `memoryd` it starts: `log` with no count (ten of twelve), with 3 and with 20; a rejected write + leaves `memory.md` as it was, an accepted one changes it; an accepted `consolidate` that moves + an entry to `notes.md` is found by `recall` at once, tagged `identity`; a reindex counts one + note. +- **The check that only a move happened**: step 5 compares the moved functions in `admin.rs` + with the ones deleted from `serve.rs`, and prints nothing when they are the same but for + `pub(super)`. + +## The edits, exactly + +Nothing else in this file changes. Each quoted "before" block occurs exactly once in the file +when its turn comes (make them in order). + +### `crates/memoryd/src/serve.rs` + +**E1.** The lines + +```rust +use bxcore::{ + DataClass, Empty, Envelope, ErrorCode, MemoryAdminReply, MemoryArgs, MemoryBlockReply, + MemoryCall, MemoryCommand, Message, PROTOCOL_VERSION, RecallArgs, Reindexed, SessionId, + SessionUpdated, Timestamp, ToolResponse, WireError, read_frame, write_frame, +}; +``` + +become + +```rust +use bxcore::{ + DataClass, Empty, Envelope, ErrorCode, MemoryArgs, MemoryBlockReply, MemoryCall, Message, + PROTOCOL_VERSION, RecallArgs, SessionId, SessionUpdated, Timestamp, ToolResponse, WireError, + read_frame, write_frame, +}; +``` + +**E2.** Remove one line. The lines + +```rust +use crate::honcho::mirror::Mirror; +use crate::honcho::report; +use crate::honcho::search; +``` + +become + +```rust +use crate::honcho::mirror::Mirror; +use crate::honcho::search; +``` + +**E3.** Add two lines. The line + +```rust +use crate::{journal, recall, review}; +``` + +becomes + +```rust +use crate::{journal, recall, review}; + +mod admin; +``` + +**E4.** Delete `reindex` and `admin`, from the end of `index_notes` to the end of the file: the +lines + +```rust + if let Err(e) = state.index.index_notes(&text) { + (self.log)(format!("memoryd: notes not indexed: {e}").as_str()); + } + } + + fn reindex(&self) -> Message { + let mut state = self.lock(); + let max_class = self.config.index.max_class; + match index::rebuild( + &self.config.index_path(), + max_class, + &self.config.sessions_dir(), + &self.store.path("notes.md"), + ) { + Ok((counts, problems)) => { + for problem in problems { + (self.log)(format!("memoryd: not indexed: {problem}").as_str()); + } + match Index::open(&self.config.index_path(), max_class) { + Ok(index) => { + state.index = index; + Message::Reindexed(Reindexed { + logs: counts.logs, + records: counts.records, + notes: counts.notes, + }) + } + Err(e) => error(e), + } + } + Err(e) => error(e), + } + } + + fn admin(&self, command: MemoryCommand, arg: &str) -> Message { + let state = self.lock(); + let now = Timestamp::now(); + let result = match command { + MemoryCommand::Log => { + let n = if arg.is_empty() { + 10 + } else { + match arg.parse::() { + Ok(n) => n, + Err(_) => return error(format!("log takes a count; {arg:?} is not one")), + } + }; + journal::log(&self.store, n) + } + MemoryCommand::Undo => journal::undo(&self.store, arg, now), + MemoryCommand::Pending => review::list(&self.store), + MemoryCommand::Accept => review::accept(&self.store, arg, now), + MemoryCommand::Reject => review::reject(&self.store, arg, now), + MemoryCommand::Sync => Ok(match &self.mirror { + Some(mirror) => mirror.report(), + None => report::NOT_CONFIGURED.to_string(), + }), + }; + if result.is_ok() && matches!(command, MemoryCommand::Undo | MemoryCommand::Accept) { + self.mirror(None); + } + self.index_notes(&state); + match result { + Ok(text) => Message::MemoryAdminReply(MemoryAdminReply { text }), + Err(e) => error(e), + } + } +} +``` + +become + +```rust + if let Err(e) = state.index.index_notes(&text) { + (self.log)(format!("memoryd: notes not indexed: {e}").as_str()); + } + } +} +``` + +## Steps + +- [ ] **1. Copy the given test**, and see it pass on the tree as it is: the move changes no + behaviour. From the repository root: + + ```sh + cp docs/plans/M5c2/files/crates/memoryd/tests/serve_admin.rs crates/memoryd/tests/ + ``` + + Then `cargo test -p memoryd --test serve_admin`, with a shell timeout of 600000 ms: 4 tests + pass. +- [ ] **2. Copy the new module.** + + ```sh + mkdir crates/memoryd/src/serve + cp docs/plans/M5c2/files/crates/memoryd/src/serve/admin.rs crates/memoryd/src/serve/ + ``` + + (Until edit E3, nothing declares it, and the build does not read it.) +- [ ] **3. The edits.** Make edits E1 to E4, in order. `cargo check -p memoryd` prints no error + and no warning. +- [ ] **4. The tests.** With a shell timeout of 600000 ms, every test of `memoryd` passes: 4 in + `serve_admin`, and every other file as before: + + ```sh + cargo test -p memoryd + ``` +- [ ] **5. Check.** Three commands: + - `wc -l crates/memoryd/src/serve.rs` prints 433. + - This prints nothing, because the moved functions are the deleted ones but for `pub(super)`: + + ```sh + diff <(git show HEAD:crates/memoryd/src/serve.rs | sed -n '/^ fn reindex/,/^}/p' | sed '$d') \ + <(sed -n '/pub(super) fn reindex/,/^}/p' crates/memoryd/src/serve/admin.rs | sed '$d' \ + | sed 's/pub(super) fn/fn/') + ``` + - `git diff --stat` lists only `crates/memoryd/src/serve.rs`, and `git status --short` shows + `crates/memoryd/src/serve/` and `serve_admin.rs` as new. +- [ ] **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: **1448** after this + task, 1444 before it; the 4 new ones are the tests of `serve_admin.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/11-memoryd-serve-admin`. 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 1448 tests. +- `git diff HEAD~1 --stat` lists only `serve.rs`, `serve/admin.rs`, `serve_admin.rs` and + `docs/implementer-log.md`. + +## Stop and report if + +- The branch check at the top fails. +- Step 1's tests do not all pass before the move. +- A "before" block is not in the file exactly as shown when its turn comes, or is there more + than once. +- `cargo check` in step 3 prints an error or a warning. Quote it, and stop. Do not change + `admin.rs`. +- A test fails in step 4. Do not change a test: stop, and give the failing tests' names and + their messages. +- Step 5's `diff` prints anything. +- The fix seems to need a change anywhere else: another source, `lib.rs`, a `pub` on + `Memoryd`'s fields. It does not; stop and say why you think so. +- 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 1448. + +## What the given tests pin + +With the new tests, each of these changes to the moved code makes at least one test fail (8 +tried, all caught; the four marked * passed every test before `serve_admin.rs`): `log`'s default +count changed*, or its count ignored; an undo or accept not decided for Honcho; `sync` always +"not configured"; a reindex keeping the old index, or miscounting the notes*; `notes.md` not +indexed after an admin command*; `reject` doing what `accept` does*. diff --git a/docs/plans/M5c2/README.md b/docs/plans/M5c2/README.md index ee296db..d38ad96 100644 --- a/docs/plans/M5c2/README.md +++ b/docs/plans/M5c2/README.md @@ -83,9 +83,10 @@ Honcho sources and chats outside its state lock, and answers `ask_memory`. | 08 | `memoryd`: `recall`'s Honcho part (`honcho/search.rs`): every source of `recall_sources` at once, each on its own thread with a limit of 5 s from the start; a read agent's workspace is a search and two conclusion queries; `: unavailable`; a query holding a secret asks nothing; the answer from the local items and Honcho's, with the stricter label | memoryd | | 09 | `memoryd`: `recall` with `[honcho]` runs the local part under the state lock and the Honcho part outside it; the readers taken at start; without `[honcho]`, `recall` as before | memoryd | | 10 | `memoryd`: `ask_memory`: workspace names checked against `[honcho]`, both chats at once, each answer tagged and capped, labels, `: unavailable`; a question holding a secret sends nothing | memoryd | -| 11 | `memoryd` serves `ask_memory` beside `recall` and `memory`, outside the lock; `bxctl`'s usage and the runbook where a new message needs a pointer | memoryd, bxctl | +| 11 | `memoryd`, making room: `reindex` and `admin` move unchanged from `serve.rs` to `serve/admin.rs`, with tests for four of their behaviours no test pinned | memoryd | +| 12 | `memoryd` serves `ask_memory` beside `recall` and `memory`, outside the lock; `bxctl`'s usage and the runbook where a new message needs a pointer | memoryd, bxctl | -After task 11, the design model runs the M5c-2 device check (spec 13) on straylight, rift and the +After task 12, the design model runs the M5c-2 device check (spec 13) on straylight, rift and the owner's Mattermost: a second `memoryd` under its own name writes a fleet note that `recall` finds, tagged `memory-tainted`, turning the session untrusted; `ask_memory` answers from the agent's own trusted workspace within its limit; with rift down, `recall` answers from the local index with @@ -109,6 +110,7 @@ The gate count is the passed total of `cargo test --workspace` plus one, for the | 08 | 1430 | 19, 19 | | 09 | 1434 | 8, 8 | | 10 | 1444 | 18, 18 | +| 11 | 1448 | 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/src/serve/admin.rs b/docs/plans/M5c2/files/crates/memoryd/src/serve/admin.rs new file mode 100644 index 0000000..8e13e2f --- /dev/null +++ b/docs/plans/M5c2/files/crates/memoryd/src/serve/admin.rs @@ -0,0 +1,75 @@ +//! The owner's requests on `memoryd`'s admin socket: `reindex`, and the memory commands `log`, +//! `undo`, `pending`, `accept`, `reject` and `sync` (M5a spec, section 10; M5c spec, section 5). +//! Moved out of `serve.rs` as they were, to keep it under the line limit. + +use bxcore::{MemoryAdminReply, MemoryCommand, Message, Reindexed, Timestamp}; + +use super::{Memoryd, error}; +use crate::honcho::report; +use crate::index::{self, Index}; +use crate::{journal, review}; + +impl Memoryd { + pub(super) fn reindex(&self) -> Message { + let mut state = self.lock(); + let max_class = self.config.index.max_class; + match index::rebuild( + &self.config.index_path(), + max_class, + &self.config.sessions_dir(), + &self.store.path("notes.md"), + ) { + Ok((counts, problems)) => { + for problem in problems { + (self.log)(format!("memoryd: not indexed: {problem}").as_str()); + } + match Index::open(&self.config.index_path(), max_class) { + Ok(index) => { + state.index = index; + Message::Reindexed(Reindexed { + logs: counts.logs, + records: counts.records, + notes: counts.notes, + }) + } + Err(e) => error(e), + } + } + Err(e) => error(e), + } + } + + pub(super) fn admin(&self, command: MemoryCommand, arg: &str) -> Message { + let state = self.lock(); + let now = Timestamp::now(); + let result = match command { + MemoryCommand::Log => { + let n = if arg.is_empty() { + 10 + } else { + match arg.parse::() { + Ok(n) => n, + Err(_) => return error(format!("log takes a count; {arg:?} is not one")), + } + }; + journal::log(&self.store, n) + } + MemoryCommand::Undo => journal::undo(&self.store, arg, now), + MemoryCommand::Pending => review::list(&self.store), + MemoryCommand::Accept => review::accept(&self.store, arg, now), + MemoryCommand::Reject => review::reject(&self.store, arg, now), + MemoryCommand::Sync => Ok(match &self.mirror { + Some(mirror) => mirror.report(), + None => report::NOT_CONFIGURED.to_string(), + }), + }; + if result.is_ok() && matches!(command, MemoryCommand::Undo | MemoryCommand::Accept) { + self.mirror(None); + } + self.index_notes(&state); + match result { + Ok(text) => Message::MemoryAdminReply(MemoryAdminReply { text }), + Err(e) => error(e), + } + } +} diff --git a/docs/plans/M5c2/files/crates/memoryd/tests/serve_admin.rs b/docs/plans/M5c2/files/crates/memoryd/tests/serve_admin.rs new file mode 100644 index 0000000..0d4d039 --- /dev/null +++ b/docs/plans/M5c2/files/crates/memoryd/tests/serve_admin.rs @@ -0,0 +1,173 @@ +//! The owner's commands on `memoryd`'s admin socket, end to end (M5a spec, section 10): `log` +//! shows the last ten edits unless told how many; `reject` discards a held write and `accept` +//! applies one; what an accepted write moves to `notes.md` is found by `recall` at once; a +//! reindex counts the notes. They pin `serve/admin.rs`, which M5c-2 task 11 moved out of +//! `serve.rs`. Do not edit. + +#[path = "support/tmp.rs"] +mod tmp; + +use std::os::unix::net::UnixStream; +use std::path::Path; +use std::sync::Arc; +use std::time::Duration; + +use bxcore::{ + CallId, DataClass, Empty, Envelope, MemoryAdmin, MemoryCall, MemoryCommand, Message, SessionId, + ToolRequest, ToolResponse, read_frame, write_frame, +}; +use memoryd::config::Config; +use memoryd::serve; +use tmp::TempDir; + +struct Running { + config: Config, + /// Removed when the test ends; declared last, so it outlives what writes into it (T44). + dir: TempDir, +} + +fn start(tag: &str, review: bool) -> Running { + let dir = TempDir::new(tag); + dir.write("etc/soul.md", "You are Boxmaker.\n"); + let text = format!( + "[paths]\nhome = \"{}\"\n[memory]\nsoul_starter = \"{}\"\nreview = {review}\n", + dir.path().display(), + dir.path().join("etc/soul.md").display() + ); + let config = Config::parse(&text).unwrap(); + let log: serve::Log = Arc::new(|_: &str| {}); + let running = serve::start(config.clone(), log).unwrap(); + std::thread::spawn(move || running.run()); + Running { config, dir } +} + +fn ask(socket: &Path, msg: Message) -> Message { + let mut stream = UnixStream::connect(socket).unwrap(); + stream + .set_read_timeout(Some(Duration::from_secs(10))) + .unwrap(); + let env = Envelope { + v: 1, + id: 9, + r#final: true, + msg, + }; + write_frame(&mut stream, &env).unwrap(); + read_frame(&mut stream).unwrap().msg +} + +/// A `memory` or `recall` call from a clean session; the answer's text. +fn call(r: &Running, tool: &str, arguments: &str) -> String { + let msg = Message::MemoryCall(MemoryCall { + request: ToolRequest { + session: SessionId::new("mm-a").unwrap(), + call: CallId(1), + tool: tool.to_string(), + arguments: arguments.to_string(), + }, + taint: DataClass::Private, + untrusted: false, + }); + match ask(&r.config.broker_socket(), msg) { + Message::ToolResponse(ToolResponse::Result { content, .. }) => content, + other => panic!("not a result: {other:?}"), + } +} + +fn admin(r: &Running, command: MemoryCommand, arg: &str) -> String { + let msg = Message::MemoryAdmin(MemoryAdmin { + command, + arg: arg.to_string(), + }); + match ask(&r.config.admin_socket(), msg) { + Message::MemoryAdminReply(reply) => reply.text, + other => panic!("{other:?}"), + } +} + +fn memory_md(r: &Running) -> String { + std::fs::read_to_string(r.dir.path().join("memory/memory.md")).unwrap_or_default() +} + +/// The id of the newest write waiting for review. +fn newest_pending(r: &Running) -> String { + let pending = admin(r, MemoryCommand::Pending, ""); + let last = pending.lines().last().unwrap_or_default(); + last.split_whitespace() + .next() + .unwrap_or_default() + .to_string() +} + +#[test] +fn log_shows_the_last_ten_edits_unless_told_how_many() { + let r = start("admin-log", false); + for n in 1..=12 { + call( + &r, + "memory", + &format!(r#"{{"action":"note","text":"note {n}"}}"#), + ); + } + assert_eq!(admin(&r, MemoryCommand::Log, "").lines().count(), 10); + assert_eq!(admin(&r, MemoryCommand::Log, "3").lines().count(), 3); + assert_eq!(admin(&r, MemoryCommand::Log, "20").lines().count(), 12); +} + +#[test] +fn reject_discards_a_held_write_and_accept_applies_one() { + let r = start("admin-review", true); + call( + &r, + "memory", + r#"{"action":"add","file":"memory","text":"Rejected fact."}"#, + ); + let rejected = newest_pending(&r); + assert!(admin(&r, MemoryCommand::Reject, &rejected).starts_with("rejected: ")); + assert_eq!(memory_md(&r), ""); + call( + &r, + "memory", + r#"{"action":"add","file":"memory","text":"Accepted fact."}"#, + ); + let accepted = newest_pending(&r); + assert!(admin(&r, MemoryCommand::Accept, &accepted).starts_with("accepted: ")); + assert_eq!(memory_md(&r), "Accepted fact.\n"); +} + +/// An accepted `consolidate` moves an entry to `notes.md`; the admin command indexes it, so +/// `recall` finds it at once, as identity moved out of `memory.md`. +#[test] +fn what_an_accepted_write_moves_to_notes_is_recalled_at_once() { + let r = start("admin-notes", true); + call( + &r, + "memory", + r#"{"action":"add","file":"memory","text":"Zebra crossings are painted on Mondays."}"#, + ); + admin(&r, MemoryCommand::Accept, &newest_pending(&r)); + assert_eq!( + call(&r, "recall", r#"{"query":"zebra"}"#), + "(no matches for \"zebra\")" + ); + call( + &r, + "memory", + r#"{"action":"consolidate","file":"memory","entries":[{"old":"Zebra crossings are painted on Mondays.","move":true}]}"#, + ); + admin(&r, MemoryCommand::Accept, &newest_pending(&r)); + assert_eq!(memory_md(&r), ""); + let found = call(&r, "recall", r#"{"query":"zebra"}"#); + assert!(found.starts_with("[1] identity ยท moved "), "{found}"); +} + +#[test] +fn a_reindex_counts_the_notes() { + let r = start("admin-reindex", false); + call(&r, "memory", r#"{"action":"note","text":"one note"}"#); + let Message::Reindexed(counts) = ask(&r.config.admin_socket(), Message::Reindex(Empty {})) + else { + panic!("not reindexed") + }; + assert_eq!((counts.logs, counts.records, counts.notes), (0, 0, 1)); +}