Skip to content

fix: keep delegate credential pools provider-coherent - #39862

Open
Waritboo wants to merge 1 commit into
NousResearch:mainfrom
Waritboo:fix/delegation-runtime-pool-coherence-pr
Open

fix: keep delegate credential pools provider-coherent#39862
Waritboo wants to merge 1 commit into
NousResearch:mainfrom
Waritboo:fix/delegation-runtime-pool-coherence-pr

Conversation

@Waritboo

@Waritboo Waritboo commented Jun 5, 2026

Copy link
Copy Markdown

Summary

  • Prevents delegated child agents from reusing or leasing a parent credential pool when its provider does not match the child runtime provider.
  • Keeps credential-pool provider coherence across the delegated runtime path.
  • Adds regression coverage for mismatched parent pools and child lease acquisition/release.

Test Plan

  • python -m pytest tests/tools/test_delegate.py -k 'credential or provider or runtime or pool' -o 'addopts=' -q
  • python -m pytest tests/tools/test_delegate.py -o 'addopts=' -q
  • python -m py_compile tools/delegate_tool.py tests/tools/test_delegate.py
  • git diff --check origin/main...HEAD
  • Direct provider smokes: OPENCODE_FC_OK, CODEX_FC_OK
  • Fresh controlled contaminated-parent delegation e2e: DELEGATE_FRESH_E2E_OK
  • Log invariant: no OpenAI Codex child session used Opencode base URL (bad_count=0)

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/delegate Subagent delegation area/auth Authentication, OAuth, credential pools labels Jun 5, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Same root-cause family as open PR #35499 (guard child credential pool binding by provider identity) and issue #7833. Several overlapping delegate credential-pool coherence fixes in flight. Flagging for maintainer dedup.

@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 tracing the parent-pool rebinding path. The reported non-custom mismatch remains on current main: tools/delegate_tool.py:3009-3010 can return a parent pool without checking the pool's provider, then tools/delegate_tool.py:1385-1389 attaches it after child construction and :1770-1778 leases it.

Problems

  • The new exact-string matcher at tools/delegate_tool.py:1366 in this PR would reject valid named custom pools. Current agent/credential_pool.py:448-483 intentionally matches a child runtime provider="custom" to provider="custom:<name>" only when its base URL resolves to the same endpoint key.
  • The resolver patch predates the current custom-endpoint branch at tools/delegate_tool.py:2971-3007; that branch must remain endpoint-scoped.

Suggested changes

  • Reuse credential_pool_matches_provider(..., base_url=...) at both resolver and lease boundaries rather than adding a local exact comparison.
  • Add a lease-path regression for a matching custom:<name> pool, alongside the mismatched-provider case.

This is an automated hermes-sweeper review.

Comment thread tools/delegate_tool.py

child_pool = getattr(child, "_credential_pool", None)
leased_cred_id = None
if child_pool is not None and not _pool_matches_provider(

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.

This exact provider-name check regresses named custom pools on current main: a child runtime uses provider="custom", while a correctly scoped production pool is keyed custom:<name>. Please reuse agent.credential_pool.credential_pool_matches_provider(..., base_url=child.base_url) so only the matching custom endpoint is accepted.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Addressed in the current-main companion PR Waritboo#2 (a85af2c49): it reuses credential_pool_matches_provider(..., base_url=...), preserves matching custom:<name> pools, and adds lease-path coverage. It also extends coherence to same-provider/different-endpoint pools (Azure vs public OpenAI).

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 14, 2026
@oferlaor

Copy link
Copy Markdown

Extended this PR with a current-main, endpoint-aware follow-up:

  • Companion PR into this branch: fix(delegation): keep credential pools endpoint-coherent Waritboo/hermes-agent#2
  • Commit: a85af2c4900d23fb6bc40efaf5b63c35ba22f55e
  • Adds same-provider endpoint validation so an openai-api pool for public OpenAI cannot overwrite an Azure child runtime.
  • Uses credential_pool_matches_provider(..., base_url=...), preserving named custom pools as requested in review.
  • Rechecks coherence immediately before lease binding.
  • Validation: 161 passed; Ruff, py_compile, and diff checks passed; production CredentialPool probe rejects public-OpenAI-for-Azure and accepts matching Azure.

I could not push directly to the contributor fork despite maintainerCanModify=true because GitHub returned 403, so the companion PR is the verifiable handoff that updates this PR once merged.

@oferlaor

Copy link
Copy Markdown

Tracking issue for the same-provider/different-endpoint credential-boundary bug: #68237

The local/current-main endpoint-aware validation is covered by companion PR Waritboo#2 and is intended to extend this PR.

@oferlaor

Copy link
Copy Markdown

A directly mergeable current-main replacement now covers the surviving provider + endpoint coherence invariant and the Azure/public-OpenAI regression:
#68240

Tracking issue: #68237

This avoids depending on a merge into the stale contributor-fork head.

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 P2 Medium — degraded but workaround exists 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 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.

4 participants