fix(rivetkit): fix onError handler not working

[NathanFlurry]

No description provided.

[vercel]

Deployment failed with the following error:

Too many requests - try again in 3 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

[NathanFlurry]

• feat(rivetkit): add ability to destroy actors #3458 : 2 dependent PRs (#3459 , #3462 )
• feat(rivet-engine): udb key parser #3457
• fix(rivetkit): fix zod not validating body if content-type header not provided #3456
• fix(rivetkit): fix onError handler not working #3455 👈 (View in Graphite)
• chore(rivetkit): switch dynamic node imports to use require #3461 : 1 other dependent PR (#3280 )
• chore(rivetkit-typescript): remove dependency on node modules #3460
• feat(site): add typedoc docs #3446
• chore(rivetkit): remove deprecated packages #3447
• chore(rivetkit): add support for conn params on onRequest and onWebSocket #3445
• feat(rivetkit): add asyncapi spec #3442
• chore(rivetkit): add actor router to the openapi spec #3441
• fix(rivetkit): skip sending RivetKit messages to conns that do not support it #3440
• fix(rivetkit): fix race condition with websocket open events #3439
• fix(rivetkit): standardize error metadata return type #3438
• fix(rivetkit): remove incorrect getEndpoint call in metadata handler #3433 : 1 other dependent PR (#3451 )
• feat(rivetkit): add json protocol support #3432
• chore(rivetkit): add required zod validation for json encoding #3431
• chore(rivetkit): add listing actors by only name to manager api #3430
• refactor(rivetkit): extract WebSocket protocol parsing to shared utility #3429
• refactor(rivetkit): restructure connection lifecycle contexts #3428
• chore(rivetkit): add WebSocketContext and RequestContext #3427
• chore(rivetkit): add conn type #3426
• chore(rivetkit): add connection for raw http #3425
• chore(rivetkit): rename onFetch to onRequest #3424
• refactor(rivetkit): add state manager for conn with symbol-based access #3423
• fix(rivetkit): use loggerWithoutContext for websocket error when actor undefined #3412
• feat(rivetkit): add GET /actors/names #3411
• feat(api-public): expose actor kv api #3410
• chore(rivetkit): auto-generate inspector token for engine driver #3409
• chore(rivetkit): close websockets on out of sequence messages #3404
• fix(rivetkit): properly apply clientEndpoint from metadata lookup #3402
• fix(rivetkit): properly handle msgIndex for hibernatable websocket reconnection #3401
• chore(rivetkit): rename onstart/onstop to onwake/onsleep #3422
• chore(rivetkit): split ActorInstance logic in to multiple classes #3421
• chore(rivetkit): move conns to separate persisted kv keys #3420
• chore(rivetkit): remove deprecated conn tokens & sse support #3419
• main

---

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]

Code Review

Summary

This PR fixes the onError handler not working in RivetKit by simplifying the server initialization flow. The changes remove unnecessary abstraction that was preventing Hono's error handlers from functioning properly.

Changes Overview

1. serve.ts: Simplified crossPlatformServe() to accept a single app parameter instead of separate rivetKitRouter and userRouter parameters
2. mod.ts: Updated the call site to pass only the Hono router created by createManagerRouter()

Code Quality ✅

Strengths:

• The simplification is correct - the router created by createManagerRouter() already has onError and notFound handlers registered (see manager/router.ts:130-131)
• Removing the unused userRouter parameter and the conditional logic that created a new Hono instance improves clarity
• The change fixes a real bug: when a new Hono instance was created in crossPlatformServe, the error handlers from the original router were lost
• Type import change (import type { Hono }) is appropriate since Hono is only used as a type

Potential Issues 🔍

Minor:

1. Redundant object property shorthand (serve.ts:43)

   app: app,  // Could be just: app

   This is harmless but could be simplified to just app in modern JavaScript/TypeScript.

Performance Considerations ✅

No performance concerns. The change actually improves performance slightly by:

• Eliminating unnecessary Hono instance creation
• Removing dead code (commented-out routing logic)

Security Concerns ✅

No security issues identified. Error handling is properly configured on the router before being passed to this function.

Test Coverage ⚠️

Recommendation:
Consider adding a test that verifies onError handlers work correctly. This could prevent similar regressions. For example:

• Send a request that triggers an error
• Verify the error handler is invoked
• Check the error response format

Architecture Note 📝

The fix makes sense architecturally - crossPlatformServe() should be a platform adapter that takes a configured Hono app, not a router configurator. The error handling configuration belongs in createManagerRouter() and createActorRouter(), which is where it already exists.

Verdict: ✅ LGTM

This is a clean fix that correctly addresses the root cause. The previous implementation was creating a new Hono instance without the error handlers, and this PR fixes that by passing the already-configured router directly.

Optional improvement: Consider changing app: app, to app, on line 43 for consistency with modern JavaScript conventions.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Nov 13, 2025 10:33pm

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 13, 2025 10:33pm
rivet-inspector	Ignored	Preview		Nov 13, 2025 10:33pm
rivet-site	Ignored	Preview		Nov 13, 2025 10:33pm

[claude]

Code Review

Summary

This PR fixes a bug where the onError handler was not working correctly in the RivetKit server. The fix simplifies the crossPlatformServe function by removing the unused userRouter parameter and having the caller pass in the fully-configured Hono app directly.

What Changed

Before: The crossPlatformServe function accepted both a rivetKitRouter and an optional userRouter, then conditionally created a new Hono app and mounted the rivetKitRouter on it.

After: The function now accepts a single pre-configured Hono app parameter, eliminating the router composition logic.

Analysis

Positive Aspects ✅

1. Root Cause Fix: The issue was that creating a new Hono app inside crossPlatformServe meant the error handlers (router.onError) registered in createManagerRouter (router.ts:131) were being applied to a different Hono instance than the one actually serving requests. This fix ensures the same router instance with registered error handlers is passed through to the server.

2. Simplified Architecture: Removing the router composition logic makes the code cleaner and more maintainable. The responsibility for router setup now clearly lives in one place (createManagerRouter).

3. Reduced Coupling: The function no longer needs to know about mounting logic or how routers should be composed.

4. Type Improvement: Changed import from import { Hono } to import type { Hono } which is more semantically correct since it's only used for typing.

5. Consistent Pattern: This aligns with how actor routers work (see actor/router.ts:229-230), where error handlers are registered on the router that gets served.

Potential Concerns ⚠️

1. No Test Coverage: This PR doesn't include tests to verify the error handler now works correctly. Consider adding a test that:

   • Triggers an error condition
   • Verifies the error is handled by handleRouteError
   • Confirms the proper error response format

2. Breaking Change Risk: If there were any external consumers expecting to pass a userRouter as the third parameter, this would be a breaking change. However, looking at the call site (mod.ts:225), this parameter was always undefined, so this appears safe.

3. Redundant Code: Line 43 has redundant syntax: app: app, could be simplified to just app, (ES6 shorthand property).

Code Quality

The changes follow the repository patterns well:

• Consistent with error handling setup in createActorRouter (actor/router.ts:230)
• Maintains the existing cross-platform server abstraction
• Follows TypeScript best practices with type imports

Recommendations

1. Add a test to verify error handling works:

// Example test structure
test('error handler catches and formats errors correctly', async () => {
  // Setup registry that throws an error
  // Make request that triggers the error
  // Assert error response matches expected format from handleRouteError
});

2. Minor cleanup - simplify line 43:

-	app: app,
+	app,

3. Consider adding a comment in crossPlatformServe explaining why the app must be pre-configured (to preserve error handlers).

Security

No security concerns identified.

Performance

No performance impact - if anything, this is slightly more efficient by removing unnecessary router composition.

Verdict

Approve with minor suggestions ✅

This is a good fix that addresses the root cause properly. The architectural simplification is a bonus. Would be even better with test coverage to prevent regression.

[claude]

Code Review

Summary

This PR fixes an issue where the onError handler in the manager router was not being properly applied. The root cause was that crossPlatformServe was creating a new Hono instance instead of using the router that already had error handlers configured via router.onError(handleRouteError) at manager/router.ts:131.

Positive Changes

1. Correct Architecture: The fix properly ensures that the configured router (with its error handlers) is used directly, rather than being potentially bypassed or replaced.
2. Simplified API: The function signature is cleaner - from 3 parameters down to 2. This removes unnecessary complexity.
3. Removed Dead Code: The commented-out routing code has been removed, which is good housekeeping.
4. Type Import Optimization: Changed from import Hono to import type Hono, which is a minor optimization for TypeScript compilation.

Code Quality Assessment

Overall: Good - The changes are minimal, focused, and solve a specific problem.

Strengths:

• The fix is surgical and addresses the root cause
• No unnecessary changes or scope creep
• Proper TypeScript type usage

Concerns

1. Missing Test Coverage - There don't appear to be tests added to verify that onError handlers work correctly. Consider adding a test that triggers an error in a route, verifies that handleRouteError is called, and checks the error response format matches expectations.

2. Redundant Property Assignment (Minor) - Line 43 in serve.ts has app: app which can be simplified to just app in modern JavaScript.

3. Lack of Context in PR Description - The PR description is empty. It would be helpful to include how the bug was discovered, steps to reproduce the original issue, and explanation of why the previous approach didn't work.

Security Considerations

No security concerns identified. The changes don't introduce new attack vectors.

Performance Considerations

Positive Impact: By removing the creation of an unnecessary Hono instance, there's a minor performance improvement (one less object allocation).

Suggestions for Follow-up

1. Add Integration Test: Create a test that verifies error handling works end-to-end to prevent regression.
2. Documentation: Consider adding a comment in crossPlatformServe explaining that the app parameter must be the fully-configured router with error handlers already attached.
3. Code Cleanup: Fix the redundant property shorthand on line 43.

Recommendation

Approve with minor suggestions - The core fix is solid and addresses a real bug. The suggestions above are nice-to-haves that can be addressed in follow-up PRs if needed. The most important one is adding test coverage to prevent regression.

Review performed by Claude Code following CLAUDE.md conventions

[vercel]

Deployment failed with the following error:

Too many requests - try again in 1 minute (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

[claude]

Code Review

Summary

This PR fixes a bug where the onError handler registered on the Hono router was not being triggered. The root cause was that the previous implementation was creating a new Hono app instance in crossPlatformServe when no userRouter was provided, then mounting the rivetKitRouter onto it. This meant the error handler registered on rivetKitRouter (line 131 in router.ts) was not being used by the outer app instance that actually served requests.

Changes Analysis

Before:

• crossPlatformServe accepted two parameters: rivetKitRouter and optional userRouter
• Created a new Hono app if userRouter was undefined: const app = userRouter ?? new Hono()
• Mounted the registry router: app.route("/", rivetKitRouter)
• This created a nested router structure where the error handler on rivetKitRouter was not accessible to the outer app

After:

• crossPlatformServe accepts a single app parameter (the already-configured Hono router)
• The router passed in already has onError configured (router.ts:131)
• No more router mounting/nesting, so the error handler works correctly

Review Feedback

✅ Positive aspects:

1. Correct fix: This properly addresses the root cause by eliminating the unnecessary router nesting
2. Cleaner API: Simplifying the function signature makes the code more maintainable
3. Type safety: Changing to import type { Hono } is appropriate since Hono is only used as a type annotation
4. No breaking changes: The change is internal and does not affect the public API

⚠️ Minor observations:

1. Redundant property assignment (serve.ts:43):

   app: app,

   Could be simplified to:

   app,

   This is a very minor style issue and does not affect functionality.

2. Missing test coverage: It would be valuable to add a test case that verifies the onError handler is correctly invoked when errors occur. This would prevent regression of this bug in the future.

3. Commented code history: The PR diff shows commented-out code was removed:

   // app.route("/registry", rivetKitRouter);

   This is good - removing dead code improves maintainability.

Security & Performance

• ✅ No security concerns identified
• ✅ No performance implications
• ✅ No changes to error handling logic itself, just fixing where it is applied

Recommendations

Optional improvements (not blockers):

1. Add a test case to verify error handling works correctly (e.g., trigger an error and verify handleRouteError is called)
2. Simplify app: app to app on line 43 of serve.ts

Verdict: ✅ Approved - This is a solid bug fix that correctly addresses the issue with minimal code changes. The fix is well-scoped and does not introduce any new risks.

---

Review generated by Claude Code

[claude]

Pull Request Review

Summary

This PR fixes a bug where the onError handler was not working correctly in RivetKit. The fix simplifies the server initialization by passing the fully-configured Hono router directly to crossPlatformServe instead of creating a new Hono instance that loses the registered error handlers.

What Changed

• serve.ts: Simplified crossPlatformServe() to accept a single app parameter instead of separate rivetKitRouter and userRouter parameters
• mod.ts: Updated call site to pass the pre-configured Hono router from createManagerRouter()
• Changed import from import { Hono } to import type { Hono } for better type-only usage

Code Quality: ✅ Excellent

Strengths:

1. Correct Root Cause Fix: The issue was that error handlers (router.onError(handleRouteError)) registered in manager/router.ts:131 were being applied to a different Hono instance than the one serving requests. This fix ensures the same router instance is used throughout.

2. Architectural Improvement: Removes unnecessary abstraction and makes the code flow clearer. The responsibility for router configuration now clearly lives in createManagerRouter().

3. Follows Repository Patterns: Consistent with how error handling is set up elsewhere in the codebase.

4. Type Safety: Properly uses import type for type-only imports.

5. Code Cleanup: Removes dead code (commented-out mounting logic).

Potential Issues

Minor:

1. Redundant object shorthand (serve.ts:43):

   // Current:
   app: app,
   
   // Could simplify to:
   app,

2. Missing Test Coverage: No tests verify that error handlers now work correctly. Consider adding a test that:

   • Triggers an error in a route handler
   • Verifies handleRouteError is called
   • Checks the error response format

Performance: ✅ No Concerns

The change slightly improves performance by eliminating unnecessary Hono instance creation.

Security: ✅ No Concerns

No security issues identified. Error handlers are properly configured before the router is exposed.

Breaking Changes: ✅ None

The userRouter parameter was always passed as undefined at the only call site (mod.ts:223), so this is not a breaking change in practice.

Recommendations

1. Add integration test to verify error handling works end-to-end and prevent regression
2. Optional cleanup: Simplify app: app, to app, on line 43
3. Consider adding a comment explaining why the app must be pre-configured with error handlers

Verdict: ✅ LGTM - Approve

This is a well-executed fix that addresses the root cause correctly. The architectural simplification is a bonus. The suggestions above are optional improvements that can be addressed in follow-up work if desired.

---

Review performed following CLAUDE.md conventions

[graphite-app]

Merge activity

• Nov 13, 9:34 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 13, 9:34 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3441.
• Nov 13, 9:37 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3441.
• Nov 13, 9:38 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 13, 9:43 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 13, 9:43 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3441.
• Nov 13, 10:05 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 13, 10:05 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3441.
• Nov 13, 10:11 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 13, 10:12 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3442.

[claude]

PR Review: fix(rivetkit): fix onError handler not working

Summary

This PR fixes a bug where the onError handler configured on Hono routers wasn't working properly. The fix simplifies the crossPlatformServe function by passing the fully configured Hono app directly instead of trying to mount it on a separate router.

Code Quality ✅

Positive aspects:

1. Simplified architecture - Removing the unnecessary userRouter parameter and the mounting logic makes the code cleaner and more maintainable
2. Type safety improvement - Changing import { Hono } to import type { Hono } is a good TypeScript practice
3. Fix is minimal and focused - Only changes what's necessary to fix the bug
4. Consistent with codebase patterns - The fix aligns with how routers are created and used elsewhere (e.g., in test/mod.ts and driver-test-suite/mod.ts)

Minor observations:

1. In serve.ts:43, the assignment app: app is redundant - could be simplified to just app (object property shorthand). This is not critical but follows modern JavaScript conventions.

Bug Analysis ✅

The root cause was clear: the previous code created a new Hono instance if userRouter was undefined, then mounted the rivetKitRouter on it. This meant the error handlers configured on rivetKitRouter (via router.onError() in manager/router.ts:131 and actor/router.ts:230) weren't being called because the outer app was handling requests first.

The fix correctly passes the fully configured router directly, ensuring all middleware and error handlers work as expected.

Performance Considerations ✅

• Neutral impact - No performance concerns. Actually slightly better since we're eliminating one unnecessary router layer.
• The removal of the commented-out app.route("/registry", rivetKitRouter) line is good housekeeping.

Security Concerns ✅

No security issues identified. The change doesn't affect:

• Authentication/authorization flows
• Input validation
• Error message exposure (error handlers remain the same)

Test Coverage ⚠️

Concerns:

1. No test coverage - I couldn't find automated tests specifically validating that onError handlers work correctly
2. Manual testing recommended - This should be manually tested to ensure:
   • Error responses are properly formatted when routes throw errors
   • WebSocket error handlers (onError in WebSocket handlers) continue to work
   • Both manager and actor router error handlers are called appropriately

Suggestion: Consider adding an integration test that:

• Triggers an intentional error in a route
• Verifies the onError handler is called
• Validates the error response format matches expectations

Recommendations

1. Test the fix thoroughly - Especially test scenarios where routes throw errors to ensure handleRouteError is being called
2. Optional cleanup - Change app: app to just app in line 43 for consistency with modern JS style
3. Documentation - Consider adding a comment explaining why the app must be passed directly (to preserve error handlers)

Verdict: ✅ Approved with Minor Suggestions

This is a good bug fix that simplifies the code while fixing the actual issue. The main concern is test coverage - please ensure manual testing has been performed before merging.

---

Great work identifying and fixing this issue! 🚀

[claude]

PR Review: Fix onError handler not working

Summary

This PR fixes a bug where the onError handler in RivetKit wasn't working properly. The issue was caused by the way the Hono router was being passed to crossPlatformServe.

Changes Analysis

1. Simplified crossPlatformServe signature (serve.ts)

• Before: crossPlatformServe(config, rivetKitRouter, userRouter) - accepted both a RivetKit router and optional user router, then merged them
• After: crossPlatformServe(config, app) - accepts a single pre-configured Hono app
• Removed the internal router mounting logic (app.route("/", rivetKitRouter))

2. Updated call site (mod.ts:223)

• Changed from passing undefined as third parameter to only passing two parameters
• The hono variable (created by createManagerRouter) is now passed directly

Root Cause Analysis

The bug occurred because:

1. createManagerRouter creates a router with error handlers configured (router.ts:130-131):

   router.notFound(handleRouteNotFound);
   router.onError(handleRouteError);

2. The old code would then mount this configured router into a new Hono instance inside crossPlatformServe
3. When mounting a sub-router, the parent router's error handlers take precedence, and the new instance had no error handlers configured
4. Result: Custom error handling was lost

Code Quality ✅

Strengths:

• Clean simplification that removes unnecessary abstraction
• Follows single responsibility principle better
• The fix directly addresses the root cause
• Import changed from Hono to type Hono (type-only import) which is good for tree-shaking

Minor observations:

• Redundant syntax at serve.ts:43: app: app could be just app (object shorthand)

Potential Issues ✅ None Found

The changes are straightforward and low-risk:

• Type safety is maintained
• No breaking changes to external APIs
• The function signature change is internal to the package

Performance Considerations ✅

Improvements:

• Eliminates one unnecessary Hono instance creation
• Removes one router mounting operation
• Slightly reduces memory footprint

Security Concerns ✅ None

The error handling fix actually improves security by ensuring that:

• Error handlers are properly invoked
• Structured error responses are returned via handleRouteError
• Internal error details can be properly controlled via exposeInternalError

Test Coverage ⚠️

Observations:

• No new tests added in this PR
• The fix is for runtime behavior (error handler invocation)

Recommendations:

• Consider adding an integration test that verifies onError handler is invoked when errors occur
• Test could make a request that triggers an error and verify the response format matches what handleRouteError produces
• Could be added to the existing driver test suite

Additional Notes

Architecture improvement:
This change represents a better separation of concerns:

• createManagerRouter is responsible for the complete router configuration (including error handling)
• crossPlatformServe is responsible only for platform-specific server setup (Node.js server, WebSocket injection)

Backwards compatibility:
This is an internal change - external consumers of RivetKit won't be affected.

---

Recommendation: ✅ Approve with suggestion

The fix is correct and improves the codebase. The only suggestion is to consider adding test coverage to prevent regression of this issue.

Optional improvements (can be done in follow-up):

1. Add integration test for error handling
2. Change app: app to app at line 43 in serve.ts

Great catch on this bug! The fix properly restores error handling functionality.

[claude]

Code Review

Summary

This PR fixes a bug where the onError handler registered on the Hono router was not being triggered. The fix simplifies the crossPlatformServe function by removing unnecessary router composition logic.

Analysis

Root Cause

The previous implementation in crossPlatformServe was:

1. Creating a new Hono app (or accepting a user-provided one via userRouter)
2. Mounting the RivetKit router onto this app with app.route("/", rivetKitRouter)

The problem is that error handlers (router.onError(handleRouteError)) are registered on the rivetKitRouter in createManagerRouter (router.ts:131), but requests were being handled by the outer app instance. In Hono, when you mount a router, errors thrown in the mounted router don't automatically propagate to use the mounted router's error handlers - they use the parent app's handlers.

The Fix

The fix removes the unnecessary indirection:

• Removed the userRouter parameter (which was always undefined anyway per mod.ts:223)
• Removed the logic that created a new Hono app and mounted the router
• Passes the rivetKitRouter (renamed to app) directly to the WebSocket and server setup

This ensures that:

1. Error handlers registered in createManagerRouter work correctly
2. The handleRouteError function can properly format errors with the correct encoding, status codes, and metadata
3. Errors are logged consistently through the configured logger

Code Quality

Positive aspects:

• ✅ Simplifies the code by removing ~8 lines of unnecessary logic
• ✅ Fixes the bug without introducing new complexity
• ✅ Uses TypeScript type import for Hono (better for type-only imports)
• ✅ Maintains consistent naming by renaming parameter to app

Minor observations:

• The redundant app: app on line 43 could be simplified to just app (ES6 shorthand), but this is a stylistic nit and doesn't affect functionality

Testing Considerations

The error handling paths should be tested to verify this fix works correctly:

1. Test case: Trigger an error in a route handler and verify:

   • The handleRouteError function is called
   • Error response has correct status code and format
   • Errors are logged with proper context

2. Encoding handling: Test that errors respect the request encoding (JSON/CBOR/BARE) as handled in handleRouteError

3. Custom errors: Verify RivetKit custom errors (InvalidRequest, ActorNotFound, etc.) are properly formatted in responses

Potential Concerns

None identified. This is a straightforward bug fix that:

• Has no breaking changes (removes unused parameter)
• Reduces complexity
• Fixes error handling as intended

Recommendation

Approve ✅

This is a clean fix for a legitimate bug. The change improves code quality by removing unnecessary abstraction while fixing the error handler registration issue.

---

Note: Consider adding integration tests that verify error handlers work correctly to prevent regression.

[claude]

Code Review for PR #3455: fix(rivetkit): fix onError handler not working

Summary

This PR fixes a critical bug where the onError handler in RivetKit was not working properly. The fix simplifies the crossPlatformServe function signature and ensures that error handlers registered on the Hono router are preserved.

Analysis

Root Cause
The issue stemmed from the way the Hono router was being passed to crossPlatformServe. Previously:

1. createManagerRouter would create a router and register router.onError(handleRouteError) (manager/router.ts:131)
2. This router was then passed to crossPlatformServe as rivetKitRouter
3. crossPlatformServe would create a new Hono instance and mount the rivetKitRouter as a sub-route
4. When mounted as a sub-route, the error handler wouldn't propagate correctly

The Fix
The PR simplifies the approach by:

1. Passing the already-configured Hono app directly (now called app instead of rivetKitRouter)
2. Removing the unnecessary logic that created a new Hono instance and mounted routes
3. This preserves the error handler that was already registered

Code Quality: ✅ Good

Strengths:

• Simplification: Removed 7 lines of unnecessary code, making the function cleaner and easier to understand
• Correct abstraction: The function now has a single, clear responsibility - serve the app - rather than also handling route mounting
• Type safety: Changed import from import { Hono } to import type { Hono } which is more appropriate since Hono is only used as a type annotation (rivetkit-typescript/packages/rivetkit/src/registry/serve.ts:1)

Minor observations:

• Line 43 has redundant syntax: app: app, could be simplified to just app, (ES6 shorthand property)

Potential Issues: ✅ None Found

No bugs or issues identified. The fix correctly addresses the root cause.

Performance: ✅ Neutral/Slight Improvement

• Positive: Removed unnecessary Hono instance creation, which eliminates a small allocation
• Positive: Removed unnecessary route mounting operation
• Overall: Negligible performance impact, but technically a micro-optimization

Security: ✅ No Concerns

No security issues introduced or fixed. Error handling is properly preserved.

Test Coverage: ⚠️ Needs Attention

Missing test coverage:

• No tests found for the crossPlatformServe function
• No tests for the error handling behavior that this PR fixes

Recommendations:

1. Add integration tests that verify onError handlers are called correctly when errors occur in routes
2. Add tests for crossPlatformServe to ensure WebSocket upgrade functionality works
3. Consider adding a regression test that specifically validates the error handling flow end-to-end

Recommendations

1. Fix the redundant property syntax (optional, minor):

   // Line 42-43
   const { injectWebSocket, upgradeWebSocket } = createNodeWebSocket({
   -    app: app,
   +    app,
   });

2. Add tests (strongly recommended):

   • Create a test file at rivetkit-typescript/packages/rivetkit/src/registry/serve.test.ts
   • Test that error handlers are invoked when routes throw errors
   • Test WebSocket functionality

3. Documentation (optional):

   • Consider adding a JSDoc comment to crossPlatformServe explaining that the app should already have error handlers configured

Conclusion

This is a solid bug fix that improves code quality by removing unnecessary complexity. The change correctly addresses the root cause and makes the code more maintainable. The main concern is the lack of test coverage for this critical functionality.

Approval Status: ✅ Approved with minor recommendations

The code is ready to merge, but I strongly recommend adding tests to prevent regression of this bug in the future.