Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 20, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 20, 2025 11:36pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 20, 2025 11:36pm
rivet-inspector Ignored Ignored Preview Nov 20, 2025 11:36pm
rivet-site Ignored Ignored Preview Nov 20, 2025 11:36pm

Copy link
Member Author

NathanFlurry commented Nov 20, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Nov 20, 2025

PR Review: chore(rivetkit): fix lockfile

Summary

This PR cleans up the pnpm-lock.yaml by removing unused Biome dependencies and updating transitive dependency resolutions. Net reduction of 72 lines (104 deletions, 32 additions).

Code Quality ✅

  • Clean lockfile maintenance: Properly removes unused @biomejs/[email protected] and all platform-specific binaries
  • Follows conventions: Adheres to the repository's use of pnpm for package management as specified in CLAUDE.md

Changes Analysis

Removed Dependencies:

Updated Resolutions:

  • Various transitive dependency resolutions updated (vite-tsconfig-paths, @next/third-parties, etc.)
  • No breaking changes in the updated resolutions

Security ✅

No security concerns. This is a lockfile cleanup that:

  • Removes unused dependencies (reduces attack surface)
  • Updates to compatible versions within semver ranges

Performance ✅

  • Positive impact: Smaller lockfile and fewer unused dependencies
  • No performance regressions expected from the transitive updates

Testing Considerations ✅

Recommendation: Run the following to verify the lockfile is consistent:

pnpm install --frozen-lockfile
pnpm build
pnpm test

This should be validated in CI to ensure:

  • All packages still resolve correctly
  • No build failures introduced by dependency updates
  • Tests continue to pass

Potential Issues ⚠️

Minor consideration: The diff shows some transitive dependency chain updates (e.g., eslint-plugin-import, vite versions). While these appear to be within compatible semver ranges, it would be good to verify:

  1. CI passes all checks
  2. No unexpected runtime behavior changes

Verdict: ✅ LGTM

This is a straightforward lockfile cleanup that:

  • Removes genuinely unused dependencies
  • Maintains dependency consistency
  • Follows pnpm best practices
  • Reduces lockfile size

Recommendation: Approve pending successful CI runs.

@claude
Copy link

claude bot commented Nov 20, 2025

Code Review for PR #3503: chore(rivetkit): fix lockfile

Summary

This PR updates the TypeScript test runner to accommodate API changes in the RivetKit engine runner, specifically around WebSocket hibernation and gateway/request ID handling. It also includes a lockfile update that removes Biome 2.3.6 and related dependencies.

Code Quality & Best Practices

Positive:

  • Parameter naming follows convention with underscore prefix for unused parameters (_gatewayId, _requestId)
  • The code maintains backward compatibility by keeping the existing WebSocket functionality working

Concerns:

  1. Incomplete Implementation (Critical) - engine/sdks/typescript/test-runner/src/index.ts:263

    // TODO:
    hibernatableWebSocket: undefined as any,
    • Using undefined as any is a type safety bypass that could lead to runtime errors
    • The commented-out getActorHibernationConfig function (lines 264-270) suggests this feature is partially implemented
    • Recommendation: Either complete the implementation or add a clear explanation of when this will be addressed. If this is a breaking change that needs coordination, document it in the PR description.
  2. Method Name Change Without Documentation - engine/sdks/typescript/test-runner/src/index.ts:244

    runner.sendHibernatableWebSocketMessageAck(
    • Method renamed from sendWebsocketMessageAck to sendHibernatableWebSocketMessageAck
    • No explanation in PR description about why this change was necessary
    • Recommendation: Add a brief note in the PR description about the API changes from the engine runner
  3. Signature Mismatch Risk - The commented-out function signature change:

    // Old: getActorHibernationConfig(actorId, requestId)
    // New: getActorHibernationConfig(actorId, gatewayId, requestId)
    • The new gatewayId parameter is being passed but not used
    • Recommendation: Clarify if this is temporary scaffolding for an upcoming feature or if it can be removed

Potential Bugs

  1. Runtime Failure Risk - Setting hibernatableWebSocket: undefined as any means any code path that tries to use this property will fail at runtime. Verify that:
    • The engine runner doesn't call this property in the current version
    • There are tests that would catch this if it becomes an issue
    • Action Required: Add a note about test coverage for this scenario

Security Concerns

No security issues identified. The changes are internal test infrastructure updates.

Performance Considerations

No performance impact expected. The additional gatewayId parameter is a straightforward pass-through.

Test Coverage

Critical Gap:

  • No tests appear to be included or modified in this PR
  • The test runner itself is being modified, but there's no evidence of validation
  • Recommendation:
    • Run the existing test suite to verify the changes don't break anything
    • Add a comment confirming that tests pass
    • Consider adding a test specifically for the WebSocket message acknowledgment flow

Lockfile Changes

The pnpm-lock.yaml changes show:

  • Removal of Biome 2.3.6 dependencies (down to 2.2.3)
  • Various dependency resolution updates
  • Net reduction of 67 lines

Concerns:

  • The PR title says "fix lockfile" but doesn't explain what was broken
  • Recommendation: Add a note about what issue this lockfile change addresses

Questions for Author

  1. Is the hibernatableWebSocket: undefined temporary? When will it be implemented?
  2. What breaks if we remove the commented-out getActorHibernationConfig function entirely?
  3. Have you verified that the test runner still works with these changes?
  4. What was the specific lockfile issue that needed fixing?

Overall Assessment

Approval Status: ⚠️ Needs clarification before merging

While the code changes appear syntactically correct, the incomplete hibernatableWebSocket implementation and lack of documentation about what changed and why raises concerns. This looks like it might be part of a larger refactoring effort that's in progress.

Recommended Actions Before Merge:

  1. Add PR description explaining the context (API changes in engine runner)
  2. Clarify the status of the hibernatable WebSocket feature
  3. Confirm tests pass
  4. Either complete the implementation or add a tracking issue for the TODO

Review performed following Rivet CLAUDE.md conventions

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 20, 2025

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3503

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3503

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3503

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3503

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3503

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3503

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3503

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3503

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3503

commit: 6557c65

@NathanFlurry NathanFlurry force-pushed the 11-20-chore_rivetkit_make_action_cancelled_error_more_descriptive branch from db9062e to bd44aa4 Compare November 20, 2025 23:35
@NathanFlurry NathanFlurry force-pushed the 11-20-chore_rivetkit_fix_lockfile branch from 57ae58f to 6557c65 Compare November 20, 2025 23:35
@claude
Copy link

claude bot commented Nov 20, 2025

PR Review

Summary

This PR updates the test runner to support hibernatable WebSocket functionality by adding gateway ID parameters and updating method signatures. The changes also include a pnpm lockfile update.

Code Quality & Best Practices

Positive:

  • Clean parameter naming with underscore prefix for unused parameters (_gatewayId, _requestId)
  • Consistent application of changes across both fetch and websocket handlers

Concerns:

  1. Incomplete Implementation (High Priority) - engine/sdks/typescript/test-runner/src/index.ts:263

    • The hibernatableWebSocket is set to undefined as any, which is a type-unsafe workaround
    • The getActorHibernationConfig function is commented out rather than properly implemented or removed
    • This creates technical debt and leaves the feature incomplete
    • Recommendation: Either implement the feature properly or create a follow-up issue to track this work. The TODO comment should reference a specific issue number.
  2. Naming Inconsistency - engine/sdks/typescript/test-runner/src/index.ts:264-270

    • The commented code still references the old parameter order without gatewayId
    • If/when uncommented, this will cause runtime errors
    • Recommendation: Update the commented code to match the new signature, or remove it entirely if it's not going to be used.
  3. Missing Context in Comments - engine/sdks/typescript/test-runner/src/index.ts:262

    • The TODO comment provides no context about what needs to be done
    • Recommendation: Add context like TODO: Implement hibernatable WebSocket support - see issue #XXXX

Potential Bugs

  1. Type Safety Issue - engine/sdks/typescript/test-runner/src/index.ts:263

    • Using undefined as any defeats TypeScript's type checking
    • This could cause runtime errors if the hibernatableWebSocket is accessed without null checks
    • Recommendation: Use proper optional typing or implement a stub that throws a meaningful error
  2. Parameter Mismatch - engine/sdks/typescript/test-runner/src/index.ts:264-270

    • If the commented getActorHibernationConfig is ever uncommented, it won't match the expected signature (missing gatewayId parameter)
    • Recommendation: Either fix the signature or remove the dead code

Performance Considerations

  • No performance concerns identified
  • The additional gatewayId parameter is just passed through without processing

Security Concerns

  • No security issues identified
  • Parameters are appropriately prefixed with underscore when unused

Test Coverage

  • Missing: No tests verify the new gatewayId parameter handling
  • Missing: No tests for the incomplete hibernation feature
  • Recommendation: Add integration tests that verify:
    • The new parameters are correctly passed through
    • WebSocket message acknowledgments work with the gateway ID
    • Document the incomplete hibernation feature's test requirements

Lockfile Changes

The pnpm-lock.yaml changes appear to be legitimate dependency updates:

  • Biome CLI packages removed (version 2.3.6)
  • Vite and related dependencies updated
  • No suspicious changes detected

Action Items

Before Merge:

  1. Replace undefined as any with a proper implementation or typed stub
  2. Either fix or remove the commented getActorHibernationConfig code
  3. Add a meaningful TODO comment with issue reference

Follow-up:

  1. Create an issue to track hibernatable WebSocket implementation
  2. Add test coverage for the new parameters
  3. Implement or remove the hibernation configuration feature

Overall Assessment

The PR correctly updates method signatures to support the new gateway ID parameter, but leaves the hibernation feature in an incomplete state with type-unsafe workarounds. This should be addressed before merging to maintain code quality standards.

@claude
Copy link

claude bot commented Nov 20, 2025

PR Review: chore(rivetkit): fix lockfile

Summary

This PR updates the TypeScript test-runner to align with API changes for hibernatable WebSocket support, specifically adding the gatewayId parameter to fetch and websocket handlers. The changes also include pnpm lockfile updates.

Code Quality & Best Practices

Positive:

  • ✅ The changes correctly add the _gatewayId parameter to match the updated interface signatures
  • ✅ Proper use of underscore prefix for unused parameters (_gatewayId, _requestId)
  • ✅ Updated method call from sendWebsocketMessageAck to sendHibernatableWebSocketMessageAck with correct parameters

Concerns:

  1. Incomplete Implementation ⚠️

    • The hibernatableWebSocket config is set to undefined as any (line 263), which is a type system bypass and suggests incomplete work
    • The getActorHibernationConfig function is commented out (lines 264-270), but the websocket message handling still references websocketLastMsgIndexes
    • This creates a partial implementation where message acknowledgment works but hibernation config doesn't
  2. Inconsistent State ⚠️

    • websocketLastMsgIndexes Map is still being updated (lines 237-243) but the hibernation config that would consume this data is commented out
    • This dead code should either be removed or the hibernation config should be properly implemented
  3. TODO Without Context ⚠️

    • Line 262 has // TODO: without explaining what needs to be done
    • Based on the interface at engine/sdks/typescript/runner/src/mod.ts:138-149, the hibernatableWebSocket.canHibernate method needs implementation

Recommendations

  1. Complete the implementation or clean up:

    // Either implement properly:
    hibernatableWebSocket: {
      canHibernate: (actorId, gatewayId, requestId, request) => {
        // Add logic to determine if websocket can hibernate
        return true; // or false based on criteria
      }
    },
    
    // Or if not needed for tests, remove websocketLastMsgIndexes tracking entirely
  2. Add descriptive TODO:

    // TODO: Implement hibernatableWebSocket.canHibernate for test scenarios
    // or remove if hibernation testing is not required
  3. Consider test coverage:

    • Are there tests that validate the new gatewayId parameter is properly passed through?
    • Should hibernatable WebSocket functionality be tested?

Lockfile Changes

The pnpm-lock.yaml changes appear to be dependency resolution updates (removal of @biomejs 2.3.6, vite version consolidation). These look reasonable but are substantial (-112/+45 lines). Ensure:

  • All packages still build correctly
  • No breaking changes in updated dependencies
  • CI passes all checks

Security & Performance

  • ✅ No security concerns identified
  • ✅ No performance concerns
  • ✅ Proper use of existing patterns (Buffer.from().toString('base64'))

Overall Assessment

The changes correctly address API signature updates but leave the implementation in an incomplete state. This is acceptable for a chore commit that's fixing a lockfile/API mismatch, but should be tracked for completion. The as any type assertion and commented-out code suggest this is a work-in-progress that unblocks development.

Recommendation: Approve with follow-up issue to complete hibernatable WebSocket implementation in test-runner.

@claude
Copy link

claude bot commented Nov 20, 2025

PR Review: chore(rivetkit): fix lockfile

Summary

This PR updates the test runner to handle changes in the RivetKit API and fixes the pnpm lockfile. The changes include adding new parameters to the fetch and websocket callbacks, updating WebSocket message acknowledgment methods, and temporarily stubbing out hibernation configuration.


Code Quality & Best Practices ✅

Strengths:

  • Clear parameter naming with underscore prefix for unused parameters (_gatewayId, _requestId)
  • Maintains existing functionality while adapting to API changes
  • Good use of structured logging with getLogger() (lowercase messages as per CLAUDE.md)

Concerns:

  1. TODO comment without context (engine/sdks/typescript/test-runner/src/index.ts:262-270)

    • The hibernatableWebSocket is set to undefined as any with a TODO comment
    • The getActorHibernationConfig function is commented out but shows the new signature with gatewayId parameter
    • Recommendation: Add a GitHub issue reference or more context about when this will be implemented
    • Impact: This appears to disable hibernation functionality temporarily - is this intentional?
  2. Type safety bypass (engine/sdks/typescript/test-runner/src/index.ts:263)

    • Using undefined as any bypasses TypeScript's type checking
    • Recommendation: Consider using a proper type stub or optional property if this is temporary

Potential Bugs 🔍

  1. Incomplete API migration

    • The commented-out getActorHibernationConfig suggests this feature was working before
    • Setting hibernatableWebSocket: undefined as any might cause runtime errors if the Runner expects this property
    • Action needed: Verify that the Runner gracefully handles undefined for hibernatableWebSocket
  2. Parameter changes without validation

    • New _gatewayId parameter added to both fetch and websocket callbacks
    • These are prefixed with underscore indicating they're unused
    • Question: Should these parameters be logged or validated, or are they truly unnecessary for the test runner?

Security Concerns 🔒

No significant security issues identified. The changes are primarily parameter additions and don't introduce new attack vectors.


Performance Considerations ⚡

Positive:

  • No performance regressions expected
  • The lockfile changes remove duplicate/unused dependencies

Neutral:

  • Additional parameters in callbacks have minimal overhead since they're unused

Test Coverage 🧪

Missing:

  • No test changes included
  • The commented-out hibernation config suggests feature regression
  • Recommendation: Add tests to verify:
    1. WebSocket message acknowledgment still works with new signature
    2. Hibernation behavior when hibernatableWebSocket is undefined
    3. Fetch and websocket handlers work correctly with new gateway ID parameter

Lockfile Changes 📦

The pnpm-lock.yaml changes show:

  • Removal of @biomejs/[email protected] and its platform-specific binaries
  • Dependency version updates for various packages
  • Net reduction of 72 lines (good for maintenance)

Concern: The PR title mentions "fix lockfile" but doesn't explain what was broken. Consider adding:

  1. What issue the lockfile fix addresses
  2. Whether this was caused by a dependency conflict or version mismatch

Recommendations

High Priority:

  1. Clarify TODO intent: Either implement hibernatableWebSocket or document why it's temporarily disabled and when it will be re-enabled
  2. Add tests: Cover the API changes, especially the new parameters and hibernation behavior
  3. Document lockfile fix: Explain what was broken in the PR description

Medium Priority:

  1. Type safety: Replace undefined as any with proper typing
  2. Parameter usage: Document why _gatewayId parameters are unused - is this for future use?

Low Priority:

  1. Consider extracting WebSocket ID generation into a helper function (repeated at lines 237-239)

Verdict

Conditional Approval ⚠️ - The code changes are reasonable API adaptations, but the incomplete hibernation implementation and lack of context around the lockfile fix raise concerns. Please address the high-priority recommendations before merging.

Questions for Author:

  1. Is the hibernation feature intentionally disabled? If so, why?
  2. What specific lockfile issue was this fixing?
  3. Are there any breaking changes this introduces for test runner users?

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 21, 2025

Merge activity

  • Nov 21, 2:40 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 21, 2:42 AM UTC: CI is running for this pull request on a draft pull request (#3515) due to your merge queue CI optimization settings.
  • Nov 21, 2:43 AM UTC: Merged by the Graphite merge queue via draft PR: #3515.

graphite-app bot pushed a commit that referenced this pull request Nov 21, 2025
@graphite-app graphite-app bot closed this Nov 21, 2025
@graphite-app graphite-app bot deleted the 11-20-chore_rivetkit_fix_lockfile branch November 21, 2025 02:43
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