-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
feat: introduce Dependency Injection to enhance developer experience #2115
Conversation
- Created a new plugin package `@elizaos/plugin-di` to extend the Eliza platform. - Added essential files including `package.json`, `README.md`, and TypeScript configuration. - Implemented core functionalities such as dependency injection using Inversify, action handling, and content evaluation. - Introduced decorators for content properties and Zod schema integration. - Included sample actions and evaluators to demonstrate plugin capabilities. - Configured build and linting scripts for development workflow.
- Introduced a new TypeScript configuration file `tsconfig.build.json` for build-specific settings. - Updated `tsconfig.json` to enable experimental decorators and metadata emission. - Added unit tests for content decorators, validating schema creation and property description loading. - Implemented tests for the `property` decorator to ensure metadata storage functionality.
- Removed export of providers from index.ts. - Added a new file `baseInjectableAction.ts` that defines the `BaseInjactableAction` class, which serves as an abstract base for creating injectable actions. - Introduced action options and methods for processing messages, validating content, and handling actions within the Eliza framework. - Updated `index.ts` in actions directory to export the new base class.
- Fixed a typo in the export statement from `evaluators` to `evalutors` in `index.ts`. - Added a new file `baseInjectableEvalutor.ts` to define the base class for injectable evaluators. - Removed obsolete sample action and evaluator files to streamline the codebase. - Updated `types.ts` to include the `ActionOptions` type definition for better action handling.
- Fixed export typo in `index.ts` from `evalutors` to `evaluators`. - Added `EvaluatorOptions` type definition in `types.ts` for better evaluator configuration. - Introduced new sample action and evaluator files to demonstrate usage within the plugin. - Implemented a base abstract class for injectable evaluators in `baseInjactableEvaluator.ts`. - Updated `createPlugin` function to handle actions more effectively. - Removed obsolete `index.ts` from the `evalutors` directory to clean up the codebase.
…actor structure - Modified the `execute` method in `BaseInjactableAction` and `CreateResourceAction` to accept nullable content. - Introduced a new `CreateResourceContent` class to define the structure of the content for resource creation. - Refactored the `CreateResourceAction` to utilize the new content class and improved validation logic. - Enhanced the callback responses to handle cases where no content is provided, ensuring better error handling.
…and plugin integration - Introduced `CreateResourceAction` to manage resource creation with dependency injection. - Added `SampleEvaluator` for evaluating important content in memory, implementing the `BaseInjactableEvaluator`. - Created `SampleProvider` for data retrieval, adhering to the `InjectableProvider` interface. - Registered new components with the global container for dependency injection. - Defined a `samplePlugin` to encapsulate the new action, evaluator, and provider, facilitating resource management.
- Changed `CreateResourceAction` and `SampleEvaluator` to use `inRequestScope()` for better lifecycle management. - Introduced dynamic data binding in `SampleProvider` with `toDynamicValue()` for asynchronous data retrieval. - Updated `SampleProvider` to inject dynamic data and modified the `get` method to return a string representation of the shared and dynamic data. - Adjusted import statements in `baseInjectableAction.ts` and `baseInjactableEvaluator.ts` to use `import type` for improved type safety.
- Updated the normalizeCharacter function to use optional chaining for plugin properties, enhancing null safety. - Removed unnecessary return statements to streamline the code flow, ensuring that the function consistently returns the plugin object when valid.
- Updated the `createPlugin` function to improve error handling for providers, actions, and evaluators by logging errors and filtering out undefined values. - Implemented optional chaining in the `buildContentOutputTemplate` function to enhance null safety when accessing examples. - Added a new test suite for normalizing characters, ensuring the integrity of plugins and their properties.
- Included "inversify", "reflect-metadata", "zod", and "uuid" as external modules in the tsup configuration to enhance functionality and support for dependency injection and validation.
- Removed the ExtendedPlugin type definition from types.ts to simplify the type structure. - Updated the createPlugin function in plugin.ts to return a Plugin type instead of ExtendedPlugin, reflecting the changes in the type definitions.
@coderabbitai review |
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: 17
🧹 Nitpick comments (20)
packages/plugin-di/src/_examples/samplePlugin.ts (1)
1-3
: Consider using path aliases for importsReplace relative imports with path aliases to improve maintainability and prevent path traversal issues.
-import { CreateResourceAction } from "./sampleAction"; -import { SampleProvider } from "./sampleProvider"; -import { SampleEvaluator } from "./sampleEvalutor"; +import { CreateResourceAction } from "@plugins/sampleAction"; +import { SampleProvider } from "@plugins/sampleProvider"; +import { SampleEvaluator } from "@plugins/sampleEvalutor";packages/plugin-di/tsup.config.ts (2)
8-8
: Fix misleading comment about module formatThe comment suggests CommonJS but the configuration specifies ESM.
- format: ["esm"], // Ensure you're targeting CommonJS + format: ["esm"], // Using ES Modules format
3-23
: Consider additional build optimizationsThe configuration could benefit from additional build options.
export default defineConfig({ entry: ["src/index.ts"], outDir: "dist", sourcemap: true, clean: true, format: ["esm"], + dts: true, // Generate declaration files + minify: true, // Minify output + splitting: true, // Enable code splitting external: [packages/plugin-di/src/_examples/sampleProvider.ts (1)
27-43
: Add lifecycle management for shared instanceThe shared instance could lead to memory leaks without proper cleanup.
private _sharedInstance: Record<string, string>; +private _disposed = false; + +public dispose(): void { + this._disposed = true; + this._sharedInstance = undefined; +} async getInstance( _runtime: IAgentRuntime ): Promise<Record<string, string>> { + if (this._disposed) { + throw new Error('Provider has been disposed'); + } if (!this._sharedInstance) { this._sharedInstance = {}; } return this._sharedInstance; }packages/plugin-di/src/evaluators/baseInjactableEvaluator.ts (1)
44-51
: Consider strengthening the default validate implementationThe current default implementation always returns true, which might be too permissive. Consider adding basic validation checks or making it abstract.
- async validate( + abstract async validate( _runtime: IAgentRuntime, _message: Memory, _state?: State - ): Promise<boolean> { - // Default implementation is to return true - return true; - } + ): Promise<boolean>;packages/plugin-di/src/decorators/content.decorators.ts (1)
31-42
: Rename 'constructor' parameter to avoid shadowingThe parameter name 'constructor' shadows the global property. Consider using a more specific name.
-export function createZodSchema<T>(constructor: ContentClass<T>): z.ZodType<T> { +export function createZodSchema<T>(classConstructor: ContentClass<T>): z.ZodType<T> { const properties: Record<string, ContentPropertyConfig> = - Reflect.getMetadata(CONTENT_METADATA_KEY, constructor.prototype) || {}; + Reflect.getMetadata(CONTENT_METADATA_KEY, classConstructor.prototype) || {}; // Similar change in loadPropertyDescriptions -export function loadPropertyDescriptions<T>(constructor: ContentClass<T> +export function loadPropertyDescriptions<T>(classConstructor: ContentClass<T>Also applies to: 50-62
🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/plugin-di/src/tests/content.decorators.test.ts (2)
26-40
: Add more edge cases to schema validation testsThe current tests could be more comprehensive. Consider adding:
- Empty object test
- Null/undefined property tests
- Extra property tests
it("should handle edge cases", () => { const schema = createZodSchema(TestClass); expect(schema.safeParse({}).success).toBe(false); expect(schema.safeParse({ testProperty: null }).success).toBe(false); expect(schema.safeParse({ testProperty: "valid", extraProp: "invalid" }).success).toBe(true); // or false, depending on your requirements });
58-64
: Enhance property decorator testsThe current test only verifies initial undefined state. Consider testing:
- Property assignment
- Type validation at runtime
- Metadata persistence
packages/plugin-di/src/templates.ts (1)
15-26
: Consider using array join for better performance.Instead of string concatenation in a loop, consider using array push and join for better performance with large property sets.
- let propDesc = ""; - Object.entries(properties).forEach(([key, { description, examples }]) => { - propDesc += `- Field **"${key}"**: ${description}.`; - if (examples?.length > 0) { - propDesc += " Examples or Rules for this field:\n"; - } else { - propDesc += "\n"; - } - examples?.forEach((example, index) => { - propDesc += ` ${index + 1}. ${example}\n`; - }); - }); + const propDescLines = []; + Object.entries(properties).forEach(([key, { description, examples }]) => { + propDescLines.push(`- Field **"${key}"**: ${description}.`); + if (examples?.length > 0) { + propDescLines.push(" Examples or Rules for this field:"); + examples.forEach((example, index) => { + propDescLines.push(` ${index + 1}. ${example}`); + }); + } + }); + const propDesc = propDescLines.join("\n");packages/plugin-di/src/types.ts (2)
37-44
: Consider adding readonly modifiers.For immutable properties in ActionOptions, consider adding readonly modifiers to prevent accidental modifications.
export type ActionOptions<T> = Pick< Action, "name" | "similes" | "description" | "examples" | "suppressInitialMessage" > & { - contentClass: ContentClass<T>; - template?: string; - contentSchema?: z.ZodSchema<T>; + readonly contentClass: ContentClass<T>; + readonly template?: string; + readonly contentSchema?: z.ZodSchema<T>; };
115-118
: Add index signature for dynamic properties.Consider adding an index signature to ContentPropertyDescription to allow for additional metadata properties.
export interface ContentPropertyDescription { description: string; examples?: string[]; + [key: string]: unknown; }
packages/plugin-di/src/_examples/sampleAction.ts (2)
122-133
: Remove commented code.Either implement the memory persistence logic or remove the commented code. Consider creating a TODO issue if this is planned for future implementation.
101-107
: Improve validation logic.Consider:
- Making the API_KEY configurable
- Adding validation for required fields
async validate( runtime: IAgentRuntime, - _message: Memory, - _state?: State + message: Memory, + state?: State ): Promise<boolean> { - return !!runtime.character.settings.secrets?.API_KEY; + const apiKey = runtime.character.settings.secrets?.API_KEY; + if (!apiKey) { + elizaLogger.warn("Missing API_KEY in settings"); + return false; + } + return true; }packages/plugin-di/src/factories/plugin.ts (1)
25-32
: Enhance error handling with error context.The error handling could be improved by:
- Including more context in error messages
- Considering retry mechanisms for transient failures
- Adding error types/codes for better error handling upstream
Also applies to: 49-57, 75-82
packages/plugin-di/README.md (6)
11-11
: Consider using absolute GitHub URLs for examples.Replace the relative path with an absolute GitHub URL to ensure the link remains valid regardless of where the documentation is viewed.
-Check the [example](./src/_examples/) folder for a simple example of how to create a plugin using Dependency Injection. +Check the [example](https://github.com/organization/repo/tree/main/packages/plugin-di/src/_examples/) folder for a simple example of how to create a plugin using Dependency Injection.
66-68
: Fix grammar in the DI plugin introduction.-DI plugin provides abstract classes that you can extend to create Injectable actions or evaluators. +The DI plugin provides abstract classes that you can extend to create Injectable actions or evaluators.🧰 Tools
🪛 LanguageTool
[uncategorized] ~66-~66: You might be missing the article “the” here.
Context: ...dency ) {} } ``` ### From di plugin DI plugin provides abstract classes that y...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
74-75
: Improve property decorator description readability.Break down the long description into multiple sentences for better clarity.
-This decorator is used to define a property in an action content class which will be used to generate the action content object Schema and content description template for LLM object generation. +This decorator defines properties in action content classes. These properties are used to: +1. Generate the action content object Schema +2. Create content description templates for LLM object generation
89-89
: Fix typo in heading.-## Abstract Classes for Injaectable Actions and Evaluators +## Abstract Classes for Injectable Actions and Evaluators
95-96
: Fix verb agreement and improve clarity.-This abstract class simplify the creation of injectable actions. -You don't need to think about the template for content generation, it will be generated automatically based on the properties of the content Class. +This abstract class simplifies the creation of injectable actions. +Templates for content generation are automatically generated based on the properties of the content Class.🧰 Tools
🪛 LanguageTool
[uncategorized] ~95-~95: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...eInjactableAction` This abstract class simplify the creation of injectable actions. You...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
139-139
: Add a conclusion section.The documentation ends abruptly with a reference. Consider adding:
- A conclusion section summarizing key points
- A troubleshooting guide
- Links to additional resources
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
agent/package.json
(1 hunks)agent/src/index.ts
(2 hunks)packages/plugin-di/.npmignore
(1 hunks)packages/plugin-di/README.md
(1 hunks)packages/plugin-di/eslint.config.mjs
(1 hunks)packages/plugin-di/package.json
(1 hunks)packages/plugin-di/src/_examples/sampleAction.ts
(1 hunks)packages/plugin-di/src/_examples/sampleEvalutor.ts
(1 hunks)packages/plugin-di/src/_examples/samplePlugin.ts
(1 hunks)packages/plugin-di/src/_examples/sampleProvider.ts
(1 hunks)packages/plugin-di/src/actions/baseInjectableAction.ts
(1 hunks)packages/plugin-di/src/actions/index.ts
(1 hunks)packages/plugin-di/src/decorators/content.decorators.ts
(1 hunks)packages/plugin-di/src/decorators/index.ts
(1 hunks)packages/plugin-di/src/di.ts
(1 hunks)packages/plugin-di/src/evaluators/baseInjactableEvaluator.ts
(1 hunks)packages/plugin-di/src/evaluators/index.ts
(1 hunks)packages/plugin-di/src/factories/charactor.ts
(1 hunks)packages/plugin-di/src/factories/index.ts
(1 hunks)packages/plugin-di/src/factories/plugin.ts
(1 hunks)packages/plugin-di/src/index.ts
(1 hunks)packages/plugin-di/src/symbols.ts
(1 hunks)packages/plugin-di/src/templates.ts
(1 hunks)packages/plugin-di/src/tests/content.decorators.test.ts
(1 hunks)packages/plugin-di/src/tests/normalizeCharacter.test.ts
(1 hunks)packages/plugin-di/src/types.ts
(1 hunks)packages/plugin-di/tsconfig.build.json
(1 hunks)packages/plugin-di/tsconfig.json
(1 hunks)packages/plugin-di/tsup.config.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- packages/plugin-di/src/actions/index.ts
- packages/plugin-di/src/decorators/index.ts
- packages/plugin-di/tsconfig.build.json
- packages/plugin-di/src/evaluators/index.ts
- packages/plugin-di/.npmignore
- packages/plugin-di/eslint.config.mjs
- packages/plugin-di/tsconfig.json
- packages/plugin-di/src/factories/index.ts
- packages/plugin-di/src/index.ts
🧰 Additional context used
🪛 LanguageTool
packages/plugin-di/README.md
[uncategorized] ~66-~66: You might be missing the article “the” here.
Context: ...dency ) {} } ``` ### From di plugin DI plugin provides abstract classes that y...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~95-~95: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...eInjactableAction` This abstract class simplify the creation of injectable actions. You...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🪛 Biome (1.9.4)
packages/plugin-di/src/_examples/sampleEvalutor.ts
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/plugin-di/src/decorators/content.decorators.ts
[error] 31-31: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 51-51: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (5)
packages/plugin-di/src/symbols.ts (1)
4-6
: 'FACTORIES' object definition looks goodThe
FACTORIES
object is correctly defined with thePluginFactory
symbol.packages/plugin-di/src/di.ts (1)
7-15
: Dependency injection container is properly set upThe DI container is correctly configured with the plugin factory binding.
agent/src/index.ts (1)
929-931
: Verify character normalization impact.The normalization step is crucial for injectable plugins. Ensure that:
- All existing plugins remain compatible after normalization
- Character configurations are preserved
✅ Verification successful
Character normalization process verified as safe
The normalization step properly preserves plugin compatibility and character configurations. No issues found in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential plugin compatibility issues after normalization # Test: Search for plugin initialization patterns rg -A 5 'plugin\.(providers|actions|evaluators)' # Test: Search for character configuration usage rg -A 3 'character\.(settings|plugins|clients)'Length of output: 28797
agent/package.json (1)
43-43
: LGTM!The workspace dependency is correctly added.
packages/plugin-di/README.md (1)
1-139
: Well-structured documentation with clear examples!The documentation effectively introduces DI concepts and provides practical examples. The minor issues noted above don't impact the overall quality and usefulness of the content.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~66-~66: You might be missing the article “the” here.
Context: ...dency ) {} } ``` ### From di plugin DI plugin provides abstract classes that y...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~95-~95: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...eInjactableAction` This abstract class simplify the creation of injectable actions. You...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
I will keep this branch updated daily. |
…p sampleEvaluator imports - Changed private properties to protected in BaseInjectableAction for better inheritance support. - Cleaned up imports in sampleEvaluator by removing unused State import and standardizing string quotes.
…tableAction to Promise<any | null> - Updated the return type of the callback in the BaseInjectableAction class to allow for more flexible handling of results, returning null instead of void.
@coderabbitai security and second order effect analysis please |
Tip For best results, initiate chat on the files or code changes. @wtfsayo Here's the security and second-order effect analysis of the dependency injection implementation: Security Analysis:
Second-Order Effects:
Recommendations:
Would you like me to open a GitHub issue for tracking these recommendations? 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script:
Length of output: 15461 Script:
Length of output: 1062 Script:
Length of output: 1340 |
…o develop * 'develop' of https://github.com/mgunnin/virtualDegen: (41 commits) Update README.md (elizaOS#2495) Update README_CN.md (elizaOS#2489) Update README_JA.md (elizaOS#2490) feat: introduce Dependency Injection to enhance developer experience (elizaOS#2115) chore: corrected the link to the banner (elizaOS#2491) chore: update createToken.ts (elizaOS#2493) feat: Solana plugin improvement for flawless transfers (elizaOS#2340) Update README_DE.md (elizaOS#2483) feat: Sui supports the secp256k1/secp256r1 algorithms (elizaOS#2476) chore: remove eslint, prettier, tslint and replace with biome (elizaOS#2439) chore: let -> const fix: lint command fix: unused import and type errors chore: console -> elizaLogger fix: error logging and remove unused import fix: remove unused error var chore: pnpm lock file add openai var Revert "Revert "refactor: dockerize smoke tests (elizaOS#2420)" (elizaOS#2459)" fix swaps evm plugin (elizaOS#2332) ...
Relates to
No issue
Risks
Low.
This is a brand new feature.
It is only effective when using dependency injection to load a
Class
rather than anObject
within the plugin.Background
What does this PR do?
It provides dependency injection-driven object management functions for Eliza's plugin system, enabling Providers, Actions, and Evaluators in the plugin to be initialized in a dynamic way.
It also enables these objects to support multiple existence modes such as
Singleton
andonRequest
, making instance management more flexible.An abstract class of
BaseInjectableAction
is created at the same time. Through the@property
decorator, the generation of ContentClass and its Template becomes more automated without the need for additional manual creation.If you want to use this di plugin, your plugin exports will look like:
All the actions, providers, evaluators are
Class
instead ofObject
.So the normalization process is added to
agent/src/index.ts
'sstartAgents
method:This
normalizeCharacter
method will normalize the plugin with dependency injection Class to the normal plugin.What kind of change is this?
Features (non-breaking change which adds functionality)
Why are we doing this? Any context or related work?
Action
andProvider
in some plugins hope to be initialized in a dynamic way rather than directly exporting an object.Provider
s may be shared among multiplePlugin
s. They need to exist in aSingleton
instance rather than creating an instance for each plugin.Action
that can be automated through decorator definitions, and this plugin provides a convenient abstract class.Documentation changes needed?
My changes require a change to the project documentation.
Testing
Where should a reviewer start?
pnpm test --filter=@elizaos/plugin-di
Detailed testing steps
run test cases
Discord username
bt.wood