Skip to content

fix(checks): descendants JSONPath returns scalar vs list depending on match count - #2634

Closed
Qalipso wants to merge 1 commit into
Giskard-AI:mainfrom
Qalipso:fix/jsonpath-descendants-list
Closed

fix(checks): descendants JSONPath returns scalar vs list depending on match count#2634
Qalipso wants to merge 1 commit into
Giskard-AI:mainfrom
Qalipso:fix/jsonpath-descendants-list

Conversation

@Qalipso

@Qalipso Qalipso commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Fixes #2633.

Problem

_is_list_expression handles Descendants in the same recursive branch as Child:

if isinstance(expression, Child | Descendants):
    return _is_list_expression(expression.right) or _is_list_expression(expression.left)

For trace..ctx both children are single-field Fields, so this returns False and resolve() falls back to its match-count heuristic (len(matches) > 1 or _is_list_expression(expression)). A .. expression then yields a bare scalar when the data holds exactly one match and a list when it holds two or more.

This is the defect #2605 fixed for dot-wildcards and multi-field selectors. That fix added a Fields branch; .. compiles to Descendants and was not covered. .. is a supported expression — test_extraction.py already asserts trace..outputs validates.

It breaks ComparisonCheck's match=any/all/none, which requires a list/set/tuple:

Equals(key="trace..ctx", expected_value="doc-1", match="any")

one interaction carries ctx : FAIL | Expected a list, set, or tuple at key 'trace..ctx'
                                     when match is 'any', but got str.
two interactions carry ctx  : PASS | At least one value in ['doc-1', 'doc-2'] is equal to 'doc-1'.

Identical check configuration; the outcome depends on incidental data shape.

Fix

Give Descendants its own branch returning True, and leave Child recursing. A .. searches a whole subtree, so its match count is a property of the data, not of the expression — the same reasoning the bracket-wildcard and Fields branches already encode.

Behavior change worth flagging

trace..missing now returns [] instead of NoMatch. That matches how list-expressions already behave — trace.last.metadata.* on empty metadata returns [] today — while ordinary paths such as trace.last.nope keep returning NoMatch. Covered by a test.

Verification

  • uv run pytest libs/giskard-checks -m "not functional"761 passed, 4 skipped (757 passed, 4 skipped on main before this change; the 4 added tests are the difference).
  • TDD red→green: reverting only extraction.py reproduces 'doc-1' == ['doc-1'] (bare scalar) and fails 2 of the new tests; reapplying passes all 5 in the class.
  • ruff check / ruff format --check on both changed files: clean.
  • basedpyright on extraction.py: 0 errors, 28 warnings — byte-identical to the count on main, so nothing new was introduced.

Python 3.13, jsonpath-ng from the pinned range.

Separate observation, not addressed here

Trace.model_dump() includes a last key duplicating the final interaction, so trace..outputs on a single-interaction trace returns ['hello', 'hello']. Different problem, different blast radius — noted in #2633 and left alone.

… match count

_is_list_expression handles Descendants in the same recursive branch as
Child, so `trace..ctx` -- whose children are both single-field Fields --
returns False. resolve() then falls back to its match-count heuristic
(`len(matches) > 1 or _is_list_expression(expression)`), and a `..`
expression yields a bare scalar when the data holds exactly one match and
a list when it holds two or more.

This is the defect Giskard-AI#2605 fixed for dot-wildcards and multi-field
selectors; `..` compiles to Descendants and was not covered. It breaks
ComparisonCheck's match=any/all/none, which requires a list/set/tuple:
Equals(key="trace..ctx", expected_value="doc-1", match="any") fails with
a type error when one interaction carries the field and passes when two
do, for an identical check configuration.

Give Descendants its own branch returning True and leave Child recursing.
A `..` searches a whole subtree, so its match count is a property of the
data, not of the expression.

Consequence: `trace..missing` now returns [] rather than NoMatch, which
matches how list-expressions already behave (`trace.last.metadata.*` on
empty metadata returns [] today). Ordinary paths such as
`trace.last.nope` still return NoMatch.

Fixes Giskard-AI#2633
@Qalipso
Qalipso force-pushed the fix/jsonpath-descendants-list branch from 6cef49c to 1733080 Compare July 26, 2026 03:28
@davidberenstein1957
davidberenstein1957 self-requested a review July 29, 2026 14:13
Comment on lines +68 to +72
# `..` searches a whole subtree, so how many values it finds is a property
# of the data, not of the expression. Treat it as a list-expression the way
# bracket wildcards are, or resolve() falls back to its match-count
# heuristic and returns a bare scalar whenever the data happens to hold one
# match.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
# `..` searches a whole subtree, so how many values it finds is a property
# of the data, not of the expression. Treat it as a list-expression the way
# bracket wildcards are, or resolve() falls back to its match-count
# heuristic and returns a bare scalar whenever the data happens to hold one
# match.

@davidberenstein1957

Copy link
Copy Markdown
Member

Opened a same-repo follow-up with the same fix plus format fixes so CI lint can pass (fork PRs cannot receive the auto-format push): this supersedes the branch here.

Thanks for the clear repro and tests — credited as co-author on the fix commit.

@davidberenstein1957

Copy link
Copy Markdown
Member

Opened a formatted follow-up so CI check-format can pass: #2645

Same change as this PR, plus the markdown/ruff format fix. Please use #2645 going forward — we can close this one once that is green.

@Qalipso

Qalipso commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — happy to have #2645 carry it, and no objection to closing this once that lands.

One heads-up: #2645 currently shows as conflicting with main (mergeStateStatus: DIRTY), same for #2652. Keeping this branch open until yours is green, as you suggested; say the word and I'll close it, or rebase this one instead if that turns out to be easier.

kevinmessiaen added a commit that referenced this pull request Aug 5, 2026
* style: auto-format README code blocks for current ruff

Unblocks check-format on PRs; same drift as CI style: auto-format.

* fix(checks): descendants JSONPath always returns a list

Treat Descendants as a list-expression so resolve() does not fall back
to the match-count heuristic and return a bare scalar for a single hit.

Fixes #2633.
Supersedes #2634.

Co-authored-by: Qalipso <quadwailt@gmail.com>

* fix(checks): drop unnecessary pyright ignore for MISSING

The reportInvalidTypeForm ignore is flagged as unnecessary after
chore(pyright): flag and remove unused ignore comments (#2690) merged
from main, which blocked the lint CI job on PR #2644.

---------

Co-authored-by: Qalipso <quadwailt@gmail.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Kevin Messiaen <kevinmessiaen@users.noreply.github.com>
Co-authored-by: Kevin Messiaen <114553769+kevinmessiaen@users.noreply.github.com>
@kevinmessiaen

Copy link
Copy Markdown
Member

Duplicate of #2644

@kevinmessiaen kevinmessiaen marked this as a duplicate of #2644 Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

JSONPath descendants (..) resolves to a scalar or a list depending on the data

3 participants