fix: screenshot Content-Type guard + empty session cleanup (#38)

Two fixes from the code audit in #38:

1. plugin.ts camofox_screenshot: check Content-Type header before
   base64-encoding response. If server returns JSON/text instead of
   image (e.g. error with 200 status), return as text error instead
   of crashing the client. Also reject responses < 100 bytes.

2. Tab reaper now cleans up sessions with zero tabs remaining.
   Previously, after the reaper closed all idle tabs in a session,
   the empty session (browser context ~50-100MB) would linger until
   the 10-minute session timeout. Now it's closed immediately and
   the browser idle shutdown is triggered if no sessions remain.

plugin.ts

@@ -399,6 +399,13 @@ export default function register(api: PluginApi) {
         const text = await res.text();
         throw new Error(`${res.status}: ${text}`);
       }
+      // Guard: if server returns JSON/text instead of image (e.g. error with 200),
+      // return as text to avoid crashing the client with base64-encoded JSON.
+      const contentType = res.headers.get('content-type') || '';
+      if (!contentType.startsWith('image/')) {
+        const text = await res.text();
+        return { content: [{ type: "text", text: `Screenshot failed: ${text}` }] };
+      }
       const arrayBuffer = await res.arrayBuffer();
       const base64 = Buffer.from(arrayBuffer).toString("base64");
       return {
@@ -406,7 +413,7 @@ export default function register(api: PluginApi) {
           {
             type: "image",
             data: base64,
-            mimeType: "image/png",
+            mimeType: contentType || "image/png",
           },
         ],
       };

server.js

@@ -2675,7 +2675,17 @@ setInterval(() => {
         session.tabGroups.delete(listItemId);
       }
     }
+    // Clean up sessions with zero tabs remaining — free browser context memory
+    if (session.tabGroups.size === 0) {
+      log('info', 'session empty after tab reaper, closing', { userId });
+      clearSessionDownloads(session).catch(() => {});
+      session.context.close().catch(() => {});
+      sessions.delete(userId);
+      sessionsExpiredTotal.inc();
+      refreshActiveTabsGauge();
+    }
   }
+  if (sessions.size === 0) scheduleBrowserIdleShutdown();
 }, 60_000);
 
 // =============================================================================

tests/unit/screenshotToolResult.test.js

@@ -24,7 +24,11 @@ let mockPort;
 beforeAll(async () => {
   await new Promise((resolve) => {
     mockServer = http.createServer((req, res) => {
-      if (req.url.includes('/screenshot') && !req.url.includes('missing')) {
+      if (req.url.includes('/screenshot') && req.url.includes('json-error')) {
+        // Simulate server returning JSON error with 200 status
+        res.writeHead(200, { 'Content-Type': 'application/json' });
+        res.end(JSON.stringify({ error: 'Tab not found' }));
+      } else if (req.url.includes('/screenshot') && !req.url.includes('missing')) {
         // Simulate the real server: return raw PNG binary
         res.writeHead(200, { 'Content-Type': 'image/png' });
         res.end(TINY_PNG);
@@ -94,7 +98,7 @@ describe('OLD plugin screenshot logic (broken)', () => {
 });
 
 describe('NEW plugin screenshot logic (fixed)', () => {
-  // Reproduces the fixed code path from plugin.ts
+  // Reproduces the fixed code path from plugin.ts (with Content-Type guard)
 
   async function screenshotExecute(baseUrl, tabId, userId) {
     const url = `${baseUrl}/tabs/${tabId}/screenshot?userId=${userId}`;
@@ -103,6 +107,11 @@ describe('NEW plugin screenshot logic (fixed)', () => {
       const text = await res.text();
       throw new Error(`${res.status}: ${text}`);
     }
+    const contentType = res.headers.get('content-type') || '';
+    if (!contentType.startsWith('image/')) {
+      const text = await res.text();
+      return { content: [{ type: 'text', text: `Screenshot failed: ${text}` }] };
+    }
     const arrayBuffer = await res.arrayBuffer();
     const base64 = Buffer.from(arrayBuffer).toString('base64');
     return {
@@ -110,7 +119,7 @@ describe('NEW plugin screenshot logic (fixed)', () => {
         {
           type: 'image',
           data: base64,
-          mimeType: 'image/png',
+          mimeType: contentType || 'image/png',
         },
       ],
     };
@@ -165,4 +174,17 @@ describe('NEW plugin screenshot logic (fixed)', () => {
       screenshotExecute(baseUrl, 'missing-tab', 'testuser')
     ).rejects.toThrow('404');
   });
+
+  test('returns text error when server responds with JSON instead of image', async () => {
+    const baseUrl = `http://localhost:${mockPort}`;
+    const result = await screenshotExecute(baseUrl, 'json-error-tab', 'testuser');
+
+    expect(result.content).toHaveLength(1);
+    expect(result.content[0].type).toBe('text');
+    expect(result.content[0].text).toContain('Screenshot failed');
+    expect(result.content[0].text).toContain('Tab not found');
+    // Must NOT have an image block
+    expect(result.content.find(c => c.type === 'image')).toBeUndefined();
+  });
+
 });

tests/unit/sessionCleanup.test.js

@@ -0,0 +1,147 @@
+/**
+ * Unit tests for session cleanup after tab reaper empties all tabs.
+ *
+ * Tests the logic that when the tab reaper closes all idle tabs in a session,
+ * the empty session (browser context) should be cleaned up immediately rather
+ * than lingering until the session timeout.
+ */
+
+describe('session cleanup after tab reaper', () => {
+  // Simulate the reaper loop logic from server.js
+  function runTabReaper({ sessions, TAB_INACTIVITY_MS, destroyTab, onSessionEmpty }) {
+    const now = Date.now();
+    for (const [userId, session] of sessions) {
+      for (const [listItemId, group] of session.tabGroups) {
+        for (const [tabId, tabState] of group) {
+          if (!tabState._lastReaperCheck) {
+            tabState._lastReaperCheck = now;
+            tabState._lastReaperToolCalls = tabState.toolCalls;
+            continue;
+          }
+          if (tabState.toolCalls === tabState._lastReaperToolCalls) {
+            const idleMs = now - tabState._lastReaperCheck;
+            if (idleMs >= TAB_INACTIVITY_MS) {
+              destroyTab(tabId);
+              group.delete(tabId);
+            }
+          } else {
+            tabState._lastReaperCheck = now;
+            tabState._lastReaperToolCalls = tabState.toolCalls;
+          }
+        }
+        if (group.size === 0) {
+          session.tabGroups.delete(listItemId);
+        }
+      }
+      // The fix: clean up empty sessions
+      if (session.tabGroups.size === 0) {
+        onSessionEmpty(userId);
+        sessions.delete(userId);
+      }
+    }
+  }
+
+  function makeSession(tabs) {
+    const tabGroups = new Map();
+    const group = new Map();
+    for (const [tabId, tabState] of Object.entries(tabs)) {
+      group.set(tabId, { toolCalls: 0, ...tabState });
+    }
+    tabGroups.set('list-1', group);
+    return { tabGroups, lastAccess: Date.now() };
+  }
+
+  test('empty session is cleaned up when all tabs are reaped', () => {
+    const past = Date.now() - 600_000; // 10 min ago
+    const sessions = new Map();
+    sessions.set('user-1', makeSession({
+      'tab-1': { _lastReaperCheck: past, _lastReaperToolCalls: 0, toolCalls: 0 },
+      'tab-2': { _lastReaperCheck: past, _lastReaperToolCalls: 0, toolCalls: 0 },
+    }));
+
+    const destroyed = [];
+    const emptied = [];
+
+    runTabReaper({
+      sessions,
+      TAB_INACTIVITY_MS: 300_000,
+      destroyTab: (id) => destroyed.push(id),
+      onSessionEmpty: (userId) => emptied.push(userId),
+    });
+
+    expect(destroyed).toEqual(['tab-1', 'tab-2']);
+    expect(emptied).toEqual(['user-1']);
+    expect(sessions.size).toBe(0);
+  });
+
+  test('session with active tabs is NOT cleaned up', () => {
+    const past = Date.now() - 600_000;
+    const sessions = new Map();
+    sessions.set('user-1', makeSession({
+      'tab-1': { _lastReaperCheck: past, _lastReaperToolCalls: 0, toolCalls: 0 },
+      'tab-2': { _lastReaperCheck: past, _lastReaperToolCalls: 0, toolCalls: 5 }, // active
+    }));
+
+    const destroyed = [];
+    const emptied = [];
+
+    runTabReaper({
+      sessions,
+      TAB_INACTIVITY_MS: 300_000,
+      destroyTab: (id) => destroyed.push(id),
+      onSessionEmpty: (userId) => emptied.push(userId),
+    });
+
+    expect(destroyed).toEqual(['tab-1']);
+    expect(emptied).toEqual([]);
+    expect(sessions.size).toBe(1);
+    // Active tab should still be present
+    const session = sessions.get('user-1');
+    expect(session.tabGroups.get('list-1').has('tab-2')).toBe(true);
+  });
+
+  test('multiple sessions: only empty ones are cleaned up', () => {
+    const past = Date.now() - 600_000;
+    const sessions = new Map();
+    sessions.set('user-1', makeSession({
+      'tab-1': { _lastReaperCheck: past, _lastReaperToolCalls: 0, toolCalls: 0 },
+    }));
+    sessions.set('user-2', makeSession({
+      'tab-2': { _lastReaperCheck: past, _lastReaperToolCalls: 0, toolCalls: 3 }, // active
+    }));
+
+    const emptied = [];
+
+    runTabReaper({
+      sessions,
+      TAB_INACTIVITY_MS: 300_000,
+      destroyTab: () => {},
+      onSessionEmpty: (userId) => emptied.push(userId),
+    });
+
+    expect(emptied).toEqual(['user-1']);
+    expect(sessions.size).toBe(1);
+    expect(sessions.has('user-2')).toBe(true);
+  });
+
+  test('tabs not yet checked are skipped (first pass initializes reaper state)', () => {
+    const sessions = new Map();
+    sessions.set('user-1', makeSession({
+      'tab-1': { toolCalls: 0 }, // no _lastReaperCheck
+    }));
+
+    const destroyed = [];
+    const emptied = [];
+
+    runTabReaper({
+      sessions,
+      TAB_INACTIVITY_MS: 300_000,
+      destroyTab: (id) => destroyed.push(id),
+      onSessionEmpty: (userId) => emptied.push(userId),
+    });
+
+    expect(destroyed).toEqual([]);
+    expect(emptied).toEqual([]);
+    expect(sessions.size).toBe(1);
+  });
+});