chore: github installations rfc#1017
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 32 minutes and 55 seconds. ⌛ 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new RFC document specifies the design for First-Class GitHub Installations, including database schema linking GitHub App installations to ctrlplane workspaces, OAuth setup flow with JWT-protected state binding, and integration approach replacing free-text JSON fields with foreign key references. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🧹 Nitpick comments (1)
docs/rfc/0012-github-installations.mdx (1)
190-192: Pick one storage model forgithubInstallationIdin v1.The RFC currently leaves the job-agent reference location as “TBD” (column vs JSON). This will fragment API/UI/engine implementation decisions. Recommend locking one path in this RFC and deferring the alternative explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/rfc/0012-github-installations.mdx` around lines 190 - 192, Decide and lock the storage model for githubInstallationId by choosing one option and removing the “TBD”: update the RFC to state that job_agent will include a dedicated column github_installation_id uuid NULL REFERENCES github_installation(id) (not stored inside config), remove the alternative suggestion, and add a short note instructing API/UI/engine implementers to read/write githubInstallationId from the job_agent.github_installation_id column (update any references to config.githubInstallationId accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/rfc/0012-github-installations.mdx`:
- Line 106: The RFC defines the column created_by_user_id as NOT NULL but the
migration plan uses manual inserts without guaranteeing a value and the RFC text
leaves nullability undecided; pick and document a single resolution: either
change the schema to allow created_by_user_id NULL (alter the CREATE
TABLE/column definition where created_by_user_id uuid is declared) and update
the migration steps to accept NULL for legacy rows, or keep NOT NULL and add a
deterministic attribution strategy in the migration plan and RFC text (e.g., set
a specific system user id or map an existing user) and ensure every manual
INSERT/seed referenced in the migration plan supplies created_by_user_id; update
all mentions in the RFC to reflect the decided behavior.
---
Nitpick comments:
In `@docs/rfc/0012-github-installations.mdx`:
- Around line 190-192: Decide and lock the storage model for
githubInstallationId by choosing one option and removing the “TBD”: update the
RFC to state that job_agent will include a dedicated column
github_installation_id uuid NULL REFERENCES github_installation(id) (not stored
inside config), remove the alternative suggestion, and add a short note
instructing API/UI/engine implementers to read/write githubInstallationId from
the job_agent.github_installation_id column (update any references to
config.githubInstallationId 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: CHILL
Plan: Pro
Run ID: d61812fd-7687-4069-94a5-fe130774a13d
📒 Files selected for processing (1)
docs/rfc/0012-github-installations.mdx
| owner text NOT NULL, -- GitHub org or user login | ||
| account_type text NOT NULL, -- 'Organization' | 'User' | ||
|
|
||
| created_by_user_id uuid NOT NULL REFERENCES "user"(id), |
There was a problem hiding this comment.
Resolve created_by_user_id nullability before implementation.
created_by_user_id is NOT NULL (Line 106), but the migration plan uses manual inserts (Line 282) and the RFC still leaves nullability undecided (Line 305). This is a schema/migration conflict that should be decided in the RFC now to avoid blocking rollout or forcing fake attribution.
Also applies to: 282-285, 305-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/rfc/0012-github-installations.mdx` at line 106, The RFC defines the
column created_by_user_id as NOT NULL but the migration plan uses manual inserts
without guaranteeing a value and the RFC text leaves nullability undecided; pick
and document a single resolution: either change the schema to allow
created_by_user_id NULL (alter the CREATE TABLE/column definition where
created_by_user_id uuid is declared) and update the migration steps to accept
NULL for legacy rows, or keep NOT NULL and add a deterministic attribution
strategy in the migration plan and RFC text (e.g., set a specific system user id
or map an existing user) and ensure every manual INSERT/seed referenced in the
migration plan supplies created_by_user_id; update all mentions in the RFC to
reflect the decided behavior.
There was a problem hiding this comment.
Pull request overview
Adds RFC 0012 describing a secure, workspace-scoped model for GitHub App installations to prevent cross-workspace installation impersonation in shared ctrlplane deployments.
Changes:
- Proposes a new
github_installationtable scoped toworkspace_idwith(workspace_id, installation_id)uniqueness. - Defines a verified linking flow using GitHub App install redirect + ctrlplane state token + user OAuth check via
GET /user/installations. - Outlines migration steps to move GitHub job agent configs from raw
installationIdJSON to agithubInstallationIdFK reference.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Category | Status | Created | Author | | ||
| | ----------- | --------------------------------- | ---------- | ---------------- | | ||
| | Integration | <Badge color="gray">Draft</Badge> | 2026-04-20 | Aditya Choudhari | |
There was a problem hiding this comment.
The RFC metadata table uses Integration as the Category, but other RFCs consistently use Integrations (plural). To keep the docs taxonomy consistent (and make searching/grouping RFCs easier), update this to match the existing category naming.
| | Category | Status | Created | Author | | |
| | ----------- | --------------------------------- | ---------- | ---------------- | | |
| | Integration | <Badge color="gray">Draft</Badge> | 2026-04-20 | Aditya Choudhari | | |
| | Category | Status | Created | Author | | |
| | ------------ | --------------------------------- | ---------- | ---------------- | | |
| | Integrations | <Badge color="gray">Draft</Badge> | 2026-04-20 | Aditya Choudhari | |
|
|
||
| ```sql | ||
| CREATE TABLE github_installation ( | ||
| id uuid PRIMARY KEY DEFAULT uuid_generate_v4(), |
There was a problem hiding this comment.
The proposed SQL uses DEFAULT uuid_generate_v4() for the PK. In this repo's Postgres schema/migrations, UUID defaults are typically gen_random_uuid() / Drizzle .defaultRandom() (pgcrypto), not uuid-ossp. Consider updating the RFC SQL to use gen_random_uuid() to match the existing DB conventions and avoid requiring the uuid-ossp extension.
| id uuid PRIMARY KEY DEFAULT uuid_generate_v4(), | |
| id uuid PRIMARY KEY DEFAULT gen_random_uuid(), |
| 3. If the user does not have a fresh GitHub OAuth token on file, redirect | ||
| through the GitHub OAuth authorize endpoint to obtain one (scope: minimal, | ||
| enough to call `GET /user/installations`). |
There was a problem hiding this comment.
There’s an internal inconsistency about GitHub OAuth token storage: the Non-Goals section says the OAuth token is used only for a single verification request, but this step refers to a “fresh GitHub OAuth token on file,” which implies persistence. Please clarify whether the token is persisted (and for how long/where) vs stored only ephemerally in the linking session.
| 3. If the user does not have a fresh GitHub OAuth token on file, redirect | |
| through the GitHub OAuth authorize endpoint to obtain one (scope: minimal, | |
| enough to call `GET /user/installations`). | |
| 3. If the current linking session does not already have a freshly obtained | |
| GitHub OAuth token for this verification flow, redirect through the GitHub | |
| OAuth authorize endpoint to obtain one (scope: minimal, enough to call | |
| `GET /user/installations`). This token is held only ephemerally for the | |
| linking session, used for the verification request below, and is not stored | |
| as a durable credential "on file". |
| 3. **Attribution on manual inserts.** For the handful of rows we hand-insert | ||
| during migration, `created_by_user_id` should be nullable, or we pick the | ||
| workspace creator as a sensible default. Worth deciding before the column | ||
| lands. |
There was a problem hiding this comment.
This suggests created_by_user_id may need to be nullable for manual inserts, but the Schema section defines it as NOT NULL. Please reconcile the RFC by either making it nullable with clear semantics, or specifying a required fallback (e.g., a system user/service principal) for backfilled rows.
| 3. **Attribution on manual inserts.** For the handful of rows we hand-insert | |
| during migration, `created_by_user_id` should be nullable, or we pick the | |
| workspace creator as a sensible default. Worth deciding before the column | |
| lands. | |
| 3. **Attribution on manual inserts.** Keep `created_by_user_id` as `NOT NULL`. | |
| For the handful of rows we hand-insert during migration, populate it with a | |
| dedicated system user/service principal used for backfills rather than | |
| making the column nullable. This preserves consistent attribution semantics | |
| for both OAuth-created and manually backfilled rows. |
Summary by CodeRabbit