-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add flexible deployment modes for different environments #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis PR introduces a configurable plugin factory pattern enabling multiple deployment variants. Three preset configurations are added (Full, Headless, Core) with toggleable features for UI, routes, actions, and tests. Documentation is expanded to explain installation modes and usage across deployment scenarios. Changes
Sequence DiagramsequenceDiagram
participant User
participant Factory as createKnowledgePlugin()
participant Plugin
participant Features as Conditional Features
User->>Factory: Call with config (enableUI, enableRoutes, etc.)
Factory->>Plugin: Create base Plugin instance
alt enableUI = true
Factory->>Features: Attach UI routes
Features->>Plugin: Add routes
end
alt enableActions = true
Factory->>Features: Attach actions
Features->>Plugin: Add actions
end
alt enableTests = true
Factory->>Features: Attach test suite
Features->>Plugin: Add tests
end
Factory->>User: Return configured Plugin
Note over User,Plugin: Presets created via factory:<br/>- knowledgePluginCore (all features off)<br/>- knowledgePluginHeadless (actions on)<br/>- knowledgePlugin (all features on)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The primary complexity lies in Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
Bug: README UUID Example ErrorThe Additional Locations (1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (4)
README.md (3)
72-83: Clarify plugin registration: string vs object.Explicitly note both supported forms to avoid confusion:
- String form (auto‑loader): '@elizaos/plugin-knowledge'
- Object form (direct import): knowledgePlugin
Add a short note like: “You can register by package name (string) or by importing the plugin object. Both are supported.”
146-151: Tighten wording about API keys/providers.“ No environment variables or API keys needed.” can mislead—an AI provider plugin (OpenAI/Anthropic/Google/OpenRouter) and its keys are required for embeddings.
Suggest: “No additional keys beyond your existing AI provider plugin. Ensure at least one AI provider plugin is configured with a valid API key.”
254-286: Programmatic example: avoid'' as UUIDcasts; show omit/auto‑gen.Use an optional field or explicit
undefinedinstead of a cast.-const result = await knowledgeService.addKnowledge({ - agentId: runtime.agentId, - clientDocumentId: '' as UUID, // Auto-generated based on content +const result = await knowledgeService.addKnowledge({ + agentId: runtime.agentId, + // Omit to auto-generate: + // clientDocumentId: undefined, content: documentContent, contentType: 'application/pdf', originalFilename: 'document.pdf', worldId: runtime.agentId, roomId: runtime.agentId, entityId: runtime.agentId,Optionally add an import hint for types if needed:
import type { UUID } from '@elizaos/core';.src/index.ts (1)
67-78: Eager imports prevent tree-shaking; optional refactors recommended for bundle optimization.Routes, actions, and tests are imported statically at module load (lines 10–12), so bundlers include them regardless of configuration. Even with
knowledgePluginCore, these modules ship in the bundle. Conditional checks at runtime (lines 80, 88, 93) don't prevent their inclusion.Consider one of these refactors when optimizing bundle size:
- Lazy import inside the factory (requires
async createKnowledgePluginand dynamicimport('./routes'),import('./actions'),import('./tests')).- Separate build entrypoints (core/headless/full) with
"sideEffects": falsein package.json for tree‑shaking.- Move tests behind a separate export to exclude them from default variants.
Also applies to: 79–86, 92–96, 100–121, 122–132, 146–147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
README.md(3 hunks)__tests__/plugin-exports.test.ts(1 hunks)package.json(1 hunks)src/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
package.json (2)
49-65: Add Vite to devDependencies; proposed patch is redundant.Vite is genuinely missing and will cause build failures. However, the proposed patch is incorrect:
- tsup.config.ts already has
clean: falseanddts: true, so the--dts --no-cleanflags in the patch are redundant.- vite.config.ts already has
emptyOutDir: false, so the separate outDir suggestion is unnecessary.- Both tools are configured to coexist in the same
distdirectory without clobbering each other.Required fix:
"devDependencies": { + "vite": "^5.4.10",No build script changes are needed; the existing config files already handle the output correctly.
Likely an incorrect or invalid review comment.
26-28: Add"sideEffects": falseto enable tree-shaking, but do not move react/react-dom to peerDependencies.The core library code (src/*.ts) has no React imports—React is only used in src/frontend/ which is not bundled. Consumers using
enableUI: falsealready get a headless plugin. Moving react to peerDependencies would break existing consumers (including the default case where enableUI=true) and introduce a breaking change. The current architecture already supports headless usage;sideEffects: falsealone is sufficient to let bundlers optimize.__tests__/plugin-exports.test.ts (1)
1-1: Bun is available in CI, but tests are not run before publishing.The CI workflow explicitly sets up Bun (
oven-sh/setup-bun@v2) and uses it for installation and building. However, no test step exists in the workflow before the package is published to npm. Tests in__tests__/import frombun:test, but CI never invokes them. Either:
- Add a test step before publishing (e.g.,
run: bun testorrun: elizaos test), or- Confirm that tests are validated elsewhere (e.g., in a separate PR workflow) before merging to 1.x
| import { | ||
| knowledgePlugin, | ||
| knowledgePluginCore, | ||
| knowledgePluginHeadless, | ||
| createKnowledgePlugin, | ||
| KnowledgeService, | ||
| knowledgeProvider, | ||
| type KnowledgePluginConfig, | ||
| } from '../src/index'; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused type import and add a default export parity test.
- Drop the unused
KnowledgePluginConfigtype import. - Add a quick check that the default export equals the named
knowledgePlugin.
-import {
- knowledgePlugin,
- knowledgePluginCore,
- knowledgePluginHeadless,
- createKnowledgePlugin,
- KnowledgeService,
- knowledgeProvider,
- type KnowledgePluginConfig,
-} from '../src/index';
+import knowledgeDefault, {
+ knowledgePlugin,
+ knowledgePluginCore,
+ knowledgePluginHeadless,
+ createKnowledgePlugin,
+ KnowledgeService,
+ knowledgeProvider,
+} from '../src/index';
@@
describe('Knowledge Plugin Exports', () => {
+ it('default export equals named knowledgePlugin', () => {
+ expect(knowledgeDefault).toBe(knowledgePlugin);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { | |
| knowledgePlugin, | |
| knowledgePluginCore, | |
| knowledgePluginHeadless, | |
| createKnowledgePlugin, | |
| KnowledgeService, | |
| knowledgeProvider, | |
| type KnowledgePluginConfig, | |
| } from '../src/index'; | |
| import knowledgeDefault, { | |
| knowledgePlugin, | |
| knowledgePluginCore, | |
| knowledgePluginHeadless, | |
| createKnowledgePlugin, | |
| KnowledgeService, | |
| knowledgeProvider, | |
| } from '../src/index'; | |
| describe('Knowledge Plugin Exports', () => { | |
| it('default export equals named knowledgePlugin', () => { | |
| expect(knowledgeDefault).toBe(knowledgePlugin); | |
| }); |
🤖 Prompt for AI Agents
In __tests__/plugin-exports.test.ts around lines 2 to 11, remove the unused
named type import "KnowledgePluginConfig" from the import list and add a small
parity test that asserts the module's default export is equal to the named
export "knowledgePlugin"; update imports accordingly (drop the type) and append
a test (e.g., expect(require('../src').default).toBe(knowledgePlugin) or import
default and compare) to validate default export equality.
| * @example | ||
| * // Browser-only mode (no routes) | ||
| * const plugin = createKnowledgePlugin({ | ||
| * enableRoutes: false, | ||
| * }); | ||
| * | ||
| * @example | ||
| * // Full mode (default) | ||
| * const plugin = createKnowledgePlugin(); | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the “Browser‑only mode (no routes)” example.
With defaults, enableUI is true, so routes will still be attached due to (enableUI || enableRoutes). Update the example label and options:
- * @example
- * // Browser-only mode (no routes)
- * const plugin = createKnowledgePlugin({
- * enableRoutes: false,
- * });
+ * @example
+ * // Headless (no UI, no routes)
+ * const plugin = createKnowledgePlugin({
+ * enableUI: false,
+ * enableRoutes: false,
+ * });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @example | |
| * // Browser-only mode (no routes) | |
| * const plugin = createKnowledgePlugin({ | |
| * enableRoutes: false, | |
| * }); | |
| * | |
| * @example | |
| * // Full mode (default) | |
| * const plugin = createKnowledgePlugin(); | |
| */ | |
| * @example | |
| * // Headless (no UI, no routes) | |
| * const plugin = createKnowledgePlugin({ | |
| * enableUI: false, | |
| * enableRoutes: false, | |
| * }); | |
| * | |
| * @example | |
| * // Full mode (default) | |
| * const plugin = createKnowledgePlugin(); | |
| */ |
🤖 Prompt for AI Agents
In src/index.ts around lines 57 to 66, the example labeled "Browser-only mode
(no routes)" is incorrect because enableUI defaults to true and routes are
attached when (enableUI || enableRoutes); update the example label and options
so both UI and routes are disabled — e.g., change the comment to "Browser-only
mode (no UI, no routes)" and pass { enableUI: false, enableRoutes: false } in
the createKnowledgePlugin call.
Note
Introduce a configurable plugin factory with core and headless modes, update docs and programmatic usage, add export tests, and bump version.
createKnowledgePlugin(config)with flags:enableUI,enableRoutes,enableActions,enableTests.knowledgePlugin(full),knowledgePluginHeadless,knowledgePluginCore.knowledgePlugin; explicitly exportKnowledgeServiceandknowledgeProvider.runtime.getService,getKnowledgeusage, UUID handling, agentId-scoped IDs) and add cloud runtime example.__tests__/plugin-exports.test.tsvalidating exports and configuration behaviors.1.5.12.Written by Cursor Bugbot for commit fee53ba. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores