Skip to content

fix(server): reject empty string list evaluator values#121

Open
lan17 wants to merge 3 commits intomainfrom
codex/fix-list-empty-values
Open

fix(server): reject empty string list evaluator values#121
lan17 wants to merge 3 commits intomainfrom
codex/fix-list-empty-values

Conversation

@lan17
Copy link
Contributor

@lan17 lan17 commented Mar 12, 2026

Summary

  • reject empty-string list entries at list evaluator config validation time
  • defensively filter exact empty strings in the evaluator so legacy invalid configs do not compile into a match-all regex
  • add evaluator and server regression tests for both validation paths

Testing

  • make evaluators-test
  • make server-test
  • make lint
  • make typecheck

Fixes #120

@codecov
Copy link

codecov bot commented Mar 12, 2026

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...uiltin/src/agent_control_evaluators/list/config.py 85.71% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@lan17 lan17 changed the title fix: reject empty string list evaluator values fix(server): reject empty string list evaluator values Mar 12, 2026
@@ -24,3 +24,11 @@ class ListEvaluatorConfig(EvaluatorConfig):
description="'exact' for full string match, 'contains' for keyword/substring match",
)
case_sensitive: bool = Field(False, description="Whether matching is case sensitive")
Copy link
Collaborator

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(\ )\b in contains mode.

Consider using strip():

if any(isinstance(value, str) and value.strip() == "" for value in values):
    raise ValueError("values must not contain empty or whitespace-only strings")

Same change needed in the defensive filter in evaluator.py:38 (if value != "" -> if value.strip()).


# Then: the evaluator ignores the empty control values
assert result.matched is False
assert result.message == "Empty control values - control ignored"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests the denylist path (match_on="match"), but the original report was about an allowlist blocking all calls. Could we add a companion case with match_on="no_match" that asserts matched is False for normal input? That encodes the failure mode that prompted this fix.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ListEvaluator: values=[""] builds match-all regex, can cause complete agent denial-of-service. Can be handled from UI/Backend.

2 participants