refactor(plugins): give the plugin capability surface its own module - #1027
Merged
Conversation
This is the security-review surface: the verbs a plugin can actually reach —
ctx.messages, ctx.engine, ctx.net, ctx.storage, ctx.conversations, ctx.handover,
ctx.mappings — and the gates in front of every one of them. A plugin supplies
its own sessionId, so assertSessionAllowed (the static manifest boundary) and
the operator-activation check ARE the boundary, not a convenience.
It sat inside a loader that is otherwise about directory discovery, lifecycle
and worker plumbing, so reviewing the boundary meant reading 1640 lines to find
330. PluginCapabilityContext now holds the seven gate helpers, createPluginContext
and the hook AsyncLocalStorage. It holds no plugin registry and no worker hosts —
everything it needs comes from the PluginInstance it is handed.
The constructor SIGNATURE is untouched: the facade is built in the body, as
PluginHostServices already is. All 33 positional spec constructions and the three
subclasses overriding createSandboxHost compile and pass unchanged.
Two things this surfaced that are worth recording:
The loader's logger is passed in rather than a second one created. A plugin's
ctx.logger lines carry that logger's context, which is operator-visible; a fresh
createLogger('PluginCapabilityContext') silently retagged every plugin log line,
and the sandbox log-relay tests caught it.
dispatchConversationMedia moved too. Leaving it behind would have made the new
module import the loader while the loader imports it — a require cycle, in a file
that already pays a lazy-require tax to avoid exactly that. The loader no longer
references it.
1640 to 1267 lines. Spec changes are pointer-only across four files: three
reach-in helpers now name the object that owns the surface, and one import line
follows dispatchConversationMedia. No assertion, fixture or harness was touched;
307 plugin tests and 4012 overall pass unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third step on
plugin-loader.service.ts, and the one with the most value: it removes thesecurity-review audience.
Why
createPluginContextand its seven gate helpers are the surface a plugin can actually reach —ctx.messages,ctx.engine,ctx.net,ctx.storage,ctx.conversations,ctx.handover,ctx.mappings— plus the gates in front of each. A plugin supplies its ownsessionId, soassertSessionAllowed(the static manifest boundary) and the operator-activation check are thesecurity boundary.
They sat inside a loader otherwise concerned with directory discovery, lifecycle and worker plumbing.
Reviewing the boundary meant reading 1640 lines to find 330.
What moves
PluginCapabilityContextholds the seven gate helpers,createPluginContext, and the hookAsyncLocalStorage. It holds no plugin registry and no worker hosts — I verified the movedrange references neither
this.pluginsnorthis.sandboxHosts. Everything it needs comes from thePluginInstanceit is handed.The constructor signature is untouched — the facade is built in the body, as
PluginHostServicesalready is. All 33 positional spec constructions and the three subclasses overriding
createSandboxHostcompile and pass unchanged.1640 → 1267 lines.
Two things this surfaced
The loader's logger is passed in, not recreated. A plugin's
ctx.loggerlines carry that logger'scontext, and it is operator-visible. A fresh
createLogger('PluginCapabilityContext')silentlyretagged every plugin log line — the sandbox log-relay tests caught it, which is exactly what they are
for.
dispatchConversationMediamoved too. Leaving it in the loader would have made the new moduleimport the loader while the loader imports the new module — a require cycle, in a file that already
pays a lazy-require tax to avoid precisely that class of problem. The loader no longer references it.
Spec changes: pointer-only
Four files, +29/−18, and no assertion, fixture or harness touched:
plugin-capability,plugin-loader-conversation-repair,plugin-loader-activation) now name the object that owns the surface;dispatchConversationMedia.The plan this came from predicted zero spec edits. That was wrong for the same reason it was wrong
twice before this week: these specs reach into private members, so a member that moves takes its
reach-in with it. Recorded rather than glossed.
307 plugin tests pass; 4012 overall, unchanged.
Verification
Backend lint,
prettier --check,tsc --noEmitover the full project including specs, build, 4012 unittests, 137 e2e tests, and the dashboard build all pass on Node 22.