fix(skills): retry ClawHub ZIP download HTTP errors - #57714
Open
VectorPeak wants to merge 1 commit into
Open
Conversation
Contributor
|
Thanks for the focused regression fix. Current main declares a three-attempt ZIP download loop at Automated hermes-sweeper review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This fixes a narrow ClawHub ZIP
/downloadretry gap inClawHubSource._download_zip(): the function already had a bounded retry loop, buthttpx.HTTPErrorexceptions raised while issuing the ZIP request, such as a temporaryConnectTimeout, returned an empty file map on the first attempt.What Problem This Solves
ClawHubSource.fetch()resolves a ClawHub skill version, then tries the/downloadZIP endpoint first because that path can return the complete skill bundle in one response. If the ZIP path does not produceSKILL.md,fetch()can still fall back to the version metadata/raw-content path.The retry loop in
_download_zip()already declaresmax_retries = 3, but the previous retry behavior was uneven:429used the retry loop.httpx.ConnectTimeout, skipped the remaining attempts and returned{}immediately.That meant a short network interruption before a usable
/downloadresponse was received could make the primary ZIP path look unavailable after one failed request, even when the next attempt would have succeeded:This is a robustness fix, not a security fix. It does not claim that every ClawHub download failure is recoverable; it only retries
httpx.HTTPErrorexceptions raised while requesting the ZIP bundle.Changes
httpx.HTTPErrorfailures insideClawHubSource._download_zip()while attempts remain in the existingmax_retries = 3loop._download_zip()still logs and returns{}instead of raising, soClawHubSource.fetch()can continue to the metadata/raw-content fallback.429handling, non-200 fail-fast behavior, invalid ZIP handling, unsafe ZIP member filtering, large-file skipping, non-UTF-8 skipping, and text decoding behavior are not changed.The main tradeoff is timing: if the ZIP endpoint is persistently unreachable, the metadata/raw-content fallback is reached after the retry delay rather than immediately. That keeps the fallback intact while allowing short-lived transport failures to recover.
Evidence
The new regression test covers the exact branch that previously returned too early. It simulates one transient ZIP request exception followed by a valid ZIP response:
Before this change, the first
httpx.ConnectTimeoutwas caught byexcept httpx.HTTPError,_download_zip()returned{}, and the second mocked response was never requested.After this change, the same path consumes one retry attempt, requests the ZIP again, extracts the text file, and returns:
{"SKILL.md": "# Skill"}The test also asserts that
httpx.getwas called twice and that the retry delay was invoked once. That proves the successful result came from the retry path, not from the metadata/raw-content fallback.Possible call chain / impact
The affected path is limited to ClawHub ZIP bundle downloads. This PR does not change GitHub-backed skills, direct URL skills, official optional skills, ClawHub catalog search, ClawHub metadata parsing, the raw-content fallback itself, ZIP member path validation, quarantine/install behavior, or any tool schema.
I also checked nearby PRs before opening this:
httpx.HTTPErrorfailures./downloadrequest exceptions.Related Issue
No linked issue. Duplicate search performed for ClawHub
_download_zip, ZIP download, and HTTPError retry behavior.Type of Change
Changes Made
tools/skills_hub.pyhttpx.HTTPErrorinsideClawHubSource._download_zip()until the existing retry budget is exhausted.tests/tools/test_skills_hub_clawhub.pyhttpx.ConnectTimeoutis retried and the next valid ZIP response is extracted.How to Test
Observed locally:
Observed locally:
Observed locally: passed with no output.
This did not complete because the local environment was missing test-only optional dependencies such as
aiohttpduring collection.I then retried with the practical extras needed for the gateway tests:
That cleared the
aiohttpimport errors but still hit a Windows collection issue intests/tools/test_search_hidden_dirs.py, where the test calls Unixwhich rgdirectly. After adding a temporary localwhich.exeshim for this process, the full run proceeded further but did not return a final pytest summary before the 30-minute local timeout. Because I do not have a completed passing full-suite result, I left thepytest tests/ -qchecklist item unchecked.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passuv runDocumentation & Housekeeping
docs/, docstrings) - N/Acli-config.yaml.exampleif I added/changed config keys - N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows - N/AScreenshots / Logs
Full-suite local attempts did not produce a passing final summary: