fix(inspector): make inspector work again

[jog1t]

No description provided.

[claude]

Claude finished @jog1t's task —— View job

---

Code Review Complete ✅

Reviewing PR: fix(inspector): make inspector work again

Review Tasks:

• Read and analyze all 5 changed files
• Review packages/rivetkit/scripts/dump-openapi.ts changes
• Review packages/rivetkit/src/actor/router.ts changes
• Review packages/rivetkit/src/inspector/config.ts changes
• Review packages/rivetkit/src/manager/router.ts changes
• Review packages/rivetkit/src/registry/mod.ts changes
• Provide comprehensive feedback on code quality, bugs, performance, security, and test coverage

Code Review Results

This PR fixes inspector functionality by making several key changes across 5 files. Here's my comprehensive analysis:

✅ Positive Changes

1. CORS Configuration Fixed (src/inspector/config.ts:27): Added missing localhost:43709 port to defaultInspectorOrigins, improving development flexibility.

2. Enhanced CORS Headers (src/inspector/config.ts:43-51): Updated allowed headers to include modern web standards like baggage, sentry-trace, and Rivet-specific headers (x-rivet-actor, x-rivet-target).

3. Inspector Null Safety (src/registry/mod.ts:85,99): Added proper null checks (&& managerDriver.inspector) before logging inspector URLs, preventing potential runtime errors.

4. Consistent CORS Implementation (src/manager/router.ts:73-76): Refactored CORS handling to use conditional middleware pattern for better maintainability.

5. Inspector Integration (src/manager/router.ts:380-395): Added proper inspector routing with security middleware and error handling.

6. Async Chain Fix (src/actor/router.ts:253): Changed from await next() to return next() which is the correct pattern for Hono middleware.

⚠️ Critical Issues

🚨 Bug Found: packages/rivetkit/src/manager/router.ts:182

cors: [cors], // ❌ Invalid property

Should be:

middleware: [cors], // ✅ Correct property name

This will cause CORS to fail for the PUT /actors/by-id endpoint.

🔍 Code Quality Issues

1. Type Safety (src/manager/router.ts:386,390): Using inspector: any reduces type safety. Consider defining proper inspector interface types.

2. Import Cleanup (src/registry/mod.ts:4-6): Removed unused getPinoLevel import - good cleanup.

3. Error Handling (src/manager/router.ts:381-382): Good defensive programming with Unsupported error when inspector is not available.

🧪 Test Coverage Assessment

Existing Coverage: Inspector functionality has comprehensive test coverage in driver-test-suite/tests/actor-inspector.ts with 20+ test cases covering:

• Manager inspector endpoints (ping, actors CRUD, pagination, bootstrap)
• Actor inspector endpoints (state management, connections, events, RPCs)
• Error scenarios (404s, invalid parameters)

Missing Coverage: No tests were added/modified for the new CORS changes, but existing integration tests should cover the functionality.

🚀 Performance Considerations

• Efficient CORS Handling: New conditional middleware pattern prevents unnecessary CORS processing
• Lazy Inspector Loading: Good pattern of checking inspector.enabled before initializing routes
• Minimal Overhead: Changes don't introduce performance regressions

🔒 Security Assessment

• CORS Properly Restricted: Only allows Rivet domains and localhost development ports
• Inspector Security: secureInspector middleware properly applied to all inspector routes
• No Sensitive Data Exposure: Changes don't introduce security vulnerabilities
• Token-based Auth: Inspector routes properly protected with Bearer token authentication

🎯 Final Recommendations

1. 🚨 CRITICAL: Fix the cors: [cors] → middleware: [cors] bug on line 182
2. Type Safety: Replace inspector: any with proper typing when possible
3. Testing: Consider adding a specific test for CORS header validation
4. Documentation: Consider adding JSDoc for the inspector configuration changes

📊 Overall Assessment

Code Quality: 8/10 (minus critical bug)
Security: 9/10 (excellent CORS and auth handling)
Performance: 9/10 (efficient implementation)
Test Coverage: 8/10 (good existing coverage, minor gaps)

Verdict: ✅ Approve with required fix - The critical CORS bug must be addressed before merge, but otherwise this is a solid fix that properly restores inspector functionality.

[jog1t]

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(core): fix websockets #1229
• fix(inspector): make inspector work again #1228 👈 (View in Graphite)
• chore(core): remove manager driver #1227
• fix(core): fix issue deserializing WebSocket message with BARE #1226
• chore(core): fix simple engine test #1225
• chore(core): simplify engine keys #1224
• chore(core): remove engine runner addresses config #1223
• chore(core): add welcome message #1222
• chore(fs): remove unneeded warning for onRunnerStart #1221
• chore(core): add readme #1220
• chore(core): add logger to engine runner #1219
• chore(core): update engine runner #1218
• main

---

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.