Skip to content

fix(deepseek): clamp 'minimal' reasoning effort to 'low' instead of dropping it - #65954

Closed
Drexuxux wants to merge 1 commit into
NousResearch:mainfrom
Drexuxux:fix/deepseek-minimal-reasoning-effort-clamp
Closed

fix(deepseek): clamp 'minimal' reasoning effort to 'low' instead of dropping it#65954
Drexuxux wants to merge 1 commit into
NousResearch:mainfrom
Drexuxux:fix/deepseek-minimal-reasoning-effort-clamp

Conversation

@Drexuxux

Copy link
Copy Markdown
Contributor

What

minimal reasoning effort was silently dropped by the DeepSeek profile (matched no arm → reasoning_effort omitted → DeepSeek fell back to its high default). So asking for the least reasoning gave you the most. Added the missing minimallow clamp (the floor), matching the existing xhigh/maxmax ceiling clamp.

Fix

  • plugins/model-providers/deepseek/__init__.py — add minimallow arm.
  • tests/plugins/model_providers/test_deepseek_profile.py — add test_minimal_clamps_to_low.

Tests

pytest tests/plugins/model_providers/test_deepseek_profile.py -q32 passed. Revert the fix and the 3 new minimal cases fail (effort omitted → default high); with the fix they assert low.

…ropping it

The DeepSeek profile clamps stronger-than-supported levels down to the
ceiling (xhigh/max/ultra → max) and passes low/medium/high through, but
'minimal' matched neither arm and fell through with no reasoning_effort
emitted. An omitted effort makes DeepSeek apply its server default —
currently 'high', the STRONGEST tier — so a user who explicitly asked for
'minimal' reasoning got maximum reasoning instead: the opposite of their
request, at higher latency and token cost.

'minimal' is a valid Hermes effort level (hermes_constants.VALID_REASONING_EFFORTS),
distinct from a garbage value, so it should clamp to DeepSeek's nearest
supported level ('low', the floor) rather than being treated like unknown
input. This mirrors both this profile's own xhigh/max → max ceiling clamp
and the canonical minimal → low clamp already used across the codebase
(auxiliary/codex/xai transports).

Adds a regression test (test_minimal_clamps_to_low, case-insensitive)
asserting reasoning_effort='low'; the existing garbage/empty → omit tests
are unaffected since neither equals 'minimal'.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have labels Jul 16, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good. No obvious issues found.


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. The premise holds on current main: minimal is a valid Hermes effort (hermes_constants.py:794-822), while DeepSeekProfile only maps xhigh/max/ultra and low/medium/high (plugins/model-providers/deepseek/__init__.py:75-80). The standard chat transport applies this profile hook’s returned top-level kwargs to the request (agent/transports/chat_completions.py:579-592).

The proposed minimallow mapping and case/whitespace regression coverage are consistent with that contract. Current GitHub main has not changed either touched path since the PR base in a way that conflicts; GitHub reports the PR cleanly mergeable.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 18, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Four PRs are associated with #30818. #30832 removes the default extra_body.thinking injection implicated by the successful curl/custom-provider comparison, while #30883 changes effort normalization, #54556 changes only the auxiliary-model default, and #65954 handles the separate minimal effort mapping.

Related pull requests

Duplicates

#30883 duplicates the reasoning-effort normalization in #28945; #54556 and #65954 do not duplicate #30832 because they change independent auxiliary-model and effort-mapping behavior.

Suggested consolidation

Author action: rebase #30832 onto main and split out, if necessary, the opt-in thinking guard plus focused first-request regression coverage; this preserves the recorded best-fix path without closing it. Keep #54556 open with a salvage path that applies the contributor-requested behavior-contract tests, keep #65954 open with its focused minimal-to-low regression coverage, and leave #30883 closed as a duplicate of #28945.

Cross-PR triage: Reviewed 4 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 29 kB of PR diffs, 14 kB of issue/PR text, 4 kB of discussion (8 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@teknium1

Copy link
Copy Markdown
Contributor

Your diagnosis was exactly right — dropping minimal fell through to DeepSeek's server default (high), the opposite of the ask. This is now fixed on main via the centralized effort vocabulary: agent/reasoning_effort.py (PR #90350) declares DeepSeek V4's set as low/medium/high/max and the shared clamp_effort() degrades minimal → low (nearest weaker level) in both the deepseek profile and the opencode-go relay path. Your earlier-submitted fix predates that refactor and matches its behavior exactly — closing as implemented on main, with credit for the correct diagnosis and direction.

@teknium1 teknium1 closed this Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants