reject non-3xx status codes; close the CSV file; add tests #3

2026-09-07T13:20:26Z

claude commented

2026-09-07 13:20:26 +00:00

The bug

A CSV row with a status code outside 3xx — most likely a typo like 0 or 30 — was accepted at load. On the first request matching that row, w.WriteHeader(statusCode) panics (net/http rejects codes < 100), taking down that request.

ReadCsvRedirects now rejects any code that isn't 300–399 at load time, with the line number in the message, and reports a non-numeric code clearly instead of surfacing the raw strconv error.

If you'd rather allow non-3xx (e.g. 410), loosen the check to 100–599 — that still prevents the panic. Noting it so the 3xx choice is deliberate.

Also

defer reader.Close() — the file was held open for the process lifetime.
Dropped the unreachable row == nil branch (csv.Reader signals end with io.EOF, already handled).

Tests (first in the repo)

internal/csv_test.go — valid file, reordered headings, header-only, missing heading, non-numeric status, non-3xx status, ragged row, missing file; plus Headings.IndexOf.
internal/redirect_test.go — Redirects.Find (hit, miss, case-sensitivity) and Redirect.Match.
internal/http_test.go — ServeHTTP: redirect with Location, 404 with none, query string dropped.

go vet / go build / go test ./... green.

Broader review notes are in a comment below — this repo has its issue tracker disabled, so they could not be filed as an issue.

🤖 Generated with Claude Code

### The bug A CSV row with a status code outside 3xx — most likely a typo like `0` or `30` — was accepted at load. On the first request matching that row, `w.WriteHeader(statusCode)` panics (`net/http` rejects codes `< 100`), taking down that request. `ReadCsvRedirects` now rejects any code that isn't `300`–`399` at load time, with the line number in the message, and reports a non-numeric code clearly instead of surfacing the raw `strconv` error. If you'd rather allow non-3xx (e.g. `410`), loosen the check to `100`–`599` — that still prevents the panic. Noting it so the 3xx choice is deliberate. ### Also - `defer reader.Close()` — the file was held open for the process lifetime. - Dropped the unreachable `row == nil` branch (`csv.Reader` signals end with `io.EOF`, already handled). ### Tests (first in the repo) - `internal/csv_test.go` — valid file, reordered headings, header-only, missing heading, non-numeric status, non-3xx status, ragged row, missing file; plus `Headings.IndexOf`. - `internal/redirect_test.go` — `Redirects.Find` (hit, miss, case-sensitivity) and `Redirect.Match`. - `internal/http_test.go` — `ServeHTTP`: redirect with `Location`, 404 with none, query string dropped. `go vet` / `go build` / `go test ./...` green. Broader review notes are in a comment below — this repo has its issue tracker disabled, so they could not be filed as an issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude added 1 commit 2026-09-07 13:20:26 +00:00

reject non-3xx status codes; close the CSV file; add tests

CI / go-tests (pull_request) Successful in 2m4s

Details

68546badb3

A row with a status code outside 3xx (e.g. "0" from a typo) was accepted
at load and then panicked net/http's WriteHeader on the first matching
request. ReadCsvRedirects now rejects anything that isn't 300-399, with a
line number in the message, and reports a non-numeric code clearly.

Also defer reader.Close() (the file was left open for the process
lifetime) and drop the unreachable "row == nil" check.

Adds internal/{csv,redirect,http}_test.go covering CSV parsing (valid,
reordered headings, header-only, missing heading, non-numeric and non-3xx
status, ragged row, missing file), Redirects.Find / Redirect.Match, and
ServeHTTP (redirect, 404, query string dropped).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

claude commented

2026-09-07 13:20:55 +00:00

Review notes (the issue tracker is off on this repo, so leaving these here).

Review notes from a pass over the whole repo. The two PRs alongside handle the module path and the invalid-status panic + a starter test suite; this issue collects the judgement calls and smaller items.

Behaviour

Query string is dropped. ServeHTTP redirects to redirect.To verbatim, so /old?ref=x → Location: <To> with no ?ref=x. Fine if intended; worth a line in the README either way. Forwarding it would be To + "?" + req.URL.RawQuery when RawQuery != "".
No path normalisation. Matching is exact (from == r.From) against req.URL.Path, which always has a leading /. A CSV From of old (no slash) silently never matches; /old and /old/ are distinct. Consider trimming/normalising both sides at load, or at least documenting that From must be written exactly as the path arrives.
main.go panics on user error. A bad --file path prints a stack trace rather than a message + non-zero exit. And the server goroutine ends in panic(<-errc), so a failed bind (port in use) is also a stack trace. There's no signal handling / graceful shutdown. For a tool this small that may be acceptable, but a log.Fatal-style exit for the CSV-load and bind errors would be friendlier.

Efficiency

Redirects.Find is a linear scan per request. Matching is exact-string, so a map[string]*Redirect built once in ReadCsvRedirects would make lookups O(1). Keep the slice too if insertion order matters for logging.

CI

ci.yml runs only on pull_request; a direct push to main gets no vet/test/build. Add push: branches: [main].
govulncheck is go install ...@latest on every run — unpinned and re-downloaded each time. Pin a version, and ideally cache $GOPATH/bin or use a prebuilt action.

Smaller

CLI.Host defaults to localhost but there's no short flag and the Dockerfile has to pass --host 0.0.0.0; a 0.0.0.0 default (or an --all-interfaces convenience) would remove that footgun for the containerised case.
No LICENSE file (other repos here have one).

Remaining test gaps (after the test-suite PR)

main.go wiring (kong parsing, JoinHostPort) is untested — could be extracted into a testable run() function.
CSV with quoted fields / embedded commas, UTF-8 BOM, CRLF line endings.
ServeHTTP with a non-GET method (currently redirects regardless of method — is that wanted for POST?).

**Review notes** (the issue tracker is off on this repo, so leaving these here). Review notes from a pass over the whole repo. The two PRs alongside handle the module path and the invalid-status panic + a starter test suite; this issue collects the judgement calls and smaller items. ## Behaviour - **Query string is dropped.** `ServeHTTP` redirects to `redirect.To` verbatim, so `/old?ref=x` → `Location: <To>` with no `?ref=x`. Fine if intended; worth a line in the README either way. Forwarding it would be `To + "?" + req.URL.RawQuery` when `RawQuery != ""`. - **No path normalisation.** Matching is exact (`from == r.From`) against `req.URL.Path`, which always has a leading `/`. A CSV `From` of `old` (no slash) silently never matches; `/old` and `/old/` are distinct. Consider trimming/normalising both sides at load, or at least documenting that `From` must be written exactly as the path arrives. - **`main.go` panics on user error.** A bad `--file` path prints a stack trace rather than a message + non-zero exit. And the server goroutine ends in `panic(<-errc)`, so a failed bind (port in use) is also a stack trace. There's no signal handling / graceful shutdown. For a tool this small that may be acceptable, but a `log.Fatal`-style exit for the CSV-load and bind errors would be friendlier. ## Efficiency - **`Redirects.Find` is a linear scan per request.** Matching is exact-string, so a `map[string]*Redirect` built once in `ReadCsvRedirects` would make lookups O(1). Keep the slice too if insertion order matters for logging. ## CI - `ci.yml` runs only on `pull_request`; a direct push to `main` gets no vet/test/build. Add `push: branches: [main]`. - `govulncheck` is `go install ...@latest` on every run — unpinned and re-downloaded each time. Pin a version, and ideally cache `$GOPATH/bin` or use a prebuilt action. ## Smaller - `CLI.Host` defaults to `localhost` but there's no short flag and the Dockerfile has to pass `--host 0.0.0.0`; a `0.0.0.0` default (or an `--all-interfaces` convenience) would remove that footgun for the containerised case. - No `LICENSE` file (other repos here have one). ## Remaining test gaps (after the test-suite PR) - `main.go` wiring (kong parsing, `JoinHostPort`) is untested — could be extracted into a testable `run()` function. - CSV with quoted fields / embedded commas, UTF-8 BOM, CRLF line endings. - `ServeHTTP` with a non-GET method (currently redirects regardless of method — is that wanted for POST?).

aneurin approved these changes

2026-09-07 15:42:05 +00:00

aneurin commented

2026-09-07 15:42:38 +00:00

Approved. Go ahead and merge and extract the remaining test gaps into issues for another day.

claude referenced this pull request

2026-09-07 15:49:28 +00:00

Test gaps remaining after the starter suite #4

claude commented

2026-09-07 15:49:38 +00:00