diff --git a/README.md b/README.md index cada9528b..46d7291cb 100644 --- a/README.md +++ b/README.md @@ -579,6 +579,7 @@ These are only needed for **headless / CI extraction** (`graphify extract`). Whe | `GRAPHIFY_QUERY_LOG` | Enable the query log and write it to this path instead of the default | optional — off unless this or `_ENABLE` is set | | `GRAPHIFY_QUERY_LOG_DISABLE` | Set to `1` to force the query log off (wins over the enable vars) | optional | | `GRAPHIFY_QUERY_LOG_RESPONSES` | When the log is enabled, also record full subgraph responses (off by default) | optional | +| `GRAPHIFY_NO_AUTO_REFRESH` | Set to `1` to stop the CLI from refreshing installed skills that are older than the package after an upgrade | optional — refresh is on by default | | `GRAPHIFY_MAX_GRAPH_BYTES` | Override the 512 MiB graph.json size cap — e.g. `700MB`, `2GB`, or plain bytes | optional — useful for very large corpora | | `GRAPHIFY_MAX_CONTEXTS` | Maximum number of non-default project graphs retained by one multi-project MCP server | optional — default: `8`; invalid values use `8`, and values below `1` use `1` | | `GRAPHIFY_LLM_TEMPERATURE` | Override LLM temperature for semantic extraction — e.g. `0.7`, or `none` to omit | optional — auto-omitted for o1/o3/o4/gpt-5 reasoning models | @@ -674,10 +675,10 @@ ANTHROPIC_API_KEY=sk-... graphify extract ./docs --backend claude ``` **Skill version mismatch warning in your IDE** -Your installed graphify version is different from the skill file. Update: +Your installed graphify version is different from the skill file. After an upgrade, the first `graphify` command refreshes every installed skill that is older than the package, on all platforms at once, and keeps a locally edited `SKILL.md` as `SKILL.md.bak`. Set `GRAPHIFY_NO_AUTO_REFRESH=1` to turn this off. A skill it can't refresh safely keeps the warning: one newer than the package, or a directory that two installers share (for example Copilot and `graphify vscode install`). Reinstall that platform by hand: ```bash uv tool upgrade graphifyy -graphify install # overwrites the skill file +graphify install --platform # overwrites the skill file ``` **Claude Code prompt cache invalidated after every `graphify extract`** diff --git a/graphify/__main__.py b/graphify/__main__.py index 736f9f1e9..93a36f266 100644 --- a/graphify/__main__.py +++ b/graphify/__main__.py @@ -1,8 +1,10 @@ """graphify CLI - `graphify install` sets up the Claude Code skill.""" from __future__ import annotations +import contextlib import errno import functools +import io import json import os import platform @@ -85,6 +87,7 @@ from graphify.install import ( # noqa: E402,F401 _uninstall_gemini_hook, _uninstall_kilo_plugin, _uninstall_opencode_plugin, + _vscode_skill_destination, claude_install, claude_uninstall, codebuddy_install, @@ -116,6 +119,7 @@ from graphify.install import ( # noqa: E402,F401 _OPENCODE_PLUGIN_PATH, _OPENCODE_CONFIG_PATH, _PLATFORM_CONFIG, + _skill_lock, ) from graphify.cli import ( # noqa: E402,F401 dispatch_command, @@ -175,7 +179,7 @@ def _check_skill_version(skill_dst: Path, platform_names: "list[str] | None" = N if platform_names is None: try: platform_names = [ - name for name in _PLATFORM_CONFIG + name for name in _skill_platforms() if _platform_skill_destination(name) == skill_dst ] except Exception: @@ -253,6 +257,129 @@ def _version_tuple(version: str) -> tuple[int, ...]: return tuple(parts) +def _refresh_stale_skills() -> None: + """Refresh user-scope skills left behind by a package upgrade (#1805). + + Upgrading the package (``uv tool upgrade graphifyy``, ``pip install -U``) + does not touch the installed skill copies, and a plain ``graphify install`` + refreshes only the detected platform, so every other platform (codex, + opencode, gemini, ...) kept running the old skill instructions until each + was reinstalled by hand. On the first CLI run after an upgrade, re-copy + every user-scope skill whose ``.graphify_version`` stamp is older than the + running package - exactly the copies the stale-skill warning would tell the + user to reinstall. + + Only copies graphify installed are touched (a stamp and SKILL.md exist); + nothing is installed that was not there before. Left alone, so the + warning still fires for them: + + * a stamp NEWER than the package - installing would downgrade it (#1568); + * a directory written by more than one installer (gemini + agents on + Windows, copilot + ``graphify vscode install``) - the stamp does not + record which variant is there, and guessing would swap it. + + Directories are grouped by their resolved path, so two platforms sharing + one directory through a symlink count as shared too. + + A locally edited SKILL.md is kept as ``SKILL.md.bak`` by _copy_skill_file + (#3144). Output goes to stderr so piped stdout (``--json``, the MCP stdio + server) stays clean. Set ``GRAPHIFY_NO_AUTO_REFRESH=1`` to opt out, e.g. + for pinned or centrally managed skill directories. + """ + if os.environ.get("GRAPHIFY_NO_AUTO_REFRESH", "").strip().lower() in ("1", "true", "yes"): + return + if __version__ == "unknown": + return + # 'claude'/'windows' and 'antigravity'/'antigravity-windows' write one + # directory and differ only in the host shell; keep the variant a plain + # `graphify install` picks on this OS rather than calling the pair ambiguous. + if platform.system() == "Windows": + other_os = {"claude", "antigravity"} + else: + other_os = {"windows", "antigravity-windows"} + candidates = [("vscode", _vscode_skill_destination)] + candidates += [ + (name, functools.partial(_platform_skill_destination, name)) + for name in _skill_platforms() if name not in other_os + ] + writers: dict[Path, list[tuple[str, Path]]] = {} + for name, destination in candidates: + try: + skill_dst = destination() + except Exception: + continue # one unresolvable platform must not cancel the others + try: + real = skill_dst.resolve() + except (OSError, RuntimeError): # symlink loop: the is_file checks skip it + real = skill_dst + writers.setdefault(real, []).append((name, skill_dst)) + for entries in writers.values(): + if len(entries) != 1 or entries[0][0] == "vscode": + continue + name, skill_dst = entries[0] + if _stale_stamp(skill_dst) is None: + continue + # Without waiting: when another process holds the directory (a refresh, + # an install, an uninstall) this one skips it and leaves it to them. + with _skill_lock(skill_dst.parent, wait=False) as owned: + # Read the stamp again under the lock: a process that refreshed + # this directory in the meantime has already brought it current, + # and one that uninstalled it has left nothing to refresh. + installed = _stale_stamp(skill_dst) if owned else None + if installed is not None: + _refresh_one_skill(name, skill_dst, installed) + + +def _stale_stamp(skill_dst: Path) -> str | None: + """The version stamped beside ``skill_dst`` if it is older than the package.""" + version_file = skill_dst.parent / ".graphify_version" + try: + if not (version_file.is_file() and skill_dst.is_file()): + return None + installed = version_file.read_text(encoding="utf-8").strip() + except OSError: + return None + if installed == __version__ or _version_tuple(installed) > _version_tuple(__version__): + return None + return installed + + +# gemini installs through gemini_install, not _PLATFORM_CONFIG, but +# _platform_skill_destination and _copy_skill_file both handle it. +def _skill_platforms() -> list[str]: + """Every platform with a user-scope skill directory, gemini included.""" + return list(dict.fromkeys([*_PLATFORM_CONFIG, "gemini"])) + + +def _refresh_one_skill(name: str, skill_dst: Path, installed: str) -> None: + """Re-copy one platform's skill and report it in one line on stderr.""" + backup = skill_dst.with_suffix(skill_dst.suffix + ".bak") + captured = io.StringIO() + try: + with contextlib.redirect_stdout(captured): + _copy_skill_file(name) + except (Exception, SystemExit) as exc: + # _copy_skill_file exits on a broken package (after printing why); + # a refresh must never take the user's actual command down with it. + # The stamp stays old, so the version warning names the manual fix. + detail = "" if isinstance(exc, SystemExit) else f": {exc}" + print( + f"graphify: could not refresh skill at {skill_dst.parent} " + f"({installed} -> {__version__}){detail}", + file=sys.stderr, + ) + return + print( + f"graphify: refreshed skill at {skill_dst.parent} ({installed} -> {__version__}); " + f"set GRAPHIFY_NO_AUTO_REFRESH=1 to disable", + file=sys.stderr, + ) + # Of _copy_skill_file's per-file lines, only the backup notice matters here. + for line in captured.getvalue().splitlines(): + if str(backup) in line: + print(line, file=sys.stderr) + + @@ -593,10 +720,14 @@ def _run_cli() -> None: # Deduplicate paths so platforms sharing the same install dir don't warn twice. _silent_cmds = {"install", "uninstall", "hook-check", "hook-guard"} if not any(arg in _silent_cmds for arg in sys.argv): + # A package upgrade leaves every installed skill at the old version; + # refresh them first, so the check below only fires for copies the + # refresh could not or must not touch (#1805). + _refresh_stale_skills() # Resolve each platform's real user-scope destination so per-platform # overrides (gemini, opencode, devin, antigravity, amp) check the dir # they actually install into, not the bare cfg['skill_dst']. - for skill_dst in {_platform_skill_destination(name) for name in _PLATFORM_CONFIG}: + for skill_dst in {_platform_skill_destination(name) for name in _skill_platforms()}: _check_skill_version(skill_dst) if len(sys.argv) >= 2 and sys.argv[1] in ("-v", "--version", "version"): diff --git a/graphify/install.py b/graphify/install.py index 47def8454..b01d887c8 100644 --- a/graphify/install.py +++ b/graphify/install.py @@ -11,7 +11,10 @@ installers resolve packaged assets via `Path(__file__).parent / "always_on"` and """ from __future__ import annotations +import contextlib +import errno import functools +import hashlib import json import os import platform @@ -19,9 +22,18 @@ import re import shutil import stat import sys +import threading +import time from pathlib import Path from typing import NoReturn +if sys.platform == "win32": + import msvcrt + fcntl = None +else: + import fcntl + msvcrt = None + try: from importlib.metadata import version as _pkg_version @@ -33,6 +45,106 @@ from graphify.paths import GRAPHIFY_OUT as _GRAPHIFY_OUT from graphify.paths import os_replace_with_fallback as _os_replace_with_fallback +def _skill_lock_path(skill_dir: Path) -> Path: + """The lock file for ``skill_dir``, kept out of the skill directory. + + Inside it, a project-scope install would leave the file in the user's + repository, and uninstall would have to delete a file another process may + still hold - letting that process and a new one both own the lock. + """ + try: + real = skill_dir.resolve() + except (OSError, RuntimeError): # symlink loop + real = skill_dir.absolute() + digest = hashlib.sha256(os.path.normcase(str(real)).encode("utf-8")).hexdigest()[:16] + return Path.home() / ".cache" / "graphify" / "skill-locks" / f"{digest}.lock" + + +# Lock files this thread holds: the CLI's refresh takes the lock and then calls +# _copy_skill_file, which takes it again. Per thread, so another thread of the +# same process still waits its turn. +_held_skill_locks = threading.local() + + +def _try_skill_lock(fh) -> bool: + """Take the lock on ``fh`` without waiting; False if another process holds it. + + Raises OSError when the file system cannot lock at all (e.g. some network + mounts). + """ + try: + if fcntl is not None: + fcntl.flock(fh.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) + else: + fh.seek(0) + msvcrt.locking(fh.fileno(), msvcrt.LK_NBLCK, 1) + except OSError as exc: + if exc.errno in (errno.EAGAIN, errno.EWOULDBLOCK, errno.EACCES): + return False + raise + return True + + +def _release_skill_lock(fh) -> None: + with contextlib.suppress(OSError): + if fcntl is not None: + fcntl.flock(fh.fileno(), fcntl.LOCK_UN) + else: + fh.seek(0) + msvcrt.locking(fh.fileno(), msvcrt.LK_UNLCK, 1) + + +@contextlib.contextmanager +def _skill_lock(skill_dir: Path, *, wait: bool = True): + """Serialize every process that writes ``skill_dir``. + + Install, uninstall and the CLI's stale-skill refresh (#1805) all write the + same files and stage through fixed ``SKILL.md.tmp`` / ``references.tmp`` + names, so two of them at once would write through each other - and an + uninstall racing a refresh could see the refresh put the skill back. + + Yields True while this process holds the lock. With ``wait`` off (the + refresh) it yields False instead of waiting when another process holds it. + It yields None when locking is unavailable here (no writable cache + directory, a file system that refuses to lock): install and uninstall then + go ahead unlocked, as they did before the lock existed; the refresh skips. + + An OS lock (flock / msvcrt.locking): the OS releases it however the holder + exits, so a crash or a kill never leaves the directory locked and there is + no staleness timeout to guess. The lock file is never deleted. + """ + path = _skill_lock_path(skill_dir) + held = _held_skill_locks.__dict__.setdefault("paths", set()) + if path in held: + yield True + return + try: + path.parent.mkdir(parents=True, exist_ok=True) + fh = open(path, "a+b") + except OSError: + fh = None + if fh is None: + yield None + return + with fh: + try: + locked = _try_skill_lock(fh) + while not locked and wait: + time.sleep(0.05) + locked = _try_skill_lock(fh) + except OSError: + locked = None + if not locked: + yield locked + return + held.add(path) + try: + yield True + finally: + held.discard(path) + _release_skill_lock(fh) + + def _write_version_stamp(skill_dst: Path, version: str) -> None: """Atomically write ``.graphify_version`` beside ``skill_dst``. @@ -219,70 +331,72 @@ def _copy_skill_file(platform_name: str, *, project: bool = False, project_dir: sys.exit(1) skill_dst = _platform_skill_destination(platform_name, project=project, project_dir=project_dir) - skill_dst.parent.mkdir(parents=True, exist_ok=True) + with _skill_lock(skill_dst.parent): + skill_dst.parent.mkdir(parents=True, exist_ok=True) - # Install the references/ sidecar (or clear an orphan one) BEFORE writing - # SKILL.md, so SKILL.md is the last artifact laid down. An install that is - # interrupted partway then leaves no SKILL.md rather than a SKILL.md that - # points at an absent references/ dir. - if refs_src is not None: - _install_skill_references(skill_dst, refs_src) - print(f" references -> {skill_dst.parent / 'references'}") - else: - # Monolith (or progressive-with-no-refs): clear any orphan references/. - orphan_refs = skill_dst.parent / "references" - if orphan_refs.exists(): - shutil.rmtree(orphan_refs) + # Install the references/ sidecar (or clear an orphan one) BEFORE writing + # SKILL.md, so SKILL.md is the last artifact laid down. An install that is + # interrupted partway then leaves no SKILL.md rather than a SKILL.md that + # points at an absent references/ dir. + if refs_src is not None: + _install_skill_references(skill_dst, refs_src) + print(f" references -> {skill_dst.parent / 'references'}") + else: + # Monolith (or progressive-with-no-refs): clear any orphan references/. + orphan_refs = skill_dst.parent / "references" + if orphan_refs.exists(): + shutil.rmtree(orphan_refs) - # A SKILL.md that differs from what is about to be written may carry the - # user's local edits (a tuned description:, extra guidance); replacing it - # wholesale with only "skill installed ->" for output read like a no-op - # while the edits were gone (#3144). Keep one .bak beside it and say so. - # Every upgrade differs too - the .bak is overwritten each install, so it - # always holds exactly the previous copy. - try: - if skill_dst.exists() and skill_dst.read_bytes() != skill_src.read_bytes(): - backup = skill_dst.with_suffix(skill_dst.suffix + ".bak") - shutil.copy2(skill_dst, backup) - print(f" previous copy -> {backup} (differed from the packaged skill)") - except OSError: - pass # a failed backup must not block the install - # SKILL.md last (crash-safety), via an atomic temp + rename. - tmp_dst = skill_dst.with_suffix(skill_dst.suffix + ".tmp") - try: - shutil.copy(skill_src, tmp_dst) - _os_replace_with_fallback(tmp_dst, skill_dst) - except Exception: + # A SKILL.md that differs from what is about to be written may carry the + # user's local edits (a tuned description:, extra guidance); replacing it + # wholesale with only "skill installed ->" for output read like a no-op + # while the edits were gone (#3144). Keep one .bak beside it and say so. + # Every upgrade differs too - the .bak is overwritten each install, so it + # always holds exactly the previous copy. try: - tmp_dst.unlink(missing_ok=True) + if skill_dst.exists() and skill_dst.read_bytes() != skill_src.read_bytes(): + backup = skill_dst.with_suffix(skill_dst.suffix + ".bak") + shutil.copy2(skill_dst, backup) + print(f" previous copy -> {backup} (differed from the packaged skill)") except OSError: - pass - raise + pass # a failed backup must not block the install + # SKILL.md last (crash-safety), via an atomic temp + rename. + tmp_dst = skill_dst.with_suffix(skill_dst.suffix + ".tmp") + try: + shutil.copy(skill_src, tmp_dst) + _os_replace_with_fallback(tmp_dst, skill_dst) + except Exception: + try: + tmp_dst.unlink(missing_ok=True) + except OSError: + pass + raise - _write_version_stamp(skill_dst, __version__) - print(f" skill installed -> {skill_dst}") + _write_version_stamp(skill_dst, __version__) + print(f" skill installed -> {skill_dst}") return skill_dst def _remove_skill_file(platform_name: str, *, project: bool = False, project_dir: Path | None = None) -> bool: """Remove a platform skill file and its version stamp without touching other scopes.""" skill_dst = _platform_skill_destination(platform_name, project=project, project_dir=project_dir) removed = False - if skill_dst.exists(): - skill_dst.unlink() - print(f" skill removed -> {skill_dst}") - removed = True - version_file = skill_dst.parent / ".graphify_version" - if version_file.exists(): - version_file.unlink() - removed = True - refs_dir = skill_dst.parent / "references" - if refs_dir.exists(): - shutil.rmtree(refs_dir) - removed = True - for d in (skill_dst.parent, skill_dst.parent.parent, skill_dst.parent.parent.parent): - try: - d.rmdir() - except OSError: - break + with _skill_lock(skill_dst.parent): + if skill_dst.exists(): + skill_dst.unlink() + print(f" skill removed -> {skill_dst}") + removed = True + version_file = skill_dst.parent / ".graphify_version" + if version_file.exists(): + version_file.unlink() + removed = True + refs_dir = skill_dst.parent / "references" + if refs_dir.exists(): + shutil.rmtree(refs_dir) + removed = True + for d in (skill_dst.parent, skill_dst.parent.parent, skill_dst.parent.parent.parent): + try: + d.rmdir() + except OSError: + break return removed def _project_scope_root(path: Path, project_dir: Path) -> Path: """Return the top-level project artifact for a project-scoped skill path.""" @@ -940,6 +1054,14 @@ def gemini_uninstall(project_dir: Path | None = None, *, project: bool = False, print(f"GEMINI.md was empty after removal - deleted {target.resolve()}") _uninstall_gemini_hook(project_dir) _VSCODE_INSTRUCTIONS_MARKER = "## graphify" +def _vscode_skill_destination() -> Path: + """Skill path written by ``graphify vscode install``. + + The same directory as the ``copilot`` platform's user-scope skill, but with + the VS Code variant of SKILL.md, so the version stamp there cannot tell which + of the two is installed. + """ + return Path.home() / ".copilot" / "skills" / "graphify" / "SKILL.md" def vscode_install(project_dir: Path | None = None) -> None: """Install graphify skill for VS Code Copilot Chat + write .github/copilot-instructions.md.""" skill_src = Path(__file__).parent / "skill-vscode.md" @@ -947,29 +1069,30 @@ def vscode_install(project_dir: Path | None = None) -> None: if not skill_src.exists(): skill_src = Path(__file__).parent / "skill-copilot.md" refs_bundle = "copilot" - skill_dst = Path.home() / ".copilot" / "skills" / "graphify" / "SKILL.md" - skill_dst.parent.mkdir(parents=True, exist_ok=True) - tmp_dst = skill_dst.with_suffix(skill_dst.suffix + ".tmp") - try: - shutil.copy(skill_src, tmp_dst) - _os_replace_with_fallback(tmp_dst, skill_dst) - except Exception: + skill_dst = _vscode_skill_destination() + with _skill_lock(skill_dst.parent): + skill_dst.parent.mkdir(parents=True, exist_ok=True) + tmp_dst = skill_dst.with_suffix(skill_dst.suffix + ".tmp") try: - tmp_dst.unlink(missing_ok=True) - except OSError: - pass - raise - # Progressive-capable: install the packaged references/ sidecar when present. - refs_src = Path(__file__).parent / "skills" / refs_bundle / "references" - if refs_src.exists(): - _install_skill_references(skill_dst, refs_src) - print(f" references -> {skill_dst.parent / 'references'}") - else: - orphan_refs = skill_dst.parent / "references" - if orphan_refs.exists(): - shutil.rmtree(orphan_refs) - _write_version_stamp(skill_dst, __version__) - print(f" skill installed -> {skill_dst}") + shutil.copy(skill_src, tmp_dst) + _os_replace_with_fallback(tmp_dst, skill_dst) + except Exception: + try: + tmp_dst.unlink(missing_ok=True) + except OSError: + pass + raise + # Progressive-capable: install the packaged references/ sidecar when present. + refs_src = Path(__file__).parent / "skills" / refs_bundle / "references" + if refs_src.exists(): + _install_skill_references(skill_dst, refs_src) + print(f" references -> {skill_dst.parent / 'references'}") + else: + orphan_refs = skill_dst.parent / "references" + if orphan_refs.exists(): + shutil.rmtree(orphan_refs) + _write_version_stamp(skill_dst, __version__) + print(f" skill installed -> {skill_dst}") instructions = (project_dir or Path(".")) / ".github" / "copilot-instructions.md" instructions.parent.mkdir(parents=True, exist_ok=True) @@ -994,25 +1117,26 @@ def vscode_install(project_dir: Path | None = None) -> None: print("Note: for GitHub Copilot CLI (terminal), use: graphify copilot install") def vscode_uninstall(project_dir: Path | None = None) -> None: """Remove graphify VS Code Copilot Chat skill and .github/copilot-instructions.md section.""" - skill_dst = Path.home() / ".copilot" / "skills" / "graphify" / "SKILL.md" - if skill_dst.exists(): - skill_dst.unlink() - print(f" skill removed -> {skill_dst}") - version_file = skill_dst.parent / ".graphify_version" - if version_file.exists(): - version_file.unlink() - refs_dir = skill_dst.parent / "references" - if refs_dir.exists(): - shutil.rmtree(refs_dir) - for d in ( - skill_dst.parent, - skill_dst.parent.parent, - skill_dst.parent.parent.parent, - ): - try: - d.rmdir() - except OSError: - break + skill_dst = _vscode_skill_destination() + with _skill_lock(skill_dst.parent): + if skill_dst.exists(): + skill_dst.unlink() + print(f" skill removed -> {skill_dst}") + version_file = skill_dst.parent / ".graphify_version" + if version_file.exists(): + version_file.unlink() + refs_dir = skill_dst.parent / "references" + if refs_dir.exists(): + shutil.rmtree(refs_dir) + for d in ( + skill_dst.parent, + skill_dst.parent.parent, + skill_dst.parent.parent.parent, + ): + try: + d.rmdir() + except OSError: + break instructions = (project_dir or Path(".")) / ".github" / "copilot-instructions.md" if not instructions.exists(): @@ -1167,24 +1291,25 @@ def _antigravity_uninstall(project_dir: Path, *, project: bool = False) -> None: # Remove skill file skill_dst = _platform_skill_destination("antigravity", project=project, project_dir=project_dir) - if skill_dst.exists(): - skill_dst.unlink() - print(f"graphify skill removed from {skill_dst}") - version_file = skill_dst.parent / ".graphify_version" - if version_file.exists(): - version_file.unlink() - refs_dir = skill_dst.parent / "references" - if refs_dir.exists(): - shutil.rmtree(refs_dir) - for d in ( - skill_dst.parent, - skill_dst.parent.parent, - skill_dst.parent.parent.parent, - ): - try: - d.rmdir() - except OSError: - break + with _skill_lock(skill_dst.parent): + if skill_dst.exists(): + skill_dst.unlink() + print(f"graphify skill removed from {skill_dst}") + version_file = skill_dst.parent / ".graphify_version" + if version_file.exists(): + version_file.unlink() + refs_dir = skill_dst.parent / "references" + if refs_dir.exists(): + shutil.rmtree(refs_dir) + for d in ( + skill_dst.parent, + skill_dst.parent.parent, + skill_dst.parent.parent.parent, + ): + try: + d.rmdir() + except OSError: + break _CURSOR_RULE_PATH = Path(".cursor") / "rules" / "graphify.mdc" _CURSOR_RULE = """\ --- @@ -1802,21 +1927,22 @@ def _kilo_uninstall_global() -> list[str]: pass skill_dst = Path.home() / _PLATFORM_CONFIG["kilo"]["skill_dst"] - if skill_dst.exists(): - skill_dst.unlink() - removed.append(f"skill removed: {skill_dst}") - version_file = skill_dst.parent / ".graphify_version" - if version_file.exists(): - version_file.unlink() - for d in ( - skill_dst.parent, - skill_dst.parent.parent, - skill_dst.parent.parent.parent, - ): - try: - d.rmdir() - except OSError: - break + with _skill_lock(skill_dst.parent): + if skill_dst.exists(): + skill_dst.unlink() + removed.append(f"skill removed: {skill_dst}") + version_file = skill_dst.parent / ".graphify_version" + if version_file.exists(): + version_file.unlink() + for d in ( + skill_dst.parent, + skill_dst.parent.parent, + skill_dst.parent.parent.parent, + ): + try: + d.rmdir() + except OSError: + break return removed def _kilo_install(project_dir: Path) -> None: diff --git a/tests/test_skill_auto_refresh.py b/tests/test_skill_auto_refresh.py new file mode 100644 index 000000000..3ac809cae --- /dev/null +++ b/tests/test_skill_auto_refresh.py @@ -0,0 +1,527 @@ +"""#1805: a package upgrade must not leave the installed skills behind. + +`uv tool upgrade graphifyy` / `pip install -U graphifyy` replace the package +but not the skill copies under ~/.claude, ~/.codex, ~/.config/opencode, ...; +a plain `graphify install` then refreshes only the detected platform, so every +other agent kept the old instructions. The CLI now refreshes every stale +user-scope copy on its first run after an upgrade, and leaves alone the copies +it cannot refresh safely. +""" +from __future__ import annotations + +import contextlib +import errno +import os +import subprocess +import sys +import threading +from pathlib import Path + +import pytest + +import graphify.__main__ as mainmod +import graphify.install as installmod +from graphify.install import __version__, _PLATFORM_CONFIG, _platform_skill_destination + +OLD = "0.0.1" +OLD_BODY = "---\nname: graphify\n---\nold skill body\n" + + +@pytest.fixture(autouse=True) +def _refresh_enabled(monkeypatch): + monkeypatch.delenv("GRAPHIFY_NO_AUTO_REFRESH", raising=False) + + +def _packaged(name: str) -> bytes: + skill_file = "skill.md" if name == "gemini" else _PLATFORM_CONFIG[name]["skill_file"] + return (Path(installmod.__file__).parent / skill_file).read_bytes() + + +def _stale(name: str, version: str = OLD, body: str = OLD_BODY) -> Path: + """Lay down `name`'s skill as an older release left it.""" + dst = _platform_skill_destination(name) + dst.parent.mkdir(parents=True, exist_ok=True) + dst.write_text(body, encoding="utf-8") + (dst.parent / ".graphify_version").write_text(version, encoding="utf-8") + return dst + + +def _stamp(dst: Path) -> str: + return (dst.parent / ".graphify_version").read_text(encoding="utf-8") + + +# --------------------------------------------------------------------------- +# What gets refreshed +# --------------------------------------------------------------------------- + +def test_every_stale_platform_is_refreshed_not_only_the_detected_one(capsys): + dsts = {name: _stale(name) for name in ("claude", "codex", "opencode", "gemini")} + + mainmod._refresh_stale_skills() + + out, err = capsys.readouterr() + for name, dst in dsts.items(): + assert _stamp(dst) == __version__, name + assert dst.read_bytes() == _packaged(name), name + assert str(dst.parent) in err + assert OLD in err and "GRAPHIFY_NO_AUTO_REFRESH" in err + assert out == "", "stdout must stay clean for --json and the MCP stdio server" + + +def test_a_local_edit_survives_as_bak(capsys): + dst = _stale("codex", body=OLD_BODY + "MY LOCAL EDIT\n") + + mainmod._refresh_stale_skills() + + backup = dst.with_suffix(dst.suffix + ".bak") + assert "MY LOCAL EDIT" in backup.read_text(encoding="utf-8") + assert "MY LOCAL EDIT" not in dst.read_text(encoding="utf-8") + assert str(backup) in capsys.readouterr().err + + +def test_a_platform_that_was_never_installed_is_not_created(): + home = Path.home() + before = sorted(p.relative_to(home) for p in home.rglob("*")) + + mainmod._refresh_stale_skills() + + assert sorted(p.relative_to(home) for p in home.rglob("*")) == before + + +def test_a_stamp_without_skill_md_is_left_for_the_repair_warning(): + dst = _stale("codex") + dst.unlink() + + mainmod._refresh_stale_skills() + + assert not dst.exists() + assert _stamp(dst) == OLD + + +# --------------------------------------------------------------------------- +# What is left alone +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("value", ["1", "true", "YES"]) +def test_the_env_opt_out_disables_it(monkeypatch, capsys, value): + monkeypatch.setenv("GRAPHIFY_NO_AUTO_REFRESH", value) + dst = _stale("codex") + + mainmod._refresh_stale_skills() + + assert _stamp(dst) == OLD + assert dst.read_text(encoding="utf-8") == OLD_BODY + assert capsys.readouterr().err == "" + + +def test_a_falsy_opt_out_value_keeps_it_on(monkeypatch): + monkeypatch.setenv("GRAPHIFY_NO_AUTO_REFRESH", "0") + dst = _stale("codex") + + mainmod._refresh_stale_skills() + + assert _stamp(dst) == __version__ + + +def test_a_newer_skill_is_never_downgraded(capsys): + """#1568: installing from an older package would downgrade the skill.""" + dst = _stale("codex", version="999.0.0") + + mainmod._refresh_stale_skills() + + assert _stamp(dst) == "999.0.0" + assert dst.read_text(encoding="utf-8") == OLD_BODY + assert capsys.readouterr().err == "" + + +def test_a_current_skill_is_untouched_and_silent(capsys): + dst = _stale("codex", version=__version__) + + mainmod._refresh_stale_skills() + + assert dst.read_text(encoding="utf-8") == OLD_BODY + assert not dst.with_suffix(dst.suffix + ".bak").exists() + assert capsys.readouterr().err == "" + + +def test_the_copilot_dir_shared_with_vscode_is_not_guessed(): + """`graphify vscode install` writes a different SKILL.md to the same dir as + the copilot platform; the stamp can't say which one is there.""" + assert _platform_skill_destination("copilot") == mainmod._vscode_skill_destination() + dst = _stale("copilot") + + mainmod._refresh_stale_skills() + + assert _stamp(dst) == OLD + assert dst.read_text(encoding="utf-8") == OLD_BODY + + +def test_a_directory_shared_by_two_platforms_is_not_guessed(monkeypatch): + """On Windows gemini and agents both install into ~/.agents/skills (#2800).""" + monkeypatch.setattr(mainmod.platform, "system", lambda: "Windows") + assert _platform_skill_destination("gemini") == _platform_skill_destination("agents") + dst = _stale("agents") + + mainmod._refresh_stale_skills() + + assert _stamp(dst) == OLD + + +def test_a_directory_shared_through_a_symlink_is_not_guessed(requires_symlinks): + """Two platform dirs linked to one real dir hold ONE copy, like the shared + dirs above - whichever platform refreshed it would overwrite the other's.""" + real = _platform_skill_destination("opencode").parent + real.mkdir(parents=True) + link = _platform_skill_destination("codex").parent + link.parent.mkdir(parents=True, exist_ok=True) + link.symlink_to(real, target_is_directory=True) + dst = _stale("codex") + + mainmod._refresh_stale_skills() + + assert _stamp(dst) == OLD + assert dst.read_text(encoding="utf-8") == OLD_BODY + + +@pytest.mark.parametrize("system, variant", [("Linux", "claude"), ("Windows", "windows")]) +def test_the_claude_dir_gets_the_host_variant(monkeypatch, system, variant): + """claude and windows share ~/.claude/skills; refresh writes the variant a + plain `graphify install` would pick on this OS.""" + monkeypatch.setattr(mainmod.platform, "system", lambda: system) + dst = _stale("claude") + + mainmod._refresh_stale_skills() + + assert dst.read_bytes() == _packaged(variant) + assert _stamp(dst) == __version__ + + +# --------------------------------------------------------------------------- +# Failure and the CLI entry point +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("failure", [OSError("read-only file system"), SystemExit(1)]) +def test_a_failed_refresh_never_breaks_the_command(monkeypatch, capsys, failure): + def boom(*_args, **_kwargs): + raise failure + + monkeypatch.setattr(mainmod, "_copy_skill_file", boom) + dst = _stale("codex") + + mainmod._refresh_stale_skills() # must not raise + + assert _stamp(dst) == OLD, "an unrefreshed copy keeps its stamp, so the warning still fires" + assert "could not refresh" in capsys.readouterr().err + + +def test_one_unresolvable_destination_does_not_cancel_the_others(monkeypatch): + def boom(): + raise RuntimeError("no home for vscode") + + monkeypatch.setattr(mainmod, "_vscode_skill_destination", boom) + dst = _stale("codex") + + mainmod._refresh_stale_skills() + + assert _stamp(dst) == __version__ + + +def test_a_refresh_reports_only_the_result_and_the_backup(capsys): + """_copy_skill_file's per-file install lines are noise on every CLI run; + only where the previous copy went is worth keeping.""" + dst = _stale("codex") + + mainmod._refresh_stale_skills() + + lines = capsys.readouterr().err.splitlines() + assert len(lines) == 2, lines + assert lines[0] == ( + f"graphify: refreshed skill at {dst.parent} ({OLD} -> {__version__}); " + f"set GRAPHIFY_NO_AUTO_REFRESH=1 to disable" + ) + assert str(dst.with_suffix(dst.suffix + ".bak")) in lines[1] + + +# --------------------------------------------------------------------------- +# Concurrent CLI processes +# --------------------------------------------------------------------------- + +def _lock(dst: Path) -> Path: + return installmod._skill_lock_path(dst.parent) + + +def _lock_is_free(dst: Path) -> bool: + with installmod._skill_lock(dst.parent, wait=False) as owned: + return owned is True + + +# Another graphify process (a refresh, an install, an uninstall): takes dst's +# skill lock, says so, then either waits for stdin to close or dies on the spot +# without releasing anything. +_HOLD_LOCK = """ +import os, sys +from pathlib import Path +import graphify.install as inst +lock = inst._skill_lock(Path(sys.argv[1]), wait=False) # keep a reference: collecting it releases +assert lock.__enter__() is True +print("locked", flush=True) +if sys.argv[2] == "crash": + os._exit(1) +sys.stdin.read() +""" + + +def _hold_lock(dst: Path, mode: str) -> subprocess.Popen: + proc = subprocess.Popen( + [sys.executable, "-c", _HOLD_LOCK, str(dst.parent), mode], + stdin=subprocess.PIPE, stdout=subprocess.PIPE, text=True, + ) + assert proc.stdout.readline().strip() == "locked" + return proc + + +def test_a_directory_another_process_is_refreshing_is_skipped(): + """_copy_skill_file stages through fixed .tmp names; a second process + writing the same dir at once would write through the first.""" + dst = _stale("codex") + proc = _hold_lock(dst, "hold") + try: + mainmod._refresh_stale_skills() + finally: + proc.communicate(timeout=30) + + assert _stamp(dst) == OLD + + +def test_a_process_that_died_holding_the_lock_does_not_block(): + """The OS drops the lock with its holder: no timeout, and the lock file + it leaves behind means nothing.""" + dst = _stale("codex") + _hold_lock(dst, "crash").communicate(timeout=30) + assert _lock(dst).exists() + + mainmod._refresh_stale_skills() + + assert _stamp(dst) == __version__ + + +def test_a_refresh_finished_by_another_process_is_not_redone(monkeypatch, capsys): + """Stale when first checked, current once the lock is ours: another + process refreshed the directory in between, so nothing is left to do.""" + dst = _stale("codex") + real_lock = mainmod._skill_lock + + @contextlib.contextmanager + def lock_after_the_other_process(skill_dir, **kwargs): + (skill_dir / ".graphify_version").write_text(__version__, encoding="utf-8") + with real_lock(skill_dir, **kwargs) as owned: + yield owned + + monkeypatch.setattr(mainmod, "_skill_lock", lock_after_the_other_process) + + mainmod._refresh_stale_skills() + + assert dst.read_text(encoding="utf-8") == OLD_BODY + assert capsys.readouterr().err == "" + + +def test_a_skill_uninstalled_meanwhile_is_not_brought_back(monkeypatch, capsys): + """Stale when first checked, uninstalled by another process before the + lock is ours: the refresh must not reinstall what the user just removed.""" + dst = _stale("codex") + real_lock = mainmod._skill_lock + + @contextlib.contextmanager + def lock_after_an_uninstall(skill_dir, **kwargs): + with contextlib.redirect_stdout(None): + installmod._remove_skill_file("codex") + with real_lock(skill_dir, **kwargs) as owned: + yield owned + + monkeypatch.setattr(mainmod, "_skill_lock", lock_after_an_uninstall) + + mainmod._refresh_stale_skills() + + assert not dst.parent.exists() + assert capsys.readouterr().err == "" + + +@pytest.mark.parametrize("write", [ + lambda: installmod._remove_skill_file("codex"), + lambda: installmod._copy_skill_file("codex"), +], ids=["uninstall", "install"]) +def test_install_and_uninstall_wait_for_a_refresh_in_progress(write): + """Install and uninstall take the same lock as the refresh, but wait for + it instead of skipping: they only go ahead once the refresh is done.""" + dst = _stale("codex") + proc = _hold_lock(dst, "hold") + worker = threading.Thread(target=write) + try: + with contextlib.redirect_stdout(None): + worker.start() + worker.join(0.5) + assert worker.is_alive(), "wrote while another process held the lock" + assert dst.read_text(encoding="utf-8") == OLD_BODY + assert _stamp(dst) == OLD + finally: + proc.communicate(timeout=30) + worker.join(30) + + assert not worker.is_alive() + + +def test_the_lock_file_stays_out_of_the_skill_directory(tmp_path): + """A project-scope install writes into the user's repository; the lock + file must not end up there, nor in any user-scope skill directory.""" + dst = installmod._copy_skill_file("claude", project=True, project_dir=tmp_path) + user_dst = _stale("codex") + mainmod._refresh_stale_skills() + + for skill_dir in (dst.parent, user_dst.parent): + assert not any(p.suffix == ".lock" for p in skill_dir.rglob("*")) + assert not _lock(dst).is_relative_to(tmp_path) + + +def test_without_file_locking_install_still_works_and_the_refresh_skips(monkeypatch): + """A file system that refuses to lock (some network mounts): install goes + ahead as it did before the lock existed, the refresh leaves the skill to + the warning.""" + + def no_locks(_fh): + raise OSError(errno.ENOLCK, "No locks available") + + monkeypatch.setattr(installmod, "_try_skill_lock", no_locks) + stale = _stale("codex") + + mainmod._refresh_stale_skills() + assert _stamp(stale) == OLD + + with contextlib.redirect_stdout(None): + installed = installmod._copy_skill_file("codex") + assert _stamp(installed) == __version__ + + +def test_the_lock_is_released_after_a_refresh(): + dsts = [_stale(name) for name in ("claude", "codex")] + + mainmod._refresh_stale_skills() + + assert all(_lock_is_free(dst) for dst in dsts) + + +def test_the_lock_is_released_after_a_failed_refresh(monkeypatch): + def boom(*_args, **_kwargs): + raise SystemExit(1) + + monkeypatch.setattr(mainmod, "_copy_skill_file", boom) + dst = _stale("codex") + + mainmod._refresh_stale_skills() + + assert _lock_is_free(dst) + + +@pytest.mark.parametrize("name, uninstall", [ + ("codex", lambda _project: installmod._remove_skill_file("codex")), + ("kilo", lambda _project: installmod._kilo_uninstall_global()), + ("antigravity", installmod._antigravity_uninstall), +]) +def test_uninstall_after_the_lock_was_taken_leaves_no_directory_behind(tmp_path, name, uninstall): + """The lock file lives outside the skill directory and is never deleted, + so it cannot keep the directory alive after an uninstall.""" + dst = _stale(name) + with installmod._skill_lock(dst.parent): + pass + assert _lock(dst).exists() + + with contextlib.redirect_stdout(None): + uninstall(tmp_path) + + assert not dst.parent.exists() + assert _lock(dst).exists() + + +def test_uninstall_after_a_refresh_leaves_no_directory_behind(): + dst = _stale("codex") + dst.write_bytes(_packaged("codex")) # unedited, so the refresh keeps no .bak + mainmod._refresh_stale_skills() + + installmod._remove_skill_file("codex") + + assert not dst.parent.exists() + + +def test_a_stale_gemini_skill_gets_the_warning_too(monkeypatch, capsys): + """gemini is not in _PLATFORM_CONFIG, so the version check never saw its + ~/.gemini copy; with the refresh off it must still warn.""" + monkeypatch.setenv("GRAPHIFY_NO_AUTO_REFRESH", "1") + dst = _stale("gemini") + monkeypatch.setattr(sys, "argv", ["graphify", "--version"]) + + mainmod._run_cli() + + err = capsys.readouterr().err + assert f"warning: skill at {dst.parent} is from graphify {OLD}" in err + assert "graphify install --platform gemini" in err + + +def test_the_first_cli_run_after_an_upgrade_refreshes_and_stays_quiet(monkeypatch, capsys): + dsts = [_stale(name) for name in ("claude", "codex")] + monkeypatch.setattr(sys, "argv", ["graphify", "--version"]) + + mainmod._run_cli() + + out, err = capsys.readouterr() + assert out.strip() == f"graphify {__version__}" + assert all(_stamp(dst) == __version__ for dst in dsts) + assert "warning: skill at" not in err, "a refreshed copy must not also warn" + + +def test_what_the_refresh_skips_still_gets_the_warning(monkeypatch, capsys): + dst = _stale("copilot") + monkeypatch.setattr(sys, "argv", ["graphify", "--version"]) + + mainmod._run_cli() + + err = capsys.readouterr().err + assert _stamp(dst) == OLD + assert f"warning: skill at {dst.parent} is from graphify {OLD}" in err + + +def test_graphify_update_refreshes_before_it_runs(monkeypatch, capsys): + """Often the first command after an upgrade; it must fix the skills, not + just warn about them.""" + dst = _stale("codex") + monkeypatch.setattr(sys, "argv", ["graphify", "update", "."]) + monkeypatch.setattr(mainmod, "dispatch_install_cli", lambda _cmd: False) + ran = [] + monkeypatch.setattr(mainmod, "dispatch_command", ran.append) + + mainmod._run_cli() + + assert ran == ["update"] + assert _stamp(dst) == __version__ + assert "warning: skill at" not in capsys.readouterr().err + + +@pytest.mark.parametrize("cmd", ["install", "uninstall", "hook-check", "hook-guard"]) +def test_install_and_hook_commands_skip_the_refresh(monkeypatch, cmd): + calls = [] + monkeypatch.setattr(mainmod, "_refresh_stale_skills", lambda: calls.append(cmd)) + # Stop right after the pre-dispatch block: no real install/hook work runs. + monkeypatch.setattr(mainmod, "_check_skill_version", lambda *_a, **_k: None) + monkeypatch.setattr(sys, "argv", ["graphify", cmd, "--help"]) + + class _Stop(Exception): + pass + + def stop(*_a, **_k): + raise _Stop + + monkeypatch.setattr(mainmod, "dispatch_install_cli", stop) + monkeypatch.setattr(mainmod, "dispatch_command", stop) + try: + mainmod._run_cli() + except (_Stop, SystemExit): + pass + assert calls == []