You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This commit was created on GitHub.com and signed with GitHub’s verified signature.
Added
RouterOS CHR integration test harness.test/integration/ runs tool handlers against a real RouterOS instance — a Cloud Hosted Router booted in Docker via docker-compose.test.yml (QEMU, KVM-accelerated where available). The suite provisions a fresh CHR automatically (readiness poll + admin password setup) and covers REST response parsing, the full idempotency lifecycle (dry-run → create → already_exists → conflict → remove → not-found) for IP addresses, routes, firewall rules, DNS entries, VLANs, and VRRP instances, and the complete change-safety cycle (plan_changes → apply_plan → snapshot → rollback_change) against live state. A smoke sweep additionally runs every parameter-less RouterOS read tool (~50) against the live router, so new list/get tools are covered automatically; SSH-backed tools (run_command, export_config, ping) are exercised over their real transport, and one suite drives the full tool executor (retry, circuit breaker, snapshot + journal wiring). An opt-in second CHR (docker compose --profile pair + MIKROMCP_ITEST_PAIR=1; always on in CI) covers fleet operations — list_routers and bulk_execute fan-out across two genuinely distinct live routers. Runs via npm run test:integration; a new Integration GitHub Actions workflow runs it on demand (workflow_dispatch or the integration PR label). This closes the harness the v1.0 roadmap had deferred.
Fixed
rollback_change could not restore a set-menu singleton, and made things worse trying: ip/dns, system/ntp/client, container/config, and the pre-7.16 OVPN server have no .id and no semantic key, so the restore planner fell through to signature matching and emitted a DELETE <path>/undefined followed by a PUT that RouterOS rejects. Singletons are now detected structurally (at most one record per side, no .id) and restored as a whole-record POST <path>/set. The read-only cache-used and dynamic-servers fields join the runtime-field filter, since a whole-record write fails outright if it carries one. Reachable only since the singleton write fix above — before that, those tools could not write at all, so there was never anything to roll back.
manage_route falsely reported a CONFLICT when re-adding an identical route: the idempotency check compared the record's distance (parsed to a number by the response parser) against a string with strict equality. Found by the new integration harness on its first run.
manage_ip_address with action=update always reported updated (and re-sent disabled) even when nothing changed: the change detection compared the record's parsed boolean disabled against the string "false". Also found by the integration harness.
manage_vrrp_instance could never report already_exists: the idempotency check compared the record's vrid (parsed to a number) against a string, so every identical re-add threw a false CONFLICT. Same bug class as the two above, found by reviewing for the pattern.
manage_vlan had the same false CONFLICT on an identical re-add (vlan-id parsed to a number, compared against a string). Confirmed and covered live by the integration harness.
manage_ovpn_server with action=disable was a silent no-op on an enabled server: the state check compared the parsed boolean enabled field against the string "yes", so the server always looked disabled. Enable/disable now use isTrue(); set no longer re-sends an unchanged port.
manage_ovpn_client updates always re-sent (and reported a change for) an unchanged port — parsed number compared against a string.
manage_certificate trust/untrust never recognised the current trust state (trusted compared against "yes"/"no" literals), so the idempotent short-circuit never fired.
manage_ntp_client always reported (and re-applied) an enabled change even when the value already matched — parsed boolean compared against a stringified boolean.
manage_dns_settings always reported (and re-applied) changes for unchanged allow-remote-requests, max-udp-packet-size, and cache-size — same comparison class, so every no-op call issued a phantom write. Found by a second audit pass; the non-boolean comparisons now go through a shared sameValue() helper next to isTrue() in the response parser so the rule lives in one place.
manage_ipsec_policy never matched an existing policy: its composite key compared the parsed boolean tunnel against a string, so identical re-adds created duplicate IPSec policies and remove/enable/disable threw NOT_FOUND for policies that plainly exist.
manage_container_config always reported (and re-applied) an unchanged ram-high — parsed number compared against a string.
get_ovpn_server reported enabled=undefined on RouterOS 7.16+ (which sends disabled instead of enabled); it now derives the state through the same dialect detection as manage_ovpn_server.
list_connections was completely broken: every call failed with HTTP 400. Queries carrying a .proplist were POSTed to the bare collection path, which RouterOS rejects — the REST client now targets the /print command endpoint. Found by the new read-tool smoke sweep on its first run; list_connections was the only tool using .proplist.
A tool call naming an unknown routerId returned a generic INTERNAL_ERROR ("An unexpected internal error occurred") because the router registry threw a plain Error. It now returns a typed NOT_FOUND (ROUTER_NOT_FOUND) listing the available routers and pointing at list_routers.
manage_ovpn_server enable/disable did not work on RouterOS 7.16+: that release replaced the singleton's enabled flag with per-instance disabled, so the tool read a field the device no longer sends and wrote a parameter the API rejects (unknown parameter enabled). The tool now detects which dialect the device speaks and reads/writes the matching field. Found while provisioning the OVPN server for the integration suite.
manage_dns_settings, manage_ntp_client, and manage_container_config could never actually write: RouterOS set-menu singletons return no .id, so every non-dry-run change was PATCHed to .../undefined and failed with HTTP 500 (verified live against the CHR). Singleton writes now go through the POST <path>/set command endpoint. The pre-7.16 manage_ovpn_server write path had the same flaw and uses the same fix when the record carries no .id.
manage_ovpn_server and get_ovpn_server acted on an arbitrary instance when a RouterOS 7.16+ router carried more than one OVPN server instance; they now throw a typed CONFLICT (OVPN_SERVER_AMBIGUOUS) naming the instances instead of silently picking the first.
manage_wifi_interface always reported (and re-applied) a disabled change even when the value already matched — a parsed boolean compared against a string, the same bug class as the sweep above; no_change was unreachable. The ssid comparison had the numeric variant of the same bug (a purely numeric SSID parses to a number) and now goes through sameValue().
manage_pppoe_client updates always re-sent (and reported a change for) unchanged add-default-route and dial-on-demand flags — parsed booleans compared against "yes"/"no" literals.
manage_vlan, manage_ovpn_client, and manage_pppoe_client could not find a resource with a purely numeric name (the parser turns a name like "100" into a number), so remove/enable/disable reported NOT_FOUND and identical re-adds mis-errored; identity matching now goes through sameValue().