feat(plugins): custom hooks system with event-driven registry - [deferred to v4.0.0-rc1] #2387
feat(plugins): custom hooks system with event-driven registry - [deferred to v4.0.0-rc1] #2387oyi77 wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive plugin system featuring lifecycle management, a sandboxed execution environment using the Node.js vm module, and an event-driven hook registry integrated into the chat core. Feedback identifies several critical issues, including a potential path traversal vulnerability during plugin installation, a violation of repository security rules regarding arbitrary code execution, and an argument mismatch in hook handlers. Further recommendations suggest optimizing performance by moving dynamic imports out of hot paths, enhancing robustness in database and JSON operations, and ensuring the request pipeline remains responsive by handling slow plugin hooks with timeouts or concurrency.
| inputSchema: z.object({ | ||
| path: z.string().describe("Absolute path to the plugin directory containing plugin.json"), | ||
| }), | ||
| handler: async (args: { path: string }) => { |
There was a problem hiding this comment.
The plugin_install tool accepts an arbitrary absolute path, which allows for potential arbitrary file system access. Installation should be restricted to a specific staging directory or use a more secure transfer mechanism to prevent path traversal vulnerabilities.
| const wrapped = `(async function(module, exports, require) { ${source} })(module, exports, require);`; | ||
| vm.runInContext(wrapped, context, { | ||
| filename: entryPoint, | ||
| timeout: 10000, // 10s init timeout | ||
| }); |
There was a problem hiding this comment.
The use of vm.runInContext violates Repository Rule 2.3 (Hard Rules), which prohibits the use of eval(), new Function(), or any implied eval. While vm is a dedicated module, it executes arbitrary code strings and is explicitly documented by Node.js as not being a security mechanism for running untrusted code.
References
- Repository Rule 2.3: Never use eval(), new Function(), or any implied eval. (link)
| plugin.onResponse = async (ctx: PluginContext, response: any): Promise<any | void> => { | ||
| try { | ||
| return await pluginExports.onResponse(ctx, response); | ||
| } catch (err: any) { | ||
| log.error("plugin.onResponse_error", { name: manifest.name, error: err.message }); | ||
| } | ||
| }; | ||
| hooks.push("onResponse"); | ||
| } |
There was a problem hiding this comment.
The onResponse and onError handlers are defined to accept two arguments (ctx and response/error), but the emitHook function in hooks.ts only passes a single payload argument. This will result in the second argument being undefined when these hooks are triggered via the event system.
|
|
||
| // ── Plugin onRequest hook ── | ||
| try { | ||
| const { runOnRequest } = await import("@/lib/plugins/index"); |
There was a problem hiding this comment.
Performing dynamic imports inside the handleChatCore function, which is a hot path for every request, introduces unnecessary latency and overhead. These imports should be moved to the top level or handled via a cached singleton to ensure optimal performance.
| ); | ||
|
|
||
| log.info("plugin.inserted", { id: input.id, name: input.name }); | ||
| return getPluginByName(input.name)!; |
There was a problem hiding this comment.
The use of the non-null assertion operator (!) here is risky. If the insertion succeeded but the subsequent retrieval fails (e.g., due to a race condition or database issue), this will throw a runtime error. It is safer to handle the potential null case explicitly.
| for (const reg of list) { | ||
| try { | ||
| await reg.handler(payload); | ||
| } catch (err: unknown) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| log.error("hook.handler_error", { | ||
| event, | ||
| pluginName: reg.pluginName, | ||
| error: message, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Hook handlers are executed sequentially and awaited. A single slow or hanging plugin handler will block the entire request pipeline. Consider using a timeout for each handler or executing them concurrently where appropriate (e.g., using Promise.allSettled).
| if (!row) throw new Error(`Plugin '${name}' not found`); | ||
| if (row.status === "active") return; | ||
|
|
||
| const manifest = JSON.parse(row.manifest) as PluginManifestWithDefaults; |
There was a problem hiding this comment.
Directly calling JSON.parse on the manifest string from the database without error handling could lead to unhandled exceptions if the data is corrupted. Use a safe parsing utility or wrap in a try-catch block to maintain stability.
diegosouzapw
changed the title
2 participants
Summary
Adds custom hooks system for plugins (Wave 5 of #2060).
Depends on: #2385 (plugin backend core)
What's included:
src/lib/plugins/hooks.ts— event-driven hook registry with 10 built-in eventsonModelSelecthook wired in chatCore.ts after model resolutiononProviderErrorhook wired in chatCore.ts error handlerBuilt-in events: onRequest, onResponse, onError, onModelSelect, onComboResolve, onRateLimit, onQuotaExhaust, onProviderError, onStreamStart, onStreamEnd
Test plan