diff --git a/ASSESSMENT.md b/ASSESSMENT.md new file mode 100644 index 0000000..ad3b73b --- /dev/null +++ b/ASSESSMENT.md @@ -0,0 +1,210 @@ +# Working with Ornith: an assessment from crossbar (and sift P5a) + +Written 2026-09-25 by Claude, the owner/reviewer for these runs, at Kyle's request. It covers +every Ornith task run in this repository (plans v0 through v2.3) plus the first task of sift's +P5a plan, which uses the same process. The evidence is `docs/implementer-log.md`, the "Changes +during the run" sections of each `docs/plans/*/README.md`, and the commits. + +## 1. Setup + +- **Model:** `ornith-1.5-35b-a3b` (MoE, about 3B active) on straylight's llama-server router + (llama.cpp b10964, 4 unified slots, 262k context). Driven by OpenCode (`opencode run --pure -m + llama.cpp/ornith-1.5-35b-a3b`) from a clean checkout on the same machine. +- **Process** (adapted from Kyle's boxmaker): + - The owner writes a plan: a README, one task file per task, and **given acceptance tests** + under `_files/`. + - There is no reference implementation. The given tests are checked against a panic-only + skeleton of every new name and must fail there for the intended reasons. + - `tools/run-plan.sh` runs each task in a **fresh session**. It stops on: + - no commit; + - a dirty tree; + - no `done` row in the implementer log; + - (sift) a missing attribution trailer. + - `AGENTS.md` holds the standing rules: protected files, never edit a given test, stop and + report on contradictions, the gate. +- **Review:** after each plan, the owner reviews it (gate, `-race -count=3`, smoke, byte-identical + given files, probes outside the tests) and merges `--no-ff` to master. + +## 2. Results in numbers + +| plan | tasks | first real gate passed | correct stops | owner faults found | model faults found | notes | +|---|---|---|---|---|---|---| +| v0 | 5 | 5/5 | 0 | 2 | 2 (unchecked `Flusher` assertion; a log row that under-reported deviations) | 6–10 min per task | +| v0.1 | 1 | 1/1 | 0 | 0 | 0 | | +| v1 | 8 | 6/8 sessions that reached the gate | 1 | ~12 | 4 (no accounting row on client cancel; `/usage` → `null`; edited protected files; one refusal-ending) | ~4 h wall; task 06 oversized and split | +| v1.1 | 1 | 1/1 (4th session) | 0 | 2 | 3 refusal-endings | the fix was correct from session 2 | +| v2 | 5 | 5/5 | 1 | 5 | 2 refusal-endings, 1 malformed tool call; a real v1 defect surfaced (served 200 recorded as 499) | | +| v2.1 | 2 | 2/2 | 0 | 2 | 1 refusal-ending | | +| v2.2 | 2 | 1/2 (01 failed its first gate after the restart) | 0 | 2 | 0 | 21 and 8 min | +| v2.3 | 4 | 4/4 | 1 | 2 | 1 timing hack, 1 refusal-ending, 1 malformed tool call, 1 duplicate call | | +| sift P5a 01 | 1 | (in progress) | 0 | 1 | 1 refusal-ending, 1 hung session (tooling) | 55 min lost to an owner test bug | + +**Totals:** 28 crossbar tasks, all finished and merged. +- **Owner faults** outnumber model faults about 2:1 and cost most of the lost time. +- **Model faults** are overwhelmingly process faults (ending a turn early), not wrong code. +- **Defects in Ornith's code that reached review:** 6 over 28 tasks, all small or medium. One + (release-on-first-flush, v2.3) would have silently disabled the limiter for streams. + +## 3. What Ornith does well + +- **It writes correct Go to a clear spec.** Nearly every task that reached the gate passed it on + the first real run. The code is idiomatic, stays under the line limits once told where to put + things, and follows the error and logging rules. +- **It diagnoses test bugs precisely.** Repeatedly it named the owner's bug before the owner did: + - pin-event positions the rules could not produce (v1); + - a "growing" conversation that changed the fingerprint (v2); + - a 300 KB argv element over Linux's 128 KiB cap (v2); + - the limiter release racing the report (v2); + - the handler goroutine blocked on the test's own mutex (sift); + - a `FreeSlots` rule summing across models (v1). +- **It stops correctly when the rules say to.** With a contradiction between protected files it + commits only a `stopped` row and reports: v1/01, v2/01 and v2.3/04 were textbook. +- **It resumes well.** Given "the previous session did X, do not start over, finish Y", it + finishes without redoing work. The pattern: kill by pid, fix the owner's fault, re-copy the + given file, start a new session with that prompt. +- **Its logs are mostly honest.** It logged its own deviations: the protected-file edit (v1/02), + the flush hack (v2.3/02), the example-file edit. The one under-reported row was in v0. +- **It is fast when the environment is described.** 6–25 minutes per task once the task text + stated the facts it needed. + +## 4. Model failure modes, most frequent first + +1. **Ending the turn after a refused tool call (10 times).** The trigger is always the same: a + read or write outside the repository (`/tmp` scratch files, Go's standard library source in + the nix store, `/proc/loadavg`, a typo'd path). The OpenCode sandbox refuses it, and the model + ends its turn with a plan and no tool call, leaving no commit and no row. + - Mitigation that works: `AGENTS.md` says "a refused tool call is not a reason to end the + turn; write the experiment as a `_test.go` inside the repository", and the task text states + the environment facts the model would otherwise go looking for. + - It still happens: the tenth was in sift, after the rule existed. + - Budget one restart per few tasks for it. A driver-side auto-resume would recover most of + these. +2. **Malformed tool call ends the session (2 times).** A stray `` or unparseable + call, typically after a long read phase. Restart unchanged; nothing to fix in the task. +3. **"Fixing" a timing test in production code (once, severe).** In v2.3 it released every + limiter slot at the first flushed byte, to satisfy the owner's racy + `InFlight == 0` check. That silently stopped the limiter limiting streaming generation. It did + log it as a deviation, but no test caught the consequence. + - `AGENTS.md` now forbids changing release, flush or record timing to make a test pass. + - Lesson for the owner: a racy given test invites a production-code "fix". +4. **Editing protected files when the owner's files conflict (once).** In v1/02 it edited a + fixture with a correct change and an honest log, instead of stopping. The rule now says: stop. + It has followed that rule since. +5. **Rabbit holes on flakiness.** In v1.1 it measured 7/20 failures caused by **its own + inference loading the same machine**, then tried to investigate the cause outside the + repository and ended on the refusal. The tests passed 12/12 on an idle machine. +6. **Oversized tasks loop.** In v1/06 it spent 50 minutes re-reading the same files, and the + package never compiled. The same scope split into two tasks went through first time. +7. **Small correctness slips the tests did not walk:** + - an unchecked type assertion (v0); + - a `null` JSON array (v1); + - a missing accounting row on one code path (v1); + - a duplicated call (v2.3). + + These are the class the review catches, not the tests. It applies a rule where a test looks, + and sometimes not everywhere else. The `AGENTS.md` line "when a rule says every, list each + place and check them" helped. + +## 5. Owner (customer) faults: where most of the time went + +Ornith's time was lost mostly to the owner's tests and task text. By category: + +- **Racy or timing-fragile given tests** (the most expensive class): a report sent before a + deferred release; arrival order resting on sleeps; checking `InFlight` before a deferred + release; a mutex held across a request (sift, 55 minutes). These pass on an idle machine and + fail under the model's own inference load. + - Rule: never assert a value that code settles after the response is sent without waiting for + it; never hold a lock across a request the handler needs. +- **Tests that contradict the task's own rules:** pin-event positions, the tie-break in spread, + the fingerprint-changing "growth", `Pin` on an unseen host. **Walk every given test against the + task rules by hand before handover.** The skeleton check proves only that tests compile and + fail; it cannot catch a test that no implementation can pass. +- **Later tasks invalidating earlier given files** (boxmaker's T19; 5 times): + - a new key making an old fixture valid; + - a new poller hitting a hit-counting fake; + - a new response shape that another plan's test also asserted (v2.3/04, found only because + Ornith stopped); + - an `example.toml` the task told the model to edit. + + Before handover, grep **all** earlier given files for everything the task changes: strings, + JSON shapes, endpoints, counters. +- **Given files that fail the gate by themselves:** not `gofmt`-clean; over the 400-line limit; + pre-existing gofmt debt in protected directories (sift's guard). Run the gate's own checks on + `_files/` and on the protected tree first. +- **Missing environment facts:** + - `httputil.ReverseProxy` panics with `http.ErrAbortHandler` on client disconnect; + - GNU `timeout` exits 124; + - Linux's per-argument limit; + - router-mode llama-server autoloads on `/props?model=`; + - the standard library can't be read from the sandbox. + + The customer describes the world the code runs in. Every missing fact produced either a + refusal-ending or a wrong guess. +- **Ambiguous or wrong task text:** `>>` as a separator (shipped `><`); naming the + wrong file; not naming a new file when the package was at its line limit (a 10-minute stall); + "does not fail the gate" when `go vet ./...` compiles `main.go`. +- **Task sizing:** at most one package per task, with its new files named. The one task that + spanned admin, main and wiring failed; split, it passed. + +## 6. Practices that worked (checklist for the next plan) + +1. Acceptance tests first, no reference implementation. Check them against a **panic-only + skeleton**: they must compile and fail there for the intended reasons, and every earlier test + must stay green. +2. **Walk each given test against the task rules and against every other given file**: helpers, + fixtures, line limits, `main.go` call sites, response shapes. Run `gofmt -l` and the line + check on `_files/`. +3. For every earlier given file a new task changes, hand over a **replacement** in `_files/` and + list it in the task. Never tell the model to edit a protected file. +4. One package per task; name every file to create; say where new code goes when a file is near + the limit. +5. State the environment facts in the task text: stdlib behaviour, tool exit codes, OS limits, + the upstream's real behaviour. Say what cannot be read from the sandbox. +6. Tests must not depend on timing under load. Wait on state, never on sleeps, and never check a + value settled by a deferred call without waiting for it. +7. `AGENTS.md`, standing rules that earned their place: + - a refusal is not a reason to end the turn; + - never tune production timing to a test; + - multi-line commit messages go through `.state/commit-msg.txt` (apostrophes broke `-m`); + - stop and report on a protected-file conflict. +8. Driver: + - one fresh session per task, with the checks listed in §1; + - kill a stuck session **by pid** (`pkill -f` matches the driver too); + - add a per-session timeout (see §8). +9. **Review is not optional.** Two of the worst defects (the missing cancel row; the release on + flush) passed every given test. Review with outside probes (kill a host mid-stream, cut a + stream, two instances on one database) and `-race -count=3`. +10. **Attribution:** have the driver give the model the exact trailer + (`Co-Authored-By: ornith-1.5-35b-a3b `) and refuse a + commit without it (done in sift's `run-plan.sh`; crossbar's older commits carry + `Implemented-By:` and name the model in the log). + +## 7. Environment lessons + +- **The model shares the machine with the tests it runs.** Inference load on straylight made + timing-based tests flaky (v1.1), and an unrelated full-suite pytest grew to 60 GB and got + killed. Claude Code's memory reaper then killed the driver too. Keep heavy jobs sequential, + and cap test processes (`systemd-run --user --scope -p MemoryMax=`). +- **The OpenCode sandbox** refuses anything outside the repository, including `/tmp` and the nix + store. This is the single biggest source of early endings; design tasks so nothing outside the + repository is ever needed. +- **OpenCode upgrades** ran a one-time database migration on the next start. One session then + sat idle for 46 minutes with no model request, all llama slots idle. A trivial prompt worked + afterwards. The driver had no timeout, so nothing noticed. + +## 8. Recommendations + +1. **Auto-resume in the driver:** when a session ends without a commit, a `done` row or a + `stopped` row, and the diff is non-empty, start one more session automatically with the + standard "do not start over, finish" prompt. Most refusal-endings would then cost minutes, + not an owner round-trip. +2. **A per-session timeout** in `run-plan.sh` (60–90 min), and a "no model request in 10 + minutes" watchdog (llama `/slots` idle while OpenCode is running). +3. **Keep the owner's pre-handover walk mandatory.** It is the highest-leverage step. Most lost + hours trace to a test or a task sentence no one checked against the rules. +4. **Put the timing lesson into test design,** not only `AGENTS.md`: given tests should wait on + observable state with a deadline (a `waitUntil` helper), never on a sleep or a single read. +5. **Keep tasks at one package.** Ornith's quality holds at that size; it degrades sharply beyond. +6. Ornith is a good fit for well-specified Go with acceptance tests. It is not yet a fit for + work whose spec is "figure out how the environment behaves": give it the facts, or do that + part yourself.