diff --git a/app/desktop/studio_server/test_eval_api.py b/app/desktop/studio_server/test_eval_api.py index 8ea0cb9a5a..8b20306c88 100644 --- a/app/desktop/studio_server/test_eval_api.py +++ b/app/desktop/studio_server/test_eval_api.py @@ -6388,6 +6388,33 @@ def test_exact_match_fail(self, client, mock_task, mock_task_from_id): assert response.status_code == 200 assert response.json()["scores"]["accuracy"] == 0.0 + @pytest.mark.parametrize( + "final_message,expected_score", + [ + # Same expression and same expected_value both times. The only + # difference is whether `user` is there, so the 0.0 can only come + # from the extraction failing -- not from a value that never matched. + ('{"user": {"status": "hello"}}', 1.0), + ('{"status": "hello"}', 0.0), + ], + ids=["field_present", "field_missing"], + ) + def test_missing_nested_field_scores_fail( + self, client, mock_task, mock_task_from_id, final_message, expected_score + ): + # Reaching into a value that isn't there is a scored FAIL, not a request error. + mock_task_from_id.return_value = mock_task + payload = self._payload() + payload["properties"]["value_expression"] = ( + "(final_message | fromjson).user.status" + ) + payload["eval_input"]["final_message"] = final_message + response = client.post(self._url(), json=payload) + assert response.status_code == 200 + body = response.json() + assert body["scores"]["accuracy"] == expected_score + assert body["skipped_reason"] is None + def test_nothing_is_persisted(self, client, mock_task, mock_task_from_id): mock_task_from_id.return_value = mock_task response = client.post(self._url(), json=self._payload()) diff --git a/libs/core/kiln_ai/adapters/eval/test_v2_eval_helpers.py b/libs/core/kiln_ai/adapters/eval/test_v2_eval_helpers.py index f366044a5f..bba4ca02cf 100644 --- a/libs/core/kiln_ai/adapters/eval/test_v2_eval_helpers.py +++ b/libs/core/kiln_ai/adapters/eval/test_v2_eval_helpers.py @@ -170,6 +170,19 @@ def test_fromjson_valid_json_succeeds(self): assert value == "val" assert fail_result is None + def test_empty_trace_scores_fail_not_missing_trace_skip(self): + # A trace that exists but is empty is not a missing trace: the run did + # produce one, it just has nothing in it. That scores FAIL, so the + # missing-trace skip must key off `is None`, not falsiness. + inp = _make_input(trace=[]) + value, fail_result = extract_output_value( + "trace[-1].tool_calls[0].function.name", inp, _SAMPLE_SCORES + ) + assert value is None + assert fail_result is not None + assert fail_result.skipped_reason is None + assert fail_result.scores == {"s1": 0.0, "s2": 0.0} + # --------------------------------------------------------------------------- # stringify_for_match diff --git a/libs/core/kiln_ai/datamodel/eval.py b/libs/core/kiln_ai/datamodel/eval.py index 7d0cc71823..03e4ca642c 100644 --- a/libs/core/kiln_ai/datamodel/eval.py +++ b/libs/core/kiln_ai/datamodel/eval.py @@ -1028,7 +1028,7 @@ def validate_properties(self) -> Self: raise ValueError(f"Invalid eval config type: {self.config_type}") @model_validator(mode="after") - def validate_v2_templates_and_expressions(self) -> Self: + def validate_v2_templates_and_expressions(self, info: ValidationInfo) -> Self: if self.config_type != EvalConfigType.v2 or not isinstance( self.properties, BaseModel ): @@ -1037,6 +1037,7 @@ def validate_v2_templates_and_expressions(self) -> Self: from kiln_ai.utils.jinja_engine import ( compile_expression_or_raise, compile_template_or_raise, + expression_variables, ) props = self.properties @@ -1069,6 +1070,25 @@ def validate_v2_templates_and_expressions(self) -> Self: ): if props.value_expression is not None: compile_expression_or_raise(props.value_expression) + # Syntax alone isn't enough: a typo'd root variable resolves to + # Undefined at eval time, which scores every row a silent 0.0. + # Catch it while the author can still see what they typed. + # + # Authoring-time only. A config written before this check exists + # may name a variable we now reject, and refusing to load it would + # take the whole eval down rather than the one check that was + # already scoring zeros. + if not self.loading_from_file(info): + allowed = set(EvalTaskInput.model_fields.keys()) + unknown = sorted( + expression_variables(props.value_expression) - allowed + ) + if unknown: + raise ValueError( + f"value_expression references unknown variable " + f"'{unknown[0]}'. Available variables: " + f"{', '.join(sorted(allowed))}." + ) return self diff --git a/libs/core/kiln_ai/datamodel/test_eval_model.py b/libs/core/kiln_ai/datamodel/test_eval_model.py index 02586cc43b..66487927bf 100644 --- a/libs/core/kiln_ai/datamodel/test_eval_model.py +++ b/libs/core/kiln_ai/datamodel/test_eval_model.py @@ -2557,6 +2557,16 @@ def _make_v2_eval_config(**kwargs) -> EvalConfig: return EvalConfig(name="V2 Test", config_type=EvalConfigType.v2, **kwargs) +# One value_expression per EvalTaskInput field, keyed by the root variable it +# reads. Checked against EvalTaskInput.model_fields so the coverage claim holds. +_EVAL_INPUT_FIELD_EXPRESSIONS = { + "final_message": "final_message", + "trace": "trace[-1].content", + "task_input": "task_input | upper", + "reference_data": "reference_data", +} + + class TestV2TemplateValidation: def test_valid_prompt_template(self): """A prompt_template with a Jinja expression passes validation.""" @@ -2666,6 +2676,69 @@ def test_invalid_value_expression(self): ), ) + @pytest.mark.parametrize( + "root,expression", + list(_EVAL_INPUT_FIELD_EXPRESSIONS.items()), + ) + def test_value_expression_accepts_every_eval_input_field(self, root, expression): + # Tied to the model so a new EvalTaskInput field can't leave this test + # named "every field" while silently skipping one. + assert set(_EVAL_INPUT_FIELD_EXPRESSIONS) == set(EvalTaskInput.model_fields) + cfg = _make_v2_eval_config( + properties=ExactMatchProperties( + expected_value="yes", + value_expression=expression, + ), + ) + assert cfg.properties.value_expression == expression + + @pytest.mark.parametrize( + "expression,unknown", + [ + ("outpt.status", "outpt"), + ("messages[-1].content", "messages"), + ("final_message ~ typo", "typo"), + ], + ) + def test_value_expression_rejects_unknown_variable(self, expression, unknown): + """A typo'd root fails silently at runtime, so it has to fail loudly here. + + Depending on what the expression does with it, the typo either resolves + to Undefined and scores every row 0.0, or -- as with `~`, which + stringifies Undefined to '' -- produces a plausible-looking wrong value + ('hi' for `final_message ~ typo`) that no one notices. + """ + with pytest.raises( + ValidationError, match=f"unknown variable '{unknown}'" + ) as exc: + _make_v2_eval_config( + properties=ExactMatchProperties( + expected_value="yes", + value_expression=expression, + ), + ) + assert "final_message, reference_data, task_input, trace" in str(exc.value) + + def test_value_expression_unknown_variable_still_loads_from_file(self): + """Already-saved configs keep loading; the check gates writes, not reads.""" + cfg = EvalConfig.model_validate( + { + "v": 1, + "id": "123", + "name": "Saved before the check existed", + "config_type": "v2", + "model_type": "eval_config", + "properties": { + "type": "exact_match", + "expected_value": "yes", + "value_expression": "outpt.status", + }, + }, + context={"loading_from_file": True}, + ) + assert isinstance(cfg.properties, ExactMatchProperties) + assert cfg.properties.value_expression == "outpt.status" + def test_none_value_expression_skipped(self): """value_expression=None (default) should not be validated.""" cfg = _make_v2_eval_config( diff --git a/libs/core/kiln_ai/utils/jinja_engine.py b/libs/core/kiln_ai/utils/jinja_engine.py index ebea2cab70..5ce5bec79f 100644 --- a/libs/core/kiln_ai/utils/jinja_engine.py +++ b/libs/core/kiln_ai/utils/jinja_engine.py @@ -10,6 +10,7 @@ Public API: - compile_template_or_raise(template) -> None - compile_expression_or_raise(expression) -> None + - expression_variables(expression) -> set[str] - render_input_transform(transform, task_input) -> str - extract(expression, data) -> Any """ @@ -20,7 +21,7 @@ import types from typing import TYPE_CHECKING, Any -from jinja2 import StrictUndefined, TemplateSyntaxError, Undefined +from jinja2 import StrictUndefined, TemplateSyntaxError, Undefined, UndefinedError from jinja2.sandbox import SandboxedEnvironment if TYPE_CHECKING: @@ -28,10 +29,13 @@ class JinjaExtractionError(ValueError): - """Raised when a Jinja2 filter encounters invalid data during extraction. + """Raised when a Jinja2 expression encounters missing or invalid data during extraction. - For example, the ``fromjson`` filter raises this when the input string is - not valid JSON. + Covers both filters that reject their input (``fromjson`` on a non-JSON + string) and expressions that operate on a value that isn't there (reaching + into a missing nested field, indexing past the end of a list). Callers treat + it as "this input didn't have what the expression asked for" rather than an + unexpected crash. """ @@ -85,6 +89,24 @@ def compile_expression_or_raise(expression: str) -> None: ) from e +def expression_variables(expression: str) -> set[str]: + """Return the namespace variables a Jinja2 expression reads. + + An expression is not a template -- ``final_message.strip()`` parsed as a + template is just literal text with no variables -- so it is wrapped in an + output block before the AST walk. Raises ValueError on syntax error. + """ + from jinja2 import meta + + try: + ast = _expression_env.parse("{{ " + expression + " }}") + except TemplateSyntaxError as e: + raise ValueError( + f"Invalid Jinja2 expression: {e.message} (line {e.lineno})" + ) from e + return meta.find_undeclared_variables(ast) + + def render_input_transform( transform: InputTransform, task_input: Any, @@ -108,6 +130,7 @@ def extract(expression: str, data: dict) -> Any: - Missing keys return Undefined (not None, not a raise). - Explicit null values return None. - Generators are auto-materialized to lists. + - Operating on a missing value raises JinjaExtractionError, never UndefinedError. """ try: compiled = _expression_env.compile_expression( @@ -117,9 +140,19 @@ def extract(expression: str, data: dict) -> Any: raise ValueError( f"Invalid Jinja2 expression: {e.message} (line {e.lineno})" ) from e - result = compiled(**data) - if isinstance(result, types.GeneratorType): - result = list(result) + try: + result = compiled(**data) + # Materializing inside the try is load-bearing: map/selectattr/groupby + # return lazy generators, so their per-item lookups don't run until + # list() does. A missing field there raises here, not above. + if isinstance(result, types.GeneratorType): + result = list(result) + except UndefinedError as e: + # A single missing lookup yields Undefined, but any further operation on + # it (attribute access, indexing, a filter) raises. That's still just + # missing data, so surface it as an extraction error callers handle -- + # keeping Jinja's message, which names the field that wasn't there. + raise JinjaExtractionError(str(e)) from e return result diff --git a/libs/core/kiln_ai/utils/test_jinja_engine.py b/libs/core/kiln_ai/utils/test_jinja_engine.py index 20d3d37ad7..3e314a2290 100644 --- a/libs/core/kiln_ai/utils/test_jinja_engine.py +++ b/libs/core/kiln_ai/utils/test_jinja_engine.py @@ -7,6 +7,7 @@ JinjaExtractionError, compile_expression_or_raise, compile_template_or_raise, + expression_variables, extract, render_input_transform, ) @@ -173,6 +174,86 @@ def test_malformed_expression_raises_value_error(self): extract("{{ invalid", {}) +class TestExpressionVariables: + @pytest.mark.parametrize( + "expression,expected", + [ + # A bare expression has no {{ }} of its own, so it only reports + # variables once the function wraps it. + ("final_message", {"final_message"}), + ("final_message.strip()", {"final_message"}), + ("(final_message | fromjson).user.status", {"final_message"}), + ("trace | map(attribute='x') | list", {"trace"}), + ("outpt.status", {"outpt"}), + # Delimiters inside a string literal are lexed as text, not as the + # end of the wrapper block. + ('final_message ~ "}}"', {"final_message"}), + ("'literal'", set()), + ], + ) + def test_reports_referenced_variables(self, expression, expected): + assert expression_variables(expression) == expected + + def test_syntax_error_raises_value_error(self): + with pytest.raises(ValueError, match="Invalid Jinja2 expression"): + expression_variables("final_message[") + + +class TestExtractOperationsOnMissingData: + """Operating on a missing value is an extraction error, not an unhandled raise. + + A single missing lookup returns Undefined, but anything done to that + Undefined raises inside Jinja. Callers only handle JinjaExtractionError, so + extract() must convert -- while keeping Jinja's message, which names the + field that wasn't there. + """ + + @pytest.mark.parametrize( + "expression,data,expected_message", + [ + # A filter or operator that forces the Undefined. + ("missing | int", {}, "'missing' is undefined"), + ("missing + 1", {}, "'missing' is undefined"), + # Reaching into a field the parsed JSON doesn't have. + ( + "(final_message | fromjson).user.status", + {"final_message": '{"status": "ok"}'}, + "'dict object' has no attribute 'user'", + ), + # Indexing past the end of a list. + ( + "trace[-1].tool_calls[0].function.name", + {"trace": []}, + "has no element -1", + ), + ], + ) + def test_raising_operation_on_missing_value( + self, expression, data, expected_message + ): + # Only operations that force the value raise; others stay silent, which + # test_silent_operation_on_missing_value pins. + with pytest.raises(JinjaExtractionError, match=expected_message): + extract(expression, data) + + def test_silent_operation_on_missing_value(self): + # The other side of the boundary: `~` stringifies Undefined to '' rather + # than raising, so a typo'd name yields a wrong value, not an error. + assert extract("final_message ~ typo", {"final_message": "hi"}) == "hi" + + def test_lazy_generator_over_missing_field_raises_extraction_error(self): + # The sole guard that generators are materialized inside extract()'s try: + # map() defers its per-item lookups, so the raise fires during list(), + # not when the compiled expression is called. + with pytest.raises( + JinjaExtractionError, match="'dict object' has no attribute 'tool_calls'" + ): + extract( + "trace | map(attribute='tool_calls.0.function.name')", + {"trace": [{"content": "no tools"}]}, + ) + + class TestTrimAndLstripBlocks: def test_trim_blocks_strips_newline_after_block_tag(self): template = "{% if True %}\nyes\n{% endif %}"