feat(delegate): add phase-based delegation routing - #3
Conversation
|
Review Complete Files Reviewed: 9 By Severity:
Three bugs found in the per-phase delegation routing: a restrictive reasoning_effort guard that silently drops 'minimal' and 'none' levels (high), a per-phase api_key being silently ignored when a named provider is specified (medium), and orphaned child agents left registered on batch credential failures (low). Files Reviewed (9 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (68/100) — 1 high finding, 1 low · 744 LOC across 9 files
Summary
This PR introduces per-phase delegation routing (model, provider, reasoning config per phase) and adds a process_registry for enhanced process tracking. The review found 3 bugs, one high severity.
Findings
High — reasoning_effort guard is too restrictive (tools/delegate_tool.py:3156)
The _resolve_phase_reasoning_config() function uses a hardcoded whitelist ({"low", "medium", "high", "xhigh"}) to gate whether a phase's reasoning_effort is used. This excludes "minimal" and "none", both valid values accepted by parse_reasoning_effort. Users who configure these levels in a phase assignment will silently fall through to the global/parent reasoning config with a misleading warning logged.
Medium — Per-phase api_key silently dropped for named providers (tools/delegate_tool.py:3124)
When a phase assigns both a named provider (e.g., openrouter) and a custom api_key, _resolve_delegation_credentials unconditionally uses runtime.get("api_key") in the provider-resolution path, ignoring the configured_api_key extracted from the merged config. The key is silently dropped with no warning.
Low — Orphaned children on batch credential failure (tools/delegate_tool.py:2528)
Per-task credential resolution means a ValueError from a later task in the batch loop causes an immediate tool_error() return, but children built for earlier tasks remain registered in parent_agent._active_children with no cleanup. Previously credential resolution happened once before any children were built.
Risk Assessment
The api_key handling bug (finding-002) is the most concerning — it could cause delegation to silently use the wrong API key, leading to billing/rate-limit issues or, in the worst case, cross-tenant credential leakage if profiles share a provider. The reasoning_effort bug is a correctness regression for users of the new per-phase feature.
| if phase_effort: | ||
| if phase_effort.lower() in {"low", "medium", "high", "xhigh"}: | ||
| parsed = parse_reasoning_effort(phase_effort) | ||
| if parsed is not None: | ||
| return parsed | ||
| logger.warning( | ||
| "Unknown delegation.phase_assignments reasoning_effort %r; falling back", | ||
| phase_effort, | ||
| ) |
There was a problem hiding this comment.
🟠 Phase reasoning_effort guard excludes valid 'minimal' and 'none' levels, causing silent fallback to global/parent reasoning (bug)
In _resolve_phase_reasoning_config() at tools/delegate_tool.py:3156-3164, the pre-check phase_effort.lower() in {"low", "medium", "high", "xhigh"} gates whether a phase assignment's reasoning_effort is parsed and applied. This set excludes "minimal" (a standard effort level in hermes_constants.VALID_REASONING_EFFORTS) and "none" (accepted by parse_reasoning_effort, returning {"enabled": False}). When a user configures delegation.phase_assignments.<phase>.reasoning_effort: minimal or none, the phase-specific resolution rejects it as unknown, logs a spurious warning, and falls back to the global delegation.reasoning_effort or parent agent's reasoning config. The global reasoning path in _build_child_agent correctly calls parse_reasoning_effort directly without the restrictive guard, so this is an inconsistency introduced only in the phase-specific resolver. The per-phase routing correctly selects the phase's model/provider/base_url, but the reasoning effort silently diverges from the user's explicit configuration.
💡 Suggestion: Replace the hardcoded whitelist guard with a direct call to parse_reasoning_effort, matching the approach already used in _build_child_agent for global delegation.reasoning_effort. If parse_reasoning_effort returns non-None, use it; otherwise warn and fall through.
📋 Prompt for AI Agents
In tools/delegate_tool.py, function _resolve_phase_reasoning_config around lines 3156-3164, replace the restrictive guard if phase_effort.lower() in {"low", "medium", "high", "xhigh"}: with a direct call to parse_reasoning_effort(phase_effort). If the result is not None, return it immediately. Only log the warning when parse_reasoning_effort returns None. This removes the incomplete whitelist and delegates validation entirely to parse_reasoning_effort, which already handles all valid levels including "minimal" and "none". Add a test case for reasoning_effort: "minimal" in a phase assignment to prevent regression.
| creds = _resolve_task_delegation_routing(cfg, t.get("phase"), parent_agent) | ||
| except ValueError as exc: | ||
| return tool_error(str(exc)) |
There was a problem hiding this comment.
🟢 Orphaned children in _active_children on per-task credential failure inside batch loop (bug)
In delegate_task (tools/delegate_tool.py:2528), credential resolution now happens per-task inside the child-construction loop via _resolve_task_delegation_routing. If a ValueError is raised for task N (where N >= 1), the function returns tool_error() immediately via the try/except at lines 2528-2530. However, children 0..N-1 were already built by _build_child_agent, which unconditionally registers them to parent_agent._active_children (line 1418-1424) and fires subagent_start hooks (line 1437-1443). These children are never cleaned up — no removal from _active_children, no subagent_end hooks, and no run_conversation is ever called on them. Previously (pre-PR), credential resolution happened once before the loop, so any failure meant zero children were built. The finally block at lines 2561-2563 only restores _last_resolved_tool_names and does not address the orphaned children.
💡 Suggestion: Add a pre-validation pass that calls _resolve_task_delegation_routing for every task in task_list before building any children. If any task fails, return tool_error before constructing child agents. This restores the all-or-nothing behavior for credential failures.
📋 Prompt for AI Agents
In tools/delegate_tool.py, delegate_task function, before the child-construction loop (around line 2525), add a pre-validation pass that iterates over task_list and calls _resolve_task_delegation_routing(cfg, t.get("phase"), parent_agent) for each task. Collect any ValueError raised. If any task fails, return tool_error with the error message before building any children. This ensures that credential failures are detected atomically before any children are constructed, restoring the pre-PR all-or-nothing behavior. Example:
# Pre-validate all task credentials before building children
for t in task_list:
try:
_resolve_task_delegation_routing(cfg, t.get("phase"), parent_agent)
except ValueError as exc:
return tool_error(str(exc))
…d curator The skill-authoring guide and curator prompt both reference descriptions as the primary discovery mechanism but never mentioned the 57-char system prompt truncation. Add explicit guidance: - Authoring guide: frontmatter docs, template comment, size limits, pitfall #3 with good/bad examples, verification checklist - Curator prompt: parenthetical noting the 57-char window when writing umbrella skill descriptions
… (re-review #3) The last_activity_at/description/provenance columns already live in SCHEMA_SQL and the column reconciler; existing DBs heal via the reconciler, but the version stamp must advance so downgrade/upgrade tooling sees the new layout. No version-literal test assertions exist (tests compare against the imported constant).
1 participant
What does this PR do?
Adds config-driven phase-based routing for delegated subagents, so orchestrators can pass a semantic
phasetodelegate_taskand Hermes can select the configured provider/model/effort for that phase while preserving the existing default fallback behavior.This keeps routing deterministic and cache-friendly: the model only passes a phase label, while concrete provider/model credentials stay in
config.yaml.Related Issue
Refs NousResearch#54839
Related work checked before opening this PR:
phase, and Hermes maps that phase to config-owned routing. That keeps model/provider choice out of the model output while still allowing orchestrators to route repeatable workflow phases.Type of Change
Changes Made
tools/delegate_tool.py— adds the optionalphaseparameter, resolvesdelegation.phase_assignments, and preserves backwards-compatible fallback to the current delegation config.hermes_cli/config.py— documents the new defaultdelegation.phase_assignmentsconfig key.run_agent.py— preservesphasewhen live delegate dispatch converts tool calls into child tasks.tools/async_delegation.pyandtools/process_registry.py— carry/render mixed async child metadata when a batch uses different routed models/providers.tests/tools/test_delegate.py— covers phase routing, invalid config warnings, fallback behavior, and provider/direct-endpoint precedence.tests/tools/test_async_delegation.py— covers async metadata for mixed routed child tasks.website/docs/user-guide/configuration.mdandwebsite/docs/user-guide/features/delegation.md— add user-facing configuration and usage docs.How to Test
python -m pytest tests/tools/test_delegate.py tests/tools/test_async_delegation.py -qgit diff --check origin/main...HEADChecklist
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
N/A — this PR does not add a skill.
Screenshots / Logs
Targeted verification:
Mirror-of: NousResearch#55885
NousResearch#55885