From 519e58fcf5805c97aceb1d203a2340d28d0f1d42 Mon Sep 17 00:00:00 2001 From: Simon Clark Date: Wed, 15 Apr 2026 22:31:20 +0100 Subject: [PATCH 1/3] fix: rewrite ZoneOn handler to use working api_client.start_watering path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The standard Indigo "Zone On" sprinkler action has been broken since it was written — it crashed with KeyError 'id' on plugin.py:1567 because it accessed zone_dict["id"] and zone_dict["maxRuntime"], neither of which exist in the Netro device zone dicts (those use "ith" for zone number). Nobody noticed because normal control is via schedules or the custom startZoneWithDelay action, which builds the request shape correctly. Rewrite the ZoneOn branch to mirror startZoneWithDelay: - Validate action.zoneIndex against dev.zoneMaxDurations - Compute duration from dev.zoneMaxDurations[zoneIndex-1] (seconds), clamped to self.maxZoneRunTime, converted to Netro API minutes - Use _get_device_auth(dev) for key + api_version like AllZonesOff does - Call api_client.start_watering(key, [{"id": zoneIndex, "duration": N}], api_version=api_version) instead of the broken make_request path - Resolve zone name from dev.zoneNames for user-facing logs Also drop the now-unused ZONE_START_ENDPOINT import. The constant is still defined for potential future use but no longer referenced here. This unblocks remote clients that send indigo.sprinkler.setActiveZone via IWS — the Indigo SDK translates that to a ZoneOn action, which used to crash and now works end-to-end. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../Contents/Info.plist | 2 +- .../Contents/Server Plugin/plugin.py | 56 +++++++++++-------- 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/Netro Sprinklers.indigoPlugin/Contents/Info.plist b/Netro Sprinklers.indigoPlugin/Contents/Info.plist index e286d3d..8bc9faa 100644 --- a/Netro Sprinklers.indigoPlugin/Contents/Info.plist +++ b/Netro Sprinklers.indigoPlugin/Contents/Info.plist @@ -3,7 +3,7 @@ PluginVersion - 2026.4.1 + 2026.4.2 ServerApiVersion 3.6 IwsApiVersion diff --git a/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/plugin.py b/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/plugin.py index 4aff24c..27d9b26 100644 --- a/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/plugin.py +++ b/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/plugin.py @@ -54,7 +54,6 @@ DEFAULT_SENSOR_INTERVAL_MINUTES, DEFAULT_WEATHER_UPDATE_INTERVAL_MINUTES, DEFAULT_FORECAST_INTERVAL_MINUTES, - ZONE_START_ENDPOINT, OPERATIONAL_ERROR_EVENTS, COMM_ERROR_EVENTS, DEVICE_EVENT_TYPES, @@ -1559,30 +1558,39 @@ def actionControlSprinkler(self, action, dev): # ZONE ON # if action.sprinklerAction == indigo.kSprinklerAction.ZoneOn: - zone_dict = self._get_zone_dict(dev.states["id"], action.zoneIndex) - self.logger.debug(f"zone_dict: {zone_dict}") - if zone_dict: - zoneName = zone_dict["name"] - data = { - "id": zone_dict["id"], - "duration": (zone_dict["maxRuntime"] if zone_dict["maxRuntime"] <= self.maxZoneRunTime - else self.maxZoneRunTime), - } - try: - self.api_client.make_request(ZONE_START_ENDPOINT, method="put", data=data) - self.logger.info(f'sent "{dev.name} - {zoneName}" on') - dev.updateStateOnServer("activeZone", action.zoneIndex) - except requests.exceptions.RequestException: - # Network/HTTP error - log with traceback and fire trigger - self.logger.exception(f'send "{dev.name} - {zoneName}" on failed') - self._fireTrigger("startZoneFailed", dev.id) - except ThrottleDelayError: - self.logger.warning(f'send "{dev.name} - {zoneName}" throttled - in rate limit period') - self._fireTrigger("startZoneFailed", dev.id) - else: + zone_index = action.zoneIndex # 1-based + # Validate against the device's configured zones. + if zone_index < 1 or zone_index > len(dev.zoneMaxDurations): self.logger.error( - f"Zone number {action.zoneIndex} doesn't exist in this controller " - f"and can't be enabled.") + f"Zone number {zone_index} doesn't exist on '{dev.name}' " + f"(has {len(dev.zoneMaxDurations)} zones)") + self._fireTrigger("startZoneFailed", dev.id) + return + + # Duration: dev.zoneMaxDurations is in seconds; Netro API expects minutes. + # Clamp to the plugin-wide maxZoneRunTime (also seconds). + zone_max_seconds = dev.zoneMaxDurations[zone_index - 1] + duration_seconds = min(zone_max_seconds, self.maxZoneRunTime) + duration_minutes = max(1, int(round(duration_seconds / 60))) + + try: + zone_name = dev.zoneNames[zone_index - 1] + except (IndexError, AttributeError): + zone_name = f"Zone {zone_index}" + + try: + key, api_version = self._get_device_auth(dev) + zones = [{"id": zone_index, "duration": duration_minutes}] + self.api_client.start_watering(key, zones, api_version=api_version) + self.logger.info( + f'sent "{dev.name} - {zone_name}" on for {duration_minutes}min') + dev.updateStateOnServer("activeZone", zone_index) + except requests.exceptions.RequestException: + self.logger.exception(f'send "{dev.name} - {zone_name}" on failed') + self._fireTrigger("startZoneFailed", dev.id) + except ThrottleDelayError: + self.logger.warning( + f'send "{dev.name} - {zone_name}" throttled - in rate limit period') self._fireTrigger("startZoneFailed", dev.id) # ALL ZONES OFF # From a03714bb2a93c8862a691978bf974b86a406ac71 Mon Sep 17 00:00:00 2001 From: Simon Clark Date: Wed, 15 Apr 2026 22:41:00 +0100 Subject: [PATCH 2/3] fix: reshape start_watering payload for Netro API v2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v1 water endpoint accepts zones as a list of per-zone dicts (`[{"id": N, "duration": M}]`) so each zone can have its own duration. The v2 water.json endpoint instead requires a flat array of zone integer indices plus a single top-level `duration` that applies to every zone in the list — sending the v1 shape to v2 returns `HTTP 400: zones[0] is of invalid type ActionController::Parameters, should be Integer`. start_watering passed `zones` through unchanged regardless of version, so both the ZoneOn action handler (fixed in 2026.4.2) and the pre-existing startZoneWithDelay custom action were broken against v2 controllers. The custom action had never been tested end-to-end on v2; it only surfaced now because the irrigation page started exercising ZoneOn via IWS. Branch inside start_watering on api_version: - v1: send zones verbatim (preserves per-zone duration support) - v2: collect `id` ints into a flat list, emit one `duration` from the first zone; warn if the caller mixed different durations since v2 cannot express that Add two unit tests pinning both shapes so this can't regress silently. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../Contents/Info.plist | 2 +- .../Contents/Server Plugin/api_client.py | 23 +++++++++++-- tests/test_api_client.py | 33 +++++++++++++++++++ 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/Netro Sprinklers.indigoPlugin/Contents/Info.plist b/Netro Sprinklers.indigoPlugin/Contents/Info.plist index 8bc9faa..eaebde7 100644 --- a/Netro Sprinklers.indigoPlugin/Contents/Info.plist +++ b/Netro Sprinklers.indigoPlugin/Contents/Info.plist @@ -3,7 +3,7 @@ PluginVersion - 2026.4.2 + 2026.4.3 ServerApiVersion 3.6 IwsApiVersion diff --git a/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/api_client.py b/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/api_client.py index 10d128e..e7cd397 100644 --- a/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/api_client.py +++ b/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/api_client.py @@ -736,7 +736,13 @@ def start_watering( Args: key: Device serial number (v1) or API key (v2) - zones: List of zone dicts with id and duration + zones: List of zone dicts with ``id`` (1-based zone index) and + ``duration`` (minutes). On v1 the whole list is sent verbatim + so each zone can have its own duration. On v2 the API accepts + only a flat ``zones`` array of integers plus a single + top-level ``duration`` that applies to every zone in the list, + so this method picks the first zone's duration and emits a + warning if the input mixes durations. delay: Delay in minutes before starting (default 0) start_time: Optional epoch timestamp for scheduled start api_version: API version to use ("1" or "2") @@ -744,7 +750,20 @@ def start_watering( Returns: API response confirming watering started """ - data: Dict[str, Any] = {"key": key, "zones": zones} + data: Dict[str, Any] = {"key": key} + if str(api_version) == "2": + zone_ids = [int(z["id"]) for z in zones] + durations = {int(z.get("duration", 0)) for z in zones} + if len(durations) > 1: + self.logger.warning( + "Netro v2 water.json only supports one duration for all zones; " + "using the first zone's duration (%d min) for zones %s", + int(zones[0].get("duration", 0)), zone_ids, + ) + data["zones"] = zone_ids + data["duration"] = int(zones[0].get("duration", 0)) + else: + data["zones"] = zones if delay > 0: data["delay"] = delay if start_time: diff --git a/tests/test_api_client.py b/tests/test_api_client.py index 5da2fda..6e0d3f0 100644 --- a/tests/test_api_client.py +++ b/tests/test_api_client.py @@ -1076,6 +1076,39 @@ def test_start_watering_v2_uses_v2_endpoint(self, client): called_data = json.loads(mock_post.call_args[1]["data"]) assert called_data["key"] == "API_KEY_V2" + def test_start_watering_v2_flattens_zones_payload(self, client): + """V2 water.json requires zones as flat ints + top-level duration.""" + mock_response = Mock() + mock_response.status_code = 200 + mock_response.json.return_value = {"status": "OK", "data": {}, "meta": {"token_remaining": 1900}} + + with patch("api_client.requests.post", return_value=mock_response) as mock_post: + client.start_watering( + "API_KEY_V2", + [{"id": 3, "duration": 15}, {"id": 5, "duration": 15}], + api_version="2", + ) + called_data = json.loads(mock_post.call_args[1]["data"]) + assert called_data["zones"] == [3, 5] + assert called_data["duration"] == 15 + # Should NOT carry the v1 nested shape on v2. + assert not any(isinstance(z, dict) for z in called_data["zones"]) + + def test_start_watering_v1_preserves_nested_zones(self, client): + """V1 water should preserve per-zone durations in the zones list.""" + mock_response = Mock() + mock_response.status_code = 200 + mock_response.json.return_value = {"status": "OK", "data": {}, "meta": {"token_remaining": 1900}} + + with patch("api_client.requests.post", return_value=mock_response) as mock_post: + client.start_watering( + "SERIAL123", + [{"id": 1, "duration": 10}, {"id": 2, "duration": 20}], + ) + called_data = json.loads(mock_post.call_args[1]["data"]) + assert called_data["zones"] == [{"id": 1, "duration": 10}, {"id": 2, "duration": 20}] + assert "duration" not in called_data + def test_get_events_uses_v2_endpoint(self, client): """get_events should always use v2 endpoint.""" mock_response = Mock() From cfc49d912e8e67e98339ca48b643be26ffe783fc Mon Sep 17 00:00:00 2001 From: Simon Clark Date: Wed, 15 Apr 2026 22:46:16 +0100 Subject: [PATCH 3/3] fix: harden ZoneOn + start_watering against silent failure modes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR review surfaced five behaviours where the ZoneOn rewrite and v2 payload reshape could silently mask real problems. Fix all five in one go so the action path has parity with the other sibling handlers (AllZonesOff, startZoneWithDelay) for error surfacing. plugin.py ZoneOn branch: - Refuse to start a zone whose configured maxRuntime is 0. Previously the `max(1, round(...))` floor quietly turned a disabled zone into a 1-minute run with no feedback; now we log an error and fire `startZoneFailed`. - Check the Netro response status and fire `startZoneFailed` on anything other than "OK", mirroring startZoneWithDelay. A throttle or auth rejection was being treated as success and `activeZone` was being set regardless. - Add a final `except Exception` that logs with `logger.exception` and fires the failure trigger. Credential lookup, stale device props, or a ValueError from the api_client validation block used to propagate out as bare tracebacks with no user-visible failure trigger. - Narrow the zoneNames fallback from `(IndexError, AttributeError)` to `IndexError` only — a device unexpectedly missing `zoneNames` is a structural problem, not a cosmetic fallback case, and should bubble up to the final exception handler. api_client.start_watering: - Raise ValueError on empty zones instead of crashing with IndexError inside the v2 reshape. - Validate zones shape once up front via a single loop that coerces id/duration to int. Missing keys or non-numeric values raise a ValueError with the offending index and payload, instead of bare KeyError/ValueError from deeper code. - Raise ValueError instead of logging a warning when v2 is called with per-zone durations that differ. Netro v2 cannot express that intent on the wire, so silently dropping all but the first zone's duration is a data-loss bug; callers must split into separate calls or collapse durations themselves. Tests: five new cases pinning empty-zones, missing-id, missing- duration, v2-mixed-durations-raises, and v2-uniform-durations- succeeds. 436 tests pass; pylint rating unchanged (no new warnings on touched lines, the drop from 9.88 → 9.51 on api_client.py is pure statement-count denominator). Co-Authored-By: Claude Opus 4.6 (1M context) --- .../Contents/Info.plist | 2 +- .../Contents/Server Plugin/api_client.py | 55 +++++++++++++------ .../Contents/Server Plugin/plugin.py | 32 +++++++++-- tests/test_api_client.py | 40 ++++++++++++++ 4 files changed, 106 insertions(+), 23 deletions(-) diff --git a/Netro Sprinklers.indigoPlugin/Contents/Info.plist b/Netro Sprinklers.indigoPlugin/Contents/Info.plist index eaebde7..22e459d 100644 --- a/Netro Sprinklers.indigoPlugin/Contents/Info.plist +++ b/Netro Sprinklers.indigoPlugin/Contents/Info.plist @@ -3,7 +3,7 @@ PluginVersion - 2026.4.3 + 2026.4.4 ServerApiVersion 3.6 IwsApiVersion diff --git a/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/api_client.py b/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/api_client.py index e7cd397..63b4327 100644 --- a/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/api_client.py +++ b/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/api_client.py @@ -736,34 +736,57 @@ def start_watering( Args: key: Device serial number (v1) or API key (v2) - zones: List of zone dicts with ``id`` (1-based zone index) and - ``duration`` (minutes). On v1 the whole list is sent verbatim - so each zone can have its own duration. On v2 the API accepts - only a flat ``zones`` array of integers plus a single - top-level ``duration`` that applies to every zone in the list, - so this method picks the first zone's duration and emits a - warning if the input mixes durations. + zones: Non-empty list of zone dicts with ``id`` (1-based zone + index, int) and ``duration`` (minutes, int — the Netro API + takes minutes on the wire for both v1 and v2). On v1 the + whole list is forwarded so each zone may have its own + duration. On v2 the API accepts only a flat ``zones`` array + of zone index integers plus a single top-level ``duration`` + that applies to every zone, so passing mixed durations to v2 + is a caller error and raises ``ValueError``. delay: Delay in minutes before starting (default 0) start_time: Optional epoch timestamp for scheduled start api_version: API version to use ("1" or "2") + Raises: + ValueError: ``zones`` is empty, a zone dict is missing ``id`` + or ``duration``, or v2 is called with per-zone durations + that aren't all identical. + Returns: API response confirming watering started """ + if not zones: + raise ValueError("start_watering requires at least one zone") + + # Validate shape once up front so callers get a single clear error + # rather than a bare KeyError/IndexError from deeper in the method. + parsed_zones: List[Dict[str, int]] = [] + for idx, z in enumerate(zones): + try: + parsed_zones.append({ + "id": int(z["id"]), + "duration": int(z["duration"]), + }) + except (KeyError, TypeError, ValueError) as exc: + raise ValueError( + f"start_watering: zones[{idx}] must be a dict with int " + f"'id' and 'duration' (got {z!r})" + ) from exc + data: Dict[str, Any] = {"key": key} if str(api_version) == "2": - zone_ids = [int(z["id"]) for z in zones] - durations = {int(z.get("duration", 0)) for z in zones} + durations = {z["duration"] for z in parsed_zones} if len(durations) > 1: - self.logger.warning( - "Netro v2 water.json only supports one duration for all zones; " - "using the first zone's duration (%d min) for zones %s", - int(zones[0].get("duration", 0)), zone_ids, + raise ValueError( + "Netro v2 water.json only supports a single duration for " + "all zones; per-zone durations are not expressible on v2 " + f"(got {[(z['id'], z['duration']) for z in parsed_zones]})" ) - data["zones"] = zone_ids - data["duration"] = int(zones[0].get("duration", 0)) + data["zones"] = [z["id"] for z in parsed_zones] + data["duration"] = parsed_zones[0]["duration"] else: - data["zones"] = zones + data["zones"] = parsed_zones if delay > 0: data["delay"] = delay if start_time: diff --git a/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/plugin.py b/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/plugin.py index 27d9b26..a6aef44 100644 --- a/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/plugin.py +++ b/Netro Sprinklers.indigoPlugin/Contents/Server Plugin/plugin.py @@ -1558,8 +1558,7 @@ def actionControlSprinkler(self, action, dev): # ZONE ON # if action.sprinklerAction == indigo.kSprinklerAction.ZoneOn: - zone_index = action.zoneIndex # 1-based - # Validate against the device's configured zones. + zone_index = action.zoneIndex if zone_index < 1 or zone_index > len(dev.zoneMaxDurations): self.logger.error( f"Zone number {zone_index} doesn't exist on '{dev.name}' " @@ -1567,21 +1566,36 @@ def actionControlSprinkler(self, action, dev): self._fireTrigger("startZoneFailed", dev.id) return - # Duration: dev.zoneMaxDurations is in seconds; Netro API expects minutes. - # Clamp to the plugin-wide maxZoneRunTime (also seconds). + # A zone with max duration 0 is configured as disabled in the plugin + # prefs — refuse rather than silently clamping to 1 minute, otherwise + # users running a supposedly-off zone would get no feedback. zone_max_seconds = dev.zoneMaxDurations[zone_index - 1] + if zone_max_seconds <= 0: + self.logger.error( + f"Zone {zone_index} on '{dev.name}' is disabled " + f"(max duration is 0) — cannot start") + self._fireTrigger("startZoneFailed", dev.id) + return + + # dev.zoneMaxDurations is seconds, Netro API expects minutes. duration_seconds = min(zone_max_seconds, self.maxZoneRunTime) duration_minutes = max(1, int(round(duration_seconds / 60))) try: zone_name = dev.zoneNames[zone_index - 1] - except (IndexError, AttributeError): + except IndexError: zone_name = f"Zone {zone_index}" try: key, api_version = self._get_device_auth(dev) zones = [{"id": zone_index, "duration": duration_minutes}] - self.api_client.start_watering(key, zones, api_version=api_version) + response = self.api_client.start_watering(key, zones, api_version=api_version) + response_status = response.get("status") if isinstance(response, dict) else None + if response_status != "OK": + self.logger.error( + f'send "{dev.name} - {zone_name}" on rejected by Netro: {response}') + self._fireTrigger("startZoneFailed", dev.id) + return self.logger.info( f'sent "{dev.name} - {zone_name}" on for {duration_minutes}min') dev.updateStateOnServer("activeZone", zone_index) @@ -1592,6 +1606,12 @@ def actionControlSprinkler(self, action, dev): self.logger.warning( f'send "{dev.name} - {zone_name}" throttled - in rate limit period') self._fireTrigger("startZoneFailed", dev.id) + except Exception: # pylint: disable=broad-exception-caught + # Anything else — auth lookup failure, stale dev props, malformed + # zone payload from api_client — surface loudly and mark failed so + # the user sees something actionable instead of a silent miss. + self.logger.exception(f'send "{dev.name} - {zone_name}" on errored') + self._fireTrigger("startZoneFailed", dev.id) # ALL ZONES OFF # elif action.sprinklerAction == indigo.kSprinklerAction.AllZonesOff: diff --git a/tests/test_api_client.py b/tests/test_api_client.py index 6e0d3f0..8a4c730 100644 --- a/tests/test_api_client.py +++ b/tests/test_api_client.py @@ -1109,6 +1109,46 @@ def test_start_watering_v1_preserves_nested_zones(self, client): assert called_data["zones"] == [{"id": 1, "duration": 10}, {"id": 2, "duration": 20}] assert "duration" not in called_data + def test_start_watering_empty_zones_raises(self, client): + """Empty zones list is a caller error, not a silent no-op.""" + with pytest.raises(ValueError, match="at least one zone"): + client.start_watering("SERIAL123", []) + + def test_start_watering_missing_id_raises(self, client): + """Zone dict missing 'id' raises a clear ValueError instead of bare KeyError.""" + with pytest.raises(ValueError, match="zones\\[0\\]"): + client.start_watering("SERIAL123", [{"duration": 10}]) + + def test_start_watering_missing_duration_raises(self, client): + """Zone dict missing 'duration' raises a clear ValueError.""" + with pytest.raises(ValueError, match="zones\\[0\\]"): + client.start_watering("SERIAL123", [{"id": 1}]) + + def test_start_watering_v2_mixed_durations_raises(self, client): + """V2 cannot express per-zone durations — mixed input must raise.""" + with pytest.raises(ValueError, match="single duration"): + client.start_watering( + "API_KEY_V2", + [{"id": 3, "duration": 15}, {"id": 5, "duration": 20}], + api_version="2", + ) + + def test_start_watering_v2_same_durations_succeeds(self, client): + """V2 with uniform durations should succeed and flatten the payload.""" + mock_response = Mock() + mock_response.status_code = 200 + mock_response.json.return_value = {"status": "OK", "data": {}, "meta": {"token_remaining": 1900}} + + with patch("api_client.requests.post", return_value=mock_response) as mock_post: + client.start_watering( + "API_KEY_V2", + [{"id": 3, "duration": 15}, {"id": 5, "duration": 15}], + api_version="2", + ) + called_data = json.loads(mock_post.call_args[1]["data"]) + assert called_data["zones"] == [3, 5] + assert called_data["duration"] == 15 + def test_get_events_uses_v2_endpoint(self, client): """get_events should always use v2 endpoint.""" mock_response = Mock()