Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

Conversation

jog1t
Copy link
Collaborator

@jog1t jog1t commented Oct 20, 2025

No description provided.

@vercel
Copy link

vercel bot commented Oct 20, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-next-js-max Ready Ready Preview Comment Oct 21, 2025 6:47pm
rivetkit-serverless Ready Ready Preview Comment Oct 21, 2025 6:47pm

Copy link
Collaborator Author

jog1t commented Oct 20, 2025


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

claude bot commented Oct 20, 2025

PR Review: Execute Actor Actions Through Inspector

Overview

This PR adds the ability to execute actor actions through the inspector API, which is useful for debugging and testing actors. The implementation creates a temporary HTTP connection and executes the action through it.

Code Quality & Best Practices

✅ Strengths

  • Consistent with existing patterns: The implementation follows similar patterns to existing inspector endpoints (e.g., /state, /db)
  • Proper validation: Uses sValidator with zod schema for input validation
  • Type safety: Properly typed accessor interface with executeAction method
  • Clean API design: Simple POST endpoint with intuitive request/response structure

⚠️ Areas for Improvement

1. Connection Cleanup (Critical)

Location: packages/rivetkit/src/actor/instance.ts:235-250

The temporary connection created for action execution is never cleaned up:

executeAction: async (name, params) => {
    const conn = await this.createConn(
        {
            socketId: generateConnSocketId(),
            driverState: { [ConnDriverKind.HTTP]: {} },
        },
        undefined,
        undefined,
    );

    return this.executeAction(
        new ActionContext(this.actorContext, conn),
        name,
        params || [],
    );
},

Issue: The connection is created but never removed from this.#connections, which could lead to:

  • Memory leaks over time
  • Incorrect connection counts in inspector
  • Stale connections accumulating

Recommendation: Add cleanup in a try-finally block:

executeAction: async (name, params) => {
    const conn = await this.createConn(
        {
            socketId: generateConnSocketId(),
            driverState: { [ConnDriverKind.HTTP]: {} },
        },
        undefined,
        undefined,
    );

    try {
        return await this.executeAction(
            new ActionContext(this.actorContext, conn),
            name,
            params || [],
        );
    } finally {
        // Clean up the temporary connection
        await this.removeConn(conn.id);
    }
},

2. Error Handling

Location: packages/rivetkit/src/inspector/actor.ts:254-268

The endpoint doesn't handle errors from action execution:

.post(
    "/action",
    sValidator(
        "json",
        z.object({ name: z.string(), params: z.array(z.any()).optional() }),
    ),
    async (c) => {
        const { name, params } = c.req.valid("json");
        const result = await c.var.inspector.accessors.executeAction(
            name,
            params,
        );
        return c.json({ result }, 200);
    },
);

Issue: If the action throws an error, it will bubble up unhandled. While middleware might catch it, it's better to handle it explicitly for consistency with other endpoints (see /db endpoint for reference).

Recommendation: Add try-catch similar to the /db endpoint:

.post(
    "/action",
    sValidator(
        "json",
        z.object({ name: z.string(), params: z.array(z.any()).optional() }),
    ),
    async (c) => {
        const { name, params } = c.req.valid("json");
        try {
            const result = await c.var.inspector.accessors.executeAction(
                name,
                params,
            );
            return c.json({ result }, 200);
        } catch (error) {
            return c.json({ error: (error as Error).message }, 500);
        }
    },
);

3. Import Type Change

Location: packages/rivetkit/src/actor/instance.ts:23

Changed from type import to regular import:

-import type { ActionContext } from "./action";
+import { ActionContext } from "./action";

Note: This is correct since ActionContext is now instantiated at runtime. However, verify this doesn't cause circular dependency issues during bundling.

Performance Considerations

Connection Creation Overhead

Creating a full connection object just to execute an action adds overhead. For a debugging/inspection tool, this is acceptable, but worth noting:

  • Connection creation involves state initialization if connState is enabled
  • Connection lifecycle hooks (onConnect) won't be called (params is undefined)

Consideration: Document that inspector action execution bypasses normal connection lifecycle hooks.

Security Concerns

⚠️ Authentication/Authorization

The inspector API should already be protected by authentication middleware, but ensure:

  1. This endpoint is only accessible in development/debugging contexts
  2. The inspector authentication is properly configured in production environments
  3. Consider rate limiting to prevent abuse

Parameter Validation

Using z.array(z.any()) for params means any data can be passed. This is likely intentional for flexibility, but:

  • Malicious or malformed parameters could cause unexpected behavior
  • Consider documenting that params are not validated beyond being an array

Test Coverage

❌ Missing Tests

The PR doesn't include tests for the new functionality. The existing test file (packages/rivetkit/src/driver-test-suite/tests/actor-inspector.ts) is currently skipped but should include a test for action execution:

Recommendation: Add a test case like:

test("should execute actor action", async (c) => {
    const { client, endpoint } = await setupDriverTest(c, driverTestConfig);

    const handle = await client.counter.create(["test-execute-action"]);
    const actorId = await handle.resolve();

    const http = createActorInspectorClient(`${endpoint}/actors/inspect`, {
        headers: {
            Authorization: `Bearer token`,
            [HEADER_ACTOR_QUERY]: JSON.stringify({
                getForId: { name: "counter", actorId },
            } satisfies ActorQuery),
        },
    });

    const response = await http.action.$post({
        json: {
            name: "increment",
            params: [5],
        },
    });
    expect(response.status).toBe(200);

    const data = await response.json();
    expect(data).toHaveProperty("result");
});

Additional Recommendations

  1. Documentation: Add JSDoc comments to the executeAction accessor method explaining:

    • That it creates a temporary connection
    • That connection lifecycle hooks are not invoked
    • Intended use case (debugging/inspection)
  2. Logging: Consider adding debug logging when actions are executed via inspector (similar to packages/rivetkit/src/actor/instance.ts:1331-1335)

  3. Type Safety: The return type is Promise<unknown>. Consider if there's a way to preserve type information, though this may not be feasible for a generic inspector API.

Summary

This is a useful feature for debugging actors. The main concern is the connection leak which should be addressed before merging. The error handling improvement would also make the API more robust and consistent with other endpoints.

Recommended Actions Before Merge:

  1. ✅ Add connection cleanup (critical)
  2. ✅ Add error handling (recommended)
  3. ✅ Add tests (recommended)
  4. ✅ Add documentation (nice-to-have)

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 20, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/actor@1416

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/cloudflare-workers@1416

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/core@1416

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/db@1416

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/framework-base@1416

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/next-js@1416

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/react@1416

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivetkit@1416

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/sql-loader@1416

commit: c1e5de1

@claude
Copy link

claude bot commented Oct 20, 2025

Pull Request Review: Execute Actor Actions Through Inspector

Summary

This PR adds the ability to execute actor actions through the inspector API. The implementation creates a temporary HTTP connection for each action execution, similar to how HTTP actions are handled in the regular actor router.

Code Quality & Best Practices

✅ Strengths

  1. Clean abstraction: The executeAction accessor is properly added to the ActorInspectorAccessors interface
  2. Consistent pattern: Uses the same HTTP connection pattern as router-endpoints.ts:466-475
  3. Proper cleanup: Uses try-finally block to ensure connection cleanup via __connDisconnected
  4. Type safety: Proper TypeScript types with zod validation for the request payload

⚠️ Issues & Concerns

1. Missing Error Handling (High Priority)

The /action endpoint at packages/rivetkit/src/inspector/actor.ts:254-268 lacks error handling. Compare with the /db/execute endpoint at line 242-252 which has proper try-catch:

// Current implementation - no error handling
.post("/action", async (c) => {
    const { name, params } = c.req.valid("json");
    const result = await c.var.inspector.accessors.executeAction(name, params);
    return c.json({ result }, 200);
})

// Should be (following pattern from /db/execute):
.post("/action", async (c) => {
    const { name, params } = c.req.valid("json");
    try {
        const result = await c.var.inspector.accessors.executeAction(name, params);
        return c.json({ result }, 200);
    } catch (error) {
        return c.json({ error: (error as Error).message }, 500);
    }
})

Impact: Unhandled errors will cause the inspector endpoint to crash or return unhelpful errors to clients.

2. Import Change Inconsistency (Low Priority)

The change from type import to regular import for ActionContext at packages/rivetkit/src/actor/instance.ts:23:

-import type { ActionContext } from "./action";
+import { ActionContext } from "./action";

This is necessary since ActionContext is now instantiated at runtime (line 248). However, consider if other type-only imports in the same file might benefit from being explicit import type for build optimization.

3. Connection Parameters (Medium Priority)

In the executeAction implementation at packages/rivetkit/src/actor/instance.ts:237-244, the connection is created with undefined for both parameters and the request object:

const conn = await this.createConn(
    { socketId, driverState: { [ConnDriverKind.HTTP]: {} } },
    undefined,  // parameters
    undefined,  // request
);

Questions:

  • Could this cause issues if actions rely on connection parameters?
  • Should there be a way to pass parameters through the inspector API?
  • Are there any validators that might fail with undefined parameters?

4. Potential Resource Leak (Low Priority)

While the finally block ensures cleanup, if executeAction throws before the connection is created (unlikely but possible), the error handling could be more defensive. Consider:

executeAction: async (name, params) => {
    const socketId = generateConnSocketId();
    let conn: Conn<...> | undefined;
    
    try {
        conn = await this.createConn(...);
        return await this.executeAction(
            new ActionContext(this.actorContext, conn),
            name,
            params || [],
        );
    } finally {
        if (conn) {
            this.__connDisconnected(conn, true, socketId);
        }
    }
}

Performance Considerations

Connection Overhead

Each action execution creates and destroys a connection. This is acceptable for inspector/debugging purposes but should be documented. Consider adding a JSDoc comment explaining this design choice.

No Connection Pooling

Since this is for inspector debugging, the lack of connection pooling is acceptable. However, if this becomes a high-frequency operation, consider implications.

Security Concerns

⚠️ CORS Configuration in Example

The example at examples/counter/src/server.ts:4-7 uses a permissive CORS policy:

cors: {
    origin: (o) => o,  // Allows ANY origin!
    credentials: true,
}

Recommendation: Add a comment warning that this is for development only:

inspector: {
    cors: {
        // WARNING: Development only - allows all origins
        // In production, specify allowed origins explicitly
        origin: (o) => o,
        credentials: true,
    },
},

Inspector Access Control

There doesn't appear to be any authentication/authorization for the inspector endpoints. This is likely intentional for development, but ensure it's documented that inspector should not be exposed in production without proper security measures.

Test Coverage

❌ Missing Tests

No tests were added for this functionality. Recommended test cases:

  1. Success case: Execute a valid action and verify the result
  2. Error case: Execute a non-existent action
  3. Error case: Execute an action that throws an error
  4. Connection cleanup: Verify connection is properly cleaned up after execution
  5. Parameter passing: Verify params are correctly passed to the action

Reference pattern: Look at existing action tests (if any) or create new test file: packages/rivetkit/src/inspector/action-execution.test.ts

Additional Recommendations

  1. Documentation: Add JSDoc comments to the executeAction accessor explaining:

    • Purpose: For inspector/debugging use
    • Connection lifecycle: Creates temporary HTTP connection
    • Parameter limitations: No connection params supported
  2. Type safety: Consider validating params more strictly than z.array(z.any())

  3. Logging: Consider adding debug logging in the executeAction accessor similar to what's in packages/rivetkit/src/actor/instance.ts:1336-1340

  4. Consistency: The HTTP driver at packages/rivetkit/src/actor/conn-drivers.ts:180-189 has a TODO comment about connection ready state and request abortion. This might affect inspector action execution.

Verdict

Overall Assessment: Good implementation following existing patterns, but needs error handling before merge.

Required Changes:

  • ✅ Add try-catch error handling to /action endpoint
  • ✅ Add warning comment about CORS in example
  • ✅ Add tests for the new functionality

Recommended Changes:

  • 📝 Add JSDoc documentation
  • 📝 Consider defensive null-checking in cleanup
  • 📝 Evaluate if connection parameters should be supported

Approval Status: ⚠️ Approve with required changes implemented


Great work on maintaining consistency with existing patterns! The implementation is clean and follows the codebase conventions from CLAUDE.md. Once the error handling is added, this will be ready to merge.

@claude
Copy link

claude bot commented Oct 21, 2025

Pull Request Review: Execute Actor Actions Through Inspector

Summary

This PR adds the ability to execute actor actions through the inspector API by creating a temporary HTTP connection and executing the action within that connection context. The implementation is clean and follows existing patterns in the codebase.


Code Quality & Best Practices

Positive Observations:

  • ✅ Follows existing patterns for connection creation (similar to router-endpoints.ts:468-471)
  • ✅ Properly uses ActionContext instead of importing it as a type
  • ✅ Correctly imports ConnDriverKind enum for creating the HTTP driver state
  • ✅ Proper cleanup with finally block to disconnect the connection
  • ✅ Consistent with RivetKit's code style (camelCase, structured logging patterns)

Import Change (packages/rivetkit/src/actor/instance.ts:23):
The change from type ActionContext to ActionContext is correct since the code now needs to instantiate ActionContext at runtime (line 248).


Potential Issues & Concerns

1. Error Handling - Missing Try-Catch in Inspector Route

Location: packages/rivetkit/src/inspector/actor.ts:254-266

The new /action endpoint lacks error handling. If executeAction throws an error (e.g., ActionNotFound, ActionTimedOut, or user errors), it will bubble up unhandled.

Recommendation:

.post(
  "/action",
  sValidator(
    "json",
    z.object({ name: z.string(), params: z.array(z.any()).optional() }),
  ),
  async (c) => {
    const { name, params } = c.req.valid("json");
    try {
      const result = await c.var.inspector.accessors.executeAction(
        name,
        params,
      );
      return c.json({ result }, 200);
    } catch (error) {
      return c.json({ error: (error as Error).message }, 500);
    }
  },
)

2. Connection Lifecycle - Potential Race Condition

Location: packages/rivetkit/src/actor/instance.ts:235-254

The createConn call may trigger lifecycle hooks (onConnect, createConnState) that could have side effects. For inspector-triggered actions, these hooks might not be appropriate.

Concerns:

  • onConnect callbacks will be invoked for synthetic inspector connections
  • If createConnState has side effects or expensive operations, they run unnecessarily
  • Connection state is created but never used

Recommendation: Consider adding a flag to createConn to skip hooks for internal/inspector use:

async createConn(
  socket: ConnSocket,
  params: any,
  token?: string,
  opts?: { skipHooks?: boolean }
): Promise<Conn<S, CP, CS, V, I, DB>>

3. Connection Parameters - Using undefined

Location: packages/rivetkit/src/actor/instance.ts:241-242

Passing undefined for params and token to createConn may cause issues if the actor expects specific connection parameters or if validation is in place.

Recommendation: Pass appropriate default values based on actor config:

const conn = await this.createConn(
  { socketId, driverState: { [ConnDriverKind.HTTP]: {} } },
  this.connParamsEnabled ? {} : undefined,  // Or default conn params
  undefined,
);

4. Type Safety - Missing Parameter Validation

Location: packages/rivetkit/src/inspector/actor.ts:257

The Zod schema allows z.array(z.any()).optional() which doesn't validate parameter types. While this is flexible, it bypasses any validation that might exist on the action itself.

Note: This is acceptable for an inspector/debugging tool but worth documenting.


Performance Considerations

Connection Creation Overhead

Each inspector action call creates a full connection with:

  • Connection state initialization (if enabled)
  • onConnect hook execution (if defined)
  • Event subscription setup
  • Persistence updates

Impact: Low for debugging scenarios, but could add overhead if used programmatically.

Suggestion: Document that this is intended for debugging/inspection, not production action calls.


Security Concerns

1. Authentication & Authorization

Critical: The inspector API has no authentication shown in this diff. If the inspector endpoints are exposed without authentication, this creates a significant security risk.

Questions:

  • Is there authentication middleware on the inspector router?
  • Should there be rate limiting on action execution?
  • Are inspector endpoints only accessible in development mode?

Recommendation: Ensure inspector endpoints are properly secured before production use.

2. Action Parameter Injection

Allowing arbitrary parameters via the inspector could potentially bypass business logic validation if not careful.

Note: This is acceptable for a debugging tool but should be documented clearly.


Test Coverage

Missing: No tests are included in this PR.

Recommended Test Cases:

  1. Execute a simple action and verify the result
  2. Execute an action that throws an error
  3. Execute an action with parameters
  4. Execute a non-existent action (should throw ActionNotFound)
  5. Execute an action on an actor without actions defined
  6. Verify connection is properly cleaned up after execution
  7. Verify onConnect/onDisconnect hooks are (or aren't) called appropriately

Example Test Structure:

test("inspector can execute actions", async () => {
  const actor = await createTestActor({
    actions: {
      add: (ctx, a: number, b: number) => a + b,
    },
  });
  
  const result = await actor.inspector.accessors.executeAction("add", [2, 3]);
  expect(result).toBe(5);
});

Additional Observations

Clean Code Pattern

The implementation mirrors the HTTP action endpoint pattern (router-endpoints.ts:468-483), which is good for consistency.

Documentation

Consider adding JSDoc comments to the new executeAction method in ActorInspectorAccessors interface explaining:

  • When to use this vs. direct action calls
  • That it creates a temporary connection
  • Performance implications

Summary of Recommendations

High Priority:

  1. ✅ Add error handling to the inspector /action endpoint
  2. ✅ Verify inspector API authentication/authorization
  3. ✅ Add test coverage

Medium Priority:
4. Consider adding a flag to skip connection hooks for inspector calls
5. Document that this is for debugging/inspection purposes
6. Add JSDoc documentation

Low Priority:
7. Review default connection parameter handling


Conclusion

This is a well-implemented feature that adds useful debugging capabilities. The main concerns are around error handling, test coverage, and security. With the recommended fixes, this would be a solid addition to RivetKit.

Overall Assessment: Approve with suggested changes

@NathanFlurry NathanFlurry merged commit 4e07ed5 into 10-20-fix_get_actor_in_remote_driver Oct 22, 2025
7 of 9 checks passed
@NathanFlurry NathanFlurry deleted the 10-20-feat_execute_actor_actions_through_inspector branch October 22, 2025 03:29
NathanFlurry pushed a commit that referenced this pull request Oct 22, 2025
* fix: get actor in remote driver

* feat: execute actor actions through inspector (#1416)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants