feat: connect get-task and get-tasks to remote#1346
Conversation
🦋 Changeset detectedLatest commit: f0caca9 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughMigrates task tools from mcp-server JS into apps/mcp TypeScript and re-exports them from apps/mcp; removes legacy direct-functions/tools; adds logger callback support and a logger getter in tm-core; adds IStorage.getCurrentBriefName and implements storage-aware tag/brief-name propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MCP as MCP Server
participant Registry as Tool Registry
participant Tools as apps/mcp Task Tools
participant TmCore as TmCore
participant Storage as Storage Adapter
participant Logger as Logger
User->>MCP: call get_tasks / get_task
MCP->>Registry: dispatch tool
Registry->>Tools: invoke registered tool
Tools->>Tools: validate params (Zod)
Tools->>TmCore: create TmCore instance (may pass loggerConfig)
TmCore->>Logger: createLogger / provide logger
Tools->>TmCore: request list/show tasks (tag/filter)
TmCore->>Storage: list/fetch tasks (requests briefName if API)
Storage-->>TmCore: tasks + storageType (+ briefName if API)
TmCore-->>Tools: task data + metadata (tag may be briefName)
Tools->>Tools: handleApiResult(using providedTag || computed tag)
Tools-->>MCP: standardized API response
MCP-->>User: deliver response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
f4d73b1 to
333af2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
apps/mcp/src/tools/tasks/get-task.tool.ts (3)
27-36: Remove unused parameters or wire them throughSchema exposes file and complexityReport but they are unused in execute; this confuses clients and bloats schema.
Either plumb into tm-core or remove for now:
- file: z - .string() - .optional() - .describe('Path to the tasks file relative to project root'), - complexityReport: z - .string() - .optional() - .describe( - 'Path to the complexity report file (relative to project root or absolute)' - ),Based on learnings.
81-87: Avoid mutating result.task; normalize status comparisonMutating subtasks in place can leak side effects. Also, status matching is case-sensitive.
Use a copy and case-normalize:
- if (status && result.task.subtasks) { - const statusFilters = status.split(',').map((s) => s.trim()); - result.task.subtasks = result.task.subtasks.filter((st) => - statusFilters.includes(st.status) - ); - } + if (status && result.task.subtasks) { + const statusFilters = status + .split(',') + .map((s) => s.trim().toLowerCase()); + const filteredSubtasks = result.task.subtasks.filter((st) => + statusFilters.includes(String(st.status).toLowerCase()) + ); + result.task = { ...result.task, subtasks: filteredSubtasks }; + }
73-91: Parallelize multi-ID fetchesCurrent for/await is sequential. Use Promise.all for latency-bound remote calls.
- const taskIds = id.split(',').map((tid) => tid.trim()); - const tasks: Task[] = []; - - for (const taskId of taskIds) { - const result = await tmCore.tasks.get(taskId, tag); - if (result.task) { - // filtering... - tasks.push(result.task); - } - } + const taskIds = id.split(',').map((tid) => tid.trim()); + const results = await Promise.all( + taskIds.map((taskId) => tmCore.tasks.get(taskId, tag)) + ); + const tasks: Task[] = []; + for (const result of results) { + if (!result.task) continue; + // filtering (as in previous suggestion)... + tasks.push(result.task); + }packages/tm-core/src/common/logger/logger.spec.ts (1)
47-60: Strengthen “callback instead of console” assertionAlso assert console methods were not called to prevent regressions.
it('should call callback instead of console when logCallback is provided', () => { + const consoleSpy = vi.spyOn(console, 'log'); const mockCallback = vi.fn(); @@ logger.info('Test message'); - expect(mockCallback).toHaveBeenCalledWith( + expect(mockCallback).toHaveBeenCalledWith( 'info', expect.stringContaining('Test message') ); + expect(consoleSpy).not.toHaveBeenCalled(); + consoleSpy.mockRestore(); });apps/mcp/src/tools/tasks/get-tasks.tool.ts (3)
18-21: Consider making projectRoot optional to rely on HOF/env/sessionHOF already resolves project root. Making it optional reduces friction for clients and matches get_task’s intention.
- projectRoot: z - .string() - .describe('The directory of the project. Must be an absolute path.'), + projectRoot: z + .string() + .optional() + .describe('Project root (absolute). Optional: resolved from env/session when omitted'),Based on learnings.
74-81: Normalize status filter; avoid brittle castDo case-insensitive match and avoid casting without validation.
- const filter = - status && status !== 'all' + const filter = + status && status.toLowerCase() !== 'all' ? { - status: status - .split(',') - .map((s: string) => s.trim() as TaskStatus) + status: status + .split(',') + .map((s: string) => s.trim().toLowerCase() as TaskStatus) } : undefined;Optionally, add a zod enum for TaskStatus to validate inputs.
31-40: Remove unused fields or wire throughfile and complexityReport aren’t used in execute. Drop them or pass to tm-core if supported to avoid API drift.
- file: z - .string() - .optional() - .describe('Path to the tasks file (relative to project root or absolute)'), - complexityReport: z - .string() - .optional() - .describe( - 'Path to the complexity report file (relative to project root or absolute)' - ),mcp-server/src/tools/tool-registry.js (1)
1-4: Comment drift: tool count inconsistentHeader says 36 tools; later comment says 44. Avoid hard-coded counts to prevent future drift.
- * Tool Registry Object Structure - Maps all 36 tool names to registration functions + * Tool Registry - Maps tool names to registration functions @@ - * Comprehensive tool registry mapping all 44 tool names to their registration functions + * Comprehensive tool registry mapping tool names to their registration functionsAlso applies to: 56-58
packages/tm-core/src/tm-core.ts (1)
54-75: Doc snippet import path may be wrongExample uses import { LogLevel } from '@tm/core/logger'. If logger types aren’t published at that subpath, prefer importing from '@tm/core' main entry.
- import { LogLevel } from '@tm/core/logger'; + import { LogLevel } from '@tm/core';packages/tm-core/src/common/logger/logger.ts (2)
219-231: Policy: should callback errors bubble or be contained?Currently, exceptions from logCallback bubble up. If you prefer “graceful” handling, wrap in try/catch and fallback to console.warn; otherwise keep current behavior and adjust tests wording.
Example (if you choose to contain errors):
- if (this.isLogObject(this.config.logCallback)) { - const method = levelName.toLowerCase() as keyof LogObject; - if (method in this.config.logCallback) { - this.config.logCallback[method](formatted); - } - } else { - // Otherwise it's a function callback - this.config.logCallback(levelName.toLowerCase(), formatted); - } + try { + if (this.isLogObject(this.config.logCallback)) { + const method = levelName.toLowerCase() as keyof LogObject; + if (method in this.config.logCallback) { + this.config.logCallback[method](formatted); + } + } else { + this.config.logCallback(levelName.toLowerCase(), formatted); + } + } catch (e) { + // Optional: surface minimally without crashing + console.warn(`Logger callback error: ${(e as Error).message}`); + }
142-175: Optional: disable ANSI colors when using callbacksMany consumers (MCP) prefer plain text; consider auto-disabling colors when logCallback is set.
this.config = { ...Logger.DEFAULT_CONFIG, ...config, ...envConfig }; + if (this.config.logCallback) { + this.config.colors = false; + }Also applies to: 176-191
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/mcp/src/index.ts(1 hunks)apps/mcp/src/shared/utils.ts(1 hunks)apps/mcp/src/tools/tasks/get-task.tool.ts(1 hunks)apps/mcp/src/tools/tasks/get-tasks.tool.ts(1 hunks)apps/mcp/src/tools/tasks/index.ts(1 hunks)mcp-server/src/core/direct-functions/list-tasks.js(0 hunks)mcp-server/src/core/direct-functions/show-task.js(0 hunks)mcp-server/src/core/task-master-core.js(0 hunks)mcp-server/src/tools/get-task.js(0 hunks)mcp-server/src/tools/get-tasks.js(0 hunks)mcp-server/src/tools/tool-registry.js(3 hunks)packages/tm-core/src/common/interfaces/storage.interface.ts(1 hunks)packages/tm-core/src/common/logger/index.ts(1 hunks)packages/tm-core/src/common/logger/logger.spec.ts(1 hunks)packages/tm-core/src/common/logger/logger.ts(5 hunks)packages/tm-core/src/modules/storage/adapters/api-storage.ts(1 hunks)packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts(1 hunks)packages/tm-core/src/modules/tasks/services/task-service.ts(2 hunks)packages/tm-core/src/tm-core.ts(6 hunks)
💤 Files with no reviewable changes (5)
- mcp-server/src/core/task-master-core.js
- mcp-server/src/core/direct-functions/show-task.js
- mcp-server/src/core/direct-functions/list-tasks.js
- mcp-server/src/tools/get-task.js
- mcp-server/src/tools/get-tasks.js
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/git_workflow.mdc)
**/*.{test,spec}.{js,ts,jsx,tsx}: Create a test file and ensure all tests pass when all subtasks are complete; commit tests if added or modified
When all subtasks are complete, run final testing using the appropriate test runner (e.g., npm test, jest, or manual testing)
Files:
packages/tm-core/src/common/logger/logger.spec.ts
**/*.{test,spec}.*
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Test files should follow naming conventions: .test., .spec., or _test. depending on the language
Files:
packages/tm-core/src/common/logger/logger.spec.ts
**/?(*.)+(spec|test).ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
In JavaScript/TypeScript projects using Jest, test files should match *.test.ts and *.spec.ts patterns
Files:
packages/tm-core/src/common/logger/logger.spec.ts
{packages/*/src/**/*.spec.ts,apps/*/src/**/*.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Place package and app unit test files alongside source as *.spec.ts under src (packages//src//.spec.ts or apps//src//.spec.ts)
Files:
packages/tm-core/src/common/logger/logger.spec.ts
{packages/*/src/**/*.@(spec|test).ts,apps/*/src/**/*.@(spec|test).ts,packages/*/tests/**/*.@(spec|test).ts,apps/*/tests/**/*.@(spec|test).ts,tests/unit/packages/*/**/*.@(spec|test).ts}
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/*/src/**/*.@(spec|test).ts,apps/*/src/**/*.@(spec|test).ts,packages/*/tests/**/*.@(spec|test).ts,apps/*/tests/**/*.@(spec|test).ts,tests/unit/packages/*/**/*.@(spec|test).ts}: Do not use async/await in test functions unless the test is explicitly exercising asynchronous behavior
Use synchronous top-level imports in tests; avoid dynamic await import()
Keep test bodies synchronous whenever possible
Files:
packages/tm-core/src/common/logger/logger.spec.ts
mcp-server/src/tools/*.js
📄 CodeRabbit inference engine (.cursor/rules/architecture.mdc)
MCP server tools in mcp-server/src/tools/*.js must have their execute methods wrapped with the withNormalizedProjectRoot higher-order function from tools/utils.js to ensure consistent path handling.
mcp-server/src/tools/*.js: MCP tools must follow a specific structure: use server.addTool with snake_case tool names, define parameters using Zod, and implement the execute function as an async function.
All MCP tool execute methods that require access to the project root MUST be wrapped with the withNormalizedProjectRoot Higher-Order Function (HOF) from mcp-server/src/tools/utils.js.
MCP tools should always call *Direct wrappers instead of executeTaskMasterCommand, except as a fallback if a direct function is not yet implemented.
Use camelCase with Tool suffix for tool registration functions in mcp-server/src/tools/.
Use snake_case for tool names exposed to MCP clients in server.addTool definitions.
Log the start of execution with arguments (sanitized if sensitive), log successful completion with result summary, log all error conditions with appropriate log levels, and include the cache status in result logs in MCP tool files.
Do not log entire large data structures or sensitive information in MCP tool files.
Use handleApiResult to format and return the response from MCP tools.
mcp-server/src/tools/*.js: Create tool definitions in 'mcp-server/src/tools/', use Zod for parameter validation, include optional tag parameter for multi-context support, and follow established naming conventions.
For long-running operations that should not block the client, use the AsyncOperationManager in MCP tools and implement progress reporting.
Files:
mcp-server/src/tools/tool-registry.js
mcp-server/src/{tools,core/direct-functions}/*.js
📄 CodeRabbit inference engine (.cursor/rules/mcp.mdc)
mcp-server/src/{tools,core/direct-functions}/*.js: Use kebab-case for all file names in mcp-server/src/tools/ and mcp-server/src/core/direct-functions/.
Use helpers from mcp-server/src/tools/utils.js, mcp-server/src/core/utils/path-utils.js, and mcp-server/src/core/utils/ai-client-utils.js for centralized utilities.
Files:
mcp-server/src/tools/tool-registry.js
mcp-server/src/tools/**/*.js
📄 CodeRabbit inference engine (.cursor/rules/telemetry.mdc)
MCP tool files in mcp-server/src/tools/ must call the corresponding direct function wrapper and pass the result to handleApiResult(result, log) from mcp-server/src/tools/utils.js, ensuring telemetryData is included in the final MCP response.
Files:
mcp-server/src/tools/tool-registry.js
**/*.js
📄 CodeRabbit inference engine (.cursor/rules/tests.mdc)
**/*.js: Declare and initialize global variables at the top of modules to avoid hoisting issues.
Use proper function declarations to avoid hoisting issues and initialize variables before they are referenced.
Do not reference variables before their declaration in module scope.
Use dynamic imports (import()) to avoid initialization order issues in modules.
Files:
mcp-server/src/tools/tool-registry.js
mcp-server/src/{core/utils,tools}/**/*.js
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
Place utilities specifically designed to support the MCP server implementation into the appropriate subdirectories within
mcp-server/src/(e.g., path/core logic helpers inmcp-server/src/core/utils/, tool execution/response helpers inmcp-server/src/tools/utils.js).
Files:
mcp-server/src/tools/tool-registry.js
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : Use camelCase with Tool suffix for tool registration functions in mcp-server/src/tools/.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/{tools,core/direct-functions}/*.js : Use helpers from mcp-server/src/tools/utils.js, mcp-server/src/core/utils/path-utils.js, and mcp-server/src/core/utils/ai-client-utils.js for centralized utilities.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : Use snake_case for tool names exposed to MCP clients in server.addTool definitions.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-07-18T17:18:17.759Z
Learning: Applies to mcp-server/src/{core/utils,tools}/**/*.js : Place utilities specifically designed to support the MCP server implementation into the appropriate subdirectories within `mcp-server/src/` (e.g., path/core logic helpers in `mcp-server/src/core/utils/`, tool execution/response helpers in `mcp-server/src/tools/utils.js`).
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/telemetry.mdc:0-0
Timestamp: 2025-07-18T17:14:54.131Z
Learning: Applies to mcp-server/src/tools/**/*.js : MCP tool files in mcp-server/src/tools/ must call the corresponding direct function wrapper and pass the result to handleApiResult(result, log) from mcp-server/src/tools/utils.js, ensuring telemetryData is included in the final MCP response.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : MCP tools must follow a specific structure: use server.addTool with snake_case tool names, define parameters using Zod, and implement the execute function as an async function.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to mcp-server/src/tools/*.js : Create tool definitions in 'mcp-server/src/tools/', use Zod for parameter validation, include optional tag parameter for multi-context support, and follow established naming conventions.
📚 Learning: 2025-09-26T19:05:47.555Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1252
File: packages/ai-sdk-provider-grok-cli/package.json:11-13
Timestamp: 2025-09-26T19:05:47.555Z
Learning: In the eyaltoledano/claude-task-master repository, internal tm/ packages use a specific export pattern where the "exports" field points to TypeScript source files (./src/index.ts) while "main" points to compiled output (./dist/index.js) and "types" points to source files (./src/index.ts). This pattern is used consistently across internal packages like tm/core and tm/ai-sdk-provider-grok-cli because they are consumed directly during build-time bundling with tsdown rather than being published as separate packages.
Applied to files:
apps/mcp/src/index.tsapps/mcp/src/tools/tasks/index.ts
📚 Learning: 2025-07-18T17:12:57.903Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to mcp-server/src/tools/*.js : Create tool definitions in 'mcp-server/src/tools/', use Zod for parameter validation, include optional tag parameter for multi-context support, and follow established naming conventions.
Applied to files:
apps/mcp/src/tools/tasks/get-tasks.tool.tsapps/mcp/src/tools/tasks/get-task.tool.ts
📚 Learning: 2025-07-18T17:11:36.732Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : MCP tools must follow a specific structure: use server.addTool with snake_case tool names, define parameters using Zod, and implement the execute function as an async function.
Applied to files:
apps/mcp/src/tools/tasks/get-tasks.tool.tsapps/mcp/src/tools/tasks/get-task.tool.tsmcp-server/src/tools/tool-registry.js
📚 Learning: 2025-07-18T17:11:36.732Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : Use camelCase with Tool suffix for tool registration functions in mcp-server/src/tools/.
Applied to files:
apps/mcp/src/tools/tasks/get-tasks.tool.tsapps/mcp/src/tools/tasks/get-task.tool.tsapps/mcp/src/tools/tasks/index.tsmcp-server/src/tools/tool-registry.js
📚 Learning: 2025-07-18T17:10:53.657Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/glossary.mdc:0-0
Timestamp: 2025-07-18T17:10:53.657Z
Learning: Comprehensive reference for Taskmaster MCP tools and CLI commands with tagged task lists information (taskmaster.mdc).
Applied to files:
apps/mcp/src/tools/tasks/get-tasks.tool.ts
📚 Learning: 2025-07-18T17:10:12.881Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/dev_workflow.mdc:0-0
Timestamp: 2025-07-18T17:10:12.881Z
Learning: Use the Taskmaster command set (`task-master` CLI or MCP tools) for all task management operations: listing, expanding, updating, tagging, and status changes.
Applied to files:
apps/mcp/src/tools/tasks/get-tasks.tool.ts
📚 Learning: 2025-07-18T17:12:57.903Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to scripts/modules/**/*.test.js : Test CLI and MCP interfaces with real task data, verify end-to-end workflows across tag contexts, and test error scenarios and recovery in integration tests.
Applied to files:
packages/tm-core/src/common/logger/logger.spec.ts
📚 Learning: 2025-09-03T12:20:36.005Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1178
File: packages/tm-core/src/index.ts:56-57
Timestamp: 2025-09-03T12:20:36.005Z
Learning: The logger functionality in tm-core should only be available through the main package entry point (import { getLogger, createLogger, setGlobalLogger } from 'tm-core'), not as a separate subpath export like other modules such as auth or storage.
Applied to files:
packages/tm-core/src/tm-core.ts
📚 Learning: 2025-07-18T17:11:36.732Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : Use snake_case for tool names exposed to MCP clients in server.addTool definitions.
Applied to files:
mcp-server/src/tools/tool-registry.js
📚 Learning: 2025-07-18T17:11:36.732Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/{tools,core/direct-functions}/*.js : Use helpers from mcp-server/src/tools/utils.js, mcp-server/src/core/utils/path-utils.js, and mcp-server/src/core/utils/ai-client-utils.js for centralized utilities.
Applied to files:
mcp-server/src/tools/tool-registry.js
🧬 Code graph analysis (8)
apps/mcp/src/shared/utils.ts (1)
mcp-server/src/tools/utils.js (1)
versionInfo(295-295)
apps/mcp/src/tools/tasks/get-tasks.tool.ts (3)
apps/mcp/src/shared/utils.ts (2)
withNormalizedProjectRoot(183-270)handleApiResult(43-112)apps/mcp/src/shared/types.ts (1)
MCPContext(24-32)packages/tm-core/src/tm-core.ts (1)
createTmCore(215-217)
apps/mcp/src/tools/tasks/get-task.tool.ts (3)
apps/mcp/src/shared/utils.ts (2)
withNormalizedProjectRoot(183-270)handleApiResult(43-112)apps/mcp/src/shared/types.ts (1)
MCPContext(24-32)packages/tm-core/src/tm-core.ts (2)
createTmCore(215-217)tasks(92-94)
packages/tm-core/src/common/logger/logger.spec.ts (1)
packages/tm-core/src/common/logger/logger.ts (3)
Logger(44-354)child(343-353)LogCallback(28-30)
packages/tm-core/src/tm-core.ts (1)
packages/tm-core/src/common/logger/logger.ts (3)
LoggerConfig(32-42)Logger(44-354)error(250-253)
packages/tm-core/src/common/logger/logger.ts (1)
packages/tm-core/src/common/logger/index.ts (5)
LogObject(7-7)LogCallback(7-7)LoggerConfig(7-7)LogLevel(6-6)Logger(6-6)
mcp-server/src/tools/tool-registry.js (3)
apps/mcp/src/tools/tasks/get-tasks.tool.ts (1)
registerGetTasksTool(49-130)apps/mcp/src/tools/tasks/index.ts (2)
registerGetTasksTool(6-6)registerGetTaskTool(7-7)apps/mcp/src/tools/tasks/get-task.tool.ts (1)
registerGetTaskTool(50-142)
packages/tm-core/src/modules/storage/adapters/api-storage.ts (1)
packages/tm-core/src/modules/auth/managers/auth-manager.ts (1)
AuthManager(26-286)
🪛 GitHub Actions: CI
packages/tm-core/src/common/interfaces/storage.interface.ts
[error] 240-240: TypeScript error TS2420: Class 'BaseStorage' incorrectly implements interface 'IStorage'. (tsc --noEmit)
🔇 Additional comments (13)
apps/mcp/src/tools/tasks/index.ts (1)
1-7: LGTM!The export aggregator follows established naming conventions and patterns. The file correctly re-exports tool registration functions using the camelCase with Tool suffix convention.
apps/mcp/src/shared/utils.ts (1)
48-68: LGTM! Optional tag parameter enhances flexibility.The addition of the optional
tagparameter allows callers to override the state.json-derived tag, which aligns with the new API storage brief name handling. The logic correctly checksprovidedTag !== undefinedto distinguish between an explicitly passednulland an omitted parameter.apps/mcp/src/index.ts (1)
7-7: LGTM!The new export correctly extends the public API surface to include task tool registrations, following the established pattern in this file.
packages/tm-core/src/common/logger/index.ts (1)
7-7: LGTM!The additional type exports extend the public API surface without introducing runtime changes, supporting the new callback-based logging integration.
packages/tm-core/src/modules/storage/adapters/api-storage.ts (1)
152-160: LGTM!The implementation correctly retrieves the current brief name from the auth context. The use of optional chaining and null fallback aligns with the interface contract.
packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts (1)
58-64: LGTM!The implementation correctly returns
nullfor file storage, which doesn't use the brief concept. The comment clearly explains the rationale.packages/tm-core/src/modules/tasks/services/task-service.ts (1)
156-169: LGTM! Storage-aware tag resolution implemented correctly.The logic appropriately differentiates between API and file storage when computing the tag/brief name. For API storage, it correctly falls back to the tag parameter when
getCurrentBriefName()returnsnull. The updated JSDoc and inline comments help clarify this dual-purpose behavior.apps/mcp/src/tools/tasks/get-task.tool.ts (2)
114-122: Prefer tm-core’s resolved tag in response (if available)To keep tags consistent with storage/brief resolution, prefer tmCore’s tag if it differs from the provided one.
If tmCore.tasks.get returns a tag/brief, pass that to handleApiResult; otherwise keep current.
6-13: zod/v3 subpath is properly supported — no action neededZod v4 (including 4.1.11) officially supports the subpath strategy where v3 remains available at "zod/v3". The codebase correctly uses this documented feature, with 48+ files already importing from
zod/v3. Repo tooling resolves it correctly; no fallback is necessary.apps/mcp/src/tools/tasks/get-tasks.tool.ts (1)
49-71: Overall: structure, logging, and response handling look solidTool name, HOF use, logging phases, and handleApiResult integration align with MCP tool guidelines.
Based on learnings.
Also applies to: 94-111
mcp-server/src/tools/tool-registry.js (2)
70-72: Mappings for get_tasks/get_task look correctSnake_case names and camelCase registration functions align with conventions.
Based on learnings.
Also applies to: 110-118
41-53: Import path @tm/mcp is valid and fully resolvable.Verification confirmed:
- Package exports from main entry (./src/index.ts) include
export * from './tools/tasks/index.js'- Both
registerGetTasksToolandregisterGetTaskToolare properly re-exported through the package- Runtime import will succeed
packages/tm-core/src/tm-core.ts (1)
159-168: Logger init and usage order LGTMLogger is created before components that may log; success message after domain init; error path guards for undefined logger. Solid.
Also applies to: 180-185
| it('should handle callback errors gracefully', () => { | ||
| const errorCallback: LogCallback = () => { | ||
| throw new Error('Callback error'); | ||
| }; | ||
|
|
||
| const logger = new Logger({ | ||
| level: LogLevel.INFO, | ||
| logCallback: errorCallback | ||
| }); | ||
|
|
||
| // Should not throw | ||
| expect(() => { | ||
| logger.info('Test message'); | ||
| }).toThrow('Callback error'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Test intent mismatch: “gracefully” vs expecting throw
The spec title says “handle callback errors gracefully” but the assertion expects toThrow. Clarify desired behavior and align name/assertion.
Two options:
- If errors should bubble: rename test and fix comment.
- it('should handle callback errors gracefully', () => {
+ it('should propagate callback errors', () => {
@@
- // Should not throw
+ // Should throw- If logger should swallow callback errors: wrap callback invocation in logger (see suggestion in logger.ts) and update test to not throw.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find Logger implementation file
fd -t f 'logger\.' packages/tm-core/src/common/logger/ | grep -v spec
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for logCallback usage in Logger class
rg -n 'logCallback' packages/tm-core/src/common/logger/ --type=ts -A 5 -B 2Length of output: 16811
🏁 Script executed:
#!/bin/bash
# Search for callback-related tests to understand expected behavior
rg -n 'callback' packages/tm-core/src/common/logger/logger.spec.ts -B 2 -A 8 | head -100Length of output: 2592
🏁 Script executed:
#!/bin/bash
# Check for try-catch blocks around callback invocation
ast-grep --pattern 'try {
$_
logCallback($_)
$_
}'Length of output: 0
Test name contradicts assertion—align with actual logger behavior
The implementation does not wrap callback invocations in try-catch (lines 224, 228, 291, 294 in logger.ts), so callback errors propagate. The test assertion correctly expects toThrow, but the name "should handle callback errors gracefully" and comment "Should not throw" contradict this.
Rename test and fix comment to reflect actual behavior:
- it('should handle callback errors gracefully', () => {
+ it('should propagate callback errors', () => {
const errorCallback: LogCallback = () => {
throw new Error('Callback error');
};
const logger = new Logger({
level: LogLevel.INFO,
logCallback: errorCallback
});
- // Should not throw
+ // Should throw
expect(() => {
logger.info('Test message');
}).toThrow('Callback error');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should handle callback errors gracefully', () => { | |
| const errorCallback: LogCallback = () => { | |
| throw new Error('Callback error'); | |
| }; | |
| const logger = new Logger({ | |
| level: LogLevel.INFO, | |
| logCallback: errorCallback | |
| }); | |
| // Should not throw | |
| expect(() => { | |
| logger.info('Test message'); | |
| }).toThrow('Callback error'); | |
| }); | |
| it('should propagate callback errors', () => { | |
| const errorCallback: LogCallback = () => { | |
| throw new Error('Callback error'); | |
| }; | |
| const logger = new Logger({ | |
| level: LogLevel.INFO, | |
| logCallback: errorCallback | |
| }); | |
| // Should throw | |
| expect(() => { | |
| logger.info('Test message'); | |
| }).toThrow('Callback error'); | |
| }); |
🤖 Prompt for AI Agents
In packages/tm-core/src/common/logger/logger.spec.ts around lines 335 to 349,
the test name and inline comment claim the logger should handle callback errors
gracefully and "Should not throw", but the assertion expects the error to be
thrown and the implementation lets callback errors propagate; rename the test to
something like "should propagate callback errors from logCallback" (or
equivalent) and update the inline comment to reflect that the logger will throw
(e.g., "Should throw when logCallback throws"), leaving the expect(...).toThrow
assertion as-is so the test matches actual behavior.
c14abe3 to
652eff6
Compare
652eff6 to
f0caca9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/mcp/src/tools/tasks/get-task.tool.ts (1)
27-31: Update the description to reflect required parameter.The schema requires projectRoot (no
.optional()), but the description says "Optional, usually from session". This is misleading. Based on the past review discussion, projectRoot should be required. Update the description to clarify this.Apply this diff:
projectRoot: z .string() .describe( - 'Absolute path to the project root directory (Optional, usually from session)' + 'Absolute path to the project root directory (required; typically provided from session context)' ),
🧹 Nitpick comments (4)
apps/mcp/src/tools/tasks/get-tasks.tool.ts (1)
77-78: Coerce includeSubtasks to boolean.Avoid passing undefined; coerce to a strict boolean.
Apply this diff:
- includeSubtasks: withSubtasks + includeSubtasks: Boolean(withSubtasks)packages/tm-core/src/common/logger/logger.ts (3)
196-205: Strengthen type guard for LogObject.Check method types, not just property presence.
Apply this diff:
- return ( - typeof callback === 'object' && - callback !== null && - 'info' in callback && - 'warn' in callback && - 'error' in callback && - 'debug' in callback - ); + return ( + typeof callback === 'object' && + callback !== null && + typeof (callback as any).info === 'function' && + typeof (callback as any).warn === 'function' && + typeof (callback as any).error === 'function' && + typeof (callback as any).debug === 'function' + );
110-114: Avoid ANSI color codes when routing to MCP/callback sinks.ANSI can pollute structured logs. Disable colors when
mcpModeorlogCallbackis set.Apply this diff:
// MCP mode overrides to silent ONLY if no callback is provided if (this.config.mcpMode && !this.config.logCallback) { this.config.silent = true; } + // Avoid ANSI sequences for external log sinks + if (this.config.mcpMode || this.config.logCallback) { + this.config.colors = false; + }And mirror in
setConfig:// MCP mode overrides to silent ONLY if no callback is provided if (this.config.mcpMode && !this.config.logCallback) { this.config.silent = true; } + if (this.config.mcpMode || this.config.logCallback) { + this.config.colors = false; + }Also applies to: 318-322
233-245: Use console.debug for DEBUG level.Route debug logs to
console.debug.Apply this diff:
// Otherwise use console switch (level) { case LogLevel.ERROR: console.error(formatted); break; case LogLevel.WARN: console.warn(formatted); break; + case LogLevel.DEBUG: + console.debug(formatted); + break; default: console.log(formatted); break; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.changeset/four-bugs-occur.md(1 hunks)apps/mcp/src/index.ts(1 hunks)apps/mcp/src/shared/utils.ts(1 hunks)apps/mcp/src/tools/tasks/get-task.tool.ts(1 hunks)apps/mcp/src/tools/tasks/get-tasks.tool.ts(1 hunks)apps/mcp/src/tools/tasks/index.ts(1 hunks)mcp-server/src/core/direct-functions/list-tasks.js(0 hunks)mcp-server/src/core/direct-functions/show-task.js(0 hunks)mcp-server/src/core/task-master-core.js(0 hunks)mcp-server/src/tools/get-task.js(0 hunks)mcp-server/src/tools/get-tasks.js(0 hunks)mcp-server/src/tools/tool-registry.js(4 hunks)packages/tm-core/src/common/interfaces/storage.interface.ts(2 hunks)packages/tm-core/src/common/logger/index.ts(1 hunks)packages/tm-core/src/common/logger/logger.spec.ts(1 hunks)packages/tm-core/src/common/logger/logger.ts(5 hunks)packages/tm-core/src/modules/storage/adapters/api-storage.ts(1 hunks)packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts(1 hunks)packages/tm-core/src/modules/tasks/services/task-service.ts(2 hunks)packages/tm-core/src/tm-core.ts(6 hunks)
💤 Files with no reviewable changes (5)
- mcp-server/src/tools/get-tasks.js
- mcp-server/src/core/direct-functions/list-tasks.js
- mcp-server/src/core/task-master-core.js
- mcp-server/src/tools/get-task.js
- mcp-server/src/core/direct-functions/show-task.js
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/tm-core/src/modules/tasks/services/task-service.ts
- packages/tm-core/src/modules/storage/adapters/file-storage/file-storage.ts
- apps/mcp/src/tools/tasks/index.ts
- packages/tm-core/src/common/logger/logger.spec.ts
- packages/tm-core/src/common/logger/index.ts
🧰 Additional context used
📓 Path-based instructions (7)
mcp-server/src/tools/*.js
📄 CodeRabbit inference engine (.cursor/rules/architecture.mdc)
MCP server tools in mcp-server/src/tools/*.js must have their execute methods wrapped with the withNormalizedProjectRoot higher-order function from tools/utils.js to ensure consistent path handling.
mcp-server/src/tools/*.js: MCP tools must follow a specific structure: use server.addTool with snake_case tool names, define parameters using Zod, and implement the execute function as an async function.
All MCP tool execute methods that require access to the project root MUST be wrapped with the withNormalizedProjectRoot Higher-Order Function (HOF) from mcp-server/src/tools/utils.js.
MCP tools should always call *Direct wrappers instead of executeTaskMasterCommand, except as a fallback if a direct function is not yet implemented.
Use camelCase with Tool suffix for tool registration functions in mcp-server/src/tools/.
Use snake_case for tool names exposed to MCP clients in server.addTool definitions.
Log the start of execution with arguments (sanitized if sensitive), log successful completion with result summary, log all error conditions with appropriate log levels, and include the cache status in result logs in MCP tool files.
Do not log entire large data structures or sensitive information in MCP tool files.
Use handleApiResult to format and return the response from MCP tools.
mcp-server/src/tools/*.js: Create tool definitions in 'mcp-server/src/tools/', use Zod for parameter validation, include optional tag parameter for multi-context support, and follow established naming conventions.
For long-running operations that should not block the client, use the AsyncOperationManager in MCP tools and implement progress reporting.
Files:
mcp-server/src/tools/tool-registry.js
mcp-server/src/{tools,core/direct-functions}/*.js
📄 CodeRabbit inference engine (.cursor/rules/mcp.mdc)
mcp-server/src/{tools,core/direct-functions}/*.js: Use kebab-case for all file names in mcp-server/src/tools/ and mcp-server/src/core/direct-functions/.
Use helpers from mcp-server/src/tools/utils.js, mcp-server/src/core/utils/path-utils.js, and mcp-server/src/core/utils/ai-client-utils.js for centralized utilities.
Files:
mcp-server/src/tools/tool-registry.js
mcp-server/src/tools/**/*.js
📄 CodeRabbit inference engine (.cursor/rules/telemetry.mdc)
MCP tool files in mcp-server/src/tools/ must call the corresponding direct function wrapper and pass the result to handleApiResult(result, log) from mcp-server/src/tools/utils.js, ensuring telemetryData is included in the final MCP response.
Files:
mcp-server/src/tools/tool-registry.js
**/*.js
📄 CodeRabbit inference engine (.cursor/rules/tests.mdc)
**/*.js: Declare and initialize global variables at the top of modules to avoid hoisting issues.
Use proper function declarations to avoid hoisting issues and initialize variables before they are referenced.
Do not reference variables before their declaration in module scope.
Use dynamic imports (import()) to avoid initialization order issues in modules.
Files:
mcp-server/src/tools/tool-registry.js
mcp-server/src/{core/utils,tools}/**/*.js
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
Place utilities specifically designed to support the MCP server implementation into the appropriate subdirectories within
mcp-server/src/(e.g., path/core logic helpers inmcp-server/src/core/utils/, tool execution/response helpers inmcp-server/src/tools/utils.js).
Files:
mcp-server/src/tools/tool-registry.js
.changeset/*.md
📄 CodeRabbit inference engine (.cursor/rules/changeset.mdc)
.changeset/*.md: When runningnpm run changesetornpx changeset add, provide a concise summary of the changes for theCHANGELOG.mdin imperative mood, typically a single line, and not a detailed Git commit message.
The changeset summary should be user-facing, describing what changed in the released version that is relevant to users or consumers of the package.
Do not use your detailed Git commit message body as the changeset summary.Write changeset entries as user-facing summaries of what users get or what was fixed, not low-level implementation details
Files:
.changeset/four-bugs-occur.md
.changeset/*
📄 CodeRabbit inference engine (.cursor/rules/new_features.mdc)
Create appropriate changesets for new features, use semantic versioning, include tagged system information in release notes, and document breaking changes if any.
Files:
.changeset/four-bugs-occur.md
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/{tools,core/direct-functions}/*.js : Use helpers from mcp-server/src/tools/utils.js, mcp-server/src/core/utils/path-utils.js, and mcp-server/src/core/utils/ai-client-utils.js for centralized utilities.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to mcp-server/src/tools/*.js : Create tool definitions in 'mcp-server/src/tools/', use Zod for parameter validation, include optional tag parameter for multi-context support, and follow established naming conventions.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : Use camelCase with Tool suffix for tool registration functions in mcp-server/src/tools/.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : Use snake_case for tool names exposed to MCP clients in server.addTool definitions.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-07-18T17:18:17.759Z
Learning: Applies to {scripts/modules/utils.js,mcp-server/src/core/utils/path-utils.js,mcp-server/src/tools/utils.js} : Export all utility functions explicitly, group related functions logically, and include new tagged system utilities.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-07-18T17:18:17.759Z
Learning: Applies to mcp-server/src/{core/utils,tools}/**/*.js : Place utilities specifically designed to support the MCP server implementation into the appropriate subdirectories within `mcp-server/src/` (e.g., path/core logic helpers in `mcp-server/src/core/utils/`, tool execution/response helpers in `mcp-server/src/tools/utils.js`).
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : MCP tools must follow a specific structure: use server.addTool with snake_case tool names, define parameters using Zod, and implement the execute function as an async function.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/telemetry.mdc:0-0
Timestamp: 2025-07-18T17:14:54.131Z
Learning: Applies to mcp-server/src/tools/**/*.js : MCP tool files in mcp-server/src/tools/ must call the corresponding direct function wrapper and pass the result to handleApiResult(result, log) from mcp-server/src/tools/utils.js, ensuring telemetryData is included in the final MCP response.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to mcp-server/src/core/direct-functions/*.js : Create direct function wrappers in 'mcp-server/src/core/direct-functions/' for MCP tool implementation, following silent mode patterns and using findTasksJsonPath for consistent path resolution.
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-07-18T17:18:17.759Z
Learning: Applies to scripts/modules/utils.js : Implement complete migration for all related files in the tagged task system, handle configuration and state file creation, and provide migration status tracking.
📚 Learning: 2025-07-18T17:12:57.903Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to mcp-server/src/tools/*.js : Create tool definitions in 'mcp-server/src/tools/', use Zod for parameter validation, include optional tag parameter for multi-context support, and follow established naming conventions.
Applied to files:
apps/mcp/src/tools/tasks/get-tasks.tool.tsapps/mcp/src/tools/tasks/get-task.tool.ts
📚 Learning: 2025-07-18T17:11:36.732Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : MCP tools must follow a specific structure: use server.addTool with snake_case tool names, define parameters using Zod, and implement the execute function as an async function.
Applied to files:
apps/mcp/src/tools/tasks/get-tasks.tool.tsmcp-server/src/tools/tool-registry.jsapps/mcp/src/tools/tasks/get-task.tool.ts
📚 Learning: 2025-07-18T17:11:36.732Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : Use camelCase with Tool suffix for tool registration functions in mcp-server/src/tools/.
Applied to files:
apps/mcp/src/tools/tasks/get-tasks.tool.tsmcp-server/src/tools/tool-registry.jsapps/mcp/src/tools/tasks/get-task.tool.ts
📚 Learning: 2025-07-18T17:10:53.657Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/glossary.mdc:0-0
Timestamp: 2025-07-18T17:10:53.657Z
Learning: Comprehensive reference for Taskmaster MCP tools and CLI commands with tagged task lists information (taskmaster.mdc).
Applied to files:
apps/mcp/src/tools/tasks/get-tasks.tool.ts
📚 Learning: 2025-07-18T17:11:36.732Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/tools/*.js : Use snake_case for tool names exposed to MCP clients in server.addTool definitions.
Applied to files:
mcp-server/src/tools/tool-registry.jsapps/mcp/src/tools/tasks/get-task.tool.ts
📚 Learning: 2025-07-18T17:11:36.732Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/mcp.mdc:0-0
Timestamp: 2025-07-18T17:11:36.732Z
Learning: Applies to mcp-server/src/{tools,core/direct-functions}/*.js : Use helpers from mcp-server/src/tools/utils.js, mcp-server/src/core/utils/path-utils.js, and mcp-server/src/core/utils/ai-client-utils.js for centralized utilities.
Applied to files:
mcp-server/src/tools/tool-registry.js
📚 Learning: 2025-07-18T17:07:39.336Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-07-18T17:07:39.336Z
Learning: Applies to mcp-server/src/core/direct-functions/*.js : Always use the isSilentMode() function to check silent mode state in mcp-server/src/core/direct-functions/*.js; do not access global variables like silentMode directly.
Applied to files:
packages/tm-core/src/common/logger/logger.ts
📚 Learning: 2025-07-18T05:38:17.352Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#943
File: scripts/modules/task-manager/move-task.js:24-24
Timestamp: 2025-07-18T05:38:17.352Z
Learning: In the Claude Task Master system, core task-manager functions are designed with fallback mechanisms for missing projectRoot parameters using the pattern `const projectRoot = providedProjectRoot || findProjectRoot();`. The readJSON and writeJSON functions have default parameters (projectRoot = null, tag = null) and handle missing parameters gracefully. Adding strict validation to these core functions would break the established flexible architecture pattern.
Applied to files:
apps/mcp/src/tools/tasks/get-task.tool.ts
📚 Learning: 2025-09-26T19:05:47.555Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1252
File: packages/ai-sdk-provider-grok-cli/package.json:11-13
Timestamp: 2025-09-26T19:05:47.555Z
Learning: In the eyaltoledano/claude-task-master repository, internal tm/ packages use a specific export pattern where the "exports" field points to TypeScript source files (./src/index.ts) while "main" points to compiled output (./dist/index.js) and "types" points to source files (./src/index.ts). This pattern is used consistently across internal packages like tm/core and tm/ai-sdk-provider-grok-cli because they are consumed directly during build-time bundling with tsdown rather than being published as separate packages.
Applied to files:
apps/mcp/src/index.ts
📚 Learning: 2025-09-03T12:20:36.005Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1178
File: packages/tm-core/src/index.ts:56-57
Timestamp: 2025-09-03T12:20:36.005Z
Learning: The logger functionality in tm-core should only be available through the main package entry point (import { getLogger, createLogger, setGlobalLogger } from 'tm-core'), not as a separate subpath export like other modules such as auth or storage.
Applied to files:
packages/tm-core/src/tm-core.ts
📚 Learning: 2025-07-18T17:18:17.759Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-07-18T17:18:17.759Z
Learning: Applies to scripts/modules/utils.js : Use tag resolution functions for all task data access, provide backward compatibility with legacy format, and default to 'master' tag when no tag is specified.
Applied to files:
apps/mcp/src/shared/utils.ts
📚 Learning: 2025-07-18T17:12:57.903Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to scripts/modules/*.js : Default to current tag when not specified, support explicit tag selection in advanced features, validate tag existence before operations, and provide clear messaging about tag context.
Applied to files:
apps/mcp/src/shared/utils.ts
📚 Learning: 2025-07-18T17:13:30.188Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tags.mdc:0-0
Timestamp: 2025-07-18T17:13:30.188Z
Learning: Applies to scripts/modules/* : Do not hard-code tag resolution (e.g., const tag = options.tag || 'master';); always use getCurrentTag
Applied to files:
apps/mcp/src/shared/utils.ts
🧬 Code graph analysis (6)
apps/mcp/src/tools/tasks/get-tasks.tool.ts (3)
apps/mcp/src/shared/utils.ts (2)
withNormalizedProjectRoot(183-270)handleApiResult(43-112)apps/mcp/src/shared/types.ts (1)
MCPContext(24-32)packages/tm-core/src/tm-core.ts (1)
createTmCore(215-217)
mcp-server/src/tools/tool-registry.js (3)
apps/mcp/src/tools/tasks/get-tasks.tool.ts (1)
registerGetTasksTool(39-166)apps/mcp/src/tools/tasks/index.ts (2)
registerGetTasksTool(6-6)registerGetTaskTool(7-7)apps/mcp/src/tools/tasks/get-task.tool.ts (1)
registerGetTaskTool(40-136)
packages/tm-core/src/common/logger/logger.ts (1)
packages/tm-core/src/common/logger/index.ts (5)
LogObject(7-7)LogCallback(7-7)LoggerConfig(7-7)LogLevel(6-6)Logger(6-6)
apps/mcp/src/tools/tasks/get-task.tool.ts (3)
apps/mcp/src/shared/utils.ts (2)
withNormalizedProjectRoot(183-270)handleApiResult(43-112)apps/mcp/src/shared/types.ts (1)
MCPContext(24-32)packages/tm-core/src/tm-core.ts (1)
createTmCore(215-217)
packages/tm-core/src/modules/storage/adapters/api-storage.ts (1)
packages/tm-core/src/modules/auth/managers/auth-manager.ts (1)
AuthManager(26-286)
packages/tm-core/src/tm-core.ts (2)
packages/tm-core/src/common/logger/index.ts (3)
LoggerConfig(7-7)Logger(6-6)createLogger(8-8)packages/tm-core/src/common/logger/logger.ts (3)
LoggerConfig(32-42)Logger(44-354)error(250-253)
🪛 LanguageTool
.changeset/four-bugs-occur.md
[style] ~7-~7: Consider a more concise word here.
Context: ...from get-tasks and get-task mcp tool - In an effort to reduce complexity and context bloat for...
(IN_ORDER_TO_PREMIUM)
🔇 Additional comments (20)
packages/tm-core/src/tm-core.ts (6)
20-24: LGTM! Logger imports are correct.The logger imports are properly structured and align with the logger module exports.
34-35: LGTM! Optional logger configuration added correctly.The optional loggerConfig parameter enables MCP integration and debugging as intended.
81-81: LGTM! Logger field follows established patterns.The private _logger field uses definite assignment assertion, which is safe since initialization is guaranteed through the factory method.
110-112: LGTM! Logger getter properly exposes logging capability.The public getter allows consumers to access the logger, which is essential for MCP integration scenarios.
161-162: LGTM! Logger initialization is correctly placed first.Creating the logger before other components ensures that all initialization steps can log properly.
183-189: LGTM! Defensive error logging implementation.The error handler correctly checks if the logger exists before using it, which is essential since initialization might fail during logger creation.
apps/mcp/src/index.ts (1)
7-7: LGTM! Task tools export follows established pattern.The new export extends the public API surface and aligns with the existing module structure.
.changeset/four-bugs-occur.md (1)
1-7: LGTM! Changeset appropriately documents the changes.The changeset correctly categorizes this as a minor change and provides a clear user-facing summary. The static analysis style suggestion is a nitpick and can be safely ignored.
apps/mcp/src/shared/utils.ts (1)
48-68: LGTM! Tag parameter addition is well-implemented.The optional tag parameter allows callers to provide an explicit tag/brief name, with proper fallback logic to getCurrentTag. The comments clearly explain the behavior for API storage scenarios.
packages/tm-core/src/modules/storage/adapters/api-storage.ts (1)
152-160: LGTM! Brief name retrieval is correctly implemented.The method appropriately retrieves the brief name from the auth context for API storage scenarios, with a null fallback when no brief is selected.
packages/tm-core/src/common/interfaces/storage.interface.ts (2)
191-195: LGTM! Interface method is properly documented.The getCurrentBriefName method is correctly added to the IStorage interface with clear documentation about its applicability.
280-280: LGTM! Abstract method enforces implementation.The abstract method declaration in BaseStorage correctly ensures all concrete storage implementations provide the getCurrentBriefName method.
mcp-server/src/tools/tool-registry.js (2)
41-53: LGTM! Imports correctly reference new MCP tools.The imports from @tm/mcp follow the established naming conventions and properly integrate the new TypeScript-based task tools.
70-71: LGTM! Tool registry correctly maps to new registrations.The mappings for get_tasks and get_task now use the new MCP tool registrations, completing the migration from the old JavaScript implementations.
apps/mcp/src/tools/tasks/get-task.tool.ts (5)
63-67: LGTM! Parallel task fetching improves performance.Using Promise.all to fetch multiple tasks in parallel is an efficient approach when handling comma-separated IDs.
73-84: LGTM! Subtask filtering avoids mutation.Creating a copy of the task object when filtering subtasks is a good defensive programming practice that prevents unintended side effects.
87-99: LGTM! Appropriate error handling for empty results.Returning a structured error response when no tasks are found provides clear feedback to the caller.
105-106: LGTM! Smart response shaping based on input.Returning a single task for single-ID requests and an array for multiple IDs provides a better developer experience.
108-116: Tag parameter is properly passed through the entire call chain.The verification confirms that the tag parameter flows correctly from
get-task.tool.tsthroughTasksDomain.get()andTaskService.getTask()to the storage layer. TheTaskService.getTask()method receives the tag, resolves it to an active tag if needed, and delegates tostorage.loadTask(taskId, activeTag)for appropriate handling by the storage layer.apps/mcp/src/tools/tasks/get-tasks.tool.ts (1)
39-46: Tool registration looks correct.Snake_case name, Zod schema, async execute with normalization and handleApiResult. Good alignment with MCP tool conventions.
| status && status !== 'all' | ||
| ? { | ||
| status: status | ||
| .split(',') | ||
| .map((s: string) => s.trim() as TaskStatus) | ||
| } | ||
| : undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
Validate/normalize status values and align stat keys with tm-core.
- Mapping
status.split(',')toTaskStatusvia cast risks invalid values at runtime. - Stat keys use
'in-progress'; confirm tm-core’s canonical literals (e.g.,in_progress,in-progress,inProgress).
Run to discover the canonical TaskStatus literals and tasks.list filter shape:
If literals are known, consider:
- Validate with Zod (enum or custom refine for comma‑separated lists).
- Normalize inputs (e.g., map hyphen/underscore variants) before building
filter. Based on learnings.
Also applies to: 121-129, 133-137
🏁 Script executed:
#!/bin/bash
# Locate TaskStatus definition and usages
rg -nP -C2 '\b(export\s+(type|enum)\s+TaskStatus\b|type\s+TaskStatus\s*=\s*["'\''][^;]+)' --type=ts
rg -nP -C2 '\btasks\s*\.\s*list\s*\(' --type=ts
# Common status strings across core
rg -nP -C1 '\b(done|pending|blocked|deferred|cancelled|canceled|in[-_ ]?progress|review)\b' --type=tsLength of output: 82031
Validate status input against TaskStatus literal union before mapping; remove unsafe cast.
The concern is valid. The canonical TaskStatus literals in tm-core use hyphenated format ('in-progress', not 'in_progress'), and line 65–71's cast as TaskStatus bypasses type safety—an invalid status like 'in_progress' would slip through at runtime.
Replace the unsafe spread-and-cast with explicit validation:
- Validate each trimmed status value against the TaskStatus union (e.g., using Zod
.enum()or a guard function). - Reject or normalize invalid statuses before building the filter.
- Lines 121–129 and 133–137 correctly reference
taskCounts['in-progress']andsubtaskCounts['in-progress'], but will reflect accurate counts only if upstream filtering validates inputs.
| const result = await tmCore.tasks.list({ | ||
| tag, | ||
| filter, | ||
| includeSubtasks: withSubtasks | ||
| }); | ||
|
|
||
| context.log.info( | ||
| `Retrieved ${result.tasks?.length || 0} tasks (${result.filtered} filtered, ${result.total} total)` | ||
| ); | ||
|
|
||
| // Calculate stats using reduce for cleaner code | ||
| const totalTasks = result.total; | ||
| const taskCounts = result.tasks.reduce( | ||
| (acc, task) => { | ||
| acc[task.status] = (acc[task.status] || 0) + 1; | ||
| return acc; | ||
| }, | ||
| {} as Record<string, number> | ||
| ); | ||
|
|
||
| const completionPercentage = | ||
| totalTasks > 0 ? ((taskCounts.done || 0) / totalTasks) * 100 : 0; | ||
|
|
||
| // Count subtasks using reduce | ||
| const subtaskCounts = result.tasks.reduce( | ||
| (acc, task) => { | ||
| task.subtasks?.forEach((st) => { | ||
| acc.total++; | ||
| acc[st.status] = (acc[st.status] || 0) + 1; | ||
| }); | ||
| return acc; | ||
| }, | ||
| { total: 0 } as Record<string, number> | ||
| ); |
There was a problem hiding this comment.
Fix potential crash and inconsistent stats base.
result.tasks may be undefined; calling .reduce will throw. Also, stats mix counts from the filtered array with result.total as denominator. Use a null-safe tasks array and keep the denominator consistent.
Apply this diff:
// Call tm-core tasks.list()
const result = await tmCore.tasks.list({
tag,
filter,
- includeSubtasks: withSubtasks
+ includeSubtasks: withSubtasks
});
- context.log.info(
- `Retrieved ${result.tasks?.length || 0} tasks (${result.filtered} filtered, ${result.total} total)`
- );
+ const tasks = result.tasks ?? [];
+ context.log.info(
+ `Retrieved ${tasks.length} tasks (${result.filtered} filtered, ${result.total} total)`
+ );
- // Calculate stats using reduce for cleaner code
- const totalTasks = result.total;
- const taskCounts = result.tasks.reduce(
+ // Calculate stats using reduce for cleaner code
+ const totalTasks = tasks.length;
+ const taskCounts = tasks.reduce(
(acc, task) => {
acc[task.status] = (acc[task.status] || 0) + 1;
return acc;
},
{} as Record<string, number>
);
const completionPercentage =
- totalTasks > 0 ? ((taskCounts.done || 0) / totalTasks) * 100 : 0;
+ totalTasks > 0 ? ((taskCounts.done || 0) / totalTasks) * 100 : 0;
// Count subtasks using reduce
- const subtaskCounts = result.tasks.reduce(
+ const subtaskCounts = tasks.reduce(
(acc, task) => {
task.subtasks?.forEach((st) => {
acc.total++;
acc[st.status] = (acc[st.status] || 0) + 1;
});
return acc;
},
{ total: 0 } as Record<string, number>
);🤖 Prompt for AI Agents
In apps/mcp/src/tools/tasks/get-tasks.tool.ts around lines 74 to 107,
result.tasks may be undefined and the code mixes counts computed from the
filtered tasks array with result.total as the denominator; fix by creating a
null-safe tasks array (const tasks = result.tasks ?? []) and use that tasks
array for all reductions and counts, set totalTasks = tasks.length (so
completionPercentage and subtask counts use the same base), and ensure the
subtask accumulator is correctly typed/initialized (e.g., { total: 0 } plus
status keys) before reducing; keep the original log that uses
result.filtered/result.total if you still need overall totals but use tasks for
internal stats to avoid crashes and inconsistent denominators.
| private shouldLog(level: LogLevel): boolean { | ||
| // If a callback is provided, route logs through it while still respecting the configured level | ||
| if (this.config.logCallback) { | ||
| return level <= this.config.level; | ||
| } | ||
|
|
||
| // Otherwise, respect silent/mcpMode flags | ||
| if (this.config.silent || this.config.mcpMode) { |
There was a problem hiding this comment.
Silent should override callbacks; raw logs must respect silent.
Currently, a provided logCallback bypasses silent, and log() emits even in silent mode. This breaks expected semantics.
Apply this diff:
private shouldLog(level: LogLevel): boolean {
- // If a callback is provided, route logs through it while still respecting the configured level
- if (this.config.logCallback) {
- return level <= this.config.level;
- }
-
- // Otherwise, respect silent/mcpMode flags
- if (this.config.silent || this.config.mcpMode) {
+ // Silent overrides everything
+ if (this.config.silent) {
+ return false;
+ }
+ // If a callback is provided, route logs through it while still respecting the configured level
+ if (this.config.logCallback) {
+ return level <= this.config.level;
+ }
+ // Otherwise, respect mcpMode flag
+ if (this.config.mcpMode) {
return false;
}
return level <= this.config.level;
}
...
log(message: string, ...args: any[]): void {
- // If callback is provided, use it for raw logs too
- if (this.config.logCallback) {
+ // Respect silent first
+ if (this.config.silent) return;
+ // If callback is provided, use it for raw logs too
+ if (this.config.logCallback) {
const fullMessage =
args.length > 0 ? [message, ...args].join(' ') : message;
// If callback is a log object, use info method for raw logs
if (this.isLogObject(this.config.logCallback)) {
this.config.logCallback.info(fullMessage);
} else {
// Otherwise it's a function callback
this.config.logCallback('log', fullMessage);
}
return;
}
- // Otherwise, respect silent/mcpMode
- if (this.config.silent || this.config.mcpMode) return;
+ // Otherwise, respect mcpMode
+ if (this.config.mcpMode) return;Also applies to: 283-301
🤖 Prompt for AI Agents
In packages/tm-core/src/common/logger/logger.ts around lines 119-126 (and
similarly lines 283-301), the current logic lets a provided logCallback bypass
the silent flag so log() still emits in silent mode; change shouldLog to have
silent and mcpMode checked first and return false immediately if either is true,
then only if not silent consider honoring logCallback and level—i.e., evaluate
this.config.silent || this.config.mcpMode before checking logCallback and ensure
the callback path still enforces level and silent semantics so raw logs never
emit when silent is true.
What type of PR is this?
Description
Related Issues
How to Test This
# Example commands or stepsExpected result:
Contributor Checklist
npm run changesetnpm testnpm run format-check(ornpm run formatto fix)Changelog Entry
For Maintainers
Summary by CodeRabbit
New Features
Refactor
Tests