fix: fixes plugin names sanitization for live loaded plugins for connectors - #4236
Merged
Pratham-Mishra04 merged 1 commit intoJun 9, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a centralized ChangesPlugin Name Sanitization and Loaded Plugins API
Sequence Diagram(s)sequenceDiagram
participant UI
participant FrontendAPI
participant PluginsHandler
participant Config
UI->>FrontendAPI: useGetLoadedPluginsQuery() request
FrontendAPI->>PluginsHandler: GET /api/plugins/loaded
PluginsHandler->>Config: GetLoadedPluginNames()
Config-->>PluginsHandler: sanitized, deduped, sorted names
PluginsHandler-->>FrontendAPI: { "plugins": [...] }
FrontendAPI-->>UI: plugin list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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" Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
roroghost17
marked this pull request as ready for review
June 9, 2026 21:30
|
|
Contributor
Confidence Score: 5/5Safe to merge — all filter modes are guarded against saving while the plugin list is loading, the new endpoint is behind the same middleware as existing plugin routes, and the round-trip invariant test prevents the span-name/filter-name mismatch from regressing. The Go-side changes are straightforward atomic reads with deduplication and sorting. The new endpoint and interface methods have adequate test coverage. The initialization guard in the tracing sheet correctly handles loading, error, and empty states by keeping Save disabled. The only gap is that an API error shows an empty list with no user-visible explanation, which is a UX nit that does not risk data loss. No files require special attention for correctness. Important Files Changed
Reviews (3): Last reviewed commit: "fix: fixes plugin names sanitization for..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@transports/bifrost-http/handlers/plugins.go`:
- Around line 171-177: Add a unit test for PluginsHandler.getLoadedPlugins that
mirrors other handler tests: create a mock plugins loader (e.g.,
mockPluginsLoaderWithNames) returning a known slice like
["logging","telemetry","governance"], instantiate PluginsHandler with that
loader, call getLoadedPlugins on a new fasthttp.RequestCtx (set method to GET),
then assert ctx.Response.StatusCode() == 200 and that the JSON body contains a
"plugins" array of the expected length and values; also use
SetLogger(&mockLogger{}) as in other tests to silence logging.
In `@transports/bifrost-http/server/server.go`:
- Around line 1162-1166: GetLoadedPluginNames currently calls
s.Config.GetLoadedPluginNames() without checking s.Config for nil; add the same
defensive nil-check used in GetModelsForProvider and
GetUnfilteredModelsForProvider: if s.Config == nil return an empty []string,
otherwise return s.Config.GetLoadedPluginNames(); update the
GetLoadedPluginNames method on BifrostHTTPServer to mirror those methods'
pattern.
In `@ui/app/workspace/observability/sheets/pluginTracingSheet.tsx`:
- Around line 63-67: The include-mode initialization is treating an empty array
from useGetLoadedPluginsQuery as "still loading" because it checks
allPlugins.length === 0, which deadlocks when the backend legitimately returns
[] and prevents wasOpenRef.current from being set and Save from enabling; change
the guard to detect loading by checking whether allPlugins is undefined/null
(i.e., use allPlugins === undefined or data === undefined) instead of length,
and ensure the logic around wasOpenRef.current and the include-mode
initialization in the component that references useGetLoadedPluginsQuery,
allPlugins, useGetPluginQuery, and wasOpenRef.current treats an empty array as a
valid loaded state so initialization and Save can proceed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34f34495-6ec3-47e6-a562-4e7744fcbd5a
📒 Files selected for processing (11)
core/schemas/span_filter.gocore/schemas/span_filter_test.gocore/utils.godocs/enterprise/datadog-connector.mdxdocs/features/observability/otel.mdxtransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/handlers/plugins_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/observability/sheets/pluginTracingSheet.tsxui/lib/store/apis/pluginsApi.ts
roroghost17
force-pushed
the
06-10-fix_fixes_plugin_names_sanitization_for_live_loaded_plugins_for_connectors
branch
from
June 9, 2026 21:59
8dbaa21 to
525b897
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/observability/sheets/pluginTracingSheet.tsx (1)
66-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock initialization/save when loaded-plugins query fails.
Line 76 only gates on loading. If
/plugins/loadedfails,allPluginsfalls back to[], initialization still completes, and Save can wipe an existing include filter.Suggested patch
- const { data: allPlugins = [], isLoading: isLoadingLoadedPlugins } = useGetLoadedPluginsQuery(); + const { + data: loadedPlugins, + isLoading: isLoadingLoadedPlugins, + isError: isLoadedPluginsError, + } = useGetLoadedPluginsQuery(); + const allPlugins = loadedPlugins ?? []; @@ - if (filter?.mode === "include" && isLoadingLoadedPlugins) return; + if (filter?.mode === "include" && (isLoadingLoadedPlugins || isLoadedPluginsError)) return; @@ - }, [open, targetPlugin, allPlugins, isLoadingLoadedPlugins]); + }, [open, targetPlugin, allPlugins, isLoadingLoadedPlugins, isLoadedPluginsError]); @@ if (!targetPlugin) { toast.error(`${destination} is not configured yet. Save its configuration before configuring plugin tracing.`); return; } + if (isLoadedPluginsError) { + toast.error("Could not load plugin list. Please retry."); + return; + } @@ - }, [toggles, targetPlugin, updatePlugin, onClose, pluginName, destination]); + }, [toggles, targetPlugin, updatePlugin, onClose, pluginName, destination, isLoadedPluginsError]); @@ - disabled={isLoading || !wasOpenRef.current} + disabled={isLoading || !wasOpenRef.current || isLoadedPluginsError}Also applies to: 87-113, 170-170
🤖 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 `@ui/app/workspace/observability/sheets/pluginTracingSheet.tsx` around lines 66 - 82, The initialization currently proceeds when the loaded-plugins query fails because it only checks isLoadingLoadedPlugins; update the effect(s) that set toggles (where you call setToggles(resolveToggleState(...))) to also check the loaded-plugins error state returned by useGetLoadedPluginsQuery (e.g., isError or error) and abort initialization/save if the query errored so you don't treat allPlugins fallback [] as a valid source; specifically, in the effect that references open, targetPlugin, allPlugins, isLoadingLoadedPlugins (and the other similar blocks noted), add a guard like "if (isErrorLoadedPlugins) return" before calling resolveToggleState or allowing Save to proceed. Ensure you reference the existing hooks/variables (useGetLoadedPluginsQuery, allPlugins, isLoadingLoadedPlugins, isErrorLoadedPlugins, targetPlugin, resolveToggleState) when implementing the check.
🤖 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.
Inline comments:
In `@core/schemas/span_filter_test.go`:
- Around line 67-69: The test failure message for SanitizePluginSpanName has the
got/want arguments swapped; update the t.Errorf call in the test (the assertion
around SanitizePluginSpanName(tt.in)) so the formatting order is
t.Errorf("SanitizePluginSpanName(%q) = %q, want %q", tt.in, got, tt.want) —
i.e., pass the variable holding the actual result (got) before the expected
value (tt.want) so failures print correctly.
---
Outside diff comments:
In `@ui/app/workspace/observability/sheets/pluginTracingSheet.tsx`:
- Around line 66-82: The initialization currently proceeds when the
loaded-plugins query fails because it only checks isLoadingLoadedPlugins; update
the effect(s) that set toggles (where you call
setToggles(resolveToggleState(...))) to also check the loaded-plugins error
state returned by useGetLoadedPluginsQuery (e.g., isError or error) and abort
initialization/save if the query errored so you don't treat allPlugins fallback
[] as a valid source; specifically, in the effect that references open,
targetPlugin, allPlugins, isLoadingLoadedPlugins (and the other similar blocks
noted), add a guard like "if (isErrorLoadedPlugins) return" before calling
resolveToggleState or allowing Save to proceed. Ensure you reference the
existing hooks/variables (useGetLoadedPluginsQuery, allPlugins,
isLoadingLoadedPlugins, isErrorLoadedPlugins, targetPlugin, resolveToggleState)
when implementing the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d4d7ea3c-cf66-46f0-9331-c32616e378f6
📒 Files selected for processing (11)
core/schemas/span_filter.gocore/schemas/span_filter_test.gocore/utils.godocs/enterprise/datadog-connector.mdxdocs/features/observability/otel.mdxtransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/handlers/plugins_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/observability/sheets/pluginTracingSheet.tsxui/lib/store/apis/pluginsApi.ts
roroghost17
force-pushed
the
06-10-fix_fixes_plugin_names_sanitization_for_live_loaded_plugins_for_connectors
branch
from
June 9, 2026 22:18
525b897 to
e8a8e48
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@ui/app/workspace/observability/sheets/pluginTracingSheet.tsx`:
- Line 76: The current guard if (isLoadingLoadedPlugins || allPlugins.length ===
0) incorrectly treats an empty allPlugins array as "not loaded" and prevents
initialization; change the condition to only block when loading: check
isLoadingLoadedPlugins (or a dedicated loading flag) but allow empty arrays to
proceed so wasOpenRef.current gets set to true and enabling Save; ensure
initialization logic that calls resolveToggleState(filter, allPlugins),
buildFilter({}), and sets plugin_span_filter can handle an empty allPlugins
array as a valid loaded state (references: isLoadingLoadedPlugins, allPlugins,
wasOpenRef.current, resolveToggleState, buildFilter, plugin_span_filter).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0dccfbd3-e1f6-4be0-97f2-81f48112849f
📒 Files selected for processing (11)
core/schemas/span_filter.gocore/schemas/span_filter_test.gocore/utils.godocs/enterprise/datadog-connector.mdxdocs/features/observability/otel.mdxtransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/handlers/plugins_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/observability/sheets/pluginTracingSheet.tsxui/lib/store/apis/pluginsApi.ts
Collaborator
Merge activity
|
Pratham-Mishra04
deleted the
06-10-fix_fixes_plugin_names_sanitization_for_live_loaded_plugins_for_connectors
branch
June 9, 2026 22:33
akshaydeo
pushed a commit
that referenced
this pull request
Jun 12, 2026
…ectors (#4236) ## Summary The plugin tracing configuration sheet previously built its plugin list by merging a hardcoded built-in plugin list with custom plugins fetched from the config store. This meant enterprise plugins, auto-loaded plugins, and any plugin registered under a name different from its config key (e.g. `enterprise-prompts` instead of `prompts`) were silently missing from the list. As a result, users could not configure span filtering for those plugins, and any manually entered filter names could silently no-op. This PR replaces that approach with a single `/api/plugins/loaded` endpoint that returns the sanitized names of every plugin actually loaded at runtime — the exact names embedded in their trace spans — and uses that list throughout the tracing sheet and filter logic. ## Changes - Extracted `SanitizePluginSpanName` from `core/utils.go` into `core/schemas/span_filter.go` as an exported function so the same normalization logic is shared between span construction and span filtering. - Added `GetLoadedPluginNames()` to `Config`, `BifrostHTTPServer`, and the `PluginsLoader`/`ServerCallbacks` interfaces, returning deduplicated, sorted, sanitized plugin names for all currently loaded plugins. - Added a `GET /api/plugins/loaded` route backed by `getLoadedPlugins`, which returns the runtime plugin list. - Added a `getLoadedPlugins` RTK Query endpoint (`useGetLoadedPluginsQuery`) in the UI. - Replaced the built-in/custom split in `pluginTracingSheet.tsx` with a single flat list sourced from `useGetLoadedPluginsQuery`, removing the separate "Built-in Plugins" and "Custom Plugins" sections. - Added `TestSanitizePluginSpanName` and `TestSanitizedNameMatchesSpanExtraction` to lock the invariant that names used to build spans round-trip correctly through `PluginNameFromSpan`. - Updated the OTel and Datadog connector docs to clarify that plugin names in span filters must match the name shown in the tracing sheet, and that enterprise plugins like `enterprise-prompts` and `enterprise-governance` differ from their config keys. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [x] Docs ## How to test ```sh # Core/Transports go test ./core/schemas/... ./transports/bifrost-http/... # UI cd ui pnpm i pnpm test pnpm build ``` 1. Start the gateway with a mix of built-in, enterprise, and custom plugins loaded. 2. Open the **Configure Plugin Tracing** sheet for an observability connector. 3. Verify the plugin list includes enterprise plugins (e.g. `enterprise-prompts`, `enterprise-governance`) and any auto-loaded plugins, not just the hardcoded built-in set. 4. Call `GET /api/plugins/loaded` directly and confirm the returned names match what appears in the sheet and in actual trace span names (`plugin.<name>.<stage>`). 5. Configure an `include` or `exclude` filter using a name from the sheet and verify spans are correctly filtered in the connected APM backend. ## Breaking changes - [x] Yes - [ ] No `PluginsLoader` and `ServerCallbacks` interfaces gain a `GetLoadedPluginNames() []string` method. Any external implementations of these interfaces must add this method. ## Related issues ## Security considerations The `/api/plugins/loaded` endpoint exposes the names of all loaded plugins. It should be protected by the same middleware chain as other `/api/plugins` routes, which it is via `ChainMiddlewares`. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * API endpoint exposing currently loaded plugin names * Observability UI: unified "Plugins" list with improved initialization and select-all behavior * Consistent plugin name normalization so filter matching aligns with displayed plugin names * **Documentation** * Clarified plugin name guidance for span filtering; instructs copying exact names from the UI * **Tests** * Added tests for plugin name normalization and loaded-plugins endpoint <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The plugin tracing configuration sheet previously built its plugin list by merging a hardcoded built-in plugin list with custom plugins fetched from the config store. This meant enterprise plugins, auto-loaded plugins, and any plugin registered under a name different from its config key (e.g.
enterprise-promptsinstead ofprompts) were silently missing from the list. As a result, users could not configure span filtering for those plugins, and any manually entered filter names could silently no-op.This PR replaces that approach with a single
/api/plugins/loadedendpoint that returns the sanitized names of every plugin actually loaded at runtime — the exact names embedded in their trace spans — and uses that list throughout the tracing sheet and filter logic.Changes
SanitizePluginSpanNamefromcore/utils.gointocore/schemas/span_filter.goas an exported function so the same normalization logic is shared between span construction and span filtering.GetLoadedPluginNames()toConfig,BifrostHTTPServer, and thePluginsLoader/ServerCallbacksinterfaces, returning deduplicated, sorted, sanitized plugin names for all currently loaded plugins.GET /api/plugins/loadedroute backed bygetLoadedPlugins, which returns the runtime plugin list.getLoadedPluginsRTK Query endpoint (useGetLoadedPluginsQuery) in the UI.pluginTracingSheet.tsxwith a single flat list sourced fromuseGetLoadedPluginsQuery, removing the separate "Built-in Plugins" and "Custom Plugins" sections.TestSanitizePluginSpanNameandTestSanitizedNameMatchesSpanExtractionto lock the invariant that names used to build spans round-trip correctly throughPluginNameFromSpan.enterprise-promptsandenterprise-governancediffer from their config keys.Type of change
Affected areas
How to test
enterprise-prompts,enterprise-governance) and any auto-loaded plugins, not just the hardcoded built-in set.GET /api/plugins/loadeddirectly and confirm the returned names match what appears in the sheet and in actual trace span names (plugin.<name>.<stage>).includeorexcludefilter using a name from the sheet and verify spans are correctly filtered in the connected APM backend.Breaking changes
PluginsLoaderandServerCallbacksinterfaces gain aGetLoadedPluginNames() []stringmethod. Any external implementations of these interfaces must add this method.Related issues
Security considerations
The
/api/plugins/loadedendpoint exposes the names of all loaded plugins. It should be protected by the same middleware chain as other/api/pluginsroutes, which it is viaChainMiddlewares.Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
New Features
Documentation
Tests