Skip to content

feat(github): add restricted secondary connections - #6510

Merged
pandemicsyn merged 10 commits into
mainfrom
feat/github-agent-only-connections
Sep 23, 2026
Merged

pandemicsyn merged 10 commits into
mainfrom
feat/github-agent-only-connections

Conversation

@pandemicsyn

@pandemicsyn pandemicsyn commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Allow an approved second Kilo organization to use an existing GitHub App installation from Slackbot and browser Cloud Agent without taking over the original connection or disabling its workflows.

  • Store explicit workflow and agent_only roles on separate owner-scoped connections. Preserve exact connection identity and trusted purpose throughout managed agent sessions and credentials.
  • Keep Code Reviewer, Security Agent, GitHub bot, deployments, App Builder, and Gastown integration use with the workflow connection. Disconnecting that owner never promotes a secondary.
  • Use the existing GITHUB_SHARED_INSTALLATION_ORGANIZATION_IDS allowlist for admission. Already-allowlisted organizations need no additional opt-in or new feature flag.
  • Keep secondary model settings and independent Slack setup, with Agent access labeling and local disconnect instead of upstream uninstall.
  • Merge current main, retaining its removal of the obsolete per-repository customization editor and regenerating migrations on its latest history.

Verification

  • Signed in with a synthetic, preseeded local account and opened both the original and secondary organization GitHub settings.
  • Confirmed the secondary Agent access badge, Slack/Cloud Agent restriction copy, and model control; the original connection does not show that restriction.
  • Opened the secondary management menu and confirmed local disconnect was available while upstream uninstall was absent.
  • During the earlier smoke check, disconnected the synthetic secondary through the UI and inspected local SQL state: the original workflow connection stayed connected, the installation stayed active, and both roles were unchanged.
  • Captured desktop and 390px mobile-width screenshots of the merged UI; no horizontal overflow was observed.
  • Real Slack/GitHub OAuth and production credential end-to-end verification have not been performed.

Visual Changes

Actual screenshots from local synthetic fixtures. Both columns use the current merged build: this compares the original workflow connection with a secondary connection, not two historical builds.

Original workflow connection Secondary agent-access connection
Original workflow connection Secondary connection and local-disconnect menu
Secondary connection at mobile width Secondary GitHub connection at mobile width

Reviewer Notes

  • Why two migrations: 0255_github_connection_role adds the role column and generated CHECK. 0256_github_connection_role_indexes explicitly commits that DDL before building the unique indexes concurrently, then starts a separate transaction for backfill. This avoids extending the column/CHECK table lock through index builds and backfill. Separate filenames alone would not release it. Real-migrator probes on PG16 and PG18 confirmed the validated CHECK was committed and no AccessExclusiveLock remained at all three checkpoints. The initial CHECK still requires ordinary validation locking; this is not a zero-lock migration.
  • Existing allowlist, no extra gate: Organizations already allowed by GITHUB_SHARED_INSTALLATION_ORGANIZATION_IDS get secondary admission with this implementation. GITHUB_AGENT_ONLY_CONNECTIONS_ENABLED was removed. Existing connection-management, organization authorization, and multiple-installation rules remain unchanged. Deploy the role migrations and all affected web/Worker consumers together, and reconcile unresolved historical groups. No remote configuration change or deployment has been performed.
  • Backfill and recovery: The ranked CTE is materialized for PostgreSQL 16 compatibility. Ambiguous historical ownership stays blocked. Fresh verified reconnect can recover only narrowly classified sole legacy null-role connections; shadow, pending, dedup, and conflicting claims remain fail-closed.
  • Review follow-up: All six existing inline review threads were answered and resolved in 60de8c0e2. Fixes preserve legacy create fingerprints, make bare --reportRoles read-only, validate non-GitHub replay identities, and clarify unknown-role/model-setting UI. The removed preview component stays removed. Missing broker authorization intentionally remains fail-closed, including cached grants; deploy the compatible broker before the consumers rather than bypassing reauthorization. Added regressions cover each disposition.
  • Model-setting scope: Saving a preference on the GitHub connection does not override Slack integration model settings or the model selected in a browser Cloud Agent session. The helper now states this explicitly. Unknown roles display access-unavailable guidance rather than claiming agent-only or workflow access.
  • Local validation: Cloud Agent test:all passed: 7,489 unit tests, 721 Workers-runtime tests, and 1,595 wrapper tests, with three unit skips. The affected web matrix passed 600 tests across 45 suites, with one skip. The earlier PG16 SQL matrix passed 147 tests. Fresh full replays passed all 257 migrations on PG16 and PG18, with valid concurrent indexes and successful lock probes. Scoped typechecks, lint, formatting, and feature-diff whitespace checks passed.
  • Historical fixture now fixed upstream: Current main corrects the previously missing 0204 fixture reference. The standalone schema suite was rerun and now passes all 59 tests; this branch did not introduce a workaround or suppress that test.
  • Merge and CI: Conflicts were resolved by a normal merge of main, without rebasing or force-pushing. Consult the current GitHub merge/check status for subsequent base changes. New-head CI runs separately; local results above are not a claim that all GitHub checks have completed successfully.
  • Credential boundary: Public origin strings cannot grant secondary access. Direct public start stays workflow-only; approved browser/Slack flows use trusted internal prepare. No mobile-header restrictions or new signing framework were added. Already-issued native tokens retain their existing expiry/revocation behavior; managed preparation/renewal and capability redemption recheck authorization. Full monorepo tests and live OAuth E2E are not claimed.

@pandemicsyn
pandemicsyn marked this pull request as ready for review September 22, 2026 15:53
Comment thread services/cloud-agent-next/src/session/session-registration.ts Outdated
Comment thread apps/web/src/scripts/backfill-github-installations.ts
Comment thread apps/web/src/components/integrations/GitHubRepositoryCustomizationsPreview.tsx Outdated
Comment thread apps/web/src/components/integrations/OrganizationGitHubInstallations.tsx Outdated
Comment thread services/cloud-agent-next/src/sandbox-session/SandboxSession.ts
@kilo-code-bot

kilo-code-bot Bot commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of c5418744c ("fix(github): address specialist review findings") — the only PR-owned change since 4b0f81509 (main-merge commits touched no PR files) — found no issues; the rework fixes the previously raised UI/role-fallthrough concerns and the new migration predicates and RPC response validation are behavior-preserving for the production path.

Files Reviewed (13 files)
  • ENVIRONMENT.md
  • apps/web/src/components/integrations/OrganizationGitHubInstallations.tsx
  • apps/web/src/components/integrations/OrganizationGitHubInstallations.test.ts
  • apps/web/src/lib/integrations/db/github-installations.test.ts
  • apps/web/src/routers/github-apps-router.ts
  • apps/web/src/routers/github-apps-router.test.ts
  • packages/db/src/migrations/0256_github_connection_role_indexes.sql
  • packages/worker-utils/package.json
  • packages/worker-utils/src/github-authorization.ts
  • services/cloud-agent-next/src/services/git-token-service-client.ts
  • services/cloud-agent-next/src/services/git-token-service-client.test.ts
  • services/cloud-agent-next/src/types.ts
  • services/git-token-service/src/index.ts
What was verified in changed code
  • 0256: the new pi.github_connection_role IS NULL and (assigned_workflow_claims = 1 OR agent_only_claims = 0) predicates plus the role = 'workflow' DESC NULLS LAST ordering are no-ops on the first (and only) production run — 0255 adds the column NULL and this backfill is its first writer — and are conservative when re-applied, preserving existing roles and failing closed when only agent_only rows exist.
  • git-token-service-client.ts: every failure reason the real authorizeCloudAgentGitHubRepo RPC can return (installation-lookup-service set with ambiguous_installation remapped to no_installation_found) is covered by the new shared schema, so safeParse cannot downgrade a current terminal reason to rpc_error; unknown response shapes fail closed.
  • github-apps-router.ts / OrganizationGitHubInstallations.tsx: canRefresh and the model control now gate on an explicit workflow/agent_only role, and badge/description/model states are consistent for workflow, agent_only, null, missing, and non-connected statuses.
  • No leak-prone additions (listeners, timers, unbounded caches) were introduced by the changed code.
Previous Review Summaries (3 snapshots, latest commit 4b0f815)

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

Previous review (commit 4b0f815)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of the test-only commit 4b0f81509 (workflow/agent authorization fixtures) since 60de8c0e2 found no issues; every updated expectation matches current source behavior and no production code changed.

Files Reviewed (5 files)
  • apps/web/src/lib/integrations/db/platform-integrations.test.ts
  • apps/web/src/lib/security-agent/router/shared-handlers.test.ts
  • apps/web/src/routers/cli-sessions-v2-router.test.ts
  • apps/web/src/routers/organizations/organization-cloud-agent-next-router.branches.test.ts
  • apps/web/src/routers/organizations/organization-cloud-agent-next-router.test.ts

Previous review (commit 60de8c0)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of the changes since c5d023adf (fix commit 60de8c0e2, the main merge resolution, and the renumbered role migrations) found no new issues; the prior findings were fixed, superseded by merged main, or are deliberate fail-closed behavior that is now documented for rollout ordering.

Files Reviewed (20 files)
  • apps/web/src/scripts/backfill-github-installations.ts
  • apps/web/src/components/integrations/OrganizationGitHubInstallations.tsx
  • apps/web/src/components/integrations/OrganizationGitHubInstallations.test.ts
  • apps/web/src/lib/bot/tools/spawn-cloud-agent-session.ts
  • apps/web/src/lib/integrations/db/platform-integrations.ts
  • apps/web/src/lib/integrations/db/github-installations-script.test.ts
  • apps/web/src/lib/integrations/github-apps-service.ts
  • apps/web/src/routers/github-apps-router.ts
  • packages/db/src/migrations/0255_github_connection_role.sql
  • packages/db/src/migrations/0256_github_connection_role_indexes.sql
  • packages/db/src/schema.ts
  • services/cloud-agent-next/src/session/session-registration.ts
  • services/cloud-agent-next/src/sandbox-session/SandboxSession.ts
  • services/cloud-agent-next/src/sandbox-control/session-credentials.ts
  • services/cloud-agent-next/src/types.ts
  • services/cloud-agent-next/src/persistence/session-metadata.ts
  • services/cloud-agent-next/src/session/session-prepare.test.ts
  • services/cloud-agent-next/test/integration/sandbox-control.test.ts
  • ENVIRONMENT.md

Test files were read for expected behavior but not reported on; generated Drizzle snapshots and journals were treated as generated.

Verified fixes on current HEAD: the create-intent fingerprint again omits the default workflow purpose (session-registration.ts:1415), the bare --reportRoles flag now runs the read-only report (backfill-github-installations.ts:19), the re-registration guard compares repository URL/UUID identity for non-GitHub repositories (SandboxSession.ts:2585-2594), and the unassigned-role UI no longer advertises Slack/Cloud Agent access (OrganizationGitHubInstallations.tsx:334,451-456). The authorizeCloudAgentGitHubRepo preflight remains fail-closed by design with deploy-order guidance added to ENVIRONMENT.md.

Previous review (commit c5d023a)

Status: 6 Issues Found | Recommendation: Address before merge

Executive Summary

The highest-risk finding is that the new githubAccessPurpose field in the Cloud Agent create-intent fingerprint rotates every stored fingerprint, so same-key retries of GitHub session creates admitted before the deploy now fail with a non-retryable error; the rest are rollout-skew and UX/guard-completeness gaps.

Overview

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

WARNING

File Line Issue
services/cloud-agent-next/src/session/session-registration.ts 1413 githubAccessPurpose added unconditionally to the create-intent serialization rotates stored fingerprints; same-key retries of pre-deploy GitHub creates throw non-retryable session_creation_failed.
services/cloud-agent-next/src/sandbox-control/session-credentials.ts 583 Optional authorizeCloudAgentGitHubRepo binding is treated as a hard credential failure, breaking GitHub sessions during git-token-service rollout skew and skipping the valid-capability cache check.

SUGGESTION

File Line Issue
apps/web/src/scripts/backfill-github-installations.ts 11 Bare --reportRoles is parsed as undefined and silently falls through to the mutating backfill instead of the read-only report.
apps/web/src/components/integrations/GitHubRepositoryCustomizationsPreview.tsx 375 Agent-only copy is driven by !canEditReviews, so null-role connections are described as supporting Slack/Cloud Agent even though they fail closed.
apps/web/src/components/integrations/OrganizationGitHubInstallations.tsx 460 Agent-only model helper omits Cloud Agent and a null role falls into the GitHub-bot wording.
services/cloud-agent-next/src/sandbox-session/SandboxSession.ts 2574 Re-registration guard compares only the repository discriminator for non-GitHub repositories, so a mismatched URL/UUID retry still reports success.
Files Reviewed (91 files)

Reviewed all changed files in PR #6510 against HEAD c5d023adf, including DB migrations and schema (packages/db), the web GitHub authorization core (runtime-authorization.ts, sharing-compatibility.ts, github-apps-service.ts, provider-oauth-attempts.ts, github-apps-router.ts, adapter.ts, webhook handlers), the git-token-service worker and gastown town SCM, services/cloud-agent-next (session registration, credentials, sandbox session, repository-access validation, git-token client), web consumers (bot, slack, security agent/auto-analysis/sync, deployments, code reviews, cloud-agent helpers), and UI/docs (GitHubRepositoryCustomizationsPreview.tsx, OrganizationGitHubInstallations.tsx, GitHubConnectionAttemptState.tsx, ENVIRONMENT.md). 29 of the 91 files are test files, which were read for expected behavior but not reported on. Generated Drizzle snapshots/journals were treated as generated.

Explicitly checked and cleared: the COMMIT;/BEGIN; sequencing around CREATE UNIQUE INDEX CONCURRENTLY in migration 0255 matches the established pattern (0242, 0247); the ranked backfill cannot produce two workflow rows for one github_installation_id under maintained binding invariants (github_installation_id is always bound by the matching (github_app_type, platform_installation_id) identity); agent_only is excluded from webhook dispatch, code review, security agent, deployments, bot, and Gastown lookups with fail-closed handling of null roles; public start remains workflow-only; and no new timers/listeners/subscriptions (memory leaks) were introduced on the changed lines.

Fix these issues in Kilo Cloud


Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0

Review guidance: REVIEW.md from base branch main

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