From aef65375e9783b74857f61ac8a5c12b7a2c4656b Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Sat, 8 Aug 2026 19:19:50 +0500 Subject: [PATCH] fix(presets): skip an unreadable restore source in `preset remove` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_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) --- src/specify_cli/presets/__init__.py | 23 +++++++- tests/test_presets.py | 85 +++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index f01d0c1561..bdb9b174d7 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -3393,8 +3393,19 @@ 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): + continue frontmatter, body = registrar.parse_frontmatter(content) if isinstance(selected_ai, str): body = registrar.resolve_skill_placeholders( @@ -3435,7 +3446,13 @@ 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): + continue frontmatter, body = registrar.parse_frontmatter(content) # Mirror the register-time rewrite (#2101): resolve # extension-relative subdir references (agents/, diff --git a/tests/test_presets.py b/tests/test_presets.py index c35a370608..5a5d4cdd30 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -9174,6 +9174,91 @@ 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) + 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) + 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_rejects_absolute_registry_name( self, project_dir ):