Skip to content

Conversation

@graphite-app
Copy link
Contributor

@graphite-app graphite-app bot commented Jan 12, 2026

This draft PR was created by the Graphite merge queue.
Trunk will be fast forwarded to the HEAD of this PR when CI passes, and the original PRs will be closed.

The following PRs are included in this draft PR:

@railway-app
Copy link

railway-app bot commented Jan 12, 2026

This PR was not deployed automatically as @graphite-app[bot] does not have access to the Railway project.

In order to get automatic PR deploys, please add @graphite-app[bot] to your workspace on Railway.

@graphite-app graphite-app bot closed this Jan 12, 2026
@railway-app railway-app bot temporarily deployed to rivet-frontend / production January 12, 2026 22:03 Inactive
@graphite-app graphite-app bot deleted the gtmq_spec_e15a2a_1768255338971-e58fe11d-55f7-45e1-8c50-724fd57b4735 branch January 12, 2026 22:03
@claude
Copy link

claude bot commented Jan 12, 2026

PR Review: Merge Queue #3842 (PRs #3803, #3834, #3833, #3839)

This merge queue contains 4 interdependent PRs with significant architectural changes. Overall the code quality is good, but there are several breaking changes and test coverage gaps that need attention.


🔴 Critical Issues

1. BREAKING: Environment Variable Renaming

All RIVETKIT_* environment variables have been renamed to RIVET_*. This affects:

  • All deployment configurations (Docker, Railway, Vercel, etc.)
  • CI/CD pipelines
  • Developer local setups
  • All example scripts

Required Action:

  • Create a comprehensive migration guide
  • Consider adding deprecation warnings for backward compatibility instead of hard cutover
  • Update all documentation and deployment guides

Code Issue: Function naming inconsistency in rivetkit-typescript/packages/rivetkit/src/utils/env-vars.ts:34-36:

export const getRivetkitInspectorToken = (): string | undefined =>
	getEnvUniversal("RIVET_INSPECTOR_TOKEN");
export const getRivetkitInspectorDisable = (): boolean =>
	getEnvUniversal("RIVET_INSPECTOR_DISABLE") === "1";

Functions still use Rivetkit prefix while others use Rivet. Should be:

export const getRivetInspectorToken = (): string | undefined =>
export const getRivetInspectorDisable = (): boolean =>

2. Test Coverage Completely Disabled

The Cloudflare Workers driver tests have been completely commented out:

  • rivetkit-typescript/packages/cloudflare-workers/src/actor-driver.ts (340 lines commented)
  • rivetkit-typescript/packages/cloudflare-workers/src/actor-handler-do.ts (438 lines commented)
  • Multiple other critical files

Risk: Major changes to serverless startup, Cloudflare Workers integration, and manager/engine initialization have zero test coverage. This is a significant regression risk.

Required Action:

  • Re-enable and update driver tests before merging
  • Add integration tests for new startup sequence
  • Add tests for serverless configuration validation

3. Engine Startup Resource Consumption

In rivetkit-typescript/packages/rivetkit/runtime/index.ts:107-120, the engine now spawns preemptively during Runtime.create():

Concerns:

  • Engine binary downloads and starts synchronously during initialization
  • No timeout handling if engine binary is corrupted/missing
  • In Next.js dev mode, engine spawns automatically (spawnEngine = true)
  • Could significantly impact developer experience and startup time
  • Multiple instances could conflict on port 6420

Recommendations:

  • Add timeout with graceful fallback
  • Add configuration option to disable engine in development
  • Implement better port conflict detection
  • Document resource consumption expectations

⚠️ High Priority Issues

4. Cloudflare Workers SQLite Migration

examples/cloudflare-workers-hono/wrangler.json:9:

"new_sqlite_classes": ["ActorHandler"]

This is a breaking change from KV-based storage to SQLite Durable Objects.

Risk: Data loss for existing Cloudflare Workers deployments if migrations aren't executed properly.

Required:

  • Migration guide for existing deployments
  • Backward compatibility strategy or explicit breaking change notice
  • Testing plan for production migrations

5. Documentation Path Issues

Several documentation files reference incorrect paths:

  • Examples reference src/backend/registry.ts which doesn't exist
  • Should be src/registry.ts for simple examples
  • Broken links throughout documentation

Files affected:

  • website/src/content/docs/actors/quickstart/backend.mdx
  • Various example READMEs

6. Next.js Dependency Changes

examples/next-js/tsconfig.json:17:

"jsx": "react-jsx"

Changed from "preserve" to "react-jsx". This affects how JSX is compiled.

Also added:

  • @hono/node-server: 1.14.2
  • @hono/node-ws: 1.3.0

These dependencies enable the serverless manager but should be documented. Why are Hono dependencies needed in a Next.js project? This needs clarification.


💡 Medium Priority Issues

7. Serverless Configuration Validation

In rivetkit-typescript/packages/rivetkit/src/registry/config/serverless.ts:14-32, new strict validation was added. This is good, but could break existing custom configurations:

if (configureRunnerPool) {
	invariant(
		endpoint || spawnEngine,
		"configureRunnerPool requires either endpoint or spawnEngine"
	);
}
invariant(
	\!(spawnEngine && endpoint),
	"cannot specify both spawnEngine and endpoint"
);

Consider: Adding migration warnings instead of hard failures for one release cycle.

8. Commented Out Example Code

Multiple examples had their main code commented out and are now uncommented:

  • examples/cloudflare-workers-hono/src/index.ts
  • examples/cloudflare-workers-inline-client/src/index.ts
  • examples/cloudflare-workers/src/index.ts

Were these intentionally disabled before? If so, why are they now being re-enabled? This suggests these examples were broken and are now fixed, but there's no context.

9. Startup Sequence Comment Quality

rivetkit-typescript/packages/rivetkit/runtime/index.ts:93-105:

The comment is very helpful but violates CLAUDE.md guidelines:

"Write comments as normal, complete sentences. Avoid fragmented structures with parentheticals and dashes"

Comment has multiple dashes and fragmented structure. Per the new CLAUDE.md rule (line 194), this should be rewritten as complete sentences.


✅ Positive Changes

1. Two-Phase Startup Architecture

The new startup architecture (Runtime.create()startServerless()/startRunner()) is well-designed:

  • Separates infrastructure initialization from mode configuration
  • Allows lazy initialization of serverless mode
  • Makes the architecture more flexible

2. Consistent Environment Variable Naming

While breaking, the RIVET_* naming is more consistent with the public API and product branding.

3. Serverless as Default

Making serverless the default for development is a good DX improvement, though resource consumption should be documented.

4. Package Updates

Updating to latest Cloudflare Workers types, Next.js 16, React 19, and Hono is good maintenance.


🔒 Security Assessment

No new security vulnerabilities identified.

  • Environment variable consolidation follows security best practices
  • SQLite storage via Durable Objects maintains proper isolation
  • Engine preemptive startup doesn't introduce new attack surface
  • Configuration validation prevents misconfigurations

📝 Recommendations

Before Merging:

  1. ✅ Re-enable Cloudflare Workers driver tests
  2. ✅ Fix function naming: getRivetkitInspector*getRivetInspector*
  3. ✅ Add timeout handling to engine startup
  4. ✅ Create migration guide for environment variables
  5. ✅ Document SQLite migration path for Cloudflare Workers
  6. ✅ Fix documentation path references

After Merging:

  1. Monitor engine spawn failures in production
  2. Track port 6420 conflicts in containerized environments
  3. Monitor Next.js dev mode startup time impact
  4. Add integration tests for new startup sequence
  5. Consider adding deprecation period for old env vars
  6. Document Hono dependency requirements in Next.js

📊 Impact Summary

  • Breaking Changes: Environment variables, Cloudflare Workers storage
  • Performance Impact: Engine spawns earlier, may affect startup time
  • Test Coverage: Significantly decreased (tests commented out)
  • Documentation: Requires updates for migrations
  • Developer Experience: Improved with serverless defaults, but needs resource docs

Overall Assessment: The architectural changes are solid, but the lack of test coverage and breaking changes without migration guides are concerning. I recommend addressing the critical issues before merging, especially re-enabling tests and creating migration documentation.


Review conducted by Claude Code analyzing 3,492 additions and 2,981 deletions across 87 files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants