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%> (+52.79%)	⬆️
src/aignostics/application/_service.py	61.32% <100.00%> (+6.94%)	⬆️
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 6 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 379exceptrequests.Timeoutase:
raiseServiceException(...) frome# ✅ Handledexceptrequests.ConnectionErrorase:
raiseServiceException(...) frome# ✅ 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,
)
exceptrequests.Timeoutase:
raiseServiceException(status=HTTPStatus.SERVICE_UNAVAILABLE, reason="Request timed out") fromeexceptrequests.ConnectionErrorase:
raiseServiceException(status=HTTPStatus.SERVICE_UNAVAILABLE, reason="Connection failed") fromeexceptrequests.RequestExceptionase: # ✅ ADD THIS - Catch all other requests exceptionsraiseServiceException(
status=HTTPStatus.SERVICE_UNAVAILABLE, reason=f"Request failed: {e}"
) frome# Status code handlingifresponse.status_codein {
HTTPStatus.MOVED_PERMANENTLY,
HTTPStatus.FOUND,
HTTPStatus.TEMPORARY_REDIRECT,
HTTPStatus.PERMANENT_REDIRECT,
}:
location=response.headers.get("Location")
ifnotlocation:
msg=f"Redirect response {response.status_code} missing Location header for artifact {artifact_id}"raiseRuntimeError(msg)
returnlocationifresponse.status_code==HTTPStatus.NOT_FOUND:
raiseNotFoundException(status=HTTPStatus.NOT_FOUND, reason="Artifact not found")
ifresponse.status_code>=HTTPStatus.INTERNAL_SERVER_ERROR:
raiseServiceException(status=response.status_code, reason=response.reason)
# Handle 4xx errors (other than 404)ifresponse.status_code>=400: # ✅ ADD THIS - Explicit 4xx handlingraiseServiceException(status=response.status_code, reason=response.reason)
# Remove raise_for_status() since we handle all error cases abovemsg=f"Unexpected status {response.status_code} from artifact file endpoint for artifact {artifact_id}"raiseRuntimeError(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:

defget_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] +"...") # ✅ ADDreturnresult

---

⚠️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:

ifnotartifact.output_artifact_id:
logger.error( # Changed from warning"Skipping artifact %s for item %s, missing output_artifact_id",
artifact.name,
item.external_id,
)
ifprint_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 endpointifget_artifact_download_urlandartifact.output_artifact_id:
artifact_url=get_artifact_download_url(application_run.run_id, artifact.output_artifact_id)
elifdownload_url:=getattr(artifact, "download_url", None):
# Deprecated fallback: Once download_url is removed from schema, this will be Noneartifact_url=download_urllogger.warning(
"Using deprecated download_url fallback for artifact {} - this will stop working when PAPI-4760 is complete",
artifact.output_artifact_idorartifact.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.

[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.

[Copilot]

Pull request overview

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

[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