Skip to content

Conversation

MasterPtato
Copy link
Contributor

No description provided.

Copy link

vercel bot commented Sep 30, 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 Oct 1, 2025 4:41am

Copy link

claude bot commented Sep 30, 2025

Claude finished @MasterPtato's task —— View job


PR Review: fix: make serverless use bare

Todo List:

  • Read CLAUDE.md for project guidelines
  • Analyze changed files to understand modifications
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage implications
  • Provide constructive feedback

Review Summary

This PR makes two main changes:

  1. Updates @rivetkit/engine-runner dependency from commit f594631 to 03e3532 (version 25.7.1 → 25.7.2)
  2. Refactors serverless initialization in packages/rivetkit/src/drivers/engine/actor-driver.ts:382-387 to use getServerlessInitPacket() instead of accessing runnerId directly

Code Quality & Best Practices ✅

Positive aspects:

  • Good encapsulation: The change moves from direct property access (this.#runner.runnerId) to a proper method call (this.#runner.getServerlessInitPacket())
  • Follows the project's naming conventions (camelCase for methods)
  • Maintains existing error handling pattern with invariant()
  • Preserves the same SSE streaming behavior

Areas for improvement:

  • The error message "runnerId not set" in the invariant is now misleading since we're checking for a payload packet, not just the runner ID
  • Consider updating the error message to be more descriptive: "serverless init packet not available"

Potential Bugs & Issues ⚠️

Low risk observations:

  • The change assumes getServerlessInitPacket() returns the same data format that was previously just the runnerId string. This should be verified in the engine-runner dependency
  • The invariant error message doesn't match what's being validated anymore

Performance Considerations ✅

Positive impact:

  • Method call vs. property access has negligible performance difference
  • The new approach likely provides more structured data, which could be beneficial for future extensibility

Security Concerns ✅

No security issues identified:

  • The change maintains the same access patterns and doesn't introduce new attack vectors
  • The dependency update follows proper package versioning practices using pkg.pr.new

Test Coverage Implications ⚠️

Considerations:

  • Tests that mock or assert on the runnerId property may need updates
  • The serverless initialization behavior should be tested to ensure the new packet format works correctly
  • No test files were modified in this PR, so existing tests should be reviewed for compatibility

Dependencies & Package Management ✅

Good practices:

  • Uses pkg.pr.new for pre-release dependency management
  • Dependency version bump is minimal (patch-level change)
  • Lock file properly updated

Recommendations

  1. Update error message in the invariant to be more accurate:

    invariant(payload, "serverless init packet not available");
  2. Add JSDoc comment to clarify the new behavior:

    // Get serverless initialization packet containing runner metadata
    const payload = this.#runner.getServerlessInitPacket();
  3. Verify compatibility with any existing tests that might depend on the previous runnerId format

  4. Consider adding type safety by defining the expected structure of the payload if not already done in the engine-runner package

Overall Assessment

This is a well-structured refactoring that improves the abstraction layer between the actor driver and the engine runner. The change follows good engineering practices by replacing direct property access with a proper method interface. The dependency update appears to be a coordinated change to support this new API.

Approval recommended with the minor suggestion to update the error message for clarity.


Copy link
Contributor Author

MasterPtato commented Sep 30, 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 either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

Copy link

pkg-pr-new bot commented Sep 30, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

commit: de19ac5

@MasterPtato MasterPtato force-pushed the 09-26-chore_bump_engine_runner branch from 74198be to caa2ed2 Compare September 30, 2025 21:09
@MasterPtato MasterPtato force-pushed the 09-30-fix_make_serverless_use_bare branch from ef8c5b1 to de19ac5 Compare September 30, 2025 21:09
Copy link

claude bot commented Sep 30, 2025

Claude finished @MasterPtato's task —— View job


Claude finished @MasterPtato's task —— View job


PR Review: fix: make serverless use bare

Todo List:

  • Read CLAUDE.md for project guidelines
  • Analyze changed files to understand modifications
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage implications
  • Provide constructive feedback

Review Summary

This PR makes two main changes:

  1. Updates @rivetkit/engine-runner dependency from commit f594631 to 03e3532 (packages/rivetkit/package.json:166)
  2. Refactors serverless initialization in packages/rivetkit/src/drivers/engine/actor-driver.ts:382-387 to use getServerlessInitPacket() instead of accessing runnerId directly

Code Quality & Best Practices ✅

Positive aspects:

  • Good encapsulation: The change moves from direct property access (this.#runner.runnerId) to a proper method call (this.#runner.getServerlessInitPacket())
  • Follows the project's naming conventions (camelCase for methods)
  • Maintains existing error handling pattern with invariant()
  • Preserves the same SSE streaming behavior
  • Uses proper private field syntax (#runner) as per CLAUDE.md guidelines

Areas for improvement:

  • The error message "runnerId not set" in the invariant is now misleading since we're checking for a payload packet, not just the runner ID
  • Consider updating the error message to be more descriptive: "serverless init packet not available"

Potential Bugs & Issues ⚠️

Low risk observations:

  • The change assumes getServerlessInitPacket() returns the same data format that was previously just the runnerId string. This should be verified in the engine-runner dependency
  • The invariant error message doesn't match what's being validated anymore

Performance Considerations ✅

Positive impact:

  • Method call vs. property access has negligible performance difference
  • The new approach likely provides more structured data, which could be beneficial for future extensibility

Security Concerns ✅

No security issues identified:

  • The change maintains the same access patterns and doesn't introduce new attack vectors
  • The dependency update follows proper package versioning practices using pkg.pr.new

Test Coverage Implications ⚠️

Considerations:

  • Tests that mock or assert on the runnerId property may need updates
  • The serverless initialization behavior should be tested to ensure the new packet format works correctly
  • No test files were modified in this PR, so existing tests should be reviewed for compatibility

Dependencies & Package Management ✅

Good practices:

  • Uses pkg.pr.new for pre-release dependency management
  • Dependency version bump is minimal (patch-level change)
  • Lock file properly updated

Recommendations

  1. Update error message in the invariant to be more accurate:

    invariant(payload, "serverless init packet not available");
  2. Add JSDoc comment to clarify the new behavior:

    // Get serverless initialization packet containing runner metadata
    const payload = this.#runner.getServerlessInitPacket();
  3. Verify compatibility with any existing tests that might depend on the previous runnerId format

  4. Consider adding type safety by defining the expected structure of the payload if not already done in the engine-runner package

Overall Assessment

This is a well-structured refactoring that improves the abstraction layer between the actor driver and the engine runner. The change follows good engineering practices by replacing direct property access with a proper method interface. The dependency update appears to be a coordinated change to support this new API.

Approval recommended with the minor suggestion to update the error message for clarity.


Copy link

claude bot commented Sep 30, 2025

Claude finished @MasterPtato's task —— View job


PR Review: fix: make serverless use bare

Todo List:

  • Read CLAUDE.md for project guidelines
  • Analyze changed files to understand modifications
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage implications
  • Provide constructive feedback

Review Summary

This PR makes two coordinated changes:

  1. Updates @rivetkit/engine-runner dependency from commit f594631 to 03e3532
  2. Refactors serverless initialization in packages/rivetkit/src/drivers/engine/actor-driver.ts:382-387 to use getServerlessInitPacket() instead of accessing runnerId directly

Code Quality & Best Practices ✅

Positive aspects:

  • Good encapsulation: The change moves from direct property access (this.#runner.runnerId) to a proper method call (this.#runner.getServerlessInitPacket())
  • Follows project conventions: Uses camelCase naming as specified in CLAUDE.md
  • Maintains existing error handling: Preserves the invariant() pattern for validation
  • Preserves SSE behavior: The streaming functionality remains unchanged
  • Proper private member usage: Continues using # prefix for private members as per project guidelines

Areas for improvement:

  • Misleading error message: The invariant message "runnerId not set" no longer accurately describes what's being validated since we're now checking for a payload packet
  • Missing JSDoc: Could benefit from a comment explaining the new behavior

Potential Bugs & Issues ⚠️

Medium-risk observations:

  1. Error message mismatch: The invariant error "runnerId not set" doesn't match what's actually being validated (payload)
  2. Data format assumptions: The code assumes getServerlessInitPacket() returns data that can be directly passed to stream.writeSSE({ data: payload }). This should be compatible but needs verification that the packet structure is correct
  3. Missing type information: Without seeing the return type of getServerlessInitPacket(), it's unclear if the payload structure matches what clients expect

Low-risk observations:

  • The dependency update from f594631 to 03e3532 appears coordinated with the API change

Performance Considerations ✅

Neutral impact:

  • Method call vs. property access has negligible performance difference
  • The new approach likely provides more structured data which could be beneficial for future extensibility
  • SSE streaming behavior is preserved, so no performance regression expected

Security Concerns ✅

No security issues identified:

  • The change maintains the same data access patterns
  • No new attack vectors introduced
  • Dependency update follows proper package versioning practices using pkg.pr.new
  • The invariant check still validates that required data is present before streaming

Test Coverage Implications ⚠️

Potential testing concerns:

  • No test changes included: Tests that mock or assert on the runnerId property may need updates
  • Integration testing needed: The serverless initialization behavior should be tested to ensure the new packet format works correctly with client expectations
  • Missing test coverage: No existing tests found for serverlessHandleStart method, which makes it difficult to validate the change

Dependencies & Package Management ✅

Good practices:

  • Uses pkg.pr.new for pre-release dependency management (following project patterns)
  • Lock file properly updated with new dependency versions
  • Dependency version appears to be a coordinated change supporting the new API

Recommendations

  1. Fix error message to accurately reflect what's being validated:

    invariant(payload, "serverless init packet not available");
  2. Add documentation for the behavior change:

    // Get serverless initialization packet containing runner metadata
    const payload = this.#runner.getServerlessInitPacket();
  3. Verify client compatibility: Ensure clients expecting the previous runnerId string format can handle the new packet structure

  4. Add integration tests: Consider adding tests for the serverlessHandleStart method to prevent future regressions

  5. Type safety: If not already done in the engine-runner package, consider adding TypeScript definitions for the expected packet structure

Overall Assessment

This is a well-structured refactoring that improves the API design by:

  • Moving from direct property access to a proper method interface
  • Following good encapsulation principles
  • Maintaining backward compatibility in behavior

The change appears to be part of a coordinated effort to use "bare" protocols (as indicated by the PR title), which suggests this is moving toward a more structured data format.

Approval recommended with the suggestion to update the error message for accuracy. The change follows project conventions and improves the abstraction layer between components.

Risk Level: Low - The change is well-contained and follows good engineering practices, though the error message should be updated for clarity.


Copy link

graphite-app bot commented Oct 1, 2025

Merge activity

  • Oct 1, 4:30 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Oct 1, 4:31 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #1316.
  • Oct 1, 4:31 AM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #1316.
  • Oct 1, 4:39 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Oct 1, 4:40 AM UTC: CI is running for this pull request on a draft pull request (#1322) due to your merge queue CI optimization settings.
  • Oct 1, 4:41 AM UTC: Merged by the Graphite merge queue via draft PR: #1322.

@graphite-app graphite-app bot force-pushed the 09-26-chore_bump_engine_runner branch 2 times, most recently from 2979376 to 31e38ca Compare October 1, 2025 04:33
@NathanFlurry NathanFlurry force-pushed the 09-30-fix_make_serverless_use_bare branch from de19ac5 to 6d1f0f5 Compare October 1, 2025 04:39
Copy link

claude bot commented Oct 1, 2025

Claude finished @MasterPtato's task —— View job


PR Review: fix: make serverless use bare

Todo List:

  • Read CLAUDE.md for project guidelines
  • Analyze changed files to understand modifications
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage implications
  • Provide constructive feedback

Review Summary

This PR makes two coordinated changes to support "bare" protocol usage in serverless initialization:

  1. Updates @rivetkit/engine-runner dependency from commit f594631 to 03e3532 in packages/rivetkit/package.json:166
  2. Refactors serverless initialization in packages/rivetkit/src/drivers/engine/actor-driver.ts:414-416 to use getServerlessInitPacket() instead of accessing runnerId directly

Code Quality & Best Practices ✅

Positive aspects:

  • Good encapsulation: Changes from direct property access (this.#runner.runnerId) to a proper method call (this.#runner.getServerlessInitPacket())
  • Follows project conventions: Uses camelCase naming and private field syntax (#runner) as specified in CLAUDE.md:95
  • Maintains error handling: Preserves the invariant() validation pattern
  • Preserves SSE streaming: The Server-Sent Events functionality remains unchanged

Areas for improvement:

  • Misleading error message: The invariant message "runnerId not set" no longer accurately describes what's being validated since we're now checking for a payload packet instead of just a runner ID

Potential Bugs & Issues ⚠️

Medium-risk observations:

  1. Error message mismatch: The invariant error message doesn't match the validation logic anymore
  2. Data format assumptions: The code assumes getServerlessInitPacket() returns data that's compatible with stream.writeSSE({ data: payload }). From other SSE usages in the codebase, this expects either a string or JSON-serializable data
  3. Backward compatibility: Need to verify clients expecting the previous runnerId string format can handle the new packet structure

Low-risk observations:

  • The dependency update appears coordinated with the API change, suggesting this is an intentional refactoring

Performance Considerations ✅

Neutral to positive impact:

  • Method call vs. property access has negligible performance difference
  • The new approach likely provides more structured data which could be beneficial for future extensibility
  • SSE streaming behavior is preserved, so no performance regression expected

Security Concerns ✅

No security issues identified:

  • The change maintains the same data access patterns
  • The invariant check still validates that required data is present before streaming
  • No new attack vectors introduced
  • Dependency update follows proper package versioning practices using pkg.pr.new

Test Coverage Implications ⚠️

Testing concerns:

  • No test coverage found: No tests found for serverlessHandleStart method or getServerlessInitPacket functionality
  • Missing integration tests: The serverless initialization behavior should be tested to ensure client compatibility
  • Potential test breakage: Any tests that mock or assert on the runnerId property may need updates (though none were found in the current search)

Dependencies & Package Management ✅

Good practices:

  • Uses pkg.pr.new for pre-release dependency management, following project patterns
  • Lock file properly updated with new dependency versions
  • Dependency version appears to be a coordinated change supporting the new "bare" protocol API

Recommendations

  1. Fix error message to accurately reflect what's being validated:

    invariant(payload, "serverless init packet not available");
  2. Add JSDoc documentation for the behavior change:

    // Get serverless initialization packet containing runner metadata
    const payload = this.#runner.getServerlessInitPacket();
  3. Verify client compatibility: Ensure clients expecting the previous runnerId string format can handle the new packet structure

  4. Add test coverage: Consider adding tests for the serverlessHandleStart method to prevent future regressions

  5. Type safety: If not already done in the engine-runner package, consider adding TypeScript definitions for the expected packet structure

Overall Assessment

This is a well-structured refactoring that improves the API design by:

  • Moving from direct property access to a proper method interface
  • Following good encapsulation principles
  • Supporting the "bare" protocol initiative (as indicated by the PR title)

The change follows project conventions from CLAUDE.md and maintains backward compatibility in behavior while improving the abstraction layer between components.

Approval recommended with the minor suggestion to update the error message for accuracy and clarity.

Risk Level: Low - The change is well-contained and follows good engineering practices, though the error message should be updated and test coverage would be beneficial.


graphite-app bot pushed a commit that referenced this pull request Oct 1, 2025
invariant(runnerId, "runnerId not set");
stream.writeSSE({ data: runnerId });
const payload = this.#runner.getServerlessInitPacket();
invariant(payload, "runnerId not set");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message "runnerId not set" doesn't match what's being validated in the updated code. Since the check is now for payload rather than runnerId, consider updating the error message to something like "serverless init packet not available" to accurately reflect the condition being tested.

Suggested change
invariant(payload, "runnerId not set");
invariant(payload, "serverless init packet not available");

Spotted by Diamond

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@graphite-app graphite-app bot closed this Oct 1, 2025
@graphite-app graphite-app bot deleted the 09-30-fix_make_serverless_use_bare branch October 1, 2025 04:41
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