[rtmalikian]

Summary

When the Zenodo API returns a non-200 status code, the error response body (which contains the actual explanation from Zenodo) was being discarded. The error message only showed the status code, making it impossible to diagnose issues like rate limiting (429), access denied (403), or invalid records (404).

What was broken

In _get_metadata_and_archive_url(), the error message was:

Cannot find record '15236131' on Zenodo (response.status_code=403).

This provided no information about why the request failed. Users had no way to distinguish between rate limiting, access restrictions, or invalid record IDs without manually visiting the URL in a browser.

What the fix does

1. Parses the response body — attempts JSON first (Zenodo returns structured error responses), falls back to raw text if the response isn't valid JSON.
2. Includes the error detail in the exception message — users now see the full context, e.g.:

   Cannot find record '15236131' on Zenodo (status_code=403). Response: {'message': 'Rate limit exceeded', 'status': 403}
   

3. Updated tests — the existing 404 test now verifies the response body is included, and a new test covers the non-JSON fallback path.

Fixes #169

Verification

All 13 tests pass:

tests/test_zenodo.py::test_get_metadata_and_archive_url_success PASSED
tests/test_zenodo.py::test_get_metadata_and_archive_url_failure PASSED
tests/test_zenodo.py::test_get_metadata_and_archive_url_failure_with_text_body PASSED
tests/test_zenodo.py::test_get_metadata_and_archive_url_connection_error PASSED
tests/test_zenodo.py::test_get_latest_version_folder_name PASSED
tests/test_zenodo.py::test_get_latest_version_folder_name_empty PASSED
tests/test_zenodo.py::test_get_latest_version_folder_name_ignores_empty_folder PASSED
tests/test_zenodo.py::test_fetch_downloads_new_if_no_local PASSED
tests/test_zenodo.py::test_fetch_zenodo_unreachable_raises PASSED
tests/test_zenodo.py::test_fetch_skips_if_latest_present PASSED
tests/test_zenodo.py::test_fetch_replaces_old_version PASSED
tests/test_zenodo.py::test_fetch_atlases_calls_fetch PASSED
tests/test_zenodo.py::test_fetch_synthstrip_calls_fetch PASSED

======================== 13 passed ========================

---

About the Author: Raphael Malikian — Clinical AI Solutions Architect. I specialise in building and fixing AI/ML systems for healthcare, including vector databases, RAG pipelines, and clinical NLP. If you need help with your project or think I can add value to your organisation, feel free to reach out — I'd love to connect.

📧 rtmalikian@gmail.com
🔗 GitHub: https://github.com/rtmalikian
🔗 LinkedIn: http://www.linkedin.com/in/raphael-t-malikian-mbbs-bsc-hons-71075436a

---

Disclosure: This code was developed with assistance from mimo-2.5-pro (Xiaomi) via Hermes Agent (Nous Research). All changes were reviewed, tested against the actual codebase, and verified for correctness.

[Copilot]

Pull request overview

This PR improves diagnosability of Zenodo download failures by surfacing Zenodo’s error response body in the raised ZenodoException, and extends test coverage to validate both JSON and plain-text error bodies.

Changes:

• Include Zenodo error response content (JSON or text fallback) in _get_metadata_and_archive_url() exception messages.
• Update the existing failure test to assert that JSON error details appear in the exception message.
• Add a new test covering the non-JSON (text body) fallback path.

Reviewed changes

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

File	Description
brainles_preprocessing/utils/zenodo.py	Adds parsing of Zenodo error bodies and includes them in the raised exception message.
tests/test_zenodo.py	Expands assertions for error messages and adds a text-body fallback test.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[rtmalikian]

Hi, I've addressed both Copilot review comments:

1. ** portability (line 144):** Changed to catch only — is a subclass of and is guaranteed to be caught, while itself may not exist in older versions.

2. Status-code-aware error messages (line 148): Changed from the generic "Cannot find record" to:

   • 404 → "Cannot find record" (record genuinely doesn't exist)
   • Other codes (403, 429, 500, etc.) → "Failed to fetch record" (more accurate for permission/rate-limit/server errors)

All 13 tests pass with the updated code. PR is ready for re-review. 🙏

[MarcelRosier]

looks solid, please fix the lint issues so we can approve and merge! Thanks for contributing @rtmalikian

[neuronflow]

@rtmalikian friendly reminder ;)

[neuronflow]

@rtmalikian friendly reminder ;)

friendly reminder ;)

[Copilot]

🟢 Approval recommended

No active review issues are reported for the changes that preserve Zenodo API error details.

No open issues 🎉

2 resolved since last review

• The except (ValueError, requests.exceptions.JSONDecodeError) is unsafe because…
• The error message currently says "Cannot find record" for all non-200 responses, but Zenodo can…

_{Review effort: Balanced}

[neuronflow]

@rtmalikian please apply linting and resolve the merge conflict ;)

[rtmalikian]

Thanks for the nudge! I looked into both items:

Linting: black (the version pinned in pyproject) would reformat one spot in brainles_preprocessing/utils/zenodo.py at the branch head — the parenthesised single f-string for the 404 prefix. tests/test_zenodo.py is already black-clean. I have not pushed that formatting fix on its own, for the reason below.

Merge conflict: I tried the merge against current main locally, and the only conflicting file is tests/test_zenodo.py — but the conflict is a symptom of something bigger. This branch still uses the old archive-URL download path (_get_metadata_and_archive_url, BytesIO, shutil.rmtree), while main has since moved to the files-list API (_get_metadata_and_files, request timeouts, staged download). More importantly, the thing this PR adds — including the Zenodo response body in error messages — is already on main: _get_metadata_and_files builds its error as ... (status_code=...). Response: {error_detail}, falling back to response.text when the body is not JSON, so both the JSON-error and the text-body cases are covered there.

So merging this branch as-is would regress main back to the archive-URL path, and resolving just the test conflict would still leave that. I cannot push a merge commit or rebase the branch from here (no force-push), so before I do anything I wanted to check with you: would you prefer this PR closed as superseded by the files-API rewrite, or would you like me to port just the two tests (JSON-error and text-body cases) onto the new _get_metadata_and_files method? Happy to do the latter if the extra coverage is wanted.

[neuronflow]

@MarcelRosier, how shall we move forward?