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.

---