fix(presets): create skill overrides when skills_source is marketplace#2905
fix(presets): create skill overrides when skills_source is marketplace#2905echarrod wants to merge 2 commits into
Conversation
/analyze is explicitly read-only and produces a compact analysis report from heavy artefact reads (spec.md, plan.md, tasks.md). It matches the canonical use case for context: fork — bulk inputs that collapse to a short summary, no need for conversation history. Forking keeps the artefact contents out of the main conversation context, which is the concern raised in github#752. Done as a per-command opt-in via FORK_CONTEXT_COMMANDS so other spec-kit commands (which are interactive or have side effects) are unaffected. Refs github#752
When a project had `"skills_source": "marketplace"` in integration.json alongside `"ai_skills": false` in init-options.json, `_register_skills()` silently skipped writing the local SKILL.md override. Two guards both needed relaxing: 1. `_get_skills_dir()` returned None early for non-kimi agents when ai_skills was false, so _register_skills never reached the create_missing_skills check at all. 2. `create_missing_skills` was gated on `ai_skills_enabled`, so even if the skills dir existed the new skill file would not be created. For marketplace-sourced projects a local SKILL.md is required to shadow the marketplace plugin version. Both checks now treat ai_skills as effectively enabled when skills_source == "marketplace". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
echarrod
force-pushed
the
fix/preset-skills-marketplace-source
branch
from
d21c355 to
9a6557d
Compare
Explains the manual skill override step needed for projects using skills_source: marketplace, with a clear note to remove the file once github/spec-kit#2905 is released. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes preset skill override generation for marketplace-integrated projects where .specify/integration.json sets skills_source: "marketplace" but .specify/init-options.json has ai_skills: false, ensuring local SKILL.md overrides are still written when installing presets. It also introduces Claude-specific per-command forked-subagent frontmatter injection (currently for speckit-analyze) and adds/updates tests covering both behaviors.
Changes:
- Adjust
PresetManagerskill-dir resolution and missing-skill creation logic to treatai_skillsas effectively enabled whenskills_source == "marketplace". - Add regression coverage ensuring preset installs still create local
SKILL.mdoverrides in the marketplace +ai_skills: falsescenario. - Add Claude integration support/tests for injecting
context: fork+agent: general-purposeintospeckit-analyzeskill frontmatter.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/presets.py |
Bypass ai_skills guard for marketplace projects so preset installs can still write local skill overrides. |
src/specify_cli/integrations/claude/__init__.py |
Add per-command fork-context frontmatter injection for Claude skills (currently analyze). |
tests/test_presets.py |
Add regression test for marketplace skills_source with ai_skills: false still creating SKILL.md. |
tests/integrations/test_integration_claude.py |
Add tests verifying fork-context frontmatter injection is applied only to configured commands. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| from . import load_init_options, _get_skills_dir as _module_get_skills_dir | ||
| opts = load_init_options(self.project_root) or {} | ||
| agent = opts.get("ai") | ||
| if not isinstance(agent, str) or not agent: | ||
| return None | ||
| skills_dir = _module_get_skills_dir(self.project_root, agent) | ||
| return skills_dir if skills_dir.is_dir() else None |
| fork_config = FORK_CONTEXT_COMMANDS.get(stem) | ||
| if fork_config: | ||
| for key, value in fork_config.items(): | ||
| updated = self._inject_frontmatter_flag(updated, key, value) | ||
|
|
|
Hey @echarrod — thanks for the detailed write-up! I took a look but have some concerns. The The repro steps in #2906 also reference Could you point me to where PRs should deliver a single focused change. This PR bundles two unrelated things: a preset/marketplace fix and the |
Summary
"skills_source": "marketplace"in.specify/integration.jsonand"ai_skills": falseininit-options.json, runningspecify preset add <preset>appeared to succeed (registry updated, files placed in.specify/presets/) but silently skipped writing the local.claude/skills/<skill-name>/SKILL.mdoverride file.PresetManagerboth keyed onai_skills_enabled:_get_skills_dir()returnedNoneearly for non-kimi agents whenai_skillswas false, so_register_skills()bailed before reaching thecreate_missing_skillscheck.create_missing_skillswas itself gated onai_skills_enabled, so even when a skills dir existed the new skill file would not be created.skills_sourcefromintegration.jsonand treatai_skillsas effectively enabled whenskills_source == "marketplace". This enables the local SKILL.md to shadow the marketplace plugin version, which is the whole point of a preset override in a marketplace-integrated project.Fixes #2906
Test plan
TestPresetSkills::test_skill_created_when_ai_skills_false_but_marketplace_sourcecovers the exact failure scenario:ai_skills: false+skills_source: marketplace+ preset with atype: commandentry → SKILL.md IS created.TestPresetSkillstests (19 total) continue to pass -ai_skills: falsewithoutskills_source: marketplaceis unchanged.test_skill_not_updated_when_ai_skills_disabledconfirms the non-marketplaceai_skills: falsebehaviour is preserved.🤖 Generated with Claude Code