Skip to content

Commit bae1045

Browse files
authored
feat(github-app): per-repo Octokit resolution and multi-installation queries (#283 PR 2/7) (#296)
## Summary Issue #283 の実装 stack **PR 2/7** — query / mutation / Octokit 解決層を複数 installation 対応にする。アプリケーションの動作はまだ変えない(fallback で互換維持)。 設計根拠: [`docs/rdd/issue-283-multiple-github-accounts.md`](./docs/rdd/issue-283-multiple-github-accounts.md) 作業計画: [`docs/rdd/issue-283-work-plan.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` をサーバ側で検証する境界 guard - `getGithubAppLink()` は最古の active link を返す互換 shim(`@deprecated`) ### mutation 層 (`app/services/github-app-mutations.server.ts`) - `disconnectGithubAppLink(orgId, installationId)` 追加 — 単一 installation の soft-delete + 最後の active を失った時のみ `method='token'` に戻す + audit log 書き込み - 1 transaction 内で完結(idempotent) - `disconnectGithubApp()` は legacy UI 互換 wrapper として 1 transaction で全 link 一括 soft-delete に書き換え(`@deprecated`) ### audit log writer (`app/services/github-app-link-events.server.ts`) 新規追加 - `logGithubAppLinkEvent()` helper - event_type / source / status の string union 型を export - `Kysely<DB> | Transaction<DB>` を受け取り、呼び出し側のトランザクションに乗せられる - PR 1 で追加した `github_app_link_events` table の **初回 writer**(disconnect 経由) ### Octokit 解決 (`app/services/github-octokit.server.ts`) - `resolveOctokitForInstallation(installationId)` 追加 - `resolveOctokitForRepository({ integration, githubAppLinks, repository })` 追加 — repository ごとの解決 - `repository.githubInstallationId` がセットされている場合は厳密にそれを使う(suspended は弾く) - `github_app` モードで未割当の repository に対する移行期間 fallback: - active link 1 件 → そのまま使う - 0 件 → エラー(**PAT 自動 fallback はしない**、RDD ルール) - 2+ 件 → エラー(曖昧、明示的な assignment が必要) - `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`) - 単一 Octokit 共有から per-repository 解決に変更 - `load-organization` step 内で `github_app + active 0` / `token + privateToken null` の早期エラー検出 - repository ループ内で `resolveOctokitForRepository()` を呼び、解決失敗時は warn ログ + skip(crawl 全体は止めない) ### tsconfig - `lib` を `ES2024` に bump(`Map.groupBy` を使うため) ### tests - `app/services/github-octokit.server.test.ts` に `resolveOctokitForRepository` の 11 ケース追加 - 明示 installation id (一致 / 不一致 / suspended) - 移行期間 fallback (active 1 / 0 / 2+ / suspended 除外) - token モード (PAT あり / なし) - integration null ## 満たす受入条件 - **#6**: `crawl.server.ts` / `backfill.server.ts` が repository ごとに対応 installation の Octokit を使う ## Stack 位置 ```text PR 1 (#288): schema └ [PR 2: query/octokit] ← this PR └ PR 3 (webhook/membership) └ PR 4 (UI) └ PR 5 (repo UI) └ PR 6 (backfill) └ PR 7 (strict) ``` ## 後続 PR への影響 - PR 3: webhook handler 群がここで追加した `getGithubAppLinkByInstallationId` / `logGithubAppLinkEvent` / canonical reassignment helper(PR 3 で実装)を使う - PR 4: UI loader / action から `getGithubAppLink()` を `getGithubAppLinks()` に移行 + `assertInstallationBelongsToOrg` を loader 境界で呼ぶ - PR 7: 移行期間 fallback (`activeLinks.length === 1` 分岐) を削除し、`github_installation_id IS NULL` を strict エラーにする ## テスト - [x] \`pnpm validate\` (lint / format / typecheck / build / test 全 331 tests) - [x] \`resolveOctokitForRepository\` の主要 11 ケースをユニットテストで検証 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## リリースノート * **新機能** * GitHub Appのリンク接続・切断イベントに対する監査ログ機能を追加しました。 * **バグ修正** * リポジトリごとのGitHub認証解決ロジックを改善し、複数のアクティブなリンク存在時のエラーハンドリングを強化しました。 * GitHub Appの削除状態をより厳密に追跡するようにしました。 * **改善** * GitHub統合に関する検証とエラー処理を堅牢化しました。 <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 386b835 commit bae1045

9 files changed

Lines changed: 539 additions & 67 deletions
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
import type { Kysely, Transaction } from 'kysely'
2+
import { db } from '~/app/services/db.server'
3+
import type { DB } from '~/app/services/type'
4+
import type { OrganizationId } from '~/app/types/organization'
5+
6+
export type GithubAppLinkEventType =
7+
| 'link_created'
8+
| 'link_deleted'
9+
| 'link_suspended'
10+
| 'link_unsuspended'
11+
| 'membership_initialized'
12+
| 'membership_repaired'
13+
| 'canonical_reassigned'
14+
| 'canonical_cleared'
15+
| 'assignment_required'
16+
17+
export type GithubAppLinkEventSource =
18+
| 'setup_callback'
19+
| 'installation_webhook'
20+
| 'installation_repositories_webhook'
21+
| 'user_disconnect'
22+
| 'crawl_repair'
23+
| 'manual_reassign'
24+
| 'cli_repair'
25+
26+
export type GithubAppLinkEventStatus = 'success' | 'failed' | 'skipped'
27+
28+
export type LogGithubAppLinkEventInput = {
29+
organizationId: OrganizationId
30+
installationId: number
31+
eventType: GithubAppLinkEventType
32+
source: GithubAppLinkEventSource
33+
status: GithubAppLinkEventStatus
34+
details?: Record<string, unknown>
35+
}
36+
37+
/**
38+
* Append an entry to the `github_app_link_events` audit log.
39+
*
40+
* Pass a transaction to commit the log together with the underlying mutation;
41+
* otherwise it writes against the default connection.
42+
*/
43+
export const logGithubAppLinkEvent = async (
44+
input: LogGithubAppLinkEventInput,
45+
dbOrTrx: Kysely<DB> | Transaction<DB> = db,
46+
): Promise<void> => {
47+
await dbOrTrx
48+
.insertInto('githubAppLinkEvents')
49+
.values({
50+
organizationId: input.organizationId,
51+
installationId: input.installationId,
52+
eventType: input.eventType,
53+
source: input.source,
54+
status: input.status,
55+
detailsJson: input.details ? JSON.stringify(input.details) : null,
56+
})
57+
.execute()
58+
}
Lines changed: 95 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,117 @@
11
import { clearOrgCache } from '~/app/services/cache.server'
22
import { db } from '~/app/services/db.server'
3+
import { logGithubAppLinkEvent } from '~/app/services/github-app-link-events.server'
34
import type { OrganizationId } from '~/app/types/organization'
45

56
/**
6-
* Soft-delete the org's GitHub App link and revert integration to token method.
7-
* Clears appSuspendedAt so a prior suspend flag cannot linger after disconnect.
7+
* Soft-delete a single GitHub App installation link.
8+
*
9+
* Reverts `integrations.method` to `'token'` (and clears `app_suspended_at`)
10+
* only when the deleted link was the last active one for the org.
11+
*
12+
* Writes a `link_deleted` audit log event with `source='user_disconnect'`.
13+
*
14+
* Idempotent: a no-op if the link is already deleted or does not exist.
815
*/
9-
export async function disconnectGithubApp(organizationId: OrganizationId) {
16+
export async function disconnectGithubAppLink(
17+
organizationId: OrganizationId,
18+
installationId: number,
19+
) {
1020
const now = new Date().toISOString()
21+
1122
await db.transaction().execute(async (trx) => {
12-
await trx
23+
const updateResult = await trx
1324
.updateTable('githubAppLinks')
1425
.set({ deletedAt: now, updatedAt: now })
1526
.where('organizationId', '=', organizationId)
27+
.where('installationId', '=', installationId)
28+
.where('deletedAt', 'is', null)
29+
.executeTakeFirst()
30+
31+
if (updateResult.numUpdatedRows === 0n) return
32+
33+
const remaining = await trx
34+
.selectFrom('githubAppLinks')
35+
.select('installationId')
36+
.where('organizationId', '=', organizationId)
37+
.where('deletedAt', 'is', null)
38+
.executeTakeFirst()
39+
40+
const revertedToToken = !remaining
41+
if (revertedToToken) {
42+
await trx
43+
.updateTable('integrations')
44+
.set({ method: 'token', appSuspendedAt: null, updatedAt: now })
45+
.where('organizationId', '=', organizationId)
46+
.execute()
47+
}
48+
49+
await logGithubAppLinkEvent(
50+
{
51+
organizationId,
52+
installationId,
53+
eventType: 'link_deleted',
54+
source: 'user_disconnect',
55+
status: 'success',
56+
details: { revertedToToken },
57+
},
58+
trx,
59+
)
60+
})
61+
62+
clearOrgCache(organizationId)
63+
}
64+
65+
/**
66+
* Soft-delete every active GitHub App link for the org in a single transaction
67+
* and revert integration to token method. Convenience for legacy "disconnect all"
68+
* UI flows that treat GitHub App as a single org-wide connection.
69+
*
70+
* Writes one `link_deleted` audit log entry per affected installation.
71+
*
72+
* @deprecated Prefer {@link disconnectGithubAppLink} for installation-scoped
73+
* disconnects.
74+
*/
75+
export async function disconnectGithubApp(organizationId: OrganizationId) {
76+
const now = new Date().toISOString()
77+
78+
await db.transaction().execute(async (trx) => {
79+
const links = await trx
80+
.selectFrom('githubAppLinks')
81+
.select('installationId')
82+
.where('organizationId', '=', organizationId)
1683
.where('deletedAt', 'is', null)
1784
.execute()
1885

86+
if (links.length > 0) {
87+
await trx
88+
.updateTable('githubAppLinks')
89+
.set({ deletedAt: now, updatedAt: now })
90+
.where('organizationId', '=', organizationId)
91+
.where('deletedAt', 'is', null)
92+
.execute()
93+
}
94+
1995
await trx
2096
.updateTable('integrations')
2197
.set({ method: 'token', appSuspendedAt: null, updatedAt: now })
2298
.where('organizationId', '=', organizationId)
2399
.execute()
100+
101+
for (const link of links) {
102+
await logGithubAppLinkEvent(
103+
{
104+
organizationId,
105+
installationId: link.installationId,
106+
eventType: 'link_deleted',
107+
source: 'user_disconnect',
108+
status: 'success',
109+
details: { revertedToToken: true },
110+
},
111+
trx,
112+
)
113+
}
24114
})
115+
25116
clearOrgCache(organizationId)
26117
}
Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,17 @@
11
import { db } from '~/app/services/db.server'
22
import type { OrganizationId } from '~/app/types/organization'
33

4+
const githubAppLinkColumns = [
5+
'organizationId',
6+
'installationId',
7+
'githubAccountId',
8+
'githubAccountType',
9+
'githubOrg',
10+
'appRepositorySelection',
11+
'suspendedAt',
12+
'membershipInitializedAt',
13+
] as const
14+
415
export const getIntegration = async (organizationId: OrganizationId) => {
516
return await db
617
.selectFrom('integrations')
@@ -9,13 +20,72 @@ export const getIntegration = async (organizationId: OrganizationId) => {
920
.executeTakeFirst()
1021
}
1122

23+
/**
24+
* All active (not deleted) GitHub App links for an organization, ordered by
25+
* `createdAt` for deterministic iteration.
26+
*/
27+
export const getGithubAppLinks = async (organizationId: OrganizationId) => {
28+
return await db
29+
.selectFrom('githubAppLinks')
30+
.select(githubAppLinkColumns)
31+
.where('organizationId', '=', organizationId)
32+
.where('deletedAt', 'is', null)
33+
.orderBy('createdAt', 'asc')
34+
.execute()
35+
}
36+
37+
/**
38+
* Oldest active GitHub App link for an organization, or null.
39+
*
40+
* @deprecated Returns an arbitrary link when an org has multiple active
41+
* installations. Use {@link getGithubAppLinks} and operate per-installation.
42+
*/
1243
export const getGithubAppLink = async (organizationId: OrganizationId) => {
44+
const links = await getGithubAppLinks(organizationId)
45+
return links[0] ?? null
46+
}
47+
48+
export const getGithubAppLinkByInstallationId = async (
49+
installationId: number,
50+
) => {
1351
return (
1452
(await db
1553
.selectFrom('githubAppLinks')
16-
.select(['githubOrg', 'appRepositorySelection', 'installationId'])
17-
.where('organizationId', '=', organizationId)
54+
.select([...githubAppLinkColumns, 'deletedAt'])
55+
.where('installationId', '=', installationId)
1856
.where('deletedAt', 'is', null)
1957
.executeTakeFirst()) ?? null
2058
)
2159
}
60+
61+
/**
62+
* Boundary guard for client-provided `installationId`. Throws if the
63+
* installation does not belong to the given org or is deleted/suspended.
64+
*
65+
* The query is constrained by `organizationId` first so the database can never
66+
* leak the existence of installations from other organizations — even the
67+
* "not found" branch only fires when the row is missing *for this org*.
68+
*/
69+
export const assertInstallationBelongsToOrg = async (
70+
organizationId: OrganizationId,
71+
installationId: number,
72+
): Promise<void> => {
73+
const link = await db
74+
.selectFrom('githubAppLinks')
75+
.select(['suspendedAt', 'deletedAt'])
76+
.where('organizationId', '=', organizationId)
77+
.where('installationId', '=', installationId)
78+
.executeTakeFirst()
79+
80+
if (!link) {
81+
throw new Error(
82+
`Installation ${installationId} does not belong to this organization`,
83+
)
84+
}
85+
if (link.deletedAt !== null) {
86+
throw new Error(`Installation ${installationId} is disconnected`)
87+
}
88+
if (link.suspendedAt !== null) {
89+
throw new Error(`Installation ${installationId} is suspended`)
90+
}
91+
}

0 commit comments

Comments
 (0)