diff --git a/pyproject.toml b/pyproject.toml index a43423c..c808c98 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -64,8 +64,18 @@ skillport = "skillport.__main__:main" skillport-mcp = "skillport.__main__:main" # legacy alias [tool.ruff] +target-version = "py310" +line-length = 100 + exclude = [ ".agent/skills", - "test_lancedb_scores", - "test_verification_db", ] + +[tool.ruff.lint] +select = ["F", "E", "W", "I", "UP"] +ignore = [ + "E501", # line-too-long（formatterに任せる） +] + +[tool.ruff.lint.isort] +known-first-party = ["skillport"] diff --git a/src/skillport/__init__.py b/src/skillport/__init__.py index dc498b1..aecd6bd 100644 --- a/src/skillport/__init__.py +++ b/src/skillport/__init__.py @@ -1,32 +1,32 @@ """SkillPort package entry.""" -from skillport.shared.config import Config -from skillport.shared import exceptions from skillport.modules import ( - search_skills, - load_skill, - add_skill, - remove_skill, - list_skills, - read_skill_file, - validate_skill, - SkillSummary, - SkillDetail, - FileContent, - SearchResult, AddResult, - RemoveResult, + FileContent, + IndexBuildResult, ListResult, + ReindexDecision, + RemoveResult, + SearchResult, + SkillDetail, + SkillSummary, ValidationIssue, ValidationResult, + add_skill, build_index, - should_reindex, - index_search, get_by_id, + index_search, list_all, - IndexBuildResult, - ReindexDecision, + list_skills, + load_skill, + read_skill_file, + remove_skill, + search_skills, + should_reindex, + validate_skill, ) +from skillport.shared import exceptions +from skillport.shared.config import Config __all__ = [ "Config", diff --git a/src/skillport/interfaces/cli/app.py b/src/skillport/interfaces/cli/app.py index a0aeb1f..67d1943 100644 --- a/src/skillport/interfaces/cli/app.py +++ b/src/skillport/interfaces/cli/app.py @@ -6,30 +6,32 @@ - add: Install skills from various sources - list: Show installed skills - remove: Uninstall skills +- update: Update skills from original sources - lint: Validate skill definitions - serve: Start MCP server - doc: Generate skill documentation for AGENTS.md """ -from pathlib import Path import os -from typing import Optional +from pathlib import Path import typer from skillport.shared.config import Config -from .config import load_project_config -from .commands.search import search -from .commands.show import show + +from .auto_index import should_auto_reindex from .commands.add import add -from .commands.remove import remove -from .commands.list import list_cmd -from .commands.lint import lint -from .commands.serve import serve from .commands.doc import doc from .commands.init import init +from .commands.lint import lint +from .commands.list import list_cmd +from .commands.remove import remove +from .commands.search import search +from .commands.serve import serve +from .commands.show import show +from .commands.update import update +from .config import load_project_config from .theme import VERSION, console -from .auto_index import should_auto_reindex def version_callback(value: bool): @@ -54,7 +56,7 @@ def version_callback(value: bool): @app.callback(invoke_without_command=True) def main( ctx: typer.Context, - version: Optional[bool] = typer.Option( + version: bool | None = typer.Option( None, "--version", "-v", @@ -62,17 +64,17 @@ def main( is_eager=True, help="Show version and exit", ), - skills_dir: Optional[Path] = typer.Option( + skills_dir: Path | None = typer.Option( None, "--skills-dir", help="Override skills directory (CLI > env > default)", ), - db_path: Optional[Path] = typer.Option( + db_path: Path | None = typer.Option( None, "--db-path", help="Override LanceDB path (CLI > env > default)", ), - auto_reindex: Optional[bool] = typer.Option( + auto_reindex: bool | None = typer.Option( None, "--auto-reindex/--no-auto-reindex", help="Automatically rebuild index if stale (default: enabled; respects SKILLPORT_AUTO_REINDEX)", @@ -162,6 +164,19 @@ def main( " skillport remove team/skill --force", )(remove) +app.command( + "update", + help="Update skills from their original sources.\n\n" + "By default shows available updates. Use --all to update all,\n" + "or specify a skill ID to update one.\n\n" + "[bold]Examples:[/bold]\n\n" + " skillport update\n\n" + " skillport update my-skill\n\n" + " skillport update --all\n\n" + " skillport update my-skill --force\n\n" + " skillport update --all --dry-run", +)(update) + app.command( "lint", help="Validate skill definitions.\n\n" diff --git a/src/skillport/interfaces/cli/auto_index.py b/src/skillport/interfaces/cli/auto_index.py index e15420e..d9cca36 100644 --- a/src/skillport/interfaces/cli/auto_index.py +++ b/src/skillport/interfaces/cli/auto_index.py @@ -3,14 +3,14 @@ from __future__ import annotations import os -from typing import Optional from skillport.modules.indexing import build_index, should_reindex from skillport.shared.config import Config + from .theme import stderr_console -def _env_auto_reindex_default() -> Optional[bool]: +def _env_auto_reindex_default() -> bool | None: """Parse SKILLPORT_AUTO_REINDEX env var. Returns: diff --git a/src/skillport/interfaces/cli/commands/add.py b/src/skillport/interfaces/cli/commands/add.py index 8465d42..9599ab2 100644 --- a/src/skillport/interfaces/cli/commands/add.py +++ b/src/skillport/interfaces/cli/commands/add.py @@ -7,11 +7,23 @@ from rich.progress import Progress, SpinnerColumn, TextColumn from rich.prompt import Prompt -from skillport.modules.skills import add_skill -from skillport.modules.skills.internal import detect_skills, fetch_github_source, parse_github_url from skillport.modules.indexing import build_index +from skillport.modules.skills import add_skill +from skillport.modules.skills.internal import ( + detect_skills, + fetch_github_source_with_info, + parse_github_url, +) + from ..context import get_config -from ..theme import console, stderr_console, is_interactive, print_error, print_success, print_warning +from ..theme import ( + console, + is_interactive, + print_error, + print_success, + print_warning, + stderr_console, +) def _is_external_source(source: str) -> bool: @@ -35,10 +47,11 @@ def _get_default_namespace(source: str) -> str: return Path(source.rstrip("/")).name -def _detect_skills_from_source(source: str) -> tuple[list[str], str, Path | None]: - """Detect skills from source. Returns (skill_names, source_name, temp_dir).""" +def _detect_skills_from_source(source: str) -> tuple[list[str], str, Path | None, str]: + """Detect skills from source. Returns (skill_names, source_name, temp_dir, commit_sha).""" source_name = _get_source_name(source) temp_dir: Path | None = None + commit_sha: str = "" if source.startswith("https://"): try: @@ -50,27 +63,29 @@ def _detect_skills_from_source(source: str) -> tuple[list[str], str, Path | None transient=True, ) as progress: progress.add_task(f"Fetching {source}...", total=None) - temp_dir = fetch_github_source(source) + fetch_result = fetch_github_source_with_info(source) + temp_dir = fetch_result.extracted_path + commit_sha = fetch_result.commit_sha skills = detect_skills(Path(temp_dir)) skill_names = [s.name for s in skills] if skills else [source_name] - return skill_names, source_name, temp_dir + return skill_names, source_name, temp_dir, commit_sha except Exception as e: if temp_dir and Path(temp_dir).exists(): shutil.rmtree(temp_dir, ignore_errors=True) print_warning(f"Could not fetch source: {e}") - return [source_name], source_name, None + return [source_name], source_name, None, "" source_path = Path(source).expanduser().resolve() if source_path.exists() and source_path.is_dir(): try: skills = detect_skills(source_path) skill_names = [s.name for s in skills] if skills else [source_name] - return skill_names, source_name, None + return skill_names, source_name, None, "" except Exception: - return [source_name], source_name, None + return [source_name], source_name, None, "" - return [source_name], source_name, None + return [source_name], source_name, None, "" def add( @@ -116,11 +131,12 @@ def add( ): """Add skills from various sources.""" temp_dir: Path | None = None + commit_sha: str = "" try: # Interactive namespace selection for external sources if _is_external_source(source) and keep_structure is None and namespace is None: - skill_names, source_name, temp_dir = _detect_skills_from_source(source) + skill_names, source_name, temp_dir, commit_sha = _detect_skills_from_source(source) is_single = len(skill_names) == 1 # Non-interactive mode: use sensible defaults @@ -137,12 +153,12 @@ def add( 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" [cyan][1][/cyan] Flat → skills/{skill_names[0]}/") - console.print(f" [cyan][2][/cyan] Namespace → skills/[dim][/dim]/{skill_names[0]}/") + console.print(f" [info][1][/info] Flat → skills/{skill_names[0]}/") + console.print(f" [info][2][/info] Namespace → skills/[dim][/dim]/{skill_names[0]}/") else: - console.print(f" [cyan][1][/cyan] Flat → skills/{skill_names[0]}/, skills/{skill_names[1]}/, ...") - console.print(f" [cyan][2][/cyan] Namespace → skills/[dim][/dim]/{skill_names[0]}/, ...") - console.print(" [cyan][3][/cyan] Skip") + console.print(f" [info][1][/info] Flat → skills/{skill_names[0]}/, skills/{skill_names[1]}/, ...") + console.print(f" [info][2][/info] Namespace → skills/[dim][/dim]/{skill_names[0]}/, ...") + console.print(" [info][3][/info] Skip") choice = Prompt.ask("Choice", choices=["1", "2", "3"], default="1") if choice == "3": @@ -163,6 +179,7 @@ def add( namespace=namespace, name=name, pre_fetched_dir=temp_dir, + pre_fetched_commit_sha=commit_sha, ) # Auto-reindex if skills were added diff --git a/src/skillport/interfaces/cli/commands/doc.py b/src/skillport/interfaces/cli/commands/doc.py index fef5d19..b6fcf1d 100644 --- a/src/skillport/interfaces/cli/commands/doc.py +++ b/src/skillport/interfaces/cli/commands/doc.py @@ -6,15 +6,15 @@ import re from pathlib import Path -from typing import Optional import typer -from skillport.modules.skills import list_skills, SkillSummary +from skillport.modules.skills import SkillSummary, list_skills from skillport.shared.config import Config + +from ..auto_index import ensure_index_fresh from ..config import load_project_config from ..theme import console -from ..auto_index import ensure_index_fresh MARKER_START = "" MARKER_END = "" @@ -172,12 +172,12 @@ def doc( "--append/--replace", help="Append to existing file or replace entirely", ), - skills_filter: Optional[str] = typer.Option( + skills_filter: str | None = typer.Option( None, "--skills", help="Comma-separated skill IDs to include", ), - category_filter: Optional[str] = typer.Option( + category_filter: str | None = typer.Option( None, "--category", help="Comma-separated categories to include", diff --git a/src/skillport/interfaces/cli/commands/init.py b/src/skillport/interfaces/cli/commands/init.py index 0981fcd..fc3caf9 100644 --- a/src/skillport/interfaces/cli/commands/init.py +++ b/src/skillport/interfaces/cli/commands/init.py @@ -5,15 +5,15 @@ """ from pathlib import Path -from typing import Optional import typer from skillport.modules.indexing import build_index from skillport.modules.skills import list_skills + from ..context import get_config -from .doc import generate_skills_block, update_agents_md from ..theme import console, print_banner +from .doc import generate_skills_block, update_agents_md # Default choices for interactive mode # (display_name, actual_path) - None means "use display as path" @@ -110,13 +110,13 @@ def _create_skillportrc( def init( ctx: typer.Context, - skills_dir: Optional[Path] = typer.Option( + skills_dir: Path | None = typer.Option( None, "--skills-dir", "-d", help="Skills directory path", ), - instructions: Optional[list[str]] = typer.Option( + instructions: list[str] | None = typer.Option( None, "--instructions", "-i", diff --git a/src/skillport/interfaces/cli/commands/lint.py b/src/skillport/interfaces/cli/commands/lint.py index 297839c..ad213a0 100644 --- a/src/skillport/interfaces/cli/commands/lint.py +++ b/src/skillport/interfaces/cli/commands/lint.py @@ -5,6 +5,7 @@ from skillport.modules.indexing import list_all from skillport.modules.skills.public.validation import validate_skill + from ..context import get_config from ..theme import console, print_success, print_warning diff --git a/src/skillport/interfaces/cli/commands/list.py b/src/skillport/interfaces/cli/commands/list.py index fd936b6..aa880f7 100644 --- a/src/skillport/interfaces/cli/commands/list.py +++ b/src/skillport/interfaces/cli/commands/list.py @@ -3,10 +3,11 @@ import typer from rich.table import Table -from skillport.modules.skills import list_skills, ListResult +from skillport.modules.skills import ListResult, list_skills + +from ..auto_index import ensure_index_fresh from ..context import get_config from ..theme import console, empty_skills_panel -from ..auto_index import ensure_index_fresh def list_cmd( diff --git a/src/skillport/interfaces/cli/commands/remove.py b/src/skillport/interfaces/cli/commands/remove.py index 43e70da..d0949df 100644 --- a/src/skillport/interfaces/cli/commands/remove.py +++ b/src/skillport/interfaces/cli/commands/remove.py @@ -3,10 +3,11 @@ import typer from rich.progress import Progress, SpinnerColumn, TextColumn -from skillport.modules.skills import remove_skill from skillport.modules.indexing import build_index +from skillport.modules.skills import remove_skill + from ..context import get_config -from ..theme import console, stderr_console, print_error, print_success, is_interactive +from ..theme import console, is_interactive, print_error, print_success, stderr_console def remove( diff --git a/src/skillport/interfaces/cli/commands/search.py b/src/skillport/interfaces/cli/commands/search.py index d67ed95..5c435a6 100644 --- a/src/skillport/interfaces/cli/commands/search.py +++ b/src/skillport/interfaces/cli/commands/search.py @@ -2,10 +2,11 @@ import typer -from skillport.modules.skills import search_skills, SearchResult -from ..context import get_config -from ..theme import console, no_results_panel, create_skills_table, format_score +from skillport.modules.skills import SearchResult, search_skills + from ..auto_index import ensure_index_fresh +from ..context import get_config +from ..theme import console, create_skills_table, format_score, no_results_panel def search( diff --git a/src/skillport/interfaces/cli/commands/serve.py b/src/skillport/interfaces/cli/commands/serve.py index 812062a..a36ca7b 100644 --- a/src/skillport/interfaces/cli/commands/serve.py +++ b/src/skillport/interfaces/cli/commands/serve.py @@ -4,8 +4,9 @@ import typer from skillport.interfaces.mcp.server import run_server + from ..context import get_config -from ..theme import stderr_console, VERSION +from ..theme import VERSION, stderr_console def serve( diff --git a/src/skillport/interfaces/cli/commands/show.py b/src/skillport/interfaces/cli/commands/show.py index 239108f..858b861 100644 --- a/src/skillport/interfaces/cli/commands/show.py +++ b/src/skillport/interfaces/cli/commands/show.py @@ -6,9 +6,10 @@ from skillport.modules.skills import load_skill from skillport.shared.exceptions import SkillNotFoundError + +from ..auto_index import ensure_index_fresh from ..context import get_config from ..theme import console, print_error -from ..auto_index import ensure_index_fresh def show( @@ -51,8 +52,8 @@ def show( console.print(Panel( header, - title=f"[cyan]{detail.id}[/cyan]", - border_style="cyan", + title=f"[skill.id]{detail.id}[/skill.id]", + border_style="info", )) # Instructions as markdown diff --git a/src/skillport/interfaces/cli/commands/update.py b/src/skillport/interfaces/cli/commands/update.py new file mode 100644 index 0000000..e15e65e --- /dev/null +++ b/src/skillport/interfaces/cli/commands/update.py @@ -0,0 +1,353 @@ +"""Update skills command.""" + +import typer +from rich.progress import Progress, SpinnerColumn, TextColumn + +from skillport.modules.indexing import build_index +from skillport.modules.skills import ( + check_update_available, + detect_local_modification, + update_all_skills, + update_skill, +) +from skillport.modules.skills.internal import get_all_origins + +from ..context import get_config +from ..theme import console, print_error, print_success, print_warning, stderr_console + + +def update( + ctx: typer.Context, + skill_id: str = typer.Argument( + None, + help="Skill ID to update (omit for --all or --check)", + show_default=False, + ), + all_skills: bool = typer.Option( + False, + "--all", + "-a", + help="Update all updatable skills", + ), + force: bool = typer.Option( + False, + "--force", + "-f", + help="Overwrite local modifications", + ), + dry_run: bool = typer.Option( + False, + "--dry-run", + "-n", + help="Show what would be updated without making changes", + ), + check: bool = typer.Option( + False, + "--check", + "-c", + help="Check for available updates without updating", + ), + json_output: bool = typer.Option( + False, + "--json", + help="Output as JSON (for scripting/AI agents)", + ), +): + """Update skills from their original sources. + + By default (no arguments), shows available updates like --check. + Use --all to update all skills, or specify a skill ID to update one. + """ + config = get_config(ctx) + + # Detect "default invocation" (no flags/ID) to apply spec default behavior + default_invocation = skill_id is None and not all_skills and not check + + # Default behavior: check mode + if default_invocation: + check = True + + # Check mode: show available updates + if check: + result = _show_available_updates( + config, + json_output, + interactive=default_invocation and not json_output, + ) + + # In the default (flagなし) case, offer to run --all when updates exist + if ( + default_invocation + and not json_output + and result["updates_available"] + and console.is_interactive + ): + if typer.confirm("\nUpdate all listed skills now?", default=False): + candidate_ids = [item["skill_id"] for item in result["updates_available"]] + with Progress( + SpinnerColumn(), + TextColumn("[progress.description]{task.description}"), + console=stderr_console, + transient=True, + ) as progress: + if not dry_run: + progress.add_task("Updating selected skills...", total=None) + bulk_result = update_all_skills( + config=config, + force=force, + dry_run=dry_run, + skill_ids=candidate_ids, + ) + + _render_update_all_result( + bulk_result, + config=config, + dry_run=dry_run, + ) + else: + console.print("[dim]No changes made.[/dim]") + return + + # Update single skill + if skill_id and not all_skills: + with Progress( + SpinnerColumn(), + TextColumn("[progress.description]{task.description}"), + console=stderr_console, + transient=True, + ) as progress: + if not dry_run: + progress.add_task(f"Updating {skill_id}...", total=None) + + result = update_skill( + skill_id, + config=config, + force=force, + dry_run=dry_run, + ) + + # JSON output + if json_output: + console.print_json( + data={ + "updated": result.updated, + "skipped": result.skipped, + "errors": result.errors, + "message": result.message, + "local_modified": result.local_modified, + "details": [d.model_dump() for d in result.details], + } + ) + if not result.success: + raise typer.Exit(code=1) + return + + # Human-readable output + if result.local_modified and not force: + print_warning(f"Local modifications detected in '{skill_id}'") + console.print(" Use [bold]--force[/bold] to overwrite local changes") + raise typer.Exit(code=1) + + if result.updated: + for detail in result.details: + if detail.from_commit and detail.to_commit: + console.print( + f"[success] + Updated '{skill_id}' ({detail.from_commit} -> {detail.to_commit})[/success]" + ) + else: + console.print(f"[success] + Updated '{skill_id}'[/success]") + print_success(result.message) + + # Rebuild index after update + if not dry_run: + with Progress( + SpinnerColumn(), + TextColumn("[progress.description]{task.description}"), + console=stderr_console, + transient=True, + ) as progress: + progress.add_task("Rebuilding index...", total=None) + build_index(config=config, force=True) + + elif result.skipped: + console.print(f"[dim] - '{skill_id}' is already up to date[/dim]") + else: + print_error(result.message) + raise typer.Exit(code=1) + + return + + # Update all skills + if all_skills: + with Progress( + SpinnerColumn(), + TextColumn("[progress.description]{task.description}"), + console=stderr_console, + transient=True, + ) as progress: + if not dry_run: + progress.add_task("Updating all skills...", total=None) + + result = update_all_skills( + config=config, + force=force, + dry_run=dry_run, + ) + + # JSON output + if json_output: + console.print_json( + data={ + "updated": result.updated, + "skipped": result.skipped, + "errors": result.errors, + "message": result.message, + "details": [d.model_dump() for d in result.details], + } + ) + if not result.success: + raise typer.Exit(code=1) + return + + _render_update_all_result(result, config=config, dry_run=dry_run) + return + + # No skill specified and not --all + print_error("Specify a skill ID or use --all to update all skills") + raise typer.Exit(code=1) + + +def _show_available_updates(config, json_output: bool, interactive: bool = False): + """Show available updates without making changes.""" + origins = get_all_origins(config=config) + + updates_available = [] + up_to_date = [] + not_updatable = [] + + for skill_id, origin in origins.items(): + kind = origin.get("kind", "") + + if kind == "builtin": + not_updatable.append( + {"skill_id": skill_id, "reason": "Built-in skill"} + ) + continue + + has_local_mods = detect_local_modification(skill_id, config=config) + result = check_update_available(skill_id, config=config) + + if result.get("available"): + updates_available.append( + { + "skill_id": skill_id, + "kind": kind, + "local_modified": has_local_mods, + "new_commit": result.get("new_commit", ""), + } + ) + continue + + reason = result.get("reason", "Already up to date") + # classify non-updatable reasons + if any( + term in reason.lower() + for term in [ + "not found", + "missing", + "unreadable", + "unknown origin", + "not checkable", + "too_many_files", + "too_large", + ] + ): + not_updatable.append({"skill_id": skill_id, "reason": reason}) + else: + up_to_date.append({"skill_id": skill_id, "kind": kind, "reason": reason}) + + # JSON output + data = { + "updates_available": updates_available, + "up_to_date": up_to_date, + "not_updatable": not_updatable, + } + + if json_output: + console.print_json(data=data) + return data + + # Human-readable output + if updates_available: + console.print("\n[bold]Updates available:[/bold]") + for item in updates_available: + mod_marker = ( + " [warning](local changes)[/warning]" + if item.get("local_modified") + else "" + ) + commit = "" + if item.get("new_commit"): + commit = f" @ {item['new_commit']}" + console.print( + f" [skill.id]{item['skill_id']}[/skill.id] ({item['kind']}{commit}){mod_marker}" + ) + + console.print( + "\n[dim]Run 'skillport update --all' to update all, or 'skillport update ' for one.[/dim]" + ) + else: + console.print("\n[dim]All skills are up to date.[/dim]") + + if not_updatable: + console.print("\n[dim]Not updatable:[/dim]") + for item in not_updatable: + console.print(f" [dim]{item['skill_id']}: {item['reason']}[/dim]") + + return data + + +def _render_update_all_result(result, *, config, dry_run: bool): + """Render human-readable output for bulk update results.""" + if result.updated: + for detail in result.details: + if detail.from_commit and detail.to_commit: + console.print( + f"[success] + Updated '{detail.skill_id}' ({detail.from_commit} -> {detail.to_commit})[/success]" + ) + else: + console.print(f"[success] + Updated '{detail.skill_id}'[/success]") + + if result.skipped: + console.print( + f"[dim] - {len(result.skipped)} skill(s) already up to date[/dim]" + ) + + if result.errors: + print_error("Errors encountered during update:") + for err in result.errors: + console.print(f" - {err}") + + if result.updated: + print_success(result.message) + + # Rebuild index after update + if not dry_run: + with Progress( + SpinnerColumn(), + TextColumn("[progress.description]{task.description}"), + console=stderr_console, + transient=True, + ) as progress: + progress.add_task("Rebuilding index...", total=None) + build_index(config=config, force=True) + elif result.skipped: + console.print("[dim]All skills are up to date[/dim]") + else: + if result.success: + console.print("[dim]No skills to update[/dim]") + else: + print_error(result.message) + + if not result.success: + raise typer.Exit(code=1) diff --git a/src/skillport/interfaces/cli/config.py b/src/skillport/interfaces/cli/config.py index 9832fd6..2d7fc62 100644 --- a/src/skillport/interfaces/cli/config.py +++ b/src/skillport/interfaces/cli/config.py @@ -6,11 +6,11 @@ Note: MCP Server uses environment variables only. This module is CLI-only. """ +import os +import sys from dataclasses import dataclass from pathlib import Path from typing import Optional -import os -import sys import yaml @@ -146,7 +146,7 @@ def default(cls) -> "ProjectConfig": ) -def load_project_config(cwd: Optional[Path] = None) -> ProjectConfig: +def load_project_config(cwd: Path | None = None) -> ProjectConfig: """Load project configuration with priority resolution. Resolution order (SPEC2-CLI Section 4.2.1): diff --git a/src/skillport/interfaces/cli/theme.py b/src/skillport/interfaces/cli/theme.py index 03303ba..bd2cf7e 100644 --- a/src/skillport/interfaces/cli/theme.py +++ b/src/skillport/interfaces/cli/theme.py @@ -7,7 +7,7 @@ """ import os -from importlib.metadata import version, PackageNotFoundError +from importlib.metadata import PackageNotFoundError, version from typing import Any from rich.console import Console @@ -15,7 +15,6 @@ from rich.table import Table from rich.theme import Theme - # Version from package metadata (pyproject.toml) try: VERSION = version("skillport") diff --git a/src/skillport/interfaces/mcp/tools.py b/src/skillport/interfaces/mcp/tools.py index 2f21c02..d9f0354 100644 --- a/src/skillport/interfaces/mcp/tools.py +++ b/src/skillport/interfaces/mcp/tools.py @@ -1,4 +1,4 @@ -from typing import Any, Dict +from typing import Any from fastmcp import FastMCP @@ -21,7 +21,7 @@ def register_tools(mcp: FastMCP, config: Config, *, is_remote: bool = False) -> registered: list[str] = [] @mcp.tool(name="search_skills") - def search_skills_tool(query: str) -> Dict[str, Any]: + def search_skills_tool(query: str) -> dict[str, Any]: """Find skills relevant to a task description. Use this to discover skills. If you already have a skill_id, skip to load_skill. @@ -37,7 +37,7 @@ def search_skills_tool(query: str) -> Dict[str, Any]: result = search_skills(query, limit=config.search_limit, config=config) skills_list = [] for s in result.skills: - item: Dict[str, Any] = { + item: dict[str, Any] = { "id": s.id, "description": s.description, "score": s.score, @@ -51,7 +51,7 @@ def search_skills_tool(query: str) -> Dict[str, Any]: registered.append("search_skills") @mcp.tool(name="load_skill") - def load_skill_tool(skill_id: str) -> Dict[str, Any]: + def load_skill_tool(skill_id: str) -> dict[str, Any]: """Load a skill's instructions and absolute filesystem path. Call this after selecting an id from search_skills. The returned `path` is @@ -78,7 +78,7 @@ def load_skill_tool(skill_id: str) -> Dict[str, Any]: if is_remote: @mcp.tool(name="read_skill_file") - def read_skill_file_tool(skill_id: str, file_path: str) -> Dict[str, Any]: + def read_skill_file_tool(skill_id: str, file_path: str) -> dict[str, Any]: """Read a file inside a skill directory. Handles both text and binary files: diff --git a/src/skillport/modules/__init__.py b/src/skillport/modules/__init__.py index 5f73e02..f86ba07 100644 --- a/src/skillport/modules/__init__.py +++ b/src/skillport/modules/__init__.py @@ -1,31 +1,33 @@ """Modules layer entry point.""" +from .indexing import ( + IndexBuildResult, + ReindexDecision, + build_index, + get_by_id, + list_all, + should_reindex, +) +from .indexing import ( + search as index_search, +) from .skills import ( - search_skills, - load_skill, - add_skill, - remove_skill, - list_skills, - read_skill_file, - validate_skill, - SkillSummary, - SkillDetail, - FileContent, - SearchResult, AddResult, - RemoveResult, + FileContent, ListResult, + RemoveResult, + SearchResult, + SkillDetail, + SkillSummary, ValidationIssue, ValidationResult, -) -from .indexing import ( - build_index, - should_reindex, - search as index_search, - get_by_id, - list_all, - IndexBuildResult, - ReindexDecision, + add_skill, + list_skills, + load_skill, + read_skill_file, + remove_skill, + search_skills, + validate_skill, ) __all__ = [ diff --git a/src/skillport/modules/indexing/__init__.py b/src/skillport/modules/indexing/__init__.py index 9ef6ed0..376422e 100644 --- a/src/skillport/modules/indexing/__init__.py +++ b/src/skillport/modules/indexing/__init__.py @@ -1,7 +1,7 @@ """Public API for the indexing module.""" from .public.index import build_index, should_reindex -from .public.query import search, get_by_id, list_all, get_core_skills +from .public.query import get_by_id, get_core_skills, list_all, search from .public.types import IndexBuildResult, ReindexDecision __all__ = [ diff --git a/src/skillport/modules/indexing/internal/__init__.py b/src/skillport/modules/indexing/internal/__init__.py index 036066f..9fc81b5 100644 --- a/src/skillport/modules/indexing/internal/__init__.py +++ b/src/skillport/modules/indexing/internal/__init__.py @@ -1,8 +1,8 @@ """Internal indexing components (not part of public API).""" -from .lancedb import IndexStore from .embeddings import get_embedding -from .state import IndexStateStore +from .lancedb import IndexStore from .search_service import SearchService +from .state import IndexStateStore __all__ = ["IndexStore", "get_embedding", "IndexStateStore", "SearchService"] diff --git a/src/skillport/modules/indexing/internal/embeddings.py b/src/skillport/modules/indexing/internal/embeddings.py index 16cbdf8..6c30c14 100644 --- a/src/skillport/modules/indexing/internal/embeddings.py +++ b/src/skillport/modules/indexing/internal/embeddings.py @@ -3,12 +3,11 @@ from __future__ import annotations import sys -from typing import List, Optional from skillport.shared.config import Config -def get_embedding(text: str, config: Config) -> Optional[List[float]]: +def get_embedding(text: str, config: Config) -> list[float] | None: """Fetch embedding according to provider; returns None when provider='none'.""" provider = config.embedding_provider text = text.replace("\n", " ") diff --git a/src/skillport/modules/indexing/internal/lancedb.py b/src/skillport/modules/indexing/internal/lancedb.py index 649e8b1..b6e00ac 100644 --- a/src/skillport/modules/indexing/internal/lancedb.py +++ b/src/skillport/modules/indexing/internal/lancedb.py @@ -3,12 +3,13 @@ import json import sys from pathlib import Path -from typing import Any, Dict, List, Optional +from typing import Any import lancedb from skillport.shared.config import Config from skillport.shared.utils import normalize_token, parse_frontmatter + from .embeddings import get_embedding from .models import SkillRecord from .search_service import SearchService @@ -69,7 +70,7 @@ def _prefilter_clause(self) -> str: return "" - def _embedding_signature(self) -> Dict[str, Any]: + def _embedding_signature(self) -> dict[str, Any]: provider = self.config.embedding_provider if provider == "openai": return { @@ -81,13 +82,13 @@ def _embedding_signature(self) -> Dict[str, Any]: # --- indexing -------------------------------------------------------- def _canonical_metadata( self, - original_meta: Dict[str, Any], - metadata_block: Dict[str, Any], - skillport_meta: Dict[str, Any], + original_meta: dict[str, Any], + metadata_block: dict[str, Any], + skillport_meta: dict[str, Any], category: Any, tags: Any, always_apply: bool, - ) -> Dict[str, Any]: + ) -> dict[str, Any]: meta_copy = dict(original_meta) meta_metadata = dict(metadata_block) if isinstance(metadata_block, dict) else {} skillport = dict(skillport_meta) if isinstance(skillport_meta, dict) else {} @@ -131,7 +132,7 @@ def initialize_index(self) -> None: self.db.drop_table(self.table_name) return - records: List[SkillRecord] = [] + records: list[SkillRecord] = [] vectors_present = False ids_seen: set[str] = set() @@ -190,7 +191,7 @@ def _iter_skill_dirs(base: Path): always_apply = False category_norm = normalize_token(category) if category else "" - tags_norm: List[str] = [] + tags_norm: list[str] = [] if isinstance(tags, list): tags_norm = [normalize_token(t) for t in tags] elif isinstance(tags, str): @@ -251,7 +252,7 @@ def _iter_skill_dirs(base: Path): if self.table_name in self.db.table_names(): self.db.drop_table(self.table_name) - data: List[Dict[str, Any]] = [] + data: list[dict[str, Any]] = [] for r in records: d = r.model_dump() d["tags_text"] = ( @@ -295,12 +296,12 @@ def _iter_skill_dirs(base: Path): # --- state ----------------------------------------------------------- def should_reindex( self, *, force: bool = False, skip_auto: bool = False - ) -> Dict[str, Any]: + ) -> dict[str, Any]: return self.state_store.should_reindex( self._embedding_signature(), force=force, skip_auto=skip_auto ) - def persist_state(self, state: Dict[str, Any]) -> None: + def persist_state(self, state: dict[str, Any]) -> None: self.state_store.persist( state, skills_dir=self.config.skills_dir, db_path=self.db_path ) @@ -311,7 +312,7 @@ def _table(self): return self.db.open_table(self.table_name) return None - def search(self, query: str, *, limit: int) -> List[Dict[str, Any]]: + def search(self, query: str, *, limit: int) -> list[dict[str, Any]]: tbl = self._table() return self.search_service.search( tbl, @@ -321,7 +322,7 @@ def search(self, query: str, *, limit: int) -> List[Dict[str, Any]]: normalize_query=self._normalize_query, ) - def get_by_id(self, identifier: str) -> Optional[Dict[str, Any]]: + def get_by_id(self, identifier: str) -> dict[str, Any] | None: tbl = self._table() if not tbl: return None @@ -341,7 +342,7 @@ def get_by_id(self, identifier: str) -> Optional[Dict[str, Any]]: ) return None - def get_core_skills(self) -> List[Dict[str, Any]]: + def get_core_skills(self) -> list[dict[str, Any]]: tbl = self._table() if not tbl: return [] @@ -355,7 +356,7 @@ def get_core_skills(self) -> List[Dict[str, Any]]: print(f"Error fetching core skills: {exc}", file=sys.stderr) return [] - def list_all(self, *, limit: int) -> List[Dict[str, Any]]: + def list_all(self, *, limit: int) -> list[dict[str, Any]]: tbl = self._table() if not tbl: return [] diff --git a/src/skillport/modules/indexing/internal/models.py b/src/skillport/modules/indexing/internal/models.py index 64d3ff1..62ee8c4 100644 --- a/src/skillport/modules/indexing/internal/models.py +++ b/src/skillport/modules/indexing/internal/models.py @@ -1,4 +1,3 @@ -from typing import List, Optional from lancedb.pydantic import LanceModel from pydantic import Field @@ -9,10 +8,10 @@ class SkillRecord(LanceModel): name: str description: str category: str = "" - tags: List[str] = Field(default_factory=list) + tags: list[str] = Field(default_factory=list) always_apply: bool = False instructions: str path: str lines: int = 0 metadata: str - vector: Optional[List[float]] = None + vector: list[float] | None = None diff --git a/src/skillport/modules/indexing/internal/search_service.py b/src/skillport/modules/indexing/internal/search_service.py index 2b27e5e..a07f524 100644 --- a/src/skillport/modules/indexing/internal/search_service.py +++ b/src/skillport/modules/indexing/internal/search_service.py @@ -3,11 +3,12 @@ from __future__ import annotations import sys +from collections.abc import Callable from dataclasses import dataclass -from typing import Any, Callable, Dict, List, Optional +from typing import Any -def _normalize_score(row: Dict[str, Any]) -> float: +def _normalize_score(row: dict[str, Any]) -> float: if row.get("_score") is not None: try: return float(row["_score"]) @@ -30,11 +31,11 @@ def _normalize_score(row: Dict[str, Any]) -> float: class SearchHit: """Internal search hit (not to be confused with public SearchResult).""" - row: Dict[str, Any] + row: dict[str, Any] score: float source: str - def to_dict(self) -> Dict[str, Any]: + def to_dict(self) -> dict[str, Any]: merged = dict(self.row) merged["_score"] = self.score merged["_source"] = self.source @@ -48,7 +49,7 @@ def __init__( self, *, search_threshold: float, - embed_fn: Callable[[str], Optional[List[float]]], + embed_fn: Callable[[str], list[float] | None], ): self.search_threshold = search_threshold self.embed_fn = embed_fn @@ -61,12 +62,12 @@ def search( limit: int, prefilter: str, normalize_query: Callable[[str], str], - ) -> List[Dict[str, Any]]: + ) -> list[dict[str, Any]]: if not table: return [] query_norm = normalize_query(query) - vec: Optional[List[float]] = None + vec: list[float] | None = None try: vec = self.embed_fn(query_norm) except Exception as exc: # pragma: no cover - defensive logging @@ -112,8 +113,8 @@ def search( # --- strategies --- def _vector_search( - self, table, vec: List[float], prefilter: str, limit: int - ) -> List[SearchHit]: + self, table, vec: list[float], prefilter: str, limit: int + ) -> list[SearchHit]: op = table.search(vec) if prefilter: op = op.where(prefilter) @@ -122,7 +123,7 @@ def _vector_search( def _fts_search( self, table, query: str, prefilter: str, limit: int - ) -> List[SearchHit]: + ) -> list[SearchHit]: op = table.search(query, query_type="fts") if prefilter: op = op.where(prefilter) @@ -131,14 +132,14 @@ def _fts_search( def _substring_search( self, table, query: str, prefilter: str, limit: int - ) -> List[SearchHit]: + ) -> list[SearchHit]: op = table.search() if prefilter: op = op.where(prefilter) rows = op.limit(limit * 3).to_list() qlow = query.lower() - hits: List[SearchHit] = [] + hits: list[SearchHit] = [] for row in rows: if ( qlow in str(row.get("id", "")).lower() @@ -153,7 +154,7 @@ def _substring_search( # --- helpers --- def _fts_then_substring( self, table, query: str, prefilter: str, limit: int - ) -> List[SearchHit]: + ) -> list[SearchHit]: try: return self._fts_search(table, query, prefilter, limit) except Exception as exc: @@ -163,7 +164,7 @@ def _fts_then_substring( return self._substring_search(table, query, prefilter, limit) def _to_hit( - self, row: Dict[str, Any], source: str, default_score: Optional[float] = None + self, row: dict[str, Any], source: str, default_score: float | None = None ) -> SearchHit: score = _normalize_score(row) if score == 0.0 and default_score is not None: diff --git a/src/skillport/modules/indexing/internal/state.py b/src/skillport/modules/indexing/internal/state.py index 1fb615e..efc7e4d 100644 --- a/src/skillport/modules/indexing/internal/state.py +++ b/src/skillport/modules/indexing/internal/state.py @@ -3,7 +3,7 @@ import sys from datetime import datetime, timezone from pathlib import Path -from typing import Any, Dict, Optional, List +from typing import Any from skillport.shared.config import Config @@ -17,9 +17,9 @@ def __init__(self, config: Config, schema_version: str, state_path: Path): self.state_path = state_path # --- hashing --- - def _hash_skills_dir(self) -> Dict[str, Any]: + def _hash_skills_dir(self) -> dict[str, Any]: skills_dir = self.config.skills_dir - entries: List[str] = [] + entries: list[str] = [] if not skills_dir.exists(): return {"hash": "", "count": 0} @@ -43,17 +43,17 @@ def _hash_skills_dir(self) -> Dict[str, Any]: return {"hash": f"sha256:{digest}", "count": len(entries)} # --- IO helpers --- - def _load_state(self) -> Optional[Dict[str, Any]]: + def _load_state(self) -> dict[str, Any] | None: if not self.state_path.exists(): return None try: - with open(self.state_path, "r", encoding="utf-8") as f: + with open(self.state_path, encoding="utf-8") as f: return json.load(f) except Exception as exc: print(f"Failed to load index state: {exc}", file=sys.stderr) return None - def _write_state(self, state: Dict[str, Any]) -> None: + def _write_state(self, state: dict[str, Any]) -> None: try: self.state_path.parent.mkdir(parents=True, exist_ok=True) with open(self.state_path, "w", encoding="utf-8") as f: @@ -63,8 +63,8 @@ def _write_state(self, state: Dict[str, Any]) -> None: # --- public --- def build_current_state( - self, embedding_signature: Dict[str, Any] - ) -> Dict[str, Any]: + self, embedding_signature: dict[str, Any] + ) -> dict[str, Any]: current = self._hash_skills_dir() return { "schema_version": self.schema_version, @@ -75,7 +75,7 @@ def build_current_state( def should_reindex( self, - embedding_signature: Dict[str, Any], + embedding_signature: dict[str, Any], *, force: bool = False, skip_auto: bool = False, @@ -145,7 +145,7 @@ def should_reindex( } def persist( - self, state: Dict[str, Any], *, skills_dir: Path, db_path: Path + self, state: dict[str, Any], *, skills_dir: Path, db_path: Path ) -> None: payload = dict(state) payload["built_at"] = datetime.now(timezone.utc).isoformat() diff --git a/src/skillport/modules/indexing/public/__init__.py b/src/skillport/modules/indexing/public/__init__.py index d2aecca..2f859aa 100644 --- a/src/skillport/modules/indexing/public/__init__.py +++ b/src/skillport/modules/indexing/public/__init__.py @@ -1,5 +1,5 @@ from .index import build_index, should_reindex -from .query import search, get_by_id, list_all +from .query import get_by_id, list_all, search from .types import IndexBuildResult, ReindexDecision __all__ = [ diff --git a/src/skillport/modules/indexing/public/index.py b/src/skillport/modules/indexing/public/index.py index d2e2265..c9906ce 100644 --- a/src/skillport/modules/indexing/public/index.py +++ b/src/skillport/modules/indexing/public/index.py @@ -1,11 +1,23 @@ """Index build-related public APIs.""" +import sys + +from skillport.modules.skills.internal import prune_orphan_origins from skillport.shared.config import Config + from ..internal.lancedb import IndexStore from .types import IndexBuildResult, ReindexDecision def build_index(*, config: Config, force: bool = False) -> IndexBuildResult: + # Clean up stale origin entries before deciding on reindex + removed = prune_orphan_origins(config=config) + if removed: + print( + f"Pruned {len(removed)} orphan origin(s): {', '.join(removed)}", + file=sys.stderr, + ) + store = IndexStore(config) decision = store.should_reindex(force=force, skip_auto=False) diff --git a/src/skillport/modules/indexing/public/query.py b/src/skillport/modules/indexing/public/query.py index dcecf71..a9fbc5b 100644 --- a/src/skillport/modules/indexing/public/query.py +++ b/src/skillport/modules/indexing/public/query.py @@ -1,27 +1,27 @@ """Query-facing public APIs.""" -from typing import Dict, List, Optional from skillport.shared.config import Config + from ..internal.lancedb import IndexStore -def search(query: str, *, limit: int, config: Config) -> List[Dict]: +def search(query: str, *, limit: int, config: Config) -> list[dict]: store = IndexStore(config) return store.search(query, limit=limit) -def get_by_id(skill_id: str, *, config: Config) -> Optional[Dict]: +def get_by_id(skill_id: str, *, config: Config) -> dict | None: store = IndexStore(config) return store.get_by_id(skill_id) -def list_all(*, limit: int, config: Config) -> List[Dict]: +def list_all(*, limit: int, config: Config) -> list[dict]: store = IndexStore(config) return store.list_all(limit=limit) -def get_core_skills(*, config: Config) -> List[Dict]: +def get_core_skills(*, config: Config) -> list[dict]: """Get core skills based on core_skills_mode setting. Modes: diff --git a/src/skillport/modules/skills/__init__.py b/src/skillport/modules/skills/__init__.py index 6b0b862..bc70d5c 100644 --- a/src/skillport/modules/skills/__init__.py +++ b/src/skillport/modules/skills/__init__.py @@ -1,22 +1,28 @@ """Skills module public API.""" from .public import ( - search_skills, - load_skill, - add_skill, - remove_skill, - list_skills, - read_skill_file, - validate_skill, - SkillSummary, - SkillDetail, - FileContent, - SearchResult, AddResult, - RemoveResult, + FileContent, ListResult, + RemoveResult, + SearchResult, + SkillDetail, + SkillSummary, + UpdateResult, + UpdateResultItem, ValidationIssue, ValidationResult, + add_skill, + check_update_available, + detect_local_modification, + list_skills, + load_skill, + read_skill_file, + remove_skill, + search_skills, + update_all_skills, + update_skill, + validate_skill, ) __all__ = [ @@ -27,12 +33,18 @@ "list_skills", "read_skill_file", "validate_skill", + "update_skill", + "update_all_skills", + "detect_local_modification", + "check_update_available", "SkillSummary", "SkillDetail", "FileContent", "SearchResult", "AddResult", "RemoveResult", + "UpdateResult", + "UpdateResultItem", "ListResult", "ValidationIssue", "ValidationResult", diff --git a/src/skillport/modules/skills/internal/__init__.py b/src/skillport/modules/skills/internal/__init__.py index cbc5243..a3ba75a 100644 --- a/src/skillport/modules/skills/internal/__init__.py +++ b/src/skillport/modules/skills/internal/__init__.py @@ -1,16 +1,36 @@ """Internal implementations for the skills module.""" +from .github import ( + GitHubFetchResult, + ParsedGitHubURL, + fetch_github_source, + fetch_github_source_with_info, + get_latest_commit_sha, + get_remote_tree_hash, + parse_github_url, +) from .manager import ( - resolve_source, - detect_skills, + SkillInfo, add_builtin, add_local, + detect_skills, remove_skill, - SkillInfo, + resolve_source, +) +from .origin import ( + compute_content_hash, + compute_content_hash_with_reason, + get_all_origins, + get_origin, + migrate_origin_v2, + prune_orphan_origins, + record_origin, + update_origin, +) +from .origin import ( + remove_origin as remove_origin_record, ) from .validation import validate_skill_record -from .github import parse_github_url, fetch_github_source, ParsedGitHubURL -from .origin import record_origin, remove_origin as remove_origin_record __all__ = [ "resolve_source", @@ -22,7 +42,18 @@ "validate_skill_record", "parse_github_url", "fetch_github_source", + "fetch_github_source_with_info", + "get_latest_commit_sha", + "get_remote_tree_hash", "ParsedGitHubURL", + "GitHubFetchResult", "record_origin", "remove_origin_record", + "get_origin", + "get_all_origins", + "compute_content_hash", + "compute_content_hash_with_reason", + "update_origin", + "migrate_origin_v2", + "prune_orphan_origins", ] diff --git a/src/skillport/modules/skills/internal/github.py b/src/skillport/modules/skills/internal/github.py index f5f2db6..09b5862 100644 --- a/src/skillport/modules/skills/internal/github.py +++ b/src/skillport/modules/skills/internal/github.py @@ -2,9 +2,9 @@ import re import tarfile import tempfile +from collections.abc import Iterable from dataclasses import dataclass from pathlib import Path -from typing import Iterable, Optional import requests @@ -16,9 +16,14 @@ MAX_DOWNLOAD_BYTES = 200_000_000 # 200MB tarball download limit MAX_EXTRACTED_BYTES = 10_000_000 # 10MB extracted skill limit EXCLUDE_NAMES = { - ".git", ".env", "__pycache__", - ".DS_Store", ".Spotlight-V100", ".Trashes", # macOS - "Thumbs.db", "desktop.ini", # Windows + ".git", + ".env", + "__pycache__", + ".DS_Store", + ".Spotlight-V100", + ".Trashes", # macOS + "Thumbs.db", + "desktop.ini", # Windows } @@ -40,7 +45,15 @@ def normalized_path(self) -> str: return self.path.lstrip("/") -def _get_default_branch(owner: str, repo: str, token: Optional[str]) -> str: +@dataclass +class GitHubFetchResult: + """Result of fetching from GitHub including extracted path and commit info.""" + + extracted_path: Path + commit_sha: str # Short SHA (first 7 chars typically) + + +def _get_default_branch(owner: str, repo: str, token: str | None) -> str: """Fetch default branch from GitHub API.""" headers = {"Accept": "application/vnd.github+json"} if token: @@ -56,7 +69,9 @@ def _get_default_branch(owner: str, repo: str, token: Optional[str]) -> str: return "main" -def parse_github_url(url: str, *, resolve_default_branch: bool = False) -> ParsedGitHubURL: +def parse_github_url( + url: str, *, resolve_default_branch: bool = False +) -> ParsedGitHubURL: match = GITHUB_URL_RE.match(url.strip()) if not match: raise ValueError( @@ -92,7 +107,7 @@ def _iter_members_for_prefix( yield member -def download_tarball(parsed: ParsedGitHubURL, token: Optional[str]) -> Path: +def download_tarball(parsed: ParsedGitHubURL, token: str | None) -> Path: headers = {"Accept": "application/vnd.github+json"} if token: headers["Authorization"] = f"Bearer {token}" @@ -122,8 +137,30 @@ def download_tarball(parsed: ParsedGitHubURL, token: Optional[str]) -> Path: tmp.close() -def extract_tarball(tar_path: Path, parsed: ParsedGitHubURL) -> Path: +def _extract_commit_sha_from_root(root_dir_name: str, owner: str, repo: str) -> str: + """Extract commit SHA from tarball root directory name. + + GitHub tarball root dirs follow format: owner-repo-commitsha + e.g., "anthropics-skill-repo-abc1234def" + """ + prefix = f"{owner}-{repo}-" + if root_dir_name.startswith(prefix): + return root_dir_name[len(prefix) :] + # Fallback: try to get the last part after splitting by dash + parts = root_dir_name.split("-") + if len(parts) >= 3: + return parts[-1] + return "" + + +def extract_tarball(tar_path: Path, parsed: ParsedGitHubURL) -> tuple[Path, str]: + """Extract tarball and return (extracted_path, commit_sha). + + The commit SHA is extracted from the tarball root directory name. + """ dest_root = Path(tempfile.mkdtemp(prefix="skillport-gh-")) + commit_sha = "" + with tarfile.open(tar_path, "r:gz") as tar: roots = { member.name.split("/")[0] for member in tar.getmembers() if member.name @@ -131,6 +168,10 @@ def extract_tarball(tar_path: Path, parsed: ParsedGitHubURL) -> Path: if not roots: raise ValueError("Tarball is empty") root = sorted(roots)[0] + + # Extract commit SHA from root directory name + commit_sha = _extract_commit_sha_from_root(root, parsed.owner, parsed.repo) + target_prefix = f"{root}/{parsed.normalized_path}".rstrip("/") if parsed.normalized_path: target_prefix = target_prefix + "/" @@ -168,18 +209,146 @@ def extract_tarball(tar_path: Path, parsed: ParsedGitHubURL) -> Path: raise ValueError("Extracted skill exceeds 10MB limit") with open(dest_path, "wb") as f: f.write(data) - return dest_root + + return dest_root, commit_sha def fetch_github_source(url: str) -> Path: + """Fetch GitHub source and return extracted path (legacy interface).""" + result = fetch_github_source_with_info(url) + return result.extracted_path + + +def fetch_github_source_with_info(url: str) -> GitHubFetchResult: + """Fetch GitHub source and return extracted path with commit info.""" parsed = parse_github_url(url, resolve_default_branch=True) token = os.getenv("GITHUB_TOKEN") tar_path = download_tarball(parsed, token) try: - extracted = extract_tarball(tar_path, parsed) - return extracted + extracted_path, commit_sha = extract_tarball(tar_path, parsed) + return GitHubFetchResult( + extracted_path=extracted_path, + commit_sha=commit_sha, + ) finally: try: tar_path.unlink(missing_ok=True) except Exception: pass + + +def get_latest_commit_sha(parsed: ParsedGitHubURL, token: str | None = None) -> str: + """Fetch the latest commit SHA for a given ref from GitHub API. + + Args: + parsed: ParsedGitHubURL with owner, repo, and ref + token: Optional GitHub token for auth + + Returns: + The full commit SHA string, or empty string on error + """ + headers = {"Accept": "application/vnd.github+json"} + if token: + headers["Authorization"] = f"Bearer {token}" + + # Use commits endpoint to resolve ref to commit + url = f"https://api.github.com/repos/{parsed.owner}/{parsed.repo}/commits/{parsed.ref}" + try: + resp = requests.get(url, headers=headers, timeout=10) + if resp.ok: + return resp.json().get("sha", "")[:40] # Full SHA + except Exception: + pass + return "" + + +# --- Tree-based content hash (for update checking) --- +_tree_cache: dict[tuple[str, str, str], dict] = {} + + +def _fetch_tree(parsed: ParsedGitHubURL, token: str | None) -> dict: + """Fetch repo tree (recursive) with simple in-process cache.""" + cache_key = (parsed.owner, parsed.repo, parsed.ref) + if cache_key in _tree_cache: + return _tree_cache[cache_key] + + headers = {"Accept": "application/vnd.github+json"} + if token: + headers["Authorization"] = f"Bearer {token}" + + url = f"https://api.github.com/repos/{parsed.owner}/{parsed.repo}/git/trees/{parsed.ref}?recursive=1" + resp = requests.get(url, headers=headers, timeout=15) + if not resp.ok: + raise ValueError(f"Failed to fetch tree: HTTP {resp.status_code}") + data = resp.json() + if data.get("truncated"): + raise ValueError("GitHub tree response truncated") + _tree_cache[cache_key] = data + return data + + +def get_remote_tree_hash(parsed: ParsedGitHubURL, token: str | None, path: str) -> str: + """Compute remote content hash for a skill path using the GitHub tree API. + + Returns: + hash string "sha256:..." or "" if unavailable. + """ + try: + tree = _fetch_tree(parsed, token) + except Exception: + return "" + + base_path = (path or parsed.normalized_path).rstrip("/") + prefix = f"{base_path}/" if base_path else "" + + entries = tree.get("tree", []) + if not isinstance(entries, list): + return "" + + import hashlib + + # First pass: collect valid entries with their relative paths + valid_entries: list[tuple[str, str]] = [] # (rel_path, blob_sha) + total_files = 0 + total_bytes = 0 + + for entry in entries: + if entry.get("type") != "blob": + continue + entry_path = entry.get("path", "") + if not entry_path.startswith(prefix): + continue + rel = entry_path[len(prefix):] if prefix else entry_path + if not rel: + continue + parts = Path(rel).parts + if any(part.startswith(".") for part in parts): + continue + if any(part in ("__pycache__", ".git") for part in parts): + continue + size = entry.get("size", 0) or 0 + total_bytes += size + total_files += 1 + # thresholds mirror local hash safeguards + if total_files > 5000 or total_bytes > 100 * 1024 * 1024: + return "" + blob_sha = entry.get("sha", "") + if not blob_sha: + continue + valid_entries.append((str(Path(rel)), blob_sha)) + + if not valid_entries: + return "" + + # Sort entries by relative path to match compute_content_hash_with_reason() + valid_entries.sort(key=lambda x: x[0]) + + # Second pass: compute hash in sorted order + hasher = hashlib.sha256() + for rel_path, blob_sha in valid_entries: + hasher.update(rel_path.encode("utf-8")) + hasher.update(b"\x00") + hasher.update(blob_sha.encode("utf-8")) + hasher.update(b"\x00") + + return f"sha256:{hasher.hexdigest()}" diff --git a/src/skillport/modules/skills/internal/manager.py b/src/skillport/modules/skills/internal/manager.py index 03060b0..8355b7d 100644 --- a/src/skillport/modules/skills/internal/manager.py +++ b/src/skillport/modules/skills/internal/manager.py @@ -5,14 +5,14 @@ import sys from dataclasses import dataclass from pathlib import Path -from typing import List, Tuple import yaml +from skillport.modules.skills.public.types import AddResult, RemoveResult from skillport.shared.config import Config from skillport.shared.types import SourceType from skillport.shared.utils import parse_frontmatter, resolve_inside -from skillport.modules.skills.public.types import AddResult, RemoveResult + from .validation import validate_skill_record # Built-in skills @@ -77,7 +77,7 @@ class SkillInfo: source_path: Path -def resolve_source(source: str) -> Tuple[SourceType, str]: +def resolve_source(source: str) -> tuple[SourceType, str]: """Determine source type and resolved value.""" if not source: raise ValueError("Source is required") @@ -106,14 +106,14 @@ def _load_skill_info(skill_dir: Path) -> SkillInfo: return SkillInfo(name=name, source_path=skill_dir) -def detect_skills(path: Path) -> List[SkillInfo]: +def detect_skills(path: Path) -> list[SkillInfo]: """Detect skills under the given path (root or one-level children).""" if not path.exists(): raise FileNotFoundError(f"Source not found: {path}") if not path.is_dir(): raise ValueError(f"Source must be a directory: {path}") - skills: List[SkillInfo] = [] + skills: list[SkillInfo] = [] root_skill = path / "SKILL.md" if root_skill.exists(): skills.append(_load_skill_info(path)) @@ -248,18 +248,18 @@ def add_builtin(name: str, *, config: Config, force: bool) -> AddResult: def add_local( source_path: Path, - skills: List[SkillInfo], + skills: list[SkillInfo], *, config: Config, keep_structure: bool, force: bool, namespace_override: str | None = None, rename_single_to: str | None = None, -) -> List[AddResult]: +) -> list[AddResult]: target_root = config.skills_dir target_root.mkdir(parents=True, exist_ok=True) - results: List[AddResult] = [] + results: list[AddResult] = [] namespace = namespace_override or source_path.name seen_ids: set[str] = set() diff --git a/src/skillport/modules/skills/internal/origin.py b/src/skillport/modules/skills/internal/origin.py index c14acac..db54101 100644 --- a/src/skillport/modules/skills/internal/origin.py +++ b/src/skillport/modules/skills/internal/origin.py @@ -1,21 +1,30 @@ +import hashlib import json import sys from datetime import datetime, timezone -from typing import Any, Dict +from pathlib import Path +from typing import Any from skillport.shared.config import Config +# Maximum number of entries in update_history +MAX_UPDATE_HISTORY = 10 -def _path_for_config(config: Config): +# Hash calculation safeguards (configurable via env in the future) +MAX_HASH_BYTES = 100 * 1024 * 1024 # 100 MB +MAX_HASH_FILES = 5000 + + +def _path_for_config(config: Config) -> Path: return (config.meta_dir / "origins.json").expanduser().resolve() -def _load(config: Config) -> Dict[str, Any]: +def _load(config: Config) -> dict[str, Any]: path = _path_for_config(config) if not path.exists(): return {} try: - with open(path, "r", encoding="utf-8") as f: + with open(path, encoding="utf-8") as f: data = json.load(f) if isinstance(data, dict): return data @@ -24,17 +33,19 @@ def _load(config: Config) -> Dict[str, Any]: return {} -def _save(config: Config, data: Dict[str, Any]) -> None: +def _save(config: Config, data: dict[str, Any]) -> None: path = _path_for_config(config) path.parent.mkdir(parents=True, exist_ok=True) with open(path, "w", encoding="utf-8") as f: json.dump(data, f, ensure_ascii=False, indent=2) -def record_origin(skill_id: str, payload: Dict[str, Any], *, config: Config) -> None: +def record_origin(skill_id: str, payload: dict[str, Any], *, config: Config) -> None: data = _load(config) enriched = dict(payload) - enriched.setdefault("added_at", datetime.now(timezone.utc).isoformat()) + now_iso = datetime.now(timezone.utc).isoformat() + enriched.setdefault("added_at", now_iso) + enriched.setdefault("updated_at", enriched.get("added_at", now_iso)) enriched.setdefault("skills_dir", str(config.skills_dir)) data[skill_id] = enriched _save(config, data) @@ -45,3 +56,200 @@ def remove_origin(skill_id: str, *, config: Config) -> None: if skill_id in data: del data[skill_id] _save(config, data) + + +def prune_orphan_origins(*, config: Config) -> list[str]: + """Remove origin entries whose skill directory no longer exists. + + Returns: + List of skill_ids removed. + """ + data = _load(config) + if not data: + return [] + + removed: list[str] = [] + for skill_id, origin in list(data.items()): + # Only touch entries that belong to the same skills_dir to avoid + # accidentally pruning other catalogs. + origin_skills_dir = origin.get("skills_dir") + if origin_skills_dir and Path(origin_skills_dir).resolve() != config.skills_dir: + continue + + skill_path = config.skills_dir / skill_id + if not skill_path.exists(): + removed.append(skill_id) + data.pop(skill_id, None) + + if removed: + _save(config, data) + + return removed + + +def get_origin(skill_id: str, *, config: Config) -> dict[str, Any] | None: + """Get origin info for a skill. + + Returns the origin dict if found, otherwise None. + The origin is automatically migrated to v2 format. + """ + data = _load(config) + origin = data.get(skill_id) + if origin is None: + return None + + migrated = migrate_origin_v2(dict(origin)) + if migrated != origin: + data[skill_id] = migrated + _save(config, data) + return migrated + + +def get_all_origins(*, config: Config) -> dict[str, dict[str, Any]]: + """Get all origins, migrated to v2 format.""" + data = _load(config) + migrated: dict[str, dict[str, Any]] = {} + changed = False + + for skill_id, origin in data.items(): + new_origin = migrate_origin_v2(dict(origin)) + migrated[skill_id] = new_origin + if new_origin != origin: + changed = True + + if changed: + _save(config, migrated) + + return migrated + + +def migrate_origin_v2(origin: dict[str, Any]) -> dict[str, Any]: + """Migrate origin entry to v2 format in-place and return it. + + v2 fields: + - updated_at: ISO8601 timestamp of last update + - commit_sha: GitHub commit SHA (for github kind) + - content_hash: SHA256 hash of SKILL.md content + - local_modified: Whether local changes exist + - update_history: List of update events (max 10) + """ + # Ensure v2 fields are present with safe defaults + origin.setdefault("content_hash", "") # Will be computed on next update + origin.setdefault("commit_sha", "") # Will be fetched on next update + origin.setdefault("local_modified", False) + origin.setdefault("update_history", []) + origin.setdefault("updated_at", origin.get("added_at", "")) + + # Trim history to the allowed window + if len(origin["update_history"]) > MAX_UPDATE_HISTORY: + origin["update_history"] = origin["update_history"][:MAX_UPDATE_HISTORY] + + return origin + + +def compute_content_hash(skill_path: Path) -> str: + """Backward-compatible wrapper returning only the hash.""" + hash_value, _reason = compute_content_hash_with_reason(skill_path) + return hash_value + + +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). + + Returns: + (hash, skipped_reason) + - hash: "sha256:..." when successful, "" on failure/skip + - skipped_reason: None if computed, else a human-readable reason + """ + if not skill_path.exists() or not skill_path.is_dir(): + return "", "missing" + + files: list[Path] = [] + total_bytes = 0 + # Sort by string representation to match get_remote_tree_hash() behavior + # (Path sorting differs from string sorting: Path("a/b") < Path("a.x") but "a.x" < "a/b") + for p in sorted(skill_path.rglob("*"), key=lambda p: str(p.relative_to(skill_path))): + if not p.is_file(): + continue + rel = p.relative_to(skill_path) + parts = rel.parts + if any(part.startswith(".") for part in parts): + continue + if any(part in ("__pycache__", ".git") for part in parts): + continue + try: + st = p.stat() + except OSError: + continue + total_bytes += st.st_size + files.append(p) + if len(files) > MAX_HASH_FILES: + return "", "too_many_files" + if total_bytes > MAX_HASH_BYTES: + return "", "too_large" + + hasher = hashlib.sha256() + if not files: + return "", "empty" + + 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") + + return f"sha256:{hasher.hexdigest()}", None + + +def update_origin( + skill_id: str, + updates: dict[str, Any], + *, + config: Config, + add_history_entry: dict[str, Any] | None = None, +) -> None: + """Update origin fields for a skill. + + Args: + skill_id: The skill ID + updates: Dict of fields to update (merged into existing origin) + config: Config instance + add_history_entry: Optional history entry to prepend to update_history + + Note: + - Existing fields not in updates are preserved + - update_history is rotated to keep max 10 entries + """ + data = _load(config) + + if skill_id not in data: + # Create new entry with updates + origin = dict(updates) + origin.setdefault("added_at", datetime.now(timezone.utc).isoformat()) + origin.setdefault("skills_dir", str(config.skills_dir)) + else: + # Merge updates into existing origin (migrated to v2) + origin = migrate_origin_v2(dict(data[skill_id])) + origin.update(updates) + + # Handle update_history rotation + if add_history_entry: + history = origin.get("update_history", []) + # Prepend new entry and limit to MAX_UPDATE_HISTORY + history = [add_history_entry] + history[: MAX_UPDATE_HISTORY - 1] + origin["update_history"] = history + + data[skill_id] = origin + _save(config, data) diff --git a/src/skillport/modules/skills/internal/validation.py b/src/skillport/modules/skills/internal/validation.py index 061d51c..d2febc2 100644 --- a/src/skillport/modules/skills/internal/validation.py +++ b/src/skillport/modules/skills/internal/validation.py @@ -4,7 +4,6 @@ import re from pathlib import Path -from typing import Dict, List, Set from skillport.shared.types import ValidationIssue from skillport.shared.utils import parse_frontmatter @@ -17,7 +16,7 @@ XML_TAG_PATTERN = re.compile(r"<[^>]+>") # Allowed top-level frontmatter properties -ALLOWED_FRONTMATTER_KEYS: Set[str] = { +ALLOWED_FRONTMATTER_KEYS: set[str] = { "name", "description", "license", @@ -27,11 +26,11 @@ def validate_skill_record( - skill: Dict, + skill: dict, *, strict: bool = False, - meta: Dict | None = None, -) -> List[ValidationIssue]: + meta: dict | None = None, +) -> list[ValidationIssue]: """Validate a skill dict; returns issue list. Args: @@ -43,7 +42,7 @@ def validate_skill_record( Returns: List of validation issues. """ - issues: List[ValidationIssue] = [] + issues: list[ValidationIssue] = [] name = skill.get("name", "") description = skill.get("description", "") lines = skill.get("lines", 0) diff --git a/src/skillport/modules/skills/public/__init__.py b/src/skillport/modules/skills/public/__init__.py index 94b4932..adb80cc 100644 --- a/src/skillport/modules/skills/public/__init__.py +++ b/src/skillport/modules/skills/public/__init__.py @@ -1,21 +1,29 @@ -from .search import search_skills -from .load import load_skill from .add import add_skill -from .remove import remove_skill from .list import list_skills +from .load import load_skill from .read import read_skill_file -from .validation import validate_skill +from .remove import remove_skill +from .search import search_skills from .types import ( - SkillSummary, - SkillDetail, - FileContent, - SearchResult, AddResult, - RemoveResult, + FileContent, ListResult, + RemoveResult, + SearchResult, + SkillDetail, + SkillSummary, + UpdateResult, + UpdateResultItem, ValidationIssue, ValidationResult, ) +from .update import ( + check_update_available, + detect_local_modification, + update_all_skills, + update_skill, +) +from .validation import validate_skill __all__ = [ "search_skills", @@ -25,12 +33,18 @@ "list_skills", "read_skill_file", "validate_skill", + "update_skill", + "update_all_skills", + "detect_local_modification", + "check_update_available", "SkillSummary", "SkillDetail", "FileContent", "SearchResult", "AddResult", "RemoveResult", + "UpdateResult", + "UpdateResultItem", "ListResult", "ValidationIssue", "ValidationResult", diff --git a/src/skillport/modules/skills/public/add.py b/src/skillport/modules/skills/public/add.py index 2f78449..dda46f2 100644 --- a/src/skillport/modules/skills/public/add.py +++ b/src/skillport/modules/skills/public/add.py @@ -3,17 +3,23 @@ import shutil from pathlib import Path -from skillport.shared.config import Config -from skillport.shared.types import SourceType from skillport.modules.skills.internal import ( - resolve_source, - detect_skills, add_builtin as _add_builtin, +) +from skillport.modules.skills.internal import ( add_local as _add_local, +) +from skillport.modules.skills.internal import ( + compute_content_hash, + detect_skills, + fetch_github_source_with_info, parse_github_url, - fetch_github_source, record_origin, + resolve_source, ) +from skillport.shared.config import Config +from skillport.shared.types import SourceType + from .types import AddResult, AddResultItem @@ -26,6 +32,7 @@ def add_skill( namespace: str | None = None, name: str | None = None, pre_fetched_dir: Path | None = None, + pre_fetched_commit_sha: str = "", ) -> AddResult: """Add a skill from builtin/local/github source.""" try: @@ -36,6 +43,7 @@ def add_skill( temp_dir: Path | None = None cleanup_temp_dir = False origin_payload: dict | None = None + commit_sha: str = "" source_path: Path source_label: str @@ -57,9 +65,12 @@ def add_skill( parsed = parse_github_url(resolved) if pre_fetched_dir: temp_dir = Path(pre_fetched_dir) + commit_sha = pre_fetched_commit_sha cleanup_temp_dir = True else: - temp_dir = fetch_github_source(resolved) + fetch_result = fetch_github_source_with_info(resolved) + temp_dir = fetch_result.extracted_path + commit_sha = fetch_result.commit_sha cleanup_temp_dir = True source_path = Path(temp_dir) source_label = Path(parsed.normalized_path or parsed.repo).name @@ -67,12 +78,14 @@ def add_skill( "source": resolved, "kind": "github", "ref": parsed.ref, - "path": parsed.normalized_path, + # パスが空の場合でも単一スキルなら skill 名を path に保存して判定対象を限定する + "path": parsed.normalized_path or "", + "commit_sha": commit_sha, } else: source_path = Path(resolved) source_label = source_path.name - origin_payload = {"source": str(source_path), "kind": "local"} + origin_payload = {"source": str(source_path), "kind": "local", "path": ""} skills = detect_skills(source_path) @@ -91,6 +104,9 @@ def add_skill( source_path = renamed temp_dir = renamed skills = detect_skills(source_path) + # 単一スキルの場合は origin.path をスキル名で確定させる + if origin_payload is not None: + origin_payload["path"] = origin_payload.get("path") or single.name if not skills: return AddResult( success=False, skill_id="", message=f"No skills found in {source_path}" @@ -103,6 +119,9 @@ def add_skill( effective_keep_structure = ( False if effective_keep_structure is None else effective_keep_structure ) + # 単一スキル: path をスキル名で固定 + if origin_payload is not None and not origin_payload.get("path"): + origin_payload["path"] = skills[0].name else: if effective_keep_structure is None: effective_keep_structure = True @@ -147,7 +166,9 @@ def _summarize_skipped(reasons: list[str]) -> str: # Show first other reason and count remainder first_other = others[0] extra = len(others) - 1 - parts.append(first_other if extra == 0 else f"{first_other} (+{extra} more)") + parts.append( + first_other if extra == 0 else f"{first_other} (+{extra} more)" + ) return "; ".join(parts) if parts else "No skills added" @@ -169,9 +190,38 @@ def _summarize_skipped(reasons: list[str]) -> str: overall_id = added_ids[0] if len(added_ids) == 1 else ",".join(added_ids) if added_ids and origin_payload: + # Build per-skill origin payload with path filled for sid in added_ids: try: - record_origin(sid, origin_payload, config=config) + # Compute content_hash from the installed skill location + skill_path = config.skills_dir / sid + content_hash = compute_content_hash(skill_path) + + # Determine path for this skill relative to source_path + rel_path = "" + if source_path.exists(): + try: + rel_path = str((source_path / sid.split("/")[-1]).relative_to(source_path)) + except Exception: + rel_path = sid.split("/")[-1] + + # Enrich payload with content_hash for v2 origin tracking + enriched_payload = dict(origin_payload) + # For GitHub sources, origin.path は「リポジトリ基準のサブパス」を持つ必要がある。 + # 複数スキル追加時もスキル単位のサブディレクトリを正しく記録する。 + if origin_payload.get("kind") == "github": + 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}" + elif prefix: + enriched_payload["path"] = prefix + else: + enriched_payload["path"] = rel_path + else: + enriched_payload["path"] = rel_path + enriched_payload["content_hash"] = content_hash + + record_origin(sid, enriched_payload, config=config) except Exception: pass diff --git a/src/skillport/modules/skills/public/list.py b/src/skillport/modules/skills/public/list.py index 23f8ead..3a4b8ab 100644 --- a/src/skillport/modules/skills/public/list.py +++ b/src/skillport/modules/skills/public/list.py @@ -1,10 +1,9 @@ from __future__ import annotations -from typing import List - -from skillport.modules.indexing import list_all as idx_list_all +from skillport.modules.indexing.public.query import list_all as idx_list_all from skillport.shared.config import Config from skillport.shared.filters import is_skill_enabled, normalize_token + from .types import ListResult, SkillSummary @@ -12,7 +11,7 @@ def list_skills(*, config: Config, limit: int | None = None) -> ListResult: effective_limit = limit or config.search_limit rows = idx_list_all(limit=effective_limit * 2, config=config) - skills: List[SkillSummary] = [] + skills: list[SkillSummary] = [] for row in rows: skill_id = row.get("id") or row.get("name") category = row.get("category", "") diff --git a/src/skillport/modules/skills/public/load.py b/src/skillport/modules/skills/public/load.py index bba63ce..2a96d2e 100644 --- a/src/skillport/modules/skills/public/load.py +++ b/src/skillport/modules/skills/public/load.py @@ -2,10 +2,11 @@ import json -from skillport.modules.indexing import get_by_id as idx_get_by_id +from skillport.modules.indexing.public.query import get_by_id as idx_get_by_id from skillport.shared.config import Config from skillport.shared.exceptions import AmbiguousSkillError, SkillNotFoundError from skillport.shared.filters import is_skill_enabled, normalize_token + from .types import SkillDetail diff --git a/src/skillport/modules/skills/public/read.py b/src/skillport/modules/skills/public/read.py index 783d011..ab99185 100644 --- a/src/skillport/modules/skills/public/read.py +++ b/src/skillport/modules/skills/public/read.py @@ -4,11 +4,12 @@ import mimetypes from pathlib import Path -from skillport.modules.indexing import get_by_id as idx_get_by_id +from skillport.modules.indexing.public.query import get_by_id as idx_get_by_id from skillport.shared.config import Config from skillport.shared.exceptions import SkillNotFoundError from skillport.shared.filters import is_skill_enabled from skillport.shared.utils import resolve_inside + from .types import FileContent # Extensions that should be treated as text even if mimetypes doesn't recognize them diff --git a/src/skillport/modules/skills/public/remove.py b/src/skillport/modules/skills/public/remove.py index 37b7548..9a32728 100644 --- a/src/skillport/modules/skills/public/remove.py +++ b/src/skillport/modules/skills/public/remove.py @@ -1,8 +1,11 @@ -from skillport.shared.config import Config from skillport.modules.skills.internal import ( - remove_skill as _remove_skill_internal, remove_origin_record, ) +from skillport.modules.skills.internal import ( + remove_skill as _remove_skill_internal, +) +from skillport.shared.config import Config + from .types import RemoveResult diff --git a/src/skillport/modules/skills/public/search.py b/src/skillport/modules/skills/public/search.py index ce60be5..bc3b6a7 100644 --- a/src/skillport/modules/skills/public/search.py +++ b/src/skillport/modules/skills/public/search.py @@ -1,10 +1,10 @@ from __future__ import annotations -from typing import List - -from skillport.modules.indexing import list_all as idx_list_all, search as idx_search -from skillport.shared.config import Config, MAX_SKILLS +from skillport.modules.indexing.public.query import list_all as idx_list_all +from skillport.modules.indexing.public.query import search as idx_search +from skillport.shared.config import MAX_SKILLS, Config from skillport.shared.filters import is_skill_enabled, normalize_token + from .types import SearchResult, SkillSummary @@ -15,14 +15,14 @@ def search_skills(query: str, *, limit: int = 10, config: Config) -> SearchResul is_list_all = not normalized_query.strip() or normalized_query.strip() == "*" # Fetch up to MAX_SKILLS to count total filtered results - raw_results: List[dict] + raw_results: list[dict] if is_list_all: raw_results = idx_list_all(limit=MAX_SKILLS, config=config) else: raw_results = idx_search(normalized_query, limit=MAX_SKILLS, config=config) # Filter and collect all matching skills - all_matching: List[SkillSummary] = [] + all_matching: list[SkillSummary] = [] for row in raw_results: skill_id = row.get("id") or row.get("name") category = row.get("category", "") diff --git a/src/skillport/modules/skills/public/types.py b/src/skillport/modules/skills/public/types.py index d126fe7..8340a92 100644 --- a/src/skillport/modules/skills/public/types.py +++ b/src/skillport/modules/skills/public/types.py @@ -1,10 +1,41 @@ -from typing import Any, Literal +from typing import Any, Literal, TypedDict from pydantic import Field from skillport.shared.types import FrozenModel, ValidationIssue +class OriginKind(TypedDict, total=False): + """Base origin fields common to all origin kinds.""" + + source: str # Source URL or path + kind: Literal["builtin", "local", "github"] + added_at: str # ISO8601 timestamp + updated_at: str # ISO8601 timestamp + skills_dir: str # Directory where skill is installed + + +class OriginLocal(OriginKind): + """Origin info for locally sourced skills.""" + + path: str # Relative path within source + + +class OriginGitHub(OriginKind): + """Origin info for GitHub sourced skills.""" + + ref: str # Git ref (branch/tag) + path: str # Path within repo (e.g., "skills/my-skill") + commit_sha: str # Short commit SHA (7 chars) + content_hash: str # Content hash for change detection + local_modified: bool # Whether local modifications detected + update_history: list[dict[str, str]] # Update history entries + + +# Union type for any origin +Origin = OriginKind | OriginLocal | OriginGitHub + + class SkillSummary(FrozenModel): """検索結果・一覧用のスキル情報""" @@ -90,6 +121,32 @@ class RemoveResult(FrozenModel): message: str +class UpdateResultItem(FrozenModel): + """Individual skill update result.""" + + skill_id: str + success: bool + message: str + from_commit: str = "" + to_commit: str = "" + + +class UpdateResult(FrozenModel): + """update_skill の戻り値""" + + success: bool + skill_id: str = Field(..., description="Updated skill ID (empty if failed)") + message: str = Field(..., description="Human-readable result message") + updated: list[str] = Field(default_factory=list, description="Successfully updated skill IDs") + skipped: list[str] = Field(default_factory=list, description="Skipped skill IDs (no updates/errors)") + details: list[UpdateResultItem] = Field( + default_factory=list, + description="Per-skill results for bulk updates", + ) + local_modified: bool = Field(default=False, description="Whether local modifications were detected") + errors: list[str] = Field(default_factory=list, description="Errors encountered during update (if any)") + + class ListResult(FrozenModel): """list_skills の戻り値""" @@ -110,6 +167,12 @@ class ValidationResult(FrozenModel): __all__ = [ + # Origin types + "Origin", + "OriginKind", + "OriginLocal", + "OriginGitHub", + # Skill types "SkillSummary", "SkillDetail", "FileContent", @@ -117,6 +180,8 @@ class ValidationResult(FrozenModel): "AddResult", "AddResultItem", "RemoveResult", + "UpdateResult", + "UpdateResultItem", "ListResult", "ValidationIssue", "ValidationResult", diff --git a/src/skillport/modules/skills/public/update.py b/src/skillport/modules/skills/public/update.py new file mode 100644 index 0000000..101231c --- /dev/null +++ b/src/skillport/modules/skills/public/update.py @@ -0,0 +1,687 @@ +"""Update skills from their original sources.""" + +from __future__ import annotations + +import os +import shutil +from datetime import datetime, timezone +from pathlib import Path +from typing import Any + +from skillport.modules.skills.internal import ( + compute_content_hash, + compute_content_hash_with_reason, + detect_skills, + fetch_github_source_with_info, + get_all_origins, + get_origin, + get_remote_tree_hash, + parse_github_url, + update_origin, +) +from skillport.shared.config import Config + +from .types import Origin, UpdateResult, UpdateResultItem + + +def detect_local_modification(skill_id: str, *, config: Config) -> bool: + """Check if a skill has local modifications. + + Compares the current SKILL.md content hash against the stored hash. + Returns False if origin info is missing or doesn't have content_hash. + """ + origin = get_origin(skill_id, config=config) + if not origin: + return False # No origin = not tracked + + stored_hash = origin.get("content_hash") + if not stored_hash: + return False # Old format = can't detect, assume no changes + + skill_path = config.skills_dir / skill_id + current_hash = compute_content_hash(skill_path) + + return stored_hash != current_hash + + +# --- common helpers --------------------------------------------------------- + +def _installed_hash(skill_id: str, *, config: Config) -> tuple[str, str | None]: + """Hash installed skill; returns (hash, reason).""" + return compute_content_hash_with_reason(config.skills_dir / skill_id) + + +def _resolve_origin_path(origin: Origin, parsed_path_fallback: str, skill_id: str) -> str: + """Pick the narrowest path for hashing/copying.""" + return origin.get("path") or parsed_path_fallback or skill_id.split("/")[-1] + + +def _local_source_hash( + origin: Origin, skill_id: str, *, config: Config +) -> tuple[str, str | None]: + """Compute source hash for local origin; returns (hash, reason).""" + source_base = Path(origin.get("source", "")) + if not source_base.exists(): + return "", f"Source path not found: {source_base}" + if not source_base.is_dir(): + return "", f"Source is not a directory: {source_base}" + + origin_path = _resolve_origin_path(origin, "", skill_id) + if origin_path: + candidate = source_base / origin_path + if (candidate / "SKILL.md").exists(): + source_path = candidate + else: + source_path = _resolve_local_skill_path(source_base, skill_id) + else: + source_path = _resolve_local_skill_path(source_base, skill_id) + + if source_path is None: + return "", f"Could not find skill in source: {source_base}" + + return compute_content_hash_with_reason(source_path) + + +def _github_source_hash( + origin: Origin, skill_id: str, *, config: Config +) -> tuple[str, str | None]: + """Compute source hash for GitHub origin via tree API; returns (hash, reason).""" + source_url = origin.get("source", "") + if not source_url: + return "", "Missing source URL" + + parsed = parse_github_url(source_url, resolve_default_branch=True) + path = _resolve_origin_path(origin, parsed.normalized_path, skill_id) + token = os.getenv("GITHUB_TOKEN") + + # 1st try: as-is + remote_hash = get_remote_tree_hash(parsed, token, path) + + # If the recorded path points to a parent dir (e.g., "skills"), + # try narrowing to "/" and persist when found. + if not remote_hash or path == parsed.normalized_path: + skill_tail = skill_id.split("/")[-1] + candidate = "/".join(p for p in [parsed.normalized_path, skill_tail] if p) + if candidate != path: + alt_hash = get_remote_tree_hash(parsed, token, candidate) + if alt_hash: + remote_hash = alt_hash + try: + update_origin(skill_id, {"path": candidate}, config=config) + except Exception: + pass + + if not remote_hash: + return "", "Could not fetch remote tree (treated as unknown)" + return remote_hash, None + + +def _source_hash( + origin: Origin, skill_id: str, *, config: Config +) -> tuple[str, str | None]: + """Compute source-side hash; returns (hash, reason). + + Dispatches to _local_source_hash or _github_source_hash based on origin kind. + """ + kind = origin.get("kind", "") + + if kind == "local": + return _local_source_hash(origin, skill_id, config=config) + + if kind == "github": + return _github_source_hash(origin, skill_id, config=config) + + return "", f"Unknown origin kind: {kind}" + + +def check_update_available(skill_id: str, *, config: Config) -> dict[str, Any]: + """Check if an update is available for a skill. + + Returns a dict with: + - available: bool - whether an update is available + - reason: str - explanation + - origin: dict | None - the origin info + - new_commit: str - the new commit SHA (if available) + """ + origin = get_origin(skill_id, config=config) + + if not origin: + return { + "available": False, + "reason": "No origin info (cannot update)", + "origin": None, + "new_commit": "", + } + + kind = origin.get("kind", "") + + if kind == "builtin": + return { + "available": False, + "reason": "Built-in skill cannot be updated", + "origin": origin, + "new_commit": "", + } + + # Unified source / installed hash comparison + source_hash, source_reason = _source_hash(origin, skill_id, config=config) + if source_reason: + return { + "available": False, + "reason": source_reason, + "origin": origin, + "new_commit": "", + } + + installed_hash, installed_reason = _installed_hash(skill_id, config=config) + if installed_reason: + return { + "available": False, + "reason": f"Installed skill unreadable: {installed_reason}", + "origin": origin, + "new_commit": "", + } + + if source_hash == installed_hash: + return { + "available": False, + "reason": "Already at latest content", + "origin": origin, + "new_commit": "", + } + + return { + "available": True, + "reason": "Remote content differs" + if kind == "github" + else "Local source changed", + "origin": origin, + "new_commit": source_hash.split(":", 1)[-1][:7] + if source_hash.startswith("sha256:") + else source_hash[:7], + } + + +def update_skill( + skill_id: str, + *, + config: Config, + force: bool = False, + dry_run: bool = False, +) -> UpdateResult: + """Update a single skill from its original source. + + Args: + skill_id: The skill ID to update + config: Config instance + force: If True, overwrite local modifications + dry_run: If True, don't actually update, just check + + Returns: + UpdateResult with success status and details + """ + # Check if skill exists + skill_path = config.skills_dir / skill_id + if not skill_path.exists(): + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Skill '{skill_id}' not found", + ) + + # Get origin info + origin = get_origin(skill_id, config=config) + if not origin: + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Skill '{skill_id}' has no origin info (cannot update)", + ) + + kind = origin.get("kind", "") + + # Handle different origin types + # Local modification check is now done inside each handler + # because we need to compare with source to determine if it's truly modified + if kind == "builtin": + return UpdateResult( + success=False, + skill_id=skill_id, + message="Built-in skill cannot be updated", + ) + + if kind == "local": + return _update_from_local( + skill_id, origin, config=config, force=force, dry_run=dry_run + ) + + if kind == "github": + return _update_from_github( + skill_id, origin, config=config, force=force, dry_run=dry_run + ) + + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Unknown origin kind: {kind}", + ) + + +def _resolve_local_skill_path(source: Path, skill_id: str) -> Path | None: + """Resolve the actual skill directory within a source. + + The source can be: + - A container directory with skills inside (e.g., /path/to/source with source/my-skill/) + - A single skill directory (e.g., /path/to/my-skill with my-skill/SKILL.md) + """ + # Extract skill name (last part of skill_id for namespaced skills) + skill_name = skill_id.split("/")[-1] + + # Check various possible locations + candidates = [ + source / skill_id, # Full path (e.g., ns/my-skill) + source / skill_name, # Just the name (e.g., my-skill) + source, # Direct skill directory + ] + + for candidate in candidates: + if (candidate / "SKILL.md").exists(): + return candidate + + return None + + +def _update_from_local( + skill_id: str, + origin: Origin, + *, + config: Config, + force: bool, + dry_run: bool, +) -> UpdateResult: + """Update a skill from a local source.""" + source_base = Path(origin.get("source", "")) + + if not source_base.exists(): + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Source path not found: {source_base}", + ) + + if not source_base.is_dir(): + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Source is not a directory: {source_base}", + ) + + # Resolve actual skill path within source (prefer origin.path) + origin_path = origin.get("path", "") + if origin_path: + candidate = source_base / origin_path + source_path = candidate if (candidate / "SKILL.md").exists() else None + else: + source_path = _resolve_local_skill_path(source_base, skill_id) + if source_path is None: + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Could not find skill in source: {source_base}", + ) + + if not origin.get("path"): + try: + rel = source_path.relative_to(source_base) + update_origin(skill_id, {"path": str(rel)}, config=config) + except Exception: + pass + + # Compute hashes for comparison + source_hash, source_reason = compute_content_hash_with_reason(source_path) + if source_reason: + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Source not readable: {source_reason}", + ) + + current_hash, current_reason = compute_content_hash_with_reason( + config.skills_dir / skill_id + ) + if current_reason: + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Installed skill unreadable: {current_reason}", + ) + + stored_hash = origin.get("content_hash", "") + + # If current matches source, already up to date (even if locally modified to match source) + if source_hash == current_hash: + # Sync stored hash if outdated (e.g., old format -> new git blob format) + if stored_hash != current_hash: + update_origin( + skill_id, + {"content_hash": current_hash}, + config=config, + ) + return UpdateResult( + success=True, + skill_id=skill_id, + message="Already up to date", + skipped=[skill_id], + ) + + # Check for local modifications (current differs from what was installed) + has_local_mods = stored_hash and stored_hash != current_hash + + if has_local_mods and not force: + return UpdateResult( + success=False, + skill_id=skill_id, + message="Local modifications detected. Use --force to overwrite", + local_modified=True, + ) + + if dry_run: + return UpdateResult( + success=True, + skill_id=skill_id, + message=f"Would update from {source_path}", + updated=[skill_id], + ) + + # Perform update: remove old, copy new + try: + dest_path = config.skills_dir / skill_id + shutil.rmtree(dest_path) + shutil.copytree(source_path, dest_path) + + # Update origin with new content_hash + new_hash, _ = compute_content_hash_with_reason(dest_path) + update_origin( + skill_id, + { + "content_hash": new_hash, + "updated_at": datetime.now(timezone.utc).isoformat(), + }, + config=config, + ) + + return UpdateResult( + success=True, + skill_id=skill_id, + message="Updated from local source", + updated=[skill_id], + ) + except Exception as e: + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Failed to update: {e}", + ) + + +def _update_from_github( + skill_id: str, + origin: Origin, + *, + config: Config, + force: bool, + dry_run: bool, +) -> UpdateResult: + """Update a skill from GitHub. + + Optimized to check for updates via tree API before downloading tarball. + Only downloads when an actual update is needed. + """ + source_url = origin.get("source", "") + old_commit = origin.get("commit_sha", "")[:7] or "unknown" + stored_hash = origin.get("content_hash", "") + + if not source_url: + return UpdateResult( + success=False, + skill_id=skill_id, + message="Missing GitHub source URL", + ) + + # --- Phase 1: Check if update is needed (no download) --- + + # Get installed hash + current_hash, current_reason = compute_content_hash_with_reason( + config.skills_dir / skill_id + ) + if current_reason: + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Installed skill unreadable: {current_reason}", + ) + + # Get remote hash via tree API (no tarball download) + remote_hash, remote_reason = _source_hash(origin, skill_id, config=config) + if remote_reason: + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Cannot check remote: {remote_reason}", + ) + + # Helper to sync stored hash if outdated + def _sync_stored_hash_if_needed() -> None: + if stored_hash != current_hash: + update_origin( + skill_id, + {"content_hash": current_hash}, + config=config, + ) + + # If hashes match, no update needed + if remote_hash == current_hash: + _sync_stored_hash_if_needed() + return UpdateResult( + success=True, + skill_id=skill_id, + message="Already up to date", + skipped=[skill_id], + ) + + # Check for local modifications before downloading + has_local_mods = stored_hash and stored_hash != current_hash + if has_local_mods and not force: + return UpdateResult( + success=False, + skill_id=skill_id, + message="Local modifications detected. Use --force to overwrite", + local_modified=True, + ) + + # For dry-run, we can return here without downloading + if dry_run: + return UpdateResult( + success=True, + skill_id=skill_id, + message=f"Would update ({old_commit} -> latest)", + updated=[skill_id], + details=[ + UpdateResultItem( + skill_id=skill_id, + success=True, + message="Would update", + from_commit=old_commit, + to_commit="latest", + ) + ], + ) + + # --- Phase 2: Download and apply update --- + + temp_dir: Path | None = None + try: + fetch_result = fetch_github_source_with_info(source_url) + temp_dir = fetch_result.extracted_path + new_commit = fetch_result.commit_sha[:7] if fetch_result.commit_sha else "" + + # Compute relative path for extraction + parsed = parse_github_url(source_url, resolve_default_branch=True) + url_prefix = parsed.normalized_path + origin_path = origin.get("path") or "" + + # Strip URL prefix from origin.path if present + if url_prefix and origin_path.startswith(url_prefix + "/"): + relative_path = origin_path[len(url_prefix) + 1 :] + elif url_prefix and origin_path == url_prefix: + relative_path = "" + else: + relative_path = origin_path + + # Perform update: remove old, copy new + dest_path = config.skills_dir / skill_id + shutil.rmtree(dest_path) + + if relative_path: + candidate = temp_dir / relative_path + if candidate.exists(): + shutil.copytree(candidate, dest_path) + else: + shutil.copytree(temp_dir, dest_path) + else: + skills = detect_skills(temp_dir) + if skills: + source_skill_path = temp_dir + if len(skills) == 1 and skills[0].name != temp_dir.name: + renamed = temp_dir.parent / skills[0].name + if renamed.exists(): + shutil.rmtree(renamed) + temp_dir.rename(renamed) + source_skill_path = renamed + temp_dir = renamed + shutil.copytree(source_skill_path, dest_path) + else: + shutil.copytree(temp_dir, dest_path) + + # Update origin with new content_hash and commit_sha + new_hash, _ = compute_content_hash_with_reason(dest_path) + history_entry = { + "from_commit": old_commit, + "to_commit": new_commit or "latest", + "updated_at": datetime.now(timezone.utc).isoformat(), + } + + update_origin( + skill_id, + { + "content_hash": new_hash, + "commit_sha": fetch_result.commit_sha, + "updated_at": datetime.now(timezone.utc).isoformat(), + "local_modified": False, + }, + config=config, + add_history_entry=history_entry, + ) + + return UpdateResult( + success=True, + skill_id=skill_id, + message=f"Updated ({old_commit} -> {new_commit or 'latest'})", + updated=[skill_id], + details=[ + UpdateResultItem( + skill_id=skill_id, + success=True, + message="Updated", + from_commit=old_commit, + to_commit=new_commit or "latest", + ) + ], + ) + + except Exception as e: + return UpdateResult( + success=False, + skill_id=skill_id, + message=f"Failed to fetch from GitHub: {e}", + ) + finally: + if temp_dir and temp_dir.exists(): + shutil.rmtree(temp_dir, ignore_errors=True) + + +def update_all_skills( + *, + config: Config, + force: bool = False, + dry_run: bool = False, + skill_ids: list[str] | None = None, +) -> UpdateResult: + """Update all updatable skills (optionally limited to skill_ids). + + Returns a combined UpdateResult with all results. + """ + origins = get_all_origins(config=config) + + if skill_ids is not None: + origins = {k: v for k, v in origins.items() if k in skill_ids} + + if not origins: + return UpdateResult( + success=True, + skill_id="", + message="No skills to update", + ) + + updated: list[str] = [] + skipped: list[str] = [] + details: list[UpdateResultItem] = [] + errors: list[str] = [] + + for skill_id, origin in origins.items(): + kind = origin.get("kind", "") + + # Skip non-updatable origins + if kind == "builtin": + continue + + result = update_skill(skill_id, config=config, force=force, dry_run=dry_run) + + if result.updated: + updated.extend(result.updated) + if result.skipped: + skipped.extend(result.skipped) + if result.details: + details.extend(result.details) + if not result.success and not result.skipped: + errors.append(f"{skill_id}: {result.message}") + details.append( + UpdateResultItem( + skill_id=skill_id, + success=False, + message=result.message, + from_commit="", + to_commit="", + ) + ) + + # Build summary message + parts = [] + if updated: + parts.append(f"Updated {len(updated)} skill(s)") + if skipped: + parts.append(f"Skipped {len(skipped)} (up to date)") + if errors: + parts.append(f"{len(errors)} error(s)") + + message = ", ".join(parts) if parts else "No skills to update" + + return UpdateResult( + success=len(errors) == 0, + skill_id=",".join(updated) if updated else "", + message=message, + updated=updated, + skipped=skipped, + details=details, + errors=errors, + ) diff --git a/src/skillport/modules/skills/public/validation.py b/src/skillport/modules/skills/public/validation.py index 04648ce..a9af80e 100644 --- a/src/skillport/modules/skills/public/validation.py +++ b/src/skillport/modules/skills/public/validation.py @@ -4,6 +4,7 @@ from typing import Any from skillport.modules.skills.internal import validate_skill_record + from .types import SkillSummary, ValidationResult diff --git a/src/skillport/shared/__init__.py b/src/skillport/shared/__init__.py index 931ac4d..ace92ff 100644 --- a/src/skillport/shared/__init__.py +++ b/src/skillport/shared/__init__.py @@ -1,23 +1,23 @@ """Shared infrastructure for SkillPort.""" -from .config import Config, SKILLPORT_HOME +from .config import SKILLPORT_HOME, Config from .exceptions import ( - SkillPortError, - SkillNotFoundError, AmbiguousSkillError, - ValidationError, IndexingError, + SkillNotFoundError, + SkillPortError, SourceError, + ValidationError, ) from .filters import is_skill_enabled, normalize_token from .types import ( FrozenModel, + Namespace, Severity, - SourceType, - ValidationIssue, SkillId, SkillName, - Namespace, + SourceType, + ValidationIssue, ) from .utils import parse_frontmatter, resolve_inside diff --git a/src/skillport/shared/config.py b/src/skillport/shared/config.py index 5ac8637..25afb8f 100644 --- a/src/skillport/shared/config.py +++ b/src/skillport/shared/config.py @@ -5,14 +5,14 @@ prefixed with SKILLPORT_ (e.g., SKILLPORT_SKILLS_DIR). """ +import hashlib import json from pathlib import Path -import hashlib -from typing import Any, Literal, Tuple, Type +from typing import Any, Literal from pydantic import Field, field_validator, model_validator from pydantic.fields import FieldInfo -from pydantic_settings import BaseSettings, SettingsConfigDict, PydanticBaseSettingsSource +from pydantic_settings import BaseSettings, PydanticBaseSettingsSource, SettingsConfigDict from pydantic_settings.sources import EnvSettingsSource @@ -148,12 +148,12 @@ class Config(BaseSettings): @classmethod def settings_customise_sources( cls, - settings_cls: Type[BaseSettings], + settings_cls: type[BaseSettings], init_settings: PydanticBaseSettingsSource, env_settings: PydanticBaseSettingsSource, dotenv_settings: PydanticBaseSettingsSource, file_secret_settings: PydanticBaseSettingsSource, - ) -> Tuple[PydanticBaseSettingsSource, ...]: + ) -> tuple[PydanticBaseSettingsSource, ...]: """Use custom env source that handles comma-separated lists.""" return ( init_settings, diff --git a/src/skillport/shared/filters.py b/src/skillport/shared/filters.py index 20ac667..554e763 100644 --- a/src/skillport/shared/filters.py +++ b/src/skillport/shared/filters.py @@ -13,7 +13,7 @@ def normalize_token(value: str) -> str: return " ".join(str(value).strip().split()).lower() -def is_skill_enabled(skill_id: str, category: str | None, *, config: "Config") -> bool: +def is_skill_enabled(skill_id: str, category: str | None, *, config: Config) -> bool: """Check filters against config (skills, namespaces, categories). Args: diff --git a/src/skillport/shared/utils.py b/src/skillport/shared/utils.py index 7da93b5..ff91622 100644 --- a/src/skillport/shared/utils.py +++ b/src/skillport/shared/utils.py @@ -4,7 +4,7 @@ import os from pathlib import Path -from typing import Any, Dict, Tuple +from typing import Any import yaml @@ -12,7 +12,7 @@ from .filters import normalize_token -def parse_frontmatter(file_path: Path) -> Tuple[Dict[str, Any], str]: +def parse_frontmatter(file_path: Path) -> tuple[dict[str, Any], str]: """Parse a Markdown file with YAML frontmatter. Returns (metadata, body). If frontmatter is absent or invalid, metadata is {}. diff --git a/tests/integration/test_cli_commands.py b/tests/integration/test_cli_commands.py index e56e8a6..b186997 100644 --- a/tests/integration/test_cli_commands.py +++ b/tests/integration/test_cli_commands.py @@ -14,7 +14,6 @@ from skillport.modules.indexing import build_index from skillport.shared.config import Config - runner = CliRunner() diff --git a/tests/integration/test_search_and_tools_integration.py b/tests/integration/test_search_and_tools_integration.py index f88bbe2..b693b12 100644 --- a/tests/integration/test_search_and_tools_integration.py +++ b/tests/integration/test_search_and_tools_integration.py @@ -5,7 +5,7 @@ import pytest from skillport.modules.indexing import search as idx_search -from skillport.modules.skills import search_skills, read_skill_file +from skillport.modules.skills import read_skill_file, search_skills from skillport.shared.config import Config diff --git a/tests/pbt/test_name_properties.py b/tests/pbt/test_name_properties.py index b437136..9027906 100644 --- a/tests/pbt/test_name_properties.py +++ b/tests/pbt/test_name_properties.py @@ -8,16 +8,16 @@ import string -from hypothesis import given, strategies as st, assume, settings +from hypothesis import assume, given, settings +from hypothesis import strategies as st from skillport.modules.skills.internal.validation import ( - NAME_PATTERN, NAME_MAX_LENGTH, + NAME_PATTERN, NAME_RESERVED_WORDS, validate_skill_record, ) - # Strategy for valid skill names: lowercase letters, digits, hyphens valid_name_chars = st.sampled_from(string.ascii_lowercase + string.digits + "-") valid_name_strategy = st.text( diff --git a/tests/unit/test_add_logic.py b/tests/unit/test_add_logic.py index e2220f6..97e7978 100644 --- a/tests/unit/test_add_logic.py +++ b/tests/unit/test_add_logic.py @@ -1,14 +1,15 @@ """Unit tests for add command logic (SPEC2-CLI Section 3.3).""" -import pytest from pathlib import Path +import pytest + from skillport.modules.skills.internal.manager import ( - detect_skills, - add_local, - add_builtin, BUILTIN_SKILLS, _validate_skill_file, + add_builtin, + add_local, + detect_skills, ) from skillport.shared.config import Config diff --git a/tests/unit/test_add_namespace.py b/tests/unit/test_add_namespace.py index b65bddb..7049f02 100644 --- a/tests/unit/test_add_namespace.py +++ b/tests/unit/test_add_namespace.py @@ -1,5 +1,5 @@ -from skillport.modules.skills.internal.manager import detect_skills, add_local +from skillport.modules.skills.internal.manager import add_local, detect_skills from skillport.shared.config import Config diff --git a/tests/unit/test_auto_index.py b/tests/unit/test_auto_index.py index e4ce991..34dff62 100644 --- a/tests/unit/test_auto_index.py +++ b/tests/unit/test_auto_index.py @@ -1,7 +1,7 @@ """Unit tests for CLI auto index helpers.""" -from types import SimpleNamespace from pathlib import Path +from types import SimpleNamespace from skillport.interfaces.cli import auto_index from skillport.shared.config import Config diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 9922c6b..e8565d1 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -4,7 +4,7 @@ import pytest -from skillport.shared.config import Config, SKILLPORT_HOME +from skillport.shared.config import SKILLPORT_HOME, Config class TestConfigDefaults: diff --git a/tests/unit/test_core_skills_mode.py b/tests/unit/test_core_skills_mode.py index ceb60f3..c4d1480 100644 --- a/tests/unit/test_core_skills_mode.py +++ b/tests/unit/test_core_skills_mode.py @@ -2,9 +2,8 @@ from unittest.mock import MagicMock, patch - -from skillport.shared.config import Config from skillport.modules.indexing.public.query import get_core_skills +from skillport.shared.config import Config class TestGetCoreSkillsAutoMode: diff --git a/tests/unit/test_doc.py b/tests/unit/test_doc.py index d80ad00..c6ccf31 100644 --- a/tests/unit/test_doc.py +++ b/tests/unit/test_doc.py @@ -2,13 +2,12 @@ from pathlib import Path - from skillport.interfaces.cli.commands.doc import ( + MARKER_END, + MARKER_START, + _truncate_description, generate_skills_block, update_agents_md, - _truncate_description, - MARKER_START, - MARKER_END, ) from skillport.modules.skills import SkillSummary diff --git a/tests/unit/test_github_source.py b/tests/unit/test_github_source.py index 290bc51..0c5cb9a 100644 --- a/tests/unit/test_github_source.py +++ b/tests/unit/test_github_source.py @@ -1,16 +1,16 @@ """Unit tests for GitHub URL parsing and extraction (SPEC2-CLI Section 3.3).""" -import tarfile import io +import tarfile from pathlib import Path import pytest from skillport.modules.skills.internal.github import ( - parse_github_url, - extract_tarball, - ParsedGitHubURL, GITHUB_URL_RE, + ParsedGitHubURL, + extract_tarball, + parse_github_url, ) @@ -141,10 +141,11 @@ def test_extract_subpath(self, tmp_path): tar_path = _make_tar(tmp_path, structure) parsed = ParsedGitHubURL(owner="user", repo="repo", ref="main", path="/skills") - dest = extract_tarball(tar_path, parsed) + dest, commit_sha = extract_tarball(tar_path, parsed) assert (dest / "a" / "SKILL.md").exists() assert (dest / "b" / "SKILL.md").exists() + assert commit_sha == "sha" # From "owner-repo-sha" root def test_extract_rejects_symlink(self, tmp_path): """Symlinks in tarball → rejected.""" @@ -184,7 +185,7 @@ def test_extract_excludes_dotfiles(self, tmp_path): tar_path = _make_tar(tmp_path, structure) parsed = ParsedGitHubURL(owner="user", repo="repo", ref="main", path="/skills") - dest = extract_tarball(tar_path, parsed) + dest, _ = extract_tarball(tar_path, parsed) assert (dest / "a" / "SKILL.md").exists() assert not (dest / "a" / ".hidden").exists() @@ -199,7 +200,7 @@ def test_extract_root_path(self, tmp_path): tar_path = _make_tar(tmp_path, structure) parsed = ParsedGitHubURL(owner="user", repo="repo", ref="main", path="") - dest = extract_tarball(tar_path, parsed) + dest, _ = extract_tarball(tar_path, parsed) assert (dest / "skill-a" / "SKILL.md").exists() assert (dest / "skill-b" / "SKILL.md").exists() @@ -233,10 +234,11 @@ def test_extract_tarball_subpath(tmp_path): tar_path = _make_tar(tmp_path, structure) parsed = ParsedGitHubURL(owner="user", repo="repo", ref="main", path="/skills") - dest = extract_tarball(tar_path, parsed) + dest, commit_sha = extract_tarball(tar_path, parsed) assert (dest / "a" / "SKILL.md").exists() assert (dest / "b" / "SKILL.md").exists() + assert commit_sha == "sha" def test_extract_tarball_rejects_symlink(tmp_path): diff --git a/tests/unit/test_init_command.py b/tests/unit/test_init_command.py index 571415a..37d0ebd 100644 --- a/tests/unit/test_init_command.py +++ b/tests/unit/test_init_command.py @@ -5,9 +5,9 @@ import yaml from skillport.interfaces.cli.commands.init import ( - _create_skillportrc, - DEFAULT_SKILLS_DIRS, DEFAULT_INSTRUCTIONS, + DEFAULT_SKILLS_DIRS, + _create_skillportrc, ) diff --git a/tests/unit/test_normalization_and_threshold.py b/tests/unit/test_normalization_and_threshold.py index 6448e1d..263f0d9 100644 --- a/tests/unit/test_normalization_and_threshold.py +++ b/tests/unit/test_normalization_and_threshold.py @@ -1,6 +1,7 @@ from pathlib import Path -from hypothesis import given, settings, strategies as st +from hypothesis import given, settings +from hypothesis import strategies as st from skillport.modules.indexing.internal.lancedb import IndexStore from skillport.modules.indexing.internal.search_service import SearchService diff --git a/tests/unit/test_origin_v2.py b/tests/unit/test_origin_v2.py new file mode 100644 index 0000000..4daefed --- /dev/null +++ b/tests/unit/test_origin_v2.py @@ -0,0 +1,314 @@ +"""Unit tests for origin.json v2 functionality.""" + + +from skillport.modules.skills.internal import ( + compute_content_hash, + get_all_origins, + get_origin, + migrate_origin_v2, + prune_orphan_origins, + record_origin, + update_origin, +) +from skillport.shared.config import Config + + +class TestMigrateOriginV2: + """Tests for origin v1 to v2 migration.""" + + def test_migrate_adds_new_fields(self): + """v1 origin should get new v2 fields.""" + v1_origin = { + "source": "https://github.com/user/repo", + "kind": "github", + "added_at": "2025-01-01T00:00:00Z", + "skills_dir": "/path/to/skills", + "ref": "main", + } + + v2_origin = migrate_origin_v2(v1_origin) + + assert "content_hash" in v2_origin + assert "commit_sha" in v2_origin + assert "local_modified" in v2_origin + assert "update_history" in v2_origin + assert "updated_at" in v2_origin + + # Default values + assert v2_origin["content_hash"] == "" + assert v2_origin["commit_sha"] == "" + assert v2_origin["local_modified"] is False + assert v2_origin["update_history"] == [] + assert v2_origin["updated_at"] == "2025-01-01T00:00:00Z" # From added_at + + def test_migrate_preserves_existing_v2_fields(self): + """Already v2 origin should not be modified.""" + v2_origin = { + "source": "https://github.com/user/repo", + "kind": "github", + "added_at": "2025-01-01T00:00:00Z", + "content_hash": "sha256:abc123", + "commit_sha": "abc1234", + "local_modified": True, + "update_history": [{"from_commit": "old", "to_commit": "new"}], + "updated_at": "2025-01-02T00:00:00Z", + } + + result = migrate_origin_v2(v2_origin) + + assert result["content_hash"] == "sha256:abc123" + assert result["commit_sha"] == "abc1234" + assert result["local_modified"] is True + assert len(result["update_history"]) == 1 + + +class TestComputeContentHash: + """Tests for content hash computation.""" + + def test_compute_hash_for_skill(self, tmp_path): + """Compute SHA256 hash of skill directory.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: test\n---\nbody") + + hash_value = compute_content_hash(skill_dir) + + assert hash_value.startswith("sha256:") + assert len(hash_value) == 7 + 64 # "sha256:" + 64 hex chars + + def test_compute_hash_changes_with_other_files(self, tmp_path): + """Hash includes other files (not SKILL.md only).""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: test\n---\nbody") + + hash1 = compute_content_hash(skill_dir) + + # Add another file + (skill_dir / "helper.py").write_text("def foo(): pass") + hash2 = compute_content_hash(skill_dir) + + # Hash should change because helper.py is included + assert hash1 != hash2 + + def test_compute_hash_includes_subdirectories(self, tmp_path): + """Hash changes when files in subdirectories change.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: test\n---\nbody") + + hash1 = compute_content_hash(skill_dir) + + # Add file in subdirectory + lib_dir = skill_dir / "lib" + lib_dir.mkdir() + (lib_dir / "utils.py").write_text("# utils") + hash2 = compute_content_hash(skill_dir) + + assert hash1 != hash2 + + def test_compute_hash_excludes_hidden_files(self, tmp_path): + """Hash excludes hidden files and __pycache__.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: test\n---\nbody") + + hash1 = compute_content_hash(skill_dir) + + # Add hidden file and __pycache__ + (skill_dir / ".hidden").write_text("hidden") + pycache = skill_dir / "__pycache__" + pycache.mkdir() + (pycache / "module.pyc").write_bytes(b"\x00\x00") + + hash2 = compute_content_hash(skill_dir) + + # Hash should be same (hidden files excluded) + assert hash1 == hash2 + + def test_compute_hash_returns_empty_for_missing_skill(self, tmp_path): + """Missing SKILL.md returns empty string.""" + skill_dir = tmp_path / "no-skill" + skill_dir.mkdir() + + hash_value = compute_content_hash(skill_dir) + + assert hash_value == "" + + def test_compute_hash_is_deterministic(self, tmp_path): + """Same content produces same hash.""" + skill_dir = tmp_path / "skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("---\nname: test\n---\nbody") + + hash1 = compute_content_hash(skill_dir) + hash2 = compute_content_hash(skill_dir) + + assert hash1 == hash2 + + def test_different_content_different_hash(self, tmp_path): + """Different content produces different hash.""" + skill1 = tmp_path / "skill1" + skill1.mkdir() + (skill1 / "SKILL.md").write_text("content A") + + skill2 = tmp_path / "skill2" + skill2.mkdir() + (skill2 / "SKILL.md").write_text("content B") + + assert compute_content_hash(skill1) != compute_content_hash(skill2) + + +class TestGetOrigin: + """Tests for get_origin function.""" + + def test_get_origin_returns_none_for_missing(self, tmp_path): + """Non-existent skill returns None.""" + config = Config(skills_dir=tmp_path / "skills", db_path=tmp_path / "db.lancedb") + + result = get_origin("nonexistent", config=config) + + assert result is None + + def test_get_origin_returns_migrated_v2(self, tmp_path): + """Returned origin is migrated to v2.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + # Record a v1-style origin + record_origin("my-skill", {"source": "test", "kind": "local"}, config=config) + + result = get_origin("my-skill", config=config) + + assert result is not None + assert "content_hash" in result # v2 field present + + +class TestUpdateOrigin: + """Tests for update_origin function.""" + + def test_update_origin_merges_fields(self, tmp_path): + """Update origin merges new fields into existing.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + # Record initial origin + record_origin("my-skill", {"source": "test", "kind": "github"}, config=config) + + # Update with new fields + update_origin( + "my-skill", + {"content_hash": "sha256:new", "commit_sha": "abc123"}, + config=config, + ) + + result = get_origin("my-skill", config=config) + + assert result["source"] == "test" # Preserved + assert result["kind"] == "github" # Preserved + assert result["content_hash"] == "sha256:new" # Updated + assert result["commit_sha"] == "abc123" # Updated + + def test_update_origin_adds_history_entry(self, tmp_path): + """History entry is prepended correctly.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin("my-skill", {"source": "test", "kind": "github"}, config=config) + + update_origin( + "my-skill", + {"commit_sha": "new123"}, + config=config, + add_history_entry={"from_commit": "old", "to_commit": "new123"}, + ) + + result = get_origin("my-skill", config=config) + + assert len(result["update_history"]) == 1 + assert result["update_history"][0]["from_commit"] == "old" + assert result["update_history"][0]["to_commit"] == "new123" + + def test_update_origin_history_rotation(self, tmp_path): + """History is limited to MAX_UPDATE_HISTORY entries.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin("my-skill", {"source": "test", "kind": "github"}, config=config) + + # Add 15 history entries + for i in range(15): + update_origin( + "my-skill", + {"commit_sha": f"commit{i}"}, + config=config, + add_history_entry={"from_commit": f"from{i}", "to_commit": f"to{i}"}, + ) + + result = get_origin("my-skill", config=config) + + # Should be limited to 10 + assert len(result["update_history"]) == 10 + # Most recent should be first + assert result["update_history"][0]["to_commit"] == "to14" + + +class TestGetAllOrigins: + """Tests for get_all_origins function.""" + + def test_get_all_origins_empty(self, tmp_path): + """Empty origins returns empty dict.""" + config = Config(skills_dir=tmp_path / "skills", db_path=tmp_path / "db.lancedb") + + result = get_all_origins(config=config) + + assert result == {} + + def test_get_all_origins_returns_all_migrated(self, tmp_path): + """All origins are returned and migrated to v2.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin("skill-a", {"source": "a", "kind": "github"}, config=config) + record_origin("skill-b", {"source": "b", "kind": "local"}, config=config) + + result = get_all_origins(config=config) + + assert "skill-a" in result + assert "skill-b" in result + assert "content_hash" in result["skill-a"] # v2 migrated + assert "content_hash" in result["skill-b"] # v2 migrated + + +class TestPruneOrphanOrigins: + """Tests for pruning orphan origins.""" + + def test_prune_removes_missing_skill_dirs(self, tmp_path): + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + # existing skill + skill_a = skills_dir / "skill-a" + skill_a.mkdir() + (skill_a / "SKILL.md").write_text("a") + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin("skill-a", {"source": "a", "kind": "local"}, config=config) + record_origin("skill-b", {"source": "b", "kind": "github"}, config=config) + + removed = prune_orphan_origins(config=config) + + assert "skill-b" in removed + assert get_origin("skill-b", config=config) is None + assert get_origin("skill-a", config=config) is not None diff --git a/tests/unit/test_update.py b/tests/unit/test_update.py new file mode 100644 index 0000000..1daf0cf --- /dev/null +++ b/tests/unit/test_update.py @@ -0,0 +1,404 @@ +"""Unit tests for skill update functionality.""" + + +from skillport.modules.skills import ( + check_update_available, + detect_local_modification, + update_skill, +) +from skillport.modules.skills.internal import ( + compute_content_hash, + record_origin, +) +from skillport.shared.config import Config + + +class TestDetectLocalModification: + """Tests for local modification detection.""" + + def test_no_origin_returns_false(self, tmp_path): + """No origin info means no tracking, returns False.""" + config = Config(skills_dir=tmp_path / "skills", db_path=tmp_path / "db.lancedb") + + result = detect_local_modification("nonexistent", config=config) + + assert result is False + + def test_no_content_hash_returns_false(self, tmp_path): + """Origin without content_hash (v1) returns False.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + # Record origin without content_hash (simulating v1) + record_origin("my-skill", {"source": "test", "kind": "local"}, config=config) + + result = detect_local_modification("my-skill", config=config) + + # Migration adds empty content_hash, which means "unknown", so no modification detected + assert result is False + + def test_matching_hash_returns_false(self, tmp_path): + """Matching content_hash means no modification.""" + skills_dir = tmp_path / "skills" + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\n---\nbody") + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + content_hash = compute_content_hash(skill_dir) + record_origin( + "my-skill", + {"source": str(skill_dir), "kind": "local", "content_hash": content_hash}, + config=config, + ) + + result = detect_local_modification("my-skill", config=config) + + assert result is False + + def test_different_hash_returns_true(self, tmp_path): + """Different content_hash means modification detected.""" + skills_dir = tmp_path / "skills" + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\n---\nbody") + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin( + "my-skill", + {"source": str(skill_dir), "kind": "local", "content_hash": "sha256:old_hash"}, + config=config, + ) + + result = detect_local_modification("my-skill", config=config) + + assert result is True + + +class TestCheckUpdateAvailable: + """Tests for check_update_available function.""" + + def test_no_origin_not_available(self, tmp_path): + """No origin info means not updatable.""" + config = Config(skills_dir=tmp_path / "skills", db_path=tmp_path / "db.lancedb") + + result = check_update_available("nonexistent", config=config) + + assert result["available"] is False + assert "no origin" in result["reason"].lower() + + def test_builtin_not_available(self, tmp_path): + """Builtin skills cannot be updated.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin("hello-world", {"source": "hello-world", "kind": "builtin"}, config=config) + + result = check_update_available("hello-world", config=config) + + assert result["available"] is False + assert "built-in" in result["reason"].lower() + + def test_local_missing_source_not_available(self, tmp_path): + """Local skill with missing source path is not updatable.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin( + "my-skill", + {"source": "/nonexistent/path", "kind": "local"}, + config=config, + ) + + result = check_update_available("my-skill", config=config) + + assert result["available"] is False + assert "not found" in result["reason"].lower() + + def test_local_with_source_available(self, tmp_path): + """Local skill with valid source is updatable.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + source_dir = tmp_path / "source" + source_dir.mkdir() + (source_dir / "SKILL.md").write_text("source body") + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + # installed copy differs + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("installed body") + + record_origin( + "my-skill", + {"source": str(source_dir), "kind": "local"}, + config=config, + ) + + result = check_update_available("my-skill", config=config) + + assert result["available"] is True + + def test_github_same_content_not_available(self, tmp_path, monkeypatch): + """GitHub skill with same tree hash is up to date.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin( + "my-skill", + { + "source": "https://github.com/user/repo", + "kind": "github", + "commit_sha": "abc1234567890", + }, + config=config, + ) + + # create installed content + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("body") + + def mock_get_remote_tree_hash(parsed, token, path=None): + return compute_content_hash(skill_dir) + + from skillport.modules.skills.public import update as update_module + + monkeypatch.setattr(update_module, "get_remote_tree_hash", mock_get_remote_tree_hash) + + result = check_update_available("my-skill", config=config) + + assert result["available"] is False + assert "latest" in result["reason"].lower() + + def test_github_different_content_available(self, tmp_path, monkeypatch): + """GitHub skill with different tree hash has update available.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin( + "my-skill", + { + "source": "https://github.com/user/repo", + "kind": "github", + "commit_sha": "abc1234567890", + }, + config=config, + ) + + # create installed content + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("old") + + from skillport.modules.skills.public import update as update_module + + def mock_get_remote_tree_hash(parsed, token, path=None): + return "sha256:remotehash" + + monkeypatch.setattr(update_module, "get_remote_tree_hash", mock_get_remote_tree_hash) + + result = check_update_available("my-skill", config=config) + + assert result["available"] is True + assert "remote" in result["reason"].lower() + + def test_github_api_failure_not_available(self, tmp_path, monkeypatch): + """GitHub API failure should not mark as available immediately after add.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin( + "my-skill", + { + "source": "https://github.com/user/repo", + "kind": "github", + "commit_sha": "abc1234567890", + }, + config=config, + ) + + # Mock get_remote_tree_hash to return empty string (API failure) + def mock_get_remote_tree_hash(parsed, token, path=None): + return "" + + from skillport.modules.skills.public import update as update_module + + monkeypatch.setattr(update_module, "get_remote_tree_hash", mock_get_remote_tree_hash) + + result = check_update_available("my-skill", config=config) + + assert result["available"] is False + assert "remote tree" in result["reason"].lower() + + +class TestUpdateSkill: + """Tests for update_skill function.""" + + def test_update_nonexistent_skill_fails(self, tmp_path): + """Updating non-existent skill fails.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + result = update_skill("nonexistent", config=config) + + assert result.success is False + assert "not found" in result.message.lower() + + def test_update_skill_without_origin_fails(self, tmp_path): + """Updating skill without origin fails.""" + skills_dir = tmp_path / "skills" + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\n---\nbody") + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + result = update_skill("my-skill", config=config) + + assert result.success is False + assert "no origin" in result.message.lower() + + def test_update_builtin_fails(self, tmp_path): + """Updating builtin skill fails.""" + skills_dir = tmp_path / "skills" + skill_dir = skills_dir / "hello-world" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("---\nname: hello-world\n---\nbody") + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + record_origin("hello-world", {"source": "hello-world", "kind": "builtin"}, config=config) + + result = update_skill("hello-world", config=config) + + assert result.success is False + assert "built-in" in result.message.lower() + + def test_update_local_modified_without_force_fails(self, tmp_path): + """Updating locally modified skill without force fails.""" + skills_dir = tmp_path / "skills" + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\n---\nmodified body") + + source_dir = tmp_path / "source" + source_dir.mkdir() + (source_dir / "SKILL.md").write_text("---\nname: my-skill\n---\noriginal body") + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + # Record with original hash + original_hash = compute_content_hash(source_dir) + record_origin( + "my-skill", + {"source": str(source_dir), "kind": "local", "content_hash": original_hash}, + config=config, + ) + + result = update_skill("my-skill", config=config) + + assert result.success is False + assert result.local_modified is True + assert "--force" in result.message + + def test_update_local_modified_with_force_succeeds(self, tmp_path): + """Updating locally modified skill with force succeeds.""" + skills_dir = tmp_path / "skills" + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\n---\nmodified body") + + source_dir = tmp_path / "source" / "my-skill" + source_dir.mkdir(parents=True) + (source_dir / "SKILL.md").write_text("---\nname: my-skill\n---\nnew body") + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + # Record with different hash + record_origin( + "my-skill", + {"source": str(source_dir), "kind": "local", "content_hash": "sha256:old"}, + config=config, + ) + + result = update_skill("my-skill", config=config, force=True) + + assert result.success is True + assert "my-skill" in result.updated + + # Verify content was updated + assert (skill_dir / "SKILL.md").read_text() == "---\nname: my-skill\n---\nnew body" + + def test_update_local_already_up_to_date(self, tmp_path): + """Local skill with matching hash is already up to date.""" + skills_dir = tmp_path / "skills" + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\n---\nbody") + + source_dir = tmp_path / "source" / "my-skill" + source_dir.mkdir(parents=True) + (source_dir / "SKILL.md").write_text("---\nname: my-skill\n---\nbody") # Same content + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + content_hash = compute_content_hash(skill_dir) + record_origin( + "my-skill", + {"source": str(source_dir), "kind": "local", "content_hash": content_hash}, + config=config, + ) + + result = update_skill("my-skill", config=config) + + assert result.success is True + assert "my-skill" in result.skipped + assert "up to date" in result.message.lower() + + def test_update_dry_run_no_changes(self, tmp_path): + """Dry run shows what would be updated without changes.""" + skills_dir = tmp_path / "skills" + skill_dir = skills_dir / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\n---\nold body") + + source_dir = tmp_path / "source" / "my-skill" + source_dir.mkdir(parents=True) + (source_dir / "SKILL.md").write_text("---\nname: my-skill\n---\nnew body") + + config = Config(skills_dir=skills_dir, db_path=tmp_path / "db.lancedb") + + content_hash = compute_content_hash(skill_dir) + record_origin( + "my-skill", + {"source": str(source_dir), "kind": "local", "content_hash": content_hash}, + config=config, + ) + + result = update_skill("my-skill", config=config, dry_run=True) + + assert result.success is True + assert "my-skill" in result.updated + assert "would" in result.message.lower() + + # Content should NOT be changed + assert (skill_dir / "SKILL.md").read_text() == "---\nname: my-skill\n---\nold body" diff --git a/tests/unit/test_validation_rules.py b/tests/unit/test_validation_rules.py index 5282c13..f1fcc4c 100644 --- a/tests/unit/test_validation_rules.py +++ b/tests/unit/test_validation_rules.py @@ -3,13 +3,13 @@ import pytest from skillport.modules.skills.internal.validation import ( - validate_skill_record, + ALLOWED_FRONTMATTER_KEYS, + DESCRIPTION_MAX_LENGTH, NAME_MAX_LENGTH, NAME_PATTERN, NAME_RESERVED_WORDS, SKILL_LINE_THRESHOLD, - DESCRIPTION_MAX_LENGTH, - ALLOWED_FRONTMATTER_KEYS, + validate_skill_record, ) diff --git a/tests/unit/test_xml_instructions.py b/tests/unit/test_xml_instructions.py index 2a01c90..12e8606 100644 --- a/tests/unit/test_xml_instructions.py +++ b/tests/unit/test_xml_instructions.py @@ -2,10 +2,9 @@ from unittest.mock import patch - from skillport.interfaces.mcp.instructions import ( - build_xml_instructions, _escape_xml, + build_xml_instructions, ) from skillport.shared.config import Config diff --git a/uv.lock b/uv.lock index f8290f1..6f3c8ea 100644 --- a/uv.lock +++ b/uv.lock @@ -1836,7 +1836,7 @@ wheels = [ [[package]] name = "skillport" -version = "0.1.5" +version = "0.1.6" source = { editable = "." } dependencies = [ { name = "fastmcp" }, diff --git a/verify_server.py b/verify_server.py index 5064b29..1b723b6 100644 --- a/verify_server.py +++ b/verify_server.py @@ -1,6 +1,7 @@ import asyncio import os import tempfile + from mcp import ClientSession, StdioServerParameters from mcp.client.stdio import stdio_client