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

[bug-fix] Fix upgrade-overwrites-copilot-skills: pass force=True to extension skill re-registration after upgrade (#3853)

Browse Source 
* Fix upgrade-overwrites-copilot-skills: pass force=True to extension skill re-registration after upgrade

Apply the remediation from the bug assessment on issue #3849.

_register_extension_skills() had a skip guard that refused to overwrite
existing SKILL.md files (protecting user customizations). In the upgrade
path, setup() regenerates all core-template SKILL.md files first, then
calls register_enabled_extensions_for_agent(). The guard then sees those
freshly-written core files as 'existing' and skips every extension, leaving
only core template content on disk.

Fix: add force: bool = False to _register_extension_skills() and thread it
through register_enabled_extensions_for_agent() and
_register_extensions_for_agent(). In integration_upgrade(), pass force=True
so extension content layers on top of the just-regenerated core files.

The force flag is off-by-default so plain extension add still protects
user-modified skill files.

Refs #3849

Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Potential fix for pull request finding 'Unused local variable'

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

* test: add end-to-end regression guard for upgrade-overwrites-copilot-skills (#3849)

The existing regression tests in TestRegisterExtensionSkillsForceFlag exercise
the new force parameter at the helper level, so without the fix they fail only
with a TypeError (unknown kwarg) rather than on the user-facing behaviour.

Add a command-level test that runs 'specify integration upgrade copilot --skills
--force' end-to-end and asserts the installed git extension's SKILL.md is
restored (with its extension content, not a bare core-template stub) when the
skill directory already exists — the exact skill_dir_preexists path the bug
depends on. The test fails on pre-fix source (the skill is never recreated) and
passes with the fix, so it is a genuine behavioural regression guard rather than
an API-surface check.

Refs #3849

Assisted-by: GitHub Copilot (model: claude-opus-4.8, autonomous)

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Manfred Riem <15701806+mnriem@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
This commit is contained in:
github-actions[bot]
2026-07-29 10:03:14 -05:00
committed by GitHub
parent 89126f3a33
commit 8394c8d536
5 changed files with 213 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
src/specify_cli/extensions/__init__.py
View File

@@ -1324,6 +1324,7 @@ class ExtensionManager:
manifest: ExtensionManifest,
extension_dir: Path,
link_outputs: bool = False,
force: bool = False,
) -> List[str]:
"""Generate SKILL.md files for extension commands as agent skills.
@@ -1337,6 +1338,11 @@ class ExtensionManager:
extension_dir: Installed extension directory.
link_outputs: If True, create dev-mode symlinks for rendered
skill files when supported by the OS.
force: If True, overwrite existing SKILL.md files even when they
are not dev-mode symlinks. Use in the upgrade path, where
``setup()`` has just freshly regenerated core-template skill
files and the skip guard would otherwise prevent extension
content from being layered on top.
Returns:
List of skill names that were created (for registry storage).
@@ -1424,13 +1430,16 @@ class ExtensionManager:
)
# Do not overwrite user-customized skills, but allow dev-mode
# symlinks that point back to this extension's generated cache
# to be refreshed on a subsequent dev install.
if not is_expected_dev_symlink:
# to be refreshed on a subsequent dev install. In the upgrade
# path (force=True) the file was just written by setup(), so
# overwriting it with the composed extension content is correct.
if not is_expected_dev_symlink and not force:
continue
elif skill_dir_preexists:
elif skill_dir_preexists and not force:
# Never add files to a pre-existing user directory. Without a
# verifiable SKILL.md ownership marker, rollback/removal cannot
# distinguish our output from unrelated user artifacts.
# Skipped when force=True (upgrade path).
continue
# Create skill directory; track whether we created it so we can clean
@@ -2669,7 +2678,7 @@ class ExtensionManager:
if updates:
self.registry.update(ext_id, updates)
def register_enabled_extensions_for_agent(self, agent_name: str) -> None:
def register_enabled_extensions_for_agent(self, agent_name: str, *, force: bool = False) -> None:
"""Register installed, enabled extensions for ``agent_name``.
Command-file registration is scoped to the explicit ``agent_name``
@@ -2787,7 +2796,7 @@ class ExtensionManager:
if agent_name == active_agent:
try:
registered_skills = self._register_extension_skills(
manifest, ext_dir
manifest, ext_dir, force=force
)
except Exception as skills_err:
# Skills are a companion artifact. If command registration

8
src/specify_cli/integrations/_helpers.py
View File

@@ -395,6 +395,7 @@ def _register_extensions_for_agent(
agent_key: str,
*,
continuing: str,
force: bool = False,
) -> None:
"""Register all enabled extensions' commands/skills for ``agent_key``.
@@ -408,6 +409,11 @@ def _register_extensions_for_agent(
before registering), so extension *skill* rendering — which is scoped to
the active ``ai`` / ``ai_skills`` init-options — matches ``agent_key``.
When ``force=True``, existing skill files are overwritten even when they
are not dev-mode symlinks. Pass ``force=True`` in the upgrade path so that
extension content is layered on top of the core-template files that
``setup()`` just regenerated (fixes the skip-guard bug for skills mode).
Best-effort: never aborts the surrounding integration operation. Callers
invoke it *after* the use/upgrade/switch transaction has committed so a
failure here cannot trigger a rollback.
@@ -415,7 +421,7 @@ def _register_extensions_for_agent(
_best_effort_extension_op(
project_root,
agent_key,
lambda mgr, key: mgr.register_enabled_extensions_for_agent(key),
lambda mgr, key: mgr.register_enabled_extensions_for_agent(key, force=force),
phase="register extension artifacts for",
continuing=continuing,
)

1
src/specify_cli/integrations/_migrate_commands.py
View File

@@ -886,6 +886,7 @@ def integration_upgrade(
_register_extensions_for_agent(
project_root,
key,
force=True,
continuing="The integration was upgraded, but installed extensions may need re-registration.",
)
_register_presets_for_agent(

62
tests/integrations/test_integration_subcommand.py
View File

@@ -3831,6 +3831,68 @@ class TestIntegrationUpgrade:
"upgrade of the active integration re-registers extension commands"
)
def test_upgrade_copilot_skills_restores_extension_skill_over_regenerated_dir(
self, tmp_path
):
"""End-to-end regression for #3849 (upgrade-overwrites-copilot-skills).
In Copilot skills mode, ``integration upgrade`` runs ``setup()`` — which
regenerates the core-template skill directories — *before* re-registering
installed extensions. The extension re-registration then hits the
``skill_dir_preexists`` guard in ``_register_extension_skills`` (the skill
sub-directory exists, courtesy of ``setup()``, but its ``SKILL.md`` has
not been rewritten with extension content), so pre-fix the extension
skill was silently left missing — its command content lost even though the
extension remained installed and registered.
The fix threads ``force=True`` from ``integration_upgrade()`` down to
``_register_extension_skills`` so the guard is bypassed and the extension
content is re-composed on top of the just-regenerated directory. This test
exercises the full ``specify integration upgrade`` command path and fails
without the fix (the skill is never recreated).
"""
project = _init_project(
tmp_path, "copilot", integration_options="--skills"
)
result = _run_in_project(project, ["extension", "add", "git"])
assert result.exit_code == 0, f"extension add failed: {result.output}"
skill_dir = project / ".github" / "skills" / "speckit-git-feature"
skill_file = skill_dir / "SKILL.md"
assert skill_file.exists(), (
"precondition: git extension renders as a Copilot skill"
)
original = skill_file.read_text(encoding="utf-8")
assert "source: extension:git" in original, (
"precondition: skill carries the git extension ownership marker"
)
# Simulate the exact pre-condition the bug depends on: the skill file is
# gone but its directory survives (as it does once setup() regenerates the
# core-template layout during upgrade), triggering the skill_dir_preexists
# skip guard on re-registration.
skill_file.unlink()
assert skill_dir.exists() and not skill_file.exists()
result = _run_in_project(project, [
"integration", "upgrade", "copilot",
"--integration-options", "--skills",
"--script", "sh", "--force",
])
assert result.exit_code == 0, result.output
assert skill_file.exists(), (
"upgrade must restore the extension skill even when its directory "
"already exists (regression #3849)"
)
restored = skill_file.read_text(encoding="utf-8")
assert "source: extension:git" in restored, (
"restored skill must contain the git extension content, not a bare "
"core-template stub"
)
assert "# Git Feature Skill" in restored
def test_upgrade_active_integration_reregisters_presets(self, tmp_path):
"""Upgrading the active integration restores missing preset artifacts."""
import yaml

129
tests/test_extension_skills.py
View File

@@ -2943,6 +2943,135 @@ class TestExtensionSkillRegistration:
assert "speckit-early-ext-world" in metadata["registered_skills"]
# ===== Regression test: upgrade-overwrites-copilot-skills (#3849) =====
class TestRegisterExtensionSkillsForceFlag:
"""Regression tests for the ``force`` flag on ``_register_extension_skills``.
Issue #3849: ``integration upgrade --force`` called ``setup()`` which
regenerated all core-template SKILL.md files, then called
``register_enabled_extensions_for_agent()``. The skip-guard in
``_register_extension_skills`` treated the freshly-written core files as
existing user content and skipped every extension skill, leaving only core
template content on disk.
The fix introduces ``force=True`` in the upgrade path so the guard does not
fire for core-template files that setup() just wrote.
"""
def test_force_false_skips_existing_skill(self, project_dir, temp_dir):
"""Without force=True the skip guard must still protect existing files."""
_create_init_options(project_dir, ai="claude", ai_skills=True)
skills_dir = _create_skills_dir(project_dir, ai="claude")
ext_dir = _create_extension_dir(temp_dir)
# Manually pre-create a SKILL.md as if setup() had already written it
skill_subdir = skills_dir / "speckit-test-ext-hello"
skill_subdir.mkdir(parents=True, exist_ok=True)
skill_file = skill_subdir / "SKILL.md"
skill_file.write_text("core-template content only", encoding="utf-8")
manager = ExtensionManager(project_dir)
manifest = ExtensionManifest(ext_dir / "extension.yml")
# Default (force=False): existing file must not be overwritten
written = manager._register_extension_skills(manifest, ext_dir, force=False)
assert "speckit-test-ext-hello" not in written
assert skill_file.read_text(encoding="utf-8") == "core-template content only"
def test_force_true_overwrites_existing_skill(self, project_dir, temp_dir):
"""With force=True the function must overwrite the existing SKILL.md.
This is the core regression test for #3849: calling
``_register_extension_skills(force=True)`` after ``setup()`` has
written a fresh core-template SKILL.md must replace it with the
composed extension content.
"""
_create_init_options(project_dir, ai="claude", ai_skills=True)
skills_dir = _create_skills_dir(project_dir, ai="claude")
ext_dir = _create_extension_dir(temp_dir)
# Simulate what setup() writes: a bare core-template SKILL.md
skill_subdir = skills_dir / "speckit-test-ext-hello"
skill_subdir.mkdir(parents=True, exist_ok=True)
skill_file = skill_subdir / "SKILL.md"
skill_file.write_text("core-template content only", encoding="utf-8")
manager = ExtensionManager(project_dir)
manifest = ExtensionManifest(ext_dir / "extension.yml")
# Upgrade path (force=True): extension content should replace the core file
written = manager._register_extension_skills(manifest, ext_dir, force=True)
assert "speckit-test-ext-hello" in written, (
"force=True should overwrite the core-template file and return the skill name"
)
content = skill_file.read_text(encoding="utf-8")
assert "Run this to say hello." in content, (
"Extension command body must appear in the overwritten SKILL.md"
)
assert "core-template content only" not in content, (
"Core-template placeholder must have been replaced by extension content"
)
def test_register_enabled_extensions_for_agent_force_flag_threads_through(
self, project_dir, temp_dir
):
"""force=True on register_enabled_extensions_for_agent must reach _register_extension_skills.
End-to-end check: after an upgrade writes a fresh core-template SKILL.md,
``register_enabled_extensions_for_agent(force=True)`` must produce a
SKILL.md that contains the extension content.
"""
_create_init_options(project_dir, ai="claude", ai_skills=True)
skills_dir = _create_skills_dir(project_dir, ai="claude")
ext_dir = _create_extension_dir(temp_dir)
manager = ExtensionManager(project_dir)
# Install extension so it is in the registry
manager.install_from_directory(ext_dir, "0.1.0", register_commands=False)
# Simulate a freshly-regenerated core-template SKILL.md (as setup() would write)
skill_file = skills_dir / "speckit-test-ext-hello" / "SKILL.md"
skill_file.write_text("core-template content only", encoding="utf-8")
# Re-register with force=True (upgrade path)
manager.register_enabled_extensions_for_agent("claude", force=True)
content = skill_file.read_text(encoding="utf-8")
assert "Run this to say hello." in content, (
"After register_enabled_extensions_for_agent(force=True), the SKILL.md "
"must contain the extension body, not just the core-template stub."
)
def test_force_true_with_preexisting_dir_but_no_skill_file(
self, project_dir, temp_dir
):
"""force=True must write into a pre-existing directory with no SKILL.md.
The second skip guard (``elif skill_dir_preexists``) should also be
bypassed by force=True so an upgrade can create a missing SKILL.md
even when the skill sub-directory already exists.
"""
_create_init_options(project_dir, ai="claude", ai_skills=True)
skills_dir = _create_skills_dir(project_dir, ai="claude")
ext_dir = _create_extension_dir(temp_dir)
# Create the skill directory without the SKILL.md file
skill_subdir = skills_dir / "speckit-test-ext-hello"
skill_subdir.mkdir(parents=True, exist_ok=True)
skill_file = skill_subdir / "SKILL.md"
assert not skill_file.exists()
manager = ExtensionManager(project_dir)
manifest = ExtensionManifest(ext_dir / "extension.yml")
written = manager._register_extension_skills(manifest, ext_dir, force=True)
assert "speckit-test-ext-hello" in written
assert skill_file.exists()
assert "Run this to say hello." in skill_file.read_text(encoding="utf-8")
# ===== Extension Skill Unregistration Tests =====
class TestExtensionSkillUnregistration: