test(05): add phase verification report

.planning/phases/05-files/05-VERIFICATION.md

@@ -0,0 +1,189 @@
+---
+phase: 05-files
+verified: 2026-05-15T00:00:00Z
+status: human_needed
+score: 6/6 must-haves verified
+overrides_applied: 0
+human_verification:
+  - test: "Tab navigation updates browser URL"
+    expected: "Clicking Tasks tab changes URL to /tablos/{id}/tasks; clicking Files tab changes URL to /tablos/{id}/files; browser Back restores previous URL and content"
+    why_human: "hx-push-url attribute is present in template code but URL-bar update behavior requires a live browser session — cannot be verified programmatically"
+  - test: "File upload end-to-end with real S3/MinIO"
+    expected: "POST /tablos/{id}/files stores bytes in MinIO bucket and the file row appears in the list immediately"
+    why_human: "Integration tests skip without TEST_DATABASE_URL; S3 wiring requires live MinIO — unit tests use a stub FileStorer"
+  - test: "Download delivers file content via presigned redirect"
+    expected: "Clicking Download redirects browser to a signed MinIO URL that serves the file correctly"
+    why_human: "302 to presigned URL behaviour verified in unit test but actual file delivery through MinIO requires a live service"
+  - test: "Oversize upload shows friendly error (not 500)"
+    expected: "Uploading a file larger than MAX_UPLOAD_SIZE_MB (default 25 MB) returns 422 with 'too large' message above the form"
+    why_human: "TestFileUploadTooLarge is implemented and correct in code, but skips in CI because it needs a real DB — developer should run with TEST_DATABASE_URL set"
+  - test: "Delete removes file from list immediately; gone after refresh"
+    expected: "HTMX delete swaps out the file row; full-page refresh confirms the row is absent"
+    why_human: "TestFileDelete passes in code logic but skips in CI — live browser confirmation needed"
+  - test: "Only tablo owner can access file routes"
+    expected: "Non-owner navigating to /tablos/{other_user_id}/files gets 404"
+    why_human: "TestFileOwnership is implemented and correct in code but skips in CI — developer should exercise with TEST_DATABASE_URL or real browser login"
+---
+
+# Phase 5: Files Verification Report
+
+**Phase Goal:** A user can attach files to a tablo, list them, download them via signed URLs, and delete them — backed by S3-compatible storage.
+**Verified:** 2026-05-15T00:00:00Z
+**Status:** human_needed
+**Re-verification:** No — initial verification
+
+---
+
+## Goal Achievement
+
+### Observable Truths (ROADMAP Success Criteria)
+
+| # | Truth | Status | Evidence |
+|---|-------|--------|----------|
+| SC-1 | Uploading a file creates a `tablo_files` row and stores bytes in S3 | VERIFIED | `FileUploadHandler` calls `deps.Files.Upload` then `InsertTabloFile`; `TestFileUpload` asserts DB row + S3 key; S3 key uses `"files/{tablo_id}/{uuid}"` (D-04) |
+| SC-2 | Files list shows original filename, size, and uploaded-at; sorted newest first | VERIFIED | `FilesTabFragment` + `FileListRow` render `file.Filename`, `formatBytes(file.SizeBytes)`, `file.CreatedAt`; `ListFilesByTablo` query orders `BY created_at DESC`; `TestFilesList` asserts filename and size appear in response |
+| SC-3 | Downloads use signed URLs with 5-minute TTL generated server-side | VERIFIED | `FileDownloadHandler` calls `deps.Files.PresignDownload`; `Store.PresignDownload` sets `o.Expires = 5 * time.Minute`; returns `http.StatusFound` (302 redirect); `TestFileDownload` asserts 302 + Location header |
+| SC-4 | Deleting a file removes both DB row and S3 object; failures surfaced/logged | VERIFIED | `FileDeleteHandler` calls `deps.Files.Delete` first (line 312), logs error but continues (no early return); then calls `deps.Queries.DeleteTabloFile` (line 317); `TestFileDelete_S3Failure` verifies DB row deleted even when S3 returns error |
+| SC-5 | Only tablo owner can upload/list/download/delete (verified by tests) | VERIFIED | All handlers call `loadOwnedTablo` / `loadOwnedTabloForFile` which uses `GetTabloByID` with user_id filter; `GetTabloFileByID` includes `tablo_id = $2`; `TestFileDownload_NonOwner` and `TestFileOwnership` verify 404 for non-owners |
+| SC-6 | Configurable max upload size enforced server-side with friendly error message | VERIFIED | `http.MaxBytesReader` wraps `r.Body` before `ParseMultipartForm` (line 147-149); `*http.MaxBytesError` detected; `UploadErrorFragment` renders error with message from `formatMBError(deps.MaxUploadMB)`; `MAX_UPLOAD_SIZE_MB` env var parsed in `main.go` (default 25) |
+
+**Score:** 6/6 truths verified
+
+---
+
+### Required Artifacts
+
+| Artifact | Expected | Status | Details |
+|----------|----------|--------|---------|
+| `backend/migrations/0005_files.sql` | tablo_files schema | VERIFIED | Contains `CREATE TABLE tablo_files`, `ON DELETE CASCADE`, `-- +goose Up`, `-- +goose Down`, `bigint` for `size_bytes`, `timestamptz` |
+| `backend/internal/db/queries/files.sql` | sqlc query definitions | VERIFIED | Contains `InsertTabloFile :one`, `ListFilesByTablo :many`, `GetTabloFileByID :one`, `DeleteTabloFile :exec` |
+| `backend/internal/files/store.go` | S3 client + FileStorer interface | VERIFIED | Exports `FileStorer` interface, `Store` struct, `NewStore`, `Upload`, `Delete`, `PresignDownload`; contains `UsePathStyle` (MinIO guard) and `io.ErrUnexpectedEOF` handling (Pitfall 3 guard) |
+| `backend/internal/web/handlers_files.go` | FilesDeps + all handlers | VERIFIED | Exports `FilesDeps`, `TabloFilesTabHandler`, `TabloTasksTabHandler`, `FileUploadHandler`, `FileDownloadHandler`, `FileDeleteConfirmHandler`, `FileDeleteHandler`; no remaining 501 stubs |
+| `backend/templates/files.templ` | File UI components | VERIFIED | Contains `FilesTabFragment`, `FileUploadForm` (with `hx-encoding="multipart/form-data"`), `FileListRow` (with `/download` href and `/delete-confirm` hx-get), `FileDeleteConfirmFragment` (with `@ui.CSRFField`), `FileListEmpty`, `FileRowGone`, `UploadErrorFragment` |
+| `backend/templates/tablos.templ` | 3-tab layout | VERIFIED | `TabloDetailPage` has 6 parameters including `files []sqlc.TabloFile` and `activeTab string`; tab nav contains `hx-push-url` on all three tab links; `tab-content` div dispatches on `activeTab`; contains `TasksTabFragment` and `TabloOverviewTabFragment` |
+| `backend/internal/web/router.go` | File + tab routes wired | VERIFIED | `NewRouter` signature includes `fileDeps FilesDeps`; all 5 file routes registered; `TabloTasksTabHandler` registered at `/tablos/{id}/tasks` |
+| `backend/cmd/web/main.go` | S3 client + FilesDeps wired | VERIFIED | Reads `S3_ENDPOINT`, `S3_BUCKET`, `S3_ACCESS_KEY`, `S3_SECRET_KEY`, `S3_REGION`, `S3_USE_PATH_STYLE`, `MAX_UPLOAD_SIZE_MB`; constructs `files.NewStore`; sets `fileDeps.Files = nil` when endpoint unset (503 path) |
+| `backend/compose.yaml` | MinIO local dev service | VERIFIED | Contains `minio/minio:latest` service, `minio/mc:latest` init service with `restart: "no"`, `minio_data:` volume |
+| `backend/internal/web/handlers_files_test.go` | All 6 FILE requirement tests | VERIFIED | Contains `TestFileUpload`, `TestFileUploadTooLarge`, `TestFilesList`, `TestFilesTab`, `TestFileDownload`, `TestFileDownload_NonOwner`, `TestFileDelete`, `TestFileDelete_S3Failure`, `TestFileOwnership`; all implement behavior assertions (not skipped stubs) |
+
+---
+
+### Key Link Verification
+
+| From | To | Via | Status | Details |
+|------|----|-----|--------|---------|
+| `handlers_files.go FileUploadHandler` | `files/store.go Upload` | `deps.Files.Upload(r.Context(), s3Key, file)` | WIRED | Line 178: `contentType, bytesWritten, err := deps.Files.Upload(...)` |
+| `handlers_files.go FileDownloadHandler` | `files/store.go PresignDownload` | `deps.Files.PresignDownload(ctx, file.S3Key)` | WIRED | Line 262: `url, err := deps.Files.PresignDownload(r.Context(), file.S3Key)` |
+| `handlers_files.go FileDeleteHandler` | `files/store.go Delete` | `deps.Files.Delete(ctx, file.S3Key)` | WIRED | Line 312: `deps.Files.Delete(r.Context(), file.S3Key)` — before DB delete |
+| `templates/tablos.templ` | `templates/files.templ FilesTabFragment` | `@FilesTabFragment(tablo, files, csrfToken)` in `tab-content` div | WIRED | Line 232: `@FilesTabFragment(tablo, files, csrfToken)` when `activeTab == "files"` |
+| `cmd/web/main.go` | `files/store.go NewStore` | `files.NewStore(ctx, s3Endpoint, ...)` | WIRED | Line 105: `filesStore, fsErr = files.NewStore(...)` |
+| `router.go` | `handlers_files.go TabloFilesTabHandler` | route registration | WIRED | Line 109: `r.Get("/tablos/{id}/files", TabloFilesTabHandler(fileDeps))` |
+| `handlers_tablos.go` (both call sites) | `TabloDetailPage` 6-arg signature | updated call sites | WIRED | Lines 205, 311: both pass `nil, "overview"` as last two args |
+
+---
+
+### Data-Flow Trace (Level 4)
+
+| Artifact | Data Variable | Source | Produces Real Data | Status |
+|----------|---------------|--------|--------------------|--------|
+| `FilesTabFragment` | `files []sqlc.TabloFile` | `deps.Queries.ListFilesByTablo(r.Context(), tablo.ID)` in `TabloFilesTabHandler` | Yes — real DB query | FLOWING |
+| `FileListRow` | `file sqlc.TabloFile` | passed from `FilesTabFragment` range loop | Yes — from DB query above | FLOWING |
+| `FileUploadForm` | `uploadErr string` | passed from `UploadErrorFragment` or `""` on success path | Yes — real error message or empty | FLOWING |
+| `FileDownloadHandler` | `url string` | `deps.Files.PresignDownload` → `s3.NewPresignClient.PresignGetObject` | Yes — generated server-side per request | FLOWING |
+
+---
+
+### Behavioral Spot-Checks
+
+Note: Integration tests (`TestFileUpload`, `TestFilesList`, etc.) require `TEST_DATABASE_URL` and skip in this environment without a live database. The following checks are against code correctness only.
+
+| Behavior | Check | Result | Status |
+|----------|-------|--------|--------|
+| `go build ./...` exits 0 | `go build ./...` from `backend/` | Exit 0, no output | PASS |
+| Full test suite exits 0 (all non-integration tests pass) | `go test ./... -count=1 -timeout 60s` | All 8 packages pass (integration tests skip, none fail) | PASS |
+| No 501 stubs remain in handlers_files.go | `grep "501" handlers_files.go` | No matches | PASS |
+| FileStorer interface compile-time assertion | `TestStoreImplementsFileStorer` | PASS | PASS |
+| S3-first delete ordering | `deps.Files.Delete` at line 312 before `deps.Queries.DeleteTabloFile` at line 317 | Correct ordering confirmed | PASS |
+| `MaxBytesReader` before `ParseMultipartForm` | Lines 147 then 149 | Correct ordering confirmed | PASS |
+
+---
+
+### Requirements Coverage
+
+| Requirement | Source Plan | Description | Status | Evidence |
+|-------------|-------------|-------------|--------|----------|
+| FILE-01 | 05-01, 05-02 | Upload file with size limit; metadata in Postgres, bytes in S3 | SATISFIED | `FileUploadHandler` + `InsertTabloFile` + `deps.Files.Upload`; `TestFileUpload` asserts DB row and S3 key |
+| FILE-02 | 05-01, 05-02 | Server-proxied upload (not direct-to-S3 presigned PUT) | SATISFIED | `FileUploadHandler` streams multipart to `deps.Files.Upload` (PutObject); no presigned PUT path; D-01 design decision |
+| FILE-03 | 05-02 | List files with original filename and size | SATISFIED | `ListFilesByTablo` query + `FileListRow` rendering `file.Filename`, `formatBytes(file.SizeBytes)`, `file.CreatedAt`; `TestFilesList` asserts filename and "KB" size display |
+| FILE-04 | 05-03 | Download via signed time-limited URL | SATISFIED | `FileDownloadHandler` → `PresignDownload` with 5-minute TTL → 302 redirect; `TestFileDownload` asserts 302 + signed Location |
+| FILE-05 | 05-03 | Delete removes DB row and S3 object | SATISFIED | `FileDeleteHandler` calls S3 Delete first, always proceeds to DB delete; `TestFileDelete` verifies DB row gone; `TestFileDelete_S3Failure` verifies DB delete proceeds even on S3 failure |
+| FILE-06 | 05-02, 05-03 | Authorization: only tablo owner can operate on files | SATISFIED | `loadOwnedTablo` / `loadOwnedTabloForFile` called as first step in all handlers; returns 404 for non-owners; `TestFileDownload_NonOwner` and `TestFileOwnership` assert 404 |
+
+---
+
+### Anti-Patterns Found
+
+No blockers or warnings found:
+
+- No `TBD`, `FIXME`, or `XXX` markers in any phase 5 files
+- No remaining 501 stubs — all were replaced in Plan 03
+- No empty handlers (`return null`, `return {}`)
+- `placeholder` references in `tablos.templ` are HTML form placeholder attributes (not code stubs)
+- `FilesDeps.Files = nil` in main.go when S3_ENDPOINT is unset is an intentional design decision (handlers return 503; server starts without S3 for non-file routes in dev)
+
+---
+
+### Human Verification Required
+
+#### 1. Tab Navigation URL Sync
+
+**Test:** Log into the Go server (just dev), open a tablo detail page, and click through the three tabs: Overview, Tasks, Files.
+**Expected:** Browser URL bar updates to `/tablos/{id}`, `/tablos/{id}/tasks`, `/tablos/{id}/files` respectively on each click. Browser Back restores the previous tab's URL and content.
+**Why human:** `hx-push-url` attribute is present in the template (verified in code) but push-state behavior requires a live browser; cannot assert URL-bar changes programmatically.
+
+#### 2. File Upload End-to-End with MinIO
+
+**Test:** With MinIO running via `docker compose up -d`, upload a small file (PDF, image, or text) using the upload form on the Files tab.
+**Expected:** File appears immediately in the list with correct filename, human-readable size (e.g. "42.5 KB"), and upload date. File object is present in the MinIO `xtablo-dev` bucket.
+**Why human:** Unit tests use a stub FileStorer and skip when `TEST_DATABASE_URL` is absent. Live S3 integration requires running MinIO.
+
+#### 3. Download via Presigned URL
+
+**Test:** Click the Download link for an uploaded file.
+**Expected:** Browser follows the 302 redirect to a presigned MinIO URL and delivers the file content. URL contains a signature and expires parameter.
+**Why human:** End-to-end delivery through MinIO requires a live service; stub returns `"https://example.com/signed?key=foo"` in tests.
+
+#### 4. Delete Confirmation and Row Removal
+
+**Test:** Click Delete on a file row, verify inline confirmation appears (filename, "Yes, delete", "Keep file"). Click "Yes, delete". Verify row disappears. Refresh page — row must still be absent.
+**Expected:** Inline HTMX confirm swap, then FileRowGone OOB removal, then full-page refresh confirms deletion persists.
+**Why human:** Requires browser to observe HTMX DOM swap and navigation.
+
+#### 5. Oversize Upload Friendly Error
+
+**Test:** Upload a file larger than 25 MB (or set `MAX_UPLOAD_SIZE_MB=1` and upload a 2 MB file).
+**Expected:** Response is 422 with a red error message above the upload form reading "File too large (max N MB)." — not a blank page, 500, or hang.
+**Why human:** `TestFileUploadTooLarge` correctly implements this assertion but skips without a live DB. Developer should confirm with `TEST_DATABASE_URL` set or in browser.
+
+#### 6. Authorization Enforcement
+
+**Test:** Sign up a second user account. Try to navigate to the first user's tablo files URL: `http://localhost:8080/tablos/{first_user_tablo_id}/files`.
+**Expected:** 404 page — "This tablo doesn't exist or you don't have access."
+**Why human:** `TestFileOwnership` is fully implemented but skips in CI. Browser test with two user accounts needed for real-world confidence.
+
+---
+
+### Gaps Summary
+
+No gaps found. All six ROADMAP success criteria are satisfied by the codebase evidence. All required artifacts exist, are substantive, and are wired. The six FILE-01..06 requirements are fully implemented.
+
+The `human_needed` status is set because:
+1. Integration tests skip in CI (no `TEST_DATABASE_URL` in this environment) — tests are correctly written but could not be run against a live database.
+2. Browser behaviors (hx-push-url URL sync, MinIO redirect delivery, HTMX DOM swap) require a live session to confirm.
+
+These are runtime verification items, not code defects. The implementation is complete and correct per code inspection.
+
+---
+
+_Verified: 2026-05-15T00:00:00Z_
+_Verifier: Claude (gsd-verifier)_