feat: implement local-only command checkers for cli and mcp#1426
Conversation
|
WalkthroughAdds an auth guard for local-only CLI commands, integrates it into the CLI preAction hook, refactors many MCP and mcp-server tools to use a new ToolContext wrapper and centralized Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Program
participant Guard as checkAndBlockIfAuthenticated
participant TmCore as TmCore.auth
participant CardBox as displayCardBox
User->>CLI: Run command (e.g., add-dependency)
CLI->>Guard: preAction calls guard
Guard->>TmCore: auth.guardCommand(commandName)
alt Blocked (authenticated + API storage)
TmCore-->>Guard: { isBlocked: true, briefName }
Guard->>CardBox: render cardBox (header/body/footer)
CardBox-->>User: display block message
Guard-->>CLI: return blocked -> exit 1
else Allowed
TmCore-->>Guard: { isBlocked: false }
Guard-->>CLI: return allowed -> continue
end
sequenceDiagram
participant Server as MCP Server
participant Context as withToolContext
participant TmCore as TmCore Instance
participant Guard as tmCore.auth.guardCommand
participant Tool as Tool executeFn
participant Workflow as Workflow Service
participant Log as Logger
Server->>Context: Tool invoked
Context->>TmCore: ensure tmCore instance available
Context->>Guard: auth.guardCommand(toolName)
alt Blocked
Context-->>Server: return formatted error (blocked)
else Allowed
Context->>Workflow: hasWorkflow()
alt No workflow
Context-->>Server: return early error
else Workflow exists
Context->>Workflow: resumeWorkflow()
Context->>Tool: call executeFn with ToolContext
Tool->>Log: use injected logger
Tool->>Workflow: perform operations
Tool-->>Context: return result
Context-->>Server: format/send response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to inspect closely:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
0e0de2f to
2e9e011
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
mcp-server/src/tools/add-tag.js (1)
7-11: Update add-tag.js to usewithToolContextpattern like all peer MCP tools.The file is missing the ToolContext migration that's been applied to all other tools (fix-dependencies, validate-dependencies, remove-dependency, models.js, clear-subtasks, add-dependency).
Update lines 7-11 to import
withToolContextinstead ofwithNormalizedProjectRoot, and update line 53 to usewithToolContext('add-tag', ...)as the wrapper, matching the pattern across all other MCP tools.mcp-server/src/tools/remove-subtask.js (1)
7-11: Fix critical import error:withNormalizedProjectRootis not exported from@tm/mcp.Line 11 attempts to import
withNormalizedProjectRootfrom@tm/mcp, but it's not exported there. The function is defined locally inmcp-server/src/tools/utils.js(line 686). This import will fail at runtime.Two solutions:
- Import locally: Change line 11 to import from
./utils.jsinstead of@tm/mcp- Complete the refactor: Update this tool to use
withToolContext(from@tm/mcp) like the other tools that have already been migrated (validate-dependencies, remove-dependency, models, fix-dependencies, clear-subtasks, add-dependency)The second approach aligns with the PR's apparent refactor strategy, as 7 other tools already use the new
withToolContextpattern.mcp-server/src/tools/scope-down.js (1)
7-11: Fix handleApiResult call signatures across all mcp-server tools.The migration to @tm/mcp exports was successful—all three functions are properly exported. However, there's a critical signature mismatch: the imported
handleApiResultfunction expects an object parameter{result, log?, errorPrefix?, projectRoot?, tag?}, but all call sites in mcp-server/src/tools/ still use the old positional argument patternhandleApiResult(result, log, errorPrefix, projectRoot, tag).This affects 30+ tool files including scope-down.js (lines 91-97), use-tag.js, update.js, update-task.js, update-subtask.js, set-task-status.js, scope-up.js, rules.js, and many others. All these calls will fail at runtime with a type mismatch.
Update all
handleApiResultcall sites to use object syntax:handleApiResult({ result, log, errorPrefix: 'Error message', projectRoot: args.projectRoot })instead of:
handleApiResult(result, log, 'Error message', undefined, args.projectRoot)mcp-server/src/tools/rules.js (1)
7-11: Import refactor in rules.js is correct and complete. All three functions are properly exported from@tm/mcpwith matching signatures.The refactor has already been successfully applied to
mcp-server/src/tools/rules.js, which correctly importscreateErrorResponse,handleApiResult, andwithNormalizedProjectRootfrom@tm/mcp. These functions are properly exported by the@tm/mcppackage viaapps/mcp/src/shared/utils.ts.However, the old
mcp-server/src/tools/utils.jsfile should be removed, as it is no longer used by any files in the codebase and is dead code left over from the refactoring.mcp-server/src/tools/move-task.js (1)
184-197: Pre-existing critical issue: Unreachable code with undefined variable references.This code block is unreachable because it follows a return statement at line 182. Additionally, it references undefined variables
resultsandskippedMovesthat would cause a runtime error if this code were ever reached. The variables should beresultsandskippedbased on the logic at lines 133 and 169-182.This appears to be dead code from an incomplete refactoring. Apply this diff to remove the unreachable block:
); } - return handleApiResult( - { - success: true, - data: { - moves: results, - skippedMoves: skippedMoves, - message: `Successfully moved ${results.length} tasks${skippedMoves.length > 0 ? `, skipped ${skippedMoves.length} moves` : ''}` - } - }, - log, - 'Error moving multiple tasks', - undefined, - args.projectRoot - ); } else {mcp-server/src/tools/update.js (1)
7-11: Fix function call signatures - incompatible with migrated @tm/mcp API.The import change is valid, but the
handleApiResultfunction signature has changed from positional parameters to an object parameter. Line 91 and similar calls throughout mcp-server/src/tools/ will fail at runtime.Change from:
return handleApiResult( result, log, 'Error updating tasks', undefined, args.projectRoot );To:
return handleApiResult({ result, log, errorPrefix: 'Error updating tasks', projectRoot: args.projectRoot });This applies to all 35 tool files that migrated to
@tm/mcp—each call tohandleApiResultmust be updated to use the new object-based signature.mcp-server/src/tools/next-task.js (1)
7-11: API breaking change:handleApiResultexpects options object but called with positional parameters.The import has been changed to use
@tm/mcp, but the function calls haven't been updated. On line 85,handleApiResultis called with positional parameters:handleApiResult(result, log, 'Error finding next task', undefined, args.projectRoot)However,
@tm/mcp'shandleApiResultsignature expects an options object:handleApiResult({ result, log, errorPrefix?, projectRoot?, tag? })This mismatch will cause a runtime error. Update line 85 to:
return handleApiResult({ result, log, errorPrefix: 'Error finding next task', projectRoot: args.projectRoot });Similarly, verify line 55 (another
handleApiResultcall based on grep output) uses the correct options object format.mcp-server/src/tools/add-task.js (1)
91-91: Fix typo in comment.The comment contains a typo: "functionP" should be "function".
Apply this diff:
- // Call the direct functionP + // Call the direct functionmcp-server/src/tools/fix-dependencies.js (1)
7-81: UpdatehandleApiResultcall to match new options-object APIAs with
remove-dependency, this tool still uses the old positionalhandleApiResultsignature:return handleApiResult( result, context.log, 'Error fixing dependencies', undefined, args.projectRoot );But the shared implementation now expects a single options object. Adjust to:
- return handleApiResult( - result, - context.log, - 'Error fixing dependencies', - undefined, - args.projectRoot - ); + return handleApiResult({ + result, + log: context.log, + errorPrefix: 'Error fixing dependencies', + projectRoot: args.projectRoot + });This keeps behavior the same while aligning with the new helper.
mcp-server/src/tools/clear-subtasks.js (1)
42-100: FixhandleApiResultpositional call to use options objectThis tool also uses the old positional
handleApiResultsignature:return handleApiResult( result, context.log, 'Error clearing subtasks', undefined, args.projectRoot );With the shared implementation expecting an options object, this will break. Update it to:
- return handleApiResult( - result, - context.log, - 'Error clearing subtasks', - undefined, - args.projectRoot - ); + return handleApiResult({ + result, + log: context.log, + errorPrefix: 'Error clearing subtasks', + projectRoot: args.projectRoot + });so it matches the new helper contract.
mcp-server/src/tools/remove-dependency.js (1)
6-88: Based on the review comment and the extensive search results showing ~40 call sites across the MCP server tools, I need to verify the actual implementation. Let me check the specific implementation in the codebase:Based on my analysis, I've discovered significant evidence supporting the review comment but cannot definitively verify the exact implementation in
@tm/mcpwithout direct codebase access. The verification revealed:
- 40+ call sites across all MCP server tools use the old positional argument signature
- All tools import
handleApiResultfrom@tm/mcp(not local utils)- The pattern is consistent across the codebase—every file uses identical positional args
However, I cannot execute code or directly inspect the
apps/mcp/src/shared/utils.tsexport to confirm the new object signature with absolute certainty. Given that this appears to be an internal monorepo package, I need to mark this for manual verification while confirming the broader systematic impact.Rewritten Review Comment:
Critical:
handleApiResultcalls throughout MCP server tools use incompatible old signature (40+ sites affected)The exported
handleApiResultfrom@tm/mcpexpects a single options object:handleApiResult({ result, log, errorPrefix?, projectRoot?, tag? })However, approximately 40 call sites across all MCP tools (including
remove-dependency.js:79-81) still use the old positional argument pattern:handleApiResult(result, context.log, 'Error message', undefined, args.projectRoot)This is a systemic issue. Fix all occurrences by converting to object form. Example for
remove-dependency.js:- return handleApiResult( - result, - context.log, - 'Error removing dependency', - undefined, - args.projectRoot - ); + return handleApiResult({ + result, + log: context.log, + errorPrefix: 'Error removing dependency', + projectRoot: args.projectRoot + });Affected files (sample):
mcp-server/src/tools/validate-dependencies.js:73mcp-server/src/tools/use-tag.js:62mcp-server/src/tools/update.js:91mcp-server/src/tools/update-task.js:94- ...and ~35 more
🧹 Nitpick comments (9)
mcp-server/src/tools/get-operation-status.js (1)
3-3: Keep the new centralized import, but drop the speculative commentSwitching to
@tm/mcpforcreateErrorResponse/createContentResponsealigns with the shared MCP utilities pattern and looks good, assuming the package exports stay stable. The inline// Assuming these utils existcomment now reads like scaffolding / uncertainty and can confuse future readers; consider removing it and letting the import stand on its own.packages/tm-core/src/modules/auth/auth-domain.ts (1)
19-20: guardCommand correctly wraps the core auth guardThe new guardCommand wires AuthDomain into the pure checkAuthBlock logic cleanly: it derives hasValidSession and briefName once and returns an AuthBlockResult, matching the documented examples and intended behavior for Hamster/API storage blocking.
You may want to narrow the storageType parameter to a resolved type (e.g.,
Exclude<StorageType, 'auto'>or'file' | 'api') to encode at the type level that guardCommand expects the actual runtime storage mode rather than the config value. This isn’t a blocker, just a small safety/clarity improvement.Also applies to: 229-262
scripts/modules/commands.js (1)
6-7: Global preAction guard integrates core auth logic cleanlyThe new preAction hook that:
- filters by
LOCAL_ONLY_COMMANDS.includes(commandName),- initializes TaskMaster once via
initTaskMaster(actionCommand.opts()),- and delegates to
checkAndBlockIfAuthenticated(commandName, taskMaster.getProjectRoot()),gives you a single, centralized gate for all local-only commands across the legacy CLI layer.
Behaviorally this lines up with the tm-core
guardCommandsemantics (block only when authenticated on API storage) and avoids sprinkling auth checks into each individual handler.One nuance to be aware of:
initTaskMasterhere receives the raw parsed options from Commander, whereas individual command actions often construct a more specific override object (e.g.,{ tasksPath: options.file || true, tag: options.tag }). Since the guard only needs a reliableprojectRootfor tm-core, this is fine today, but ifinitTaskMaster’s behavior around options changes in the future, keep in mind that the guard’s initialization path is slightly different from the per-command initialization.If you ever see divergence between guard behavior and per-command behavior, consider sharing a small helper that derives the common initTaskMaster options for local-only commands, so both the guard and the handlers construct overrides in one place.
Also applies to: 9-11, 66-68, 160-175
packages/tm-core/src/modules/auth/command.guard.ts (1)
6-8: Core command-guard logic is correct and nicely isolatedThe isLocalOnlyCommand and checkAuthBlock implementations:
- centralize local-only detection via LOCAL_ONLY_COMMANDS,
- only block when the user is authenticated and storageType is
'api',- and otherwise return a simple AuthBlockResult without any presentation concerns.
This matches the documented behavior and is easy to consume from AuthDomain and higher layers.
Similar to AuthDomain, you might consider narrowing
AuthBlockParams['storageType']to a resolved variant (e.g.,Exclude<StorageType, 'auto'>) so that callers are nudged to pass the actual runtime storage mode rather than a configuration value. Not urgent, but it would make misuses harder.Also applies to: 12-42, 53-77
apps/cli/src/utils/command-guard.ts (3)
19-86: Local-only command messaging categories look solid; consider DRY-ing the repeated footerThe hardcoded grouping of dependency/subtask/model commands with a default fallback is clear and correctly constrained to
LocalOnlyCommandvia the default branch. You also keep the auth/source-of-truth separation well documented.If you touch this again, you might pull the shared footer string into a small constant to avoid future wording drift across categories, but that’s purely optional.
106-141: Avoid the uncheckedas LocalOnlyCommandcast by using a type guard (if available)
guardCommandis expected to only blockLOCAL_ONLY_COMMANDS, so the cast toLocalOnlyCommandis effectively safe today, but it does rely on that invariant never changing.If
@tm/corealready exposes anisLocalOnlyCommandtype guard, you could make this a bit more future-proof and self-documenting:-import { createTmCore, type TmCore, type LocalOnlyCommand } from '@tm/core'; +import { + createTmCore, + type TmCore, + type LocalOnlyCommand, + isLocalOnlyCommand +} from '@tm/core'; // ... if (result.isBlocked) { - // Get command-specific message configuration - // Safe to cast: guardCommand only blocks commands in LOCAL_ONLY_COMMANDS - const message = getCommandMessage(commandName as LocalOnlyCommand); + // Get command-specific message configuration + // Narrow to LocalOnlyCommand defensively before calling getCommandMessage + const message = getCommandMessage( + isLocalOnlyCommand(commandName) ? commandName : 'models' // or another sensible fallback + );You’d likely substitute a more appropriate fallback than
'models'(or handle the unexpected case with a generic message), but the main idea is to remove the unchecked cast and let the type system enforce the invariant. This is a recommended, not blocking, refactor.
143-144: Mark the legacy alias as deprecated for downstream consumersThe legacy export is a good compatibility shim. To make migration easier in editors/IDE tooling, you could annotate it as deprecated:
-// Legacy export for backward compatibility -export const checkAndBlockDependencyCommand = checkAndBlockIfAuthenticated; +// Legacy export for backward compatibility +/** + * @deprecated Use {@link checkAndBlockIfAuthenticated} instead. + */ +export const checkAndBlockDependencyCommand = checkAndBlockIfAuthenticated;This keeps the runtime behavior identical while gently nudging callers to the new name.
apps/mcp/src/tools/tasks/get-task.tool.ts (1)
43-122: Get-task ToolContext wiring looks correct; optional ID sanitizationThe migration to
withToolContextandtmCore.tasks.getusage looks good, andhandleApiResultis called with the new options object correctly. If you want to harden edge cases, you could ignore empty IDs when splitting comma-separated lists:- const taskIds = id.split(',').map((tid) => tid.trim()); + const taskIds = id + .split(',') + .map((tid) => tid.trim()) + .filter(Boolean);This avoids calling
tmCore.tasks.get('')if the input contains stray commas.apps/mcp/src/tools/autopilot/commit.tool.ts (1)
33-231: Commit tool flow looks robust; minor unreachable branch in commit-type selectionThe move to
withToolContext, the COMMIT-phase / subtask / staged-changes checks, and the git + workflow interactions all look solid, andhandleApiResult({ ... })is used correctly.One tiny simplification opportunity: after enforcing
status.tddPhase === 'COMMIT', thetypeselection:const type = status.tddPhase === 'COMMIT' ? 'feat' : 'test';will always pick
'feat'. You could either hardcode'feat'here or relax the phase check if you intend to support non‑COMMIT commits later.
| import { z } from 'zod'; | ||
| import type { ToolContext } from '../../shared/types.js'; | ||
| import { handleApiResult, withToolContext } from '../../shared/utils.js'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
ToolContext migration and workflow guards look solid; please confirm command name alignment
The switch to withToolContext plus handleApiResult is consistent with the shared utilities, and the hasWorkflow + resumeWorkflow + getNextAction flow is coherent. Logging via log and shaping the success/error payloads through handleApiResult match the newer MCP patterns.
The only thing to double‑check is that the 'autopilot-next' string you pass into withToolContext matches whatever is in LOCAL_ONLY_COMMANDS (and any corresponding CLI command identifiers), otherwise the new local‑only auth guard won’t trigger for this tool even though it’s wired through the wrapper.
Also applies to: 29-90
🏁 Script executed:
#!/bin/bash
# Verify that the MCP tool name matches the local-only command guard list
rg -n "LOCAL_ONLY_COMMANDS" -C3
rg -n "autopilot-next" -C3Length of output: 7518
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract the complete LOCAL_ONLY_COMMANDS array definition
cat packages/tm-core/src/modules/auth/constants.ts | sed -n '/export const LOCAL_ONLY_COMMANDS/,/\] as const;/p'
Length of output: 165
Add 'autopilot-next' to LOCAL_ONLY_COMMANDS to align with local-only auth guard
The tool is wrapped with withToolContext('autopilot-next', ...) and uses the shared utilities auth guard, but 'autopilot-next' is not present in the LOCAL_ONLY_COMMANDS array. This means the local-only auth guard in apps/mcp/src/shared/utils.ts (line 405) will never trigger for this tool, defeating the intent of the local-only protection.
Add 'autopilot-next' to the LOCAL_ONLY_COMMANDS array in packages/tm-core/src/modules/auth/constants.ts.
🤖 Prompt for AI Agents
In packages/tm-core/src/modules/auth/constants.ts (update LOCAL_ONLY_COMMANDS),
add the string 'autopilot-next' to the LOCAL_ONLY_COMMANDS array so the
local-only auth guard will recognize the tool; modify the array literal to
include 'autopilot-next' alongside the existing entries and run tests/lint to
ensure no formatting or typing errors.
There was a problem hiding this comment.
I wrapped everything with withToolContext, but only certain commands that are in the local only commands list are local only commands. autopilot-next not being one of them
There was a problem hiding this comment.
@Crunchyman-ralph, thanks for clarifying! You're absolutely right. I see now that withToolContext serves as general infrastructure for all MCP tools—providing the context, tmCore instance, and logging—while the auth guard within it only blocks commands that are actually in the LOCAL_ONLY_COMMANDS list.
Since autopilot-next is intentionally not in that list (it should be available when authenticated), the current implementation is correct. The wrapper provides the enhanced context without imposing unnecessary restrictions.
My apologies for the false positive!
✏️ Learnings added
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1426
File: apps/mcp/src/tools/autopilot/next.tool.ts:8-11
Timestamp: 2025-11-19T18:45:16.641Z
Learning: In the MCP server tools, withToolContext is used as a universal wrapper for all tools to provide consistent context (log, tmCore, session), not just for local-only commands. The auth guard within withToolContext only blocks commands that are explicitly listed in LOCAL_ONLY_COMMANDS; other tools like autopilot commands remain available when authenticated.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/commands.mdc:0-0
Timestamp: 2025-07-18T17:08:48.695Z
Learning: Applies to scripts/modules/commands.js : For AI-powered commands that benefit from project context, use the ContextGatherer utility for multi-source context extraction, support task IDs, file paths, custom context, and project tree, implement fuzzy search for automatic task discovery, and display detailed token breakdown for transparency.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/fix-tag-complexity-detection.md:0-0
Timestamp: 2025-08-02T15:33:22.656Z
Learning: For changeset files (.changeset/*.md), Crunchyman-ralph prefers to ignore formatting nitpicks about blank lines between frontmatter and descriptions, as he doesn't mind having them and wants to avoid such comments in future reviews.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1105
File: scripts/modules/supported-models.json:242-254
Timestamp: 2025-08-08T11:33:15.297Z
Learning: Preference: In scripts/modules/supported-models.json, the "name" field is optional. For OpenAI entries (e.g., "gpt-5"), Crunchyman-ralph prefers omitting "name" when the id is explicit enough; avoid nitpicks requesting a "name" in such cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1200
File: src/ai-providers/custom-sdk/grok-cli/language-model.js:96-100
Timestamp: 2025-09-19T16:06:42.182Z
Learning: The user Crunchyman-ralph prefers to keep environment variable names explicit (like GROK_CLI_API_KEY) rather than supporting multiple aliases, to avoid overlap and ensure clear separation between different CLI implementations.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1217
File: apps/cli/src/index.ts:16-21
Timestamp: 2025-09-18T16:35:35.147Z
Learning: The user Crunchyman-ralph considers suggestions to export types for better ergonomics (like exporting UpdateInfo type alongside related functions) as nitpicky and prefers not to implement such suggestions.
Learnt from: eyaltoledano
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/floppy-news-buy.md:7-38
Timestamp: 2025-08-02T14:54:52.216Z
Learning: For major feature additions like new CLI commands, eyaltoledano prefers detailed changesets with comprehensive descriptions, usage examples, and feature explanations rather than minimal single-line summaries.
dba18af to
1d8a171
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mcp-server/src/tools/add-dependency.js(2 hunks)mcp-server/src/tools/response-language.js(2 hunks)mcp-server/src/tools/rules.js(2 hunks)mcp-server/src/tools/validate-dependencies.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp-server/src/tools/rules.js
🧰 Additional context used
🧠 Learnings (26)
📓 Common learnings
Learnt from: eyaltoledano
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/floppy-news-buy.md:7-38
Timestamp: 2025-08-02T14:54:52.216Z
Learning: For major feature additions like new CLI commands, eyaltoledano prefers detailed changesets with comprehensive descriptions, usage examples, and feature explanations rather than minimal single-line summaries.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1426
File: apps/mcp/src/tools/autopilot/next.tool.ts:8-11
Timestamp: 2025-11-19T18:45:16.641Z
Learning: In the MCP server tools, withToolContext is used as a universal wrapper for all tools to provide consistent context (log, tmCore, session), not just for local-only commands. The auth guard within withToolContext only blocks commands that are explicitly listed in LOCAL_ONLY_COMMANDS; other tools like autopilot commands remain available when authenticated.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/fix-tag-complexity-detection.md:0-0
Timestamp: 2025-08-02T15:33:22.656Z
Learning: For changeset files (.changeset/*.md), Crunchyman-ralph prefers to ignore formatting nitpicks about blank lines between frontmatter and descriptions, as he doesn't mind having them and wants to avoid such comments in future reviews.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1105
File: scripts/modules/supported-models.json:242-254
Timestamp: 2025-08-08T11:33:15.297Z
Learning: Preference: In scripts/modules/supported-models.json, the "name" field is optional. For OpenAI entries (e.g., "gpt-5"), Crunchyman-ralph prefers omitting "name" when the id is explicit enough; avoid nitpicks requesting a "name" in such cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1200
File: src/ai-providers/custom-sdk/grok-cli/language-model.js:96-100
Timestamp: 2025-09-19T16:06:42.182Z
Learning: The user Crunchyman-ralph prefers to keep environment variable names explicit (like GROK_CLI_API_KEY) rather than supporting multiple aliases, to avoid overlap and ensure clear separation between different CLI implementations.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1217
File: apps/cli/src/index.ts:16-21
Timestamp: 2025-09-18T16:35:35.147Z
Learning: The user Crunchyman-ralph considers suggestions to export types for better ergonomics (like exporting UpdateInfo type alongside related functions) as nitpicky and prefers not to implement such suggestions.
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Track and report changes made during dependency cleanup
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-09-26T19:03:33.225Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: package.json:130-132
Timestamp: 2025-09-26T19:03:33.225Z
Learning: In the eyaltoledano/claude-task-master repository, packages are bundled using tsdown during the build process, which means dependencies imported by the source code (including tm internal packages like tm/ai-sdk-provider-grok-cli) are included in the final bundle and don't need to be available as separate runtime dependencies, so they should remain as devDependencies rather than being moved to dependencies.
Applied to files:
mcp-server/src/tools/add-dependency.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Format task and dependency IDs consistently when adding dependencies
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Handle both direct and indirect self-dependencies
Applied to files:
mcp-server/src/tools/add-dependency.js
📚 Learning: 2025-10-12T11:06:36.374Z
Learnt from: rtmcrc
Repo: eyaltoledano/claude-task-master PR: 933
File: mcp-server/src/tools/agent-llm.js:1-3
Timestamp: 2025-10-12T11:06:36.374Z
Learning: The agent_llm MCP tool in mcp-server/src/tools/agent-llm.js is an exception to the standard MCP tool pattern. It should NOT use handleApiResult because it handles delegated LLM calls rather than direct API calls. Using createErrorResponse for error messages is the correct approach for this tool.
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.jsmcp-server/src/tools/response-language.js
📚 Learning: 2025-07-17T21:33:57.585Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1002
File: .changeset/puny-friends-give.md:2-3
Timestamp: 2025-07-17T21:33:57.585Z
Learning: In the eyaltoledano/claude-task-master repository, the MCP server code in mcp-server/src/ is part of the main "task-master-ai" package, not a separate "mcp-server" package. When creating changesets for MCP server changes, use "task-master-ai" as the package name.
Applied to files:
mcp-server/src/tools/add-dependency.js
📚 Learning: 2025-09-26T19:28:42.612Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:21-35
Timestamp: 2025-09-26T19:28:42.612Z
Learning: Successfully implemented dependency safety system for Crunchyman-ralph using Python-based validation that integrates with their build process. The system prevents production failures by catching missing external dependencies during build time and provides clear error messages directing developers to add missing dependencies to root package.json. Peer dependencies were removed from internal packages as they're unnecessary with their tsdown bundling setup.
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Format dependency lists consistently for visualization
Applied to files:
mcp-server/src/tools/add-dependency.js
📚 Learning: 2025-09-26T19:07:10.485Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:21-35
Timestamp: 2025-09-26T19:07:10.485Z
Learning: In the eyaltoledano/claude-task-master repository, the tsdown build configuration uses `noExternal: [/^tm\//]` which means internal tm/ packages are bundled into the final output while external npm dependencies remain external and are resolved from the root package.json dependencies at runtime. This eliminates the need for peer dependencies in internal packages since the root package.json already provides the required external dependencies.
Applied to files:
mcp-server/src/tools/add-dependency.js
📚 Learning: 2025-09-26T19:28:42.612Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:21-35
Timestamp: 2025-09-26T19:28:42.612Z
Learning: Crunchyman-ralph is concerned about preventing production failures when developers add external dependencies to internal packages but forget to add them to the root package.json in their tsdown bundling setup. He wants automated safety mechanisms to catch this during development or CI rather than discovering it in production.
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Provide feedback about the removal result
Applied to files:
mcp-server/src/tools/add-dependency.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Validate that referenced tasks exist before adding dependencies
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Do not create circular dependencies between subtasks
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Prevent tasks from depending on themselves
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Represent task dependencies as arrays of task IDs
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Support both task and subtask dependencies in cycle detection
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:08:48.695Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/commands.mdc:0-0
Timestamp: 2025-07-18T17:08:48.695Z
Learning: Applies to scripts/modules/commands.js : Follow the provided structure for adding subtasks, including required options and detailed error handling.
Applied to files:
mcp-server/src/tools/add-dependency.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Remove references to non-existent tasks during validation
Applied to files:
mcp-server/src/tools/add-dependency.jsmcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:08:48.695Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/commands.mdc:0-0
Timestamp: 2025-07-18T17:08:48.695Z
Learning: Applies to scripts/modules/commands.js : For AI-powered commands that benefit from project context, use the ContextGatherer utility for multi-source context extraction, support task IDs, file paths, custom context, and project tree, implement fuzzy search for automatic task discovery, and display detailed token breakdown for transparency.
Applied to files:
mcp-server/src/tools/add-dependency.js
📚 Learning: 2025-07-18T05:38:17.352Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 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:
mcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-09-26T19:28:42.612Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:21-35
Timestamp: 2025-09-26T19:28:42.612Z
Learning: Crunchyman-ralph's dependency safety system is now implemented using a Python-based validator that checks external dependencies in internal packages are available in root package.json. The system includes prebuild hooks, clear error messages, and automatic validation during the build process to prevent production failures from missing dependencies in their tsdown bundling setup.
Applied to files:
mcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T07:52:44.890Z
Learnt from: mm-parthy
Repo: eyaltoledano/claude-task-master PR: 943
File: scripts/modules/ui.js:1211-1211
Timestamp: 2025-07-18T07:52:44.890Z
Learning: The boundary-first tag resolution pattern means that context validation in UI functions like displayTaskById is unnecessary because tag and projectRoot resolution happens at the CLI command boundary layer before reaching these functions.
Applied to files:
mcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-11-19T18:45:16.641Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1426
File: apps/mcp/src/tools/autopilot/next.tool.ts:8-11
Timestamp: 2025-11-19T18:45:16.641Z
Learning: In the MCP server tools, withToolContext is used as a universal wrapper for all tools to provide consistent context (log, tmCore, session), not just for local-only commands. The auth guard within withToolContext only blocks commands that are explicitly listed in LOCAL_ONLY_COMMANDS; other tools like autopilot commands remain available when authenticated.
Applied to files:
mcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:09:40.548Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/dependencies.mdc:0-0
Timestamp: 2025-07-18T17:09:40.548Z
Learning: Applies to scripts/modules/dependency-manager.js : Check for and remove references to non-existent tasks during cleanup
Applied to files:
mcp-server/src/tools/validate-dependencies.js
📚 Learning: 2025-07-18T17:08:48.695Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/commands.mdc:0-0
Timestamp: 2025-07-18T17:08:48.695Z
Learning: Applies to scripts/modules/commands.js : Validate file existence for critical file operations, provide context-specific validation for identifiers, and check required API keys for features that depend on them.
Applied to files:
mcp-server/src/tools/validate-dependencies.js
🧬 Code graph analysis (3)
mcp-server/src/tools/add-dependency.js (3)
apps/mcp/src/shared/utils.ts (3)
withToolContext(389-436)createErrorResponse(49-78)handleApiResult(125-194)mcp-server/src/core/direct-functions/add-dependency.js (2)
args(26-26)addDependencyDirect(24-111)src/utils/path-utils.js (1)
findTasksPath(63-128)
mcp-server/src/tools/validate-dependencies.js (3)
apps/mcp/src/shared/utils.ts (3)
withToolContext(389-436)createErrorResponse(49-78)handleApiResult(125-194)mcp-server/src/core/direct-functions/validate-dependencies.js (2)
args(23-23)validateDependenciesDirect(21-83)src/utils/path-utils.js (1)
findTasksPath(63-128)
mcp-server/src/tools/response-language.js (2)
apps/mcp/src/shared/utils.ts (1)
handleApiResult(125-194)mcp-server/src/core/direct-functions/response-language.js (1)
args(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (3)
mcp-server/src/tools/add-dependency.js (1)
49-52: Fix the incorrectfindTasksPathcall signature.The
findTasksPathfunction signature isfindTasksPath(explicitPath, args, log), but the current call passes an object as the first parameter andlogas the second. This will cause the function to misinterpret the arguments at runtime.Apply this diff to fix the call:
- tasksJsonPath = findTasksPath( - { projectRoot: args.projectRoot, file: args.file }, - log - ); + tasksJsonPath = findTasksPath( + args.file, + { projectRoot: args.projectRoot }, + log + );⛔ Skipped due to learnings
Learnt from: CR Repo: eyaltoledano/claude-task-master PR: 0 File: .cursor/rules/dependencies.mdc:0-0 Timestamp: 2025-07-18T17:09:40.548Z Learning: Applies to scripts/modules/dependency-manager.js : Remove references to non-existent tasks during validationLearnt from: Crunchyman-ralph Repo: eyaltoledano/claude-task-master PR: 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.Learnt from: rtmcrc Repo: eyaltoledano/claude-task-master PR: 933 File: scripts/modules/task-manager/parse-prd.js:226-226 Timestamp: 2025-07-20T01:35:05.831Z Learning: The parsePRD function in scripts/modules/task-manager/parse-prd.js has a different parameter structure than other task-manager functions - it uses `options` parameter instead of `context` parameter because it generates tasks from PRD documents rather than operating on existing tasks.Learnt from: CR Repo: eyaltoledano/claude-task-master PR: 0 File: .cursor/rules/dependencies.mdc:0-0 Timestamp: 2025-07-18T17:09:40.548Z Learning: Applies to scripts/modules/dependency-manager.js : Check for and remove references to non-existent tasks during cleanupLearnt from: CR Repo: eyaltoledano/claude-task-master PR: 0 File: .cursor/rules/commands.mdc:0-0 Timestamp: 2025-07-18T17:08:48.695Z Learning: Applies to scripts/modules/commands.js : Use path.join() to construct file paths, follow established naming conventions (like task_001.txt), check file existence before deletion, and handle file deletion errors gracefully.mcp-server/src/tools/response-language.js (2)
6-6: LGTM! Import centralization aligns with repo-wide refactoring.The migration from local
./utils.jsto the centralized@tm/mcppackage is consistent with the broader refactoring effort mentioned in the PR summary.
39-44: Parameter migration correctly implemented.The refactoring from positional arguments to object parameters is properly implemented. All required parameters (result, log, errorPrefix, projectRoot) are correctly mapped to the function signature, and
handleApiResultis properly exported from@tm/mcppackage throughapps/mcp/src/shared/utils.ts.
This PR was automatically generated to update documentation based on recent changes. Original commit: feat: implement local-only command checkers for cli and mcp (#1426)\n\n\n Co-authored-by: Claude <claude-assistant@anthropic.com>
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
Improvements