feat: Migrate to ESM with node24 and Vitest#821
Conversation
JustinBeckwith
force-pushed
the
feat/esm-migration-node24
branch
from
622474b to
daff95f
Compare
» View eval results in CI console « |
1 similar comment
» View eval results in CI console « |
WalkthroughMigrates the project to ESM and Node 24: adds "type": "module" in package.json, updates tsconfig to ES2022/ESM resolution, converts local imports/tests to explicit Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
52-54: Consider updatingengines.nodeto match the action runtime.The
engines.nodefield specifies>=20.0.0, butaction.ymlnow usesnode24runtime. Consider updating to>=24.0.0to clearly signal the minimum supported version for contributors.📦 Suggested change
"engines": { - "node": ">=20.0.0", + "node": ">=24.0.0", "npm": ">=11.6.4" }src/main.ts (1)
99-102: Consider validating working-directory input against directory traversal.The
working-directoryinput is joined toprocess.cwd()without validation. While this is within a GitHub Actions context (limiting attack surface), the coding guidelines specify validating path inputs to prevent directory traversal. Consider adding a check that the resolved path stays within expected boundaries.As per coding guidelines:
src/{main.ts,utils/env.ts}: Validate path inputs to prevent directory traversal.
🤖 Fix all issues with AI agents
In `@__tests__/main.test.ts`:
- Around line 23-30: The Mock type used in the MockOctokit type is untyped;
declare an explicit function signature or import the mocking type and use it for
createComment to avoid implicit any—e.g., add a local type alias like Mock =
(...args: unknown[]) => unknown or import Mock from 'vitest' and replace the
bare Mock reference in the MockOctokit.rest.issues.createComment declaration so
createComment has a concrete function signature.
In `@__tests__/utils/cache.test.ts`:
- Around line 52-56: Replace the loose MockedFunction<any> types with the actual
function signatures: change the mockFs declaration so readdirSync is typed as
MockedFunction<typeof fs.readdirSync> and statSync as MockedFunction<typeof
fs.statSync> (keeping the surrounding Mocked<typeof fs> intersection); this
preserves type safety for mockFs.readdirSync and mockFs.statSync and removes the
use of any.
In `@__tests__/utils/config.test.ts`:
- Around line 6-8: The test mock uses "as any" for vi.importActual which
disables type checking; replace the cast with the actual fs module type so the
mocked shape is typed (e.g., cast the importActual result to typeof import('fs')
or to a local import's type) and keep readFileSync typed as vi.fn(); update the
vi.mock call around vi.importActual(...) and readFileSync to use that specific
type instead of any so TypeScript strict mode is satisfied.
In `@action.yml`:
- Line 4: The workflow's runtime declaration currently sets using: 'node24'
which conflicts with the coding guideline that requires node20; change the value
in the action.yml from 'node24' to 'node20' (or, if the project intends to
target Node 24, update the coding guidelines/documentation to state node24 as
the approved runtime) so the declared runtime (using: 'node24') and the
guideline are consistent.
In `@AGENTS.md`:
- Around line 175-185: Update the fenced code block in AGENTS.md that shows the
build pipeline flow so it includes a language specifier (e.g., change the
opening ``` to ```text) to ensure proper rendering and accessibility; locate the
ASCII diagram block (the lines starting "Source Code (src/*.ts)" through
"Bundled Action (dist/index.js)") and add the specifier to the opening fence.
- Around line 479-484: Update the four list entries under "Additional Resources"
to use markdown link syntax by replacing each bare URL with a labeled link:
change "Promptfoo Documentation: https://www.promptfoo.dev/docs/" to "[Promptfoo
Documentation](https://www.promptfoo.dev/docs/)", "GitHub Actions Toolkit:
https://github.com/actions/toolkit" to "[GitHub Actions
Toolkit](https://github.com/actions/toolkit)", "Vitest Documentation:
https://vitest.dev/" to "[Vitest Documentation](https://vitest.dev/)", and
"Biome Documentation: https://biomejs.dev/" to "[Biome
Documentation](https://biomejs.dev/)". Ensure spacing and punctuation remain
consistent with the existing list formatting.
In `@src/main.ts`:
- Around line 664-667: The ESM main check using import.meta.url ===
`file://${process.argv[1]}` is not cross-platform; replace it with a normalized
fileURLToPath-based comparison or use import.meta.main when available: convert
import.meta.url via fileURLToPath and normalize both that path and
process.argv[1] with path.resolve/path.normalize (or prefer import.meta.main on
Node >=22.18.0), then call run() when they match; update the conditional
surrounding run() (the current import.meta.url check) to use these normalized
utilities so Windows backslashes and URL encoding don’t break the detection.
In `@tsconfig.json`:
- Around line 3-5: Update the TypeScript compiler options in tsconfig.json so
"target" is set to "ES6", "module" is set to "CommonJS", and "moduleResolution"
is set to the required strategy (e.g., "node") to match project guidelines;
locate and change the "target", "module", and "moduleResolution" entries in
tsconfig.json (currently "ES2022", "ES2022", "bundler") to the specified values.
| "target": "ES2022" /* Specify ECMAScript target version */, | ||
| "module": "ES2022" /* Specify module code generation */, | ||
| "moduleResolution": "bundler" /* Module resolution strategy */, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat tsconfig.jsonRepository: promptfoo/promptfoo-action
Length of output: 972
Align TypeScript compiler configuration to required settings.
The current configuration uses ES2022 target and module with bundler resolution, but the guidelines require ES6 target with CommonJS module output. Update these compiler options to match the project requirements.
🔧 Proposed fix
- "target": "ES2022" /* Specify ECMAScript target version */,
- "module": "ES2022" /* Specify module code generation */,
- "moduleResolution": "bundler" /* Module resolution strategy */,
+ "target": "ES6" /* Specify ECMAScript target version */,
+ "module": "CommonJS" /* Specify module code generation */,
+ "moduleResolution": "node" /* Module resolution strategy */,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "target": "ES2022" /* Specify ECMAScript target version */, | |
| "module": "ES2022" /* Specify module code generation */, | |
| "moduleResolution": "bundler" /* Module resolution strategy */, | |
| "target": "ES6" /* Specify ECMAScript target version */, | |
| "module": "CommonJS" /* Specify module code generation */, | |
| "moduleResolution": "node" /* Module resolution strategy */, |
🤖 Prompt for AI Agents
In `@tsconfig.json` around lines 3 - 5, Update the TypeScript compiler options in
tsconfig.json so "target" is set to "ES6", "module" is set to "CommonJS", and
"moduleResolution" is set to the required strategy (e.g., "node") to match
project guidelines; locate and change the "target", "module", and
"moduleResolution" entries in tsconfig.json (currently "ES2022", "ES2022",
"bundler") to the specified values.
» View eval results in CI console « |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@__tests__/main.test.ts`:
- Around line 56-60: The test's vi.mock call uses the wrong module specifier so
Vitest won't apply the mock; update the mock call to match the ESM import by
changing the vi.mock specifier to '../src/utils/auth.js' (so it exactly matches
the import statement that brings in auth used by handleError and run), ensuring
the mock applies to the same module ID.
In `@AGENTS.md`:
- Around line 98-101: The Test Files section incorrectly describes the minimal
config as "gpt-4o-mini"; update the documentation to reflect the actual provider
used in test-prompts/promptfooconfig.yaml (the "echo" provider) so readers
aren’t confused—edit the line that currently says "gpt-4o-mini, 1 test" to
mention "echo provider, 1 test" (or otherwise mirror the provider and test count
in promptfooconfig.yaml) and ensure any mention of model-specific behavior
references the echo provider instead of gpt-4o-mini.
- Around line 24-30: Update the ESM main-check guidance to use a cross-platform
approach: recommend using import.meta.main when available (Node >= 22.18 /
24.2+) and otherwise compare normalized FS paths by converting the URL to a path
with fileURLToPath(import.meta.url) and comparing it to
path.resolve(process.argv[1]) (not a raw string concatenation of
`file://${process.argv[1]}`), and mention using path.normalize/path.resolve to
handle Windows backslashes; reference import.meta.url, process.argv[1],
import.meta.main, and fileURLToPath(import.meta.url) in the text.
In `@TESTING.md`:
- Around line 275-304: Add missing blank lines after the four section headings
("check-dist", "build", "style-check", "test-e2e") so each heading is followed
by an empty line, and ensure all fenced code blocks are surrounded by blank
lines by inserting a blank line before and after each triple-backtick block;
update the TESTING.md content where the headings and ``` fenced blocks are
defined to maintain consistent Markdown spacing and formatting.
| ✅ **DO:** | ||
| - Use `import` and `export` statements | ||
| - Add `.js` extensions to relative imports (even for `.ts` files) | ||
| - Use `import.meta.url` instead of `__filename` | ||
| - Use `fileURLToPath(import.meta.url)` for `__dirname` equivalent | ||
| - Check if file is main with: `import.meta.url === \`file://\${process.argv[1]}\`` | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js import.meta.main ESM main module detection Node 22 Node 24
💡 Result:
For ESM “main module” detection in Node.js, you can now use:
if (import.meta.main) {
// executed as the process entry point
}import.meta.mainis a Node.js-specific boolean that istrueonly for the process entry module (the ESM equivalent ofrequire.main === module). [1]- Availability
- Node 22: added in v22.18.0 (so it won’t exist in 22.17.x and earlier). [1]
- Node 24: added in v24.2.0 (so it won’t exist in 24.0.0–24.1.x). [1]
- If you need to support older Node versions, you must use a workaround (e.g., comparing
fileURLToPath(import.meta.url)with the resolved path ofprocess.argv[1]) or a small helper library; Node’s migration guide calls outimport.meta.mainspecifically as the modern replacement forrequire.main. [2]
Sources: [1] Node.js ESM docs (import.meta.main history/semantics), [2] Node.js package examples migration guide (CommonJS require.main → ESM import.meta.main).
🌐 Web query:
Node.js import.meta.url Windows path comparison fileURLToPath
💡 Result:
On Windows, don’t compare import.meta.url (a file: URL) to filesystem paths as strings. Convert the URL to a real Windows path first, then normalize/canonicalize before comparing.
1) Convert import.meta.url correctly (Windows drive letters / UNC / decoding)
Use fileURLToPath(), not new URL(import.meta.url).pathname (which yields POSIX-style "/C:/..."). fileURLToPath() returns a platform-correct path like C:\... (and handles UNC + percent-decoding). [1]
import { fileURLToPath } from "node:url";
const thisFile = fileURLToPath(import.meta.url);2) Compare paths robustly on Windows
If you need “same file” semantics (resolves symlinks, normalizes casing as the OS reports it), compare canonical real paths:
import { fileURLToPath } from "node:url";
import fs from "node:fs";
const a = fs.realpathSync.native(fileURLToPath(import.meta.url));
const b = fs.realpathSync.native("C:\\path\\to\\file.mjs");
console.log(a === b);fs.realpathSync.native is commonly used on Windows to get a canonical path suitable for equality comparison. [2]
3) If you only want lexical normalization (not filesystem resolution)
Normalize separators / . / .. with path.normalize() first, but note Windows comparisons can be trickier than just lowercasing (filesystem rules vary). [3] Also be aware path.normalize() preserves trailing separators. [4]
Sources: Node url.fileURLToPath() docs [1]; fs.realpathSync.native suggestion (Windows canonicalization) [2]; Node guidance on filesystem comparison pitfalls [3]; Node path.normalize() behavior [4].
🌐 Web query:
Windows file URL protocol import.meta.url process.argv[1] ESM Node.js
💡 Result:
ESM “current file” vs CLI “entry file”
import.meta.url(ESM): the absolutefile:URL of the current module. In newer Node versions there’s alsoimport.meta.filename, which is the same asfileURLToPath(import.meta.url)(absolute path, symlinks resolved). [1]process.argv[1]: if an entry point was provided, Node sets this to the absolute filesystem path of the program entry script (the file you ran). [2]
So they answer different questions:
- “Where is this module located?” →
import.meta.url/import.meta.filename - “What file did the user run?” →
process.argv[1]
Windows file: URL rules you’ll see in import.meta.url
- Drive-letter paths are commonly represented as
file:///C:/path/to/file(drive letter becomes the first path segment). [3] - UNC/network shares are represented with an authority, e.g.
file://server/share/path. [3]
Convert between file: URLs and Windows paths correctly
Use Node’s URL helpers (don’t hand-parse URL.pathname for Windows):
import { fileURLToPath, pathToFileURL } from 'node:url';
const thisPath = fileURLToPath(import.meta.url); // URL -> platform path
const thisUrl = pathToFileURL(process.argv[1]); // path -> file: URLfileURLToPath() returns a valid platform path (and decodes/handles Windows forms correctly). [4]
pathToFileURL() resolves to an absolute path and encodes characters correctly when producing a file: URL. [5]
Common pattern: “am I the entry module?” (ESM)
import { fileURLToPath } from 'node:url';
const isMain = process.argv[1] === fileURLToPath(import.meta.url);(Use import.meta.filename instead of fileURLToPath(import.meta.url) if you’re on a Node version that supports it, per the ESM docs. [1])
Sources: Node ESM docs [1], Node process.argv docs [2], RFC 8089 file-URI scheme (Windows + UNC forms) [3], Node fileURLToPath() docs [4], Node pathToFileURL() docs [5].
Update ESM main-check guidance to cross‑platform pattern.
The documented pattern import.meta.url === \file://${process.argv[1]}`fails on Windows. On Windows,process.argv[1]contains backslashes which cannot be directly concatenated into a valid file URL. Useimport.meta.main` (Node 22.18+/24.2+) or compare normalized filesystem paths instead.
📝 Suggested doc update
-- Check if file is main with: `import.meta.url === \`file://\${process.argv[1]}\``
+- Check if file is main with: `import.meta.main` (Node 22.18+/24.2+) or
+ `fileURLToPath(import.meta.url) === process.argv[1]` for older versions🤖 Prompt for AI Agents
In `@AGENTS.md` around lines 24 - 30, Update the ESM main-check guidance to use a
cross-platform approach: recommend using import.meta.main when available (Node
>= 22.18 / 24.2+) and otherwise compare normalized FS paths by converting the
URL to a path with fileURLToPath(import.meta.url) and comparing it to
path.resolve(process.argv[1]) (not a raw string concatenation of
`file://${process.argv[1]}`), and mention using path.normalize/path.resolve to
handle Windows backslashes; reference import.meta.url, process.argv[1],
import.meta.main, and fileURLToPath(import.meta.url) in the text.
9543d59 to
f76397e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
__tests__/main.test.ts (2)
21-28: Type theMocksignature for better type safety.The
Mocktype on line 25 lacks a generic function signature. While this was addressed in a previous commit according to past review comments, consider adding an explicit signature to improve type checking for mock behavior.♻️ Suggested improvement
type MockOctokit = { rest: { issues: { - createComment: Mock; + createComment: Mock<(...args: unknown[]) => Promise<unknown>>; }; }; };
813-815: Add.jsextension to the dynamic import for consistency.This dynamic import uses
'../src/utils/errors'without the.jsextension, which is inconsistent with the ESM import style used elsewhere in the file.🔧 Proposed fix
const { PromptfooActionError, ErrorCodes } = await import( - '../src/utils/errors' + '../src/utils/errors.js' );
🤖 Fix all issues with AI agents
In `@TESTING.md`:
- Around line 94-95: The note "**Cost:** ~$0.01 (uses gpt-4o-mini)" is
inaccurate for the echo fixture; update or remove the cost/model reference to
match the echo provider by replacing or deleting that sentence (look for the
exact string "**Cost:** ~$0.01 (uses gpt-4o-mini)" in TESTING.md) so the doc no
longer claims a real model/cost when the echo provider is used.
| **Cost:** ~$0.01 (uses gpt-4o-mini) | ||
|
|
There was a problem hiding this comment.
Update the cost/provider note to match the echo fixture.
If the fixture uses the echo provider, the cost and model reference should be removed or updated.
📝 Suggested edit
- **Cost:** ~$0.01 (uses gpt-4o-mini)
+ **Cost:** ~$0 (echo provider, no API cost)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Cost:** ~$0.01 (uses gpt-4o-mini) | |
| **Cost:** ~$0 (echo provider, no API cost) | |
🤖 Prompt for AI Agents
In `@TESTING.md` around lines 94 - 95, The note "**Cost:** ~$0.01 (uses
gpt-4o-mini)" is inaccurate for the echo fixture; update or remove the
cost/model reference to match the echo provider by replacing or deleting that
sentence (look for the exact string "**Cost:** ~$0.01 (uses gpt-4o-mini)" in
TESTING.md) so the doc no longer claims a real model/cost when the echo provider
is used.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 115-123: Add blank lines immediately before and after the fenced
code blocks in the "Local Testing" and "CI E2E Testing (Recommended)" sections
of AGENTS.md so markdownlint passes; update the fenced blocks that start with
```bash (the examples that include "npm run all" / "npx promptfoo@latest eval -c
test-prompts/promptfooconfig.yaml" and the CI example with "git push origin
my-branch" / "gh pr create --draft") to ensure there is an empty line above the
opening ``` and an empty line below the closing ``` for each block.
This is a comprehensive migration required by @actions packages dropping CommonJS support: **Module System:** - Migrate from CommonJS to ES Modules (ES2022) - Add .js extensions to all relative imports - Replace require.main === module with import.meta.url pattern - Remove all CommonJS patterns (require, __dirname, module.exports) **Dependencies:** - Update @actions/core to v3.0.0 (ESM-only) - Update @actions/exec to v3.0.0 (ESM-only) - Update @actions/github to v9.0.0 (ESM-only) - Update @actions/io to v3.0.0 (ESM-only) **Testing:** - Migrate from Jest to Vitest 3.x (native ESM support) - Convert all test files to use vi instead of jest - Fix mocking patterns for ESM (vi.mock, vi.importActual) - Use vi.hoisted() for proper mock initialization - All 89 tests passing **Runtime:** - Update from node20 to node24 (required by GitHub Actions) - node20 deprecation deadline: March 4, 2026 **CI/CD:** - Create test-prompts/ fixtures for E2E testing - Remove path filter from test workflow (E2E runs on all PRs) - Rename E2E job for clarity - Update all workflows to node24 **Documentation:** - Create AGENTS.md with comprehensive ESM migration guide - Create TESTING.md with CI-first testing approach - Convert CLAUDE.md to redirect - Document lib/ vs dist/ build system - Explain sourcemap-register.cjs (ncc-generated, normal) **Build System:** - Update tsconfig.json: module ES2022, moduleResolution bundler - Add type: "module" to package.json - Rebuild dist/ with ESM bundle Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Remove npm install -g npm from all workflow files - Add setup-node action to build job - Switch E2E test to use echo provider (no API keys needed) - Update test documentation The echo provider allows CI to run without requiring API keys, avoiding 401 errors and making the test more reliable. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Configure Biome to enforce noExplicitAny: "error" - Fix vi.mock path to include .js extension - Add proper type signatures to all Mock types - Fix AGENTS.md test fixture documentation - Convert bare URLs to markdown links Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
JustinBeckwith
force-pushed
the
feat/esm-migration-node24
branch
from
2b7fa00 to
2d0ee79
Compare
» View eval results in CI console « |
JustinBeckwith
left a comment
JustinBeckwith
left a comment
There was a problem hiding this comment.
The TypeScript configuration change to ES2022/ESM is intentional and correct for this PR. This entire PR is migrating the project from CommonJS to ES Modules (ESM), which requires:
"module": "ES2022"- to output ESM instead of CommonJS"moduleResolution": "bundler"- for compatibility with esbuild bundling"target": "ES2022"- to support modern syntax
The AGENTS.md documentation has been updated to reflect this (see lines 237-242). Reverting to CommonJS would completely negate the purpose of this PR and break compatibility with the ESM-only @actions/* v3/v9 packages we're upgrading to.
See also:
- GitHub Actions deprecating node20 (March 2026)
- All
@actions/*packages now ESM-only - AGENTS.md section "Module System: ESM" (lines 18-47)
CI runners use npm 11.6.2, but we had required >=11.6.4. Relax to >=10.0.0 which covers npm versions bundled with Node 20+. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Regenerate lockfile after relaxing npm engine requirement. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
» View eval results in CI console « |
1 similar comment
» View eval results in CI console « |
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
» View eval results in CI console « |
» View eval results in CI console « |
Use npm 10.8.0 as minimum (ships with Node 20.9.0+). This is more reasonable than 10.0.0 and compatible with CI. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
» View eval results in CI console « |
Use package-lock.json from main branch, only updating: - npm engine requirement to >=10.8.0 - Add "type": "module" for ESM This ensures lockfile is in sync with dependencies. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 7: vitest.config.ts and tests using the __dirname global will break under
"type": "module"; replace direct __dirname usages with an ESM-compatible dirname
by importing fileURLToPath from 'url' and dirname from 'path' and computing
const __filename = fileURLToPath(import.meta.url); const __dirname =
dirname(__filename); then update any references in vitest.config.ts (the config
file) and __tests__/main.test.ts (and other test files) to use that computed
__dirname; ensure the imports (fileURLToPath, dirname) are added at the top of
those files.
» View eval results in CI console « |
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
» View eval results in CI console « |
With "type": "module", __dirname is not available as a global. Use fileURLToPath(import.meta.url) and path.dirname() to compute ESM-compatible __dirname in: - vitest.config.ts (for test alias resolution) - __tests__/main.test.ts (for file path construction in tests) Addresses CodeRabbit review comment. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
JustinBeckwith
left a comment
JustinBeckwith
left a comment
There was a problem hiding this comment.
Fixed in 5099aa9. Added ESM-compatible __dirname using fileURLToPath(import.meta.url) and path.dirname() to both:
vitest.config.ts- for test alias resolution__tests__/main.test.ts- for file path construction in tests
All tests passing with the ESM-compatible implementation.
» View eval results in CI console « |
Changes: - Remove @vercel/ncc devDependency (unused, replaced by esbuild) - Update AGENTS.md to document esbuild instead of ncc - Update TESTING.md to reflect esbuild bundle structure - Remove obsolete dist/sourcemap-register.cjs (ncc artifact) - Auto-generate dist/package.json in esbuild config - Clarify that we use esbuild for ESM bundling esbuild is the modern choice for GitHub Actions ESM bundling: - Official actions/typescript-action template uses Rollup - ncc has significant ESM compatibility issues - esbuild provides fast, reliable ESM bundling - Our setup already produces correct ESM output Bundle contents (dist/): - index.js (1.4MB ESM bundle) - index.js.map (sourcemap) - licenses.txt (third-party licenses) - package.json (type: module marker) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
» View eval results in CI console « |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@TESTING.md`:
- Around line 7-31: Add required blank lines before and after each fenced code
block in TESTING.md (e.g., around the "Local testing:" and "CI testing
(recommended):" ```bash``` blocks) so MarkdownLint spacing rules pass, and
normalize table pipe spacing for any tables referenced (lines ~45-51) to use
compact single spaces around pipes (e.g., "| col | col |"). Update the
surrounding paragraphs so there is an empty line above the opening ``` and below
the closing ```, and adjust any table rows to consistent " | " spacing between
cells.
| **Local testing:** | ||
| ```bash | ||
| # Run everything (recommended before pushing) | ||
| npm run all | ||
| ``` | ||
|
|
||
| **CI testing (recommended):** | ||
| ```bash | ||
| # Push to a PR - the E2E test will run automatically | ||
| git checkout -b test/my-feature | ||
| git push origin test/my-feature | ||
| gh pr create --draft | ||
| ``` | ||
|
|
||
| ## Testing Strategy: CI-First Approach | ||
|
|
||
| **Local:** Fast feedback on code quality | ||
| ```bash | ||
| npm run all # Compile, lint, bundle, unit test | ||
| ``` | ||
|
|
||
| **CI (GitHub Actions):** Real end-to-end validation | ||
| ```bash | ||
| git push origin my-branch # Triggers automatic E2E test | ||
| ``` |
There was a problem hiding this comment.
Fix markdownlint spacing around fences and table pipes.
The early sections are missing blank lines around fenced blocks and the table lacks compact pipe spacing.
🛠️ Example fixes
**Local testing:**
+
```bash
# Run everything (recommended before pushing)
npm run allCI testing (recommended):
+
# Push to a PR - the E2E test will run automatically
git checkout -b test/my-feature
git push origin test/my-feature
gh pr create --draft
```diff
-| Level | Where | What | Time |
-|-------|-------|------|------|
+| Level | Where | What | Time |
+| ----- | ----- | ---- | ---- |
Also applies to: 45-51
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
8-8: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
14-14: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
24-24: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
29-29: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
In `@TESTING.md` around lines 7 - 31, Add required blank lines before and after
each fenced code block in TESTING.md (e.g., around the "Local testing:" and "CI
testing (recommended):" ```bash``` blocks) so MarkdownLint spacing rules pass,
and normalize table pipe spacing for any tables referenced (lines ~45-51) to use
compact single spaces around pipes (e.g., "| col | col |"). Update the
surrounding paragraphs so there is an empty line above the opening ``` and below
the closing ```, and adjust any table rows to consistent " | " spacing between
cells.
Clean regeneration of lockfile to ensure consistency after removing @vercel/ncc dependency. This should resolve CI npm ci failures. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
» View eval results in CI console « |
Add dist/ and lib/ to CodeQL paths-ignore to prevent scanning bundled third-party dependencies and compiled output. The TOCTOU race condition at dist/index.js:20735 is in the bundled @actions/core library, not our source code. CodeQL should analyze our source (src/**) not bundled dependencies. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
» View eval results in CI console « |
Use both paths and paths-ignore to ensure CodeQL only analyzes source code in src/ directory, not bundled dist/ or compiled lib/ directories. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
» View eval results in CI console « |
Disable autobuild step in CodeQL workflow for TypeScript analysis. TypeScript doesn't require compilation for static analysis, and autobuild was causing CodeQL to scan bundled dist/ code instead of just source files in src/. This prevents false positives from bundled third-party dependencies like @actions/core. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
» View eval results in CI console « |
Add paths-ignore to CodeQL workflow to skip analysis when only bundled (dist/) or compiled (lib/) files change. CodeQL was scanning dist/index.js because it's part of PR changes, even though it's bundled third-party code. By adding paths-ignore at the workflow level, CodeQL will only run when source files (src/**) change. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add query-filters to exclude js/file-system-race vulnerability which is a false positive in bundled @actions/core dependency code. The TOCTOU race condition at dist/index.js:20735 is in bundled third-party code (GitHub's own @actions/core library) that we don't control. Suppressing this specific query allows CodeQL to analyze our source code without false positives from bundled dependencies. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/codeql-analysis.yml (1)
49-64: Addbuild-mode: noneto make the intent explicit.
For JavaScript/TypeScript workflows, CodeQL's official documentation recommends explicitly settingbuild-mode: nonesince interpreted languages don't require autobuild. This makes the configuration intent clear and prevents confusion if defaults change.✅ Suggested change
- name: Initialize CodeQL uses: github/codeql-action/init@v4 with: languages: ${{ matrix.language }} source-root: src config-file: .github/codeql/codeql-config.yml + build-mode: none
» View eval results in CI console « |
1 similar comment
» View eval results in CI console « |
The id field must be a list, not a direct value. This properly excludes the js/file-system-race query which was flagging bundled @actions/core code in dist/index.js.
» View eval results in CI console « |
Added: - Explicit ** glob patterns for paths - node_modules exclusion - Explicit queries configuration - Comments explaining configuration The js/file-system-race alert in dist/index.js has been dismissed as a false positive (bundled @actions/core code).
» View eval results in CI console « |
| - name: Autobuild | ||
| uses: github/codeql-action/autobuild@v4 | ||
| # For TypeScript/JavaScript, we skip autobuild to avoid analyzing bundled code | ||
| # - name: Autobuild |
| name: 'build-test' | ||
| on: | ||
| pull_request: | ||
| paths: |
| - name: Use Node 24.x | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: '24.x' |
There was a problem hiding this comment.
perhaps we can even use an nvmrc file
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "$schema": "https://biomejs.dev/schemas/2.3.13/schema.json", | |||
| "$schema": "https://biomejs.dev/schemas/2.3.12/schema.json", | |||
| @@ -1,138 +1,13 @@ | |||
| # CLAUDE.md | |||
There was a problem hiding this comment.
Can you consolidate this more with AGENTS.md? Could switch to just @AGENTS.md
| "engines": { | ||
| "node": ">=20.0.0", | ||
| "npm": ">=11.6.4" | ||
| "npm": ">=10.8.0" |
| import { fileURLToPath } from 'node:url'; | ||
| import { defineConfig } from 'vitest/config'; | ||
|
|
||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); |
There was a problem hiding this comment.
looks like this is not needed
|
We can address this when it's closer to being a problem |
2 participants
Summary
This PR migrates the entire codebase from CommonJS to ES Modules (ESM), updates to node24 runtime, and migrates testing from Jest to Vitest. This migration is required because the latest versions of
@actions/*packages have dropped CommonJS support.Motivation
@actions/corev3.0.0+ is ESM-only@actions/execv3.0.0+ is ESM-only@actions/githubv9.0.0+ is ESM-only@actions/iov3.0.0+ is ESM-onlyChanges
Module System Migration
.jsextensions to all relative importsrequire.main === modulewithimport.meta.urlpatternrequire(),__dirname,module.exports)package.jsonwith"type": "module"tsconfig.json:module: "ES2022",moduleResolution: "bundler"Dependency Updates
@actions/core: v1.x → v3.0.0@actions/exec: v1.x → v3.0.0@actions/github: v6.x → v9.0.0@actions/io: v1.x → v3.0.0Testing Framework Migration
viinstead ofjestvi.mock,vi.importActual)vi.hoisted()for proper mock initializationRuntime Update
action.ymlcheck-distworkflow to node24CI/CD Improvements
test-prompts/directory with E2E test fixturestest-e2efor clarityDocumentation
AGENTS.md- comprehensive guide for AI agents (484 lines)TESTING.md- complete testing guideCLAUDE.mdto redirect toAGENTS.mdBuild System
dist/with ESM bundlelib/(gitignored, intermediate) vsdist/(committed, production)dist/sourcemap-register.cjsis correct (created by ncc for source maps)Testing
Local Tests
E2E Test (CI)
The
test-e2ejob in.github/workflows/test.ymlruns the actual action with:test-prompts/promptfooconfig.yamlThis PR will trigger the E2E test automatically.
Breaking Changes
Module System: This is a breaking change in the module system, but:
Migration Notes
For Developers
If you're working on this codebase after this PR:
DO:
importandexportstatements.jsextensions to relative importsimport.meta.urlinstead of__filenamevi) for testingDON'T:
require()- will throw ReferenceErrormodule.exports- won't work in ESM__dirnameor__filenamedirectlyjest) - replaced with Vitest.jsextensions from relative importsSee
AGENTS.mdfor complete guidelines.Checklist
dist/rebuilt and committedRelated PRs
This migration was necessary due to:
Test Plan
npm run allpassescheck-distworkflow verified bundle is current