Skip to content

OpenAI native: use nested function tools by default#169

Merged
PrzemyslawKlys merged 1 commit intomasterfrom
feature/openai-responses-tools-nested
Feb 9, 2026
Merged

OpenAI native: use nested function tools by default#169
PrzemyslawKlys merged 1 commit intomasterfrom
feature/openai-responses-tools-nested

Conversation

@PrzemyslawKlys
Copy link
Member

Fixes ChatGPT native responses rejecting tool schemas like ools[0].parameters by defaulting to standard Responses API tool format: ools[].type=function with ools[].function.{name,parameters}.

Also broadens tool wire-format fallbacks (nested -> flat -> custom) but only retries when the server indicates an unknown tool field under ools....

Tests:

  • dotnet build .\IntelligenceX.sln -c Release
  • dotnet run --project .\IntelligenceX.Tests -c Release --framework net10.0

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ca38d15f4c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +409 to +410
if (!IsToolSchemaUnknownParameter(ex)) {
throw;

Choose a reason for hiding this comment

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

P1 Badge Retry on unsupported tool-format validation errors

This loop now stops unless IsToolSchemaUnknownParameter(ex) is true, so a backend that rejects the new default function tool shape with a non-unknown_parameter validation error (for example invalid_value on tools[0].type) will throw immediately and never reach the later Custom* fallback candidates. Because this commit switched the initial attempt to FunctionNestedParameters, that behavior is a regression for custom-only ChatGPT-native variants that previously worked with the old custom-first default.

Useful? React with 👍 / 👎.

@PrzemyslawKlys PrzemyslawKlys merged commit a29b743 into master Feb 9, 2026
6 of 9 checks passed
@PrzemyslawKlys PrzemyslawKlys deleted the feature/openai-responses-tools-nested branch February 9, 2026 10:32
@intelligencex-review
Copy link

intelligencex-review bot commented Feb 9, 2026

IntelligenceX Review

Reviewing PR #169: OpenAI native: use nested function tools by default
Reviewed commit: ca38d15

Merge blockers: items in Todo List ✅ and Critical Issues ⚠️ sections (if present). Other Issues 🧯 are suggestions.

Summary 📝

This moves the default tool wire format to the standard Responses API shape (tools[].type="function", tools[].function.{name,parameters}) and broadens fallback retries across multiple observed variants. The approach looks directionally correct, and the added tests cover more server error-message permutations. Main concern: the new retry loop no longer uses the server’s indicated failing field to choose the next candidate, which can cause extra retries and potentially retry on mismatched schema key (parameters vs input_schema) when nested format is supported but only one key is accepted.

Todo List ✅

  • Use TryGetToolSchemaKeyFallback(...) to select the next candidate (parameters vs input_schema) for a given wire-format family instead of iterating a fixed list. Rationale: reduces unnecessary retries and avoids choosing the wrong schema key when the server supports a family but rejects only a specific field.
  • Ensure retry gating is consistent: IsToolSchemaUnknownParameter currently retries on any “tools” unknown-field message (including non-schema/tool-choice issues). Rationale: prevents masking real request bugs and avoids unintended retry storms.

Critical Issues ⚠️

  • Retry selection is no longer guided by the failing field path (e.g., .function.parameters vs .function.input_schema), so you may retry the wrong variant first within the same family. Rationale: can convert a single fallback into multiple failing requests and increase latency/cost, and in some cases can fail entirely if only one key is accepted and you exhaust retries due to ordering.

Other Issues 🧯

  • IsToolSchemaUnknownParameter treats any message containing “tools” + “unknown field/parameter” as retryable; consider tightening to require a recognizable tools[<n>]... / tools.<n>... path (you already have a parser). Rationale: avoids retrying on unrelated “tools” problems (e.g., wrong tool_choice fields) where wire-format changes won’t help.
  • SendWithWireFormatAsync(candidate, usePrebuilt ? body : null) special-cases only FunctionNestedParameters; if the caller prebuilt with default and default changes again, this coupling can break. Rationale: keeps behavior stable when defaults evolve.
  • The “fallback exhausted” InvalidOperationException should include attempted formats for diagnostics. Rationale: improves debuggability in production without changing behavior.

Tests / Coverage 🧪

  • Good: tests now cover nested .function.parameters / .function.input_schema and both bracket and dot indexing.
  • Gap: no test asserts that retries stop when the error is not tool-schema-related (i.e., IsToolSchemaUnknownParameter returns false). Add a negative test to prevent over-retrying.

Next Steps 🚀

  • Re-introduce field-path-driven selection (via TryGetToolSchemaKeyFallback) to pick between Parameters/InputSchema for the current wire-format family before moving to the next family.
  • Tighten IsToolSchemaUnknownParameter to require a parsed tools field path (reuse TryGetToolSchemaKeyFallback(string, ...) logic) and add a negative test ensuring no retries occur for unrelated 400s.

Static Analysis Policy 🧭

  • Config mode: respect
  • Packs: All Essentials (50)
  • Rules: 53 enabled
  • Rule list display: up to 10 items per section
  • Enabled rules preview: CA2000 (Dispose objects before losing scope), CA1062 (Validate arguments of public methods), SA1600 (Elements should be documented), CA1016 (Mark assemblies with assembly version), CA1018 (Mark attributes with AttributeUsageAttribute), CA1041 (Provide ObsoleteAttribute message), CA1047 (Do not declare protected member in sealed type), CA1050 (Declare types in namespaces), CA1061 (Do not hide base class methods), CA1067 (Override Object.Equals(object) when implementing IEquatable<T>) (truncated)
  • Result files: 2 input patterns, 1 matched, 1 parsed, 0 failed
  • Status: pass
  • Rule outcomes: 0 with findings, 53 clean
  • Failing rules: none
  • Clean rules: CA2000 (Dispose objects before losing scope), CA1062 (Validate arguments of public methods), SA1600 (Elements should be documented), CA1016 (Mark assemblies with assembly version), CA1018 (Mark attributes with AttributeUsageAttribute), CA1041 (Provide ObsoleteAttribute message), CA1047 (Do not declare protected member in sealed type), CA1050 (Declare types in namespaces), CA1061 (Do not hide base class methods), CA1067 (Override Object.Equals(object) when implementing IEquatable<T>) (truncated)
  • Outside-pack rules: none

Static Analysis 🔎

  • Findings: 0 (no issues at or above configured severity)

Model & Usage 🤖

  • Model: gpt-5.3-codex
  • Length: medium
  • Mode: inline
  • Reasoning: not configured
  • Usage: 5h limit: 90% remaining | weekly limit: 53% remaining | code review weekly limit: 100% remaining | credits: 0

if (!attempted.Add(retryFormat)) {
lastError!.Throw();
}
var candidates = new[] {

Choose a reason for hiding this comment

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

Retry order is fixed and ignores the server-indicated failing field (parameters vs input_schema); use TryGetToolSchemaKeyFallback to choose the next candidate within the same family to avoid extra failing requests.

throw;
} catch (Exception ex) {
lastError = ExceptionDispatchInfo.Capture(ex);
if (!IsToolSchemaUnknownParameter(ex)) {

Choose a reason for hiding this comment

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

IsToolSchemaUnknownParameter is currently broader than “unknown tool schema field” (it matches any “unknown field/parameter” + “tools” substring); consider gating retries on a parsed tools[<n>]... path to avoid retrying non-schema request bugs.

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.

1 participant