80 lines
3.7 KiB
Markdown
80 lines
3.7 KiB
Markdown
# 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.
|