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

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:

   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.

   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.

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.

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.