Merge pull request #2526 from glifocat/fix/2525-groups-delete-cascade

fix(cli): cascade dependent rows on groups delete (#2525)

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<string, number> } }).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<string, number> } }).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);
+  });
+});

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 <group-id>. Out of scope: killing running containers, on-disk cleanup of groups/<folder>/ and data/v2-sessions/<group-id>/.',
+      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: