M5a plan: task 19, a staged write never reuses an id

review::submit counted only the writes still waiting, so a write staged after an accepted one got that one's id and was lost: never listed, never acceptable. It now counts every Waiting line of the day. On a scratch tree with task 16's line applied, both new tests fail and then pass, the old code and a count of every line are both caught, and the gate passes (938 there; 941 after task 18). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2026-09-25 16:41:44 -07:00
parent a32f94aead
commit 0a7a95d709
3 changed files with 165 additions and 3 deletions
@@ -0,0 +1,81 @@
+# M5a task 19: a staged write never reuses an id
+
+**Branch:** `m5a` (run `git switch m5a`; `git status --short` must be empty, otherwise stop)
+**Commit subject:** `memoryd: a staged write's id counts every write staged that day`
+
+## Goal
+
+A fix for a bug the M5a review found (2026-09-24). When review is on, `review::submit` gives each
+staged write an id, `p-<day>-NNNN`, numbered by how many writes that day already have one. It
+counted only the writes still **waiting**. So once `p-2026-09-24-0001` had been accepted, the next
+write that day was given `p-2026-09-24-0001` again. `waiting` treats every id with an `Accepted`
+line as done, so that write never showed in `bxctl memory pending` and could not be accepted or
+rejected: it was lost, while the model was told it was waiting for review.
+
+The fix counts every `Waiting` line in `pending.jsonl`, done or not, so an id is never used twice.
+The comment above says the same, more plainly than before.
+
+## Files
+
+- Copy: the new test `crates/memoryd/tests/review_ids.rs`, from `docs/plans/M5a/files/` (the same
+  path under it)
+- Modify: `crates/memoryd/src/review.rs`, `docs/implementer-log.md`
+
+## The edits, exactly
+
+Both are in `submit` in `crates/memoryd/src/review.rs`. Nothing else in the file changes;
+`waiting` does not change, since `list`, `accept` and `reject` still use it.
+
+**1.** The comment lines
+
+```rust
+    // 2. `day` is the first 10 characters of `ctx.now.to_rfc3339()`. `taken` = how many Waiting
+    //    lines have an id starting format!("p-{day}-"). The id is format!("p-{day}-{:04}", taken +
+    //    1).
+```
+
+become
+
+```rust
+    // 2. `day` is the first 10 characters of `ctx.now.to_rfc3339()`. `taken` = how many Waiting
+    //    lines of `lines(store)?`, done or not, have an id starting format!("p-{day}-"): an id is
+    //    never used twice. The id is format!("p-{day}-{:04}", taken + 1).
+```
+
+**2.** The line
+
+```rust
+    let taken = waiting(store)?
+```
+
+becomes
+
+```rust
+    let taken = lines(store)?
+```
+
+The `.iter()`, `.filter(..)` and `.count()` lines after it stay as they are.
+
+## Steps
+
+- [ ] **1. Copy the test:**
+  `cp docs/plans/M5a/files/crates/memoryd/tests/review_ids.rs crates/memoryd/tests/`
+- [ ] **2. See it fail.** `cargo test -p memoryd --test review_ids`. Expected: both tests fail,
+  `a_write_staged_after_an_accepted_one_gets_a_new_id_and_can_be_accepted` and
+  `ids_count_every_write_staged_that_day_waiting_or_done`.
+- [ ] **3. Make the two edits.** Run `cargo fmt --all`.
+- [ ] **4. See it pass.** `cargo test -p memoryd --test review_ids`: both pass. Then
+  `cargo test -p memoryd`: every suite passes.
+- [ ] **5. Run the gate.** `make gate`. Expected last line: `gate: ok`, with 941 tests (939 before
+  this task, plus the two new ones).
+- [ ] **6. Log and commit.** `git add crates/memoryd docs/implementer-log.md && git commit`
+
+## Done when
+
+- `make gate` prints `gate: ok` with 941 tests.
+
+## Stop and report if
+
+- Either quoted block is not in the file exactly as shown.
+- Step 2 shows a different result than two failures.
+- A test other than these two fails after the edits. Do not change a test.
@@ -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 `18-…`). This file is the index for the owner and the reviewer.
+> a time (`01-…` to `19-…`). 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
@@ -67,9 +67,10 @@ the skeletons were cut (tip T25).
 | 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 |
 | 18 | `18-no-forged-headers.md` | a note cannot forge a `notes.md` header (review finding) | `forgery.rs`; `memory_args.rs`, `notes.rs` updated | reference; 4 mutations caught |
+| 19 | `19-review-ids.md` | a staged write never reuses an id (review finding) | `review_ids.rs` | reference; 2 mutations caught |

 At the end of task 15: 936 tests (854 before task 01); tasks 16 and 17 change no behaviour and
-keep 936; at the end of task 18, 939. Each task's count was checked by running
+keep 936; at the end of task 18, 939; at the end of task 19, 941. Each task's count was checked by running
 the gate at its end state on its own.

 ## Changes during the run
@@ -96,7 +97,9 @@ the gate at its end state on its own.
  a `notes.md` header, so an untrusted session could plant a `moved from memory.md` entry that
  `recall` showed as identity. The spec was changed (`4ae827a`, section 3). Since task 18 hands
  over new versions of two given tests, task 02's `memory_args.rs` is now `memory_args.rs-task02`
-  and task 06's `notes.rs` is now `notes.rs-task06`. Resume from task 16.
+  and task 06's `notes.rs` is now `notes.rs-task06`. Task 19 fixes review ids: counting only the
+  waiting writes gave a write staged after an accepted one the accepted one's id, and the write
+  was lost. Resume from task 16.

 ## Running it

@@ -0,0 +1,78 @@
+//! A staged write's id is never one used before that day, so a write staged after another was
+//! accepted or rejected stays waiting and can be accepted (M5a spec, section 5; review of
+//! 2026-09-24). Do not edit.
+
+#[path = "support/tmp.rs"]
+mod tmp;
+
+use memoryd::edit::Context;
+use memoryd::files::Store;
+use memoryd::review;
+use proto::{DataClass, MemoryArgs, MemoryFile, Timestamp};
+use tmp::TempDir;
+
+fn now() -> Timestamp {
+    Timestamp::parse("2026-09-24T20:14:37.123Z").unwrap()
+}
+
+fn setup(tag: &str) -> (TempDir, Store) {
+    let dir = TempDir::new(tag);
+    let starter = dir.write("etc/soul.md", "Soul.\n");
+    let store = Store::new(&dir.path().join("memory"));
+    store.init(&starter).unwrap();
+    (dir, store)
+}
+
+fn ctx() -> Context {
+    Context {
+        session: "mm-s1".to_string(),
+        call: 7,
+        taint: DataClass::Private,
+        untrusted: false,
+        now: now(),
+    }
+}
+
+/// Stage an add of `text` to memory.md; the answer.
+fn stage(store: &Store, text: &str) -> String {
+    let json = format!(r#"{{"action":"add","file":"memory","text":"{text}"}}"#);
+    let args: MemoryArgs = serde_json::from_str(&json).unwrap();
+    review::submit(store, &args, &json, &ctx()).unwrap()
+}
+
+fn waiting_as(id: &str) -> String {
+    format!("waiting for the owner's review ({id}); nothing is changed until it is accepted")
+}
+
+#[test]
+fn a_write_staged_after_an_accepted_one_gets_a_new_id_and_can_be_accepted() {
+    let (_dir, store) = setup("review-ids-accepted");
+    assert_eq!(stage(&store, "a"), waiting_as("p-2026-09-24-0001"));
+    review::accept(&store, "p-2026-09-24-0001", now()).unwrap();
+    assert_eq!(stage(&store, "b"), waiting_as("p-2026-09-24-0002"));
+    let listed = review::list(&store).unwrap();
+    assert_eq!(listed.lines().count(), 1, "{listed}");
+    assert!(
+        listed.starts_with("p-2026-09-24-0002  mm-s1  {"),
+        "{listed}"
+    );
+    review::accept(&store, "p-2026-09-24-0002", now()).unwrap();
+    assert_eq!(store.entries(MemoryFile::Memory).unwrap(), ["a", "b"]);
+}
+
+#[test]
+fn ids_count_every_write_staged_that_day_waiting_or_done() {
+    let (_dir, store) = setup("review-ids-mixed");
+    stage(&store, "a");
+    stage(&store, "b");
+    stage(&store, "c");
+    review::reject(&store, "p-2026-09-24-0001", now()).unwrap();
+    review::accept(&store, "p-2026-09-24-0003", now()).unwrap();
+    assert_eq!(stage(&store, "d"), waiting_as("p-2026-09-24-0004"));
+    let listed = review::list(&store).unwrap();
+    let ids: Vec<&str> = listed
+        .lines()
+        .filter_map(|line| line.split("  ").next())
+        .collect();
+    assert_eq!(ids, ["p-2026-09-24-0002", "p-2026-09-24-0004"], "{listed}");
+}