diff --git a/api/profiles.py b/api/profiles.py index f87ac985..fc94a336 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -150,7 +150,7 @@ def get_hermes_home_for_profile(name: str) -> Path: empty, 'default', or does not match the profile-name format (rejects path traversal such as '../../etc'). """ - if not name or name == 'default' or not _PROFILE_ID_RE.match(name): + if not name or name == 'default' or not _PROFILE_ID_RE.fullmatch(name): return _DEFAULT_HERMES_HOME profile_dir = _DEFAULT_HERMES_HOME / 'profiles' / name return profile_dir diff --git a/api/streaming.py b/api/streaming.py index e6c6700c..4ed12b83 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -2782,7 +2782,23 @@ def cancel_stream(stream_id: str) -> bool: if _has_reasoning: _partial_msg['reasoning'] = _cancel_reasoning.strip() if _has_tools: - _partial_msg['tool_calls'] = list(_cancel_tool_calls) + # NOTE: store under the private '_partial_tool_calls' key + # (NOT 'tool_calls'). The captured entries use the WebUI + # internal shape {name, args, done, duration, is_error} + # — they do NOT carry the OpenAI/Anthropic API id + + # function: {name, arguments} envelope. If we put them + # under 'tool_calls', `_sanitize_messages_for_api` + # (which whitelists 'tool_calls' via _API_SAFE_MSG_KEYS) + # would forward them to the next-turn LLM call and + # strict providers (OpenAI, Anthropic, Z.AI/GLM) would + # 400 on the malformed entries — turning a "data lost + # on cancel" bug into a "next message returns 400" + # bug, which is worse. The underscore-prefixed key is + # not in the whitelist, so sanitize strips it. The UI + # reads it via static/messages.js and renders it + # alongside the regular tool_calls path. + # (Opus pre-release review pass 2 of v0.50.251.) + _partial_msg['_partial_tool_calls'] = list(_cancel_tool_calls) _cs.messages.append(_partial_msg) # Cancel marker — flagged _error=True so it is stripped from conversation # history on the next turn (prevents model from seeing "Task cancelled." diff --git a/static/messages.js b/static/messages.js index e9e9b868..7bbf8c19 100644 --- a/static/messages.js +++ b/static/messages.js @@ -1072,9 +1072,14 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ S.session=session;S.messages=(session.messages||[]).filter(m=>m&&m.role); const hasMessageToolMetadata=S.messages.some(m=>{ if(!m||m.role!=='assistant') return false; + // Recognize both the standard `tool_calls` (used by completed assistant + // turns where the LLM emitted tool_call entries) and the WebUI-internal + // `_partial_tool_calls` (used on Stop/Cancel partial messages — see + // api/streaming.py cancel_stream). const hasTc=Array.isArray(m.tool_calls)&&m.tool_calls.length>0; + const hasPartialTc=Array.isArray(m._partial_tool_calls)&&m._partial_tool_calls.length>0; const hasTu=Array.isArray(m.content)&&m.content.some(p=>p&&p.type==='tool_use'); - return hasTc||hasTu; + return hasTc||hasPartialTc||hasTu; }); if(!hasMessageToolMetadata&&session.tool_calls&&session.tool_calls.length){ S.toolCalls=(session.tool_calls||[]).map(tc=>({...tc,done:true})); diff --git a/tests/test_issue1361_cancel_data_loss.py b/tests/test_issue1361_cancel_data_loss.py index c7f08d52..cdf89dcd 100644 --- a/tests/test_issue1361_cancel_data_loss.py +++ b/tests/test_issue1361_cancel_data_loss.py @@ -213,9 +213,9 @@ class TestCancelPreservesToolCalls: s2 = models.SESSIONS[sid] assistant_msgs = [m for m in s2.messages if isinstance(m, dict) and m.get('role') == 'assistant'] - has_tools = any(m.get('tool_calls') or m.get('tools') for m in assistant_msgs) + has_tools = any(m.get('_partial_tool_calls') or m.get('tool_calls') or m.get('tools') for m in assistant_msgs) assert has_tools, \ - f"Expected tool_calls on partial assistant msg after cancel. Got: {assistant_msgs}" + f"Expected _partial_tool_calls on partial assistant msg after cancel. Got: {assistant_msgs}" def test_cancel_with_tools_and_text_preserves_both(self): """Cancel after tools + partial text should keep both.""" diff --git a/tests/test_issue798.py b/tests/test_issue798.py index 702bef9f..4207400b 100644 --- a/tests/test_issue798.py +++ b/tests/test_issue798.py @@ -192,3 +192,17 @@ def test_get_hermes_home_for_profile_rejects_path_traversal(): assert p.get_hermes_home_for_profile('alice') == base / 'profiles' / 'alice' assert p.get_hermes_home_for_profile('my-profile') == base / 'profiles' / 'my-profile' assert p.get_hermes_home_for_profile('profile_1') == base / 'profiles' / 'profile_1' + # R19j coverage gaps closed in v0.50.251 per Opus pre-release review: + # - Trailing-newline names must be rejected (re.match would let them through; + # re.fullmatch correctly anchors $). Catches the match-vs-fullmatch footgun. + assert p.get_hermes_home_for_profile('valid\n') == base + assert p.get_hermes_home_for_profile('a\n') == base + # - Length boundaries: 64 chars (max valid: 1 + 63 suffix) routes to profile path, + # 65 chars rejected. + assert p.get_hermes_home_for_profile('a' * 64) == base / 'profiles' / ('a' * 64) + assert p.get_hermes_home_for_profile('a' * 65) == base + # - Single-char name is the minimum valid form. + assert p.get_hermes_home_for_profile('a') == base / 'profiles' / 'a' + # - Non-ASCII / Unicode-trick names are rejected by the ASCII-only charset. + assert p.get_hermes_home_for_profile('voilà') == base + assert p.get_hermes_home_for_profile('名前') == base diff --git a/tests/test_pr1375_partial_tool_calls_sanitize.py b/tests/test_pr1375_partial_tool_calls_sanitize.py new file mode 100644 index 00000000..1c4a5ff6 --- /dev/null +++ b/tests/test_pr1375_partial_tool_calls_sanitize.py @@ -0,0 +1,105 @@ +"""Regression test for the v0.50.251 Opus pass-2 MUST-FIX on PR #1375. + +Original PR #1375 stored cancelled tool calls under the message key +`tool_calls`. That key is whitelisted by `_API_SAFE_MSG_KEYS` so the +sanitize-for-API path forwarded them to the next-turn LLM call. But the +captured entries use the WebUI internal shape ({name, args, done, +duration, is_error}) — they don't have OpenAI/Anthropic's id + +function: {name, arguments} envelope. Strict providers (OpenAI, +Anthropic, Z.AI/GLM) would 400 on the malformed entries — turning a +"data lost on cancel" bug into a "next message returns 400" bug. + +The fix renames the key to `_partial_tool_calls` (underscore-prefixed +private key NOT in the whitelist), so sanitize correctly strips it. +The UI reads it via static/messages.js. + +This test pins the invariant: a partial assistant message with +`_partial_tool_calls` set must produce ZERO `tool_calls` after +sanitize-for-API. +""" +import pathlib +import sys + +import pytest + +REPO_ROOT = pathlib.Path(__file__).parent.parent.resolve() +sys.path.insert(0, str(REPO_ROOT)) + + +def test_partial_tool_calls_field_not_forwarded_to_llm(): + """The `_partial_tool_calls` field must not survive _sanitize_messages_for_api. + Otherwise the malformed entries get sent to the LLM and cause 400 errors.""" + from api.streaming import _sanitize_messages_for_api + + messages = [ + {"role": "user", "content": "do a search"}, + { + "role": "assistant", + "content": "Looking up...", + "_partial": True, + "_partial_tool_calls": [ + {"name": "web_search", "args": {"query": "x"}, "done": False}, + ], + "reasoning": "Let me think about this", + }, + ] + sanitized = _sanitize_messages_for_api(messages) + # The partial assistant message must NOT have _partial_tool_calls (private key). + # It must NOT have tool_calls (would bypass the rename and 400 the LLM). + # It must NOT have reasoning (not in whitelist). + assistant_msgs = [m for m in sanitized if m.get("role") == "assistant"] + assert assistant_msgs, "Sanitized output must include the assistant message" + for m in assistant_msgs: + assert "_partial_tool_calls" not in m, ( + "Sanitize-for-API must strip _partial_tool_calls — it's a UI-only key. " + f"Got: {m}" + ) + assert "tool_calls" not in m, ( + "Sanitize-for-API must NOT have tool_calls on a partial message — the " + "captured entries use WebUI shape and would 400 on strict providers. " + f"Got: {m}" + ) + assert "reasoning" not in m, ( + "Sanitize-for-API must strip reasoning (not in _API_SAFE_MSG_KEYS). " + f"Got: {m}" + ) + + +def test_legitimate_tool_calls_are_preserved_for_completed_turns(): + """Completed assistant turns with REAL tool_calls (with id + function envelope) + must still pass through sanitize unchanged. The rename only affects + cancel-partial messages, not normal completed turns.""" + from api.streaming import _sanitize_messages_for_api + + messages = [ + {"role": "user", "content": "search"}, + { + "role": "assistant", + "content": "I'll search.", + "tool_calls": [ + { + "id": "call_abc", + "type": "function", + "function": {"name": "web_search", "arguments": '{"query":"x"}'} + }, + ], + }, + { + "role": "tool", + "tool_call_id": "call_abc", + "name": "web_search", + "content": "result", + }, + ] + sanitized = _sanitize_messages_for_api(messages) + assistant_msgs = [m for m in sanitized if m.get("role") == "assistant"] + assert assistant_msgs, "Sanitized output must include the assistant message" + assert assistant_msgs[0].get("tool_calls"), ( + "Legitimate tool_calls on completed turns must survive sanitize. " + f"Got: {assistant_msgs[0]}" + ) + # Must still have the OpenAI envelope shape + tc = assistant_msgs[0]["tool_calls"][0] + assert "id" in tc and "function" in tc, ( + f"tool_calls envelope must be preserved. Got: {tc}" + )