fix(agent-inbox): review round 1

- codeman agent post: accept text that starts with "-" (markdown from a
  file), --json and -h stay options.
- DELETE /api/sessions/:id on a persisted-only row drops the inbox a detach
  kept.
- inbox --wait: wait.ended is exit 3 only when the session list no longer has
  the session; a restart or detach ending the wait is exit 2 (wait again).
- A read whose client hung up during ?wait= marks nothing seen.
- InboxPostSchema bound is 4x the byte cap, so every oversize message gets
  the store's "post a file path" answer.
- Store: restore sorts oldest first; the TTL sweep checks every message,
  not only the first.
- Docs: admins can act as any session of the receiver's owner (now tested
  for admin and non-admin); a peek with ?wait= does not block; exit codes.
- Tests: partial PUT /api/settings keeps persistence on.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

test/agent-inbox.test.ts

@@ -161,6 +161,15 @@ describe('AgentInbox', () => {
       expect(new AgentInbox(() => now).restore(snap)).toBe(0);
     });
 
+    it('sweeps an expired message that is not first (clock stepped back)', () => {
+      const late = post('posted first, stamped later');
+      now -= 1000; // the clock steps back
+      const early = post('posted second, stamped earlier');
+      now += MESSAGE_TTL_MS + 1000 - (late.createdAt - early.createdAt) / 2;
+      // `early` is past the TTL, `late` is not, and `early` sits behind `late`.
+      expect(inbox.read(RX, { receiver: false }).messages.map((m) => m.id)).toEqual([late.id]);
+    });
+
     it('expiry fires onChange so the snapshot catches up', () => {
       post('m');
       let changes = 0;
@@ -248,6 +257,13 @@ describe('AgentInbox', () => {
       ).toBe(1);
     });
 
+    it('restores oldest first whatever the file order, so the caps keep the oldest', () => {
+      const msg = (id: string, createdAt: number) => ({ id, from: null, text: 'x', createdAt, seenAt: null });
+      const restored = new AgentInbox(() => now);
+      restored.restore({ version: 1, inboxes: { s: [msg('new', now - 10), msg('old', now - 20)] } });
+      expect(restored.read('s', { receiver: false }).messages.map((m) => m.id)).toEqual(['old', 'new']);
+    });
+
     it('holds the count cap on restore', () => {
       const many = Array.from({ length: MAX_MESSAGES_PER_INBOX + 5 }, (_, i) => ({
         id: `m${i}`,

test/cli-agent.test.ts

@@ -714,12 +714,32 @@ describe('agent inbox verbs', () => {
     expect(deps.err.join('')).toMatch(/peek: nothing was marked seen/);
   });
 
-  it('inbox --wait passes the budget and maps timeout to exit 2, a vanished session to exit 3', async () => {
+  it('inbox --wait passes the budget and maps timeout to exit 2', async () => {
     const timedOut = fakeDeps([ok({ receiver: true, messages: [], pending: 0, unseen: 0, wait: { timedOut: true } })]);
     expect(await agentInboxRead(timedOut, { waitMs: 30_000 })).toBe(EXIT.timeout);
     expect(timedOut.calls[0]).toMatchObject({ query: { wait: 30_000 }, timeoutMs: 40_000 });
-    const ended = fakeDeps([ok({ receiver: true, messages: [], pending: 0, unseen: 0, wait: { ended: true } })]);
-    expect(await agentInboxRead(ended, { waitMs: 30_000 })).toBe(EXIT.dead);
+  });
+
+  describe('a wait the server ended', () => {
+    const ended = ok({ receiver: true, messages: [], pending: 0, unseen: 0, wait: { ended: true } });
+
+    it('is exit 3 only when the session list confirms the session is gone', async () => {
+      const deps = fakeDeps([ended, ok([{ id: OTHER }])]);
+      expect(await agentInboxRead(deps, { waitMs: 30_000 })).toBe(EXIT.dead);
+      expect(deps.calls[1]).toMatchObject({ method: 'GET', path: '/api/v1/sessions' });
+      expect(deps.err.join('')).toMatch(/went away/);
+    });
+
+    it('is exit 2 (wait again) while the session is still listed: a restart or detach', async () => {
+      const deps = fakeDeps([ended, ok([{ id: SELF }])]);
+      expect(await agentInboxRead(deps, { waitMs: 30_000 })).toBe(EXIT.timeout);
+      expect(deps.err.join('')).toMatch(/ended the wait early/);
+    });
+
+    it('is exit 2 when the list cannot be had (the server is still coming back)', async () => {
+      const deps = fakeDeps([ended, apiError(503, 'INTERNAL_ERROR', 'restarting')]);
+      expect(await agentInboxRead(deps, { waitMs: 30_000 })).toBe(EXIT.timeout);
+    });
   });
 
   it('ack posts the ids to this session’s own inbox', async () => {
@@ -767,6 +787,26 @@ describe('help texts carry the traps the skill documents', () => {
     expect(sub('send').description()).toContain('send <id> -- "- fix the bug"');
   });
 
+  it('post takes text that starts with "-" (markdown from a file) without --', async () => {
+    const program = new Command();
+    program.exitOverride();
+    const post = registerAgentCommands(program).commands.find((c) => c.name() === 'post')!;
+    const got: Array<{ words: string[]; json: boolean }> = [];
+    post.action((_id: string, words: string[], o: { json?: boolean }) => {
+      got.push({ words, json: Boolean(o.json) });
+    });
+    for (const text of ['- [ ] item\nnext', '---\ntitle: a', '-v is great']) {
+      await program.parseAsync(['node', 'codeman', 'agent', 'post', 'abcdefgh', text]);
+    }
+    await program.parseAsync(['node', 'codeman', 'agent', 'post', 'abcdefgh', 'hi', '--json']);
+    expect(got).toEqual([
+      { words: ['- [ ] item\nnext'], json: false },
+      { words: ['---\ntitle: a'], json: false },
+      { words: ['-v is great'], json: false },
+      { words: ['hi'], json: true },
+    ]);
+  });
+
   it('rm does not claim a lineage check it does not make', () => {
     expect(sub('rm').description()).toMatch(/^Delete any session except this one/);
   });

test/routes/inbox-routes.test.ts

@@ -101,6 +101,28 @@ describe('agent inbox routes', () => {
       expect(res.statusCode).toBe(400);
     });
 
+    it('multi-user, non-admin: a session the caller cannot see never resolves as the sender', async () => {
+      process.env.CODEMAN_MULTIUSER = '1';
+      currentUser = { username: 'alice', role: 'user' };
+      (h.ctx.sessions.get(RX) as unknown as { owner: string }).owner = 'alice';
+      (h.ctx.sessions.get(TX) as unknown as { owner: string }).owner = 'bob';
+      (h.ctx.sessions.get(OTHER) as unknown as { owner: string }).owner = 'alice';
+      expect(data(await post(RX, 'x', TX)).message.from).toBeNull();
+      expect(data(await post(RX, 'y', OTHER)).message.from).toBe(OTHER);
+      // Nor can alice read as bob's session or ack for it: bob's inbox is a 404 to her.
+      expect((await read(TX, TX)).statusCode).toBe(404);
+      expect((await ack(TX, ['m'], TX)).statusCode).toBe(404);
+    });
+
+    it('multi-user, admin: may act as any session of the receiver’s owner (admins see everything)', async () => {
+      process.env.CODEMAN_MULTIUSER = '1';
+      currentUser = { username: 'root', role: 'admin' };
+      (h.ctx.sessions.get(RX) as unknown as { owner: string }).owner = 'bob';
+      (h.ctx.sessions.get(TX) as unknown as { owner: string }).owner = 'bob';
+      expect(data(await post(RX, 'x', TX)).message.from).toBe(TX);
+      expect(data(await read(RX, RX)).receiver).toBe(true);
+    });
+
     it('multi-user: a session of another owner never resolves as the sender', async () => {
       process.env.CODEMAN_MULTIUSER = '1';
       currentUser = { username: 'alice', role: 'admin' };
@@ -128,8 +150,12 @@ describe('agent inbox routes', () => {
       expect(data(await read(RX)).messages[0].id).toBe(first);
     });
 
-    it('a message over the byte cap is 400', async () => {
-      expect((await post(RX, 'é'.repeat(8193))).statusCode).toBe(400);
+    it('a message over the byte cap is 400 with the store’s hint, multi-byte or not', async () => {
+      for (const text of ['é'.repeat(8193), 'x'.repeat(16_385)]) {
+        const res = await post(RX, text);
+        expect(res.statusCode).toBe(400);
+        expect(JSON.parse(res.body).error).toMatch(/Write it to a file in the workspace and post the path/);
+      }
     });
 
     it('404 for an unknown session', async () => {

test/routes/session-routes.test.ts

@@ -26,6 +26,7 @@ import { createMockRouteContext, type MockRouteContext } from '../mocks/index.js
 import { installRouteErrorHandler } from '../../src/web/route-error-handler.js';
 import { ApiErrorCode, httpStatusForErrorCode } from '../../src/types.js';
 import { Session } from '../../src/session.js';
+import { agentInbox } from '../../src/web/agent-inbox.js';
 
 // Mock execFile so the send-key route's `tmux` invocation is observable (not run for real).
 const { execFile } = vi.hoisted(() => ({ execFile: vi.fn() }));
@@ -985,6 +986,17 @@ describe('session-routes', () => {
       expect(harness.ctx.broadcast).toHaveBeenCalledWith('session:deleted', { id: 'ghost-session' });
     });
 
+    it('a persisted-only delete drops the agent inbox the detach kept', async () => {
+      // Detach (killMux=false) keeps the inbox; retiring the row afterwards is the delete.
+      agentInbox.resetForTests();
+      agentInbox.post('ghost-session', null, 'mail from before the detach');
+      vi.mocked(harness.ctx.store.getSession).mockReturnValueOnce({ id: 'ghost-session', owner: undefined } as never);
+      const res = await harness.app.inject({ method: 'DELETE', url: '/api/sessions/ghost-session' });
+      expect(res.statusCode).toBe(200);
+      expect(agentInbox.pendingCount('ghost-session')).toBe(0);
+      agentInbox.resetForTests();
+    });
+
     it('404s a persisted-only session id the state store does not recognize either', async () => {
       vi.mocked(harness.ctx.store.getSession).mockReturnValueOnce(null);
       const res = await harness.app.inject({

test/routes/system-routes-settings-partial-put.test.ts

@@ -47,13 +47,15 @@ const { EXISTING_SETTINGS, subagentWatcher, imageWatcher, workflowRunWatcher } =
     };
   };
   return {
-    // Persisted settings.json for these tests: two watchers ON, subagent tracking OFF.
+    // Persisted settings.json for these tests: two watchers ON, subagent tracking OFF,
+    // agent inbox snapshot ON.
     EXISTING_SETTINGS: {
       subagentTrackingEnabled: false,
       imageWatcherEnabled: true,
       showUltracodeAgents: true,
       promptCacheKeepWarm: false,
       promptCacheKeepWarmMaxHours: 2,
+      agentInboxPersistEnabled: true,
     },
     subagentWatcher: makeWatcher(false),
     imageWatcher: makeWatcher(true),
@@ -61,6 +63,9 @@ const { EXISTING_SETTINGS, subagentWatcher, imageWatcher, workflowRunWatcher } =
   };
 });
 
+// setEnabled(false) deletes the snapshot file, so a wrong default here loses mail.
+const { agentInboxPersistence } = vi.hoisted(() => ({ agentInboxPersistence: { setEnabled: vi.fn() } }));
+
 vi.mock('node:fs/promises', () => ({
   default: {
     readFile: vi.fn(async () => JSON.stringify(EXISTING_SETTINGS)),
@@ -76,6 +81,7 @@ vi.mock('node:fs', async (importOriginal) => {
 vi.mock('../../src/subagent-watcher.js', () => ({ subagentWatcher }));
 vi.mock('../../src/image-watcher.js', () => ({ imageWatcher }));
 vi.mock('../../src/workflow-run-watcher.js', () => ({ workflowRunWatcher }));
+vi.mock('../../src/web/agent-inbox-persistence.js', () => ({ agentInboxPersistence }));
 
 describe('PUT /api/settings — partial body must not reset service toggles', () => {
   let harness: RouteTestHarness;
@@ -87,6 +93,7 @@ describe('PUT /api/settings — partial body must not reset service toggles', ()
       w.stop.mockClear();
       w.__resetRunning(); // running state, not just call records — see makeWatcher
     }
+    agentInboxPersistence.setEnabled.mockClear();
   });
 
   afterEach(async () => {
@@ -167,4 +174,25 @@ describe('PUT /api/settings — partial body must not reset service toggles', ()
     expect((await put({ promptCacheKeepWarm: true })).statusCode).toBe(200);
     expect(harness.ctx.keepWarm.rescheduleAll).toHaveBeenCalledTimes(2);
   });
+
+  it('keeps the agent inbox snapshot on when the body omits its key', async () => {
+    const res = await harness.app.inject({
+      method: 'PUT',
+      url: '/api/settings',
+      payload: { showPlanUsageLimits: true },
+    });
+    expect(res.statusCode).toBe(200);
+    expect(agentInboxPersistence.setEnabled).toHaveBeenCalledWith(true);
+    expect(agentInboxPersistence.setEnabled).not.toHaveBeenCalledWith(false);
+  });
+
+  it('switches the agent inbox snapshot off when the body says so', async () => {
+    const res = await harness.app.inject({
+      method: 'PUT',
+      url: '/api/settings',
+      payload: { agentInboxPersistEnabled: false },
+    });
+    expect(res.statusCode).toBe(200);
+    expect(agentInboxPersistence.setEnabled).toHaveBeenCalledWith(false);
+  });
 });