Skip to content

feat: implement preset wrap strategy#2189

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

feat: implement preset wrap strategy#2189
kennedy-whytech wants to merge 4 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 and skill overrides. Presets can now inject content before and after a core command using a {CORE_TEMPLATE} placeholder, without fully replacing it.
Multiple wrap presets compose in deterministic priority order — the lowest-priority number wraps outermost, the highest-priority wraps innermost.

What's new

Core feature

  • strategy: wrap wired into both register_commands (all agents) and _register_skills (ai-skills path)
  • {CORE_TEMPLATE} resolves against core templates and extensions, never against installed presets, to prevent accidental nesting
  • Presets inherit scripts/agent_scripts from the core command when they don't define their own

Multi-preset composability (according to PR comments)

  • Installing multiple wrap presets for the same command composes them in priority order rather than last-write-wins
  • _replay_wraps_for_command recomposes all enabled wrap presets after every install and remove
  • _replay_skill_override keeps SKILL.md in sync with the recomposed body for ai-skills-enabled projects
  • Registry stores wrap_commands per preset; remove() re-orders operations so replay sees post-removal state before recomposing

Bug fixes

  • {SCRIPT} and {AGENT_SCRIPT} placeholders were not being resolved in the markdown/YAML agent branch when strategy: wrap was active — fixed by calling resolve_skill_placeholders +
    _convert_argument_placeholder after wrap substitution
  • strategy: wrap key was leaking into the rendered frontmatter of the output file — stripped after substitution
  • Argument placeholder conversion ({ARGS} → agent-specific format) was running before resolve_skill_placeholders, so $ARGUMENTS injected by skill resolution was never converted — ordering fixed
  • Removed preset key from init-options.json during project init (was being stored but never used as authoritative state)

Test hygiene

  • 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

    • uvx pytest tests/test_presets.py::TestWrapStrategy — all wrap strategy tests pass
    • uvx pytest tests/test_presets.py — full preset suite (209 tests)
    • uvx pytest — full suite, 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

Comment thread src/specify_cli/presets.py Outdated
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

Comment thread tests/test_presets.py
Comment thread tests/test_presets.py
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 thread src/specify_cli/agents.py Outdated
@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

@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 thread src/specify_cli/presets.py Outdated
Comment thread src/specify_cli/agents.py
Comment thread tests/test_presets.py
Comment thread tests/test_presets.py Outdated
@mnriem mnriem self-requested a review April 14, 2026 12:30
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

@mnriem mnriem requested a review from Copilot April 14, 2026 20:26
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 a new preset command override strategy (strategy: wrap) that allows presets to inject content before/after an existing command template via a {CORE_TEMPLATE} placeholder, and wires this behavior into both skills generation and agent command registration paths.

Changes:

  • Added _substitute_core_template() and integrated wrap behavior into preset skill registration (_register_skills) and agent command registration (register_commands).
  • Updated TOML command generation to resolve script placeholders and convert argument placeholders appropriately.
  • Expanded preset test coverage (including self-test preset updates) for wrap strategy behavior and script metadata inheritance.
Show a summary per file
File Description
src/specify_cli/presets.py Adds {CORE_TEMPLATE} substitution helper and merges core script metadata into wrapped presets for skills generation.
src/specify_cli/agents.py Applies wrap substitution for command registration and resolves placeholders for TOML outputs.
tests/test_presets.py Adds a dedicated wrap strategy test suite and updates self-test preset assertions.
presets/self-test/preset.yml Registers a new self-test wrap command entry.
presets/self-test/commands/speckit.wrap-test.md Adds a wrap-strategy command fixture using {CORE_TEMPLATE}.
presets/README.md Updates documentation to reflect wrap support status (commands/artifacts implemented; scripts 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:3300

  • Same AGENT_CONFIGS restoration issue as above: reassigning CommandRegistrar.AGENT_CONFIGS = original won’t restore the dict object referenced by specify_cli.extensions.CommandRegistrar.AGENT_CONFIGS, so the temporary test-agent entry can leak into later tests. Prefer an in-place restore (clear/update) or patch.dict.
        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:
            CommandRegistrar.AGENT_CONFIGS = original

tests/test_presets.py:3348

  • Same AGENT_CONFIGS restoration issue as above for the TOML agent config: reassigning the class attribute can leave specify_cli.extensions.CommandRegistrar.AGENT_CONFIGS pointing at the mutated dict (with test-toml-agent still present). Restore the dict contents in-place or use patch.dict to avoid cross-test leakage.
        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:
            CommandRegistrar.AGENT_CONFIGS = original

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

Comment thread tests/test_presets.py
Comment thread src/specify_cli/presets.py Outdated
Comment thread tests/test_presets.py Outdated
@mnriem mnriem self-requested a review April 14, 2026 20:35
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

@mnriem mnriem requested a review from Copilot April 14, 2026 20: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

Implements a strategy: wrap composition mode for preset command overrides, allowing presets to inject pre/post content around an existing command via a {CORE_TEMPLATE} placeholder while preserving core script metadata needed for placeholder/script resolution.

Changes:

  • Added _substitute_core_template() to replace {CORE_TEMPLATE} with the resolved underlying command body and return the underlying command frontmatter for inheritance.
  • Wired wrap behavior into both preset skill generation (_register_skills) and general command registration (register_commands), including inheriting scripts / agent_scripts from the wrapped command.
  • Expanded self-test preset + added a dedicated wrap strategy test suite covering substitution, script inheritance, TOML placeholder resolution, and extension command lookup.
Show a summary per file
File Description
tests/test_presets.py Adds wrap-strategy unit/e2e tests and updates self-test manifest expectations.
src/specify_cli/presets.py Introduces _substitute_core_template() and applies wrap strategy + script inheritance in _register_skills.
src/specify_cli/agents.py Applies wrap strategy + script inheritance in register_commands and resolves {SCRIPT}/{ARGS} placeholders for TOML outputs.
presets/self-test/preset.yml Adds a new self-test wrap-strategy command entry.
presets/self-test/commands/speckit.wrap-test.md Adds a wrap command template demonstrating {CORE_TEMPLATE} usage.
presets/README.md Clarifies wrap is implemented for artifacts/commands and not yet implemented for 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: 0 new

@kennedy-whytech kennedy-whytech requested a review from mnriem April 15, 2026 20:39
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 test & linting errors

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.

Nice work on the core {CORE_TEMPLATE} substitution mechanism — the approach and test coverage are solid. A few issues need addressing before merge:

1. {SCRIPT} unresolved for Markdown and YAML agents

The PR adds resolve_skill_placeholders() + _convert_argument_placeholder() for the TOML branch in register_commands() (lines 500-501 of agents.py), but the markdown and YAML branches don't get the same treatment. When a preset wraps a core command that uses {SCRIPT} (which most do), the literal {SCRIPT} placeholder will appear in the installed command file for all markdown agents (Claude, Windsurf, Copilot, Forge, etc.) and Goose.

The TOML fix you already have proves the gap — the same resolution needs to happen for markdown and YAML.

2. strategy: wrap leaks into markdown output

render_frontmatter() dumps the full frontmatter dict, so strategy: wrap ends up verbatim in installed .md / .agent.md files. TOML and YAML are unaffected (they only extract description/title), and skills rebuild frontmatter from scratch, but all markdown-format agents will have this stray key. Either strip it from frontmatter after the wrap substitution block, or add "strategy" to the keys that get popped.

3. Suggestion: consider a test with {SCRIPT} in the core template for markdown output

The existing test_register_commands_toml_resolves_inherited_scripts is great — a parallel test asserting {SCRIPT} is absent from a markdown agent's output would catch issue #1 above and prevent regressions.

@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented Apr 15, 2026

4. Multi-preset wrapping must respect priority ordering

Currently, _substitute_core_template() resolves {CORE_TEMPLATE} via PresetResolver.resolve(), which returns the first match from the 4-tier priority stack. This means:

  • If two presets both strategy: wrap the same command, the one installed second simply overwrites the first — there's no composable nesting.
  • Worse, PresetResolver can accidentally resolve another preset's already-installed wrap output as the "core", producing order-dependent nesting that changes based on which preset was installed first.

Expected behavior: When multiple presets wrap the same command, priority should control nesting order — highest precedence (lowest number) outermost, lowest precedence innermost, core template at the center:

[priority 1 pre]
  [priority 5 pre]
    [priority 10 pre]
      <core template body>
    [priority 10 post]
  [priority 5 post]
[priority 1 post]

Suggested approach — replay on install/uninstall:

  1. When resolving {CORE_TEMPLATE}, always resolve against the actual core (skip .specify/presets/ in the resolver, or resolve from tiers 1/3/4 only — overrides, extensions, core templates). This prevents accidental nesting from previously-installed presets.

  2. After installing or uninstalling any preset, replay all wrap-strategy commands for affected command names:

    • Collect all installed presets that declare strategy: wrap for a given command (the registry already tracks registered_commands per preset, so this data is available).
    • Sort them by priority descending (highest number = lowest precedence first, so they become the innermost layer).
    • Start with the core template body.
    • Apply each preset's wrap in that order — substituting {CORE_TEMPLATE} with the accumulated result so far.
    • The highest-precedence preset wraps last and becomes the outermost layer.
    • Write the final composed result to each agent directory.
  3. Uninstall should trigger the same replay for any commands that the removed preset was wrapping, so the remaining presets re-compose correctly without the removed one.

The registry already stores registered_commands per preset and priority per preset, so the data needed for replay is available. The main change is: (a) make _substitute_core_template skip the presets tier when resolving core, (b) add a _replay_wraps_for_command(cmd_name) helper that collects all wrap presets for that command and re-composes them in priority order, and (c) call it from both install_from_directory / install_from_zip and remove.

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 a new strategy: wrap mechanism for preset command overrides so presets can inject content around an existing command body via {CORE_TEMPLATE}, while inheriting relevant script metadata (e.g., scripts / agent_scripts) from the wrapped core command.

Changes:

  • Added _substitute_core_template() to resolve and substitute {CORE_TEMPLATE} (including extension command resolution via PresetResolver).
  • Wired strategy: wrap into both preset skills generation (_register_skills) and agent command registration (register_commands), including inheriting missing scripts / agent_scripts.
  • Added comprehensive wrap-strategy tests and updated the self-test preset to include a dedicated wrap test command.
Show a summary per file
File Description
tests/test_presets.py Adds wrap-strategy unit + integration tests; updates self-test manifest count assertion.
src/specify_cli/presets.py Introduces _substitute_core_template() and uses it in _register_skills to support wrapped commands + script metadata inheritance.
src/specify_cli/agents.py Adds wrap handling in register_commands and resolves script placeholders for TOML agent output.
presets/self-test/preset.yml Registers a new self-test command to validate wrap behavior end-to-end.
presets/self-test/commands/speckit.wrap-test.md Adds a wrap-strategy command fixture using {CORE_TEMPLATE} with pre/post content.
presets/README.md Updates documentation to reflect wrap support for commands/artifacts and not yet for 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: 0 new

@mnriem mnriem self-requested a review April 15, 2026 21:19
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 comments 1-4. Great work already. So cool to see this!

@kennedy-whytech
Copy link
Copy Markdown
Author

kennedy-whytech commented Apr 16, 2026

@mnriem
Out of scope:

There appears to be a core Spec Kit bug in how preset state is interpreted downstream.

Observed behavior:

  • A project initialized with .specify/init-options.json containing "preset": null
  • Then specify preset add --dev succeeds
  • Later downstream logic treats "preset": null in init-options.json as meaning no preset is active, and skips preset-specific post-
    logic

Why this is a bug:

  • init-options.json["preset"] is init-time metadata, not authoritative active preset state
  • Presets can be added after init, removed later, and potentially stacked
  • A single nullable preset field is not a reliable representation of active preset state
  • Active preset state should come from the preset registry / resolver, not init-options.json

Impact:

  • Preset-specific command behavior can be skipped incorrectly even when the preset is installed and command resolution should apply
  • In our case this caused downstream logic to say the preset was null and skip Step 0 / post-logic

Suggested fix:

  • Stop using init-options.json["preset"] as the signal for whether a preset is active
  • Use the installed preset registry or command resolver as the source of truth
  • If preset remains in init-options.json, document it as init-only metadata

@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented Apr 16, 2026

OK not sure why you are saying out of scope?

@kennedy-whytech
Copy link
Copy Markdown
Author

kennedy-whytech commented Apr 16, 2026

@mnriem

This touches the broader preset install lifecycle rather than strategy: wrap specifically, so I think it warrants a separate issue — but wanted to flag it here and confirm before filing. Also, is this a known issue?

@mnriem
Copy link
Copy Markdown
Collaborator

mnriem commented Apr 17, 2026

Lets get your current work in and then we'll start addressing the rest. Some of it was known, some of it was not

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.

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 thread src/specify_cli/presets.py
@mnriem mnriem self-requested a review April 17, 2026 12:51
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 it is applicable within the context of the PR. If not, please explain why not

@kennedy-whytech kennedy-whytech force-pushed the kennedy-whytech-feat-preset-wrap-strategy branch from f142db7 to 5a768e6 Compare April 18, 2026 16:38
Implements comment github#4 from PR review: multiple installed wrap presets
now compose in priority order rather than overwriting each other.

Key changes:
- PresetResolver.resolve() gains skip_presets flag; resolve_core() wraps
  it to skip tier 2, preventing accidental nesting during replay
- _replay_wraps_for_command() recomposed all enabled wrap presets for a
  command in ascending priority order (innermost-first) after any
  install or remove
- _replay_skill_override() keeps SKILL.md in sync with the recomposed
  command body for ai-skills-enabled projects
- install_from_directory() detects strategy: wrap commands, stores
  wrap_commands in the registry entry, and calls replay after install
- remove() reads wrap_commands before deletion, removes registry entry
  before rmtree so replay sees post-removal state, then replays
  remaining wraps or unregisters when none remain

Tests: TestResolveCore (5), TestReplayWrapsForCommand (5),
TestInstallRemoveWrapLifecycle (5), plus 2 skill/alias regression tests
@kennedy-whytech kennedy-whytech force-pushed the kennedy-whytech-feat-preset-wrap-strategy branch from 5a768e6 to 7c643f9 Compare April 18, 2026 16:42
PresetResolver.resolve_extension_command_via_manifest() consults each
installed extension.yml to find the actual file declared for a command
name, rather than assuming the file is named <cmd_name>.md.  This fixes
_substitute_core_template for extensions like selftest where the manifest
maps speckit.selftest.extension → commands/selftest.md.

Resolution order in _substitute_core_template is now:
  1. resolve_core(cmd_name) — project overrides win, then name-based lookup
  2. resolve_extension_command_via_manifest(cmd_name) — manifest fallback
  3. resolve_core(short_name) — core template short-name fallback

Path traversal guard mirrors the containment check already present in
ExtensionManager to reject absolute paths or paths escaping the extension
root.
@kennedy-whytech
Copy link
Copy Markdown
Author

Comment 1 — {SCRIPT} unresolved for Markdown/YAML:

  • Fixed. Script placeholder resolution and _convert_argument_placeholder() now run for the markdown and YAML branches in register_commands(), matching the existing TOML treatment.

Comment 2 — strategy: wrap leaks into markdown output:

  • Fixed. strategy is popped from frontmatter immediately after the wrap substitution block, so it no longer appears in installed .md / .agent.md files.

Comment 3 — Suggest test for {SCRIPT} in markdown output:

  • Added test_register_commands_markdown_resolves_inherited_scripts which asserts {SCRIPT} is absent from a markdown agent's installed output when the core template carries it.

Comment 4 — Multi-preset wrapping must respect priority ordering:

  • Addressed. _substitute_core_template now uses resolve_core() to skip the presets tier, and the new _replay_wraps_for_command() helper re-composes all wrap presets in priority order (lowest
    precedence innermost) on both install and uninstall.

@kennedy-whytech
Copy link
Copy Markdown
Author

kennedy-whytech commented Apr 18, 2026

@mnriem Out of scope:

There appears to be a core Spec Kit bug in how preset state is interpreted downstream.

Observed behavior:

  • A project initialized with .specify/init-options.json containing "preset": null
  • Then specify preset add --dev succeeds
  • Later downstream logic treats "preset": null in init-options.json as meaning no preset is active, and skips preset-specific post-
    logic

Why this is a bug:

  • init-options.json["preset"] is init-time metadata, not authoritative active preset state
  • Presets can be added after init, removed later, and potentially stacked
  • A single nullable preset field is not a reliable representation of active preset state
  • Active preset state should come from the preset registry / resolver, not init-options.json

Impact:

  • Preset-specific command behavior can be skipped incorrectly even when the preset is installed and command resolution should apply
  • In our case this caused downstream logic to say the preset was null and skip Step 0 / post-logic

Suggested fix:

  • Stop using init-options.json["preset"] as the signal for whether a preset is active
  • Use the installed preset registry or command resolver as the source of truth
  • If preset remains in init-options.json, document it as init-only metadata

for this, I have removed init-options.json["preset"] for now. I'm not super sure about the intention of init-options.json["preset"] . Saving preset to init-options.json is redundant because the PresetRegistry already tracks all installed presets as the authoritative source. Whether the preset is maintainer-defined or user-specified, downstream operations should read the registry — not init-options.json? (We do have other options like template if it's maintainer-defined as well.) i guess it makes sense if the config is stored somewhere globally, so we can init a project with a set of presets?

@kennedy-whytech kennedy-whytech requested a review from mnriem April 18, 2026 17:07
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