-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix: limitedActions in getTools #1341
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
👍 Looks good to me! Reviewed everything up to e1dc6f2 in 1 minute and 16 seconds
More details
- Looked at
89
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. js/src/frameworks/langchain.ts:80
- Draft comment:
Passing filters.integrationId to getToolsSchema is appropriate, but ensure that filters contains the expected integrationId when not provided explicitly. Consider adding documentation on its expected behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment suggests ensuring that a specific behavior is intended and also suggests adding documentation, which is not allowed. It does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it violates the rules.
2. js/src/frameworks/openai.ts:54
- Draft comment:
IntegrationId is now passed to getToolsSchema. This change aligns the call with Langchain but verify that filters.integrationId is consistently set across caller contexts. - Reason this comment was not posted:
Marked as duplicate.
3. js/src/frameworks/vercel.ts:91
- Draft comment:
Addition of integrationId in getToolsSchema call ensures consistency. Double-check if telemetry parameters (integrationId) should also be logged for better traceability. - Reason this comment was not posted:
Marked as duplicate.
4. js/src/sdk/base.toolset.ts:147
- Draft comment:
The getToolsSchema function now accepts an optional _integrationId parameter to filter limitedActions from integrations. This extension is useful, but consider adding inline docs to clarify its usage. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. js/src/frameworks/langchain.ts:80
- Draft comment:
Passing filters.integrationId to getToolsSchema is consistent with the new integration filtering logic. Ensure that filters.integrationId is always a string (or undefined) as expected. - Reason this comment was not posted:
Marked as duplicate.
6. js/src/frameworks/openai.ts:54
- Draft comment:
The integrationId is now passed to getToolsSchema. This is consistent with the new behavior; just verify that any external callers supply a valid string when needed. - Reason this comment was not posted:
Marked as duplicate.
7. js/src/frameworks/vercel.ts:67
- Draft comment:
The function signature now includes an optional integrationId. However, note that the ZExecuteToolCallParams schema used for parsing filters does not include integrationId. Consider extending that schema to validate integrationId as well. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. js/src/sdk/base.toolset.ts:168
- Draft comment:
Good fix: switching from using parsedFilters.actions to the updated 'actions' variable ensures that any filtering based on limitedActions (from the integration) is respected when building the query. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining the reason behind a code change. It doesn't suggest any action or ask for clarification, which violates the rules.
9. js/src/types/base_toolset.ts:80
- Draft comment:
The addition of integrationId to the ZToolSchemaFilter schema is a proper update to support integration-specific filtering logic. This enhances type safety. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it praises the addition of a feature without providing any actionable feedback or suggestions. It doesn't align with the rules for useful comments in a pull request review.
Workflow ID: wflow_6xCWEYTF6jT10GQC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
const appActions = await this.client.actions.list({ | ||
apps: parsedFilters.apps?.join(","), | ||
tags: parsedFilters.tags?.join(","), | ||
useCase: parsedFilters.useCase, | ||
actions: parsedFilters.actions?.join(","), | ||
actions: actions?.join(","), |
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.
In base.toolset.ts, the change from parsedFilters.actions
to actions
in the client.actions.list call is potentially risky. The actions
variable could be undefined at this point if no integration is found or if limitedActions is not set. Consider adding a fallback to parsedFilters.actions to maintain backward compatibility:
actions: (actions || parsedFilters.actions)?.join(","),
js/src/frameworks/vercel.ts
Outdated
@@ -64,7 +64,8 @@ export class VercelAIToolSet extends BaseComposioToolSet { | |||
useCase?: Optional<string>; | |||
usecaseLimit?: Optional<number>; | |||
filterByAvailableApps?: Optional<boolean>; | |||
}): Promise<{ [key: string]: CoreTool }> { | |||
integrationId?: Optional<string>; |
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.
In the VercelAIToolSet, consider adding a default value for integrationId in the filters parameter to maintain consistency with the entityId parameter:
filters: {
useCase?: Optional<string>;
usecaseLimit?: Optional<number>;
filterByAvailableApps?: Optional<boolean>;
integrationId?: Optional<string> = null;
}
Code Review SummaryThe changes to handle Potential Issues:
Suggestions:
Code Quality: 7/10
The changes are functionally correct but could benefit from more defensive programming practices and better documentation. |
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.
👍 Looks good to me! Incremental review on 7842e18 in 54 seconds
More details
- Looked at
115
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. js/src/frameworks/langchain.ts:80
- Draft comment:
Formatting change: The multi-line call to getToolsSchema improves readability. No functional change detected. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. js/src/frameworks/openai.ts:54
- Draft comment:
Changed to multi-line call to getToolsSchema. These formatting updates enhance clarity without affecting functionality. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. js/src/frameworks/vercel.ts:87
- Draft comment:
The updated multi-line call to getToolsSchema (with filters.integrationId as the third argument) is consistent with similar changes. Review that integrationId is correctly provided by filters if applicable. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. js/src/sdk/base.toolset.ts:146
- Draft comment:
No changes in functionality. Ensure that passing integrationId to getToolsSchema in the upstream methods aligns with expected behavior here. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. js/src/frameworks/langchain.ts:80
- Draft comment:
Refactor: Changed getToolsSchema call to multi-line format for improved readability and clarity. IntegrationId is now explicitly passed. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. js/src/frameworks/openai.ts:54
- Draft comment:
Consistent formatting: Updated getToolsSchema invocation to multi-line format. Clarifies parameter passing including integrationId for limitedActions filtering. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. js/src/frameworks/vercel.ts:87
- Draft comment:
Improved readability: Refactored getToolsSchema call to multi-line format ensuring integrationId from filters is passed correctly for limitedActions support. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. js/src/sdk/base.toolset.ts:167
- Draft comment:
Minor whitespace cleanup; no functional change. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_5WIIfP54BWukTbWX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Add
integrationId
to filter actions bylimitedActions
ingetToolsSchema
, updatinggetTools
in relevant toolsets.getToolsSchema
inbase.toolset.ts
now acceptsintegrationId
to filter actions bylimitedActions
.getTools
inLangchainToolSet
,OpenAIToolSet
, andVercelAIToolSet
updated to passintegrationId
togetToolsSchema
.integrationId
toZToolSchemaFilter
inbase_toolset.ts
.actions
assignment ingetToolsSchema
inbase.toolset.ts
.This description was created by
for 7842e18. It will automatically update as commits are pushed.