Passing tests

Author: chriscarrollsmith

src/client/cli.ts

@@ -554,7 +554,12 @@ program
       }
     } catch (err: unknown) {
       if (err instanceof Error) {
-        console.error(chalk.yellow(`Warning: ${err.message}`));
+        // Check for API key related errors and format them appropriately
+        if (err.message.includes('API key') || err.message.includes('authentication') || err.message.includes('unauthorized')) {
+          console.error(chalk.red(`Error: ${err.message}`));
+        } else {
+          console.error(chalk.yellow(`Warning: ${err.message}`));
+        }
       } else {
         const normalized = normalizeError(err);
         console.error(chalk.red(formatCliError(normalized)));

src/server/FileSystemService.ts

@@ -12,6 +12,9 @@ export interface InitializedTaskData {
 
 export class FileSystemService {
   private filePath: string;
+  // Simple in-memory queue to prevent concurrent file operations
+  private operationInProgress: boolean = false;
+  private operationQueue: (() => void)[] = [];
 
   constructor(filePath: string) {
     this.filePath = filePath;
@@ -37,21 +40,74 @@ export class FileSystemService {
     }
   }
 
+  /**
+   * Queue a file operation to prevent concurrent access
+   * @param operation The operation to perform
+   * @returns Promise that resolves when the operation completes
+   */
+  private async queueOperation<T>(operation: () => Promise<T>): Promise<T> {
+    // If another operation is in progress, wait for it to complete
+    if (this.operationInProgress) {
+      return new Promise<T>((resolve, reject) => {
+        this.operationQueue.push(() => {
+          this.executeOperation(operation).then(resolve).catch(reject);
+        });
+      });
+    }
+
+    return this.executeOperation(operation);
+  }
+
+  /**
+   * Execute a file operation with mutex protection
+   * @param operation The operation to perform
+   * @returns Promise that resolves when the operation completes
+   */
+  private async executeOperation<T>(operation: () => Promise<T>): Promise<T> {
+    this.operationInProgress = true;
+    try {
+      return await operation();
+    } finally {
+      this.operationInProgress = false;
+      // Process the next operation in the queue, if any
+      const nextOperation = this.operationQueue.shift();
+      if (nextOperation) {
+        nextOperation();
+      }
+    }
+  }
+
   /**
    * Loads and initializes task data from the JSON file
    */
   public async loadAndInitializeTasks(): Promise<InitializedTaskData> {
-    const data = await this.loadTasks();
-    const { maxProjectId, maxTaskId } = this.calculateMaxIds(data);
-    
-    return {
-      data,
-      maxProjectId,
-      maxTaskId
-    };
+    return this.queueOperation(async () => {
+      const data = await this.loadTasks();
+      const { maxProjectId, maxTaskId } = this.calculateMaxIds(data);
+      
+      return {
+        data,
+        maxProjectId,
+        maxTaskId
+      };
+    });
   }
 
-  private calculateMaxIds(data: TaskManagerFile): { maxProjectId: number; maxTaskId: number } {
+  /**
+   * Explicitly reloads task data from the disk
+   * This is useful when the file may have been changed by another process
+   * @returns The latest task data from disk
+   */
+  public async reloadTasks(): Promise<TaskManagerFile> {
+    return this.queueOperation(async () => {
+      return this.loadTasks();
+    });
+  }
+
+  /**
+   * Calculate max IDs from task data
+   */
+  public calculateMaxIds(data: TaskManagerFile): { maxProjectId: number; maxTaskId: number } {
     const allTaskIds: number[] = [];
     const allProjectIds: number[] = [];
 
@@ -89,32 +145,35 @@ export class FileSystemService {
   }
 
   /**
-   * Saves task data to the JSON file
+   * Saves task data to the JSON file with an in-memory mutex to prevent concurrent writes
    */
   public async saveTasks(data: TaskManagerFile): Promise<void> {
-    try {
-      // Ensure directory exists before writing
-      const dir = dirname(this.filePath);
-      await mkdir(dir, { recursive: true });
-      
-      await writeFile(
-        this.filePath,
-        JSON.stringify(data, null, 2),
-        "utf-8"
-      );
-    } catch (error) {
-      if (error instanceof Error && error.message.includes("EROFS")) {
+    return this.queueOperation(async () => {
+      try {
+        // Ensure directory exists before writing
+        const dir = dirname(this.filePath);
+        await mkdir(dir, { recursive: true });
+        
+        // Write to the file
+        await writeFile(
+          this.filePath,
+          JSON.stringify(data, null, 2),
+          "utf-8"
+        );
+      } catch (error) {
+        if (error instanceof Error && error.message.includes("EROFS")) {
+          throw createError(
+            ErrorCode.ReadOnlyFileSystem,
+            "Cannot save tasks: read-only file system",
+            { originalError: error }
+          );
+        }
         throw createError(
-          ErrorCode.ReadOnlyFileSystem,
-          "Cannot save tasks: read-only file system",
+          ErrorCode.FileWriteError,
+          "Failed to save tasks file",
           { originalError: error }
         );
       }
-      throw createError(
-        ErrorCode.FileWriteError,
-        "Failed to save tasks file",
-        { originalError: error }
-      );
-    }
+    });
   }
 } 

src/server/TaskManager.ts

@@ -32,6 +32,19 @@ export class TaskManager {
     await this.initialized;
   }
 
+  /**
+   * Reloads data from disk
+   * This is helpful when the task file might have been modified by another process
+   * Used internally before read operations
+   */
+  public async reloadFromDisk(): Promise<void> {
+    const data = await this.fileSystemService.reloadTasks();
+    this.data = data;
+    const { maxProjectId, maxTaskId } = this.fileSystemService.calculateMaxIds(data);
+    this.projectCounter = maxProjectId;
+    this.taskCounter = maxTaskId;
+  }
+
   private async saveTasks() {
     await this.fileSystemService.saveTasks(this.data);
   }
@@ -43,6 +56,8 @@ export class TaskManager {
     autoApprove?: boolean
   ) {
     await this.ensureInitialized();
+    // Reload before creating to ensure counters are up-to-date
+    await this.reloadFromDisk();
     this.projectCounter += 1;
     const projectId = `proj-${this.projectCounter}`;
 
@@ -221,6 +236,9 @@ export class TaskManager {
 
   public async getNextTask(projectId: string): Promise<StandardResponse> {
     await this.ensureInitialized();
+    // Reload from disk to ensure we have the latest data
+    await this.reloadFromDisk();
+    
     const proj = this.data.projects.find((p) => p.projectId === projectId);
     if (!proj) {
       throw createError(
@@ -267,6 +285,8 @@ export class TaskManager {
 
   public async approveTaskCompletion(projectId: string, taskId: string) {
     await this.ensureInitialized();
+    // Reload before modifying
+    await this.reloadFromDisk();
     const proj = this.data.projects.find((p) => p.projectId === projectId);
     if (!proj) {
       throw createError(
@@ -307,6 +327,8 @@ export class TaskManager {
 
   public async approveProjectCompletion(projectId: string) {
     await this.ensureInitialized();
+    // Reload before modifying
+    await this.reloadFromDisk();
     const proj = this.data.projects.find((p) => p.projectId === projectId);
     if (!proj) {
       throw createError(
@@ -349,6 +371,9 @@ export class TaskManager {
 
   public async openTaskDetails(taskId: string) {
     await this.ensureInitialized();
+    // Reload from disk to ensure we have the latest data
+    await this.reloadFromDisk();
+    
     for (const proj of this.data.projects) {
       const target = proj.tasks.find((t) => t.id === taskId);
       if (target) {
@@ -376,6 +401,8 @@ export class TaskManager {
 
   public async listProjects(state?: TaskState) {
     await this.ensureInitialized();
+    // Reload from disk to ensure we have the latest data
+    await this.reloadFromDisk();
 
     let filteredProjects = [...this.data.projects];
 
@@ -409,6 +436,8 @@ export class TaskManager {
 
   public async listTasks(projectId?: string, state?: TaskState) {
     await this.ensureInitialized();
+    // Reload from disk to ensure we have the latest data
+    await this.reloadFromDisk();
 
     // If projectId is provided, verify the project exists
     if (projectId) {
@@ -462,6 +491,8 @@ export class TaskManager {
     tasks: { title: string; description: string; toolRecommendations?: string; ruleRecommendations?: string }[]
   ) {
     await this.ensureInitialized();
+    // Reload before modifying
+    await this.reloadFromDisk();
     const proj = this.data.projects.find((p) => p.projectId === projectId);
     if (!proj) {
       throw createError(
@@ -512,6 +543,8 @@ export class TaskManager {
     }
   ) {
     await this.ensureInitialized();
+    // Reload before modifying
+    await this.reloadFromDisk();
     const project = this.data.projects.find((p) => p.projectId === projectId);
     if (!project) {
       throw createError(
@@ -542,6 +575,8 @@ export class TaskManager {
 
   public async deleteTask(projectId: string, taskId: string) {
     await this.ensureInitialized();
+    // Reload before modifying
+    await this.reloadFromDisk();
     const proj = this.data.projects.find((p) => p.projectId === projectId);
     if (!proj) {
       throw createError(
@@ -581,6 +616,9 @@ export class TaskManager {
     tasks: Task[];
   }>> {
     await this.ensureInitialized();
+    // Reload from disk to ensure we have the latest data
+    await this.reloadFromDisk();
+    
     const project = this.data.projects.find(p => p.projectId === projectId);
     if (!project) {
       throw createError(

tests/integration/TaskManager.test.ts

@@ -14,7 +14,7 @@ describe('TaskManager Integration', () => {
     tempDir = path.join(os.tmpdir(), `task-manager-integration-test-${Date.now()}-${Math.floor(Math.random() * 10000)}`);
     await fs.mkdir(tempDir, { recursive: true });
     testFilePath = path.join(tempDir, 'test-tasks.json');
-    
+
     // Initialize the server with the test file path
     server = new TaskManager(testFilePath);
   });
@@ -171,14 +171,20 @@ describe('TaskManager Integration', () => {
     const taskId2 = createResult.data.tasks[1].id;
 
     // 2. Try to approve project before tasks are done (should fail)
-    await expect(server.approveProjectCompletion(projectId)).rejects.toThrow('Not all tasks are done');
+    await expect(server.approveProjectCompletion(projectId)).rejects.toMatchObject({
+      code: 'ERR_3003',
+      message: 'Not all tasks are done'
+    });
 
     // 3. Mark tasks as done
     await server.updateTask(projectId, taskId1, { status: 'done', completedDetails: 'Task 1 completed details' });
     await server.updateTask(projectId, taskId2, { status: 'done', completedDetails: 'Task 2 completed details' });
 
     // 4. Try to approve project before tasks are approved (should fail)
-    await expect(server.approveProjectCompletion(projectId)).rejects.toThrow('Not all done tasks are approved');
+    await expect(server.approveProjectCompletion(projectId)).rejects.toMatchObject({
+      code: 'ERR_3004',
+      message: 'Not all done tasks are approved'
+    });
 
     // 5. Approve tasks
     await server.approveTaskCompletion(projectId, taskId1);
@@ -195,7 +201,10 @@ describe('TaskManager Integration', () => {
     expect(completedProject).toBeDefined();
 
     // 8. Try to approve again (should fail)
-    await expect(server.approveProjectCompletion(projectId)).rejects.toThrow('Project is already completed');
+    await expect(server.approveProjectCompletion(projectId)).rejects.toMatchObject({
+      code: 'ERR_3001',
+      message: 'Project is already completed'
+    });
   });
 
   it("should handle complex project and task state transitions", async () => {
@@ -250,8 +259,6 @@ describe('TaskManager Integration', () => {
   });
 
   it("should handle tool/rule recommendations end-to-end", async () => {
-    const server = new TaskManager(testFilePath);
-    
     // Create a project with tasks that have recommendations
     const response = await server.createProject("Test Project", [
       {
@@ -380,10 +387,17 @@ describe('TaskManager Integration', () => {
     expect(projectState.data.projects.find(p => p.projectId === project.projectId)).toBeDefined();
   });
 
-  it("should handle multiple concurrent server instances", async () => {
-    // Create two server instances pointing to the same file
-    const server1 = new TaskManager(testFilePath);
-    const server2 = new TaskManager(testFilePath);
+  it("multiple concurrent server instances should synchronize data", async () => {
+    // Create a unique file path just for this test
+    const uniqueTestFilePath = path.join(tempDir, `concurrent-test-${Date.now()}.json`);
+
+    // Create two server instances that would typically be in different processes
+    const server1 = new TaskManager(uniqueTestFilePath);
+    const server2 = new TaskManager(uniqueTestFilePath);
+
+    // Ensure both servers are fully initialized
+    await server1["initialized"];
+    await server2["initialized"];
 
     // Create a project with server1
     const projectResponse = await server1.createProject(
@@ -411,7 +425,7 @@ describe('TaskManager Integration', () => {
     });
     await server1.approveTaskCompletion(project.projectId, project.tasks[0].id);
 
-    // Verify completion with server2
+    // Verify completion with server2 (it should automatically reload latest data)
     const completedTasks = await server2.listTasks(project.projectId, "completed");
     expect(completedTasks.status).toBe('success');
     expect(completedTasks.data.tasks!.length).toBe(1);
@@ -420,10 +434,9 @@ describe('TaskManager Integration', () => {
     const completionResult = await server2.approveProjectCompletion(project.projectId);
     expect(completionResult.status).toBe('success');
 
-    // Verify with server1
+    // Verify with server1 (it should automatically reload latest data)
     const projectState = await server1.listProjects("completed");
     expect(projectState.status).toBe('success');
     expect(projectState.data.projects.find(p => p.projectId === project.projectId)).toBeDefined();
   });
 });
-