You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(presets): recompose surviving skills when a lower preset layer is removed - #5013
Removing a preset that is the base, or a middle layer, of another preset's prepend/append/wrap composition leaves the removed preset's content in the active agent's SKILL.md. That affects Claude, Codex, Copilot with --skills, and other skills-based integrations. specify preset resolve reports the new chain, and command-file agents such as Gemini are recomposed onto it. But the skill keeps the removed preset's instructions, so /speckit-specify runs content the user just removed.
The same steps with --integration gemini drop ALPHA BODY from .gemini/commands/speckit.specify.toml on main, so only skills output is stale.
Root cause, in presets/_manager_skills.py:
On removal, _reconcile_skills() only revisits the active skills directory when _unregister_skills() restored something there (if extra_skills_dirs is None or active_provenance).
The composed skill is owned by the surviving composing preset (beta), not the removed one, so nothing is restored and the active directory is skipped.
The recomposed .composed/<cmd>.md that _reconcile_composed_commands() writes is therefore never rendered into the skill.
_reconcile_skills() now records commands whose winning layer composes (strategy other than replace).
After a removal, it also re-renders those skills in the active directory, but only when the SKILL.md still carries the winning preset's metadata.source: preset:<id> marker. A new helper, _skills_owned_by_preset(), does that check.
A hand-edited skill is left alone, matching the existing protection for non-owned skills during removal.
The install and rescaffold paths are unchanged.
Testing
Tested locally with uv run specify --help
Ran existing tests with uv sync && uv run pytest
Tested with a sample project (if applicable): the reproduction above with claude (on main and this branch) and gemini (on main)
New tests in tests/specify_cli/presets/test_manager_skills.py:
test_remove_base_preset_recomposes_surviving_composed_skill: fails on main with assert "Base preset body" not in content
test_remove_middle_preset_drops_its_layer_from_composed_skill: fails on main with assert "Middle body" not in content
test_remove_base_preset_preserves_hand_edited_composed_skill: the negative case, guarding the hand-edit protection; passes before and after
With the change the file gives 70 passed.
Preset, integration, bundle and extension suites: 6353 passed, 9 skipped.
Full suite (macOS, Python 3.13): 12332 passed, 376 skipped. 11 failures in the tests/test_*_python_parity.py files occur identically on unmodified main on this machine under full-suite load; those files pass when run on their own (257 passed, 112 skipped).
One more local failure, test_resolver.py::TestResolveCore::test_resolve_core_does_not_return_preset_files, was an artifact of a worktree directory literally named presets: the test asserts "presets" not in path.parts. It fails identically on main in that directory and passes elsewhere.
uvx ruff@0.15.0 check src tests: clean
AI Disclosure
I did not use AI assistance for this contribution
I did use AI assistance (fill in the disclosure below)
AI disclosure: Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode) opened this PR on behalf of @yxshee. A Claude Code subagent (same model, autonomous) found the bug and wrote the fix and tests. The lead agent independently re-ran the new tests against main and the branch, reproduced the behavior with the real CLI (Claude and Gemini), confirmed the #3459 origin with git log -S, reviewed the diff, and wrote this description. No human reviewed the change line by line before it was opened.
… removed
Removing a preset that is the base (or a middle layer) of another preset's
prepend/append/wrap composition left the removed preset's content in the
active agent's SKILL.md (Claude, Codex, Copilot with --skills, ...), while
command-file agents such as Gemini were recomposed onto the new stack.
The composed skill is owned by the surviving preset at the top of the
chain, so _unregister_skills() restores nothing for the removed preset,
and _reconcile_skills() only revisited the active skills directory when
that restore had touched it. The recomposed .composed/<cmd>.md that
_reconcile_composed_commands() writes was therefore never rendered into
the skill, and `specify preset resolve` disagreed with what the agent ran.
After a removal, also reconcile active-directory skills for commands whose
winning layer composes, as long as the skill still carries the winning
preset's source marker. A hand-edited skill is left alone, matching the
existing protection for non-owned skills during removal.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When both the modern (speckit-specify) and legacy (speckit.specify)
skill directories exist, the active-directory recomposition added for
lower-layer removal selected the command because the modern skill kept
the winning preset's marker, and _register_skills() then rewrote both
directories, overwriting a user-owned legacy skill.
Give _register_skills() an optional only_skill_names filter and pass the
directory's managed names on removal, so only names that are still
preset-owned are written. Install and rescaffold calls pass None and
keep writing every existing directory, as before.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addressed the Copilot finding that removal recomposition could overwrite a user-owned legacy skill (commit 8c0328f):
_register_skills() gains an optional only_skill_names filter; removal passes the directory's managed names for the active directory. Install and rescaffold behavior is unchanged.
New regression test_remove_base_preset_preserves_user_owned_legacy_skill: fails on the previous head (1239cbe), passes now.
AI disclosure: written and pushed by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the code, test and this comment are AI-generated.
When the removed preset was the only replace base of a composing
command, with no core or extension fallback, resolution returns None.
Command-file agents unregister that command, but the survivor's stale
.composed/<cmd>.md stayed behind, and the removal recomposition rendered
it back into the skill, so the removed preset's instructions stayed
active.
Track composing winners whose stack no longer resolves, delete their
stale composed cache, and on removal delete the active skill only while
it still carries the winning preset's source marker. Hand-edited skills
are kept, matching the existing ownership protection.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addressed the Copilot finding about removing a command's only base with no core or extension fallback (commit 57bdfc9):
The stale .composed/<cmd>.md of a composing winner whose stack no longer resolves is deleted, so it can't be rendered back into a skill.
On removal, the active skill for that command is deleted only while it still carries the winner's source marker; hand-edited skills are kept.
New tests: test_remove_sole_base_preset_retires_stale_composed_skill (fails on the previous head, passes now) and test_remove_sole_base_preset_keeps_hand_edited_skill.
AI disclosure: posted on behalf of @yxshee by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the fix, tests and this reply are AI-generated.
Handle aliases during preset retirement and recomposition
src/specify_cli/presets/_manager_skills.py:465
This loop checks only the primary command's modern and legacy names, not its manifest aliases. On Codex, an appending preset can declare speckit.alt as an alias of speckit.custom; removing its only base retires speckit-custom but leaves speckit-alt/SKILL.md runnable with the removed instructions. agents.py:934–1015 renders aliases separately, using <id>:<file> source markers, so the preset:<id> check also misses their ownership. Include the winning template's aliases and their actual ownership markers in recomposition and retirement, preserve user-owned files, and add a native-skill alias regression.
AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated this feedback for the requesting reviewer.
Round-3 review of the no-base removal fix:
- Delete a cache only when no path component below the project root is a
symlink, so a symlinked .composed directory can't redirect the
deletion to a file outside the project.
- Drop every surviving preset's .composed/<cmd>.md for an uncomposable
command, not only the winner's: a middle preset's stale cache could be
rendered back into the skill by a later rescaffold.
- Delete through the journal-aware unlink helper, as the cache writes
already do, so a failing integration lifecycle can restore the cache.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addressed the three Copilot findings on stale composed-cache deletion (commit c184e1a):
Deletion skips any cache reached through a symlinked path component below the project root.
All surviving presets' caches for an uncomposable command are dropped, so a rescaffold can't render the removed base back.
Deletion uses the journal-aware unlink helper, like the cache writes.
Three new tests, each failing on the previous head. Preset, integration, extension and bundle suites: 6101 passed, 9 skipped. uvx ruff@0.15.0 check src tests: clean.
AI disclosure: posted on behalf of @yxshee by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the fix, tests and this reply are AI-generated.
presets_by_priority excludes disabled presets (_registry.py:227), so their stale caches survive this cleanup. With a replace base beneath two append presets, disable the middle preset and then remove the sole base: the middle cache still contains the removed instructions. Re-enabling it and rescaffolding can render that cache before reconciliation deletes it; rescaffold reconciliation does not retire the newly written skill. Invalidate caches for all installed presets, including disabled ones, and extend the three-layer regression to cover disabling and re-enabling the middle layer.
AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated and verified this feedback for the requesting reviewer.
Preserve output when surviving layers cannot be read
src/specify_cli/presets/_manager_skills.py:211
resolve_content() also returns None when a surviving layer cannot be read or decoded (_resolver.py:780–783,865–871). Treating every None as a missing base deletes the last valid cache and owned skill even though a replace base still exists. A dependency-free execution with a non-UTF-8 base confirmed both deletions. Only retire output when the stack has no replace layer; preserve existing output on read failures, as test_unregister_skills_in_dir_unreadable_core_template_skips already requires for restoration. Add a composed-stack regression for this case.
AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated and verified this feedback for the requesting reviewer.
Journal skill-directory removals during force replacement
src/specify_cli/presets/_manager_skills.py:434
This rmtree() bypasses the lifecycle file-change journal. Force replacement calls remove() from _manager.py:408; when that runs under an active journal, a later failure cannot restore this deleted skill, although cache deletion is observed. design/integration.md, “Storage, loading, and lifecycle,” requires preset replacements and generated artifacts to participate in the journal. Wrap this removal like the other skill deletions at lines 1377–1381 and 1644–1648, and extend the observer regression to check the skill-directory removal too.
AI disclosure: GitHub Copilot (model: unknown, autonomous mode) generated and verified this feedback for the requesting reviewer.
…lures
Round-4 review of the no-base removal fix:
- Skip registry keys (and command names) that are not a single plain
path segment, so a malformed key such as "../../../outside" cannot
steer cache cleanup outside the project.
- Invalidate stale caches for every installed preset, disabled ones
included: re-enabling a disabled composer and rescaffolding could
otherwise render its stale cache back.
- Retire output only when the stack has no replace layer left.
resolve_content() also returns None when a surviving layer can't be
read; that keeps the existing cache and skill instead of deleting them.
- Journal the stale skill-directory removal like the other skill
deletions, so a failing lifecycle can restore it.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addressed the new finding and the three previously missed ones (commit b9a2c35):
Traversing registry keys: cleanup accepts only plain single-segment preset ids and command names.
Disabled presets: stale caches are invalidated for every installed preset, so re-enabling and rescaffolding can't render the removed base back.
Read failures: output is retired only when no replace layer remains; an unreadable surviving base keeps the existing cache and skill.
Journal: the stale skill-directory removal is wrapped in before_file_change/after_file_change like the other skill deletions.
Regressions: test_stale_cache_cleanup_ignores_traversing_registry_key, test_remove_sole_base_drops_disabled_preset_cache, test_unreadable_surviving_base_keeps_composed_output, and the journal test now also checks the skill directory; each fails on the previous head.
AI disclosure: posted on behalf of @yxshee by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the fix, tests and this reply are AI-generated.
Limit historical-directory reconciliation to managed skill names
src/specify_cli/presets/_manager_skills.py:395
The new name filter protects only the active directory. The historical-directory branch below still calls _register_skills() without only_skill_names, so it writes every existing modern/legacy sibling. If removal restored only speckit-specify in an inactive agent's directory and preserved a user-owned speckit.specify beside it, reconciliation overwrites that preserved file with the surviving preset's content. Pass managed_names to the targeted call too, and add a regression with mixed ownership in a non-active agent directory.
AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this feedback for the requesting reviewer.
Retire stale skills during activation and rescaffolding
src/specify_cli/presets/_manager_skills.py:453
This retirement only runs during removal, but activation and rescaffolding call _reconcile_skills() without extra_skills_dirs. After removing a custom command's only base while Codex is active, a previously written Claude skill can retain the removed body when Claude is reactivated: registration has no base/cache, and this deletion is skipped. The same problem occurs when the sole composer was disabled during removal; re-enabling it lets registration render its stale cache before reconciliation deletes only the cache. Move ownership-checked retirement outside the removal-only guard, and add regressions for both flows.
AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this feedback for the requesting reviewer.
Handle native-skill aliases during recomposition and retirement
src/specify_cli/presets/_manager_skills.py:520
Native-skill aliases still retain removed instructions. CommandRegistrar.register_commands() renders a separate skill for each manifest alias (agents.py:934–1015), but this ownership scan and _register_skills() consider only the primary command's modern/legacy names. For an appending preset with alias speckit.shortcut, removing its base refreshes the primary skill but leaves speckit-shortcut/SKILL.md stale. Include aliases in recomposition and retirement, and recognize their registrar-generated source markers (<preset-id>:<source-file>), which differ from the primary skill's preset:<id> marker. Add a native-skill alias regression.
AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this feedback for the requesting reviewer.
An alias SKILL.md hard-linked to a file outside the project passes the
ownership checks, and the registrar then wrote the recomposed text
through the shared inode. Write a sibling temp file and rename it over
the destination instead, for the direct write, the dev-mode cache file
and the copy fallback; the file-change journaling is unchanged.
The write-error tests that simulated a failing artifact write by
patching Path.write_text now also patch the registrar helper, so the
simulated failure still lands on the artifact write.
Addressed Copilot's new finding (discussion_r4482984323) in 50c62a0.
Hard-linked alias skill overwritten on removal.CommandRegistrar._write_registered_output wrote the rendered artifact in place with Path.write_text, so an alias SKILL.md hard-linked to a file outside the project carried the recomposed text to that file too. The registrar now writes a sibling temp file and renames it over the destination (_replace_file_text), so only the directory entry changes and the external file keeps its bytes. This covers the direct write, the dev-mode cache file and the copy fallback; the before_file_change/after_file_change journaling around each write is unchanged.
Regression: test_alias_refresh_does_not_write_through_hardlinked_skill hard-links the alias SKILL.md to a file outside the project, removes the base preset, and asserts the outside file still holds the old text while the alias was recomposed and has a single link. It fails on cd5296a (the outside file reads "Core custom body") and passes here.
Three existing write-error tests (test_generic_refresh_write_error_restores_artifacts_and_registry, test_generic_registration_write_error_rolls_back_partial_install, test_dev_register_commands_falls_back_to_copy_when_cache_write_fails) simulated a failing artifact write by patching Path.write_text. They now also patch the registrar helper, so the simulated failure still lands on the artifact write; the generic skills-mode path keeps writing through Path.write_text, so that patch stays.
On discussion_r4471904035 (survivor-owned alias deleted when both layers declare it): this was addressed in cd5296a with test_remove_base_keeps_survivor_alias_both_layers_declare; nothing changed there this round. Flagging it since the review summary still lists the thread as open.
pytest tests (full suite): 12,363 passed, 376 skipped. The 11 failures are the bash composed-template parity tests (test_resolve_template_python_parity.py and friends), which fail identically on cd5296a in this checkout when run as part of the full suite and pass in isolation, so they are an environment issue here rather than this change. test_resolve_core_does_not_return_preset_files is deselected because this worktree lives under a presets/ directory, which the test reads as a preset path
uvx ruff@0.15.0 check src tests: clean
Merges cleanly with current main
AI disclosure: Claude Code (Claude Mythos 5.1, autonomous) wrote this fix, the test changes and this summary, and ran the checks above. yxshee reviews the PR.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Unconditional chmod(0o644) overrides existing private permissions and restrictive creation umasks, potentially exposing command artifacts to other local users when parent directories are traversable.
The atomic replace in _replace_file_text set every temp file to 0644
before renaming it over the destination, so a private (0600) command
or skill output became world-readable on refresh and new outputs
ignored a restrictive umask. Keep an existing regular file's mode, and
give a new file 0666 under the process umask, as an in-place write
would.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Atomic replace resets output permissions (discussion_r4484399289): CommandRegistrar._replace_file_text no longer forces the temp file to 0644. It keeps an existing regular file's permission bits, and a new output gets 0666 under the process umask, which is what the earlier in-place write left. The replace stays atomic, so the hard-link protection from 50c62a0 is unchanged.
New tests, both failing on 50c62a0 (the output ended up 0644) and passing with the fix:
test_alias_refresh_preserves_existing_output_mode: a 0600 alias stays 0600 after removing the base recomposes it
test_new_alias_output_honors_umask: under umask 077 a new alias is created 0600
AI disclosure: Claude Code (Claude Opus 5.5) worked autonomously on this round. It wrote the fix, the tests and this summary, and ran the checks above. yxshee reviews the PR.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Temporarily setting the process-wide umask to 000 can race with overlapping registrations, leaving the mask at 000 and exposing outputs or later unrelated files through overly permissive access.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Unresolved composition and rendering bugs can lose or incorrectly rewrite skill content, while atomic-write regressions break long filenames and weaken file-permission protections.
… umask
Address four review findings:
- _replace_file_text no longer reads the umask by setting it to 0. It
creates the temp file with mode 0666 so the kernel applies the
umask, and restores an existing file's own bits.
- The temp name is a fixed short .speckit-<hex>.tmp, so a destination
name near the filesystem limit stays writable.
- The .composed cache refresh goes through the path guards, journaling
and _replace_file_text, so an existing cache keeps its mode.
- An alias belongs to the highest layer that declares it, not to the
primary command's winning layer, so a lower declarer recomposes it
on rescaffold and restores it when a new base arrives.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Replacing a private artifact can expose its contents to other local users because the temporary file is written with overly permissive permissions before the original mode is restored.
Addressed the four findings from the latest Copilot reviews in 586db13.
Process umask changed to read it (discussion_r4494830290): _replace_file_text no longer calls os.umask. It creates the temp file with os.open(..., O_CREAT | O_EXCL, 0o666), so the kernel applies the current umask to a new output, and it chmods to the destination's own bits only when replacing an existing regular file.
Long filenames fail with ENAMETOOLONG (discussion_r4495630946): the temp entry is now .speckit-<16 hex>.tmp, a fixed 29 bytes, instead of the destination name plus a suffix. Any name that is valid for the destination stays writable.
Composed cache reset to 0644 (discussion_r4495631107): the .composed cache write in _reconcile_preset_aliases now goes through the same path guards, lifecycle journaling and _replace_file_text, so an existing cache keeps its mode. The SKILL.md writes in _register_skills that the comment also names use shared_infra._write_shared_text, which writes with a fixed 0644 on main too. That predates this PR and applies to every shared-infra write, so I left it out of scope here and can raise it as a separate issue if you want.
Alias precedence taken from the primary's winner (discussion_r4495631046): an alias now belongs to the highest layer that declares it (_layer_declares_alias), not to whichever layer wins the primary command. When a higher preset doesn't declare the alias, the lower declarer renders it from its own layer down, both on a rescaffold and when a new base makes it composable again. A higher declarer still shadows it as before.
New tests, each failing on f956449 and passing with the fix:
Tests and lint: the only red check on f956449 was pytest (macos-latest, 3.13). It failed in checkout (Could not resolve host: github.com), and the other pytest jobs were cancelled by fail-fast, so no test or lint failure came from the change. ruff was green there.
AI disclosure: Claude Code worked autonomously on this round. It wrote the fixes, the tests and this summary, and ran the checks above. yxshee reviews the PR.
Replacing a 0600 file under a 022 umask created a 0644 temp file and
wrote the new content before the final chmod restored the private mode.
Create the temp file with the destination's own bits and set them
before writing, so the content is never readable more widely than the
destination.
Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Temp file briefly wider than a private destination (discussion_r4502724300): _replace_file_text now creates the temp file with the destination's own permission bits, not 0666, and sets them with chmod before any content is written. Under a 022 umask, replacing a 0600 file no longer writes the new text into a 0644 temp file first. New outputs still get 0666 under the process umask.
New test: test_replace_writes_private_content_only_into_private_temp records the temp file's mode at the moment it is opened for writing. On 586db13 it is 0644, and with the fix it is 0600.
AI disclosure: Claude Code (Claude Opus 5.5) worked autonomously on this round. It wrote the fix, the test and this summary, and ran the checks above. yxshee reviews the PR.
_replace_file_text restored the destination's permission bits on the
temp file before wrapping the descriptor, so a failing os.chmod left
the descriptor open; only the temp entry was removed. Registration
catches write errors and continues, so the leak accumulated. Enter the
file context first and restore the mode inside it, still before any
content is written.
Assisted-by: Claude Code (model: Claude Mythos 5.1, autonomous)
Descriptor leaked when chmod fails (discussion_r4506905678): _replace_file_text now enters the os.fdopen context first and restores the destination's permission bits inside it, before any content is written. If os.chmod raises, the context closes the descriptor and the cleanup still removes the temp file, so a registration pass that catches the error and continues no longer accumulates open descriptors. The ordering guarantee from 53a373e is unchanged: the mode is still set before the first write.
New test: test_replace_closes_descriptor_when_chmod_fails makes os.chmod raise, then checks the temp descriptor is closed, the destination is untouched and no temp file is left behind. On 53a373e the descriptor is still open after the error; with the fix it is closed.
uv run pytest tests/specify_cli tests/test_presets.py tests/test_extensions.py tests/integrations tests/integration tests/extensions: 8852 passed, 143 skipped, and test_resolve_core_does_not_return_preset_files failing only because this worktree lives in a directory named presets (it asserts no presets segment in the bundled template's absolute path); it passes from a checkout outside such a path, as do the five _replace_file_text tests
uvx ruff@0.15.0 check src tests: clean
Merges cleanly with current main
AI disclosure: Claude Code (Claude Mythos 5.1) worked autonomously on this round. It wrote the fix, the test and this summary, and ran the checks above. yxshee reviews the PR.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed to evaluate permission and ACL preservation risks that could expose previously restricted instructions to other local users during refreshes.
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
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.
mnriem
added
the
triage-must-have
Description
Removing a preset that is the base, or a middle layer, of another preset's
prepend/append/wrapcomposition leaves the removed preset's content in the active agent'sSKILL.md. That affects Claude, Codex, Copilot with--skills, and other skills-based integrations.specify preset resolvereports the new chain, and command-file agents such as Gemini are recomposed onto it. But the skill keeps the removed preset's instructions, so/speckit-specifyruns content the user just removed.Reproduction (
alphareplacesspeckit.specifywithALPHA BODY;betaappendsBETA APPENDED):The same steps with
--integration geminidropALPHA BODYfrom.gemini/commands/speckit.specify.tomlonmain, so only skills output is stale.Root cause, in
presets/_manager_skills.py:_reconcile_skills()only revisits the active skills directory when_unregister_skills()restored something there (if extra_skills_dirs is None or active_provenance).beta), not the removed one, so nothing is restored and the active directory is skipped..composed/<cmd>.mdthat_reconcile_composed_commands()writes is therefore never rendered into the skill.683bfd0) and moved in the refactor(presets): split domain internals into private modules #4747 refactor. Before fix: register extensions for the active integration only #3459 the active directory was always reconciled.The fix:
_reconcile_skills()now records commands whose winning layer composes (strategy other thanreplace).SKILL.mdstill carries the winning preset'smetadata.source: preset:<id>marker. A new helper,_skills_owned_by_preset(), does that check.Testing
uv run specify --helpuv sync && uv run pytestclaude(onmainand this branch) andgemini(onmain)New tests in
tests/specify_cli/presets/test_manager_skills.py:test_remove_base_preset_recomposes_surviving_composed_skill: fails onmainwithassert "Base preset body" not in contenttest_remove_middle_preset_drops_its_layer_from_composed_skill: fails onmainwithassert "Middle body" not in contenttest_remove_base_preset_preserves_hand_edited_composed_skill: the negative case, guarding the hand-edit protection; passes before and afterWith the change the file gives 70 passed.
tests/test_*_python_parity.pyfiles occur identically on unmodifiedmainon this machine under full-suite load; those files pass when run on their own (257 passed, 112 skipped).test_resolver.py::TestResolveCore::test_resolve_core_does_not_return_preset_files, was an artifact of a worktree directory literally namedpresets: the test asserts"presets" not in path.parts. It fails identically onmainin that directory and passes elsewhere.uvx ruff@0.15.0 check src tests: cleanAI Disclosure
AI disclosure: Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode) opened this PR on behalf of @yxshee. A Claude Code subagent (same model, autonomous) found the bug and wrote the fix and tests. The lead agent independently re-ran the new tests against
mainand the branch, reproduced the behavior with the real CLI (Claude and Gemini), confirmed the #3459 origin withgit log -S, reviewed the diff, and wrote this description. No human reviewed the change line by line before it was opened.🤖 Generated with Claude Code