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

Problem

PresetManager._unregister_skills_in_dir restores each preset-owned SKILL.md from either a core command template or an extension source. Both reads were bare:

if core_file:
    # Restore from core template
    content = core_file.read_text(encoding="utf-8")   # src/specify_cli/presets/__init__.py:3397
...
if extension_restore:
    content = extension_restore["source_file"].read_text(encoding="utf-8")   # :3438

A file that exists but cannot be read or decoded — a project-owned override in .specify/templates/commands/ saved as UTF-16/Latin-1, or one with restrictive permissions — therefore raises a raw UnicodeDecodeError/OSError. PresetManager.remove() has no handler for it, and neither does the preset remove CLI command, so specify preset remove <id> dies with a traceback.

Reproduced against main:

File ".../src/specify_cli/presets/__init__.py", line 3397, in _unregister_skills_in_dir
    content = core_file.read_text(encoding="utf-8")
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 17: invalid start byte

Why this is a gap rather than a design choice

Every other failure in this same loop degrades with continue — an unsafe registry name, a missing skill subdirectory, a foreign owner. And the sibling reads of the very same files are already guarded:

• _infer_legacy_skill_provenance — except (OSError, UnicodeDecodeError): continue (:2917)
• _delete_agent_preset_skills — same clause (:3253)
• _unregister_skills_in_dir's own ownership check a few lines above the bug — same clause (:3345)
• _substitute_core_template, which reads the identical .specify/templates/commands/ directory, received this boundary in fix(presets): treat an unreadable core template as missing #3961

So within one function, the ownership read is guarded and the restore read is not. The two restore reads were the remaining gap in an otherwise-complete boundary.

Fix

Wrap both reads, warn, and continue.

continue is deliberately the recovery, not falling through: the else branch below removes the skill outright (shutil.rmtree), so treating an unreadable source as "no source available" 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 persist a restore that never happened.

Because skipping is nonetheless a partial removal — the preset directory and registry entry are removed while this SKILL.md keeps the removed preset's content, and its absence from mutated_names also keeps it out of reconciliation — both arms report it through a shared _warn_unrestored_skill helper:

Skill 'speckit-specify' still contains the removed preset's content: its
restore source '<path>' could not be read (UnicodeDecodeError: ...). The
skill was left in place rather than deleted. Fix or remove that file and
re-run 'specify preset add'/'specify preset remove' to refresh it.

warnings.warn matches how the surrounding code reports non-fatal degradation that shouldn't roll back a persisted change — the reconciliation failures in remove()/install_from_directory, and the unreadable core template in _substitute_core_template (#3961).

Tests

Three regression tests, one per exception arm plus one per restore branch:

• test_unregister_skills_in_dir_unreadable_core_template_skips — non-UTF-8 core template
• test_unregister_skills_in_dir_unreadable_core_template_oserror_skips — mocked PermissionError, so the OSError half is covered under privileged CI where permission bits aren't enforced
• test_unregister_skills_in_dir_unreadable_extension_source_skips — non-UTF-8 extension command file; the extension branch is unreachable from the core-template tests (a skill backed by an installed extension never reaches that read), so it can regress independently

All three assert the skill survives byte-for-byte, is not reported as mutated, and that the warning fires (pytest.warns, so dropping it fails the suite).

Verified each fails without the source change — the extension case with the raw UnicodeDecodeError, i.e. reproducing the crash rather than only the missing warning.

Verification

• pytest tests/test_presets.py → 583 passed, 2 skipped, 7 failed
• The 7 failures are all pre-existing Windows symlink tests requiring elevation (test_symlinked_*, test_dangling_symlink_fails_closed). Confirmed identical on a clean checkout of main with my changes stashed — unrelated to this PR.
• ruff check → All checks passed

🤖 Generated with Claude Code