fix(presets): skip an unreadable restore source in preset remove - #4020
Open
Noor-ul-ain001 wants to merge 1 commit into
Open
fix(presets): skip an unreadable restore source in preset remove#4020Noor-ul-ain001 wants to merge 1 commit into
preset remove#4020Noor-ul-ain001 wants to merge 1 commit into
Conversation
`_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 github#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PresetManager._unregister_skills_in_dirrestores each preset-ownedSKILL.mdfrom either a core command template or an extension source. Both reads were bare: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 rawUnicodeDecodeError/OSError.PresetManager.remove()has no handler for it, and neither does thepreset removeCLI command, sospecify preset remove <id>dies with a traceback.Reproduced against
main: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 #3961So 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 and
continue.continueis deliberately the recovery, not falling through: theelsebranch 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 returnedmutated_names, so callers don't persist a restore that never happened.Tests
Two regression tests, one per exception arm:
test_unregister_skills_in_dir_unreadable_core_template_skips— non-UTF-8 core templatetest_unregister_skills_in_dir_unreadable_core_template_oserror_skips— mockedPermissionError, so theOSErrorhalf is covered under privileged CI where permission bits aren't enforcedBoth assert the skill survives byte-for-byte and is not reported as mutated.
Verified they fail without the source change (
PermissionError/UnicodeDecodeErrorraised) and pass with it.Verification
pytest tests/test_presets.py→ 582 passed, 2 skipped, 7 failedtest_symlinked_*,test_dangling_symlink_fails_closed). Confirmed identical on a clean checkout ofmainwith my changes stashed — unrelated to this PR.ruff check→ All checks passed🤖 Generated with Claude Code