chore(rivetkit): log inspector errors + standardize pino errorKey error#4986
Open
abcxff wants to merge 1 commit into
Conversation
|
🚅 Deployed to the rivet-pr-4986 environment in rivet-frontend
|
Contributor
Author
|
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.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced May 6, 2026
Contributor
PR Review: chore(rivetkit): log inspector errors + standardize pino errorKey error\n\n### Overview\n\nThis PR does two well-scoped things:\n1. Adds errorKey: 'error' to the base pino logger config so Pino built-in error serializer activates when an Error object is logged under the error key.\n2. Standardizes catch variable names from err to error and log field keys from err: to error: across the codebase to match that key.\n3. Adds missing logger().error(...) calls around the three inspector request handlers in native.ts so errors are no longer silently swallowed before errorResponse().\n\nThe intent is correct and the inspector logging additions are a clear improvement.\n\n### Issues\n\n#### Bug: Variable shadowing in inline-websocket-adapter.ts (lines 164-185)\n\nThe rename from err to error introduces a shadowed binding. The original code explicitly named the inner variable handlerErr to avoid this. Catch bindings are block-scoped so the behavior is correct at runtime, but the shadowing is a readability hazard and TypeScript will warn under --noShadow / strict linting. Rename the inner catch binding back to handlerError (or keep it as handlerErr).\n\n#### Style: Log message capitalization in native.ts\n\nCLAUDE.md states log messages should be lowercase unless mentioning specific code symbols. Two of the three new log messages start with a capital E: 'Error handling inspector action request' and 'Error handling inspector request'. The third is correct: 'error replaying workflow history'. Lowercase all message strings.\n\n#### Minor: Inconsistent rename in registry/index.ts Mode B shutdown handler\n\nMode A renames catch (err) to catch (error) and uses the error shorthand. Mode B keeps catch (err) but changes the field to error: err. Rename the catch variable to error for consistency with the rest of the PR.\n\n### Positive Observations\n\n- errorKey fix is correct and necessary. Without it, Pino serializes Error objects as empty objects when logged under an arbitrary key. Configuring errorKey activates the built-in error serializer (type, message, stack) automatically.\n- sleep-db.ts simplification is good. Removing the manual instanceof Error conversion is the right call now that Pino handles Error objects properly.\n- Inspector error logging additions in native.ts are a clear improvement. Errors were previously only surfaced via errorResponse() with no server-side log.\n- Runner parity is maintained. Both engine/sdks/typescript/runner/src/mod.ts and rivetkit-typescript/packages/engine-runner/src/mod.ts are updated together as required.\n- The session.ts rename is clean and correct.\n\n### Summary\n\nThe two substantive issues are the catch-variable shadowing in handleError (a readability/correctness risk if the block grows) and the capitalized log message strings. Otherwise this is a solid housekeeping PR with a real fix (errorKey) and useful new observability. |
1b843f0 to
b150344
Compare
b150344 to
fc4756e
Compare
This was referenced May 7, 2026
fc4756e to
5ec0839
Compare
error
11 tasks
c58ef25 to
b999f59
Compare
5ec0839 to
4ff7adb
Compare
This was referenced May 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Adds error logs to inpector requests + standardizes
errorerrorKey for pino (was being used but not configured in base logger)