Conversation
- Phase 2 の詳細実装計画を新規追加（UX フロー設計、PR 分割、状態遷移） - 署名付き state パラメータによる安全なテナント特定 - shared DB github_app_links テーブルで O(1) ルックアップ - method は実効認証方式（リンク完了時に自動切替、crawl が止まらない） - migration.md を Phase 2 計画に追従更新 - PoC スクリプト（Installation Token 検証済み）を追加 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- shared DB に github_app_links テーブルを追加（installation_id, github_account_id に UNIQUE 制約、soft delete 対応） - tenant DB integrations に app_suspended_at カラムを追加 - @octokit/auth-app を dependencies に追加 - getTenantData に github_app_links の JOIN クエリを追加 - appRepositorySelection の型を "all" | "selected" に narrowing - integrations.method の型を "token" | "github_app" に拡張 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR 1 のスコープ拡大: integrations を tenant DB → shared DB に移行。 - PR 2 で全 call site を書き換えるので移すなら今（2回触りを回避） - github_app_links と同一 DB で単一トランザクション更新が可能に - scheduler の N+1 解消（全 org の認証状態を1クエリで取得） - クロス DB 整合性の問題が原理的に消滅 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
設定系テーブルを shared DB に寄せる構造変更。PR 2 で全 call site を 書き換えるタイミングに合わせ、同じファイルの2回書き換えを回避。 - shared DB に integrations テーブルを作成（organization_id UNIQUE + FK） - tenant DB から integrations テーブルを削除、repositories の FK も削除 - データ移行スクリプト（db/migrate-integrations-to-shared.ts）を追加 - db:apply の実行順序を shared → データ移行 → tenant に変更 - integrations を参照する全ファイルを tenantDb → db に変更 - listAllOrganizations: integrations を一括取得で N+1 解消 - getOrganization: integrations LEFT JOIN githubAppLinks の1クエリ - codegen config: shared に integrations 型 override 追加、tenant から削除 - テスト・seed・ドキュメントを更新 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Step 2-4 の実装計画を具体的なコード変更レベルで記述: - Step 2: createOctokit factory + fetcher の token→octokit 変更 + 全6 call site - Step 3: Webhook + Setup callback + 署名付き state + 接続解除 - Step 4: 設定 UI の状態表示 + リポ追加画面の App 対応 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- ドキュメント相互参照を先頭に追加（正本・ロードマップの所在） - Step 4 の getGithubAppLink をルート層の queries.server.ts に置く方針を明記 （batch/db/queries.ts からの import は依存方向が不自然） Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
重大: - state に nonce + 単回消費を追加。GITHUB_APP_STATE_SECRET を専用 env var に - setup callback / disconnect / webhook の shared DB 更新をトランザクション必須に - Step 4 の UI action（install, copy-url, disconnect, revert-to-token）を明記 高: - setup callback のリダイレクト条件を3パターンに明文化 - webhook 検証に raw body 順序、timingSafeEqual、sha256= prefix 確認を明記 中: - installation.created の検索を installation_id or github_account_id に修正 - createOctokit を InvariantError に統一 - 再インストール時の appSuspendedAt クリアを追加 - テスト計画を状態遷移全パターンに拡充 - 「要再接続」の判定条件（method=github_app + activeLink=null）を明記 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. nonce 消費をトランザクション内に移動（API 検証成功後に消費。失敗時は再試行可能） 2. 例外の使い分け方針を明記（invariant=開発者バグ、Error=ユーザー起因） 3. revert-to-token の PAT 未保存時の挙動を定義（未接続+PAT入力促し） Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codex との議論で5案中2案を採用: - #3: disconnect/revert の内部処理を共通ヘルパーに統合（intent名・ログは維持） - #5: resolveOctokitFromOrg ヘルパーで method 分岐を1箇所に集約、全 call site を簡素化 不採用（現行維持）: - #1 nonce: 委託 URL の一回性は運用明確性に必要 - #2 secret 分離: ローテーション責務が異なる - #4 copy-install-url: 委託フローは一級要件 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. resolveOctokitFromOrg の PAT fallback を削除 method='github_app' + link なし → エラー（要再接続）。 PAT への暗黙フォールバックは UI 状態と実動作のずれを生むため不可。 2. Step 2 のスコープを明記 repositories.add 系（get-unique-owners, get-repositories-by-owner-and-keyword）は Step 4 で UI ごと置き換えるため Step 2 対象外。 補足: $pull/queries.server.ts は action 側で分離取得する方針を明記。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- github-octokit.server.ts: createOctokit / assertOrgGithubAuthResolvable / resolveOctokitFromOrg - fetcher.ts: token → octokit に変更 - crawl/backfill: getOrganization を1回に最適化（二重呼び出し解消） - $pull/index.tsx: 3クエリを Promise.all で並列化 - search-github-users: raw fetch → Octokit に統一 - backfill-repo: 引数を octokit: Octokit に変更 - テスト追加（github-octokit.server.test.ts） - ドキュメントを実装結果に合わせて更新 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Webhook + Setup callback + Installation 紐付けの実装手順を コードベース調査に基づいて具体化。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
重大: - state 形式を base64url(JSON).signature に変更（: 区切りは ISO 8601 と衝突） - setup callback の integrations 更新を UPSERT に変更（新規 org 対応） 中: - 接続解除 action を intent 先行分岐パターンに修正（既存 parseWithZod と共存） - セッション API を実コードに合わせて修正 + clearOrgCache を明記 - テスト計画を整理（UPSERT 冪等 / 別 org 制約 / パラメータ欠落 / malformed JSON） Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. state を nonce-only に簡素化（HMAC 署名廃止、GITHUB_APP_STATE_SECRET 不要） 防御層は1枚減るが UUID は推測不能で DB lookup は sub-ms。env var 1つ削減。 2. createAppOctokit() ヘルパーを github-octokit.server.ts に追加 base64 デコード等の重複を解消。setup callback / webhook で共用。 3. 3-0 の共有クエリ整理を「先にコミット + validate」と明記 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
phase2-plan, impl-steps, migration の3ファイルで 「署名付き state / GITHUB_APP_STATE_SECRET」の記述を 「DB ベースの nonce（UUID）」に更新。step3-plan が正。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add github-integration-queries.server.ts with getIntegration and getGithubAppLink - Remove duplicate implementations from settings routes - repositories.add uses shared getIntegration + getIntegrationWithRepositories Made-with: Cursor
- Add github_app_install_states table and Kysely types - Webhook: HMAC verify (timing-safe), installation + installation_repositories handlers - Setup loader: verify installation via App JWT, consume nonce outside trx, UPSERT link + integration in trx - createAppOctokit() for GET /app/installations/:id - disconnect-github-app intent on integration settings (before parseWithZod) - GITHUB_WEBHOOK_SECRET in .env.example - Tests: verify, install state, webhook processor, routes, createAppOctokit Note: run db:generate:tenant only when tenant_seed.db has schema, or tenant-type.ts can empty. Made-with: Cursor
設定 UI + リポ追加画面の App UX: - loader 拡張（integration + githubAppLink を並列取得） - action 追加（install, copy-url, revert-to-token） - 設定画面コンポーネントの状態別表示（5状態） - リポ追加画面の method 分岐（Installation API + クライアント側フィルタ） - disconnectGithubApp ヘルパーの抽出 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
設定画面: - loader: integration + githubAppLink を並列取得 - action: install / copy-url / disconnect / revert-to-token の各 intent - disconnectGithubApp ヘルパーを github-app-mutations.server.ts に抽出 - App 接続中は PAT セクション非表示（method 上書き防止） - GitHubAppSection: 5状態の表示 + ConfirmDialog リポ追加画面: - get-installation-repos.ts: 全件1回取得 → owners/repos を導出（二重API呼び出し解消） - loader を loadReposForToken / loadReposForApp に分離 - selected repos の注記表示 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughこのプルリクエストはGitHub App対応を追加します。共有スキーマに Changes
Sequence Diagram(s)sequenceDiagram
participant User as ユーザー
participant Client as クライアント
participant Setup as セットアップ<br/>エンドポイント
participant GitHub as GitHub API
participant DB as 共有DB
participant Cache as キャッシュ
User->>Client: インストール開始
Client->>Setup: インストール状態を生成
Setup->>DB: generateInstallState()<br/>（nonce生成）
DB-->>Setup: nonce
Setup-->>Client: インストールURL<br/>（state=nonce）
Client->>GitHub: GitHubアプリインストール
GitHub-->>Client: リダイレクト<br/>（installation_id, state）
Client->>Setup: セットアップコールバック<br/>（installation_id, state）
Setup->>DB: consumeInstallState(state)
DB-->>Setup: organizationId
Setup->>GitHub: getInstallation(installation_id)
GitHub-->>Setup: インストール情報
Setup->>DB: トランザクション開始
DB->>DB: upsert githubAppLinks
DB->>DB: upsert integrations<br/>（method='github_app'）
DB->>DB: トランザクションコミット
Setup->>Cache: clearOrgCache(organizationId)
Setup-->>Client: リダイレクト<br/>（settings/integration）
sequenceDiagram
participant GitHub as GitHub
participant Webhook as Webhookエンドポイント
participant Verify as 署名検証
participant Process as イベント処理
participant DB as 共有DB
participant Cache as キャッシュ
GitHub->>Webhook: Webhookペイロード<br/>+ X-Hub-Signature-256
Webhook->>Verify: verifyWebhookSignature()
alt 署名無効
Verify-->>Webhook: false
Webhook-->>GitHub: 401 Unauthorized
else 署名有効
Verify-->>Webhook: true
Webhook->>Process: processGithubWebhookPayload()<br/>（イベント、ペイロード）
alt インストールイベント
Process->>DB: githubAppLinks更新
Process->>DB: integrations更新<br/>（suspension状態）
Process->>Cache: clearOrgCache()
else installation_repositoriesイベント
Process->>DB: appRepositorySelection更新
else サポートされていない
Process-->>Webhook: スキップ
end
Process-->>Webhook: 完了
Webhook-->>GitHub: 204 No Content
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
app/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.ts (1)
34-79:node_idなしの ID フォールバックも 1 ケース固定しておきたいです。今の assertion で通るのは
node_idありの repo だけなので、String(id)側が壊れても検知できません。owner が一致するnode_idなし fixture を 1 件足して、返り値のidまで固定しておくと回帰を拾いやすいです。As per coding guidelines, "Added or changed logic must have unit test coverage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.ts around lines 34 - 79, Add a test case to ensure filterInstallationRepos falls back to String(id) when node_id is missing: in the existing test 'filterInstallationRepos filters by owner and keyword and maps fields' add an extra fixture repo object that has an id (e.g., 1 or another unique number), no node_id, owner.login matching 'acme', and a name that matches the keyword; then assert that the returned mapped item's id is the stringified id (e.g., '1') for that repo. This targets the filterInstallationRepos logic and uses the same fetchAllInstallationRepos-typed fixtures so any regression in the node_id -> id fallback will be caught.app/routes/$orgSlug/settings/github-users._index/+functions/search-github-users.server.ts (1)
13-15: shared DB クエリは並列化した方がよいです。ここは入力補完のホットパスなので、
getIntegration()とgetGithubAppLink()を直列に待つと毎回 1 往復分遅くなります。依存関係はないのでPromise.allにまとめた方が軽いです。♻️ 例
- const integration = await getIntegration(organizationId) - const githubAppLink = await getGithubAppLink(organizationId) + const [integration, githubAppLink] = await Promise.all([ + getIntegration(organizationId), + getGithubAppLink(organizationId), + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/github-users._index/+functions/search-github-users.server.ts around lines 13 - 15, getIntegration(organizationId) と getGithubAppLink(organizationId) は依存がないため直列 await をやめ、Promise.all で並列実行して結果を分割代入し、その後 resolveOctokitFromOrg({ integration, githubAppLink }) を呼ぶように変更してください；具体的には getIntegration と getGithubAppLink を同時に実行して両方の結果を受け取る（例：const [integration, githubAppLink] = await Promise.all([getIntegration(organizationId), getGithubAppLink(organizationId)])）ようにして、resolveOctokitFromOrg に渡す流れに修正してください。scripts/poc-repo-add-api.ts (1)
8-19: 環境変数の検証を追加することを検討してください。PoC スクリプトですが、環境変数が未設定の場合にわかりやすいエラーメッセージを表示すると、デバッグが容易になります。
💡 環境変数検証の提案
+const keyPath = process.env.GITHUB_APP_PRIVATE_KEY_PATH +const appId = process.env.GITHUB_APP_ID +const installationId = process.env.GITHUB_APP_INSTALLATION_ID + +if (!keyPath || !appId || !installationId) { + console.error('Required env vars: GITHUB_APP_PRIVATE_KEY_PATH, GITHUB_APP_ID, GITHUB_APP_INSTALLATION_ID') + process.exit(1) +} + const privateKey = fs.readFileSync( - process.env.GITHUB_APP_PRIVATE_KEY_PATH!, + keyPath, 'utf-8', ) const octokit = new Octokit({ authStrategy: createAppAuth, auth: { - appId: Number(process.env.GITHUB_APP_ID), + appId: Number(appId), privateKey, - installationId: Number(process.env.GITHUB_APP_INSTALLATION_ID), + installationId: Number(installationId), }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/poc-repo-add-api.ts` around lines 8 - 19, Add explicit validation for required environment variables before reading the private key and creating the Octokit client: check process.env.GITHUB_APP_PRIVATE_KEY_PATH, process.env.GITHUB_APP_ID, and process.env.GITHUB_APP_INSTALLATION_ID and if any are missing or invalid throw or log a clear, actionable error (including the missing variable name) rather than proceeding; then read the private key into privateKey and instantiate Octokit with createAppAuth as shown (ensure Number conversion for appId and installationId only after validation).docs/github-app-step4-plan.md (1)
210-232: ドキュメントと実装の軽微な差異。ドキュメントでは
getInstallationRepositoriesの戻り値のidがnumberとなっていますが、実際の実装（get-installation-repos.ts）ではr.node_id ?? String(r.id)でstringを返しています。ドキュメントを更新して整合させることを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/github-app-step4-plan.md` around lines 210 - 232, The docs and implementation disagree on getInstallationRepositories' id type: the implementation uses r.node_id ?? String(r.id) and returns a string, while the docs declare id as number; update the documentation to mark id as string (or change the implementation to return a numeric id if you prefer numeric ids). Specifically, align the documented return type for getInstallationRepositories to string and mention that the id is derived from r.node_id ?? String(r.id) to match the actual behavior.app/services/github-webhook.server.test.ts (1)
210-225:installation_repositoriesイベントのエッジケーステスト追加を検討。マッチするリンクがない場合の
installation_repositoriesイベントのテストがありません。installation.created with no matching linkと同様のテストを追加することを検討してください。🧪 エッジケーステストの追加例
test('installation_repositories with no matching link does not throw', async () => { await expect( processGithubWebhookPayload('installation_repositories', { installation: { id: 999, repository_selection: 'selected', }, }), ).resolves.toBeUndefined() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/github-webhook.server.test.ts` around lines 210 - 225, Add an edge-case test for the installation_repositories webhook to ensure processGithubWebhookPayload('installation_repositories', ...) does not throw when there is no matching githubAppLinks row; create a test that calls processGithubWebhookPayload with an installation id that doesn't match any link (e.g., 999) and assert the promise resolves (e.g., expect(...).resolves.toBeUndefined()) so the handler gracefully no-ops instead of throwing.docs/github-app-phase2-plan.md (2)
17-31: Markdown のコードブロックに言語指定を追加することを検討。静的解析ツールが言語指定のないコードブロックを検出しています。ASCII アート/フロー図の場合は
textやplaintextを指定できます。📝 言語指定の追加例
-``` +```text ┌─ coji の操作 ──────────────────────────────────────────────────┐🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/github-app-phase2-plan.md` around lines 17 - 31, Update the Markdown code fence that contains the ASCII flow diagram beginning with "┌─ coji の操作 ──────────────────────────────────────────────────┐" to include a language tag such as text or plaintext (e.g. change ``` to ```text) so the static analyzer recognizes it as plain text; this change should be made in the block that holds the ASCII art/flow diagram so linters stop flagging languageless code fences.
146-153: テーブルの前後に空行を追加することを検討。静的解析ツール（MD058）がテーブル周囲の空行不足を検出しています。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/github-app-phase2-plan.md` around lines 146 - 153, The markdown table triggers MD058 (missing surrounding blank lines); fix it by adding a single blank line immediately before the table start (the line beginning with "| 状態") and a single blank line immediately after the table end (the final line ending with "| Token に戻すボタン |"), so the block is separated from surrounding paragraphs and satisfies the linter.db/migrate-integrations-to-shared.ts (1)
78-81: ファイル名パースのエッジケース。
tenant_と.dbの間に空文字列が入る可能性（例:tenant_.db）は低いですが、念のため検証を追加することを検討してください。ただし、これは一度きりのマイグレーションスクリプトなので、優先度は低いです。🔧 organizationId の検証追加
const organizationId = name.slice('tenant_'.length, -'.db'.length) + if (!organizationId) continue if (!orgExists.get(organizationId)) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate-integrations-to-shared.ts` around lines 78 - 81, ファイル名パースで空の organizationId（例: "tenant_.db"）を許容してしまう可能性があるため、ループ内の既存チェックに加えて生成した organizationId が空文字または空白のみでないことを検証してください（チェック対象: the for ... of loop over files, variable organizationId, and the orgExists.get(...) usage）；具体的には organizationId を切り出した直後に空文字/空白のみなら continue するガードを追加してから orgExists.get(organizationId) を呼ぶように修正してください。app/routes/api.github.setup.ts (1)
50-52: 空の catch ブロックでエラー情報が失われています。例外をキャッチしていますが、エラーの詳細がログに記録されていません。デバッグのためにエラー情報を記録することを検討してください。
🔧 エラーログ追加の提案
- } catch { + } catch (e) { + console.error('[api.github.setup] GitHub installation verification failed:', e) return new Response('Could not verify GitHub installation', { status: 502 }) }🤖 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 50 - 52, The empty catch block swallows errors when verifying the GitHub installation; update the catch to capture the thrown error (e.g., catch (err)) and log it before returning the 502 Response (use the project logger if available or console.error) so the handler that returns new Response('Could not verify GitHub installation', { status: 502 }) emits the error details for debugging; ensure you include context in the log message (e.g., "Could not verify GitHub installation:" + err).app/routes/$orgSlug/settings/_index/+forms/integration-settings.tsx (1)
51-68:useEffectの依存配列にcopyFetcherを含めると不要な再実行が発生する可能性があります。このリポジトリでは
useFetcherが返すオブジェクトは毎レンダーで新しい参照になるため、依存配列に含めるとエフェクトが毎回実行されます。copyFetcher.dataとcopyFetcher.stateのみを依存配列に含め、copyFetcher.resetは省略してください。♻️ 修正案
useEffect(() => { if (copyFetcher.state !== 'idle') return const d = copyFetcher.data if ( !d || typeof d !== 'object' || !('intent' in d) || d.intent !== 'copy-install-url' || !('installUrl' in d) || typeof d.installUrl !== 'string' ) { return } void navigator.clipboard.writeText(d.installUrl).then(() => { toast.success('Install URL copied to clipboard') copyFetcher.reset() }) - }, [copyFetcher.data, copyFetcher.reset, copyFetcher.state]) + // biome-ignore lint/correctness/useExhaustiveDependencies: copyFetcher.reset is unstable + }, [copyFetcher.data, copyFetcher.state])Based on learnings: "Treat
fetcherreturned by React Router'suseFetcheras an unstable reference. Do not include it in dependency arrays."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/`$orgSlug/settings/_index/+forms/integration-settings.tsx around lines 51 - 68, The effect using useEffect currently depends on the unstable copyFetcher reference which causes unnecessary re-runs; update the dependency array to only include the stable pieces used: replace [copyFetcher.data, copyFetcher.reset, copyFetcher.state] with [copyFetcher.data, copyFetcher.state], keeping the same logic inside (checks against copyFetcher.data, call to navigator.clipboard.writeText, toast.success and copyFetcher.reset invocation remain unchanged).
🤖 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/libs/webhook-verify.server.test.ts`:
- Around line 21-49: Add a unit test in webhook-verify.server.test.ts that
covers the signature-format rejection branch of verifyWebhookSignature: set
GITHUB_WEBHOOK_SECRET via vi.stubEnv, create a body string, and pass a signature
that includes the "sha256=" prefix but has an invalid digest (e.g., wrong length
or non-hex characters like "sha256=abc" or "sha256=zzzz..."), then assert
verifyWebhookSignature(body, invalidSig) returns false; ensure the test targets
the function verifyWebhookSignature so the 16-hex-digit/64-char hex-digest
validation branch is exercised.
In `@app/routes/`$orgSlug/settings/repositories/$repository/$pull/index.tsx:
- Around line 103-110: When repository.owner or repository.repo are null and
when resolveOctokitFromOrg({ integration, githubAppLink }) can fail, convert
those failures from generic exceptions into HTTP Responses with 4xx status so
the fetcher can handle them: replace the throw new Error('Repository is not
integrated') with a Response containing a clear client-error status (e.g. 400)
and message, and wrap the call to resolveOctokitFromOrg in a try/catch that
catches any error and throws a Response (e.g. 401 or 400) with a concise message
like "Authentication not configured" plus the original error message.
In `@app/routes/api.github.webhook.ts`:
- Around line 23-34: 現在のコードは未対応イベントでも processGithubWebhookPayload(event,
payload) を呼んでいるため、未対応イベントが processor 側で例外を投げると 500 を返して GitHub が再試行します。handled
フラグ（event === 'installation' || event ===
'installation_repositories'）をチェックして、handled が false の場合は
processGithubWebhookPayload を呼ばずに即座に new Response(null, { status: 202 })
を返すようにし、handled が true の場合のみ try/catch で processGithubWebhookPayload を実行してエラー時に
500 を返す挙動に修正してください。
In `@app/services/type.ts`:
- Around line 158-160: CI failed because generated DB types are out of sync; run
the DB generation and commit the results: run the same command used in CI (pnpm
db:generate), verify that regenerated type files (including app/services/type.ts
and app/services/tenant-type.ts) now include updated declarations such as
githubAppInstallStates, githubAppLinks, integrations, and commit those changes
so the pipeline no longer shows diffs.
In `@db/migrations/tenant/20260324000000_move_integrations_to_shared.sql`:
- Line 20: The DROP TABLE statement for the repositories table is missing IF
EXISTS; update the DROP TABLE `repositories` statement to include IF EXISTS so
the destructive migration is idempotent and consistent with other drops (i.e.,
use DROP TABLE IF EXISTS `repositories`), and rerun the migration against a
production-like DB to verify behavior.
- Around line 1-4: Move the PRAGMA foreign_keys = off statement so it runs
before DROP TABLE `integrations` to ensure foreign key constraints are disabled
before deleting the table (affects the DROP TABLE `integrations` and the
repositories FK removal). Update the migration so PRAGMA foreign_keys = off
appears prior to any DROP TABLE that affects referenced tables (e.g., the DROP
TABLE `integrations` call) and re-enable it afterward as appropriate;
additionally, change the second DROP TABLE at around line 20 to include IF
EXISTS (i.e., use DROP TABLE IF EXISTS ...) to avoid errors when the table is
already absent.
---
Nitpick comments:
In `@app/routes/`$orgSlug/settings/_index/+forms/integration-settings.tsx:
- Around line 51-68: The effect using useEffect currently depends on the
unstable copyFetcher reference which causes unnecessary re-runs; update the
dependency array to only include the stable pieces used: replace
[copyFetcher.data, copyFetcher.reset, copyFetcher.state] with [copyFetcher.data,
copyFetcher.state], keeping the same logic inside (checks against
copyFetcher.data, call to navigator.clipboard.writeText, toast.success and
copyFetcher.reset invocation remain unchanged).
In
`@app/routes/`$orgSlug/settings/github-users._index/+functions/search-github-users.server.ts:
- Around line 13-15: getIntegration(organizationId) と
getGithubAppLink(organizationId) は依存がないため直列 await をやめ、Promise.all
で並列実行して結果を分割代入し、その後 resolveOctokitFromOrg({ integration, githubAppLink })
を呼ぶように変更してください；具体的には getIntegration と getGithubAppLink
を同時に実行して両方の結果を受け取る（例：const [integration, githubAppLink] = await
Promise.all([getIntegration(organizationId),
getGithubAppLink(organizationId)])）ようにして、resolveOctokitFromOrg に渡す流れに修正してください。
In
`@app/routes/`$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.ts:
- Around line 34-79: Add a test case to ensure filterInstallationRepos falls
back to String(id) when node_id is missing: in the existing test
'filterInstallationRepos filters by owner and keyword and maps fields' add an
extra fixture repo object that has an id (e.g., 1 or another unique number), no
node_id, owner.login matching 'acme', and a name that matches the keyword; then
assert that the returned mapped item's id is the stringified id (e.g., '1') for
that repo. This targets the filterInstallationRepos logic and uses the same
fetchAllInstallationRepos-typed fixtures so any regression in the node_id -> id
fallback will be caught.
In `@app/routes/api.github.setup.ts`:
- Around line 50-52: The empty catch block swallows errors when verifying the
GitHub installation; update the catch to capture the thrown error (e.g., catch
(err)) and log it before returning the 502 Response (use the project logger if
available or console.error) so the handler that returns new Response('Could not
verify GitHub installation', { status: 502 }) emits the error details for
debugging; ensure you include context in the log message (e.g., "Could not
verify GitHub installation:" + err).
In `@app/services/github-webhook.server.test.ts`:
- Around line 210-225: Add an edge-case test for the installation_repositories
webhook to ensure processGithubWebhookPayload('installation_repositories', ...)
does not throw when there is no matching githubAppLinks row; create a test that
calls processGithubWebhookPayload with an installation id that doesn't match any
link (e.g., 999) and assert the promise resolves (e.g.,
expect(...).resolves.toBeUndefined()) so the handler gracefully no-ops instead
of throwing.
In `@db/migrate-integrations-to-shared.ts`:
- Around line 78-81: ファイル名パースで空の organizationId（例:
"tenant_.db"）を許容してしまう可能性があるため、ループ内の既存チェックに加えて生成した organizationId
が空文字または空白のみでないことを検証してください（チェック対象: the for ... of loop over files, variable
organizationId, and the orgExists.get(...) usage）；具体的には organizationId
を切り出した直後に空文字/空白のみなら continue するガードを追加してから orgExists.get(organizationId)
を呼ぶように修正してください。
In `@docs/github-app-phase2-plan.md`:
- Around line 17-31: Update the Markdown code fence that contains the ASCII flow
diagram beginning with "┌─ coji の操作
──────────────────────────────────────────────────┐" to include a language tag
such as text or plaintext (e.g. change ``` to ```text) so the static analyzer
recognizes it as plain text; this change should be made in the block that holds
the ASCII art/flow diagram so linters stop flagging languageless code fences.
- Around line 146-153: The markdown table triggers MD058 (missing surrounding
blank lines); fix it by adding a single blank line immediately before the table
start (the line beginning with "| 状態") and a single blank line immediately after
the table end (the final line ending with "| Token に戻すボタン |"), so the block is
separated from surrounding paragraphs and satisfies the linter.
In `@docs/github-app-step4-plan.md`:
- Around line 210-232: The docs and implementation disagree on
getInstallationRepositories' id type: the implementation uses r.node_id ??
String(r.id) and returns a string, while the docs declare id as number; update
the documentation to mark id as string (or change the implementation to return a
numeric id if you prefer numeric ids). Specifically, align the documented return
type for getInstallationRepositories to string and mention that the id is
derived from r.node_id ?? String(r.id) to match the actual behavior.
In `@scripts/poc-repo-add-api.ts`:
- Around line 8-19: Add explicit validation for required environment variables
before reading the private key and creating the Octokit client: check
process.env.GITHUB_APP_PRIVATE_KEY_PATH, process.env.GITHUB_APP_ID, and
process.env.GITHUB_APP_INSTALLATION_ID and if any are missing or invalid throw
or log a clear, actionable error (including the missing variable name) rather
than proceeding; then read the private key into privateKey and instantiate
Octokit with createAppAuth as shown (ensure Number conversion for appId and
installationId only after validation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 354fb084-1036-4a49-95fe-c188b1387d5b
⛔ Files ignored due to path filters (3)
db/migrations/shared/atlas.sumis excluded by!**/*.sumdb/migrations/tenant/atlas.sumis excluded by!**/*.sumpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
.env.exampleCLAUDE.mdapp/components/confirm-dialog.tsxapp/libs/github-app-state.server.test.tsapp/libs/github-app-state.server.tsapp/libs/webhook-verify.server.test.tsapp/libs/webhook-verify.server.tsapp/routes/$orgSlug/settings/_index/+forms/integration-settings.tsxapp/routes/$orgSlug/settings/_index/+functions/mutations.server.tsapp/routes/$orgSlug/settings/_index/+functions/queries.server.tsapp/routes/$orgSlug/settings/_index/+schema.tsapp/routes/$orgSlug/settings/github-users._index/+functions/search-github-users.server.tsapp/routes/$orgSlug/settings/integration/index.tsxapp/routes/$orgSlug/settings/repositories._index/mutations.server.test.tsapp/routes/$orgSlug/settings/repositories.add/+components/repository-item.tsxapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.tsapp/routes/$orgSlug/settings/repositories.add/+functions/mutations.server.tsapp/routes/$orgSlug/settings/repositories.add/+functions/queries.server.tsapp/routes/$orgSlug/settings/repositories.add/index.tsxapp/routes/$orgSlug/settings/repositories/$repository/$pull/index.tsxapp/routes/$orgSlug/settings/repositories/$repository/$pull/queries.server.tsapp/routes/$orgSlug/settings/repositories/$repository/settings/+functions/mutations.server.test.tsapp/routes/$orgSlug/settings/repositories/$repository/settings/+functions/queries.server.tsapp/routes/$orgSlug/settings/repositories/$repository/settings/index.tsxapp/routes/$orgSlug/workload/$login/+functions/queries.server.test.tsapp/routes/api.github.setup.test.tsapp/routes/api.github.setup.tsapp/routes/api.github.webhook.test.tsapp/routes/api.github.webhook.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/github-webhook.server.test.tsapp/services/github-webhook.server.tsapp/services/jobs/backfill.server.tsapp/services/jobs/crawl.server.tsapp/services/tenant-type.tsapp/services/type.tsbatch/db/queries.tsbatch/github/backfill-repo.tsbatch/github/fetcher.tsbatch/github/store.test.tsdb/migrate-integrations-to-shared.tsdb/migrations/shared/20260324000000_github_app.sqldb/migrations/shared/20260324042158.sqldb/migrations/tenant/20260324000000_move_integrations_to_shared.sqldb/seed.tsdb/shared.sqldb/tenant.sqldocs/github-app-impl-steps.mddocs/github-app-migration.mddocs/github-app-phase2-plan.mddocs/github-app-step3-plan.mddocs/github-app-step4-plan.mdkysely-codegen.shared.config.tskysely-codegen.tenant.config.tspackage.jsonscripts/poc-github-app.tsscripts/poc-repo-add-api.tsstart.sh
💤 Files with no reviewable changes (8)
- app/routes/$orgSlug/workload/$login/+functions/queries.server.test.ts
- app/routes/$orgSlug/settings/repositories/$repository/settings/+functions/queries.server.ts
- kysely-codegen.tenant.config.ts
- batch/github/store.test.ts
- app/routes/$orgSlug/settings/repositories/$repository/settings/+functions/mutations.server.test.ts
- app/routes/$orgSlug/settings/_index/+functions/queries.server.ts
- app/routes/$orgSlug/settings/repositories._index/mutations.server.test.ts
- app/services/tenant-type.ts
Comment on lines
+21
to
+49
| test('returns false without sha256= prefix', () => { | ||
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'my-secret') | ||
| const body = '{}' | ||
| const hex = createHmac('sha256', 'my-secret') | ||
| .update(body, 'utf8') | ||
| .digest('hex') | ||
| expect(verifyWebhookSignature(body, hex)).toBe(false) | ||
| }) | ||
|
|
||
| test('returns false when body is tampered', () => { | ||
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'my-secret') | ||
| const body = '{"ok":true}' | ||
| expect(verifyWebhookSignature(`${body}x`, sign(body, 'my-secret'))).toBe( | ||
| false, | ||
| ) | ||
| }) | ||
|
|
||
| test('returns false when secret differs', () => { | ||
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'secret-a') | ||
| const body = '{}' | ||
| expect(verifyWebhookSignature(body, sign(body, 'secret-b'))).toBe(false) | ||
| }) | ||
|
|
||
| test('returns false when GITHUB_WEBHOOK_SECRET is unset', () => { | ||
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', '') | ||
| const body = '{}' | ||
| expect(verifyWebhookSignature(body, sign(body, 'x'))).toBe(false) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
署名フォーマット検証の分岐テストが不足しています。
実装の sha256 ダイジェスト検証（16進形式・64文字長）の失敗分岐を直接検証するケースを追加したいです。
🧪 追加テスト案
describe('verifyWebhookSignature', () => {
@@
test('returns false without sha256= prefix', () => {
@@
expect(verifyWebhookSignature(body, hex)).toBe(false)
})
+
+ test('returns false when digest is not valid hex', () => {
+ vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'my-secret')
+ const body = '{}'
+ expect(verifyWebhookSignature(body, 'sha256=zzzz')).toBe(false)
+ })
+
+ test('returns false when digest length is not 64 chars', () => {
+ vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'my-secret')
+ const body = '{}'
+ expect(verifyWebhookSignature(body, 'sha256=abcd')).toBe(false)
+ })As per coding guidelines "Added or changed logic must have unit test coverage".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('returns false without sha256= prefix', () => { | |
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'my-secret') | |
| const body = '{}' | |
| const hex = createHmac('sha256', 'my-secret') | |
| .update(body, 'utf8') | |
| .digest('hex') | |
| expect(verifyWebhookSignature(body, hex)).toBe(false) | |
| }) | |
| test('returns false when body is tampered', () => { | |
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'my-secret') | |
| const body = '{"ok":true}' | |
| expect(verifyWebhookSignature(`${body}x`, sign(body, 'my-secret'))).toBe( | |
| false, | |
| ) | |
| }) | |
| test('returns false when secret differs', () => { | |
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'secret-a') | |
| const body = '{}' | |
| expect(verifyWebhookSignature(body, sign(body, 'secret-b'))).toBe(false) | |
| }) | |
| test('returns false when GITHUB_WEBHOOK_SECRET is unset', () => { | |
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', '') | |
| const body = '{}' | |
| expect(verifyWebhookSignature(body, sign(body, 'x'))).toBe(false) | |
| }) | |
| }) | |
| test('returns false without sha256= prefix', () => { | |
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'my-secret') | |
| const body = '{}' | |
| const hex = createHmac('sha256', 'my-secret') | |
| .update(body, 'utf8') | |
| .digest('hex') | |
| expect(verifyWebhookSignature(body, hex)).toBe(false) | |
| }) | |
| test('returns false when digest is not valid hex', () => { | |
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'my-secret') | |
| const body = '{}' | |
| expect(verifyWebhookSignature(body, 'sha256=zzzz')).toBe(false) | |
| }) | |
| test('returns false when digest length is not 64 chars', () => { | |
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'my-secret') | |
| const body = '{}' | |
| expect(verifyWebhookSignature(body, 'sha256=abcd')).toBe(false) | |
| }) | |
| test('returns false when body is tampered', () => { | |
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'my-secret') | |
| const body = '{"ok":true}' | |
| expect(verifyWebhookSignature(`${body}x`, sign(body, 'my-secret'))).toBe( | |
| false, | |
| ) | |
| }) | |
| test('returns false when secret differs', () => { | |
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', 'secret-a') | |
| const body = '{}' | |
| expect(verifyWebhookSignature(body, sign(body, 'secret-b'))).toBe(false) | |
| }) | |
| test('returns false when GITHUB_WEBHOOK_SECRET is unset', () => { | |
| vi.stubEnv('GITHUB_WEBHOOK_SECRET', '') | |
| const body = '{}' | |
| expect(verifyWebhookSignature(body, sign(body, 'x'))).toBe(false) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/libs/webhook-verify.server.test.ts` around lines 21 - 49, Add a unit test
in webhook-verify.server.test.ts that covers the signature-format rejection
branch of verifyWebhookSignature: set GITHUB_WEBHOOK_SECRET via vi.stubEnv,
create a body string, and pass a signature that includes the "sha256=" prefix
but has an invalid digest (e.g., wrong length or non-hex characters like
"sha256=abc" or "sha256=zzzz..."), then assert verifyWebhookSignature(body,
invalidSig) returns false; ensure the test targets the function
verifyWebhookSignature so the 16-hex-digit/64-char hex-digest validation branch
is exercised.
Owner
Author
There was a problem hiding this comment.
実装側で /^[0-9a-f]{64}$/i の正規表現チェックが入っているのでカバーはされていますが、テストケースとして明示する価値はあります。今回は対応を見送りますが、今後必要に応じて追加します。
There was a problem hiding this comment.
@coji、ご確認ありがとうございます。実装側の /^[0-9a-f]{64}$/i チェックで動作は保証されているため、今回の見送りは理解しました。必要なタイミングでテストケースを追加していただければと思います。
(^･ω･^ )～
🧠 Learnings used
Learnt from: CR
Repo: coji/upflow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T05:13:08.696Z
Learning: Applies to **/*.test.{ts,tsx} : Added or changed logic must have unit test coverage
Learnt from: coji
Repo: coji/upflow PR: 141
File: CLAUDE.md:215-215
Timestamp: 2026-03-09T08:25:06.885Z
Learning: In coji/upflow, for any UPDATE or DELETE on org-scoped tables, always include WHERE organizationId = ? with a server-derived value, and obtain per-org tenant data using getTenantDb(organizationId). There is no automatic Kysely plugin for org scoping; apply this pattern project-wide to ensure proper scoping.
Learnt from: coji
Repo: coji/upflow PR: 220
File: app/components/ui/dropdown-menu.tsx:74-76
Timestamp: 2026-03-20T10:52:59.295Z
Learning: When reviewing shadcn/ui + Tailwind v4 projects (including coji/upflow), do not flag the Tailwind v4 arbitrary variant syntax `*:[svg]:some-utility` (or similar `*:[<tag>]:...` / multi-variant chains like `data-[variant=destructive]:*:[svg]:text-destructive!`). In Tailwind v4, `*:` is the built-in “direct child” variant and `[svg]` is a valid bracket tag-name selector; these patterns are intentional and supported, so they are not invalid CSS and not incorrect Tailwind usage.
app/routes/$orgSlug/settings/repositories/$repository/$pull/index.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+158
to
+160
| githubAppInstallStates: GithubAppInstallStates; | ||
| githubAppLinks: GithubAppLinks; | ||
| integrations: Integrations; |
There was a problem hiding this comment.
DB型生成物の同期漏れでCIが落ちています。
🚀 Deploy の pipeline failure にある通り、pnpm db:generate 後の差分が未コミットです。app/services/type.ts だけでなく app/services/tenant-type.ts も含めて再生成結果をコミットしてください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/type.ts` around lines 158 - 160, CI failed because generated DB
types are out of sync; run the DB generation and commit the results: run the
same command used in CI (pnpm db:generate), verify that regenerated type files
(including app/services/type.ts and app/services/tenant-type.ts) now include
updated declarations such as githubAppInstallStates, githubAppLinks,
integrations, and commit those changes so the pipeline no longer shows diffs.
Owner
Author
There was a problem hiding this comment.
ローカルで pnpm db:generate を実行して確認しましたが、差分はありませんでした。CI 環境の seed DB の状態に依存している可能性があります。pnpm validate は全パスしています。
There was a problem hiding this comment.
@coji、ご確認ありがとうございます。pnpm db:generate でローカル差分なし、かつ pnpm validate が全パスしているとのこと、承知しました。
ファイル内容を改めて確認したところ、GithubAppInstallStates・GithubAppLinks・Integrations は app/services/type.ts にすでに正しくコミットされており、私の指摘は誤検知でした。ご不便をおかけして申し訳ありません。この指摘は無視していただいて問題ありません。
🧠 Learnings used
Learnt from: coji
Repo: coji/upflow PR: 102
File: app/services/type.ts:28-41
Timestamp: 2026-01-10T09:36:07.102Z
Learning: In app/services/type.ts, do not manually modify generated kysely-codegen types. The atlas_schema_revisions table is created and managed by Atlas during migrations, and its JSON-like columns are represented as string | null. Rely on the codegen output for types and migrations for schema changes; manual edits can cause drift between codegen and database schema.
Learnt from: CR
Repo: coji/upflow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T05:13:08.695Z
Learning: Applies to batch/**/*.ts : After adding new columns via migration, update local DB using: `pnpm ops pull-db -- --app <fly-app>` then `pnpm db:apply`; use `backfill` + `recalculate` for most new field additions instead of full `crawl --refresh`
Learnt from: CR
Repo: coji/upflow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T05:13:08.696Z
Learning: When modifying database schema, ensure `pnpm db:setup` succeeds and migrations safely apply against existing production data
Learnt from: CR
Repo: coji/upflow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T05:13:08.696Z
Learning: Run `pnpm validate` before submitting PR to ensure lint, format, typecheck, build, and test all pass
Learnt from: coji
Repo: coji/upflow PR: 141
File: CLAUDE.md:215-215
Timestamp: 2026-03-09T08:25:06.885Z
Learning: In coji/upflow, for any UPDATE or DELETE on org-scoped tables, always include WHERE organizationId = ? with a server-derived value, and obtain per-org tenant data using getTenantDb(organizationId). There is no automatic Kysely plugin for org scoping; apply this pattern project-wide to ensure proper scoping.
Learnt from: coji
Repo: coji/upflow PR: 198
File: app/services/jobs/crawl.server.ts:159-165
Timestamp: 2026-03-17T07:58:29.114Z
Learning: Guideline: Do not apply the global 'organizationId' filter to updates/deletes when using the per-organization tenant database accessed via getTenantDb(). In coji/upflow, getTenantDb(organizationId) uses a separate SQLite database file per organization, and tables there (e.g., organizationSettings, repositories) are implicitly scoped to that organization. The mandatory WHERE organizationId = ? rule only applies to the shared/global database accessed via the top-level db instance (app/services/db.server.ts). During reviews, ensure queries executed through getTenantDb() do not include or require organizationId in WHERE clauses for UPDATE/DELETE, unless referencing the global/db.server.ts path. This should be enforced by code patterns or CI checks that distinguish between tenant DB vs global DB usage.
Learnt from: coji
Repo: coji/upflow PR: 220
File: app/components/ui/dropdown-menu.tsx:74-76
Timestamp: 2026-03-20T10:52:59.295Z
Learning: When reviewing shadcn/ui + Tailwind v4 projects (including coji/upflow), do not flag the Tailwind v4 arbitrary variant syntax `*:[svg]:some-utility` (or similar `*:[<tag>]:...` / multi-variant chains like `data-[variant=destructive]:*:[svg]:text-destructive!`). In Tailwind v4, `*:` is the built-in “direct child” variant and `[svg]` is a valid bracket tag-name selector; these patterns are intentional and supported, so they are not invalid CSS and not incorrect Tailwind usage.
| CONSTRAINT `repositories_team_id_fkey` FOREIGN KEY (`team_id`) REFERENCES `teams` (`id`) ON UPDATE CASCADE ON DELETE SET NULL | ||
| ); | ||
| INSERT INTO `new_repositories` (`id`, `integration_id`, `provider`, `owner`, `repo`, `release_detection_method`, `release_detection_key`, `updated_at`, `created_at`, `team_id`) SELECT `id`, `integration_id`, `provider`, `owner`, `repo`, `release_detection_method`, `release_detection_key`, `updated_at`, `created_at`, `team_id` FROM `repositories`; | ||
| DROP TABLE `repositories`; |
There was a problem hiding this comment.
DROP TABLE repositories にも IF EXISTS を付けてください。
Line 20 だけ IF EXISTS が抜けています。破壊的 migration の再実行耐性をそろえるため、ここも合わせておきたいです。
🛠️ 修正案
-DROP TABLE `repositories`;
+DROP TABLE IF EXISTS `repositories`;As per coding guidelines, "Atlas-generated SQL must be reviewed; always add IF EXISTS to DROP TABLE statements; test destructive operations against production-equivalent DB state before deployment".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| DROP TABLE `repositories`; | |
| DROP TABLE IF EXISTS `repositories`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrations/tenant/20260324000000_move_integrations_to_shared.sql` at line
20, The DROP TABLE statement for the repositories table is missing IF EXISTS;
update the DROP TABLE `repositories` statement to include IF EXISTS so the
destructive migration is idempotent and consistent with other drops (i.e., use
DROP TABLE IF EXISTS `repositories`), and rerun the migration against a
production-like DB to verify behavior.
Owner
Author
There was a problem hiding this comment.
これは Atlas が自動生成したテーブル再作成の中間ステップ（CREATE new → INSERT SELECT → DROP old → RENAME）です。直前の INSERT SELECT が成功している前提で動くため、IF EXISTS は不要です。手動で変更すると atlas.sum のチェックサムが不整合になります。CLAUDE.md のルールもこの区別を明記するよう更新しました（ec46137）。
There was a problem hiding this comment.
@coji、ご説明ありがとうございます。おっしゃる通りです。CREATE new → INSERT SELECT → DROP old → RENAME というAtlasのテーブル再作成フローにおける中間ステップであれば、直前の INSERT SELECT の成功が前提のため IF EXISTS は不要ですし、手動変更すると atlas.sum のチェックサムも壊れますね。
長期記憶のルールが「常に IF EXISTS を付ける」と過度に広範だったため、この区別を正確に反映するよう更新します。
✏️ Learnings added
Learnt from: coji
Repo: coji/upflow PR: 245
File: db/migrations/tenant/20260324000000_move_integrations_to_shared.sql:20-20
Timestamp: 2026-03-24T08:26:01.909Z
Learning: In coji/upflow (db/**/*.sql), Atlas-generated SQL must be reviewed carefully with the following DROP TABLE rules:
1. Manually added `DROP TABLE` statements: always add `IF EXISTS` for idempotency.
2. Atlas-generated intermediate `DROP TABLE` statements that are part of a table-recreation flow (CREATE new_table → INSERT SELECT → DROP old_table → RENAME new_table → old_table): leave these unchanged WITHOUT `IF EXISTS`. These assume the preceding INSERT SELECT succeeded, and manually adding `IF EXISTS` would break the `atlas.sum` checksum validation. This distinction is documented in CLAUDE.md.
Always test destructive operations against a production-equivalent DB state before deployment.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
⛔ Learnings removed
Learnt from: CR
Repo: coji/upflow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T05:13:08.695Z
Learning: Applies to db/**/*.sql : Atlas-generated SQL must be reviewed; always add `IF EXISTS` to `DROP TABLE` statements; test destructive operations against production-equivalent DB state before deployment
🧠 Learnings used
Learnt from: CR
Repo: coji/upflow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T05:13:08.695Z
Learning: Applies to db/**/*.sql : Atlas-generated SQL must be reviewed; always add `IF EXISTS` to `DROP TABLE` statements; test destructive operations against production-equivalent DB state before deployment
Learnt from: coji
Repo: coji/upflow PR: 102
File: app/services/type.ts:28-41
Timestamp: 2026-01-10T09:36:14.875Z
Learning: The `atlas_schema_revisions` table in `db/schema.sql` is created and managed by Atlas during migrations. Its types in `app/services/type.ts` are auto-generated by kysely-codegen, and the generated types (including JSON columns typed as `string | null`) are expected behavior and should not be manually modified.
Learnt from: coji
Repo: coji/upflow PR: 198
File: app/services/jobs/crawl.server.ts:159-165
Timestamp: 2026-03-17T07:58:41.063Z
Learning: In coji/upflow, `getTenantDb(organizationId)` returns a per-org isolated SQLite database file (one DB file per organization). Tables within the tenant DB (e.g., `organizationSettings`, `repositories`) are already implicitly scoped to that single organization. Therefore, `WHERE organizationId = ?` is NOT required for UPDATE/DELETE operations on tenant DB tables. The mandatory `WHERE organizationId = ?` rule applies only to the shared/global DB accessed via the top-level `db` instance (app/services/db.server.ts), not to queries made through `getTenantDb()`.
Learnt from: coji
Repo: coji/upflow PR: 141
File: CLAUDE.md:215-215
Timestamp: 2026-03-09T08:25:06.885Z
Learning: In coji/upflow, for any UPDATE or DELETE on org-scoped tables, always include WHERE organizationId = ? with a server-derived value, and obtain per-org tenant data using getTenantDb(organizationId). There is no automatic Kysely plugin for org scoping; apply this pattern project-wide to ensure proper scoping.
- webhook route: 未対応イベントを processor に渡さず 202 で即返却 - $pull/index.tsx: 認証未設定時のエラーを throw new Response (422) に変更 - search-github-users: getIntegration + getGithubAppLink を Promise.all で並列化 - get-installation-repos.test: node_id なし時の String(id) フォールバックテスト追加 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Atlas 自動生成の中間ステップ DROP TABLE と手動追加の DROP TABLE を区別。 前者はそのままでよい（atlas.sum との整合性を保つため）。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/routes/api.github.webhook.ts (1)
30-31: 例外ログはgetErrorMessage()経由に統一してください。プロジェクト方針に合わせ、エラーメッセージ抽出をヘルパー経由に寄せると運用時の一貫性が上がります。
🔧 提案差分
+import { getErrorMessage } from '~/app/libs/error-message' import { verifyWebhookSignature } from '~/app/libs/webhook-verify.server' import { processGithubWebhookPayload } from '~/app/services/github-webhook.server' import type { Route } from './+types/api.github.webhook' @@ try { await processGithubWebhookPayload(event, payload) } catch (e) { - console.error('[github webhook]', e) + console.error('[github webhook]', getErrorMessage(e), e) return new Response('Webhook processing failed', { status: 500 }) }As per coding guidelines: Extract error messages using
getErrorMessage()function fromapp/libs/error-message.ts; do not useString(e)which returns[object Error]for Error instances.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/api.github.webhook.ts` around lines 30 - 31, Replace the raw error logging in the catch block that calls console.error('[github webhook]', e) with a call that extracts the message via getErrorMessage from app/libs/error-message.ts: import getErrorMessage and use console.error('[github webhook]', getErrorMessage(e)) (or pass the extracted string to whatever logger is used) so all exception messages are normalized through getErrorMessage; target the catch (e) block in app/routes/api.github.webhook.ts and update the console.error call accordingly.
🤖 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/routes/api.github.webhook.ts`:
- Around line 30-31: Replace the raw error logging in the catch block that calls
console.error('[github webhook]', e) with a call that extracts the message via
getErrorMessage from app/libs/error-message.ts: import getErrorMessage and use
console.error('[github webhook]', getErrorMessage(e)) (or pass the extracted
string to whatever logger is used) so all exception messages are normalized
through getErrorMessage; target the catch (e) block in
app/routes/api.github.webhook.ts and update the console.error call accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8c33e1e-f118-427d-ab13-182074a81d71
📒 Files selected for processing (6)
CLAUDE.mdapp/routes/$orgSlug/settings/github-users._index/+functions/search-github-users.server.tsapp/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.tsapp/routes/$orgSlug/settings/repositories/$repository/$pull/index.tsxapp/routes/api.github.webhook.test.tsapp/routes/api.github.webhook.ts
✅ Files skipped from review due to trivial changes (1)
- app/routes/api.github.webhook.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- app/routes/$orgSlug/settings/repositories.add/+functions/get-installation-repos.test.ts
- app/routes/$orgSlug/settings/github-users._index/+functions/search-github-users.server.ts
- app/routes/$orgSlug/settings/repositories/$repository/$pull/index.tsx
- CLAUDE.md
6つの GitHub App 関連 intent を raw string から INTENTS enum に統一: installGithubApp, copyInstallUrl, disconnectGithubApp, confirmDisconnectGithubApp, revertToToken, confirmRevertToToken action (integration/index.tsx) と UI (integration-settings.tsx) の 両方で同じ定数を参照。タイポによる runtime エラーを防止。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 24, 2026
Merged
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
PAT ベースの GitHub API 認証を GitHub App Installation Token に移行する基盤を構築。
主な変更
github_app_links+github_app_install_statesテーブル追加、integrationsを shared DB に移行createOctokit/resolveOctokitFromOrgfactory、fetcher のtoken→octokit変更デプロイ時の注意
マイグレーション順序が重要:
integrations+github_app_links+github_app_install_states作成）integrationsデータをコピー）integrationsテーブル削除 +repositoriesFK 削除）start.shで自動的にこの順序で実行される。本番予行演習済み。デプロイ後の手動作業
GITHUB_APP_PRIVATE_KEY,GITHUB_APP_ID）GITHUB_WEBHOOK_SECRET投入Test plan
pnpm validate全パス（40テスト、264ケース）pnpm db:setup成功🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
データベース / 移行
ドキュメント
テスト