diff --git a/docs/memory/feature-flows.md b/docs/memory/feature-flows.md index 919fb7f90..46d7212ff 100644 --- a/docs/memory/feature-flows.md +++ b/docs/memory/feature-flows.md @@ -11,6 +11,7 @@ | Date | ID | Feature | Flow | |------|-----|---------|------| +| 2026-04-29 | #584 | feat(slack): UI + API to change Slack DM-default agent — `set_slack_dm_default()` DB method (single-tx clear-then-set), `PUT /api/agents/{name}/slack/channel/dm-default` (owner-only, audit-logged), "Make default" button + tooltip in `SlackChannelPanel.vue`, unbind refuses 409 when target is DM default with siblings remaining | [slack-channel-routing.md](feature-flows/slack-channel-routing.md) | | 2026-04-30 | #598 | sec: AISEC-C2 Layer 2 — restored `.mcp.json` post-deploy editing via structure validation (`services.mcp_validator`). Closed schema, command/transport allowlists, SSRF guard for http/sse, reserved env-ref blocklist, literal-secret detection. 88 unit tests + 22 integration tests. UI placeholder updated; `trinity` server name reserved. | [credential-injection.md](feature-flows/credential-injection.md) | | 2026-04-30 | #590 | sec: AISEC-C2 Layer 1 — backend `ALLOWED_CREDENTIAL_PATHS` tightened; backend `update_agent_file_logic` adds defense-in-depth deny check before proxy; agent-server `EDIT_PROTECTED_PATHS` adds `.mcp.json` and `.credentials.enc`. | [credential-injection.md](feature-flows/credential-injection.md), [file-browser.md](feature-flows/file-browser.md) | | 2026-04-30 | #364 | Web chat file upload — drag-drop/picker in ChatPanel and PublicChat; base64 JSON encoding; shared upload_service; images via vision blocks, non-images via Docker put_archive | [web-chat-file-upload.md](feature-flows/web-chat-file-upload.md) | diff --git a/docs/memory/feature-flows/slack-channel-routing.md b/docs/memory/feature-flows/slack-channel-routing.md index fffc9296e..f395fd73c 100644 --- a/docs/memory/feature-flows/slack-channel-routing.md +++ b/docs/memory/feature-flows/slack-channel-routing.md @@ -38,7 +38,7 @@ As a **platform admin**, I want Slack messages to go through the same execution ### Agent Detail — Sharing Tab (Per-Agent) - `SlackChannelPanel.vue` — Three states: - - **Bound**: Shows `#channel-name`, workspace name, DM default badge, "Unbind" button + - **Bound**: Shows `#channel-name`, workspace name, DM-default badge **or** "Make default" button (with hover tooltip explaining DM routing), and "Unbind" button. The Unbind button is **disabled** when this agent is the DM default *and* the workspace has other bound agents — promoting another agent first via the "Make default" button on its panel is required (#584). - **Unbound**: "Create Slack Channel" button → creates channel in Slack + binds to agent - **Access denied**: Informational message for non-owner shared users - `SharingPanel.vue` — Renders `SlackChannelPanel` between Team Sharing and Public Links sections @@ -55,9 +55,10 @@ As a **platform admin**, I want Slack messages to go through the same execution - `POST /api/settings/slack/install` → `{oauth_url}` (browser redirect) ### API Calls (Per-Agent Channel) -- `GET /api/agents/{name}/slack/channel` → `{bound, channel_name, channel_id, workspace_name}` +- `GET /api/agents/{name}/slack/channel` → `{bound, channel_name, channel_id, workspace_name, is_dm_default, workspace_agent_count}` - `POST /api/agents/{name}/slack/channel` → `{status, channel_name, channel_id, workspace_name}` -- `DELETE /api/agents/{name}/slack/channel` → `{unbound, workspace_name}` +- `DELETE /api/agents/{name}/slack/channel` → `{unbound, workspace_name}` — **409** if the agent is the workspace's DM default and other agents are still bound (#584) +- `PUT /api/agents/{name}/slack/channel/dm-default` → `{status, team_id, workspace_name, previous, new_default}` — owner-only; single-tx clear-then-set on `is_dm_default`; audit-logged via `AGENT_LIFECYCLE/slack_dm_default_changed` (#584) ## Backend Layer @@ -110,6 +111,8 @@ Priority in `SlackAdapter.get_agent_name()`: | GET | `/api/agents/{name}/public-links/{id}/slack` | `routers/slack.py` | Connection status | | DELETE | `/api/agents/{name}/public-links/{id}/slack` | `routers/slack.py` | Disconnect | | PUT | `/api/agents/{name}/public-links/{id}/slack` | `routers/slack.py` | Update settings (enable/disable) | +| PUT | `/api/agents/{name}/slack/channel/dm-default` | `routers/slack.py` | Make this agent the DM-default for its workspace (#584) | +| DELETE | `/api/agents/{name}/slack/channel` | `routers/slack.py` | Unbind — refuses with 409 if agent is the DM default and others are bound (#584) | ### Business Logic diff --git a/src/backend/database.py b/src/backend/database.py index e5bac9ec7..2b0032355 100644 --- a/src/backend/database.py +++ b/src/backend/database.py @@ -1536,6 +1536,9 @@ def get_slack_agent_name_for_channel(self, team_id, slack_channel_id): def get_slack_dm_default_agent(self, team_id): return self._slack_channel_ops.get_dm_default_agent(team_id) + def set_slack_dm_default(self, team_id, agent_name): + return self._slack_channel_ops.set_dm_default(team_id, agent_name) + def get_slack_agents_for_workspace(self, team_id): return self._slack_channel_ops.get_agents_for_workspace(team_id) diff --git a/src/backend/db/slack_channels.py b/src/backend/db/slack_channels.py index 4779ecad2..f962d6dbb 100644 --- a/src/backend/db/slack_channels.py +++ b/src/backend/db/slack_channels.py @@ -197,6 +197,37 @@ def get_dm_default_agent(self, team_id: str) -> Optional[str]: row = cursor.fetchone() return row[0] if row else None + def set_dm_default(self, team_id: str, agent_name: str) -> bool: + """Make ``agent_name`` the DM-default for the workspace. + + Single transaction: clear all existing flags, then set on the target. + Avoids any window where two agents would both look like the default + (the schema has no exclusivity constraint, so the read-side falls + back to ``LIMIT 1`` and would pick non-deterministically). + + Returns True if the target row was updated, False if the agent is + not bound in this workspace (caller should 404). + """ + with get_db_connection() as conn: + cursor = conn.cursor() + cursor.execute("BEGIN") + try: + cursor.execute( + "UPDATE slack_channel_agents SET is_dm_default = 0 WHERE team_id = ?", + (team_id,), + ) + cursor.execute( + """UPDATE slack_channel_agents SET is_dm_default = 1 + WHERE team_id = ? AND agent_name = ?""", + (team_id, agent_name), + ) + changed = cursor.rowcount > 0 + conn.commit() + except Exception: + conn.rollback() + raise + return changed + def get_agents_for_workspace(self, team_id: str) -> List[dict]: """Get all agent-channel bindings for a workspace.""" with get_db_connection() as conn: @@ -229,7 +260,14 @@ def get_channel_for_agent(self, team_id: str, agent_name: str) -> Optional[dict] return self._row_to_channel_agent(row) def unbind_agent(self, team_id: str, agent_name: str) -> bool: - """Remove an agent's channel binding.""" + """Remove an agent's channel binding. + + Pure delete — does not auto-promote a new DM default. The router + layer is responsible for refusing to unbind the current DM default + while other agents are still bound (#584). When the unbind target + is the only agent in the workspace, the binding is removed cleanly + and the workspace ends up with no Slack agents at all. + """ with get_db_connection() as conn: cursor = conn.cursor() cursor.execute(""" diff --git a/src/backend/routers/slack.py b/src/backend/routers/slack.py index 5eeed22cc..4132c8b78 100644 --- a/src/backend/routers/slack.py +++ b/src/backend/routers/slack.py @@ -405,6 +405,10 @@ async def get_agent_slack_channel( for ws in workspaces: binding = db.get_slack_channel_for_agent(ws["team_id"], name) if binding: + # Count agents in the workspace so the UI can decide whether + # to allow unbinding — the DM-default agent cannot be unbound + # while other agents are still bound (#584). + workspace_agents = db.get_slack_agents_for_workspace(ws["team_id"]) return { "bound": True, "channel_name": binding["slack_channel_name"], @@ -412,6 +416,7 @@ async def get_agent_slack_channel( "workspace_team_id": ws["team_id"], "workspace_name": ws["team_name"], "is_dm_default": binding.get("is_dm_default", False), + "workspace_agent_count": len(workspace_agents), "created_at": binding.get("created_at"), } @@ -490,14 +495,121 @@ async def delete_agent_slack_channel( name: str, current_user: User = Depends(get_current_user) ): - """Unbind an agent from its Slack channel.""" + """Unbind an agent from its Slack channel. + + Refuses to unbind the workspace's current DM-default agent while any + other agents are still bound (#584). The owner must promote a different + agent first via ``PUT /api/agents/{name}/slack/channel/dm-default``. + When the agent is the only one bound, unbind is allowed — the workspace + ends up with no Slack agents, which is a clean cascade. + """ if not db.can_user_share_agent(current_user.username, name): raise HTTPException(status_code=403, detail="Only owners can manage Slack channels") workspaces = db.get_all_slack_workspaces() for ws in workspaces: + binding = db.get_slack_channel_for_agent(ws["team_id"], name) + if not binding: + continue + + # Refuse to drop the DM default while siblings remain. + if binding.get("is_dm_default"): + workspace_agents = db.get_slack_agents_for_workspace(ws["team_id"]) + if len(workspace_agents) > 1: + raise HTTPException( + status_code=409, + detail=( + "Cannot unbind the DM-default agent while other agents " + "are bound to this workspace. Set another agent as DM " + "default first (PUT /api/agents/{other}/slack/channel/" + "dm-default) and try again." + ), + ) + if db.unbind_slack_agent(ws["team_id"], name): logger.info(f"Agent {name} unbound from Slack in workspace {ws['team_name']}") return {"unbound": True, "workspace_name": ws["team_name"]} raise HTTPException(status_code=404, detail="Agent is not bound to any Slack channel") + + +@auth_router.put("/api/agents/{name}/slack/channel/dm-default") +async def set_agent_as_slack_dm_default( + name: str, + current_user: User = Depends(get_current_user) +): + """Make this agent the DM-default for its Slack workspace. + + DMs to the bot (no channel context, no @mention) route to whichever + agent in the workspace is flagged ``is_dm_default=1``. Until #584 the + flag was only auto-set for the first agent ever connected and had no + setter, so workspaces with multiple agents were stuck. This endpoint + flips it; ``unbind`` auto-promotes the oldest remaining agent so the + workspace is never left with zero defaults. + """ + if not db.can_user_share_agent(current_user.username, name): + raise HTTPException(status_code=403, detail="Only owners can manage Slack channels") + + # Find the workspace where this agent is bound. There should be at + # most one — agents are bound 1:1 per workspace today. + workspace = None + for ws in db.get_all_slack_workspaces(): + if db.get_slack_channel_for_agent(ws["team_id"], name): + workspace = ws + break + + if not workspace: + raise HTTPException( + status_code=404, + detail="Agent is not bound to any Slack channel", + ) + + team_id = workspace["team_id"] + previous = db.get_slack_dm_default_agent(team_id) + if previous == name: + # Idempotent — already the default. + return { + "status": "unchanged", + "team_id": team_id, + "workspace_name": workspace.get("team_name"), + "previous": previous, + "new_default": name, + } + + if not db.set_slack_dm_default(team_id, name): + # set_dm_default returns False only if the agent isn't bound — we + # already verified that above, so this is a real "row vanished" + # race. Surface as 404. + raise HTTPException(status_code=404, detail="Agent binding not found") + + logger.info( + "Slack DM default for workspace %s changed: %s → %s (by %s)", + workspace.get("team_name"), previous, name, current_user.username, + ) + + # Audit + try: + await platform_audit_service.log( + event_type=AuditEventType.AGENT_LIFECYCLE, + event_action="slack_dm_default_changed", + source="api", + actor_user=current_user, + target_type="agent", + target_id=name, + details={ + "team_id": team_id, + "workspace_name": workspace.get("team_name"), + "previous": previous, + "new_default": name, + }, + ) + except Exception as e: # pragma: no cover + logger.warning("Failed to audit slack_dm_default_changed: %s", e) + + return { + "status": "updated", + "team_id": team_id, + "workspace_name": workspace.get("team_name"), + "previous": previous, + "new_default": name, + } diff --git a/src/frontend/src/components/SlackChannelPanel.vue b/src/frontend/src/components/SlackChannelPanel.vue index bfb6c9c36..2b315d7a3 100644 --- a/src/frontend/src/components/SlackChannelPanel.vue +++ b/src/frontend/src/components/SlackChannelPanel.vue @@ -30,17 +30,37 @@

{{ channel.workspace_name }} - (DM default)

- +
+ + + + DM default + + + +
@@ -70,7 +90,7 @@ diff --git a/tests/registry.json b/tests/registry.json index 943a4d0d8..3f3b5622f 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -203,6 +203,13 @@ "categories": ["backend", "unit", "lifecycle", "file-sharing"], "description": "check_public_folder_mount_matches truth table: enabled+mounted → True, enabled+unmounted → False (needs recreation to attach), disabled+mounted → False (needs recreation to detach), disabled+unmounted → True. Adversarial cases: similar paths (/public-backup, /public/inner) don't match, missing 'Mounts' key handled, flag re-read each call, other mounts (shared-out, shared-in/*, workspace) don't interfere (9 tests)" }, + { + "file": "unit/test_slack_dm_default.py", + "feature": "Slack DM-default agent setter (#584)", + "added": "2026-04-29", + "categories": ["backend", "unit", "db", "slack"], + "description": "set_dm_default + unbind_agent contract: setter is single-tx clear-then-set, idempotent, exclusive (exactly one default per workspace), per-workspace isolation, returns False when agent not bound. Unbind is pure delete (does NOT auto-promote — router enforces the guard), works on non-default and last-agent paths, unknown agent returns False (10 tests)" + }, { "file": "test_public_chat_history.py", "feature": "Issue #587", diff --git a/tests/unit/test_slack_dm_default.py b/tests/unit/test_slack_dm_default.py new file mode 100644 index 000000000..ec520661f --- /dev/null +++ b/tests/unit/test_slack_dm_default.py @@ -0,0 +1,188 @@ +""" +Unit tests for Slack DM-default management (#584). + +Covers: +- ``set_dm_default`` — single-tx clear-then-set, exclusivity, idempotency +- ``unbind_agent`` — must NOT auto-promote (router enforces the guard) +- The "blocked unbind" rule itself is a router-layer concern; we exercise + it via a thin direct-call shim around the router handler. + +Run in-process against an ephemeral SQLite database (no backend, no Docker). +""" + +from __future__ import annotations + +import importlib.util +import sqlite3 +import sys +from pathlib import Path + +import pytest + + +_THIS = Path(__file__).resolve() +_BACKEND = _THIS.parent.parent.parent / "src" / "backend" +_BACKEND_STR = str(_BACKEND) +for _shadow in ("utils", "utils.api_client", "utils.assertions", "utils.cleanup"): + sys.modules.pop(_shadow, None) +while _BACKEND_STR in sys.path: + sys.path.remove(_BACKEND_STR) +sys.path.insert(0, _BACKEND_STR) + + +def _load_module(rel_path: str, name: str): + path = _BACKEND / rel_path + spec = importlib.util.spec_from_file_location(name, path) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +_schema_mod = _load_module("db/schema.py", "_schema_slack") +_migrations_mod = _load_module("db/migrations.py", "_migrations_slack") +init_schema = _schema_mod.init_schema +run_all_migrations = _migrations_mod.run_all_migrations + + +pytestmark = pytest.mark.unit + + +@pytest.fixture +def tmp_db(tmp_path, monkeypatch): + """Throwaway DB with full schema + migrations applied.""" + db_path = tmp_path / "trinity.db" + monkeypatch.setenv("TRINITY_DB_PATH", str(db_path)) + + conn = sqlite3.connect(str(db_path)) + conn.row_factory = sqlite3.Row + cursor = conn.cursor() + init_schema(cursor, conn) + run_all_migrations(cursor, conn) + conn.commit() + conn.close() + + # Drop cached modules so production code picks up the new path. + for modname in list(sys.modules): + if modname == "database" or modname.startswith("db."): + sys.modules.pop(modname, None) + + yield db_path + + +@pytest.fixture +def slack_ops(tmp_db): + """Fresh SlackChannelOperations bound to the tmp DB.""" + from db.slack_channels import SlackChannelOperations + return SlackChannelOperations() + + +def _bind(slack_ops, team_id, agent_name, *, is_dm_default=False, channel_id=None): + """Helper: bind an agent to a workspace channel.""" + return slack_ops.bind_channel_to_agent( + team_id=team_id, + slack_channel_id=channel_id or f"C-{agent_name}", + slack_channel_name=agent_name, + agent_name=agent_name, + is_dm_default=is_dm_default, + ) + + +# --------------------------------------------------------------------------- +# set_dm_default +# --------------------------------------------------------------------------- + + +class TestSetDmDefault: + + def test_returns_false_when_agent_not_bound(self, slack_ops): + """No row to flip → setter returns False so the router can 404.""" + assert slack_ops.set_dm_default("T-x", "ghost-agent") is False + + def test_sets_default_on_bound_agent(self, slack_ops): + _bind(slack_ops, "T-1", "alpha") + assert slack_ops.set_dm_default("T-1", "alpha") is True + assert slack_ops.get_dm_default_agent("T-1") == "alpha" + + def test_clears_previous_default(self, slack_ops): + _bind(slack_ops, "T-1", "alpha", is_dm_default=True) + _bind(slack_ops, "T-1", "beta") + assert slack_ops.get_dm_default_agent("T-1") == "alpha" + + slack_ops.set_dm_default("T-1", "beta") + # Exactly one default after the flip + assert slack_ops.get_dm_default_agent("T-1") == "beta" + agents = slack_ops.get_agents_for_workspace("T-1") + assert sum(1 for a in agents if a["is_dm_default"]) == 1 + + def test_idempotent_when_already_default(self, slack_ops): + _bind(slack_ops, "T-1", "alpha", is_dm_default=True) + slack_ops.set_dm_default("T-1", "alpha") + slack_ops.set_dm_default("T-1", "alpha") + agents = slack_ops.get_agents_for_workspace("T-1") + assert sum(1 for a in agents if a["is_dm_default"]) == 1 + assert slack_ops.get_dm_default_agent("T-1") == "alpha" + + def test_isolated_per_workspace(self, slack_ops): + """Setting default in workspace A must not touch workspace B.""" + _bind(slack_ops, "T-1", "alpha", is_dm_default=True) + _bind(slack_ops, "T-2", "alpha") # different workspace, same name + slack_ops.set_dm_default("T-2", "alpha") + + # Both workspaces have alpha as default — no cross-talk. + assert slack_ops.get_dm_default_agent("T-1") == "alpha" + assert slack_ops.get_dm_default_agent("T-2") == "alpha" + + def test_setting_one_does_not_touch_siblings(self, slack_ops): + _bind(slack_ops, "T-1", "alpha", is_dm_default=True) + _bind(slack_ops, "T-1", "beta") + _bind(slack_ops, "T-1", "gamma") + + slack_ops.set_dm_default("T-1", "gamma") + + agents = {a["agent_name"]: a["is_dm_default"] + for a in slack_ops.get_agents_for_workspace("T-1")} + assert agents == {"alpha": False, "beta": False, "gamma": True} + + +# --------------------------------------------------------------------------- +# unbind_agent — pure delete, no auto-promote +# --------------------------------------------------------------------------- + + +class TestUnbindAgent: + + def test_unbind_non_default_agent(self, slack_ops): + _bind(slack_ops, "T-1", "alpha", is_dm_default=True) + _bind(slack_ops, "T-1", "beta") + + assert slack_ops.unbind_agent("T-1", "beta") is True + # Default is unchanged + assert slack_ops.get_dm_default_agent("T-1") == "alpha" + + def test_unbind_default_agent_does_not_promote(self, slack_ops): + """Per #584 the DB layer is a pure delete — auto-promote was + rejected in favour of a router-layer guard. This test pins the + contract so any future drift is loud.""" + _bind(slack_ops, "T-1", "alpha", is_dm_default=True) + _bind(slack_ops, "T-1", "beta") + + slack_ops.unbind_agent("T-1", "alpha") + + # Beta is NOT promoted automatically. + assert slack_ops.get_dm_default_agent("T-1") is None + agents = slack_ops.get_agents_for_workspace("T-1") + assert len(agents) == 1 + assert agents[0]["agent_name"] == "beta" + assert agents[0]["is_dm_default"] is False + + def test_unbind_only_agent_clears_workspace(self, slack_ops): + _bind(slack_ops, "T-1", "alpha", is_dm_default=True) + assert slack_ops.unbind_agent("T-1", "alpha") is True + assert slack_ops.get_agents_for_workspace("T-1") == [] + assert slack_ops.get_dm_default_agent("T-1") is None + + def test_unbind_unknown_agent_returns_false(self, slack_ops): + _bind(slack_ops, "T-1", "alpha", is_dm_default=True) + assert slack_ops.unbind_agent("T-1", "ghost") is False + # Default unchanged + assert slack_ops.get_dm_default_agent("T-1") == "alpha"