Skip to content

feat: implement preset wrap strategy#2189

Open
kennedy-whytech wants to merge 5 commits intogithub:mainfrom
kennedy-whytech:kennedy-whytech-feat-preset-wrap-strategy
Open

feat: implement preset wrap strategy#2189
kennedy-whytech wants to merge 5 commits intogithub:mainfrom
kennedy-whytech:kennedy-whytech-feat-preset-wrap-strategy

Conversation

@kennedy-whytech
Copy link
Copy Markdown

@kennedy-whytech kennedy-whytech commented Apr 12, 2026

Description

  • Implements strategy: wrap for preset command/skill overrides — presets can now inject content before and after core commands using a {CORE_TEMPLATE} placeholder without fully replacing them
  • Wired into both _register_skills (Claude skills path) and register_commands (all other agents)

Fixes in this branch

{SCRIPT} placeholders broken in wrapped commands — when a preset used strategy: wrap, the core template's scripts/agent_scripts frontmatter was silently discarded after body substitution. Presets that wrap an existing
command (e.g. "use Codex for the following: {CORE_TEMPLATE}") now inherit the core's script metadata so prerequisite check scripts still run.

Extension commands never resolved — _substitute_core_template only searched core-pack and project template directories. Wrapping extension commands like speckit.git.feature now also searches
.specify/extensions//commands/, so {CORE_TEMPLATE} resolves correctly for extension overrides.

Two test hygiene fixes (from code review):

  • Restored missing "reinstall" assertion in the bundled preset CLI error test
  • Restored AGENT_CONFIGS on the class (not the instance) after test mutation, preventing state leaking across tests

Test plan

  • uv run python -m pytest tests/test_presets.py::TestWrapStrategy — all wrap strategy tests pass
  • Full suite: uv run python -m pytest — no new failures

End-to-end test with my own presets. The wrapping seems doing good. Also, I feel like "wrap" can cover prepend and append already.
Screenshot 2026-04-13 at 12 10 42 PM

Testing

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

AI Disclosure

  • I did not use AI assistance for this contribution

  • I did use AI assistance (describe below)

    AI assistance disclosure

    This PR was implemented with AI assistance (Claude Code) for code generation,
    test writing, and commit messages. All changes were reviewed and verified
    manually.

@kennedy-whytech kennedy-whytech requested a review from mnriem as a code owner April 12, 2026 14:35
@kennedy-whytech kennedy-whytech force-pushed the kennedy-whytech-feat-preset-wrap-strategy branch from 604b585 to 444daef Compare April 12, 2026 15:47
@mnriem mnriem requested a review from Copilot April 13, 2026 12:28
@mnriem mnriem self-assigned this Apr 13, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for strategy: wrap on preset-provided commands, enabling preset authors to inject pre/post content around an existing “core” command body via a {CORE_TEMPLATE} placeholder during command/skill registration.

Changes:

  • Introduces _substitute_core_template() and applies it during preset skill registration (_register_skills) and generic agent command registration (CommandRegistrar.register_commands).
  • Extends the self-test preset with a new speckit.wrap-test command and updates docs to mark wrap as implemented for commands.
  • Adds unit + E2E-style tests covering placeholder substitution and install behavior.
Show a summary per file
File Description
tests/test_presets.py Updates self-test manifest expectations; adds new TestWrapStrategy coverage.
src/specify_cli/presets.py Adds _substitute_core_template() and wires it into preset skill generation.
src/specify_cli/agents.py Wires wrap substitution into agent command registration.
presets/self-test/preset.yml Adds a new wrap-strategy command entry to the self-test preset.
presets/self-test/commands/speckit.wrap-test.md Adds a wrap command template using {CORE_TEMPLATE}.
presets/README.md Updates roadmap/docs to reflect wrap implemented for commands/artifacts.

Copilot's findings

Tip

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

  • Files reviewed: 6/6 changed files
  • Comments generated: 1

Copy link
Copy Markdown
Collaborator

@mnriem mnriem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kennedy-whytech kennedy-whytech force-pushed the kennedy-whytech-feat-preset-wrap-strategy branch from 444daef to 53220f8 Compare April 13, 2026 15:55
@kennedy-whytech
Copy link
Copy Markdown
Author

kennedy-whytech commented Apr 13, 2026

@mnriem that makes sense to me. updated.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements support for strategy: wrap on preset command templates by substituting {CORE_TEMPLATE} with the installed core command template body during installation/registration, enabling presets to extend core commands without copy/paste.

Changes:

  • Added _substitute_core_template() helper and applied it when generating skills from preset commands (Claude skills flow).
  • Applied the same substitution in CommandRegistrar.register_commands() for non-skill agent command installation.
  • Added a self-test preset wrap command + unit/E2E tests, and updated preset strategy documentation.
Show a summary per file
File Description
src/specify_cli/presets.py Adds {CORE_TEMPLATE} substitution helper and applies it in the skills generation path for wrap-strategy preset commands.
src/specify_cli/agents.py Applies {CORE_TEMPLATE} substitution during normal agent command registration when strategy: wrap is set.
tests/test_presets.py Updates self-test manifest expectations and adds unit + E2E coverage for wrap substitution.
presets/self-test/preset.yml Registers a new wrap-strategy command in the bundled self-test preset.
presets/self-test/commands/speckit.wrap-test.md New wrap command template containing {CORE_TEMPLATE} placeholder.
presets/README.md Updates roadmap/docs to reflect wrap being implemented for commands/artifacts (not scripts).

Copilot's findings

Tip

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

  • Files reviewed: 6/6 changed files
  • Comments generated: 2

Copy link
Copy Markdown
Collaborator

@mnriem mnriem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new preset composition option (strategy: wrap) for command/skill overrides, allowing presets to inject content around the existing core command by substituting a {CORE_TEMPLATE} placeholder, rather than fully replacing the command.

Changes:

  • Implement _substitute_core_template helper to resolve and substitute {CORE_TEMPLATE} from project core templates, bundled core pack, and installed extensions.
  • Wire wrap substitution into both preset skill generation (_register_skills) and agent command registration (register_commands).
  • Extend self-test preset + add comprehensive tests for wrap behavior, extension resolution, and script placeholder hygiene.
Show a summary per file
File Description
src/specify_cli/presets.py Introduces {CORE_TEMPLATE} substitution + merges core scripts/agent_scripts into wrapped skill frontmatter.
src/specify_cli/agents.py Enables wrap substitution during agent command registration.
tests/test_presets.py Adds TestWrapStrategy coverage and updates self-test manifest expectations.
presets/self-test/preset.yml Adds a self-test wrap command entry.
presets/self-test/commands/speckit.wrap-test.md New wrap-strategy command template used for end-to-end testing.
presets/README.md Updates documentation to reflect wrap is implemented for commands/artifacts (scripts TBD).

Copilot's findings

Tip

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

  • Files reviewed: 6/6 changed files
  • Comments generated: 1

Comment on lines +419 to +420
body, _ = _substitute_core_template(body, cmd_name, project_root, self)

Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When strategy: wrap is used here, _substitute_core_template returns the wrapped core template’s frontmatter as the second tuple element, but it’s discarded. For SKILL-target agents that call resolve_skill_placeholders (e.g. codex/kimi), any {SCRIPT}/{AGENT_SCRIPT} placeholders coming from the substituted core body will remain unresolved unless scripts/agent_scripts are inherited into frontmatter (similar to what _register_skills now does). Consider merging scripts and agent_scripts from the core frontmatter when the preset frontmatter doesn’t define them, before _adjust_script_paths runs so paths get normalized too.

Suggested change
body, _ = _substitute_core_template(body, cmd_name, project_root, self)
body, core_frontmatter = _substitute_core_template(
body, cmd_name, project_root, self
)
for key in ("scripts", "agent_scripts"):
if key not in frontmatter and key in core_frontmatter:
frontmatter[key] = deepcopy(core_frontmatter[key])

Copilot uses AI. Check for mistakes.
@mnriem mnriem self-requested a review April 13, 2026 17:18
Copy link
Copy Markdown
Collaborator

@mnriem mnriem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

…: wrap

_substitute_core_template now returns a (body, core_frontmatter) tuple so
callers can merge scripts and agent_scripts from the core template into the
preset frontmatter when the preset does not define them. Both _register_skills
and register_commands perform this merge before _adjust_script_paths runs so
inherited paths are normalised. The TOML render path also calls
resolve_skill_placeholders so {SCRIPT}/{AGENT_SCRIPT} placeholders are
expanded for Gemini/Tabnine agents.
@kennedy-whytech kennedy-whytech force-pushed the kennedy-whytech-feat-preset-wrap-strategy branch from 4fe7f81 to 49c83d2 Compare April 14, 2026 02:51
@mnriem mnriem requested a review from Copilot April 14, 2026 11:45
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for a new preset composition mode (strategy: wrap) so preset command/skill overrides can inject content around an existing command body via a {CORE_TEMPLATE} placeholder, instead of fully replacing it.

Changes:

  • Introduces _substitute_core_template() and wires it into preset skill generation and agent command registration for strategy: wrap.
  • Extends command registration to merge missing scripts / agent_scripts from the wrapped “core” template and resolves placeholders for TOML command output.
  • Adds wrap-strategy tests plus a self-test preset command and updates preset documentation.
Show a summary per file
File Description
src/specify_cli/presets.py Adds core-template substitution helper and applies wrap behavior when generating SKILL.md overrides.
src/specify_cli/agents.py Applies wrap substitution and frontmatter inheritance during command registration; resolves placeholders for TOML output.
tests/test_presets.py Adds wrap-strategy test suite and adjusts self-test preset expectations; modifies bundled preset CLI error assertions.
presets/self-test/preset.yml Registers a new self-test command entry for wrap strategy coverage.
presets/self-test/commands/speckit.wrap-test.md Adds a wrap-strategy self-test command file using {CORE_TEMPLATE}.
presets/README.md Updates documentation to reflect that command/template wrapping is implemented (scripts wrap not yet).

Copilot's findings

Tip

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

Comments suppressed due to low confidence (2)

tests/test_presets.py:3299

  • Same AGENT_CONFIGS leakage issue here: the test mutates registrar.AGENT_CONFIGS[...] (class-level) and then restores with registrar.AGENT_CONFIGS = original (instance-level assignment). This can leave test-agent in the class-level configs for subsequent tests. Restore on CommandRegistrar.AGENT_CONFIGS (class) or patch the dict in a context manager.
        registrar = CommandRegistrar()
        original = copy.deepcopy(registrar.AGENT_CONFIGS)
        registrar.AGENT_CONFIGS["test-agent"] = {
            "dir": str(agent_dir.relative_to(project_dir)),
            "format": "markdown",
            "args": "$ARGUMENTS",
            "extension": ".md",
            "strip_frontmatter_keys": [],
        }
        try:
            registrar.register_commands(
                "test-agent",
                [{"name": "speckit.specify", "file": "commands/speckit.specify.md"}],
                "test-preset",
                project_dir / "preset",
                project_dir,
            )
        finally:
            registrar.AGENT_CONFIGS = original

tests/test_presets.py:3347

  • Same AGENT_CONFIGS leakage issue in the TOML agent test: registrar.AGENT_CONFIGS is mutated (class-level) but restored via instance assignment (registrar.AGENT_CONFIGS = original). This can leak test-toml-agent into other tests. Restore via CommandRegistrar.AGENT_CONFIGS = original or patch the dict with patch.dict/monkeypatch.
        registrar = CommandRegistrar()
        original = copy.deepcopy(registrar.AGENT_CONFIGS)
        registrar.AGENT_CONFIGS["test-toml-agent"] = {
            "dir": str(toml_dir.relative_to(project_dir)),
            "format": "toml",
            "args": "{{args}}",
            "extension": ".toml",
            "strip_frontmatter_keys": [],
        }
        try:
            registrar.register_commands(
                "test-toml-agent",
                [{"name": "speckit.specify", "file": "commands/speckit.specify.md"}],
                "test-preset",
                project_dir / "preset",
                project_dir,
            )
        finally:
            registrar.AGENT_CONFIGS = original

  • Files reviewed: 6/6 changed files
  • Comments generated: 4

Comment on lines +54 to +59
core_file = project_root / ".specify" / "templates" / "commands" / f"{short_name}.md"
if not core_file.exists():
return body, {}

core_frontmatter, core_body = registrar.parse_frontmatter(core_file.read_text(encoding="utf-8"))
return body.replace("{CORE_TEMPLATE}", core_body), core_frontmatter
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_substitute_core_template currently only looks for the core file at .specify/templates/commands/{short_name}.md. This won’t resolve wrapped extension commands (e.g. speckit.git.feature) because their “core” source lives under .specify/extensions/<ext-id>/commands/ (and may not match the {short_name}.md naming). Consider detecting extension command names and resolving via the installed extension manifest (.specify/extensions/<ext-id>/extension.yml) to find the correct command file path, then substitute from that file; fall back to .specify/templates/commands/ for core commands.

Copilot uses AI. Check for mistakes.
Comment on lines +417 to +423
if frontmatter.get("strategy") == "wrap":
from .presets import _substitute_core_template
short_name = cmd_name
if short_name.startswith("speckit."):
short_name = short_name[len("speckit."):]
body, core_frontmatter = _substitute_core_template(body, short_name, project_root, self)
frontmatter = dict(frontmatter)
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For strategy: wrap, short_name is derived by stripping speckit. (so extension commands become e.g. git.feature) and then passed to _substitute_core_template(), which looks for .specify/templates/commands/{short_name}.md. This prevents {CORE_TEMPLATE} substitution for extension overrides. A more reliable approach is to pass the full command name into the resolver and have it locate the core source either in .specify/templates/commands/ (core) or .specify/extensions/<ext-id>/commands/ (extension), ideally via the installed extension manifest.

Copilot uses AI. Check for mistakes.
Comment on lines 3043 to +3046
assert result.exit_code == 1
output = strip_ansi(result.output).lower()
assert "bundled" in output, result.output
assert "reinstall" in output, result.output

Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bundled-preset CLI error test no longer asserts that the output includes the reinstall guidance. The CLI code paths for missing bundled presets print a “Try reinstalling …” message, so dropping this assertion reduces coverage for the intended recovery hint (and conflicts with the PR description that says it was restored). Re-add an assertion that the output contains "reinstall" (or the specific reinstall command string).

Copilot uses AI. Check for mistakes.
Comment on lines +3127 to +3142
import copy
original = copy.deepcopy(registrar.AGENT_CONFIGS)
registrar.AGENT_CONFIGS["test-agent"] = {
"dir": str(agent_dir.relative_to(project_dir)),
"format": "markdown",
"args": "$ARGUMENTS",
"extension": ".md",
"strip_frontmatter_keys": [],
}
try:
registrar.register_commands(
"test-agent", commands, "test-preset",
project_dir / "preset", project_dir
)
finally:
registrar.AGENT_CONFIGS = original
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test mutates registrar.AGENT_CONFIGS (a class-level dict) but restores it via registrar.AGENT_CONFIGS = original, which only sets an instance attribute and leaves the class-level dict permanently mutated (leaking state into other tests). Restore on the class (CommandRegistrar.AGENT_CONFIGS = original) or use a patching helper (e.g. monkeypatch/patch.dict) to avoid cross-test leakage.

This issue also appears in the following locations of the same file:

  • line 3280
  • line 3328

Copilot uses AI. Check for mistakes.
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.

3 participants