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
Copy link
Contributor

Choose a reason for hiding this comment

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

🚩 test-dotenv.ts is an ad-hoc script, not a vitest test

This file is committed as a standalone executable script at the package root rather than as a proper vitest test file. The repo's CLAUDE.md states 'We use vitest exclusively' and AGENTS.md says 'Test files live beside the files under test and use descriptive describe and it blocks.' This file uses console.log assertions and process.exit instead of vitest's describe/it/expect. Consider converting this to a proper vitest test in a location next to packages/cli-v3/src/utilities/dotEnv.ts, or removing it if it was only intended as a temporary debugging aid.

Open in Devin Review

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

🟡 Temp directory not cleaned up when test fails

process.exit(1) on line 19 terminates the process before the cleanup call fs.rmSync(tempDir, ...) on line 22 executes, leaving the temp-env-test directory behind on disk whenever the assertion fails. The cleanup should happen before process.exit.

Suggested change
process.exit(1);
fs.rmSync(tempDir, { recursive: true, force: true });
process.exit(1);
Open in Devin Review

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

}

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

🚩 zodNamespace test uses as any casts and may not test real behavior

The test at packages/core/test/v3/zodNamespace.test.ts:34 uses as any to bypass type checking when calling ns.sender.send("SERVER_MSG", { bar: 1 } as any), and the second test at line 58 casts serverMessages as any. The comments acknowledge this: 'Cast for now until we update the types' and 'Currently this might throw or require passing { message: { bar: 1 } }'. These tests may not actually verify correct behavior — they may be aspirational tests for a future API shape rather than tests of current behavior. If so, they could be misleading or flaky.

Open in Devin Review

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


expect(emitSpy).toHaveBeenCalledWith("SERVER_MSG", {
payload: { bar: 1 },
version: "v1"
});
});

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;

/**
* [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
Contributor

Choose a reason for hiding this comment

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

🟡 Missing mutual exclusion validation for pyprojectFile with requirements/requirementsFile

The constructor at packages/python/src/extension.ts:46-48 validates that requirements and requirementsFile can't both be specified, but no equivalent assertion is added for pyprojectFile. If a user provides both pyprojectFile and requirements (or requirementsFile), one is silently ignored due to the if/else if chain in onBuildComplete (packages/python/src/extension.ts:114-198). For example, { pyprojectFile: "pyproject.toml", requirements: ["numpy"] } silently drops the requirements with no warning, which is inconsistent with the existing validation pattern that explicitly rejects conflicting options.

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.requirements || this.options.requirementsFile),
"Cannot specify pyprojectFile together with requirements or requirementsFile"
);
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" : ""}
`),
},
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 +162 to +168
Copy link
Contributor

Choose a reason for hiding this comment

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

🚩 pip install . with only pyproject.toml may fail at Docker build time

The pyprojectFile branch at packages/python/src/extension.ts:149-174 copies only the pyproject.toml into the container and runs pip install . (or uv pip install .). The pip install . command attempts to build and install the package in the current directory, which typically requires more than just a pyproject.toml — it needs the actual source code and a valid build system definition. If the user's pyproject.toml defines a package (with [build-system], name, etc.), the build step will fail because the source files aren't present. This would only work if the user also uses the scripts option to copy their source files, which is not documented or enforced. Worth verifying the intended usage pattern with the author — this could be a design-level issue rather than a simple code bug.

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