fix(desktop): delete legacy custom providers - #69258
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Desktop Custom Endpoints management so legacy custom_providers: entries (still consumed by the Desktop model picker via compatibility logic) are surfaced in the settings UI and can be deleted safely, including cleanup of active model routing credentials when the deleted endpoint is in use.
Changes:
- Extend the Desktop custom endpoints REST surface to list legacy
custom_providersentries, mark them read-only in the UI, and support source-qualified deletion. - Add path-safe, source-qualified identifiers for legacy list entries and deduplicate equivalent modern/legacy rows to avoid “invisible but selectable” endpoints.
- Add Python + Vitest coverage for legacy listing, deletion, credential scrubbing, and collision/ambiguity handling.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/hermes_cli/test_web_server.py | Adds coverage for listing/dedup/deleting legacy custom_providers endpoints and credential scrubbing behavior. |
| hermes_cli/web_server.py | Implements legacy row projection + identity/dedup logic and extends DELETE to support both schemas with source-qualified deletion. |
| apps/desktop/src/hermes.ts | Updates the Desktop REST helper to use a query-based DELETE route for legacy endpoints. |
| apps/desktop/src/hermes.test.ts | Adds coverage that the correct DELETE route is chosen based on endpoint source. |
| apps/desktop/src/app/settings/custom-endpoints-settings.tsx | Renders legacy endpoints as read-only “Legacy config” rows with a delete action and refreshes after deletion. |
| apps/desktop/src/app/settings/custom-endpoints-settings.test.tsx | Adds UI tests ensuring legacy rows are delete-only and modern/direct-config rows retain existing behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
55d7af3 to
d48e177
Compare
alt-glitch
added
type/bug
Related to #14360: both address deletion across the legacy and modern custom-provider schemas. This PR adds the Desktop/API visibility and source-qualified legacy-management path, while #14360 currently has a separate CLI reachability gap; please choose or consolidate the approach. |
|
Thanks for connecting these two PRs and for pointing out the overlap. I took a closer look at the current head of #14360 and the latest main branch. I think the clearest path for now is to keep the PRs separate, since they cover different user-facing entry points and currently do not overlap at the file level. #69258 is intentionally focused on Desktop/API visibility and deletion, while #14360 is focused on the CLI flow reported in #5525. This PR does not intend to close that issue. To make that boundary explicit, I have narrowed #69258 so the modern From what I can see, #14360 may still need a small follow-up for the current CLI path: the removal action is gated on the legacy My preference would therefore be to keep #69258 scoped to Desktop/API and let #14360 complete the CLI entry-point coverage independently. If maintainers would prefer one shared deletion primitive, I would be happy to coordinate a follow-up config-layer extraction with a common, source-safe identity contract. |
d48e177 to
40e3b17
Compare
Re: PR #68591 coordinationHi @SacrEllfarch, I noticed our PRs have some overlap in the Custom Endpoints area. I wanted to reach out to discuss how we can coordinate. PR #68591 (my PR)My PR takes a different approach:
PR #69258 (your PR)Your PR:
Conflict
Proposed coordinationWould you be open to merging the approaches? Option A: Use #68591's UI (compact card) + #69258's legacy support
Option B: Keep #69258's UI (full page) + #68591's route cleanup
Option C: Merge #69258 first, then I'll rebase #68591
What do you think? Happy to collaborate on a combined solution. |
|
感谢你主动联系并详细整理这些协调方案。 综合目前两个 PR 的范围和现有功能，我更倾向于 Option B：保留当前完整的 Custom Endpoints 页面，同时确保接口继续统一使用复数 我选择保留完整页面，主要基于以下几点：
关于路由，我重新检查了
这个额外的 legacy DELETE 形式是为了区分 modern provider key、重名 legacy 条目以及名称中包含 因此我建议按 Option B 协调： 如果后续继续推进紧凑卡片方案，我建议在 rebase 后明确保留或重新承载以下能力：多个端点、legacy 条目管理、端点测试与切换，以及 再次感谢协调，也欢迎维护者对最终的界面方向和合并顺序给出意见。 |
Adds a compact inline card under the LocalEndpointRow in the API Keys page, giving users a quick-edit affordance for their active custom endpoint's base_url and api_key without leaving the page. - Calls plural /api/providers/custom-endpoints (aligned with upstream main) - Falls back to the existing CustomEndpointsSettings full page for advanced flows (new/edit multiple, API compatibility mode, legacy entries) - Sits on top of NousResearch#69258 which adds legacy custom_providers management to the full page
40e3b17 to
686c5fe
Compare
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for covering the Desktop/API path separately from the CLI work; current main does leave legacy custom_providers runtime-reachable but absent from this management API (hermes_cli/web_server.py:7672-7712, hermes_cli/runtime_provider.py:748-780).
Problems
hermes_cli/web_server.py:7717returns the raw legacybase_url. Although_canonical_endpoint_url()excludes URL userinfo from the management-ID fingerprint (hermes_cli/web_server.py:7637-7662), it does not sanitize the returned row. Ahttps://user:secret@host/v1legacy URL is then rendered directly byapps/desktop/src/app/settings/custom-endpoints-settings.tsx:79.
Suggested changes
- Return a display-safe legacy URL with credentials and fragments removed, without changing the raw value used for matching/deletion, and cover that response/rendering invariant with a credential-bearing URL fixture.
This is an automated hermes-sweeper review.
| "id": _legacy_custom_endpoint_id(index, name, base_url, model), | ||
| "_config_index": index, | ||
| "name": name, | ||
| "base_url": base_url, |
There was a problem hiding this comment.
This returns the raw legacy URL. _canonical_endpoint_url() strips userinfo only for the management ID, so a configured https://user:secret@host/v1 is exposed by this API and rendered in Desktop. Please return a display-safe URL without userinfo/fragments and add a response-level regression test.
There was a problem hiding this comment.
Thanks for catching this. Fixed in 3ef0b4ff0590aa5f4d162d8413089de0d82d3990. Legacy rows now expose a display-safe base_url with URL userinfo and fragments removed, while the raw configured URL is retained internally for identity, matching, active-model cleanup, and deletion. Added a response-level regression test covering a credential-bearing URL and verified that the stored configuration remains unchanged. The focused web-server tests pass.
teknium1
added
sweeper:risk-security-boundary
alt-glitch
removed
the
sweeper:risk-security-boundary
3cf53fd to
34393fc
Compare
SummarySeven PRs address or reference this three-issue complex: #5549, #14360, and #63287 implement dual-schema CLI removal; #7658 adds provider editing; #57583 sorts model choices; #62160 hardens null legacy URLs; and #69258 adds a distinct Desktop/API legacy-management path. The CLI removal candidates address the schema mismatch behind #5525, while #69258 makes legacy entries visible and individually deletable in Desktop but does not repair the reported CLI entry point. Related pull requests
Duplicates#5549, #14360, and #63287 substantially overlap on dual-schema CLI provider removal; #63287 was explicitly closed in favor of #14360, while #5549 remains a closed reference implementation. #69258 is related but not a duplicate because it owns the Desktop/API path. Suggested consolidationKeep #69258 open with a salvage path: retain its source-qualified Desktop/API legacy listing and deletion design, but satisfy the contributor keep_open review by returning a display-safe URL without userinfo or fragments while preserving the raw value for matching/deletion, and add response-level regression coverage. Keep #14360, #7658, #57583, and #62160 as separate issue-specific work; leave #5549 closed as a reference and #63287 closed as superseded by #14360. Cross-PR triage: Reviewed 7 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 90 kB of PR diffs, 17 kB of issue/PR text, 10 kB of discussion (15 comments), 12 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
34393fc to
3ef0b4f
Compare
3ef0b4f to
c3e45b2
Compare
|
Rebased this branch onto current |
Summary
custom_providers:entries in Desktop Custom Endpoints settingsLegacy configrows with a Delete actionProblem
The Desktop model picker reads the compatibility
custom_providers:schema, but the Custom Endpoints management API only listed and deleted the newerproviders:schema. A provider could therefore remain selectable while being invisible and undeletable in Settings.Scope boundary
This PR owns the Desktop/API path only. The source-qualified DELETE contract handles legacy
custom_providersrows, while the existing modern DELETE path remains scoped toprovidersentries. Listing may deduplicate equivalent rows for the Desktop view, but deleting a modern row does not delete a legacy row. CLI removal remains separate in #14360 and this PR does not close #5525.Baseline on
9ecacd6bfwith a syntheticLegacy Proxyconfig:After this change:
Legacy management IDs include the config-list position and a non-secret identity fingerprint. The DELETE contract includes the source schema, avoiding collisions with modern provider keys, colliding legacy runtime slugs, and names containing
/. URL matching normalizes only scheme, hostname, default ports, and trailing slashes; case-sensitive paths, queries, and model IDs remain distinct. URL credentials and fragments are excluded from management identity.Compatibility with #71141
This branch is rebased on the merged #71141 credential/model-catalogue work. It preserves
.env-backedkey_envstorage and model catalogue persistence, reports bothkey_envand legacyapi_key_envreferences without resolving their values, and removes the derived modern endpoint env entry when a modern endpoint is deleted. Deleting a legacycustom_providersrow deliberately does not remove an arbitrary user-owned environment variable.When an active endpoint is removed, the model mirror cleanup covers inline and environment-backed credential fields plus protocol fields (
api_modeand canonicaltransport) so stale routing state is not left behind.Testing
3cf53fd46is rebased onupstream/main98105f31f7 passed22 passed25 passed, Desktop settings/helper28 passed, fulltests/hermes_cli/test_web_server.py125 passed432 passed, 1 skippedtest files;4038 passed, 2 skippedtestsgit diff --check: passed0 errors(74existing upstream warnings)tools/file_operations.pyand its tests; the PR-relevant source/test tree is byte-identicalThis complements the CLI/config work in #14360 by covering the Desktop REST and GUI path only. Related settings design discussion: #68591.