[kartsan03]

Description

When a preset's skill is cleaned up, _PresetSkillMethods removes the whole skill directory with shutil.rmtree(), although Spec Kit only generated the SKILL.md in it. Skills can keep extra files next to SKILL.md (references/, scripts/, assets/), so anything the user put there is deleted along with it. Two paths do this:

• specify preset remove (_unregister_skills_in_dir, when no core or extension template takes the skill back);
• specify integration switch away from an agent (_delete_agent_preset_skills).

Both now remove only the generated SKILL.md and then remove the skill directory only if it is empty. They go through _file_changes.unlink_in_directory(), which keeps the deletions journaled and deletes relative to directory handles below the project root, so a skill directory replaced by a link isn't followed. A directory with nothing else in it is still removed, as before. On a later switch back, the preset's SKILL.md is generated again next to the user's files.

Repro with the bundled scaffold preset (it overrides speckit.myext.myextcmd, which becomes the speckit-myext-myextcmd skill for Copilot):

specify init --here --integration copilot --script sh --ignore-agent-tools --offline
specify preset add --dev <spec-kit>/presets/scaffold
mkdir -p .github/skills/speckit-myext-myextcmd/references
echo 'my notes' > .github/skills/speckit-myext-myextcmd/references/notes.md
specify preset remove my-preset            # or: specify integration switch claude --script sh
ls .github/skills/speckit-myext-myextcmd/references/notes.md

Step	main (a227e38)	This PR
preset remove my-preset	notes.md deleted with the whole directory	notes.md kept, SKILL.md removed
integration switch claude	notes.md deleted with the whole directory	notes.md kept, SKILL.md removed
then integration switch copilot	SKILL.md back, notes.md gone	SKILL.md back next to notes.md

ExtensionManager._unregister_extension_skills has the same rmtree() for extension skills, and I left it out on purpose. If extension removal kept the user's files, the directory would stay without a SKILL.md. On the next extension add, _register_extension_skills skips such a directory as user-owned (test_existing_skill_directory_without_marker_is_not_modified, from #3141), so re-adding the extension would silently drop that skill. How that should work is a design question, so this PR only covers presets, where re-adding regenerates the skill.

Testing

• Tested locally with uv run specify --help
• Ran existing tests with uv sync && uv run pytest
• Tested with a sample project (if applicable)

New test tests/specify_cli/presets/test_command_remove.py::test_preset_skill_cleanup_preserves_user_files, through the CLI with the bundled scaffold preset: preset remove and integration switch claude, each with and without a user file (references/user.md, BOM and CRLF, compared byte for byte) in the skill directory. With the user file, it must survive unchanged and SKILL.md must be gone. Without it, the whole directory must be gone, as on main. On main the two user-file cases fail and the two cases without a user file pass; all four pass with this change.

Full suite on Linux, Python 3.13: 13841 passed, 445 skipped, 1 failed (uv sync --extra test, then pytest -n 2, 7 min). The failure, test_version_execution.py::TestWindowsUvToolEnvironment::test_non_windows_uv_tool_dry_run_still_previews_installer, fails the same way on main here: Rich wraps the dry-run output, and my checkout's venv path is long enough to be split mid-path. Ruff 0.15.0 (ruff check src tests) passes. I haven't run it on Windows.

AI Disclosure

• I did not use AI assistance for this contribution
• I did use AI assistance (fill in the disclosure below)

AI disclosure: Codex CLI (model: gpt-6.1-sol, reasoning effort xhigh, autonomous, offline sandbox) found the bug while checking how Spec Kit edits files users own, reproduced it on main and wrote the fix and the test. Claude Code (model: claude-opus-5-5, autonomous) narrowed the change to presets after finding the extension re-add problem, reran the repro with the real CLI on main and the branch, ran the full suite and lint, and drafted this description. The commit carries Assisted-by: trailers for both.

[Copilot]

🟡 Changes recommended

Rollback recreates deleted skill directories with default permissions, potentially exposing restored skill files to local users previously blocked by restrictive directory permissions.

1 open issue

• Preserve skill directory permissions during rollback · New

What changed in this PR

This PR limits preset skill cleanup to generated files, preserving user content during preset removal and integration switching.

Changes:

• Delete only SKILL.md, then remove its directory if empty.
• Keep generated-file deletions journaled and update cleanup comments.
• Add four regression cases covering both cleanup operations, with and without user files.

File	Description
tests/​specify_cli/​presets/​test_command_remove.py	Tests byte-for-byte preservation of user files and removal of empty skill directories.
src/​specify_cli/​presets/​_manager_skills.py	Replaces recursive directory deletion with journaled file deletion and empty-directory cleanup.

_{Review effort: Balanced (auto)}

[kartsan03]

Addressed the Copilot finding in ccf78c7.

The empty skill directory is now removed through a new _file_changes.rmdir(), next to unlink(), at both cleanup sites (_delete_agent_preset_skills and _unregister_skills_in_dir). It journals the directory only once it is empty, so the snapshot holds just the directory itself and a rollback recreates it with its original mode. A directory that still has user files in it isn't removed, so nothing of the user's is snapshotted.

test_preset_skill_cleanup_rollback_preserves_directory_mode[remove|switch] runs preset remove and integration switch claude inside a lifecycle transaction that then fails, with the skill directory at 0700. After the rollback, SKILL.md is back byte for byte and the directory is 0700 again. With a user file in the directory, the test also checks that the directory was never snapshotted and that an edit made to the user file during the transaction is kept. The two empty-directory cases fail on the previous head and pass now. The mode check is skipped on Windows.

Full suite on Linux, Python 3.13: 13845 passed, 445 skipped, 1 failed. The failure is the same test_version_execution.py::TestWindowsUvToolEnvironment::test_non_windows_uv_tool_dry_run_still_previews_installer as before: Rich wraps the dry-run output at my long venv path, and it fails the same way on main here. Ruff 0.15.0 passes.

Fix and test by Codex CLI (model: gpt-6.1-sol, autonomous); reviewed, verified and drafted on behalf of @kartsan03 by Claude Code (model: claude-opus-5-5, autonomous).

[Copilot]

🟡 Changes recommended

A critical cleanup race allows symlink substitution to redirect deletion to an external SKILL.md file under the CLI user's permissions.

2 open issues

• Prevent symlink substitution during skill-file cleanup · New
• Preserve skill directory permissions during rollback

_{Review effort: Balanced (auto)}

[Copilot]

🔵 Needs a closer look

An unresolved symlink race in preset removal can cause the CLI to delete a SKILL.md outside the intended directory with the user's permissions.

2 open issues

• Prevent symlink substitution during skill-file cleanup
• Preserve skill directory permissions during rollback

_{Review effort: Balanced (auto)}

[mnriem]

Please address Copilot feedback

[kartsan03]

Addressed the second finding in 2e1dcf4.

Both cleanup sites now call a shared _file_changes.unlink_in_directory(skill_file, root=...). It opens the trusted root (the project root, or the skills root for global skills such as Hermes), then each directory below it with O_DIRECTORY | O_NOFOLLOW, and deletes SKILL.md relative to the skill directory's handle. The empty directory is removed relative to its parent's handle, and only if it is still the same directory. If a directory on that path has been replaced by a link or a file, nothing is deleted and the command carries on. The journal calls stay on the same paths, so rollback and last round's directory-mode fix still work. The rmdir() helper from that round is gone, since nothing uses it now.

Links above the root are left alone, so a project under a linked directory (macOS /tmp, a linked home directory) is cleaned up as before. Where dir_fd isn't available (Windows), the helper checks each directory below the root again right before each delete. That narrows the window but can't close it.

The new tests in test_command_remove.py run preset remove and integration switch claude, on both the dir_fd path and the fallback. They cover a skill directory replaced by a link to an outside SKILL.md, a skill directory that is already a link, a link loop or a file, replacements just before the unlink, after it and before the rmdir, a project under a linked parent, and global skills. The 8 replaced-by-link cases fail on the previous head, where the outside SKILL.md is deleted.

Full suite on Linux, Python 3.13: 13887 passed, 445 skipped, 1 failed, the same local-only test_non_windows_uv_tool_dry_run_still_previews_installer failure as before (it fails on main here too). Ruff 0.15.0 passes. I haven't run it on Windows, so the fallback there is tested only by forcing it on Linux.

Fix and tests by Codex CLI (model: gpt-6.1-sol, autonomous); reviewed, verified and drafted on behalf of @kartsan03 by Claude Code (model: claude-opus-5-5, autonomous).

[Copilot]

🟡 Changes recommended

The handler suppresses deletion and journal failures, allowing preset removal or switching to discard a skill’s ownership record while leaving its generated file active.

3 open issues

• Prevent symlink substitution during skill-file cleanup
• Propagate unexpected SKILL.md deletion and journal failures · New
• Preserve skill directory permissions during rollback

_{Review effort: Balanced (auto)}

[mnriem]

Please address Copilot feedback

[kartsan03]

Addressed the third finding in c9bb159.

unlink_in_directory() no longer wraps the whole body in except (OSError, ValueError). Each expected case is caught at the operation that raises it: the path not under the root, a directory below the root that is missing, a link, a loop or not a directory, SKILL.md already gone (cleanup then goes on to the empty directory), and a directory that isn't empty or was replaced at rmdir. Anything else propagates unchanged, including IntegrationInstallError from the journal and a PermissionError or other OSError from the unlink. The callers handle those as on main: preset remove stops before it drops the skill's ownership record, and integration switch keeps its existing warning.

test_preset_skill_cleanup_propagates_errors makes the unlink raise PermissionError and the journal raise IntegrationInstallError, on the dir_fd path and the fallback. The same exception reaches the caller and SKILL.md is left as it was. Those 4 cases fail on the previous head. test_preset_skill_cleanup_tolerates_expected_errors covers the skipped cases, and 2 of its cases fail on the previous head too, where an already-missing SKILL.md stopped the empty-directory cleanup.

Full suite on Linux, Python 3.13: 13899 passed, 445 skipped, 1 failed, the same local-only test_non_windows_uv_tool_dry_run_still_previews_installer failure as before. Ruff 0.15.0 passes.

Fix and tests by Codex CLI (model: gpt-6.1-sol, autonomous); reviewed, verified and drafted on behalf of @kartsan03 by Claude Code (model: claude-opus-5-5, autonomous).

[Copilot]

🟡 Changes recommended

Direct helper and manager tests are misplaced in the command suite and need to move to the integration and manager test modules required by the documented test structure.

2 open issues

• Preserve skill directory permissions during rollback
• Move helper and manager tests out of the command suite · New

2 resolved since last review

• Prevent symlink substitution during skill-file cleanup
• Propagate unexpected SKILL.md deletion and journal failures

_{Review effort: Max (auto)}

[mnriem]

Please address Copilot feedback

[kartsan03]

Addressed the fourth finding in d1f2183.

The four tests that call unlink_in_directory() directly are now in tests/specify_cli/integrations/test_file_changes.py, renamed after the helper (test_unlink_in_directory_*). test_preset_skill_cleanup_uses_global_boundary calls PresetManager methods directly, so it moved into TestPresetSkills in tests/specify_cli/presets/test_manager_skills.py. test_command_remove.py keeps the tests that go through preset remove and integration switch. Apart from the new names, the imports moved to the top of the module and a self parameter, the tests are unchanged. The three files still collect the same 130 tests.

The directory-mode thread from the first review still shows as open. It was fixed in ccf78c7, and the later rounds kept the fix. I answered in that thread.

Full suite on Linux, Python 3.13: 13899 passed, 445 skipped, 1 failed, the same local-only test_non_windows_uv_tool_dry_run_still_previews_installer failure as before. Ruff 0.15.0 passes.

Test move, checks and this comment by Claude Code (model: claude-opus-5-5, autonomous), on behalf of @kartsan03.

[Copilot]

🟡 Changes recommended

The directory identity check occurs before journaling, allowing a replacement directory introduced during before_file_change() to be deleted by rmdir without detection.

2 open issues

• Recheck skill directory identity after journaling before rmdir · New
• Preserve skill directory permissions during rollback

1 resolved since last review

• Move helper and manager tests out of the command suite

_{Review effort: Balanced (auto)}

[mnriem]

Please address Copilot feedback

[Copilot]

🔵 Needs a closer look

Human assessment is needed for directory-replacement and trusted-root edge cases that can cause cleanup or rollback to modify unintended files.

No open issues 🎉

2 resolved since last review

• Recheck skill directory identity after journaling before rmdir
• Preserve skill directory permissions during rollback

_{Review effort: Balanced (auto)}

[Copilot]

🟡 Changes recommended

Fallback cleanup can delete a replacement directory’s SKILL.md, while retained markerless directories can silently block later extension skill registration.

3 open issues

• Verify directory identity before unlinking the fallback skill · New
• Report extension skills blocked by markerless directories · New
• Move integration-switch cases to the integration test suite · New

_{Review effort: Max (auto)}

[mnriem]

Please address Copilot feedback. If not applicable, please explain why

[kartsan03]

Addressed the latest Copilot pass. c87c111 moves the integration switch cleanup cases into the switch suite. The other two findings aren't changed, and the reasons are in their threads: the fallback race can't be closed with path checks on Windows, and the skipped extension skill follows the existing #3141 rule.

Drafted on behalf of @kartsan03 by Claude Code (model: claude-opus-5-5, autonomous).

[Copilot]

🔵 Needs a closer look

Human assessment is needed to evaluate a subtle symlink-replacement risk in home-scoped cleanup that could delete an unrelated SKILL.md even on the descriptor-based path.

No open issues 🎉

3 resolved since last review

• Verify directory identity before unlinking the fallback skill
• Report extension skills blocked by markerless directories
• Move integration-switch cases to the integration test suite

_{Review effort: Balanced (auto)}