Add S3FS prefix validation and source building functions #335
Conversation
🦋 Changeset detectedLatest commit: 13aebd9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Remove patch version for @repo/shared and add S3FS prefix validation.
Add documentation for the new prefix option in mountBucket() that allows mounting specific subdirectories within S3-compatible buckets. Changes: - Add prefix field to MountBucketOptions type documentation - Add example of mounting bucket subdirectory in API reference - Add dedicated section in mount-buckets guide with multiple examples - Document prefix format requirements (must start and end with /) Related: cloudflare/sandbox-sdk#335
commit: |
🐳 Docker Images PublishedDefault: FROM cloudflare/sandbox:0.0.0-pr-335-370c309With Python: FROM cloudflare/sandbox:0.0.0-pr-335-370c309-pythonWith OpenCode: FROM cloudflare/sandbox:0.0.0-pr-335-370c309-opencodeVersion: Use the 📦 Standalone BinaryFor arbitrary Dockerfiles: COPY --from=cloudflare/sandbox:0.0.0-pr-335-370c309 /container-server/sandbox /sandbox
ENTRYPOINT ["/sandbox"]Download via GitHub CLI: gh run download 20861092124 -n sandbox-binaryExtract from Docker: docker run --rm cloudflare/sandbox:0.0.0-pr-335-370c309 cat /container-server/sandbox > sandbox && chmod +x sandbox |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
The implementation is well-structured with good separation of concerns. The core logic correctly handles the s3fs bucket:/prefix/ syntax and properly uses the plain bucket name for password files. However, there are several important issues to address:
Critical Issues
1. Empty string prefix not handled
The validation at sandbox.ts:611 only checks prefix !== undefined, so empty string "" passes through and fails validation with a confusing error. Users might provide prefix: "" thinking it means "no prefix" but get an error saying it must start with /.
Add explicit empty string check:
if (options.prefix === '') {
throw new InvalidMountConfigError(
'Prefix cannot be empty. To mount the entire bucket, omit the prefix option. ' +
'Otherwise, provide a prefix like "/path/to/data/"'
);
}2. Missing E2E test with prefix
The existing E2E test doesn't verify prefix mounting works. Without this, we don't know if s3fs actually accepts the bucket:/prefix/ syntax or if files appear in the correct location.
Need test that:
- Mounts bucket with prefix (e.g.,
/data/) - Verifies only prefix contents are visible
- Writes through mount and confirms file appears under prefix in bucket
3. Password file behavior not tested
The code correctly uses plain bucket (not s3fsSource) when creating the password file at sandbox.ts:492. This is critical but untested - a future refactor could break this and cause authentication failures.
Error Logging Gaps
Validation errors aren't logged before throwing. When validatePrefix() fails, there's no context in logs about which mount failed or why. Add try-catch around validation with error logging including bucket, mountPath, and prefix values.
Changeset Needs Improvement
Current changeset is too technical: "Add S3FS prefix validation and source building functions"
Per CLAUDE.md, write for end users:
Mount subdirectories of S3 buckets by specifying a prefix, reducing access scope and improving security.
Use `prefix: '/path/to/data/'` in mount options.Package Lock Changes
The package-lock.json diff adds "peer": true to many dependencies. This appears unrelated to the prefix feature - was this an accidental npm install side effect? If so, consider reverting these changes to keep the PR focused.
Verdict
The core implementation is solid, but needs:
- Empty string validation
- E2E test with actual prefix
- Test verifying password file uses plain bucket name
- Better changeset description
Fix these and it's ready to merge.
| * @param prefix - The prefix to validate | ||
| * @throws InvalidMountConfigError if prefix format is invalid | ||
| */ | ||
| export function validatePrefix(prefix: string): void { |
There was a problem hiding this comment.
Add explicit check for empty string before the startsWith check. Users might provide prefix: "" expecting it to mean "no prefix", but it will fail validation with a confusing error message. Suggest:
if (prefix === '') {
throw new InvalidMountConfigError(
'Prefix cannot be empty. To mount the entire bucket, omit the prefix option.'
);
}| try { | ||
| // Create password file with credentials | ||
| // Create password file with credentials (uses bucket name only, not prefix) | ||
| await this.createPasswordFile(passwordFilePath, bucket, credentials); |
There was a problem hiding this comment.
Good - this correctly uses the plain bucket name (not s3fsSource with prefix) for the password file. However, this critical behavior isn't tested. Add a unit test verifying the password file contains the plain bucket name when a prefix is used.
.changeset/stupid-beans-relate.md
Outdated
| "@cloudflare/sandbox": patch | ||
| --- | ||
|
|
||
| Add S3FS prefix validation and source building functions |
There was a problem hiding this comment.
Per CLAUDE.md, changeset descriptions should be user-focused, not technical. This appears in GitHub releases.
Suggest:
Mount subdirectories of S3 buckets by specifying a prefix, reducing access scope and improving security.
Use `prefix: '/path/to/data/'` in mount options.
Prefix no longer requires trailing slash - s3fs accepts both forms. Bucket names are now validated with a helpful error when users accidentally use s3fs inline syntax (bucket:/path).
Empty string prefix now mounts the whole bucket instead of erroring.
2 participants
Add prefix option to mountBucket for mounting S3 bucket subdirectories.
Closes #334
Usage
Changes