feat(github-app): per-repo Octokit resolution and multi-installation queries (#283 PR 2/7)#289
Closed
coji wants to merge 1 commit intofeat/issue-283-schemafrom
Closed
feat(github-app): per-repo Octokit resolution and multi-installation queries (#283 PR 2/7)#289coji wants to merge 1 commit intofeat/issue-283-schemafrom
coji wants to merge 1 commit intofeat/issue-283-schemafrom
Conversation
📝 WalkthroughWalkthroughインストール単位のGitHub Appリンク管理と監査ログを追加し、リポジトリ単位でOctokitを解決する制御フローへ移行。既存の単一インストール前提APIは一部非推奨化され、ジョブ処理はリポジトリ単位で認証解決とエラーハンドリングを行うように変更されました。 Changes
Sequence Diagram(s)sequenceDiagram
participant Job as バックグラウンドジョブ
participant Org as Organization レコード
participant Repo as Repository アイテム
participant Auth as Auth Resolver
participant DB as DB/queries
participant Octokit as Octokit インスタンス
participant GitHub as GitHub API
Job->>Org: 組織データを読み込み
Org->>DB: `getGithubAppLinks(organizationId)` を取得
Job->>Repo: 各リポジトリをループ
Repo->>Auth: resolveOctokitForRepository(repo, integration, githubAppLinks)
Auth->>DB: 必要に応じインストール照会 / assertInstallationBelongsToOrg
Auth-->>Octokit: 認証情報で Octokit を生成
Octokit->>GitHub: API 呼び出しを実行
GitHub-->>Octokit: レスポンス返却
alt 解決エラーまたは例外
Repo->>Job: エラーログ記録してスキップ
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Apr 7, 2026
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: 3
🤖 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-integration-queries.server.ts`:
- Around line 69-88: The initial SELECT from githubAppLinks should be
constrained by organizationId to avoid leaking existance of installations from
other orgs; update the query that sets link (the
db.selectFrom('githubAppLinks')...executeTakeFirst) to include a
where('organizationId', '=', organizationId) in addition to the installationId
filter so the read is org-scoped before performing the subsequent existence and
status checks (suspendedAt/deletedAt) and leave the following error throws
intact.
In `@app/services/jobs/backfill.server.ts`:
- Around line 54-65: 現在の try/catch 周りは resolveOctokitForRepository の認証失敗だけでなく
backfillRepo 内の GitHub/API/DB エラーまで警告で握り潰しているので、resolveOctokitForRepository
の呼び出しだけを分離してその失敗を捕捉し（step.log.warn と getErrorMessageForLog を使う）、認証エラー以外は再スローして
job を失敗させるようにしてください; 具体的には backfillRepo
呼び出しを別のブロックに移し、resolveOctokitForRepository のみを try/catch
して認証未解決時にのみスキップし、それ以外の例外は catch で rethrow して処理を中断するように実装してください (参照:
resolveOctokitForRepository, backfillRepo, step.log.warn,
getErrorMessageForLog).
In `@app/services/jobs/crawl.server.ts`:
- Around line 48-53: 現在のチェック uses fullOrg.githubAppLinks.length === 0 which
counts suspended links; change it to count only active installations by checking
fullOrg.githubAppLinks.filter(l => !l.suspendedAt).length === 0 so suspendedAt
links are ignored, and apply the same fix to the analogous check in
backfill.server.ts; update the condition near the GitHub App branch (the block
that throws 'GitHub App is not connected') so it matches the resolver's
active判定.
🪄 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: 05021ef0-4862-4fa7-8b1d-d005c505c86b
📒 Files selected for processing (9)
app/services/github-app-link-events.server.tsapp/services/github-app-mutations.server.tsapp/services/github-integration-queries.server.tsapp/services/github-octokit.server.test.tsapp/services/github-octokit.server.tsapp/services/jobs/backfill.server.tsapp/services/jobs/crawl.server.tsbatch/db/queries.tstsconfig.json
Comment on lines
+54
to
+65
| try { | ||
| const octokit = resolveOctokitForRepository({ | ||
| integration: fullOrg.integration, | ||
| githubAppLinks: fullOrg.githubAppLinks, | ||
| repository, | ||
| }) | ||
| await backfillRepo(orgId, repository, octokit, { | ||
| files: input.files, | ||
| }) | ||
| } catch (e) { | ||
| step.log.warn(`Skipping ${repoLabel}: ${getErrorMessageForLog(e)}`) | ||
| } |
There was a problem hiding this comment.
backfillRepo() の失敗まで成功扱いにしない方が良いです。
いまの catch は auth 解決失敗だけでなく、GitHub API や DB の一時障害まで warning で握りつぶします。結果として実際には何も backfill できていなくても job は成功扱いになるので、skip したいのが auth 未解決だけなら resolveOctokitForRepository() だけを分離して catch した方が安全です。
🛠️ 修正案
await step.run(`backfill:${repoLabel}`, async () => {
step.progress(i + 1, repoCount, `Backfilling ${repoLabel}...`)
- try {
- const octokit = resolveOctokitForRepository({
- integration: fullOrg.integration,
- githubAppLinks: fullOrg.githubAppLinks,
- repository,
- })
- await backfillRepo(orgId, repository, octokit, {
- files: input.files,
- })
- } catch (e) {
- step.log.warn(`Skipping ${repoLabel}: ${getErrorMessageForLog(e)}`)
- }
+ let octokit: ReturnType<typeof resolveOctokitForRepository>
+ try {
+ octokit = resolveOctokitForRepository({
+ integration: fullOrg.integration,
+ githubAppLinks: fullOrg.githubAppLinks,
+ repository,
+ })
+ } catch (e) {
+ step.log.warn(`Skipping ${repoLabel}: ${getErrorMessageForLog(e)}`)
+ return
+ }
+
+ await backfillRepo(orgId, repository, octokit, {
+ files: input.files,
+ })
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/jobs/backfill.server.ts` around lines 54 - 65, 現在の try/catch 周りは
resolveOctokitForRepository の認証失敗だけでなく backfillRepo 内の GitHub/API/DB
エラーまで警告で握り潰しているので、resolveOctokitForRepository の呼び出しだけを分離してその失敗を捕捉し（step.log.warn
と getErrorMessageForLog を使う）、認証エラー以外は再スローして job を失敗させるようにしてください; 具体的には
backfillRepo 呼び出しを別のブロックに移し、resolveOctokitForRepository のみを try/catch
して認証未解決時にのみスキップし、それ以外の例外は catch で rethrow して処理を中断するように実装してください (参照:
resolveOctokitForRepository, backfillRepo, step.log.warn,
getErrorMessageForLog).
Comment on lines
+48
to
+53
| if ( | ||
| fullOrg.integration.method === 'github_app' && | ||
| fullOrg.githubAppLinks.length === 0 | ||
| ) { | ||
| throw new Error('GitHub App is not connected') | ||
| } |
There was a problem hiding this comment.
停止中リンクだけでも「接続済み」扱いになります。
ここは length === 0 なので、suspendedAt 付きのリンクも数えてしまいます。resolver 側は !l.suspendedAt を active 判定に使っているので、ここも合わせないと active installation が 0 件でも load-organization を通ってしまいます。backfill.server.ts 側の同じ条件も合わせて直しておくとズレが出にくいです。
🩹 修正案
if (
fullOrg.integration.method === 'github_app' &&
- fullOrg.githubAppLinks.length === 0
+ !fullOrg.githubAppLinks.some((link) => !link.suspendedAt)
) {
throw new Error('GitHub App is not connected')
}🤖 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 48 - 53, 現在のチェック uses
fullOrg.githubAppLinks.length === 0 which counts suspended links; change it to
count only active installations by checking fullOrg.githubAppLinks.filter(l =>
!l.suspendedAt).length === 0 so suspendedAt links are ignored, and apply the
same fix to the analogous check in backfill.server.ts; update the condition near
the GitHub App branch (the block that throws 'GitHub App is not connected') so
it matches the resolver's active判定.
This was referenced Apr 7, 2026
…queries (#283 PR 2/7) query 層: - getGithubAppLinks() で配列返却 - getGithubAppLinkByInstallationId() / assertInstallationBelongsToOrg() 追加 - getGithubAppLink() は 1 件目を返す互換 shim として deprecated 維持 mutation 層: - disconnectGithubAppLink(orgId, installationId) を追加 - 最後の active link を失った時のみ method=token に戻す - github_app_link_events への audit log writer (table の初回 writer) - disconnectGithubApp() は全 link を順次切る互換 wrapper として deprecated Octokit 解決: - resolveOctokitForRepository() 追加。repository ごとに installation を選び、移行期間 fallback (active 1 件) を含む - resolveOctokitForInstallation() 追加 - resolveOctokitFromOrg() は deprecated 互換 wrapper batch shape: - getOrganization() / listAllOrganizations() が githubAppLinks: [] を返すよう変更 - crawl / backfill ジョブを per-repo 解決に書き換え audit log: - app/services/github-app-link-events.server.ts 新規追加 - logGithubAppLinkEvent() helper を export tests: - resolveOctokitForRepository の 11 ケース追加 (active 1/0/2+/suspended/explicit installation の組み合わせ) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
coji
force-pushed
the
feat/issue-283-query-octokit
branch
from
744462f to
9d945da
Compare
3 tasks
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-octokit.server.ts`:
- Around line 129-165: The branch currently treats any non-'github_app' value as
a token fallback, which can mis-select auth; change the logic in the block that
checks integration.method so you explicitly compare for 'token' (e.g., if
(integration.method === 'token') { return createOctokit({...}) }) and otherwise
throw a configuration error (rather than silently using
integration.privateToken). Ensure you still keep the existing github_app flow
that calls resolveOctokitForInstallation and update the deprecated wrapper/any
other helper with matching explicit checks so both places enforce known methods
('github_app' and 'token') and error on unknown values.
🪄 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: 25e0ab90-2f56-4772-a9a3-161579ec02d7
📒 Files selected for processing (9)
app/services/github-app-link-events.server.tsapp/services/github-app-mutations.server.tsapp/services/github-integration-queries.server.tsapp/services/github-octokit.server.test.tsapp/services/github-octokit.server.tsapp/services/jobs/backfill.server.tsapp/services/jobs/crawl.server.tsbatch/db/queries.tstsconfig.json
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (2)
- app/services/github-octokit.server.test.ts
- app/services/jobs/backfill.server.ts
Comment on lines
+129
to
+165
| if (integration.method === 'github_app') { | ||
| if (repository.githubInstallationId !== null) { | ||
| const matched = githubAppLinks.find( | ||
| (l) => l.installationId === repository.githubInstallationId, | ||
| ) | ||
| if (!matched) { | ||
| throw new Error( | ||
| `GitHub App installation ${repository.githubInstallationId} is not active for this organization`, | ||
| ) | ||
| } | ||
| if (matched.suspendedAt) { | ||
| throw new Error( | ||
| `GitHub App installation ${repository.githubInstallationId} is suspended`, | ||
| ) | ||
| } | ||
| return resolveOctokitForInstallation(matched.installationId) | ||
| } | ||
|
|
||
| const activeLinks = githubAppLinks.filter((l) => !l.suspendedAt) | ||
| if (activeLinks.length === 1) { | ||
| return resolveOctokitForInstallation(activeLinks[0].installationId) | ||
| } | ||
| if (activeLinks.length === 0) { | ||
| throw new Error('GitHub App is not connected') | ||
| } | ||
| throw new Error( | ||
| `Repository has no canonical installation assigned and ${activeLinks.length} active installations exist. Backfill required.`, | ||
| ) | ||
| } | ||
|
|
||
| if (integration.privateToken) { | ||
| return createOctokit({ | ||
| method: 'token', | ||
| privateToken: integration.privateToken, | ||
| }) | ||
| } | ||
| throw new Error('No auth configured') |
There was a problem hiding this comment.
未知の integration.method がトークン認証へ静かにフォールバックしています。
この分岐だと 'github_app' 以外は privateToken があるだけで token 扱いになるので、DB 値の破損や将来のメソッド追加時に誤った認証を選びます。'token' を明示判定し、それ以外は設定エラーにした方が安全です。同じフォールバックが deprecated wrapper 側にも残っているなら、そこも揃えておくとズレません。
🛠️ 修正例
- if (integration.privateToken) {
- return createOctokit({
- method: 'token',
- privateToken: integration.privateToken,
- })
+ if (integration.method === 'token') {
+ if (integration.privateToken) {
+ return createOctokit({
+ method: 'token',
+ privateToken: integration.privateToken,
+ })
+ }
+ throw new Error('No auth configured')
}
- throw new Error('No auth configured')
+ throw new Error(`Unsupported integration method: ${integration.method}`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/github-octokit.server.ts` around lines 129 - 165, The branch
currently treats any non-'github_app' value as a token fallback, which can
mis-select auth; change the logic in the block that checks integration.method so
you explicitly compare for 'token' (e.g., if (integration.method === 'token') {
return createOctokit({...}) }) and otherwise throw a configuration error (rather
than silently using integration.privateToken). Ensure you still keep the
existing github_app flow that calls resolveOctokitForInstallation and update the
deprecated wrapper/any other helper with matching explicit checks so both places
enforce known methods ('github_app' and 'token') and error on unknown values.
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 2/7 — query / mutation / Octokit 解決層を複数 installation 対応にする。アプリケーションの動作はまだ変えない（fallback で互換維持）。
設計根拠:
docs/rdd/issue-283-multiple-github-accounts.md作業計画:
docs/rdd/issue-283-work-plan.md依存: #288 (PR 1: schema)
変更内容
query 層 (
app/services/github-integration-queries.server.ts)getGithubAppLinks(orgId)配列返却（ORDER BY createdAt ASC で決定的順序）getGithubAppLinkByInstallationId(installationId)追加assertInstallationBelongsToOrg(orgId, installationId)追加 — クライアント由来のinstallationIdをサーバ側で検証する境界 guardgetGithubAppLink()は最古の active link を返す互換 shim（@deprecated）mutation 層 (
app/services/github-app-mutations.server.ts)disconnectGithubAppLink(orgId, installationId)追加 — 単一 installation の soft-delete + 最後の active を失った時のみmethod='token'に戻す + audit log 書き込みdisconnectGithubApp()は legacy UI 互換 wrapper として 1 transaction で全 link 一括 soft-delete に書き換え（@deprecated）audit log writer (
app/services/github-app-link-events.server.ts) 新規追加logGithubAppLinkEvent()helperKysely<DB> | Transaction<DB>を受け取り、呼び出し側のトランザクションに乗せられるgithub_app_link_eventstable の 初回 writer（disconnect 経由）Octokit 解決 (
app/services/github-octokit.server.ts)resolveOctokitForInstallation(installationId)追加resolveOctokitForRepository({ integration, githubAppLinks, repository })追加 — repository ごとの解決repository.githubInstallationIdがセットされている場合は厳密にそれを使う（suspended は弾く）github_appモードで未割当の repository に対する移行期間 fallback:tokenモード:privateTokenがあれば PAT、無ければ未接続エラーIntegrationForOctokit.methodを'token' | 'github_app' | (string & {})のユニオンに型を絞るresolveOctokitFromOrg()は legacy 互換 wrapper（@deprecated、PR 4 で削除予定）batch shape 更新 (
batch/db/queries.ts)getGithubAppLinkByOrgId→getGithubAppLinksByOrgId（配列返却）getAllGithubAppLinksをMap.groupByで書き換えgetOrganization()/listAllOrganizations()がgithubAppLinks: []を返すよう変更crawl / backfill ジョブ (
app/services/jobs/{crawl,backfill}.server.ts)load-organizationstep 内でgithub_app + active 0/token + privateToken nullの早期エラー検出resolveOctokitForRepository()を呼び、解決失敗時は warn ログ + skip（crawl 全体は止めない）tsconfig
libをES2024に bump（Map.groupByを使うため）tests
app/services/github-octokit.server.test.tsにresolveOctokitForRepositoryの 11 ケース追加満たす受入条件
crawl.server.ts/backfill.server.tsが repository ごとに対応 installation の Octokit を使うStack 位置
後続 PR への影響
getGithubAppLinkByInstallationId/logGithubAppLinkEvent/ canonical reassignment helper（PR 3 で実装）を使うgetGithubAppLink()をgetGithubAppLinks()に移行 +assertInstallationBelongsToOrgを loader 境界で呼ぶactiveLinks.length === 1分岐) を削除し、github_installation_id IS NULLを strict エラーにするテスト
🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート
New Features
Bug Fixes
Tests
Chores