Skip to content

Conversation

@graphite-app
Copy link
Contributor

@graphite-app graphite-app bot commented Jan 2, 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:

@vercel
Copy link

vercel bot commented Jan 2, 2026

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

Project Deployment Review Updated (UTC)
rivetkit-serverless Error Error Jan 2, 2026 10:05am
3 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Jan 2, 2026 10:05am
rivet-inspector Ignored Ignored Preview Jan 2, 2026 10:05am
rivet-site Ignored Ignored Preview Jan 2, 2026 10:05am

@graphite-app graphite-app bot closed this Jan 2, 2026
@graphite-app graphite-app bot deleted the gtmq_spec_89b5d6_1767348267909-e997a5c8-069b-4a7a-ae2d-3c76e4546659 branch January 2, 2026 10:05
@claude
Copy link

claude bot commented Jan 2, 2026

Pull Request Review: feat(rivetkit): allow including namespace & token in endpoint

Summary

This PR adds support for embedding namespace and token credentials directly in the endpoint URL using HTTP Basic Auth syntax (e.g., https://namespace:[email protected]). The implementation is well-structured with good test coverage and proper validation.

✅ Strengths

  1. Excellent Test Coverage: The test file (endpoint-parser.test.ts) is comprehensive with 21 test cases covering:

    • Full auth syntax with namespace and token
    • Namespace-only scenarios
    • No-auth scenarios
    • Edge cases (percent-encoding, ports, paths)
    • Error handling (invalid URLs, query strings, fragments, token-without-namespace)
  2. Good Code Organization: The refactoring properly separates concerns:

    • EndpointSchema for parsing
    • resolveEndpoint() for resolution logic
    • transformClientConfig() and transformEngineConfig() for transformations
    • zodCheckDuplicateCredentials() for validation
  3. Proper URL Handling: Correctly uses decodeURIComponent() to handle percent-encoded characters in credentials (endpoint-parser.ts:49-50).

  4. Good Documentation: Clear JSDoc comments explaining the auth syntax formats and behavior.


🔍 Issues Found

1. Typo Fix (Minor)

File: rivetkit-typescript/packages/rivetkit/src/drivers/engine/mod.ts:12

Good catch fixing the typo from EngingConfigSchema to EngineConfigSchema! ✅

2. Missing Test Coverage for zodCheckDuplicateCredentials

File: rivetkit-typescript/packages/rivetkit/src/utils/endpoint-parser.test.ts

The zodCheckDuplicateCredentials() function (endpoint-parser.ts:94-118) has no direct test coverage. While it's exercised indirectly through config schema transforms, explicit tests would improve maintainability.

Recommendation: Add tests to verify:

  • Error when namespace specified in both endpoint and config
  • Error when token specified in both endpoint and config
  • Success when no duplicates exist

3. Type Safety Issue in convertRegistryConfigToClientConfig

File: rivetkit-typescript/packages/rivetkit/src/client/config.ts:117-131

The function constructs the object directly rather than parsing (see comment on line 113-115), which bypasses type checking. If ClientConfig type changes, this function won't catch mismatches.

Recommendation: Use satisfies ClientConfig to maintain type safety:

return {
  endpoint: config.endpoint,
  token: config.token,
  namespace: config.namespace,
  runnerName: config.runner.runnerName,
  headers: config.headers,
  encoding: "bare",
  getUpgradeWebSocket: undefined,
  disableMetadataLookup: true,
} satisfies ClientConfig;

This ensures the object matches the ClientConfig type without re-parsing.

4. Potential Security Consideration (Low Priority)

File: rivetkit-typescript/packages/rivetkit/src/utils/endpoint-parser.ts:21-74

The endpoint parser allows embedding credentials in URLs, which could appear in logs, error messages, or browser history if URLs are constructed client-side.

Recommendations:

  • Add documentation warning about credential exposure risks
  • The code already sanitizes URLs correctly (via url.toString() after clearing auth)
  • Consider adding a comment about the security trade-off

💡 Code Quality Observations

  1. Good Pattern: The separation of Base schemas (without transforms) from transformed schemas is excellent. This allows composition without double-transformation issues.

  2. Validation Logic: The check for "token without namespace" (endpoint-parser.ts:53-60) is a good UX improvement - prevents invalid states.

  3. Query String/Fragment Rejection: Lines 26-45 correctly reject query strings and fragments to avoid conflicts with runtime parameters.

  4. ENV Variable Fallback Chain: The resolution logic properly implements fallback chains:

    • config.endpointgetRivetEngine()getRivetEndpoint()
    • For namespace/token: endpoint-embeddedconfigenv varsdefault

🎯 Performance Considerations

No performance issues identified. URL parsing is fast, and validation happens during config initialization (not in hot path).


🔒 Security Assessment

Overall: Good

  • ✅ Properly handles percent-encoded credentials
  • ✅ Rejects potentially problematic URL components (query strings, fragments)
  • ✅ Validates against duplicate credentials (prevents confusion/mistakes)
  • ⚠️ Consider documenting credential exposure risks (see issue [SVC-2483] Remove hardcoded uses of rivet.gg #4 above)

📝 Recommendations Summary

Must Fix: None (all issues are minor)

Should Fix:

  1. Add test coverage for zodCheckDuplicateCredentials
  2. Add satisfies ClientConfig to convertRegistryConfigToClientConfig

Nice to Have:
3. Add security documentation about credential embedding trade-offs


✨ Overall Assessment

Rating: Approve with Minor Suggestions

This is a well-implemented feature with excellent test coverage and thoughtful error handling. The refactoring improves code organization, and the new functionality is useful for simplified configuration. The issues identified are minor and don't block merging.

Great work! 🎉

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