fix(mcp): pass resolved tag through to handleApiResult in JS tools#1703
Open
gw1108 wants to merge 3 commits into
Open
fix(mcp): pass resolved tag through to handleApiResult in JS tools#1703gw1108 wants to merge 3 commits into
gw1108 wants to merge 3 commits into
Conversation
When an MCP tool was called with an explicit `tag` argument (e.g. next_task tag=phase3), the response payload still carried the `currentTag` from `.taskmaster/state.json` (usually `master`). The data was loaded from the right tag, but the response's `tag` field misled callers into thinking the operation ran on a different tag, which led to destructive corrective actions in multi-tag/multi-agent workspaces. The fix threads the already-resolved `tag` into handleApiResult in every JS-side MCP tool that wasn't already doing so, matching the existing pattern in add-dependency.js and the TS tools in @tm/mcp. Adds a regression test that pins state.json to master, calls next_task with tag=phase3, and asserts the response carries "tag": "phase3". Resolves upstream issue eyaltoledano#1683. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 8a72ef4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMCP tools now preserve an explicitly provided tag in responses by threading resolved tag into direct calls and into handleApiResult across tools; a changeset and an integration test verify the fix. ChangesMCP Tool Tag Response Fix
Sequence Diagram(s)sequenceDiagram
participant Inspector as MCP Inspector/Client
participant Tool as MCP Tool Handler
participant Direct as *Direct Function
participant Result as handleApiResult
Inspector->>Tool: call tools/call with tag argument
Tool->>Tool: resolveTag from args/projectRoot
Tool->>Direct: call *Direct(..., tag: resolvedTag)
Direct->>Tool: operation result
Tool->>Result: handleApiResult(result, { projectRoot, tag: resolvedTag })
Result->>Inspector: return response payload with tag field
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp-server/src/tools/move-task.js (1)
134-221:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix multi-move control flow and undefined variables in
move_task.Line 134 uses the mismatch condition to enter multi-move handling, which breaks valid multi-move calls and can reference undefined variables (
results,skippedMoves) in the fallback branch. Equal-length multi-ID input also incorrectly falls into the single-move path.Proposed fix
- // Validate matching IDs count - if (fromIds.length !== toIds.length) { - if (fromIds.length > 1) { + const isMultiMove = fromIds.length > 1 || toIds.length > 1; + if (isMultiMove) { + if (fromIds.length !== toIds.length) { + return createErrorResponse( + 'For multi-move operations, source and destination ID counts must match', + 'MISMATCHED_MOVE_COUNTS' + ); + } const results = []; const skipped = []; // Move tasks one by one, only generate files on the last move for (let i = 0; i < fromIds.length; i++) { @@ return handleApiResult({ result: { success: true, data: { moves: results, skipped: skipped.length > 0 ? skipped : undefined, message: `Successfully moved ${results.length} tasks${skipped.length > 0 ? `, skipped ${skipped.length}` : ''}` } }, log, errorPrefix: 'Error moving multiple tasks', projectRoot: args.projectRoot, tag: resolvedTag }); - } - return handleApiResult({ - result: { - success: true, - data: { - moves: results, - skippedMoves: skippedMoves, - message: `Successfully moved ${results.length} tasks${skippedMoves.length > 0 ? `, skipped ${skippedMoves.length} moves` : ''}` - } - }, - log, - errorPrefix: 'Error moving multiple tasks', - projectRoot: args.projectRoot, - tag: resolvedTag - }); - } else { - // Moving a single task - return handleApiResult({ - result: await moveTaskDirect( - { - sourceId: args.from, - destinationId: args.to, + } + + // Moving a single task + return handleApiResult({ + result: await moveTaskDirect( + { + sourceId: fromIds[0], + destinationId: toIds[0], tasksJsonPath, projectRoot: args.projectRoot, tag: resolvedTag, generateFiles: true - }, - log, - { session } - ), - log, - errorPrefix: 'Error moving task', - projectRoot: args.projectRoot, - tag: resolvedTag - }); - } + }, + log, + { session } + ), + log, + errorPrefix: 'Error moving task', + projectRoot: args.projectRoot, + tag: resolvedTag + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp-server/src/tools/move-task.js` around lines 134 - 221, The multi-move branch is entered when fromIds.length !== toIds.length which is backwards and also references undefined variables (results, skippedMoves); change the control flow so that when fromIds.length === toIds.length && fromIds.length > 1 you iterate and call moveTaskDirect for each pair (use local arrays results and skipped), return handleApiResult with data.moves and data.skipped (no skippedMoves), add a separate branch that returns a clear error via handleApiResult when fromIds.length !== toIds.length (mismatched counts), and fall back to the single-move path that calls moveTaskDirect with args.from/args.to when there is exactly one id; update any variable names to consistently use results and skipped and reference moveTaskDirect and handleApiResult accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mcp/tests/integration/tools/next-task-tag.tool.test.ts`:
- Around line 38-41: The tests currently call execSync with a shell command
string using cliPath which is injection-prone; replace those calls with
execFileSync using the binary path and an argument array (e.g.,
execFileSync(cliPath, ['init', '--yes'], { stdio: 'pipe', env: { ...process.env,
TASKMASTER_SKIP_AUTO_UPDATE: '1' } })) and do the same for the other occurrence
(the command at lines ~106-109) so both invocations use execFileSync(cliPath,
argsArray, options) instead of execSync(shellString, options).
---
Outside diff comments:
In `@mcp-server/src/tools/move-task.js`:
- Around line 134-221: The multi-move branch is entered when fromIds.length !==
toIds.length which is backwards and also references undefined variables
(results, skippedMoves); change the control flow so that when fromIds.length ===
toIds.length && fromIds.length > 1 you iterate and call moveTaskDirect for each
pair (use local arrays results and skipped), return handleApiResult with
data.moves and data.skipped (no skippedMoves), add a separate branch that
returns a clear error via handleApiResult when fromIds.length !== toIds.length
(mismatched counts), and fall back to the single-move path that calls
moveTaskDirect with args.from/args.to when there is exactly one id; update any
variable names to consistently use results and skipped and reference
moveTaskDirect and handleApiResult accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac0012d9-a67b-4cd4-8058-724b0060cacb
📒 Files selected for processing (22)
.changeset/fix-mcp-response-tag.mdapps/mcp/tests/integration/tools/next-task-tag.tool.test.tsmcp-server/src/tools/add-subtask.jsmcp-server/src/tools/add-task.jsmcp-server/src/tools/analyze.jsmcp-server/src/tools/clear-subtasks.jsmcp-server/src/tools/expand-all.jsmcp-server/src/tools/expand-task.jsmcp-server/src/tools/fix-dependencies.jsmcp-server/src/tools/move-task.jsmcp-server/src/tools/next-task.jsmcp-server/src/tools/parse-prd.jsmcp-server/src/tools/remove-dependency.jsmcp-server/src/tools/remove-subtask.jsmcp-server/src/tools/remove-task.jsmcp-server/src/tools/research.jsmcp-server/src/tools/scope-down.jsmcp-server/src/tools/scope-up.jsmcp-server/src/tools/set-task-status.jsmcp-server/src/tools/update-subtask.jsmcp-server/src/tools/update-task.jsmcp-server/src/tools/update.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/mcp/tests/integration/tools/next-task-tag.tool.test.ts (1)
14-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
execFileSyncwith argument arrays instead of shell-interpolatedexecSync.Line 38 and Line 102 build shell command strings with interpolation. Even in tests, this is fragile and can become injection-prone when inputs change. Prefer argument arrays.
Suggested patch
-import { execSync } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; @@ - execSync(`node "${cliPath}" init --yes`, { + execFileSync('node', [cliPath, 'init', '--yes'], { stdio: 'pipe', env: { ...process.env, TASKMASTER_SKIP_AUTO_UPDATE: '1' } }); @@ - const toolArgs = Object.entries(args) - .map(([key, value]) => `--tool-arg ${key}=${value}`) - .join(' '); + const toolArgs = Object.entries(args).flatMap(([key, value]) => [ + '--tool-arg', + `${key}=${value}` + ]); - const output = execSync( - `npx `@modelcontextprotocol/inspector` --cli node "${mcpServerPath}" --method tools/call --tool-name ${toolName} ${toolArgs}`, - { encoding: 'utf-8', stdio: 'pipe' } - ); + const output = execFileSync( + 'npx', + [ + '`@modelcontextprotocol/inspector`', + '--cli', + 'node', + mcpServerPath, + '--method', + 'tools/call', + '--tool-name', + toolName, + ...toolArgs + ], + { encoding: 'utf-8', stdio: 'pipe' } + );#!/bin/bash # Verify remaining risky command construction in the updated file. rg -nP -C2 'execSync\(|execFileSync\(' apps/mcp/tests/integration/tools/next-task-tag.tool.test.tsAlso applies to: 38-41, 96-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mcp/tests/integration/tools/next-task-tag.tool.test.ts` at line 14, The tests currently use node:child_process.execSync with shell-interpolated command strings (see usages of execSync in the file) which is fragile and injection-prone; replace those with execFileSync by importing execFileSync from 'node:child_process' and call execFileSync with the executable as the first argument and an array of arguments instead of a concatenated shell string (preserve any options like { encoding: 'utf8', stdio: 'pipe' } or { stdio: 'inherit' } as used), updating the two places that build commands (the blocks around the current execSync calls) to pass each dynamic value as a separate array element so no shell interpolation is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mcp/tests/integration/tools/next-task-tag.tool.test.ts`:
- Around line 76-82: Replace the direct fs.writeFileSync calls with the
project's JSON utility: import and use writeJSON to write tasksPath with data
and statePath with { currentTag: 'master' } (instead of fs.writeFileSync).
Ensure you add error handling around the write operations (catch/rethrow or
assert) and, after writing, optionally read back via readJSON to validate the
written structure matches expectations (e.g., tasks shape and state.currentTag)
so the test follows the project's readJSON/writeJSON validation conventions.
---
Duplicate comments:
In `@apps/mcp/tests/integration/tools/next-task-tag.tool.test.ts`:
- Line 14: The tests currently use node:child_process.execSync with
shell-interpolated command strings (see usages of execSync in the file) which is
fragile and injection-prone; replace those with execFileSync by importing
execFileSync from 'node:child_process' and call execFileSync with the executable
as the first argument and an array of arguments instead of a concatenated shell
string (preserve any options like { encoding: 'utf8', stdio: 'pipe' } or {
stdio: 'inherit' } as used), updating the two places that build commands (the
blocks around the current execSync calls) to pass each dynamic value as a
separate array element so no shell interpolation is needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17054793-bc7d-423d-ae3f-8ba6f97d5cce
📒 Files selected for processing (1)
apps/mcp/tests/integration/tools/next-task-tag.tool.test.ts
Comment on lines
+76
to
+82
| fs.writeFileSync(tasksPath, JSON.stringify(data, null, 2)); | ||
|
|
||
| // Pin currentTag to master so we can detect a fallback bug | ||
| fs.writeFileSync( | ||
| statePath, | ||
| JSON.stringify({ currentTag: 'master' }, null, 2) | ||
| ); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace direct JSON writes with the project JSON utility.
Line 76 and Line 79 use fs.writeFileSync for JSON. This bypasses the shared JSON utility and its validation/error-handling conventions.
As per coding guidelines, "Use readJSON and writeJSON utilities for all JSON file operations instead of raw fs.readFileSync or fs.writeFileSync" and "Include error handling for JSON file operations and validate JSON structure after reading."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mcp/tests/integration/tools/next-task-tag.tool.test.ts` around lines 76
- 82, Replace the direct fs.writeFileSync calls with the project's JSON utility:
import and use writeJSON to write tasksPath with data and statePath with {
currentTag: 'master' } (instead of fs.writeFileSync). Ensure you add error
handling around the write operations (catch/rethrow or assert) and, after
writing, optionally read back via readJSON to validate the written structure
matches expectations (e.g., tasks shape and state.currentTag) so the test
follows the project's readJSON/writeJSON validation conventions.
Replaces execSync with shell-string interpolation in three MCP integration tests (next-task-tag, get-tasks, generate) with execFileSync using argument arrays. The previous form interpolated cliPath, mcpServerPath, toolName, and testDir into a shell command, which is injection-prone and breaks when paths contain spaces. - Init call: execFileSync(process.execPath, [cliPath, 'init', '--yes'], ...) to invoke the .js CLI via the current Node binary. - MCP inspector call: execFileSync(npxBin, [...argv]) where npxBin is 'npx.cmd' on Windows and 'npx' elsewhere. - toolArgs flattened from a space-joined string to a flat argv array. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1683. When an MCP tool is called with an explicit
tagargument (e.g.next_task tag=phase3), the response payload was still carrying thecurrentTagfrom.taskmaster/state.json(usuallymaster). The data was loaded from the correct tag, but the response'stagfield misled callers into thinking the operation ran on a different tag — leading to destructive corrective actions in multi-tag / multi-agent workspaces.The fix threads the already-resolved
tagintohandleApiResultin every JS-side MCP tool that wasn't already doing so, matching the existing pattern inadd-dependency.jsand the TS tools in@tm/mcp.Changes
mcp-server/src/tools/to passresolvedTag(orargs.tag) through tohandleApiResult:add-subtask,add-task,analyze,clear-subtasks,expand-all,expand-task,fix-dependencies,move-task,next-task,parse-prd,remove-dependency,remove-subtask,remove-task,research,scope-down,scope-up,set-task-status,update-subtask,update-task,update.apps/mcp/tests/integration/tools/next-task-tag.tool.test.tsthat pinsstate.jsontomaster, callsnext_taskwithtag=phase3, and asserts the response carries"tag": "phase3".Test plan
npm run test -w apps/mcppasses (includes the new regression test)next_task/set_task_status/update_taskwith an explicittagarg and confirm the response'stagfield matches the argumentadd-dependency.jsand TS tools remain unchanged (they already handled this correctly)Resolves #1683.
Summary by CodeRabbit
Bug Fixes
Tests