mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
feat: update Bob integration to skills-based layout for Bob 2.0 (#3415)
* feat: update Bob integration to skills-based layout for Bob 2.0 Bob 2.0 replaces the command-based workflow (.bob/commands/*.md) with a skills-based layout (.bob/skills/speckit-<name>/SKILL.md), matching the pattern used by Claude Code, Codex, and other skills-first agents. - Switch BobIntegration from MarkdownIntegration to SkillsIntegration - Update folder/dir from .bob/commands to .bob/skills - Change extension from .md to /SKILL.md (skills layout) - Add --skills option (default: True) consistent with Codex pattern - Update tests to inherit from SkillsIntegrationTests (28 tests pass) - Bump catalog entry to version 2.0.0 with updated description Assisted-by: IBM Bob (model: claude-sonnet-4-5, autonomous) * PR comments fix: keep old Bob 1 commands till next release * Copilot suggested change Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * feat(bob): address copilot comments, make skills layout default, demote legacy commands to opt-in * fix(bob): honor legacy_commands in ai_skills persistence and add bob to ALWAYS_SLASH_AGENTS - init.py: suppress ai_skills=True when --legacy-commands is passed so extensions and presets target .bob/commands, not .bob/skills - _invocation_style.py: add 'bob' to ALWAYS_SLASH_AGENTS so init next-steps and hook invocations always show /speckit-<name> (skills is the default layout; no ai_skills flag required) * Copilot suggestion Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(bob): extend IntegrationBase directly to avoid false isinstance(SkillsIntegration) - bob/__init__.py: switch BobIntegration base from SkillsIntegration to IntegrationBase; add _BobSkillsHelper for skills-mode delegation; set invoke_separator='-' explicitly; set _skills_mode flag in setup() so consumers can derive the effective mode without isinstance checks - _helpers.py: replace isinstance(integration, SkillsIntegration) guard with getattr(_skills_mode) so legacy-commands mode does not persist ai_skills=True - _invocation_style.py: remove 'bob' from ALWAYS_SLASH_AGENTS — Bob 2.0 skills are invoked via natural language, not /skill-name slash commands - integrations/catalog.json: advance updated_at to 2026-07-15 * fix(lint): remove unused SkillsIntegration import from _helpers.py * Copilot suggested change Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * feat(bob): add bob skills integration with registrar-based mode detection * address 3 comments from copilot * feat(bob): update registrar config to use legacy commands layout * fix lint * Suggested fix from Copilot Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix pr comment * fix pr comment * fix pr comment * refactor(bob): resolve skills mode via base-class hooks + fix command-ref separators Rework the dual-mode handling introduced for Bob 2.0 so an integration's internal representation never leaks into shared init/install/upgrade code, and fix the legacy command-reference separator surfaced in review. Base-class contract: - Add IntegrationBase.is_skills_mode(parsed_options) — the single hook the shared machinery consults to decide whether to persist ai_skills and render skill invocations. SkillsIntegration returns True; Copilot honors --skills / self._skills_mode; Bob returns `not legacy_commands`. - Add IntegrationBase.invoke_separator_for_mode(skills_enabled) — resolves the command-ref separator from a project's persisted mode for registration paths that only have the ai_skills flag (no CLI parsed_options). Default is behavior-preserving; Bob maps skills->"-", legacy->".". - BobIntegration stays on IntegrationBase (mirroring Copilot, the other dual-mode agent) and delegates setup() to internal _BobSkillsHelper / _BobMarkdownHelper. Removes the _skills_mode method and all isinstance(SkillsIntegration) / callable(_skills_mode) probing from _helpers.py and init.py. Fix legacy separator (review feedback): CommandRegistrar.register_commands and PresetManager._resolve_skill_command_refs previously read the single static AGENT_CONFIGS[key]["invoke_separator"], so legacy .bob/commands/ extension and preset command refs rendered /speckit-<cmd> instead of Bob 1.x /speckit.<cmd>. Both now resolve the separator per project mode via invoke_separator_for_mode. Tests: add regression coverage for the is_skills_mode / invoke_separator_for_mode hooks and legacy extension command-ref separators; normalize a width-sensitive workflow assertion to match its siblings. Full suite green. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob,copilot): address review — preserve legacy layout, dual-mode separators, extension-skill token resolution Addresses PR review 4716036212 (3 comments): 1. Bob legacy-install regression: `use`/`switch`/`upgrade` on an existing Bob 1.x project (only `.bob/commands/` on disk, no stored `legacy_commands`) called `is_skills_mode(None)` -> True and rewrote `ai_skills=True`, silently switching extension/command-reference handling to the skills layout. `is_skills_mode` now takes an optional `project_root`; Bob preserves an already-installed legacy layout until an explicit upgrade creates `.bob/skills/`. A fresh project still defaults to skills. 2. Copilot dual-mode separator: `invoke_separator_for_mode` was inherited from the base (mode-independent) and returned Copilot's static `.`, so preset/extension command refs in a Copilot skills project rendered `/speckit.<name>` instead of `/speckit-<name>`. Override it on Copilot to track the persisted `ai_skills` state, consistent with `build_command_invocation` and `effective_invoke_separator`. 3. Bob extension-skill command-ref tokens: verified that merging main's generic `_resolve_command_ref_tokens` (#3544) resolves Bob's tokens via the `CONDITIONAL_SLASH_AGENTS` path (`/speckit-<name>`); added Bob to the command-ref regression parametrize plus dedicated Bob use-path tests. All tests pass (full suite green; merged with current main incl. #3544). Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): resolve command-ref separator with project-aware mode before shared-infra refresh (review #3415) The `use`/`switch` paths refresh shared infrastructure via `_with_integration_setting()` / `_invoke_separator_for_integration()`, which previously resolved the invoke separator through `effective_invoke_separator` / `is_skills_mode` WITHOUT a project_root. For a pre-PR Bob 1.x project (.bob/commands/ on disk, no stored options), this defaulted to the skills "-" separator and rewrote rendered shared-template command refs to /speckit-*, even though ai_skills stayed false. Thread project_root through effective_invoke_separator, the two runtime helpers, and every call site so Bob's on-disk legacy detection governs the separator before shared infra is refreshed. Add a rendered-shared-template regression test covering `use --force`. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): scope persisted ai_skills flag to active agent when resolving command-ref separator (review #3415) `register_commands` runs once per detected agent, but the persisted `ai_skills` flag describes only the active integration (`opts["ai"]`). When another agent (e.g. Copilot) is active in skills mode while a legacy `.bob/commands` layout is also present, the previous code passed that global `True` to Bob's `invoke_separator_for_mode`, rewriting Bob 1.x command refs to `/speckit-*` instead of `/speckit.*`. Only consult the persisted flag for the agent it describes (`opts["ai"] == agent_name`); otherwise resolve the separator from the agent's own project-aware `effective_invoke_separator(None, project_root)`. Add regression tests covering the mismatched-active-agent case and a control for Bob-active skills mode. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): detect Spec Kit layout from managed artifacts, not any skills dir (review #3415) Two related mis-detections from review 4723246468: 1. `BobIntegration.is_skills_mode` treated the mere presence of a `.bob/skills/` directory as proof the project is skills-based. A legacy Spec Kit install (managed `.bob/commands/speckit.*.md`) that also carried unrelated Bob 2 skills would be misclassified as skills, so `integration use bob` persisted `ai_skills` and rewrote shared refs. Now the layout is inferred from managed Spec Kit artifacts: legacy/command mode only when managed `speckit.*.md` command files exist and no managed `speckit-*` skill dirs do. 2. The `register_commands` separator for an inactive agent used a disk-based `effective_invoke_separator(None, project_root)` fallback that could pick the skills separator even though the registrar writes the static command layout (`.bob/commands/*.md`). Inactive agents now resolve the separator from the registrar's actual output layout (`extension == "/SKILL.md"`), so command-layout files keep `/speckit.*` refs regardless of sibling dirs. Update the affected hook/E2E tests to use managed artifacts and add regression tests for the mixed-layout and inactive-registrar scenarios. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): apply managed-artifact detection on upgrade + consistent skill post-processing (review #3415) Two issues from review 4723782860: 1. `BobIntegration.setup()` resolved the layout via `is_skills_mode(parsed_options)` WITHOUT `project_root`, so `integration upgrade bob` on a Bob 1.x install (managed `.bob/commands/speckit.*.md`, no stored options) ignored the existing command files, generated skills, and stale-deleted the legacy commands — silently migrating the project. Pass `project_root` so the same managed-artifact detection used by `use` also governs upgrades. 2. Only `_BobSkillsHelper` overrode `post_process_skill_content` to suppress the shared slash-command hook note. Preset/extension skill generators call that hook on the registered `BobIntegration`, which inherited `IntegrationBase`'s note-injecting default. Repeat the no-op (delegating to the skills helper) on the registered class so every Bob skill-generation path is consistent with intent-activated core Bob skills. Add regression tests for the upgrade-preservation and post-processing paths. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * feat(bob): add --skills migration opt-in; fix separator + manifest loss (review #3415) Address review #3415 (4724160183): - Comment 1: Add an explicit `--skills` opt-in to BobIntegration. It forces the skills layout over on-disk auto-detection, giving legacy Bob 1.x installs a supported migration path (`integration upgrade bob --integration-options="--skills"`). `--skills` and `--legacy-commands` are mutually exclusive (clean exit-1 error). - Comment 2: In CommandRegistrar.register_commands, derive the command-ref separator from the output layout (agent_config["extension"]) for the active agent too, not the persisted ai_skills flag. A command-layout file (.bob/commands/*.md, .github/agents/*.agent.md) always renders /speckit.*; only a /SKILL.md scaffold uses /speckit-*. Dual-layout agents (Bob, Copilot) write skills via their own setup()/skills path, so register_commands only ever emits their command-layout files. - Comment 3: Update docs/reference/integrations.md Bob entry to document the skills-based default (.bob/skills/), the deprecated --legacy-commands opt-out, and the --skills migration path. Also fix a latent manifest-loss bug surfaced by the migration path: the upgrade Phase 2 stale-file cleanup built a throwaway manifest sharing the integration key and called uninstall(), which always deleted {key}.manifest.json. Any layout-shrinking upgrade (e.g. legacy->skills) thus wiped the freshly-saved manifest, leaving the project untracked and un-upgradeable. uninstall() now takes remove_manifest (default True); the stale-cleanup pass passes False. Adds regression tests for the --skills opt-in, mutual exclusion, corrected active-agent separator, remove_manifest=False, and an end-to-end legacy->skills migration that verifies the manifest survives and the project remains upgradeable. Full suite: 4555 passed, 5 skipped. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * docs(agents): align token-resolution comment with output-layout separator rule (review #3415) Address review #3415 (4725516805). The comment above resolve_command_refs still described the removed state-based behavior ("resolve it from the integration using the project's persisted skills state"). Update it to describe the output-layout rule that register_commands now uses: _sep is derived from the layout this registrar writes (a /SKILL.md scaffold uses the skills separator; a command-layout file uses the command separator), not the persisted ai_skills state. Comment-only change; no behavior change. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): reconcile extension artifacts on layout change (review #3415) When a dual-mode agent (Bob) flips between the legacy commands layout and the skills layout during `integration upgrade` (via `--skills` / `--legacy-commands`), the old layout's extension command/skill files were left orphaned: Phase 2 stale cleanup only removes files tracked by the *integration* manifest, while extension artifacts are tracked in the extension registry. Detect the layout flip by comparing whether the old vs new manifest tracks a `/SKILL.md` scaffold, and when it changed, unregister the agent's extension artifacts before the existing re-registration so they are recreated in the new layout (and the per-agent registry is updated). Preset artifacts are documented as a known, pre-existing cross-cutting gap: no agent-scoped preset re-registration exists in use/switch/upgrade for any agent, so reconciling them is out of scope for this Bob migration. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): reject layout migration when preset overrides are installed (review #3415) A command↔skills layout change during `integration upgrade` cannot reconcile preset artifacts: presets track their command/skill files in per-preset `registered_commands`/`registered_skills` metadata, and there is no agent-scoped preset re-registration anywhere in the CLI. Migrating would delete a preset's old-layout files without recreating them in the new layout and leave the preset registry claiming artifacts that no longer exist. Detect the intended layout via `is_skills_mode` (so a plain same-layout upgrade is unaffected) and, when it flips while preset overrides are installed for the agent, reject the upgrade *before any mutation* with an actionable error pointing at the remove → upgrade → reinstall workaround. Extension artifacts are still reconciled for the safe (no-preset) case. Adds a regression test and documents the migration caveat in the Bob integration reference entry. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): restrict layout reconciliation to the active integration (review #3415) `integration_upgrade` supports upgrading a secondary (non-active) integration, but the layout-change extension reconciliation was unsafe there. `ExtensionManager.unregister_agent_artifacts()` treats the unscoped per-extension `registered_skills` list as belonging to the passed agent and, when that agent's skills directory is absent, falls back to scanning every agent's skills directory — so reconciling a secondary Bob layout flip could delete or untrack the *active* agent's extension skills. The subsequent re-registration cannot repair that because extension skill rendering is intentionally scoped to the active agent (#2948). Gate the unregister-before-register reconciliation on `installed_key == key` so it only runs for the active integration. Secondary agents only ever have extension command files (skills are active-agent-only), which the existing re-registration rewrites in place, so skipping the unregister orphans nothing new. Adds a regression test asserting a secondary Bob layout change leaves the active agent's extension skill intact on disk and in the registry. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): fail closed when preset registry is unreadable (review #3415) Address review 4744636079: - _migrate_commands: the preset guard previously failed *open* — a registry read/parse error returned an empty "no presets" list, so a --force layout-changing upgrade could delete preset-overridden command files while their registry state was unknown. Read the registry file directly and raise _PresetRegistryUnreadableError on any read/parse failure or malformed structure, rejecting the migration before any mutation. A genuinely absent registry still returns [] (safe). - bob: correct the is_skills_mode docstring — upgrade *does* run setup(); disk detection is needed because legacy Bob 1.x installs never persisted a legacy_commands option, so the stored mode is unavailable. - tests: add fail-closed E2E (corrupted registry rejected, valid-empty allowed) plus a unit test for _installed_presets_affecting_agent covering absent / corrupted / malformed / valid / affecting-agent cases. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): fail closed on malformed preset entries too (review #3415) Address review 4745191015: the preset guard read a parseable registry but silently skipped malformed per-preset metadata and treated a malformed registered_commands value as "no matching artifacts". A registry such as {"presets":{"p1":[]}} therefore allowed a layout migration even though p1's ownership is unknown, risking deletion of preset-managed files. Now raise _PresetRegistryUnreadableError for a non-dict preset entry, a non-dict registered_commands, or a non-list registered_skills. Extend the unit test to cover these malformed shapes. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Manfred Riem <15701806+mnriem@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Davide Barletta
@@ -2477,6 +2477,300 @@ class TestIntegrationUpgrade:
|
||||
f"found: {[f.name for f in core_remaining]}"
|
||||
)
|
||||
|
||||
def test_upgrade_bob_skills_migration_preserves_manifest(self, tmp_path):
|
||||
"""Regression (review #3415, 4724160183, comment 1).
|
||||
|
||||
``integration upgrade bob --integration-options="--skills"`` migrates a
|
||||
legacy Bob 1.x install (``.bob/commands/*.md``) to the skills layout
|
||||
(``.bob/skills/speckit-*/SKILL.md``) and stale-removes the old command
|
||||
files. Because that stale-file pass shrinks the tracked set, the
|
||||
upgrade's Phase 2 must NOT delete the freshly-saved ``bob.manifest.json``
|
||||
— otherwise the migrated project is left untracked and un-upgradeable.
|
||||
"""
|
||||
project = _init_project(
|
||||
tmp_path, "bob", integration_options="--legacy-commands"
|
||||
)
|
||||
|
||||
commands = project / ".bob" / "commands"
|
||||
skills = project / ".bob" / "skills"
|
||||
manifest_path = (
|
||||
project / ".specify" / "integrations" / "bob.manifest.json"
|
||||
)
|
||||
assert commands.is_dir() and sorted(commands.glob("speckit.*.md"))
|
||||
assert not skills.exists()
|
||||
assert manifest_path.is_file()
|
||||
|
||||
result = _run_in_project(project, [
|
||||
"integration", "upgrade", "bob",
|
||||
"--integration-options", "--skills",
|
||||
"--script", "sh", "--force",
|
||||
])
|
||||
assert result.exit_code == 0, f"migration upgrade failed: {result.output}"
|
||||
|
||||
# Skills layout scaffolded; legacy core command files removed.
|
||||
assert skills.is_dir(), ".bob/skills/ must exist after --skills migration"
|
||||
assert sorted(skills.glob("speckit-*")), "expected migrated skill dirs"
|
||||
core_commands = [
|
||||
f for f in commands.glob("speckit.*.md")
|
||||
if "agent-context" not in f.name
|
||||
] if commands.exists() else []
|
||||
assert core_commands == [], (
|
||||
f"legacy core command files should be removed, found: "
|
||||
f"{[f.name for f in core_commands]}"
|
||||
)
|
||||
|
||||
# The manifest must survive so the project stays tracked/upgradeable.
|
||||
assert manifest_path.is_file(), (
|
||||
"bob.manifest.json must survive a layout-shrinking migration"
|
||||
)
|
||||
reupgrade = _run_in_project(project, [
|
||||
"integration", "upgrade", "bob", "--script", "sh", "--force",
|
||||
])
|
||||
assert reupgrade.exit_code == 0, (
|
||||
f"migrated project must remain upgradeable: {reupgrade.output}"
|
||||
)
|
||||
|
||||
def test_upgrade_bob_layout_change_reconciles_extension_artifacts(self, tmp_path):
|
||||
"""Regression (review #3415, 4725829110).
|
||||
|
||||
When a dual-mode agent (Bob) flips layout across an upgrade, the old
|
||||
layout's *extension* artifacts must be reconciled — not left orphaned.
|
||||
A legacy Bob install renders enabled extensions as ``.bob/commands/``
|
||||
command files; migrating to skills via ``--skills`` must remove those
|
||||
command files, recreate the extension as ``.bob/skills/`` skills, and
|
||||
update the extension registry accordingly (and vice-versa for the
|
||||
reverse ``--legacy-commands`` migration).
|
||||
"""
|
||||
project = _init_project(
|
||||
tmp_path, "bob", integration_options="--legacy-commands"
|
||||
)
|
||||
|
||||
result = _run_in_project(project, ["extension", "add", "git"])
|
||||
assert result.exit_code == 0, f"extension add failed: {result.output}"
|
||||
|
||||
commands = project / ".bob" / "commands"
|
||||
skills = project / ".bob" / "skills"
|
||||
registry_path = project / ".specify" / "extensions" / ".registry"
|
||||
|
||||
def _git_registry():
|
||||
data = json.loads(registry_path.read_text(encoding="utf-8"))
|
||||
g = data["extensions"]["git"]
|
||||
return list(g.get("registered_commands", {})), g.get(
|
||||
"registered_skills", []
|
||||
)
|
||||
|
||||
# Legacy precondition: git renders as command files under .bob/commands.
|
||||
assert sorted(commands.glob("speckit.git.*.md")), (
|
||||
"legacy Bob should render the git extension as command files"
|
||||
)
|
||||
assert not list(skills.glob("speckit-git-*")) if skills.exists() else True
|
||||
cmds_agents, skill_names = _git_registry()
|
||||
assert "bob" in cmds_agents and not skill_names
|
||||
|
||||
# Migrate legacy -> skills.
|
||||
result = _run_in_project(project, [
|
||||
"integration", "upgrade", "bob",
|
||||
"--integration-options", "--skills",
|
||||
"--script", "sh", "--force",
|
||||
])
|
||||
assert result.exit_code == 0, f"--skills migration failed: {result.output}"
|
||||
|
||||
# Old-layout git command files removed; skills recreated.
|
||||
assert not sorted(commands.glob("speckit.git.*.md")), (
|
||||
"git extension command files must be removed after --skills migration"
|
||||
)
|
||||
assert sorted(skills.glob("speckit-git-*")), (
|
||||
"git extension must be recreated as skills after --skills migration"
|
||||
)
|
||||
cmds_agents, skill_names = _git_registry()
|
||||
assert "bob" not in cmds_agents, (
|
||||
"extension registry must drop the stale bob command entry"
|
||||
)
|
||||
assert skill_names, "extension registry must record the migrated skills"
|
||||
|
||||
# Migrate skills -> legacy: the reverse reconciliation must also hold.
|
||||
result = _run_in_project(project, [
|
||||
"integration", "upgrade", "bob",
|
||||
"--integration-options", "--legacy-commands",
|
||||
"--script", "sh", "--force",
|
||||
])
|
||||
assert result.exit_code == 0, (
|
||||
f"--legacy-commands migration failed: {result.output}"
|
||||
)
|
||||
assert not sorted(skills.glob("speckit-git-*")), (
|
||||
"git extension skills must be removed after --legacy-commands migration"
|
||||
)
|
||||
assert sorted(commands.glob("speckit.git.*.md")), (
|
||||
"git extension command files must be recreated in legacy layout"
|
||||
)
|
||||
cmds_agents, skill_names = _git_registry()
|
||||
assert "bob" in cmds_agents and not skill_names
|
||||
|
||||
def test_upgrade_bob_layout_change_rejected_with_presets_installed(self, tmp_path):
|
||||
"""Regression (review #3415, 4726193915).
|
||||
|
||||
A command↔skills layout change cannot reconcile preset artifacts (no
|
||||
agent-scoped preset re-registration exists). Rather than silently
|
||||
orphaning preset files / leaving the registry inconsistent, a
|
||||
layout-changing ``upgrade`` must reject the migration with an
|
||||
actionable error *before any mutation* when preset overrides are
|
||||
installed for the agent. A same-layout upgrade must still succeed.
|
||||
"""
|
||||
project = _init_project(
|
||||
tmp_path, "bob", integration_options="--legacy-commands"
|
||||
)
|
||||
commands = project / ".bob" / "commands"
|
||||
skills = project / ".bob" / "skills"
|
||||
assert sorted(commands.glob("speckit.*.md"))
|
||||
|
||||
# Simulate an installed preset that registered command overrides for bob.
|
||||
presets_dir = project / ".specify" / "presets"
|
||||
presets_dir.mkdir(parents=True, exist_ok=True)
|
||||
(presets_dir / ".registry").write_text(
|
||||
json.dumps({
|
||||
"presets": {
|
||||
"my-preset": {
|
||||
"version": "1.0.0",
|
||||
"enabled": True,
|
||||
"registered_commands": {"bob": ["speckit.plan"]},
|
||||
"registered_skills": [],
|
||||
}
|
||||
}
|
||||
}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
# Layout-changing upgrade is rejected, and nothing is mutated.
|
||||
result = _run_in_project(project, [
|
||||
"integration", "upgrade", "bob",
|
||||
"--integration-options", "--skills",
|
||||
"--script", "sh", "--force",
|
||||
])
|
||||
assert result.exit_code != 0, "layout change with presets must be rejected"
|
||||
assert "preset" in result.output.lower()
|
||||
assert "my-preset" in result.output
|
||||
assert not skills.exists(), "no skills layout must be scaffolded on rejection"
|
||||
assert sorted(commands.glob("speckit.*.md")), (
|
||||
"legacy command files must be left untouched on rejection"
|
||||
)
|
||||
|
||||
# A same-layout upgrade (no flag) must still succeed with presets present.
|
||||
result = _run_in_project(project, [
|
||||
"integration", "upgrade", "bob", "--script", "sh", "--force",
|
||||
])
|
||||
assert result.exit_code == 0, (
|
||||
f"same-layout upgrade must not be blocked by presets: {result.output}"
|
||||
)
|
||||
|
||||
def test_upgrade_bob_layout_change_rejected_when_preset_registry_unreadable(
|
||||
self, tmp_path
|
||||
):
|
||||
"""Regression (review #3415, 4744636079).
|
||||
|
||||
The preset guard must fail *closed*: if the preset registry exists but
|
||||
cannot be read/parsed (corruption, permissions), the layout-changing
|
||||
upgrade must be rejected before any mutation rather than proceeding on
|
||||
a false "no presets installed" assumption (which would let ``--force``
|
||||
delete preset-overridden command files while their registry state is
|
||||
unknown). A genuinely absent registry must still be allowed.
|
||||
"""
|
||||
project = _init_project(
|
||||
tmp_path, "bob", integration_options="--legacy-commands"
|
||||
)
|
||||
commands = project / ".bob" / "commands"
|
||||
skills = project / ".bob" / "skills"
|
||||
assert sorted(commands.glob("speckit.*.md"))
|
||||
|
||||
# Corrupted (unparseable) registry: exists but cannot be read as JSON.
|
||||
presets_dir = project / ".specify" / "presets"
|
||||
presets_dir.mkdir(parents=True, exist_ok=True)
|
||||
(presets_dir / ".registry").write_text("{ not valid json", encoding="utf-8")
|
||||
|
||||
result = _run_in_project(project, [
|
||||
"integration", "upgrade", "bob",
|
||||
"--integration-options", "--skills",
|
||||
"--script", "sh", "--force",
|
||||
])
|
||||
assert result.exit_code != 0, (
|
||||
"layout change must be rejected when preset registry is unreadable"
|
||||
)
|
||||
assert "preset registry" in result.output.lower()
|
||||
assert not skills.exists(), "no skills layout may be scaffolded on rejection"
|
||||
assert sorted(commands.glob("speckit.*.md")), (
|
||||
"legacy command files must be untouched when failing closed"
|
||||
)
|
||||
|
||||
# A valid, empty registry must NOT block the migration.
|
||||
(presets_dir / ".registry").write_text(
|
||||
json.dumps({"presets": {}}), encoding="utf-8"
|
||||
)
|
||||
result = _run_in_project(project, [
|
||||
"integration", "upgrade", "bob",
|
||||
"--integration-options", "--skills",
|
||||
"--script", "sh", "--force",
|
||||
])
|
||||
assert result.exit_code == 0, (
|
||||
f"valid empty preset registry must not block migration: {result.output}"
|
||||
)
|
||||
assert skills.exists(), "skills layout should be scaffolded once unblocked"
|
||||
|
||||
def test_upgrade_secondary_bob_layout_change_preserves_active_agent_skills(
|
||||
self, tmp_path
|
||||
):
|
||||
"""Regression (review #3415, 4726347306).
|
||||
|
||||
``integration upgrade`` supports upgrading a *secondary* (non-active)
|
||||
integration. The layout-change extension reconciliation must NOT run
|
||||
for a secondary agent: ``unregister_agent_artifacts`` treats the
|
||||
unscoped per-extension ``registered_skills`` as belonging to the passed
|
||||
agent and, if that agent's skills dir is absent, scans every agent's
|
||||
skills dir — which could delete/untrack the *active* agent's extension
|
||||
skills. The following re-registration cannot repair that because
|
||||
extension skill rendering is active-agent-scoped (#2948).
|
||||
"""
|
||||
# Active agent: copilot in skills mode → git extension renders as skills.
|
||||
project = _init_project(tmp_path, "copilot", integration_options="--skills")
|
||||
result = _run_in_project(project, ["extension", "add", "git"])
|
||||
assert result.exit_code == 0, f"extension add failed: {result.output}"
|
||||
|
||||
skill = project / ".github" / "skills" / "speckit-git-feature" / "SKILL.md"
|
||||
assert skill.exists(), "precondition: active copilot has the git extension skill"
|
||||
|
||||
registry_path = project / ".specify" / "extensions" / ".registry"
|
||||
|
||||
def _git_skills():
|
||||
data = json.loads(registry_path.read_text(encoding="utf-8"))
|
||||
return data["extensions"]["git"].get("registered_skills", [])
|
||||
|
||||
assert _git_skills(), "precondition: git skills registered for active copilot"
|
||||
|
||||
# Add a secondary (non-active) Bob in the legacy commands layout.
|
||||
result = _run_in_project(project, [
|
||||
"integration", "install", "bob",
|
||||
"--integration-options", "--legacy-commands",
|
||||
"--script", "sh", "--force",
|
||||
])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
# Flip the *secondary* Bob's layout to skills. copilot stays active.
|
||||
result = _run_in_project(project, [
|
||||
"integration", "upgrade", "bob",
|
||||
"--integration-options", "--skills",
|
||||
"--script", "sh", "--force",
|
||||
])
|
||||
assert result.exit_code == 0, result.output
|
||||
|
||||
# The active agent's extension skill must be untouched on disk and in
|
||||
# the registry — the secondary layout change must not reconcile it.
|
||||
assert skill.exists(), (
|
||||
"secondary Bob layout change must not delete the active agent's "
|
||||
"extension skill"
|
||||
)
|
||||
assert _git_skills(), (
|
||||
"secondary Bob layout change must not untrack the active agent's "
|
||||
"extension skills in the registry"
|
||||
)
|
||||
|
||||
def test_upgrade_preserves_existing_vscode_settings(self, tmp_path):
|
||||
"""Regression: copilot upgrade must not stale-delete .vscode/settings.json.
|
||||
|
||||
@@ -2616,6 +2910,82 @@ class TestIntegrationUpgrade:
|
||||
"deleted extension skill (#2886)"
|
||||
)
|
||||
|
||||
def test_installed_presets_affecting_agent_absent_vs_unreadable(self, tmp_path):
|
||||
"""Unit (review #3415, 4744636079): fail closed only when unreadable.
|
||||
|
||||
The preset guard helper must return an empty list for a genuinely
|
||||
absent registry, but raise ``_PresetRegistryUnreadableError`` when the
|
||||
registry exists yet cannot be read/parsed — so a layout-changing
|
||||
upgrade never proceeds on a false "no presets" result.
|
||||
"""
|
||||
from specify_cli.integrations._migrate_commands import (
|
||||
_PresetRegistryUnreadableError,
|
||||
_installed_presets_affecting_agent,
|
||||
)
|
||||
|
||||
project = tmp_path / "proj"
|
||||
project.mkdir()
|
||||
|
||||
# Genuinely absent registry → empty list (safe to proceed).
|
||||
assert _installed_presets_affecting_agent(project, "bob") == []
|
||||
|
||||
presets_dir = project / ".specify" / "presets"
|
||||
presets_dir.mkdir(parents=True)
|
||||
registry = presets_dir / ".registry"
|
||||
|
||||
# Corrupted JSON → unreadable → raise.
|
||||
registry.write_text("{ not json", encoding="utf-8")
|
||||
with pytest.raises(_PresetRegistryUnreadableError):
|
||||
_installed_presets_affecting_agent(project, "bob")
|
||||
|
||||
# Malformed structure (presets not a dict) → unreadable → raise.
|
||||
registry.write_text(json.dumps({"presets": []}), encoding="utf-8")
|
||||
with pytest.raises(_PresetRegistryUnreadableError):
|
||||
_installed_presets_affecting_agent(project, "bob")
|
||||
|
||||
# Malformed per-preset entry (not a dict) → ownership unknown → raise.
|
||||
registry.write_text(
|
||||
json.dumps({"presets": {"p1": []}}), encoding="utf-8"
|
||||
)
|
||||
with pytest.raises(_PresetRegistryUnreadableError):
|
||||
_installed_presets_affecting_agent(project, "bob")
|
||||
|
||||
# Malformed registered_commands (not a dict) → raise.
|
||||
registry.write_text(
|
||||
json.dumps({"presets": {"p1": {"registered_commands": []}}}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
with pytest.raises(_PresetRegistryUnreadableError):
|
||||
_installed_presets_affecting_agent(project, "bob")
|
||||
|
||||
# Malformed registered_skills (not a list) → raise.
|
||||
registry.write_text(
|
||||
json.dumps({"presets": {"p1": {"registered_skills": {}}}}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
with pytest.raises(_PresetRegistryUnreadableError):
|
||||
_installed_presets_affecting_agent(project, "bob")
|
||||
|
||||
# Valid, empty registry → empty list.
|
||||
registry.write_text(json.dumps({"presets": {}}), encoding="utf-8")
|
||||
assert _installed_presets_affecting_agent(project, "bob") == []
|
||||
|
||||
# Valid registry with a preset registered for bob → reported.
|
||||
registry.write_text(
|
||||
json.dumps({
|
||||
"presets": {
|
||||
"p1": {"registered_commands": {"bob": ["speckit.plan"]}},
|
||||
"p2": {"registered_commands": {"codex": ["speckit.plan"]}},
|
||||
"p3": {"registered_skills": ["speckit-x"]},
|
||||
}
|
||||
}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
assert sorted(_installed_presets_affecting_agent(project, "bob")) == [
|
||||
"p1",
|
||||
"p3",
|
||||
]
|
||||
|
||||
|
||||
# ── Full lifecycle ───────────────────────────────────────────────────
|
||||
|
||||
|
||||
Reference in New Issue
Block a user