Skip to content

refactor(db): consolidate identity schemas into @nexiom/database as single source of truth - #82

Merged
pramodnarayana merged 5 commits into
developmentfrom
feature/drizzle-schema-consolidation
Mar 6, 2026
Merged

pramodnarayana merged 5 commits into
developmentfrom
feature/drizzle-schema-consolidation

Conversation

@pramodnarayana

@pramodnarayana pramodnarayana commented Mar 6, 2026 •

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features
    • Full identity/authorization schema published: users, sessions, accounts, verifications, RBAC/ABAC, organizations, members, invitations.
  • Refactor
    • Identity schema centralized into the shared database package; identity package now re-exports it.
    • Tenant references in app connections consolidated to reference organizations.
  • Chores
    • Package export map added and identity package now depends on the database package.
  • Migrations
    • Migration to convert tenant references to organization IDs with guard and index rebuild steps.

@coderabbitai

coderabbitai Bot commented Mar 6, 2026 •

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Centralizes the identity schema in the database package and re-exports it; identity package now delegates to that export; appConnections.tenant_id is converted to text referencing organization.id and a migration consolidates tenant→organization.

Changes

Cohort / File(s) Summary
Database: identity schema
packages/database/src/schema/identity.ts
Adds a full identity/RBAC/tenancy schema (users, sessions, accounts, verifications, permissions, roles, role_permission with ABAC jsonb, organizations, members, invitations), relations, constraints, indices, enums, and exported TS types.
Database: public API & index
packages/database/src/index.ts, packages/database/package.json
Re-exports identity schema from package index (export * from './schema/identity') and adds exports map entries exposing ./schema/identity and ./schema/tenant.
Tenant schema & wiring
packages/database/src/schema/tenant.ts
Removes local tenants table export; changes appConnections.tenant_id from uuid referencing tenants.id to text referencing organization.id with ON DELETE CASCADE.
Migration: consolidate tenant→organization
packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql
New migration converting app_connection.tenant_id uuid→text, rewire FK to organization.id, recreate indexes, guard against existing rows, and drop the shadow tenant table.
Identity package: delegate to database
packages/identity/src/schema.ts, packages/identity/package.json
Replaces inline identity schema with export * from "@nexiom/database/dist/schema/identity" and adds @nexiom/database dependency in package.json (removes local schema exports).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐇 I hopped where tables merge and indexes grow,

Moved identities in a tidy row,
Tenants turned to orgs, migrations sway,
I nibble constraints and scamper away — hooray! 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main objective of consolidating identity schemas from packages/identity into packages/database as a single source of truth, which is reflected across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/drizzle-schema-consolidation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/database/src/schema/identity.ts`:
- Around line 64-72: The schema currently persists raw ipAddress and userAgent
but lacks the required 30-day retention job; implement a background job function
runPIICleanup that the background scheduler runs daily, select session rows
where updatedAt or createdAt is older than 30 days, atomically anonymize
ipAddress and userAgent (e.g., replace with null or hashed token) via the same
repository/ORM used by the schema, write an audit log entry for each batch (or
aggregated) including row ids and action, and wire runPIICleanup into the app's
scheduler/bootstrap so it runs automatically; reference the ipAddress and
userAgent fields in the session schema and the runPIICleanup function when
locating the changes and add unit/integration tests to validate selection
criteria and anonymization.
- Around line 276-278: The organization-scoped foreign keys (organizationId) in
the member and invitation schemas must be non-nullable and the unique index
should be a plain unique on (userId, organizationId) instead of using
COALESCE('__NULL__'); update the organizationId column definitions (keep
references(() => organization.id, { onDelete: "cascade" })) to call notNull(),
remove any COALESCE-based index definitions, and add a regular unique index for
(userId, organizationId) (e.g., for the member table replace the COALESCE-backed
active-member index with a plain unique index on (userId, organizationId); apply
the same change to the invitation table and the other affected occurrences).

In `@packages/database/src/schema/tenant.ts`:
- Around line 28-30: The appConnections schema now defines tenantId as text
referencing organization.id but no migration exists to change the existing
tenant_id UUID column and FK; add a migration that (1) alters
app_connection.tenant_id column from uuid to text (handling nullability), (2)
drops the old FK constraint that references tenant.id and creates a new FK to
organization.id, (3) backfills/rewrites existing tenant_id values so they match
organization.id values (or adds a mapping/lookup step to translate tenant.uuid
-> organization.id), and (4) recreates any dependent indexes (tenant_status_idx,
tenant_external_id_unique_idx) after the type change; reference the
appConnections/tenantId symbol and organization.id when implementing these steps
so the schema and migration remain consistent during rollout.

In `@packages/identity/src/schema.ts`:
- Line 1: The file currently re-exports the built dist path via the statement
export * from "@nexiom/database/dist/schema/identity"; which hard-codes
`@nexiom/database`'s build layout; fix it by adding a subpath export
"./schema/identity" to `@nexiom/database`'s package.json exports (mapping the
import to the built JS and types to the built .d.ts) and change the re-export in
packages/identity/src/schema.ts to export * from
"@nexiom/database/schema/identity"; so source builds no longer depend on
packages/database/dist existing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 38bb1986-9184-4b5c-b5f1-b4e4612556c8

📥 Commits

Reviewing files that changed from the base of the PR and between ccd2316 and 4c68a75.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (5)
  • packages/database/src/index.ts
  • packages/database/src/schema/identity.ts
  • packages/database/src/schema/tenant.ts
  • packages/identity/package.json
  • packages/identity/src/schema.ts

Comment on lines +64 to +72
// PII / Retention Policy:
// IP Address and User Agent containing PII should be anonymized or retained only for
// a limited period (e.g., 30 days) for security auditing, then purged.
// TASK: Implement scheduled PII cleanup (Issue #TRACK-142)
// - Criteria: Run daily, older than 30d, anonymize fields, audit log
// - Owner: Security Team
// - Implementation: Create `runPIICleanup` job in background module
ipAddress: text("ipAddress"),
userAgent: text("userAgent"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Ship the retention control with these session PII fields.

This schema persists raw ipAddress and userAgent, but the purge/anonymization path is still only a TODO. If the 30-day policy is required, please land that job in the same rollout instead of deferring it.

If useful, I can sketch the cleanup job contract and selection criteria for the follow-up.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/database/src/schema/identity.ts` around lines 64 - 72, The schema
currently persists raw ipAddress and userAgent but lacks the required 30-day
retention job; implement a background job function runPIICleanup that the
background scheduler runs daily, select session rows where updatedAt or
createdAt is older than 30 days, atomically anonymize ipAddress and userAgent
(e.g., replace with null or hashed token) via the same repository/ORM used by
the schema, write an audit log entry for each batch (or aggregated) including
row ids and action, and wire runPIICleanup into the app's scheduler/bootstrap so
it runs automatically; reference the ipAddress and userAgent fields in the
session schema and the runPIICleanup function when locating the changes and add
unit/integration tests to validate selection criteria and anonymization.

Comment thread packages/database/src/schema/identity.ts Outdated
Comment on lines 28 to +30
export const appConnections = pgTable('app_connection', {
id: uuid('id').defaultRandom().primaryKey(),
tenantId: uuid('tenant_id').notNull().references(() => tenants.id, { onDelete: 'cascade' }),
tenantId: text('tenant_id').notNull().references(() => organization.id, { onDelete: 'cascade' }),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== changed schema fragments =="
sed -n '1,90p' packages/database/src/schema/tenant.ts
sed -n '239,330p' packages/database/src/schema/identity.ts

echo "== migration directories =="
fd 'drizzle|migrations' . -td

echo "== migrations touching app_connection / tenant_id / tenants / organization =="
while IFS= read -r dir; do
  rg -n --glob '*.{sql,ts,js}' 'app_connection|tenant_id|tenants\b|organization\b' "$dir"
done < <(fd 'drizzle|migrations' . -td)

Repository: pramodnarayana/nexiom

Length of output: 6530


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Listing migration files =="
find ./apps/api/drizzle -type f \( -name "*.sql" -o -name "*.ts" -o -name "*.js" \) | sort
echo ""
find ./packages/database/drizzle -type f \( -name "*.sql" -o -name "*.ts" -o -name "*.js" \) | sort
echo ""
find ./packages/identity/drizzle -type f \( -name "*.sql" -o -name "*.ts" -o -name "*.js" \) | sort

echo ""
echo "== Searching migrations for app_connection references =="
rg 'app_connection' ./apps/api/drizzle ./packages/database/drizzle ./packages/identity/drizzle --glob '*.{sql,ts,js}'

echo ""
echo "== Searching migrations for tenant_id references =="
rg 'tenant_id' ./apps/api/drizzle ./packages/database/drizzle ./packages/identity/drizzle --glob '*.{sql,ts,js}'

echo ""
echo "== Searching migrations for type changes (text/uuid/type) =="
rg -A 5 'ALTER.*app_connection|ALTER.*tenant|ALTER.*organization' ./apps/api/drizzle ./packages/database/drizzle ./packages/identity/drizzle --glob '*.{sql,ts,js}'

Repository: pramodnarayana/nexiom

Length of output: 10317


Migration required: Add tenant_id column alteration and FK target rewrite before rollout.

The code currently references appConnections.tenantId as text targeting organization.id, but the migrations only show the initial schema with tenant_id as uuid targeting tenant.id (via 0001_goofy_mercury.sql). No migration exists to backfill and alter the column type or update the FK constraint. Deploying this without a corresponding migration will cause foreign key constraint violations or deployment failure.

Required migration must:

  1. Alter tenant_id column type from uuid to text
  2. Drop and recreate the FK constraint to target organization.id instead of tenant.id
  3. Backfill existing tenant_id values to match organization.id values (or handle mismatch appropriately)
  4. Recreate dependent indexes (tenant_status_idx, tenant_external_id_unique_idx)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/database/src/schema/tenant.ts` around lines 28 - 30, The
appConnections schema now defines tenantId as text referencing organization.id
but no migration exists to change the existing tenant_id UUID column and FK; add
a migration that (1) alters app_connection.tenant_id column from uuid to text
(handling nullability), (2) drops the old FK constraint that references
tenant.id and creates a new FK to organization.id, (3) backfills/rewrites
existing tenant_id values so they match organization.id values (or adds a
mapping/lookup step to translate tenant.uuid -> organization.id), and (4)
recreates any dependent indexes (tenant_status_idx,
tenant_external_id_unique_idx) after the type change; reference the
appConnections/tenantId symbol and organization.id when implementing these steps
so the schema and migration remain consistent during rollout.

};
export type AbacConditions = Record<string, AbacValue | AbacOperator>;
export type Permission = typeof permission.$inferSelect;
export * from "@nexiom/database/dist/schema/identity";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== identity schema re-export =="
sed -n '1,20p' packages/identity/src/schema.ts

echo "== database package entry points =="
sed -n '1,120p' packages/database/package.json

echo "== import sites using database schema entry points =="
rg -n '@nexiom/database(/dist/schema/identity|/schema/identity)?' packages

echo "== tsconfig path aliases mentioning `@nexiom/database` =="
rg -n --glob 'tsconfig*.json' '@nexiom/database|paths' .

Repository: pramodnarayana/nexiom

Length of output: 2131


Don't re-export a sibling package's dist file from source.

This hard-codes @nexiom/database's build layout into @nexiom/identity, making clean/source builds dependent on packages/database/dist/schema/identity existing first. Add a ./schema/identity subpath export to packages/database/package.json and re-export from that instead.

Possible fix
-export * from "@nexiom/database/dist/schema/identity";
+export * from "@nexiom/database/schema/identity";

Add this to packages/database/package.json:

"exports": {
  "./schema/identity": {
    "import": "./dist/schema/identity.js",
    "types": "./dist/schema/identity.d.ts"
  }
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/identity/src/schema.ts` at line 1, The file currently re-exports the
built dist path via the statement export * from
"@nexiom/database/dist/schema/identity"; which hard-codes `@nexiom/database`'s
build layout; fix it by adding a subpath export "./schema/identity" to
`@nexiom/database`'s package.json exports (mapping the import to the built JS and
types to the built .d.ts) and change the re-export in
packages/identity/src/schema.ts to export * from
"@nexiom/database/schema/identity"; so source builds no longer depend on
packages/database/dist existing.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
packages/database/src/schema/identity.ts (1)

289-296: ⚠️ Potential issue | 🟠 Major

Soft-deleted members still block re-adding the same user.

deletedAt makes this table soft-deleteable, but the unconditional unique constraint on (userId, organizationId) means a deleted membership still prevents inserting a new active row for the same pair. Make the uniqueness apply only to active memberships.

🛠️ Suggested fix
   (table) => [
-    unique("member_user_org_unique").on(table.userId, table.organizationId),
+    uniqueIndex("member_user_org_active_idx")
+      .on(table.userId, table.organizationId)
+      .where(sql`"deletedAt" IS NULL`),
     index("member_org_idx").on(table.organizationId),
   ],
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/database/src/schema/identity.ts` around lines 289 - 296, The current
unconditional unique constraint member_user_org_unique on (userId,
organizationId) prevents re-adding a user after soft-delete; change it to a
partial/filtered unique index that only enforces uniqueness for active rows by
adding the condition deletedAt IS NULL (i.e., make the unique apply only when
deletedAt is null). Update the schema definition that creates
member_user_org_unique (referencing userId, organizationId, deletedAt) to create
a conditional/partial unique index instead, leaving the member_org_idx as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql`:
- Around line 12-13: The migration currently unconditionally truncates
"app_connection" (Step 3) which can irreversibly drop data; change it to fail
loudly instead: replace the TRUNCATE statement with a pre-check that queries
COUNT(*) from "app_connection" and raises an exception (aborting the migration)
if rows exist, or alternatively implement an explicit backfill/mapping step for
existing rows; locate the TRUNCATE TABLE "app_connection" line in this migration
and implement the conditional check/RAISE EXCEPTION logic so the migration
aborts when the table is non-empty instead of silently deleting data.

In `@packages/database/package.json`:
- Around line 22-26: Remove the published internal export
"./dist/schema/identity" from the package.json exports and update the consumer
in packages/identity/src/schema.ts to re-export from the public entry
"@nexiom/database/schema/identity" (replace any use of "./dist/schema/identity"
in that file); this drops the hard-coded build path from the package API and
prevents other packages from depending on the dist layout.

---

Duplicate comments:
In `@packages/database/src/schema/identity.ts`:
- Around line 289-296: The current unconditional unique constraint
member_user_org_unique on (userId, organizationId) prevents re-adding a user
after soft-delete; change it to a partial/filtered unique index that only
enforces uniqueness for active rows by adding the condition deletedAt IS NULL
(i.e., make the unique apply only when deletedAt is null). Update the schema
definition that creates member_user_org_unique (referencing userId,
organizationId, deletedAt) to create a conditional/partial unique index instead,
leaving the member_org_idx as-is.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1d66185d-b13f-42b9-a452-892510bdc949

📥 Commits

Reviewing files that changed from the base of the PR and between 4c68a75 and 8d1d484.

📒 Files selected for processing (3)
  • packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql
  • packages/database/package.json
  • packages/database/src/schema/identity.ts

Comment thread packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql Outdated
Comment on lines +22 to +26
"./dist/schema/identity": {
"require": "./dist/schema/identity.js",
"import": "./dist/schema/identity.js",
"default": "./dist/schema/identity.js"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Avoid publishing the internal dist path.

"./schema/identity" already exposes this module. Keeping ./dist/schema/identity public hard-codes your build layout into the package API, and packages/identity/src/schema.ts:1 is currently the only consumer relying on it. Switch that re-export to @nexiom/database/schema/identity and drop this alias before other packages start depending on it.

♻️ Suggested cleanup
         "./schema/tenant": {
             "require": "./dist/schema/tenant.js",
             "import": "./dist/schema/tenant.js",
             "default": "./dist/schema/tenant.js"
-        },
-        "./dist/schema/identity": {
-            "require": "./dist/schema/identity.js",
-            "import": "./dist/schema/identity.js",
-            "default": "./dist/schema/identity.js"
         }
// packages/identity/src/schema.ts
export * from "@nexiom/database/schema/identity";
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/database/package.json` around lines 22 - 26, Remove the published
internal export "./dist/schema/identity" from the package.json exports and
update the consumer in packages/identity/src/schema.ts to re-export from the
public entry "@nexiom/database/schema/identity" (replace any use of
"./dist/schema/identity" in that file); this drops the hard-coded build path
from the package API and prevents other packages from depending on the dist
layout.

@pramodnarayana
pramodnarayana marked this pull request as draft March 6, 2026 10:40
@pramodnarayana
pramodnarayana marked this pull request as ready for review March 6, 2026 10:40

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql (1)

37-38: ⚠️ Potential issue | 🟠 Major

Fail loudly before dropping the shadow tenant table.

The migration guards app_connection, but it will still unconditionally drop tenant and discard any rows left there. If any environment still has source data in that table, this is irreversible data loss. Abort when tenant is non-empty, or backfill it into organization in the same migration.

Safer migration shape
 -- Step 7: Drop the orphaned shadow tenant table
-DROP TABLE IF EXISTS "tenant";--> statement-breakpoint
+DO $$
+BEGIN
+  IF to_regclass('public.tenant') IS NOT NULL
+     AND EXISTS (SELECT 1 FROM "tenant" LIMIT 1) THEN
+    RAISE EXCEPTION
+      'Migration aborted: tenant still has rows. Backfill tenant -> organization before dropping the shadow table.';
+  END IF;
+END $$;--> statement-breakpoint
+DROP TABLE IF EXISTS "tenant";--> statement-breakpoint
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql`
around lines 37 - 38, The migration currently unconditionally drops the shadow
table with DROP TABLE IF EXISTS "tenant" (Step 7); add a pre-drop safety check
that fails loudly or backfills instead: run a COUNT check on the "tenant" table
and if count > 0 either RAISE EXCEPTION to abort the migration or perform a
deterministic backfill into the "organization" table (INSERT INTO "organization"
... SELECT ... FROM "tenant") within the same migration, then only DROP TABLE
"tenant" after the check/backfill completes; update the Step 7 block around DROP
TABLE IF EXISTS "tenant" to implement this guard.
packages/database/src/schema/identity.ts (1)

64-72: ⚠️ Potential issue | 🟠 Major

Land the session PII cleanup with this schema move.

This still stores raw ipAddress and userAgent and only documents a future cleanup job. Because @nexiom/database is now the canonical schema export, the 30-day anonymization/purge path needs to ship with the same rollout instead of remaining a TODO.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/database/src/schema/identity.ts` around lines 64 - 72, The schema
currently exposes raw ipAddress and userAgent fields; change the schema and
related persistence flow so these fields are stored anonymized by default and
add the actual scheduled cleanup job: (1) update the identity schema's ipAddress
and userAgent handling to store a deterministic hash/truncated fingerprint
(referencing ipAddress and userAgent in identity.ts) and add an
onCreate/onUpdate sanitizer helper that performs anonymizeIp() and
anonymizeUserAgent() before insert; (2) add a daily background job runPIICleanup
in the background module that finds sessions older than 30 days, replaces
remaining raw values with the anonymized fingerprint (or null), and emits an
audit log entry for each modified session; (3) provide a migration/backfill to
process existing rows to replace raw values with the anonymized form so the
canonical `@nexiom/database` export never contains raw PII.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/database/src/schema/identity.ts`:
- Around line 324-325: The invitations table currently allows arbitrary strings
in the role column (in schema at identity.ts where email:
text("email").notNull(), role: text("role")), so add a foreign-key constraint on
invitations.role to the canonical roles table/ID used by member.role and
role_permission.roleId so that invalid role strings are rejected at insert;
update the invitations.role column definition to reference the same roles
primary key (the same FK target used by member.role/role_permission.roleId) and
run migrations to enforce the constraint.

---

Duplicate comments:
In `@packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql`:
- Around line 37-38: The migration currently unconditionally drops the shadow
table with DROP TABLE IF EXISTS "tenant" (Step 7); add a pre-drop safety check
that fails loudly or backfills instead: run a COUNT check on the "tenant" table
and if count > 0 either RAISE EXCEPTION to abort the migration or perform a
deterministic backfill into the "organization" table (INSERT INTO "organization"
... SELECT ... FROM "tenant") within the same migration, then only DROP TABLE
"tenant" after the check/backfill completes; update the Step 7 block around DROP
TABLE IF EXISTS "tenant" to implement this guard.

In `@packages/database/src/schema/identity.ts`:
- Around line 64-72: The schema currently exposes raw ipAddress and userAgent
fields; change the schema and related persistence flow so these fields are
stored anonymized by default and add the actual scheduled cleanup job: (1)
update the identity schema's ipAddress and userAgent handling to store a
deterministic hash/truncated fingerprint (referencing ipAddress and userAgent in
identity.ts) and add an onCreate/onUpdate sanitizer helper that performs
anonymizeIp() and anonymizeUserAgent() before insert; (2) add a daily background
job runPIICleanup in the background module that finds sessions older than 30
days, replaces remaining raw values with the anonymized fingerprint (or null),
and emits an audit log entry for each modified session; (3) provide a
migration/backfill to process existing rows to replace raw values with the
anonymized form so the canonical `@nexiom/database` export never contains raw PII.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0eda107d-741d-4204-a2e4-d65aa430b612

📥 Commits

Reviewing files that changed from the base of the PR and between 8d1d484 and 575a742.

📒 Files selected for processing (2)
  • packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql
  • packages/database/src/schema/identity.ts

Comment thread packages/database/src/schema/identity.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
packages/database/src/schema/identity.ts (1)

64-72: ⚠️ Potential issue | 🟠 Major

Don’t ship raw session PII behind a TODO.

ipAddress and userAgent are still being persisted without the 30-day anonymization/purge path described in the comment. Please land the cleanup job in the same rollout, or defer these columns until the retention control exists.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/database/src/schema/identity.ts` around lines 64 - 72, The schema is
persisting raw PII fields ipAddress and userAgent without the 30-day
anonymization/purge; either implement the scheduled cleanup or remove/defer
these columns. Fix by adding a background job runPIICleanup that runs daily,
queries session records older than 30 days, anonymizes or purges ipAddress and
userAgent, writes an audit log entry per batch, and wire it into the background
module startup; ensure the job is idempotent, covered by tests, and add a
migration or schema flag if you prefer deferring these columns until retention
is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql`:
- Around line 1-3: Update the migration header comments to remove the statement
about truncating app_connection and instead state that the migration will abort
if app_connection contains rows (reflecting the new guard behavior);
specifically, replace the phrase "Since we are pre-production, we truncate
app_connection to avoid data mapping complexity." with language that mentions
aborting/guarding on existing rows in app_connection so readers of this
migration (referencing app_connection and the migration purpose "Consolidate
tenant_id to reference organization.id") understand the current behavior.

---

Duplicate comments:
In `@packages/database/src/schema/identity.ts`:
- Around line 64-72: The schema is persisting raw PII fields ipAddress and
userAgent without the 30-day anonymization/purge; either implement the scheduled
cleanup or remove/defer these columns. Fix by adding a background job
runPIICleanup that runs daily, queries session records older than 30 days,
anonymizes or purges ipAddress and userAgent, writes an audit log entry per
batch, and wire it into the background module startup; ensure the job is
idempotent, covered by tests, and add a migration or schema flag if you prefer
deferring these columns until retention is enforced.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 86915e77-8a72-40c8-89bb-e79b755c2ca5

📥 Commits

Reviewing files that changed from the base of the PR and between 575a742 and c1e868a.

📒 Files selected for processing (2)
  • packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql
  • packages/database/src/schema/identity.ts

Comment on lines +1 to +3
-- Migration: Consolidate tenant_id to reference organization.id
-- Replaces the old shadow `tenant` table FK with a direct FK to `organization.id`.
-- Since we are pre-production, we truncate app_connection to avoid data mapping complexity.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Update the migration header to match the new guard behavior.

The file no longer truncates app_connection; it aborts when rows exist. Keeping the old wording here is misleading for anyone reading migration history during rollout/debugging.

Suggested fix
 -- Migration: Consolidate tenant_id to reference organization.id
 -- Replaces the old shadow `tenant` table FK with a direct FK to `organization.id`.
--- Since we are pre-production, we truncate app_connection to avoid data mapping complexity.
+-- This migration only proceeds when `app_connection` is empty; otherwise it aborts
+-- so operators can backfill `tenant_id` -> `organization.id` first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/database/drizzle/0002_consolidate_tenant_id_to_organization.sql`
around lines 1 - 3, Update the migration header comments to remove the statement
about truncating app_connection and instead state that the migration will abort
if app_connection contains rows (reflecting the new guard behavior);
specifically, replace the phrase "Since we are pre-production, we truncate
app_connection to avoid data mapping complexity." with language that mentions
aborting/guarding on existing rows in app_connection so readers of this
migration (referencing app_connection and the migration purpose "Consolidate
tenant_id to reference organization.id") understand the current behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant