-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add GitHub shorthand format for add command #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Support `skillport add owner/repo [paths...]` format: - `skillport add anthropics/skills` - add from repo root - `skillport add owner/repo skills` - add from specific path - `skillport add owner/repo skills examples` - multiple paths (single download) Refactor add.py for maintainability: - Extract shared `_prompt_namespace_selection()` helper - Extract shared `_display_add_result()` helper - Add `UserSkipped` exception for clean flow control - Reduce main `add()` function from ~180 to ~65 lines 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
5dbc59a to
0a9818d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for a GitHub shorthand format (owner/repo [paths...]) to the skillport add command, enabling simpler syntax and efficient multi-path downloads. The implementation includes a new authentication module with automatic token resolution from multiple sources (GH_TOKEN, GITHUB_TOKEN, gh CLI), refactored add command logic with improved error handling, and comprehensive documentation updates.
Key Changes
- New authentication module with fallback chain for GitHub token resolution (environment variables → gh CLI)
- GitHub shorthand format support (
owner/repo) with optional path arguments for single-download, multi-path skill installation - Refactored add command with extracted helper functions for namespace selection and result display, plus exception-based flow control via UserSkipped
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Version bump to 0.5.2 |
| src/skillport/shared/auth.py | New authentication module with token resolution fallback chain and context-aware error messages |
| src/skillport/shared/init.py | Export new auth module functions and types |
| src/skillport/modules/skills/public/update.py | Updated to use new auth module instead of direct os.getenv |
| src/skillport/modules/skills/internal/validation.py | Minor formatting improvement (added blank line) |
| src/skillport/modules/skills/internal/manager.py | Added GitHub shorthand parsing functions and updated resolve_source to support shorthand format |
| src/skillport/modules/skills/internal/github.py | Made get_default_branch public, updated functions to use TokenResult, added context-aware error message builders |
| src/skillport/modules/skills/internal/init.py | Export new GitHub shorthand functions and get_default_branch |
| src/skillport/interfaces/cli/commands/validate.py | Minor formatting improvements in JSON output |
| src/skillport/interfaces/cli/commands/add.py | Major refactor: added multi-path GitHub shorthand support, extracted helper functions, added UserSkipped exception for clean flow control |
| guide/configuration.md | Updated authentication documentation to describe fallback chain and recommend gh CLI |
| guide/cli.md | Added GitHub shorthand format examples and updated authentication documentation |
| README.md | Added GitHub shorthand examples throughout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from .validation import validate_skill_record | ||
|
|
||
| # GitHub shorthand pattern: owner/repo (no slashes in owner or repo) | ||
| GITHUB_SHORTHAND_RE = re.compile(r"^(?P<owner>[a-zA-Z0-9_-]+)/(?P<repo>[a-zA-Z0-9_.-]+)$") |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GitHub shorthand pattern regex may be overly restrictive. GitHub allows dots in repository names (e.g., owner/repo.name), but the current pattern only allows dots at the end via the character class. Consider testing with repository names that have dots in the middle to ensure they're properly supported. For example, owner/my.repo.name should be valid according to GitHub's naming rules.
| def _add_from_github_paths( | ||
| source: str, | ||
| paths: list[str], | ||
| *, | ||
| config: "Config", # noqa: F821 | ||
| force: bool, | ||
| yes: bool, | ||
| keep_structure: bool | None, | ||
| namespace: str | None, | ||
| ) -> "AddResult": # noqa: F821 | ||
| """Add skills from GitHub shorthand with multiple paths. | ||
| Downloads the repository once and adds skills from each specified path. | ||
| Raises: | ||
| UserSkipped: If user chooses to skip | ||
| """ | ||
| from skillport.modules.skills.public.types import AddResult, AddResultItem | ||
|
|
||
| parsed = parse_github_shorthand(source) | ||
| if not parsed: | ||
| return AddResult( | ||
| success=False, | ||
| skill_id="", | ||
| message=f"Invalid GitHub shorthand: {source}", | ||
| ) | ||
|
|
||
| owner, repo = parsed | ||
| base_url = f"https://github.com/{owner}/{repo}" | ||
|
|
||
| temp_dir: Path | None = None | ||
| try: | ||
| # Phase 1: Fetch tarball | ||
| with Progress( | ||
| SpinnerColumn(), | ||
| TextColumn("[progress.description]{task.description}"), | ||
| console=stderr_console, | ||
| transient=True, | ||
| ) as progress: | ||
| progress.add_task(f"Fetching {base_url}...", total=None) | ||
| fetch_result = fetch_github_source_with_info(base_url) | ||
| temp_dir = fetch_result.extracted_path | ||
| commit_sha = fetch_result.commit_sha | ||
|
|
||
| default_branch = get_default_branch(owner, repo) | ||
|
|
||
| # Phase 2: Collect paths and detect skills | ||
| path_infos: list[tuple[str, str, Path]] = [] | ||
| all_skill_names: list[str] = [] | ||
| invalid_paths: list[tuple[str, str]] = [] | ||
|
|
||
| for path in paths: | ||
| path = path.strip("/") | ||
| path_url = f"https://github.com/{owner}/{repo}/tree/{default_branch}/{path}" | ||
| path_dir = temp_dir / path | ||
|
|
||
| if not path_dir.exists(): | ||
| invalid_paths.append((path, f"Path not found in repository: {path}")) | ||
| continue | ||
|
|
||
| path_infos.append((path, path_url, path_dir)) | ||
| skills = detect_skills(path_dir) | ||
| all_skill_names.extend([s.name for s in skills]) | ||
|
|
||
| # Phase 3: Interactive prompt (raises UserSkipped if user skips) | ||
| if path_infos and all_skill_names: | ||
| keep_structure, namespace = _prompt_namespace_selection( | ||
| all_skill_names, | ||
| source, | ||
| yes=yes, | ||
| keep_structure=keep_structure, | ||
| namespace=namespace, | ||
| ) | ||
|
|
||
| # Phase 4: Add skills | ||
| all_added: list[str] = [] | ||
| all_skipped: list[str] = [] | ||
| all_details: list[AddResultItem] = [] | ||
| messages: list[str] = [] | ||
|
|
||
| for path, msg in invalid_paths: | ||
| all_skipped.append(path) | ||
| all_details.append(AddResultItem(skill_id=path, success=False, message=msg)) | ||
|
|
||
| for _path, path_url, path_dir in path_infos: | ||
| result = add_skill( | ||
| path_url, | ||
| config=config, | ||
| force=force, | ||
| keep_structure=keep_structure, | ||
| namespace=namespace, | ||
| pre_fetched_dir=path_dir, | ||
| pre_fetched_commit_sha=commit_sha, | ||
| ) | ||
| all_added.extend(result.added) | ||
| all_skipped.extend(result.skipped) | ||
| all_details.extend(result.details) | ||
| if result.message and result.message not in messages: | ||
| messages.append(result.message) | ||
|
|
||
| return AddResult( | ||
| success=len(all_added) > 0, | ||
| skill_id=all_added[0] if all_added else "", | ||
| message="; ".join(messages) if messages else "", | ||
| added=all_added, | ||
| skipped=all_skipped, | ||
| details=all_details, | ||
| ) | ||
|
|
||
| finally: | ||
| if temp_dir and temp_dir.exists(): | ||
| shutil.rmtree(temp_dir, ignore_errors=True) | ||
|
|
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The multi-path GitHub shorthand feature (_add_from_github_paths function and the routing logic in the add command) lacks test coverage. Tests should verify:
- Single path with shorthand works correctly
- Multiple paths download the repository only once
- Invalid paths are properly handled and reported
- Interactive namespace selection works for multi-path scenarios
- The UserSkipped exception flow works correctly
- Error handling when the repository or paths don't exist
| temp_dir = fetch_result.extracted_path | ||
| commit_sha = fetch_result.commit_sha | ||
|
|
||
| default_branch = get_default_branch(owner, repo) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An extra API call to get_default_branch is made here after already fetching the tarball. The fetch_github_source_with_info function internally calls parse_github_url with resolve_default_branch=True, which already makes an API call to get the default branch. Consider:
- Adding the resolved branch/ref to GitHubFetchResult
- Returning it from fetch_github_source_with_info
- Reusing that value here instead of calling get_default_branch again
This would reduce API calls and improve performance, especially important for rate-limited scenarios.
| default_branch = get_default_branch(owner, repo) | |
| # Prefer the ref/branch resolved during fetch (if available) to avoid | |
| # an extra API call; fall back to get_default_branch for compatibility. | |
| default_branch = getattr(fetch_result, "ref", None) or get_default_branch(owner, repo) |
| if _is_github_shorthand_source(source) and paths: | ||
| result = _add_from_github_paths( | ||
| source, | ||
| paths, | ||
| config=config, | ||
| force=force, | ||
| yes=yes, | ||
| keep_structure=keep_structure, | ||
| namespace=namespace, | ||
| ) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using the GitHub shorthand with paths feature (Route 1), the --name parameter is silently ignored without warning the user. This could be confusing since --name is a valid command-line option but has no effect in this scenario. Consider adding validation to either:
- Raise an error if --name is provided with multiple paths (since --name only works for single skill adds)
- Display a warning that --name is being ignored
- Document in the command help that --name is incompatible with the paths argument
| else: | ||
| # Route 2: Standard flow (URL, local, builtin, shorthand without paths) | ||
| if _is_external_source(source) and keep_structure is None and namespace is None: | ||
| skill_names, _source_name, temp_dir, commit_sha = _detect_skills_from_source(source) | ||
| keep_structure, namespace = _prompt_namespace_selection( | ||
| skill_names, | ||
| source, | ||
| yes=yes, | ||
| keep_structure=keep_structure, | ||
| namespace=namespace, | ||
| ) | ||
|
|
||
| console.print(f"\n[bold]Found {len(skill_names)} skill(s):[/bold] {skill_display}") | ||
| console.print("[bold]Where to add?[/bold]") | ||
| if is_single: | ||
| console.print(f" [info][1][/info] Flat → skills/{skill_names[0]}/") | ||
| console.print( | ||
| f" [info][2][/info] Namespace → skills/[dim]<ns>[/dim]/{skill_names[0]}/ " | ||
| "[warning](Claude Code incompatible)[/warning]" | ||
| ) | ||
| else: | ||
| console.print( | ||
| f" [info][1][/info] Flat → skills/{skill_names[0]}/, skills/{skill_names[1]}/, ..." | ||
| ) | ||
| console.print( | ||
| f" [info][2][/info] Namespace → skills/[dim]<ns>[/dim]/{skill_names[0]}/, ... " | ||
| "[warning](Claude Code incompatible)[/warning]" | ||
| ) | ||
| console.print(" [info][3][/info] Skip") | ||
| choice = Prompt.ask("Choice", choices=["1", "2", "3"], default="1") | ||
|
|
||
| if choice == "3": | ||
| print_warning("Skipped") | ||
| raise typer.Exit(code=0) | ||
| if choice == "1": | ||
| keep_structure = False | ||
| if choice == "2": | ||
| keep_structure = True | ||
| namespace = Prompt.ask("Namespace", default=_get_default_namespace(source)) | ||
|
|
||
| config = get_config(ctx) | ||
| result = add_skill( | ||
| source, | ||
| config=config, | ||
| force=force, | ||
| keep_structure=keep_structure, | ||
| namespace=namespace, | ||
| name=name, | ||
| pre_fetched_dir=temp_dir, | ||
| pre_fetched_commit_sha=commit_sha, | ||
| ) | ||
| result = add_skill( | ||
| source, | ||
| config=config, | ||
| force=force, | ||
| keep_structure=keep_structure, | ||
| namespace=namespace, | ||
| name=name, | ||
| pre_fetched_dir=temp_dir, | ||
| pre_fetched_commit_sha=commit_sha, | ||
| ) |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When paths are provided but the source is not GitHub shorthand (e.g., a full GitHub URL, local path, or builtin), the paths argument is silently ignored without any warning to the user. This can be confusing since users may not understand why their paths argument has no effect. Consider adding validation to either:
- Raise an error if paths are provided with non-shorthand sources
- Display a warning that paths are only supported with GitHub shorthand format
- Add documentation to the paths parameter help text clarifying it only works with GitHub shorthand
Summary
owner/repo [paths...]shorthand format forskillport addcommandChanges
is_github_shorthand()andparse_github_shorthand()in manager.pyresolve_source()to handle shorthand format (local path priority)get_default_branch()public in github.py_add_from_github_paths()for multi-path handling_prompt_namespace_selection()and_display_add_result()shared helpersUserSkippedexception for clean flow controlTest plan
skillport add anthropics/skills skills- single pathskillport add anthropics/skills skills examples- multiple pathsskillport add gotalab/skillport- repo root🤖 Generated with Claude Code