mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-07 06:25:16 +02:00
Devin Foley
8316ceb0b9
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - A self-hosted install in `authenticated` mode signs users in with Better Auth, mounted at `/api/auth` over a hand-written Drizzle `account` table in `packages/db` > - Better Auth 1.7.0 added a required `issuer` field to that `account` model, plus a unique index on `(issuer, accountId)` > - The dependency bump in #11886 changed only `server/package.json` and the lockfile, so the Drizzle table never grew the column > - The Drizzle adapter checks the model against the schema on every write, so `linkAccount` throws and sign-up answers 500 with an empty body; a fresh install cannot create its first user, and an upgraded install locks out every existing user > - This pull request adds the `issuer` column and its unique index, and migrates the column in with a backfill that covers every existing row > - The benefit is that sign-up and sign-in work again, on a new install and after an upgrade ## Linked Issues or Issue Description No existing issue. Describing it inline, following `.github/ISSUE_TEMPLATE/bug_report.yml`. Refs #11886 (the dependency bump that introduced the required field). Refs #12269 (an earlier attempt at this fix; its backfill covers only `provider_id = 'credential'`). **What happened?** Sign-up fails on a self-hosted install. `POST /api/auth/sign-up/email` answers HTTP 500 with a zero-byte body. The server log carries: ``` [Better Auth]: The field "issuer" does not exist in the "account" Drizzle schema. # SERVER_ERROR: [BetterAuthError: The field "issuer" does not exist in the "account" Drizzle schema.] ``` The request writes the `user` row and then fails on the `account` row. The address is stuck after that: a second sign-up answers 422 `USER_ALREADY_EXISTS`, sign-in answers 401, and password reset answers 400 `RESET_PASSWORD_DISABLED` because the account that would hold the password does not exist. An upgraded install is worse. `sign-in/email` matches the credential account on `account.issuer === 'local:credential'`. Rows written before the upgrade have no issuer, so every existing user is locked out. **Expected behavior** `POST /api/auth/sign-up/email` answers 2xx and writes both the `user` row and its credential `account` row. `POST /api/auth/sign-in/email` then answers 2xx and sets a session cookie. An install that upgrades keeps its existing users. **Steps to reproduce** 1. Start a server from `master` with `PAPERCLIP_DEPLOYMENT_MODE=authenticated` against an empty database. 2. `curl -X POST http://127.0.0.1:<port>/api/auth/sign-up/email -H 'Content-Type: application/json' -H 'Origin: http://127.0.0.1:<port>' --data '{"name":"A","email":"a@example.com","password":"a-long-password"}'` 3. The response is HTTP 500 with an empty body. **Paperclip version or commit** `master` at4436cf0. The defect starts at69e8585(#11886), which moved Better Auth from 1.6.28 to 1.7.0. **Deployment mode** `authenticated`. `local_trusted` does not sign users in, so it is not affected. Hosted tenants are not affected either: that path resolves the actor from a trusted header and never reads `account`. **Database mode** Both. Embedded PostgreSQL and external PostgreSQL use the same Drizzle schema. **Relevant logs or output** Reproduced in a test by reverting the schema change: ``` stderr | better-auth-credential-signup.integration.test.ts [Better Auth]: The field "issuer" does not exist in the "account" Drizzle schema. AssertionError: expected 500 to be 200 ``` ## What Changed - `packages/db/src/schema/auth.ts`: adds `issuer` (text, NOT NULL) to `authAccounts`, and the `(issuer, account_id)` unique index that mirrors the index Better Auth declares on the model. The field name, type, requiredness, and index all come from `@better-auth/core/dist/db/get-tables.mjs` in 1.7.0. - `packages/db/src/migrations/0230_better_auth_account_issuer.sql`: adds the column, backfills every existing row, sets NOT NULL, and creates the unique index. - `packages/db/src/migrations/meta/0230_snapshot.json` and `_journal.json`: regenerated with `pnpm --filter @paperclipai/db generate`. - `packages/db/src/better-auth-account-issuer-migration.test.ts`: new. Asserts the schema shape, then rewinds the migration on a real database, seeds pre-upgrade rows, and re-applies it. - `server/src/__tests__/better-auth-credential-signup.integration.test.ts`: new. Real sign-up and sign-in through the Better Auth mount, against the real Drizzle schema and a migrated PostgreSQL. - `cli/src/__tests__/worktree.test.ts`: the worktree seed fixture writes a credential `account` row, so it now writes `issuer` too. `server/package.json` and `pnpm-lock.yaml` are untouched. The dependency is correct; the schema was what was missing. ### The issuer values, and where they come from Better Auth builds these itself, in `@better-auth/core/src/db/schema/account.ts`: ```ts export function createLocalAccountIssuer(providerId: string): string { return `local:${encodeURIComponent(providerId)}`; } export function createOAuthAccountIssuer(providerId: string): string { return `local:oauth:${encodeURIComponent(providerId)}`; } ``` Sign-up and sign-in both call `createLocalAccountIssuer("credential")`, so a credential account is `local:credential`. An OAuth account whose provider declares no `accountIssuer` of its own is `local:oauth:<providerId>` — no built-in social provider declares one. The migration writes exactly those two forms: ```sql ALTER TABLE "account" ADD COLUMN IF NOT EXISTS "issuer" text; UPDATE "account" SET "issuer" = CASE WHEN "provider_id" = 'credential' THEN 'local:credential' ELSE 'local:oauth:' || "provider_id" END WHERE "issuer" IS NULL; ALTER TABLE "account" ALTER COLUMN "issuer" SET NOT NULL; CREATE UNIQUE INDEX IF NOT EXISTS "account_issuer_account_id_uq" ON "account" USING btree ("issuer","account_id"); ``` Two limits are worth stating plainly. The OAuth branch reproduces `createOAuthAccountIssuer` for provider ids that need no percent-encoding, which covers every built-in provider id; a provider id with a character `encodeURIComponent` would escape would get a slightly different string. And a generic-OAuth provider that sets `accountIssuer` explicitly (Okta, Auth0, Keycloak, Slack, Line) uses the real issuer URL, which this migration cannot know. Neither case can arise on Paperclip today: `createBetterAuthInstance` configures `emailAndPassword` only and registers no social or generic-OAuth provider, so every existing row is a credential row. The OAuth branch is there so the backfill stays total rather than leaving a NULL that aborts `SET NOT NULL`. ## Verification - `pnpm --filter @paperclipai/db check:migrations` — passes. - `pnpm --filter @paperclipai/db typecheck` — passes. - `packages/db` suite: 30 files, 107 tests, all pass. - `npx tsc --noEmit` in `server/` — no error in any changed file. (The wrapped `pnpm typecheck` builds the runner vendor first, which needs cargo; that toolchain was not available here, so the pre-existing "cannot find module" errors from the unbuilt workspace packages remain in the bare run.) - `node --test scripts/__tests__/run-vitest-stable-shard.test.mjs` — passes with the new server suite in the file list. - The two new tests were confirmed to fail without the fix: - Reverting `packages/db/src/schema/auth.ts` to its `master` content makes the server test fail with the reported error and `expected 500 to be 200`. - Narrowing the backfill to `WHERE "issuer" IS NULL AND "provider_id" = 'credential'` makes the migration test fail with `column "issuer" of relation "account" contains null values` — the failure mode of #12269. - End to end against a server built from this branch, started with `PAPERCLIP_DEPLOYMENT_MODE=authenticated` on embedded PostgreSQL: - `POST /api/auth/sign-up/email` → 200 with a user and token. - `POST /api/auth/sign-in/email` → 200 with a session cookie. - `GET /api/auth/get-session` → 200 with the session. - The stored row is `issuer = 'local:credential'`, `provider_id = 'credential'`, `account_id = user_id`, and `pg_indexes` lists `account_issuer_account_id_uq`. - `scripts/docker-onboard-smoke.sh` was not used as proof: it installs `paperclipai` from npm inside the container, so it exercises a published release rather than this branch. ## Risks - **Migration.** The migration backfills every existing row before `SET NOT NULL`, so an install that upgrades keeps working and its users keep signing in. `account` is one row per user per provider, so the full-table `UPDATE` and the index build are cheap; `packages/db/src/table-size-estimates.ts` already classes `account` as small, and `check:migrations` passes with no new safety finding. - **New unique index.** `(issuer, account_id)` is the key Better Auth resolves accounts by, so a duplicate would already be a defect. Better Auth writes one credential account per user keyed on the user id, so the pair is unique by construction. An install that somehow holds a duplicate would fail the index build rather than corrupt anything, and the migration is a single transaction. - **Orphaned users are not repaired.** An address that hit the broken window has a `user` row and no `account` row. This migration does not delete or repair those rows, so that address stays unusable after the upgrade: sign-up says the user exists, and there is no credential account to sign in as or reset. Only installs that ran a build containing #11886 are affected, and the repair — deleting the orphaned `user` rows — is a judgment call about live data that does not belong in an automatic migration. - **Not a behavior change anywhere else.** Only the `account` table changes. Hosted tenants resolve their actor from a trusted header and never read it. ## Model Used Claude (Anthropic), Claude Opus, 1M context, extended thinking, agentic tool use via Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge