-
Notifications
You must be signed in to change notification settings - Fork 11
fix(server): reject empty string list evaluator values #121
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: main
Are you sure you want to change the base?
Changes from all commits
b6ceccd
dcd6bcd
8e20d0e
319eede
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| """Tests for list evaluator.""" | ||
|
|
||
| import pytest | ||
| from pydantic import ValidationError | ||
|
|
||
| from agent_control_evaluators.list import ListEvaluator, ListEvaluatorConfig | ||
|
|
||
|
|
||
| class TestListEvaluatorConfig: | ||
| """Tests for list evaluator config validation.""" | ||
|
|
||
| def test_empty_string_value_rejected(self) -> None: | ||
| """Test that empty-string list entries are rejected at config validation time.""" | ||
| # Given: a list evaluator config with an empty-string value | ||
| # When: constructing the config model | ||
| with pytest.raises( | ||
| ValidationError, match="values must not contain empty or whitespace-only strings" | ||
| ): | ||
| ListEvaluatorConfig(values=[""]) | ||
| # Then: validation rejects the config (asserted by pytest) | ||
|
|
||
| def test_whitespace_only_value_rejected(self) -> None: | ||
| """Test that whitespace-only list entries are rejected at config validation time.""" | ||
| # Given: a list evaluator config with a whitespace-only value | ||
| # When: constructing the config model | ||
| with pytest.raises( | ||
| ValidationError, match="values must not contain empty or whitespace-only strings" | ||
| ): | ||
| ListEvaluatorConfig(values=[" "]) | ||
| # Then: validation rejects the config (asserted by pytest) | ||
|
|
||
|
|
||
| class TestListEvaluator: | ||
| """Tests for list evaluator runtime behavior.""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_legacy_empty_string_value_is_ignored_defensively(self) -> None: | ||
| """Test that legacy invalid configs do not compile into a match-all regex.""" | ||
| # Given: a legacy invalid config constructed without validation | ||
| config = ListEvaluatorConfig.model_construct( | ||
| values=[""], | ||
| logic="any", | ||
| match_on="match", | ||
| match_mode="contains", | ||
| case_sensitive=False, | ||
| ) | ||
| evaluator = ListEvaluator(config) | ||
|
|
||
| # When: evaluating normal text against the legacy config | ||
| result = await evaluator.evaluate("Tell me a joke") | ||
|
|
||
| # Then: the evaluator ignores the empty control values | ||
| assert result.matched is False | ||
| assert result.message == "Empty control values - control ignored" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This tests the denylist path ( Something like: async def test_legacy_empty_string_allowlist_does_not_block_all(self) -> None:
config = ListEvaluatorConfig.model_construct(
values=[""],
logic="any",
match_on="no_match",
match_mode="exact",
case_sensitive=False,
)
evaluator = ListEvaluator(config)
result = await evaluator.evaluate("legitimate_value")
assert result.matched is False
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in I added a companion legacy allowlist regression covering |
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_legacy_whitespace_only_value_is_ignored_defensively(self) -> None: | ||
| """Test that legacy whitespace-only configs do not compile into pathological regexes.""" | ||
| # Given: a legacy invalid config with a whitespace-only value | ||
| config = ListEvaluatorConfig.model_construct( | ||
| values=[" "], | ||
| logic="any", | ||
| match_on="match", | ||
| match_mode="contains", | ||
| case_sensitive=False, | ||
| ) | ||
| evaluator = ListEvaluator(config) | ||
|
|
||
| # When: evaluating normal text against the legacy config | ||
| result = await evaluator.evaluate("Tell me a joke") | ||
|
|
||
| # Then: the evaluator ignores the empty control values | ||
| assert result.matched is False | ||
| assert result.message == "Empty control values - control ignored" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_legacy_empty_string_allowlist_does_not_block_all(self) -> None: | ||
| """Test that legacy invalid allowlist configs do not block all inputs.""" | ||
| # Given: a legacy invalid allowlist config constructed without validation | ||
| config = ListEvaluatorConfig.model_construct( | ||
| values=[""], | ||
| logic="any", | ||
| match_on="no_match", | ||
| match_mode="contains", | ||
| case_sensitive=False, | ||
| ) | ||
| evaluator = ListEvaluator(config) | ||
|
|
||
| # When: evaluating normal text against the legacy config | ||
| result = await evaluator.evaluate("legitimate_value") | ||
|
|
||
| # Then: the evaluator ignores the empty control values instead of blocking all input | ||
| assert result.matched is False | ||
| assert result.message == "Empty control values - control ignored" | ||
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 catches exact
""but allows whitespace-only strings like" "or"\t"through. Those produce similarly pathological regexes - e.g.\b(\ )\bin contains mode.Consider using
strip():Same change needed in the defensive filter in
evaluator.py:38(if value != ""->if value.strip()).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.
Addressed in
319eede.I broadened the guard from exact
""to any blank/whitespace-only string in both the config validator and the evaluator fallback. That now rejects values like" "and"\t"at validation time and defensively drops them for legacy invalid configs.