Skip to content

fix: dropped instructions and content_filter status on responses->chat fallback - #5223

Open
Shaik-Sirajuddin wants to merge 4 commits into
maximhq:devfrom
Shaik-Sirajuddin:fix/responses-chat-fallback-instructions-status
Open

Shaik-Sirajuddin wants to merge 4 commits into
maximhq:devfrom
Shaik-Sirajuddin:fix/responses-chat-fallback-instructions-status

Conversation

@Shaik-Sirajuddin

Copy link
Copy Markdown
Contributor

Summary

Every provider without native Responses API support (Ollama, vLLM, DeepSeek, Groq, Mistral, and others) converts v1/responses requests to Chat Completions via ToChatRequest. That conversion silently discarded the caller's instructions field and never mapped a content_filter finish reason to an incomplete status — responses came back well-formed but semantically wrong, with no signal anything was lost.

Changes

  • Prepend instructions as a leading system message in ToChatRequest (mirrors LiteLLM's equivalent chat-completions bridge).
  • Map content_filter finish reason to status: incomplete, reusing the previously-declared-but-unused ResponsesResponseIncompleteReasonContentFilter constant — fixes both streaming and non-streaming paths since they share one mapping function.
  • Log a warning whenever a tool or tool_choice value is dropped for having no Chat Completions equivalent, via a new FallbackWarningHook (thread-safe atomic.Pointer, following the existing providerUtils.SetLogger/getLogger convention).

Type of change

  • Bug fix

How to test

cd core
go test ./schemas/... -run "TestToChatRequest|TestToBifrostResponses"
go test -race ./schemas/...

Also verified live end-to-end against a real Ollama backend (qwen3:0.6b) via the openai Python SDK — before/after request/response transcripts, server logs, and terminal screenshots are in memory/gen/responses-chat-fallback/reports/.

Breaking changes

  • No

@Shaik-Sirajuddin
Shaik-Sirajuddin requested a review from a team as a code owner July 15, 2026 04:53
@CLAassistant

CLAassistant commented Jul 15, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Responses-to-Chat fallback conversion now preserves instructions, logs dropped unsupported fields, and receives provider loggers across fallback paths. Finish-reason mapping and tests now cover content-filter incomplete responses and streams.

Changes

Responses fallback behavior

Layer / File(s) Summary
Fallback conversion and warning hooks
core/schemas/mux.go, core/schemas/mux_test.go
ToChatRequest preserves instructions as a leading system message, logs dropped tools and tool choices, and retains compatibility with existing conversion tests.
Provider fallback logger propagation
core/providers/*
Responses and ResponsesStream fallback paths pass provider loggers into ToChatRequest; the Hugging Face adapter forwards optional logger arguments.
Finish-reason mapping coverage
core/schemas/mux.go, core/schemas/mux_test.go
Max-output-tokens uses the defined constant, while content-filter results map to incomplete non-streaming responses and terminal streaming events.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ResponsesProvider
  participant ToChatRequest
  participant Logger
  participant ChatCompletion
  Client->>ResponsesProvider: submit Responses request
  ResponsesProvider->>ToChatRequest: convert with provider logger
  ToChatRequest->>Logger: warn about dropped unsupported fields
  ToChatRequest-->>ChatCompletion: return Chat request
  ChatCompletion-->>ResponsesProvider: return response or stream
  ResponsesProvider-->>Client: return Responses result
Loading

Possibly related PRs

Suggested reviewers: akshaydeo, pratham-mishra04

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title matches the main change: fixing dropped instructions and content_filter handling in responses→chat fallback.
Description check ✅ Passed The description covers summary, changes, type of change, testing, and breaking changes, though several optional template sections are omitted.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

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

```sh
# 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 maximhq#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
@Shaik-Sirajuddin
Shaik-Sirajuddin force-pushed the fix/responses-chat-fallback-instructions-status branch from c49b7cb to 973fc3a Compare July 15, 2026 05:12
@coderabbitai
coderabbitai Bot requested a review from akshaydeo July 15, 2026 05:13
@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

This looks safe to merge.

  • The instance logger is passed through all changed unary and streaming fallback paths.
  • The previous cross-instance warning attribution problem is no longer reachable.
  • No blocking issues were found in the updated code.

Important Files Changed

Filename Overview
core/schemas/mux.go Preserves instructions, maps filtered responses to incomplete status, and emits fallback warnings through a caller-supplied logger.
core/providers/huggingface/responses.go Forwards the optional logger while preserving the existing exported call shape.
core/providers/openai/openai.go Passes the provider instance logger through unary and streaming fallback conversions.
core/schemas/mux_test.go Adds coverage for instruction preservation and content-filter status mapping.

Reviews (3): Last reviewed commit: "fix: dropped instructions and content_fi..." | Re-trigger Greptile

Comment thread core/bifrost.go Outdated
@Shaik-Sirajuddin
Shaik-Sirajuddin force-pushed the fix/responses-chat-fallback-instructions-status branch from 973fc3a to a8811dc Compare July 15, 2026 05:16
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 15, 2026
…t fallback

Every provider without native Responses API support (Ollama, vLLM,
DeepSeek, Groq, Mistral, and others) converts v1/responses requests to
Chat Completions via ToChatRequest. That conversion silently discarded
the caller's `instructions` field and never mapped a `content_filter`
finish reason to an `incomplete` status, so responses came back
well-formed but semantically wrong with no signal that anything was
lost.

- Prepend `instructions` as a leading system message in ToChatRequest,
  mirroring LiteLLM's equivalent chat-completions bridge.
- Map `content_filter` finish reason to `status: incomplete`, reusing
  the previously-declared-but-unused
  ResponsesResponseIncompleteReasonContentFilter constant; fixes both
  streaming and non-streaming paths since they share one mapping
  function.
- Log a warning whenever a tool or tool_choice value is dropped for
  having no Chat Completions equivalent, via each provider's own
  instance-bound logger (not a process-global — avoids cross-instance/
  tenant log attribution bugs when multiple Bifrost instances run with
  different loggers).
- ToChatRequest's new logger parameter is variadic and optional, so
  the exported zero-argument call shape still compiles for existing
  Go SDK consumers.

Verified live against a real Ollama backend; before/after evidence in
memory/gen/responses-chat-fallback/reports/.
@Shaik-Sirajuddin
Shaik-Sirajuddin force-pushed the fix/responses-chat-fallback-instructions-status branch from a8811dc to 4ae75d4 Compare July 15, 2026 05:32
@Shaik-Sirajuddin Shaik-Sirajuddin changed the title fix: stop dropping instructions and content_filter status on responses->chat fallback fix: dropped instructions and content_filter status on responses->chat fallback Jul 15, 2026
@coderabbitai
coderabbitai Bot requested a review from Pratham-Mishra04 July 15, 2026 05:33
@Shaik-Sirajuddin

Copy link
Copy Markdown
Contributor Author

Before/after evidence for the Greptile P1 fix

Addressing the review comment with logs, not just a claim.

Before (commit 973fc3a2c) — the bug

$ grep -n "FallbackWarningHook\|fallbackWarningHook" core/schemas/mux.go core/bifrost.go
schemas/mux.go:1288:var fallbackWarningHook atomic.Pointer[func(msg string, args ...any)]
schemas/mux.go:1291:func SetFallbackWarningHook(hook func(msg string, args ...any)) {
core/bifrost.go:226:	schemas.SetFallbackWarningHook(config.Logger.Warn)

A single package-level atomic.Pointer set on every Init() call — the second Bifrost instance's Init() silently overwrites the first instance's routing, exactly as described in the review comment.

After (commit 4ae75d493) — fixed

$ grep -n "FallbackWarningHook\|fallbackWarningHook" core/schemas/mux.go core/bifrost.go
(no matches)

$ grep -rn "ToChatRequest(provider.logger)\|ToChatRequest(p.logger)" core/providers | wc -l
26

The global hook is gone. All 26 call sites across the 15 chat-fallback providers now pass that provider's own logger field (bound at construction time, one per Bifrost instance) directly into ToChatRequest(logger ...Logger) — variadic so the exported zero-argument call shape still compiles for existing Go SDK consumers.

Build/vet/test — both commits

go build ./...   # clean, both commits
go vet ./...     # clean, both commits
go test ./schemas/... ./providers/...
  # both commits: all packages pass except the same pre-existing
  # TestToOpenAIResponsesRequest_GPTOSS_SummaryToContentBlocks failure,
  # confirmed unrelated to this change (reproduces on a clean checkout
  # with none of this branch's changes applied).

Full logs (build/vet/test output + the exact grep runs above) captured for both commits — happy to paste in full if useful, trimmed here for readability.

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.

4 participants