Preserve global skill tracking across agents in unregister_agent_artifacts

The present-directory branch of ExtensionManager.unregister_agent_artifacts()
recomputed "remaining" registered_skills only by checking whether each name
still existed under the just-cleaned agent's own directory. registered_skills
is a single flat list shared across every agent an extension was ever
activated under (skills are only ever rendered for the currently active
agent, so there's no per-agent registry key). Repro: auggie and copilot both
have mirrors for the same extension; unregister_agent_artifacts("auggie")
correctly removes auggie's own mirror, sees the names absent from auggie's
(now empty) directory, and stores an empty registered_skills list - even
though copilot's mirror is still live on disk and now untracked. A later full
remove() then reads an empty registry and leaves copilot's mirror orphaned.

Fix: after the agent-scoped cleanup, recompute remaining names with
_extension_owned_skill_names(), which scans every safe, configured agent
skills directory (not just the one just cleaned) and keeps a name only if a
marker-verified SKILL.md for this extension still exists somewhere. This is
the same helper already used for the analogous same-agent toggle-cleanup
case, so no new abstraction was introduced. Explicit per-agent cleanup,
marker ownership verification, and symlink/containment safety are unchanged.

Added regression test:
- test_unregister_agent_artifacts_preserves_tracking_for_other_agent_mirror

Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
marcelsafin
2026-07-11 11:17:26 +02:00
parent 0ab9a5f3f4
commit 31c9b97cde
2 changed files with 101 additions and 8 deletions

View File

@@ -1901,15 +1901,21 @@ class ExtensionManager:
# When the directory does exist, _unregister_extension_skills
# may intentionally skip deletion when ownership cannot be
# verified (e.g., corrupted/missing SKILL.md or mismatching
# metadata.source). Only drop registry entries for skill
# directories that were actually removed so future cleanup
# attempts can still find skipped ones.
# metadata.source). A name no longer present under *this*
# agent's directory isn't necessarily gone everywhere either
# — registered_skills is a single flat list shared across
# every agent this extension was ever activated under, so
# an earlier activation under a different, still-active
# agent may have left its own marker-verified mirror behind.
# Recompute across every safe, supported skills directory
# (the same helper used for the analogous toggle-cleanup
# case) rather than just this one, or a still-existing
# mirror elsewhere would be silently dropped from tracking
# and orphaned on later removal (#2948).
if agent_skills_dir.is_dir():
remaining_skills = [
skill_name
for skill_name in registered_skills
if (agent_skills_dir / skill_name).is_dir()
]
remaining_skills = self._extension_owned_skill_names(
registered_skills, ext_id
)
if remaining_skills != registered_skills:
updates["registered_skills"] = remaining_skills

View File

@@ -1579,6 +1579,93 @@ class TestExtensionSkillRegistration:
"must not widen cleanup to every configured agent directory"
)
def test_unregister_agent_artifacts_preserves_tracking_for_other_agent_mirror(
self, project_dir, temp_dir
):
"""Present-directory reconciliation in ``unregister_agent_artifacts``
must not drop global ``registered_skills`` tracking for a name that
still has a marker-verified mirror under a *different* agent's
directory.
Auggie and Copilot are both activated in skills mode, each writing
its own mirror for the same extension skill names into the single,
agent-agnostic flat ``registered_skills`` list.
``unregister_agent_artifacts("auggie")`` removes auggie's own
mirror (its directory exists, so the fast path finds and deletes
it) — but before this fix, the registry reconciliation afterward
only checked whether each name still existed under *auggie's* own
(now-empty) directory, concluding every name was gone and wiping
``registered_skills`` to ``[]`` even though Copilot's mirror was
still live and now untracked. A later full ``remove()`` would then
read an empty registry and leave Copilot's mirror permanently
orphaned (#2948).
"""
_create_init_options(project_dir, ai="auggie", ai_skills=True)
manager = ExtensionManager(project_dir)
manager.install_from_directory(
_create_extension_dir(temp_dir, ext_id="dual-agent-unregister-ext"), "0.1.0",
register_commands=False,
)
manager.register_enabled_extensions_for_agent("auggie")
auggie_skills_dir = project_dir / ".augment" / "skills"
auggie_hello = auggie_skills_dir / "speckit-dual-agent-unregister-ext-hello" / "SKILL.md"
auggie_world = auggie_skills_dir / "speckit-dual-agent-unregister-ext-world" / "SKILL.md"
assert auggie_hello.exists() and auggie_world.exists(), (
"sanity: auggie's skills-mode activation should mirror both "
"extension commands as SKILL.md files"
)
_create_init_options(project_dir, ai="copilot", ai_skills=True)
manager.register_enabled_extensions_for_agent("copilot")
copilot_skills_dir = project_dir / ".github" / "skills"
copilot_hello = copilot_skills_dir / "speckit-dual-agent-unregister-ext-hello" / "SKILL.md"
copilot_world = copilot_skills_dir / "speckit-dual-agent-unregister-ext-world" / "SKILL.md"
assert copilot_hello.exists() and copilot_world.exists(), (
"sanity: copilot's skills-mode activation should also mirror "
"both extension commands"
)
# Unregister artifacts for auggie only (its directory exists and
# is cleaned up), while copilot's mirror is untouched and remains
# live on disk.
manager.unregister_agent_artifacts("auggie")
assert not auggie_hello.exists() and not auggie_world.exists(), (
"sanity: auggie's own mirror must be removed"
)
assert copilot_hello.exists() and copilot_world.exists(), (
"sanity: copilot's mirror must be untouched by an auggie-scoped "
"unregister call"
)
registry_metadata = manager.registry.get("dual-agent-unregister-ext")
tracked = registry_metadata.get("registered_skills", [])
assert "speckit-dual-agent-unregister-ext-hello" in tracked, (
"registered_skills tracking must be preserved for names still "
"owned by copilot's live mirror, even though they were removed "
"from auggie's own (now nonexistent) directory — reconciling "
"against only the just-cleaned agent's directory incorrectly "
"concludes the name is gone everywhere (#2948)"
)
assert "speckit-dual-agent-unregister-ext-world" in tracked, (
"registered_skills tracking must be preserved for names still "
"owned by copilot's live mirror, even though they were removed "
"from auggie's own (now nonexistent) directory — reconciling "
"against only the just-cleaned agent's directory incorrectly "
"concludes the name is gone everywhere (#2948)"
)
# A subsequent full extension removal must still find and clean up
# copilot's remaining mirror via the preserved tracking.
assert manager.remove("dual-agent-unregister-ext") is True
assert not copilot_hello.exists() and not copilot_world.exists(), (
"full removal must clean up copilot's remaining mirror — this "
"only works if registered_skills tracking wasn't prematurely "
"dropped by the earlier auggie-scoped unregister call (#2948)"
)
def test_extension_owned_skill_names_rejects_symlinked_candidate_directory(
self, project_dir, temp_dir
):