diff --git a/gns3server/agent/gns3_copilot/tools_v2/config_tools_nornir.py b/gns3server/agent/gns3_copilot/tools_v2/config_tools_nornir.py index d16ef714c..485f02dbc 100644 --- a/gns3server/agent/gns3_copilot/tools_v2/config_tools_nornir.py +++ b/gns3server/agent/gns3_copilot/tools_v2/config_tools_nornir.py @@ -47,7 +47,6 @@ from nornir.core.task import AggregatedResult, Result, Task from nornir_netmiko.tasks import netmiko_send_config from gns3server.agent.gns3_copilot.gns3_client import get_gns3_server_host - # Import custom Netmiko device types for GNS3 emulation # This registers gns3_huawei_telnet_ce and other custom device types # NOTE: Must be imported BEFORE any Nornir operations to ensure device types are registered @@ -55,6 +54,9 @@ from gns3server.agent.gns3_copilot.utils import get_device_ports_from_topology from gns3server.agent.gns3_copilot.utils.command_filter import ( filter_forbidden_commands, ) +from gns3server.agent.gns3_copilot.utils.device_configs import ( + merge_duplicate_device_configs, +) # Explicitly register custom device types to ensure they are available # This is a safety measure in case the auto-registration on import doesn't work @@ -196,6 +198,9 @@ class ExecuteMultipleDeviceConfigCommands(BaseTool): if len(device_configs_list) > 0 and "error" in device_configs_list[0]: return device_configs_list + # Merge same-device entries before anything keys by device_name + device_configs_list = merge_duplicate_device_configs(device_configs_list, commands_field="config_commands") + # Filter forbidden commands and store blocked commands info device_configs_list, blocked_commands_map = self._filter_forbidden_commands_from_device_configs( device_configs_list diff --git a/gns3server/agent/gns3_copilot/tools_v2/display_tools_nornir.py b/gns3server/agent/gns3_copilot/tools_v2/display_tools_nornir.py index c7526f2c0..1569c224e 100644 --- a/gns3server/agent/gns3_copilot/tools_v2/display_tools_nornir.py +++ b/gns3server/agent/gns3_copilot/tools_v2/display_tools_nornir.py @@ -47,7 +47,6 @@ from nornir.core.task import AggregatedResult, Result, Task from nornir_netmiko.tasks import netmiko_multiline from gns3server.agent.gns3_copilot.gns3_client import get_gns3_server_host - # Import custom Netmiko device types for GNS3 emulation # This registers gns3_huawei_telnet_ce and other custom device types # NOTE: Must be imported BEFORE any Nornir operations to ensure device types are registered @@ -55,6 +54,9 @@ from gns3server.agent.gns3_copilot.utils import get_device_ports_from_topology from gns3server.agent.gns3_copilot.utils.command_filter import ( filter_forbidden_commands, ) +from gns3server.agent.gns3_copilot.utils.device_configs import ( + merge_duplicate_device_configs, +) # Explicitly register custom device types to ensure they are available # This is a safety measure in case the auto-registration on import doesn't work @@ -199,6 +201,9 @@ class ExecuteMultipleDeviceCommands(BaseTool): if len(device_configs_list) > 0 and "error" in device_configs_list[0]: return device_configs_list + # Merge same-device entries before anything keys by device_name + device_configs_list = merge_duplicate_device_configs(device_configs_list, commands_field="commands") + # Filter forbidden commands and store blocked commands info device_configs_list, blocked_commands_map = self._filter_forbidden_commands_from_device_configs( device_configs_list diff --git a/gns3server/agent/gns3_copilot/tools_v2/vpcs_tools_netmiko.py b/gns3server/agent/gns3_copilot/tools_v2/vpcs_tools_netmiko.py index 2148106e7..2444453f3 100644 --- a/gns3server/agent/gns3_copilot/tools_v2/vpcs_tools_netmiko.py +++ b/gns3server/agent/gns3_copilot/tools_v2/vpcs_tools_netmiko.py @@ -47,11 +47,13 @@ from nornir.core.task import AggregatedResult, Result, Task from nornir_netmiko.tasks import netmiko_multiline from gns3server.agent.gns3_copilot.gns3_client import get_gns3_server_host - # Import custom Netmiko device types for GNS3 emulation # This registers gns3_vpcs_telnet and other custom device types # NOTE: Must be imported BEFORE any Nornir operations to ensure device types are registered from gns3server.agent.gns3_copilot.utils import get_device_ports_from_topology +from gns3server.agent.gns3_copilot.utils.device_configs import ( + merge_duplicate_device_configs, +) # Explicitly register VPCS device type to ensure it is available try: @@ -179,6 +181,9 @@ class VPCSCommands(BaseTool): if len(device_configs_list) > 0 and "error" in device_configs_list[0]: return device_configs_list + # Merge same-device entries before anything keys by device_name + device_configs_list = merge_duplicate_device_configs(device_configs_list, commands_field="commands") + # Create a mapping of device names to their commands device_configs_map = self._configs_map(device_configs_list) diff --git a/gns3server/agent/gns3_copilot/utils/device_configs.py b/gns3server/agent/gns3_copilot/utils/device_configs.py new file mode 100644 index 000000000..e757eb089 --- /dev/null +++ b/gns3server/agent/gns3_copilot/utils/device_configs.py @@ -0,0 +1,71 @@ +# SPDX-License-Identifier: GPL-3.0-or-later +# +# GNS3-Copilot - AI-powered Network Lab Assistant for GNS3 +# +# This file is part of GNS3-Copilot project. +# +# GNS3-Copilot is free software: you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by the +# Free Software Foundation, either version 3 of the License, or (at your +# option) any later version. +# +# GNS3-Copilot is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General +# Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with GNS3-Copilot. If not, see . +# +# Copyright (C) 2025 Yue Guobin (岳国宾) +# Author: Yue Guobin (岳国宾) +# +# Project Home: https://github.com/yueguobin/gns3-copilot +# +""" +Device config entry normalization shared by the tools_v2 executors. + +Callers (LLM agents via MCP, or gns3-copilot directly) may send the same +device several times in one batch. Every downstream stage keys by +device_name, so duplicates must merge before execution, not after. +""" + +from typing import Any + + +def merge_duplicate_device_configs( + device_configs: list[dict[str, Any]], + commands_field: str, +) -> list[dict[str, Any]]: + """ + Merge entries that share a device_name, concatenating their command lists. + + Without this merge the per-device command map keeps only the last + entry's commands while result processing iterates every original entry — + duplicate devices would silently drop all but the last command batch and + every row would report that same output. + + Args: + device_configs: Device config entries; each holds device_name and a + command list under commands_field. + commands_field: Field carrying the commands ("commands" or + "config_commands" depending on the tool). + + Returns: + One entry per device_name, in first-appearance order. The first + entry's extra fields are preserved; later duplicates only contribute + their commands. + """ + merged: dict[str, dict[str, Any]] = {} + for config in device_configs: + name = config.get("device_name") + if not name: + continue + commands = list(config.get(commands_field, [])) + if name in merged: + merged[name][commands_field].extend(commands) + else: + entry = dict(config) + entry[commands_field] = commands + merged[name] = entry + return list(merged.values()) diff --git a/tests/agent/mcp/test_device_tools.py b/tests/agent/mcp/test_device_tools.py index db65412c8..e68075d3c 100644 --- a/tests/agent/mcp/test_device_tools.py +++ b/tests/agent/mcp/test_device_tools.py @@ -1,14 +1,24 @@ """ Device config tool tests with mocked topology and Nornir layers. -Covers the VPCS node-type guard and the error contract shared by -device_config_send / device_show_run / vpcs_config_set. +Covers the VPCS node-type guard, the error contract shared by +device_config_send / device_show_run / vpcs_config_set, and the +same-device entry merging that keeps duplicate entries from +overwriting each other's commands and outputs. """ import json from unittest.mock import MagicMock, patch +from gns3server.agent.gns3_copilot.utils.device_configs import ( + merge_duplicate_device_configs, +) + VPCS_MOD = "gns3server.agent.gns3_copilot.tools_v2.vpcs_tools_netmiko" +DISPLAY_MOD = "gns3server.agent.gns3_copilot.tools_v2.display_tools_nornir" +CONFIG_MOD = "gns3server.agent.gns3_copilot.tools_v2.config_tools_nornir" + +PROJECT_ID = "0c0fde25-6ead-4413-a283-ea8fd2324291" def _topology_ports(node_type): @@ -16,6 +26,26 @@ def _topology_ports(node_type): return {"PC1": {"port": 5000, "node_type": node_type}} +def _topology_two_devices(): + """Mocked topology with two console-reachable devices.""" + return { + "R1": {"port": 5000, "node_type": "iou"}, + "R2": {"port": 5001, "node_type": "iou"}, + } + + +def _aggregated_result(output_by_device): + """Real AggregatedResult: device -> MultiResult with one successful Result.""" + from nornir.core.task import AggregatedResult, MultiResult, Result + + aggregated = AggregatedResult("task") + for name, output in output_by_device.items(): + multi = MultiResult("task") + multi.append(Result(host=None, result=output, failed=False)) + aggregated[name] = multi + return aggregated + + class TestVPCSNodeTypeGuard: def test_non_vpcs_node_is_rejected(self): from gns3server.agent.gns3_copilot.tools_v2.vpcs_tools_netmiko import VPCSCommands @@ -161,3 +191,182 @@ class TestDeviceToolErrorContract: assert len(result) == 1 assert result[0]["status"] == "failed" assert "Template rendering failed" in result[0]["error"] + + +class TestDuplicateDeviceMerging: + """ + Same-device entries in one batch must merge before execution. + + Regression: the per-device command map kept only the last entry's + commands while result processing iterated every original entry, so all + rows reported the last entry's output (first reproduced with the MCP + direct-commands path, no template). + """ + + def test_show_run_merges_same_device_entries(self): + from gns3server.agent.gns3_copilot.tools_v2.display_tools_nornir import ( + ExecuteMultipleDeviceCommands, + ) + + nornir = MagicMock() + nornir.run.return_value = _aggregated_result({"R1": "merged output", "R2": "r2 output"}) + with ( + patch(f"{DISPLAY_MOD}.get_device_ports_from_topology", return_value=_topology_two_devices()), + patch.object(ExecuteMultipleDeviceCommands, "_initialize_nornir", return_value=nornir), + ): + result = ExecuteMultipleDeviceCommands()._run( + json.dumps( + { + "project_id": PROJECT_ID, + "device_configs": [ + {"device_name": "R1", "commands": ["show ip route"]}, + {"device_name": "R2", "commands": ["show version"]}, + {"device_name": "R1", "commands": ["show ip interface brief"]}, + ], + } + ) + ) + + # every command reached the execution layer, in order + assert nornir.run.call_args.kwargs["device_configs_map"] == { + "R1": ["show ip route", "show ip interface brief"], + "R2": ["show version"], + } + # one truthful row per device + assert [row["device_name"] for row in result] == ["R1", "R2"] + assert result[0]["status"] == "success" + assert result[0]["output"] == "merged output" + assert result[0]["diagnostic_commands"] == ["show ip route", "show ip interface brief"] + + def test_config_send_merges_same_device_entries(self): + from gns3server.agent.gns3_copilot.tools_v2.config_tools_nornir import ( + ExecuteMultipleDeviceConfigCommands, + ) + + nornir = MagicMock() + nornir.run.return_value = _aggregated_result({"R1": "merged output"}) + with ( + patch(f"{CONFIG_MOD}.get_device_ports_from_topology", return_value={"R1": {"port": 5000, "node_type": "iou"}}), + patch.object(ExecuteMultipleDeviceConfigCommands, "_initialize_nornir", return_value=nornir), + ): + result = ExecuteMultipleDeviceConfigCommands()._run( + json.dumps( + { + "project_id": PROJECT_ID, + "device_configs": [ + {"device_name": "R1", "config_commands": ["int lo0"]}, + {"device_name": "R1", "config_commands": ["ip address 1.1.1.1 255.255.255.255", "exit"]}, + ], + } + ) + ) + + assert nornir.run.call_args.kwargs["device_configs_map"] == { + "R1": ["int lo0", "ip address 1.1.1.1 255.255.255.255", "exit"], + } + assert len(result) == 1 + assert result[0]["status"] == "success" + assert result[0]["config_commands"] == ["int lo0", "ip address 1.1.1.1 255.255.255.255", "exit"] + + def test_vpcs_merges_same_device_entries(self): + from gns3server.agent.gns3_copilot.tools_v2.vpcs_tools_netmiko import VPCSCommands + + nornir = MagicMock() + host_result = MagicMock(failed=False) + host_result.result = "OK" + nornir.run.return_value = {"PC1": host_result} + with ( + patch(f"{VPCS_MOD}.get_device_ports_from_topology", return_value=_topology_ports("vpcs")), + patch.object(VPCSCommands, "_initialize_nornir", return_value=nornir), + ): + result = VPCSCommands()._run( + json.dumps( + { + "project_id": PROJECT_ID, + "device_configs": [ + {"device_name": "PC1", "commands": ["ip 10.0.0.1/24"]}, + {"device_name": "PC1", "commands": ["save"]}, + ], + } + ) + ) + + assert nornir.run.call_args.kwargs["device_configs_map"] == {"PC1": ["ip 10.0.0.1/24", "save"]} + assert len(result) == 1 + assert result[0]["status"] == "success" + assert result[0]["commands"] == ["ip 10.0.0.1/24", "save"] + + def test_blocked_commands_accumulate_across_duplicate_entries(self): + """Blocked-command info must not last-win between same-device entries.""" + from gns3server.agent.gns3_copilot.tools_v2.display_tools_nornir import ( + ExecuteMultipleDeviceCommands, + ) + + def fake_filter(commands): + blocked = {c: "matched forbidden pattern" for c in commands if c.startswith("debug")} + allowed = [c for c in commands if not c.startswith("debug")] + return allowed, blocked + + nornir = MagicMock() + nornir.run.return_value = _aggregated_result({"R1": "ok"}) + with ( + patch(f"{DISPLAY_MOD}.get_device_ports_from_topology", return_value={"R1": {"port": 5000, "node_type": "iou"}}), + patch.object(ExecuteMultipleDeviceCommands, "_initialize_nornir", return_value=nornir), + patch(f"{DISPLAY_MOD}.filter_forbidden_commands", side_effect=fake_filter), + ): + result = ExecuteMultipleDeviceCommands()._run( + json.dumps( + { + "project_id": PROJECT_ID, + "device_configs": [ + {"device_name": "R1", "commands": ["show version", "debug ip routing"]}, + {"device_name": "R1", "commands": ["debug ospf events", "show clock"]}, + ], + } + ) + ) + + assert nornir.run.call_args.kwargs["device_configs_map"] == {"R1": ["show version", "show clock"]} + assert result[0]["status"] == "partial_success" + assert set(result[0]["blocked_commands"]) == {"debug ip routing", "debug ospf events"} + + +class TestMergeDuplicateDeviceConfigs: + """Unit tests for the shared normalization helper.""" + + def test_merges_in_first_appearance_order(self): + merged = merge_duplicate_device_configs( + [ + {"device_name": "R1", "commands": ["a"]}, + {"device_name": "R2", "commands": ["b"]}, + {"device_name": "R1", "commands": ["c", "d"]}, + ], + commands_field="commands", + ) + assert merged == [ + {"device_name": "R1", "commands": ["a", "c", "d"]}, + {"device_name": "R2", "commands": ["b"]}, + ] + + def test_preserves_first_entry_fields_without_mutating_input(self): + original = [ + {"device_name": "R1", "commands": ["a"], "extra": 1}, + {"device_name": "R1", "commands": ["b"]}, + ] + merged = merge_duplicate_device_configs(original, commands_field="commands") + assert merged == [{"device_name": "R1", "commands": ["a", "b"], "extra": 1}] + assert original[0]["commands"] == ["a"] + + def test_entry_without_device_name_is_dropped(self): + merged = merge_duplicate_device_configs( + [{"commands": ["a"]}, {"device_name": "R1", "commands": ["b"]}], + commands_field="commands", + ) + assert merged == [{"device_name": "R1", "commands": ["b"]}] + + def test_missing_commands_field_defaults_to_empty(self): + merged = merge_duplicate_device_configs( + [{"device_name": "R1"}, {"device_name": "R1", "commands": ["b"]}], + commands_field="commands", + ) + assert merged == [{"device_name": "R1", "commands": ["b"]}]