diff --git a/packages/views/locales/en/settings.json b/packages/views/locales/en/settings.json index a3445d026..5ec3f45f1 100644 --- a/packages/views/locales/en/settings.json +++ b/packages/views/locales/en/settings.json @@ -302,7 +302,7 @@ }, "slack": { "section_title": "Slack", - "page_description": "Connect each Multica Agent to a Slack bot. A workspace admin installs it with one click via OAuth; members can then DM the bot, @mention it in channels, and type /issue to spin up a new Multica issue.", + "page_description": "Connect each Multica Agent to a Slack bot. A workspace admin installs it with one click via OAuth; members can then DM the bot or @mention it in a channel, and start a message with /issue (e.g. \"@bot /issue Fix the login bug\") to spin up a new Multica issue.", "not_enabled_title": "Slack integration not enabled", "not_enabled_description_prefix": "Set", "not_enabled_description_suffix": "on the server to enable Slack bot installations.", diff --git a/packages/views/locales/zh-Hans/settings.json b/packages/views/locales/zh-Hans/settings.json index cedaeb75f..1be73a2a1 100644 --- a/packages/views/locales/zh-Hans/settings.json +++ b/packages/views/locales/zh-Hans/settings.json @@ -302,7 +302,7 @@ }, "slack": { "section_title": "Slack", - "page_description": "把每个 Multica Agent 连接到一个 Slack 机器人。工作区管理员通过 OAuth 一键安装；成员之后即可私聊机器人、在频道中 @ 它，并输入 /issue 创建新的 Multica issue。", + "page_description": "把每个 Multica Agent 连接到一个 Slack 机器人。工作区管理员通过 OAuth 一键安装；成员之后即可私聊机器人，或在频道中 @ 它，并以 /issue 开头发消息（例如「@机器人 /issue 修复登录问题」）来创建新的 Multica issue。", "not_enabled_title": "Slack 集成未启用", "not_enabled_description_prefix": "在服务器上设置", "not_enabled_description_suffix": "以启用 Slack 机器人安装。", diff --git a/server/internal/integrations/slack/app_connector.go b/server/internal/integrations/slack/app_connector.go index b4b428623..f9ca66a3a 100644 --- a/server/internal/integrations/slack/app_connector.go +++ b/server/internal/integrations/slack/app_connector.go @@ -162,8 +162,24 @@ func (c *AppConnector) connectOnce(ctx context.Context) error { api := slack.New("", slack.OptionAppLevelToken(c.appToken)) sm := socketmode.New(api) - runErr := make(chan error, 1) - go func() { runErr <- sm.RunContext(ctx) }() + // Each connection runs under its OWN cancellable context, not the parent + // ctx directly. Every exit path (handler error, event-stream close, ctx + // cancellation) cancels runCtx and waits for the run goroutine to observe it + // and exit — so a transient handler error tears the live connection down + // before Run reconnects. Without this, the old Socket Mode goroutine would + // keep running on the long-lived ctx, leaking the connection/goroutine and + // consuming events into an unread channel while a second connection opens. + runCtx, runCancel := context.WithCancel(ctx) + runErr := make(chan error, 1) // buffered: the goroutine sends once and exits even if nobody reads + done := make(chan struct{}) + go func() { + runErr <- sm.RunContext(runCtx) + close(done) + }() + defer func() { + runCancel() + <-done + }() for { select { @@ -216,8 +232,16 @@ func (c *AppConnector) handleSocketEvent(ctx context.Context, sm *socketmode.Cli case socketmode.EventTypeIncomingError, socketmode.EventTypeErrorBadMessage: c.logger.WarnContext(ctx, "slack: socket mode error", "event", evt.Type) default: - // Interactive / slash-command / other events are out of scope; ACK so - // Slack does not retry, then ignore. + // Interactive / slash-command / other envelopes are intentionally + // ignored (ACK so Slack does not retry). In particular, /issue is NOT a + // registered Slack slash command — the hosted app requests no `commands` + // scope, so Slack never routes a slash-command envelope here. Issue + // creation runs through the normal message path instead: `@bot /issue + // ` in a channel (the mention is stripped, leaving "/issue …") or + // `/issue <title>` in a DM with the bot, which the engine's + // ParseIssueCommand picks up. Adding native slash-command support + // (scope + registration + response_url handling) is a possible later + // enhancement, not required for the message-driven /issue flow. if evt.Request != nil { _ = sm.Ack(*evt.Request) } diff --git a/server/internal/integrations/slack/install.go b/server/internal/integrations/slack/install.go index 1fe4e72f0..f8ad4ade7 100644 --- a/server/internal/integrations/slack/install.go +++ b/server/internal/integrations/slack/install.go @@ -76,7 +76,7 @@ func (c OAuthConfig) supported() bool { // installQueries is the slice of generated queries InstallService needs. // *db.Queries satisfies it. type installQueries interface { - UpsertChannelInstallation(ctx context.Context, arg db.UpsertChannelInstallationParams) (db.ChannelInstallation, error) + UpsertChannelInstallationByAppID(ctx context.Context, arg db.UpsertChannelInstallationByAppIDParams) (db.ChannelInstallation, error) CreateChannelUserBinding(ctx context.Context, arg db.CreateChannelUserBindingParams) (db.ChannelUserBinding, error) ListChannelInstallationsByWorkspace(ctx context.Context, arg db.ListChannelInstallationsByWorkspaceParams) ([]db.ChannelInstallation, error) GetChannelInstallationInWorkspace(ctx context.Context, arg db.GetChannelInstallationInWorkspaceParams) (db.ChannelInstallation, error) @@ -217,7 +217,11 @@ func (s *InstallService) Complete(ctx context.Context, code, rawState string) (C if err != nil { return CompletedInstall{}, fmt.Errorf("encode slack installation config: %w", err) } - inst, err := s.q.UpsertChannelInstallation(ctx, db.UpsertChannelInstallationParams{ + // Team-keyed upsert: a Slack workspace (team_id) is one installation. Re- + // connecting the same team — including to represent a different agent — + // updates the existing row rather than colliding with the (channel_type, + // app_id) unique index (Niko review must-fix #3). + inst, err := s.q.UpsertChannelInstallationByAppID(ctx, db.UpsertChannelInstallationByAppIDParams{ WorkspaceID: wsID, AgentID: agentID, ChannelType: string(TypeSlack), diff --git a/server/internal/integrations/slack/install_test.go b/server/internal/integrations/slack/install_test.go index 8d773ae6c..671a0f688 100644 --- a/server/internal/integrations/slack/install_test.go +++ b/server/internal/integrations/slack/install_test.go @@ -40,13 +40,13 @@ func mustUUID(t *testing.T, s string) pgtype.UUID { } type fakeInstallQueries struct { - upsertParams db.UpsertChannelInstallationParams + upsertParams db.UpsertChannelInstallationByAppIDParams bindParams db.CreateChannelUserBindingParams bindCalled bool rowID pgtype.UUID } -func (f *fakeInstallQueries) UpsertChannelInstallation(_ context.Context, arg db.UpsertChannelInstallationParams) (db.ChannelInstallation, error) { +func (f *fakeInstallQueries) UpsertChannelInstallationByAppID(_ context.Context, arg db.UpsertChannelInstallationByAppIDParams) (db.ChannelInstallation, error) { f.upsertParams = arg return db.ChannelInstallation{ ID: f.rowID, diff --git a/server/pkg/db/generated/channel.sql.go b/server/pkg/db/generated/channel.sql.go index 7133c7ad7..62d5907fe 100644 --- a/server/pkg/db/generated/channel.sql.go +++ b/server/pkg/db/generated/channel.sql.go @@ -1085,3 +1085,66 @@ func (q *Queries) UpsertChannelInstallation(ctx context.Context, arg UpsertChann ) return i, err } + +const upsertChannelInstallationByAppID = `-- name: UpsertChannelInstallationByAppID :one +INSERT INTO channel_installation ( + workspace_id, agent_id, channel_type, config, installer_user_id +) VALUES ( + $1, $2, $3, $4, $5 +) +ON CONFLICT (channel_type, (config ->> 'app_id')) DO UPDATE SET + workspace_id = EXCLUDED.workspace_id, + agent_id = EXCLUDED.agent_id, + config = EXCLUDED.config, + installer_user_id = EXCLUDED.installer_user_id, + status = 'active', + installed_at = now(), + updated_at = now() +RETURNING id, workspace_id, agent_id, channel_type, config, status, ws_lease_token, ws_lease_expires_at, installer_user_id, installed_at, created_at, updated_at +` + +type UpsertChannelInstallationByAppIDParams struct { + WorkspaceID pgtype.UUID `json:"workspace_id"` + AgentID pgtype.UUID `json:"agent_id"` + ChannelType string `json:"channel_type"` + Config []byte `json:"config"` + InstallerUserID pgtype.UUID `json:"installer_user_id"` +} + +// Team-keyed install / re-install for channels whose natural identity is the +// platform workspace, not the (agent) pairing. Slack: one Slack workspace +// (team_id, stored as config->>'app_id') maps to exactly one installation, so +// re-connecting it — even to represent a DIFFERENT agent — UPDATES the existing +// row (moving agent_id) instead of colliding with the (channel_type, app_id) +// unique index. Contrast UpsertChannelInstallation, whose conflict key is +// (workspace_id, agent_id, channel_type): right for Feishu (one app per agent), +// wrong for Slack (a second agent connecting the same team would hit the +// (channel_type, app_id) index). NOTE: a re-connect that would move the team to +// an agent that already holds a different Slack install still trips the +// (workspace_id, agent_id, channel_type) unique constraint — that genuine +// conflict surfaces as an error the OAuth callback turns into a redirect. +func (q *Queries) UpsertChannelInstallationByAppID(ctx context.Context, arg UpsertChannelInstallationByAppIDParams) (ChannelInstallation, error) { + row := q.db.QueryRow(ctx, upsertChannelInstallationByAppID, + arg.WorkspaceID, + arg.AgentID, + arg.ChannelType, + arg.Config, + arg.InstallerUserID, + ) + var i ChannelInstallation + err := row.Scan( + &i.ID, + &i.WorkspaceID, + &i.AgentID, + &i.ChannelType, + &i.Config, + &i.Status, + &i.WsLeaseToken, + &i.WsLeaseExpiresAt, + &i.InstallerUserID, + &i.InstalledAt, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} diff --git a/server/pkg/db/queries/channel.sql b/server/pkg/db/queries/channel.sql index fa9f0f716..7f474bfd6 100644 --- a/server/pkg/db/queries/channel.sql +++ b/server/pkg/db/queries/channel.sql @@ -38,6 +38,34 @@ ON CONFLICT (workspace_id, agent_id, channel_type) DO UPDATE SET updated_at = now() RETURNING *; +-- name: UpsertChannelInstallationByAppID :one +-- Team-keyed install / re-install for channels whose natural identity is the +-- platform workspace, not the (agent) pairing. Slack: one Slack workspace +-- (team_id, stored as config->>'app_id') maps to exactly one installation, so +-- re-connecting it — even to represent a DIFFERENT agent — UPDATES the existing +-- row (moving agent_id) instead of colliding with the (channel_type, app_id) +-- unique index. Contrast UpsertChannelInstallation, whose conflict key is +-- (workspace_id, agent_id, channel_type): right for Feishu (one app per agent), +-- wrong for Slack (a second agent connecting the same team would hit the +-- (channel_type, app_id) index). NOTE: a re-connect that would move the team to +-- an agent that already holds a different Slack install still trips the +-- (workspace_id, agent_id, channel_type) unique constraint — that genuine +-- conflict surfaces as an error the OAuth callback turns into a redirect. +INSERT INTO channel_installation ( + workspace_id, agent_id, channel_type, config, installer_user_id +) VALUES ( + $1, $2, $3, $4, $5 +) +ON CONFLICT (channel_type, (config ->> 'app_id')) DO UPDATE SET + workspace_id = EXCLUDED.workspace_id, + agent_id = EXCLUDED.agent_id, + config = EXCLUDED.config, + installer_user_id = EXCLUDED.installer_user_id, + status = 'active', + installed_at = now(), + updated_at = now() +RETURNING *; + -- name: GetChannelInstallation :one -- Scoped by channel_type: a per-channel caller (e.g. the Feishu store) -- must never resolve another channel's installation by guessing its UUID.