feat(ai): add code interpreter for AI data analysis#16559
feat(ai): add code interpreter for AI data analysis#16559FelixMalfait merged 11 commits intomainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 85 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 12 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/types/code-interpreter-input.type.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/types/code-interpreter-input.type.ts:1">
P2: Inconsistent pattern: This file manually defines types instead of inferring them from the Zod schema (`CodeInterpreterInputZodSchema`) like other tool type files do. This could lead to type drift if the schema changes. Follow the pattern from `http-request-input.type.ts` and `send-email-input.type.ts`.</violation>
</file>
<file name="packages/twenty-server/src/engine/metadata-modules/ai/ai-chat/utils/extract-code-interpreter-files.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/ai/ai-chat/utils/extract-code-interpreter-files.util.ts:81">
P2: Hardcoded path `/home/user/` should be extracted to a named constant shared with the code interpreter execution environment to ensure consistency.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/code-interpreter/drivers/e2b.driver.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/code-interpreter/drivers/e2b.driver.ts:49">
P1: Security: Environment variable key is not escaped, allowing potential Python code injection. While the value is properly escaped, the key is inserted directly into the Python code template. If `context.env` keys come from untrusted sources, an attacker could inject arbitrary Python code.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/tool-provider/services/tool-provider.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/tool-provider/services/tool-provider.service.ts:176">
P2: The `executionContext` is missing `userId`, `userWorkspaceId`, and `onCodeExecutionUpdate` fields that `CodeInterpreterTool` uses. Without `onCodeExecutionUpdate`, real-time streaming of code execution output won't work when tools are obtained through this service. Consider extending `ToolSpecification` to include these fields or documenting that this code path doesn't support streaming.</violation>
</file>
<file name="packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts:113">
P2: Missing `CODE_INTERPRETER_TOOL` in `TOOL_PERMISSION_FLAGS` constant. This new permission is added to defaults but not registered as a tool permission in `constants/tool-permission-flags.ts`. As a result, `isToolPermission()` will return `false`, causing the permission check to use `canUpdateAllSettings` instead of `canAccessAllTools`, which is inconsistent with other tool permissions like `SEND_EMAIL_TOOL` and `HTTP_REQUEST_TOOL`. Add `'CODE_INTERPRETER_TOOL'` to the `TOOL_PERMISSION_FLAGS` array.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/code-interpreter/drivers/local.driver.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/code-interpreter/drivers/local.driver.ts:41">
P1: Path traversal vulnerability: `file.filename` is not sanitized before joining with `workDir`. If `filename` contains `../` sequences, files can be written outside the sandbox directory. Consider using `path.basename()` to strip directory components:
```js
await fs.writeFile(join(workDir, path.basename(file.filename)), file.content);
```</violation>
</file>
<file name="packages/twenty-front/src/modules/ai/components/CodeExecutionDisplay.tsx">
<violation number="1" location="packages/twenty-front/src/modules/ai/components/CodeExecutionDisplay.tsx:341">
P2: Using `filename` as the React key may cause issues if files with duplicate names are generated. Consider using `file.url` which is more likely to be unique.</violation>
<violation number="2" location="packages/twenty-front/src/modules/ai/components/CodeExecutionDisplay.tsx:344">
P2: Consider adding an `onError` handler to `StyledPreviewImage` to gracefully fall back to the file icon if the image fails to load.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts:537">
P2: `@IsOptional()` should be removed from `E2B_API_KEY`. The factory code throws if this key is missing when E2B is selected, so validation should catch this at startup rather than failing at runtime. Other required conditional keys (like `AUTH_GOOGLE_CLIENT_SECRET`, `BILLING_STRIPE_API_KEY`) don't use `@IsOptional()`.</violation>
</file>
<file name="packages/twenty-front/src/modules/ai/components/ToolStepRenderer.tsx">
<violation number="1" location="packages/twenty-front/src/modules/ai/components/ToolStepRenderer.tsx:163">
P2: Tool-level error (`errorText`) is not displayed when code_interpreter execution fails. If the tool fails at a higher level (timeout, service error), users will see "Failed" status without any error message. Consider passing `errorText` to handle this case, e.g., `stderr={codeOutput?.result?.stderr || errorText || ''}`.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/code-interpreter-tool.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/code-interpreter-tool.ts:285">
P2: Missing timeout for HTTP request. If the remote server is unresponsive, this could hang indefinitely. Consider adding a timeout option.</violation>
<violation number="2" location="packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/code-interpreter-tool.ts:358">
P1: Potential path traversal vulnerability: `file.filename` is used directly in file path construction without sanitization. Consider sanitizing the filename to remove path separators and traversal sequences (e.g., `path.basename(file.filename)` or a stricter sanitization function).</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| @@ -0,0 +1,9 @@ | |||
| export type CodeInterpreterFileInput = { | |||
There was a problem hiding this comment.
P2: Inconsistent pattern: This file manually defines types instead of inferring them from the Zod schema (CodeInterpreterInputZodSchema) like other tool type files do. This could lead to type drift if the schema changes. Follow the pattern from http-request-input.type.ts and send-email-input.type.ts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/types/code-interpreter-input.type.ts, line 1:
<comment>Inconsistent pattern: This file manually defines types instead of inferring them from the Zod schema (`CodeInterpreterInputZodSchema`) like other tool type files do. This could lead to type drift if the schema changes. Follow the pattern from `http-request-input.type.ts` and `send-email-input.type.ts`.</comment>
<file context>
@@ -0,0 +1,9 @@
+export type CodeInterpreterFileInput = {
+ filename: string;
+ url: string;
</file context>
|
|
||
| newParts.push({ | ||
| type: 'text', | ||
| text: `\n\n[Files available for code interpreter at /home/user/:\n${fileList}]\n\nUse the code_interpreter tool to analyze these files.`, |
There was a problem hiding this comment.
P2: Hardcoded path /home/user/ should be extracted to a named constant shared with the code interpreter execution environment to ensure consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/metadata-modules/ai/ai-chat/utils/extract-code-interpreter-files.util.ts, line 81:
<comment>Hardcoded path `/home/user/` should be extracted to a named constant shared with the code interpreter execution environment to ensure consistency.</comment>
<file context>
@@ -0,0 +1,95 @@
+
+ newParts.push({
+ type: 'text',
+ text: `\n\n[Files available for code interpreter at /home/user/:\n${fileList}]\n\nUse the code_interpreter tool to analyze these files.`,
+ });
+ }
</file context>
| .replace(/\\/g, '\\\\') | ||
| .replace(/'/g, "\\'"); | ||
|
|
||
| return `os.environ['${key}'] = '${escapedValue}'`; |
There was a problem hiding this comment.
P1: Security: Environment variable key is not escaped, allowing potential Python code injection. While the value is properly escaped, the key is inserted directly into the Python code template. If context.env keys come from untrusted sources, an attacker could inject arbitrary Python code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/code-interpreter/drivers/e2b.driver.ts, line 49:
<comment>Security: Environment variable key is not escaped, allowing potential Python code injection. While the value is properly escaped, the key is inserted directly into the Python code template. If `context.env` keys come from untrusted sources, an attacker could inject arbitrary Python code.</comment>
<file context>
@@ -0,0 +1,109 @@
+ .replace(/\\/g, '\\\\')
+ .replace(/'/g, "\\'");
+
+ return `os.environ['${key}'] = '${escapedValue}'`;
+ })
+ .join('\n')}\n\n`
</file context>
|
|
||
| private async getActionTools(spec: ToolSpecification): Promise<ToolSet> { | ||
| const tools: ToolSet = {}; | ||
| const executionContext = { workspaceId: spec.workspaceId }; |
There was a problem hiding this comment.
P2: The executionContext is missing userId, userWorkspaceId, and onCodeExecutionUpdate fields that CodeInterpreterTool uses. Without onCodeExecutionUpdate, real-time streaming of code execution output won't work when tools are obtained through this service. Consider extending ToolSpecification to include these fields or documenting that this code path doesn't support streaming.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/tool-provider/services/tool-provider.service.ts, line 176:
<comment>The `executionContext` is missing `userId`, `userWorkspaceId`, and `onCodeExecutionUpdate` fields that `CodeInterpreterTool` uses. Without `onCodeExecutionUpdate`, real-time streaming of code execution output won't work when tools are obtained through this service. Consider extending `ToolSpecification` to include these fields or documenting that this code path doesn't support streaming.</comment>
<file context>
@@ -164,6 +173,7 @@ export class ToolProviderService {
private async getActionTools(spec: ToolSpecification): Promise<ToolSet> {
const tools: ToolSet = {};
+ const executionContext = { workspaceId: spec.workspaceId };
for (const [toolType, { tool, flag }] of this.actionTools) {
</file context>
packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/ai/components/CodeExecutionDisplay.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts
Show resolved
Hide resolved
| <CodeExecutionDisplay | ||
| code={codeInput?.code ?? ''} | ||
| stdout={codeOutput?.result?.stdout ?? ''} | ||
| stderr={codeOutput?.result?.stderr ?? ''} |
There was a problem hiding this comment.
P2: Tool-level error (errorText) is not displayed when code_interpreter execution fails. If the tool fails at a higher level (timeout, service error), users will see "Failed" status without any error message. Consider passing errorText to handle this case, e.g., stderr={codeOutput?.result?.stderr || errorText || ''}.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/ai/components/ToolStepRenderer.tsx, line 163:
<comment>Tool-level error (`errorText`) is not displayed when code_interpreter execution fails. If the tool fails at a higher level (timeout, service error), users will see "Failed" status without any error message. Consider passing `errorText` to handle this case, e.g., `stderr={codeOutput?.result?.stderr || errorText || ''}`.</comment>
<file context>
@@ -141,6 +142,32 @@ export const ToolStepRenderer = ({ toolPart }: { toolPart: ToolUIPart }) => {
+ <CodeExecutionDisplay
+ code={codeInput?.code ?? ''}
+ stdout={codeOutput?.result?.stdout ?? ''}
+ stderr={codeOutput?.result?.stderr ?? ''}
+ exitCode={codeOutput?.result?.exitCode}
+ files={codeOutput?.result?.files}
</file context>
| stderr={codeOutput?.result?.stderr ?? ''} | |
| stderr={codeOutput?.result?.stderr || errorText || ''} |
✅ Addressed in 4b90a1d
| } | ||
|
|
||
| // Handle HTTP URLs | ||
| const response = await this.httpService.axiosRef.get(file.url, { |
There was a problem hiding this comment.
P2: Missing timeout for HTTP request. If the remote server is unresponsive, this could hang indefinitely. Consider adding a timeout option.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/code-interpreter-tool.ts, line 285:
<comment>Missing timeout for HTTP request. If the remote server is unresponsive, this could hang indefinitely. Consider adding a timeout option.</comment>
<file context>
@@ -0,0 +1,435 @@
+ }
+
+ // Handle HTTP URLs
+ const response = await this.httpService.axiosRef.get(file.url, {
+ responseType: 'arraybuffer',
+ });
</file context>
✅ Addressed in 4b90a1d
...nty-server/src/engine/core-modules/tool/tools/code-interpreter-tool/code-interpreter-tool.ts
Outdated
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:35397 This environment will automatically shut down when the PR is closed or after 5 hours. |
- Extract getMimeType to shared utility to reduce code duplication - Fix security issue: escape single quotes in E2B driver env variable injection - Add CodeInterpreterTool to ToolProviderService for consistency - Create buildExecutionState helper to reduce duplicated state object construction - Add DEFAULT_CODE_INTERPRETER_TIMEOUT_MS constant - Fix lingui warning by extracting filename variable - Fix TypeScript errors: use theme.background.transparent.success/danger - Remove trailing empty lines from new files
FelixMalfait
force-pushed
the
code-mode-poc
branch
from
490b50f to
c4de608
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| ), | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Bug: Async callback causes race condition with file uploads
The onResult callback is defined as async but the StreamCallbacks interface specifies onResult as returning void. The drivers call callbacks?.onResult?.(outputFile) synchronously without awaiting, so the async upload runs in the background while execution continues. By the time uploadOutputFiles is called on line 189, streamedFiles may be incomplete because the async uploads haven't finished. This can cause duplicate file uploads and inconsistent streaming state.
Additional Locations (1)
packages/twenty-server/src/engine/core-modules/tool-provider/services/tool-provider.service.ts
Show resolved
Hide resolved
| // Allow requests to the server's own URL (for internal file downloads) | ||
| // but block all other private/internal IPs to prevent SSRF attacks | ||
| const isInternalFileUrl = file.url.startsWith(serverUrl); | ||
| const adapter = isInternalFileUrl ? undefined : getSecureAdapter(); |
There was a problem hiding this comment.
Bug: SSRF protection bypass via URL prefix matching
The SSRF protection check uses file.url.startsWith(serverUrl) to determine if a URL is internal, but this string prefix matching is insufficient. If SERVER_URL is https://app.example.com and an attacker crafts a URL like https://app.example.com.attacker.com/malicious, the startsWith check returns true, bypassing the secure adapter. The request then goes to the attacker-controlled domain without SSRF protection. The check should parse both URLs and compare hostnames to ensure they match exactly.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const content = await sbx.files.read( | ||
| `/home/user/output/${file.name}`, | ||
| ); | ||
|
|
||
| const outputFile: OutputFile = { | ||
| filename: file.name, | ||
| content: Buffer.from(content), |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| for (const file of files ?? []) { | ||
| const arrayBuffer = new Uint8Array(file.content).buffer; | ||
|
|
||
| await sbx.files.write(`/home/user/${file.filename}`, arrayBuffer); |
There was a problem hiding this comment.
Bug: E2B driver lacks path traversal protection for input files
The E2BDriver directly interpolates file.filename into the file path without sanitization, unlike the LocalDriver which properly uses basename(file.filename) to strip path traversal characters. A malicious filename like ../../../etc/passwd would be written to /home/user/../../../etc/passwd instead of /home/user/passwd. While E2B runs in a sandbox, this creates inconsistent behavior between development and production environments and could allow writing files to unexpected locations within the sandbox.
Additional Locations (1)
| sub: userId ?? workspaceId, | ||
| type: JwtTokenTypeEnum.ACCESS, | ||
| workspaceId, | ||
| userId: userId ?? workspaceId, | ||
| userWorkspaceId: userWorkspaceId ?? workspaceId, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return `os.environ['${key}'] = '${escapedValue}'`; | ||
| }) | ||
| .join('\n')}\n\n` | ||
| : ''; |
There was a problem hiding this comment.
Bug: Environment variable key not escaped in Python code generation
The PR description states this fixes "security issue: escape single quotes/backslashes in E2B driver env variable injection", but the fix is incomplete. While escapedValue properly escapes the environment variable value, the key variable is interpolated directly into the Python code without any escaping on line 88. If an environment variable key contains a single quote or bracket (e.g., test'] = 'x'; malicious_code(); os.environ['y), it could break out of the string literal and inject arbitrary Python code. The ExecutionContext interface allows arbitrary string keys via Record<string, string>, so both keys and values need escaping for the fix to be complete.
|
Hey @FelixMalfait! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
1 participant
Summary
Code Quality Improvements
getMimeTypeto shared utility to reduce code duplication between driversbuildExecutionStatehelper to reduce duplicated state object constructionDEFAULT_CODE_INTERPRETER_TIMEOUT_MSconstant for consistencyTest Plan
Note
Updates generated i18n catalogs for Polish and pseudo-English, adding strings for code execution/output (code interpreter) and various UI messages, with minor text adjustments.
locales/generated/pl-PL.tsandlocales/generated/pseudo-en.ts.pt-BR; other files unchanged functionally.Written by Cursor Bugbot for commit befc13d. This will update automatically on new commits. Configure here.