[fix]: classify dynamic plugins by the hooks they actually export - #5702
[fix]: classify dynamic plugins by the hooks they actually export#5702eyeveil wants to merge 1 commit into
Conversation
A .so plugin is wrapped in a DynamicPlugin, which structurally satisfies LLMPlugin, MCPPlugin and HTTPTransportPlugin regardless of which symbols the .so exports. Bare type assertions therefore classified every dynamic plugin as all three, so it landed in all three interface caches and ran no-op hooks on every request, reported misleading types through the API, and had Cleanup() called once per list on removal - a double close for plugins that release resources there. Both call sites now use the framework's As* helpers, which additionally check the hook function pointers. Built-in plugins are unaffected; they are concrete types whose classification is unchanged. Changes: - transports/bifrost-http/lib/config.go - rebuildInterfaceCaches - transports/bifrost-http/server/plugins.go - InferPluginTypes
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesDynamic plugin classification
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
d6855d3 to
9c5fb61
Compare
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
transports/bifrost-http/lib/config.go (1)
5134-5144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven coverage for hook-based plugin classification.
rebuildInterfaceCaches()andInferPluginTypes()now depend on the framework’sAs*helpers. Add deterministic coverage for dynamic plugins with LLM-only, MCP-only, HTTP-only, multiple hooks, and no hooks, asserting both cache contents andInferPluginTypesresults.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config.go` around lines 5134 - 5144, Add table-driven tests covering dynamic plugins with LLM-only, MCP-only, HTTP-only, multiple-hook, and no-hook configurations. For each case, invoke rebuildInterfaceCaches() and InferPluginTypes(), then assert the expected cache membership and inferred plugin types, ensuring classification uses the framework As* helpers rather than bare interface assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 5134-5144: Add table-driven tests covering dynamic plugins with
LLM-only, MCP-only, HTTP-only, multiple-hook, and no-hook configurations. For
each case, invoke rebuildInterfaceCaches() and InferPluginTypes(), then assert
the expected cache membership and inferred plugin types, ensuring classification
uses the framework As* helpers rather than bare interface assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 55f3fab1-351d-4c14-a2a4-1d9049aeff56
📒 Files selected for processing (3)
framework/logstore/asyncjob_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
transports/bifrost-http/server/plugins.go (1)
24-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven regression coverage for adapter-based classification.
Cover dynamic plugins with no hooks, each individual hook family, multiple hook families, and built-in plugins. Verify both
InferPluginTypesandrebuildInterfaceCachesreport the same classifications.As per coding guidelines, Go behavior changes should have deterministic, table-driven coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/server/plugins.go` around lines 24 - 42, Add deterministic table-driven tests covering dynamic plugins with no hooks, each individual hook family, combinations of multiple hook families, and built-in plugins. For every case, assert that both InferPluginTypes and rebuildInterfaceCaches produce the same expected classifications, using the existing plugin adapters and test helpers rather than duplicating classification logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@transports/bifrost-http/server/plugins.go`:
- Around line 24-42: Add deterministic table-driven tests covering dynamic
plugins with no hooks, each individual hook family, combinations of multiple
hook families, and built-in plugins. For every case, assert that both
InferPluginTypes and rebuildInterfaceCaches produce the same expected
classifications, using the existing plugin adapters and test helpers rather than
duplicating classification logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 38faa928-5b2b-4cec-b3f2-ab8494312ce1
📒 Files selected for processing (2)
transports/bifrost-http/lib/config.gotransports/bifrost-http/server/plugins.go
|
recheck |
2 participants
Fixes #5700
Summary
DynamicPluginsatisfies all three hook interfaces regardless of which symbols the.soexports, and both classification sites use plain type assertions. So every dynamic plugin lands in all three interface caches, runs no-op hooks per request, reports types it does not implement, and getsCleanup()called once per list on removal, which is a double close for anything that frees resources there.Switched both sites to the
As*helpers inframework/plugins, which also check the hook pointers. That is what they are there for. Builtins are concrete types, so nothing changes for them.Type of change
Affected areas
How to test
Install a
.soexportingGetName,CleanupandPreLLMHookbut no MCP hooks, then disable it. Before:Cleanupruns twice and the API listsmcpin its types. After: once, and the types match the exported symbols.Breaking changes
Checklist
docs/contributing/README.mdand followed the guidelinesDynamicPluginhook fields are unexported, so a test would need to live inframework/pluginsnext to theAs*helpers. Happy to add one there if you would like.