refactor(slack): store the workspace bot token on the workspace - #5004
Open
RSO wants to merge 6 commits into
Open
refactor(slack): store the workspace bot token on the workspace#5004RSO wants to merge 6 commits into
RSO wants to merge 6 commits into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe soft-delete gap on Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (7 files, incremental)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit ba47402)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ba47402)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe previously flagged unique-index drop is fully reverted — the migration is now purely additive and Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (12 files)
Fix these issues in Kilo Cloud Previous review (commit b52235a)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryDropping Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Reviewed by kimi-k3 · Input: 116K · Output: 18.8K · Cached: 528K Review guidance: REVIEW.md from base branch |
Slack issues exactly one installation, and therefore one bot token, per (app, workspace). The token was stored on each owner's `platform_integrations.metadata.access_token`, which ties a workspace-level credential to a per-owner row. Add `slack_workspace_installations`, keyed on the Slack team ID, and make it the read path for the bot token. Every completed OAuth install writes it, so the record stays current. `metadata.access_token` and `metadata.bot_user_id` are still mirrored and the read falls back to them, which keeps a rolling deploy and a rollback safe; a follow-up removes both. Lifecycle is handled everywhere a Slack integration can go away: disconnecting, Slack's workspace-wide `app_uninstalled` event, the dev-only row removal, and `softDeleteUser`. The workspace record is removed once no integration references the workspace, so a stored token cannot outlive the integrations that used it. Also clear `suspended_at` and `suspended_by` when an install activates an integration. They were left set, so rows suspended by 0108_wise_psylocke kept stale suspension metadata after reconnecting. The migration is purely additive: it creates the table and seeds it from the existing per-integration tokens. No index or column is changed, and `UQ_platform_integrations_slack_platform_inst` still limits a workspace to one owner. Allowing several organizations per workspace needs message routing to tell owners apart and is left to a follow-up.
RSO
changed the title
…paces An owner holds at most one Slack integration, so installing a different workspace repoints that row's `platform_installation_id`. Nothing then referenced the previous workspace, so its `slack_workspace_installations` row and bot token were left behind with no path to ever remove them: uninstall, `app_uninstalled`, and soft-delete cleanup all resolve the record from an integration. Capture the previous team ID and, once the row has moved, delete the old workspace record if nothing references it. Apply the same guard when persisting the integration fails after the workspace record was written, which would otherwise orphan the new record. Both paths go through the unreferenced check, so a workspace another owner legitimately claimed in the meantime is left alone.
Reading the workspace record first is unsafe until the previous release is gone. That release writes tokens only to `platform_integrations.metadata.access_token`: a disconnect there deletes the integration row without touching the workspace record, and the following reconnect recreates the row with a fresh token in metadata only. The workspace record is left holding the revoked token, and reading it first would make this release keep using that revoked token for auth checks, permalinks and direct sends until someone reinstalls. Every writer updates metadata, so metadata is never staler than the workspace record while the mirror exists; the reverse does not hold. Read metadata first and use the workspace record as the fallback. The follow-up that removes the mirror removes this preference with it, which is what leaves the workspace record as the only source. Also avoids a query on the common path, since metadata is populated for every currently connected workspace.
`last_installed_by_user_id` is recorded on every install, including installs made for an organization. Those workspace records outlive the installer's account, so they kept pointing at a soft-deleted user. The `set null` foreign key does not cover this: soft deletion retains the `kilocode_users` row, so the reference is never cleared. Null the column on the rows that survive, matching how the surrounding cleanup already handles actor references such as `dismissed_by_user_id`.
Two review points, with one root cause: the install wrote `slack_workspace_installations` and `platform_integrations` as separate commits, so the code had to compensate for a half-applied install. Wrap both writes in a transaction. A failed integration write now rolls the workspace record back on its own, so the compensating delete in the catch block goes away. That delete was awaited before the caught error was inspected, so a transient failure in it masked the original error, including the `SlackWorkspaceAlreadyConnectedError` translation the `workspace_already_connected` redirect depends on. The teardown guard was also check-then-act. Making it atomic on its own was not enough: an install committed the workspace record before the integration row, so a concurrent teardown could observe neither and delete a record that was about to be referenced. With the install now atomic, fold the reference check into the delete statement. Either it sees the integration row and keeps the record, or the install has not committed and there is no record to remove. `countSlackConnections` is gone; the reference check is no longer a separate query. Covers both with real-database tests, including that a failed integration write leaves no workspace record behind.
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
1 participant
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
Slack issues exactly one installation, and therefore one bot token, per (app, workspace). That token was stored on each owner's
platform_integrations.metadata.access_token, which ties a workspace-level credential to a per-owner row.slack_workspace_installations, keyed on the Slack team ID, to hold it.suspended_at/suspended_bywhen an install re-activates an integration.The migration is purely additive. It creates the table and seeds it from the existing per-integration tokens. No index and no column is changed.
Scope: this does not yet allow multiple organizations per workspace
An earlier revision of this PR dropped
UQ_platform_integrations_slack_platform_inst. That was wrong as a standalone change, and I've reverted it here.Dropping that index only becomes correct once message routing can tell two owners apart. Until then the application layer still refuses a second owner via
getConflictingSlackInstallation, so the index is the correct enforcement of a rule that still exists — removing it would only open the OAuth race where two concurrent callbacks both pass the pre-check, after whichgetPlatformIntegration()'s unordered.limit(1)could route work and billing to an arbitrary owner.So this PR is the prerequisite refactor: get the workspace token onto the workspace. The index drop plus link-based org routing follows separately.
Why a separate table
The token is a property of the workspace, not of an owner. Keeping it per-owner is what makes multi-org impossible without duplicating a secret across rows that then drift apart on re-install. The Chat SDK's own installation store is already keyed on team id only (
@chat-adapter/slack,slack:installation:{teamId}), so this matches both Slack's model and the SDK's.bot_tokenis stored unencrypted, at parity with themetadata.access_tokencopy it replaces. Moving Slack onto the keyed envelope encryption used by GitLab/Bitbucket is a pre-existing gap, deliberately not bundled here.Rolling-deploy safety, and why reads still prefer
metadatametadata.access_tokenandmetadata.bot_user_idare still written, andgetSlackBotTokenprefers them, with the workspace record as the fallback.That order looks backwards for a table introduced as the workspace-level store, and it is deliberate. Every writer updates
metadata— the previous release writes only there, this release mirrors into it — sometadatais never staler than the workspace record. The reverse does not hold: a disconnect and reconnect served by the previous release deletes and recreatesplatform_integrationswithout touching the workspace record, leaving it holding a revoked token. Reading the workspace record first would then keep using that revoked token for auth checks, permalinks and direct sends until someone reinstalled.So this PR expands (write both, read the safe one) and the follow-up contracts (drop the mirror, which flips the preference to workspace-only). Doing it in that order is what makes the rollout safe in both directions.
Lifecycle coverage
uninstallApp(disconnect)deleteInstallationByTeamId(app_uninstalled)removeDbRowOnly(dev-only)softDeleteUsersoftDeleteUserdeletes before the integration rows go, because deleting the integration first would lose the team IDs needed to find these records.Verification
pnpm drizzle:verify-bootstrap— clean apply from an empty databaseapps/webtypecheck: 0 errors.oxlint: 0 warnings, 0 errors.oxfmtclean.slack-service,slack-workspace-installation,bot,platform-helperssrc/lib/user/index.test.tssuite: 97 passed, including newsoftDeleteUsercoverage against a real databaseUQ_platform_integrations_slack_platform_instpresent onplatform_integrationsbot_user_id, scopesplatform_installation_id) excludedVisual Changes
N/A
Reviewer Notes
--> statement-breakpointmarker perpackages/db/AGENTS.md. It usesDISTINCT ON ... ORDER BY updated_at DESCrather than the unorderedLIMIT 1that0108used, andON CONFLICT DO NOTHING.access_tokenorbot_user_id— so the backfill seeds every connected workspace.last_installed_by_user_idis left NULL by the backfill on purpose. The nearest source column,platform_integrations.created_by_user_id, is plaintext()with no FK and may hold ids no longer inkilocode_users, which would violate the new FK. New installs populate it from the acting user.organizationsrow cascades itsplatform_integrationsaway and orphans the workspace record. Verified in the fresh-DB run above. Organizations are only hard-deleted by dev scripts (scripts/db/create-trial-test-orgs.ts,app/api/dev/create-kilocode-org), never by product code, so there is no production path to hook. Flagging rather than leaving it silent; worth revisiting if org deletion is ever productised.slack-service.test.tstheslack-workspace-installationmock forwards lazily through arrow wrappers. The factory runs whileslack-serviceis being imported, which is before theconstmock declarations initialize, so capturing the mock functions directly throws a TDZReferenceError.Follow-ups
UQ_platform_integrations_slack_platform_instand add link-based org routing, enabling multiple organizations per workspace.metadata.access_token/metadata.bot_user_idmirror, which flipsgetSlackBotTokento read the workspace record only. Safe once the release below is fully rolled out.bot_tokenwith the keyed envelope scheme used by GitLab/Bitbucket.getPlatformIntegrationByBotUserIdmatches onplatform+metadata->>'bot_user_id'only. The bot user id is app-wide, so it already returns an arbitrary row across workspaces today.Review notes (codex autoreview)
Four rounds. Three findings accepted and fixed, one rejected:
737ee64b3. Reachable:getInstallation(owner)finds the owner's single row regardless of team, so re-running connect repoints it. A regression this PR introduced — previously the token lived inmetadataon that same row and was simply overwritten. Also guarded the case where the integration write fails after the workspace record was written.metadatatoken during the rollout0fb9e5568by inverting the read preference. See the rolling-deploy section above.last_installed_by_user_idkept referencing a soft-deleted user for org-owned installs8047f690d. Theset nullFK does not cover it: soft deletion retains thekilocode_usersrow. Now nulled on surviving rows, matching howdismissed_by_user_idand friends are already handled.uninstallApphappens before thecountSlackConnectionsguardUQ_platform_integrations_slack_platform_instlimits a workspace to one owner, so the count is only ever 0 or 1, and the owner's row is deleted before it is consulted — it is always 0. Empirically confirmed: inserting a second owner for the same team is rejected by that constraint. It is the correct requirement for the follow-up that drops the index, so the invariant and the required reordering are documented at the call site in5b6b84284.The final confirmation run could not complete: the Codex workspace is out of credits. The three accepted fixes were each verified by targeted tests before committing, but the branch has not had a clean review pass end to end.