Conversation
Owner
Author
This was referenced Apr 7, 2026
Closed
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughインストール処理でGitHubアカウントの Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant API as "GitHub Setup API"
participant TX as "Shared DB TX"
participant DB as "Shared DB"
participant Log as "Event Log Service"
participant Fetch as "fetchInstallationRepositories"
participant Init as "initializeMembershipsForInstallation"
User->>API: インストール完了通知をPOST
API->>TX: トランザクション開始（upsert githubAppLinks）
TX->>DB: upsert (organizationId, installationId, githubAccountType...)
DB-->>TX: upsert結果
TX->>Log: logGithubAppLinkEvent(link_created, accountType)
Log-->>TX: ログ書込結果
TX->>TX: コミット
API->>Fetch: fetchInstallationRepositories(installationId)
Fetch-->>API: [{owner,name}, ...]
API->>Init: initializeMembershipsForInstallation(org, installationId, repos)
Init->>DB: tenant側で membership upsert
DB-->>Init: マッチした repoIds
Init-->>API: マッチ結果
API->>DB: update githubAppLinks.membershipInitializedAt
DB-->>API: 更新完了
API->>Log: tryLogGithubAppLinkEvent(membership_initialized)
Log-->>API: ログ結果
API-->>User: 完了
sequenceDiagram
participant Webhook as "Installation Deleted Webhook"
participant Handler as "Installation Handler"
participant DB as "Shared DB"
participant Reassign as "reassignCanonicalAfterLinkLoss"
participant TenantDB as "Tenant DB"
participant Log as "Event Log"
Webhook->>Handler: 削除ペイロード受信
Handler->>DB: githubAppLinks を deleted（organizationId, installationId で更新）
DB-->>Handler: 更新結果
Handler->>Log: logGithubAppLinkEvent(link_deleted)
Log-->>Handler: ログ完了
Handler->>Reassign: reassignCanonicalAfterLinkLoss(org, lostInstallationId, source)
Reassign->>TenantDB: 影響リポジトリと候補 membership をクエリ
TenantDB-->>Reassign: repo rows + membership candidates
Reassign->>TenantDB: repositories.githubInstallationId を決定値でグループ更新
TenantDB-->>Reassign: 更新結果
Reassign->>Log: tryLogGithubAppLinkEvent(各 repo の決定イベント)
Log-->>Reassign: ログ完了
Reassign-->>Handler: 完了
Handler-->>Webhook: 処理終了
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 分 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 |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/services/jobs/crawl.server.ts (1)
21-56: 自動修復ヘルパーの実装は適切です。
repairMembershipForLinkは、初期化されていないリンクのメンバーシップを修復するベストエフォート型のヘルパーとして正しく実装されています。エラーを飲み込んで監査ログに記録する設計は、自動修復の目的に適しています。ただし、一貫性のため
getErrorMessage()の使用を検討してください。♻️ 提案する修正
+import { getErrorMessage } from '~/app/libs/error-message'} catch (e) { await tryLogGithubAppLinkEvent({ organizationId, installationId, eventType: 'membership_repaired', source: 'crawl_repair', status: 'failed', - details: { error: e instanceof Error ? e.message : String(e) }, + details: { error: getErrorMessage(e) }, }) }As per coding guidelines: "Use
getErrorMessage()fromapp/libs/error-message.tsto extract error messages"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/jobs/crawl.server.ts` around lines 21 - 56, The catch block in repairMembershipForLink currently converts the error with e instanceof Error ? e.message : String(e); replace that with a call to getErrorMessage(e) (imported from app/libs/error-message.ts) so the standardized extractor is used for the logged details; update the top of the file to import getErrorMessage and change the details payload in tryLogGithubAppLinkEvent to use getErrorMessage(e).app/routes/api.github.setup.ts (1)
164-174:getErrorMessage()の使用を検討してください。コーディングガイドラインでは、エラーメッセージの抽出には
getErrorMessage()を使用し、String(e)を避けるよう指定されています。監査ログ用途ではありますが、一貫性のためgetErrorMessage()の使用を検討してください。♻️ 提案する修正
+import { getErrorMessage } from '~/app/libs/error-message'await tryLogGithubAppLinkEvent({ organizationId, installationId: installation.id, eventType: 'membership_initialized', source: 'setup_callback', status: 'failed', - details: { error: e instanceof Error ? e.message : String(e) }, + details: { error: getErrorMessage(e) }, })As per coding guidelines: "Use
getErrorMessage()fromapp/libs/error-message.tsto extract error messages, never useString(e)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/api.github.setup.ts` around lines 164 - 174, Replace the inline String(e) extraction in the catch block that calls tryLogGithubAppLinkEvent with a call to getErrorMessage(...) from app/libs/error-message.ts; specifically, in the catch handling around membership init (the catch that logs '[api.github.setup] membership init failed' and calls tryLogGithubAppLinkEvent with details.error), import getErrorMessage and pass the caught exception to it so details.error uses getErrorMessage(e) instead of String(e).app/services/github-webhook-installation.server.ts (5)
67-76: 共有DBの更新にorganizationId条件を追加することを推奨
installationIdのみで更新していますが、コーディングガイドラインでは共有DBの org-scoped テーブルへの UPDATE にはWHERE organizationId = ?を含めることが求められています。link.organizationIdが利用可能なので、防御的コーディングとして追加することを検討してください。♻️ 推奨される修正
await trx .updateTable('githubAppLinks') .set({ githubOrg: login, githubAccountType: accountType, appRepositorySelection: selectionFromInstallation(installation), updatedAt: now, }) + .where('organizationId', '=', link.organizationId) .where('installationId', '=', installation.id) .execute()As per coding guidelines: "Every UPDATE/DELETE on shared database org-scoped tables must include
WHERE organizationId = ?with server-derived organization ID"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/github-webhook-installation.server.ts` around lines 67 - 76, The UPDATE against the shared org-scoped table githubAppLinks uses only installationId; add a server-derived organizationId guard to the WHERE clause to satisfy the guideline. Modify the trx.updateTable('githubAppLinks')...where('installationId','=', installation.id).execute() chain to also include .where('organizationId', '=', link.organizationId) (using the existing link.organizationId value) so the update scopes to the correct org; ensure link.organizationId is used (not client-provided data) and keep both WHERE conditions in the same query.
88-93: 同様にorganizationId条件を追加することを推奨
handleInstallationDeletedでもlink.organizationIdが利用可能なので、UPDATE に追加することを推奨します。♻️ 推奨される修正
await trx .updateTable('githubAppLinks') .set({ deletedAt: now, updatedAt: now }) + .where('organizationId', '=', link.organizationId) .where('installationId', '=', installation.id) .execute()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/github-webhook-installation.server.ts` around lines 88 - 93, The UPDATE currently only filters by installationId; add the organizationId filter to avoid touching other orgs' rows by extending the query on trx.updateTable('githubAppLinks').set(...) to include a .where('organizationId', '=', link.organizationId) (using the existing link.organizationId available in handleInstallationDeleted) so the update matches both installationId and organizationId.
189-198: イベントタイプmembership_repairedは意味的に不正確かもしれませんこの関数は
installation_repositorieswebhook の add/remove を処理していますが、membership_repairedは自動修復ジョブで使用されるイベントタイプのように見えます。repositories_added/repositories_removedまたはmembership_syncedのような、より正確なイベントタイプを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/github-webhook-installation.server.ts` around lines 189 - 198, The eventType currently passed to logGithubAppLinkEvent ('membership_repaired') is semantically incorrect for handling installation_repositories webhook add/remove events; update the call in the installation_repositories_webhook handling code to emit a more precise eventType such as 'repositories_added' or 'repositories_removed' (or 'membership_synced' if this block is doing a full sync) based on the webhook action, and ensure you use the link.organizationId and installation.id values as before; also update any downstream consumers/tests that expect 'membership_repaired' to the new event name.
180-187:handleInstallationRepositoriesEventでも同様の修正を推奨一貫性のため、
link.organizationIdを WHERE 条件に追加してください。♻️ 推奨される修正
await trx .updateTable('githubAppLinks') .set({ appRepositorySelection: selectionFromInstallation(installation), updatedAt: now, }) + .where('organizationId', '=', link.organizationId) .where('installationId', '=', installation.id) .execute()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/github-webhook-installation.server.ts` around lines 180 - 187, The update on githubAppLinks only filters by installationId; add the organizationId condition for consistency by modifying the trx.updateTable('githubAppLinks')...where(...) clause to include .where('organizationId', '=', link.organizationId) (or an AND-equivalent chained where) so the update targets the specific organization—use the existing link variable from the surrounding function (same pattern as handleInstallationRepositoriesEvent).
139-146:handleInstallationSuspendでも同様の修正を推奨一貫性のため、
link.organizationIdを WHERE 条件に追加してください。♻️ 推奨される修正
await trx .updateTable('githubAppLinks') .set({ suspendedAt: suspend ? now : null, updatedAt: now, }) + .where('organizationId', '=', link.organizationId) .where('installationId', '=', installation.id) .execute()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/github-webhook-installation.server.ts` around lines 139 - 146, この更新クエリは installationId のみで行を絞っていますが、一貫性のため handleInstallationSuspend と同様に組織でスコープを狭める必要があります。trx.updateTable('githubAppLinks') の .where('installationId', '=', installation.id) に加えて link.organizationId を参照する WHERE 条件（例: .where('organizationId', '=', link.organizationId 相当））を追加して、更新対象を該当組織のリンクに限定してください。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/services/github-webhook-pull.server.ts`:
- Around line 53-66: The current single query with OR on githubInstallationId
can return the NULL-backed row first; change to a two-step lookup using
tenantDb: first query repositories where owner=coords.owner, repo=coords.name
and githubInstallationId = installation.id (use executeTakeFirst()), and if that
returns null run a second query matching githubInstallationId IS NULL as a
fallback; return the id from the first non-null result so installation-matching
rows are always preferred.
---
Nitpick comments:
In `@app/routes/api.github.setup.ts`:
- Around line 164-174: Replace the inline String(e) extraction in the catch
block that calls tryLogGithubAppLinkEvent with a call to getErrorMessage(...)
from app/libs/error-message.ts; specifically, in the catch handling around
membership init (the catch that logs '[api.github.setup] membership init failed'
and calls tryLogGithubAppLinkEvent with details.error), import getErrorMessage
and pass the caught exception to it so details.error uses getErrorMessage(e)
instead of String(e).
In `@app/services/github-webhook-installation.server.ts`:
- Around line 67-76: The UPDATE against the shared org-scoped table
githubAppLinks uses only installationId; add a server-derived organizationId
guard to the WHERE clause to satisfy the guideline. Modify the
trx.updateTable('githubAppLinks')...where('installationId','=',
installation.id).execute() chain to also include .where('organizationId', '=',
link.organizationId) (using the existing link.organizationId value) so the
update scopes to the correct org; ensure link.organizationId is used (not
client-provided data) and keep both WHERE conditions in the same query.
- Around line 88-93: The UPDATE currently only filters by installationId; add
the organizationId filter to avoid touching other orgs' rows by extending the
query on trx.updateTable('githubAppLinks').set(...) to include a
.where('organizationId', '=', link.organizationId) (using the existing
link.organizationId available in handleInstallationDeleted) so the update
matches both installationId and organizationId.
- Around line 189-198: The eventType currently passed to logGithubAppLinkEvent
('membership_repaired') is semantically incorrect for handling
installation_repositories webhook add/remove events; update the call in the
installation_repositories_webhook handling code to emit a more precise eventType
such as 'repositories_added' or 'repositories_removed' (or 'membership_synced'
if this block is doing a full sync) based on the webhook action, and ensure you
use the link.organizationId and installation.id values as before; also update
any downstream consumers/tests that expect 'membership_repaired' to the new
event name.
- Around line 180-187: The update on githubAppLinks only filters by
installationId; add the organizationId condition for consistency by modifying
the trx.updateTable('githubAppLinks')...where(...) clause to include
.where('organizationId', '=', link.organizationId) (or an AND-equivalent chained
where) so the update targets the specific organization—use the existing link
variable from the surrounding function (same pattern as
handleInstallationRepositoriesEvent).
- Around line 139-146: この更新クエリは installationId のみで行を絞っていますが、一貫性のため
handleInstallationSuspend
と同様に組織でスコープを狭める必要があります。trx.updateTable('githubAppLinks') の
.where('installationId', '=', installation.id) に加えて link.organizationId を参照する
WHERE 条件（例: .where('organizationId', '=', link.organizationId
相当））を追加して、更新対象を該当組織のリンクに限定してください。
In `@app/services/jobs/crawl.server.ts`:
- Around line 21-56: The catch block in repairMembershipForLink currently
converts the error with e instanceof Error ? e.message : String(e); replace that
with a call to getErrorMessage(e) (imported from app/libs/error-message.ts) so
the standardized extractor is used for the logged details; update the top of the
file to import getErrorMessage and change the details payload in
tryLogGithubAppLinkEvent to use getErrorMessage(e).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25950167-87bf-4c58-91ba-643aafd11726
📒 Files selected for processing (11)
app/routes/api.github.setup.tsapp/services/github-app-link-events.server.tsapp/services/github-app-membership.server.test.tsapp/services/github-app-membership.server.tsapp/services/github-installation-repos.server.tsapp/services/github-webhook-installation.server.tsapp/services/github-webhook-pull.server.tsapp/services/github-webhook-shared.server.tsapp/services/github-webhook.server.test.tsapp/services/github-webhook.server.tsapp/services/jobs/crawl.server.ts
Comment on lines
+53
to
+66
| // Match by `(owner, repo)` *and* the installation that delivered the | ||
| // webhook. `github_installation_id IS NULL` is still accepted while | ||
| // existing rows are unbackfilled; the strict variant drops the OR clause. | ||
| const repo = await tenantDb | ||
| .selectFrom('repositories') | ||
| .select('id') | ||
| .where('owner', '=', coords.owner) | ||
| .where('repo', '=', coords.name) | ||
| .where((eb) => | ||
| eb.or([ | ||
| eb('githubInstallationId', '=', installation.id), | ||
| eb('githubInstallationId', 'is', null), | ||
| ]), | ||
| ) |
There was a problem hiding this comment.
インストール一致を優先していないため、誤った repositoryId を選ぶ可能性があります。
OR githubInstallationId IS NULL と executeTakeFirst() の組み合わせだと、同一 (owner, repo) に一致行と NULL 行が共存する移行期に NULL 側を返し得ます。フォールバックは残しつつ、まず installation 一致を確定で取り、その後に NULL を試す2段階にしてください。
💡 提案差分（installation一致を優先し、NULLをフォールバックにする）
- const repo = await tenantDb
- .selectFrom('repositories')
- .select('id')
- .where('owner', '=', coords.owner)
- .where('repo', '=', coords.name)
- .where((eb) =>
- eb.or([
- eb('githubInstallationId', '=', installation.id),
- eb('githubInstallationId', 'is', null),
- ]),
- )
- .executeTakeFirst()
+ const baseRepoQuery = tenantDb
+ .selectFrom('repositories')
+ .select('id')
+ .where('owner', '=', coords.owner)
+ .where('repo', '=', coords.name)
+
+ const repo =
+ (await baseRepoQuery
+ .where('githubInstallationId', '=', installation.id)
+ .executeTakeFirst()) ??
+ (await baseRepoQuery
+ .where('githubInstallationId', 'is', null)
+ .executeTakeFirst())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/github-webhook-pull.server.ts` around lines 53 - 66, The current
single query with OR on githubInstallationId can return the NULL-backed row
first; change to a two-step lookup using tenantDb: first query repositories
where owner=coords.owner, repo=coords.name and githubInstallationId =
installation.id (use executeTakeFirst()), and if that returns null run a second
query matching githubInstallationId IS NULL as a fallback; return the id from
the first non-null result so installation-matching rows are always preferred.
Owner
Author
There was a problem hiding this comment.
スキーマを確認しました。repositories テーブルは (integration_id, owner, repo) で UNIQUE で、1 テナント = 1 integration なので、同一 (owner, repo) に一致行と NULL 行が共存することは構造的に発生しません。executeTakeFirst() も高々 1 行しか返せない前提です。
加えて、この fallback は PR #294 (strict) で完全に削除されるので、ここでの 2 段階化は見送ります。
coji
force-pushed
the
feat/issue-283-query-octokit
branch
from
744462f to
9d945da
Compare
coji
force-pushed
the
feat/issue-283-webhook-membership
branch
from
6768429 to
1c62eb5
Compare
3 tasks
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coji
force-pushed
the
feat/issue-283-query-octokit
branch
from
9d945da to
25d8948
Compare
coji
force-pushed
the
feat/issue-283-webhook-membership
branch
from
1c62eb5 to
e40a7c8
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/services/github-webhook-installation.server.ts`:
- Around line 193-210: The code logs a 'membership_repaired' event via
logGithubAppLinkEvent before the actual membership changes are applied
(applyMembershipChangesAfterCommit) which can cause log/state divergence if the
post-commit operation fails; fix by either (A) moving the logGithubAppLinkEvent
call to after applyMembershipChangesAfterCommit completes successfully (ensuring
you run it outside the transaction/after commit and still pass installation.id,
link.organizationId and readRepositoryCoords(payload).added/removed), or (B)
change the eventType from 'membership_repaired' to a provisional name like
'selection_updated' to reflect that changes are only scheduled, not yet applied.
Ensure you update the call site that currently uses trx so the final log runs in
the correct (post-commit) context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc6e466b-e85c-4286-9f64-60f8ab93735c
📒 Files selected for processing (18)
app/routes/api.github.setup.tsapp/services/github-app-link-events.server.tsapp/services/github-app-membership.server.test.tsapp/services/github-app-membership.server.tsapp/services/github-app-mutations.server.tsapp/services/github-installation-repos.server.tsapp/services/github-integration-queries.server.tsapp/services/github-octokit.server.test.tsapp/services/github-octokit.server.tsapp/services/github-webhook-installation.server.tsapp/services/github-webhook-pull.server.tsapp/services/github-webhook-shared.server.tsapp/services/github-webhook.server.test.tsapp/services/github-webhook.server.tsapp/services/jobs/backfill.server.tsapp/services/jobs/crawl.server.tsbatch/db/queries.tstsconfig.json
✅ Files skipped from review due to trivial changes (3)
- tsconfig.json
- app/services/github-webhook-pull.server.ts
- app/services/github-webhook-shared.server.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- app/services/github-webhook.server.ts
- app/routes/api.github.setup.ts
- app/services/github-installation-repos.server.ts
- app/services/github-app-link-events.server.ts
- app/services/github-app-membership.server.test.ts
…rship initialization (#283 PR 3/7) setup callback (api.github.setup.ts): - (organizationId, installationId) 単位 upsert + github_account_type 保存 - membership 初期投入 (fetchInstallationRepositories → initializeMembershipsForInstallation) - 成功時に membership_initialized_at をセット、失敗時は link のみ保存 - audit log: link_created / membership_initialized installation webhook (github-webhook-installation.server.ts): - findActiveLinkByInstallationOrAccount 削除、installation_id のみで lookup - installation.deleted: 該当 link のみ soft-delete + 最後の active を失った時のみ method=token 復帰 - installation.suspend/unsuspend: github_app_links.suspended_at を更新 - installation_repositories.added/removed: membership upsert/soft-delete + canonical reassignment 呼び出し - 全 event を audit log に記録 - bulk lookup: 1 query で owner/repo 解決し N+1 を解消 PR webhook (github-webhook-pull.server.ts): - owner + repo + installation_id で repository を引く（移行期間は github_installation_id IS NULL も許可） canonical reassignment helper (github-app-membership.server.ts) 新規: - reassignCanonicalAfterLinkLoss: 候補 0/1/2+ で reassign / cleared / assignment_required を判定 - 候補は repository_installation_memberships を正本とし、active / non-suspended / membership_initialized_at IS NOT NULL の link のみ - 未初期化 link 残存時は assignment_required に倒す保守ガード - LEFT JOIN + bulk update で N+1 を回避 - tenant first / shared second の cross-store 順序 - audit log: canonical_reassigned / canonical_cleared / assignment_required - upsertRepositoryMembership / softDeleteRepositoryMembership / initializeMembershipsForInstallation helpers installation repos fetcher (github-installation-repos.server.ts) 新規: - fetchInstallationRepositories: GitHub API で installation の見える repository を全ページ取得 audit log (github-app-link-events.server.ts): - tryLogGithubAppLinkEvent best-effort wrapper を追加 auto repair (jobs/crawl.server.ts): - crawl 冒頭に repair-membership:<id> step を追加 - membership_initialized_at IS NULL の active link を検出し installation_repositories を再 fetch して埋める - per-link で独立 step、durably の再開性を維持 tests: - github-app-membership.server.test.ts: reassignCanonicalAfterLinkLoss の 8 ケース (1/0/2+/未初期化ガード/suspended 除外/未初期化除外/soft-deleted membership/idempotency) - github-webhook.server.test.ts: 新 schema (suspended_at, membership_initialized_at, github_account_type, github_app_link_events) + 既存 12 ケース更新 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously the `membership_repaired` event was logged inside the transaction before `applyMembershipChangesAfterCommit` executed the actual membership mutations. If the post-commit step failed, the audit log would claim success while the state diverged. Move the log emission to run only after the post-commit membership changes succeed, so the audit trail reflects actual state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
coji
force-pushed
the
feat/issue-283-webhook-membership
branch
from
e40a7c8 to
763a5c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/services/github-app-membership.server.ts`:
- Around line 148-160: The loop calling logGithubAppLinkEvent for each decision
can throw and abort the webhook after tenant updates; replace those calls with
the non-throwing variant tryLogGithubAppLinkEvent (or wrap logGithubAppLinkEvent
in a try/catch) inside the same for (const decision of decisions) loop so audit
logging failures are swallowed/handled and do not cause a 500; keep the same
payload (organizationId, installationId: lostInstallationId, eventType:
decision.eventType, source, status: 'success', details: { repositoryId:
decision.repositoryId, nextCanonical: decision.nextCanonical, candidateCount:
decision.candidateCount }) and ensure any caught errors are logged but not
rethrown.
In `@app/services/github-webhook.server.test.ts`:
- Around line 182-185: The shared DB mutations in the test are unscoped: make
the delete on githubAppLinkEvents
(db.deleteFrom('githubAppLinkEvents').execute()) and the update invoked around
mockTenantUpdateTable/mockTenantSelectFrom include a WHERE organizationId =
<server-derived-org-id> condition; locate where the test sets up the fixture
organization (e.g., org or serverOrganization) and use that ID in the delete and
the update so both operations are limited to that org only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ab76844-332e-4468-971b-23b420109f7e
📒 Files selected for processing (11)
app/routes/api.github.setup.tsapp/services/github-app-link-events.server.tsapp/services/github-app-membership.server.test.tsapp/services/github-app-membership.server.tsapp/services/github-installation-repos.server.tsapp/services/github-webhook-installation.server.tsapp/services/github-webhook-pull.server.tsapp/services/github-webhook-shared.server.tsapp/services/github-webhook.server.test.tsapp/services/github-webhook.server.tsapp/services/jobs/crawl.server.ts
✅ Files skipped from review due to trivial changes (3)
- app/services/github-webhook.server.ts
- app/services/github-webhook-shared.server.ts
- app/services/github-installation-repos.server.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/services/github-webhook-pull.server.ts
- app/services/github-app-membership.server.test.ts
- app/services/github-webhook-installation.server.ts
After reassignCanonicalAfterLinkLoss completes the tenant mutation, logGithubAppLinkEvent failures would surface to the webhook caller as 500 errors even though the canonical was already reassigned. Same for the installation_repositories post-commit log. Switch both post-commit log calls to tryLogGithubAppLinkEvent so audit-log infrastructure failures never block the webhook response. Also scope two test fixtures to organizationId='o1' so they don't clobber other orgs if additional fixtures are added later. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/services/github-webhook-installation.server.ts`:
- Around line 354-360: The call to reassignCanonicalAfterLinkLoss in the
installation_repositories.removed handler currently treats the installation as
fully lost (removing lostInstallationId from all repos) which is incorrect for
partial removals; change the call so it only reassigns canonical for the
repositories that were actually removed (i.e. pass the list of removed
repository IDs from pending.removed into reassignCanonicalAfterLinkLoss), or add
a flag like {partialRemoval: true, removedRepoIds: [...] } and update
reassignCanonicalAfterLinkLoss to restrict its lostInstallationId exclusion to
those specific repo candidates; update call sites and the
reassignCanonicalAfterLinkLoss logic accordingly (see
applyMembershipChangesAfterCommit and softDeleteRepositoryMembership for the
flow).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 203d3a5f-ffe6-4e8e-b879-a54a84b9a8fd
📒 Files selected for processing (3)
app/services/github-app-membership.server.tsapp/services/github-webhook-installation.server.tsapp/services/github-webhook.server.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/services/github-webhook.server.test.ts
installation_repositories.removed previously invoked reassignCanonicalAfterLinkLoss without scoping, so repositories that were still legitimately bound to the installation would be reassigned (or orphaned) alongside the actually-removed ones. - reassignCanonicalAfterLinkLoss takes an optional repositoryIds parameter that narrows the scan to the given repository IDs. - applyMembershipChangesAfterCommit returns the removed repository IDs. - The removed handler passes that list through as the scope. - installation.deleted keeps the default (whole-installation) behavior. Tests: two new cases cover the scoped path and the empty-array no-op. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/services/github-app-membership.server.test.ts (1)
25-88: テストセットアップがモジュールロード時に実行されています。テンポラリディレクトリの作成、DBスキーマの初期化、
vi.stubEnvがモジュールレベルで実行されています。これはUPFLOW_DATA_DIRに依存するモジュールのインポート前に環境を整えるための意図的な設計と理解しますが、テスト終了後にテンポラリディレクトリが残ります。CI環境では問題になりませんが、ローカル開発では不要なファイルが蓄積する可能性があります。
♻️ クリーンアップの追加案
+import { rmSync } from 'node:fs' import { mkdirSync, writeFileSync } from 'node:fs' ... afterAll(async () => { await closeAllTenantDbs() await closeDb() + try { + rmSync(testDir, { recursive: true, force: true }) + } catch { + // ignore cleanup errors + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/github-app-membership.server.test.ts` around lines 25 - 88, The test currently creates testDir, writes DB files, initializes SQLite schemas and calls vi.stubEnv at module load (testDir, sharedDbPath, tenantDbPath, vi.stubEnv); move this setup into a beforeAll (or beforeEach if isolation needed) so it runs during test lifecycle, and add an afterAll that removes the temp directory (use fs.rmSync/testDir deletion with recursive+force) to clean up created files and avoid leaving artifacts; ensure the beforeAll sets vi.stubEnv(testDir) before importing modules that depend on UPFLOW_DATA_DIR and the afterAll undoes/stubs any env changes if necessary.app/services/github-app-membership.server.ts (1)
154-160: 変数名repositoryIdsがシャドウイングされています。ループ変数
repositoryIdsが入力パラメータrepositoryIds（Line 51, 53）をシャドウイングしています。機能的には正しいですが、可読性のために別名を検討してください。♻️ 修正案
- for (const [nextCanonical, repositoryIds] of reassignBuckets) { + for (const [nextCanonical, repoIds] of reassignBuckets) { await tenantDb .updateTable('repositories') .set({ githubInstallationId: nextCanonical }) - .where('id', 'in', repositoryIds) + .where('id', 'in', repoIds) .execute() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/github-app-membership.server.ts` around lines 154 - 160, The for-loop declares a loop binding named repositoryIds which shadows the function parameter repositoryIds; rename the loop variable to a non-conflicting identifier (e.g., bucketRepositoryIds or repoIds) in the destructuring expression for reassignBuckets (for (const [nextCanonical, <newName>] of reassignBuckets)) and update the usage in the tenantDb.updateTable('repositories').set(...).where('id', 'in', <newName>).execute() call to use the new name so the parameter and loop variable are no longer shadowing each other.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/services/github-app-membership.server.test.ts`:
- Around line 25-88: The test currently creates testDir, writes DB files,
initializes SQLite schemas and calls vi.stubEnv at module load (testDir,
sharedDbPath, tenantDbPath, vi.stubEnv); move this setup into a beforeAll (or
beforeEach if isolation needed) so it runs during test lifecycle, and add an
afterAll that removes the temp directory (use fs.rmSync/testDir deletion with
recursive+force) to clean up created files and avoid leaving artifacts; ensure
the beforeAll sets vi.stubEnv(testDir) before importing modules that depend on
UPFLOW_DATA_DIR and the afterAll undoes/stubs any env changes if necessary.
In `@app/services/github-app-membership.server.ts`:
- Around line 154-160: The for-loop declares a loop binding named repositoryIds
which shadows the function parameter repositoryIds; rename the loop variable to
a non-conflicting identifier (e.g., bucketRepositoryIds or repoIds) in the
destructuring expression for reassignBuckets (for (const [nextCanonical,
<newName>] of reassignBuckets)) and update the usage in the
tenantDb.updateTable('repositories').set(...).where('id', 'in',
<newName>).execute() call to use the new name so the parameter and loop variable
are no longer shadowing each other.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0330c9c1-0df5-4129-a739-62069753461e
📒 Files selected for processing (3)
app/services/github-app-membership.server.test.tsapp/services/github-app-membership.server.tsapp/services/github-webhook-installation.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/services/github-webhook-installation.server.ts
- Merge duplicated JSDoc blocks on reassignCanonicalAfterLinkLoss into a single block that documents both the default and scoped modes. - Drop the redundant callee-side empty-array guard; the sole caller already short-circuits on removedRepositoryIds.length === 0. - Hoist `orgId` to a local in the installation_repositories handler so the OrganizationId cast isn't repeated three times. - Trim narrating WHAT comments from the new scoped-reassignment test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ip_synced The installation_repositories webhook handler was emitting `membership_repaired`, reusing the event type designed for the crawl auto-repair path. That conflated two operationally distinct concerns in the audit log: - `membership_repaired` (crawl auto-repair): recovery for links whose setup-time membership initialization failed. Rare, and repeated occurrences are an alert signal. - `membership_synced` (installation_repositories webhook): routine sync triggered by the user changing repository selection via GitHub. Frequent, benign, driven by user action. Split them so the audit log distinguishes chronic setup failures from normal user activity. RDD event_type list updated to mirror the new type. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ories The existing installation_repositories test only verified the app_repository_selection column was updated; it never checked that the audit log received an event. As a result the rename to membership_synced was technically unverified. Extend the assertion to read back the github_app_link_events row and expect exactly `membership_synced` / `installation_repositories_webhook`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
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
Issue #283 の実装 stack PR 3/7 — webhook handler / canonical reassignment / membership 初期投入の中核実装。stack で最重要 PR。
設計根拠:
docs/rdd/issue-283-multiple-github-accounts.md作業計画:
docs/rdd/issue-283-work-plan.md依存: #288 (PR 1: schema), #296 (PR 2: query/octokit)
変更内容
setup callback (
app/routes/api.github.setup.ts)(organizationId, installationId)単位 upsert（複合主キー対応）github_account_typeを保存（personal / Organization の UI 分岐用）fetchInstallationRepositories→initializeMembershipsForInstallation→membership_initialized_at = nowmembership_initialized_at IS NULLのまま auto-repair に委譲link_created/membership_initialized(success / failed)installation webhook (
app/services/github-webhook-installation.server.ts)findActiveLinkByInstallationOrAccount削除installation_idで行うinstallation.deleted:integrations.method = 'token'に戻すlink_deleted監査ログinstallation.suspend/unsuspend:github_app_links.suspended_atを更新（旧integrations.app_suspended_atから移行）installation_repositories.added/removed:installation.created: setup callback が正本のため、既存 link が無ければ no-opcanonical reassignment (
app/services/github-app-membership.server.ts) 新規reassignCanonicalAfterLinkLoss(orgId, lostInstallationId, source):repository_installation_membershipsを正本とするmembership_initialized_at IS NOT NULLの link のみcanonical_reassignedcanonical_cleared(orassignment_requiredif 未初期化 link 残存)assignment_requiredcanonical_clearedではなくassignment_requiredに倒すupsertRepositoryMembership/softDeleteRepositoryMembership/initializeMembershipsForInstallationhelpersinstallation repos fetcher (
app/services/github-installation-repos.server.ts) 新規fetchInstallationRepositories(installationId): GitHub API でその installation が見える repository を全ページ取得audit log writer (
app/services/github-app-link-events.server.ts)tryLogGithubAppLinkEventbest-effort wrapper を追加（呼び出し側の.catch(() => {})ノイズを排除）auto repair (
app/services/jobs/crawl.server.ts)repair-membership:<installation_id>を追加membership_initialized_at IS NULLの active link を検出 →installation_repositoriesを再 fetch → membership upsert →membership_initialized_at = nowPR webhook (
app/services/github-webhook-pull.server.ts)owner + repo + installation_idで repository を引くgithub_installation_id IS NULLの repository も許可（PR 7 で strict 化）tests
app/services/github-app-membership.server.test.ts新規 (8 ケース):canonical_reassignedcanonical_clearedassignment_requiredassignment_required(cleared じゃない)app/services/github-webhook.server.test.ts既存 12 ケース更新:suspended_at,membership_initialized_at,github_account_type,github_app_link_events) 対応満たす受入条件
installation.suspend/unsuspendが対象 installation row のみ更新installation_repositoriesが対象 installation のみ更新installation.deletedが対象 installation row のみdeleted_atセットnull+ manual reselectmembership_initialized_atをセット、失敗時は repair に委譲Stack 位置
```text
PR 1 (#288): schema
└ PR 2 (#296): query/octokit
└ [PR 3: webhook/membership] ← this PR
└ PR 4 (UI)
└ PR 5 (repo UI)
└ PR 6 (backfill)
└ PR 7 (strict)
```
後続 PR への影響
getGithubAppLinks()に切替 + installation selector 追加 +assertInstallationBelongsToOrgを loader 境界で呼ぶassignment requiredバッジ + 個別再選択 mutation（同 helper を再利用）OR github_installation_id IS NULL削除 + crawl/backfill の移行期間 fallback 削除テスト
🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート
新機能
バグ修正
テスト