chore(runner): explicitly stringify tunnel messages, commands, and events #3386
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
NathanFlurry
force-pushed
the
11-05-fix_runner_do_not_send_websocket_close_on_tunnel_shutdown_since_pegboard-runner_takes_care_of_this
branch
from
3e95003 to
a232088
Compare
NathanFlurry
force-pushed
the
11-05-chore_runner_explicitly_stringify_tunnel_messages_commands_and_events
branch
from
48c08ef to
2895f0b
Compare
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
Code ReviewSummaryThis PR adds explicit stringify functions for tunnel messages, commands, and events to improve logging. The changes address the issue of logging objects that contain non-JSON-serializable types like ArrayBuffer, BigInt, and Map. Positives
Issues & Recommendations1. Security Concern - Potential Data Leakage in LogsSeverity: High The stringify functions log header contents and other potentially sensitive data: // Line 82 in stringify.ts
return `ToClientRequestStart{actorId: "${actorId}", method: "${method}", path: "${path}", headers: ${stringifyMap(headers)}, body: ${bodyStr}, stream: ${stream}}`;Recommendation: Consider:
2. Missing Escaping in String InterpolationSeverity: Medium String values are directly interpolated without escaping, which could break the log format if they contain quotes or special characters: // Line 63 in stringify.ts
const reasonStr = reason === null ? "null" : `"${reason}"`;If Recommendation: Add proper escaping or use JSON.stringify() for string values: const reasonStr = reason === null ? "null" : JSON.stringify(reason);3. Inconsistent Handling of Index/BigIntSeverity: Low Some places convert bigint to string with template literals, others use the helper: // Line 58 in stringify.ts
return `ToServerWebSocketMessageAck{index: ${index}}`; // Missing stringifyBigIntvs. // Line 50 in stringify.ts
return `ToServerWebSocketOpen{canHibernate: ${canHibernate}, lastMsgIndex: ${stringifyBigInt(lastMsgIndex)}}`;Recommendation: Be consistent - either always use 4. Missing Type Exhaustiveness ChecksSeverity: Low The switch statements do not have default cases that would catch new enum variants at compile time. This means if new message types are added to the protocol, TypeScript will not warn you that the stringify functions are incomplete. Recommendation: Add exhaustiveness checks to catch new variants: export function stringifyEvent(event: protocol.Event): string {
switch (event.tag) {
case "EventActorIntent":
// ...
case "EventActorStateUpdate":
// ...
case "EventActorSetAlarm":
// ...
default:
const _exhaustive: never = event;
return `Unknown event: ${(_exhaustive as any).tag}`;
}
}5. Missing Unit TestsSeverity: Medium The new
Recommendation: Add unit tests to ensure the stringify functions handle all edge cases correctly. 6. Performance Consideration - Map StringificationSeverity: Low The function stringifyMap(map: ReadonlyMap<string, string>): string {
const entries = Array.from(map.entries())
.map(([k, v]) => `"${k}": "${v}"`)
.join(", ");
return `Map(${map.size}){${entries}}`;
}For large maps in high-frequency logging, this could create GC pressure. Recommendation: If maps are typically large, consider:
Code Quality✅ Good separation of concerns Testing❌ No test files included Recommendation: Add tests for edge cases and various message types. SummaryApprove with changes requested The core implementation is solid and addresses the logging issue well. However, there are some security and reliability concerns that should be addressed: Must fix:
Should fix: Nice to have: Let me know if you would like help implementing any of these suggestions! |
Code ReviewSummaryThis PR improves logging by adding explicit stringification for tunnel messages, commands, and events that contain non-JSON-serializable types (ArrayBuffer, BigInt, Map). This is a good improvement for log readability and debugging. Positive Aspects ✅
Issues and Recommendations1. Type Safety Concern
|
NathanFlurry
force-pushed
the
11-05-fix_runner_do_not_send_websocket_close_on_tunnel_shutdown_since_pegboard-runner_takes_care_of_this
branch
from
a232088 to
d5276a3
Compare
NathanFlurry
force-pushed
the
11-05-chore_runner_explicitly_stringify_tunnel_messages_commands_and_events
branch
from
2895f0b to
f4e3b5b
Compare
Code ReviewThis PR introduces explicit stringification for tunnel messages, commands, and events to handle non-JSON-serializable types (ArrayBuffers, BigInts, Maps) in logging. Overall, this is a solid improvement that addresses a real issue with logging complex data structures. Strengths
Code Quality and Best PracticesGood:
Potential Improvements:
Potential Bugs
Security Considerations
Performance Considerations
Test CoverageMissing:
Suggested Test Structure: RecommendationsHigh Priority:
Medium Priority: Low Priority: SummaryThis is a well-implemented solution to a real logging problem. The code is clean, well-structured, and addresses the core issue of logging non-JSON-serializable types. The main concerns are:
The changes are generally safe and improve the codebase. Addressing the high-priority items (especially tests and the bigint bug) would make this fully production-ready. |
Code Review for PR #3386OverviewThis PR adds explicit stringification for tunnel messages, commands, and events to improve logging readability. The changes prevent issues with structured logging of non-serializable types like ArrayBuffer, BigInt, and Map. Strengths
Code QualityGood:
Potential Issues
Performance Considerations
Security Considerations
Suggestions
Test CoverageNo tests were added. Consider adding unit tests for each stringify function with various input types and edge cases. SummaryThis is a solid improvement that makes logging more readable and maintainable. The implementation is clean and comprehensive. The suggestions above are minor improvements. Recommendation: Approve with minor suggestions The code is production-ready as-is, but addressing the suggestions would make it more robust. |
NathanFlurry
force-pushed
the
11-05-chore_runner_explicitly_stringify_tunnel_messages_commands_and_events
branch
from
f4e3b5b to
b479fad
Compare
NathanFlurry
force-pushed
the
11-05-fix_runner_do_not_send_websocket_close_on_tunnel_shutdown_since_pegboard-runner_takes_care_of_this
branch
from
d5276a3 to
7a186f6
Compare
PR Review: Explicitly stringify tunnel messages, commands, and eventsSummaryThis PR adds custom stringify functions to handle logging of protocol messages that contain types incompatible with JSON serialization (ArrayBuffer, BigInt, Map). The changes improve log readability and prevent potential serialization errors. Positives ✅
Issues & Concerns 🔍1. Potential Information Leakage (Security)The stringify functions expose potentially sensitive data in logs:
Recommendation: Consider adding sanitization for sensitive headers (Authorization, Cookie, etc.) or add a configuration option to control verbosity levels. function stringifyMap(map: ReadonlyMap<string, string>, sanitize = true): string {
const sensitiveKeys = new Set(['authorization', 'cookie', 'x-api-key']);
const entries = Array.from(map.entries())
.map(([k, v]) => {
const value = (sanitize && sensitiveKeys.has(k.toLowerCase())) ? '[REDACTED]' : v;
return `"${k}": "${value}"`;
})
.join(", ");
return `Map(${map.size}){${entries}}`;
}2. Missing Error HandlingThe stringify functions don't handle potential errors:
Recommendation: Add try-catch wrappers or ensure these are only called with valid inputs. 3. Incomplete Type CoverageIn const intentStr =
intent.tag === "ActorIntentSleep"
? "Sleep"
: intent.tag === "ActorIntentStop"
? "Stop"
: "Unknown";Similarly for Recommendation: Use exhaustive switch statements with switch (intent.tag) {
case "ActorIntentSleep":
return "Sleep";
case "ActorIntentStop":
return "Stop";
default:
unreachable(intent);
}4. Performance ConsiderationLarge headers or many map entries will be fully stringified even if logs are filtered out. Consider:
Example: function stringifyMap(map: ReadonlyMap<string, string>, maxEntries = 20): string {
const entries = Array.from(map.entries())
.slice(0, maxEntries)
.map(([k, v]) => `"${k}": "${v}"`)
.join(", ");
const suffix = map.size > maxEntries ? `, ... (${map.size - maxEntries} more)` : '';
return `Map(${map.size}){${entries}${suffix}}`;
}5. Code DuplicationThe pattern of handling nullable values is repeated: const codeStr = code === null ? "null" : code.toString();
const reasonStr = reason === null ? "null" : `"${reason}"`;Recommendation: Extract a helper function: function stringifyNullable<T>(value: T | null, formatter: (v: T) => string = String): string {
return value === null ? "null" : formatter(value);
}6. Testing CoverageThere are no test additions for the new stringify functions. Recommendation: Add unit tests covering:
Example test structure: describe('stringify', () => {
describe('stringifyArrayBuffer', () => {
it('should format buffer with byte length', () => {
const buffer = new ArrayBuffer(1024);
expect(stringifyArrayBuffer(buffer)).toBe('ArrayBuffer(1024)');
});
});
describe('stringifyMap', () => {
it('should format map entries', () => {
const map = new Map([['key1', 'val1'], ['key2', 'val2']]);
expect(stringifyMap(map)).toBe('Map(2){"key1": "val1", "key2": "val2"}');
});
});
// ... more tests
});Minor Issues
Performance ImpactThe changes add minimal overhead:
Recommendations SummaryHigh Priority:
Medium Priority: Low Priority: ConclusionThis is a solid improvement that addresses a real logging issue. With the suggested security and robustness enhancements, particularly around sensitive data handling and test coverage, this will be a strong addition to the codebase. Verdict: Approve with recommendations for follow-up improvements. |
Pull Request ReviewSummaryThis PR adds explicit stringification functions for tunnel messages, commands, and events to improve logging output. Previously, objects containing ArrayBuffer, bigint, and Map types were being logged directly, which could lead to poor formatting or serialization issues. Code Quality & Best PracticesStrengths:
Minor Suggestions:
Potential IssuesSecurity Considerations:
Performance ConsiderationsArrayBuffer stringification: The approach is efficient and appropriate for logging. If debugging requires seeing actual content, you might want to add a debug mode that shows a hex preview for small buffers. String concatenation: The use of template literals is fine for logging. Performance impact should be minimal unless processing extremely high message volumes. Test CoverageMissing:
Architecture & DesignExcellent separation of concerns: Moving stringification logic into a dedicated module is the right architectural decision. This:
Structured logging improvement: The changes in mod.ts and tunnel.ts show better structured logging practices by using structured fields instead of spreading object properties. Summary & RecommendationOverall Assessment: Approve with minor suggestions This is a solid improvement to logging infrastructure. The code is well-structured and addresses a real problem with logging complex types. Action Items:
The PR can be merged as-is if security considerations around header logging are acceptable for your use case, but I recommend addressing the test coverage before merging. Great work on improving observability! |
Merge activity
|
graphite-app
bot
deleted the
11-05-chore_runner_explicitly_stringify_tunnel_messages_commands_and_events
branch
No description provided.