PR Review: fix(build): add destination option to additionalFiles extension

Summary

This PR adds a destination option to the additionalFiles extension to address path resolution issues when using glob patterns with parent directory references (../). The implementation is clean and the documentation is thorough.

Code Quality & Best Practices ✅

• Good: Well-documented with JSDoc comments including examples
• Good: The getGlobBase helper function is clearly commented and handles common glob characters
• Good: Backwards compatible - existing behavior is preserved when destination is not specified
• Minor: The type AdditionalFilesOptions is duplicated in both packages/build/src/extensions/core/additionalFiles.ts and packages/build/src/internal/additionalFiles.ts. Consider exporting from one location and importing in the other to maintain DRY principles.

Potential Issues ⚠️

1. Path separator inconsistency: In getGlobBase(), the pattern is split by /[/\\]/ (both forward and back slashes) but rejoined with posix.sep (always /). While this works, it could cause issues on Windows if the input pattern uses backslashes and the result is compared with a path using native separators. Consider using path.sep for the join, or documenting that glob patterns should always use forward slashes.

2. Glob base extraction edge cases: The getGlobBase function may not handle all glob patterns correctly:

   • Patterns like ../** would return .. which resolves correctly
   • But patterns like {a,b}/** would return . (treating {a,b} as glob)
   • Consider whether patterns starting with brace expansion need special handling

3. Security consideration: When destination is user-provided, there's no validation that it doesn't contain path traversal sequences like ../../../etc. While this is a build-time operation running in the user's own environment, it's worth documenting that users should not pass untrusted input to this option.

Performance ✅

• The getGlobBase function is called once per file in the loop at line 115, but since it only depends on the matcher (which is constant for all files from that matcher), it could be hoisted outside the file loop for a minor optimization. However, this is unlikely to matter in practice.

Test Coverage ⚠️

• No unit tests added: This PR adds new functionality (getGlobBase, destination path computation) but no tests. Consider adding tests for:
  • getGlobBase with various patterns (../shared/**, ./assets/*.txt, **/*.js, {a,b}/**)
  • Path computation with destination specified vs not specified
  • Edge cases like empty destination, absolute destination paths

Documentation ✅

• Good: Comprehensive documentation update in additionalFiles.mdx
• Good: Typo fix (effects → affects)
• Good: Multiple examples showing different use cases including monorepo setups

Suggestions

1. Add unit tests for the new functionality, particularly for getGlobBase and the path computation logic.

2. Consider extracting getGlobBase call outside the file loop:

const globBase = options?.destination ? getGlobBase(matcher) : null;
const absoluteGlobBase = globBase && !isAbsolute(globBase) ? resolve(cwd, globBase) : globBase;
for (const file of files) {
 // ... use absoluteGlobBase
}

3. Consider validating that destination doesn't start with / or contain .. to prevent unexpected behavior.

Overall

This is a well-implemented feature that solves a real problem for monorepo users. The code is clean, the documentation is excellent, and the implementation is backwards compatible. The main gap is test coverage - adding tests would significantly improve confidence in the path handling logic, especially around edge cases.

Recommendation: Approve with suggestion to add tests in a follow-up PR.

Review Complete

Your review story is ready!

View Story

Comment !reviewfast on this PR to re-generate the story.

@claude add a patch changeset file please

Claude finished @ericallam's task in 39s —— View job

Adding Changeset

• Create patch changeset file for @trigger.dev/build
• Commit and push changes

Added changeset file .changeset/light-apricots-listen.md with a patch bump for @trigger.dev/build.
|

PR Review: fix(build): add destination option to additionalFiles extension

Summary

This PR adds a destination option to the additionalFiles extension to give users explicit control over where files are placed when using glob patterns with parent directory references (../). This is a useful enhancement for monorepo setups.

Code Quality and Best Practices

Positives:

• Well-documented with JSDoc comments and comprehensive documentation updates
• The type definition is duplicated in both files (packages/build/src/extensions/core/additionalFiles.ts and packages/build/src/internal/additionalFiles.ts), which is intentional since the extension re-exports the options type
• The getGlobBase helper function is well-commented and handles common glob characters
• The changeset is properly included

Suggestions:

1. Type duplication: Consider importing the AdditionalFilesOptions type from the internal module in the extension file rather than duplicating it. This ensures they stay in sync:

   export type { AdditionalFilesOptions } from "../../internal/additionalFiles.js";

2. Typo fix in docs: Good catch on fixing "effects" → "affects" in the documentation.

Potential Bugs or Issues

1. Cross-platform path handling (Minor): The getGlobBase function uses posix.sep when joining base parts (line 90), but splits on both / and \ (line 79). While this is likely intentional for cross-platform input handling with POSIX-style output, consider adding a comment to clarify this design decision.

2. Path separator inconsistency: In the new code path (lines 113-120), join() from node:path is used (which is platform-specific), while the original code (lines 123-126) uses posix.sep. This could cause inconsistent behavior on Windows. Consider using posix.join consistently:

   pathInsideDestinationDir = posix.join(options.destination, relativeToGlobBase);

3. Absolute destination paths: If a user provides an absolute path for destination (e.g., /apps/shared), the behavior might be unexpected. Consider validating that destination is a relative path, or documenting the expected behavior.

Performance Considerations

• No concerns: The getGlobBase function is called once per file, which is minimal overhead. The implementation is efficient.

Security Concerns

1. Path traversal (Low risk): The destination option could theoretically allow placing files outside the intended build directory if combined with .. segments. However, since this is a build-time configuration controlled by the developer (not user input), the risk is minimal. Still, you might consider normalizing the destination path and ensuring files stay within the output directory:

   const normalizedDest = posix.normalize(options.destination);
   if (normalizedDest.startsWith('..')) {
    throw new Error('destination cannot reference parent directories');
   }

Test Coverage

Concern: There don't appear to be any unit tests for the additionalFiles functionality, including this new feature. Given that this touches file system operations and path manipulation (which can be tricky cross-platform), I'd recommend adding tests for:

1. Basic destination functionality with simple patterns
2. Nested directory structures with destination
3. Edge cases like empty destination, destination with trailing slashes
4. The getGlobBase function (could be unit tested in isolation)
5. Patterns without glob characters (e.g., exact file paths)

Example test cases for getGlobBase:

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 Assessment

This 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:

1. Address the path separator inconsistency (use posix.join consistently)
2. Consider adding basic unit tests for the new functionality

Nice to have:

• Validate that destination doesn't contain path traversal sequences
• Consider extracting and sharing the type definition to avoid duplication

Good work on this enhancement! 👍

Code Review: fix(build): add destination option to additionalFiles extension

Thanks for this PR! The feature to add a destination option for the additionalFiles extension is useful for monorepo setups. Here is my feedback:

Overall Assessment

The PR provides a clean solution to a real problem - when using glob patterns with parent directory references (../), the default behavior strips .. segments, which can be confusing. The new destination option gives users explicit control.

Code Quality and Best Practices

Strengths:

• Good JSDoc documentation on the destination option with practical examples
• The getGlobBase function is well-documented with clear comments explaining its purpose
• Documentation updates are thorough and include practical monorepo examples
• Nice typo fix ("effects" -> "affects") in the docs

Suggestions:

1. Type Duplication: The AdditionalFilesOptions type is duplicated in both files:

   • packages/build/src/extensions/core/additionalFiles.ts
   • packages/build/src/internal/additionalFiles.ts

   Consider exporting from one location and importing in the other to maintain DRY principles and avoid drift.

2. Consider extracting getGlobBase: This function might be useful elsewhere. Consider whether it should be a shared utility.

Potential Bugs or Issues

1. Path Separator Consistency (Medium Priority):
   In getGlobBase, the code splits on both / and \\ (/[/\\]/), but joins with posix.sep (which is always /). While this works, mixing join() (which uses native separators) with posix.sep in getGlobBase could theoretically cause issues on Windows:

   // Line 129 uses join() which uses native path.sep on Windows
   pathInsideDestinationDir = join(options.destination, relativeToGlobBase);

   Consider using posix.join() consistently for the destination path to ensure consistent forward slashes across platforms.

2. Empty Pattern Edge Case:
   The getGlobBase function returns "." for an empty string input. While unlikely in practice, you might want to validate input patterns before processing.

3. Single-file Glob Patterns:
   For a pattern like "../shared/specific-file.ts" (no glob characters), getGlobBase returns "../shared" (pops the last segment). This seems intentional but the behavior should be documented or tested to ensure it produces the expected destination paths.

Performance Considerations

The implementation looks efficient:

• getGlobBase is called once per file rather than being precomputed per matcher, which is slightly suboptimal but unlikely to matter in practice given typical file counts
• Consider caching getGlobBase(matcher) and absoluteGlobBase outside the file loop (lines 124-125) since they do not change per file

Security Concerns

1. Path Traversal (Low Risk):
   The destination option allows users to specify arbitrary paths. While this is user-controlled configuration (not untrusted input), users could potentially specify destinations like "../../some-sensitive-location". This is likely acceptable since:

   • Users have full control over their config file
   • Build output is generally sandboxed to the build directory

   However, you might want to add a note in the documentation about ensuring the destination stays within expected boundaries.

2. The existing .. stripping behavior in the default path (lines 132-135) is a good security practice - this is preserved correctly.

Test Coverage

This is the main concern: I could not find any unit tests for the new functionality. Consider adding tests for:

1. Basic destination option functionality
2. getGlobBase function with various patterns:
   • Glob patterns: "../shared/**", "./assets/*.ts"
   • Specific files: "../file.ts"
   • Edge cases: "**", "file.ts", empty strings
3. Path computation with nested directories
4. Backward compatibility (ensure behavior without destination is unchanged)

Example test cases for getGlobBase:

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

1. The matches variable was removed (line ~73 in original) - good cleanup since it was unused.

2. Consider adding a warning log when destination is used with a pattern that does not contain .. (since users might not need destination in that case).

Summary

This 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 getGlobBase function and destination path computation work correctly across edge cases.

Once tests are added, this looks good to merge! 🎉

@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

• Read current implementation and understand the code
• Analyze all review comments to address
• Fix edge case for single-part non-glob patterns in getGlobBase
• Fix Windows path separator handling in fallback behavior
• Create vitest config for packages/build
• Add unit tests for getGlobBase function
• Commit and push changes

Changes made:

1. Fixed getGlobBase single-part pattern edge case (packages/build/src/internal/additionalFiles.ts:97-98)

   • Previously, patterns like "file.txt" would return "file.txt" as the base, causing relative() to return an empty string and losing the filename
   • Now correctly returns "." for single-part patterns

2. Fixed Windows path separator handling (packages/build/src/internal/additionalFiles.ts:136)

   • Changed from .split(posix.sep) to .split(sep) in the fallback behavior
   • This ensures ".." segments are properly filtered on Windows where path.relative() returns backslashes

3. Added unit tests (packages/build/test/additionalFiles.test.ts)

   • 21 test cases covering:
     • Glob patterns with wildcards (../shared/**, ./assets/*.txt, **/*.js, etc.)
     • Specific file paths without globs (including single-part patterns)
     • Edge cases (empty string, Windows backslashes, mixed separators)

4. Added vitest configuration for @trigger.dev/build package
   | Branch

@claude always run pnpm install and commit the pnpm lockfile when making any changes around deps

Claude finished @ericallam's task in 20s —— View job

Updating pnpm Lockfile

• Run pnpm install to update lockfile
• Commit and push the updated lockfile

⚠️ I'm unable to run pnpm install because the command requires approval. To enable this, you'll need to add pnpm to the --allowedTools configuration for the Claude Code Action.

Alternatively, you can run pnpm install locally and commit the updated lockfile.
| Branch

PR Review: fix(build): add destination option to additionalFiles extension

Summary

This PR adds a destination option to the additionalFiles extension to address path resolution issues when using glob patterns with parent directory references (../). The current behavior strips .. segments, which can lead to unexpected file placement in monorepo setups.

✅ Code Quality and Best Practices

Strengths:

• Well-documented: The JSDoc comments on the destination option are thorough with clear examples
• Consistent type duplication: The AdditionalFilesOptions type is defined in both the internal module and the extension module, following the existing pattern in the codebase
• Clean separation of concerns: The getGlobBase helper function is well-isolated and properly exported for testing
• Documentation: The MDX docs clearly explain the problem and solution with good examples

Minor suggestions:

1. Consider extracting the AdditionalFilesOptions type to a shared location to avoid duplication between src/extensions/core/additionalFiles.ts and src/internal/additionalFiles.ts. If one is updated without the other, it could lead to inconsistencies.

🐛 Potential Bugs or Issues

1. Path separator inconsistency in getGlobBase: The function splits on both / and \\ but always joins with posix.sep (forward slash). While this works for the glob pattern parsing use case, it may produce unexpected results on Windows if the caller expects backslashes. This seems intentional for cross-platform glob support, but worth noting.

2. Edge case with trailing slashes:

   getGlobBase("./") // returns "."
   getGlobBase("../") // returns ".."

   These work correctly per the tests, but consider whether patterns like "./foo/" (directory with trailing slash but no glob) behave as expected - currently it would return "." because it pops the empty string after the trailing slash.

3. No validation on destination: If a user provides an absolute path for destination, it would work but could place files outside the build directory unexpectedly. Consider adding validation or documentation warning.

⚡ Performance Considerations

The implementation looks efficient:

• getGlobBase is called once per file only when destination is specified
• The glob base calculation is O(n) where n is the number of path segments (typically small)
• No unnecessary file system operations added

No performance concerns identified.

🔒 Security Concerns

1. Path traversal consideration: The destination option accepts arbitrary strings. While this is used within the build process context, malicious or accidental values like ../../.. could potentially place files outside the intended build directory. The join() call with destinationPath should prevent escaping the build output directory, but this assumption depends on how destinationPath is constructed upstream.

   Recommendation: Consider adding validation that the final relativeDestinationPath is within the expected build output directory, or document that destination should be a relative path without parent directory references.

🧪 Test Coverage

Strengths:

• Comprehensive unit tests for getGlobBase covering:
  • Various glob patterns with wildcards
  • Specific file paths without globs
  • Edge cases (empty string, Windows backslashes, mixed slashes, trailing slashes)
• Good test organization with descriptive test names

Gaps:

1. No integration tests: The tests only cover the getGlobBase helper function. Consider adding tests for findStaticAssetsForMatcher with the destination option to verify the full path resolution logic works correctly end-to-end.

2. Missing test cases for getGlobBase:

   • Pattern with only wildcards: "**" or "***"
   • Negation patterns: "!foo/**" (if supported)
   • Very deep nesting: "a/b/c/d/e/f/g/**"

3. Missing integration test scenarios:

   • Verify files are actually copied to correct destination with destination option
   • Test behavior when destination is an absolute path
   • Test behavior when destination contains .. segments

📝 Additional Notes

1. Typo fix: Good catch fixing "effects" → "affects" in the documentation.

2. vitest addition: The package.json changes to add vitest and test scripts are appropriate. The vitest.config.ts follows the pattern established in @trigger.dev/core.

3. Semicolon missing: In vitest.config.ts line 8, there is no semicolon after the closing parenthesis. This is fine if the project uses a linter that allows it, but check project conventions.

Overall Assessment

This 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:

1. Add at least one integration test that verifies the full destination flow
2. Consider adding validation/documentation for the destination parameter to prevent path traversal issues
3. Consider extracting the shared type to avoid duplication

Verdict: 👍 Approve with minor suggestions

@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

• Run pnpm install to update lockfile
• Commit and push the updated lockfile

⚠️ I'm unable to run pnpm install because pnpm is not installed in this environment and the commands needed to install it (corepack enable, corepack prepare) require approval.

To fix this, you have a couple of options:

1. Add pnpm-related commands to your allowed tools - You can update the Claude Code Action --allowedTools configuration to include corepack and pnpm commands.

2. Run locally - You can run pnpm install on your local machine and commit the updated pnpm-lock.yaml file.

View job | Branch

@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

• Run pnpm install to update lockfile
• Commit and push the updated lockfile

Successfully ran pnpm install and committed the updated pnpm-lock.yaml file. The lockfile now includes the vitest dependency added in packages/build/package.json.
| Branch

PR Review: fix(build): add destination option to additionalFiles extension

Summary

This PR adds a destination option to the additionalFiles build extension, solving a real problem with monorepo setups where glob patterns with .. references get their parent directory segments stripped, leading to unexpected file placement.

✅ Strengths

1. Well-documented: Excellent JSDoc comments and comprehensive MDX documentation with practical monorepo examples.

2. Backward compatible: The destination option is optional, preserving existing behavior when not specified.

3. Good test coverage: The getGlobBase function has thorough unit tests covering glob patterns, specific file paths, and edge cases including Windows-style paths.

4. Clean implementation: The getGlobBase helper is well-designed with clear comments explaining its purpose.

🔍 Potential Issues & Suggestions

1. Type Definition Duplication

The AdditionalFilesOptions type with the destination property and its JSDoc is duplicated in two files:

• packages/build/src/extensions/core/additionalFiles.ts (lines 4-25)
• packages/build/src/internal/additionalFiles.ts (lines 7-28)

Suggestion: Consider exporting the type from one location and importing it in the other to maintain a single source of truth.

2. Missing Integration Tests

The tests only cover the getGlobBase helper function. There are no integration tests for findStaticAssetsForMatcher with the destination option to verify the full path resolution logic.

Suggestion: Add tests that exercise the complete flow, for example:

• Verify that ../shared/utils.ts with destination: "libs" produces libs/utils.ts
• Test multiple files with nested directories to ensure structure is preserved

3. Path Separator Handling on Windows

In getGlobBase, the function joins parts with posix.sep (line 101), which correctly normalizes to forward slashes. However, the tests for Windows-style backslashes (line 77-82) expect the output to use forward slashes:

it("handles Windows-style backslashes", () => {
 expect(getGlobBase("..\\shared\\**")).toBe("../shared");
});

This works correctly due to the posix.sep usage, but it's worth noting this intentional cross-platform normalization in a comment.

4. Edge Case: Empty Destination String

What happens if destination: "" is passed? The code would check if (options?.destination) which would be falsy for an empty string, falling back to default behavior. This might be the intended behavior, but it's worth documenting or validating.

5. Potential Path Traversal Consideration

The destination option accepts arbitrary strings. While this is a build-time configuration (not user input), it might be worth considering whether paths like ../../../ in the destination could write files outside the build directory.

// 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

1. Vitest config consistency: The new vitest.config.ts matches the pattern in packages/core/vitest.config.ts - good consistency.

2. Typo fix: Good catch on changing "effects" to "affects" in the documentation.

Overall Assessment

This 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

PR Review: fix(build): add destination option to additionalFiles extension

Summary

This PR adds an optional destination parameter to the additionalFiles extension to control where matched files are placed when using glob patterns with parent directory references (..). This is a useful enhancement for monorepo setups.

✅ Code Quality and Best Practices

Strengths:

• Clean implementation with clear separation of concerns
• Good JSDoc documentation on the destination option with practical examples
• The getGlobBase helper function is well-documented and handles various edge cases
• Documentation is thorough and provides good examples for users

Minor Suggestions:

• The AdditionalFilesOptions type is duplicated in both packages/build/src/extensions/core/additionalFiles.ts and packages/build/src/internal/additionalFiles.ts. Consider exporting the type from one location and importing in the other to maintain DRY principles.

🐛 Potential Bugs or Issues

1. Path traversal concern: When destination is specified, there's no validation to prevent users from accidentally specifying paths that could escape the destination directory (e.g., destination: "../../../etc"). While this is user-controlled configuration, a warning or validation could prevent misconfiguration.

2. Trailing slash handling in getGlobBase: The function handles "../" and "./" patterns, but the behavior might be unexpected when a trailing slash is present:

   • getGlobBase("../") returns ".." (after popping empty string)
   • This works because splitting "../" gives [".", ".", ""] - wait, actually it should give ["..", ""]. Let me verify: splitting by / gives ["..", ""], and since no glob chars, it pops to get [".."], returning "..". This looks correct.

3. Cross-platform path handling: The code uses both posix.sep and sep (platform-specific). The logic in getGlobBase joins with posix.sep, which is consistent. However, in findStaticAssetsForMatcher, when destination is set, join() is used (which is platform-specific), but in the default path, posix.sep is used for joining. This inconsistency could lead to different behavior on Windows vs Unix:

   // With destination - uses platform join()
   pathInsideDestinationDir = join(options.destination, relativeToGlobBase);
   // Without destination - uses posix.sep
   .join(posix.sep);

⚡ Performance Considerations

• The implementation is efficient - getGlobBase is called once per matcher with destination specified
• No performance concerns with this approach

🔒 Security Concerns

• Low risk: The destination option only controls the output path within the build directory. Since this is configuration provided by the developer in trigger.config.ts, the risk is minimal.
• Consider documenting that the destination path should be relative and within the project structure.

🧪 Test Coverage

Strengths:

• Excellent unit test coverage for getGlobBase function
• Tests cover various glob patterns, specific file paths, and edge cases
• Good handling of Windows-style paths in tests

Areas for Improvement:

1. Missing integration tests: The tests only cover the getGlobBase helper. Consider adding tests for findStaticAssetsForMatcher with the destination option to verify end-to-end behavior.

2. Missing tests for the actual file copying with destination: A test that verifies files are actually copied to the correct destination path would increase confidence.

3. Edge cases not tested:

   • What happens when destination is an empty string?
   • What happens when destination is an absolute path?
   • What happens with multiple files matching the same glob pattern?

📝 Documentation

• The docs update is clear and provides helpful examples
• Good fix for the typo: "effects" → "affects"
• The monorepo example is practical and demonstrates the problem well

Overall Assessment

This 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:

1. Consider adding validation or a warning for potentially problematic destination values
2. Ensure consistent path separator usage across platforms (minor)
3. Consider adding an integration test for the full flow with destination option

Good work! 👍

@claude our PR checks unit tests merge reports is failing with the following:

Run pnpm dlx vitest@3.1.4 run --merge-reports --pass-with-no-tests
Progress: resolved 1, reused 0, downloaded 0, added 0
Packages: +48
++++++++++++++++++++++++++++++++++++++++++++++++
Progress: resolved 97, reused 0, downloaded 48, added 48, done

⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯
Error: vitest.mergeReports() requires all blob files to be generated by the same Vitest version, received

• "packages_build-blob-1-1.json" uses v2.1.9
• "packages_core-blob-1-1.json" uses v3.1.4
• "packages_redis-worker-blob-1-1.json" uses v3.1.4
• "packages_schema-to-json-blob-1-1.json" uses v3.1.4
• "packages_trigger-sdk-blob-1-1.json" uses v3.1.4

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

• Investigate the vitest version issue in packages/build
• Update vitest to v3.1.4 to match other packages
• Update pnpm-lock.yaml
• Commit and push changes

Issue found: packages/build/package.json had "vitest": "^2.0.0" while the root package.json has "vitest": "3.1.4". This caused the merge-reports step to fail since the blobs were generated with different vitest versions.

Fix applied: Updated packages/build/package.json to use "vitest": "3.1.4" (exact version match with the root) and regenerated the lockfile.
| Branch

PR Review: Add destination option to additionalFiles extension

Summary

This PR adds an optional destination parameter to the additionalFiles build extension, addressing a real pain point in monorepo setups where .. segments in glob patterns get stripped during file copying.

Code Quality and Best Practices ✅

Strengths:

• Clean, well-documented code with comprehensive JSDoc comments
• The getGlobBase() function is well-isolated and testable
• Good separation of concerns between the public API and internal implementation
• Consistent coding style throughout
• Type definitions are duplicated appropriately between the public API (extensions/core/additionalFiles.ts) and internal implementation

Minor Suggestions:

• The type AdditionalFilesOptions is defined in two places (src/extensions/core/additionalFiles.ts and src/internal/additionalFiles.ts). Consider exporting from one location and importing in the other to maintain a single source of truth, or add a comment explaining the intentional duplication.

Potential Bugs or Issues ⚠️

1. Path separator inconsistency (line 101 in additionalFiles.ts): The getGlobBase() function always joins with posix.sep (forward slash), even when handling Windows-style backslashes in input. This is fine for glob patterns (which typically use forward slashes), but worth verifying that resolve() on Windows handles this correctly when passed to resolve(cwd, globBase) at line 127.

2. Edge case with trailing slashes: The tests cover "./" and "../" but the behavior when a user passes a destination with a trailing slash (e.g., destination: "apps/shared/") isn't explicitly tested. While join() should handle this, it would be good to add a test case for confidence.

3. Absolute paths in destination: What happens if a user passes an absolute path as destination? The current implementation would create files outside the build directory. Consider validating that destination is a relative path, or documenting this behavior explicitly.

Performance Considerations ✅

• The getGlobBase() function is called once per matcher, not per file, so there's no performance concern there
• File operations are sequential within each matcher, which is appropriate for avoiding file system contention
• No unnecessary allocations or operations introduced

Security Concerns ⚠️

1. Path traversal: If destination accepts user input (unlikely in this context but worth considering), a malicious value like ../../../etc could write files outside the intended directory. Since this is a build configuration that developers control, this is low risk, but you might want to add a note in the documentation that destination should be a relative path within the build output.

2. The existing behavior of stripping .. segments actually served as an implicit security measure - worth noting that the new destination option bypasses this, putting more responsibility on the user to specify safe paths.

Test Coverage ✅

Excellent test coverage for getGlobBase():

• 21 test cases covering glob patterns, specific files, and edge cases
• Good coverage of Windows-style paths and mixed separators
• Edge cases like empty strings handled

Missing test coverage:

1. No integration tests for findStaticAssetsForMatcher() with the new destination option
2. No tests verifying the end-to-end behavior of addAdditionalFilesToBuild() with different destination values
3. Consider adding tests for:
   • Multiple files matching a single glob with destination
   • Nested directory structures preserved correctly
   • Error cases (e.g., invalid patterns)

Documentation ✅

The documentation updates are thorough and well-written:

• Clear explanation of the problem being solved
• Practical monorepo example structure
• Multiple usage examples including combining multiple additionalFiles extensions
• Appropriate use of <Note> components for important details

Small typo fix included: "effects" → "affects" (good catch!)

Overall Assessment

This is a well-implemented feature that solves a real problem. The code is clean, well-documented, and includes good test coverage for the core logic.

Recommendations before merging:

1. Consider adding an integration test for the destination option
2. Optionally add validation/documentation for absolute paths in destination
3. Consider consolidating the AdditionalFilesOptions type definition

Verdict: Approve with minor suggestions 👍

PR Review: fix(build): add destination option to additionalFiles extension

Overall, 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 Practices

Strengths:

• Excellent JSDoc documentation on both the public API (additionalFiles.ts) and internal implementation (internal/additionalFiles.ts)
• The getGlobBase function is well-isolated and pure, making it easy to test
• Good separation of concerns - the glob base extraction logic is cleanly separated from the file path computation
• Documentation is thorough and includes practical monorepo examples

Minor suggestions:

1. The AdditionalFilesOptions type is duplicated in both src/extensions/core/additionalFiles.ts and src/internal/additionalFiles.ts with identical JSDoc. Consider exporting from one location and re-exporting from the other to avoid potential drift.

2. In getGlobBase, the function uses posix.sep for joining but splits on both / and \\. This is intentional cross-platform handling, but a brief comment explaining why would help future maintainers.

Potential Bugs or Issues

1. Path traversal concern (line 131): When using join(options.destination, relativeToGlobBase), if relativeToGlobBase somehow contains .. segments (which could happen if the glob matches files outside the expected base), files could be written outside the intended destination. Consider validating that the final path is actually under the destination directory, or documenting this as expected behavior.

2. Empty destination string: If a user passes destination: "", the current code will use the empty string in join(), which should work but may produce unexpected results. Consider either documenting this or adding validation.

3. Absolute paths in destination: The code doesn't explicitly handle if someone passes an absolute path for destination. The behavior would likely be platform-dependent via join(). This edge case should either be documented or validated.

Performance Considerations

The implementation looks performant:

• The getGlobBase function is O(n) where n is the number of path segments - efficient
• No unnecessary iterations or allocations
• The glob operation is already the bottleneck, and no additional file system operations were added

Security Concerns

1. Path traversal: As mentioned above, there's a potential for path traversal if the glob matches unexpected files. In a build tool context, this is likely acceptable since the user controls the configuration, but it's worth being aware of.

2. No new external dependencies for the core logic - vitest is dev-only, which is good.

Test Coverage

Strengths:

• Comprehensive test suite for getGlobBase covering:
  • Various glob patterns (**, *, ?, brackets, braces)
  • Specific file paths without globs
  • Edge cases (empty strings, Windows paths, mixed separators)
• Tests are well-organized with descriptive names

Suggestions for improvement:

1. Integration tests: Consider adding integration tests that actually exercise findStaticAssetsForMatcher with the destination option. The current tests only cover the getGlobBase helper function.

2. Test the main code path: Tests for scenarios like:

   • destination with a glob pattern that has .. segments
   • destination with an absolute glob base
   • Multiple files matched with a destination
   • Edge case: destination with trailing slash vs without

3. vitest.config.ts: The config uses globals: true. Consider whether this is consistent with other test configurations in the monorepo.

Documentation

The documentation changes are excellent:

• Clear explanation of the problem with monorepos
• Practical examples with directory structures
• Shows both single and multiple destination configurations
• Good use of <Note> components to highlight important behavior

Summary

This 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:

1. Consider adding integration tests for the destination option path
2. Document or validate edge cases around empty/absolute destination paths
3. Consider consolidating the duplicated AdditionalFilesOptions type definition

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

• Find existing changeset file for this PR
• Create patch changeset for @trigger.dev/build
• Commit and push changes

Added changeset file .changeset/polite-lobsters-double.md with a patch bump for @trigger.dev/build.
| Branch

PR Review: Add destination option to additionalFiles extension

Summary

This PR adds a destination option to the additionalFiles build extension, solving a real problem where glob patterns with parent directory references (../) would have their .. segments stripped, causing unexpected file placement. This is particularly useful for monorepo setups.

✅ Strengths

1. Well-designed solution
The destination option elegantly solves the problem while maintaining backward compatibility. The default behavior (stripping .. segments) is preserved when destination is not specified.

2. Excellent documentation
The docs (additionalFiles.mdx) are comprehensive with clear examples showing:

• The problem with default behavior
• How to use the destination option
• How to handle multiple directories with different destinations

3. Good test coverage
The 24 unit tests for getGlobBase cover:

• Various glob patterns with wildcards
• Specific file paths without globs
• Edge cases including empty strings and Windows-style backslashes

4. Clean implementation
The getGlobBase function is well-documented and handles edge cases appropriately (glob vs specific file patterns, cross-platform path separators).

⚠️ Issues & Suggestions

1. Duplicate type definitions (Code Quality)

AdditionalFilesOptions is defined in two places:

• packages/build/src/internal/additionalFiles.ts:7
• packages/build/src/extensions/core/additionalFiles.ts:4

While both currently have the same properties, this duplication could lead to inconsistencies if one is updated but not the other. Consider:

• Having the public extension import and re-export the internal type, OR
• Creating a shared types file

// Option A: In extensions/core/additionalFiles.ts
export type { AdditionalFilesOptions } from "../../internal/additionalFiles.js";

2. Missing integration tests

The tests only cover the getGlobBase utility function. Consider adding integration tests for findStaticAssetsForMatcher that verify the actual file copying behavior with the destination option. This would ensure:

• Files are copied to the correct destination paths
• The relative structure is preserved as documented
• Edge cases like nested directories work correctly

3. Potential path traversal concern (Security)

When destination is specified, there's no validation that the resulting path stays within the expected output directory. While this is a build-time operation (not runtime) and users control the config, consider adding a check:

// In findStaticAssetsForMatcher after computing pathInsideDestinationDir
const normalizedPath = posix.normalize(pathInsideDestinationDir);
if (normalizedPath.startsWith('..')) {
 throw new Error(`Destination path escapes build directory: ${pathInsideDestinationDir}`);
}

4. Minor: vitest globals configuration

In vitest.config.ts, you have globals: true, but the test file explicitly imports { describe, it, expect } from vitest. Either:

• Remove the imports in test files to use globals, OR
• Remove globals: true from the config since you're using explicit imports

5. Consider documenting the empty result warning behavior

When no files match a pattern, a warning is logged. This is good, but users might want to know about this behavior in the docs, especially for patterns that legitimately match zero files in some environments.

🔍 Code Quality Notes

Path handling is correct:

• Uses platform-specific sep for splitting relative() output
• Uses posix.sep for joining output paths (ensuring consistent forward slashes)
• Handles both forward slashes and backslashes in glob patterns

The changeset is appropriate:

• Patch version bump is correct for this backward-compatible addition
• Description is clear and concise

Summary

This is a well-implemented feature that solves a real problem for monorepo users. The main suggestions are:

1. Consider deduplicating the type definitions
2. Add integration tests for the destination option
3. Optionally add path traversal validation

The PR is in good shape and ready to merge after addressing the duplicate type definitions.