diff --git a/agent/context_compressor.py b/agent/context_compressor.py index d4441a1c7ec2..e9a9ff7a5ddc 100644 --- a/agent/context_compressor.py +++ b/agent/context_compressor.py @@ -940,6 +940,37 @@ def _sanitize_tool_pairs(self, messages: List[Dict[str, Any]]) -> List[Dict[str, if not self.quiet_mode: logger.info("Compression sanitizer: added %d stub tool result(s)", len(missing_results)) + # 3. Remove tool messages at the start of the list (or after a non-assistant + # message) that have no preceding assistant with tool_calls. This handles + # the boundary case where compress_end lands on a tool message whose parent + # assistant was in the summarised region. The orphan removal in step 1 + # catches tool results whose call_id no longer matches any surviving + # assistant, but some providers (DeepSeek, etc.) additionally require that + # every ``tool`` message is *immediately* preceded by the ``assistant`` + # message that issued the corresponding ``tool_calls``. + filtered: List[Dict[str, Any]] = [] + last_assistant_had_calls = False + removed_boundary = 0 + for msg in messages: + if msg.get("role") == "tool": + # Valid only if immediately preceded by an assistant with tool_calls + # or another tool message from the same group + if not last_assistant_had_calls and not ( + filtered and filtered[-1].get("role") == "tool" + ): + removed_boundary += 1 + continue + filtered.append(msg) + last_assistant_had_calls = ( + msg.get("role") == "assistant" and bool(msg.get("tool_calls")) + ) + if removed_boundary and not self.quiet_mode: + logger.info( + "Compression sanitizer: removed %d tool message(s) at invalid boundary", + removed_boundary, + ) + messages = filtered + return messages def _align_boundary_forward(self, messages: List[Dict[str, Any]], idx: int) -> int: diff --git a/gateway/platforms/whatsapp.py b/gateway/platforms/whatsapp.py index a82417a6015c..7a163c71dee0 100644 --- a/gateway/platforms/whatsapp.py +++ b/gateway/platforms/whatsapp.py @@ -379,12 +379,15 @@ async def connect(self) -> bool: if not (bridge_dir / "node_modules").exists(): print(f"[{self.name}] Installing WhatsApp bridge dependencies...") try: + # Read timeout from environment variable, default to 300 seconds (5 minutes) + # to accommodate slower systems like Unraid NAS + npm_install_timeout = int(os.environ.get("WHATSAPP_NPM_INSTALL_TIMEOUT", "300")) install_result = subprocess.run( ["npm", "install", "--silent"], cwd=str(bridge_dir), capture_output=True, text=True, - timeout=60, + timeout=npm_install_timeout, ) if install_result.returncode != 0: print(f"[{self.name}] npm install failed: {install_result.stderr}") diff --git a/tests/agent/test_context_compressor.py b/tests/agent/test_context_compressor.py index 8072a58d98f7..50c1f7d750cb 100644 --- a/tests/agent/test_context_compressor.py +++ b/tests/agent/test_context_compressor.py @@ -969,3 +969,93 @@ def test_pass3_emits_valid_json_for_downstream_provider(self): parsed = _json.loads(shrunk) assert parsed["path"] == "~/.hermes/skills/shopping/browser-setup-notes.md" assert parsed["content"].endswith("...[truncated]") + + +class TestSanitizeToolPairsBoundary: + """Regression: tool messages at compression boundary cause HTTP 400. + + After compression, the first message in the tail may be a ``tool`` + message whose parent ``assistant`` was summarised away. Some providers + (DeepSeek, etc.) require every ``tool`` message to be *immediately* + preceded by the ``assistant`` that issued the corresponding + ``tool_calls``. The sanitizer must strip such boundary orphans. + """ + + def test_tool_message_at_start_is_removed(self, compressor): + """A tool message at position 0 (no preceding assistant) is removed.""" + messages = [ + {"role": "tool", "tool_call_id": "tc_orphan", "content": "result"}, + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "hi"}, + ] + result = compressor._sanitize_tool_pairs(messages) + assert len(result) == 2 + assert result[0]["role"] == "user" + + def test_tool_message_after_summary_is_removed(self, compressor): + """A tool message after a summary (non-assistant-with-calls) is removed. + + Simulates the post-compression layout: [...head..., summary, tool, ...tail]. + """ + messages = [ + {"role": "system", "content": "sys"}, + {"role": "user", "content": "question"}, + {"role": "assistant", "content": "summary of earlier turns"}, + {"role": "tool", "tool_call_id": "tc_old", "content": "old result"}, + {"role": "user", "content": "follow up"}, + {"role": "assistant", "content": "answer"}, + ] + result = compressor._sanitize_tool_pairs(messages) + # The orphaned tool(tc_old) should be removed — its parent assistant + # was summarised and no longer has matching tool_calls. + tool_msgs = [m for m in result if m.get("role") == "tool"] + assert len(tool_msgs) == 0 + + def test_valid_tool_group_preserved(self, compressor): + """A valid assistant→tool group is NOT affected by the boundary check.""" + messages = [ + {"role": "user", "content": "question"}, + {"role": "assistant", "content": None, "tool_calls": [ + {"id": "tc_1", "type": "function", + "function": {"name": "read_file", "arguments": "{}"}}, + ]}, + {"role": "tool", "tool_call_id": "tc_1", "content": "file contents"}, + {"role": "user", "content": "thanks"}, + ] + result = compressor._sanitize_tool_pairs(messages) + assert len(result) == 4 + tool_msgs = [m for m in result if m.get("role") == "tool"] + assert len(tool_msgs) == 1 + + def test_consecutive_orphaned_tools_at_boundary(self, compressor): + """Multiple consecutive orphaned tool messages at the boundary are all removed.""" + messages = [ + {"role": "assistant", "content": "summary"}, + {"role": "tool", "tool_call_id": "tc_a", "content": "a"}, + {"role": "tool", "tool_call_id": "tc_b", "content": "b"}, + {"role": "user", "content": "next"}, + ] + result = compressor._sanitize_tool_pairs(messages) + assert len(result) == 2 + assert result[0]["role"] == "assistant" + assert result[1]["role"] == "user" + + def test_mixed_valid_and_orphaned_tools(self, compressor): + """Valid tool group preserved; orphaned tools after it removed.""" + messages = [ + {"role": "user", "content": "q1"}, + {"role": "assistant", "content": None, "tool_calls": [ + {"id": "tc_good", "type": "function", + "function": {"name": "foo", "arguments": "{}"}}, + ]}, + {"role": "tool", "tool_call_id": "tc_good", "content": "ok"}, + {"role": "assistant", "content": "summary"}, + {"role": "tool", "tool_call_id": "tc_bad", "content": "orphan"}, + {"role": "user", "content": "q2"}, + ] + result = compressor._sanitize_tool_pairs(messages) + # tc_good is valid (preceded by assistant with tool_calls) + # tc_bad is orphaned (preceded by assistant without tool_calls) + tool_msgs = [m for m in result if m.get("role") == "tool"] + assert len(tool_msgs) == 1 + assert tool_msgs[0]["tool_call_id"] == "tc_good" diff --git a/tests/tools/test_file_tools.py b/tests/tools/test_file_tools.py index 5a215df14a0e..979d309b09da 100644 --- a/tests/tools/test_file_tools.py +++ b/tests/tools/test_file_tools.py @@ -323,4 +323,41 @@ def test_truncated_hint_with_nonzero_offset(self, mock_get): assert "offset=100" in raw +class TestPatchSchema: + """Tests for PATCH_SCHEMA to ensure required parameters are properly declared.""" + + def test_patch_schema_includes_all_required_params(self): + """PATCH_SCHEMA should include all parameters that are conditionally required.""" + from tools.file_tools import PATCH_SCHEMA + + # Verify schema structure + assert "parameters" in PATCH_SCHEMA + assert "required" in PATCH_SCHEMA["parameters"] + + # All parameters that are mode-specific should be in required list + required = PATCH_SCHEMA["parameters"]["required"] + assert "mode" in required + assert "path" in required + assert "old_string" in required + assert "new_string" in required + assert "patch" in required + + # replace_all is optional (has default), so it should NOT be in required + assert "replace_all" not in required + + def test_patch_schema_description_mentions_mode_specific_requirements(self): + """PATCH_SCHEMA description should explain mode-specific requirements.""" + from tools.file_tools import PATCH_SCHEMA + + description = PATCH_SCHEMA.get("description", "") + + # Description should mention mode-specific requirements + assert "mode-specific" in description.lower() or "IMPORTANT:" in description + + # Should mention both modes + assert "mode='replace'" in description + assert "mode='patch'" in description + + + diff --git a/tools/file_tools.py b/tools/file_tools.py index 609506c05e1b..385a0146d778 100644 --- a/tools/file_tools.py +++ b/tools/file_tools.py @@ -908,18 +908,18 @@ def _check_file_reqs(): PATCH_SCHEMA = { "name": "patch", - "description": "Targeted find-and-replace edits in files. Use this instead of sed/awk in terminal. Uses fuzzy matching (9 strategies) so minor whitespace/indentation differences won't break it. Returns a unified diff. Auto-runs syntax checks after editing.\n\nReplace mode (default): find a unique string and replace it.\nPatch mode: apply V4A multi-file patches for bulk changes.", + "description": "Targeted find-and-replace edits in files. Use this instead of sed/awk in terminal. Uses fuzzy matching (9 strategies) so minor whitespace/indentation differences won't break it. Returns a unified diff. Auto-runs syntax checks after editing.\n\nIMPORTANT: Parameters are mode-specific:\n- For mode='replace': provide path, old_string, new_string (optionally replace_all)\n- For mode='patch': provide patch content", "parameters": { "type": "object", "properties": { "mode": {"type": "string", "enum": ["replace", "patch"], "description": "Edit mode: 'replace' for targeted find-and-replace, 'patch' for V4A multi-file patches", "default": "replace"}, - "path": {"type": "string", "description": "File path to edit (required for 'replace' mode)"}, - "old_string": {"type": "string", "description": "Text to find in the file (required for 'replace' mode). Must be unique in the file unless replace_all=true. Include enough surrounding context to ensure uniqueness."}, - "new_string": {"type": "string", "description": "Replacement text (required for 'replace' mode). Can be empty string to delete the matched text."}, + "path": {"type": "string", "description": "File path to edit (required when mode='replace')"}, + "old_string": {"type": "string", "description": "Text to find in file (required when mode='replace'). Must be unique in file unless replace_all=true. Include enough surrounding context to ensure uniqueness."}, + "new_string": {"type": "string", "description": "Replacement text (required when mode='replace'). Can be empty string to delete the matched text."}, "replace_all": {"type": "boolean", "description": "Replace all occurrences instead of requiring a unique match (default: false)", "default": False}, - "patch": {"type": "string", "description": "V4A format patch content (required for 'patch' mode). Format:\n*** Begin Patch\n*** Update File: path/to/file\n@@ context hint @@\n context line\n-removed line\n+added line\n*** End Patch"} + "patch": {"type": "string", "description": "V4A format patch content (required when mode='patch'). Format:\n*** Begin Patch\n*** Update File: path/to/file\n@@ context hint @@\n context line\n-removed line\n+added line\n*** End Patch"} }, - "required": ["mode"] + "required": ["mode", "path", "old_string", "new_string", "patch"] } }