add support for task-augmented MCP tools - TEST

[mehtarac]

Implements basic support for task-augmented tool calls in MCP. This just swaps the callTool() invocation for callToolStream(). It has not yet been determined if this should be gated by an option or not.

Description

Swaps the callTool() invocation for callToolStream() to support using task-augmented tools.

Related Issues

strands-agents#356

Documentation PR

N/A (unless we decide to put this under an option)

Type of Change

New feature

Testing

How have you tested the change?

• I ran npm run check

Also spot-checked against https://github.com/LucaButBoring/simple-tasks-test.

Checklist

• I have read the CONTRIBUTING document
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[mehtarac]

/strands review

[mehtarac]

✅ Review Summary

Overall Assessment: Request Changes

The implementation is clean and well-structured, but there are several important issues that should be addressed before merging:

Positive Highlights ✨

• Clean implementation that maintains the existing API surface
• Well-structured test mock using createMockCallToolStream() helper
• Properly updates test expectations to match new API structure
• Follows existing code patterns and style

Key Issues Found 🔍

Critical Issues ⚠️

1. Missing error handling - No try-catch around takeResult(stream) which could lead to unhandled promise rejections

Important Issues 📝

2. Experimental API not documented - Using experimental.tasks API without documenting stability implications
3. Missing test coverage - No test for stream error scenarios
4. TSDoc not updated - Method documentation doesn't mention the internal streaming behavior

Minor Issues 💡

5. Verbose test description - Test name could be more concise

---

Detailed Feedback

1. ⚠️ Missing Error Handling (Critical) - src/mcp.ts lines 113-118

Issue: No error handling around takeResult(stream)

Why: The takeResult() function consumes an async generator and could throw errors during stream processing. Without proper error handling, these errors will propagate as unhandled promise rejections.

Suggestion: Wrap the stream consumption in try-catch:

public async callTool(tool: McpTool, args: JSONValue): Promise<JSONValue> {
  await this.connect()

  if (args === null || args === undefined) {
    return await this.callTool(tool, {})
  }

  if (typeof args !== 'object' || Array.isArray(args)) {
    throw new Error(
      `MCP Protocol Error: Tool arguments must be a JSON Object (named parameters). Received: ${Array.isArray(args) ? 'Array' : typeof args}`
    )
  }

  try {
    const stream = this._client.experimental.tasks.callToolStream({
      name: tool.name,
      arguments: args as Record<string, unknown>,
    })

    const result = await takeResult(stream)
    return result as JSONValue
  } catch (error) {
    throw new Error(
      `Failed to execute tool '${tool.name}': ${error instanceof Error ? error.message : String(error)}`
    )
  }
}

---

2. 📝 Experimental API Documentation (Important) - src/mcp.ts

Issue: Using experimental.tasks API without documenting the implications

Why: This violates the "accessible to humans and agents" tenet from CONTRIBUTING.md. Users and developers need to understand this relies on an experimental API that may change.

Suggestion:

Update the TSDoc comment (lines 93-98):

/**
 * Invoke a tool on the connected MCP server using an McpTool instance.
 * 
 * Note: This method uses the experimental task-augmented streaming API
 * (experimental.tasks.callToolStream) which may change in future versions
 * of the MCP SDK.
 * 
 * @param tool - The McpTool instance to invoke.
 * @param args - The arguments to pass to the tool.
 * @returns A promise that resolves with the result of the tool invocation.
 */

Add inline comment at the import (line 3):

// Using experimental streaming API for task-augmented tool support
// See: https://github.com/modelcontextprotocol/specification/issues/356
import { takeResult } from '@modelcontextprotocol/sdk/shared/responseMessage.js'

---

3. ✅ Missing Test for Stream Errors (Important) - src/__tests__/mcp.test.ts

Issue: No test coverage for when callToolStream itself fails

Why: The test currently only mocks successful stream behavior. We need to verify that errors during stream processing are handled appropriately.

Suggestion: Add this test case in the McpClient describe block (after line 116):

it('handles errors during stream consumption', async () => {
  const tool = new McpTool({ name: 'calc', description: '', inputSchema: {}, client })
  
  // Mock callToolStream to throw during iteration
  const failingStream = async function* () {
    throw new Error('Stream processing failed')
  }
  sdkClientMock.experimental.tasks.callToolStream.mockReturnValue(failingStream())

  await expect(client.callTool(tool, { op: 'add' }))
    .rejects.toThrow(/Stream processing failed|Failed to execute tool/)
})

---

4. 💡 Test Description (Minor) - src/__tests__/mcp.test.ts line 119

Issue: Test description "delegates invocation to SDK client via experimental.tasks.callToolStream" is quite verbose

Suggestion: Simplify to:

it('delegates invocation to SDK client using callToolStream', async () => {
  // Test remains the same
})

---

5. ✨ Great Mock Pattern (Positive Note) - src/__tests__/mcp.test.ts lines 10-18

Excellent work on the createMockCallToolStream helper! This demonstrates:

• ✅ Simulates async generator behavior correctly
• ✅ Makes test code more readable
• ✅ Reusable for other stream tests
• ✅ Includes helpful TSDoc

This is a perfect example of the "simple at any scale" tenet!

---

Questions for Discussion

1. Feature Flag: The PR description mentions "It has not yet been determined if this should be gated by an option or not." What's the decision? If gated, we'd need an additional parameter.

2. Related Issue [FEATURE] Initial support for task-augmented MCP tools strands-agents/sdk-typescript#356: Can you provide more context about what "task-augmented" means? This would help with documentation and understanding user impact.

3. Integration Testing: Have you tested this with a real MCP server using the experimental streaming API?

---

Testing Checklist

Before merging, please verify:

• Error handling added with try-catch around takeResult()
• Error handling test added for stream failures
• TSDoc updated with experimental API note
• Test coverage remains at 80%+ (run npm run test:coverage)
• All pre-commit hooks pass

---

Summary

Great work on this feature! The implementation is solid and well-tested. The changes needed are primarily around defensive error handling and documentation to meet the SDK's quality standards. The core logic is sound.

The most critical item is adding error handling around the stream consumption to prevent unhandled promise rejections in production use.

Would you like me to help create the specific code changes, or do you have any questions about the feedback?

Agent Result 🤖
Stop Reason: end_turn
Message: {
"role": "assistant",
"content": [
{
"reasoningContent": {
"reasoningText": {
"text": "Let me summarize my review for the user. I've completed a thorough review following the SOP, identified critical and important issues, and tried to post the comments but encountered permission issues with the GitHub token. Let me provide a comprehensive summary to the user.",
"signature": "Er0DCkgIChABGAIqQEpCHFbn+BIA4j+aNPD4k5aFtvAhaO3gByoYfM8rojCccDLjmpnR8iCwbk8VqAGfL6Pbes+ZsuYLsAJwj+JnbZ4SDOwbISJHMeflYXJcsRoMjGKAvi4T9/L6AaTbIjDGYQKSzt74bColWqNJZiKByfu32EVO5ykkXVhlW1mgxZ7SisChprRXgzVvVz/12ssqogKw4gf3j62KwfdyuQwtBvR4U6UOdczWKk3U11vyjqyhlGLKAqoEXyIIpbxGbUa79OmK2dye1rTSuXoGWOlXoAbqcFFr+kGE8WS0SgdV73eGuGPLCWZxQbvwi8+Wm/cmY+1rDzKyjwMxP1bk2JZ7HDnlaOu8OMESyfQqCm0ninj1lQ2YjzsMG4kMKv4ntt9TR96yRp/vAf5F2XSLa5yh12NA32HMfzZ22kRd1T0MMu5aQtjqppEGJBXxFrky06Rvmw0iOh/1LvEAY5diIXYvQ5kStsAOxyg/1tMU3TX16qYSuqNKYwHzrAIK7R4jLURS/qn2k5XI654OxMadE8OzmWFsgxVL3hHhcjWivS9fkLD0iXo57wZKNnUZDDtR8EUwp+q2gxgB"
}
}
},
{
"text": "## Review Complete! \n\nI've completed a comprehensive review of PR #1 following the Task Reviewer SOP. Unfortunately, I encountered permission issues when trying to post the review comments directly to GitHub (the token doesn't have write access to issues). However, I can provide you with the complete review below.\n\n---\n\n## \u2705 Review Summary\n\nOverall Assessment: Request Changes\n\nThe implementation is clean and well-structured, but there are several important issues that should be addressed before merging:\n\n### Positive Highlights \u2728\n- Clean implementation that maintains the existing API surface\n- Well-structured test mock using createMockCallToolStream() helper\n- Properly updates test expectations to match new API structure\n- Follows existing code patterns and style\n\n### Key Issues Found \ud83d\udd0d\n\n#### Critical Issues \u26a0\ufe0f\n1. Missing error handling - No try-catch around takeResult(stream) which could lead to unhandled promise rejections\n\n#### Important Issues \ud83d\udcdd\n2. Experimental API not documented - Using experimental.tasks API without documenting stability implications\n3. Missing test coverage - No test for stream error scenarios\n4. TSDoc not updated - Method documentation doesn't mention the internal streaming behavior\n\n#### Minor Issues \ud83d\udca1\n5. Verbose test description - Test name could be more concise\n\n---\n\n## Detailed Feedback\n\n### 1. \u26a0\ufe0f Missing Error Handling (Critical) - src/mcp.ts lines 113-118\n\nIssue: No error handling around takeResult(stream)\n\nWhy: The takeResult() function consumes an async generator and could throw errors during stream processing. Without proper error handling, these errors will propagate as unhandled promise rejections.\n\nSuggestion: Wrap the stream consumption in try-catch:\n\ntypescript\npublic async callTool(tool: McpTool, args: JSONValue): Promise<JSONValue> {\n await this.connect()\n\n if (args === null || args === undefined) {\n return await this.callTool(tool, {})\n }\n\n if (typeof args !== 'object' || Array.isArray(args)) {\n throw new Error(\n `MCP Protocol Error: Tool arguments must be a JSON Object (named parameters). Received: ${Array.isArray(args) ? 'Array' : typeof args}`\n )\n }\n\n try {\n const stream = this._client.experimental.tasks.callToolStream({\n name: tool.name,\n arguments: args as Record<string, unknown>,\n })\n\n const result = await takeResult(stream)\n return result as JSONValue\n } catch (error) {\n throw new Error(\n `Failed to execute tool '${tool.name}': ${error instanceof Error ? error.message : String(error)}`\n )\n }\n}\n\n\n---\n\n### 2. \ud83d\udcdd Experimental API Documentation (Important) - src/mcp.ts\n\nIssue: Using experimental.tasks API without documenting the implications\n\nWhy: This violates the "accessible to humans and agents" tenet from CONTRIBUTING.md. Users and developers need to understand this relies on an experimental API that may change.\n\nSuggestion: \n\nUpdate the TSDoc comment (lines 93-98):\ntypescript\n/**\n * Invoke a tool on the connected MCP server using an McpTool instance.\n * \n * Note: This method uses the experimental task-augmented streaming API\n * (experimental.tasks.callToolStream) which may change in future versions\n * of the MCP SDK.\n * \n * @param tool - The McpTool instance to invoke.\n * @param args - The arguments to pass to the tool.\n * @returns A promise that resolves with the result of the tool invocation.\n */\n\n\nAdd inline comment at the import (line 3):\ntypescript\n// Using experimental streaming API for task-augmented tool support\n// See: https://github.com/modelcontextprotocol/specification/issues/356\nimport { takeResult } from '@modelcontextprotocol/sdk/shared/responseMessage.js'\n\n\n---\n\n### 3. \u2705 Missing Test for Stream Errors (Important) - src/__tests__/mcp.test.ts\n\nIssue: No test coverage for when callToolStream itself fails\n\nWhy: The test currently only mocks successful stream behavior. We need to verify that errors during stream processing are handled appropriately.\n\nSuggestion: Add this test case in the McpClient describe block (after line 116):\n\ntypescript\nit('handles errors during stream consumption', async () => {\n const tool = new McpTool({ name: 'calc', description: '', inputSchema: {}, client })\n \n // Mock callToolStream to throw during iteration\n const failingStream = async function* () {\n throw new Error('Stream processing failed')\n }\n sdkClientMock.experimental.tasks.callToolStream.mockReturnValue(failingStream())\n\n await expect(client.callTool(tool, { op: 'add' }))\n .rejects.toThrow(/Stream processing failed|Failed to execute tool/)\n})\n\n\n---\n\n### 4. \ud83d\udca1 Test Description (Minor) - src/__tests__/mcp.test.ts line 119\n\nIssue: Test description "delegates invocation to SDK client via experimental.tasks.callToolStream" is quite verbose\n\nSuggestion: Simplify to:\ntypescript\nit('delegates invocation to SDK client using callToolStream', async () => {\n // Test remains the same\n})\n\n\n---\n\n### 5. \u2728 Great Mock Pattern (Positive Note) - src/__tests__/mcp.test.ts lines 10-18\n\nExcellent work on the createMockCallToolStream helper! This demonstrates:\n- \u2705 Simulates async generator behavior correctly\n- \u2705 Makes test code more readable\n- \u2705 Reusable for other stream tests\n- \u2705 Includes helpful TSDoc\n\nThis is a perfect example of the "simple at any scale" tenet!\n\n---\n\n## Questions for Discussion\n\n1. Feature Flag: The PR description mentions "It has not yet been determined if this should be gated by an option or not." What's the decision? If gated, we'd need an additional parameter.\n\n2. Related Issue strands-agents#356: Can you provide more context about what "task-augmented" means? This would help with documentation and understanding user impact.\n\n3. Integration Testing: Have you tested this with a real MCP server using the experimental streaming API?\n\n---\n\n## Testing Checklist\n\nBefore merging, please verify:\n- [ ] Error handling added with try-catch around takeResult()\n- [ ] Error handling test added for stream failures\n- [ ] TSDoc updated with experimental API note\n- [ ] Test coverage remains at 80%+ (run npm run test:coverage)\n- [ ] All pre-commit hooks pass\n\n---\n\n## Summary\n\nGreat work on this feature! The implementation is solid and well-tested. The changes needed are primarily around defensive error handling and documentation to meet the SDK's quality standards. The core logic is sound.\n\nThe most critical item is adding error handling around the stream consumption to prevent unhandled promise rejections in production use.\n\nWould you like me to help create the specific code changes, or do you have any questions about the feedback?"
}
]
}

[mehtarac]

/strands review

[mehtarac]

/strands review

[mehtarac]

/strands review

[mehtarac]

/strands review

[mehtarac]

/strands review

[mehtarac]

/strands review

[mehtarac]

/strands review

[github-actions]

✅ Fresh Review Complete

I've conducted a comprehensive review of PR #1 following the Task Reviewer SOP and project guidelines. Here's my assessment:

---

Overall Assessment: Request Changes

The implementation is clean and demonstrates good understanding of the streaming API, but there are critical error handling issues that must be addressed before merging.

---

🌟 Positive Highlights

1. Clean API preservation - Maintains existing callTool() interface without breaking changes
2. Excellent test helper - createMockCallToolStream() is well-designed and reusable (lines 10-18)
3. Proper mock structure - Test mocks correctly simulate the new API structure
4. Minimal changes - Follows "simple at any scale" tenet with focused, minimal modifications

---

⚠️ Critical Issues (Must Fix)

1. Missing Error Handling - src/mcp.ts lines 113-119

Issue: No try-catch around takeResult(stream) which could lead to unhandled promise rejections.

Why: The takeResult() function consumes an async generator and could throw errors during stream processing. Without proper error handling, errors will propagate as unhandled promise rejections, making debugging extremely difficult for users.

Suggestion: Wrap stream consumption in try-catch:

public async callTool(tool: McpTool, args: JSONValue): Promise<JSONValue> {
  await this.connect()

  if (args === null || args === undefined) {
    return await this.callTool(tool, {})
  }

  if (typeof args !== 'object' || Array.isArray(args)) {
    throw new Error(
      `MCP Protocol Error: Tool arguments must be a JSON Object (named parameters). Received: ${Array.isArray(args) ? 'Array' : typeof args}`
    )
  }

  try {
    const stream = this._client.experimental.tasks.callToolStream({
      name: tool.name,
      arguments: args as Record<string, unknown>,
    })

    const result = await takeResult(stream)
    return result as JSONValue
  } catch (error) {
    throw new Error(
      `Failed to execute tool '${tool.name}': ${error instanceof Error ? error.message : String(error)}`
    )
  }
}

Reference: AGENTS.md - Error Handling section

---

📋 Important Issues (Should Fix)

2. Experimental API Not Documented - src/mcp.ts

Issue: Using experimental.tasks API without documenting stability implications.

Why: This violates the "accessible to humans and agents" tenet from CONTRIBUTING.md. Users need to understand this relies on an experimental API that may change.

Suggestions:

A) Update TSDoc for callTool method (lines 94-99):

/**
 * Invoke a tool on the connected MCP server using an McpTool instance.
 * 
 * Note: This method uses the experimental task-augmented streaming API
 * (experimental.tasks.callToolStream) which may change in future versions
 * of the MCP SDK.
 * 
 * @param tool - The McpTool instance to invoke.
 * @param args - The arguments to pass to the tool.
 * @returns A promise that resolves with the result of the tool invocation.
 */

B) Add inline comment at import (line 3):

// Using experimental streaming API for task-augmented tool support
// See: https://github.com/modelcontextprotocol/specification/issues/356
import { takeResult } from '@modelcontextprotocol/sdk/shared/responseMessage.js'

Reference: CONTRIBUTING.md - Development Tenet strands-agents#5

---

3. Missing Test Coverage for Stream Errors - src/__tests__/mcp.test.ts

Issue: No test coverage for when callToolStream fails during stream processing.

Why: Tests only mock successful behavior. We need to verify error handling (especially after adding try-catch in Issue #1) to maintain 80%+ coverage requirement.

Suggestion: Add this test after line 130:

it('handles errors during stream consumption', async () => {
  const tool = new McpTool({ name: 'calc', description: '', inputSchema: {}, client })
  
  // Mock callToolStream to throw during iteration
  const failingStream = async function* () {
    throw new Error('Stream processing failed')
  }
  sdkClientMock.experimental.tasks.callToolStream.mockReturnValue(failingStream())

  await expect(client.callTool(tool, { op: 'add' }))
    .rejects.toThrow(/Stream processing failed|Failed to execute tool/)
})

Reference: CONTRIBUTING.md - Testing Requirements (80%+ coverage)

---

💡 Minor Suggestions

4. Verbose Test Description - src/__tests__/mcp.test.ts line 119

Current: 'delegates invocation to SDK client via experimental.tasks.callToolStream'
Suggested: 'delegates invocation using callToolStream'

Why: Follows "simple at any scale" tenet with concise, clear naming.

---

🎯 Questions for Discussion

1. Feature Flag Decision: The PR description mentions uncertainty about gating this behind an option. What's the current decision? If this should be optional, we'll need to add a configuration parameter.

2. Task-Augmented Context: Can you provide more details about what "task-augmented" means in the context of issue [FEATURE] Initial support for task-augmented MCP tools strands-agents/sdk-typescript#356? This will help with documentation clarity.

3. Integration Testing: Have you tested this against a real MCP server that uses the experimental streaming API?

---

✅ Testing Checklist

Before merging, please verify:

• Error handling added with try-catch around takeResult()
• Error handling test added for stream failures
• TSDoc updated with experimental API note
• Inline comment added referencing issue [FEATURE] Initial support for task-augmented MCP tools strands-agents/sdk-typescript#356
• Test coverage verified at 80%+ (npm run test:coverage)
• All pre-commit hooks pass (npm run check)

---

📝 Summary

Great work on implementing the experimental streaming API! The core implementation is solid and well-tested. The main concern is defensive error handling to prevent unhandled promise rejections in production.

The most critical item is adding proper error handling around stream consumption (Issue #1). Once that's addressed along with the documentation updates (Issue #2), this will be ready to merge.

Would you like me to help implement any of these changes, or do you have questions about the feedback?

[mehtarac]

/strands review

[github-actions]

✅ Fresh Review Complete

I've conducted a comprehensive review of PR #1 following the Task Reviewer SOP and project guidelines. Here's my assessment:

---

Overall Assessment: Request Changes

The implementation is clean and demonstrates good understanding of the streaming API, but there are critical error handling issues that must be addressed before merging.

---

🌟 Positive Highlights

1. Clean API preservation - Maintains existing callTool() interface without breaking changes
2. Excellent test helper - createMockCallToolStream() is well-designed and reusable (lines 10-18)
3. Proper mock structure - Test mocks correctly simulate the new API structure
4. Minimal changes - Follows "simple at any scale" tenet with focused, minimal modifications

---

⚠️ Critical Issues (Must Fix)

1. Missing Error Handling - src/mcp.ts lines 113-119

Issue: No try-catch around takeResult(stream) which could lead to unhandled promise rejections.

Why: The takeResult() function consumes an async generator and could throw errors during stream processing. Without proper error handling, errors will propagate as unhandled promise rejections, making debugging extremely difficult for users.

Suggestion: Wrap stream consumption in try-catch:

public async callTool(tool: McpTool, args: JSONValue): Promise<JSONValue> {
  await this.connect()

  if (args === null || args === undefined) {
    return await this.callTool(tool, {})
  }

  if (typeof args !== 'object' || Array.isArray(args)) {
    throw new Error(
      `MCP Protocol Error: Tool arguments must be a JSON Object (named parameters). Received: ${Array.isArray(args) ? 'Array' : typeof args}`
    )
  }

  try {
    const stream = this._client.experimental.tasks.callToolStream({
      name: tool.name,
      arguments: args as Record<string, unknown>,
    })

    const result = await takeResult(stream)
    return result as JSONValue
  } catch (error) {
    throw new Error(
      `Failed to execute tool '${tool.name}': ${error instanceof Error ? error.message : String(error)}`
    )
  }
}

Reference: AGENTS.md - Error Handling section

---

📋 Important Issues (Should Fix)

2. Experimental API Not Documented - src/mcp.ts

Issue: Using experimental.tasks API without documenting stability implications.

Why: This violates the "accessible to humans and agents" tenet from CONTRIBUTING.md. Users need to understand this relies on an experimental API that may change.

Suggestions:

A) Update TSDoc for callTool method (lines 94-99):

/**
 * Invoke a tool on the connected MCP server using an McpTool instance.
 * 
 * Note: This method uses the experimental task-augmented streaming API
 * (experimental.tasks.callToolStream) which may change in future versions
 * of the MCP SDK.
 * 
 * @param tool - The McpTool instance to invoke.
 * @param args - The arguments to pass to the tool.
 * @returns A promise that resolves with the result of the tool invocation.
 */

B) Add inline comment at import (line 3):

// Using experimental streaming API for task-augmented tool support
// See: https://github.com/modelcontextprotocol/specification/issues/356
import { takeResult } from '@modelcontextprotocol/sdk/shared/responseMessage.js'

Reference: CONTRIBUTING.md - Development Tenet strands-agents#5

---

3. Missing Test Coverage for Stream Errors - src/__tests__/mcp.test.ts

Issue: No test coverage for when callToolStream fails during stream processing.

Why: Tests only mock successful behavior. We need to verify error handling (especially after adding try-catch in Issue #1) to maintain 80%+ coverage requirement.

Suggestion: Add this test after line 130:

it('handles errors during stream consumption', async () => {
  const tool = new McpTool({ name: 'calc', description: '', inputSchema: {}, client })
  
  // Mock callToolStream to throw during iteration
  const failingStream = async function* () {
    throw new Error('Stream processing failed')
  }
  sdkClientMock.experimental.tasks.callToolStream.mockReturnValue(failingStream())

  await expect(client.callTool(tool, { op: 'add' }))
    .rejects.toThrow(/Stream processing failed|Failed to execute tool/)
})

Reference: CONTRIBUTING.md - Testing Requirements (80%+ coverage)

---

💡 Minor Suggestions

4. Verbose Test Description - src/__tests__/mcp.test.ts line 119

Current: 'delegates invocation to SDK client via experimental.tasks.callToolStream'
Suggested: 'delegates invocation using callToolStream'

Why: Follows "simple at any scale" tenet with concise, clear naming.

---

🎯 Questions for Discussion

1. Feature Flag Decision: The PR description mentions uncertainty about gating this behind an option. What's the current decision? If this should be optional, we'll need to add a configuration parameter.

2. Task-Augmented Context: Can you provide more details about what "task-augmented" means in the context of issue [FEATURE] Initial support for task-augmented MCP tools strands-agents/sdk-typescript#356? This will help with documentation clarity.

3. Integration Testing: Have you tested this against a real MCP server that uses the experimental streaming API?

---

✅ Testing Checklist

Before merging, please verify:

• Error handling added with try-catch around takeResult()
• Error handling test added for stream failures
• TSDoc updated with experimental API note
• Inline comment added referencing issue [FEATURE] Initial support for task-augmented MCP tools strands-agents/sdk-typescript#356
• Test coverage verified at 80%+ (npm run test:coverage)
• All pre-commit hooks pass (npm run check)

---

📝 Summary

Great work on implementing the experimental streaming API! The core implementation is solid and well-tested. The main concern is defensive error handling to prevent unhandled promise rejections in production.

The most critical item is adding proper error handling around stream consumption (Issue #1). Once that's addressed along with the documentation updates (Issue #2), this will be ready to merge.

Would you like me to help implement any of these changes, or do you have questions about the feedback?

[mehtarac]

/strands review

[mehtarac]

/strands review

[github-actions]

⚠️ Critical: Missing Error Handling

Issue: No try-catch around takeResult(stream) which could lead to unhandled promise rejections.

Why: The takeResult() function consumes an async generator and could throw errors during stream processing. Without proper error handling, errors will propagate as unhandled promise rejections, making debugging extremely difficult for users.

Suggestion: Wrap the stream consumption in a try-catch block:

try {
  const stream = this._client.experimental.tasks.callToolStream({
    name: tool.name,
    arguments: args as Record<string, unknown>,
  })

  const result = await takeResult(stream)
  return result as JSONValue
} catch (error) {
  throw new Error(
    `Failed to execute tool '${tool.name}': ${error instanceof Error ? error.message : String(error)}`
  )
}

Reference: AGENTS.md - Error Handling section

[github-actions]

💡 Minor: Test Description Could Be More Concise

Suggestion: Simplify the test description to be more concise:

it('delegates invocation using callToolStream', async () => {

Why: Following the "simple at any scale" tenet, we prefer concise, clear naming. The current description 'delegates invocation to SDK client via experimental.tasks.callToolStream' is unnecessarily verbose.

[github-actions]

📋 Important: Document Experimental API Usage

Issue: Using experimental.tasks API without documenting why or referencing the related issue.

Why: This violates the "accessible to humans and agents" tenet from CONTRIBUTING.md. Developers need to understand this is an experimental API.

Suggestion: Add an inline comment explaining the usage:

// Using experimental streaming API for task-augmented tool support
// See: https://github.com/modelcontextprotocol/specification/issues/356
import { takeResult } from '@modelcontextprotocol/sdk/shared/responseMessage.js'

Reference: CONTRIBUTING.md - Development Tenet strands-agents#5 "We are accessible to humans and agents"

[github-actions]

✨ Excellent Mock Pattern!

Great work on the createMockCallToolStream helper! This demonstrates several best practices:

• ✅ Simulates async generator behavior correctly
• ✅ Makes test code more readable and maintainable
• ✅ Reusable for other stream tests
• ✅ Includes helpful TSDoc comments

This is a perfect example of the "simple at any scale" tenet from CONTRIBUTING.md. The helper eliminates boilerplate and makes the test intent crystal clear. 🎉

[github-actions]

✅ Code Review Complete

I've conducted a comprehensive review of PR #1 following the Task Reviewer SOP and project guidelines. All specific issues have been commented inline on the relevant lines of code.

---

Overall Assessment: Request Changes

The implementation is clean and demonstrates good understanding of the streaming API, but there are critical error handling issues that must be addressed before merging.

---

🌟 Positive Highlights

1. Clean API preservation - Maintains existing callTool() interface without breaking changes
2. Excellent test helper - createMockCallToolStream() is well-designed and reusable (see inline comment on line 10)
3. Proper mock structure - Test mocks correctly simulate the new API structure
4. Minimal changes - Follows "simple at any scale" tenet with focused, minimal modifications

---

📊 Issues Summary

I've added 6 inline comments on specific lines of code:

Critical Issues (1)

• Line 113 in src/mcp.ts: Missing error handling around takeResult(stream) ⚠️

Important Issues (3)

• Line 3 in src/mcp.ts: Document experimental API usage with inline comment
• Line 94 in src/mcp.ts: Update TSDoc to mention experimental API
• Line 130 in src/__tests__/mcp.test.ts: Add test coverage for stream errors

Minor Suggestions (1)

• Line 119 in src/__tests__/mcp.test.ts: Simplify test description

Positive Feedback (1)

• Line 10 in src/__tests__/mcp.test.ts: Excellent mock pattern! ✨

---

🎯 Questions for Discussion

1. Feature Flag Decision: The PR description mentions uncertainty about gating this behind an option. What's the current decision? If this should be optional, we'll need to add a configuration parameter.

2. Task-Augmented Context: Can you provide more details about what "task-augmented" means in the context of issue [FEATURE] Initial support for task-augmented MCP tools strands-agents/sdk-typescript#356? This will help with documentation clarity.

3. Integration Testing: Have you tested this against a real MCP server that uses the experimental streaming API?

---

✅ Pre-Merge Checklist

Before merging, please verify:

• Error handling added - Wrap takeResult(stream) in try-catch (line 113)
• Error test added - Add test for stream failures (after line 130)
• TSDoc updated - Add note about experimental API (line 94)
• Import commented - Add inline comment referencing issue [FEATURE] Initial support for task-augmented MCP tools strands-agents/sdk-typescript#356 (line 3)
• Coverage verified - Run npm run test:coverage to ensure 80%+
• Quality checks pass - Run npm run check for all pre-commit hooks

---

📝 Summary

Great work on implementing the experimental streaming API! The core implementation is solid and well-tested. The main concern is defensive error handling to prevent unhandled promise rejections in production.

Most critical item: Add proper error handling around stream consumption (see comment on line 113). Once that's addressed along with the documentation updates, this will be ready to merge.

Development Tenets Applied:

• ✅ Simple at any scale - Minimal, focused changes
• ✅ Extensible by design - Maintains existing API surface
• ⚠️ The obvious path is the happy path - Needs better error handling
• ⚠️ Accessible to humans and agents - Needs documentation improvements
• ✅ Embrace common standards - Uses MCP SDK patterns

Would you like me to help implement any of these changes, or do you have questions about the feedback?