fix: Fix plugin loading from a character.json file #2095
Conversation
odilitime
changed the title
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces two new dependencies for video generation and web search plugins in the Changes
Sequence DiagramsequenceDiagram
participant Agent
participant PluginImporter
participant Plugins
Agent->>PluginImporter: Call handlePluginImporting
PluginImporter->>Plugins: Attempt to import plugins
alt Import Successful
Plugins-->>PluginImporter: Return imported plugins
else Import Failed
PluginImporter-->>Agent: Return empty array
PluginImporter->>Agent: Log import errors
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
agent/package.json(1 hunks)agent/src/index.ts(3 hunks)
🔇 Additional comments (4)
agent/src/index.ts (3)
202-204: LGTM! Clean separation of plugin importing logic.The delegation of plugin importing to a dedicated function improves code organization and maintainability.
790-793: LGTM! Consistent plugin loading implementation.The integration of handlePluginImporting in startAgent maintains consistency with loadCharacters.
227-227: Add test coverage for handlePluginImporting.This new function handles critical plugin loading functionality but lacks test coverage. Consider adding tests for:
- Successful plugin loading
- Failed plugin imports
- Empty plugin array handling
- Invalid plugin format handling
Would you like me to help generate test cases for this functionality?
agent/package.json (1)
62-63: LGTM! Consistent dependency additions.The new plugin dependencies follow the existing workspace package pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
agent/src/index.ts (1)
230-258: 🛠️ Refactor suggestionConsider additional improvements to plugin importing.
While the previous review comment covers essential improvements to error handling and type safety, there are additional concerns:
- The function returns an empty array
[]for failed imports at line 250, which could lead to silent failures in the agent's functionality.- There's no validation that the imported plugins conform to the expected plugin interface.
Additionally, consider adding plugin validation:
async function handlePluginImporting(plugins: string[]) { + interface ElizaPlugin { + // Add minimum required plugin interface + name?: string; + initialize?: () => Promise<void>; + } + + function isValidPlugin(plugin: any): plugin is ElizaPlugin { + return typeof plugin === 'function' || + (typeof plugin === 'object' && plugin !== null); + } + if (plugins.length > 0) { elizaLogger.info("Plugins are: ", plugins); const importedPlugins = await Promise.all( plugins.map(async (plugin) => { try { const importedPlugin = await import(plugin); const functionName = plugin .replace("@elizaos/plugin-", "") .replace(/-./g, (x) => x[1].toUpperCase()) + "Plugin"; - return ( - importedPlugin.default || importedPlugin[functionName] - ); + const pluginFunction = importedPlugin.default || importedPlugin[functionName]; + if (!isValidPlugin(pluginFunction)) { + throw new Error(`Invalid plugin format: ${plugin}`); + } + return pluginFunction; } catch (importError) { elizaLogger.error( `Failed to import plugin: ${plugin}`, importError ); - return []; // Return null for failed imports + throw importError; // Propagate error to caller } }) - ); + ).catch(error => { + elizaLogger.error('Plugin loading failed:', error); + return []; // Return empty array only at top level + }); return importedPlugins; } else { return []; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
agent/package.json(1 hunks)agent/src/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- agent/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (1)
agent/src/index.ts (1)
799-802: 🛠️ Refactor suggestionAdd error handling for plugin importing in startAgent.
Similar to the
loadCharactersfunction, plugin importing should be wrapped in a try-catch block.directClient.startAgent = async (character) => { - // Handle plugins - character.plugins = await handlePluginImporting(character.plugins); + try { + character.plugins = await handlePluginImporting(character.plugins); + } catch (error) { + elizaLogger.error(`Failed to load plugins for character ${character.name}:`, error); + // Consider whether to throw or continue with empty plugins array + character.plugins = []; + } // wrap it so we don't have to inject directClient later return startAgent(character, directClient); };Likely invalid or redundant comment.
|
Hey @odilitime, quite curious, I'm running into this error when loading the image gen plugin into json. This is what I've got in character JSON { C:\MyApps\Eliza\eliza\node_modules\better-sqlite3\lib\methods\wrappers.js:5 TypeError: The database connection is not open Node.js v23.5.0 I'm obviously using together and I noticed that this fix was supposed to clear this. |
|
I also get the same error: |
Relates to
No specific ticket or issue linked.
Risks
Medium:
Background
What does this PR do?
This PR resolves issues and improves functionality for loading plugins in the following ways:
1. Ensures plugins in the plugin array in the character.json file are loaded, regardless if the API key exists in the createAgent function and/or if the plugin is listed in the createAgent list.
2. Adds support for loading plugins via POST settings.
3. Fixes a bug where plugins without a default export for their main function (e.g., video gen, image gen, and web search plugins) fail to load. The fix assumes all plugins are named using the camelCase version of the import name with “Plugin” as a suffix.
4. Load some missing plugins into the package.json
What kind of change is this?
Documentation changes needed?
Yes:
Testing
Where should a reviewer start?
Detailed testing steps
Discord username
treppers
Summary by CodeRabbit