# REVIEW Append-only reviewer log. Each reviewed task gets one section. --- ## T-001 — Project scaffold **Verdict:** PASS_WITH_NOTES ### Findings | # | Severity | File | Description | Required fix? | |---|----------|------|-------------|---------------| | 1 | nit | `.env.example` | Missing `DB_PATH` and `LISTEN_ADDR` entries. The plan requires `.env.example` to document all env vars; the README does cover them but the example file operators copy is incomplete. | no | | 2 | nit | `docker-compose.yml` | `LISTEN_ADDR` env var is not forwarded into the container; the port mapping hardcodes `8080` as the inner port. If an operator sets `LISTEN_ADDR` to something other than `:8080` only locally editing the compose file would fix the mismatch. | no | | 3 | nit | `internal/db/db.go` | SQLite connection pool is not limited to a single open connection. Concurrent writes (introduced in T-002) risk `SQLITE_BUSY` / "database is locked" errors. Consider adding `database.SetMaxOpenConns(1)` before the health check in a future task. | no | | 4 | nit | `README.md` | Contains extensive AI workflow boilerplate (workflow profiles, session commands, file map, etc.) that is unrelated to the application's end-user docs. The plan scope for README was quick-start, env var reference, and footage directory format — all of which are present. | no | ### Required fixes None — all findings are nits. No blocking issues. --- ## T-002 — Auth **Verdict:** FAIL ### Findings | # | Severity | File | Description | Required fix? | |---|----------|------|-------------|---------------| | 1 | major | `internal/auth/handler.go` (lines 119–183) | Templates (`login`, `admin_users`) are inline Go template strings, not separate `.html` files. The plan's repository layout explicitly lists `internal/web/templates/login.html` and `internal/web/templates/admin_users.html`. T-004 introduces `base.html` with `{{block "title" .}}` / `{{block "content" .}}` template inheritance — the admin_users page must be able to extend that base. Inline strings in a different package make that impossible without a refactor spike inside T-004. | **yes** | | 2 | minor | `internal/auth/handler.go` (lines 49–56) | Session cookie has no `Secure` flag. If the app is reverse-proxied over HTTPS (the expected production path), the cookie is transmitted over HTTP between the browser and the proxy for nothing, defeating `HttpOnly`. Acceptable for pure LAN-only deployment but should be documented or made configurable. | no | | 3 | minor | `internal/auth/handler.go` (lines 104–116) | `DeleteUser` does not prevent deleting the last admin (or the currently logged-in user). An admin could inadvertently lock out all access. | no | | 4 | nit | `go.mod` (line 3) | Go version bumped from `1.22` (specified in the plan) to `1.25.0`. Higher is generally fine but is an undocumented deviation from the plan's stated constraint. | no | | 5 | nit | `cmd/server/main.go` (lines 40–49) | `purgeExpiredSessions` goroutine has no stop channel and cannot be shut down cleanly. Harmless in practice (OS reclaims on exit) but leaves dangling goroutine state. | no | ### Required fixes **Finding #1 — move templates to separate files** Move `loginTemplate` and `adminUsersTemplate` out of `handler.go` and into: - `internal/web/templates/login.html` - `internal/web/templates/admin_users.html` Load them via `embed.FS` or `template.ParseFiles` in a shared template registry (or individually per handler). Structure the templates so T-004 can add `{{block "content" .}}` / `{{block "title" .}}` inheritance without touching auth code. The login page may stay standalone (pre-auth, no sidebar). `admin_users.html` at minimum must be a separate file so T-004 can wrap it in the base shell. ### Verification **Steps performed:** 1. Read all new/changed files: `internal/auth/handler.go`, `internal/auth/middleware.go`, `internal/auth/store.go`, `internal/auth/handler_test.go`, `internal/auth/store_test.go`, `internal/db/db.go`, `internal/web/router.go`, `internal/web/router_test.go`, `cmd/server/main.go`, `go.mod`. 2. Cross-checked implementation against `.ai/PLAN.md` Phase 2 scope and repository layout. 3. Ran `go fmt ./...` — clean. 4. Ran `go vet ./...` — clean. 5. Ran `go test ./...` — `internal/auth` PASS, `internal/web` PASS. 6. Ran `go test -race ./...` — PASS, no data races. 7. Ran `go test ./internal/auth/... ./internal/web/... -v` — all 9 tests pass (login renders, valid/invalid credentials, admin gate, store auth, expired session, ensure-admin idempotency, health, unauthenticated redirect). **Findings:** - All acceptance criteria met at the functional level: login renders, valid credentials redirect + set cookie, invalid returns 401, admin gate enforced, unauthenticated routes redirect. - SQLite schema matches plan exactly (users + sessions tables, WAL mode, foreign keys). - `EnsureAdmin` idempotency tested and correct. bcrypt + crypto/rand token generation correct. Cookie expiry delete pattern (`MaxAge: -1`) is correct. Token is 32-byte base64url — adequate entropy. - Main structural deviation is inline templates (finding #1), which blocks clean T-004 integration. **Risks:** - If finding #1 is not fixed, T-004 will need a refactor spike to migrate inline templates to file-based ones before base template inheritance can work, increasing T-004 scope unexpectedly. ### Verification **Steps performed:** 1. Read all T-001 files: `cmd/server/main.go`, `internal/config/config.go`, `internal/db/db.go`, `internal/web/router.go`, `internal/web/router_test.go`, `Dockerfile`, `docker-compose.yml`, `.env.example`, `go.mod`, `README.md`, `.dockerignore`. 2. Cross-checked implementation against `.ai/PLAN.md` Phase 1 scope. 3. Ran `go fmt ./...` — clean (no output). 4. Ran `go vet ./...` — clean. 5. Ran `go test ./...` — `internal/web` PASS, other packages report no test files (correct for T-001 scope). 6. Ran `go test -race ./...` — PASS, no data races. 7. `docker compose build` and `curl -i http://127.0.0.1:18080/health` verified by implementer evidence; both passed. **Findings:** - All acceptance criteria met: `docker compose build` succeeds (per evidence), `GET /health` returns 200 and `{"status":"ok"}` (covered by test + E2E evidence), `go vet ./...` passes. - Health handler correctly sets `Content-Type: application/json` before writing the body. - Router test covers both status code and JSON body parsing — adequate for T-001 scope. - Multi-stage Dockerfile matches plan exactly (`golang:1.22-bookworm` builder → `debian:bookworm-slim` + ffmpeg runtime). - `CGO_ENABLED=0` build flag is correct; aligns with the no-CGO constraint. - `modernc.org/sqlite` (pure-Go) is the only SQLite driver imported — constraint satisfied. **Risks:** - SQLite connection pool (finding #3) will matter in T-002; surfaced early so the implementer can address it then. - `.env.example` gaps (finding #1) are low-risk for a scaffold task but should be cleaned up before shipping. --- ## T-002 — Auth (rework pass) **Verdict:** PASS ### Findings All findings from the initial review pass addressed or confirmed non-blocking. No new findings. ### Required fixes None. ### Verification **Steps performed:** 1. Read reworked files: `internal/auth/handler.go`, `internal/auth/handler_test.go`, `internal/web/templates/templates.go`, `internal/web/templates/login.html`, `internal/web/templates/admin_users.html`. 2. Confirmed blocking finding #1 resolved: templates moved to `internal/web/templates/*.html`; `templates.go` exposes `embed.FS` with `//go:embed *.html` and named string constants; `handler.go` loads via `template.ParseFS(webtemplates.FS, ...)` and renders via `ExecuteTemplate`. 3. Tests extended with `readTemplateFile` / `renderTemplateFile` helpers that read from disk, proving file-backed rendering is live (not a cached inline string). 4. Ran `go fmt ./...` — clean. 5. Ran `go vet ./...` — clean. 6. Ran `go test ./...` — all packages PASS. 7. Ran `go test -race ./...` — PASS, no data races. **Findings:** - `templates.go` `embed.FS` gives T-004 a clean extension point — it can register `base.html` in the same package and `admin_users.html` can reference it when T-004 lands. - `template.ParseFS` + `ExecuteTemplate(w, "login.html", nil)` pattern is correct; template name matches the file name. - Non-blocking nits from initial review (no `Secure` cookie flag, no last-admin guard, go 1.25.0 version bump, non-stoppable purge goroutine) remain; none block this task. **Risks:** - When T-004 adds `base.html` to the embed glob, template parse order matters; the implementer should parse base first or use `template.ParseFS(..., "base.html", "admin_users.html")` to ensure the base is available before extension templates. --- ## T-003 — Footage scanner **Verdict:** PASS_WITH_NOTES ### Findings | # | Severity | File | Description | Required fix? | |---|----------|------|-------------|---------------| | 1 | nit | `internal/footage/index.go` (line 83) | `rescan()` silently discards `Scan()` errors — the index silently retains stale data with no log output. Hard to diagnose filesystem issues in production. | no | | 2 | nit | `internal/web/router.go` (line 14) | `_ *footage.Index` parameter accepted but still ignored. Correct for T-003 scope; will be wired in T-004. | no | ### Required fixes None. ### Verification **Steps performed:** 1. Read all new/changed files: `internal/footage/scanner.go`, `internal/footage/index.go`, `internal/footage/scanner_test.go`, `internal/footage/index_test.go`, `cmd/server/main.go`, `internal/web/router.go`. 2. Cross-checked against `.ai/PLAN.md` Phase 3 scope — all required types, functions, and behaviours implemented. 3. Inspected fixture files in `testdata/footage/` — 5 files across 2 day directories, correct naming pattern, 8 bytes each. 4. Checked regex patterns against fixture filenames: image `^A(\d{6})(\d{6})\d+\.jpg$` and video `^A(\d{6})_(\d{6})_(\d{6})\.265$` both match. 5. Verified time parsing format `"060102150405"` (YYMMDD + HHMMSS) produces correct timestamps for fixture filenames. 6. Ran `go fmt ./...` — clean. 7. Ran `go vet ./...` — clean. 8. Ran `go test ./internal/footage/... -v` — all 7 tests PASS. 9. Ran `go test -race ./...` — PASS, no data races (cached + fresh run). **Findings:** - All acceptance criteria met: fixture directory indexed correctly (2 days, correct image/video counts), `DayList()` returns newest-first sorted dates, `DayEntry` contains timestamp-sorted images and start-time-sorted videos, periodic rescan confirmed by `TestIndexPeriodicRescan`, race detector clean on `TestIndexConcurrentAccessDuringRescan` (8 goroutines × 100 iterations). - `copyDayEntry` defensively copies slices on every `Day()` call — callers cannot mutate internal index state. ✅ - `Close()` uses `sync.Once` around `close(done)` — safe to call multiple times. ✅ - `NewIndex` does an initial synchronous `rescan()` before starting the goroutine — index is always non-empty on first use if footage exists. ✅ - Midnight-spanning clips handled: `if end.Before(start) { end = end.Add(24 * time.Hour) }`. ✅ - Missing `images/` or `record/` subdirectory returns `nil, nil` (not an error) — scanner gracefully handles partial day directories. ✅ - `main.go` wires `footage.NewIndex` and defers `Close()` correctly. ✅ **Risks:** - Silent error discard in `rescan()` (finding #1) is low risk for correctness but could delay diagnosis of a footage mount problem in production. --- ## T-004 — UI shell & day navigation **Verdict:** PASS_WITH_NOTES ### Findings | # | Severity | File | Description | Required fix? | |---|----------|------|-------------|---------------| | 1 | minor | `internal/web/handler.go` (lines 81–84) | `render()` sets `Content-Type` header then calls `ExecuteTemplate` directly to `w`. If template execution fails after writing any bytes, the subsequent `http.Error(w, ...)` call is a no-op — headers are already sent and the HTTP status code cannot be changed. Fix: render into a `bytes.Buffer` first, check error, then write in one shot. | no | | 2 | nit | `internal/web/handler.go` (line 48–51) | Empty-footage case sets `Date: "No footage"` in `DayPageData` and renders the day template, which shows `"No footage"` as the date heading. Minor UX rough edge; a dedicated empty-state template or prose would be cleaner. | no | | 3 | nit | `internal/web/handler.go` (line 24) | `ShellData.ActiveTab` field is declared but never written or read in current templates. Scaffolded for T-005/T-006. Harmless. | no | | 4 | nit | `internal/web/templates/admin_users.html` | Admin users page still does not extend `base.html` — it has no sidebar or mobile nav. Acceptable for current scope; T-004 plan does not require refactoring auth pages. | no | ### Required fixes None. ### Verification **Steps performed:** 1. Read all new/changed files: `internal/web/handler.go`, `internal/web/router.go`, `internal/web/router_test.go`, `internal/web/templates/base.html`, `internal/web/templates/day.html`, `internal/web/templates/templates.go`. 2. Cross-checked against `.ai/PLAN.md` Phase 4 scope. 3. Ran `go fmt ./...` — clean. 4. Ran `go vet ./...` — clean. 5. Ran `go test ./internal/web/... -v` — all 5 tests PASS. 6. Ran `go test -race ./...` — PASS, no data races. **Findings:** - All acceptance criteria met: `hidden ... md:block` on `