-
Notifications
You must be signed in to change notification settings - Fork 15
feat: add skill update command #26
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
- Implement `skillport update` CLI command for updating skills from origin - Add origin tracking with TypedDict (type, url, ref, tree_sha) - Support GitHub origin updates with tree SHA comparison - Configure ruff with F, E, W, I (isort), UP (pyupgrade) rules - Fix circular imports by using deep module imports - Modernize type hints (List -> list, Optional -> |) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
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 a comprehensive skill update system with a new skillport update CLI command that enables updating skills from their original sources (GitHub, local). The implementation tracks origin metadata in .origin.json v2 format with content hashes for efficient change detection and uses the GitHub Tree API to check for updates without downloading tarballs.
Key changes include:
- New update command with
--all,--dry-run,--force, and--checkoptions - Origin v2 tracking with content hashes, commit SHAs, and update history
- Tree-based change detection for GitHub sources to avoid unnecessary downloads
Reviewed changes
Copilot reviewed 65 out of 66 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
src/skillport/modules/skills/public/update.py |
Core update logic with local/GitHub source handlers and change detection |
src/skillport/modules/skills/internal/origin.py |
Origin v2 format with content hash computation and migration from v1 |
src/skillport/modules/skills/internal/github.py |
Enhanced GitHub integration with tree API, commit SHA extraction, and remote hash computation |
src/skillport/modules/skills/public/types.py |
TypedDict definitions for origin types and UpdateResult models |
src/skillport/interfaces/cli/commands/update.py |
CLI command implementation with interactive update flow and JSON output support |
src/skillport/modules/skills/public/add.py |
Enhanced to record content_hash and commit_sha during skill installation |
src/skillport/modules/indexing/public/index.py |
Integrated orphan origin pruning into index build process |
tests/unit/test_update.py |
Comprehensive test coverage for update functionality (404 lines) |
tests/unit/test_origin_v2.py |
Test coverage for origin v2 migration and operations (314 lines) |
pyproject.toml |
Configured ruff with F, E, W, I (isort), UP (pyupgrade) rules |
| Multiple test files | Import statement reordering per isort configuration |
| Multiple source files | Type hint modernization (List → list, Optional → X |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def compute_content_hash_with_reason(skill_path: Path) -> tuple[str, str | None]: | ||
| """Compute directory content hash with safeguards. | ||
| Hash = sha256( join(relpath + NUL + file_sha1 + NUL) for files sorted by relpath ) | ||
| file_sha1 is sha1 over file bytes (matches Git blob hash). | ||
Copilot
AI
Dec 6, 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 comment references "Git blob format" and Git's SHA1 algorithm, but this creates SHA256 hashes. While individual file hashes use SHA1 (matching Git), the aggregate hash is SHA256. Consider clarifying the comment to explain: "Uses Git blob SHA1 format for individual files (sha1('blob ' + length + '\0' + contents)), then combines them into an aggregate SHA256 hash."
| # Union type for any origin | ||
| Origin = OriginKind | OriginLocal | OriginGitHub |
Copilot
AI
Dec 6, 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.
In the Origin union type definition, the order matters for type checking. Since OriginKind has total=False (all fields optional), it will match any dict, making the more specific types (OriginLocal, OriginGitHub) unreachable. Consider reordering as OriginGitHub | OriginLocal | OriginKind to check specific types first.
| for p in files: | ||
| rel = p.relative_to(skill_path) | ||
| try: | ||
| data = p.read_bytes() | ||
| except OSError: | ||
| return "", "unreadable" | ||
| # Use Git blob format: sha1("blob " + length + "\0" + contents) | ||
| # This matches the SHA returned by GitHub's tree API | ||
| blob_header = f"blob {len(data)}\x00".encode() | ||
| blob_sha = hashlib.sha1(blob_header + data).hexdigest() | ||
| hasher.update(str(rel).encode("utf-8")) | ||
| hasher.update(b"\x00") | ||
| hasher.update(blob_sha.encode("utf-8")) | ||
| hasher.update(b"\x00") | ||
|
|
Copilot
AI
Dec 6, 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.
[nitpick] The hash computation uses git blob format with SHA1, but doesn't handle files that may be Git LFS pointers. If a repository uses Git LFS, the hash will be computed over the pointer file content (typically ~120 bytes) rather than the actual large file. Consider documenting this limitation or adding a check for LFS pointer files (they start with "version https://git-lfs.github.com/spec/").
| try: | ||
| tree = _fetch_tree(parsed, token) | ||
| except Exception: | ||
| return "" |
Copilot
AI
Dec 6, 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 API call catches all exceptions and returns empty string, which loses valuable debugging information. Consider logging the exception before returning empty string, especially for authentication failures (401/403) vs. other errors. This would help users diagnose issues with GITHUB_TOKEN.
| effective_keep_structure = ( | ||
| False if effective_keep_structure is None else effective_keep_structure | ||
| ) | ||
| # 単一スキル: path をスキル名で固定 |
Copilot
AI
Dec 6, 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.
Another Japanese comment: "単一スキル: path をスキル名で固定" should be translated to English ("Single skill: fix path with skill name").
| source_path = renamed | ||
| temp_dir = renamed | ||
| skills = detect_skills(source_path) | ||
| # 単一スキルの場合は origin.path をスキル名で確定させる |
Copilot
AI
Dec 6, 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.
Similarly, the comment "単一スキルの場合は origin.path をスキル名で確定させる" should be translated to English. It means "For single skill case, fix origin.path with skill name".
| prefix = origin_payload.get("path", "").rstrip("/") | ||
| if prefix and rel_path and rel_path != prefix and not prefix.endswith(f"/{rel_path}"): | ||
| enriched_payload["path"] = f"{prefix}/{rel_path}" |
Copilot
AI
Dec 6, 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.
[nitpick] The function attempts to narrow the path by trying f"{prefix}/{rel_path}" without first checking if this causes the path to exceed GitHub's path length limits (typically 4096 characters). While unlikely in practice, this could cause issues with deeply nested directory structures. Consider adding a length check or documenting the assumption.
| resp = requests.get(url, headers=headers, timeout=10) | ||
| if resp.ok: | ||
| return resp.json().get("sha", "")[:40] # Full SHA | ||
| except Exception: |
Copilot
AI
Dec 6, 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.
'except' clause does nothing but pass and there is no explanatory comment.
| remote_hash = alt_hash | ||
| try: | ||
| update_origin(skill_id, {"path": candidate}, config=config) | ||
| except Exception: |
Copilot
AI
Dec 6, 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.
'except' clause does nothing but pass and there is no explanatory comment.
| try: | ||
| rel = source_path.relative_to(source_base) | ||
| update_origin(skill_id, {"path": str(rel)}, config=config) | ||
| except Exception: |
Copilot
AI
Dec 6, 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.
'except' clause does nothing but pass and there is no explanatory comment.
Summary
skillport updateCLI command for updating skills from their origin (GitHub).origin.jsonChanges
skillport update [SKILL_ID] [--all] [--dry-run] [--force]Test plan
test_update.py)test_origin_v2.py)Follow-up
🤖 Generated with Claude Code