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

[github-actions]

Bug fix — upgrade-overwrites-copilot-skills

Proposed fix for issue #3849, applying the remediation from the bug assessment.

Verdict: Valid · Severity: high

Summary

_register_extension_skills() had a skip guard that refused to overwrite existing SKILL.md files (to protect user customizations). In the upgrade path, setup() regenerates all core-template SKILL.md files first, then calls register_enabled_extensions_for_agent(). The skip guard then sees those freshly-written core files as "existing" and skips every extension, leaving only core template content on disk. This fix introduces a force=True path that bypasses the guard in the upgrade context.

Changes

File	Change	Notes
src/specify_cli/extensions/__init__.py	modified	Add force: bool = False to _register_extension_skills(); bypass skip guards when force=True; thread force through register_enabled_extensions_for_agent()
src/specify_cli/integrations/_helpers.py	modified	Add force: bool = False to _register_extensions_for_agent(); forward it to mgr.register_enabled_extensions_for_agent() lambda
src/specify_cli/integrations/_migrate_commands.py	modified	Pass force=True when calling _register_extensions_for_agent() in integration_upgrade()
tests/test_extension_skills.py	added tests	Four regression tests in TestRegisterExtensionSkillsForceFlag

Tests Added or Updated

• TestRegisterExtensionSkillsForceFlag::test_force_false_skips_existing_skill — confirms the default (non-upgrade) behavior is unchanged: existing files are still protected
• TestRegisterExtensionSkillsForceFlag::test_force_true_overwrites_existing_skill — core regression test: force=True overwrites a core-template stub with extension content
• TestRegisterExtensionSkillsForceFlag::test_register_enabled_extensions_for_agent_force_flag_threads_through — end-to-end: register_enabled_extensions_for_agent(force=True) produces the correct composed SKILL.md
• TestRegisterExtensionSkillsForceFlag::test_force_true_with_preexisting_dir_but_no_skill_file — force=True also bypasses the second skip guard (pre-existing dir, no file)

Local Verification

• No project test command is available in the CI runner environment (no uv / venv present). Changes verified by code inspection against the assessment root cause and confirmed guard logic is correct.

Deviations from Assessment

None. The preferred remediation (add force flag, thread through call chain, pass force=True in upgrade path) was implemented as described. The skill_dir_preexists guard was also conditioned on not force as a natural follow-on (the assessment focused on the skill_file.exists() guard but the dir-preexists guard would have the same effect when a dir exists without a SKILL.md).

Risks & Review Notes

• force is False by default — no behavior change for extension add, use, or switch paths
• Only integration_upgrade() passes force=True, scoped to the active integration only (guarded by if key == installed_key)
• The switch path continues to call _register_extensions_for_agent() without force, which is correct: switch starts fresh and should not overwrite user-customized skills

Refs #3849 · cc @dtimon

• Fixes [Bug]:  integration upgrade overwrites extension-and preset- provided Copilot skills #3849

Generated by 🛠️ Fix Bug from Labeled Issue for issue #3849 · 1.4K AIC · ⌖ 17.5 AIC · ⊞ 36.4K · ◷

[Copilot]

Pull request overview

Adds forced extension-skill re-registration after integration upgrades to restore extension content.

Changes:

• Threads a force flag through extension registration.
• Bypasses existing-skill guards during upgrades.
• Adds force-flag regression tests.

Show a summary per file

File	Description
src/specify_cli/extensions/__init__.py	Adds forced skill overwrites.
src/specify_cli/integrations/_helpers.py	Forwards the force flag.
src/specify_cli/integrations/_migrate_commands.py	Enables forced upgrade re-registration.
tests/test_extension_skills.py	Adds regression tests.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Medium

[Copilot]

Review details

Comments suppressed due to low confidence (2)

src/specify_cli/integrations/_migrate_commands.py:889

• Pass the CLI's force value through instead of enabling this unconditionally. integration upgrade also runs without --force, and extension-only skills are tracked in the extension registry rather than the integration manifest, so old_manifest.check_modified() cannot detect a user-edited extension skill. As written, an ordinary upgrade now overwrites that customization even though the user did not opt into forced replacement; forwarding force still fixes the reported upgrade --force path.

            force=True,

src/specify_cli/extensions/init.py:1438

• Do not let force bypass the guard for a symlinked skill directory. The active skills root is validated, but this per-skill child is not; if .github/skills/<skill-name> is a symlink to an outside directory, skill_file.exists() follows it and the forced write below overwrites a file outside the project. Reject a symlinked skill_subdir before examining or writing SKILL.md.

                if not is_expected_dev_symlink and not force:
                    continue
            elif skill_dir_preexists and not force:

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Medium

[Copilot]

Review details

Comments suppressed due to low confidence (3)

src/specify_cli/extensions/init.py:1436

• force bypasses the ownership guard for every extension skill, although setup() regenerates only core-template skills. Extension-only skill files are tracked in the extension registry rather than the integration manifest, so an active integration upgrade can now overwrite a user-edited extension SKILL.md even without CLI --force; old_manifest.check_modified() cannot detect it. Scope the bypass to the exact paths recorded by the new integration manifest (or pass the regenerated skill-name set) and keep this guard for all other files.

                if not is_expected_dev_symlink and not force:

src/specify_cli/extensions/init.py:1438

• skill_dir_preexists is also true for a directory symlink. With force=True, this now falls through to mkdir() and the later SKILL.md write, following an extension-only skill directory symlink outside the project. Reject a symlinked skill directory unconditionally; force should only relax the policy for an existing regular directory.

            elif skill_dir_preexists and not force:

tests/integrations/test_integration_subcommand.py:3875

• This does not reproduce the reported upgrade flow: speckit-git-feature is extension-only (not a core template), and deleting its file manually means setup() never replaces it with core content. The test therefore covers only the pre-existing-directory guard and can pass while the actual “setup rewrites an overridden core skill” scenario remains broken. Add an end-to-end case that leaves the installed artifact intact, runs upgrade, and verifies the reported extension composition survives.

        # 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()

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Medium