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) <noreply@anthropic.com>
This commit is contained in:
2026-09-25 16:41:44 -07:00
committed by kyle
co-authored by Claude Opus 5.5
parent fb65206a6c
commit d707be072d
3 changed files with 346 additions and 3 deletions
+109
View File
@@ -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.
+228
View File
@@ -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<usize>,
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::<Grant>(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::<Grant>(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 <crate>` 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.
+9 -3
View File
@@ -1,7 +1,7 @@
# M5a implementation plan: local memory # M5a implementation plan: local memory
> **For the implementing model:** do not work from this file. The owner gives you one task file at > **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`, **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 `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 | | 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 | | 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 | | 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. the gate at its end state on its own.
## Changes during the run ## 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 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`, 04. The measurements behind this are in `docs/research/2026-09-24-chat-template-tokens.md`,
added after the run. 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 ## Running it
```sh ```sh
git switch m5a 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. Keep the OpenCode TUI closed while it runs.