Skip to content

ai: split OpenAI Responses and compatible providers - #475

Open
martinhoyer wants to merge 3 commits into
sashiko-dev:mainfrom
martinhoyer:openai-responses-provider
Open

martinhoyer wants to merge 3 commits into
sashiko-dev:mainfrom
martinhoyer:openai-responses-provider

Conversation

@martinhoyer

Copy link
Copy Markdown

Summary

Split the existing OpenAI integration into two distinct providers:

  • openai uses OpenAI’s native /v1/responses endpoint.
  • openai-compatible retains the Chat Completions implementation.

Motivation

Using OpenAI API with gpt-5.6 models currently fails on:

{
  "error": {
    "message": "Unsupported value: 'temperature' does not support 0.0 with this model. Only the default (1) value is supported.",
    "type": "invalid_request_error",
    "param": "temperature",
    "code": "unsupported_value"
  }
}

Shoehorning special temperature handling into the existing compatible provider did not solve the underlying problem. Modern OpenAI reasoning and tool-calling workflows ultimately need the Responses API, while third-party OpenAI-compatible services generally continue to expose Chat Completions.
Splitting openai and openai-compatible therefore seemed like the most practical and maintainable approach.

Changes

  • Add a dedicated OpenAI Responses API client.
  • Add [ai.openai] configuration, separate from [ai.openai_compat].
  • Clearly reject legacy-only provider = "openai" configurations with migration guidance.
  • Preserve and replay complete Responses output items, including encrypted reasoning state and original function-call identifiers.
  • Classify retryable OpenAI errors correctly.
  • Handle JSON mode requirements and cached-input token accounting.
  • Update settings, examples, provider documentation, and design documentation.

Not sure if there should be separate design doc as well.

@rgushchin

Copy link
Copy Markdown
Member

Note

This review was generated with the assistance of an AI tool.

Overall looks great and well-architected! A few quick observations and potential regression risks:

  1. Default max_tokens (4,096) may truncate reasoning:
    In src/ai/mod.rs, omitting [ai.openai].max_tokens defaults to 4,096. On the Responses API, max_output_tokens bounds the sum of internal reasoning tokens and visible output. Reasoning models like gpt-5.6 with medium effort can easily exhaust 4,096 tokens during the reasoning phase alone, causing premature incomplete (truncated) responses. Consider defaulting to 65536 for the openai provider.

  2. Handle status: "failed" in translate_ai_response:
    translate_ai_response checks resp.status == "incomplete", but if OpenAI returns a 200 OK with status: "failed" and an error object, it currently returns Ok(AiResponse) with empty content and tool calls. Propagating the error message directly would prevent confusing downstream ValidationErrors.

  3. Global cache invalidation:
    Bumping CACHE_KEY_VERSION in src/ai/cache.rs to "sashiko-ai-cache-v2" invalidates cached responses for all providers (Gemini, Claude, Bedrock, Ollama, etc.). Users running benchmarks or workloads with response_cache = true will experience a full cold-cache restart across all backends.

  4. base_url doc & normalization:
    docs/configuration.md lists [ai.openai].base_url as "model-derived", but default_base_url() statically points to https://api.openai.com/v1/responses. Also, OpenAiClient does not normalize paths (e.g. appending /responses), so custom proxy URLs like http://host/v1 without /responses will fail.

  5. temperature suppression:
    translate_ai_request unconditionally passes temperature: None. This fixes gpt-5.6 rejecting 0.0, but prevents tuning temperature for non-reasoning models (like gpt-4o) or future models that support it on /v1/responses.

@martinhoyer

Copy link
Copy Markdown
Author

Consider defaulting to 65536 for the openai provider.

Thanks, already noticed 4096 is too low. Trying 16k and it was quite enough for sol with xhigh reasoning for a patch review. Wouldn't 65k be an overkill?

@martinhoyer

Copy link
Copy Markdown
Author

@rgushchin adressing feedback in fixup commits, thanks.

  • Raised the default output budget from 4K to 16K, validated with Sol/xhigh, and now log incomplete reasons and token usage.
  • Surface failed Responses as provider errors instead of empty responses.
  • Scoped cache invalidation to OpenAI Responses only.
  • Corrected base_url documentation and added /responses normalization.
  • Forward temperature for supported models while omitting it for known reasoning models.

@rgushchin

Copy link
Copy Markdown
Member

Can you please rebase it and also squash fixups into corresponding commits? Thanks

@martinhoyer
martinhoyer force-pushed the openai-responses-provider branch from c7b1248 to 2657135 Compare September 9, 2026 08:11
@martinhoyer

Copy link
Copy Markdown
Author

Can you please rebase it and also squash fixups into corresponding commits? Thanks

@rgushchin Done 🫡

@martinhoyer
martinhoyer force-pushed the openai-responses-provider branch from 2657135 to bfb30a8 Compare September 17, 2026 08:07
@martinhoyer

Copy link
Copy Markdown
Author

@rgushchin Rebased once more. Please let me know if you want me to keep it in sync with main or I shouldn't bother.

@rgushchin

Copy link
Copy Markdown
Member

Sorry, was busy with merging some other stuff. Can you, please, check some findings here?
https://sashiko.sashiko.dev/#/patchset/sashiko-475

@martinhoyer
martinhoyer force-pushed the openai-responses-provider branch from bfb30a8 to 9f0dfae Compare September 18, 2026 18:40
@martinhoyer

Copy link
Copy Markdown
Author

Sorry, was busy with merging some other stuff. Can you, please, check some findings here? https://sashiko.sashiko.dev/#/patchset/sashiko-475

Thanks, addressed and rebased. I'm not sure about the "Missing validation in commit message" - did I miss some commit message requirements other than being signed?

@sashiko-bot

sashiko-bot Bot commented Sep 18, 2026

Copy link
Copy Markdown

Sashiko review — v2

Commit 2/3 — 99352b99 ai: add OpenAI Responses API provider and wire into provider factory

  • [MEDIUM] In src/ai/openai_responses.rs (OpenAiClient::post_request),
    transport errors are formatted with e.to_string() instead of {:#}.
    This strips the underlying cause of a transport failure, such as DNS
    resolution versus TLS handshake failure, from the error string. This
    degrades observability and violates the error formatting invariant for AI
    providers, which requires {:#} so the reqwest source chain survives.

Commit 3/3 — 9f0dfae8 docs: update design doc and provider guide for Responses API split

  • [LOW] In designs/DESIGN_OPENAI_COMPAT_PROVIDER.md (Request Translation:
    AiRequest to ResponsesRequest), the documentation claims that the explicit
    JSON instruction is injected conditionally if no existing input contains
    JSON. This incorrectly describes the legacy chat completions behavior.
    The actual implementation in src/ai/openai_responses.rs injects it
    unconditionally as a leading system message to preserve prompt cache
    stability between turns.

Full review and stage logs on sashiko.sashiko.dev

Add a dedicated [ai.openai] settings table for the official OpenAI
Responses API, separate from [ai.openai_compat], which remains tied to
Chat Completions compatible services.

Expose optional base URL, context window, output token limit, and
reasoning effort settings while allowing the provider to select
model-specific defaults when values are omitted.

Signed-off-by: Martin Hoyer <mhoyer@redhat.com>
Introduce a dedicated client for OpenAI's /v1/responses endpoint and
select it for provider = "openai", while keeping the existing Chat
Completions client behind provider = "openai-compatible".

Preserve every response output item as opaque, versioned continuation
state so reasoning data and server-issued function call identifiers are
replayed exactly across tool turns. Keep this state out of persisted
review logs and invalidate older cache entries that cannot provide
lossless continuation.

Bound response bodies, continuation metadata, and persistent cache
payloads. Validate function call identifiers, arguments, and tool-result
correlation before provider continuation. Preserve compatible JSON
schemas without rewriting caller semantics, and prevent endpoint
redirects. Allow compatible Chat Completions deployments to select the
required token limit field explicitly.

Also classify Responses API retryable errors, account for cached input
tokens, satisfy JSON mode's prompt requirement, use current GPT-5.6
context defaults, and preserve legacy configurations without
[ai.openai] with a deprecation warning.

Validation:
- make check-pr RANGE=upstream/main..HEAD

Signed-off-by: Martin Hoyer <mhoyer@redhat.com>
Document the dedicated OpenAI Responses provider, its configuration,
and the migration from legacy [ai.openai_compat] settings. Update the
checked-in settings and standalone example to use the new [ai.openai]
table.

Describe lossless output-item replay, JSON mode, cached-token
accounting, and function call identifier handling. Refresh GPT-5.6
reasoning effort values and context limits to match the current API
documentation.

Signed-off-by: Martin Hoyer <mhoyer@redhat.com>
@martinhoyer
martinhoyer force-pushed the openai-responses-provider branch from 9f0dfae to 20dc7e9 Compare September 19, 2026 10:42
@martinhoyer

Copy link
Copy Markdown
Author

@rgushchin I've addressed the v2 review and then started dogfooding sashiko review w/ openai provider. A lot of changes were needed, especially around preserving backwards compatibility.

@sashiko-bot

sashiko-bot Bot commented Sep 19, 2026

Copy link
Copy Markdown

Sashiko review — v3

Commit 2/3 — 8e8528e8 ai: add OpenAI Responses API provider and wire into provider factory

  • [HIGH] In src/ai/cache.rs (CachingAiProvider::prune_payload and
    store_entry), the prune_payload query uses an unbounded window function
    without a LIMIT clause to calculate the total payload size. Because SQLite
    must scan every row in response_cache and compute text blob lengths, and
    this runs inside an Immediate write transaction on every cache insertion,
    it will increasingly hold the database write lock as the cache fills. This
    violates the invariant to keep write transactions short and will cause
    SQLITE_BUSY stalls that severely degrade daemon throughput.

  • [MEDIUM] In src/ai/openai_responses.rs (decode_response), src/ai/mod.rs
    (provider_metadata_size), and src/ai/cache.rs (diagnostic_request_json),
    heavy CPU-bound JSON processing (like parsing up to 17MB or serializing
    massive structures) executes synchronously on the Tokio worker thread. This
    stalls the cooperative scheduler, causing latency spikes and delaying other
    concurrent async tasks. These operations should be offloaded using
    tokio::task::spawn_blocking.

Full review and stage logs on sashiko.sashiko.dev

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