Repository navigation
fix: validate_tool_arguments does not enforce Literal/const constraints on tool fields - #1547
Conversation
AngeloDanducci
left a comment
There was a problem hiding this comment.
Hi Jessica, thanks for the PR!
Overall I think this looks pretty good. Are single value constraints handled as well? I have a suggestion for an additional test to verify.
You will want to install the pre-commit hooks to use in the future (I see you fixed a ruff format problem). Additionally you need to sign off on your commits. To fix this you can do the following:
To add your Signed-off-by line to every commit in this branch:
Ensure you have a local copy of your branch by [checking out the pull request locally via command line](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally).
In your local branch, run: git rebase HEAD~2 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin jessicah/issue-1106
In the future when you commit if you use an -s flag in your git commit commands ie git commit -s -m "commit message" you won't have to do the above. If you have any issues with this let me know and we can work around it.
Signed-off-by: Jessica He <jessicahe@Mac-440.lan>
Signed-off-by: Jessica He <jessicahe@Mac-440.lan>
500d382 to
e840840
Compare
Signed-off-by: Jessica He <jessicahe@Mac-440.lan>
Signed-off-by: Jessica He <jessicahe@Mac-440.lan>
Hi Angelo, thanks for the review! I have added code and test to handle single value constraint. |
c890d40
Pull Request
Issue
Fixes #1106
Description
The issue #1106 identified two independent gaps where
Literal[...]constraints on tool fields were silently dropped.Gap A — validator ignores
conston discriminated union tag fieldsAlready fixed in previous PR. This PR adds a test (
test_strict_rejects_invalid_discriminator_value) to explicitly assert that a discriminator value matching no branch (e.g.kind: "horse"when only"cat"and"dog"are defined) is rejected understrict=True.Gap B —
enumkeyword dropped for plainLiteral[...]fieldsFixed in this PR with two changes:
convert_function_to_ollama_tool: the simple-type flattening branch was discarding theenumarray that Pydantic emits forLiteralfields. It now carriesenumforward onto the output schema so the LLM sees the allowed values._build_pydantic_type_from_schema: the simple-type fallback was mapping anytype: stringfield to plainstr, ignoringenumandconst. It now mapsenum→Literal[V1, V2, ...]andconst→Literal[V]before falling through to the bare type lookup, so Pydantic enforces the constraint during validation.Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.