fix(providers): honour tool_choice: auto for openai_compatible models

[FenjuFu]

Fixes #707.

Some OpenAI-compatible endpoints ignore both response_format and a forced tool_choice and answer in prose. iFlytek's Astron Token Plan (spark-x2.5) is one. Behind openai_compatible, every semantic analyzer then fails structured-response validation and the scan degrades to static analysis only. The Bedrock provider already handles this class of model through tool_choice: auto and _require_tool_call. This PR lets openai_compatible use the same path.

Changes

• OpenAICompatibleProvider reads tool_choice from its registry (bundled or SKILLSPECTOR_MODEL_REGISTRY). For tool_choice: auto it builds ChatOpenAI with disabled_params={"tool_choice": None}. LangChain's _filter_disabled_params then drops the forced choice, and structured_output_method returns function_calling. A declared structured_output: still wins, and so does SKILLSPECTOR_STRUCTURED_OUTPUT_METHOD.
• create_openai_compatible_chat_model takes an optional disabled_params.
• bind_structured_output treats a ChatOpenAI with tool_choice disabled like Bedrock's supports_tool_choice_values=("auto",), but only when function_calling is selected explicitly, because ChatOpenAI defaults to json_schema. It then adds the prompt instruction, and a prose answer raises StructuredOutputParseError, which the analyzers retry.
• Bedrock behaviour change: SKILLSPECTOR_STRUCTURED_OUTPUT_METHOD=function_calling on a Bedrock model restricted to toolChoice auto used to skip _require_tool_call, so a prose answer parsed to None. It is now wrapped too, so a prose answer becomes a retryable parse error. SKILLSPECTOR_STRUCTURED_OUTPUT_METHOD=json_schema stays unwrapped. (BedrockProvider has no structured_output_method, so a registry structured_output: entry does not reach it; that is unchanged here.)
• Bundle spark-x2.5 in the openai_compatible registry: context_length: 262144 (256K, as published for the hosted model) and tool_choice: auto. There is no max_output_tokens, because the endpoint accepted max_completion_tokens up to 300000 in a probe.
• README: one sentence in the structured-output paragraph.

Apart from the Bedrock case above, other providers and models are unchanged. For a ChatOpenAI without disabled_params, the binding is the same as before.

Validation

• New tests in tests/unit/test_new_providers.py cover: default models keep a forced tool_choice; spark-x2.5 gets disabled_params and function_calling; a registry override can declare tool_choice: auto; and an explicit structured_output wins.
• New test in tests/unit/test_llm_utils.py uses a real ChatOpenAI with stubbed _generate. The request carries the tool without tool_choice, the prompt asks for the call, a prose answer raises StructuredOutputParseError, and the next tool-call answer parses. All 5 new tests fail on main and pass with the change.
• Review follow-up: tests pin an explicit function_calling (wrapped) and json_schema (unwrapped) on an auto-only model, and a tool_choice-disabled ChatOpenAI with no method or json_schema sending response_format without tools or the prompt instruction.
• tests/unit/test_llm_utils.py, test_new_providers.py, test_providers.py and test_bedrock_provider.py: 291 passed, 9 skipped. ruff check and ruff format --check pass.
• Live scan of tests/fixtures/malicious_skill against the Token Plan endpoint with SKILLSPECTOR_MODEL=spark-x2.5:

	main (2226747)	this branch
analysis_completeness.status	partial	complete
coverage_percent	0	100
fully inspected files	0 / 2	2 / 2
structured-response retries / exhausted	20 / 4	4 / 0
issues	7 (static only)	15 (adds SDI-1, SDI-2, SDI-4, SQP-2, SSD-4, TP4)

On main, semantic_developer_intent, semantic_security_discovery and semantic_quality_policy end with llm_structured_response_invalid. On this branch the 4 retries are prose answers that the prompt-and-retry path recovers.

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation is scoped, preserves precedence rules, and includes targeted regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Enables reliable structured output for OpenAI-compatible endpoints that require automatic tool selection.

Changes:

• Honors registry-based tool_choice: auto.
• Adds prompt-and-retry handling for prose responses.
• Registers spark-x2.5 and adds regression tests and documentation.

File	Description
README.md	Documents OpenAI-compatible automatic tool selection.
src/​skillspector/​llm_utils.py	Detects disabled forced tool choice and requires tool calls.
src/​skillspector/​providers/​chat_models.py	Passes disabled parameters to ChatOpenAI.
src/​skillspector/​providers/​openai_compatible/​model_registry.yaml	Registers spark-x2.5.
src/​skillspector/​providers/​openai_compatible/​provider.py	Applies registry tool-choice behavior.
tests/​unit/​test_llm_utils.py	Tests prose rejection and successful tool parsing.
tests/​unit/​test_new_providers.py	Tests registry and provider behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rng1995]

[SkillSpector Review]

Hi @FenjuFu, thank you for the careful diagnosis in #707 and for a fix that reuses the existing Bedrock tool_choice: auto path instead of adding a new one!

Value and readiness: This solves the stated problem. A model with tool_choice: auto in the openai_compatible registry now gets a ChatOpenAI with disabled_params={"tool_choice": None} and the function_calling method. Prose answers become retryable StructuredOutputParseErrors, so an endpoint like spark-x2.5 no longer drops semantic analysis to static-only. Models without that entry, and the openai/nv_build/ollama providers that share create_openai_compatible_chat_model, are unchanged. Retry exhaustion still reports llm_structured_response_invalid, so completeness is not overclaimed. It is ready for final maintainer review. The notes below are optional.

Material findings

1. [Non-blocking] src/skillspector/llm_utils.py:413: the condition rewrite also changes Bedrock behavior, which the PR body describes as unchanged. Before, any explicit method skipped _require_tool_call (if kwargs or ...). Now an explicit function_calling on an auto-only Bedrock model is wrapped. That applies to SKILLSPECTOR_STRUCTURED_OUTPUT_METHOD=function_calling or a registry structured_output: function_calling. The tool-call instruction is then appended, and a prose answer raises a retryable StructuredOutputParseError instead of reaching parse_response as None. I think this is an improvement. Please mention it in the description, and consider pinning it with a test next to test_auto_only_tool_choice_asks_for_the_call_and_rejects_prose.
2. [Non-blocking] src/skillspector/llm_utils.py:413: kwargs.get("method", "function_calling") assumes LangChain's default method is function_calling. That holds for ChatBedrockConverse. It does not hold for ChatOpenAI, whose with_structured_output defaults to json_schema (langchain-openai 1.3.3). Today this cannot fire, because OpenAICompatibleProvider always returns an explicit method whenever it sets disabled_params. Any future caller that binds a disabled_params ChatOpenAI with no method hint would get a "call the tool" instruction on a response_format request. Wrapping the disabled_params branch only when method == "function_calling" is explicit would remove that trap.
3. [Non-blocking] tests/unit/test_llm_utils.py:859: a short companion case for SKILLSPECTOR_STRUCTURED_OUTPUT_METHOD=json_schema (or a declared structured_output: json_schema) with a tool_choice: auto ChatOpenAI would show that the binding is left unwrapped and that response_format is sent. test_declared_structured_output_method_wins covers only the provider hint, not the binder.

PIC tradeoffs:

• Bundled model entry. The PR adds a vendor-specific spark-x2.5 entry to the bundled openai_compatible registry. This follows the existing Groq/Together/DeepSeek entries. It also matters because SKILLSPECTOR_MODEL_REGISTRY replaces the bundled file rather than merging with it, so users would otherwise have to copy the whole registry. The cost is a maintained entry that CI cannot exercise live. Its context_length: 262144 and missing max_output_tokens rest on the author's probe. The output budget then falls back to the percentage-of-context default in model_info.get_max_output_tokens.
• Overlap with #691. #691 (chrisknvidia) edits the same bind_structured_output condition to add a TP4 preferred_method. The two PRs will textually conflict, so whichever lands second needs a rebase. They compose semantically: a preferred function_calling on an auto-only model would be wrapped under this PR's rule.

Verification and gaps: I read the full diff at the PR head and traced:

• Request path. The path runs OpenAICompatibleProvider.create_chat_model → create_openai_compatible_chat_model → ChatOpenAI(disabled_params=...) → with_structured_output(method="function_calling"). In langchain-openai 1.1.10 (the floor), 1.3.3 (locked) and 1.6.6, bind_kwargs = self._filter_disabled_params(...) drops tool_choice while keeping tools. The new ChatOpenAI-level test asserts exactly that.
• Retry path. _require_tool_call errors reach _StructuredResponseValidationError and the bounded retry loop in llm_analyzer_base._invoke_batch_with_retries.
• Precedence. SKILLSPECTOR_STRUCTURED_OUTPUT_METHOD comes first, then a declared structured_output, then tool_choice: auto.
• Other providers. Bedrock's default method is function_calling in langchain-aws 1.6.1, so the empty-kwargs case is unchanged.

The live scan numbers in the PR body are the author's; I did not reproduce them. Tests were not executed locally per review policy. CI: all 6 checks are green, DCO is signed off, and there are no merge conflicts with main. The Copilot review raised no findings.

---

Decision: Approved (reviewed head cc49361ee74572439a62610ed27bec248aa6d71b)

[Copilot]

Copilot review overview

🟡 Changes recommended

Bedrock registry-declared structured-output methods are ignored, contradicting the documented behavior.

Review effort: Balanced
Findings: 1

Open (1)

• Bedrock registry override fails to expose structured output method · New

[rng1995]

[SkillSpector Review]

Hi @FenjuFu, thank you for tightening this after the last review!

Value and readiness: ed66750 resolves the hidden-default note. _binds_unforced_tool_call now wraps only when the bound method actually binds a tool. For Bedrock auto-only models this is None or function_calling, as before. For a ChatOpenAI with tool_choice disabled it is only an explicit function_calling, so the default json_schema path stays unwrapped. The openai_compatible fix the PR is for still works: the provider test without an env override still gets the tool call asked for. Ready for final maintainer review.

Previous findings:

• The condition assumed the LangChain default method is function_calling, which is not true for ChatOpenAI: Resolved (llm_utils.py:420-433).
• No test that a json_schema override on an auto-only model stays unwrapped: Resolved. test_auto_only_tool_choice_with_an_explicit_method covers both methods, and test_chat_openai_with_tool_choice_disabled_keeps_a_non_tool_method checks that response_format is sent with no tools.
• Bedrock side effect of an explicitly requested function_calling: now covered by the parametrized auto-only test. The behavior is deliberate and documented in the docstring.

Material findings: None.

PIC tradeoffs: Unchanged: the vendor-specific spark-x2.5 registry entry is bundled here, and #691 edits nearby lines, so whichever PR lands second needs a rebase.

Verification and gaps: I read the full cc49361..ed66750 delta and traced both branches of the new predicate. All 6 CI checks pass on ed66750. Tests were not run locally, per policy.

---

Decision: Approved (reviewed head ed667501c8e7cc8e599068d0ca2119dfa1159f48)