-
-
Notifications
You must be signed in to change notification settings - Fork 36.4k
Improve HassLightSet intent definition #160071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves the HassLightSet intent definition to require at least one of three optional parameters (color, temperature, or brightness) using voluptuous's new anyOf constraint support. This prevents LLMs from making invalid tool calls with no arguments.
Key changes:
- Upgrades voluptuous (0.15.2 → 0.16.0) and voluptuous-openapi (0.2.0 → 0.3.0) to support
anyOfconstraints - Adds required_slots with
vol.Any("color", "temperature", "brightness")constraint to light intent handler - Enhances error messages to include voluptuous validation details for better debugging
- Adds comprehensive tests for validation success and failure cases
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| homeassistant/components/light/intent.py | Adds required_slots with vol.Any constraint to enforce that at least one of color, temperature, or brightness must be provided |
| homeassistant/helpers/intent.py | Adds logic to skip vol.Any keys when building service data and improves error messages to include validation details |
| tests/components/light/test_intent.py | Adds comprehensive tests for the new validation: one test verifying rejection when no required slots are provided, another verifying success when at least one is provided |
| requirements.txt | Updates voluptuous to 0.16.0 and voluptuous-openapi to 0.3.0 to support anyOf constraints |
| pyproject.toml | Updates dependency versions for voluptuous and voluptuous-openapi |
| homeassistant/package_constraints.txt | Updates package constraints to match new dependency versions |
epenet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update dependencies in a preliminary PR
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
This is a pre-requisite for a fix in home-assistant#160071
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR needs to be rebased and CI needs to pass.
…intent handling This has been tested with the current state of the forked libraries, so once those are merged this could be updated.
c675264 to
a43cd62
Compare
|
I think this is ready now. The failure in one specific version of python seems unrelated. |
Note: This is bringing back from the death #151583 which was closed because the necessary changes to voluptuous were taking too long to get merged. Those are merged now and voluptuous supports
anyOfconstraints and thus this PR is finally possible.Breaking change
Unsure, but I'd say no. This PR changes the OpenAPI definition of the HassLightSet intent to be more exhaustive, but that's not used directly by users.
Proposed change
While technically color, temperature and brightness are optional, al least one of those three must be provided, otherwise nothing is being set. I noticed this because my LLM was making tool calls HassLightSet without any argument, which is obviously wrong.
This should make the OpenAPI json schema generated by voluptuous_openapi express that at least one is required.
It does so by using required(anyOf(...)), which voluptuous_openapi already supports.
Hopefully this helps LLMs make less mistakes calling tools.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: