CamMonitor/.ai/REVIEW.md

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 <aside> hides sidebar at <768 px and shows at ≥768 px; data-mobile-drawer <details>/<summary> provides hamburger drawer on mobile; bottom <nav> with md:hidden is the mobile tab bar; TestDayOverviewRendersShellNavigationAndCounts asserts both responsive class names, month grouping, sidebar day links, image/video counts, and tab links.
• {{define "base"}} / {{template "content" .}} pattern is a sound Go template composition approach — functionally equivalent to {{block "content" .}}.
• groupDaysByMonth correctly preserves newest-first order within each month group (days come from DayList() which is already sorted).
• Mobile hamburger uses pure <details>/<summary> — no JavaScript required for open/close. ✅
• render() pulls User from context and Months from the live index on every request — sidebar always fresh. ✅
• Index handler nil-safe (h.index == nil guard). ✅
• Day() returns http.NotFound for unknown dates. ✅
• Tests cover: redirect to newest day on /, day overview with all shell assertions, 404 for missing day, and existing auth/health routes still pass.

Risks:

• Finding #1 (buffer-less render) will become more visible when T-005/T-006 handlers reuse the same render() pattern with more dynamic data; recommend fixing before T-006 lands.

---

T-005 — Image browser

Verdict: PASS_WITH_NOTES

Findings

#	Severity	File	Description	Required fix?
1	minor	internal/image/handler.go (lines 27–40, 221–251)	ShellData, MonthGroup, and groupDaysByMonth are exact duplicates of the same identifiers already in internal/web/handler.go. internal/image cannot import internal/web (circular: internal/web → internal/image), so the fix requires extracting these shared shell types to a new package (e.g. internal/shell or internal/ui). T-006 will add a third copy of the same code if this is not resolved.	no
2	nit	internal/image/handler.go (lines 182–185)	Same buffer-less render pattern as T-004: Content-Type header written before ExecuteTemplate, making the http.Error fallback a no-op if the template fails mid-write.	no
3	nit	internal/web/templates/images.html (lines 41–45)	Arrow navigation uses window.location.href = url.toString() (full page reload) rather than history.pushState with a partial update as described in the plan design notes. The acceptance criteria don't require push-state; navigation is functionally correct.	no

Required fixes

None.

Verification

Steps performed:

1. Read all new files: internal/image/thumb.go, internal/image/handler.go, internal/image/handler_test.go, internal/web/templates/images.html, internal/web/templates/templates.go, internal/web/router.go, go.mod.
2. Cross-checked against .ai/PLAN.md Phase 5 scope.
3. Confirmed golang.org/x/image v0.42.0 added to go.mod. ✅
4. Ran go fmt ./... — clean.
5. Ran go vet ./... — clean.
6. Ran go test ./internal/image/... -v — all 4 tests PASS.
7. Ran go test -race ./... — PASS, no data races.

Findings:

• All acceptance criteria met: thumbnail JPEG ≤30 KB verified by test decoding the response; raw endpoint byte-equality verified; path traversal (%2e%2e) returns 400; page renders strip (ring highlight), viewer, prev/next links, keyboard arrows.
• FIFO eviction (order []string + slice shift) is correct and bounded to max entries. ✅
• resizeToFit uses aspect-ratio-preserving scaling: scales to fit within 160×90, never stretches. ✅
• Cache.get and Cache.add return defensive copies — callers cannot alias internal byte slices. ✅
• Path validation uses both a string prefix check (..") and filepath.Rel escape check — two independent layers. ✅
• Cache-Control: max-age=3600 set on thumb responses. ✅
• Routes wired correctly under RequireAuth middleware in router.go. ✅
• imageRouter test helper bypasses auth (tests the handler directly), which is correct for unit-level coverage. ✅
• Wrapping arithmetic for prev/next is correct: (active ± 1 + len) % len. Test validates idx=1 with 2 images gives prev=0, next=0. ✅

Risks:

• Finding #1 (shell type duplication) is the main structural risk: T-006 will face the same choice and may add a third copy. The T-006 implementer should extract shared types before or during that task.

---