fix: add forbidden-pattern check to ToolUpdate.validate_description (…#3785
Conversation
…BM#3773) Signed-off-by: Oriol Morros Vilaseca <OM368@student.aru.ac.uk>
The original PR added the forbidden-pattern check to ToolUpdate but hardcoded it to always raise, ignoring settings.validation_strict. This created an asymmetry with ToolCreate (which respects the setting), meaning tools created with VALIDATION_STRICT=false could not be updated. Align ToolUpdate with ToolCreate: check settings.validation_strict and log a warning instead of raising when the setting is false. Expand tests to mirror TestToolCreateDescriptionValidationStrict, covering strict-mode rejection, non-strict-mode warnings, single-warning logging, safe/None passthrough in both modes, and pattern parity. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Add tests for empty string, forbidden patterns at string boundaries (start/end), and exact MAX_DESCRIPTION_LENGTH descriptions to verify boundary behavior matches ToolCreate. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
fb407d0 to
073bb50
Compare
|
Thanks for the contribution @omorros! The forbidden-pattern check for I rebased onto main and made two additional commits:
All 75 tests in the file pass. |
crivetimihai
left a comment
There was a problem hiding this comment.
LGTM — forbidden-pattern check correctly mirrors ToolCreate with VALIDATION_STRICT support. Tests comprehensive.
🔗 Related Issue
Closes #3773
📝 Summary
ToolUpdate.validate_description was missing the forbidden-pattern check (&&, ;, ||, $(, |, > , < ) that ToolCreate.validate_description enforces. This allowed a tool registered with a safe description to later have its description updated to include shell metacharacters, bypassing the validation enforced at creation time. This PR adds the same check to ToolUpdate.validate_description.
🏷️ Type of Change
[ x] Bug fix
[ ] Feature / Enhancement
[ ] Documentation
[ ] Refactor
[ ] Chore (deps, CI, tooling)
[ ] Other (describe below)
🧪 Verification
make lintmake testmake coverage✅ Checklist
[x] Code formatted (make black isort pre-commit)
[x] Tests added/updated for changes
[ ] Documentation updated (if applicable)
[x] No secrets or credentials committed
📓 Notes (optional)
Added 4 unit tests in test_schemas_validators_extra.py:
Parametrized test rejecting all 7 forbidden patterns on ToolUpdate, safe description acceptance test, None passthrough test, and parity test verifying
ToolCreate and ToolUpdate reject the same pattern set.