Skip to content

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

Open
Noor-ul-ain001 wants to merge 1 commit into
github:mainfrom
Noor-ul-ain001:fix/preset-remove-unreadable-restore-source
Open

fix(presets): skip an unreadable restore source in preset remove#4020
Noor-ul-ain001 wants to merge 1 commit into
github:mainfrom
Noor-ul-ain001:fix/preset-remove-unreadable-restore-source

Conversation

@Noor-ul-ain001

Copy link
Copy Markdown
Contributor

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_provenanceexcept (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 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.

Tests

Two regression tests, one per exception arm:

  • 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

Both assert the skill survives byte-for-byte and is not reported as mutated.

Verified they fail without the source change (PermissionError / UnicodeDecodeError raised) and pass with it.

Verification

  • pytest tests/test_presets.py582 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

`_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>
@Noor-ul-ain001
Noor-ul-ain001 requested a review from mnriem as a code owner August 8, 2026 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant