chore(rivetkit): log close code & reason in disconnect warning

[NathanFlurry]

No description provided.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 4, 2025 8:26pm

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 4, 2025 8:26pm
rivet-inspector	Ignored	Preview		Nov 4, 2025 8:26pm
rivet-site	Ignored	Preview		Nov 4, 2025 8:26pm

[NathanFlurry]

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

• fix(fe): show connect page when there are no runner configs but runner names #3298 : 2 dependent PRs (#3306 , #3310 )
• chore(rivetkit): log version with error logs #3303
• chore(rivetkit): log close code & reason in disconnect warning #3302 👈 (View in Graphite)
• feat(rivetkit): integrate websocket hibernation #3301
• fix: use correct vbare functions for runner tunnel #3300 : 1 other dependent PR (#3332 )
• feat(grafana): fix api dashboards #3193
• fix: docker compose build #3273
• chore: fix remaining warnings for all targets #3305
• fix(gas): fix from_workflow when creating standalone ctx from op ctx #3342
• ci: udpate ci filter to succeed if no relevant changes #3343
• 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]

PR Review: Log close code & reason in disconnect warning

Summary

This PR enhances the WebSocket disconnect logging by passing the close code and reason from the WebSocket close event through to the onDisconnected callback, allowing for better debugging and observability of connection terminations.

Code Quality: Good ✅

Strengths:

1. Consistent with existing patterns - aligns with logging at line 636-640 in mod.ts
2. Improved observability - follows project's structured logging conventions from CLAUDE.md
3. Type safety - interface change properly types the new parameters
4. Minimal scope - focused change without unnecessary complexity

Potential Issues: Minor Concerns ⚠️

Breaking Change - Not all callers updated:

The signature change to onDisconnected is breaking, but not all implementations were updated:

• ✅ Updated: rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:128-135
• ❌ Not updated: engine/sdks/typescript/test-runner/src/index.ts:164 (uses empty function body)
• ❌ Not updated: engine/sdks/typescript/runner/benches/utils.ts:114 (commented out code)
• ⚠️ Commented out: engine/sdks/typescript/runner/tests/lifecycle.test.ts:134

Recommendation: Update empty implementations to accept the parameters even if unused, e.g., onDisconnected: (_code, _reason) => { } to ensure TypeScript compatibility.

Other Aspects

• Performance: ✅ No concerns - negligible impact
• Security: ✅ No concerns - standard WebSocket protocol values
• Test Coverage: ⚠️ Main test file is commented out, cannot validate through automated tests
• Logging Best Practices: ✅ Excellent - uses structured fields, lowercase message, includes context

Recommendations

1. Fix breaking change in test-runner/src/index.ts:164
2. Consider documenting WebSocket close code meanings in a comment
3. Update commented-out code in benches/utils.ts if it will be reused

Verdict: Approve with minor fix required ✅

The change improves observability and follows project conventions. Please update the test-runner implementation to accept the new parameters to avoid TypeScript compilation errors.

Files requiring update:

• engine/sdks/typescript/test-runner/src/index.ts:164

Great work on improving debugging capabilities! 🎯

[pkg-pr-new]

More templates

• example-ai-agent
• example-background-jobs
• example-better-auth-external-db
• example-bots
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• example-counter
• example-counter-serverless
• example-crdt
• example-cursors
• example-cursors-raw-websocket
• example-database
• example-deno
• example-sqlite
• example-elysia
• example-express
• example-freestyle
• example-game
• example-hono
• example-hono-bun
• example-hono-react
• example-kitchen-sink
• example-next-js
• example-rate
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• example-smoke-test
• starter
• example-stream
• example-sync
• example-tenant
• example-trpc
• example-workflows

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: f76b79c

[claude]

PR Review

Summary

This PR enhances observability by adding close code and reason parameters to the onDisconnected callback, allowing better debugging of WebSocket disconnections.

Code Quality ✅

• Clean implementation: The changes properly thread the close code and reason through the callback chain
• Type safety: TypeScript interface correctly updated with proper parameter types
• Consistent logging: Follows the repository's structured logging patterns using the tracing approach

Issues Found 🔴

1. Missing update in test-runner (Breaking Change)
The test-runner file at engine/sdks/typescript/test-runner/src/index.ts:164 still uses the old callback signature:

onDisconnected: () => { },

This needs to be updated to:

onDisconnected: (code, reason) => { },

Without this change, the test-runner will fail to compile due to the signature mismatch.

Location: engine/sdks/typescript/test-runner/src/index.ts:164

Recommendations 📋

1. Update test-runner: Fix the signature mismatch in test-runner to match the new interface
2. Consider logging in test-runner: Since you're adding observability, consider logging the disconnect details in test-runner as well for consistency:

   onDisconnected: (code, reason) => {
       getLogger().warn({ msg: "runner disconnected", code, reason });
   },

Performance ✅

No performance concerns - the changes are minimal and only add two primitive parameters to an existing callback.

Security ✅

No security concerns identified.

Test Coverage ⚠️

• The commented-out test code doesn't need updating (it's disabled)
• The benchmark code is also commented out
• However, you should verify that the test-runner fix works correctly with manual testing

Style & Conventions ✅

• Follows structured logging patterns from CLAUDE.md
• Lowercase log messages as specified in guidelines
• Proper parameter formatting in tracing logs

---

Action Required: Please update engine/sdks/typescript/test-runner/src/index.ts:164 to fix the callback signature before merging.

[claude]

PR Review: chore(rivetkit): log close code & reason in disconnect warning

Summary

This PR enhances logging for WebSocket disconnections by adding the close code and reason to the onDisconnected callback and the associated warning log. This is a small but valuable improvement for debugging connection issues.

Code Quality ✅

Strengths:

• Clean, focused change that improves observability
• The modification correctly threads the code and reason parameters from the WebSocket close event through to the logging handler
• Follows existing structured logging patterns using tracing-style field logging

Interface Changes:

• Updates the RunnerConfig.onDisconnected signature from () => void to (code: number, reason: string) => void
• The change is applied consistently across both the interface definition and implementation

Compatibility Analysis ⚠️

Breaking Change Detected:

I've identified 3 other call sites that implement this callback with the old signature:

1. engine/sdks/typescript/runner/tests/lifecycle.test.ts:134 - Empty function: onDisconnected: () => { },
2. engine/sdks/typescript/test-runner/src/index.ts:164 - Empty function: onDisconnected: () => { },
3. engine/sdks/typescript/runner/benches/utils.ts:114 - Empty function: onDisconnected: () => {},

While these files are commented out (wrapped in block comments), this is still a breaking API change that will affect any external consumers of the @rivetkit/engine-runner package who implement the onDisconnected callback.

Recommendations:

1. For internal code: Update the commented-out implementations to match the new signature, even though they're not currently active. This prevents future confusion when the code is re-enabled.

2. For external compatibility: Consider one of these approaches:

   • If this is pre-1.0 or internal-only: Document this as a breaking change in release notes
   • For broader compatibility: Make the parameters optional to maintain backward compatibility

3. Type safety: Since this changes a public interface, ensure the version bump follows semantic versioning (likely a minor bump if pre-1.0, or major if post-1.0).

Security ✅

No security concerns. The change only adds logging and doesn't introduce new attack vectors.

Performance ✅

Minimal performance impact - just passing two additional parameters and logging two extra fields.

Testing Considerations 📝

The PR doesn't include test updates, which is acceptable given:

• The affected test files are commented out
• The change is straightforward and low-risk
• This is primarily a logging enhancement

However, when those tests are re-enabled, they should verify that:

• The close code is correctly propagated (e.g., 1000 for normal closure, 1011 for errors)
• The reason string is properly captured

Suggested Improvements

Optional Enhancement: Consider logging the close code as a human-readable string for easier debugging by mapping standard WebSocket close codes (1000 = Normal Closure, 1006 = Abnormal Closure, 1011 = Internal Error, etc.)

Verdict

Approve with minor suggestions ✅

This is a solid improvement to observability. The main consideration is ensuring backward compatibility is handled appropriately for any external consumers of this API.

---

Files reviewed:

• engine/sdks/typescript/runner/src/mod.ts:43,643
• rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:128-135

[claude]

PR Review: Log Close Code & Reason in Disconnect Warning

Summary

This PR enhances the onDisconnected callback to include WebSocket close code and reason, improving debuggability when runners disconnect. The changes are straightforward and follow good logging practices from CLAUDE.md.

Code Quality ✅

Positive aspects:

• Follows structured logging pattern correctly: tracing::info!(?x, "foo") equivalent in TypeScript
• Lowercase log message follows convention: "runner disconnected"
• Clean interface change that adds valuable debugging information
• Consistent with existing logging at mod.ts:636-641 which already logs code/reason

Issues Found ⚠️

1. Breaking Change - Incomplete Migration

The signature change to onDisconnected is a breaking change, but not all callers have been updated:

Files that need updates:

• engine/sdks/typescript/test-runner/src/index.ts:164 - Currently uses onDisconnected: () => { }
• engine/sdks/typescript/runner/benches/utils.ts:114 - Currently uses onDisconnected: () => {} (commented out)
• engine/sdks/typescript/runner/tests/lifecycle.test.ts:134 - Currently uses onDisconnected: () => { } (commented out)

Recommendation: Update all implementations to match the new signature:

onDisconnected: (code, reason) => { },

Even if the parameters are unused, they should be present to match the interface. This prevents runtime errors and TypeScript compilation issues.

Performance Considerations ✅

• No performance concerns
• Adding parameters to callback has negligible overhead
• Logging structured data is already being done elsewhere in the codebase

Security Concerns ✅

• No security issues identified
• Close codes and reasons are standard WebSocket protocol information
• No sensitive data exposure

Test Coverage ⚠️

Current state:

• The test file that uses onDisconnected is commented out (lifecycle.test.ts)
• No active tests verify the new parameters are passed correctly

Recommendation:

• When lifecycle.test.ts is re-enabled, ensure the test validates that close code/reason are logged
• Consider adding a simple unit test to verify the callback receives the correct parameters

Best Practices ✅

• Follows CLAUDE.md logging patterns
• Improves observability as recommended for workflow engine
• Clean, minimal change with clear intent

Action Items

Required before merge:

1. Update engine/sdks/typescript/test-runner/src/index.ts:164 to accept the new parameters
2. Update commented implementations in benches/utils.ts:114 and tests/lifecycle.test.ts:134 for future compatibility

Optional improvements:

1. Add JSDoc comment to the interface documenting what the code/reason parameters represent
2. Consider logging the close code as a named constant (e.g., "NORMAL_CLOSURE" for 1000) for better readability

Overall Assessment

Good improvement for debuggability! The core change is solid, but needs completion to avoid breaking other parts of the codebase. Once the remaining implementations are updated, this will be ready to merge.

Rating: Approve with required changes ⚠️

[claude]

PR Review: chore(rivetkit): log close code & reason in disconnect warning

Summary

This PR enhances observability by adding WebSocket close code and reason to the disconnect warning logs. The change propagates these values from the WebSocket close event through the runner's onDisconnected callback.

Code Quality ✅

• Clean, focused change with a single clear purpose
• Good use of structured logging following the project's tracing patterns
• Consistent with existing code style

Issues Found 🔴

Breaking Change Not Fully Applied

The signature of onDisconnected was changed from () => void to (code: number, reason: string) => void, but not all consumers were updated:

Missing update in: engine/sdks/typescript/test-runner/src/index.ts:164

onDisconnected: () => { },  // ❌ Needs to accept (code, reason) parameters

This will cause a TypeScript compilation error or runtime issues. The callback should be updated to:

onDisconnected: (_code, _reason) => { },

Recommendations

1. Fix the breaking change: Update test-runner/src/index.ts:164 to match the new signature
2. Consider adding type safety: If not already present, ensure TypeScript strict mode catches these signature mismatches at compile time
3. Test coverage: Verify that the test runner still works after updating the callback signature

Security & Performance ✅

• No security concerns identified
• No performance impact - just adding parameters to logging

Positive Notes ✨

• The enhanced logging will make debugging WebSocket disconnections much easier
• Good adherence to structured logging patterns (code and reason as separate fields)
• Minimal, surgical change that doesn't introduce unnecessary complexity

Verdict

⚠️ Needs Changes - Please update the test-runner callback signature before merging to avoid breaking changes.

[claude]

Code Review

Summary

This PR improves observability by adding code and reason parameters to the onDisconnected callback, allowing better debugging of WebSocket disconnections in the runner.

✅ Positive Points

1. Improved Observability: Adding the close code and reason to the disconnect warning is excellent for debugging. This will help diagnose why runners are disconnecting.

2. Minimal, Focused Change: The PR is appropriately scoped - it only touches what's necessary to pass through the disconnect information.

3. Consistent with Existing Pattern: The change follows the pattern already used in the close event handler in mod.ts:636-641, where code and reason are already logged.

4. TypeScript Type Safety: The interface signature change ensures type safety across all implementations.

🔍 Observations

Backward Compatibility

This is a breaking change to the RunnerConfig interface. All existing implementations of onDisconnected callbacks must be updated to accept the two new parameters, even if they don't use them.

I verified the following files have been updated correctly:

• ✅ lifecycle.test.ts:134 - Empty implementation updated
• ✅ test-runner/src/index.ts:164 - Empty implementation updated
• ✅ benches/utils.ts:114 (commented out code) - Empty implementation updated
• ✅ actor-driver.ts:128 - Main implementation updated with logging

Logging Pattern Compliance

The logging in actor-driver.ts:129-135 correctly follows the structured logging pattern from CLAUDE.md:

• ✅ Uses structured fields (code, reason) instead of formatting into the message
• ✅ Message is lowercase ("runner disconnected")

💡 Suggestions

1. Consider Type Safety for Optional Parameters

If you want to maintain backward compatibility in the future, consider making the parameters optional with TypeScript:

onDisconnected: (code?: number, reason?: string) => void;

However, since all call sites are internal and have been updated, the current approach is fine.

2. Add JSDoc Documentation

Consider adding JSDoc to the interface to document the WebSocket close codes:

/**
 * Called when the runner WebSocket disconnects
 * @param code - WebSocket close code (see https://www.rfc-editor.org/rfc/rfc6455#section-7.4.1)
 * @param reason - Human-readable close reason
 */
onDisconnected: (code: number, reason: string) => void;

3. Consider Additional Context

The ev.reason in mod.ts:643 is converted to string via .toString(), but the close event handler at line 640 also uses ev.reason.toString(). This is consistent, but worth noting that if ev.reason is not a string type, this ensures it's converted properly.

🎯 Verdict

Approved ✅

This is a clean, well-executed change that improves debugging capabilities without introducing any bugs or security issues. The breaking change is acceptable given that all implementations are internal and have been updated.

Test Coverage

The change doesn't require new tests since:

• It's primarily adding observability logging
• The WebSocket close event handling is already tested via existing lifecycle tests
• All implementations have been updated correctly

Great work on improving observability! 🎉

[claude]

Code Review

Summary

This PR enhances the onDisconnected callback to include close code and reason parameters, improving observability when WebSocket connections are closed. The change improves debugging capabilities by exposing this information in the disconnect warning logs.

Issues Found

🔴 Breaking Change - Missing Update

The signature change to onDisconnected is a breaking change that affects other parts of the codebase:

Location: engine/sdks/typescript/test-runner/src/index.ts:164

onDisconnected: () => { },

This needs to be updated to match the new signature:

onDisconnected: (code, reason) => { },

Impact: This will cause a TypeScript compilation error since the callback signature no longer matches the interface definition.

---

Positive Observations

✅ Good: The change improves observability by exposing WebSocket close codes and reasons in logs, which will aid in debugging connection issues.

✅ Good: The implementation correctly passes through the existing ev.code and ev.reason values that are already being logged elsewhere (line 639-640 in mod.ts).

✅ Good: Follows the repository's logging pattern from CLAUDE.md - using structured logging with parameters rather than formatting them into the message string.

✅ Good: The rivetkit implementation properly uses the new parameters in the structured log output.

---

Recommendations

1. Update test-runner - Please update engine/sdks/typescript/test-runner/src/index.ts:164 to accept the new parameters.

2. Consider commented code - There are commented-out onDisconnected callbacks in:

   • engine/sdks/typescript/runner/tests/lifecycle.test.ts:134
   • engine/sdks/typescript/runner/benches/utils.ts:114

   While these are commented out, consider updating them too to avoid confusion if they're ever uncommented.

3. Type safety - Consider adding the parameters to the rivetkit callback signature for better type safety:

   onDisconnected: (code: number, reason: string) => {

---

Verdict

Requires Changes - Please fix the breaking change in test-runner before merging.

Once the test-runner is updated, this is a clean improvement to the logging infrastructure. 👍

[claude]

Code Review

Summary

This PR improves observability by adding code and reason parameters to the onDisconnected callback, allowing consumers to log more detailed information about WebSocket disconnections.

Issues Found

🚨 Breaking Change - Incomplete Migration

The signature of onDisconnected has been changed from:

onDisconnected: () => void

to:

onDisconnected: (code: number, reason: string) => void

However, not all usages have been updated. This will cause TypeScript compilation errors:

Needs to be updated:

• engine/sdks/typescript/test-runner/src/index.ts:164 - Still uses the old signature without parameters

Should be updated for consistency (commented code):

• engine/sdks/typescript/runner/tests/lifecycle.test.ts:134
• engine/sdks/typescript/runner/benches/utils.ts:114

Recommendations

1. Fix the breaking change: Update test-runner/src/index.ts to accept the new parameters:

   onDisconnected: (code, reason) => { },

2. Update commented code: While the code in test files is commented out, it should still be updated to match the new signature for consistency and to avoid confusion when uncommented.

3. Consider type safety: The reason parameter is typed as string, but in the implementation at mod.ts:640, it's converted with .toString(). Consider whether the type should be string or if the conversion is necessary.

Positive Aspects

✅ Improved observability: Adding close code and reason to logs will help with debugging connection issues
✅ Consistent logging: The structured logging follows the project's patterns (lowercase message, structured fields)
✅ Type safety: The interface properly defines the new parameter types

Testing

Please ensure:

• All TypeScript files compile without errors
• Test runner still works with the updated signature
• The logged code and reason values are helpful for debugging

Severity

Medium - This PR introduces a compilation error that will prevent the codebase from building. The fix is straightforward but necessary before merging.

[vercel]

Deployment failed with the following error:

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

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

[claude]

Code Review

Summary

This PR enhances the onDisconnected callback to include WebSocket close code and reason, improving observability when runner disconnections occur. The changes are minimal and focused.

Positive Aspects

• Good structured logging practice: Following the project's convention of using structured logging with separate fields rather than formatting values into the message string (per CLAUDE.md logging patterns)
• Useful debugging information: Adding code and reason to the disconnect warning will help diagnose connection issues
• Type safety: The interface update properly reflects the new signature

Issues Found

Breaking Change - Incomplete Migration

The signature change to onDisconnected is a breaking change that affects other consumers in the codebase. This implementation is currently incomplete:

File that needs updating:

• engine/sdks/typescript/test-runner/src/index.ts:164 - Still uses the old signature onDisconnected: () => { }

This will cause a TypeScript compilation error since the callback now expects 2 parameters.

Recommended fix: Update the callback to accept the two parameters (code, reason) even if they are not used.

Note: The commented-out usages in lifecycle.test.ts:134 and benches/utils.ts:114 should also be updated when uncommented to prevent future issues.

Minor Suggestions

• Consider adding explicit type annotations to the parameters in actor-driver.ts for clarity: onDisconnected: (code: number, reason: string) =>

Test Coverage

No tests were modified or added. Consider:

• Adding a test case that verifies the code and reason are properly logged on disconnection
• Updating existing tests if they mock/stub the onDisconnected callback

Verdict

Request Changes - The PR needs to update all implementations of onDisconnected to match the new signature before merging to avoid compilation errors.

[graphite-app]

Merge activity

• Nov 4, 8:09 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 4, 8:11 PM UTC: CI is running for this pull request on a draft pull request (#3349) due to your merge queue CI optimization settings.
• Nov 4, 8:41 PM UTC: The Graphite merge queue removed this pull request due to removal of a downstack PR #3342.
• Nov 4, 8:49 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 4, 8:50 PM UTC: CI is running for this pull request on a draft pull request (#3352) due to your merge queue CI optimization settings.
• Nov 4, 8:52 PM UTC: Merged by the Graphite merge queue via draft PR: #3352.

[claude]

PR Review: Add close code & reason to disconnect logging

Summary

This PR enhances the logging for WebSocket disconnections by adding code and reason parameters to the onDisconnected callback, providing better observability when runners disconnect.

Code Quality ✅

Strengths:

• Clean, focused change that improves observability
• Follows existing logging patterns with structured logging
• Signature change is straightforward and logical

Areas for Improvement:

1. Incomplete signature updates ⚠️

   The signature for onDisconnected was updated in the interface and implementation, but there are other call sites that need updating:

   • engine/sdks/typescript/test-runner/src/index.ts:164 - Currently uses empty function () => { }, should be updated to (code, reason) => { } to match the new signature
   • engine/sdks/typescript/runner/tests/lifecycle.test.ts:134 (commented out) - If this is uncommented in the future, it should use the new signature
   • engine/sdks/typescript/runner/benches/utils.ts:114 (commented out) - Same concern if uncommented

   Recommendation: Update the test-runner implementation to accept the new parameters, even if it doesn't use them:

   onDisconnected: (_code, _reason) => { },

2. Type safety consideration

   The ev.reason is converted to string with .toString() before passing to the callback. Consider whether the type should be string (as in the interface) or if it could be undefined/empty. The current implementation looks safe since it's coming from the WebSocket close event.

Testing 🧪

Missing:

• No test updates to verify the new parameters are properly logged
• The test-runner consumer needs signature update

Recommendation: Since this changes a public interface, consider:

• Updating test-runner to use the new signature (even if it ignores the params)
• Adding a test case that verifies close code/reason are properly propagated

Performance ✅

No performance concerns - this is a logging enhancement only.

Security ✅

No security concerns. The close code and reason are standard WebSocket protocol data.

Best Practices ✅

• Follows the existing structured logging pattern
• Maintains consistency with how close events are logged elsewhere in the code

Overall Assessment

Approved with minor suggestions ✅

This is a good improvement for debugging connection issues. The main concern is ensuring all consumers of the onDisconnected callback are updated to match the new signature to prevent TypeScript compilation errors.

Action items:

1. Update engine/sdks/typescript/test-runner/src/index.ts:164 to use the new signature
2. Consider updating commented-out usages for future-proofing