feat(build): Add support for Python scripts via pythonExtension
#1686
Conversation
🦋 Changeset detectedLatest commit: 311aed3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces a new module in the build system to manage Python extensions. It defines a new type for Python configuration, creates a Changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/build/src/extensions/python.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/build/src/extensions/python.ts (5)
8-21: Add doc comments for other properties
While pythonBinaryPath has helpful documentation, consider briefly documenting how "requirements" and "requirementsFile" are expected to be used for added clarity.
30-41: Filter out blank lines or comment lines
When reading requirements from the file, consider filtering out empty or comment lines to avoid unexpected installation errors if the file has whitespace or commented lines.You could do something like this:
this.options.requirements = fs .readFileSync(this.options.requirementsFile, "utf-8") - .split("\n"); + .split("\n") + .map(line => line.trim()) + .filter(line => line && !line.startsWith("#"));
43-50: Consider logging in development mode
Adding a small log statement clarifying that Python installation steps are skipped in development mode can help with visibility and troubleshooting.
52-86: Optimize Docker layering and fix spelling
• To speed up builds, you might separate the Python install from the pip install steps into different layers so that changes to your requirements file don’t invalidate the entire layer.
• There is a minor spelling issue on line 75 where “dependenciess” is spelled with an extra “s.”Example to fix the spelling:
- # Install dependenciess + # Install dependencies
116-116: Consider consistent exports
Having both a named “run” export and a default export referring to the same entity can be confusing. Consider removing the default export or clearly documenting how each should be used to avoid duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (1)
packages/build/src/extensions/python.ts (1)
23-25: Looks good
Returning an instance of PythonExtension is straightforward and clear.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/build/src/extensions/python.ts (1)
97-100:⚠️ Potential issueShell injection vulnerability in command execution.
The use of
shell: truewith user-provided arguments poses a security risk.This was previously flagged in an earlier review. Please implement the suggested fix to use execa's argument array instead of shell execution.
🧹 Nitpick comments (3)
packages/build/src/extensions/python.ts (3)
8-21: Consider adding validation for requirements and pythonBinaryPath.The
PythonOptionstype could benefit from additional validation:
- For
requirements: Validate package names and version specifiers format- For
pythonBinaryPath: Add OS-specific path validationConsider adding validation in the constructor:
private validateRequirements(reqs: string[]) { const packagePattern = /^[a-zA-Z0-9\-_.]+(==|>=|<=|!=|~=|>|<)?[0-9a-zA-Z\-.]*$/; reqs.forEach(req => { assert(packagePattern.test(req.trim()), `Invalid requirement format: ${req}`); }); } private validatePythonPath(path: string) { if (process.platform === 'win32') { assert(path.endsWith('.exe'), 'Windows Python path must end with .exe'); } assert(fs.existsSync(path), `Python binary not found at ${path}`); }
62-77: Optimize Docker layer setup for better caching.The current Docker layer setup could be optimized to better leverage Docker's layer caching:
- Separate Python installation and requirements installation into distinct layers
- Use multi-stage builds to reduce final image size
- Consider using
--no-install-recommendsfor pip installationsConsider this optimized Dockerfile structure:
- # Install Python - RUN apt-get update && apt-get install -y --no-install-recommends \ - python3 python3-pip python3-venv && \ - apt-get clean && rm -rf /var/lib/apt/lists/* - - # Set up Python environment - RUN python3 -m venv /opt/venv - ENV PATH="/opt/venv/bin:$PATH" - - ARG REQUIREMENTS_CONTENT - RUN echo "$REQUIREMENTS_CONTENT" > requirements.txt - - # Install dependenciess - RUN pip install --no-cache-dir -r requirements.txt + # Install Python in a separate layer + RUN apt-get update \ + && apt-get install -y --no-install-recommends \ + python3 \ + python3-pip \ + python3-venv \ + && apt-get clean \ + && rm -rf /var/lib/apt/lists/* + + # Set up venv in a separate layer + RUN python3 -m venv /opt/venv \ + && /opt/venv/bin/pip install --no-cache-dir --upgrade pip + + ENV PATH="/opt/venv/bin:$PATH" + + # Install dependencies in a separate layer + ARG REQUIREMENTS_CONTENT + RUN if [ ! -z "$REQUIREMENTS_CONTENT" ]; then \ + echo "$REQUIREMENTS_CONTENT" > requirements.txt \ + && pip install --no-cache-dir -r requirements.txt \ + && rm requirements.txt; \ + fi
102-108: Enhance error handling for Python-specific errors.The current error handling doesn't distinguish between different types of Python errors (syntax errors, import errors, etc.).
Consider parsing Python's error output:
try { assert(!result.failed, `Command failed: ${result.stderr}`); assert(result.exitCode === 0, `Non-zero exit code: ${result.exitCode}`); } catch (e) { // Parse Python error type from stderr const errorMatch = result.stderr.match(/^([A-Za-z]+Error):/m); if (errorMatch) { logger.error(`Python ${errorMatch[1]}: ${e.message}`, result); } else { logger.error(e.message, result); } throw e; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/build/src/extensions/python.ts (2)
23-27: 🛠️ Refactor suggestionImprove requirements parsing robustness.
The current implementation doesn't handle advanced requirements.txt features.
Consider this improved parsing that handles more edge cases:
-const splitAndCleanComments = (str: string) => - str - .split("\n") - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith("#")); +const splitAndCleanComments = (str: string) => + str + .split("\n") + .map((line) => line.trim()) + .filter((line) => line && !line.startsWith("#")) + .map((line) => { + // Handle line continuations + if (line.endsWith("\\")) { + return line.slice(0, -1).trim(); + } + // Extract package name before any version specifiers or options + const [name] = line.split(/[=<>~]/); + return name.trim(); + }) + .filter((line) => !line.startsWith("-r")); // Skip recursive requirements
113-116:⚠️ Potential issueFix shell injection vulnerability.
Using shell=true with user-provided arguments is unsafe.
Apply this fix:
- const result = await $({ - shell: true, - ...options, - })(pythonBin, ...scriptArgs); + const result = await $({ + ...options, + })(pythonBin, ...scriptArgs);
🧹 Nitpick comments (1)
packages/build/src/extensions/python.ts (1)
129-132: Improve input validation for inline scripts.The current validation only checks if the script content exists.
Consider adding more validation:
-export const runInline = (scriptContent: string, options: Parameters<typeof $>[1] = {}) => { - assert(scriptContent, "Script content is required"); - return run([""], { input: scriptContent, ...options }); +export const runInline = (scriptContent: string, options: Parameters<typeof $>[1] = {}) => { + assert(scriptContent, "Script content is required"); + assert( + typeof scriptContent === "string" && scriptContent.trim().length > 0, + "Script content must be a non-empty string" + ); + // Validate script syntax before execution + try { + return run(["-c", "import ast; ast.parse(input())"], { + input: scriptContent, + ...options, + }); + } catch (e) { + throw new Error(`Invalid Python syntax: ${e.message}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/build/src/extensions/python.ts (2)
31-35: 🛠️ Refactor suggestionEnhance requirements file parsing.
The current implementation doesn't handle advanced requirements.txt features.
Consider this improved parsing:
const splitAndCleanComments = (str: string) => str .split('\n') .map(line => line.trim()) .filter(line => line && !line.startsWith('#')) .map(line => { // Handle line continuations if (line.endsWith('\\')) { return line.slice(0, -1).trim(); } // Extract package name before any version specifiers or options return line.split(/[=<>~\s]/)[0]; }) .filter(Boolean);
102-108:⚠️ Potential issueEnhance security of requirements installation.
The current approach of echoing requirements to a file and installing them could be improved.
Consider these security enhancements:
ARG REQUIREMENTS_CONTENT -RUN echo "$REQUIREMENTS_CONTENT" > requirements.txt - -# Install dependencies -RUN pip install --no-cache-dir -r requirements.txt +# Create requirements file with proper permissions +RUN echo "$REQUIREMENTS_CONTENT" > requirements.txt && \ + chmod 644 requirements.txt && \ + # Verify package integrity during installation + pip install --no-cache-dir --require-hashes -r requirements.txt && \ + # Clean up + rm requirements.txt
🧹 Nitpick comments (2)
packages/build/src/extensions/python.ts (2)
9-29: Add validation for pythonBinaryPath format.Consider adding platform-specific path format validation for the
pythonBinaryPathoption to prevent runtime issues.function validatePythonPath(path: string): boolean { if (process.platform === 'win32') { return /^[a-zA-Z]:\\.*\\python\.exe$/i.test(path); } return /^(\/[\w-]+)+\/python[23]?$/.test(path); }
153-157: Add input validation for inline scripts.Validate the script content to ensure it's not empty or malformed.
export const runInline = (scriptContent: string, options: Parameters<typeof $>[1] = {}) => { assert(scriptContent, "Script content is required"); + assert( + scriptContent.trim().length > 0, + "Script content cannot be empty" + ); + assert( + !scriptContent.includes("import os; os.system"), + "Direct system commands are not allowed" + ); return run([""], { input: scriptContent, ...options }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (1)
packages/build/src/extensions/python.ts (1)
117-140:⚠️ Potential issueRemove shell execution for better security.
Using
shell: truecan expose the application to command injection vulnerabilities.- const result = await execa({ - shell: true, - verbose: (verboseLine, verboseObject) => logger.debug(verboseLine, verboseObject), - ...options, - })(pythonBin, scriptArgs); + const result = await execa(pythonBin, scriptArgs, { + verbose: (verboseLine, verboseObject) => logger.debug(verboseLine, verboseObject), + ...options, + });Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/build/src/extensions/python.ts (2)
11-31: Add validation for pythonBinaryPath and scripts options.The type definition is well-documented, but could benefit from additional runtime validation:
pythonBinaryPathshould be validated to ensure it points to a valid Python executablescriptsglob patterns should be validated for syntaxConsider adding these validations in the constructor:
if (this.options.pythonBinaryPath) { assert( fs.existsSync(this.options.pythonBinaryPath), `Python binary not found: ${this.options.pythonBinaryPath}` ); assert( fs.statSync(this.options.pythonBinaryPath).mode & fs.constants.X_OK, `Python binary is not executable: ${this.options.pythonBinaryPath}` ); } if (this.options.scripts) { assert( Array.isArray(this.options.scripts) && this.options.scripts.every(s => typeof s === 'string'), 'scripts must be an array of strings' ); // Optional: Add glob pattern validation }
131-137: Enhance error handling with detailed error types.The current error handling could be more informative by using custom error types and preserving the error chain.
Consider this improvement:
class PythonExecutionError extends Error { constructor( message: string, public readonly result: ExecaReturnValue, public readonly cause?: Error ) { super(message); this.name = 'PythonExecutionError'; } } try { assert(!result.failed, `Command failed: ${result.stderr}`); assert(result.exitCode === 0, `Non-zero exit code: ${result.exitCode}`); } catch (e) { logger.error(e.message, result); throw new PythonExecutionError( `Python execution failed: ${e.message}`, result, e ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
🔇 Additional comments (2)
packages/build/src/extensions/python.ts (2)
35-43: Improve requirements file parsing.The current implementation could be enhanced to handle more complex requirements file formats.
66-72: undefined
It has now been fixed! Yupi! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/little-trains-begin.md (1)
5-5: Clear Update Description Provided.
The message clearly outlines the new Python extension’s purpose—enhancing script execution and error handling. For extra clarity, consider linking to more detailed documentation or including a bit more context (e.g., mentioning that this addresses earlier deployment concerns) if it adds value in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/little-trains-begin.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
🔇 Additional comments (1)
.changeset/little-trains-begin.md (1)
1-3: Validate Changeset Header Format.
The changeset header correctly declares a minor update for "@trigger.dev/build" using the expected YAML delimiter syntax. This aligns well with our versioning guidelines.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/build/src/extensions/python.ts (2)
24-30: Enhance documentation for scripts option security implications.The documentation for the
scriptsoption should include security considerations and best practices.Add the following to the documentation:
/** * An array of glob patterns that specify which Python scripts are allowed to be executed. * * @remarks * These scripts will be copied to the container during the build process. + * + * @security + * - Ensure glob patterns are specific to avoid including unintended scripts + * - Avoid patterns that could match user-uploaded content + * - Consider using allowlists instead of broad patterns */
131-137: Enhance error handling with detailed error information.The current error handling could provide more context about the failure.
Apply this diff to improve error handling:
try { - assert(!result.failed, `Command failed: ${result.stderr}`); - assert(result.exitCode === 0, `Non-zero exit code: ${result.exitCode}`); + assert(!result.failed, `Python command failed: ${result.stderr}\nCommand: ${result.command}`); + assert(result.exitCode === 0, `Python command exited with non-zero code ${result.exitCode}\nStdout: ${result.stdout}\nStderr: ${result.stderr}`); } catch (e) { - logger.error(e.message, result); + logger.error("Python command execution failed", { + error: e.message, + command: result.command, + stdout: result.stdout, + stderr: result.stderr, + exitCode: result.exitCode + }); throw e; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/build/package.json(1 hunks)packages/build/src/extensions/python.ts(1 hunks)
🔇 Additional comments (2)
packages/build/package.json (1)
69-70: Dependency Update: Adding New Python Extension Support Dependencies.The additions of "@trigger.dev/sdk": "workspace:3.3.13" and "execa": "^9.5.2" in the dependencies block appropriately support the new pythonExtension functionality. Both dependencies align with the PR objectives and are correctly specified.
packages/build/src/extensions/python.ts (1)
90-95: Consider environment variable conflict resolution strategy.The
override: trueflag for environment variables could lead to conflicts if multiple extensions set the same variables.Consider implementing a conflict resolution strategy:
- Document the behavior when conflicts occur
- Add a warning when conflicting environment variables are detected
Run this script to check for potential environment variable conflicts:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other extensions that might set PYTHON_BIN_PATH rg -l "PYTHON_BIN_PATH" --type tsLength of output: 73
Environment Variable Conflict Check Outcome
Our search found that PYTHON_BIN_PATH is only defined in packages/build/src/extensions/python.ts, with no other instances in the repository. This suggests there is currently no conflict with environment variable assignment. However, consider the following for future-proofing:
- Document that the PYTHON_BIN_PATH is uniquely used by the Python extension.
- Consider adding warning logs or notices if a conflict is detected when multiple extensions are used.
- Review the override behavior in case other extensions are introduced that might reuse similar environment variable names.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| assert(scriptPath, "Script path is required"); | ||
| assert(fs.existsSync(scriptPath), `Script does not exist: ${scriptPath}`); |
There was a problem hiding this comment.
TODO
- in dev, we should check whether
scriptPathmatches the patterns defined inthis.options.scriptsotherwise throw an error even if file exists locally (fail-fast approach to avoid errors only when deploying) - To do so, we better define a
doesMatchPatternfunction (insideadditionalFiles.ts?) similar to findStaticAssetFiles
trigger.dev/packages/build/src/extensions/core/additionalFiles.ts
Lines 37 to 41 in bc7d445
Questions
- How do I check if I am in dev when I have no access to
context: BuildContext?process.env.NODE_DEV === 'development'? - Should
doesMatchPatternbe defined locally or exported fromadditionalFiles.ts?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/build/src/extensions/python.ts (1)
45-123: Add script pattern validation in dev mode.The implementation looks good, but as noted in the TODO comment, we should validate script paths against patterns in dev mode to fail fast.
Would you like me to help implement the
doesMatchPatternfunction and the dev mode validation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/build/package.json(4 hunks)packages/build/src/extensions/python.ts(1 hunks)references/v3-catalog/trigger.config.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/build/src/extensions/python.ts (3)
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:0-0
Timestamp: 2025-02-10T11:19:37.014Z
Learning: In the Python extension for Trigger.dev, do not enforce `.py` file extensions for Python scripts to maintain flexibility for developers.
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:110-116
Timestamp: 2025-02-10T10:56:31.402Z
Learning: In Docker build contexts for Trigger.dev extensions, avoid over-engineering security measures when handling user-provided configuration (like Python requirements) as the build context is already isolated and the content is user-controlled.
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:85-87
Timestamp: 2025-02-10T10:54:17.345Z
Learning: In Python-related Dockerfiles for trigger.dev, avoid adding explicit Python version pinning as the base image already provides conservative version management. Additional pinning would unnecessarily slow down builds.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (5)
references/v3-catalog/trigger.config.ts (1)
10-10: LGTM! Python extension is correctly integrated.The Python extension is properly imported and added to the build extensions array.
Also applies to: 90-90
packages/build/src/extensions/python.ts (3)
1-34: LGTM! Well-structured type definitions and imports.The code has clear type definitions and proper imports. The
PythonOptionstype is well-documented with JSDoc comments.
125-155: LGTM! Robust command execution implementation.The
runfunction includes:
- Proper error handling with detailed error messages
- Comprehensive logging
- Safe command execution using execa
157-183: LGTM! Secure script execution implementation.The implementation includes:
- Proper validation of script paths and content
- Secure handling of inline scripts using temporary files
- Proper cleanup of temporary files
packages/build/package.json (1)
31-31: LGTM! Package configuration is properly updated.The changes correctly:
- Add Python extension to exports and typesVersions
- Include necessary dependencies
- Follow the package's structure for other extensions
Also applies to: 55-57, 73-74, 159-169
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/build/src/extensions/python.ts (4)
11-31: Add validation constraints to PythonOptions type.Consider adding Zod schema validation to enforce:
- Non-empty array for requirements
- Valid file path format for pythonBinaryPath
- Valid glob patterns for scripts
import { z } from "zod"; const PythonOptionsSchema = z.object({ requirements: z.array(z.string()).min(1).optional(), requirementsFile: z.string().optional(), pythonBinaryPath: z.string().regex(/^[\/\\]?[\w\-. ]+(?:[\/\\][\w\-. ]+)*$/).optional(), scripts: z.array(z.string()).min(1).optional(), }).refine(data => !(data.requirements && data.requirementsFile), { message: "Cannot specify both requirements and requirementsFile" }); export type PythonOptions = z.infer<typeof PythonOptionsSchema>;
35-40: Enhance requirements parsing error handling.The current implementation might fail silently on malformed input. Consider adding error handling for common edge cases.
const splitAndCleanComments = (str: string) => - str - .split("\n") - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith("#")); + str + .split("\n") + .map((line, index) => { + const trimmed = line.trim(); + if (trimmed.includes("\\")) { + logger.warn(`Line continuation found at line ${index + 1}, this is not supported yet`); + } + return trimmed; + }) + .filter((line) => line && !line.startsWith("#")) + .map(line => { + const [pkg] = line.split(/[><=~^]/); + return pkg.trim(); + });
162-163: Address TODO: Implement script pattern validation.The TODO comment indicates a need for script pattern validation in dev mode. I can help implement this feature.
Would you like me to generate an implementation for:
- A
doesMatchPatternfunction inadditionalFiles.ts- Dev mode detection using
process.env.NODE_ENV- Integration with the
runScriptfunction?Here's a preview of the implementation:
// In additionalFiles.ts export const doesMatchPattern = (filePath: string, patterns: string[]) => { const { isMatch } = await import("micromatch"); return patterns.some(pattern => isMatch(filePath, pattern)); }; // In python.ts export const runScript = ( scriptPath: string, scriptArgs: string[] = [], options: ExecaOptions = {} ) => { assert(scriptPath, "Script path is required"); assert(fs.existsSync(scriptPath), `Script does not exist: ${scriptPath}`); if (process.env.NODE_ENV === "development" && this.options.scripts) { assert( doesMatchPattern(scriptPath, this.options.scripts), `Script ${scriptPath} does not match any allowed patterns: ${this.options.scripts.join(", ")}` ); } return run([scriptPath, ...scriptArgs], options); };
137-152: Enhance error messages with more context.The error messages could be more descriptive to help with debugging.
assert( result.exitCode === 0, - `Python command exited with non-zero code ${result.exitCode}\nStdout: ${result.stdout}\nStderr: ${result.stderr}` + `Python command failed with exit code ${result.exitCode} + Command: ${result.command} + Working Directory: ${options.cwd || process.cwd()} + Environment: ${JSON.stringify(options.env || {})} + Stdout: ${result.stdout} + Stderr: ${result.stderr}` ); } catch (error) { logger.error("Python command execution failed", { error: error instanceof Error ? error.message : error, + workingDirectory: options.cwd || process.cwd(), + environment: options.env || {}, command: result.command, stdout: result.stdout, stderr: result.stderr, exitCode: result.exitCode, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/extensions/python.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/build/src/extensions/python.ts (3)
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:0-0
Timestamp: 2025-02-10T11:19:37.014Z
Learning: In the Python extension for Trigger.dev, do not enforce `.py` file extensions for Python scripts to maintain flexibility for developers.
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:110-116
Timestamp: 2025-02-10T10:56:31.402Z
Learning: In Docker build contexts for Trigger.dev extensions, avoid over-engineering security measures when handling user-provided configuration (like Python requirements) as the build context is already isolated and the content is user-controlled.
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:85-87
Timestamp: 2025-02-10T10:54:17.345Z
Learning: In Python-related Dockerfiles for trigger.dev, avoid adding explicit Python version pinning as the base image already provides conservative version management. Additional pinning would unnecessarily slow down builds.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/build/src/extensions/python.ts (2)
9-29: Enhance type definitions for better type safety.Consider adding constraints to the type definition:
export type PythonOptions = { - requirements?: string[]; + requirements?: readonly string[]; requirementsFile?: string; pythonBinaryPath?: string; - scripts?: string[]; + scripts?: readonly string[]; + /** + * @throws {Error} If any script pattern is invalid or potentially unsafe + */
81-83: Optimize the Python installation layer.Consider combining the cleanup commands in the same layer to reduce the image size.
- RUN apt-get update && apt-get install -y --no-install-recommends \ - python3 python3-pip python3-venv && \ - apt-get clean && rm -rf /var/lib/apt/lists/* + RUN apt-get update \ + && apt-get install -y --no-install-recommends python3 python3-pip python3-venv \ + && apt-get clean \ + && rm -rf /var/lib/apt/lists/* \ + && pip install --no-cache-dir --upgrade pip
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/build/package.json(4 hunks)packages/build/src/extensions/python.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/build/src/extensions/python.ts (3)
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:0-0
Timestamp: 2025-02-10T11:19:37.014Z
Learning: In the Python extension for Trigger.dev, do not enforce `.py` file extensions for Python scripts to maintain flexibility for developers.
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:110-116
Timestamp: 2025-02-10T10:56:31.402Z
Learning: In Docker build contexts for Trigger.dev extensions, avoid over-engineering security measures when handling user-provided configuration (like Python requirements) as the build context is already isolated and the content is user-controlled.
Learnt from: zvictor
PR: triggerdotdev/trigger.dev#1686
File: packages/build/src/extensions/python.ts:85-87
Timestamp: 2025-02-10T10:54:17.345Z
Learning: In Python-related Dockerfiles for trigger.dev, avoid adding explicit Python version pinning as the base image already provides conservative version management. Additional pinning would unnecessarily slow down builds.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
🔇 Additional comments (6)
packages/build/package.json (4)
31-31: Python Extension Export in the "tshy" Block
The new export entry"./extensions/python": "./src/extensions/python.ts"is added consistently with the other extension exports under the
"tshy"configuration section. This looks correct provided that the file structure places the Python extension source at the indicated path.
55-57: TypesVersions Update for the Python Extension
The addition of the types mapping:"extensions/python": [ "dist/commonjs/extensions/python.d.ts" ]is consistent with the pattern used by existing extensions. Just confirm that the generated declaration file is up-to-date with the current implementation of the Python extension.
73-75: New Dependency Additions for Python Extension Support
The following new dependencies have been added:
"@trigger.dev/sdk": "workspace:3.3.13""tinyexec": "^0.3.2"These additions are likely needed for the Python extension functionality. Please verify that these versions are compatible with the rest of the codebase and that there are no conflicts.
159-169: Export Block for Python Extension
The new export block:"./extensions/python": { "import": { "@triggerdotdev/source": "./src/extensions/python.ts", "types": "./dist/esm/extensions/python.d.ts", "default": "./dist/esm/extensions/python.js" }, "require": { "types": "./dist/commonjs/extensions/python.d.ts", "default": "./dist/commonjs/extensions/python.js" } }is structured consistently with the other extension exports. This setup ensures that both ESM and CommonJS consumers can correctly import the Python extension. Please verify that the built files (
python.jsandpython.d.ts) are generated at the specified locations during the build process.packages/build/src/extensions/python.ts (2)
31-39: Well-structured initialization code!The helper function and factory pattern implementation look good. The
splitAndCleanCommentsfunction effectively handles comments and empty lines as discussed in previous reviews.
121-173: Robust implementation of Python execution utilities!The implementation includes:
- Proper error handling with detailed logging
- Secure execution of inline scripts using temporary files
- Clean separation of concerns between different execution modes
| assert(scriptPath, "Script path is required"); | ||
| assert(fs.existsSync(scriptPath), `Script does not exist: ${scriptPath}`); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address the TODO comment about script pattern validation.
As noted in the TODO comment, we need to implement the fail-fast approach for script pattern validation in development.
Let's verify the current behavior and propose a solution:
🏁 Script executed:
#!/bin/bash
# Search for any existing pattern matching utilities
rg -A 5 "doesMatchPattern|findStaticAssetFiles" .Length of output: 1173
Implement the script pattern validation check in development
The search confirms that the functionality to validate script patterns (using a fail-fast approach during development) hasn’t been implemented in the Python extension. To address the TODO, consider adding a validation function similar to the following:
- Create a utility function (e.g.,
validateScriptPattern) that verifies the script’s filename (or full path) complies with the expected pattern (for example, ensuring it ends with.py). - Invoke this function immediately after the existing assertions, but only when in development mode (e.g., by checking
process.env.NODE_ENVor a similar flag). - Fail fast by throwing an error with a descriptive message if the validation fails.
Example implementation snippet:
// packages/build/src/extensions/python.ts
import * as path from 'path';
/**
* Validates that the script matches the expected Python script pattern.
* @param scriptPath The path of the script.
*/
function validateScriptPattern(scriptPath: string) {
const scriptName = path.basename(scriptPath);
// Define your expected pattern for Python scripts (customize as needed)
const pattern = /^[\w\-]+\.py$/;
if (!pattern.test(scriptName)) {
throw new Error(`Invalid script file name: ${scriptName}. Ensure it follows the expected naming convention.`);
}
}
// Existing assertions
assert(scriptPath, "Script path is required");
assert(fs.existsSync(scriptPath), `Script does not exist: ${scriptPath}`);
// In development, validate the script pattern early
if (process.env.NODE_ENV === 'development') {
validateScriptPattern(scriptPath);
}This approach ensures that any deviation from the expected script naming convention is caught early in the development phase, adhering to the fail-fast principle.
Description:
This PR introduces the
pythonExtensionbuild extension, enabling limited support for running Python scripts within Trigger.dev tasks.Discussion.
Key features:
pip.requirements.txtfile.trigger.config.tsfile using therequirementsoption./opt/venv) inside the containers to isolate Python dependencies.runfunction for executing Python scripts with proper environment setup, arunInlinefor quick python coding from node, andrunScriptfor proper .py scripts execution.pythonBinaryPathcan be set to use any python installation.Usage:
pythonExtensionto yourtrigger.config.tsfile:(optional) Create a
requirements.txtfile in your project root with your desired Python dependencies.Use the
runfunction to execute Python scripts within your tasks:Beware: it does not take care of copying your python scripts to the staging/production containers.fixed!Limitations:
Summary by CodeRabbit
Summary by CodeRabbit