Skip to content

fix(rivetkit): detect unsupported inspector workflows#5455

Open
jog1t wants to merge 3 commits into
mainfrom
fix/inspector-workflow-capability
Open

fix(rivetkit): detect unsupported inspector workflows#5455
jog1t wants to merge 3 commits into
mainfrom
fix/inspector-workflow-capability

Conversation

@jog1t

@jog1t jog1t commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • report inspector workflows as unsupported when no history callback is registered
  • keep supported workflows with empty history distinct from unsupported workflows
  • align native and WASM behavior
  • add unit and cross-runtime WebSocket regression coverage

Verification

  • git diff --check
  • npx --yes @biomejs/biome@2.3.0 check rivetkit-typescript/packages/rivetkit/tests/driver/actor-inspector.test.ts (passes with one pre-existing non-null assertion warning)

Rust and driver tests were not run because the orb does not have Rust or workspace dependencies installed.

Co-authored-by: Kacper <kacper@rivet.gg>
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5455 July 20, 2026 22:34 Destroyed
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review

Solid, well-scoped fix. The core disambiguation logic (workflow_dispatch_result / is_dropped_reply_error in dispatch.rs, plus Reply<T>'s Drop impl that auto-sends actor/dropped_reply) already existed; this PR correctly wires the NAPI and wasm adapters to participate in it by dropping the reply when no get_workflow_history/replay_workflow callback is registered, instead of sending Ok(None) (which was indistinguishable from "callback exists but returned no history"). The flow through to inspector_ws.rs/inspector.rs (isWorkflowEnabled) is consistent end-to-end.

Correctness

  • NAPI and wasm changes are symmetric, both now drop(reply) in the no-callback branch, matching the #[must_use = "... or dropping it sends actor/dropped_reply"] convention already established elsewhere in this codebase (event.rs). No leftover reachable code path sends Ok(None) for the unsupported case anymore.
  • The wasm change correctly hoists the let Some(callback) = ... else { drop(reply); return; } check outside the async move block, so the borrow of callbacks and the early-return both happen before entering the callback-invocation future. No behavior change for the callback-present path.
  • The updated NAPI test (workflow_requests_without_callbacks_drop_replies) correctly asserts on group()/code() == "actor"/"dropped_reply" rather than a magic string, matching how RivetTransportError::extract is used elsewhere in the same file.

Test coverage

  • The new driver test exercises both the WebSocket Init message and a live WorkflowReplayRequest/WorkflowReplayResponse round trip for a non-workflow actor, complementing the existing HTTP /inspector/workflow-history coverage (which already asserted isWorkflowEnabled === false for the counter actor). Since it's registered under describeDriverMatrix, it runs against both native (NAPI) and wasm drivers, satisfying the "cross-runtime" claim in the PR description without a separate wasm-only test file.
  • One gap: there's no new test asserting the positive case for WorkflowReplayResponse over the websocket specifically (isWorkflowEnabled === true when a callback is registered but returns no history for a given entryId). Existing HTTP-path tests around lines 738/784 likely already exercise the callback-present branch, but a websocket-level assertion would close the loop symmetrically with the new negative test.

Style

  • Minor/non-blocking: drop(reply); immediately before return; is redundant (the Reply<T> would be dropped on scope exit regardless), but it's harmless and arguably clarifies intent alongside the accompanying comment.

Nothing blocking. Nice job aligning the two adapter layers with the pre-existing core semantics.

jog1t commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review in bc25e85:

  • missing workflow replay callbacks now drop the reply in both NAPI and WASM, so replay responses report isWorkflowEnabled: false
  • updated the NAPI unit test to cover dropped history and replay replies
  • extended the cross-runtime WebSocket test to assert WorkflowReplayResponse
  • Rust formatting was already fixed in cead2c1; Rustfmt CI passes

Validation: focused NAPI unit test passed; formatting and pre-commit hooks passed.

@railway-app

railway-app Bot commented Jul 20, 2026

Copy link
Copy Markdown

🚅 Deployed to the rivet-pr-5455 environment in rivet-frontend

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web Jul 21, 2026 at 1:08 am
frontend-cloud 😴 Sleeping (View Logs) Web Jul 21, 2026 at 1:08 am
frontend-inspector 😴 Sleeping (View Logs) Web Jul 21, 2026 at 1:06 am
kitchen-sink 😴 Sleeping (View Logs) Web Jul 21, 2026 at 1:05 am
ladle ✅ Success (View Logs) Web Jul 20, 2026 at 10:32 pm
mcp-hub ✅ Success (View Logs) Web Jul 20, 2026 at 10:30 pm

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