fix(checks): dot-wildcard/multi-field JSONPath returns scalar vs list depending on match count#2605
Conversation
… depending on match count _is_list_expression special-cases bracket wildcards (Slice, Union, Intersect) to force resolve() to return a list, but never checks jsonpath_ng.Fields -- what dot-wildcards like `trace.last.metadata.*` (and comma-separated multi-field selectors) actually compile to. As a result, resolve() falls through to its match-count heuristic (`len(matches) > 1 or _is_list_expression(expression)`) for these expressions: a wildcard matching exactly 1 key returns a bare scalar, while 2+ matches correctly returns a list. This breaks ComparisonCheck's match=any/all/none semantics (which require list/set/tuple) whenever the matched dict happens to have exactly one key -- entirely dependent on incidental data shape, not the expression's own structure. Fix: add a Fields branch treating any multi-field selector (`len(expression.fields) > 1`) or wildcard (`"*" in expression.fields`) as a list-expression, matching the existing bracket-wildcard handling. Added TestResolveDotWildcard covering the single-match, multi-match, and non-wildcard-single-field cases. TDD red->green verified: reverting only extraction.py reproduces `'v1' == ['v1']` (bare scalar instead of a list) for the single-match wildcard case; reapplying passes. Full libs/giskard-checks/tests/ suite (741 passed, 4 skipped); ruff/ ruff-format clean; basedpyright 0 errors (pre-existing unrelated warnings elsewhere in the file untouched). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates the JSONPath extraction logic to recognize multi-field and wildcard selectors (using the Fields expression) as list expressions, ensuring they consistently resolve to a list. Unit tests were added to verify wildcard resolution. The reviewer suggested adding an additional unit test to explicitly cover the multi-field selector branch to ensure complete test coverage.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| result = resolve(trace, "trace.last.metadata.*") | ||
| assert isinstance(result, list) | ||
| assert sorted(result) == ["v1", "v2"] | ||
|
|
There was a problem hiding this comment.
While the new implementation in extraction.py correctly handles multi-field selectors (where len(expression.fields) > 1), there is currently no unit test covering this specific branch. Adding a test case for a multi-field selector (e.g., using [k1,k2]) will ensure full test coverage and prevent future regressions.
| def test_multi_field_selector_returns_a_list(self): | |
| trace = Trace[str, str]( | |
| interactions=[ | |
| Interaction( | |
| inputs="hi", | |
| outputs="hello", | |
| metadata={"k1": "v1", "k2": "v2", "k3": "v3"}, | |
| ) | |
| ] | |
| ) | |
| result = resolve(trace, "trace.last.metadata.[k1,k2]") | |
| assert isinstance(result, list) | |
| assert sorted(result) == ["v1", "v2"] | |
kevinmessiaen
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix!
Problem
_is_list_expressionspecial-cases bracket wildcards (Slice,Union,Intersect) to forceresolve()to return a list, but never checksjsonpath_ng.Fields— what dot-wildcards liketrace.last.metadata.*(and comma-separated multi-field selectors) actually compile to.As a result,
resolve()falls through to its match-count heuristic (len(matches) > 1 or _is_list_expression(expression)) for these expressions: a wildcard matching exactly 1 key returns a bare scalar, while 2+ matches correctly returns a list. This breaksComparisonCheck'smatch=any/all/nonesemantics (which require list/set/tuple) whenever the matched dict happens to have exactly one key — entirely dependent on incidental data shape, not the expression's own structure.Fix
Add a
Fieldsbranch treating any multi-field selector (len(expression.fields) > 1) or wildcard ("*" in expression.fields) as a list-expression, matching the existing bracket-wildcard handling.Testing
TestResolveDotWildcardcovering the single-match, multi-match, and non-wildcard-single-field cases.extraction.pyreproduces'v1' == ['v1'](bare scalar instead of a list) for the single-match wildcard case; reapplying passes.libs/giskard-checks/tests/suite: 741 passed, 4 skipped (unrelated).ruff check/ruff format --checkandbasedpyright— 0 errors (pre-existing unrelated warnings elsewhere in the file untouched).core/extraction.py.AI-Generated disclosure
Found via an AI-assisted code review pass (Claude Code) over
giskard-checks/src/giskard/checks/core/. I personally reproduced the match-count-dependent behavior with a minimal repro, verified the fix against single-match/multi-match/non-wildcard cases, and ran the full test suite plus lint/type checks before submitting.