Skip to content

fix(webui): make long conversations usable — stop the full-transcript ratchet and the virtualized render loop - #2

Open
alanjds wants to merge 5 commits into
masterfrom
claude/hermes-webui-architecture-5mx701
Open

alanjds wants to merge 5 commits into
masterfrom
claude/hermes-webui-architecture-5mx701

Conversation

@alanjds

@alanjds alanjds commented Aug 21, 2026 •

Copy link
Copy Markdown
Owner

Four commits against two independent causes of "long conversations are unusable".

1–2 remove the cliff on the default path (virtualize_transcript off).
3–4 make the virtualize_transcript checkbox actually usable.

Problem A — the full-transcript ratchet (default path)

renderMessages() rebuilds the whole transcript, and runs on every send, every SSE batch and every refresh. Measured on a synthetic 2000-message session (desktop Chromium — a phone is several times slower):

loaded rows render DOM nodes
37 46 ms 960
537 546 ms 13,460
2000 2,883 ms 50,007

Cost tracks S.messages.length exactly. The server is not involved: Session.load() is 4.6 ms, GET /api/session ~330 ms flat at both 200 and 2000 messages.

1. The bare full-transcript refetch

_messageReloadLimitForSession() returned null — "send no msg_limit" — when the previous load wasn't truncated, or when the reload hint exceeded the server ceiling. The hint grows to the loaded row count, so paging back crossed the 500-row ceiling and from then on every same-session refresh (tab focus, SSE catch-up, visibility change) refetched and re-rendered all 2000 rows.

That null was deliberate — a clamped window would silently drop loaded older rows (nesquena#6152/nesquena#6154). The window is now always bounded, and the invariant is upheld differently: rows older than the returned window that we already hold are retained and re-prepended. The re-join is skipped, letting the server window win, when the prior rows can't be trusted to line up (session shrank server-side via fork/undo/truncate, or we don't hold enough rows to cover the gap) — losing older rows to a re-fetch is recoverable via "load earlier", splicing a stale prefix onto a rewritten transcript is not.

loadSession() clears S.messages and _oldestIdx before the reload fetch, so the pre-clear offset is stashed alongside the existing carry-forward snapshot and consumed with it.

2. The render-window ratchet

nesquena#6999 capped the auto-expanding render window at 4× the default, but the two stream-completion sites in messages.js still expanded it to every loaded row — one completed turn undid the cap. All three sites now share one bounded helper.

This restores the nesquena#6999 invariant; it does not by itself bound render cost (with virtualization off the DOM is built for every loaded row regardless). Fix 1 is what removes the cliff.

Problem B — the virtualized transcript never stopped re-rendering

With virtualize_transcript on, an idle 2000-message session ran 47–50 full renders during 5 seconds of no input, forever. Each re-ran the scroll-restore path, so scrolling felt like fighting the page.

4. The loop

renderMessages()
  -> _compensateScrollForMeasurementDelta() writes container.scrollTop
    -> scroll event
      -> scroll listener calls _scheduleMessageVirtualizedRender()
        -> renderMessages()   ... round again

_scheduleMessageVirtualizedRender() exists to break exactly this, deduping on _messageVirtualWindowKey. But renderMessages() only assigned that key inside its cached-HTML early return — gated on sid !== _sessionHtmlCacheSid, so it runs only when switching to a session. Every steady-state re-render left the key at '', the guard could never match, and the compensation's own scroll write scheduled another render. Nothing converged because nothing was recorded as painted.

The key is now assigned on the normal path too, after the DOM is built and before the measurement pass.

virtualization ON, 2000 msgs before after
renders during 5 s idle 47–50 0
renders to settle after load 11–13 2–3
a 600 px wheel-up actually moves 67–186 px 600 px
viewport movement after settle ~800 px 0 px

The last two rows are the user-visible symptom, and now match the non-virtualized path exactly.

3. Row-height calibration (independent, kept)

rowHeightFor() gives unmeasured rows a height for the scroll geometry, and always took the flat per-role constant — roleForIdx is always supplied, so the measurement-derived value _currentMessageVirtualWindow passed as defaultHeight was dead code. Only ~45 of 2000 rows are ever measured, and the constants are wrong in both directions (measured median 214 vs user:120/assistant:160/tool_call:400), leaving total scroll height 34% short.

Now a running mean is kept per role and used for unmeasured rows, at all three unmeasured-row sites. Roles fall back to their static seed until they have samples (first paint unchanged); a session switch drops the samples with the height cache.

This was not the oscillation fix — worth stating plainly, since it looks like one. It closes the geometry gap from 34% to 6.6% and changed the drift by zero pixels; commit 4 is what the drift actually was. It's kept because the geometry it feeds is what makes a scrolled-up position meaningful at all.

virtualize_transcript is left default-off in this PR — flipping it is a product call, and worth a round of real-device testing on top of these numbers first.

Deliberately out of scope

Per "fix the class, not the instance", naming the siblings left alone. The render window is also expanded to the full transcript by jump-to-session-start, jump-to-message when the target is outside the window, and the outline jump; and outline.js still issues a bare no-msg_limit fetch. Those are explicit user requests to reveal the whole transcript, addressed by absolute index; windowing them needs a jump-to-index-with-context fetch.

Verification

  • Reload requests always carry msg_limit (verified by intercepting the page's real /api/session traffic).
  • Render window stays at the 200 cap across repeated turn completions.
  • Idle render count, wheel travel and post-settle drift measured with real wheel events in headless Chromium; numbers above.
  • 2486 tests pass across the 155 files reading the changed JS plus the virtualization suite. The 13 test_update_banner_fixes.py failures are pre-existing test-ordering pollution — identical on a clean tree (13 failed / 2453 passed) and the file passes standalone. Ruff clean.

On test quality: several changed behaviours were pinned by exact source strings, and those assertions are updated rather than deleted. Two of the new tests were explicitly checked to fail when their fix is reverted (test_unmeasured_rows_use_measured_role_mean_once_samples_exist → 120 != 300; test_render_messages_records_window_key_on_the_normal_path), because the pre-existing virtualization tests all still pass against a reverted calibration and would have proven nothing.

claude added 3 commits August 21, 2026 23:28
…ders

A conversation of a few thousand messages becomes progressively unusable:
every renderMessages() rebuilds the whole transcript, and renderMessages()
runs on every send, every SSE batch and every refresh. Measured on a
synthetic 2000-message session (desktop Chromium; a phone is several times
slower), with the shipped default of virtualize_transcript=false:

    loaded rows   render   DOM nodes
             37     46 ms         960
            537    546 ms      13,460
           2000  2,883 ms      50,007

Render cost tracks S.messages.length exactly, so the bug is whatever loads
the whole transcript into the browser. Two things did, and together they
formed a one-way ratchet that no later action undid.

1. _messageReloadLimitForSession() returned null — "no msg_limit", i.e. the
   bare full-transcript request — in two cases: when the previous load was
   not truncated, and when the reload hint exceeded the server's msg_limit
   ceiling. The hint grows to the loaded row count, so paging back through a
   long conversation crossed the 500-row ceiling and every subsequent
   same-session refresh (tab focus, SSE catch-up, visibility change) refetched
   and re-rendered all 2000 rows. The null was deliberate: a clamped window
   would silently drop already-loaded older rows (nesquena#6152/nesquena#6154).

   The window is now always bounded, and that invariant is upheld a different
   way — rows older than the returned window that we already hold are retained
   and re-prepended, so a bounded refresh still cannot lose history. The
   re-join is skipped, letting the server window win, when the prior rows
   can't be trusted to line up (the session shrank server-side via fork/undo/
   truncate, or we don't hold enough rows to cover the gap): losing older rows
   to a re-fetch is recoverable via "load earlier", splicing a stale prefix
   onto a rewritten transcript is not. loadSession() clears S.messages and
   _oldestIdx before the reload fetch, so the pre-clear offset is stashed
   alongside the existing carry-forward snapshot and consumed with it.

2. nesquena#6999 capped the auto-expanding render window at 4x the default so it could
   not grow to the whole loaded transcript, but the two stream-completion sites
   in messages.js still expanded it to every loaded row. A single completed
   turn silently undid the cap. All three sites now share one bounded helper in
   ui.js so the bound cannot drift apart again. This restores the nesquena#6999
   invariant (it does not by itself bound render cost — with virtualization off
   the DOM is built for every loaded row regardless of the window).

Deliberately out of scope, per the "fix the class" rule: the render window is
also expanded to the full transcript by jump-to-session-start (ui.js),
jump-to-message when the target is outside the window (ui.js), and the outline
jump (outline.js), and outline.js still issues a bare no-msg_limit fetch.
Those are explicit user requests to reveal the whole transcript, addressed by
absolute index; windowing them needs a jump-to-index-with-context fetch, which
is a separate change.

Verified: reload requests always carry msg_limit; the render window stays at
the 200 cap across repeated turn completions instead of ratcheting to the
loaded row count; 2455 tests across the 155 files that read the changed JS
pass (the 13 failures in test_update_banner_fixes.py are pre-existing
test-ordering pollution — identical on a clean tree, and the file passes
standalone both before and after).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018y9pr7ppZgDuCZCT6x96kY
… constants

Groundwork for nesquena#4343, and a dead-code fix. NOT a fix for the scroll
oscillation itself — see the measurement below before assuming otherwise.

_messageVirtualWindow's rowHeightFor() gives every UNMEASURED row a height so
the virtual scroll geometry can be computed. It sourced that from the flat
per-role constants in MESSAGE_VIRTUAL_DEFAULT_ROW_HEIGHTS:

    return roleForIdx ? _messageVirtualDefaultHeightForRole(roleForIdx(idx))
                      : defaultHeight;

_updateMessageVirtualMeasurements already maintained a measurement-derived mean
in _messageVirtualEstimatedRowHeight, and _currentMessageVirtualWindow already
passed it down as `defaultHeight` — but roleForIdx is ALWAYS supplied, so that
branch never ran and the calibrated number was dead code. Every row that never
entered the render window kept the guess forever.

On a 2000-message session only ~45 rows are ever measured, so 98% of the
geometry came from the guess, and the guesses are wrong in both directions
(measured median 214 against user:120 / assistant:160 / tool_call:400). The
errors do not cancel: total scroll height came out 326,267px against a real
492,942px, 34% short.

Track a running mean PER ROLE instead and use it for unmeasured rows (a single
global mean would be worse than the constants, since a tool_call row and a user
row differ enormously). A role falls back to its static seed until it has
MESSAGE_VIRTUAL_ROLE_SAMPLE_MIN samples, so first paint is unchanged, and a
session switch drops the samples with the rest of the height cache. Applied at
all three unmeasured-row sites (rowHeightFor, _messageVirtualPrependedHeightDelta,
_messageVirtualScrollTopForVisibleIdx) rather than just the one, so they cannot
disagree about the same row's height.

Effect, measured with real wheel events on that session:

  - scroll geometry: 326,267px -> 460,631px against a real 492,942px
    (34% short -> 6.6% short)
  - drift of the row the reader is looking at, after the measurement pass
    settles: 800px median / 848px max BEFORE, and 800px median / 848px max
    AFTER. Unchanged. Zero.

So the geometry error is real and now largely fixed, but it is NOT what causes
the nesquena#4343 oscillation. Trapping the scrollTop setter names the actual writer:
_scrollAfterMessageRender -> _followMessagesAfterDomReplace -> scrollToBottom
-> _setMessageScrollToBottom, firing on the measurement-driven re-render even
though the caller passes preserveScroll:true and _messageUserUnpinned is
already true; the delayed _settleMessageScrollToBottom writes look like the
live culprit, re-scheduled by each re-render after _cancelBottomSettle clears
them. That is a separate change and virtualize_transcript stays default-off
until it lands.

The existing virtualization tests all still pass with the static fallback (no
samples recorded), so they would pass against a reverted fix and prove nothing
on their own; test_unmeasured_rows_use_measured_role_mean_once_samples_exist
pins the behaviour that actually changed and fails (120 != 300) when the
calibration branch is removed. Five tests that eval these helpers in an
isolated node scope needed the new state/companion declared.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018y9pr7ppZgDuCZCT6x96kY
…quena#4343)

With virtualize_transcript on, a long conversation re-rendered about ten times
a second, indefinitely, with no user input at all. Measured on an idle
2000-message session: 47-50 full renders during 5 seconds of doing nothing.
Every one of those re-ran the scroll-restore path, so scrolling felt like
fighting the page — the "unusable when enabled" that got the setting flipped
default-off for everyone.

The loop:

    renderMessages()
      -> _compensateScrollForMeasurementDelta() writes container.scrollTop
        -> scroll event
          -> the scroll listener calls _scheduleMessageVirtualizedRender()
            -> renderMessages()   ... and round again

_scheduleMessageVirtualizedRender() is supposed to break exactly this cycle. It
dedupes on _messageVirtualWindowKey — "the virtual window has not moved, there
is nothing to re-render":

    if(!force && nextKey===_messageVirtualWindowKey) return;

But renderMessages() only ever assigned that key inside its cached-HTML early
return, and that branch is gated on `sid !== _sessionHtmlCacheSid` — it runs
only when switching TO a session. Every steady-state re-render of the CURRENT
session left the key at '', so nextKey could never match it, and the
compensation's own scrollTop write scheduled another full re-render. Nothing
converged because nothing was ever recorded as painted.

Assign the key on the normal render path too, after the DOM is built (so an
early return cannot claim a window it never painted) and before the measurement
pass reads the same window. A scroll event that does not actually move the
virtual window is now a no-op.

Measured, same 2000-message session, virtualization on:

                                    before      after
  renders during 5s of idle          47-50          0
  renders to settle after a load     11-13        2-3
  wheel-up of 600px actually moves   67-186px    600px
  viewport movement after settle     ~800px         0px

The last two rows are the user-visible symptom: the wheel now moves the full
600px and the row under the reader does not move afterwards, matching the
non-virtualized path exactly. The earlier 800px figure was inflated by the
render loop starving the page — the wheel was being applied in fragments across
several hundred ms rather than yanked back — but it was real movement either
way, and it is now zero.

Note this is NOT the height-estimate problem fixed in the previous commit.
Calibrating the estimates closed the scroll-geometry gap from 34% to 6.6% and
changed the drift by zero pixels; this is what the drift actually was. The two
are independent, and the calibration is still worth having (the geometry it
feeds is what makes a scrolled-up position meaningful at all).

Existing scroll/virtualization coverage passes unchanged (135 tests across
test_issue500, older-history viewport preservation, jump-to-answer settle,
programmatic-scroll user intent, question jump, session jump buttons).
test_render_messages_records_window_key_on_the_normal_path pins the assignment
and its placement, and was confirmed to fail when it is removed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018y9pr7ppZgDuCZCT6x96kY
@alanjds alanjds changed the title fix(webui): stop long sessions ratcheting into full-transcript re-renders fix(webui): make long conversations usable — stop the full-transcript ratchet and the virtualized render loop Aug 22, 2026
With virtualize_transcript on, a reader who has scrolled up into history slides
backward while the agent streams. Real report: 437px of unrequested movement
over 31 seconds. Reproduced on a synthetic 2000-message session, reader
unpinned by a real wheel event, 12 streamed appends: +646px cumulative,
53.8px per render, every render, never healing.

It is NOT the stale-anchor realign the nesquena#5637 comments warn about. Trapping the
scrollTop setter and logging the geometry on both sides of
_restoreMessageViewportAnchor shows that realign doing exactly its job: the
virtual top spacer grew 214px, the recycled rows it replaced were 278px, so the
anchor row sat 64px too high; it wrote -64px and landed the row on its captured
offset to within 0.4px. Sampling the anchor row every frame instead names the
real writer — nothing writes at all:

    afterRestore   off=-109.3  scrollTop=459491
    raf0           off=-109.3  scrollTop=459491
    beforePP       off=-109.3  scrollTop=459491
    afterPP        off=-45.0   scrollTop=459491   <- 64.3px, no scroll write
    settled        off=-45.0   scrollTop=459491

All of it happens inside postProcessRenderedMessages(). That pass is scheduled
one frame AFTER the render and after the JS anchor restore, and it GROWS rows
above the viewport: Prism highlighting, and above all the .code-copy-btn it
injects into every .pre-header (measured .pre-header 35.5px -> 38.3px, 12 such
rows above the viewport). On touch the browser's native overflow-anchor engine
absorbs that growth — which is exactly why _postProcessWithAnchorSuppression
has to suppress the engine around the pass, or it stacks with the JS write and
yanks the reader (nesquena#5637/nesquena#5338). Desktop .messages rests at overflow-anchor:none,
so on desktop nothing absorbs it at all. The reader slides back by the full
growth, and the next render's _captureMessageViewportAnchor records the
already-drifted offset as its target, so the error ratchets rather than heals
(captured offsets climbing -109, -45, +17).

Virtualization is what makes it constant rather than a one-off: each append
shifts the virtual window, so the rows above the viewport are rebuilt as FRESH
elements and post-processed — and re-grown — on every single render. With the
checkbox off the pass finds nothing new to grow and the drift is zero, matching
the report exactly.

The fix extends the realign across the post-process instead of refusing it:
_beginPostProcessAnchorHold() snapshots the viewport anchor before the pass and
_restoreMessageViewportAnchor()s to it after, giving desktop in JS the hold the
touch path gets from the engine. It returns null — no behavior change whatsoever
— when _isTouchLikeMessageViewport says the native engine is holding, when the
reader is following the tail (bottom<=250, the readerAwayFromBottom idiom: a
follower is bound to the bottom, not to a row), or when there is no anchor; and
the applied hold abandons if _messageScrollInputGeneration moved during the
pass, so reader input always wins. Verified inert on an emulated touch viewport
(pointer:coarse -> _isTouchLikeMessageViewport true -> hold null); and even if
that gate were ever wrong, _restoreMessageViewportAnchor's own nesquena#5637 refusal
would decline the write there anyway.

Two directions were tried and rejected. Extending the _touchHold refusal to
desktop is what the nesquena#5637 gate comment already forbids, and it is still right:
with overflow-anchor:none nothing would hold the reader. Correcting the realign
target by the captured topPadBefore delta is wrong for this bug and would have
made it worse — the pad grew +214px in the same render where the row moved -64px,
so a pad-corrected target would have written roughly +278px in the wrong
direction. The captured topOffset was never stale; the DOM simply was not
finished changing.

Measured, same session, desktop Chromium (hover:hover and pointer:fine, computed
overflow-anchor: none, so the same media path as the reported Firefox desktop):

                                       before      after
  cumulative drift over 12 appends     +646px       -1px
  per render                           53.8px     -0.1px
  anchor row offset, first -> last   -109/+537  -109/-110

Not regressed: settled-state drift stays at median 0px / max 0px over 8 wheel
steps, and idle stays at 0 renders during 5s with calibration both on and off
(the nesquena#4343 loop check), including with the new scrollTop write in the loop.

NOT fixed, deliberately. The hold covers the SYNCHRONOUS post-process only. Any
growth that lands later — image decode, mermaid/katex resolving async — is still
uncompensated on desktop; it measured zero in this reproduction (the anchor row
does not move again in the 14 frames after the pass) but that is this session's
content, not a guarantee. The pre-existing "one more frame" of overflow-anchor
suppression still covers that window on touch. Nothing here touches the mobile
paths, the pinned/tail-follower paths, or _restoreMessageViewportAnchor itself.

Verification: 2490 tests pass across the 155 files that read the changed JS
plus test_issue500 (the 13 failures in test_update_banner_fixes.py are the known
pre-existing test-ordering pollution, identical on a clean tree). Four new tests
in test_issue500_message_list_virtualization.py drive the real
_captureMessageViewportAnchor / _restoreMessageViewportAnchor /
_beginPostProcessAnchorHold / _postProcessWithAnchorSuppression against a fake
DOM whose post-process grows above-viewport content by 64px:
test_post_process_holds_desktop_anchor_against_above_viewport_growth was
confirmed to fail (scroll write [] instead of [1164], anchor left 64px low) when
just the holdAnchor() call is removed, and that same removal was confirmed to
restore the full +646px browser drift. The other three pin the gates (touch
inert, tail follower skipped, reader input wins) and would pass against a
reverted fix on their own.

The wrapper's added comments are kept short on purpose: four unrelated suites
(csv, excalidraw, inline diff, json tree) assert that
postProcessRenderedMessages(container) appears within 500 characters of
_postProcessWithAnchorSuppression's opening, and the rationale lives on the
helper above instead. It currently sits at 435.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018y9pr7ppZgDuCZCT6x96kY
alanjds pushed a commit that referenced this pull request Aug 24, 2026
…squena#7107)

* fix: route overlapping custom_providers models by active endpoint

When two custom_providers[] entries list the same bare model id (e.g.
dogapi and packyapi both advertising 'claude-sonnet-5'),
resolve_model_provider scanned them in config write order and returned
the first match, silently ignoring the active provider's base_url. A
user whose model.base_url pointed at packyapi got routed to dogapi
because dogapi appeared earlier in config.

Now, when the active provider resolves to a named custom provider
(custom:<slug>, including via model.base_url -> named-slug matching),
that provider claims models it owns before the ordered scan runs. Falls
through to the legacy first-match scan when the active provider is bare
'custom' with no base_url to disambiguate, so existing single-endpoint
setups are unaffected.

Adds 3 regression tests covering: shared-model active-endpoint wins,
provider-unique model still routes by ownership, and bare-custom
no-base_url preserves legacy order.

* fix: extend active-endpoint routing to aux, handoff, and slug-collision paths

Review follow-up: the overlapping-id fix must reach three sibling
resolution paths that still routed on the wrong endpoint.

1. Auxiliary slot base_url (config.py set_auxiliary_model): a bare
   resolve_model_provider(model) ignored the SELECTED aux provider, so a
   custom:A slot was persisted with the active custom:B endpoint when both
   listed the model. Look up the selected provider's own custom entry via
   resolve_custom_provider_connection. (NOT model_with_provider_context
   here: the @Custom:<slug>:model form resolves base_url to None, which
   would drop the base_url entirely.)

2. Handoff summary (routes.py): read s_obj.model_provider and pass
   model_with_provider_context(model, model_provider) so a session pinned
   to custom:A is not rerouted to the active custom:B. base_url is
   backfilled from the provider's own custom entry downstream.

3. Normalized-name slug collision (config.py resolve_model_provider): two
   distinct provider names can normalize to the same slug (e.g. "Foo Bar"
   and "foo-bar"). The active-slug guard now matches the EXACT entry by
   base_url instead of re-deriving it from the non-unique slug, and fails
   closed (falls through to the ordered scan) on genuine ambiguity.

Adds 4 regression tests: normalized-slug base_url disambiguation,
slug-collision fail-closed, aux-slot selected-provider base_url, and
session-provider-context handoff routing.

* fix: fail closed on custom-provider slug collisions and break aux deadlock

resolve_model_provider() returns a provider slug (custom:<slug>) and the
credential lookup (resolve_custom_provider_connection) later resolves the
API key from the FIRST same-slug entry regardless of base_url. So when two
distinct provider names normalize to the same slug (e.g. "Foo Bar" and
"foo-bar") and both list the requested model, returning the slug silently
pairs one entry's endpoint with another entry's credential. Now raise a
clear AmbiguousCustomProviderError instead of guessing, on both the
active-slug guard and the ordered-scan fall-through paths.

Also: an explicit, unambiguous named custom provider now stays authoritative
even when model.base_url is stale/absent and points at neither entry — a
stale URL is no longer evidence to demote it to config order.

And fix a self-deadlock in set_auxiliary_model(): it held the non-reentrant
_cfg_lock while calling resolve_custom_provider_connection() -> get_config()
-> reload_config_if_stale(), which re-acquires the same lock. The selected
custom provider's base_url is now resolved inline from the config_data
already loaded under the lock.

Tests: collision cases now assert the ambiguity error; added a stale-URL
explicit-provider regression and a watchdog-guarded aux no-deadlock
regression (both fail before the fix).

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix: fail closed on custom-provider slug collisions across all four paths

The previous cut only guarded the symmetric bare-model case, and it built
collision membership from entries that OWN the requested model. But credential
resolution scans ALL same-slug entries and first-matches regardless of
ownership, so several slug-only boundaries could still pair one entry's
endpoint with another entry's credential — including the asymmetric case where
only one colliding entry lists the model.

Introduce one shared, pure/lock-safe helper (_custom_provider_slug_key +
_unique_custom_provider_entry) that groups ALL named custom_providers entries by
a single canonical slug normalization and raises AmbiguousCustomProviderError
whenever a consumed slug maps to >=2 entries. Apply it at every slug-only
boundary so endpoint and credential can never come from different entries:

- bare resolution (resolve_model_provider): all-entry membership for both the
  active-slug guard and the ordered ownership scan (was owner-only).
- provider-qualified @Custom:<slug>:model return (the session/send/handoff
  model_with_provider_context shape), which previously bypassed the check.
- credential lookup (resolve_custom_provider_connection), replacing its private
  first-match-by-slug scan and its duplicate _slugify.
- auxiliary persistence (set_auxiliary_model), replacing its own inline
  first-match-by-slug; the ambiguity now propagates (fails the save) instead of
  being swallowed, while other best-effort errors stay non-fatal. Still resolves
  from the in-scope config_data so it remains lock-safe (no deadlock regression).

Tests: add fail-closed regressions for the asymmetric A-first-non-owner/B-owner
bare case (active-endpoint and bare-custom paths), the model_with_provider_context
qualified handoff shape, the credential boundary called directly, and the
auxiliary-save ambiguity (asserts nothing is persisted). All five fail before
this change and pass after.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix: one canonical custom-provider slug identity, point-of-return ambiguity, profile-scoped credentials

Deep re-gate follow-up (5 findings):

1. Collision detection now derives from the single slug PRODUCER
   (_custom_provider_slug_from_name) instead of a looser private key, so
   producer-level collisions on punctuated/parenthesized names (e.g.
   'Foo (Bar)' vs 'foo-bar', both -> custom:foo-bar) are detected. Previously
   the looser key normalized 'Foo (Bar)' to 'foo-(bar)' and missed the
   collision, reintroducing the endpoint-A/credential-B bug for those names.

2. The all-entry ambiguity check now runs ONLY at the point a custom:<slug> is
   about to be returned (via a _finalize() guard covering every custom-return in
   resolve_model_provider, including the final active-provider fallback), not up
   front. An unrelated collision on the active slug no longer blocks lanes that
   resolve to a different provider (@openrouter, a non-colliding @Custom:other).

3. streaming.py now resolves endpoint AND credential inside ONE profile scope:
   the main path extends the scope over runtime-key + custom-provider resolution,
   and both self-heal retry paths bind the session profile. This stops a detached
   worker from pairing a named profile's endpoint with the default profile's key.

4. Handoff summary: AmbiguousCustomProviderError now returns HTTP 400 with the
   actionable rename message instead of degrading to a 200 local fallback the UI
   treats as success.

5. UI: aux + default model saves surface the server's message and abort (retain
   dirty state) instead of swallowing it into a generic failure / "settings
   saved"; the handoff catch shows the 400 rename message verbatim.

AmbiguousCustomProviderError gains a .message attribute for verbatim forwarding.

Tests: parenthesized-name collision fails closed on all direct paths; unrelated
lanes are not blocked by an active collision; custom overrides resolve inside the
passed profile scope; handoff returns 400 (not a persisted fallback); JS
error-surfacing source guards. Verified the finding #1/#2 cases fail before and
pass after (old key misses 'Foo (Bar)'/'foo-bar'; old up-front check over-blocks).

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: nesquena-hermes <nesquena+hermes@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants