M5c-2 plan: task 11, the admin socket's requests move out of serve.rs

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

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::<usize>() {
+                        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*.

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; `<source>: 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, `<workspace>: 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

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::<usize>() {
+                        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),
+        }
+    }
+}

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));
+}