-
Notifications
You must be signed in to change notification settings - Fork 5
nodejs unit tests #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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-toolingpackage 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 |
tests/agents-a365-tooling-extensions-openai-unittest/package.json
Outdated
Show resolved
Hide resolved
tests/agents-a365-tooling-extensions-langchain-unittest/package.json
Outdated
Show resolved
Hide resolved
tests/agents-a365-tooling-extensions-claude-unittest/tsconfig.json
Outdated
Show resolved
Hide resolved
tests/agents-a365-notifications-unittest/agent-notification-core.test.ts
Outdated
Show resolved
Hide resolved
tests/agents-a365-tooling-unittest/src/McpToolServerConfigurationService.test.ts
Show resolved
Hide resolved
tests/agents-a365-tooling-unittest/src/McpToolServerConfigurationService.test.ts
Outdated
Show resolved
Hide resolved
tests/agents-a365-tooling-unittest/src/McpToolServerConfigurationService.test.ts
Outdated
Show resolved
Hide resolved
juliomenendez
left a comment
There was a problem hiding this 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.
tests/agents-a365-notifications-unittest/agent-notification-core.test.ts
Outdated
Show resolved
Hide resolved
tests/agents-a365-notifications-unittest/extensions/agent-notification-handler.test.ts
Outdated
Show resolved
Hide resolved
tests/agents-a365-notifications-unittest/models/email-response.test.ts
Outdated
Show resolved
Hide resolved
tests/agents-a365-tooling-extensions-claude-unittest/src/core-logic.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
| "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.ts", | ||
| "!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.d.ts", | ||
| "!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts" |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
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.
| } | ||
| }, | ||
| "moduleNameMapper": { | ||
| "^@microsoft/agents-a365-tooling$": "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts" |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
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.
| "moduleNameMapper": { | ||
| "^@microsoft/agents-a365-tooling$": "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts" | ||
| }, | ||
| "moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"] |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
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.
| "<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" |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
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.
| } | ||
| }, | ||
| "moduleNameMapper": { | ||
| "^@microsoft/agents-a365-tooling-extensions-claude$": "<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/index.ts" |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
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.
| "moduleNameMapper": { | ||
| "^@microsoft/agents-a365-tooling-extensions-claude$": "<rootDir>/../../../packages/agents-a365-tooling-extensions-claude/src/index.ts" | ||
| }, | ||
| "moduleDirectories": ["node_modules", "<rootDir>/../../../node_modules"] |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
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.
| "<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\**\\*.ts", | ||
| "!<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\**\\*.d.ts", | ||
| "!<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts" |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
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.
| } | ||
| }, | ||
| "moduleNameMapper": { | ||
| "^@microsoft/agents-a365-notifications$": "<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts" |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
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.
| "moduleNameMapper": { | ||
| "^@microsoft/agents-a365-notifications$": "<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts" | ||
| }, | ||
| "moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"] |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
tests/agents-a365-tooling-unittest/src/McpToolServerConfigurationService.test.ts
Outdated
Show resolved
Hide resolved
|
I don't see any tests for the |
There was a problem hiding this 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.
| "^@microsoft/agents-a365-notifications$": "<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts" | ||
| }, | ||
| "moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"] |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
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"]| expect(ToolsMode.MockMCPServer).toBe('MockMCPServer'); | ||
| expect(ToolsMode.MCPPlatform).toBe('MCPPlatform'); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
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".
| expect(ToolsMode.MockMCPServer).toBe('MockMCPServer'); | |
| expect(ToolsMode.MCPPlatform).toBe('MCPPlatform'); | |
| expect(ToolsMode.MockMCPServer).toBe(0); | |
| expect(ToolsMode.MCPPlatform).toBe(1); |
| "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.ts", | ||
| "!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\**\\*.d.ts", | ||
| "!<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts" |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
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.
| "<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" |
| } | ||
| }, | ||
| "moduleNameMapper": { | ||
| "^@microsoft/agents-a365-tooling$": "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts" |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
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"| "moduleNameMapper": { | ||
| "^@microsoft/agents-a365-tooling$": "<rootDir>\\..\\..\\packages\\agents-a365-tooling\\src\\index.ts" | ||
| }, | ||
| "moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"] |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
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"]| "moduleDirectories": ["node_modules", "<rootDir>\\..\\..\\node_modules"] | |
| "moduleDirectories": ["node_modules", "<rootDir>/../../node_modules"] |
| "<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\**\\*.ts", | ||
| "!<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\**\\*.d.ts", | ||
| "!<rootDir>\\..\\..\\packages\\agents-a365-notifications\\src\\index.ts" |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
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"
]| createWpxComment, | ||
| createAgentNotificationActivity | ||
| } from '@microsoft/agents-a365-notifications'; | ||
| import { Activity } from '@microsoft/agents-activity'; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import Activity.
| import { Activity } from '@microsoft/agents-activity'; |
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import { AgentApplication, TurnContext, TurnState } from '@microsoft/agents-hosting'; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
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.
| import { AgentApplication, TurnContext, TurnState } from '@microsoft/agents-hosting'; |
| import { | ||
| AgentNotificationActivity, | ||
| NotificationType, | ||
| createEmailReference, | ||
| EMAIL_NOTIFICATION_TYPE, | ||
| WPX_COMMENT_TYPE, | ||
| isEmailReference, | ||
| isWpxComment, | ||
| createWpxComment | ||
| } from '@microsoft/agents-a365-notifications'; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import AgentNotificationActivity.
| import { | ||
| AgentNotificationActivity, | ||
| NotificationType, | ||
| EmailReference, | ||
| WpxComment, | ||
| createEmailReference, | ||
| createWpxComment, | ||
| AGENTS_EMAIL_SUBCHANNEL, | ||
| AGENTS_WORD_SUBCHANNEL, | ||
| AGENTS_EXCEL_SUBCHANNEL, | ||
| AGENTS_POWERPOINT_SUBCHANNEL | ||
| } from '@microsoft/agents-a365-notifications'; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports EmailReference, WpxComment.
tests/agents-a365-notifications/extensions/agent-notification-routing.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
| "<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"] |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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"]| "<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"] |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.
| "<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"] |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.
| "@microsoft/agents-hosting": "1.1.0-alpha.58" | ||
| "@microsoft/agents-activity": "1.1.0-alpha.58" |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| export interface MCPServerConfig { |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| export enum ToolsMode { |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import fs from 'fs'; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.| createWpxComment, | ||
| createAgentNotificationActivity | ||
| } from '@microsoft/agents-a365-notifications'; | ||
| import { Activity } from '@microsoft/agents-activity'; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import Activity.
There was a problem hiding this 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", |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.
| "@microsoft/agents-a365-runtime": "0.0.0-placeholder", |
| async addMcpToolServers( | ||
| agent: Agent, | ||
| agentUserId: string, | ||
| authorization: Authorization, | ||
| turnContext: TurnContext, | ||
| authToken: string | ||
| ): Promise<Agent> { | ||
| ): Promise<void> { |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.
| async addMcpToolServers( | ||
| mcpClientConfig: ClientConfig, | ||
| agentUserId: string, | ||
| authorization: Authorization, | ||
| turnContext: TurnContext, | ||
| authToken: string | ||
| ): Promise<ReactAgent> { | ||
| ): Promise<DynamicStructuredTool[]> { |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.
| * Call this to enable dynamic Claude tool access. | ||
| */ | ||
| async addToolServersToAgent( | ||
| async addToolServers( |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| export interface MCPServerConfig { |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| export enum ToolsMode { |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import fs from 'fs'; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.| // 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'; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import McpClientTool.
| import { McpToolServerConfigurationService, McpClientTool, Utility } from '@microsoft/agents-a365-tooling'; | |
| import { McpToolServerConfigurationService, Utility } from '@microsoft/agents-a365-tooling'; |
| 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'; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports EmailReference, WpxComment.
There was a problem hiding this 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. |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.| * Discover MCP servers and list tools formatted for the Claude SDK. | |
| * Discover MCP servers and list tools formatted for the LangChain SDK. |
| } | ||
|
|
||
| // Create Connection instance for LangChain agents | ||
| // Create Connection instance for OpenAI agents |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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| // Create Connection instance for OpenAI agents | |
| // Create Connection instance for LangChain agents |
| /** | ||
| * 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. |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
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.| * Discover MCP servers and list tools formatted for the Claude SDK. | |
| * Discover MCP servers and list tools formatted for the OpenAI Agents SDK. |
No description provided.