From 8ae9878465edf9fc0a8fd7875e2544a2d9a1107d Mon Sep 17 00:00:00 2001 From: berce <75228263+bercedev@users.noreply.github.com> Date: Mon, 28 Sep 2026 15:48:40 +0300 Subject: [PATCH] feat(cli): refresh stale skills after a package upgrade (#1805) After `uv tool upgrade graphifyy` or `pip install -U graphifyy`, the installed skills stay on the old version. `graphify install` only fixes the platform it detects, so codex, opencode, gemini and the others keep running outdated instructions until someone reinstalls each one by hand. The CLI now checks the version stamp of every user-scope skill on its first run after an upgrade, `graphify update` included, and re-copies the ones that are older than the package. This is on by default and can be turned off with GRAPHIFY_NO_AUTO_REFRESH=1. It is deliberately conservative about what it touches: - only skills graphify installed itself (stamp and SKILL.md both present) - never a skill newer than the package, so nothing gets downgraded (#1568) - never a directory that two installers write to, such as copilot and `graphify vscode install`, or two platform dirs symlinked to the same place, because the stamp can't say which variant lives there - a locally edited SKILL.md is kept as SKILL.md.bak (#3144) Install, uninstall and the refresh take the same lock per skill directory, so two processes never write through each other's fixed .tmp staging names, and an uninstall can't race a refresh that would put the skill back. It is an OS file lock (flock on POSIX, msvcrt.locking on Windows). The OS releases it however the holder exits, so a crash never leaves a directory locked and there is no staleness timeout to tune. The lock file lives under ~/.cache/graphify/skill-locks/ rather than in the skill directory, so a project install leaves nothing behind in the repository. Install and uninstall wait for the lock; the refresh skips a directory another process holds and reads the stamp again once it has the lock. Where the file system can't lock, install and uninstall run unlocked as they did before, and the refresh leaves the skill to the version warning. Output goes to stderr, one line per refreshed directory, so --json and the MCP stdio server are unaffected. A failed refresh never breaks the command the user actually ran: the stamp stays old and the existing version warning still points to the manual fix. While here, the version warning now also checks the gemini skill, which it used to miss because gemini is not in _PLATFORM_CONFIG, and the vscode skill path moved into _vscode_skill_destination() so install, uninstall and the refresh share it. The README lists the new variable and mentions the refresh in the version-mismatch troubleshooting entry. (cherry picked from commit 50ebe642af8276d1a1c5d42df8c6b960c300226a) --- README.md | 5 +- graphify/__main__.py | 135 +++++++- graphify/install.py | 382 ++++++++++++++-------- tests/test_skill_auto_refresh.py | 527 +++++++++++++++++++++++++++++++ 4 files changed, 917 insertions(+), 132 deletions(-) create mode 100644 tests/test_skill_auto_refresh.py 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 == []