fix(build): add destination option to additionalFiles extension #2855
Conversation
When using glob patterns with parent directory references (../), the
default behavior strips ".." segments resulting in unexpected paths.
For example, "../shared/**" would place files at "shared/" instead of
preserving the original path structure.
This adds an optional "destination" parameter that allows users to
explicitly specify where matched files should be placed:
additionalFiles({
files: ["../shared/**"],
destination: "apps/shared"
})
When destination is specified, files are placed relative to the glob
pattern's base directory under the destination path. This is useful
in monorepo setups where files need to maintain their structure.
Also updates documentation to explain this behavior and the new option.
Slack thread: https://triggerdotdev.slack.com/archives/C08N6PJTK2Q/p1767946469914139?thread_ts=1756405171.439939&cid=C08N6PJTK2Q
🦋 Changeset detectedLatest commit: 674f5c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 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 |
WalkthroughAdds an optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-11-27T16:27:35.304ZApplied to files:
📚 Learning: 2025-11-27T16:27:35.304ZApplied to files:
📚 Learning: 2025-11-27T16:27:35.304ZApplied to files:
📚 Learning: 2025-11-27T16:26:58.661ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
🔇 Additional comments (1)
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 |
PR Review: fix(build): add destination option to additionalFiles extensionSummaryThis PR adds a Code Quality & Best Practices ✅
Potential Issues
|
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/build/src/internal/additionalFiles.ts (1)
76-135: Fix Windows path separator bug and add destination path safety validation.Three security/correctness issues:
Windows separator mismatch (critical):
relative(cwd, file)returns platform-specific separators (backslashes on Windows), but code splits byposix.sep(/). This means the..filtering on line 125 silently fails on Windows—a malicious glob pattern could escape via../segments.Absolute
destinationbypass: Ifoptions.destinationis absolute,join(destinationPath, pathInsideDestinationDir)ignoresdestinationPathentirely, bypassing output directory bounds.No final safety guard: No validation ensures the computed
destinationactually stays withindestinationPathbeforecopyFile()executes.Proposed fix
-import { dirname, isAbsolute, join, posix, relative, resolve } from "node:path"; +import { dirname, isAbsolute, join, relative, resolve, sep } from "node:path"; function getGlobBase(pattern: string): string { const parts = pattern.split(/[/\\]/); const baseParts: string[] = []; for (const part of parts) { if (part.includes("*") || part.includes("?") || part.includes("[") || part.includes("{")) { break; } baseParts.push(part); } - return baseParts.length > 0 ? baseParts.join(posix.sep) : "."; + return baseParts.length > 0 ? baseParts.join(sep) : "."; } async function findStaticAssetsForMatcher( matcher: string, destinationPath: string, options?: { cwd?: string; ignore?: string[]; destination?: string } ): Promise<MatchedStaticAssets> { const result: MatchedStaticAssets = []; const files = await glob({ patterns: [matcher], cwd: options?.cwd, ignore: options?.ignore ?? [], onlyFiles: true, absolute: true, }); const cwd = options?.cwd ?? process.cwd(); + const validateRelativePath = (path: string): void => { + if (isAbsolute(path) || path.split(sep).includes("..")) { + throw new Error(`[additionalFiles] Path traversal detected: ${path}`); + } + }; + + const resolveDestinationSafely = (root: string, rel: string): string => { + const resolved = resolve(root, rel); + if (!resolved.startsWith(root + sep) && resolved !== root) { + throw new Error( + `[additionalFiles] Refusing to write outside outputPath: ${resolved} is not under ${root}` + ); + } + return resolved; + }; for (const file of files) { let pathInsideDestinationDir: string; if (options?.destination) { + if (isAbsolute(options.destination)) { + throw new Error(`[additionalFiles] destination must be a relative path, got: ${options.destination}`); + } const globBase = getGlobBase(matcher); const absoluteGlobBase = isAbsolute(globBase) ? globBase : resolve(cwd, globBase); const relativeToGlobBase = relative(absoluteGlobBase, file); + validateRelativePath(relativeToGlobBase); pathInsideDestinationDir = join(options.destination, relativeToGlobBase); } else { pathInsideDestinationDir = relative(cwd, file) - .split(posix.sep) + .split(sep) .filter((p) => p !== "..") - .join(posix.sep); + .join(sep); + validateRelativePath(pathInsideDestinationDir); } - const relativeDestinationPath = join(destinationPath, pathInsideDestinationDir); + const relativeDestinationPath = resolveDestinationSafely(destinationPath, pathInsideDestinationDir); result.push({ source: file, destination: relativeDestinationPath, }); } return result; }
🧹 Nitpick comments (3)
packages/build/src/extensions/core/additionalFiles.ts (1)
4-25: Document (and/or validate) thatdestinationmust be a safe relative path.Given the internal code ultimately does
join(manifest.outputPath, destination, ...), allowing absolute paths or..segments indestinationcan escape the build directory unless explicitly prevented. Consider adding a one-liner to the JSDoc like “Must be a relative path and must not contain..”.docs/config/extensions/additionalFiles.mdx (1)
27-83: Add a short note:destinationis relative (no absolute paths / no..).This will prevent readers from assuming they can set
/tmp/fooor../../somethingand also aligns with the safest implementation.packages/build/src/internal/additionalFiles.ts (1)
30-50: Minor:options.files ?? []is unnecessary (and can hide invalid inputs).
AdditionalFilesOptions.filesis required in the public type; if this can beundefinedin practice, consider making it optional at the type level and validating early. Otherwise, dropping?? []makes misconfiguration fail fast.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/config/extensions/additionalFiles.mdxpackages/build/src/extensions/core/additionalFiles.tspackages/build/src/internal/additionalFiles.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
packages/build/src/internal/additionalFiles.tspackages/build/src/extensions/core/additionalFiles.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
packages/build/src/internal/additionalFiles.tspackages/build/src/extensions/core/additionalFiles.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
packages/build/src/internal/additionalFiles.tspackages/build/src/extensions/core/additionalFiles.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
packages/build/src/internal/additionalFiles.tspackages/build/src/extensions/core/additionalFiles.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Use build extensions in trigger.config.ts (additionalFiles, additionalPackages, aptGet, prismaExtension, etc.) to customize the build
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/repo.mdc:0-0
Timestamp: 2025-11-27T16:26:47.602Z
Learning: Refer to the monorepo structure documentation at repo.md before making changes or adding new files
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Use build extensions in trigger.config.ts (additionalFiles, additionalPackages, aptGet, prismaExtension, etc.) to customize the build
Applied to files:
packages/build/src/internal/additionalFiles.tsdocs/config/extensions/additionalFiles.mdxpackages/build/src/extensions/core/additionalFiles.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure build process in trigger.config.ts using `build` object with external packages, extensions, and JSX settings
Applied to files:
packages/build/src/internal/additionalFiles.tsdocs/config/extensions/additionalFiles.mdx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Specify task locations in trigger.config.ts using the `dirs` array, with automatic exclusion of .test and .spec files
Applied to files:
packages/build/src/internal/additionalFiles.tsdocs/config/extensions/additionalFiles.mdxpackages/build/src/extensions/core/additionalFiles.ts
📚 Learning: 2025-11-27T16:26:47.602Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/repo.mdc:0-0
Timestamp: 2025-11-27T16:26:47.602Z
Learning: Refer to the monorepo structure documentation at repo.md before making changes or adding new files
Applied to files:
docs/config/extensions/additionalFiles.mdx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `metadata.parent` and `metadata.root` to update parent and root task metadata from child tasks
Applied to files:
docs/config/extensions/additionalFiles.mdx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure Trigger.dev project in `trigger.config.ts` using `defineConfig()` with project ref and task directories
Applied to files:
docs/config/extensions/additionalFiles.mdx
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : When importing from `trigger.dev/core` in the webapp, use subpath exports from the package.json instead of importing from the root path
Applied to files:
docs/config/extensions/additionalFiles.mdx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `trigger.dev/sdk/v3` for all imports in Trigger.dev tasks
Applied to files:
docs/config/extensions/additionalFiles.mdx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Export tasks with unique IDs within the project to enable proper task discovery and execution
Applied to files:
docs/config/extensions/additionalFiles.mdx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Export every task, including subtasks
Applied to files:
docs/config/extensions/additionalFiles.mdx
🧬 Code graph analysis (1)
packages/build/src/internal/additionalFiles.ts (1)
packages/build/src/extensions/core/additionalFiles.ts (1)
AdditionalFilesOptions(4-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
- GitHub Check: Cursor Bugbot
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: claude-review
- GitHub Check: Analyze (javascript-typescript)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@claude add a patch changeset file please |
|
Claude finished @ericallam's task in 39s —— View job Adding Changeset
Added changeset file |
PR Review: fix(build): add destination option to additionalFiles extensionSummaryThis PR adds a Code Quality and Best PracticesPositives:
Suggestions:
Potential Bugs or Issues
Performance Considerations
Security Concerns
Test CoverageConcern: There don't appear to be any unit tests for the
Example test cases for expect(getGlobBase("../shared/**")).toBe("../shared");
expect(getGlobBase("./assets/*.txt")).toBe("./assets");
expect(getGlobBase("files/**/*.js")).toBe("files");
expect(getGlobBase("**/*.js")).toBe(".");
expect(getGlobBase("exact-file.txt")).toBe("exact-file.txt");Overall AssessmentThis is a well-implemented feature that solves a real pain point for monorepo users. The documentation is thorough and the implementation is straightforward. Recommended before merge:
Nice to have:
Good work on this enhancement! 👍 |
… option When a pattern has no glob characters (e.g., './config/settings.json'), return the parent directory instead of the full path. This ensures that relative() preserves the filename when computing the destination path. Co-authored-by: Eric Allam <[email protected]>
claude
bot
force-pushed
the
claude/slack-fix-additionalfiles-path-resolution-kDZvR
branch
from
b30a65c to
55aba75
Compare
Code Review: fix(build): add destination option to additionalFiles extensionThanks for this PR! The feature to add a Overall AssessmentThe PR provides a clean solution to a real problem - when using glob patterns with parent directory references ( Code Quality and Best PracticesStrengths:
Suggestions:
Potential Bugs or Issues
Performance ConsiderationsThe implementation looks efficient:
Security Concerns
Test CoverageThis is the main concern: I could not find any unit tests for the new functionality. Consider adding tests for:
Example test cases for expect(getGlobBase("../shared/**")).toBe("../shared");
expect(getGlobBase("./assets/*.ts")).toBe("./assets");
expect(getGlobBase("../shared/specific-file.ts")).toBe("../shared");
expect(getGlobBase("**")).toBe(".");
expect(getGlobBase("file.ts")).toBe(".");Minor Suggestions
SummaryThis is a solid PR that addresses a real user pain point. The code is clean and well-documented. The main recommendation is to add unit tests to ensure the Once tests are added, this looks good to merge! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/build/src/internal/additionalFiles.ts (1)
30-51: Add tests for the destination option and getGlobBase helper.The
destinationoption andgetGlobBasehelper currently have no test coverage. Add unit tests to verify:
- Destination path resolution works correctly for various glob patterns
- Monorepo scenarios with parent directory references like
../are handled properly- Edge cases like single-file patterns, absolute paths, and complex globs are tested
- The glob base extraction logic handles all pattern types
Use vitest for tests in the packages/build package.
🤖 Fix all issues with AI agents
In @packages/build/src/internal/additionalFiles.ts:
- Around line 76-100: getGlobBase incorrectly returns a filename for single-part
non-glob patterns like "file.txt"; update the non-glob branch in getGlobBase so
that if (!hasGlobCharacters) you return "." when baseParts.length <= 1 (covering
zero- or single-part patterns) and only pop the last part when baseParts.length
> 1, then join with posix.sep as before; reference function name getGlobBase and
variable baseParts to locate the change.
- Around line 131-136: The fallback handling for computing
pathInsideDestinationDir uses relative(cwd, file) then splits on posix.sep,
which fails on Windows because path.relative returns backslashes; update the
logic in the block that sets pathInsideDestinationDir to either split on
path.sep (so split(relative(cwd, file), path.sep) then filter out "..") or
replace the call to relative with posix.relative(cwd, file) so the returned
string uses "/" consistently before splitting by posix.sep; ensure you reference
and update the code around the pathInsideDestinationDir assignment and the use
of relative/cwd/file accordingly.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/build/src/internal/additionalFiles.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
packages/build/src/internal/additionalFiles.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
packages/build/src/internal/additionalFiles.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
packages/build/src/internal/additionalFiles.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
packages/build/src/internal/additionalFiles.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Use build extensions in trigger.config.ts (additionalFiles, additionalPackages, aptGet, prismaExtension, etc.) to customize the build
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Use build extensions in trigger.config.ts (additionalFiles, additionalPackages, aptGet, prismaExtension, etc.) to customize the build
Applied to files:
packages/build/src/internal/additionalFiles.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : When importing from `trigger.dev/core` in the webapp, use subpath exports from the package.json instead of importing from the root path
Applied to files:
packages/build/src/internal/additionalFiles.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Specify task locations in trigger.config.ts using the `dirs` array, with automatic exclusion of .test and .spec files
Applied to files:
packages/build/src/internal/additionalFiles.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure build process in trigger.config.ts using `build` object with external packages, extensions, and JSX settings
Applied to files:
packages/build/src/internal/additionalFiles.ts
🧬 Code graph analysis (1)
packages/build/src/internal/additionalFiles.ts (1)
packages/build/src/extensions/core/additionalFiles.ts (1)
AdditionalFilesOptions(4-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: claude-review
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/build/src/internal/additionalFiles.ts (4)
4-4: LGTM!The new imports
isAbsoluteandresolveare correctly added and used in the destination path resolution logic.
7-28: Excellent documentation and type definition.The JSDoc clearly explains the purpose and behavior of the
destinationoption with a helpful monorepo example. The optional field correctly uses TypeScript's type syntax per coding guidelines.
60-64: LGTM!The function signatures are correctly updated to propagate the
destinationoption through the call chain while maintaining backward compatibility with the optional parameter.Also applies to: 102-106
122-129: LGTM! Well-implemented destination logic.The destination path calculation correctly:
- Extracts the glob pattern's base directory
- Resolves relative patterns to absolute paths
- Preserves file structure relative to the glob base
- Places files under the specified destination
This matches the PR's intended behavior for monorepo setups.
|
@claude can you add unit tests to this PR and please address all review comments |
|
Claude finished @ericallam's task in 2m 56s —— View job Adding Unit Tests and Addressing Review Comments
Changes made:
|
…Files - Fix getGlobBase to return '.' for single-part patterns like 'file.txt' - Fix Windows path separator handling by using 'sep' instead of 'posix.sep' when splitting - Export getGlobBase function for testing - Add comprehensive unit tests for getGlobBase function - Add vitest configuration for @trigger.dev/build package Co-authored-by: Eric Allam <[email protected]>
|
@claude always run |
|
Claude finished @ericallam's task in 20s —— View job Updating pnpm Lockfile
Alternatively, you can run |
PR Review: fix(build): add destination option to additionalFiles extensionSummaryThis PR adds a ✅ Code Quality and Best PracticesStrengths:
Minor suggestions:
🐛 Potential Bugs or Issues
⚡ Performance ConsiderationsThe implementation looks efficient:
No performance concerns identified. 🔒 Security Concerns
🧪 Test CoverageStrengths:
Gaps:
📝 Additional Notes
Overall AssessmentThis is a well-implemented feature that solves a real problem for monorepo users. The code is clean, well-documented, and includes appropriate tests for the new helper function. Suggested improvements before merging:
Verdict: 👍 Approve with minor suggestions |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/build/package.json:
- Around line 77-79: The lockfile is outdated after adding the vitest dependency
(scripts "test" and "test:dev" in package.json), causing pipeline failures; run
`pnpm install` in the repository to regenerate `pnpm-lock.yaml`, verify the
lockfile changes reflect the new vitest dependency, and commit the updated
`pnpm-lock.yaml` alongside the package.json changes so the CI pipeline passes.
🧹 Nitpick comments (1)
packages/build/test/additionalFiles.test.ts (1)
43-71: Consider adding a test for absolute paths.The test suite doesn't appear to cover absolute paths (e.g.,
/absolute/path/**orC:\\absolute\\path\\**). While these may not be common in practice, they could occur and it would be good to verify the behavior.Suggested test cases
Add these tests to the "edge cases" describe block:
+ it("handles absolute Unix-style paths with globs", () => { + expect(getGlobBase("/absolute/path/**")).toBe("/absolute/path"); + }); + + it("handles absolute Unix-style file paths", () => { + expect(getGlobBase("/absolute/path/file.txt")).toBe("/absolute/path"); + });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/build/package.jsonpackages/build/src/internal/additionalFiles.tspackages/build/test/additionalFiles.test.tspackages/build/vitest.config.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
packages/build/vitest.config.tspackages/build/src/internal/additionalFiles.tspackages/build/test/additionalFiles.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
packages/build/vitest.config.tspackages/build/src/internal/additionalFiles.tspackages/build/test/additionalFiles.test.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
packages/build/vitest.config.tspackages/build/package.jsonpackages/build/src/internal/additionalFiles.tspackages/build/test/additionalFiles.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
packages/build/vitest.config.tspackages/build/src/internal/additionalFiles.tspackages/build/test/additionalFiles.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
packages/build/test/additionalFiles.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Avoid mocks or stubs in tests; use helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for unit tests
Files:
packages/build/test/additionalFiles.test.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Use build extensions in trigger.config.ts (additionalFiles, additionalPackages, aptGet, prismaExtension, etc.) to customize the build
📚 Learning: 2025-11-27T16:27:48.109Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:27:48.109Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vitest for unit tests
Applied to files:
packages/build/vitest.config.tspackages/build/package.jsonpackages/build/test/additionalFiles.test.ts
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Use vitest for all tests in the Trigger.dev repository
Applied to files:
packages/build/vitest.config.tspackages/build/package.jsonpackages/build/test/additionalFiles.test.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure build process in trigger.config.ts using `build` object with external packages, extensions, and JSX settings
Applied to files:
packages/build/vitest.config.tspackages/build/src/internal/additionalFiles.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Use build extensions in trigger.config.ts (additionalFiles, additionalPackages, aptGet, prismaExtension, etc.) to customize the build
Applied to files:
packages/build/vitest.config.tspackages/build/src/internal/additionalFiles.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : Test files should only import classes and functions from `app/**/*.ts` files and should not import `env.server.ts` directly or indirectly; pass configuration through options instead
Applied to files:
packages/build/vitest.config.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure Trigger.dev project in `trigger.config.ts` using `defineConfig()` with project ref and task directories
Applied to files:
packages/build/vitest.config.tspackages/build/src/internal/additionalFiles.ts
📚 Learning: 2025-11-27T16:27:48.109Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:27:48.109Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Test files should live beside the files under test and use descriptive `describe` and `it` blocks
Applied to files:
packages/build/vitest.config.tspackages/build/test/additionalFiles.test.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Specify task locations in trigger.config.ts using the `dirs` array, with automatic exclusion of .test and .spec files
Applied to files:
packages/build/vitest.config.tspackages/build/src/internal/additionalFiles.tspackages/build/test/additionalFiles.test.ts
📚 Learning: 2025-11-26T14:40:07.146Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 2710
File: packages/schema-to-json/package.json:0-0
Timestamp: 2025-11-26T14:40:07.146Z
Learning: Node.js 24+ has native TypeScript support and can execute .ts files directly without tsx or ts-node for scripts that use only erasable TypeScript syntax (type annotations, interfaces, etc.). The trigger.dev repository uses Node.js 24.11.1+ and scripts like updateVersion.ts can be run with `node` instead of `tsx`.
Applied to files:
packages/build/package.json
📚 Learning: 2025-11-27T16:26:44.496Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/executing-commands.mdc:0-0
Timestamp: 2025-11-27T16:26:44.496Z
Learning: For running tests, navigate into the package directory and run `pnpm run test --run` to enable single-file test execution (e.g., `pnpm run test ./src/engine/tests/ttl.test.ts --run`)
Applied to files:
packages/build/package.json
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to internal-packages/database/**/*.{ts,tsx} : Use Prisma for database interactions in internal-packages/database with PostgreSQL
Applied to files:
packages/build/package.json
📚 Learning: 2025-11-27T16:27:48.109Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T16:27:48.109Z
Learning: pnpm version `10.23.0` and Node.js version `20.11.1` are required for development
Applied to files:
packages/build/package.json
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : When importing from `trigger.dev/core` in the webapp, use subpath exports from the package.json instead of importing from the root path
Applied to files:
packages/build/src/internal/additionalFiles.ts
🧬 Code graph analysis (3)
packages/build/vitest.config.ts (1)
packages/trigger-sdk/src/v3/config.ts (1)
defineConfig(11-13)
packages/build/src/internal/additionalFiles.ts (1)
packages/build/src/extensions/core/additionalFiles.ts (1)
AdditionalFilesOptions(4-25)
packages/build/test/additionalFiles.test.ts (1)
packages/build/src/internal/additionalFiles.ts (1)
getGlobBase(80-102)
🪛 GitHub Actions: 🤖 PR Checks
packages/build/package.json
[error] 1-1: pnpm install --frozen-lockfile failed: lockfile is outdated. specifiers in the lockfile don't match package.json (1 dependency added: vitest@^2.0.0). Run 'pnpm install' or 'pnpm install --no-frozen-lockfile' to update.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/build/package.json (1)
77-79: LGTM!The test scripts follow standard Vitest conventions.
vitest runexecutes tests once, whilevitestruns in watch mode for development.packages/build/vitest.config.ts (1)
1-8: LGTM!The Vitest configuration is appropriate for this package. The include patterns cover both dedicated test directories and co-located tests, and enabling globals provides a convenient testing experience.
packages/build/test/additionalFiles.test.ts (1)
1-94: Excellent test coverage!The test suite is well-organized with descriptive blocks and covers a comprehensive range of cases including:
- Various glob wildcards (*, ?, [], {})
- Relative and parent directory references
- Specific file paths vs. glob patterns
- Cross-platform path separators (Windows backslashes, mixed slashes)
- Edge cases (empty string, single filename, dot files)
The tests properly validate that the function normalizes paths to POSIX separators, which is important for cross-platform compatibility.
packages/build/src/internal/additionalFiles.ts (5)
4-4: LGTM!The additional imports (
isAbsolute,resolve) are necessary for the destination-aware path resolution logic introduced in this PR.
9-27: Well-documented feature addition.The documentation clearly explains the destination option's purpose and behavior, with a practical monorepo example. This addresses the core problem described in the PR objectives where parent directory references were being normalized incorrectly.
124-139: Correct platform-specific path handling.The path calculation logic properly handles cross-platform differences:
- Line 136 correctly uses platform-specific
septo split the output ofrelative(), which returns platform-specific separators- Line 138 normalizes to POSIX separators for consistency
- Lines 124-131 handle the destination option by computing paths relative to the glob base
This approach ensures consistent behavior across Windows and Unix-like systems while preserving the original structure.
80-102: Consider edge case: patterns with only glob characters.What happens if the pattern consists only of glob characters, like
"*"or"**"? The function would:
- Split into
["*"]or["**"]- Immediately detect glob characters and break
basePartswould be empty- Return
"."This behavior seems correct (returns current directory), but it would be good to verify this is intentional and add test coverage if missing.
Looking at the test file, line 23 covers
"*.js"→"."and line 19 covers"**/*.js"→".", which validates this behavior. No action needed.
76-102: Verify the intended behavior for glob patterns with trailing slashes.The
getGlobBasefunction's handling of trailing slashes (e.g.,"../shared/") produces"../shared", which occurs because empty strings resulting from trailing separators are also collected and then popped. Confirm whether this behavior is correct by reviewing test cases and actual usage patterns in the codebase, or add explicit test coverage for trailing slash scenarios if it's currently missing.
|
@claude can you try doing pnpm install again and committing the lock file |
|
Claude finished @ericallam's task in 35s —— View job Running pnpm install and Committing Lockfile
To fix this, you have a couple of options:
|
|
@claude try Running pnpm install and Committing Lockfile again |
|
Claude finished @ericallam's task in 2m 7s —— View job Running pnpm install and Committing Lockfile
Successfully ran |
PR Review: fix(build): add destination option to additionalFiles extensionSummaryThis PR adds a ✅ Strengths
🔍 Potential Issues & Suggestions1. Type Definition DuplicationThe
Suggestion: Consider exporting the type from one location and importing it in the other to maintain a single source of truth. 2. Missing Integration TestsThe tests only cover the Suggestion: Add tests that exercise the complete flow, for example:
3. Path Separator Handling on WindowsIn it("handles Windows-style backslashes", () => {
expect(getGlobBase("..\\shared\\**")).toBe("../shared");
});This works correctly due to the 4. Edge Case: Empty Destination StringWhat happens if 5. Potential Path Traversal ConsiderationThe // This could potentially write outside the build directory
additionalFiles({
files: ["./config.json"],
destination: "../../../../etc"
})Suggestion: Consider adding validation or a note in the documentation that the destination should be a relative path within the build output. 📝 Minor Suggestions
Overall AssessmentThis is a well-implemented feature that solves a genuine pain point for monorepo users. The code is clean, well-documented, and maintains backward compatibility. The main suggestions are around adding integration tests and considering the type duplication. Recommendation: ✅ Approve with minor suggestions |
Co-authored-by: Eric Allam <[email protected]>
PR Review: fix(build): add destination option to additionalFiles extensionSummaryThis PR adds an optional ✅ Code Quality and Best PracticesStrengths:
Minor Suggestions:
🐛 Potential Bugs or Issues
⚡ Performance Considerations
🔒 Security Concerns
🧪 Test CoverageStrengths:
Areas for Improvement:
📝 Documentation
Overall AssessmentThis is a well-implemented feature that solves a real problem for monorepo users. The code is clean, well-documented, and the test coverage for the helper function is thorough. Recommendations before merging:
Good work! 👍 |
|
@claude our PR checks unit tests merge reports is failing with the following: Run pnpm dlx [email protected] run --merge-reports --pass-with-no-tests ⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯
make sure you are using the correct version of vitest in the build package please |
|
Claude finished @ericallam's task in 1m 34s —— View job Fixing Vitest Version Mismatch
Issue found: Fix applied: Updated |
…eporting Co-authored-by: Eric Allam <[email protected]>
PR Review: Add
|
PR Review: fix(build): add destination option to additionalFiles extensionOverall, this is a well-implemented feature that solves a real problem for monorepo users. The code is clean, well-documented, and includes good test coverage. Here's my detailed review: Code Quality and Best PracticesStrengths:
Minor suggestions:
Potential Bugs or Issues
Performance ConsiderationsThe implementation looks performant:
Security Concerns
Test CoverageStrengths:
Suggestions for improvement:
DocumentationThe documentation changes are excellent:
SummaryThis is a solid PR that addresses a real pain point. The implementation is clean and well-tested for the core utility function. My main recommendations are:
Verdict: Approve with minor suggestions - The code is production-ready, and the suggestions above are nice-to-haves rather than blockers. |
|
@claude can you add a patch changeset for this please |
|
Claude finished @ericallam's task in 1m 12s —— View job Adding Patch Changeset
Added changeset file |
Co-authored-by: Eric Allam <[email protected]>
PR Review: Add destination option to additionalFiles extensionSummaryThis PR adds a ✅ Strengths1. Well-designed solution 2. Excellent documentation
3. Good test coverage
4. Clean implementation
|
When using glob patterns with parent directory references (../), the default behavior strips ".." segments resulting in unexpected paths. For example, "../shared/**" would place files at "shared/" instead of preserving the original path structure.
This adds an optional "destination" parameter that allows users to explicitly specify where matched files should be placed:
additionalFiles({
files: ["../shared/**"],
destination: "apps/shared"
})
When destination is specified, files are placed relative to the glob pattern's base directory under the destination path. This is useful in monorepo setups where files need to maintain their structure.
Also updates documentation to explain this behavior and the new option.