Skip to content

feat(provider): keep reasoning effort pass-through consistent across OpenAI and Anthropic providers#285

Merged
keshprad merged 3 commits into
NVIDIA:mainfrom
rodboev:pr/reasoning-effort-283
Jul 20, 2026
Merged

feat(provider): keep reasoning effort pass-through consistent across OpenAI and Anthropic providers#285
keshprad merged 3 commits into
NVIDIA:mainfrom
rodboev:pr/reasoning-effort-283

Conversation

@rodboev

@rodboev rodboev commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

SkillSpector now treats SKILLSPECTOR_REASONING_EFFORT as one provider-neutral pass-through setting across its OpenAI-compatible and native Anthropic provider paths. Non-empty values are trimmed and forwarded unchanged, and unset or blank input preserves provider defaults.

Closes #283

Implements the maintainer-requested Anthropic expansion from the July 20 review.

Root cause

The shared OpenAI-compatible constructor already forwarded SKILLSPECTOR_REASONING_EFFORT, but the native Anthropic providers used separate constructor paths and the previous rework added an Anthropic-only validation branch. That made the same environment variable behave differently by provider, and test_llm_utils also failed to clear the new setting from inherited process state.

Diff Notes

  • use one shared trim-and-pass-through resolver for SKILLSPECTOR_REASONING_EFFORT
  • forward the same optional setting through OpenAI-compatible, native Anthropic, and Anthropic proxy construction
  • remove the local Anthropic capability matrix and invalid-value rejection tests
  • keep direct coverage that Anthropic proxy rewriting preserves nested output_config.effort
  • add SKILLSPECTOR_REASONING_EFFORT to the test_llm_utils environment cleanup
  • update the env-var docs to describe provider- and model-dependent pass-through behavior

Scope

This change covers the shared OpenAI-compatible path plus the two native Anthropic providers. SkillSpector does not validate provider- or model-specific support for non-empty values locally. Bedrock and CLI-provider behavior stay out of scope for this PR.

Verification

  • uv sync --all-extras passed; checked 151 packages.
  • Focused provider and proxy tests passed through the repo-configured Windows runner for python -m pytest tests/unit/test_providers.py tests/unit/test_anthropic_proxy_provider.py; pytest reported 98 passed, 9 skipped.
  • The inherited-env regression test passed through the repo-configured Windows runner for python -m pytest tests/unit/test_llm_utils.py::TestCredentialResolution::test_get_chat_model_returns_native_anthropic_client; pytest reported 1 passed.
  • uv run ruff check src/skillspector/providers/chat_models.py src/skillspector/providers/anthropic/provider.py src/skillspector/providers/anthropic_proxy/provider.py tests/unit/test_providers.py tests/unit/test_anthropic_proxy_provider.py tests/unit/test_llm_utils.py passed with All checks passed!.
  • uv run ruff format --check src/skillspector/providers/chat_models.py src/skillspector/providers/anthropic/provider.py src/skillspector/providers/anthropic_proxy/provider.py tests/unit/test_providers.py tests/unit/test_anthropic_proxy_provider.py tests/unit/test_llm_utils.py passed with 6 files already formatted.

@keshprad

keshprad commented Jul 20, 2026

Copy link
Copy Markdown
Member

Could you please expand this PR to support the native Anthropic provider paths as part of this PR, rather than leaving them out of scope?

SKILLSPECTOR_REASONING_EFFORT is provider-neutral, but currently it only affects providers using ChatOpenAI. ChatAnthropic definitely supports model reasoning through adaptive thinking and explicit effort configuration.

Could we translate the common SkillSpector setting into the native Anthropic request format in this PR?

  • AnthropicProvider should map the setting to the corresponding ChatAnthropic effort configuration.
  • AnthropicProxyProvider is also built on ChatAnthropic, so it should use the same configuration where the proxy accepts those fields. Please add coverage confirming that the proxy request rewrite preserves output_config.effort.
  • Anthropic-specific values should be validated at the provider boundary because its accepted effort levels are low, medium, high, xhigh, and max, rather than the full OpenAI value set.

Otherwise, the same environment variable silently has no effect when users switch from an OpenAI-compatible provider to a native Anthropic provider.

References:

@keshprad

Copy link
Copy Markdown
Member

FYI, I edited the comment: Let's remove Bedrock from scope for this PR since it seems Bedrock has a wide range of such parameter names including thinking, output_config.effort, budget_tokens.

We can come back to this support in a followup PR or future work based on community feedback. @rodboev Do you agree with that approach?

@keshprad keshprad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting changes to include native Anthropic support in this PR. Please map SKILLSPECTOR_REASONING_EFFORT for both AnthropicProvider and AnthropicProxyProvider, validate the Anthropic-specific accepted values at the provider boundary, and add coverage confirming that the proxy rewrite preserves output_config.effort. See the linked conversation comment for the detailed rationale and references.

…hs (NVIDIA#283)

Signed-off-by: Rod Boev <rod.boev@gmail.com>
@rodboev

rodboev commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Agreed on keeping Bedrock out of this PR. I limited the expansion to the two native Anthropic paths covered by the formal review.

  1. I added Anthropic-specific validation for SKILLSPECTOR_REASONING_EFFORT, accepting low, medium, high, xhigh, and max, while treating unset or blank input as provider default. The OpenAI-compatible pass-through behavior stays unchanged.
  2. AnthropicProvider and AnthropicProxyProvider now pass accepted values through ChatAnthropic's effort configuration. The existing model, credentials, endpoint, timeout, and token-limit settings stay intact.
  3. I added a direct proxy rewrite assertion showing that nested output_config.effort survives the URL, authentication, and body rewrite, and I updated the env-var docs and PR description so they no longer describe the setting as OpenAI-only.

I left Bedrock parsing, translation, tests, and documentation out of this PR. Its provider-specific reasoning controls can be handled separately if that support is requested later. CLI provider behavior also stays outside this change.

@rodboev rodboev changed the title feat(provider): forward reasoning effort to OpenAI-compatible models feat(provider): forward reasoning effort across OpenAI and Anthropic providers Jul 20, 2026
@keshprad

keshprad commented Jul 20, 2026

Copy link
Copy Markdown
Member

[P1] Could we remove the Anthropic-specific validation and keep SKILLSPECTOR_REASONING_EFFORT as a provider pass-through setting?

The behavior should be consistent across providers:

  • If the environment variable is unset or blank, omit the argument and preserve the provider default.
  • If it is non-empty, trim and forward it unchanged.
  • Let LangChain or the upstream provider validate whether the value is supported by the selected model.

Maintaining an Anthropic-specific allowlist in SkillSpector creates an unnecessary capability matrix that can become stale as models and supported effort levels evolve. It is also inconsistent with the OpenAI-compatible path, which already passes non-empty values through.

Please remove the local Anthropic enum validation and its invalid-value tests. Retain coverage for trimming, pass-through, omission when unset or blank, and preservation of output_config.effort through the proxy rewrite. The documentation should note that supported values depend on the selected provider and model.

@keshprad

Copy link
Copy Markdown
Member

[P2] Add the new variable to the test_llm_utils environment cleanup.

_LLM_ENV_VARS omits SKILLSPECTOR_REASONING_EFFORT, despite promising to clear all LLM-related variables. I reproduced a deterministic failure with:

SKILLSPECTOR_REASONING_EFFORT=invalid ... pytest \
  tests/unit/test_llm_utils.py::TestCredentialResolution::test_get_chat_model_returns_native_anthropic_client

Add the variable to that fixture’s cleanup tuple.

@keshprad

Copy link
Copy Markdown
Member

Two small changes and this looks good to me. Thanks @rodboev!

@rodboev rodboev changed the title feat(provider): forward reasoning effort across OpenAI and Anthropic providers feat(provider): keep reasoning effort pass-through consistent across OpenAI and Anthropic providers Jul 20, 2026
@rodboev

rodboev commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

I'll make SKILLSPECTOR_REASONING_EFFORT a single pass-through setting across the paths touched by this PR. OpenAI-compatible, native Anthropic, and Anthropic proxy construction will all omit unset or blank input, trim non-empty input, and forward it unchanged for LangChain and the selected provider or model to validate.

I'll remove the Anthropic accepted-value and invalid-value tests, keep the trimming and omission coverage, keep the proxy preservation check, and add SKILLSPECTOR_REASONING_EFFORT to the LLM test env cleanup in tests/unit/test_llm_utils.py.

I'll also remove the Anthropic-specific support wording from the docs and PR description. Bedrock stays out of this PR.

@keshprad keshprad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved, thanks!

@keshprad
keshprad merged commit fda4a01 into NVIDIA:main Jul 20, 2026
5 checks passed
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.

[Feature] Configurable reasoning effort for LLM analysis

2 participants