fix: protect externally managed skills - #2371
Conversation
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR introduces an "externally managed" ownership marker (.graphify_externally_managed) for Graphify's assistant skill installations, mirroring Python's EXTERNALLY-MANAGED convention. When this marker file is present beside a platform's SKILL.md, the installer is intended to suppress version warnings and refuse to overwrite/remove the skill, mutate hooks, or touch instruction files—both for named-platform and bulk install/uninstall commands. The change adds a _is_externally_managed helper plus refusal guards wired into _copy_skill_file, _remove_skill_file, _refresh_all_version_stamps, _check_skill_version, and the CLI dispatch path, and updates the README to document the marker. It also adds tests covering the skill overwrite/removal refusal, named-platform install refusal for Claude and Codex, and silenced version warnings under the marker.
Worth a look
- install ordering: hooks/instructions may be written before _copy_skill_file refuses externally-managed skill —
graphify/install.py· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- _refuse_externally_managed_cli_action only guards subcmd at argv[2], missing scope-specific skill destination —
graphify/install.py:45· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 431 functions depend on the 220 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
dispatch_install_cli()— 2 callers, 33 callees - worse:
_copy_skill_file()— 8 callers, 4 callees - worse:
_remove_skill_file()— 11 callers, 2 callees - worse:
_check_skill_version()— 7 callers, 2 callees
Verification — 431 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 431 function(s) in the blast radius were not formally verified this run
· 2 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
| @@ -163,6 +164,8 @@ def __getattr__(name: str) -> str: | |||
|
|
|||
| def _check_skill_version(skill_dst: Path) -> None: | |||
There was a problem hiding this comment.
_check_skill_version()
7 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| @@ -232,6 +300,9 @@ def _copy_skill_file(platform_name: str, *, project: bool = False, project_dir: | |||
| def _remove_skill_file(platform_name: str, *, project: bool = False, project_dir: Path | None = None) -> bool: | |||
There was a problem hiding this comment.
_remove_skill_file()
11 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
1 participant
Summary
.graphify_externally_managedownership marker for skills maintained by configuration managersWhy
Graphify currently tells users with a stale skill to run its installer. That is unsafe when another system intentionally owns Claude, Codex, or other assistant configuration. The installer can replace reviewed instructions and platform hooks.
This follows the externally managed ownership pattern: package use and upgrades remain available, while assistant configuration stays under its declared manager.
Tests