Conversation
f17400c to
14b7c27
Compare
|
Pulled the branch and read The backend change reads correctlymodel_recognized = bool(supported_efforts)
if not supported_efforts:
supported_efforts = list(VALID_REASONING_EFFORTS)
return {
...
"supported_efforts": supported_efforts,
"supports_reasoning_effort": True,
"reasoning_default_on": model_recognized,
}
Frontend matches
var defaultOn=(meta&&meta.reasoning_default_on!==undefined)?meta.reasoning_default_on:true;
if(!defaultOn&&(!effort||effort==='')){ effort='none'; }The error/catch path in One nuance:
|
b2f6201 to
2631940
Compare
|
updated the implementation, summary of changes below, preformed manual testing to validate. @nesquena-hermes Changes Made
Automated TestsVerified the changes locally using: uv run --with pytest --with pyyaml --with cryptography pytest tests/test_reasoning_effort_model_capabilities.py tests/test_reasoning_chip_btw_fixes.py
Manual Verification
|
|
Pulled the updated branch (HEAD The two
|
e0195e6 to
6918580
Compare
Changes Made - @nesquena-hermes
|
|
| Filename | Overview |
|---|---|
| api/config.py | Adds reasoning_default_on flag, explicitly_unsupported detection, and a standard-provider fallback loop in _models_dev_reasoning_efforts; the exception-to-fallback path change and the second _models_dev_reasoning_efforts call (flagged in a prior thread) are the areas worth reviewing. |
| static/ui.js | All three _applyReasoningChip call sites pass a meta object, so the defaultOn fallback-to-true path is never reached in practice; the error-handler and option-visibility changes are correct. |
| tests/test_reasoning_effort_model_capabilities.py | New tests are well-structured; test_get_reasoning_status_cursor_acp_not_supported is non-hermetic but safe because the ACP early-return fires before any I/O; provider-loop ordering test correctly expects all 9 standard providers after the primary call. |
| tests/test_reasoning_chip_btw_fixes.py | Only cosmetic change — error message suffix (#3377) added to assertion string; no logic change. |
| CHANGELOG.md | Conflict marker from previous review thread is resolved; new Unreleased entry accurately describes the behavior change. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[get_reasoning_status called] --> B[resolve_model_reasoning_efforts]
B --> C{supported_efforts non-empty?}
C -- Yes --> D[model_recognized = True]
C -- No --> E{ACP provider?}
E -- Yes --> F[explicitly_unsupported = True]
E -- No --> G[_models_dev_reasoning_efforts metadata check]
G --> H{metadata_efforts empty list?}
H -- Yes --> F
H -- No or None --> I[model_recognized = False]
D --> J[reasoning_default_on=True, supports=True, efforts unchanged]
I --> K[reasoning_default_on=False, supports=True, efforts=VALID list]
F --> L[reasoning_default_on=False, supports=False, efforts empty]
J --> M[API response to frontend]
K --> M
L --> M
M --> N[_applyReasoningChip in ui.js]
N --> O{supports_reasoning_effort?}
O -- False --> P[Hide chip entirely]
O -- True --> Q{reasoning_default_on and no persisted effort?}
Q -- False --> R[Default effort = none]
Q -- True --> S[Default effort = active reasoning]
R --> T[Show chip inactive]
S --> U[Show chip active]
Reviews (3): Last reviewed commit: "chore(changelog): resolve residual upstr..." | Re-trigger Greptile
| def test_get_reasoning_status_unrecognized_model_still_offers_efforts(monkeypatch): | ||
| """Unrecognized models get the full effort list but reasoning_default_on=False.""" | ||
| monkeypatch.setattr( | ||
| cfg, | ||
| "resolve_model_reasoning_efforts", | ||
| lambda *a, **k: [], | ||
| ) | ||
| status = cfg.get_reasoning_status( | ||
| model_id="some-unknown-model", | ||
| provider_id="custom:myproxy", | ||
| ) | ||
| assert len(status["supported_efforts"]) > 0, ( | ||
| "Unrecognized models should still expose effort levels" | ||
| ) | ||
| assert status["supports_reasoning_effort"] is True | ||
| assert status["reasoning_default_on"] is False |
There was a problem hiding this comment.
_models_dev_reasoning_efforts not mocked — test can flip if catalog is reachable
The test patches resolve_model_reasoning_efforts to return [], but get_reasoning_status then calls _models_dev_reasoning_efforts directly (the new explicitly_unsupported check). If agent.models_dev is importable in the test environment and get_model_capabilities returns a capabilities object with supports_reasoning=False for "some-unknown-model", the function returns [], explicitly_unsupported becomes True, and both supports_reasoning_effort and len(supported_efforts) assertions fail. Adding monkeypatch.setattr(cfg, "_models_dev_reasoning_efforts", lambda *a, **k: None) makes the intent explicit and removes the environmental dependency.
|
Pulled the updated branch (HEAD The gate is correctif (provider in {"cursor-acp", "copilot-acp"} or model_prefix in {"cursor-acp", "copilot-acp"}):
explicitly_unsupported = True
elif resolve_model and not supported_efforts: # <-- the new guard
hinted_model = _strip_provider_hint_for_reasoning(resolve_model)
metadata_efforts = _models_dev_reasoning_efforts(hinted_model, provider)
if metadata_efforts == []:
explicitly_unsupported = TrueThe The new test actually proves it
assert status["supported_efforts"] == ["medium", "high"]
assert not called_metadata_check, "Should not query models.dev metadata since resolver returned success"The One small test-isolation note
Contract is now explicit on both sides and the quadrant coverage (recognized / unrecognized / ACP / metadata-unsupported / authoritative-disagreement) is solid. Reads merge-ready to me modulo that one test mock. |
## Release v0.51.247 — Release HO (stage-q19) Backend correctness fix. ### Fixed | Issue | Author | Fix | |-------|--------|-----| | #3505 | @franksong2702 | **Reasoning effort is coerced to a level the active model/provider actually supports** before each request, instead of being sent verbatim and rejected. `openai-codex` `gpt-5` no longer gets `max` (→ `xhigh`); `o1`/`o3`/`o4` clamp to `low`/`medium`/`high`. Coercion only steps *down* (never escalates); `none`/unset preserved. The capability filter is applied across heuristic / models.dev / Copilot / LM Studio paths. | This is the narrow, correct fix for the detection gap that #3431 tried to address by removing the chip-visibility gate (which we shelved). The chip-visibility gate is **untouched** (Codex confirmed) — `get_reasoning_status`/`_applyReasoningChip` still hide the chip for unconfirmed models. ### Review fix absorbed (Codex + self-flagged) The first cut **dropped** a configured effort for *unrecognized* models, because capability detection returns `[]` for both "known-unsupported" and "simply-unknown" (custom providers, aggregator-rewritten ids, new releases) — that's a behavior change vs master (which sent it verbatim) and would silently disable reasoning. Fixed: an **empty** capability set now **preserves** the configured effort (provider stays the final authority; worst case = the same rejected request master already produces, i.e. no regression). Known-bad clamps return *non-empty* filtered sets, so they still degrade correctly. Nathan chose this "preserve-for-unknown" behavior. + regression test. ### Gate - Full pytest suite: **7548 passed, 0 failed** - ruff: CLEAN · 48 reasoning tests pass (incl. preserve-for-unknown + codex-clamp + never-escalate) - Codex (regression): SHIP-ONLY-WITH-FIXES (unknown-model drop) → fixed → **SAFE TO SHIP** - Verified empirically: gpt-5/codex max→xhigh, o3 max/xhigh→high, unknown high→high (preserved), none/unset preserved Co-authored-by: franksong2702 <franksong2702@users.noreply.github.com>
…ized models (nesquena#3377) Instead of hiding the thinking/reasoning effort chip entirely when a model is not recognized as reasoning-capable, always present the selector with the full effort scale available. For recognized models (GPT-5+, Claude 4/3.7+, Qwen-3+, DeepSeek, Kimi, etc.) the chip defaults to "Default" (reasoning active). For unrecognized or ambiguous models the chip defaults to "None" (reasoning off), letting users opt-in on any model. Backend: get_reasoning_status() now always returns the full VALID_REASONING_EFFORTS list in supported_efforts, plus a new reasoning_default_on flag indicating whether the model was positively identified as reasoning-capable. Frontend: _applyReasoningChip() always displays the chip; when reasoning_default_on is false and no effort is persisted, it defaults to "None". _applyReasoningOptions() shows all effort levels when the supported set is empty (error fallback).
…g-level-missing # Conflicts: # CHANGELOG.md
6918580 to
d37a66d
Compare
|
@nesquena-hermes Merged master into Also addressed the test isolation concern by adding mock coverage for |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
Cleaned up a residual conflict marker in |
|
Thanks for the thoughtful rework here, @b3nw — and you've correctly identified a real problem: the heuristic produces false negatives, hiding the reasoning selector for custom providers and aggregator-rewritten model IDs (e.g. After a maintainer review, we've decided not to take the "always show the chip" approach. The product direction is that the reasoning-effort selector should appear only when the model actually supports reasoning options — surfacing it on every model (even with a sensible default-off) puts a control in front of users that does nothing on most models, which is the opposite of the intent. So this is a detection problem, not a visibility-model problem: the fix we want is to make I'm going to close this PR in favor of that approach. If you'd like to take a run at improving the capability detection itself (the suffix-stripping + custom-provider path in particular), that would be a very welcome contribution and we'd happily review it. Really appreciate the effort and the clear write-up — the false-negative cases you documented here are exactly the ones a detection fix should target. |
nesquena#3431) Version-gated the nested-gateway allow to 2.5-series/3-era (reviewer fix: gemini-1.5/1.0 have no thinking controls -> selector would fail on send) + pre-2.5 exclusion regression tests. Stamped v0.51.408 (Release NU). Co-authored-by: b3nw <b3nw@users.noreply.github.com>
…3837 + credential hardening) (#4202) * fix(#3750): preserve LM Studio auth on reasoning probe Rebased rodboev's #3837 onto current master (master reworked resolve_model_reasoning_efforts via #3431 with a combined hermes_cli import + _nested_route_reasoning_denied guard). Threaded the api_key resolution + the _lmstudio_model_reasoning_options wrapper + the HTTP-fallback probe into master's current structure; intent preserved byte-identically (+164/-24, same as the original PR diff). Co-authored-by: Rod Boev <rod.boev@gmail.com> * fix(#3837): gate LM Studio probe credential to configured endpoint + no-redirect opener Codex security review (CORE) on the rebased #3837: GET /api/reasoning accepts a caller-supplied base_url, and the new probe attached the configured LM Studio API key as a Bearer token; urllib also re-sends Authorization across redirects. Together that could exfiltrate the stored credential to an attacker-controlled host. (master only escalates here because master probed WITHOUT a key.) Fixes: 1. Only forward the LM Studio credential when the probe target normalizes to the configured LM Studio base URL (reuses _normalize_base_url_for_match); a caller-supplied non-matching base_url is probed keyless. 2. _lmstudio_reasoning_probe_options_fallback uses a no-redirect opener (_NoRedirectHandler) so a 3xx can't forward the Authorization header onward. 3. Two regression tests: caller-supplied base_url drops the key; redirect is not followed (credential never reaches the redirect target). Co-authored-by: Rod Boev <rod.boev@gmail.com> * fix(#3837): route ALL credentialed LM Studio probes through the no-redirect probe Codex re-gate caught that the first fix only hardened the built-in fallback; in production hermes_cli IS importable, so the preferred path called hermes_cli.models.lmstudio_model_reasoning_options(..., api_key=...) which uses plain urlopen and still follows redirects with the credential. Fix: _lmstudio_model_reasoning_options now short-circuits to the built-in no-redirect probe whenever api_key is set; the hermes_cli path is used ONLY for keyless probes (no credential to leak). Reworked tests accordingly: - test_credentialed_probe_never_calls_hermes_cli: credentialed probe bypasses CLI - test_keyless_probe_logs_signature_mismatch_before_fallback: keyless CLI degrade - test_credentialed_probe_does_not_follow_redirects_end_to_end: E2E through resolve_model_reasoning_efforts with real hermes_cli; redirect target never hit Verified both security tests fail without the guard and pass with it. Co-authored-by: Rod Boev <rod.boev@gmail.com> * chore(release): stamp v0.51.420 (Release OG) in CHANGELOG --------- Co-authored-by: nesquena-hermes <agent@nesquena-hermes> Co-authored-by: Rod Boev <rod.boev@gmail.com>
## Release v0.51.247 — Release HO (stage-q19) Backend correctness fix. ### Fixed | Issue | Author | Fix | |-------|--------|-----| | nesquena#3505 | @franksong2702 | **Reasoning effort is coerced to a level the active model/provider actually supports** before each request, instead of being sent verbatim and rejected. `openai-codex` `gpt-5` no longer gets `max` (→ `xhigh`); `o1`/`o3`/`o4` clamp to `low`/`medium`/`high`. Coercion only steps *down* (never escalates); `none`/unset preserved. The capability filter is applied across heuristic / models.dev / Copilot / LM Studio paths. | This is the narrow, correct fix for the detection gap that nesquena#3431 tried to address by removing the chip-visibility gate (which we shelved). The chip-visibility gate is **untouched** (Codex confirmed) — `get_reasoning_status`/`_applyReasoningChip` still hide the chip for unconfirmed models. ### Review fix absorbed (Codex + self-flagged) The first cut **dropped** a configured effort for *unrecognized* models, because capability detection returns `[]` for both "known-unsupported" and "simply-unknown" (custom providers, aggregator-rewritten ids, new releases) — that's a behavior change vs master (which sent it verbatim) and would silently disable reasoning. Fixed: an **empty** capability set now **preserves** the configured effort (provider stays the final authority; worst case = the same rejected request master already produces, i.e. no regression). Known-bad clamps return *non-empty* filtered sets, so they still degrade correctly. Nathan chose this "preserve-for-unknown" behavior. + regression test. ### Gate - Full pytest suite: **7548 passed, 0 failed** - ruff: CLEAN · 48 reasoning tests pass (incl. preserve-for-unknown + codex-clamp + never-escalate) - Codex (regression): SHIP-ONLY-WITH-FIXES (unknown-model drop) → fixed → **SAFE TO SHIP** - Verified empirically: gpt-5/codex max→xhigh, o3 max/xhigh→high, unknown high→high (preserved), none/unset preserved Co-authored-by: franksong2702 <franksong2702@users.noreply.github.com>
nesquena#3431) Version-gated the nested-gateway allow to 2.5-series/3-era (reviewer fix: gemini-1.5/1.0 have no thinking controls -> selector would fail on send) + pre-2.5 exclusion regression tests. Stamped v0.51.408 (Release NU). Co-authored-by: b3nw <b3nw@users.noreply.github.com>
…rebased nesquena#3837 + credential hardening) (nesquena#4202) * fix(nesquena#3750): preserve LM Studio auth on reasoning probe Rebased rodboev's nesquena#3837 onto current master (master reworked resolve_model_reasoning_efforts via nesquena#3431 with a combined hermes_cli import + _nested_route_reasoning_denied guard). Threaded the api_key resolution + the _lmstudio_model_reasoning_options wrapper + the HTTP-fallback probe into master's current structure; intent preserved byte-identically (+164/-24, same as the original PR diff). Co-authored-by: Rod Boev <rod.boev@gmail.com> * fix(nesquena#3837): gate LM Studio probe credential to configured endpoint + no-redirect opener Codex security review (CORE) on the rebased nesquena#3837: GET /api/reasoning accepts a caller-supplied base_url, and the new probe attached the configured LM Studio API key as a Bearer token; urllib also re-sends Authorization across redirects. Together that could exfiltrate the stored credential to an attacker-controlled host. (master only escalates here because master probed WITHOUT a key.) Fixes: 1. Only forward the LM Studio credential when the probe target normalizes to the configured LM Studio base URL (reuses _normalize_base_url_for_match); a caller-supplied non-matching base_url is probed keyless. 2. _lmstudio_reasoning_probe_options_fallback uses a no-redirect opener (_NoRedirectHandler) so a 3xx can't forward the Authorization header onward. 3. Two regression tests: caller-supplied base_url drops the key; redirect is not followed (credential never reaches the redirect target). Co-authored-by: Rod Boev <rod.boev@gmail.com> * fix(nesquena#3837): route ALL credentialed LM Studio probes through the no-redirect probe Codex re-gate caught that the first fix only hardened the built-in fallback; in production hermes_cli IS importable, so the preferred path called hermes_cli.models.lmstudio_model_reasoning_options(..., api_key=...) which uses plain urlopen and still follows redirects with the credential. Fix: _lmstudio_model_reasoning_options now short-circuits to the built-in no-redirect probe whenever api_key is set; the hermes_cli path is used ONLY for keyless probes (no credential to leak). Reworked tests accordingly: - test_credentialed_probe_never_calls_hermes_cli: credentialed probe bypasses CLI - test_keyless_probe_logs_signature_mismatch_before_fallback: keyless CLI degrade - test_credentialed_probe_does_not_follow_redirects_end_to_end: E2E through resolve_model_reasoning_efforts with real hermes_cli; redirect target never hit Verified both security tests fail without the guard and pass with it. Co-authored-by: Rod Boev <rod.boev@gmail.com> * chore(release): stamp v0.51.420 (Release OG) in CHANGELOG --------- Co-authored-by: nesquena-hermes <agent@nesquena-hermes> Co-authored-by: Rod Boev <rod.boev@gmail.com>
feat: always show reasoning effort selector, default off for unrecognized models (#3377)
Improvement on the #3379 which fixed #3377.
Thinking Path
claude-sonnet-4-6:free), and new model releases would silently hide the selector even though the user might want to enable reasoning.What Changed
1. Backend: Always return full effort list with
reasoning_default_onflagIn
api/config.py(get_reasoning_status):resolve_model_reasoning_effortsreturns an empty list (unrecognized model), fall back to the fullVALID_REASONING_EFFORTSlist instead of[].supports_reasoning_effortis now alwaysTrue.reasoning_default_on:Truewhen the model was positively identified as reasoning-capable,Falseotherwise.2. Frontend: Always show chip, default to "None" for unrecognized models
In
static/ui.js:_applyReasoningChip()no longer hides the chip whensupported_effortsis empty. The chip is always displayed.reasoning_default_onisFalseand no effort has been explicitly set by the user, the chip defaults to "None" (inactive state)._applyReasoningOptions()now shows all effort levels in the dropdown when the supported set is empty (previously hid them all).fetchReasoningChip()error handler defaults toreasoning_default_on: falseso the chip remains functional even on API errors.3. Expanded Test Coverage
In
tests/test_reasoning_effort_model_capabilities.py:test_get_reasoning_status_includes_supported_effortsto assertreasoning_default_onisTrue.test_get_reasoning_status_unrecognized_model_still_offers_efforts: verifies that unrecognized models get the full effort list withreasoning_default_on=False.test_get_reasoning_status_recognized_model_default_on: verifies that recognized models getreasoning_default_on=True.4. Changelog Documentation
CHANGELOG.mdunder## [Unreleased].Why It Matters
Previously, the heuristic-based hide/show logic was a constant source of false negatives: every new model release, custom provider configuration, or aggregator-rewritten model ID risked hiding the reasoning selector. This PR eliminates that class of bugs entirely by never hiding the chip. Users can always opt into reasoning, and the default is informed but not restrictive.
Verification
Automated tests
Ran the pytest suite targeting reasoning effort model capabilities:
Result: 28 passed successfully.
Risks / Follow-ups
agent.reasoning_effortconfig is profile-scoped, not model-scoped.supports_reasoning_effortis now alwaysTrue, which changes the API contract. No known consumers rely on this boolean for chip visibility (the frontend usesreasoning_default_on), but third-party integrations should be aware.AI Usage Disclosure