github/github-spec-kit
1
0
Fork 0
mirror of https://github.com/github/spec-kit.git synced 2026-08-03 06:26:30 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: broaden legacy skill provenance inference to command-backed agents

Browse Source 
_infer_legacy_skill_provenance() only probed agents whose registrar
config statically declares extension == "/SKILL.md", excluding
command-backed agents (e.g. Copilot) that can also render preset
overrides as SKILL.md files when ai_skills is enabled. A real
preset-owned .github/skills/.../SKILL.md written while Copilot was the
active skills-mode agent was therefore never probed and got
misattributed entirely to whichever agent activated first after the
upgrade, permanently orphaning Copilot's override on later removal.

Broaden the candidate set to every configured integration
(CommandRegistrar.AGENT_CONFIGS), reusing the existing safe-path
helper (_safe_skills_dir_for_agent, itself built on the shared
_get_skills_dir resolver) rather than inventing new path-construction
logic. The existing preset-marker match (metadata.source ==
"preset:<pack_id>") continues to gate every attribution, so
command-mode agents that never rendered this preset's skill are not
falsely attributed.

Add red-first regression tests: a legacy flat-list entry owned by
Copilot in skills mode, switched directly to Claude with no
intervening Copilot rescaffold, now migrates to a per-agent dict
covering both agents, and removal restores both agents' files instead
of orphaning Copilot's override; plus a negative-case test confirming
a command-mode Copilot with no preset-owned skill marker is not
falsely attributed during the same migration.

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 09:03:53 +02:00
parent 1bdb5b3473
commit ee03a5055e
2 changed files with 191 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

71
src/specify_cli/presets/__init__.py
View File

@@ -1667,46 +1667,55 @@ class PresetManager:
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.
*different* agent — e.g. a legacy Copilot override (written while
Copilot was active with ``ai_skills`` enabled) followed directly by
``integration use claude``, with no intervening rescaffold for
Copilot — permanently orphaning Copilot'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.
Every *configured* integration's skills directory is probed (via
the same safe, symlink-validated helpers used for
restore/removal), not only agents whose registrar config is
statically ``/SKILL.md``-only: a command-backed agent (e.g.
Copilot, whose command extension is ``.agent.md``) renders its
preset overrides as ``SKILL.md`` files exactly like a native
skill-only agent whenever it was the active agent with
``ai_skills`` enabled, so excluding it would miss real,
preset-owned provenance and misattribute it to whichever agent
happens to be processed first. Each directory is probed 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) — this marker check is what keeps
the broadened probe from falsely attributing ownership to an
agent's directory that never actually held this preset's override
(e.g. a command-mode agent that never rendered skills, or an
unrelated skill of the same name). 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 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"
)
candidate_agents = sorted(registrar.AGENT_CONFIGS)
# 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
# (e.g. agy/amp/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:
for agent_name in candidate_agents:
skills_dir = self._safe_skills_dir_for_agent(agent_name)
if skills_dir is None:
continue

151
tests/test_presets.py
View File

@@ -4756,6 +4756,157 @@ class TestPresetSkills:
)
assert "Core specify body" in content
def test_rescaffold_legacy_flat_list_infers_command_backed_skills_owner(
self, project_dir, temp_dir
):
"""Legacy provenance inference must also probe command-backed agents
that were running in skills mode, not only agents whose command
registrar config is statically ``/SKILL.md``-only.
Copilot is command-backed (``extension: ".agent.md"``), but with
``ai_skills`` enabled its preset overrides render as ``SKILL.md``
files under ``.github/skills`` exactly like a native skill-only
agent (claude, codex, ...). Before the fix,
``_infer_legacy_skill_provenance`` only probed agents whose
registrar config has a static ``extension == "/SKILL.md"``, so a
real preset-owned ``.github/skills/.../SKILL.md`` written while
Copilot was the active, skills-mode agent was never found — the
legacy flat list was misattributed entirely to whichever agent the
first post-upgrade switch happened to activate, permanently
orphaning Copilot's override on later removal (#2948).
"""
self._write_init_options(project_dir, ai="copilot", 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",
)
copilot_skills_dir = project_dir / ".github" / "skills"
self._create_skill(copilot_skills_dir, "speckit-specify")
claude_skills_dir = project_dir / ".claude" / "skills"
self._create_skill(claude_skills_dir, "speckit-specify")
preset_dir = self._create_command_preset(
temp_dir, "legacy-copilot-skills-preset", "speckit.specify",
"Legacy copilot skills test", "preset body",
)
manager = PresetManager(project_dir)
manager.install_from_directory(preset_dir, "0.1.5")
copilot_skill = copilot_skills_dir / "speckit-specify" / "SKILL.md"
assert "preset:legacy-copilot-skills-preset" in copilot_skill.read_text(), (
"sanity: install should have written the override under "
"copilot's skills directory while copilot was active in "
"skills mode"
)
# Sanity: no command-mode artifact was written either — copilot's
# command file and skills file are mutually exclusive.
assert not list((project_dir / ".github" / "agents").glob("*specify*")), (
"sanity: copilot in skills mode must not also write a command "
"file that could be falsely attributed instead"
)
# Simulate a pre-#2948 registry: a flat list with no per-agent
# provenance, even though the file on disk was actually written
# under copilot's skills directory.
manager.registry.update(
"legacy-copilot-skills-preset",
{"registered_skills": ["speckit-specify"]},
)
# Directly switch to claude — no intervening rescaffold for
# copilot — mirroring `integration use claude` run right after
# upgrading spec-kit versions.
self._write_init_options(project_dir, ai="claude", ai_skills=True)
manager.register_enabled_presets_for_agent("claude")
metadata = manager.registry.get("legacy-copilot-skills-preset")
registered_skills = metadata.get("registered_skills")
assert isinstance(registered_skills, dict)
assert set(registered_skills) == {"copilot", "claude"}, (
"migrating a legacy flat-list entry on a direct switch must "
"infer the actual writer (copilot, running in skills mode) "
"even though copilot's registrar config is command-backed, "
"not just agents with a static /SKILL.md extension (#2948)"
)
assert manager.remove("legacy-copilot-skills-preset") is True
claude_skill = claude_skills_dir / "speckit-specify" / "SKILL.md"
for skill_file, label in ((copilot_skill, "copilot"), (claude_skill, "claude")):
assert skill_file.exists(), f"{label} skill file should still exist after removal"
content = skill_file.read_text()
assert "preset:legacy-copilot-skills-preset" not in content, (
f"{label}'s preset override must be restored on removal, "
"not permanently orphaned by a legacy migration that "
"failed to probe command-backed skills-mode agents (#2948)"
)
assert "Core specify body" in content
def test_infer_legacy_skill_provenance_does_not_falsely_attribute_command_mode_copilot(
self, project_dir, temp_dir
):
"""Broadening provenance inference to command-backed agents must not
falsely attribute ownership to an agent's directory that has no
preset-owned marker.
Copilot stays in plain command mode throughout (no skills ever
rendered there), so ``.github/skills`` never receives this
preset's ``SKILL.md``. Probing copilot's skills directory anyway
(now that inference isn't restricted to static ``/SKILL.md``
agents) must find nothing there and must not invent a false
``"copilot"`` entry (#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")
# Copilot has never been active; its command directory holds an
# unrelated file so the directory exists, but no skills directory
# or SKILL.md was ever written for it.
copilot_commands_dir = project_dir / ".github" / "agents"
copilot_commands_dir.mkdir(parents=True)
preset_dir = self._create_command_preset(
temp_dir, "no-false-attribution-preset", "speckit.specify",
"No false attribution test", "preset body",
)
manager = PresetManager(project_dir)
manager.install_from_directory(preset_dir, "0.1.5")
manager.registry.update(
"no-false-attribution-preset",
{"registered_skills": ["speckit-specify"]},
)
# Rescaffold again for the same agent (claude) with unchanged
# names, triggering the legacy migration path.
manager.register_enabled_presets_for_agent("claude")
metadata = manager.registry.get("no-false-attribution-preset")
registered_skills = metadata.get("registered_skills")
assert isinstance(registered_skills, dict)
assert set(registered_skills) == {"claude"}, (
"copilot must not appear in the migrated registry when it has "
"never actually rendered this preset's skill — probing its "
"directory for a marker match must not create a false "
"attribution (#2948)"
)
assert not (project_dir / ".github" / "skills").exists(), (
"no .github/skills directory should have been created as a "
"side effect of probing for provenance (#2948)"
)
def test_symlinked_skills_dir_rejected_on_removal(self, project_dir, temp_dir):
"""Removal must validate a recorded skill directory before touching it.