fix(presets): treat an unreadable core template as missing by marcelsafin · Pull Request #3961 · github/spec-kit

marcelsafin · 2026-08-03T19:56:49Z

Description

_substitute_core_template() reads the resolved core template with a bare read_text(), so one corrupted project-owned override in .specify/templates/commands/ crashes the whole wrap-strategy command registration with a raw UnicodeDecodeError. Both callers (CommandRegistrar.register_pack and _register_commands) are unguarded here — even though register_pack already skips an unreadable preset source with a warning a few lines above the call.

Fix: treat an unreadable core template like a missing one — warn and return the body unchanged with empty frontmatter — matching the function's documented no-core contract ("Both are unchanged / empty when … the core template file does not exist").

Testing

Tested locally with uv run specify --help
Ran existing tests with uv sync && uv run pytest (6,310 passed, 176 skipped)
New regression test test_substitute_core_template_unreadable_core_treated_as_missing (fails on main, passes with fix)
ruff check src tests clean

AI Disclosure

I did not use AI assistance for this contribution
I did use AI assistance (describe below)

Implemented autonomously by GitHub Copilot CLI (model: Claude Fable 5) under human direction; TDD (failing test first), full suite and lint verified locally. Commit includes Assisted-by/Co-authored-by trailers.

_substitute_core_template() read the resolved core template with a bare read_text(), so one corrupted project-owned override in .specify/templates/commands/ crashed the whole wrap-strategy command registration with a raw UnicodeDecodeError. Both callers (CommandRegistrar.register_pack and _register_commands) are unguarded here, even though register_pack already skips an unreadable preset source with a warning a few lines above the call. Treat an unreadable core template like a missing one — warn and return the body unchanged with empty frontmatter — matching the function's documented no-core contract. Assisted-by: GitHub Copilot (model: claude-fable-5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Handles unreadable core templates without crashing preset command registration.

Changes:

Warns and falls back when core templates cannot be read or decoded.
Adds regression coverage and updates the function contract.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`src/specify_cli/presets/__init__.py`	Handles unreadable core templates safely.
`tests/test_presets.py`	Tests invalid UTF-8 fallback behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Review follow-up: use pytest.warns so removing or changing the promised warning fails the test. Assisted-by: GitHub Copilot (model: claude-fable-5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/specify_cli/presets/init.py:214

CommandRegistrar has no register_pack method; the guarded caller is register_commands, while the other direct caller is PresetManager._register_skills. Referencing the nonexistent method makes this rationale difficult to verify.

    # the wrap-strategy callers already skip an unreadable preset source with
    # a warning (CommandRegistrar.register_pack).

tests/test_presets.py:10732

This rationale names nonexistent CommandRegistrar.register_pack and says both callers skip unreadable preset sources. The actual call sites are register_commands (guarded) and _register_skills (whose source read is unguarded), so the regression explanation is inaccurate.

        The wrap-strategy callers (``CommandRegistrar.register_pack`` and
        ``_register_commands``) skip an unreadable preset source with a

Copilot

Review details

Suppressed comments (1)

tests/test_presets.py:10728

This docstring incorrectly says both callers skip unreadable preset sources. _register_commands still performs a bare source_file.read_text() at src/specify_cli/presets/__init__.py:2720; only CommandRegistrar.register_pack has that warning boundary. Reword this so the regression test does not document behavior that the other caller lacks.

        The wrap-strategy callers (``CommandRegistrar.register_pack`` and
        ``_register_commands``) skip an unreadable preset source with a
        warning, but the core template read inside

Files reviewed: 2/2 changed files
Comments generated: 0 new
Review effort level: Balanced

mnriem · 2026-08-07T17:50:04Z

Thank you!

…estore 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 github#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>

…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>

Copilot AI review requested due to automatic review settings August 3, 2026 19:56

marcelsafin requested a review from mnriem as a code owner August 3, 2026 19:56

Copilot started reviewing on behalf of marcelsafin August 3, 2026 19:57 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Comment thread tests/test_presets.py Outdated

Copilot AI review requested due to automatic review settings August 3, 2026 20:13

Copilot started reviewing on behalf of marcelsafin August 3, 2026 20:13 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

mnriem requested a balanced review from Copilot August 4, 2026 19:00

Copilot started reviewing on behalf of mnriem August 4, 2026 19:01 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

mnriem merged commit 247abbf into github:main Aug 7, 2026
14 checks passed

Noor-ul-ain001 mentioned this pull request Aug 8, 2026

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

Merged

Navigation Menu

Uh oh!

fix(presets): treat an unreadable core template as missing - #3961

fix(presets): treat an unreadable core template as missing#3961
mnriem merged 2 commits into
github:mainfrom
marcelsafin:fix/core-template-decode

marcelsafin commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

mnriem commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

marcelsafin commented Aug 3, 2026

Description

Testing

AI Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

mnriem commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants