Skip to content

Conversation

@graphite-app
Copy link
Contributor

@graphite-app graphite-app bot commented Nov 21, 2025

This draft PR was created by the Graphite merge queue.
Trunk will be fast forwarded to the HEAD of this PR when CI passes, and the original PRs will be closed.

The following PRs are included in this draft PR:

@vercel
Copy link

vercel bot commented Nov 21, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Nov 21, 2025 5:26am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 21, 2025 5:26am
rivet-inspector Ignored Ignored Preview Nov 21, 2025 5:26am
rivet-site Ignored Ignored Preview Nov 21, 2025 5:26am

@claude
Copy link

claude bot commented Nov 21, 2025

Pull Request Review

This merge queue PR combines 4 individual PRs. Here's my review of each change:


PR #3516: OpenSSL Version Update (Docker)

File: engine/docker/universal/Dockerfile

Changes:

  • Upgrades base images from Debian 12.9 (bookworm) to Debian 13.1 (trixie)
  • Updates Rust builder from 1.88.0 to 1.91.0
  • This addresses a TLS panic issue related to OpenSSL version (3.0.17 -> 3.5.1)

Assessment:Approved

  • Security fix addressing TLS panic - critical for production stability
  • Debian version upgrade is reasonable and follows stable release pattern
  • Rust version bump (1.88 -> 1.91) is appropriate

Recommendations:

  • Ensure thorough testing of TLS connections after deployment
  • Consider documenting the specific TLS panic issue in a comment or issue reference

PR #3517: RivetKit FileSystemDriver Custom Path

Files:

  • rivetkit-typescript/packages/rivetkit/src/drivers/file-system/global-state.ts
  • rivetkit-typescript/packages/rivetkit/src/drivers/file-system/utils.ts
  • rivetkit-typescript/packages/rivetkit/src/mod.ts

Changes:

  • Refactored getStoragePath() to no longer accept a custom path parameter
  • Updated FileSystemGlobalState constructor to directly use customPath if provided
  • Re-enabled exports for createFileSystemDriver and createMemoryDriver

Assessment: ⚠️ Approved with concerns

Issues Found:

  1. Typo in PR title: "speciying" should be "specifying"

  2. Logic change may break existing behavior:

    • Before: getStoragePath(customPath) would hash the custom path and use it as storage location
    • After: customPath is used directly without hashing

    This is a breaking change in behavior. If customPath was previously being hashed to create a unique storage location, it now bypasses that entirely. This could lead to:

    • Path collision issues if multiple instances use the same customPath
    • Loss of the isolation that hashing provided
    • Potential data corruption if instances overwrite each other's data
  3. Comment mismatch: The comment in utils.ts still says "Get the storage path for the current working directory or a specified path" but the function no longer accepts a parameter.

Recommendations:

  • Clarify the intended behavior: Should customPath be hashed or used directly?
  • If direct usage is intended, update the documentation to explain why hashing was removed
  • Add tests to verify the custom path behavior works as expected
  • Update the comment in utils.ts:51 to reflect the new signature

PR #3518: Build Metadata Refactoring

Files:

  • engine/packages/util/src/build_meta.rs (new)
  • engine/packages/util/build.rs (new)
  • engine/packages/api-public/build.rs
  • engine/packages/api-public/src/metadata.rs
  • engine/packages/engine/src/main.rs

Changes:

  • Centralized build metadata generation in rivet_util::build_meta module
  • Removed duplicate vergen build dependencies from api-public
  • Added --version long format with detailed build information
  • Added once_cell dependency for lazy static initialization

Assessment:Approved - Excellent refactoring

Strengths:

  • Follows DRY principle by centralizing build metadata
  • Reduces dependency duplication
  • Improves CLI UX with detailed version output
  • Clean module structure with good documentation
  • Proper use of once_cell::Lazy for static initialization

Minor suggestions:

  1. Code style consistency (engine/packages/engine/src/main.rs:23):

    • Consider using &LONG_VERSION instead of LONG_VERSION.as_str() for consistency
    • Both work, but dereferencing is more idiomatic with Lazy<String>
  2. Documentation: The build_meta module is well-documented, but consider adding a module-level doc comment explaining its purpose

Code quality: Excellent adherence to project conventions from CLAUDE.md:

  • Uses workspace dependencies correctly
  • Proper error handling with anyhow
  • Good documentation comments
  • Follows naming conventions

PR #3519: Disable Biome in Lefthook

File: lefthook.yml

Changes:

  • Commented out biome-fmt pre-commit hook
  • Added TODO comment to re-enable after fixes

Assessment:Approved

Observations:

  • Aligns with CLAUDE.md instruction: "Do not run ./scripts/cargo/fix.sh. Do not format the code yourself."
  • Temporary workaround with clear TODO for re-enabling
  • cargo fmt hook remains active for Rust code

Recommendations:

  • Create a tracking issue for re-enabling biome formatting
  • Link the issue in the TODO comment for better tracking

Overall Assessment

Summary: 3 approved, 1 approved with concerns

Critical Issues:

  1. PR chore(rivetkit): allow speciying custom path for createFIleSystemDriver #3517 (RivetKit): Potential breaking change in storage path behavior needs clarification

Security Considerations:

Performance Considerations:

Test Coverage:

Code Quality:

Action Items:

  1. Verify the intended behavior of customPath in PR chore(rivetkit): allow speciying custom path for createFIleSystemDriver #3517
  2. Add tests for custom path functionality
  3. Fix typo in PR chore(rivetkit): allow speciying custom path for createFIleSystemDriver #3517 title
  4. Update documentation/comments for changed functions
  5. Create tracking issue for re-enabling biome

Recommendation: Request clarification on PR #3517 behavior before merging. Other changes look good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants