Skip to content

fix(delegation): honor documented delegation.api_mode on the provider-resolution path - #74254

Open
MahdiHedhli wants to merge 2 commits into
NousResearch:mainfrom
MahdiHedhli:fix/delegation-api-mode-provider-path
Open

fix(delegation): honor documented delegation.api_mode on the provider-resolution path#74254
MahdiHedhli wants to merge 2 commits into
NousResearch:mainfrom
MahdiHedhli:fix/delegation-api-mode-provider-path

Conversation

@MahdiHedhli

Copy link
Copy Markdown
Contributor

delegation.api_mode is documented (user-guide delegation.md, first config example) as a config-level transport override alongside delegation.provider, and _resolve_delegation_credentials() parses it — but it is only honored on the direct-endpoint (delegation.base_url) branch. On the provider-resolution branch the key is silently discarded and the child always gets the runtime-resolved api_mode, so the documented example is a no-op: a user forcing api_mode: anthropic_messages for a provider whose default resolves to chat_completions gets 404s with no hint why.

This change makes the provider branch honor an explicitly configured delegation.api_mode when it is one of the supported override transports (chat_completions, codex_responses, anthropic_messages) and the provider is not a native-SDK provider (bedrock/vertex/google, whose wire protocol is fixed by the bundle — the same exclusion the base_url branch already applies). When the key is set but cannot be honored, a warning is logged instead of a silent drop.

Scope: config-level only — no new tool parameters, no per-call or per-task selection. Behavior is unchanged for every config that does not set delegation.api_mode, and the existing base_url-branch semantics (including invalid-value fallback to URL detection) are untouched and still covered by their existing tests.

Precedence note: if #6647 (derive delegation api_mode from the delegation model) lands, the natural composition is that an explicit delegation.api_mode beats auto-derivation — same "explicit config wins" rule the base_url branch documents.

Tests: four regression tests added in TestDelegationCredentialResolution (explicit override honored; unsupported value keeps the resolved mode + warns; native-SDK provider ignores the override + warns; no api_mode set → unchanged). Full tests/tools/test_delegate.py: 164 passed. Docs comment updated to state the valid values and the override semantics.

Fixes #74252.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/delegate Subagent delegation area/config Config system, migrations, profiles labels Jul 29, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing the provider-path omission; current main does parse delegation.api_mode but returns runtime.get("api_mode") unchanged on that branch (tools/delegate_tool.py:3504,3599).

Problems

  • The new native-SDK guard is based on the configured provider spelling, not the resolved provider. That leaves supported aliases unprotected: Bedrock accepts aws, aws-bedrock, amazon-bedrock, and amazon (plugins/model-providers/bedrock/__init__.py:21-28), while Vertex accepts google-vertex, vertex-ai, and gcp-vertex (plugins/model-providers/vertex/__init__.py:65-73) and resolves to canonical vertex (hermes_cli/runtime_provider.py:1722-1743). An override must not become valid merely because an alias was used.
  • The new native-provider test covers only canonical bedrock, so this alias-dependent behavior is not exercised.

Suggested changes

  • Apply the exclusion after runtime resolution using the canonical resolved provider/runtime, and add alias regression tests (at minimum aws and google-vertex).
  • Relocate the test insertion: its current anchor, test_missing_config_keys_inherit_parent, was removed by test-pruning commit 6b81590c55.

Automated hermes-sweeper review.

@MahdiHedhli
MahdiHedhli force-pushed the fix/delegation-api-mode-provider-path branch from a9a1057 to 5cff8f2 Compare July 30, 2026 17:48
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 30, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

One PR addresses Issue #74252. #74254 changes the provider-resolution path to honor supported explicit delegation.api_mode overrides, preserves resolved transports when overrides are invalid or unsafe, and adds documentation and regression coverage for native-SDK provider aliases.

Related pull requests

Suggested consolidation

Keep #74254 open with a salvage path focused on the provider-path override, canonical resolved-provider guard, alias regression tests, and aligned documentation. This follows the automated keep_open verdict while recognizing that the current diff incorporates its requested aws and google-vertex coverage; there are no competing PRs to close as duplicates.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I74252(["issue #74252 (open)"])
    P74254["PR #74254 (open)"]
    P74254 -->|best fix| I74252
    class I74252 open
    class P74254 open
    class P74254 best
    class P74254 target
    click I74252 "https://github.com/NousResearch/hermes-agent/issues/74252"
    click P74254 "https://github.com/NousResearch/hermes-agent/pull/74254"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

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

…-resolution path

delegation.api_mode is documented (user-guide delegation.md, first config example) as a
config-level transport override alongside delegation.provider, and
_resolve_delegation_credentials() parses it — but only the direct-endpoint
(delegation.base_url) branch honors it. On the provider-resolution branch the key was
silently discarded and the child always got the runtime-resolved api_mode, making the
documented example a no-op: forcing api_mode: anthropic_messages for a provider whose
default resolves to chat_completions produced 404s with no hint why.

The provider branch now honors an explicitly configured delegation.api_mode when it is one
of the supported override transports (chat_completions, codex_responses,
anthropic_messages) and the provider is not a native-SDK provider (bedrock/vertex/google,
whose wire protocol is fixed by the bundle - the same exclusion the base_url branch already
applies). When the key is set but cannot be honored, a warning is logged instead of a
silent drop.

Config-level only: no new tool parameters, no per-call or per-task selection. Behavior is
unchanged for configs that do not set delegation.api_mode; base_url-branch semantics are
untouched. Docs comment updated to match; four regression tests added in
TestDelegationCredentialResolution.
…d provider

Review follow-up: the exclusion previously keyed on the configured provider spelling,
which registered aliases slip past (bedrock: aws/aws-bedrock/amazon-bedrock/amazon;
vertex: google-vertex/vertex-ai/gcp-vertex). The guard now applies after runtime
resolution using the canonical resolved provider, with the configured-spelling check
retained as a fallback for named-custom resolutions. An override cannot become valid
merely because an alias was used.

Adds alias regression tests (aws -> bedrock, google-vertex -> vertex); rebased onto
current main, which relocated the earlier tests past the pruned anchor.
@MahdiHedhli
MahdiHedhli force-pushed the fix/delegation-api-mode-provider-path branch from 5cff8f2 to 61c6685 Compare August 3, 2026 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/delegate Subagent delegation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

delegation.api_mode is silently ignored when delegation.provider is set

4 participants