Conversation
Adds a Philips Hue workflow that controls lights directly via the Hue Bridge REST API, with support for mood-based scenes (romantic, party, bedtime, relax, focus, movie, morning, energize). Also fixes the LLM system prompt examples that were causing the assistant to prefix every response with its name. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@copilot are there any secrets in my pull request? |
|
@premsantosh I've opened a new pull request, #5, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
This PR adds native Philips Hue Bridge integration using the CLIP v2 API, removing template/example light workflows in favor of real implementations. The integration supports direct bridge control, mood-based lighting scenes, and intelligent room/light discovery.
Changes:
- Added PhilipsHueLightsWorkflow with support for 8 mood presets (romantic, relax, energize, party, bedtime, focus, movie, morning) and standard light controls
- Removed template LightsWorkflow and HomeAssistantLightsWorkflow to avoid conflicts; removed MediaWorkflow registration to prevent keyword overlap with Hue mood "movie"
- Updated workflow matching to prefer last-registered workflow, allowing real integrations to take priority over templates
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| workflows/philips_hue.py | New PhilipsHueClient and PhilipsHueLightsWorkflow implementing full CLIP v2 API integration with SSL context for self-signed certs, discovery mechanism for lights/rooms, and mood presets |
| workflows/home_assistant.py | Removed HomeAssistantLightsWorkflow (121 lines) to eliminate duplicate light control functionality |
| workflows/base.py | Removed LightsWorkflow template class; updated find_matching_workflow to return last match instead of first for proper prioritization; removed MediaWorkflow from default registration |
| workflows/init.py | Updated exports to remove LightsWorkflow, MediaWorkflow, and HomeAssistantLightsWorkflow; added PhilipsHue exports |
| main.py | Added Philips Hue workflow registration with configuration check; removed "lights" workflow unregistration logic; updated environment variable documentation |
| core/assistant.py | Enhanced entity extraction with mood keywords and additional action keywords (brighten/darken, increase/decrease) to support Hue mood scenes |
| llm/providers.py | Removed assistant name from example responses in personality prompt for cleaner output |
| .env.example | Added Philips Hue configuration section with bridge IP and application key setup instructions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif action == "dim" and brightness is not None: | ||
| return f"I've dimmed the {room} lights to {brightness}%, sir." |
There was a problem hiding this comment.
When the action is "dim" but brightness is None (line 405 condition fails), the function falls through to return the generic "I've toggled the {room} lights, sir." message (line 407), which doesn't accurately describe what happened. Consider adding an explicit case for dim without brightness, like "I've dimmed the {room} lights, sir." to match the actual behavior of setting lights to 30%.
| elif action == "dim" and brightness is not None: | |
| return f"I've dimmed the {room} lights to {brightness}%, sir." | |
| elif action == "dim": | |
| if brightness is not None: | |
| return f"I've dimmed the {room} lights to {brightness}%, sir." | |
| return f"I've dimmed the {room} lights, sir." |
| # Lazy discovery on first call | ||
| if not self.client._discovered: | ||
| try: | ||
| await self.client.discover() |
There was a problem hiding this comment.
Accessing the internal _discovered attribute directly breaks encapsulation. Consider adding a public property or method like is_discovered() to check discovery status, or make the discovery check part of the client's internal logic.
| # Lazy discovery on first call | |
| if not self.client._discovered: | |
| try: | |
| await self.client.discover() | |
| # Lazy discovery on first call without accessing client's private attributes | |
| is_discovered_attr = getattr(self.client, "is_discovered", None) | |
| if callable(is_discovered_attr): | |
| discovered = is_discovered_attr() | |
| else: | |
| # Fall back to workflow-local discovery flag | |
| discovered = getattr(self, "_discovered", False) | |
| if not discovered: | |
| try: | |
| await self.client.discover() | |
| # Mark as discovered for future calls if we manage the flag locally | |
| if not callable(is_discovered_attr): | |
| self._discovered = True |
| elif action == "off": | ||
| state = {"on": {"on": False}} | ||
| elif action == "dim": | ||
| bri = self._pct_to_bri(brightness) if brightness is not None else 30.0 |
There was a problem hiding this comment.
When the action is "dim" without a brightness value, it defaults to 30.0. However, this doesn't actually dim the lights if they're already at a lower brightness - it would brighten them to 30%. Consider either getting the current brightness first and reducing it by a percentage, or documenting that "dim" sets lights to 30% regardless of current state.
| bri = self._pct_to_bri(brightness) if brightness is not None else 30.0 | |
| # If no brightness is specified, default to 30% and expose that | |
| # via the brightness variable so responses remain accurate. | |
| if brightness is None: | |
| bri = 30.0 | |
| brightness = 30 | |
| else: | |
| bri = self._pct_to_bri(brightness) |
| message=f"I don't have a lighting preset for '{mood}', sir.", | ||
| error=f"Unknown mood: {mood}", | ||
| ) | ||
| state = dict(state) # copy so we don't mutate the class-level dict |
There was a problem hiding this comment.
The mood state dictionary is copied with dict(state) which creates a shallow copy. Since the MOODS dictionary contains nested dictionaries (e.g., {"on": {"on": True}}), modifications to nested values could still affect the original. Consider using copy.deepcopy() if you need to prevent any mutation of the class-level MOODS dictionary, or document that the state dictionaries should not be modified after copying.
| async def discover(self): | ||
| """Fetch all lights, rooms, and grouped_lights, building name-to-ID lookup maps.""" | ||
| lights = await self.get_lights() | ||
| rooms = await self.get_rooms() | ||
| grouped_lights = await self.get_grouped_lights() | ||
|
|
||
| # Build light name -> light resource ID | ||
| self._light_names = {} | ||
| for light in lights: | ||
| name = light.get("metadata", {}).get("name", "") | ||
| if name: | ||
| self._light_names[name.lower()] = light["id"] | ||
|
|
||
| # Build a service rid -> grouped_light ID lookup | ||
| gl_by_id = {gl["id"]: gl for gl in grouped_lights} | ||
|
|
||
| # Build room name -> grouped_light ID | ||
| self._group_names = {} | ||
| for room in rooms: | ||
| room_name = room.get("metadata", {}).get("name", "") | ||
| if not room_name: | ||
| continue | ||
| # Each room has services; find the grouped_light service | ||
| for svc in room.get("services", []): | ||
| if svc.get("rtype") == "grouped_light" and svc.get("rid") in gl_by_id: | ||
| self._group_names[room_name.lower()] = svc["rid"] | ||
| break | ||
|
|
||
| # Find the bridge_home grouped_light for "all lights" control | ||
| bridge_homes = await self._get_resource("bridge_home") | ||
| for bh in bridge_homes: | ||
| for svc in bh.get("services", []): | ||
| if svc.get("rtype") == "grouped_light": | ||
| self._all_lights_group_id = svc["rid"] | ||
| break | ||
| if self._all_lights_group_id: | ||
| break | ||
|
|
||
| self._discovered = True | ||
| logger.info( | ||
| "Hue discovery complete: %d lights, %d rooms", | ||
| len(self._light_names), | ||
| len(self._group_names), | ||
| ) |
There was a problem hiding this comment.
The discover() method populates lookup maps but doesn't handle the case where the API calls fail. While the caller in execute() catches exceptions from discover(), users calling discover() directly won't get a clear indication that discovery failed. Consider adding explicit error handling or returning a boolean to indicate success/failure.
| async def discover(self): | |
| """Fetch all lights, rooms, and grouped_lights, building name-to-ID lookup maps.""" | |
| lights = await self.get_lights() | |
| rooms = await self.get_rooms() | |
| grouped_lights = await self.get_grouped_lights() | |
| # Build light name -> light resource ID | |
| self._light_names = {} | |
| for light in lights: | |
| name = light.get("metadata", {}).get("name", "") | |
| if name: | |
| self._light_names[name.lower()] = light["id"] | |
| # Build a service rid -> grouped_light ID lookup | |
| gl_by_id = {gl["id"]: gl for gl in grouped_lights} | |
| # Build room name -> grouped_light ID | |
| self._group_names = {} | |
| for room in rooms: | |
| room_name = room.get("metadata", {}).get("name", "") | |
| if not room_name: | |
| continue | |
| # Each room has services; find the grouped_light service | |
| for svc in room.get("services", []): | |
| if svc.get("rtype") == "grouped_light" and svc.get("rid") in gl_by_id: | |
| self._group_names[room_name.lower()] = svc["rid"] | |
| break | |
| # Find the bridge_home grouped_light for "all lights" control | |
| bridge_homes = await self._get_resource("bridge_home") | |
| for bh in bridge_homes: | |
| for svc in bh.get("services", []): | |
| if svc.get("rtype") == "grouped_light": | |
| self._all_lights_group_id = svc["rid"] | |
| break | |
| if self._all_lights_group_id: | |
| break | |
| self._discovered = True | |
| logger.info( | |
| "Hue discovery complete: %d lights, %d rooms", | |
| len(self._light_names), | |
| len(self._group_names), | |
| ) | |
| async def discover(self) -> bool: | |
| """Fetch all lights, rooms, and grouped_lights, building name-to-ID lookup maps. | |
| Returns: | |
| bool: True if discovery completed successfully, False otherwise. | |
| """ | |
| try: | |
| lights = await self.get_lights() | |
| rooms = await self.get_rooms() | |
| grouped_lights = await self.get_grouped_lights() | |
| # Build light name -> light resource ID | |
| self._light_names = {} | |
| for light in lights: | |
| name = light.get("metadata", {}).get("name", "") | |
| if name: | |
| self._light_names[name.lower()] = light["id"] | |
| # Build a service rid -> grouped_light ID lookup | |
| gl_by_id = {gl["id"]: gl for gl in grouped_lights} | |
| # Build room name -> grouped_light ID | |
| self._group_names = {} | |
| for room in rooms: | |
| room_name = room.get("metadata", {}).get("name", "") | |
| if not room_name: | |
| continue | |
| # Each room has services; find the grouped_light service | |
| for svc in room.get("services", []): | |
| if svc.get("rtype") == "grouped_light" and svc.get("rid") in gl_by_id: | |
| self._group_names[room_name.lower()] = svc["rid"] | |
| break | |
| # Find the bridge_home grouped_light for "all lights" control | |
| bridge_homes = await self._get_resource("bridge_home") | |
| self._all_lights_group_id = None | |
| for bh in bridge_homes: | |
| for svc in bh.get("services", []): | |
| if svc.get("rtype") == "grouped_light": | |
| self._all_lights_group_id = svc["rid"] | |
| break | |
| if self._all_lights_group_id: | |
| break | |
| self._discovered = True | |
| logger.info( | |
| "Hue discovery complete: %d lights, %d rooms", | |
| len(self._light_names), | |
| len(self._group_names), | |
| ) | |
| return True | |
| except Exception as exc: | |
| # Reset discovery state on failure to avoid partial/invalid mappings | |
| self._light_names = {} | |
| self._group_names = {} | |
| self._all_lights_group_id = None | |
| self._discovered = False | |
| logger.error("Hue discovery failed", exc_info=exc) | |
| return False |
| elif any(word in text_lower for word in ["turn off", "switch off", "disable"]): | ||
| entities["action"] = "off" | ||
| elif any(word in text_lower for word in ["increase", "raise", "brighten", "brighter", "more bright", "turn up", "bump up"]): | ||
| entities["action"] = "on" |
There was a problem hiding this comment.
The entity extraction adds several new action keywords like "increase", "raise", "brighten" that set action to "on", but these might be more appropriate for a dedicated "brighten" action that increases brightness relative to the current level rather than just turning lights on. Consider whether these keywords should map to a different action or if the workflow should handle brightness increases differently.
| entities["action"] = "on" | |
| entities["action"] = "brighten" |
| def _pct_to_bri(pct: int) -> float: | ||
| """Convert brightness 0-100% to Hue API v2's 0.0-100.0 scale.""" | ||
| return max(0.0, min(100.0, float(pct))) |
There was a problem hiding this comment.
The brightness value is converted from percentage (int) to float, but the type hint on the parameter says pct: int. If the function accepts other numeric types in practice, consider updating the type hint to Union[int, float] for accuracy. Alternatively, if only int is expected, the cast to float is correct but could be documented in the docstring.
| self._ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) | ||
| self._ssl_context.check_hostname = False | ||
| self._ssl_context.verify_mode = ssl.CERT_NONE |
There was a problem hiding this comment.
SSL verification is disabled for the Hue Bridge connection. While this is necessary because Hue Bridges use self-signed certificates, it opens a potential security risk for man-in-the-middle attacks. Consider documenting this risk in the docstring and providing guidance for users on network security best practices (e.g., ensuring the bridge is on a trusted local network).
| warnings.append("PORCUPINE_ACCESS_KEY not set - wake word detection disabled") | ||
|
|
||
|
|
||
| if os.getenv("HUE_BRIDGE_IP") and not (os.getenv("HUE_APPLICATION_KEY") or os.getenv("HUE_USERNAME")): |
There was a problem hiding this comment.
The warning check uses or os.getenv("HUE_USERNAME") which means the warning won't be shown if HUE_USERNAME is set even though the PhilipsHueConfig.from_env() method checks for it as a fallback. This inconsistency could confuse users. The warning logic should match the config logic exactly.
| if os.getenv("HUE_BRIDGE_IP") and not (os.getenv("HUE_APPLICATION_KEY") or os.getenv("HUE_USERNAME")): | |
| if os.getenv("HUE_BRIDGE_IP") and not os.getenv("HUE_APPLICATION_KEY"): |
|
|
||
| # Philips Hue (direct bridge control via API v2 / CLIP v2) | ||
| # Press the bridge button, then create an application key via: | ||
| # curl -X POST https://<bridge-ip>/api -d '{"devicetype":"friday#assistant","generateclientkey":true}' -k |
There was a problem hiding this comment.
The example curl command uses the -k flag, which disables TLS certificate validation and encourages connecting to the Hue bridge over HTTPS without verifying the peer. An attacker on the same network could perform a man-in-the-middle attack during this step to steal the returned HUE_APPLICATION_KEY and then control the user's Hue devices. Remove the -k option and either rely on proper certificate validation or explicitly document a safer trust model (e.g., pinning or verifying the bridge certificate) when obtaining the application key.
| # curl -X POST https://<bridge-ip>/api -d '{"devicetype":"friday#assistant","generateclientkey":true}' -k | |
| # curl -X POST https://<bridge-ip>/api -d '{"devicetype":"friday#assistant","generateclientkey":true}' | |
| # Ensure your system trusts the Hue bridge's TLS certificate or manually verify its certificate/fingerprint before proceeding. |
No description provided.