Skip to content
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

feat: add action versioning support #1241

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions js/src/sdk/base.toolset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import { Triggers } from "./models/triggers";
import { getUserDataJson } from "./utils/config";
import { CEG } from "./utils/error";
import { COMPOSIO_SDK_ERROR_CODES } from "./utils/errors/src/constants";
import { getVersionsFromLockFileAsJson } from "./utils/lockFile";
import { actionLockProcessor } from "./utils/processor/action_lock";
import {
fileInputProcessor,
fileResponseProcessor,
Expand Down Expand Up @@ -54,6 +56,12 @@ export class ComposioToolSet {
activeTriggers: ActiveTriggers;

userActionRegistry: ActionRegistry;
actionVersionMap: Record<string, string>;

lockFile: {
path: string | null;
save: boolean;
};

private internalProcessors: {
pre: TPreProcessor[];
Expand All @@ -79,19 +87,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
Comment on lines +90 to +92
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
* @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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lockFile parameter documentation mentions a lock property, but the code uses a save property. Update the documentation to reflect the correct property name.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
constructor({
apiKey,
baseUrl,
runtime,
entityId,
connectedAccountIds,
lockFile,
}: {
apiKey?: string | null;
baseUrl?: string | null;
runtime?: string | null;
entityId?: string;
connectedAccountIds?: Record<string, string>;
lockFile?: {
path: string;
save: boolean;
};
} = {}) {
const clientApiKey: string | undefined =
apiKey ||
Expand All @@ -113,6 +129,23 @@ export class ComposioToolSet {
this.integrations = this.client.integrations;
this.activeTriggers = this.client.activeTriggers;
this.connectedAccountIds = connectedAccountIds || {};
this.actionVersionMap = {};

this.lockFile = lockFile || {
path: ".composio.lock",
save: false,
};

if (this.lockFile.save) {
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",
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
  });
}

}
Comment on lines +140 to +145
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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",
});
}

const actionLock = actionLockProcessor.bind(this, this.lockFile.path);
this.internalProcessors.schema.push(actionLock);
}

this.userActionRegistry = new ActionRegistry(this.client);

Expand Down Expand Up @@ -149,13 +182,17 @@ export class ComposioToolSet {
): Promise<RawActionData[]> {
const parsedFilters = ZToolSchemaFilter.parse(filters);

const apps = await this.client.actions.list({
const actionVersions = this.lockFile.path
? getVersionsFromLockFileAsJson(this.lockFile.path)
: {};
const actions = await this.client.actions.list({
apps: parsedFilters.apps?.join(","),
tags: parsedFilters.tags?.join(","),
useCase: parsedFilters.useCase,
actions: parsedFilters.actions?.join(","),
usecaseLimit: parsedFilters.useCaseLimit,
filterByAvailableApps: parsedFilters.filterByAvailableApps,
actionVersions: actionVersions,
});

const customActions = await this.userActionRegistry.getAllActions();
Expand All @@ -171,7 +208,7 @@ export class ComposioToolSet {
);
});

const toolsActions = [...(apps?.items || []), ...toolsWithCustomActions];
const toolsActions = [...(actions?.items || []), ...toolsWithCustomActions];

const allSchemaProcessor = [
...this.internalProcessors.schema,
Expand All @@ -182,6 +219,10 @@ export class ComposioToolSet {

return toolsActions.map((tool) => {
let schema = tool as RawActionData;
if (schema?.version) {
// store the version in the map for execution
this.actionVersionMap[schema?.name.toUpperCase()] = schema?.version;
Comment on lines +222 to +224
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;
}

}
allSchemaProcessor.forEach((processor) => {
schema = processor({
actionName: schema?.name,
Expand Down Expand Up @@ -277,11 +318,13 @@ export class ComposioToolSet {
});
}

const version = this.actionVersionMap[action.toUpperCase()];
const data = await this.client.getEntity(entityId).execute({
actionName: action,
params: params,
text: nlaText,
connectedAccountId: connectedAccountId,
version: version,
});

return this.processResponse(data, {
Expand Down
4 changes: 4 additions & 0 deletions js/src/sdk/models/Entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ export class Entity {
params,
text,
connectedAccountId,
version,
}: ExecuteActionParams): Promise<ActionExecuteResponse> {
Comment on lines 79 to 83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
params,
text,
connectedAccountId,
version,
}: ExecuteActionParams): Promise<ActionExecuteResponse> {
params,
text,
connectedAccountId,
version?: string | number,
}: ExecuteActionParams): Promise<ActionExecuteResponse> {

TELEMETRY_LOGGER.manualTelemetry(TELEMETRY_EVENTS.SDK_METHOD_INVOKED, {
method: "execute",
Expand All @@ -91,6 +92,7 @@ export class Entity {
params,
text,
connectedAccountId,
version,
});
const action = await this.actionsModel.get({
actionName: actionName,
Expand All @@ -104,6 +106,7 @@ export class Entity {
if (app.no_auth) {
return this.actionsModel.execute({
actionName: actionName,
version: version,
requestBody: {
input: params,
appName: action.appKey,
Expand All @@ -126,6 +129,7 @@ export class Entity {
}
return this.actionsModel.execute({
actionName: actionName,
version: version,
requestBody: {
// @ts-ignore
connectedAccountId: connectedAccount?.id as unknown as string,
Expand Down
4 changes: 4 additions & 0 deletions js/src/sdk/models/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ export class Actions {
showEnabledOnly: data.showEnabledOnly,
usecaseLimit: data.usecaseLimit || undefined,
useCase: data.useCase as string,
actionVersions: data.actionVersions,
},
body: {
useCase: data.useCase as string,
Expand Down Expand Up @@ -155,6 +156,9 @@ export class Actions {
path: {
actionId: parsedData.actionName,
},
query: {
version: parsedData.version,
},
});
return res!;
} catch (error) {
Expand Down
2 changes: 2 additions & 0 deletions js/src/sdk/types/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const ZGetListActionsParams = z.object({
.boolean()
.optional()
.describe("Filter actions by available apps"),
actionVersions: z.record(z.string()).optional(),
});

export const ZParameter = z.object({
Expand All @@ -36,6 +37,7 @@ export const ZCustomAuthParams = z.object({

export const ZExecuteParams = z.object({
actionName: z.string(),
version: z.string().optional(),
requestBody: z.object({
connectedAccountId: z.string().optional(),
input: z.record(z.unknown()).optional(),
Expand Down
1 change: 1 addition & 0 deletions js/src/sdk/types/entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export const ZExecuteActionParams = z.object({
params: z.record(z.any()).optional(),
text: z.string().optional(),
connectedAccountId: z.string().optional(),
version: z.string().optional(),
});

export const ZInitiateConnectionParams = z.object({
Expand Down
14 changes: 14 additions & 0 deletions js/src/sdk/utils/lock/load.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import fs from "fs";

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));
};
Comment on lines +3 to +14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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}`);
}
};

60 changes: 60 additions & 0 deletions js/src/sdk/utils/lockFile.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import logger from "../../utils/logger";

export const updateLockFileWithActionVersion = (
filePath: string,
actionName: string,
version: string
) => {
const actionVersions = getVersionsFromLockFileAsJson(filePath);
actionVersions[actionName] = version;
saveLockFile(filePath, actionVersions);
};
Comment on lines +3 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);
};


export const getVersionsFromLockFileAsJson = (filePath: string) => {
try {
const lockFileContent = require("fs").readFileSync(filePath, "utf8");

Check warning on line 15 in js/src/sdk/utils/lockFile.ts

View workflow job for this annotation

GitHub Actions / lint-and-prettify

A `require()` style import is forbidden

Check warning on line 15 in js/src/sdk/utils/lockFile.ts

View workflow job for this annotation

GitHub Actions / lint-and-prettify

A `require()` style import is forbidden
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);

Check warning on line 46 in js/src/sdk/utils/lockFile.ts

View workflow job for this annotation

GitHub Actions / lint-and-prettify

A `require()` style import is forbidden

Check warning on line 46 in js/src/sdk/utils/lockFile.ts

View workflow job for this annotation

GitHub Actions / lint-and-prettify

A `require()` style import is forbidden
Comment on lines +15 to +46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);

} catch (e) {
const error = e as NodeJS.ErrnoException;
if (error.code === "EACCES" || error.code === "EPERM") {
logger.error("Permission denied writing to lock file", e);
throw new Error("Permission denied writing to lock file");
} else if (error.code === "ENOENT") {
logger.error("Directory does not exist for lock file", e);
throw new Error("Directory does not exist for lock file");
} else {
logger.error("Error writing to lock file", e);
throw new Error("Error writing to lock file");
}
}
};
19 changes: 19 additions & 0 deletions js/src/sdk/utils/processor/action_lock.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { RawActionData } from "../../../types/base_toolset";
import { updateLockFileWithActionVersion } from "../lockFile";

export const actionLockProcessor = (
filePath: string,
{
actionName,
toolSchema,
}: {
actionName: string;
toolSchema: RawActionData;
}
): RawActionData => {
const version = toolSchema.version;

updateLockFileWithActionVersion(filePath, actionName, version);

return toolSchema;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
};

};
2 changes: 2 additions & 0 deletions js/src/types/base_toolset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export const ZRawActionSchema = z.object({
name: z.string(),
toolName: z.string().optional(),
}),
version: z.string().optional(),
availableVersions: z.array(z.string()).optional(),
});

export type RawActionData = z.infer<typeof ZRawActionSchema>;
Expand Down
Loading