diff --git a/AGENTS.md b/AGENTS.md index a5b8dab..aafee6b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -61,6 +61,9 @@ These come from defects found in review. The evidence is in `docs/implementer-le - If a tool you were told to use does not exist, stop and say so. Do not invent a command in its place. - A `read` returns as soon as it has any data to give and blocks only when it has none. +- A rule about one path applies to every path that does the same thing, including early returns + and error paths the task did not walk through. +- A file that exists but cannot be read is an error. Only a missing file may count as absent. ## The gate diff --git a/docs/implementer-lessons.md b/docs/implementer-lessons.md index 69fb6fd..ba77290 100644 --- a/docs/implementer-lessons.md +++ b/docs/implementer-lessons.md @@ -16,16 +16,18 @@ How it is used: | # | Tip | Evidence | In AGENTS.md | Seen again | |---|---|---|---|---| -| I1 | When a rule says "every" or "everywhere", finish by listing each place it could apply (every struct and enum in the file, every script, every branch) and check them one by one. An example in the task shows one place; the rule covers all of them. | M1 finding 1: `deny_unknown_fields` was put on both enums in task 06, where the task's example showed it, and on neither struct. In task 03, where the task said "works on structs and on enums", every type got it. | yes | M2a: no. All six config structs have it; none of the server-format structs do. | -| I2 | A check must fail when it cannot do its job: missing input, unreadable file, a tool that errors. Never throw errors away (`2>/dev/null`, `\|\| true`, an ignored `Result`). | M1 finding 4: all three gate scripts passed when `crates/` did not exist, and hid `find` and `awk` errors. | yes | M2a: no recurrence. | +| I1 | When a rule says "every" or "everywhere", finish by listing each place it could apply (every struct and enum in the file, every script, every branch) and check them one by one. An example in the task shows one place; the rule covers all of them. | M1 finding 1: `deny_unknown_fields` was put on both enums in task 06, where the task's example showed it, and on neither struct. In task 03, where the task said "works on structs and on enums", every type got it. | yes | M2a: no. All six config structs have it; none of the server-format structs do. M2b: partly. The busy-release rule was applied to the path the task named and missed on the three error paths that do the same thing (finding 1). | +| I2 | A check must fail when it cannot do its job: missing input, unreadable file, a tool that errors. Never throw errors away (`2>/dev/null`, `\|\| true`, an ignored `Result`). | M1 finding 4: all three gate scripts passed when `crates/` did not exist, and hid `find` and `awk` errors. | yes | M2a: no recurrence. M2b: partly. An unreadable core.md was treated as absent (finding 3), and a poisoned lock skipped the busy removal (finding 2). | | I3 | Report every problem found, not only the first. | M1 finding 8: `check-lines.sh` exits at the first long file. | yes | M2a: not exercised. | | I4 | Run the formatter before the gate, and let it decide ordering. | M1 finding 7: three first-gate failures were rustfmt reordering `lib.rs`. | yes | M2a: no fmt failures; the first-gate failures were clippy. | -| I5 | Log every attempt, including one you abandon. | M1 review: the task 01 row mentions files "from a prior attempt" that has no row of its own. | yes | M2a: partly. Task 11's `stopped` row was overwritten by its later `done` row. | +| I5 | Log every attempt, including one you abandon. | M1 review: the task 01 row mentions files "from a prior attempt" that has no row of its own. | yes | M2a: partly. Task 11's `stopped` row was overwritten by its later `done` row. M2b: no recurrence; all ten rows correct, one with a stray pipe. | | I6 | Prefer a conversion that can fail (`u32::try_from`) to a cast that is only safe because of a check somewhere else (`as u32`). | M1 finding 6: bounded `as` casts in `frame.rs`. Correct today, but the safety depends on a line ten lines away. | no (already implied by the code rules) | M2a: no `as` casts at all. | -| I7 | A `read` returns as soon as it has any data to give. It blocks only when it has none. Never keep reading to fill the caller's buffer. | M2a finding 2: the chunked reader delivered a whole stream at its end. | yes | | +| I7 | A `read` returns as soon as it has any data to give. It blocks only when it has none. Never keep reading to fill the caller's buffer. | M2a finding 2: the chunked reader delivered a whole stream at its end. | yes | M2b: no recurrence. | | I8 | When forwarding between two connections, pass a close on in both directions. Do not rely on the client half-closing first. | M2a finding 1. | no (specific to inferproxy) | | -| I9 | Never end a turn by describing what you are about to do. Do it, then report. | Two Ornith turns in M2a ended with a "## Objective" plan and no tool call. | yes | | +| I9 | Never end a turn by describing what you are about to do. Do it, then report. | Two Ornith turns in M2a ended with a "## Objective" plan and no tool call. | yes | M2b: not seen; all ten tasks ran unattended to a commit. | | I10 | If a tool you were told to use does not exist, stop and say so. Do not invent a command in its place. | Laguna as coordinator ran `opencodec`, which does not exist, and then diagnosed its own typo. | yes | | +| I11 | A rule about one path applies to every path that does the same thing. If a task says "release X before the final frame", every final frame counts, including the error frames written earlier in the function. | M2b finding 1. | yes | | +| I12 | A file that exists but cannot be read is an error. Only a file that does not exist may be treated as absent. | M2b finding 3. | yes | | ## Tips for writing tasks @@ -44,6 +46,8 @@ How it is used: | T11 | Make the fake client behave like the real one. If the real client never half-closes, no test client may half-close either. | M2a finding 1: all test clients half-closed; `loopd` does not. | | T12 | A weak model should not coordinate other sessions. Use the shell driver (`tools/run-plan.sh`), whose checks are code. | The `opencodec` episode. | | T13 | Set the sampling defaults on the server for coding agents (`temp`, `top-p`, `top-k`, `reasoning-budget`). OpenCode sends none, so the server's default temperature of 1.0 applies, and thinking has no cap. | Five Ornith turns ran 16k tokens of thinking to the output limit and produced nothing. | +| T14 | When a rule has a "before the final frame" or "on every exit" shape, list the exits in the task, or say "including the early returns". The implementer follows the path the task walks through. | M2b finding 1: task 07 walked the success path and the turn's error; the open and create errors were early returns it did not mention. | +| T15 | Running the whole plan through `tools/run-plan.sh` worked: ten tasks, one commit each, unattended, in about three hours. Keep the TUI closed while it runs; a second message into the driver's session starts a second agent on the same tree. | M2b run, 2026-09-18. | ## What worked and should be kept @@ -76,3 +80,8 @@ missing; its two failure modes were runaway thinking to the output limit and end plan instead of a tool call. GLM-5.3 did one task, the assembler, correctly and quickly, with the best comments of the milestone. Laguna did three and a half tasks in M2a with more nudging than in M1, and failed as a coordinator. First-gate pass rate for the milestone: 8 of 13. + +M2b was Ornith alone, driven by the shell script: ten tasks, ten commits, no intervention, first +gate 4 of 10, and four low findings in review, two of which are the "everywhere" pattern again in +a new shape (paths instead of types). Its code reads well and follows the numbered steps in the +tasks closely; where the task did not walk a path, the rule was not applied there. diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 58600c3..b82de55 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -31,15 +31,15 @@ reviewer adds findings under "Reviews" once per milestone. | M2a/14-inferproxy-close | 2026-09-18 | done | 2 | fail | none | Made the proxy close towards the client as soon as the server-to-client copy ends, for any reason. `forward` now joins only the s2c thread and returns the c2s JoinHandle, so it returns when the server stops sending instead of waiting for the client to stop sending too; `handle` drops the OpenGuard inside a block scope, then shuts down the client (Both) and server (Both) so the client's read returns EOF at once and the c2s thread ends, then joins c2s. This is rule 4 of task 02 (drop the open-place before closing the client). The half-close when the client stops sending first is unchanged. The copied `forward.rs` is byte-identical to the plan. 7 passed ten runs in a row; `make gate` prints `gate: ok`. | ? | Wrote no library code. Put the two given files in place (`crates/loopd/tests/device.rs`, and a `Makefile` whose only difference from the old one is the new `verify-device` target), confirmed `make gate` prints `gate: ok` with `device` at `0 passed; 0 failed; 4 ignored`, and `curl http://straylight:11434/health` returned `{"status":"ok"}`. `make verify-device` ran the four `#[ignore]` checks one at a time against the real server on slot 0: self-test, capped thinking block, a request surviving its own proxy being killed and restarted, and a second turn reusing the cache of a first turn that contained thinking. All 4 passed in ~21s (well under two minutes). | ? | Wrote crates/loopd/src/selftest.rs (SelfTestError with Display/std::error::Error/From, and `run` doing the three checks in order) and registered `pub mod selftest;` in lib.rs; rewrote main.rs into `loopd selftest --config `. Check 1 calls props() and compares chat-template sha256, then n_ctx, then total_slots, a sha256 error being Hash so a wrong server is sent no prompt; check 2 posts one read_file tool with chat_with_retry and requires finish_reason ToolCalls, a first call named read_file whose arguments parse as JSON carrying a string path, wrapping an InferError as Infer; check 3 runs turn 1 then an extension of it and maps a cache Loss to CacheMiss. The copied test's cache_outcome/CacheOutcome live at crate::llama::info, so the import follows that. main.rs parses args as &[&str] via a two-step String->&str collect; unknown/missing args are exit 2 and a config load failure is `loopd: ` exit 1. Real server via inferproxy against straylight: minimal.toml gave three step lines and `selftest: ok` exit 0; setting slots=3 gave `selftest: FAILED: slot count: expected 3, got 2` exit 1. | Ornith-1.5-35B-A3B | | M2a/15-http-streaming | 2026-09-18 | done | 1 | pass | none | Fixed `read_chunked` so `Body::read` in the Chunked phase returns as soon as it has copied at least one byte of chunk data, even when the caller's buffer is not full and the chunk's trailing CRLF has not arrived; the CRLF is consumed at the start of the next call. It reads from the socket only when it has no data to give (a chunk-size line, a pending CRLF, or the trailers). The old `Data` arm looped back to read more from the socket whenever the buffer was not full and the chunk was not done, so a body streamed in 300 ms pieces arrived in one burst. All 16 http tests pass (the new `streamed_data_is_delivered_as_it_arrives` and `the_result_does_not_depend_on_how_the_bytes_arrive`), all loopd tests pass, `make gate` prints `gate: ok`. `cargo fmt --all` re-sorted a stray unused `use std::sync::mpsc;` left uncommitted in crates/inferproxy/src/lib.rs by a prior session; restored that file to HEAD so the commit stays scoped to crates/loopd. | Ornith-1.5-35B-A3B | | M2b/01-proto-channel-types | 2026-09-18 | done | 1 | pass | none | Added `Usage` struct and a `Usage` variant (between ToolResult and CacheLoss) in log.rs, and `Turn`, `TurnEvent`, `TurnDone` plus six `ErrorCode` variants (SessionFull..Inference) and three `Message` variants (after Error) in wire.rs; re-exported Usage, Turn, TurnEvent, TurnDone from lib.rs. All four new types carry `deny_unknown_fields`; field order matches the byte-exact fixtures (attempt/after_ms/error, name/class/truncated). 55 proto tests pass (turn_wire 5, strict 5, wire 9, ids 12, frame 13, grant 4, hash 4, records 3) and `make gate` prints `gate: ok`; the old fixtures stay byte-identical. One duplicate block of the three wire types left by an interrupted edit had to be removed mid-task. | Ornith-1.5-35B-A3B | -| M2b/02-loopd-config | 2026-09-18 | done | 2 | fail | none | Added Paths/Channel/Loop/Baseline structs to config.rs with `#[serde(deny_unknown_fields, default)]` and Debug/Clone/PartialEq/Eq/Deserialize (Default derived for Channel, hand-written for the other three: home is $BOXMAKER_HOME else /var/lib/boxmaker, loop is 8/true/16384, baseline system is system.md); Config gained the four `#[serde(default)]` fields and `channel_socket()` fills the default `/run/loop/loop.sock` when the socket is empty. `load` joins `baseline.system` to the config file's directory via `parent.join` (which replaces an already-absolute path); `parse` leaves it. 9 config tests pass, `deny_unknown_fields` count is 10. Two clippy fixes on the first (failing) gate run: the nested if in `load` collapsed by relying on Path::join replacing absolute paths instead of a 1.98 let-chain, and `Path::is_empty` (stable 1.98) replaced with `as_os_str().is_empty()`. | ? | -| M2b/03-loopd-tools | 2026-09-18 | done | 1 | pass | none | Added `pub mod tools;` to lib.rs and `serde::Serialize`/`serde::Deserialize`/`deny_unknown_fields` to ToolSchema; wrote crates/loopd/src/tools.rs with ToolPort, Entry, Registry (m2b/config), core_schemas/get/find, FIND_TOOL/CALL_TOOL constants, clock_schema/echo_schema, Dispatch with dispatch (find_tool/call_tool/local dispatch rows), cap_result via floor_char_boundary, and FakeTools recording calls and answering clock/echo/denying others with `unwrap_or_else(|p| p.into_inner())` on Mutex::lock. 7 tools tests pass, `make gate` prints `gate: ok`, no `unwrap()` in tools.rs. | ? | -| M2b/04-loopd-baseline | 2026-09-18 | done | 2 | fail | none | Wrote crates/loopd/src/baseline.rs: `Baseline` (system prompt + core tool schemas, `deny_unknown_fields`), `BaselineError` (Read/Parse name the file, plus Hash) with Display/std::error::Error, `assemble` (system prompt trimmed of trailing whitespace, core memory appended with a blank line when its trimmed content is non-empty), `to_json`/`from_json`, `load`, and `hash` (sha256 of the canonical JSON). `messages` prepends the system message and replays every `LogRecord` variant explicitly named, so a new one is a compile error. The `\n\n` separator between system prompt and core memory had to be two newlines (a blank line), not one. All 6 baseline tests pass and all loopd tests pass with the new support module; first gate run failed on a rustfmt import-order diff, fixed with `cargo fmt`. | ? | -| M2b/05-loopd-session | 2026-09-18 | done | 2 | fail | none | Copied the given test byte-identical and wrote crates/loopd/src/session.rs: `Session` (id, dir, baseline, records, appended log file, next_call) and `SessionError` (Exists/NotFound/Io/Torn/Baseline/Encode) with derived Debug, Display and std::error::Error::source. `create` refuses an existing dir, writes `0.baseline.json`, opens `0.jsonl` with `create_new`+`append`, and appends a `SessionStart` (`Timestamp::now()`, epoch 0, the slot, `baseline.hash()`). `open` reads the baseline from the file (not `system.md`), requires every log line to end in `\n` and parse as a `LogRecord` else `Torn` with the 1-based line and reason, and sets `next_call` to one past the highest `ToolResult` call. `append` encodes, writes, `sync_data()`, then pushes to memory. All 7 session tests pass. First gate failed on clippy: split the `source()` arm that bound three different error types into three arms, removed the redundant `.write(true)` (implied by `append`), and used `path.display()` for the `Torn` path. | ? | -| M2b/06-loopd-turn | 2026-09-18 | done | 1 | pass | none | Wrote crates/loopd/src/turn.rs (299 lines) and registered `pub mod turn;` in lib.rs; copied the two given tests and three fixtures byte-identical. `TurnError` (SessionFull/TurnLimit/Infer/Session, Display + std::error::Error + From), `TurnOutcome`, `Runtime`, `is_context_full` (the one 400 whose JSON `error.type` is `exceed_context_size_error`), and `run_turn`: append User, build the ChatRequest (slot, messages, tools, thinking), capture `last_usage`, `chat_with_retry` mapping ChatEvent->TurnEvent (dropping ToolCallDelta), append Assistant then Usage, report cache loss between the two conversations, and on no tool calls return `TurnOutcome { content: completion.content.unwrap_or_default(), usage }`; otherwise iterate tool calls under the iteration cap with a repeated-call detector (first repeat returns "already called", a second repeat is TurnLimit), `cap_result`, and `dispatch` (find_tool/call_tool local, every other tool — including read_file — to the port). `run_call` maps Dispatch::Local and every ToolResponse variant to (text, Public, untrusted). Two compile fixes before the gate: `u64::try_from(*ahead).unwrap_or(u64::MAX)` (usize has no From) and `let Ok(value) = from_str(body) else { return false }` (a temporary borrow); `session.baseline()` returns a reference so it is bound inside the loop. All 6 turn and 9 limits tests pass; `make gate` prints `gate: ok`. | ? | -| M2b/07-loopd-channel | 2026-09-18 | done | 3 | fail | none | Wrote crates/loopd/src/channel.rs and registered `pub mod channel;` in lib.rs. `Context` holds a private `Mutex>`; `serve` accepts forever with one thread per connection and returns on an accept error; `handle` does the seven steps (read_frame with Closed-before-anything, turn-only, mark busy, open-or-create plus run_turn streaming events, release before the final frame, error-code mapping, quiet write failure). The busy guard is a `Held` struct that borrows the context immutably and holds a clone of the id but never the lock, and it is dropped before sending turn_done or error so a client can send the next turn the moment it reads the last one — that is what keeps `a_busy_session_is_refused_at_once` and the concurrent-session test correct. Channel test reported `6 passed` ten runs in a row, all clean. Two fixes before a clean gate: `cargo fmt` import order and a clippy `question_mark` on the accept loop, re-run after each. | ? | -| M2b/08-loopd-serve | 2026-09-18 | done | 2 | fail | none | Rewrote crates/loopd/src/main.rs into two commands, `selftest` and `serve`, both sharing `run_selftest_check` so the self-test lines are identical. `serve` loads config (exit 1 on failure), removes an existing socket via `channel_socket()` before the self-test, runs the self-test and exits 1 without binding on failure, then creates the socket's parent dir, binds, sets mode 0600 with `std::fs::set_permissions`, prints `serving on`, and calls `channel::serve` with a `Context` from the config, client, `Box::new(FakeTools::new())` and `Registry::m2b()`. Anything else prints both usages and exits 2. The `serve_refuses...` test's "no socket left behind" holds because the socket is removed before the self-test and binding happens only after it passes. First gate run failed on two clippy `collapsible_if` lints; collapsed the two nested `if let` into edition-2024 let-chains and re-ran, which passed. `cargo test -p loopd --test serve` reports 3 passed. | ? | -| M2b/09-bxctl-chat | 2026-09-18 | done | 5 | fail | none | Wrote crates/bxctl/src/chat.rs: `run_turn` (open socket, one `write_frame` with id 1, loop `read_frame` asserting id 1, dispatch final `TurnDone`/`Error` and non-final `TurnEvent` to `on_event`, every other frame `Protocol`); `ChatError` (Connect/Frame/Refused/Protocol) with `source()` returning the io and FrameError; `new_session_id` = `chat--` via two `expect`s (the epoch check and a private-field construction that cannot fail); `Printer` with json mode (one serde_json line per event, no skipping, no escape codes), a dimmed reasoning block opened on the first `Reasoning` and closed on the next non-reasoning event or `end_reasoning`, and every other event kind named exactly. Registered `pub mod chat;` in lib.rs. Rewrote main.rs into a `chat` subcommand: usage + exit 2 for a wrong first arg or unknown flag/missing value/invalid id, `$BOXMAKER_HOME/run/loop/loop.sock` else `/var/lib/boxmaker/...`, `--say` (events to stderr, answer to stdout, resume=true then one retry with resume=false on no_such_session), interactive (create on first turn, resume on the rest, `/quit` stops, the created session id printed once to stdout), `--json` (events to stderr, the TurnDone also to stderr after them, plain answer to stdout). A `Sink` records the first write error so the `on_event` closure (which cannot return a Result) does not lose it. All 11 chat tests pass. Four gate runs before clean: clippy `io_other_error` (switched to `Error::other`), then `redundant_closure` twice (the `other` map and `get_or_insert_with`), then a rustfmt import-order diff. | ? | Committed Cargo.lock alongside bxctl: adding serde_json to bxctl's Cargo.toml changes the workspace lock, and the gate's `--locked` deny check would otherwise fail on the checked-out tree. | -| M2b/10-verify-device | 2026-09-18 | done | 1 | pass | none | No library code. Copied the three given files byte-identical (`cmp` clean): `crates/loopd/tests/device.rs` (replaces the M2a one, its four checks still in it), `Makefile` (only change: `verify-device` now also passes `BOXMAKER_BXCTL`), and `config/system.md`. `make gate` printed `gate: ok` with device at `0 passed; 0 failed; 6 ignored`. `curl http://straylight:11434/health` returned `{"status":"ok"}`. `make verify-device` ran all six checks against the real server in 41.6s, all passed: self-test, capped-thinking block, a four-turn conversation surviving a `loopd` restart with its cache, a request surviving its proxy being killed and restarted, a second turn reusing the first turn's cache, and the baseline fitting the token budget. The baseline is 251 tokens (the brief allows 3000). Ran directly rather than via a subagent: the `delegate` tool returned `Agent "undefined" not found` on every attempt. | ? | +| M2b/02-loopd-config | 2026-09-18 | done | 2 | fail | none | Added Paths/Channel/Loop/Baseline structs to config.rs with `#[serde(deny_unknown_fields, default)]` and Debug/Clone/PartialEq/Eq/Deserialize (Default derived for Channel, hand-written for the other three: home is $BOXMAKER_HOME else /var/lib/boxmaker, loop is 8/true/16384, baseline system is system.md); Config gained the four `#[serde(default)]` fields and `channel_socket()` fills the default `/run/loop/loop.sock` when the socket is empty. `load` joins `baseline.system` to the config file's directory via `parent.join` (which replaces an already-absolute path); `parse` leaves it. 9 config tests pass, `deny_unknown_fields` count is 10. Two clippy fixes on the first (failing) gate run: the nested if in `load` collapsed by relying on Path::join replacing absolute paths instead of a 1.98 let-chain, and `Path::is_empty` (stable 1.98) replaced with `as_os_str().is_empty()`. | Ornith-1.5-35B-A3B | +| M2b/03-loopd-tools | 2026-09-18 | done | 1 | pass | none | Added `pub mod tools;` to lib.rs and `serde::Serialize`/`serde::Deserialize`/`deny_unknown_fields` to ToolSchema; wrote crates/loopd/src/tools.rs with ToolPort, Entry, Registry (m2b/config), core_schemas/get/find, FIND_TOOL/CALL_TOOL constants, clock_schema/echo_schema, Dispatch with dispatch (find_tool/call_tool/local dispatch rows), cap_result via floor_char_boundary, and FakeTools recording calls and answering clock/echo/denying others with `unwrap_or_else(/p/p.into_inner())` on Mutex::lock. 7 tools tests pass, `make gate` prints `gate: ok`, no `unwrap()` in tools.rs. | Ornith-1.5-35B-A3B | +| M2b/04-loopd-baseline | 2026-09-18 | done | 2 | fail | none | Wrote crates/loopd/src/baseline.rs: `Baseline` (system prompt + core tool schemas, `deny_unknown_fields`), `BaselineError` (Read/Parse name the file, plus Hash) with Display/std::error::Error, `assemble` (system prompt trimmed of trailing whitespace, core memory appended with a blank line when its trimmed content is non-empty), `to_json`/`from_json`, `load`, and `hash` (sha256 of the canonical JSON). `messages` prepends the system message and replays every `LogRecord` variant explicitly named, so a new one is a compile error. The `\n\n` separator between system prompt and core memory had to be two newlines (a blank line), not one. All 6 baseline tests pass and all loopd tests pass with the new support module; first gate run failed on a rustfmt import-order diff, fixed with `cargo fmt`. | Ornith-1.5-35B-A3B | +| M2b/05-loopd-session | 2026-09-18 | done | 2 | fail | none | Copied the given test byte-identical and wrote crates/loopd/src/session.rs: `Session` (id, dir, baseline, records, appended log file, next_call) and `SessionError` (Exists/NotFound/Io/Torn/Baseline/Encode) with derived Debug, Display and std::error::Error::source. `create` refuses an existing dir, writes `0.baseline.json`, opens `0.jsonl` with `create_new`+`append`, and appends a `SessionStart` (`Timestamp::now()`, epoch 0, the slot, `baseline.hash()`). `open` reads the baseline from the file (not `system.md`), requires every log line to end in `\n` and parse as a `LogRecord` else `Torn` with the 1-based line and reason, and sets `next_call` to one past the highest `ToolResult` call. `append` encodes, writes, `sync_data()`, then pushes to memory. All 7 session tests pass. First gate failed on clippy: split the `source()` arm that bound three different error types into three arms, removed the redundant `.write(true)` (implied by `append`), and used `path.display()` for the `Torn` path. | Ornith-1.5-35B-A3B | +| M2b/06-loopd-turn | 2026-09-18 | done | 1 | pass | none | Wrote crates/loopd/src/turn.rs (299 lines) and registered `pub mod turn;` in lib.rs; copied the two given tests and three fixtures byte-identical. `TurnError` (SessionFull/TurnLimit/Infer/Session, Display + std::error::Error + From), `TurnOutcome`, `Runtime`, `is_context_full` (the one 400 whose JSON `error.type` is `exceed_context_size_error`), and `run_turn`: append User, build the ChatRequest (slot, messages, tools, thinking), capture `last_usage`, `chat_with_retry` mapping ChatEvent->TurnEvent (dropping ToolCallDelta), append Assistant then Usage, report cache loss between the two conversations, and on no tool calls return `TurnOutcome { content: completion.content.unwrap_or_default(), usage }`; otherwise iterate tool calls under the iteration cap with a repeated-call detector (first repeat returns "already called", a second repeat is TurnLimit), `cap_result`, and `dispatch` (find_tool/call_tool local, every other tool — including read_file — to the port). `run_call` maps Dispatch::Local and every ToolResponse variant to (text, Public, untrusted). Two compile fixes before the gate: `u64::try_from(*ahead).unwrap_or(u64::MAX)` (usize has no From) and `let Ok(value) = from_str(body) else { return false }` (a temporary borrow); `session.baseline()` returns a reference so it is bound inside the loop. All 6 turn and 9 limits tests pass; `make gate` prints `gate: ok`. | Ornith-1.5-35B-A3B | +| M2b/07-loopd-channel | 2026-09-18 | done | 3 | fail | none | Wrote crates/loopd/src/channel.rs and registered `pub mod channel;` in lib.rs. `Context` holds a private `Mutex>`; `serve` accepts forever with one thread per connection and returns on an accept error; `handle` does the seven steps (read_frame with Closed-before-anything, turn-only, mark busy, open-or-create plus run_turn streaming events, release before the final frame, error-code mapping, quiet write failure). The busy guard is a `Held` struct that borrows the context immutably and holds a clone of the id but never the lock, and it is dropped before sending turn_done or error so a client can send the next turn the moment it reads the last one — that is what keeps `a_busy_session_is_refused_at_once` and the concurrent-session test correct. Channel test reported `6 passed` ten runs in a row, all clean. Two fixes before a clean gate: `cargo fmt` import order and a clippy `question_mark` on the accept loop, re-run after each. | Ornith-1.5-35B-A3B | +| M2b/08-loopd-serve | 2026-09-18 | done | 2 | fail | none | Rewrote crates/loopd/src/main.rs into two commands, `selftest` and `serve`, both sharing `run_selftest_check` so the self-test lines are identical. `serve` loads config (exit 1 on failure), removes an existing socket via `channel_socket()` before the self-test, runs the self-test and exits 1 without binding on failure, then creates the socket's parent dir, binds, sets mode 0600 with `std::fs::set_permissions`, prints `serving on`, and calls `channel::serve` with a `Context` from the config, client, `Box::new(FakeTools::new())` and `Registry::m2b()`. Anything else prints both usages and exits 2. The `serve_refuses...` test's "no socket left behind" holds because the socket is removed before the self-test and binding happens only after it passes. First gate run failed on two clippy `collapsible_if` lints; collapsed the two nested `if let` into edition-2024 let-chains and re-ran, which passed. `cargo test -p loopd --test serve` reports 3 passed. | Ornith-1.5-35B-A3B | +| M2b/09-bxctl-chat | 2026-09-18 | done | 5 | fail | none | Wrote crates/bxctl/src/chat.rs: `run_turn` (open socket, one `write_frame` with id 1, loop `read_frame` asserting id 1, dispatch final `TurnDone`/`Error` and non-final `TurnEvent` to `on_event`, every other frame `Protocol`); `ChatError` (Connect/Frame/Refused/Protocol) with `source()` returning the io and FrameError; `new_session_id` = `chat--` via two `expect`s (the epoch check and a private-field construction that cannot fail); `Printer` with json mode (one serde_json line per event, no skipping, no escape codes), a dimmed reasoning block opened on the first `Reasoning` and closed on the next non-reasoning event or `end_reasoning`, and every other event kind named exactly. Registered `pub mod chat;` in lib.rs. Rewrote main.rs into a `chat` subcommand: usage + exit 2 for a wrong first arg or unknown flag/missing value/invalid id, `$BOXMAKER_HOME/run/loop/loop.sock` else `/var/lib/boxmaker/...`, `--say` (events to stderr, answer to stdout, resume=true then one retry with resume=false on no_such_session), interactive (create on first turn, resume on the rest, `/quit` stops, the created session id printed once to stdout), `--json` (events to stderr, the TurnDone also to stderr after them, plain answer to stdout). A `Sink` records the first write error so the `on_event` closure (which cannot return a Result) does not lose it. All 11 chat tests pass. Four gate runs before clean: clippy `io_other_error` (switched to `Error::other`), then `redundant_closure` twice (the `other` map and `get_or_insert_with`), then a rustfmt import-order diff./? | Ornith-1.5-35B-A3B | +| M2b/10-verify-device | 2026-09-18 | done | 1 | pass | none | No library code. Copied the three given files byte-identical (`cmp` clean): `crates/loopd/tests/device.rs` (replaces the M2a one, its four checks still in it), `Makefile` (only change: `verify-device` now also passes `BOXMAKER_BXCTL`), and `config/system.md`. `make gate` printed `gate: ok` with device at `0 passed; 0 failed; 6 ignored`. `curl http://straylight:11434/health` returned `{"status":"ok"}`. `make verify-device` ran all six checks against the real server in 41.6s, all passed: self-test, capped-thinking block, a four-turn conversation surviving a `loopd` restart with its cache, a request surviving its proxy being killed and restarted, a second turn reusing the first turn's cache, and the baseline fitting the token budget. The baseline is 251 tokens (the brief allows 3000). Ran directly rather than via a subagent: the `delegate` tool returned `Agent "undefined" not found` on every attempt. | Ornith-1.5-35B-A3B | ## Reviews @@ -179,3 +179,46 @@ stop when the subagent tool it was told to use was not available. Task 14 kept rule 4 of task 02: the open-connection place is released before the client is closed. Task 15 was the smallest correct change: the `Data` phase returns after copying, and the chunk's CRLF is consumed at the start of the next read. + +### M2b, tasks 01 to 10 — reviewed 2026-09-18 by the design model (Claude) + +**Verdict: accepted, with one follow-up task (11) of four small fixes.** Nothing has to be +redone. All ten tasks by Ornith-1.5-35B-A3B, driven by `tools/run-plan.sh` in fresh sessions. + +Checklist from `docs/plans/M2b/README.md`: + +| Check | Result | +|---|---| +| Ten commits, one per task, each with the trailer | pass | +| All 27 copied files byte-identical to the plan | pass | +| No change to the brief, specs, plans, `AGENTS.md`, `CLAUDE.md`, `deny.toml` | pass | +| `make gate` | `gate: ok`, 217 tests, 6 ignored (the reference had the same numbers) | +| `make audit` | `advisories ok` | +| `make verify-device` on straylight | 6 passed in 36 s, including the four-turn conversation with a `loopd` restart and no cache loss | +| `channel`, `serve`, `limits`, `turn`, `session` and `bxctl`'s `chat` suites 12 times each under heavy CPU load | no failure | +| No `unwrap`, `panic!`, `#[allow]`, `unsafe` or `as` cast in new library code | pass, except two `expect` in `bxctl` (finding 4) | +| `deny_unknown_fields` on every new struct of ours (config, baseline, wire, log), on none of the server's | pass | + +Probes from outside the tests: 15 `bxctl chat --session ` runs in a row, each refused for +resume and then created (no busy refusal seen; see finding 1 for why it could happen); a log with a +hand-inserted unknown record, refused with the line number; two sessions sending at once on one +slot, the second told `queued { ahead: 1 }`. + +Process: first gate run passed in 4 of 10 tasks. The failures were compile errors and clippy lints +fixed within the session. Task 09 took five gate runs. Time from the first commit to the last: 3 h +8 min, unattended. + +| # | Severity | Owner | Finding | Fix | +|---|---|---|---|---| +| 1 | low | implementer (task 07) and task | The busy guard is released before `turn_done` and the turn's error, as the task said, but not before the three error frames for a session that cannot be opened or created. `bxctl`'s create-after-`no_such_session` retry can hit `session_busy`. The task stated the rule for one path; the general rule is "never send a final frame while the session is busy". | Task 11 | +| 2 | low | implementer | The guard's `Drop` skips the removal when the lock is poisoned, leaving the session busy for the life of the process. `handle` itself recovers a poisoned lock. | Task 11 | +| 3 | low | implementer | An unreadable `memory/core.md` is treated like a missing one; the session starts without the owner's memory and nothing says so. | Task 11, with a test | +| 4 | low | implementer | `bxctl`'s interactive loop exits on a failed turn; the spec says it goes on. Two `expect` calls in `new_session_id`. | Task 11, with a test | +| 5 | nit | implementer | `turn.rs` carries a comment about "the recordings" that belongs to the tests, not the code. A whitespace-only `find_tool` query is not trimmed. | When next touched | + +What was good: the turn loop is 299 lines and reads top to bottom as the spec's numbered list; +`messages` names every record variant; the session store syncs before it remembers; the channel +server releases the session before the final frame exactly as asked; the fixes to the M2a lessons +held (a `read` returns as soon as it has data; unknown fields rejected in ours, ignored in the +server's). On straylight the model used `clock`, then `find_tool` and `call_tool` for `echo`, +unprompted, and the log shows one `Usage` per completion and not one `CacheLoss`. diff --git a/docs/plans/M2b/11-review-fixes.md b/docs/plans/M2b/11-review-fixes.md new file mode 100644 index 0000000..96c2070 --- /dev/null +++ b/docs/plans/M2b/11-review-fixes.md @@ -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 ` 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: ` 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. diff --git a/docs/plans/M2b/README.md b/docs/plans/M2b/README.md index 18af3f9..be9b457 100644 --- a/docs/plans/M2b/README.md +++ b/docs/plans/M2b/README.md @@ -1,7 +1,7 @@ # M2b implementation plan: the agent loop > **For the implementing model:** do not work from this file. The owner gives you one task file at -> a time (`01-…` to `10-…`). This file is the index for the owner and the reviewer. +> a time (`01-…` to `11-…`). This file is the index for the owner and the reviewer. **Goal:** `loopd serve` holds conversations: each turn's request extends the one before, sessions live on disk and survive a restart, tool calls go through a port with limits on every kind of @@ -25,7 +25,7 @@ M1 frame protocol. `bxctl chat` is a client of that protocol. - Nothing is written to a session log until a completion is final. Nothing volatile is ever put in a message or the baseline. - Branch `m2b`. One task, one fresh OpenCode session, one commit. Run `cargo fmt --all` before the - gate. Review happens once, after task 10. + gate. Review happened once, after task 10; task 11 is its follow-up. ## Tasks @@ -41,6 +41,7 @@ M1 frame protocol. `bxctl chat` is a client of that protocol. | 08 | `08-loopd-serve.md` | `loopd serve` | `loopd/tests/serve.rs` | | 09 | `09-bxctl-chat.md` | `bxctl chat` | `bxctl/tests/chat.rs` | | 10 | `10-verify-device.md` | `make verify-device` extended; the first `config/system.md` | `loopd/tests/device.rs` | +| 11 | `11-review-fixes.md` | Review follow-up: busy release before every final frame, poison recovery, unreadable `core.md`, chat loop continues | updated `loopd/tests/baseline.rs`, `bxctl/tests/chat.rs` | `files/` holds everything the tasks copy in. As in M2a, all of it was checked against a private reference implementation: the gate passes after every task in order, the new suites pass under CPU diff --git a/docs/plans/M2b/files/crates/bxctl/tests/chat.rs b/docs/plans/M2b/files/crates/bxctl/tests/chat.rs index fa3e845..061a097 100644 --- a/docs/plans/M2b/files/crates/bxctl/tests/chat.rs +++ b/docs/plans/M2b/files/crates/bxctl/tests/chat.rs @@ -57,6 +57,8 @@ fn fake_loopd(events: Vec, end: End) -> FakeLoopd { let id = request.id; let end = if turn.resume && turn.content == "trigger-no-such-session" { End::Error(ErrorCode::NoSuchSession, "session x does not exist") + } else if turn.content == "trigger-turn-limit" { + End::Error(ErrorCode::TurnLimit, "the turn hit a limit") } else { end.clone() }; @@ -417,3 +419,45 @@ fn bad_arguments_print_usage() { .unwrap(); assert_eq!(output.status.code(), Some(2)); } + +/// A failed turn is reported, and the conversation goes on: the session still exists and the +/// next line is a new turn on it. +#[test] +fn interactive_mode_survives_a_failed_turn() { + let fake = fake_loopd( + vec![TurnEvent::Content { + text: "ok".to_string(), + }], + End::Done(TurnDone { + content: "ok".to_string(), + usage: usage(), + }), + ); + let mut child = Command::new(env!("CARGO_BIN_EXE_bxctl")) + .args(["chat", "--socket"]) + .arg(&fake.socket) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .unwrap(); + { + let mut stdin = child.stdin.take().unwrap(); + std::io::Write::write_all(&mut stdin, b"first\ntrigger-turn-limit\nthird\n/quit\n") + .unwrap(); + } + let output = child.wait_with_output().unwrap(); + assert!( + output.status.success(), + "a failed turn does not end the chat" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("turn limit: the turn hit a limit"), + "{stderr}" + ); + let turns = fake.turns.lock().unwrap(); + assert_eq!(turns.len(), 3, "the turn after the failure was sent"); + assert!(turns[2].resume, "and it resumed the same session"); + assert_eq!(turns[2].session, turns[0].session); +} diff --git a/docs/plans/M2b/files/crates/loopd/tests/baseline.rs b/docs/plans/M2b/files/crates/loopd/tests/baseline.rs index 7efd4fd..1e70921 100644 --- a/docs/plans/M2b/files/crates/loopd/tests/baseline.rs +++ b/docs/plans/M2b/files/crates/loopd/tests/baseline.rs @@ -232,3 +232,28 @@ fn replay_of_a_prefix_is_a_prefix() { assert_eq!(whole[..part.len()], part[..], "prefix of {n} records"); } } + +/// A core memory file that exists but cannot be read is an error, not silently absent: the owner +/// would otherwise get a session without the memory they curated, and no sign of it. +#[test] +fn an_unreadable_core_memory_file_is_an_error() { + use std::os::unix::fs::PermissionsExt; + if running_as_root() { + return; // root can read anything; the check is meaningless there + } + let home = Home::new(); + let cfg = home.config(Path::new("/tmp/unused.sock")); + home.write("memory/core.md", "secret memory\n"); + let core = home.dir.join("memory/core.md"); + std::fs::set_permissions(&core, std::fs::Permissions::from_mode(0o000)).unwrap(); + let result = Baseline::assemble(&cfg, &Registry::m2b()); + std::fs::set_permissions(&core, std::fs::Permissions::from_mode(0o644)).unwrap(); + let e = result.expect_err("an unreadable core.md must not be ignored"); + assert!(e.to_string().contains("core.md"), "{e}"); +} + +fn running_as_root() -> bool { + std::fs::read_to_string("/proc/self/status") + .map(|s| s.lines().any(|l| l.starts_with("Uid:\t0\t"))) + .unwrap_or(false) +} diff --git a/docs/specs/2026-09-18-m2b-agent-loop.md b/docs/specs/2026-09-18-m2b-agent-loop.md index 4058e08..b95a659 100644 --- a/docs/specs/2026-09-18-m2b-agent-loop.md +++ b/docs/specs/2026-09-18-m2b-agent-loop.md @@ -158,8 +158,9 @@ Events for one turn arrive in order on one connection. Nothing else is promised. (from config, `[channel] socket`) with mode 0600; accepts connections, one thread each. The M2a `Client` is shared in an `Arc`; its slot gate serialises requests per slot. A registry of per-session locks makes a concurrent `turn` on a busy session answer `session_busy` at once; the -lock is released before the final frame is sent, so a channel that sends its next turn on reading -that frame is never refused. A stale socket file is removed before the self-test runs, so clients +lock is released before the final frame is sent, whichever frame that is (`turn_done`, the turn's +error, or the error for a session that cannot be opened or created), so a channel that sends its +next turn on reading that frame is never refused. A stale socket file is removed before the self-test runs, so clients see "not running" rather than "connection refused" during startup. Nothing is held in memory that is not also on disk, except the locks and the `call` counters.