Plugin system (5/8): agents + vcs capabilities#3729
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}
| prefix: `${path.basename(input.lockfilePath)}.`, | ||
| }); | ||
| const tempPath = path.join(tempDir, "contents.tmp"); | ||
| const file = yield* fs.open(tempPath, { flag: "w", mode: 0o600 }); |
There was a problem hiding this comment.
🟡 Medium plugins/PluginLockfileStore.ts:173
writeLockfileToPath renames tempPath into place at line 176 while the file handle opened at line 173 is still open inside the Effect.scoped scope. On Windows, renaming an open file fails with a sharing violation, so every lockfile write can fail instead of updating the lockfile. Close the file handle (e.g. by scoping the open call or explicitly closing it) before calling fs.rename.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginLockfileStore.ts around line 173:
`writeLockfileToPath` renames `tempPath` into place at line 176 while the file handle opened at line 173 is still open inside the `Effect.scoped` scope. On Windows, renaming an open file fails with a sharing violation, so every lockfile write can fail instead of updating the lockfile. Close the file handle (e.g. by scoping the `open` call or explicitly closing it) before calling `fs.rename`.
| 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 without checking whether a corresponding approval.resolved, user-input.resolved, or stale-response event has since closed that requestId. As a result, listPendingRequests returns already-resolved prompts as actionable pending requests, so plugins see stale approval and user-input prompts that are no longer pending. Consider filtering out activities whose requestId has been resolved by a later activity before returning them.
🤖 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 without checking whether a corresponding `approval.resolved`, `user-input.resolved`, or stale-response event has since closed that `requestId`. As a result, `listPendingRequests` returns already-resolved prompts as actionable pending requests, so plugins see stale approval and user-input prompts that are no longer pending. Consider filtering out activities whose `requestId` has been resolved by a later activity before returning them.
| // Re-anchor the resolution to a host module so Node's resolver finds the | ||
| // host's `effect`/SDK (handles bare packages AND arbitrary subpaths), keeping | ||
| // the plugin on the host's singleton instances. | ||
| return nextResolve(specifier, { parentURL: hostAnchorUrl ?? context.parentURL }); |
There was a problem hiding this comment.
🟡 Medium plugins/pluginResolveHooks.ts:37
When re-anchoring resolution for effect/effect/*/@t3tools/plugin-sdk, resolve calls nextResolve with { parentURL: ... }, dropping context.conditions and context.importAttributes. This breaks conditional-export resolution and causes imports with attributes (e.g. with { type: "json" }) to fail or cache under the wrong key. Consider spreading the original context: { ...context, parentURL: ... }.
| return nextResolve(specifier, { parentURL: hostAnchorUrl ?? context.parentURL }); | |
| return nextResolve(specifier, { ...context, parentURL: hostAnchorUrl ?? context.parentURL }); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/pluginResolveHooks.ts around line 37:
When re-anchoring resolution for `effect`/`effect/*`/`@t3tools/plugin-sdk`, `resolve` calls `nextResolve` with `{ parentURL: ... }`, dropping `context.conditions` and `context.importAttributes`. This breaks conditional-export resolution and causes imports with attributes (e.g. `with { type: "json" }`) to fail or cache under the wrong key. Consider spreading the original context: `{ ...context, parentURL: ... }`.
| }).pipe( | ||
| Effect.flatMap((row) => { | ||
| if (!isTerminalTurn(row)) return Effect.succeed(null); | ||
| // Prune the alias once the turn is terminal so the in-memory map does |
There was a problem hiding this comment.
🟠 High capabilities/AgentsCapability.ts:202
readTerminalTurn deletes the turnAliases entry at line 204 as soon as any caller observes a terminal row. The turnId returned from startTurn is a synthetic local ID never sent to the orchestration engine, so subsequent awaitTurn calls for the same turn rely on the alias to correlate the projected row via pendingMessageId. Once one waiter deletes the alias, concurrent or later awaitTurn calls fall back to getByTurnId and never find the real row, causing them to time out even though the turn is already terminal. Consider retaining the alias after the turn is terminal (e.g., by not deleting it in readTerminalTurn) so all waiters can resolve the turn.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 202:
`readTerminalTurn` deletes the `turnAliases` entry at line 204 as soon as any caller observes a terminal row. The `turnId` returned from `startTurn` is a synthetic local ID never sent to the orchestration engine, so subsequent `awaitTurn` calls for the same turn rely on the alias to correlate the projected row via `pendingMessageId`. Once one waiter deletes the alias, concurrent or later `awaitTurn` calls fall back to `getByTurnId` and never find the real row, causing them to time out even though the turn is already terminal. Consider retaining the alias after the turn is terminal (e.g., by not deleting it in `readTerminalTurn`) so all waiters can resolve the turn.
| } | ||
| }); | ||
|
|
||
| const validateMigrationList = (pluginId: PluginId, migrations: ReadonlyArray<PluginMigration>) => |
There was a problem hiding this comment.
🟡 Medium plugins/PluginMigrator.ts:164
The run function sorts migrations and applies every version above recordedHead, but never verifies that the versions form a contiguous sequence starting from recordedHead. If a plugin ships [1, 3] (missing 2) or upgrades from recorded head 1 directly to 3, the loop skips migration 2, applies 3, and records version 3 in plugin_migrations. This leaves the plugin schema/data in a partially migrated state with no opportunity to run the skipped step. Consider rejecting non-contiguous migration lists in validateMigrationList.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginMigrator.ts around line 164:
The `run` function sorts migrations and applies every version above `recordedHead`, but never verifies that the versions form a contiguous sequence starting from `recordedHead`. If a plugin ships `[1, 3]` (missing `2`) or upgrades from recorded head `1` directly to `3`, the loop skips migration `2`, applies `3`, and records version `3` in `plugin_migrations`. This leaves the plugin schema/data in a partially migrated state with no opportunity to run the skipped step. Consider rejecting non-contiguous migration lists in `validateMigrationList`.
| }); | ||
| return { handle, snapshot }; | ||
| }), | ||
| observe: (handle, listener) => |
There was a problem hiding this comment.
🟠 High capabilities/TerminalsCapability.ts:63
observe, sendInput, and kill forward caller-supplied TerminalSessionHandle fields directly to input.manager without verifying the threadId matches the plugin:${input.pluginId}: prefix or that the terminalId is present in live. A plugin can fabricate a handle for another plugin's terminal session and read its output, inject input, or close it by guessing or knowing the ids. Consider validating that the handle's threadId belongs to this plugin (or that its terminalId exists in live) before forwarding it 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 caller-supplied `TerminalSessionHandle` fields directly to `input.manager` without verifying the `threadId` matches the `plugin:${input.pluginId}:` prefix or that the `terminalId` is present in `live`. A plugin can fabricate a handle for another plugin's terminal session and read its output, inject input, or close it by guessing or knowing the ids. Consider validating that the handle's `threadId` belongs to this plugin (or that its `terminalId` exists in `live`) before forwarding it to the manager.
| : {}), | ||
| runtimeMode: DEFAULT_RUNTIME_MODE, | ||
| interactionMode: DEFAULT_PROVIDER_INTERACTION_MODE, | ||
| ...(turnBootstrap !== undefined ? { bootstrap: turnBootstrap as any } : {}), |
There was a problem hiding this comment.
🟡 Medium capabilities/AgentsCapability.ts:354
startTurn forwards turnBootstrap into the thread.turn.start dispatch (line 354), but the decider ignores command.bootstrap for thread.turn.start — only the WebSocket entrypoint's dispatchBootstrapTurnStart handles bootstrap.prepareWorktree and runSetupScript. As a result, a plugin that passes bootstrap.prepareWorktree or runSetupScript through startTurn gets a successful turn start with none of the requested bootstrap work executed. Consider routing bootstrap through the WebSocket entrypoint path, or document that startTurn silently drops bootstrap worktree/setup-script configuration.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 354:
`startTurn` forwards `turnBootstrap` into the `thread.turn.start` dispatch (line 354), but the decider ignores `command.bootstrap` for `thread.turn.start` — only the WebSocket entrypoint's `dispatchBootstrapTurnStart` handles `bootstrap.prepareWorktree` and `runSetupScript`. As a result, a plugin that passes `bootstrap.prepareWorktree` or `runSetupScript` through `startTurn` gets a successful turn start with none of the requested bootstrap work executed. Consider routing bootstrap through the WebSocket entrypoint path, or document that `startTurn` silently drops bootstrap worktree/setup-script configuration.
| merge: (request) => | ||
| Effect.gen(function* () { | ||
| const cwd = yield* requireAbsolute("worktreePath", request.worktreePath); | ||
| const args = ["merge", request.ref]; |
There was a problem hiding this comment.
🟠 High capabilities/VcsCapability.ts:197
The merge capability passes request.ref directly into the args array ["merge", request.ref] with no -- separator. When a plugin supplies a value starting with -- (e.g. --abort, --quit, --continue), git merge interprets it as a subcommand option instead of a ref, so the capability runs an unintended control action that mutates repository state and then returns a misleading merge result. Insert -- before request.ref so git treats it strictly as a ref argument.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/VcsCapability.ts around line 197:
The `merge` capability passes `request.ref` directly into the args array `["merge", request.ref]` with no `--` separator. When a plugin supplies a value starting with `--` (e.g. `--abort`, `--quit`, `--continue`), `git merge` interprets it as a subcommand option instead of a ref, so the capability runs an unintended control action that mutates repository state and then returns a misleading merge result. Insert `--` before `request.ref` so git treats it strictly as a ref argument.
| yield* httpRegistry.put(pluginId, registration.http ?? []); | ||
| yield* Scope.addFinalizer(scope, httpRegistry.remove(pluginId)); | ||
| } | ||
| yield* registry.put(pluginId, { manifest, registration, readiness, scope }); |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:446
registry.put(pluginId, { manifest, registration, readiness, scope }) registers the plugin runtime but no corresponding registry.remove finalizer is added to the plugin scope. If activation fails after line 446, or if the plugin scope is later closed for a stop/disable/crash, the stale runtime entry remains in PluginRuntimeRegistry and downstream consumers (PluginRpcDispatcher, PluginCatalog) will continue treating the dead plugin as active. Consider adding yield* Scope.addFinalizer(scope, registry.remove(pluginId)) immediately after the registry.put call, mirroring the pattern already used for httpRegistry.
- yield* registry.put(pluginId, { manifest, registration, readiness, scope });
+ yield* registry.put(pluginId, { manifest, registration, readiness, scope });
+ yield* Scope.addFinalizer(scope, registry.remove(pluginId));🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 446:
`registry.put(pluginId, { manifest, registration, readiness, scope })` registers the plugin runtime but no corresponding `registry.remove` finalizer is added to the plugin scope. If activation fails after line 446, or if the plugin scope is later closed for a stop/disable/crash, the stale runtime entry remains in `PluginRuntimeRegistry` and downstream consumers (`PluginRpcDispatcher`, `PluginCatalog`) will continue treating the dead plugin as active. Consider adding `yield* Scope.addFinalizer(scope, registry.remove(pluginId))` immediately after the `registry.put` call, mirroring the pattern already used for `httpRegistry`.
| ); | ||
| } | ||
| yield* Deferred.succeed(readiness, undefined).pipe(Effect.orDie); | ||
| const clearHealthyActivation = store.updatePlugin(pluginId, ({ current }) => |
There was a problem hiding this comment.
🟠 High plugins/PluginHost.ts:454
The delayed clearHealthyActivation effect leaves activation.activatingSince non-null for up to 30 seconds after a successful plugin start. On the next process start, reconcilePendingState treats any non-null activatingSince as evidence of a crash and increments crashCount. Two ordinary restarts within that 30-second window disable the plugin with lastError = "disabled after repeated crashes" even though it never crashed. Consider clearing activatingSince synchronously on successful activation (e.g. persist it to null immediately, then apply the crashCount reset after the health delay).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 454:
The delayed `clearHealthyActivation` effect leaves `activation.activatingSince` non-null for up to 30 seconds after a successful plugin start. On the next process start, `reconcilePendingState` treats any non-null `activatingSince` as evidence of a crash and increments `crashCount`. Two ordinary restarts within that 30-second window disable the plugin with `lastError = "disabled after repeated crashes"` even though it never crashed. Consider clearing `activatingSince` synchronously on successful activation (e.g. persist it to null immediately, then apply the `crashCount` reset after the health delay).
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
ccdwyer
force-pushed
the
wb/05-capabilities-agents-vcs
branch
from
9a491b8 to
1c8b40a
Compare
There was a problem hiding this comment.
Effect Service Conventions review. Two error-modeling issues in the new plugin service modules: a wrapped failure is stringified into a detail field instead of being preserved as a structured cause, and a pure domain condition manufactures a synthetic Error to satisfy a required cause. Details inline.
Posted via Macroscope — Effect Service Conventions
| return yield* new PluginLockfileLockError({ | ||
| path: lock.path, | ||
| cause: new Error( | ||
| "advisory lock ownership lost before write (reclaimed by another writer)", | ||
| ), | ||
| }); |
There was a problem hiding this comment.
This is a pure domain condition (advisory-lock ownership lost), not a wrapped lower-level failure, yet it manufactures new Error(...) solely to populate the required cause. The conventions say not to manufacture an Error merely to populate cause, and to make cause optional when the same error can legitimately originate without an underlying failure. Since PluginLockfileLockError is raised both from real fs failures and from this domain check, make cause optional (Schema.optional(Schema.Defect())) and omit it here, moving the explanatory text into the derived message/a structural field.
Posted via Macroscope — Effect Service Conventions
| new PluginRegistrationError({ | ||
| pluginId, | ||
| detail: Cause.pretty(cause), | ||
| }), |
There was a problem hiding this comment.
PluginRegistrationError only carries detail: Schema.String, and here a wrapped failure is stringified into it via Cause.pretty(cause), then interpolated into message. Per the error conventions, when wrapping a real failure the immediate underlying error should be preserved as a structured cause (e.g. cause: Schema.Defect()) rather than flattening arbitrary defect text into a detail field used to build the message.
validateRegistration legitimately uses detail for structured validation strings, so prefer adding a separate cause field and preserving the cause here instead of overloading detail:
| new PluginRegistrationError({ | |
| pluginId, | |
| detail: Cause.pretty(cause), | |
| }), | |
| Effect.catchCause((cause) => | |
| Effect.fail( | |
| new PluginRegistrationError({ | |
| pluginId, | |
| detail: "registration threw", | |
| cause, | |
| }), | |
| ), | |
| ), |
(with a cause: Schema.optional(Schema.Defect()) field added to the error class).
Posted via Macroscope — Effect Service Conventions
| const routesRef = yield* Ref.make(new Map<PluginId, ReadonlyArray<PluginHttpDescriptor>>()); | ||
|
|
||
| return PluginHttpRegistry.of({ | ||
| put: (pluginId, routes) => |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHttpRegistry.ts:72
PluginHttpRegistry.put stores any PluginHttpDescriptor value without validating descriptor.auth, so a plugin that submits a typo like "tokne" instead of "token" has that route treated as public by pluginHttpRouteLayer, which only checks descriptor.auth === "token". The route is then reachable without the required plugin:<id>:operate scope. Consider validating descriptor.auth in put and rejecting descriptors with unrecognized values.
Also found in 1 other location(s)
packages/plugin-sdk/src/index.ts:854
PluginHttpDescriptor.authat line854is only a TypeScript literal type, but the host never validates it at registration time. InPluginHttpRoutes.ts, authentication runs only whendescriptor.auth === "token"; any other runtime value is treated as public. A JS plugin or malformed descriptor using a typo like"Token"or"tokn"will therefore expose an endpoint intended to requireplugin:<id>:operatewithout any auth check.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHttpRegistry.ts around line 72:
`PluginHttpRegistry.put` stores any `PluginHttpDescriptor` value without validating `descriptor.auth`, so a plugin that submits a typo like `"tokne"` instead of `"token"` has that route treated as public by `pluginHttpRouteLayer`, which only checks `descriptor.auth === "token"`. The route is then reachable without the required `plugin:<id>:operate` scope. Consider validating `descriptor.auth` in `put` and rejecting descriptors with unrecognized values.
Also found in 1 other location(s):
- packages/plugin-sdk/src/index.ts:854 -- `PluginHttpDescriptor.auth` at line `854` is only a TypeScript literal type, but the host never validates it at registration time. In `PluginHttpRoutes.ts`, authentication runs only when `descriptor.auth === "token"`; any other runtime value is treated as public. A JS plugin or malformed descriptor using a typo like `"Token"` or `"tokn"` will therefore expose an endpoint intended to require `plugin:<id>:operate` without any auth check.
| Effect.catchCause((cause) => | ||
| Effect.logWarning("Failed to register plugin host singleton resolution hook", { | ||
| cause, | ||
| }), | ||
| ), |
There was a problem hiding this comment.
🟡 Medium plugins/PluginModuleLoader.ts:158
ensureHostSingletonResolution catches and logs registerHook failures via Effect.catchCause at line 158 but still returns success, so callers like PluginHost.start proceed into loadServerEntry() even when the loader hook was never installed. This means plugins get imported without the singleton-resolution hook active — the exact bad-import scenario this change is meant to prevent — and can be permanently marked failed before a later retry succeeds. Consider propagating the failure so callers do not continue when the hook registration failed.
- Effect.catchCause((cause) =>
- Effect.logWarning("Failed to register plugin host singleton resolution hook", {
- cause,
- }),
- ),Also found in 1 other location(s)
apps/server/src/plugins/PluginHost.ts:312
startServiceunconditionally wraps everyservice.run(...)inEffect.repeat(...)after converting failures into a successful log effect. BecauseEffect.repeatrepeats on success, a service that exits normally is restarted forever every ~250ms, and a crashing service is also treated as success and kept in an endless restart loop. Any plugin service that performs one-shot work or is expected to terminate cleanly will be executed repeatedly, causing duplicate side effects and hiding that the original run already completed.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginModuleLoader.ts around lines 158-162:
`ensureHostSingletonResolution` catches and logs `registerHook` failures via `Effect.catchCause` at line 158 but still returns success, so callers like `PluginHost.start` proceed into `loadServerEntry()` even when the loader hook was never installed. This means plugins get imported without the singleton-resolution hook active — the exact bad-import scenario this change is meant to prevent — and can be permanently marked failed before a later retry succeeds. Consider propagating the failure so callers do not continue when the hook registration failed.
Also found in 1 other location(s):
- apps/server/src/plugins/PluginHost.ts:312 -- `startService` unconditionally wraps every `service.run(...)` in `Effect.repeat(...)` after converting failures into a successful log effect. Because `Effect.repeat` repeats on success, a service that exits normally is restarted forever every ~250ms, and a crashing service is also treated as success and kept in an endless restart loop. Any plugin service that performs one-shot work or is expected to terminate cleanly will be executed repeatedly, causing duplicate side effects and hiding that the original run already completed.
| ), | ||
| ); | ||
| } | ||
| }).pipe(Effect.ignoreCause({ log: true })); |
There was a problem hiding this comment.
🟡 Medium plugins/PluginHost.ts:586
Each plugin activation creates an independent scope via Scope.make("sequential") (line 396) and stores it in the runtime registry, but the PluginHost service never ties those scopes to its own lifetime. When the PluginHost layer is shut down, none of the active plugin scopes are closed, so plugin service fibers keep running and scope finalizers — such as terminalsBundle.shutdown and httpRegistry.remove(pluginId) — never execute. This leaks plugin-owned processes/PTYs and leaves plugin runtime state alive past server shutdown. Consider registering a finalizer on the host's own scope that closes every active plugin scope (e.g. by iterating registry.list and calling Scope.close on each entry's scope).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginHost.ts around line 586:
Each plugin activation creates an independent scope via `Scope.make("sequential")` (line 396) and stores it in the runtime registry, but the `PluginHost` service never ties those scopes to its own lifetime. When the `PluginHost` layer is shut down, none of the active plugin scopes are closed, so plugin service fibers keep running and scope finalizers — such as `terminalsBundle.shutdown` and `httpRegistry.remove(pluginId)` — never execute. This leaks plugin-owned processes/PTYs and leaves plugin runtime state alive past server shutdown. Consider registering a finalizer on the host's own scope that closes every active plugin scope (e.g. by iterating `registry.list` and calling `Scope.close` on each entry's scope).
| } | ||
| 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 staleness check on line 246 uses only the advisory lock file's mtime, but the lock owner never refreshes that timestamp while holding the lock. A legitimate mutation that runs longer than STALE_LOCK_MS (60s) appears stale to another process, which reclaims the lock and becomes a concurrent writer. Two writers then race and overwrite each other's plugin lockfile updates. The pre-write token re-check mitigates the original holder, but the reclaimer still writes without knowing its acquisition was based on a false stale verdict. Consider periodically touching the lock file's mtime while the mutation is in progress (a heartbeat) so live ownership is distinguishable from a truly stale lock.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/PluginLockfileStore.ts around line 246:
The staleness check on line 246 uses only the advisory lock file's `mtime`, but the lock owner never refreshes that timestamp while holding the lock. A legitimate mutation that runs longer than `STALE_LOCK_MS` (60s) appears stale to another process, which reclaims the lock and becomes a concurrent writer. Two writers then race and overwrite each other's plugin lockfile updates. The pre-write token re-check mitigates the original holder, but the reclaimer still writes without knowing its acquisition was based on a false stale verdict. Consider periodically touching the lock file's `mtime` while the mutation is in progress (a heartbeat) so live ownership is distinguishable from a truly stale lock.
| const RelativeEntryPath = TrimmedNonEmptyString.check( | ||
| Schema.makeFilter<string>((entryPath) => { | ||
| if (entryPath.startsWith("/") || entryPath.startsWith("\\")) { | ||
| return "entry paths must be relative"; | ||
| } |
There was a problem hiding this comment.
🟡 Medium src/plugin.ts:36
RelativeEntryPath only rejects paths starting with / or \\, so Windows absolute paths like C:\\plugin\\server.js pass validation even though they are not relative. If the runtime resolves such a path as an entry file, it escapes the plugin directory, defeating the path-safety check. Consider also rejecting paths that match a Windows drive-letter pattern (e.g. /^[a-zA-Z]:[\\/]/).
const RelativeEntryPath = TrimmedNonEmptyString.check(
Schema.makeFilter<string>((entryPath) => {
- if (entryPath.startsWith("/") || entryPath.startsWith("\\\\")) {
+ if (/^[a-zA-Z]:[\\\\/]/.test(entryPath) || entryPath.startsWith("/") || entryPath.startsWith("\\\\")) {
return "entry paths must be relative";
}🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/contracts/src/plugin.ts around lines 36-40:
`RelativeEntryPath` only rejects paths starting with `/` or `\\`, so Windows absolute paths like `C:\\plugin\\server.js` pass validation even though they are not relative. If the runtime resolves such a path as an entry file, it escapes the plugin directory, defeating the path-safety check. Consider also rejecting paths that match a Windows drive-letter pattern (e.g. `/^[a-zA-Z]:[\\/]/`).
| ...(turnBootstrap !== undefined ? { bootstrap: turnBootstrap as any } : {}), | ||
| createdAt: nowIso(), | ||
| }) | ||
| .pipe( |
There was a problem hiding this comment.
🟡 Medium capabilities/AgentsCapability.ts:357
startTurn adds every generated turnId to the turnAliases map, but entries are only pruned inside readTerminalTurn (called via awaitTurn) and on the rollback path for newly created threads. When a plugin starts turns and tracks completion through observeThread or the returned messageId instead of calling awaitTurn, or when thread.turn.start fails on an existing thread, those alias entries are never deleted. A long-lived plugin process therefore accumulates stale entries in turnAliases without bound, eventually exhausting memory. Consider pruning the alias on the Effect.tapError failure path for existing threads as well, or adopting a bounded/evicting strategy so entries cannot grow unbounded.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 357:
`startTurn` adds every generated `turnId` to the `turnAliases` map, but entries are only pruned inside `readTerminalTurn` (called via `awaitTurn`) and on the rollback path for newly created threads. When a plugin starts turns and tracks completion through `observeThread` or the returned `messageId` instead of calling `awaitTurn`, or when `thread.turn.start` fails on an existing thread, those alias entries are never deleted. A long-lived plugin process therefore accumulates stale entries in `turnAliases` without bound, eventually exhausting memory. Consider pruning the alias on the `Effect.tapError` failure path for existing threads as well, or adopting a bounded/evicting strategy so entries cannot grow unbounded.
| ), | ||
| ), | ||
|
|
||
| switchRef: (request) => |
There was a problem hiding this comment.
🟡 Medium capabilities/VcsCapability.ts:170
switchRef passes request.ref directly to git switch / git checkout without --end-of-options, so a ref name beginning with - (e.g. --detach) is interpreted as a flag by Git and alters repository state in an unintended way instead of switching to the requested branch. Consider passing --end-of-options before the ref name so option-looking values are treated as refs.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/VcsCapability.ts around line 170:
`switchRef` passes `request.ref` directly to `git switch` / `git checkout` without `--end-of-options`, so a ref name beginning with `-` (e.g. `--detach`) is interpreted as a flag by Git and alters repository state in an unintended way instead of switching to the requested branch. Consider passing `--end-of-options` before the ref name so option-looking values are treated as refs.
| const holdsStandardClientMarker = (granted: ReadonlyArray<AuthScope>): boolean => | ||
| AuthStandardClientMarkerScopes.every((markerScope) => granted.includes(markerScope)); | ||
|
|
||
| export function satisfiesScope(required: AuthScope, granted: ReadonlyArray<AuthScope>): boolean { |
There was a problem hiding this comment.
🟠 High src/auth.ts:154
satisfiesScope implicitly grants every PluginScope and plugins:manage to any token that happens to contain the five AuthStandardClientMarkerScopes, not just legacy persisted sessions. Callers can request custom scope subsets (e.g. via AuthCreatePairingCredentialInput.scopes or token exchange), so a freshly issued downscoped token carrying exactly those five scopes is silently escalated to full plugin read/operate and plugin-management permissions. Consider gating the marker-based implicit grant on a durable flag recorded at session creation rather than inferring full-trust status from the scope set alone, or document why any holder of these five scopes is trusted as a full standard client.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/contracts/src/auth.ts around line 154:
`satisfiesScope` implicitly grants every `PluginScope` and `plugins:manage` to any token that happens to contain the five `AuthStandardClientMarkerScopes`, not just legacy persisted sessions. Callers can request custom scope subsets (e.g. via `AuthCreatePairingCredentialInput.scopes` or token exchange), so a freshly issued downscoped token carrying exactly those five scopes is silently escalated to full plugin read/operate and plugin-management permissions. Consider gating the marker-based implicit grant on a durable flag recorded at session creation rather than inferring full-trust status from the scope set alone, or document why any holder of these five scopes is trusted as a full standard client.
| ); | ||
| } | ||
|
|
||
| function toTimeoutDuration(input: string | number | undefined): Duration.Duration { |
There was a problem hiding this comment.
🟡 Medium capabilities/AgentsCapability.ts:90
toTimeoutDuration calls Duration.fromInputUnsafe on caller-supplied timeout strings, and for an invalid value like "abc" this throws a synchronous exception. Because awaitTurn invokes toTimeoutDuration inside Effect.gen, the throw becomes an unexpected defect that crashes the request instead of surfacing a normal typed error. Consider validating the input and returning a typed Effect.fail for invalid timeout values.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/plugins/capabilities/AgentsCapability.ts around line 90:
`toTimeoutDuration` calls `Duration.fromInputUnsafe` on caller-supplied timeout strings, and for an invalid value like `"abc"` this throws a synchronous exception. Because `awaitTurn` invokes `toTimeoutDuration` inside `Effect.gen`, the throw becomes an unexpected defect that crashes the request instead of surfacing a normal typed error. Consider validating the input and returning a typed `Effect.fail` for invalid timeout values.
| // Plugin method RPCs have this static environment-read baseline; the dispatcher | ||
| // performs the real per-plugin plugin:<id>:read|operate authorization. | ||
| [PLUGINS_WS_METHODS.call, AuthOrchestrationReadScope], | ||
| [PLUGINS_WS_METHODS.subscribe, AuthOrchestrationReadScope], |
There was a problem hiding this comment.
🟠 High src/ws.ts:351
PLUGINS_WS_METHODS.call and PLUGINS_WS_METHODS.subscribe are gated by AuthOrchestrationReadScope in RPC_REQUIRED_SCOPE, so a downscoped token carrying only plugin:<id>:read or plugin:<id>:operate is rejected with EnvironmentAuthorizationError before the PluginRpcDispatcher can perform the per-plugin authorization. This breaks WebSocket RPC access for least-privilege plugin tokens, which must additionally carry unrelated orchestration:read to pass the transport-level check. Consider omitting these two methods from RPC_REQUIRED_SCOPE (or using a dedicated plugin:read baseline scope) so the dispatcher alone controls plugin method authorization.
- // Plugin method RPCs have this static environment-read baseline; the dispatcher
- // performs the real per-plugin plugin:<id>:read|operate authorization.
- [PLUGINS_WS_METHODS.call, AuthOrchestrationReadScope],
- [PLUGINS_WS_METHODS.subscribe, AuthOrchestrationReadScope],
+ // Plugin method RPCs are authorized per-plugin by the dispatcher using
+ // plugin:<id>:read|operate scopes, so they are excluded from the static
+ // environment-level scope check here.🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/ws.ts around lines 351-354:
`PLUGINS_WS_METHODS.call` and `PLUGINS_WS_METHODS.subscribe` are gated by `AuthOrchestrationReadScope` in `RPC_REQUIRED_SCOPE`, so a downscoped token carrying only `plugin:<id>:read` or `plugin:<id>:operate` is rejected with `EnvironmentAuthorizationError` before the `PluginRpcDispatcher` can perform the per-plugin authorization. This breaks WebSocket RPC access for least-privilege plugin tokens, which must additionally carry unrelated `orchestration:read` to pass the transport-level check. Consider omitting these two methods from `RPC_REQUIRED_SCOPE` (or using a dedicated `plugin:read` baseline scope) so the dispatcher alone controls plugin method authorization.
1 participant
What Changed
The two orchestration-integrated capabilities.
agentsis built entirely on the orchestration engine (dispatch +streamDomainEvents+ projections — never raw provider access); the plugin's owner is injected server-side and every thread-scoped op verifies ownership.vcsis a curated worktree / branch / merge (conflict-as-value) / diff / checkpoint façade.Why
Part 5 of 8 of a runtime plugin system (see #3725 for the overview). Builds on #3728. These let a plugin drive real agent turns and git operations through the same engine the app uses, so plugin turns are attributable and indistinguishable in the event log.
Review — four-model tribunal (GPT-5.5 · Grok · GLM-5.2 · Fable)
Fable verified the ownership guard, owner-injection,
observeThreaddedup, and theawaitTurnrace are correct.turnAliasescorrelation (two MUSTs): prune-on-terminal-read broke re-await; the FIFO cap could evict a still-pending turn → coherent lifecycle redesign (terminalflag, no delete-on-read, evict-oldest-terminal-first), cap still bounded.--output=→ arbitrary write) →--end-of-options+ validation; sub-paths/filePaths→ containment; roots scoped to the granted workspace; merge-abort no longer masks the conflict result.toTimeoutDurationtyped-fail;listPendingRequestsfilters resolved requests; inert bootstrap-prep now rejects loudly.UI Changes
N/A — server only.
Checklist
https://claude.ai/code/session_011H86UHL1RPuWBX3LBTUPsk