fix(providers): request JSON-schema structured output for models that reject forced tool calls - #529
Conversation
945b6fc to
ab87ba8
Compare
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Reviewed current head ab87ba8bdfd37bd2fa2c15d32b009b4387ecfd49. The capability hook fixes the forced-tool failure for the providers that implement it, but the same documented failure remains for supported Bedrock models because BedrockProvider has no structured-output-method selection. Add Bedrock model-family/ID/ARN recognition and regressions, or make capability selection generic so semantic analysis does not silently disappear on those models.
All required checks pass, but this incomplete provider fix and mergeStateStatus=BEHIND block merging.
with_structured_output defaults to a forced tool call, which some model families answer with HTTP 400, so every semantic batch failed and reports silently lost the LLM findings. The two direct-API providers bind json_schema for those models (registry entry or model name). Bedrock has no JSON-schema output for them, so it leaves toolChoice at auto, asks for the tool call in the prompt and retries prose. SKILLSPECTOR_STRUCTURED_OUTPUT_METHOD overrides the method for any provider. Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
ab87ba8 to
9e1de33
Compare
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed current head 9e1de33eedcfef0caf127b79b9ab8d941de9fe27. The prior Bedrock coverage gap is resolved. The provider now recognizes affected bare model IDs, geo/global inference profiles, versioned IDs, foundation/inference-profile ARNs, and registry-declared opaque application-profile ARNs; it constrains those clients to automatic tool choice, adds an explicit tool-call instruction, and converts prose fallback into the existing retryable structured-response error. The wire-format regressions verify that affected models do not send forced toolChoice or unsupported outputConfig, while unaffected models retain forced tool selection. Direct Anthropic providers use JSON-schema output as intended. I found no remaining required code, test, documentation, security, or compatibility changes.
Required checks are green. The earlier inline thread is still unresolved in GitHub even though its code concern is addressed; mergeStateStatus=BEHIND also blocks merging.
|
@stefanoamorelli - Please resolve merge conflicts to merge the PR. |
|
@rng1995 ty, consider it done by EOD |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed exact head 9e1de33eedcfef0caf127b79b9ab8d941de9fe27 following the author's new reply and thread resolution. Read the complete 15-file diff and verified the prior Bedrock finding against the provider implementation, structured-output binding, synchronous/asynchronous retry paths, registry metadata, documentation, and regression tests.
Resolved: bare, geo/global, versioned, and foundation/inference-profile ARN model identifiers select the auto-only Bedrock strategy; opaque application profiles can opt in via registry metadata. The wire-format tests check that affected models do not send forced tool choice or unsupported output configuration, and that prose fallback raises the existing retryable structured-response error. Unaffected models retain forced tool selection; direct Anthropic clients use their JSON-schema hint. No required change remains and all review threads are now resolved.
All five hosted checks pass. GitHub reports merge conflicts (DIRTY), and the author has acknowledged the requested rebase; resolve those separately and revalidate the resulting head before merging. Static inspection and hosted CI only; no contributor code/tests executed and no live provider calls made. No merge performed.
Resolve the analyzer imports, CLI structured-wrapper ownership, and Bedrock tool-choice/sampling conflicts against main at 071d217. Keep main's mutable timeout propagation and provider-control provenance alongside the PR's structured-output capability selection. Extend the timeout regression across structured-output methods and assert that Bedrock auto-only tool choice retains requested sampling settings. Prepared by Codex on behalf of Narendran Raghavan. Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
LangChain forces a tool call when binding the analyzers' response schema, and the current model generation answers that with HTTP 400, so every semantic batch failed and reports silently fell back to static analysis.
I route those models to the native JSON-schema response format on the two direct-API providers. Bedrock rejects that format for them as well, so there I keep toolChoice at auto, ask for the tool call in the prompt, and retry a prose answer. langchain-aws made the same choice in 1.7.6. SKILLSPECTOR_STRUCTURED_OUTPUT_METHOD overrides the method for any provider.
Related history: #66, #76, #132.