feat(api): extend /api/credentials/pool with base_url + pool management endpoints - #66972
feat(api): extend /api/credentials/pool with base_url + pool management endpoints#66972DeamonDev888 wants to merge 1 commit into
Conversation
This carries a byte-identical credential_security.py hunk from focused open PR #66970, alongside a much broader pool-API/OAuth/probe expansion. Please split the duplicated validator work from the independent API scope; related broader work: #62467, #54524, and #54011. |
|
Good catch on the duplicate. Fixed in commit To clarify the scope: this PR only adds REST API endpoints ( |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for extending the existing credential-pool contract. Current main only has collection list/add and index deletion (hermes_cli/web_server.py:12058-12161), so the pool-management premise is real.
Problems
- The final diff is broader than the stated focused API scope: it adds OAuth login, status, logout, summary, and probe routes (
hermes_cli/web_server.py:12386-12690), while the dashboard client currently exposes only the existing three calls (web/src/lib/api.ts:1087-1106). Please split or explicitly scope these additions. thread.join()athermes_cli/web_server.py:12382is called from asyncpool_oauth_login()at:12417; it can block the dashboard event loop for the OAuth timeout. Move that blocking work behindawait asyncio.to_thread(...).- The conditional validator path has incompatible exception behavior: the fallback raises
HTTPException(:6496-6506), but #66970's imported validator raisesValueError; the route callers do not translate it to HTTP 400. - No tests are added, while current credential-pool coverage only exercises list/add/remove (
tests/hermes_cli/test_dashboard_admin_endpoints.py:221-254).
This is an automated hermes-sweeper review.
|
|
||
| thread = threading.Thread(target=_runner, name=f"oauth-login-{provider}", daemon=True) | ||
| thread.start() | ||
| thread.join(timeout=max(body.timeout or 180.0, 1.0) + 5.0) |
There was a problem hiding this comment.
This synchronous join runs on the async request thread because pool_oauth_login() calls this helper directly. A browser OAuth flow can block the dashboard event loop for up to timeout + 5 seconds; run the blocking helper through await asyncio.to_thread(...) instead.
| try: | ||
| from hermes_cli.credential_security import ( | ||
| validate_base_url_safe as _validate_base_url, | ||
| validate_provider_name as _validate_provider_name, |
There was a problem hiding this comment.
The fallback validator raises HTTPException, but #66970's imported validate_provider_name raises ValueError. The new route callers do not catch that error, so their invalid-provider response changes from 400 to 500 when the companion validator is available. Normalize the exception contract at this boundary.
| else: | ||
| chosen = list(entries) | ||
|
|
||
| results = [_probe_one_entry(e) for e in chosen] |
There was a problem hiding this comment.
This invokes blocking urllib.request.urlopen() from an async endpoint, once per selected entry and with an eight-second timeout each. Move probe execution off the event loop and add coverage for multiple entries and timeout behavior.
a9743dc to
a092bfd
Compare
|
All four points addressed. The PR has been rewritten from scratch (commit
before: +528 lines (8 routes, OAuth/probe/summary)
after: +100 lines (3 routes: strategy, reset, health) |
SummaryOne open PR addresses the backend portion of Issue #47548. The complete #66972 diff adds three credential-pool operations—rotation strategy changes, cooldown resets, and health summaries—but does not add the requested Desktop UI. Related pull requests
Suggested consolidationkeep open with a salvage path: retain #66972's focused three-endpoint addition, add endpoint tests, and normalize imported provider-validation failures to HTTP 400 before further consideration. This follows the visible contributor Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 5 kB of PR diffs, 4 kB of issue/PR text, 5 kB of discussion (7 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Summary
Extends the existing
/api/credentials/pooldashboard API (per teknium1's feedback on #62467) with 6 new operations:/api/credentials/pool/{provider}/api/credentials/pool/{provider}/api/credentials/pool/{provider}/{id}/api/credentials/pool/{provider}/strategy/api/credentials/pool/{provider}/{id}/reset/api/credentials/pool/{provider}/healthSecurity
All endpoints use
credential_security.validate_provider_name+validate_base_url_safe:[a-z0-9_-]+only (no path traversal)api_keyWhy extend (not duplicate)
teknium1 pointed out on #62467 that current main already has authenticated pool routes under
/api/credentials/pool. This PR extends that contract rather than introducing a parallel/api/providers/{p}/poolfamily.Files
hermes_cli/web_server.py(+637 lines — 6 endpoints + validation)hermes_cli/credential_security.py(+111 lines — shared validators from security PR)Related
base_urlfeature requested in Credential pool: support per-credential base_url override for multi-account same-provider rotation #54011.