Skip to content

Comments

chore(inspector): add back database tab#4255

Merged
NathanFlurry merged 6 commits intomainfrom
02-21-chore_inspector_add_back_database_tab
Feb 24, 2026
Merged

chore(inspector): add back database tab#4255
NathanFlurry merged 6 commits intomainfrom
02-21-chore_inspector_add_back_database_tab

Conversation

@NathanFlurry
Copy link
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@railway-app
Copy link

railway-app bot commented Feb 22, 2026

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

Service Status Web Updated (UTC)
website ❌ Build Failed (View Logs) Web Feb 22, 2026 at 4:52 am
mcp-hub ✅ Success (View Logs) Web Feb 22, 2026 at 4:50 am
frontend-cloud ❌ Build Failed (View Logs) Web Feb 22, 2026 at 4:50 am
frontend-inspector ❌ Build Failed (View Logs) Web Feb 22, 2026 at 4:50 am
ladle ❌ Build Failed (View Logs) Web Feb 22, 2026 at 4:50 am

@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4255 February 22, 2026 04:49 Destroyed
Copy link
Member Author

NathanFlurry commented Feb 22, 2026


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.

This was referenced Feb 22, 2026
@claude
Copy link

claude bot commented Feb 22, 2026

PR Review: chore(inspector): add back database tab

This PR does two distinct things: (1) restores the Database tab to the actor inspector UI, and (2) refactors access control from a single canInvoke hook to per-event/queue canSubscribe/canPublish hooks. The refactor is the substantial part.


Bugs

Dead code in getValidationSchema (schema.ts)

The two raw if blocks added after the isEventSchemaDefinition and isQueueSchemaDefinition guards are unreachable:

if (isEventSchemaDefinition(schema)) { return schema.schema; }  // catches { schema: defined }
if (isQueueSchemaDefinition(schema)) { return schema.message; } // catches { message: defined, non-event }

// These two never execute — the conditions above already cover them
if (typeof schema === 'object' && 'schema' in schema && ...) { ... }
if (typeof schema === 'object' && 'message' in schema && ...) { ... }

These should be removed.

Indentation regression in instance/mod.ts

The return await this.runInTraceSpan(...) block inside handleRawRequest gained an extra indent level in this diff, making it misaligned relative to the surrounding method bodies. This appears to be a reformat artifact.

Indentation regression in registry.ts

The newly added accessControlNoQueuesActor lines use inconsistent triple-tab indentation that doesn't match the enclosing actors object.


Design / API concerns

Unused generic parameter on QueueSchemaConfig

export type QueueSchemaConfig<TContext = any> = Record<string, QueueSchema>;

TContext is declared but not threaded into QueueSchema, so it has no effect. Either propagate it (to match the treatment of EventSchemaConfig<TContext>) or remove it.

canInvoke removal is a breaking change with no migration path

canInvoke was a public API that enabled per-action access control at the connection level. The new model moves authorization to per-event canSubscribe and per-queue canPublish, but there is no equivalent replacement for per-action authorization. Callers who relied on canInvoke to restrict specific action calls per-connection have no fine-grained alternative. This deserves a migration note (e.g., in the PR description or docs).

Raw WebSocket authorization silently dropped

assertCanInvokeWebSocket was removed from router-websocket-endpoints.ts with no replacement. Raw WebSocket connections now rely solely on onBeforeConnect. This is a behavioral change that should be explicitly documented.

onBeforeConnect logic in access-control fixture inverts semantics

The old canInvoke check was deny-by-default (allow only if === true); the new onBeforeConnect throws only if the flag is explicitly === false (allow-by-default). The tests at lines 158–213 that verify HTTP request / WebSocket denial should be confirmed to still exercise the right path under the new semantics.


Minor

Undefined event warning could use a comment

In conn/mod.ts and contexts/base/actor.ts, sending/broadcasting an event with no matching schema entry now emits a warning rather than throwing. The new test confirms this is intentional, but a one-line comment at the warn site would prevent future readers from treating it as an oversight.


What looks good

  • Granular canSubscribe/canPublish per event/queue is a cleaner design than the monolithic canInvoke discriminated union.
  • Silently completing queue sends to undefined queues (when a queue config is present) is good defensive behavior.
  • The h3-before-h2 guard in rehype.ts is a useful static check.
  • Test coverage for the new behaviors is solid (publish context, undefined queues, undefined events, subscription access control).

@NathanFlurry NathanFlurry force-pushed the 02-21-chore_inspector_add_back_database_tab branch from 578d019 to ebbe0a2 Compare February 22, 2026 21:06
@NathanFlurry NathanFlurry marked this pull request as ready for review February 23, 2026 06:40
@NathanFlurry NathanFlurry force-pushed the 02-20-chore_impl_caninvoke_for_matchmaker branch from 2299455 to 5cd4249 Compare February 23, 2026 08:25
@NathanFlurry NathanFlurry force-pushed the 02-21-chore_inspector_add_back_database_tab branch from ebbe0a2 to 240add0 Compare February 23, 2026 08:25
@NathanFlurry NathanFlurry force-pushed the 02-20-chore_impl_caninvoke_for_matchmaker branch from 5cd4249 to f5dbebd Compare February 23, 2026 18:22
@NathanFlurry NathanFlurry force-pushed the 02-21-chore_inspector_add_back_database_tab branch from 240add0 to 88fcb5f Compare February 23, 2026 18:22
@NathanFlurry NathanFlurry force-pushed the 02-20-chore_impl_caninvoke_for_matchmaker branch from f5dbebd to 5cd4249 Compare February 23, 2026 18:34
@NathanFlurry NathanFlurry force-pushed the 02-21-chore_inspector_add_back_database_tab branch from 88fcb5f to 240add0 Compare February 23, 2026 18:34
@NathanFlurry NathanFlurry force-pushed the 02-20-chore_impl_caninvoke_for_matchmaker branch from 5cd4249 to d1bce69 Compare February 23, 2026 18:35
@NathanFlurry NathanFlurry force-pushed the 02-21-chore_inspector_add_back_database_tab branch from 240add0 to 070db06 Compare February 23, 2026 18:35
@NathanFlurry NathanFlurry force-pushed the 02-21-chore_inspector_add_back_database_tab branch from 070db06 to 3c2f302 Compare February 24, 2026 02:40
@NathanFlurry NathanFlurry force-pushed the 02-20-chore_impl_caninvoke_for_matchmaker branch from d1bce69 to 2654312 Compare February 24, 2026 02:40
@NathanFlurry NathanFlurry mentioned this pull request Feb 24, 2026
11 tasks
Comment on lines +52 to 55
onBeforeConnect: (_c, params: AccessControlConnParams) => {
if (params?.allowRequest === false || params?.allowWebSocket === false) {
throw new Forbidden();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical Logic Bug: Inverted Access Control Logic

The new onBeforeConnect hook has inverted the access control logic compared to the old canInvoke implementation:

Old behavior (lines 49-63 in original):

  • Request: Deny by default, allow only if params?.allowRequest === true
  • WebSocket: Deny by default, allow only if params?.allowWebSocket === true

New behavior:

  • Denies only if params?.allowRequest === false or params?.allowWebSocket === false
  • Allows by default when params is undefined or when these fields are undefined

This means connections that were previously denied (when params are not explicitly set to true) will now be allowed, creating a security vulnerability.

Fix:

onBeforeConnect: (_c, params: AccessControlConnParams) => {
    if (params?.allowRequest !== true || params?.allowWebSocket !== true) {
        throw new Forbidden();
    }
}

Or to match the old deny-by-default behavior more precisely:

onBeforeConnect: (_c, params: AccessControlConnParams) => {
    if (!params?.allowRequest && !params?.allowWebSocket) {
        throw new Forbidden();
    }
}
Suggested change
onBeforeConnect: (_c, params: AccessControlConnParams) => {
if (params?.allowRequest === false || params?.allowWebSocket === false) {
throw new Forbidden();
}
onBeforeConnect: (_c, params: AccessControlConnParams) => {
if (params?.allowRequest !== true || params?.allowWebSocket !== true) {
throw new Forbidden();
}

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@NathanFlurry NathanFlurry force-pushed the 02-20-chore_impl_caninvoke_for_matchmaker branch from 2654312 to b9b4e42 Compare February 24, 2026 02:57
@NathanFlurry NathanFlurry force-pushed the 02-21-chore_inspector_add_back_database_tab branch from 3c2f302 to 62a8778 Compare February 24, 2026 02:57
@NathanFlurry NathanFlurry force-pushed the 02-20-chore_impl_caninvoke_for_matchmaker branch from d1bce69 to a809615 Compare February 24, 2026 04:01
@NathanFlurry NathanFlurry force-pushed the 02-21-chore_inspector_add_back_database_tab branch from 070db06 to ada2163 Compare February 24, 2026 04:01
@NathanFlurry NathanFlurry force-pushed the 02-20-chore_impl_caninvoke_for_matchmaker branch from a809615 to b96f19e Compare February 24, 2026 04:06
Base automatically changed from 02-20-chore_impl_caninvoke_for_matchmaker to main February 24, 2026 04:06
@NathanFlurry NathanFlurry merged commit 0ccfa42 into main Feb 24, 2026
11 of 30 checks passed
@NathanFlurry NathanFlurry deleted the 02-21-chore_inspector_add_back_database_tab branch February 24, 2026 04:07
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.

1 participant