Fix deriveAuthLists shadowing an FK column for non-Id-suffixed reference names by borisno2 · Pull Request #1549 · OpenSaasAU/stack

borisno2 · 2026-09-13T11:53:55Z

Summary

deriveAuthLists (packages/auth/src/config/derive-auth-lists.ts) derived a relationship() field for every id-referencing better-auth column, naming the relation by stripping a trailing Id from the upstream field key. When the upstream key does not end in Id, stripping is a no-op, so the relation's own field name ends up identical to the exact physical column name its foreign key maps onto (db.foreignKey.map) — a self-collision the contract derivation already refuses at generate time (A to-one relation and its foreign key collide on one alias, breaking nested to-one includes #1236: "rc.8 aliases an include by relation name and a scalar by physical column name, so the two would collide").
better-auth's own four base models never hit this (every default reference name ends in Id), but a better-auth plugin schema can declare a reference field named otherwise (e.g. nullableReference), which is exactly what better-auth's own normal conformance suite's "should return null for nullable foreign keys" test exercises via an ad-hoc plugin schema.
Fix: when stripping Id from the upstream field key is a no-op, fall back to deriving a plain scalar column instead of a relation — the same fallback the code already uses for a reference whose target field isn't the target's id (issue auth: consolidate the two better-auth→list converters, and give plugin tables real foreign keys #992). This keeps the derived field's contract name identical to better-auth's own canonical field key, which is what the stack-authored Auth adapter's create/find/update round-trip relies on.
Un-skipped the previously-failing 'create - should return null for nullable foreign keys' entry in packages/auth/tests/adapter-conformance.test.ts's NOT_IMPLEMENTED map, and updated the doc comments in that file and in packages/auth/CLAUDE.md accordingly.

Test plan

create - should return null for nullable foreign keys (both the normal and uuid suite variants) now pass un-skipped
auth-lists-drift.test.ts still passes
Full packages/auth test suite passes (511 passed, 79 skipped — the remaining skips are documented and unrelated: join/numberId/generateId/issuer-scoped-key)
pnpm build, tsc --noEmit, pnpm lint, pnpm format (repo-wide) all clean
Added a changeset (@opensaas/stack-auth, patch)

Closes #1222

🤖 Generated with Claude Code

https://claude.ai/code/session_019XYJgtohJXN2UNtCfL873k

Generated by Claude Code

…nce names A better-auth reference field whose name doesn't end in `Id` (e.g. a plugin's `nullableReference`) derived a relation() whose own field name equaled the exact string its foreign-key column physically maps to — a self-collision the contract derivation refuses (#1236), since Prisma rc.8 aliases an include by relation name and a scalar by physical column name. Fall back to a plain scalar column in that case, mirroring the existing fallback for a reference whose target field isn't the target's `id`. Un-skips the previously-failing "should return null for nullable foreign keys" conformance test. Closes #1222 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XYJgtohJXN2UNtCfL873k

vercel · 2026-09-13T11:54:00Z

Deployment failed for project stack-docs with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/open-saas?upgradeToPro=build-rate-limit

changeset-bot · 2026-09-13T11:54:00Z

🦋 Changeset detected

Latest commit: c0419a2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

Name	Type
@opensaas/stack-auth	Patch
@opensaas/stack-cli	Patch
@opensaas/stack-core	Patch
@opensaas/stack-rag	Patch
@opensaas/stack-storage-s3	Patch
@opensaas/stack-storage-vercel	Patch
@opensaas/stack-storage	Patch
@opensaas/stack-tiptap	Patch
@opensaas/stack-ui	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

chatgpt-codex-connector · 2026-09-13T11:54:00Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

borisno2 · 2026-09-13T12:00:16Z

Review

Reviewed the diff (main...claude/jolly-turing-0q2o06, 4 files) against the four points raised.

1. Guard condition/placement (relationFieldKey !== fieldKey) — Correct. relationshipFieldName is a pure, side-effect-free string transform, so hoisting its computation above the if (rather than only inside the id-reference branch) is behaviorally and perf neutral — it's just always computed now. The new condition upstream.references.field === 'id' && relationFieldKey !== fieldKey correctly narrows the relation-building branch to exactly the cases where a real, non-colliding relation name exists.

2. Fallback-to-scalar path correctness — Correct on all three sub-points. The no-op-strip case falls into the same else branch already used for the pre-existing references.field !== 'id' fallback (issue #992), so it inherits that path's behavior for free rather than needing new logic:

Index suppression: goes through buildScalarField(fieldKey, upstream, claimedFieldsByModel[...].has(fieldKey)), same as every other scalar field.
Credential access: wrapped in withCredentialAccess(credentialRegistry, modelKey, fieldKey, ...), same as every other scalar field.
Reverse relation: correctly not registered — the reverseRelationFields/buildForeignKeyField calls are only reachable inside the if branch, so no phantom reverse relation is created for what is now a plain column.

3. Other relationshipFieldName call sites — Only one call site in the file (line 647). No latent no-op-assuming bugs elsewhere.

4. Doc/comment updates — Accurate and consistent with CLAUDE.md's comment rules. The expanded inline comment explains two external constraints (Prisma's relation-column addressing, and the contract derivation's self-collision refusal from #1236) with issue links — it doesn't restate the code, and it's the kind of "the obvious edit is wrong" warning the rules call out as earning its place. The CLAUDE.md addition is factually accurate against the code and fits the file's existing level of design-rationale documentation. The test file's un-skip and removed doc bullet are consistent with the fix actually closing that gap — confirmed the un-skipped case (nullableReference, references.field: 'id', required: false, no Id suffix) is exactly the shape this fix targets, exercised through a real createTestDatabase/deriveAuthLists run, not a mock.

One thing worth fixing before or shortly after merge

buildCredentialFieldRegistry (line ~191) still validates:

if (upstream.references?.field === 'id') {
  throw new Error(
    `deriveAuthLists: credentialFields names "${modelKey}.${fieldKey}", but "${fieldKey}" is a ` +
      `relationship field (references "${upstream.references.model}.id"), not a scalar credential column`,
  )
}

This check's premise — "any id-referencing field always derives to a relationship, never a scalar" — is exactly what this PR falsifies for the no-op-strip case. After this fix, an id-referencing field whose name doesn't end in Id (the same shape as the nullableReference test fixture) derives to a scalar column that withCredentialAccess can legitimately be applied to, but buildCredentialFieldRegistry still unconditionally rejects any credentialFields entry naming it, with a now-inaccurate error message. A plugin author who needs to mark such a field as a credential column (e.g. a sensitive optional reference-shaped field on their own plugin table) can't do so.

Suggested fix — mirror the same guard used at the derivation site:

if (upstream.references?.field === 'id' && relationshipFieldName(fieldKey) !== fieldKey) {

This is a pre-existing check whose assumption this PR's own fix invalidates, not a bug in the new code path itself, and it doesn't affect the #1222 fix or the un-skipped test — I'd call it a should-fix-for-consistency rather than a blocker, since it only bites a narrower, currently-untested combination (credentialFields naming a no-op-strip reference field). Worth a follow-up test case if addressed.

Nothing else found

No correctness bugs, missed edge cases, or simplification opportunities beyond the above. The change is minimal, well-scoped, and correctly reuses the existing fallback path rather than introducing parallel logic.

Generated by Claude Code

buildCredentialFieldRegistry rejected any id-referencing field named in credentialFields, on the premise that it always derives to a relationship() that withCredentialAccess can't apply to. This PR's own fix (#1222) falsifies that premise for the no-op-strip case, where the field now derives to a scalar column instead — exactly the case withCredentialAccess applies to. Add the same relationshipFieldName guard so the validation matches the derivation it checks. Found in review of PR #1549. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XYJgtohJXN2UNtCfL873k

borisno2 · 2026-09-13T12:04:46Z

Thanks for the review — fixed the buildCredentialFieldRegistry inconsistency in c0419a2: added the same relationshipFieldName(fieldKey) !== fieldKey guard so the validation matches the derivation it checks, and added a test covering credentialFields on a non-Id-suffixed id-referencing field (now a legitimate scalar column). Full packages/auth suite still green (512 passed, 79 skipped).

Generated by Claude Code

borisno2 · 2026-09-13T12:05:38Z

The Vercel status is failing with "Deployment rate limited — retry in 24 hours" (api-deployments-free-per-day) — this is the stack-docs project's free-tier build-count quota, unrelated to this PR's diff (which only touches packages/auth). It reproduces identically on any PR right now regardless of content, and will not be fixed by a re-run within the 24-hour window the message itself states, so I'm not re-running it. The GitHub Actions checks (test, e2e, example-gates) that actually exercise this change are still in progress; I'll keep watching those.

Generated by Claude Code

github-actions · 2026-09-13T12:22:13Z