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