From d707be072dd4e9fc6afef08041046e171817406b Mon Sep 17 00:00:00 2001 From: "K. Isom" Date: Thu, 24 Sep 2026 20:22:58 -0700 Subject: [PATCH] M5a plan: tasks 16 and 17, the review's nits Task 16: memoryd takes the day and the month of a time with str::get, never a slice. Task 17: repeated error lines in memoryd, proto, brokerd and bxctl written once. Neither changes behaviour. Both were applied to a scratch tree from 5f82260: the gate passes with 936 tests, and four mutations of the new helpers are caught by the existing tests. The README runs with OPENCODE_MODEL. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/plans/M5a/16-memoryd-no-slicing.md | 109 +++++++++++ docs/plans/M5a/17-repeated-code.md | 228 ++++++++++++++++++++++++ docs/plans/M5a/README.md | 12 +- 3 files changed, 346 insertions(+), 3 deletions(-) create mode 100644 docs/plans/M5a/16-memoryd-no-slicing.md create mode 100644 docs/plans/M5a/17-repeated-code.md diff --git a/docs/plans/M5a/16-memoryd-no-slicing.md b/docs/plans/M5a/16-memoryd-no-slicing.md new file mode 100644 index 0000000..6a0be82 --- /dev/null +++ b/docs/plans/M5a/16-memoryd-no-slicing.md @@ -0,0 +1,109 @@ +# M5a task 16: `memoryd` takes the day and the month with `get`, not by slicing + +**Branch:** `m5a` (run `git switch m5a`; `git status --short` must be empty, otherwise stop) +**Commit subject:** `memoryd: the day and the month of a time with get, never a slice` + +## Goal + +No behaviour change. Four files of `memoryd` cut the day (`2026-09-24`) or the month (`2026-09`) +out of `to_rfc3339()` with a slice such as `&rfc[..10]`. `AGENTS.md` forbids indexing that can go +out of range in library code. The time is always long enough, so none of these can panic today, +but two of them rely on that without a check. All five places become `str::get` with a fallback, +which cannot panic whatever the string is. Found in the M5a review (2026-09-24). + +## Files + +- Modify: `crates/memoryd/src/notes.rs`, `crates/memoryd/src/journal.rs`, + `crates/memoryd/src/review.rs`, `crates/memoryd/src/recall.rs`, `docs/implementer-log.md` +- No test changes: the existing tests cover every line changed. + +## The edits, exactly + +Nothing else in these files changes. Leave the comments above each line as they are. + +**1. `crates/memoryd/src/notes.rs`**, in `minute`, the two lines + +```rust + let head = if s.len() >= 16 { &s[..16] } else { &s }; + format!("{head}Z") +``` + +become the one line + +```rust + format!("{}Z", s.get(..16).unwrap_or(&s)) +``` + +**2. `crates/memoryd/src/journal.rs`**, in `append`, the five lines + +```rust + let day = if rfc.len() >= 10 { + &rfc[..10] + } else { + "0000-00-00" + }; +``` + +become + +```rust + let day = rfc.get(..10).unwrap_or("0000-00-00"); +``` + +and, further down in the same function, the line + +```rust + let month = &day[..7]; +``` + +becomes + +```rust + let month = day.get(..7).unwrap_or("0000-00"); +``` + +Leave `&all[all.len().saturating_sub(count)..]` in `log` as it is: its start is never past the end. + +**3. `crates/memoryd/src/review.rs`**, in `submit`, the line + +```rust + let day = &rfc[..10]; +``` + +becomes + +```rust + let day = rfc.get(..10).unwrap_or("0000-00-00"); +``` + +**4. `crates/memoryd/src/recall.rs`**, in `recall`, the line + +```rust + let month = &rfc[..7]; +``` + +becomes + +```rust + let month = rfc.get(..7).unwrap_or("0000-00"); +``` + +## Steps + +- [ ] **1. Make the five edits.** Run `cargo fmt --all`. +- [ ] **2. Check none is left.** `grep -nE '\[\.\.[0-9]+\]' crates/memoryd/src/*.rs` prints + nothing. +- [ ] **3. Run the tests.** `cargo test -p memoryd`: every suite passes. +- [ ] **4. Run the gate.** `make gate`. Expected last line: `gate: ok`, with 936 tests, the same + as before this task. +- [ ] **5. Log and commit.** `git add crates/memoryd docs/implementer-log.md && git commit` + +## Done when + +- `make gate` prints `gate: ok`, and step 2's `grep` prints nothing. + +## Stop and report if + +- Any line quoted above is not in the file exactly as shown. +- Any test fails. This task changes no behaviour, so a failure means an edit is wrong; do not + change a test. diff --git a/docs/plans/M5a/17-repeated-code.md b/docs/plans/M5a/17-repeated-code.md new file mode 100644 index 0000000..a61304f --- /dev/null +++ b/docs/plans/M5a/17-repeated-code.md @@ -0,0 +1,228 @@ +# M5a task 17: the same few lines written once, not four times + +**Branch:** `m5a` (run `git switch m5a`; `git status --short` must be empty, otherwise stop) +**Commit subject:** `memoryd, proto, brokerd, bxctl: repeated error lines written once` + +## Goal + +No behaviour change. In four places the same lines are written out again and again. Each one +becomes a small helper, so a later change to a sentence or a check is made once. Found in the M5a +review (2026-09-24). + +| File | Repeated | Becomes | +|---|---|---| +| `crates/memoryd/src/edit.rs` | the stale and "named twice" check, 4 times in `consolidate` | a function `claim` | +| `crates/proto/src/memory.rs` | `if let Some(problem) = entry_problem(..) { return Err(problem); }`, 9 times in `MemoryArgs::check` | a function `entry` | +| `crates/brokerd/src/builtin.rs` | the `GrantProblem`, twice in `grants` | a closure `problem` | +| `crates/bxctl/src/memory.rs` | `cannot reach memoryd at …`, 4 times in `run` | a closure `unreachable` | + +## Files + +- Modify: the four files in the table, `docs/implementer-log.md` +- No test changes: the existing tests cover every line changed. + +## The edits, exactly + +Nothing else in these files changes. Leave the comments that are there as they are. + +### 1. `crates/memoryd/src/edit.rs` + +**a.** Insert this function immediately above the line `fn consolidate(`, with one blank line +between them: + +```rust +/// Mark the entry `old` names as named: stale if `before` has no such entry, an error if it was +/// named already. +fn claim( + file: MemoryFile, + before: &[String], + named: &mut HashSet, + old: &str, +) -> Result<(), String> { + let pos = position(before, old).ok_or_else(|| stale(file))?; + if named.insert(pos) { + Ok(()) + } else { + Err(format!( + "consolidate names an entry of {} twice", + file.name() + )) + } +} +``` + +**b.** In `consolidate`, this block appears four times, in the `Keep`, `Rewrite`, `Move` and +`Drop` arms: + +```rust + let pos = position(before, &old).ok_or_else(|| stale(file))?; + if !named.insert(pos) { + return Err(format!( + "consolidate names an entry of {} twice", + file.name() + )); + } +``` + +Replace each of the four with the one line + +```rust + claim(file, before, &mut named, &old)?; +``` + +The line after it in each arm (`after.push(..)` or `out.push(..)`) stays. The `New` arm does not +have the block and does not change. + +### 2. `crates/proto/src/memory.rs` + +**a.** Insert this function immediately above the line `impl RecallArgs {`, with one blank line +between them: + +```rust +/// `entry_problem` as a `Result`, so a check can use `?`. +fn entry(text: &str, max: usize) -> Result<(), String> { + match entry_problem(text, max) { + Some(problem) => Err(problem), + None => Ok(()), + } +} +``` + +**b.** In `impl MemoryArgs`, replace the whole function `pub fn check(&self) -> Result<(), String>` +(from that line to its closing `}`, the `Ok(())` at its end included) with the following. Keep +the doc comment above it. + +```rust + pub fn check(&self) -> Result<(), String> { + match self { + MemoryArgs::Add { text, .. } => entry(text, MAX_ENTRY), + MemoryArgs::Replace { old, text, .. } => { + entry(old, MAX_ENTRY)?; + entry(text, MAX_ENTRY) + } + MemoryArgs::Remove { old, .. } => entry(old, MAX_ENTRY), + MemoryArgs::Note { text } => entry(text, MAX_NOTE), + MemoryArgs::Consolidate { entries, .. } => { + if entries.is_empty() { + return Err("consolidate needs every entry of the file".to_string()); + } + for line in entries { + match line.action().map_err(str::to_string)? { + EntryAction::Keep(old) + | EntryAction::Move(old) + | EntryAction::Drop(old) => entry(&old, MAX_ENTRY)?, + EntryAction::Rewrite { old, text } => { + entry(&old, MAX_ENTRY)?; + entry(&text, MAX_ENTRY)?; + } + EntryAction::New(text) => entry(&text, MAX_ENTRY)?, + } + } + Ok(()) + } + } + } +``` + +`RecallArgs::check` does not change. + +### 3. `crates/brokerd/src/builtin.rs` + +In `grants`, inside the `for` loop, the lines + +```rust + let grant = toml::from_str::(text).map_err(|error| GrantProblem { + file: id.to_string(), + line: None, + problem: error.to_string(), + })?; + let sha256 = sha256(text.as_bytes()).map_err(|error| GrantProblem { + file: id.to_string(), + line: None, + problem: error.to_string(), + })?; +``` + +become + +```rust + let problem = |error: String| GrantProblem { + file: id.to_string(), + line: None, + problem: error, + }; + let grant = toml::from_str::(text).map_err(|e| problem(e.to_string()))?; + let sha256 = sha256(text.as_bytes()).map_err(|e| problem(e.to_string()))?; +``` + +### 4. `crates/bxctl/src/memory.rs` + +**a.** In `run`, the first lines of the body + +```rust + let mut stream = UnixStream::connect(socket) + .map_err(|e| format!("cannot reach memoryd at {}: {e}", socket.display()))?; + stream + .set_read_timeout(Some(timeout)) + .map_err(|e| format!("cannot reach memoryd at {}: {e}", socket.display()))?; + stream + .set_write_timeout(Some(timeout)) + .map_err(|e| format!("cannot reach memoryd at {}: {e}", socket.display()))?; +``` + +become + +```rust + let unreachable = + |e: &dyn std::fmt::Display| format!("cannot reach memoryd at {}: {e}", socket.display()); + let mut stream = UnixStream::connect(socket).map_err(|e| unreachable(&e))?; + stream + .set_read_timeout(Some(timeout)) + .map_err(|e| unreachable(&e))?; + stream + .set_write_timeout(Some(timeout)) + .map_err(|e| unreachable(&e))?; +``` + +**b.** Further down in `run`, the lines + +```rust + write_frame(&mut stream, &env) + .map_err(|e| format!("cannot reach memoryd at {}: {e}", socket.display()))?; +``` + +become + +```rust + write_frame(&mut stream, &env).map_err(|e| unreachable(&e))?; +``` + +The `memoryd did not answer` line after it does not change: it is a different sentence. + +## Steps + +- [ ] **1. Make the edits**, one file at a time, running `cargo check -p ` after each + (`memoryd`, `proto`, `brokerd`, `bxctl`). Then run `cargo fmt --all`. +- [ ] **2. Check the repeats are gone.** Each of these prints the number shown: + - `grep -c 'named.insert' crates/memoryd/src/edit.rs` prints `1` + - `grep -c 'if let Some(problem) = entry_problem' crates/proto/src/memory.rs` prints `0` + - `grep -c 'GrantProblem {' crates/brokerd/src/builtin.rs` prints `2` (the closure, and the + comment above the loop) + - `grep -c 'cannot reach memoryd' crates/bxctl/src/memory.rs` prints `1` +- [ ] **3. Run the tests.** `cargo test -p memoryd -p proto -p brokerd -p bxctl`: every suite + passes. +- [ ] **4. Run the gate.** `make gate`. Expected last line: `gate: ok`, with 936 tests, the same + as before this task. +- [ ] **5. Log and commit.** + `git add crates/memoryd crates/proto crates/brokerd crates/bxctl docs/implementer-log.md && git commit` + +## Done when + +- `make gate` prints `gate: ok`, and step 2's four counts are as shown. + +## Stop and report if + +- Any block quoted above is not in the file exactly as shown, or appears a different number of + times than the task says. +- Any test fails. This task changes no behaviour, so a failure means an edit is wrong; do not + change a test. diff --git a/docs/plans/M5a/README.md b/docs/plans/M5a/README.md index 65a4125..307ecd4 100644 --- a/docs/plans/M5a/README.md +++ b/docs/plans/M5a/README.md @@ -1,7 +1,7 @@ # M5a implementation plan: local memory > **For the implementing model:** do not work from this file. The owner gives you one task file at -> a time (`01-…` to `15-…`). This file is the index for the owner and the reviewer. +> a time (`01-…` to `17-…`). This file is the index for the owner and the reviewer. **Goal:** a new role, `memoryd`, owns the agent's memory: identity (`soul.md`, `memory.md`, `user.md`, in every new session's prompt) and experience (`notes.md` and the session logs, found @@ -64,8 +64,11 @@ the skeletons were cut (tip T25). | 13 | `13-loopd-memory.md` | the block, the core tools, `session_updated` | `memory.rs`; `baseline.rs`, `pointers.rs`, `channel.rs` updated | reference; 6 mutations caught | | 14 | `14-bxctl-memory.md` | `bxctl reindex`, `bxctl memory` | `cli_memory.rs` | reference; 5 mutations caught | | 15 | `15-soul-and-device.md` | the starter soul; device checks with `memoryd` | `device.rs` updated, `support/memoryd.rs` | reference; `make verify-device` on straylight | +| 16 | `16-memoryd-no-slicing.md` | the day and the month with `get`, not a slice (review nit) | the existing ones | reference | +| 17 | `17-repeated-code.md` | repeated error lines written once (review nit) | the existing ones | reference; 4 mutations caught | -At the end of task 15: 936 tests (854 before task 01). Each task's count was checked by running +At the end of task 15: 936 tests (854 before task 01); tasks 16 and 17 change no behaviour and +keep 936. Each task's count was checked by running the gate at its end state on its own. ## Changes during the run @@ -87,12 +90,15 @@ the gate at its end state on its own. are rustfmt-clean. The attempt is saved in `.state/runs/M5a/04-attempt-2/`. Resume from task 04. The measurements behind this are in `docs/research/2026-09-24-chat-template-tokens.md`, added after the run. +- 2026-09-24, after the review: tasks 16 and 17 clean up the review's nits. Resume from task 16. ## Running it ```sh git switch m5a -BOXMAKER_MODEL=straylight/ornith-1.5-35b-a3b tools/run-plan.sh docs/plans/M5a +OPENCODE_MODEL=straylight/ornith-1.5-35b-a3b tools/run-plan.sh docs/plans/M5a +# the review's follow-ups: +OPENCODE_MODEL=straylight/ornith-1.5-35b-a3b tools/run-plan.sh docs/plans/M5a 16 ``` Keep the OpenCode TUI closed while it runs.