Skip to content

feat(github): add safe connection management foundation - #5903

Merged
pandemicsyn merged 21 commits into
mainfrom
feat/github-connections-pr1
Sep 8, 2026
Merged

feat(github): add safe connection management foundation#5903
pandemicsyn merged 21 commits into
mainfrom
feat/github-connections-pr1

Conversation

@pandemicsyn

@pandemicsyn pandemicsyn commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

What this PR does

  • Separates canonical GitHub App installation data from Kilo-owned integration records while preserving existing integration IDs and single-owner exclusivity.
  • Adds a purpose-bound OAuth/PKCE flow for discovering and reconnecting existing Standard or Lite installations, with fresh GitHub organization-owner and Kilo owner/admin verification.
  • Adds local disconnect semantics without uninstalling the GitHub App, enforced across token minting, webhooks, bots, callbacks, Security workers, and Gas Town fallback paths.
  • Keeps connection management default-off behind GITHUB_CONNECTION_MANAGEMENT_ENABLED; this PR does not enable sharing one installation across multiple Kilo entities.

Migration and compatibility

Migration 0239_old_hemingway creates canonical installation and connection-attempt storage, then automatically links every healthy, active, unambiguous GitHub integration using idempotent set-based SQL. The observed aggregate was 37,154 GitHub rows in a 121 MB source table: 36,951 eligible, 0 ambiguous active rows, 2 migration-0205 losers, and 201 inactive or invalid rows intentionally excluded.

The remaining script is for exception reporting and reconciliation only; normal deployment does not require a manual backfill. Existing install, pending-approval, suspend/unsuspend, upstream-uninstall, session, Slack, and Code Reviewer behavior remains compatible while the flag is off.

Why PostgreSQL advisory locks are required

The authorization rules cover multiple rows, including connections that do not exist yet, so row locks cannot protect the decision. Transaction-scoped advisory locks serialize two boundaries:

  1. App type + installation ID, preventing concurrent owners from claiming the same GitHub installation.
  2. Non-allowlisted Kilo organization, preventing concurrent different installation IDs from both observing that the organization has no installation.

This also makes PR1 writers safe when PR2 later removes the old global uniqueness index to support intentional sharing. Locks use one consistent order (installation, then organization), release automatically on commit/rollback, and do not serialize users or organizations already allowed to use multiple installations.

Safety properties

  • Existing connection IDs, configuration, ownership, and historical references are retained.
  • Disconnected associations retain the installation reservation and cannot mint or publish through cached or delayed paths.
  • Standard and Lite identities remain distinct, including signed bot-link state and legacy Standard-null handling.
  • Canonical personal-account data is removed during user deletion when no association remains.
  • Ambiguous, unhealthy, malformed, deduplicated, and non-App rows fail closed.

Verification

  • Executable rollback-transaction migration test covers eligible, excluded, ambiguous, pre-existing, and idempotent cases.
  • Database concurrency tests exercise cross-owner and same-organization races using real advisory locks.
  • Fresh migration bootstrap passed.
  • Web and DB typecheck/lint, changed-package typecheck, formatting, and affected Worker checks passed locally; required CI reruns for every pushed head.

Rollout and follow-ups

Keep GITHUB_CONNECTION_MANAGEMENT_ENABLED off through PR2. Before enabling the shared-installation pilot, verify live Standard/Lite authorization, confirm reconciliation and writer cutover, and complete the deferred canonical foreign key and shared Cloud Agent authorization.

Planned stack:

  1. This PR: connection management foundation and automatic canonical migration.
  2. Shared installations for web Cloud Agents.
  3. Shared installations through Slack.
  4. Safe Code Reviewer routing without duplicate automation.

@pandemicsyn
pandemicsyn marked this pull request as ready for review September 8, 2026 03:53
@kilo-code-bot

kilo-code-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Fix these issues in Kilo Cloud

Executive Summary

findIntegrationByInstallationId now matches legacy-null GitHub app rows and returns an unordered first hit, so webhooks and bot-link can attach to the wrong owner.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/db/platform-integrations.ts 41 Standard-or-null lookup can return another owner's row
Files Reviewed (88 files)
  • ENVIRONMENT.md
  • apps/web/src/app/api/integrations/github/callback/route.test.ts
  • apps/web/src/app/api/integrations/github/callback/route.ts
  • apps/web/src/app/api/integrations/github/connection/callback/route.test.ts
  • apps/web/src/app/api/integrations/github/connection/callback/route.ts
  • apps/web/src/app/api/internal/bot-session-callback/[botRequestId]/route.test.ts
  • apps/web/src/app/api/internal/bot-session-callback/[botRequestId]/route.ts
  • apps/web/src/app/api/internal/code-review-status/[reviewId]/route.test.ts
  • apps/web/src/app/api/webhooks/github/route.test.ts
  • apps/web/src/app/api/webhooks/github/route.ts
  • apps/web/src/app/github/link/route.test.ts
  • apps/web/src/app/github/link/route.ts
  • apps/web/src/components/integrations/GitHubConnectionAttemptState.test.tsx
  • apps/web/src/components/integrations/GitHubConnectionAttemptState.tsx
  • apps/web/src/components/integrations/OrganizationGitHubInstallations.tsx
  • apps/web/src/lib/admin/github-installation-uninstall.test.ts
  • apps/web/src/lib/admin/github-installation-uninstall.ts
  • apps/web/src/lib/bot-identity.test.ts
  • apps/web/src/lib/bot-identity.ts
  • apps/web/src/lib/bot/github-link-state.test.ts
  • apps/web/src/lib/bot/github-link-state.ts
  • apps/web/src/lib/bot/platform-helpers.ts
  • apps/web/src/lib/bot/platforms/github.test.ts
  • apps/web/src/lib/bot/platforms/github.ts
  • apps/web/src/lib/bot/platforms/types.ts
  • apps/web/src/lib/bot/run.ts
  • apps/web/src/lib/cloud-agent/github-integration-helpers.test.ts
  • apps/web/src/lib/cloud-agent/github-integration-helpers.ts
  • apps/web/src/lib/integrations/core/health.test.ts
  • apps/web/src/lib/integrations/core/health.ts
  • apps/web/src/lib/integrations/db/github-installations-backfill.ts
  • apps/web/src/lib/integrations/db/github-installations.test.ts
  • apps/web/src/lib/integrations/db/github-installations.ts
  • apps/web/src/lib/integrations/db/platform-integrations.test.ts
  • apps/web/src/lib/integrations/db/platform-integrations.ts - 1 issue
  • apps/web/src/lib/integrations/github/connection-service.test.ts
  • apps/web/src/lib/integrations/github/connection-service.ts
  • apps/web/src/lib/integrations/github/connection-state.test.ts
  • apps/web/src/lib/integrations/github/connection-state.ts
  • apps/web/src/lib/integrations/github/installation-authorization.test.ts
  • apps/web/src/lib/integrations/github/installation-authorization.ts
  • apps/web/src/lib/integrations/github/multiple-installations.ts
  • apps/web/src/lib/integrations/github/runtime-authorization.test.ts
  • apps/web/src/lib/integrations/github/runtime-authorization.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.find-kilo-review-comment.test.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.runtime-authorization.test.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.ts
  • apps/web/src/lib/integrations/platforms/github/fetch-pull-request-review-decision.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler-dispatch.integration.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-handler.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-repositories-handler.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-target-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-target-handler.ts
  • apps/web/src/lib/redis-keys.ts
  • apps/web/src/lib/slack-bot/github-repository-context.test.ts
  • apps/web/src/lib/slack-bot/github-repository-context.ts
  • apps/web/src/lib/user/index.test.ts
  • apps/web/src/lib/user/index.ts
  • apps/web/src/routers/github-apps-router.test.ts
  • apps/web/src/routers/github-apps-router.ts
  • apps/web/src/scripts/backfill-github-installations.ts
  • packages/db/src/migrations/0239_old_hemingway.sql
  • packages/db/src/schema.test.ts
  • packages/db/src/schema.ts
  • services/gastown/src/dos/town/town-scm.ts
  • services/gastown/test/unit/town-scm.test.ts
  • services/gastown/worker-configuration.d.ts
  • services/git-token-service/src/index.test.ts
  • services/git-token-service/src/index.ts
  • services/git-token-service/src/installation-lookup-service.behavior.test.ts
  • services/git-token-service/src/installation-lookup-service.test.ts
  • services/git-token-service/src/installation-lookup-service.ts
  • services/security-auto-analysis/src/auto-dismiss.ts
  • services/security-auto-analysis/src/consumer.test.ts
  • services/security-auto-analysis/src/consumer.ts
  • services/security-auto-analysis/src/manual-analysis.ts
  • services/security-auto-analysis/src/remediation-settled-outcomes.test.ts
  • services/security-auto-analysis/src/remediation.test.ts
  • services/security-auto-analysis/src/remediation.ts
  • services/security-auto-analysis/worker-configuration.d.ts
  • services/security-sync/src/dismiss.test.ts
  • services/security-sync/src/dismiss.ts
  • services/security-sync/src/sync.test.ts
  • services/security-sync/src/sync.ts
  • services/security-sync/worker-configuration.d.ts
Previous Review Summaries (2 snapshots, latest commit 7190a8f)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 7190a8f)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (10 files)
  • apps/web/src/app/api/integrations/github/callback/route.ts
  • apps/web/src/app/api/integrations/github/callback/route.test.ts
  • apps/web/src/lib/integrations/db/github-installations.ts
  • apps/web/src/lib/integrations/db/github-installations.test.ts
  • apps/web/src/lib/integrations/db/github-installations-backfill.ts
  • apps/web/src/lib/integrations/db/platform-integrations.ts
  • apps/web/src/lib/user/index.ts
  • apps/web/src/lib/user/index.test.ts
  • packages/db/src/migrations/0238_worried_leo.sql
  • packages/db/src/schema.test.ts

Previous review (commit 260a7e0)

Status: 5 Issues Found | Recommendation: Address before merge

Fix these issues in Kilo Cloud

Overview

Severity Count
CRITICAL 0
WARNING 4
SUGGESTION 1
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/app/api/integrations/github/callback/route.ts 693 Flag-off canonical accountType treats Organization installs as User because 'login' in account is true for orgs
apps/web/src/app/api/integrations/github/callback/route.ts 698 Canonical bind re-looks-up by installation id without owner; standard-or-null limit(1) can bind another owner's row
apps/web/src/lib/user/index.ts 1369 Account deletion only removes canonicals already linked via github_installation_id, leaving webhook-created personal PII
apps/web/src/lib/integrations/db/github-installations.ts 154 Connect takes an unordered first standard/null match, so a legacy-null peer can steal exclusivity from the caller-owned row

SUGGESTION

File Line Issue
packages/db/src/schema.ts 4244 platform_integrations.github_installation_id is used as a join key with no partial index
Files Reviewed (2 files)
  • packages/db/src/migrations/0238_worried_leo.sql
  • packages/db/src/schema.test.ts

Reviewed by grok-4.6 · Input: 355K · Output: 34.2K · Cached: 1.2M

Review guidance: REVIEW.md from base branch main

@pandemicsyn
pandemicsyn merged commit 302cfa9 into main Sep 8, 2026
93 of 94 checks passed
@pandemicsyn
pandemicsyn deleted the feat/github-connections-pr1 branch September 8, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants