fix: rewrite ZoneOn handler to use working start_watering path#50
Merged
Conversation
…path
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) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
This was referenced Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
KeyError 'id'onplugin.py:1567because it accessedzone_dict["id"]/zone_dict["maxRuntime"], neither of which exist in Netro device zone dicts (those use"ith"for zone number).startZoneWithDelayaction, which builds the request shape correctly. The bug surfaced while building a remote irrigation control page that callsindigo.sprinkler.setActiveZonevia IWS — the Indigo SDK translates that toZoneOn, which crashed the plugin.ZoneOnbranch to mirror the workingstartZoneWithDelaypath: validate zone index againstdev.zoneMaxDurations, compute duration from the device's configured max (seconds → minutes, clamped toself.maxZoneRunTime), use_get_device_auth(dev)for key + api_version likeAllZonesOffalready does, and callapi_client.start_watering(key, [{"id": zoneIndex, "duration": N}], api_version=api_version).ZONE_START_ENDPOINTimport fromplugin.py. The constant itself stays defined inconstants.pyfor potential future use.Test plan
pytest tests/→ 429 passed, no regressionspylint plugin.py→ rating unchanged at 9.41/10, no new warnings on changed linesindigo.sprinkler.setActiveZone {index:N}against a real Netro controller via REST, confirm zone starts and stops correctly🤖 Generated with Claude Code