From 74e7d4895b8f5d16b47a90cf2cf7b66b5b9b4c1c Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Fri, 25 Sep 2026 02:55:57 -0700 Subject: [PATCH] v0.1 plan: review fixes as failing acceptance tests first; AGENTS.md lessons + deviations rule Co-Authored-By: Claude Fable 5.1 --- AGENTS.md | 13 ++- docs/plans/v0.1/01-review-fixes.md | 79 +++++++++++++++++++ docs/plans/v0.1/README.md | 28 +++++++ .../_files/internal/config/unreadable_test.go | 47 +++++++++++ .../_files/internal/proxy/recorder_test.go | 66 ++++++++++++++++ 5 files changed, 232 insertions(+), 1 deletion(-) create mode 100644 docs/plans/v0.1/01-review-fixes.md create mode 100644 docs/plans/v0.1/README.md create mode 100644 docs/plans/v0.1/_files/internal/config/unreadable_test.go create mode 100644 docs/plans/v0.1/_files/internal/proxy/recorder_test.go diff --git a/AGENTS.md b/AGENTS.md index 8f5c18d..692be77 100644 --- a/AGENTS.md +++ b/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. | diff --git a/docs/plans/v0.1/01-review-fixes.md b/docs/plans/v0.1/01-review-fixes.md new file mode 100644 index 0000000..96f428a --- /dev/null +++ b/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. diff --git a/docs/plans/v0.1/README.md b/docs/plans/v0.1/README.md new file mode 100644 index 0000000..035f5db --- /dev/null +++ b/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 :=`). diff --git a/docs/plans/v0.1/_files/internal/config/unreadable_test.go b/docs/plans/v0.1/_files/internal/config/unreadable_test.go new file mode 100644 index 0000000..5253df0 --- /dev/null +++ b/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) + } +} diff --git a/docs/plans/v0.1/_files/internal/proxy/recorder_test.go b/docs/plans/v0.1/_files/internal/proxy/recorder_test.go new file mode 100644 index 0000000..b5ffa45 --- /dev/null +++ b/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)) + } +}