Plugin system (3/8): RPC transport + plugin-aware auth scopes#3727
Plugin system (3/8): RPC transport + plugin-aware auth scopes#3727ccdwyer wants to merge 4 commits into
Conversation
Threads carry an owner (user or plugin:<id>); plugin-owned threads are hidden from user-facing projections, activity relays, and client thread state. Includes migration 033 and the decider/projector/read-model wiring. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
vouch:unvouched
github-actions
Bot
added
the
size:XXL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| ), | ||
| ); | ||
|
|
||
| function validateRegistration( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHost.ts:100
validateRegistration only checks for duplicate rpc.method entries but never checks registration.streams. When a plugin registers two stream descriptors with the same method, PluginRpcDispatcher.subscribe() uses Array.find, which returns the first match and silently ignores the rest. Activation succeeds, so the duplicate is never rejected and part of the plugin's API is unreachable at runtime. Add a duplicate-method check for registration.streams alongside the existing rpc loop.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 100:
`validateRegistration` only checks for duplicate `rpc.method` entries but never checks `registration.streams`. When a plugin registers two stream descriptors with the same `method`, `PluginRpcDispatcher.subscribe()` uses `Array.find`, which returns the first match and silently ignores the rest. Activation succeeds, so the duplicate is never rejected and part of the plugin's API is unreachable at runtime. Add a duplicate-method check for `registration.streams` alongside the existing `rpc` loop.
| ), | ||
| // Exponential backoff capped at 30s so a flapping service keeps | ||
| // retrying at a bounded cadence instead of backing off forever. | ||
| Effect.repeat( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHost.ts:200
startService restarts plugin services after any successful return, not just after failures. Effect.repeat(...) repeats only on success, and the preceding Effect.catchCause(...) converts failures into success after logging, so both a clean completion and a crash are retried forever. Any plugin service implemented as a one-shot task will be re-run every 250ms/30s indefinitely, causing duplicate side effects instead of a single startup run. Consider using Effect.retry (repeats only on failure) instead of Effect.repeat, or only retrying on failure causes rather than catching and logging them.
Also found in 1 other location(s)
packages/client-runtime/src/rpc/client.ts:275
subscribePluginalways routes plugin streams through the durablesubscribe(...)helper at line275, which automatically re-subscribes wheneverEnvironmentSupervisor.sessionchanges. That is only correct for long-lived subscriptions, but pluginstreamsare defined as arbitraryStream.Stream<unknown, Error>handlers and can be finite command-style streams as well. If a plugin stream is mid-flight when the session reconnects, the same{ pluginId, method, payload }will be invoked again on the new session, duplicating side effects or restarting the operation instead of behaving like a one-shot stream bound to the original session.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 200:
`startService` restarts plugin services after *any* successful return, not just after failures. `Effect.repeat(...)` repeats only on success, and the preceding `Effect.catchCause(...)` converts failures into success after logging, so both a clean completion and a crash are retried forever. Any plugin service implemented as a one-shot task will be re-run every 250ms/30s indefinitely, causing duplicate side effects instead of a single startup run. Consider using `Effect.retry` (repeats only on failure) instead of `Effect.repeat`, or only retrying on failure causes rather than catching and logging them.
Also found in 1 other location(s):
- packages/client-runtime/src/rpc/client.ts:275 -- `subscribePlugin` always routes plugin streams through the durable `subscribe(...)` helper at line `275`, which automatically re-subscribes whenever `EnvironmentSupervisor.session` changes. That is only correct for long-lived subscriptions, but plugin `streams` are defined as arbitrary `Stream.Stream<unknown, Error>` handlers and can be finite command-style streams as well. If a plugin stream is mid-flight when the session reconnects, the same `{ pluginId, method, payload }` will be invoked again on the new session, duplicating side effects or restarting the operation instead of behaving like a one-shot stream bound to the original session.
| : Effect.fail(new PluginLockfileReadError({ path: lockfilePath, cause })), | ||
| ), | ||
| ); | ||
| if (raw === null) return EMPTY_PLUGIN_LOCKFILE; |
There was a problem hiding this comment.
🟡 Medium plugins/PluginLockfileStore.ts:143
readLockfileFromPath returns the shared EMPTY_PLUGIN_LOCKFILE singleton when the lockfile is missing, but PluginLockfile holds mutable sources (array) and plugins (object). Callers like updatePlugin spread lockfile into a new object yet keep references to the same sources array and plugins object, so mutations poison the shared constant for every subsequent "missing lockfile" read in the process. Consider returning a fresh copy of EMPTY_PLUGIN_LOCKFILE so each caller gets its own sources and plugins instances.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginLockfileStore.ts around line 143:
`readLockfileFromPath` returns the shared `EMPTY_PLUGIN_LOCKFILE` singleton when the lockfile is missing, but `PluginLockfile` holds mutable `sources` (array) and `plugins` (object). Callers like `updatePlugin` spread `lockfile` into a new object yet keep references to the same `sources` array and `plugins` object, so mutations poison the shared constant for every subsequent "missing lockfile" read in the process. Consider returning a fresh copy of `EMPTY_PLUGIN_LOCKFILE` so each caller gets its own `sources` and `plugins` instances.
| textGeneration: unavailable("textGeneration"), | ||
| }); | ||
|
|
||
| const upgradeLockfileEntry = ( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHost.ts:148
upgradeLockfileEntry copies entry.activation into the upgraded lockfile entry, carrying over the old version's crashCount. When a plugin is upgraded after one crash, the new version starts with crashCount already at 1, so a single startup failure of the new version hits the crashCount >= 2 threshold and the plugin is disabled — the new build never gets a clean slate. The activation state should be reset when promoting a staged version.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 148:
`upgradeLockfileEntry` copies `entry.activation` into the upgraded lockfile entry, carrying over the old version's `crashCount`. When a plugin is upgraded after one crash, the new version starts with `crashCount` already at 1, so a single startup failure of the new version hits the `crashCount >= 2` threshold and the plugin is disabled — the new build never gets a clean slate. The activation state should be reset when promoting a staged version.
| // bundles are user-readable — and the clearer typo diagnostics win. | ||
| const call: PluginRpcDispatcher["Service"]["call"] = (pluginId, method, payload, session) => | ||
| Effect.gen(function* () { | ||
| const runtime = yield* lookupRuntime(registry, pluginId); |
There was a problem hiding this comment.
🟡 Medium plugins/PluginRpcDispatcher.ts:164
call and subscribe treat any runtime found in PluginRuntimeRegistry as active, but a runtime is stored in the registry before activation completes and is never removed if later startup work fails. As a result, when activation fails after the put, the dispatcher still finds the stale runtime: non-always methods return not-ready forever instead of treating the plugin as failed, and readiness: "always" methods can still be invoked against a plugin whose scope was already closed. Consider removing the runtime from the registry on activation failure, or checking the readiness deferred's failure state so stale runtimes are rejected.
Also found in 1 other location(s)
apps/server/src/serverRuntimeStartup.ts:436
pluginHost.startis invoked only aftercommandGate.signalCommandReadyand after the HTTP server may already be accepting connections. The WS handlers exposeplugins.list/plugins.call/plugins.subscribedirectly fromPluginCatalog/PluginRpcDispatcherwithout waiting for startup readiness, and both readPluginRuntimeRegistry. UntilpluginHost.startfinishes populating that registry, early clients can get an empty plugin list ornot-foundfor installed plugins even though the server is already reachable.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginRpcDispatcher.ts around line 164:
`call` and `subscribe` treat any runtime found in `PluginRuntimeRegistry` as active, but a runtime is stored in the registry before activation completes and is never removed if later startup work fails. As a result, when activation fails after the `put`, the dispatcher still finds the stale runtime: non-`always` methods return `not-ready` forever instead of treating the plugin as failed, and `readiness: "always"` methods can still be invoked against a plugin whose scope was already closed. Consider removing the runtime from the registry on activation failure, or checking the `readiness` deferred's failure state so stale runtimes are rejected.
Also found in 1 other location(s):
- apps/server/src/serverRuntimeStartup.ts:436 -- `pluginHost.start` is invoked only after `commandGate.signalCommandReady` and after the HTTP server may already be accepting connections. The WS handlers expose `plugins.list` / `plugins.call` / `plugins.subscribe` directly from `PluginCatalog` / `PluginRpcDispatcher` without waiting for startup readiness, and both read `PluginRuntimeRegistry`. Until `pluginHost.start` finishes populating that registry, early clients can get an empty plugin list or `not-found` for installed plugins even though the server is already reachable.
| } | ||
|
|
||
| const unavailable = (capability: string) => | ||
| Effect.die(new PluginCapabilityUnavailable({ capability })); |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:122
unavailable() uses Effect.die(...) to create unavailable capabilities, so calling any unavailable capability (e.g. hostApi.http) raises an unrecoverable defect instead of a typed PluginCapabilityUnavailable failure. This bypasses normal error-channel recovery — Effect.catchTag("PluginCapabilityUnavailable", ...) cannot intercept it, and plugin code aborts rather than receiving a catchable error. Use Effect.fail(...) instead so the error is recoverable as declared by the PluginHostApi type.
| Effect.die(new PluginCapabilityUnavailable({ capability })); | |
| const unavailable = (capability: string) => | |
| Effect.fail(new PluginCapabilityUnavailable({ capability })); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 122:
`unavailable()` uses `Effect.die(...)` to create unavailable capabilities, so calling any unavailable capability (e.g. `hostApi.http`) raises an unrecoverable defect instead of a typed `PluginCapabilityUnavailable` failure. This bypasses normal error-channel recovery — `Effect.catchTag("PluginCapabilityUnavailable", ...)` cannot intercept it, and plugin code aborts rather than receiving a catchable error. Use `Effect.fail(...)` instead so the error is recoverable as declared by the `PluginHostApi` type.
There was a problem hiding this comment.
🟡 Medium
t3code/packages/contracts/src/auth.ts
Line 357 in 776bfdd
AuthScope now includes PluginScope, but EnvironmentAuthorizationError still declares requiredScope as AuthEnvironmentScope. When the server denies a plugin RPC over WebSocket, the real missing scope is something like plugin:<id>:operate, which does not satisfy AuthEnvironmentScope, so the error cannot serialize the actual required scope. The server must either fail to encode the error or misrepresent the required scope as an environment scope, producing a wrong or undecodable error response. Consider widening requiredScope to AuthScope so WebSocket errors can faithfully report the missing plugin scope.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/contracts/src/auth.ts around line 357:
`AuthScope` now includes `PluginScope`, but `EnvironmentAuthorizationError` still declares `requiredScope` as `AuthEnvironmentScope`. When the server denies a plugin RPC over WebSocket, the real missing scope is something like `plugin:<id>:operate`, which does not satisfy `AuthEnvironmentScope`, so the error cannot serialize the actual required scope. The server must either fail to encode the error or misrepresent the required scope as an environment scope, producing a wrong or undecodable error response. Consider widening `requiredScope` to `AuthScope` so WebSocket errors can faithfully report the missing plugin scope.
| [WS_METHODS.subscribeServerConfig, AuthOrchestrationReadScope], | ||
| [WS_METHODS.subscribeServerLifecycle, AuthOrchestrationReadScope], | ||
| [WS_METHODS.subscribeAuthAccess, AuthAccessReadScope], | ||
| [PLUGINS_WS_METHODS.list, AuthOrchestrationReadScope], |
There was a problem hiding this comment.
🟠 High src/ws.ts:350
RPC_REQUIRED_SCOPE gates PLUGINS_WS_METHODS.call and PLUGINS_WS_METHODS.subscribe with AuthOrchestrationReadScope, but satisfiesScope only recognizes plugin:<id>:read|operate as satisfying a required AuthOrchestrationReadScope when the required scope itself is a plugin scope. A token issued with only plugin:<id>:read or plugin:<id>:operate therefore fails the outer websocket authorization before PluginRpcDispatcher runs its per-plugin check, so plugin-scoped clients cannot call or subscribe to any plugin method. Consider mapping these entries to a scope that plugin-scoped tokens satisfy, or handle plugin methods before the static RPC_REQUIRED_SCOPE check.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/ws.ts around line 350:
`RPC_REQUIRED_SCOPE` gates `PLUGINS_WS_METHODS.call` and `PLUGINS_WS_METHODS.subscribe` with `AuthOrchestrationReadScope`, but `satisfiesScope` only recognizes `plugin:<id>:read|operate` as satisfying a required `AuthOrchestrationReadScope` when the *required* scope itself is a plugin scope. A token issued with only `plugin:<id>:read` or `plugin:<id>:operate` therefore fails the outer websocket authorization before `PluginRpcDispatcher` runs its per-plugin check, so plugin-scoped clients cannot call or subscribe to any plugin method. Consider mapping these entries to a scope that plugin-scoped tokens satisfy, or handle plugin methods before the static `RPC_REQUIRED_SCOPE` check.
| hostResolutionHookRegistered = true; | ||
| }), | ||
| ), | ||
| Effect.catchCause((cause) => |
There was a problem hiding this comment.
🟡 Medium plugins/PluginModuleLoader.ts:158
ensureHostSingletonResolution catches every registerHook failure (lines 158–162), logs it as a warning, and still returns Effect.void. This means PluginHost.start proceeds to loadServerEntry without the resolver hook installed, so a single transient nodeModule.register(...) failure causes plugins to import against the wrong module graph (or fail to import), and PluginHost then marks the plugin state: "failed" in the lockfile — the exact persistent-failure path the rest of this change was designed to prevent. Consider letting registration failures propagate (or surface a typed error) so callers do not continue without the hook.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginModuleLoader.ts around line 158:
`ensureHostSingletonResolution` catches every `registerHook` failure (lines 158–162), logs it as a warning, and still returns `Effect.void`. This means `PluginHost.start` proceeds to `loadServerEntry` without the resolver hook installed, so a single transient `nodeModule.register(...)` failure causes plugins to import against the wrong module graph (or fail to import), and `PluginHost` then marks the plugin `state: "failed"` in the lockfile — the exact persistent-failure path the rest of this change was designed to prevent. Consider letting registration failures propagate (or surface a typed error) so callers do not continue without the hook.
| return before.filter((entry) => !afterKeys.has(objectKey(entry))); | ||
| }; | ||
|
|
||
| const validateMigrationObjects = (input: { |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:112
validateMigrationObjects only diffs sqlite_master schema objects, so a plugin migration that runs direct DML against core tables (e.g. DELETE FROM core_users or UPDATE sessions ...) passes validation and commits — no schema object changes, so the namespace guard never fires. This lets a plugin migration silently modify or destroy non-plugin data. Consider blocking DML against non-plugin tables (e.g. by inspecting the migration's SQL or enforcing read-only access to core tables) so data-only changes are covered too.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 112:
`validateMigrationObjects` only diffs `sqlite_master` schema objects, so a plugin migration that runs direct DML against core tables (e.g. `DELETE FROM core_users` or `UPDATE sessions ...`) passes validation and commits — no schema object changes, so the namespace guard never fires. This lets a plugin migration silently modify or destroy non-plugin data. Consider blocking DML against non-plugin tables (e.g. by inspecting the migration's SQL or enforcing read-only access to core tables) so data-only changes are covered too.
Clients could send thread.create with owner=plugin:* via the client wire union and forge a hidden thread. Add an owner-less ClientThreadCreateCommand and use it in the client-facing unions; owner stays server-injected. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
PluginHost lifecycle (discover, activate, deactivate), lockfile store, per-plugin migrations (034), module loader + resolve hooks, plugin manifest contract, and the @t3tools/plugin-sdk server package. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
Plugin RPC dispatcher + catalog wiring over ws, plugin-aware auth scope model (plugin:<id>:read|operate), session/pairing-grant scope plumbing, and the client-runtime authorization surface for plugin scopes. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
ccdwyer
force-pushed
the
wb/03-transport-scopes
branch
from
776bfdd to
d96dadd
Compare
There was a problem hiding this comment.
One error-handling convention finding in the new plugin services. See inline comment.
Posted via Macroscope — Effect Service Conventions
| Effect.catchCause((cause) => | ||
| Effect.fail( | ||
| new PluginRegistrationError({ | ||
| pluginId, | ||
| detail: Cause.pretty(cause), |
There was a problem hiding this comment.
PluginRegistrationError has no cause field, so this wrapping site flattens a genuine thrown failure with Cause.pretty(cause) into the detail: Schema.String attribute, and message is then derived from that stringified defect. This erases the structured upstream error and its stack chain, which the conventions ask you to preserve as a cause (never derive the wrapper message from a stringified defect).
Add a cause: Schema.Defect() field and preserve the underlying failure here, mirroring the sibling PluginMigrationExecutionError (whose message derives only from pluginId/version and keeps cause). The detail-based validation constructions (invalid RPC scope, duplicate RPC method, manifest-id mismatch) are pure domain errors and can keep using detail.
| Effect.catchCause((cause) => | |
| Effect.fail( | |
| new PluginRegistrationError({ | |
| pluginId, | |
| detail: Cause.pretty(cause), | |
| Effect.catchCause((cause) => | |
| Effect.fail( | |
| new PluginRegistrationError({ | |
| pluginId, | |
| detail: "plugin register() threw", | |
| cause, | |
| }), | |
| ), | |
| ), |
(This requires adding cause: Schema.Defect() to the PluginRegistrationError fields and building message from the structural attributes rather than detail alone.)
Posted via Macroscope — Effect Service Conventions
| ), | ||
| ); | ||
|
|
||
| const scope = yield* Scope.make("sequential"); |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:268
loadPlugin creates a Scope at line 268 with Scope.make("sequential") and forks all plugin services into it, but that scope is never attached to the surrounding server lifetime via Effect.addFinalizer. When the server runtime shuts down, scope is never closed, so plugin service fibers are not interrupted and can keep file watchers, sockets, and timers alive — hanging graceful shutdown or leaking background work. Consider registering a finalizer that closes scope when the enclosing server scope exits.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 268:
`loadPlugin` creates a `Scope` at line 268 with `Scope.make("sequential")` and forks all plugin services into it, but that scope is never attached to the surrounding server lifetime via `Effect.addFinalizer`. When the server runtime shuts down, `scope` is never closed, so plugin service fibers are not interrupted and can keep file watchers, sockets, and timers alive — hanging graceful shutdown or leaking background work. Consider registering a finalizer that closes `scope` when the enclosing server scope exits.
| id: manifest.id, | ||
| name: manifest.name, | ||
| version: manifest.version, |
There was a problem hiding this comment.
🟡 Medium plugins/PluginCatalog.ts:78
pluginInfoFromLockfileEntry uses manifest.id and manifest.version from the on-disk manifest.json instead of the lockfile's pluginId key and entry.version. If the manifest is corrupt or belongs to a different plugin, list returns a PluginInfo with the wrong identity — it can duplicate an active plugin's id (which is deduplicated by the activePluginIds set, causing the active runtime to be hidden) while the real lockfile entry silently disappears from the results. Use the lockfile pluginId and entry.version for id and version, as fallbackPluginInfo already does.
| id: manifest.id, | |
| name: manifest.name, | |
| version: manifest.version, | |
| id: PluginId.make(pluginId), | |
| name: manifest.name, | |
| version: entry.version, |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginCatalog.ts around lines 78-80:
`pluginInfoFromLockfileEntry` uses `manifest.id` and `manifest.version` from the on-disk `manifest.json` instead of the lockfile's `pluginId` key and `entry.version`. If the manifest is corrupt or belongs to a different plugin, `list` returns a `PluginInfo` with the wrong identity — it can duplicate an active plugin's id (which is deduplicated by the `activePluginIds` set, causing the active runtime to be hidden) while the real lockfile entry silently disappears from the results. Use the lockfile `pluginId` and `entry.version` for `id` and `version`, as `fallbackPluginInfo` already does.
| return subscribe(PLUGINS_WS_METHODS.subscribe, { | ||
| pluginId, | ||
| method, | ||
| ...(payload === undefined ? {} : { payload }), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Medium rpc/client.ts:275
subscribePlugin routes all plugin streams through the durable subscribe helper, which re-invokes the RPC method whenever the supervisor session is replaced. Plugin streams are not classified as subscriptions versus finite commands, so a one-shot or side-effecting plugin stream restarts after a session swap and can run twice, emitting duplicated or inconsistent results. Consider routing plugin streams through runStream (which binds to a single session) instead, or add an option to control reconnection behavior.
| return subscribe(PLUGINS_WS_METHODS.subscribe, { | |
| pluginId, | |
| method, | |
| ...(payload === undefined ? {} : { payload }), | |
| }); | |
| } | |
| return runStream(PLUGINS_WS_METHODS.subscribe, { | |
| pluginId, | |
| method, | |
| ...(payload === undefined ? {} : { payload }), | |
| }); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/rpc/client.ts around lines 275-280:
`subscribePlugin` routes all plugin streams through the durable `subscribe` helper, which re-invokes the RPC method whenever the supervisor session is replaced. Plugin streams are not classified as subscriptions versus finite commands, so a one-shot or side-effecting plugin stream restarts after a session swap and can run twice, emitting duplicated or inconsistent results. Consider routing plugin streams through `runStream` (which binds to a single session) instead, or add an option to control reconnection behavior.
| input.service.run({ pluginId: input.pluginId, logger: input.logger }).pipe( | ||
| Effect.catchCause((cause) => |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHost.ts:191
startService calls input.service.run(...) eagerly. If a plugin service's run implementation throws synchronously before returning its Effect, the exception bypasses the Effect.catchCause(...).repeat(...) restart wrapper and aborts plugin activation instead of being logged and retried. Consider wrapping the run call in Effect.suspend so a synchronous throw becomes a failed effect that the catchCause/repeat handlers can catch.
- input.service.run({ pluginId: input.pluginId, logger: input.logger }).pipe(
+ Effect.suspend(() => input.service.run({ pluginId: input.pluginId, logger: input.logger })).pipe(
Effect.catchCause((cause) =>🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around lines 191-192:
`startService` calls `input.service.run(...)` eagerly. If a plugin service's `run` implementation throws synchronously before returning its `Effect`, the exception bypasses the `Effect.catchCause(...).repeat(...)` restart wrapper and aborts plugin activation instead of being logged and retried. Consider wrapping the `run` call in `Effect.suspend` so a synchronous throw becomes a failed effect that the `catchCause`/`repeat` handlers can catch.
| detail: `object name must start with ${input.prefix}`, | ||
| }); | ||
| } | ||
| if (entry.type !== "trigger" && entry.type !== "view") continue; |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:147
validateMigrationObjects skips the core-table reference check for table and index objects because the loop continues whenever entry.type is not trigger or view. A migration that creates a prefixed table with a foreign key to a core table — e.g. CREATE TABLE p_test_items (... REFERENCES users(id)) — passes the prefix check and is never inspected, so the cross-namespace dependency is not detected. Apply the same body reference scan to all object types (or at least to table and index) rather than only trigger and view.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 147:
`validateMigrationObjects` skips the core-table reference check for `table` and `index` objects because the loop `continue`s whenever `entry.type` is not `trigger` or `view`. A migration that creates a prefixed table with a foreign key to a core table — e.g. `CREATE TABLE p_test_items (... REFERENCES users(id))` — passes the prefix check and is never inspected, so the cross-namespace dependency is not detected. Apply the same body reference scan to all object types (or at least to `table` and `index`) rather than only `trigger` and `view`.
| threadId: ThreadId, | ||
| projectId: ProjectId, | ||
| title: TrimmedNonEmptyString, | ||
| owner: Schema.optional(ThreadOwner), |
There was a problem hiding this comment.
🟠 High src/orchestration.ts:514
ThreadCreateCommand now accepts an owner field, enabling plugin-owned (hidden) threads. However, subscribeThread is gated only by orchestration:read and calls getThreadDetailById without any owner filter — the projection layer only filters by owner = 'user' in shell/list views, not in thread-detail lookups. Any client that guesses or learns a plugin thread id can call orchestration.subscribeThread and read the full hidden thread detail and messages, defeating the privacy boundary this change introduces. Consider applying the same owner-based visibility filter in the subscribeThread handler (or getThreadDetailById) that the shell/list views use.
Also found in 1 other location(s)
apps/server/src/relay/AgentAwarenessRelay.ts:329
The new owner check in
publishThreadUnsafedrops every thread whose owner is not"user"before computing or publishing relay state. That means plugin-created / agent-owned threads can never reachrelayClient.server.publishAgentActivity, so their activity/notifications stop propagating to the relay even whenprojectThreadAwareness(...)would otherwise produce a publishable state. This breaks agent-awareness for the new plugin-owned thread flow introduced by the PR.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/contracts/src/orchestration.ts around line 514:
`ThreadCreateCommand` now accepts an `owner` field, enabling plugin-owned (hidden) threads. However, `subscribeThread` is gated only by `orchestration:read` and calls `getThreadDetailById` without any owner filter — the projection layer only filters by `owner = 'user'` in shell/list views, not in thread-detail lookups. Any client that guesses or learns a plugin thread id can call `orchestration.subscribeThread` and read the full hidden thread detail and messages, defeating the privacy boundary this change introduces. Consider applying the same owner-based visibility filter in the `subscribeThread` handler (or `getThreadDetailById`) that the shell/list views use.
Also found in 1 other location(s):
- apps/server/src/relay/AgentAwarenessRelay.ts:329 -- The new owner check in `publishThreadUnsafe` drops every thread whose owner is not `"user"` before computing or publishing relay state. That means plugin-created / agent-owned threads can never reach `relayClient.server.publishAgentActivity`, so their activity/notifications stop propagating to the relay even when `projectThreadAwareness(...)` would otherwise produce a publishable state. This breaks agent-awareness for the new plugin-owned thread flow introduced by the PR.
| const file = yield* fs.open(tempPath, { flag: "w", mode: 0o600 }); | ||
| yield* file.writeAll(bytes); | ||
| yield* file.sync; | ||
| yield* fs.rename(tempPath, input.lockfilePath); |
There was a problem hiding this comment.
🟠 High plugins/PluginLockfileStore.ts:176
The ownership re-check at owner !== lock.token runs once before writeLockfileToPath is called, but nothing re-verifies ownership after the write completes. If the writer is paused (GC/CPU starvation) or the temp-file write, sync, and rename take longer than STALE_LOCK_MS, another process can reclaim the advisory lock, perform its own mutation, and release the lock while the original writer is still inside writeLockfileToPath. When the original writer resumes and completes fs.rename(tempPath, input.lockfilePath), it overwrites the newer lockfile, silently losing the other writer's update. Consider re-checking the owner token immediately before the rename (or using a write path that fails if ownership was lost) so a reclaimed lock aborts the write instead of clobbering the newer contents.
yield* file.writeAll(bytes);
yield* file.sync;
- yield* fs.rename(tempPath, input.lockfilePath);
+ // Re-verify lock ownership immediately before the atomic rename so a
+ // writer paused past STALE_LOCK_MS does not clobber a newer lockfile
+ // produced by a reclaimer.
+ const owner = yield* fs.readFileString(input.lockfilePath).pipe(
+ Effect.map((content) => content.trim()),
+ Effect.orElseSucceed(() => ""),
+ );
+ if (owner !== input.ownerToken) {
+ return yield* new PluginLockfileWriteError({
+ path: input.lockfilePath,
+ cause: new Error("advisory lock ownership lost before rename"),
+ });
+ }
+ yield* fs.rename(tempPath, input.lockfilePath);🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginLockfileStore.ts around line 176:
The ownership re-check at `owner !== lock.token` runs once before `writeLockfileToPath` is called, but nothing re-verifies ownership after the write completes. If the writer is paused (GC/CPU starvation) or the temp-file write, `sync`, and `rename` take longer than `STALE_LOCK_MS`, another process can reclaim the advisory lock, perform its own mutation, and release the lock while the original writer is still inside `writeLockfileToPath`. When the original writer resumes and completes `fs.rename(tempPath, input.lockfilePath)`, it overwrites the newer lockfile, silently losing the other writer's update. Consider re-checking the owner token immediately before the `rename` (or using a write path that fails if ownership was lost) so a reclaimed lock aborts the write instead of clobbering the newer contents.
| const RelativeEntryPath = TrimmedNonEmptyString.check( | ||
| Schema.makeFilter<string>((entryPath) => { | ||
| if (entryPath.startsWith("/") || entryPath.startsWith("\\")) { | ||
| return "entry paths must be relative"; | ||
| } | ||
| if (entryPath.split(/[\\/]/).includes("..")) { | ||
| return "entry paths may not contain '..' segments"; | ||
| } | ||
| return true; | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🟡 Medium src/plugin.ts:36
RelativeEntryPath only rejects paths starting with / or \ and paths containing .. segments. A Windows drive-letter absolute path such as C:\plugin\server.js passes both checks, so PluginManifest.entries.server / web can be an absolute path even though the schema is supposed to require a relative entry. Any loader that resolves these entries on Windows can load code from outside the plugin package. Consider adding a check for Windows drive-letter prefixes (e.g., ^[a-zA-Z]:) so absolute Windows paths are also rejected.
| const RelativeEntryPath = TrimmedNonEmptyString.check( | |
| Schema.makeFilter<string>((entryPath) => { | |
| if (entryPath.startsWith("/") || entryPath.startsWith("\\")) { | |
| return "entry paths must be relative"; | |
| } | |
| if (entryPath.split(/[\\/]/).includes("..")) { | |
| return "entry paths may not contain '..' segments"; | |
| } | |
| return true; | |
| }), | |
| ); | |
| const RelativeEntryPath = TrimmedNonEmptyString.check( | |
| Schema.makeFilter<string>((entryPath) => { | |
| if (entryPath.startsWith("/") || entryPath.startsWith("\\")) { | |
| return "entry paths must be relative"; | |
| } | |
| if (/^[a-zA-Z]:/.test(entryPath)) { | |
| return "entry paths must be relative"; | |
| } | |
| if (entryPath.split(/[\\/]/).includes("..")) { | |
| return "entry paths may not contain '..' segments"; | |
| } | |
| return true; | |
| }), | |
| ); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/contracts/src/plugin.ts around lines 36-46:
`RelativeEntryPath` only rejects paths starting with `/` or `\` and paths containing `..` segments. A Windows drive-letter absolute path such as `C:\plugin\server.js` passes both checks, so `PluginManifest.entries.server` / `web` can be an absolute path even though the schema is supposed to require a relative entry. Any loader that resolves these entries on Windows can load code from outside the plugin package. Consider adding a check for Windows drive-letter prefixes (e.g., `^[a-zA-Z]:`) so absolute Windows paths are also rejected.
| @@ -430,6 +432,8 @@ export const make = Effect.gen(function* () { | |||
|
|
|||
| yield* Effect.logDebug("Accepting commands"); | |||
| yield* commandGate.signalCommandReady; | |||
There was a problem hiding this comment.
🟡 Medium src/serverRuntimeStartup.ts:434
pluginHost.start runs after commandGate.signalCommandReady, so the command gate opens before plugins are initialized. Clients that connect in that window can invoke plugin RPCs while PluginHost is still loading manifests, and PluginRpcDispatcher.lookupRuntime returns not-found, making installed plugins intermittently appear unavailable during startup. Consider moving pluginHost.start before commandGate.signalCommandReady so plugins are initialized before the server accepts commands.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/serverRuntimeStartup.ts around line 434:
`pluginHost.start` runs after `commandGate.signalCommandReady`, so the command gate opens before plugins are initialized. Clients that connect in that window can invoke plugin RPCs while `PluginHost` is still loading manifests, and `PluginRpcDispatcher.lookupRuntime` returns `not-found`, making installed plugins intermittently appear unavailable during startup. Consider moving `pluginHost.start` before `commandGate.signalCommandReady` so plugins are initialized before the server accepts commands.
1 participant
What Changed
The plugin RPC transport: a WS envelope (
plugins.call/subscribe/list), aPluginRpcDispatcherthat validates each call's method against the plugin's registered descriptors, and the plugin-aware auth scope model — aplugins:managescope, per-pluginplugin:<id>:read|operatescopes, and asatisfiesScopemarker so a full standard client implicitly holds plugin scopes.Why
Part 3 of 8 of a runtime plugin system (see #3725 for the overview). Builds on #3726. This is how a plugin's server code becomes callable, and how those calls are authorized — the client-facing security boundary of the system.
Review — four-model tribunal (GPT-5.5 · Grok · GLM-5.2 · Fable)
Fable independently verified the security core holds (no scope bypass; the grant/consumption path reads scopes unfiltered).
registration.streamswas unvalidated and an unrecognized scope value downgraded to the weakerread→ streams validated like RPC +authorizeDescriptorfails closed.plugins.call/subscribebaseline requiredorchestration:read→ aplugin:*scope now satisfies it (with a no-broadening test).AuthClientSession.scopesso the Clients panel shows the real grant.pluginRpcErroryields a success" claim — it's aSchema.TaggedError(yieldable-as-failure); the existingResult.failuretests prove it.UI Changes
N/A — server + contracts only.
Checklist
https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk