v0.1 plan: review fixes as failing acceptance tests first; AGENTS.md lessons + deviations rule

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

AGENTS.md

@@ -41,6 +41,17 @@ implementing it one task at a time.
   compile against them.
 - Comments say why, not what. `gofmt` decides layout; run it before the gate.
 
+## Lessons from earlier reviews
+
+These come from defects found in review; the evidence is in `docs/implementer-log.md`.
+
+- A type assertion on a value that came from outside your package (`w.(http.Flusher)`, a decoded
+  JSON field) uses the two-value form and handles the `false` case. An unchecked assertion is a
+  panic waiting for a caller you did not think of.
+- When a rule says "every" or "everywhere", finish by listing each place it applies and checking
+  them one by one. The task shows one place; the rule covers all of them.
+- Never end a turn by describing what you are about to do. Do it, then report.
+
 ## The gate
 
 `make gate` must print `gate: ok` before a task is done. It runs offline: `gofmt -l`, `go vet`,
@@ -67,6 +78,6 @@ the commit. Be honest: the log is how the owner judges the process.
 | Status | `done` or `stopped` |
 | Gate runs | How many times you ran `make gate` |
 | First gate | `pass` or `fail` for the first run |
-| Deviations | Anything you did that the task did not say, or `none` |
+| Deviations | Anything you did that the task did not say, or `none`. If your Notes describe a change you made, it belongs here as well — a row that says `none` next to Notes that describe a change is wrong. |
 | Notes | Problems you hit and how you solved them, in one or two sentences |
 | Model | Write `?`. The owner fills this in. |

docs/plans/v0.1/01-review-fixes.md

@@ -0,0 +1,79 @@
+# v0.1 task 01: review fixes — a writer without `Flush`, an unreadable config file
+
+**Branch:** `v0.1` (create it from `master`: `git switch master && git switch -c v0.1`; `git status --short` must be empty first, otherwise stop)
+**Commit subject:** `Review fixes: recorder Flush without panic; unreadable config file is an error`
+
+## What the reviewer observed
+
+1. `internal/proxy/proxy.go`, `statusRecorder.Flush`:
+   ```go
+   func (r *statusRecorder) Flush() {
+       r.ResponseWriter.(http.Flusher).Flush()
+   }
+   ```
+   The assertion is unchecked. Every `http.ResponseWriter` the standard server hands out is a
+   Flusher, but wrappers written by middleware or tests often are not, and then a streamed
+   response **panics inside the reverse proxy** instead of falling back to buffering. The rule
+   "library code never panics on input" applies to every type assertion, including this one.
+   The task text said "forwarding to the underlying `http.Flusher`" and did not say "if it
+   implements it" — that half is the task's fault; the panic is still a defect.
+2. The given `config_test.go` never covered a file that exists but cannot be read. `Load` already
+   handles it (an `open` error wrapped as `config: …`); the suite just did not say so.
+
+## Files
+
+- Copy (never edit afterwards): `internal/proxy/recorder_test.go`,
+  `internal/config/unreadable_test.go`
+- Modify: `internal/proxy/proxy.go`, `docs/implementer-log.md`
+
+## Rules
+
+1. `statusRecorder.Flush` becomes: assert with the two-value form, and call `Flush` only when it
+   is there. Nothing else in the recorder changes.
+   ```go
+   if f, ok := r.ResponseWriter.(http.Flusher); ok {
+       f.Flush()
+   }
+   ```
+2. Then **list every other type assertion in `internal/`** (`grep -n '\.(' internal/*/*.go`) and
+   check each is either the two-value form or on a value you constructed yourself. Put the list,
+   with one word per line saying why it is safe, in your log row's Notes. If you find another
+   unchecked assertion on a value that came from outside the package, fix it the same way and
+   say so in Deviations.
+3. No change to `internal/config`: the two new tests must pass against the code as it is. If one
+   does not, stop and report — that is a finding about `Load`, not something to patch around.
+
+## Steps
+
+- [ ] **1. Branch and copy.**
+
+```sh
+git switch master && git switch -c v0.1
+cp docs/plans/v0.1/_files/internal/proxy/recorder_test.go internal/proxy/
+cp docs/plans/v0.1/_files/internal/config/unreadable_test.go internal/config/
+```
+
+- [ ] **2. See the recorder test fail.** `go test -run WithoutFlusher ./internal/proxy/`.
+  Expected: `FAIL`, with `ServeHTTP panicked on a writer without Flush` (or a panic trace naming
+  `statusRecorder.Flush`). If it passes already, stop and report.
+- [ ] **3. See the config tests pass as they are.** `go test -run 'Unreadable|Directory' ./internal/config/`.
+  Expected: `ok`.
+- [ ] **4. Fix `Flush`** as in rule 1, then do the assertion inventory of rule 2. `gofmt -w internal/proxy/`.
+- [ ] **5. See everything pass.** `go test -race -count=1 ./...`. Expected: all `ok`.
+- [ ] **6. Run the gate.** `make gate`. Expected last line: `gate: ok`.
+- [ ] **7. Log and commit.** Row `v0.1/01-review-fixes`. Anything you changed that these rules
+  did not name goes in **Deviations**, not only in Notes.
+
+```sh
+git add internal/proxy internal/config/unreadable_test.go docs/implementer-log.md
+git commit
+```
+
+## Done when
+
+- Step 2 failed before the fix and `go test -race -count=1 ./...` passes after it; `make gate` prints `gate: ok`.
+- `cmp` of both copied tests against `docs/plans/v0.1/_files/…` prints nothing.
+
+## Stop and report if
+
+- Step 2 passes before any change, or step 3 fails: the plan's premise is wrong, and the owner needs to know before code moves.

docs/plans/v0.1/README.md

@@ -0,0 +1,28 @@
+# v0.1 implementation plan: review follow-ups
+
+> **For the implementing model:** do not work from this file. The owner gives you one task file at
+> a time. This file is the index for the owner and the reviewer.
+
+**Goal:** close the findings of the v0 review (`docs/implementer-log.md`, "v0 review"): the
+proxy's status recorder must never panic on a writer without `Flush`, and the acceptance suite
+must cover a configuration file that exists but cannot be read.
+
+**How this plan was made:** acceptance tests first, from the review findings and `PLAN.md`; no
+reference implementation. Both given tests were compiled and run against `master` at the merge of
+`v0`: the recorder test **fails** there (it panics, which is the finding), the config tests pass
+(finding 6 was a gap in the suite, not in the code).
+
+## Tasks
+
+| # | File | Delivers | Tests that define it |
+|---|---|---|---|
+| 01 | `01-review-fixes.md` | `Flush` that degrades instead of panicking; the two config tests | `internal/proxy/recorder_test.go`, `internal/config/unreadable_test.go` |
+
+Branch `v0.1`. One task, one fresh OpenCode session, one commit.
+
+## For the reviewer
+
+1. `git log --oneline master..v0.1`: one commit with the trailer.
+2. `cmp` both copied tests against `_files/`; `git diff master..v0.1 --stat -- PLAN.md AGENTS.md docs/plans` empty.
+3. `make gate`, `make smoke`.
+4. `grep -rn '\.(http\.' internal/` — every type assertion on a writer is checked (`v, ok :=`).

docs/plans/v0.1/_files/internal/config/unreadable_test.go

@@ -0,0 +1,47 @@
+package config_test
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"git.wntrmute.dev/kyle/crossbar/internal/config"
+)
+
+// A file that exists but cannot be read is an error, and not a validation error: nothing about
+// the configuration has been judged. Only a missing file is "absent" (and that is an error too).
+func TestUnreadableFileIsAnError(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root can read a 000 file")
+	}
+	dir := t.TempDir()
+	path := filepath.Join(dir, "crossbar.toml")
+	good, err := os.ReadFile(filepath.Join("testdata", "good.toml"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(path, good, 0o000); err != nil {
+		t.Fatal(err)
+	}
+	_, err = config.Load(path)
+	if err == nil {
+		t.Fatal("Load on an unreadable file must fail")
+	}
+	if _, ok := config.IsError(err); ok {
+		t.Errorf("an unreadable file is not a validation *Error: %v", err)
+	}
+	if !strings.HasPrefix(err.Error(), "config: ") {
+		t.Errorf("Error() = %q, want the config: prefix", err.Error())
+	}
+}
+
+func TestDirectoryIsAnError(t *testing.T) {
+	_, err := config.Load(t.TempDir())
+	if err == nil {
+		t.Fatal("Load on a directory must fail")
+	}
+	if _, ok := config.IsError(err); ok {
+		t.Errorf("a directory is not a validation *Error: %v", err)
+	}
+}

docs/plans/v0.1/_files/internal/proxy/recorder_test.go

@@ -0,0 +1,66 @@
+package proxy_test
+
+import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"git.wntrmute.dev/kyle/crossbar/internal/config"
+	"git.wntrmute.dev/kyle/crossbar/internal/health"
+	"git.wntrmute.dev/kyle/crossbar/internal/proxy"
+)
+
+// noFlush is a ResponseWriter that does not implement http.Flusher. Middleware and test
+// recorders like this exist in the wild; the proxy must degrade to buffering, never panic.
+type noFlush struct{ w http.ResponseWriter }
+
+func (n noFlush) Header() http.Header         { return n.w.Header() }
+func (n noFlush) Write(b []byte) (int, error) { return n.w.Write(b) }
+func (n noFlush) WriteHeader(code int)        { n.w.WriteHeader(code) }
+
+func TestStreamingWriterWithoutFlusherDoesNotPanic(t *testing.T) {
+	up := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "text/event-stream")
+		w.WriteHeader(200)
+		for i := 0; i < 3; i++ {
+			fmt.Fprintf(w, "data: chunk %d\n\n", i)
+			w.(http.Flusher).Flush()
+		}
+	}))
+	t.Cleanup(up.Close)
+	cfg, err := config.Parse(strings.NewReader(fmt.Sprintf(`
+listen = "127.0.0.1:1"
+[hosts.alpha]
+base_url = %q
+models = { "m" = { } }
+[routes.r]
+hosts = ["alpha"]
+`, up.URL)))
+	if err != nil {
+		t.Fatal(err)
+	}
+	h := &fakeHealth{st: map[string]health.Status{"alpha": {Healthy: true, Loaded: []string{"m"}}}}
+	p := proxy.New(cfg, h, nil)
+
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodPost, "/r/v1/chat/completions", strings.NewReader(`{"model":"m","stream":true}`))
+	func() {
+		defer func() {
+			if r := recover(); r != nil {
+				t.Fatalf("ServeHTTP panicked on a writer without Flush: %v", r)
+			}
+		}()
+		p.ServeHTTP(noFlush{rec}, req)
+	}()
+	if rec.Code != 200 {
+		t.Fatalf("status %d", rec.Code)
+	}
+	if got := rec.Body.String(); !strings.Contains(got, "chunk 0") || !strings.Contains(got, "chunk 2") {
+		t.Errorf("body = %q, want all three chunks", got)
+	}
+	if rec.Header().Get(proxy.HostHeader) != "alpha" {
+		t.Errorf("host header %q", rec.Header().Get(proxy.HostHeader))
+	}
+}