Skip to content

plugin and sidebar fixes - #4213

Merged
akshaydeo merged 4 commits into
v2.0.0from
06-09-plugin_and_sidebar_fixes
Jun 17, 2026
Merged

akshaydeo merged 4 commits into
v2.0.0from
06-09-plugin_and_sidebar_fixes

Conversation

@akshaydeo

@akshaydeo akshaydeo commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Briefly explain the purpose of this PR and the problem it solves.

Changes

  • What was changed and why
  • Any notable design decisions or trade-offs

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

Describe the steps to validate this change. Include commands and expected outcomes.

# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build

If adding new configs or environment variables, document them here.

Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

Breaking changes

  • Yes
  • No

If yes, describe impact and migration instructions.

Related issues

Link related issues and discussions. Example: Closes #123

Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

Summary by CodeRabbit

  • Bug Fixes

    • Preserve auth-required errors so upstream hooks can handle them.
    • Adjusted tool-execution error handling to surface original error types.
  • New Features

    • App-name normalization and canonical app context propagation.
    • Optional Virtual Key auth mode allowing VK-authenticated requests to bypass standard auth when enabled.
    • Additional execution logging for tool runs.
  • Tests

    • Added tests for pre-execution short-circuit behavior and app-context detection.
  • Style

    • Updated edge inventory layout and spacing.

This was referenced Jun 9, 2026
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR normalizes app names into a canonical context key, updates the logging plugin to set that context, preserves typed MCPAuthRequired errors through plugin pipeline paths, moves tool preparation inside the plugin pipeline callback, adds VK credential detection with optional auth bypass, inserts MCP tool execution logs, and adds tests for pre-hook short-circuiting plus a UI layout tweak.

Changes

MCP app context and execution flow

Layer / File(s) Summary
App key contracts and normalization
core/schemas/bifrost.go, core/schemas/useragents.go, core/schemas/useragents_test.go
Introduces BifrostContextKeyApp and AppKeyFromName (canonical kebab-case app keys). Adjusts BifrostResponseExtraFields alignment and extends user-agent tests.
Logging plugin app context assignment
plugins/logging/main.go, plugins/logging/operations_test.go
Replaces header-based app extraction with AppKeyFromName(initialData.App) in PreLLMHook; sets context only when non-empty. Adds tests for detection, header precedence, and custom mappings.
Plugin pipeline auth-required error preservation
core/mcp/pluginpipeline.go
Wraps op errors into BifrostError and uses errors.As to detect and populate MCPAuthRequired in ExtraFields for both pipeline and no-pipeline branches.
Tool execution preparation ordering
core/mcp/exec.go
Runs prepareToolExecution inside RunWithPluginPipeline callback (after pre-hook); callback returns prepare errors directly. Derives executionConfig/toolNameMapping only when client state is non-nil.
MCP pre-hook short-circuit tests
core/internal/mcptests/plugin_test.go
Adds TestPlugin_PreMCPHookShortCircuitsBeforeToolResolution to assert pre-hook short-circuit content is returned before tool resolution; minor formatting tweak in an existing test.
MCP tool execution observability logging
transports/bifrost-http/handlers/mcpserver.go
Adds logger.Info at tool start (tool name + args), on error (tool name + user-facing message), and on success (tool name) for observability.

Auth middleware updates

Layer / File(s) Summary
Virtual Key detection and bypass
transports/bifrost-http/handlers/middlewares.go
Adds hasVirtualKeyCredential (header/token checks) and an allowVirtualKeyAuth flag to AuthMiddleware.middleware; short-circuits to next(ctx) when allowed; InferenceMiddleware disallows VK bypass.

UI layout refinement

Layer / File(s) Summary
Edge inventory page layout styling
ui/app/workspace/edge-control/inventory/page.tsx
Adds no-padding-parent, viewport-based height h-[calc(100dvh-1rem)], and padding p-4 to the page wrapper while retaining width/centering and flex layout.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • maximhq/bifrost#3794: Related pipeline/gate changes around RunWithPluginPipeline and MCP execution plumbing.
  • maximhq/bifrost#3702: Related changes to MCP tool execution ordering and auth-required error handling.

Suggested reviewers

  • danpiths
  • roroghost17

Poem

🐰 A twitch, a hop, a patch in flight,
App keys tidy into kebab-case light,
Hooks may halt before tools awake,
Auth errors kept for the pipeline's sake,
Logs sing the tale—now steady, bright.

🚥 Pre-merge checks | ❌ 5

❌ Failed checks (4 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is the template with no actual content filled in—no summary, specific changes, or affected areas are documented. Complete all required sections: fill in summary, list specific changes, check affected areas, provide testing steps, and document any security implications.
Linked Issues check ⚠️ Warning The linked issue #123 requests Files API support for providers, but the changes implement plugin hooks, authentication, and logging features unrelated to file upload APIs. Either link issues that match the actual changes (auth/plugin enhancements) or implement the Files API functionality described in #123.
Out of Scope Changes check ⚠️ Warning Most changes address plugin hook handling, auth middleware, and logging; these are out of scope for the Files API support objective in #123. Clarify the PR scope: either focus on Files API implementation per #123 or link/create issues specific to the plugin, auth, and UI changes included.
Docstring Coverage ⚠️ Warning Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'plugin and sidebar fixes' is vague and generic, using non-descriptive terms that don't convey meaningful information about specific changes. Replace with a specific, descriptive title that highlights the primary change (e.g., 'Add VirtualKey auth bypass support and enhance plugin MCP hook handling').

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 06-09-plugin_and_sidebar_fixes

Comment @coderabbitai help to get the list of available commands and usage tips.

akshaydeo commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@akshaydeo akshaydeo mentioned this pull request Jun 9, 2026
18 tasks
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch 3 times, most recently from 31e8395 to 4d35169 Compare June 11, 2026 11:02
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from 4d35169 to 8e156c7 Compare June 11, 2026 11:25
@akshaydeo
akshaydeo marked this pull request as ready for review June 11, 2026 14:22
@greptile-apps

greptile-apps Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

The handlers package will not compile until InferenceMiddleware is updated to pass the new required second argument — all inference and MCP routes are unreachable until that one-liner is fixed.

The compile error in InferenceMiddleware (flagged in a prior pass) remains unresolved in the current HEAD. Every other change in this PR is correct: the MCPAuthRequiredError threading, the VK prefix guard, the app-context propagation, and the sidebar layout fix all look sound. The compile blocker is the only thing standing between this PR and safe merge.

transports/bifrost-http/handlers/middlewares.go — InferenceMiddleware still calls m.middleware with one argument; the updated signature requires two.

Important Files Changed

Filename Overview
core/mcp/exec.go Moves prepareToolExecution inside the op callback so pre-hooks run before connection acquisition; MCPAuthRequiredError propagation is delegated to pluginpipeline. RequestType is stamped onto bErr after RunWithPluginPipeline returns, preserving the correct value.
core/mcp/pluginpipeline.go MCPAuthRequiredError preservation added to both the no-pipeline and pipeline error paths, correctly unwrapping the typed error before it is boxed into BifrostError.
core/schemas/bifrost.go Adds BifrostContextKeyApp = "app"; the bare key risks collision with third-party middleware (noted in prior review thread).
plugins/logging/main.go Removes the x-bf-app header override in favour of propagating the UA-detected app key into BifrostContextKeyApp; the behaviour change is intentional and documented in the new tests.
transports/bifrost-http/handlers/middlewares.go Adds hasVirtualKeyCredential with correct sk-bf- prefix check (prior security concern addressed). APIMiddleware updated to pass allowVirtualKeyAuth=false. InferenceMiddleware still calls middleware with only one argument — a compile error flagged in a prior review pass.
transports/bifrost-http/handlers/mcpserver.go Adds per-invocation Info logging for tool start, error, and success. The error path logs at Info rather than Warn/Error, which silences it under production log-level thresholds.
ui/app/workspace/edge-control/inventory/page.tsx Layout fix adding height/padding classes to the inventory page wrapper; data-testid preserved.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant executeToolWithHooks
    participant RunWithPluginPipeline
    participant PreHooks
    participant prepareToolExecution
    participant ExecuteTool
    participant PostHooks

    Caller->>executeToolWithHooks: ExecuteChatMCPTool(ctx, toolCall)
    executeToolWithHooks->>RunWithPluginPipeline: op callback
    RunWithPluginPipeline->>PreHooks: RunMCPPreHooks(ctx, req)
    alt short-circuit
        PreHooks-->>RunWithPluginPipeline: shortCircuit response/error
        RunWithPluginPipeline->>PostHooks: RunMCPPostHooks
        PostHooks-->>Caller: short-circuit result
    else proceed
        PreHooks-->>RunWithPluginPipeline: preReq (mutated)
        RunWithPluginPipeline->>prepareToolExecution: (ctx, preReq) — NEW: after pre-hooks
        prepareToolExecution-->>RunWithPluginPipeline: conn + release
        RunWithPluginPipeline->>ExecuteTool: (ctx, preReq, conn, ...)
        ExecuteTool-->>RunWithPluginPipeline: result / MCPAuthRequiredError
        Note over RunWithPluginPipeline: defer release() fires here
        RunWithPluginPipeline->>PostHooks: RunMCPPostHooks(ctx, resp, bErr)
        PostHooks-->>executeToolWithHooks: finalResp / finalErr
        executeToolWithHooks-->>Caller: stamps RequestType on bErr
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant executeToolWithHooks
    participant RunWithPluginPipeline
    participant PreHooks
    participant prepareToolExecution
    participant ExecuteTool
    participant PostHooks

    Caller->>executeToolWithHooks: ExecuteChatMCPTool(ctx, toolCall)
    executeToolWithHooks->>RunWithPluginPipeline: op callback
    RunWithPluginPipeline->>PreHooks: RunMCPPreHooks(ctx, req)
    alt short-circuit
        PreHooks-->>RunWithPluginPipeline: shortCircuit response/error
        RunWithPluginPipeline->>PostHooks: RunMCPPostHooks
        PostHooks-->>Caller: short-circuit result
    else proceed
        PreHooks-->>RunWithPluginPipeline: preReq (mutated)
        RunWithPluginPipeline->>prepareToolExecution: (ctx, preReq) — NEW: after pre-hooks
        prepareToolExecution-->>RunWithPluginPipeline: conn + release
        RunWithPluginPipeline->>ExecuteTool: (ctx, preReq, conn, ...)
        ExecuteTool-->>RunWithPluginPipeline: result / MCPAuthRequiredError
        Note over RunWithPluginPipeline: defer release() fires here
        RunWithPluginPipeline->>PostHooks: RunMCPPostHooks(ctx, resp, bErr)
        PostHooks-->>executeToolWithHooks: finalResp / finalErr
        executeToolWithHooks-->>Caller: stamps RequestType on bErr
    end
Loading

Reviews (15): Last reviewed commit: "rebase change" | Re-trigger Greptile

Comment thread core/schemas/bifrost.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@docs/edge/supported-applications.mdx`:
- Around line 52-76: The docs include a "Databricks" Card that is not
implemented in code (no core/providers/databricks, no Databricks entry in the
ModelProvider enum in core/schemas/bifrost.go), so either remove the Card and
update the wording that claims "every provider Bifrost supports" or implement
Databricks support; to fix, choose one: (A) remove the Card with title
"Databricks" from docs/edge/supported-applications.mdx and change the
accompanying copy to avoid claiming all providers are supported, or (B)
implement the provider by adding a databricks provider implementation under
core/providers (matching other providers), add a Databricks variant to the
ModelProvider enum in core/schemas/bifrost.go, and wire any necessary
transport/schemas so rg tests find no missing references; ensure tests/build
pass after the change.

In `@plugins/logging/operations_test.go`:
- Around line 99-128: The new tests call Init(...) which starts background
workers but never stop them; after successfully creating plugin in
TestPreLLMHookSetsAppContextFromDetectedApp (and the other affected tests at
lines ~136-168 and ~193-229), ensure you call plugin.Cleanup() to avoid leaking
goroutines—i.e., after checking err != nil from Init, add a defer
plugin.Cleanup() (or explicit cleanup at test end) so background workers started
by Init are properly stopped; reference the Init(...) call and the plugin
variable in each test and add the cleanup immediately after Init succeeds.
🪄 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 Plus

Run ID: 00fcb12b-d9d8-4526-93f4-1c0bdd584439

📥 Commits

Reviewing files that changed from the base of the PR and between f5ae62d and 8e156c7.

⛔ Files ignored due to path filters (2)
  • docs/media/edge/edge-app-policy.png is excluded by !**/*.png
  • plugins/modelcatalogresolver/go.sum is excluded by !**/*.sum
📒 Files selected for processing (15)
  • core/internal/mcptests/plugin_test.go
  • core/mcp/exec.go
  • core/mcp/pluginpipeline.go
  • core/schemas/bifrost.go
  • core/schemas/useragents.go
  • core/schemas/useragents_test.go
  • docs/edge/admin-approvals.mdx
  • docs/edge/admin-configurations.mdx
  • docs/edge/admin-devices.mdx
  • docs/edge/deployment-mdm.mdx
  • docs/edge/supported-applications.mdx
  • plugins/logging/main.go
  • plugins/logging/operations_test.go
  • plugins/modelcatalogresolver/go.mod
  • ui/app/workspace/edge-control/inventory/page.tsx

Comment thread docs/edge/supported-applications.mdx
Comment thread plugins/logging/operations_test.go
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from 8e156c7 to 16f313e Compare June 11, 2026 14:35
Comment thread transports/bifrost-http/handlers/mcpserver.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
core/mcp/exec.go (1)

84-110: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Recompute MCP attribution after pre-hook rerouting.

Moving prepareToolExecution into the callback means PreMCPHook can now rewrite the tool target before resolution, but RunWithPluginPipeline still snapshots clientName/toolName before this closure runs. If a hook changes the tool name without also keeping preReq.ClientName in sync, the call executes against one MCP client while the response/post-hook metadata are attributed to another.

Please recompute the attributed client/tool after op(preReq) returns, or have the callback surface the resolved client name back to the pipeline. As per coding guidelines, MCP pre-hooks may mutate requests, so post-hook consumers need metadata that matches the resolved target.

🤖 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 `@core/mcp/exec.go` around lines 84 - 110, The pipeline currently snapshots
client/tool attribution before the callback, but because PreMCPHook may mutate
preReq (and you now call prepareToolExecution inside the callback), you must
recompute or return the resolved client/tool after execution so post-hook
attribution matches the actual resolved target; modify the callback used in
RunWithPluginPipeline (the closure that calls prepareToolExecution and
m.toolsManager.ExecuteTool) to either (a) return the resolved client and tool
name alongside the MCP response or (b set fields on the response/metadata with
the resolved values from prepareToolExecution (e.g.,
state.ExecutionConfig/client name or mapping) before returning so
RunWithPluginPipeline can use those values for post-hook attribution, ensuring
PreMCPHook mutations are reflected in post-hook metadata.

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.

Inline comments:
In `@transports/bifrost-http/handlers/mcpserver.go`:
- Around line 334-335: The Info-level log in the MCP tool handler is leaking
user-supplied request.GetArguments(); remove the raw arguments from the default
Info log and only log stable metadata such as toolName (e.g., change
logger.Info("[mcp-server] tool handler start tool=%q", toolName)). If you need
to inspect arguments, log them at Debug level or behind an explicit
redaction/consent flag and ensure any debug path redacts secrets before logging;
avoid printing request.GetArguments() at Info level in the handler that calls
request.GetArguments().

---

Outside diff comments:
In `@core/mcp/exec.go`:
- Around line 84-110: The pipeline currently snapshots client/tool attribution
before the callback, but because PreMCPHook may mutate preReq (and you now call
prepareToolExecution inside the callback), you must recompute or return the
resolved client/tool after execution so post-hook attribution matches the actual
resolved target; modify the callback used in RunWithPluginPipeline (the closure
that calls prepareToolExecution and m.toolsManager.ExecuteTool) to either (a)
return the resolved client and tool name alongside the MCP response or (b set
fields on the response/metadata with the resolved values from
prepareToolExecution (e.g., state.ExecutionConfig/client name or mapping) before
returning so RunWithPluginPipeline can use those values for post-hook
attribution, ensuring PreMCPHook mutations are reflected in post-hook metadata.
🪄 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 Plus

Run ID: b1cedee7-cf56-4f91-89b5-cfe38e2df3da

📥 Commits

Reviewing files that changed from the base of the PR and between 8e156c7 and 16f313e.

📒 Files selected for processing (11)
  • core/internal/mcptests/plugin_test.go
  • core/mcp/exec.go
  • core/mcp/pluginpipeline.go
  • core/schemas/bifrost.go
  • core/schemas/useragents.go
  • core/schemas/useragents_test.go
  • plugins/logging/main.go
  • plugins/logging/operations_test.go
  • transports/bifrost-http/handlers/mcpserver.go
  • transports/bifrost-http/lib/config.go
  • ui/app/workspace/edge-control/inventory/page.tsx
💤 Files with no reviewable changes (1)
  • transports/bifrost-http/lib/config.go

Comment thread transports/bifrost-http/handlers/mcpserver.go Outdated
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from 16f313e to d324fe8 Compare June 11, 2026 15:01

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
plugins/logging/operations_test.go (1)

99-128: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add plugin cleanup in the new PreLLMHook app-context tests (duplicate)

Init(...) starts background workers; these three new tests close the store but never call plugin.Cleanup(). This can leak goroutines and create cross-test flakiness.

Add cleanup after Init succeeds:

t.Cleanup(func() {
    if cleanupErr := plugin.Cleanup(); cleanupErr != nil {
        t.Errorf("Cleanup() error = %v", cleanupErr)
    }
})

This issue was already flagged in a previous review.

Also applies to: 136-168, 193-229

🤖 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 `@plugins/logging/operations_test.go` around lines 99 - 128, The tests that
call Init (e.g., TestPreLLMHookSetsAppContextFromDetectedApp) start background
workers but do not call plugin.Cleanup(), causing goroutine leaks and flakiness;
after a successful Init in each affected test add a t.Cleanup that calls
plugin.Cleanup() and reports any error (e.g., if cleanupErr := plugin.Cleanup();
cleanupErr != nil { t.Errorf("Cleanup() error = %v", cleanupErr) }) so the
plugin is always torn down when the test finishes.
🤖 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/internal/mcptests/plugin_test.go`:
- Around line 732-743: Replace the ad-hoc test initialization that calls
setupMCPManager and core.Init with the declarative mcptests harness: use
DynamicLLMMocker and the SetupAgentTest helper to build the test fixture and
register the TestShortCircuitPlugin (NewTestShortCircuitPlugin) with the MCP
plugins in the declarative config instead of passing schemas.BifrostConfig to
core.Init; ensure the test uses the shared manager provided by the harness
rather than calling SetMCPManager directly, and categorize the test using the
harness pattern for MCP cases (agent/tool/connection/codemode) per existing
mcptests conventions.

---

Duplicate comments:
In `@plugins/logging/operations_test.go`:
- Around line 99-128: The tests that call Init (e.g.,
TestPreLLMHookSetsAppContextFromDetectedApp) start background workers but do not
call plugin.Cleanup(), causing goroutine leaks and flakiness; after a successful
Init in each affected test add a t.Cleanup that calls plugin.Cleanup() and
reports any error (e.g., if cleanupErr := plugin.Cleanup(); cleanupErr != nil {
t.Errorf("Cleanup() error = %v", cleanupErr) }) so the plugin is always torn
down when the test finishes.
🪄 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 Plus

Run ID: 9ec74cc0-52dc-47d8-9772-8bf18dc9c514

📥 Commits

Reviewing files that changed from the base of the PR and between 16f313e and d324fe8.

📒 Files selected for processing (11)
  • core/internal/mcptests/plugin_test.go
  • core/mcp/exec.go
  • core/mcp/pluginpipeline.go
  • core/schemas/bifrost.go
  • core/schemas/useragents.go
  • core/schemas/useragents_test.go
  • plugins/logging/main.go
  • plugins/logging/operations_test.go
  • transports/bifrost-http/handlers/mcpserver.go
  • transports/bifrost-http/lib/config.go
  • ui/app/workspace/edge-control/inventory/page.tsx
💤 Files with no reviewable changes (1)
  • transports/bifrost-http/lib/config.go

Comment thread core/internal/mcptests/plugin_test.go
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from d324fe8 to 19abc8a Compare June 12, 2026 13:31
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch 2 times, most recently from 9556524 to 2ef33fc Compare June 13, 2026 13:17

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/useragents.go`:
- Around line 149-151: The regex branch for UserAgentMappingMatchTypeRegex
currently compiles the pattern case-sensitively; modify the regex compilation in
that branch to be case-insensitive by prepending the inline flag to the pattern
(e.g., use regexp.Compile("(?i)"+pattern) or equivalent) so that the call that
creates re and the subsequent re.MatchString(userAgent) perform a
case-insensitive match consistent with the other match types.

In `@ui/app/workspace/edge-control/inventory/page.tsx`:
- Line 1: The import of InventoryView from
"`@enterprise/components/edge-control/inventoryView`" is unresolvable in the
fallback enterprise tree; either add the missing fallback component file(s named
and cased exactly as referenced under the fallback enterprise components (create
ui/app/_fallbacks/enterprise/components/edge-control/inventoryView.[tsx|ts|jsx|js]
as appropriate) so the module exists, or change the import in page.tsx to the
correct existing module path/name (matching filename casing) that exports
InventoryView (or update the imported symbol if the export is named
differently).
🪄 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 Plus

Run ID: 70fabe7c-5c56-45c5-abe6-edef329eabc1

📥 Commits

Reviewing files that changed from the base of the PR and between 2ef33fc and 4d411a7.

📒 Files selected for processing (11)
  • core/internal/mcptests/plugin_test.go
  • core/mcp/exec.go
  • core/mcp/pluginpipeline.go
  • core/schemas/bifrost.go
  • core/schemas/useragents.go
  • core/schemas/useragents_test.go
  • plugins/logging/main.go
  • plugins/logging/operations_test.go
  • transports/bifrost-http/handlers/mcpserver.go
  • transports/bifrost-http/handlers/middlewares.go
  • ui/app/workspace/edge-control/inventory/page.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 2

🤖 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/useragents.go`:
- Around line 149-151: The regex branch for UserAgentMappingMatchTypeRegex
currently compiles the pattern case-sensitively; modify the regex compilation in
that branch to be case-insensitive by prepending the inline flag to the pattern
(e.g., use regexp.Compile("(?i)"+pattern) or equivalent) so that the call that
creates re and the subsequent re.MatchString(userAgent) perform a
case-insensitive match consistent with the other match types.

In `@ui/app/workspace/edge-control/inventory/page.tsx`:
- Line 1: The import of InventoryView from
"`@enterprise/components/edge-control/inventoryView`" is unresolvable in the
fallback enterprise tree; either add the missing fallback component file(s named
and cased exactly as referenced under the fallback enterprise components (create
ui/app/_fallbacks/enterprise/components/edge-control/inventoryView.[tsx|ts|jsx|js]
as appropriate) so the module exists, or change the import in page.tsx to the
correct existing module path/name (matching filename casing) that exports
InventoryView (or update the imported symbol if the export is named
differently).
🪄 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 Plus

Run ID: 70fabe7c-5c56-45c5-abe6-edef329eabc1

📥 Commits

Reviewing files that changed from the base of the PR and between 2ef33fc and 4d411a7.

📒 Files selected for processing (11)
  • core/internal/mcptests/plugin_test.go
  • core/mcp/exec.go
  • core/mcp/pluginpipeline.go
  • core/schemas/bifrost.go
  • core/schemas/useragents.go
  • core/schemas/useragents_test.go
  • plugins/logging/main.go
  • plugins/logging/operations_test.go
  • transports/bifrost-http/handlers/mcpserver.go
  • transports/bifrost-http/handlers/middlewares.go
  • ui/app/workspace/edge-control/inventory/page.tsx
🛑 Comments failed to post (2)
core/schemas/useragents.go (1)

149-151: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make regex matching case-insensitive to align with other match types.

Line 149–151 currently applies case-sensitive regex matching, while other match modes are case-insensitive. This creates inconsistent behavior for equivalent patterns.

Suggested fix
 	case UserAgentMappingMatchTypeRegex:
-		re, err := regexp.Compile(pattern)
-		return err == nil && re.MatchString(userAgent)
+		re, err := regexp.Compile("(?i)" + pattern)
+		return err == nil && re.MatchString(userAgent)
📝 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.

	case UserAgentMappingMatchTypeRegex:
		re, err := regexp.Compile("(?i)" + pattern)
		return err == nil && re.MatchString(userAgent)
🤖 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 `@core/schemas/useragents.go` around lines 149 - 151, The regex branch for
UserAgentMappingMatchTypeRegex currently compiles the pattern case-sensitively;
modify the regex compilation in that branch to be case-insensitive by prepending
the inline flag to the pattern (e.g., use regexp.Compile("(?i)"+pattern) or
equivalent) so that the call that creates re and the subsequent
re.MatchString(userAgent) perform a case-insensitive match consistent with the
other match types.
ui/app/workspace/edge-control/inventory/page.tsx (1)

1-1: ⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Verify enterprise component import resolution
# Check if the enterprise symlink exists and the target file is present

# 1. Check if `@enterprise` symlink/alias exists in tsconfig or vite config
echo "=== Checking tsconfig paths ==="
jq '.compilerOptions.paths["`@enterprise/`*"]' ui/tsconfig.json 2>/dev/null || echo "Not found in tsconfig"

# 2. List available enterprise components
echo "=== Checking for enterprise components directory ==="
find ui -type d -name enterprise 2>/dev/null | head -20

# 3. Check if the specific component file exists
echo "=== Checking for inventoryView component ==="
find ui -name "inventoryView*" -o -name "InventoryView*" 2>/dev/null

# 4. Check vite config for alias resolution
echo "=== Checking vite.config.ts for `@enterprise` alias ==="
grep -n "`@enterprise`" ui/vite.config.ts 2>/dev/null || echo "Not configured in vite"

# 5. Verify the symlink in ui/app/_fallbacks/enterprise (if it exists)
echo "=== Checking _fallbacks symlink ==="
ls -la ui/app/_fallbacks/ 2>/dev/null | grep enterprise || echo "Symlink not found"

Repository: maximhq/bifrost

Length of output: 452


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== ui/vite.config.ts (resolve / plugins) ==="
sed -n '1,200p' ui/vite.config.ts || true
echo "=== grep for tsconfigPaths / alias setup ==="
rg -n "tsconfig|paths|alias|`@enterprise`" ui/vite.config.ts ui/tsconfig.json ui/**/*.ts ui/**/*.tsx || true

echo "=== Search for inventoryView file/component ==="
# Find any file that mentions inventoryView in its path
find ui -type f | rg -i "inventoryview" || true
# Search code for import specifier
rg -n "`@enterprise/components/edge-control/inventoryView`" ui || true
rg -n "inventoryView" ui/app ui/components ui/lib || true

Repository: maximhq/bifrost

Length of output: 1598


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Find Vite config files ==="
ls -ლა | rg -n "vite\.config\." || true
ls -la ui | rg -n "vite\.config\." || true
find ui -maxdepth 2 -type f | rg -i "vite\.config\." || true

echo "=== Search for edge-control/inventoryView references ==="
rg -n "edge-control/inventoryView|inventoryView" ui || true
rg -n "edge-control.*inventory" ui || true

echo "=== Locate any enterprise component for edge-control inventory ==="
find ui/app/enterprise ui/app/_fallbacks/enterprise -type f | rg -i "inventory" || true

echo "=== List directories under edge-control in enterprise roots ==="
ls -la ui/app/enterprise/components/edge-control 2>/dev/null || true
ls -la ui/app/_fallbacks/enterprise/components/edge-control 2>/dev/null || true
find ui/app/_fallbacks/enterprise -maxdepth 4 -type f | rg -i "edge-control.*inventory" || true

echo "=== Show top of ui/package.json (to see build tooling/plugins) ==="
cat ui/package.json | head -n 60

Repository: maximhq/bifrost

Length of output: 3038


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== ui/vite.config.mts (alias / tsconfig paths bits) ==="
# Print a focused window around likely alias/paths/plugin config
rg -n "enterprise|tsconfig|paths|alias|resolve" ui/vite.config.mts || true
sed -n '1,220p' ui/vite.config.mts || true

echo "=== Check for expected fallback component file(s) ==="
# If tsconfig path maps `@enterprise/`* -> ./app/_fallbacks/enterprise/*
# then expected path is: ui/app/_fallbacks/enterprise/components/edge-control/inventoryView.*
find ui/app/_fallbacks/enterprise -type f | rg -i "edge-control|inventoryview|inventory-view|inventory" || true

echo "=== List edge-control under fallbacks/components ==="
ls -la ui/app/_fallbacks/enterprise 2>/dev/null || true
ls -la ui/app/_fallbacks/enterprise/components 2>/dev/null || true
ls -la ui/app/_fallbacks/enterprise/components/edge-control 2>/dev/null || true

echo "=== Exact filename matches for inventoryView ==="
find ui/app/_fallbacks/enterprise -type f -iname "inventoryview*" -o -iname "*inventoryView*" 2>/dev/null || true

Repository: maximhq/bifrost

Length of output: 5303


Fix unresolvable enterprise import for Edge inventory fallback

The import @enterprise/components/edge-control/inventoryView can’t be resolved because this build resolves @enterprise to ui/app/_fallbacks/enterprise (when ui/app/enterprise is absent), and the fallback tree contains no edge-control/inventoryView component (only the import reference exists). Either add the missing fallback component file(s) under ui/app/_fallbacks/enterprise/components/edge-control/ or correct the import to the actual existing component path/name (including filename casing).

🧰 Tools
🪛 GitHub Actions: Snyk checks / 0_Snyk Open Source (deps).txt

[error] 1-1: UI build failed (Vite/rolldown): [UNLOADABLE_DEPENDENCY] Could not load app/_fallbacks/enterprise/components/edge-control/inventoryView. Import "@enterprise/components/edge-control/inventoryView" failed: No such file or directory (os error 2).

🪛 GitHub Actions: Snyk checks / 1_Snyk Code (SAST).txt

[error] 1-1: Vite build failed: UNLOADABLE_DEPENDENCY. Could not load app/_fallbacks/enterprise/components/edge-control/inventoryView. Import "@enterprise/components/edge-control/inventoryView" could not be resolved. No such file or directory (os error 2).

🪛 GitHub Actions: Snyk checks / Snyk Code (SAST)

[error] 1-1: vite build failed: [UNLOADABLE_DEPENDENCY] Could not load app/_fallbacks/enterprise/components/edge-control/inventoryView. Missing module '@enterprise/components/edge-control/inventoryView'. (os error 2: No such file or directory)


[error] 1-1: UI build step failed: 'vite build' reported 'Build failed with 3 errors' and exited non-zero, causing make build LOCAL=1 (Makefile:306: build-ui) to fail (exit code 1/2).

🪛 GitHub Actions: Snyk checks / Snyk Open Source (deps)

[error] 1-1: Vite build failed (UNLOADABLE_DEPENDENCY): Could not load app/_fallbacks/enterprise/components/edge-control/inventoryView. No such file or directory (os error 2). Import: "@enterprise/components/edge-control/inventoryView".

🤖 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/edge-control/inventory/page.tsx` at line 1, The import of
InventoryView from "`@enterprise/components/edge-control/inventoryView`" is
unresolvable in the fallback enterprise tree; either add the missing fallback
component file(s named and cased exactly as referenced under the fallback
enterprise components (create
ui/app/_fallbacks/enterprise/components/edge-control/inventoryView.[tsx|ts|jsx|js]
as appropriate) so the module exists, or change the import in page.tsx to the
correct existing module path/name (matching filename casing) that exports
InventoryView (or update the imported symbol if the export is named
differently).

@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from 4d411a7 to f74a943 Compare June 16, 2026 19:18
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from f74a943 to 0c9a5f0 Compare June 17, 2026 08:14
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from 0c9a5f0 to 5a5dcb3 Compare June 17, 2026 08:19
Comment thread transports/bifrost-http/handlers/middlewares.go
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from 5a5dcb3 to 037c637 Compare June 17, 2026 10:19
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from 037c637 to 54162ef Compare June 17, 2026 11:09
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from 54162ef to c42aa9b Compare June 17, 2026 12:39
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from c42aa9b to 0f8dad9 Compare June 17, 2026 13:08
@akshaydeo
akshaydeo force-pushed the 06-09-plugin_and_sidebar_fixes branch from 0f8dad9 to 96edbe3 Compare June 17, 2026 13:47

akshaydeo commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Jun 17, 2:21 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 17, 2:26 PM UTC: @akshaydeo merged this pull request with Graphite.

@akshaydeo
akshaydeo changed the base branch from 06-09-agent_docs to graphite-base/4213 June 17, 2026 14:25
@akshaydeo
akshaydeo changed the base branch from graphite-base/4213 to v2.0.0 June 17, 2026 14:26
@akshaydeo
akshaydeo merged commit c89e8cf into v2.0.0 Jun 17, 2026
8 of 11 checks passed
@akshaydeo
akshaydeo deleted the 06-09-plugin_and_sidebar_fixes branch June 17, 2026 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants