Repository navigation
fix(skills): fail fast and name the reason when a skill fetch is refused - #6508
Merged
senamakel merged 1 commit intoSep 23, 2026
Merged
Conversation
Installing a BUILT-IN skill fetches its SKILL.md live from raw.githubusercontent.com. When that host is slow or throttling, the install sat for a full 60 seconds behind a disabled button and then failed with a string the UI could not act on (tinyhumansai#6409). Two changes, both narrow. Distinguish a throttled host from an unreachable one. Every non-2xx previously produced the same `fetch failed: <url> returned status N`, so "come back shortly" and "this host is not answering" were the same message. A 429 — or a 5xx that carries Retry-After — now reports as rate limiting and includes the host's own delay when it sends a parseable one. `RATE_LIMITED_ERROR_PREFIX` is exported so a caller can branch on the kind without parsing a status code back out of prose. The header is read before anything consumes the response body, because `response.bytes()` takes `response` by value and a later read is not merely wrong but impossible. The same ordering silently drops the header on the model-call path (tinyhumansai#6413, found independently); the comment notes the parallel rather than inventing a shared helper for two call sites in different crates. Retry-After's HTTP-date form is deliberately not parsed: it would pull in a date parser to serve a form GitHub's raw CDN does not send, and the caller degrades correctly without it — the error still says the host is throttling, just without a specific delay. Cut the catalog install timeout from 60s to 15s, matching `download::PROBE_TIMEOUT_SECS`, which bounds requests to the same hosts in the same flow. `DEFAULT_INSTALL_TIMEOUT_SECS` is left at 60 — that is the documented default for the public `skills.install_from_url` tool and a scripted fetch is not a user waiting on a button. Worth recording for whoever reads the issue: a 429 returns immediately, so the reported 60-second hang was a transport timeout, not the 429 path. QA saw the 429 separately via curl. The timeout cut addresses the logged symptom; Retry-After addresses the throttling they observed. The three new tests assert the KIND of failure, not that install fails — the latter passes on the code this replaces. Reverting the branch fails both 429 cases with `returned status 429` while the 404 case still passes, so the branch discriminates rather than catching every non-2xx. They live in their own file because the existing one reached the 750-line layout cap. Addresses tinyhumansai#6409 acceptance criteria 3 and 4. Criterion 1 — no dependency on raw.githubusercontent.com at install time — is NOT addressed here and is filed as tinyhumansai#6505; tinyhumansai#6409 stays open. Refs tinyhumansai#6409
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 0 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Reviewing pending checks Review snapshot
Completeness: Complete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. FindingsNo active actionable findings. Pending checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS), Rust Feature-Gate Smoke (gates off) Before merge
How this fits togetherflowchart LR
n0["install_from_catalog<br/>changed"]:::changed
n1["url_validation<br/>changed"]:::changed
n2["install_workflow_from_url<br/>changed"]:::changed
n3["redact_url<br/>changed"]:::changed
n4["make_legacy_skill<br/>changed"]:::changed
n5["read_workflow_resource"]:::impacted
n6["install_workflow_from_url_with_home"]:::impacted
n7["format"]:::impacted
n8["CatalogEntry"]:::impacted
n9["execute"]:::impacted
n10["...l_is_idempotent_when_skill_already_exists"]:::impacted
n0 -->|calls| n2
n0 -->|calls| n7
n0 -->|uses| n8
n2 -->|calls| n6
n3 -->|calls| n7
n4 -->|calls| n7
n5 -->|calls| n7
n6 -->|uses| n1
n6 -->|calls| n3
n6 -->|calls| n7
n9 -->|calls| n0
n9 -->|calls| n7
n10 -->|calls| n6
n10 -->|tests| n6
n10 -->|calls| n7
n10 -->|tests| n7
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe install path now uses a 15-second catalog timeout. Fetch handling classifies HTTP 429 and eligible 5xx responses as rate-limited, includes parsed retry delays, and preserves status errors for other failures. Tests cover these cases. ChangesInstall fetch reliability
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Rate-limited and ordinary fetch failures retain the intended behavior, with no actionable merge-blocking issue identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation PR Resolution Implement the
A rabbit found a timeout bright, Comment ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0171 · 298,061 in / 5,061 out · 13,984 cached (5%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 779 embedded
critique: $0.0053 · 123,119 in / 1,526 out · 8,423 cached (7%) · gpt-5.6-luna
security: $0.0065 · 119,271 in / 1,639 out · 5,561 cached (5%) · gpt-5.6-luna
tests: $0.0016 · 17,935 in / 100 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0009 · 10,437 in / 68 out · 0 cached (0%) · deepseek/deepseek-v4-flash
e2e: $0.0020 · 21,971 in / 91 out · 0 cached (0%) · deepseek/deepseek-v4-flash
tinysweeper
Bot
added
the
priority: p3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
Summary
Retry-After.Problem
Every non-2xx produced the same string:
So "this host is rate limiting you, wait a moment" and "this host is not answering" reached the user identically, and
Retry-Afterwas never read at all —rg "429|Retry-After|retry_after"acrossskills/ops_install/andskills/catalog/returned nothing.Meanwhile the catalogue install path passed
timeout_secs: Some(60)whiledownload.rsbounds its own probes to the same hosts atPROBE_TIMEOUT_SECS = 15.One correction to the issue's reproduction, from the code: a 429 is a response and returns immediately. The 60-second failure in QA's log was therefore a transport timeout, not the 429 path — they observed the 429 separately via
curl. Both are real; they are different failures. The timeout cut addresses the logged symptom, theRetry-Afterwork addresses the throttling.Solution
Kind, not just status. A 429 — or a 5xx carrying
Retry-After— now reports as rate limiting with the host's own delay when it sends a parseable one.RATE_LIMITED_ERROR_PREFIXis exported so a caller can branch on the kind rather than parsing a status code back out of prose.Header read before the body.
response.bytes()takesresponseby value, so a later header read is not merely wrong — it does not compile. The same ordering silently drops the header on the model-call path (#6413, found independently by another worker within the hour). The comment notes that parallel rather than inventing a shared helper for two call sites in different crates and different loops.Retry-After's HTTP-date form is deliberately not parsed. It would pull in a date parser to serve a form GitHub's raw CDN does not send, and the caller degrades correctly without it — the error still says the host is throttling, just without a specific delay.Timeout scoped to the catalogue path only.
CATALOG_INSTALL_TIMEOUT_SECS = 15matchesdownload::PROBE_TIMEOUT_SECS, with a comment saying why it matches so the next person tuning one knows the other exists.DEFAULT_INSTALL_TIMEOUT_SECSstays at 60 — that is the documented default for the publicskills.install_from_urltool, and a scripted fetch is not a user waiting on a button.Tests
Three cases, asserting the kind of failure rather than that install fails — the latter passes on the code this replaces:
Retry-After: 4242sRetry-AfterThe 404 case exists because the rate-limit branch could otherwise widen to every non-2xx and silently lose the distinction again.
Revert-proved. Removing the branch fails both 429 cases with the pre-fix string verbatim, while the 404 case keeps passing:
They live in a new
ops_install_failure_tests.rsbecauseops_create_and_url_tests.rshit the 750-line layout cap at 777. Extracted rather than trimmed, per the gate's own guidance, and both files land with headroom (663 / 124) rather than at the limit.Scope — #6409 stays open
Retry-AfterOn criterion 2:
SkillsExplorerTab.handleRegistryInstallalready set an installing state and toasted the error with its reason, wired viapages/Skills.tsx, shipped in #3457 on 2026-06-06. The issue's "no UI feedback" line describes the 60-second wait before the failure surfaced, not missing UI. Corrected on the issue so nobody rewrites working code.Criterion 1 is not a repair: the skill catalogue is remotely aggregated (
skills/catalog/ops.rs:4), sosource: "built-in"means built in to HermesHub, not ships with OpenHuman. Nothing about that path was ever local. Bundling or proxying is new capability needing decisions on which skills ship, how they update, and the licensing position on redistributing third-party content — filed as #6505 with those questions as its first acceptance criterion.Impact
skills.install_from_urlcallers.Related
Retry-Aftergap on the model-call path, found independently; deliberately not abstracted overSubmission Checklist
Some/NoneRetry-Afterarms and the non-rate-limited fallback each have a dedicated test. Full-suitediff-coveris CI's authority.## Related— N/A: no matrix row covers the skill install fetch.wiremockhost.docs/RELEASE-MANUAL-SMOKE.mdhas no skill-install step.Closes #NNN— deliberately not used;Refs #6409only, because criterion 1 remains open. See Scope.AI Authored PR Metadata
Linear Issue
Commit & Branch
fix/skills-install-fail-fast59ab60073f46c5c7de0186d41271b51e04052b82Validation Run
pnpm --filter openhuman-app format:check— N/A: no frontend files changed.pnpm typecheck— N/A: no TypeScript changed.cargo fmt -p openhuman --checkclean; layout gate passed; both CI clippy invocations (--features "$(product-features.sh)"and contributor default) clean with-D warnings.crates/openhuman-appuntouched.Validation Blocked
command:end-to-end install against a genuinely throttlingraw.githubusercontent.comerror:not reproducible on demand — the upstream 429 depends on region and rate-limit stateimpact:the 429 path is covered by a local wiremock host returning 429 with and withoutRetry-After, which exercises the same code. The real-world confirmation is Skills: BUILT-IN skills are fetched live from NousResearch GitHub raw at install; 60 s silent hang on 429, no UI feedback #6409's "confirm fix in next build".Behavior Changes
Parity Contract
skills.install_from_urlkeeps its documented 60s default and 600s ceiling.Retry-After— and the 404 test pins that it does not widen.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests