Repository navigation
fix(github): fix connect/disconnect lifecycle bugs found in live org testing - #6100
Merged
Merged
Conversation
The F3 change awaited fetchGitHubRepositoriesForMaintenance unconditionally before the connection transaction, so a transient listing failure (rate limit, 5xx, timeout) aborted completeGitHubConnectionAttempt and the connection was never created — previously an all-repositories attach never depended on that call at all. Restore best-effort semantics for the all-repositories case: the fetch still runs (so the cache is populated when it succeeds, which is F3's intent), but a failure is captured and the attach proceeds with repositories left null for a later refresh. The selected-repositories case keeps surfacing the failure, because that listing is the user's own selection. This matches how the pending-install completion path already treats the same call. Test: 'a failing inventory listing does not block an all-repositories attach' — the attach completes with an active association and null repositories (aborts before this fix); the existing caching test still asserts the inventory is populated on success.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
This was referenced Sep 15, 2026
…tion A pending-install completion could still attach a second tenant to an installation the legacy (connection-management-disabled) callback was already claiming. That path commits the association unbound and binds it in a separate step, and the webhook's locked existence check only counted CANONICALLY BOUND rows — so a peer that was committed but not yet bound was invisible, and a second organization could be auto-completed and bound onto the same canonical installation with sharing_mode left exclusive. - Broaden the locked check in handleInstallationCreated to count every other live association for this installation (pending or active, bound or not, excluding the pending row and locally disconnected rows), so an unbound legacy peer blocks the completion instead of being skipped. - Serialize the legacy leg: the install callback's create+bind now runs under the same installation advisory lock with the same competing-tenant check, and refuses with 'installation_already_claimed' rather than double-binding. - Share one app-type predicate (effectiveAppTypeCondition, now exported) instead of a third copy. Tests: installation-handler.db 'does not auto-attach a second tenant while legacy associations are committed but unbound' (the pending row became active before this fix) and the callback route suite updated for the locked bind.
…rbitrary row Canonical lifecycle handlers resolved their tenant with findIntegrationByInstallationId, which returns an arbitrary association for the installation, and unsuspend then discarded the result when it happened to be locally disconnected. After one tenant disconnects and another stays connected, the lookup can return the retained disconnected row, so the event was dropped and the connected tenant's suspension never cleared — the association and its canonical installation stayed suspended until a manual reconnect. Suspend had the same arbitrary-row selection. Resolve the tenant through a new findLifecycleIntegration: keep using the existing lookup first (so behaviour is unchanged when it already returns a connected row), and when it returns a locally disconnected former tenant fall back to an explicit connected-association query for the installation and app type. suspend and unsuspend both use it; canonical state (observeGitHubInstallationLifecycle) still runs unconditionally. A locally disconnected tenant is never the association acted on, so the earlier never-act-on-a-disconnected-tenant fix is not reopened. Test: installation-handler.db 'recovers the connected tenant when a disconnected former tenant is also present' — with a disconnected row inserted first, the connected suspended tenant stayed suspended before this fix and now becomes active.
…pend findIntegrationByInstallationId has no health filter and no ordering, so for an installation with a retained locally disconnected row it can return that row instead of the tenant that still owns the installation. The route guard then dropped the unsuspend delivery before the handler ran, and the handler discarded the disconnected row, so the live tenant's suspension was never cleared — the same stuck state, still reachable. Add findConnectedIntegrationByInstallationId (shared condition builder with findIntegrationByInstallationId, plus github_disconnected_at IS NULL) and use it for the unsuspend path in both places: the webhook route resolves the connected tenant for logging and dispatch, and the handler resolves the connected tenant for the tenant-facing unsuspend, instead of resolving an arbitrary row and discarding it. Suspend uses the same connected resolution. Canonical state (observeGitHubInstallationLifecycle) still runs unconditionally, and a locally disconnected tenant is never acted on. Tests: webhook-handler 'does not log installation.unsuspend when only a disconnected former tenant remains' replaces the drop-the-event assertion, and the suspended/connected cases assert the connected lookup is used; installation-handler mocks updated to the new lookup.
Two defects in the competing-tenant guard added for the legacy install path: - It ran unconditionally, so a connection-management-ENABLED install was rejected with installation_already_claimed even though connectVerifiedGitHubInstallation had already admitted and committed the shared association. The guard now only runs on the legacy (management-disabled) writer path, where the unbound-commit-then-bind window exists; the verified path binds directly as before. - The owner comparison used ne() against nullable columns, and is NULL, so a competitor owned by the other owner type (for example an org-owned row when the caller is user-owned) was invisible and the guard would bind anyway, reopening the two-tenants race. NULL is now treated as a different owner explicitly. Tests (callback route suite, DB-backed): 'does not block a management-enabled install when a shared tenant already owns the installation' (returned installation_already_claimed before the gating fix) and 'refuses the legacy bind when a competitor of the other owner type owns the installation' (bound before the owner-comparison fix).
…action The legacy (management-disabled) callback committed the association through upsertPlatformIntegrationForOwner in its own transaction and only then ran the competing-tenant guard and the canonical bind in a second transaction. The advisory lock serialized admission, but a bind failure or an interrupted request left a committed active-but-unbound association behind even though an error was returned, and a concurrent pending-install completion could then attach a second tenant. The legacy path now runs lockGitHubInstallationIdentity, the NULL-safe cross-owner competitor check, the write, observeGitHubInstallationLifecycle, the owner-scoped read, and the bind inside ONE db.transaction, so any failure rolls the whole thing back and the caller still gets today's installation_already_claimed / error redirect. upsertPlatformIntegrationForOwner and findIntegrationByInstallationIdForOwner gained optional transaction parameters (the same pattern autoCompleteInstallation and the bind already use) so the writer joins the caller's transaction instead of opening its own. The verified-writer path is untouched: it still uses its own writer and binds directly. Tests (callback route suite, DB-backed): 'rolls back the legacy write when the canonical bind fails' (the real writer runs inside the caller transaction; the failed bind leaves no association row) and the competitor test now also asserts nothing was written for the caller.
installation.suspend in the webhook route still resolved an arbitrary association with findIntegrationByInstallationId and early-returned Integration unavailable through isRoutableGitHubIntegration when that row happened to be a retained disconnected former tenant. handleInstallationSuspend therefore never ran, so the canonical installation state was never reconciled — the same class already fixed for unsuspend, but only in the handler. Mirror the unsuspend fix: the suspend route resolves the connected association with findConnectedIntegrationByInstallationId and no longer applies the generic routability early-return, so the handler always runs and reconciles canonical state even when only a disconnected tenant remains. The handler still restricts the tenant-facing suspend to the connected, non-disconnected association, so never-act-on-a-disconnected-tenant is preserved. Tests: 'dispatches installation.suspend for the connected tenant when a disconnected former tenant exists' (asserts the unfiltered lookup is not used, the delivery is logged, and the handler runs) and 'still dispatches installation.suspend when only a disconnected former tenant remains' (handler runs for canonical state, nothing logged against the disconnected tenant). Both fail against the pre-fix route.
Resolves a migration-number collision: main added 0243_ambiguous_namor and 0244_many_vermin (organization_service_fee_exemptions, stripe_service_fee_assessments) at the same indices our stack already used. Kept main's 0243/0244 as-is and renumbered our five migrations above them via the drizzle workflow (schema.ts staged through the same sequence used to originally generate them, then pnpm drizzle generate at each step, never hand-writing journal/snapshot content): 0243_ambiguous_namor (main, unchanged) 0244_many_vermin (main, unchanged) 0245_reconcile_orphaned_github_installation_id (was 0243) 0246_salty_cyclops (was 0244) 0247_calm_lockheed (was 0245) 0248_clean_the_renegades (was 0246) 0249_groovy_agent_zero (new) Verified every regenerated file is byte-identical to its pre-renumber content (0245/0246/0247/0248 diffed clean against backups; 0247 still carries the DROP INDEX CONCURRENTLY IF EXISTS retry-safety guards and COMMIT/BEGIN fencing unchanged). 0249 is a new migration: pulled the schema-only part of feat/github-connections-pr4's commit 805e161 (github_installation_webhook_receipts.status + its CHECK constraint) forward onto this branch, since PR4 depends on it and the task asked for our five migrations to land at 0245-0249 inclusive of it. Did not pull PR4's accompanying application-logic changes (github-installations.ts, webhook-handler.ts, routers) - that is a separate feature port, out of scope for a migration renumber. schema.ts, schema.test.ts, and all four cloud-agent-next router files (apps/web/src/routers/cloud-agent-next-router.ts + .test.ts, organizations/organization-cloud-agent-next-router.ts + .test.ts) auto-merged cleanly with no conflicts; verified by inspection that both sides' changes are present (main's service-fee tables, worktree-access control/assertSessionWorktree, getMessageResult; our sharing_mode/ sharing_admission_checked_at columns, the four canonical/pending unique indexes, provider_oauth_attempts, the platform_integrations FK, and the githubAppType/platformIntegrationId router output fields). Validated per the database-migrations skill: drizzle-kit check is clean; drizzle-kit generate reports "No schema changes, nothing to migrate" (schema.ts matches the regenerated chain exactly); the full chain 0000-0249 replays successfully via pnpm drizzle:verify-bootstrap against a fresh throwaway database, and again via a direct migrate run, confirming all four unique index names, the platform_integrations_github_installation_id_..._fk (ON DELETE restrict), and both CHECK constraints (github_app_installations_sharing_mode_check, github_installation_webhook_receipts_status_check) are present with unchanged names. _journal.json has 250 entries, idx 0-249 with no gaps or duplicates. pnpm --filter web typecheck clean; oxlint clean on schema.ts; 587 tests pass across github-installations, connection-service, runtime-authorization, webhook-handler (+ dispatch integration), installation-handler, platform-integrations, both cloud-agent-next routers, schema.test.ts, review-shared-repositories, prepare-review-payload, and github-integration-helpers. pnpm format made no changes; git diff --check clean (excluding pre-existing trailing whitespace in a vendored patches/react-native@0.86.3.patch, unrelated to this merge). No conflict markers anywhere in the repository.
…ns-pr4 Reconciles PR4's security hardening with the squashed PR2 (#5978) + PR3 (#6089) now in main. Eleven files conflicted; every resolution keeps main's shared-installation base and layers PR4's hardening on top (verified PR4 had no behavior that main lacked in these regions — main's conflicted hunks were the pre-hardening base): - callback/route.ts: the atomic legacy write+guard+bind transaction under lockGitHubInstallationIdentity, the NULL-safe 'col::text IS DISTINCT FROM owner.id' competing-owner guard, the verifiedIdentity gating (verified path never hits the legacy guard), and the installation_already_claimed mapping. - installation-handler.ts: the unbound-peer locked existence check (pending|active, bound or unbound, excluding the pending row and disconnected rows), the single-transaction complete+bind, and connected association resolution for suspend/unsuspend. - webhook-handler.ts: isRoutableGitHubIntegration health gating, connected tenant resolution for suspend/unsuspend, and exact association id threading into the deferred-dispatch authorization check. - github-installations.ts: the incumbent personal-owner reconnect guard, the active/disconnected sharing split, review terminalization with settle-after-commit, identity-based canonical/sibling resolution on uninstall, and the transaction-capable bind. - platform-integrations.ts: findConnectedIntegrationByInstallationId and the transaction-capable writers; connection-service keeps the best-effort all-repositories inventory sync. - github-apps-router.ts: disconnected rows excluded from installation cardinality; sharing-compatibility.ts: disconnected associations excluded from compatibility assessment. - runtime-authorization.ts needed no conflict resolution: main's reason-based API already carries the exact-id allowShared carve-out, so the generic/empty-string path stays exclusive-only. Test files are a union of both sides (all main tests retained, no duplicate test names); main's obsolete 'reconnects the same association after local disconnect and rejects another owner' is not kept because it asserts the superseded claimed_by_other_owner behavior for a disconnected incumbent, whose two scenarios PR4 covers with its split tests. Migrations are untouched: packages/db/src/migrations and schema.ts are byte-identical to main (single 0243_ambiguous_namor..0249_groovy_agent_zero).
This was referenced Sep 17, 2026
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
2 participants
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
Fixes connect/disconnect lifecycle defects found during live two-org testing of the shared-GitHub-installations stack. No new scope, no auth-model change, no migration. Stacked on
feat/github-connections-pr3.suspendedwith its installation still bound, blocking fresh install, connect-existing, and uninstall. Disconnected rows no longer count toward cardinality checks, and uninstall keys off whether any other tenant is still connected.exchangeGitHubOAuthCodenever forwardedcode_verifier(@octokit/oauth-methodsdrops it for GitHub Apps), so the OAuth-first connect/attach flow failed withinvalid_grant. Now exchanges directly against GitHub's token endpoint and surfaces GitHub's real error; non-PKCE callers unchanged.github_authorized_*now recorded on every connect path via a shared helper.Verification
pandemicsyn; Org B discovers and attaches the same canonical installation. DB confirmed onegithub_app_installationsrow with two active associations. Screenshots below.pnpm --filter web typecheck,oxlinton changed files,pnpm format,git diff --check— clean.Visual Changes
Reviewer Notes
claimed_by_other_owner) while any other owner is actively connected. What changed is (a) an incumbent personal owner reconnecting/refreshing their own existing association is no longer locked out by a tenant that attached afterwards, and (b) a purely locally disconnected other-owner row no longer blocks a new personal claimant, matching the disconnected-relinquish semantics already used on the org path. A disconnected row is still never treated as an active incumbent.GitHubConnectionAttemptState.test.tsxis excluded by the repo's.test.ts-onlyjesttestMatch(pre-existing, out of scope); verified passing via a temporary local override.