mirror of
https://github.com/github/spec-kit.git
synced 2026-07-03 12:28:06 +08:00
* fix(shared-infra): record skipped files in speckit.manifest.json
`install_shared_infra` skipped files that already existed on disk
when `force=False`, but the skip branches in both the scripts loop
and the templates loop only appended to `skipped_files` without
calling `manifest.record_existing`. So when the function ran with a
fresh manifest against an already-populated `.specify/` tree (e.g.
after the manifest was deleted, corrupted, or extracted out of band),
every file went down the skip path, `planned_copies` /
`planned_templates` stayed empty, and `manifest.save()` wrote an
empty `files` field — leaving the integration believing nothing was
installed.
Record every skipped file in the manifest, but only when it is not
already tracked. This preserves the original hash for files that
were previously recorded so `check_modified()` (used by
`integration use` to decide whether a user has customized a
template) keeps working correctly.
Add `TestSpeckitManifestRecordsSkippedFiles` in
`tests/integrations/test_integration_claude.py` covering both the
fresh-skip path and the recover-after-lost-manifest path.
Fixes #2107
* fix(shared-infra): guard manifest.record_existing against non-file dst
Address Copilot review feedback on PR #2483. The previous fix called
``manifest.record_existing(rel_skip)`` from the skip branch of both
loops in ``install_shared_infra``, which would crash with
``IsADirectoryError`` (or another ``OSError``) if a directory or other
non-regular-file happened to exist at the expected destination path —
since ``record_existing`` opens the file to compute its SHA-256.
Three coordinated fixes:
1. ``IntegrationManifest.record_existing`` now validates its
precondition: it raises ``ValueError`` if the path is a symlink or
is not a regular file. The docstring already promised "an
already-existing file"; this enforces it. The symlink check runs on
the un-resolved path because ``_validate_rel_path`` calls
``resolve()``, which would silently follow the symlink. Mirrors the
existing ``_ensure_safe_manifest_destination`` precedent in the
same module.
2. In ``install_shared_infra``'s scripts and templates skip branches,
guard the ``record_existing`` call with ``dst.is_file()`` and wrap
it in ``try/except (OSError, ValueError)``. A directory collision,
permission error, or TOCTOU race no longer aborts the whole
install — the user gets a per-path warning, the path still
surfaces in ``skipped_files``, and the rest of the install
continues.
3. ``_read_manifest_files`` in the regression test no longer falls
back to ``data.get("_files")`` (Copilot's low-confidence finding):
the silent fallback could mask a schema regression where the
public ``files`` key is renamed. It now asserts ``"files" in data``
and that the value is a dict.
Add two regression tests in ``TestSpeckitManifestRecordsSkippedFiles``
covering the directory-at-destination edge case for both the scripts
loop and the templates loop. Both verify (a) install does not crash,
(b) the non-file path is not recorded in the manifest, and (c) the
path still surfaces in the user-visible warning.
The "shared infrastructure file(s)" warning text is changed to
"path(s)" so it remains accurate when non-file entries appear in the
list.
Refs #2107
* fix(manifest): lexical pre-check for record_existing + add error-case tests
Address Copilot review (2026-05-11, review id 4266902103):
1. `record_existing` was calling `(self.project_root / rel).is_symlink()`
BEFORE validating containment. For absolute paths or paths containing
`..`, this performed a filesystem stat outside the project root before
`_validate_rel_path()` raised. Add a cheap lexical pre-check that
delegates to `_validate_rel_path()` for the canonical error messages,
so the symlink stat only ever runs on paths that are already lexically
inside the project root.
2. Add focused unit tests in `tests/integrations/test_manifest.py` for
the symlink and non-regular-file error paths, including:
- symlink target rejection
- dangling symlink rejection (caught by the symlink guard before
the is_file check)
- directory path rejection (is_file == False)
- missing-path rejection (is_file == False)
- absolute-path lexical pre-check
The Copilot reviewer noted these guards had no focused coverage in
`test_manifest.py`, only via the `test_integration_claude.py`
regression test.
3. The third Copilot finding (repeated `dict(self._files)` copies via
`manifest.files` in the skip branches) is already resolved on this
branch by using `prior_hashes` — the function-scope snapshot taken at
the top of `install_shared_infra` — for the membership check, instead
of `manifest.files`.
AI disclosure: drafted with assistance from Claude (Opus 4.7).
* fix(manifest): track recovered files separately + symlink-ancestor + canonical-path guards
Address Copilot review id 4309888722 (2026-05-18) on PR #2483:
1. Recovery semantics (shared_infra.py:371, 412) — install_shared_infra
now passes ``recovered=True`` when re-recording a skipped existing
file. This flag funnels into a new ``recovered_files`` array in the
manifest JSON, so a future ``refresh_managed`` run can distinguish
"hash I produced" from "hash I observed on a file that may be a user
customization" and avoid silent overwrite without ``--refresh-shared-infra``.
Schema is purely additive: ``files: dict[str, str]`` is unchanged; the
new ``recovered_files: list[str]`` is omitted when empty.
2. Symlinked ancestor (manifest.py:172) — ``record_existing`` now walks
every component of the rel path and rejects any symlinked ancestor,
not just a symlinked leaf. Catches ``linked_dir/file.txt`` where
``linked_dir`` is a symlink, which previously slipped past the leaf-only
``is_symlink()`` check and was resolved through by ``_validate_rel_path``.
Mirrors the component-walk pattern in ``_ensure_safe_manifest_directory``.
3. Misleading "escapes project root" message (manifest.py:168) — paths
like ``dir/../file.txt`` normalize inside the project, so the old
message lied about what was wrong. New message: "Manifest paths must
be canonical; '..' segments are not allowed". Still rejects (canonical
keys are required so ``check_modified``/``uninstall`` cannot key the
same file under two paths).
Tests: 7 new test methods across TestManifestRecoveredFiles and
TestRecordExistingNewGuards covering all 4 Copilot findings. Full suite
passes locally.
🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7).
* fix(manifest): normalize is_recovered input through _validate_rel_path
Address Copilot review comment id 4309888722 round-5 (2026-05-21) on PR #2483:
``is_recovered()`` previously checked ``self._recovered_files`` membership
with bare ``Path(rel).as_posix()``, while ``record_existing()`` stores keys
via ``_validate_rel_path(rel, root).relative_to(root).as_posix()``. The two
normalizations disagreed on absolute paths and paths that escape the
project root — ``is_recovered`` would silently return False for inputs that
``record_existing`` would have refused entirely.
The fix routes ``is_recovered`` through the same ``_validate_rel_path``
pipeline; ``ValueError`` from the validator is caught and converted to
False so query semantics stay exception-free (Python ``__contains__``
convention).
Tests: 2 new methods in ``TestManifestRecoveredFiles``:
- ``test_is_recovered_absolute_path_returns_false``
- ``test_is_recovered_escaping_path_returns_false``
🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7).
* fix(manifest): clear recovered marker on managed re-record + reject '..' in is_recovered
Address Copilot Round-7 review comments on PR #2483:
1. record_existing(recovered=False) and record_file now BOTH discard the
path from _recovered_files. The marker is meant to flag "we observed
this file but cannot vouch it's a managed baseline" — once the same
path is re-recorded as managed (either explicitly or by writing fresh
bytes), the marker is stale and must clear so refresh_managed and
future is_recovered queries return the truthful answer.
2. is_recovered now applies the same canonical-key guard as record_existing
(rejects absolute paths and '..' segments lexically before delegating
to _validate_rel_path). Such paths can never be stored keys, so the
query correctly returns False without depending on _validate_rel_path
semantics that diverged from record_existing's stricter contract.
record_file docstring updated to mention the side-effect on recovered
markers.
Tests: 3 new methods in TestManifestRecoveredFiles covering
record_existing(false) clearing, record_file clearing, and is_recovered
dotdot rejection.
* test(manifest): update is_recovered comments to reflect Round-7 lexical guard
Round 8 — addresses Copilot review comment on tests/integrations/test_manifest.py:362.
After Round-7 (1dbf0c2), is_recovered() rejects absolute paths and '..' segments
up front via a lexical guard, returning False without calling _validate_rel_path
at all. The test comments still described the prior "_validate_rel_path raises;
we catch" code path, which is misleading for readers.
Updated comments in both:
- test_is_recovered_absolute_path_returns_false (Copilot's exact target)
- test_is_recovered_escaping_path_returns_false (same comment-class issue;
fixed preemptively to avoid a Round-9 finding on the same drift)
Pure documentation change. Test assertions and behavior unchanged; all manifest
tests still green.
* fix(manifest): document OS errors on record_existing + filter orphan recovered_files on load
Round 9 — addresses Copilot review on PR #2483:
1. record_existing's docstring now documents OSError/PermissionError as
possible raises (in addition to ValueError) — the implementation has
always been able to raise them from is_symlink, is_file, or the
file-read used to hash, but the contract did not reflect that.
Callers should be prepared for both surfaces.
2. load() now filters recovered_files entries that don't correspond to
keys in files. An externally-edited or partially-corrupted manifest
can deserialize with orphan recovered paths; rather than reject the
whole manifest (too strict on the upgrade path), we drop the orphans
and let the inconsistency self-correct on the next save(). is_recovered
then returns the truthful False for the orphan.
Tests: new test_load_filters_recovered_files_not_in_files asserting an
orphan recovered entry is dropped on load.
782 lines
32 KiB
Python
782 lines
32 KiB
Python
"""Tests for ClaudeIntegration."""
|
|
|
|
import codecs
|
|
import json
|
|
import os
|
|
from pathlib import Path
|
|
from unittest.mock import patch
|
|
|
|
import yaml
|
|
|
|
from specify_cli.integrations import INTEGRATION_REGISTRY, get_integration
|
|
from specify_cli.integrations.base import IntegrationBase, SkillsIntegration
|
|
from specify_cli.integrations.claude import ARGUMENT_HINTS
|
|
from specify_cli.integrations.manifest import IntegrationManifest
|
|
|
|
|
|
class TestClaudeIntegration:
|
|
def test_registered(self):
|
|
assert "claude" in INTEGRATION_REGISTRY
|
|
assert get_integration("claude") is not None
|
|
|
|
def test_is_base_integration(self):
|
|
assert isinstance(get_integration("claude"), IntegrationBase)
|
|
|
|
def test_config_uses_skills(self):
|
|
integration = get_integration("claude")
|
|
assert integration.config["folder"] == ".claude/"
|
|
assert integration.config["commands_subdir"] == "skills"
|
|
|
|
def test_registrar_config_uses_skill_layout(self):
|
|
integration = get_integration("claude")
|
|
assert integration.registrar_config["dir"] == ".claude/skills"
|
|
assert integration.registrar_config["format"] == "markdown"
|
|
assert integration.registrar_config["args"] == "$ARGUMENTS"
|
|
assert integration.registrar_config["extension"] == "/SKILL.md"
|
|
|
|
def test_context_file(self):
|
|
integration = get_integration("claude")
|
|
assert integration.context_file == "CLAUDE.md"
|
|
|
|
def test_setup_creates_skill_files(self, tmp_path):
|
|
integration = get_integration("claude")
|
|
manifest = IntegrationManifest("claude", tmp_path)
|
|
created = integration.setup(tmp_path, manifest, script_type="sh")
|
|
|
|
skill_files = [path for path in created if path.name == "SKILL.md"]
|
|
assert skill_files
|
|
|
|
skills_dir = tmp_path / ".claude" / "skills"
|
|
assert skills_dir.is_dir()
|
|
|
|
plan_skill = skills_dir / "speckit-plan" / "SKILL.md"
|
|
assert plan_skill.exists()
|
|
|
|
content = plan_skill.read_text(encoding="utf-8")
|
|
assert "{SCRIPT}" not in content
|
|
assert "{ARGS}" not in content
|
|
assert "__AGENT__" not in content
|
|
assert "__SPECKIT_COMMAND_" not in content, "unprocessed __SPECKIT_COMMAND_*__"
|
|
assert "/speckit." not in content, "skills agent must use /speckit-<name> not /speckit.<name>"
|
|
|
|
parts = content.split("---", 2)
|
|
parsed = yaml.safe_load(parts[1])
|
|
assert parsed["name"] == "speckit-plan"
|
|
assert parsed["user-invocable"] is True
|
|
assert parsed["disable-model-invocation"] is False
|
|
assert parsed["metadata"]["source"] == "templates/commands/plan.md"
|
|
|
|
def test_setup_upserts_context_section(self, tmp_path):
|
|
integration = get_integration("claude")
|
|
manifest = IntegrationManifest("claude", tmp_path)
|
|
integration.setup(tmp_path, manifest, script_type="sh")
|
|
|
|
ctx_path = tmp_path / integration.context_file
|
|
assert ctx_path.exists()
|
|
content = ctx_path.read_text(encoding="utf-8")
|
|
assert "<!-- SPECKIT START -->" in content
|
|
assert "<!-- SPECKIT END -->" in content
|
|
assert "read the current plan" in content
|
|
|
|
def test_upsert_context_section_strips_bom(self, tmp_path):
|
|
"""Existing context file with UTF-8 BOM must be cleaned up on upsert."""
|
|
integration = get_integration("claude")
|
|
ctx_path = tmp_path / integration.context_file
|
|
|
|
# Write a file that starts with a UTF-8 BOM (as the old PowerShell script did)
|
|
bom = codecs.BOM_UTF8
|
|
ctx_path.write_bytes(bom + b"# CLAUDE.md\n\nSome existing content.\n")
|
|
|
|
integration.upsert_context_section(tmp_path)
|
|
|
|
result = ctx_path.read_bytes()
|
|
assert not result.startswith(bom), "BOM must be stripped after upsert"
|
|
content = result.decode("utf-8")
|
|
assert "<!-- SPECKIT START -->" in content
|
|
assert "Some existing content." in content
|
|
|
|
def test_remove_context_section_strips_bom(self, tmp_path):
|
|
"""remove_context_section must clean BOM from context file on Windows-authored files."""
|
|
integration = get_integration("claude")
|
|
ctx_path = tmp_path / integration.context_file
|
|
|
|
marker_content = (
|
|
"# CLAUDE.md\n\n"
|
|
"<!-- SPECKIT START -->\n"
|
|
"For additional context about technologies to be used, project structure,\n"
|
|
"shell commands, and other important information, read the current plan\n"
|
|
"<!-- SPECKIT END -->\n"
|
|
)
|
|
ctx_path.write_bytes(codecs.BOM_UTF8 + marker_content.encode("utf-8"))
|
|
|
|
result = integration.remove_context_section(tmp_path)
|
|
|
|
assert result is True
|
|
assert ctx_path.exists(), "File should exist (non-empty content remains)"
|
|
remaining = ctx_path.read_bytes()
|
|
assert not remaining.startswith(codecs.BOM_UTF8), "BOM must be stripped after remove"
|
|
assert b"<!-- SPECKIT" not in remaining
|
|
assert b"# CLAUDE.md" in remaining
|
|
|
|
def test_ai_flag_auto_promotes_and_enables_skills(self, tmp_path):
|
|
from typer.testing import CliRunner
|
|
from specify_cli import app
|
|
|
|
project = tmp_path / "claude-promote"
|
|
project.mkdir()
|
|
old_cwd = os.getcwd()
|
|
try:
|
|
os.chdir(project)
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
app,
|
|
[
|
|
"init",
|
|
"--here",
|
|
"--ai",
|
|
"claude",
|
|
"--script",
|
|
"sh",
|
|
"--no-git",
|
|
"--ignore-agent-tools",
|
|
],
|
|
catch_exceptions=False,
|
|
)
|
|
finally:
|
|
os.chdir(old_cwd)
|
|
|
|
assert result.exit_code == 0, result.output
|
|
assert (project / ".claude" / "skills" / "speckit-plan" / "SKILL.md").exists()
|
|
assert not (project / ".claude" / "commands").exists()
|
|
|
|
init_options = json.loads(
|
|
(project / ".specify" / "init-options.json").read_text(encoding="utf-8")
|
|
)
|
|
assert init_options["ai"] == "claude"
|
|
assert init_options["ai_skills"] is True
|
|
assert init_options["integration"] == "claude"
|
|
|
|
def test_integration_flag_creates_skill_files(self, tmp_path):
|
|
from typer.testing import CliRunner
|
|
from specify_cli import app
|
|
|
|
project = tmp_path / "claude-integration"
|
|
project.mkdir()
|
|
old_cwd = os.getcwd()
|
|
try:
|
|
os.chdir(project)
|
|
runner = CliRunner()
|
|
result = runner.invoke(
|
|
app,
|
|
[
|
|
"init",
|
|
"--here",
|
|
"--integration",
|
|
"claude",
|
|
"--script",
|
|
"sh",
|
|
"--no-git",
|
|
"--ignore-agent-tools",
|
|
],
|
|
catch_exceptions=False,
|
|
)
|
|
finally:
|
|
os.chdir(old_cwd)
|
|
|
|
assert result.exit_code == 0, result.output
|
|
assert (project / ".claude" / "skills" / "speckit-specify" / "SKILL.md").exists()
|
|
assert (project / ".specify" / "integrations" / "claude.manifest.json").exists()
|
|
|
|
def test_interactive_claude_selection_uses_integration_path(self, tmp_path):
|
|
from typer.testing import CliRunner
|
|
from specify_cli import app
|
|
|
|
project = tmp_path / "claude-interactive"
|
|
project.mkdir()
|
|
old_cwd = os.getcwd()
|
|
try:
|
|
os.chdir(project)
|
|
runner = CliRunner()
|
|
with (
|
|
patch("specify_cli.commands.init._stdin_is_interactive", return_value=True),
|
|
patch("specify_cli.commands.init.select_with_arrows", return_value="claude"),
|
|
):
|
|
result = runner.invoke(
|
|
app,
|
|
[
|
|
"init",
|
|
"--here",
|
|
"--script",
|
|
"sh",
|
|
"--no-git",
|
|
"--ignore-agent-tools",
|
|
],
|
|
catch_exceptions=False,
|
|
)
|
|
finally:
|
|
os.chdir(old_cwd)
|
|
|
|
assert result.exit_code == 0, result.output
|
|
assert (project / ".specify" / "integration.json").exists()
|
|
assert (project / ".specify" / "integrations" / "claude.manifest.json").exists()
|
|
|
|
skill_file = project / ".claude" / "skills" / "speckit-plan" / "SKILL.md"
|
|
assert skill_file.exists()
|
|
skill_content = skill_file.read_text(encoding="utf-8")
|
|
assert "user-invocable: true" in skill_content
|
|
assert "disable-model-invocation: false" in skill_content
|
|
|
|
init_options = json.loads(
|
|
(project / ".specify" / "init-options.json").read_text(encoding="utf-8")
|
|
)
|
|
assert init_options["ai"] == "claude"
|
|
assert init_options["ai_skills"] is True
|
|
assert init_options["integration"] == "claude"
|
|
|
|
def test_claude_init_remains_usable_when_converter_fails(self, tmp_path):
|
|
"""Claude init should succeed even without install_ai_skills."""
|
|
from typer.testing import CliRunner
|
|
from specify_cli import app
|
|
|
|
runner = CliRunner()
|
|
target = tmp_path / "fail-proj"
|
|
|
|
result = runner.invoke(
|
|
app,
|
|
["init", str(target), "--ai", "claude", "--script", "sh", "--no-git", "--ignore-agent-tools"],
|
|
)
|
|
|
|
assert result.exit_code == 0
|
|
assert (target / ".claude" / "skills" / "speckit-specify" / "SKILL.md").exists()
|
|
|
|
def test_claude_hooks_render_skill_invocation(self, tmp_path):
|
|
from specify_cli.extensions import HookExecutor
|
|
|
|
project = tmp_path / "claude-hooks"
|
|
project.mkdir()
|
|
init_options = project / ".specify" / "init-options.json"
|
|
init_options.parent.mkdir(parents=True, exist_ok=True)
|
|
init_options.write_text(json.dumps({"ai": "claude", "ai_skills": True}))
|
|
|
|
hook_executor = HookExecutor(project)
|
|
message = hook_executor.format_hook_message(
|
|
"before_plan",
|
|
[
|
|
{
|
|
"extension": "test-ext",
|
|
"command": "speckit.plan",
|
|
"optional": False,
|
|
}
|
|
],
|
|
)
|
|
|
|
assert "Executing: `/speckit-plan`" in message
|
|
assert "EXECUTE_COMMAND: speckit.plan" in message
|
|
assert "EXECUTE_COMMAND_INVOCATION: /speckit-plan" in message
|
|
|
|
def test_claude_preset_creates_new_skill_without_commands_dir(self, tmp_path):
|
|
from specify_cli import save_init_options
|
|
from specify_cli.presets import PresetManager
|
|
|
|
project = tmp_path / "claude-preset-skill"
|
|
project.mkdir()
|
|
save_init_options(project, {"ai": "claude", "ai_skills": True, "script": "sh"})
|
|
|
|
skills_dir = project / ".claude" / "skills"
|
|
skills_dir.mkdir(parents=True, exist_ok=True)
|
|
|
|
preset_dir = tmp_path / "claude-skill-command"
|
|
preset_dir.mkdir()
|
|
(preset_dir / "commands").mkdir()
|
|
(preset_dir / "commands" / "speckit.research.md").write_text(
|
|
"---\n"
|
|
"description: Research workflow\n"
|
|
"---\n\n"
|
|
"preset:claude-skill-command\n"
|
|
)
|
|
manifest_data = {
|
|
"schema_version": "1.0",
|
|
"preset": {
|
|
"id": "claude-skill-command",
|
|
"name": "Claude Skill Command",
|
|
"version": "1.0.0",
|
|
"description": "Test",
|
|
},
|
|
"requires": {"speckit_version": ">=0.1.0"},
|
|
"provides": {
|
|
"templates": [
|
|
{
|
|
"type": "command",
|
|
"name": "speckit.research",
|
|
"file": "commands/speckit.research.md",
|
|
}
|
|
]
|
|
},
|
|
}
|
|
with open(preset_dir / "preset.yml", "w") as f:
|
|
yaml.dump(manifest_data, f)
|
|
|
|
manager = PresetManager(project)
|
|
manager.install_from_directory(preset_dir, "0.1.5")
|
|
|
|
skill_file = skills_dir / "speckit-research" / "SKILL.md"
|
|
assert skill_file.exists()
|
|
content = skill_file.read_text(encoding="utf-8")
|
|
assert "preset:claude-skill-command" in content
|
|
assert "name: speckit-research" in content
|
|
assert "user-invocable: true" in content
|
|
assert "disable-model-invocation: false" in content
|
|
|
|
metadata = manager.registry.get("claude-skill-command")
|
|
assert "speckit-research" in metadata.get("registered_skills", [])
|
|
|
|
|
|
class TestClaudeArgumentHints:
|
|
"""Verify that argument-hint frontmatter is injected for Claude skills."""
|
|
|
|
def test_all_skills_have_hints(self, tmp_path):
|
|
"""Every generated SKILL.md must contain an argument-hint line."""
|
|
i = get_integration("claude")
|
|
m = IntegrationManifest("claude", tmp_path)
|
|
created = i.setup(tmp_path, m, script_type="sh")
|
|
skill_files = [f for f in created if f.name == "SKILL.md"]
|
|
assert len(skill_files) > 0
|
|
for f in skill_files:
|
|
content = f.read_text(encoding="utf-8")
|
|
assert "argument-hint:" in content, (
|
|
f"{f.parent.name}/SKILL.md is missing argument-hint frontmatter"
|
|
)
|
|
|
|
def test_hints_match_expected_values(self, tmp_path):
|
|
"""Each skill's argument-hint must match the expected text."""
|
|
i = get_integration("claude")
|
|
m = IntegrationManifest("claude", tmp_path)
|
|
created = i.setup(tmp_path, m, script_type="sh")
|
|
skill_files = [f for f in created if f.name == "SKILL.md"]
|
|
for f in skill_files:
|
|
# Extract stem: speckit-plan -> plan
|
|
stem = f.parent.name
|
|
if stem.startswith("speckit-"):
|
|
stem = stem[len("speckit-"):]
|
|
expected_hint = ARGUMENT_HINTS.get(stem)
|
|
assert expected_hint is not None, (
|
|
f"No expected hint defined for skill '{stem}'"
|
|
)
|
|
content = f.read_text(encoding="utf-8")
|
|
assert f'argument-hint: "{expected_hint}"' in content, (
|
|
f"{f.parent.name}/SKILL.md: expected hint '{expected_hint}' not found"
|
|
)
|
|
|
|
def test_hint_is_inside_frontmatter(self, tmp_path):
|
|
"""argument-hint must appear between the --- delimiters, not in the body."""
|
|
i = get_integration("claude")
|
|
m = IntegrationManifest("claude", tmp_path)
|
|
created = i.setup(tmp_path, m, script_type="sh")
|
|
skill_files = [f for f in created if f.name == "SKILL.md"]
|
|
for f in skill_files:
|
|
content = f.read_text(encoding="utf-8")
|
|
parts = content.split("---", 2)
|
|
assert len(parts) >= 3, f"No frontmatter in {f.parent.name}/SKILL.md"
|
|
frontmatter = parts[1]
|
|
body = parts[2]
|
|
assert "argument-hint:" in frontmatter, (
|
|
f"{f.parent.name}/SKILL.md: argument-hint not in frontmatter section"
|
|
)
|
|
assert "argument-hint:" not in body, (
|
|
f"{f.parent.name}/SKILL.md: argument-hint leaked into body"
|
|
)
|
|
|
|
def test_hint_appears_after_description(self, tmp_path):
|
|
"""argument-hint must immediately follow the description line."""
|
|
i = get_integration("claude")
|
|
m = IntegrationManifest("claude", tmp_path)
|
|
created = i.setup(tmp_path, m, script_type="sh")
|
|
skill_files = [f for f in created if f.name == "SKILL.md"]
|
|
for f in skill_files:
|
|
content = f.read_text(encoding="utf-8")
|
|
lines = content.splitlines()
|
|
found_description = False
|
|
for idx, line in enumerate(lines):
|
|
if line.startswith("description:"):
|
|
found_description = True
|
|
assert idx + 1 < len(lines), (
|
|
f"{f.parent.name}/SKILL.md: description is last line"
|
|
)
|
|
assert lines[idx + 1].startswith("argument-hint:"), (
|
|
f"{f.parent.name}/SKILL.md: argument-hint does not follow description"
|
|
)
|
|
break
|
|
assert found_description, (
|
|
f"{f.parent.name}/SKILL.md: no description: line found in output"
|
|
)
|
|
|
|
def test_inject_argument_hint_only_in_frontmatter(self):
|
|
"""inject_argument_hint must not modify description: lines in the body."""
|
|
from specify_cli.integrations.claude import ClaudeIntegration
|
|
|
|
content = (
|
|
"---\n"
|
|
"description: My command\n"
|
|
"---\n"
|
|
"\n"
|
|
"description: this is body text\n"
|
|
)
|
|
result = ClaudeIntegration.inject_argument_hint(content, "Test hint")
|
|
lines = result.splitlines()
|
|
hint_count = sum(1 for ln in lines if ln.startswith("argument-hint:"))
|
|
assert hint_count == 1, (
|
|
f"Expected exactly 1 argument-hint line, found {hint_count}"
|
|
)
|
|
|
|
def test_inject_argument_hint_skips_if_already_present(self):
|
|
"""inject_argument_hint must not duplicate if argument-hint already exists."""
|
|
from specify_cli.integrations.claude import ClaudeIntegration
|
|
|
|
content = (
|
|
"---\n"
|
|
"description: My command\n"
|
|
'argument-hint: "Existing hint"\n'
|
|
"---\n"
|
|
"\n"
|
|
"Body text\n"
|
|
)
|
|
result = ClaudeIntegration.inject_argument_hint(content, "New hint")
|
|
assert result == content, "Content should be unchanged when hint already exists"
|
|
lines = result.splitlines()
|
|
hint_count = sum(1 for ln in lines if ln.startswith("argument-hint:"))
|
|
assert hint_count == 1
|
|
|
|
|
|
class TestClaudeDisableModelInvocation:
|
|
"""Verify disable-model-invocation is false for Claude skills."""
|
|
|
|
def test_setup_sets_disable_model_invocation_false(self, tmp_path):
|
|
"""Generated SKILL.md files must have disable-model-invocation: false."""
|
|
i = get_integration("claude")
|
|
m = IntegrationManifest("claude", tmp_path)
|
|
created = i.setup(tmp_path, m, script_type="sh")
|
|
skill_files = [f for f in created if f.name == "SKILL.md"]
|
|
assert len(skill_files) > 0
|
|
for f in skill_files:
|
|
content = f.read_text(encoding="utf-8")
|
|
parts = content.split("---", 2)
|
|
parsed = yaml.safe_load(parts[1])
|
|
assert parsed["disable-model-invocation"] is False, (
|
|
f"{f.parent.name}: expected disable-model-invocation: false"
|
|
)
|
|
|
|
def test_disable_model_invocation_not_true(self, tmp_path):
|
|
"""No Claude skill should have disable-model-invocation: true."""
|
|
i = get_integration("claude")
|
|
m = IntegrationManifest("claude", tmp_path)
|
|
created = i.setup(tmp_path, m, script_type="sh")
|
|
for f in created:
|
|
if f.name != "SKILL.md":
|
|
continue
|
|
content = f.read_text(encoding="utf-8")
|
|
assert "disable-model-invocation: true" not in content, (
|
|
f"{f.parent.name}: must not have disable-model-invocation: true"
|
|
)
|
|
|
|
def test_non_claude_agents_lack_disable_model_invocation(self, tmp_path):
|
|
"""Non-Claude skill agents should not get disable-model-invocation."""
|
|
from specify_cli.agents import CommandRegistrar
|
|
|
|
fm = CommandRegistrar.build_skill_frontmatter(
|
|
"codex", "speckit-plan", "desc", "templates/commands/plan.md"
|
|
)
|
|
assert "disable-model-invocation" not in fm
|
|
assert "user-invocable" not in fm
|
|
|
|
def test_skills_default_post_process_preserves_content_without_hooks(self, tmp_path):
|
|
"""SkillsIntegration agents without an override preserve non-hook content."""
|
|
# ``agy`` is a plain SkillsIntegration with no post-process override,
|
|
# so it stands in for the base-class default behavior.
|
|
agy = get_integration("agy")
|
|
if agy is None:
|
|
return # agy not registered in this build
|
|
content = "---\nname: test\n---\nBody"
|
|
assert agy.post_process_skill_content(content) == content
|
|
|
|
|
|
class TestClaudeHookCommandNote:
|
|
"""Verify dot-to-hyphen normalization note is injected in hook sections."""
|
|
|
|
def test_hook_note_injected_in_skills_with_hooks(self, tmp_path):
|
|
"""Skills that have hook sections should get the normalization note."""
|
|
i = get_integration("claude")
|
|
m = IntegrationManifest("claude", tmp_path)
|
|
i.setup(tmp_path, m, script_type="sh")
|
|
specify_skill = tmp_path / ".claude/skills/speckit-specify/SKILL.md"
|
|
assert specify_skill.exists()
|
|
content = specify_skill.read_text(encoding="utf-8")
|
|
# specify.md has hook sections
|
|
assert "replace dots" in content, (
|
|
"speckit-specify should have dot-to-hyphen hook note"
|
|
)
|
|
|
|
def test_hook_note_not_in_skills_without_hooks(self, tmp_path):
|
|
"""Skills without hook sections should not get the note."""
|
|
content = "---\nname: test\ndescription: test\n---\n\nNo hooks here.\n"
|
|
result = SkillsIntegration._inject_hook_command_note(content)
|
|
assert "replace dots" not in result
|
|
|
|
def test_hook_note_idempotent(self, tmp_path):
|
|
"""Injecting the note twice should not duplicate it."""
|
|
content = (
|
|
"---\nname: test\n---\n\n"
|
|
"- For each executable hook, output the following based on its flag:\n"
|
|
)
|
|
once = SkillsIntegration._inject_hook_command_note(content)
|
|
twice = SkillsIntegration._inject_hook_command_note(once)
|
|
assert once == twice, "Hook note injection should be idempotent"
|
|
|
|
def test_hook_note_fills_missing_repeated_instructions(self, tmp_path):
|
|
"""Already-noted hook sections should not suppress later sections."""
|
|
from specify_cli.integrations.base import _HOOK_COMMAND_NOTE
|
|
|
|
content = (
|
|
"---\nname: test\n---\n\n"
|
|
f"{_HOOK_COMMAND_NOTE}"
|
|
"- For each executable hook, output the following based on its flag:\n"
|
|
"\n"
|
|
" - For each executable hook, output the following based on its flag:\n"
|
|
)
|
|
result = SkillsIntegration._inject_hook_command_note(content)
|
|
assert result.count("replace dots (`.`) with hyphens") == 2
|
|
|
|
def test_hook_note_not_suppressed_by_unrelated_phrase(self, tmp_path):
|
|
"""Unrelated text should not trip the hook-note idempotence guard."""
|
|
content = (
|
|
"---\nname: test\n---\n\n"
|
|
"This paragraph says replace dots in a different context.\n"
|
|
"- For each executable hook, output the following based on its flag:\n"
|
|
)
|
|
result = SkillsIntegration._inject_hook_command_note(content)
|
|
assert "This paragraph says replace dots in a different context." in result
|
|
assert result.count("replace dots (`.`) with hyphens") == 1
|
|
|
|
def test_hook_note_preserves_indentation(self, tmp_path):
|
|
"""The injected note should match the indentation of the target line."""
|
|
content = (
|
|
"---\nname: test\n---\n\n"
|
|
" - For each executable hook, output the following\n"
|
|
)
|
|
result = SkillsIntegration._inject_hook_command_note(content)
|
|
lines = result.splitlines()
|
|
note_line = [line for line in lines if "replace dots" in line][0]
|
|
assert note_line.startswith(" "), "Note should preserve indentation"
|
|
|
|
def test_post_process_injects_all_claude_flags(self):
|
|
"""post_process_skill_content should inject all Claude-specific fields."""
|
|
i = get_integration("claude")
|
|
content = (
|
|
"---\nname: test\ndescription: test\n---\n\n"
|
|
"- For each executable hook, output the following\n"
|
|
)
|
|
result = i.post_process_skill_content(content)
|
|
assert "user-invocable: true" in result
|
|
assert "disable-model-invocation: false" in result
|
|
assert "replace dots" in result
|
|
|
|
|
|
class TestSpeckitManifestRecordsSkippedFiles:
|
|
"""Regression test for issue #2107.
|
|
|
|
``install_shared_infra`` must record every shared-infrastructure file
|
|
under ``.specify/`` in ``speckit.manifest.json``, including files that
|
|
were *skipped* because they already existed on disk and ``force=False``.
|
|
|
|
Before the fix, the skip branches in the scripts and templates loops
|
|
appended to ``skipped_files`` without calling ``manifest.record_existing``.
|
|
So when ``install_shared_infra`` ran with a fresh (or lost) manifest
|
|
against an already-populated ``.specify/`` tree, every file went down the
|
|
skip path, ``planned_copies`` and ``planned_templates`` stayed empty, and
|
|
``manifest.save()`` wrote an empty ``files`` field — leaving the
|
|
integration believing nothing was installed.
|
|
|
|
Reproduction (without the fix) using ``install_shared_infra`` directly:
|
|
|
|
install_shared_infra(p, "sh", ..., force=False) # 1st run → 10 files
|
|
(p / ".specify/integrations/speckit.manifest.json").unlink()
|
|
install_shared_infra(p, "sh", ..., force=False) # 2nd run → 0 files
|
|
# ^^ BUG: empty
|
|
"""
|
|
|
|
def _read_manifest_files(self, project_path: Path) -> dict:
|
|
manifest_path = (
|
|
project_path / ".specify" / "integrations" / "speckit.manifest.json"
|
|
)
|
|
assert manifest_path.exists(), (
|
|
f"speckit.manifest.json not written at {manifest_path}"
|
|
)
|
|
data = json.loads(manifest_path.read_text(encoding="utf-8"))
|
|
# ``IntegrationManifest.save`` serialises a ``files`` dict — assert
|
|
# the schema explicitly so a regression to a different key (e.g.
|
|
# the internal ``_files`` attribute name) fails loudly instead of
|
|
# being masked by a silent fallback.
|
|
assert isinstance(data, dict), (
|
|
f"manifest root is not a dict, got {type(data).__name__}"
|
|
)
|
|
assert "files" in data, (
|
|
f"manifest missing 'files' key, got keys: {sorted(data.keys())}"
|
|
)
|
|
files = data["files"]
|
|
assert isinstance(files, dict), (
|
|
f"manifest 'files' is not a dict, got {type(files).__name__}"
|
|
)
|
|
return files
|
|
|
|
def test_install_shared_infra_records_skipped_files(self, tmp_path):
|
|
"""With ``force=False`` and ``.specify/`` already populated, the
|
|
manifest must still record every file — the skip branches are not
|
|
allowed to drop files from the manifest."""
|
|
from rich.console import Console
|
|
from specify_cli.shared_infra import install_shared_infra
|
|
|
|
# Resolve the project's own packaged sources by walking up from this
|
|
# test file to the repo root (which contains ``scripts/`` and
|
|
# ``templates/`` that ``shared_scripts_source`` looks for).
|
|
repo_root = Path(__file__).resolve().parents[2]
|
|
console = Console(quiet=True)
|
|
|
|
# First run — fresh project, manifest gets populated normally.
|
|
install_shared_infra(
|
|
tmp_path,
|
|
"sh",
|
|
version="0.0.0",
|
|
core_pack=None,
|
|
repo_root=repo_root,
|
|
console=console,
|
|
force=False,
|
|
)
|
|
first_files = self._read_manifest_files(tmp_path)
|
|
assert first_files, "first install produced an empty manifest"
|
|
|
|
# Simulate a lost manifest while ``.specify/`` is still on disk
|
|
# (e.g. the manifest was deleted, corrupted, or the layout was
|
|
# extracted out-of-band).
|
|
manifest_path = (
|
|
tmp_path / ".specify" / "integrations" / "speckit.manifest.json"
|
|
)
|
|
manifest_path.unlink()
|
|
|
|
# Second run — every file already exists, so every iteration takes
|
|
# the skip branch. With the fix, those files are still recorded.
|
|
install_shared_infra(
|
|
tmp_path,
|
|
"sh",
|
|
version="0.0.0",
|
|
core_pack=None,
|
|
repo_root=repo_root,
|
|
console=console,
|
|
force=False,
|
|
)
|
|
second_files = self._read_manifest_files(tmp_path)
|
|
assert second_files, (
|
|
"speckit.manifest.json files dict is empty after install with "
|
|
"skipped files (issue #2107) — every file went down the skip "
|
|
"branch but none were recorded"
|
|
)
|
|
|
|
# The recovered manifest must cover everything the first run tracked.
|
|
missing = set(first_files) - set(second_files)
|
|
assert not missing, (
|
|
f"these files were tracked on the first install but missing after "
|
|
f"the skipped-files re-install: {sorted(missing)[:5]}"
|
|
)
|
|
|
|
def test_install_shared_infra_handles_directory_at_script_destination(
|
|
self, tmp_path
|
|
):
|
|
"""A non-file (directory) at a script's destination must NOT crash
|
|
``install_shared_infra`` and must NOT be recorded in the manifest —
|
|
the path still appears in the user-visible skipped-paths warning.
|
|
"""
|
|
from io import StringIO
|
|
from rich.console import Console
|
|
from specify_cli.shared_infra import install_shared_infra
|
|
|
|
repo_root = Path(__file__).resolve().parents[2]
|
|
output = StringIO()
|
|
console = Console(file=output, force_terminal=False, width=200)
|
|
|
|
# Pre-create the .specify/scripts/bash tree, then plant a directory
|
|
# where a script file is expected so the skip branch hits a
|
|
# non-regular-file path.
|
|
bash_dir = tmp_path / ".specify" / "scripts" / "bash"
|
|
bash_dir.mkdir(parents=True)
|
|
(bash_dir / "common.sh").mkdir() # collision: dir where file expected
|
|
|
|
# Must not crash.
|
|
install_shared_infra(
|
|
tmp_path,
|
|
"sh",
|
|
version="0.0.0",
|
|
core_pack=None,
|
|
repo_root=repo_root,
|
|
console=console,
|
|
force=False,
|
|
)
|
|
|
|
files = self._read_manifest_files(tmp_path)
|
|
assert ".specify/scripts/bash/common.sh" not in files, (
|
|
"directory at script dst must not be recorded in the manifest"
|
|
)
|
|
text = output.getvalue()
|
|
assert "common.sh" in text, (
|
|
"directory-at-script-dst path must surface in the skipped warning"
|
|
)
|
|
|
|
def test_install_shared_infra_handles_directory_at_template_destination(
|
|
self, tmp_path
|
|
):
|
|
"""Symmetric coverage for the templates loop: a directory at a
|
|
template's destination must NOT crash install nor be recorded."""
|
|
from io import StringIO
|
|
from rich.console import Console
|
|
from specify_cli.shared_infra import install_shared_infra
|
|
|
|
repo_root = Path(__file__).resolve().parents[2]
|
|
output = StringIO()
|
|
console = Console(file=output, force_terminal=False, width=200)
|
|
|
|
templates_dir = tmp_path / ".specify" / "templates"
|
|
templates_dir.mkdir(parents=True)
|
|
|
|
src_templates = repo_root / "templates"
|
|
real_template = next(
|
|
(
|
|
p.name
|
|
for p in src_templates.iterdir()
|
|
if p.is_file()
|
|
and not p.name.startswith(".")
|
|
and p.name != "vscode-settings.json"
|
|
),
|
|
None,
|
|
)
|
|
assert real_template, (
|
|
"no real template found in repo to collide against"
|
|
)
|
|
(templates_dir / real_template).mkdir() # collision
|
|
|
|
install_shared_infra(
|
|
tmp_path,
|
|
"sh",
|
|
version="0.0.0",
|
|
core_pack=None,
|
|
repo_root=repo_root,
|
|
console=console,
|
|
force=False,
|
|
)
|
|
|
|
files = self._read_manifest_files(tmp_path)
|
|
template_rel = f".specify/templates/{real_template}"
|
|
assert template_rel not in files, (
|
|
"directory at template dst must not be recorded in manifest"
|
|
)
|
|
text = output.getvalue()
|
|
assert real_template in text, (
|
|
"directory-at-template-dst path must surface in the skipped warning"
|
|
)
|