diff --git a/src/cli/resources/groups.test.ts b/src/cli/resources/groups.test.ts new file mode 100644 index 000000000..bb5f08af8 --- /dev/null +++ b/src/cli/resources/groups.test.ts @@ -0,0 +1,220 @@ +/** + * Regression test for #2525 — `ncl groups delete` must cascade dependent + * rows in FK order so the final `DELETE FROM agent_groups` succeeds even + * when the group has sessions, destinations, approvals, role grants, etc. + * + * The bug pre-fix: the generic single-table DELETE handler ran a bare + * `DELETE FROM agent_groups WHERE id = ?` which always failed with a + * `SQLITE_CONSTRAINT_FOREIGNKEY` when anything pointed at the group. + * + * The approval handler in `dispatch.ts` re-enters `dispatch()` with + * `caller: 'host'` after admin approval, so the test invokes dispatch + * with the host caller — same code path a real approval would take. + */ +import fs from 'fs'; +import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest'; + +vi.mock('../../container-runner.js', () => ({ + wakeContainer: vi.fn().mockResolvedValue(undefined), + isContainerRunning: vi.fn().mockReturnValue(false), + getActiveContainerCount: vi.fn().mockReturnValue(0), + killContainer: vi.fn(), + buildAgentGroupImage: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock('../../config.js', async () => { + const actual = await vi.importActual('../../config.js'); + return { ...actual, DATA_DIR: '/tmp/nanoclaw-test-cli-groups' }; +}); + +const TEST_DIR = '/tmp/nanoclaw-test-cli-groups'; + +import { initTestDb, closeDb, runMigrations, createAgentGroup, getDb } from '../../db/index.js'; +import { createSession } from '../../db/sessions.js'; +import { dispatch } from '../dispatch.js'; +// Side-effect import: registers the `groups-*` commands (including delete). +import './groups.js'; + +function now(): string { + return new Date().toISOString(); +} + +function count(sql: string, ...params: unknown[]): number { + return ( + getDb() + .prepare(sql) + .get(...params) as { c: number } + ).c; +} + +describe('groups CLI delete cascades dependent rows (#2525)', () => { + beforeEach(() => { + if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true }); + fs.mkdirSync(TEST_DIR, { recursive: true }); + + const db = initTestDb(); + runMigrations(db); + }); + + afterEach(() => { + closeDb(); + if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true }); + }); + + it('deletes a group with sessions, destinations, approvals, members, roles, and wirings', async () => { + const GID = 'ag-victim'; + const SID = 'sess-victim-1'; + const MGID = 'mg-1'; + const UID = 'tg:42'; + + createAgentGroup({ id: GID, name: 'victim', folder: 'victim', agent_provider: null, created_at: now() }); + createSession({ + id: SID, + agent_group_id: GID, + messaging_group_id: null, + thread_id: null, + agent_provider: null, + status: 'active', + container_status: 'stopped', + last_active: null, + created_at: now(), + }); + + const db = getDb(); + + // Direct inserts for the dependent tables. Keeps the fixture minimal — + // we only need rows that establish FK relationships, not full domain + // entities. + db.prepare(`INSERT INTO users (id, kind, display_name, created_at) VALUES (?, 'telegram', 'someone', ?)`).run( + UID, + now(), + ); + db.prepare( + `INSERT INTO messaging_groups (id, channel_type, platform_id, name, is_group, unknown_sender_policy, created_at) + VALUES (?, 'telegram', 'tg-1', 'chat', 1, 'strict', ?)`, + ).run(MGID, now()); + + db.prepare( + `INSERT INTO agent_destinations (agent_group_id, local_name, target_type, target_id, created_at) + VALUES (?, 'chan', 'channel', ?, ?)`, + ).run(GID, MGID, now()); + + db.prepare( + `INSERT INTO pending_questions (question_id, session_id, message_out_id, title, options_json, created_at) + VALUES (?, ?, 'mout-1', 'q', '[]', ?)`, + ).run('q-1', SID, now()); + + db.prepare( + `INSERT INTO pending_approvals (approval_id, session_id, request_id, action, payload, created_at, agent_group_id, status, title, options_json) + VALUES (?, ?, 'req-1', 'cli_command', '{}', ?, ?, 'pending', '', '[]')`, + ).run('pa-1', SID, now(), GID); + + db.prepare( + `INSERT INTO pending_sender_approvals (id, messaging_group_id, agent_group_id, sender_identity, sender_name, original_message, approver_user_id, created_at) + VALUES ('psa-1', ?, ?, 'tg:99', 'them', '{}', ?, ?)`, + ).run(MGID, GID, UID, now()); + + db.prepare( + `INSERT INTO pending_channel_approvals (messaging_group_id, agent_group_id, original_message, approver_user_id, created_at) + VALUES (?, ?, '{}', ?, ?)`, + ).run(MGID, GID, UID, now()); + + db.prepare( + `INSERT INTO messaging_group_agents (id, messaging_group_id, agent_group_id, engage_mode, sender_scope, ignored_message_policy, session_mode, priority, created_at) + VALUES ('mga-1', ?, ?, 'mention', 'all', 'drop', 'shared', 0, ?)`, + ).run(MGID, GID, now()); + + db.prepare( + `INSERT INTO agent_group_members (user_id, agent_group_id, added_by, added_at) VALUES (?, ?, NULL, ?)`, + ).run(UID, GID, now()); + + db.prepare( + `INSERT INTO user_roles (user_id, role, agent_group_id, granted_by, granted_at) VALUES (?, 'admin', ?, NULL, ?)`, + ).run(UID, GID, now()); + + // Container config row exercises the ON DELETE CASCADE on container_configs. + db.prepare( + `INSERT INTO container_configs + (agent_group_id, provider, model, effort, image_tag, assistant_name, max_messages_per_prompt, + skills, mcp_servers, packages_apt, packages_npm, additional_mounts, cli_scope, updated_at) + VALUES (?, NULL, NULL, NULL, NULL, NULL, NULL, '"all"', '{}', '[]', '[]', '[]', 'group', ?)`, + ).run(GID, now()); + + const resp = await dispatch({ id: 'req-del', command: 'groups-delete', args: { id: GID } }, { caller: 'host' }); + + expect(resp.ok).toBe(true); + const data = (resp as { ok: true; data: { deleted: string; removed: Record } }).data; + expect(data.deleted).toBe(GID); + expect(data.removed).toMatchObject({ + sessions: 1, + pending_questions: 1, + pending_approvals: 1, + agent_destinations_owned: 1, + agent_destinations_pointing: 0, + pending_sender_approvals: 1, + pending_channel_approvals: 1, + messaging_group_agents: 1, + agent_group_members: 1, + user_roles: 1, + container_configs: 1, + }); + + // The group and every dependent row must be gone. + expect(count('SELECT COUNT(*) AS c FROM agent_groups WHERE id = ?', GID)).toBe(0); + expect(count('SELECT COUNT(*) AS c FROM sessions WHERE agent_group_id = ?', GID)).toBe(0); + expect(count('SELECT COUNT(*) AS c FROM pending_questions WHERE session_id = ?', SID)).toBe(0); + expect( + count('SELECT COUNT(*) AS c FROM pending_approvals WHERE agent_group_id = ? OR session_id = ?', GID, SID), + ).toBe(0); + expect(count('SELECT COUNT(*) AS c FROM agent_destinations WHERE agent_group_id = ?', GID)).toBe(0); + expect(count('SELECT COUNT(*) AS c FROM pending_sender_approvals WHERE agent_group_id = ?', GID)).toBe(0); + expect(count('SELECT COUNT(*) AS c FROM pending_channel_approvals WHERE agent_group_id = ?', GID)).toBe(0); + expect(count('SELECT COUNT(*) AS c FROM messaging_group_agents WHERE agent_group_id = ?', GID)).toBe(0); + expect(count('SELECT COUNT(*) AS c FROM agent_group_members WHERE agent_group_id = ?', GID)).toBe(0); + expect(count('SELECT COUNT(*) AS c FROM user_roles WHERE agent_group_id = ?', GID)).toBe(0); + expect(count('SELECT COUNT(*) AS c FROM container_configs WHERE agent_group_id = ?', GID)).toBe(0); + + // Unrelated tables untouched. + expect(count('SELECT COUNT(*) AS c FROM users WHERE id = ?', UID)).toBe(1); + expect(count('SELECT COUNT(*) AS c FROM messaging_groups WHERE id = ?', MGID)).toBe(1); + }); + + it('removes polymorphic agent_destinations that point at the deleted group', async () => { + const A = 'ag-a'; + const B = 'ag-b'; + createAgentGroup({ id: A, name: 'a', folder: 'a', agent_provider: null, created_at: now() }); + createAgentGroup({ id: B, name: 'b', folder: 'b', agent_provider: null, created_at: now() }); + + const db = getDb(); + + // B has a destination pointing at A. target_id is polymorphic — no FK + // constraint enforces it, so without explicit cleanup the row would + // dangle after A is deleted. + db.prepare( + `INSERT INTO agent_destinations (agent_group_id, local_name, target_type, target_id, created_at) + VALUES (?, 'sibling', 'agent', ?, ?)`, + ).run(B, A, now()); + + const resp = await dispatch({ id: 'req-del-a', command: 'groups-delete', args: { id: A } }, { caller: 'host' }); + + expect(resp.ok).toBe(true); + const data = (resp as { ok: true; data: { removed: Record } }).data; + expect(data.removed.agent_destinations_pointing).toBe(1); + + // A is gone, B remains, and B's stale destination is cleaned up. + expect(count('SELECT COUNT(*) AS c FROM agent_groups WHERE id = ?', A)).toBe(0); + expect(count('SELECT COUNT(*) AS c FROM agent_groups WHERE id = ?', B)).toBe(1); + expect(count('SELECT COUNT(*) AS c FROM agent_destinations WHERE agent_group_id = ?', B)).toBe(0); + }); + + it('returns a handler error for an unknown group id', async () => { + const resp = await dispatch( + { id: 'req-missing', command: 'groups-delete', args: { id: 'ag-does-not-exist' } }, + { caller: 'host' }, + ); + + expect(resp.ok).toBe(false); + expect((resp as { ok: false; error: { code: string; message: string } }).error.code).toBe('handler-error'); + expect((resp as { ok: false; error: { code: string; message: string } }).error.message).toMatch(/not found/i); + }); +}); diff --git a/src/cli/resources/groups.ts b/src/cli/resources/groups.ts index 031ccb5ca..e3765c0fb 100644 --- a/src/cli/resources/groups.ts +++ b/src/cli/resources/groups.ts @@ -1,6 +1,7 @@ import type { McpServerConfig } from '../../container-config.js'; import { buildAgentGroupImage, killContainer, wakeContainer } from '../../container-runner.js'; import { restartAgentGroupContainers } from '../../container-restart.js'; +import { getDb, hasTable } from '../../db/connection.js'; import { getSession } from '../../db/sessions.js'; import { writeSessionMessage } from '../../session-manager.js'; import { @@ -57,8 +58,97 @@ registerResource({ }, { name: 'created_at', type: 'string', description: 'Auto-set.', generated: true }, ], - operations: { list: 'open', get: 'open', create: 'approval', update: 'approval', delete: 'approval' }, + // `delete` is intentionally not in `operations` — the generic single-table + // DELETE violates FK constraints (see #2525). The cascading handler is + // provided as `customOperations.delete` below. + operations: { list: 'open', get: 'open', create: 'approval', update: 'approval' }, customOperations: { + delete: { + access: 'approval', + description: + 'Delete an agent group and its dependent rows (sessions, destinations, approvals, role grants, ' + + 'memberships, channel wirings). FK-ordered cascade in a single transaction. ' + + 'Use --id . Out of scope: killing running containers, on-disk cleanup of groups// and data/v2-sessions//.', + handler: async (args) => { + const id = args.id as string; + if (!id) throw new Error('--id is required'); + const db = getDb(); + + // Verify the group exists before doing anything — preserves the + // genericDelete behaviour of throwing "not found" for unknown IDs. + const exists = db.prepare('SELECT 1 FROM agent_groups WHERE id = ? LIMIT 1').get(id); + if (!exists) throw new Error(`group not found: ${id}`); + + const hasAgentDestinations = hasTable(db, 'agent_destinations'); + const hasPendingApprovals = hasTable(db, 'pending_approvals'); + + // FK-ordered cascade. Single sync transaction — better-sqlite3 rolls + // back the whole thing if any statement throws (e.g. an FK constraint + // we missed), so the central DB stays consistent. The `removed` counts + // are sourced from each DELETE's `changes` so they describe exactly + // what the transaction did, not a separate pre-flight snapshot. + const cascade = db.transaction((groupId: string) => { + const counts = { + sessions: 0, + pending_questions: 0, + pending_approvals: 0, + agent_destinations_owned: 0, + agent_destinations_pointing: 0, + pending_sender_approvals: 0, + pending_channel_approvals: 0, + messaging_group_agents: 0, + agent_group_members: 0, + user_roles: 0, + container_configs: 0, + }; + + if (hasAgentDestinations) { + counts.agent_destinations_owned = db + .prepare('DELETE FROM agent_destinations WHERE agent_group_id = ?') + .run(groupId).changes; + counts.agent_destinations_pointing = db + .prepare('DELETE FROM agent_destinations WHERE target_type = ? AND target_id = ?') + .run('agent', groupId).changes; + } + counts.pending_questions = db + .prepare( + 'DELETE FROM pending_questions WHERE session_id IN (SELECT id FROM sessions WHERE agent_group_id = ?)', + ) + .run(groupId).changes; + if (hasPendingApprovals) { + counts.pending_approvals = db + .prepare( + 'DELETE FROM pending_approvals WHERE agent_group_id = ? OR session_id IN (SELECT id FROM sessions WHERE agent_group_id = ?)', + ) + .run(groupId, groupId).changes; + } + counts.sessions = db.prepare('DELETE FROM sessions WHERE agent_group_id = ?').run(groupId).changes; + counts.pending_sender_approvals = db + .prepare('DELETE FROM pending_sender_approvals WHERE agent_group_id = ?') + .run(groupId).changes; + counts.pending_channel_approvals = db + .prepare('DELETE FROM pending_channel_approvals WHERE agent_group_id = ?') + .run(groupId).changes; + counts.messaging_group_agents = db + .prepare('DELETE FROM messaging_group_agents WHERE agent_group_id = ?') + .run(groupId).changes; + counts.agent_group_members = db + .prepare('DELETE FROM agent_group_members WHERE agent_group_id = ?') + .run(groupId).changes; + counts.user_roles = db.prepare('DELETE FROM user_roles WHERE agent_group_id = ?').run(groupId).changes; + // migration-014 has ON DELETE CASCADE on container_configs.agent_group_id; + // the explicit delete here mirrors the other tables and surfaces the count. + counts.container_configs = db + .prepare('DELETE FROM container_configs WHERE agent_group_id = ?') + .run(groupId).changes; + db.prepare('DELETE FROM agent_groups WHERE id = ?').run(groupId); + return counts; + }); + const removed = cascade(id); + + return { deleted: id, removed }; + }, + }, restart: { access: 'approval', description: