Skip to content

refactor: move virtual key validation and context attachment before passthrough check#3213

Draft
Pratham-Mishra04 wants to merge 1 commit intomainfrom
04-30-fix_passthrough_detection_for_routing_fixes
Draft

refactor: move virtual key validation and context attachment before passthrough check#3213
Pratham-Mishra04 wants to merge 1 commit intomainfrom
04-30-fix_passthrough_detection_for_routing_fixes

Conversation

@Pratham-Mishra04
Copy link
Copy Markdown
Collaborator

Summary

Virtual key validation and team/customer context attachment now occur before the passthrough path check, ensuring that metadata is correctly populated on the context even for passthrough requests that use a virtual key.

Changes

  • Moved virtual key lookup and validation (including the inactive key early-return) to execute before the passthrough path check, so that team and customer IDs/names are attached to the context regardless of whether the request is a passthrough.
  • The payload, virtualKey, ok, and needsMarshal variable declarations were relocated accordingly to maintain correct scoping after the reorder.
  • Previously, passthrough requests with a valid virtual key would skip team/customer context attachment entirely, which could cause missing metadata downstream.

Type of change

  • Bug fix

Affected areas

  • Plugins

How to test

  1. Configure a virtual key with an associated team and/or customer.
  2. Send a request to a passthrough endpoint using that virtual key.
  3. Verify that BifrostContextKeyGovernanceTeamID, BifrostContextKeyGovernanceTeamName, BifrostContextKeyGovernanceCustomerID, and BifrostContextKeyGovernanceCustomerName are correctly set on the context after the pre-hook runs.
  4. Verify that requests with no virtual key and no routing rules still return early without processing.
  5. Verify that requests using an inactive virtual key still return early.
go test ./plugins/governance/...

Breaking changes

  • No

Security considerations

No new auth mechanisms introduced. The existing virtual key active/inactive check is preserved and now runs earlier in the flow, which is strictly more restrictive for passthrough paths.

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

@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.

Copy link
Copy Markdown
Collaborator Author

Pratham-Mishra04 commented May 4, 2026

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 4, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 99b976cc-8559-4de0-ab90-d9a55e744851

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Refactors HTTPTransportPreHook to optimize payload processing with early-exit paths when governance rules are absent, defers unmarshalling, adds content-type parsing, and introduces a dedicated governLargePayload function for read-only large requests.

Changes

HTTPTransportPreHook Optimization & Large-Payload Routing

Layer / File(s) Summary
Early-Exit Gating
plugins/governance/main.go
HTTPTransportPreHook now short-circuits when no virtual key and no routing rules exist, avoiding unnecessary unmarshalling and processing for non-governed paths.
VK Processing & Context Attachment
plugins/governance/main.go
Virtual key is fetched, activity validated, and governance metadata (team ID/name, customer ID/name) attached to the Bifrost context when VK is present.
Content-Type Parsing
plugins/governance/main.go
Content-Type header is parsed and normalized to determine payload structure (multipart/form-data or application/json), with conditional marshalling/unmarshalling based on type.
Routing & Load-Balancing Integration
plugins/governance/main.go
Routing rules are evaluated and load-balancing applied after VK processing; MCP tool-injection integrated behind routing-availability checks.
Large-Payload Handler
plugins/governance/main.go
New governLargePayload function introduced to handle read-only large payloads by constructing synthetic payloads from pre-extracted metadata and applying governance without body rewriting.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

A rabbit hops through governance with glee,
Short-circuiting paths with efficiency,
Large payloads glide through new workaround streams,
While Content-Types parse all the dream schemes.
Early exits dance, unmarshalling waits—
This refactor optimizes the gates! 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main refactoring change: moving virtual key validation and context attachment before the passthrough check, which directly aligns with the core objective of the pull request.
Description check ✅ Passed The pull request description includes all required sections from the template with substantial content: summary explaining the bug fix, detailed changes, type of change marked, affected areas selected, testing instructions provided, breaking changes addressed, and security considerations documented.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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 04-30-fix_passthrough_detection_for_routing_fixes

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

@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 04-30-fix_passthrough_detection_for_routing_fixes branch from b06fc15 to 16bfd46 Compare May 5, 2026 12:20
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 05-04-fix_mcp_sse_hang_fixes branch 2 times, most recently from 0132257 to e3d6e25 Compare May 5, 2026 15:29
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 04-30-fix_passthrough_detection_for_routing_fixes branch 2 times, most recently from 064ea8b to f15baf7 Compare May 6, 2026 07:32
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 05-04-fix_mcp_sse_hang_fixes branch from e3d6e25 to 3dcc042 Compare May 6, 2026 07:32
@akshaydeo akshaydeo marked this pull request as ready for review May 6, 2026 07:34
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 6, 2026

Confidence Score: 4/5

Safe to merge; the reordering correctly fixes passthrough metadata gaps with no change to the non-passthrough code path.

The core reordering is correct and the existing inactive-key guard is preserved. governLargePayload now does a second store lookup for a key already resolved upstream, leaving a small staleness window if the key is deactivated mid-request.

plugins/governance/main.go — specifically the governLargePayload call site and function, which now redundantly re-validates a key already checked upstream.

Important Files Changed

Filename Overview
plugins/governance/main.go Virtual key validation and team/customer context attachment moved before the passthrough check; governLargePayload now performs a redundant VK store lookup since the key was already resolved upstream.

Reviews (1): Last reviewed commit: "fix: passthrough detection for routing f..." | Re-trigger Greptile

@@ -364,12 +394,6 @@ func (p *GovernancePlugin) HTTPTransportPreHook(ctx *schemas.BifrostContext, req
return p.governLargePayload(ctx, req, virtualKeyValue, hasRoutingRules)
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.

P2 Redundant VK lookup in governLargePayload

HTTPTransportPreHook now validates the virtual key and attaches team/customer context before reaching this call site, but governLargePayload re-runs the same p.store.GetVirtualKey lookup and re-sets the identical context values unconditionally. Because governLargePayload is only ever called from this path, both the store roundtrip and the ctx.SetValue calls are now no-ops in the happy path. If the key happens to be deactivated between the two lookups, the context will already carry stale team/customer metadata from the first lookup even though governLargePayload returns early without applying governance — a subtle inconsistency. Consider passing the already-resolved *configstoreTables.TableVirtualKey directly to governLargePayload to eliminate the double lookup and the staleness window.

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 6, 2026
@Pratham-Mishra04 Pratham-Mishra04 marked this pull request as draft May 6, 2026 07:45
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 05-04-fix_mcp_sse_hang_fixes branch from 3dcc042 to e2bc0b5 Compare May 6, 2026 07:53
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 04-30-fix_passthrough_detection_for_routing_fixes branch from f15baf7 to 7b58229 Compare May 6, 2026 07:53
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 05-04-fix_mcp_sse_hang_fixes branch from e2bc0b5 to 83ff8a4 Compare May 6, 2026 08:11
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the 04-30-fix_passthrough_detection_for_routing_fixes branch from 7b58229 to ac0cefd Compare May 6, 2026 08:11
@akshaydeo akshaydeo changed the base branch from 05-04-fix_mcp_sse_hang_fixes to graphite-base/3213 May 6, 2026 08:43
@akshaydeo akshaydeo force-pushed the graphite-base/3213 branch from 83ff8a4 to beff6d0 Compare May 6, 2026 08:44
@akshaydeo akshaydeo force-pushed the 04-30-fix_passthrough_detection_for_routing_fixes branch from ac0cefd to 3f34ef9 Compare May 6, 2026 08:44
@graphite-app graphite-app Bot changed the base branch from graphite-base/3213 to main May 6, 2026 08:44
@graphite-app graphite-app Bot dismissed coderabbitai[bot]’s stale review May 6, 2026 08:44

The base branch was changed.

@akshaydeo akshaydeo force-pushed the 04-30-fix_passthrough_detection_for_routing_fixes branch from 3f34ef9 to 6ba4e35 Compare May 6, 2026 08:44
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