Convert in-place pkg.__path__=[] mutations on the REAL agent/hermes_cli
packages to monkeypatch.setattr (auto-restored), and make
test_issue1574's _activate_spawn_fake_agent snapshot+restore the
HERMES_WEBUI_AGENT_DIR/PYTHONPATH/sys.path it mutates.
Fixes:
- test_v050259_sessiondb_fd_leak::test_session_db_close_is_idempotent
- test_tls_support::test_tls_startup_failure_fallback_to_http
Greptile rerun and the maintainer review both raised the same concern:
state.db and the sidecar self.messages can diverge by hundreds of
messages during recovery / mid-flight writes / pending_user_message.
On the test corpus:
0db167553ac7 db=11 sidecar=12
295592fd560e db=813 sidecar=889
7478cae31f01 db=2730 sidecar=2405
The previous patch's SQL path was reading state.db. The pre-patch
inline walk was reading self.messages (the sidecar). Sidebar stale-row
detection (_looks_like_stale_zero_message_row,
_row_may_need_sidecar_metadata_refresh) consumes this field as if the
sidecar were the source of truth, so swapping the source silently flips
the field's semantics.
Drop the SQL path entirely. The helper now does the same in-memory
walk the pre-patch code did, extracted into a named method for
visibility. The inline role check (dict.get('role') with default
empty string) is 1.7x faster than calling _message_role() per
iteration on the test corpus (2.57ms -> 1.49ms across 8 sidecars,
2405 msgs total).
Bench (2405-msg sidecar): 0.76ms per call, vs ~1.5s for the pre-patch
inline walk on the same sidecar (the original perf hotspot). The
1.5s figure included JSON parse, dict construction, and the SQLite
overhead of the now-removed SQL path; the bare walk is the
0.76ms shown here.
Correctness: 27/27 webui sidecars produce identical counts vs the
pre-patch inline walk. No behavior change for any caller.
Addresses Greptile findings (P1 silent-zero, P2 docstring, P2 dead
allowlist) by removing the SQL path entirely rather than patching
around its divergent-source issue.
Greptile flagged that _compute_user_message_count_lazy silently returns 0
when the SQLite query fails (DB locked, missing file, schema drift).
The pre-patch compact() walked self.messages in Python and returned the
correct count without ever touching the DB, so the silent zero
fallback is a regression that breaks _looks_like_stale_zero_message_row
under contention.
Fix: helper now takes an optional messages argument. On SQLite failure
it falls back to the same in-memory walk the original code used, so
callers see the same number they'd have seen before for any session
where Session.load() populated self.messages.
Also drop the dead ('GET', '/api/session') allowlist entry flagged by
Greptile — the /api/session handler already does its own per-stage
logging via the _t0.._t6 block (with auto-log on slow requests), and
adding RequestDiagnostics on top would just duplicate the slow-request
journal entries without adding information.
Session.compact() called _last_message_timestamp(self.messages) which
reversed-walked all 2,730 messages on every /api/session response.
The messages array is chronologically ordered, so the last non-tool
message sits at the very end. Scan only the last 8 messages; fall
back to a full scan only if no timestamp is found in the window
(unusual sessions with >8 trailing tool rows).
Reverts to identical behavior when the window hits, since the most
recent user/assistant message's timestamp is what compact() emits.
Bench (single-shot, live Chromebook eMMC):
before: get_session=1107ms compact=1725ms total=2880ms
after: get_session= 648ms compact= 288ms total=1313ms
(-80% on compact, -54% on total)
Revert: git checkout master && systemctl --user restart hermes-webui
Adds auto-logging when total /api/session latency exceeds 2s, so we don't
need HERMES_DEBUG_SLOW env var to diagnose regressions in production.
Env var still forces logging on every request for development.
Baseline data on the Chromebook (Celeron N3350, eMMC storage):
- long_session_idle (2,445 msgs) p50=5.8s, p95=9.1s, max=9.7s, 2/10 timeouts
- stage breakdown for a 2.8s request:
get_session=1041ms, compact=1717ms, redact=31ms, json_write=8ms
- compact stage iterates all messages for user_message_count and runs
recursive redact_session_data + json.dumps -- biggest single cost
Ref: perf/session-load-latency
The slow-request journal showed /api/profiles, /api/models, and /api/session
fire on every session click but had no per-stage timing. Add:
- RequestDiagnostics coverage for GET /api/profiles, /api/models, /api/session
- Stage markers inside /api/profiles (list / active lookup / isolated check)
- Stage markers inside /api/models (freshness routing, serialize)
- Per-stage stage-log inside get_available_models_for_session_visit that
emits a [SLOW] line when total wall time crosses HERMES_DEBUG_SLOW ms
(default 500ms) so we can pinpoint which sub-step is blocking
No behavior change. Existing _t0.._t6 timing on /api/session and the
2.5s/10s TTL on /api/sessions are unchanged. Reversible: revert commit.
The renderMessages() re-anchor call `queueMicrotask(()=>_reanchorPinnedTailAfterRender(...))`
threw `ReferenceError: _reanchorPinnedTailAfterRender is not defined` in the standalone
renderMessages() node harness (test_anchor_fallback_ownership), which evals the extracted
renderMessages body with only a minimal set of stubbed helpers.
Guard the call with `typeof` checks on both `queueMicrotask` and
`_reanchorPinnedTailAfterRender`, mirroring the existing
`if(typeof _deferClearProgrammaticScroll==='function')` guard a couple of lines below, so
a harness that evals renderMessages without defining these helpers does not throw. In the
browser both are always defined, so the fix path is unchanged.
Add a source-lock asserting the guard so a future refactor can't drop it and re-break the
harness.
A pinned/tail-following reader gets a fast ~1-row back-and-forth bounce on every
mid-stream re-render (tool completion, activity-scene refresh, clarify echo).
renderMessages() rebuilds the transcript (inner.innerHTML='' then re-append). For a
pinned reader the post-render path writes scrollTop=scrollHeight while still INSIDE
the render sync stack, where the browser reports a transient scrollHeight a few px
short of the settled value (layout is batched). scrollToBottom lands scrollTop a
little high (short of the true tail); that intermediate is painted this frame and the
settle rAF corrects it the next frame -> a ~1-row bounce (measured ~82px, confirmed
with a per-frame scrollTop flight recorder and an isolated CSSOM A/B).
Fix: capture the pre-wipe near-tail state (geometry, since the wipe's clamp scroll
event can perturb the closure pin flags) and, at the end of renderMessages, schedule
_reanchorPinnedTailAfterRender in a microtask. The microtask runs after the sync stack
flushes layout to the settled height but before paint, so writing the settled max
lands the tail exactly and the short intermediate never reaches the screen. A
synchronous call reads the same transient short height and is a no-op (verified: sync
leaves the 82px bounce; microtask drives it to 0). Only re-anchors a pre-wipe
tail-follower left short of the settled max, so an unpinned reader parked in history
is never moved (verified 0px displacement); arms the existing _programmaticScroll
latch; idempotent.
Tests: test_pinned_tail_midstream_jitter.py runs the real extracted helper in a Node
harness (re-anchor short tail-follower / never move a history reader / idempotent
no-op / mutation check / source locks on the queueMicrotask call site and settled-max
write). Neighboring scroll suites remain green.
Address greptile review: the new S.activeStreamId restore branch omitted the
_maybeShowNewMessageScrollCue call that the preserveScroll and idle-unpin restore
branches both make. An unpinned reader whose position is restored mid-stream should
still see the new-message cue, so add it for consistency. Update the behavioral test
to assert the cue fires and the test_issue1690 source-lock for the new branch body.
renderMessages() rebuilds the transcript via inner.innerHTML='' then re-appends
rows. That wipe collapses #msgInner scrollHeight toward the empty-table height, so
the browser is forced to clamp #messages.scrollTop down to the new near-zero max --
a browser primitive (no JS writes scrollTop; overflow-anchor does not govern it).
When this rebuild happens mid-stream while a reader has scrolled up into history
(_messageUserUnpinned), _scrollAfterMessageRender took the S.activeStreamId branch
and called scrollIfPinned(), which is a no-op for an unpinned reader -- so the
clamped-to-0 scrollTop was never restored and the reader was stranded at the top.
Fix: in that branch, when the reader is unpinned and a pre-wipe scroll snapshot
exists (renderMessages already captures one whenever _messageUserUnpinned), restore
the snapshot instead of the no-op. Pinned / tail-following readers keep
scrollIfPinned() so live-follow is unchanged. Reuses the existing
_restoreMessageScrollSnapshot helper (same one the preserveScroll and idle-unpin
branches use), so no new core-timing risk.
Tests: test_wipe_rows0_streaming_unpinned_restore.py executes the real extracted
_scrollAfterMessageRender against a fake #messages + instrumented restore stubs and
asserts the unpinned+activeStreamId+snapshot case restores (scrollTop returns to the
pre-clamp value) while pinned / no-snapshot cases fall back to scrollIfPinned();
mutation-checked (reverting to the bare scrollIfPinned() fails the behavioral asserts).
Updates the test_issue1690 source-lock for the new branch structure.
The module-level _userRowIntrinsicHeightBySessionIdx cache is keyed by
session-relative index (_messageSessionIndexForRawIdx = _messageSessionIndexBase()
+ rawIdx). The base is 0 for the common non-offset session, so keys collide across
sessions: after switching from a session with tall user rows to one with short
rows, the new session's off-screen rows at colliding keys inherited the previous
session's remembered heights and inflated scrollHeight until re-measured.
Add _clearUserRowIntrinsicHeightCache() and call it from
_clearMessageVirtualHeightCache — the existing per-session height-cache clear that
_resetMessageRenderWindow invokes on session switch — so the new cache shares the
exact same lifetime as _messageVirtualHeightCache. Keys are deleted in place to
keep the const binding stable. Guard the call with typeof so the node harnesses
that extract _clearMessageVirtualHeightCache without this helper keep running.
Adds a mutation-checked regression (test_cache_cleared_on_session_switch_prevents_
stale_height_bleed): a rebuilt row at a colliding key after the clear must fall
back to the content estimate, not the stale remembered height; making the clear a
no-op fails the test.
Round-2 gate cert (iOS CORE): the touch predicate _isTouchLikeMessageViewport
used matchMedia('(pointer:coarse)'), which is true on BOTH Android and iOS. But
overflow-anchor is inert on iOS WebKit (the repo's own static/style.css mobile
content-visibility block documents this — it deliberately does not set
overflow-anchor:none because it is a no-op on iOS and re-opens the #4856/#5338
jump on Android). So on iOS the stale-anchor refusal fired but its premise (let
the native overflow-anchor layer hold the viewport) is false → a scrolled-up iOS
reader was left unheld after above-viewport growth, the same class as the round-1
desktop regression, one platform over.
Split the platform check: add _isIOSWebKit() (classic iPhone/iPod/iPad UA, plus
iPadOS 13+ which masquerades as MacIntel but has maxTouchPoints>1 unlike a real
Mac) and exclude it from _isTouchLikeMessageViewport. The refusal now fires ONLY
on Android (pointer:coarse AND overflow-anchor actually works); desktop and iOS
both keep the explicit semantic realign / absolute snapshot.top restore.
Tests: 3 new predicate cases (iPhone, iPadOS-as-Mac, Android control), all
mutation-checked — removing the _isIOSWebKit exclusion fails exactly the iOS
cases while Android/desktop stay green; broadening _isIOSWebKit to any touch
device fails the Android control. The Node harness forces the navigator mock via
Object.defineProperty because Node 18+ ships a built-in read-only navigator that
a plain assignment silently ignores.
Note: iOS Safari cannot be exercised by the Node harness (the 'native anchor
holds' postcondition is mocked, never observed), so this needs a real iOS-Safari
recording before merge per the gate cert.
The node test harnesses (e.g. test_anchor_fallback_ownership) extract
renderMessages and _measureMessageVirtualRow by source and run them with every
collaborator stubbed by name. The new _applyUserRowIntrinsicHeight /
_rememberUserRowIntrinsicHeight helpers were not among those stubs, so the
extracted functions threw ReferenceError ("node behavior check failed") on the
CI shard that runs those files. Gate both cross-function call sites on
`typeof helper === 'function'` (same pattern already used across ui.js for
harness-extracted helpers). Production always has the helpers defined, so the
guard is a no-op there; a harness without them cleanly skips the reservation
and keeps prior behavior. No behavior change on the real page.
A virtualization wipe-and-rebuild recreates user rows as fresh DOM elements,
which discards content-visibility:auto's last-remembered size. An off-screen
tall user row (e.g. a long paste) then falls back to the flat
`contain-intrinsic-size: auto 96px` from the stylesheet the instant it is
rebuilt, collapsing scrollHeight by (realHeight - 96px). The browser then either
force-clamps scrollTop (the dTop≈dH jump) or re-anchors to a far row (the dTop≫dH
jump) — both mobile jump-back symptoms trace to this one collapse.
This is layer-scoped to the CSS/content-visibility path and is only active under
`@media (pointer: coarse)` where content-visibility:auto is applied; desktop
rests at content-visibility:visible so the intrinsic-size is ignored (verified:
writing an intrinsic-size on a visible row produces zero scrollHeight/layout
delta), making the change inert on desktop with no platform gate needed.
Fix: remember each user row's measured height keyed by its stable
data-session-msg-idx and re-apply it as an inline contain-intrinsic-size, so a
rebuilt fresh element reserves the real height instead of 96px:
- `_measureMessageVirtualRow` persists + writes the measured height (user rows
only; assistant rows are content-visibility:visible on mobile).
- the user-row build path in `renderMessages` applies the remembered height, or
a content-length estimate (floored at 96px so short rows never regress) before
the row has ever been measured, so even the first fresh-element frame does not
collapse.
Adds tests/test_issue5638_user_row_intrinsic_height_collapse.py (node harness),
mutation-checked: disabling either the measure writeback or the build-time apply
makes the corresponding assertions fail.
Address the two non-blocking observations from re-review of the touch-gate fix:
1. Add a direct unit test for _isTouchLikeMessageViewport itself — the guard-wiring
tests stub the predicate via a boolean, which validates that the _touchHold term
gates the refusal but not the predicate's own mid-realign stability (the claim that
motivated choosing matchMedia over the computed overflow-anchor probe). The new
_predicate_harness exercises the real predicate + _browserOverflowAnchorActive with
mocked matchMedia/getComputedStyle:
- test_predicate_stays_true_on_touch_when_inline_anchor_clobbered_to_none: on a
pointer:coarse device whose inline overflowAnchor was clobbered to 'none' by a
prior realign tick, the predicate must still report touch=true. Mutation-checked:
reverting the predicate to the bare computed probe makes exactly this test fail.
- test_predicate_false_on_desktop_fine_pointer / _falls_back_to_computed_probe_without_matchmedia
cover the desktop and no-matchMedia paths.
2. Note in the predicate comment that the no-matchMedia fallback to the computed probe
is best-effort (matchMedia('(pointer:coarse)') is universally supported in every
targeted browser, so the primary path is what runs).
nesquena-hermes