feat(tasks): implement GitHub App wave 0#44
Conversation
|
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:
WalkthroughGitHub App連携を追加:Settings/.env注記、依存追加、SeaORMモデル、GitHub API/JWT・installation/token処理、OAuth stateのRedis管理、access token暗号化、install/callback/webhook/integrationハンドラ、Webhookジョブストレージとワーカー、ルーティング統合、単体・統合テスト。 ChangesGitHub App統合機能
Sequence Diagram(s)sequenceDiagram
participant Client
participant StartInstall as start_github_install
participant Callback as github_callback
participant GitHubAPI as GitHub API
participant Redis as Redis (OAuth state)
participant Postgres as Postgres (github_integrations)
participant Worker as Apalis Worker (enqueue/process)
Client->>StartInstall: GET /install (session)
StartInstall->>Redis: store state (github_oauth_state:{state})
StartInstall-->>Client: 200 { url: <install_url>?state=... }
Client->>Callback: GET /callback?state=...&installation_id=...
Callback->>Redis: consume state
Callback->>GitHubAPI: GET /app/installations/{id}
Callback->>GitHubAPI: POST /app/installations/{id}/access_tokens
GitHubAPI-->>Callback: installation info + access_token
Callback->>Postgres: upsert github_integrations (encrypted token)
Callback-->>Client: 302 Redirect
GitHubAPI->>Worker: POST /webhook -> /v1/github/webhook
Worker->>Worker: verify signature
Worker->>Postgres: enqueue GithubWebhookJob
Worker-->>GitHubAPI: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Deploying koyori with
|
| Latest commit: |
cf39ae3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e6d5173b.koyori.pages.dev |
| Branch Preview URL: | https://feat-tasks-github-app.koyori.pages.dev |
85f9f4d to
3983eb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/src/handlers/github.rs`:
- Around line 196-216: The current callback trusts query.installation_id when
payload.installation_id is None (initial linkage), allowing an attacker to bind
arbitrary installations; update the flow in the callback handler to verify that
the installation being bound was actually selected during the OAuth flow by
adding an ownership binding check in state before calling
github_api::fetch_installation_access_token / fetch_installation_account_login /
fetch_primary_repository: store/verifying the GitHub owner (or installation id)
in the saved state during the initial install request, then in the handler
compare that stored value against query.installation_id (and if
payload.installation_id is None require an additional owner match or a second
confirmation step), and only after that call github_api functions and call
require_tenant_owner/require_project_in_tenant; reference
payload.installation_id, query.installation_id, state, require_tenant_owner,
fetch_installation_access_token, fetch_installation_account_login, and
fetch_primary_repository when making the changes.
- Around line 405-409: The code currently assumes
github_api::delete_app_installation succeeds and returns AppError::Internal on
any failure, causing github_integrations::ActiveModel (from row.into()) to
remain when GitHub already deleted the installation; change the flow so you call
github_api::delete_app_installation(github, row.installation_id).await and match
its error: treat HTTP 404 and 410 responses (or whatever variant the client
returns for NotFound/Gone) as idempotent successes (log a warning including
row.installation_id and continue), and only map other failures to
AppError::Internal; then proceed to call active.delete(&state.db).await? to
remove the DB row. Ensure you reference github_api::delete_app_installation,
AppError::Internal, row.installation_id and github_integrations::ActiveModel
when making the change.
In `@apps/backend/src/server.rs`:
- Around line 91-94: board_api is registering github_webhook_storage which
exposes GitHub webhook payloads on the public Apalis board; remove the
.register(github_webhook_storage) from the ApiBuilder chain that creates
board_api and instead register github_webhook_storage only on a private/internal
ApiBuilder (e.g., create internal_api =
ApiBuilder::new(Router::new()).register(github_webhook_storage).build()) or
mount it behind your existing auth/tenant-guarding middleware so
github_webhook_storage is not exposed via board_api or the public /api/v1 UI.
In `@apps/backend/src/utils/github_oauth_state.rs`:
- Around line 25-29: The new_state_token function double-encodes the random
bytes: base64::engine::general_purpose::URL_SAFE_NO_PAD already produces a
URL-safe base64 string, so remove the redundant urlencoding::encode call and
return the base64 output directly (i.e., replace the
urlencoding::encode(&base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(bytes)).into_owned()
expression with base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(bytes)),
keeping the random byte generation and return type unchanged.
In `@apps/backend/src/utils/github_token_crypto.rs`:
- Around line 45-55: The derive_key function currently slices the first 32 bytes
of key_material into an AES key; replace this with a proper KDF (e.g., HKDF) to
derive a 32-byte key from key_material to ensure cryptographic strength and
allow key separation/rotation. In derive_key, feed key_material.as_bytes() into
an HKDF instance (provide an optional salt and an info/context string), expand
to 32 bytes and return that array, and map any HKDF errors into anyhow::Error;
keep the function signature derive_key(key_material: &str) -> Result<[u8; 32],
anyhow::Error> so callers are unchanged.
In `@apps/backend/tests/common/mod.rs`:
- Around line 255-258: The cleanup currently swallows delete errors (let _ =
users::Entity::delete_by_id(...).exec(...).await), which hides teardown
failures; change cleanup_user to surface errors by removing the ignored
assignment and either propagate the Result (change pub async fn
cleanup_user(&self, user_id: Uuid) -> Result<(), E> and use ? on
users::Entity::delete_by_id(...).exec(&self.state.db).await) or explicitly
assert/panic on Err so failures fail fast; update all callers/tests accordingly
to handle the Result if you choose propagation.
In `@apps/backend/tests/github_http_integration.rs`:
- Around line 108-127: The test currently bypasses the install flow by calling
github_oauth_state::store_state directly; instead invoke the /install flow (e.g.
call the start_github_install handler or test helper that hits the /install
endpoint), capture the returned Location header and extract the state parameter
from that URL, then use that extracted state in the subsequent /callback request
rather than github_oauth_state::new_state_token or direct store_state; this
ensures the test exercises start_github_install, preserves
tenant/project/user/installation_id wiring, and validates actual CSRF state
generation/consumption.
- Around line 43-47: The DELETE /app/installations/{id} mock in
apps/backend/tests/github_http_integration.rs currently has no call-count
expectation, so update the Mock defined with method("DELETE") and
path_regex(r"^/app/installations/\d+$") to require at least one invocation
(e.g., add an expectation like .expect(1) or equivalent on the Mock) before
mounting it to the test server; this ensures the handler actually calls the
GitHub uninstall endpoint and fails the test if it does not.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b842404f-9c4f-4411-95d2-b50585b625e7
⛔ Files ignored due to path filters (2)
apps/backend/Cargo.lockis excluded by!**/*.lockapps/backend/tests/fixtures/github_test_rsa.pemis excluded by!**/*.pem
📒 Files selected for processing (23)
apps/backend/.env.exampleapps/backend/Cargo.tomlapps/backend/src/entities/github_integrations.rsapps/backend/src/entities/mod.rsapps/backend/src/handlers/github.rsapps/backend/src/handlers/mod.rsapps/backend/src/jobs/github_webhook.rsapps/backend/src/jobs/mod.rsapps/backend/src/lib.rsapps/backend/src/main.rsapps/backend/src/openapi/mod.rsapps/backend/src/routes/github.rsapps/backend/src/routes/mod.rsapps/backend/src/routes/tenants.rsapps/backend/src/server.rsapps/backend/src/settings.rsapps/backend/src/utils/github_api.rsapps/backend/src/utils/github_oauth_state.rsapps/backend/src/utils/github_token_crypto.rsapps/backend/src/utils/mod.rsapps/backend/tests/common/mod.rsapps/backend/tests/github_http_integration.rsapps/backend/tests/github_integration.rs
| fn derive_key(key_material: &str) -> Result<[u8; 32], anyhow::Error> { | ||
| let bytes = key_material.as_bytes(); | ||
| if bytes.len() < 32 { | ||
| return Err(anyhow!( | ||
| "github_token_encryption_key must be at least 32 bytes" | ||
| )); | ||
| } | ||
| let mut key = [0u8; 32]; | ||
| key.copy_from_slice(&bytes[..32]); | ||
| Ok(key) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
鍵導出の強化を推奨
derive_key は key_material の先頭 32 バイトをそのまま AES 鍵として使用しています。現在は設定で 32 文字以上を要求しており、環境変数からのランダムな秘密値を想定しているため実用上は問題ありませんが、暗号学的ベストプラクティスとしては HKDF などの KDF(鍵導出関数)を使用することを推奨します。
Wave 0 では現状で許容できますが、将来的に鍵ローテーションや異なる用途での鍵分離が必要になった場合に備え、KDF への移行を検討してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/utils/github_token_crypto.rs` around lines 45 - 55, The
derive_key function currently slices the first 32 bytes of key_material into an
AES key; replace this with a proper KDF (e.g., HKDF) to derive a 32-byte key
from key_material to ensure cryptographic strength and allow key
separation/rotation. In derive_key, feed key_material.as_bytes() into an HKDF
instance (provide an optional salt and an info/context string), expand to 32
bytes and return that array, and map any HKDF errors into anyhow::Error; keep
the function signature derive_key(key_material: &str) -> Result<[u8; 32],
anyhow::Error> so callers are unchanged.
|
@gemini-code-assist レビューして |
There was a problem hiding this comment.
Code Review
This pull request implements Wave 0 of the GitHub App integration, adding configuration settings, database entities, API endpoints, background webhook processing via Apalis, and cryptographic utilities for secure token storage. Feedback on these changes suggests optimizing HTTP requests by reusing a single reqwest::Client instance to prevent socket exhaustion, and improving configuration loading efficiency by querying specific keys directly from the Config object instead of deserializing the entire Settings struct.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Verify installations via GitHub API on OAuth callback, reuse a shared HTTP client, harden delete/cleanup paths, and keep webhook jobs off the public Apalis board. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@gemini-code-assist レビューして |
There was a problem hiding this comment.
Code Review
This pull request implements GitHub App integration (Wave 0), adding endpoints for installation, callbacks, webhooks, and integration management, alongside a background worker for webhook processing. Several critical issues were identified in the review: the deletion handler destructively uninstalls the entire GitHub App instead of just disconnecting the local project; the unique constraint on installation_id prevents multiple projects from sharing an installation; the JWT generation lacks clock drift tolerance; the HTTP client is missing a timeout; errors during installation verification are discarded without logging; private key parsing does not handle surrounding double quotes; and refreshed tokens are not persisted to the database, which bypasses caching.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| github_api::delete_app_installation(github, row.installation_id) | ||
| .await | ||
| .map_err(AppError::Internal)?; | ||
| let active: github_integrations::ActiveModel = row.into(); | ||
| active.delete(&state.db).await?; |
There was a problem hiding this comment.
In delete_github_integration, calling github_api::delete_app_installation uninstalls the entire GitHub App from the user's GitHub account/organization. This is highly destructive and unexpected when a user simply wants to disconnect a single project's integration. If they have other projects linked to the same installation, those will also be broken. Disconnecting should only delete the local integration record from our database.
let active: github_integrations::ActiveModel = row.into();
active.delete(&state.db).await?;| #[sea_orm(unique)] | ||
| pub installation_id: i64, |
There was a problem hiding this comment.
The installation_id field is marked as #[sea_orm(unique)]. Since a GitHub App installation is per-organization/account, a tenant can only have one installation. If they want to link multiple projects to different repositories under the same GitHub organization, they will share the same installation_id. The unique constraint will prevent them from doing so, causing database unique constraint violations. The unique constraint should be removed from installation_id (or made unique on (installation_id, repo_owner, repo_name)), while keeping project_id unique.
| #[sea_orm(unique)] | |
| pub installation_id: i64, | |
| pub installation_id: i64, |
| let now = Utc::now(); | ||
| let claims = AppJwtClaims { | ||
| iss: settings.github_app_id.clone(), | ||
| iat: now.timestamp(), | ||
| exp: (now + Duration::minutes(9)).timestamp(), | ||
| }; |
There was a problem hiding this comment.
The iat (issued at) claim is set to the exact current time (now.timestamp()). If the server's clock is slightly ahead of GitHub's clock, GitHub will reject the JWT with an "issued in the future" error. Setting iat to 60 seconds in the past is a standard best practice to prevent clock drift issues.
| let now = Utc::now(); | |
| let claims = AppJwtClaims { | |
| iss: settings.github_app_id.clone(), | |
| iat: now.timestamp(), | |
| exp: (now + Duration::minutes(9)).timestamp(), | |
| }; | |
| let now = Utc::now(); | |
| let claims = AppJwtClaims { | |
| iss: settings.github_app_id.clone(), | |
| iat: (now - Duration::seconds(60)).timestamp(), | |
| exp: (now + Duration::minutes(8)).timestamp(), | |
| }; |
| fn http_client() -> &'static Client { | ||
| HTTP_CLIENT.get_or_init(|| { | ||
| Client::builder() | ||
| .build() | ||
| .expect("build shared reqwest client") | ||
| }) | ||
| } |
There was a problem hiding this comment.
The shared reqwest::Client is built without any timeout. If GitHub's API hangs or becomes extremely slow, requests will block indefinitely, potentially exhausting server resources. It is a critical production best practice to always configure a reasonable timeout on HTTP clients.
| fn http_client() -> &'static Client { | |
| HTTP_CLIENT.get_or_init(|| { | |
| Client::builder() | |
| .build() | |
| .expect("build shared reqwest client") | |
| }) | |
| } | |
| fn http_client() -> &'static Client { | |
| HTTP_CLIENT.get_or_init(|| { | |
| Client::builder() | |
| .timeout(std::time::Duration::from_secs(10)) | |
| .build() | |
| .expect("build shared reqwest client") | |
| }) | |
| } |
| let installation = github_api::verify_installation_for_callback( | ||
| github, | ||
| query.installation_id, | ||
| payload.installation_id, | ||
| ) | ||
| .await | ||
| .map_err(|_| AppError::BadRequest)?; |
There was a problem hiding this comment.
In github_callback, any error from verify_installation_for_callback is mapped to AppError::BadRequest using map_err(|_| AppError::BadRequest). This completely discards the underlying error without logging it. If verification fails due to a network issue, clock drift, or configuration error, developers will have no visibility into the root cause in production logs.
let installation = github_api::verify_installation_for_callback(
github,
query.installation_id,
payload.installation_id,
)
.await
.map_err(|e| {
tracing::error!(error = %e, "failed to verify installation for callback");
AppError::BadRequest
})?;| .try_deserialize() | ||
| .map_err(|e| anyhow::anyhow!("failed to deserialize github app settings: {e}"))?; | ||
|
|
||
| gh.github_app_private_key = gh.github_app_private_key.replace("\\n", "\n"); |
There was a problem hiding this comment.
In .env.example, the private key is shown wrapped in double quotes. If a user copies this format to their .env file, the parsed string may contain literal leading and trailing double quotes. EncodingKey::from_rsa_pem will fail to parse the key if it contains these quotes. Trimming surrounding double quotes ensures robustness.
gh.github_app_private_key = gh.github_app_private_key
.trim_matches('"')
.replace("\\n", "\n");| pub async fn refresh_token_if_needed( | ||
| settings: &GithubAppSettings, | ||
| installation_id: i64, | ||
| token_expires_at: chrono::DateTime<chrono::FixedOffset>, | ||
| access_token_enc: &str, | ||
| ) -> Result<(String, chrono::DateTime<chrono::FixedOffset>), anyhow::Error> { | ||
| let refresh_threshold = | ||
| chrono::Utc::now().fixed_offset() + chrono::Duration::minutes(5); | ||
| if token_expires_at > refresh_threshold { | ||
| return Ok(( | ||
| crate::utils::github_token_crypto::decrypt_token( | ||
| &settings.github_token_encryption_key, | ||
| access_token_enc, | ||
| )?, | ||
| token_expires_at, | ||
| )); | ||
| } | ||
| let fresh = fetch_installation_access_token(settings, installation_id).await?; | ||
| Ok((fresh.token, fresh.expires_at)) | ||
| } |
There was a problem hiding this comment.
refresh_token_if_needed fetches a new token if the current one is expired or expiring soon, but it does not persist the new token back to the database. Consequently, every subsequent API call will see the old expired token in the database and trigger a new token request to GitHub, bypassing the cache and potentially hitting GitHub's rate limits. The caller or this function should persist the new token to the database.
Verify installations via GitHub API on OAuth callback, reuse a shared HTTP client, harden delete/cleanup paths, and keep webhook jobs off the public Apalis board. Co-authored-by: Cursor <cursoragent@cursor.com>
9bd35a8 to
9316fbb
Compare
Verify installations via GitHub API on OAuth callback, reuse a shared HTTP client, harden delete/cleanup paths, and keep webhook jobs off the public Apalis board.
79ee33d to
489cfce
Compare
Wave 0 data model and AES-256-GCM encryption plus GitHub App API helpers.
Implements spec endpoints with Redis CSRF state and Apalis webhook queue.
Adds required env vars and registers the github_webhook Apalis worker.
Prefer repositories owned by the installation account login instead of arbitrary list order. Add installation account lookup and GitHub-side uninstall helper. Mark refresh_token_if_needed for Wave 1. Assisted-by: multi-agent-shogun-aki-tweak
GitHub App settings load only when GITHUB_APP_ID is set so deployments without GitHub integration can still start. Assisted-by: multi-agent-shogun-aki-tweak
Store optional installation_id in CSRF state for re-link flows. Assisted-by: multi-agent-shogun-aki-tweak
Validate installation_id against OAuth state, require github_app settings, and call GitHub App uninstall API on disconnect. Assisted-by: multi-agent-shogun-aki-tweak
Prevent one GitHub App installation from being linked to multiple projects. Assisted-by: multi-agent-shogun-aki-tweak
…selection Assisted-by: multi-agent-shogun-aki-tweak
…ation tests installation_id mismatch returns 400; DELETE without integration returns 404. Add wiremock-backed Axum HTTP tests for install/callback/integration flows. GITHUB_API_BASE_URL allows mocking GitHub API in tests. Assisted-by: multi-agent-shogun-aki-tweak
Verify installations via GitHub API on OAuth callback, reuse a shared HTTP client, harden delete/cleanup paths, and keep webhook jobs off the public Apalis board.
Assisted-by: multi-agent-shogun-aki-tweak
Assisted-by: multi-agent-shogun-aki-tweak
Assisted-by: multi-agent-shogun-aki-tweak
…::warn Assisted-by: multi-agent-shogun-aki-tweak
Merge cleanup_user and test entity imports after rebase onto main.
489cfce to
a18d8bc
Compare
- delete_github_integration: DB削除→GitHub API削除の順に修正(逆順だとDB残存リスク)
- github_callback: setup_action=request を早期リターンで弾く(承認待ち = 連携未完了)
- github_callback: 再連携時に created_by/created_at を上書きしない
- github_webhook: 未知の installation_id をwarnログで明示
- start_github_install: 202+Locationヘッダーを廃止し、200+JSON {url} に変更
- github_api.rs: 独自staticクライアントを廃止、AppState.http_clientを使用
- github_api_base(): 毎回env読みをOnceLockでキャッシュ
- worker_concurrency: Settings.github_webhook_worker_concurrency を参照
- テスト: install_path期待値をJSON対応に更新、require_2faフィールド追加
Assisted-by: multi-agent-shogun-aki-tweak
- installation_id の UNIQUE 制約を外す(同一 installation を複数プロジェクトで使用可能に) - select_primary_repository をリポジトリ 1 件のみ自動選択に制限(複数時は None を返し明示選択を要求) - default_github_app_frontend_base_url を空文字列に変更し、未設定時に email_verification_app_url へ確実にフォールバックするよう修正 Assisted-by: multi-agent-shogun-aki-tweak
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/backend/tests/common/mod.rs (1)
787-791:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
cleanup_userでoauth_connections削除エラーを握りつぶしています。ここで失敗を無視すると teardown 不整合が潜伏し、後続テストの順序依存や汚染を見逃します。失敗は即時に表面化してください。
差分案
- let _ = oauth_connections::Entity::delete_many() + oauth_connections::Entity::delete_many() .filter(oauth_connections::Column::UserId.eq(user_id)) .exec(&self.state.db) - .await; + .await + .expect("cleanup oauth connections");As per coding guidelines,
apps/backend/**/*.rsではエラーハンドリングと非同期処理の安全性を優先して確認してください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/tests/common/mod.rs` around lines 787 - 791, The cleanup_user function is swallowing errors from oauth_connections::Entity::delete_many().exec(&self.state.db).await; change cleanup_user to return a Result (e.g., async fn cleanup_user(&self, user_id: Uuid) -> Result<(), DbErr>) and propagate the deletion error instead of ignoring it by using the ? operator on the exec(...) call so failures surface immediately; update any call sites in tests to await and handle the Result accordingly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/src/handlers/github.rs`:
- Around line 427-442: Current flow deletes the DB record (ActiveModel via
active.delete) before calling github_api::delete_app_installation, which can
lose installation_id and swallow real errors; change to call
github_api::delete_app_installation(&state.http_client, github, installation_id)
first, treat only HTTP 404/410 as idempotent success and continue, but propagate
any other errors instead of logging-and-ignoring, and only after a
successful/allowed GitHub outcome perform active.delete(&state.db).await; keep
use of installation_id and preserve error context (return or map the error)
rather than always treating non-204 as success.
In `@apps/backend/src/settings.rs`:
- Around line 93-97: The current require_github_app() method returns
AppError::BadRequest when github_app is None, which semantically mislabels a
server-side disabled feature as a client error; update require_github_app() to
return a more appropriate error variant (e.g., AppError::NotFound or a new
AppError::FeatureDisabled) instead of AppError::BadRequest so callers receive a
correct server-side/feature-disabled signal; modify the match/ok_or call in
require_github_app to use the chosen variant and add the new variant to the
AppError enum if necessary, keeping the return type Result<&GithubAppSettings,
crate::error::AppError>.
In `@apps/backend/src/utils/github_api.rs`:
- Around line 161-166: The check comparing info.id to installation_id inside the
code that calls fetch_installation(client, settings, installation_id) is
redundant in normal operation; keep the defensive check but add a short comment
above it explaining intent (e.g., that fetch_installation should return the
requested installation but we verify to detect any unexpected GitHub
API/contract violations), referencing fetch_installation, info.id and
installation_id so future readers understand why the guard remains.
In `@apps/backend/tests/github_http_integration.rs`:
- Around line 103-110: The test flakes because github_api_base() uses static
BASE: OnceLock<String> and caches GITHUB_API_BASE_URL on first access; fix by
either making the test run serially or by removing/adjusting the global cache:
Option A - add the serial_test attribute to github_http_integration_suite (use
#[serial] from serial_test) so it runs non-parallel and the env var set takes
effect; Option B - change github_api_base (and the static BASE usage) to avoid
permanent caching during tests (e.g., read env each call or provide a test-only
reset() to clear BASE or allow an override hook) so setting GITHUB_API_BASE_URL
in github_http_integration_suite is respected. Ensure references to
github_api_base and static BASE: OnceLock<String> are updated consistently.
---
Duplicate comments:
In `@apps/backend/tests/common/mod.rs`:
- Around line 787-791: The cleanup_user function is swallowing errors from
oauth_connections::Entity::delete_many().exec(&self.state.db).await; change
cleanup_user to return a Result (e.g., async fn cleanup_user(&self, user_id:
Uuid) -> Result<(), DbErr>) and propagate the deletion error instead of ignoring
it by using the ? operator on the exec(...) call so failures surface
immediately; update any call sites in tests to await and handle the Result
accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c268d8ac-2ff5-46af-a180-81ffb338af3c
⛔ Files ignored due to path filters (2)
apps/backend/Cargo.lockis excluded by!**/*.lockapps/backend/tests/fixtures/github_test_rsa.pemis excluded by!**/*.pem
📒 Files selected for processing (23)
apps/backend/.env.exampleapps/backend/Cargo.tomlapps/backend/src/entities/github_integrations.rsapps/backend/src/entities/mod.rsapps/backend/src/handlers/github.rsapps/backend/src/handlers/mod.rsapps/backend/src/jobs/github_webhook.rsapps/backend/src/jobs/mod.rsapps/backend/src/lib.rsapps/backend/src/main.rsapps/backend/src/openapi/mod.rsapps/backend/src/routes/github.rsapps/backend/src/routes/mod.rsapps/backend/src/routes/tenants.rsapps/backend/src/server.rsapps/backend/src/settings.rsapps/backend/src/utils/github_api.rsapps/backend/src/utils/github_oauth_state.rsapps/backend/src/utils/github_token_crypto.rsapps/backend/src/utils/mod.rsapps/backend/tests/common/mod.rsapps/backend/tests/github_http_integration.rsapps/backend/tests/github_integration.rs
| if info.id != installation_id { | ||
| return Err(anyhow!( | ||
| "installation id mismatch: api={} query={installation_id}", | ||
| info.id | ||
| )); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
冗長なチェックの可能性があります。
fetch_installation(client, settings, installation_id) は指定した installation_id のインストール情報を取得するため、戻り値の info.id が installation_id と異なることは通常ありません。このチェックは防御的プログラミングとしては有効ですが、GitHub API の契約に反する状況を検出するものであり、発生確率は極めて低いです。
現状維持でも問題ありませんが、コメントで意図を明示すると可読性が向上します。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/utils/github_api.rs` around lines 161 - 166, The check
comparing info.id to installation_id inside the code that calls
fetch_installation(client, settings, installation_id) is redundant in normal
operation; keep the defensive check but add a short comment above it explaining
intent (e.g., that fetch_installation should return the requested installation
but we verify to detect any unexpected GitHub API/contract violations),
referencing fetch_installation, info.id and installation_id so future readers
understand why the guard remains.
- ensure_schema()のスキーマ同期順序修正(schema sync → ALTER TABLE の順に変更) - insert_personal_token_for_test()から平文tokenカラムのINSERTを削除 - mainのCIワークフロー追加・Renovate設定更新・oauth.rsバグ修正を取り込み Assisted-by: multi-agent-shogun-aki-tweak
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/backend/tests/common/mod.rs (2)
788-792:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
oauth_connections削除のエラーハンドリングが他と一貫していません。同じ関数内で
github_integrations、tenants、usersは.expect()でエラー時にパニックしますが、oauth_connectionsだけlet _ =でエラーを無視しています。削除失敗を握りつぶすと、後続の FK 制約違反で混乱を招く可能性があります。🔧 修正案
pub async fn cleanup_user(&self, user_id: Uuid) { - let _ = oauth_connections::Entity::delete_many() + oauth_connections::Entity::delete_many() .filter(oauth_connections::Column::UserId.eq(user_id)) .exec(&self.state.db) - .await; + .await + .expect("cleanup oauth connections");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/tests/common/mod.rs` around lines 788 - 792, In cleanup_user ensure oauth_connections deletion uses the same error handling as the other deletes: replace the silent `let _ = oauth_connections::Entity::delete_many().filter(oauth_connections::Column::UserId.eq(user_id)).exec(&self.state.db).await` with a call that unwraps or propagates the error (e.g. `.await.expect("failed to delete oauth_connections for user")` or `?`) so failures are not ignored; update the call site in the cleanup_user function where oauth_connections::Entity::delete_many is invoked.Source: Coding guidelines
78-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winハードコードされた絶対パスは移植性を損ないます。
Line 82 の
/home/coder/task/apps/backend/.envは特定の開発環境に依存しており、CI/CD や他の開発者のマシンでは機能しません。CARGO_MANIFEST_DIRからの相対パス (Line 81) で十分なはずです。🔧 修正案
fn load_dotenv() { let manifest = Path::new(env!("CARGO_MANIFEST_DIR")); let _ = dotenvy::dotenv().ok(); let _ = dotenvy::from_path(manifest.join(".env")).ok(); - let _ = dotenvy::from_path("/home/coder/task/apps/backend/.env").ok(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/tests/common/mod.rs` around lines 78 - 83, The function load_dotenv contains a hardcoded absolute path `/home/coder/task/apps/backend/.env` which breaks portability; remove that hardcoded from load_dotenv and rely on the manifest-based path using env!("CARGO_MANIFEST_DIR") (e.g., manifest.join(".env")) and/or a runtime fallback to the current working directory (std::env::current_dir()) if needed; update the calls in load_dotenv so only dotenvy::dotenv(), dotenvy::from_path(manifest.join(".env")), and an optional dotenvy::from_path(current_dir.join(".env")) are attempted, and drop the hardcoded literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/backend/tests/common/mod.rs`:
- Around line 788-792: In cleanup_user ensure oauth_connections deletion uses
the same error handling as the other deletes: replace the silent `let _ =
oauth_connections::Entity::delete_many().filter(oauth_connections::Column::UserId.eq(user_id)).exec(&self.state.db).await`
with a call that unwraps or propagates the error (e.g. `.await.expect("failed to
delete oauth_connections for user")` or `?`) so failures are not ignored; update
the call site in the cleanup_user function where
oauth_connections::Entity::delete_many is invoked.
- Around line 78-83: The function load_dotenv contains a hardcoded absolute path
`/home/coder/task/apps/backend/.env` which breaks portability; remove that
hardcoded from load_dotenv and rely on the manifest-based path using
env!("CARGO_MANIFEST_DIR") (e.g., manifest.join(".env")) and/or a runtime
fallback to the current working directory (std::env::current_dir()) if needed;
update the calls in load_dotenv so only dotenvy::dotenv(),
dotenvy::from_path(manifest.join(".env")), and an optional
dotenvy::from_path(current_dir.join(".env")) are attempted, and drop the
hardcoded literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ba91ac0e-a8bc-4ad7-af53-85130529eb0c
⛔ Files ignored due to path filters (1)
apps/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
apps/backend/tests/common/mod.rs
- github.rs: delete_github_integration の削除順序を修正 GitHub API → DB の順に変更(404/410 は冪等成功) 旧: DB 先削除で GitHub 失敗時に installation_id を喪失するリスク - github_api.rs: JWT iat を 60 秒前に設定しクロックドリフト対策 - settings.rs: require_github_app() のエラー型を BadRequest → NotFound (機能無効はサーバ側の問題であり 400 は意味的に不正確) - github_http_integration.rs: serial_test::serial を追加 OnceLock + set_var の並列実行競合を防止 - Cargo.toml: serial_test v3 を dev-dependencies に追加 Assisted-by: multi-agent-shogun-aki-tweak
複数リポジトリがある場合にpreferred_ownerに一致する1件のみを 自動選択するロジックを実装。 従来は常にNoneを返しておりtest_primary_repository_selection_prefers_account_ownerが失敗していた。 選定ルール: - 1件 → そのまま返す - preferred_ownerと一致が1件のみ → それを返す - 複数一致・ゼロ件 → None(明示的選択が必要) Assisted-by: multi-agent-shogun-aki-tweak
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/backend/src/settings.rs (2)
21-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
github_app_frontend_base_urlも base URL として検証してください。この値は callback 後のリダイレクト先組み立てにそのまま使われますが、現状は未設定時の fallback しかなく、明示設定された不正 URL は起動時に落ちません。設定ミスを runtime まで持ち込まないように、
email_verification_app_urlと同じ URL バリデータを付けるべきです。差分案
+ #[validate(custom( + function = "validate_email_verification_app_url", + message = "github_app_frontend_base_url must be a valid http or https base URL" + ))] #[serde(default = "default_github_app_frontend_base_url")] pub github_app_frontend_base_url: String,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/settings.rs` around lines 21 - 22, The github_app_frontend_base_url field isn’t validated as a base URL so an explicitly-bad value can crash redirect logic at runtime; update the Settings struct to apply the same URL/base-URL validation used for email_verification_app_url (i.e. add the same validator attribute and use the same validation function used by email_verification_app_url) and ensure the default function default_github_app_frontend_base_url remains the fallback; locate github_app_frontend_base_url and default_github_app_frontend_base_url in the settings.rs and mirror the validation annotations and helper used for email_verification_app_url to reject invalid URLs at startup.
147-163:⚠️ Potential issue | 🟠 MajorGitHub App 秘密鍵のPEMはロード時に fail-fast してください(apps/backend/src/settings.rs 147-163)
GithubAppSettingsはgithub_app_private_keyを長さ検証しかしておらず、EncodingKey::from_rsa_pem()はcreate_app_jwt()内で初めて実行されます(現在は主に/callbackと/integration(連携解除) で失敗し、400/500 に化け得ます)。\\n正規化後に同じPEMパーサで検証して起動時に落とすべきです。github_app_frontend_base_urlもvalidate()で http(s) base URL といった形式検証が無いため、settings_redirect_urlのリダイレクト破損を防ぐバリデーション追加が必要です。差分案
gh.github_app_private_key = gh.github_app_private_key.replace("\\n", "\n"); + jsonwebtoken::EncodingKey::from_rsa_pem(gh.github_app_private_key.as_bytes()) + .map_err(|e| anyhow::anyhow!("invalid github app private key PEM: {e}"))?; if gh.github_app_frontend_base_url.is_empty() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/settings.rs` around lines 147 - 163, After deserializing into GithubAppSettings, perform fail-fast validation on the PEM by parsing gh.github_app_private_key (after the .replace("\\n","\n")) with the same PEM parser used at runtime (EncodingKey::from_rsa_pem / the code path used by create_app_jwt) and return an error if parsing fails; additionally extend gh.validate() (or add an explicit check after deserialization) to ensure gh.github_app_frontend_base_url is a proper http(s) base URL (scheme http or https, host present, no disallowed path/query so it can be used as settings_redirect_url) so invalid redirect bases are rejected at startup.
♻️ Duplicate comments (1)
apps/backend/src/handlers/github.rs (1)
152-161:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift初回連携の state が GitHub 側の主体に結び付いていません。
Line 156-161 では初回フローで
installation_idがNoneのまま保存され、Line 212-222 ではその場合に「TTL 内に作られた installation か」だけでquery.installation_idを受け入れています。これだと正当なstateを持つテナントオーナーが、最近作られた別 installation ID を callback に差し込むだけで、Line 224-232 の token 発行とgithub_integrationsへの保存まで進められます。初回連携でも state に GitHub 側の束縛情報を持たせてfetch_installation()の結果と照合するか、別の確認ステップを挟いでください。As per coding guidelines, "apps/backend/**/*.rs: Rust / Axum / SeaORM のバックエンドです。認証・認可、テナント境界、SQL の正しさ、エラーハンドリング、非同期処理の安全性を優先して確認してください。"Also applies to: 212-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/handlers/github.rs` around lines 152 - 161, The state saved by github_oauth_state::store_state (via GithubOAuthStatePayload) is allowing installation_id = None which later accepts any recent installation via query.installation_id; modify the flow so the saved state includes a GitHub-side binding (e.g., expected installation id or installation account id/owner identifier returned by the initial GitHub handshake) when creating the state_token, or add an explicit verification step in the callback: call fetch_installation() and compare the fetched installation's canonical identifier to the value stored in the state before proceeding to token issuance and inserting into github_integrations; update the logic around github_oauth_state::store_state, GithubOAuthStatePayload, fetch_installation(), and the code paths that rely on query.installation_id to enforce this match.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/backend/src/settings.rs`:
- Around line 21-22: The github_app_frontend_base_url field isn’t validated as a
base URL so an explicitly-bad value can crash redirect logic at runtime; update
the Settings struct to apply the same URL/base-URL validation used for
email_verification_app_url (i.e. add the same validator attribute and use the
same validation function used by email_verification_app_url) and ensure the
default function default_github_app_frontend_base_url remains the fallback;
locate github_app_frontend_base_url and default_github_app_frontend_base_url in
the settings.rs and mirror the validation annotations and helper used for
email_verification_app_url to reject invalid URLs at startup.
- Around line 147-163: After deserializing into GithubAppSettings, perform
fail-fast validation on the PEM by parsing gh.github_app_private_key (after the
.replace("\\n","\n")) with the same PEM parser used at runtime
(EncodingKey::from_rsa_pem / the code path used by create_app_jwt) and return an
error if parsing fails; additionally extend gh.validate() (or add an explicit
check after deserialization) to ensure gh.github_app_frontend_base_url is a
proper http(s) base URL (scheme http or https, host present, no disallowed
path/query so it can be used as settings_redirect_url) so invalid redirect bases
are rejected at startup.
---
Duplicate comments:
In `@apps/backend/src/handlers/github.rs`:
- Around line 152-161: The state saved by github_oauth_state::store_state (via
GithubOAuthStatePayload) is allowing installation_id = None which later accepts
any recent installation via query.installation_id; modify the flow so the saved
state includes a GitHub-side binding (e.g., expected installation id or
installation account id/owner identifier returned by the initial GitHub
handshake) when creating the state_token, or add an explicit verification step
in the callback: call fetch_installation() and compare the fetched
installation's canonical identifier to the value stored in the state before
proceeding to token issuance and inserting into github_integrations; update the
logic around github_oauth_state::store_state, GithubOAuthStatePayload,
fetch_installation(), and the code paths that rely on query.installation_id to
enforce this match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99107015-8a44-4abd-ade4-0e69a7d8a101
⛔ Files ignored due to path filters (1)
apps/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
apps/backend/Cargo.tomlapps/backend/src/handlers/github.rsapps/backend/src/settings.rsapps/backend/src/utils/github_api.rsapps/backend/tests/github_http_integration.rs
.one() は installation_id が非 unique なため複数行ヒット時に失敗する。 .all() に変更し、マッチした全 integration に対してジョブをエンキューする。 Assisted-by: multi-agent-shogun-aki-tweak
payload に repository フィールドがある場合は installation_id + repo_owner + repo_name で絞り込む。installation 全体イベント (installation, installation_repositories 等) のように repository が含まれない場合は installation 配下の全件を対象とする。 Assisted-by: multi-agent-shogun-aki-tweak
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/src/handlers/github.rs`:
- Around line 334-347:
ループ途中での部分的失敗で既にキューに入ったジョブが重複するリスクがあるため、github_webhook::enqueue
呼び出しを冪等化してください:enqueue 側(または呼び出し前)で delivery_id + integration_id
をキーに重複チェックを行い、既存ジョブがある場合は再登録をスキップしてログを出すように変更します。対象箇所は integrations を回すループと構造体
GithubWebhookJob(fields: integration_id, project_id, event, delivery_id,
payload)で、将来的には integrations を collect して一括で処理するオプションも用意してください。
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8c8516e1-43c2-4ffe-b440-bccb22fd8030
📒 Files selected for processing (1)
apps/backend/src/handlers/github.rs
| for integration in integrations { | ||
| github_webhook::enqueue( | ||
| &state.github_webhook_storage, | ||
| GithubWebhookJob { | ||
| integration_id: integration.id, | ||
| project_id: integration.project_id, | ||
| event: event.clone(), | ||
| delivery_id: delivery_id.clone(), | ||
| payload: payload.clone(), | ||
| }, | ||
| ) | ||
| .await | ||
| .map_err(AppError::Internal)?; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
部分的な enqueue 失敗時の重複処理リスク
ループ途中で enqueue が失敗すると、すでにキューに入ったジョブはそのまま残り、GitHub のリトライ時に再度全 integration がエンキューされます。これにより、成功済みの integration に対して重複ジョブが発生します。
Wave 0 はログ出力のみなので実害はありませんが、PR #9b でタスクリンクを実装する際は delivery_id + integration_id での冪等性チェックが必要です。
♻️ 将来の対応案: collect してから一括処理
+ let jobs: Vec<GithubWebhookJob> = integrations
+ .into_iter()
+ .map(|integration| GithubWebhookJob {
+ integration_id: integration.id,
+ project_id: integration.project_id,
+ event: event.clone(),
+ delivery_id: delivery_id.clone(),
+ payload: payload.clone(),
+ })
+ .collect();
+
+ // TODO: Wave 1+ で delivery_id ベースの重複排除を追加
+ for job in jobs {
github_webhook::enqueue(
&state.github_webhook_storage,
- GithubWebhookJob {
- integration_id: integration.id,
- project_id: integration.project_id,
- event: event.clone(),
- delivery_id: delivery_id.clone(),
- payload: payload.clone(),
- },
+ job,
)
.await
.map_err(AppError::Internal)?;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/handlers/github.rs` around lines 334 - 347,
ループ途中での部分的失敗で既にキューに入ったジョブが重複するリスクがあるため、github_webhook::enqueue
呼び出しを冪等化してください:enqueue 側(または呼び出し前)で delivery_id + integration_id
をキーに重複チェックを行い、既存ジョブがある場合は再登録をスキップしてログを出すように変更します。対象箇所は integrations を回すループと構造体
GithubWebhookJob(fields: integration_id, project_id, event, delivery_id,
payload)で、将来的には integrations を collect して一括で処理するオプションも用意してください。
ループ内で直接 enqueue するパターンから、先に Vec<GithubWebhookJob> を collect してから enqueue するパターンに変更。 構造自体では重複排除を解決しないが、TODO コメントで Wave 1+ (#9b) での delivery_id + integration_id ベース冪等性チェックが必要であることを明示する。 Assisted-by: multi-agent-shogun-aki-tweak
Summary
github_integrationsSeaORM entity (schema-sync) per spec PR #9astate(10 min TTL, consume-on-use)github_webhookqueue); Wave 0 worker logs only (task linking in PR #9b)API endpoints
/v1/tenants/{tid}/projects/{pid}/github/install/v1/github/callback/v1/github/webhook/v1/tenants/{tid}/projects/{pid}/github/integration/v1/tenants/{tid}/projects/{pid}/github/integrationConfig
New env vars documented in
apps/backend/.env.example:github_app_id,github_app_private_key,github_app_webhook_secret,github_app_name,github_token_encryption_key, optionalgithub_app_frontend_base_url.Test plan
cargo buildinapps/backendcargo test github_token_cryptoPOST /v1/github/webhookMade with Cursor
Summary by CodeRabbit