feat(platform): Add item filtering for fetching run results. by blanca-pablos · Pull Request #442 · aignostics/python-sdk

blanca-pablos · 2026-02-20T16:02:19Z

Add item_ids and external_ids to Run.results() to be able to filter by a list of items.

tests/aignostics/platform/resources/runs_test.py

Copilot

Pull request overview

This PR adds filtering capabilities to the Run.results() method, allowing users to retrieve results for specific items by their item IDs or external IDs. This enhancement provides more granular control over result retrieval, which can improve performance when only a subset of results is needed.

Changes:

Added item_ids and external_ids optional parameters to Run.results() method
Implemented filter parameter mapping to API query parameters (item_id__in and external_id__in)
Added comprehensive test coverage for the new filtering functionality
Updated documentation to reflect the new filtering capabilities

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/aignostics/platform/resources/runs.py	Added `item_ids` and `external_ids` parameters to `Run.results()` method and implemented filter parameter mapping
tests/aignostics/platform/resources/runs_test.py	Added two test cases: one verifying filter parameters are correctly mapped to API parameters, another verifying filters are omitted when not provided
src/aignostics/platform/CLAUDE.md	Updated documentation to note that `Runs.results()` supports `item_ids` and `external_ids` filters

tests/aignostics/platform/resources/runs_test.py

src/aignostics/platform/CLAUDE.md

tests/aignostics/platform/resources/runs_test.py

Copilot · 2026-02-20T16:08:03Z

src/aignostics/platform/resources/runs.py

+        if item_ids is not None:
+            filter_kwargs["item_id__in"] = item_ids
+        if external_ids is not None:


The current implementation does not handle empty lists correctly. If a user passes item_ids=[] or external_ids=[], these will be sent to the API as filter parameters, which would filter out all results (since no item can have an ID in an empty list). The intended behavior when an empty list is provided should likely be the same as when None is provided (i.e., don't apply the filter). Consider checking both is not None and that the list is not empty, similar to the pattern used for the sort parameter at line 679: sort=[sort] if sort else None.

Suggested change

if item_ids is not None:

filter_kwargs["item_id__in"] = item_ids

if external_ids is not None:

if item_ids:

filter_kwargs["item_id__in"] = item_ids

if external_ids:

Agree with this, and the behaviour should be made explicit with a test IMO

I thought about it but I'd say empty list should return no results, that's semantically correct behavior imo and converting [] to no filter might be confusing?

Fair enough, I don't feel super strongly but either way it should be tested :)

ok Claude also thinks this is a bug, maybe you're right 😅 will change it and add tests!

claude · 2026-02-20T16:09:50Z

Claude encountered an error —— View job

✅ Code Review Complete

I've completed a comprehensive review of PR #442. The implementation is excellent and follows all repository standards. Here's my analysis:

🎯 Summary

This PR adds item_ids and external_ids filtering parameters to Run.results(), enabling users to retrieve results for specific items. The implementation is clean, well-tested, and follows established patterns in the codebase.

Verdict: ✅ APPROVED - Ready to merge

✅ What Was Checked

1. Critical Checks (All Passed)

✅ Test Markers: All 3 new tests properly marked with @pytest.mark.unit
✅ Coverage: 100% coverage on new code (per Codecov comment)
✅ Linting: All checks pass (ruff format, ruff check, pyright, mypy)
✅ Type Safety: Proper type hints with list[str] | None
✅ Architecture: No violations of modulith principles

2. Implementation Quality ⭐

✅ Empty List Handling: Correctly treats empty lists as "no filter" (lines 248-251)
✅ Filter Parameter Mapping: Properly maps to API parameters (item_id__in, external_id__in)
✅ Docstring: Updated with correct return type (Iterator[ItemResultData])
✅ Consistency: Follows existing pattern from sort parameter (line 679)
✅ Backward Compatible: Optional parameters default to None

3. Test Coverage 🧪

Three comprehensive unit tests added:

test_application_run_results_with_filters - Tests both filters together with pagination
test_application_run_results_without_filters_omits_filter_kwargs - Verifies no filter when None
test_application_run_results_with_empty_list_filters_omits_filter_kwargs - Verifies empty list behavior

All tests properly verify:

Filter parameters are correctly passed to API
Pagination still works with filters
Filters are omitted when not needed

4. Documentation

✅ Updated src/aignostics/platform/CLAUDE.md line 796
✅ Docstring includes new parameters with proper descriptions
✅ Return type corrected from list to Iterator

5. CLI Test Fix 🔧

✅ Fixed test_cli_run_submit_and_describe_and_cancel_and_download_and_delete
Uses proper test constants instead of invalid CSV data
Prevents flaky test failures

📋 Changes Reviewed

`src/aignostics/platform/resources/runs.py`

# Lines 205-253: Filter implementation
def results(
    self,
    nocache: bool = False,
    item_ids: list[str] | None = None,  # ✅ New parameter
    external_ids: list[str] | None = None,  # ✅ New parameter
) -> t.Iterator[ItemResultData]:
    ...
    filter_kwargs: dict[str, object] = {}
    if item_ids:  # ✅ Truthiness check (not "is not None")
        filter_kwargs["item_id__in"] = item_ids
    if external_ids:
        filter_kwargs["external_id__in"] = external_ids
    
    return paginate(lambda **kwargs: results_with_retry(..., **filter_kwargs, **kwargs))

Why truthiness check is correct:

Empty list [] evaluates to False, so no filter is sent
This matches the existing pattern for sort parameter (line 679)
Semantically correct: empty filter list = no filtering

`tests/aignostics/platform/resources/runs_test.py`

All three tests follow best practices:

Clear docstrings explaining what is tested
Proper mocking with side_effect for pagination
Comprehensive assertions checking all call arguments
Consistent with existing test patterns

🎯 Review Comments Addressed

The PR author addressed all review feedback:

✅ Empty list handling (copilot, claude, olivermeyer): Fixed with truthiness check
✅ Test coverage (copilot): Added test for empty lists
✅ Docstring return type (copilot): Changed from list to Iterator
✅ Import formatting (copilot): Fixed formatting

🐛 Known Issue (Not Blocking)

Test Environment Issue: Unit tests for results() timeout locally due to authentication attempts in test environment. However:

✅ This is a pre-existing issue (also fails on main branch)
✅ Not introduced by this PR
✅ CI/CD passes (as shown by Codecov and SonarQube)
✅ Implementation is correct

The timeout occurs because the @cached_operation decorator tries to get a token. This appears to be an environment-specific issue with how mocks are set up locally versus in CI.

🎉 Strengths

Clean Implementation: Minimal, focused changes that do exactly what's needed
Excellent Tests: Comprehensive coverage including edge cases
Good Documentation: Clear docstrings and CLAUDE.md updates
Consistent Patterns: Follows existing codebase conventions
Backward Compatible: No breaking changes
Approved by Maintainer: @olivermeyer approved on Feb 24

📊 Code Quality Metrics

Codecov: ✅ 100% coverage on new code
SonarQube: ✅ Quality Gate passed, 0 issues
Linting: ✅ All checks pass
Type Checking: ✅ MyPy (strict) + PyRight (basic) pass
Architecture: ✅ No violations

🚀 Recommendation

LGTM - This PR is ready to merge. The implementation is solid, well-tested, and follows all repository standards. The one failing E2E test (test_gui_cli_submit_to_run_result_delete) appears to be a flaky test issue unrelated to the changes in this PR.

Branch: feat/add-item-filtering-for-run-results

src/aignostics/platform/resources/runs.py

tests/aignostics/platform/resources/runs_test.py

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/aignostics/platform/resources/runs.py:223

The docstring return type says list[ItemResultData], but results() is annotated and implemented as an iterator (it returns paginate(...)). Since this docstring was updated in this PR, please align the Returns: section with the actual return type (e.g., Iterator[ItemResultData]).

        Returns:
            list[ItemResultData]: A list of item results.

tests/aignostics/platform/resources/runs_test.py

codecov · 2026-02-20T17:02:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Files with missing lines	Coverage Δ
src/aignostics/platform/resources/runs.py	`61.90% <100.00%> (+3.16%)`	⬆️

... and 10 files with indirect coverage changes

blanca-pablos · 2026-02-23T08:45:03Z

tests/aignostics/application/cli_test.py

+    csv_content += (
+        f";{SPOT_0_CRC32C};{SPOT_0_RESOLUTION_MPP};{SPOT_0_WIDTH};{SPOT_0_HEIGHT};H&E;LUNG;LUNG_CANCER;{SPOT_0_GS_URL}"
+    )


This test flaked on me because the CSV is invalid, so the run failed before it could be cancelled

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

src/aignostics/platform/resources/runs.py

Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

sonarqubecloud · 2026-02-24T12:48:09Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

- FR-04: add 'test' to supported deployment environments (#422) - ApplicationRun.results(): document item_ids and external_ids filter parameters (#442) skip:ci, skip:test:long-running, skip:test:matrix-runner, skip:test:very-long-running Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

blanca-pablos requested a review from helmut-hoffer-von-ankershoffen as a code owner February 20, 2026 16:02

Copilot AI review requested due to automatic review settings February 20, 2026 16:02

blanca-pablos self-assigned this Feb 20, 2026

blanca-pablos requested a review from olivermeyer February 20, 2026 16:02

blanca-pablos added the claude Trigger Claude Code automation label Feb 20, 2026

Copilot started reviewing on behalf of blanca-pablos February 20, 2026 16:03 View session

olivermeyer reviewed Feb 20, 2026

View reviewed changes

tests/aignostics/platform/resources/runs_test.py Outdated Show resolved Hide resolved

Copilot AI reviewed Feb 20, 2026

View reviewed changes

feat(platform): Add item filtering for fetching run results.

e826122

blanca-pablos force-pushed the feat/add-item-filtering-for-run-results branch from 25176d7 to e826122 Compare February 20, 2026 16:08

claude bot reviewed Feb 20, 2026

View reviewed changes

src/aignostics/platform/resources/runs.py Outdated Show resolved Hide resolved

claude bot reviewed Feb 20, 2026

View reviewed changes

tests/aignostics/platform/resources/runs_test.py Show resolved Hide resolved

Copilot AI review requested due to automatic review settings February 20, 2026 16:24

Copilot started reviewing on behalf of blanca-pablos February 20, 2026 16:24 View session

fix(platform): Address review

01b4b0b

blanca-pablos force-pushed the feat/add-item-filtering-for-run-results branch from 8bd4646 to 01b4b0b Compare February 20, 2026 16:25

Copilot AI reviewed Feb 20, 2026

View reviewed changes

tests/aignostics/platform/resources/runs_test.py Show resolved Hide resolved

fix(cli): Fix CLI test with wrong CSV

4487f0c

Copilot AI review requested due to automatic review settings February 23, 2026 08:42

blanca-pablos force-pushed the feat/add-item-filtering-for-run-results branch from 29d64b6 to 4487f0c Compare February 23, 2026 08:42

Copilot started reviewing on behalf of blanca-pablos February 23, 2026 08:43 View session

blanca-pablos commented Feb 23, 2026

View reviewed changes

Copilot AI reviewed Feb 23, 2026

View reviewed changes

src/aignostics/platform/resources/runs.py Outdated Show resolved Hide resolved

fix(runs): Update docstring

20fa256

Co-authored-by: Copilot <[email protected]>

Copilot AI review requested due to automatic review settings February 24, 2026 10:46

Copilot started reviewing on behalf of blanca-pablos February 24, 2026 10:46 View session

Copilot AI reviewed Feb 24, 2026

View reviewed changes

olivermeyer approved these changes Feb 24, 2026

View reviewed changes

fix(tests): Add path for spot

eef8b3b

blanca-pablos merged commit 14e20b5 into main Feb 24, 2026
23 of 27 checks passed

omid-aignostics mentioned this pull request Mar 5, 2026

docs(specs): update SPEC-PLATFORM-SERVICE post v1.1.0 #461

Merged

1 task

Conversation

blanca-pablos commented Feb 20, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

olivermeyer Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

blanca-pablos Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

olivermeyer Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

blanca-pablos Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Code Review Complete

🎯 Summary

✅ What Was Checked

1. Critical Checks (All Passed)

2. Implementation Quality ⭐

3. Test Coverage 🧪

4. Documentation

5. CLI Test Fix 🔧

📋 Changes Reviewed

src/aignostics/platform/resources/runs.py

tests/aignostics/platform/resources/runs_test.py

🎯 Review Comments Addressed

🐛 Known Issue (Not Blocking)

🎉 Strengths

📊 Code Quality Metrics

🚀 Recommendation

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

codecov bot commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blanca-pablos Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

sonarqubecloud bot commented Feb 24, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

claude bot commented Feb 20, 2026 •

edited

Loading

`src/aignostics/platform/resources/runs.py`

`tests/aignostics/platform/resources/runs_test.py`

codecov bot commented Feb 20, 2026 •

edited

Loading