From 7fb31566382b82965d5c0b14138dd5f16adf0a94 Mon Sep 17 00:00:00 2001 From: Arthur Belleville Date: Fri, 15 May 2026 12:46:02 +0200 Subject: [PATCH] test(05): add phase verification report --- .planning/phases/05-files/05-VERIFICATION.md | 189 +++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 .planning/phases/05-files/05-VERIFICATION.md diff --git a/.planning/phases/05-files/05-VERIFICATION.md b/.planning/phases/05-files/05-VERIFICATION.md new file mode 100644 index 0000000..9fcfd07 --- /dev/null +++ b/.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)_