From e64e02479f7b38787100378218bc583cb81f8dc5 Mon Sep 17 00:00:00 2001 From: Frank Song Date: Sun, 10 May 2026 20:44:34 +0800 Subject: [PATCH] Fix CLI session patch diff rendering --- CHANGELOG.md | 2 + static/ui.js | 151 +++++++++-- ...test_issue1824_cli_patch_diff_rendering.py | 245 ++++++++++++++++++ 3 files changed, 383 insertions(+), 15 deletions(-) create mode 100644 tests/test_issue1824_cli_patch_diff_rendering.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f60fbcd..35152371 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,8 @@ Three nice-to-have polish items called out by Opus that don't block this release ### Fixed +- **bug(cli-import): patch diffs invisible in imported CLI sessions** ([#1824](https://github.com/nesquena/hermes-webui/issues/1824)). Historical CLI sessions that predate session-level `tool_calls` fall back to reconstructing tool cards from per-message metadata in `static/ui.js`. That fallback truncated tool results to 200 chars and only showed the first 120 chars of tool arguments, so `apply_patch`/edit diffs recorded with `verbosity=all` could disappear behind a generic `Success` result. The renderer now preserves diff-like tool outputs, promotes `apply_patch`/edit payloads into the tool-card snippet when the result is non-diff, and labels long diff expanders as `Show diff`. Adds regression coverage for both the API payload preservation and the renderer fallback. + - **bug(profile/mcp): non-default profile MCP servers never load** ([#1968](https://github.com/nesquena/hermes-webui/issues/1968)). `_run_agent_streaming` called `discover_mcp_tools()` ~100 lines BEFORE the per-session `os.environ['HERMES_HOME'] = _profile_home` mutation, so MCP discovery always read the default profile's `~/.hermes/config.yaml` regardless of which profile the session was stamped with. Result: switching profiles in the WebUI dropdown was effectively cosmetic for MCP — non-default profiles never registered their stdio (npx/node) MCP servers. Fix relocates the `discover_mcp_tools()` call past the `_ENV_LOCK` env-mutation block so `get_hermes_home()` resolves to the session's actual profile home. Adds 4 static regression tests (`tests/test_issue1968_mcp_profile_discovery.py`) pinning the call ordering, lock-release placement, single call site, and try/except wrapping. **Caveat (out of scope, agent-side):** `_servers` in `tools/mcp_tool.py` is a process-global dict keyed only by server name, so concurrent use of multiple non-default profiles in the same WebUI process still has a "first profile wins per name" issue. Fully fixing that requires keying `_servers` by `(profile_home, name)` upstream in hermes-agent. This PR ships layer 1 only. ## [v0.51.31] — 2026-05-09 — Release H (12-PR contributor batch: image-mode + race fixes + composer drafts + locale parity) diff --git a/static/ui.js b/static/ui.js index abbf2fc7..1e6b3eee 100644 --- a/static/ui.js +++ b/static/ui.js @@ -4681,6 +4681,113 @@ function clearMessageRenderCache(){ _sessionHtmlCacheSid=null; } +function _clipCliToolSnippet(text, maxLen=20000){ + const s=String(text||''); + if(s.length<=maxLen) return s; + return `${s.slice(0,maxLen)}\n\n... truncated ${s.length-maxLen} chars ...`; +} + +function _cliToolResultText(raw){ + const s=String(raw||''); + try{ + const rd=JSON.parse(s); + if(rd && typeof rd==='object'){ + for(const key of ['output','result','error','content','diff','patch']){ + if(Object.prototype.hasOwnProperty.call(rd,key)){ + const v=rd[key]; + if(v==null) return ''; + return typeof v==='string' ? v : JSON.stringify(v,null,2); + } + } + } + }catch(e){} + return s; +} + +function _cliLooksLikePatchDiff(text){ + const s=String(text||''); + if(!s) return false; + if(/\*\*\* Begin Patch/.test(s)) return true; + if(/^diff --git /m.test(s)) return true; + if(/^@@\s/m.test(s)) return true; + if(/(^|\n)---\s+/.test(s) && /(^|\n)\+\+\+\s+/.test(s)) return true; + return false; +} + +function _cliToolResultSnippet(raw){ + const fullText=_cliToolResultText(raw); + if(_cliLooksLikePatchDiff(fullText)) return _clipCliToolSnippet(fullText); + return String(fullText||'').slice(0,200); +} + +function _prefixedCliDiffLines(prefix, value){ + return String(value||'').split('\n').map(line=>`${prefix}${line}`).join('\n'); +} + +function _firstOwnedValue(obj, keys){ + for(const key of keys){ + if(obj && Object.prototype.hasOwnProperty.call(obj,key)) return obj[key]; + } + return undefined; +} + +function _cliPatchSnippetFromArgs(name, args){ + if(!args || typeof args!=='object') return ''; + const toolName=String(name||'').toLowerCase(); + for(const key of ['patch','diff']){ + const v=args[key]; + if(typeof v==='string' && v.trim()) return _clipCliToolSnippet(v); + } + for(const key of ['input','content']){ + const v=args[key]; + if(typeof v==='string' && _cliLooksLikePatchDiff(v)) return _clipCliToolSnippet(v); + } + const isEditLike=toolName==='apply_patch' + || toolName==='patch' + || toolName.includes('edit') + || toolName==='replace' + || toolName==='str_replace'; + if(!isEditLike) return ''; + const oldValue=_firstOwnedValue(args,['old_string','old_str','old','before']); + const newValue=_firstOwnedValue(args,['new_string','new_str','new','after']); + if(oldValue!==undefined || newValue!==undefined){ + const path=String(_firstOwnedValue(args,['file_path','path','filename'])||''); + const lines=[]; + if(path) lines.push(path); + if(oldValue!==undefined) lines.push(_prefixedCliDiffLines('-', oldValue)); + if(newValue!==undefined) lines.push(_prefixedCliDiffLines('+', newValue)); + return _clipCliToolSnippet(lines.join('\n')); + } + if(Array.isArray(args.edits)){ + const path=String(_firstOwnedValue(args,['file_path','path','filename'])||''); + const chunks=[]; + if(path) chunks.push(path); + args.edits.slice(0,5).forEach(edit=>{ + if(!edit || typeof edit!=='object') return; + const before=_firstOwnedValue(edit,['old_string','old_str','old','before']); + const after=_firstOwnedValue(edit,['new_string','new_str','new','after']); + if(before!==undefined) chunks.push(_prefixedCliDiffLines('-', before)); + if(after!==undefined) chunks.push(_prefixedCliDiffLines('+', after)); + }); + if(chunks.length) return _clipCliToolSnippet(chunks.join('\n')); + } + return ''; +} + +function _cliToolCardSnippet(resultSnippet, patchSnippet){ + if(_cliLooksLikePatchDiff(resultSnippet)) return resultSnippet; + if(!patchSnippet) return resultSnippet || ''; + const result=String(resultSnippet||'').trim(); + if(!result) return patchSnippet; + const generic=/^(success|ok|done|done\.|exit code: 0)$/i.test(result); + if(generic) return patchSnippet; + return `${resultSnippet}\n\n${patchSnippet}`; +} + +function _cliToolCardHasDiffSnippet(resultSnippet, patchSnippet){ + return !!patchSnippet || _cliLooksLikePatchDiff(resultSnippet); +} + function _captureMessageScrollSnapshot(){ const el=$('messages'); if(!el) return null; @@ -5070,20 +5177,12 @@ function renderMessages(options){ // fallback-built cards carry their result snippet (not just the command). // Without this step CLI-origin sessions reload with empty tool cards. const resultsByTid={}; - const _snipFromRaw=(raw)=>{ - const s=String(raw||''); - try{ - const rd=JSON.parse(s); - if(rd && typeof rd==='object') return String(rd.output||rd.result||rd.error||s).slice(0,200); - }catch(e){} - return s.slice(0,200); - }; S.messages.forEach(m=>{ if(!m) return; // OpenAI / Hermes CLI format: role=tool with tool_call_id if(m.role==='tool'){ const tid=m.tool_call_id||m.tool_use_id||''; - if(tid) resultsByTid[tid]=_snipFromRaw(m.content); + if(tid) resultsByTid[tid]=_cliToolResultSnippet(m.content); return; } // Anthropic format: tool_result blocks inside a user message content array @@ -5095,7 +5194,7 @@ function renderMessages(options){ const raw=typeof p.content==='string'?p.content :Array.isArray(p.content)?p.content.map(c=>c&&c.text?c.text:'').join('') :''; - resultsByTid[tid]=_snipFromRaw(raw); + resultsByTid[tid]=_cliToolResultSnippet(raw); }); } }); @@ -5109,10 +5208,20 @@ function renderMessages(options){ const name=fn.name||tc.name||'tool'; let args={}; try{ args=JSON.parse(fn.arguments||'{}'); }catch(e){} + const tid=tc.id||tc.call_id||''; + const patchSnippet=_cliPatchSnippetFromArgs(name,args); + const resultSnippet=resultsByTid[tid]||''; let argsSnap={}; Object.keys(args).slice(0,4).forEach(k=>{ const v=String(args[k]); argsSnap[k]=v.slice(0,120)+(v.length>120?'...':''); }); - const tid=tc.id||tc.call_id||''; - derived.push({name,snippet:resultsByTid[tid]||'',tid,assistant_msg_idx:rawIdx,args:argsSnap,done:true}); + derived.push({ + name, + snippet:_cliToolCardSnippet(resultSnippet,patchSnippet), + is_diff:_cliToolCardHasDiffSnippet(resultSnippet,patchSnippet), + tid, + assistant_msg_idx:rawIdx, + args:argsSnap, + done:true, + }); }); // Anthropic format: tool_use blocks inside assistant content array if(Array.isArray(m.content)){ @@ -5120,12 +5229,22 @@ function renderMessages(options){ if(!p||typeof p!=='object'||p.type!=='tool_use') return; const name=p.name||'tool'; const args=p.input||{}; + const tid=p.id||''; + const patchSnippet=_cliPatchSnippetFromArgs(name,args); + const resultSnippet=resultsByTid[tid]||''; const argsSnap={}; if(args && typeof args==='object'){ Object.keys(args).slice(0,4).forEach(k=>{ const v=String(args[k]); argsSnap[k]=v.slice(0,120)+(v.length>120?'...':''); }); } - const tid=p.id||''; - derived.push({name,snippet:resultsByTid[tid]||'',tid,assistant_msg_idx:rawIdx,args:argsSnap,done:true}); + derived.push({ + name, + snippet:_cliToolCardSnippet(resultSnippet,patchSnippet), + is_diff:_cliToolCardHasDiffSnippet(resultSnippet,patchSnippet), + tid, + assistant_msg_idx:rawIdx, + args:argsSnap, + done:true, + }); }); } }); @@ -5347,6 +5466,8 @@ function buildToolCard(tc){ } } const hasMore=tc.snippet&&tc.snippet.length>displaySnippet.length; + const moreLabel=tc.is_diff?'Show diff':'Show more'; + const lessLabel=tc.is_diff?'Hide diff':'Show less'; const runIndicator=tc.done===false?'':''; const isSubagent=tc.name==='subagent_progress'; const isDelegation=tc.name==='delegate_task'; @@ -5370,7 +5491,7 @@ function buildToolCard(tc){ }`:''} ${displaySnippet?`
${esc(displaySnippet)}
- ${hasMore?``:''} + ${hasMore?``:''}
`:''} `:''} `; diff --git a/tests/test_issue1824_cli_patch_diff_rendering.py b/tests/test_issue1824_cli_patch_diff_rendering.py new file mode 100644 index 00000000..54280f6b --- /dev/null +++ b/tests/test_issue1824_cli_patch_diff_rendering.py @@ -0,0 +1,245 @@ +import json +import re +import sqlite3 +import subprocess +import textwrap +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[1] +UI_JS = (ROOT / "static" / "ui.js").read_text(encoding="utf-8") +COMPACT_UI = re.sub(r"\s+", "", UI_JS) + + +def test_cli_tool_result_diff_snippet_is_not_cut_to_200_chars(): + """Diff-like CLI tool results should reach the existing tool-card expander.""" + assert "function _cliToolResultSnippet" in UI_JS + assert "function _cliLooksLikePatchDiff" in UI_JS + assert r"\*\*\* Begin Patch" in UI_JS + assert "diff --git" in UI_JS + assert ( + "if(_cliLooksLikePatchDiff(fullText))return_clipCliToolSnippet(fullText);" + in COMPACT_UI + ) + assert "returnString(fullText||'').slice(0,200);" in COMPACT_UI + + +def test_cli_tool_fallback_promotes_apply_patch_args_to_tool_card_snippet(): + """A successful apply_patch result may only say 'Success'; keep the patch visible.""" + assert "function _cliPatchSnippetFromArgs" in UI_JS + assert "toolName==='apply_patch'" in COMPACT_UI + assert "'old_string'" in UI_JS + assert "'new_string'" in UI_JS + assert "constpatchSnippet=_cliPatchSnippetFromArgs(name,args);" in COMPACT_UI + assert "snippet:_cliToolCardSnippet(resultSnippet,patchSnippet)" in COMPACT_UI + assert "is_diff:_cliToolCardHasDiffSnippet(resultSnippet,patchSnippet)" in COMPACT_UI + + +def test_diff_tool_cards_use_show_diff_expander_label(): + assert "const moreLabel=tc.is_diff?'Show diff':'Show more';" in UI_JS + assert "const lessLabel=tc.is_diff?'Hide diff':'Show less';" in UI_JS + assert 'data-more-label="${esc(moreLabel)}"' in UI_JS + + +def _function_source(src: str, name: str) -> str: + match = re.search(rf"function\s+{re.escape(name)}\s*\(", src) + assert match, f"{name}() not found" + brace = src.find("{", match.end()) + assert brace != -1, f"{name}() has no body" + depth = 1 + i = brace + 1 + in_string = None + escaped = False + in_line_comment = False + in_block_comment = False + while i < len(src) and depth: + ch = src[i] + nxt = src[i + 1] if i + 1 < len(src) else "" + if in_line_comment: + if ch == "\n": + in_line_comment = False + i += 1 + continue + if in_block_comment: + if ch == "*" and nxt == "/": + in_block_comment = False + i += 2 + continue + i += 1 + continue + if in_string: + if escaped: + escaped = False + elif ch == "\\": + escaped = True + elif ch == in_string: + in_string = None + i += 1 + continue + if ch == "/" and nxt == "/": + in_line_comment = True + i += 2 + continue + if ch == "/" and nxt == "*": + in_block_comment = True + i += 2 + continue + if ch in "'\"`": + in_string = ch + i += 1 + continue + if ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + i += 1 + assert depth == 0, f"{name}() body did not close" + return src[match.start() : i] + + +def test_rendered_apply_patch_tool_card_html_contains_diff_lines(): + """Drive the actual snippet helpers and buildToolCard() through Node.""" + function_names = [ + "_clipCliToolSnippet", + "_cliToolResultText", + "_cliLooksLikePatchDiff", + "_cliToolResultSnippet", + "_prefixedCliDiffLines", + "_firstOwnedValue", + "_cliPatchSnippetFromArgs", + "_cliToolCardSnippet", + "_cliToolCardHasDiffSnippet", + "buildToolCard", + ] + functions = "\n".join(_function_source(UI_JS, name) for name in function_names) + script = textwrap.dedent( + f""" + function esc(s){{return String(s||'').replace(/[&<>]/g,c=>({{'&':'&','<':'<','>':'>'}}[c]));}} + function li(){{return '';}} + function toolIcon(){{return '';}} + function _toolDisplayName(tc){{return tc.name||'tool';}} + const document={{ + createElement(){{return {{className:'', innerHTML:''}};}} + }}; + {functions} + + const longPatch = [ + '*** Begin Patch', + '*** Update File: app.py', + '@@', + '-old', + '+new', + ...Array.from({{length: 150}}, (_, i) => '+line ' + i), + '*** End Patch' + ].join('\\n'); + const resultSnippet = _cliToolResultSnippet(JSON.stringify({{output:'Success'}})); + const patchSnippet = _cliPatchSnippetFromArgs('apply_patch', {{patch: longPatch}}); + const row = buildToolCard({{ + name: 'apply_patch', + snippet: _cliToolCardSnippet(resultSnippet, patchSnippet), + is_diff: _cliToolCardHasDiffSnippet(resultSnippet, patchSnippet), + args: {{patch: '(shown in diff)'}}, + done: true + }}); + const errorSnippet = _cliToolCardSnippet('Patch failed: context not found', patchSnippet); + process.stdout.write(JSON.stringify({{html: row.innerHTML, errorSnippet}})); + """ + ) + proc = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True) + payload = json.loads(proc.stdout) + html = payload["html"] + assert "-old" in html + assert "+new" in html + assert "Show diff" in html + assert "Patch failed: context not found" in payload["errorSnippet"] + assert "-old" in payload["errorSnippet"] + + +def _make_state_db(path: Path) -> None: + patch = "\n".join( + [ + "*** Begin Patch", + "*** Update File: app.py", + "@@", + "-old", + "+new", + "*** End Patch", + ] + ) + tool_calls = [ + { + "id": "call_patch", + "type": "function", + "function": { + "name": "apply_patch", + "arguments": json.dumps({"patch": patch}), + }, + } + ] + conn = sqlite3.Connection(str(path)) + try: + conn.executescript( + """ + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, + role TEXT, + content TEXT, + timestamp TEXT, + tool_call_id TEXT, + tool_calls TEXT, + tool_name TEXT + ); + """ + ) + conn.execute( + """ + INSERT INTO messages (session_id, role, content, timestamp, tool_calls) + VALUES (?, ?, ?, ?, ?) + """, + ("issue1824", "assistant", "", "2026-01-01T00:00:01Z", json.dumps(tool_calls)), + ) + conn.execute( + """ + INSERT INTO messages (session_id, role, content, timestamp, tool_call_id, tool_name) + VALUES (?, ?, ?, ?, ?, ?) + """, + ( + "issue1824", + "tool", + json.dumps({"output": "Success"}), + "2026-01-01T00:00:02Z", + "call_patch", + "apply_patch", + ), + ) + conn.commit() + finally: + conn.close() + + +def test_cli_session_reader_preserves_apply_patch_metadata(tmp_path, monkeypatch): + """The API payload should keep tool_calls/tool rows for the UI renderer.""" + _make_state_db(tmp_path / "state.db") + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + import api.profiles + from api.models import get_cli_session_messages + + monkeypatch.setattr(api.profiles, "get_active_hermes_home", lambda: str(tmp_path)) + + messages = get_cli_session_messages("issue1824") + assert [m["role"] for m in messages] == ["assistant", "tool"] + + assistant = messages[0] + assert assistant["tool_calls"][0]["function"]["name"] == "apply_patch" + args = json.loads(assistant["tool_calls"][0]["function"]["arguments"]) + assert "*** Begin Patch" in args["patch"] + assert "-old" in args["patch"] + assert "+new" in args["patch"] + + tool = messages[1] + assert tool["tool_call_id"] == "call_patch" + assert tool["tool_name"] == "apply_patch" + assert tool["name"] == "apply_patch" + assert json.loads(tool["content"])["output"] == "Success"