Review M2b: accept with one follow-up task; record lessons
All ten tasks pass the checklist, the gate, the audit and the device checks, including a four-turn conversation with a loopd restart and no cache loss. Reading and probing found four low defects: the busy guard is released before the final frame on the main path but not on the three error paths, its Drop skips a poisoned lock, an unreadable core.md is treated as missing, and bxctl's interactive loop exits on a failed turn. Task 11 carries the fixes with two new tests, checked against a fixed copy of the branch. The Model column is filled in (all Ornith) and one malformed row is repaired. Two rules are promoted to AGENTS.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,74 @@
|
||||
# M2b task 11: four small fixes (review follow-up)
|
||||
|
||||
**Branch:** `m2b` (run `git switch m2b`; `git status --short` must be empty, otherwise stop)
|
||||
**Commit subject:** `Fix four review findings: busy release, poison recovery, core.md errors, chat loop`
|
||||
|
||||
## Goal
|
||||
|
||||
The M2b review found four small defects. Two have a test to copy in; two are rules to apply. None
|
||||
changes an interface.
|
||||
|
||||
## Files
|
||||
|
||||
- Copy (replacing the old ones): `crates/loopd/tests/baseline.rs`, `crates/bxctl/tests/chat.rs`
|
||||
- Modify: `crates/loopd/src/channel.rs`, `crates/loopd/src/baseline.rs`,
|
||||
`crates/bxctl/src/chat.rs`, `crates/bxctl/src/main.rs`, `docs/implementer-log.md`
|
||||
|
||||
## The four fixes
|
||||
|
||||
1. **`channel.rs`: release the session before *every* final frame.** Task 07 said to drop the
|
||||
busy guard before `turn_done` or the turn's error, and that is done. But the error frames for
|
||||
a session that cannot be opened or created (`no_such_session`, `session_exists`, `internal`)
|
||||
are still sent while the session is marked busy. `bxctl chat --session <new id>` reads
|
||||
`no_such_session` and at once sends the same turn again with `resume: false`, so it can be
|
||||
answered `session_busy` by a server thread that has not returned yet. Drop the guard before
|
||||
each of those three writes. The rule: the last frame of a connection is never sent while the
|
||||
session is busy.
|
||||
2. **`channel.rs`: a poisoned lock must not leave a session busy forever.** The guard's `Drop`
|
||||
does `if let Ok(mut busy) = self.ctx.busy.lock()`, which skips the removal when the lock is
|
||||
poisoned. Recover it with `unwrap_or_else(|p| p.into_inner())`, as `handle` already does.
|
||||
3. **`baseline.rs`: a `memory/core.md` that exists but cannot be read is an error.** Today
|
||||
`if let Ok(text) = read_to_string(..)` treats an unreadable file like a missing one, so a
|
||||
session starts without the memory the owner curated, and nothing says so. If the file exists,
|
||||
a read failure is `BaselineError::Read(path, e)`. A missing file is still fine.
|
||||
4. **`bxctl`: the interactive loop goes on after a failed turn, and `new_session_id` has no
|
||||
`expect`.** The spec says "A failed turn prints `bxctl: <error>` and the loop goes on"; today it
|
||||
exits 1. Report the error and continue; the session exists, so the next line resumes it. And
|
||||
`AGENTS.md` forbids `expect` in library code: replace the two in `new_session_id` with
|
||||
`unwrap_or_default()` for the clock and `unwrap_or_else` with a fixed valid id for the
|
||||
`SessionId` (it cannot fail, but the type must not be forced).
|
||||
|
||||
## Steps
|
||||
|
||||
- [ ] **1. Copy.**
|
||||
|
||||
```sh
|
||||
git switch m2b
|
||||
cp docs/plans/M2b/files/crates/loopd/tests/baseline.rs crates/loopd/tests/
|
||||
cp docs/plans/M2b/files/crates/bxctl/tests/chat.rs crates/bxctl/tests/
|
||||
```
|
||||
|
||||
- [ ] **2. See them fail.** `cargo test -p loopd --test baseline`: 1 of 7 fails,
|
||||
`an_unreadable_core_memory_file_is_an_error`. `cargo test -p bxctl --test chat`: 1 of 12 fails,
|
||||
`interactive_mode_survives_a_failed_turn`.
|
||||
- [ ] **3. Make the four fixes.** Run `cargo fmt --all`.
|
||||
- [ ] **4. See them pass.** `cargo test -p loopd --test baseline --test channel -p bxctl`.
|
||||
Expected: 7, 6 and 12 passed. Run the `channel` tests ten times in a row.
|
||||
- [ ] **5. Run the gate.** `make gate`. Expected last line: `gate: ok`.
|
||||
- [ ] **6. Log and commit.**
|
||||
|
||||
```sh
|
||||
git add crates/loopd crates/bxctl docs/implementer-log.md
|
||||
git commit
|
||||
```
|
||||
|
||||
## Done when
|
||||
|
||||
- `make gate` prints `gate: ok` with 219 tests.
|
||||
- `grep -n "expect(" crates/bxctl/src/chat.rs` prints nothing.
|
||||
- `grep -c "drop(held)" crates/loopd/src/channel.rs` prints 4 (or the guard is scoped so that
|
||||
every final frame is written after it is gone).
|
||||
|
||||
## Stop and report if
|
||||
|
||||
- `interactive_mode_survives_a_failed_turn` cannot pass without changing what `--say` does.
|
||||
Reference in New Issue
Block a user