Skip to content

Refactor Bifrost for plugin support and multi-provider architecture along with tests#1

Closed
danpiths wants to merge 1 commit intomainfrom
feature/03-21-base_structure_fixes
Closed

Refactor Bifrost for plugin support and multi-provider architecture along with tests#1
danpiths wants to merge 1 commit intomainfrom
feature/03-21-base_structure_fixes

Conversation

@danpiths
Copy link
Copy Markdown
Collaborator

@danpiths danpiths commented Mar 21, 2025 •
edited
Loading

No description provided.

@danpiths danpiths requested a review from akshaydeo March 21, 2025 04:20
@danpiths Graphite App
Copy link
Copy Markdown
Collaborator Author

danpiths commented Mar 21, 2025 •
edited by Pratham-Mishra04
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Pratham-Mishra04 Pratham-Mishra04 force-pushed the feature/03-21-base_structure_fixes branch from 50ad314 to 40ce794 Compare March 21, 2025 06:28
This was referenced Mar 21, 2025
Closed
Closed
danpiths
danpiths commented Mar 22, 2025
View reviewed changes
Comment thread interfaces/provider.go Outdated
danpiths
danpiths commented Mar 22, 2025
View reviewed changes
Comment thread interfaces/provider.go Outdated
danpiths
danpiths commented Mar 22, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator Author

@danpiths danpiths left a comment

Choose a reason for hiding this comment

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

why did we remove omitempty for all optional fields @Pratham-Mishra04 ?

danpiths
danpiths commented Mar 22, 2025
View reviewed changes
Comment thread providers/anthropic.go Outdated
@danpiths Graphite App
Copy link
Copy Markdown
Collaborator Author

danpiths commented Mar 22, 2025

also can we add proper descriptions with each PR that lists the changes made and why did we do it

@Pratham-Mishra04 Pratham-Mishra04 mentioned this pull request Mar 22, 2025
Closed
This was referenced Mar 31, 2025
Closed
Closed
Closed
Closed
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the feature/03-21-base_structure_fixes branch from 40ce794 to a54ac5d Compare March 31, 2025 07:57
@Pratham-Mishra04 Pratham-Mishra04 changed the title base structure fixes Refactor Bifrost for plugin support and multi-provider architecture along with tests Mar 31, 2025
@Pratham-Mishra04 Pratham-Mishra04 mentioned this pull request Mar 31, 2025
Closed
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread interfaces/provider.go Outdated
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread interfaces/provider.go Outdated
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread interfaces/plugin.go Outdated
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread interfaces/plugin.go Outdated
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread interfaces/provider.go Outdated
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread interfaces/provider.go Outdated
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread bifrost.go Outdated
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread bifrost.go Outdated
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread bifrost.go
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread bifrost.go Outdated
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread bifrost.go Outdated
danpiths
danpiths commented Mar 31, 2025
View reviewed changes
Comment thread interfaces/account.go Outdated
@Pratham-Mishra04 Pratham-Mishra04 mentioned this pull request Apr 1, 2025
Closed
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the feature/03-21-base_structure_fixes branch from 4f06678 to b864e1d Compare April 1, 2025 11:00
@Pratham-Mishra04 Pratham-Mishra04 mentioned this pull request Apr 2, 2025
Closed
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the feature/03-21-base_structure_fixes branch 3 times, most recently from 83c42c0 to ee2cc44 Compare April 6, 2025 06:26
This was referenced Apr 6, 2025
Closed
Closed
danpiths
danpiths commented Apr 7, 2025
View reviewed changes
Comment thread interfaces/provider.go Outdated
@Pratham-Mishra04 Pratham-Mishra04 force-pushed the feature/03-21-base_structure_fixes branch from ee2cc44 to f60e9be Compare April 7, 2025 04:08
This was referenced Apr 9, 2025
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@akshaydeo akshaydeo deleted the feature/03-21-base_structure_fixes branch April 15, 2025 05:36
rohhann12 added a commit to rohhann12/bifrost that referenced this pull request Sep 8, 2025
@rohhann12
fix coderabbit issue maximhq#1-Align JSON field with UI
10f0cca
@coderabbitai coderabbitai Bot mentioned this pull request Oct 23, 2025
Merged
15 tasks
@coderabbitai coderabbitai Bot mentioned this pull request Jan 8, 2026
Merged
thiscantbeserious added a commit to thiscantbeserious/bifrost that referenced this pull request Apr 17, 2026
@thiscantbeserious @claude
review: address 2nd-round CodeRabbit + Greptile findings
3c0217a 
CodeRabbit round 2:

10. /models query-param fallback now checks for existing client_version
    before injecting. Preserves caller-supplied params like foo=bar and
    picks up caller's own client_version when present. Uses simple
    string split (no net/url allocation) since paths are well-formed.

11. chatGPTOAuthWebSocketHeaders now routes through
    mergeHeadersCaseInsensitive so caller-supplied headers with
    different casing (OPENAI-BETA, Openai-Beta, etc.) can't cause
    duplicate-case headers reaching the wire — OAuth values and the
    Authorization bearer always win deterministically.

Greptile findings:

P1 maximhq#1 Non-streaming Responses() path: return a clear BifrostOperationError
      pointing callers to ResponsesStream when chatgpt_oauth is on.
      The ChatGPT backend only accepts stream=true (matching the
      openai-oauth reference proxy behavior), so non-streaming cannot
      work via Bifrost without server-side SSE aggregation which is
      out of scope for this PR. In practice Codex always streams.

P1 maximhq#2 /models response format: OpenAIListModelsResponse now has a
      UnmarshalJSON that accepts BOTH the standard OpenAI shape
      ({"data":[{id,...}]}) and the ChatGPT backend shape
      ({"models":[{slug}]}). ChatGPT entries are projected into
      OpenAIModel with ID=slug, Object="model", OwnedBy="chatgpt-oauth".

P2 maximhq#3 client_version doc comment: corrected — the fallback only
      applies when the inbound path reaches chatGPTOAuthPath without
      a caller-supplied client_version (previously the doc implied
      forwarding logic that didn't exist; it now does).

P2 maximhq#4 store doc comment: corrected from "sets store to false if not
      already present" to "forces store to false" to match the code
      and explanation.

P2 maximhq#5 Double JWT decode per Responses/ResponsesStream: fixed by
      passing raw networkConfig.ExtraHeaders to chatGPTOAuthApplyRequest
      instead of the already-merged effectiveExtraHeaders(key). The
      merge now happens exactly once per call.

Test coverage: 100% on every function in chatgpt_oauth.go. Added tests
for query-param preservation, models dual-shape parsing, and the
streaming-only sentinel error.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@thiscantbeserious thiscantbeserious mentioned this pull request Apr 23, 2026
Draft
22 tasks
thiscantbeserious added a commit to thiscantbeserious/bifrost that referenced this pull request Apr 24, 2026
@thiscantbeserious @claude
review: address 2nd-round CodeRabbit + Greptile findings
e66af1c 
CodeRabbit round 2:

10. /models query-param fallback now checks for existing client_version
    before injecting. Preserves caller-supplied params like foo=bar and
    picks up caller's own client_version when present. Uses simple
    string split (no net/url allocation) since paths are well-formed.

11. chatGPTOAuthWebSocketHeaders now routes through
    mergeHeadersCaseInsensitive so caller-supplied headers with
    different casing (OPENAI-BETA, Openai-Beta, etc.) can't cause
    duplicate-case headers reaching the wire — OAuth values and the
    Authorization bearer always win deterministically.

Greptile findings:

P1 maximhq#1 Non-streaming Responses() path: return a clear BifrostOperationError
      pointing callers to ResponsesStream when chatgpt_oauth is on.
      The ChatGPT backend only accepts stream=true (matching the
      openai-oauth reference proxy behavior), so non-streaming cannot
      work via Bifrost without server-side SSE aggregation which is
      out of scope for this PR. In practice Codex always streams.

P1 maximhq#2 /models response format: OpenAIListModelsResponse now has a
      UnmarshalJSON that accepts BOTH the standard OpenAI shape
      ({"data":[{id,...}]}) and the ChatGPT backend shape
      ({"models":[{slug}]}). ChatGPT entries are projected into
      OpenAIModel with ID=slug, Object="model", OwnedBy="chatgpt-oauth".

P2 maximhq#3 client_version doc comment: corrected — the fallback only
      applies when the inbound path reaches chatGPTOAuthPath without
      a caller-supplied client_version (previously the doc implied
      forwarding logic that didn't exist; it now does).

P2 maximhq#4 store doc comment: corrected from "sets store to false if not
      already present" to "forces store to false" to match the code
      and explanation.

P2 maximhq#5 Double JWT decode per Responses/ResponsesStream: fixed by
      passing raw networkConfig.ExtraHeaders to chatGPTOAuthApplyRequest
      instead of the already-merged effectiveExtraHeaders(key). The
      merge now happens exactly once per call.

Test coverage: 100% on every function in chatgpt_oauth.go. Added tests
for query-param preservation, models dual-shape parsing, and the
streaming-only sentinel error.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@amarcin amarcin mentioned this pull request May 2, 2026
Open
3 tasks
@ryan-orphic ryan-orphic mentioned this pull request May 4, 2026
Merged
16 tasks
ryan-orphic added a commit to Orphic-AI/bifrost that referenced this pull request May 4, 2026
@ryan-orphic
fix(bedrock): omit toolChoice.tool on Llama for synthetic structured-…
a49a95f 
…output tool

Bedrock Converse rejects toolConfig.toolChoice.tool on Meta Llama variants
with HTTP 400 ("This model doesn't support the toolConfig.toolChoice.tool
field. Remove toolConfig.toolChoice.tool and try again."). The synthetic
bf_so_* tool path used for structured output unconditionally pinned the
synthetic tool via toolChoice.tool, which broke every with_structured_output
caller against Llama 4 Maverick / Scout / Llama 3.x on Bedrock.

This patch adds an IsLlamaModel helper next to the existing IsNovaModel /
IsAnthropicModel family and gates the forced-tool emission off it in four
places (covering both API surfaces and both the synthetic and explicit
tool_choice paths on each):

  1. core/providers/bedrock/utils.go: ChatCompletions synthetic-tool injection
     (response_format=json_schema -> bf_so_* tool path).
  2. core/providers/bedrock/responses.go: OpenAI Responses API mirror of maximhq#1
     (text.format=json_schema -> bf_so_* tool path).
  3. core/providers/bedrock/utils.go: convertToolConfigFromFiltered's explicit
     tool_choice path (defense-in-depth for ChatCompletions callers using
     bind_tools(tool_choice={"type": "function", "function": {"name": "X"}})
     directly).
  4. core/providers/bedrock/responses.go: Responses API mirror of maximhq#3 — the
     explicit tool_choice path runs convertResponsesToolChoice and writes
     BedrockToolChoice{Tool: ...} straight onto the request, which trips the
     same Llama 400 without the gate.

With one synthetic tool bound and Bedrock's default "auto" behavior, omitting
tool_choice yields the same outcome on Llama because there's exactly one tool
the model can call. This mirrors the gate langchain-aws ships
(supports_tool_choice_values for llama4 / llama3-1 / llama3-3 sets only
"auto", causing with_structured_output to emit no tool_choice at all).

Tests added (mirroring PR maximhq#3184's TestBedrockAnthropicChatStructuredOutputUsesSyntheticTool
naming and structure):

  - TestBedrockLlamaChatStructuredOutputOmitsForcedToolChoice — synthetic tool
    present, ToolChoice nil for Llama on the ChatCompletions path.
  - TestBedrockNonLlamaChatStructuredOutputForcesToolChoice — regression guard:
    Nova / Anthropic still get the forced tool_choice (gate doesn't over-fire).
  - TestToBedrockResponsesRequest_LlamaStructuredOutputOmitsForcedToolChoice —
    same negative+positive on the Responses API synthetic-tool path.
  - TestBedrockLlamaConvertToolConfigOmitsForcedToolChoice — defense-in-depth
    coverage for ChatCompletions explicit tool_choice struct callers.
  - TestToBedrockResponsesRequest_LlamaConvertResponsesToolChoiceOmitsForcedToolChoice
    — defense-in-depth coverage for Responses API explicit tool_choice
    (function-typed) struct callers — the gap surfaced in code review.
  - TestToBedrockResponsesRequest_NonLlamaConvertResponsesToolChoiceForcesToolChoice
    — Anthropic regression guard for maximhq#5 so the Responses-side Llama gate
    doesn't over-fire.

Refs:
  - https://docs.aws.amazon.com/bedrock/latest/APIReference/API_runtime_ToolChoice.html
  - https://github.com/langchain-ai/langchain-aws (bedrock_converse.py
    supports_tool_choice_values family-detection lines 678-711).
  - PR maximhq#3184 (Anthropic structured-output fallback to synthetic tool path) —
    immediate predecessor; this PR completes the per-family gating story.
akshaydeo pushed a commit that referenced this pull request May 4, 2026
@ryan-orphic
fix(bedrock): omit toolChoice.tool on Llama for synthetic structured-…
7eb2631 
…output tool (#3196)

Bedrock Converse rejects toolConfig.toolChoice.tool on Meta Llama variants
with HTTP 400 ("This model doesn't support the toolConfig.toolChoice.tool
field. Remove toolConfig.toolChoice.tool and try again."). The synthetic
bf_so_* tool path used for structured output unconditionally pinned the
synthetic tool via toolChoice.tool, which broke every with_structured_output
caller against Llama 4 Maverick / Scout / Llama 3.x on Bedrock.

This patch adds an IsLlamaModel helper next to the existing IsNovaModel /
IsAnthropicModel family and gates the forced-tool emission off it in four
places (covering both API surfaces and both the synthetic and explicit
tool_choice paths on each):

  1. core/providers/bedrock/utils.go: ChatCompletions synthetic-tool injection
     (response_format=json_schema -> bf_so_* tool path).
  2. core/providers/bedrock/responses.go: OpenAI Responses API mirror of #1
     (text.format=json_schema -> bf_so_* tool path).
  3. core/providers/bedrock/utils.go: convertToolConfigFromFiltered's explicit
     tool_choice path (defense-in-depth for ChatCompletions callers using
     bind_tools(tool_choice={"type": "function", "function": {"name": "X"}})
     directly).
  4. core/providers/bedrock/responses.go: Responses API mirror of #3 — the
     explicit tool_choice path runs convertResponsesToolChoice and writes
     BedrockToolChoice{Tool: ...} straight onto the request, which trips the
     same Llama 400 without the gate.

With one synthetic tool bound and Bedrock's default "auto" behavior, omitting
tool_choice yields the same outcome on Llama because there's exactly one tool
the model can call. This mirrors the gate langchain-aws ships
(supports_tool_choice_values for llama4 / llama3-1 / llama3-3 sets only
"auto", causing with_structured_output to emit no tool_choice at all).

Tests added (mirroring PR #3184's TestBedrockAnthropicChatStructuredOutputUsesSyntheticTool
naming and structure):

  - TestBedrockLlamaChatStructuredOutputOmitsForcedToolChoice — synthetic tool
    present, ToolChoice nil for Llama on the ChatCompletions path.
  - TestBedrockNonLlamaChatStructuredOutputForcesToolChoice — regression guard:
    Nova / Anthropic still get the forced tool_choice (gate doesn't over-fire).
  - TestToBedrockResponsesRequest_LlamaStructuredOutputOmitsForcedToolChoice —
    same negative+positive on the Responses API synthetic-tool path.
  - TestBedrockLlamaConvertToolConfigOmitsForcedToolChoice — defense-in-depth
    coverage for ChatCompletions explicit tool_choice struct callers.
  - TestToBedrockResponsesRequest_LlamaConvertResponsesToolChoiceOmitsForcedToolChoice
    — defense-in-depth coverage for Responses API explicit tool_choice
    (function-typed) struct callers — the gap surfaced in code review.
  - TestToBedrockResponsesRequest_NonLlamaConvertResponsesToolChoiceForcesToolChoice
    — Anthropic regression guard for #5 so the Responses-side Llama gate
    doesn't over-fire.

Refs:
  - https://docs.aws.amazon.com/bedrock/latest/APIReference/API_runtime_ToolChoice.html
  - https://github.com/langchain-ai/langchain-aws (bedrock_converse.py
    supports_tool_choice_values family-detection lines 678-711).
  - PR #3184 (Anthropic structured-output fallback to synthetic tool path) —
    immediate predecessor; this PR completes the per-family gating story.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Pratham-Mishra04 Pratham-Mishra04 Pratham-Mishra04 left review comments

@akshaydeo akshaydeo Awaiting requested review from akshaydeo

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@danpiths @Pratham-Mishra04