Skip to content

fix(delegation): keep credential pools endpoint-coherent - #68240

Open
oferlaor wants to merge 4 commits into
NousResearch:mainfrom
oferlaor:fix/pr39862-endpoint-coherence
Open

fix(delegation): keep credential pools endpoint-coherent#68240
oferlaor wants to merge 4 commits into
NousResearch:mainfrom
oferlaor:fix/pr39862-endpoint-coherence

Conversation

@oferlaor

@oferlaor oferlaor commented Jul 20, 2026

Copy link
Copy Markdown

Summary

  • Prevent delegated children from sharing or leasing a credential pool unless it matches both the runtime provider and normalized endpoint.
  • Filter automatic lease selection by endpoint, including the post-refresh re-selection path.
  • Bind the child to the exact entry identified by the returned lease ID instead of the pool's shared mutable current() cursor; revalidate that exact entry before swapping and fail closed if it disappeared or mismatched.
  • Revalidate pool coherence immediately before lease binding while preserving named custom:<name> and legacy adapter compatibility.
  • Keep delegated fallback inheritance configurable via delegation.inherit_fallback_providers (default true; set false to pin children to the primary route).

Fixes #68237. Supersedes the historical endpoint-coherence attempt in #39862 and the temporary contributor-fork handoff Waritboo#2.

Current-main refresh

  • Base: acc614e72fa6b635a19e5eddbc29ed147147a090
  • Head: 9031c961825d846b1938384d7e05d27b4662a5c6
  • The mixed-pool review is addressed at selection time and the concurrent cursor race is covered by binding the exact leased ID.
  • Deferred single-use-token refresh keeps the same endpoint filter on re-selection.

Validation

  • python -m pytest tests/tools/test_delegate.py tests/agent/test_credential_pool_provider_boundary.py tests/agent/test_credential_pool.py tests/agent/test_credential_pool_lease_refresh_reselect.py -q -o 'addopts='140 passed
  • Ruff on changed files → passed
  • Python compilation → passed
  • git diff --check origin/main...HEAD → passed
  • Cursor-race regression advances pool.current() to a wrong-endpoint entry after lease acquisition and proves the child still binds the exact matching leased entry.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/delegate Subagent delegation provider/openai OpenAI / Codex Responses API area/auth Authentication, OAuth, credential pools sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 20, 2026
@PRATHAMESH75

Copy link
Copy Markdown
Contributor

Reviewed against upstream/main — this correctly fixes #68237 (delegated Azure child leasing a public-OpenAI pool). It targets the actual pre-fallback lease boundary the issue describes, and covers all four required behaviors:

  • Provider identity + endpoint match — the new _credential_pool_matches_runtime (tools/delegate_tool.py) calls credential_pool_matches_provider(pool, provider, base_url=base_url) and then additionally requires the child's normalized base_url to appear in the pool's entry endpoints (runtime_base_url/base_url). For the repro (openai-api child at an Azure endpoint vs a pool seeded at https://api.openai.com/v1), the provider names match but the endpoint check fails closed → the Azure key is no longer sent to api.openai.com.
  • custom ↔ custom: preserved — delegated through the existing credential_pool_matches_provider, and pinned by test_run_single_child_allows_matching_named_custom_pool.
  • Revalidate immediately before lease binding — the guard added in _run_single_child (before acquire_lease()) nulls out a mismatched child._credential_pool, so even a pool attached via another path can't rebind the child. Covered by test_run_single_child_skips_same_provider_wrong_endpoint_pool (asserts acquire_lease/_swap_credential not called).
  • Legacy adapter compatibility — non-str pool.provider and missing/non-callable entries() both return True, so lightweight/plugin/test pools that predate endpoint metadata are unaffected.

Both leak paths are closed: _resolve_child_credential_pool now guards the parent-pool-share branch and the load_pool() branch, and the caller at delegate_tool.py:1430 already passes effective_base_url, so resolution receives the endpoint. Signatures line up with current main (credential_pool_matches_provider accepts the base_url kwarg; _resolve_child_credential_pool already accepts effective_base_url).

One minor note (non-blocking): _credential_pool_matches_runtime fails closed when endpoint metadata is present but empty (return bool(endpoints) and expected in endpoints) — correct for production pools, and the legacy escape hatches above keep it from regressing adapters without metadata. No sibling code path in the issue is left uncovered, so no consolidation is needed here.

LGTM — this resolves the reported bug.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for targeting the real delegation credential-boundary gap. Current main still shares same-provider pools without an endpoint check at tools/delegate_tool.py:3462-3463, then leases and swaps an entry at tools/delegate_tool.py:1991-1996.

Problems

  • Blocking: the proposed pool guard accepts a pool when any entry matches (tools/delegate_tool.py:3239 in this PR), but _run_single_child then calls unfiltered acquire_lease() and swaps current() (tools/delegate_tool.py:1832-1837). CredentialPool.acquire_lease() selects across all available entries by lease count and priority, with no endpoint filter (agent/credential_pool.py:1940-1955). A mixed same-provider pool can therefore still lease a public-OpenAI entry for an Azure child.

Suggested changes

  • Bind only an endpoint-matching entry, or validate the exact leased entry before _swap_credential() and release it on mismatch.
  • Add a two-entry regression test where the nonmatching entry wins the normal lease selection.

Automated hermes-sweeper review.

Comment thread tools/delegate_tool.py
@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@oferlaor

oferlaor commented Aug 1, 2026

Copy link
Copy Markdown
Author

Rebased the fix locally on current upstream 57b1eb8c4d and addressed the blocking endpoint-selection review: the lease selection itself is now filtered to the child endpoint, so a mixed pool cannot bind an Azure child to a public OpenAI credential.

Focused verification: 119 passed (tests/tools/test_delegate.py, tests/agent/test_credential_pool_provider_boundary.py, tests/agent/test_credential_pool.py).

I could not update this PR branch because the available PAT lacks the workflow scope and GitHub rejects the rewritten ancestry: it includes .github/workflows/ci.yml from current upstream. The ready patch is retained locally at /root/.hermes/tmp/patches/0001-fix-delegation-lease-only-endpoint-matching-credenti.patch. With a token that has workflow, I will force-push the verified rebase with --force-with-lease.

@oferlaor

oferlaor commented Aug 1, 2026

Copy link
Copy Markdown
Author

Maintainer assistance requested: the corrected current-main rebase is verified locally, but my fork PAT cannot update the rebased branch because it lacks GitHub's workflow scope. Could a maintainer with workflow-authorized access apply/rebase the patch onto current main (or provide a maintainer-owned branch for it)?

Ready patch: /root/.hermes/tmp/patches/0001-fix-delegation-lease-only-endpoint-matching-credenti.patch
Verification: 119 focused delegation/credential-pool tests passed. The key correction filters the actual lease selection by endpoint, not merely the pool-level match.

@oferlaor
oferlaor force-pushed the fix/pr39862-endpoint-coherence branch from a85af2c to 4b83e2b Compare August 1, 2026 08:15
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

One open PR, #68240, addresses #68237 by making delegated credential-pool eligibility provider-and-endpoint aware and filtering the actual lease selection so an Azure child cannot receive a public OpenAI credential from a mixed pool.

Related pull requests

  • fix(delegation): keep credential pools endpoint-coherent #68240 best fix — (+168/-11) — n/a: The visible diff checks runtime coherence during pool resolution and immediately before leasing, then filters CredentialPool.acquire_lease() to endpoint-matching entries and adds a two-entry regression test in which the public OpenAI entry would otherwise win normal selection. This addresses the blocking contributor concern in the COMMENTED keep_open review that the earlier any-entry guard still allowed an unfiltered wrong-endpoint lease, while retaining compatibility for legacy pool adapters.

Suggested consolidation

Keep #68240 open with a salvage path: preserve the endpoint-filtered lease selection, pre-lease coherence guard, and mixed-pool regression coverage now visible in the diff, and obtain contributor confirmation that these changes resolve the blocking keep_open review. There are no competing PRs in this complex 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
    I68237(["issue #68237 (open)"])
    P68240["PR #68240 (open)"]
    P68240 -->|best fix| I68237
    class I68237 open
    class P68240 open
    class P68240 best
    class P68240 target
    click I68237 "https://github.com/NousResearch/hermes-agent/issues/68237"
    click P68240 "https://github.com/NousResearch/hermes-agent/pull/68240"
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: 11 kB of PR diffs, 5 kB of issue/PR text, 6 kB of discussion (6 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@oferlaor
oferlaor force-pushed the fix/pr39862-endpoint-coherence branch from 4b83e2b to 24e71b8 Compare August 3, 2026 19:33
@oferlaor

oferlaor commented Aug 3, 2026

Copy link
Copy Markdown
Author

Refreshed onto current main and the mixed-pool blocking review is now addressed by filtering the actual credential lease by normalized endpoint. The review thread is resolved; 125 focused tests plus Ruff/compile/diff checks pass. GitHub reports the branch mergeable. CI is awaiting maintainer approval: https://github.com/NousResearch/hermes-agent/actions/runs/30846277217

@oferlaor

oferlaor commented Aug 3, 2026

Copy link
Copy Markdown
Author

Refreshed onto current main at ad2c7af86a28075296a1888b16a8fc10f8dedd76; live head is 826fc47c2ad1228216f55dbfe37191391bba96d9. 134 focused tests passed with Ruff/compile/diff checks. GitHub reports zero unresolved review threads. Fork CI requires upstream admin approval: https://github.com/NousResearch/hermes-agent/actions/runs/31370596515

@alt-glitch alt-glitch added area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard needs-decision Awaiting maintainer decision before any implementation and removed sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 3, 2026
@oferlaor
oferlaor force-pushed the fix/pr39862-endpoint-coherence branch 2 times, most recently from 6a297c9 to 065df34 Compare August 10, 2026 08:12
@alt-glitch alt-glitch added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Aug 10, 2026
@alt-glitch alt-glitch removed comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard needs-decision Awaiting maintainer decision before any implementation labels Aug 10, 2026
@oferlaor
oferlaor force-pushed the fix/pr39862-endpoint-coherence branch from d7d57d7 to 826fc47 Compare August 10, 2026 08:33
@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data and removed sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data needs-decision Awaiting maintainer decision before any implementation labels Aug 10, 2026
@oferlaor
oferlaor force-pushed the fix/pr39862-endpoint-coherence branch from 826fc47 to 9031c96 Compare August 18, 2026 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists provider/openai OpenAI / Codex Responses API sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/delegate Subagent delegation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Delegated Azure child can lease public OpenAI pool and send credential to wrong endpoint

5 participants