feat(delegation): per-task model selection and profile identity for delegate_task - #98031
CaptainPickard wants to merge 5 commits into
Conversation
alt-glitch
added
type/feature
Overall: Well-gated feature — per-task model selection and profile identity for What it does
Non-blocking / consider before merge
Please use your judgment — the traversal guard is the only blocking-adjacent item. |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
I reviewed exact head 4c37a70d976c8b2a9144fd6e412f1fc791c1214c against current main@4f22543509d1b91dc45bcb369447126c5eb14fb7, including the full 16-file diff / 9-commit history, the behavior-critical child-construction and session-DB paths, the new tests, exact-head check state, and the overlapping delegation/profile/session PRs.
There is good engineering in this branch. In particular, the earlier traversal finding was actually absorbed; model switching now anchors on the effective delegation provider rather than blindly on the parent; cross-provider switches clear stale ACP/pin transport fields; and the model/profile schema remains default-off. Those are real improvements.
I do not think this exact object is safe to land yet, though. The remaining blockers are architectural/correctness issues rather than polish.
P1 — batch override/profile preflight is not atomic; a later bad task can orphan earlier constructed children
At this head, delegate_task() resolves each task's model/profile and then immediately constructs that child in the same for i, t in enumerate(task_list) loop (tools/delegate_tool.py, around the new per-task routing block at ~4246 and _build_child_preserving_parent_tools at ~4332). A later task can then fail model resolution, profile lookup, allowlist validation, or profile-model resolution and return tool_error(...) after earlier children already exist.
That is load-bearing because child construction is not inert: _build_child_agent() opens a dedicated SessionDB and registers the child on parent_agent._active_children. The early return does not run those already-constructed children through the normal lifecycle/cleanup path.
There is already a narrower concurrent implementation of this exact slice in #98128 / #97653. Its two-pass preflight is explicit about the same failure mode: resolve every task before any child construction because construction opens the DB handle and registers _active_children. That PR is overlap, not a duplicate of this whole profile-identity feature, but this branch needs the same atomic boundary regardless of which vehicle survives.
Required fix: preflight all model/profile inputs for the whole batch before constructing task 0: profile syntax/existence/allowlist, effective profile provider+model, explicit task model/provider, and the final credential bundle. Then build children from immutable pre-resolved task plans. Add a public-path regression where task 0 is valid and task 1 is invalid and assert zero child constructors ran / _active_children is unchanged (and no child DB ownership is left behind).
P1 — the selected profile's configured provider is loaded but never applied
_load_profile_identity() reads both:
model.default→result["model"]model.provider→result["provider"]
and the PR/schema promise that a profile's model/provider config is used when not explicitly overridden.
But the dispatch path only consumes _profile_identity["model"]:
_profile_model = _profile_identity.get("model")
if _profile_model and allow_model_selection:
task_creds = _resolve_task_model_creds(_profile_model, parent_agent, creds)_profile_identity["provider"] is never used. _resolve_task_model_creds() then anchors on base_creds["provider"], which is the delegation/parent route, not the selected profile route.
So a parent/delegation running on provider A can select a profile configured for provider B and still resolve that profile's model against A. That can mean the wrong endpoint/credential/billing route or a false model-resolution failure. The loader tests prove the provider string was read, but not that the child actually receives it.
Required fix: resolve a selected profile's provider+model as one routing unit through the canonical runtime resolver, with the documented precedence (explicit task override > profile config > delegation config > parent). Add a dispatch-level regression where parent/delegation provider != profile provider and assert the child gets the expected provider, model, base URL, API key/API mode, and transport fields—not merely the loader dictionary.
P1 — the new model-facing profile input still has a known filesystem exception path
Commit 51ea414ba4abbdc8b19f796168b074fc6d8aef1f records the remaining bug directly: a regex-valid profile name longer than the filesystem component limit can raise OSError: ENAMETOOLONG at profile_path.is_dir() because that probe sits outside the read-side exception handling.
I would not defer that as a follow-up. This PR is the code introducing a model-facing profile-name → filesystem path boundary, so rejecting hostile/invalid names belongs to this boundary. #90331 is adjacent profile-management work and already uses Hermes' canonical normalize_profile_name / validate_profile_name path rather than defining a second name law.
Required fix: reuse the canonical profile-name validation/bounds (or equivalently make the filesystem probe fail closed) before touching disk, and add a delegate_task regression for an overlong alphanumeric name. It should return a bounded tool error and construct nothing.
P1 — provenance/ownership needs recomposition; two substantial slices already have open upstream owners
The 9-commit branch has grown well beyond the model/profile feature in the PR description. Two of those additions are already open upstream as their own reviewable objects:
- #91252 (
teknium1, exact head7e60ebc5d495efa1fba8c25201727c54792ca3cb) is the existingforkimplementation porting MoonshotAI/kimi-code#3007. This branch's fork commit explicitly ports that upstream head and carries the same module/tests/schema behavior. That slice is duplicate/derived work; #91252 remains the original repository owner. - #91271 (
teknium1, exact head037ae2724c99d354392a14d1aff92e63e4755f3a) is the existing per-delegation usage ledger inspired by Copilot CLI. This branch carries that ledger plus a usefulprofileattribution extension. The profile column is complementary work; the underlying ledger is not new ownership. - #98128 / #97653 (
DavidMetcalfe) directly overlaps the per-dispatch model/provider routing slice and already supplies the atomic preflight this branch is missing. It is not a duplicate of profile identity, but the two cannot land independently indelegate_tool.pywithout choosing/recomposing one routing owner.
Please preserve those edges and authorship rather than landing rewritten copies as a single omnibus commit train. The clean shape is to drop/rebase onto the surviving upstream owners (or cherry-pick the original objects with authorship intact) and keep only this PR's genuinely complementary profile-identity delta. External source credit to Kilo, Moonshot/Kimi, and Copilot CLI should remain as well.
There are additional adjacent/complementary edges worth preserving during that rebase rather than flattening them into duplicates: #95534 (bounded reusable roles/personas), #95578 (delegate anti-propagation prompt guidance), and #92001 (explicit profile-home ownership for delegation persistence). In particular, if #95578 lands first, the profile-identity prompt path must not replace/drop its security guidance; if #92001 lands first, new child persistence must reuse its explicit-home boundary rather than recreate ambient storage selection.
P1 landing gate — this exact object has no hosted acceptance and is far off current main
Current topology at review time:
- PR head:
4c37a70d976c8b2a9144fd6e412f1fc791c1214c - current main:
4f22543509d1b91dc45bcb369447126c5eb14fb7 - merge base:
1e21fe862421081e14af4564076af74bc58c050b - branch: 9 ahead / 154 behind,
mergeable=false - exact head: 0 workflow runs, 0 check runs, 0 commit statuses
The local targeted tests are useful evidence, but they cannot substitute for hosted acceptance of the submitted object—especially after the branch has accreted fork, usage, and session-persistence behavior not reflected in the original test-plan summary. Recompose on current main, then reacquire exact-head CI/Docker/Nix (and every submitted commit must be green, not only a merge ref/head snapshot).
The size/ownership gate also still fails: this PR edits tools/delegate_tool.py past ~5.5K lines, cli.py around ~14K, and agent/agent_init.py past ~2.9K. New routing/identity/persistence authority should move behind bounded owners rather than add another concern to those godfiles. tools/delegation_fork.py is the right shape of extraction; it just already belongs to #91252's lineage.
Separate the session-title side channel from this feature
The late session-title commits are also orthogonal to per-task model/profile identity and currently write sessions.title with raw sqlite3 from delegate_tool.py, including a fallback that can synthesize a profile DB path and finally falls back to /home/hermeswebui/.hermes/profiles/io/state.db.
That creates a second persistence authority beside SessionDB exactly while session/profile ownership is being hardened elsewhere (#92001 and the broader session-lifecycle work). The pre-run UPDATE is also necessarily best-effort because the row may not exist yet, then _finalize_child_results repeats the write.
I would remove that ride-along from this PR and give descriptive subagent titles their own canonical session-owner change, or route it through the existing session title API with real lifecycle/profile-home tests. It should not be coupled to whether profile identity lands.
What I would re-review
A much cleaner next object would be: current-main rebase; fork and usage work removed/recomposed onto #91252/#91271 with original credit; one owner selected for the #97653 model-routing slice; all task routes/profile identities preflighted before any child construction; profile provider actually honored; canonical profile-name validation; bounded module ownership; and fresh exact-object hosted CI for every commit.
The core product idea is useful, and the conservative defaults plus the resolver hardening are solid. The branch now needs the same care at the composition boundary that it already applies inside the resolver. Once those ownership and atomicity seams are closed, this will be much easier to reason about and much safer to merge.
|
Thanks for the thorough review. The bug findings are well-taken and we agree with the composition direction. Here is our plan. What we are doing: full rebase + recomposeWe will rebase on current Commits being dropped from this PR
Commits staying (the profile-identity delta)
Fixes for each P1P1 #1 — atomic batch preflightAgreed. The current loop constructs children inline with resolution, so a later failure orphans earlier children that already have open SessionDB handles and Fix: Two-pass design. Pass 1 resolves every task's effective model, provider, profile identity, allowlist status, and credential bundle into an immutable plan list. If any task fails resolution, return a bounded tool error with zero child construction. Pass 2 iterates the pre-resolved plans and constructs children. We will add a regression test where task 0 is valid and task 1 has an invalid profile, then assert We will also review #98128 (DavidMetcalfe) and coordinate so we do not land conflicting routing owners in P1 #2 — profile provider loaded but never appliedConfirmed bug. Fix: Resolve a selected profile's provider+model as one routing unit through the canonical resolver. Precedence: explicit task override > profile config > delegation config > parent. The preflight pass will produce the final Regression test: parent/delegation provider A, profile configured for provider B, no explicit task override. Assert the child receives provider B's endpoint, credentials, and transport fields, not A's. P1 #3 — ENAMETOOLONG filesystem exceptionAgreed that name-to-filesystem-path validation belongs to this boundary since this PR introduces the model-facing profile name input. Fix: Replace the ad-hoc regex guard with Hermes' canonical Regression test: delegate with an overlong alphanumeric profile name, assert bounded error, zero child construction. P1 #4 — provenance/ownershipAcknowledged. The fork slice is #91252's work (teknium1, porting kimi-code#3007). The usage ledger is #91271's work (teknium1, Copilot CLI port). We are dropping both from this PR (see above). For the #97653/#98128 model-routing overlap (DavidMetcalfe): #97653 is the feature issue, #98128 is the PR. We will coordinate on a single routing owner rather than landing parallel implementations. If #98128 lands first, we rebase onto it. If ours lands first, we will ensure the preflight boundary is compatible. Adjacent edges we will respect during rebase:
P1 #5 — landing gateAgreed. After the rebase, we will push the exact head and verify hosted CI/Docker/Nix checks run green on every commit, not just the head snapshot. The branch will be current-main-based with a clean mergeable state before requesting re-review. On the godfile concern: we agree Timeline
Appreciate the review. The core resolver hardening and default-off schema will carry forward into the clean object. |
…elegate_task
Two opt-in features for delegate_task, both gated behind config flags
(default off) to preserve the existing subagent contract:
1. Per-task model selection (delegation.allow_model_selection):
Each task in a batch can name a model ('opus', 'gpt-5', 'glm', or a
full 'vendor/model' slug). Resolution reuses the existing model_switch
pipeline (same as the /model command) so names are matched leniently
and the provider is resolved, not dictated. Unresolvable names return
a clear per-task error instead of silently falling back.
2. Per-task profile identity (delegation.allow_profile_identity):
Each task can name a Hermes profile. The child loads that profile's
SOUL.md, IDENTITY.md, and AGENTS.md as its system prompt identity,
and reads model/provider from its config.yaml when not explicitly
overridden. The child becomes the named bot rather than a generic
subagent. Useful for multi-role workflows (code reviewer, test
validator, security auditor) in a single delegate_task batch.
Both schema fields only appear in the tool definition when the
corresponding flag is enabled, keeping the tool surface minimal when off.
The flag-off code path is byte-identical to prior behavior.
Validate profile_name against ^[A-Za-z0-9_-]+$ before path joining to prevent traversal attacks (e.g. ../../.ssh) from escaping the profiles root. Also clarify in docs that a profile's config.yaml model fallback requires allow_model_selection to be enabled. Addresses AI code review feedback on PR NousResearch#98031.
…odel Add Kilo-Org/kilocode#11786 attribution to _get_allow_model_selection() and _resolve_task_model_creds() docstrings, noting our two bugfixes (provider anchoring, stale ACP clearing) over the upstream version. Create tests/tools/test_delegate_model_selection.py mirroring the kilocode branch test file name/structure with integration-style tests using real switch_model() calls (9 tests). Add convergence note to delegation.md documenting the shared origin with the kilocode-port branch. Both implementations use the same config key (allow_model_selection), resolver function (_resolve_task_model_creds), and switch_model() pipeline. This convergence prevents perpetual merge conflicts on future upstream merges. Our profile-identity half remains the unique differentiator with no upstream equivalent.
… integration tests Add tests/tools/test_delegate_test_gap.py with 26 tests + 48 subtests across 4 categories closing the audit-identified test gaps: 1. Property-based path traversal (Hypothesis): Fuzz _load_profile_identity with arbitrary strings, unicode, null bytes, control chars, path separators across platforms, and extremely long names. Verifies the ^[A-Za-z0-9_-]+$ guard rejects all adversarial inputs before filesystem access. 2. allow_model_selection=false gating regression: Verifies that when model selection is disabled but profile identity is enabled, the schema advertises profile but NOT model. Tests flag getter isolation and profile-model suppression at the dispatch level. 3. Fallback chain semantics: Verifies fail-loudly contract: unresolvable models raise ValueError (no silent fallback), None/empty/whitespace model names return base creds unchanged, base creds not mutated on resolution failure, error message surfaced in ValueError. 4. Integration tests with mock child spawns: Exercises delegate_task end-to-end with mocked AIAgent to verify profile identity appears in child system prompt, generic preamble used without profile, fork snapshot attached when fork=True, explicit model takes precedence over profile model, and usage ledger populated after delegation. Discovered follow-up bug (out of scope, test-only card): _load_profile_identity raises OSError ENAMETOOLONG for valid-regex names >255 bytes because profile_path.is_dir() is not exception-wrapped. Recommend a separate one-line fix card. No production code changes. All 156 existing tests still pass.
…ation, profile propagation P1 fixes from PR NousResearch#98031 review: 1. Atomic two-pass batch preflight: resolve all tasks' model/profile/creds into immutable plans BEFORE constructing any child. A mid-batch failure returns tool_error with zero children constructed, preventing orphaned SessionDB handles and _active_children registrations. 2. Profile provider routing: new _resolve_profile_model_creds() resolves a profile's model+provider as one routing unit against the PROFILE's provider, not the parent's. Precedence: explicit task model > profile config (model+provider) > delegation config > parent inherit. 3. ENAMETOOLONG guard: new _validate_profile_name() checks regex + length (<=255) before any filesystem access. try/except OSError around profile_path.is_dir() as defense-in-depth. 4. _profile_name added to _load_profile_identity() return dict. child._delegate_profile set in _build_child_agent from the identity dict. Regression tests: - TestAtomicBatchPreflight: task 0 valid + task 1 invalid = zero children - TestProfileProviderRouting: profile provider used, not parent provider - TestOverlongProfileName: 256-char name returns None, no OSError - All 3 pre-existing test failures fixed (54/54 pass)
4c37a70 to
7d1ad50
Compare
|
Updated branch pushed. Summary of changes since the review: Branch recomposed
P1 fixes applied (commit 7d1ad50)P1 #1 -- atomic preflight: The dispatch loop is now two-pass. Pass 1 resolves every task's role, model, profile identity, profile model fallback, and credential bundle into immutable per-task plan dicts. Any resolution failure returns P1 #2 -- profile provider routing: New P1 #3 -- ENAMETOOLONG guard: New P1 #4 -- profile name propagation: Ownership
Test results54 passed, 0 failures across CI is pending on the new head. Will confirm once checks complete. |
4 participants
Summary
delegate_taskbatch can name a model ("opus", "gpt-5", "glm", or a full "vendor/model" slug), resolved leniently through the existingmodel_switchpipeline (same as/model). Provider is resolved, not dictated.SOUL.md,IDENTITY.md, andAGENTS.mdas its system prompt, and reads model/provider from itsconfig.yamlwhen not explicitly overridden. The child becomes the named bot rather than a generic subagent.Motivation
Multi-role workflows (e.g. dispatching a code reviewer, test validator, and security auditor in one
delegate_taskbatch) currently require every child to run on the same model with the same generic identity. This PR adds two independent opt-in capabilities:Model routing lets the agent fan work across different models in a single batch -- useful for comparing results across models, or routing specialized work to specialized models.
Profile identity lets each child adopt a named profile's identity files and model config, so a delegation batch can dispatch specialized bots with their own personas, rules, and model configurations.
Both flags are off by default because:
The model selection feature overlaps conceptually with the
kilocode-port/per-task-delegation-modelbranch, but this PR takes a different approach: it adds profile identity as a complementary feature and uses a unified code path. Happy to coordinate if there's interest in merging the approaches.Changes
tools/delegate_tool.py:_get_allow_model_selection()/_get_allow_profile_identity(): config-gated flag getters_resolve_task_model_creds(): resolves a per-task model name viamodel_switch.switch_model()(reuses the/modelcommand's resolution pipeline)_load_profile_identity(): loads a named profile'sSOUL.md,IDENTITY.md,AGENTS.md, andconfig.yamlmodel/provider_build_child_system_prompt(): accepts optionalprofile_identityto replace the generic subagent preamble with the profile's identity files_build_child_agent(): passesprofile_identitythrough to the system prompt builderdelegate_task(): acceptsmodelandprofileparams; per-task resolution in the child loop (gated)_build_dynamic_schema_overrides(): addsmodel/profilefields only when the corresponding flag is on; deep-copies tasks schema to avoid mutating the static schemamodel/profilethroughhermes_cli/config_defaults.py: addsallow_model_selectionandallow_profile_identitydefaults (bothFalse)website/docs/user-guide/features/delegation.md: documents both features with config examplestests/tools/test_delegate_per_task_overrides.py: 17 tests covering schema gating, flag getters, model resolution, profile identity loading, and system prompt constructionTest Plan
python3 -m pytest tests/tools/test_delegate_per_task_overrides.py -v)python3 -m pytest tests/tools/test_delegate.py -v)Notes for Reviewers
False._resolve_task_model_credsfunction reuses the existingmodel_switch.switch_model()pipeline rather than reinventing model resolution, so aggregator-aware aliasing, fuzzy matching, and cross-provider fallback all come for free.kilocode-port/per-task-delegation-modelbranch. Both features can be combined: a task can name bothmodelandprofile, with the explicit model taking precedence over the profile's config.DELEGATE_TASK_SCHEMAis never mutated.