From 3640d32de0a7e60dea1710a1e435ec7346e8c4d5 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Fri, 25 Sep 2026 02:16:51 -0700 Subject: [PATCH] memoryd, proto, brokerd, bxctl: repeated error lines written once Implemented-By: OpenCode session (model recorded in docs/implementer-log.md) --- crates/brokerd/src/builtin.rs | 13 +++----- crates/bxctl/src/memory.rs | 12 +++---- crates/memoryd/src/edit.rs | 51 ++++++++++++++---------------- crates/proto/src/memory.rs | 59 +++++++++++------------------------ docs/implementer-log.md | 1 + 5 files changed, 54 insertions(+), 82 deletions(-) diff --git a/crates/brokerd/src/builtin.rs b/crates/brokerd/src/builtin.rs index c6d2aac..0d7138d 100644 --- a/crates/brokerd/src/builtin.rs +++ b/crates/brokerd/src/builtin.rs @@ -25,16 +25,13 @@ pub fn grants() -> Result, GrantProblem> { let sources: &[(&str, &str)] = &[("builtin-memory", MEMORY), ("builtin-recall", RECALL)]; let mut out: Vec = Vec::new(); for (id, text) in sources { - let grant = toml::from_str::(text).map_err(|error| GrantProblem { + let problem = |error: String| 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(), - })?; + 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()))?; out.push(LoadedGrant { id: id.to_string(), grant, diff --git a/crates/bxctl/src/memory.rs b/crates/bxctl/src/memory.rs index 3997001..38e379e 100644 --- a/crates/bxctl/src/memory.rs +++ b/crates/bxctl/src/memory.rs @@ -98,14 +98,15 @@ fn grouped(n: u64) -> String { /// Ask `memoryd` and print its answer. `Err` is the line for stderr. pub fn run(socket: &Path, ask: &Ask, timeout: Duration, out: &mut dyn Write) -> Result<(), String> { - let mut stream = UnixStream::connect(socket) - .map_err(|e| format!("cannot reach memoryd at {}: {e}", socket.display()))?; + 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| format!("cannot reach memoryd at {}: {e}", socket.display()))?; + .map_err(|e| unreachable(&e))?; stream .set_write_timeout(Some(timeout)) - .map_err(|e| format!("cannot reach memoryd at {}: {e}", socket.display()))?; + .map_err(|e| unreachable(&e))?; let msg = match ask { Ask::Reindex => Message::Reindex(Empty {}), @@ -120,8 +121,7 @@ pub fn run(socket: &Path, ask: &Ask, timeout: Duration, out: &mut dyn Write) -> r#final: true, msg, }; - write_frame(&mut stream, &env) - .map_err(|e| format!("cannot reach memoryd at {}: {e}", socket.display()))?; + write_frame(&mut stream, &env).map_err(|e| unreachable(&e))?; let reply = read_frame(&mut stream).map_err(|e| format!("memoryd did not answer: {e}"))?; let text = match reply.msg { diff --git a/crates/memoryd/src/edit.rs b/crates/memoryd/src/edit.rs index 777addf..5366991 100644 --- a/crates/memoryd/src/edit.rs +++ b/crates/memoryd/src/edit.rs @@ -236,6 +236,25 @@ fn noted( }) } +/// 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() + )) + } +} + fn consolidate( file: MemoryFile, before: &[String], @@ -249,43 +268,19 @@ fn consolidate( let action = line.action().map_err(|e| e.to_string())?; match action { proto::EntryAction::Keep(old) => { - 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() - )); - } + claim(file, before, &mut named, &old)?; after.push(old.trim().to_string()); } proto::EntryAction::Rewrite { old, text } => { - 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() - )); - } + claim(file, before, &mut named, &old)?; after.push(checked(&text)?); } proto::EntryAction::Move(old) => { - 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() - )); - } + claim(file, before, &mut named, &old)?; out.push(note(ctx, Kind::Moved(file), old.trim())); } proto::EntryAction::Drop(old) => { - 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() - )); - } + claim(file, before, &mut named, &old)?; out.push(note(ctx, Kind::Dropped(file), old.trim())); } proto::EntryAction::New(text) => { diff --git a/crates/proto/src/memory.rs b/crates/proto/src/memory.rs index bc985c0..90b4469 100644 --- a/crates/proto/src/memory.rs +++ b/crates/proto/src/memory.rs @@ -165,6 +165,14 @@ pub fn entry_problem(text: &str, max: usize) -> Option { None } +/// `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(()), + } +} + impl RecallArgs { /// The shape rules of `recall` (M5a spec, section 5). pub fn check(&self) -> Result<(), String> { @@ -183,61 +191,32 @@ impl MemoryArgs { /// every `consolidate` line has one action and the list is not empty. pub fn check(&self) -> Result<(), String> { match self { - MemoryArgs::Add { text, .. } => { - if let Some(problem) = entry_problem(text, MAX_ENTRY) { - return Err(problem); - } - } + MemoryArgs::Add { text, .. } => entry(text, MAX_ENTRY), MemoryArgs::Replace { old, text, .. } => { - if let Some(problem) = entry_problem(old, MAX_ENTRY) { - return Err(problem); - } - if let Some(problem) = entry_problem(text, MAX_ENTRY) { - return Err(problem); - } - } - MemoryArgs::Remove { old, .. } => { - if let Some(problem) = entry_problem(old, MAX_ENTRY) { - return Err(problem); - } - } - MemoryArgs::Note { text } => { - if let Some(problem) = entry_problem(text, MAX_NOTE) { - return Err(problem); - } + 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 { - let action = line.action().map_err(|problem| problem.to_string())?; - match action { + match line.action().map_err(str::to_string)? { EntryAction::Keep(old) | EntryAction::Move(old) - | EntryAction::Drop(old) => { - if let Some(problem) = entry_problem(&old, MAX_ENTRY) { - return Err(problem); - } - } + | EntryAction::Drop(old) => entry(&old, MAX_ENTRY)?, EntryAction::Rewrite { old, text } => { - if let Some(problem) = entry_problem(&old, MAX_ENTRY) { - return Err(problem); - } - if let Some(problem) = entry_problem(&text, MAX_ENTRY) { - return Err(problem); - } - } - EntryAction::New(text) => { - if let Some(problem) = entry_problem(&text, MAX_ENTRY) { - return Err(problem); - } + entry(&old, MAX_ENTRY)?; + entry(&text, MAX_ENTRY)?; } + EntryAction::New(text) => entry(&text, MAX_ENTRY)?, } } + Ok(()) } } - Ok(()) } } diff --git a/docs/implementer-log.md b/docs/implementer-log.md index d924219..eb75acc 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -6,6 +6,7 @@ reviewer adds findings under "Reviews" once per milestone. | Task | Date | Status | Gate runs | First gate | Deviations | Notes | Model | |---|---|---|---|---|---|---|---| +| M5a/18-repeated-code | 2026-09-25 | done | 2 | pass | none | The four edits exactly as the task gives: `edit.rs` added the private `claim(file, before, &mut named, old)` one blank line above `consolidate`, and replaced the four identical `Keep`/`Rewrite`/`Move`/`Drop` blocks (the `position` check plus the `named.insert` duplicate guard) with `claim(file, before, &mut named, &old)?;`, leaving the `New` arm untouched; `proto/src/memory.rs` added the private `entry(text, max)` one blank line above `impl RecallArgs`, and rewrote `MemoryArgs::check` to call `entry` with `?` instead of the nine `if let Some(problem) = entry_problem(..) { return Err(problem); }` blocks (the `Consolidate` arm's `map_err(|problem| problem.to_string())` became `map_err(str::to_string)`), `RecallArgs::check` unchanged; `brokerd/src/builtin.rs` replaced the two `GrantProblem {..}` inline closures in `grants` with a single `let problem = |error: String| GrantProblem {..}` and `map_err(|e| problem(e.to_string()))`; `bxctl/src/memory.rs` added `let unreachable = |e: &dyn std::fmt::Display| format!("cannot reach memoryd at {}: {e}", socket.display())` and used it for connect, the two `set_*_timeout` sets and the `write_frame` (the `memoryd did not answer` line was left as its own sentence). `cargo fmt --all`; the step-2 greps print 1, 0, 2, 1 in order; `cargo test -p memoryd -p proto -p brokerd -p bxctl` every suite passes; `make gate` printed `gate: ok` with 936 tests, unchanged. No new dependency, so `Cargo.lock` is unchanged. | Ornith-1.5-35B-A3B | | M5a/17-memoryd-no-slicing | 2026-09-25 | done | 1 | pass | none | The five slice-to-`get` edits exactly as the task gives: `notes.rs` `minute` two lines -> `format!("{}Z", s.get(..16).unwrap_or(&s))`; `journal.rs` `append` the five-line `day` -> `rfc.get(..10).unwrap_or("0000-00-00")` and `&day[..7]` -> `day.get(..7).unwrap_or("0000-00")`; `review.rs` `submit` `&rfc[..10]` -> `rfc.get(..10).unwrap_or("0000-00-00")`; `recall.rs` `recall` `&rfc[..7]` -> `rfc.get(..7).unwrap_or("0000-00")`. Left `&all[all.len().saturating_sub(count)..]` in `log` as instructed. `cargo fmt --all`; the step-2 grep for `\[..[0-9]+\]` prints nothing; `cargo test -p memoryd` every suite passes; `make gate` printed `gate: ok` on the first run, 936 tests unchanged. | Ornith-1.5-35B-A3B | | M5a/16-macos-gate | 2026-09-25 | done (by the design model) | 1 | pass | none | The session's work was right and left uncommitted: the diff matches the task file line for line, and the copied `shell.rs` is identical to the plan's. The stop was right too: clippy 1.95 (the workspace's `rust-version`) flags `collapsible_match` in `memoryd`'s `index.rs`, which the plan's gate run did not catch. The design model fixed that in its own commit (44bdcbd), outside this task. Then `make gate` printed `gate: ok`, 936 tests. | Ornith-1.5-35B-A3B (the session); design model (the commit) | | M5a/16-macos-gate | 2026-09-24 | stopped | 1 | fail | none | Created `.cargo/config.toml` (TMPDIR=/tmp for every process Cargo runs, tests included), copied the new `crates/toolkit/tests/shell.rs` from `docs/plans/M5a/files/`, and made the three edits to `read_checked` in `crates/gatewayd/src/secrets.rs`: the comment `"/proc/self"` -> `user_id()`, the three `/proc/self` lines -> `let owner = user_id()?;`, and the `user_id()` function (the uid of a freshly-made pipe) inserted above `value`. `cargo fmt --all`; `cargo test -p gatewayd -p toolkit -p brokerd` every suite passes. `make gate` never reaches the tests: it fails to compile `memoryd` on clippy `collapsible_match` at `crates/memoryd/src/index.rs:224` (the `LogRecord::User { time, content }` arm's nested `if !content.trim().is_empty()` guard), a crate this task does not touch and a file it does not list. Verified pre-existing and unrelated by stashing all my changes and re-running `make gate`: identical `memoryd` failure on the original committed code, so the gate does not compile `memoryd` with or without this task. Did not edit `memoryd` (outside this task's scope, AGENTS.md "do that task and nothing else"); committing only this log row per AGENTS.md point 4. | Ornith-1.5-35B-A3B |