Skip to content

Conversation

@abdulanu0
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings November 2, 2025 09:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces comprehensive unit test infrastructure for the Agent365 tooling ecosystem, including tests for the core tooling package and three extension packages (OpenAI, LangChain, and Claude). The changes include Jest configuration, TypeScript configuration, and test files that validate package structure, exports, and integration patterns.

Key Changes

  • Added unit test infrastructure with Jest and ts-jest configuration
  • Created tests for core agents-a365-tooling package validating exports, contracts, utility functions, and configuration service
  • Added tests for OpenAI, LangChain, and Claude extension packages analyzing their integration patterns
  • Established consistent test structure across all packages with proper TypeScript configuration

Reviewed Changes

Copilot reviewed 34 out of 40 changed files in this pull request and generated 9 comments.

File Description
tests/agents-a365-tooling-unittest/* Core tooling package tests including index exports, contracts, utility methods, and configuration service tests
tests/agents-a365-tooling-extensions-openai-unittest/* OpenAI extension tests validating Agent integration and MCPServerStreamableHttp usage
tests/agents-a365-tooling-extensions-langchain-unittest/* LangChain extension tests validating DynamicStructuredTool and MultiServerMCPClient integration
tests/agents-a365-tooling-extensions-claude-unittest/* Claude extension tests validating Options interface and MCP SDK integration

Copy link

@juliomenendez juliomenendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to add -unittest to the folders, let's make the folders match the packages from the SDK.
Please import from the built packages like from '@microsoft/agents-a365-notifications';, instead of relative imports.

@abdulanu0 abdulanu0 requested a review from a team as a code owner November 7, 2025 01:42
Copilot AI review requested due to automatic review settings November 7, 2025 20:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 27 out of 35 changed files in this pull request and generated 9 comments.

Comment on lines +16 to +18
"<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.d.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts"
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path separators use Windows-style backslashes (\) which will cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility in Jest configuration paths.

Copilot uses AI. Check for mistakes.
}
},
"moduleNameMapper": {
"^@microsoft/agents-a365-tooling$": "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts"
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path separators use Windows-style backslashes (\) which will cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility in Jest configuration paths.

Copilot uses AI. Check for mistakes.
"moduleNameMapper": {
"^@microsoft/agents-a365-tooling$": "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts"
},
"moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"]
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path separators use Windows-style backslashes (\) which will cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility in Jest configuration paths.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +18
"<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/**/*.ts",
"!<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/**/*.d.ts",
"!<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/index.ts"
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path separators use Windows-style backslashes (\) which will cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility in Jest configuration paths.

Copilot uses AI. Check for mistakes.
}
},
"moduleNameMapper": {
"^@microsoft/agents-a365-tooling-extensions-claude$": "<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/index.ts"
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path separators use Windows-style backslashes (\) which will cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility in Jest configuration paths.

Copilot uses AI. Check for mistakes.
"moduleNameMapper": {
"^@microsoft/agents-a365-tooling-extensions-claude$": "<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/index.ts"
},
"moduleDirectories": ["node_modules", "<rootDir>/../../../node_modules"]
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path separators use Windows-style backslashes (\) which will cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility in Jest configuration paths.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +18
"<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\**\\*.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\**\\*.d.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts"
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path separators use Windows-style backslashes (\) which will cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility in Jest configuration paths.

Copilot uses AI. Check for mistakes.
}
},
"moduleNameMapper": {
"^@microsoft/agents-a365-notifications$": "<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts"
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path separators use Windows-style backslashes (\) which will cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility in Jest configuration paths.

Copilot uses AI. Check for mistakes.
"moduleNameMapper": {
"^@microsoft/agents-a365-notifications$": "<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts"
},
"moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"]
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path separators use Windows-style backslashes (\) which will cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility in Jest configuration paths.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files shouldn't be added, please run pnpm i or pnpm install instead of npm install

@JesuTerraz
Copy link
Contributor

I don't see any tests for the onAgentNotification etc methods?

Copilot AI review requested due to automatic review settings November 12, 2025 18:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated 12 comments.

Comment on lines +29 to +31
"^@microsoft/agents-a365-notifications$": "<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts"
},
"moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"]
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The moduleNameMapper and moduleDirectories paths use Windows-style backslashes (\), which will cause issues on Unix-based systems. Use forward slashes for cross-platform compatibility:

"moduleNameMapper": {
  "^@microsoft/agents-a365-notifications$": "<rootDir>/../../packages/agents-a365-notifications/src/index.ts"
},
"moduleDirectories": ["node_modules", "<rootDir>/../../node_modules"]

Copilot uses AI. Check for mistakes.
Comment on lines +17 to +18
expect(ToolsMode.MockMCPServer).toBe('MockMCPServer');
expect(ToolsMode.MCPPlatform).toBe('MCPPlatform');
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test description says "should have correct enum values" but the test is checking string values instead of numeric enum values. Based on the enum definition shown in notification-type.test.ts (lines 8-22), these enums have numeric values (0, 1, 2, 3), not string values. This test should be:

expect(ToolsMode.MockMCPServer).toBe(0);
expect(ToolsMode.MCPPlatform).toBe(1);

Or if these are string enums (which seems more likely given lines 172-191 test string comparisons), the test description should be "should have correct string enum values".

Suggested change
expect(ToolsMode.MockMCPServer).toBe('MockMCPServer');
expect(ToolsMode.MCPPlatform).toBe('MCPPlatform');
expect(ToolsMode.MockMCPServer).toBe(0);
expect(ToolsMode.MCPPlatform).toBe(1);

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +18
"<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.d.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts"
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The collectCoverageFrom paths use Windows-style backslashes (\), which will cause issues on Unix-based systems (Linux, macOS). Jest configuration should use forward slashes (/) for cross-platform compatibility.

Replace:

"<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.ts"

With:

"<rootDir>/../../packages/agents-a365-tooling/src/**/*.ts"

Apply the same fix to lines 17 and 18.

Suggested change
"<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.d.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts"
"<rootDir>/../../packages/agents-a365-tooling/src/**/*.ts",
"!<rootDir>/../../packages/agents-a365-tooling/src/**/*.d.ts",
"!<rootDir>/../../packages/agents-a365-tooling/src/index.ts"

Copilot uses AI. Check for mistakes.
}
},
"moduleNameMapper": {
"^@microsoft/agents-a365-tooling$": "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts"
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The moduleNameMapper path uses Windows-style backslashes (\), which will cause issues on Unix-based systems. Use forward slashes for cross-platform compatibility:

"^@microsoft/agents-a365-tooling$": "<rootDir>/../../packages/agents-a365-tooling/src/index.ts"

Copilot uses AI. Check for mistakes.
"moduleNameMapper": {
"^@microsoft/agents-a365-tooling$": "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts"
},
"moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"]
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The moduleDirectories path uses Windows-style backslashes (\), which will cause issues on Unix-based systems. Use forward slashes for cross-platform compatibility:

"moduleDirectories": ["node_modules", "<rootDir>/../../node_modules"]
Suggested change
"moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"]
"moduleDirectories": ["node_modules", "<rootDir>/../../node_modules"]

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +18
"<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\**\\*.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\**\\*.d.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts"
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The collectCoverageFrom paths use Windows-style backslashes (\), which will cause issues on Unix-based systems. Use forward slashes for cross-platform compatibility:

"collectCoverageFrom": [
  "<rootDir>/../../packages/agents-a365-notifications/src/**/*.ts",
  "!<rootDir>/../../packages/agents-a365-notifications/src/**/*.d.ts",
  "!<rootDir>/../../packages/agents-a365-notifications/src/index.ts"
]

Copilot uses AI. Check for mistakes.
createWpxComment,
createAgentNotificationActivity
} from '@microsoft/agents-a365-notifications';
import { Activity } from '@microsoft/agents-activity';
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import Activity.

Suggested change
import { Activity } from '@microsoft/agents-activity';

Copilot uses AI. Check for mistakes.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

import { AgentApplication, TurnContext, TurnState } from '@microsoft/agents-hosting';
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused imports AgentApplication, TurnContext, TurnState.

Suggested change
import { AgentApplication, TurnContext, TurnState } from '@microsoft/agents-hosting';

Copilot uses AI. Check for mistakes.
Comment on lines +6 to +15
import {
AgentNotificationActivity,
NotificationType,
createEmailReference,
EMAIL_NOTIFICATION_TYPE,
WPX_COMMENT_TYPE,
isEmailReference,
isWpxComment,
createWpxComment
} from '@microsoft/agents-a365-notifications';
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import AgentNotificationActivity.

Copilot uses AI. Check for mistakes.
Comment on lines 6 to 17
import {
AgentNotificationActivity,
NotificationType,
EmailReference,
WpxComment,
createEmailReference,
createWpxComment,
AGENTS_EMAIL_SUBCHANNEL,
AGENTS_WORD_SUBCHANNEL,
AGENTS_EXCEL_SUBCHANNEL,
AGENTS_POWERPOINT_SUBCHANNEL
} from '@microsoft/agents-a365-notifications';
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused imports EmailReference, WpxComment.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings November 13, 2025 20:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 68 out of 70 changed files in this pull request and generated 8 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

Comment on lines +16 to +31
"<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.d.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts"
],
"coverageThreshold": {
"global": {
"branches": 80,
"functions": 80,
"lines": 80,
"statements": 80
}
},
"moduleNameMapper": {
"^@microsoft/agents-a365-tooling$": "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts"
},
"moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"]
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent path separators in Jest configuration. The collectCoverageFrom and moduleNameMapper paths use Windows-style backslashes (\), which may cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility:

"collectCoverageFrom": [
  "<rootDir>/../../packages/agents-a365-tooling/src/**/*.ts",
  "!<rootDir>/../../packages/agents-a365-tooling/src/**/*.d.ts",
  "!<rootDir>/../../packages/agents-a365-tooling/src/index.ts"
],
"moduleNameMapper": {
  "^@microsoft/agents-a365-tooling$": "<rootDir>/../../packages/agents-a365-tooling/src/index.ts"
},
"moduleDirectories": ["node_modules", "<rootDir>/../../node_modules"]

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +31
"<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/**/*.ts",
"!<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/**/*.d.ts",
"!<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/index.ts"
],
"coverageThreshold": {
"global": {
"branches": 80,
"functions": 80,
"lines": 80,
"statements": 80
}
},
"moduleNameMapper": {
"^@microsoft/agents-a365-tooling-extensions-claude$": "<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/index.ts"
},
"moduleDirectories": ["node_modules", "<rootDir>/../../../node_modules"]
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent path separators in Jest configuration. The collectCoverageFrom and moduleNameMapper paths use Windows-style backslashes (\), which may cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +31
"<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\**\\*.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\**\\*.d.ts",
"!<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts"
],
"coverageThreshold": {
"global": {
"branches": 80,
"functions": 80,
"lines": 80,
"statements": 80
}
},
"moduleNameMapper": {
"^@microsoft/agents-a365-notifications$": "<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts"
},
"moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"]
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent path separators in Jest configuration. The collectCoverageFrom and moduleNameMapper paths use Windows-style backslashes (\), which may cause issues on Unix-based systems. Use forward slashes (/) for cross-platform compatibility.

Copilot uses AI. Check for mistakes.
Comment on lines +12 to +13
"@microsoft/agents-hosting": "1.1.0-alpha.58"
"@microsoft/agents-activity": "1.1.0-alpha.58"
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version constraint changed from caret (^) to exact version without caret. This means the dependency will be locked to exactly version 1.1.0-alpha.58, preventing any patch or minor updates. This is unusual for internal packages and could cause issues with dependency resolution. Consider using the caret range ^1.1.0-alpha.58 to allow compatible updates, or document why an exact version is required.

Copilot uses AI. Check for mistakes.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

export interface MCPServerConfig {
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Microsoft copyright header. According to the coding guidelines, all JavaScript and TypeScript source files should include the Microsoft copyright header at the top of the file:

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

Copilot generated this review using guidance from repository custom instructions.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

export enum ToolsMode {
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Microsoft copyright header. According to the coding guidelines, all JavaScript and TypeScript source files should include the Microsoft copyright header at the top of the file:

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

Copilot generated this review using guidance from repository custom instructions.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

import fs from 'fs';
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Microsoft copyright header. According to the coding guidelines, all JavaScript and TypeScript source files should include the Microsoft copyright header at the top of the file:

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

Copilot generated this review using guidance from repository custom instructions.
createWpxComment,
createAgentNotificationActivity
} from '@microsoft/agents-a365-notifications';
import { Activity } from '@microsoft/agents-activity';
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import Activity.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings November 13, 2025 21:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 66 out of 68 changed files in this pull request and generated 9 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

"directory": "nodejs/packages/agents-a365-observability"
},
"dependencies": {
"@microsoft/agents-a365-runtime": "0.0.0-placeholder",
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate dependency entry. The dependency @microsoft/agents-a365-runtime is defined twice - once with version "0.0.0-placeholder" and once with "workspace:*". Remove the placeholder entry.

Suggested change
"@microsoft/agents-a365-runtime": "0.0.0-placeholder",

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +24
async addMcpToolServers(
agent: Agent,
agentUserId: string,
authorization: Authorization,
turnContext: TurnContext,
authToken: string
): Promise<Agent> {
): Promise<void> {
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking API change: The method name has been changed from addToolServersToAgent to addMcpToolServers, and the return type changed from Promise<Agent> to Promise<void>. This is a breaking change that will affect existing consumers of this API. Consider keeping the old method as deprecated or clearly documenting this in release notes.

Copilot uses AI. Check for mistakes.
Comment on lines +22 to +28
async addMcpToolServers(
mcpClientConfig: ClientConfig,
agentUserId: string,
authorization: Authorization,
turnContext: TurnContext,
authToken: string
): Promise<ReactAgent> {
): Promise<DynamicStructuredTool[]> {
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking API change: The method name has been changed from addToolServersToAgent to addMcpToolServers, the first parameter type changed from ReactAgent to ClientConfig, and the return type changed from Promise<ReactAgent> to Promise<DynamicStructuredTool[]>. This is a breaking change that will affect existing consumers.

Copilot uses AI. Check for mistakes.
* Call this to enable dynamic Claude tool access.
*/
async addToolServersToAgent(
async addToolServers(
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking API change: The method name has been changed from addToolServersToAgent to addToolServers. This is a breaking change that will affect existing consumers of this API.

Copilot uses AI. Check for mistakes.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

export interface MCPServerConfig {
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing copyright header. All source files should include the Microsoft copyright header at the top:

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

Copilot generated this review using guidance from repository custom instructions.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

export enum ToolsMode {
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing copyright header. All source files should include the Microsoft copyright header at the top:

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

Copilot generated this review using guidance from repository custom instructions.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

import fs from 'fs';
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing copyright header. All source files should include the Microsoft copyright header at the top:

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

Copilot generated this review using guidance from repository custom instructions.
// Agent365 SDK
import { McpToolServerConfigurationService, Utility } from '@microsoft/agents-a365-tooling';
import { AgenticAuthenticationService } from '@microsoft/agents-a365-runtime';
import { McpToolServerConfigurationService, McpClientTool, Utility } from '@microsoft/agents-a365-tooling';
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import McpClientTool.

Suggested change
import { McpToolServerConfigurationService, McpClientTool, Utility } from '@microsoft/agents-a365-tooling';
import { McpToolServerConfigurationService, Utility } from '@microsoft/agents-a365-tooling';

Copilot uses AI. Check for mistakes.
Comment on lines +6 to +18
import {
AgentNotificationActivity,
NotificationType,
EmailReference,
WpxComment,
createEmailReference,
createWpxComment,
createAgentNotificationActivity,
AGENTS_EMAIL_SUBCHANNEL,
AGENTS_WORD_SUBCHANNEL,
AGENTS_EXCEL_SUBCHANNEL,
AGENTS_POWERPOINT_SUBCHANNEL
} from '@microsoft/agents-a365-notifications';
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused imports EmailReference, WpxComment.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings November 13, 2025 21:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 66 out of 68 changed files in this pull request and generated 3 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

/**
* Discover MCP servers and list tools formatted for the LangChain Orchestrator.
* Uses listToolServers to fetch server configs and getTools to enumerate tools.
* Discover MCP servers and list tools formatted for the Claude SDK.
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect documentation comment. This service is for LangChain, not Claude SDK:

 * Discover MCP servers and list tools formatted for the LangChain SDK.
Suggested change
* Discover MCP servers and list tools formatted for the Claude SDK.
* Discover MCP servers and list tools formatted for the LangChain SDK.

Copilot uses AI. Check for mistakes.
}

// Create Connection instance for LangChain agents
// Create Connection instance for OpenAI agents
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect documentation comment. This is for OpenAI agents, not Claude:

      // Create Connection instance for LangChain agents
Suggested change
// Create Connection instance for OpenAI agents
// Create Connection instance for LangChain agents

Copilot uses AI. Check for mistakes.
/**
* Discover MCP servers and list tools formatted for the OpenAI Agents SDK.
* Uses listToolServers to fetch server configs.
* Discover MCP servers and list tools formatted for the Claude SDK.
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect documentation comment. This service is for OpenAI, not Claude SDK:

 * Discover MCP servers and list tools formatted for the OpenAI Agents SDK.
Suggested change
* Discover MCP servers and list tools formatted for the Claude SDK.
* Discover MCP servers and list tools formatted for the OpenAI Agents SDK.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants