Added support for Azure bot #521
Conversation
|
Warning CodeRabbit GitHub Action DetectedThe repository is using both CodeRabbit Pro and CodeRabbit Open Source (via GitHub Actions), which is not recommended as it may lead to duplicate comments and extra noise. Please remove the CodeRabbit GitHub Action. WalkthroughThe project has introduced a new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
WalkthroughThis update introduces support for Azure bot alongside existing bots. It adds new inputs and properties for Azure API configurations, a new Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![coderabbitaidev[bot]](https://avatars.githubusercontent.com/in/375467?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (8)
action.ymlis excluded by:!**/*.ymldist/37.index.jsis excluded by:!dist/**dist/73.index.jsis excluded by:!dist/**dist/index.jsis excluded by:!dist/**dist/licenses.txtis excluded by:!dist/**package-lock.jsonis excluded by:!**/*.jsonpackage.jsonis excluded by:!**/*.jsontsconfig.jsonis excluded by:!**/*.json
Files selected for processing (7)
- src/azure-bot.ts (1 hunks)
- src/bot-interface.ts (1 hunks)
- src/bot.ts (1 hunks)
- src/main.ts (3 hunks)
- src/options.ts (4 hunks)
- src/review-comment.ts (2 hunks)
- src/review.ts (2 hunks)
Additional comments: 12
src/bot-interface.ts (1)
- 1-10: The introduction of the
IdsandBotProtocolinterfaces is well-implemented. The use of optional properties inIdsand the method signature inBotProtocolare correctly defined, adhering to TypeScript best practices. The decision to return a tuple from thechatmethod inBotProtocolis a good choice for handling both the response message and updatedIdsefficiently.src/main.ts (3)
- 8-16: The addition of imports for
AzureBot,BotProtocol, andTokenLimitsis correctly implemented, aligning with the new functionality introduced in this PR.- 36-39: The new inputs for Azure API configuration are correctly retrieved and integrated into the
Optionsobject, following the established pattern for configuration management.- 52-67: The refactored bot creation logic in the
createBotfunction is well-designed, offering improved modularity and readability. It correctly handles the dynamic creation of either anAzureBotor a regularBotbased on the provided configuration, with appropriate error handling and logging.src/bot.ts (2)
- 13-15: The
Botclass correctly implements theBotProtocolinterface, including the requiredchatmethod. This adherence to the interface ensures consistency and standardization across different bot implementations.- 10-18: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [52-111]
The implementation of the
chatmethod within theBotclass is well-designed, with appropriate error handling and logging. The separation of concerns between the publicchatmethod and the privatechat_method for the actual chat logic is a good design choice, enhancing readability and maintainability.src/azure-bot.ts (1)
- 12-111: The
AzureBotclass is correctly implemented, adhering to theBotProtocolinterface. The constructor and thechatmethod are well-designed, with appropriate initialization, error handling, and logging. The design choices, including the use of a privatechat_method for the actual chat logic, enhance the class's readability and maintainability.src/options.ts (2)
- 25-27: The addition of Azure API configuration properties (
azureApiInstance,azureApiDeployment,azureApiVersion) to theOptionsclass is correctly implemented, enhancing the class's functionality to support Azure bot integration.- 96-98: Including the new Azure API configuration properties in the
src/review-comment.ts (1)
- 23-23: The update to use
BotProtocolinstead ofBotin thehandleReviewCommentfunction parameter is a positive change, promoting flexibility and standardization in bot handling across the application.src/review.ts (2)
- 5-5: Renaming the imported type
BottoBotProtocolaligns with the PR's objective to standardize bot communication within the application. This change enhances modularity and flexibility in handling different types of bots. Good job on maintaining consistency with the new interface naming convention.- 28-29: Updating the function parameters
lightBotandheavyBotfrom typeBottoBotProtocolin thecodeReviewfunction signature is a necessary change following the introduction of theBotProtocolinterface. This ensures that the function can accept any bot implementation that adheres to theBotProtocol, thereby increasing the flexibility and scalability of the bot handling mechanism within the application.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (11)
- action.yml (1)
- package-lock.json (93)
- package.json (1)
- src/azure-bot.ts (1)
- src/bot-interface.ts (1)
- src/bot.ts (1)
- src/main.ts (3)
- src/options.ts (4)
- src/review-comment.ts (2)
- src/review.ts (2)
- tsconfig.json (1)
Files ignored due to filter (4)
- dist/37.index.js
- dist/73.index.js
- dist/index.js
- dist/licenses.txt
Files not summarized due to errors (1)
- package-lock.json (diff tokens exceeds limit)
Files skipped from review due to trivial changes (1)
- tsconfig.json
Review comments generated (25)
- Review: 13
- LGTM: 12
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| "@langchain/openai": "^0.0.14", | ||
| "@octokit/action": "^6.0.4", |
There was a problem hiding this comment.
The new dependencies @langchain/openai and langchain have been added. Please ensure that these packages are reliable, well-maintained, and do not introduce any security vulnerabilities. It's also important to check if their licenses are compatible with the project.
+ "@langchain/openai": "^0.0.14",
+ "langchain": "^0.1.17",| import {BotProtocol, Ids} from './bot-interface' | ||
|
|
||
| // define type to save parentMessageId and conversationId | ||
| export interface Ids { | ||
| parentMessageId?: string | ||
| conversationId?: string | ||
| } | ||
|
|
||
| export class Bot { | ||
| export class Bot implements BotProtocol { |
There was a problem hiding this comment.
The changes made here are good. The BotProtocol and Ids interfaces have been moved to a separate file, which improves the modularity of the code. Now, the Bot class implements the BotProtocol interface, which is a good practice as it ensures that any class implementing this interface will have certain methods and properties. This makes the code more maintainable and easier to understand.
- // define type to save parentMessageId and conversationId
- export interface Ids {
- parentMessageId?: string
- conversationId?: string
- }
-
- export class Bot {
+ import {BotProtocol, Ids} from './bot-interface'
+
+ export class Bot implements BotProtocol {
private readonly api: ChatGPTAPI | null = null // not free
private readonly options: Options| info(`azureApiInstance: ${this.azureApiInstance}`) | ||
| info(`azureApiDeployment: ${this.azureApiDeployment}`) | ||
| info(`azureApiVersion: ${this.azureApiVersion}`) |
There was a problem hiding this comment.
While it's good to log the values of azureApiInstance, azureApiDeployment, and azureApiVersion for debugging purposes, it's important to consider that these might contain sensitive information. If they do, you should avoid logging them directly.
- info(`azureApiInstance: ${this.azureApiInstance}`)
- info(`azureApiDeployment: ${this.azureApiDeployment}`)
- info(`azureApiVersion: ${this.azureApiVersion}`)
+ info(`azureApiInstance: ${this.azureApiInstance ? 'provided' : 'not provided'}`)
+ info(`azureApiDeployment: ${this.azureApiDeployment ? 'provided' : 'not provided'}`)
+ info(`azureApiVersion: ${this.azureApiVersion ? 'provided' : 'not provided'}`)This way, we can still know if these values are set or not without exposing their actual content.
| azure_api_instance_name: | ||
| required: false | ||
| description: 'Instance name of your Azure API. For example, if your Azure instance is hosted under https://{INSTANCE_NAME}.openai.azure.com/openai/deployments/{DEPLOYMENT_NAME}, use INSTANCE_NAME' | ||
| azure_api_deployment_name: | ||
| required: false | ||
| description: 'Deployment name of your Azure API. For example, if your Azure instance is hosted under https://{INSTANCE_NAME}.openai.azure.com/openai/deployments/{DEPLOYMENT_NAME}, use DEPLOYMENT_NAME' | ||
| azure_api_version: | ||
| required: false | ||
| description: 'Api version like "2023-07-01-preview"' |
There was a problem hiding this comment.
The new inputs for Azure API instance name, deployment name, and API version are optional (required: false). However, if the user wants to use the Azure bot functionality, these fields would be necessary. It might be better to add a note in the description of these fields indicating that they are required for Azure bot functionality.
azure_api_instance_name:
required: false
- description: 'Instance name of your Azure API. For example, if your Azure instance is hosted under https://{INSTANCE_NAME}.openai.azure.com/openai/deployments/{DEPLOYMENT_NAME}, use INSTANCE_NAME'
+ description: 'Instance name of your Azure API. Required if using Azure bot. For example, if your Azure instance is hosted under https://{INSTANCE_NAME}.openai.azure.com/openai/deployments/{DEPLOYMENT_NAME}, use INSTANCE_NAME'
azure_api_deployment_name:
required: false
- description: 'Deployment name of your Azure API. For example, if your Azure instance is hosted under https://{INSTANCE_NAME}.openai.azure.com/openai/deployments/{DEPLOYMENT_NAME}, use DEPLOYMENT_NAME'
+ description: 'Deployment name of your Azure API. Required if using Azure bot. For example, if your Azure instance is hosted under https://{INSTANCE_NAME}.openai.azure.com/openai/deployments/{DEPLOYMENT_NAME}, use DEPLOYMENT_NAME'
azure_api_version:
required: false
- description: 'Api version like "2023-07-01-preview"'
+ description: 'Api version like "2023-07-01-preview". Required if using Azure bot.'| getInput('language'), | ||
| getInput('azure_api_instance_name'), | ||
| getInput('azure_api_deployment_name'), | ||
| getInput('azure_api_version') |
There was a problem hiding this comment.
The new inputs for Azure API instance name, deployment name, and API version are not validated. It's a good practice to validate user inputs to ensure they meet certain criteria before using them in the application. This can help prevent potential issues down the line.
+ const azureApiInstanceName = getInput('azure_api_instance_name');
+ const azureApiDeploymentName = getInput('azure_api_deployment_name');
+ const azureApiVersion = getInput('azure_api_version');
+
+ if (!azureApiInstanceName || !azureApiDeploymentName || !azureApiVersion) {
+ setFailed("Azure API configuration is incomplete. Please provide all required inputs.");
+ return;
+ }| chat = async (message: string, ids: Ids): Promise<[string, Ids]> => { | ||
| let res: [string, Ids] = ['', ids] | ||
| try { | ||
| res = await this.chat_(message) | ||
| return res | ||
| } catch (e: unknown) { | ||
| warning(`Failed to chat: ${e}`) | ||
| return res | ||
| } | ||
| } |
There was a problem hiding this comment.
The chat method catches errors but only logs a warning and returns the default response. This might hide potential issues with the chat functionality. Consider rethrowing the error after logging the warning, or handle it in a way that doesn't silence it.
- warning(`Failed to chat: ${e}`)
- return res
+ warning(`Failed to chat: ${e}`)
+ throw e| private readonly chat_ = async (message: string): Promise<[string, Ids]> => { | ||
| // record timing | ||
| const start = Date.now() | ||
| if (!message) { | ||
| return ['', {}] | ||
| } | ||
|
|
||
| let response: ChainValues | undefined | ||
|
|
||
| if (this.api != null) { | ||
| try { | ||
| response = await this.api.call({input: message}) | ||
| } catch (e: unknown) { | ||
| info(`response: ${response}, failed to send message to openai: ${e}`) | ||
| } | ||
| const end = Date.now() | ||
| info(`response: ${JSON.stringify(response)}`) | ||
| info( | ||
| `openai sendMessage (including retries) response time: ${ | ||
| end - start | ||
| } ms` | ||
| ) | ||
| } else { | ||
| setFailed('The OpenAI API is not initialized') | ||
| } | ||
| let responseText = '' | ||
| if (response != null) { | ||
| responseText = response.response | ||
| } else { | ||
| warning('openai response is null') | ||
| } | ||
| // remove the prefix "with " in the response | ||
| if (responseText.startsWith('with ')) { | ||
| responseText = responseText.substring(5) | ||
| } | ||
| if (this.options.debug) { | ||
| info(`openai responses: ${responseText}`) | ||
| } | ||
| return [responseText, {}] | ||
| } |
There was a problem hiding this comment.
The chat_ method is also doing too much work. It's responsible for sending the message, handling the response, and formatting the response text. Consider breaking this method into smaller methods each with a single responsibility.
- private readonly chat_ = async (message: string): Promise<[string, Ids]> => {
+ private readonly chat_ = async (message: string): Promise<[string, Ids]> => {
+ // record timing
+ const start = Date.now()
+ if (!message) {
+ return ['', {}]
+ }
+
+ let response: ChainValues | undefined
+
+ if (this.api != null) {
+ response = await this.sendMessageToOpenAI(message)
+ } else {
+ setFailed('The OpenAI API is not initialized')
+ }
+
+ let responseText = this.formatResponse(response)
+
+ if (this.options.debug) {
+ info(`openai responses: ${responseText}`)
+ }
+ return [responseText, {}]
+ }
+
+ private async sendMessageToOpenAI(message: string): Promise<ChainValues | undefined> {
+ // rest of the code...
+ }
+
+ private formatResponse(response: ChainValues | undefined): string {
+ // rest of the code...
+ }These changes will make the code more maintainable and easier to test by separating concerns into different methods.
| "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-3.0.1.tgz", | ||
| "integrity": "sha512-2JAn3z8AR6rjK8Sm8orRC0h/bcl/DqL7tRPdGZ4I1CjdF+EaMLmYxBHyXuKL849eucPFhvBoxMsflfOb8kxaeQ==" | ||
| }, | ||
| "node_modules/@anthropic-ai/sdk/node_modules/whatwg-url": { | ||
| "version": "5.0.0", | ||
| "resolved": "https://registry.npmjs.org/whatwg-url/-/whatwg-url-5.0.0.tgz", | ||
| "integrity": "sha512-saE57nupxk6v3HY35+jzBwYa0rKSy0XR8JSxZPwgLr7ys0IBzhGviA1/TUGJLmSVqs8pb9AnvICXEuOHLprYTw==", | ||
| "dependencies": { | ||
| "tr46": "~0.0.3", | ||
| "webidl-conversions": "^3.0.0" | ||
| } | ||
| }, | ||
| "node_modules/@babel/code-frame": { | ||
| "version": "7.18.6", | ||
| "resolved": "https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.18.6.tgz", | ||
| "integrity": "sha512-TDCmlK5eOvH+eH7cdAFlNXeVJqWIQ7gW9tY1GJIpUtFb6CmjVyq2VM3u71bOyR8CRihcCgMUYoDNyLXao3+70Q==", | ||
| "version": "7.23.5", | ||
| "resolved": "https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.23.5.tgz", | ||
| "integrity": "sha512-CgH3s1a96LipHCmSUmYFPwY7MNx8C3avkq7i4Wl3cfa662ldtUe4VM1TPXX70pfmrlWTb6jLqTYrZyT2ZTJBgA==", | ||
| "dev": true, | ||
| "dependencies": { | ||
| "@babel/highlight": "^7.18.6" | ||
| "@babel/highlight": "^7.23.4", | ||
| "chalk": "^2.4.2" | ||
| }, | ||
| "engines": { | ||
| "node": ">=6.9.0" | ||
| } | ||
| }, | ||
| "node_modules/@babel/code-frame/node_modules/ansi-styles": { | ||
| "version": "3.2.1", | ||
| "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-3.2.1.tgz", | ||
| "integrity": "sha512-VT0ZI6kZRdTh8YyJw3SMbYm/u+NqfsAxEpWO0Pf9sq8/e94WxxOpPKx9FR1FlyCtOVDNOQ+8ntlqFxiRc+r5qA==", | ||
| "dev": true, | ||
| "dependencies": { | ||
| "color-convert": "^1.9.0" | ||
| }, | ||
| "engines": { | ||
| "node": ">=4" | ||
| } | ||
| }, | ||
| "node_modules/@babel/code-frame/node_modules/chalk": { | ||
| "version": "2.4.2", | ||
| "resolved": "https://registry.npmjs.org/chalk/-/chalk-2.4.2.tgz", | ||
| "integrity": "sha512-Mti+f9lpJNcwF4tWV8/OrTTtF1gZi+f8FqlyAdouralcFWFQWF2+NgCHShjkCb+IFBLq9buZwE1xckQU4peSuQ==", | ||
| "dev": true, | ||
| "dependencies": { | ||
| "ansi-styles": "^3.2.1", | ||
| "escape-string-regexp": "^1.0.5", | ||
| "supports-color": "^5.3.0" | ||
| }, | ||
| "engines": { | ||
| "node": ">=4" | ||
| } | ||
| }, | ||
| "node_modules/@babel/code-frame/node_modules/color-convert": { | ||
| "version": "1.9.3", | ||
| "resolved": "https://registry.npmjs.org/color-convert/-/color-convert-1.9.3.tgz", | ||
| "integrity": "sha512-QfAUtd+vFdAtFQcC8CCyYt1fYWxSqAiK2cSD6zDB8N3cpsEBAvRxp9zOGg6G/SHHJYAT88/az/IuDGALsNVbGg==", | ||
| "dev": true, | ||
| "dependencies": { | ||
| "color-name": "1.1.3" | ||
| } | ||
| }, | ||
| "node_modules/@babel/code-frame/node_modules/color-name": { | ||
| "version": "1.1.3", | ||
| "resolved": "https://registry.npmjs.org/color-name/-/color-name-1.1.3.tgz", | ||
| "integrity": "sha512-72fSenhMw2HZMTVHeCA9KCmpEIbzWiQsjN+BHcBbS9vr1mtt+vJjPdksIBNUmKAW8TFUDPJK5SUU3QhE9NEXDw==", | ||
| "dev": true | ||
| }, | ||
| "node_modules/@babel/code-frame/node_modules/escape-string-regexp": { | ||
| "version": "1.0.5", | ||
| "resolved": "https://registry.npmjs.org/escape-string-regexp/-/escape-string-regexp-1.0.5.tgz", | ||
| "integrity": "sha512-vbRorB5FUQWvla16U8R/qgaFIya2qGzwDrNmCZuYKrbdSUMG6I1ZCGQRefkRVhuOkIGVne7BQ35DSfo1qvJqFg==", | ||
| "dev": true, | ||
| "engines": { | ||
| "node": ">=0.8.0" | ||
| } | ||
| }, | ||
| "node_modules/@babel/code-frame/node_modules/has-flag": { | ||
| "version": "3.0.0", | ||
| "resolved": "https://registry.npmjs.org/has-flag/-/has-flag-3.0.0.tgz", | ||
| "integrity": "sha512-sKJf1+ceQBr4SMkvQnBDNDtf4TXpVhVGateu0t918bl30FnbE2m4vNLX+VWe/dpjlb+HugGYzW7uQXH98HPEYw==", | ||
| "dev": true, | ||
| "engines": { | ||
| "node": ">=4" | ||
| } | ||
| }, | ||
| "node_modules/@babel/code-frame/node_modules/supports-color": { | ||
| "version": "5.5.0", | ||
| "resolved": "https://registry.npmjs.org/supports-color/-/supports-color-5.5.0.tgz", | ||
| "integrity": "sha512-QjVjwdXIt408MIiAqCX4oUKsgU2EqAGzs2Ppkm4aQYbjm+ZEWEcW4SfFNTr4uMNZma0ey4f5lgLrkB0aX0QMow==", | ||
| "dev": true, | ||
| "dependencies": { | ||
| "has-flag": "^3.0.0" | ||
| }, | ||
| "engines": { | ||
| "node": ">=4" | ||
| } | ||
| }, |
There was a problem hiding this comment.
The new dependencies introduced in the package-lock.json file are part of the @anthropic-ai/sdk and @babel/code-frame packages. However, it's not clear from the PR summary why these dependencies are needed. Please provide a justification for these new dependencies or remove them if they're not necessary.
| "version": "6.3.1", | ||
| "resolved": "https://registry.npmjs.org/semver/-/semver-6.3.1.tgz", | ||
| "integrity": "sha512-BR7VvDCVHO+q2xBEWskxS6DJE1qRnb7DxzUrogb71CWoSficBxYsiAGd+Kl0mmq/MprG9yArRkyrQxTO6XjMzA==", | ||
| "dev": true, | ||
| "bin": { | ||
| "semver": "bin/semver.js" | ||
| } |
There was a problem hiding this comment.
The version of semver has been updated from 6.3.0 to 6.3.1. This is a patch version update, so it should be safe. However, it would be good to test this change thoroughly to ensure that it doesn't introduce any unexpected behavior.
| "node_modules/@babel/generator": { | ||
| "version": "7.21.3", | ||
| "resolved": "https://registry.npmjs.org/@babel/generator/-/generator-7.21.3.tgz", | ||
| "integrity": "sha512-QS3iR1GYC/YGUnW7IdggFeN5c1poPUurnGttOV/bZgPGV+izC/D8HnD6DLwod0fsatNyVn1G3EVWMYIF0nHbeA==", | ||
| "version": "7.23.6", | ||
| "resolved": "https://registry.npmjs.org/@babel/generator/-/generator-7.23.6.tgz", | ||
| "integrity": "sha512-qrSfCYxYQB5owCmGLbl8XRpX1ytXlpueOb0N0UmQwA073KZxejgQTzAmJezxvpwQD9uGtK2shHdi55QT+MbjIw==", | ||
| "dev": true, | ||
| "dependencies": { | ||
| "@babel/types": "^7.21.3", | ||
| "@babel/types": "^7.23.6", |
There was a problem hiding this comment.
The version of @babel/generator has been updated from 7.21.3 to 7.23.6. This is a minor version update, so it should be backward compatible according to semantic versioning rules. However, it's always a good idea to test this change thoroughly to ensure that it doesn't introduce any unexpected behavior.
Summary by CodeRabbit
BotProtocolinterface and updated function parameters to use this interface, enhancing modularity and flexibility.Summary by CodeRabbit
New Features
AzureBotclass for integrating chatbot functionality using Azure and OpenAI services.Optionsclass to include Azure API configuration properties.Refactor
BotProtocolinterface for improved consistency and flexibility in bot handling.