feat: add auto-generated RPC wrapper system for plugins#112
feat: add auto-generated RPC wrapper system for plugins#112Pratham-Mishra04 wants to merge 2 commits intographite-base/112from
Conversation
Summary by CodeRabbit
WalkthroughThis update introduces a fully standardized, auto-generated RPC-based plugin system for Bifrost. It defines a uniform plugin interface and constructor, implements dynamic plugin loading and lifecycle management, and provides comprehensive documentation and configuration examples. The plugin manager supports both local and package-based plugins, with automated build, RPC wrapper generation, and graceful shutdown procedures. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainServer
participant PluginManager
participant PluginProcess
participant PluginRPC
User->>MainServer: Start server (with config)
MainServer->>PluginManager: LoadPlugins(config.Plugins)
PluginManager->>PluginManager: For each plugin config:
alt Local plugin
PluginManager->>PluginManager: Generate RPC wrapper, build binary
else Package plugin
PluginManager->>PluginManager: Download, build binary
end
PluginManager->>PluginProcess: Launch plugin binary (subprocess)
PluginProcess->>PluginRPC: ServePlugin (over RPC)
PluginManager->>PluginRPC: Handshake, instantiate plugin
PluginManager->>MainServer: Return loaded plugin(s)
MainServer->>MainServer: Start HTTP server, process requests
MainServer->>PluginRPC: PreHook/PostHook via RPC
Note over MainServer,PluginManager: On shutdown: Cleanup plugins, remove temp dirs, shutdown server
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (1)
transports/bifrost-http/lib/plugins/config.go (1)
1-31: Well-structured plugin configuration system.The configuration design effectively separates concerns between local development and package-based production plugins. The struct fields are appropriately tagged and the source-specific fields use
omitemptycorrectly.Consider adding validation tags or documentation to clarify field requirements:
PluginPathis required for local sourcePackageis required for package source
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
plugins/maxim/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
core/schemas/plugin.go(3 hunks)docs/plugin-system-complete-guide.md(1 hunks)docs/plugins.md(7 hunks)plugins/maxim/go.mod(2 hunks)plugins/maxim/main.go(4 hunks)plugins/mocker/main.go(3 hunks)transports/Dockerfile(1 hunks)transports/README.md(1 hunks)transports/bifrost-http/lib/config.go(3 hunks)transports/bifrost-http/lib/ctx.go(2 hunks)transports/bifrost-http/lib/plugins/config.go(1 hunks)transports/bifrost-http/lib/plugins/manager.go(1 hunks)transports/bifrost-http/lib/plugins/rpc.go(1 hunks)transports/bifrost-http/lib/plugins/runtime.go(1 hunks)transports/bifrost-http/main.go(3 hunks)transports/bifrost-http/tracking/plugin.go(2 hunks)transports/config.example.json(3 hunks)transports/go.mod(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
transports/bifrost-http/lib/ctx.go (2)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and always returns a pointer to a valid context. It starts with context.Background() and only adds values to it, so nil checks are not needed when calling this function.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:62-64
Timestamp: 2025-06-09T11:27:00.925Z
Learning: The `lib.ConvertToBifrostContext` function in the bifrost HTTP transport never returns nil and handles the conversion internally, so nil checks are not needed when calling this function.
transports/go.mod (1)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#103
File: .github/workflows/transport-dependency-update.yml:53-75
Timestamp: 2025-06-20T16:21:18.889Z
Learning: In the bifrost repository's transport dependency update workflow, when updating the core dependency to a new version using `go get`, the go.mod and go.sum files will always change in normal operation, making the safety check for changes more of a defensive programming practice rather than handling a common scenario.
🪛 Checkov (3.2.334)
transports/Dockerfile
[LOW] 1-51: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 LanguageTool
transports/README.md
[uncategorized] ~452-~452: You might be missing the article “a” here.
Context: ...anual cmd/main.go files required For comprehensive plugin development guide, see [Plugin S...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
docs/plugin-system-complete-guide.md
[uncategorized] ~20-~20: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...Local Builds** (for development) - Build and test plugins locally 2. Go Packages...
(COMMA_COMPOUND_SENTENCE_2)
[grammar] ~268-~268: This phrase is duplicated. You should probably use “plugin directory” only once.
Context: ... - plugin_path must point to a valid plugin directory - Plugin directory must contain Go files with NewPlugin ...
(PHRASE_REPETITION)
docs/plugins.md
[uncategorized] ~448-~448: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...} ] } ## 8. Plugin Examples ### Rate Limiting Plugin go package ratelimit import...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 golangci-lint (1.64.8)
plugins/mocker/main.go
973-973: func (*MockerPlugin).log is unused
(unused)
🪛 markdownlint-cli2 (0.17.2)
docs/plugin-system-complete-guide.md
338-338: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
679-679: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
683-683: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
687-687: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
691-691: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
695-695: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
703-703: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
707-707: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
docs/plugins.md
392-392: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (24)
transports/go.mod (2)
7-7: LGTM: Addition of go-plugin dependency supports the new RPC-based plugin system.The addition of
github.com/hashicorp/go-plugin v1.6.3as a direct dependency is appropriate for implementing the standardized RPC plugin architecture described in the PR objectives.
97-97: LGTM: Local replace directive enables development workflow.The replace directive for the core module is consistent with the plugin system refactoring and enables local development by ensuring the transports module uses the latest local core schema definitions.
plugins/maxim/go.mod (1)
10-10: LGTM: Replace directive ensures consistent core interface usage.The replace directive correctly points to the local core module, ensuring the maxim plugin uses the latest plugin interface definitions including the new
SetLoggermethod and standardized constructor pattern.transports/Dockerfile (1)
51-51: LGTM: Removal of APP_PLUGINS aligns with configuration-driven plugin system.The removal of the
APP_PLUGINSenvironment variable and corresponding command-line flag is appropriate, as plugins are now loaded dynamically from the configuration file via the new plugin manager rather than through legacy environment variables.transports/bifrost-http/lib/ctx.go (2)
20-24: LGTM: Focused functionality improves code clarity.The simplified function documentation accurately reflects the focused scope of processing only Prometheus headers. This change aligns with the plugin system refactoring where plugin-specific header handling is managed within plugin implementations.
44-49: LGTM: Prometheus header processing logic is correct.The header processing logic correctly:
- Filters for x-bf-prom-* headers using case-insensitive comparison
- Strips the prefix to create clean context keys
- Uses the appropriate PrometheusContextKey for context storage
transports/bifrost-http/lib/config.go (3)
13-13: LGTM: Plugin package import supports new configuration field.The import of the plugins package is necessary to reference the
PluginConfigtype in the configuration structure.
29-34: LGTM: Plugin configuration integration is well-designed.The addition of the
Pluginsfield with type[]plugins.PluginConfigproperly integrates plugin configuration into the main HTTP transport configuration, supporting multiple plugins as described in the PR objectives.
57-79: LGTM: Comprehensive plugin configuration examples.The JSON configuration examples clearly demonstrate both plugin types (local and package) with appropriate configuration options including environment variable placeholders, which maintains consistency with the existing configuration pattern.
transports/bifrost-http/tracking/plugin.go (2)
51-51: LGTM! Interface signature correctly updated.The method signature change from returning
*schemas.BifrostResponseto*schemas.PluginShortCircuitcorrectly aligns with the standardized plugin interface. The return statement on line 60 (return req, nil, nil) is consistent with the new signature.
113-115: Appropriate no-op implementation for SetLogger.The no-op implementation is suitable for this plugin since it doesn't require custom logging functionality. This correctly implements the new Plugin interface requirement.
transports/config.example.json (2)
147-202: Comprehensive plugin configuration examples.The plugin examples effectively demonstrate both local and package plugin types with realistic configurations. The examples show proper usage of environment variables, plugin-specific config, and different enabled states.
3-3: ```shell
#!/bin/bashSearch for references to the old or new provider keys in code (.js, .ts, .jsx, .tsx) and documentation (.md)
echo "Searching for ‘openai’ in code files..."
rg -i openai --type js --type ts --type jsx --type tsx -A 2 || trueecho -e "\nSearching for ‘openai’ in markdown files..."
rg -i openai --glob '*.md' -A 2 || true</details> <details> <summary>core/schemas/plugin.go (4)</summary> `4-7`: **Appropriate imports for enhanced interface.** The addition of `context` and `encoding/json` imports supports the new standardized plugin development features correctly. --- `55-58`: **Well-designed logger injection mechanism.** The SetLogger method design is robust: - Enables dependency injection for plugins that need logging - Graceful handling of nil logger encourages defensive programming - Clear documentation of expectations --- `78-81`: **Excellent standardization through type alias.** The PluginConstructor type alias effectively documents the standardized constructor signature while maintaining flexibility. This approach promotes consistency without enforcing it at compile time, which is appropriate for a plugin system. --- `45-49`: **Comprehensive standardized development guidance.** The documentation clearly outlines the three key conventions for plugin development: 1. Standardized constructor signature 2. SetLogger implementation 3. Graceful nil logger handling This provides excellent guidance for plugin developers. </details> <details> <summary>plugins/mocker/main.go (2)</summary> `944-963`: **LGTM! Well-implemented standardized plugin constructor.** The function properly handles JSON configuration parsing with appropriate error handling and provides sensible defaults when no configuration is provided. --- `965-971`: **LGTM! Thread-safe logger injection.** Proper use of mutex to ensure thread-safe access to the logger field. </details> <details> <summary>transports/bifrost-http/main.go (2)</summary> `157-166`: **LGTM! Clean plugin loading implementation.** The code properly loads plugins from configuration and ensures the Prometheus plugin is always included. The approach of logging warnings for plugin loading failures while continuing execution is appropriate. --- `224-252`: **Excellent graceful shutdown implementation!** The shutdown sequence is well-ordered: 1. Plugin cleanup (releases plugin resources) 2. Plugin directory cleanup (removes temporary files) 3. Client cleanup (closes connections) 4. Server shutdown (stops accepting requests) Each step includes appropriate error handling that logs warnings without blocking the shutdown process. </details> <details> <summary>plugins/maxim/main.go (1)</summary> `21-56`: **Well-designed configuration handling with environment variable fallback.** The implementation provides excellent backward compatibility by supporting both JSON configuration and environment variables. The validation ensures required fields are present with clear error messages. </details> <details> <summary>docs/plugin-system-complete-guide.md (1)</summary> `20-20`: **Skip grammar suggestion - sentence is clear as written.** The compound sentence structure without a comma before "and" is appropriate here since the clauses are short and closely related. </details> <details> <summary>docs/plugins.md (1)</summary> `448-448`: **Skip grammar suggestion - compound adjective is correctly used.** The term "Rate Limiting Plugin" is a proper heading and doesn't require hyphenation. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| Bifrost supports a simplified plugin system with auto-generated RPC wrappers. You can explore the [available plugins](https://github.com/maximhq/bifrost/tree/main/plugins). | ||
|
|
||
| e.g., `-plugins maxim` | ||
| **Plugin Configuration:** | ||
|
|
||
| Note: Check plugin-specific documentation (github.com/maximhq/bifrost/tree/main/plugins/{plugin_name}) for more granular control and additional setup requirements. | ||
| ```json | ||
| { | ||
| "plugins": [ | ||
| { | ||
| "name": "maxim", | ||
| "source": "local", | ||
| "plugin_path": "./plugins/maxim", | ||
| "enabled": true, | ||
| "env_vars": { | ||
| "MAXIM_API_KEY": "env.MAXIM_API_KEY", | ||
| "MAXIM_LOG_REPO_ID": "env.MAXIM_LOG_REPO_ID" | ||
| } | ||
| }, | ||
| { | ||
| "name": "production-plugin", | ||
| "source": "package", | ||
| "package": "github.com/company/bifrost-plugin", | ||
| "version": "v1.0.0", | ||
| "enabled": true, | ||
| "config": { | ||
| "setting": "value" | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| ``` | ||
|
|
||
| **Plugin Types:** | ||
|
|
||
| - **Local plugins** (`source: "local"`): Point to plugin directory, system auto-builds with generated RPC wrapper | ||
| - **Package plugins** (`source: "package"`): Reference Go modules, system downloads and builds automatically | ||
|
|
||
| **Key Benefits:** | ||
|
|
||
| - **Zero Boilerplate**: Plugin developers only implement business logic | ||
| - **Auto-Generated RPC**: All RPC communication handled automatically | ||
| - **Simplified Development**: No manual `cmd/main.go` files required | ||
|
|
||
| For comprehensive plugin development guide, see [Plugin System Documentation](../docs/plugin-system-complete-guide.md). |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Excellent plugin system documentation.
The documentation provides comprehensive coverage of the new plugin system with clear examples for both local and package plugins. The benefits and key features are well articulated.
Fix the grammatical issue on line 452:
-For comprehensive plugin development guide, see [Plugin System Documentation](../docs/plugin-system-complete-guide.md).
+For a comprehensive plugin development guide, see [Plugin System Documentation](../docs/plugin-system-complete-guide.md).📝 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.
| Bifrost supports a simplified plugin system with auto-generated RPC wrappers. You can explore the [available plugins](https://github.com/maximhq/bifrost/tree/main/plugins). | |
| e.g., `-plugins maxim` | |
| **Plugin Configuration:** | |
| Note: Check plugin-specific documentation (github.com/maximhq/bifrost/tree/main/plugins/{plugin_name}) for more granular control and additional setup requirements. | |
| ```json | |
| { | |
| "plugins": [ | |
| { | |
| "name": "maxim", | |
| "source": "local", | |
| "plugin_path": "./plugins/maxim", | |
| "enabled": true, | |
| "env_vars": { | |
| "MAXIM_API_KEY": "env.MAXIM_API_KEY", | |
| "MAXIM_LOG_REPO_ID": "env.MAXIM_LOG_REPO_ID" | |
| } | |
| }, | |
| { | |
| "name": "production-plugin", | |
| "source": "package", | |
| "package": "github.com/company/bifrost-plugin", | |
| "version": "v1.0.0", | |
| "enabled": true, | |
| "config": { | |
| "setting": "value" | |
| } | |
| } | |
| ] | |
| } | |
| ``` | |
| **Plugin Types:** | |
| - **Local plugins** (`source: "local"`): Point to plugin directory, system auto-builds with generated RPC wrapper | |
| - **Package plugins** (`source: "package"`): Reference Go modules, system downloads and builds automatically | |
| **Key Benefits:** | |
| - **Zero Boilerplate**: Plugin developers only implement business logic | |
| - **Auto-Generated RPC**: All RPC communication handled automatically | |
| - **Simplified Development**: No manual `cmd/main.go` files required | |
| For comprehensive plugin development guide, see [Plugin System Documentation](../docs/plugin-system-complete-guide.md). | |
| **Key Benefits:** | |
| - **Zero Boilerplate**: Plugin developers only implement business logic | |
| - **Auto-Generated RPC**: All RPC communication handled automatically | |
| - **Simplified Development**: No manual `cmd/main.go` files required | |
| For a comprehensive plugin development guide, see [Plugin System Documentation](../docs/plugin-system-complete-guide.md). |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~452-~452: You might be missing the article “a” here.
Context: ...anual cmd/main.go files required For comprehensive plugin development guide, see [Plugin S...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
🤖 Prompt for AI Agents
In transports/README.md around lines 410 to 452, fix the grammatical issue on
line 452 by revising the sentence for clarity and correctness. Review the
sentence structure and punctuation to ensure it reads smoothly and
professionally without altering the intended meaning.
| // log is a helper method that logs messages using the injected logger or falls back to no-op | ||
| func (p *MockerPlugin) log(level string, message string, args ...interface{}) { | ||
| p.mu.RLock() | ||
| logger := p.logger | ||
| p.mu.RUnlock() | ||
|
|
||
| if logger != nil { | ||
| formattedMsg := fmt.Sprintf(message, args...) | ||
| switch level { | ||
| case "debug": | ||
| logger.Debug(formattedMsg) | ||
| case "info": | ||
| logger.Info(formattedMsg) | ||
| case "warn": | ||
| logger.Warn(formattedMsg) | ||
| case "error": | ||
| logger.Error(fmt.Errorf("%s", formattedMsg)) | ||
| } | ||
| } | ||
| // If no logger is injected, do nothing (graceful degradation) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused log method or fix the race condition if intended for future use.
Static analysis correctly identifies this method as unused. Additionally, there's a potential race condition: the logger reference is accessed after releasing the mutex lock (line 976), which could lead to using a stale logger if SetLogger is called concurrently.
If this method is intended for future use, consider keeping the lock until after the logger is used:
func (p *MockerPlugin) log(level string, message string, args ...interface{}) {
p.mu.RLock()
- logger := p.logger
- p.mu.RUnlock()
+ defer p.mu.RUnlock()
- if logger != nil {
+ if p.logger != nil {
formattedMsg := fmt.Sprintf(message, args...)
switch level {
case "debug":
- logger.Debug(formattedMsg)
+ p.logger.Debug(formattedMsg)
// ... rest of the cases
}
}
}Otherwise, remove this unused code to keep the codebase clean.
📝 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.
| // log is a helper method that logs messages using the injected logger or falls back to no-op | |
| func (p *MockerPlugin) log(level string, message string, args ...interface{}) { | |
| p.mu.RLock() | |
| logger := p.logger | |
| p.mu.RUnlock() | |
| if logger != nil { | |
| formattedMsg := fmt.Sprintf(message, args...) | |
| switch level { | |
| case "debug": | |
| logger.Debug(formattedMsg) | |
| case "info": | |
| logger.Info(formattedMsg) | |
| case "warn": | |
| logger.Warn(formattedMsg) | |
| case "error": | |
| logger.Error(fmt.Errorf("%s", formattedMsg)) | |
| } | |
| } | |
| // If no logger is injected, do nothing (graceful degradation) | |
| } | |
| // log is a helper method that logs messages using the injected logger or falls back to no-op | |
| func (p *MockerPlugin) log(level string, message string, args ...interface{}) { | |
| p.mu.RLock() | |
| defer p.mu.RUnlock() | |
| if p.logger != nil { | |
| formattedMsg := fmt.Sprintf(message, args...) | |
| switch level { | |
| case "debug": | |
| p.logger.Debug(formattedMsg) | |
| case "info": | |
| p.logger.Info(formattedMsg) | |
| case "warn": | |
| p.logger.Warn(formattedMsg) | |
| case "error": | |
| p.logger.Error(fmt.Errorf("%s", formattedMsg)) | |
| } | |
| } | |
| // If no logger is injected, do nothing (graceful degradation) | |
| } |
🧰 Tools
🪛 golangci-lint (1.64.8)
973-973: func (*MockerPlugin).log is unused
(unused)
🤖 Prompt for AI Agents
In plugins/mocker/main.go around lines 972 to 992, the log method is currently
unused and has a potential race condition because the logger is accessed after
releasing the read lock. To fix this, either remove the entire log method to
clean up unused code or, if you plan to use it later, hold the read lock until
after all logger calls are completed to prevent concurrent access issues.
| func (plugin *Plugin) SetLogger(logger schemas.Logger) { | ||
| // no-op | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add documentation explaining why SetLogger is a no-op.
Consider adding a comment to explain why this plugin doesn't use the injected logger:
+// SetLogger is a no-op for this plugin as it uses Maxim's own logging infrastructure
func (plugin *Plugin) SetLogger(logger schemas.Logger) {
// no-op
}🤖 Prompt for AI Agents
In plugins/maxim/main.go around lines 329 to 331, the SetLogger method is
currently a no-op without explanation. Add a comment above the method explaining
why this plugin does not use the injected logger, such as indicating it manages
logging differently or does not require external logging. This clarifies the
intent for future maintainers.
| // LoadPlugin loads a plugin from the given path and returns the plugin instance | ||
| func LoadPlugin(pluginPath string) (schemas.Plugin, error) { | ||
| // Check if plugin binary exists | ||
| if _, err := os.Stat(pluginPath); os.IsNotExist(err) { | ||
| return nil, fmt.Errorf("plugin binary not found at %s", pluginPath) | ||
| } | ||
|
|
||
| // Make sure plugin is executable | ||
| if err := os.Chmod(pluginPath, 0755); err != nil { | ||
| return nil, fmt.Errorf("failed to make plugin executable: %v", err) | ||
| } | ||
|
|
||
| // Create the plugin client | ||
| client := plugin.NewClient(&plugin.ClientConfig{ | ||
| HandshakeConfig: HandshakeConfig, | ||
| Plugins: PluginMap, | ||
| Cmd: exec.Command(pluginPath), | ||
| AllowedProtocols: []plugin.Protocol{ | ||
| plugin.ProtocolNetRPC, | ||
| }, | ||
| }) | ||
|
|
||
| // Connect via RPC | ||
| rpcClient, err := client.Client() | ||
| if err != nil { | ||
| client.Kill() | ||
| return nil, fmt.Errorf("failed to connect to plugin: %v", err) | ||
| } | ||
|
|
||
| // Request the plugin | ||
| raw, err := rpcClient.Dispense("plugin") | ||
| if err != nil { | ||
| client.Kill() | ||
| return nil, fmt.Errorf("failed to dispense plugin: %v", err) | ||
| } | ||
|
|
||
| // Type assert to our plugin interface | ||
| pluginInstance, ok := raw.(schemas.Plugin) | ||
| if !ok { | ||
| client.Kill() | ||
| return nil, fmt.Errorf("plugin does not implement Plugin interface") | ||
| } | ||
|
|
||
| return pluginInstance, nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider making file permissions configurable.
The implementation is solid with proper error handling and cleanup. However, the file permissions are hardcoded to 0755 (line 32). Consider making this configurable or at least defining it as a constant for better maintainability.
+const defaultPluginPermissions = 0755
+
func LoadPlugin(pluginPath string) (schemas.Plugin, error) {
// ... existing code ...
// Make sure plugin is executable
- if err := os.Chmod(pluginPath, 0755); err != nil {
+ if err := os.Chmod(pluginPath, defaultPluginPermissions); err != nil {📝 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.
| // LoadPlugin loads a plugin from the given path and returns the plugin instance | |
| func LoadPlugin(pluginPath string) (schemas.Plugin, error) { | |
| // Check if plugin binary exists | |
| if _, err := os.Stat(pluginPath); os.IsNotExist(err) { | |
| return nil, fmt.Errorf("plugin binary not found at %s", pluginPath) | |
| } | |
| // Make sure plugin is executable | |
| if err := os.Chmod(pluginPath, 0755); err != nil { | |
| return nil, fmt.Errorf("failed to make plugin executable: %v", err) | |
| } | |
| // Create the plugin client | |
| client := plugin.NewClient(&plugin.ClientConfig{ | |
| HandshakeConfig: HandshakeConfig, | |
| Plugins: PluginMap, | |
| Cmd: exec.Command(pluginPath), | |
| AllowedProtocols: []plugin.Protocol{ | |
| plugin.ProtocolNetRPC, | |
| }, | |
| }) | |
| // Connect via RPC | |
| rpcClient, err := client.Client() | |
| if err != nil { | |
| client.Kill() | |
| return nil, fmt.Errorf("failed to connect to plugin: %v", err) | |
| } | |
| // Request the plugin | |
| raw, err := rpcClient.Dispense("plugin") | |
| if err != nil { | |
| client.Kill() | |
| return nil, fmt.Errorf("failed to dispense plugin: %v", err) | |
| } | |
| // Type assert to our plugin interface | |
| pluginInstance, ok := raw.(schemas.Plugin) | |
| if !ok { | |
| client.Kill() | |
| return nil, fmt.Errorf("plugin does not implement Plugin interface") | |
| } | |
| return pluginInstance, nil | |
| } | |
| // Define default plugin permissions in one place | |
| const defaultPluginPermissions = 0755 | |
| // LoadPlugin loads a plugin from the given path and returns the plugin instance | |
| func LoadPlugin(pluginPath string) (schemas.Plugin, error) { | |
| // Check if plugin binary exists | |
| if _, err := os.Stat(pluginPath); os.IsNotExist(err) { | |
| return nil, fmt.Errorf("plugin binary not found at %s", pluginPath) | |
| } | |
| // Make sure plugin is executable | |
| if err := os.Chmod(pluginPath, defaultPluginPermissions); err != nil { | |
| return nil, fmt.Errorf("failed to make plugin executable: %v", err) | |
| } | |
| // Create the plugin client | |
| client := plugin.NewClient(&plugin.ClientConfig{ | |
| HandshakeConfig: HandshakeConfig, | |
| Plugins: PluginMap, | |
| Cmd: exec.Command(pluginPath), | |
| AllowedProtocols: []plugin.Protocol{ | |
| plugin.ProtocolNetRPC, | |
| }, | |
| }) | |
| // Connect via RPC | |
| rpcClient, err := client.Client() | |
| if err != nil { | |
| client.Kill() | |
| return nil, fmt.Errorf("failed to connect to plugin: %v", err) | |
| } | |
| // Request the plugin | |
| raw, err := rpcClient.Dispense("plugin") | |
| if err != nil { | |
| client.Kill() | |
| return nil, fmt.Errorf("failed to dispense plugin: %v", err) | |
| } | |
| // Type assert to our plugin interface | |
| pluginInstance, ok := raw.(schemas.Plugin) | |
| if !ok { | |
| client.Kill() | |
| return nil, fmt.Errorf("plugin does not implement Plugin interface") | |
| } | |
| return pluginInstance, nil | |
| } |
🤖 Prompt for AI Agents
In transports/bifrost-http/lib/plugins/runtime.go between lines 24 and 68, the
file permission for making the plugin executable is hardcoded as 0755. To
improve maintainability, define this permission value as a constant at the top
of the file or make it configurable via a parameter or configuration setting.
Replace the hardcoded 0755 in the os.Chmod call with the constant or
configurable variable.
| // IsPluginBinary checks if a binary is a Bifrost plugin by attempting a quick handshake | ||
| func IsPluginBinary(path string) bool { | ||
| // Check if file exists and is executable | ||
| if info, err := os.Stat(path); err != nil || info.IsDir() { | ||
| return false | ||
| } | ||
|
|
||
| // Try a quick plugin client connection | ||
| client := plugin.NewClient(&plugin.ClientConfig{ | ||
| HandshakeConfig: HandshakeConfig, | ||
| Plugins: PluginMap, | ||
| Cmd: exec.Command(path), | ||
| AllowedProtocols: []plugin.Protocol{ | ||
| plugin.ProtocolNetRPC, | ||
| }, | ||
| }) | ||
| defer client.Kill() | ||
|
|
||
| // Try to connect - if it fails, it's not a plugin | ||
| rpcClient, err := client.Client() | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Try to dispense - if it fails, it's not our plugin | ||
| _, err = rpcClient.Dispense("plugin") | ||
| return err == nil | ||
| } |
There was a problem hiding this comment.
Fix potential resource leak in plugin verification.
The function creates an RPC client connection but doesn't properly close it. Based on the pattern in LoadPlugin, you should close the RPC client when the connection succeeds:
func IsPluginBinary(path string) bool {
// ... existing checks ...
client := plugin.NewClient(&plugin.ClientConfig{
// ... config ...
})
defer client.Kill()
// Try to connect - if it fails, it's not a plugin
rpcClient, err := client.Client()
if err != nil {
return false
}
+ // Close the RPC client connection
+ defer rpcClient.Close()
// Try to dispense - if it fails, it's not our plugin
_, err = rpcClient.Dispense("plugin")
return err == nil
}📝 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.
| // IsPluginBinary checks if a binary is a Bifrost plugin by attempting a quick handshake | |
| func IsPluginBinary(path string) bool { | |
| // Check if file exists and is executable | |
| if info, err := os.Stat(path); err != nil || info.IsDir() { | |
| return false | |
| } | |
| // Try a quick plugin client connection | |
| client := plugin.NewClient(&plugin.ClientConfig{ | |
| HandshakeConfig: HandshakeConfig, | |
| Plugins: PluginMap, | |
| Cmd: exec.Command(path), | |
| AllowedProtocols: []plugin.Protocol{ | |
| plugin.ProtocolNetRPC, | |
| }, | |
| }) | |
| defer client.Kill() | |
| // Try to connect - if it fails, it's not a plugin | |
| rpcClient, err := client.Client() | |
| if err != nil { | |
| return false | |
| } | |
| // Try to dispense - if it fails, it's not our plugin | |
| _, err = rpcClient.Dispense("plugin") | |
| return err == nil | |
| } | |
| func IsPluginBinary(path string) bool { | |
| // Check if file exists and is executable | |
| if info, err := os.Stat(path); err != nil || info.IsDir() { | |
| return false | |
| } | |
| // Try a quick plugin client connection | |
| client := plugin.NewClient(&plugin.ClientConfig{ | |
| HandshakeConfig: HandshakeConfig, | |
| Plugins: PluginMap, | |
| Cmd: exec.Command(path), | |
| AllowedProtocols: []plugin.Protocol{ | |
| plugin.ProtocolNetRPC, | |
| }, | |
| }) | |
| defer client.Kill() | |
| // Try to connect - if it fails, it's not a plugin | |
| rpcClient, err := client.Client() | |
| if err != nil { | |
| return false | |
| } | |
| // Close the RPC client connection | |
| defer rpcClient.Close() | |
| // Try to dispense - if it fails, it's not our plugin | |
| _, err = rpcClient.Dispense("plugin") | |
| return err == nil | |
| } |
🤖 Prompt for AI Agents
In transports/bifrost-http/lib/plugins/runtime.go lines 70 to 97, the
IsPluginBinary function creates an RPC client connection but does not close it,
causing a potential resource leak. To fix this, after successfully obtaining the
rpcClient from client.Client(), defer a call to rpcClient.Close() to ensure the
connection is properly closed when the function exits.
| // determinePackagePath determines the local path for a package | ||
| func determinePackagePath(packageName string) string { | ||
| // For local development, assume packages are in the same workspace | ||
| // This is a simple heuristic - in production you might want more sophisticated logic | ||
| if strings.HasPrefix(packageName, "github.com/maximhq/bifrost/") { | ||
| // Handle local bifrost plugins | ||
| return "../../../" + strings.TrimPrefix(packageName, "github.com/maximhq/bifrost/") | ||
| } | ||
| // For external packages, don't use replace directive - let go mod download them | ||
| return packageName | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve package path resolution for production use.
The current implementation uses a simple heuristic that may not work in all scenarios, especially for production deployments.
Consider implementing a more robust solution:
- Check if the package exists locally using
go list - Support configuration for custom workspace layouts
- Allow explicit path mappings in the plugin configuration
Example implementation:
func determinePackagePath(packageName string, config PluginConfig) string {
// Check if explicit path mapping is provided
if config.LocalPath != "" {
return config.LocalPath
}
// Try to resolve using go list
cmd := exec.Command("go", "list", "-f", "{{.Dir}}", packageName)
if output, err := cmd.Output(); err == nil {
return strings.TrimSpace(string(output))
}
// Fall back to current heuristic
if strings.HasPrefix(packageName, "github.com/maximhq/bifrost/") {
return "../../../" + strings.TrimPrefix(packageName, "github.com/maximhq/bifrost/")
}
return packageName
}🤖 Prompt for AI Agents
In transports/bifrost-http/lib/plugins/manager.go around lines 617 to 627, the
current determinePackagePath function uses a simple heuristic that may fail in
production. Update the function to accept a PluginConfig parameter, check for an
explicit local path mapping in the config and return it if present, then attempt
to resolve the package path by running "go list" to get the local directory, and
if that fails, fall back to the existing heuristic. This will make package path
resolution more robust and configurable.
| // validateLocalPluginInterface checks if a local plugin implements the required NewPlugin function | ||
| func validateLocalPluginInterface(pluginPath string) error { | ||
| // Find all Go files in the plugin directory | ||
| files, err := filepath.Glob(filepath.Join(pluginPath, "*.go")) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to find Go files: %w", err) | ||
| } | ||
|
|
||
| if len(files) == 0 { | ||
| return fmt.Errorf("no Go files found in plugin directory") | ||
| } | ||
|
|
||
| // Parse files to check for NewPlugin function (simplified check) | ||
| for _, file := range files { | ||
| if strings.HasSuffix(file, "_test.go") { | ||
| continue // Skip test files | ||
| } | ||
|
|
||
| content, err := os.ReadFile(file) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| // Simple text search for NewPlugin function | ||
| if strings.Contains(string(content), "func NewPlugin(") { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| return fmt.Errorf("plugin must implement 'func NewPlugin(json.RawMessage) (schemas.Plugin, error)'") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider more robust plugin interface validation.
The current validation only performs a simple text search for "func NewPlugin(" which could match comments or string literals, and doesn't validate the actual function signature.
Consider using Go's AST parser for more accurate validation:
import (
"go/ast"
"go/parser"
"go/token"
)
func validateLocalPluginInterface(pluginPath string) error {
// Parse Go files in the directory
fset := token.NewFileSet()
pkgs, err := parser.ParseDir(fset, pluginPath, nil, 0)
if err != nil {
return fmt.Errorf("failed to parse Go files: %w", err)
}
// Look for NewPlugin function with correct signature
for _, pkg := range pkgs {
for _, file := range pkg.Files {
for _, decl := range file.Decls {
if fn, ok := decl.(*ast.FuncDecl); ok && fn.Name.Name == "NewPlugin" {
// Validate function signature
if validateNewPluginSignature(fn) {
return nil
}
}
}
}
}
return fmt.Errorf("plugin must implement 'func NewPlugin(json.RawMessage) (schemas.Plugin, error)'")
}Would you like me to provide the complete implementation with signature validation?
🤖 Prompt for AI Agents
In transports/bifrost-http/lib/plugins/manager.go around lines 196 to 226, the
current plugin validation uses a simple text search for "func NewPlugin(" which
can produce false positives by matching comments or strings and does not verify
the function signature. Replace this with Go's AST parser to parse the Go files
in the plugin directory, iterate through the declarations to find the NewPlugin
function, and then validate its signature precisely. Implement a helper function
to check that NewPlugin accepts a json.RawMessage parameter and returns
(schemas.Plugin, error). Return nil if a valid function is found; otherwise,
return the error indicating the required signature.
| importPath := fmt.Sprintf("github.com/maximhq/bifrost/plugins/%s", packageName) | ||
|
|
There was a problem hiding this comment.
Incorrect import path generation for local plugins.
The import path is hardcoded to assume all plugins are under github.com/maximhq/bifrost/plugins/, which is incorrect for local development plugins that may be outside the Bifrost repository.
-// Generate module-aware import path
-importPath := fmt.Sprintf("github.com/maximhq/bifrost/plugins/%s", packageName)
+// For local plugins, use a relative import path
+// The go.mod replace directive will handle the actual resolution
+importPath := packageNameThe replace directive in the generated go.mod (line 321) will correctly map this to the parent directory (../), so using just the package name as the import path is sufficient.
📝 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.
| importPath := fmt.Sprintf("github.com/maximhq/bifrost/plugins/%s", packageName) | |
| // For local plugins, use a relative import path | |
| // The go.mod replace directive will handle the actual resolution | |
| importPath := packageName |
🤖 Prompt for AI Agents
In transports/bifrost-http/lib/plugins/manager.go around lines 258-259, the
import path for plugins is hardcoded to "github.com/maximhq/bifrost/plugins/%s",
which breaks local development for plugins outside the Bifrost repo. To fix
this, change the import path to use only the package name without the full repo
path, relying on the go.mod replace directive to map local plugins correctly.
| // setPluginEnvironmentVariables sets environment variables for the plugin | ||
| func setPluginEnvironmentVariables(config PluginConfig) error { | ||
| for key, value := range config.EnvVars { | ||
| // Handle environment variable placeholders | ||
| if strings.HasPrefix(value, "env.") { | ||
| envKey := strings.TrimPrefix(value, "env.") | ||
| envValue := os.Getenv(envKey) | ||
| if envValue == "" { | ||
| log.Printf("warning: environment variable %s is not set for plugin %s", envKey, config.Name) | ||
| } | ||
| value = envValue | ||
| } | ||
|
|
||
| if err := os.Setenv(key, value); err != nil { | ||
| return fmt.Errorf("failed to set environment variable %s for plugin %s: %w", key, config.Name, err) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
❌ Incorrect review comment
Environment variables are set in the wrong process.
The setPluginEnvironmentVariables function sets environment variables in the current process using os.Setenv, but these won't be available to the plugin subprocess. The environment variables need to be passed when starting the plugin process.
The environment variables should be set when starting the plugin process in the RPC client, not in the manager process. Consider either:
- Passing environment variables through the plugin configuration that gets embedded in the generated main.go
- Setting them when the plugin subprocess is started (in the LoadPlugin function that starts the RPC client)
🏁 Script executed:
#!/bin/bash
# Check if environment variables are passed to plugin subprocesses
rg -A 10 "exec\.Command.*plugin" --type go
rg -A 5 "cmd\.Env.*=" --type goLength of output: 2310
Environment variables are set in the wrong process.
The setPluginEnvironmentVariables function sets environment variables in the current process using os.Setenv, but these won't be available to the plugin subprocess. The environment variables need to be passed when starting the plugin process.
#!/bin/bash
# Check if environment variables are passed to plugin subprocesses
rg -A 10 "exec\.Command.*plugin" --type go
rg -A 5 "cmd\.Env.*=" --type goThe environment variables should be set when starting the plugin process in the RPC client, not in the manager process. Consider either:
- Passing environment variables through the plugin configuration that gets embedded in the generated main.go
- Setting them when the plugin subprocess is started (in the LoadPlugin function that starts the RPC client)
🤖 Prompt for AI Agents
In transports/bifrost-http/lib/plugins/manager.go around lines 629 to 647, the
function setPluginEnvironmentVariables sets environment variables using
os.Setenv in the current process, but these do not propagate to the plugin
subprocess. To fix this, remove or avoid setting environment variables in the
manager process and instead pass the necessary environment variables to the
plugin subprocess when it is started, typically in the LoadPlugin function where
the exec.Command is created. Modify the plugin start code to include the
environment variables in the cmd.Env slice so they are available to the plugin
process.
|
|
||
| func main() { | ||
| // Create plugin instance | ||
| pluginInstance, err := pluginpkg.NewPlugin(json.RawMessage("{}")) |
There was a problem hiding this comment.
Package plugin configuration is not passed to NewPlugin.
The generated main.go for package plugins always passes an empty JSON configuration to NewPlugin, ignoring the actual configuration specified in config.json.
The generatePackagePluginMain function should accept and use the actual configuration:
-func generatePackagePluginMain(config PluginConfig) string {
+func generatePackagePluginMain(config PluginConfig) string {
+ // Convert config to JSON string for embedding
+ configJSON := "{}"
+ if len(config.Config) > 0 {
+ configJSON = string(config.Config)
+ }
+
return fmt.Sprintf(`package main
import (
"encoding/json"
"log"
"github.com/maximhq/bifrost/transports/bifrost-http/lib/plugins"
pluginpkg "%s"
)
func main() {
// Create plugin instance
- pluginInstance, err := pluginpkg.NewPlugin(json.RawMessage("{}"))
+ pluginInstance, err := pluginpkg.NewPlugin(json.RawMessage(%s))
if err != nil {
log.Fatalf("Failed to create plugin: %%v", err)
}
// Serve using the existing RPC implementation
plugins.ServePlugin(pluginInstance)
}
-`, config.Package)
+`, config.Package, fmt.Sprintf("`%s`", configJSON))📝 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.
| pluginInstance, err := pluginpkg.NewPlugin(json.RawMessage("{}")) | |
| func generatePackagePluginMain(config PluginConfig) string { | |
| // Convert config to JSON string for embedding | |
| configJSON := "{}" | |
| if len(config.Config) > 0 { | |
| configJSON = string(config.Config) | |
| } | |
| return fmt.Sprintf(`package main | |
| import ( | |
| "encoding/json" | |
| "log" | |
| "github.com/maximhq/bifrost/transports/bifrost-http/lib/plugins" | |
| pluginpkg "%s" | |
| ) | |
| func main() { | |
| // Create plugin instance | |
| pluginInstance, err := pluginpkg.NewPlugin(json.RawMessage(%s)) | |
| if err != nil { | |
| log.Fatalf("Failed to create plugin: %%v", err) | |
| } | |
| // Serve using the existing RPC implementation | |
| plugins.ServePlugin(pluginInstance) | |
| } | |
| `, config.Package, fmt.Sprintf("`%s`", configJSON)) | |
| } |
🤖 Prompt for AI Agents
In transports/bifrost-http/lib/plugins/manager.go at line 571, the call to
NewPlugin currently passes an empty JSON configuration, ignoring the actual
plugin configuration. Modify the generatePackagePluginMain function to accept
the real plugin configuration from config.json and pass it as a JSON.RawMessage
argument to NewPlugin instead of the empty JSON object.
Pratham-Mishra04
changed the base branch from
06-21-feat_mocker_plugin_added
to
graphite-base/112
|
|
2 participants
Standardized Plugin System with Auto-Generated RPC Wrappers
This PR introduces a simplified plugin system with auto-generated RPC wrappers, significantly reducing the development effort required to create Bifrost plugins. The key improvements include:
NewPlugin(config json.RawMessage) (Plugin, error)SetLoggermethod to the Plugin interface for dependency injectionThe PR includes comprehensive documentation:
The implementation includes:
This change simplifies plugin development by eliminating boilerplate code while maintaining process isolation for security and stability. Existing plugins (maxim, mocker) have been updated to support the new standardized interface.