Skip to content
Closed
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
22 changes: 22 additions & 0 deletions packages/cli-v3/test-dotenv.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { resolveDotEnvVars } from "./src/utilities/dotEnv.js";
import fs from "node:fs";
import { resolve } from "node:path";

const tempDir = resolve(process.cwd(), "temp-env-test");
if (!fs.existsSync(tempDir)) fs.mkdirSync(tempDir);

fs.writeFileSync(resolve(tempDir, ".env"), "TEST_VAR=base\nTRIGGER_API_URL=http://base");
fs.writeFileSync(resolve(tempDir, ".env.local"), "TEST_VAR=override");

console.log("Testing with .env and .env.local...");
const vars = resolveDotEnvVars(tempDir);
console.log("Resolved vars:", vars);

if (vars.TEST_VAR === "override") {
console.log("✅ SUCCESS: .env.local overrides .env");
} else {
console.log("❌ FAILURE: .env.local did NOT override .env. Got:", vars.TEST_VAR);
process.exit(1);
}

fs.rmSync(tempDir, { recursive: true, force: true });
69 changes: 69 additions & 0 deletions packages/core/test/v3/zodNamespace.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { describe, it, expect, vi } from "vitest";
import { ZodNamespace } from "../../src/v3/zodNamespace.js";
import { z } from "zod";
import { Server } from "socket.io";
import { createServer } from "node:http";

describe("ZodNamespace", () => {
it("should allow sending messages with the ZodSocketMessageCatalogSchema structure", async () => {
const io = new Server(createServer());

const clientMessages = {
CLIENT_MSG: {
message: z.object({ foo: z.string() })
}
};

const serverMessages = {
SERVER_MSG: {
message: z.object({ bar: z.number() })
}
};

const ns = new ZodNamespace({
io,
name: "test",
clientMessages,
serverMessages,
});

const emitSpy = vi.spyOn(ns.namespace, "emit");

// This should not throw and should emit the correct payload
// Currently this might throw or require passing { message: { bar: 1 } }
await ns.sender.send("SERVER_MSG", { bar: 1 } as any);

expect(emitSpy).toHaveBeenCalledWith("SERVER_MSG", {
payload: { bar: 1 },
version: "v1"
});
});
Comment on lines +1 to +40

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Unrelated leftover scratch files are included in the pull request

Two files unrelated to Python build support are committed — a throwaway env-loading script (packages/cli-v3/test-dotenv.ts) and a socket namespace test (packages/core/test/v3/zodNamespace.test.ts) — so the change no longer addresses a single issue and adds a test that fails.
Impact: Reviewers get unrelated code, and the newly added test fails, turning the build red.

Why the added test fails and why the files violate repo rules

CONTRIBUTING.md: "We only accept PRs that address a single issue. Please do not submit PRs containing multiple unrelated fixes or features."

packages/core runs vitest over test/** (packages/core/package.json:167), so packages/core/test/v3/zodNamespace.test.ts executes in CI. It will fail:

  • ZodNamespace passes the socket catalog ({ SERVER_MSG: { message: zodSchema } }) straight into ZodMessageSender (packages/core/src/v3/zodNamespace.ts:111-115), and send() calls schema.safeParse(payload) on that plain object (packages/core/src/v3/zodMessageHandler.ts:273), which is not a function.
  • The assertion also expects emit("SERVER_MSG", { payload, version }), but the implementation emits emit(message.type, message.payload) (packages/core/src/v3/zodNamespace.ts:117).
  • Both socket.io Server instances are created and never closed, leaving open handles.

Both new files also use 4-space indentation, which does not match the Prettier configuration enforced by pnpm run format (AGENTS.md "Coding style").

Prompt for agents
Remove the unrelated files packages/cli-v3/test-dotenv.ts and packages/core/test/v3/zodNamespace.test.ts from this PR. The core test additionally fails against the current implementation (ZodNamespace passes the socket catalog directly to ZodMessageSender, whose send() calls safeParse on the catalog entry, and emits (type, payload) rather than an object), and it leaks socket.io servers. If the ZodNamespace behaviour genuinely needs fixing, do it in a separate PR with tests that match repo conventions and Prettier formatting.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


it("should support schemas with callbacks if updated", async () => {
// This test represents the desired state
const io = new Server(createServer());

const clientMessages = {
CLIENT_MSG: {
message: z.object({ foo: z.string() }),
callback: z.object({ ok: z.boolean() })
}
};

const serverMessages = {
SERVER_MSG: {
message: z.object({ bar: z.number() }),
callback: z.object({ success: z.boolean() })
}
} as any; // Cast for now until we update the types

const ns = new ZodNamespace({
io,
name: "test-cb",
clientMessages,
serverMessages,
});

expect(ns).toBeDefined();
});
});
49 changes: 47 additions & 2 deletions packages/python/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import { BuildContext, BuildExtension } from "@trigger.dev/core/v3/build";
export type PythonOptions = {
requirements?: string[];
requirementsFile?: string;
pyprojectFile?: string;
useUv?: boolean;
Comment on lines +10 to +11

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Python package release notes will be missing because no changeset was added

New Python build options are added to the published package (pyprojectFile/useUv at packages/python/src/extension.ts:10-11) without the required changeset entry, so the change ships with no version bump or changelog.
Impact: The feature can be released without a version bump or release-note entry for users.

Repository rule: changesets are mandatory for package changes

CONTRIBUTING.md ("Adding changesets") and CLAUDE.md both state that any change to a package under packages/* requires a changeset. git diff --name-only against the merge base shows only packages/cli-v3/test-dotenv.ts, packages/core/test/v3/zodNamespace.test.ts, and packages/python/src/extension.ts, with no file added under .changeset/.

Prompt for agents
The PR modifies the published package @trigger.dev/python but does not include a changeset. Per CONTRIBUTING.md and CLAUDE.md, run `pnpm run changeset:add` and select @trigger.dev/python (patch or minor as appropriate) so the new pyprojectFile/useUv options are versioned and appear in the changelog.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


/**
* [Dev-only] The path to the python binary.
*
Expand Down Expand Up @@ -54,6 +57,14 @@ class PythonExtension implements BuildExtension {
fs.readFileSync(this.options.requirementsFile, "utf-8")
);
}

if (this.options.pyprojectFile) {
assert(
fs.existsSync(this.options.pyprojectFile),
`pyproject.toml not found: ${this.options.pyprojectFile}`
);
}
Comment on lines +61 to +66

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Python dependency options can be combined and silently ignored

A project file can be supplied alongside inline or file-based requirements with no validation (pyprojectFile check at packages/python/src/extension.ts:61-66), so one of the dependency sources is silently dropped at deploy time.
Impact: Users who configure both see some of their Python dependencies never installed in the deployed image, with no warning.

Branch precedence in onBuildComplete

The constructor only asserts that requirements and requirementsFile are not both set. onBuildComplete picks exactly one branch: requirementsFile (packages/python/src/extension.ts:114), else pyprojectFile (:149), else requirements (:175). So pyprojectFile + requirementsFile installs only the requirements file, and pyprojectFile + requirements installs only the pyproject, with no log or error. The existing code warns for the requirements/requirementsFile overlap; the new option has no equivalent guard.

Suggested change
if (this.options.pyprojectFile) {
assert(
fs.existsSync(this.options.pyprojectFile),
`pyproject.toml not found: ${this.options.pyprojectFile}`
);
}
if (this.options.pyprojectFile) {
assert(
!this.options.requirementsFile,
"Cannot specify both pyprojectFile and requirementsFile"
);
assert(
!this.options.requirements,
"Cannot specify both pyprojectFile and requirements"
);
assert(
fs.existsSync(this.options.pyprojectFile),
`pyproject.toml not found: ${this.options.pyprojectFile}`
);
}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


}

async onBuildComplete(context: BuildContext, manifest: BuildManifest) {
Expand Down Expand Up @@ -88,6 +99,8 @@ class PythonExtension implements BuildExtension {
# Set up Python environment
RUN python3 -m venv /opt/venv
ENV PATH="/opt/venv/bin:$PATH"

${this.options.useUv ? "RUN pip install uv" : ""}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔍 uv target environment is not explicit

RUN pip install uv installs uv into /opt/venv and later layers call uv pip install ... with /opt/venv/bin on PATH but without VIRTUAL_ENV set and without --system. uv resolves its target environment from VIRTUAL_ENV, a .venv in the working directory, or the environment uv itself is installed into. The last case is what this relies on; if it does not apply in the builder image, every uv install step fails with "No virtual environment found". Setting ENV VIRTUAL_ENV=/opt/venv (or passing --python /opt/venv/bin/python) would make this deterministic and is worth verifying with a real deploy.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

`),
},
deploy: {
Expand Down Expand Up @@ -123,7 +136,36 @@ class PythonExtension implements BuildExtension {
# Copy the requirements file
COPY ${this.options.requirementsFile} .
# Install dependencies
RUN pip install --no-cache-dir -r ${this.options.requirementsFile}
${this.options.useUv
? `RUN uv pip install --no-cache -r ${this.options.requirementsFile}`
: `RUN pip install --no-cache-dir -r ${this.options.requirementsFile}`
}
`),
},
deploy: {
override: true,
},
});
} else if (this.options.pyprojectFile) {
// Copy pyproject file to the container
await addAdditionalFilesToBuild(
"pythonExtension",
{
files: [this.options.pyprojectFile],
},
context,
manifest
);

// Add a layer to the build that installs the dependencies
context.addLayer({
id: "python-dependencies",
image: {
instructions: splitAndCleanComments(`
# Copy the pyproject file
COPY ${this.options.pyprojectFile} .
# Install dependencies
${this.options.useUv ? "RUN uv pip install ." : "RUN pip install ."}
`),
Comment on lines +164 to 169

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Deploys using a Python project file can fail because only that file is copied into the image

Only the project definition file is copied before the project is installed (COPY/pip install . at packages/python/src/extension.ts:166-168), so the build backend cannot find the package sources or README it references.
Impact: Deployments configured with a Python project file can fail during image build instead of installing dependencies.

Mechanism

The layer copies just pyproject.toml into the working directory and then runs pip install . / uv pip install ., which builds the project as a distribution. Common backends (hatchling, poetry-core) error out when the declared package directory or readme file is absent (e.g. hatchling's "Unable to determine which files to ship"). If the intent is only to install the declared dependencies, a dependency-only install (e.g. uv pip install -r pyproject.toml, or uv sync/uv pip compile with a lock file) is safer, otherwise the referenced project sources must also be copied into the image.

Prompt for agents
The pyproject branch of PythonExtension.onBuildComplete copies only the pyproject.toml file into the image and then runs `pip install .` / `uv pip install .`. Building the project as a distribution requires the package sources (and any readme referenced in pyproject.toml), which are not copied, so most real projects will fail the image build. Decide whether the goal is installing only the declared dependencies (then use a dependency-only install path such as `uv pip install -r pyproject.toml`, or generate a requirements file), or installing the project itself (then also add the relevant source directories to the build context/COPY instructions). Consider also honouring uv.lock when present.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

},
deploy: {
Expand All @@ -144,7 +186,10 @@ class PythonExtension implements BuildExtension {
RUN echo "$REQUIREMENTS_CONTENT" > requirements.txt

# Install dependencies
RUN pip install --no-cache-dir -r requirements.txt
${this.options.useUv
? "RUN uv pip install --no-cache -r requirements.txt"
: "RUN pip install --no-cache-dir -r requirements.txt"
}
`),
},
deploy: {
Expand Down