From 380df4b65f79b935d84e6cd9ac66db7467c8f364 Mon Sep 17 00:00:00 2001 From: claudemm Date: Fri, 18 Sep 2026 20:42:39 +0300 Subject: [PATCH] room_ack: refuse a blind clear instead of warning after the fact A bare room_ack (no room_list_new this session) blanked the whole notification file and returned a sentence advising against it. The advice arrived after the messages were gone, which is the shape of guard that cannot help: it performs the destructive act and then names it. The code already knew. Its own comment called the path "inherently racy" and cited the 2026-07-08 incident where an owner instruction sat five hours unseen. Two agents on this fleet hit it again today. room_ack now refuses when the file has unread lines and there is nothing to ack against, and the refusal names the one command that makes the ack safe. Acking an empty file cold is harmless, so that stays a no-op. The normal path -- room_list_new, then room_ack -- is untouched: it still removes only the consumed bytes and preserves late arrivals. The legacy clear-all unit test was the specification of the old behaviour, so it is replaced rather than deleted: one test asserts the refusal AND that the file is unchanged after it, one asserts the empty no-op, and a new end-to-end test drives all three paths over stdio. 333/333 tests pass. Co-Authored-By: Claude Opus 5 --- src/mcp-server.mjs | 36 +++++++++++++++++----- test/mcp-server.test.mjs | 64 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 90 insertions(+), 10 deletions(-) diff --git a/src/mcp-server.mjs b/src/mcp-server.mjs index d3e41fd..a698583 100644 --- a/src/mcp-server.mjs +++ b/src/mcp-server.mjs @@ -353,8 +353,30 @@ export function ackNotificationFile(notifyFile, consumedRaw) { current = ''; // missing file == already empty } if (consumedRaw == null) { - if (current !== '') atomicWriteNotify(notifyFile, ''); - return { mode: 'all', consumedLines: countNotificationLines(current), preservedLines: 0 }; + // No room_list_new this session. The old behaviour was to blank the file + // anyway and return a sentence advising against it. A warning that still + // performs the destructive act is not a guard: it destroys unread messages + // and tells you afterwards. Two agents on this fleet hit it in one day. + // + // Clearing an EMPTY file is harmless, so that still succeeds as a no-op. + // Clearing a file with unread lines in it is refused, and the refusal names + // the one command that makes the ack safe. + const pending = countNotificationLines(current); + if (pending === 0) { + return { mode: 'noop', consumedLines: 0, preservedLines: 0 }; + } + return { + mode: 'refused', + consumedLines: 0, + preservedLines: pending, + error: + `REFUSING to ack: ${pending} unread line(s) in ${notifyFile} and no room_list_new ` + + 'was recorded this session, so there is nothing to ack AGAINST. Blanking the file ' + + 'here would discard messages nobody has read — that is exactly how an owner ' + + 'instruction sat unseen for five hours on 2026-07-08.\n' + + 'Call room_list_new first, act on what it returns, then room_ack: it removes only ' + + 'those lines and preserves anything the poller appended meanwhile.', + }; } const { remainder, consumedLines, mode } = removeConsumedNotifications(current, consumedRaw); if (remainder !== current) atomicWriteNotify(notifyFile, remainder); @@ -708,11 +730,11 @@ export async function runMcpServer({ configPath } = {}) { const consumedRaw = lastRoomListNew.has(notifyFile) ? lastRoomListNew.get(notifyFile) : null; const result = ackNotificationFile(notifyFile, consumedRaw); lastRoomListNew.delete(notifyFile); - if (result.mode === 'all') { - return ok( - 'Acknowledged new messages (no room_list_new recorded this session — cleared the whole file; ' + - 'prefer room_list_new → room_ack so late arrivals are preserved).' - ); + if (result.mode === 'refused') { + return ok(result.error); + } + if (result.mode === 'noop' && result.consumedLines === 0 && result.preservedLines === 0) { + return ok('Nothing to acknowledge — the notification file is already empty.'); } if (result.preservedLines > 0) { return ok( diff --git a/test/mcp-server.test.mjs b/test/mcp-server.test.mjs index e94eaee..02fca30 100644 --- a/test/mcp-server.test.mjs +++ b/test/mcp-server.test.mjs @@ -378,14 +378,32 @@ test('ackNotificationFile: REGRESSION — poller append between read and ack sur } }); -test('ackNotificationFile: no prior read (null) keeps legacy clear-all contract', () => { +// CONTRACT CHANGE 2026-09-18: a null prior read used to clear the file and return +// advice not to do that. The advice arrived after the messages were gone. It now +// refuses when there is anything unread, and stays a no-op when there is not. +test('ackNotificationFile: no prior read (null) REFUSES and preserves unread lines', () => { const dir = mkdtempSync(join(tmpdir(), 'iak-ack-test-')); const notifyFile = join(dir, 'new-messages.txt'); try { writeFileSync(notifyFile, '[room] a: x\n[room] b: y\n'); const r = ackNotificationFile(notifyFile, null); - assert.equal(r.mode, 'all'); - assert.equal(readFileSync(notifyFile, 'utf8'), ''); + assert.equal(r.mode, 'refused'); + assert.equal(r.preservedLines, 2); + assert.match(r.error, /room_list_new first/); + assert.equal(readFileSync(notifyFile, 'utf8'), '[room] a: x\n[room] b: y\n'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('ackNotificationFile: no prior read (null) on an EMPTY file is still a no-op', () => { + const dir = mkdtempSync(join(tmpdir(), 'iak-ack-test-')); + const notifyFile = join(dir, 'new-messages.txt'); + try { + writeFileSync(notifyFile, ''); + const r = ackNotificationFile(notifyFile, null); + assert.equal(r.mode, 'noop'); + assert.equal(r.preservedLines, 0); } finally { rmSync(dir, { recursive: true, force: true }); } @@ -474,6 +492,46 @@ test('iak-mcp.mjs REGRESSION: room_ack clears only what room_list_new returned', } }); +test('iak-mcp.mjs REGRESSION: a bare room_ack REFUSES to discard unread messages', async () => { + const dir = mkdtempSync(join(tmpdir(), 'iak-mcp-test-')); + const cfgPath = join(dir, 'config.json'); + const notifyFile = join(dir, 'new-messages.txt'); + writeFileSync(cfgPath, JSON.stringify({ + poller: { notification_file: notifyFile }, + tmux: { allow: [], default_session: 't' }, + })); + // Two lines nobody has read, and NO room_list_new this session. + writeFileSync(notifyFile, '[room] petrus: did you hear me\n[room] petrus: answer\n'); + const { request, close } = bootMcp(cfgPath); + try { + const acked = await request('tools/call', { name: 'room_ack', arguments: {} }); + const text = acked.result.content[0].text; + assert.match(text, /REFUSING to ack/); + assert.match(text, /room_list_new first/); + // The point of the guard: the messages are STILL THERE. + assert.equal( + readFileSync(notifyFile, 'utf8'), + '[room] petrus: did you hear me\n[room] petrus: answer\n', + 'a refused ack must not modify the notification file' + ); + + // An empty file is harmless to ack cold -- that still succeeds as a no-op, + // so the guard refuses the destructive case only. + writeFileSync(notifyFile, ''); + const acked2 = await request('tools/call', { name: 'room_ack', arguments: {} }); + assert.match(acked2.result.content[0].text, /already empty/); + + // And the normal path is untouched: list, then ack, and it clears. + writeFileSync(notifyFile, '[room] alice: hello\n'); + await request('tools/call', { name: 'room_list_new', arguments: {} }); + await request('tools/call', { name: 'room_ack', arguments: {} }); + assert.equal(readFileSync(notifyFile, 'utf8'), ''); + } finally { + await close(); + rmSync(dir, { recursive: true, force: true }); + } +}); + // --- end-to-end stdio regression: wake_remote gate auth ---------------------- // A tiny stand-in for a remote IAK daemon whose auth_token is set: every