-
Notifications
You must be signed in to change notification settings - Fork 278
Added support for Azure bot #521
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. |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (8)
action.yml
is excluded by:!**/*.yml
dist/37.index.js
is excluded by:!dist/**
dist/73.index.js
is excluded by:!dist/**
dist/index.js
is excluded by:!dist/**
dist/licenses.txt
is excluded by:!dist/**
package-lock.json
is excluded by:!**/*.json
package.json
is excluded by:!**/*.json
tsconfig.json
is 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
Ids
andBotProtocol
interfaces is well-implemented. The use of optional properties inIds
and the method signature inBotProtocol
are correctly defined, adhering to TypeScript best practices. The decision to return a tuple from thechat
method inBotProtocol
is a good choice for handling both the response message and updatedIds
efficiently.src/main.ts (3)
- 8-16: The addition of imports for
AzureBot
,BotProtocol
, andTokenLimits
is 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
Options
object, following the established pattern for configuration management.- 52-67: The refactored bot creation logic in the
createBot
function is well-designed, offering improved modularity and readability. It correctly handles the dynamic creation of either anAzureBot
or a regularBot
based on the provided configuration, with appropriate error handling and logging.src/bot.ts (2)
- 13-15: The
Bot
class correctly implements theBotProtocol
interface, including the requiredchat
method. 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
chat
method within theBot
class is well-designed, with appropriate error handling and logging. The separation of concerns between the publicchat
method 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
AzureBot
class is correctly implemented, adhering to theBotProtocol
interface. The constructor and thechat
method 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 theOptions
class 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
BotProtocol
instead ofBot
in thehandleReviewComment
function 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
Bot
toBotProtocol
aligns 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
lightBot
andheavyBot
from typeBot
toBotProtocol
in thecodeReview
function signature is a necessary change following the introduction of theBotProtocol
interface. 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.
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.
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
@coderabbitai
in 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: ignore
anywhere 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
BotProtocol
interface and updated function parameters to use this interface, enhancing modularity and flexibility.Summary by CodeRabbit
New Features
AzureBot
class for integrating chatbot functionality using Azure and OpenAI services.Options
class to include Azure API configuration properties.Refactor
BotProtocol
interface for improved consistency and flexibility in bot handling.