feat: add action versioning support #1241
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| * @param {Object} config.lockFile - Lock file configuration | ||
| * @param {string} config.lockFile.path - Path to the lock file, Default: .composio.lock | ||
| * @param {boolean} config.lockFile.lock - Whether to lock the file |
There was a problem hiding this comment.
The lockFile.lock parameter in JSDoc doesn't match the actual parameter name lockFile.save in the code, causing incorrect documentation
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * @param {Object} config.lockFile - Lock file configuration | |
| * @param {string} config.lockFile.path - Path to the lock file, Default: .composio.lock | |
| * @param {boolean} config.lockFile.lock - Whether to lock the file | |
| * @param {Object} config.lockFile - Lock file configuration | |
| * @param {string} config.lockFile.path - Path to the lock file, Default: .composio.lock | |
| * @param {boolean} config.lockFile.save - Whether to lock the file |
| if (!this.lockFile.path) { | ||
| CEG.getCustomError(COMPOSIO_SDK_ERROR_CODES.SDK.INVALID_PARAMETER, { | ||
| message: "Lock file path is required when save is true", | ||
| description: "Lock file path is required when save is true", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing error throw after generating error with CEG.getCustomError(). The error is created but never thrown, allowing execution to continue with invalid state
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (!this.lockFile.path) { | |
| CEG.getCustomError(COMPOSIO_SDK_ERROR_CODES.SDK.INVALID_PARAMETER, { | |
| message: "Lock file path is required when save is true", | |
| description: "Lock file path is required when save is true", | |
| }); | |
| } | |
| if (!this.lockFile.path) { | |
| throw CEG.getCustomError(COMPOSIO_SDK_ERROR_CODES.SDK.INVALID_PARAMETER, { | |
| message: "Lock file path is required when save is true", | |
| description: "Lock file path is required when save is true", | |
| }); | |
| } |
| export const loadLockFile = (filePath: string) => { | ||
| const fileContent = fs.readFileSync(filePath, "utf8"); | ||
| return JSON.parse(fileContent); | ||
| }; | ||
|
|
||
| export const saveLockFile = ( | ||
| filePath: string, | ||
| actionName: string, | ||
| version: string | ||
| ) => { | ||
| fs.writeFileSync(filePath, JSON.stringify({ actionName, version }, null, 2)); | ||
| }; |
There was a problem hiding this comment.
Missing error handling for file operations. Wrap fs.readFileSync(), fs.writeFileSync(), and JSON.parse() in try-catch blocks to handle errors gracefully
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| export const loadLockFile = (filePath: string) => { | |
| const fileContent = fs.readFileSync(filePath, "utf8"); | |
| return JSON.parse(fileContent); | |
| }; | |
| export const saveLockFile = ( | |
| filePath: string, | |
| actionName: string, | |
| version: string | |
| ) => { | |
| fs.writeFileSync(filePath, JSON.stringify({ actionName, version }, null, 2)); | |
| }; | |
| export const loadLockFile = (filePath: string) => { | |
| try { | |
| const fileContent = fs.readFileSync(filePath, "utf8"); | |
| return JSON.parse(fileContent); | |
| } catch (error) { | |
| throw new Error(`Failed to load lock file: ${error.message}`); | |
| } | |
| }; | |
| export const saveLockFile = ( | |
| filePath: string, | |
| actionName: string, | |
| version: string | |
| ) => { | |
| try { | |
| fs.writeFileSync(filePath, JSON.stringify({ actionName, version }, null, 2)); | |
| } catch (error) { | |
| throw new Error(`Failed to save lock file: ${error.message}`); | |
| } | |
| }; |
| const lockFileContent = require("fs").readFileSync(filePath, "utf8"); | ||
| const actionVersions: Record<string, string> = {}; | ||
| const lines = lockFileContent.split("\n"); | ||
| for (const line of lines) { | ||
| if (line) { | ||
| const [actionName, version] = line.split("="); | ||
| actionVersions[actionName] = version; | ||
| } | ||
| } | ||
| return actionVersions; | ||
| } catch (e) { | ||
| const error = e as NodeJS.ErrnoException; | ||
| if (error.code === "ENOENT") { | ||
| logger.warn("Lock file does not exist, creating new one"); | ||
| } else if (error.code === "EACCES" || error.code === "EPERM") { | ||
| logger.error("Permission denied accessing lock file", e); | ||
| } else { | ||
| logger.warn("Error reading lock file", e); | ||
| } | ||
| return {}; | ||
| } | ||
| }; | ||
|
|
||
| export const saveLockFile = ( | ||
| filePath: string, | ||
| actionVersions: Record<string, string> | ||
| ) => { | ||
| try { | ||
| const lockFileContent = Object.entries(actionVersions) | ||
| .map(([actionName, version]) => `${actionName}=${version}`) | ||
| .join("\n"); | ||
| require("fs").writeFileSync(filePath, lockFileContent); |
There was a problem hiding this comment.
Lock file read/write operations use dynamic require('fs') which is not recommended. Import fs at the top level instead using import * as fs from 'fs' for better performance and static analysis.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const lockFileContent = require("fs").readFileSync(filePath, "utf8"); | |
| const actionVersions: Record<string, string> = {}; | |
| const lines = lockFileContent.split("\n"); | |
| for (const line of lines) { | |
| if (line) { | |
| const [actionName, version] = line.split("="); | |
| actionVersions[actionName] = version; | |
| } | |
| } | |
| return actionVersions; | |
| } catch (e) { | |
| const error = e as NodeJS.ErrnoException; | |
| if (error.code === "ENOENT") { | |
| logger.warn("Lock file does not exist, creating new one"); | |
| } else if (error.code === "EACCES" || error.code === "EPERM") { | |
| logger.error("Permission denied accessing lock file", e); | |
| } else { | |
| logger.warn("Error reading lock file", e); | |
| } | |
| return {}; | |
| } | |
| }; | |
| export const saveLockFile = ( | |
| filePath: string, | |
| actionVersions: Record<string, string> | |
| ) => { | |
| try { | |
| const lockFileContent = Object.entries(actionVersions) | |
| .map(([actionName, version]) => `${actionName}=${version}`) | |
| .join("\n"); | |
| require("fs").writeFileSync(filePath, lockFileContent); | |
| const lockFileContent = fs.readFileSync(filePath, "utf8"); | |
| const actionVersions: Record<string, string> = {}; | |
| const lines = lockFileContent.split("\n"); | |
| for (const line of lines) { | |
| if (line) { | |
| const [actionName, version] = line.split("="); | |
| actionVersions[actionName] = version; | |
| } | |
| } | |
| return actionVersions; | |
| } catch (e) { | |
| const error = e as NodeJS.ErrnoException; | |
| if (error.code === "ENOENT") { | |
| logger.warn("Lock file does not exist, creating new one"); | |
| } else if (error.code === "EACCES" || error.code === "EPERM") { | |
| logger.error("Permission denied accessing lock file", e); | |
| } else { | |
| logger.warn("Error reading lock file", e); | |
| } | |
| return {}; | |
| } | |
| }; | |
| export const saveLockFile = ( | |
| filePath: string, | |
| actionVersions: Record<string, string> | |
| ) => { | |
| try { | |
| const lockFileContent = Object.entries(actionVersions) | |
| .map(([actionName, version]) => `${actionName}=${version}`) | |
| .join("\n"); | |
| fs.writeFileSync(filePath, lockFileContent); |
| export const updateLockFileWithActionVersion = ( | ||
| filePath: string, | ||
| actionName: string, | ||
| version: string | ||
| ) => { | ||
| const actionVersions = getVersionsFromLockFileAsJson(filePath); | ||
| actionVersions[actionName] = version; | ||
| saveLockFile(filePath, actionVersions); | ||
| }; |
There was a problem hiding this comment.
Missing input validation for actionName and version parameters could allow injection of malicious content into the lock file. Add validation to ensure they don't contain = or newline characters.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| export const updateLockFileWithActionVersion = ( | |
| filePath: string, | |
| actionName: string, | |
| version: string | |
| ) => { | |
| const actionVersions = getVersionsFromLockFileAsJson(filePath); | |
| actionVersions[actionName] = version; | |
| saveLockFile(filePath, actionVersions); | |
| }; | |
| export const updateLockFileWithActionVersion = ( | |
| filePath: string, | |
| actionName: string, | |
| version: string | |
| ) => { | |
| if (actionName.includes('=') || actionName.includes('\n') || version.includes('=') || version.includes('\n')) { | |
| throw new Error('Action name and version must not contain "=" or newline characters'); | |
| } | |
| const actionVersions = getVersionsFromLockFileAsJson(filePath); | |
| actionVersions[actionName] = version; | |
| saveLockFile(filePath, actionVersions); | |
| }; |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 9e9c773 in 1 minute and 41 seconds
More details
- Looked at
247lines of code in7files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. js/src/sdk/utils/lock/load.ts:13
- Draft comment:
ThesaveLockFilefunction is saving the lock file content as a JSON object withactionNameandversionkeys, which is inconsistent with the format used inlockFile.ts. Consider updating it to match the format used inlockFile.ts. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment might be correct, but we have no way to verify this without seeing lockFile.ts. Following our rules, we should not keep comments that require additional context to verify. The comment is about a cross-file consistency issue, which our rules say to ignore.
The inconsistency could be a real issue that causes runtime problems. Maybe we should ask to see lockFile.ts?
No - our rules explicitly state to ignore cross-file issues and to delete comments if we need more context to verify them. We must stick to these principles.
Delete this comment because it requires knowledge of another file to verify, and we're instructed to ignore cross-file issues.
2. js/src/sdk/utils/processor/action_lock.ts:13
- Draft comment:
TheactionLockProcessorfunction currently returns thetoolSchemawithout any processing. If processing is intended, consider implementing the necessary logic. - Reason this comment was not posted:
Confidence changes required:50%
TheactionLockProcessorfunction currently does not perform any processing on thetoolSchema. It simply returns thetoolSchemaas is. This might be intentional, but if processing is expected, this function needs to be implemented.
Workflow ID: wflow_fDh5a2oC8ZlJpFmZ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -79,19 +86,27 @@ export class ComposioToolSet { | |||
| * @param {string|null} config.runtime - Runtime environment | |||
| * @param {string} config.entityId - Entity ID for operations | |||
| * @param {Record<string, string>} config.connectedAccountIds - Map of app names to their connected account IDs | |||
| * @param {Object} config.lockFile - Lock file configuration | |||
| * @param {string} config.lockFile.path - Path to the lock file, Default: .composio.lock | |||
| * @param {boolean} config.lockFile.lock - Whether to lock the file | |||
There was a problem hiding this comment.
The lockFile parameter documentation mentions a lock property, but the code uses a save property. Update the documentation to reflect the correct property name.
|
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: |
| CEG.getCustomError(COMPOSIO_SDK_ERROR_CODES.SDK.INVALID_PARAMETER, { | ||
| message: "Lock file path is required when save is true", | ||
| description: "Lock file path is required when save is true", | ||
| }); |
There was a problem hiding this comment.
The error handling here is incorrect. The error is created but not thrown, which means the code will continue executing with an invalid state. You should either throw the error or handle it appropriately:
if (!this.lockFile.path) {
throw CEG.getCustomError(COMPOSIO_SDK_ERROR_CODES.SDK.INVALID_PARAMETER, {
message: "Lock file path is required when save is true",
description: "Lock file path is required when save is true",
});
}|
|
||
| export const getVersionsFromLockFileAsJson = (filePath: string) => { | ||
| try { | ||
| const lockFileContent = require("fs").readFileSync(filePath, "utf8"); |
There was a problem hiding this comment.
Using require('fs') directly is not recommended. Instead, import the fs module at the top of the file:
import fs from 'fs';This makes the code more maintainable and follows better module import practices.
| toolSchema: RawActionData; | ||
| } | ||
| ): RawActionData => { | ||
| return toolSchema; |
There was a problem hiding this comment.
The actionLockProcessor currently returns the toolSchema without any modification. If this is intended to be a placeholder for future version locking logic, it should be documented:
export const actionLockProcessor = (
filePath: string,
{
actionName,
toolSchema,
}: {
actionName: string;
toolSchema: RawActionData;
}
): RawActionData => {
// TODO: Implement version locking logic
return toolSchema;
};| @@ -79,19 +86,27 @@ export class ComposioToolSet { | |||
| * @param {string|null} config.runtime - Runtime environment | |||
| * @param {string} config.entityId - Entity ID for operations | |||
| * @param {Record<string, string>} config.connectedAccountIds - Map of app names to their connected account IDs | |||
| * @param {Object} config.lockFile - Lock file configuration | |||
| * @param {string} config.lockFile.path - Path to the lock file, Default: .composio.lock | |||
| * @param {boolean} config.lockFile.lock - Whether to lock the file | |||
There was a problem hiding this comment.
The lock file configuration in the constructor's JSDoc is incorrect. The parameter is documented as lock but the actual property is save:
* @param {Object} config.lockFile - Lock file configuration
* @param {string} config.lockFile.path - Path to the lock file, Default: .composio.lock
* @param {boolean} config.lockFile.save - Whether to save version information to the lock file
Code Review SummaryOverall AssessmentThe PR introduces action versioning support with a lock file mechanism, which is a valuable addition. However, there are several issues that need to be addressed: Critical Issues
Code Quality: 6/10
Recommendations
Please address these issues before merging to ensure code quality and maintainability. |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f9bc94a in 29 seconds
More details
- Looked at
139lines of code in6files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. js/src/sdk/base.toolset.ts:129
- Draft comment:
The PR description indicates that tests are not yet written. It's important to include tests to ensure the new feature works correctly and does not introduce regressions. - Reason this comment was not posted:
Comment did not seem useful.
2. js/src/sdk/base.toolset.ts:224
- Draft comment:
Using optional chaining withschema?.nameandschema?.versionmight be unnecessary ifRawActionDataguarantees these properties are defined. Consider removing the optional chaining if these properties are always present. - Reason this comment was not posted:
Confidence changes required:50%
The code uses optional chaining withschema?.nameandschema?.version, butschemais already defined asRawActionData, which should have these properties. This might be unnecessary unlessRawActionDataallows these properties to be undefined.
3. js/src/sdk/base.toolset.ts:321
- Draft comment:
Ensure thatactionVersionMapkeys are consistently stored and accessed in uppercase to avoid potential mismatches. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_mIK42P7XRkh8P5dM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| if (schema?.version) { | ||
| // store the version in the map for execution | ||
| this.actionVersionMap[schema?.name.toUpperCase()] = schema?.version; |
There was a problem hiding this comment.
Potential runtime error if schema?.name is undefined when accessing toUpperCase(). Should add null check or use optional chaining.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (schema?.version) { | |
| // store the version in the map for execution | |
| this.actionVersionMap[schema?.name.toUpperCase()] = schema?.version; | |
| if (schema?.version && schema?.name) { | |
| // store the version in the map for execution | |
| this.actionVersionMap[schema.name.toUpperCase()] = schema.version; | |
| } |
| params, | ||
| text, | ||
| connectedAccountId, | ||
| version, | ||
| }: ExecuteActionParams): Promise<ActionExecuteResponse> { |
There was a problem hiding this comment.
The version parameter is added to the interface but lacks type definition and validation, which could lead to runtime errors if invalid values are passed.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| params, | |
| text, | |
| connectedAccountId, | |
| version, | |
| }: ExecuteActionParams): Promise<ActionExecuteResponse> { | |
| params, | |
| text, | |
| connectedAccountId, | |
| version?: string | number, | |
| }: ExecuteActionParams): Promise<ActionExecuteResponse> { |
Important
Adds action versioning support with lock file management in
ComposioToolSet, updating schema and execution processes.ComposioToolSetby introducingactionVersionMapandlockFileconfiguration.getToolsSchema()andexecuteAction()inbase.toolset.tsto handle action versions.execute()inEntity.tsto include version parameter.list()andexecute()inactions.tsto support action versions.loadLockFileandsaveLockFileinload.tsfor reading and writing lock files.updateLockFileWithActionVersionandgetVersionsFromLockFileAsJsoninlockFile.tsfor managing action versions.actionLockProcessorinaction_lock.tsto update lock files during schema processing.versionandavailableVersionstoZRawActionSchemainbase_toolset.ts.ZExecuteActionParamsinentity.tsandaction.tsto includeversion.This description was created by
for f9bc94a. It will automatically update as commits are pushed.