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

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>

internal/csv.go

@@ -3,6 +3,7 @@ package internal
 import (
 	"encoding/csv"
 	"errors"
+	"fmt"
 	"io"
 	"os"
 	"strconv"
@@ -25,6 +26,7 @@ func ReadCsvRedirects(file string) (Redirects, error) {
 	if err != nil {
 		return nil, err
 	}
+	defer reader.Close()
 
 	csvReader := csv.NewReader(reader)
 
@@ -44,7 +46,7 @@ func ReadCsvRedirects(file string) (Redirects, error) {
 	// Read data
 	redirects := Redirects{}
 
-	for {
+	for line := 2; ; line++ {
 		row, err := csvReader.Read()
 		if err != nil {
 			if errors.Is(err, io.EOF) {
@@ -52,22 +54,20 @@ func ReadCsvRedirects(file string) (Redirects, error) {
 			}
 			return nil, err
 		}
-		if row == nil {
-			break
-		}
 
 		statusCode, err := strconv.Atoi(row[statusCol])
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("line %d: status code %q is not a number", line, row[statusCol])
+		}
+		if statusCode < 300 || statusCode > 399 {
+			return nil, fmt.Errorf("line %d: status code %d is not a redirect (must be 3xx)", line, statusCode)
 		}
 
-		redirect := &Redirect{
+		redirects = append(redirects, &Redirect{
 			From:       row[fromCol],
 			To:         row[toCol],
 			StatusCode: statusCode,
-		}
-
-		redirects = append(redirects, redirect)
+		})
 	}
 
 	return redirects, nil

internal/csv_test.go

@@ -0,0 +1,102 @@
+package internal
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func writeCSV(t *testing.T, content string) string {
+	t.Helper()
+	p := filepath.Join(t.TempDir(), "redirects.csv")
+	if err := os.WriteFile(p, []byte(content), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	return p
+}
+
+func TestReadCsvRedirects(t *testing.T) {
+	t.Run("valid file", func(t *testing.T) {
+		p := writeCSV(t, "From,To,Status Code\n/a,https://example.com/a,301\n/b,https://example.com/b,302\n")
+
+		got, err := ReadCsvRedirects(p)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(got) != 2 {
+			t.Fatalf("got %d redirects, want 2", len(got))
+		}
+		if got[0].From != "/a" || got[0].To != "https://example.com/a" || got[0].StatusCode != 301 {
+			t.Errorf("row 0 = %+v", got[0])
+		}
+	})
+
+	t.Run("headings in any order", func(t *testing.T) {
+		p := writeCSV(t, "Status Code,To,From\n301,https://example.com,/x\n")
+
+		got, err := ReadCsvRedirects(p)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if got[0].From != "/x" || got[0].To != "https://example.com" || got[0].StatusCode != 301 {
+			t.Errorf("got %+v", got[0])
+		}
+	})
+
+	t.Run("header only yields no redirects", func(t *testing.T) {
+		got, err := ReadCsvRedirects(writeCSV(t, "From,To,Status Code\n"))
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(got) != 0 {
+			t.Fatalf("got %d redirects, want 0", len(got))
+		}
+	})
+
+	t.Run("missing heading is an error", func(t *testing.T) {
+		if _, err := ReadCsvRedirects(writeCSV(t, "From,To\n/a,https://example.com\n")); err == nil {
+			t.Fatal("want error for missing Status Code heading")
+		}
+	})
+
+	t.Run("non-numeric status is an error", func(t *testing.T) {
+		if _, err := ReadCsvRedirects(writeCSV(t, "From,To,Status Code\n/a,https://example.com,oops\n")); err == nil {
+			t.Fatal("want error for non-numeric status code")
+		}
+	})
+
+	t.Run("non-3xx status is an error", func(t *testing.T) {
+		for _, code := range []string{"0", "-1", "200", "404", "500", "1000"} {
+			_, err := ReadCsvRedirects(writeCSV(t, "From,To,Status Code\n/a,https://example.com,"+code+"\n"))
+			if err == nil {
+				t.Errorf("status %s: want error, got nil", code)
+			}
+		}
+	})
+
+	t.Run("ragged row is an error", func(t *testing.T) {
+		if _, err := ReadCsvRedirects(writeCSV(t, "From,To,Status Code\n/a,https://example.com\n")); err == nil {
+			t.Fatal("want error for a row with too few fields")
+		}
+	})
+
+	t.Run("missing file is an error", func(t *testing.T) {
+		if _, err := ReadCsvRedirects(filepath.Join(t.TempDir(), "nope.csv")); err == nil {
+			t.Fatal("want error for a file that does not exist")
+		}
+	})
+}
+
+func TestHeadingsIndexOf(t *testing.T) {
+	h := Headings{"From", "To", "Status Code"}
+
+	if got := h.IndexOf("To"); got != 1 {
+		t.Errorf("IndexOf(To) = %d, want 1", got)
+	}
+	if got := h.IndexOf("Status Code"); got != 2 {
+		t.Errorf("IndexOf(Status Code) = %d, want 2", got)
+	}
+	if got := h.IndexOf("Nope"); got != -1 {
+		t.Errorf("IndexOf(Nope) = %d, want -1", got)
+	}
+}

internal/http_test.go

@@ -0,0 +1,56 @@
+package internal
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/rs/zerolog"
+)
+
+func newTestServer(rs Redirects) *HttpServer {
+	return &HttpServer{
+		CLI:       &CLI{},
+		Log:       zerolog.Nop(),
+		Redirects: rs,
+	}
+}
+
+func TestServeHTTP(t *testing.T) {
+	srv := newTestServer(Redirects{
+		{From: "/old", To: "https://example.com/new", StatusCode: 301},
+	})
+
+	t.Run("known path redirects with Location", func(t *testing.T) {
+		rec := httptest.NewRecorder()
+		srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/old", nil))
+
+		if rec.Code != 301 {
+			t.Errorf("status = %d, want 301", rec.Code)
+		}
+		if loc := rec.Header().Get("Location"); loc != "https://example.com/new" {
+			t.Errorf("Location = %q", loc)
+		}
+	})
+
+	t.Run("unknown path is 404 with no Location", func(t *testing.T) {
+		rec := httptest.NewRecorder()
+		srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/nope", nil))
+
+		if rec.Code != http.StatusNotFound {
+			t.Errorf("status = %d, want 404", rec.Code)
+		}
+		if loc := rec.Header().Get("Location"); loc != "" {
+			t.Errorf("Location = %q, want empty", loc)
+		}
+	})
+
+	t.Run("query string is dropped (current behaviour)", func(t *testing.T) {
+		rec := httptest.NewRecorder()
+		srv.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/old?ref=x", nil))
+
+		if loc := rec.Header().Get("Location"); loc != "https://example.com/new" {
+			t.Errorf("Location = %q", loc)
+		}
+	})
+}

internal/redirect_test.go

@@ -0,0 +1,31 @@
+package internal
+
+import "testing"
+
+func TestRedirectsFind(t *testing.T) {
+	rs := Redirects{
+		{From: "/a", To: "https://example.com/a", StatusCode: 301},
+		{From: "/b", To: "https://example.com/b", StatusCode: 302},
+	}
+
+	if got := rs.Find("/b"); got == nil || got.To != "https://example.com/b" {
+		t.Errorf("Find(/b) = %+v", got)
+	}
+	if got := rs.Find("/missing"); got != nil {
+		t.Errorf("Find(/missing) = %+v, want nil", got)
+	}
+	if got := rs.Find("/A"); got != nil {
+		t.Errorf("Find is case-sensitive; Find(/A) = %+v, want nil", got)
+	}
+}
+
+func TestRedirectMatch(t *testing.T) {
+	r := &Redirect{From: "/x"}
+
+	if !r.Match("/x") {
+		t.Error("Match(/x) = false, want true")
+	}
+	if r.Match("/x/") {
+		t.Error("Match(/x/) = true, want false (exact match only)")
+	}
+}