fix(auxiliary): pass auxiliary.<task>.extra_body through to API requests - #35568
fix(auxiliary): pass auxiliary.<task>.extra_body through to API requests#35568magnus919 wants to merge 1 commit into
Conversation
get_auxiliary_extra_body() only returned Nous Portal product tags and never read the extra_body configured in auxiliary.<task>.extra_body from config.yaml. This meant any provider-specific parameters (e.g. enable_thinking: false for Qwen3 models) were silently dropped. - Add optional task parameter to get_auxiliary_extra_body() - When provided, merge auxiliary.<task>.extra_body into the result - Wire task='profile_describer' in the profile_describer caller - Bump profile_describer max_tokens from 400 to 600 (tight with 55+ skill names in context) Fixes NousResearch#35566 Signed-off-by: Magnus Hedemark <magnus919@pm.me>
|
I found two issues worth addressing before merge.
try:
task_cfg = _get_auxiliary_task_config(task)
task_extra = task_cfg.get("extra_body", {}) or {}
if isinstance(task_extra, dict):
result.update(task_extra)
except Exception:
passIf Suggested fix: at minimum log at DEBUG so operators can diagnose: except Exception as exc:
logger.debug("get_auxiliary_extra_body(%s): ignoring task config: %s", task, exc)
The PR title says "pass auxiliary.<task>.extra_body through to API requests" but the diff also changes |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review Findings
This PR fixes the same bug as #35570 but with slightly different scope — focused on the profile_describer path with an additional max_tokens bump from 400→600. Same core fix: get_auxiliary_extra_body() now accepts a task parameter and merges task-specific extra_body from config.yaml.
✅ Looks Good
- Same correct merge logic as the parallel fix —
result.update(task_extra)with proper guarding. - The
max_tokensbump (400→600) inprofile_describer.pyis well-justified: generating a JSON{"description": "..."}with 55+ skill names in context is tight at 400 tokens. This is an independent improvement that addresses a real issue. - Backward compatible — no callers without a
taskargument are broken. - Defensive error handling —
try/exceptaround config loading. - Good PR description with quantitative evidence (60% failure rate before, 5/5 success after).
Note
- This PR and #35570 (by liuhao1024) fix the same bug with overlapping implementations. The merge will need to reconcile which version of
get_auxiliary_extra_body()wins — the implementations are similar enough that one should supersede the other cleanly. The test coverage from #35570 is more comprehensive (5 tests vs 0 here for the config-reading path), so adopting that test suite alongside the max_tokens bump would be ideal.
Reviewed by Hermes Agent
|
Closing this in favor of #35570 by liuhao1024 — their fix is more comprehensive, covers all the call sites the original issue identified, and adds proper regression tests. The core approach is the same, so no duplication risk. The only thing this PR had that theirs doesn't is the Good collaboration all around — same bug, same approach, just different scope. Thanks for the thorough fix. — Filed by Jasper (AI agent on behalf of Magnus Hedemark) |
|
Closing per original author's decision. |
What does this PR do?
get_auxiliary_extra_body()inagent/auxiliary_client.pyignores theextra_bodyconfigured inauxiliary.<task>.extra_bodyfromconfig.yaml. The config field exists, the schema accepts it,_get_auxiliary_task_config()returns it — but the function only returns Nous Portal product tags and never reads the task-specific extra_body.This means every auxiliary task that configures
extra_body(e.g.enable_thinking: falsefor Qwen3 models running locally) has that configuration silently dropped from API requests.Related Issue
Fixes #35566
Type of Change
Changes Made
agent/auxiliary_client.pyModified
get_auxiliary_extra_body()to accept an optionaltaskparameter. When provided, it readsauxiliary.<task>.extra_bodyfrom the resolved config and merges it with any Nous Portal extras:task: str = ""parameter toget_auxiliary_extra_body()_get_auxiliary_task_config(task).get("extra_body", {})and merges into the result dicttaskcontinue to get only Nous Portal tags (backward compatible)hermes_cli/profile_describer.pyTwo changes:
task="profile_describer"toget_auxiliary_extra_body()so the configured extra_body is actually deliveredmax_tokensfrom 400 to 600 — generating{"description": "..."}with 55+ skill names in context is tight at 400How to Test
Pre-requisite
Set
enable_thinking: falseinauxiliary.profile_describer.extra_body:Steps
hermes profile describe researcher --autoon a Qwen3 model"LLM returned an empty response"because thinking mode burns all 400 tokens on reasoning,contentis nullenable_thinking: falseextra_body is forwarded in the API requestVerification (unit)
python3 -m pytest tests/hermes_cli/test_profile_describer.py tests/agent/test_auxiliary_client.py -v -q # 201 passedChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
hermes --toolsets skills -q "Use the X skill to do Y"N/A — not a skill PR.
Screenshots / Logs
Before the fix, the HTTP request body contained no
extra_bodykey:After the fix, with
auxiliary.profile_describer.extra_bodyconfigured, theextra_bodykey is present and includes the configured parameters. Verified via live API tracing against a local Qwen3.6 endpoint: 5/5 consecutive profile describe calls succeeded vs ~60% failure rate before the change.Filed by Jasper (AI agent on behalf of Magnus Hedemark)