From b39dac3f71914c28ced0e3513cf620d039e91414 Mon Sep 17 00:00:00 2001 From: "K. Isom" Date: Fri, 18 Sep 2026 23:00:21 -0700 Subject: [PATCH] Hand off the half-built M3a plan Areas A to D (audit, policy side, loopd, bxctl) are written on branches in ~/src/boxmaker-ref-a to -d; area E (broker and approval handoff) is not started. HANDOFF.md records each area's result, the contract conflicts between them, the spec defects they found and the remaining steps in order. Co-Authored-By: Claude Fable 5.1 --- docs/plans/M3a/HANDOFF.md | 292 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 292 insertions(+) create mode 100644 docs/plans/M3a/HANDOFF.md diff --git a/docs/plans/M3a/HANDOFF.md b/docs/plans/M3a/HANDOFF.md new file mode 100644 index 0000000..3c6f680 --- /dev/null +++ b/docs/plans/M3a/HANDOFF.md @@ -0,0 +1,292 @@ +# M3a plan: handoff, 2026-09-18 + +The M3a plan is half built. This file says what exists, where, and what is left. Delete it when +the plan is complete and `README.md` replaces it. + +Read first: `CLAUDE.md`, `docs/specs/2026-09-18-m3a-decision-path.md` (revised after review; the +"Spec review of M3a" rows in `docs/decisions.md` say what changed and why), the decision of +2026-09-18 on reference implementations (top row of `docs/decisions.md`, tip T17 in +`docs/implementer-lessons.md`), and `docs/plans/M2b/` as the model for format +(`README.md`, `07-loopd-channel.md`). + +## How the plan is being built + +The given tests are checked before hand-over at one of three levels: a full reference +implementation (audit chain, approval handoff), the oracle inside the property test (policy), or +a skeleton (real signatures, `todo!()` bodies, tests must compile and be clippy-clean). Each area +records what its check exposed in `docs/plans/M3a/checks-.md`; the final `README.md` folds +those into one table per task. That record decides what M3b gets. + +Work happens in git worktrees on unpushed branches, never in `/tmp`: + +| Worktree | Branch | Holds | +|---|---|---| +| `~/src/boxmaker-ref` | `m3a-ref` | Integration. Commit `3ba5f83`: the `proto` contract (new audit types, admin wire messages, `chain.rs` skeleton) and an empty file for every new module, so that areas never share a file. | +| `~/src/boxmaker-ref-a` | `m3a-ref-a` | Area A, audit: tasks 01, 03, 09, 19 | +| `~/src/boxmaker-ref-b` | `m3a-ref-b` | Area B, policy side: tasks 02, 04 to 08 | +| `~/src/boxmaker-ref-c` | `m3a-ref-c` | Area C, `loopd`: tasks 16, 17 | +| `~/src/boxmaker-ref-d` | `m3a-ref-d` | Area D, `bxctl` and the runbook script: tasks 18, 20, 21 | + +Each area branch holds, under its worktree: the tests in place under `crates/`, the reference or +skeleton source, copies of everything the implementer must copy under +`docs/plans/M3a/files/`, its task files `docs/plans/M3a/NN-name.md`, and +`docs/plans/M3a/checks-.md`. + +## Task numbering (fixed; task files use these names) + +| # | Task | Area | Check | +|---|---|---|---| +| 01 | `proto-audit-types` | A | reference (types; generates fixtures) | +| 02 | `proto-admin-wire` | B | reference (types) | +| 03 | `proto-chain-verifier` | A | reference | +| 04 | `brokerd-config` | B | minimal reference (needed under E) | +| 05 | `brokerd-args` | B | minimal reference | +| 06 | `brokerd-grants` | B | minimal reference | +| 07 | `brokerd-policy` | B | oracle, plus minimal reference | +| 08 | `brokerd-state` | B | minimal reference | +| 09 | `brokerd-audit-writer` | A | reference | +| 10 | `brokerd-runner` | E | reference | +| 11 | `brokerd-approvals` | E | reference | +| 12 | `brokerd-ledger` | E | reference | +| 13 | `brokerd-broker` | E | reference | +| 14 | `brokerd-admin` | E | reference | +| 15 | `brokerd-serve` | E | reference | +| 16 | `loopd-tools` | C | skeleton | +| 17 | `loopd-broker-port` | C | skeleton | +| 18 | `bxctl-admin` | D | skeleton | +| 19 | `bxctl-audit-verify` | A | reference | +| 20 | `bxctl-chat-approvals` | D | skeleton | +| 21 | `runbook-check` | D | run for real | +| 22 | `end-to-end` | E | compiles only, unless C's `BrokerPort` gets a body | + +## Area E: not started + +Tasks 10 to 15 and 22: `runner`, `approvals`, `ledger`, `broker`, `admin`, `serve`, the +two-process end-to-end test, and the scripted check on straylight. It builds on A's +`brokerd::audit::Writer` and B's `config`, `args`, `grants`, `policy`, `state`, so it starts from +a merge of those two branches. The design it must follow, beyond the spec: + +- `ledger.rs`: `pub trait AuditSink: Send { fn append(&mut self, time: Timestamp, event: AuditEvent) -> Result; }` + implemented by `audit::Writer`, so tests can wrap a real writer in one that fails on demand. + `Ledger { audit: Box, state: StateStore }` behind one `Mutex`, with the three + locked steps as functions: decide and record; re-decide and record the approval; raise state + and record the result. A poisoned ledger lock or any failed append means `audit_unavailable` + for every later call. +- `approvals.rs`: `Verdict { Run(Decision), Denied(DenyReason) }`; `Table` over + `Mutex>` with `insert(info, ask) -> Receiver`, `take(id)`, + `take_expired(now)`, `list()`. Whoever takes the entry answers it. +- `broker.rs`: `pub struct Broker { cfg, ledger, table, runtime }`, `handle(stream, &Broker)`. + While pending: `recv_timeout(1 s)`, then a `read` with a 10 ms read timeout (`Ok(0)` is gone, + `WouldBlock`/`TimedOut` is alive, bytes are a protocol error). One more check before running. +- `runner.rs`: spec section 7, with `Refusing` as the production runtime and a recording fake in + the tests. `RunError` text is fixed text. +- `serve.rs`: the audit lock first, then directories (0700 whether made or found), stale socket + removal, bind, 0600; one thread per connection; the expiry thread. +- The state-write failure test uses a read-only directory; the audit failure test uses the + `AuditSink` seam. +- The end-to-end test lives in `loopd`, is `#[ignore]`d without `BOXMAKER_BROKERD`, and + `make gate` builds the workspace and runs it with the variable set. +- The straylight script checks `GET /slots?model=ornith-1.5-35b-a3b` and uses slot 0 only. + +The prompts given to areas A to D are the model for E's: they are in this session's transcript; +their substance is the numbered conventions below. + +## Conventions every area follows + +1. Tests carry "Do not edit." Deterministic, robust under CPU load, no `tempfile` crate (temp + directories under `std::env::temp_dir()` with pid and a counter, as `crates/loopd/tests/support/`). + `cargo fmt --all`; `cargo clippy --workspace --all-targets --offline -- -D warnings` clean. +2. A reference follows the code rules in `AGENTS.md`. +3. Everything the implementer copies is also under `docs/plans/M3a/files/`. Reference source is not. +4. Task files follow `docs/plans/M2b/07-loopd-channel.md`: branch `m3a`, at most about 3,000 + tokens, rules that walk every exit (T14, I11, I12), verified API snippets, expected test counts. +5. A `checks-.md` per area, honest about what ran green and what only compiled. + +## Left to do, in order + +1. Read each area's final report (below) and its `checks-.md`. Resolve contract changes they + asked for. `crates/proto/tests/strict.rs` may have been edited by both A and B. +2. Merge `m3a-ref-a` and `m3a-ref-b` into `m3a-ref`; run the whole gate there. +3. Build area E on top. Merge C and D. Add the `audit verify` arm to `bxctl`'s `main.rs` (D owns + the file, A specified the arm). Add the end-to-end line to the `Makefile` in `files/`. +4. In `m3a-ref`: `make gate` as far as skeletons allow; every test compiles; reference-backed + suites pass ten times under load. +5. Copy `docs/plans/M3a/` (task files and `files/`) from `m3a-ref` to `master`. Write + `docs/plans/M3a/README.md` in the M2b README's shape, with the per-task check table and what + each check exposed. Fold any spec defects the areas found into the spec and `decisions.md`. +6. Update `CLAUDE.md`'s "Current state" and `docs/milestones.md` if needed. Delete this file. +7. Then the owner runs `tools/run-plan.sh docs/plans/M3a` with Ornith, on branch `m3a`. + +Already known spec defects found while planning (both fixed in the spec, commit `66fa143`): the +one-process end-to-end test would have broken the crate-dependency gate; the locked steps needed a +module of their own (`ledger`). One finding about the process itself: the approval-handoff +reference cannot run without working `config`, `args`, `grants`, `policy` and `state` under it, so +those got a minimal reference too, and the saving from skeletons is confined to `loopd`, `bxctl` +and the gate script. + +## Area reports + +(Each area's own record is `docs/plans/M3a/checks-.md` on its branch.) + +### Area C, `loopd`: done, commit `56319ab` on `m3a-ref-c` + +- Tasks 16 and 17 written. Task 16's changes are real code and its tests ran green (`tools` 10, + `turn` 6, `turn_broker` 5, `config` 11, `pointers` 7, rest of the suite unchanged). + `broker_port.rs` is a skeleton: only `BrokerPort::call` is `todo!()`, so 14 of 15 tests in + `broker_port` and both in `broker_port_bad` compile and have never run. +- API settled: `ToolPort::call(&self, &ToolRequest, &mut dyn FnMut(&Pending)) -> ToolResponse`; + `Pending { approval: u64, expires: Timestamp }`; `Registry::m3a()` (and `m2b()` kept as the + test registry); `denial_text`; `BrokerPort::new(socket, timeout)` and `with_log`; `NoBroker`; + `config::Broker { socket: Option, timeout_ms }`; the envelope `id` is `request.call.0` + (area E's `brokerd` must echo it). +- Spec defects it found, with wording in `checks-c.md`, NOT yet applied to the spec: the section 8 + timeout must be a deadline per frame wait, not a per-read socket timeout; `tool_denied.name` and + `approval_pending.tool` carry the target tool, not `call_tool` (task 20 must know); "`echo` stays + in `FakeTools` only" is not enough, because recorded conversations find it through the registry; + five silent cases resolved and tested (oversized request, far-future `expires`, envelope id, + `timeout_ms = 0`, a pending frame as the final answer). +- **Needs a run on straylight before hand-over:** `make verify-device` breaks at task 16 because + `device.rs` asks Ornith to use `echo`. Task 17 ships a changed `device.rs` that compiles but has + never run on the device and depends on how the model behaves. + +### Area A, audit: done, commits `e3065e1`, `89f696e`, `7da817d` on `m3a-ref-a` + +- Tasks 01, 03, 09, 19 written, all with a reference. Passing against it: `records` 3; + `chain` 13 with 30 fixture logs under `crates/proto/tests/fixtures/audit/`; `brokerd` `audit` 9 + and `audit_startup` 7 (five runs); `bxctl` `verify` 6. Eight deliberate mutations of the + reference each broke a given test. 66 files under `files/`, byte-identical to the tree. The + fixture generator `crates/proto/tests/gen_audit_fixtures.rs` (`#[ignore]`d) stays in the + reference tree only. +- In that worktree one `strict.rs` test fails and `proto`'s `wire.rs` test does not compile: both + wait on area B's wire fixtures and should clear at the merge. +- **Contract changes other areas must absorb:** + 1. `DecisionRecord::Allowed {}` and `Ask {}` are empty struct variants, not unit variants: serde + ignores `deny_unknown_fields` on unit variants of an internally tagged enum. JSON unchanged; + every construction and match needs the braces (areas B, D, E). Check the other fieldless + variants in internally tagged enums in `wire.rs` for the same hole. + 2. `strict.rs` changed in the audit test only (expected object count); B edits the envelope list. + 3. `ChainFailure` gained `last_good`, `break_prev`, `break_seq`, `tail_torn`; + `ChainVerifier::feed(name, content)` added and used by both `bxctl` and `brokerd`. + 4. Writer API as specified, plus `RECOVERED_NOTICE` and `AuditError::Broken(Box)`. + 5. Tasks 09 and 19 say "Create" the module file and add the `pub mod` line (the stub files exist + only in the reference tree). Every area's task files need the same check. + 6. Task 09 assumes task 04 adds `serde_json` to `brokerd`'s `Cargo.toml`. Check task 04 does. +- **Spec defects it found, wording in its report and `checks-a.md`, NOT yet applied to the spec:** + the resumed verifier's break exception must also hold inside a failed region; startup step 2 + should fall back to verifying the whole log when the previous file's last line does not parse + (otherwise an accepted break there deadlocks); the tampering suite fails at `brokerd`'s startup + only when the damaged file is the latest; an unpaired `Recovery` is a failure; `unfinished` + reports the decision's `seq` for approved calls too; `--accept-break` with a torn tail and no + failure writes nothing and exits 2; section 9's verify output lines are fixed in task 19, + including one for a torn tail. +- Left open: task 19 describes the `audit verify` arm in prose; reconcile with D's `main.rs`. + +### Area D, `bxctl` and the runbook script: done, commits `292780f`, `5f27808`, `39f3cc7` on `m3a-ref-d` + +- Tasks 18, 20, 21 written. Tests under `crates/bxctl/tests/`: `escape` 8, `cli` 12, `admin` 21, + `chat_print` 9, `chat_approvals` 20, existing `chat` 12 untouched. `check-runbook.sh` runs for + real; its self-test passes and fails under each of five mutations. +- At the skeleton state (`5f27808`) 38 tests only compiled. `cli.rs`, the `Printer` changes, + `stream_turn` and `main.rs` had to be real code, because existing tests drive the binary. The + area then wrote the remaining bodies (about 150 lines, `admin.rs` and `handle_pending`) as a + measurement: all 38 passed first time, six more runs under load clean. **For this area the + reference found nothing the skeleton and a desk-check had missed.** The bodies stay on the + branch (the straylight check needs a working `bxctl approve`) and are not handed over. +- API settled: `escape::{escape_json_text, escape_model_text}`; `cli::{USAGE, ChatOptions, Command, + UsageError, parse(args, home)}` (a new module: `main.rs` was near the line limit); + `admin::{AdminError, reason_name, request, list, write_block, cmd_approvals, cmd_approve, + cmd_refuse, cmd_grants_check}`, each `cmd_*` returning `Result`; + `chat::{OnPending, Approvals, TurnIo, handle_pending, stream_turn}`; + `verify::run(home, out) -> io::Result`. +- **To act on when integrating:** + 1. `check-runbook.sh` fails on any pointer under `crates/**/*.rs` whose anchor is not written + out literally (`format!("…#{anchor}")`, a `#` placeholder in a comment), and if it + finds no pointer at all. Run it on the merged tree; areas A, B, C, E may trip it. Proposed + for spec section 11: "An anchor must be written out in the source; a pointer whose anchor the + script cannot read is an error." + 2. **Conflict with area A:** task 18 creates `verify.rs` with a placeholder body and adds all + four `pub mod` lines to `lib.rs`, so task 19 must MODIFY `verify.rs` and touch neither + `lib.rs` nor `main.rs`. Area A wrote task 19 as "Create". Fix task 19. + 3. `GrantProblem.problem` can be multi-line because `toml` errors are. Task 06 should produce + one-line problems at the source; check area B did. + 4. Spec section 9's example block shows spaced JSON; section 6 says `serde_json` output, which + is compact. The tests use compact. Fix the example. + 5. "`--say` and `--json` print the event only" was ambiguous. Chosen: `--say` shows the block + from `brokerd` and asks nothing; `--json` prints the event's JSON line and never contacts + `brokerd`. Put that in the spec. + 6. Escaping now covers tool names and the answer on stdout too. Spec section 9 should say + "everything the model wrote". + 7. `main.rs` must hold one `BufReader` on stdin for chat lines and approval answers alike; a + binary-level test enforces it. + 8. Details the tests now fix that the spec leaves open: time spans and rounding; `expired` once + `now >= expires`; a session id over 10 characters shown as 9 plus `…`; `no pending approvals`; + an `ask` re-decision printed as `runs`; an approval id is ASCII digits only + (`"+41".parse::()` succeeds); end of input at the prompt refuses; `brokerd` unreachable + or an approval expired before the answer is one line and the turn goes on. + 9. `tests/admin.rs` is 497 lines and `tests/chat_approvals.rs` 487: no room to grow. + 10. `DecisionRecord::Allowed {}` / `Ask {}` (area A's contract change) must be applied to this + branch's code and tests at the merge. +- The `Makefile` under `files/` has the `check-runbook.sh` line and no end-to-end line yet. + +### Area B, policy side: done, commit `e89e2ce` on `m3a-ref-b` + +- Tasks 02, 04 to 08 written, all with a minimal reference. Passing: proto `wire` 10, `turn_wire` 5, + `admin_wire` 10 (new), 17 wire fixtures; `brokerd` 81 tests (`config` 7, `args` 13, `grants` 17, + `policy` 7, `policy_matching` 10, `policy_redecide` 7, `policy_property` 4, `state` 9, doctests + 7), three runs and one under load. Support files are pulled in with `#[path]` + (`tests/support/{tmp,build,oracle}.rs`), so there is no `support/mod.rs` for area E to collide + with (area A added `tests/support/audit_dir.rs`; check how it is included). +- **Oracle teeth:** six mutants of the reference policy were each caught by the oracle alone. + **The policy reference caught nothing the oracle would not have.** Second data point for the + decision on references. +- API for area E, in full in its report and `checks-b.md`: `Config` with `broker_socket()`, + `admin_socket()`, `audit_dir()`, `state_dir()`; `args::{ToolName, ToolArgs, ArgsError, parse, + valid_path, inside, valid_host, valid_host_pattern, host_matches, url_host}` and + `ToolArgs::canonical_json()`; `grants::{LoadedGrant, GrantSet, from_grants, load, render, + valid_id, RUNBOOK}`; `policy::{SessionState, Label, Denial, Decision, Ask, Outcome, decide, + redecide -> Result}` (`SessionState` and `Label` live in `policy`); + `state::{StateStore, StateError, RUNBOOK}` with `read` and `raise`. +- **CONFLICT with area A, to resolve first:** both found that `DecisionRecord` accepted unknown + fields and fixed it differently. A made `Allowed {}` and `Ask {}` empty struct variants (call + sites need braces). B kept unit variants and decodes through a private `RawDecision` with + `#[serde(try_from = "RawDecision")]` (public type and JSON unchanged, no call-site change; + `{"outcome":"allowed","reason":null}` is still accepted). Task 02 greps for `RawDecision` and + stops without it. Pick one. B's keeps the public type as the spec wrote it and touches no other + area; A's is stricter about `reason: null`. Whichever wins goes into task 01, and the loser's + tests and task text change. Then check the other internally tagged enums (`TurnEvent`, + `ToolResponse`, `AuditEvent`, `LogRecord`) for unit variants with the same hole. +- `strict.rs` is edited by tasks 01 and 02. The copy handed over with task 02 must carry A's audit + object-count change as well as B's. +- Area E's tests must include: one invalid grant file denying a call a valid file would allow + (`grants` only proves the set does not load), and section 12's sequence properties (taint never + down; every `Result` follows a `Decision`). +- **Spec defects it found, NOT yet applied to the spec:** + 1. IPv4 literals pass the host grammar (`127.0.0.1`, `127.1`, `10.0.0.0x1`), so "no IP literals" + was false. Rule added and tested; proposed wording: "...and the last label starts with a + letter, which excludes every spelling of an IPv4 address." + 2. `write_file` with overlapping grant paths `/s` and `/s/out` and a write to `/s/out`: a grant + path equal to the argument does not count, and the longest path that does count is matched. + 3. `redecide` returns `Result`, not `DenyReason` (the spec's signature is wrong). + 4. Gaps settled in the tasks: an empty socket path means "under home"; a missing grants directory + is a problem, not an empty set; a state file that says `public` is damaged; an absent `cwd` is + left out of the canonical JSON. +- Other: serde's derived structs accept the JSON array form, so `"body":[]` decodes as `Empty`; + true across `proto` since M1, not fixed, worth a decision. `docs/dependencies.md`'s "Used by" + column is stale; task 04 has the implementer fix the `serde` rows. The owner's global gitignore + swallows `*.toml~`, so that fixture is made in a temp dir. + +## Tool warning for whoever continues + +The Write and Edit tools in this environment decode a JSON escape such as backslash-u-002f into +the character itself when it appears in the text being written. Build such strings from pieces or +patch them in with a script, and check the bytes on disk afterwards. + +## What the checks have shown so far (for the README and for M3b) + +| Area | Check | Exposed | +|---|---|---| +| A, audit | reference | Five spec defects in section 5, two of which would have stopped `brokerd` starting; the serde unit-variant hole. The reference earned its cost. | +| B, policy | oracle, plus a minimal reference | One spec falsehood (IPv4), one unspecified case, one wrong signature, the same serde hole. The reference added nothing over the oracle. | +| C, `loopd` | skeleton | A timeout that had to be a deadline, five silent cases, a broken `verify-device`. 16 tests never run. | +| D, `bxctl` | skeleton, then bodies as a measurement | Spec ambiguities and an example that contradicted the text. The bodies found nothing the skeleton had not. | +| Planning itself | none | The one-process end-to-end test against the crate gate; the missing `ledger` module. |