From eef285ba3b22c96ee8af6e8f59c326371f9f9f97 Mon Sep 17 00:00:00 2001 From: gavrielc Date: Thu, 11 Jun 2026 13:54:17 +0300 Subject: [PATCH] fix(session-manager): open outbound.db read-write in writeOutboundDirect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit writeOutboundDirect opened the session's outbound DB through openOutboundDb, which sets readonly: true. The INSERT it then runs threw SQLITE_READONLY on every call, so the command-gate denial path (router.ts) never delivered its 'Permission denied' response — the sender just got silence, and the throw aborted routing for that inbound event. Switch to the openOutboundDbRw wrapper, which opens the same path with write access (DELETE journal + busy_timeout). The host-side write to the container-owned outbound.db is safe: both sides use DELETE journal mode, and the even host seq stays out of the container's odd-seq space. Adds a guard test that drives writeOutboundDirect against a real session folder and asserts the denial rows land in messages_out with even seqs; it goes red if the open call reverts to the readonly form. Co-Authored-By: Claude Fable 5 --- src/session-manager.test.ts | 100 ++++++++++++++++++++++++++++++++++++ src/session-manager.ts | 8 ++- 2 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 src/session-manager.test.ts diff --git a/src/session-manager.test.ts b/src/session-manager.test.ts new file mode 100644 index 000000000..f8b37a47a --- /dev/null +++ b/src/session-manager.test.ts @@ -0,0 +1,100 @@ +/** + * Tests for session-manager's direct outbound write path. + * + * Drives the real `writeOutboundDirect` entry against a real session folder + * on disk. A previous implementation opened the outbound DB through + * `openOutboundDb` (readonly: true), so every INSERT threw SQLITE_READONLY + * and the command-gate denial path silently never delivered. Goes red if the + * open call reverts to the readonly form. + */ +import fs from 'fs'; +import Database from 'better-sqlite3'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('./config.js', async () => { + const actual = await vi.importActual('./config.js'); + return { ...actual, DATA_DIR: '/tmp/nanoclaw-test-write-outbound' }; +}); + +import { initSessionFolder, outboundDbPath, writeOutboundDirect } from './session-manager.js'; + +const TEST_DIR = '/tmp/nanoclaw-test-write-outbound'; +const AG = 'ag-test'; +const SESS = 'sess-test'; + +function readMessagesOut(): Array<{ id: string; seq: number; kind: string; content: string }> { + const db = new Database(outboundDbPath(AG, SESS), { readonly: true }); + try { + return db.prepare('SELECT id, seq, kind, content FROM messages_out ORDER BY seq').all() as Array<{ + id: string; + seq: number; + kind: string; + content: string; + }>; + } finally { + db.close(); + } +} + +beforeEach(() => { + if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true }); + fs.mkdirSync(TEST_DIR, { recursive: true }); + initSessionFolder(AG, SESS); +}); + +afterEach(() => { + if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true }); +}); + +describe('writeOutboundDirect', () => { + it('inserts into messages_out with an even host-side seq (requires a writable outbound.db)', () => { + // With a readonly open this very call throws SQLITE_READONLY. + writeOutboundDirect(AG, SESS, { + id: 'denial-1', + kind: 'chat', + platformId: 'slack:C1', + channelType: 'slack', + threadId: null, + content: JSON.stringify({ text: 'Admin commands are restricted.' }), + }); + + const rows = readMessagesOut(); + expect(rows).toHaveLength(1); + expect(rows[0].id).toBe('denial-1'); + expect(rows[0].seq).toBe(2); + expect(rows[0].seq % 2).toBe(0); // host uses even seq numbers + expect(JSON.parse(rows[0].content).text).toBe('Admin commands are restricted.'); + }); + + it('keeps host seq numbers even across multiple writes and ignores duplicate ids', () => { + writeOutboundDirect(AG, SESS, { + id: 'denial-1', + kind: 'chat', + platformId: null, + channelType: null, + threadId: null, + content: '{"text":"first"}', + }); + writeOutboundDirect(AG, SESS, { + id: 'denial-2', + kind: 'chat', + platformId: null, + channelType: null, + threadId: null, + content: '{"text":"second"}', + }); + // INSERT OR IGNORE — a delivery retry with the same id must not throw or duplicate. + writeOutboundDirect(AG, SESS, { + id: 'denial-1', + kind: 'chat', + platformId: null, + channelType: null, + threadId: null, + content: '{"text":"retry"}', + }); + + const rows = readMessagesOut(); + expect(rows.map((r) => r.id)).toEqual(['denial-1', 'denial-2']); + expect(rows.map((r) => r.seq)).toEqual([2, 4]); + }); +}); diff --git a/src/session-manager.ts b/src/session-manager.ts index 38c77f28d..ba345aed8 100644 --- a/src/session-manager.ts +++ b/src/session-manager.ts @@ -378,6 +378,12 @@ export function openOutboundDbRw(agentGroupId: string, sessionId: string): Datab * Write a message directly to a session's outbound DB so the host delivery * loop picks it up. Used by the command gate to send denial responses * without waking a container. + * + * Needs the read-write open — the readonly handle the delivery poll uses + * can't INSERT. This is a host-side write to the container-owned outbound.db, + * but it's safe even with a container running: both sides open with DELETE + * journal + busy_timeout, and the even host seq stays out of the container's + * odd-seq space. */ export function writeOutboundDirect( agentGroupId: string, @@ -391,7 +397,7 @@ export function writeOutboundDirect( content: string; }, ): void { - const db = openOutboundDb(agentGroupId, sessionId); + const db = openOutboundDbRw(agentGroupId, sessionId); try { db.prepare( `INSERT OR IGNORE INTO messages_out (id, seq, timestamp, kind, platform_id, channel_type, thread_id, content)