fix(core): disable inspector unless needed

[NathanFlurry]

No description provided.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-express	Error			Sep 16, 2025 10:31am

[graphite-app]

Merge activity

• Sep 16, 9:46 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Sep 16, 9:47 AM UTC: CI is running for this pull request on a draft pull request (#1248) due to your merge queue CI optimization settings.
• Sep 16, 9:48 AM UTC: Merged by the Graphite merge queue via draft PR: #1248.

[NathanFlurry]

• chore(core): update inspector url #1247
• chore(core): upgrade engine runner #1246
• fix(core): disable inspector unless needed #1245 👈 (View in Graphite)
• 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.

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: fix(core): disable inspector unless needed

Reviewing the pull request for code quality, potential issues, performance, security, and test coverage.

Todo List:

• Read and analyze the changed file
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage needs
• Provide comprehensive feedback

---

Review Summary

This PR makes targeted changes to conditionally disable the inspector for the "engine" driver and prevent the server from starting by default for that driver. The changes are focused and serve a clear purpose.

✅ Strengths

1. Clear Intent: The changes logically group together - both disabling the inspector and preventing auto-server start for the "engine" driver
2. Improved API: Adding config and driver to the ServerOutput interface makes the API more useful for consumers
3. Consistent Pattern: The logic for checking driver.name === "engine" is used consistently in both places
4. Safe Mutations: The config mutation config.inspector.enabled = false occurs after validation and before the config is used

⚠️ Issues & Concerns

1. Config Mutation After Validation (packages/rivetkit/src/registry/mod.ts:63-65)

const config = RunConfigSchema.parse(inputConfig);
// ... 
if (driver.name === "engine") {
    config.inspector.enabled = false; // Mutating after validation
}

Issue: Mutating the config object after Zod validation could lead to inconsistencies. The validated config no longer matches its validated state.

Recommendation: Either validate with the correct config upfront, or create a new config object:

const baseConfig = RunConfigSchema.parse(inputConfig);
const config = driver.name === "engine" 
    ? { ...baseConfig, inspector: { ...baseConfig.inspector, enabled: false } }
    : baseConfig;

2. Hard-coded Driver Name (packages/rivetkit/src/registry/mod.ts:63, 143)

Issue: String literals like "engine" are brittle and error-prone. If the driver name changes, these comparisons will silently fail.

Recommendation: Consider using constants or an enum:

const ENGINE_DRIVER_NAME = "engine" as const;
// or
enum DriverNames {
    ENGINE = "engine",
    // others...
}

3. Potential Type Safety Issue (packages/rivetkit/src/registry/mod.ts:64)

Issue: The code assumes config.inspector exists, but depending on the schema, this might not be guaranteed.

Recommendation: Add a null check:

if (driver.name === "engine" && config.inspector) {
    config.inspector.enabled = false;
}

4. TODOs in Production Code

Issue: Two TODO comments indicate incomplete implementations:

• Line 62: "Find cleaner way of disabling by default"
• Line 141: "FInd better way of doing this" (typo: "FInd")

Recommendation: These suggest the current approach is temporary. Consider either:

• Filing issues to track the proper implementations
• Implementing the cleaner approach now
• Adding more context about why this approach was chosen

🚀 Performance Considerations

• Low Impact: The changes add minimal computational overhead (simple string comparisons)
• Memory: Destructuring assignment adds negligible memory usage
• Startup: Conditionally disabling inspector could improve startup performance for "engine" driver

🔒 Security Assessment

• No Direct Security Issues: The changes don't introduce security vulnerabilities
• Inspector Disabling: Actually improves security for the "engine" driver by disabling development tools in that context

🧪 Test Coverage Recommendations

The changes lack test coverage for critical behaviors:

Recommended Tests:

1. Inspector disabling for engine driver:

   test('disables inspector when driver is "engine"', () => {
     // Test that config.inspector.enabled = false when driver.name === "engine"
   });

2. Server start prevention for engine driver:

   test('does not start server when driver is "engine"', () => {
     // Test that serve() is not called for engine driver
   });

3. Normal behavior for other drivers:

   test('enables inspector and starts server for non-engine drivers', () => {
     // Verify default behavior is preserved
   });

🎯 Recommendations

1. High Priority: Fix config mutation pattern to maintain validation integrity
2. Medium Priority: Replace string literals with constants for better maintainability
3. Medium Priority: Add comprehensive tests for the new conditional logic
4. Low Priority: Address or remove TODO comments
5. Low Priority: Fix typo in TODO comment ("FInd" → "Find")

---