Plugin system (6/8): web host + extension points#3730
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}
There was a problem hiding this comment.
One Effect service-convention issue on the new plugin error modeling. Other new service modules (canonical imports → errors → Context.Service → make → layer order, namespace effect imports, inline service interfaces, Schema.TaggedErrorClass with real cause: Schema.Defect(), direct Schema.is predicates) look consistent with the conventions.
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.
When wrapping a real failure here, the immediate cause is stringified into detail (Cause.pretty(cause)) and the structured cause is dropped. Per the error conventions, a wrapper must preserve the underlying error as cause and derive its message from stable structural attributes — not stuff arbitrary (third-party plugin) defect text into a caller/log-visible detail field.
Give PluginRegistrationError an optional cause and keep it out of the message. The existing validation call sites (invalid RPC scope …, duplicate RPC method …) are pure domain errors with no cause and stay as-is:
export class PluginRegistrationError extends Schema.TaggedErrorClass<PluginRegistrationError>()(
"PluginRegistrationError",
{ pluginId: Schema.String, detail: Schema.String, cause: Schema.optional(Schema.Defect()) },
) {
override get message(): string {
return `Plugin ${this.pluginId} returned an invalid registration: ${this.detail}`;
}
}Then preserve the cause at this boundary instead of stringifying it:
Effect.catchCause((cause) =>
Effect.fail(
new PluginRegistrationError({
pluginId,
detail: "register() failed",
cause: Cause.squash(cause),
}),
),
),Posted via Macroscope — Effect Service Conventions
| turnAliases.set(String(turnId), { threadId: request.threadId, messageId }); | ||
| // Do NOT forward bootstrap.createThread into turn-start: the thread now | ||
| // exists, and the decider would ignore it anyway. | ||
| const turnBootstrap = createdThread ? undefined : bootstrap; |
There was a problem hiding this comment.
🟠 High capabilities/AgentsCapability.ts:337
When startTurn creates the thread first, it sets turnBootstrap = undefined, which drops the entire bootstrap object before dispatching thread.turn.start. Non-createThread bootstrap fields such as prepareWorktree and runSetupScript are silently discarded, so plugin callers requesting a bootstrapped worktree/setup on the first turn get a normal turn start and the preparation never runs. Consider stripping only createThread from bootstrap instead of discarding the whole object.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 337:
When `startTurn` creates the thread first, it sets `turnBootstrap = undefined`, which drops the entire `bootstrap` object before dispatching `thread.turn.start`. Non-`createThread` bootstrap fields such as `prepareWorktree` and `runSetupScript` are silently discarded, so plugin callers requesting a bootstrapped worktree/setup on the first turn get a normal turn start and the preparation never runs. Consider stripping only `createThread` from `bootstrap` instead of discarding the whole object.
| const fileSystem = yield* FileSystem.FileSystem; | ||
| const path = yield* Path.Path; | ||
| const staticRoot = path.resolve(staticDir); | ||
| const serveIndexHtml = (indexPath: string) => |
There was a problem hiding this comment.
🟡 Medium src/http.ts:254
serveIndexHtml uses Effect.orElseSucceed(() => null) to convert readFile failures into a 404 Not Found, but these failures include permission errors and transient I/O errors — not just missing files. So a request for an existing index.html that the server cannot read returns a cacheable 404 instead of 500, while the same failure for a non-HTML asset at line 310 correctly returns 500. This masks runtime faults and gives clients the wrong status code. Consider distinguishing file-not-found from other read errors so that only genuine missing files produce 404.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/http.ts around line 254:
`serveIndexHtml` uses `Effect.orElseSucceed(() => null)` to convert `readFile` failures into a `404 Not Found`, but these failures include permission errors and transient I/O errors — not just missing files. So a request for an existing `index.html` that the server cannot read returns a cacheable `404` instead of `500`, while the same failure for a non-HTML asset at line 310 correctly returns `500`. This masks runtime faults and gives clients the wrong status code. Consider distinguishing `file-not-found` from other read errors so that only genuine missing files produce `404`.
| readInstalledManifest(pluginId, entry).pipe( | ||
| Effect.map( | ||
| (manifest): PluginInfo => ({ | ||
| id: manifest.id, |
There was a problem hiding this comment.
🟡 Medium plugins/PluginCatalog.ts:78
pluginInfoFromLockfileEntry sets id from manifest.id (the on-disk manifest.json) instead of the lockfile key pluginId. If the installed manifest is stale or tampered and its id differs from the lockfile entry, the returned PluginInfo reports the wrong plugin ID and can duplicate another plugin's ID in the list. Consider using PluginId.make(pluginId) for the id field, consistent with fallbackPluginInfo.
| id: manifest.id, | |
| id: PluginId.make(pluginId), |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginCatalog.ts around line 78:
`pluginInfoFromLockfileEntry` sets `id` from `manifest.id` (the on-disk `manifest.json`) instead of the lockfile key `pluginId`. If the installed manifest is stale or tampered and its `id` differs from the lockfile entry, the returned `PluginInfo` reports the wrong plugin ID and can duplicate another plugin's ID in the list. Consider using `PluginId.make(pluginId)` for the `id` field, consistent with `fallbackPluginInfo`.
| ) ?? null | ||
| ); | ||
| }).pipe( | ||
| Effect.flatMap((row) => { |
There was a problem hiding this comment.
🟡 Medium capabilities/AgentsCapability.ts:200
readTerminalTurn deletes the turnAliases entry at line 204 as soon as the first waiter reads the terminal row. Because startTurn returns a session-local turnId alias (not a durable id), any later awaitTurn call for the same turn will miss in getByTurnId, fail to find the alias, and then in awaitTerminalTurn re-check after subscribe returns null — so it falls back to Deferred.await on a deferred that will never complete, timing out even though the turn already finished. Consider not pruning the alias until the turn is genuinely complete at the projection level and all waiters can resolve it, or keying the deferred so multiple waiters share one completion signal.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 200:
`readTerminalTurn` deletes the `turnAliases` entry at line 204 as soon as the first waiter reads the terminal row. Because `startTurn` returns a session-local `turnId` alias (not a durable id), any later `awaitTurn` call for the same turn will miss in `getByTurnId`, fail to find the alias, and then in `awaitTerminalTurn` re-check after subscribe returns `null` — so it falls back to `Deferred.await` on a deferred that will never complete, timing out even though the turn already finished. Consider not pruning the alias until the turn is genuinely complete at the projection level and all waiters can resolve it, or keying the deferred so multiple waiters share one completion signal.
| mutate<E>((lockfile) => | ||
| Effect.gen(function* () { | ||
| const current = lockfile.plugins[id]; | ||
| const nextPlugin = yield* fn({ lockfile, current }); | ||
| const plugins = { ...lockfile.plugins }; |
There was a problem hiding this comment.
🟡 Medium plugins/PluginLockfileStore.ts:409
updatePlugin passes the live lockfile object to the callback via PluginLockfileMutationContext. The callback's return value is only used to replace lockfile.plugins[id], but the rest of the lockfile (e.g. sources) is reused by reference. A callback that mutates nested objects in place — such as context.lockfile.sources.push(...) or mutating fields on context.current — will silently persist those changes even when it returns undefined or an unchanged plugin, corrupting unrelated lockfile state. Consider passing defensive (deep) copies of lockfile and current to the callback so in-place mutations cannot escape into the persisted lockfile.
mutate<E>((lockfile) =>
Effect.gen(function* () {
- const current = lockfile.plugins[id];
- const nextPlugin = yield* fn({ lockfile, current });
+ const current = lockfile.plugins[id];
+ const context: PluginLockfileMutationContext = {
+ lockfile: structuredClone(lockfile),
+ current: current === undefined ? undefined : structuredClone(current),
+ };
+ const nextPlugin = yield* fn(context);
const plugins = { ...lockfile.plugins };🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginLockfileStore.ts around lines 409-413:
`updatePlugin` passes the live `lockfile` object to the callback via `PluginLockfileMutationContext`. The callback's return value is only used to replace `lockfile.plugins[id]`, but the rest of the lockfile (e.g. `sources`) is reused by reference. A callback that mutates nested objects in place — such as `context.lockfile.sources.push(...)` or mutating fields on `context.current` — will silently persist those changes even when it returns `undefined` or an unchanged plugin, corrupting unrelated lockfile state. Consider passing defensive (deep) copies of `lockfile` and `current` to the callback so in-place mutations cannot escape into the persisted lockfile.
|
|
||
| const quoteShellArg = (value: string) => `'${value.replaceAll("'", "'\\''")}'`; | ||
|
|
||
| const commandLine = (command: string, args: ReadonlyArray<string> | undefined) => |
There was a problem hiding this comment.
🟡 Medium capabilities/TerminalsCapability.ts:11
commandLine wraps the command and every argument in POSIX single quotes, but on Windows the terminal falls back to cmd.exe, which treats single quotes as literal characters rather than delimiters. A command like C:\Program Files\tool.exe or any argument containing spaces gets written as 'C:\Program Files\tool.exe' ..., which cmd.exe fails to parse correctly. If supporting Windows hosts is intended, the escaping should branch on the target shell; otherwise consider documenting that commandLine is POSIX-only.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/TerminalsCapability.ts around line 11:
`commandLine` wraps the command and every argument in POSIX single quotes, but on Windows the terminal falls back to `cmd.exe`, which treats single quotes as literal characters rather than delimiters. A command like `C:\Program Files\tool.exe` or any argument containing spaces gets written as `'C:\Program Files\tool.exe' ...`, which `cmd.exe` fails to parse correctly. If supporting Windows hosts is intended, the escaping should branch on the target shell; otherwise consider documenting that `commandLine` is POSIX-only.
| Effect.catchCause((cause) => | ||
| input.logger.error("plugin service failed; restarting", { | ||
| service: input.service.name, | ||
| cause: Cause.pretty(cause), | ||
| }), | ||
| ), |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:306
startService uses Effect.catchCause, which also catches Cause.Interrupt. Since the service fiber is started with Effect.forkScoped, closing the plugin scope during shutdown interrupts it, but the interruption is caught here and converted into a logged success. Effect.repeat then treats that as a restartable outcome and re-runs the service, causing restart/log spam or shutdown hangs during plugin teardown. Consider re-raising interruption instead of recovering from it — e.g., use Effect.catchCause only when the cause is not an interruption, or use a more targeted catch that excludes Cause.isInterrupt.
- input.service.run({ pluginId: input.pluginId, logger: input.logger }).pipe(
- Effect.catchCause((cause) =>
- input.logger.error("plugin service failed; restarting", {
- service: input.service.name,
- cause: Cause.pretty(cause),
- }),
- ),
+ input.service.run({ pluginId: input.pluginId, logger: input.logger }).pipe(
+ Effect.catchCause((cause) =>
+ Cause.isInterrupt(cause)
+ ? Effect.failCause(cause)
+ : input.logger.error("plugin service failed; restarting", {
+ service: input.service.name,
+ cause: Cause.pretty(cause),
+ }),
+ ),Also found in 1 other location(s)
apps/server/src/plugins/PluginRpcDispatcher.ts:129
mapPluginHandlerCausepreserves interrupt-only causes viaEffect.failCause(cause)at line 129. If a plugin RPC handler returnsEffect.interrupt(or otherwise ends with an interrupt-onlyCause),dispatcher.call()is interrupted instead of failing with aPluginRpcError. That breaks the declaredEffect<unknown, PluginRpcError>contract and can make the RPC request terminate without any typed error response.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around lines 306-311:
`startService` uses `Effect.catchCause`, which also catches `Cause.Interrupt`. Since the service fiber is started with `Effect.forkScoped`, closing the plugin scope during shutdown interrupts it, but the interruption is caught here and converted into a logged success. `Effect.repeat` then treats that as a restartable outcome and re-runs the service, causing restart/log spam or shutdown hangs during plugin teardown. Consider re-raising interruption instead of recovering from it — e.g., use `Effect.catchCause` only when the cause is not an interruption, or use a more targeted catch that excludes `Cause.isInterrupt`.
Also found in 1 other location(s):
- apps/server/src/plugins/PluginRpcDispatcher.ts:129 -- `mapPluginHandlerCause` preserves interrupt-only causes via `Effect.failCause(cause)` at line 129. If a plugin RPC handler returns `Effect.interrupt` (or otherwise ends with an interrupt-only `Cause`), `dispatcher.call()` is interrupted instead of failing with a `PluginRpcError`. That breaks the declared `Effect<unknown, PluginRpcError>` contract and can make the RPC request terminate without any typed error response.
| return row !== null && terminalState(row.state); | ||
| } | ||
|
|
||
| function pendingRequestFromActivity(activity: { |
There was a problem hiding this comment.
🟡 Medium capabilities/AgentsCapability.ts:112
pendingRequestFromActivity returns every approval.requested and user-input.requested activity as a pending request without checking whether a later approval.resolved, user-input.resolved, or failure activity with the same requestId exists in the thread history. listPendingRequests maps over thread.value.activities and flat-maps each one through this function, so it returns already-resolved requests as if they were still open whenever the thread contains both the original request and its resolution event.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 112:
`pendingRequestFromActivity` returns every `approval.requested` and `user-input.requested` activity as a pending request without checking whether a later `approval.resolved`, `user-input.resolved`, or failure activity with the same `requestId` exists in the thread history. `listPendingRequests` maps over `thread.value.activities` and flat-maps each one through this function, so it returns already-resolved requests as if they were still open whenever the thread contains both the original request and its resolution event.
| yield* migration.up.pipe( | ||
| Effect.provideService(SqlClient.SqlClient, sql), | ||
| Effect.mapError( | ||
| (cause) => | ||
| new PluginMigrationExecutionError({ | ||
| pluginId, | ||
| version: migration.version, | ||
| cause, | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:218
migration.up is wrapped with Effect.mapError, which only catches typed errors. If a plugin migration dies (e.g. throws inside Effect.gen or uses Effect.die), the defect bypasses PluginMigrationExecutionError entirely and escapes as a raw cause, violating the PluginMigratorError contract and losing the pluginId/version context. Use Effect.catchAllCause (or Effect.sandbox/Effect.catchAllDefect) instead so both failures and defects are wrapped in PluginMigrationExecutionError.
yield* migration.up.pipe(
Effect.provideService(SqlClient.SqlClient, sql),
- Effect.mapError(
- (cause) =>
- new PluginMigrationExecutionError({
- pluginId,
- version: migration.version,
- cause,
- }),
- ),
+ Effect.catchAllCause((cause) =>
+ Effect.fail(
+ new PluginMigrationExecutionError({
+ pluginId,
+ version: migration.version,
+ cause,
+ }),
+ ),
+ ),
);🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around lines 218-228:
`migration.up` is wrapped with `Effect.mapError`, which only catches typed errors. If a plugin migration dies (e.g. throws inside `Effect.gen` or uses `Effect.die`), the defect bypasses `PluginMigrationExecutionError` entirely and escapes as a raw cause, violating the `PluginMigratorError` contract and losing the `pluginId`/`version` context. Use `Effect.catchAllCause` (or `Effect.sandbox`/`Effect.catchAllDefect`) instead so both failures and defects are wrapped in `PluginMigrationExecutionError`.
| 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 namespace-reference checks for indexes because line 147 only inspects trigger and view entries. A plugin can create a plugin-prefixed index on a core table (e.g. CREATE INDEX p_demo_idx ON core_items(id)); the name passes the prefix check, and the loop continues without inspecting the index body, so the violation goes undetected. Consider extending the body-reference check to index (and any other object type whose SQL can reference a core table) 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 namespace-reference checks for indexes because line 147 only inspects `trigger` and `view` entries. A plugin can create a plugin-prefixed index on a core table (e.g. `CREATE INDEX p_demo_idx ON core_items(id)`); the name passes the prefix check, and the loop continues without inspecting the index body, so the violation goes undetected. Consider extending the body-reference check to `index` (and any other object type whose SQL can reference a core table) rather than only `trigger` and `view`.
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
Database, secrets, terminals, environments-read, projections-read, source-control, text-generation, and plugin HTTP route facades gated by per-plugin scopes, plus the projection read-model support they sit on. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
Durable agent turn dispatch/observe surface and vcs (git) capability facade for plugins, wired into the plugin host API and server registry. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
Serve plugin web bundles + import-map host shims from the server, load and render plugin UI (routes, sidebar sections, settings pages) in the web app via PluginUiHost, and ship the @t3tools/plugin-sdk-web package. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
| projectsLength={projects.length} | ||
| /> | ||
|
|
||
| <PluginSidebarSections /> |
There was a problem hiding this comment.
🟡 Medium components/Sidebar.tsx:3746
<PluginSidebarSections /> is rendered after <SidebarProjectsContent>, which closes the sidebar's only scrollable region. The plugin sections are therefore mounted outside the scroll area, so with enough projects or plugin items the plugin UI and/or the footer get pushed off-screen with no way to scroll to them, making plugin-contributed sidebar content inaccessible on smaller windows. Consider rendering <PluginSidebarSections /> inside the same SidebarContent/ScrollArea as the project list so it participates in the sidebar's scroll.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/Sidebar.tsx around line 3746:
`<PluginSidebarSections />` is rendered after `<SidebarProjectsContent>`, which closes the sidebar's only scrollable region. The plugin sections are therefore mounted outside the scroll area, so with enough projects or plugin items the plugin UI and/or the footer get pushed off-screen with no way to scroll to them, making plugin-contributed sidebar content inaccessible on smaller windows. Consider rendering `<PluginSidebarSections />` inside the same `SidebarContent`/`ScrollArea` as the project list so it participates in the sidebar's scroll.
|
|
||
| const maxBodyBytes = bodyLimit(descriptor.maxBodyBytes); | ||
| const declaredLength = contentLength(request); | ||
| if (declaredLength !== null && declaredLength > maxBodyBytes) { |
There was a problem hiding this comment.
🟠 High plugins/PluginHttpRoutes.ts:173
When the Content-Length header exceeds maxBodyBytes, the route returns 413 without draining or destroying request.stream. Because HttpServerRequest.stream wraps Node's http.IncomingMessage, unread request bytes remain buffered on the socket, so a client can send oversized bodies on many keep-alive connections to pin sockets and exhaust memory. Consider consuming or destroying the request stream before returning the early 413 (and the 400 body-rejection path).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHttpRoutes.ts around line 173:
When the `Content-Length` header exceeds `maxBodyBytes`, the route returns `413` without draining or destroying `request.stream`. Because `HttpServerRequest.stream` wraps Node's `http.IncomingMessage`, unread request bytes remain buffered on the socket, so a client can send oversized bodies on many keep-alive connections to pin sockets and exhaust memory. Consider consuming or destroying the request stream before returning the early `413` (and the `400` body-rejection path).
| return Effect.void; | ||
| } | ||
|
|
||
| const unavailable = (capability: string) => |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:149
unavailable uses Effect.die to signal a missing capability, so a plugin calling yield* hostApi.vcs and handling it with Effect.catchTag("PluginCapabilityUnavailable", ...) never reaches its catch branch — the fiber dies with a defect instead. PluginHostApi advertises PluginCapabilityUnavailable as a recoverable typed failure, but Effect.die makes it an unrecoverable defect that bypasses typed error handlers. Consider using Effect.fail so the error matches the declared type.
Also found in 1 other location(s)
packages/plugin-sdk/src/index.ts:817
PluginHostApiadvertises each capability property asEffect.Effect<..., PluginCapabilityUnavailable>, which implies plugins can recover from a missing capability through the normal error channel. The host implementation actually builds missing capabilities withEffect.die(new PluginCapabilityUnavailable(...))inapps/server/src/plugins/PluginHost.ts, so yieldinghostApi.agents/vcs/etc. terminates the fiber as a defect instead of failing withPluginCapabilityUnavailable. A plugin that follows this SDK contract and tries to handle the typed error withcatchTag/catchAllwill be unable to recover and its registration/service will fail unexpectedly.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 149:
`unavailable` uses `Effect.die` to signal a missing capability, so a plugin calling `yield* hostApi.vcs` and handling it with `Effect.catchTag("PluginCapabilityUnavailable", ...)` never reaches its catch branch — the fiber dies with a defect instead. `PluginHostApi` advertises `PluginCapabilityUnavailable` as a recoverable typed failure, but `Effect.die` makes it an unrecoverable defect that bypasses typed error handlers. Consider using `Effect.fail` so the error matches the declared type.
Also found in 1 other location(s):
- packages/plugin-sdk/src/index.ts:817 -- `PluginHostApi` advertises each capability property as `Effect.Effect<..., PluginCapabilityUnavailable>`, which implies plugins can recover from a missing capability through the normal error channel. The host implementation actually builds missing capabilities with `Effect.die(new PluginCapabilityUnavailable(...))` in `apps/server/src/plugins/PluginHost.ts`, so yielding `hostApi.agents`/`vcs`/etc. terminates the fiber as a defect instead of failing with `PluginCapabilityUnavailable`. A plugin that follows this SDK contract and tries to handle the typed error with `catchTag`/`catchAll` will be unable to recover and its registration/service will fail unexpectedly.
| registerSidebarSection: (registration) => { | ||
| sidebarSections.push({ ...registration, pluginId: plugin.id }); | ||
| }, | ||
| registerSettingsPage: (registration) => { |
There was a problem hiding this comment.
🟡 Medium plugins/PluginUiHost.tsx:191
registerSettingsPage stores the id verbatim, but resolvePluginSettingsPageRegistration normalizes the lookup id via normalizePluginPath. When a plugin registers a settings page with an id like /general/ or foo//bar, the stored id is never normalized, so the resolver strips slashes/duplicates from the lookup key but never matches the stored value — the settings page always renders as not found after navigation. Consider normalizing registration.id in registerSettingsPage the same way registerRoute normalizes registration.path.
Also found in 1 other location(s)
apps/web/src/components/settings/SettingsSidebarNav.tsx:59
getSettingsNavItemsbuilds plugin settings URLs from rawpage.idat line 59, but the settings route later normalizes away empty slash segments before lookup (parseSettingsSplat/resolvePluginSettingsPageRegistration). If a plugin registers an id like"/general/"or"foo//bar", the sidebar link navigates to/settings/<plugin>//general/while lookup searches for normalizedgeneral; the registry still stores the original id, so the page renders the "Plugin settings not found" view and the sidebar entry is unusable.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/plugins/PluginUiHost.tsx around line 191:
`registerSettingsPage` stores the `id` verbatim, but `resolvePluginSettingsPageRegistration` normalizes the lookup id via `normalizePluginPath`. When a plugin registers a settings page with an id like `/general/` or `foo//bar`, the stored id is never normalized, so the resolver strips slashes/duplicates from the lookup key but never matches the stored value — the settings page always renders as not found after navigation. Consider normalizing `registration.id` in `registerSettingsPage` the same way `registerRoute` normalizes `registration.path`.
Also found in 1 other location(s):
- apps/web/src/components/settings/SettingsSidebarNav.tsx:59 -- `getSettingsNavItems` builds plugin settings URLs from raw `page.id` at line 59, but the settings route later normalizes away empty slash segments before lookup (`parseSettingsSplat` / `resolvePluginSettingsPageRegistration`). If a plugin registers an id like `"/general/"` or `"foo//bar"`, the sidebar link navigates to `/settings/<plugin>//general/` while lookup searches for normalized `general`; the registry still stores the original id, so the page renders the "Plugin settings not found" view and the sidebar entry is unusable.
|
|
||
| const objectKey = (entry: SqliteMasterObject) => `${entry.type}:${entry.name}`; | ||
|
|
||
| const changedObjects = ( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:96
changedObjects() compares only the sql text of objects present in the after snapshot. A migration that runs DROP TABLE core_victim; CREATE TABLE core_victim (...) with identical schema text will not appear in removedObjects() (the table exists afterward) nor in changedObjects() (the sql text is unchanged), so the drop-and-recreate of a core table goes undetected and the migration commits successfully despite destroying all rows in a non-plugin table.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 96:
`changedObjects()` compares only the `sql` text of objects present in the `after` snapshot. A migration that runs `DROP TABLE core_victim; CREATE TABLE core_victim (...)` with identical schema text will not appear in `removedObjects()` (the table exists afterward) nor in `changedObjects()` (the `sql` text is unchanged), so the drop-and-recreate of a core table goes undetected and the migration commits successfully despite destroying all rows in a non-plugin table.
| ); | ||
| }); | ||
|
|
||
| const ensureHostSingletonResolution = registrationGate.withPermits(1)( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginModuleLoader.ts:144
ensureHostSingletonResolution catches and logs every failure from registerHook at the Effect.catchCause block, then returns void as if registration succeeded. Callers like PluginHost.start proceed to loadServerEntry without the loader hook installed. When import("node:module"), hook-file access, or nodeModule.register(...) fails transiently, the latch (hostResolutionHookRegistered) stays false, but the caller has no way to know the hook is missing — it continues importing plugins unhooked, which can resolve duplicate or missing effect/SDK modules and leave plugins marked as failed. Consider propagating the error to callers instead of swallowing it, so they can decide whether to abort or retry before activating plugins.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginModuleLoader.ts around line 144:
`ensureHostSingletonResolution` catches and logs every failure from `registerHook` at the `Effect.catchCause` block, then returns `void` as if registration succeeded. Callers like `PluginHost.start` proceed to `loadServerEntry` without the loader hook installed. When `import("node:module")`, hook-file access, or `nodeModule.register(...)` fails transiently, the latch (`hostResolutionHookRegistered`) stays `false`, but the caller has no way to know the hook is missing — it continues importing plugins unhooked, which can resolve duplicate or missing `effect`/SDK modules and leave plugins marked as failed. Consider propagating the error to callers instead of swallowing it, so they can decide whether to abort or retry before activating plugins.
| transform: (stream) => { | ||
| const loadPlugins = listPlugins().pipe( | ||
| Effect.map((result) => result.plugins), | ||
| Effect.catchCause((cause) => |
There was a problem hiding this comment.
🟡 Medium state/plugins.ts:47
environmentPluginListResultAtom catches every listPlugins() failure and returns EMPTY_PLUGIN_LIST, so a transient RPC error is indistinguishable from a legitimately empty plugin list. Downstream, pluginListAtom exposes that empty array as a successful refresh, which causes existing plugin UI registrations to be deleted as though no plugins are active. A single network failure during a refresh silently unloads all plugin-provided routes, settings pages, and commands until another lifecycle event happens to trigger a reload. Consider preserving the previous successful result on failure (or propagating the error) instead of substituting an empty list.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/state/plugins.ts around line 47:
`environmentPluginListResultAtom` catches every `listPlugins()` failure and returns `EMPTY_PLUGIN_LIST`, so a transient RPC error is indistinguishable from a legitimately empty plugin list. Downstream, `pluginListAtom` exposes that empty array as a successful refresh, which causes existing plugin UI registrations to be deleted as though no plugins are active. A single network failure during a refresh silently unloads all plugin-provided routes, settings pages, and commands until another lifecycle event happens to trigger a reload. Consider preserving the previous successful result on failure (or propagating the error) instead of substituting an empty list.
| requireAbsolute("worktreePath", request.worktreePath).pipe( | ||
| Effect.flatMap((cwd) => | ||
| input.git.createRef({ | ||
| cwd, |
There was a problem hiding this comment.
🟡 Medium capabilities/VcsCapability.ts:163
createBranch forwards request.branch directly to input.git.createRef, which runs git branch <refName> without --end-of-options. When branch is --show-current, git prints the current branch and exits successfully without creating anything, yet this method returns success as if a branch were created. Consider inserting --end-of-options before the user-supplied ref name so option-like values are treated as branch names rather than git flags.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/VcsCapability.ts around line 163:
`createBranch` forwards `request.branch` directly to `input.git.createRef`, which runs `git branch <refName>` without `--end-of-options`. When `branch` is `--show-current`, git prints the current branch and exits successfully without creating anything, yet this method returns success as if a branch were created. Consider inserting `--end-of-options` before the user-supplied ref name so option-like values are treated as branch names rather than git flags.
| yield* Effect.logDebug("Accepting commands"); | ||
| yield* commandGate.signalCommandReady; | ||
| yield* Effect.logDebug("startup phase: starting plugin host"); | ||
| yield* runStartupPhase("plugins.start", pluginHost.start); |
There was a problem hiding this comment.
🟡 Medium src/serverRuntimeStartup.ts:436
pluginHost.start is awaited synchronously in the startup sequence, so any plugin that hangs during import, migration, or recover() blocks the startup fiber indefinitely. The server then never reaches http.wait, never publishes the ready event, and never opens the browser or prints headless access info, even though the HTTP listener is already listening. Consider forking pluginHost.start (like keybindings.start and serverSettings.start above) so plugin activation cannot block command readiness and the ready event.
| yield* runStartupPhase("plugins.start", pluginHost.start); | |
| yield* runStartupPhase("plugins.start", pluginHost.start.pipe( | |
| Effect.catch((cause) => | |
| Effect.logWarning("failed to start plugin host", { cause }), | |
| ), | |
| Effect.forkScoped, | |
| )); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/serverRuntimeStartup.ts around line 436:
`pluginHost.start` is awaited synchronously in the startup sequence, so any plugin that hangs during import, migration, or `recover()` blocks the startup fiber indefinitely. The server then never reaches `http.wait`, never publishes the `ready` event, and never opens the browser or prints headless access info, even though the HTTP listener is already listening. Consider forking `pluginHost.start` (like `keybindings.start` and `serverSettings.start` above) so plugin activation cannot block command readiness and the `ready` event.
| } | ||
| } | ||
|
|
||
| const pluginsToLoad = activeWebPlugins.filter((plugin) => !state.loaded.has(plugin.id)); |
There was a problem hiding this comment.
🟡 Medium plugins/PluginUiHost.tsx:163
Once a plugin's import or register fails, syncPluginUiHostRegistrations stores it in state.loaded with failure set, but pluginsToLoad filters out any plugin already present in state.loaded. On every later sync the same active plugin id/version is skipped, so the plugin's UI stays disabled until a full reload or version change even though the transient failure may have resolved. Consider filtering pluginsToLoad against successful loads only (e.g. skip entries where failure === null) so failed plugins are retried.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/plugins/PluginUiHost.tsx around line 163:
Once a plugin's import or `register` fails, `syncPluginUiHostRegistrations` stores it in `state.loaded` with `failure` set, but `pluginsToLoad` filters out any plugin already present in `state.loaded`. On every later sync the same active plugin id/version is skipped, so the plugin's UI stays disabled until a full reload or version change even though the transient failure may have resolved. Consider filtering `pluginsToLoad` against successful loads only (e.g. skip entries where `failure === null`) so failed plugins are retried.
1 participant
What Changed
The web/Electron plugin host. The server serves plugin web bundles same-origin under
/plugins/:id/:version/{web,assets}/*(per-segment decode +../sep/null reject + realpath containment + version gate) behind an import map sharing React/Effect/atoms as host singletons;PluginUiHostloads and renders plugin bundles; plugins contribute routes, sidebar sections, settings pages, command-palette entries, and per-project actions.@t3tools/plugin-sdk-web+@t3tools/sharedcarry the shared surface.Why
Part 6 of 8 of a runtime plugin system (see #3725 for the overview). Builds on #3729. This is the client-side host that lets a plugin ship UI that renders inside the app, isolated in its own error boundary and CSS cascade layer.
Review — four-model tribunal (GPT-5.5 · Grok · GLM-5.2 · Fable)
Fable verified the untrusted-path file server + XSS posture are solid.
PluginSurfaceErrorBoundarynever reset (MUST): one plugin's crash poisoned every plugin surface on the same route →key+resetKeyremount.PluginId.makethrew on a bad URL param (MUST): a typo'd plugin URL hit the root error screen → non-throwing parse routes to the existing not-found view.externals.tsomitted the SDK packages (broken plugin builds) → aligned + drift guard; eagereffect-barrel retention → −104 KB off the eager bundle;plugin-sdk-web→apps/web/srccircular dep documented as a virtual compile-time surface.UI Changes
This part is host infrastructure: it renders whatever a plugin contributes, so there is no standalone screen to screenshot — a plugin surface only appears once a plugin is installed (which needs part 7 / the marketplace). Extension points wire into the existing sidebar, settings nav, and command palette without changing their default appearance when no plugin is present.
Checklist
https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk