v5 data platform, Phases 3 and 4: writes on Postgres, accounts on database sessions - #35
Conversation
Phase 3 moves the last writes off Notion. Tickets land in maintenance_logs, corrections in feedback, project submissions in projects and project_tools. POST /api/uploads replaces the Notion upload with Vercel Blob plus an attachments row, and /api/cron/daily does the backup, the blob retention prune and the orphaned-attachment sweep. The Phase 2 notion-ids bridge has no callers left. Phase 4 gives the app real accounts. Better Auth moves onto the Drizzle adapter with database sessions and the admin plugin; the stateless makerlab.identity cookie is gone. Roles live in the users table instead of the AUTH_STAFF_EMAILS and AUTH_ADMIN_EMAILS env lists, with AUTH_SUPER_ADMIN_EMAILS kept as the bootstrap floor. /admin/users changes a role, audit_events records that it happened, and submitting a project now requires sign-in while browsing stays anonymous. All of it runs with no credentials: PGlite serves the writes, an unset blob token makes uploads refuse rather than pretend, and an unset CRON_SECRET makes the cron route refuse. 96 files, 1331 unit tests, 49 E2E on a production build, spec:coverage 0 undocumented. Three major review findings are outstanding and fixed in the next commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjvfabH9CRceoC9GfjjSvE
Three reviews returned nine findings on this branch. Four of them described code that was already here — the review had read an earlier working state and the fixes had been amended into 1c967c4 itself, whose message still claims they are outstanding. Nothing to do for those but say so, which the spec amendment now does, along with the warning that every line number in those reports is off by the size of the fixes they missed. Four held up. The super-admin floor now beats a ban, not just a demotion. identityFromSession read the floor after the ban check, so a listed address whose row was banned resolved anonymous: a recovery for exactly half of what the floor promises. It is read first now. It cannot rescue a sign-in — the admin plugin throws BANNED_USER from a hook that runs ahead of anything this app can register — so reconcileSuperAdminFloor also lifts the ban off the row, and the first admin write a recovered director performs makes ordinary sign-in work again. This contradicts one sentence in spec 3.4 and the amendment flags it as the lab's call rather than settling it quietly. An audit write that fails after the change committed is now a warning on a success. It was awaited unguarded after auth.api.setRole had returned, so a transient failure threw and the island restored the old role over a database holding the new one. AdminActionResult gains warning on its ok variant, and both islands keep the new value and say what was not recorded. A change that landed minus a guarantee is never ok: false — a refusal is what the islands answer by rolling back. POST /api/projects reports photosSubmitted and photosAttached, and the form says so on the confirmation. A form left open overnight submits ids the cron has already swept, and thanking a student for pictures nobody has is the quiet lie Article 4 forbids. Partial loss counts, unlike the maintenance path. /projects/new tells three identity states apart instead of two. A failed /api/identity fetch was read as "not signed in", so a 429 replaced the whole form with a sign-in wall; anonymous comes back as a 200 with a role, so null means only "could not ask". Unavailable keeps the form up with a notice and a retry, and the server stays the authority. Documentation the three passes deferred: the spec amendment entry, and AGENTS.md on the floor's ban exception, floor-role.ts, admin_api_not_exposed and the warning channel Phase 5 should reuse. Gate observed green with every environment variable unset: lint 0 errors (3 pre-existing warnings), typecheck clean, vitest 97 files / 1356 tests, playwright 49 passed, spec:coverage 73 items / 0 undocumented, build succeeds with no database. Each code fix confirmed red with the fix reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjvfabH9CRceoC9GfjjSvE
Adversarial verification of 3c76839 found the guard it had just added to actions.ts missing one function away. reconcileSuperAdminFloor commits its row UPDATE and then writes two audit events unguarded, so an unreachable audit table threw past a committed change: authorize() caught it and returned "failed", which the page renders as "That did not save. Nothing was changed" — over a row that had just been promoted and, since 3c76839, un-banned. A ban lifted with no trail and the UI saying nothing happened. The function now reports {changed, audited} instead of a bare boolean, and guards its audit writes the way actions.ts already guards its own. The gap rides back through authorize() as the existing audit_unavailable warning, so both halves of a two-write action answer the admin's one question: did the trail record this. A success with a gap keeps the value the database holds; only the row UPDATE itself still throws. Two smaller things the same pass turned up: An upload refused for want of a session showed the route's English to a reader in another language. /projects/new tells three identity states apart now, so picking a photo while signed out is a routine path rather than an edge case; the 401 gets signInRequiredError, as submit already did. A test comment claimed the permission check "refuses before reading the file, so the refusal costs nothing". req.formData() has already parsed the body by then. The assertion was right and the explanation was not. Verified with every credential unset: lint 0 errors (3 pre-existing warnings), typecheck clean, 97 files / 1358 tests, spec:coverage 73 items / 0 undocumented. The two new regression tests were confirmed red against the unguarded version, failing with exactly {ok: false, error: "failed"}. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjvfabH9CRceoC9GfjjSvE
The floor beats a ban, decided rather than left open: the alternative makes the documented recovery a dead end, and the cost is already held by anyone who can deploy. The one case the environment variable cannot rescue — a ban applied before the address joined the list, whose sessions are already gone — is written down rather than fixed. Phase 6 keeps the Workflow SDK. eve was considered and rejected: it consumes the same SDK rather than replacing it, and it wants ai@7, Node 24 and an HTTP target for its evals, none of which this app can give it. The phase is sized for Hobby's 300s step ceiling — four searches and four fetches, two steps, a 240s abort, 25 items a batch — and carries the two traps found while checking: mapWithConcurrency diverges on replay, and vi.mock cannot reach step code. Phase 7 leaves the plan (Isaac and Luis, not code) and Phase 9 waits until the app is otherwise good. Phase 8 stays, and still needs its two open questions answered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjvfabH9CRceoC9GfjjSvE
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57d7ea3e71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "label": "Demo data", | ||
| "body": "This catalogue is built-in sample data, not {institution}'s real inventory. Notion is not configured." | ||
| }, | ||
| "admin": { |
There was a problem hiding this comment.
The new admin namespace, the header's admin label, and most new project-form messages exist only in en.json; the other 11 locale files add only the upload-unavailable messages. Consequently, non-English users receive English fallback text throughout the new admin and sign-in flows, and existing translations such as Spanish projectForm.requiredError still incorrectly require a name even though that field was removed. Update all locale files with the complete set of changed keys.
AGENTS.md reference: AGENTS.md:L84-L84
Useful? React with 👍 / 👎.
| if (target.role === "super_admin") { | ||
| const remaining = await countUsersWithRole("super_admin", { | ||
| excludeUserId: target.id, | ||
| }); | ||
| if (remaining === 0) return "last_super_admin"; |
There was a problem hiding this comment.
In a deployment without a configured floor and with exactly two unbanned super admins, concurrent demotions can both observe the other account in this count and then both commit through auth.api.setRole, leaving no super admin able to reverse the changes. The count and role update need a shared lock or another atomic database invariant rather than separate read and write operations.
AGENTS.md reference: v5/AGENTS.md:L176-L179
Useful? React with 👍 / 👎.
| const pathnames = orphans.map((row) => row.blobPathname); | ||
| await store.del(pathnames); | ||
|
|
||
| const rowsDeleted = await deleteAttachments( | ||
| orphans.map((row) => row.id), |
There was a problem hiding this comment.
When a submission claims an attachment after listOrphanedAttachments returns but before this Blob deletion completes, cleanup still deletes its bytes and then deletes its now-owned row because deleteAttachments has no owner_id IS NULL guard. This race is particularly plausible for a form left open past the 24-hour cutoff and makes the submission report the photo as attached even though cleanup has destroyed it; claiming and cleanup must coordinate atomically.
AGENTS.md reference: v5/AGENTS.md:L65-L68
Useful? React with 👍 / 👎.
| // upload route moves to Blob this is the normal case, because the ids in | ||
| // the hint are still Notion file_upload ids and no `attachments` row | ||
| // answers to them. | ||
| const photosLost = photoIds.length > 0 && record.photosAttached === 0; |
There was a problem hiding this comment.
When multiple photo IDs are supplied and only some remain claimable—for example, one upload expired while another did not—photosAttached is nonzero, so photosLost is false and the assistant reports an unqualified success. Compare the claimed count with the number of submitted distinct valid IDs so the student is warned whenever any requested photo did not attach.
AGENTS.md reference: v5/AGENTS.md:L68-L73
Useful? React with 👍 / 👎.
Phases 3 and 4 of the v5 data platform spec (§9). The last writes leave Notion, and the app gets real accounts.
Production is unaffected — it is pinned to
isam-frozenwhile this lands. Merging this deploys to themainpreview only.Phase 3 — the writes
maintenance_logs,feedback,projects+project_tools), withcreated_by/updated_bystamped and cache tags invalidated on write. The Notion retry-without-reporter_emailworkaround and the raw Notionfetchin the flags capability are gone.POST /api/uploadsreplaces the Notion upload: Vercel Blob plus anattachmentsrow.file_upload_idbecomesattachmentId, a Postgres uuid, which ripples through the chat photo hint,ChatFabandProjectSubmitForm./api/cron/dailydoes the Postgres export, the blob retention prune and the orphaned-attachment sweep.src/lib/data/notion-ids.tshas no callers left.Phase 4 — the accounts
makerlab.identitycookie is gone.userstable, not inAUTH_STAFF_EMAILS/AUTH_ADMIN_EMAILS, which are retired.AUTH_SUPER_ADMIN_EMAILSstays as the bootstrap floor.can()and the access-control declaration,rolein/api/identity,requiredPermissionon capabilities, replacing PR Pre-ISAM fixes: photos reach the assistant, staff-only intake, dead button removed #31's ad-hocminimumRole./admin/users,audit_events, and sign-in required to submit a project. Browsing stays anonymous.Three security findings, found by review and closed by verification
Three independent reviewers read the diff; two adversarial verifiers then tried to reproduce each finding against the fixed code rather than trusting the fix reports.
/api/auth/admin/set-rolereached every write/admin/usersperforms, with no audit event and none of the guards. Now refused at the catch-all withadmin_api_not_exposed. Verified across 14 path spellings — encoded, cased, traversal, trailing-slash — each asserting the target's stored row was untouched./api/uploadshanded anonymous callers permanent public Blob URLs.curl -F kind=resourcewas arbitrary file hosting on the deployment's blob host. Public kinds now each require a permission;chatandmaintenancestay anonymous and private, so a visitor with no account can still send a photo and file a ticket. Verified end to end with no cookie.can(), so a floor director got an opaquefailedon every write — the lock-out the floor exists to undo.reconcileSuperAdminFloornow writes the floor onto the row insideauthorize().One bug the verification itself found
reconcileSuperAdminFloorcommitted its rowUPDATEand then wrote audit events unguarded, so an unreachableaudit_eventsmadesetUserRoleanswer "That did not save. Nothing was changed" over a row that had just been promoted and un-banned. Fixed inbb8c6ae; both regression tests confirmed red against the unguarded version.Decisions recorded in the amendment
super_adminand its ban is lifted on the first admin write. The alternative made the documented recovery a dead end. One narrow case the env var cannot rescue is documented rather than fixed.ai@7, Node 24 and an HTTP eval target.Verification
Every command below run with
DATABASE_URL,AUTH_SECRET,GOOGLE_*,BLOB_READ_WRITE_TOKEN,CRON_SECRET,AUTH_SUPER_ADMIN_EMAILSand the Notion variables all unset:lint— 0 errors, 3 pre-existing warningstypecheck— cleanvitest— 97 files, 1358 testsplaywright— 49 passed on a production buildspec:coverage— 73 items, 0 undocumentedbuild— succeeds with no databaseNeeds a person before this reaches production
DATABASE_URL(Production and Preview). Without it the app serves the PGlite demo seed and says so.BLOB_READ_WRITE_TOKEN. Uploads refuse honestly until then.GOOGLE_CLIENT_ID/_SECRET, plusAUTH_SECRETandAUTH_SUPER_ADMIN_EMAILS=ies22@cornell.edu.npm run import:notion, thennpm run verify-import.AUTH_STAFF_EMAILSandAUTH_ADMIN_EMAILSare no longer read. Bootstrap with the super-admin floor, sign in, promote everyone else from/admin/users.Residual risks, accepted not fixed
A lost audit event survives only as a
console.error— no retry, no outbox.report_issuereports photo loss only when every photo is lost, where projects now reports partial loss.POST /api/uploadsparses the body before checking the permission, so an anonymous caller can make the server read 18 MB before its 401.How it was built
Two workflows: eight agents for the build, then five for the review fixes and adversarial verification, with the last bug and the amendment done by hand.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AjvfabH9CRceoC9GfjjSvE