refactor(tests): align test files with unit testing rubric

apps/web/src/lib/ai/core/__tests__/ai-tools.test.ts

@@ -1,3 +1,12 @@
+/**
+ * @scaffold - aggregation tripwire
+ *
+ * This test validates that pageSpaceTools correctly aggregates all tool modules.
+ * It's a scaffold test because:
+ * - The unit is pure wiring/aggregation with no business logic
+ * - Changes here indicate tools were added/removed/renamed
+ * - Failures should prompt review of whether the change was intentional
+ */
 import { describe, it, expect, vi } from 'vitest';
 
 // Mock all tool modules to avoid database dependencies
@@ -64,91 +73,66 @@ vi.mock('../../tools/web-search-tools', () => ({
 }));
 
 import { pageSpaceTools } from '../ai-tools';
+import { driveTools } from '../../tools/drive-tools';
+import { pageReadTools } from '../../tools/page-read-tools';
+import { pageWriteTools } from '../../tools/page-write-tools';
+import { searchTools } from '../../tools/search-tools';
+import { taskManagementTools } from '../../tools/task-management-tools';
+import { agentTools } from '../../tools/agent-tools';
+import { agentCommunicationTools } from '../../tools/agent-communication-tools';
+import { webSearchTools } from '../../tools/web-search-tools';
 
 describe('ai-tools', () => {
   describe('pageSpaceTools aggregation', () => {
-    it('aggregates all tools from individual modules', () => {
-      expect(pageSpaceTools).toBeDefined();
-      expect(typeof pageSpaceTools).toBe('object');
+    it('equals the merged object of all tool modules', () => {
+      expect(pageSpaceTools).toEqual({
+        ...driveTools,
+        ...pageReadTools,
+        ...pageWriteTools,
+        ...searchTools,
+        ...taskManagementTools,
+        ...agentTools,
+        ...agentCommunicationTools,
+        ...webSearchTools,
+      });
     });
 
-    it('includes driveTools', () => {
-      expect(pageSpaceTools.list_drives).toBeDefined();
-      expect(pageSpaceTools.create_drive).toBeDefined();
-      expect(pageSpaceTools.rename_drive).toBeDefined();
-    });
-
-    it('includes pageReadTools', () => {
-      expect(pageSpaceTools.list_pages).toBeDefined();
-      expect(pageSpaceTools.read_page).toBeDefined();
-      expect(pageSpaceTools.list_trash).toBeDefined();
-    });
-
-    it('includes pageWriteTools', () => {
-      expect(pageSpaceTools.replace_lines).toBeDefined();
-      expect(pageSpaceTools.create_page).toBeDefined();
-      expect(pageSpaceTools.rename_page).toBeDefined();
-      expect(pageSpaceTools.trash).toBeDefined();
-      expect(pageSpaceTools.restore).toBeDefined();
-      expect(pageSpaceTools.move_page).toBeDefined();
-      expect(pageSpaceTools.edit_sheet_cells).toBeDefined();
-    });
-
-    it('includes searchTools', () => {
-      expect(pageSpaceTools.regex_search).toBeDefined();
-      expect(pageSpaceTools.glob_search).toBeDefined();
-      expect(pageSpaceTools.multi_drive_search).toBeDefined();
-    });
-
-    it('includes taskManagementTools', () => {
-      expect(pageSpaceTools.update_task).toBeDefined();
-    });
-
-    it('includes agentTools', () => {
-      expect(pageSpaceTools.update_agent_config).toBeDefined();
-    });
-
-    it('includes agentCommunicationTools', () => {
-      expect(pageSpaceTools.list_agents).toBeDefined();
-      expect(pageSpaceTools.multi_drive_list_agents).toBeDefined();
-      expect(pageSpaceTools.ask_agent).toBeDefined();
-    });
-
-    it('includes webSearchTools', () => {
-      expect(pageSpaceTools.web_search).toBeDefined();
-    });
-
-    it('has expected number of tools', () => {
-      const toolCount = Object.keys(pageSpaceTools).length;
-      // Total expected: 3 drive + 3 read + 7 write + 3 search + 1 task + 1 agent + 3 agent-comm + 1 web = 22
-      expect(toolCount).toBe(22);
-    });
-
-    it('has no undefined tools', () => {
-      for (const [, tool] of Object.entries(pageSpaceTools)) {
-        expect(tool).toBeDefined();
-        expect(tool).not.toBeNull();
+    it('has no key collisions between tool modules', () => {
+      const moduleKeysets = [
+        Object.keys(driveTools),
+        Object.keys(pageReadTools),
+        Object.keys(pageWriteTools),
+        Object.keys(searchTools),
+        Object.keys(taskManagementTools),
+        Object.keys(agentTools),
+        Object.keys(agentCommunicationTools),
+        Object.keys(webSearchTools),
+      ];
+
+      const allKeys = moduleKeysets.flat();
+      const uniqueKeys = new Set(allKeys);
+
+      // If there are duplicates, find them for a helpful error message
+      if (uniqueKeys.size !== allKeys.length) {
+        const seen = new Set<string>();
+        const duplicates: string[] = [];
+        for (const key of allKeys) {
+          if (seen.has(key)) {
+            duplicates.push(key);
+          }
+          seen.add(key);
+        }
+        throw new Error(`Tool name collisions detected: ${duplicates.join(', ')}`);
       }
-    });
 
-    it('has no duplicate tool names', () => {
-      const toolNames = Object.keys(pageSpaceTools);
-      const uniqueNames = new Set(toolNames);
-      expect(uniqueNames.size).toBe(toolNames.length);
+      expect(uniqueKeys.size).toBe(allKeys.length);
     });
-  });
 
-  describe('tool structure', () => {
-    it('each tool has a name property', () => {
+    it('has no undefined or null tools', () => {
       for (const [name, tool] of Object.entries(pageSpaceTools)) {
-        expect((tool as { name: string }).name).toBe(name);
-      }
-    });
-
-    it('each tool has a description property', () => {
-      for (const [, tool] of Object.entries(pageSpaceTools)) {
-        expect((tool as { description: string }).description).toBeDefined();
-        expect(typeof (tool as { description: string }).description).toBe('string');
+        if (tool === undefined || tool === null) {
+          throw new Error(`Tool "${name}" is ${tool}`);
+        }
       }
     });
   });

apps/web/src/lib/ai/tools/__tests__/drive-tools.test.ts

@@ -1,31 +1,18 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 
-// Mock database and dependencies
+// Mock database - only mock what's actually used in tests
 vi.mock('@pagespace/db', () => ({
   db: {
-    select: vi.fn().mockReturnThis(),
-    from: vi.fn().mockReturnThis(),
-    where: vi.fn(),
-    insert: vi.fn().mockReturnThis(),
-    values: vi.fn().mockReturnThis(),
-    returning: vi.fn(),
-    update: vi.fn().mockReturnThis(),
-    set: vi.fn().mockReturnThis(),
     query: {
       drives: { findFirst: vi.fn() },
-      driveMembers: { findFirst: vi.fn() },
     },
   },
   drives: { id: 'id', ownerId: 'ownerId' },
-  driveMembers: { driveId: 'driveId' },
   eq: vi.fn(),
   and: vi.fn(),
-  or: vi.fn(),
-  isNull: vi.fn(),
 }));
 
 vi.mock('@pagespace/lib/server', () => ({
-  getUserDriveAccess: vi.fn(),
   loggers: {
     ai: {
       child: vi.fn(() => ({
@@ -53,6 +40,17 @@ import type { ToolExecutionContext } from '../../core';
 
 const mockDb = vi.mocked(db);
 
+/**
+ * @scaffold - happy path coverage deferred
+ *
+ * These tests cover authentication and validation error paths.
+ * Happy path tests (list_drives returning data, create_drive success, etc.)
+ * are deferred because they require either:
+ * - A repository seam to avoid query-builder chain mocking, OR
+ * - Integration tests against a real database
+ *
+ * TODO: Add DriveRepository seam and test happy paths against it.
+ */
 describe('drive-tools', () => {
   beforeEach(() => {
     vi.clearAllMocks();
@@ -72,7 +70,6 @@ describe('drive-tools', () => {
         driveTools.list_drives.execute({}, context)
       ).rejects.toThrow('User authentication required');
     });
-
   });
 
   describe('create_drive', () => {

apps/web/src/lib/ai/tools/__tests__/page-write-tools.test.ts

@@ -178,10 +178,11 @@ describe('page-write-tools', () => {
         context
       );
 
+      // Observable outcomes prove the operation succeeded
       expect(result.success).toBe(true);
       expect(result.linesReplaced).toBe(1);
+      // Verify permission check was called with correct arguments
       expect(mockCanUserEditPage).toHaveBeenCalledWith('user-123', 'page-1');
-      expect(mockDb.update).toHaveBeenCalled();
     });
   });
 

apps/web/src/lib/ai/tools/__tests__/search-tools.test.ts

@@ -1,29 +1,8 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 
-// Mock database and dependencies
-vi.mock('@pagespace/db', () => ({
-  db: {
-    select: vi.fn().mockReturnThis(),
-    selectDistinct: vi.fn().mockReturnThis(),
-    from: vi.fn().mockReturnThis(),
-    where: vi.fn(),
-    limit: vi.fn(),
-    query: {
-      pages: { findFirst: vi.fn() },
-      drives: { findFirst: vi.fn() },
-    },
-  },
-  pages: { id: 'id', driveId: 'driveId', content: 'content', title: 'title' },
-  drives: { id: 'id', isTrashed: 'isTrashed' },
-  eq: vi.fn(),
-  and: vi.fn(),
-  sql: vi.fn(),
-  inArray: vi.fn(),
-}));
-
+// Mock only the boundary we actually test
 vi.mock('@pagespace/lib/server', () => ({
   getUserDriveAccess: vi.fn(),
-  getUserAccessiblePagesInDriveWithDetails: vi.fn(),
 }));
 
 import { searchTools } from '../search-tools';
@@ -32,6 +11,23 @@ import type { ToolExecutionContext } from '../../core';
 
 const mockGetUserDriveAccess = vi.mocked(getUserDriveAccess);
 
+/**
+ * @scaffold - happy path coverage deferred
+ *
+ * These tests cover authentication and authorization error paths.
+ * Happy path tests (actual search results, filtering, pagination) are deferred
+ * because they require either:
+ * - A SearchRepository seam to avoid complex DB iteration mocking, OR
+ * - Integration tests against a real database with seeded content
+ *
+ * The search logic involves iterating over pages and matching patterns,
+ * which is impractical to mock without coupling to implementation details.
+ *
+ * TODO: Add SearchService seam with methods like:
+ * - searchByRegex(driveId, pattern, options): SearchResult[]
+ * - searchByGlob(driveId, pattern, options): SearchResult[]
+ * Then test happy paths against that seam.
+ */
 describe('search-tools', () => {
   beforeEach(() => {
     vi.clearAllMocks();
@@ -70,7 +66,6 @@ describe('search-tools', () => {
         )
       ).rejects.toThrow("You don't have access to this drive");
     });
-
   });
 
   describe('glob_search', () => {
@@ -104,7 +99,6 @@ describe('search-tools', () => {
         )
       ).rejects.toThrow("You don't have access to this drive");
     });
-
   });
 
   describe('multi_drive_search', () => {

apps/web/src/lib/mcp/__tests__/mcp-bridge.test.ts

@@ -92,8 +92,7 @@ describe('MCPBridge', () => {
       // Start the request but don't await it yet
       const promise = bridge.executeTool('user-123', 'my-server', 'my-tool', { foo: 'bar' });
 
-      // Verify send was called
-      expect(mockWs.send).toHaveBeenCalledTimes(1);
+      // Verify send was called with correct payload
       const sentData = JSON.parse((mockWs.send as ReturnType<typeof vi.fn>).mock.calls[0][0]);
       expect(sentData.type).toBe('tool_execute');
       expect(sentData.serverName).toBe('my-server');