diff --git a/CHANGELOG.md b/CHANGELOG.md index 5212775b..53d0a1cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ ### Fixed +- **Clicking a session link that belongs to another profile now switches to that profile instead of failing.** A valid `session://` deep link pointing at a session owned by a different Hermes profile used to look identical to a deleted session — the UI showed "Session not available in web UI" and self-healed away. Now the WebUI recognizes a valid-but-wrong-profile session, offers to switch to the owning profile, and retries the load once; truly missing/deleted sessions still self-heal as before. No foreign-profile transcript is ever returned — the mismatch response carries only the owning profile name. Thanks @harcek. (#5419) + +- **Windows: git operations no longer leave hidden console windows piling up.** On Windows, every workspace git command (`_run_git`) and git-config probe spawned a short-lived `git.exe` whose console window could flash and accumulate as orphaned `conhost.exe` processes over a long session. Those subprocess calls now pass `CREATE_NO_WINDOW` so no window is created. Safe no-op on macOS/Linux, and it uses a small in-module helper (mirroring the update path) rather than a hard dependency on the optional agent package, so a standalone WebUI keeps full git functionality. Thanks @jin89ho. (#5692) + +- **The agent's closing summary now shows when it hits the iteration limit.** When a turn exhausted its tool/iteration budget, the agent produces a graceful closing message (a summary of what it got done, or a fallback like "I reached the iteration limit…"), but the WebUI showed a bare `tool_limit_reached` error card instead — most visible with reasoning-only models (extended-thinking Claude, DeepSeek, Codex Responses) whose summary arrives empty. The closing text is now surfaced as a proper assistant message. Purely additive and guarded — it never fires on a turn that already produced a final answer, never double-injects, and degrades to the old behavior when no fallback text is available. Thanks @claw-io (co-authored by @b3nw). (#5717, #5494) + +- **The workspace file tree no longer jumps to the top when you expand a folder.** Every folder expand/collapse, breadcrumb navigation, refresh, and hidden-files toggle re-rendered the tree by wiping its container, which collapsed its height and made the browser reset the scroll position to the top — teleporting you away from where you were in a long tree. The scroll position is now captured before the re-render and restored after. Reported by @claw-io. (#5664, #5657) + - **Self-update recovers when a `.git/index.lock` is blocking it.** If "Update Now" failed because a stale `.git/index.lock` was present (a previous git process left it behind), the WebUI gave no in-UI way out — you were stuck on an un-updatable install. The update banner now detects that specific failure, shows the exact `rm -f …/.git/index.lock` command to run on the host, and offers a "Retry update" button that re-runs the normal update once the lock is gone. The server never deletes anything under `.git` itself (fail-closed — git's lock can't be safely auto-cleared without racing), so this is purely a detect-and-guide recovery. Thanks @b3nw. (#5688, #5687) ### Internal diff --git a/api/routes.py b/api/routes.py index a22ad08e..3e5ae1bc 100644 --- a/api/routes.py +++ b/api/routes.py @@ -528,6 +528,11 @@ def _session_visible_to_active_profile(session_profile, handler=None) -> bool: def _request_session_visibility_exempt(method: str, path: str | None) -> bool: if not path: return False + if method == "GET" and path == "/api/session": + # Detail-load owns profile mismatch handling so the frontend can switch + # to the session's profile instead of treating a valid cross-profile + # deep link as a deleted/stale session. + return True if method != "POST": return False # Import routes create/claim sessions before normal ownership exists, and @@ -11730,6 +11735,20 @@ def handle_get(handler, parsed) -> bool: s = get_session(sid, metadata_only=(not load_messages)) _session_profile = getattr(s, 'profile', None) or None if not _session_visible_to_active_profile(_session_profile, handler): + if _session_profile: + # Valid session owned by a KNOWN other profile: 409 so the + # client can offer to switch to it (#5419). + return j(handler, { + "error": "Session belongs to a different profile", + "code": "session_profile_mismatch", + "session_id": sid, + "profile": _session_profile, + }, status=409) + # Unknown/legacy None-profile sidecar: keep the original 404 so + # the frontend's self-heal (clear stale URL + localStorage) still + # fires. _profiles_match coerces None->'default', so a truly + # missing/legacy session under a non-default active profile would + # otherwise emit a useless 409 with profile=null. return bad(handler, "Session not found", 404) original_stream_id = getattr(s, "active_stream_id", None) _clear_stale_stream_state(s) @@ -12061,6 +12080,20 @@ def handle_get(handler, parsed) -> bool: cli_meta = _lookup_cli_session_metadata(sid) _session_profile = (cli_meta or {}).get("profile") or None if not _session_visible_to_active_profile(_session_profile, handler): + if _session_profile: + # Valid CLI/foreign session owned by a KNOWN other profile: + # 409 so the client can offer to switch to it (#5419). + return j(handler, { + "error": "Session belongs to a different profile", + "code": "session_profile_mismatch", + "session_id": sid, + "profile": _session_profile, + }, status=409) + # Missing session (cli_meta={} -> profile=None): keep the 404 + # self-heal path. _profiles_match coerces None->'default', so a + # truly-missing session under a non-default active profile would + # otherwise emit a useless 409 with profile=null and skip the + # frontend self-heal + spin the SSE reconnect against a dead sid. return bad(handler, "Session not found", 404) synth, reason = _claim_or_synthesize_cli_session(sid, cli_meta=cli_meta or {}) if reason == "was_webui": diff --git a/api/streaming.py b/api/streaming.py index 6eeb3892..4bc49fe0 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -1389,6 +1389,40 @@ def _agent_result_tool_limit_reached(result) -> bool: return False +def _maybe_inject_max_iteration_summary_fallback(messages, result) -> list: + """Append the agent's graceful summary text as an assistant turn when one is missing. + + When ``AIAgent`` exhausts its iteration budget, ``agent.handle_max_iterations`` + always returns a non-empty ``final_response`` — either the model-generated + summary or a graceful fallback (e.g. ``"I reached the iteration limit and + couldn't generate a summary."``). Hermes Agent surfaces that string as the + final answer to the user; the WebUI, by contrast, reads only ``messages``, + so an empty summary (common with reasoning-only responses) left the user + with a bare ``tool_limit_reached`` error instead of any closure text. + + When ``_tool_limit_reached`` is true and ``messages`` ends without a final + assistant answer, inject ``result['final_response']`` as a new assistant + turn so ``_mark_latest_assistant_tool_limit_status`` can attach the status + card in the normal flow and the user sees the same closure text as + hermes-agent. Returns the (possibly new) messages list; does nothing when + a usable assistant answer already exists or when ``result`` carries no + graceful fallback text. + """ + if not isinstance(result, dict): + return list(messages or []) + fallback = result.get('final_response') + if not isinstance(fallback, str) or not fallback.strip(): + return list(messages or []) + out = list(messages or []) + if not _session_lacks_final_assistant_answer(out): + return out + # Append a synthetic summary turn. Tag it so downstream consumers can + # distinguish it from model-emitted assistant turns if needed; mirrors the + # synthetic-scaffolding flag convention already used elsewhere (#5334). + out.append({"role": "assistant", "content": fallback, "_max_iteration_summary_fallback": True}) + return out + + def _mark_latest_assistant_tool_limit_status(messages) -> bool: """Annotate the latest usable assistant final answer as limit-stopped.""" for msg in reversed(list(messages or [])): @@ -8239,6 +8273,24 @@ def _run_agent_streaming( _result_messages, enabled=_tool_limit_reached, ) + # #5494 — parity with hermes-agent's handle_max_iterations() return + # value. When the agent produced no usable summary assistant + # message but result['final_response'] carries a graceful fallback + # string, inject it as a final assistant turn so the user sees + # closure text instead of a bare tool_limit_reached error. Apply + # the synthesis to result['messages'] AND _result_messages so the + # downstream _all_result_messages checks (silent-failure detection + # at api/streaming.py:_assistant_reply_added_after_current_turn) + # see the fallback too. `finalize_turn` in the agent always returns + # messages as a list, but we write back unconditionally so the + # contract is "if we built a result-messages list, the silent-failure + # classifier reads the augmented version." + if _tool_limit_reached: + _result_messages = _maybe_inject_max_iteration_summary_fallback( + _result_messages, result + ) + if isinstance(result, dict): + result = {**result, 'messages': _result_messages} if cancel_event.is_set(): _finalize_cancelled_turn(s, ephemeral=False) try: diff --git a/api/workspace_git.py b/api/workspace_git.py index a33fd316..457d8720 100644 --- a/api/workspace_git.py +++ b/api/workspace_git.py @@ -12,6 +12,7 @@ import logging import os import shutil import subprocess +import sys import tempfile import threading import re @@ -22,6 +23,20 @@ from typing import Iterable logger = logging.getLogger(__name__) + +def _windows_hide_flags() -> int: + """Win32 ``creationflags`` that hide a short-lived console child's window + (``CREATE_NO_WINDOW``) without detaching it, so ``capture_output`` still + works. Returns ``0`` on non-Windows — the ``subprocess`` default, a genuine + no-op. Mirrors the ``api/updates.py`` pattern; kept local so workspace-git + never takes a hard dependency on the optional ``hermes_cli`` package (a + standalone/agent-less WebUI must keep full git functionality). See #5692. + """ + if sys.platform == "win32": + return getattr(subprocess, "CREATE_NO_WINDOW", 0) + return 0 + + from api.workspace import rmtree_anchored, safe_resolve_ws, unlink_anchored @@ -248,6 +263,7 @@ def _run_git( text=True, timeout=timeout, env=run_env, + creationflags=_windows_hide_flags(), ) except subprocess.TimeoutExpired as exc: raise GitWorkspaceError("Git command timed out", "timeout") from exc @@ -290,6 +306,7 @@ def _config_names_for_scope( capture_output=True, timeout=GIT_TIMEOUT, env=env, + creationflags=_windows_hide_flags(), ) if result.returncode not in {0, 1}: if ignore_unsupported: diff --git a/static/sessions.js b/static/sessions.js index b4726c53..71092b2e 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -1299,6 +1299,51 @@ function _rearmActiveSessionStream(){ if(activeSid) startSessionStream(activeSid); } +function _sessionProfileMismatchFromError(e){ + if(!e || e.status!==409 || !e.body) return null; + try{ + const body=JSON.parse(e.body); + if(body && body.code==='session_profile_mismatch' && body.profile){ + return {profile:String(body.profile), session_id:String(body.session_id||'')}; + } + }catch(_){ } + return null; +} + +async function _switchProfileForSessionLoad(profile){ + const name=String(profile||'').trim(); + if(!name) throw new Error('missing profile'); + if(name===S.activeProfile) return; + if(typeof _invalidateSessionListRenders==='function') _invalidateSessionListRenders(); + if(typeof _setProfileSwitchListEmbargo==='function') _setProfileSwitchListEmbargo(true); + if(typeof showSessionListSkeleton==='function') showSessionListSkeleton(name); + try{ + const data=await api('/api/profile/switch',{method:'POST',body:JSON.stringify({name}),timeoutToast:false}); + S.activeProfile=data.active||name; + S.activeProfileIsDefault=!!data.is_default; + if(typeof _clearPersistedModelState==='function') _clearPersistedModelState(); + else localStorage.removeItem('hermes-webui-model'); + if(data.default_model) window._defaultModel=data.default_model; + if(data.default_model_provider) window._activeProvider=data.default_model_provider; + if(typeof startGatewaySSE==='function') startGatewaySSE(); + if(typeof syncTopbar==='function') syncTopbar(); + if(typeof _setProfileSwitchListEmbargo==='function') _setProfileSwitchListEmbargo(false); + if(typeof renderSessionList==='function') await renderSessionList(); + }catch(switchErr){ + // The switch POST failed, so we're still on the previous profile and its + // caches are intact. Clear the up-front skeleton and re-render the real + // list so the sidebar doesn't strand on the skeleton (the #4671 strand bug + // — _sessionListSkeletonActive hard-gates renderSessionListFromCache + the + // SSE/poll repaints until an unrelated full render fires). Mirror the + // canonical switch's catch in panels.js, then rethrow so loadSession's + // catch(switchErr) still routes into the generic error handler. + if(typeof _setProfileSwitchListEmbargo==='function') _setProfileSwitchListEmbargo(false); + _sessionListSkeletonActive=false; + if(typeof renderSessionListFromCache==='function') renderSessionListFromCache(); + throw switchErr; + } +} + async function loadSession(sid){ const opts = arguments[1] || {}; if(!opts.skipLineageResolve && typeof _resolveSessionIdFromSidebarLineage==='function'){ @@ -1428,6 +1473,30 @@ async function loadSession(sid){ try { data = await api(`/api/session?session_id=${encodeURIComponent(sid)}&messages=0&resolve_model=0`); } catch(e) { + const profileMismatch=_sessionProfileMismatchFromError(e); + if(profileMismatch && profileMismatch.profile && !opts.skipProfileResolve){ + if (_loadingSessionId !== sid) { + _rearmActiveSessionStream(); + return; + } + try{ + if(typeof showToast==='function') showToast(`Switching to ${profileMismatch.profile} profile for this session…`,2200); + await _switchProfileForSessionLoad(profileMismatch.profile); + // Post-await stale-load guard (Codex): the profile switch above does a + // network POST + session-list re-render, during which the user may have + // navigated to a different session. If we no longer own the load, bail + // before clearing _loadingSessionId or retrying so the stale + // continuation can't hijack the UI back to the old target. + if (_loadingSessionId !== sid) { + _rearmActiveSessionStream(); + return; + } + if (_loadingSessionId === sid) _loadingSessionId = null; + return loadSession(sid,{...opts,skipProfileResolve:true,force:true}); + }catch(switchErr){ + e=switchErr; + } + } const _msgInner = $('msgInner'); // Stale-load guard (Codex): a newer loadSession() may have started while this // request was awaiting (e.g. the user clicked a healthy session during a diff --git a/static/ui.js b/static/ui.js index 653d1aa1..9a3c80d7 100644 --- a/static/ui.js +++ b/static/ui.js @@ -17666,7 +17666,18 @@ if(!S._expandedDirs) S._expandedDirs=new Set(); if(!S._dirCache) S._dirCache={}; function renderFileTree(){ - const box=$('fileTree');box.innerHTML=''; + const box=$('fileTree'); + // #5657: capture the scroll position before wiping the container. box.innerHTML='' + // detaches every row, collapsing scrollHeight so the browser clamps scrollTop to 0; + // without this, every expand/collapse, breadcrumb nav, refresh, and hidden-files + // toggle that re-runs renderFileTree() teleports the reader back to the top of a + // long tree. Restored only after the normal render tail below — the two early-return + // paths (no-workspace hides the box; empty-dir has nothing to scroll) legitimately + // reset. A plain scrollTop restore suffices here: expand/collapse insert/remove rows + // BELOW the clicked disclosure, so the clicked row keeps its offset from the top (no + // getBoundingClientRect anchor delta needed — that's only for prepend-above cases). + const prevScrollTop=box?box.scrollTop:0; + box.innerHTML=''; // Cache current dir entries S._dirCache[S.currentDir||'.']=S.entries; // Show empty-state when no workspace is set or the directory is empty (#703) @@ -17685,6 +17696,8 @@ function renderFileTree(){ return; } _renderTreeItems(box, visibleEntries, 0); + // #5657: restore the pre-wipe scroll position now that the tree is tall again. + if(box) box.scrollTop=prevScrollTop; } let _wsActiveDragPath=null; diff --git a/tests/test_bugfix_sweep.py b/tests/test_bugfix_sweep.py index c1fc6dba..039cf30a 100644 --- a/tests/test_bugfix_sweep.py +++ b/tests/test_bugfix_sweep.py @@ -141,6 +141,17 @@ def test_session_url_builder_strips_legacy_session_query_alias(): assert "current.searchParams.delete('session_id');" in helper +def test_cross_profile_session_deep_links_switch_profile_instead_of_self_healing(): + routes = (ROOT / "api" / "routes.py").read_text(encoding="utf-8") + sessions = (ROOT / "static" / "sessions.js").read_text(encoding="utf-8") + + assert '"code": "session_profile_mismatch"' in routes + assert 'if method == "GET" and path == "/api/session":' in routes + assert "function _sessionProfileMismatchFromError" in sessions + assert "_switchProfileForSessionLoad(profileMismatch.profile)" in sessions + assert "skipProfileResolve:true" in sessions + + def test_service_worker_precaches_same_origin_vendor_shell_assets(): sw = (ROOT / "static" / "sw.js").read_text(encoding="utf-8") diff --git a/tests/test_issue1611_session_profile_filtering.py b/tests/test_issue1611_session_profile_filtering.py index 01fc0f35..63071a64 100644 --- a/tests/test_issue1611_session_profile_filtering.py +++ b/tests/test_issue1611_session_profile_filtering.py @@ -323,6 +323,37 @@ class _ProfileScopedSession: } +# Keys the profile-mismatch 409 envelope is ALLOWED to contain. Any key beyond +# these would mean session content is leaking across the profile boundary. +_ALLOWED_MISMATCH_KEYS = {"error", "code", "session_id", "profile"} + + +def _assert_profile_mismatch_envelope(captured, session_id, profile, *, leak_msg): + """#5419: a valid-but-wrong-profile /api/session load now returns a + structured 409 ``session_profile_mismatch`` envelope (so the frontend can + switch to the owning profile) instead of a misleading 404. This asserts the + new contract WHILE preserving the isolation guarantee this suite exists to + protect: the response body must carry ONLY the error envelope — never any + transcript/messages/title/content from the foreign-profile session. + """ + assert "bad" not in captured, ( + "wrong-profile session should no longer 404 via bad(); expected the 409 envelope" + ) + entry = captured.get("json") + assert entry is not None, "expected a structured 409 profile-mismatch response" + assert entry.get("status") == 409, f"expected status 409, got {entry.get('status')}" + data = entry.get("data") or {} + assert data.get("code") == "session_profile_mismatch" + assert data.get("profile") == profile + assert data.get("session_id") == session_id + assert "error" in data + # Boundary guard: no foreign-profile content may ride along in the envelope. + extra = set(data.keys()) - _ALLOWED_MISMATCH_KEYS + assert not extra, f"{leak_msg} (unexpected keys leaked: {sorted(extra)})" + for forbidden in ("messages", "content", "title", "workspace", "model", "tool_calls"): + assert forbidden not in data, f"{leak_msg} ('{forbidden}' present in envelope)" + + def test_get_session_rejects_session_from_inactive_profile(): """A known session_id from another profile must not bypass /api/sessions scoping. @@ -352,8 +383,12 @@ def test_get_session_rejects_session_from_inactive_profile(): patch("api.routes.j", side_effect=fake_j): routes.handle_get(SimpleNamespace(headers={"Cookie": "hermes_profile=default"}), parsed) - assert captured.get("bad", {}).get("status") == 404 - assert "json" not in captured, "foreign-profile transcript must not be returned" + # #5419: a valid-but-wrong-profile session now returns a structured 409 + # (session_profile_mismatch) so the frontend can switch profiles, instead + # of a misleading 404. The isolation boundary this suite protects still + # holds: the response carries ONLY the error envelope, never any transcript. + _assert_profile_mismatch_envelope(captured, "foreign_001", "other", + leak_msg="foreign-profile transcript must not be returned") def test_get_session_rejects_metadata_only_session_from_inactive_profile(): @@ -377,8 +412,8 @@ def test_get_session_rejects_metadata_only_session_from_inactive_profile(): patch("api.routes.j", side_effect=fake_j): routes.handle_get(SimpleNamespace(headers={"Cookie": "hermes_profile=default"}), parsed) - assert captured.get("bad", {}).get("status") == 404 - assert "json" not in captured, "foreign-profile metadata must not be returned" + _assert_profile_mismatch_envelope(captured, "foreign_001", "other", + leak_msg="foreign-profile metadata must not be returned") def test_get_session_rejects_cookieless_session_from_inactive_profile(): @@ -402,8 +437,8 @@ def test_get_session_rejects_cookieless_session_from_inactive_profile(): patch("api.routes.j", side_effect=fake_j): routes.handle_get(SimpleNamespace(headers={}), parsed) - assert captured.get("bad", {}).get("status") == 404 - assert "json" not in captured, "cookieless foreign-profile metadata must not be returned" + _assert_profile_mismatch_envelope(captured, "foreign_001", "other", + leak_msg="cookieless foreign-profile metadata must not be returned") def test_get_session_rejects_cli_session_from_inactive_profile(): @@ -430,8 +465,76 @@ def test_get_session_rejects_cli_session_from_inactive_profile(): patch("api.routes.j", side_effect=fake_j): routes.handle_get(SimpleNamespace(headers={"Cookie": "hermes_profile=default"}), parsed) - assert captured.get("bad", {}).get("status") == 404 - assert "json" not in captured, "foreign-profile CLI transcript must not be returned" + _assert_profile_mismatch_envelope(captured, "cli_foreign", "other", + leak_msg="foreign-profile CLI transcript must not be returned") + + +def test_missing_session_under_nondefault_profile_still_404_primary_branch(): + """#5419 regression (Fable Finding 1): a truly-missing/legacy session whose + owning profile is UNKNOWN (None) must keep the 404 self-heal path even when + the active profile is non-default — NOT emit a useless 409 with profile=null. + + _profiles_match coerces a None row-profile to 'default', so visibility fails + against a non-default active profile; the fix must fall back to 404 (not 409) + when _session_profile is falsy so the frontend self-heal + empty-state still + fire (and it doesn't spin the SSE reconnect against a dead session id). + """ + import api.routes as routes + + captured = {} + + def fake_bad(_handler, message, status=400, **_kwargs): + captured["bad"] = {"message": message, "status": status} + return captured["bad"] + + def fake_j(_handler, data, status=200, **_kwargs): + captured["json"] = {"data": data, "status": status} + return captured["json"] + + # None-profile sidecar (legacy/missing) + a NON-DEFAULT active profile. + parsed = urlparse("/api/session?session_id=ghost_001&messages=0&resolve_model=0") + with patch("api.routes._get_active_profile_name", return_value="research"), \ + patch("api.routes.get_session", return_value=_ProfileScopedSession(session_id="ghost_001", profile=None)), \ + patch("api.routes._lookup_cli_session_metadata", return_value={}), \ + patch("api.routes.bad", side_effect=fake_bad), \ + patch("api.routes.j", side_effect=fake_j): + routes.handle_get(SimpleNamespace(headers={"Cookie": "hermes_profile=research"}), parsed) + + assert captured.get("bad", {}).get("status") == 404, ( + "unknown-profile (None) session must 404 for self-heal, not a profile=null 409" + ) + assert "json" not in captured, "must not emit a 409 envelope for an unknown-profile session" + + +def test_missing_session_under_nondefault_profile_still_404_cli_branch(): + """#5419 regression (Fable Finding 1), CLI/foreign fallback branch: a truly + missing session (cli_meta={} -> profile=None) under a non-default active + profile must keep the 404 self-heal, not a profile=null 409.""" + import api.routes as routes + + captured = {} + + def fake_bad(_handler, message, status=400, **_kwargs): + captured["bad"] = {"message": message, "status": status} + return captured["bad"] + + def fake_j(_handler, data, status=200, **_kwargs): + captured["json"] = {"data": data, "status": status} + return captured["json"] + + parsed = urlparse("/api/session?session_id=ghost_cli&messages=1&resolve_model=0") + with patch("api.routes._get_active_profile_name", return_value="research"), \ + patch("api.routes.get_session", side_effect=KeyError), \ + patch("api.routes.SESSION_INDEX_FILE", SimpleNamespace(exists=lambda: False)), \ + patch("api.routes._lookup_cli_session_metadata", return_value={}), \ + patch("api.routes.bad", side_effect=fake_bad), \ + patch("api.routes.j", side_effect=fake_j): + routes.handle_get(SimpleNamespace(headers={"Cookie": "hermes_profile=research"}), parsed) + + assert captured.get("bad", {}).get("status") == 404, ( + "missing CLI session must 404 for self-heal, not a profile=null 409" + ) + assert "json" not in captured, "must not emit a 409 envelope for a missing CLI session" # ── Direct session export must also honor active profile ───────────────── diff --git a/tests/test_issue5657_filetree_scroll_preserve.py b/tests/test_issue5657_filetree_scroll_preserve.py new file mode 100644 index 00000000..fb21fd46 --- /dev/null +++ b/tests/test_issue5657_filetree_scroll_preserve.py @@ -0,0 +1,64 @@ +"""Regression coverage for #5657 — workspace file-tree preserves scroll on re-render. + +`renderFileTree()` clears its scroll container with ``box.innerHTML=''`` and +rebuilds every row. That detaches the rows, collapses ``scrollHeight``, and the +browser clamps ``scrollTop`` to 0 — so every folder expand/collapse, breadcrumb +nav, refresh, and hidden-files toggle that re-runs the renderer teleported the +reader to the top of a long tree. + +Fix: capture ``scrollTop`` before the wipe and restore it after the normal +render tail. A plain scrollTop restore is sufficient (expand/collapse insert or +remove rows BELOW the clicked disclosure, so the clicked row keeps its offset) — +the reporter's getBoundingClientRect anchor sketch is deliberately NOT used, and +the ``.file-item`` rows carry no ``data-path`` for it anyway. +""" + +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parents[1] +UI_JS = (REPO_ROOT / "static" / "ui.js").read_text(encoding="utf-8") + + +def _render_file_tree_body() -> str: + start = UI_JS.index("function renderFileTree()") + end = UI_JS.index("\nfunction ", start + 1) + return UI_JS[start:end] + + +def _render_file_tree_code() -> str: + """Body with // comment lines stripped, so assertions anchor on real code.""" + lines = [ln for ln in _render_file_tree_body().splitlines() if not ln.strip().startswith("//")] + return "\n".join(lines) + + +def test_render_file_tree_captures_and_restores_scrolltop(): + body = _render_file_tree_code() + # It must still wipe the container (the behavior that loses scroll)... + assert "box.innerHTML=''" in body or 'box.innerHTML = ""' in body + # ...and it must capture scrollTop BEFORE the wipe and restore it after. + assert "scrollTop" in body, ( + "renderFileTree must capture + restore scrollTop around the innerHTML wipe (#5657)" + ) + capture_idx = body.index("prevScrollTop=box") + wipe_idx = body.index("box.innerHTML=''") + assert capture_idx < wipe_idx, ( + "scrollTop must be captured BEFORE box.innerHTML='' so it isn't already clamped to 0" + ) + render_idx = body.index("_renderTreeItems(box") + restore_idx = body.rindex("box.scrollTop=") + assert restore_idx > render_idx, ( + "scrollTop must be restored AFTER _renderTreeItems repaints the tree" + ) + + +def test_early_return_paths_still_reset_scroll(): + # The no-workspace and empty-dir early returns legitimately want scroll reset + # (box hidden / nothing to scroll) — the restore must live on the normal tail + # only, i.e. AFTER _renderTreeItems, not before the early returns. + body = _render_file_tree_code() + render_idx = body.index("_renderTreeItems(box") + tail = body[render_idx:] + assert "box.scrollTop=" in tail or "box.scrollTop =" in tail, ( + "the scrollTop restore must be on the normal render tail, after _renderTreeItems" + ) diff --git a/tests/test_tool_limit_terminal_state.py b/tests/test_tool_limit_terminal_state.py index 80e4cd38..dcc6e61e 100644 --- a/tests/test_tool_limit_terminal_state.py +++ b/tests/test_tool_limit_terminal_state.py @@ -280,6 +280,132 @@ def test_streaming_tool_limit_without_final_answer_emits_no_final_apperror(tmp_p ) +def test_streaming_tool_limit_with_fallback_final_response_surfaces_closure_text(tmp_path, monkeypatch): + """#5494 — handle_max_iterations() guarantees a non-empty ``final_response`` + on iteration-limit exhaustion. This test pins the WebUI contract that, + when ``messages`` ends without a final assistant turn and ``final_response`` + is set, the user sees that closure text instead of a bare + ``tool_limit_reached`` error. Serves both as a live-bug fix pin and as a + regression guard for the agent's "delivered final_response ⇒ assistant + row" invariant: if a future agent regression drops that invariant, this + test still passes because the WebUI honors the contract locally. + """ + graceful = "I reached the iteration limit and couldn't generate a summary." + result = { + "turn_exit_reason": "max_iterations_reached(30/30)", + "final_response": graceful, + "messages": [ + {"role": "user", "content": "Do the long task."}, + {"role": "assistant", "content": "", "tool_calls": [{"id": "call_1"}]}, + {"role": "tool", "tool_call_id": "call_1", "content": "result"}, + {"role": "user", "content": streaming._MAX_ITERATION_SUMMARY_REQUEST}, + ], + } + + events, payload = _run_streaming_with_fake_agent(tmp_path, monkeypatch, result) + + # The user sees the graceful fallback, not a bare tool_limit_reached error. + # Either (a) we synthesized the fallback here, or (b) the agent guarantee + # already added an assistant row and `_mark_latest_assistant_tool_limit_status` + # attached the status card. Both routes satisfy the contract. + assert not [ + ap for ev, ap in events if ev == "apperror" + and ap.get("type") == "tool_limit_reached" + ], "expected no tool_limit_reached apperror when fallback was returned" + done_payloads = [payload for event, payload in events if event == "done"] + assert done_payloads, "expected done SSE payload" + assert done_payloads[-1]["terminal_state"] == "tool_limit_reached" + + # Fallback text is shown as a final assistant message and is annotated + # with the status card so the UI can render the 'limit reached' chip. + assistant = payload["messages"][-1] + assert assistant["role"] == "assistant" + assert assistant["content"] == graceful + assert assistant["_terminal_state"] == "tool_limit_reached" + assert assistant["_statusCard"]["title"] == "Tool iteration limit reached" + # Synthetic scaffolding turn was still dropped, even after fallback injection. + assert all( + message.get("content") != streaming._MAX_ITERATION_SUMMARY_REQUEST + for message in payload["messages"] + ) + + +def test_streaming_tool_limit_with_fallback_does_not_double_inject_when_assistant_exists(tmp_path, monkeypatch): + """#5494 — when the agent already appended a model-generated summary + AND ``final_response`` carries the same text, the WebUI must not duplicate + the assistant turn. Pins the no-op contract on the synthesis path. + """ + summary = "I reached the limit; here is the summary." + result = { + "turn_exit_reason": "max_iterations_reached(30/30)", + "final_response": summary, + "messages": [ + {"role": "user", "content": "Do the long task."}, + {"role": "assistant", "content": "", "tool_calls": [{"id": "call_1"}]}, + {"role": "tool", "tool_call_id": "call_1", "content": "result"}, + {"role": "user", "content": streaming._MAX_ITERATION_SUMMARY_REQUEST}, + {"role": "assistant", "content": summary}, + ], + } + + events, payload = _run_streaming_with_fake_agent(tmp_path, monkeypatch, result) + + done_payloads = [payload for event, payload in events if event == "done"] + assert done_payloads + assistant_msgs = [ + m for m in payload["messages"] + if m.get("role") == "assistant" and m.get("content") == summary + ] + assert len(assistant_msgs) == 1, "fallback must not duplicate the existing summary" + assert assistant_msgs[0]["_terminal_state"] == "tool_limit_reached" + + +def test_maybe_inject_max_iteration_summary_fallback_unit(): + """Unit-level coverage for the injection helper.""" + messages = [ + {"role": "user", "content": "Do the long task."}, + {"role": "assistant", "content": "", "tool_calls": [{"id": "call_1"}]}, + {"role": "tool", "tool_call_id": "call_1", "content": "result"}, + ] + graceful = "I reached the iteration limit and couldn't generate a summary." + result = {"final_response": graceful} + + injected = streaming._maybe_inject_max_iteration_summary_fallback(messages, result) + + assert injected[-1]["role"] == "assistant" + assert injected[-1]["content"] == graceful + assert injected[-1]["_max_iteration_summary_fallback"] is True + + +def test_maybe_inject_max_iteration_summary_fallback_skips_when_assistant_present(): + messages = [ + {"role": "user", "content": "Do the long task."}, + {"role": "assistant", "content": "real summary"}, + ] + result = {"final_response": "fallback text"} + + out = streaming._maybe_inject_max_iteration_summary_fallback(messages, result) + assert out == messages + + +def test_maybe_inject_max_iteration_summary_fallback_skips_when_no_fallback(): + messages = [ + {"role": "user", "content": "Do the long task."}, + {"role": "assistant", "content": "", "tool_calls": [{"id": "call_1"}]}, + ] + + out = streaming._maybe_inject_max_iteration_summary_fallback(messages, {}) + assert out == messages + + out = streaming._maybe_inject_max_iteration_summary_fallback( + messages, {"final_response": " "} + ) + assert out == messages + + out = streaming._maybe_inject_max_iteration_summary_fallback(messages, None) + assert out == messages + + def test_streaming_tool_limit_terminal_failure_does_not_mark_final_answer(tmp_path, monkeypatch): result = { "status": "partial", diff --git a/tests/test_workspace_git.py b/tests/test_workspace_git.py index f9e28ac3..98e0838d 100644 --- a/tests/test_workspace_git.py +++ b/tests/test_workspace_git.py @@ -2515,3 +2515,61 @@ def test_dirty_worktree_uses_filter_neutralization(tmp_path): _dirty_worktree(ctx) assert not marker.exists() + + +def test_run_git_passes_windows_hide_flags(monkeypatch, tmp_path): + """_run_git must pass creationflags=_windows_hide_flags() so git child + processes don't accumulate visible console windows on Windows (#5692). + windows_hide_flags() is 0 on non-Windows, so this is a safe no-op there; + the test asserts the kwarg is wired regardless of platform.""" + import api.workspace_git as wg + from api.workspace_git import _windows_hide_flags + + repo = _init_repo(tmp_path / "repo") + (repo / "f.txt").write_text("x\n", encoding="utf-8") + _commit_all(repo) + + captured = {} + real_run = subprocess.run + + def fake_run(*args, **kwargs): + captured["creationflags"] = kwargs.get("creationflags", "MISSING") + return real_run(*args, **kwargs) + + monkeypatch.setattr(wg.subprocess, "run", fake_run) + wg._run_git(repo, ["rev-parse", "HEAD"]) + + assert captured.get("creationflags") == _windows_hide_flags(), ( + "_run_git must pass creationflags=_windows_hide_flags() to subprocess.run" + ) + + +def test_config_names_for_scope_passes_windows_hide_flags(monkeypatch, tmp_path): + """_config_names_for_scope must also pass creationflags=_windows_hide_flags() + (#5692) — the git-config probe spawns a console window on Windows too.""" + import re as _re + import api.workspace_git as wg + from api.workspace_git import _windows_hide_flags + + repo = _init_repo(tmp_path / "repo") + + captured = {} + real_run = subprocess.run + + def fake_run(*args, **kwargs): + captured["creationflags"] = kwargs.get("creationflags", "MISSING") + return real_run(*args, **kwargs) + + monkeypatch.setattr(wg.subprocess, "run", fake_run) + wg._config_names_for_scope( + "--local", + repo, + {}, + "filter\\..*", + _re.compile(r"^filter\."), + ignore_unsupported=True, + ) + + assert captured.get("creationflags") == _windows_hide_flags(), ( + "_config_names_for_scope must pass creationflags=_windows_hide_flags()" + )