Chore/papi 4760 stop using download url

[nahua-aignx]

Gist of the PR

• We will remove download_url from the artifact schema
• Dima already added this new endpoint GET runs/{run-id}/artifacts/{artifact-id}/file  which can be used to download the file instead of using the Signed URL directly
• Python SDK (and launchpad?) must construct this URL based on the artifact-id which is still return from the artifact schema to download files

[Copilot]

Pull request overview

This PR updates the SDK’s artifact downloading flow to stop relying on download_url from the artifact schema and instead resolve a presigned URL via the new GET /v1/runs/{run_id}/artifacts/{artifact_id}/file endpoint.

Changes:

• Added Run.get_artifact_download_url() and updated run artifact downloads to resolve URLs via the new endpoint.
• Updated application download logic to accept an optional (run_id, artifact_id) -> url resolver callback.
• Updated the Launchpad run describe page to resolve URLs on-demand for preview/download actions, and added/updated unit tests.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/aignostics/platform/resources/runs.py	Adds URL resolution helper and switches artifact downloads to use artifact IDs + file endpoint.
src/aignostics/application/_download.py	Adds optional URL resolver callback and threads resolved URL into download_item_artifact.
src/aignostics/application/_service.py	Wires a resolver callback into the application run download loop.
src/aignostics/application/_gui/_page_application_run_describe.py	Updates GUI preview/download buttons to resolve presigned URLs via the run endpoint.
tests/aignostics/platform/resources/runs_test.py	Adds tests for URL resolution + updated artifact download behavior.
tests/aignostics/application/download_test.py	Adds tests for resolver callback behavior and explicit URL handling in downloads.

[Copilot]

Pull request overview

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

[codecov]

Codecov Report

❌ Patch coverage is 76.97842% with 32 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...application/_gui/_page_application_run_describe.py	45.45%	23 Missing and 1 partial ⚠️
src/aignostics/platform/resources/runs.py	89.47%	5 Missing and 3 partials ⚠️

Files with missing lines	Coverage Δ
src/aignostics/application/_download.py	97.40% <100.00%> (+18.98%)	⬆️
src/aignostics/application/_service.py	61.32% <100.00%> (-6.21%)	⬇️
src/aignostics/platform/__init__.py	100.00% <100.00%> (ø)
src/aignostics/platform/resources/runs.py	78.32% <89.47%> (+13.10%)	⬆️
...application/_gui/_page_application_run_describe.py	57.69% <45.45%> (-1.50%)	⬇️

... and 8 files with indirect coverage changes

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[claude]

Claude finished @nahua-aignx's task in 7m 45s —— View job

---

PR Review Complete

Reviewing PR #507: Stop using download_url from artifact schema

---

Summary

This PR migrates artifact downloads from using the deprecated download_url field to resolving presigned URLs via the new GET /v1/runs/{run_id}/artifacts/{artifact_id}/file endpoint. The implementation is well-structured with comprehensive tests, but there are critical exception handling issues causing the E2E test failures.

---

🔴 BLOCKING ISSUES (Must Fix)

1. Incomplete Exception Handling in get_artifact_download_url() - ROOT CAUSE OF TEST FAILURES

Location: src/aignostics/platform/resources/runs.py:376-406

Problem: The method catches some requests exceptions but not all, causing unhandled exceptions to escape:

def _resolve() -> str:
    token = token_provider()
    try:
        response = requests.get(...)  # Line 379
    except requests.Timeout as e:
        raise ServiceException(...) from e  # ✅ Handled
    except requests.ConnectionError as e:
        raise ServiceException(...) from e  # ✅ Handled
    # ❌ Missing: requests.HTTPError, requests.RequestException
    
    # ... status code checks ...
    
    response.raise_for_status()  # Line 404 - Raises requests.HTTPError!
    # ❌ This HTTPError is NOT caught!

Impact:

• Breaks E2E tests: Both failing tests timeout waiting for download completion because exceptions are escaping
• No retry logic: Uncaught requests exceptions are NOT in RETRYABLE_EXCEPTIONS, so Tenacity won't retry
• Inconsistent error handling: Users get raw requests exceptions instead of SDK exceptions

Fix Required:

def _resolve() -> str:
    token = token_provider()
    try:
        response = requests.get(
            endpoint_url,
            headers={"Authorization": f"Bearer {token}", "User-Agent": user_agent()},
            allow_redirects=False,
            timeout=settings().run_timeout,
        )
    except requests.Timeout as e:
        raise ServiceException(status=HTTPStatus.SERVICE_UNAVAILABLE, reason="Request timed out") from e
    except requests.ConnectionError as e:
        raise ServiceException(status=HTTPStatus.SERVICE_UNAVAILABLE, reason="Connection failed") from e
    except requests.RequestException as e:  # ✅ ADD THIS - Catch all other requests exceptions
        raise ServiceException(
            status=HTTPStatus.SERVICE_UNAVAILABLE, 
            reason=f"Request failed: {e}"
        ) from e
    
    # Status code handling
    if response.status_code in {
        HTTPStatus.MOVED_PERMANENTLY,
        HTTPStatus.FOUND,
        HTTPStatus.TEMPORARY_REDIRECT,
        HTTPStatus.PERMANENT_REDIRECT,
    }:
        location = response.headers.get("Location")
        if not location:
            msg = f"Redirect response {response.status_code} missing Location header for artifact {artifact_id}"
            raise RuntimeError(msg)
        return location
    if response.status_code == HTTPStatus.NOT_FOUND:
        raise NotFoundException(status=HTTPStatus.NOT_FOUND, reason="Artifact not found")
    if response.status_code >= HTTPStatus.INTERNAL_SERVER_ERROR:
        raise ServiceException(status=response.status_code, reason=response.reason)
    
    # Handle 4xx errors (other than 404)
    if response.status_code >= 400:  # ✅ ADD THIS - Explicit 4xx handling
        raise ServiceException(status=response.status_code, reason=response.reason)
    
    # Remove raise_for_status() since we handle all error cases above
    msg = f"Unexpected status {response.status_code} from artifact file endpoint for artifact {artifact_id}"
    raise RuntimeError(msg)

Why This Fixes the E2E Tests:

1. All requests exceptions are now caught and converted to ServiceException
2. ServiceException is in RETRYABLE_EXCEPTIONS, so transient failures will be retried
3. No more unhandled exceptions escaping to break the download flow

Verify Fix:

uv run pytest tests/aignostics/application/gui_test.py::test_gui_run_download -v
uv run pytest tests/aignostics/qupath/gui_test.py::test_gui_run_qupath_install_to_inspect -v

---

2. Missing Logging in URL Resolution

Location: src/aignostics/platform/resources/runs.py:351-414

Problem: No logging when URL resolution succeeds or fails, making debugging difficult.

Fix:

def get_artifact_download_url(self, artifact_id: str) -> str:
    """Get a presigned download URL for an artifact via the file endpoint."""
    endpoint_url = (
        f"{self._api.api_client.configuration.host.rstrip('/')}/v1/runs/{self.run_id}/artifacts/{artifact_id}/file"
    )
    logger.trace("Resolving download URL for artifact {} via {}", artifact_id, endpoint_url)  # ✅ ADD
    
    # ... _resolve() implementation ...
    
    result = Retrying(...)(resolve)
    logger.trace("Resolved download URL for artifact {}: {}", artifact_id, result[:100] + "...")  # ✅ ADD
    return result

---

⚠️ SUGGESTIONS (Non-Blocking)

3. Enhance Artifact Skipping Feedback

Location: src/aignostics/platform/resources/runs.py:441-449

Copilot's suggestion is good - improve user feedback when artifacts are skipped:

if not artifact.output_artifact_id:
    logger.error(  # Changed from warning
        "Skipping artifact %s for item %s, missing output_artifact_id",
        artifact.name,
        item.external_id,
    )
    if print_status:
        print(
            f"> Skipping artifact {artifact.name} for item {item.external_id}, missing output_artifact_id"
        )
    continue

Rationale: Missing output_artifact_id is an error condition, not just a warning.

---

4. Clarify Deprecated Fallback Path

Location: src/aignostics/application/_download.py:208-215

The fallback logic is correct, but could be clearer:

# Resolve artifact download URL via the new file endpoint
if get_artifact_download_url and artifact.output_artifact_id:
    artifact_url = get_artifact_download_url(application_run.run_id, artifact.output_artifact_id)
elif download_url := getattr(artifact, "download_url", None):
    # Deprecated fallback: Once download_url is removed from schema, this will be None
    artifact_url = download_url
    logger.warning(
        "Using deprecated download_url fallback for artifact {} - this will stop working when PAPI-4760 is complete",
        artifact.output_artifact_id or artifact.name
    )
else:
    logger.warning("No download URL available for artifact {}", artifact.output_artifact_id)
    continue

---

✅ POSITIVE FINDINGS

Test Coverage

• Excellent: All new tests have proper @pytest.mark.unit markers ✅
• Comprehensive: Tests cover URL resolution, fallback, retries, edge cases ✅
• Well-structured: Clear test names and assertions ✅

Test Files:

• tests/aignostics/application/download_test.py: 17 new unit tests
• tests/aignostics/platform/resources/runs_test.py: 6 new unit tests

Code Quality

• Good separation: URL resolution in runs.py, download logic in _download.py
• Proper retry logic: Uses Tenacity with exponential backoff
• Fallback path: Maintains backward compatibility during migration
• Type hints: Proper typing throughout

Architecture

• Clean callback pattern: get_artifact_download_url parameter allows flexible URL resolution
• Proper error propagation: Most exceptions are wrapped in SDK types (except the bug above)

---

📋 REVIEW CHECKLIST

Check	Status	Notes
Test Markers	✅	All new tests have @pytest.mark.unit
Test Coverage	✅	Comprehensive unit test coverage
Linting	🔄	Running...
Breaking Changes	⚠️	Gradual migration - download_url fallback preserves compatibility
Security	✅	Proper token handling, no secrets exposed
Documentation	✅	Clear docstrings and inline comments
Exception Handling	🔴	Critical issue - see #1 above
E2E Test Failures	🔴	Caused by exception handling bug - see #1

---

🔧 REQUIRED ACTIONS

1. FIX BLOCKING ISSUE chore(deps): Bump actions/upload-artifact from 4.6.1 to 4.6.2 #1: Add comprehensive exception handling in get_artifact_download_url()
2. VERIFY: Run E2E tests after fix:

   uv run pytest tests/aignostics/application/gui_test.py::test_gui_run_download -v
   uv run pytest tests/aignostics/qupath/gui_test.py::test_gui_run_qupath_install_to_inspect -v

3. IMPLEMENT: Add logging (Suggestion chore(deps): update actions/upload-artifact action to v4.6.2 #2)
4. CONSIDER: Suggestions chore(deps): update python docker tag to v3.13 - autoclosed #3 and Dependency Dashboard #4

---

📚 REFERENCES

• CLAUDE.md: Architecture compliance ✅
• Testing Requirements: 85% coverage maintained, proper markers ✅
• Copilot Review: Identified same exception handling issue
• SonarQube: Quality gate passed (with 6 new issues - review separately)

---

Recommendation: REQUEST CHANGES - Critical exception handling bug must be fixed before merge. The E2E test failures are not flaky - they're caused by unhandled requests exceptions escaping from get_artifact_download_url().

Once the blocking issue is fixed, this will be a solid migration that properly decouples the SDK from the deprecated download_url field.

---

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

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

[nahua-aignx]

Response to codecov/patch issue: The 23 uncovered lines in the GUI file are the bodies of _preview_tiff, _preview_csv, and _download_artifact — the async inner functions on the per-artifact buttons. Testing them properly requires NiceGUI's async test harness, mocking nicegui_run.io_bound, and simulating button clicks in the artifact expansion panels. That's significant test scaffolding for three functions that all follow identical structure (set loading → resolve URL → open → clear loading → notify on error). The e2e tests already exercise the critical start_download() path.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

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

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

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