fix(presets): skip an unreadable restore source in preset remove (#4020)

* fix(presets): skip an unreadable restore source in `preset remove`

`_unregister_skills_in_dir` restores each preset-owned SKILL.md from a core
command template or an extension source. Both of those reads were bare
`read_text(encoding="utf-8")` calls, so a project-owned override in
`.specify/templates/commands/` that exists but cannot be read or decoded
raised a raw `UnicodeDecodeError`/`OSError` straight out of
`PresetManager.remove()`, which has no handler for it — `specify preset
remove` dies with a traceback.

Every other failure in this loop degrades with `continue`: an unsafe
registry name, a missing skill subdirectory, a foreign owner. Sibling reads
of the very same directory are already guarded — `_infer_legacy_skill_
provenance` and `_delete_agent_preset_skills` both wrap their SKILL.md read
in `except (OSError, UnicodeDecodeError): continue`, and the read inside
`_substitute_core_template` was just given the same boundary in #3961. The
two restore reads were the remaining gap.

`continue` is the right recovery here rather than falling through: the
`else` branch below removes the skill outright, so treating an unreadable
source as "no source" would delete a user's skill at exactly the moment its
replacement cannot be generated. Skipping leaves the skill in place and
keeps it out of the returned `mutated_names`, so callers don't record a
restore that never happened.

Two regression tests, one per exception arm: a non-UTF-8 core template, and
a mocked `PermissionError` so the `OSError` half is also covered under
privileged CI where permission bits aren't enforced. Both assert the skill
survives untouched and is not reported as mutated.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(presets): warn when a skill keeps preset content after a failed restore

Review follow-up on two points.

Surface the skipped restore. Skipping is still the correct recovery — the
alternative branch deletes the skill — but it was silent, and it is a
partial removal: `remove()` goes on to delete the preset directory and the
registry entry, while this `SKILL.md` keeps the removed preset's content,
and leaving the name out of `mutated_names` also keeps it out of
reconciliation, so nothing retries it. Both arms now emit a warning naming
the skill, the unreadable source, and the exception, and pointing at the
re-run that refreshes it once the file is fixed. `warnings.warn` matches
how the surrounding code reports non-fatal degradation (the reconciliation
failures in `remove()`/`install_from_directory`, the unreadable core
template in `_substitute_core_template` from #3961).

Cover the extension arm. A skill backed by an installed extension never
reaches the core-template read, so the two branches can regress
independently and both prior tests exercised only the core one.
`test_unregister_skills_in_dir_unreadable_extension_source_skips` installs
an extension whose command file is non-UTF-8 and asserts the skill survives
byte-for-byte and is absent from `mutated_names`. Verified it raises the
raw `UnicodeDecodeError` against unpatched source. The two existing tests
now assert the warning via `pytest.warns` so dropping it fails the suite.

pytest tests/test_presets.py -> 583 passed, 2 skipped, 7 failed; the 7 are
the pre-existing Windows symlink tests that need elevation, unchanged from
main. ruff check passes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: Noor-ul-ain001

src/specify_cli/presets/__init__.py

@@ -3268,6 +3268,30 @@ def _delete_agent_preset_skills(
             if source in owned_sources:
                 shutil.rmtree(skill_subdir)
 
+    @staticmethod
+    def _warn_unrestored_skill(
+        skill_name: str, source_file: Path, exc: BaseException
+    ) -> None:
+        """Warn that a skill kept preset content because its restore source is unreadable.
+
+        Skipping the restore is the safe recovery — the alternative branch
+        deletes the skill outright — but it is still a partial removal: the
+        preset directory and registry entry go away while this ``SKILL.md``
+        keeps the removed preset's content, and reconciliation never revisits
+        it because the name is left out of ``mutated_names``. Name the skill
+        and the source so the condition is actionable instead of silent.
+        """
+        import warnings
+
+        warnings.warn(
+            f"Skill '{skill_name}' still contains the removed preset's content: "
+            f"its restore source '{source_file}' could not be read "
+            f"({exc.__class__.__name__}: {exc}). The skill was left in place "
+            f"rather than deleted. Fix or remove that file and re-run "
+            f"'specify preset add'/'specify preset remove' to refresh it.",
+            stacklevel=2,
+        )
+
     def _unregister_skills_in_dir(
         self,
         skill_names: List[str],
@@ -3394,8 +3418,20 @@ def _unregister_skills_in_dir(
                 core_file = None
 
             if core_file:
-                # Restore from core template
-                content = core_file.read_text(encoding="utf-8")
+                # Restore from core template. An unreadable/undecodable
+                # source cannot produce restored content, so leave the
+                # existing skill untouched rather than leaking a raw
+                # OSError/UnicodeDecodeError out of `preset remove` — and
+                # rather than falling through to the rmtree below, which
+                # would delete a skill precisely when its replacement
+                # cannot be generated. Matches the `continue` guards above
+                # (unsafe name, missing subdir, foreign owner), which also
+                # skip without recording the name as mutated.
+                try:
+                    content = core_file.read_text(encoding="utf-8")
+                except (OSError, UnicodeDecodeError) as exc:
+                    self._warn_unrestored_skill(skill_name, core_file, exc)
+                    continue
                 frontmatter, body = registrar.parse_frontmatter(content)
                 if isinstance(selected_ai, str):
                     body = registrar.resolve_skill_placeholders(
@@ -3436,7 +3472,16 @@ def _unregister_skills_in_dir(
                 continue
 
             if extension_restore:
-                content = extension_restore["source_file"].read_text(encoding="utf-8")
+                # Same boundary as the core-template branch above: an
+                # unreadable extension source leaves the skill in place
+                # instead of crashing or being deleted.
+                try:
+                    content = extension_restore["source_file"].read_text(encoding="utf-8")
+                except (OSError, UnicodeDecodeError) as exc:
+                    self._warn_unrestored_skill(
+                        skill_name, extension_restore["source_file"], exc
+                    )
+                    continue
                 frontmatter, body = registrar.parse_frontmatter(content)
                 # Mirror the register-time rewrite (#2101): resolve
                 # extension-relative subdir references (agents/,

tests/test_presets.py

@@ -9452,6 +9452,155 @@ def test_unregister_legacy_fallback_skips_non_owned_skill(
             "---\nname: speckit-specify\n---\n\nuser-owned content\n"
         )
 
+    def test_unregister_skills_in_dir_unreadable_core_template_skips(
+        self, project_dir
+    ):
+        """An undecodable core template must not crash `preset remove`.
+
+        Every other failure in the restore loop — an unsafe registry name,
+        a missing skill subdirectory, a foreign owner — skips the skill
+        with ``continue``. The core-template read was outside that
+        boundary, so one non-UTF-8 project-owned override in
+        ``.specify/templates/commands/`` raised a raw ``UnicodeDecodeError``
+        straight out of ``PresetManager.remove()``, which has no handler
+        for it. Sibling reads of the very same directory are already
+        guarded (``_substitute_core_template``, the provenance reads in
+        ``_infer_legacy_skill_provenance``).
+        """
+        self._write_init_options(project_dir, ai="claude", ai_skills=True)
+        skills_dir = project_dir / ".claude" / "skills"
+        skill_dir = self._create_skill(
+            skills_dir, "speckit-specify", "installed content"
+        )
+        core_commands = project_dir / ".specify" / "templates" / "commands"
+        core_commands.mkdir(parents=True, exist_ok=True)
+        (core_commands / "specify.md").write_bytes(
+            b"---\ndescription: \xff\xfe not utf-8\n---\n\nCore body\n"
+        )
+
+        manager = PresetManager(project_dir)
+        with pytest.warns(UserWarning, match="speckit-specify"):
+            mutated = manager._unregister_skills_in_dir(
+                ["speckit-specify"], skills_dir, "claude"
+            )
+
+        assert mutated == [], (
+            "a skill whose restore source could not be read was not "
+            "restored, so it must not be reported as mutated"
+        )
+        assert (skill_dir / "SKILL.md").read_text(encoding="utf-8") == (
+            "---\nname: speckit-specify\n---\n\ninstalled content\n"
+        ), (
+            "an unreadable core template must leave the skill untouched — "
+            "falling through to the rmtree branch would delete it exactly "
+            "when its replacement cannot be generated"
+        )
+
+    def test_unregister_skills_in_dir_unreadable_core_template_oserror_skips(
+        self, project_dir, monkeypatch
+    ):
+        """The same boundary must cover ``OSError`` (e.g. permission denied).
+
+        Mocked rather than chmod-based so the case also holds under
+        privileged CI, where permission bits are not enforced.
+        """
+        self._write_init_options(project_dir, ai="claude", ai_skills=True)
+        skills_dir = project_dir / ".claude" / "skills"
+        skill_dir = self._create_skill(
+            skills_dir, "speckit-specify", "installed content"
+        )
+        core_commands = project_dir / ".specify" / "templates" / "commands"
+        core_commands.mkdir(parents=True, exist_ok=True)
+        core_template = core_commands / "specify.md"
+        core_template.write_text(
+            "---\ndescription: Core specify\n---\n\nCore body\n",
+            encoding="utf-8",
+        )
+
+        original_read_text = Path.read_text
+
+        def failing_read_text(self_path, *args, **kwargs):
+            if self_path == core_template:
+                raise PermissionError(13, "Permission denied")
+            return original_read_text(self_path, *args, **kwargs)
+
+        monkeypatch.setattr(Path, "read_text", failing_read_text)
+
+        manager = PresetManager(project_dir)
+        with pytest.warns(UserWarning, match="speckit-specify"):
+            mutated = manager._unregister_skills_in_dir(
+                ["speckit-specify"], skills_dir, "claude"
+            )
+
+        monkeypatch.undo()
+
+        assert mutated == []
+        assert (skill_dir / "SKILL.md").read_text(encoding="utf-8") == (
+            "---\nname: speckit-specify\n---\n\ninstalled content\n"
+        )
+
+    def test_unregister_skills_in_dir_unreadable_extension_source_skips(
+        self, project_dir
+    ):
+        """The extension-restore arm needs the same boundary as the core arm.
+
+        The two restore reads are independent branches — a skill backed by an
+        installed extension never reaches the core-template read — so this
+        half of the guard can regress on its own. An undecodable extension
+        command file must warn, leave the skill byte-for-byte intact, and stay
+        out of ``mutated_names``.
+        """
+        self._write_init_options(project_dir, ai="claude", ai_skills=True)
+        skills_dir = project_dir / ".claude" / "skills"
+        skill_dir = self._create_skill(
+            skills_dir, "speckit-fakeext-cmd", "installed content"
+        )
+
+        extension_dir = project_dir / ".specify" / "extensions" / "fakeext"
+        (extension_dir / "commands").mkdir(parents=True, exist_ok=True)
+        (extension_dir / "commands" / "cmd.md").write_bytes(
+            b"---\ndescription: \xff\xfe not utf-8\n---\n\nExtension body\n"
+        )
+        extension_manifest = {
+            "schema_version": "1.0",
+            "extension": {
+                "id": "fakeext",
+                "name": "Fake Extension",
+                "version": "1.0.0",
+                "description": "Test",
+            },
+            "requires": {"speckit_version": ">=0.1.0"},
+            "provides": {
+                "commands": [
+                    {
+                        "name": "speckit.fakeext.cmd",
+                        "file": "commands/cmd.md",
+                        "description": "Fake extension command",
+                    }
+                ]
+            },
+        }
+        with open(extension_dir / "extension.yml", "w") as f:
+            yaml.dump(extension_manifest, f)
+
+        manager = PresetManager(project_dir)
+        with pytest.warns(UserWarning, match="speckit-fakeext-cmd"):
+            mutated = manager._unregister_skills_in_dir(
+                ["speckit-fakeext-cmd"], skills_dir, "claude"
+            )
+
+        assert mutated == [], (
+            "a skill whose extension restore source could not be read was "
+            "not restored, so it must not be reported as mutated"
+        )
+        assert (skill_dir / "SKILL.md").read_text(encoding="utf-8") == (
+            "---\nname: speckit-fakeext-cmd\n---\n\ninstalled content\n"
+        ), (
+            "an unreadable extension source must leave the skill untouched — "
+            "falling through to the rmtree branch would delete it exactly "
+            "when its replacement cannot be generated"
+        )
+
     def test_unregister_skills_in_dir_rejects_absolute_registry_name(
         self, project_dir
     ):