fix(skills): treat bare github.com/<owner>/<repo> URLs as GitHubRepo#276
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes /skill install https://github.com/<owner>/<repo> being mis-parsed as a direct URL (downloading the HTML repo page, then failing with invalid gzip header) by recognizing bare GitHub repo URLs and routing them to the GitHubRepo install source.
Changes:
- Route bare
github.com/<owner>/<repo>[.git]URLs (with optionalwww., optional trailing slash) toInstallSource::GitHubRepo. - Add regression tests covering the new GitHub URL parsing behavior and ensuring archive/blob/tree URLs remain
DirectUrl. - Document the fix in the Unreleased changelog.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/tui/src/skills/install.rs | Adds GitHub browser URL detection in InstallSource::parse plus regression tests. |
| CHANGELOG.md | Notes the install fix for issue #269 under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+125
to
+147
| fn parse_github_browser_url(url: &str) -> Option<String> { | ||
| let after_scheme = url | ||
| .strip_prefix("https://") | ||
| .or_else(|| url.strip_prefix("http://"))?; | ||
| let (host, rest) = after_scheme.split_once('/')?; | ||
| if !host.eq_ignore_ascii_case("github.com") && !host.eq_ignore_ascii_case("www.github.com") { | ||
| return None; | ||
| } | ||
| let trimmed = rest.trim_end_matches('/'); | ||
| let mut parts = trimmed.splitn(3, '/'); | ||
| let owner = parts.next()?.trim(); | ||
| let repo = parts.next()?.trim().trim_end_matches(".git"); | ||
| if owner.is_empty() || repo.is_empty() { | ||
| return None; | ||
| } | ||
| // If there is a third segment, the URL points at a sub-resource | ||
| // (`/archive/...`, `/blob/...`, `/tree/...`). Treat that as a real direct | ||
| // URL — the user explicitly wants whatever lives at that path. | ||
| if parts.next().is_some() { | ||
| return None; | ||
| } | ||
| Some(format!("{owner}/{repo}")) | ||
| } |
| /// | ||
| /// * `github:owner/repo` → [`InstallSource::GitHubRepo`] | ||
| /// * `http://` or `https://` prefix → [`InstallSource::DirectUrl`] | ||
| /// * `https://github.com/owner/repo[.git]` (no path past the repo) → |
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses issue #269 by improving the parsing of GitHub URLs during skill installation. Previously, bare repository URLs were treated as direct URLs, causing the installer to download HTML pages and fail with gzip errors. The changes introduce logic to detect these bare URLs and route them to the GitHubRepo source, while preserving direct URL behavior for specific paths like archives or blobs. Unit tests were added to ensure correct routing for various URL formats. I have no feedback to provide.
Closes #269. `/skill install https://github.com/obra/superpowers` failed on every platform with `invalid gzip header`. Root cause: `InstallSource::parse` matched any `https://`-prefixed spec as `DirectUrl`, so the installer downloaded the HTML repo page (200 OK, `text/html`) and tried to gzip-decode HTML. The user reported it from Win11 + PowerShell but the parse path is platform-independent. Recognize bare GitHub repo URLs in `InstallSource::parse`: - `https://github.com/<owner>/<repo>` - `https://github.com/<owner>/<repo>/` - `https://github.com/<owner>/<repo>.git` - `https://github.com/<owner>/<repo>.git/` - `https://www.github.com/<owner>/<repo>` - `http://github.com/<owner>/<repo>` (legacy) …all route to the existing `GitHubRepo` source, which already produces `https://github.com/<repo>/archive/refs/heads/{main,master}.tar.gz` candidates with proper fallback. URLs with a third path segment (`/archive/...`, `/blob/...`, `/tree/...`) keep going through `DirectUrl` because the user picked that exact path. Adds two regression tests: one asserting the seven recognised forms all canonicalize to `github:obra/superpowers`, and one pinning the sub-resource paths to `DirectUrl`.
fa63a1c to
0ca1238
Compare
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
Closes #269 (`安装不了 superpowers` / "invalid gzip header" reported by @bigfoot88).
`/skill install https://github.com/obra/superpowers\` failed with `invalid gzip header` because `InstallSource::parse` routed any `https://`-prefixed spec to `DirectUrl`. The installer downloaded the HTML repo page (200 OK, `text/html`) and `GzDecoder` choked.
The platform was a red herring: the user reported Win11 + PowerShell + 0.8.1 but the parse path is platform-independent. The macOS happy path was confirmed working pre-fix on `https://github.com/obra/superpowers/archive/refs/heads/main.tar.gz\` and post-fix on `https://github.com/obra/superpowers\`.
Test plan
🤖 Generated with Claude Code