fix(delegation): resolve Nous auth before direct endpoint - #61499
fix(delegation): resolve Nous auth before direct endpoint#61499TurgutKural wants to merge 4 commits into
Conversation
Related: #32068 (open — same |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Overview
- Resolves Nous auth before direct endpoint in delegation
- Small change (+36/-1)
Looks Good
- Clean auth resolution fix
- No security concerns
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the delegation credential path and adding a focused regression test.
Problems
tools/delegate_tool.py:3035currently implements the documented direct-endpoint contract: a configureddelegation.base_urlreturns that URL anddelegation.api_key(tools/delegate_tool.py:3035-3079). The proposed condition bypasses that branch for Nous, and the subsequent resolver call does not receive either explicit value (tools/delegate_tool.py:3095). This changes an explicit endpoint/key into runtime-selected credentials, conflicting withwebsite/docs/user-guide/configuration.md:1939-1945.- The literal set omits the registered Nous aliases
nous-portalandnousresearch(plugins/model-providers/nous/__init__.py:41), so equivalent configurations take different credential paths.
Suggested changes
- Establish the intended precedence before changing this branch. If direct endpoints remain authoritative, use
delegation.providerwithoutdelegation.base_url/delegation.api_keyfor runtime Nous auth and test that supported path. - If an exception is intended, canonicalize provider identity and document/test endpoint, key, and alias behavior.
This is an automated hermes-sweeper review.
|
Agreed on the explicit-endpoint precedence issue. I pushed Updated behavior:
Also updated the Delegation docs and added regression coverage for explicit key precedence, runtime-key fallback, and aliases. Validation run locally on the PR worktree:
|
a7c8495 to
6d1cd1b
Compare
|
Follow-up: rebased the PR branch onto current Additional PR-branch validation after rebase:
|
|
Rebased onto current Validation: |
f4a6a01 to
ada4ebd
Compare
|
Rebased onto current |
ada4ebd to
2f8780a
Compare
Deep upstream-already-fixed analysis — verdict: STILL_OPENI read What the PR intendsWhen to so a Nous+base_url delegation falls through to Why upstream/main still has the gapIn
Is there a generic "if no api_key, resolve from hermes auth" path?No. The only runtime-auth resolution in this file is the Conclusion
Recommendation: Keep PR #61499 open and merge it. Two small hardening suggestions for the PR author: (a) the allowlist should match the stated intent — add |
2f8780a to
39adf2d
Compare
Thanks for isolating the delegation credential path and adding a regression test. The core mechanism does close the
On #32068: that 401 has a different root cause ( |
187daf1 to
9a1ebb6
Compare
f833cd7 to
b724f6f
Compare
b724f6f to
2ac2a20
Compare
2ac2a20 to
7964d8a
Compare
|
Rebased onto current upstream/main (7965462). The CI failure was a pre-existing vercel sandbox test issue now fixed on main — not related to this PR's delegation auth changes. All four prior review items remain addressed. |
7964d8a to
260809f
Compare
260809f to
d3ab685
Compare
d3ab685 to
0c1a076
Compare
0c1a076 to
3ed1843
Compare
3ed1843 to
fa0850a
Compare
fa0850a to
fb561d2
Compare
|
Rebased onto current upstream/main (395c70d). All required checks pass (47/47 pass+skip, no pending, no failures). The delegation auth resolution (registered Nous aliases incl. nous-portal/nousresearch, explicit |
…dence
Address GottZ review (4 items):
1. Alias set: replaced hard-coded {nous, nous-research} with registered
aliases {nous, nous-portal, nousresearch} from the provider plugin.
2. Precedence: explicit delegation.api_key now wins over runtime auth —
an explicit key means 'use this direct endpoint with this key'.
Runtime auth only fires for Nous-family providers WITHOUT explicit key.
3. Docs: rewrote docstring with numbered precedence list matching the
actual code behavior.
4. Coverage: added tests for primary path (Nous + base_url, no api_key
→ runtime auth), explicit-key-wins path, and registered alias coverage.
Summary
provider: nouswith an explicitdelegation.base_urland no explicitdelegation.api_keycan use the runtime Nous auth resolver.base_url + api_keyprecedence for intentionally configured direct endpoints.Why
When
delegation.providerselects Nous anddelegation.base_urlis present without a delegation-specific key, the direct custom-endpoint path bypasses runtime Nous credential resolution and fails even though the main/default runtime is authenticated. Explicit delegation keys continue to win by design; this PR targets the missing-key fallback rather than replacing configured keys.Validation
python -m pytest tests/tools/test_delegate.py::TestDelegationCredentialResolution -qpython -m pytest tests/tools/test_delegate.py tests/run_agent/test_auth_provider_failover.py -qgit diff --checkNotes
base_url + api_keyremains on the existing direct-endpoint path.