Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Nov 14, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Nov 17, 2025 7:36pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 17, 2025 7:36pm
rivet-inspector Ignored Ignored Preview Nov 17, 2025 7:36pm
rivet-site Ignored Ignored Preview Nov 17, 2025 7:36pm

Copy link
Contributor Author

MasterPtato commented Nov 14, 2025

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


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

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
Copy link

claude bot commented Nov 14, 2025

PR Review: Documentation Updates

Summary

This PR adds valuable documentation for the actor lifecycle and serverless flows while removing an outdated script reference from CLAUDE.md. Overall, this is a positive contribution to the project's documentation.

Positive Aspects

Clear Visual Documentation: The Mermaid sequence diagrams effectively illustrate complex system interactions
Comprehensive Coverage: Both ideal and edge-case scenarios are documented ("Messy Serverless Flow")
Good Organization: Separate files for different concerns (actor lifecycle vs serverless flows)
Cleanup: Removed reference to outdated update_workspace.ts script

Suggestions for Improvement

1. Missing File Extension in CLAUDE.md (Line 111)

The removed line references a non-existent file path. Consider verifying if this script removal is intentional or if documentation elsewhere needs updating.

2. Documentation Clarity

ACTOR_LIFECYCLE_FLOW.md:

  • Line 28: A->>U: appears incomplete - should specify what is returned to the user (e.g., A->>U: 201 Created (actor_id))
  • Lines 40-41, 44-45: Empty message arrows (RWF->>RWS: and RWS->>RWF:) could benefit from labels describing what's being communicated
  • Consider adding a brief introduction paragraph explaining what this diagram represents and when to reference it

SERVERLESS_FLOW.md:

  • Lines 68-83: Commented-out Mermaid code should either be removed or have a comment explaining why it's kept
  • Line 95: Commented participant (%% participant A as API) - remove if unused
  • Multiple empty message arrows throughout - consider labeling them for clarity
  • The "Messy Serverless Flow" diagram is quite complex. Consider:
    • Adding numbered steps or annotations
    • Breaking into multiple smaller diagrams
    • Adding a summary table of edge cases handled

3. Missing Context

Consider adding:

  • A top-level README in docs/engine/ explaining the documentation structure
  • Links between related documents
  • Glossary for abbreviations (AWF, RWF, RWS, etc.) or expand them on first use
  • When to use each diagram (troubleshooting guide, onboarding, etc.)

4. Mermaid Syntax Consistency

The diagrams use consistent theming (theme: mc, look: classic) which is good, but consider:

  • Adding version comments if specific Mermaid features are required
  • Testing rendering in common viewers (GitHub, GitLab, VS Code)
  • Some empty arrows might not render clearly in all viewers

Minor Issues

  • Line consistency: Both files end without trailing newlines (which is fine, but verify your .editorconfig preferences)
  • Commenting style: Mix of %% comments and uncommented sections in SERVERLESS_FLOW.md

Testing Recommendations

Since these are documentation files:

  1. Verify Mermaid diagrams render correctly on GitHub
  2. Review with team members unfamiliar with the system to ensure clarity
  3. Validate that the flows match current implementation

Verdict

LGTM with suggestions - The documentation adds significant value. The suggestions above are mostly about polish and clarity rather than blocking issues. Consider addressing the empty message arrows and adding contextual documentation before merging.


Review generated with assistance from Claude Code

@claude
Copy link

claude bot commented Nov 14, 2025

PR Review: Documentation Updates for Rivet Engine

This PR adds comprehensive documentation for key Rivet engine components. The documentation is well-structured and provides valuable insights into the system architecture. Here's my detailed review:

Strengths

Excellent Visual Documentation

  • The Mermaid sequence diagrams in ACTOR_LIFECYCLE.md and SERVERLESS.md provide clear visualizations of complex workflows
  • The "Messy Serverless Flow" diagram is particularly valuable as it documents edge cases and race conditions that developers need to understand

Comprehensive Coverage

  • Documents critical components: Guard (routing/proxying), actor lifecycle, hibernating websockets, runner shutdown, and serverless flows
  • The terminology document provides a helpful reference for understanding abbreviations used throughout

Clear Structure

  • Documentation is well-organized in docs/engine/ directory
  • Cross-references between docs (e.g., GUARD.md → HIBERNATING_WS.md) help readers navigate

Issues & Suggestions

1. Empty Documentation Files

Severity: Medium

Three files are created but left empty:

  • docs/engine/ACTOR_KV.md
  • docs/engine/GASOLINE.md
  • docs/engine/GATEWAY.md

Recommendation: Either add content to these files or remove them from this PR. Empty files can be confusing for readers. Based on the existing docs:

  • GASOLINE.md should document the workflow engine mentioned in CLAUDE.md
  • GATEWAY.md appears to be a duplicate concept with GUARD.md - clarify the distinction or merge

2. Typo in HIBERNATING_WS.md

Severity: Low

Line 25:

- When a runner receives a CommandStartActor message via the runner protocol, it contains information about which hiberating requests are still active.

Should be: hibernating (not hiberating)

3. Inconsistent Terminology

Severity: Low

GUARD.md line 8 uses "Gateway" in the title "Rivet Guard" context, but the document differentiates between Guard and Gateway. From reading the docs:

  • Guard = HTTP routing/proxying layer
  • Gateway = Used in sequence diagrams as a participant (appears to be the same as Guard?)

Recommendation: Add clarification to TERMINOLOGY.md about Guard vs Gateway distinction, or use consistent naming in diagrams.

4. CLAUDE.md Change Context Missing

Severity: Low

The PR removes these lines from CLAUDE.md:

# When adding a new package to the workspace
deno run -A scripts/cargo/update_workspace.ts

Question: Is this script no longer needed, or was this workflow changed? Consider adding a commit message explanation or updating relevant documentation if the workflow changed.

5. Incomplete Step in HIBERNATING_WS.md

Severity: Low

Line 14 (step 9) starts with a bare numbered list without context:

9. 
	- If the actor is awoken from any other source...

Recommendation: Add a brief intro sentence after "9." like:

9. When the websocket is hibernating, the following can occur:
	- If the actor is awoken from any other source...

6. SERVERLESS.md Commented-Out Code

Severity: Low

Lines 68-83 in "Ideal Serverless Flow" contain commented-out Mermaid code for actor sleep flow.

Recommendation: Either:

  • Uncomment if this flow should be documented
  • Remove if it's not part of the "ideal" flow
  • Add a comment explaining why it's omitted (e.g., "Actor sleep flow omitted for clarity - see Messy Serverless Flow")

Minor Suggestions

  1. GUARD.md line 36: "is websocket" → "its websocket"

    - The runner sends HTTP responses and websocket messages back to Rivet through is websocket via the runner protocol
  2. Cross-reference consistency: Consider adding a header comment in each file linking to related docs, e.g.:

    # Hibernating Websockets
    
    Related: [GUARD.md](GUARD.md), [ACTOR_LIFECYCLE.md](ACTOR_LIFECYCLE.md)
  3. RUNNER_SHUTDOWN.md: Consider adding a sequence diagram similar to other docs for visual consistency

Performance, Security & Testing

Not Applicable: This PR only adds documentation files, so there are no code changes to review for performance, security vulnerabilities, or test coverage.

Summary

This is a valuable documentation addition that will help developers understand Rivet's complex distributed actor system. The main action items are:

  1. ✅ Fix the typo: "hiberating" → "hibernating"
  2. ✅ Fix the grammar: "through is websocket" → "through its websocket"
  3. 🔧 Decide whether to add content or remove empty .md files
  4. 📝 Clarify Guard vs Gateway terminology
  5. 🧹 Clean up commented-out Mermaid code in SERVERLESS.md

Overall: Approve with minor changes recommended

Great work documenting these complex flows! 🎉

@claude
Copy link

claude bot commented Nov 17, 2025

Code Review: Documentation Updates

Overview

This PR adds comprehensive technical documentation for core Rivet Engine components. The additions are valuable for understanding the system architecture, but there are several issues to address.


Critical Issues

1. Empty GASOLINE.md File

Location: docs/engine/GASOLINE.md
Issue: The file is completely empty (0 bytes added according to the diff).

According to CLAUDE.md:32, Gasoline is described as "Workflow Engine" which "Handles complex multi-step operations with reliability and observability." This is a critical component that should be documented.

Recommendation: Either:

  • Add proper documentation for Gasoline/workflow engine
  • Remove this file from the PR until content is ready
  • Add a TODO comment indicating planned content

Issues to Fix

2. Typo: "hiberating" → "hibernating"

Location: docs/engine/HIBERNATING_WS.md:25

- When a runner receives a CommandStartActor message via the runner protocol, it contains information about which hiberating requests are still active.
+ When a runner receives a CommandStartActor message via the runner protocol, it contains information about which hibernating requests are still active.

3. Grammar Error: "through is websocket" → "through its websocket"

Location: docs/engine/GUARD.md:36

- The runner sends HTTP responses and websocket messages back to Rivet through is websocket via the runner protocol
+ The runner sends HTTP responses and websocket messages back to Rivet through its websocket via the runner protocol

4. Incomplete Text: "how maximum returned keys" → "how many maximum returned keys"

Location: docs/engine/ACTOR_KV.md:32

- Limit - how maximum returned keys
+ Limit - maximum number of returned keys

Documentation Quality Suggestions

5. ACTOR_KV.md - Clarify Version Metadata

Location: docs/engine/ACTOR_KV.md:9

The document states: "version being a string byte array denoting the version of the Rivet Engine"

Question: Is this the Rivet Engine version or a version number for the KV entry itself (for optimistic concurrency control)? This should be clarified since version fields in KV stores are typically used for conflict detection.

6. HIBERNATING_WS.md - Step 9 Formatting

Location: docs/engine/HIBERNATING_WS.md:14

Step 9 has unusual formatting with no introductory text:

9. 
	- If the actor is awoken from any other source, go to step 6.

Recommendation:

9. Once hibernating, one of the following occurs:
	- If the actor is awoken from any other source, go to step 6. We do not send a ToClientWebSocketOpen message in this case

7. ACTOR_LIFECYCLE.md - Empty Message Arrows

Multiple Locations: Lines 40-41, 44-45, 53-54, 64-65, etc.

The Mermaid diagrams contain many unlabeled arrows like:

RWF->>RWS:
RWS->>R:
Loading

Recommendation: Add message labels for clarity, e.g.:

RWF->>RWS: Proxy StartActor
RWS->>R: Send StartActor command
Loading

This applies to multiple diagrams throughout.

8. Missing Context for CLAUDE.md Change

The PR removes:

# When adding a new package to the workspace
deno run -A scripts/cargo/update_workspace.ts

Question: Was this script removed from the codebase? Or is this workflow no longer necessary? Consider explaining in the PR description or commit message.


Strengths

Excellent visual documentation - The Mermaid sequence diagrams effectively communicate complex distributed system flows
Edge case coverage - The "Messy Serverless Flow" documents important race conditions and edge cases
Good cross-referencing - Documents reference each other appropriately
Terminology guide - TERMINOLOGY.md provides helpful abbreviation reference
Practical examples - GUARD.md shows concrete routing patterns with headers and paths


Recommendations for Future Work

  1. Add a README in docs/engine/ explaining the documentation structure and recommended reading order
  2. Expand TERMINOLOGY.md with Guard vs Gateway clarification (both terms are used somewhat interchangeably)
  3. Consider adding troubleshooting sections to key documents showing common failure modes
  4. Add examples to ACTOR_KV.md showing actual usage patterns

Verdict

Needs Changes - Please address the critical issue (#1 empty GASOLINE.md) and typos/grammar errors (#2-4) before merging. The documentation quality suggestions can be addressed in follow-up PRs if preferred.

Overall, this is valuable technical documentation that will help developers understand Rivet's architecture. Thank you for taking the time to document these complex systems!


Reviewed per repository guidelines in CLAUDE.md

@claude
Copy link

claude bot commented Nov 17, 2025

PR Review: Documentation Updates

This PR adds comprehensive documentation for the Rivet Engine's actor system and workflow architecture. Overall, this is excellent documentation that will greatly help developers understand the system. Here's my detailed feedback:

Strengths

  1. Excellent Visual Documentation: The Mermaid sequence diagrams in ACTOR_LIFECYCLE.md and SERVERLESS.md are incredibly helpful for understanding complex distributed system flows.

  2. Comprehensive Coverage: The docs cover critical aspects of the system including actor lifecycle, KV storage, hibernating websockets, and serverless flows.

  3. Practical Examples: The "Messy Serverless Flow" diagram is particularly valuable as it shows real-world race conditions and edge cases, not just the happy path.

  4. Clear Terminology: TERMINOLOGY.md provides essential definitions that help disambiguate concepts like "Runner WF" vs "Actor WF".

Issues & Suggestions

1. Typo in HIBERNATING_WS.md (Line 25)

File: docs/engine/HIBERNATING_WS.md:25

which hiberating requests are still active.

Should be: "hibernating" (missing 'n')

2. Grammar Issue in GUARD.md (Line 36)

File: docs/engine/GUARD.md:36

through is websocket via the runner protocol

Should be: "through its websocket via the runner protocol"

3. Incomplete Sentence in HIBERNATING_WS.md (Line 14)

File: docs/engine/HIBERNATING_WS.md:14

Step 9 has a numbered item that appears incomplete:

9. 
	- If the actor is awoken from any other source, go to step 6...

The "9." should probably be removed since it's immediately followed by bullet points, or the structure should be clarified.

4. Missing Grammar Article in HIBERNATING_WS.md (Line 8)

File: docs/engine/HIBERNATING_WS.md:8

Guard sends the runner a ToClientWebSocketOpen message a via the runner protocol

Should be: "Guard sends the runner a ToClientWebSocketOpen message via the runner protocol" (remove extra "a")

5. TODO Comment in Schema File

File: engine/sdks/schemas/runner-protocol/v3.bare:15

There's a TODO comment:

# TODO: Rename to update_ts
createTs: i64

While the TODO is helpful, it would be good to track this as an issue or address it soon to avoid confusion about the field's actual meaning (is it create timestamp or update timestamp?). The documentation in ACTOR_KV.md:9 states it's a "create timestamp," but the TODO suggests it should be "update timestamp" which would be more accurate for a KV system.

6. Inconsistency in ACTOR_KV.md (Line 32)

File: docs/engine/ACTOR_KV.md:32

- Limit - how maximum returned keys

Should be: "Limit - the maximum number of returned keys" or "Limit - maximum number of returned keys"

7. Minor CLAUDE.md Removal

The removal of the deno run -A scripts/cargo/update_workspace.ts command from CLAUDE.md appears intentional (likely automated now or deprecated). This is fine if the script is no longer needed.

Documentation Quality Suggestions

1. Cross-References

Consider adding more cross-references between documents. For example:

  • ACTOR_LIFECYCLE.md could reference ACTOR_KV.md for KV operations
  • GUARD.md could link to ACTOR_LIFECYCLE.md for full request flow context

2. Error Handling Documentation

ACTOR_KV.md mentions that operations can return errors, but it would be helpful to document:

  • Common error scenarios (e.g., key not found, storage full, etc.)
  • Whether errors are retryable
  • Error handling best practices

3. Performance Considerations

Consider adding a section about:

  • KV operation performance characteristics (latency, throughput)
  • Best practices for batch operations vs individual operations
  • Limitations (max key/value sizes, max number of keys, etc.)

4. Ordering Guarantees

File: docs/engine/ACTOR_KV.md:68

The docs mention:

Rivet makes no attempt to order or deduplicate the responses to KV requests

This is important! Consider making this more prominent (perhaps in a warning callout) since developers might assume ordering guarantees.

Security & Performance Notes

No security or performance concerns identified in this documentation-only PR. The docs appropriately warn about dangerous operations like Drop which deletes all KV data.

Test Coverage

N/A - This is a documentation-only PR with no code changes (except a minor schema TODO comment).

Overall Assessment

This is high-quality documentation that significantly improves the project's developer experience. The issues identified are minor (mostly typos and grammar). I recommend:

  1. Fix the typos and grammar issues listed above
  2. Consider whether the createTsupdate_ts rename should happen now or be tracked as a separate issue
  3. Merge after addressing the typos

Great work on these docs! The sequence diagrams especially will be invaluable for onboarding and debugging.

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