Plugin system (4/8): mechanical capability façades#3728
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}
| } | ||
| }); | ||
|
|
||
| export const make = Effect.fn("PluginMigrator.make")(function* () { |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:184
The run method reads recordedHead via SELECT MAX(version), applies each pending migration, then inserts into plugin_migrations with no per-plugin lock or uniqueness guard around that sequence. When two fibers call run for the same pluginId concurrently, both read the same recordedHead and both execute the same migration; the second execution then fails on duplicate DDL (e.g. CREATE TABLE ... already exists) or on the primary key of (plugin_id, version), surfacing as a spurious PluginMigrationExecutionError even though the plugin package is fine. Consider serializing migrations per plugin (e.g. a per-plugin lock or INSERT ... ON CONFLICT DO NOTHING/DO UPDATE guard with a check constraint) so concurrent callers don't re-run migrations.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 184:
The `run` method reads `recordedHead` via `SELECT MAX(version)`, applies each pending migration, then inserts into `plugin_migrations` with no per-plugin lock or uniqueness guard around that sequence. When two fibers call `run` for the same `pluginId` concurrently, both read the same `recordedHead` and both execute the same migration; the second execution then fails on duplicate DDL (e.g. `CREATE TABLE ... already exists`) or on the primary key of `(plugin_id, version)`, surfacing as a spurious `PluginMigrationExecutionError` even though the plugin package is fine. Consider serializing migrations per plugin (e.g. a per-plugin lock or `INSERT ... ON CONFLICT DO NOTHING`/`DO UPDATE` guard with a check constraint) so concurrent callers don't re-run migrations.
There was a problem hiding this comment.
🟡 Medium
When a project contains only plugin-owned threads (created via thread.create with owner set to a non-"user" value), project.delete checks listThreadsByProjectId and rejects deletion because it sees those threads as non-deleted, even though the UI snapshot query filters them out via owner = DEFAULT_THREAD_OWNER. The project appears empty in the UI but the delete fails with "project is not empty" unless the caller passes force=true. Consider filtering activeThreads in the project.delete case to exclude non-"user"-owned threads (or otherwise align the invariant check with the visibility rules used by snapshot queries).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/decider.ts around line 169:
When a project contains only plugin-owned threads (created via `thread.create` with `owner` set to a non-`"user"` value), `project.delete` checks `listThreadsByProjectId` and rejects deletion because it sees those threads as non-deleted, even though the UI snapshot query filters them out via `owner = DEFAULT_THREAD_OWNER`. The project appears empty in the UI but the delete fails with "project is not empty" unless the caller passes `force=true`. Consider filtering `activeThreads` in the `project.delete` case to exclude non-`"user"`-owned threads (or otherwise align the invariant check with the visibility rules used by snapshot queries).
|
|
||
| const objectKey = (entry: SqliteMasterObject) => `${entry.type}:${entry.name}`; | ||
|
|
||
| const changedObjects = ( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:96
Because changedObjects only compares type:name keys and sql text against the before-snapshot, a migration that drops a core object and recreates it with the same name and identical definition goes undetected — the recreated object reappears in the after-snapshot with the same key and sql, so changedObjects excludes it. The removedObjects check also misses it since the object still exists afterward. This lets a plugin destroy all rows in a core table (or otherwise clobber core data) without triggering a PluginMigrationViolation. Consider persisting a marker that survives drops — e.g., a per-object rowid, creation timestamp, or hash of the before-snapshot's object identity — so a drop-and-recreate is distinguishable from an unchanged object.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 96:
Because `changedObjects` only compares `type:name` keys and `sql` text against the before-snapshot, a migration that drops a core object and recreates it with the same name and identical definition goes undetected — the recreated object reappears in the after-snapshot with the same key and `sql`, so `changedObjects` excludes it. The `removedObjects` check also misses it since the object still exists afterward. This lets a plugin destroy all rows in a core table (or otherwise clobber core data) without triggering a `PluginMigrationViolation`. Consider persisting a marker that survives drops — e.g., a per-object rowid, creation timestamp, or hash of the before-snapshot's object identity — so a drop-and-recreate is distinguishable from an unchanged object.
| const exit = yield* activation.pipe(Scope.provide(scope), Effect.exit); | ||
| if (Exit.isFailure(exit)) { | ||
| yield* Scope.close(scope, exit); | ||
| const message = Cause.pretty(exit.cause); | ||
| yield* updateFailure(store, pluginId, message); |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:444
registry.put at line 412 stores the plugin in PluginRuntimeRegistry before activation finishes, but neither the success path nor the failure path ever calls registry.remove. When activation fails after line 412, lines 444-449 close the scope and mark the lockfile entry failed, yet the stale runtime entry remains in the registry. Callers that look up the plugin via PluginRuntimeRegistry will keep finding it and return not-ready forever because readiness was never completed, and plugin catalogs will misreport the failed plugin as active. Consider adding registry.remove(pluginId) to the activation-failure path (and to the scope finalizer) so the runtime entry is cleaned up when the plugin is torn down.
const exit = yield* activation.pipe(Scope.provide(scope), Effect.exit);
if (Exit.isFailure(exit)) {
+ yield* registry.remove(pluginId).pipe(Effect.ignore);
yield* Scope.close(scope, exit);
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 444-448:
`registry.put` at line 412 stores the plugin in `PluginRuntimeRegistry` before activation finishes, but neither the success path nor the failure path ever calls `registry.remove`. When activation fails after line 412, lines 444-449 close the scope and mark the lockfile entry failed, yet the stale runtime entry remains in the registry. Callers that look up the plugin via `PluginRuntimeRegistry` will keep finding it and return `not-ready` forever because `readiness` was never completed, and plugin catalogs will misreport the failed plugin as active. Consider adding `registry.remove(pluginId)` to the activation-failure path (and to the scope finalizer) so the runtime entry is cleaned up when the plugin is torn down.
| listTurnsByThreadId: ({ threadId, limit }) => | ||
| input.turns | ||
| .listByThreadId({ threadId }) | ||
| .pipe(Effect.map((rows) => rows.slice(0, boundedLimit(limit)))), | ||
| listMessagesByThreadId: ({ threadId, limit }) => | ||
| input.messages | ||
| .listByThreadId({ threadId }) | ||
| .pipe(Effect.map((rows) => rows.slice(0, boundedLimit(limit)).map(toMessage))), | ||
| listActivitiesByThreadId: ({ threadId, limit }) => | ||
| input.activities | ||
| .listByThreadId({ threadId }) | ||
| .pipe(Effect.map((rows) => rows.slice(0, boundedLimit(limit)).map(toActivity))), |
There was a problem hiding this comment.
🟠 High capabilities/ProjectionsReadCapability.ts:70
listTurnsByThreadId, listMessagesByThreadId, and listActivitiesByThreadId pass only { threadId } to listByThreadId, ignoring the caller's limit and slicing in memory afterward. This loads every matching row from the database even when the caller requests a small limit, causing unbounded memory use and latency on large threads. Consider passing limit: boundedLimit(limit) to listByThreadId so the repository applies the SQL LIMIT.
listTurnsByThreadId: ({ threadId, limit }) =>
input.turns
- .listByThreadId({ threadId })
- .pipe(Effect.map((rows) => rows.slice(0, boundedLimit(limit)))),
+ .listByThreadId({ threadId, limit: boundedLimit(limit) }),
listMessagesByThreadId: ({ threadId, limit }) =>
input.messages
- .listByThreadId({ threadId })
- .pipe(Effect.map((rows) => rows.slice(0, boundedLimit(limit)).map(toMessage))),
+ .listByThreadId({ threadId, limit: boundedLimit(limit) })
+ .pipe(Effect.map((rows) => rows.map(toMessage))),
listActivitiesByThreadId: ({ threadId, limit }) =>
input.activities
- .listByThreadId({ threadId })
- .pipe(Effect.map((rows) => rows.slice(0, boundedLimit(limit)).map(toActivity))),
+ .listByThreadId({ threadId, limit: boundedLimit(limit) })
+ .pipe(Effect.map((rows) => rows.map(toActivity))),🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/ProjectionsReadCapability.ts around lines 70-81:
`listTurnsByThreadId`, `listMessagesByThreadId`, and `listActivitiesByThreadId` pass only `{ threadId }` to `listByThreadId`, ignoring the caller's `limit` and slicing in memory afterward. This loads every matching row from the database even when the caller requests a small `limit`, causing unbounded memory use and latency on large threads. Consider passing `limit: boundedLimit(limit)` to `listByThreadId` so the repository applies the SQL `LIMIT`.
| } | ||
| const stat = statOption.value; | ||
| const mtime = Option.getOrUndefined(stat.mtime); | ||
| const ageMs = mtime ? (yield* Clock.currentTimeMillis) - mtime.getTime() : 0; |
There was a problem hiding this comment.
🟡 Medium plugins/PluginLockfileStore.ts:246
The advisory lock is considered stale based only on the file's mtime (ageMs <= STALE_LOCK_MS), but the timestamp is never refreshed while a legitimate holder is still working. Any lockfile mutation that takes longer than 60 seconds (e.g., a large write or a paused/scheduled process) gets reclaimed by another process even though the first writer is still active, so two writers proceed concurrently and one update is silently lost. Consider refreshing the lock file's mtime (e.g., rewriting the token or touching the file) on a heartbeat while the holder is still working, so long-running mutations are not falsely treated as stale.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginLockfileStore.ts around line 246:
The advisory lock is considered stale based only on the file's `mtime` (`ageMs <= STALE_LOCK_MS`), but the timestamp is never refreshed while a legitimate holder is still working. Any lockfile mutation that takes longer than 60 seconds (e.g., a large write or a paused/scheduled process) gets reclaimed by another process even though the first writer is still active, so two writers proceed concurrently and one update is silently lost. Consider refreshing the lock file's `mtime` (e.g., rewriting the token or touching the file) on a heartbeat while the holder is still working, so long-running mutations are not falsely treated as stale.
| readonly service: PluginServiceDescriptor; | ||
| }) => | ||
| input.service.run({ pluginId: input.pluginId, logger: input.logger }).pipe( | ||
| Effect.catchCause((cause) => |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:278
startService uses Effect.catchCause to log and swallow all failures, which also catches fiber interruptions. Since service fibers are started with Effect.forkScoped, plugin shutdown interrupts them via Scope.close — but that interrupt is logged as a service failure and Effect.repeat schedules a restart, so the service keeps restarting during teardown and blocks clean scope closure. Consider filtering out interruption causes (e.g. with Effect.catchSomeCause checking Cause.isInterrupted) before logging and retrying.
Also found in 1 other location(s)
apps/server/src/plugins/PluginHttpRoutes.ts:224
The outer
Effect.catchCauseat line224converts every failure cause into a500response, including fiber interruptions. When the client disconnects or the server cancels the request while reading the body or running the plugin handler, that interruption is swallowed instead of being re-raised, so the route logs a warning and tries to write500 Internal Server Errorto an already-cancelled request instead of stopping promptly.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 278:
`startService` uses `Effect.catchCause` to log and swallow all failures, which also catches fiber interruptions. Since service fibers are started with `Effect.forkScoped`, plugin shutdown interrupts them via `Scope.close` — but that interrupt is logged as a service failure and `Effect.repeat` schedules a restart, so the service keeps restarting during teardown and blocks clean scope closure. Consider filtering out interruption causes (e.g. with `Effect.catchSomeCause` checking `Cause.isInterrupted`) before logging and retrying.
Also found in 1 other location(s):
- apps/server/src/plugins/PluginHttpRoutes.ts:224 -- The outer `Effect.catchCause` at line `224` converts every failure cause into a `500` response, including fiber interruptions. When the client disconnects or the server cancels the request while reading the body or running the plugin handler, that interruption is swallowed instead of being re-raised, so the route logs a warning and tries to write `500 Internal Server Error` to an already-cancelled request instead of stopping promptly.
| return Effect.void; | ||
| } | ||
|
|
||
| const unavailable = (capability: string) => |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:141
unavailable uses Effect.die, which raises an unrecoverable defect that bypasses typed error handlers. When a plugin probes an undeclared capability and catches PluginCapabilityUnavailable, the defect aborts the fiber instead of yielding the advertised Effect.fail value. Use Effect.fail so the error flows through normal recovery channels.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 141:
`unavailable` uses `Effect.die`, which raises an unrecoverable defect that bypasses typed error handlers. When a plugin probes an undeclared capability and catches `PluginCapabilityUnavailable`, the defect aborts the fiber instead of yielding the advertised `Effect.fail` value. Use `Effect.fail` so the error flows through normal recovery channels.
| rows: request.rows ?? 30, | ||
| }); | ||
| live.set(terminalId, handle); | ||
| yield* input.manager.write({ |
There was a problem hiding this comment.
🟡 Medium capabilities/TerminalsCapability.ts:57
In spawn, if input.manager.write(...) fails or the fiber is interrupted after manager.open succeeds, the effect exits without closing the newly opened terminal. The handle is added to live but never returned to the caller on failure, so the caller can't clean it up — the underlying PTY leaks until plugin shutdown runs. Consider wrapping the write in an effect that closes the terminal on failure, e.g. via Effect.catchAll / Effect.ensuring that calls closeHandle(handle) when the write fails.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/TerminalsCapability.ts around line 57:
In `spawn`, if `input.manager.write(...)` fails or the fiber is interrupted after `manager.open` succeeds, the effect exits without closing the newly opened terminal. The handle is added to `live` but never returned to the caller on failure, so the caller can't clean it up — the underlying PTY leaks until plugin `shutdown` runs. Consider wrapping the `write` in an effect that closes the terminal on failure, e.g. via `Effect.catchAll` / `Effect.ensuring` that calls `closeHandle(handle)` when the write fails.
| })), | ||
| ), | ||
| discoverProviders: input.registry.discover, | ||
| listOpenPullRequests: (request) => input.github.listOpenPullRequests(request), |
There was a problem hiding this comment.
🟠 High capabilities/SourceControlCapability.ts:21
makeSourceControlCapability forwards the plugin-supplied cwd and bodyFile arguments directly into GitHubCli without validating that they are confined to the plugin's workspace. A plugin with the sourceControl capability can pass any cwd path on disk and use createPullRequest({ bodyFile }) to have gh pr create --body-file <path> read an arbitrary local file (such as /etc/passwd) into the PR body, exfiltrating host files outside its granted workspace. Consider validating cwd and bodyFile against the plugin's allowed paths before forwarding them to GitHubCli, or document why this is acceptable for your threat model.
Also found in 2 other location(s)
packages/plugin-sdk/src/index.ts:272
SourceControlCapability.createPullRequestexposes a rawbodyFilepath and the implementation forwards it directly togh pr create --body-file. There is no containment check against the plugin's granted workspace (the current test even passes/tmp/body.md), so a plugin can point this at any readable local file and exfiltrate its contents into a pull request body.
apps/server/src/plugins/capabilities/TerminalsCapability.ts:63
observe,sendInput, andkillforward the caller-suppliedthreadId/terminalIdstraight intoTerminalManagerwithout checking that the handle belongs to this plugin'splugin:${pluginId}:...namespace.TerminalSessionHandleis just a plain object, so a plugin can forge{ threadId: "some-user-thread", terminalId: "default" }and then read output from or close arbitrary host terminals.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/SourceControlCapability.ts around line 21:
`makeSourceControlCapability` forwards the plugin-supplied `cwd` and `bodyFile` arguments directly into `GitHubCli` without validating that they are confined to the plugin's workspace. A plugin with the `sourceControl` capability can pass any `cwd` path on disk and use `createPullRequest({ bodyFile })` to have `gh pr create --body-file <path>` read an arbitrary local file (such as `/etc/passwd`) into the PR body, exfiltrating host files outside its granted workspace. Consider validating `cwd` and `bodyFile` against the plugin's allowed paths before forwarding them to `GitHubCli`, or document why this is acceptable for your threat model.
Also found in 2 other location(s):
- packages/plugin-sdk/src/index.ts:272 -- `SourceControlCapability.createPullRequest` exposes a raw `bodyFile` path and the implementation forwards it directly to `gh pr create --body-file`. There is no containment check against the plugin's granted workspace (the current test even passes `/tmp/body.md`), so a plugin can point this at any readable local file and exfiltrate its contents into a pull request body.
- apps/server/src/plugins/capabilities/TerminalsCapability.ts:63 -- `observe`, `sendInput`, and `kill` forward the caller-supplied `threadId` / `terminalId` straight into `TerminalManager` without checking that the handle belongs to this plugin's `plugin:${pluginId}:...` namespace. `TerminalSessionHandle` is just a plain object, so a plugin can forge `{ threadId: "some-user-thread", terminalId: "default" }` and then read output from or close arbitrary host terminals.
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
ccdwyer
force-pushed
the
wb/04-capabilities-mechanical
branch
from
e7bbc9b to
7535a23
Compare
| } | ||
| }); | ||
|
|
||
| const exit = yield* activation.pipe(Scope.provide(scope), Effect.exit); |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:444
loadPlugin creates a detached plugin scope with Scope.make("sequential") (line 366), but on the success path that scope is never closed or tied to the caller's lifetime with Effect.addFinalizer. On activation failure the scope is closed via Scope.close(scope, exit) (line 446), but on success the scope stays open indefinitely. This means plugin service fibers forked with Effect.forkScoped / Scope.provide(scope) (line 414) and capability teardown finalizers registered on that scope (line 398) never run on shutdown — long-lived services like one ending in Effect.never outlive the server runtime and keep the process hanging, and resources such as terminals and HTTP registrations leak. Consider adding Effect.addFinalizer on the success path to close the plugin scope when the host's surrounding scope closes.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 444:
`loadPlugin` creates a detached plugin scope with `Scope.make("sequential")` (line 366), but on the success path that scope is never closed or tied to the caller's lifetime with `Effect.addFinalizer`. On activation failure the scope is closed via `Scope.close(scope, exit)` (line 446), but on success the scope stays open indefinitely. This means plugin service fibers forked with `Effect.forkScoped` / `Scope.provide(scope)` (line 414) and capability teardown finalizers registered on that scope (line 398) never run on shutdown — long-lived services like one ending in `Effect.never` outlive the server runtime and keep the process hanging, and resources such as terminals and HTTP registrations leak. Consider adding `Effect.addFinalizer` on the success path to close the plugin scope when the host's surrounding scope closes.
| ), | ||
| otlpTracesProxyRouteLayer, | ||
| assetRouteLayer, | ||
| pluginHttpRouteLayer, |
There was a problem hiding this comment.
🟡 Medium src/server.ts:415
Mounting pluginHttpRouteLayer under browserApiCorsLayer makes every /hooks/plugins/* endpoint advertise only the fixed browserApiCorsAllowedMethods set (GET, POST, OPTIONS) and the fixed browserApiCorsAllowedHeaders allowlist. Plugin routes registered via PluginHttpRegistry can use arbitrary HTTP methods like PUT, PATCH, or DELETE, and plugin handlers can expect custom request headers. Browser CORS preflight for any such route will fail because the advertised methods and headers do not include what the route requires, even though the route itself is registered and functional for non-browser clients. Consider applying a broader or per-plugin CORS policy to /hooks/plugins/* so plugin-defined methods and headers are reflected in preflight responses.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/server.ts around line 415:
Mounting `pluginHttpRouteLayer` under `browserApiCorsLayer` makes every `/hooks/plugins/*` endpoint advertise only the fixed `browserApiCorsAllowedMethods` set (`GET`, `POST`, `OPTIONS`) and the fixed `browserApiCorsAllowedHeaders` allowlist. Plugin routes registered via `PluginHttpRegistry` can use arbitrary HTTP methods like `PUT`, `PATCH`, or `DELETE`, and plugin handlers can expect custom request headers. Browser CORS preflight for any such route will fail because the advertised methods and headers do not include what the route requires, even though the route itself is registered and functional for non-browser clients. Consider applying a broader or per-plugin CORS policy to `/hooks/plugins/*` so plugin-defined methods and headers are reflected in preflight responses.
| return { api, teardown }; | ||
| }; | ||
|
|
||
| const upgradeLockfileEntry = ( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHost.ts:234
upgradeLockfileEntry preserves the old activation object (including crashCount) when promoting a pending-upgrade plugin to the new staged version. If the previous version crashed once before the upgrade, the upgraded version inherits crashCount=1, so the next startup failure in reconcilePendingState treats it as a repeated crash (crashCount >= 2) and immediately disables the new version. Consider resetting crashCount to 0 when promoting the staged version.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 234:
`upgradeLockfileEntry` preserves the old `activation` object (including `crashCount`) when promoting a `pending-upgrade` plugin to the new staged version. If the previous version crashed once before the upgrade, the upgraded version inherits `crashCount=1`, so the next startup failure in `reconcilePendingState` treats it as a repeated crash (`crashCount >= 2`) and immediately disables the new version. Consider resetting `crashCount` to `0` when promoting the staged version.
| ); | ||
| }); | ||
|
|
||
| const ensureHostSingletonResolution = registrationGate.withPermits(1)( |
There was a problem hiding this comment.
🟡 Medium plugins/PluginModuleLoader.ts:144
A registerHook failure is caught and converted to a warning, but hostResolutionHookRegistered is also not set, so subsequent callers retry — including PluginHost.start continuing in the same run. That run proceeds into loadServerEntry and imports plugins without the resolve hook ever being registered, so plugins import effect off the host singleton (or fail to resolve at all) and become persistently stuck in a failed state. The transient register() failure should abort startup rather than degrading to a broken plugin activation. Consider allowing the failure to propagate to callers (at least the first one) instead of swallowing it with a warning, or having loadServerEntry fail when the hook is not yet registered.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginModuleLoader.ts around line 144:
A `registerHook` failure is caught and converted to a warning, but `hostResolutionHookRegistered` is also not set, so subsequent callers retry — including `PluginHost.start` continuing in the same run. That run proceeds into `loadServerEntry` and imports plugins without the resolve hook ever being registered, so plugins import `effect` off the host singleton (or fail to resolve at all) and become persistently stuck in a failed state. The transient `register()` failure should abort startup rather than degrading to a broken plugin activation. Consider allowing the failure to propagate to callers (at least the first one) instead of swallowing it with a warning, or having `loadServerEntry` fail when the hook is not yet registered.
| import * as ServerConfig from "../../config.ts"; | ||
| import * as ServerSecretStore from "../../auth/ServerSecretStore.ts"; | ||
|
|
||
| const keyPrefix = (pluginId: PluginId) => `plugin:${pluginId}:`; |
There was a problem hiding this comment.
🟠 High capabilities/SecretsCapability.ts:12
The keyPrefix function embeds colons in secret keys (plugin:${pluginId}:), and the backing store maps keys directly to filenames. On Windows, : is a reserved filename character, so set("api-key", ...) attempts to create a file like plugin:<id>:api-key.bin and fails. This makes the secrets capability unusable on Windows. Consider using a colon-free separator (e.g., __ or -) in keyPrefix so the generated keys are valid filenames across platforms.
| const keyPrefix = (pluginId: PluginId) => `plugin:${pluginId}:`; | |
| +const keyPrefix = (pluginId: PluginId) => `plugin__${pluginId}__`; |
Also found in 1 other location(s)
apps/server/src/plugins/capabilities/TerminalsCapability.ts:11
commandLinealways emits a POSIX-style single-quoted shell command like'echo' 'hello world'.TerminalManagerdefaults topwsh.exeon Windows, and PowerShell treats a quoted command name as a string literal unless it is invoked with&, so plugin terminal spawns on Windows will not run the requested command at all.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/SecretsCapability.ts around line 12:
The `keyPrefix` function embeds colons in secret keys (`plugin:${pluginId}:`), and the backing store maps keys directly to filenames. On Windows, `:` is a reserved filename character, so `set("api-key", ...)` attempts to create a file like `plugin:<id>:api-key.bin` and fails. This makes the secrets capability unusable on Windows. Consider using a colon-free separator (e.g., `__` or `-`) in `keyPrefix` so the generated keys are valid filenames across platforms.
Also found in 1 other location(s):
- apps/server/src/plugins/capabilities/TerminalsCapability.ts:11 -- `commandLine` always emits a POSIX-style single-quoted shell command like `'echo' 'hello world'`. `TerminalManager` defaults to `pwsh.exe` on Windows, and PowerShell treats a quoted command name as a string literal unless it is invoked with `&`, so plugin terminal spawns on Windows will not run the requested command at all.
| 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 checks the SQL body of trigger and view objects for references to core tables; all other object types (e.g. index, table) hit continue and skip the body check. A prefixed CREATE INDEX on a core table (e.g. CREATE UNIQUE INDEX p_demo_idx ON projection_threads(...)) or a prefixed table with a REFERENCES clause pointing at a core table therefore passes validation. This lets plugin migrations mutate host schema outside the plugin namespace and can cause later host writes to fail or behave differently. Consider extending the body-reference check to index and table objects (for indexes, inspect the indexed table; for tables, inspect REFERENCES/foreign-key clauses).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 112:
`validateMigrationObjects` only checks the SQL body of `trigger` and `view` objects for references to core tables; all other object types (e.g. `index`, `table`) hit `continue` and skip the body check. A prefixed `CREATE INDEX` on a core table (e.g. `CREATE UNIQUE INDEX p_demo_idx ON projection_threads(...)`) or a prefixed table with a `REFERENCES` clause pointing at a core table therefore passes validation. This lets plugin migrations mutate host schema outside the plugin namespace and can cause later host writes to fail or behave differently. Consider extending the body-reference check to `index` and `table` objects (for indexes, inspect the indexed table; for tables, inspect `REFERENCES`/foreign-key clauses).
| const pluginInfoFromLockfileEntry = (pluginId: string, entry: PluginLockfilePlugin) => | ||
| readInstalledManifest(pluginId, entry).pipe( | ||
| Effect.map( | ||
| (manifest): PluginInfo => ({ | ||
| id: manifest.id, | ||
| name: manifest.name, | ||
| version: manifest.version, | ||
| state: entry.state, | ||
| capabilities: Array.from(manifest.capabilities), | ||
| hasWeb: manifest.entries.web !== undefined, | ||
| lastError: entry.lastError, | ||
| }), | ||
| ), |
There was a problem hiding this comment.
🟡 Medium plugins/PluginCatalog.ts:74
pluginInfoFromLockfileEntry returns manifest.id, manifest.name, and manifest.version from the on-disk manifest instead of the lockfile key and entry.version. If the installed manifest.json is stale or tampered but still schema-valid, PluginCatalog.list publishes a plugin record whose id does not match the lockfile entry, so the UI can show or act on the wrong plugin identity. Consider using pluginId and entry.version from the lockfile as the source of truth, and only read the manifest for supplemental fields like capabilities and hasWeb.
| const pluginInfoFromLockfileEntry = (pluginId: string, entry: PluginLockfilePlugin) => | |
| readInstalledManifest(pluginId, entry).pipe( | |
| Effect.map( | |
| (manifest): PluginInfo => ({ | |
| id: manifest.id, | |
| name: manifest.name, | |
| version: manifest.version, | |
| state: entry.state, | |
| capabilities: Array.from(manifest.capabilities), | |
| hasWeb: manifest.entries.web !== undefined, | |
| lastError: entry.lastError, | |
| }), | |
| ), | |
| const pluginInfoFromLockfileEntry = (pluginId: string, entry: PluginLockfilePlugin) => | |
| readInstalledManifest(pluginId, entry).pipe( | |
| Effect.map( | |
| (manifest): PluginInfo => ({ | |
| id: PluginId.make(pluginId), | |
| name: manifest.name, | |
| version: entry.version, | |
| state: entry.state, | |
| capabilities: Array.from(manifest.capabilities), | |
| hasWeb: manifest.entries.web !== undefined, | |
| lastError: entry.lastError, | |
| }), | |
| ), |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginCatalog.ts around lines 74-86:
`pluginInfoFromLockfileEntry` returns `manifest.id`, `manifest.name`, and `manifest.version` from the on-disk manifest instead of the lockfile key and `entry.version`. If the installed `manifest.json` is stale or tampered but still schema-valid, `PluginCatalog.list` publishes a plugin record whose `id` does not match the lockfile entry, so the UI can show or act on the wrong plugin identity. Consider using `pluginId` and `entry.version` from the lockfile as the source of truth, and only read the manifest for supplemental fields like `capabilities` and `hasWeb`.
1 participant
What Changed
The curated capability façades handed to a plugin only when its manifest declares them:
database(plugin-prefixed tables),secrets(plugin:<id>:name grammar),environments.read,projections.read(id-keyed, capped),textGeneration,sourceControl,terminals, plus the inboundhttproute registry serving/hooks/plugins/:pluginId/*. Each is a narrow, auditable surface.Why
Part 4 of 8 of a runtime plugin system (see #3725 for the overview). Builds on #3727. These are the safe, curated APIs a plugin actually uses — nothing raw is exposed except where deliberate (documented below).
Review — four-model tribunal (GPT-5.5 · Grok · GLM-5.2 · Fable)
sendInput/kill/observeforwarded a caller handle unchecked → ownership guard rejects handles outside the plugin's namespace;spawnmade interrupt-safe.bodyFileexfiltration (MUST):createPullRequest({ bodyFile })could read an arbitrary local file into the PR body → containment to the plugin's granted workspace.projections.readcap loaded all rows then sliced (SHOULD): bound pushed into the SQLLIMIT.:secret filenames (SHOULD): Windows-safe key encoding.databasecapability exposes the rawSqlClient— a plugin granteddatabasealready has full in-process trust; thep_<id>_*prefix is a migration-time convention, not a runtime sandbox.UI Changes
N/A — server only.
Checklist
https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk