Plugin system (7/8): marketplace + install lifecycle#3731
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
vouch:unvouched
![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 violation found: the lockfileError wrapper derives its message from cause.message. See inline comments.
Posted via Macroscope — Effect Service Conventions
| const lockfileError = (cause: unknown) => | ||
| managementError( | ||
| "lockfile", | ||
| cause instanceof Error ? cause.message : "Plugin lockfile update failed.", | ||
| { | ||
| cause, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
lockfileError derives the error message from cause.message, which forwards raw underlying error text into a caller-visible PluginManagementError.message. A wrapper's message should come only from stable structural attributes, never from cause/cause.message. The original failure is already preserved under data.cause, so use a fixed message.
| const lockfileError = (cause: unknown) => | |
| managementError( | |
| "lockfile", | |
| cause instanceof Error ? cause.message : "Plugin lockfile update failed.", | |
| { | |
| cause, | |
| }, | |
| ); | |
| const lockfileError = (cause: unknown) => | |
| managementError("lockfile", "Plugin lockfile update failed.", { | |
| cause, | |
| }); |
Posted via Macroscope — Effect Service Conventions
| const lockfileError = (cause: unknown) => | ||
| managementError( | ||
| "lockfile", | ||
| cause instanceof Error ? cause.message : "Plugin lockfile update failed.", | ||
| { | ||
| cause, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Same issue as in PluginInstaller.ts: lockfileError builds the error message from cause.message, leaking raw underlying error text into a caller-visible PluginManagementError.message. Derive the message only from stable structural attributes and keep the original failure as cause (already retained under data).
| const lockfileError = (cause: unknown) => | |
| managementError( | |
| "lockfile", | |
| cause instanceof Error ? cause.message : "Plugin lockfile update failed.", | |
| { | |
| cause, | |
| }, | |
| ); | |
| const lockfileError = (cause: unknown) => | |
| managementError("lockfile", "Plugin lockfile update failed.", { | |
| cause, | |
| }); |
Posted via Macroscope — Effect Service Conventions
| return null; | ||
| } | ||
| const rows = yield* input.turns.listByThreadId({ threadId }); | ||
| return ( |
There was a problem hiding this comment.
🟠 High capabilities/AgentsCapability.ts:193
readTerminalTurn uses getByTurnId to find a turn by the turnId returned from startTurn, but that ID is generated locally and never persisted as the projection row's turnId, so the direct lookup always returns None. The fallback alias lookup matches on pendingMessageId === alias.messageId, but the projection clears pendingMessageId to null once the turn reaches a terminal state, so the alias lookup also stops matching after completion. As a result, awaitTurn can never locate the finished turn and times out even when the turn completed successfully. Consider resolving the alias to the real projected turnId after the thread.turn.start dispatch (or matching on a field that survives terminal transition) so awaitTerminalTurn can find the completed row.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 193:
`readTerminalTurn` uses `getByTurnId` to find a turn by the `turnId` returned from `startTurn`, but that ID is generated locally and never persisted as the projection row's `turnId`, so the direct lookup always returns `None`. The fallback alias lookup matches on `pendingMessageId === alias.messageId`, but the projection clears `pendingMessageId` to `null` once the turn reaches a terminal state, so the alias lookup also stops matching after completion. As a result, `awaitTurn` can never locate the finished turn and times out even when the turn completed successfully. Consider resolving the alias to the real projected `turnId` after the `thread.turn.start` dispatch (or matching on a field that survives terminal transition) so `awaitTerminalTurn` can find the completed row.
| 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 or user-input.requested activity as pending, and listPendingRequests feeds it the full activity history. Because resolved activities (approval.resolved / user-input.resolved) remain in the array alongside the original request, requests that were already resolved are reported as pending forever. Consider filtering out requests whose requestId has already been resolved by a matching resolution activity.
🤖 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` or `user-input.requested` activity as pending, and `listPendingRequests` feeds it the full activity history. Because resolved activities (`approval.resolved` / `user-input.resolved`) remain in the array alongside the original request, requests that were already resolved are reported as pending forever. Consider filtering out requests whose `requestId` has already been resolved by a matching resolution activity.
|
|
||
| const requireOwnedThread = (threadId: ThreadId) => | ||
| input.snapshots.getThreadOwnerById(threadId).pipe( | ||
| Effect.flatMap((actualOwner) => { |
There was a problem hiding this comment.
🟡 Medium capabilities/AgentsCapability.ts:167
requireOwnedThread treats Option.none() from getThreadOwnerById as an ownership violation. When the thread is missing or deleted, actualOwner is None, but the code falls through to Effect.fail(new AgentsThreadOwnershipError(...)) with actualOwner: null. This means every caller — observeThread, awaitTurn, listPendingRequests, deleteThread, interruptTurn, stopSession, respondToApproval, and respondToUserInput — fails with AgentsThreadOwnershipError for unknown threads instead of AgentsThreadNotFoundError. The AgentsThreadNotFoundError branches in observeThread and listPendingRequests are therefore unreachable when the thread doesn't exist, because requireOwnedThread already failed. Consider returning Effect.void (or yielding AgentsThreadNotFoundError) when actualOwner is None, and only failing with AgentsThreadOwnershipError when an owner exists but doesn't match.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 167:
`requireOwnedThread` treats `Option.none()` from `getThreadOwnerById` as an ownership violation. When the thread is missing or deleted, `actualOwner` is `None`, but the code falls through to `Effect.fail(new AgentsThreadOwnershipError(...))` with `actualOwner: null`. This means every caller — `observeThread`, `awaitTurn`, `listPendingRequests`, `deleteThread`, `interruptTurn`, `stopSession`, `respondToApproval`, and `respondToUserInput` — fails with `AgentsThreadOwnershipError` for unknown threads instead of `AgentsThreadNotFoundError`. The `AgentsThreadNotFoundError` branches in `observeThread` and `listPendingRequests` are therefore unreachable when the thread doesn't exist, because `requireOwnedThread` already failed. Consider returning `Effect.void` (or yielding `AgentsThreadNotFoundError`) when `actualOwner` is `None`, and only failing with `AgentsThreadOwnershipError` when an owner exists but doesn't match.
| directory: input.pluginsDir, | ||
| prefix: `${path.basename(input.lockfilePath)}.`, | ||
| }); | ||
| const tempPath = path.join(tempDir, "contents.tmp"); |
There was a problem hiding this comment.
🟠 High plugins/PluginLockfileStore.ts:172
writeLockfileToPath renames tempPath to input.lockfilePath on line 176 while the file handle opened by fs.open(tempPath, ...) on line 173 is still open — it is only released when the surrounding Effect.scoped(...) exits. On Windows, renaming an open file fails with access-denied errors, so every lockfile write fails on that platform. Consider closing the file handle (e.g. via a nested scope or explicit close) before calling fs.rename.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginLockfileStore.ts around line 172:
`writeLockfileToPath` renames `tempPath` to `input.lockfilePath` on line 176 while the file handle opened by `fs.open(tempPath, ...)` on line 173 is still open — it is only released when the surrounding `Effect.scoped(...)` exits. On Windows, renaming an open file fails with access-denied errors, so every lockfile write fails on that platform. Consider closing the file handle (e.g. via a nested scope or explicit close) before calling `fs.rename`.
| ), | ||
| ); | ||
| return { plugin: yield* pluginInfo(stage.pluginId) }; | ||
| }).pipe(Effect.tapError(() => cleanupStage(stageToken))); |
There was a problem hiding this comment.
🟡 Medium plugins/PluginInstaller.ts:760
In confirmInstall and confirmUpgrade, after moveStagingToVersionDir succeeds, Effect.tapError(() => cleanupStage(stageToken)) only deletes stage.stagingDir — but fs.rename already moved the files to plugins/<id>/<version>. A subsequent lockfile update or activation failure leaves the version directory orphaned on disk with no corresponding lockfile entry. Consider rolling back the version directory on error, or deferring the fs.rename until after the lockfile write succeeds.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginInstaller.ts around line 760:
In `confirmInstall` and `confirmUpgrade`, after `moveStagingToVersionDir` succeeds, `Effect.tapError(() => cleanupStage(stageToken))` only deletes `stage.stagingDir` — but `fs.rename` already moved the files to `plugins/<id>/<version>`. A subsequent lockfile update or activation failure leaves the version directory orphaned on disk with no corresponding lockfile entry. Consider rolling back the version directory on error, or deferring the `fs.rename` until after the lockfile write succeeds.
| function isThreadDetailEvent(event: OrchestrationEvent): boolean { | ||
| return ( | ||
| event.type === "thread.message-sent" || | ||
| event.type === "thread.proposed-plan-upserted" || | ||
| event.type === "thread.activity-appended" || | ||
| event.type === "thread.turn-diff-completed" || | ||
| event.type === "thread.reverted" || | ||
| event.type === "thread.session-set" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Medium capabilities/AgentsCapability.ts:79
isThreadDetailEvent omits thread.meta-updated, thread.runtime-mode-set, and thread.interaction-mode-set, so observeThread's live stream silently drops those events. Plugins subscribed to a thread never receive title/model/branch/runtime-mode/interaction-mode updates and keep a stale view after those changes. Consider adding the missing event types to the filter.
| function isThreadDetailEvent(event: OrchestrationEvent): boolean { | |
| return ( | |
| event.type === "thread.message-sent" || | |
| event.type === "thread.proposed-plan-upserted" || | |
| event.type === "thread.activity-appended" || | |
| event.type === "thread.turn-diff-completed" || | |
| event.type === "thread.reverted" || | |
| event.type === "thread.session-set" | |
| ); | |
| } | |
| function isThreadDetailEvent(event: OrchestrationEvent): boolean { | |
| return ( | |
| event.type === "thread.message-sent" || | |
| event.type === "thread.proposed-plan-upserted" || | |
| event.type === "thread.activity-appended" || | |
| event.type === "thread.turn-diff-completed" || | |
| event.type === "thread.reverted" || | |
| event.type === "thread.session-set" || | |
| event.type === "thread.meta-updated" || | |
| event.type === "thread.runtime-mode-set" || | |
| event.type === "thread.interaction-mode-set" | |
| ); | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around lines 79-88:
`isThreadDetailEvent` omits `thread.meta-updated`, `thread.runtime-mode-set`, and `thread.interaction-mode-set`, so `observeThread`'s live stream silently drops those events. Plugins subscribed to a thread never receive title/model/branch/runtime-mode/interaction-mode updates and keep a stale view after those changes. Consider adding the missing event types to the filter.
| }, | ||
| ); | ||
| } | ||
| yield* store |
There was a problem hiding this comment.
🟡 Medium plugins/PluginManagementRpcHandlers.ts:133
removeSource checks for plugins using the source before calling store.updateSources, but the check happens outside the lockfile mutation. A concurrent install or upgrade can add a plugin referencing that source between the check and the actual removal, so the source is deleted while an installed plugin still points at its sourceId. This leaves the plugin orphaned, breaking later upgrade/update/catalog operations. Consider re-checking for dependent plugins inside the updateSources callback so the validation and mutation are atomic under the lock.
Also found in 1 other location(s)
apps/server/src/plugins/PluginInstaller.ts:738
confirmInstallwrites the new plugin entry atstore.updatePlugin(stage.pluginId, () => ...)without re-checkingassertNoPluginIdCollisioninside the lockfile mutation. A plugin can passbeginInstall, then another install can add a colliding plugin id beforeconfirmInstallruns; this confirm will still commit the staged plugin and leave two installed plugins whosepluginSqlPrefixnamespaces collide, which can corrupt each other's plugin database tables.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginManagementRpcHandlers.ts around line 133:
`removeSource` checks for plugins using the source before calling `store.updateSources`, but the check happens outside the lockfile mutation. A concurrent install or upgrade can add a plugin referencing that source between the check and the actual removal, so the source is deleted while an installed plugin still points at its `sourceId`. This leaves the plugin orphaned, breaking later upgrade/update/catalog operations. Consider re-checking for dependent plugins inside the `updateSources` callback so the validation and mutation are atomic under the lock.
Also found in 1 other location(s):
- apps/server/src/plugins/PluginInstaller.ts:738 -- `confirmInstall` writes the new plugin entry at `store.updatePlugin(stage.pluginId, () => ...)` without re-checking `assertNoPluginIdCollision` inside the lockfile mutation. A plugin can pass `beginInstall`, then another install can add a colliding plugin id before `confirmInstall` runs; this confirm will still commit the staged plugin and leave two installed plugins whose `pluginSqlPrefix` namespaces collide, which can corrupt each other's plugin database tables.
| provideLocalServices( | ||
| semaphore.withPermits(1)( | ||
| Effect.scoped( | ||
| acquireAdvisoryLock({ pluginsDir: config.pluginsDir, advisoryLockPath }).pipe( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginLockfileStore.ts:367
The advisory lock is acquired once at the start of mutate and never refreshed while update(current) runs. If a mutation takes longer than STALE_LOCK_MS (60s), another process treats the lock as stale, reclaims it, and enters the critical section concurrently against its own lockfile snapshot — so both writers can write concurrently and the first writer's update can be lost or corrupt the lockfile, even though the first writer is still active. The ownership re-check before the write only detects reclamation after the fact; it does not prevent a concurrent writer from having already committed. Consider refreshing the lock file's mtime periodically while the mutation is in progress (e.g. touching the lock file on a timer) so that long-running mutations are not incorrectly reclaimed as stale.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginLockfileStore.ts around line 367:
The advisory lock is acquired once at the start of `mutate` and never refreshed while `update(current)` runs. If a mutation takes longer than `STALE_LOCK_MS` (60s), another process treats the lock as stale, reclaims it, and enters the critical section concurrently against its own lockfile snapshot — so both writers can write concurrently and the first writer's update can be lost or corrupt the lockfile, even though the first writer is still active. The ownership re-check before the write only detects reclamation after the fact; it does not prevent a concurrent writer from having already committed. Consider refreshing the lock file's `mtime` periodically while the mutation is in progress (e.g. touching the lock file on a timer) so that long-running mutations are not incorrectly reclaimed as stale.
| } | ||
| }); | ||
|
|
||
| const validateMigrationList = (pluginId: PluginId, migrations: ReadonlyArray<PluginMigration>) => |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:174
validateMigrationList only checks that versions are positive and unique, but never enforces contiguity. When run is given [1, 3], it records MAX(version) = 3 and skips any migration with version <= recordedHead. If migration 2 is added in a later release it is skipped forever, leaving the schema permanently incomplete with no error. Consider rejecting gaps so that versions must form a contiguous 1..N sequence.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 174:
`validateMigrationList` only checks that versions are positive and unique, but never enforces contiguity. When `run` is given `[1, 3]`, it records `MAX(version) = 3` and skips any migration with `version <= recordedHead`. If migration `2` is added in a later release it is skipped forever, leaving the schema permanently incomplete with no error. Consider rejecting gaps so that versions must form a contiguous `1..N` sequence.
| return left.localeCompare(right); | ||
| }; | ||
|
|
||
| const installedEntry = (entry: PluginLockfilePlugin | undefined): PluginLockfilePlugin => { |
There was a problem hiding this comment.
🟡 Medium plugins/PluginInstaller.ts:205
installedEntry throws a PluginManagementError synchronously, but it's called inside functions that return Effect (e.g. updatePlugin(... => Effect.succeed({ ...installedEntry(current), ... })) and beginUpgrade). A synchronous throw inside an Effect body is an uncaught defect, not a typed failure, so a missing plugin crashes with an internal error instead of surfacing as the declared plugin-not-found failure. Consider returning the error via Effect.fail (e.g. by converting installedEntry to return an Effect or using Effect.sync/Effect.try) so it propagates through the PluginManagementError channel.
Also found in 3 other location(s)
apps/server/src/plugins/PluginMarketplace.ts:82
resolveTarballUrlcallsnew URL(input.tarball, input.marketplaceUrl)at line 82 without catching parse failures.MarketplaceVersion.tarballis only a non-empty string, so a marketplace entry like"https://exa mple.com/x.tgz"or":"decodes successfully, butfindVersioninvokesresolveTarballUrldirectly insideEffect.gen. In Effect, that synchronous throw becomes a defect instead of a typedPluginManagementError, so looking up such a version will crash the request path instead of returning a recoverable install error.
apps/server/src/plugins/PluginMigrator.ts:230
migration.upis wrapped withEffect.mapError(...), which only converts typed failures and leaves defects untouched. If a plugin migration throws insideEffect.gen/Effect.syncor otherwise dies instead of failing with the typedErrorchannel,run()escapes with an unwrapped defect instead of returningPluginMigrationExecutionError, so callers lose the promised plugin/version context for real migration crashes.
apps/server/src/plugins/capabilities/AgentsCapability.ts:93
toTimeoutDurationcallsDuration.fromInputUnsafeon any string timeout.AgentsAwaitTurnInput.timeoutis juststring | number, so a plugin can pass an invalid value such as"abc";fromInputUnsafethrows for invalid syntax, turningawaitTurninto an untyped defect/internal error instead of a normal timeout or validation failure.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginInstaller.ts around line 205:
`installedEntry` throws a `PluginManagementError` synchronously, but it's called inside functions that return `Effect` (e.g. `updatePlugin(... => Effect.succeed({ ...installedEntry(current), ... }))` and `beginUpgrade`). A synchronous throw inside an Effect body is an uncaught defect, not a typed failure, so a missing plugin crashes with an internal error instead of surfacing as the declared `plugin-not-found` failure. Consider returning the error via `Effect.fail` (e.g. by converting `installedEntry` to return an `Effect` or using `Effect.sync`/`Effect.try`) so it propagates through the `PluginManagementError` channel.
Also found in 3 other location(s):
- apps/server/src/plugins/PluginMarketplace.ts:82 -- `resolveTarballUrl` calls `new URL(input.tarball, input.marketplaceUrl)` at line 82 without catching parse failures. `MarketplaceVersion.tarball` is only a non-empty string, so a marketplace entry like `"https://exa mple.com/x.tgz"` or `":"` decodes successfully, but `findVersion` invokes `resolveTarballUrl` directly inside `Effect.gen`. In Effect, that synchronous throw becomes a defect instead of a typed `PluginManagementError`, so looking up such a version will crash the request path instead of returning a recoverable install error.
- apps/server/src/plugins/PluginMigrator.ts:230 -- `migration.up` is wrapped with `Effect.mapError(...)`, which only converts typed failures and leaves defects untouched. If a plugin migration throws inside `Effect.gen`/`Effect.sync` or otherwise dies instead of failing with the typed `Error` channel, `run()` escapes with an unwrapped defect instead of returning `PluginMigrationExecutionError`, so callers lose the promised plugin/version context for real migration crashes.
- apps/server/src/plugins/capabilities/AgentsCapability.ts:93 -- `toTimeoutDuration` calls `Duration.fromInputUnsafe` on any string timeout. `AgentsAwaitTurnInput.timeout` is just `string | number`, so a plugin can pass an invalid value such as `"abc"`; `fromInputUnsafe` throws for invalid syntax, turning `awaitTurn` into an untyped defect/internal error instead of a normal timeout or validation failure.
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
Marketplace catalog/source management, staged install/upgrade/uninstall flows over ws RPC, the plugins settings UI, the hello-board fixture plugin, and plugin authoring docs. Claude-Session: https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk
ccdwyer
force-pushed
the
wb/07-marketplace
branch
from
bd01b41 to
be7b6ad
Compare
|
|
||
| const objectKey = (entry: SqliteMasterObject) => `${entry.type}:${entry.name}`; | ||
|
|
||
| const changedObjects = ( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:98
changedObjects compares only the sql text of objects matched by type:name, so a migration that drops and recreates a core object with identical DDL is treated as unchanged. The object is removed from before and re-added in after with the same sql, so neither removedObjects nor changedObjects flags it, and the namespace gate does not detect the modification. Consider tracking object identity across the drop/recreate (e.g., by detecting a core object present in both snapshots regardless of sql equality, or by recording DDL events) so that recreating a core object is treated as a violation.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 98:
`changedObjects` compares only the `sql` text of objects matched by `type:name`, so a migration that drops and recreates a core object with identical DDL is treated as unchanged. The object is removed from `before` and re-added in `after` with the same `sql`, so neither `removedObjects` nor `changedObjects` flags it, and the namespace gate does not detect the modification. Consider tracking object identity across the drop/recreate (e.g., by detecting a core object present in both snapshots regardless of `sql` equality, or by recording DDL events) so that recreating a core object is treated as a violation.
| const tempBefore = yield* sql<{ readonly name: string }>` | ||
| SELECT name FROM sqlite_temp_master | ||
| `.pipe(Effect.orElseSucceed(() => [])); | ||
| yield* migration.up.pipe( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:228
migration.up is wrapped with Effect.mapError at line 230, which only catches typed errors. A defect from the migration (e.g. Effect.die, a thrown exception in Effect.sync, or a rejected Effect.promise) bypasses PluginMigrationExecutionError entirely and escapes to the caller as an untyped defect instead of the declared PluginMigratorError. Consider using Effect.catchAllDefect (or Effect.sandbox/Effect.catchAllCause) so defects are also wrapped in PluginMigrationExecutionError.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 228:
`migration.up` is wrapped with `Effect.mapError` at line 230, which only catches typed errors. A defect from the migration (e.g. `Effect.die`, a thrown exception in `Effect.sync`, or a rejected `Effect.promise`) bypasses `PluginMigrationExecutionError` entirely and escapes to the caller as an untyped defect instead of the declared `PluginMigratorError`. Consider using `Effect.catchAllDefect` (or `Effect.sandbox`/`Effect.catchAllCause`) so defects are also wrapped in `PluginMigrationExecutionError`.
| const unavailable = (capability: string) => | ||
| Effect.die(new PluginCapabilityUnavailable({ capability })); |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:153
unavailable uses Effect.die(new PluginCapabilityUnavailable(...)), so requesting an undeclared capability raises an uncaught defect instead of a typed failure. Plugins that handle PluginCapabilityUnavailable via Effect.catchTag or Effect.mapError will never see it — the defect crashes registration or service startup. Consider using Effect.fail so the error propagates as the advertised typed failure.
-const unavailable = (capability: string) =>
- Effect.die(new PluginCapabilityUnavailable({ capability }));
+const unavailable = (capability: string) =>
+ Effect.fail(new PluginCapabilityUnavailable({ capability }));Also found in 1 other location(s)
packages/plugin-sdk/src/index.ts:817
The capability properties are typed as
Effect.Effect<..., PluginCapabilityUnavailable>, which tells plugin authors they can recover with normal error-channel combinators likeyield*,catchAll, ormapError. But the host actually constructs unavailable capabilities withEffect.die(new PluginCapabilityUnavailable(...)), so those combinators never see the error. A plugin that tries to gracefully handle a missing capability will instead defect during registration/use and get wrapped as a generic host failure.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around lines 153-154:
`unavailable` uses `Effect.die(new PluginCapabilityUnavailable(...))`, so requesting an undeclared capability raises an uncaught defect instead of a typed failure. Plugins that handle `PluginCapabilityUnavailable` via `Effect.catchTag` or `Effect.mapError` will never see it — the defect crashes registration or service startup. Consider using `Effect.fail` so the error propagates as the advertised typed failure.
Also found in 1 other location(s):
- packages/plugin-sdk/src/index.ts:817 -- The capability properties are typed as `Effect.Effect<..., PluginCapabilityUnavailable>`, which tells plugin authors they can recover with normal error-channel combinators like `yield*`, `catchAll`, or `mapError`. But the host actually constructs unavailable capabilities with `Effect.die(new PluginCapabilityUnavailable(...))`, so those combinators never see the error. A plugin that tries to gracefully handle a missing capability will instead defect during registration/use and get wrapped as a generic host failure.
There was a problem hiding this comment.
🟡 Medium
The publish method accepts "plugins" events, but nextEvents only persists "welcome" and "ready" — a "plugins" event is never added to the snapshot. Clients subscribing after a plugin event is published miss it because snapshot never contains it.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/serverLifecycleEvents.ts around line 43:
The `publish` method accepts `"plugins"` events, but `nextEvents` only persists `"welcome"` and `"ready"` — a `"plugins"` event is never added to the snapshot. Clients subscribing after a plugin event is published miss it because `snapshot` never contains it.
| : 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
When the lockfile does not exist, readLockfileFromPath returns the shared EMPTY_PLUGIN_LOCKFILE object by reference. Mutations passed through updateSources/updatePlugin (e.g., Array.from(sources) or { ...lockfile, plugins }) may still mutate nested arrays/objects in place, so subsequent reads of a still-missing lockfile return a stale, in-memory copy populated with phantom plugins or sources — even though nothing was written to disk. Consider returning a fresh copy of EMPTY_PLUGIN_LOCKFILE so each read yields an independent object.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginLockfileStore.ts around line 143:
When the lockfile does not exist, `readLockfileFromPath` returns the shared `EMPTY_PLUGIN_LOCKFILE` object by reference. Mutations passed through `updateSources`/`updatePlugin` (e.g., `Array.from(sources)` or `{ ...lockfile, plugins }`) may still mutate nested arrays/objects in place, so subsequent reads of a still-missing lockfile return a stale, in-memory copy populated with phantom plugins or sources — even though nothing was written to disk. Consider returning a fresh copy of `EMPTY_PLUGIN_LOCKFILE` so each read yields an independent object.
| ), | ||
| Stream.runDrain, | ||
| ); | ||
| yield* waitForEvent.pipe(Effect.forkScoped); |
There was a problem hiding this comment.
🟡 Medium capabilities/AgentsCapability.ts:245
awaitTerminalTurn() forks waitForEvent with Effect.forkScoped and then waits on Deferred.await, but never joins or observes the forked fiber. If readTerminalTurn() fails inside the stream listener (e.g. ProjectionTurnRepository returns an error), the forked fiber dies silently and awaitTurn() waits until timeout instead of surfacing the real failure. Consider propagating errors from the forked stream into the deferred so the caller sees the actual error.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 245:
`awaitTerminalTurn()` forks `waitForEvent` with `Effect.forkScoped` and then waits on `Deferred.await`, but never joins or observes the forked fiber. If `readTerminalTurn()` fails inside the stream listener (e.g. `ProjectionTurnRepository` returns an error), the forked fiber dies silently and `awaitTurn()` waits until timeout instead of surfacing the real failure. Consider propagating errors from the forked stream into the deferred so the caller sees the actual error.
| if (Exit.isFailure(exit)) { | ||
| yield* Scope.close(scope, exit); | ||
| const message = Cause.pretty(exit.cause); | ||
| yield* markFailure(pluginId, message); |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHost.ts:506
When activation succeeds enough to register the plugin at registry.put (line 473) but then fails before completing (e.g. clearHealthyActivation fails), the failure branch at lines 505-510 closes the scope and calls markFailure but never calls registry.remove(pluginId). The plugin stays in PluginRuntimeRegistry, so subsequent activatePlugin calls hit the early Option.isSome return and never retry, leaving the plugin stuck until process restart.
if (Exit.isFailure(exit)) {
yield* Scope.close(scope, exit);
+ yield* registry.remove(pluginId).pipe(Effect.ignore);
const message = Cause.pretty(exit.cause);🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around lines 506-509:
When activation succeeds enough to register the plugin at `registry.put` (line 473) but then fails before completing (e.g. `clearHealthyActivation` fails), the failure branch at lines 505-510 closes the scope and calls `markFailure` but never calls `registry.remove(pluginId)`. The plugin stays in `PluginRuntimeRegistry`, so subsequent `activatePlugin` calls hit the early `Option.isSome` return and never retry, leaving the plugin stuck until process restart.
| .pipe( | ||
| Effect.tapError(() => | ||
| createdThread | ||
| ? input.engine | ||
| .dispatch({ | ||
| type: "thread.delete", | ||
| commandId: nextCommandId("thread-create-rollback"), | ||
| threadId: request.threadId, | ||
| }) | ||
| .pipe( | ||
| Effect.ignore, | ||
| Effect.andThen(Effect.sync(() => turnAliases.delete(String(turnId)))), | ||
| ) | ||
| : Effect.void, | ||
| ), |
There was a problem hiding this comment.
🟡 Medium capabilities/AgentsCapability.ts:357
turnAliases grows unbounded: entries are inserted at line 334 before the thread.turn.start dispatch, but they are only deleted in readTerminalTurn (when the turn reaches a terminal state) or in the rollback branch when a newly created thread fails. When thread.turn.start fails on an existing thread, or when a plugin starts a turn but never calls awaitTurn, the alias entry is never removed, so a long-lived plugin process accumulates stale turnId -> messageId mappings indefinitely. Consider deleting the alias in the Effect.tapError cleanup for the existing-thread failure path as well, and/or pruning aliases that never reach a terminal state.
- Effect.tapError(() =>
+ Effect.tapError(() =>
createdThread
? input.engine
.dispatch({
type: "thread.delete",
commandId: nextCommandId("thread-create-rollback"),
threadId: request.threadId,
})
.pipe(
Effect.ignore,
Effect.andThen(Effect.sync(() => turnAliases.delete(String(turnId)))),
)
- : Effect.void,
+ : Effect.sync(() => turnAliases.delete(String(turnId))),🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around lines 357-371:
`turnAliases` grows unbounded: entries are inserted at line 334 before the `thread.turn.start` dispatch, but they are only deleted in `readTerminalTurn` (when the turn reaches a terminal state) or in the rollback branch when a newly created thread fails. When `thread.turn.start` fails on an existing thread, or when a plugin starts a turn but never calls `awaitTurn`, the alias entry is never removed, so a long-lived plugin process accumulates stale `turnId -> messageId` mappings indefinitely. Consider deleting the alias in the `Effect.tapError` cleanup for the existing-thread failure path as well, and/or pruning aliases that never reach a terminal state.
| }); | ||
| return { handle, snapshot }; | ||
| }), | ||
| observe: (handle, listener) => |
There was a problem hiding this comment.
🟠 High capabilities/TerminalsCapability.ts:63
observe, sendInput, and kill forward the caller-supplied threadId/terminalId directly to TerminalManager without verifying they match the plugin:${input.pluginId}:... namespace that spawn uses. A plugin with the terminals capability can pass another plugin's handle (or any guessed threadId/terminalId) to read output, inject input, or terminate that session. Consider validating in each of these methods that the handle belongs to the current plugin (e.g. by checking the live map or verifying the threadId prefix) before delegating to the manager.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/TerminalsCapability.ts around line 63:
`observe`, `sendInput`, and `kill` forward the caller-supplied `threadId`/`terminalId` directly to `TerminalManager` without verifying they match the `plugin:${input.pluginId}:...` namespace that `spawn` uses. A plugin with the `terminals` capability can pass another plugin's handle (or any guessed `threadId`/`terminalId`) to read output, inject input, or terminate that session. Consider validating in each of these methods that the handle belongs to the current plugin (e.g. by checking the `live` map or verifying the `threadId` prefix) before delegating to the manager.
| ); | ||
| } | ||
|
|
||
| export function resolveTarballUrl(input: { |
There was a problem hiding this comment.
🟠 High plugins/PluginMarketplace.ts:78
resolveTarballUrl allows any absolute file: URL when T3_PLUGIN_DEV=1, regardless of whether the marketplace itself was loaded from an untrusted HTTPS source. A remote marketplace can publish "file:///etc/passwd" as its tarball, and PluginInstaller.downloadBytes will read that local file from disk, turning plugin installation in dev mode into an arbitrary local file read driven by remote content. Consider restricting file: tarball URLs to the same scheme as the marketplace URL, or only allowing them when the marketplace itself was loaded from a file: source.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMarketplace.ts around line 78:
`resolveTarballUrl` allows any absolute `file:` URL when `T3_PLUGIN_DEV=1`, regardless of whether the marketplace itself was loaded from an untrusted HTTPS source. A remote marketplace can publish `"file:///etc/passwd"` as its tarball, and `PluginInstaller.downloadBytes` will read that local file from disk, turning plugin installation in dev mode into an arbitrary local file read driven by remote content. Consider restricting `file:` tarball URLs to the same scheme as the marketplace URL, or only allowing them when the marketplace itself was loaded from a `file:` source.
1 participant
What Changed
The install lifecycle: marketplace sources + catalog, an install-consent dialog (shows every requested capability from the staged manifest), install / enable / disable / uninstall / upgrade, streaming-safe download with sha256 + gzip-bomb protection, a hand-written safe tar extractor (traversal/symlink/absolute rejected, size-capped), a single-writer lockfile with a safe-mode crash counter, the
plugins:manage-gated management RPCs, the settings UI, and an in-repohello-boardexample fixture.Why
Part 7 of 8 of a runtime plugin system (see #3725 for the overview). Builds on #3730. This is what actually lets a user browse a source, review the capabilities a plugin requests, and install it — the untrusted-tarball + untrusted-marketplace-data ingestion path.
Review — four-model tribunal (GPT-5.5 · Grok · GLM-5.2 · Fable)
Fable/Grok verified the tar extractor, sha256 ordering, and size caps are solid; findings were on the network-ingestion edges + render:
httpssource can no longer 30x-redirect to169.254.169.254/loopback. Legit CDN redirects still work.javascript:URI inauthor.url(SHOULD): rendered into an<a href>(dangerous in Electron) → scheme-gated tohttp(s)at decode +rel="noreferrer".UI Changes
Adds a Plugins settings page (sources list, catalog browse, install button) and an install-consent dialog that lists every capability a plugin requests before confirming. These are net-new screens gated behind Settings; I can attach a capture on request (the flow needs a running marketplace source to populate).
Checklist
https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk