refactor: branded OrganizationId type + remove RepositoryWithOrg#116
Merged
refactor: branded OrganizationId type + remove RepositoryWithOrg#116
Conversation
…WithOrg - Add `OrganizationId` branded type to prevent mixing with arbitrary strings - Update all function signatures across routes, batch, and services - Remove `RepositoryWithOrg` type; pass `organizationId` as separate arg to Provider.fetch/analyze - Remove organizationId re-attach hack from batch/db/queries getTenantData - Cast at system boundaries (auth guards, DB reads, CLI args) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request introduces a branded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
coji
added a commit
that referenced
this pull request
Feb 26, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/routes/$orgSlug/settings/repositories.$repository.settings/+functions/mutations.server.ts (1)
18-21:⚠️ Potential issue | 🟠 MajorScope the repository UPDATE with
organizationIdas well.Line 19 currently scopes only by repository ID. For org-scoped mutations, add
WHERE organizationId = organizationIdwith the server-derived value.Proposed fix
return tenantDb .updateTable('repositories') .where('id', '=', repositoryId) + .where('organizationId', '=', organizationId) .set(data) .executeTakeFirst()As per coding guidelines, "Every UPDATE/DELETE mutation on org-scoped tables must include
WHERE organizationId = ?with a server-derived value to enforce multi-tenant data isolation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.$repository.settings/+functions/mutations.server.ts around lines 18 - 21, The UPDATE on the repositories table currently only filters by repositoryId; add a second where clause to enforce org scoping by including WHERE organizationId = organizationId using the server-derived organization id variable (i.e., chain .where('organizationId', '=', organizationId) alongside the existing .where('id', '=', repositoryId) before .set(data). Update the block that calls updateTable('repositories') / where('id', '=', repositoryId) / set(data) / executeTakeFirst() so the query includes the organizationId check sourced from the server-derived value.batch/jobs/crawl.ts (1)
58-62:⚠️ Potential issue | 🟠 MajorScope the refresh-flag update with a
WHEREpredicate.Line 60-62 currently executes an unscoped update and can clear
refreshRequestedAtfor every row inorganizationSettingsin that tenant DB.Suggested fix
if (refresh) { const tenantDb = getTenantDb(orgId) await tenantDb .updateTable('organizationSettings') .set({ refreshRequestedAt: null }) + .where('organizationId', '=', orgId) .execute() logger.info('refresh flag consumed.') }Based on learnings: Every UPDATE/DELETE mutation on org-scoped tables must include
WHERE organizationId = ?with a server-derived value to enforce multi-tenant data isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/jobs/crawl.ts` around lines 58 - 62, The update on organizationSettings uses tenantDb.updateTable('organizationSettings').set({ refreshRequestedAt: null }).execute() with no WHERE clause, which risks clearing every row; scope the mutation by adding a WHERE predicate that filters by organizationId (use the server-derived orgId variable) so the query only updates rows where organizationId = orgId (or the equivalent server-provided identifier) before executing.
🧹 Nitpick comments (1)
batch/provider/github/provider.ts (1)
16-37: Add a tenant-consistency guard whenorganizationIdis passed separately.With
organizationIddecoupled from repository objects, Line 30+ and Line 192+ can silently process a repository belonging to a different org if upstream data is mixed. Add an invariant check before store/path usage.Suggested guard
const fetch: Provider['fetch'] = async ( organizationId, repository, { refresh = false, halt = false }, ) => { + invariant( + repository.organizationId === organizationId, + 'repository does not belong to organization', + ) invariant(repository.repo, 'private token not specified') invariant(repository.owner, 'private token not specified') invariant(integration.privateToken, 'private token not specified') @@ for (const repository of repositories) { + invariant( + repository.organizationId === organizationId, + 'repository does not belong to organization', + ) current++ onProgress?.({ repo: repository.repo, current, total })Also applies to: 188-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/provider/github/provider.ts` around lines 16 - 37, Add a tenant-consistency invariant that ensures the passed organizationId matches the repository's owning org before creating storage/path helpers: check that repository.organizationId (or repository.orgId if that is the field used) equals organizationId and throw/invariant if not, then proceed to call createStore and createPathBuilder; apply the same guard in the other code path where createStore/createPathBuilder are used (the block around the later usage referenced in the review) so you never process a repository belonging to a different org.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@app/routes/`$orgSlug/settings/repositories.$repository.settings/+functions/mutations.server.ts:
- Around line 18-21: The UPDATE on the repositories table currently only filters
by repositoryId; add a second where clause to enforce org scoping by including
WHERE organizationId = organizationId using the server-derived organization id
variable (i.e., chain .where('organizationId', '=', organizationId) alongside
the existing .where('id', '=', repositoryId) before .set(data). Update the block
that calls updateTable('repositories') / where('id', '=', repositoryId) /
set(data) / executeTakeFirst() so the query includes the organizationId check
sourced from the server-derived value.
In `@batch/jobs/crawl.ts`:
- Around line 58-62: The update on organizationSettings uses
tenantDb.updateTable('organizationSettings').set({ refreshRequestedAt: null
}).execute() with no WHERE clause, which risks clearing every row; scope the
mutation by adding a WHERE predicate that filters by organizationId (use the
server-derived orgId variable) so the query only updates rows where
organizationId = orgId (or the equivalent server-provided identifier) before
executing.
---
Nitpick comments:
In `@batch/provider/github/provider.ts`:
- Around line 16-37: Add a tenant-consistency invariant that ensures the passed
organizationId matches the repository's owning org before creating storage/path
helpers: check that repository.organizationId (or repository.orgId if that is
the field used) equals organizationId and throw/invariant if not, then proceed
to call createStore and createPathBuilder; apply the same guard in the other
code path where createStore/createPathBuilder are used (the block around the
later usage referenced in the review) so you never process a repository
belonging to a different org.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
app/libs/auth.server.tsapp/routes/$orgSlug/_index/+functions/queries.server.tsapp/routes/$orgSlug/ongoing/+functions/queries.server.tsapp/routes/$orgSlug/settings/_index/+functions/mutations.server.tsapp/routes/$orgSlug/settings/_index/+functions/queries.server.tsapp/routes/$orgSlug/settings/data-management/index.tsxapp/routes/$orgSlug/settings/github-users._index/mutations.server.tsapp/routes/$orgSlug/settings/github-users._index/queries.server.tsapp/routes/$orgSlug/settings/members/mutations.server.tsapp/routes/$orgSlug/settings/members/queries.server.tsapp/routes/$orgSlug/settings/repositories.$repository.$pull/queries.server.tsapp/routes/$orgSlug/settings/repositories.$repository._index/queries.server.tsapp/routes/$orgSlug/settings/repositories.$repository.delete/+functions/mutations.server.tsapp/routes/$orgSlug/settings/repositories.$repository.delete/+functions/queries.server.tsapp/routes/$orgSlug/settings/repositories.$repository.settings/+functions/mutations.server.tsapp/routes/$orgSlug/settings/repositories.$repository.settings/+functions/queries.server.tsapp/routes/$orgSlug/settings/repositories._index/queries.server.tsapp/routes/$orgSlug/settings/repositories.add/+functions/mutations.server.tsapp/routes/$orgSlug/settings/repositories.add/+functions/queries.server.tsapp/routes/admin/+create/mutations.server.tsapp/services/tenant-db.server.test.tsapp/services/tenant-db.server.tsbatch/commands/fetch.tsbatch/commands/report.tsbatch/commands/upsert.tsbatch/config/index.tsbatch/db/mutations.tsbatch/db/queries.tsbatch/helper/path-builder.test.tsbatch/helper/path-builder.tsbatch/jobs/crawl.tsbatch/provider/github/provider.tsbatch/provider/github/pullrequest.tsbatch/provider/github/store.tsbatch/provider/index.tsbatch/scripts/golden-compare.tsbatch/scripts/golden-snapshot.tsbatch/usecases/analyze-and-upsert.tsdb/seed.tslab/fetch.ts
7 tasks
This was referenced Mar 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OrganizationIdbranded type (string & { __brand: 'OrganizationId' }) for compile-time safetyRepositoryWithOrgtype hack from Provider interface; passorganizationIdas separate argument tofetch/analyzebatch/db/queries.ts(getTenantData)Follows up on items documented in
docs/database-per-tenant.md"フォローアップ TODO" section.Design decisions
requireOrgMember/requireOrgAdminreturn, DB reads, CLI argsOrganizationId— no rawstringflows through tenant-scoped code pathsTest plan
pnpm validate— lint, format, typecheck, build, test all pass (31 tests)stringmisuse at compile time🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests