Hand over the M3a plan: 22 tasks, their files, and the check record
Task files, the files they copy in (byte-identical to the reference on m3a-ref), each area's check record, and a README with the per-task table of what each check exposed. The handoff note is done with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,107 @@
|
||||
# M3a checks, fork C: `loopd` (tasks 16 and 17)
|
||||
|
||||
Reference tree: `~/src/boxmaker-ref-c`, branch `m3a-ref-c`. The rest of the workspace does not
|
||||
build its tests there (`proto`'s `records.rs` and `wire.rs` belong to other forks), so every
|
||||
command below is `-p loopd`. `cargo clippy -p loopd -p bxctl --all-targets -- -D warnings` is
|
||||
clean, and `scripts/check-lines.sh` passes.
|
||||
|
||||
## Task 16: the tool port, the registry and denials
|
||||
|
||||
**Check: in effect a reference.** The task changes working code, and the smallest change that lets
|
||||
the updated tests compile is the whole change (about 150 lines in `tools.rs`, 30 in `turn.rs`). A
|
||||
`todo!()` skeleton was not possible: the existing turn-loop tests run through this code.
|
||||
|
||||
Run to green, three times: `tools` (10), `turn` (6), `turn_broker` (5, new), and unchanged
|
||||
`limits` (9), `channel` (6), `session` (7), `baseline` (7), `serve` (3) and the rest of the suite.
|
||||
|
||||
Defects the check exposed:
|
||||
|
||||
1. **Spec, section 8.** "`echo` stays in `FakeTools` for tests only" is not enough: two recorded
|
||||
conversations (`find_tool.http`, `call_tool.http`) and four test files discover `echo`
|
||||
through the registry. Removing `Registry::m2b()` would have meant new copies of `baseline.rs`,
|
||||
`channel.rs` and `support/turn.rs` for a one-word change each. Resolved: `m2b()` stays as the
|
||||
test registry, `m3a()` is what `loopd serve` uses. Proposed wording for section 8: "`echo`
|
||||
stays in `FakeTools` and in the test registry `Registry::m2b()`; `loopd serve` uses
|
||||
`Registry::m3a()`."
|
||||
2. **Spec, section 8, unclear.** `tool_denied { name, reason }` does not say which name. For a
|
||||
`call_tool` call, `call.name` is `call_tool`, which tells the owner nothing. Resolved: both new
|
||||
events carry the tool `brokerd` decides on (`request.tool`); `tool_call_started` and
|
||||
`tool_result` keep `call.name`. A test pins it (`a_denied_call_tool_names_the_target_tool`).
|
||||
Task 20 (`bxctl chat`) should know that `ApprovalPending.tool` is the target tool.
|
||||
3. **Spec, silent.** What `run_call` does if a port returns `PendingApproval` as its answer. It
|
||||
cannot be removed (the `match` must be exhaustive while `ToolPort` returns `ToolResponse`).
|
||||
Resolved: a plain failure, "The tool failed: the tool broker gave no final answer", no event.
|
||||
It is a failure of the port, not a decision about the call.
|
||||
4. **Tests.** `turn.rs` grew past 500 lines; the new tests moved to `turn_broker.rs`.
|
||||
5. **Verified claim.** "The registry change affects only new `find_tool` results; the tools array
|
||||
is unchanged" is true: `Baseline::assemble` takes only `registry.core_schemas()`, which is
|
||||
`clock`, `find_tool`, `call_tool` with identical schemas for `m2b()` and `m3a()`; resumed
|
||||
sessions use their snapshot. `the_m3a_registry_has_the_same_core…` asserts equality.
|
||||
6. **Plan, outside this fork: `make verify-device` breaks at task 16.** `device.rs` asks Ornith to
|
||||
"use the echo tool" through `loopd serve`, and asserts that `find_tool` and `call_tool` were
|
||||
called. With `m3a()` there is no `echo` to find. Task 17's files carry a changed `device.rs`
|
||||
(find a file-reading tool, call it on `/etc/hostname`, which fails with "no tool broker is
|
||||
configured"; the restart check asks for the first turn's words instead of the echoed word). It
|
||||
compiles and is clippy-clean. **It has not been run on straylight** (this fork was told not to
|
||||
contact it), and it depends on model behaviour, so the owner should run it once before the
|
||||
plan is handed over. Between tasks 16 and 17 `verify-device` is broken either way.
|
||||
|
||||
## Task 17: `BrokerPort`, `[broker]` and the pointers
|
||||
|
||||
**Check: skeleton** for `BrokerPort::call` (`todo!()`); everything else in the task is small
|
||||
enough that the skeleton is the code (`config::Broker`, `NoBroker`, the two line functions, the
|
||||
`main.rs` wiring, the three pointers).
|
||||
|
||||
Run to green: `config` (11, two new), `pointers` (7, new; three of them run the `loopd` binary),
|
||||
`broker_port::without_a_broker…` (1).
|
||||
**Compiled only, never run to green:** the other 14 tests of `broker_port` and both tests of
|
||||
`broker_port_bad`. They were desk-checked against the exit list in the task, not executed. The
|
||||
likeliest faults, if any: a timing bound too tight under load (`a_broker_that_never_answers…`
|
||||
allows 250 to 1,100 ms for a 300 ms timeout; `a_pending_call_nobody_answers…` allows 550 to
|
||||
1,700 ms), and a fake-broker thread that panics noisily, but harmlessly, when the port closes
|
||||
without sending.
|
||||
|
||||
Defects the check exposed:
|
||||
|
||||
1. **Spec, section 8, ambiguous.** "The read timeout is `timeout_ms` until the first frame" can be
|
||||
read as a per-read socket timeout. That lets a peer that trickles bytes hold a turn for ever,
|
||||
and it cannot express "until `expires` plus `timeout_ms`" at all. Resolved as a deadline per
|
||||
frame wait, with a test whose answer (not its timing) tells the two apart. Note this is a total
|
||||
deadline, unlike the inference path's liveness rule; the spec already implies it ("M3b must
|
||||
keep its tool time limit under `timeout_ms`"). Proposed wording: "`BrokerPort` waits for the
|
||||
first frame until `timeout_ms` after the call began, and after a pending frame until the
|
||||
frame's `expires` plus `timeout_ms`. These are deadlines, not per-read timeouts."
|
||||
2. **Spec, silent: a request too large for a frame.** `arguments` near 1 MiB makes `write_frame`
|
||||
fail with `TooLarge`. Calling that "the tool broker is unavailable" with a runbook pointer
|
||||
would send the owner looking for an outage. Resolved: its own failure text,
|
||||
"the request is too large for the tool broker", nothing printed.
|
||||
3. **Spec, silent: `expires` is a peer's number.** A far-future `expires` overflows
|
||||
`Instant + Duration`, which panics. Resolved: `checked_add`, and `None` is exit 10.
|
||||
4. **Spec, silent: the envelope `id`.** Resolved: `request.call.0`; every answer must carry it.
|
||||
5. **Spec, silent: `timeout_ms = 0`.** `set_write_timeout(Some(0))` is an error in std. Resolved:
|
||||
no config validation; the call fails closed at exit 3, and a test checks there is no panic.
|
||||
6. **Spec: `core-memory-unreadable` needs a new error variant.** `BaselineError::Read` serves both
|
||||
`system.md` and `core.md`; the pointer belongs to one. Added `BaselineError::Core`. The M2b
|
||||
test that checks the unreadable `core.md` only looks for "core.md", so it still passes.
|
||||
7. **Testability.** "`loopd` prints …" cannot be asserted in-process through `eprintln!`.
|
||||
Resolved with `BrokerPort::with_log` and the two `*_line()` functions; the startup and
|
||||
self-test lines are asserted by running the binary.
|
||||
8. **Task-writing.** The line-limit hit again: `broker_port.rs` was 546 lines. Split into
|
||||
`broker_port.rs`, `broker_port_bad.rs` and `support/broker.rs`.
|
||||
|
||||
## For the record of the experiment
|
||||
|
||||
What skeleton-only cost here: 16 of about 40 new or changed tests in this area were never run. What
|
||||
it saved: about 110 lines of `BrokerPort`. The five spec gaps under task 17 (1 to 5) were all found
|
||||
while writing the exit list and the tests, not by running anything, so a reference would not have
|
||||
found them sooner; what a reference would add is only confidence in the timing bounds.
|
||||
|
||||
## Run on straylight (2026-09-18, at the merge)
|
||||
|
||||
Task 17's changed `device.rs` was run with `make verify-device` on the merged reference tree
|
||||
(`f9195ec`: areas A to D, `loopd` at the task 17 state with no broker configured) against Ornith
|
||||
on straylight, slot 0, after checking `/slots` showed both slots idle. It passed twice in a row,
|
||||
6 of 6 each time (43 s and 39 s). What the test asserts is that Ornith called `find_tool` and
|
||||
then `call_tool`, that `clock`, `find_tool` and `call_tool` each left a result, and that the
|
||||
restart check passed; which tool it found and what the call returned are not asserted. Between
|
||||
tasks 16 and 17 `verify-device` is still broken, as noted above; the owner should not run it there.
|
||||
Reference in New Issue
Block a user