From 1985fd6a5e55739b8ed0838b3f7e24a09067acdb Mon Sep 17 00:00:00 2001 From: glifocat Date: Sat, 4 Jul 2026 01:39:46 +0000 Subject: [PATCH] fix(cli): live-refresh destinations on `ncl wirings create` Closes the restart-required behavior: wirings-create's postCreate wrote the central agent_destinations row but never projected it into running sessions' inbound.db, so an agent with a live container dropped every reply to the newly-wired chat as "unknown destination" until a group restart (observed on a live instance). `ncl destinations add` already projects for exactly this reason; wirings create now has parity. postCreate is sync and runs inside the central-DB transaction, so it's the wrong place for an async, cross-DB (session inbound.db) side effect. - crud.ts: add a postCommit hook that runs after the create transaction commits; correct the "pure DB writes" comment. - resources/destinations.ts: export projectDestinationsToSessions so wirings.ts reuses the exact same projection as destinations add/remove. - resources/wirings.ts: project the new destination to live sessions in postCommit. - crud.test.ts: cover projection with and without a running session. --- src/cli/crud.test.ts | 79 +++++++++++++++++++++++++++++++ src/cli/crud.ts | 23 ++++++++- src/cli/resources/destinations.ts | 10 ++-- src/cli/resources/wirings.ts | 13 +++++ 4 files changed, 119 insertions(+), 6 deletions(-) diff --git a/src/cli/crud.test.ts b/src/cli/crud.test.ts index fb3d7b387..a700edd4d 100644 --- a/src/cli/crud.test.ts +++ b/src/cli/crud.test.ts @@ -20,7 +20,17 @@ vi.mock('../log.js', () => ({ log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, })); +// `wirings.ts`'s postCommit projects the new destination into every running +// session's `inbound.db` via the agent-to-agent module's `writeDestinations`. +// That helper opens on-disk session DB files we don't have in a unit test, so +// mock it and observe that the projection is invoked per live session. +const writeDestinationsSpy = vi.fn(); +vi.mock('../modules/agent-to-agent/write-destinations.js', () => ({ + writeDestinations: (...args: unknown[]) => writeDestinationsSpy(...args), +})); + import { initTestDb, closeDb, runMigrations, createAgentGroup, createMessagingGroup } from '../db/index.js'; +import { createSession } from '../db/sessions.js'; import { getContainerConfig } from '../db/container-configs.js'; import { getDestinations } from '../modules/agent-to-agent/db/agent-destinations.js'; import { lookup } from './registry.js'; @@ -37,6 +47,7 @@ beforeEach(() => { const db = initTestDb(); runMigrations(db); ensureContainerConfigSpy.mockClear(); + writeDestinationsSpy.mockClear(); }); afterEach(() => { @@ -95,3 +106,71 @@ describe('genericCreate postCreate hook', () => { expect(destinations[0].target_id).toBe('mg-1'); }); }); + +describe('genericCreate postCommit hook', () => { + it('wirings-create projects the new destination into live sessions', async () => { + createAgentGroup({ + id: 'ag-1', + name: 'Agent One', + folder: 'agent-one', + agent_provider: null, + created_at: new Date().toISOString(), + }); + createMessagingGroup({ + id: 'mg-1', + channel_type: 'discord', + platform_id: 'channel-123', + name: 'general', + is_group: 1, + unknown_sender_policy: 'strict', + created_at: new Date().toISOString(), + }); + // A container is already running for this agent — its session inbound.db + // holds a stale destination projection. + createSession({ + id: 'sess-1', + agent_group_id: 'ag-1', + messaging_group_id: null, + thread_id: null, + agent_provider: null, + status: 'active', + container_status: 'running', + last_active: new Date().toISOString(), + created_at: new Date().toISOString(), + }); + + const cmd = lookup('wirings-create'); + await cmd!.handler({ messaging_group_id: 'mg-1', agent_group_id: 'ag-1' }, hostCtx); + + // Live-refresh parity with `ncl destinations add`: without postCommit the + // running container keeps serving the stale projection and drops replies + // to this chat as "unknown destination" until a restart — issue #2389. + expect(writeDestinationsSpy).toHaveBeenCalledWith('ag-1', 'sess-1'); + }); + + it('wirings-create projection is a no-op when no sessions are running', async () => { + createAgentGroup({ + id: 'ag-2', + name: 'Agent Two', + folder: 'agent-two', + agent_provider: null, + created_at: new Date().toISOString(), + }); + createMessagingGroup({ + id: 'mg-2', + channel_type: 'discord', + platform_id: 'channel-456', + name: 'ops', + is_group: 1, + unknown_sender_policy: 'strict', + created_at: new Date().toISOString(), + }); + + const cmd = lookup('wirings-create'); + await cmd!.handler({ messaging_group_id: 'mg-2', agent_group_id: 'ag-2' }, hostCtx); + + // No live session for ag-2 → nothing to project, and the central + // destination row was still written (covered above). + expect(writeDestinationsSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/src/cli/crud.ts b/src/cli/crud.ts index b96da9e95..3b050285c 100644 --- a/src/cli/crud.ts +++ b/src/cli/crud.ts @@ -80,6 +80,22 @@ export interface ResourceDef { * inserted, so generated fields like `id` and `created_at` are populated. */ postCreate?: (row: Record) => void; + /** + * Runs AFTER the create transaction has committed, with the row that was + * written. Use this — not `postCreate` — for side effects that live + * OUTSIDE the central DB (filesystem writes, projecting rows into a + * running agent's session `inbound.db`) or that are async: those must not + * sit inside the better-sqlite3 transaction, which only covers central-DB + * statements and is synchronous. + * + * The canonical case is live-refresh parity with `ncl destinations add`: + * after `ncl wirings create` writes the companion `agent_destinations` + * row, the change has to be projected into any running container's session + * DB or the agent won't see the new delivery target until its next spawn + * (#2389). Runs only if the transaction succeeds, so it never observes a + * rolled-back row. + */ + postCommit?: (row: Record) => void | Promise; } // --------------------------------------------------------------------------- @@ -170,13 +186,16 @@ function genericCreate(def: ResourceDef) { const placeholders = colNames.map((c) => `@${c}`); // Single transaction so a postCreate throw rolls back the parent INSERT — // closes the partial-state class this PR exists to fix (#2415, #2389). - // better-sqlite3 .transaction() is sync, which matches postCreate's - // signature (`(row) => void`); current hooks are pure DB writes. + // better-sqlite3 .transaction() is sync, so `postCreate` is sync too and + // must only touch the central DB (it's the atomic companion-row write). + // Anything async or outside the central DB — filesystem, session-DB + // projection — belongs in `postCommit`, which runs after commit below. const db = getDb(); db.transaction(() => { db.prepare(`INSERT INTO ${def.table} (${colNames.join(', ')}) VALUES (${placeholders.join(', ')})`).run(values); if (def.postCreate) def.postCreate(values); })(); + if (def.postCommit) await def.postCommit(values); return values; }; } diff --git a/src/cli/resources/destinations.ts b/src/cli/resources/destinations.ts index 98f30a736..455be3f5c 100644 --- a/src/cli/resources/destinations.ts +++ b/src/cli/resources/destinations.ts @@ -9,13 +9,15 @@ import { registerResource } from '../crud.js'; * on `hasTable('agent_destinations')` and load `writeDestinations` lazily — * same pattern as container-runner.ts on container wake. * - * Called from both `add` and `remove` so the live container picks up the - * change without waiting for the next spawn. Without this, send_message to - * the new local_name silently drops with "unknown destination" until restart. + * Called from every destination-mutating ncl command — `add` and `remove` + * here, plus `wirings create` (which writes a companion destination row in + * its postCreate hook) — so the live container picks up the change without + * waiting for the next spawn. Without this, send_message to the new + * local_name silently drops with "unknown destination" until restart. * See the destination-projection invariant in * src/modules/agent-to-agent/db/agent-destinations.ts. */ -async function projectDestinationsToSessions(agentGroupId: string): Promise { +export async function projectDestinationsToSessions(agentGroupId: string): Promise { if (!hasTable(getDb(), 'agent_destinations')) return; const { writeDestinations } = await import('../../modules/agent-to-agent/write-destinations.js'); for (const session of getSessionsByAgentGroup(agentGroupId)) { diff --git a/src/cli/resources/wirings.ts b/src/cli/resources/wirings.ts index c7de00e5d..1b3de8fdb 100644 --- a/src/cli/resources/wirings.ts +++ b/src/cli/resources/wirings.ts @@ -1,6 +1,7 @@ import { ensureAgentDestinationForWiring } from '../../db/messaging-groups.js'; import type { MessagingGroupAgent } from '../../types.js'; import { registerResource } from '../crud.js'; +import { projectDestinationsToSessions } from './destinations.js'; registerResource({ name: 'wiring', @@ -78,4 +79,16 @@ registerResource({ // CRUD path doesn't, hence this hook. See issue #2389. ensureAgentDestinationForWiring(row as unknown as MessagingGroupAgent); }, + postCommit: async (row) => { + // Live-refresh parity with `ncl destinations add`: `postCreate` above + // only wrote the central `agent_destinations` row. Any container already + // running for this agent keeps serving its stale session projection, so + // it would drop replies to this chat as "unknown destination" until the + // next spawn (the exact symptom operators hit running `ncl wirings + // create` against a live instance — it needed a group restart). Project + // the new destination into live sessions now so the fix takes effect + // without a restart. Runs after commit because it writes to session + // `inbound.db` files (outside the central-DB transaction) and is async. + await projectDestinationsToSessions((row as unknown as MessagingGroupAgent).agent_group_id); + }, });