fix: reconcile before fallible skills phase, infer legacy skill provenance, and unregister stale extension artifacts on toggle

Three findings from the Copilot review on HEAD b9d9053/3a1e749:

1. `register_enabled_presets_for_agent()` only recorded a preset's command
   names into `affected_cmd_names` (the set later passed to
   `_reconcile_composed_commands`/`_reconcile_skills`) in the loop that ran
   *after* `_register_skills()`, inside the same per-preset `try` block. If
   `_register_skills` raised, the `except` caught it and `continue`d before
   that loop ever ran — so a preset whose commands phase already wrote real
   content to disk never got reconciled against the full priority stack,
   leaving its raw content in place instead of a project override or
   higher-precedence preset's content. Fix: record the manifest's command
   names immediately after the commands phase succeeds and persists, before
   calling the independently fallible `_register_skills()`.

2. The legacy flat-list `registered_skills` migration (added for the
   previous review round) attributed every name in the list to whichever
   agent was currently being (re)activated. If the first operation after
   upgrading from a pre-#2948 registry was a direct switch to a *different*
   skill-mode agent (e.g. a legacy Claude override, then `integration use
   codex` with no intervening Claude rescaffold), the migrated dict only
   recorded `{"codex": [...]}`, permanently losing Claude's actual
   provenance and orphaning its override on later removal. Fix: added
   `_infer_legacy_skill_provenance()`, which probes every configured
   skill-mode agent's directory (via the same safe, symlink-validated
   helpers already used for restore/removal) for a `SKILL.md` whose
   frontmatter records this exact preset as the owner
   (`metadata.source == "preset:<pack_id>"`). A name found under more than
   one directory is attributed to every matching agent (the preset may have
   been active while the user switched between several skill-mode agents
   before provenance tracking existed); names that can't be matched to any
   directory still fall back to the previously-active best-effort
   behaviour. Directory grouping for shared-path aliases (e.g.
   agy/codex/zed all resolving to `.agents/skills`) intentionally does not
   call `.resolve()` on the path, since doing so diverges from
   `project_root`'s own resolution state on platforms where a path
   component is itself a symlink (e.g. macOS's `/var` -> `/private/var`)
   and made every subsequent containment check spuriously fail.

3. `register_enabled_extensions_for_agent()` has the same command/skill
   mutual-exclusion gap the preset path had (fixed in a previous round):
   toggling `ai_skills` for the *same active* agent left the opposite
   mode's artifact behind. Command -> skills left the extension's
   `.agent.md` file and its `registered_commands[agent]` entry in place
   once `skills_mode_active` made the commands phase a no-op. Skills ->
   command left the extension's `SKILL.md` file in place, since an empty
   `_register_extension_skills()` result (because this agent's skills
   directory no longer resolves once `ai_skills` is off) was treated as
   "nothing to register" rather than "this was rendered here before and is
   now stale". This diverges from the preset path in one respect:
   `registered_skills` for extensions has always been a flat list with no
   per-agent provenance (extension skills are only ever rendered for the
   active agent, never per-preset-per-agent tracked), so the fix resolves
   ownership by checking which of the extension's tracked skill names
   still exist as directories under this specific agent's directory before
   removing them — mirroring the same technique `unregister_agent_artifacts`
   already uses for full agent deactivation, but scoped narrowly to firing
   only when a toggle is actually detected (`skills_mode_active` /
   `command_mode_active`), so a same-mode re-run never disturbs
   already-correct artifacts or a user's manual customizations.

Regression tests (all confirmed red before their respective fix, green
after):
- tests/test_presets.py::TestPresetSkills::test_rescaffold_reconciles_override_even_when_skills_phase_fails
- tests/test_presets.py::TestPresetSkills::test_rescaffold_legacy_flat_list_direct_switch_preserves_original_agent
- tests/test_extension_skills.py::TestExtensionSkillRegistration::test_rescaffold_toggle_command_to_skills_removes_stale_extension_command_file
- tests/test_extension_skills.py::TestExtensionSkillRegistration::test_rescaffold_toggle_skills_to_command_removes_stale_extension_skill_file

Verification: tests/test_presets.py + tests/test_extensions.py +
tests/test_extension_skills.py (753 passed), tests/integrations/ (1768
passed, 1 skipped), full suite `pytest tests -q` (3942 passed, 109
skipped), `ruff check` on changed files clean.

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 08:46:15 +02:00
parent 3a1e74931d
commit 1bdb5b3473
4 changed files with 435 additions and 7 deletions

View File

@@ -1805,6 +1805,22 @@ class ExtensionManager:
and bool(agent_config)
and agent_config.get("extension") != "/SKILL.md"
)
# Mirror image of skills_mode_active: this agent is command-backed,
# active, and currently in command mode. Used to detect a
# skills -> command toggle for this same agent, where the skills
# phase below returns empty (its directory no longer resolves) but
# a previously-written extension SKILL.md is now stale (#2948).
command_mode_active = (
active_agent == agent_name
and not ai_skills_enabled
and bool(agent_config)
and agent_config.get("extension") != "/SKILL.md"
)
agent_skills_dir = None
if agent_config and agent_config.get("extension") != "/SKILL.md":
from .. import _get_skills_dir as _resolve_agent_skills_dir
agent_skills_dir = _resolve_agent_skills_dir(self.project_root, agent_name)
for ext_id, metadata in self.registry.list().items():
if not metadata.get("enabled", True):
@@ -1840,6 +1856,30 @@ class ExtensionManager:
new_registered.pop(agent_name, None)
if new_registered != registered_commands:
updates["registered_commands"] = new_registered
elif agent_config and skills_mode_active:
# Toggled command -> skills for this same agent: the
# commands phase above is skipped, but a command file
# this extension previously wrote for this agent while
# command mode was active is still on disk and still
# tracked. Remove it narrowly for this agent so
# command-mode and skills-mode artifacts stay mutually
# exclusive, matching unregister_agent_artifacts's
# per-agent command cleanup (#2948).
registered_commands = metadata.get("registered_commands", {})
if isinstance(registered_commands, dict) and registered_commands.get(
agent_name
):
stale_commands = self._valid_name_list(
registered_commands.get(agent_name)
)
if stale_commands:
registrar.unregister_commands(
{agent_name: stale_commands}, self.project_root
)
new_registered = copy.deepcopy(registered_commands)
new_registered.pop(agent_name, None)
if new_registered != registered_commands:
updates["registered_commands"] = new_registered
# Extension *skills* are only ever rendered for the active agent:
# `_register_extension_skills` resolves the skills dir and
@@ -1879,6 +1919,34 @@ class ExtensionManager:
dict.fromkeys(existing_skills + registered_skills)
)
updates["registered_skills"] = merged_skills
elif command_mode_active and agent_skills_dir is not None:
# Mirror image: toggled skills -> command for
# this same agent. _register_extension_skills
# returned empty because this agent's skills
# directory no longer resolves once ai_skills is
# off, but a SKILL.md this extension wrote while
# skills mode was active may still be tracked
# and still on disk. Remove it narrowly for this
# agent's directory only (#2948).
existing_skills = self._valid_name_list(
metadata.get("registered_skills", [])
)
owned_here = [
name
for name in existing_skills
if (agent_skills_dir / name).is_dir()
]
if owned_here:
self._unregister_extension_skills(
owned_here, ext_id, skills_dir=agent_skills_dir
)
remaining = [
name
for name in existing_skills
if (agent_skills_dir / name).is_dir()
]
if remaining != existing_skills:
updates["registered_skills"] = remaining
if updates:
self.registry.update(ext_id, updates)

View File

@@ -804,11 +804,36 @@ class PresetManager:
if merged_commands != existing_commands:
self.registry.update(pack_id, {"registered_commands": merged_commands})
# Record this preset's command names for reconciliation now,
# right after the commands phase succeeds and before calling
# the independently fallible _register_skills(). Both used to
# be recorded together after skills also succeeded; if skills
# raised, the exception (caught below) skipped this entirely,
# so a preset whose commands phase wrote real content never
# got reconciled against the full priority stack and its raw
# content could be left in place instead of a project
# override or higher-precedence preset (#2948).
for tmpl in manifest.templates:
if tmpl.get("type") == "command":
affected_cmd_names.add(tmpl["name"])
registered_skills = self._register_skills(manifest, pack_dir)
raw_existing_skills = metadata.get("registered_skills")
existing_skills = self._normalize_registered_skills(
raw_existing_skills, fallback_agent=agent_name
)
if isinstance(raw_existing_skills, list) and raw_existing_skills:
# Legacy flat-list value: don't assume agent_name wrote
# every name (the first post-upgrade operation may be a
# direct switch to a different skill-mode agent) —
# infer real ownership from on-disk provenance instead
# (#2948).
existing_skills = self._infer_legacy_skill_provenance(
[n for n in raw_existing_skills if isinstance(n, str)],
pack_id,
fallback_agent=agent_name,
)
else:
existing_skills = self._normalize_registered_skills(
raw_existing_skills, fallback_agent=agent_name
)
merged_skills = copy.deepcopy(existing_skills)
if registered_skills.get(agent_name):
merged_skills[agent_name] = registered_skills[agent_name]
@@ -836,10 +861,6 @@ class PresetManager:
)
if merged_skills != existing_skills or needs_migration:
self.registry.update(pack_id, {"registered_skills": merged_skills})
for tmpl in manifest.templates:
if tmpl.get("type") == "command":
affected_cmd_names.add(tmpl["name"])
except Exception as pack_err:
from .. import _print_cli_warning
@@ -1636,6 +1657,97 @@ class PresetManager:
return {selected_ai: written} if written else {}
def _infer_legacy_skill_provenance(
self, skill_names: List[str], pack_id: str, fallback_agent: str
) -> Dict[str, List[str]]:
"""Infer per-agent ownership of a legacy flat-list ``registered_skills`` value.
Pre-#2948 registries recorded ``registered_skills`` as a flat list
with no record of which agent directory each name was actually
written under. Blindly attributing every name to ``fallback_agent``
(the agent currently being processed) loses the real writer whenever
the *first* operation after upgrading is a direct switch to a
*different* skill-mode agent — e.g. a legacy Claude override
followed directly by ``integration use codex``, with no
intervening rescaffold for Claude — permanently orphaning Claude's
override on later removal.
Instead, every configured skill-mode agent's directory is probed
(via the same safe, symlink-validated helpers used for
restore/removal) for a ``SKILL.md`` whose frontmatter records this
exact preset as the owner (``metadata.source == "preset:<pack_id>"``,
the same marker :meth:`_register_skills` writes). A name can
legitimately be found under more than one agent's directory — the
preset may have been active while the user switched between several
skill-mode agents before provenance tracking existed — so every
matching agent is recorded, not just the first. Names that can't be
matched to any directory (e.g. the file was deleted out of band)
fall back to ``fallback_agent``, preserving the previous
best-effort behaviour for the unrecoverable case.
"""
from ..agents import CommandRegistrar
registrar = CommandRegistrar()
skill_mode_agents = sorted(
name
for name, cfg in registrar.AGENT_CONFIGS.items()
if cfg.get("extension") == "/SKILL.md"
)
# Multiple agent names can resolve to the same physical directory
# (e.g. agy/codex/zed all use .agents/skills); group by directory so
# each is probed once and attributed to a single deterministic
# canonical agent name, matching the tie-break already used by
# _unregister_skills's directory grouping. Deliberately keep the
# unresolved path (matching what _safe_skills_dir_for_agent already
# validated) rather than calling .resolve() here: on macOS /var is
# itself a symlink to /private/var, so resolving would make this
# path diverge from self.project_root's own resolution state and
# make every subsequent containment check in
# _validate_skill_subdir() spuriously fail.
dir_to_agents: Dict[Path, List[str]] = {}
for agent_name in skill_mode_agents:
skills_dir = self._safe_skills_dir_for_agent(agent_name)
if skills_dir is None:
continue
dir_to_agents.setdefault(skills_dir, []).append(agent_name)
marker = f"preset:{pack_id}"
inferred: Dict[str, List[str]] = {}
matched_names: set = set()
for resolved_dir, agents in dir_to_agents.items():
canonical_agent = fallback_agent if fallback_agent in agents else sorted(agents)[0]
for name in skill_names:
skill_subdir = resolved_dir / name
if not self._validate_skill_subdir(skill_subdir, create=False):
continue
skill_file = skill_subdir / "SKILL.md"
if not skill_file.is_file():
continue
try:
content = skill_file.read_text(encoding="utf-8")
except OSError:
continue
frontmatter, _ = registrar.parse_frontmatter(content)
skill_metadata = frontmatter.get("metadata")
source = (
skill_metadata.get("source")
if isinstance(skill_metadata, dict)
else None
)
if source == marker:
inferred.setdefault(canonical_agent, []).append(name)
matched_names.add(name)
unmatched = [name for name in skill_names if name not in matched_names]
if unmatched and fallback_agent:
fallback_names = inferred.setdefault(fallback_agent, [])
for name in unmatched:
if name not in fallback_names:
fallback_names.append(name)
return inferred
@staticmethod
def _normalize_registered_skills(
value: Any, fallback_agent: Optional[str] = None
@@ -1651,6 +1763,11 @@ class PresetManager:
list to the agent currently being processed so the format
self-migrates on the next write. Without a fallback agent, legacy
lists are dropped rather than guessed at.
Callers that can identify the owning preset (i.e. have a
``pack_id``) should prefer :meth:`_infer_legacy_skill_provenance`
for a legacy flat-list value instead, which probes on-disk
provenance rather than assuming ``fallback_agent`` wrote every name.
"""
if isinstance(value, dict):
return {

View File

@@ -1262,6 +1262,103 @@ class TestExtensionSkillRegistration:
assert "register extension skills for extension 'skill-fail'" in captured.out
assert "Continuing with available registration results" in captured.out
def test_rescaffold_toggle_command_to_skills_removes_stale_extension_command_file(
self, project_dir, temp_dir
):
"""Toggling the *same* active agent from command mode to skills mode
must remove the stale extension command-mode artifact, not just add
the skills-mode one.
Copilot stays the active agent throughout (mirroring ``integration
upgrade copilot --skills``, not a switch to a different agent).
``register_enabled_extensions_for_agent`` skips the commands phase
once ``skills_mode_active`` is true, but before this fix it never
removed the ``.agent.md`` file (and ``registered_commands["copilot"]``
entry) the commands phase previously wrote while command mode was
active — leaving both artifacts on disk at once and violating the
command/skill mutual-exclusion the PR description claims (#2948).
"""
_create_init_options(project_dir, ai="copilot", ai_skills=False)
manager = ExtensionManager(project_dir)
manager.install_from_directory(
_create_extension_dir(temp_dir, ext_id="toggle-ext"), "0.1.0",
register_commands=False,
)
manager.register_enabled_extensions_for_agent("copilot")
agents_dir = project_dir / ".github" / "agents"
cmd_file = agents_dir / "speckit.toggle-ext.hello.agent.md"
assert cmd_file.exists(), "sanity: command mode should write .agent.md"
# Toggle ai_skills on for the same active agent (copilot) and
# rescaffold, mirroring `integration upgrade copilot --skills`.
_create_init_options(project_dir, ai="copilot", ai_skills=True)
manager.register_enabled_extensions_for_agent("copilot")
assert not cmd_file.exists(), (
"the stale command-mode .agent.md file must be removed once "
"this agent toggles to skills mode, not left alongside the "
"new SKILL.md (#2948)"
)
metadata = manager.registry.get("toggle-ext")
registered_commands = metadata.get("registered_commands", {})
assert not registered_commands.get("copilot"), (
"registered_commands tracking for copilot must be cleared "
"once its command file is removed on toggle (#2948)"
)
skills_dir = project_dir / ".github" / "skills"
skill_file = skills_dir / "speckit-toggle-ext-hello" / "SKILL.md"
assert skill_file.exists(), "sanity: skills mode should write SKILL.md"
def test_rescaffold_toggle_skills_to_command_removes_stale_extension_skill_file(
self, project_dir, temp_dir
):
"""Toggling the *same* active agent from skills mode to command mode
must remove the stale extension skills-mode artifact, not just add
the command-mode one.
Mirror image of the command->skills toggle: ``_register_extension_skills``
returns an empty list once skills mode is off for this agent (its
skills directory no longer resolves), but before this fix an empty
result was silently treated as "nothing to register" rather than
"this agent's skill was rendered here previously and is now stale",
so the ``SKILL.md`` this extension wrote while skills mode was
active stayed on disk even though a fresh ``.agent.md`` was written
right alongside it (#2948).
"""
_create_init_options(project_dir, ai="copilot", ai_skills=True)
manager = ExtensionManager(project_dir)
manager.install_from_directory(
_create_extension_dir(temp_dir, ext_id="toggle-ext2"), "0.1.0",
register_commands=False,
)
manager.register_enabled_extensions_for_agent("copilot")
skills_dir = project_dir / ".github" / "skills"
skill_file = skills_dir / "speckit-toggle-ext2-hello" / "SKILL.md"
assert skill_file.exists(), "sanity: skills mode should write SKILL.md"
# Toggle ai_skills off for the same active agent (copilot) and
# rescaffold, mirroring `integration upgrade copilot` (no --skills).
_create_init_options(project_dir, ai="copilot", ai_skills=False)
manager.register_enabled_extensions_for_agent("copilot")
agents_dir = project_dir / ".github" / "agents"
cmd_file = agents_dir / "speckit.toggle-ext2.hello.agent.md"
assert cmd_file.exists(), "sanity: command mode should write .agent.md"
assert not skill_file.exists(), (
"the stale skills-mode SKILL.md file must be removed once this "
"agent toggles to command mode, not left alongside the new "
".agent.md (#2948)"
)
metadata = manager.registry.get("toggle-ext2")
registered_skills = metadata.get("registered_skills", [])
assert "speckit-toggle-ext2-hello" not in registered_skills, (
"registered_skills tracking must be cleared for the removed "
"skill file, not left dangling once it's orphaned (#2948)"
)
def test_existing_agent_command_path_file_is_not_detected(
self, project_dir, temp_dir
):

View File

@@ -4274,6 +4274,68 @@ class TestPresetSkills:
"and preset removal can't clean it up (#2948)"
)
def test_rescaffold_reconciles_override_even_when_skills_phase_fails(
self, project_dir, temp_dir
):
"""A project override must still win after rescaffold even if the
independently-fallible skills phase raises for that preset.
``register_enabled_presets_for_agent`` only records a preset's
command names into ``affected_cmd_names`` — the set later passed to
``_reconcile_composed_commands``/``_reconcile_skills`` — in the
``for tmpl in manifest.templates`` loop that runs *after*
``_register_skills`` inside the per-preset ``try`` block. If
``_register_skills`` raises, the per-preset ``except`` catches it
and ``continue``s before that loop ever runs, so this preset's
command names never make it into ``affected_cmd_names`` even though
``_register_commands`` already wrote its raw content to disk. The
final reconciliation call is skipped for this preset entirely,
leaving the raw preset content in place instead of the project
override that should win (#2948).
"""
self._write_init_options(project_dir, ai="claude", ai_skills=True)
overrides_dir = project_dir / ".specify" / "templates" / "overrides"
overrides_dir.mkdir(parents=True, exist_ok=True)
(overrides_dir / "speckit.specify.md").write_text(
"---\ndescription: Override specify\n---\n\nOverride body\n",
encoding="utf-8",
)
gemini_dir = project_dir / ".gemini" / "commands"
gemini_dir.mkdir(parents=True)
preset_dir = self._create_command_preset(
temp_dir, "reconcile-despite-skills-failure", "speckit.specify",
"Preset specify", "Preset body",
)
manager = PresetManager(project_dir)
manager.install_from_directory(preset_dir, "0.1.5")
# Simulate `integration use gemini` with the skills phase failing
# for this preset (e.g. a symlink/permission error unrelated to the
# commands phase, which already succeeded).
self._write_init_options(project_dir, ai="gemini", ai_skills=False)
from unittest.mock import patch
with patch.object(
PresetManager, "_register_skills",
side_effect=RuntimeError("simulated skills failure"),
):
manager.register_enabled_presets_for_agent("gemini")
cmd_file = gemini_dir / "speckit.specify.toml"
assert cmd_file.exists(), "sanity: gemini should get a command file at all"
content = cmd_file.read_text()
assert "Override body" in content, (
"the project override must still win after rescaffold, even "
"though this preset's skills phase raised — a fallible skills "
"phase must not skip reconciliation for command writes that "
"already succeeded (#2948)"
)
assert "Preset body" not in content
def test_copilot_skills_mode_skips_command_registration(self, project_dir, temp_dir):
"""``integration use copilot`` with skills mode enabled must only
write the SKILL.md mirror, not also copilot's static command file.
@@ -4610,6 +4672,90 @@ class TestPresetSkills:
)
assert "Core specify body" in content
def test_rescaffold_legacy_flat_list_direct_switch_preserves_original_agent(
self, project_dir, temp_dir
):
"""A legacy flat-list ``registered_skills`` entry must not be
misattributed to the wrong agent when the *first* post-upgrade
operation is a direct switch to a different skill-mode agent.
Blindly attributing every legacy flat-list name to ``agent_name`` —
the agent currently being (re)activated — loses the actual writer
whenever that first operation is ``integration use codex`` (or
``switch``) run directly against a legacy Claude override, without
an intervening same-agent rescaffold for Claude first. The
migrated dict then only records ``{"codex": [...]}``, so a later
``remove()`` restores Codex but permanently orphans the Claude
override that was never in the registry to begin with (#2948).
"""
self._write_init_options(project_dir, ai="claude", ai_skills=True)
core_cmds = project_dir / ".specify" / "templates" / "commands"
core_cmds.mkdir(parents=True, exist_ok=True)
(core_cmds / "specify.md").write_text(
"---\ndescription: Core specify command\n---\n\nCore specify body\n",
encoding="utf-8",
)
claude_skills_dir = project_dir / ".claude" / "skills"
self._create_skill(claude_skills_dir, "speckit-specify")
codex_skills_dir = project_dir / ".agents" / "skills"
self._create_skill(codex_skills_dir, "speckit-specify")
preset_dir = self._create_command_preset(
temp_dir, "legacy-direct-switch-preset", "speckit.specify",
"Legacy direct switch test", "preset body",
)
manager = PresetManager(project_dir)
manager.install_from_directory(preset_dir, "0.1.5")
# install_from_directory wrote the preset's override to Claude's
# skill directory (the active agent at install time) — sanity-check
# that the marker is actually there before simulating the legacy
# registry format.
claude_skill = claude_skills_dir / "speckit-specify" / "SKILL.md"
assert "preset:legacy-direct-switch-preset" in claude_skill.read_text(), (
"sanity: install should have written the override under claude"
)
# Simulate a pre-#2948 registry: a flat list with no per-agent
# provenance, even though the file on disk was actually written
# under claude's directory.
manager.registry.update(
"legacy-direct-switch-preset",
{"registered_skills": ["speckit-specify"]},
)
# Directly switch to codex — no intervening rescaffold for claude —
# mirroring `integration use codex` / `switch codex` run right after
# upgrading spec-kit versions.
self._write_init_options(project_dir, ai="codex", ai_skills=True)
manager.register_enabled_presets_for_agent("codex")
metadata = manager.registry.get("legacy-direct-switch-preset")
registered_skills = metadata.get("registered_skills")
assert isinstance(registered_skills, dict)
assert set(registered_skills) == {"claude", "codex"}, (
"migrating a legacy flat-list entry on a direct switch must "
"infer the actual writer (claude) from the existing on-disk "
"SKILL.md provenance, not attribute every name to whichever "
"agent happens to be activated first after the upgrade "
"(#2948)"
)
assert manager.remove("legacy-direct-switch-preset") is True
codex_skill = codex_skills_dir / "speckit-specify" / "SKILL.md"
for skill_file, label in ((claude_skill, "claude"), (codex_skill, "codex")):
assert skill_file.exists(), f"{label} skill file should still exist after removal"
content = skill_file.read_text()
assert "preset:legacy-direct-switch-preset" not in content, (
f"{label}'s preset override must be restored on removal, "
"not permanently orphaned by a misattributed legacy "
"migration (#2948)"
)
assert "Core specify body" in content
def test_symlinked_skills_dir_rejected_on_removal(self, project_dir, temp_dir):
"""Removal must validate a recorded skill directory before touching it.