feat: Slice 2 (Module 02) — document delete with atomic cascade

[mGasiorek998]

Summary

• DELETE /documents/:id for the owning patient: 204 + DB row gone + shares gone + file unlinked
• DB removal happens inside a single Drizzle transaction (shares first, then document); FK ON DELETE CASCADE provides defense-in-depth
• Disk-side failure after a successful txn returns 500 DELETE_FAILED with the contractually-required body; the committed txn remains the source of truth
• 403 for non-owners, 404 for unknown ids, 401 without JWT, 403 with a doctor JWT
• New useDeleteDocument mutation hook + delete button on the patient documents list

Closes #10

Note: This PR is stacked on top of #15 (Slice 1) because the documents foundation hasn't merged to main yet. Re-base/re-target to main once #15 lands.

Test plan

• pnpm test — 75/75 pass (7 new integration tests for delete: 204 happy path, cascade with shares, 500 disk-fail, 403 ownership, 404 unknown, 401, doctor 403)
• pnpm typecheck clean
• pnpm lint clean
• Manual UI: log in as a patient, upload a doc, click Delete → row disappears
• Manual UI: with the API stopped or after manually deleting the storage file, the delete-error banner shows on Delete

🤖 Generated with Claude Code

[claude]

Claude finished @mGasiorek998's task in 2m 16s —— View job

---

Review — Slice 2 (Module 02): document delete with atomic cascade

• Read issue Slice 2 (Module 02): Delete document with atomic cascade #10, ARCHITECTURE_RULES.md, GLOSSARY.md, module-02 spec
• Inspect PR diff (8 files, +394/-5)
• Spec compliance against acceptance criteria
• Architecture / boundary checks
• Security review
• Test quality review
• Vertical slice end-to-end check

---

Verdict: ✅ APPROVE

Solid slice. Use-case logic is small and correct, the DB removal is genuinely atomic via Drizzle's db.transaction, the disk-failure path matches EH-002 exactly (committed txn + 500 with the contract-required body), the contract was added in packages/contracts, and the UI wires the feature folder's mutation hook through apiClient cleanly. Tests are real-DB integration, exercise external behavior (status codes, DB rows, FS state), and one of them even seeds a document_shares row to prove the cascade works for shared documents (AC-005 / AC-006 from the module spec).

Spec compliance (issue #10)

• ✅ AC1 — DELETE /documents/:id by owner returns 204; row + shares + UUID file all gone (documents.test.ts:322-362).
• ✅ AC2 — DB removal in a single txn: deleteDocumentWithSharesById wraps both deletes in db.transaction (document-repository.ts:52-62). Schema also has ON DELETE CASCADE on document_shares.document_id as defense-in-depth (db/schema.ts:58).
• ✅ AC3 — Cascade with shares: documents.test.ts:364-409 inserts a real share, deletes the doc, asserts both rows are gone.
• ✅ AC4 — 500 DELETE_FAILED with the exact contract body when disk delete fails (documents-router.ts:108-115, test at documents.test.ts:411-451). Importantly, the test verifies the DB row is still gone — txn is the source of truth as the spec requires.
• ✅ AC5 — 403 for non-owner (documents.test.ts:453-497).
• ✅ AC6 — 404 DOCUMENT_NOT_FOUND for unknown id (documents.test.ts:499-511).
• ✅ AC7 — Delete button removes entry from list: useDeleteDocument invalidates ['documents'] on success (use-delete-document.ts:28-30); button wired in DocumentList.tsx:50-59 with a deletingId disable state.

Architecture

• ✅ Layer flow respected: route → use-case → infrastructure. No skipping.
• ✅ Contract is in packages/contracts/src/documents.ts (documentDeleteFailedSchema is a discriminated success/failure body).
• ✅ Feature folder owns its mutation hook; UI goes through apiClient rather than direct fetch.
• ✅ Named exports, kebab-case filenames, PascalCase components, function components only.
• No any, no console.log, no sync I/O — all good.

Security

• ✅ Auth (requireAuth) and role guard (requireRole(['patient'])) applied to the DELETE route (documents-router.ts:87-88). Note that the existing router.use('/documents', …) middleware only matches /documents exactly in Hono, not /documents/:id, so the explicit middleware on the DELETE handler is required — that's correct here, just worth flagging since the duplication looks suspicious at a glance.
• ✅ Ownership check happens in the use case before any mutation (delete-document.ts:21-23).
• ✅ Storage filename comes from the DB row, never client input — no path-traversal risk.
• ✅ 401 / 403 for the doctor-JWT and missing-JWT cases verified by tests.
• The 403-vs-404 distinction is intentional per AC5/AC6 (issue Slice 2 (Module 02): Delete document with atomic cascade #10), so the resulting "exists but not yours" vs "doesn't exist" oracle is a deliberate spec choice, not a leak we need to fix here.

Test quality

• Real DB, real filesystem, no mocks — matches the testing rule in ARCHITECTURE_RULES.md.
• Tests assert behavior at the HTTP boundary (status, body) and verify durable state (DB rows, files on disk). No tautologies.
• Cleanup uses fixed test emails and afterAll; not parallel-safe, but that's a pre-existing pattern in this file.

Vertical slice

Schema (from Slice 1) → use-case (new) → router (new) → contract (new) → UI hook + button (new) → integration tests (new). End-to-end value delivered. ✅

Suggestions (non-blocking)

1. TOCTOU on concurrent delete. Two concurrent DELETEs of the same doc currently both pass the SELECT, both hit the txn (the second is a no-op delete), and the loser's disk unlink fails with ENOENT → spurious 500 DELETE_FAILED. Could be tightened by using DELETE ... RETURNING on documents as the existence/ownership probe, then only attempting unlink if a row was actually deleted. Edge case for MVP — flagging for future hardening, not for this PR. Fix this →

2. UI surfaces only the error code, not message. When the API returns 500 { error: 'DELETE_FAILED', message: 'Document metadata removed but file could not be deleted from storage' }, the banner displays just DELETE_FAILED. The richer message is exactly what the spec says is "for operators" — a small extractor change in use-delete-document.ts:4-14 would surface it.

3. isDeleting ? deletingId ?? null : null. Works, but useMutation.variables retains the last value past success. isPending ? variables : null reads cleaner, which is what the current code does — just noting the ?? null is redundant since variables is string | undefined and the surrounding ternary already gates on pending state.

4. Test isolation. Fixed emails (doc.patient@example.com, etc.) and a shared tmpDir mean these can't run in parallel against the same DB, and a crashed run leaves rows behind that the next run cleans up. Pre-existing pattern from Slice 1 — fine to leave, but a randomized email suffix per beforeAll would make CI more robust later.

5. Stacked-PR note. The PR body flags this is stacked on feat: Slice 1 (Module 02) — Document upload & list foundation #15. Once feat: Slice 1 (Module 02) — Document upload & list foundation #15 lands, re-target to main before merge so the diff isn't muddied.
   · Branch afk/issue-10