Skip to content

Overhaul error handling and test patterns #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions .cursor/rules/errors.mdc
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
---
description:
globs: **/errors.ts
alwaysApply: false
---
# Error Flow

```mermaid
graph TD
subgraph Core_Logic
FS[FileSystemService: e.g., FileReadError] --> TM[TaskManager: Throws App Errors, e.g., ProjectNotFound, TaskNotDone]
TM -->|App Error with code ERR_xxxx| CLI_Handler["cli.ts Command Handler"]
TM -->|App Error with code ERR_xxxx| ToolExec["toolExecutors.ts: execute"]
end

subgraph CLI_Path
CLI_Handler -->|App Error| CLI_Catch["cli.ts catch block"]
CLI_Catch -->|Error Object| FormatCLI["client errors.ts formatCliError"]
FormatCLI -->|"Error [ERR_xxxx]: message"| ConsoleOut["console.error Output"]
end

subgraph MCP_Server_Path
subgraph Validation_Layer
ToolExecVal["toolExecutors.ts Validation"] -->|App Error, e.g., MissingParameter| ExecToolErrHandler
end

subgraph App_Execution
ToolExec -->|App Error with code ERR_xxxx| ExecToolErrHandler["tools.ts executeToolAndHandleErrors catch block"]
ExecToolErrHandler -->|Map AppError to Protocol Error or Tool Result| ErrorMapping
ErrorMapping -->|"If validation error (ERR_1xxx)"| McpError["Create McpError with appropriate ErrorCode"]
ErrorMapping -->|"If business logic error (ERR_2xxx+)"| FormatResult["Format as isError true result"]

McpError -->|Throw| SDKHandler["server index.ts SDK Handler"]
FormatResult -->|"{ content: [{ text: Error [ERR_xxxx]: message }], isError: true }"| SDKHandler
end

SDKHandler -- Protocol Error --> SDKFormatError["SDK Formats as JSON-RPC Error Response"]
SDKHandler -- Tool Result --> SDKFormatResult["SDK Formats as JSON-RPC Success Response"]

SDKFormatError -->|"{ error: { code: -326xx, message: ... } }"| MCPClient["MCP Client"]
SDKFormatResult -->|"{ result: { content: [...], isError: true } }"| MCPClient
end
```

**Explanation of Updated Error Flow and Transformations:**

Errors are consistently through a unified `AppError` system:

1. **Validation Errors** (`ERR_1xxx` series)
- Used for validation issues (e.g., MissingParameter, InvalidArgument)
- Thrown by tool executors during parameter validation
- Mapped to protocol-level McpErrors in `executeToolAndHandleErrors`

2. **Business Logic Errors** (`ERR_2xxx` and higher)
- Used for all business logic and application-specific errors
- Include specific error codes
- Returned as serialized CallToolResults with `isError: true`
6 changes: 6 additions & 0 deletions .cursor/rules/general.mdc
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
description:
globs:
alwaysApply: true
---
Work step-by-step. If presented with an implementation plan, implement the plan exactly. If the plan presents more than one implementation option, consult with the human user to decide between options. If you are tempted to embellish or imporve upon the plan, consult with the human user. Always complete the current task and wait for human review before proceeding to the next task.
195 changes: 1 addition & 194 deletions .cursor/rules/tests.mdc
Original file line number Diff line number Diff line change
@@ -3,197 +3,4 @@ description: Writing unit tests with `jest`
globs: tests/**/*
alwaysApply: false
---
# Testing Guidelines for TypeScript + ES Modules + Jest

This guide contains cumulative in-context learnings about working with this project's testing stack.

## Unit vs. Integration Tests

**Never Mix Test Types**: Separate integration tests from unit tests into different files:
- Simple unit tests without mocks for validating rules (like state transitions)
- Integration tests with mocks for filesystem and external dependencies

## File Path Handling in Tests

1. **Environment Variables**:
- Use `process.env.TASK_MANAGER_FILE_PATH` for configuring file paths in tests
- Set this in `beforeEach` and clean up in `afterEach`:
```typescript
beforeEach(async () => {
tempDir = path.join(os.tmpdir(), `test-${Date.now()}`);
await fs.mkdir(tempDir, { recursive: true });
tasksFilePath = path.join(tempDir, "test-tasks.json");
process.env.TASK_MANAGER_FILE_PATH = tasksFilePath;
});

afterEach(async () => {
await fs.rm(tempDir, { recursive: true, force: true });
delete process.env.TASK_MANAGER_FILE_PATH;
});
```

2. **Temporary Files**:
- Create unique temp directories for each test run
- Use `os.tmpdir()` for platform-independent temp directories
- Include timestamps in directory names to prevent conflicts
- Always clean up temp files in `afterEach`

## Jest ESM Mocking, Step-by-Step

1. **Type-Only Import:**
Import types for static analysis without actually executing the module code:
```typescript
import type { MyService as MyServiceType } from 'path/to/MyService.js';
import type { readFile as ReadFileType } from 'node:fs/promises';
```

2. **Register Mock:**
Use `jest.unstable_mockModule` to replace the real module:
```typescript
jest.unstable_mockModule('node:fs/promises', () => ({
__esModule: true,
readFile: jest.fn(),
}));
```

3. **Set Default Mock Implementations, Then Dynamically Import Modules:**
You must dynamically import the modules to be mocked and/or tested *after* registering mocks and setting any mock implementations. This ensures that when `MyService` attempts to import `node:fs/promises`, it gets your mocked version. Depending how you want to scope your mock implementations, you can do this in `beforeAll`, `beforeEach`, or at the top of each test.
```typescript
let MyService: typeof MyServiceType;
let readFile: jest.MockedFunction<ReadFileType>;

beforeAll(async () => {
const fsPromisesMock = await import('node:fs/promises');
readFile = fsPromisesMock.readFile as jest.MockedFunction<ReadFileType>;

// Set default implementation
readFile.mockResolvedValue('default mocked content');

const serviceModule = await import('path/to/MyService.js');
MyService = serviceModule.MyService;
});
```

4. **Setup in `beforeEach`:**
Reset mocks and set default behaviors before each test:
```typescript
beforeEach(() => {
jest.clearAllMocks();
readFile.mockResolvedValue('');
});
```

5. **Write a Test:**
Now you can test your service with the mocked `readFile`:
```typescript
describe('MyService', () => {
let myServiceInstance: MyServiceType;

beforeEach(() => {
myServiceInstance = new MyService('somePath');
});

it('should do something', async () => {
readFile.mockResolvedValueOnce('some data');
const result = await myServiceInstance.someMethod();
expect(result).toBe('expected result');
expect(readFile).toHaveBeenCalledWith('somePath', 'utf-8');
});
});
```

### Mocking a Class with Methods

If you have a class `MyClass` that has both instance methods and static methods, you can mock it in an **ES Modules + TypeScript** setup using the same pattern. For instance:

```typescript
// 1. Create typed jest mock functions using the original types
type InitResult = { data: string };

const mockInit = jest.fn() as jest.MockedFunction<MyClass['init']>;
const mockDoWork = jest.fn() as jest.MockedFunction<MyClass['doWork']>;
const mockStaticHelper = jest.fn() as jest.MockedFunction<typeof MyClass.staticHelper>;

// 2. Use jest.unstable_mockModule with an ES6 class in the factory
jest.unstable_mockModule('path/to/MyClass.js', () => {
class MockMyClass {
// Instance methods
init = mockInit;
doWork = mockDoWork;

// Static method
static staticHelper = mockStaticHelper;
}

return {
__esModule: true,
MyClass: MockMyClass, // same name/structure as real export
};
});

// 3. Import your class after mocking
let MyClass: typeof import('path/to/MyClass.js')['MyClass'];

beforeAll(async () => {
const myClassModule = await import('path/to/MyClass.js');
MyClass = myClassModule.MyClass;
});

// 4. Write tests and reset mocks
beforeEach(() => {
jest.clearAllMocks();
mockInit.mockResolvedValue({ data: 'default' });
mockStaticHelper.mockReturnValue(42);
});

describe('MyClass', () => {
it('should call init', async () => {
const instance = new MyClass();
const result = await instance.init();
expect(result).toEqual({ data: 'default' });
expect(mockInit).toHaveBeenCalledTimes(1);
});

it('should call the static helper', () => {
const val = MyClass.staticHelper();
expect(val).toBe(42);
expect(mockStaticHelper).toHaveBeenCalledTimes(1);
});
});
```

### Best Practice: **Type** Your Mocked Functions

By default, `jest.fn()` is very generic and doesn't enforce parameter or return types. This can cause TypeScript errors like:

> `Argument of type 'undefined' is not assignable to parameter of type 'never'`

or

> `Type 'Promise<SomeType>' is not assignable to type 'FunctionLike'`

To avoid these, **use the original type with `jest.MockedFunction`**. For example, if your real function is:

```typescript
async function loadStuff(id: string): Promise<string[]> {
// ...
}
```

then you should type the mock as:

```typescript
const mockLoadStuff = jest.fn() as jest.MockedFunction<typeof loadStuff>;
```

For class methods, use the class type to get the method signature:

```typescript
const mockClassMethod = jest.fn() as jest.MockedFunction<YourClass['classMethod']>;
```

This helps TypeScript catch mistakes if you:
- call the function with the wrong argument types
- use `mockResolvedValue` with the wrong shape

Once typed properly, your `mockResolvedValue(...)`, `mockImplementation(...)`, etc. calls will be fully type-safe.
Make use of the helpers in tests/mcp/test-helpers.ts.
1 change: 0 additions & 1 deletion .github/workflows/npm-publish.yml
Original file line number Diff line number Diff line change
@@ -13,7 +13,6 @@ jobs:
node-version: 'latest'
- run: npm ci
- run: npm install -g tsx
- run: npm run build
- run: npm test

publish:
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
@@ -40,14 +40,14 @@ This will show the available commands and options.
The task manager supports multiple LLM providers for generating project plans. You can configure one or more of the following environment variables depending on which providers you want to use:

- `OPENAI_API_KEY`: Required for using OpenAI models (e.g., GPT-4)
- `GEMINI_API_KEY`: Required for using Google's Gemini models
- `GOOGLE_GENERATIVE_AI_API_KEY`: Required for using Google's Gemini models
- `DEEPSEEK_API_KEY`: Required for using Deepseek models

To generate project plans using the CLI, set these environment variables in your shell:

```bash
export OPENAI_API_KEY="your-api-key"
export GEMINI_API_KEY="your-api-key"
export GOOGLE_GENERATIVE_AI_API_KEY="your-api-key"
export DEEPSEEK_API_KEY="your-api-key"
```

@@ -61,7 +61,7 @@ Or you can include them in your MCP client configuration to generate project pla
"args": ["-y", "taskqueue-mcp"],
"env": {
"OPENAI_API_KEY": "your-api-key",
"GEMINI_API_KEY": "your-api-key",
"GOOGLE_GENERATIVE_AI_API_KEY": "your-api-key",
"DEEPSEEK_API_KEY": "your-api-key"
}
}
6 changes: 6 additions & 0 deletions babel.config.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
presets: [
['@babel/preset-env', { targets: { node: 'current' } }],
'@babel/preset-typescript',
],
};
9 changes: 1 addition & 8 deletions jest.config.cjs
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
const { createDefaultEsmPreset } = require('ts-jest');

const presetConfig = createDefaultEsmPreset({
useESM: true,
});

module.exports = {
...presetConfig,
testEnvironment: 'node',
moduleNameMapper: {
'^(\\.{1,2}/.*)\\.js$': '$1',
'^(\\.{1,2}/.*)\\.js$': '$1'
},
modulePathIgnorePatterns: ['<rootDir>/dist/'],
// Force Jest to exit after all tests have completed
Loading